Просмотр исходного кода

fix(app): gate session lineage by store identity and tidy route boundary

LukeParkerDev 1 месяц назад
Родитель
Сommit
cd06ecb694

+ 20 - 14
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}`
 }

+ 4 - 1
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.
     <Show when={requireServerKey(params.serverKey)} keyed>
       <ServerSDKProvider server={conn}>
         <ServerSyncProvider server={conn}>

+ 2 - 1
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)
     })

+ 17 - 20
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 (
     <SessionRouteErrorBoundary
       sessionID={params.id}
       fallback={(error) => (
-        <SessionRouteFallback
-          error={error}
-          sessionID={params.id}
-          serverKey={requireServerKey(params.serverKey)}
-          padded
-        />
+        <SessionRouteFallback error={error} sessionID={params.id} serverKey={requireServerKey(params.serverKey)} />
       )}
     >
       <ResolvedTargetSessionRoute />
@@ -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 (
     <Show when={settings.general.newLayoutDesigns()} fallback={<ErrorPage error={props.error} />}>
-      <SessionRouteFrame padded={props.padded}>
-        <SessionPanelFrame newLayout raised={!!props.sessionID}>
+      <SessionRouteFrame padded>
+        <SessionPanelFrame newLayout raised>
           <SessionErrorFallback error={props.error} sessionID={props.sessionID} serverKey={props.serverKey} />
         </SessionPanelFrame>
       </SessionRouteFrame>
@@ -248,6 +238,10 @@ function ResolvedTargetSessionRoute() {
 
   return (
     <TargetServerScopedProviders directory={directory} sessionID={() => 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. */}
       <Show when={directory()}>
         <Show
           when={settings.general.newLayoutDesigns()}
@@ -264,6 +258,9 @@ function ResolvedTargetSessionRoute() {
   )
 }
 
+// Owns the workspace-identity remount. Must not include the session ID in the
+// key: SessionPage handles session changes reactively, and remounting here
+// destroys workspace-scoped state (terminal PTYs, file/prompt providers).
 function TargetSessionPage() {
   const sdk = useSDK()
   const serverSDK = useServerSDK()

+ 3 - 2
packages/app/src/pages/session/route-boundary.ts

@@ -7,9 +7,10 @@ import type { JSX } from "solid-js"
 // WebSockets) that has to survive switching tabs within the same workspace.
 // Remount boundaries live elsewhere: app.tsx keys the route per server around
 // the server-scoped providers, and TargetSessionPage re-keys per workspace.
-// Kept free of app contexts (and JSX) so these semantics are directly testable.
+// Context-free so these semantics are directly unit-testable, and JSX-free so
+// bun test can import it without the Solid JSX transform.
 export function SessionRouteErrorBoundary(props: {
-  sessionID: string | undefined
+  sessionID: string
   fallback: (error: unknown) => JSX.Element
   children: JSX.Element
 }) {

+ 31 - 24
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<T> = { peek: (id: string) => T | undefined; resolve: (id: string) => Promise<unknown> }
+
+type Resolution<T> = { id: string; store: LineageStore<T> } & (
+  | { 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<T>(
-  sessionID: () => string,
-  lineage: () => { peek: (id: string) => T | undefined; resolve: (id: string) => Promise<unknown> },
-) {
+// 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<T>(sessionID: () => string, lineage: () => LineageStore<T>) {
   const cached = createMemo(() => lineage().peek(sessionID()))
-  const [status, setStatus] = createSignal<{ id: string; settled: boolean; failure?: unknown }>()
+  const [status, setStatus] = createSignal<Resolution<T>>()
 
   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<T>(
     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
   })
 }

+ 14 - 0
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

+ 66 - 3
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<string, Lineage> = {}) {
       },
     },
     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<string, Lineage> = {}) {
   }
 }
 
+// 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 () => {