fix(maestro): stop retrying a WS attach that never opened

Integration review (#168): a rejected attach handshake — a dormant
session's 409 not_attachable, most likely — looks identical to a
dropped stream from the WebSocket API (close code 1006, no reason).
shouldRetryClose kept retrying it on the catalog's Running/Dead check
alone, which loops for as long as the catalog hasn't caught up with
the session going dormant.

Track whether the socket ever reached `open`; a handshake that never
did is a rejection, not a blip, so it is never retried regardless of
catalog status. The caller (wired up in the next commit) gets a
distinct onNeverAttached callback instead of onClosed, so it can offer
the resume affordance rather than assuming the session is gone.
This commit is contained in:
Miguel Palhas
2026-09-04 09:19:09 +01:00
parent 8269bcabcc
commit 938dd62648
4 changed files with 46 additions and 5 deletions
+22 -3
View File
@@ -65,9 +65,17 @@ export interface AttachOptions {
* dropped — but NOT on an intentional `dispose()` (eviction / user close). The * 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. */ * shell uses this to auto-close the pane rather than leave a dead one. */
onClosed?: (sessionId: Uuid) => void; 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 /** Asked on every close: should the socket be retried? The shell answers
* from the catalog — see `shouldRetryClose`. */ * from the catalog — see `shouldRetryClose`. `everOpened` is false when
shouldReconnect?: (sessionId: Uuid, code: number) => boolean; * this attempt never reached `open`. */
shouldReconnect?: (sessionId: Uuid, code: number, everOpened: boolean) => boolean;
} }
/** What the header shows about the session (name / command). Optional — falls /** What the header shows about the session (name / command). Optional — falls
@@ -379,6 +387,10 @@ export function createAttachConnection(
let ws: WebSocket; let ws: WebSocket;
let reconnectMs = INITIAL_RECONNECT_MS; let reconnectMs = INITIAL_RECONNECT_MS;
let reconnectTimer: number | null = null; 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) => { const sendControl = (msg: ClientControl) => {
if (ws.readyState !== WebSocket.OPEN) return; if (ws.readyState !== WebSocket.OPEN) return;
@@ -402,6 +414,7 @@ export function createAttachConnection(
ws.binaryType = "arraybuffer"; ws.binaryType = "arraybuffer";
ws.onopen = () => { ws.onopen = () => {
everOpened = true;
reconnectMs = INITIAL_RECONNECT_MS; reconnectMs = INITIAL_RECONNECT_MS;
setState("connected", "open"); setState("connected", "open");
sendControl({ type: "Resize", cols: term.cols, rows: term.rows }); sendControl({ type: "Resize", cols: term.cols, rows: term.rows });
@@ -444,7 +457,7 @@ export function createAttachConnection(
// close handle their own pane teardown. // close handle their own pane teardown.
if (disposed) return; if (disposed) return;
if (opts.shouldReconnect?.(sessionId, ev.code)) { if (opts.shouldReconnect?.(sessionId, ev.code, everOpened)) {
const delay = Math.min(reconnectMs, MAX_RECONNECT_MS); const delay = Math.min(reconnectMs, MAX_RECONNECT_MS);
setState("reconnecting…", "connecting"); setState("reconnecting…", "connecting");
reconnectTimer = window.setTimeout(() => { reconnectTimer = window.setTimeout(() => {
@@ -455,6 +468,12 @@ export function createAttachConnection(
return; return;
} }
if (!everOpened) {
setState("not attached", "closed");
opts.onNeverAttached?.(sessionId);
return;
}
setState(`closed (${ev.code})`, "closed"); setState(`closed (${ev.code})`, "closed");
opts.onClosed?.(sessionId); opts.onClosed?.(sessionId);
}; };
+9 -2
View File
@@ -25,8 +25,13 @@ export interface PoolOptions {
/** Fired when a connection's socket closes on its own (session ended/dropped), /** Fired when a connection's socket closes on its own (session ended/dropped),
* not via an intentional close/eviction. The embedder closes the pane. */ * not via an intentional close/eviction. The embedder closes the pane. */
onClosed?: (id: Uuid) => void; 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? */ /** 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), onRequestClose: (sid) => this.opts.onRequestClose?.(sid),
onClosed: (sid) => this.opts.onClosed?.(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, info,
); );
+7
View File
@@ -47,6 +47,13 @@ describe("shouldRetryClose", () => {
it("never retries a clean close", () => { it("never retries a clean close", () => {
expect(shouldRetryClose("a", 1000, running)).toBe(false); 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", () => { describe("backoff bounds", () => {
+8
View File
@@ -26,7 +26,15 @@ export function shouldRetryClose(
sessionId: Uuid, sessionId: Uuid,
code: number, code: number,
sessions: ReadonlyMap<Uuid, Session>, sessions: ReadonlyMap<Uuid, Session>,
/** 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 { ): boolean {
if (code >= APPLICATION_CLOSE || code === CLEAN_CLOSE) return false; if (code >= APPLICATION_CLOSE || code === CLEAN_CLOSE) return false;
if (!everOpened) return false;
return sessions.get(sessionId)?.status === "Running"; return sessions.get(sessionId)?.status === "Running";
} }