fix(ci): harden Bun isolate worker teardown and probe timeouts - #1179
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe worker lifecycle now exports platform-specific OS-join settle durations and applies them during termination. Tests cover Windows, macOS, Linux, and FreeBSD. A child-process probe test now has bounded execution and explicit spawn-error handling. ChangesWorker OS-join settling
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a53163f0b1
ℹ️ 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".
| const result = spawnSync(process.execPath, [probe, ...args], { | ||
| encoding: "utf8", | ||
| env, | ||
| timeout: PROBE_TIMEOUT_MS, |
There was a problem hiding this comment.
Give the Bun child a contention-safe deadline
On contended cross-platform runners, this 3-second ceiling can turn a healthy cold Bun launch into ETIMEDOUT, and line 46 then fails all three transport tests before they exercise the behavior. The Windows CI notes in tests/codex-catalog-restore.test.ts:41-42 record the same spawnSync(bun ...) pattern taking about 5.4 seconds under load, while tests/helpers/test-budget.ts:33-34 assigns real child processes a 45-second budget. Keep the subprocess bounded, but use a deadline with CI-contention headroom (and an appropriate outer test budget) rather than 3 seconds.
Useful? React with 👍 / 👎.
|
✅ Deterministic PR hygiene checks passed. |
Summary
Failure observed
Linux shard
test 3/4repeatedly fails under Bun 1.3.14 with two linked symptoms:tests/api-storage-policy-already-running.test.tshits an unhandled Bun runtime failure while creating a stream around Worker activity:Bun then reports test-runner corruption (
Cannot call beforeEach() after the test run has completed) but continues into later files.tests/claude-dotenv-provenance-transport.test.ts, its unboundedspawnSync()child can wedge. The log showskilled 1 dangling process, followed by no progress until the 15-minute job timeout.The repository already documents Bun 1.3.14 Worker-reclaim crashes on Ubuntu, but
worker-lifecycle.tsonly applied its post-close OS-join settle on Windows/macOS. This PR closes that Linux gap and independently makes the Claude subprocess fail boundedly instead of hanging the whole shard.Test coverage
test 3/4hang is eliminatedScope
This is intentionally separate from #1175; it changes only CI/runtime hardening for the pre-existing Bun isolate failure and subprocess hang.
Summary by CodeRabbit
Bug Fixes
Tests