Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/bun.js/api/bun/process.zig
Original file line number Diff line number Diff line change
Expand Up @@ -1508,9 +1508,13 @@
try extra_fds.append(fds[0]);
},
.pipe => |fd| {
try actions.dup2(fd, fileno);

try extra_fds.append(fd);
// This file descriptor was passed in by the user (e.g. a
// number in the stdio array). It is not owned by Bun.spawn,
// so we must not close it in finalizeStreams. Store
// invalid_fd to keep the positional slot for
// subprocess.stdio without taking ownership.
try extra_fds.append(bun.invalid_fd);

Check warning on line 1517 in src/bun.js/api/bun/process.zig

View check run for this annotation

Claude / Claude Code Review

proc.stdio[n] now returns null instead of the fd number on POSIX

This change makes `proc.stdio[n]` return `null` instead of the fd number for user-provided extra fds on POSIX (since `getStdio` at subprocess.zig:488-492 now sees `invalid_fd`). This is arguably the correct behavior — it matches Node.js and the existing Windows path — but it's an observable change worth calling out, and the TypeScript type `readonly stdio: [null, null, null, ...number[]]` at packages/bun-types/bun.d.ts:7165 should be updated to allow `null` in the rest tuple.
Comment on lines 1511 to +1517

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 This change makes proc.stdio[n] return null instead of the fd number for user-provided extra fds on POSIX (since getStdio at subprocess.zig:488-492 now sees invalid_fd). This is arguably the correct behavior — it matches Node.js and the existing Windows path — but it's an observable change worth calling out, and the TypeScript type readonly stdio: [null, null, null, ...number[]] at packages/bun-types/bun.d.ts:7165 should be updated to allow null in the rest tuple.

Extended reasoning...

What changed

Subprocess.getStdio (src/bun.js/api/bun/subprocess.zig:480-493) iterates stdio_pipes and, on POSIX, pushes JSValue.jsNumber(item.cast()) when item.isValid() and null otherwise. Before this PR, when a raw fd number was passed at stdio[3+], spawnProcessPosix stored that fd directly in extra_fdsstdio_pipes, so proc.stdio[3] returned the fd number. After this PR, bun.invalid_fd is stored instead, so item.isValid() is false and proc.stdio[3] returns null.

Step-by-step proof

  1. User calls Bun.spawn({ stdio: ['ignore', 'ignore', 'ignore', 7] }).
  2. The fd 7 is mapped to PosixSpawnOptions.Stdio{ .pipe = 7 } in options.extra_fds[0].
  3. spawnProcessPosix (process.zig:1509-1517) hits the .pipe arm and now appends bun.invalid_fd to extra_fds (previously appended 7).
  4. extra_fds becomes spawned.extra_pipes, which becomes Subprocess.stdio_pipes.
  5. JS reads proc.stdiogetStdio runs → for index 3, item == bun.invalid_fd, item.isValid() is false → pushes .null.
  6. Before: proc.stdio[3] === 7. After: proc.stdio[3] === null.

Why existing code doesn't prevent it

Nothing in getStdio special-cases "user-provided fd that we don't own"; it only checks isValid(). The PR intentionally uses invalid_fd as the sentinel for "slot exists but Bun doesn't own it", and getStdio maps that to null.

Addressing the refutation ("this is intentional, not a bug")

The refuter is right that this is the intended direction of the fix and aligns with both Node.js (which returns null for non-'pipe' stdio slots) and Bun's own Windows path (which already reported .unavailablenull). The previous return value was also dangerous: it echoed back an fd that Bun was about to close on GC, so any code relying on it was already racy/broken. And no information is lost — the user already has the fd they passed in. I agree this should not block the PR.

However, two things make it worth a comment rather than silence:

  • The TypeScript type is now wrong. packages/bun-types/bun.d.ts:7165 declares readonly stdio: [null, null, null, ...number[]], and the type-test fixture at test/integration/bun-types/fixture/spawn.ts:74 expects number | undefined for indices ≥ 3. With this change, those slots can be null on every platform, so the type should become something like [null, null, null, ...(number | null)[]].
  • It's a real observable change on POSIX. The PR description says the positional entry is "preserved" but doesn't mention the value changes from the fd number to null. Worth a one-line note for anyone scanning the changelog.

Impact

Low. Existing POSIX code that did const fd = proc.stdio[3] to read back the fd it just passed in will now get null. Since the user already has that fd, the workaround is trivial (use the value you passed in). The type mismatch is the more concrete actionable item.

Suggested fix

Update packages/bun-types/bun.d.ts:7165 to readonly stdio: [null, null, null, ...(number | null)[]] (and the corresponding type-test fixture), and add a sentence to the PR description noting that proc.stdio[n] now returns null for raw-fd slots on POSIX, matching Windows and Node.js.

},
}
}
Expand Down
68 changes: 68 additions & 0 deletions test/js/bun/spawn/spawn-extra-fd-ownership.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import { test, expect } from "bun:test";
import { bunEnv, bunExe, isPosix, tempDir } from "harness";

