Skip to content

Poll a tty fd in Bun.file(fd).stream() the way stdin is polled - #41504

Open
robobun wants to merge 4 commits into
mainfrom
robobun/a6032ab2/tty-readstream-pollable
Open

robobun wants to merge 4 commits into
mainfrom
robobun/a6032ab2/tty-readstream-pollable

Conversation

@robobun

@robobun robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • Bun.file(fd).stream() on a tty fd other than stdin never delivers input. fs.openSync("/dev/tty") then stream().getReader().read() hangs forever under a pty, and with no pending read the process exits before any input.
  • Lazy::open_file_blob (src/runtime/webcore/FileReader.rs:135) only applied the tty path to fd 0, 1 and 2. Any other fd was treated as a regular file: a dup with synchronous reads and no poll.

Fix

  • The tty path now applies to any fd that isatty(): the reader reopens the terminal by ttyname with O_NONBLOCK (open_as_nonblocking_tty) and polls that private description. The caller's fd keeps its flags and stays open.
  • When the reopen is denied, the reader polls a dup it owns and reads only when poll(2) reports data, the same fallback stdin already has. A pty master is never reopened (the existing TIOCGPTN guard) and gets the polled dup, which also reads an O_NONBLOCK master.
  • Verified: test/js/bun/util/bun-file-fd-read.test.ts (one new test under Bun.Terminal, hangs on 1.4.3). Also tty.test.ts, process-stdin, streams.test.js, bun-file*, node-stream, child_process, spawn.

Background

  • FileReader is the native source behind Bun.file().stream(). A pollable fd registers a FilePoll and reads on readiness, which holds the event loop. A non-pollable fd is read synchronously with pread, which a tty does not support.
  • libuv's uv_tty_init reopens a tty through /dev/pts/N so that O_NONBLOCK never leaks onto a shared description. Bun ports that as open_as_nonblocking_tty in c-bindings.cpp and used it for stdio only.
Notes

This came out of a fuzz ledger item: tty.ReadStream on a /dev/tty fd cannot be destroyed until the user presses Enter, and that line is lost. Routing tty.ReadStream through this pollable reader fixes that, but #41495 replaces tty.ReadStream with a net.Socket over a native TTY handle that reopens the terminal the same way, and #41421 edits the same constructor for an O_NONBLOCK fd. So this PR carries only the FileReader change, which stands on its own.

macOS: ttyname_r of an fd opened from /dev/tty (the controlling-terminal alias) returns /dev/tty again, and kqueue rejects that device with EINVAL. That is a kqueue limit (libuv falls back to a select thread for it) and it applies to stdin the same way. A pty slave opened by its own path works, so the test opens /dev/fd/0 on macOS and /dev/tty on Linux.

Probed on Linux: a blocking /dev/tty fd, a pty master with and without O_NONBLOCK, and stdin all deliver data and cancel() releases the poll. Pre-existing debug-only timing failures, identical on main: stdin-fixtures.test.ts (1 s auto-kill) and child_process.test.ts "extra stdio pipes are not double-closed on GC".


no test proof · iteration 2 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/bun-file-fd-read.test.ts

Bun.file(fd).stream() treated a tty fd other than stdin as a regular
file: a dup with synchronous reads that returned nothing. The stream
never delivered input and nothing held the event loop.

FileReader now applies the stdin tty path to any fd that isatty(): it
reopens the terminal by ttyname with O_NONBLOCK and polls that private
description, so the caller's fd keeps its flags. When the reopen is
denied, the reader polls a dup it owns and reads only when poll(2)
reports data. A pty master is never reopened (TIOCGPTN guard); it gets
the polled dup.
@coderabbitai

coderabbitai Bot commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file.

Or wait 10 seconds for your next included review.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 4b8e0b68-c2a2-45a7-a7cc-e143e9cb10ed

📥 Commits

Reviewing files that changed from the base of the PR and between 4593030 and 12acaf4.

📒 Files selected for processing (2)
  • src/runtime/webcore/FileReader.rs
  • test/js/bun/util/bun-file-fd-read.test.ts

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the claude label Sep 6, 2026
@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author

Reproduced under a pty: a child spawned with Bun.Terminal opens a tty fd other than stdin, calls Bun.file(fd).stream().getReader().read(), and the parent types a line. On 1.4.3 the read never resolves. With this change the chunk arrives, cancel() with a second read pending resolves it as done and the process exits. Test: test/js/bun/util/bun-file-fd-read.test.ts, green on every lane.

CI status: the diff is green. The red lanes are unrelated and reported separately: test/js/node/test/parallel/test-crypto-dh-leak.js on x64-asan (an RSS threshold, fails on main) and test/regression/issue/10887.test.ts (a transpiler test that stalled in a parallel batch on alpine x64).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I checked fd ownership on the new non-stdio reopen path in FileReader.rs — the reopened fd is owned by the reader the same as the old dup path and is closed on the fstat error exit (line 223), so no new leak. Also traced the get_or_insert_with side-effect: caching is_atty = Some(false) for non-tty fds is benign since every downstream read is unwrap_or(false). The #[cfg(not(unix))] arm binds reopened_tty = None and falls through to the pre-existing stdio/dup branches unchanged.

Extended reasoning...

The native change extends an existing tty-reopen mechanism from stdio-only to any isatty() fd, with the fallback structure preserved (stdio → shared fd, non-stdio → owned dup). I traced ownership of the newly-introduced reopened fd through the error paths in open_file_blob and confirmed it is closed on fstat failure the same way the dup was; the is_atty cache mutation via get_or_insert_with changes None → Some(false) for non-tty fds, but every consumer in the function uses unwrap_or(false) so the observable behavior is identical. The Windows arm is a static None that routes to unchanged code. The inline finding covers the test-side resource cleanup; nothing further to add on the Rust side.

Comment thread test/js/bun/util/bun-file-fd-read.test.ts Outdated
@robobun

robobun commented Sep 6, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 2:34 AM PT - Sep 6th, 2026

❌ @robobun, your commit 12acaf4 has 4 failures in Build #110872 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 41504

That installs a local version of the PR into your bun-41504 executable, so you can run:

bun-41504 --bun

Comment thread src/runtime/webcore/FileReader.rs Outdated
Comment thread src/runtime/webcore/FileReader.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

sergeiwallace added a commit to sergeiwallace/ai-cli-utils that referenced this pull request Sep 15, 2026
…stdin (#147)

commit 6f0127b redirected a backgrounded agent's stdin from the literal
/dev/tty alias. On macOS, kqueue refuses to poll an fd opened through that
alias path (ttyname_r resolves it to /dev/tty again rather than a real
device) -- Bun-compiled claude then crashes with EINVAL on launch. Resolve
the controlling terminal's actual device path via `tty` first and redirect
from that instead, falling back to the literal /dev/tty when resolution
fails. Root cause corroborated externally: oven-sh/bun#41504.


Claude-Session: https://claude.ai/code/session_01PFi83LbqMTznL3DW2sVm9d

Co-authored-by: T <t@t.com>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants