diff --git a/src/servers/api/terminal/pty-sidecar.mjs b/src/servers/api/terminal/pty-sidecar.mjs index c09d2999..f2b4f5a1 100644 --- a/src/servers/api/terminal/pty-sidecar.mjs +++ b/src/servers/api/terminal/pty-sidecar.mjs @@ -41,9 +41,17 @@ const sessions = new Map(); // ── Helpers ── -const sendJson = (ws, msg) => { +// Always resolve the CURRENT registration socket, never one captured in a closure. +// +// `term.onData` used to close over the socket that was live when the session was created. Officer is a +// PM2 peer that restarts often, and each restart gives this process a brand new socket — so every +// pre-existing session went on writing to a closed one, where the readyState check below dropped it +// silently. The shell stayed alive and kept accepting input (that arrives on the new socket), but its +// output never came back: the terminal looked frozen until you closed the panel. Reading the module +// variable at send time is the whole fix. +const sendJson = (msg) => { try { - if (ws.readyState === WebSocket.OPEN) { + if (ws && ws.readyState === WebSocket.OPEN) { ws.send(JSON.stringify(msg)); } } catch { @@ -86,7 +94,7 @@ const appendBuffer = (session, data) => { // ── Command handler ── -async function handleCommand(ws, msg) { +async function handleCommand(msg) { switch (msg.type) { case 'pty:init': { const { sessionId, config } = msg; @@ -96,9 +104,12 @@ async function handleCommand(ws, msg) { console.log(`[pty-sidecar] init sessionId=${sessionId} existing=${!!existing} total=${sessions.size}`); if (existing) { - // Replay buffer + // Re-attach. The scrollback goes out as `pty:replay`, not as ordinary output, because the client + // may already be showing some of it: after an officer restart the browser keeps its terminal and + // reconnects, so replaying blind appended a second copy of everything on screen. Marked as + // history, the client can reset and rebuild from it instead. if (existing.buffer.length > 0) { - sendJson(ws, { type: 'pty:output', sessionId, data: existing.buffer }); + sendJson({ type: 'pty:replay', sessionId, data: existing.buffer }); } // Resize PTY to new client dimensions @@ -114,7 +125,7 @@ async function handleCommand(ws, msg) { } } - sendJson(ws, { type: 'pty:ready', id: msg.id, sessionId }); + sendJson({ type: 'pty:ready', id: msg.id, sessionId }); return; } @@ -163,8 +174,8 @@ async function handleCommand(ws, msg) { }); } catch (err) { const message = err instanceof Error ? err.message : 'Failed to start terminal'; - sendJson(ws, { type: 'pty:output', sessionId, data: `\r\n[Terminal error] ${message}\r\n` }); - sendJson(ws, { type: 'pty:exit', sessionId, exitCode: 1 }); + sendJson({ type: 'pty:output', sessionId, data: `\r\n[Terminal error] ${message}\r\n` }); + sendJson({ type: 'pty:exit', sessionId, exitCode: 1 }); return; } @@ -179,16 +190,16 @@ async function handleCommand(ws, msg) { term.onData((output) => { appendBuffer(session, output); - sendJson(ws, { type: 'pty:output', sessionId, data: output }); + sendJson({ type: 'pty:output', sessionId, data: output }); }); term.onExit(({ exitCode, signal }) => { console.log(`[pty-sidecar] session ${sessionId} exited code=${exitCode} signal=${signal}`); - sendJson(ws, { type: 'pty:exit', sessionId, exitCode, signal }); + sendJson({ type: 'pty:exit', sessionId, exitCode, signal }); sessions.delete(sessionId); }); - sendJson(ws, { type: 'pty:ready', id: msg.id, sessionId }); + sendJson({ type: 'pty:ready', id: msg.id, sessionId }); return; } @@ -251,7 +262,7 @@ function connect() { ws.on('open', () => { reconnectAttempt = 0; console.log('[pty-sidecar] connected, sending registration...'); - sendJson(ws, { type: 'register', name: 'pty', capabilities: ['terminal'] }); + sendJson({ type: 'register', name: 'pty', capabilities: ['terminal'] }); }); ws.on('message', (data) => { @@ -263,7 +274,7 @@ function connect() { return; } - handleCommand(ws, msg); + handleCommand(msg); } catch { // skip malformed messages } diff --git a/src/servers/api/terminal/pty-sidecar.test.ts b/src/servers/api/terminal/pty-sidecar.test.ts new file mode 100644 index 00000000..ba774e01 --- /dev/null +++ b/src/servers/api/terminal/pty-sidecar.test.ts @@ -0,0 +1,137 @@ +import { describe, test, expect, afterAll } from 'bun:test'; +import type { ServerWebSocket, Subprocess } from 'bun'; + +// Integration test for the one thing about the pty sidecar that cannot be reasoned about from the code +// alone: what happens to a live shell when officer goes away and comes back. It stands up a fake +// registration socket, runs the real sidecar against it, then kills the socket and rebinds the same port +// the way `pm2 restart officer` does. +// +// Nothing here touches the running officer — the sidecar dials API_URL, which is overridden per spawn. + +const SIDECAR = 'src/servers/api/terminal/pty-sidecar.mjs'; + +type Frame = Record; + +// One fake officer. `stop()` drops the socket; a new instance on the same port is the restart. +function fakeOfficer(port?: number) { + let socket: ServerWebSocket | null = null; + const frames: Frame[] = []; + const waiters: Array<{ match: (f: Frame) => boolean; resolve: (f: Frame) => void }> = []; + + const server = Bun.serve({ + port: port ?? 0, + fetch(req, srv) { + if (new URL(req.url).pathname === '/api/sidecar/register' && srv.upgrade(req)) return undefined; + return new Response('no', { status: 404 }); + }, + websocket: { + open(ws) { + socket = ws; + }, + message(_ws, raw) { + const frame = JSON.parse(String(raw)) as Frame; + frames.push(frame); + for (let i = waiters.length - 1; i >= 0; i--) { + if (waiters[i]!.match(frame)) waiters.splice(i, 1)[0]!.resolve(frame); + } + }, + close() { + socket = null; + }, + }, + }); + + return { + port: server.port, + send: (msg: Frame) => socket?.send(JSON.stringify(msg)), + /** Resolve on the first frame matching `match`, including ones already received. */ + await: (match: (f: Frame) => boolean, timeoutMs = 15_000) => + new Promise((resolve, reject) => { + const seen = frames.find(match); + if (seen) return resolve(seen); + const timer = setTimeout(() => reject(new Error(`timed out waiting for a frame`)), timeoutMs); + waiters.push({ + match, + resolve: (f) => { + clearTimeout(timer); + resolve(f); + }, + }); + }), + /** Output frames for one session, concatenated — the terminal's visible text. */ + outputFor: (sessionId: string) => + frames + .filter((f) => f.type === 'pty:output' && f.sessionId === sessionId) + .map((f) => f.data as string) + .join(''), + stop: () => server.stop(true), + }; +} + +const isOutput = (sessionId: string, needle: string) => (f: Frame) => + f.type === 'pty:output' && f.sessionId === sessionId && String(f.data).includes(needle); + +let child: Subprocess | null = null; +afterAll(() => child?.kill()); + +describe('pty sidecar', () => { + test('a shell keeps streaming output after officer restarts under it', async () => { + const sessionId = 'test-restart'; + let officer = fakeOfficer(); + const port = officer.port; + + child = Bun.spawn(['node', SIDECAR], { + env: { ...process.env, API_URL: `ws://127.0.0.1:${port}` }, + stdout: 'ignore', + stderr: 'ignore', + }); + + await officer.await((f) => f.type === 'register' && f.capabilities?.includes('terminal')); + + // A plain non-interactive shell: no rc files, no prompt, so the assertions below are about output + // the test itself caused. + officer.send({ + type: 'pty:init', + id: 'i1', + sessionId, + config: { + sessionId, + host: true, + shell: { command: '/bin/sh', args: [] }, + cwd: '/tmp', + homeDir: '/tmp', + cols: 80, + rows: 24, + }, + }); + await officer.await((f) => f.type === 'pty:ready' && f.sessionId === sessionId); + + officer.send({ type: 'pty:input', id: 'in1', sessionId, data: 'echo before-restart\n' }); + await officer.await(isOutput(sessionId, 'before-restart')); + + // ── the restart ── + officer.stop(); + officer = fakeOfficer(port); + await officer.await((f) => f.type === 'register'); + + // The shell is the same process; only officer changed. Before the sendJson fix this input was + // accepted and executed, but its output went to the socket captured at init time and vanished. + officer.send({ type: 'pty:input', id: 'in2', sessionId, data: 'echo after-restart\n' }); + await officer.await(isOutput(sessionId, 'after-restart')); + + // Re-attaching replays the scrollback, marked as history so the client can rebuild rather than + // append — and it contains what happened on both sides of the restart. + officer.send({ type: 'pty:init', id: 'i2', sessionId, config: { sessionId, host: true, cols: 80, rows: 24 } }); + const replay = await officer.await((f) => f.type === 'pty:replay' && f.sessionId === sessionId); + expect(String(replay.data)).toContain('before-restart'); + expect(String(replay.data)).toContain('after-restart'); + + // Re-attach must not spawn a second shell, and must not replay as ordinary output. + await officer.await((f) => f.type === 'pty:ready' && f.id === 'i2'); + expect(officer.outputFor(sessionId)).not.toContain('before-restart'); + + officer.send({ type: 'pty:close', id: 'c1', sessionId }); + await officer.await((f) => f.type === 'pty:exit' && f.sessionId === sessionId); + officer.stop(); + }, 30_000); +}); diff --git a/src/servers/api/terminal/websocket.ts b/src/servers/api/terminal/websocket.ts index 682059b3..045fe9e0 100644 --- a/src/servers/api/terminal/websocket.ts +++ b/src/servers/api/terminal/websocket.ts @@ -16,8 +16,7 @@ type WSData = { type BridgeSession = { client: ServerWebSocket; sessionId: string; - unsubOutput: (() => void) | null; - unsubExit: (() => void) | null; + unsubs: Array<() => void>; }; const sessions = new Map, BridgeSession>(); @@ -67,28 +66,22 @@ export const terminalWebsocket = { rows: ws.data.rows, }; - // Subscribe to events for this session - const unsubOutput = on('pty:output', (msg) => { - if (msg.type === 'pty:output' && msg.sessionId === sessionId) { + // The sidecar emits one global stream, so each frame is filtered down to this session and relabelled. + const relay = (event: 'pty:output' | 'pty:replay' | 'pty:exit', clientType: string) => + on(event, (msg) => { + if (msg.type !== event || msg.sessionId !== sessionId) return; try { - ws.send(JSON.stringify({ type: 'output', data: msg.data })); + ws.send(JSON.stringify({ type: clientType, data: 'data' in msg ? msg.data : undefined })); } catch { // ws already closed } - } - }); + }); - const unsubExit = on('pty:exit', (msg) => { - if (msg.type === 'pty:exit' && msg.sessionId === sessionId) { - try { - ws.send(JSON.stringify({ type: 'exit' })); - } catch { - // ws already closed - } - } - }); - - const session: BridgeSession = { client: ws, sessionId, unsubOutput, unsubExit }; + const session: BridgeSession = { + client: ws, + sessionId, + unsubs: [relay('pty:output', 'output'), relay('pty:replay', 'replay'), relay('pty:exit', 'exit')], + }; sessions.set(ws, session); // Send init command to PTY sidecar @@ -97,8 +90,7 @@ export const terminalWebsocket = { } catch (err) { const message = err instanceof Error ? err.message : 'Failed to initialize terminal'; sendOutput(ws, `\r\n[Terminal error] ${message}\r\n`); - unsubOutput(); - unsubExit(); + for (const unsub of session.unsubs) unsub(); sessions.delete(ws); } }, @@ -145,8 +137,7 @@ export const terminalWebsocket = { close(ws: ServerWebSocket) { const session = sessions.get(ws); if (session) { - session.unsubOutput?.(); - session.unsubExit?.(); + for (const unsub of session.unsubs) unsub(); // Don't kill PTY — it can be reattached sessions.delete(ws); } diff --git a/src/servers/sidecar/protocol.ts b/src/servers/sidecar/protocol.ts index 2386e42a..522df93d 100644 --- a/src/servers/sidecar/protocol.ts +++ b/src/servers/sidecar/protocol.ts @@ -153,4 +153,7 @@ export type PtyCommand = export type PtyEvent = | { type: 'pty:ready'; id: string; sessionId: string } | { type: 'pty:output'; sessionId: string; data: string } + // Scrollback sent on re-attach, which the client may already be showing in part — distinct from + // `pty:output` so it can rebuild the screen rather than append a second copy of it. + | { type: 'pty:replay'; sessionId: string; data: string } | { type: 'pty:exit'; sessionId: string; exitCode: number; signal?: number }; diff --git a/src/workspaces/officerdev/src/apps/Terminal/Terminal.tsx b/src/workspaces/officerdev/src/apps/Terminal/Terminal.tsx index 97af268c..8e4963f8 100644 --- a/src/workspaces/officerdev/src/apps/Terminal/Terminal.tsx +++ b/src/workspaces/officerdev/src/apps/Terminal/Terminal.tsx @@ -218,6 +218,14 @@ export const TerminalView = ({ onCommandDoneRef.current(exitCode, output); } } + } else if (msg.type === 'replay') { + // Scrollback for a session we are re-attaching to. On a page load this terminal is empty and + // the reset is a no-op; after an officer restart it still holds what it had before the socket + // dropped, and the replay overlaps it — so rebuild from the sidecar's copy rather than append + // a second one. Deliberately outside the `output` branch: replay must not re-trigger the + // command/initial-input logic above. + term.reset(); + term.write(msg.data); } else if (msg.type === 'exit') { processExited = true; term.write('\r\n[Process exited]\r\n');