diff --git a/packages/app/e2e/regression/terminal-tab-switch.spec.ts b/packages/app/e2e/regression/terminal-tab-switch.spec.ts index 7e44e00d96..cbb72958ad 100644 --- a/packages/app/e2e/regression/terminal-tab-switch.spec.ts +++ b/packages/app/e2e/regression/terminal-tab-switch.spec.ts @@ -10,6 +10,9 @@ const sessionB = "ses_terminal_tab_b" const titleA = "Alpha session" const titleB = "Beta session" const ptyID = "pty_tab_switch" +const server = `http://${process.env.PLAYWRIGHT_SERVER_HOST ?? "127.0.0.1"}:${process.env.PLAYWRIGHT_SERVER_PORT ?? "4096"}` +// Marks the terminal DOM node so a remount (fresh node) is detectable. +const PROBE = "original" test.use({ viewport: { width: 1440, height: 900 } }) @@ -17,8 +20,7 @@ test.use({ viewport: { width: 1440, height: 900 } }) // workspace must keep the terminal mounted and its PTY connection open instead // of tearing it down and reconnecting. test("keeps the terminal session alive when switching session tabs in a workspace", async ({ page }) => { - const connections: string[] = [] - await setup(page, connections) + const connections = await setup(page) await page.goto(sessionHref(sessionA)) await expectSessionTitle(page, titleA) @@ -27,34 +29,38 @@ test("keeps the terminal session alive when switching session tabs in a workspac const terminal = page.locator('[data-component="terminal"]') await expect(terminal).toBeVisible() await expect.poll(() => connections.length).toBe(1) - await terminal.evaluate((el) => { - ;(el as HTMLElement & { __e2eProbe?: string }).__e2eProbe = "original" - }) + await writeProbe(page) await switchTab(page, titleB) await expectSessionTitle(page, titleB) await expect(terminal).toBeVisible() - expect(await probe(page)).toBe("original") + expect(await readProbe(page)).toBe(PROBE) expect(connections.length).toBe(1) await switchTab(page, titleA) await expectSessionTitle(page, titleA) await expect(terminal).toBeVisible() - expect(await probe(page)).toBe("original") + expect(await readProbe(page)).toBe(PROBE) expect(connections.length).toBe(1) }) +type Probed = HTMLElement & { __e2eProbe?: string } + async function switchTab(page: Page, title: string) { await page.locator("[data-titlebar-tab-slot]", { hasText: title }).click() } -async function probe(page: Page) { - return page - .locator('[data-component="terminal"]') - .evaluate((el) => (el as HTMLElement & { __e2eProbe?: string }).__e2eProbe) +async function writeProbe(page: Page) { + await page.locator('[data-component="terminal"]').evaluate((el, probe) => { + ;(el as Probed).__e2eProbe = probe + }, PROBE) } -async function setup(page: Page, connections: string[]) { +async function readProbe(page: Page) { + return page.locator('[data-component="terminal"]').evaluate((el) => (el as Probed).__e2eProbe) +} + +async function setup(page: Page) { await mockOpenCodeServer(page, { directory, project: { @@ -97,11 +103,11 @@ async function setup(page: Page, connections: string[]) { body: JSON.stringify({ ticket: "e2e-ticket" }), }), ) + const connections: string[] = [] await page.routeWebSocket(new RegExp(`/pty/${ptyID}/connect`), (ws) => { connections.push(ws.url()) }) - const server = `http://${process.env.PLAYWRIGHT_SERVER_HOST ?? "127.0.0.1"}:${process.env.PLAYWRIGHT_SERVER_PORT ?? "4096"}` await page.addInitScript( ({ directory, server, sessions }) => { localStorage.setItem("settings.v3", JSON.stringify({ general: { newLayoutDesigns: true } })) @@ -119,6 +125,7 @@ async function setup(page: Page, connections: string[]) { }, { directory, server, sessions: [sessionA, sessionB] }, ) + return connections } function session(id: string, title: string, created: number) { @@ -134,6 +141,5 @@ function session(id: string, title: string, created: number) { } function sessionHref(sessionID: string) { - const server = `http://${process.env.PLAYWRIGHT_SERVER_HOST ?? "127.0.0.1"}:${process.env.PLAYWRIGHT_SERVER_PORT ?? "4096"}` return `/server/${base64Encode(server)}/session/${sessionID}` } diff --git a/packages/app/src/app.tsx b/packages/app/src/app.tsx index 4ab0a95c23..c7cdc73f9b 100644 --- a/packages/app/src/app.tsx +++ b/packages/app/src/app.tsx @@ -54,7 +54,7 @@ import { ErrorPage } from "./pages/error" import { useCheckServerHealth } from "./utils/server-health" import { legacySessionServer, requireServerKey, sessionHref } from "./utils/session-route" -import { SessionPage, TargetSessionRoute as TargetSessionRouteContent } from "@/pages/session" +import { SessionPage, TargetSessionRouteContent } from "@/pages/session" import { NewHome, LegacyHome } from "@/pages/home" const NewSession = lazy(() => import("@/pages/new-session")) @@ -100,6 +100,9 @@ const TargetSessionRoute = () => { }) return ( + // Owns the server-identity remount. Session changes must NOT remount this + // subtree (SessionRouteErrorBoundary resets and createSessionLineage + // re-resolves reactively instead); both rely on this key for server changes. diff --git a/packages/app/src/context/server-session.ts b/packages/app/src/context/server-session.ts index 9ba712ef25..36cf56c708 100644 --- a/packages/app/src/context/server-session.ts +++ b/packages/app/src/context/server-session.ts @@ -14,6 +14,7 @@ import type { import { batch } from "solid-js" import { createStore, produce, reconcile } from "solid-js/store" import { diffs as cleanDiffs, message as cleanMessage } from "@/utils/diffs" +import { sessionNotFoundError } from "@/utils/server-errors" import { rootSession } from "@/utils/session-route" import { dropSessionCaches, pickSessionCacheEvictions, SESSION_CACHE_LIMIT } from "./global-sync/session-cache" @@ -235,7 +236,7 @@ export function createServerSession(client: OpencodeClient, options?: { retry?: if (pending) return pending const active = generation(sessionID) const request = client.session.get({ sessionID }).then((result) => { - if (!result.data) throw new Error(`Session not found: ${sessionID}`) + if (!result.data) throw sessionNotFoundError(sessionID) if (generations.get(sessionID) !== active) return result.data return remember(result.data) }) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 379a007f31..c558df6ceb 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -84,7 +84,7 @@ import { Identifier } from "@/utils/id" import { diffs as list } from "@/utils/diffs" import { Persist, persisted } from "@/utils/persist" import { extractPromptFromParts } from "@/utils/prompt" -import { formatServerError, isSessionNotFoundError } from "@/utils/server-errors" +import { formatServerError, isLocalSessionNotFoundError, isSessionNotFoundError } from "@/utils/server-errors" import { legacySessionHref, requireServerKey, sessionHref } from "@/utils/session-route" import { useUsageExceededDialogs } from "./session/usage-exceeded-dialogs" import { createSessionOwnership } from "./session/session-ownership" @@ -104,10 +104,6 @@ const sessionViewState = () => ({ changes: "git" as ChangeMode, }) -function isLocalSessionNotFoundError(error: unknown, sessionID: string) { - return error instanceof Error && error.message === `Session not found: ${sessionID}` -} - function isCurrentSessionNotFoundError(error: unknown, sessionID: string | undefined) { if (!sessionID) return false return isSessionNotFoundError(error, sessionID) || isLocalSessionNotFoundError(error, sessionID) @@ -144,18 +140,17 @@ export function SessionPage() { ) } -export function TargetSessionRoute() { +// Rendered under app.tsx's TargetSessionRoute, which owns the per-server keyed +// remount around the server-scoped providers. Nothing here may key on the +// session ID: session tabs on the same server share this route instance, and +// workspace-scoped state (terminal, directory providers) lives below. +export function TargetSessionRouteContent() { const params = useParams<{ serverKey: string; id: string }>() return ( ( - + )} > @@ -163,17 +158,12 @@ export function TargetSessionRoute() { ) } -function SessionRouteFallback(props: { - error: unknown - sessionID?: string - serverKey?: ServerConnection.Key - padded?: boolean -}) { +function SessionRouteFallback(props: { error: unknown; sessionID: string; serverKey: ServerConnection.Key }) { const settings = useSettings() return ( }> - - + + @@ -248,6 +238,10 @@ function ResolvedTargetSessionRoute() { return ( params.id}> + {/* Non-keyed: closes only while the target's directory is unknown (uncached + lineage mid-resolution), which tears down the workspace subtree including + the terminal. Same-workspace tab switches keep it open because warm + targets resolve synchronously from the sync cache. */} JSX.Element children: JSX.Element }) { diff --git a/packages/app/src/pages/session/session-lineage.ts b/packages/app/src/pages/session/session-lineage.ts index 97416a9152..d86c7b657f 100644 --- a/packages/app/src/pages/session/session-lineage.ts +++ b/packages/app/src/pages/session/session-lineage.ts @@ -1,4 +1,13 @@ import { createEffect, createMemo, createSignal, on, onCleanup } from "solid-js" +import { sessionNotFoundError } from "@/utils/server-errors" + +type LineageStore = { peek: (id: string) => T | undefined; resolve: (id: string) => Promise } + +type Resolution = { id: string; store: LineageStore } & ( + | { state: "pending" } + | { state: "settled" } + | { state: "failed"; failure: unknown } +) // Reactive session lineage for the target session route, read from the sync store. // All session tabs on a server share one route instance, so the target session ID @@ -11,36 +20,33 @@ import { createEffect, createMemo, createSignal, on, onCleanup } from "solid-js" // render. // // The returned accessor is a pure derivation. The sync cache is authoritative, and -// status only applies while it matches the current target: on navigation the memo -// re-evaluates before the trigger runs, so trusting a previous target's settlement -// would fabricate a not-found for a session that simply has not resolved yet. -// Resolve failures rethrow on read so the enclosing SessionRouteErrorBoundary -// renders the scoped session error. -export function createSessionLineage( - sessionID: () => string, - lineage: () => { peek: (id: string) => T | undefined; resolve: (id: string) => Promise }, -) { +// status only applies while it matches the current target (store + session ID): on +// navigation or store replacement the memo re-evaluates before the trigger runs, +// so trusting a previous target's settlement would fabricate a not-found for a +// session that simply has not resolved yet. Resolve failures rethrow on read so +// the enclosing SessionRouteErrorBoundary renders the scoped session error. +export function createSessionLineage(sessionID: () => string, lineage: () => LineageStore) { const cached = createMemo(() => lineage().peek(sessionID())) - const [status, setStatus] = createSignal<{ id: string; settled: boolean; failure?: unknown }>() + const [status, setStatus] = createSignal>() createEffect( - on(sessionID, (id) => { + on([sessionID, lineage] as const, ([id, store]) => { let stale = false onCleanup(() => { stale = true }) if (cached()) { - setStatus({ id, settled: true }) + setStatus({ id, store, state: "settled" }) return } - setStatus({ id, settled: false }) - lineage() + setStatus({ id, store, state: "pending" }) + store .resolve(id) .then(() => { - if (!stale) setStatus({ id, settled: true }) + if (!stale) setStatus({ id, store, state: "settled" }) }) - .catch((error) => { - if (!stale) setStatus({ id, settled: true, failure: error }) + .catch((failure) => { + if (!stale) setStatus({ id, store, state: "failed", failure }) }) }), ) @@ -50,13 +56,14 @@ export function createSessionLineage( const value = cached() if (value) return value const state = status() - if (state?.id !== id) return undefined - if (state.failure !== undefined) throw state.failure - // The viewed session is pinned and pinned lineages are exempt from cache pruning, - // so a lineage missing after settlement means the session (or an ancestor) was - // deleted, possibly by another client. Match the resolve error so the boundary - // shows the session not found fallback. - if (state.settled) throw new Error(`Session not found: ${id}`) + if (state?.id !== id || state.store !== lineage()) return undefined + if (state.state === "failed") throw state.failure + // The viewed session is pinned (DirectoryDataProvider, directory-layout.tsx) + // and pinned lineages are exempt from cache pruning, so a lineage missing + // after settlement means the session (or an ancestor) was deleted, possibly + // by another client. Match the resolve error so the boundary shows the + // session not found fallback. + if (state.state === "settled") throw sessionNotFoundError(id) return undefined }) } diff --git a/packages/app/src/utils/server-errors.ts b/packages/app/src/utils/server-errors.ts index 409b49f54e..b34ae609ae 100644 --- a/packages/app/src/utils/server-errors.ts +++ b/packages/app/src/utils/server-errors.ts @@ -42,6 +42,20 @@ function unwrapNamedError(error: unknown): unknown { return error } +// Client-synthesized session not-found errors share one constructor and +// predicate so the message contract cannot drift between the sync store +// (server-session.ts), the route lineage (session-lineage.ts), and the +// not-found fallback matching (session.tsx). +const sessionNotFoundMessage = (sessionID: string) => `Session not found: ${sessionID}` + +export function sessionNotFoundError(sessionID: string) { + return new Error(sessionNotFoundMessage(sessionID)) +} + +export function isLocalSessionNotFoundError(error: unknown, sessionID: string) { + return error instanceof Error && error.message === sessionNotFoundMessage(sessionID) +} + export function isSessionNotFoundError(error: unknown, sessionID: string) { const unwrapped = unwrapNamedError(error) if (typeof unwrapped !== "object" || unwrapped === null) return false diff --git a/packages/app/test-browser/session-lineage.test.ts b/packages/app/test-browser/session-lineage.test.ts index 70443d3d9f..2c2154d190 100644 --- a/packages/app/test-browser/session-lineage.test.ts +++ b/packages/app/test-browser/session-lineage.test.ts @@ -4,6 +4,8 @@ import { createSessionLineage } from "@/pages/session/session-lineage" type Lineage = { session: { id: string; directory: string } } +const lineageOf = (id: string): Lineage => ({ session: { id, directory: `/dir/${id}` } }) + // Fake sync lineage store: peek reads a reactive cache, resolve returns a // deferred promise the test settles or fails explicitly. The lineage memo is // live (read below), so it recomputes eagerly on cache/status writes — throws @@ -25,11 +27,14 @@ function createFixture(initial: Record = {}) { }, }, settle(id: string) { - setCache({ ...cache(), [id]: { session: { id, directory: `/dir/${id}` } } }) + setCache({ ...cache(), [id]: lineageOf(id) }) deferred.get(id)?.resolve(undefined) }, fail(id: string, error: unknown) { deferred.get(id)?.reject(error) + // The real store does not cache failures: the inflight request entry is + // dropped on rejection so the next resolve retries (server-session.ts). + deferred.delete(id) }, remove(id: string) { const next = { ...cache() } @@ -39,13 +44,13 @@ function createFixture(initial: Record = {}) { } } +// Two microtask ticks: one for the resolve promise handed back by the fixture, +// one for the .then/.catch chain inside createSessionLineage. const flush = async () => { await Promise.resolve() await Promise.resolve() } -const lineageOf = (id: string): Lineage => ({ session: { id, directory: `/dir/${id}` } }) - test("resolves an uncached session and exposes its lineage", async () => { await createRoot(async (dispose) => { const fixture = createFixture() @@ -143,6 +148,64 @@ test("returning to a pruned session re-resolves instead of throwing not found", }) }) +// A resolution that fails while its session is unfocused must not leave a +// poisoned status behind: revisiting that session retries cleanly instead of +// rethrowing the stale failure before the retry can start. +test("revisiting a session whose resolution failed while unfocused retries cleanly", async () => { + await createRoot(async (dispose) => { + const fixture = createFixture() + const [id, setId] = createSignal("ses_a") + const current = createSessionLineage(id, () => fixture.lineage) + + await flush() + setId("ses_b") + fixture.fail("ses_a", new Error("resolve failed")) + await flush() + + expect(() => { + setId("ses_a") + current() + }).not.toThrow() + expect(fixture.resolves).toEqual(["ses_a", "ses_b", "ses_a"]) + + fixture.settle("ses_a") + await flush() + expect(current()?.session.id).toBe("ses_a") + + dispose() + }) +}) + +// The lineage accessor is reactive: replacing the sync store (for example after +// the server context is rebuilt) must gate out the old store's status and +// re-resolve against the new one instead of fabricating a not-found. +test("re-resolves against a replaced lineage store", async () => { + await createRoot(async (dispose) => { + const first = createFixture() + const second = createFixture() + const [store, setStore] = createSignal(first.lineage) + const current = createSessionLineage(() => "ses_a", store) + + await flush() + first.settle("ses_a") + await flush() + expect(current()?.session.id).toBe("ses_a") + + expect(() => { + setStore(second.lineage) + current() + }).not.toThrow() + await flush() + expect(second.resolves).toEqual(["ses_a"]) + + second.settle("ses_a") + await flush() + expect(current()?.session.id).toBe("ses_a") + + dispose() + }) +}) + // The viewed session is pinned in the cache, so disappearing after settlement // means it was deleted; the boundary must show the not found fallback. test("throws not found when the settled session is deleted", async () => {