spawn: hold the Subprocess as an OwnedRef across spawn_maybe_sync

[robobun]

Problem

• Nothing user-visible breaks on Bun.spawn. This is a lifetime cleanup of the one function that builds a Subprocess for both spawn and spawnSync.
• It held the new Subprocess as &mut for its whole length, but what it wires up re-enters the object before it returns (stdin setup runs JS, an aborted signal fires at once, spawnSync runs a whole event loop). Each collision had its own raw-pointer or unsafe workaround.
• The ref count was started at 2 with a comment naming the two holders it stood for, and releases were hand-written deref() calls and raw pointer dereferences, so every exit path had to balance the count by hand.
• One real leak: on the spawnSync path, exits that throw after its event loop has finished returned without tearing the Subprocess down, leaking it with its Process and buffers.

Fix

• Commit 1 binds it as &Subprocess (every field is Cell-style, built for shared access), so the workarounds become plain field access. Commit 2 holds the allocation as an OwnedRef: the function's ref is a local, the exit handler and the JS wrapper each clone() one where they are installed, and a release is a drop.
• Property to check: every exit drops the function's ref exactly once, so after return the count is the same 2 as before (spawnSync has no wrapper and releases the function's ref at teardown).
• spawnSync teardown runs from a scope guard armed once its event loop has drained, so the throwing exits tear the Subprocess down too. This is the only behaviour change. The failed-watch() exit notification holds its own ref instead of a raw pointer behind a flag.
• Verification: cargo check and clippy clean (Windows target too), ASAN debug build, existing spawn suites pass (spawn.test.ts plus eleven others, 0 failures). No new test; the leak fix has no test of its own. spawn-pipe-leak.test.ts timed out on the loaded ASAN build, RSS flat as far as it got.

Background

• Stacked on bun_ptr: add OwnedRef, an intrusive ref that is released on drop #37665: that PR's branch is this PR's base, so the diff here is only these two commits.
• Subprocess is intrusively ref-counted (bun_ptr::RefCount): the count lives inside the object and the allocation is freed when the last holder releases. Holders are the JS wrapper, the process exit handler, and open stdio pipes.
• OwnedRef<T> (bun_ptr) is one counted ref held as a value: new takes the first ref, clone takes another, drop releases, and into_raw / from_raw carry a ref across an FFI or callback boundary without touching the count. It derefs to &T and is one pointer wide.
• The JS wrapper keeps its ref as an opaque pointer, and the generated finalizer hands it back as a Box<Subprocess>. That is why finalize turns the Box back into an OwnedRef instead of dropping it: other holders may still be alive.
• spawnSync never creates a JS wrapper. It runs an isolated event loop until the child exits, builds a plain result object, and has to tear the Subprocess down itself.

Original description

Stacked on #37665 (the base of this PR is that branch). Two commits: the original one, which binds the Subprocess as &Subprocess instead of &mut across spawn_maybe_sync, and, following review ("Can we use a better RAII container type instead?"), a second one that holds it as an OwnedRef.

What

1. &Subprocess, not &mut (first commit)

spawn_maybe_sync materialised the freshly boxed Subprocess as &mut and kept that borrow live to the end of the function, although the code in between hands out back-pointers to the same allocation and re-enters it (Writable::init runs JS, PipeReader::start can reach on_close_io, AbortSignal::add_listener fires immediately for an already aborted signal, the spawnSync path runs its whole isolated event loop). Each collision had its own workaround: an unsafe block writing the stdin source through the raw pointer plus BackRef::from_raw, a lifetime-erasing cast plus null check for subprocess_nn, a subprocess_ptr_exit copy for the exit-notification guard, raw writes to abort_signal. The binding became the shared form every field of Subprocess is built for (Cell/JsCell, the R-2 convention on the struct), Writable::init takes &Subprocess (it only called &self methods and cast to *mut at its four StaticPipeWriter::create sites, which now get the existing as_ctx_ptr()), and the workarounds became plain field access.

2. The allocation is an OwnedRef (second commit)

What was left was the container itself: heap::into_raw(Box::new(..)), an unsafe { &*ptr } for the body, a count constructed as init_exact_refs(2) with a comment saying which two holders the 2 stood for, two hand-written deref() calls on the Writable::init error path, a raw pointer dereference inside the exit-notification defer!, and Box::from_raw at the end of the spawnSync path so that finalize(Box<Self>) could be called. Now:

let spawn_ref = OwnedRef::new(SubprocessT { ref_count: RefCount::init(), .. });   // the ref this function holds
let subprocess: &SubprocessT<'static> = &spawn_ref;                               // what the body uses
...
process.set_exit_handler(ProcessExit::new(Subprocess, spawn_ref.clone().into_raw()));  // the handler's ref
let out = SubprocessT::to_js_from_ref(spawn_ref.clone(), global_this);                 // the wrapper's ref

• spawn_ref is the ref this function holds while it wires the object up, so the body reads it safely and it is alive for every use; on the async path it drops at every exit. The exit handler and the JS wrapper take their own refs where they are installed (on_process_exit and finalize release them, as before), so once the function returns the count is the same 2 as before. The Writable::init error path, where neither exists yet, is drop(spawn_ref) instead of two deref() calls, and the Windows IPC setup error path (the one other place that released the wrapper's stand-in ref by hand) loses its deref(), since spawn_ref dropping on the way out is that release now; with it left in, that path would have released one ref too many.
• to_js_from_ptr(*mut) becomes to_js_from_ref(OwnedRef<Self>), which into_raws the ref into m_ctx; finalize(self: Box<Self>), the codegen's spelling of that same ref, turns it back into an OwnedRef and calls the new finalize_owned, which does the teardown and drops it. The spawnSync path calls finalize_owned with spawn_ref itself, since there the function's own ref plays the wrapper's part; it does so from a scopeguard armed once the isolated loop has finished, so the throwing exits of that tail (has_exception, the ? on to_buffered_value and on create_resource_usage_object) tear the Subprocess down too, where until now they returned without finalize and leaked it together with its Process and buffers. That is the one behaviour change; the state at those exits is the same as at the normal finalize call a few lines further down (the loop has drained), and the normal exit drops the guard at exactly the point where finalize was called before.
• The exit-notification guard for a failed watch() holds an Option<OwnedRef<..>> (a clone taken only on that path) instead of dereferencing a raw pointer under a flag, so the guard is safe code and the object is alive by construction when it runs.
• The IPC owner is built from spawn_ref.as_non_null() directly, so the subprocess_ipc_owner helper (a null check on a pointer that cannot be null) is gone; the abort listener is registered with as_ctx_ptr(), the same expression clear_abort_signal uses to unregister it; record_stdio_pipe_ownership (Windows) gets as_ptr().

Across both commits js_bun_spawn_bindings.rs goes from 32 unsafe blocks to 28 (the BackRef::from_raw write, &*subprocess_ptr, the guard's dereference, Box::from_raw) and subprocess.rs gains the one in finalize; the 4 &mut-to-*mut casts in Writable::init are gone, and the ref_count comment explaining the 2 is replaced by the two clones at the two installation sites. No dependency changes.

Why

Who holds a ref on a Subprocess during spawn, and for how long, is now spelled by values: the function's ref is a local, the handler's and the wrapper's are clones taken where they are installed, and a release is a drop, so none of the exits can forget one or release one twice; the unsafe blocks left in this stretch are about other handles (the VM pointer, the AbortSignal, the Process ref, ProcessExit::new's dispatch contract), none of them about whether the Subprocess itself is still alive. The cost on the Bun.spawn path is one extra non-atomic increment/decrement pair on the count (the wrapper's ref is a clone rather than the function's ref renamed), next to a fork/exec; spawnSync performs exactly the count operations it did before, and OwnedRef is a NonNull, so nothing changes layout.

Part of a series of small type-system hardening changes.

Verification

cargo check and cargo clippy are clean for bun_runtime, and cargo check --target x86_64-pc-windows-msvc passes for it as well (the IPC path above is Windows-only). Debug (ASAN) build succeeds. bun bd test on spawnSync.test.ts, spawn-signal.test.ts, spawn.ipc.test.ts, spawn-pipe-start-error.test.ts, spawn-many-teardown.test.ts, spawn-maxbuf.test.ts, spawnsync-isolated-event-loop.test.ts, spawn-stdin-destroy.test.ts, exit-code.test.ts, spawn-stdin-readable-stream.test.ts and spawn-noread-leak.test.ts: 101 pass, 3 skip, 0 fail; spawn.test.ts run on its own: 140 pass, 6 skip, 0 fail. spawn-pipe-leak.test.ts (which is todo under ASAN in CI) hits its 30 s per-test timeout on this ASAN debug build on a loaded machine, at about 9 s per batch of 50 children; the RSS it prints stays flat across the batches it gets through (605, 611, 596, 598, 597 MB), which is what the test measures. The first commit had been verified earlier with spawn.test.ts, spawnSync.test.ts, spawn-signal.test.ts and spawn-stdin-readable-stream.test.ts: 200 pass, 8 skip, 0 fail.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9251236d-7228-47e6-abdc-73ccb208aeaa

📥 Commits

Reviewing files that changed from the base of the PR and between e7abdf7 and 6f866ac.

📒 Files selected for processing (2)

• src/runtime/api/bun/js_bun_spawn_bindings.rs
• src/runtime/api/bun/subprocess/Writable.rs

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 5:53 AM PT - Aug 12th, 2026}

