Conversation
…out on debug builds The test spawned process.execPath 20 times. A debug build takes 120 to 250 ms to start, so the test took about 5.1 s and hit the 5 s timeout. Spawn `true` instead, since the child only has to exit. Wait for the three sockets to emit 'close' instead of sleeping 10 ms. Run 5 iterations. With both fd ownership guards removed from src, the rewritten test still aborts in the first iteration.
|
Warning Review limit reached
Next review available in: 2 minutes Limit details: You’ve used all 5 included reviews currently available. You can run this review on demand instead of waiting. On-demand reviews are free until September 18, 2026. After that, they cost $0.25 per reviewed file.
How can I continue?Run this review now using the option above, or comment You can also wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day 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 (1)
Comment |
|
Status Reproduced with This PR changes only the test. It now takes about 2.05 s on the debug build (8 of 8 reruns) and 42 ms on the release binary. With both fd ownership guards removed from CI: the Buildkite build for this branch passed (build 101392). The diff is ready for a maintainer. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only speedup that shrinks the workload instead of raising the timeout, and swaps a 10 ms sleep for the actual observable condition.
What was reviewed:
trueis POSIX-standard and the test is already.skipIf(isWindows); the same child is used inspawn-stdin-pipe-fd-leak.test.ts.'close'listeners are attached beforedestroy()is called, so the event cannot be missed.- The lowered iteration count was verified against a scratch build with the fd-ownership guards removed — the test still aborts on iteration 0, so it keeps protecting the invariant it was written for.
Extended reasoning...
Overview
Test-only change to one test in test/js/node/child_process/child_process.test.ts ("extra stdio pipes are not double-closed on GC"). The subprocess script now spawns true instead of process.execPath -e "", awaits each extra socket's 'close' event instead of a 10 ms setTimeout, and runs 5 iterations instead of 20. No production code is touched.
Security risks
None. This edits an inline fixture script inside a test; it does not touch runtime, auth, crypto, or any user-facing surface.
Level of scrutiny
Low. This is a de-flake / speedup of an existing regression test. The repo's own test guidance says to shrink the workload rather than raise the timeout and to await the actual condition instead of sleeping — this change does exactly both. The two comments added explain why true is used and why waiting on 'close' is the right condition, which is the kind of non-obvious "why" the guidelines ask for.
Other factors
- The test is
.skipIf(isWindows), so relying on the POSIXtrueutility is safe; the same pattern already appears in other spawn tests. - The
closedPromise.allis created (listeners attached with.once) before the child's'exit'is awaited and befores.destroy()runs, so a socket that closes early is still observed and the await cannot hang on a missed event. - The critical "never silently weaken an existing test" concern is addressed head-on: the PR description documents rebuilding with both fd-ownership guards disabled and confirming the new 5-iteration script still aborts with exit 134 on iteration 0 in 10/10 runs. The assertion (
{ stdout: "OK", stderr: "", exitCode: 0 }) is unchanged. - No prior reviewer comments to address; no CODEOWNERS on test files.
|
The automated review above raised no change requests, and there are no open review threads. Nothing to change in the diff from it. The PR is waiting for CI. |
### Problem - `spawn()` and `spawnSync()` accept a `tls.TLSSocket` in `stdio` and give the child the raw TCP descriptor under the TLS session. The child reads TLS records (`17 03 03 ...`). A child that writes puts plaintext into the TLS stream. Node throws `ERR_INVALID_ARG_VALUE`. - The cause is `streamFdOf` (`src/js/node/child_process.ts:1752`). It returns `_handle.fd` for any stream. Node accepts a stream only when its handle is a pipe, TCP, TTY or UDP wrap ([child_process.js#L1063-L1083](https://github.com/nodejs/node/blob/v26.3.0/lib/internal/child_process.js#L1063-L1083)). ### Fix - `streamFdOf` throws `ERR_INVALID_ARG_VALUE` for a `tls.TLSSocket` before it reads `_handle.fd`. It reuses the check that `subprocess.send()` has for a TLS socket (`src/js/builtins/Ipc.ts:31`). - The message names only the class: `Received '[TLSSocket]'`. The error builder inspects its value in full, and an inspected `TLSSocket` reaches its key and passphrase. - Correct because a TLS socket is the only Bun stream whose `_handle.fd` does not carry the stream's own bytes. A `net.Socket` is still accepted. - Verified: `test/js/node/child_process/child_process.test.ts` (`a socket as a stdio entry`, the TLS test fails without the fix). Also `test/js/node/child_process/` and 18 vendored `test-child-process-*` scripts. ### Background - A stream in `stdio` shares its descriptor with the child: both processes hold the same kernel socket. - A `tls.TLSSocket` is a `net.Socket` whose bytes pass through a TLS session in the parent. Its kernel socket carries only TLS records. - In Bun, `socket._handle` is the native socket object. Native `TCPSocket` and `TLSSocket` expose `fd`. - Only `tls.TLSSocket` and its subclasses have the `Symbol.for("::buntls::")` method. net.ts and Ipc.ts detect a TLS socket with it. <details><summary>Notes</summary> **Repro.** A TLS server hands its accepted socket to a child as stdin. The peer sends 20,000 bytes of `0x41`. The child counts what it reads (python3, so the reader does not depend on Bun). ```js const tls = require("tls"), fs = require("fs"), os = require("os"), { spawn, execFileSync } = require("child_process"); const d = fs.mkdtempSync(os.tmpdir() + "/c-"); execFileSync("openssl", ["req", "-x509", "-newkey", "rsa:2048", "-nodes", "-keyout", d + "/k", "-out", d + "/c", "-days", "2", "-subj", "/CN=localhost"], { stdio: "ignore" }); const srv = tls.createServer({ key: fs.readFileSync(d + "/k"), cert: fs.readFileSync(d + "/c") }, (s) => { s.on("error", () => {}); let c; try { c = spawn("python3", ["-c", "import os,time\nn=0;first=b''\nwhile True:\n try: b=os.read(0,65536)\n except BlockingIOError: time.sleep(0.01); continue\n if not b: break\n first=first or b[:5]; n+=len(b)\nprint('child read',n,'bytes; first 5:',first.hex())"], { stdio: [s, "pipe", "inherit"] }); } catch (e) { console.log("spawn() throws", e.code); s.destroy(); return srv.close(); } let out = ""; c.stdout.on("data", (x) => (out += x)); c.on("close", () => { console.log(`${out.trim()}; parent socket.bytesRead=${s.bytesRead}`); s.destroy(); srv.close(); }); }); srv.listen(0, "127.0.0.1", () => { const cl = tls.connect({ port: srv.address().port, host: "127.0.0.1", rejectUnauthorized: false }, () => { let i = 0; const t = setInterval(() => { cl.write(Buffer.alloc(1000, 65)); if (++i === 20) { clearInterval(t); cl.end(); } }, 5); }); cl.on("error", () => {}); }); ``` | build | result (3 runs) | |---|---| | node v26.3.0 | `spawn() throws ERR_INVALID_ARG_VALUE` | | bun 1.4.3-canary.1 (367d939) | `child read 1022 bytes; first 5: 17030303f9; parent socket.bytesRead=5000`, then `child read 0 bytes`, then `child read 1046 bytes; first 5: 17030303f9` | | this branch (debug build) | `spawn() throws ERR_INVALID_ARG_VALUE` | **Compared with node v26.3.0, entry by entry** (Linux x64, `spawn("true", [], { stdio: [entry, "ignore", "ignore"] })`): | stdio entry | node | main | this branch | |---|---|---|---| | `TLSSocket` that a `tls.Server` accepted, at index 0, 1, 2 or 3, `spawn` and `spawnSync` | `ERR_INVALID_ARG_VALUE` | accepted | `ERR_INVALID_ARG_VALUE` | | `TLSSocket` from `tls.connect()` | `ERR_INVALID_ARG_VALUE` | accepted | `ERR_INVALID_ARG_VALUE` | | `TLSSocket` from `tls.connect({ socket })` | `ERR_INVALID_ARG_VALUE` | accepted | `ERR_INVALID_ARG_VALUE` | | the `net.Socket` under that wrap | accepted | accepted | accepted | | `session.socket` of a secure HTTP/2 session | `ERR_INVALID_ARG_VALUE` | accepted | `ERR_INVALID_ARG_VALUE` | | `new tls.TLSSocket()`, a destroyed `TLSSocket`, a subclass instance | `ERR_INVALID_ARG_VALUE` | plain `Error` ("without an underlying file descriptor") | `ERR_INVALID_ARG_VALUE` | | `TLSSocket` with an own numeric `fd` property | accepted as that fd | accepted | accepted | | `net.Socket` | accepted | accepted | accepted | Node takes a numeric `fd` property before it looks at handles, so the new check sits after the own `fd` check. **Why only TLS.** Node has an allowlist of handle wraps. Bun has no wrap classes: a stream reaches the child through `_handle.fd`, and the only handles with an `fd` are the native `TCPSocket` (TCP and unix sockets, the raw transport) and the native `TLSSocket`. fs, tty and `process.std*` streams carry their own `fd`. The HTTP/2 `session.socket` proxy forwards the `::buntls::` lookup to its `TLSSocket`, so the same check covers it. **Error message.** `$ERR_INVALID_ARG_VALUE(name, value)` inspects `value` into the message with no depth or length limit. The first commit passed the socket. With a PEM string `key` and a `passphrase`, the message was 81,943 characters for an accepted socket and 16,431 for a connected one, and it contained the key body and the passphrase. Node's message is 173 characters (node cuts the inspected value at 128) and contains neither. The second commit keeps the socket away from the builder. `inspect(item, { depth: -1 })` is the shallow form that `node:events` uses for the MaxListeners warning. It reads the constructor name and no property value: `'[TLSSocket]'`, `'[Sub]'` for a subclass, `'[ServerHttp2Session]'` for the HTTP/2 `session.socket` proxy. The quotes are there because the builder quotes a string value. The test pins the exact message. #38402 (open) adds node's 128 character cut to the builder for every caller. This PR does not depend on it. **Windows.** Checked on canary 1.4.3-canary.1+9cba9036a: any socket in `stdio` fails with `EBADF: bad file descriptor, uv_spawn` (node: `spawn ENOTSUP`). The new check runs before that, so the TLS test is not platform specific. The `net.Socket` control is POSIX only. **Related open PRs on the same mapping:** #35226 (error codes for entries with no fd), #39220 (entries that carry an `fd`), #43707 (stop the parent's reads after a hand-off). None of them rejects a TLS socket. A trial merge with #43707 conflicts in one line, the `node:net` import of `child_process.test.ts` (keep #43707's line, it is a superset of this one). #35226 and #39220 conflict in `streamFdOf`, so whichever merges later needs a small rebase. **Local runs of `test/js/node/child_process/` (debug build).** Three tests fail, none through this diff (a fourth, "should allow us to set env", takes 4.6 s to 5.1 s on my debug build and crosses the 5 s timeout in some runs): - `child_process.test.ts`, "should allow us to spawn in the default shell": also fails on the release canary in my container. - `child_process.test.ts`, "extra stdio pipes are not double-closed on GC": 5 s timeout. Its script prints `OK` after 6.2 s on a debug build (#39687). - `child-process-exec.test.ts`, "stderr > exceeding maxBuffer should throw": fails the same way on a debug build of main without this change, passes on the release canary. `child_process_ipc_handle.test.ts` hit the 5 s timeout in 2 tests on one run and passed 14 of 14 on the next. Of the 18 vendored scripts, `test-child-process-emfile.js` fails only on a debug build: it loads built-in modules from disk and cannot under `EMFILE`. It passes on the release canary. </details> <!-- 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/node/child_process/child_process.test.ts <!-- robobun:evidence:end -->
This branch has not been deployed
1 participant
Problem
child_process.test.tsfails:(fail) extra stdio pipes are not double-closed on GC [5005.63ms],this test timed out after 5000ms.process.execPath20 times. A debug build takes 120 to 250 ms to start. Plus 40 full GCs, that is about 4.7 s.Fix
trueinstead ofprocess.execPath. The child only has to exit: the bug is about who closes the parent end of each pipe.'close'events instead of a 10 ms timer. usockets closes the fd beforenet.Socketemits'close', so a later close by the finalizer is a double close.src(Notes), the new script aborts at the firstBun.gc(true)of iteration 0 in 10 of 10 runs.test/js/node/child_process/child_process.test.ts. The GC test takes about 2.05 s on debug (8 of 8 reruns) and 42 ms on release. With the guards removed it fails:exitCode: 134,panic: assertion failed: err.is_none().Background
node:child_processwraps each extra"pipe"in anet.Socket. usockets closes the fd when that socket closes.Subprocess::finalize_streams(src/runtime/api/bun/subprocess.rs) runs when the GC collects the wrapper. It closes each slot still markedExtraPipe::OwnedFd. Two guards mark the slotUnownedFdfirst:"socket-fd"(build: enable Rust debug-assertions for asan/assertions builds; decouple IS_DEBUG #32520) and the.stdiogetter (Bun.spawn: don't double-close extra stdio fds exposed via .stdio #33828).bun bd, the release-asan CI lane),Fd::close(src/sys/fd.rs:73) asserts thatclose(2)succeeded. A double close is EBADF, so the script aborts (exit code 134).Notes
The release binary (1.4.0) passes the current test in about 330 ms. The behavior under test is fine. Only the time budget is too small for a debug build.
Timing on the debug build (16 vCPU, idle). The script alone, without the test runner:
truechild, 5 iterationstruechild, 10 iterationstruechild, 20 iterationsPer iteration with the bun child: 125 to 250 ms for spawn and exit, 40 to 100 ms for the two GCs. The test runner's own start of the script adds the rest of the 5 s.
Every variant pays a fixed cost of about 1.3 s: process start of the debug binary is about 250 ms,
require("node:child_process")about 290 ms, and the firstrequire("node:net")about 355 ms. That is most of the 2.05 s the test takes now.bun -vcosts the same asbun -e ""on the debug build (about 120 ms each). The cost is the start of the binary, not the JS it runs, so a cheaper bun flag does not help. The old child did not run any JS either: on main,bun -e ""prints the help text and exits. So the switch totrueremoves no coverage of bun as the child.cmd: ["true"]is what the POSIX spawn tests use for a child that exits at once (for exampletest/js/bun/spawn/spawn-stdin-pipe-fd-leak.test.ts). This test skips Windows.To confirm the new test still detects the bug, I built a scratch binary with both guards disabled behind an environment variable: the
"pipe"to"socket-fd"loop insrc/js/node/child_process.ts, and theOwnedFdtoUnownedFddowngrade inSubprocess::get_stdio. With that binary:Bun.gc(true)in 10 of 10 runs,exitCode: 134andpanic: assertion failed: err.is_none()inSubprocess::finalize_streams.The scratch change is not part of this PR. The extra iterations only cover a GC pass that does not collect the wrapper on one try.
Why the
'close'wait is correct:Socket.prototype._destroycallscloseSocketHandle, which callshandle.close()and then emits'close'from asetImmediate.handle.close()runsus_socket_close, which callsbsd_close_socketbefore it dispatches the close callback (packages/bun-usockets/src/socket.c). The listeners are attached right afterspawn(), so a socket that closes beforedestroy()is not missed either. In 60 traced closes, none did.spawn() > should allow us to spawn in the default shellalso fails in my container, with the release binary too.$SHELLis not exported there. It is not related to this change.#38587 touches the same test. It gives the test a 30 s timeout because its change makes
bun -e ""boot the runtime. With this PR the test does not spawn bun, so that hunk is not needed after a rebase.A self-review of the diff (edge cases, coverage, alternatives such as keeping
process.execPathwith fewer iterations, or adding an fd-recycling check) raised no concern that needs a change.Suites run:
test/js/node/child_process/child_process.test.tson the debug build (65 pass, 2 skip, plus the$SHELLfailure above) and withUSE_SYSTEM_BUN=1(same result).no test proof · iteration 0 · Platform-specific test-only change; deferring to CI.