Bun.spawn: don't double-close extra stdio fds exposed via .stdio

[robobun]

What

With stdio[N>=3] = "pipe", Bun.spawn creates a socketpair and stores the parent end as ExtraPipe::OwnedFd. The .stdio getter exposes that fd as a raw number to JS, but Subprocess::finalize_streams still closed the same number at GC time.

If the caller had already closed it (the only way to release it or signal EOF to the child), the kernel may have recycled the value for an unrelated fd, which the finalizer then silently destroys. On debug-assert builds the second close trips debug_assert!(err.is_none()) in FdExt::close.

Reproduction

import * as fs from "node:fs";
let p = Bun.spawn({
  cmd: ["/bin/sh", "-c", "printf hi >&3"],
  stdio: ["ignore", "ignore", "ignore", "pipe"],
});
await p.exited;
const fd = p.stdio[3];            // raw number, no stream object
fs.closeSync(fd);                 // caller releases its only handle
const victim = fs.openSync(process.execPath, "r");  // recycles the same number
p = null; Bun.gc(true); await Bun.sleep(0); Bun.gc(true);
fs.fstatSync(victim);             // EBADF: the finalizer closed it

Fix

Downgrade OwnedFd to UnownedFd in the .stdio getter so the caller becomes the sole owner once the number is observed. If .stdio is never read, finalize_streams still closes the fd (no leak). This mirrors the ownership-transfer-on-getter pattern already used for stdin/stdout/stderr via ObservableGetter, and matches what "socket-fd" (used by node:child_process) already does at return time.

Verification

• bun bd test test/js/bun/spawn/spawn.test.ts -t "'pipe' at index >= 3" passes
• Same test fails on an unfixed build with finalizer closed 3/4 recycled fds
• Full close handling describe (83 tests) passes

---

no test proof · iteration 1 · 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

[coderabbitai]

Walkthrough

This PR clarifies and enforces fd ownership semantics when reading Subprocess.stdio on POSIX. get_stdio now downgrades OwnedFd entries to UnownedFd after exposing raw fd numbers, so finalizeStreams no longer closes caller-owned fds. Comments, type docs, and a regression test were updated accordingly.

Changes

FD Ownership Downgrade on stdio Access

Layer / File(s)	Summary
Ownership downgrade implementation and docs src/runtime/api/bun/subprocess.rs, src/spawn_sys/spawn_process.rs, packages/bun-types/bun.d.ts	get_stdio downgrades ExtraPipe::OwnedFd entries to ExtraPipe::UnownedFd after exposing raw fd numbers on non-Windows platforms; comment on OwnedFd and TypeScript Subprocess.stdio docs updated to describe this ownership transfer.
Regression test for finalizer fd handling test/js/bun/spawn/spawn.test.ts	New Windows-skipped test verifies that closing a raw fd exposed via proc.stdio[3] after reading it does not cause finalization to close/recycle a later-opened victim fd.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly and concisely describes the main fix: preventing double-close of extra stdio fds exposed through .stdio.
Description check	✅ Passed	The description covers what changed and how it was verified, though it uses custom headings instead of the exact template labels.

---

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

[robobun]

Reproduced on 1.4.0 release and current main: with stdio: [..., "pipe"], proc.stdio[3] returns a raw fd number that the GC finalizer closes again, destroying whatever recycled it.

Fix: the .stdio getter transfers ownership (downgrades OwnedFd to UnownedFd) so finalize_streams skips it. If .stdio is never read, the finalizer still closes the fd.

Test added in test/js/bun/spawn/spawn.test.ts alongside the existing 'socket-fd' coverage.

---

CI status: the diff is green on every test lane that ran in build 70933 (all linux/musl/asan/alpine, darwin-x64, all windows). The build is marked failing only because two darwin-14-aarch64 test shards expired without an agent picking them up, and the TypeScript types GitHub check is also red on main (pre-existing). Ready for review.

[robobun]

^{Updated 4:08 AM PT - Jul 9th, 2026}

✅ @robobun, your commit 1d5f386372c97644a012180d0cf0d8ad205c41b6 passed in Build #70933! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 33828

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

bun-33828 --bun

[github-actions]

This PR may be a duplicate of:

1. test: run the full ported Node suite leak-clean under the ASAN runner and fix the bugs it surfaces — subprocess fd double-close, console/Strong-handle teardown UAFs, SAN/scope-function/valkey leaks, post-exit child_process streams, runner tmpdir isolation (2322 tests green) #31833 - Also fixes the subprocess fd double-close bug by downgrading OwnedFd to unowned in the .stdio getter, as part of a larger ASAN cleanup PR

