spawn(windows): release StaticPipeWriter start() ref when buffer-stdin write completes - #35297
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesThe PR updates buffer-backed subprocess stdin writer reference handling across pending-start, write, and close callbacks, hardens Windows pending-data accounting, and adds Windows-only RSS regression tests for draining and rejecting child stdin consumers. Stdin writer lifecycle
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?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 `@test/js/bun/spawn/spawn-buffer-stdin-leak-fixture.ts`:
- Around line 38-41: Document the purpose of the 50 ms delay in settle():
explain that it allows finalization to complete after forced garbage collection
before RSS is measured, and retain the existing GC and sleep behavior.
- Around line 15-17: Expand spawn-buffer-stdin-leak-fixture.ts at lines 15-17 to
parameterize stdin construction for Buffer, ArrayBuffer, and Blob inputs, and
add a Bun.spawnSync measurement path at lines 26-31 while preserving both drain
and reject modes. Update spawn-buffer-stdin-leak.test.ts lines 38-64 to execute
both close-path modes across every supported stdin type and both Bun.spawn and
Bun.spawnSync APIs.
In `@test/js/bun/spawn/spawn-buffer-stdin-leak.test.ts`:
- Line 50: Remove the explicit 120_000 per-test timeout overrides at both
referenced test cases, allowing the repository-managed timeout policy to apply.
Do not alter the test behavior or surrounding assertions.
🪄 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: a790339e-2b2c-4422-8b9d-703d9bba9b9a
📒 Files selected for processing (4)
src/runtime/api/bun/subprocess.rssrc/spawn/static_pipe_writer.rstest/js/bun/spawn/spawn-buffer-stdin-leak-fixture.tstest/js/bun/spawn/spawn-buffer-stdin-leak.test.ts
|
Updated 3:42 PM PT - Jul 23rd, 2026
❌ @robobun, your commit 87ce158 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 35297That installs a local version of the PR into your bun-35297 --bun |
3e81ba8 to
576a55b
Compare
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/spawn/spawn-buffer-stdin-leak-fixture.ts`:
- Around line 32-34: Update the child-process completion handling around
proc.exited so each fixture child rejects when its exit code is nonzero, while
still resolving successful exits. Ensure Promise.all(procs) therefore fails when
any drain child fails and only passes after successful on_write execution.
🪄 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: 3448ebfa-ed6a-4305-9794-5b1f8295bb3a
📒 Files selected for processing (5)
src/io/PipeWriter.rssrc/runtime/api/bun/subprocess.rssrc/spawn/static_pipe_writer.rstest/js/bun/spawn/spawn-buffer-stdin-leak-fixture.tstest/js/bun/spawn/spawn-buffer-stdin-leak.test.ts
576a55b to
a9afac1
Compare
a9afac1 to
14b431c
Compare
There was a problem hiding this comment.
All three earlier findings are addressed and I didn't find anything new, but this is intrusive-refcount lifecycle in the Windows spawn path — worth a human pass on the release-site mutual exclusion.
What was reviewed:
- Traced start()'s +1 through the three release sites (
on_write, Windowson_close,take_pending_start_writer) — all clearstartedbefore deref, so they're mutually exclusive; no double-deref found on the drain, error-arm, or exit-first orderings. - Checked
on_write→writer.close()→on_closere-entry on Windows:startedis already false inon_close, so the inner release is a no-op. - Confirmed
SecurityScanSubprocesssetsstarted = falseimmediately afterstart(), so both new release sites are no-ops there. - Verified
on_write_complete's scopeguard holds write()'s +1 across the newon_closederef on the error arm.
Extended reasoning...
Overview
Fixes a Windows-only per-spawn leak of StaticPipeWriter when Bun.spawn/spawnSync is given a non-empty ArrayBuffer/Blob stdin. Touches src/spawn/static_pipe_writer.rs (removes cfg(not(windows)) gates on the on_write release, adds a Windows-only release in on_close), src/runtime/api/bun/subprocess.rs (take_pending_start_writer now clears started when it claims the pointer), and src/io/PipeWriter.rs (saturating_sub for the exit-first late-callback ordering — a fix I flagged in a prior run). Adds a Windows-only RSS-delta leak test with two modes covering the two release paths.
Security risks
None. This is internal refcount bookkeeping on a native writer struct; no user-controlled input reaches the changed logic beyond the buffer bytes themselves, which are only written to the child's stdin pipe as before.
Level of scrutiny
High. This is exactly REVIEW.md's most-blocked category — intrusive refcounts with multiple release sites made mutually exclusive via a started boolean, across three consumers (Subprocess, ShellSubprocess, SecurityScanSubprocess) and two platform-specific writer backends with different callback orderings. The PR description traces every path carefully and my own tracing agrees, but the correctness argument leans on libuv delivery-order guarantees (queued write callback after uv_close fires with real status, not ECANCELED) and on write()'s scopeguard keeping the object live across the new on_close deref. A maintainer with Windows-spawn context should confirm those invariants.
Other factors
All three of my earlier inline findings (error-arm leak, PipeWriter.rs:1513 debug underflow, test.concurrent) were addressed in follow-up commits, and all CodeRabbit threads are resolved. The new test has verified fail-before (USE_SYSTEM_BUN=1: 2.5 MB / 28 MB) and pass-after (~0 KB / ~350 KB) numbers in the PR body, with ASAN/debug-branched thresholds well below the unfixed leak. The POSIX on_close gate is explicitly justified (POSIX drain_buffered_data calls on_error then on_write with no extra ref held) with a note that #34697 will let it be dropped.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/spawn/static_pipe_writer.rs`:
- Line 318: Remove the unconditional eprintln!("SPW_DROP") debug print from the
buffer-backed writer destruction path in static pipe writer handling, leaving
the destruction behavior otherwise unchanged.
In `@test/js/bun/spawn/spawn-buffer-stdin-leak.test.ts`:
- Line 40: Update the test suite declaration for the leak measurements from
describe.concurrent.skipIf to describe.skipIf, preserving the existing Windows
condition so the two 6,000-process fixtures run sequentially.
🪄 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: e9a851c1-1dfa-41b9-bc73-c908a99884e9
📒 Files selected for processing (5)
src/io/PipeWriter.rssrc/runtime/api/bun/subprocess.rssrc/spawn/static_pipe_writer.rstest/js/bun/spawn/spawn-buffer-stdin-leak-fixture.tstest/js/bun/spawn/spawn-buffer-stdin-leak.test.ts
0be6a78 to
ddf245e
Compare
There was a problem hiding this comment.
No new findings from me. My earlier comments (debug-only saturating_sub underflow, test.concurrent, and the leftover eprintln!("SPW_DROP")) are all addressed in the current diff — the temp print is gone and Drop now decrements a proper LIVE_COUNT atomic that the reworked test observes directly instead of RSS. Deferring to a human on the core change: the started-token refcount balance now spans three mutually-exclusive release sites (on_write, Windows-only on_close, and take_pending_start_writer) across several libuv callback orderings, which is squarely in the intrusive-refcount / lifecycle territory that warrants a maintainer's eyes.
What was reviewed: traced start()'s +1 through all three release sites for the write-first, exit-first, and error-arm orderings on Windows and confirmed each clears started before deref so no double-release; checked that on_write's ungated release on POSIX is behavior-preserving (it was already cfg(not(windows))); confirmed SecurityScanSubprocess and ShellSubprocess owners are not broken by the new on_close release (both see started == false or benefit from the fix); verified the new LIVE_COUNT atomic is only read via bun:internal-for-testing and the fixture polls with a bounded loop rather than a bare sleep.
Extended reasoning...
Overview
The PR fixes a Windows-only per-spawn leak of StaticPipeWriter when Bun.spawn is given an ArrayBuffer/Blob stdin. The fix un-gates the release_start_ref block in StaticPipeWriter::on_write (previously #[cfg(not(windows))]), adds a Windows-only release in on_close for the error arm that never reaches on_write, and makes Subprocess::take_pending_start_writer clear started when it claims the pointer so the three sites are mutually exclusive. It also adds a saturating_sub in WindowsBufferedWriter::on_write_complete to avoid a debug-only underflow on the exit-first ordering, a LIVE_COUNT atomic exposed via bun:internal-for-testing, and a Windows-only leak test that asserts the live count returns to baseline.
Since my last review pass the temp eprintln!("SPW_DROP") was removed and the test approach was reworked from RSS deltas (6000 spawns, 120s timeout) to a direct live-count assertion (400 spawns, 60s timeout, serial), which is both faster and exact.
Security risks
None identified. No user-controlled data reaches new parsing or allocation paths; the change only adjusts when an existing intrusive deref fires. The new LIVE_COUNT static and its JS binding are read-only introspection gated behind bun:internal-for-testing.
Level of scrutiny
High. This is native intrusive-refcount lifecycle code — REVIEW.md's most-blocked category — on a Windows path the author cannot run locally (per the evidence footer). The correctness argument depends on which of on_write_complete (success/error), on_close, and on_process_exit fire in which order, and on write()'s scopeguard +1 keeping the object live past each new deref. The reasoning in the PR description and inline comments is detailed and I did not find a hole in it, but a wrong call here is a UAF or a re-introduced leak, so a maintainer familiar with the Windows spawn/libuv lifecycle should confirm the ordering claims (in particular that on_close on Windows is always reached under write()'s scopeguard or another outstanding ref).
Other factors
- All prior review-thread items (mine and CodeRabbit's) are addressed in the current diff; the one CodeRabbit thread still shown unresolved (SPW_DROP) is stale — the line is gone.
- POSIX behavior is unchanged: the
on_writerelease was already active there, and the newon_closerelease is#[cfg(windows)]. - The new test is Windows-only, uses a bounded poll for callback drain, checks child exit codes per batch, and asserts an exact
leaked == 0against the native counter — no RSS thresholds to flake on. ShellSubprocessandSecurityScanSubprocessshareStaticPipeWriter<P>; the PR description accounts for both, and the changed code is generic overPso neither needed a per-owner edit.
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Delete the test and instrumentation code
…l write path On Windows, Bun.spawn/spawnSync with an ArrayBuffer/Blob stdin leaked one StaticPipeWriter per spawn. StaticPipeWriter::start() takes a +1 and sets started=true. #30875 added a release_start_ref block to on_write() that clears started and derefs once the buffer drains, but gated it behind cfg(not(windows)). On Windows both terminal arms of WindowsBufferedWriter::on_write_complete reach on_close_io (via close() -> on_close_source -> StaticPipeWriter::on_close) with started still set, flipping stdin to Writable::Ignore so the other release site (take_pending_start_writer, called from on_process_exit / close_io) matches nothing and start()'s +1 is stranded. - on_write: drop the cfg(not(windows)) gates so release_start_ref runs on every platform. - on_close (Windows only): claim started for paths that reach close() without going through on_write (the on_write_complete error arm). write()'s +1, held by that callback's scopeguard, keeps self live past the deref. Gated to Windows because POSIX drain_buffered_data may call on_error() -> close() -> on_close and then on_write() on the same object with no extra ref held. - take_pending_start_writer: clear started when it claims the pointer so a uv_write whose I/O already completed before uv_close (delivered with its real status, not ECANCELED) does not double-release. - WindowsBufferedWriter::on_write_complete: saturating_sub for has_pending_data so the late-success-after-close ordering does not underflow in debug builds.
f8f4ec7 to
87ce158
Compare
|
Removed the test files and the live-count instrumentation; the diff is now just the three src/ changes. |
…ails on POSIX (#37774) ### Problem - On POSIX, a Buffer or Blob passed as a child's `stdin` leaks the native writer that pumps it into the child when the write fails before the buffer drains, typically `EPIPE` because the child closed its stdin without reading it. - Reached by the shell's `cmd < ${buffer}` redirect on every POSIX configuration, and by `Bun.spawn` always on macOS but on Linux only when memfd is unavailable. - Under LeakSanitizer the shell's stranded writers appear as `Direct leak of 1024 byte(s) in 2 object(s)` from `StaticPipeWriter<ShellSubprocess>::create`; with refcount logging the count stays at 1 forever after `onError(err=EPIPE: Broken pipe (send()))` and `onClose()`. - Cause: the writer holds a ref on itself while a write is in flight, and only a drained buffer or the owner closing it at exit released that ref. After a failed write the writer closes itself and the owner empties its slot, so nothing that could release the ref can still reach the writer. ### Fix - The writer's close callback releases the in-flight ref on every platform, not only Windows. Sound because close is the last callback a parent receives for an error, and every release site claims the same token first, so exactly one of them (drain completion, close after a failure, or the owner) releases. - The POSIX drain loop now only writes and cannot invoke callbacks itself; an error is always returned and reported once by the caller. This deletes the arm that #35297 cited when it fixed the same leak on Windows only; the arm was unreachable, so no behaviour changes here. - Two rarer error paths get the same report-then-close shape: a failed poll re-registration used to report without closing, and a zero-byte write closed without releasing. Both are reasoned about, not tested. Out of scope, pre-existing: on Windows a write failing synchronously inside `start()` still strands the ref; tracked separately. - Verification: a new ASAN-lane test runs a fixture under `detect_leaks=1` that drives both owners through failing and draining writes. It fails 4 of 4 runs without the fix with the report above and passes 5 of 5 with it. LSan only reports the shell-owned writers; the `Bun.spawn` cases exercise the same teardown without being what fails. ### Background - The static pipe writer is the native object that writes one fixed in-memory buffer (a Buffer or Blob `stdin`) into a child's stdin pipe. It is refcounted: one ref lives in the owning process's stdin slot, a second is taken when the write starts so the object outlives the write. - It has two owners: a `Bun.spawn` subprocess and a shell subprocess for the `< ${buffer}` redirect. On Linux `Bun.spawn` normally hands the child a memfd and never creates the writer, which is why the test disables memfd; the shell redirect always uses it. - The buffered pipe writer underneath reports to its parent through three callbacks: bytes drained, error, closed. Parents are written assuming closed is the last one they receive, so freeing the object there is safe only if nothing underneath touches it afterwards. - The `started` flag is the token for the in-flight ref: whichever site swaps it to false does the release, so three possible release sites cannot release twice. - `EPIPE` is what writing to a pipe returns once the reader has closed its end; it is how a child closing its stdin reaches the parent while the buffer is still being written. <!-- robobun:evidence:begin --> --- **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/spawn/spawn-stdin-pipe-fd-leak.test.ts <!-- robobun:evidence:end --> <details> <summary>Original description</summary> On POSIX, a Buffer/Blob `stdin` whose write fails before it has drained (typically EPIPE, because the child closed its stdin without reading it) leaks the native `StaticPipeWriter` that pumps the buffer into the child. Two owners share that writer: the shell's `cmd < ${buffer}` redirect, which uses it on every POSIX configuration, and `Bun.spawn`, which on Linux only uses it when memfd is unavailable (`BUN_FEATURE_FLAG_DISABLE_MEMFD=1`, or no `memfd_create`; otherwise the child gets a memfd) and on macOS always. ### Repro ```ts // BUN_FEATURE_FLAG_DISABLE_MEMFD=1 BUN_DEBUG_StaticPipeWriter=1 BUN_DEBUG_ref_count=1 bun-debug run repro.ts const big = Buffer.alloc(1 << 20, 0x61); const proc = Bun.spawn({ cmd: ["sh", "-c", "exec 0<&-; sleep 0.3"], stdin: big, stdout: "ignore" }); console.log("exit", await proc.exited); ``` Before, for the writer: ``` StaticPipeWriter(0x..) start() 0x.. ref 1 -> 2 start() StaticPipeWriter(0x..) onError(err=EPIPE: Broken pipe (send())) StaticPipeWriter(0x..) onClose() 0x.. deref 2 -> 1 owner drops create()'s ref in on_close_io ``` and the count stays at 1 forever. After, the same trace continues with `deref 1 -> 0`. `$\`cmd < ${big}\`` with a command that exits without reading stdin shows the same before/after (no memfd flag involved), and under LeakSanitizer the shell's stranded writers come out as ``` Direct leak of 1024 byte(s) in 2 object(s) allocated from: ... #12 <StaticPipeWriter<ShellSubprocess>>::create src/spawn/static_pipe_writer.rs:110 #13 <shell::subproc::Writable>::init src/runtime/shell/subproc.rs:1149 ``` ### Cause `start()` takes a ref on the writer and sets `started`; that ref was released from `on_write` when the buffer drained, or by the owner (`Subprocess::on_process_exit` / `close_io`) if it closed the writer while the slot still held it. A failed write takes neither route: `PosixBufferedWriter::_on_error` runs `on_error` and then `close()`, `on_close` tells the owner, the owner empties its slot and drops `create()`'s ref, and nothing is left that can find the writer to release `start()`'s ref. #35297 fixed exactly this on Windows by releasing in `on_close`, and left POSIX alone because of the shape of `PosixPipeWriter::drain_buffered_data`: it had an arm that, for an error after a partial drain, called `on_error` inline (closing the writer) and returned `Wrote(n)`, after which `on_poll` delivered `on_write(n, Drained)` to the same object, so a release in `on_close` looked unsafe. That arm was in fact unreachable (`try_write` reports a short write as `Pending`, never as `Wrote`, so the drain loop never gets to an error with bytes already drained), but as written it was the documented reason the POSIX release site did not exist. ### Fix * `drain_buffered_data` (src/io/PipeWriter.rs) loses that arm and takes `&self`, so it structurally cannot dispatch callbacks: an error is always returned as `Err` and reported once by the caller, and `on_close` is the last callback a parent sees. That is the contract the parents are written against (`FileSink::on_close` releases its keep-alive as "the last thing", `Terminal::on_writer_close` derefs the terminal, `FileSink::on_auto_flush` documents `flush()` returning `Err` as the error contract); this makes it explicit rather than a property of which arms happen to be reachable. No behaviour changes here. * `StaticPipeWriter::on_close` (src/spawn/static_pipe_writer.rs) claims and releases `start()`'s ref on every platform, not just Windows. This is the behavioural fix. The release is the last access: the frames underneath it (`close_impl` -> `close` -> `_on_error` -> `on_poll`) do nothing with the writer after the callback, which is the same shape `SecurityScanSubprocess` (whose owner ref is the last one) already relies on in `on_close_io` today. `on_write` keeps taking the token before it closes and the owners keep taking it before they close, so exactly one site releases the ref. * `PosixBufferedWriter::register_poll` reports a failed re-registration through `_on_error` (report, then `close()`), like every other error report and like the streaming writer's `register_poll` already did, instead of reporting and leaving the writer open. Raised in review: that (ENOMEM-class) failure was the one report not followed by the `on_close` that now releases the ref; `Bun.spawn` would still have picked the writer up at exit, the shell had nothing to pick it up with. Reasoned about rather than tested, since it needs the registration syscall itself to fail. * The `EndOfFile` arm of `on_write` releases the ref as well (non-final, since the owner's ref is dropped by the `on_close` the buffered writer delivers right after) instead of closing the writer itself and leaving the ref outstanding. That arm needs `write(2)` to return 0 on a pipe, so it is reasoned about rather than tested; it is the same change #37755 makes to this arm. Known gap, pre-existing and not touched here: on Windows a `uv_write` that fails synchronously inside `start()` closes the writer (`on_close` runs, the owner drops its ref) before `start()` sets `started`, which strands the ref the same way. Fixing it needs `SecurityScanSubprocess`'s post-`start()` release to become token-aware as well, so it is tracked separately. Related: #34697 proposes the same `drain_buffered_data` change as one item of a broader hardening pass, but is 600+ commits behind with conflicts in three files. #37755 moves these callbacks onto raw `this` pointers and leaves the error chain (and this leak) alone; the two conflict textually in `on_close` / `on_poll` but compose, whichever lands second needs to keep the unconditional release in `on_close` and the `Err` return in `drain_buffered_data`. ### Test `test/js/bun/spawn/spawn-stdin-pipe-fd-leak.test.ts` runs a fixture on the ASAN lane with `detect_leaks=1` (the setup from shell-worker-terminate-leak.test.ts). The fixture drives both owners through both endings: children that close their stdin, report it and wait to be killed (so the write fails while the child is alive and the exit path cannot be what cleans up), and children that drain it; memfd is disabled so that the `Bun.spawn` scenarios reach the writer at all. Unfixed it fails every time (4 of 4) with the report quoted above: the two shell-owned writers. The two stranded `Bun.spawn`-owned writers from the same run have their refcount stuck at 1 just the same (checked with `BUN_DEBUG_ref_count`, and their `Subprocess` structs are freed before exit), but LSan still finds their address somewhere and does not report them, so those scenarios only exercise the same teardown under ASAN. Fixed, the fixture is clean, 5 of 5 runs, about 0.7s each. An earlier revision used a live-writer counter in `bun:internal-for-testing`; it was removed per review in favour of LSan. ### Verification Debug (ASAN) build: the file above, plus spawn.test.ts, spawnSync.test.ts, spawn-empty-arrayBufferOrBlob, spawn-streaming-stdin, spawn-stdin-readable-stream, spawn-stdin-destroy, spawn-pipe-start-error, spawn-many-teardown, memfd-disabled, bunshell.test.ts (418 pass), bunshell-instance, epipe, shell-blocking-pipe, shell-write-fault, file-io, pipeline_stack, filesink.test.ts, terminal.test.ts, terminal-spawn, child_process.test.ts and bun-security-scanner-workspaces all pass. `cargo check` of bun_io and bun_spawn for `x86_64-pc-windows-msvc` and `aarch64-apple-darwin`, `cargo clippy` on both, and `cargo fmt --check` are clean. CI on the two previous revisions (the first of which ran its tests on every lane, Windows included) was green apart from flaky tests that passed on retry. </details>
2 participants
On Windows,
Bun.spawn/Bun.spawnSyncwith a non-emptyArrayBuffer/Blobstdinleaked oneStaticPipeWriterallocation per spawn.Cause
StaticPipeWriter::start()takes a+1and setsstarted = true. #30875 added arelease_start_refblock toon_write()that clearsstartedand derefs once the buffer drains, but gated it behind#[cfg(not(windows))]:https://github.com/oven-sh/bun/blob/892b1dabc6/src/spawn/static_pipe_writer.rs#L228-L241
On Windows, when the
uv_writecompletes before the child exits (the common ordering for a small buffer):Net:
startedis stilltruebutstdinisWritable::Ignore, so the other release site,take_pending_start_writer(called fromon_process_exit/close_io), matches nothing and start()'s+1is stranded. The error arm ofon_write_complete(e.g.ERROR_BROKEN_PIPEwhen the child closes stdin without reading) reacheson_closeviaclose()without ever callingParent::on_write, so start()'s+1is stranded there the same way.ShellSubprocesshas the same leak forcmd < ${buf}stdin since itson_process_exit/close_ionever look atstartedat all and depend entirely onon_write's release.Fix
StaticPipeWriter::on_write: drop thecfg(not(windows))gates sorelease_start_refruns on every platform.WindowsBufferedWriternever passesEndOfFile, so the existingstatus != EndOfFileguard is a no-op there and is left for the POSIX path (PosixBufferedWriter::_on_writestill touchesselfafter the callback onEndOfFile).StaticPipeWriter::on_close(Windows only): claimstartedand deref afteron_close_iofor paths that reachclose()without going throughon_write(theon_write_completeerror arm).write()'s +1, held by that callback's scopeguard, keepsselflive past the release. This is gated to Windows because on POSIXdrain_buffered_datamay callon_error()->close()->on_closeand thenon_write()on the same object with no extra ref held; io: harden POSIX pipe writer error paths against owner-dropping callbacks #34697 changes that ordering and the gate can go once it lands.Subprocess::take_pending_start_writer: clearstartedwhen it claims the pointer. The three release sites are then mutually exclusive viastarted, covering the ordering whereon_process_exitcloses the pipe while auv_writewhose I/O already completed is still queued (libuv delivers that callback with its real status afteruv_close, notECANCELED).WindowsBufferedWriter::on_write_complete:saturating_subforhas_pending_dataso the late-success-after-close ordering does not underflow in debug builds.SecurityScanSubprocessalready derefs and writesstarted = falseimmediately afterstart(), so both new release sites seestarted = false; no change in its refcount balance.Verification
Verified on Windows x64 by instrumenting
Dropwith a live counter and spawning 400 children per mode: unfixed leaves 400 live writers after each of the drain (64 B intosort) and reject (256 KB intocmd /c exit) workloads; fixed returns to 0 on both, in release and debug builds.Also verified on Windows x64:
spawn.test.ts -t 'stdin|Uint8Array|Blob'(45/45),spawnSync.test.ts(6/6),file-io.test.ts(26/26),child_process.test.ts(41/41), and on Linux:bunshell.test.ts(414/414),bun-security-scanner-workspaces.test.ts(3/3).rust:check-allis green across all 10 targets.Found while working on #35286, which scopes its change to the empty-buffer path and does not overlap.