From 42b5040c8009ef3b69d824c04e327d9d661a86f1 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 07:39:06 +0000 Subject: [PATCH 1/6] refactor: the terminal router keeps its sessions and takes the new client after a reconnect --- .../TerminalView/TerminalView.test.tsx | 29 ++- .../components/TerminalView/TerminalView.tsx | 40 ++- .../contexts/subscriptionInventory.test.ts | 6 +- .../terminal/TerminalRouterContext.test.tsx | 100 ++++++++ .../terminal/TerminalRouterContext.tsx | 31 +-- .../terminal/TerminalSessionRouter.test.ts | 196 ++++++++++++++ src/browser/terminal/TerminalSessionRouter.ts | 239 ++++++++++++------ 7 files changed, 536 insertions(+), 105 deletions(-) create mode 100644 src/browser/terminal/TerminalRouterContext.test.tsx create mode 100644 src/browser/terminal/TerminalSessionRouter.test.ts diff --git a/src/browser/components/TerminalView/TerminalView.test.tsx b/src/browser/components/TerminalView/TerminalView.test.tsx index cf90947fed7..ff5e793ddec 100644 --- a/src/browser/components/TerminalView/TerminalView.test.tsx +++ b/src/browser/components/TerminalView/TerminalView.test.tsx @@ -12,7 +12,7 @@ import { restoreModulesAfterSuite } from "../../../../tests/ui/moduleMocks"; interface TerminalSubscribeCallbacks { onOutput: (data: string) => void; - onScreenState: (state: string) => void; + onScreenState: (state: string, options?: { replace: boolean }) => void; onExit: (code: number) => void; } @@ -55,8 +55,14 @@ class MockTerminal { cols = 80; rows = 24; options: MockTerminalOptions; + calls: string[] = []; clear = mock(() => undefined); - write = mock((_data: string) => undefined); + reset = mock(() => { + this.calls.push("reset"); + }); + write = mock((data: string) => { + this.calls.push(`write:${data}`); + }); resize = mock((cols: number, rows: number) => { this.cols = cols; this.rows = rows; @@ -177,6 +183,25 @@ describe("TerminalView", () => { mock.restore(); }); + // After a reconnect the router sends the full screen again. Written on top of the old screen, + // the scrollback would repeat, so a replacing screen starts from an empty terminal. + test("a screen that replaces the old one resets the terminal before writing", async () => { + renderTerminal(mock((_exitCode: number) => undefined)); + await waitFor(() => expect(mockRouter.subscribe).toHaveBeenCalledTimes(1)); + const terminal = terminalInstances[0]; + expect(terminal?.calls).toEqual(["write:initial screen"]); + + act(() => subscribeCallbacks[0]?.onScreenState("screen after reconnect", { replace: true })); + expect(terminal?.calls).toEqual([ + "write:initial screen", + "reset", + "write:screen after reconnect", + ]); + // The view keeps its one subscription: no new subscribe, no clear. + expect(mockRouter.subscribe).toHaveBeenCalledTimes(1); + expect(terminal?.clear).toHaveBeenCalledTimes(1); + }); + test("keeps the router subscription stable when onExit changes", async () => { const firstOnExit = mock((_exitCode: number) => undefined); const secondOnExit = mock((_exitCode: number) => undefined); diff --git a/src/browser/components/TerminalView/TerminalView.tsx b/src/browser/components/TerminalView/TerminalView.tsx index 240918be61f..c9304db4ecb 100644 --- a/src/browser/components/TerminalView/TerminalView.tsx +++ b/src/browser/components/TerminalView/TerminalView.tsx @@ -101,6 +101,34 @@ async function preloadTerminalWebfonts( } } +/** + * Writes a full screen from the server (may be empty for new sessions). + * + * `replace` is set after a reconnect, when the router reattaches and sends the full screen again + * (TerminalSessionRouter). clear() keeps the scrollback, so writing on top would repeat it: + * start from an empty terminal instead. + */ +export function writeScreenState( + term: Pick, + state: string, + replace: boolean +): void { + if (replace) { + try { + term.reset(); + } catch (err) { + console.warn("[TerminalView] Error resetting terminal:", err); + } + } + if (!state) return; + try { + term.write(state); + } catch (err) { + // xterm WASM can throw "memory access out of bounds" intermittently + console.warn("[TerminalView] Error writing screenState:", err); + } +} + interface TerminalViewProps { workspaceId: string; /** Session ID to connect to (required - must be created before mounting) */ @@ -365,16 +393,8 @@ export function TerminalView({ console.warn("[TerminalView] Error writing output:", err); } }, - onScreenState: (state) => { - // Write screen state (may be empty for new sessions) - if (state) { - try { - term.write(state); - } catch (err) { - // xterm WASM can throw "memory access out of bounds" intermittently - console.warn("[TerminalView] Error writing screenState:", err); - } - } + onScreenState: (state, options) => { + writeScreenState(term, state, options?.replace === true); // Mark loading complete - we now have valid content to show setIsLoading(false); }, diff --git a/src/browser/contexts/subscriptionInventory.test.ts b/src/browser/contexts/subscriptionInventory.test.ts index e6f273c4fcb..df2a3cba550 100644 --- a/src/browser/contexts/subscriptionInventory.test.ts +++ b/src/browser/contexts/subscriptionInventory.test.ts @@ -16,8 +16,8 @@ type RestartRule = | "effect-deps" /** A store fed through setClient(): it drops the client while offline and resubscribes. */ | "store-live-feed" - /** The terminal router: today the provider builds a new router per client. */ - | "router-per-client" + /** The terminal router: setClient() restarts its streams on each new client. */ + | "router-setClient" /** A long-lived holder with its own capped backoff. */ | "holder-backoff" /** One operation's progress stream (a clone): it is not restarted, the operation fails. */ @@ -56,7 +56,7 @@ const RESTART_RULES: Record = { "src/browser/stores/ProvidersConfigStore.ts": "store-live-feed", "src/browser/stores/ReviewStateStore.ts": "store-live-feed", "src/browser/stores/WorkspaceStore.ts": "store-live-feed", - "src/browser/terminal/TerminalSessionRouter.ts": "router-per-client", + "src/browser/terminal/TerminalSessionRouter.ts": "router-setClient", }; /** diff --git a/src/browser/terminal/TerminalRouterContext.test.tsx b/src/browser/terminal/TerminalRouterContext.test.tsx new file mode 100644 index 00000000000..a54e9a7028a --- /dev/null +++ b/src/browser/terminal/TerminalRouterContext.test.tsx @@ -0,0 +1,100 @@ +import "../../../tests/ui/dom"; + +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import { cleanup, render, waitFor } from "@testing-library/react"; +import { useEffect } from "react"; +import { installDom } from "../../../tests/ui/dom"; +import { createConnectionHarness, GAP_MODES } from "@/browser/contexts/connection.testHarness"; +import { TerminalRouterProvider, useTerminalRouter } from "./TerminalRouterContext"; +import type { TerminalSessionRouter } from "./TerminalSessionRouter"; + +/** One entry per connect: the attach calls that connection's server saw. */ +let attachesPerConnection: number[] = []; + +function serverOverrides() { + const index = attachesPerConnection.length; + attachesPerConnection.push(0); + return { + terminal: { + attach: () => { + attachesPerConnection[index] += 1; + return Promise.resolve( + (async function* () { + yield { type: "screenState" as const, data: `screen-${index}` }; + await new Promise(() => undefined); // stays open until aborted + })() + ); + }, + // An exit stream that never yields: the process keeps running. + onExit: () => + Promise.resolve( + (async function* () { + await new Promise(() => undefined); + yield 0; + })() + ), + resize: () => Promise.resolve(), + }, + }; +} + +let routers: Array = []; +let subscribes = 0; +let screens: Array<{ state: string; replace: boolean }> = []; + +/** Subscribes once per router, as TerminalView does. */ +function TerminalProbe() { + const router = useTerminalRouter(); + routers.push(router); + useEffect(() => { + if (router == null) return; + subscribes += 1; + return router.subscribe("t1", { + onOutput: () => undefined, + onScreenState: (state, options) => + screens.push({ state, replace: options?.replace === true }), + onExit: () => undefined, + }); + }, [router]); + return null; +} + +describe.each([...GAP_MODES])("TerminalRouterProvider across a reconnect (%s)", (mode) => { + let cleanupDom: (() => void) | null = null; + beforeEach(() => { + cleanupDom = installDom(); + attachesPerConnection = []; + routers = []; + subscribes = 0; + screens = []; + }); + afterEach(() => { + cleanup(); + cleanupDom?.(); + cleanupDom = null; + }); + + test("the terminal keeps its router and subscription, and reattaches on the new client", async () => { + const harness = createConnectionHarness(mode, serverOverrides); + render( + + + , + { wrapper: harness.Wrapper } + ); + await waitFor(() => expect(screens).toEqual([{ state: "screen-0", replace: false }])); + const router = routers.at(-1) ?? null; + expect(router).not.toBeNull(); + + await harness.drop(); + harness.reconnect(); + await waitFor(() => expect(attachesPerConnection).toEqual([1, 1])); + await waitFor(() => expect(screens.at(-1)).toEqual({ state: "screen-1", replace: true })); + + // A new router (or a null one in the gap) made the view subscribe again and clear itself. + const distinct: unknown[] = [...new Set(routers.filter((r) => r != null))]; + expect(distinct).toEqual([router]); + expect(routers.slice(routers.indexOf(router))).not.toContain(null); + expect(subscribes).toBe(1); + }); +}); diff --git a/src/browser/terminal/TerminalRouterContext.tsx b/src/browser/terminal/TerminalRouterContext.tsx index 4293875660e..5880a111f85 100644 --- a/src/browser/terminal/TerminalRouterContext.tsx +++ b/src/browser/terminal/TerminalRouterContext.tsx @@ -20,46 +20,39 @@ interface TerminalRouterProviderProps { /** * Provides TerminalSessionRouter to the component tree. * - * Creates a single router instance that lives for the lifetime of the provider. - * The router is recreated if the API client changes (e.g., reconnection). + * One router lives as long as the provider. Each new API client (a reconnect) goes to the same + * router through setClient(), so open terminals keep their sessions and screens: a router built + * per client made every TerminalView subscribe again, which cleared the terminal. * * Always renders children so the app UI stays visible during reconnection. - * The router may be null when API is unavailable; consumers must handle this. */ export function TerminalRouterProvider(props: TerminalRouterProviderProps) { const { api } = useAPI(); const [router, setRouter] = useState(null); + const popout = props.popout === true; + // Create/cleanup after commit to avoid render-time disposal in concurrent mode. useEffect(() => { - if (!api) { - setRouter(null); - return; - } - - // Create/cleanup after commit to avoid render-time disposal in concurrent mode. - const nextRouter = new TerminalSessionRouter(api, { popout: props.popout === true }); + const nextRouter = new TerminalSessionRouter(null, { popout }); setRouter(nextRouter); return () => { nextRouter.dispose(); }; - }, [api, props.popout]); + }, [popout]); - const routerForContext = api && router?.getApi() === api ? router : null; + useEffect(() => { + router?.setClient(api); + }, [router, api]); - // Always render children - the router may be null during reconnection. - // Consumers (useTerminalRouter) must handle the null case gracefully. return ( - - {props.children} - + {props.children} ); } /** * Hook to access the TerminalSessionRouter. * - * Returns null when the API is disconnected (e.g., during reconnection). - * Callers should handle the null case gracefully. + * Returns null only before the provider's first commit. Callers should handle the null case. * * @throws If used outside of TerminalRouterProvider */ diff --git a/src/browser/terminal/TerminalSessionRouter.test.ts b/src/browser/terminal/TerminalSessionRouter.test.ts new file mode 100644 index 00000000000..6c74b05bc32 --- /dev/null +++ b/src/browser/terminal/TerminalSessionRouter.test.ts @@ -0,0 +1,196 @@ +import { describe, expect, test } from "bun:test"; +import { createTestApiClient } from "@/browser/testUtils"; +import { TerminalSessionRouter, type TerminalSubscriberCallbacks } from "./TerminalSessionRouter"; + +type Message = { type: "screenState"; data: string } | { type: "output"; data: string }; + +/** A server whose attach and onExit streams the test feeds by hand. */ +function fakeServer(name: string) { + const attaches: Array<{ input: unknown; push: (m: Message) => void; signal?: AbortSignal }> = []; + const exits: Array<{ push: (code: number) => void }> = []; + const inputs: string[] = []; + const resizes: Array<{ cols: number; rows: number }> = []; + let releaseAttach: (() => void) | null = null; + let holdAttach = false; + + function stream(register: (push: (value: T) => void) => void, signal?: AbortSignal) { + const queue: T[] = []; + let wake: (() => void) | null = null; + register((value) => { + queue.push(value); + wake?.(); + }); + return (async function* () { + while (!signal?.aborted) { + if (queue.length === 0) { + await new Promise((resolve) => { + wake = resolve; + signal?.addEventListener("abort", () => resolve(), { once: true }); + }); + wake = null; + continue; + } + yield queue.shift() as T; + } + })(); + } + + const client = createTestApiClient({ + terminal: { + attach: async (input: unknown, options?: { signal?: AbortSignal }) => { + if (holdAttach) await new Promise((resolve) => (releaseAttach = resolve)); + return stream( + (push) => attaches.push({ input, push, signal: options?.signal }), + options?.signal + ); + }, + onExit: (_input: unknown, options?: { signal?: AbortSignal }) => + Promise.resolve(stream((push) => exits.push({ push }), options?.signal)), + sendInput: (input: { data: string }) => { + inputs.push(input.data); + return Promise.resolve(); + }, + resize: (input: { cols: number; rows: number }) => { + resizes.push({ cols: input.cols, rows: input.rows }); + return Promise.resolve(); + }, + }, + }); + return { + name, + client, + attaches, + exits, + inputs, + resizes, + holdNextAttach() { + holdAttach = true; + }, + releaseAttach() { + holdAttach = false; + releaseAttach?.(); + }, + }; +} + +async function settle() { + for (let i = 0; i < 10; i++) await new Promise((resolve) => setTimeout(resolve, 0)); +} + +function recorder() { + const screens: Array<{ state: string; replace: boolean }> = []; + const output: string[] = []; + const exits: number[] = []; + const callbacks: TerminalSubscriberCallbacks = { + onOutput: (data) => output.push(data), + onScreenState: (state, options) => screens.push({ state, replace: options?.replace === true }), + onExit: (code) => exits.push(code), + }; + return { screens, output, exits, callbacks }; +} + +describe("TerminalSessionRouter across reconnects", () => { + test("setClient() keeps the subscriber and restarts its streams on the new client", async () => { + const first = fakeServer("first"); + const router = new TerminalSessionRouter(first.client); + const view = recorder(); + router.subscribe("t1", view.callbacks); + await settle(); + first.attaches[0]?.push({ type: "screenState", data: "screen-1" }); + await settle(); + + const second = fakeServer("second"); + router.setClient(second.client); + await settle(); + expect(second.attaches).toHaveLength(1); + second.attaches[0]?.push({ type: "screenState", data: "screen-2" }); + second.attaches[0]?.push({ type: "output", data: "after" }); + // The old server's stream is superseded: nothing it sends reaches the view. + first.attaches[0]?.push({ type: "output", data: "stale" }); + await settle(); + + expect(view.screens).toEqual([ + { state: "screen-1", replace: false }, + { state: "screen-2", replace: true }, + ]); + expect(view.output).toEqual(["after"]); + expect(first.attaches[0]?.signal?.aborted).toBe(true); + }); + + test("an attach that resolves after a newer client arrived delivers nothing", async () => { + const first = fakeServer("first"); + first.holdNextAttach(); + const router = new TerminalSessionRouter(first.client); + const view = recorder(); + router.subscribe("t1", view.callbacks); + await settle(); + + const second = fakeServer("second"); + router.setClient(second.client); + first.releaseAttach(); + await settle(); + first.attaches[0]?.push({ type: "output", data: "late" }); + second.attaches[0]?.push({ type: "output", data: "current" }); + await settle(); + expect(view.output).toEqual(["current"]); + }); + + test("offline, the session waits: input is dropped and the size is sent again later", async () => { + const first = fakeServer("first"); + const router = new TerminalSessionRouter(first.client); + const view = recorder(); + router.subscribe("t1", view.callbacks); + await settle(); + + router.setClient(null); + router.sendInput("t1", "lost"); + await router.resize("t1", 120, 40); + expect(first.inputs).toEqual([]); + + const second = fakeServer("second"); + router.setClient(second.client); + await settle(); + expect(second.resizes).toEqual([{ cols: 120, rows: 40 }]); + expect(second.attaches).toHaveLength(1); + router.sendInput("t1", "typed"); + await settle(); + expect(second.inputs).toEqual(["typed"]); + }); + + test("an exited session is not restarted", async () => { + const first = fakeServer("first"); + const router = new TerminalSessionRouter(first.client); + const view = recorder(); + router.subscribe("t1", view.callbacks); + await settle(); + first.exits[0]?.push(0); + await settle(); + expect(view.exits).toEqual([0]); + + const second = fakeServer("second"); + router.setClient(second.client); + await settle(); + expect(second.attaches).toHaveLength(0); + }); + + test("a pop-out router attaches as a pop-out again", async () => { + const router = new TerminalSessionRouter(fakeServer("first").client, { popout: true }); + router.subscribe("t1", recorder().callbacks); + const second = fakeServer("second"); + router.setClient(second.client); + await settle(); + expect(second.attaches[0]?.input).toEqual({ sessionId: "t1", popout: true }); + }); + + test("a first screen after a reconnect does not replace when nothing was shown", async () => { + const router = new TerminalSessionRouter(null); + const view = recorder(); + router.subscribe("t1", view.callbacks); + const server = fakeServer("first"); + router.setClient(server.client); + await settle(); + server.attaches[0]?.push({ type: "screenState", data: "screen" }); + await settle(); + expect(view.screens).toEqual([{ state: "screen", replace: false }]); + }); +}); diff --git a/src/browser/terminal/TerminalSessionRouter.ts b/src/browser/terminal/TerminalSessionRouter.ts index a8f08011234..c5c9eb66600 100644 --- a/src/browser/terminal/TerminalSessionRouter.ts +++ b/src/browser/terminal/TerminalSessionRouter.ts @@ -23,12 +23,18 @@ import type { RouterClient } from "@orpc/server"; import type { AppRouter } from "@/node/orpc/router"; import { getErrorMessage } from "@/common/utils/errors"; +import { isConnectionLostError } from "@/browser/contexts/connectionLink"; type APIClient = RouterClient; export interface TerminalSubscriberCallbacks { onOutput: (data: string) => void; - onScreenState: (state: string) => void; + /** + * The full screen (with the server's scrollback). `replace` is set when it follows a reattach + * after a reconnect: the subscriber already shows an older screen and must reset before + * writing, or the scrollback lines repeat. + */ + onScreenState: (state: string, options?: { replace: boolean }) => void; onExit: (code: number) => void; } @@ -37,8 +43,12 @@ interface SessionState { subscribers: Map; /** Cached screen state (sent to new subscribers immediately) */ screenState: string | null; - /** Abort controller for the attach stream */ + /** Abort controller for the attach and exit streams of the current client */ abortController: AbortController; + /** Bumped on each new client: a stream of an older client stops delivering. */ + generation: number; + /** The next screenState follows a reattach and replaces the screen. */ + replaceNextScreen: boolean; /** Whether the session has exited */ exited: boolean; /** Exit code if exited */ @@ -47,23 +57,49 @@ interface SessionState { let nextSubscriberId = 1; +/** + * The router lives as long as its provider. When the API client changes (a reconnect), it keeps + * every session, subscriber and cached screen and restarts the streams on the new client, so an + * open terminal keeps its screen instead of being cleared. With no client (the connection is + * down), streams stop, input is dropped, and the sessions wait. + */ export class TerminalSessionRouter { - private readonly api: APIClient; + private api: APIClient | null; private sessions = new Map(); + /** The last size sent per session, sent again after a reattach. */ + private sizes = new Map(); /** True in a pop-out terminal window: its attaches keep the session out of the sidebar. */ private readonly popout: boolean; - constructor(api: APIClient, options?: { popout?: boolean }) { + constructor(api: APIClient | null, options?: { popout?: boolean }) { this.api = api; this.popout = options?.popout === true; } - /** Get the API client (for identity comparison when recreating router) */ - getApi(): APIClient { + /** The client the streams run on now; null while the connection is down. */ + getApi(): APIClient | null { return this.api; } + /** + * Move every session to a new client (a reconnect), or stop the streams (null). Sessions, + * subscribers and cached screens stay. Exited sessions are not restarted. + */ + setClient(api: APIClient | null): void { + if (api === this.api) return; + this.api = api; + for (const [sessionId, session] of this.sessions) { + session.abortController.abort(); + session.abortController = new AbortController(); + session.generation += 1; + if (session.exited) continue; + // Only a subscriber that already shows a screen must reset before the new one. + session.replaceNextScreen = session.screenState !== null; + if (api != null) this.startStream(sessionId, session); + } + } + /** * Subscribe to a terminal session's output. * @@ -85,6 +121,8 @@ export class TerminalSessionRouter { subscribers: new Map(), screenState: null, abortController: new AbortController(), + generation: 0, + replaceNextScreen: false, exited: false, }; this.sessions.set(sessionId, session); @@ -141,6 +179,7 @@ export class TerminalSessionRouter { console.debug(`[TerminalRouter] No more subscribers for ${sessionId}, tearing down stream`); currentSession.abortController.abort(); this.sessions.delete(sessionId); + this.sizes.delete(sessionId); } }; } @@ -256,7 +295,17 @@ export class TerminalSessionRouter { * Send input to a terminal session. */ sendInput(sessionId: string, data: string): void { - void this.api.terminal.sendInput({ sessionId, data }); + this.withClient( + () => console.debug(`[TerminalRouter] Dropped input for ${sessionId}: not connected`), + (api) => { + // Typing while the connection drops must not surface as an unhandled rejection. + api.terminal + .sendInput({ sessionId, data }) + .catch((err: unknown) => + console.debug(`[TerminalRouter] Input for ${sessionId} not sent:`, err) + ); + } + ); } /** @@ -266,7 +315,17 @@ export class TerminalSessionRouter { * (This is used by TerminalView to keep frontend and PTY dimensions in sync.) */ resize(sessionId: string, cols: number, rows: number): Promise { - return this.api.terminal.resize({ sessionId, cols, rows }); + // Offline, the size is kept and sent again once the streams restart. + this.sizes.set(sessionId, { cols, rows }); + return this.withClient( + () => Promise.resolve(), + (api) => api.terminal.resize({ sessionId, cols, rows }) + ); + } + + /** Runs `run` on the current client, or `offline` while the connection is down. */ + private withClient(offline: () => T, run: (api: APIClient) => T): T { + return this.api == null ? offline() : run(this.api); } /** @@ -277,84 +336,122 @@ export class TerminalSessionRouter { session.abortController.abort(); } this.sessions.clear(); + this.sizes.clear(); } // ============================================================================ // Private methods // ============================================================================ + /** Starts the attach and exit streams of a session on the current client, if any. */ private startStream(sessionId: string, session: SessionState): void { + const api = this.api; + if (api == null) return; const { signal } = session.abortController; + const generation = session.generation; + // Streams of an older client (or of a removed session) stop delivering. + const isCurrent = () => + !signal.aborted && + this.sessions.get(sessionId) === session && + session.generation === generation; console.debug(`[TerminalRouter] Starting stream for session ${sessionId}`); - - // Start attach stream (fire-and-forget, but managed by abort controller) - void (async () => { - try { - const iterator = await this.api.terminal.attach( - this.popout ? { sessionId, popout: true } : { sessionId }, - { signal } + const size = this.sizes.get(sessionId); + if (size != null) { + // The PTY may have missed a resize while the connection was down. + api.terminal + .resize({ sessionId, ...size }) + .catch((err: unknown) => + console.debug(`[TerminalRouter] Resize after reattach failed:`, err) ); - for await (const msg of iterator) { - // Check if session was removed (unsubscribed) - const currentSession = this.sessions.get(sessionId); - if (!currentSession) { - console.debug(`[TerminalRouter] Session ${sessionId} removed, stopping stream`); - break; - } - - if (msg.type === "screenState") { - // Cache and broadcast - currentSession.screenState = msg.data; - console.debug( - `[TerminalRouter] Broadcasting screenState for ${sessionId} to ${currentSession.subscribers.size} subscribers` - ); - for (const callbacks of currentSession.subscribers.values()) { - callbacks.onScreenState(msg.data); - } - } else if (msg.type === "output") { - // Centralized shim for OSC 10/11 color queries that some TUIs rely on. - this.handleTerminalQueries(sessionId, msg.data); - - // Broadcast to all subscribers - for (const callbacks of currentSession.subscribers.values()) { - callbacks.onOutput(msg.data); - } - } - } - } catch (err) { - if (!signal.aborted) { - console.error(`[TerminalRouter] Stream error for ${sessionId}:`, err); + } + void this.runAttachStream(api, sessionId, session, signal, isCurrent); + void this.runExitStream(api, sessionId, session, signal, isCurrent); + } + + private async runAttachStream( + api: APIClient, + sessionId: string, + session: SessionState, + signal: AbortSignal, + isCurrent: () => boolean + ): Promise { + try { + const iterator = await api.terminal.attach( + this.popout ? { sessionId, popout: true } : { sessionId }, + { signal } + ); + for await (const msg of iterator) { + if (!isCurrent()) { + console.debug(`[TerminalRouter] Stream for ${sessionId} superseded, stopping`); + break; } + this.deliverMessage(sessionId, session, msg); } - })(); + } catch (err) { + // A lost connection is not a stream error: the next client restarts the stream. + if (isCurrent() && !isConnectionLostError(err)) { + console.error(`[TerminalRouter] Stream error for ${sessionId}:`, err); + } + } + } - // Start exit stream - void (async () => { - try { - const iterator = await this.api.terminal.onExit({ sessionId }, { signal }); - for await (const code of iterator) { - const currentSession = this.sessions.get(sessionId); - if (!currentSession) break; - - currentSession.exited = true; - currentSession.exitCode = code; - - // Broadcast to all subscribers - for (const callbacks of currentSession.subscribers.values()) { - callbacks.onExit(code); - } - break; // Exit only happens once - } - } catch (err) { - if (!signal.aborted) { - // Ignore "session not found" errors for exit stream - const errMsg = getErrorMessage(err); - if (!errMsg.includes("isOpen") && !errMsg.includes("undefined")) { - console.error(`[TerminalRouter] Exit stream error for ${sessionId}:`, err); - } + private async runExitStream( + api: APIClient, + sessionId: string, + session: SessionState, + signal: AbortSignal, + isCurrent: () => boolean + ): Promise { + try { + const iterator = await api.terminal.onExit({ sessionId }, { signal }); + for await (const code of iterator) { + if (!isCurrent()) break; + session.exited = true; + session.exitCode = code; + for (const callbacks of session.subscribers.values()) { + callbacks.onExit(code); } + break; // Exit only happens once + } + } catch (err) { + if (isCurrent() && !isExpectedExitStreamError(err)) { + console.error(`[TerminalRouter] Exit stream error for ${sessionId}:`, err); } - })(); + } + } + + private deliverMessage( + sessionId: string, + session: SessionState, + msg: { type: "screenState" | "output"; data: string } + ): void { + if (msg.type === "screenState") { + this.deliverScreen(session, msg.data); + return; + } + // Centralized shim for OSC 10/11 color queries that some TUIs rely on. + this.handleTerminalQueries(sessionId, msg.data); + for (const callbacks of session.subscribers.values()) { + callbacks.onOutput(msg.data); + } } + + /** Cache and broadcast a full screen; the first one after a reattach replaces the old one. */ + private deliverScreen(session: SessionState, state: string): void { + session.screenState = state; + const replace = session.replaceNextScreen; + session.replaceNextScreen = false; + for (const callbacks of session.subscribers.values()) { + if (replace) callbacks.onScreenState(state, { replace: true }); + else callbacks.onScreenState(state); + } + } +} + +/** Exit-stream errors that mean nothing ended: a lost connection, or no session yet. */ +export function isExpectedExitStreamError(err: unknown): boolean { + if (isConnectionLostError(err)) return true; + const message = getErrorMessage(err); + return message.includes("isOpen") || message.includes("undefined"); } From 0865d397cd8040acf2374de574386b8b5e7078fb Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 07:47:11 +0000 Subject: [PATCH 2/6] tests: the router's inventory rule checks for setClient() --- src/browser/contexts/subscriptionInventory.test.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/browser/contexts/subscriptionInventory.test.ts b/src/browser/contexts/subscriptionInventory.test.ts index df2a3cba550..88712279f88 100644 --- a/src/browser/contexts/subscriptionInventory.test.ts +++ b/src/browser/contexts/subscriptionInventory.test.ts @@ -72,10 +72,11 @@ const RULE_SHAPES: Record /\bsetClient\s*\(\s*\w+\s*:/.test(text), }, - "router-per-client": { - describe: "a router bound to one client (the provider builds one per client)", - matches: (text) => /constructor\(api: APIClient\b/.test(text), + "router-setClient": { + describe: "a setClient(api) method that restarts the streams", + matches: (text) => /\bsetClient\s*\(\s*api\s*:/.test(text), }, + "holder-backoff": { describe: "a retry with a delay", matches: (text) => text.includes("setTimeout(") && /backoff|retry/i.test(text), From 3c4fc0ab586eb3044d6423922cabf512a13cc538 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 07:59:07 +0000 Subject: [PATCH 3/6] fix: replace the terminal screen with a VT full reset (RIS), not Terminal.reset() (use-after-free in ghostty-web) --- .../TerminalView/TerminalView.test.tsx | 11 ++-- .../components/TerminalView/TerminalView.tsx | 24 ++++---- .../TerminalView/writeScreenState.test.ts | 55 +++++++++++++++++++ 3 files changed, 72 insertions(+), 18 deletions(-) create mode 100644 src/browser/components/TerminalView/writeScreenState.test.ts diff --git a/src/browser/components/TerminalView/TerminalView.test.tsx b/src/browser/components/TerminalView/TerminalView.test.tsx index ff5e793ddec..16ea96d909a 100644 --- a/src/browser/components/TerminalView/TerminalView.test.tsx +++ b/src/browser/components/TerminalView/TerminalView.test.tsx @@ -185,18 +185,17 @@ describe("TerminalView", () => { // After a reconnect the router sends the full screen again. Written on top of the old screen, // the scrollback would repeat, so a replacing screen starts from an empty terminal. - test("a screen that replaces the old one resets the terminal before writing", async () => { + test("a screen that replaces the old one is written after a full VT reset", async () => { renderTerminal(mock((_exitCode: number) => undefined)); await waitFor(() => expect(mockRouter.subscribe).toHaveBeenCalledTimes(1)); const terminal = terminalInstances[0]; expect(terminal?.calls).toEqual(["write:initial screen"]); act(() => subscribeCallbacks[0]?.onScreenState("screen after reconnect", { replace: true })); - expect(terminal?.calls).toEqual([ - "write:initial screen", - "reset", - "write:screen after reconnect", - ]); + // One write: a full reset (RIS) in the VT, then the screen. Never Terminal.reset(), which + // frees ghostty's WASM terminal while other parts of ghostty-web still use it. + expect(terminal?.calls).toEqual(["write:initial screen", "write:\x1bcscreen after reconnect"]); + expect(terminal?.reset).toHaveBeenCalledTimes(0); // The view keeps its one subscription: no new subscribe, no clear. expect(mockRouter.subscribe).toHaveBeenCalledTimes(1); expect(terminal?.clear).toHaveBeenCalledTimes(1); diff --git a/src/browser/components/TerminalView/TerminalView.tsx b/src/browser/components/TerminalView/TerminalView.tsx index c9304db4ecb..41bae1c7619 100644 --- a/src/browser/components/TerminalView/TerminalView.tsx +++ b/src/browser/components/TerminalView/TerminalView.tsx @@ -101,28 +101,28 @@ async function preloadTerminalWebfonts( } } +/** RIS (full reset): clears the screen, the scrollback and the modes inside the VT itself. */ +const FULL_RESET = "\x1bc"; + /** * Writes a full screen from the server (may be empty for new sessions). * * `replace` is set after a reconnect, when the router reattaches and sends the full screen again - * (TerminalSessionRouter). clear() keeps the scrollback, so writing on top would repeat it: - * start from an empty terminal instead. + * (TerminalSessionRouter). clear() keeps the scrollback, so writing on top would repeat it: the + * screen is written after a full reset instead, in one write, so no empty frame shows. + * + * Not Terminal.reset(): in ghostty-web it frees the WASM terminal and makes a new one, but the + * selection manager and input handler keep the freed one, and the next write after a second + * reconnect failed with "memory access out of bounds" and garbled the screen. */ export function writeScreenState( - term: Pick, + term: Pick, state: string, replace: boolean ): void { - if (replace) { - try { - term.reset(); - } catch (err) { - console.warn("[TerminalView] Error resetting terminal:", err); - } - } - if (!state) return; + if (!replace && !state) return; try { - term.write(state); + term.write(replace ? FULL_RESET + state : state); } catch (err) { // xterm WASM can throw "memory access out of bounds" intermittently console.warn("[TerminalView] Error writing screenState:", err); diff --git a/src/browser/components/TerminalView/writeScreenState.test.ts b/src/browser/components/TerminalView/writeScreenState.test.ts new file mode 100644 index 00000000000..3b630fe9b93 --- /dev/null +++ b/src/browser/components/TerminalView/writeScreenState.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, test } from "bun:test"; +import { dirname, join } from "node:path"; +import { Ghostty } from "ghostty-web"; +import { writeScreenState } from "./TerminalView"; + +// From the file: the test DOM's URL rejects the long data: URL that Ghostty.load() uses by default. +const WASM_PATH = join(dirname(Bun.resolveSync("ghostty-web", import.meta.dir)), "ghostty-vt.wasm"); + +/** Every non-empty line of a real ghostty VT, scrollback first. */ +function visibleLines(term: ReturnType, rows: number): string[] { + const text = (cells: Array<{ codepoint: number }> | null) => + (cells ?? []) + .map((cell) => String.fromCodePoint(cell.codepoint || 32)) + .join("") + .trimEnd(); + const lines: string[] = []; + for (let i = 0; i < term.getScrollbackLength(); i++) lines.push(text(term.getScrollbackLine(i))); + for (let row = 0; row < rows; row++) lines.push(text(term.getLine(row))); + return lines.filter((line) => line.length > 0); +} + +// Runs on the real VT (WASM), so a ghostty-web change to how RIS treats the scrollback fails here. +describe("writeScreenState on the ghostty VT", () => { + test("a replacing screen leaves no line of the old screen or scrollback behind", async () => { + const ghostty = await Ghostty.load(WASM_PATH); + const term = ghostty.createTerminal(80, 24); + try { + let before = ""; + for (let i = 1; i <= 60; i++) before += `line-${i}\r\n`; + term.write(before); + + // The server's screen state after a reconnect: its own copy of the same scrollback. + writeScreenState({ write: (data) => term.write(data) }, before + "after-reconnect\r\n", true); + + const lines = visibleLines(term, 24); + expect(lines.filter((line) => line === "line-1")).toHaveLength(1); + expect(lines.filter((line) => line === "line-60")).toHaveLength(1); + expect(lines.at(-1)).toBe("after-reconnect"); + } finally { + term.free(); + } + }); + + test("a first screen is written as is", async () => { + const ghostty = await Ghostty.load(WASM_PATH); + const term = ghostty.createTerminal(80, 24); + try { + term.write("kept\r\n"); + writeScreenState({ write: (data) => term.write(data) }, "first\r\n", false); + expect(visibleLines(term, 24)).toEqual(["kept", "first"]); + } finally { + term.free(); + } + }); +}); From 06a7ed52778c073c124b80912460342405706991 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 08:17:02 +0000 Subject: [PATCH 4/6] fix: the terminal router gets only the live client, and attach waits for the replayed resize --- .../terminal/TerminalRouterContext.test.tsx | 13 ++++++- .../terminal/TerminalRouterContext.tsx | 5 +-- .../terminal/TerminalSessionRouter.test.ts | 36 ++++++++++++++++++- src/browser/terminal/TerminalSessionRouter.ts | 25 +++++++------ 4 files changed, 65 insertions(+), 14 deletions(-) diff --git a/src/browser/terminal/TerminalRouterContext.test.tsx b/src/browser/terminal/TerminalRouterContext.test.tsx index a54e9a7028a..171afc29380 100644 --- a/src/browser/terminal/TerminalRouterContext.test.tsx +++ b/src/browser/terminal/TerminalRouterContext.test.tsx @@ -10,10 +10,13 @@ import type { TerminalSessionRouter } from "./TerminalSessionRouter"; /** One entry per connect: the attach calls that connection's server saw. */ let attachesPerConnection: number[] = []; +/** One entry per connect: the sizes that connection's server got. */ +let resizesPerConnection: Array> = []; function serverOverrides() { const index = attachesPerConnection.length; attachesPerConnection.push(0); + resizesPerConnection.push([]); return { terminal: { attach: () => { @@ -33,7 +36,10 @@ function serverOverrides() { yield 0; })() ), - resize: () => Promise.resolve(), + resize: (input: { cols: number; rows: number }) => { + resizesPerConnection[index]?.push({ cols: input.cols, rows: input.rows }); + return Promise.resolve(); + }, }, }; } @@ -64,6 +70,7 @@ describe.each([...GAP_MODES])("TerminalRouterProvider across a reconnect (%s)", beforeEach(() => { cleanupDom = installDom(); attachesPerConnection = []; + resizesPerConnection = []; routers = []; subscribes = 0; screens = []; @@ -87,7 +94,11 @@ describe.each([...GAP_MODES])("TerminalRouterProvider across a reconnect (%s)", expect(router).not.toBeNull(); await harness.drop(); + // A resize in the gap goes to no client, and to the PTY once the next client attaches. + await router?.resize("t1", 100, 30); + expect(resizesPerConnection[0]).toEqual([]); harness.reconnect(); + await waitFor(() => expect(resizesPerConnection[1]).toEqual([{ cols: 100, rows: 30 }])); await waitFor(() => expect(attachesPerConnection).toEqual([1, 1])); await waitFor(() => expect(screens.at(-1)).toEqual({ state: "screen-1", replace: true })); diff --git a/src/browser/terminal/TerminalRouterContext.tsx b/src/browser/terminal/TerminalRouterContext.tsx index 5880a111f85..ff2fe50c3a2 100644 --- a/src/browser/terminal/TerminalRouterContext.tsx +++ b/src/browser/terminal/TerminalRouterContext.tsx @@ -6,7 +6,7 @@ */ import { createContext, useContext, useEffect, useState } from "react"; -import { useAPI } from "@/browser/contexts/API"; +import { useLiveAPI } from "@/browser/contexts/API"; import { TerminalSessionRouter } from "./TerminalSessionRouter"; const TerminalRouterContext = createContext(null); @@ -27,7 +27,8 @@ interface TerminalRouterProviderProps { * Always renders children so the app UI stays visible during reconnection. */ export function TerminalRouterProvider(props: TerminalRouterProviderProps) { - const { api } = useAPI(); + // Only the live client: a reconnecting state can keep the old one, and calls on it fail. + const api = useLiveAPI(); const [router, setRouter] = useState(null); const popout = props.popout === true; diff --git a/src/browser/terminal/TerminalSessionRouter.test.ts b/src/browser/terminal/TerminalSessionRouter.test.ts index 6c74b05bc32..f6f9d3f22c3 100644 --- a/src/browser/terminal/TerminalSessionRouter.test.ts +++ b/src/browser/terminal/TerminalSessionRouter.test.ts @@ -12,6 +12,8 @@ function fakeServer(name: string) { const resizes: Array<{ cols: number; rows: number }> = []; let releaseAttach: (() => void) | null = null; let holdAttach = false; + let heldResize: { resolve: () => void; reject: (error: Error) => void } | null = null; + let holdResize = false; function stream(register: (push: (value: T) => void) => void, signal?: AbortSignal) { const queue: T[] = []; @@ -52,7 +54,8 @@ function fakeServer(name: string) { }, resize: (input: { cols: number; rows: number }) => { resizes.push({ cols: input.cols, rows: input.rows }); - return Promise.resolve(); + if (!holdResize) return Promise.resolve(); + return new Promise((resolve, reject) => (heldResize = { resolve, reject })); }, }, }); @@ -70,6 +73,14 @@ function fakeServer(name: string) { holdAttach = false; releaseAttach?.(); }, + holdNextResize() { + holdResize = true; + }, + settleResize(error?: Error) { + holdResize = false; + if (error == null) heldResize?.resolve(); + else heldResize?.reject(error); + }, }; } @@ -157,6 +168,29 @@ describe("TerminalSessionRouter across reconnects", () => { expect(second.inputs).toEqual(["typed"]); }); + // attach's first message is the server's screen: taken before the resize, it has the old width. + test.each([ + ["succeeds", undefined], + ["fails", new Error("resize failed")], + ])("after a reconnect, attach waits until the replayed resize %s", async (_, error) => { + const first = fakeServer("first"); + const router = new TerminalSessionRouter(first.client); + router.subscribe("t1", recorder().callbacks); + await settle(); + router.setClient(null); + await router.resize("t1", 120, 40); + + const second = fakeServer("second"); + second.holdNextResize(); + router.setClient(second.client); + await settle(); + expect(second.resizes).toEqual([{ cols: 120, rows: 40 }]); + expect(second.attaches).toHaveLength(0); + second.settleResize(error); + await settle(); + expect(second.attaches).toHaveLength(1); + }); + test("an exited session is not restarted", async () => { const first = fakeServer("first"); const router = new TerminalSessionRouter(first.client); diff --git a/src/browser/terminal/TerminalSessionRouter.ts b/src/browser/terminal/TerminalSessionRouter.ts index c5c9eb66600..ba49ee8279e 100644 --- a/src/browser/terminal/TerminalSessionRouter.ts +++ b/src/browser/terminal/TerminalSessionRouter.ts @@ -357,15 +357,17 @@ export class TerminalSessionRouter { console.debug(`[TerminalRouter] Starting stream for session ${sessionId}`); const size = this.sizes.get(sessionId); - if (size != null) { - // The PTY may have missed a resize while the connection was down. - api.terminal - .resize({ sessionId, ...size }) - .catch((err: unknown) => - console.debug(`[TerminalRouter] Resize after reattach failed:`, err) - ); - } - void this.runAttachStream(api, sessionId, session, signal, isCurrent); + // The PTY may have missed a resize while the connection was down. attach waits for it: its + // first message is the server's screen, which must already have the new size (PTY first). + const resized = + size == null + ? Promise.resolve() + : api.terminal + .resize({ sessionId, ...size }) + .catch((err: unknown) => + console.debug(`[TerminalRouter] Resize after reattach failed:`, err) + ); + void this.runAttachStream(api, sessionId, session, signal, isCurrent, resized); void this.runExitStream(api, sessionId, session, signal, isCurrent); } @@ -374,8 +376,11 @@ export class TerminalSessionRouter { sessionId: string, session: SessionState, signal: AbortSignal, - isCurrent: () => boolean + isCurrent: () => boolean, + resized: Promise ): Promise { + await resized; + if (!isCurrent()) return; try { const iterator = await api.terminal.attach( this.popout ? { sessionId, popout: true } : { sessionId }, From 17b9272d4456416303bf16c6a1b7a64dea651408 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 08:21:28 +0000 Subject: [PATCH 5/6] fix: an empty first screen after a reconnect keeps the shown screen (a restarted server sends "" for a session it does not know) --- .../terminal/TerminalSessionRouter.test.ts | 18 ++++++++++++++++++ src/browser/terminal/TerminalSessionRouter.ts | 5 ++++- 2 files changed, 22 insertions(+), 1 deletion(-) diff --git a/src/browser/terminal/TerminalSessionRouter.test.ts b/src/browser/terminal/TerminalSessionRouter.test.ts index f6f9d3f22c3..9c8e59e343b 100644 --- a/src/browser/terminal/TerminalSessionRouter.test.ts +++ b/src/browser/terminal/TerminalSessionRouter.test.ts @@ -191,6 +191,24 @@ describe("TerminalSessionRouter across reconnects", () => { expect(second.attaches).toHaveLength(1); }); + test("an empty first screen after a reconnect keeps the shown screen", async () => { + const first = fakeServer("first"); + const router = new TerminalSessionRouter(first.client); + const view = recorder(); + router.subscribe("t1", view.callbacks); + await settle(); + first.attaches[0]?.push({ type: "screenState", data: "screen-1" }); + await settle(); + + // A restarted server no longer knows the session: its screen is empty. + const second = fakeServer("second"); + router.setClient(second.client); + await settle(); + second.attaches[0]?.push({ type: "screenState", data: "" }); + await settle(); + expect(view.screens).toEqual([{ state: "screen-1", replace: false }]); + }); + test("an exited session is not restarted", async () => { const first = fakeServer("first"); const router = new TerminalSessionRouter(first.client); diff --git a/src/browser/terminal/TerminalSessionRouter.ts b/src/browser/terminal/TerminalSessionRouter.ts index ba49ee8279e..6c32f3b6206 100644 --- a/src/browser/terminal/TerminalSessionRouter.ts +++ b/src/browser/terminal/TerminalSessionRouter.ts @@ -444,9 +444,12 @@ export class TerminalSessionRouter { /** Cache and broadcast a full screen; the first one after a reattach replaces the old one. */ private deliverScreen(session: SessionState, state: string): void { - session.screenState = state; const replace = session.replaceNextScreen; session.replaceNextScreen = false; + // The server sends "" for a session it does not know, e.g. after a server restart. Replacing + // with it would erase the last screen of a dead session, so the view keeps what it shows. + if (replace && state === "") return; + session.screenState = state; for (const callbacks of session.subscribers.values()) { if (replace) callbacks.onScreenState(state, { replace: true }); else callbacks.onScreenState(state); From 0aec776cd9c5e8cb57978a1bc22ad37001cd9418 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Tue, 13 Oct 2026 09:18:25 +0000 Subject: [PATCH 6/6] revert: an empty first screen after a reconnect replaces the screen again (an empty snapshot can be a real blank screen) --- .../terminal/TerminalSessionRouter.test.ts | 18 ------------------ src/browser/terminal/TerminalSessionRouter.ts | 5 +---- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/src/browser/terminal/TerminalSessionRouter.test.ts b/src/browser/terminal/TerminalSessionRouter.test.ts index 9c8e59e343b..f6f9d3f22c3 100644 --- a/src/browser/terminal/TerminalSessionRouter.test.ts +++ b/src/browser/terminal/TerminalSessionRouter.test.ts @@ -191,24 +191,6 @@ describe("TerminalSessionRouter across reconnects", () => { expect(second.attaches).toHaveLength(1); }); - test("an empty first screen after a reconnect keeps the shown screen", async () => { - const first = fakeServer("first"); - const router = new TerminalSessionRouter(first.client); - const view = recorder(); - router.subscribe("t1", view.callbacks); - await settle(); - first.attaches[0]?.push({ type: "screenState", data: "screen-1" }); - await settle(); - - // A restarted server no longer knows the session: its screen is empty. - const second = fakeServer("second"); - router.setClient(second.client); - await settle(); - second.attaches[0]?.push({ type: "screenState", data: "" }); - await settle(); - expect(view.screens).toEqual([{ state: "screen-1", replace: false }]); - }); - test("an exited session is not restarted", async () => { const first = fakeServer("first"); const router = new TerminalSessionRouter(first.client); diff --git a/src/browser/terminal/TerminalSessionRouter.ts b/src/browser/terminal/TerminalSessionRouter.ts index 6c32f3b6206..ba49ee8279e 100644 --- a/src/browser/terminal/TerminalSessionRouter.ts +++ b/src/browser/terminal/TerminalSessionRouter.ts @@ -444,12 +444,9 @@ export class TerminalSessionRouter { /** Cache and broadcast a full screen; the first one after a reattach replaces the old one. */ private deliverScreen(session: SessionState, state: string): void { + session.screenState = state; const replace = session.replaceNextScreen; session.replaceNextScreen = false; - // The server sends "" for a session it does not know, e.g. after a server restart. Replacing - // with it would erase the last screen of a dead session, so the view keeps what it shows. - if (replace && state === "") return; - session.screenState = state; for (const callbacks of session.subscribers.values()) { if (replace) callbacks.onScreenState(state, { replace: true }); else callbacks.onScreenState(state);