Repository navigation
Conversation
…closes the terminal `Terminal::write` tested `CLOSED` only before it converted `data`. A String object is converted with ToString, which runs user code. When that code closed the terminal, the call returned the byte length and wrote nothing. Read the flags once after the conversion. A closed terminal throws. A terminal whose write side ended at PTY EOF still returns the byte length.
|
Updated 10:19 PM PT - Oct 26th, 2026
✅ @robobun, your commit afecccfd534c7e1e5036fef8639c05e4be04fd43 passed in 🧪 To try this PR locally: bunx bun-pr 47032That installs a local version of the PR into your bun-47032 --bun |
|
Status: the fix is ready for review. How to reproduce (bun 1.4.3-canary and const t = new Bun.Terminal({ cols: 80, rows: 24 });
const s = new String("hello");
s.toString = () => {
t.close();
return "hello";
};
console.log(t.write(s)); // 5
console.log(t.closed); // true
t.write("x"); // throws "Terminal is closed"On |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 4 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Write conversion and terminal-state handling src/runtime/api/bun/Terminal.rs, test/js/bun/terminal/terminal.test.ts |
Terminal.write throws “Terminal is closed” if conversion closes the terminal. If the writer is done but the terminal remains open, it returns the input length without queuing bytes. Tests cover coercion hooks, already-closed terminals, open-terminal writes, and PTY EOF. |
Possibly related PRs
- oven-sh/bun#45294: Closes graph-owned
Bun.Terminalinstances through the terminal close state that this change checks after input conversion.
Suggested reviewers: jarred-sumner, dylan-conway
Priority:
Merge Risk: 
e1bd5
-
Writes reject terminals closed during conversion while preserving EOF returns. No merge-blocking risk was identified.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly identifies the main change: terminal.write() now throws when converting data closes the terminal. Description check The description explains the problem, fix, scope, testing, measurements, platform coverage, and limitations. It does not use the template headings exactly, but it provides the required change summary … Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the problem, fix, scope, testing, measurements, platform coverage, and limitations. It does not use the template headings exactly, but it provides the required change summary and verification details.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 also checked that bytes borrowed from StringOrBuffer stays valid after a closing hook (close_internal tears down the writer and fds but never detaches or frees the JS argument, which is still rooted on the call stack), and that the CLOSED check wins when close() sets both CLOSED and WRITER_DONE in the same hook. The sibling coerce-after-liveness-check in set_termios_flag (Terminal.rs:1321-1324) is pre-existing, already a documented no-op on a closed terminal, and explicitly left out of this PR.
Extended reasoning...
The change re-reads the Terminal flags after the user-reachable ToString coercion in Terminal::write (src/runtime/api/bun/Terminal.rs) so a hook that closes the terminal makes write() throw "Terminal is closed" instead of returning a byte count, and moves the empty-input early return below that check; it adds a test.each matrix and four tests in test/js/bun/terminal/terminal.test.ts. It touches no security-sensitive surface. The native change is ten lines and behavior-preserving on every path except the closing-hook one, but it is a user-facing semantic change in native runtime code and one test-hygiene finding was posted inline, so a human look is still worthwhile.
The closing-hook tests closed the terminal only inside the hook. If write() threw before it ran the hook, the terminal stayed open until the garbage collector ran. `await using` now closes it in every case.
There was a problem hiding this comment.
LGTM — the follow-up commit registers await using before the assertions in the new tests, and the re-check in write() is correct.
What was reviewed:
Terminal::writere-readsflagsafterStringOrBuffer::from_jsand throws onCLOSED;close_internalsetsCLOSEDbefore anything else, so atoStringhook that callsclose()/[Symbol.asyncDispose]()is caught on every platform.- Moving the
input_len == 0short-circuit below the flag check preserves the old outcomes for open (Ok(0)) and PTY-EOF (Ok(input_len)== 0) terminals; only the closed case changes, to a throw. - Checked the sibling termios setter for the same close-after-check order:
close_internalresetsmaster_fdtoINVALIDso the post-coercion re-read falls into the existing no-op path, not a stale fd.
Extended reasoning...
The change is a 12-line re-entrancy fix in src/runtime/api/bun/Terminal.rs plus a self-contained test block in test/js/bun/terminal/terminal.test.ts; it touches no auth, crypto, or input-parsing surface, and the Rust edit holds no raw pointers across the user-code boundary. The only behavior change is that write() now throws "Terminal is closed" when converting the argument closes the terminal, matching the existing contract for a closed terminal. The one inline concern from the prior run (PTY leaked when assertions fail before the hook closes it) was addressed by commit afecccf, which adds await using before the assertions, and no third-party objections are outstanding. No CODEOWNERS entry covers the changed files.
Problem
Bun.Terminal#write(data)returns the byte length ofdatawhen convertingdatacloses the terminal. It writes nothing. A closed terminal throwsTerminal is closed.Terminal::write(src/runtime/api/bun/Terminal.rs:1392) testsCLOSEDonly before the conversion, which runstoStringof a String object. After it,:1418tests onlyWRITER_DONE.toStringcloses the terminal. Nothing crashes or leaks, so it is safe to decline.Fix
write()keeps its entry test and reads the flags again after the conversion.CLOSEDthrows.WRITER_DONEalone still returns the byte length.test/js/bun/terminal/terminal.test.ts(8 new tests, 5 fail on main), plusterminal-spawn.test.tsandterminal-platform-gaps.test.ts. Linux x64 only.Background
close(),[Symbol.asyncDispose]()and aBun.ModuleGraphdispose setCLOSED. The writer setsWRITER_DONEwhen it ends: at close, or at PTY EOF with the terminal open (terminal: release the wrapper after PTY EOF leaves input unflushed #42150).wsrewritten on native transports (consolidates the open WebSocket PRs) #45223 gaveServerWebSocket.sendthe same order. There the missing second test also caused a use-after-free. Here nothing is freed.dataargument (b.bytes): it changes the class and message of every argument error. Considered one test after the conversion: a closed terminal then runs the hook.Downsides
write()whose argument closes the terminal now throws where it returned a number.write()runs 2 more instructions (6 for empty data) on 351 to 1329. Allocations and syscalls do not change. The function grows by 38 bytes.Notes
Reproduction (bun 1.4.3-canary and
mainat 449877f, Linux x64, deterministic):The same happens with a String subclass, with
Symbol.toPrimitiveorvalueOfas the hook, and with[Symbol.asyncDispose]()as the closer. A hook that closes and returns""makeswrite()return 0.Outcome matrix. 23 argument kinds, each on an open and on an already closed terminal, on debug and release builds, without and with this change.
write()returned 5, 0, 4 or 5000 and now throws.Terminal is closedwith 0 hook calls on both builds.Outside the matrix: after PTY EOF (
closedisfalse)write("x")returns 1 andwrite("")returns 0 on both builds.Measurements. Release builds (ThinLTO) of
mainat 449877f, without and with this change, Linux x64.perf,valgrindandstraceare not installed in this environment, so the counts come from gdb.main) (objdump).write():''351 to 357,'x'579 to 581,Uint8Array(1)518 to 520, 16 non-ASCII characters 1329 to 1331, String object 1097 to 1099,'x'after PTY EOF 390 to 392 (gdbstepi, JIT off, call 41 of each kind, 3 runs each, all equal).nm -S,size).mi_malloccalls in 1000write()calls:'x'1 to 1, 16 non-ASCII characters 1001 to 1001, 64 KBUint8Array1 to 1. A closing hook: 2057 to 5094, the thrown error. A write to a closed terminal: 3035 on both builds.mi_realloccalls: 7 to 7, 1008 to 1007 and 11 to 11 (gdb breakpoint counts, main thread, inside the call).write2 to 2,epoll_ctl1002 to 1002. A closing-hook call issues 0write(2)of its payload on both builds (gdbcatch syscall).CLOSEDtest after the conversion, as inServerWebSocket::send_message, was also measured: 3 more instructions on every path and 146 bytes.Sites with the same order that this PR does not change
Terminal.rs:1321):valueOfcan close the terminal after the closed test. The result equals the documented no-op on a closed terminal. terminal: throw when a termios flag assignment fails and clear its mordant baseline entry #39162 and terminal: throw when a termios flag is set to a value that is not a uint32 #42589 rewrite that function.Bun.WebView: webview: re-check that the view is open after argument conversion, in the shared pre-dispatch check #42160.Bun.listen().reload()(src/runtime/socket/Listener.rs:187): an option getter that stops the listener makesreload()return, where a stopped listener throwsListener is closed.resize()(it accepts only numbers),setRawMode()(ToBoolean runs no user code) andBun.spawn({ terminal })(no user code runs between the closed test and the spawn).#46789 rewrites the type of the
dataargument and keeps this order on purpose. Its schema comment says: "toStringof aStringobject runs once the terminal is known to be open". At its head 68867e8 the lines after the conversion are unchanged, so the symptom is still there. This commit applies to that head without a conflict (git merge-tree --merge-base).Platforms. Built and tested on Linux x64 only. The edited lines have no
cfg, andclose()setsCLOSEDbefore any platform code. The PTY EOF test does not run on Windows, where the child's exit leaves the writer open.Docs and types. No change.
bun.d.tsdoes not describe the throw on a closed terminal.no test proof · iteration 0 · 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