Don't block worker/VM teardown on fs thread-pool ops that never complete - #37170
Don't block worker/VM teardown on fs thread-pool ops that never complete#37170robobun wants to merge 6 commits into
Conversation
A pending node:fs thread-pool operation whose syscall can block forever (a read from a FIFO or pipe with no writer, idle stdin, a hung mount) made worker.terminate() hang and spin a core: WebWorker::shutdown waited on wait_for_concurrent_posters(), a yield loop over a counter that bracketed the entire fs operation, so the wait was bounded by syscall latency only for syscalls that finish. The same wait hung process exit under BUN_DESTRUCT_VM_ON_EXIT=1. Replace the whole-operation counter with ConcurrentPosterGate, an Arc-shared state word that brackets only the completion post itself. Shutdown closes the gate before the final queue drain: posts already mid-enqueue are waited out (bounded by an enqueue + wakeup), later posts are refused. A refused poster still owns its task and frees it on the pool thread without touching the torn-down VM: JS handles (the promise Strong, protect counts) died with the heap and are not released, while owned Rust payloads are freed via the new FsReturn::fs_discard hook and ThreadSafe::dispose_skip_unprotect. fs_discard in destroy() also stops the shutdown drain from leaking readFile/readdir buffers whose bytes only a to_js() conversion used to free. The gate is shared via Arc clones held by each in-flight task, so a pool thread stranded in a blocked syscall can still observe the closed state after the VM's memory is gone; the last clone frees it.
|
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 (7)
WalkthroughThe change replaces concurrent-poster counters with shared gates. VM and worker shutdown close these gates before draining tasks. Filesystem completions reject late posts and dispose pending payloads without accessing destroyed JavaScript state. Windows tests cover blocked and late completions. ChangesConcurrent poster shutdown
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 8:06 PM PT - Aug 7th, 2026
✅ @robobun, your commit 89da1001dc282ecbe3ba425921896f94877e023e passed in 🧪 To try this PR locally: bunx bun-pr 37170That installs a local version of the PR into your bun-37170 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![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 `@src/runtime/node/node_fs.rs`:
- Around line 1938-1951: Update dispose_without_post for ShellCpTask to reclaim
both the NewAsyncCpTask allocation and its embedded shell-task state when the
completion post is refused. Release the shell task through the existing cleanup
mechanism used by Cp::on_shell_cp_task_done, while preserving the no-JS-heap
access and single-owner safety guarantees.
In `@test/js/node/worker_threads/worker_threads.test.ts`:
- Around line 1854-1859: Replace the fixed setTimeout in the worker-thread
completion test with an observable signal or deadline-bounded polling that
confirms the detached pool thread attempted the refused completion before
exiting. Preserve the test’s assertions and only call process.exit(0) after that
condition is observed; if no signal can be exposed, document that limitation and
use polling with a bounded deadline rather than a wall-clock delay.
- Around line 1809-1813: Update all three subprocess tests in
test/js/node/worker_threads/worker_threads.test.ts:1809-1813, 1828-1831, and
1877-1881 to drain stderr concurrently with stdout and process exit. At
1809-1813 and 1877-1881, include proc.stderr.text() in the existing Promise.all
and assert stdout, stderr, and exitCode together; at 1828-1831, replace the
standalone proc.exited await with a concurrent Promise.all and assert both
results. Preserve the expected ordered stage output while making stderr
diagnostics visible in failures.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 215ca20a-be39-4738-af12-ecb8fa8db377
📒 Files selected for processing (8)
src/event_loop/lib.rssrc/jsc/VirtualMachine.rssrc/jsc/event_loop.rssrc/jsc/lib.rssrc/jsc/node_path.rssrc/jsc/web_worker.rssrc/runtime/node/node_fs.rstest/js/node/worker_threads/worker_threads.test.ts
💤 Files with no reviewable changes (1)
- src/event_loop/lib.rs
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 `@src/jsc/event_loop.rs`:
- Around line 1071-1076: Update EventLoop::poster_gate to replace the direct
.clone() call on the Arc reference with std::sync::Arc::clone, preserving the
existing deinitialized-state expectation and returned Arc behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8fc4382d-985b-4ecd-a8e6-1a9bc6cd95a8
📒 Files selected for processing (5)
src/jsc/VirtualMachine.rssrc/jsc/event_loop.rssrc/jsc/node_path.rssrc/jsc/web_worker.rssrc/runtime/node/node_fs.rs
… tests, fix clippy lints
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
A refused completion post for fs.open dropped the live descriptor without closing it; FsReturn::fs_discard for FD now closes it. The off-thread disposal also ran args Drop impls that touch JS-heap or VM-owned state: PathLike::Drop unpins a pinned Buffer path (a JS-cell deref), and ReadFile/WriteFile::Drop deref the AbortSignal's non-atomic WebCore refcount. Unprotect gains a required disarm_for_dead_vm hook that clears pins and forgets signal refs before the drop; ThreadSafe's dead-VM disposal calls it. The late-completion test now uses a Buffer path plus an AbortSignal under Malloc=1 so ASAN watches the JSC heap.
|
Verified three additional shapes against this branch head (89da100, ASAN debug build), all settling promptly where pre-fix main hangs:
No new tests added for these; the committed tests cover the same mechanism (gate refusal, signal-held disposal). |
There was a problem hiding this comment.
I re-reviewed after 89da100 and didn't find any new bugs — the FD fs_discard close, the required disarm_for_dead_vm hook (pinned-Buffer flag cleared, AbortSignalRef forgotten), and the stderr-draining test changes all address the earlier findings. Given this is cross-thread memory-safety code that introduces a new atomic gate primitive and a new off-thread disposal contract across ~20 Unprotect impls, a human look at the ConcurrentPosterGate orderings and the per-impl disarm_for_dead_vm coverage would still be worthwhile.
What was reviewed
ConcurrentPosterGateCAS/close protocol and its Arc-sharing so refused posters read live memory after VM dealloc.- The three
dispose_without_postpaths: promiseStrongwrapped inManuallyDrop,fs_discardon results (including the new FD close),dispose_for_dead_vmon args. - Each new
disarm_for_dead_vmimpl against its type'sDropfor JS-heap touches — PathLike/StringOrBuffer clearpinned, ReadFile/WriteFile forget the signal,FdVectorIo's no-op matches itsDrop-freeVectorArrayBuffer. - Tests: FIFO setup, concurrent stderr drain, and the Buffer-path + AbortSignal +
Malloc=1coverage in the late-completion test.
Extended reasoning...
Overview
This PR replaces #34660's whole-operation poster counter (which made worker/VM teardown spin forever on a blocked fs syscall) with an Arc-shared ConcurrentPosterGate that brackets only the enqueue. It touches 11 files: the gate primitive and its wiring in event_loop.rs/VirtualMachine.rs/web_worker.rs; three dispose_without_post paths in node_fs.rs (AsyncFSTask, NewAsyncCpTask, AsyncReaddirRecursiveTask); a new required Unprotect::disarm_for_dead_vm trait method with ~20 impls across node_path.rs/types.rs/node_fs.rs/PBKDF2.rs/node_crypto_binding.rs; a new FsReturn::fs_discard hook; and three POSIX-only regression tests.
My previous review found two 🔴 bugs (FD leak via missing fs_discard, and off-thread Drop touching the freed JSC heap via pinned-Buffer unpin / non-atomic AbortSignal deref) plus a 🟡 test-hygiene note. Commit 89da100 addressed all three; this run's bug hunt found nothing new, and I verified each fix landed as described.
Security risks
None user-facing. The risk surface is memory safety: use-after-free of JSC-heap cells or VM-owned C++ state from a detached pool thread after WebWorker__teardownJSCVM, and data races on non-atomic WebCore refcounts. The disarm_for_dead_vm mechanism is designed exactly to neutralize those, and the required-method (no default) design forces the compiler to enumerate every impl — a good structural guard. The ConcurrentPosterGate uses Acquire on the successful CAS in begin_post and AcqRel on the fetch_or in close, with an Acquire spin on the count; the Arc keeps the gate itself readable after the VM box is dealloc'd.
Level of scrutiny
High. This is exactly the category REVIEW.md flags hardest: cross-thread lifetime, JSC heap interaction after teardown, atomics with hand-picked orderings, and a new disposal contract every future Unprotect impl must honor. Two critical bugs were found and fixed during review, which is evidence the surface is subtle. The gate's memory-ordering choices, the completeness of disarm_for_dead_vm coverage (e.g. whether any args type's Drop reaches JS state through a path not obvious from its unprotect), and the intentional-leak of JSPromiseStrong/AbortSignalRef on the dead-VM path all deserve a maintainer's eyes.
Other factors
All prior review threads (CodeRabbit clippy fixes, comment-cop, my three findings) are marked resolved with commits. Tests are hermetic (mkfifo in a tempDir, no network), skipIf(isWindows) with a stated reason, drain stderr concurrently, and the late-completion test now exercises the Buffer-path + AbortSignal disarm under Malloc=1 for ASAN visibility. The PR description's verification section reports the #34660 guard test and the broader worker/fs suites still pass. The second new test leaves default-piped stdout undrained, but the child writes nothing to stdout so it's immaterial.
2 participants
Symptom
worker.terminate()never settles, noexitevent fires, and the worker OS thread spins a core at 40-85% forever whenever the worker has any pendingnode:fsthread-pool operation that cannot complete:fs.readFile/fs.promises.readFile/fs.createReadStream/fs.openon a FIFO with no writer, a pipe nobody writes to, or a hung mount. The same wait makes plainprocess.exit(0)hang underBUN_DESTRUCT_VM_ON_EXIT=1. Regression from #34660 (the poster counter + wait it introduced); stock 1.4.0-canary terminates in ~200ms.Hot-thread stack:
sched_yield <- EventLoop::wait_for_concurrent_posters (event_loop.rs) <- WebWorker::shutdown (web_worker.rs).Cause
#34660 made shutdown wait for in-flight work-pool fs completions so their posts cannot land after the final queue drain (a post into a torn-down VM leaked, or worse). But the counter brackets the whole fs operation, from schedule on the JS thread to the completion post on the pool thread, so the
while count > 0 { yield_now() }loop is bounded by syscall latency. A read from a writer-less FIFO never returns, so teardown spins forever.Fix
Replace the whole-operation counter with
ConcurrentPosterGate, anArc-shared state word (closed bit + active-post count) that brackets only the enqueue itself:begin_post()/end_post(). A successfulbegin_post()guarantees the VM stays live until the matchingend_post()(shutdown cannot finish closing the gate in between).Strong, protect counts) died with the heap and are deliberately not released; owned Rust payloads are freed via the newFsReturn::fs_discardhook andThreadSafe::dispose_skip_unprotect.Arc-cloned into each in-flight task, so a pool thread stranded in a blocked syscall can still read the closed state after the VM's memory is gone.fs_discardindestroy()also fixes an adjacent leak: the shutdown drain droppedreadFile/readdirresults whose buffers are normally freed only by the JSC finalizer installed into_js().Ops that hold JS-heap-backed buffers while blocked (e.g.
fs.writeof a Buffer into a full pipe) are unchanged by this PR: their completions are now refused safely, but the buffer lifetime question during teardown is the known pre-existing class that also covers crypto/zlib completions, and is out of scope here.Verification
On an ASAN debug build of main, both repros hang forever (killed at 20s); with this change
terminate()resolves (~2s debug/ASAN, ~200ms release-equivalent path) and theBUN_DESTRUCT_VM_ON_EXIT=1exit completes. A FIFO-unblock-after-terminate run exercises the refused-post disposal under ASAN+LSan with no errors and no new leaks.Tests added to
test/js/node/worker_threads/worker_threads.test.ts(POSIX; the libuv completion path on Windows runs on the JS thread and never had the wait):terminate()settles while the worker has an fs read blocked on a FIFOprocess.exit()with a blocked fs read pending completes underBUN_DESTRUCT_VM_ON_EXITterminate()is discarded without touching the dead worker VMAll three time out on main and pass with the fix. Also green locally: the #34660 guard test (
worker-shutdown-post-leak.test.ts),worker_threads.test.ts(91 tests),fs.test.ts(457),cp.test.ts,fs-leak.test.js, worker destruction/terminate-stress, andrust:check-all(10/10 targets). The abort-signal fs benchmark shows no per-op overhead change.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/node/worker_threads/worker_threads.test.ts