repl: pump event loop while waiting for stdin so timers and IPC fire - #30560
repl: pump event loop while waiting for stdin so timers and IPC fire#30560robobun wants to merge 6 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe REPL stdin-read path was refactored to pump the JavaScript event loop and uSockets handlers while waiting for user input. A new ChangesREPL event-loop responsive stdin
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/repl/repl.test.ts`:
- Around line 1096-1106: Remove the in-child 3s watchdog that makes the test
timing-sensitive: delete the setInterval(() => process.exit(1), 3000) call
inside the IIFE (the iife string) so the child only exits via the message
handler (process.exit(0) or process.exit(2)); rely on the test harness/outer
timeout and proc cleanup instead of an embedded timeout to avoid spurious exits.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0ac242de-1224-4710-b608-7e2f463ecbe8
📒 Files selected for processing (2)
src/cli/repl.zigtest/js/bun/repl/repl.test.ts
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/repl.zig`:
- Around line 894-907: The current ordering calls
event_loop.tickImmediateTasks(vm) before loop.tickWithoutIdle() and
vm.timer.drainTimers(vm), so any setImmediate scheduled by I/O/timer callbacks
will be left until the next wake; fix by invoking
event_loop.tickImmediateTasks(vm) again after loop.tickWithoutIdle() and
vm.timer.drainTimers(vm) (i.e., add a second call to
event_loop.tickImmediateTasks(vm) just before checking event_loop.tasks.count)
so setImmediate callbacks enqueued by those callbacks are drained immediately;
keep the original pre-callback call if desired for the original invariant.
- Around line 960-965: When stdin_file.read(&self.stdin_buf) returns 0 on
Windows it indicates a non-character wake (focus/resize), not EOF; update the
branch in repl.zig so that in the else path (after windowsWaitForStdin(&next_ts,
has_deadline) returns false) you inspect the result from stdin_file.read: if
.result |got yields got == 0 then do not return that value to readByte(), but
treat it as a spurious wake and continue the waiting loop (i.e., ignore
zero-length reads and retry waiting/reading); only return nonzero .result values
or propagate actual errors. This change touches the logic around
windowsWaitForStdin, stdin_file.read, self.stdin_buf and readByte handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7bf3d7d8-dfa6-4b14-b2dd-98a485385da5
📒 Files selected for processing (2)
src/cli/repl.zigtest/js/bun/repl/repl.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/repl.zig`:
- Around line 926-930: The POSIX branch computes timeout_ms from next_ts.ms()
but doesn't clamp negatives, causing poll() to wait indefinitely when a deadline
is overdue; update the Environment.isPosix branch that sets timeout_ms (the
variable computed when has_deadline is true) to clamp the computed value to a
minimum of 0 (e.g., use `@max/`@as like the Windows path does) so that when
next_ts.ms() is negative you pass 0 to poll() instead of a negative timeout.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7a9fd930-6d28-4054-a4be-6c67b90c8e9f
📒 Files selected for processing (1)
src/cli/repl.zig
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/repl.zig`:
- Around line 906-909: The current sequence calls
event_loop.tickImmediateTasks(vm) once, then vm.tick(), but vm.tick() can
enqueue additional setImmediate tasks which are never drained before timeout
calculation and vm.onAfterEventLoop(); update the logic around
event_loop.tickImmediateTasks(vm), vm.tick(), event_loop.tasks.count and
vm.onAfterEventLoop() to re-drain immediate tasks after vm.tick() — e.g., loop
calling event_loop.tickImmediateTasks(vm) and vm.tick() (or at minimum call
tickImmediateTasks(vm) again) until both immediate queues remain empty before
invoking vm.onAfterEventLoop(), so no second-order immediates are left stranded.
In `@test/js/bun/repl/repl.test.ts`:
- Around line 1071-1122: The IPC regression test ("process.on('message') fires
in a spawned `bun repl` (`#30559`)") is currently inside
describe.todoIf(isWindows) so it is skipped on Windows where
windowsWaitForStdin/PeekNamedPipe behavior must be exercised; move or duplicate
that single test out of the describe.todoIf(isWindows) block into a normal
describe/test block so it runs on Windows (keep the rest of the PTY/terminal
tests gated by describe.todoIf(isWindows)). Locate the test by its exact title
or the wrapping describe.todoIf(isWindows) and ensure the test still uses the
same proc spawn, IPC callbacks, and expectations but is not under the Windows
TODO guard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bc30969d-45b2-4ce9-9a9c-d2f51c069014
📒 Files selected for processing (2)
src/cli/repl.zigtest/js/bun/repl/repl.test.ts
ac87798 to
e62ae8d
Compare
There was a problem hiding this comment.
The Rust re-port looks faithful to the reviewed Zig version and carries forward all prior fixes, but ~300 lines of fresh unsafe event-loop coordination plus hand-declared Win32 FFI is more than I'm comfortable waving through without a human pass — particularly since the Windows path is todoIf(isWindows) and the latest commit (b0d38d8) just reworked the HANDLE typing to unbreak the Windows build.
Extended reasoning...
Overview
The substantive change is in src/runtime/cli/repl.rs: read_byte() now delegates to a new wait_for_stdin_readable() that pumps the JS event loop (tick → immediates → uSockets I/O → timers → re-drain immediates/tasks/concurrent_tasks → rejections) between polls of stdin + the loop fd, so timers and IPC fire while the REPL waits for input. A Windows variant slices the wait into 50ms chunks and branches on GetFileType + GetConsoleMode (real console → WaitForSingleObject; pipe → PeekNamedPipe + Sleep; NUL/disk → direct read). Two regression tests are added in test/js/bun/repl/repl.test.ts. The other ten touched files (crash_handler, errno, tracy, Arguments.rs, run_command.rs, upgrade_command.rs, jsc_hooks.rs, ChromeProcess.rs, process.rs, spawn_process.rs) are pure cargo fmt reflows of multi-target #[cfg(...)] attributes — no semantic change.
Security risks
None identified. The change reads from stdin and the process-local uSockets loop fd; no new external input surfaces, auth, or privilege boundaries. The hand-declared extern "system" block for WaitForSingleObject / PeekNamedPipe / GetFileType / GetConsoleMode / Sleep is straightforward kernel32 FFI with correct signatures.
Level of scrutiny
This warrants human review. The Zig version of this fix went through ~10 rounds of subtle event-loop-ordering corrections (stranded enqueueTask, second-pass tickImmediateTasks, POLLNVAL, handleRejectedPromises, concurrent_tasks gate, negative-timeout clamp, Windows GetConsoleMode vs FILE_TYPE_CHAR, zero-byte console reads). The Rust port faithfully carries all of those forward — I spot-checked each — but it is a fresh translation into a different language with raw pointers, unsafe aliasing of event_loop / vm, and locally-declared Win32 prototypes that just had their HANDLE typing reworked in the most recent commit to fix a Windows build break. The Windows code path remains describe.todoIf(isWindows) so CI does not exercise it.
Other factors
All prior inline comments (mine and CodeRabbit's) are resolved. The bug-hunting pass on the Rust port found nothing. The robobun CI summary at 3b7ba6f lists many failures, but apart from the windows_sys E0433 errors in repl.rs (addressed by b0d38d8), they are pre-existing main-branch warnings unrelated to this PR. Net: clean from my side, but the combination of event-loop invariants + cross-platform FFI + fresh-port status puts this outside what I'd auto-approve.
b0d38d8 to
5f815b5
Compare
There was a problem hiding this comment.
No new issues in the Rust re-port — prior feedback (POLLNVAL, concurrent_tasks gate, setImmediate re-drain, handleRejectedPromises, GetConsoleMode vs FILE_TYPE_CHAR, 0-byte console reads, timeout clamping) all carried forward correctly. Deferring to a human given the ~200 lines of new unsafe event-loop plumbing and hand-rolled Win32 FFI landing on top of a fresh Zig→Rust port.
Extended reasoning...
Overview
This PR makes the REPL pump the JS event loop while waiting on stdin so timers, IPC, setImmediate, and rejections fire between keystrokes (#30559). It adds wait_for_stdin_readable() (~200 lines) to src/runtime/cli/repl.rs plus three Windows-only helpers with local extern "system" FFI declarations for WaitForSingleObject/PeekNamedPipe/GetFileType/GetConsoleMode/Sleep, and two regression tests in test/js/bun/repl/repl.test.ts. The implementation went through ~10 rounds of inline review on the original Zig version (all resolved), then was re-ported to Rust on 2026-05-15 after #30412 landed.
Security risks
None identified. No auth/crypto/permission surfaces; input is the user's own stdin and the process's own uSockets loop fd. The unsafe blocks are FFI calls and raw-pointer access to per-thread VM/event-loop/uws-loop singletons whose lifetimes are bounded by the REPL loop — not attacker-controlled data.
Level of scrutiny
High. This is a hand-rolled mini event-loop that interleaves vm.tick(), tick_immediate_tasks, tick_without_idle, drain_timers, handle_rejected_promises, and a blocking poll()/WaitForSingleObject — the exact ordering matters and was the subject of five separate stranded-work fixes during review. It uses extensive unsafe Rust (raw *mut casts of &-borrowed VM/event-loop, FFI), declares its own Win32 prototypes rather than using a vetted crate, and the Windows path is acknowledged best-effort with tests gated todoIf(isWindows). The whole thing is a fresh re-port to a different language since the last human-visible review pass.
Other factors
- All prior bot/reviewer feedback is verifiably carried into the Rust port (I checked each: POLLNVAL in the stdin mask,
[0, i32::MAX]timeout clamp, secondtick_immediate_tasksafter I/O,tasks || concurrent_tasksgate,handle_rejected_promises,GetConsoleMode-gated console detection, 0-byte-console-readcontinue,PeekNamedPipefor pipe stdin). - 107/107 REPL tests pass per the description; the two new tests are reasonable and follow house style.
- No CODEOWNERS hit that I'm aware of, but event-loop-shape changes in this repo have historically wanted a maintainer's eyes.
- The current bug-hunter run found nothing new on the Rust version.
5f815b5 to
d5391fd
Compare
d5391fd to
9d3fdc7
Compare
There was a problem hiding this comment.
Commit 09253ca addresses my last nit (local kernel32 externs → bun_sys::windows), and the bug-hunter pass on this revision found nothing — but ~300 lines of new event-loop coordination with extensive unsafe and a Windows path that's todoIf-gated out of CI is more than I'm comfortable auto-approving, so leaving this for a human.
Extended reasoning...
Overview
This PR rewrites the REPL's stdin-read path so the JS event loop is pumped while waiting for keystrokes, fixing #30559 (IPC messages and timers never firing in a spawned bun repl). It adds wait_for_stdin_readable() (~200 lines) plus two #[cfg(windows)] helpers to src/runtime/cli/repl.rs, adds Sleep/PeekNamedPipe/WAIT_OBJECT_0 to src/sys/windows/mod.rs, and adds two regression tests to test/js/bun/repl/repl.test.ts.
The PR has been through roughly a dozen review rounds (from me and CodeRabbit) over ~4 weeks, including a Zig→Rust re-port after #30412 landed. Each round surfaced another stranded-work edge in the pump loop (same-thread tasks, setImmediate, rejected promises, concurrent_tasks, POLLNVAL, negative-timeout clamping, Windows pipe vs. console vs. NUL handling, dead_code lint, QUIC deadline parity, test diagnosability). All of those are resolved; commit 09253ca addresses my last open nit by moving the local kernel32 externs to bun_sys::windows. The current bug-hunter run found nothing.
Security risks
None identified. The change is local to the interactive REPL's stdin wait; no auth, crypto, network listening, or privilege boundaries are touched. The new unsafe blocks dereference per-thread VM/loop pointers that are already established invariants of the surrounding REPL code, and the Windows FFI calls are standard kernel32 readiness probes.
Level of scrutiny
This warrants a human reviewer despite the clean bug-hunter pass. It hand-rolls an event-loop pump shape that has to stay in lockstep with auto_tick/tickPossiblyForever across five separate work queues, and getting that right took twelve iterations — which is itself a signal that the invariants are subtle. The POSIX path is well-exercised by the new tests, but the entire Windows branch (WaitForSingleObject / PeekNamedPipe / GetConsoleMode dispatch, spurious-wake handling) is describe.todoIf(isWindows)-gated and not exercised by CI. The unsafe surface is non-trivial (raw *mut aliasing of event_loop / RuntimeState with PORT-NOTE caveats borrowed from jsc_hooks.rs).
Other factors
One cosmetic follow-up was explicitly deferred by the author (the refresh_line() write-interceptor for non-newline async output), and the EINTR-timeout-restart nit was intentionally left as-is with a stated rationale — both reasonable. The known prompt-clobber trade-off is documented in the thread. Full REPL suite passes (107/107 per the PR description). Nothing is blocking from my side; this just exceeds the "simple/mechanical" bar for bot approval.
Fixes #30559. The REPL's input loop called stdin.read() synchronously from read_byte(), so while waiting for a keystroke the JS event loop was frozen — setTimeout/setInterval never fired, setImmediate never ran, and a `bun repl` child spawned with IPC never saw parent-sent messages reach its `process.on("message")` handler. read_byte() now delegates its refill to a wait_for_stdin_readable() that: - ticks the VM (concurrent tasks, microtasks, setImmediate, rejected promises) and a non-blocking uSockets tick to flush pending I/O, - drains any fully-elapsed timers (POSIX), - re-drains setImmediate + concurrent_tasks queued by those callbacks, and reports unhandled rejections promptly, - then poll()s both stdin and the uSockets loop fd with a timeout sized to the next scheduled timer, so IPC sockets / timers wake us as readily as a keystroke, - redraws the prompt in TTY mode so async stdout doesn't visually clobber partially-typed input. On Windows, where POSIX poll over mixed pipe/console/loop handles isn't available, we slice the wait into 50ms chunks and branch on the handle kind via GetFileType + GetConsoleMode: real consoles keep WaitForSingleObject (with 0-byte reads treated as spurious focus/ resize wakes), pipes use PeekNamedPipe + Sleep, and NUL / COM* / LPT* fall through to a direct read so their 0-byte EOF isn't mistaken for a spurious wake. Also clamp the POSIX poll timeout to [0, INT32_MAX] (a negative ms from an already-overdue timer would otherwise become poll()'s infinite-wait sentinel), include POLLNVAL in the stdin-ready mask so closing fd 0 from inside the REPL cleanly EOFs instead of spinning, and poll the uSockets loop fd alongside stdin so worker-thread completions, IPC messages, and socket events wake us immediately.
Windows CI build failed with "cannot find module or crate windows_sys" because the REPL crate doesn't depend on it. HANDLE on Windows is just `*mut c_void` (`bun_core::util::FdNative`), so inline the externs and signature types with that instead of a crate-path alias.
- add missing SAFETY comments on the quic deadline and Windows get_timeout unsafe blocks (clippy::undocumented_unsafe_blocks) - replace a manual Ok/Err match with .ok() (clippy::manual_ok_err) - drop the unused FILE_TYPE_CHAR constant in windows_wait_for_stdin (dead_code is deny; only is_windows_console_input uses it) - drop a leftover 'let _ = runtime_state' suppression; the Windows branch calls runtime_state() now
- Windows get_timeout now folds in quic_next_tick_us like the POSIX branch and auto_tick, instead of hardcoding None - IPC test: stdout is never read, so don't pipe it; resolve childReady from onExit and surface the child's stderr when it dies before sending "ready", so a startup crash fails with a diff instead of an opaque timeout
…erns WaitForSingleObject, GetFileType, GetConsoleMode and the FILE_TYPE_* constants already exist in bun_sys::windows; add Sleep and PeekNamedPipe to its kernel32 module (the designated growth point) and re-export WAIT_OBJECT_0, then drop the duplicate extern block and constants from repl.rs. GetFileType also regains the syslog instrumentation of the in-tree wrapper.
09253ca to
135c24c
Compare
There was a problem hiding this comment.
No new issues found after the latest round, but this is ~300 lines of event-loop-integrated stdin waiting with several unsafe blocks, aliased-&mut raw-pointer access, and platform-specific Windows branching that CI doesn't exercise (todoIf(isWindows)) — worth a human pass on the pump ordering and the unsafe invariants before merging.
Extended reasoning...
Overview
This PR rewrites the REPL's stdin-read path so the JS event loop is pumped while waiting for keystrokes, fixing #30559 (IPC messages and timers never fire in a spawned bun repl). It adds wait_for_stdin_readable() (~200 lines) to src/runtime/cli/repl.rs, two Windows wait helpers (windows_wait_for_stdin, is_windows_console_input), two new kernel32 externs (Sleep, PeekNamedPipe) plus a WAIT_OBJECT_0 re-export in src/sys/windows/mod.rs, and two regression tests in test/js/bun/repl/repl.test.ts. The PR was originally written in Zig against src/cli/repl.zig and was re-ported to Rust mid-review after #30412 landed.
Security risks
None identified. The change is local to the interactive REPL's input-wait path; it doesn't touch auth, crypto, permissions, network parsing, or untrusted-input handling. The unsafe blocks dereference per-thread VM/event-loop/uSockets pointers that are already established by the surrounding REPL lifecycle, and the Windows FFI calls are read-only handle queries (GetFileType, GetConsoleMode, PeekNamedPipe) plus WaitForSingleObject/Sleep.
Level of scrutiny
High. This is not a config tweak or mechanical refactor — it hand-rolls a miniature event-loop driver that must correctly interleave vm.tick(), tick_immediate_tasks, tick_without_idle, drain_timers, on_after_event_loop, handle_rejected_promises, the same-thread task FIFO, the cross-thread concurrent_tasks MPSC queue, and timer-deadline computation, then sleeps on a platform-specific wait primitive. The review history bears this out: ~13 rounds of feedback uncovered real ordering bugs (stranded enqueueTask, stranded setImmediate, stranded concurrent_tasks, stranded rejected promises, negative-timeout → infinite poll, POLLNVAL spin, Windows pipe-handle misuse, NUL-device EOF spin, zero-byte console read → spurious EOF). All were fixed, but the density of subtle issues in a single function — combined with multiple unsafe blocks relying on the documented-but-unchecked aliased-&mut invariant from auto_tick — argues strongly for a human reviewer to validate the final pump shape against auto_tick/tickPossiblyForever and to sanity-check the Windows branching, which CI does not exercise (both new tests are describe.todoIf(isWindows)).
Other factors
- All prior inline comments (mine and CodeRabbit's) are resolved; the current bug-hunting run found nothing new.
- The author explicitly deferred one item (Node-style write-interceptor for prompt redraw) and declined one (capping POSIX poll timeout to defend against non-Bun signal handlers) with reasonable justifications.
- Test coverage for the headline POSIX scenarios is good (PTY
setTimeouttest + IPC round-trip test), and the IPC test now surfaces stderr on both pre-ready and post-ready failure paths. - No human reviewer has weighed in yet; given the scope and the Zig→Rust re-port, a maintainer sign-off on the event-loop integration is warranted.
|
Heads-up from a neighbouring fix: #33411 makes Ctrl+C interrupt a running REPL evaluation. It only touches the evaluate path ( Two things from that work that may be useful here:
|
1 participant
Repro
The child REPL sends "ready" fine, but the parent's
send("hello")never reaches the child'sprocess.on("message")handler, and thesetIntervalsafety timer never fires either.Cause
The REPL's
read_byte()calledstdin.read()directly, which blocks in the kernel until a keystroke arrives. While blocked, nothing ticks the JS event loop — so pending IPC reads onNODE_CHANNEL_FD,setTimeout/setInterval, andsetImmediatecallbacks never run.Fix
read_byte()now delegates its buffer refill to a newwait_for_stdin_readable()whenever a VM is attached. Each iteration of that wait pumps the VM event loop (tick, immediates, I/O, timers, rejections), thenpoll()s both stdin and the uSockets loop fd with a timeout sized to the next scheduled timer — so an incoming IPC message, timer deadline, or worker-thread completion wakes us as readily as a keystroke.On Windows, POSIX poll doesn't compose over mixed pipe/console/loop handles, so the wait is sliced into 50ms chunks and branches on handle kind via
GetFileType+GetConsoleMode: real consoles useWaitForSingleObject, pipes usePeekNamedPipe+Sleep, and NUL / COM* / LPT* / disk fall through to a direct read.Verification
USE_SYSTEM_BUN=1 bun test test/js/bun/repl/repl.test.ts -t "fires in a spawned"— FAIL (times out at 5s, bug reproduced)bun bd test test/js/bun/repl/repl.test.ts -t "fires in a spawned"— PASSRebase note
Rebased onto main on 2026-05-15 after the Rust rewrite (#30412) landed. The original Zig fix against
src/cli/repl.zigwas re-ported tosrc/runtime/cli/repl.rs— same algorithm, Rust translation. All prior review feedback (concurrent_tasks gate, setImmediate re-drain, POLLNVAL, Windows GetConsoleMode branching, etc.) is carried forward.Fixes #30559