Conversation
|
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 (4)
Comment |
|
Updated 12:41 AM PT - Sep 6th, 2026
✅ @robobun, your commit 78d145b3df20c1686cdecfad32c933212125e35f passed in 🧪 To try this PR locally: bunx bun-pr 41484That installs a local version of the PR into your bun-41484 --bun |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟡
src/jsc/bindings/BunProcess.cpp— The narrowed_ => Unknownclassification has an unaccounted-for consumer: on Windows,forceSync = fdType == file || fdType == pipenow evaluates false for stdout/stderr whose fstat fails (mode 0 — a missing/invalid handle) or returns a directory/block-device mode, where before it wasfileand true;Bun__ForceFileSinkToBeSynchronousForProcessObjectStdiois no longer called for those handles. Fix: includeunknownin the WindowsforceSyncpredicate (or keep it as the else-default), so the write-stream sync behavior for unclassified fd 1/2 is unchanged. The PR description audited only the JS-sidegetStdioWriteStreambranch onfdType.Extended reasoning...
Bun__Process__getStdinFdType(rare_data.rs:1083-1088) previously mapped everyFileKindother thanNamedPipe/UnixDomainSockettoFile; nowBlockDevice,Directory,SymLinkandUnknown(mode 0 when fstat fails, rare_data.rs:1031-1033) returnStdinFdType::Unknown.constructStdioWriteStream(BunProcess.cpp:2988) reads this for fd 1/2 and, inside#if OS(WINDOWS)at line 3012, setsforceSync = fdType == file || fdType == pipe. On the base branch a Windows process with no stdout handle (fstat fails → mode 0 → catch-allFile) gotforceSync = true; after this change it getsUnknown→forceSync = false, soBun__ForceFileSinkToBeSynchronousForProcessObjectStdioat line 3024 is skipped. The PR body claims stdout/stderr behave as before because the JSgetStdioWriteStreamonly testspipe || socket, but it missed this C++ switch on the same discriminant — REVIEW.md's "new enum variant → audit every switch on the discriminant" applies. Practical impact is limited (writes to an invalid handle fail either way), but the predicate should still coverunknownto keep…Verification: nit — The mechanism is exactly as described, but the observable impact is negligible. Base branch (src/jsc/rare_data.rs, per diff):
rust match bun_sys::kind_from_mode(mode) { bun_sys::FileKind::NamedPipe => StdinFdType::Pipe, bun_sys::FileKind::UnixDomainSocket => StdinFdType::Socket, _ => StdinFdType::File, }kind_from_mode(0)returnsFileKind::Unknown… | nit — the…
There was a problem hiding this comment.
LGTM — thanks for switching to test.concurrent.skipIf.
What was reviewed: the three BunProcessStdinFdType enums stay in lockstep (TS/C++/Rust all gain = 3); the Windows forceSync flip from file || pipe to != socket is behavior-identical for the three pre-existing variants and only sweeps the new unknown in. Checked kind_from_mode — the wildcard now catches Directory/BlockDevice/Unknown (mode 0), matching libuv's UV_UNKNOWN_HANDLE set. makeEndedReadable() is the existing worker-stdin helper, so no new stream machinery.
Extended reasoning...
Overview
The PR fixes process.stdin to end cleanly (matching Node) instead of throwing EISDIR synchronously when fd 0 is a directory or other non-readable handle type. It adds an Unknown = 3 variant to the mirrored fd-type enum in three places (ProcessObjectInternals.ts, BunProcess.cpp, rare_data.rs), narrows the Rust classifier so only regular files and character devices map to File, short-circuits getStdinStream to reuse the existing makeEndedReadable() helper for the unknown case, and adjusts the Windows forceSync predicate to a denylist so unknown also forces sync writes. A concurrent, Windows-skipped test spawns bun with a directory fd as stdin and asserts end,close with exit 0.
Security risks
None. This is a Node-compat behavior fix on the stdin classification path — no auth, crypto, network, or untrusted-input parsing is involved. The fd type comes from a cached fstat on the process's own fd 0; the change only widens which mode bits fall into a "return an already-ended Readable" branch instead of attempting a native read.
Level of scrutiny
Low-to-moderate. The diff is ~40 lines across four files, follows an existing pattern (the makeEndedReadable helper was already used for worker stdin), and the enum is updated in lockstep everywhere it lives. I verified against bun_core::FileKind that the new wildcard arm catches exactly Directory, BlockDevice, SymLink, Whiteout/Door/EventPort, and Unknown (mode 0) — the set libuv reports as UV_UNKNOWN_HANDLE. The Windows forceSync rewrite is provably identical for the three pre-existing enum values (0/1 → true, 2 → false in both spellings).
Other factors
Earlier feedback on this PR (use test.concurrent.skipIf) was addressed in commit 3f1904d; the only commit since is an empty CI retrigger. The test follows harness conventions (tempDir, bunEnv, Promise.all pipe drain, stderr/stdout asserted before exitCode, closeSync in finally). The bug hunt exited on dry_streak with no findings.
Problem
bun app.js < somedir), the firstprocess.stdin.resume(),.on("readable"),.read()or.ref()throws synchronously:EISDIR: illegal operation on a directory, fstatfromowningetStdinStream. Anerrorlistener cannot catch it. The process exits with code 1. Node printsend,closeand exits 0.own()(src/js/builtins/ProcessObjectInternals.ts) callsBun.stdin.stream().getReader(). The native file reader rejects a directory at start (src/runtime/webcore/FileReader.rs:212) andgetReader()rethrows that start error synchronously. That is the tested contract forBun.file(x).stream(), so the fix belongs inprocess.stdin.Fix
Bun__Process__getStdinFdType(src/jsc/rare_data.rs) gains a fourth value,Unknown. It covers what libuv'suv_guess_handlereports asUV_UNKNOWN_HANDLE: every fd type other than a regular file, a character device, a pipe or a socket (a directory, a block device, or an fd thatfstatrejects).getStdinStreamreturns the existingmakeEndedReadable()for a non-TTYunknownfd, withfd = 0set. This is Node'sgetStdin()default branch. The native stdin stream is never created for such an fd.getStdioWriteStreamalso receives the fd type for fd 1 and 2. It only branches on pipe and socket, so anunknownstdout or stderr behaves as before.test/js/node/process/process-stdin.test.ts(new test, fails on the released build with the EISDIR above). Alsoprocess-stdio.test.ts,test/js/node/tty,worker_threads.test.ts.directory, the helper is reused, the mode-0 case ends instead of crashing). Not addressed: a no-opWritablefor anunknownstdout, which is a separate change.Background
process.stdinis built lazily bygetStdinStream. It wrapsBun.stdin.stream(), a nativeReadableStreamover fd 0, in afs.ReadStreamortty.ReadStream.own()acquires the native reader the first time the stream flows.file,pipe,socket) from a cachedfstaton fd 0. Node usesguessHandleType()(libuvuv_guess_handle) for the same decision.makeEndedReadable()(src/js/internal/worker/stdio.ts) is the already-endedReadablethat a worker'sprocess.stdinuses when the worker has no stdin. It pushesnullon the first read.Notes
Repro on the released build:
process.stdin.isTTYalone does not throw on the current canary. The throw happens on the first acquisition of the native reader.Unchanged:
Bun.stdin.text()still rejects with EISDIR, andfor await (const c of process.stdin)with a directory now ends at once instead of throwing.Classification after this change:
File | CharacterDevicestayfile(regular files,/dev/null, TTYs),NamedPipeispipe,UnixDomainSocketissocket, everything else isunknown. On Windows fd 0 goes throughuv_fs_fstat, which reports consoles as character devices, pipes and sockets as FIFOs and disk files as regular files. A missing stdin handle failsfstat, so its mode is 0 and it isunknown, which matches Node's "dummy contentless input for non-console Windows applications".The test is skipped on Windows because the test cannot hand a directory handle to a child as stdin there.
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/node/process/process-stdin.test.ts