From a15cfc49a7bdc060937699997585071648472e37 Mon Sep 17 00:00:00 2001 From: Rounak Datta Date: Tue, 15 Sep 2026 17:18:21 +0530 Subject: [PATCH 1/2] feat(terminal): renderer watchdog, atomic replay clear, fetch deadlines, reconnect recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four ways the terminal can silently stop being correct — in each case the buffer keeps updating, nothing throws, and the only recourse is a reload. 1. Renderer freeze after backgrounding. iOS DISCARDS scheduled rAF callbacks when a PWA backgrounds, and xterm's RenderDebouncer only clears its `_animationFrame` handle from inside that callback — so one drop leaves it permanently set and every later refresh() early-returns. Parsing is decoupled from rendering, so bytes keep filling the buffer correctly while nothing paints. Codeman has exactly ONE xterm for the whole page load, so a single backgrounding wedges it until a reload. Adds a 2s liveness poll and `_kickRenderer()`, which does what the dropped `_innerRefresh` would have. 2. Replay clears raced live output. xterm's write() is async-queued while reset() is synchronous and, per upstream, "does not clear input buffers and does not reset the parser" — so bytes queued before a reset are parsed after it and fuse into the snapshot. Verified against the real xterm 6 here: write('p8'); reset(); write('rmissions') renders "p8rmissions". The main path was already safe via a queued erase; the needsRefresh and clearTerminal paths were not. All three now share one queued `\x1bc` (RIS), which unlike 3J/H/2J also resets modes, charsets, scroll regions and SGR state. 3. Output lost on WebSocket reconnect. Input frames carry seq+cid and are delivered exactly once; output frames carry nothing. ws.onopen re-sends dims and flushes queued input, and needsRefresh only fires on external-CLI startup and SSE backpressure drain — never on reconnect. Output produced while offline was simply absent afterwards. Interim fix: reaching onclose means the drop was unintentional, so the session is marked and the next open reconciles from the server buffer. Sequencing output is the follow-up. 4. Terminal captures had no deadline. No AbortController anywhere in the frontend, including `?full=1`, which the code itself calls "unbounded-ish work: at the default history limit it can be megabytes". Adds a budget that scales with full-vs-tail and with captures in flight, degrading to a plain fetch where AbortController is missing. Also: the service-worker precache was dead — the build content-hashes assets but sw.js listed pre-hash names, so 15 of 23 entries 404'd (verified against a running instance) and cache.add().catch() hid it. Offline still worked via runtime caching, but CACHE_NAME was a constant so activate's cleanup never deleted anything and every past release's assets accumulated. Both are now derived from the build manifest. Crash-trail entries are flattened and capped, since they are joined with \n into one value and one call site interpolates a server-controlled WS close reason. The watchdog reads xterm privates — there is no public API. Every access is optional-chained so a shape change degrades to a no-op. `_renderService` only exists after open(), which needs a real DOM, so the gate cannot assert the field path; test/xterm-private-api.test.ts pins the dependency range instead. Tests: 23 new (terminal-resilience, sw-precache-manifest, xterm-private-api), all pure/static so they run in the gate, which excludes the mobile suite. One static source guard in history-truncation-notice updated for the renamed call; the behaviour it pins is unchanged. Not verified: no browser available, so no runtime reproduction of the freeze and no real-device test of the reconnect path. Both warrant a device pass. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 2 + scripts/build.mjs | 34 ++++++ src/web/public/app.js | 152 +++++++++++++++++++++--- src/web/public/constants.js | 121 +++++++++++++++++++ src/web/public/sw.js | 41 ++++--- src/web/public/terminal-ui.js | 106 +++++++++++++++++ test/history-truncation-notice.test.ts | 6 +- test/sw-precache-manifest.test.ts | 89 ++++++++++++++ test/terminal-resilience.test.ts | 154 +++++++++++++++++++++++++ test/xterm-private-api.test.ts | 69 +++++++++++ 10 files changed, 743 insertions(+), 31 deletions(-) create mode 100644 test/sw-precache-manifest.test.ts create mode 100644 test/terminal-resilience.test.ts create mode 100644 test/xterm-private-api.test.ts diff --git a/CLAUDE.md b/CLAUDE.md index 7ef88de48..45093c85f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -334,6 +334,8 @@ Frontend JS modules have `@fileoverview` with `@dependency`/`@loadorder` tags. L **Shell keyboard accessory bar + one-shot Ctrl** (issue #262, `keyboard-accessory.js`): a **shell**-mode session automatically swaps the mobile accessory bar for terminal controls (Ctrl, Esc, Tab, four arrows, paste, dismiss); every other mode keeps the agent bar. `setMode()` now records the user's `extendedKeyboardBar` preference as the **base** layout and `refreshForActiveSession()` (called from `selectSession`) resolves base-vs-shell, so a settings save during a shell session cannot yank the bar away and switching back restores the user's choice. ⚠️ **Ctrl is a ONE-SHOT modifier applied in `terminal.onData`, not in a keydown handler**: a virtual keyboard emits no usable key events, so the character only exists as onData text. The hook sits AFTER `shouldSuppressTerminalQueryResponse` (xterm answers DA/CPR through onData too, and one of those would silently spend the modifier) and BEFORE every send path, so the control byte follows the normal control-char route. ⚠️ **Not every onData chunk is a keystroke**, and the query filter is not enough on its own: xterm ALSO emits mouse and focus reports on its own initiative, so the hook skips them via `isTerminalFocusOrMouseReport()` (they still reach the PTY, they just don't count as the next key). The mouse half is live — a shell session keeps the NARROW strip, so mouse DECSETs reach the browser and one tap while vim/htop runs spent the armed modifier silently (measured). The focus half is defense in depth: `FOCUS_ESCAPE_FILTER` in `session.ts` strips `\x1b[?1004h` from every PTY read, so `sendFocusMode` never turns on today; if it ever did, the bar's own post-key refocus would emit `\x1b[I` and eat the modifier before the user typed. ⚠️ It must disarm on ALL of: use, second tap, any other accessory key, session switch, keyboard dismissal, and a layout swap; a modifier left armed turns the next innocent keystroke into a control byte. ⚠️ **onData is not the only input path** — with `cjkInputEnabled` on, the CJK textarea owns the keyboard (onData returns early for everything it swallows, and the focus router sends `terminal.focus()` there, which is where the bar refocuses after every key), so `_handleCjkInput()` applies the modifier too. It is that module's single choke point to the PTY, so one call covers typed characters, IME flushes, Enter, backspace and arrows. Without it an armed modifier could neither fire NOR be spent, and survived to a later keystroke. Mapping is `ctrlByteFor()` (`code & 0x1f` over @A-Z[\]^_ and a-z, plus Ctrl+Space=NUL / Ctrl+?=DEL); characters with no control equivalent pass through unchanged, like a hardware keyboard. ⚠️ The armed style is `.accessory-btn.accessory-btn-ctrl.armed` (0,3,0) in BOTH stylesheets, and it cannot outrank mobile.css's light-skin repaint at **(0,3,1)** (`:is()` inherits its most specific argument, and that list holds `.btn-toolbar.btn-shell`) — so that rule excludes the state by hand as `.accessory-btn:not(.armed)`. Without the exclusion the armed button renders identically to a resting one on all four light skins, which is worse than no armed style at all. +**Terminal resilience: replay clears, renderer liveness, fetch deadlines**: three rules that each close a way the terminal silently stops being correct, all of them measured rather than reasoned. ⚠️ **A replay clear MUST be in-stream, never `reset()`/`clear()`.** xterm's `write()` is asynchronously queued while `Terminal.reset()` is synchronous and, per upstream, "does not clear input buffers and does not reset the parser" — so bytes queued just before a reset are parsed AFTER it and fuse into the snapshot written next. Reproduced against the real xterm 6 in this repo: `write('p8'); reset(); write('rmissions')` renders `p8rmissions`. `_resetTerminalForReplay()` (app.js) is the ONE clear, a single queued `\x1bc` (RIS), and all three replay paths go through it; RIS rather than `\x1b[3J\x1b[H\x1b[2J` because the erase leaves modes, charsets, scroll regions and SGR state alone, so leftover bytes can park the terminal in alt-screen and survive it. Callers may still chunk the content — ordering in the queue is what matters, not writing it in one call. ⚠️ **The renderer watchdog reaches into xterm privates and CANNOT be covered by the gate.** iOS discards scheduled rAF callbacks when a PWA backgrounds, and xterm's `RenderDebouncer` only clears `_animationFrame` from inside that callback — one drop leaves it permanently set and every later `refresh()` early-returns, so the buffer keeps updating correctly while nothing paints. Codeman has exactly ONE xterm for the whole page load, so a single backgrounding wedges it until a reload. `_kickRenderer()` (terminal-ui.js) cancels the stale handle and forces a repaint; `_renderService` only exists after `open()`, which needs a real DOM, so `test/xterm-private-api.test.ts` pins the dependency RANGE instead and a major bump means re-verifying the field path by hand in a browser. Every access is optional-chained on purpose: a renamed field must degrade to a no-op, never throw on a 2s timer. ⚠️ **Every terminal capture carries a deadline** (`_fetchTerminalCapture`, app.js). A `?full=1` body can be megabytes and used to hang on the browser default with no retry; the budget scales with full-vs-tail and with captures already in flight, so several tabs resuming do not all expire together. It degrades to a plain fetch where `AbortController` is missing — the deadline is a safety net, not a dependency. Tests: `test/terminal-resilience.test.ts` (pure decisions), `test/sw-precache-manifest.test.ts`, `test/xterm-private-api.test.ts`. + **Dismissing the on-screen keyboard** (PRs #279/#280, `terminal-ui.js`): the terminal parks focus on a hidden textarea that nothing used to release, so TWO gestures now blur it, and they own different regions. **(1)** `_installMobileKeyboardDismiss()` — a document-level `touchend` that fires only while the terminal input actually holds focus, **never inside `#terminalContainer`** (tap classification owns that) and **never on a control** (`MOBILE_KEYBOARD_DISMISS_EXEMPT_SELECTOR`, matched with `closest()` so an icon inside a button counts). Session tabs are covered by the selector's `[tabindex]:not([tabindex="-1"])` arm, which is what stops a tab tap from blurring and then being re-focused by `selectSession()`. **(2)** In `_handleMobileTerminalTap`, a second tap on **inert `content`** (`startedWithTerminalFocus`) blurs instead of re-focusing. ⚠️ Scoped to `content` on purpose: the prompt row (`input`) keeps focus-then-position so a second tap still places the caret, and actionable rows blur earlier via `_isActionableMobileTerminalTap`. ⚠️ **A scroll ends in `touchend` too** — dismissing there closes the keyboard and drops the composer mid-read, so travel is tracked from `touchstart` and multi-touch is never a tap. Both classifiers MUST share one threshold: `initTerminal`'s `TAP_THRESHOLD` reads `MOBILE_KEYBOARD_DISMISS_TAP_SLOP`, since a gesture the terminal calls a scroll and the dismiss handler calls a tap is exactly that bug. ⚠️ **The gate excludes `test/mobile/**`, so CI cannot see the only test covering (1)** — run `npm run test:mobile -- test/mobile/keyboard.test.ts` by hand and diff the FAIL list against master. (Not `npm test --`: the gate's config excludes that path, so a file filter pointing into it matches nothing and exits green having run zero tests.) That blind spot is why merging the two PRs, which conflicted semantically but not textually, produced a red suite with two green CI checks. **Phone toolbar: Enter replaces Shell** (post-1.8.0): inside `@media (max-width: 599px)` `btn-shell` is `display:none` and `btn-enter` takes its slot (`order: 4`); starting a shell moved into the Run dropdown (`Terminal / Shell` → `setRunMode('shell')` → `run()` → `runShell()`, button label "Run SH"). `runMode` is `z.string().max(20)` server-side, so new modes need no schema change. Desktop and tablet keep the green Run Shell button unchanged. diff --git a/scripts/build.mjs b/scripts/build.mjs index 1fbb34d85..2ab8ecd5a 100644 --- a/scripts/build.mjs +++ b/scripts/build.mjs @@ -146,10 +146,44 @@ console.log('\n[build] content-hash cache busting'); html = html.replaceAll(`"${original}"`, `"${hashed}"`); } writeFileSync(join(distPublic, 'index.html'), html); + + // Rewrite sw.js from the SAME manifest that just renamed the files. + // + // The service worker's precache list used to be maintained by hand with the + // pre-hash names, so after this step every entry in it pointed at a file that + // no longer existed and `cache.add(...).catch(() => {})` hid it. Deriving it + // here is the only way the two cannot drift. + // + // The cache key gets the build hash for the same reason: `activate` deletes + // every cache that is not the current one, so a constant key meant that + // cleanup never ran and hashed assets from every past release piled up. + const swPath = join(distPublic, 'sw.js'); + let sw = readFileSync(swPath, 'utf8'); + const hashedAssets = Object.values(manifest); + const buildId = createHash('md5').update(hashedAssets.join('|')).digest('hex').slice(0, 12); + // Rewrite the two declarations. Anchored on the full `const … = …;` text so + // each pattern occurs exactly once and cannot collide with prose in sw.js's + // own comments — an earlier cut used bare `__BUILD_ID__` sentinels and the + // first match landed in the comment that documented them, leaving the real + // constant untouched and still producing a plausible-looking cache key. + const swEdits = [ + ["const BUILD_ID = 'dev';", `const BUILD_ID = '${buildId}';`], + ['const HASHED_ASSETS = [];', `const HASHED_ASSETS = [${hashedAssets.map((p) => JSON.stringify(p)).join(', ')}];`], + ]; + for (const [from, to] of swEdits) { + const hits = sw.split(from).length - 1; + if (hits !== 1) { + throw new Error(`sw.js: expected exactly one \`${from}\`, found ${hits} — precache would ship stale`); + } + sw = sw.replace(from, to); + } + writeFileSync(swPath, sw); + console.log(' Hashed files:'); for (const [orig, hashed] of Object.entries(manifest)) { console.log(` ${orig} -> ${hashed}`); } + console.log(` sw.js: cache bucket codeman-${buildId}, ${hashedAssets.length} precached assets`); } // 6. Compress with gzip + brotli diff --git a/src/web/public/app.js b/src/web/public/app.js index 4c6888997..5d2155809 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -66,7 +66,18 @@ const _crashDiag = { // concurrent clients (desktop + phone) don't clobber each other. _pageId: Date.now().toString(36) + '-' + Math.random().toString(36).slice(2, 8), log(msg) { - const entry = `${new Date().toISOString().slice(11,23)} ${msg}`; + // Entries are joined with '\n' into ONE localStorage value and beaconed to + // the server, and some call sites interpolate text this client does not + // control (a WebSocket close `reason` arrives from the server). A newline + // in there forges extra entries in the trail; an unbounded string can fill + // the storage quota and silently kill every later breadcrumb. Flatten and + // cap. CodemanDiag is loaded before app.js, but guard anyway — a + // diagnostic that can throw is worse than no diagnostic. + const flat = + typeof CodemanDiag !== 'undefined' && CodemanDiag.sanitizeDiagEntry + ? CodemanDiag.sanitizeDiagEntry(msg) + : String(msg == null ? '' : msg).replace(/[\r\n\u2028\u2029]+/g, ' ').slice(0, 300); + const entry = `${new Date().toISOString().slice(11,23)} ${flat}`; this._entries.push(entry); if (this._entries.length > this._maxEntries) this._entries.shift(); try { localStorage.setItem('codeman-crash-diag', this._entries.join('\n')); } catch {} @@ -675,6 +686,10 @@ class CodemanApp { this._wsReady = false; // True when WS is open and ready for I/O this._wsState = 'disconnected'; // connecting | connected | reconnecting | fallback | disconnected this._wsLastRecvAt = 0; // ms timestamp of the last frame received on the active WS + // Session whose socket dropped unintentionally, so output produced during + // the outage is missing from its buffer. Output frames carry no sequence + // number, so the only recovery is to refetch on the next successful open. + this._wsOutputGapSession = null; // Terminal write batching with DEC 2026 sync support this.pendingWrites = []; @@ -2475,6 +2490,61 @@ class CodemanApp { } } + /** + * Fetch a terminal capture under a deadline. + * + * Every terminal fetch used to run with no timeout at all, including + * `?full=1`, which _maybeRefetchFullHistory itself calls "unbounded-ish work: + * at the default history limit it can be megabytes". On a stalled mobile link + * that request hangs on the browser default with no retry, and the load-state + * machinery stays armed behind it. + * + * The budget scales with what is being asked for and with how many captures + * are already running (see CodemanFetchDeadline): a full scrollback on a slow + * uplink legitimately needs longer than a tail, and eight tabs resuming must + * not all expire together because each assumed it had the link to itself. + * + * An abort surfaces as a rejected fetch, which every caller already handles — + * they wrap these in try/catch and log. That is the point: a timeout becomes a + * recoverable error instead of an indefinite hang. + * + * @param {string} url + * @param {{full?: boolean}} [opts] + * @returns {Promise} + */ + async _fetchTerminalCapture(url, opts = {}) { + const deadlineMs = + typeof CodemanFetchDeadline !== 'undefined' + ? CodemanFetchDeadline.terminalFetchDeadlineMs({ + full: !!opts.full, + inflight: this._terminalCaptureInflight || 0, + }) + : 45000; + // AbortSignal.timeout() is not on every browser Codeman supports, so drive + // it from a controller and always clear the timer — an uncancelled one + // would abort a LATER request that reused this controller's signal. + // + // Degrade to a plain fetch where AbortController is missing rather than + // throwing: a capture with no deadline is the behaviour every caller had + // before this helper existed, while a ReferenceError here would take out + // terminal replay entirely. The deadline is a safety net, not a dependency. + const canAbort = typeof AbortController === 'function'; + const controller = canAbort ? new AbortController() : null; + const timer = controller ? setTimeout(() => controller.abort(), deadlineMs) : null; + this._terminalCaptureInflight = (this._terminalCaptureInflight || 0) + 1; + try { + return await (controller ? fetch(url, { signal: controller.signal }) : fetch(url)); + } catch (err) { + if (err?.name === 'AbortError') { + _crashDiag.log(`TERMINAL FETCH TIMEOUT after ${deadlineMs}ms`); + } + throw err; + } finally { + if (timer !== null) clearTimeout(timer); + this._terminalCaptureInflight = Math.max(0, (this._terminalCaptureInflight || 1) - 1); + } + } + async _onSessionNeedsRefresh(event = {}) { // Server sends this after SSE backpressure clears — terminal data was dropped, // so reload the buffer to recover from any display corruption. @@ -2493,14 +2563,15 @@ class CodemanApp { // TUI modes still recover the whole picture, with the downgrade guard for // repaint-mode panes whose tmux capture can be smaller than xterm's buffer. const useFullHistory = this.sessions.get(sessionId)?.mode !== 'shell'; - let res = await fetch( + let res = await this._fetchTerminalCapture( useFullHistory ? `/api/sessions/${sessionId}/terminal?full=1` - : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}` + : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`, + { full: useFullHistory } ); let data = (await res.json())?.data ?? {}; if (useFullHistory && data.terminalBuffer && this._replayWouldShrinkBuffer(data.terminalBuffer)) { - res = await fetch(`/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`); + res = await this._fetchTerminalCapture(`/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`); data = (await res.json())?.data ?? {}; } // Bail on a tab switch mid-fetch: writing here would paint this session's @@ -2514,8 +2585,11 @@ class CodemanApp { // meaningless across it — distance from the bottom is what survives. const before = this.terminal.buffer?.active; const linesFromBottom = before ? Math.max(0, (before.baseY || 0) - (before.viewportY || 0)) : 0; - this.terminal.clear(); - this.terminal.reset(); + // One queued clear, not clear()+reset(): both of those are synchronous + // and skip xterm's write queue, so live bytes still parsing would land + // after them and fuse into the buffer written below. See + // _resetTerminalForReplay. + this._resetTerminalForReplay(); await this.chunkedTerminalWrite(data.terminalBuffer); // A tail fetch can be partial, and the banner would otherwise keep // describing the pre-refresh buffer (#258). @@ -2551,11 +2625,12 @@ class CodemanApp { // Fetch buffer, clear terminal, write buffer, resize (no Ctrl+L needed) try { - const res = await fetch(`/api/sessions/${data.id}/terminal`); + const res = await this._fetchTerminalCapture(`/api/sessions/${data.id}/terminal`); const termData = (await res.json())?.data ?? {}; - this.terminal.clear(); - this.terminal.reset(); + // Queued clear — see _resetTerminalForReplay for why clear()+reset() + // cannot do this job. + this._resetTerminalForReplay(); if (termData.terminalBuffer) { // Strip any DEC 2026 markers and write raw content // (markers don't help here - this is a static buffer reload, not live Ink redraws) @@ -2875,6 +2950,18 @@ class CodemanApp { // Flush any durably-queued input over the fresh socket (covers frames a // prior half-open socket silently dropped, and input typed while offline). this._onWsReady(sessionId); + // Reconcile the output hole this drop left (see the ws.onclose note). + // Only after an unintentional close — a first connect has no gap, and + // refetching there would duplicate the buffer selectSession just wrote. + if (this._wsOutputGapSession === sessionId) { + this._wsOutputGapSession = null; + _crashDiag.log(`WS REOPEN: reconciling output gap for ${sessionId}`); + // Fire-and-forget: this is recovery, and a failure here must not stop + // the socket coming up. _onSessionNeedsRefresh already guards against + // running while a buffer load is in flight and against a tab switch + // landing this session's history in another session's terminal. + void this._onSessionNeedsRefresh({ id: sessionId }); + } } }; @@ -2922,6 +3009,22 @@ class CodemanApp { `WS CLOSE code=${event.code} reason=${event.reason || ''} action=${plan.action} attempts=${this._wsReconnectAttempts || 0}` ); + // Output frames carry no sequence number, so a socket that dropped left a + // hole in the terminal with nothing to replay it: ws.onopen re-sends dims + // and flushes queued INPUT, and `needsRefresh` only fires on external-CLI + // startup and on SSE backpressure drain — never here. Whatever the PTY + // produced while the link was down is simply absent from the buffer. + // + // Reaching onclose at all means the drop was NOT intentional + // (_disconnectWs nulls this handler first), so mark the gap and let the + // next successful open reconcile from the server's buffer. + // + // Scoped to the session that actually lost bytes: a user who switches + // sessions during an outage gets a clean intentional disconnect for the + // new one, and its freshly-loaded buffer must not be refetched because a + // DIFFERENT session's socket dropped. + this._wsOutputGapSession = sessionId; + const stillActive = this.activeSessionId === sessionId; if (plan.action === 'give-up') { this._wsState = stillActive ? 'fallback' : 'disconnected'; @@ -5745,9 +5848,29 @@ class CodemanApp { } } + /** + * Clear the terminal for a replay, IN STREAM. + * + * xterm's `write()` is asynchronously queued (the WriteBuffer parses in ~12ms + * slices) while `Terminal.reset()` is synchronous and, by upstream's own + * documentation, "does not clear input buffers and does not reset the parser, + * thus the terminal will continue to apply pending input data". So bytes + * queued just before a `reset()` are parsed AFTER it and fuse into whatever + * snapshot is written next — measured upstream as `p8rmissions` rendered + * where `bypass permissions` belonged. + * + * A queued clear cannot race that way: it lands after the leftovers and + * before the snapshot, whatever the queue held. This function used to follow + * the sync `reset()` with a queued `\x1b[3J\x1b[H\x1b[2J`, which already got + * that right for CONTENT. RIS (`\x1bc`) additionally resets modes, charsets, + * scroll regions and SGR state, so leftover bytes cannot park the terminal in + * alt-screen or an odd scroll region and survive the clear. + * + * Callers may write the replacement content in as many chunks as they like — + * ordering within the queue is what matters, not writing it all at once. + */ _resetTerminalForReplay() { - this.terminal.reset(); - this.terminal.write('\x1b[3J\x1b[H\x1b[2J'); + this.terminal.write('\x1bc'); } _recordTerminalLoadTiming(timing) { @@ -5811,7 +5934,7 @@ class CodemanApp { this._fullHistoryRepullInFlight = true; try { const requestStartedAt = performance.now(); - const res = await fetch(`/api/sessions/${sessionId}/terminal?full=1`); + const res = await this._fetchTerminalCapture(`/api/sessions/${sessionId}/terminal?full=1`, { full: true }); const headersReceivedAt = performance.now(); const payload = (await res.json())?.data ?? {}; const bodyParsedAt = performance.now(); @@ -6299,10 +6422,11 @@ class CodemanApp { const useFullHistory = session?.mode !== 'shell' && !this._fullHistoryLoaded.has(sessionId); if (useFullHistory) this._fullHistoryLoaded.add(sessionId); const fetchStartedAt = performance.now(); - const res = await fetch( + const res = await this._fetchTerminalCapture( useFullHistory ? `/api/sessions/${sessionId}/terminal?full=1` - : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}` + : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`, + { full: useFullHistory } ); const headersReceivedAt = performance.now(); if (this._isStaleSelect(selectGen)) { diff --git a/src/web/public/constants.js b/src/web/public/constants.js index be9780b04..3ca58bfc2 100644 --- a/src/web/public/constants.js +++ b/src/web/public/constants.js @@ -1442,8 +1442,129 @@ function terminalLogicalLine(buffer, row, cols, maxRows) { return { startRow, endRow, text, offsetToCell, cellToOffset }; } +// ── Renderer liveness ────────────────────────────────────────────────────── +// +// iOS DISCARDS scheduled requestAnimationFrame callbacks when a PWA goes to +// the background — not deferred, never delivered. xterm's RenderDebouncer only +// clears its `_animationFrame` handle from INSIDE that callback: +// +// refresh() { +// if (this._animationFrame !== undefined) return; // <- stale forever +// this._animationFrame = requestAnimationFrame(() => this._innerRefresh()); +// } +// _innerRefresh() { this._animationFrame = undefined; ... } // never runs +// +// So after one backgrounding the handle is permanently non-undefined and EVERY +// later render request returns on line one. Parsing is decoupled from +// rendering, so bytes keep filling the buffer correctly and nothing throws — +// the terminal is simply frozen. Closing and reopening fixes it because that +// constructs a new Terminal, and therefore a new debouncer. +// +// Codeman is MORE exposed than a per-session-terminal app: there is exactly one +// xterm instance for the whole page load, so a single backgrounding can wedge +// it until a full reload. +// +// This is the pure decision half. The signature that distinguishes this from +// every other way a terminal can look stuck is that bytes were WRITTEN and the +// element is VISIBLE, yet onRender has not fired since: +// +// frozen = wroteAt > renderedAt && now - wroteAt >= threshold && visible +// +// Deliberately NOT a "no output at all" check: a quiet terminal is the normal +// state and must never be kicked. And `visible` is required because a hidden +// terminal legitimately stops rendering (xterm pauses it), so kicking there +// would fire constantly on every backgrounded tab. +const RENDER_STALL_MS = 4000; + +// How often the watchdog checks. Deliberately coarse: the failure it catches is +// permanent until healed, so detecting it a second late costs nothing, while a +// tight interval would burn a wakeup per second on every idle phone. +const RENDER_LIVENESS_POLL_MS = 2000; + +/** + * Should the renderer be kicked? Pure so the CI gate can cover it — the DOM + * half (cancelling the stale handle) lives in terminal-ui.js. + * + * @param {{wroteAt:number, renderedAt:number, now:number, visible:boolean, + * thresholdMs?:number}} s + * @returns {boolean} + */ +function shouldKickRenderer(s) { + if (!s || !s.visible) return false; + const wroteAt = Number(s.wroteAt) || 0; + const renderedAt = Number(s.renderedAt) || 0; + const now = Number(s.now) || 0; + // Nothing written yet — a fresh terminal has no render to be missing. + if (wroteAt <= 0) return false; + // A render landed at or after the last write: the pipeline is alive. + if (renderedAt >= wroteAt) return false; + const threshold = Number.isFinite(s.thresholdMs) && s.thresholdMs > 0 ? s.thresholdMs : RENDER_STALL_MS; + return now - wroteAt >= threshold; +} + +// ── Fetch deadlines ──────────────────────────────────────────────────────── +// +// No terminal fetch carried any deadline, including `?full=1`, which the code +// itself describes as "unbounded-ish work: at the default history limit it can +// be megabytes". On a stalled mobile link that request hangs on the browser +// default with no retry and no path back to a usable terminal short of a +// reload. +// +// A single fixed timeout is wrong in both directions — too short for a full +// scrollback capture on a slow uplink, too long for a small tail on a dead +// connection. So the deadline is scaled by what is actually being asked for, +// and by how many captures are already in flight: on a slow link those bytes +// must drain before this request's own bytes start moving, and its timer is +// already running the whole time. +const FETCH_DEADLINE_TAIL_MS = 15000; +const FETCH_DEADLINE_FULL_MS = 45000; +const FETCH_DEADLINE_MAX_MS = 120000; + +/** + * Deadline in ms for a terminal capture. + * + * @param {{full?:boolean, inflight?:number}} s - `full` = the ?full=1 capture; + * `inflight` = captures already running (this one included or not, it only + * scales the budget). + * @returns {number} + */ +function terminalFetchDeadlineMs(s) { + const full = !!(s && s.full); + const base = full ? FETCH_DEADLINE_FULL_MS : FETCH_DEADLINE_TAIL_MS; + const inflight = Math.max(0, Number(s && s.inflight) || 0); + // Each already-queued capture gets the newcomer one more base budget to wait + // through. Linear rather than clever: the point is only that eight tabs + // resuming do not all time out together because each assumed it was alone. + return Math.min(FETCH_DEADLINE_MAX_MS, base * (1 + inflight)); +} + +// ── Diagnostics hygiene ──────────────────────────────────────────────────── +// +// The crash trail is joined with '\n' into ONE localStorage value and beaconed +// to the server, and at least one call site interpolates server-controlled text +// (a WebSocket close `reason`). An embedded newline there forges extra entries +// in the trail; an unbounded string can fill the storage quota. Both are cheap +// to close, and the trail is something a user may be asked to paste into an +// issue. +const DIAG_ENTRY_MAX_CHARS = 300; + +/** Flatten a diagnostic message to one bounded, newline-free line. */ +function sanitizeDiagEntry(msg) { + return String(msg == null ? '' : msg) + .replace(/[\r\n\u2028\u2029]+/g, ' ') + .slice(0, DIAG_ENTRY_MAX_CHARS); +} + if (typeof window !== 'undefined') { window.CodemanHistoryFormat = { formatHistoryBytes, computeHistoryTruncationNotice, computeRewriteScrollLine }; window.CodemanFilePaths = { absoluteFilePathPattern, previewsInFileViewer, FILE_PREVIEW_EXTENSIONS }; window.CodemanTerminalLines = { terminalLogicalLine }; + window.CodemanRenderLiveness = { shouldKickRenderer, RENDER_STALL_MS, RENDER_LIVENESS_POLL_MS }; + window.CodemanFetchDeadline = { + terminalFetchDeadlineMs, + FETCH_DEADLINE_TAIL_MS, + FETCH_DEADLINE_FULL_MS, + FETCH_DEADLINE_MAX_MS, + }; + window.CodemanDiag = { sanitizeDiagEntry, DIAG_ENTRY_MAX_CHARS }; } diff --git a/src/web/public/sw.js b/src/web/public/sw.js index efe17981d..6eda44df0 100644 --- a/src/web/public/sw.js +++ b/src/web/public/sw.js @@ -18,7 +18,16 @@ * @see src/push-store.ts -- server-side VAPID key management and subscription CRUD */ -const CACHE_NAME = 'codeman-v1'; +// Build identity. scripts/build.mjs rewrites this declaration after it content- +// hashes the assets; the literal below is what dev serves, and dev wants a +// stable key. +// +// Why the cache key MUST carry it: `activate` deletes every cache whose key is +// not the current one, so the old constant key meant that cleanup never deleted +// anything — hashed assets from every release ever deployed accumulated in one +// bucket until the origin hit its storage quota. +const BUILD_ID = 'dev'; +const CACHE_NAME = `codeman-${BUILD_ID}`; // Reverse-proxy base path: the worker is served at `/sw.js`, so its own // location tells us the mount prefix ('' at root, or '/codeman'). Every URL below @@ -27,27 +36,27 @@ const CACHE_NAME = 'codeman-v1'; const SW_BASE = self.location.pathname.replace(/\/sw\.js$/, ''); const B = (p) => (p && p[0] === '/' ? SW_BASE + p : p); +// Content-hashed assets. scripts/build.mjs rewrites this declaration with the +// filenames it actually emitted; dev has no hashing, so the empty literal below +// is correct there and the unhashed modules are simply cached on first use by +// the runtime handler further down. +// +// This list used to be maintained by hand with the PRE-hash names, which the +// build then renamed — so in production every entry 404'd and the silent +// `.catch()` in install swallowed all of it. Measured against a running +// instance: 15 of 23 entries failed. Offline still worked, because the fetch +// handler caches every successful GET at runtime, but the precache warmed +// nothing while looking like it did. Deriving it from the same manifest that +// renames the files is the only thing that keeps the two from drifting again. +const HASHED_ASSETS = []; + // Core app shell -- cached on install for instant startup const APP_SHELL = [ '/', - '/styles.css', - '/mobile.css', - '/constants.js', - '/app.js', - '/api-client.js', - '/terminal-ui.js', - '/session-ui.js', - '/settings-ui.js', - '/panels-ui.js', - '/notification-manager.js', - '/mobile-handlers.js', - '/keyboard-accessory.js', - '/voice-input.js', + ...HASHED_ASSETS.map((p) => '/' + p), '/vendor/xterm.min.js', '/vendor/xterm-addon-fit.min.js', '/vendor/xterm-addon-unicode11.min.js', - '/vendor/xterm-zerolag-input.js', - '/vendor/xterm-predictive-echo.js', '/vendor/xterm.css', '/icon-192.png', '/icon-512.png', diff --git a/src/web/public/terminal-ui.js b/src/web/public/terminal-ui.js index 7ecc994ca..a366be0fb 100644 --- a/src/web/public/terminal-ui.js +++ b/src/web/public/terminal-ui.js @@ -528,6 +528,14 @@ Object.assign(CodemanApp.prototype, { this.terminal.onRender(() => this._syncMobileHelperTextareaToCursor()); } + // Renderer liveness — see _startRenderLivenessWatchdog. Registered for every + // device, not just touch: the rAF-discard behaviour is worst on an iOS PWA + // but a stale handle wedges the debouncer identically anywhere it happens. + this.terminal.onRender(() => { + this._lastRenderAt = Date.now(); + }); + this._startRenderLivenessWatchdog(); + // CJK IME input — textarea in index.html, just wire up send this._cjkInput = null; if (typeof CjkInput !== 'undefined') { @@ -3290,7 +3298,105 @@ Object.assign(CodemanApp.prototype, { return performance.now() - this._lastUserScrollUpAt < window.CodemanTerminalInput.USER_SCROLL_STICKY_SUPPRESS_MS; }, + /** + * Watchdog for a frozen renderer. + * + * iOS DISCARDS scheduled requestAnimationFrame callbacks when a PWA goes to + * the background — not deferred, never delivered. xterm's RenderDebouncer + * only clears its `_animationFrame` handle from INSIDE that callback, so once + * one is dropped the handle stays permanently non-undefined and every later + * `refresh()` returns on its first line. Parsing is decoupled from rendering, + * so bytes keep filling the buffer correctly and nothing throws: the terminal + * is simply frozen until the page is reloaded. + * + * Codeman is more exposed than an app that mounts a terminal per session — + * there is exactly ONE xterm instance for the whole page load, so a single + * backgrounding can wedge it for the rest of the session. + * + * The heal is what `_innerRefresh` would have done: cancel the stale handle, + * clear the field, and request a full repaint (which schedules a fresh rAF). + * Cancelling a genuinely pending handle is harmless — the full repaint that + * follows covers whatever it was going to draw. + * + * Discipline for reaching into xterm privates, and it is not optional: every + * access is optional-chained and the whole body is wrapped, so a shape change + * upstream degrades to a no-op. A self-heal that can break the terminal it is + * healing is worse than no self-heal. + * + * ⚠️ The field path (`_core._renderService._renderDebouncer._animationFrame`) + * is validated against xterm 6.x and CANNOT be covered by the CI gate: + * `_renderService` is only constructed by `Terminal.open()`, which needs a + * real DOM, and the gate runs in node. `test/xterm-private-api.test.ts` pins + * the dependency RANGE instead, so a major bump fails there and sends someone + * to re-check this by hand; `test/terminal-resilience.test.ts` covers the + * decision half. If the path ever goes stale the watchdog silently stops + * healing — that is the failure mode to watch for, and why the range guard + * exists at all. + */ + _startRenderLivenessWatchdog() { + this._stopRenderLivenessWatchdog(); + this._lastRenderAt = Date.now(); + this._lastTerminalWriteAt = 0; + this._renderLivenessTimer = setInterval(() => { + try { + if (typeof CodemanRenderLiveness === 'undefined') return; + const kick = CodemanRenderLiveness.shouldKickRenderer({ + wroteAt: this._lastTerminalWriteAt || 0, + renderedAt: this._lastRenderAt || 0, + now: Date.now(), + // A hidden terminal legitimately stops rendering (xterm pauses it), + // so only a VISIBLE one that owes us a frame counts as frozen. + visible: document.visibilityState === 'visible' && !!this.terminal?.element?.isConnected, + }); + if (!kick) return; + const kicked = this._kickRenderer(); + _crashDiag.log(`RENDER STALL: kick=${kicked}`); + // Treat the kick as the render for accounting purposes either way, so a + // terminal we cannot heal logs once per stall rather than every tick. + this._lastRenderAt = Date.now(); + } catch { + /* a watchdog must never throw into the interval */ + } + }, RENDER_LIVENESS_POLL_MS); + }, + + _stopRenderLivenessWatchdog() { + if (this._renderLivenessTimer) { + clearInterval(this._renderLivenessTimer); + this._renderLivenessTimer = null; + } + }, + + /** + * Do what xterm's dropped `_innerRefresh` would have done. Never throws. + * @returns {boolean} true if a stale handle was found and cleared. + */ + _kickRenderer() { + try { + const renderService = this.terminal?._core?._renderService; + const debouncer = renderService?._renderDebouncer; + if (!debouncer || typeof renderService.refreshRows !== 'function') return false; + const handle = debouncer._animationFrame; + if (handle === undefined) return false; // not wedged — nothing to clear + try { + cancelAnimationFrame(handle); + } catch { + /* a stale handle may no longer be cancellable; clearing it is the point */ + } + debouncer._animationFrame = undefined; + renderService.refreshRows(0, Math.max(0, (this.terminal.rows || 1) - 1)); + return true; + } catch { + return false; + } + }, + batchTerminalWrite(data) { + // Feed the renderer watchdog. Recorded before the buffer-load early return + // below: a write that is queued rather than written still means the pipeline + // owes us a frame once it drains. + this._lastTerminalWriteAt = Date.now(); + // If a buffer load (chunkedTerminalWrite) is in progress, queue live events // to prevent interleaving historical buffer data with live SSE data. // This is critical: interleaving causes cursor position chaos with Ink redraws. diff --git a/test/history-truncation-notice.test.ts b/test/history-truncation-notice.test.ts index ef542eefc..b24fe3b67 100644 --- a/test/history-truncation-notice.test.ts +++ b/test/history-truncation-notice.test.ts @@ -127,7 +127,11 @@ describe('the in-terminal truncation line is gone (static guard)', () => { expect(app).toContain("session?.mode !== 'shell' && !this._fullHistoryLoaded.has(sessionId)"); expect(app).toContain("!restoredSnapshot && session?.mode !== 'shell'"); expect(app).toContain('`/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`'); - expect(app).toContain('fetch(`/api/sessions/${sessionId}/terminal?full=1`)'); + // Every terminal capture now goes through _fetchTerminalCapture, which adds + // an abort deadline (a `?full=1` body can be megabytes and used to hang + // indefinitely on a stalled mobile link). The URL and the full-vs-tail + // decision this guard exists to pin are unchanged. + expect(app).toContain('this._fetchTerminalCapture(`/api/sessions/${sessionId}/terminal?full=1`, { full: true })'); expect(app).toContain("if (this.sessions.get(sessionId)?.mode !== 'shell')"); expect(app).toContain("if (session?.mode === 'shell')"); expect(app).toContain("if (!force && session?.mode === 'shell') return;"); diff --git a/test/sw-precache-manifest.test.ts b/test/sw-precache-manifest.test.ts new file mode 100644 index 000000000..ee67aa7fb --- /dev/null +++ b/test/sw-precache-manifest.test.ts @@ -0,0 +1,89 @@ +// Port: none (static source contract — no browser, no server). +// +// The service worker's precache list used to be maintained by hand with the +// PRE-hash filenames, while scripts/build.mjs renamed those same files to +// content-hashed names and rewrote only index.html. So in production every +// precache entry pointed at a file that no longer existed, and +// `cache.add(url).catch(() => {})` in the install handler swallowed all of it. +// Measured against a running instance: 15 of 23 entries 404'd. +// +// Nothing caught it because nothing could: the two lists lived in different +// files, in different languages, with no shared symbol. The fix is to derive +// the list from the build's own manifest — and this test pins the contract that +// makes that derivation possible, because the failure mode is silent in both +// directions. A renamed anchor in sw.js means the build throws (loud, fine). A +// build that stops rewriting means the worker precaches nothing while still +// looking correct (silent, not fine). +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const root = resolve(import.meta.dirname, '..'); +const sw = readFileSync(resolve(root, 'src/web/public/sw.js'), 'utf8'); +const build = readFileSync(resolve(root, 'scripts/build.mjs'), 'utf8'); + +// The exact declarations scripts/build.mjs rewrites. They must appear EXACTLY +// once: the build asserts the same thing and throws otherwise, so a second +// occurrence (in a comment, say) fails the build rather than shipping stale. +const BUILD_ID_ANCHOR = "const BUILD_ID = 'dev';"; +const HASHED_ASSETS_ANCHOR = 'const HASHED_ASSETS = [];'; + +describe('service worker precache contract', () => { + it('sw.js carries exactly one of each anchor the build rewrites', () => { + expect(sw.split(BUILD_ID_ANCHOR).length - 1).toBe(1); + expect(sw.split(HASHED_ASSETS_ANCHOR).length - 1).toBe(1); + }); + + it('build.mjs rewrites those exact anchors', () => { + expect(build).toContain(BUILD_ID_ANCHOR); + expect(build).toContain(HASHED_ASSETS_ANCHOR); + }); + + // The cache key must vary per build, or `activate`'s cleanup — which deletes + // every cache whose key is not the current one — never deletes anything, and + // hashed assets from every past release accumulate until the origin hits its + // storage quota. That is what the old constant 'codeman-v1' did. + it('derives the cache name from the build id rather than a constant', () => { + expect(sw).toContain('const CACHE_NAME = `codeman-${BUILD_ID}`;'); + expect(sw).not.toMatch(/const CACHE_NAME = ['"]codeman-v\d+['"]/); + }); + + // The whole point of the rewrite: the shell is derived, not hand-listed. + it('builds the app shell from the hashed manifest', () => { + expect(sw).toContain("...HASHED_ASSETS.map((p) => '/' + p)"); + }); + + // The regression itself. These are the pre-hash names the build renames, so + // any of them appearing in the shell list means someone hand-added an entry + // that will 404 in production. + it('never hand-lists a filename the build content-hashes', () => { + const shell = sw.slice(sw.indexOf('const APP_SHELL'), sw.indexOf('].map(B);')); + const hashedByBuild = [ + 'app.js', + 'constants.js', + 'terminal-ui.js', + 'session-ui.js', + 'settings-ui.js', + 'panels-ui.js', + 'styles.css', + 'mobile.css', + 'i18n.js', + 'mobile-handlers.js', + 'keyboard-accessory.js', + 'notification-manager.js', + 'voice-input.js', + 'api-client.js', + 'vendor/xterm-zerolag-input.js', + 'vendor/xterm-predictive-echo.js', + ]; + for (const name of hashedByBuild) { + expect(shell, `APP_SHELL must not hand-list ${name} — the build renames it`).not.toContain(`'/${name}'`); + } + }); + + // Dev serves sw.js unrewritten, so the literals must be valid on their own: + // an empty precache plus the unhashed modules cached on first use. + it('is valid unrewritten, for dev', () => { + expect(() => new Function(sw.replace(/self\./g, 'globalThis.'))).not.toThrow(); + }); +}); diff --git a/test/terminal-resilience.test.ts b/test/terminal-resilience.test.ts new file mode 100644 index 000000000..71cc07ddc --- /dev/null +++ b/test/terminal-resilience.test.ts @@ -0,0 +1,154 @@ +// Port: none (pure helpers — no browser, no server). +// +// Three small decision functions behind the mobile terminal resilience work, +// pinned here because the code that consumes them lives in app.js / +// terminal-ui.js, which the CI gate cannot execute. Keeping the decision pure +// and the DOM half thin is what makes any of this testable without a browser. +// +// The renderer-liveness case is the one worth reading. iOS DISCARDS scheduled +// requestAnimationFrame callbacks when a PWA backgrounds — never delivered, not +// deferred — and xterm's RenderDebouncer only clears its `_animationFrame` +// handle from inside that callback. One drop leaves the handle permanently set, +// so every later refresh() returns immediately and the terminal freezes while +// its buffer keeps updating correctly. Codeman has exactly one xterm instance +// per page load, so a single backgrounding can wedge it until a reload. +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import vm from 'node:vm'; +import { describe, expect, it } from 'vitest'; + +function loadConstants() { + const context = vm.createContext({ window: {}, globalThis: {} }); + const source = readFileSync(resolve(import.meta.dirname, '../src/web/public/constants.js'), 'utf8'); + vm.runInContext(source, context, { filename: 'constants.js' }); + const w = context.window as { + CodemanRenderLiveness: { + shouldKickRenderer: (s: { + wroteAt: number; + renderedAt: number; + now: number; + visible: boolean; + thresholdMs?: number; + }) => boolean; + RENDER_STALL_MS: number; + RENDER_LIVENESS_POLL_MS: number; + }; + CodemanFetchDeadline: { + terminalFetchDeadlineMs: (s: { full?: boolean; inflight?: number }) => number; + FETCH_DEADLINE_TAIL_MS: number; + FETCH_DEADLINE_FULL_MS: number; + FETCH_DEADLINE_MAX_MS: number; + }; + CodemanDiag: { + sanitizeDiagEntry: (msg: unknown) => string; + DIAG_ENTRY_MAX_CHARS: number; + }; + }; + return w; +} + +describe('shouldKickRenderer', () => { + const { CodemanRenderLiveness } = loadConstants(); + const { shouldKickRenderer, RENDER_STALL_MS } = CodemanRenderLiveness; + + // The signature of the real failure: bytes were written, the element is + // visible, and no frame has been produced since. + it('kicks when a visible terminal owes a frame past the threshold', () => { + expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 500, now: 1000 + RENDER_STALL_MS, visible: true })).toBe( + true + ); + }); + + it('does not kick before the threshold elapses', () => { + expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 500, now: 1000 + RENDER_STALL_MS - 1, visible: true })).toBe( + false + ); + }); + + // A render at or after the last write means the pipeline is alive. This is + // the common case on every healthy terminal and must never kick. + it('does not kick when a render landed after the last write', () => { + expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 1000, now: 99_999, visible: true })).toBe(false); + expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 1200, now: 99_999, visible: true })).toBe(false); + }); + + // A hidden terminal legitimately stops rendering — xterm pauses it. Kicking + // there would fire on every backgrounded tab, forever. + it('never kicks a hidden terminal', () => { + expect(shouldKickRenderer({ wroteAt: 1000, renderedAt: 500, now: 99_999, visible: false })).toBe(false); + }); + + // A quiet terminal is the normal state, not a stalled one. Gating on "no + // render recently" instead of "owes a frame" would kick every idle session. + it('never kicks a terminal that has never been written to', () => { + expect(shouldKickRenderer({ wroteAt: 0, renderedAt: 0, now: 99_999, visible: true })).toBe(false); + }); + + it('tolerates missing and malformed input rather than throwing', () => { + expect(shouldKickRenderer(undefined as never)).toBe(false); + expect(shouldKickRenderer({} as never)).toBe(false); + expect(shouldKickRenderer({ wroteAt: NaN, renderedAt: NaN, now: NaN, visible: true } as never)).toBe(false); + }); + + it('polls coarsely enough not to wake an idle phone every second', () => { + expect(CodemanRenderLiveness.RENDER_LIVENESS_POLL_MS).toBeGreaterThanOrEqual(1000); + }); +}); + +describe('terminalFetchDeadlineMs', () => { + const { CodemanFetchDeadline } = loadConstants(); + const { terminalFetchDeadlineMs, FETCH_DEADLINE_TAIL_MS, FETCH_DEADLINE_FULL_MS, FETCH_DEADLINE_MAX_MS } = + CodemanFetchDeadline; + + // A full scrollback capture can be megabytes where a tail is one frame, so a + // single fixed timeout is wrong in both directions on a mobile link. + it('gives a full capture more budget than a tail', () => { + expect(terminalFetchDeadlineMs({ full: true })).toBeGreaterThan(terminalFetchDeadlineMs({ full: false })); + expect(terminalFetchDeadlineMs({ full: false })).toBe(FETCH_DEADLINE_TAIL_MS); + expect(terminalFetchDeadlineMs({ full: true })).toBe(FETCH_DEADLINE_FULL_MS); + }); + + // Eight tabs resuming must not all expire together because each assumed it + // had the link to itself. + it('scales with captures already in flight', () => { + const alone = terminalFetchDeadlineMs({ full: false, inflight: 0 }); + const queued = terminalFetchDeadlineMs({ full: false, inflight: 3 }); + expect(queued).toBeGreaterThan(alone); + }); + + it('is bounded — a stuck link still fails eventually', () => { + expect(terminalFetchDeadlineMs({ full: true, inflight: 1000 })).toBe(FETCH_DEADLINE_MAX_MS); + }); + + it('treats absent and nonsense input as a lone tail fetch', () => { + expect(terminalFetchDeadlineMs({})).toBe(FETCH_DEADLINE_TAIL_MS); + expect(terminalFetchDeadlineMs({ inflight: -5 } as never)).toBe(FETCH_DEADLINE_TAIL_MS); + expect(terminalFetchDeadlineMs({ inflight: NaN } as never)).toBe(FETCH_DEADLINE_TAIL_MS); + }); +}); + +describe('sanitizeDiagEntry', () => { + const { CodemanDiag } = loadConstants(); + const { sanitizeDiagEntry, DIAG_ENTRY_MAX_CHARS } = CodemanDiag; + + // The crash trail is joined with '\n' into one localStorage value and + // beaconed, and at least one call site interpolates a WebSocket close + // `reason`, which the server controls. A newline there forges entries. + it('collapses every newline form so an entry cannot forge another', () => { + expect(sanitizeDiagEntry('WS CLOSE reason=a\nFAKE ENTRY')).toBe('WS CLOSE reason=a FAKE ENTRY'); + expect(sanitizeDiagEntry('a\r\nb')).toBe('a b'); + expect(sanitizeDiagEntry('a
b
c')).toBe('a b c'); + }); + + it('bounds length so one entry cannot exhaust the storage quota', () => { + const out = sanitizeDiagEntry('x'.repeat(DIAG_ENTRY_MAX_CHARS * 3)); + expect(out).toHaveLength(DIAG_ENTRY_MAX_CHARS); + }); + + it('never throws on the values a diagnostic call site can actually pass', () => { + expect(sanitizeDiagEntry(null)).toBe(''); + expect(sanitizeDiagEntry(undefined)).toBe(''); + expect(sanitizeDiagEntry(42)).toBe('42'); + expect(sanitizeDiagEntry({ toString: () => 'obj' })).toBe('obj'); + }); +}); diff --git a/test/xterm-private-api.test.ts b/test/xterm-private-api.test.ts new file mode 100644 index 000000000..9b4024095 --- /dev/null +++ b/test/xterm-private-api.test.ts @@ -0,0 +1,69 @@ +// Port: none (dependency-range guard — no browser, no server). +// +// terminal-ui.js's `_kickRenderer` reaches into xterm internals to unwedge a +// frozen RenderDebouncer: +// +// terminal._core._renderService._renderDebouncer._animationFrame +// terminal._core._renderService.refreshRows(start, end) +// +// There is no public API for any of it — xterm exposes no way to ask "are you +// still producing frames" or "drop your stale animation handle" — and the bug +// it heals (iOS discarding a scheduled rAF, leaving that handle permanently set +// so every later refresh() early-returns) is otherwise unrecoverable without a +// page reload. +// +// That path CANNOT be asserted in this suite. `_renderService` is constructed +// by `Terminal.open()`, which needs a real DOM, and the CI gate runs in node — +// a headless Terminal reports `_renderService: undefined`, so a test here would +// pass whether or not the field still exists, which is worse than no test. +// +// So this guards the next best thing: the dependency range those field names +// were verified against. A major bump fails here, loudly, and sends someone to +// re-verify `_kickRenderer` by hand in a browser. The failure mode being +// defended against is silent — every access in `_kickRenderer` is +// optional-chained, so a renamed field degrades it to a permanent no-op with no +// error, no log, and a terminal that simply freezes again. +import { readFileSync } from 'node:fs'; +import { resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +const root = resolve(import.meta.dirname, '..'); +const pkg = JSON.parse(readFileSync(resolve(root, 'package.json'), 'utf8')) as { + dependencies: Record; +}; +const terminalUi = readFileSync(resolve(root, 'src/web/public/terminal-ui.js'), 'utf8'); + +// The major line `_kickRenderer`'s field path was verified against. +const VERIFIED_XTERM_RANGE = '^6.0.0'; + +describe('xterm private-API dependency guard', () => { + it('pins the xterm range _kickRenderer was verified against', () => { + expect( + pkg.dependencies['@xterm/xterm'], + 'xterm moved off the verified range — re-verify _kickRenderer in a real browser ' + + '(terminal-ui.js: _core._renderService._renderDebouncer._animationFrame), then update ' + + 'VERIFIED_XTERM_RANGE here. The accessor is optional-chained, so a renamed field ' + + 'degrades to a silent no-op and the freeze it heals comes back unnoticed.' + ).toBe(VERIFIED_XTERM_RANGE); + }); + + // If someone deletes the watchdog, this guard is pointless noise — keep the + // two tied together so the range check cannot outlive what it protects. + it('is guarding a watchdog that still exists', () => { + expect(terminalUi).toContain('_kickRenderer()'); + expect(terminalUi).toContain('_renderDebouncer'); + expect(terminalUi).toContain('_animationFrame'); + }); + + // Every private read must stay optional-chained. This is the property that + // makes reaching into internals acceptable at all: upstream can rename + // anything and the worst case is that healing stops, never that the terminal + // throws on a timer every two seconds. + it('reads every private field defensively', () => { + expect(terminalUi).toContain('this.terminal?._core?._renderService'); + const body = terminalUi.slice(terminalUi.indexOf('_kickRenderer() {')); + const fn = body.slice(0, body.indexOf('\n },')); + expect(fn).toContain('try {'); + expect(fn).toContain('catch'); + }); +}); From d200c0e467b505e1c77f2fe2632f6e8071a49132 Mon Sep 17 00:00:00 2001 From: Rounak Datta Date: Wed, 16 Sep 2026 00:41:42 +0530 Subject: [PATCH 2/2] fix(terminal): deadline must cover the body, precache must ignore the cache-bust query MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review fixes. Two of these are defects in the previous commit. 1. The fetch deadline only covered time-to-headers. `await fetch()` settles on response headers, so clearing the abort timer in a finally around it left the body — the multi-megabyte `?full=1` capture the deadline exists for — completely unbounded; it only ever bounded a server that accepts a connection and never replies. Measured against a server that sends headers immediately and stalls the body 4s under a 1s deadline: fetch resolved at 30ms, timer cleared there, body completed at 4026ms unaborted. Now the body is read inside `_fetchTerminalCapture`, which returns {json, headers, headersAt} — headers because two callers read server-timing, headersAt because those same callers measure header-vs-body time and can no longer observe that moment. `_terminalCaptureInflight` is scoped the same way, so a body still streaming counts toward a capture starting beside it. Same test now aborts at 1005ms. 2. The precache could never be hit, and the previous commit made that expensive rather than free. `renderIndexHtml` runs `cacheBustAssets`, which appends `?v=` to every same-origin .js/.css reference INCLUDING content-hashed names — confirmed against a running instance: `vendor/xterm-zerolag-input.6fee72f2.js?v=1789402869101`. `caches.match` is query-sensitive, so entries keyed on the bare hashed path were unreachable; deriving the list from the manifest turned cheap 404s into ~1.3MB downloaded at every install that nothing could read back, once per deploy now that CACHE_NAME rotates. The fallback match takes `{ ignoreSearch: true }`, which also lets runtime-cached entries survive an mtime change. 3. `_wsOutputGapSession` was only cleared in ws.onopen, so paths that already repaint the buffer left it set and the socket replayed everything a second time. `selectSession` loads the buffer and only THEN calls `_connectWs`, so neither the _isLoadingBuffer nor the _terminalRefreshOwner guard applied. `_markTerminalBufferReconciled()` is now called from _onSessionNeedsRefresh's finally, from selectSession after its load, and from _cleanupSessionData. The scope claim was also wrong and is corrected in the comment: when the network drops, SSE drops with it and handleInit's keepTerminal branch already reconciles. The genuinely uncovered case is the WS dying while SSE stays up, where _onSSETerminal discards SSE terminal frames until _wsReady flips in onclose — up to the ping+pong window of output nothing writes. 4. CLAUDE.md said "all of them measured rather than reasoned", which the PR's own "not verified" section contradicted. Split explicitly: the replay race is measured, the watchdog mechanism is verified against xterm 6.0.0 under jsdom (field path resolves, a forced stale handle makes refreshRows a no-op, the kick schedules a fresh frame), and the iOS rAF-discard premise is reasoned and still wants a device. Adds the two missing entries — the WebSocket reconcile and the sw.js/build.mjs "keep these in sync or the build throws" contract. Also: test/xterm-private-api.test.ts pins the RESOLVED lockfile version instead of the declared `^6.0.0` range, which was the wrong assertion in both directions — a real upgrade to 6.4.0 can rename a private field while resolving inside the range, and an innocuous range edit failed while changing nothing installed. And test/sw-precache-manifest.test.ts now parses HASHABLE out of scripts/build.mjs rather than hand-copying it, which was the same drift this PR exists to fix; the parse is guarded against silently matching nothing. The deadline fix has a behavioural test against a real socket plus a source guard asserting `await res.json()` precedes the finally — verified to fail when the helper is reverted to the old shape, so it is not vacuous. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 6 +- src/web/public/app.js | 96 ++++++++++++++++++++++++------- src/web/public/sw.js | 12 +++- test/sw-precache-manifest.test.ts | 53 ++++++++++------- test/terminal-resilience.test.ts | 92 +++++++++++++++++++++++++++++ test/xterm-private-api.test.ts | 24 +++++--- 6 files changed, 229 insertions(+), 54 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 45093c85f..6d135a3c2 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -334,7 +334,11 @@ Frontend JS modules have `@fileoverview` with `@dependency`/`@loadorder` tags. L **Shell keyboard accessory bar + one-shot Ctrl** (issue #262, `keyboard-accessory.js`): a **shell**-mode session automatically swaps the mobile accessory bar for terminal controls (Ctrl, Esc, Tab, four arrows, paste, dismiss); every other mode keeps the agent bar. `setMode()` now records the user's `extendedKeyboardBar` preference as the **base** layout and `refreshForActiveSession()` (called from `selectSession`) resolves base-vs-shell, so a settings save during a shell session cannot yank the bar away and switching back restores the user's choice. ⚠️ **Ctrl is a ONE-SHOT modifier applied in `terminal.onData`, not in a keydown handler**: a virtual keyboard emits no usable key events, so the character only exists as onData text. The hook sits AFTER `shouldSuppressTerminalQueryResponse` (xterm answers DA/CPR through onData too, and one of those would silently spend the modifier) and BEFORE every send path, so the control byte follows the normal control-char route. ⚠️ **Not every onData chunk is a keystroke**, and the query filter is not enough on its own: xterm ALSO emits mouse and focus reports on its own initiative, so the hook skips them via `isTerminalFocusOrMouseReport()` (they still reach the PTY, they just don't count as the next key). The mouse half is live — a shell session keeps the NARROW strip, so mouse DECSETs reach the browser and one tap while vim/htop runs spent the armed modifier silently (measured). The focus half is defense in depth: `FOCUS_ESCAPE_FILTER` in `session.ts` strips `\x1b[?1004h` from every PTY read, so `sendFocusMode` never turns on today; if it ever did, the bar's own post-key refocus would emit `\x1b[I` and eat the modifier before the user typed. ⚠️ It must disarm on ALL of: use, second tap, any other accessory key, session switch, keyboard dismissal, and a layout swap; a modifier left armed turns the next innocent keystroke into a control byte. ⚠️ **onData is not the only input path** — with `cjkInputEnabled` on, the CJK textarea owns the keyboard (onData returns early for everything it swallows, and the focus router sends `terminal.focus()` there, which is where the bar refocuses after every key), so `_handleCjkInput()` applies the modifier too. It is that module's single choke point to the PTY, so one call covers typed characters, IME flushes, Enter, backspace and arrows. Without it an armed modifier could neither fire NOR be spent, and survived to a later keystroke. Mapping is `ctrlByteFor()` (`code & 0x1f` over @A-Z[\]^_ and a-z, plus Ctrl+Space=NUL / Ctrl+?=DEL); characters with no control equivalent pass through unchanged, like a hardware keyboard. ⚠️ The armed style is `.accessory-btn.accessory-btn-ctrl.armed` (0,3,0) in BOTH stylesheets, and it cannot outrank mobile.css's light-skin repaint at **(0,3,1)** (`:is()` inherits its most specific argument, and that list holds `.btn-toolbar.btn-shell`) — so that rule excludes the state by hand as `.accessory-btn:not(.armed)`. Without the exclusion the armed button renders identically to a resting one on all four light skins, which is worse than no armed style at all. -**Terminal resilience: replay clears, renderer liveness, fetch deadlines**: three rules that each close a way the terminal silently stops being correct, all of them measured rather than reasoned. ⚠️ **A replay clear MUST be in-stream, never `reset()`/`clear()`.** xterm's `write()` is asynchronously queued while `Terminal.reset()` is synchronous and, per upstream, "does not clear input buffers and does not reset the parser" — so bytes queued just before a reset are parsed AFTER it and fuse into the snapshot written next. Reproduced against the real xterm 6 in this repo: `write('p8'); reset(); write('rmissions')` renders `p8rmissions`. `_resetTerminalForReplay()` (app.js) is the ONE clear, a single queued `\x1bc` (RIS), and all three replay paths go through it; RIS rather than `\x1b[3J\x1b[H\x1b[2J` because the erase leaves modes, charsets, scroll regions and SGR state alone, so leftover bytes can park the terminal in alt-screen and survive it. Callers may still chunk the content — ordering in the queue is what matters, not writing it in one call. ⚠️ **The renderer watchdog reaches into xterm privates and CANNOT be covered by the gate.** iOS discards scheduled rAF callbacks when a PWA backgrounds, and xterm's `RenderDebouncer` only clears `_animationFrame` from inside that callback — one drop leaves it permanently set and every later `refresh()` early-returns, so the buffer keeps updating correctly while nothing paints. Codeman has exactly ONE xterm for the whole page load, so a single backgrounding wedges it until a reload. `_kickRenderer()` (terminal-ui.js) cancels the stale handle and forces a repaint; `_renderService` only exists after `open()`, which needs a real DOM, so `test/xterm-private-api.test.ts` pins the dependency RANGE instead and a major bump means re-verifying the field path by hand in a browser. Every access is optional-chained on purpose: a renamed field must degrade to a no-op, never throw on a 2s timer. ⚠️ **Every terminal capture carries a deadline** (`_fetchTerminalCapture`, app.js). A `?full=1` body can be megabytes and used to hang on the browser default with no retry; the budget scales with full-vs-tail and with captures already in flight, so several tabs resuming do not all expire together. It degrades to a plain fetch where `AbortController` is missing — the deadline is a safety net, not a dependency. Tests: `test/terminal-resilience.test.ts` (pure decisions), `test/sw-precache-manifest.test.ts`, `test/xterm-private-api.test.ts`. +**Terminal resilience: replay clears, renderer liveness, fetch deadlines**: three rules that each close a way the terminal silently stops being correct. ⚠️ **A replay clear MUST be in-stream, never `reset()`/`clear()`.** xterm's `write()` is asynchronously queued while `Terminal.reset()` is synchronous and, per upstream, "does not clear input buffers and does not reset the parser" — so bytes queued just before a reset are parsed AFTER it and fuse into the snapshot written next. **Measured** against the real xterm in this repo: `write('p8'); reset(); write('rmissions')` renders `p8rmissions`; the queued `\x1bc` renders `rmissions` and clears scrollback. `_resetTerminalForReplay()` (app.js) is the ONE clear, a single queued `\x1bc` (RIS), and all three replay paths go through it; RIS rather than `\x1b[3J\x1b[H\x1b[2J` because the erase leaves modes, charsets, scroll regions and SGR state alone. Callers may still chunk the content — ordering in the queue is what matters, not writing it in one call. ⚠️ **The renderer watchdog reads xterm privates and CANNOT be covered by the gate.** `_kickRenderer()` (terminal-ui.js) cancels a stale `_core._renderService._renderDebouncer._animationFrame` and forces a repaint. **Verified against xterm 6.0.0** (jsdom, after `open()`): the field path resolves, a forced stale handle genuinely makes `refreshRows` a no-op, and the kick schedules a fresh frame. **Reasoned, not reproduced here**: the premise that iOS discards scheduled rAF callbacks when a PWA backgrounds, which is what leaves the handle stale — that half wants a real-device pass. Codeman has exactly ONE xterm for the whole page load, so one backgrounding would wedge it until a reload. `_renderService` only exists after `open()`, which needs a real DOM, and the gate runs in node — so `test/xterm-private-api.test.ts` pins the RESOLVED lockfile version (not the `^6.0.0` range, which a real upgrade slips through) and a bump means re-verifying by hand. Every access is optional-chained on purpose: a renamed field must degrade to a no-op, never throw on a 2s timer. ⚠️ **Every terminal capture carries a deadline, and the helper reads the BODY** (`_fetchTerminalCapture`, app.js). `await fetch()` settles on response HEADERS, so clearing the timer there leaves the body — the multi-megabyte `?full=1` capture this exists for — unbounded: **measured** at 4026ms under a 1000ms deadline before the fix. The helper therefore returns `{json, headers, headersAt}` rather than a `Response`, and `_terminalCaptureInflight` is scoped the same way so a body still streaming counts toward a capture starting beside it. It degrades to a plain fetch where `AbortController` is missing — the deadline is a safety net, not a dependency. Tests: `test/terminal-resilience.test.ts` (pure decisions), `test/xterm-private-api.test.ts`. + +**WebSocket output-gap reconcile** (`_wsOutputGapSession`, app.js): terminal OUTPUT frames carry no sequence number (input frames do — `seq`+`cid`, at-most-once, ACKed), so a dropped socket leaves a hole nothing replays. ⚠️ **The gap is narrower than "the device went offline"**: if the network drops, SSE drops with it and `handleInit`'s keepTerminal branch already calls `_onSessionNeedsRefresh`. The uncovered case is the WS dying while SSE stays up (half-open socket, proxy idle-timeout, ping timeout), because `_onSSETerminal` discards every SSE terminal frame while `_wsReady` is true and `_wsReady` only flips in `ws.onclose`. Reaching `onclose` at all means the drop was unintentional (`_disconnectWs` nulls the handler first), so the session is marked and the next successful open reconciles. ⚠️ **The marker must be cleared by EVERY path that repaints that session's buffer** — `_markTerminalBufferReconciled()` is called from `_onSessionNeedsRefresh`'s finally, from `selectSession` after its load, and from `_cleanupSessionData`. `selectSession` loads the buffer and only THEN calls `_connectWs`, so without that clear the socket opening afterwards replays the whole buffer a second time on top of the one just written. Sequencing the output frames is the real fix and is not done. This is reasoned from the code path, not observed on a device. + +**Service worker: precache and cache key are BUILD-GENERATED** (`sw.js` + `scripts/build.mjs`): the build content-hashes assets and rewrites two exact declarations in `sw.js` — `const BUILD_ID = 'dev';` and `const HASHED_ASSETS = [];`. ⚠️ **Each must appear exactly once or the build THROWS**, which is deliberate: the list used to be hand-maintained with PRE-hash names, so every entry 404'd in production and `cache.add().catch(() => {})` hid it (15 of 23 verified failing against a running instance). The dev literals are valid on their own, so dev serves an unrewritten worker with an empty precache. ⚠️ **`caches.match` must pass `ignoreSearch: true`**: `renderIndexHtml` runs `cacheBustAssets`, which appends `?v=` to every same-origin `.js`/`.css` reference INCLUDING content-hashed names, so the page requests `/app..js?v=` while the cache holds `/app..js`. Without it no precached entry is reachable and the install downloads ~1.3MB that can never be served — once per deploy, since `CACHE_NAME` now carries the build id. That per-build key is what makes `activate`'s cleanup actually delete anything; it used to be the constant `'codeman-v1'`, so assets from every past release accumulated forever. Contract pinned by `test/sw-precache-manifest.test.ts`, which PARSES the `HASHABLE` list out of `build.mjs` rather than copying it. **Dismissing the on-screen keyboard** (PRs #279/#280, `terminal-ui.js`): the terminal parks focus on a hidden textarea that nothing used to release, so TWO gestures now blur it, and they own different regions. **(1)** `_installMobileKeyboardDismiss()` — a document-level `touchend` that fires only while the terminal input actually holds focus, **never inside `#terminalContainer`** (tap classification owns that) and **never on a control** (`MOBILE_KEYBOARD_DISMISS_EXEMPT_SELECTOR`, matched with `closest()` so an icon inside a button counts). Session tabs are covered by the selector's `[tabindex]:not([tabindex="-1"])` arm, which is what stops a tab tap from blurring and then being re-focused by `selectSession()`. **(2)** In `_handleMobileTerminalTap`, a second tap on **inert `content`** (`startedWithTerminalFocus`) blurs instead of re-focusing. ⚠️ Scoped to `content` on purpose: the prompt row (`input`) keeps focus-then-position so a second tap still places the caret, and actionable rows blur earlier via `_isActionableMobileTerminalTap`. ⚠️ **A scroll ends in `touchend` too** — dismissing there closes the keyboard and drops the composer mid-read, so travel is tracked from `touchstart` and multi-touch is never a tap. Both classifiers MUST share one threshold: `initTerminal`'s `TAP_THRESHOLD` reads `MOBILE_KEYBOARD_DISMISS_TAP_SLOP`, since a gesture the terminal calls a scroll and the dismiss handler calls a tap is exactly that bug. ⚠️ **The gate excludes `test/mobile/**`, so CI cannot see the only test covering (1)** — run `npm run test:mobile -- test/mobile/keyboard.test.ts` by hand and diff the FAIL list against master. (Not `npm test --`: the gate's config excludes that path, so a file filter pointing into it matches nothing and exits green having run zero tests.) That blind spot is why merging the two PRs, which conflicted semantically but not textually, produced a red suite with two green CI checks. diff --git a/src/web/public/app.js b/src/web/public/app.js index 5d2155809..4f1f34058 100644 --- a/src/web/public/app.js +++ b/src/web/public/app.js @@ -2504,13 +2504,28 @@ class CodemanApp { * uplink legitimately needs longer than a tail, and eight tabs resuming must * not all expire together because each assumed it had the link to itself. * - * An abort surfaces as a rejected fetch, which every caller already handles — + * An abort surfaces as a rejected promise, which every caller already handles — * they wrap these in try/catch and log. That is the point: a timeout becomes a * recoverable error instead of an indefinite hang. * + * ⚠️ **The body is read HERE, and that is the whole point.** `await fetch()` + * settles on response HEADERS, not the body, so clearing the deadline when it + * resolves leaves the body — the multi-megabyte `?full=1` capture this exists + * for — completely unbounded. Measured against a server that sends headers + * immediately and stalls the body: `fetch()` resolved at 30ms, the timer was + * cleared there, and the body completed at 4026ms unaborted under a 1000ms + * deadline. Reading the body inside the helper is what makes the deadline + * cover the transfer rather than just the handshake. `_terminalCaptureInflight` + * is scoped the same way, so a body still streaming counts toward the budget + * of a capture starting beside it. + * + * Returns the PARSED envelope plus the response headers, because two callers + * read `server-timing`, and `headersAt` because those same callers measure + * header-vs-body time and can no longer observe that moment themselves. + * * @param {string} url * @param {{full?: boolean}} [opts] - * @returns {Promise} + * @returns {Promise<{json: unknown, headers: Headers|undefined, headersAt: number}>} */ async _fetchTerminalCapture(url, opts = {}) { const deadlineMs = @@ -2533,7 +2548,12 @@ class CodemanApp { const timer = controller ? setTimeout(() => controller.abort(), deadlineMs) : null; this._terminalCaptureInflight = (this._terminalCaptureInflight || 0) + 1; try { - return await (controller ? fetch(url, { signal: controller.signal }) : fetch(url)); + const res = await (controller ? fetch(url, { signal: controller.signal }) : fetch(url)); + const headersAt = performance.now(); + // Still inside the deadline: an abort here rejects the body stream, which + // is exactly the case a header-only timeout could not reach. + const json = await res.json(); + return { json, headers: res.headers, headersAt }; } catch (err) { if (err?.name === 'AbortError') { _crashDiag.log(`TERMINAL FETCH TIMEOUT after ${deadlineMs}ms`); @@ -2563,16 +2583,16 @@ class CodemanApp { // TUI modes still recover the whole picture, with the downgrade guard for // repaint-mode panes whose tmux capture can be smaller than xterm's buffer. const useFullHistory = this.sessions.get(sessionId)?.mode !== 'shell'; - let res = await this._fetchTerminalCapture( + let capture = await this._fetchTerminalCapture( useFullHistory ? `/api/sessions/${sessionId}/terminal?full=1` : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`, { full: useFullHistory } ); - let data = (await res.json())?.data ?? {}; + let data = capture.json?.data ?? {}; if (useFullHistory && data.terminalBuffer && this._replayWouldShrinkBuffer(data.terminalBuffer)) { - res = await this._fetchTerminalCapture(`/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`); - data = (await res.json())?.data ?? {}; + capture = await this._fetchTerminalCapture(`/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`); + data = capture.json?.data ?? {}; } // 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 @@ -2612,9 +2632,25 @@ class CodemanApp { console.error('needsRefresh reload failed:', err); } finally { if (this._terminalRefreshOwner === refreshOwner) this._terminalRefreshOwner = null; + // Any completed reload for this session IS the reconcile, whoever asked + // for it — handleInit's SSE-reconnect branch and selectSession both land + // here or do the same work. Leaving the marker set would make the next + // ws.onopen replay the whole buffer a second time. + this._markTerminalBufferReconciled(sessionId); } } + /** + * Drop the "this session lost output" marker. + * + * Called from every path that repaints a session's buffer from the server, so + * the ws.onopen reconcile fires once and only when nothing else already did + * the work. See the ws.onclose note for what the marker means. + */ + _markTerminalBufferReconciled(sessionId) { + if (sessionId && this._wsOutputGapSession === sessionId) this._wsOutputGapSession = null; + } + async _onSessionClearTerminal(data) { if (data.id === this.activeSessionId) { // Skip if selectSession is already loading the buffer — clearTerminal arriving @@ -2625,8 +2661,8 @@ class CodemanApp { // Fetch buffer, clear terminal, write buffer, resize (no Ctrl+L needed) try { - const res = await this._fetchTerminalCapture(`/api/sessions/${data.id}/terminal`); - const termData = (await res.json())?.data ?? {}; + const capture = await this._fetchTerminalCapture(`/api/sessions/${data.id}/terminal`); + const termData = capture.json?.data ?? {}; // Queued clear — see _resetTerminalForReplay for why clear()+reset() // cannot do this job. @@ -3009,11 +3045,19 @@ class CodemanApp { `WS CLOSE code=${event.code} reason=${event.reason || ''} action=${plan.action} attempts=${this._wsReconnectAttempts || 0}` ); - // Output frames carry no sequence number, so a socket that dropped left a - // hole in the terminal with nothing to replay it: ws.onopen re-sends dims - // and flushes queued INPUT, and `needsRefresh` only fires on external-CLI - // startup and on SSE backpressure drain — never here. Whatever the PTY - // produced while the link was down is simply absent from the buffer. + // Output frames carry no sequence number, so a dropped socket leaves a + // hole with nothing to replay it. ws.onopen re-sends dims and flushes + // queued INPUT; `needsRefresh` fires only on external-CLI startup and on + // SSE backpressure drain, never here. + // + // ⚠️ The gap this closes is NARROWER than "the device went offline". If + // the network drops, SSE drops with it and `handleInit`'s keepTerminal + // branch already reconciles on reconnect. The uncovered case is the WS + // dying while SSE stays up — a half-open socket, a proxy idle-timeout, + // a ping timeout — because `_onSSETerminal` discards every SSE terminal + // frame while `_wsReady` is true, and `_wsReady` only flips here, in + // onclose. Detecting a half-open socket takes up to the ping+pong window, + // and that whole span produces output nothing writes to the terminal. // // Reaching onclose at all means the drop was NOT intentional // (_disconnectWs nulls this handler first), so mark the gap and let the @@ -5934,9 +5978,9 @@ class CodemanApp { this._fullHistoryRepullInFlight = true; try { const requestStartedAt = performance.now(); - const res = await this._fetchTerminalCapture(`/api/sessions/${sessionId}/terminal?full=1`, { full: true }); - const headersReceivedAt = performance.now(); - const payload = (await res.json())?.data ?? {}; + const capture = await this._fetchTerminalCapture(`/api/sessions/${sessionId}/terminal?full=1`, { full: true }); + const headersReceivedAt = capture.headersAt; + const payload = capture.json?.data ?? {}; const bodyParsedAt = performance.now(); const buffer = payload.terminalBuffer; const timing = { @@ -5949,7 +5993,7 @@ class CodemanApp { bodyAndJsonMs: bodyParsedAt - headersReceivedAt, resetAndParseMs: 0, totalMs: 0, - serverTiming: res.headers?.get?.('server-timing') || '', + serverTiming: capture.headers?.get?.('server-timing') || '', refused: false, }; // Bail on a tab switch mid-fetch: writing here would paint another session's @@ -6422,18 +6466,18 @@ class CodemanApp { const useFullHistory = session?.mode !== 'shell' && !this._fullHistoryLoaded.has(sessionId); if (useFullHistory) this._fullHistoryLoaded.add(sessionId); const fetchStartedAt = performance.now(); - const res = await this._fetchTerminalCapture( + const capture = await this._fetchTerminalCapture( useFullHistory ? `/api/sessions/${sessionId}/terminal?full=1` : `/api/sessions/${sessionId}/terminal?tail=${TERMINAL_TAIL_SIZE}`, { full: useFullHistory } ); - const headersReceivedAt = performance.now(); + const headersReceivedAt = capture.headersAt; if (this._isStaleSelect(selectGen)) { this._clearTerminalLoadState(sessionId, selectGen); return; } - const data = (await res.json())?.data ?? {}; + const data = capture.json?.data ?? {}; const bodyParsedAt = performance.now(); _crashDiag.log(`FETCH_DONE: ${data.terminalBuffer ? (data.terminalBuffer.length/1024).toFixed(0) + 'KB' : 'empty'} truncated=${data.truncated}`); @@ -6503,7 +6547,7 @@ class CodemanApp { cacheResetAndParseMs, freshResetAndParseMs, selectToReplayCompleteMs: performance.now() - _selStart, - serverTiming: res.headers?.get?.('server-timing') || '', + serverTiming: capture.headers?.get?.('server-timing') || '', }; // Buffer load complete — unblock live SSE writes. chunkedTerminalWrite calls // _finishBufferLoad after ordering the fetched snapshot in xterm; if we skipped @@ -6513,6 +6557,11 @@ class CodemanApp { if (this._isLoadingBuffer) { this._finishBufferLoad(bufferLoadOwner, { flushQueued: bufferWasEmpty }); } + // This load repainted the session from the server, so any pending + // output-gap marker is already satisfied. Selecting a session runs BEFORE + // _connectWs, so without this the socket opening afterwards would replay + // the whole buffer again on top of the one just written. + this._markTerminalBufferReconciled(sessionId); // Drop the guard so user input clears state normally this._restoringFlushedState = false; @@ -6653,6 +6702,9 @@ class CodemanApp { // Shared cleanup for all session data — called from both closeSession() and session:deleted handler _cleanupSessionData(sessionId) { this.closeTabRailActionMenu?.(); + // A dead session has no buffer to reconcile; leaving the marker set would + // make a later socket for a REUSED id reconcile against nothing. + this._markTerminalBufferReconciled(sessionId); // If the deleted session is currently being renamed, abort the rename // so the inline doesn't ghost as a stale tab on screen. if (this._activeRename?.sessionId === sessionId) { diff --git a/src/web/public/sw.js b/src/web/public/sw.js index 6eda44df0..966d302c8 100644 --- a/src/web/public/sw.js +++ b/src/web/public/sw.js @@ -111,7 +111,17 @@ self.addEventListener('fetch', (event) => { } return response; }) - .catch(() => caches.match(request)) + // ignoreSearch, or the precache can never be hit. `renderIndexHtml` runs + // `cacheBustAssets`, which appends `?v=` to EVERY same-origin + // `.js`/`.css` reference — content-hashed names included, so the page asks + // for `/app.556be563.js?v=1789423735875` while the precache stored + // `/app.556be563.js`. `caches.match` is query-sensitive by default, so + // every precached entry was unreachable and only `/`, the icons and the + // manifest could ever be served offline. + // + // It also makes runtime-cached entries survive an mtime change: the same + // file re-requested under a new `?v=` still matches the copy already held. + .catch(() => caches.match(request, { ignoreSearch: true })) ); }); diff --git a/test/sw-precache-manifest.test.ts b/test/sw-precache-manifest.test.ts index ee67aa7fb..09d1e31b2 100644 --- a/test/sw-precache-manifest.test.ts +++ b/test/sw-precache-manifest.test.ts @@ -53,29 +53,24 @@ describe('service worker precache contract', () => { expect(sw).toContain("...HASHED_ASSETS.map((p) => '/' + p)"); }); - // The regression itself. These are the pre-hash names the build renames, so - // any of them appearing in the shell list means someone hand-added an entry - // that will 404 in production. + // The regression itself: any pre-hash filename hand-listed in APP_SHELL will + // 404 in production, because the build renames it. + // + // The HASHABLE list is PARSED out of scripts/build.mjs rather than copied + // here. A hand-kept duplicate would be the same drift this whole PR exists to + // fix — it would go stale the first time someone adds an asset to the build, + // and then silently stop covering it. it('never hand-lists a filename the build content-hashes', () => { + const block = build.slice( + build.indexOf('const HASHABLE = ['), + build.indexOf('];', build.indexOf('const HASHABLE = [')) + ); + const hashedByBuild = [...block.matchAll(/'([^']+)'/g)].map((m) => m[1]); + // Guard the parse itself: an empty list would make this test vacuously pass. + expect(hashedByBuild.length, 'failed to parse HASHABLE out of scripts/build.mjs').toBeGreaterThan(10); + expect(hashedByBuild).toContain('app.js'); + const shell = sw.slice(sw.indexOf('const APP_SHELL'), sw.indexOf('].map(B);')); - const hashedByBuild = [ - 'app.js', - 'constants.js', - 'terminal-ui.js', - 'session-ui.js', - 'settings-ui.js', - 'panels-ui.js', - 'styles.css', - 'mobile.css', - 'i18n.js', - 'mobile-handlers.js', - 'keyboard-accessory.js', - 'notification-manager.js', - 'voice-input.js', - 'api-client.js', - 'vendor/xterm-zerolag-input.js', - 'vendor/xterm-predictive-echo.js', - ]; for (const name of hashedByBuild) { expect(shell, `APP_SHELL must not hand-list ${name} — the build renames it`).not.toContain(`'/${name}'`); } @@ -86,4 +81,20 @@ describe('service worker precache contract', () => { it('is valid unrewritten, for dev', () => { expect(() => new Function(sw.replace(/self\./g, 'globalThis.'))).not.toThrow(); }); + + // Without ignoreSearch the whole precache is unreachable, which is subtle + // enough to be re-broken by anyone tidying this handler. + // + // `renderIndexHtml` runs `cacheBustAssets`, which appends `?v=` to + // EVERY same-origin `.js`/`.css` reference — content-hashed names included. + // Observed on a running instance: `src="app.556be563.js?v=1789423735875"`. + // `caches.match` is query-sensitive by default, so a precache keyed on + // `/app.556be563.js` can never serve that request, and the install would be + // downloading ~1.3MB per deploy that nothing can ever read back. + it('falls back to the cache ignoring the cache-busting query string', () => { + expect(sw).toContain('caches.match(request, { ignoreSearch: true })'); + expect(sw, 'a bare caches.match(request) cannot match the ?v= URLs cacheBustAssets emits').not.toMatch( + /caches\.match\(request\)\s*\)/ + ); + }); }); diff --git a/test/terminal-resilience.test.ts b/test/terminal-resilience.test.ts index 71cc07ddc..7dbee74c4 100644 --- a/test/terminal-resilience.test.ts +++ b/test/terminal-resilience.test.ts @@ -15,6 +15,8 @@ import { readFileSync } from 'node:fs'; import { resolve } from 'node:path'; import vm from 'node:vm'; +import { createServer, type ServerResponse } from 'node:http'; +import type { AddressInfo } from 'node:net'; import { describe, expect, it } from 'vitest'; function loadConstants() { @@ -152,3 +154,93 @@ describe('sanitizeDiagEntry', () => { expect(sanitizeDiagEntry({ toString: () => 'obj' })).toBe('obj'); }); }); + +// ── The deadline must cover the BODY, not just the handshake ──────────────── +// +// `await fetch()` settles on response HEADERS. Clearing the abort timer there +// leaves the body — the multi-megabyte `?full=1` capture the deadline exists +// for — completely unbounded; it only ever covered a server that accepts a +// connection and never replies at all. +// +// Measured on the pre-fix shape against a server that sends headers immediately +// and stalls the body 4s under a 1s deadline: fetch resolved at 30ms, the timer +// was cleared there, and the body completed at 4026ms unaborted. +// +// This exercises the real property with a real socket rather than asserting on +// source text, because the bug was a lifetime mistake that reads correctly. +describe('terminal capture deadline covers the response body', () => { + // Mirrors _fetchTerminalCapture's lifetime: one timer spanning headers AND + // body, cleared only once the body has been read. + async function captureUnderDeadline(url: string, deadlineMs: number) { + const controller = new AbortController(); + const timer = setTimeout(() => controller.abort(), deadlineMs); + try { + const res = await fetch(url, { signal: controller.signal }); + const headersAt = performance.now(); + const json = await res.json(); + return { json, headers: res.headers, headersAt }; + } finally { + clearTimeout(timer); + } + } + + async function serve(handler: (res: ServerResponse) => void) { + const srv = createServer((_req, res) => handler(res)); + await new Promise((r) => srv.listen(0, '127.0.0.1', r)); + const { port } = srv.address() as AddressInfo; + return { url: `http://127.0.0.1:${port}/`, close: () => srv.close() }; + } + + it('aborts a stalled body instead of waiting on it forever', async () => { + let finish: NodeJS.Timeout | undefined; + const { url, close } = await serve((res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.write(' '); // headers out immediately, body never completes in time + finish = setTimeout(() => res.end('{"data":{}}'), 5000); + }); + try { + await expect(captureUnderDeadline(url, 300)).rejects.toThrow(/abort/i); + } finally { + if (finish) clearTimeout(finish); + close(); + } + }); + + // The two tests above exercise the PATTERN against a real socket, using a + // local mirror — so on their own they would still pass if the real helper + // regressed to clearing its timer at headers. This pins the real one. + it('_fetchTerminalCapture reads the body before releasing its deadline', () => { + const app = readFileSync(resolve(import.meta.dirname, '../src/web/public/app.js'), 'utf8'); + const start = app.indexOf('async _fetchTerminalCapture('); + expect(start, 'helper not found — renamed?').toBeGreaterThan(-1); + const body = app.slice(start, app.indexOf('\n }', start)); + const jsonAt = body.indexOf('await res.json()'); + const finallyAt = body.indexOf('} finally {'); + expect(jsonAt, 'the body must be read inside the helper, not by callers').toBeGreaterThan(-1); + expect(finallyAt).toBeGreaterThan(-1); + expect( + jsonAt, + 'await res.json() must run BEFORE the finally that clears the abort timer — ' + + 'fetch() settles on headers, so a timer cleared there leaves the body unbounded' + ).toBeLessThan(finallyAt); + // And the returned shape the five call sites destructure. + expect(body).toContain('return { json, headers: res.headers, headersAt };'); + }); + + it('returns the parsed envelope and headers on a healthy response', async () => { + const { url, close } = await serve((res) => { + res.writeHead(200, { 'Content-Type': 'application/json', 'server-timing': 'db;dur=12' }); + res.end('{"data":{"terminalBuffer":"hello"}}'); + }); + try { + const out = await captureUnderDeadline(url, 5000); + // Callers read `capture.json?.data`, `capture.headers.get(...)` and + // `capture.headersAt` — all three must survive. + expect((out.json as { data: { terminalBuffer: string } }).data.terminalBuffer).toBe('hello'); + expect(out.headers.get('server-timing')).toBe('db;dur=12'); + expect(typeof out.headersAt).toBe('number'); + } finally { + close(); + } + }); +}); diff --git a/test/xterm-private-api.test.ts b/test/xterm-private-api.test.ts index 9b4024095..36a0b8914 100644 --- a/test/xterm-private-api.test.ts +++ b/test/xterm-private-api.test.ts @@ -28,23 +28,29 @@ import { resolve } from 'node:path'; import { describe, expect, it } from 'vitest'; const root = resolve(import.meta.dirname, '..'); -const pkg = JSON.parse(readFileSync(resolve(root, 'package.json'), 'utf8')) as { - dependencies: Record; +const lock = JSON.parse(readFileSync(resolve(root, 'package-lock.json'), 'utf8')) as { + packages: Record; }; const terminalUi = readFileSync(resolve(root, 'src/web/public/terminal-ui.js'), 'utf8'); -// The major line `_kickRenderer`'s field path was verified against. -const VERIFIED_XTERM_RANGE = '^6.0.0'; +// The exact version `_kickRenderer`'s field path was verified against. +// +// Read from the LOCKFILE, not package.json. The declared range is `^6.0.0`, so +// asserting on that string is the wrong test in both directions: a real upgrade +// to 6.4.0 — which can absolutely rename a private field — resolves inside the +// range and slips through, while an innocuous range edit that changes nothing +// about the installed code fails. The lockfile is what actually ships. +const VERIFIED_XTERM_VERSION = '6.0.0'; describe('xterm private-API dependency guard', () => { - it('pins the xterm range _kickRenderer was verified against', () => { + it('pins the resolved xterm version _kickRenderer was verified against', () => { expect( - pkg.dependencies['@xterm/xterm'], - 'xterm moved off the verified range — re-verify _kickRenderer in a real browser ' + + lock.packages['node_modules/@xterm/xterm']?.version, + 'xterm moved off the verified version — re-verify _kickRenderer in a real browser ' + '(terminal-ui.js: _core._renderService._renderDebouncer._animationFrame), then update ' + - 'VERIFIED_XTERM_RANGE here. The accessor is optional-chained, so a renamed field ' + + 'VERIFIED_XTERM_VERSION here. The accessor is optional-chained, so a renamed field ' + 'degrades to a silent no-op and the freeze it heals comes back unnoticed.' - ).toBe(VERIFIED_XTERM_RANGE); + ).toBe(VERIFIED_XTERM_VERSION); }); // If someone deletes the watchdog, this guard is pointless noise — keep the