Skip to content

fix(terminal): bound live xterm backpressure - #339

Merged
Ark0N merged 1 commit into
Ark0N:masterfrom
dignfei:fix/terminal-live-write-backpressure
Aug 26, 2026
Merged

fix(terminal): bound live xterm backpressure#339
Ark0N merged 1 commit into
Ark0N:masterfrom
dignfei:fix/terminal-live-write-backpressure

Conversation

@dignfei

Copy link
Copy Markdown

Follow-up to #331.

Summary

  • Apply backpressure to live xterm writes by waiting for each parse callback before handing off the next bounded chunk.
  • Include queued, loading, in-flight, and incoming bytes in the 128 KiB client cap; keep automatic shell recovery on the bounded 1 MiB tail.
  • Drop redundant SSE terminal payloads while WebSocket owns terminal I/O, and make recovery single-flight and session-safe.
  • Document the bounded ordinary-shell scrollback behavior.

Testing

  • 14 focused Vitest files: 251 tests passed.
  • TypeScript type check passed.
  • Frontend syntax check passed for 34 files.
  • Prettier and staged diff checks passed.

@Ark0N

Copy link
Copy Markdown
Owner

Reviewed against master and checked the surrounding code paths on the live instance. The core idea is right: capping only the app-side queues while xterm's own WriteBuffer sat outside the accounting was a real hole, and gating the next chunk on the parse callback is the correct way to close it. Two things I would like sorted before this goes in with the next release.

1. Shell recovery loses the downgrade guard

_onSessionNeedsRefresh now reads:

constuseFullHistory=this.sessions.get(sessionId)?.mode!=='shell';
...
if(useFullHistory&&data.terminalBuffer&&this._replayWouldShrinkBuffer(data.terminalBuffer)){ ... }

so a shell fetches a 1 MiB tail and goes straight into terminal.clear() + reset() + chunkedTerminalWrite() with no shrink check. _replayWouldShrinkBuffer is not a repaint-mode-only concern, it answers "would this replay delete rows the browser is currently holding", and a shell is exactly where that can happen: 1 MiB of colored output is roughly 10k to 15k rows, while xterm keeps 50k. A shell that has just produced a large burst can therefore lose everything above the tail on the recovery that same burst triggered. That is the shape of the bug #284/#285/#286 fixed in 1.18.1, and the comment this PR removes carries the measurement (an 869-row buffer replaced by 158 rows).

I agree with the goal: a multi-megabyte automatic replay on the main thread is not acceptable. Could you keep the guard on both branches, and in the shell case skip the replay when it would shrink rather than falling back to full=1? A buffer that is briefly corrupted but complete beats one that is clean and truncated, and Load full history is still there. Concretely: drop useFullHistory && from the condition and return in the shell branch.

2. The effective burst budget is now roughly half

_terminalWriteInFlightBytes is up to MAX_FRAME_BYTES (64 KiB, 32 KiB for codex), so during a sustained burst the 128 KiB cap has about 64 KiB of usable headroom, and the test became queued + data.data.length > 131072 rather than queued > 131072. The accounting is more honest than before, no argument. It does mean the drop path fires more often, and every drop schedules a recovery replay, which compounds with (1): burst causes drop, drop causes recovery, recovery truncates the scrollback the burst produced. Did you measure how much more often the drop fires on a real flood (a build log, yes)? If it is noticeably more, raising the cap alongside this change may be the better trade.

3. Smaller things

  • _terminalWriteInFlight / _terminalWriteInFlightBytes are not cleared in the four reset paths where you added _terminalRefreshOwner (_resetAllAppState, _cleanupPreviousSession, the activeSessionId = null teardown). It self-heals today because the terminal is never disposed, so the callback always lands, but every other queue counter is zeroed there and leaving these two out will read as an oversight later.
  • The single-flight in _onSessionNeedsRefresh discards a refresh that arrives while one is in flight (if (this._terminalRefreshOwner?.sessionId === sessionId) return). A second genuine drop during a long recovery then goes unrepaired until the next one. A pending flag re-run in the finally would coalesce instead of discard.
  • The expect(body).toContain("const useFullHistory = ...") assertions pin literal source text, so a rename breaks them with no behavior change. The behavioral tests beside them, the parse-callback one in particular, already cover the same ground.

What I checked and am happy with

Widening the SSE guards from _wsReady && _wsSessionId === data.id to just _wsReady is safe. All three of _onSessionTerminal, _onSessionNeedsRefresh and _onSessionClearTerminal no-op for a non-active session; _cleanupPreviousSession calls _disconnectWs(), which clears _wsReady synchronously, before activeSessionId is reassigned, so there is no window where the socket is ready for a stale session; and the WS emits r/c for the same session-level events SSE carries. I also confirmed terminal.reset() does not recreate xterm's WriteBuffer, so an in-flight callback survives a session switch and the new gate cannot latch.

Test-merged this against #340 with git merge-tree: clean, so the two can land in either order.

@Ark0N
Ark0N merged commit 00b32ad into Ark0N:masterAug 26, 2026
2 checks passed
Ark0N pushed a commit that referenced this pull request Aug 26, 2026
install.sh installs a build toolchain on Linux (node-pty has no Linux
prebuild, so a stock Ubuntu 24 server died inside node-gyp with
"not found: make"), plus review hardening for #339: the write-queue
reset paths now release the one-chunk-in-flight gate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

2 participants

@dignfei@Ark0N