hot: reset process.stdin/stdout/stderr listeners on reload - #30633
hot: reset process.stdin/stdout/stderr listeners on reload#30633robobun wants to merge 16 commits into
Conversation
|
Updated 11:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 667ee41 has some failures in 🧪 To try this PR locally: bunx bun-pr 30633That installs a local version of the PR into your bun-30633 --bun |
|
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:
WalkthroughAdds a builtin reset identifier and stream helpers, implements a C++ orchestrator invoked during GlobalObject::reload(), and adds a regression test to ensure process stdin/stdout/stderr listeners do not accumulate across hot reloads. ChangesStdio listener reset on hot reload
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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/cli/hot/hot.test.ts`:
- Around line 754-773: The fire-and-forget async IIFEs that iterate
runner.stdout and runner.stderr can raise unhandled rejections when
runner.kill() aborts the streams; wrap each invoked async IIFE (the ones reading
from runner.stdout and runner.stderr) with .catch(() => {}) so any stream-abort
errors are swallowed silently, e.g., the async () => { for await (const chunk of
runner.stdout) ... }() and the async () => { for await (const chunk of
runner.stderr) ... }() invocations should have .catch(() => {}) appended to
their returned promises to avoid unhandled rejections.
🪄 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: 5dac22a1-5f69-4ae3-b637-b956b95217d9
📒 Files selected for processing (7)
src/js/builtins/BunBuiltinNames.hsrc/js/builtins/ProcessObjectInternals.tssrc/jsc/bindings/BunProcess.cppsrc/jsc/bindings/BunProcess.hsrc/jsc/bindings/ZigGlobalObject.cpptest/cli/hot/hot-stdin-readline-fixture.jstest/cli/hot/hot.test.ts
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
StatusReady for review. Current head is 667ee41. Its CI run, build 98234, passed every job that ran (177/177: all Linux lanes including ASAN, and both Windows lanes) and ended red only because the Latest changes
Verification on the rebased branch
CI notes
Fix summary
|
7f5f61f to
9c9e1f1
Compare
9c9e1f1 to
bc5513f
Compare
4e96927 to
d5bf791
Compare
In --hot mode the JSGlobalObject and its process.stdin/stdout/stderr streams survive module-registry reloads. User code (notably node:readline) that attached 'data'/'keypress'/'error'/'end' listeners on the previous evaluation remained attached, so after the entry point was re-run a second set of handlers stacked on the same stream. Every keystroke was then handled by both the old and new readline instances, producing doubled (then quadrupled, etc.) echo in the terminal. On reload, drop user-attached listeners from stdin/stdout/stderr and re-add the internal own()/disown() wiring so the stream is clean for the fresh evaluation. Fixes #15027.
hot.test.ts has a pre-existing timing-sensitive test that fails on debug builds; keep the stdin-listener coverage in a dedicated file so it can be run in isolation. Fixture is inlined via tempDir().
resetStdioForHotReload() declares a ThrowScope whose destructor simulates a throw on return; without an intervening check the next scope constructed by requireMap()->clear() trips validateExceptionChecks=1 on the asan lane.
removeAllListeners() leaves the Readable's kDataListening bit set and schedules updateReadableListening() on the next tick, which would call self.resume() → own() and undo the disown() in the reset. It also leaves kFlowing set, so a chunk pushed by an in-flight internalRead before the new load attaches its handler would be emitted to zero listeners instead of buffered. Remove 'data' listeners individually (clears kDataListening) and pause() (clears kFlowing) before removeAllListeners().
- unpipe() before removeAllListeners() so _readableState.pipes from the previous load isn't pinned across reloads. - The Windows file watcher can fire multiple events for a single writeFileSync, so one reload trigger may advance the load counter by more than one. Rework the test to assert the invariants (listener counts stay at 1; exactly one ECHO per input from a post-reset load) without pinning exact load numbers.
readline sets raw mode (ISIG cleared). Once the reset strips the keypress handler that translated ^C, Ctrl+C is dead if the new load doesn't re-enter raw mode itself. Restore cooked mode so the terminal behaves after a reload that stops using readline.
node:readline is cached in the InternalModuleRegistry and is not re-evaluated on --hot reload, so its module-local KEYPRESS_DECODER / ESCAPE_DECODER symbols keep pointing at properties left on process.stdin. emitKeypressEvents() early-returns when it sees them, so after the reset removed the data→keypress bridge it was never reinstalled and terminal-mode readline went silent after a reload. Delete those properties in the reset so the next createInterface() wires keypress events up again. Add a PTY-driven test covering the terminal path: asserts one data/keypress/resize listener on each load, exactly one echo of the pre-reload line, and exactly one echo of the post-reload line from a post-reload interface.
macOS/BSD report PTY EOF as an empty read instead of EIO; break out of the wait loop in that case so a failing run doesn't spin on the closed fd until the deadline.
The hand-rolled fork doesn't give the child a controlling TTY, so closing the PTY master won't SIGHUP it. Handle SIGTERM/SIGINT (test runner timeout, Ctrl+C) like SIGALRM and kill the child explicitly so a failing run doesn't leak a bun --hot process.
READY 2 is only printed after createInterface() has synchronously re-wired the data→keypress bridge, so waiting for it is sufficient; typed input is kernel-buffered regardless.
The emitKeypressEvents() idempotence guard checks only KEYPRESS_DECODER; ESCAPE_DECODER is overwritten unconditionally once the guard passes. Deleting the escape decoder was unnecessary and could turn a pending escape-sequence timer from the previous load into an uncaught TypeError if the next load doesn't re-enter terminal readline.
main now installs an auto-destroy 'end' handler on process.stdin, so the expected baseline is data=1 error=1 end=2 resize=0.
d5bf791 to
200b431
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed the current state of this PR (post-rebase onto 3e08719) and found no new issues. Given the number of subtle interactions here — stream kDataListening/kFlowing state, TTY raw-mode restoration, the readline KEYPRESS_DECODER marker surviving InternalModuleRegistry, JSC exception-scope simulation in GlobalObject::reload() — a human look is still worthwhile before merge.
What was reviewed:
- The stdin reset sequence in
ProcessObjectInternals.tsagainstnode:readlineand Readable-stream internals (per-listenerremoveListener('data')clearskDataListening;escape-decoderintentionally left in place so a pending escape timer doesn't throw). resetStdioForHotReload()inBunProcess.cpp:getDirect()avoids reifying lazy streams;CLEAR_IF_EXCEPTIONper iteration; theassertNoExceptionExceptTermination()inZigGlobalObject.cppbefore the lockedclearAll().- Both new tests (pipe-mode listener counts now
1 1 2 0after the internalendhandler landed on main; PTY driver's EOF/signal handling and the removedsleep(0.3)). - The comment-cop flags on the multi-line comments were examined and ruled out — they document non-obvious stream-state and InternalModuleRegistry behaviour, not workarounds.
Extended reasoning...
Overview
This PR fixes #15027: in --hot mode, process.stdin/stdout/stderr survive module-registry reloads, so listeners attached by the previous evaluation (notably node:readline) stack up and every input byte is processed N+1 times after N reloads. The fix adds a private $resetStdioForHotReload builtin on each stdio stream and calls it from GlobalObject::reload() via a new Bun::resetStdioForHotReload() C++ helper that iterates only already-reified streams (getDirect()). Seven files: one builtin-name addition, the JS reset logic in ProcessObjectInternals.ts, the C++ dispatch in BunProcess.cpp/.h, the call site in ZigGlobalObject.cpp, and two new test files (a pipe-mode TS test plus a Python PTY driver for terminal mode).
Security risks
None identified. The reset function is stored on a private builtin name (unreachable from user code), and resetStdioForHotReload() is only called from the --hot reload path. The PTY test driver forks/execs the debug bun binary in a temp dir with no network or privilege escalation.
Level of scrutiny
High. This touches the --hot reload path in ZigGlobalObject.cpp, JSC exception-scope discipline (a prior round of this PR tripped validateExceptionChecks), and the getStdinStream builtin whose closure state (reader, forceUnref, needsInternalReadRefresh, stream_destroyed) interacts with Readable's kDataListening/kFlowing machinery and TTY raw mode. The PR has already been through ~13 rounds of automated review that surfaced real issues (missing unpipe(), missing setRawMode(false), the KEYPRESS_DECODER early-return, the escape-decoder timer TypeError, PTY EOF spinning on macOS, orphaned --hot children on SIGTERM), all of which were addressed. That history is exactly why a human should confirm the final composition rather than auto-approving.
Other factors
All prior inline findings from this reviewer are marked resolved with corresponding fix commits. The only unresolved thread is today's comment-cop bot flagging multi-line comments in ProcessObjectInternals.ts/BunProcess.cpp/ZigGlobalObject.cpp; the bug-hunting system examined and refuted these — the comments explain non-obvious behaviour (why removeAllListeners() alone would let updateReadableListening re-own(), why escape-decoder must not be deleted, why getDirect() is used) rather than justifying a workaround, which is what CLAUDE.md rule #13 targets. The rebase notes indicate a recent conflict resolution with a cellLock() guard and a new internal end listener on main; the test's expected end=2 count and the ordering of resetStdioForHotReload() before the locked clearAll() block look correct for that.
…sserting reload() now uses the ordinary RETURN_IF_EXCEPTION after the call, and the per-stream loop stops early on a termination exception. Trim the stdio reset comments down to the facts that are not visible from the code.
There was a problem hiding this comment.
I reviewed this PR again after the rebase and comment-trimming pass — no bugs found. Because it introduces a new hot-reload/stdio coupling (private @resetStdioForHotReload on each stream, invoked from GlobalObject::reload()) with subtle interactions across readline internals, TTY raw mode, and Readable stream state, a human look at the overall approach would still be worthwhile.
What was reviewed:
- The new
tryClearException()loop inresetStdioForHotReload— returnsfalseonly on termination, so normal exceptions are swallowed per-stream and termination propagates;RETURN_IF_EXCEPTIONinreload()satisfies the exception-scope validator. - The
end=2baseline update — matches the new internalonStreamEndhandler, which the reset re-registers so the count stays fixed across reloads. - The remaining comment-cop flags on lines 343/351 — those are 2–3-line explanations of cross-module invariants (InternalModuleRegistry caching,
kDataListening), not workaround justifications; I don't think they need further trimming.
Extended reasoning...
Overview
This PR fixes #15027 by adding a private $resetStdioForHotReload builtin to each of process.stdin/stdout/stderr, invoked from GlobalObject::reload() before the module registry is cleared. For stdin the reset disowns the native reader, restores cooked TTY mode, deletes readline's keypress-decoder marker, unpipes, removes data listeners individually (to clear kDataListening), pauses, strips all listeners, and re-registers the four internal handlers. For stdout/stderr it just calls removeAllListeners(). Changes span ProcessObjectInternals.ts, BunProcess.{cpp,h}, ZigGlobalObject.cpp, BunBuiltinNames.h, plus a two-case test (pipe + PTY).
Since my last round on 2026-05-26 the branch was rebased and picked up two changes: (1) the pipe-mode test's expected end count moved from 1 to 2 to account for main's new internal auto-destroy end listener, and (2) the C++ side switched from CLEAR_IF_EXCEPTION + assertNoExceptionExceptTermination to a per-stream if (!scope.tryClearException()) return; with RETURN_IF_EXCEPTION in the caller — a strict improvement that lets termination stop the loop while normal exceptions on one stream don't block the others.
Security risks
None identified. The reset is only reachable from --hot reload on the main thread; it uses getDirect() on the process object so it never triggers user getters, and looks up the reset function via a private builtin name, so user code can't intercept or replace it. setRawMode(false) restoring cooked mode is a hardening step (keeps ^C alive), not an exposure.
Level of scrutiny
Medium-high. The change is not large in LOC but is dense in cross-cutting invariants: readline's module-local symbols surviving via InternalModuleRegistry, the Readable kDataListening/kFlowing state machine, TTY raw mode and ISIG, JSC exception-scope discipline, and --hot's partial-teardown semantics. Each of these produced a real bug during earlier review rounds (silent terminal after reload, escape-timer TypeError, PTY spin on macOS EOF, leaked --hot grandchild on test-runner timeout) — all now fixed and covered by the two tests. That history argues the mechanism deserves a maintainer's eye on whether @resetStdioForHotReload is the right layer for this cleanup versus, say, clearing more state in reload() itself.
Other factors
All ten prior inline threads from this reviewer are resolved with corresponding commits. No human reviewer has looked at the PR. The two unresolved comment-cop flags (github-actions) are heuristic style nags on comments that were already trimmed once in 667ee41; the surviving comments explain non-local invariants rather than justify workarounds, so I would not treat them as blocking. The tests are condition-driven (no arbitrary sleeps remain), skip on Windows for the PTY case, and reap the --hot grandchild on SIGTERM/SIGINT/SIGALRM.
1 participant
Fixes #15027
Problem
bun --hot, a script usingnode:readlineonprocess.stdinprocesses every line (or keystroke) twice after one reload, three times after two, and so on (hheellllooecho in the issue).GlobalObject::reload()(src/jsc/bindings/ZigGlobalObject.cpp) only clears the module loader and the require map.processand itsstdin/stdout/stderrstreams are the same objects across reloads, so every load adds its own'data'/'keypress'/'resize'listeners on top of the previous load's.node:readlinelives in the internal module registry and is not re-evaluated on reload, so after the listeners are dropped itsemitKeypressEvents()still sees its marker onprocess.stdinand does nothing, which would leave readline silent.Fix
ProcessObjectInternals.tsgets a private@resetStdioForHotReloadmethod.reload()callsBun::resetStdioForHotReload()(BunProcess.cpp), which invokes it on whichever ofstdin/stdout/stderrhave already been created (getDirect(), so a never-touched stream is not constructed just to be reset).stdout/stderr:removeAllListeners(). They have no internal listeners.stdin, in order:disown()), and leave raw mode if the previous load entered it;keypress-decodermarker so the nextcreateInterface()installs its keypress bridge again;unpipe(), remove'data'listeners one at a time,pause(), thenremoveAllListeners();resume/pause/end/closehandlers (now named functions so they can be re-added) and clear_readableState.reading, the same stateonStreamPauseleaves behind.reload()returns through the usualRETURN_IF_EXCEPTION.test/cli/hot/hot-stdin.test.ts:'data'listener and each line is echoed once, by the newest load;hot-stdin-pty.py, skipped on Windows): onedata/keypress/resizelistener on every load,worldechoed once after the reload.src/changes both tests fail: pipe mode reportsLISTENERS 2 2 2 3 0and echoes from the old load, PTY mode printswwoorrllddand twoECHO worldlines.hot.test.tsandhot-stdin.test.tsalso pass withBUN_JSC_validateExceptionChecks=1;test/js/node/process/process-stdin.test.tspasses.Background
--hotreload:GlobalObject::reload()drops the module registries and re-evaluates the entry point in the sameJSGlobalObject. Anything hanging off globals, includingprocess, survives.node:*modules are evaluated once per process and cached. Module-level state innode:readline, such as theSymbol("keypress-decoder")it stores on a stream onceemitKeypressEvents()has run, therefore survives a reload too. The marker is found by its description rather than imported, because the symbol is private tointernal/readline/emitKeypressEventsand requiring that module from the stdin reset would load readline into every--hotprocess on its first reload. The companionescape-decodergenerator is left in place on purpose:emitKeypressEvents()replaces it anyway once the marker is gone, and a still-pending escape-sequence timeout from the old load calls.next()on it.'data'listeners:Readable.prototype.removeListener('data', fn)clears the stream'skDataListeningflag when the last listener goes;removeAllListeners()does not, and instead schedulesupdateReadableListening(), which would callresume()and re-acquire stdin on behalf of a load that no longer exists.pause()clearskFlowing, so a chunk that was already in flight is buffered for the next load instead of being emitted to nobody.unpipe()is the only call that clearsstate.pipes.setRawMode(true)turns off terminal signal generation (ISIG); readline's own keypress handler is what turned Ctrl+C into SIGINT. Once that handler is removed, the terminal has to go back to cooked mode or Ctrl+C stops working until the new load happens to enter raw mode itself.resetStdioForHotReload()declares aThrowScope, so like any other throwing JSC function its caller has to check for an exception afterwards;reload()does that withRETURN_IF_EXCEPTION, which is also what satisfiesBUN_JSC_validateExceptionChecks. Inside the loop,tryClearException()clears ordinary exceptions but refuses to clear a termination, which is how the loop knows to stop.