terminal: release the wrapper after PTY EOF leaves input unflushed

Merged
RO
2mo ago
+156-99

Problem

  • Since #36962 (v1.4.0), a POSIX Bun.spawn({ terminal }) whose child exits with terminal.write() input still queued never releases the Terminal or its callbacks. It keeps three pty fds until process exit, spins on POLLHUP (2.6 s CPU per 2 s idle), and queues every later write in memory.
  • At reader EOF every slave fd is gone, so the queue can never drain: a pty master answers EAGAIN, not EPIPE. writer_has_buffered stayed set and maybe_downgrade_after_eof (Terminal.rs:1826 on main) never downgraded. Late echo after the EIO also re-upgraded this_value in on_read_chunk.

Fix

  • On POSIX on_reader_finished ends the writer and closes the reader: WRITER_DONE is set before the downgrade check, exit is the last callback, and a later write() returns without queuing.
  • The CONNECTED flag and its upgrade are deleted: this_value is strong until EOF, so the upgrade only fired on that late echo. The downgrade now waits for a new EXIT_DISPATCHED flag, set after the exit callback returns, instead of READER_DONE, so the wrapper stays strong through its last dispatch.
  • Behaviour change: no drain or data after exit. The #36962 test that expected a late drain is replaced: that drain came only from the kernel discarding an unterminated line.
  • Verified: the new terminal.test.ts block (collection, fd count, late callbacks) fails on 1.4.3 and passes 10 of 10. Also all of test/js/bun/terminal/ and spawn.test.ts.

Background

  • Terminal holds its wrapper in a JsRef, strong while a callback can fire, because the wrapper's cached slots are the callbacks' only GC root.
  • A BufferedReader and a PosixStreamingWriter drive the pty master. The writer queues what write(2) refuses and waits for POLLOUT.
  • At child exit bun closes its slave fd, the master reads EIO (Linux) or EOF (macOS), and exit fires.
Notes

Reproduction on released bun (1.4.3-canary.1, Linux x64, kernel 7.0):

let proc = Bun.spawn(["sh", "-c", "exit 0"], { terminal: { data() {}, exit() {}, drain() {} } });
const big = Buffer.alloc(65536, "a\n");
for (let i = 0; i < 32; i++) proc.terminal.write(big);   // ~2 MB the child never reads
await proc.exited;
  • Leak: with a FinalizationRegistry on proc.terminal and a Bun.gc(true) loop, 0 of 5 wrappers are collected. Without the write() calls, 5 of 5.
  • Fds: /dev/fd grows by 3 per terminal (master plus the reader and writer dups) and never shrinks. With the fix it returns to the baseline once the wrappers are collected.
  • Spin: process.cpuUsage() over 2 s of idle after proc.exited reports 2603 ms of CPU. Without the write, 3 ms. With the fix, 39 ms.
  • Post-exit writes: 64 MB of terminal.write() after proc.exited grows RSS by 178 MB on 1.4.3 (256 MB grows it by 491 MB). With the fix, 3 to 4 MB.
  • Size: there is no clean threshold. The wrapper leaks whenever part of the input is still queued in the writer at EOF (8 KB of lines was enough here, 12 KB happened to fit) and, at any size, whenever echo arrives after the EIO (seen with 1 KB). With an unterminated line the kernel keeps discarding input, so an unfixed build drains after a few hundred ms, fires a late drain, and then releases the wrapper; with complete lines the queue is stuck for good.

Why the reader is closed too: on Linux the master read fails with EIO as soon as the last slave fd closes, but the slave's line discipline keeps processing input that was already queued and echoes it back, so later reads return data again. The reader's poll stayed armed after the error, delivered those chunks to data after exit, and the old CONNECTED branch re-rooted the wrapper on the first one. EOF (the macOS path) already closes the reader inside BufferedReader; the error path now matches it.

Why the writer is ended in Terminal and not in PosixPipeWriter::on_poll: a first version made on_poll report EndOfFile when a short write met POLLHUP, for every posix writer. With the terminal change in place the pty case no longer reaches that branch (measured: the spin is gone with the terminal change alone), and pipes and sockets fail with EPIPE or ECONNRESET there instead, so the generic change had no reachable case and was dropped.

write() after PTY EOF returns the byte length and drops the bytes. Before this change the same call also returned the byte length but queued the bytes in the writer forever (the RSS numbers above), so the return value is unchanged and the memory growth is gone. Throwing was rejected because terminal.closed is still false in that state.

Windows is unchanged: its writer already reports close when the conhost input pipe breaks, and the test is skipped there because it spawns sh.

test/js/bun/spawn/spawn-pipe-leak.test.ts fails the same way in my container on main and on this branch (three 500-process leak tests time out at 30 s on a debug ASAN build), so it is not a regression from this diff.

Self-reviewed: the review asked for the fd assertion, the post-exit write coverage, the regression origin and the measured consequences in this body, and deletion of the dead CONNECTED state instead of a guard. All done. An RSS assertion was not added: the post-exit writes in the test already fail the collection and fd checks on a build without the write() guard (verified by removing it), and RSS bounds are noisy under ASAN.

cargo check -p bun_runtime is clean for x86_64-pc-windows-msvc and aarch64-apple-darwin.


no test proof · iteration 1 · 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