Skip to content

fix(terminal): drain deferred output without a wake event - #235

Merged
Ark0N merged 1 commit into
Ark0N:masterfrom
Lint111:feat/deferred-terminal-flush
Aug 9, 2026
Merged

fix(terminal): drain deferred output without a wake event#235
Ark0N merged 1 commit into
Ark0N:masterfrom
Lint111:feat/deferred-terminal-flush

Conversation

@Lint111

Copy link
Copy Markdown
Contributor

Reviving #190, which you listed second in #173. Rebased onto current master (fa18eee).

The bug

batchTerminalWrite() splits a large payload into 32 KB frames, but after writing the
first frame it only re-armed the drain loop from the incoming output path. When a burst
is the last thing a terminal emits — a build finishing, a long git log, a Codex
response landing in one go — nothing further arrives to wake the loop, so the remaining
chunks sit in pendingWrites indefinitely.

The visible symptom is a terminal that stops mid-output and only completes when you type
something or unrelated output happens to arrive.

Repro

Open a session, run a command whose final output exceeds the 32 KB first-frame budget
(e.g. git log -p | head -c 100000), and don't touch the terminal. The tail never renders.

The fix

The drain loop re-schedules itself while work remains, instead of depending on an external
wake. No change to the frame budgets or the anti-flicker behaviour they exist for — only
to what re-arms the loop.

Test

test/terminal-flush-budget.test.ts — "drains a large final batch without waiting for
unrelated terminal output". It queues 96 KB, then drains only the scheduled callbacks,
with no further input.

Verified against currentmaster by checking the test file out onto unmodified
master in a scratch worktree:

AssertionError: expected [ 32768 ] to deeply equal [ 32768, 32768, 32768 ]
Tests 1 failed | 6 passed (7)

master writes the first 32 KB frame and strands the other 64 KB. The other six tests in
the file pass there, so the harness itself is sound on master.

With the change: 7 passed, pendingWrites empty and writeFrameScheduled false.
Full suite on top of current master: 4075 passed, 0 failed. tsc --noEmit clean.

On your two notes from #173

The terminal-anti-flicker.md rewrite is in this PR, as you asked — the doc described
the old client-side parser and would have been wrong on its own.

On the terminal-ui.js churn since July 29: I read the commits rather than assuming.
Thirteen touch the file since then, and none touch the write/flush path this PR changes —
I checked each for edits to batchTerminalWrite / pendingWrites / writeFrameScheduled
/ _safeYield and all came back clean. The work (#205 scrollback paging, wheel capture,
touch forwarding, glide/easing) sits in the scroll-routing paths, exactly as you predicted.
The rebase was mechanical: one commit, cherry-picked onto fa18eee with no conflicts.

Notes

  • No new setting; always on, since the stranded-tail state is never desirable.
  • Branched from current master, single commit, merges cleanly.

🤖 Generated with Claude Code

CopilotAI lite review requested due to automatic review settings August 8, 2026 20:08
CopilotAI reviewed Aug 8, 2026

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Ark0N

Ark0N commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Reviewed against the terminal render pipeline and the anti-flicker invariants.

Root cause confirmed. The old yield callback cleared writeFrameScheduled only after flushPendingWrites() returned, so when a large batch requeued its remainder, the reschedule attempt inside the flush saw the flag still set and skipped. The tail then sat in pendingWrites until unrelated output arrived, which is exactly the reported symptom: truncated final responses and shell commands that look idle until you poke them. Clearing the flag before the flush is the right fix, and unifying the three duplicated scheduling sites into _scheduleTerminalWriteFlush() removes the chance of the same mistake recurring in one of them.

What I verified:

  • No runaway scheduling: the early return on pendingWrites.length === 0 plus the budget cap in flushPendingWrites() means the chain strictly drains and terminates.
  • The new test is a real regression test: on the old code the second and third 32KB frames never get scheduled, so it fails there and passes here (96KB codex batch draining as three 32KB writes with the queue empty and the flag reset at the end).
  • Server-side idle detection is unaffected: this is purely the browser render path; output-silence and token-stability detection read PTY output on the server.
  • The doc rewrite in terminal-anti-flicker.md now matches what the code actually does (xterm.js 6 native DEC 2026 handling, mode-aware 32/64KB budgets, functions living in terminal-ui.js), including removing the stale 50ms discard trade-off section.

Nice catch on a subtle one. Merging.

@Ark0N
Ark0N merged commit b067aad into Ark0N:masterAug 9, 2026
2 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@Lint111@Ark0N