diff --git a/.changeset/dropped-output-recovery.md b/.changeset/dropped-output-recovery.md new file mode 100644 index 00000000..f0fb3a5f --- /dev/null +++ b/.changeset/dropped-output-recovery.md @@ -0,0 +1,13 @@ +--- +"aicodeman": patch +--- + +fix(terminal): a dropped output frame is recovered, not just scheduled for recovery + +`_onSessionTerminal` drops an incoming frame once the app-owned render queues already hold 128KB. That is the right call — the alternative is an unbounded backlog — but a hole in a TUI byte stream is a desynced cursor, and a desynced cursor is muffled text (#464). The drop was only half of it. + +The recovery was a fire-and-forget timer: it nulled its own handle and then called `_onSessionNeedsRefresh()`, which opens with four early returns. Two of those — a buffer load already in flight, a refresh already owning this session — are **most likely to be true during exactly the output burst that caused the drop**, so the recovery was skipped precisely when it was needed, with nothing left to retry it, and those bytes were never replayed. + +`_onSessionNeedsRefresh` now reports whether it actually repainted, and `_scheduleDroppedOutputRecovery` re-arms while it has not. Bounded, because every reason the refresh can be skipped is transient contention that clears in seconds and a permanently failing refresh must not become a loop against the API — and giving up at the cap leaves exactly what the old code left, so the floor is no worse. The same 2s debounce still collapses a burst of drops into one attempt. + +This is the principle the review of #431 established for the WebSocket output-gap marker — only a repaint that actually happened settles the recovery — applied to the one recovery path that still relied on a timer having fired. diff --git a/CLAUDE.md b/CLAUDE.md index 8f8d57de..038d5fe4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -261,7 +261,7 @@ Codeman is a Claude Code session manager with web interface and autonomous Ralph **Circuit breakers**: the Ralph breaker prevents respawn thrashing (`CLOSED` → `HALF_OPEN` → `OPEN`; reset via `/api/sessions/:id/ralph-circuit-breaker/reset`). **Distinct: the PTY-exit breaker** (`session-pty-exit-breaker.ts`) trips after repeated rapid PTY exits and blocks auto-restarts. ⚠️ It resets ONLY via an explicit `{clearBreaker:true}` body on `POST /api/sessions/:id/interactive`; the frontend's auto-reattach in `selectSession()` sends no body and must never clear it. → [architecture-invariants#circuit-breakers-ralph--pty-exit](docs/architecture-invariants.md#circuit-breakers-ralph-and-pty-exit) -**Full-scrollback replay**: `GET /api/sessions/:id/terminal?full=1` returns the entire tmux scrollback, bounded by the configured history limit. On success the capture is returned ALONE (`source='mux-full-history'`), superseding the byte buffer so nothing duplicates. The first load of each non-shell TUI session per page requests `full=1` (`_fullHistoryLoaded` Set); Shell selection and automatic drop recovery always use a bounded 1 MiB `?tail=` window. Shell loads the rest only when **Load full history** is pressed; ordinary scrolling must not trigger a multi-megabyte reset+replay on xterm's main thread. Other modes may re-pull at the TOP (cooldown-guarded — tmux repaints bursty output in place, so browser scrollback shrinks while tmux's history stays complete). Live writes are one-chunk-in-flight, released by xterm's parse callback, so xterm's private queue cannot bypass the browser's 128 KiB render cap. While WebSocket owns terminal I/O, duplicate SSE terminal events are dropped before JSON parsing, and recovery is single-flight per active session. ⚠️ **A `full=1` capture ENDS with a cursor move back to the pane's own caret position**, counted UP from the last replayed row — without it the caret stays where the last character landed, which for an agent CLI is the status line, and every cursor-relative update the CLI sends afterwards is measured from the wrong row. The move is relative, not `CUP`: absolute row addressing is only right while the browser's rows equal the pane's, and `resizeWindow` does not wait for tmux, so a capture can be taken before a requested resize applies. That makes row alignment load-bearing on this path: no transform that can DELETE A LINE may run over the capture, so it keeps its trailing blank rows and skips redraw-bloat stripping, the banner trim and the leading-whitespace strip. ⚠️ Those three skips key on whether a capture actually CAME BACK (`isFullCapture`), never on `?full=1` alone — the fallback to the byte history is a stream of successive frames that must still be stripped, and a session with no mux takes it on every load. A capture holding nothing visible returns '' so the byte history survives instead of a blank screen replacing it. ⚠️ A full re-pull must never DOWNGRADE the buffer: a repaint-mode CLI pane keeps no tmux history, so its capture is one frame and the reset+rewrite would delete history mid-scroll — `_replayWouldShrinkBuffer()` refuses it and slows that session's cooldown to 60s. ⚠️ **A visible capture now REPORTS the geometry it was taken at** (`captureCols`/`captureRows`, #435), because a frame built for a pane taller or wider than the browser is damaged two ways at once (overflow rows clamp onto the last line; a narrower browser wraps every painted row) and nothing in the response used to say so. Both fields are ABSENT when no frame was positioned, so every consumer tests `Number.isFinite`, never truthiness: a `display-message` cursor query that fails makes `capturePaneBuffer` return the raw capture while the route still labels it `mux-visible`. The comparison runs on `mux-visible` ONLY, the replay is capped at one attempt, and a pane that cannot be sized to fit latches in `_geometryRetryUseless` so it is diagnosed once per session rather than on every tab switch. → [architecture-invariants#full-scrollback-replay](docs/architecture-invariants.md#full-scrollback-replay) +**Full-scrollback replay**: `GET /api/sessions/:id/terminal?full=1` returns the entire tmux scrollback, bounded by the configured history limit. On success the capture is returned ALONE (`source='mux-full-history'`), superseding the byte buffer so nothing duplicates. The first load of each non-shell TUI session per page requests `full=1` (`_fullHistoryLoaded` Set); Shell selection and automatic drop recovery always use a bounded 1 MiB `?tail=` window. Shell loads the rest only when **Load full history** is pressed; ordinary scrolling must not trigger a multi-megabyte reset+replay on xterm's main thread. Other modes may re-pull at the TOP (cooldown-guarded — tmux repaints bursty output in place, so browser scrollback shrinks while tmux's history stays complete). Live writes are one-chunk-in-flight, released by xterm's parse callback, so xterm's private queue cannot bypass the browser's 128 KiB render cap. ⚠️ **A frame dropped at that cap MUST be recovered, and the recovery must verify itself** (`_scheduleDroppedOutputRecovery`, app.js): a hole in a TUI byte stream is a desynced cursor, which is muffled text (#464). It was a fire-and-forget 2s timer that nulled its own handle and then called `_onSessionNeedsRefresh()` — whose early returns (a buffer load in flight, a refresh already owning the session) are MOST likely to be true during exactly the burst that caused the drop, so the recovery was lost silently and the bytes were never replayed. `_onSessionNeedsRefresh` now returns whether it actually repainted, and the scheduler re-arms while it has not, bounded by `DROP_RECOVERY_MAX_ATTEMPTS` because every reason it can be skipped is transient contention. The same debounce still collapses a burst into one attempt. Tests: `test/dropped-output-recovery.test.ts`, whose retry case and no-retry case only pin the fix as a pair. While WebSocket owns terminal I/O, duplicate SSE terminal events are dropped before JSON parsing, and recovery is single-flight per active session. ⚠️ **A `full=1` capture ENDS with a cursor move back to the pane's own caret position**, counted UP from the last replayed row — without it the caret stays where the last character landed, which for an agent CLI is the status line, and every cursor-relative update the CLI sends afterwards is measured from the wrong row. The move is relative, not `CUP`: absolute row addressing is only right while the browser's rows equal the pane's, and `resizeWindow` does not wait for tmux, so a capture can be taken before a requested resize applies. That makes row alignment load-bearing on this path: no transform that can DELETE A LINE may run over the capture, so it keeps its trailing blank rows and skips redraw-bloat stripping, the banner trim and the leading-whitespace strip. ⚠️ Those three skips key on whether a capture actually CAME BACK (`isFullCapture`), never on `?full=1` alone — the fallback to the byte history is a stream of successive frames that must still be stripped, and a session with no mux takes it on every load. A capture holding nothing visible returns '' so the byte history survives instead of a blank screen replacing it. ⚠️ A full re-pull must never DOWNGRADE the buffer: a repaint-mode CLI pane keeps no tmux history, so its capture is one frame and the reset+rewrite would delete history mid-scroll — `_replayWouldShrinkBuffer()` refuses it and slows that session's cooldown to 60s. ⚠️ **A visible capture now REPORTS the geometry it was taken at** (`captureCols`/`captureRows`, #435), because a frame built for a pane taller or wider than the browser is damaged two ways at once (overflow rows clamp onto the last line; a narrower browser wraps every painted row) and nothing in the response used to say so. Both fields are ABSENT when no frame was positioned, so every consumer tests `Number.isFinite`, never truthiness: a `display-message` cursor query that fails makes `capturePaneBuffer` return the raw capture while the route still labels it `mux-visible`. The comparison runs on `mux-visible` ONLY, the replay is capped at one attempt, and a pane that cannot be sized to fit latches in `_geometryRetryUseless` so it is diagnosed once per session rather than on every tab switch. → [architecture-invariants#full-scrollback-replay](docs/architecture-invariants.md#full-scrollback-replay) **Split-pane sessions** (`showSplitButton`, header button, default OFF, desktop-only, per-device): a second live session ("Pane B") beside the active one, in its own `SplitTerminalPane` (terminal-split.js) with its own xterm + WebSocket, resizable via a draggable divider. Deliberately plainer than the primary pane — no local-echo overlay, CJK IME, or touch handlers — and NOT persisted across reloads. → [architecture-invariants#split-pane-sessions](docs/architecture-invariants.md#split-pane-sessions) diff --git a/src/web/public/app.js b/src/web/public/app.js index cb073b13..b4e52067 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -2067,14 +2067,10 @@ class CodemanApp { + (this._loadBufferQueue?.reduce((s, w) => s + w.data.length, 0) || 0) + (this._terminalWriteInFlightBytes || 0); if (queued + data.data.length > 131072) { // 128KB — drop to prevent accumulation - // Schedule a self-recovery once the - // queue drains (debounced to avoid hammering the API during sustained bursts). - if (!this._clientDropRecoveryTimer) { - this._clientDropRecoveryTimer = setTimeout(() => { - this._clientDropRecoveryTimer = null; - this._onSessionNeedsRefresh(); - }, 2000); - } + // The bytes are gone from the stream now, so the recovery is the only + // thing that puts this terminal back in step with the PTY. + _crashDiag.log(`TERMINAL DROP: ${(queued / 1024).toFixed(0)}KB queued`); + this._scheduleDroppedOutputRecovery(data.id); return; } @@ -2082,6 +2078,52 @@ class CodemanApp { } } + /** + * Put the terminal back in step after a dropped frame, and keep trying until + * something actually repaints. + * + * ⚠️ A fire-and-forget timer is not a recovery, which is what this used to be: + * it nulled its own handle and then called `_onSessionNeedsRefresh()`, whose + * early returns are most likely to fire during the very burst that caused the + * drop. A skipped refresh lost the recovery with nothing left to retry it, so + * the hole stayed in the stream — and a hole in a TUI byte stream is a + * desynced cursor, which is muffled text (issue #464). + * + * Debounced by the same 2s as before, so a sustained burst still collapses + * into one attempt rather than hammering the API; bounded by + * `DROP_RECOVERY_MAX_ATTEMPTS`, because every reason the refresh can be + * skipped is transient contention. Giving up after the cap leaves exactly + * what the old code left, so the floor is no worse. + * + * @param {string} sessionId - the session whose output was dropped + * @param {number} [attempt] - zero-based, for the bound + */ + _scheduleDroppedOutputRecovery(sessionId, attempt = 0) { + if (!sessionId || this._clientDropRecoveryTimer) return; + this._clientDropRecoveryTimer = setTimeout(async () => { + this._clientDropRecoveryTimer = null; + let repainted = false; + try { + repainted = (await this._onSessionNeedsRefresh({ id: sessionId })) === true; + } catch { + // Treated as "did not repaint" — retrying is the entire point of this. + } + const retry = window.CodemanDroppedOutput.shouldRetryDroppedOutputRecovery({ + repainted, + attempt, + stillActive: this.activeSessionId === sessionId, + }); + if (retry) { + _crashDiag.log(`DROP RECOVERY: attempt ${attempt + 1} did not repaint, retrying`); + this._scheduleDroppedOutputRecovery(sessionId, attempt + 1); + } + // Read through `window.` like terminal-ui.js does with its own constants: + // a bare global resolves in a browser but not in the vm harnesses the gate + // runs app.js under, and this body executes inside a timer where a + // ReferenceError would be swallowed — taking the recovery with it. + }, window.CodemanDroppedOutput.DROP_RECOVERY_DELAY_MS); + } + // ═══════════════════════════════════════════════════════════════ // Response Viewer — native-scroll panel for reading full Claude responses // ═══════════════════════════════════════════════════════════════ @@ -2705,15 +2747,27 @@ class CodemanApp { } } + /** + * Reload this session's buffer from the server. + * + * ⚠️ Returns whether it ACTUALLY reloaded. Four of the paths out of here are + * early returns, and two of them — a buffer load in flight, a refresh already + * owning this session — are most likely to be true during exactly the output + * burst that makes a caller need this. A caller that treats "called" as + * "recovered" silently loses the recovery; `_scheduleDroppedOutputRecovery` + * is the one that cannot afford to. + * + * @returns {Promise} true only once a response has been applied. + */ async _onSessionNeedsRefresh(event = {}) { // Server sends this after SSE backpressure clears — terminal data was dropped, // so reload the buffer to recover from any display corruption. const sessionId = this.activeSessionId; - if (event?.id && event.id !== sessionId) return; - if (!sessionId || !this.terminal) return; + if (event?.id && event.id !== sessionId) return false; + if (!sessionId || !this.terminal) return false; // Skip if buffer load already in progress — avoids competing clear+rewrite cycles - if (this._isLoadingBuffer) return; - if (this._terminalRefreshOwner?.sessionId === sessionId) return; + if (this._isLoadingBuffer) return false; + if (this._terminalRefreshOwner?.sessionId === sessionId) return false; const refreshOwner = { sessionId }; this._terminalRefreshOwner = refreshOwner; try { @@ -2739,7 +2793,7 @@ class CodemanApp { // Bail on a tab switch mid-fetch: writing here would paint this session's // history into the terminal the user is now looking at. The window is two // fetches wide in the fallback case, so this guard is not optional. - if (this.activeSessionId !== sessionId || this._terminalRefreshOwner !== refreshOwner) return; + if (this.activeSessionId !== sessionId || this._terminalRefreshOwner !== refreshOwner) return false; if (data.terminalBuffer) { // This refresh is SERVER-triggered, so a user quietly reading scrollback // did not ask for it and must not be dragged to the bottom by it (#259). @@ -2789,8 +2843,10 @@ class CodemanApp { // replay. Leaving the marker set there refetched on every reconnect for // the life of the page. this._markTerminalBufferReconciled(sessionId); + return true; } catch (err) { console.error('needsRefresh reload failed:', err); + return false; } finally { if (this._terminalRefreshOwner === refreshOwner) this._terminalRefreshOwner = null; } diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 43e5d5ad..cf895d94 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -1698,6 +1698,45 @@ function sanitizeDiagEntry(msg) { .slice(0, DIAG_ENTRY_MAX_CHARS); } +// ── Recovering a dropped output frame ────────────────────────────────────── +// +// `_onSessionTerminal` drops an incoming frame when the app-owned render queues +// already hold 128KB, which is the right call — the alternative is an unbounded +// backlog — but a hole in a TUI byte stream is a desynced cursor, and a desynced +// cursor is muffled text (issue #464). So the drop is only half of it: the +// recovery has to actually happen. +// +// ⚠️ It used to be a fire-and-forget timer. `_onSessionNeedsRefresh` opens with +// four early returns, and two of them — a buffer load in flight, a refresh +// already owning this session — are MOST likely to be true during exactly the +// output burst that caused the drop. The timer nulled itself before the call, +// so a skipped refresh lost the recovery silently and the dropped bytes were +// never replayed. +// +// Bounded, because every reason the refresh can be skipped is transient +// contention that clears in seconds, and a permanently failing refresh must not +// become a forever-loop against the API. Giving up after the cap leaves exactly +// the garbled frames the old code left, so the floor is no worse than before. +const DROP_RECOVERY_DELAY_MS = 2000; +const DROP_RECOVERY_MAX_ATTEMPTS = 5; + +/** + * Should a dropped-output recovery run again? + * + * @param {{repainted: boolean, attempt: number, stillActive: boolean}} state + * `repainted` — whether `_onSessionNeedsRefresh` actually rewrote the buffer. + * `attempt` — how many have already run, zero-based. + * `stillActive` — whether the dropped session is still the one on screen. + * @returns {boolean} + */ +function shouldRetryDroppedOutputRecovery({ repainted, attempt, stillActive }) { + // Switched away: `selectSession` repaints from the server on its own, so a + // retry here would be a second replay of a buffer that is about to be written. + if (!stillActive) return false; + if (repainted) return false; + return attempt + 1 < DROP_RECOVERY_MAX_ATTEMPTS; +} + // ── Terminal geometry: xterm and the PTY must never disagree ─────────────── // // Issue #464 ("text gets muffled"). Claude Code's TUI repaints by wrapping its @@ -1783,6 +1822,11 @@ if (typeof window !== 'undefined') { FETCH_DEADLINE_MAX_MS, }; window.CodemanDiag = { sanitizeDiagEntry, DIAG_ENTRY_MAX_CHARS }; + window.CodemanDroppedOutput = { + shouldRetryDroppedOutputRecovery, + DROP_RECOVERY_DELAY_MS, + DROP_RECOVERY_MAX_ATTEMPTS, + }; window.CodemanTerminalGeometry = { clampTerminalDimensions, terminalGeometryAgrees, diff --git a/test/dropped-output-recovery.test.ts b/test/dropped-output-recovery.test.ts new file mode 100644 index 00000000..32014af3 --- /dev/null +++ b/test/dropped-output-recovery.test.ts @@ -0,0 +1,233 @@ +// Port: none (pure policy + the real scheduler from app.js under a fake clock). +// +// `_onSessionTerminal` drops an incoming frame once the app-owned render queues +// hold 128KB. That is the right call — the alternative is an unbounded backlog — +// but a hole in a TUI byte stream is a desynced cursor, and a desynced cursor is +// muffled text (issue #464). The drop is only half of it; the recovery has to +// actually happen. +// +// ⚠️ It used to be a fire-and-forget timer: it nulled its own handle and then +// called `_onSessionNeedsRefresh()`, which opens with four early returns. Two of +// them — a buffer load in flight, a refresh already owning this session — are +// MOST likely to be true during exactly the output burst that caused the drop, +// so the recovery was silently lost precisely when it was needed, and those +// bytes were never replayed. +// +// The test that matters here is `retries when the refresh was skipped`, paired +// with `does not retry once a repaint happened`. Either one alone would pass +// against the old fire-and-forget code; only the contrast pins the fix. +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import vm from 'node:vm'; +import { describe, expect, it, vi } from 'vitest'; + +const publicDir = resolve(import.meta.dirname, '../src/web/public'); +const read = (rel: string) => readFileSync(resolve(import.meta.dirname, '..', rel), 'utf8'); + +type RetryState = { repainted: boolean; attempt: number; stillActive: boolean }; + +function loadConstants() { + const context = vm.createContext({ window: {}, globalThis: {} }); + vm.runInContext(readFileSync(resolve(publicDir, 'constants.js'), 'utf8'), context, { filename: 'constants.js' }); + return ( + context.window as { + CodemanDroppedOutput: { + shouldRetryDroppedOutputRecovery: (s: RetryState) => boolean; + DROP_RECOVERY_DELAY_MS: number; + DROP_RECOVERY_MAX_ATTEMPTS: number; + }; + } + ).CodemanDroppedOutput; +} + +const { shouldRetryDroppedOutputRecovery, DROP_RECOVERY_MAX_ATTEMPTS, DROP_RECOVERY_DELAY_MS } = loadConstants(); + +describe('shouldRetryDroppedOutputRecovery', () => { + it('retries a recovery that did not repaint', () => { + expect(shouldRetryDroppedOutputRecovery({ repainted: false, attempt: 0, stillActive: true })).toBe(true); + }); + + it('stops as soon as something repainted', () => { + expect(shouldRetryDroppedOutputRecovery({ repainted: true, attempt: 0, stillActive: true })).toBe(false); + }); + + it('stops when the reader has moved to another session', () => { + // selectSession repaints from the server on its own, so a retry here would + // be a second replay of a buffer that is about to be written anyway. + expect(shouldRetryDroppedOutputRecovery({ repainted: false, attempt: 0, stillActive: false })).toBe(false); + }); + + it('gives up at the cap rather than looping against the API forever', () => { + const last = DROP_RECOVERY_MAX_ATTEMPTS - 1; + expect(shouldRetryDroppedOutputRecovery({ repainted: false, attempt: last - 1, stillActive: true })).toBe(true); + expect(shouldRetryDroppedOutputRecovery({ repainted: false, attempt: last, stillActive: true })).toBe(false); + }); +}); + +// ─────────────────────────────────────────────────────────────────────────── +// The real scheduler, from app.js, under a fake clock. +// ─────────────────────────────────────────────────────────────────────────── + +function loadAppPrototype(): Record { + const context = vm.createContext({ + console: { ...console, log: vi.fn(), warn: vi.fn(), error: vi.fn() }, + performance: { now: () => 0 }, + setInterval: vi.fn(), + clearInterval: vi.fn(), + // ⚠️ Delegated, not captured. Baking the real `setTimeout` into the context + // puts the scheduler on a clock `vi.useFakeTimers()` cannot reach, and the + // retry behaviour under test is entirely a matter of timers firing. These + // arrows resolve the identifier from the global at CALL time, so the fake + // clock installed later still owns them. + setTimeout: (fn: () => void, ms?: number) => setTimeout(fn, ms), + clearTimeout: (id: ReturnType) => clearTimeout(id), + requestAnimationFrame: vi.fn(), + HTMLCanvasElement: class HTMLCanvasElement {}, + WebSocket: { OPEN: 1 }, + fetch: vi.fn(), + document: { addEventListener: vi.fn(), getElementById: () => null, querySelector: () => null }, + localStorage: { length: 0, key: vi.fn(), getItem: vi.fn(), setItem: vi.fn(), removeItem: vi.fn() }, + window: { addEventListener: vi.fn(), removeEventListener: vi.fn() }, + MobileDetection: { isTouchDevice: () => false }, + }); + vm.runInContext( + `${readFileSync(resolve(publicDir, 'constants.js'), 'utf8')}\n` + + `${readFileSync(resolve(publicDir, 'app.js'), 'utf8')}\n` + + `globalThis.__CodemanApp = CodemanApp;`, + context + ); + return (context as { __CodemanApp: { prototype: Record } }).__CodemanApp.prototype; +} + +const proto = loadAppPrototype(); +const SESSION = 'session-A'; + +/** A minimal app carrying only what the scheduler touches. */ +function makeApp(refresh: () => unknown) { + const calls: string[] = []; + return { + calls, + app: { + _scheduleDroppedOutputRecovery: proto._scheduleDroppedOutputRecovery, + activeSessionId: SESSION, + _clientDropRecoveryTimer: null as ReturnType | null, + _onSessionNeedsRefresh: (arg: { id: string }) => { + calls.push(arg.id); + return refresh(); + }, + } as unknown as { + _scheduleDroppedOutputRecovery: (id: string, attempt?: number) => void; + activeSessionId: string | null; + _clientDropRecoveryTimer: unknown; + }, + }; +} + +/** Run every pending timer the scheduler laid down, up to `rounds` deep. */ +async function drain(rounds = DROP_RECOVERY_MAX_ATTEMPTS + 2) { + for (let i = 0; i < rounds; i++) { + await vi.advanceTimersByTimeAsync(DROP_RECOVERY_DELAY_MS + 1); + } +} + +describe('_scheduleDroppedOutputRecovery', () => { + it('retries when the refresh was SKIPPED, which is what a burst makes likely', async () => { + vi.useFakeTimers(); + try { + // `_onSessionNeedsRefresh` returns false from all four of its early + // returns — a buffer load in flight, a refresh already owning the session. + const { app, calls } = makeApp(() => Promise.resolve(false)); + app._scheduleDroppedOutputRecovery(SESSION); + await drain(); + expect(calls.length, 'a skipped refresh must be tried again — the old fire-and-forget timer stopped at one').toBe( + DROP_RECOVERY_MAX_ATTEMPTS + ); + expect(new Set(calls)).toEqual(new Set([SESSION])); + } finally { + vi.useRealTimers(); + } + }); + + // The contrast. Without this the case above is satisfied by retrying forever. + it('does not retry once a repaint actually happened', async () => { + vi.useFakeTimers(); + try { + const { app, calls } = makeApp(() => Promise.resolve(true)); + app._scheduleDroppedOutputRecovery(SESSION); + await drain(); + expect(calls.length).toBe(1); + } finally { + vi.useRealTimers(); + } + }); + + it('stops when the reader switches away mid-recovery', async () => { + vi.useFakeTimers(); + try { + const { app, calls } = makeApp(() => { + app.activeSessionId = 'session-B'; + return Promise.resolve(false); + }); + app._scheduleDroppedOutputRecovery(SESSION); + await drain(); + expect(calls.length, 'selectSession repaints session-B on its own').toBe(1); + } finally { + vi.useRealTimers(); + } + }); + + it('treats a refresh that THREW as not repainted, and tries again', async () => { + vi.useFakeTimers(); + try { + const { app, calls } = makeApp(() => Promise.reject(new Error('network'))); + app._scheduleDroppedOutputRecovery(SESSION); + await drain(); + expect(calls.length).toBe(DROP_RECOVERY_MAX_ATTEMPTS); + } finally { + vi.useRealTimers(); + } + }); + + it('coalesces a burst of drops into one attempt, as the debounce always did', async () => { + vi.useFakeTimers(); + try { + const { app, calls } = makeApp(() => Promise.resolve(true)); + for (let i = 0; i < 20; i++) app._scheduleDroppedOutputRecovery(SESSION); + await drain(); + expect(calls.length, 'twenty dropped frames must not become twenty fetches').toBe(1); + } finally { + vi.useRealTimers(); + } + }); +}); + +describe('the drop path and the refresh agree on what counts as recovered', () => { + const app = read('src/web/public/app.js'); + + it('the 128KB drop goes through the scheduler, not a bare timer', () => { + const at = app.indexOf('131072'); + expect(at, 'the cap is gone — renamed?').toBeGreaterThan(-1); + const branch = app.slice(at, at + 600); + expect(branch).toContain('this._scheduleDroppedOutputRecovery(data.id)'); + expect(branch, 'a bare setTimeout here is the bug this fixes').not.toContain('setTimeout'); + }); + + it('_onSessionNeedsRefresh reports false from every early return', () => { + const start = app.indexOf('async _onSessionNeedsRefresh(event = {})'); + expect(start).toBeGreaterThan(-1); + const head = app.slice(start, app.indexOf('const refreshOwner', start)); + const bareReturns = head.match(/\breturn;/g) ?? []; + expect(bareReturns, 'a bare `return` reads as undefined, which the caller cannot tell from false').toHaveLength(0); + expect((head.match(/return false;/g) ?? []).length).toBeGreaterThanOrEqual(4); + }); + + it('and reports true only where it settles the reconcile marker', () => { + const start = app.indexOf('async _onSessionNeedsRefresh(event = {})'); + const body = app.slice(start, app.indexOf('\n }\n', app.indexOf('needsRefresh reload failed', start))); + const markerAt = body.indexOf('this._markTerminalBufferReconciled(sessionId);'); + const trueAt = body.indexOf('return true;'); + expect(markerAt).toBeGreaterThan(-1); + expect(trueAt, 'the success return must sit with the marker it settles').toBeGreaterThan(markerAt); + expect(trueAt - markerAt).toBeLessThan(80); + }); +});