terminal: keep the wrapper alive while a drain dispatch is pending after PTY EOF

Merged
RO
3mo ago
+107-17

Problem

Bun.spawn({ terminal }) can call a garbage-collected function as the drain callback, or silently drop the dispatch, when the PTY child exits while input the child never read is still buffered in the terminal's writer.

When the PTY master hits EOF, Terminal::on_reader_finished downgrades this_value to a weak ref and fires exit. The weak variant of JsRef is a raw JSValue with no liveness tracking, and the wrapper's cached callback slots are the only GC root of the data/exit/drain functions. Buffered input keeps flushing for another few hundred milliseconds after EOF (the kernel paces the discard), and when it finishes, on_writer_ready does try_get() on the weak ref and dispatches drain through whatever that address now holds:

  • if the wrapper was collected and its finalizer ran, the dispatch is silently skipped (the drain callback is lost);
  • if the cell was collected but not yet swept, the slot read returns a dead value and an unrelated, later-allocated function can be invoked as the drain callback (heap type confusion on release builds);
  • under a debug build the same window trips member call on null 'JSC::ClassInfo' in Bun__JSValue__call via Terminal::on_writer_ready <- PosixStreamingWriter::on_poll.

Repro (roughly half of fresh processes lose the drain on an unfixed build):

let proc = Bun.spawn(["sh", "-c", "exit 0"], {
  terminal: { data() {}, exit() {}, drain() { console.log("drain"); } },
});
const big = Buffer.alloc(65536, 97);
for (let i = 0; i < 16; i++) proc.terminal.write(big); // ~1MB the child never reads
const exited = proc.exited;
proc = null;
await exited;                            // EOF: ref downgraded, drain still pending
for (let i = 0; i < 4; i++) {            // GC in the window collects the wrapper
  await new Promise(r => setTimeout(r));
  Bun.gc(true);
}

Fix

Keep this_value strong until the terminal is actually idle:

  • on_reader_finished only downgrades when the writer has no buffered data (maybe_downgrade_after_eof).
  • on_writer_ready downgrades after dispatching drain (unless the callback buffered more data), and on_writer_close downgrades when buffered data is dropped on close/error, so the wrapper never outlives its last possible callback.
  • write() re-upgrades when it buffers data after EOF (e.g. a write from the exit callback), restoring the invariant that a pending drain implies a strong wrapper.
  • The Windows writer now clears writer_has_buffered when it reports Drained; previously the flag was never cleared on Windows, which under the new rule would have kept a post-EOF wrapper strong forever.

The downgrade decision reads only Cell fields, so it is safe from the writer-parent callbacks that run while a writer borrow is live up the stack.

A strong wrapper does not pin the event loop (the polls are unreffed on child exit as before), so processes still exit normally with input left unflushed.

Verification

New test in test/js/bun/terminal/terminal.test.ts runs ten fresh child processes sequentially, each buffering 1MB into a PTY child that exits immediately, forcing GC in the EOF-to-drain window, and requiring the drain callback to fire. Collection under conservative stack scanning is probabilistic per process (about half hit the window unfixed), so one process is not a reliable probe but ten sequential ones are; the loop stops at the first loss.

  • unfixed debug build: test failed 4/4 runs (first or second child loses the drain)
  • fixed debug build: passed 6/6 runs, ~5s
  • test/js/bun/terminal/ (132 tests), spawn.test.ts, spawn-ipc-gc.test.ts, spawn-many-teardown.test.ts all pass
  • cargo check -p bun_runtime --target x86_64-pc-windows-msvc clean

Not changed here: the generic JsRef::Weak liveness gap (a raw JSValue, no GC tracking) affects other consumers and deserves its own pass; the PTY fd retention after dropped sessions and the writer buffer retained by close() are separate issues.


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