Conversation
|
Updated 12:12 PM PT - Aug 15th, 2026
🔄 @robobun, the build for your commit |
|
Status: Rebased onto current main ( This is a Windows-only libuv patch ( Build #98348 ( Rebase notes: main bumped Why fix the dispatch in Why a patch file rather than landing in the fork first: same route the two existing On the Prior CI: #73609 ( |
WalkthroughUpdates Windows TTY cancellation to choose the cancel path based on the pending read type, registers the libuv patch in the build, and adds a Windows regression test exercising setRawMode toggling around a pending read. ChangesWindows TTY read-stop cancellation fix and regression test
🚥 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/js/node/tty.test.ts`:
- Around line 184-187: The test currently force-kills the child immediately
after key detection which can mask child failures; instead, after awaiting
Promise.race([gotKey.promise, eof.promise]) stop calling proc.kill() there, wait
for the process to exit by awaiting proc.exited, then close
proc.terminal?.close(), and only assert the child's exit code at the end; update
the sequence around gotKey.promise/eof.promise, proc.kill(), proc.exited, and
proc.terminal?.close() so exit is awaited and asserted last.
🪄 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: b9c98ad0-0187-4458-b962-9d4d689d2479
📒 Files selected for processing (3)
patches/libuv/win-tty-read-stop-match-pending-req.patchscripts/build/deps/libuv.tstest/js/node/tty.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
test/js/node/tty.test.ts (1)
184-187:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winWait for the child to exit and assert the exit code last.
Force-killing here can hide a child-side failure after the
KEYwrite. Captureconst exitCode = await proc.exited;, close the terminal after that, and assertexpect(exitCode).toBe(0)after the output assertions.As per coding guidelines, “When spawning processes in tests, expect stdout before expecting exit code for more useful error messages” and “Assert the exit code last in tests.”Proposed fix
- await Promise.race([gotKey.promise, eof.promise]); - proc.kill(); - await proc.exited; + await Promise.race([gotKey.promise, eof.promise]); + const exitCode = await proc.exited; proc.terminal?.close(); output += decoder.decode(); @@ const bytes = JSON.parse(match[1]); expect(bytes).toContain("x".charCodeAt(0)); + expect(exitCode).toBe(0); });🤖 Prompt for 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. In `@test/js/node/tty.test.ts` around lines 184 - 187, The test currently force-kills the child and closes the terminal before awaiting its exit, which can hide failures; replace that sequence by awaiting and capturing the exit code (const exitCode = await proc.exited), then close proc.terminal after awaiting exit, and finally assert expect(exitCode).toBe(0) after all output/assertions (use the existing gotKey.promise and eof.promise waits before checking exitCode); update the teardown to remove proc.kill() so the process exits normally and ensure proc.terminal?.close() runs after exit is awaited.
🤖 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.
Duplicate comments:
In `@test/js/node/tty.test.ts`:
- Around line 184-187: The test currently force-kills the child and closes the
terminal before awaiting its exit, which can hide failures; replace that
sequence by awaiting and capturing the exit code (const exitCode = await
proc.exited), then close proc.terminal after awaiting exit, and finally assert
expect(exitCode).toBe(0) after all output/assertions (use the existing
gotKey.promise and eof.promise waits before checking exitCode); update the
teardown to remove proc.kill() so the process exits normally and ensure
proc.terminal?.close() runs after exit is awaited.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 44b8adae-b084-431c-8efa-c903f0e4e063
📒 Files selected for processing (1)
test/js/node/tty.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues and the analysis is thorough, but this patches libuv's Windows TTY read-cancellation dispatch — a vendored-dependency change whose correctness hinges on the read_line_buffer.len invariant, so it's worth a human sign-off from someone familiar with libuv's win/tty state machine.
Extended reasoning...
Overview
This PR adds a one-line behavioral patch to vendored libuv (src/win/tty.c), changing uv__tty_read_stop() to dispatch its cancellation path on handle->tty.rd.read_line_buffer.len == 0 instead of uv__is_raw_tty_mode(handle->tty.rd.mode.mode). The patch is registered in scripts/build/deps/libuv.ts (mechanical array append), and a ConPTY-backed regression test is added to test/js/node/tty.test.ts following the existing sibling-test pattern from #30288.
Security risks
None identified. The change is Windows-only TTY input-cancellation routing inside libuv; it does not touch auth, crypto, network, filesystem permissions, or any user-controllable parsing path. The worst plausible failure mode is mis-routed read cancellation (hung/dropped stdin), not a security exposure.
Level of scrutiny
This warrants human review despite its small surface area. It modifies a vendored core dependency in a code path that governs how Windows console reads are cancelled — effectively platform-level I/O plumbing for every Bun process with a TTY stdin on Windows. The fix's correctness rests on the invariant "read_line_buffer.len > 0 iff the pending request is a line read," which the PR claims (plausibly) is the same invariant uv__process_tty_read_req() already dispatches on. That claim is well-argued and the root-cause trace is detailed, but verifying it requires libuv-internals knowledge that goes beyond what I'd auto-approve. The patch is also described as upstreamable but has not yet been reviewed by libuv maintainers.
Other factors
- Windows CI is green on all lanes per the build-status comment; the only failure is an unrelated Linux OOM flake in
v8-heap-snapshot.test.ts. - The new test is appropriately skipped on ConPTY v1 (Server 2019 / build < 19041) with a clear rationale, and follows the established
Bun.Terminaltest pattern in the same file. - CodeRabbit's only comment (exit-code assertion ordering) was reasonably rebutted by the author and retracted.
- The test relies on a 50-iteration
setImmediatepropagation window before sending a sentinel keystroke; this is event-loop-turn-based rather than wall-clock so it should be deterministic, but ConPTY input propagation is an out-of-process IPC hop, so there is some residual flake risk worth a human eye. - No CODEOWNERS concerns surfaced, but vendored-dep patches in this repo have historically gotten explicit maintainer review.
9a76de2 to
54f66dc
Compare
There was a problem hiding this comment.
I didn't find any bugs, but this patches vendored libuv's Windows TTY cancellation path — a subtle threading/state-machine change in a critical I/O layer — so it warrants a human look from someone familiar with the Windows event loop.
What was reviewed:
- The
read_line_buffer.leninvariant matches howuv__process_tty_read_reqalready routes completions, anduv__tty_queue_read_raw/_lineset it consistently — the dispatch key looks sound. - Checked that a stale-mode raw-cancel against a raw request is harmless (extra FOCUS_EVENT write, no flag mutation).
- The ConPTY test follows the sibling
updates isRawpattern; the sentinel-'z' fallback keeps the failure mode a clean assertion rather than a hang, and the Server 2019 skip is scoped to the harness limitation only.
Extended reasoning...
Overview
This PR adds a one-line patch to vendored libuv (src/win/tty.c) changing uv__tty_read_stop to dispatch its cancellation path on handle->tty.rd.read_line_buffer.len == 0 instead of uv__is_raw_tty_mode(handle->tty.rd.mode.mode). It registers the patch in scripts/build/deps/libuv.ts and adds a ConPTY-backed regression test in test/js/node/tty.test.ts. The root-cause analysis is unusually thorough: a synchronous setRawMode(false); setRawMode(true) bounce leaves UV_HANDLE_CANCELLATION_PENDING stuck (only uv_process_tty_read_line_req clears it, but the pending req is raw), so the next real cooked-read cancel is skipped and ReadConsoleW blocks until Enter.
Security risks
None identified. This is Windows-only console read cancellation; no untrusted input parsing, auth, crypto, or network surface is touched.
Level of scrutiny
High. This is a patch to a vendored third-party dependency in the Windows event loop's stdin reader-thread cancellation logic — exactly the kind of concurrent state machine where a subtle mistake produces rare hangs or lost input. The libuv source is fetched at build time (github-archive), so the patch cannot be locally verified against the tree in this checkout, and the fail-before/pass-after delta is only observable on Windows CI. The invariant relied on (read_line_buffer.len > 0 iff a line read is pending) is well-argued and matches libuv's own completion-routing discriminator, but a maintainer who owns the Windows I/O layer should confirm it holds across all paths (including the alloc_cb-returns-zero early return in uv__tty_queue_read_line).
Other factors
The test is well-constructed (awaits observable conditions, wires eof to the race, uses a sentinel keystroke to convert the bug's hang into a deterministic assertion failure) and mirrors the existing ConPTY test pattern in the same file. The isConPTYv1 skip is documented and scoped. CodeRabbit's one nit was addressed/retracted. Prior CI runs on the pre-rebase branch showed all Windows lanes green. Still, per the approval guidelines, patches to critical platform I/O paths and vendored dependencies should not be auto-approved.
uv__tty_read_stop() picked its cancellation path from handle->tty.rd.mode.mode. But uv_tty_set_mode() flips mode.mode between its stop/start pair, so a synchronous setRawMode(false); setRawMode(true) runs read_stop twice against the same still-pending raw request. The second call sees mode=Normal, takes the line-read cancel path, and sets UV_HANDLE_CANCELLATION_PENDING with no line request in flight. That flag is only cleared in uv_process_tty_read_line_req, so it sticks. On the next false -> (yield) -> true cycle where a real ReadConsoleW is queued, read_stop sees the stale flag and skips uv__cancel_read_console. The worker thread stays blocked; the next keystroke is consumed by ReadConsoleW, routed through uv_process_tty_read_line_req, and dropped because CANCELLATION_PENDING is set. Observed as frozen-until-Enter / lost keystrokes after Ink App unmount/remount on Windows. Fix: dispatch on read_line_buffer.len (the actual pending-request type), the same invariant uv__process_tty_read_req already uses. A stale-mode read_stop against a raw request now takes the raw cancel path (an extra harmless FOCUS_EVENT) and never sets CANCELLATION_PENDING spuriously. The ConPTY-backed regression test is skipped on Windows builds < 19041 (Server 2019 / 1809): ConPTY v1 does not reliably propagate raw single-byte input after a cooked/raw mode bounce. The libuv fix still ships for those builds; only the harness cannot observe it there. Upstreamable to libuv/libuv as-is.
54f66dc to
bc435d9
Compare
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it patches vendored libuv's Windows console-read cancellation path (threadpool + UV_HANDLE_CANCELLATION_PENDING state), a human sign-off on the vendored-dep change would still be worthwhile.
What was reviewed:
- The
read_line_buffer.len == 0invariant — confirmed it matches howuv__process_tty_read_reqalready routes completions, and thatuv__tty_queue_read_rawsetsuv_null_buf_whileuv__tty_queue_read_lineearly-returns withoutREAD_PENDINGon len==0, so the discriminator is sound. - The reverse case (line read pending, mode already flipped to raw) — takes the line-cancel path as before, no regression there.
- The new ConPTY test — bounded poll with a sentinel fallback rather than a bare sleep;
eofis wired to every race so a child crash surfaces via the KEY-regex throw instead of hanging; follows the same shape as the siblingupdates isRawtest.
Extended reasoning...
Overview
This PR adds a one-line patch to vendored libuv (src/win/tty.c), registers it in scripts/build/deps/libuv.ts, and adds a ConPTY-backed regression test to test/js/node/tty.test.ts. The patch changes uv__tty_read_stop() to select its cancellation strategy based on handle->tty.rd.read_line_buffer.len (the type of the pending request) instead of handle->tty.rd.mode.mode (the current mode, which uv_tty_set_mode mutates between its stop/start calls). The failure mode — a stale UV_HANDLE_CANCELLATION_PENDING flag causing a later cooked ReadConsoleW cancel to be skipped — is traced end-to-end in the PR description and patch comment.
Security risks
None. This is Windows-only console input handling; no auth, crypto, network, or untrusted-input parsing is touched. The change affects which of two existing cancellation paths runs, not what either does.
Level of scrutiny
Higher than the diff size suggests. Per the repo's landing-PRs guidance, changes under vendor/ and patches to vendored deps warrant the situational Dependencies & vendoring review. The affected code manages threadpool worker cancellation and a handle-state flag that gates future cancels — getting the discriminator wrong in the other direction (raw-cancel path against a real line read) would leave a threadpool worker stuck in ReadConsoleW. The invariant chosen (read_line_buffer.len) is the same one uv__process_tty_read_req already uses to route completions, and the PR explains why it holds (raw queue sets uv_null_buf_; line queue fills via alloc_cb and early-returns without READ_PENDING if len==0; line-req completion resets to uv_null_buf_). That reasoning checks out, but a maintainer familiar with the oven-sh/libuv fork should confirm this is the right layer for the fix vs. clearing CANCELLATION_PENDING in uv_process_tty_read_raw_req, and whether it should land in the fork instead of a patch file.
Other factors
- Windows CI is green on all lanes per the robobun status; the two remaining reds (
test-net-connect-memleak.js,require-cache.test.ts) are unrelated main-branch flakes on non-Windows lanes where libuv isn't even compiled (enabled: cfg => cfg.windows). - The test uses a 50-iteration
setImmediatepoll with a sentinel-write fallback rather than a bare sleep, which satisfies the "await the condition, don't sleep" rule; every wait is raced againsteofso a child crash produces a descriptive throw rather than a timeout. - The one CodeRabbit comment (assert exit code) was addressed with a reasonable explanation matching the sibling test's pattern and marked resolved.
- The
isConPTYv1skip carries a specific rationale (ConPTY v1 input propagation on build < 19041) and doesn't skip the fix itself, only the harness's ability to observe it.
This branch has not been deployed
1 participant
What
On Windows,
process.stdin.setRawMode(true)now reliably cancels an in-flight cookedReadConsoleWon the libuv stdin reader thread after a prior synchronoussetRawMode(false); setRawMode(true)bounce. Without this, the next raw-mode keystroke is consumed by the stuck cooked read and silently dropped.Repro (Windows, real terminal)
This is the Ink reattach scenario: App unmount (
setRawMode(false)) → remount (setRawMode(true)), repeated.Root cause
This is a libuv bug in
src/win/tty.c, present both before and after the Rust port (the Rust side —Source__setRawModeStdininsrc/io/source.rs— correctly routes touv_tty_set_modeon the shared process-globalstdin_ttyhandle; the reader'suv_read_startuses that same handle).uv__tty_read_stop()chose its cancellation path fromhandle->tty.rd.mode.mode:But
uv_tty_set_mode()updatesmode.modebetween itsread_stop/read_startpair. When called twice back-to-back in one tick, the secondread_stopruns against the same still-pending raw request (the loop hasn't drained IOCP yet) but seesmode.mode == UV_TTY_MODE_NORMALfrom the first call. It takes the line-read path, callsuv__cancel_read_console()(a no-op — there is no line read), and setsUV_HANDLE_CANCELLATION_PENDING.That flag is only ever cleared in
uv_process_tty_read_line_req(). Since the pending request is a raw request, it completes viauv_process_tty_read_raw_req(), which never touches the flag. It sticks.On the next
setRawMode(false)→ (event loop runs, cookedReadConsoleWis queued on a threadpool thread) →setRawMode(true),uv__tty_read_stopseesCANCELLATION_PENDINGalready set and skipsuv__cancel_read_consoleentirely. The worker thread stays blocked inReadConsoleW. The next keystroke is consumed by it, delivered throughuv_process_tty_read_line_req, and dropped on the floor becauseCANCELLATION_PENDINGis set.Fix
Patch
uv__tty_read_stopto dispatch onhandle->tty.rd.read_line_buffer.len— the type of the pending request — instead ofmode.mode.read_line_buffer.len > 0iff a line read is pending; this is the same invariantuv__process_tty_read_req()already uses to route completions. A stale-moderead_stopagainst a raw request now takes the raw-cancel path (writes an extra harmlessFOCUS_EVENT) and never spuriously setsCANCELLATION_PENDING.Applied as
patches/libuv/win-tty-read-stop-match-pending-req.patch; upstreamable to libuv/libuv as-is.Relationship to #30288
#30288 fixed
process.stdin.isRawtracking so code that readsisRawto decide whether to restore cooked mode no longer spuriously drops to cooked on Windows. This PR covers code that deliberately callssetRawMode(false)(Ink teardown, readlineclose(), any re-init path) and then re-enters raw mode — a path #30288 did not touch.Notes on the single-cycle repro
A single synchronous
setRawMode(false); setRawMode(true)does not hang on its own (traced end-to-end — the raw request completes, the event loop re-arms a raw read, and input flows). It primes the stale flag; one more false→yield→true cycle after that is what hangs. The report's minimal repro happens to produce both effects when the mode bounces more than once (Ink remount, readline re-open).Second failure mode
The report also mentions a stall after
setRawMode(false)+unref()with no listener where a lateron('readable')doesn't fire until a terminal resize. That path also runsuv__tty_read_stopwithmode.mode == Normalagainst a pending raw request (viasetFlowing(false)→WindowsBufferedReader::stop_reading()→uv_read_stop), so it hits the same stale-flag bug and is covered by this fix. If a distinct symptom persists after this lands it is likely the Bun-levelown()/disown()ref dance rather than libuv and should be filed separately.Verification
Added a ConPTY-backed test in
test/js/node/tty.test.tsthat walks the exact two-cycle sequence and asserts the first post-bounce keystroke reaches the child without Enter. The test runs on all platforms; on POSIX it locks in the existing (already-correct) termios behaviour. The affected code is Windows-only (libuvsrc/win/tty.c), so — as with #30288 — the fail-before/pass-after delta is only observable on Windows CI.no test proof · iteration 11 · Platform-specific test-only change; deferring to CI.