Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
|
| Layer / File(s) | Summary |
|---|---|
Native source flowing control src/jsc/bindings/webcore/streams/ReadableStreamOperations.cpp, src/runtime/webcore/FileReader.rs |
Readable stream reader initialization calls a C ABI function to set a native source to flowing. FileReader::set_flowing updates the state and returns early if the reader has not started. |
Process stdin reader ownership src/js/builtins/ProcessObjectInternals.ts |
disown leaves the flow and reader unchanged when another reader holds the native lock. After releasing its own reader, it sets the native source reference according to forceUnref. |
Stdin lifecycle regression coverage test/js/node/process/process-stdin.test.ts |
Tests cover stdin handoff after pauses and readline closure, pending web-stream reads, resumed data listeners, and console iteration after terminal input. |
Suggested reviewers: jarred-sumner
Priority:
Merge Risk: 
Minimal · up to b3356
-
No identified issue remains that needs to be resolved before merging.
Pre-merge checks: 
4
-
Full details: Description check
-
Explanation
The description follows the repository template. It has both required sections, "What does this PR do?" and "How did you verify your code works?". The "What does this PR do?" part is detailed. It states the problem, the one-line fix in disown(), the background on updateRef and forceUnref, the downsides, and the remaining unfixed cases. The verification part is also detailed. It names 32 new tests in process-stdin.test.ts, with 27 failing without the change. It lists measured results on Linux x64 and a long list of suites that pass on a debug build. It states that nothing ran on macOS or Windows. The description also states it is stacked on
#46528and that the PR is reduced to one line in disown(). This conflicts with the raw summary, which lists changes in ReadableStreamOperations.cpp and FileReader.rs. The description says those parts were removed, or moved to#46528, in commit b3356db. The PR objectives describe a different scope: a resume on reader lock, a started check, and a 9-test suite. So the description does not match the objectives or the file summaries. The check is about the template structure, so the mismatch is recorded but does not change the status. The description is long, and the Notes section is dense. That affects readability, not completeness.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
|
Status: closed. The line of this PR ( How it was reproduced: Linux x64, debug builds of the branch of #46528 (04af03e) with and without the line. A parent waits for a marker that the child prints from an unref'd timer, writes to the child's stdin, and closes the pipe.
|
Jarred-Sumner
left a comment
There was a problem hiding this comment.
terrible PR. does this really need a getter? does this really need another function?
|
No, it needed neither. Reworked in 1d2ebbb. The diff is now 3 source files (+34, -26) and has no Rust change.
The lock-time call is the part that cannot go away. After The same rule also covers the second reader after a release (#41578, its 5 tests are here and pass with no reader change), a Both test files pass on Linux and Windows x64 with the debug build. 10 of the 10 new |
|
Updated 11:05 AM PT - Oct 24th, 2026
❌ @robobun, your commit c937323 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 43747That installs a local version of the PR into your bun-43747 --bun |
c8e37d2 to
d2925c8
Compare
|
Update: the "release stops the source" shape from my last comment was wrong, and CI showed it. A pull that arrives after the release is parked in the stopped source and roots it, so d2925c8 is the smallest fix I can find for the reported bug. 3 source files, +19, -1, no getter, no new function, no Rust change, and the release path is untouched:
All 6 rows of the report read everything, also on a pty and with a pending top-level await. 8 new tests in This shape does not cover #41578 (second reader after another reader's release), so I reopened that PR and took its tests out of this one. |
d2925c8 to
a5d5d29
Compare
|
a5d5d29: the lock-time resume no longer goes through |
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.
Beyond the inline finding, I also checked the new lock-time resume in readableStreamReaderGenericInitialize: it runs after reader->m_stream / stream->m_reader are set, so a chunk or EOF that FileReader::set_flowing(true) dispatches synchronously via IOReader::read lands on a fully wired reader, and the uncheckedDowncast to JSNativeStreamSourceAdapter is guarded by the same SourceKind::Native check the existing release path uses.
Extended reasoning...
The PR was reworked since the prior review: the resume is now a direct C++ downcast to JSFileInternalReadableStreamSource at reader-lock time (ReadableStreamOperations.cpp, FileReader.rs, WebStreamsInternals.h) plus a one-line ref change in ProcessObjectInternals.ts and new process-stdin tests; no security-sensitive surface. A confirmed inline finding is being posted, one further verified finding was not posted, and Jarred-Sumner's CHANGES_REQUESTED review predates the new commit with no visible resolution, so approval is not appropriate.
736a54b to
b3356db
Compare
|
Rebased on main (the branch had conflicts in
A release build of main fails the 11 new tests in |
b3356db to
ce0a4b9
Compare
…reader disown() ended in source.updateRef(false). The next reader of Bun.stdin.stream() resumes the source, but the poll that it arms did not keep the process alive. A console iterator, a Bun.stdin.stream() reader or a Readable.fromWeb() that started after process.stdin.pause(), rl.close() or process.stdin.destroy() read nothing, and the process exited with code 0, unless a top-level await kept it alive. disown() now ends in source.updateRef(!forceUnref). A paused source has no armed poll, so the ref keeps nothing alive until a reader resumes the source. The release of that reader drops the ref.
busybox cat copies with sendfile(). A sendfile() that waits for input on a socket keeps the output pipe locked, so the child blocks in read() on bytes that are already in the pipe. The row timed out on Alpine.
ce0a4b9 to
0156904
Compare
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.
Beyond the inline note on run(), I traced the other two disown() callers through the native side: on the EOF path (internalRead -> disown with stream_reachedEof) and on 'close', the new updateRef(true) reaches PosixBufferedReader::update_ref, which only records KEEP_ALIVE and returns before touching a poll that is not watching, and FileReader::set_ref_unref is a no-op once the source is done, so neither path can hold the process open by itself. The unref() path still passes false through !forceUnref, and a later reader's releaseLock() drops the ref again via releaseNativeSourceRef, so the "exit after one chunk" rows exercise the real release sequence.
Extended reasoning...
The diff is one line in src/js/builtins/ProcessObjectInternals.ts (disown() now ends with source?.updateRef?.(!forceUnref)) plus a new describe block of 33 subprocess tests in test/js/node/process/process-stdin.test.ts; no security-sensitive surface. Not approving because a maintainer's CHANGES_REQUESTED review from the first version is still the latest human review state, the PR has since been reshaped (C++/Rust parts moved to a stacked PR), and whether the ref belongs in disown() versus reader attachment is a layering decision the maintainers should settle.
run() read stderr only after stdout ended, and not at all when stdout ended before a marker. A child that failed early gave no reason, and a child that filled the stderr pipe could not finish.
A reader release only dropped the event-loop ref of the native file source. The source of Bun.stdin.stream() kept its poll on fd 0 and took input that arrived while no reader was attached: one parked pull, then one more chunk. A dialog, fs.readSync(0) or a child process with inherited stdin did not get that input. After a break out of `for await (const line of console)` on a terminal, a child that reads stdin got 1 to 5 of 8 typed lines. The release hook now calls Bun__FileReader__onReaderRelease. A source that reads stdin, stdout or stderr as a pipe, socket or terminal stops, and the next reader lock resumes it through resumeNativeSource. A stopped source keeps nothing alive, so it keeps its ref for that next reader: a second console loop in an async function now waits for its input. Every other source reads on and drops its ref as before: a regular file, a path, another descriptor, a Bun.spawn pipe, and on Windows a console. A consumer that stops the source inside the delivery of a chunk ends the read loop. The loop read once more after a chunk of 131,072 bytes or more. set_flowing(true) reads only when the buffer of the source is below the highwater mark. Without that limit each release and lock adds one chunk to the buffer. The console iterator keeps the unfinished line at the end of the chunk that it stopped in. The next loop lost that start of a line. The rule "a reader release stops the native source, a reader lock resumes it" is from d227cc7 in #23341 and from 1d2ebbb in #43747. Both had no scope. The first stopped the file readers of Windows and the stdout of a child. The second leaked the descriptor of an abandoned Bun.file().stream() reader.
Stacked on #46528. It needs the resume that #46528 adds when a reader attaches.
Problem
process.stdin.pause()orrl.close(),for await (const line of console)reads nothing and the process exits with code 0. ABun.stdin.stream()reader andReadable.fromWeb(Bun.stdin.stream())do the same.disown()(src/js/builtins/ProcessObjectInternals.ts:154),source?.updateRef?.(false). The next reader resumes the source, but its poll keeps nothing alive.Fix
disown()ends insource?.updateRef?.(!forceUnref). That is the whole source change.test/js/node/process/process-stdin.test.ts. 27 fail without the line. The other 5 check that the process still exits.Background
process.stdinreads through a reader onBun.stdin.stream().disown()runs when it lets go: one tick after'pause', and on'close'.setFlowing(false)unregisters the poll of the native source on fd 0.updateRef()sets whether that poll keeps the process alive when armed.forceUnrefis true afterprocess.stdin.unref().Downsides
pause()runs reads nothing.Notes
What is left of this PR. Until b3356db this PR had three parts: a resume when a reader locks the stream (C++), a
startedcheck inFileReader::set_flowing(Rust), and two lines indisown(). #46528 has the first two since 44e4858, asresumeNativeSource()insetUpReadableStreamDefaultReaderandif !self.started.get() || ...inset_flowing. They are gone from here, and so is the segfault ofpause()followed byReadable.fromWeb(Bun.stdin.stream()), which #46528 fixes. One line ofdisown()is left.Measured (Linux x64, debug builds of 04af03e with and without the line). The child runs the first column and then reads. The parent writes the input 500 ms after the child prints a marker, and then closes the pipe.
pause(), onesetImmediate, the console iteratorgetReader()loop,for await,pipeTo()or.text()Readable.fromWeb(Bun.stdin.stream())rl.close(),resume()thenpause(), or a'data'listener that pauses, then a readerpause(), a reader that waits 1 s: CPU timeThe last row: with nothing ref'd, a pending top-level await spins in
EventLoop::wait_for_promiseuntil the input arrives.await new Promise(() => {})alone does the same on 1.3.13 (#14951), so that part is older and not changed here.History: the
pause()rows read everything on 1.3.13 and nothing on 1.4.2 (SIGSEGV forReadable.fromWeb). Since #34971 (1.4.0) a reader remembers an unref that comes before its first poll registration, anddisown()unrefs a source that never started. Therl.close()rows read nothing on 1.3.13 too.The tests. One
describeblock, four ways forprocess.stdinto let go (pause(),rl.close(),destroy(), abreakout offor await (const chunk of process.stdin)) and three readers (the console iterator, aBun.stdin.stream()reader,Readable.fromWeb). No child has a top-level await.bun | bun),process.stdintakes stdin back after the reader, a readline prompt on a pty followed by the console iterator, andprocess.stdin.unref()before the pause. The writer of the pipe is notcat: busyboxcatcopies withsendfile(), and asendfile()that waits for input keeps the pipe locked, so the reader blocks inread().The child prints its marker from an unref'd 50 ms timer. That timer fires only while stdin keeps the process alive, so without the line the marker never comes and the parent never writes. On debug builds of 04af03e, 26 rows failed in 5 of 5 runs without the line and all rows passed in 5 of 5 runs with it. The shell pipe row failed in 3 of 3 runs without the line and passed in 3 of 3 with it (release builds). The 5 rows that pass both ways are the 4 exit rows and the
unref()row. WithupdateRef(true)in place of the line, theunref()row fails.Self-review. Concerns and what happened to each:
getReader()in abeforeExitlistener runs a queuedprocess.nextTickcallback before it returns). This PR no longer has a resume. The same holds forresumeNativeSourcein http: consolidate the open fetch, Bun.serve, node:http, node:http2 and HTTP/3 pull requests #46528, reported there.pause()a no-op under every lock holder. Removed, see below.forceUnrefhalf had no test. Now theunref()row.native.lockedhalf had no test. Removed with the lock check.'close'path had no test. Now two of the four doors.The lock check left this PR.
if (!reader && native.locked) return;madeprocess.stdinleave a source alone while another reader holds the lock. It is a new ownership rule with a cost: afterpause()under a lock holder, a child withstdin: "inherit"got nothing in 5 of 10 runs (0 of 10 without the check), because the holder still reads fd 0. That is the contract of #13978 and #23333. It needs its own decision. #43951 points at the same case.Not changed.
pause()ordestroy()runs reads nothing more (Readable.fromWebin the same tick aspause()too). Under a top-level await it hangs.process.stdin.unref()still applies to the shared source, so the next reader does not keep the process alive.process.stdin.resume()throwsReadableStream is locked(process.stdin.resume() throws "ReadableStream is locked" after Bun.spawn takes Bun.stdin.stream() (regression in 1.4.3) #45068, Callingnextonconsole's asyncIterable causes its stream to become locked #7541).process.stdin) still exits early (streams: restore the native source ref when a new reader locks a released stream #41578).pause()still frees fd 0 for a child:test/js/node/readline/stdin-pause-pty.test.tspasses on release builds with and without the line.Suites. Debug build with the line, 0 failures:
process-stdin.test.ts(63),console-iterator.test.ts(17),readline.node.test.ts(81),readline_promises.node.test.ts,pause_stdin_should_exit,readline_never_unrefs,stdin_fell_asleep,process-stdio.test.ts,process-stdin-stale-hup,terminal.test.ts(101),terminal-spawn.test.ts,test/js/node/tty,spawn-stdin-readable-stream.test.ts(54),spawn-socketpair-shutdown,spawn-streaming-stdin,node-stream.test.js(162), and the 33 Nodetest-stdin-*,test-tty-stdin-*,test-readline-*files. On this machinestdin-fixtures.test.ts(a 1 s kill timer) andstdin-pause-pty.test.tsfail on debug builds with and without the line, and pass 3 of 3 on release builds both ways. Nothing ran on macOS or Windows here.no test proof · iteration 2 · 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