Skip to content

fix(shutdown): make runCleanup idempotent under concurrent calls (LIB-HIGH-003)#431

Merged
ndycode merged 1 commit intomainfrom
fix/shutdown-cleanup-ordering
Apr 18, 2026
Merged

fix(shutdown): make runCleanup idempotent under concurrent calls (LIB-HIGH-003)#431
ndycode merged 1 commit intomainfrom
fix/shutdown-cleanup-ordering

Conversation

@ndycode
Copy link
Copy Markdown
Owner

@ndycode ndycode commented Apr 18, 2026

Addresses the actionable concurrency part of LIB-HIGH-003 from the deep top-level lib audit.

The audit identified three shutdown concerns:

  • cleanup ordering
  • concurrent runCleanup() races
  • signal / beforeExit semantics

This PR intentionally fixes the smallest safe slice only: overlapping runCleanup() calls now share one in-flight promise so cleanup handlers are not double-run.

It preserves the current FIFO cleanup order and current signal / beforeExit behavior to avoid a broader shutdown-contract change in the same PR.

Includes a regression test proving concurrent callers receive the same in-flight promise and the cleanup handler runs once.

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

makes runCleanup() idempotent by tracking an in-flight cleanupPromise at module scope; concurrent callers receive the same promise and cleanup handlers run exactly once, with .finally() resetting the guard after all awaiters are notified. the implementation is correct — the .finally() reset fires before awaiting callers resume, so there are no hidden races in the node.js microtask ordering.

Confidence Score: 5/5

safe to merge — implementation is correct and the single P2 finding is a test coverage gap, not a production defect

the concurrency fix is sound: module-level promise guard deduplicates concurrent calls, .finally() reset fires in the correct microtask order before callers resume, and the existing test suite is green. the only gap is missing assertions for the post-completion reset path in the new regression test, which is a style/coverage issue, not a functional bug.

test/shutdown.test.ts — the new concurrent test lacks post-completion assertions for the .finally() reset branch

Important Files Changed

Filename Overview
lib/shutdown.ts adds module-level cleanupPromise guard; concurrent calls return the same in-flight promise; .finally() reset is correctly ordered (fires before awaiters resume); logic is sound
test/shutdown.test.ts adds concurrent-call regression test; proves in-flight deduplication and single handler invocation, but lacks post-completion reset assertions and has a test-isolation fragility if assertions fail before release()

Sequence Diagram

sequenceDiagram
    participant C1 as Caller 1
    participant C2 as Caller 2
    participant RC as runCleanup()
    participant FN as cleanupFn

    C1->>RC: runCleanup()
    RC->>RC: cleanupPromise = null → create IIFE
    RC-->>C1: return cleanupPromise (P)
    C2->>RC: runCleanup()
    RC->>RC: cleanupPromise !== null → early return
    RC-->>C2: return same cleanupPromise (P)
    RC->>FN: await fn() [once]
    FN-->>RC: resolves
    RC->>RC: .finally() → cleanupPromise = null
    RC-->>C1: P resolves
    RC-->>C2: P resolves
Loading

Fix All in Codex

Prompt To Fix All With AI
This is a comment left during a code review.
Path: test/shutdown.test.ts
Line: 68-86

Comment:
**missing post-completion assertions**

the test proves in-flight deduplication but stops before checking the `finally` reset path. after `await first`, `cleanupPromise` should be `null` and a new `runCleanup()` call should return a **different** promise. without asserting this, the `.finally(() => { cleanupPromise = null; })` branch has no direct coverage.

additionally, if any `expect` fails before `release()` is called, the blocker never resolves and `beforeEach(async () => { await runCleanup(); })` in the next test will hang until vitest's per-test timeout fires. consider adding a cleanup via `afterEach` or using vitest's `onTestFinished`:

```typescript
release();
await first;

// verify reset: a post-cleanup call should be a fresh promise
const third = runCleanup();
expect(third).not.toBe(first);
await third; // resolves instantly (cleanupFunctions is empty)

// cleanupFn should still have run exactly once total
expect(cleanupFn).toHaveBeenCalledTimes(1);
```

How can I resolve this? If you propose a fix, please make it concise.

Reviews (1): Last reviewed commit: "fix(shutdown): make runCleanup idempoten..." | Re-trigger Greptile

…-HIGH-003)

Addresses the actionable part of LIB-HIGH-003 from the deep top-level lib audit.

The original finding mixed three concerns: ordering, idempotence, and signal/
beforeExit semantics. The smallest safe fix is to eliminate the actual race:
concurrent runCleanup() calls can currently double-run cleanup handlers.

Fix: cache the in-flight cleanup promise and return it to overlapping callers.
Preserve current FIFO ordering and current signal/beforeExit semantics to avoid
changing the shutdown contract in this PR. Added a focused regression test that
concurrent callers receive the same in-flight promise and only one cleanup run
executes.
@chatgpt-codex-connector
Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 18, 2026

Warning

Rate limit exceeded

@ndycode has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 20 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 20 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 393eb4de-1fd9-4250-ae2b-6ef77d23bbcd

📥 Commits

Reviewing files that changed from the base of the PR and between 3f1c1fe and 8aad2a7.

📒 Files selected for processing (2)
  • lib/shutdown.ts
  • test/shutdown.test.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/shutdown-cleanup-ordering
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch fix/shutdown-cleanup-ordering

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ndycode ndycode merged commit fd4d36d into main Apr 18, 2026
1 of 2 checks passed
ndycode added a commit that referenced this pull request Apr 18, 2026
Merge the reviewed rollup fix branch after deep audit, full verification battery, and real pack/install smoke test.

This lands the integrated fix set from PRs #414-#429 and #431 via one reviewed branch, including the two rollup-only integration commits:
- 7f50455 fix(rollup): add missing numeric tracker cleanup helpers from PR #419
- 056ad18 test(rollup): restore auth-list empty-state expectation to current behavior

Rationale: the rollup branch was the only branch deep-audited end-to-end and pack/install smoke-tested as a complete artifact. Baseline failures in codex-bin-wrapper / benchmark-runtime-path-script already exist on main and are not regressions from this merge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant