[hussainph]

Problem

The renderer acked each PTY output batch when the IPC event arrived (sessions-layer.tsx), before xterm had parsed it. xterm's write is asynchronous: it queues the chunk and parses it in time slices. So:

1. Backpressure never engaged. Acks came back one hop after each send, so main's unacked count stayed near zero. yes / cat bigfile kept sending 256K batches every 8 ms while xterm's queue grew without bound.
2. Overflow froze the PTY for good. Past 5e7 pending chars, xterm throws write data discarded… from write(). That throw happened on the line before ack(...), so the chunk's credit was lost. Main's unacked count stayed above the 5K low watermark, the PTY stayed paused, and the pane stayed frozen until it was closed.

Fix

• TerminalEngine.write(data, onParsed?): the engine calls onParsed once it no longer holds the chunk. XtermEngine passes it as xterm's write callback, which fires after parse. Chunks still queued when the engine is disposed are released by dispose, because xterm drops queued callbacks without calling them. A disposed engine releases a late write at once.
• New terminal/output-dispatch.ts (deliverTerminalOutput), used by the sessions layer. It acks exactly once per delivered batch:
  • when the engine releases the chunk;
  • at once if there is no engine;
  • in the catch if write throws (the error is logged, not rethrown).
• terminal/registry.ts disposes every engine on pagehide. A renderer reload tears xterm down without running its queued parse callbacks, and main does not treat a reload as a detach. Without this, up to a watermark plus a batch of credit would be stranded per busy PTY, leaving it paused (the VC-425 leak class, made wider by acking after parse). dispose now releases that credit while the page can still send it.
• Main-side watermarks, batching and one ack per batch are unchanged.

Decisions

• Pre-attach output is acked on receipt, not when it is flushed. The ticket proposed acking it on flush. I took the conservative option. The pre-attach buffer is already capped (64K, trimmed by whole chunks), so it cannot grow the way xterm's queue can. Holding its credit until a view mounts could pause a PTY nobody is looking at; one resync tail (up to 256K) is already above the high watermark. Today's behaviour for that path is kept exactly.
• On a refused write, the caller owns the credit. The engine contract says the engine never calls onParsed for a write that throws. The dispatcher acks in its catch, and its once-guard makes a double ack impossible either way.
• The pty-level smoke interrupts the burst instead of running the ticket's literal yes | head -c 300000000; echo done. The first push used the literal form, and check 14 failed on both CI attempts: after 240 s the pane still showed y rows, with no discard error. A marker after a fixed-size burst can only show once xterm has parsed every char of the burst, fix or no fix. The fix does not change how fast xterm parses; it changes how much is queued. So the smoke measures that instead: after Ctrl-C, the next command's output appears within moments when flow control holds xterm's queue to a few hundred K. With ack-on-receipt it waits behind tens of MB, or never shows if the PTY is stuck. A stall in the new path would also fail this check.
• Accepted: a hidden window may slow a busy PTY. Acks now wait on xterm's setTimeout-driven parse, and production windows use backgroundThrottling: true (quiet-windows.ts:38 → index.ts:576). A minimized or covered window may therefore limit a busy PTY's throughput to about one parse slice per second until it is shown again. This is reasoned, not measured, and VS Code works the same way. Follow-up to measure it: VC-859.
• output-dispatch.ts is enrolled in the desktop 100% coverage gate: a branch that loses an ack fails silently and permanently.

Tests

• apps/desktop/src/main/pty.test.ts, new block "flow control through the renderer's output dispatch". It drives main's real IPC adapter (volli:terminal-data out, volli:terminal-ack in) through the real dispatcher with an engine that parses late:
  • During a yes-style burst, pty.pause() is called; after the engine parses everything, the PTY resumes.
  • After the pause, a write that throws still acks, and the PTY resumes after the burst.
  • Verified failing without the fix: with the dispatcher swapped to the old "write, then ack on receipt" shape, both tests fail (pty.pause called 0 times). Single-file run, --maxWorkers=1.
• terminal/output-dispatch.test.ts: one ack after release, immediate ack with no engine, ack plus log on throw, no double ack.
• terminal/xterm-engine.test.ts: release only on xterm's parse callback, pre-attach release on receipt, release of queued chunks on dispose (each release isolated), no release of a refused write.
• terminal/registry.test.ts: pagehide disposes and forgets every engine, even past one whose dispose throws.
• e2e/terminal-smoke.mjs check 14 (runs in CI): run yes for 5 s, Ctrl-C it, then echo "flow-$((6 * 7))-done". flow-42-done must appear in the pane within 30 s, with no write data discarded console error. The detail reports drainMs. This check guards the ack-after-parse path against a stall; it does not by itself prove the old regression. The old code also passes it whenever xterm keeps up with 5 s of yes. The pty.test.ts tests above are the regression proof. The marker is computed by the shell, so the typed command line can't satisfy the wait.

Local runs, each through the machine's serial runner with --maxWorkers=1, one file at a time after review: pty.test.ts (226 passed), registry.test.ts (6 passed), xterm-engine.test.ts (12 passed), output-dispatch.test.ts (4 passed, before review), and vp check on every touched file. The smoke was not run locally (CI only).

Overlap: git merge-tree is clean against origin/main and against the heads of #909, #919, #920, #921 and #922. Targets main.

Review

From the review of 247936f (ready with nits, no must-fix). Addressed in b0ac31d:

• SHOULD 1, done. pagehide disposes every engine, so a reload returns queued credit. Comment references VC-425; test in registry.test.ts. I took the renderer-side option, not a main-side releaseFlow on navigation: it reuses the dispose release this PR already added and needs no new IPC.
• SHOULD 3, done by stating it. The PR body and the smoke comment now say check 14 guards against a stall rather than proving the regression. I added no drainMs < 5000 gate: this is a core e2e smoke, so a timing gate would risk flakes on a gating lane, and drainMs stays in the detail.
• SHOULD 2, recorded, not fixed. Added under Decisions; follow-up VC-859.
• Nits, all done:
  • The console.error spy in pty.test.ts is restored in a finally.
  • The swapped first/second names are now throwing/plain.
  • The onParsed call inside xterm's parse loop is wrapped in try/console.warn, with a new test. dispose relies on it.
  • output-dispatch.ts now notes that acks carry no attachment generation: the clamp means a late ack can only cause an early resume, never a freeze.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: dae97d10-860d-400b-af44-7ad65e7a29b1

• 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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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