fix(runtime): a process.stdin resume() racing the stopping fd-0 reader stranded piped stdin without a reader (#10895) - #10913
proggeramlug wants to merge 2 commits into
Conversation
…eader no longer strands stdin without a reader (PerryTS#10895) The async iterator pauses its source after every delivered chunk and resumes it on the next pull. On process.stdin, pause() latches STDIN_DETACHED — the fd-0 reader thread exits when it sees it at the top of its loop — and resume() clears the latch and respawns the reader unless STDIN_READER_STARTED says one is still running. The reader's stop decision and its STARTED reset were two separate steps, so a resume() that landed between them found STARTED still true, spawned nothing, and the old reader then left: fd 0 had no reader while every liveness view still reported an open, flowing stdin, and the process idled forever with input unread. Make the reader's check-and-clear and the restart CAS atomic with respect to each other under one lifecycle lock (never held across read()). The detach exit now releases the reader slot itself and disarms the drop guard, which otherwise could clobber the flag of a reader respawned in between. Introduced by bb57392 (2026-09-04, unified fd-0 reader): before it, a piped stdin was read by readline's own reader, which never consulted the latch, so v0.5.1520 does not reproduce. Fixes PerryTS#10895
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe fd-0 stdin reader now synchronizes stop, cleanup, and restart claims with a lifecycle mutex. New runtime and end-to-end tests cover pause/resume races and stalled pipe input. ChangesStdin reader lifecycle fix
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.89% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1🧪 Generate unit tests (beta)
🛠️ Fix failing CI checks 💡
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 |
|
Landed on The train was validated as one tree: all ratchets, |
1 participant
Fixes #10895
What broke
for await (const chunk of process.stdin)on a pipe stopped part-way through the input and never resumed: 0 % CPU, the writer blocked, the process idle forever.The async iterator pauses its source after every delivered chunk and resumes it on the next pull (
node_stream/async_iterator.rs,ns_readable_iter_on_data/ns_readable_iterator_next). Onprocess.stdin,pause()latchesSTDIN_DETACHED— the fd-0 reader thread exits when it sees the latch at the top of its loop — andresume()clears the latch and respawns the reader unlessSTDIN_READER_STARTEDis still true. The reader's stop decision and itsSTARTEDreset (a drop guard) were two separate steps:fd 0 then has no reader at all (a stalled process shows only the main thread, in
js_wait_for_event), whilejs_readline_has_activestill reports a started, un-paused, flowing stdin — so the loop stays alive and idles forever.Why it is timing- and platform-dependent
One roll of that dice per delivered chunk. A 16 KiB macOS pipe fed in small writes delivers ~170 chunks/MiB, so 1 MiB is a coin flip there and 4 MiB is near-certain. A 64 KiB Linux pipe delivers ~16 chunks/MiB and the window is a handful of instructions, so it needs preemption to land: 1 hang in 350 runs at 16 MiB, under load average ~30.
Introduced by bb57392 (2026-09-04, "unify process.stdin reader surface"). Before it a piped stdin was read by perry-stdlib readline's own reader, which never consulted the latch — that is why the published 0.5.1520 is clean.
The fix
crates/perry-runtime/src/os_process_streams.rs: one lifecycle lock makes the reader's check-and-clear (stdin_reader_claim_stop) and the restart CAS (stdin_reader_claim_start) atomic with respect to each other. A restart request now runs either entirely before the stop decision (the reader sees the cleared latch and keeps going — no second reader on fd 0) or entirely after it (STARTEDis already false, a fresh reader is spawned). The lock is never held acrossread(). The detach exit releases the slot itself and disarms the drop guard, which otherwise could clobber the flag of a reader respawned in between.Hang rates, same driver (
communicate()of N bytes, 15 s deadline)841b605c97841b605c97Binaries hashed on both hosts; the two arms are distinct.
Tests
crates/perry/tests/issue_10895_stdin_pipe_stall.rs+test-files/test_issue_10895_stdin_pipe_stall.ts: pipes 8 MiB in 256-byte writes, 12 rounds, 60 s deadline each, asserts every byte reaches the iterator. Red on unpatched main (841b605c97+ this test only, macOS):round 0: piped stdin stalled — the child was still alive after 60s with 2170368 of 8388608 bytes not even accepted by the pipe. Green with the fix: 12/12 on macOS (--release), 12/12 on Linux run CI-style (cargo test -p perrywithPERRY_RUNTIME_DIR=target/release). The fixture is inert when undriven (RESULT:idle), so the parity sweep never waits on stdin.reader_lifecycle_testsinos_process_streams.rs: replays the exact interleaving on local flags (deterministic), the opposite order (no second reader), and a 200k-iteration pause/resume storm from two threads.Verification
cargo test --lib -p perry-runtime(full,RUST_TEST_THREADS=1, Linux): 4203 passed, 0 failed, 4 ignored.issue_9692_stdin_surface,issue_9676_stdin_unref_ref_keeps_reader,issue_9594_readline_close_pauses_stdin,issue_9588_readline_reader_notifies_the_pump,issue_9593_readline_escape_timeout,issue_stdin_end_listener— all green.cargo fmt --check, product lintRUSTFLAGS="-D warnings" cargo check -p perry --bins— green (Linux).s.on("data")with a pause/resume storm delivers every byte (4 MiB); a program that never touches stdin exits immediately with stdin held open;process.stdin.unref()still does not stop the reader (TUI input dies after real use: bytes reach the process and are consumed, but never reach JS — live forensics point at the GC (swept listener) #9676 kept).warning: function relevant_box_roots is never used(box.rs:1038) in the release build of both arms.Version not bumped (merge train). Found while getting the Native Messaging host from https://github.com/guest271314/NativeMessagingHosts to run (jlucaso1/js-compiled#2); the "garbage frame header" noted in the issue is a separate, deterministic bug in
process.stdout.write(#10903, owned separately).