🤖 Generated with Claude Code

[robobun]

Not a duplicate of #31833. That PR's child_process.ts change removes the hasSocketsToEagerlyLoad guard so short stdio arrays (length < 3) still eager-load stdout/stderr; its test covers stdio: ["pipe", "pipe"]. It does not touch src/runtime/api/bun/subprocess.rs or the ExtraPipe ownership path.

This PR fixes the Bun.spawn native finalizer double-closing extra stdio slots at index >= 3 when .stdio has handed the raw fd number to JS. Different file, different bug, no overlap in the diffs.

[claude]

I didn't find any bugs, but this changes fd-ownership semantics (reading .stdio now transfers ownership of "pipe" extra fds to the caller, so unread → closed at GC, read-but-not-closed → leak) — worth a quick human sanity check on that trade-off.

Extended reasoning...

Overview

Fixes a double-close of extra stdio fds (index ≥ 3, "pipe") in Bun.spawn. The .stdio getter exposes the parent-side socketpair fd as a raw number; previously finalize_streams would close that same number at GC time, so if JS had already closed it and the kernel recycled the value, the finalizer would silently destroy an unrelated fd (or trip a debug assert). The fix downgrades ExtraPipe::OwnedFd → UnownedFd inside get_stdio after the array is built, so once JS observes the number it becomes the sole owner. A doc comment on ExtraPipe::OwnedFd is updated and a subprocess-based regression test is added.

Touches src/runtime/api/bun/subprocess.rs (getter), src/spawn_sys/spawn_process.rs (comment only), and test/js/bun/spawn/spawn.test.ts.

Security risks

None. This is fd-lifecycle bookkeeping local to the spawning process; no auth/crypto/untrusted-input parsing is involved. The bug being fixed is itself the riskier behavior (closing a recycled fd out from under unrelated code).

Level of scrutiny

Medium-high. The diff is small and mechanically correct — the downgrade loop runs after the fallible array.push(...)? calls (so a mid-loop throw leaves ownership with the finalizer), stdio is not a cached getter in BunObject.classes.ts so re-reads are fine (idempotent), and it's #[cfg(not(windows))]-gated to match finalize_streams. It also mirrors the existing ObservableGetter transfer-on-read pattern for stdin/stdout/stderr and the "socket-fd" return-time downgrade.

That said, this is native fd-ownership code in the spawn path — the exact area CLAUDE.md flags as most-blocked — and it introduces a user-visible semantic: code that reads .stdio for a "pipe" extra slot but never closes the fd will now leak it, whereas before GC would eventually close it. That trade-off (leak vs. double-close-on-recycled-fd) is clearly the right call, but it's a design point a maintainer should nod at rather than have auto-approved.

Other factors

• No bugs from the bug-hunting system.
• Test looks solid: runs in a child so a debug abort surfaces as exit ≠ 0, verifies the fd is actually readable, and probes for the recycled-fd close over 4 iterations. It follows the same {stdout, stderr, exitCode} assertion shape as neighboring tests in the file.
• No CODEOWNERS entries for these paths.

[robobun]

On the trade-off: the old behavior had no correct usage. If the caller closed the fd (the only way to release it or signal EOF to the child), GC double-closed a possibly-recycled number; if the caller never closed, the child never saw EOF until an arbitrary GC. A caller that reads .stdio[3] and then forgets to close now leaks one fd instead of corrupting an unrelated one, which is the standard fs.open contract.

The alternative (expose a proper stream object at stdio[i>=3] instead of a raw number) is a larger API change; happy to go that route instead if preferred.

Updated the JSDoc on Subprocess.stdio in bun.d.ts to document the ownership transfer.

[coderabbitai]

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 `@src/runtime/api/bun/subprocess.rs`:
- Around line 863-873: The `stdio_pipes.with_mut` loop in `subprocess.rs` is
trying to move an `ExtraPipe` out of `&mut ExtraPipe`, which will not compile
because `ExtraPipe` is not `Copy`. Update the downgrade logic to replace each
`slot` in place using `mem::replace` in the `ExtraPipe::OwnedFd` case, and keep
the `OwnedFd` to `UnownedFd` transition inside the same `this.stdio_pipes`
block.

🪄 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: 79822d1c-e073-435c-85ac-27afe49dcac2

📥 Commits

Reviewing files that changed from the base of the PR and between bdd6c1f and d4c1f5e.

📒 Files selected for processing (4)

• packages/bun-types/bun.d.ts
• src/runtime/api/bun/subprocess.rs
• src/spawn_sys/spawn_process.rs
• test/js/bun/spawn/spawn.test.ts

