From f31377c34f89f7c292b3fb1252834bfbd79546ce Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sat, 22 Aug 2026 16:31:54 +0100 Subject: [PATCH] fix(maestro): three defects the review flagged and left open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- crates/maestro-daemon/src/local_api.rs | 10 +- crates/maestro-daemon/src/session.rs | 20 +-- .../maestro-daemon/tests/session_decisions.rs | 34 ++++++ web/src/attach.ts | 115 +++++++++++------- web/src/connections.ts | 3 + web/src/main.ts | 12 ++ web/src/multiview.ts | 3 + web/src/reconnect.test.ts | 63 ++++++++++ web/src/reconnect.ts | 32 +++++ 9 files changed, 234 insertions(+), 58 deletions(-) create mode 100644 web/src/reconnect.test.ts create mode 100644 web/src/reconnect.ts diff --git a/crates/maestro-daemon/src/local_api.rs b/crates/maestro-daemon/src/local_api.rs index 7326f0c..c456364 100644 --- a/crates/maestro-daemon/src/local_api.rs +++ b/crates/maestro-daemon/src/local_api.rs @@ -135,14 +135,10 @@ async fn create_session( } } +/// `kill` already reports a missing session as `SessionError::NotFound`, which +/// maps to 404. Checking first would only add a window in which the liveness +/// watcher reaps the session between the two awaits, turning a 404 into a 502. async fn kill_session(State(s): State, Path(id): Path) -> Response { - if !s.manager.has(id).await { - return error_response( - StatusCode::NOT_FOUND, - "unknown_session", - "no such session on this daemon", - ); - } match s.manager.kill(id).await { Ok(()) => StatusCode::NO_CONTENT.into_response(), Err(e) => { diff --git a/crates/maestro-daemon/src/session.rs b/crates/maestro-daemon/src/session.rs index 2860861..1019dcb 100644 --- a/crates/maestro-daemon/src/session.rs +++ b/crates/maestro-daemon/src/session.rs @@ -145,16 +145,18 @@ impl SessionManager { } /// Attach a durable spawn-history log (#24). Builder, called once right - /// after construction while the `Arc` is uniquely owned — so - /// [`Arc::get_mut`] succeeds. A no-op (logged) on the off chance the handle - /// was already cloned. + /// after construction while the `Arc` is uniquely owned. + /// + /// # Panics + /// + /// If the handle has already been cloned. That is a construction-order + /// mistake, and the alternative — logging and carrying on — produces a + /// daemon whose "recent dirs" picker is permanently empty with nothing to + /// show for it. pub fn with_spawn_history(mut self, history: SpawnHistory) -> Self { - match Arc::get_mut(&mut self.inner) { - Some(inner) => inner.spawn_history = Some(history), - None => tracing::error!( - "with_spawn_history called after the SessionManager was cloned; ignoring" - ), - } + let inner = Arc::get_mut(&mut self.inner) + .expect("with_spawn_history must be called before the SessionManager is cloned"); + inner.spawn_history = Some(history); self } diff --git a/crates/maestro-daemon/tests/session_decisions.rs b/crates/maestro-daemon/tests/session_decisions.rs index ed31a2e..967f6fd 100644 --- a/crates/maestro-daemon/tests/session_decisions.rs +++ b/crates/maestro-daemon/tests/session_decisions.rs @@ -362,3 +362,37 @@ async fn wait_for_updated( } panic!("timed out waiting for SessionUpdated"); } + +/// The kill handler used to check `has()` first, which opened a window for the +/// liveness watcher to reap the session in between — turning a 404 into a 502. +/// `kill` reports the miss itself now. +#[tokio::test] +async fn killing_an_unknown_session_is_reported_as_not_found() { + let fx = Fx::new(); + let err = fx + .manager + .kill(uuid::Uuid::new_v4()) + .await + .expect_err("should fail"); + assert_eq!(err.to_error_info().code, "not_found"); +} + +/// A construction-order mistake must be loud: a silently-ignored history leaves +/// the recent-dirs picker permanently empty. +#[test] +#[should_panic(expected = "before the SessionManager is cloned")] +fn attaching_spawn_history_after_cloning_panics() { + let tmp = TempDir::new().expect("tempdir"); + let fake = Arc::new(FakePanes::new()); + let manager = SessionManager::new( + fake, + SidecarStore::new(tmp.path().join("sidecars")), + WorktreeManager::new(tmp.path().join("worktrees")), + tmp.path().join("fifos"), + ); + let _clone = manager.clone(); + let _ = manager.with_spawn_history(maestro_daemon::spawn_history::SpawnHistory::new( + tmp.path().join("history.jsonl"), + 7, + )); +} diff --git a/web/src/attach.ts b/web/src/attach.ts index d9aed6a..52ada32 100644 --- a/web/src/attach.ts +++ b/web/src/attach.ts @@ -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); diff --git a/web/src/connections.ts b/web/src/connections.ts index a99605b..b83123f 100644 --- a/web/src/connections.ts +++ b/web/src/connections.ts @@ -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, ); diff --git a/web/src/main.ts b/web/src/main.ts index d3f4362..aa5fd48 100644 --- a/web/src/main.ts +++ b/web/src/main.ts @@ -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 { diff --git a/web/src/multiview.ts b/web/src/multiview.ts index 4bc014d..777865b 100644 --- a/web/src/multiview.ts +++ b/web/src/multiview.ts @@ -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; /** 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, diff --git a/web/src/reconnect.test.ts b/web/src/reconnect.test.ts new file mode 100644 index 0000000..e885201 --- /dev/null +++ b/web/src/reconnect.test.ts @@ -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(); + + 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); + }); +}); diff --git a/web/src/reconnect.ts b/web/src/reconnect.ts new file mode 100644 index 0000000..d30bab9 --- /dev/null +++ b/web/src/reconnect.ts @@ -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, +): boolean { + if (code >= APPLICATION_CLOSE || code === CLEAN_CLOSE) return false; + return sessions.get(sessionId)?.status === "Running"; +}