diff --git a/CLAUDE.md b/CLAUDE.md index 08f59a8e..11d65e47 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. ⚠️ **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) +**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 the early returns it retries past are transient contention. A refresh that died at the capture fetch DEADLINE (it returns `'deadline'`) is a stalled link, not contention, and is NOT retried: each retry would be another `?full=1` capture waiting out a deadline of up to two minutes. The same debounce still collapses a burst into one attempt, and the `TERMINAL DROP` crash-trail line sits behind it, since one line per dropped frame evicted the whole 50-entry trail in under a second. 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 1849aa99..8d521b68 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -2068,9 +2068,11 @@ class CodemanApp { + (this._terminalWriteInFlightBytes || 0); if (queued + data.data.length > 131072) { // 128KB — drop to prevent accumulation // 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); + // thing that puts this terminal back in step with the PTY. It also + // writes the crash-trail line, once per debounce window: logged here, + // one line per dropped frame evicted the whole 50-entry trail in + // under a second. + this._scheduleDroppedOutputRecovery(data.id, 0, queued); return; } @@ -2091,25 +2093,34 @@ class CodemanApp { * * 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. + * `DROP_RECOVERY_MAX_ATTEMPTS`, because the early returns it retries past + * are transient contention. A refresh that died at the fetch DEADLINE is + * not retried: that is a stalled link, not contention, and each retry would + * be another `?full=1` capture waiting out a deadline of up to two minutes. + * Giving up 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 + * @param {number} [queuedBytes] - render-queue bytes at the drop, for the crash trail */ - _scheduleDroppedOutputRecovery(sessionId, attempt = 0) { + _scheduleDroppedOutputRecovery(sessionId, attempt = 0, queuedBytes) { if (!sessionId || this._clientDropRecoveryTimer) return; + // Behind the debounce guard: one line per window, not per dropped frame. + if (Number.isFinite(queuedBytes)) _crashDiag.log(`TERMINAL DROP: ${(queuedBytes / 1024).toFixed(0)}KB queued`); this._clientDropRecoveryTimer = setTimeout(async () => { this._clientDropRecoveryTimer = null; let repainted = false; + let timedOut = false; try { - repainted = (await this._onSessionNeedsRefresh({ id: sessionId })) === true; + const result = await this._onSessionNeedsRefresh({ id: sessionId }); + repainted = result === true; + timedOut = result === 'deadline'; } catch { // Treated as "did not repaint" — retrying is the entire point of this. } const retry = window.CodemanDroppedOutput.shouldRetryDroppedOutputRecovery({ repainted, + timedOut, attempt, stillActive: this.activeSessionId === sessionId, }); @@ -2757,7 +2768,9 @@ class CodemanApp { * "recovered" silently loses the recovery; `_scheduleDroppedOutputRecovery` * is the one that cannot afford to. * - * @returns {Promise} true only once a response has been applied. + * @returns {Promise} true only once a response has been + * applied; 'deadline' when the capture fetch hit its deadline (a stalled + * link, which the dropped-output scheduler does not retry); false otherwise. */ async _onSessionNeedsRefresh(event = {}) { // Server sends this after SSE backpressure clears — terminal data was dropped, @@ -2846,7 +2859,7 @@ class CodemanApp { return true; } catch (err) { console.error('needsRefresh reload failed:', err); - return false; + return err?.name === 'AbortError' ? 'deadline' : false; } finally { if (this._terminalRefreshOwner === refreshOwner) this._terminalRefreshOwner = null; } diff --git a/src/web/public/constants.js b/src/web/public/constants.js index 4855fc3d..15f24368 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -1713,27 +1713,32 @@ function sanitizeDiagEntry(msg) { // 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. +// Bounded, because the early returns it retries past are transient contention +// that clears in seconds, and a permanently failing refresh must not become a +// forever-loop against the API. A refresh that hit the capture fetch DEADLINE +// is not contention but a stalled link, and is not retried at all: each retry +// would be another `?full=1` capture waiting out a deadline of up to two +// minutes, where the old code cost exactly one. Giving up after the cap leaves +// exactly the garbled frames the old code left, so the floor is no worse. 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 + * @param {{repainted: boolean, timedOut?: boolean, attempt: number, stillActive: boolean}} state * `repainted` — whether `_onSessionNeedsRefresh` actually rewrote the buffer. + * `timedOut` - whether it failed at the capture fetch deadline. * `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 }) { +function shouldRetryDroppedOutputRecovery({ repainted, timedOut = false, 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; + if (timedOut) return false; return attempt + 1 < DROP_RECOVERY_MAX_ATTEMPTS; } diff --git a/test/dropped-output-recovery.test.ts b/test/dropped-output-recovery.test.ts index 32014af3..140e0391 100644 --- a/test/dropped-output-recovery.test.ts +++ b/test/dropped-output-recovery.test.ts @@ -24,7 +24,7 @@ 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 }; +type RetryState = { repainted: boolean; timedOut?: boolean; attempt: number; stillActive: boolean }; function loadConstants() { const context = vm.createContext({ window: {}, globalThis: {} }); @@ -57,6 +57,14 @@ describe('shouldRetryDroppedOutputRecovery', () => { expect(shouldRetryDroppedOutputRecovery({ repainted: false, attempt: 0, stillActive: false })).toBe(false); }); + it('does not retry a refresh that died at the fetch deadline', () => { + // A stalled link, not contention: each retry would be another full capture + // waiting out a deadline of up to two minutes. + expect(shouldRetryDroppedOutputRecovery({ repainted: false, timedOut: true, attempt: 0, stillActive: true })).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); @@ -93,12 +101,14 @@ function loadAppPrototype(): Record { vm.runInContext( `${readFileSync(resolve(publicDir, 'constants.js'), 'utf8')}\n` + `${readFileSync(resolve(publicDir, 'app.js'), 'utf8')}\n` + - `globalThis.__CodemanApp = CodemanApp;`, + `globalThis.__CodemanApp = CodemanApp;\nglobalThis.__crashDiag = _crashDiag;`, context ); + crashTrail = (context as { __crashDiag: { _entries: string[] } }).__crashDiag._entries; return (context as { __CodemanApp: { prototype: Record } }).__CodemanApp.prototype; } +let crashTrail: string[] = []; const proto = loadAppPrototype(); const SESSION = 'session-A'; @@ -188,6 +198,33 @@ describe('_scheduleDroppedOutputRecovery', () => { } }); + it('does not retry a refresh that hit the fetch deadline', async () => { + vi.useFakeTimers(); + try { + const { app, calls } = makeApp(() => Promise.resolve('deadline')); + app._scheduleDroppedOutputRecovery(SESSION); + await drain(); + expect(calls.length, 'a stalled link is not contention; one full capture is the old cost').toBe(1); + } finally { + vi.useRealTimers(); + } + }); + + it('writes one crash-trail line per debounce window, not one per dropped frame', async () => { + vi.useFakeTimers(); + try { + const { app } = makeApp(() => Promise.resolve(true)); + const before = crashTrail.filter((e) => e.includes('TERMINAL DROP')).length; + const schedule = app._scheduleDroppedOutputRecovery as (id: string, attempt?: number, queued?: number) => void; + for (let i = 0; i < 125; i++) schedule.call(app, SESSION, 0, 200 * 1024); + const drops = crashTrail.filter((e) => e.includes('TERMINAL DROP')).length - before; + expect(drops, 'one second of 8ms frames used to evict the whole 50-entry trail').toBe(1); + await drain(); + } finally { + vi.useRealTimers(); + } + }); + it('coalesces a burst of drops into one attempt, as the debounce always did', async () => { vi.useFakeTimers(); try { @@ -208,7 +245,9 @@ describe('the drop path and the refresh agree on what counts as recovered', () = 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).toContain('this._scheduleDroppedOutputRecovery(data.id, 0, queued)'); + // The crash-trail line belongs behind the scheduler's debounce guard. + expect(branch).not.toContain('_crashDiag.log'); expect(branch, 'a bare setTimeout here is the bug this fixes').not.toContain('setTimeout'); });