fix(terminal): four silent-failure paths — renderer freeze, replay race, reconnect gap, unbounded fetches

[rounakdatta]

Four fixes for ways the mobile terminal can silently stop being correct — where "silently" is the operative word: in every case the buffer keeps updating, nothing throws, and the user's only recourse is a reload.

They came out of reading Big-Pony/pocketshell, a mobile-first terminal that has done unusually deep forensics on xterm's failure modes. Each item below was re-verified against Codeman's own source before being written — several things that repo warns about turned out to be already handled here, and those are not in this PR.

---

1. Renderer freeze after backgrounding (terminal-ui.js)

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 after
  this._animationFrame = requestAnimationFrame(() => this._innerRefresh());
}
_innerRefresh() { this._animationFrame = undefined; ... }   // never runs

One dropped callback leaves the handle permanently set, so every later render request returns on line one. Parsing is decoupled from rendering, so bytes keep filling the buffer correctly and nothing errors — the terminal is simply frozen. Closing and reopening fixes it because that constructs a new Terminal.

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 until a reload.

_startRenderLivenessWatchdog() polls every 2s and, when a visible terminal has been written to but has produced no frame for 4s, _kickRenderer() does what the dropped _innerRefresh would have: cancel the stale handle, clear the field, force a repaint.

⚠️ This reads xterm privates — there is no public API for any of it. Every access is optional-chained and wrapped, so a shape change upstream degrades to a no-op rather than throwing on a timer. _renderService only exists after open() (needs a real DOM), so the gate can't assert the field path.

test/xterm-private-api.test.ts now pins the resolved lockfile version rather than the declared ^6.0.0 range — the range was the wrong assertion in both directions, since a real upgrade to 6.4.0 could rename a private field while resolving inside it, and an innocuous range edit would fail while changing nothing installed.

Thank you for independently verifying the field path and the heal against 6.0.0 under jsdom — that covers the half of this I couldn't reach. The remaining unverified half is the premise: that iOS discards scheduled rAF callbacks on backgrounding, which is what leaves the handle stale. That still wants a real device.

2. Replay clears race live output (app.js)

xterm's write() is asynchronously queued; Terminal.reset() is synchronous and, per upstream's own docs, "does not clear input buffers and does not reset the parser, thus the terminal will continue to apply pending input data." 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')   ->  "p8rmissions"
write('p8'); write('\x1bc'); write('rmissions')  ->  "rmissions"

