child_process: apply kernel backpressure to stdout/stderr pipes - #34971
Conversation
node:child_process spawn() registered the stdout/stderr poll immediately, so the native reader drained the child's output into an unbounded in-memory buffer before any JS consumer attached. A child writing 100 MB into a pipe nobody was reading finished in seconds (instead of blocking at the ~64 KB kernel pipe buffer as it does under Node), the parent buffered the whole output in RAM, and once 'exit' fired the autoResume path discarded the entire buffer so a late reader received 0 bytes. Defer poll registration / uv_read_start until JS first pulls when Bun.spawn is called with lazy: true (the documented semantics and what child_process uses internally). The kernel pipe buffer now provides backpressure so the child blocks until stdout is read, matching Node. Also: - match Node's flushStdio: only resume stdout/stderr on exit when the stream was never touched (readableFlowing === null) - fix a debug-build-only $debug template literal in native-readable that stringified a Uint8Array per chunk (~600 ms per 220 KB chunk under ASAN), which the chunkier delivery surfaced - drop lazy: true from the spawn.test.ts huge() fixture, whose 'after exit' subtest relied on the eager buffering this removes
|
Warning Review limit reached
Next review available in: 13 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 (2)
WalkthroughChangesSubprocess pipe lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 2:21 PM PT - Jul 21st, 2026
❌ @robobun, your commit f88d7de has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34971That installs a local version of the PR into your bun-34971 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
… drain A lazy pipe reader that JS never accesses stayed IS_PAUSED forever, so on_process_exit's reader.read() early-returned, on_reader_done never fired, and has_pending_activity() kept the Subprocess Strong-rooted. Call unpause() before read() in the on_process_exit Readable::Pipe arms so the remaining kernel-buffered bytes are drained and the slot transitions to Readable::Buffer. Backpressure is moot once the direct child has exited.
|
Diff is green on build 77082: the new Remaining red is unrelated flake, all either tagged
Both bot review threads are resolved (on_process_exit unpause in 755e1d2, KEEP_ALIVE flag in e88559f, readableFlowing gate removed in f88d7de). Ready for review. |
With a lazy subprocess pipe reader the FilePoll is not created until on_start() runs, so an unref() issued before the first _read (as esbuild does: child.stdout.unref() synchronously after spawn) was a no-op and the poll later registered with keep-alive enabled, pinning the event loop. Track the desired state in PosixFlags::KEEP_ALIVE (default true) so update_ref() records it before the poll exists and register_poll() honours it when the poll is finally created.
Node v26's flushStdio resumes any readable stdio stream regardless of readableFlowing; the extra === null guard prevented a lazily-started reader that was explicitly pause()d from ever draining after the child exited, so ChildProcess 'close' never fired and the fd leaked. Keep the readable/!destroyed checks and drop the readableFlowing clause.
There was a problem hiding this comment.
Both earlier findings (the lazy-reader Strong-root leak and the readableFlowing === null drain gate) are addressed in 755e1d2 and f88d7de, and this pass found nothing new. Deferring to a human given the scope — this rewires subprocess pipe start/pause/keep-alive semantics across POSIX and Windows and changes what lazy: true means for the public Bun.spawn API.
What was reviewed:
on_process_exitnowunpause()s beforeread()for both stdout/stderr arms; Windows was already covered viaread()→start_reading().PosixFlags::KEEP_ALIVEpropagates throughfrom()so anunref()before first poll registration is honoured after transfer toFileReader.- Windows lazy
start()path leaves the source wired (set_parentalready set.data) andIS_PAUSEDset so a laterstart_reading()is well-formed. #handleOnExitnow gates onreadable && !destroyedonly, matching Node'sflushStdio; the newpause()-before-exit test covers the regression.
Extended reasoning...
Overview
Nine files: PipeReader.rs (new KEEP_ALIVE flag, deferred register_poll() when IS_PAUSED, update_ref now &mut), SubprocessPipeReader.rs (start() gains lazy param; POSIX inserts IS_PAUSED pre-start, Windows wires source without uv_read_start), subprocess.rs (on_process_exit unpauses lazy readers before draining), FileReader.rs (on_start unpauses a from_pipe reader before first read()), js_bun_spawn_bindings.rs (threads !IS_SYNC && lazy into start()), child_process.ts (#handleOnExit resume gate), native-readable.ts (debug-log stringification fix), plus tests in spawn.test.ts and child_process.test.ts.
Security risks
None identified. No parsing of untrusted input, no auth/crypto surface. The change is I/O lifecycle plumbing.
Level of scrutiny
High. This is core subprocess pipe lifecycle across two platforms with hand-managed refcounts, FilePoll keep-alive accounting, and GC-visible pending-activity state. The PR already surfaced two real bugs during review (a Strong-root/fd leak and a 'close'-never-fires hang) plus one CI regression (esbuild unref() before first poll). It also changes observable semantics of the documented Bun.spawn({lazy: true}) option — a child writing more than the kernel pipe buffer will now block until stdout is read, which is the point of the fix but is a behaviour change for direct Bun.spawn users (the huge() fixture in spawn.test.ts had to drop lazy: true for exactly this reason).
Other factors
- Both of my prior inline findings are resolved with targeted fixes and regression tests (WeakRef GC test in
spawn.test.ts;pause()-then-closetest inchild_process.test.ts). - The Windows lazy path is untested here (both new tests are
skipIf(isWindows)/skipIf(!isPosix)); coverage relies on existing Windows CI lanes exercisingnode:child_processgenerally. - Build #77082 was still running at time of review; the previous build surfaced the esbuild
unref()regression, so green CI on the latest commit is worth confirming before merge. - The
update_ref&self→&mut selfsignature change ripples todisable/enable_keeping_process_alive; any callers holding only&PosixBufferedReaderwould fail to compile, so if it builds it's consistent.
2 participants
Problem
node:child_processstdout/stderr pipes had no backpressure: a child writing 100 MB into a pipe nobody reads was never blocked (completes in <4 s; Node blocks at the ~64 KiB kernel pipe buffer). The parent buffered the whole output in RAM with no ceiling, and if'exit'fired before a'data'consumer attached the entire buffered output was discarded, so a late reader got 0 of 104,857,600 bytes.Cause
PipeReader::start()registered the fd with epoll/IOCP immediately, so the event loop drained the child's output intoPosixBufferedReader._buffer(which has no HWM for non-streaming parents) before any JS consumer attached. When the child exited,#handleOnExitcalledresume()on a stream with no'data'listener, dropping every buffered chunk.Fix
When
Bun.spawnis called withlazy: true(the documented "don't start reading stdout until accessed" option, whichnode:child_processuses internally), start the pipe reader paused: setIS_PAUSEDbeforePosixBufferedReader::start()so it skipsregister_poll(), and on Windows wire the source without callingstart_reading(). The kernel pipe buffer then provides backpressure and the child blocks until JS first reads, matching Node.FileReader::on_start()clearsIS_PAUSEDbefore its firstread()so the transferred reader wakes up.Also:
#handleOnExitnow matches Node'sflushStdio: onlyresume()stdout/stderr whenreadableFlowing === null(the stream was never touched), so an explicitly paused stream is not drained behind the user's back.$debugtemplate literal innative-readable.tsthat interpolated the resolvedUint8Arraydirectly.Uint8Array.prototype.toString()comma-joins every byte, so a 220 KB chunk spent ~600 ms stringifying under ASAN. This was pre-existing but the chunkier delivery made it visible.lazy: truefrom thehuge()fixture inspawn.test.ts; itsafter exitsubtest relied on the eager buffering this removes (with correctlazysemantics a 250 KB write cannot finish before stdout is read).Verification
The new
stdout pipe backpressure > blocks the child until a reader attaches and delivers every bytetest intest/js/node/child_process/child_process.test.tsfails on main (child exits withexitCode: 0within 10 ms) and passes with the fix.no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/spawn/spawn.test.ts test/js/node/child_process/child_process.test.ts