shell(rm): close the DirTask parent/child hand-off lost-wakeup#34032
shell(rm): close the DirTask parent/child hand-off lost-wakeup#34032robobun wants to merge 5 commits into
Conversation
The recursive rm hand-off in remove_entry_dir did
if subtask_count.load() > 1 { need_to_wait.store(true); return waiting }
while a finishing child in post_run did
tasks_left = parent.subtask_count.fetch_sub(1);
if parent.need_to_wait.load() && tasks_left == 2 { take over parent }
If the last child slipped between the parent's load and store it would
decrement, read need_to_wait == false, and skip; the parent then stored
true and returned waiting == true, so run_from_thread_pool_impl skipped
post_run too. Nothing ever drove delete_after_waiting_for_children on
the parent, and the ShellRmTask (and so the shell promise) never
completed. This is the "pre-existing few-instruction window" called out
as out of scope in 85ef0c5.
Fix: the parent now publishes need_to_wait first, then releases its own
slot on subtask_count with the same fetch_sub the children use. Whoever
takes the counter to 0 owns the rmdir. If the parent wins it restores
the invariants post_run expects and falls through to the inline delete;
if a child wins it calls delete_after_waiting_for_children, which now
restores subtask_count = 1 before re-running post_run.
|
Updated 6:37 AM PT - Jul 12th, 2026
❌ @robobun, your commit 100760e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34032That installs a local version of the PR into your bun-34032 --bun |
|
Warning Review limit reached
Next review available in: 1 minute 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 Run ID: 📒 Files selected for processing (2)
WalkthroughChangesThe recursive Recursive rm hand-off
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 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 `@test/js/bun/shell/commands/rm.test.ts`:
- Around line 161-163: Change the temporary-directory cleanup registered near
base in the rm handoff test from the beforeExit event to the exit event,
preserving the synchronous rmSync call and its recursive/force options so
cleanup runs on process.exit(1) paths as well.
🪄 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
Run ID: 048e63e5-4a5b-4f69-a07d-eedd589b5c4f
📒 Files selected for processing (2)
src/runtime/shell/builtin/rm.rstest/js/bun/shell/commands/rm.test.ts
beforeExit does not fire on the process.exit(1) failure paths.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/js/bun/shell/commands/rm.test.ts (2)
9-9: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUse the harness-managed
tempDirfor the fixture root.The test creates a temporary directory with
fs.mkdtempSync()inside the spawned fixture, bypassing the repository’s harness cleanup contract. Create it withusing base = tempDir("rm-handoff", {})in the outer test and inject the path into the fixture.Also applies to: 157-161
🤖 Prompt for 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. In `@test/js/bun/shell/commands/rm.test.ts` at line 9, Update the rm handoff test to import and use the harness-managed tempDir, creating the fixture root in the outer test with using base = tempDir("rm-handoff", {}). Inject base’s path into the spawned fixture and remove the inner fs.mkdtempSync() creation, preserving the existing fixture behavior and cleanup contract.Sources: Coding guidelines, Learnings
178-181: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRemove the wall-clock watchdog and custom test timeout.
Bun.sleep(10_000)and120_000make the test depend on elapsed time and violate the repository’s no-timeout test guidance. Await the rm operations directly and rely on Bun’s test-runner timeout, or use an established cancellable watchdog without overriding the test timeout.Also applies to: 209-209
🤖 Prompt for 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. In `@test/js/bun/shell/commands/rm.test.ts` around lines 178 - 181, Remove the Promise.race wall-clock watchdogs using Bun.sleep from the rm tests around the rm command invocations, and await the rm operations directly. Remove the custom 120_000 test timeout as well, relying on Bun’s test-runner timeout while preserving the existing assertions and coverage.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/bun/shell/commands/rm.test.ts`:
- Line 9: Update the rm handoff test to import and use the harness-managed
tempDir, creating the fixture root in the outer test with using base =
tempDir("rm-handoff", {}). Inject base’s path into the spawned fixture and
remove the inner fs.mkdtempSync() creation, preserving the existing fixture
behavior and cleanup contract.
- Around line 178-181: Remove the Promise.race wall-clock watchdogs using
Bun.sleep from the rm tests around the rm command invocations, and await the rm
operations directly. Remove the custom 120_000 test timeout as well, relying on
Bun’s test-runner timeout while preserving the existing assertions and coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a0a8efe9-b166-42f0-8c44-064f6a677a53
📒 Files selected for processing (1)
test/js/bun/shell/commands/rm.test.ts
The outer test now owns the temporary directory and injects its path into the spawned fixture, so cleanup goes through the harness instead of an exit handler in the subprocess.
|
On the two out-of-diff review notes:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…mment - Replace per-rm Bun.sleep() with one clearable setTimeout per iteration so the subprocess exits immediately instead of waiting out the last batch of ref'd timers (~10 s shaved off every run). - Add .nothrow() so a nonzero rm exit reaches the diagnostic branch instead of rejecting with ShellError. - Drop the parenthetical from the delete_after_waiting_for_children comment; the restore services post_run's own decrement, it does not make the (pre-existing, double-TOCTOU-only) re-enqueue path participate in the hand-off.
There was a problem hiding this comment.
No issues found after the fixture fixes in 100760e, but this reworks a lock-free cross-thread ownership hand-off over raw *mut DirTask pointers, so it's worth a human pass on the atomic protocol.
Checked: the whoever-hits-0 protocol — parent fetch_sub==1 restore path (all children saw ≥2 so none claimed ownership; no late decrements possible), the !=1 path treats dir_task as freed (only the stack-local fd scopeguard runs after). The store(1) in delete_after_waiting_for_children correctly re-arms post_run's own decrement. Zero-children / error_signal-aborted-enqueue cases fall through to the inline delete as before. Test fixture: watchdog is now clearable, .nothrow() makes the diagnostic branch live, tempDir owns cleanup.
Extended reasoning...
Overview
Reworks the parent/child DirTask join in src/runtime/shell/builtin/rm.rs from check-then-publish (load > 1 → store(true)) to a unified SeqCst fetch_sub on subtask_count where whoever takes the counter to 0 owns the parent's rmdir. Touches remove_entry_dir (parent side), post_run (child side), delete_after_waiting_for_children (restore subtask_count=1), and updates the three SAFETY/doc comments describing the release/acquire hand-off. Adds an 800-iteration stress probe to rm.test.ts.
Security risks
None identified. This is internal task-tree lifecycle synchronization for the shell rm builtin; no user-controlled input reaches the changed lines beyond what already flowed through the pre-existing code.
Level of scrutiny
High. This is unsafe Rust manipulating raw *mut DirTask across worker threads, where the correctness of every &*dir_task dereference after the hand-off depends on the atomic protocol. The pre-existing race it closes was a few-instruction window that took CI-scale repetition on arm64 to surface; a newly-introduced window of similar width would be equally hard to catch by test. The PR description's interleaving analysis is thorough and I traced the three cases (parent wins, child wins, no children) without finding a hole, but lock-free protocol changes of this shape are exactly where a second human reading is most valuable.
Other factors
- Prior inline nits (uncancelled
Bun.sleepwatchdog adding ~10s dead time, unreachabler !== 0branch,beforeExitvsexitcleanup, misleading comment parenthetical about the re-enqueue path) were all addressed in 406e504/7dc9191d/100760e0. - The author explicitly could not reproduce the original race on the linux-x64 gate host (64k iterations clean), so the new test is a probe rather than a fail-before; the CI build links in the description are the actual evidence.
- A pre-existing theoretical strand on the non-Linux
remove_entry_dir_after_childrenre-enqueue path (double-TOCTOU) was noted and intentionally left out of scope; the comment was tightened so it no longer implies that path participates in the new protocol.
|
CI status for build 72199 (100760e):
This diff is ready for a look at the atomic protocol per the review summary. The hand-off race itself is not fail-before-provable on the x64 gate host (64 000-iteration probe ran clean; see the PR body), so the gate cannot mechanically demonstrate it. |
1 participant
What does this PR do?
Closes the lost-wakeup race in the shell
rm -rfDirTask parent/child join that was explicitly called out as out of scope in #32617 (85ef0c5):Mechanism
remove_entry_dirended withwhile a finishing child in
post_rundidWhen the last child slipped between the parent's
loadandstore: the child decremented (tasks_left == 2), readneed_to_wait == false, and skippeddelete_after_waiting_for_children; the parent then storedtrueand returnedwaiting == true, sorun_from_thread_pool_implskippedpost_runtoo. The parent DirTask was stranded withsubtask_count == 1, need_to_wait == true,finish_concurrentlynever fired, and the shell promise never resolved.A directory containing exactly one subdirectory (e.g. the
foo/bartree intest/js/bun/shell/leak.test.ts'sfdleak_rm) is the minimal trigger.Fix
The parent now publishes
need_to_wait = truefirst, then releases its own slot onsubtask_countwith the samefetch_subthe children use: whoever takes the counter to 0 owns the rmdir. If the parent'sfetch_subreturns 1, every child has already released (and each saw the counter > 1, so none took ownership); the parent restoressubtask_count = 1/need_to_wait = falseand falls through to the inline delete sopost_runruns as before. Otherwise a child still holds a count and the child taking it to 0 drivesdelete_after_waiting_for_children, which now also restoressubtask_count = 1sopost_run's own decrement (and any re-enqueue fromremove_entry_dir_after_children) starts from the expected value.The child side drops the separate
need_to_waitload: the SeqCst RMW chain onsubtask_countalready provides the release/acquire that makes the parent'sdeleted_entrieswrites visible to the winner.Why this surfaced as a
leak.test.tsredfd leak > fdleak_rmruns 100 iterations ofmkdir foo; touch ...; mkdir foo/bar; touch ...; rm -rfv foo/in a subprocess with no per-iteration timeout; when onermhit this race the subprocess hung, and the outer test timed out at 100 s. Recent hits on PR builds off current main:fdleak_rm100 s timeout (went red because earlier fixtures' crash reports were attributed here andisAlwaysFailureblocked the per-file retry)fdleak_rm100 s timeoutmemleak_*iteration timed out at 5 s mid-runmemleak_*iteration timed outfdleak_rm100 s timeoutHow did you verify your code works?
bun bd test test/js/bun/shell/commands/rm.test.ts(6 pass),bunshell.test.ts(399 pass),leak.test.tsrm/ls cases pass.bun run rust:check-allclean across all targets.rm.test.tsthat runs 800 concurrentrm -rfvof the minimal one-subdir tree with a 10 s per-call watchdog.Fail-before
This race is a few-instruction interleaving and I could not trigger it on the linux-x64 gate host: a 64 000-iteration probe (
rm -rfof the minimal tree, 32-wide) against the unfixed release binary ran to completion without a hang. The CI hits above are the fail-before evidence; the new test is a probe that should catch any future regression on the arm64 lanes where scheduling makes the window observable.no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/shell/leak.test.ts test/js/bun/shell/commands/rm.test.ts