diff --git a/web/src/attach.ts b/web/src/attach.ts index 3886680..a99734e 100644 --- a/web/src/attach.ts +++ b/web/src/attach.ts @@ -65,9 +65,17 @@ export interface AttachOptions { * dropped — but NOT on an intentional `dispose()` (eviction / user close). The * shell uses this to auto-close the pane rather than leave a dead one. */ onClosed?: (sessionId: Uuid) => void; + /** Invoked when the handshake itself never completed — no retry was + * attempted (see `shouldReconnect`'s `everOpened`). Unlike `onClosed`, this + * is not "the session is gone": it is at least as likely a dormant + * session's `409 not_attachable` caught mid-status-update (#168 review). + * The shell re-renders rather than tearing the pane's leaf out, so a + * dormant session's resume affordance can take over in place. */ + onNeverAttached?: (sessionId: Uuid) => void; /** Asked on every close: should the socket be retried? The shell answers - * from the catalog — see `shouldRetryClose`. */ - shouldReconnect?: (sessionId: Uuid, code: number) => boolean; + * from the catalog — see `shouldRetryClose`. `everOpened` is false when + * this attempt never reached `open`. */ + shouldReconnect?: (sessionId: Uuid, code: number, everOpened: boolean) => boolean; } /** What the header shows about the session (name / command). Optional — falls @@ -379,6 +387,10 @@ export function createAttachConnection( let ws: WebSocket; let reconnectMs = INITIAL_RECONNECT_MS; let reconnectTimer: number | null = null; + // #168 review: a fresh attempt that never reaches `open` is a rejected + // handshake (dormant/unknown session), not a dropped stream — see + // `shouldRetryClose`'s `everOpened` param. + let everOpened = false; const sendControl = (msg: ClientControl) => { if (ws.readyState !== WebSocket.OPEN) return; @@ -402,6 +414,7 @@ export function createAttachConnection( ws.binaryType = "arraybuffer"; ws.onopen = () => { + everOpened = true; reconnectMs = INITIAL_RECONNECT_MS; setState("connected", "open"); sendControl({ type: "Resize", cols: term.cols, rows: term.rows }); @@ -444,7 +457,7 @@ export function createAttachConnection( // close handle their own pane teardown. if (disposed) return; - if (opts.shouldReconnect?.(sessionId, ev.code)) { + if (opts.shouldReconnect?.(sessionId, ev.code, everOpened)) { const delay = Math.min(reconnectMs, MAX_RECONNECT_MS); setState("reconnecting…", "connecting"); reconnectTimer = window.setTimeout(() => { @@ -455,6 +468,12 @@ export function createAttachConnection( return; } + if (!everOpened) { + setState("not attached", "closed"); + opts.onNeverAttached?.(sessionId); + return; + } + setState(`closed (${ev.code})`, "closed"); opts.onClosed?.(sessionId); }; diff --git a/web/src/connections.ts b/web/src/connections.ts index 1f17afe..22c0ffa 100644 --- a/web/src/connections.ts +++ b/web/src/connections.ts @@ -25,8 +25,13 @@ export interface PoolOptions { /** Fired when a connection's socket closes on its own (session ended/dropped), * not via an intentional close/eviction. The embedder closes the pane. */ onClosed?: (id: Uuid) => void; + /** Fired when a connection's handshake never completed and so was never + * retried (see `AttachOptions.onNeverAttached`). Unlike `onClosed`, the + * embedder should not assume the session is gone — a dormant session's + * pane wants its resume affordance, not teardown. */ + onNeverAttached?: (id: Uuid) => void; /** Forwarded to each connection: should a dropped socket be retried? */ - shouldReconnect?: (id: Uuid, code: number) => boolean; + shouldReconnect?: (id: Uuid, code: number, everOpened: boolean) => boolean; } /** @@ -85,7 +90,9 @@ export class ConnectionPool { { onRequestClose: (sid) => this.opts.onRequestClose?.(sid), onClosed: (sid) => this.opts.onClosed?.(sid), - shouldReconnect: (sid, code) => this.opts.shouldReconnect?.(sid, code) ?? false, + onNeverAttached: (sid) => this.opts.onNeverAttached?.(sid), + shouldReconnect: (sid, code, everOpened) => + this.opts.shouldReconnect?.(sid, code, everOpened) ?? false, }, info, ); diff --git a/web/src/reconnect.test.ts b/web/src/reconnect.test.ts index e885201..b4f296c 100644 --- a/web/src/reconnect.test.ts +++ b/web/src/reconnect.test.ts @@ -47,6 +47,13 @@ describe("shouldRetryClose", () => { it("never retries a clean close", () => { expect(shouldRetryClose("a", 1000, running)).toBe(false); }); + + // #168 review: a handshake that never opened is a rejection (e.g. a + // dormant session's 409 not_attachable), not a dropped stream. Retrying it + // loops forever if the catalog status hasn't caught up yet. + it("never retries a socket that never opened, even if the catalog still says running", () => { + expect(shouldRetryClose("a", 1006, running, false)).toBe(false); + }); }); describe("backoff bounds", () => { diff --git a/web/src/reconnect.ts b/web/src/reconnect.ts index 14624a0..2457ad9 100644 --- a/web/src/reconnect.ts +++ b/web/src/reconnect.ts @@ -26,7 +26,15 @@ export function shouldRetryClose( sessionId: Uuid, code: number, sessions: ReadonlyMap, + /** Did this socket ever reach `open`? A handshake that never completed is + * the daemon rejecting the attach (dormant/unknown) rather than a dropped + * stream — retrying it spins in place until the catalog updates, which for + * a dormant session it never will (#168 review). Defaults to `true` so + * existing callers that only track drops of an established link are + * unaffected. */ + everOpened = true, ): boolean { if (code >= APPLICATION_CLOSE || code === CLEAN_CLOSE) return false; + if (!everOpened) return false; return sessions.get(sessionId)?.status === "Running"; }