fix(bun): close late runtime review races - #3551
Conversation
Late review exposed Bun harness gaps that could let CI miss real failures or race live workspace setup: preload rewriting depended on platform-specific paths, the empty-selection runner used URL pathname decoding, and stale lock reclamation could disturb a newer owner. The change keeps the Bun gate fail-closed while making lock reclamation generation-aware. Constraint: Bun gate coverage must stay portable across POSIX and Windows-style paths. Constraint: Workspace package locks can be observed between directory creation and marker publication. Constraint: Validation is already green in the handoff lane; this commit records that evidence without rerunning or changing source. Rejected: Keep URL.pathname for runner self-spawns | breaks paths that need file URL decoding. Rejected: Reclaim markerless or malformed locks | races live lock initialization and erases indeterminate ownership. Rejected: Rewrite extension imports from test-file preload paths | conflates two distinct preload rewrite rules. Confidence: high Scope-risk: moderate Directive: Keep Bun empty-selection, preload rewriting, and workspace lock cleanup fail-closed across OS path spellings. Tested: deno task test:bun (16 harness tests plus 1297 files, 0 failed). Tested: Targeted Deno coverage for the late review changes. Tested: deno fmt --check. Tested: deno task lint:ci. Tested: deno check / typecheck suites. Tested: git diff --check. Tested: Mutation checks for each new regression guard.
|
Warning Review limit reached
Next review available in: 25 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: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR centralizes Bun preload rewriting, fixes Bun runner filesystem path handling, and hardens workspace preparation lock recovery with generation markers, atomic reclamation, tombstones, and concurrency tests. ChangesBun preload rewriting
Bun runner path handling
Workspace lock recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BunOnLoad
participant rewriteBunPreloadSource
participant rewriteModuleSpecifiers
BunOnLoad->>rewriteBunPreloadSource: path and source
rewriteBunPreloadSource->>rewriteModuleSpecifiers: extension source imports
rewriteBunPreloadSource-->>BunOnLoad: rewritten source
sequenceDiagram
participant PreparationProcessA
participant PreparationProcessB
participant reclaimStalePreparationLock
PreparationProcessA->>reclaimStalePreparationLock: claim stale lock generation
PreparationProcessB->>reclaimStalePreparationLock: claim stale lock generation
reclaimStalePreparationLock-->>PreparationProcessA: reclaimed generation tombstone
reclaimStalePreparationLock-->>PreparationProcessB: active-lock conflict
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/bun/preload-rewrite.ts`:
- Around line 16-18: Align the rewrite condition in the preload contents flow
with the filter’s extension-source predicate, limiting rewriting to paths
matching extensions/ext-*/src/* rather than every path containing /extensions/.
First add a focused regression test covering a .test.ts path under another
extensions subdirectory, then update the condition using the existing predicate
or equivalent shared logic.
In `@tests/bun/workspace-packages.mjs`:
- Around line 65-68: Update createPreparationLock so it creates and writes the
marker in a unique sibling temporary directory before publishing it via
renameSync to lockPath, making directory creation and marker presence atomic to
acquirers. If the final rename fails, remove the temporary directory and
propagate the rename error.
In `@tests/bun/workspace-packages.test.mjs`:
- Around line 224-227: Add a synchronization barrier to the two
runWorkspacePreparationChild flows after their liveness checks and before
renameSync, tracking both child arrivals and releasing them only once the parent
observes both have reached the barrier. Replace the fixed pause with this
coordination so the test deterministically exercises the delayed
stale-generation reader case.
🪄 Autofix
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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 71fed29e-1556-4184-a970-d5a6a95bbd5a
📒 Files selected for processing (6)
tests/bun/npm-protocol-resolution.test.tstests/bun/preload-rewrite.tstests/bun/preload.tstests/bun/runner-args.test.mjstests/bun/workspace-packages.mjstests/bun/workspace-packages.test.mjs
CI exposed that the lock-race child process inherits process.execPath from Deno during integration coverage, where Node's --input-type flag is not valid. The harness now follows the existing runtime-specific eval-args pattern so the child exercises the active runtime instead of assuming Node-style process flags. Constraint: Deno integration can execute this Bun harness with process.execPath pointing at Deno. Constraint: Node-compatible child evals still require the existing --input-type=module path. Rejected: Always spawn with Node-only --input-type flags | fails under Deno with a SyntaxError before the race guard runs. Rejected: Skip the race child outside Node | would leave the late Bun lock cleanup path unverified in Deno integration. Confidence: high Scope-risk: narrow Directive: Keep workspace lock race children aligned with the active runtime's eval argument shape. Tested: Full file Deno run, 10/10 passed sequentially. Tested: Full file Node run, 10/10 passed sequentially. Tested: Targeted Deno final green. Tested: Mutation check fails with the exact SyntaxError. Tested: deno fmt --check. Tested: deno task lint:ci. Tested: deno check / typecheck suites. Tested: git diff --check.
|
@codex review |
![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: b4b1ecba2c
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
1 participant
Summary
Closes the three late review findings posted while #3543 was entering the merge queue.
fileURLToPath()This supersedes #3548. The replacement uses permanent generation tombstones so a crashed reclaimer cannot wedge future generations and a delayed reader cannot move a later live lock.
Verification
deno task test:bun: 16 harness tests and 1297 Bun test files passeddeno task typecheck: passeddeno task lint:test-typecheck: 0 new failuresdeno task lint: passeddeno fmt --check,deno lint, anddeno check: passedgit diff --check: passedReview lineage
Follow-up to #3543. Supersedes the interim implementation in #3548.
Summary by CodeRabbit
Bug Fixes
Tests