Conversation
|
Updated 11:06 PM PT - Jul 15th, 2026
❌ @robobun, your commit 00e7a19 has 5 failures in
🧪 To try this PR locally: bunx bun-pr 33580That installs a local version of the PR into your bun-33580 --bun |
WalkthroughThe change updates tty raw-mode failure handling to return signed libuv error codes, emit structured Changestty setRawMode error handling
Possibly related PRs
🚥 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: 3
🤖 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/js/node/tty.ts`:
- Around line 88-93: Document the errno sign difference in tty setup error
handling so the `handle.setRawMode` branch is not “harmonized” with the sibling
branches. Add a brief clarifying comment near the `setRawModeError(err)` call in
`TTY`’s raw-mode path explaining that this branch intentionally passes `err`
as-is, unlike the other branches that negate their error codes, so future
refactors do not change `setRawModeError` or the `emit("error", ...)` behavior
incorrectly.
In `@test/js/node/tty.test.ts`:
- Around line 87-91: Trim the inline comment in the tty test so it stays within
the repo’s 3-line limit and keeps only durable, non-obvious behavior notes.
Update the comment near the setRawMode/ErrnoException test in tty.test.ts to
remove the bug-history narration (“Bun used to emit...”) and any PR/issue-style
context, while preserving the essential explanation about EIO and the tcsetattr
failure path.
- Around line 115-123: The polling callback in the test uses a dead try/catch
around process.stdin.setRawMode in the interval loop, but the tty implementation
only emits error events and does not throw synchronously. Update the test around
the setInterval block to stop relying on catching setRawMode failures, and let
the existing process.stdin.on("error", ...) path handle the failure instead; if
the interval cleanup is still needed, tie it to the error listener rather than
the catch block.
🪄 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: c71acfc0-4b16-4fc8-a0b8-9739e24820d8
📒 Files selected for processing (2)
src/js/node/tty.tstest/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.
Thanks — both issues I flagged look correctly addressed in ef39eab. Deferring to a human for the final sign-off since the Windows-only Source__setRawModeStdin return-contract change and the open_tty failure arm remain untested (the new test is POSIX-only).
Extended reasoning...
Overview
This PR routes tty.ReadStream#setRawMode failures through ErrnoException (from internal/shared) so the emitted error carries code/errno/syscall, matching Node. It touches three files: src/js/node/tty.ts (adds a lazy setRawModeError helper and rewires two of three failure branches), src/io/source.rs (Windows-only: changes Source__setRawModeStdin's return contract from a positive POSIX-discriminant errno to the raw signed libuv rc via .int(), and negates the open_tty failure arm), and test/js/node/tty.test.ts (new POSIX-only pty test).
On the previous revision I flagged (1) a regression on the Windows non-stdin branch where a native SystemError object was being passed into ErrnoException, which would throw ERR_INVALID_ARG_TYPE, and (2) an errno-space mismatch on the Windows stdin branch. Both were addressed in ef39eab: the non-stdin branch is reverted to emit the native error as-is, and source.rs now returns the raw libuv rc so the stdin branch resolves code correctly.
Security risks
None. This is error-object shaping on a failure path; no auth, crypto, untrusted-input parsing, or resource-limit logic is touched.
Level of scrutiny
Moderate. The JS-side change is small and the POSIX branch is covered by the new test (verified to fail on system Bun per the PR description). However, the change also modifies an FFI return-value contract in Windows-only #[cfg]-gated Rust (Source__setRawModeStdin), and neither Windows branch has automated coverage — the new test is skipIf(isWindows). The errno sign/space handling across the three branches is subtle enough that the first revision contained a real regression, so a human glance at the Windows paths seems warranted.
Other factors
The author acknowledged that the open_tty failure arm's -(e.errno as c_int) still won't resolve to a proper Windows UV_E* name (it's a negated POSIX discriminant, not a -4xxx libuv code), but it's a strict improvement over the prior positive value and that arm is very rarely hit. All prior review threads (mine and CodeRabbit's) are resolved; the bug-hunting pass on the current revision found nothing.
There was a problem hiding this comment.
Both of my earlier concerns were addressed correctly in ef39eab and I found nothing new — the POSIX path is tested and the Windows Source__setRawModeStdin change traces correctly (.int() returns the raw signed libuv rc). Deferring only because the Windows native change has no CI coverage (test is skipIf(isWindows)) and the open_tty error arm's errno mapping is acknowledged-imperfect, so a quick human sanity check on that branch seems worthwhile.
Extended reasoning...
Overview
This PR changes tty.ReadStream#setRawMode() to emit a Node-compatible ErrnoException (with code/errno/syscall) instead of a bare Error when the underlying tcsetattr/uv_tty_set_mode fails. It touches three files: src/js/node/tty.ts (adds a lazily-loaded setRawModeError helper and rewires the POSIX and Windows-stdin failure branches), src/io/source.rs (Windows-only: Source__setRawModeStdin now returns the raw signed libuv rc via .int() instead of the translated positive E discriminant), and test/js/node/tty.test.ts (a new pty-based test that closes the master out from under the child and asserts the emitted error shape).
Prior review
On the first revision I flagged two issues: (1) the Windows non-stdin branch was passing a native SystemError object into ErrnoException, which would have thrown ERR_INVALID_ARG_TYPE synchronously — a regression; and (2) the Windows stdin branch was negating the already-translated small POSIX discriminant, producing code: "Unknown system error -22" instead of a real libuv name. Both were addressed in ef39eab: the non-stdin branch was reverted to this.emit("error", err) (with a clarifying comment), and Source__setRawModeStdin now returns set_mode(...).int() — I verified ReturnCode::int() returns the raw c_int (0 on success, negative UV_E* on failure), and the JS side passes it through un-negated. The open_tty failure arm returns -(e.errno), which won't resolve to a proper Windows libuv name but is acknowledged as a rare-path limitation and is strictly no worse than before.
Security risks
None. This only changes the shape of an error object emitted on a failure path; no new inputs are parsed, no auth/crypto/permissions surface is touched.
Level of scrutiny
Medium. The JS change is small and confined to an error path in a built-in module, but the native change alters an FFI return-value contract across a platform boundary (Windows), and correctness depends on subtle errno sign conventions that differ across the three branches (POSIX positive errno → negate; Windows stdin raw libuv rc → pass through; Windows non-stdin SystemError object → emit as-is). The new test is skipIf(isWindows), so the Windows changes are verified only by cargo check and code review.
Other factors
All CodeRabbit and my own inline threads are resolved. The bug-hunting system found nothing new on this revision. The POSIX path has a solid end-to-end test that fails on system Bun and passes on the debug build. The remaining uncertainty is the untested Windows stdin path — the reasoning checks out, but a human glance at the source.rs change would be prudent before merge.
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`:
- Line 88: The subprocess-based tests in tty.test.ts are still serialized even
though they spawn independent child processes. Update the affected test cases to
use test.concurrent (including the test.skipIf(isWindows) case and the other
subprocess test in this file) so they can run in parallel, while keeping any
shared process-global state checks isolated if needed.
🪄 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: 771709ab-a7a3-49f8-938b-2e152ec466ca
📒 Files selected for processing (1)
test/js/node/tty.test.ts
There was a problem hiding this comment.
The fixes in ef39eab look right and the POSIX path is now well-covered; deferring only because the Windows Source__setRawModeStdin return-value contract change (and the acknowledged lossy open_tty fallback) has no CI coverage, so a quick human sanity check on that path seems worthwhile.
Extended reasoning...
Overview
Three files: src/js/node/tty.ts swaps the bare new Error("setRawMode failed with errno: N") for a lazily-loaded ErrnoException(uvErr, "setRawMode") on the POSIX and Windows-stdin failure branches (the Windows non-stdin branch is unchanged aside from a clarifying comment). src/io/source.rs (Windows-only) changes Source__setRawModeStdin to return the raw signed libuv rc from uv_tty_set_mode via .int() instead of a translated positive E discriminant, and negates the open_tty failure errno so ErrnoException accepts it. test/js/node/tty.test.ts adds a POSIX-only subprocess test that wraps /dev/null in a tty.ReadStream, calls setRawMode(true), and asserts {code, errno, syscall} on the emitted error (ENOTTY on Linux, ENODEV on Darwin).
Security risks
None. This is error-object shape only on a rare failure path; no auth, crypto, parsing of untrusted input, or resource handling changes.
Level of scrutiny
Medium. The POSIX change is trivial and directly tested. The Windows side changes an FFI return-value contract between Rust and the built-in JS module — errno sign/magnitude semantics differ between POSIX (positive errno, negated in JS) and Windows (native negative libuv codes, passed through). The first revision of this PR had a real regression on the Windows non-stdin branch (wrapping a SystemError object in ErrnoException → synchronous ERR_INVALID_ARG_TYPE), which was caught in review and reverted; that history plus test.skipIf(isWindows) on the new test argues for a human glance at the Windows path rather than auto-approval.
Other factors
All prior review threads (mine and CodeRabbit's) are resolved. I verified set_mode() returns ReturnCode and .int() yields the raw c_int (0 on success, negative UV_E* on failure), and that internal/shared's ErrnoException(err, syscall) routes through util.getSystemErrorName(err) — so the current wiring is consistent. The author explicitly acknowledged the open_tty failure arm returns -(e.errno) (a negated small discriminant, not the true -4xxx libuv magnitude on Windows), which will produce code: "Unknown system error -N" on that rare path; that's a known, stated limitation and not a regression. The bug-hunting system found nothing on the current revision.
|
Rebased onto main at 00e7a19 after #33527 landed (per-stream raw-mode state); both additions coexist cleanly. Build #73611 is complete: 280 lanes passed,
Ready for review. |
… on win32 stdin, trim comments
7e19a57 to
00e7a19
Compare
There was a problem hiding this comment.
LGTM — both earlier review points (Windows non-stdin SystemError passthrough, Windows stdin raw libuv rc) are addressed and CI is green across POSIX lanes.
What was reviewed:
- POSIX path:
Bun__ttySetModereturns positiveerrno(wtf-bindings.cpp:131), negated intoErrnoException— matches Node'serrnoException(err, 'setRawMode'). - Windows stdin:
set_mode().int()returns the rawReturnCodec_int (0 on success, negativeUV_E*on failure), passed through un-negated; theopen_ttyfailure arm's-(e.errno)limitation is acknowledged. - Windows non-stdin: unchanged, still emits the native SystemError as-is.
- Test: the
stderr: ""assertion inside the combinedtoEqualobject was flagged and ruled out (bunEnv + green on all ASAN/debug lanes).
Extended reasoning...
Overview
This PR replaces the bare Error("setRawMode failed with errno: N") emitted by tty.ReadStream#setRawMode on failure with a proper Node-style ErrnoException carrying code/errno/syscall, so callers can branch on err.code === "EIO" etc. Three files touched: src/js/node/tty.ts (~10 lines: lazy ErrnoException helper + two call-site swaps), src/io/source.rs (~10 lines, Windows-only: Source__setRawModeStdin now returns the raw signed libuv rc via .int() instead of the translated POSIX discriminant), and a new ~40-line spawned-subprocess test in test/js/node/tty.test.ts.
Prior review resolution
I raised two issues on the first revision: (1) the Windows non-stdin branch was wrapping an already-populated SystemError object in ErrnoException, which would throw ERR_INVALID_ARG_TYPE; (2) the Windows stdin branch was negating a translated POSIX discriminant, producing "Unknown system error -22" instead of a real code. Both were fixed in ef39eab / 7ab4dbd: the non-stdin branch reverted to emitting err as-is (with a clarifying comment), and Source__setRawModeStdin now returns set_mode(...).int() — the raw ReturnCode c_int, i.e. the signed uv_tty_set_mode return value — passed through without negation. I verified ReturnCode::int() at libuv.rs:2271 and set_mode at libuv.rs:1349. The remaining open_tty failure arm returns -(e.errno) (a negated POSIX discriminant, so the Windows code won't resolve on that rare path); the author explicitly acknowledged this as unrecoverable without a wider refactor, and it is still a strict improvement over the previous positive-errno string.
Security risks
None. This is error-object shaping on an already-failing path; no new inputs, no auth/crypto/permissions surface, no untrusted data parsing.
Level of scrutiny
Low-to-moderate. The happy path is entirely unchanged (all edits are inside if (err) blocks or on the failure return of an extern "C" fn). The POSIX change is a mechanical swap of the error constructor, verified against ttySetMode returning positive errno (wtf-bindings.cpp:131). The Rust change is Windows-only, compiles under cargo check --target x86_64-pc-windows-msvc, and the .int() accessor is a trivial self.0 unwrap. The ErrnoException constructor in internal/shared.ts accepts (err, syscall, original?) and calls util.getSystemErrorName(err), matching both call sites.
Other factors
All review threads (mine and CodeRabbit's) are resolved; the one declined suggestion (converting sibling tests to test.concurrent) was reasonably scoped out. Build #69667 passed 282 lanes with tty.test.ts green on Linux and Darwin (including the ENODEV-vs-ENOTTY split added in 00e7a19). The bug hunter raised the stderr: "" assertion twice and refuted it both times; the assertion is inside a combined toEqual object per CLAUDE.md guidance and is green under debug/ASAN lanes. New test is properly skipIf(isWindows), drains pipes concurrently, and asserts exit code alongside output.
…t as debug (#34297) `test/js/bun/http/serve-body-leak.test.ts` went red on the debian 13 x64-asan lane in [build 73611](https://buildkite.com/bun/bun/builds/73611): the "should not leak memory when streaming the body and echoing it back" case timed out at 40s on all four retries. The PR under test (#33580, tty `setRawMode`) does not touch anything related, so this is the test's own budget. ### Cause This is not a hang and not a code regression. Scraping the timestamped logs from 35 recent x64-asan runs (builds 73562-73623) plus 4 pre-#33193 runs: | case | release (debian 13 x64) | release-asan (debian 13 x64-asan) | |---|---|---| | `callIgnore` | ~5s | 15-24s | | `callStreamingEcho` | ~8s | **27-39s** (median ~31s) | | total file | ~40-47s | ~125-195s | The streaming-echo case has been running at ~31s median on ASAN since well before the webstreams rewrite (pre-#33193 samples: 28.2 / 30.9 / 28.2 / 31.6s), so the 40s budget has always been tight there. On build 73611 every case in the file ran ~35% slower than typical (the shard landed on a slower EC2 instance; `callIgnore` 23.6s vs a typical ~17s), which is enough to push echo past 40s. A local 15000-request `/streaming-echo` probe against a debug build runs at a flat ~395 req/s with no stalls, confirming throughput, not a hang. The file already scales its `end_memory` threshold for ASAN (#32520), and `scripts/runner.node.mjs` already applies a 3x ASAN multiplier to the default `--timeout` for the same reason, but that multiplier does not reach tests that pass their own explicit third-argument timeout. Skipping on ASAN was tried in #28301 and reverted in #28337; this change keeps the test running there with a budget that matches the measured cost. ### Fix ```diff - isDebug ? 60_000 : 40_000, + isDebug || isASAN ? 60_000 : 40_000, ``` Matches the `isDebug || isASAN` convention already used by 16 other test files for timeouts/iteration counts. 60s is ~2x the ASAN median and ~1.4x the extrapolated worst case (73611). Release lanes stay at 40s. ### Verification - `USE_SYSTEM_BUN=1 bun test test/js/bun/http/serve-body-leak.test.ts`: 8 pass, echo 11.4s (release, budget unchanged at 40s). - `bun bd test test/js/bun/http/serve-body-leak.test.ts -t 'ignoring the body'`: passes; file parses and the unchanged isDebug=60s branch applies under the debug build. - `/tmp/echo-probe.ts` against debug build: 15000 `/streaming-echo` requests at a steady ~395 req/s, no stalls. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test-only change; deferring to CI. <!-- robobun:evidence:end -->
|
Closing: folded into #41495. There The test there ("setRawMode failure emits an ErrnoException" in |
1 participant
What
tty.ReadStream#setRawMode()failures were surfaced as a bareError("setRawMode failed with errno: N")with nocode,errno, orsyscall. Node emits a proper errno exception, so libraries that branch onerr.code === "EIO"(orerr.syscall) to shut down cleanly when the terminal goes away fell through to a generic crash path on Bun.This is the "the terminal went away" path: ssh drop, terminal-emulator crash, pty master closed while a readline/TUI holds the tty in raw mode. Both runtimes deliver the failure the same way (an
errorevent on the tty ReadStream); only Bun's error object was unclassifiable.Cause
Prototype.setRawModeinsrc/js/node/tty.tsbuilt a plainErrorfrom the raw errno on the POSIX and Windows-stdin failing branches. Node'slib/tty.jsemitserrnoException(err, 'setRawMode')instead.Fix
Bun__ttySetModereturns the raw positive errno; negate it and route through the sharedErrnoException(internal/shared), which populatescode/errno/syscalland formats the message the way Node does.fd === 0:Source__setRawModeStdinnow returns the raw signed libuv return code fromuv_tty_set_mode(previously it returned a translated positive discriminant), so the sameErrnoExceptionpath resolves the correctcodeon Windows too.handle.setRawModealready returns a native SystemError withcode/errno/syscallpopulated, so it is emitted as-is (now with a clarifying comment).ErrnoExceptionis required lazily on the error path so the commonrequire("node:tty")stays light.Verification
New test in
test/js/node/tty.test.tsspawns a child that callssetRawMode(true)on atty.ReadStreamwrapping/dev/null(sotcgetattrfails withENOTTYon every POSIX) and asserts the emitted error is{ code: "ENOTTY", errno: -25, syscall: "setRawMode" }. The original pty-master-close reproduction was verified manually on Linux, but that trigger is Linux-specific (BSD pty semantics differ), so the automated test uses the deterministic cross-POSIX ENOTTY path instead.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/node/tty.test.ts