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"; +}