tty: track raw mode per handle instead of per process - #33527
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughChangesTTY raw-mode bookkeeping now uses caller-owned per-handle state across native bindings, Rust runtime consumers, JavaScript streams, and terminal wrappers. POSIX regression tests cover independent state for multiple terminals and streams. TTY raw-mode state
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Found 5 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.
I didn't find any issues, but this is a cross-cutting ABI change (new state buffer threaded through C++/Rust/JS FFI) that alters setRawMode semantics for Node compat, so it's worth a maintainer look.
Extended reasoning...
Overview
This PR fixes a real bug: Bun__ttySetMode kept current_tty_mode and orig_tty_termios as file statics, so raw-mode bookkeeping was process-global instead of per-handle. The fix moves that state into a caller-owned BunTTYState { mode, orig_termios } buffer (mirroring libuv's per-uv_tty_t fields) and threads it through every call site: node:tty ReadStream (lazily-allocated Uint8Array), tty_wrap TTYWrapObject (C++ member), Bun.Terminal / REPL / kitty-graphics probe (Rust tty::State). The process-wide uv_tty_reset_mode() snapshot is correctly left alone. Ten files across C++, Rust, and TypeScript.
Security risks
None. No untrusted-input parsing, auth, or crypto. The one FFI concern — passing an unaligned JS Uint8Array as void* into C++ that reads it as a struct — is handled correctly by memcpy-in/memcpy-out into a stack BunTTYState before use, and the JS entry point validates length() >= Bun__ttyStateSize() and !isDetached().
Level of scrutiny
This deserves a real human review. It's a well-reasoned, well-tested fix, but it changes an FFI signature consumed from three languages, alters observable setRawMode semantics for Node.js compatibility, and touches platform-sensitive terminal code. The alignment/lifetime story for the state buffer is subtle enough (opaque bytes sized at runtime, memcpy round-trip, per-stream ownership) that a maintainer familiar with the tty layer should confirm the design.
Other factors
The tests are strong: they drive a real PTY via Bun.Terminal and assert on terminal.localFlags (actual kernel termios) rather than Bun's own bookkeeping, with a phased handshake and early-exit rejection wired in. The PR description documents byte-for-byte parity with Node on the reporter's repro and lists the local test suites run. No CODEOWNERS cover these paths. I found no correctness issues in the diff.
|
I checked each of these against the diff, and on a real PTY where it was testable, rather than taking the match at face value. None of the five are fixed by this PR, so I am not adding the The change only alters behavior when two or more tty handles exist in one process. A single stream doing
Two of them may be stale rather than open: on linux, minimal repros of #6862 ( The repro in the description remains the one this fixes: a second |
There was a problem hiding this comment.
No bugs found, but this is a cross-language change (Rust/C++/TS) that reworks the Bun__ttySetMode FFI signature and threads per-handle termios state through node:tty, tty_wrap, Bun.Terminal, and the REPL — worth a human look at the ABI/state-buffer design and the Node-compat semantics.
Extended reasoning...
Overview
This PR moves raw-mode tracking from process-global statics to per-handle state, mirroring libuv's uv_tty_t. It touches 10 files across three languages: a new BunTTYState struct in C++ (BunTTYState.h, wtf-bindings.cpp, ProcessBindingTTYWrap.cpp), a Rust tty::State wrapper over an opaque byte buffer (bun_core/tty.rs) with call-site updates in Terminal.rs, repl.rs, and ansi_renderer.rs, a per-ReadStream Uint8Array state buffer in src/js/node/tty.ts, and two new PTY-driven tests. The Bun__ttySetMode FFI signature grows a void* state parameter that is memcpy'd in and out to sidestep alignment.
Security risks
None apparent. The new void* state parameter is validated on the JS entry path (dynamicDowncast<JSUint8Array> + detach/length check against Bun__ttyStateSize()), and the Rust and C++ callers own their buffers with matching lifetimes. The memcpy-in/out pattern avoids alignment UB from the JS-owned Uint8Array. No new user-controlled input reaches size arithmetic or path handling.
Level of scrutiny
This warrants maintainer review. It changes an FFI ABI shared by Rust, C++, and generated JS bindings; alters node:tty semantics (a Node-compat surface CLAUDE.md flags for extra scrutiny); adds a field to TTYWrapObject and Terminal; and introduces a design pattern (opaque state bytes owned by the caller, sized via Bun__ttyStateSize(), round-tripped by memcpy) that a maintainer should sign off on before it becomes precedent. The behavior is also platform-sensitive (POSIX-only code paths, #[cfg(unix)] fields, Windows no-ops) and PTY-dependent, which CI covers less thoroughly than the local runs cited.
Other factors
The PR description is thorough, cites Node/libuv behavior, and reports passing local runs across the relevant tty/terminal/readline suites plus rust:check-all on all targets. The two new tests observe real termios via terminal.localFlags rather than Bun's own bookkeeping, which is the right shape. No prior reviews or comments exist on the PR. Given the cross-cutting scope and the Node-compat surface, deferring rather than auto-approving.
Status: diff is green, red lanes are unrelated flake/infraThe code lanes pass. Across the two CI runs on this branch, every failure has been off-diff and none has recurred, which is the signature of lane flake rather than a regression: Build 69190
Build 69283 (after one
None of these exercise tty, terminal, stdin raw mode, or any file this PR changes. The three failures from the first run did not reproduce in the second. This change is POSIX-only at runtime ( I have used my one retrigger. Leaving the remaining red to a maintainer rather than pushing another empty commit. Happy to rebase or re-run if that helps. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
The bun_core/tty.rs code is complete slop with duplicate type definitions. Rewrite it. Or move it to only live in the right crate.
|
Rewritten in 21dbd45. #[repr(C)]
#[derive(Copy, Clone)]
pub struct State {
mode: c_int,
#[cfg(unix)]
orig_termios: libc::termios,
}Dropped: the I left |
There was a problem hiding this comment.
Beyond the inline comment nit, I also checked the Rust State ↔ C++ BunTTYState layout equivalence — both are { int mode; struct termios } under #[repr(C)]/native C layout with libc::termios matching system <termios.h>, so the sizeof(BunTTYState) memcpy in Bun__ttySetMode reads/writes exactly the Rust struct. The JS Uint8Array path is zero-initialized to rawModeStateSize bytes, length-checked in jsTTYSetMode, and the memcpy-in/out makes its (lack of) alignment a non-issue.
Extended reasoning...
This is a well-constructed fix that moves process-global tty mode state to per-handle state, matching libuv's uv_tty_t model. The only finding is a stale comment parenthetical. I examined the cross-language struct layout invariant (Rust State must byte-match C++ BunTTYState for the memcpy to be sound) and the JS Uint8Array sizing/alignment path, and both hold. Not auto-approving because this changes an FFI signature across three languages and touches the raw-mode primitive that every TUI/REPL path depends on — worth a human glance even though nothing looks wrong.
There was a problem hiding this comment.
No issues found, but this is a cross-language FFI change (Rust #[repr(C)] ↔ C++ BunTTYState ↔ JS Uint8Array) that reshapes a core terminal primitive across ~10 files, so it warrants a maintainer sign-off on the final shape.
Checked: all Bun__ttySetMode call sites migrated to the 3-arg form (grep confirms no stragglers; c-bindings.cpp hit is a comment). The Rust State layout matches BunTTYState field-for-field, and the memcpy-in/out wrapper means a size mismatch would be caught by Bun__ttyStateSize() on the JS path. The process-wide orig_termios_fd/uv_tty_reset_mode atexit snapshot and bun_stdio_modified[] signal-exit path are preserved unchanged. The earlier stale-comment nit was fixed in cb6af4f.
Extended reasoning...
Overview
This PR moves setRawMode state from two file-static globals (current_tty_mode, orig_tty_termios) to per-handle state, mirroring libuv's uv_tty_t. The change spans three languages: a new C++ BunTTYState header and a void*-taking Bun__ttySetMode that memcpys state in/out; a Rust #[repr(C)] State struct in bun_core/tty.rs with all Rust callers (RawModeGuard, REPL, Bun.Terminal, kitty-graphics probe) updated to own one; a per-ReadStream Uint8Array in src/js/node/tty.ts; and a per-TTYWrapObject field in ProcessBindingTTYWrap.cpp. Two new PTY-driven tests assert on real termios via terminal.localFlags.
Security risks
None identified. This is termios bookkeeping on already-open fds; no untrusted input parsing, no auth/crypto. The jsTTYSetMode host function validates the Uint8Array argument (dynamicDowncast, isDetached, length() < Bun__ttyStateSize()) before dereferencing typedVector(), and the buffer is only ever reachable from the internal node:tty builtin.
Level of scrutiny
Medium-high. setRawMode is the foundational TUI primitive — every REPL, prompt library, and Bun.Terminal goes through it — and the fix requires libc::termios (Rust) and struct termios (C++) to have identical layout on each target. That should hold since both compile against the same platform headers, and the void* + memcpy indirection means the C++ side never dereferences the Rust pointer as a struct directly. But cross-language struct-layout equivalence across linux/macOS × x64/aarch64 is exactly the class of change a maintainer should confirm rather than a bot.
Other factors
The design already went through one round of maintainer feedback (the 21dbd45 rewrite from RefCell<Box<[u8]>> to a plain #[repr(C)] struct), and the author left an open question about folding Winsize into this PR — a human should close that loop. CI code lanes are green per the author's status comment; rust:check-all passes across all targets. My previous inline nit (stale "boxed slice" comment) was addressed in cb6af4f and the thread is resolved. Grep confirms every Bun__ttySetMode / tty::set_mode call site was migrated.
Bun__ttySetMode kept one file-static mode and one saved termios shared by every stream, fd and Bun.Terminal. libuv stores both on each uv_tty_t, so in Node each tty.ReadStream tracks its own mode and tearing one down never disturbs another. Give every handle its own state, as libuv does: node:tty keeps a state buffer per ReadStream, tty_wrap keeps one per TTYWrapObject, Bun.Terminal one per PTY, and the Rust callers one per owner. The uv_tty_reset_mode() snapshot stays process-wide, as it is in libuv.
Drop the RefCell<Box<[u8]>> indirection and the Bun__ttyStateSize()
runtime sizing. State is now a Copy #[repr(C)] { c_int, libc::termios }
matching C++'s BunTTYState, zeroed via ffi::Zeroable, with set_mode
taking &mut self.
cb6af4f to
aae13af
Compare
|
Root-caused the darwin 14 x64 hang: it is not this PR. It is a pre-existing bug on this branch's merge base ( Bisection on a macOS 14 x64 box with a standalone 11-way concurrent
The merge base hangs; current main does not. The bug was on main, fixed on main, and this branch had not picked the fix up yet. The earlier two CI runs never surfaced it because the darwin x64 lane happened to expire on both. Rebased onto |
![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 `@src/jsc/bindings/ProcessBindingTTYWrap.cpp`:
- Around line 224-236: Move the existing `state` validation in the TTY mode flow
to after both `fd.toInt32()` and `mode.toInt32()` complete, preserving their
exception checks. Revalidate that `state` is a non-detached `JSUint8Array` with
sufficient length immediately before `Bun__ttySetMode` uses
`state->typedVector()`, and remove the earlier validation.
🪄 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: 14956f56-7f85-441c-8ff0-66f027e978fe
📒 Files selected for processing (10)
src/bun_core/tty.rssrc/js/node/tty.tssrc/jsc/bindings/BunTTYState.hsrc/jsc/bindings/ProcessBindingTTYWrap.cppsrc/jsc/bindings/wtf-bindings.cppsrc/md/ansi_renderer.rssrc/runtime/api/bun/Terminal.rssrc/runtime/cli/repl.rstest/js/bun/terminal/terminal.test.tstest/js/node/tty.test.ts
Moves the Uint8Array detach/length check to immediately before typedVector() so a detach from inside the fd/mode coercions is caught.
There was a problem hiding this comment.
No bugs found, but deferring to a human reviewer — this changes the Bun__ttySetMode FFI signature and threads a #[repr(C)] state struct across the Rust/C++ boundary, which is worth a maintainer's eyes on the ABI contract.
Checked: Rust State { c_int, libc::termios } vs C++ BunTTYState { int, struct termios } layout match; the memcpy in/out handles the unaligned Uint8Array caller and is a no-op for the aligned Rust/C++ callers. The detach/length check on the JS state buffer now sits after both toInt32 coercions (632d2f9). The process-wide orig_termios_fd/uv_tty_reset_mode atexit snapshot is preserved unchanged, matching libuv. All Rust callers (RawModeGuard, REPL, Bun.Terminal, kitty probe) were migrated to per-owner state.
Extended reasoning...
Overview
This PR moves TTY raw-mode bookkeeping from two file-static globals (current_tty_mode, orig_tty_termios) to caller-owned per-handle state, mirroring libuv's uv_tty_t fields. The change spans 10 files across four layers: the C++ mode engine (wtf-bindings.cpp, new BunTTYState.h), JSC bindings (ProcessBindingTTYWrap.cpp), the built-in node:tty module, Rust FFI (bun_core/tty.rs), and four Rust runtime consumers (Terminal.rs, repl.rs, ansi_renderer.rs). Two PTY-driven regression tests are added.
Security risks
None identified. The new state argument to jsTTYSetMode is validated (non-null JSUint8Array, not detached, length() >= Bun__ttyStateSize()) immediately before typedVector() is dereferenced, and after both toInt32 coercions so a re-entrant detach is caught. The only caller is the builtin, which passes a plain integer fd, a boolean, and its own lazily-allocated Uint8Array. The memcpy bounds are sizeof(BunTTYState), matched against the same Bun__ttyStateSize() used to size the JS buffer.
Level of scrutiny
High. This is an FFI ABI change: Bun__ttySetMode gains a void* state parameter, and the Rust #[repr(C)] State { c_int, libc::termios } must match C++ BunTTYState { int mode; struct termios orig_termios; } field-for-field across linux/macOS × x64/aarch64. The libc crate's termios mirrors the system header, and the C++ side includes <termios.h> directly, so the layouts should agree — but cross-language struct layout assumptions on a syscall-adjacent path are exactly the kind of thing a maintainer should sign off on. TTY raw mode is also a load-bearing primitive for every TUI/REPL/prompt library.
Other factors
- Two prior review comments (my stale-comment nit, CodeRabbit's coercion-ordering concern) were both addressed in follow-up commits and the threads are resolved.
- The PR went through a design iteration mid-review (dropping the
RefCell<Box<[u8]>>+ runtime-sizing approach for a plain#[repr(C)]struct), which is now settled. - Tests are thorough: both drive a real PTY via
Bun.Terminaland assert on the kernel's actual termios (localFlags & (ICANON|ECHO)) rather than Bun's own bookkeeping, and thetty.test.tshandshake coverstty.ReadStream, a second stream's no-op, a raw→cooked round-trip, and thetty_wrapbinding path. rust:check-allreported clean across all targets; CI red on this branch was root-caused to unrelated flake/infra and a pre-existing darwin bug fixed on main after the branch cut.
Given the FFI/ABI surface and the number of consumers touched, this merits a human approval rather than a bot shadow-approve.
* upstream/main: (57 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (70 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (52 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ...
* upstream/main: (52 commits) node:http/https/http2: raise Node v26.3.0 compat to ~94%, sync the upstream suites, and fix the Windows/macOS transport-layer teardown bugs they exposed (oven-sh#32488) expect: fix panic in toBeArrayOfSize/toHaveBeenCalledTimes with length > i32 max (oven-sh#32266) lexer: fix TOKEN_TO_STRING[TColon] showing " =" instead of ":" (oven-sh#34253) Bun.Terminal: write() returns bytes accepted, fire drain on POSIX (oven-sh#34289) test(serve-body-leak): give release-asan the same 60s per-test timeout as debug (oven-sh#34297) worker: mark the context terminating before the final concurrent-queue drain (oven-sh#34278) buffer: wrap negative ucs2 indexOf offset against raw byte length for Buffer needles (oven-sh#34273) fs.promises.watch: yield events with a null prototype (oven-sh#34279) child_process: latch stdin write EPIPE as 'error' + destroy, fail later writes with ERR_STREAM_DESTROYED (oven-sh#34268) Fix asString assertion when passing String objects as signals (oven-sh#34265) Buffer: carry size_t through toString/write so length 2^32 doesn't wrap to 0 (oven-sh#34274) test: use tempDir in log-test.test.ts instead of hardcoded /tmp path (oven-sh#34294) tty: track raw mode per handle instead of per process (oven-sh#33527) test: expect the bumped mimalloc SHA in process.versions Return freed memory to the OS on a background thread instead of the JS thread (oven-sh#34181) Move WTFTimer out of the shared timer heap to fix a cross-thread race (oven-sh#33131) test: update block-scoped enum lowering expectations to let (oven-sh#34287) Error.captureStackTrace: install .stack as non-enumerable (oven-sh#34259) js_parser: treat "async as T" / "async satisfies T" as a cast, not an arrow (oven-sh#34246) js_parser: accept `!`, `#name`, and `export @dec` in standard decorator grammar (oven-sh#34245) ... # Conflicts: # test/js/bun/websocket/websocket-server.test.ts
2 participants
setRawModestate is process-global in Bun, not per-stream. CallingsetRawMode(false)on a second, never-rawtty.ReadStream(0)silently restores the shared terminal to cooked out from under a rawprocess.stdin, whileprocess.stdin.isRawkeeps reportingtrue. There is no event, no error, and no JS-visible signal: the app just stops receiving keystrokes and the kernel starts echoing them.Prompt, TUI and REPL libraries build their own
tty.ReadStream(0)and callsetRawMode(false)on a cleanup path, so this is an order-dependent, whole-process failure of the most basic TUI primitive.Repro
The converse hole falls out of the same state: once any stream is raw,
setRawMode(true)on a different tty fd is a silent, success-returning no-op. TwoBun.Terminals hit exactly that: the second one'ssetRawMode(true)never touches its own PTY.Cause
Bun__ttySetMode(fd, mode)is a port of libuv'suv_tty_set_mode(), but the two fields libuv keeps on eachuv_tty_twere lifted to file statics:So the
current_tty_mode == modeearly-return and the "restore the saved termios" path are shared by every stream, every fd, and everyBun.Terminal. In the repro, the second stream'ssetRawMode(false)seescurrent_tty_mode == 1, so it takes the restore branch and runs a realtcsetattrwith the cooked snapshot.Fix
Put the mode and the saved termios back on the handle, as in libuv:
Bun__ttySetMode(fd, mode, state)reads and writes a caller-ownedBunTTYState { mode, orig_termios }.node:ttykeeps one state buffer perReadStream, allocated lazily on the firstsetRawMode.tty_wrap'sTTYkeeps one perTTYWrapObject.Bun.Terminalkeeps one per PTY, and the Rust callers (REPL, interactive prompts, the kitty-graphics probe) one per owner.The
uv_tty_reset_mode()snapshot (orig_termios_fd/orig_termios, used by the atexit restore) stays process-wide, exactly as it is in libuv.This also makes the second-raw-stream case match Node: a stream that goes raw while the device is already raw captures the raw termios as its own snapshot, so its later
setRawMode(false)is a no-op rather than cooking the terminal under the stream that owns it.Verification
The reporter's self-checking repro now matches Node byte for byte:
Two tests, both driven over a real PTY via
Bun.Terminaland asserting onterminal.localFlags(ICANON/ECHO), which is the terminal's actual termios rather than Bun's bookkeeping:test/js/node/tty.test.tswalks a child through a handshake:process.stdingoes raw, then a secondtty.ReadStream(0)doessetRawMode(false), a raw/cooked round trip, and atty_wrapTTY(0).setRawMode(0). The device must stay raw through all of them, and onlyprocess.stdin.setRawMode(false)may cook it.test/js/bun/terminal/terminal.test.tsasserts twoBun.Terminals keep independent modes.On
mainthe first reportsafterSecondStreamCooked: false,afterSecondStreamRoundTrip: false,afterTTYWrapCooked: false; the second reports the second terminal never went raw.Suites run locally (
bun bd test, linux-x64)bun run rust:check-allis clean across linux/macos/windows x x64/aarch64.test/js/node/readlinehas 2 pre-existing failures (readline should unref,stdin pause should stop reading so child can read from stdin) that reproduce identically onmainwithout this diff.no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/bun/terminal/terminal.test.ts test/js/node/tty.test.ts