Recover terminal output flow from a dropped ack#172
Merged
Conversation
The per-terminal output flow control added in #169 sends at most one unacknowledged batch to the renderer and gates further sends until the renderer acks by sequence. The in-flight gate was only ever cleared by a matching ack or by terminal exit, so a renderer that reloaded or crashed after a batch was emitted but before it acked left the gate stuck: every later flush skipped that terminal and its live output silently froze until the process exited. The rolling buffer kept updating, so a remount showed a correct snapshot, but subsequent live output never arrived. Resetting the gate in getEmbeddedTerminalBuffer is not viable -- it has many incidental callers (control-server snapshots/SSE, App buffer-length polling) that would defeat the backpressure on every passive read. Instead extract the gate into a pure, unit-tested OutputAckGate (terminal-output-ack.ts) and treat a batch left unacknowledged for longer than DEFAULT_OUTPUT_ACK_TIMEOUT_MS (2s, well above a normal IPC round-trip) as lost: the next flush re-sends with a fresh sequence and the terminal recovers on its own. The dropped batch's bytes are already in the rolling buffer, so a remounting renderer re-renders them from the snapshot. - npm run build:electron -- clean - npm run test:electron -- 145/145 pass (incl. 7 new terminal-output-ack tests) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes a flow-control wedge introduced with the terminal output backpressure in #169.
That change made the Electron main process send at most one unacknowledged output batch per terminal, gating further sends until the renderer acks by sequence (
embeddedTerminal:dataAck). The in-flight gate (inFlightOutput) was only ever cleared by:clearTerminalOutputState).Embedded terminals live in the main process and survive renderer reloads. So if a renderer reloads or crashes after a batch is emitted but before it acks, the gate stays set forever — every later
flushOutputhitsif (... || inFlightOutput.has(id)) continueand skips that terminal. Its live output silently freezes until the process exits. The rolling buffer keeps updating (appendBufferruns independently), so a remount shows a correct one-shot snapshot viagetEmbeddedTerminalBuffer, but subsequent incremental output never arrives.Why not reset the gate on
getEmbeddedTerminalBufferThat was the obvious hook (a remount re-fetches the buffer), but
getEmbeddedTerminalBufferhas many incidental callers —control-server.tscontrol snapshots and SSE backfill, andApp.tsxpolling the buffer length as a marker. Resetting the gate there would relax the backpressure on every passive read, defeating its purpose.Change
OutputAckGate(client/electron/terminal-output-ack.ts), replacing the inlineinFlightOutputmap +nextOutputSequencecounter.DEFAULT_OUTPUT_ACK_TIMEOUT_MS(2s) — comfortably above a normal sub-millisecond IPC round-trip — as lost. The nextflushOutputre-sends with a fresh sequence, so the terminal recovers on its own without any remount signal. The dropped batch's bytes are already in the rolling buffer, so a remounting renderer re-renders them from the snapshot; no data is lost.A late ack for a since-resent batch is ignored (sequence mismatch), so there's no spurious clear or double-flush. Normal-path behavior (ack within milliseconds) is unchanged.
Verification
npm run build:electron— cleannpm run test:electron— 145/145 pass, including 7 newterminal-output-acktests (gate holds until ack, ignores stale acks, releases after timeout, late-ack-after-resend is ignored,clear()drops the batch).Context
Follow-up to the flow-control work in #169 (merged). Flagged during PR review as the one outstanding correctness gap in that change.
🤖 Generated with Claude Code