From 0ec17633ba5b0bac2f2541f55eb905e5cb5a8370 Mon Sep 17 00:00:00 2001 From: Codeman maintainer Date: Tue, 6 Oct 2026 09:54:57 +0200 Subject: [PATCH] feat(split): Pane B reconnects after a drop instead of staying dead A Codeman restart (every deploy) or a network blip used to leave Pane B dead, with a marker asking the user to close and reopen the split. TerminalTile now reconnects: - A transient close reconnects on the primary pane's backoff ladder (CodemanWsReconnect) plus jitter; the attempt count resets only on a successful open. The redelivery sweep's forced close (1005) counts as transient. - On reopen the closed state is cleared before the buffer refresh that closes the output gap, so no stale marker lands under a healthy pane. - 4003/4004/4009/4010 stop the pane for good and report once through a new onExit(code) callback; the marker says why. - Sockets are replaced race-free: the old one is detached before a new one opens, and every handler ignores events from a socket that is no longer current. destroy() cancels a pending reconnect. - reconnectNow() lets an owner skip the backoff. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/web/public/terminal-tile.js | 185 ++++++++++++++++++++++++------ test/terminal-tile-input.test.ts | 188 +++++++++++++++++++++++++++++-- test/terminal-tile-unit.test.ts | 16 ++- 3 files changed, 342 insertions(+), 47 deletions(-) diff --git a/src/web/public/terminal-tile.js b/src/web/public/terminal-tile.js index 0ca554f2..c80747b2 100644 --- a/src/web/public/terminal-tile.js +++ b/src/web/public/terminal-tile.js @@ -95,6 +95,18 @@ // this pane's socket is open, so the exactly-once input queue delivers this // session's keystrokes over it (app.js _inputSocketFor). Null otherwise. this._inputHandle = null; + // Reconnect state. `_socketUrl` is set once connect() opens the first + // socket: a pane that never connected has nothing to reconnect to. + // `_reconnectAttempts` counts consecutive failed opens and is reset ONLY by + // a successful open (resetting it per attempt is the tight-loop bug the + // primary pane's _disconnectWs documents). `_stoppedCode` is the close code + // that ended the pane for good; `onExit(code)` tells the owner once. + this._socketUrl = null; + this._reconnectAttempts = 0; + this._reconnectTimer = null; + this._stoppedCode = null; + this._markerText = TerminalTile.MARKER_RECONNECTING; + this.onExit = typeof opts.onExit === 'function' ? opts.onExit : null; } async connect() { @@ -274,16 +286,29 @@ const app = global.app; const cid = app?._clientId ? `${app._clientId}:${app._wsTabNonce}:tile` : ''; const cidQuery = cid ? `?cid=${encodeURIComponent(cid)}` : ''; - const url = `${proto}//${location.host}${window.CodemanBase.base}/ws/sessions/${this.sessionId}/terminal${cidQuery}`; - this.ws = new WebSocket(url); + this._socketUrl = `${proto}//${location.host}${window.CodemanBase.base}/ws/sessions/${this.sessionId}/terminal${cidQuery}`; + this._openSocket(); + } - this.ws.onopen = () => { - this._wsReady = true; - this._registerInputSocket(); - this._sendResize(); + // Opens a socket and makes it THE socket. A previous one is detached first + // (handlers nulled, then closed), and every handler below checks it still + // belongs to the current socket: a replacement opened while the old socket + // still looked alive (a half-open connection whose close has not landed) + // makes the server supersede the old one with a 4010, and that late close + // must not stop a pane that is already running on its successor. + _openSocket() { + if (this._destroyed || !this._socketUrl) return; + this._detachSocket(); + const ws = new WebSocket(this._socketUrl); + this.ws = ws; + + ws.onopen = () => { + if (ws !== this.ws) return; + this._onSocketOpen(); }; - this.ws.onmessage = (event) => { + ws.onmessage = (event) => { + if (ws !== this.ws) return; if (this._inputHandle) this._inputHandle.lastRecvAt = Date.now(); try { const msg = JSON.parse(event.data); @@ -294,7 +319,7 @@ } else if (msg.t === 'r') { // Server-triggered refresh (SSE backpressure cleared, terminal // data was dropped). The primary pane routes this to - // _onSessionNeedsRefresh (app.js:2990) — Pane B has its own + // _onSessionNeedsRefresh (app.js) — Pane B has its own // buffer loader for the same reason connect() does. this._refreshBuffer(); } else if (msg.t === 'ia') { @@ -306,24 +331,68 @@ } }; - // Mirror app.js's onclose/onerror pattern (app.js:2905-2964): _wsReady - // must go false on a drop or fit()/_sendResize() silently no-ops on a - // closed socket per the WebSocket spec (no exception, no log). No - // reconnect logic here — Pane B is deliberately plainer than the - // primary pane (see the fileoverview above); a drop just stops - // resizing until the parent recreates the pane. But onData already - // silently drops keystrokes while _wsReady is false (below), so - // without a visible marker a dropped socket left Pane B looking - // normal while it quietly ate everything typed into it. v1 scope is - // "say so", not reconnect — collapsing the split would lose the - // user's place in Pane B's scrollback for a transient blip. - this.ws.onclose = () => this._onSocketClosed(); + // _wsReady must go false on a drop or fit()/_sendResize() silently + // no-op on a closed socket per the WebSocket spec (no exception, no log). + // Input is not lost meanwhile: it waits in the app's durable queue and + // goes out over HTTP or the next socket. The "disconnected" marker says + // so on screen, and a transient close reconnects (_onSocketClosed). + ws.onclose = (event) => { + if (ws !== this.ws) return; + this._onSocketClosed(event); + }; - this.ws.onerror = () => { + ws.onerror = () => { // onclose fires after onerror — cleanup happens there. }; } + // Lets go of the current socket without running its close handling. + _detachSocket() { + const ws = this.ws; + if (!ws) return; + ws.onopen = null; + ws.onmessage = null; + // onclose fires asynchronously AFTER close(); without this it ran its + // "disconnected" write against a pane already torn down or replaced. + ws.onclose = null; + ws.onerror = null; + try { + ws.close(); + } catch { + /* Already closed. */ + } + this.ws = null; + this._wsReady = false; + this._unregisterInputSocket(); + } + + // A socket came up. After a drop this is a reconnect: the gap left nothing + // to replay (output frames carry no sequence number), so the buffer is + // refreshed. The closed state is reset FIRST, or the refresh would re-owe + // the "disconnected" marker (_refreshBuffer does on a closed socket) and + // stamp it under a healthy pane. + _onSocketOpen() { + const reconnected = this._wsClosed; + this._wsReady = true; + this._wsClosed = false; + this._markerOwed = false; + this._reconnectAttempts = 0; + this._registerInputSocket(); + this._sendResize(); + if (reconnected) this._refreshBuffer(); + } + + // Opens a replacement socket now instead of waiting out the backoff (for an + // owner that just learned the server is back). No-op while the current + // socket is open, after a permanent stop, or once destroyed. + reconnectNow() { + if (this._destroyed || this._stoppedCode !== null || !this._socketUrl) return; + if (this.ws && this.ws.readyState === WebSocket.OPEN) return; + clearTimeout(this._reconnectTimer); + this._reconnectTimer = null; + this._openSocket(); + } + // The socket's close, split out of connect() so the tests can drive it. // While any load runs (a history pull or a `{t:'r'}` refresh) the marker is // only owed, and that load's finally block settles it (_stampMarkerIfOwed()): @@ -332,12 +401,56 @@ // replay, or in the middle of a chunked replay. A pull still waiting for its // response holds the marker too, for as long as the request takes (up to its // budget, see _pullHistory()). - _onSocketClosed() { + // + // Then decides what comes next. Codes that cannot get better stop the pane + // for good and report once through `onExit(code)`: 4004/4009 (the session + // is gone), 4003 (refused: Host/Origin/owner, a retry gets the same answer) + // and 4010 (another socket with this pane's cid took over; only ever + // reaches here for the CURRENT socket, see _openSocket). Everything else, + // including the redelivery sweep force-closing a silent socket (1005), is + // transient and reconnects on the primary pane's backoff ladder + // (CodemanWsReconnect, constants.js) plus jitter. + _onSocketClosed(event) { this._wsReady = false; this._wsClosed = true; this._unregisterInputSocket(); + const code = event?.code; + const permanent = TerminalTile.STOP_MARKERS[code]; + this._markerText = permanent || TerminalTile.MARKER_RECONNECTING; if (this._bufferLoading) this._markerOwed = true; else this._writeDisconnectedMarker(); + if (this._destroyed) return; + if (permanent) { + this._stop(code); + return; + } + this._scheduleReconnect(code); + } + + _scheduleReconnect(code) { + if (this._destroyed || !this._socketUrl || this._reconnectTimer) return; + const plan = global.CodemanWsReconnect?.plan?.(code ?? 1006, this._reconnectAttempts) || { + action: 'reconnect', + delayMs: 1000, + }; + if (plan.action === 'give-up') { + this._stop(code); + return; + } + this._reconnectAttempts++; + const delay = plan.delayMs + Math.floor(Math.random() * 250); // jitter: tiles must not reconnect in lockstep + this._reconnectTimer = setTimeout(() => { + this._reconnectTimer = null; + this._openSocket(); + }, delay); + } + + _stop(code) { + if (this._stoppedCode !== null) return; + this._stoppedCode = code ?? null; + clearTimeout(this._reconnectTimer); + this._reconnectTimer = null; + if (!this._destroyed) this.onExit?.(code); } // Keystrokes and pastes go through the app's exactly-once input queue (seq, @@ -398,7 +511,7 @@ // Extracted so both _onSocketClosed() and a load that ends owing it on a // closed socket can write it (see _stampMarkerIfOwed()). _writeDisconnectedMarker() { - this.terminal?.write('\r\n\x1b[2m[Pane B disconnected — close and reopen the split to reconnect]\x1b[0m\r\n'); + this.terminal?.write(`\r\n\x1b[2m${this._markerText}\x1b[0m\r\n`); } // Fetches and writes the session's current scrollback. Used both by @@ -672,21 +785,13 @@ // Anything still queued for this session stays in the app's queue and is // delivered over HTTP by the redelivery sweep, so closing the pane mid- // keystroke loses nothing. - this._unregisterInputSocket(); + clearTimeout(this._reconnectTimer); + this._reconnectTimer = null; if (this._onWheel) { this.mountEl?.removeEventListener('wheel', this._onWheel, { capture: true }); this._onWheel = null; } - if (this.ws) { - this.ws.onopen = null; - this.ws.onmessage = null; - // onclose fires asynchronously AFTER close(); without this it ran - // its "disconnected" write against a pane already torn down. - this.ws.onclose = null; - this.ws.onerror = null; - this.ws.close(); - this.ws = null; - } + this._detachSocket(); if (this.terminal) { this.terminal.dispose(); this.terminal = null; @@ -695,5 +800,17 @@ } } + // The marker a pane writes when its socket drops: a transient drop says it is + // reconnecting; a permanent stop says why, keyed by close code. All start + // with `[disconnected` so a reader (and a test) can tell any of them apart + // from session output. + TerminalTile.MARKER_RECONNECTING = '[disconnected, reconnecting…]'; + TerminalTile.STOP_MARKERS = { + 4003: '[disconnected: the server refused this connection]', + 4004: '[disconnected: the session ended]', + 4009: '[disconnected: the session ended]', + 4010: '[disconnected: another connection took over this pane]', + }; + global.TerminalTile = TerminalTile; })(window); diff --git a/test/terminal-tile-input.test.ts b/test/terminal-tile-input.test.ts index 6f89758a..b7e38889 100644 --- a/test/terminal-tile-input.test.ts +++ b/test/terminal-tile-input.test.ts @@ -10,6 +10,12 @@ * its own: a query reply (DA/CPR/OSC) is dropped, exactly as the primary pane * drops it, and a focus or mouse report goes out once, ephemeral. * + * The socket's lifecycle is pinned here too: a transient drop reconnects on the + * primary pane's backoff and refreshes the buffer without leaving a stale + * "disconnected" marker under a healthy pane; codes that cannot get better stop + * the pane once (`onExit`); a late close from a REPLACED socket is ignored; and + * destroy() cancels a pending reconnect. + * * Real code under test: constants.js + app.js (the queue) + terminal-ui.js (the * shared input predicates) + terminal-tile.js, in one `vm` context. xterm, the * fit addon and WebSocket are fakes; `connect()` runs for real. @@ -18,7 +24,7 @@ import { readFileSync } from 'node:fs'; import { performance } from 'node:perf_hooks'; import { resolve } from 'node:path'; import vm from 'node:vm'; -import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; type Frame = { t: string; d?: string; seq?: number; cid?: string; c?: number; r?: number }; @@ -70,10 +76,14 @@ class FakeTerminal { } attachCustomKeyEventHandler() {} registerLinkProvider() {} - write(_data: string, cb?: () => void) { + writes: string[] = []; + write(data: string, cb?: () => void) { + this.writes.push(data); cb?.(); } - clear() {} + clear() { + this.writes.push(''); + } resize(cols: number, rows: number) { this.cols = cols; this.rows = rows; @@ -98,8 +108,10 @@ function loadContext() { performance, setInterval: vi.fn(), clearInterval: vi.fn(), - setTimeout, - clearTimeout, + // Late-bound, so vi.useFakeTimers() (which swaps the globals) reaches code + // running inside this context. + setTimeout: (fn: () => void, ms?: number) => globalThis.setTimeout(fn, ms), + clearTimeout: (id: ReturnType) => globalThis.clearTimeout(id), requestAnimationFrame: vi.fn(), HTMLCanvasElement: class HTMLCanvasElement {}, WebSocket: FakeSocket, @@ -163,18 +175,28 @@ function makeApp(): App { type Tile = { connect(): Promise; destroy(): void; + reconnectNow(): void; ws: FakeSocket | null; + _reconnectAttempts: number; }; const TerminalTile = windowStub.TerminalTile as new (id: string, mount: unknown, opts?: object) => Tile; -async function connectTile(app: App) { +async function connectTile(app: App, opts: Record = {}) { windowStub.app = app; - const tile = new TerminalTile('s-tile', { addEventListener: vi.fn(), removeEventListener: vi.fn() }, { mode: 'claude' }); + const tile = new TerminalTile( + 's-tile', + { addEventListener: vi.fn(), removeEventListener: vi.fn() }, + { mode: 'claude', ...opts } + ); await tile.connect(); const ws = FakeSocket.instances.at(-1)!; return { tile, ws, term: FakeTerminal.last! }; } +afterEach(() => { + vi.useRealTimers(); +}); + beforeEach(() => { FakeSocket.instances = []; fetchMock.mockReset(); @@ -319,3 +341,155 @@ describe('TerminalTile leaves the input-socket map', () => { expect(app._pendingDeliveries.get('s-tile')?.map((r) => r.data)).toEqual(['q']); }); }); + +const isMarker = (data: string) => data.includes('[disconnected'); + +/** + * Lets the async buffer refresh settle: the fetch, the body read, the chunked + * write AND the load's finally block, which is where a stale marker would be + * stamped. Too few turns here and that assertion passes vacuously. + */ +async function settle() { + for (let i = 0; i < 50; i++) await Promise.resolve(); +} + +describe('TerminalTile reconnects after a transient drop', () => { + it('reopens on the backoff, refreshes the buffer, and leaves no stale marker', async () => { + vi.useFakeTimers(); + const app = makeApp(); + const { tile, ws, term } = await connectTile(app); + ws.open(); + fetchMock.mockImplementation(async () => ({ + ok: true, + status: 200, + json: async () => ({ data: { terminalBuffer: 'fresh screen' } }), + })); + + ws.readyState = 3; + ws.onclose?.({ code: 1006 }); + expect(term.writes.filter(isMarker)).toEqual([expect.stringContaining('[disconnected, reconnecting')]); + expect(FakeSocket.instances).toHaveLength(1); + + await vi.advanceTimersByTimeAsync(300); + expect(FakeSocket.instances).toHaveLength(2); + const ws2 = FakeSocket.instances[1]; + expect(ws2.url).toBe(ws.url); + + ws2.open(); + await settle(); + + // The refresh cleared the pane and replayed the current screen, and nothing + // after that clear is a marker: the pane is healthy again. + const lastClear = term.writes.lastIndexOf(''); + expect(lastClear).toBeGreaterThan(-1); + expect(term.writes.slice(lastClear)).toContain('fresh screen'); + expect(term.writes.slice(lastClear).some(isMarker)).toBe(false); + expect(tile._reconnectAttempts).toBe(0); + expect(tile.ws).toBe(ws2); + }); + + it('counts failed attempts and resets the count only on a successful open', async () => { + vi.useFakeTimers(); + const { tile, ws } = await connectTile(makeApp()); + ws.open(); + + ws.onclose?.({ code: 1006 }); + await vi.advanceTimersByTimeAsync(300); + FakeSocket.instances[1].onclose?.({ code: 1006 }); // the retry fails too + expect(tile._reconnectAttempts).toBe(2); + + await vi.advanceTimersByTimeAsync(1000); + FakeSocket.instances[2].open(); + expect(tile._reconnectAttempts).toBe(0); + }); + + it('reconnects after the redelivery sweep force-closes a silent socket (1005)', async () => { + vi.useFakeTimers(); + const { ws } = await connectTile(makeApp()); + ws.open(); + + ws.onclose?.({ code: 1005 }); + await vi.advanceTimersByTimeAsync(300); + + expect(FakeSocket.instances).toHaveLength(2); + }); + + it('reconnectNow() skips the backoff, but never replaces an open socket', async () => { + vi.useFakeTimers(); + const { tile, ws } = await connectTile(makeApp()); + ws.open(); + + tile.reconnectNow(); + expect(FakeSocket.instances).toHaveLength(1); + + ws.readyState = 3; + ws.onclose?.({ code: 1006 }); + tile.reconnectNow(); + expect(FakeSocket.instances).toHaveLength(2); + // The backoff timer it pre-empted must not open a third socket later. + await vi.advanceTimersByTimeAsync(20_000); + expect(FakeSocket.instances).toHaveLength(2); + }); +}); + +describe('TerminalTile stops for good on codes that cannot get better', () => { + it.each([ + [4004, 'the session ended'], + [4009, 'the session ended'], + [4003, 'the server refused this connection'], + [4010, 'another connection took over this pane'], + ])('close %i: no reconnect, onExit once, marker says why', async (code, reason) => { + vi.useFakeTimers(); + const onExit = vi.fn(); + const { tile, ws, term } = await connectTile(makeApp(), { onExit }); + ws.open(); + + ws.onclose?.({ code }); + await vi.advanceTimersByTimeAsync(30_000); + tile.reconnectNow(); + + expect(FakeSocket.instances).toHaveLength(1); + expect(onExit).toHaveBeenCalledTimes(1); + expect(onExit).toHaveBeenCalledWith(code); + expect(term.writes.filter(isMarker)).toEqual([expect.stringContaining(reason)]); + }); +}); + +describe('TerminalTile ignores a socket it already replaced', () => { + it('a late close (4010) from the old socket neither stops the pane nor unregisters its successor', async () => { + vi.useFakeTimers(); + const onExit = vi.fn(); + const app = makeApp(); + const { ws, term } = await connectTile(app, { onExit }); + ws.open(); + const lateClose = ws.onclose!; + + ws.onclose?.({ code: 1006 }); + await vi.advanceTimersByTimeAsync(300); + const ws2 = FakeSocket.instances[1]; + ws2.open(); + + // The server supersedes the old socket by cid; its close arrives late. + lateClose({ code: 4010 }); + + expect(onExit).not.toHaveBeenCalled(); + term.type('still typing'); + expect(ws2.inputFrames().map((f) => f.d)).toEqual(['still typing']); + }); +}); + +describe('TerminalTile destroy()', () => { + it('cancels a pending reconnect and never reports an exit afterwards', async () => { + vi.useFakeTimers(); + const onExit = vi.fn(); + const { tile, ws } = await connectTile(makeApp(), { onExit }); + ws.open(); + + ws.onclose?.({ code: 1006 }); + tile.destroy(); + await vi.advanceTimersByTimeAsync(30_000); + + expect(FakeSocket.instances).toHaveLength(1); + expect(onExit).not.toHaveBeenCalled(); + }); +}); diff --git a/test/terminal-tile-unit.test.ts b/test/terminal-tile-unit.test.ts index b4fd7058..8a77470e 100644 --- a/test/terminal-tile-unit.test.ts +++ b/test/terminal-tile-unit.test.ts @@ -176,7 +176,8 @@ function deferred() { return { promise, resolve }; } -const isMarker = (data: unknown) => typeof data === 'string' && data.includes('Pane B disconnected'); +// Every marker variant (reconnecting, session ended, refused, taken over) starts the same way. +const isMarker = (data: unknown) => typeof data === 'string' && data.includes('[disconnected'); /** Lets every microtask the vm-side promise chain queued run. */ const settle = () => new Promise((r) => setTimeout(r, 0)); @@ -722,8 +723,11 @@ describe('TerminalTile scroll-to-top history pull', () => { expect(connect).toContain('this._installWheelListener();'); expect(connect).toContain('this._onLiveClear();'); expect(connect).not.toContain('this.terminal.clear();'); - // The tests below drive the close through _onSocketClosed() directly. - expect(connect).toContain('this.ws.onclose = () => this._onSocketClosed();'); + // The tests below drive the close through _onSocketClosed() directly; the + // socket's own handler forwards the close event (and its code) there, and + // only for the current socket (_openSocket). + expect(connect).toContain('this._onSocketClosed(event);'); + expect(connect).toMatch(/ws\.onclose = \(event\) => \{\s*if \(ws !== this\.ws\) return;/); }); it('a close with no pull running writes the marker straight away', () => { @@ -747,9 +751,9 @@ describe('TerminalTile scroll-to-top history pull', () => { void pane._pullHistory(); await settle(); - const marker = expect.stringContaining('Pane B disconnected'); + const marker = expect.stringContaining('[disconnected'); const writes = pane.terminal.write.mock.calls.map((c) => c[0]); - expect(writes.at(-1)).toEqual(expect.stringMatching(/Pane B disconnected/)); + expect(writes.at(-1)).toEqual(expect.stringMatching(/\[disconnected/)); expect(pane.terminal.write).toHaveBeenCalledWith(marker); }); @@ -1004,7 +1008,7 @@ describe('TerminalTile scroll-to-top history pull', () => { await settle(); for (const call of pane.terminal.write.mock.calls) { - expect(call[0]).toEqual(expect.not.stringMatching(/Pane B disconnected/)); + expect(call[0]).toEqual(expect.not.stringMatching(/\[disconnected/)); } });