Conversation
WalkthroughChangesWorker stdio exit draining
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 5:36 AM PT - Jul 30th, 2026
✅ @robobun, your commit 3ff7865b719af551a0bab1a9184659e18bd8c101 passed in 🧪 To try this PR locally: bunx bun-pr 34340That installs a local version of the PR into your bun-34340 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix mirroring Node's flushSync in is_not_main_thread.
What was reviewed:
makePortWritable/setupWorkerStdio: exposingonAckasflushInFlightForExitand theprocess.on('exit')flush;onAckis a no-op when nothing is parked, andport.unref()during exit is harmless.- Handler ordering:
flushSyncis registered in preload before user code, so userprocess.on('exit')handlers run after it with_exitingalready true — covered by the last test. - Parent-side
worker.stdinalso gains the property viamakePortWritablebut nothing calls it — harmless.
Extended reasoning...
Overview
Two-file change: src/js/node/worker_threads.ts (~20 net lines) exposes the port-Writable's ack-completion callback as stream.flushInFlightForExit and registers a process.on('exit') handler in setupWorkerStdio that calls it on stdout/stderr. test/js/node/worker_threads/worker_threads.test.ts adds a 6-test describe covering console.log/error, raw process.std{out,err}.write, 100 writes, and writes from a user process.on('exit') handler.
Security risks
None. This is a worker-internal stdio flush hook; no untrusted input parsing, no auth/crypto/permissions surface.
Level of scrutiny
Low-to-moderate. The mechanism is a direct port of Node's documented approach (internal/bootstrap/switches/is_not_main_thread → flushSync → kStdioWantsMoreDataCallback), and the diff is small and mechanical: capture the stream in a local, attach a property, register one exit listener. The existing _exiting branch inside writev was already designed for this — it just wasn't reachable without the exit-time nudge.
Other factors
onAckguards onpendingWriteCallback !== null, so calling it with nothing in flight is a safe no-op; theport.unref()it performs is irrelevant during exit.setupWorkerStdioruns in thenode:worker_threadspreload before user code, soflushSyncis the first exit listener; user exit-handler writes then takewritev's synchronous_exitingpath (verified by the last new test).- The property also lands on parent-side
worker.stdin(also built viamakePortWritable), but no one calls it there — inert. - Existing backpressure tests still pass per the PR, so the ack-gated flow control is intact for the non-exit path.
- No CODEOWNERS entry for this path; no prior human review comments; bug hunt found nothing.
There was a problem hiding this comment.
LGTM — focused Node compat fix mirroring Node's flushSync in is_not_main_thread.
What was reviewed:
flushInFlightForExitis just the existingonAck; when no callback is parked it's a no-op, and when one is it unblocks the Writable so buffered chunks re-enterwritev's_exitingsynchronous branch.- The exit handler is registered once during worker preload (before user code), so user
process.on('exit')handlers run after and their writes hit the_exitingfast path — covered by the last test. makePortWritableis also used for parent-sideworker.stdin; the extra property there is harmless (never called).- Normal-path backpressure/ack flow is unchanged;
destroy()still nullspendingWriteCallbackand drops the ack listener.
Extended reasoning...
Overview
The PR fixes silent stdout/stderr data loss when a worker calls process.exit() synchronously after writing. It touches src/js/node/worker_threads.ts (~20 net lines) and adds a new test file with 6 cases covering console.log/error, raw writes, many-writes, and writes from a user 'exit' handler.
The fix exposes the existing onAck closure as stream.flushInFlightForExit and registers a single process.on('exit', flushSync) in setupWorkerStdio that calls it on stdout/stderr. Completing the parked writev callback while process._exiting is true lets the Writable re-enter writev, whose existing _exiting branch then posts each buffered batch and completes synchronously. This is a direct port of Node's mechanism in internal/bootstrap/switches/is_not_main_thread (kStdioWantsMoreDataCallback), and the PR description and code comments cite the upstream reference.
Security risks
None. This is Node.js worker stdio plumbing over an in-process MessageChannel; no auth, crypto, filesystem, or untrusted-input parsing is involved. The added process.on('exit') listener runs only inside worker threads (guarded by the existing setupWorkerStdio call site under !isMainThread && _isNodeWorker).
Level of scrutiny
Low-to-medium. The source change is small, mechanical, and additive: no existing control flow is altered — the Writable's writev/final/destroy are byte-identical, and the normal ack-gated backpressure path is untouched (the PR notes the existing backpressure tests still pass). onAck already guards on pendingWriteCallback !== null, so calling it when nothing is in flight is a no-op. The exit handler is registered exactly once per worker during preload, before user code, which is what makes the "writes from a user exit handler" test work (user handler fires after, and by then _exiting is true so its writes flush synchronously).
Other factors
- The property is also attached on parent-side
worker.stdin(samemakePortWritable), but nothing calls it there — harmless. - Tests await real conditions (
once(worker, 'exit')+once(stream, 'end')) and assert combined{out, code}objects, matching repo conventions. The PR verified all 6 fail without the src change and pass with it. - No CODEOWNERS entry covers this path, no outstanding human review comments, and no prior review from me on this PR.
|
CI build #85722 (rebased onto main at c08f665): Remaining reds are unrelated to this diff:
Ready for review. |
d44297a to
c7df31f
Compare
There was a problem hiding this comment.
LGTM — small, well-scoped Node compat fix that mirrors Node's own flushSync mechanism.
What was reviewed
- Traced
onAck→ parked-cb completion → Writable re-enterswritevunder_exitingand drains synchronously; verified it's a no-op when nothing is parked or the stream was already destroyed. - Checked exit-handler ordering:
flushSyncregisters at preload (before user handlers), so userprocess.on('exit')writes hit the_exitingfast path — matches the test expectation. - Confirmed
makePortWritableis also used for parent-sideworker.stdin; the new property is harmless there (nothing calls it), matching Node which only flushes worker stdout/stderr. - Tests await real events, drain subprocess pipes concurrently, and cover both captured and auto-piped paths plus a no-exit baseline.
Extended reasoning...
Overview
Two-file change: src/js/node/worker_threads.ts exposes the port-writable's ack handler as flushInFlightForExit and registers a process.on('exit') handler in setupWorkerStdio that calls it on stdout/stderr. A new test file adds 8 tests covering captured stdio (console + raw write, stdout + stderr, many writes, writes from a user exit handler) and auto-piped stdio via a spawned subprocess.
Security risks
None. This is worker-internal stdio plumbing over an existing MessagePort; no new inputs are parsed, no privilege boundaries are crossed. The added flushInFlightForExit string property on process.stdout is user-observable, but users can already replace process.stdout wholesale in a worker — not a boundary.
Level of scrutiny
Moderate. It's a Node.js compat behavioral fix in built-in JS (no native code), ~25 lines of src change, and directly mirrors Node's documented mechanism (internal/bootstrap/switches/is_not_main_thread → kStdioWantsMoreDataCallback). The tricky part is the synchronous re-entry cascade through Writable's buffer, which I traced by hand: completing the parked cb while _exiting is true lets each subsequent writev post-and-complete synchronously until the buffer is empty. The 100-line and 300-line tests exercise exactly this cascade.
Other factors
onAckis safe to call when nothing is parked (guards onpendingWriteCallback !== null) and afterdestroy()(which nulls the pending cb and removes the port listener), soflushSynccannot double-unref or fire into a destroyed stream.- Handler registration order:
setupWorkerStdioruns during thenode:worker_threadspreload, soflushSyncis the firstexitlistener; user exit-handler writes then take the_exitingsynchronous branch directly — the test assertingbefore\nfrom-exit\ntail\ncovers this. - Tests follow harness conventions:
once()on real events,Promise.allfor subprocess pipe draining, exact-value assertions on combined{out, code}objects, and a no-process.exit()baseline. PR description confirms fail-before / pass-after and that the existing backpressure tests still pass.
There was a problem hiding this comment.
LGTM — small, well-reasoned Node-compat fix that mirrors Node's own flushSync mechanism.
What was reviewed
- Traced the flush path:
onAckis idempotent (null-checkspendingWriteCallback), so calling it from the exit handler when nothing is parked or afterdestroy()is a safe no-op. - Checked exit-handler ordering:
flushSyncis registered at preload so it fires before user'exit'handlers; subsequent writes hit writev's_exitingfast path, which the "writes from a user process.on('exit') handler" test covers. - Confirmed test coverage spans the variant matrix (console vs raw write, stdout vs stderr, captured vs auto-piped, many-writes, plus a no-exit control).
- Ruled out the stderr exact-set assertion flaking under debug/ASAN —
bunEnvquiets debug logs and the assertion is on filtered/sorted lines, not raw equality.
Extended reasoning...
Overview
This PR fixes silent data loss in node:worker_threads port-backed stdio when a worker calls process.exit() synchronously. The fix touches only src/js/node/worker_threads.ts (~20 lines): makePortWritable now exposes its ack-completion callback as flushInFlightForExit on the returned Writable, and setupWorkerStdio registers a process.on('exit') handler that invokes it on both stdout and stderr. This directly mirrors Node's internal/bootstrap/switches/is_not_main_thread flushSync, as cited in the code comments and PR description.
A new test file adds 8 tests covering captured stdio (console.log/error, raw process.stdout/stderr.write, 100 buffered writes, writes from a user exit handler) and auto-piped stdio (300 lines via a spawned subprocess, plus a control case without process.exit()).
Security risks
None. This is JS-side stream-completion plumbing inside a worker's own process; no untrusted input parsing, no fd/resource handling changes, no new API surface exposed to users beyond an internal expando property on the port-backed Writable (matching the existing endFromOwner pattern on the readable side).
Level of scrutiny
Medium. Worker stdio is user-visible behavior, but the change is small, mechanically follows Node's documented approach, and only adds a call to an already-existing idempotent function (onAck) at exit time. I traced the interaction with destroy() (which nulls pendingWriteCallback, making the flush a no-op), the _exiting fast path in writev (which makes post-flush writes complete synchronously), and exit-handler ordering (flushSync is registered during preload, so it runs before user handlers — and the tests confirm user-handler writes still land via the _exiting branch). The parent-side worker.stdin Writable is intentionally not flushed on parent exit, matching Node.
Other factors
The PR description confirms fail-before/pass-after for all new tests and that the existing backpressure tests (stdout write completion is withheld until the parent reads, large stdout survives writev batching and repeated acks) still pass, so ack-gated flow control is intact. The bug-hunting system found no issues; one candidate (exact stderr line-set assertion flaking under ASAN/debug) was examined and refuted — bunEnv sets BUN_DEBUG_QUIET_LOGS=1 and the assertion compares a filtered+sorted line array, not raw stderr. Tests follow harness conventions (bunEnv/bunExe, await using for spawn, concurrent pipe drain via Promise.all, once() for events).
ef42fb7 to
2fe9041
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
A worker that writes to stdout/stderr and then calls process.exit() would
lose every chunk buffered after the first writev batch: the in-flight
writev parks its completion callback awaiting a reader ack that can never
arrive inside a synchronous exit, so the Writable never re-enters writev
and its buffered chunks are dropped.
Node's is_not_main_thread bootstrap registers a process.on('exit')
listener that completes the parked callback (kStdioWantsMoreDataCallback),
letting the stream flush its buffer while _exiting is true so each
subsequent writev posts and completes synchronously. Mirror that here.
worker_threads.test.ts has pre-existing debug-mode timeouts in unrelated subprocess-spawn tests; a dedicated file gives the new coverage a clean pass/fail signal.
Since #31216 worker stdout/stderr is always port-backed, so the process.exit() drop also hits workers created without {stdout: true}; a worker that logs N lines then exits surfaces only the first.
process.on('exit') fires with _exiting === true on both, so the same
flushSync drains the Writable; without it a worker that logs N lines
then throws surfaces only LINE-0 on the parent's stdout.
Each test creates its own Worker or subprocess with independent piped stdio; running them concurrently cuts the ASAN wall-clock roughly 3x.
6ae19c2 to
accf6bb
Compare
There was a problem hiding this comment.
LGTM — the earlier describe.concurrent nit was applied, and the remaining inline nit (string-keyed flushInFlightForExit vs. a module-local Symbol) is worth addressing but mirrors the pre-existing endFromOwner pattern in the same file and doesn't affect correctness.
Extended reasoning...
Overview
~16 lines in src/js/node/worker_threads.ts: expose the parked-writev ack completion (onAck) on the port-backed Writable and register a process.on('exit', flushSync) in setupWorkerStdio that fires it for stdout/stderr. This is a direct port of Node's internal/bootstrap/switches/is_not_main_thread flushSync → kStdioWantsMoreDataCallback mechanism. Plus a 167-line test file covering captured stdio, auto-piped stdio, and the uncaught-exception / unhandled-rejection exit doors.
Security risks
None. Worker threads share the parent's address space and fd table; there is no privilege boundary here. The one tamper-resistance concern (string-keyed hook on process.stdout) is flagged inline as a nit — it only misbehaves if user code deliberately overwrites an undocumented property on its own stream, and the same shape already exists as stream.endFromOwner in makePortReadable.
Level of scrutiny
Medium. It's Node-compat runtime JS on a hot-ish path (every worker's stdio), but the change is small, the mechanism is well-understood (unblock the parked writev cb so the Writable re-enters writev while _exiting is true and each subsequent batch completes synchronously), and it mirrors the reference implementation line-for-line. I checked that onAck() is a no-op when pendingWriteCallback is null, that flushSync is registered before user code (via the node:worker_threads preload) so it fires first, and that later user exit handlers' writes still land because _exiting short-circuits the ack wait — the test at line 54 covers exactly that.
Other factors
- CI green (Build #77437). The fail-before / pass-after evidence in the PR body is convincing (9 fail on ASAN main, all pass with fix; existing backpressure tests unaffected).
- My prior
describe.concurrentnit was applied (commit accf6bb / 6ae19c2); ASAN wall-clock dropped ~26s → ~7.5s. - The comment-cop warnings are resolved (the current diff has a one-line reference comment, not a paragraph).
- Tests follow harness conventions:
bunEnv/bunExe,await using proc,Promise.allover stdout/stderr/exited, exact-value assertions, error events wired into the awaited promise viaonce().
Matches Node's kStdioWantsMoreDataCallback and this file's existing convention for internal hooks; avoids exposing a user-overridable string-keyed property on process.stdout/stderr and worker.stdin.
There was a problem hiding this comment.
LGTM — both prior review notes are addressed (Symbol-keyed hook, describe.concurrent), and the fix now matches Node's flushSync in is_not_main_thread exactly.
What was reviewed:
- Traced the flush mechanism:
onAck()unparks the pendingwritevcb, after which_exiting === truemakes subsequentwritevcalls complete synchronously — buffered chunks drain end-to-end. - Checked exit-listener ordering:
flushSyncis registered at preload before user code, and post-flush writes from a userexithandler hit the_exitingfast path (covered by test). - Confirmed the Symbol on parent-side
worker.stdinis harmless (module-local, no reader) andonAck()is a no-op when nothing is in flight. - Tests cover captured + auto-piped stdio, both streams, raw writes, user exit-handler writes, and the uncaught-exception / unhandled-rejection paths;
once()wireserrorto reject.
Extended reasoning...
Overview
Two-file change: ~19 lines in src/js/node/worker_threads.ts and a new 167-line test file. The runtime change adds a module-local kStdioWantsMoreDataCallback Symbol, attaches the port-writable's onAck to the stream under that key, and registers a process.on('exit', flushSync) handler in setupWorkerStdio that invokes it on stdout/stderr. This directly mirrors Node's internal/bootstrap/switches/is_not_main_thread and unblocks the parked writev callback so the Writable's internal buffer drains during synchronous exit (the pre-existing process._exiting fast path in writev then keeps subsequent batches synchronous).
Security risks
None. Worker threads are not a privilege boundary; the hook is keyed by a module-local Symbol so it is not user-reachable (my earlier tamper-resistance concern about the string-keyed property was addressed in 3ff7865). No untrusted input parsing, no auth/crypto/permission surface.
Level of scrutiny
Medium — Node compat behavior in a built-in JS module, but the change is small, mechanically follows the Node reference, and is backed by fail-before/pass-after evidence on both ASAN and release. The mechanism is straightforward: onAck was already the ack handler; this just calls it once at exit time so the Writable's own machinery re-enters writev. I traced that onAck no-ops when pendingWriteCallback is null, so calling it on a stream with nothing in flight is safe, and the ?. guards handle the stdout-only / stderr-only cases.
Other factors
- Both prior review comments from me (test concurrency, Symbol key) are resolved with follow-up commits, and the comment-cop notes about verbose comments were also addressed.
- CI build #85722 passed on all lanes for this test file per the robobun status; remaining reds were unrelated flakes.
- Test coverage is thorough: 10 tests across captured stdio (console + raw write, both streams), auto-piped stdio, a no-exit control, and the uncaught-exception / unhandled-rejection exit paths — all subprocess tests drain stdout/stderr/exited concurrently and assert exact structured output.
- The PR description states the existing backpressure tests still pass, so the ack-gated flow control on the normal (non-exiting) path is intact.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-30 and it conflicts with main. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
2 participants
What
A worker that writes to stdout/stderr and then exits synchronously loses every chunk buffered after the first
writevbatch. Exit code is clean; the loss is silent. Applies toprocess.exit(), an uncaught exception, and an unhandled rejection alike.Since #31216 every
node:worker_threadsworker's stdio is port-backed (auto-piped to the parent's stdout/stderr when{stdout: true}is not set), so this hits the default path too:Same with captured stdio:
Affects
console.log,console.error, and rawprocess.stdout.write/process.stderr.writealike.Why
makePortWritable'swritevposts the payload over the MessagePort and parks its completion callback awaiting a reader ack. Inside a synchronous exit there are no event-loop turns left to deliver that ack, so the Writable never re-enterswritevand everything queued in its internal buffer after the first batch is dropped. The existingprocess._exitingescape hatch insidewritevis unreachable: by the time_exitingis true the stream is already blocked on the parked callback.Node handles this in
internal/bootstrap/switches/is_not_main_thread: when the worker stdio is created it registersprocess.on('exit', flushSync), which callskStdioWantsMoreDataCallbackon stdout and stderr. That completes the parked callback while_exitingis already true, so the stream flushes its buffered chunks and each subsequentwritevposts and completes synchronously. Theexitevent fires on every worker exit path (explicitprocess.exit(), uncaught exception, unhandled rejection) with_exiting === true, so one handler covers all three.Fix
Expose the ack-completion as
flushInFlightForExiton the port-backed Writable, and register aprocess.on('exit')handler insetupWorkerStdiothat calls it on both streams.Verification
The six captured-stdio tests, the auto-piped
process.exit()test, and both uncaught-error tests fail without thesrc/change (first batch only,count: 1) and pass with it (count: 300). The existing backpressure tests (stdout write completion is withheld until the parent reads,large stdout survives writev batching and repeated acks) still pass, so the ack-gated flow control is intact.[review] gate passed · iteration 8 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 3 rejected · iteration 8
evidence per changed file