fix(terminal): four silent-failure paths — renderer freeze, replay race, reconnect gap, unbounded fetches - #431
Open
rounakdatta wants to merge 1 commit into
Conversation
…es, reconnect recovery
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
requestAnimationFramecallbacks when a PWA goes to the background — not deferred, never delivered. xterm'sRenderDebounceronly clears its_animationFramehandle from inside that callback: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_innerRefreshwould have: cancel the stale handle, clear the field, force a repaint._renderServiceonly exists afteropen()(needs a real DOM), so the gate can't assert the field path;test/xterm-private-api.test.tspins the dependency range instead, and a major bump fails there to send someone to re-verify by hand.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:
_resetTerminalForReplay()already got this right by following the syncreset()with a queued\x1b[3J\x1b[H\x1b[2J. Two other paths — theneedsRefreshreload and theclearTerminalrefresh — hand-rolledclear()+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 because3J/H/2Jleaves 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 on WebSocket reconnect (
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.ws.onopenre-sends dimensions and flushes queued input. I traced every emitter ofneedsRefresh, 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.So output produced while a phone is off-network is simply absent from the buffer afterwards, recovered only by luck when a dimension change happens to trigger a SIGWINCH repaint — which doesn't happen at the same size, and doesn't help plain shell scrollback at all.
This is the interim fix, not the real one: reaching
ws.oncloseat all means the drop was unintentional (_disconnectWsnulls the handler first), so the session is marked and the next successful open reconciles from the server's buffer. Costs a repaint; closes the data loss. Sequencing the output frames properly is the follow-up.4. Terminal captures had no deadline (
app.js)No fetch in the frontend carried a timeout — no
AbortController, noAbortSignal— including?full=1, which_maybeRefetchFullHistoryitself describes as "unbounded-ish work: at the default history limit it can be megabytes." On a stalled mobile link that hangs on the browser default with no retry._fetchTerminalCapture()scales the budget by full-vs-tail and by captures already in flight, so several tabs resuming don't all expire together. It degrades to a plain fetch whereAbortControlleris missing — the deadline is a safety net, not a dependency.Also included
index.html, butsw.jswas maintained by hand with the pre-hash names, so every entry 404'd andcache.add(...).catch(() => {})hid it. Verified against a running instance: 15 of 23 entries failed. Offline still worked (the fetch handler caches every successful GET at runtime), so this is smaller than it looks — but the list was dead code that looked alive, andCACHE_NAMEbeing the constant'codeman-v1'meantactivate's cleanup never deleted anything, so hashed assets from every past release accumulated forever. Both are now derived from the build's own manifest, withtest/sw-precache-manifest.test.tspinning the contract.\ninto one localStorage value and beaconed, and at least one call site interpolates a server-controlled WS closereason— a newline there forges entries. Now flattened and length-capped.Testing
npm test— green, with one pre-existing failure unrelated to this change (cron-service.test.ts"blocks a sensitive system file via the blocklist"), confirmed failing identically on a cleanorigin/masterin this sandbox.npm run typecheck,npm run lint,npm run format:check,npm run check:frontend-syntax,npm run check:public-assets— all green.test/terminal-resilience.test.ts(14),test/sw-precache-manifest.test.ts(6),test/xterm-private-api.test.ts(3). All pure/static so they run in the gate, which is deliberate — the mobile suite is excluded from CI.test/history-truncation-notice.test.ts— one static source guard updated for the renamed call; the behaviour it pins (bounded shell tail first, full history user-triggered) is unchanged.Not verified here
No browser on the machine I worked on, so there is no runtime verification: the renderer freeze in §1 is reasoned from xterm's source plus upstream forensics, not reproduced on a device, and the reconnect gap in §3 is traced through code rather than observed by dropping a phone off a network. Both are worth a real-device pass before release. The watchdog is the item I'd most want eyes on, since it touches xterm internals.
🤖 Generated with Claude Code