From cd76d0797efbf2a9693b6aa67e84852cbb64bf63 Mon Sep 17 00:00:00 2001 From: Jinwoo-H Date: Fri, 17 Jul 2026 03:28:10 -0400 Subject: [PATCH] fix(terminal): stop flood-abandon from duplicating replay-covered lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 750ms foreground deadline can abandon a hidden-output restore while a RESOLVED snapshot's replay is still parsing. Those replay writes are already queued in xterm's FIFO and WILL paint everything at or before the snapshot seq, but the abandon (a) rewrote its parked live chunks raw — duplicating every line the replay covers (field: TUI lines stacked twice on reveal) — and (b) skipped baseline adoption, so main's ACK-backlog re-deliveries below the seq duplicated again. Track the in-flight replay snapshot from the moment its writes are queued; on abandon, seq-slice the pending write-through against it (raw fallback only for unmappable offsets — availability still wins) and adopt it as the live reconcile baseline with the drain's continuity advance. Abandons with no started replay keep the existing raw path. Deterministic repro: pty-connection.test.ts stages snapshot-resolve → replay parse held → deadline abandon, asserting no rewrite of covered chunks, dedup of post-abandon backlog, and write-through of new bytes. Co-authored-by: Orca --- .../terminal-pane/pty-connection.test.ts | 107 ++++++++++++++++++ .../terminal-pane/pty-connection.ts | 46 +++++++- 2 files changed, 152 insertions(+), 1 deletion(-) diff --git a/src/renderer/src/components/terminal-pane/pty-connection.test.ts b/src/renderer/src/components/terminal-pane/pty-connection.test.ts index d5f2f84dd6a..3bc2700abb0 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.test.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.test.ts @@ -11498,6 +11498,113 @@ describe('connectPanePty', () => { disposable.dispose() }) + it('slices abandoned pending chunks against a replay that already painted', async () => { + // Field bug: the 750ms foreground deadline can fire while a RESOLVED + // snapshot's replay is still parsing (its writes are already in xterm's + // FIFO and WILL paint). The abandon used to rewrite parked chunks raw, + // duplicating every line the replay covers, and left the live reconcile + // baseline stale so main's ACK backlog duplicated again. + const { connectPanePty } = await import('./pty-connection') + const transport = createMockTransport('pty-id') + const capturedDataCallback: { + current: ((data: string, meta?: { seq?: number; rawLength?: number }) => void) | null + } = { current: null } + transport.connect.mockImplementation(async ({ callbacks }: { callbacks: ConnectCallbacks }) => { + capturedDataCallback.current = callbacks.onData ?? null + return 'pty-id' + }) + transportFactoryQueue.push(transport) + const getMainBufferSnapshot = window.api.pty.getMainBufferSnapshot as unknown as ReturnType< + typeof vi.fn + > + const snapshot = createDeferred<{ + data: string + cols: number + rows: number + seq: number + pendingDeliveryStartSeq: number + }>() + getMainBufferSnapshot.mockReturnValue(snapshot.promise) + const hidden = 'hidden-codex-output\r\n' + const coveredLive = 'LIVE_DUP_LINE\r\n' + const afterAbandon = 'AFTER_ABANDON\r\n' + + const pane = createPane(1) + const manager = createManager(1) + const deps = createDeps({ + isVisibleRef: { current: false }, + startup: { command: 'codex' } + }) + const disposable = connectPanePty(pane as never, manager as never, deps as never) + await flushAsyncTicks(6) + expect(capturedDataCallback.current).not.toBeNull() + + vi.useFakeTimers() + capturedDataCallback.current?.(hidden, { seq: hidden.length, rawLength: hidden.length }) + ;(deps.isVisibleRef as { current: boolean }).current = true + // Parked while the snapshot fetch is in flight; also arms the deadline. + capturedDataCallback.current?.(coveredLive, { + seq: hidden.length + coveredLive.length, + rawLength: coveredLive.length + }) + await flushAsyncTicks(4) + + // The snapshot resolves covering the parked chunk, but its replay writes + // never finish parsing (writes below hold their callbacks), so the + // deadline fires mid-replay. + const heldCallbacks: (() => void)[] = [] + pane.terminal.write.mockImplementation(function write( + data: string, + callback?: () => void + ): void { + if (callback) { + heldCallbacks.push(callback) + } + void data + }) + snapshot.resolve({ + data: 'SNAP_STATE\r\n', + cols: 100, + rows: 30, + seq: hidden.length + coveredLive.length, + pendingDeliveryStartSeq: 0 + }) + await flushAsyncTicks(6) + + vi.advanceTimersByTime(750) + await flushAsyncTicks(10) + + // The replay covers the parked chunk — the abandon must not rewrite it. + const writtenAfterAbandon = pane.terminal.write.mock.calls.map(([data]) => data as string) + expect(writtenAfterAbandon.join('')).not.toContain('LIVE_DUP_LINE') + + // The abandoned replay still paints up to its seq, so it must become the + // live reconcile baseline: a backlog re-delivery at or below it is a + // duplicate... + pane.terminal.write.mockClear() + capturedDataCallback.current?.(coveredLive, { + seq: hidden.length + coveredLive.length, + rawLength: coveredLive.length + }) + await flushAsyncTicks(4) + expect(pane.terminal.write.mock.calls.map(([data]) => data as string).join('')).not.toContain( + 'LIVE_DUP_LINE' + ) + + // ...while genuinely-new bytes past the baseline must still write through. + capturedDataCallback.current?.(afterAbandon, { + seq: hidden.length + coveredLive.length + afterAbandon.length, + rawLength: afterAbandon.length + }) + await flushAsyncTicks(4) + expect(pane.terminal.write.mock.calls.map(([data]) => data as string).join('')).toContain( + 'AFTER_ABANDON' + ) + + heldCallbacks.forEach((callback) => callback()) + disposable.dispose() + }) + it('abandons a stalled hidden restore and drains pending foreground chunks warning-first', async () => { const { connectPanePty } = await import('./pty-connection') const transport = createMockTransport('pty-id') diff --git a/src/renderer/src/components/terminal-pane/pty-connection.ts b/src/renderer/src/components/terminal-pane/pty-connection.ts index 0408f68eded..7b8f4e852be 100644 --- a/src/renderer/src/components/terminal-pane/pty-connection.ts +++ b/src/renderer/src/components/terminal-pane/pty-connection.ts @@ -5182,6 +5182,14 @@ export function connectPanePty( let hiddenOutputRestoreGeneration = 0 // Flood-backpressure suppression (HIDDEN_OUTPUT_RESTORE_FLOOD_SUPPRESS_MS). let hiddenOutputRestoreFloodSuppressedUntil = 0 + // Why: once a restore snapshot's replay writes are queued, they WILL paint + // (xterm FIFO) even if the restore is abandoned mid-parse. Abandon must + // slice its pending write-through against this snapshot or every line the + // replay covers renders twice. + let hiddenOutputRestoreReplayingSnapshot: { + seq?: number + pendingDeliveryStartSeq?: number + } | null = null let hiddenOutputRestoreFloodRepaintTimer: ReturnType | null = null // Why: after a snapshot restore, main can still drain ACK-backlog chunks // whose bytes the snapshot already covers — writing them unguarded @@ -6278,6 +6286,8 @@ export function connectPanePty( ? [] : hiddenOutputRestorePendingChunks.slice() const hadPendingOverflow = hiddenOutputRestorePendingOverflow + const replayingSnapshot = hiddenOutputRestoreReplayingSnapshot + hiddenOutputRestoreReplayingSnapshot = null hiddenOutputRestoreGeneration += 1 if ( hiddenOutputSnapshotScrollRestore?.valid && @@ -6313,7 +6323,30 @@ export function connectPanePty( if (hadPendingOverflow) { return } - const pendingData = pendingChunks.map((chunk) => chunk.data).join('') + // Why: an abandoned-but-started replay is already queued in xterm's FIFO + // and will paint everything at or before its seq. Rewriting covered + // chunks raw duplicated TUI lines (field: same line stacked twice on + // reveal); slice against the replay and fall back raw only when offsets + // cannot be mapped — availability still wins over a dropped tail. + const replayedSeq = typeof replayingSnapshot?.seq === 'number' ? replayingSnapshot.seq : null + let pendingData = '' + for (const chunk of pendingChunks) { + const sliced = + replayedSeq === null ? chunk.data : getChunkDataAfterSnapshot(chunk, replayedSeq) + pendingData += sliced ?? chunk.data + } + if (replayingSnapshot && replayedSeq !== null) { + // Why: adopt the painting replay as the live reconcile baseline so + // main's ACK backlog at or below it dedups instead of duplicating. + setRestoredSnapshotBaseline(expectedPtyId, replayingSnapshot) + for (const chunk of pendingChunks) { + // Mirror the drain's continuity: written chunks advance the point + // so later live chunks are neither re-dropped nor misread as gaps. + if (typeof chunk.seq === 'number' && restoredSnapshotExpectedStartSeq !== null) { + restoredSnapshotExpectedStartSeq = Math.max(restoredSnapshotExpectedStartSeq, chunk.seq) + } + } + } if (pendingData) { writePtyOutputToXterm(pendingData, true) } @@ -6358,6 +6391,7 @@ export function connectPanePty( resetHiddenRendererRiskState() hiddenOutputRestoreNeeded = false hiddenOutputRestorePtyId = null + hiddenOutputRestoreReplayingSnapshot = null hiddenOutputRestoreGeneration += 1 } @@ -6449,6 +6483,7 @@ export function connectPanePty( cols: number rows: number seq?: number + pendingDeliveryStartSeq?: number alternateScreen?: boolean scrollbackAnsi?: string pendingEscapeTailAnsi?: string @@ -6484,6 +6519,14 @@ export function connectPanePty( return } scrollRestore.started = true + if (typeof snapshot.seq === 'number') { + hiddenOutputRestoreReplayingSnapshot = { + seq: snapshot.seq, + ...(typeof snapshot.pendingDeliveryStartSeq === 'number' + ? { pendingDeliveryStartSeq: snapshot.pendingDeliveryStartSeq } + : {}) + } + } discardTerminalOutput(pane.terminal) if ( hasSnapshotDimensions && @@ -6750,6 +6793,7 @@ export function connectPanePty( // still draining from main's ACK backlog below that point are // duplicates the dataCallback reconciliation must suppress. setRestoredSnapshotBaseline(currentPtyId, snapshot) + hiddenOutputRestoreReplayingSnapshot = null const needsFreshSnapshot = hiddenOutputRestoreFreshSnapshotNeeded hiddenOutputRestoreFreshSnapshotNeeded = false const drainOutcome = drainPendingLiveChunksAfterSnapshot(snapshot.seq)