✅ @robobun, your commit 835077dd3c0304aa2149c6854d122869fe0b086a passed in Build #92935! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37700

That installs a local version of the PR into your bun-37700 executable, so you can run:

bun-37700 --bun

[claude]

I reviewed this PR and didn't find any bugs. Because it reworks unsafe pointer/aliasing invariants in spawn_maybe_sync (provenance of subprocess_nn, the exit-notification guard's liveness reasoning), a human look would still be worthwhile.

Checked: NonNull::from(&Subprocess) provenance is sufficient for all consumers (R-2 interior mutability, BackRef<_, Shared> only yields &T); as_ctx_ptr() contract matches what StaticPipeWriter::create needs; the defer! guard only dereferences subprocess_nn when send_exit_notification is set, which happens only on the !IS_SYNC path where the JS wrapper owns the box past function return; the let-chain reordering in the stdin Pipe block is semantically equivalent; abort_signal.set ordering vs add_listener is unchanged.

Extended reasoning...

Overview

This PR changes spawn_maybe_sync to bind the freshly-boxed Subprocess as &SubprocessT<'static> instead of &mut, collapsing several per-site unsafe workarounds (raw (*subprocess_ptr) writes, a subprocess_ptr_exit copy, lifetime-erasing casts) into plain shared-field access. Writable::init drops its &mut Subprocess parameter to &Subprocess and routes the four StaticPipeWriter::create sites through the existing Subprocess::as_ctx_ptr() helper. Writable::pipe_sink is widened to pub(in crate::api) to match its siblings. Net: −1 unsafe block, −4 &mut→*mut casts, −2 lifetime-erasing casts.

Security risks

None. No user-facing behavior, validation, or resource-handling changes; this is a type-level refactor of how an already-boxed allocation is borrowed.

Level of scrutiny

High. spawn_maybe_sync is memory-safety-critical (the most-blocked category per REVIEW.md): it wires back-pointers that re-enter the same allocation, arms a defer! guard that runs after finalize on the sync path, and hands the box to a JS wrapper on the async path. The change is a strict tightening (&mut → &), but the soundness of NonNull::from(&T) provenance for downstream consumers, and the exit-guard's flag-gated dereference, deserve a maintainer's confirmation.

Other factors

I verified: as_ctx_ptr()'s documented contract ("callbacks deref it as &*const shared") matches the new call sites; BackRef::new and BackRef<_, Shared> only ever yield &T, so shared-read provenance from NonNull::from(subprocess) suffices given all Subprocess mutation goes through Cell/JsCell; send_exit_notification is only set on the !IS_SYNC branch (after to_js_from_ptr), so the guard's subprocess_nn.as_ref() never runs on the spawnSync path where finalize has already freed the box; the stdin Writable::Pipe let-chain checks the same two conditions as the old matches! && cond { if let } shape; the two abort_signal.set calls still happen after add_listener. 208 spawn tests pass per the description. No prior review comments to address.

[Jarred-Sumner]

Can we use a better RAII container type instead?

[robobun]

