mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-04 06:29:42 +02:00
fix(terminal): deadline must cover the body, precache must ignore the cache-bust query
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=<mtime>` 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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
c0422c4e21
commit
abd39318e6
@@ -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=<mtime>` 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=<mtime> URLs cacheBustAssets emits').not.toMatch(
|
||||
/caches\.match\(request\)\s*\)/
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<void>((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();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string, string>;
|
||||
const lock = JSON.parse(readFileSync(resolve(root, 'package-lock.json'), 'utf8')) as {
|
||||
packages: Record<string, { version?: 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';
|
||||
// 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
|
||||
|
||||
Reference in New Issue
Block a user