mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-03 14:09:42 +02:00
feat(terminal): renderer watchdog, atomic replay clear, fetch deadlines, 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 commit is contained in:
co-authored by
Claude Opus 5
parent
9466acfc1a
commit
c0422c4e21
@@ -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;");
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
@@ -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<string, string>;
|
||||
};
|
||||
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');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user