Repository navigation
Conversation
…d 0 handle test closes stdin The "a received handle that lands on fd 0 is adopted" test closed fd 0 at script start and expected the next SCM_RIGHTS descriptor to be 0. JSC opens /proc/self/statm on first use (WTF::LinuxMemory, at the end of a GC cycle or after 100 heap threshold checks) and keeps it open. When that open runs between closeSync(0) and the receive, it takes fd 0 and the handle lands on the next free descriptor (9 on the x64-asan lane). The child now receives one warm-up handle and runs a full GC first, then closes fd 0 and asks for the handle under test.
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 2 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 11:02 AM PT - Sep 6th, 2026
⏳ @robobun, your commit 8743e46 is still building in
|
|
Status: ready for review. CI build 111588: this test file passes on every lane, including x64-asan (14 pass, 0 fail). The red lanes in that build ( Reproduction: the failure is in CI only (x64-asan lane, builds 110482 and 111370, Verified: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused test-only de-flake that swaps a timing assumption for an explicit handshake without weakening the assertion.
What was reviewed:
- The final assertion is unchanged (
receivedFd: 0,writable: true) — the property the test protects is still enforced. - The two-phase protocol: warmup socket lands on a non-zero fd (stdin still open),
Bun.gc(true)forces the lazy/proc/self/statmopen beforecloseSync(0), then the 'fd 0 is free' handshake gates the second connect — no sleep, awaits the actual condition. - Both client connections are cleaned up via the
on('data') → client.end()path; the whole describe is alreadyskipIf(isWindows), so the POSIX lowest-fd assumption holds.
Extended reasoning...
Overview
This PR touches a single test case in test/js/node/child_process/child_process_ipc_handle.test.ts — "a received handle that lands on fd 0 is adopted". The test verifies that when an SCM_RIGHTS-received socket is installed by the kernel at fd 0, Bun's IPC layer adopts it as a working net.Socket (guarding against fd 0 being treated as falsy or skipped by stdio-aware close helpers). The original fixture closed fd 0 at module load and assumed nothing would claim it before the receive; JSC's lazy open of /proc/self/statm at GC-end (via WTF::LinuxMemory::singleton()) occasionally stole it on the ASAN lane. The fix adds a warmup round-trip (receive one throwaway handle + Bun.gc(true)) so all known lazy fds are opened while stdin still occupies slot 0, then closes fd 0 and sends an explicit 'fd 0 is free' handshake to the parent before the real handle is sent.
Security risks
None. Test-only change to a subprocess fixture that runs against a local net.createServer on 127.0.0.1 port 0. No new network egress, no auth/crypto/permissions surface, no src/ changes.
Level of scrutiny
Low-to-moderate. The change is ~30 lines in one test file, but de-flakes are where REVIEW.md warns against silently weakening coverage. I verified: (1) the final expect on { message: "sock", receivedFd: 0, writable: true } is byte-identical to before — the test still fails if the handle doesn't land on fd 0 or isn't writable; (2) the fix awaits an explicit message handshake rather than a sleep, which is the sanctioned pattern; (3) the warmup socket cannot itself land on fd 0 because stdin is still open when it arrives, and even if its fd frees later, fd 0 is lower so the kernel picks it for the second receive; (4) the enclosing describe.skipIf(isWindows) already gates the whole suite to POSIX, so the lowest-free-fd assumption and the Linux-specific comment are appropriately scoped.
Other factors
The PR description is unusually thorough — it names the exact JSC/WTF call chain that opens the offending fd, explains why fd 9 was observed, and documents the LD_PRELOAD + gdb methodology used to find it. The 5-line comment above the test is load-bearing (a future reader would otherwise wonder why a two-phase protocol exists for a simple fd-0 check) and names concrete source locations rather than narrating the diff. Harness conventions are followed: tempDir, bunExe()/bunEnv, port: 0, test.concurrent, concurrent pipe draining, stdout asserted before exit code. Bug-hunt exit reason was dry_streak with no findings and no ruled-out candidates. Timeline shows only the initial commit with no outstanding reviewer objections.
Problem
test/js/node/child_process/child_process_ipc_handle.test.ts> "a received handle that lands on fd 0 is adopted" fails on the x64-asan lane withreceivedFd: 9instead of0(builds 110482, 111370). The test came in with cluster: port Node's cluster and child_process handle-passing suites (+43 upstream tests; cluster 54 → 85) and implement what they expose — round-robin fd handoff, SCHED_NONE shared handles, UDP clustering, IPC handle passing #31829.fs.closeSync(0)at script start and expects the nextSCM_RIGHTSdescriptor to be 0. JSC opens/proc/self/statmon first use and keeps it open (WTF::LinuxMemory::singleton()inSource/WTF/wtf/AvailableMemory.cpp:111, reached fromJSC::Heap::overCriticalMemoryThreshold,heap/Heap.cpp:713). That open runs at the end of a GC cycle, or on the 100th heap threshold check. When it runs aftercloseSync(0)and before the receive, it takes fd 0. The handle then lands on the next free descriptor, which is 9 in that process (1,2 stdio pipes, 3 ipc, 4 urandom, 5 epoll, 6 eventfd, 7 urandom, 8 statm).Fix
Bun.gc(true). Then it closes fd 0 and tells the parent to send the handle under test. The warm-up runs every code path the real receive runs, and the full GC reachesHeap::updateAllocationLimits, which opens thestatmdescriptor.net.Socket(receivedFd: 0,writable: true). Nosrc/change: the lazy opens are correct behavior, the test's assumption about descriptor numbering was the bug.bun bd test test/js/node/child_process/child_process_ipc_handle.test.ts(14 pass, 3 runs), and the same file under a localrelease-asanbuild with the CI lane's env (BUN_GARBAGE_COLLECTOR_LEVEL=1,BUN_FEATURE_FLAG_NO_ORPHANS=1), 5 runs of the fd 0 test.Background
process.send(msg, handle)passes the handle's descriptor over the IPC unix socket withSCM_RIGHTS. The kernel installs it in the receiver at the lowest free descriptor number, likeopen(2)does.src/runtime/ipc.rsmust treat descriptor 0 as a valid handle (0 is falsy in JS, and Bun's internal close helpers skip stdio numbers). That is what this test guards.process.on('message'), so module-level code in the child runs before any receive.Notes
/dev/urandomfromWTF::RandomDevice(RandomDevice.cpp:91) and frombmalloc::ARC4RandomNumberGenerator::stir(bmalloc/CryptoRandom.cpp:127). Both open during startup in practice, before user code runs. The warm-up round trip covers them too if that ever changes.LD_PRELOADhook that logs every descriptor-creating libc call in the child, plus a gdb breakpoint on thestatmopen: the backtrace isHeap::collectIfNecessaryOrDefer->overCriticalMemoryThreshold(Cached)->WTF::percentAvailableMemoryInUse->LinuxMemory::singleton.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_ipc_handle.test.ts