Reworked in 8e980dc, now stacked on #37665 (OwnedRef), as a second commit on top of the &Subprocess one. The allocation is held as an OwnedRef for as long as spawn_maybe_sync uses it; the exit handler and the JS wrapper each take a clone where they are installed (to_js_from_ptr became to_js_from_ref(OwnedRef)), the Writable::init error path is a drop instead of two deref() calls, the failed-watch() guard holds a ref instead of a raw pointer, and the spawnSync tail hands its ref to finalize_owned through a scope guard instead of Box::from_raw + finalize, which also means the throwing exits of that tail no longer leak the Subprocess. finalize(Box<Self>) converts the wrapper's Box back into the same OwnedRef and shares finalize_owned. The count ends up at the same 2 as before once the function returns; the only extra work is one increment/decrement pair on the Bun.spawn path. Details and test results are in the description.

[claude]

I re-reviewed after 835077d and found no further issues. The Windows IPC error-path fix is correct: with the stray subprocess.deref() removed, that return leaves the exit handler's ref as the sole holder, matching the pre-PR state. Given this reworks the ref-count model across every exit of spawn_maybe_sync (a path where the earlier revision did miss a cfg-gated release site), a human pass over the balance would still be worthwhile.

What was checked:

• Ref-count balance on every exit of spawn_maybe_sync for both IS_SYNC values, including the Writable::init error arm, the deliver_exit guard, and the spawnSync scopeguard tail — all end at the same holders as the old init_exact_refs(2) model.
• Drop ordering: the deliver_exit guard drops before both deliver_exit and spawn_ref, so the Subprocess is alive when the deferred on_exit runs.
• finalize_owned vs old finalize: the internal this.deref() calls only fire when their matching refs exist, so owned cannot dangle before its own drop.

Extended reasoning...

Overview

This PR replaces the raw-pointer + init_exact_refs(2) ownership of Subprocess in spawn_maybe_sync with an OwnedRef held by the function itself, from which the exit handler and JS wrapper each take an explicit clone at their installation sites. to_js_from_ptr becomes to_js_from_ref(OwnedRef), finalize(Box<Self>) reconstructs the wrapper's OwnedRef and delegates to a new finalize_owned, the failed-watch() guard carries its own ref instead of dereferencing a raw pointer, and the spawnSync tail arms a scopeguard so the throwing exits (has_exception, to_buffered_value?, create_resource_usage_object?) also tear the Subprocess down. Writable::init now takes &Subprocess and uses as_ctx_ptr() at its four StaticPipeWriter::create sites. Net: 4 fewer unsafe blocks in js_bun_spawn_bindings.rs, one added in subprocess.rs (the Box → OwnedRef reconstruction in finalize).

Prior review

My earlier pass flagged one bug: the Windows-only windows_configure_server error path still had a hand-written subprocess.deref() that, combined with spawn_ref's new Drop, would over-release and free the Subprocess while the exit handler still held a raw pointer. Commit 835077d removed that deref(), and I verified that under the new model this leaves the count at exactly the exit handler's ref on that return — the same state the old code left. The author also audited every remaining .deref() on this count (on_stdin_destroyed, the two in Writable::init's ReadableStream error arms, on_process_exit, the two in finalize_owned) and each pairs with a ref that still exists under the new model. My second finding (no test for the spawnSync-tail leak fix) was answered: those exits require a pending termination exception at a specific point or an OOM building the result, neither of which can be made deterministic; the fix is documented rather than guarded by a flaky test.

Security risks

None. This is an internal ownership refactor; no user-facing surface, parsing, or trust boundary changes.

Level of scrutiny

High. Bun.spawn/spawnSync are among the most-exercised APIs, the function has many exits (some cfg-gated), and the change swaps a pre-counted model for an explicit-clone model — every hand-written release site had to be re-evaluated. The first revision did miss one Windows-only site, which is exactly the failure mode REVIEW.md warns about for ref-count model changes ("reference counts provably balanced on every terminal path"). The current revision looks correct across every path I traced, but a maintainer familiar with the spawn lifecycle should confirm.

Other factors

The PR is stacked on #37665 (OwnedRef itself). The listed spawn test suites pass under ASAN debug. cargo check --target x86_64-pc-windows-msvc was run for the Windows path. No new tests, which is defensible for a behaviour-preserving refactor whose one incidental behaviour change (spawnSync-tail leak) is not deterministically reachable.