mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-03 22:19:42 +02:00
Merge branch 'fix/sse-stale-watchdog'
Heal a stalled SSE stream: the server's :keepalive comment becomes a named sse:heartbeat event (comments are invisible to EventSource by spec), and the client gains a staleness watchdog that forces a reconnect after three missed beats. Also applies a confirmed rename locally instead of waiting on SSE. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Codeman maintainer
@@ -358,6 +358,83 @@ describe('Inline rename input', () => {
|
||||
expect(result.editingAfter).toBe(null);
|
||||
});
|
||||
|
||||
it('Commit writes the confirmed name into app.sessions WITHOUT any session:updated frame', async () => {
|
||||
await resetState();
|
||||
expect(await startRename('no-sse', 'w9-case')).toBe(true);
|
||||
|
||||
// finishRename() re-renders the tab strip from app.sessions, so the rename
|
||||
// used to depend on the session:updated SSE frame to carry its own write
|
||||
// back. On a page whose stream has gone quiet without erroring, the PUT
|
||||
// stored the new name, the re-render repainted the stale one, and the tab
|
||||
// only showed it after a full reload. No SSE is dispatched here at all.
|
||||
const result = await page.evaluate(async () => {
|
||||
const app = (
|
||||
window as unknown as {
|
||||
app: { sessions: Map<string, { id: string; name: string }> };
|
||||
}
|
||||
).app;
|
||||
const origFetch = window.fetch;
|
||||
window.fetch = (async () =>
|
||||
new Response('{"success":true,"data":{"name":"w9-case: fresh"}}', {
|
||||
status: 200,
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
})) as typeof window.fetch;
|
||||
|
||||
const inputEl = document.querySelector('input.tab-rename-input') as HTMLInputElement;
|
||||
inputEl.value = 'fresh';
|
||||
inputEl.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true }));
|
||||
await new Promise((r) => setTimeout(r, 60));
|
||||
|
||||
window.fetch = origFetch;
|
||||
return { mapName: app.sessions.get('no-sse')?.name ?? null };
|
||||
});
|
||||
|
||||
expect(result.mapName).toBe('w9-case: fresh');
|
||||
});
|
||||
|
||||
it('A rejected rename restores the old label and leaves app.sessions untouched', async () => {
|
||||
await resetState();
|
||||
expect(await startRename('rename-500', 'w9-case')).toBe(true);
|
||||
|
||||
// _apiPut turns a network error into a null Response and an API-level
|
||||
// failure arrives as a non-ok status, neither of which throws, so a
|
||||
// rejected rename has to be detected from the response, or it reports
|
||||
// success and silently discards the user's edit.
|
||||
const result = await page.evaluate(async () => {
|
||||
const app = (
|
||||
window as unknown as {
|
||||
app: { sessions: Map<string, { id: string; name: string }>; showToast: (m: string, k: string) => void };
|
||||
}
|
||||
).app;
|
||||
const toasts: string[] = [];
|
||||
const origToast = app.showToast;
|
||||
app.showToast = (msg: string) => void toasts.push(msg);
|
||||
const origFetch = window.fetch;
|
||||
window.fetch = (async () =>
|
||||
new Response('{"success":false,"error":"boom","errorCode":"INTERNAL"}', {
|
||||
status: 500,
|
||||
headers: { 'Content-Type': 'application/json' },
|
||||
})) as typeof window.fetch;
|
||||
|
||||
const inputEl = document.querySelector('input.tab-rename-input') as HTMLInputElement;
|
||||
inputEl.value = 'never-stored';
|
||||
inputEl.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true }));
|
||||
await new Promise((r) => setTimeout(r, 60));
|
||||
|
||||
window.fetch = origFetch;
|
||||
app.showToast = origToast;
|
||||
return {
|
||||
mapName: app.sessions.get('rename-500')?.name ?? null,
|
||||
label: document.querySelector('.tab-name[data-session-id="rename-500"]')?.textContent ?? null,
|
||||
toasts,
|
||||
};
|
||||
});
|
||||
|
||||
expect(result.mapName).toBe('w9-case');
|
||||
expect(result.label).toBe('w9-case');
|
||||
expect(result.toasts).toContain('Failed to rename');
|
||||
});
|
||||
|
||||
it('Re-entry: starting rename while one is active aborts the previous one', async () => {
|
||||
await resetState();
|
||||
expect(await startRename('first-id', 'First')).toBe(true);
|
||||
|
||||
@@ -0,0 +1,145 @@
|
||||
/**
|
||||
* SSE liveness heartbeat.
|
||||
*
|
||||
* `cleanupDeadClients()` runs every SSE_HEARTBEAT_INTERVAL (15s) and does two
|
||||
* jobs: evict clients whose socket died, and write a liveness frame to the
|
||||
* ones that are still up.
|
||||
*
|
||||
* The regression these guard: that frame used to be an SSE `:keepalive`
|
||||
* COMMENT, and comments are invisible to `EventSource` by spec. A stream that
|
||||
* stopped delivering without erroring was therefore undetectable to the
|
||||
* client: `onerror` never fired, the header dot stayed green, and every
|
||||
* SSE-driven surface froze until the user reloaded. A named `sse:heartbeat`
|
||||
* event reaches a listener, which is what lets the client's staleness
|
||||
* watchdog notice the silence (see test/sse-staleness.test.ts).
|
||||
*
|
||||
* No port needed (the manager is driven directly with fake replies).
|
||||
*/
|
||||
import type { FastifyReply } from 'fastify';
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { SSE_PADDING_SIZE } from '../src/config/server-timing.js';
|
||||
import { SseEvent } from '../src/web/sse-events.js';
|
||||
import { SseStreamManager } from '../src/web/sse-stream-manager.js';
|
||||
import { CleanupManager } from '../src/utils/index.js';
|
||||
|
||||
/** A FastifyReply stand-in that records every raw write. */
|
||||
function fakeClient(opts: { destroyed?: boolean; writable?: boolean; throwOnAccess?: boolean } = {}) {
|
||||
const writes: string[] = [];
|
||||
const socket = { destroyed: opts.destroyed ?? false, writable: opts.writable ?? true };
|
||||
const raw = {
|
||||
get socket() {
|
||||
if (opts.throwOnAccess) throw new Error('socket gone');
|
||||
return socket;
|
||||
},
|
||||
write(chunk: string) {
|
||||
writes.push(chunk);
|
||||
return true;
|
||||
},
|
||||
};
|
||||
return { reply: { raw } as unknown as FastifyReply, writes };
|
||||
}
|
||||
|
||||
function makeManager() {
|
||||
const cleanup = new CleanupManager();
|
||||
const manager = new SseStreamManager({ getSessionStateWithRespawn: () => null }, cleanup);
|
||||
return { manager, cleanup };
|
||||
}
|
||||
|
||||
describe('SSE liveness heartbeat', () => {
|
||||
it('writes a NAMED sse:heartbeat event, not an invisible comment', () => {
|
||||
const { manager, cleanup } = makeManager();
|
||||
const client = fakeClient();
|
||||
manager.addClient(client.reply, null, false);
|
||||
|
||||
manager.cleanupDeadClients();
|
||||
|
||||
expect(client.writes).toHaveLength(1);
|
||||
const frame = client.writes[0];
|
||||
// A comment (`:keepalive`) never reaches an EventSource listener, and that is
|
||||
// the entire bug. The frame must be a dispatchable named event.
|
||||
expect(frame.startsWith(':')).toBe(false);
|
||||
expect(frame).toMatch(/^event: sse:heartbeat\n/);
|
||||
expect(frame.endsWith('\n\n')).toBe(true);
|
||||
expect(SseEvent.Heartbeat).toBe('sse:heartbeat');
|
||||
cleanup.dispose();
|
||||
});
|
||||
|
||||
it('carries a parseable epoch-ms payload', () => {
|
||||
const { manager, cleanup } = makeManager();
|
||||
const client = fakeClient();
|
||||
manager.addClient(client.reply, null, false);
|
||||
const before = Date.now();
|
||||
|
||||
manager.cleanupDeadClients();
|
||||
|
||||
const dataLine = client.writes[0].split('\n').find((l) => l.startsWith('data: '));
|
||||
expect(dataLine).toBeDefined();
|
||||
const payload = JSON.parse(dataLine!.slice('data: '.length)) as { t: number };
|
||||
expect(payload.t).toBeGreaterThanOrEqual(before);
|
||||
expect(payload.t).toBeLessThanOrEqual(Date.now());
|
||||
cleanup.dispose();
|
||||
});
|
||||
|
||||
it('still appends Cloudflare tunnel padding when a tunnel is active', () => {
|
||||
const { manager, cleanup } = makeManager();
|
||||
const client = fakeClient();
|
||||
manager.addClient(client.reply, null, false);
|
||||
manager.setTunnelActive(true);
|
||||
|
||||
manager.cleanupDeadClients();
|
||||
|
||||
const frame = client.writes[0];
|
||||
expect(frame).toMatch(/^event: sse:heartbeat\n/);
|
||||
// Padding rides AFTER the terminating blank line, so the event still parses.
|
||||
const [event, padding] = frame.split('\n\n');
|
||||
expect(event).toMatch(/^event: sse:heartbeat\ndata: \{/);
|
||||
expect(padding.startsWith(':')).toBe(true);
|
||||
expect(padding.length).toBeGreaterThanOrEqual(SSE_PADDING_SIZE);
|
||||
cleanup.dispose();
|
||||
});
|
||||
|
||||
it('sends no padding without a tunnel', () => {
|
||||
const { manager, cleanup } = makeManager();
|
||||
const client = fakeClient();
|
||||
manager.addClient(client.reply, null, false);
|
||||
|
||||
manager.cleanupDeadClients();
|
||||
|
||||
expect(client.writes[0].length).toBeLessThan(200);
|
||||
cleanup.dispose();
|
||||
});
|
||||
|
||||
it('still evicts dead clients instead of heartbeating them', () => {
|
||||
const { manager, cleanup } = makeManager();
|
||||
const alive = fakeClient();
|
||||
const destroyed = fakeClient({ destroyed: true });
|
||||
const unwritable = fakeClient({ writable: false });
|
||||
const exploding = fakeClient({ throwOnAccess: true });
|
||||
for (const c of [alive, destroyed, unwritable, exploding]) manager.addClient(c.reply, null, false);
|
||||
expect(manager.clientCount).toBe(4);
|
||||
|
||||
manager.cleanupDeadClients();
|
||||
|
||||
expect(manager.clientCount).toBe(1);
|
||||
expect(alive.writes).toHaveLength(1);
|
||||
for (const c of [destroyed, unwritable, exploding]) expect(c.writes).toHaveLength(0);
|
||||
cleanup.dispose();
|
||||
});
|
||||
|
||||
it('heartbeats every client on each pass', () => {
|
||||
const { manager, cleanup } = makeManager();
|
||||
const a = fakeClient();
|
||||
const b = fakeClient();
|
||||
manager.addClient(a.reply, null, false);
|
||||
manager.addClient(b.reply, null, false);
|
||||
|
||||
manager.cleanupDeadClients();
|
||||
manager.cleanupDeadClients();
|
||||
|
||||
// The frame carries no session data, so it needs no owner routing and is
|
||||
// written per-client rather than through the scoped broadcast() path.
|
||||
expect(a.writes).toHaveLength(2);
|
||||
expect(b.writes).toHaveLength(2);
|
||||
cleanup.dispose();
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,102 @@
|
||||
/**
|
||||
* SSE staleness policy.
|
||||
*
|
||||
* `CodemanSseStale.compute(input)` is the pure decision behind app.js's
|
||||
* watchdog: given when the last SSE frame arrived, the transport status and
|
||||
* the browser's online flag, it says whether the stream has gone quiet while
|
||||
* still claiming to be connected: a zombie that has to be rebuilt.
|
||||
*
|
||||
* The regression it guards: the server's liveness keepalive used to be an SSE
|
||||
* `:keepalive` COMMENT, and comments are invisible to `EventSource` by spec.
|
||||
* A stream that stopped delivering without erroring (a proxy that idle-closed
|
||||
* it, a laptop resumed from sleep, a tailnet reconnect) never fired `onerror`,
|
||||
* so the header dot stayed green and tab status dots, sessions created on
|
||||
* another device, and renames all froze until the user reloaded the page.
|
||||
*
|
||||
* Loaded in a plain node VM context (no jsdom), mirroring
|
||||
* test/connection-loss-ui.test.ts.
|
||||
*/
|
||||
import { readFileSync } from 'node:fs';
|
||||
import { resolve } from 'node:path';
|
||||
import vm from 'node:vm';
|
||||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
type StaleInput = {
|
||||
lastMessageAt?: number | null;
|
||||
now?: number;
|
||||
status?: 'connected' | 'connecting' | 'reconnecting' | 'disconnected' | 'offline';
|
||||
isOnline?: boolean;
|
||||
timeoutMs?: number;
|
||||
};
|
||||
|
||||
function loadPolicy() {
|
||||
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' });
|
||||
return (
|
||||
context.window as {
|
||||
CodemanSseStale: { compute: (input: StaleInput) => boolean; TIMEOUT_MS: number };
|
||||
}
|
||||
).CodemanSseStale;
|
||||
}
|
||||
|
||||
const T0 = 1_000_000;
|
||||
|
||||
describe('SSE staleness policy', () => {
|
||||
it('defaults to three missed 15s heartbeats', () => {
|
||||
const { TIMEOUT_MS } = loadPolicy();
|
||||
expect(TIMEOUT_MS).toBe(45000);
|
||||
});
|
||||
|
||||
it('is not stale while frames keep arriving', () => {
|
||||
const { compute, TIMEOUT_MS } = loadPolicy();
|
||||
expect(compute({ lastMessageAt: T0, now: T0 + TIMEOUT_MS - 1, status: 'connected' })).toBe(false);
|
||||
});
|
||||
|
||||
it('is stale once the threshold is reached', () => {
|
||||
const { compute, TIMEOUT_MS } = loadPolicy();
|
||||
// Boundary is inclusive: exactly three missed heartbeats already means the
|
||||
// stream has been silent through a window it was contractually filling.
|
||||
expect(compute({ lastMessageAt: T0, now: T0 + TIMEOUT_MS, status: 'connected' })).toBe(true);
|
||||
expect(compute({ lastMessageAt: T0, now: T0 + TIMEOUT_MS * 10, status: 'connected' })).toBe(true);
|
||||
});
|
||||
|
||||
it('honours a custom timeoutMs (what a browser test shrinks)', () => {
|
||||
const { compute } = loadPolicy();
|
||||
expect(compute({ lastMessageAt: T0, now: T0 + 999, status: 'connected', timeoutMs: 1000 })).toBe(false);
|
||||
expect(compute({ lastMessageAt: T0, now: T0 + 1000, status: 'connected', timeoutMs: 1000 })).toBe(true);
|
||||
});
|
||||
|
||||
it('is never stale while the transport is already reconnecting', () => {
|
||||
const { compute, TIMEOUT_MS } = loadPolicy();
|
||||
// These states already have the backoff machinery running; firing on top
|
||||
// of them would stack reconnects. This guard is also the loop breaker:
|
||||
// a forced reconnect leaves 'connected' immediately, so the watchdog
|
||||
// cannot re-fire while one is in flight.
|
||||
for (const status of ['connecting', 'reconnecting', 'disconnected', 'offline'] as const) {
|
||||
expect(compute({ lastMessageAt: T0, now: T0 + TIMEOUT_MS * 10, status })).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it('is never stale while the device is offline', () => {
|
||||
const { compute, TIMEOUT_MS } = loadPolicy();
|
||||
// Nothing to reconnect to yet; the connection-loss UI already owns this.
|
||||
expect(compute({ lastMessageAt: T0, now: T0 + TIMEOUT_MS * 10, status: 'connected', isOnline: false })).toBe(false);
|
||||
});
|
||||
|
||||
it('is not stale before any frame has ever arrived', () => {
|
||||
const { compute, TIMEOUT_MS } = loadPolicy();
|
||||
// The clock starts at onopen, and `init` lands immediately after. A zero
|
||||
// stamp means the stream has not opened yet, not that it went quiet. The
|
||||
// constructor optimistically seeds status 'connected' before the first
|
||||
// connect, so without this guard the watchdog would fire on page load.
|
||||
for (const lastMessageAt of [0, null, undefined]) {
|
||||
expect(compute({ lastMessageAt, now: T0 + TIMEOUT_MS * 10, status: 'connected' })).toBe(false);
|
||||
}
|
||||
});
|
||||
|
||||
it('tolerates a missing input object', () => {
|
||||
const { compute } = loadPolicy();
|
||||
expect(compute(undefined as unknown as StaleInput)).toBe(false);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user