fix(storage): await Worker join in test beforeEach under isolate - #827
fix(storage): await Worker join in test beforeEach under isolate#827Wibias wants to merge 6 commits into
Conversation
Sync resets were fire-and-forget terminating storage Workers between cases, leaving workers_spawned(N) workers_terminated(N-1) for Windows bun test --isolate. Suites now await async reset + drain, with a mutation-race-style regression.
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change makes storage test setup and teardown await reset operations, drain workers, and coordinate server shutdown. Policy API harness installation is asynchronous. Isolation tests validate repeated cleanup cycles. The Windows worker join delay increases to 750 ms. ChangesStorage worker lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/storage/restore-job.ts`:
- Around line 77-80: The reset operations clear shared coordination state before
storage workers finish, so move them until after worker termination, draining,
and asynchronous policy reset complete. In src/storage/restore-job.ts lines
77-80, update the reset flow around resetRestoreTrashJobForTestsAsync,
terminateStorageWorker, and drainStorageWorkers so
resetStorageMutationCoordinatorForTests runs in a finally block after cleanup.
In tests/storage-mutation-race.test.ts lines 166-170 and 174-177, move
resetArchivedCleanupJobForTests after the asynchronous policy reset and final
worker drain in both setup and teardown.
In `@tests/helpers/storage-policy-api.ts`:
- Around line 113-124: Make installPolicyApiHarness drain and reset existing
storage workers before allocating temporary homes or modifying OPENCODEX_HOME,
and wrap setup so any rejection cleans up resources and restores the prior
environment before rethrowing. Update uninstallPolicyApiHarness so
resetStorageCleanupPolicyJobForTestsAsync and drainStorageWorkers cannot bypass
restoration and temporary-directory removal; place teardown cleanup in a finally
block while preserving normal behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5fd1451f-a2c2-48c8-8f80-185c97d4e601
📒 Files selected for processing (12)
src/storage/policy-job.tssrc/storage/restore-job.tstests/api-storage-policy-already-running.test.tstests/api-storage-policy-mutation-busy.test.tstests/api-storage-policy-put-race.test.tstests/api-storage-policy-run.test.tstests/api-storage-policy.test.tstests/helpers/storage-policy-api.tstests/storage-mutation-race.test.tstests/storage-policy-job-responsive.test.tstests/storage-restore-job-responsive.test.tstests/storage-worker-teardown-isolate.test.ts
Bare server.stop left the policy scheduler armed and skipped the Windows Worker settle path, so bun test --isolate still panicked with workers_spawned(N) workers_terminated(N-1) under CI load.
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
The Windows CI result is green and the async-join direction is valuable, but two lifecycle issues remain on current head 10ab2e8. (1) restore-job and mutation-race reset shared coordination/archive state before worker termination and the final drain finish, allowing the next test run to acquire a cleared slot while an old worker can still mutate CODEX_HOME; move those resets after cleanup in finally. (2) installPolicyApiHarness mutates OPENCODEX_HOME and allocates temp homes before the first awaited reset, while install/uninstall lack finally-based restoration, so a rejected reset/drain leaks environment and directories. Keep this draft until both order and exception-safety paths are fixed and covered.
Owner review on lidge-jun#827: keep the coordinator/archive slot held until terminate+drain finish, and make the policy API harness drain before OPENCODEX_HOME mutation with finally-based env/temp restore.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc08831ac5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| export { fetch, startServer, setStorageCleanupPolicyJobTestHooks, setArchivedCleanupJobTestHooks, stopStorageCleanupScheduler, resetStorageCleanupPolicyJobForTestsAsync }; | ||
| /** Prefer over Bun.serve.stop — joins Workers and clears the policy scheduler. */ | ||
| export async function stopPolicyServer(server: ReturnType<typeof startServer>): Promise<void> { | ||
| await drainAndShutdown(server, 5_000); |
There was a problem hiding this comment.
When these suites reach an isolate boundary, stopPolicyServer() can return before the listener has actually closed: drainAndShutdown() invokes s?.stop(true) at src/server/lifecycle.ts:124 without awaiting the Promise<void> returned by Bun's Server.stop. Replacing the previous await server.stop(true) therefore leaves asynchronous server teardown racing the next test or realm reclamation, undermining the Windows teardown fix. Await s.stop(true) inside drainAndShutdown (or explicitly await it here) before returning.
AGENTS.md reference: AGENTS.md:L157-L159
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed on tip: drainAndShutdown now awaits s.stop(true) so Bun's Promise<void> close finishes before return (same contract as the previous await server.stop(true) call sites).
stopPolicyServer switched isolate suites onto drainAndShutdown, which called stop without awaiting Promise<void> and raced the next realm.
Sync reset/abort released the mutation slot without bumping the spawn-gate epoch, so a gated Worker could still start after coordination was torn down.
2 participants
Summary
beforeEach(fire-and-forget terminate raced the next spawn under Windowsbun test --isolate).installPolicyApiHarness/ mutation-race / responsive suites await*ForTestsAsync+drainStorageWorkerson setup and teardown.storage-worker-teardown-isolate.test.ts.Follow-up to #813 after #653 Windows CI still hit
workers_spawned(11) workers_terminated(10)mid-storage-mutation-raceon tip that already included #813.Test plan
bun testfocused storage isolate/mutation/policy suites (22 pass)windows-latestwithbun test --isolate tests)Summary by CodeRabbit
Bug Fixes
Tests
Documentation