[claude]

Doc nit addressed — the fix itself looks correct and idempotent, but since it changes the ownership contract of a public API (fd now leaks if the caller reads .stdio and forgets to close) and you flagged an alternative design, I'd like a maintainer to sign off on the transfer-on-getter approach.

Extended reasoning...

Overview

Fixes a double-close in Bun.spawn: when stdio[N>=3] = "pipe", the .stdio getter exposes the parent-end socketpair fd as a raw number, but finalize_streams still closed that same number at GC time. If JS had already closed it, the finalizer would close a recycled, unrelated fd. The fix downgrades ExtraPipe::OwnedFd → UnownedFd inside get_stdio after the array is built, so the finalizer skips those slots once JS has observed them. Touches src/runtime/api/bun/subprocess.rs (11 lines added to get_stdio), a doc-comment update in src/spawn_sys/spawn_process.rs, JSDoc in packages/bun-types/bun.d.ts, and a new POSIX-only regression test in test/js/bun/spawn/spawn.test.ts.

Security risks

None introduced. This is strictly a fd-lifetime tightening: it removes a path where the finalizer could close an unrelated recycled fd (which is itself a mild security/correctness hazard). No auth, crypto, or untrusted-input parsing is involved.

Level of scrutiny

Medium-high. The runtime change is small and follows the existing ObservableGetter pattern used for stdin/stdout/stderr, and node:child_process is unaffected (it maps extra "pipe" slots to "socket-fd", which is already UnownedFd). The downgrade is placed after all fallible array.push(?) calls, so an early-return error path leaves ownership with the finalizer (no leak), and repeated .stdio reads are idempotent since the getter is not cached but the loop finds no OwnedFd on the second pass. However, this is native fd-lifetime code and it changes the public ownership contract of Subprocess.stdio — a caller who reads .stdio and never closes the fd now leaks it, whereas before the finalizer would (unsafely) close it.

Other factors

My earlier nit (POSIX-qualify the JSDoc since the downgrade is #[cfg(not(windows))]-gated) was addressed in d4c1f5e and the thread is resolved. The author explicitly offered an alternative design (expose a proper stream object at stdio[i>=3] instead of a raw number) and asked for a preference — that's an API-design call a maintainer should make rather than a bot. The test looks solid (fails on unfixed build per the PR description) and CI is building.

[claude]

Doc nit is addressed — deferring to a human on the API-semantics choice (getter-side-effect ownership transfer vs. exposing a stream object), since you flagged that alternative yourself and it's a fd-lifecycle change in native Subprocess code.

Extended reasoning...

Overview

Four files: an 11-line addition to get_stdio in src/runtime/api/bun/subprocess.rs that downgrades ExtraPipe::OwnedFd → UnownedFd after the raw fd numbers are exposed to JS (POSIX-only), a comment update on the ExtraPipe::OwnedFd variant in src/spawn_sys/spawn_process.rs, a JSDoc rewording on Subprocess.stdio in bun.d.ts, and a new subprocess-in-subprocess regression test in spawn.test.ts. My earlier inline comment (that the ownership-transfer sentence needed a POSIX qualifier to match the #[cfg(not(windows))] gate) was addressed in d4c1f5e and the thread is resolved.

Security risks

None apparent. This is fd-lifecycle bookkeeping for parent-side socketpair ends created by Bun.spawn; no auth, crypto, path handling, or untrusted-input parsing is touched. The change strictly reduces the finalizer's authority (it stops closing an fd number once JS has observed it), so the failure mode moves from "double-close a possibly-recycled fd" to "leak one fd if the caller forgets to close" — safer, not riskier.

Level of scrutiny

Medium-high. The code change itself is small and well-reasoned, and the test demonstrates the bug on an unfixed build. But it sits in native Subprocess fd-ownership code (the most-blocked review category per CLAUDE.md), and it introduces a user-visible API semantics change: reading the .stdio getter now has a side effect (ownership transfer). The author explicitly surfaced a design alternative — exposing a proper stream object at stdio[i>=3] instead of a raw number — and said "happy to go that route instead if preferred." That's a maintainer-level call I shouldn't make on their behalf.

Other factors

The bug hunter found nothing, CodeRabbit's mem::replace concern was withdrawn after confirming Fd is Copy, and the new test follows harness conventions (subprocess fixture, drains all pipes concurrently, asserts combined {stdout, stderr, exitCode}). The test asserts stderr: "" exactly, which CLAUDE.md flags as fragile under ASAN/debug, but that's a minor style point rather than a blocker. Net: the implementation looks correct for the chosen approach; a human should just confirm the approach.