fix(maestro): three defects the review flagged and left open
SPA attach had no reconnect at all. Any dropped frame closed the socket, `onClosed` fired, and the pane was destroyed permanently — the TUI reconnects forever on the same endpoint. Session death also reached the UI on two uncoordinated channels: the row from the events socket, the pane from `ws.onclose`, with nothing reconciling them, so either could happen without the other. The catalog is now the authority. `shouldRetryClose` decides from session status: Running means the link dropped, so retry with 500ms-to-10s backoff; Dead means the PTY stream was sealed and nothing more is coming; absent means it is gone. Application closes (4000+, a relayed daemon rejection) and clean closes are never retried. The shell also closes any pane whose session has left the catalog. The policy lives in reconnect.ts so it is tested without a DOM. kill_session checked `has()` before `kill()`, which only opened a window for the liveness watcher to reap in between — turning a 404 into a 502. `kill` reports the miss itself. with_spawn_history logged and carried on when `Arc::get_mut` failed, producing a daemon whose recent-dirs picker is permanently empty with nothing to show for it. It panics now: that is a construction-order mistake, at startup, in code that runs once. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit was merged in pull request #50.
This commit is contained in:
co-authored by
Claude Opus 5
parent
812861f883
commit
f31377c34f
+73
-42
@@ -4,6 +4,7 @@ import "@xterm/xterm/css/xterm.css";
|
||||
|
||||
import { attachWsUrl } from "./routes.js";
|
||||
import { escapeHtml } from "./html.js";
|
||||
import { INITIAL_RECONNECT_MS, MAX_RECONNECT_MS } from "./reconnect.js";
|
||||
import { applyModsToChar, cursorSeq } from "./keys.js";
|
||||
import type { ClientControl, Uuid } from "./types.js";
|
||||
|
||||
@@ -37,8 +38,13 @@ 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;
|
||||
/** Asked on every close: should the socket be retried? The shell answers
|
||||
* from the catalog — see `shouldRetryClose`. */
|
||||
shouldReconnect?: (sessionId: Uuid, code: number) => boolean;
|
||||
}
|
||||
|
||||
|
||||
|
||||
/** What the header shows about the session (name / command). Optional — falls
|
||||
* back to a short session id when the session isn't known yet. */
|
||||
export interface AttachInfo {
|
||||
@@ -249,8 +255,9 @@ export function createAttachConnection(
|
||||
}
|
||||
};
|
||||
|
||||
const ws = new WebSocket(attachWsUrl(sessionId));
|
||||
ws.binaryType = "arraybuffer";
|
||||
let ws: WebSocket;
|
||||
let reconnectMs = INITIAL_RECONNECT_MS;
|
||||
let reconnectTimer: number | null = null;
|
||||
|
||||
const sendControl = (msg: ClientControl) => {
|
||||
if (ws.readyState !== WebSocket.OPEN) return;
|
||||
@@ -269,50 +276,70 @@ export function createAttachConnection(
|
||||
sendControl({ type: "Resize", cols: term.cols, rows: term.rows });
|
||||
};
|
||||
|
||||
ws.onopen = () => {
|
||||
setState("connected", "open");
|
||||
sendControl({ type: "Resize", cols: term.cols, rows: term.rows });
|
||||
if (mounted) term.focus();
|
||||
};
|
||||
function connect(): void {
|
||||
ws = new WebSocket(attachWsUrl(sessionId));
|
||||
ws.binaryType = "arraybuffer";
|
||||
|
||||
ws.onmessage = (ev) => {
|
||||
if (typeof ev.data === "string") {
|
||||
try {
|
||||
JSON.parse(ev.data) as ClientControl;
|
||||
} catch {
|
||||
// ignore unknown control frames
|
||||
ws.onopen = () => {
|
||||
reconnectMs = INITIAL_RECONNECT_MS;
|
||||
setState("connected", "open");
|
||||
sendControl({ type: "Resize", cols: term.cols, rows: term.rows });
|
||||
if (mounted) term.focus();
|
||||
};
|
||||
|
||||
ws.onmessage = (ev) => {
|
||||
if (typeof ev.data === "string") {
|
||||
try {
|
||||
JSON.parse(ev.data) as ClientControl;
|
||||
} catch {
|
||||
// ignore unknown control frames
|
||||
}
|
||||
return;
|
||||
}
|
||||
return;
|
||||
}
|
||||
if (ev.data instanceof ArrayBuffer) {
|
||||
term.write(new Uint8Array(ev.data));
|
||||
} else if (ev.data instanceof Blob) {
|
||||
// Some browsers may default to Blob despite binaryType="arraybuffer".
|
||||
ev.data.arrayBuffer().then((b) => term.write(new Uint8Array(b)));
|
||||
}
|
||||
};
|
||||
if (ev.data instanceof ArrayBuffer) {
|
||||
term.write(new Uint8Array(ev.data));
|
||||
} else if (ev.data instanceof Blob) {
|
||||
// Some browsers may default to Blob despite binaryType="arraybuffer".
|
||||
ev.data.arrayBuffer().then((b) => term.write(new Uint8Array(b)));
|
||||
}
|
||||
};
|
||||
|
||||
ws.onerror = () => {
|
||||
setState("error", "error");
|
||||
};
|
||||
ws.onerror = () => {
|
||||
setState("error", "error");
|
||||
};
|
||||
|
||||
ws.onclose = (ev) => {
|
||||
// The web proxy relays a daemon rejection (unknown session) as an
|
||||
// application close code (4000+) carrying a human reason. Show it in the pane — both the header and the terminal body
|
||||
// — and KEEP the pane up so the user can read why, instead of letting it
|
||||
// flash and vanish (#39). Clean session-ends and ordinary drops keep the
|
||||
// auto-close behavior below.
|
||||
const reason = ev.reason?.trim();
|
||||
if (ev.code >= 4000 && reason && !disposed) {
|
||||
setState(reason, "error");
|
||||
term.write(`\r\n\x1b[31m⚠ cannot attach: ${reason}\x1b[0m\r\n`);
|
||||
return;
|
||||
}
|
||||
setState(`closed (${ev.code})`, "closed");
|
||||
// Only bubble up server/network closes — an intentional dispose has already
|
||||
// set `disposed`, and eviction/user-close handle their own pane teardown.
|
||||
if (!disposed) opts.onClosed?.(sessionId);
|
||||
};
|
||||
ws.onclose = (ev) => {
|
||||
// The web proxy relays a daemon rejection (unknown session) as an
|
||||
// application close code (4000+) carrying a human reason. Show it in the
|
||||
// pane — both the header and the terminal body — and KEEP the pane up so
|
||||
// the user can read why, instead of letting it flash and vanish (#39).
|
||||
const reason = ev.reason?.trim();
|
||||
if (ev.code >= 4000 && reason && !disposed) {
|
||||
setState(reason, "error");
|
||||
term.write(`\r\n\x1b[31m⚠ cannot attach: ${reason}\x1b[0m\r\n`);
|
||||
return;
|
||||
}
|
||||
// An intentional dispose has already set `disposed`; eviction and user
|
||||
// close handle their own pane teardown.
|
||||
if (disposed) return;
|
||||
|
||||
if (opts.shouldReconnect?.(sessionId, ev.code)) {
|
||||
const delay = Math.min(reconnectMs, MAX_RECONNECT_MS);
|
||||
setState("reconnecting…", "connecting");
|
||||
reconnectTimer = window.setTimeout(() => {
|
||||
reconnectTimer = null;
|
||||
reconnectMs = Math.min(reconnectMs * 2, MAX_RECONNECT_MS);
|
||||
connect();
|
||||
}, delay);
|
||||
return;
|
||||
}
|
||||
|
||||
setState(`closed (${ev.code})`, "closed");
|
||||
opts.onClosed?.(sessionId);
|
||||
};
|
||||
}
|
||||
|
||||
connect();
|
||||
|
||||
// PTY input: every keystroke goes out as a binary frame. A pending key-bar
|
||||
// Ctrl modifier (mobile) folds into the next typed character here.
|
||||
@@ -357,6 +384,10 @@ export function createAttachConnection(
|
||||
dispose() {
|
||||
if (disposed) return;
|
||||
disposed = true;
|
||||
if (reconnectTimer !== null) {
|
||||
window.clearTimeout(reconnectTimer);
|
||||
reconnectTimer = null;
|
||||
}
|
||||
ro.disconnect();
|
||||
window.removeEventListener("resize", onResize);
|
||||
visual?.removeEventListener("resize", onResize);
|
||||
|
||||
@@ -24,6 +24,8 @@ 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;
|
||||
/** Forwarded to each connection: should a dropped socket be retried? */
|
||||
shouldReconnect?: (id: Uuid, code: number) => boolean;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -82,6 +84,7 @@ export class ConnectionPool {
|
||||
{
|
||||
onRequestClose: (sid) => this.opts.onRequestClose?.(sid),
|
||||
onClosed: (sid) => this.opts.onClosed?.(sid),
|
||||
shouldReconnect: (sid, code) => this.opts.shouldReconnect?.(sid, code) ?? false,
|
||||
},
|
||||
info,
|
||||
);
|
||||
|
||||
@@ -2,6 +2,7 @@ import { renderSidebar, renderTopbar } from "./catalog.js";
|
||||
import { ConnectionPool } from "./connections.js";
|
||||
import { installHudKeybind } from "./debug.js";
|
||||
import { CatalogStore } from "./events.js";
|
||||
import { shouldRetryClose } from "./reconnect.js";
|
||||
import { mountHud, type Hud } from "./hud.js";
|
||||
import { emptyLayout, type Layout, openSessions } from "./layout.js";
|
||||
import { createMultiView, type MultiView } from "./multiview.js";
|
||||
@@ -149,6 +150,17 @@ const pool = new ConnectionPool({
|
||||
// pane is never left lying around.
|
||||
onRequestClose: (id) => closePane(id),
|
||||
onClosed: (id) => closePane(id),
|
||||
shouldReconnect: (id, code) => shouldRetryClose(id, code, store.snapshot().sessions),
|
||||
});
|
||||
|
||||
// Session death used to reach the UI on two uncoordinated channels — the row
|
||||
// from the events socket, the pane from `ws.onclose` — so either could happen
|
||||
// without the other. The catalog is the authority: a pane for a session that is
|
||||
// no longer listed gets closed here.
|
||||
store.subscribe((state) => {
|
||||
for (const id of mv?.openSessions() ?? []) {
|
||||
if (!state.sessions.has(id)) closePane(id);
|
||||
}
|
||||
});
|
||||
|
||||
function closePane(id: string): void {
|
||||
|
||||
@@ -45,6 +45,8 @@ export interface MultiView {
|
||||
openSession: (sessionId: string) => void;
|
||||
/** Drop the pane showing `sessionId` (the session was disconnected/killed). */
|
||||
closeSession: (sessionId: string) => void;
|
||||
/** Every session currently shown in some pane, across all tabs. */
|
||||
openSessions: () => Set<string>;
|
||||
/** Start dragging a sidebar session into the canvas (issue #32). The
|
||||
* controller owns drag-vs-tap: a tap lets the row's normal click open it, a
|
||||
* drag shows drop zones to place it. */
|
||||
@@ -599,6 +601,7 @@ export function createMultiView(opts: MultiViewOptions): MultiView {
|
||||
el,
|
||||
openSession,
|
||||
closeSession,
|
||||
openSessions: () => openSessions(layout),
|
||||
beginSessionDrag,
|
||||
render,
|
||||
focusedSessionId: focusedLeafSession,
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
// The reconnect *policy* — which closes are retried and which end the pane.
|
||||
// `createAttachConnection` itself needs a DOM and xterm, so the decision is
|
||||
// tested here as the predicate the shell supplies, plus the backoff schedule
|
||||
// it drives.
|
||||
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
import { INITIAL_RECONNECT_MS, MAX_RECONNECT_MS, shouldRetryClose } from "./reconnect.js";
|
||||
import type { Session } from "./types.js";
|
||||
|
||||
function session(id: string, status: Session["status"]): Session {
|
||||
return {
|
||||
id,
|
||||
cwd: "/home/n",
|
||||
command: "bash",
|
||||
env: {},
|
||||
metadata: {},
|
||||
status,
|
||||
created_at: "2026-08-22T00:00:00Z",
|
||||
} as Session;
|
||||
}
|
||||
|
||||
describe("shouldRetryClose", () => {
|
||||
const running = new Map([["a", session("a", "Running")]]);
|
||||
const dead = new Map([["a", session("a", "Dead")]]);
|
||||
const gone = new Map<string, Session>();
|
||||
|
||||
it("retries a dropped link to a session that is still running", () => {
|
||||
expect(shouldRetryClose("a", 1006, running)).toBe(true);
|
||||
expect(shouldRetryClose("a", 1001, running)).toBe(true);
|
||||
});
|
||||
|
||||
it("does not retry a session the daemon has marked dead", () => {
|
||||
// Its stream is sealed; nothing further will ever arrive.
|
||||
expect(shouldRetryClose("a", 1006, dead)).toBe(false);
|
||||
});
|
||||
|
||||
it("does not retry a session that is no longer in the catalog", () => {
|
||||
expect(shouldRetryClose("a", 1006, gone)).toBe(false);
|
||||
});
|
||||
|
||||
it("never retries an application close, which carries a daemon rejection", () => {
|
||||
// 4000+ means the proxy relayed a refusal — retrying would loop on it.
|
||||
expect(shouldRetryClose("a", 4502, running)).toBe(false);
|
||||
});
|
||||
|
||||
it("never retries a clean close", () => {
|
||||
expect(shouldRetryClose("a", 1000, running)).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe("backoff bounds", () => {
|
||||
it("starts well under a second and caps at ten", () => {
|
||||
expect(INITIAL_RECONNECT_MS).toBeLessThan(1000);
|
||||
expect(MAX_RECONNECT_MS).toBe(10_000);
|
||||
});
|
||||
|
||||
it("reaches the cap by doubling and never exceeds it", () => {
|
||||
let ms = INITIAL_RECONNECT_MS;
|
||||
for (let i = 0; i < 20; i++) ms = Math.min(ms * 2, MAX_RECONNECT_MS);
|
||||
expect(ms).toBe(MAX_RECONNECT_MS);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,32 @@
|
||||
// Attach-socket reconnect policy. Split out of attach.ts so it can be tested
|
||||
// without a DOM or an xterm instance.
|
||||
|
||||
import type { Session, Uuid } from "./types.js";
|
||||
|
||||
/** First retry delay after an unexpected close, doubling up to the cap. */
|
||||
export const INITIAL_RECONNECT_MS = 500;
|
||||
export const MAX_RECONNECT_MS = 10_000;
|
||||
|
||||
/** Close codes at or above this are application-defined; `maestro-web` uses
|
||||
* them to relay a daemon rejection, which retrying would only loop on. */
|
||||
const APPLICATION_CLOSE = 4000;
|
||||
|
||||
/** Normal Closure — the peer ended the stream deliberately. */
|
||||
const CLEAN_CLOSE = 1000;
|
||||
|
||||
/**
|
||||
* Should a closed attach socket be retried?
|
||||
*
|
||||
* The catalog is the authority: a session still `Running` lost its link, so
|
||||
* retry; one that is `Dead` has had its PTY stream sealed and will never send
|
||||
* again, and one absent from the catalog is gone. Before this existed, any
|
||||
* dropped frame destroyed the pane permanently.
|
||||
*/
|
||||
export function shouldRetryClose(
|
||||
sessionId: Uuid,
|
||||
code: number,
|
||||
sessions: ReadonlyMap<Uuid, Session>,
|
||||
): boolean {
|
||||
if (code >= APPLICATION_CLOSE || code === CLEAN_CLOSE) return false;
|
||||
return sessions.get(sessionId)?.status === "Running";
|
||||
}
|
||||
Reference in New Issue
Block a user