_resetTerminalForReplay() already got this right by following the sync reset() with a queued \x1b[3J\x1b[H\x1b[2J. Two other paths — the needsRefresh reload and the clearTerminal refresh — hand-rolled clear() + reset() with no in-stream erase and were genuinely exposed.

All three now go through one function, which is a single queued \x1bc (RIS). RIS rather than the erase because 3J/H/2J leaves modes, charsets, scroll regions and SGR state alone, so leftover bytes can park the terminal in alt-screen and survive the clear. Callers can still chunk the content — ordering in the queue is what matters, not writing it in one call.

3. Output lost when the WebSocket dies but SSE stays up (app.js)

The terminal WS protocol is asymmetric in a way that's easy to miss, because one half is excellent. Input frames carry seq + cid, the server applies each pair at-most-once and ACKs, and the client holds a durable queue until the ACK lands. Output frames ({"t":"o","d":...}) carry nothing, and ws.onopen re-sends dimensions and flushes queued input only.

I traced every emitter of needsRefresh, the one server signal that could cover the gap — there are two: external-CLI startup 3s after spawn (session.ts), and SSE backpressure drain (sse-stream-manager.ts). Neither fires on a WS reconnect.

Scope correction from review: an earlier draft of this described the gap as "output produced while a phone is off-network." That was wrong. If the network drops, SSE drops with it, and handleInit's keepTerminal branch already calls _onSessionNeedsRefresh for exactly this reason. The genuinely uncovered case is narrower: the WS dying while SSE stays up — a half-open socket, a proxy idle-timeout, a ping timeout. _onSSETerminal discards every SSE terminal frame while _wsReady is true, and _wsReady only flips in ws.onclose, so detecting a half-open socket takes up to the ping+pong window and that whole span produces output nothing writes to the terminal.

Still worth closing, and this is the interim fix rather than the real one: 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 from the server's buffer. Sequencing the output frames properly is the follow-up.

_markTerminalBufferReconciled() clears the marker from every path that repaints that session's buffer — _onSessionNeedsRefresh's finally, selectSession after its load, and _cleanupSessionData. Without that, selectSession (which loads the buffer and only then calls _connectWs) would have the socket replay the whole buffer a second time on top of the one just written.

4. Terminal captures had no deadline (app.js)

No fetch in the frontend carried a timeout — no AbortController, no AbortSignal — including ?full=1, which _maybeRefetchFullHistory itself describes as "unbounded-ish work: at the default history limit it can be megabytes."

Corrected from review: the first version of this cleared its abort timer in a finally around await fetch(url) — which settles on response headers, not the body. So it only ever bounded a server that accepts a connection and never replies; the multi-megabyte body it was written for stayed unbounded. Reproduced against a server that sends headers immediately and stalls the body 4s under a 1s deadline: fetch resolved at 30ms, the timer cleared there, body completed at 4026ms unaborted.

_fetchTerminalCapture now reads the body inside the helper and 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 themselves. _terminalCaptureInflight is scoped the same way, so a body still streaming counts toward the budget of a capture starting beside it. Same test now aborts at 1005ms.

The budget scales with full-vs-tail and with captures already in flight, and degrades to a plain fetch where AbortController is missing — the deadline is a safety net, not a dependency.

Also included

• Service worker precache was dead, and my first fix made it worse. The build content-hashes assets and rewrites index.html, but sw.js was maintained by hand with the pre-hash names, so every entry 404'd and cache.add(...).catch(() => {}) hid it (15 of 23, verified against a running instance). Offline still worked, because the fetch handler caches every successful GET at runtime.

  Corrected from review: deriving the list from the manifest was not sufficient. renderIndexHtml runs cacheBustAssets, which appends ?v=<mtime> to every same-origin .js/.css reference including content-hashed names — confirmed on a live instance: vendor/xterm-zerolag-input.6fee72f2.js?v=1789402869101. caches.match is query-sensitive, so a precache keyed on the bare path could never be hit, and my change turned entries that failed cheaply into ~1.3MB downloaded at every install that nothing could ever read back — once per deploy, since CACHE_NAME now rotates. The fallback match now passes { ignoreSearch: true }, which also lets runtime-cached entries survive an mtime change. Pinned by a test, since it is subtle enough to be tidied away.

  The per-build CACHE_NAME stands on its own: activate deletes every cache that is not the current one, so the old constant 'codeman-v1' meant that cleanup never ran and assets from every past release accumulated forever.

• Crash-trail hygiene. Entries are joined with \n into one localStorage value and beaconed, and at least one call site interpolates a server-controlled WS close reason — a newline there forges entries. Now flattened and length-capped.

---

Testing

• Full gate green apart from cron-service.test.ts "blocks a sensitive system file via the blocklist", which fails identically on clean origin/master in my sandbox and — per your run — does not reproduce on yours. Environmental, not a regression.
• npm run typecheck, npm run lint, npm run format:check, npm run check:frontend-syntax, npm run check:public-assets — all green.
• New tests: test/terminal-resilience.test.ts, test/sw-precache-manifest.test.ts, test/xterm-private-api.test.ts. All run in the gate, which is deliberate given the mobile suite is excluded from CI.
• The deadline fix has a behavioural test against a real socket (a server that stalls its body), plus a source guard asserting await res.json() runs before the finally — I verified that guard fails when the helper is reverted to the old shape, so it is not vacuous.
• test/sw-precache-manifest.test.ts now parses HASHABLE out of scripts/build.mjs instead of hand-copying it. You were right that the copy was the same drift this PR exists to fix; the parse is guarded against silently matching nothing.
• §2's RIS behaviour was verified against the real xterm build headless, not reasoned from docs.

Not verified here

Still no browser on my machine, so no runtime verification from my side. With your jsdom check of the field path and the heal, what remains unverified is the premise of §1 — that iOS discards scheduled rAF callbacks on backgrounding — and the §3 reconnect path, which is traced through code rather than observed on a device. Both are worth the real-device pass you suggested before the release that carries this.

On the split

Agreed, and noted for next time — the service worker and build script are a different subsystem from the four terminal fixes, and the only build-breaking risk in the set. If you would still prefer them separated before merging, say the word and I will split the sw/build commit onto its own branch and rebase this one to the terminal work alone.

🤖 Generated with Claude Code

[Ark0N]

Thanks for this, and for the forensics write-up that goes with it. Four terminal silent-failure paths plus the service-worker precache is a lot of careful reading, and the replay-clear item in particular fixes a real corruption path nothing else here covered.

One thing worth saying up front: I verified the piece you flagged as most wanting eyes. Driving the repo's own xterm 6.0.0 under jsdom, _core._renderService._renderDebouncer exists after open(), refreshRows is a function, a forced stale _animationFrame genuinely makes refreshRows a no-op, and your kick sequence schedules a fresh frame. The field path and the heal are correct for the pinned version. I also reproduced the reset() race: write('p8'); reset(); write('rmissions') gives p8rmissions, the queued \x1bc gives rmissions, and RIS clears scrollback and leaves the alt buffer as you describe.

Two things need a change before this lands.

1. The fetch deadline only covers time-to-headers (src/web/public/app.js:2543). _fetchTerminalCapture clears its timer in a finally that runs when await fetch(url) settles, and a fetch promise settles on response headers. Every caller then reads the body with await res.json() with the deadline already gone. I checked it against a local server that sends headers immediately and stalls the body for 4s with a 1s deadline: fetch resolved at 20ms, the timer was cleared there, the body completed at 4018ms unaborted. So the multi-megabyte ?full=1 body the item was written for is still unbounded; what you have covers a server that accepts the connection and never replies. Could you read the body inside the helper and clear the timer after it? All five call sites call .json() immediately and two also read res.headers.get('server-timing'), so returning { data, headers } covers them. Same scoping applies to _terminalCaptureInflight, which decrements at headers, so bodies still streaming do not count toward the budget of a capture starting beside them.

2. The precache still never gets used (src/web/public/sw.js:56). renderIndexHtml runs cacheBustAssets, which appends ?v=<mtime> to every same-origin .js/.css reference, content-hashed names included. On my instance that is src="app.556be563.js?v=1789423735875". The precache stores /app.556be563.js with no query, and caches.match(request) is query-sensitive, so nothing in the list can be matched: only /, the icons and /manifest.json are reachable, which was already the case. The change therefore turns entries that failed cheaply into entries that succeed expensively, about 1.3 MB uncompressed downloaded a second time at install, and since CACHE_NAME now carries the build id that install happens after every deploy. One line fixes it: .catch(() => caches.match(request, { ignoreSearch: true })), which also lets runtime-cached entries survive an mtime change. The build-side plumbing is good and I want to keep it: I simulated the rewrite against a fake manifest and both anchors hit exactly once, the result parses, and APP_SHELL resolves correctly at the root and under a /codeman mount. The per-build CACHE_NAME is a genuine fix for unbounded cache growth.

Two smaller ones while you are in there.

3. _wsOutputGapSession is only cleared in ws.onopen (app.js:2956). Two other paths reload the same buffer without clearing it, so the reconcile runs a second full replay. Switch away from a dropped session and back: selectSession reloads the buffer and only then calls _connectWs (app.js:6623), so by the time the socket opens neither the _isLoadingBuffer nor the _terminalRefreshOwner guard applies. And on an SSE reconnect, handleInit's keepTerminal branch already calls _onSessionNeedsRefresh (app.js:4036-4043) for exactly this reason (its comment says "so output produced during the outage is not lost"). That second point means the PR description overstates the gap: when a phone leaves the network, SSE drops too and its reconnect already reconciles. The hole you are genuinely closing is narrower and still worth closing, a WebSocket dying while SSE stays up, where _onSSETerminal keeps discarding SSE terminal frames until _wsReady flips false. Clearing the marker whenever a buffer reload for that session completes, and in _cleanupSessionData, covers it.

4. CLAUDE.md:337 says "all of them measured rather than reasoned", which your own "Not verified here" section contradicts. That line is load-bearing in this file: its value is that a reader can trust a claim without re-deriving it. Please say which half was measured (the replay race) and which is reasoned from xterm's source. You can now describe the watchdog's mechanism as verified against 6.0.0 given the check above; the iOS rAF-discard premise stays reasoned. While editing: the WebSocket reconcile and the sw.js/build.mjs contract get no CLAUDE.md line at all, and the sw contract in particular is the kind of build-time "keep these two in sync or the build throws" rule this file carries elsewhere.

Nits, take or leave: test/xterm-private-api.test.ts:37 pins the range string from package.json, so a real upgrade inside ^6.0.0 slips through while an innocuous range edit fails, reading the resolved version from package-lock.json would observe the actual dependency; and test/sw-precache-manifest.test.ts:61 hand-copies the HASHABLE list out of scripts/build.mjs, which is the same drift the PR is fixing (the test already reads that file).

One process note for next time: this is six changes in one PR, and the service worker plus build script is a different subsystem from the four terminal fixes, with the only build-breaking risk in the set. Splitting that out would have let the terminal work land on its own timeline.

Everything else is green here: typecheck, lint, format, frontend-syntax, public-assets, and the full gate (386 files, 7273 tests, 546s). The cron-service failure you saw locally does not reproduce on my machine. Send the fixes for 1 and 2 (plus 3 and 4 if you have the appetite) and I will merge. I would still like a real-device pass on the watchdog before the release that carries it, as you suggested.

[rounakdatta]

Thank you for this — particularly for verifying the field path and the heal under jsdom. That covers the half I flagged and couldn't reach, and it's the reason I'm comfortable with the watchdog now.

All four are addressed in d200c0e, plus both nits. Two of them were defects in my previous commit and one was a regression I introduced, so taking each in turn.

1. Deadline only covered time-to-headers. You're right, and I reproduced your measurement before fixing it: headers at 30ms, timer cleared there, body completed at 4026ms unaborted under a 1000ms deadline. So the item didn't do the thing it was written for — it only bounded a server that accepts a connection and never replies.

_fetchTerminalCapture now reads the body and returns {json, headers, headersAt}. headers for the two server-timing reads; headersAt because those same two callers measure header-vs-body time and can no longer observe that moment themselves. _terminalCaptureInflight decrements after the body for the reason you gave. Same stalled-body test now aborts at 1005ms.

It has a behavioural test against a real socket, plus a source guard asserting await res.json() precedes the finally — I checked that guard fails when the helper is reverted to the old shape, since a behavioural test using its own local copy would have passed either way.

2. Precache unreachable. Confirmed on the running instance — vendor/xterm-zerolag-input.6fee72f2.js?v=1789402869101, hash and query. So my change was strictly worse than the status quo: it turned entries that failed cheaply into ~1.3MB downloaded at every install that nothing could read back, once per deploy now that CACHE_NAME rotates. { ignoreSearch: true } applied, and pinned by a test since it's subtle enough to be tidied away by someone cleaning up that handler.

3. Marker only cleared in ws.onopen. Both paths confirmed. selectSession loads the buffer and only then calls _connectWs, so by the time the socket opens neither guard applies and the reconcile replayed everything a second time. _markTerminalBufferReconciled() now runs from _onSessionNeedsRefresh's finally, from selectSession after its load, and from _cleanupSessionData.

Your second point there is the more important one: the PR description was wrong, not just imprecise. SSE drops with the network and handleInit already reconciles, so "output produced while a phone is off-network" described a case that was already handled. The genuine hole is the WS dying while SSE stays up — _onSSETerminal discards frames while _wsReady is true, and that only flips in onclose, so a half-open socket costs up to the ping+pong window. I've rewritten both the code comment and the PR description to claim only that.

4. CLAUDE.md overstated. Fair, and it's the failure mode that file exists to prevent. Now split explicitly: the replay race is measured, the watchdog mechanism is verified against 6.0.0 (credited to your check), the iOS rAF-discard premise is reasoned and still wants a device. Added the two missing entries — the WebSocket reconcile, and the sw.js/build.mjs contract as a build-time "keep these in sync or the build throws" rule.

Nits, both taken. The range assertion was wrong in both directions, exactly as you say — 6.4.0 can rename a private field while resolving inside ^6.0.0, and a harmless range edit failed while changing nothing installed. Reads the resolved lockfile version now. And the hand-copied HASHABLE list was the same drift this PR exists to fix, which I should have caught; it's parsed out of build.mjs, with a guard so an empty parse can't make the test vacuously pass.

On the split. Agreed. The sw/build work is a different subsystem and carries the only build-breaking risk in the set, and separating it would have let the terminal fixes move independently. Happy to split it now if you'd prefer — say the word and I'll put the sw/build commit on its own branch and rebase this one down to the four terminal fixes.

Gate here: 7276 passed, 1 skipped, and the same cron-service sensitive-path failure that doesn't reproduce on your machine — environmental, and it fails identically on clean origin/master in my sandbox.

Still no browser my end, so the real-device pass on the watchdog is still worth doing before the release that carries it, as you said.