// When a raw file descriptor number is passed in the stdio array at an
// index > 2, Bun.spawn does not own that fd and must not close it when the
// Subprocess is finalized.
test.skipIf(!isPosix)(
"Bun.spawn does not close user-provided extra stdio fds",
async () => {
using dir = tempDir("spawn-extra-fd", {
"run.js": `
const { openSync, fstatSync, readFileSync } = require("node:fs");
const path = require("node:path");

const file = path.join(process.argv[2], "out.txt");
const fd = openSync(file, "w");

async function once() {
const proc = Bun.spawn({
cmd: ["/bin/sh", "-c", "echo hi >&3"],
stdio: ["ignore", "ignore", "ignore", fd],
});
await proc.exited;
}

for (let i = 0; i < 4; i++) {
await once();
Bun.gc(true);
await Bun.sleep(1);
Bun.gc(true);
}

// If Bun had closed fd during finalization of one of the subprocesses
// above, the next fstat would fail with EBADF (or worse, the fd slot
// could have been reused by an unrelated file).
fstatSync(fd);

// Also make sure the child was actually able to write through fd 3 so
// we know the fd was wired up.
const contents = readFileSync(file, "utf8");
if (!contents.includes("hi")) {
throw new Error("child did not write to fd 3: " + JSON.stringify(contents));
}

console.log("ok");
`,
});

await using proc = Bun.spawn({
cmd: [bunExe(), "run.js", String(dir)],
env: bunEnv,
cwd: String(dir),
stdout: "pipe",
stderr: "pipe",
});

const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);

const stderrLines = stderr
.split("\n")
.filter(l => l && !l.startsWith("WARNING: ASAN interferes"))
.join("\n");
expect(stderrLines).toBe("");
expect(stdout.trim()).toBe("ok");
expect(exitCode).toBe(0);
},
20000,

Check warning on line 67 in test/js/bun/spawn/spawn-extra-fd-ownership.test.ts

View check run for this annotation

Claude / Claude Code Review

Explicit test timeout violates test/CLAUDE.md

Nit: per `test/CLAUDE.md` ("**CRITICAL**: Do not set a timeout on tests. Bun already has timeouts."), the explicit `20000` timeout argument should be removed — the harness already manages per-test timeouts.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Nit: per test/CLAUDE.md ("CRITICAL: Do not set a timeout on tests. Bun already has timeouts."), the explicit 20000 timeout argument should be removed — the harness already manages per-test timeouts.

Extended reasoning...

What

The new test at test/js/bun/spawn/spawn-extra-fd-ownership.test.ts:67 passes 20000 as the third argument to test(), setting an explicit 20-second timeout.

Why this violates repo conventions

test/CLAUDE.md:120 states:

CRITICAL: Do not set a timeout on tests. Bun already has timeouts.

The test harness in this repo manages timeouts centrally, and explicit per-test timeouts have historically caused friction (drift between local and CI, masking hangs vs. genuine slowness, etc.). None of the other tests in test/js/bun/spawn/ set explicit timeouts, so this also breaks local consistency.

Step-by-step

  1. test(name, fn, 20000) is called at line 67.
  2. bun:test records 20000 ms as the per-test timeout, overriding the harness default.
  3. The repo guidance explicitly forbids step 2.

Impact

This is purely a convention/style issue — the test itself is functionally correct and the 20s value is generous enough that it won't cause flakes. Filing as a nit.

Fix

Drop the third argument:

  },
);

instead of

  },
  20000,
);

);