Worker teardown test gate: add a closed mode and pin each weak post's outcome - #38458
Worker teardown test gate: add a closed mode and pin each weak post's outcome#38458robobun wants to merge 1 commit into
Conversation
… outcome BUN_DEBUG_TEST_WORKER_TEARDOWN_GATE now takes a mode. "draining" holds a cross-thread post until the worker's teardown is waiting, as before, and the wait no longer closes while a post it released has not been made, so the post is always delivered and released by the wait. "closed" holds a weak post until the wait has ended, so it is refused and the poster frees its own payload; close_and_wait does not return until that post has been made. A weak post made while a ticket is outstanding is never held past the start of the wait in either mode, since the wait may be for the thread making it (WebCrypto posts its result from the pool task that carries the ticket). worker-late-completion.test.ts runs each weak row under both modes and asserts the outcome word: released by the wait under draining, refused under closed, released under both for the WebCrypto row. The waiter thread row now leaves the worker once the child's stdout has hit EOF, so the exit post is in the gate's hands before the worker tears down. Without the src change, the three weak rows came out refused in 23 of 30 runs under the draining gate (the worker closes before the woken poster posts), which is what the previous prefix match was hiding.
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 20df49a has some failures in 🧪 To try this PR locally: bunx bun-pr 38458That installs a local version of the PR into your bun-38458 --bun |
|
Status: ready for review; the diff is green on every CI job that ran.
|
|
Warning Review limit reached
Next review available in: 9 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It extends debug-only test infrastructure with a second gate mode and reworks the condvar/atomic interplay in close_and_wait / test_gate::weak_post; the concurrency reasoning looks sound and release builds compile it all away, but given it was requested in a specific review thread and touches the VM teardown wait, the reviewer who asked for it should confirm the shape matches what they had in mind.
What was reviewed:
- Traced the
closed-mode path: manualactive.fetch_add+Accessguard is balanced against the innerenter()in the post closure, andclose_and_wait's post-Closedactivewait cannot return before the parked poster has reported. - Traced the
draining-mode path:parkedis incremented beforepark_untiland decremented afterreport+notify, sonothing_parkedcannot let the wait storeClosedunder a poster that has been let through. - Checked the ticket-outstanding bypass:
tickets == 0at check time means the poster itself holds no ticket, so parking it untilClosedcannot deadlock the wait-for-tickets. - Verified the release stub module still compiles (all changed hooks stubbed;
first_report_secsis only called undercfg(debug_assertions)), and no other reader of the old boolean feature flag remains.
Extended reasoning...
Overview
Follow-up to #38299: turns the debug-only BUN_DEBUG_TEST_WORKER_TEARDOWN_GATE from a boolean into a two-mode string (draining / closed), so the worker-teardown test can deterministically exercise both the "weak post released by the wait" and "weak post refused after Closed" paths, and assert the exact outcome word instead of just that a line appeared. Touches env_var.rs (flag kind change), VmHandle.rs (the test_gate module and two hook calls in close_and_wait), web_worker.rs / VirtualMachine.rs (pass the mode through), and the test file.
Security risks
None. Everything new is behind #[cfg(debug_assertions)] and a BUN_DEBUG_* env var read only for first-level workers; the release-build stub module makes every hook a no-op (including the new nothing_parked → true), so close_and_wait's production behaviour is byte-identical.
Level of scrutiny
Higher than the diff size suggests. The test_gate module is concurrent code that parks foreign threads on the same condvar the JS thread's teardown wait sleeps on, and the new closed mode reuses the production active count to keep close_and_wait from returning until the parked poster has reported. I walked both modes' wakeup orderings (see the summary above) and found them correct — in particular the double-Access in the closed path (one manual, one from the inner enter() inside the refused post) is balanced, and the parked counter is bumped before park_until so the JS thread cannot race past it to Closed. The waiter-thread test row is still timing-biased (stdout EOF vs. reap → post), which the PR body acknowledges and validates with 360/360 runs.
Other factors
This was requested in a specific review thread by a reviewer who presumably has the surrounding context (the deleted worker-refused-completion.test.ts, the exact bypass semantics they wanted). The PR body is thorough — cross-checked against main's src, clippy/release-check clean, source-lint test still passes — and the change is internally consistent. Deferring so the requesting reviewer can confirm this is the shape they asked for; nothing here needs changing from what I can see.
2 participants
Follow-up to #38299, from the
worker-late-completion.test.tsthread in #38299 (review).Problem
test/js/web/workers/worker-late-completion.test.tsmatched[vm] late post: <tag> (only, so they passed whether the post wasreleased by the waitorrefused.Draining, so sinceworker-refused-completion.test.tswas deleted nothing reaches theClosed -> Refusedpaths on purpose:VmHandle::post_cpp_taskdeleting theEventLoopTaskon the posting thread (src/jsc/VmHandle.rs), and the waiter thread's refused branch (src/spawn/process.rs,release_ref_from_waiter_thread).DrainingandClosed:close_and_waitpublishesDraining, runsservice(), sees no tickets and storesClosedbefore the woken poster gets topost(). Building this branch's test against main's src, the three weak rows (BroadcastChannel, MessagePort, waiter thread) came outrefusedin 23 of 30 runs here, so on main those rows mostly exercise refusal, and which one they exercise is luck.Fix
BUN_DEBUG_TEST_WORKER_TEARDOWN_GATEtakes a mode (src/bun_core/env_var.rs, string instead of a feature flag;TestGate::from_envinVmHandle.rs;web_worker.rs/VirtualMachine.rspass the mode through).draining: as before, plus aparkedcount in the gate's debug state. A weak post the gate holds forDrainingis counted from arrival until it has been made and reported, andclose_and_waitdoes not storeClosedwhile the count is non-zero (test_gate::nothing_parked), so such a post is always released by the wait. Ticketed posts need nothing extra: the wait is for their ticket.closed: a weak post is parked untilClosedand so refused. The parked thread holds one unit ofactivefor the duration (the existing "weak access in progress" count), soclose_and_waitreturns, and the VM is destroyed, only after the post has been refused and reported;close_and_waitnow notifies the condvar after publishingClosedas well asDraining.closedmode a weak post that arrives while a ticket is outstanding is parked only untilDraining. The poster may be the thread the wait is waiting for (ConcurrentCppTask::run_ownedmakes WebCrypto's result post while still holding the ticket), so parking it untilClosedwould hang the wait.released by the waitunderdraining,refusedunderclosed, andreleased by the waitunder both forcrypto.subtle.digest(underTicket), which is the row that exercises the bypass (without it that run never ends). Ticketed rows are unchanged and run once. The waiter thread row now leaves the worker after the child's stdout hits EOF instead of right away, so the exit post reaches the gate before the worker tears down; with the old shape the child (a debugbun -e 0) usually exits long after the worker has closed, and the post is never parked at all.bun bd test test/js/web/workers/worker-late-completion.test.ts37/37 (debug, Linux). The 8 weak variants repeated 15 times serially and 10 times in 3 parallel loops: 360/360. Against main's src (test only): the 3closedrows that matter fail as intended, and thedrainingrows flip between outcomes as described above.test/internal/source-lints/vm-thread-door.test.ts48/48;cargo check -p bun_jsc --release(thenot(debug_assertions)stub module) andcargo clippy -p bun_jsc -p bun_coreclean.Background
VmHandle::close_and_wait) goesDraining(wait for everyTicket, releasing whatever arrives on the worker's own thread) and thenClosed(nothing off-thread may reach the VM any more). A weak poster, something holding aVmHandlebut no ticket (another thread's MessagePort or BroadcastChannel post, the child-process waiter thread), getsPosted::QueuedbeforeClosedandPosted::Refusedafter, and on refusal must free its own payload; those refusal paths are what theclosedmode covers.activeis the count of threads currently inside a weak access on the handle;close_and_waitpublishesClosedand then waits for it to reach zero, so a weak access either finished before the VM is destroyed or sawClosed. Theclosedmode reuses it to keep the VM alive until the parked post has been reported.test_gatemodule inVmHandle.rs) exists only in builds with debug assertions and only for first-level workers; it parks a cross-thread post on its own thread until the worker's handle reaches the requested state, and names the post and its outcome on stderr, which is what the test reads.Outcome of the weak rows under the draining gate, main's src vs this branch
Built with
git stash push -- src/(this branch's test file, main's gate), thenbun test ... -t "draining gate"10 times; the three rows expectingreleased by the wait:Failure detail is always the same line, e.g.
[vm] late post: CppTask (refused).With this branch's src: 15 serial iterations of all 8 weak variants, then 10 iterations in each of 3 parallel loops, no failures (360/360).
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/web/workers/worker-late-completion.test.ts