fix(storage): await Worker join in test beforeEach under isolate - #827
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change makes storage test setup and teardown await reset operations, drain workers, and coordinate server shutdown. Policy API harness installation is asynchronous. Worker termination uses platform-specific settle periods. Isolation tests validate repeated cleanup cycles. ChangesStorage worker lifecycle
Estimated code review effort: 3 (Moderate) | ~25 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".
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.
Windows Cross-platform timed out the 5s waitFor ceiling under suite load; the waits only assert eventual fetch, not latency.
Align mutation-race and responsive suite beforeEach with installPolicyApiHarness so leftover Workers join before OPENCODEX_HOME mutation.
macOS CI segfaulted in storage-worker-teardown-isolate after terminate with balanced worker counts; add a short post-close settle and keep darwin churn to one cycle.
Bun 1.3.14 macOS Silicon still segfaults mid-file after balanced workers_spawned/terminated in storage-worker-teardown-isolate; keep win32/linux coverage and disarm terminate timeout before OS-join settle.
Keep tip's await of Bun Server.stop inside the upstream finally shell shutdown path so isolate suites still join the listener before return.
Bun's default 5s per-test budget cut waitFor short on busy ubuntu CI even after the 15s waitFor ceiling (lidge-jun#827).
Post state-store merge, windows-latest under bun test --isolate was killed at the 20m ceiling mid-suite on lidge-jun#827 while still green.
Matches the ci.yml bump after Windows hit the 20m kill on lidge-jun#827.
Tip 30bd1f2 still cancelled mid-Test at the 30m ceiling (~29m elapsed) before GUI steps on lidge-jun#827.
unref'd waitMs timers could fail to fire under bun test --isolate while the head mutation held a Promise, hanging oauth-store-multi for 20+ min.
The ~29m cancel was a hung oauth mutation waiter under isolate, not suite growth that needs a higher ceiling. Fix the hang; do not absorb it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/ci-workflows.test.ts`:
- Around line 34-37: Update the timeout assertions in the CI workflow test to
parse the workflow jobs with Bun.YAML.parse and verify the job-specific values
directly: jobs.test["timeout-minutes"] must be 30 and
jobs["npm-global-smoke"]["timeout-minutes"] must be 8. Replace the broad
count-based checks while preserving the existing assertion for the 45-minute
cross-platform test job if applicable.
🪄 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: 3c80c58e-ab61-4608-9532-cc6a98096fbe
📒 Files selected for processing (9)
.github/workflows/ci.ymlgui/tests/client-resource-poll.test.tsxsrc/server/lifecycle.tssrc/storage/policy-job.tssrc/storage/restore-job.tssrc/storage/worker-lifecycle.tstests/ci-workflows.test.tstests/storage-mutation-race.test.tstests/storage-worker-teardown-isolate.test.ts
…solate-beforeeach
Global timeout-minutes counts still passed if 30 and 8 were swapped between test and npm-global-smoke.
Bun 1.3.14 also segfaults this file mid-suite on ubuntu GHA with balanced worker counts after the first green assertion. Keep the regression on win32.
Pick up kill-grace keep-ref and OAuth queue CI fixes from tip; keep this PR's 30m Windows test ceiling and job-scoped timeout asserts with the new select-windows-runner job.
Blanket timeout stubs passed on Linux because atomicWriteFile skips ACL when process.platform is not win32, but crashed startServer on Windows CI after the isolate suite finally reached these cases.
Keep 750ms Windows Worker OS-join settle and non-Windows hammer skips for Bun 1.3.14 isolate segfaults; take tip's scoped management-auth icacls fakes.
[shipping-github] Verdict: changes-requestedPR: UsefulnessReal Windows Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
Gatehard blockers for merge-ready /
Bottom lineShip direction is sound and Windows isolate CI is green under the 30m ceiling, but GitHub review state is not merge-clean yet: need owner re-approval and the Codex thread resolved. No further product fixes pending for the stated isolate panic. |
[shipping-github] Merging (override)Merging with explicit merge anyway despite stale owner Why it helps: Windows |
Resolve oauth wait-timer, oauth-store-multi deadline, and management-auth ACL stub conflicts on top of the landed storage-worker isolate beforeEach work.
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