diff --git a/src/main/runtime/orca-runtime-build-pty-terminal-summary.ts b/src/main/runtime/orca-runtime-build-pty-terminal-summary.ts index 615353bbc13..159a709fd3c 100644 --- a/src/main/runtime/orca-runtime-build-pty-terminal-summary.ts +++ b/src/main/runtime/orca-runtime-build-pty-terminal-summary.ts @@ -5,12 +5,20 @@ import type { ResolvedWorktree } from './runtime-worktree-path-identity' import type { RuntimeTerminalRead, RuntimeTerminalSummary } from '../../shared/runtime-types' import { getLatestPtyTitle } from './runtime-worktree-status-projection' import { parsePaneKey } from '../../shared/stable-pane-id' +import { ptyHoldsRecordedSurface, type PtySurfaceTopology } from './pty-recorded-surface-topology' import type { TerminalHandleRecord } from './runtime-terminal-contracts' import { readTerminalTail } from './terminal-tail-read' import { structuredWorkerTerminalRefusal } from './structured-worker-terminal-refusal' import { randomUUID } from 'node:crypto' export class OrcaRuntimeWithBuildPtyTerminalSummary extends OrcaRuntimeWithGetPtyRecordForPaneKey { + protected ptySurfaceTopology(): PtySurfaceTopology { + return { + graphSequence: this.graphSequence, + ptyIdHoldingPane: (tabId, leafId) => this.leaves.get(this.getLeafKey(tabId, leafId))?.ptyId + } + } + protected buildPtyTerminalSummary( pty: RuntimePtyWorktreeRecord, worktreesById: Map @@ -19,7 +27,7 @@ export class OrcaRuntimeWithBuildPtyTerminalSummary extends OrcaRuntimeWithGetPt const title = getLatestPtyTitle(pty) const pane = parsePaneKey(pty.paneKey ?? '') - const orphaned = !pty.tabId || !pane || pane.tabId !== pty.tabId + const orphaned = !ptyHoldsRecordedSurface(pty, this.ptySurfaceTopology()) return { handle: this.issuePtyHandle(pty), ptyId: pty.ptyId, diff --git a/src/main/runtime/orca-runtime-record-pty-worktree.ts b/src/main/runtime/orca-runtime-record-pty-worktree.ts index 2d382f3d5eb..d4df1583000 100644 --- a/src/main/runtime/orca-runtime-record-pty-worktree.ts +++ b/src/main/runtime/orca-runtime-record-pty-worktree.ts @@ -56,6 +56,7 @@ export class OrcaRuntimeWithRecordPtyWorktree extends OrcaRuntimeWithRefreshRepo wslDistro, tabId: state.tabId ?? null, paneKey: state.paneKey ?? null, + surfaceRecordedAtGraphSequence: this.graphSequence, launchConfig: null, launchToken: null, launchIncarnationId: null, @@ -148,6 +149,7 @@ export class OrcaRuntimeWithRecordPtyWorktree extends OrcaRuntimeWithRefreshRepo } if (state.paneKey !== undefined) { pty.paneKey = state.paneKey + pty.surfaceRecordedAtGraphSequence = this.graphSequence } if (state.connected !== undefined) { pty.connected = state.connected diff --git a/src/main/runtime/orca-runtime-runtime-id.ts b/src/main/runtime/orca-runtime-runtime-id.ts index 00ccdde9da9..77fd49e8899 100644 --- a/src/main/runtime/orca-runtime-runtime-id.ts +++ b/src/main/runtime/orca-runtime-runtime-id.ts @@ -277,6 +277,10 @@ export class OrcaRuntimeWithRuntimeId { /** One-shot delivery retries, keyed by leaf. See checkDeliverySettledAndArmRecheck. */ protected deliveryRecheckTimersByLeafKey = new Map>() + // Why: counts authoritative graph statements so a PTY's recorded surface can be told apart + // from one the graph has simply not published yet (pty-recorded-surface-topology.ts). + protected graphSequence = 0 + protected leaves = new Map() // Why: PTY output is a per-keystroke hot path. Looking up affected leaves by diff --git a/src/main/runtime/orca-runtime-sync-window-graph.ts b/src/main/runtime/orca-runtime-sync-window-graph.ts index e820d15130b..70f71a03259 100644 --- a/src/main/runtime/orca-runtime-sync-window-graph.ts +++ b/src/main/runtime/orca-runtime-sync-window-graph.ts @@ -84,6 +84,9 @@ export class OrcaRuntimeWithSyncWindowGraph extends OrcaRuntimeWithAttachWindow ) const nextLeaves = new Map() const graphSyncedAt = this.nextTitleObservationSequence() + // Bumped before the leaf loop so surfaces this statement records are stamped with it, and a + // surface recorded after it is immune until the next one (pty-recorded-surface-topology.ts). + this.graphSequence += 1 // Why: renderer reloads can briefly republish the same leaf with no ptyId; // keep live CLI handles usable while the UI graph rebuilds. diff --git a/src/main/runtime/pty-recorded-surface-topology.test.ts b/src/main/runtime/pty-recorded-surface-topology.test.ts new file mode 100644 index 00000000000..a76cd1cd908 --- /dev/null +++ b/src/main/runtime/pty-recorded-surface-topology.test.ts @@ -0,0 +1,91 @@ +import { describe, expect, it } from 'vitest' +import { ptyHoldsRecordedSurface, type PtySurfaceTopology } from './pty-recorded-surface-topology' + +const TAB = 'tab-1' +const LEAF = '11111111-1111-4111-8111-111111111111' + +function pty(overrides: Partial[0]> = {}) { + return { + ptyId: 'pty-1', + tabId: TAB, + paneKey: `${TAB}:${LEAF}`, + surfaceRecordedAtGraphSequence: 0, + ...overrides + } +} + +function topology(overrides: Partial = {}): PtySurfaceTopology { + return { + graphSequence: 1, + ptyIdHoldingPane: () => 'pty-1', + ...overrides + } +} + +describe('ptyHoldsRecordedSurface', () => { + it('holds the surface when the graph binds the recorded pane to this PTY', () => { + expect(ptyHoldsRecordedSurface(pty(), topology())).toBe(true) + }) + + it('does not hold it when the graph has no such pane', () => { + // #18191: the record is self-consistent, so the incumbent check said "attached" forever. + expect(ptyHoldsRecordedSurface(pty(), topology({ ptyIdHoldingPane: () => undefined }))).toBe( + false + ) + }) + + it('does not hold it when the graph rebound that pane to another PTY', () => { + expect(ptyHoldsRecordedSurface(pty(), topology({ ptyIdHoldingPane: () => 'pty-2' }))).toBe( + false + ) + }) + + it('keeps every pane attached when a lost graph empties the leaf map', () => { + // Losing the graph clears every leaf without advancing the sequence, so the panes it held + // keep a current stamp. Reading that emptiness as absence would orphan them all at once. + expect( + ptyHoldsRecordedSurface( + pty({ surfaceRecordedAtGraphSequence: 4 }), + topology({ graphSequence: 4, ptyIdHoldingPane: () => undefined }) + ) + ).toBe(true) + }) + + it('keeps naming a pane already observed dropped after the graph goes away', () => { + // Losing the ability to re-check is not a reason to un-see the drop. + expect( + ptyHoldsRecordedSurface( + pty({ surfaceRecordedAtGraphSequence: 3 }), + topology({ graphSequence: 4, ptyIdHoldingPane: () => undefined }) + ) + ).toBe(false) + }) + + it('keeps a surface recorded since the last graph statement', () => { + // Spawn records the pane before the graph carrying it arrives (#7587); the only graph that + // has spoken since was already in flight, so its silence is not a retraction. + expect( + ptyHoldsRecordedSurface( + pty({ surfaceRecordedAtGraphSequence: 1 }), + topology({ graphSequence: 1, ptyIdHoldingPane: () => undefined }) + ) + ).toBe(true) + }) + + it('contradicts a surface once a later graph statement omits it', () => { + expect( + ptyHoldsRecordedSurface( + pty({ surfaceRecordedAtGraphSequence: 1 }), + topology({ graphSequence: 2, ptyIdHoldingPane: () => undefined }) + ) + ).toBe(false) + }) + + it('reports no surface when the record never named a pane', () => { + expect(ptyHoldsRecordedSurface(pty({ tabId: null, paneKey: null }), topology())).toBe(false) + }) + + it('reports no surface when the record disagrees with itself', () => { + expect(ptyHoldsRecordedSurface(pty({ paneKey: `other-tab:${LEAF}` }), topology())).toBe(false) + }) +}) diff --git a/src/main/runtime/pty-recorded-surface-topology.ts b/src/main/runtime/pty-recorded-surface-topology.ts new file mode 100644 index 00000000000..c5e85a97b90 --- /dev/null +++ b/src/main/runtime/pty-recorded-surface-topology.ts @@ -0,0 +1,59 @@ +/** + * Whether the pane a PTY record names as its surface still exists and still holds it. + * + * The incumbent test asked only whether the record agreed with *itself* — `!pty.tabId || !pane + * || pane.tabId !== pty.tabId`. A record whose `paneKey` still parses to its own `tabId` passes + * that forever, including after the graph dropped the pane, so a terminal that had lost its + * surface was reported `orphaned: false` with a `tabId` no tab has: field-for-field identical to + * a healthy one (#18191). Self-consistency is not topology; the leaf map is. + * + * An absence only counts against a record that a graph statement had the standing to contradict, + * which is what `graphSequence` decides. Every graph statement re-records the surface of every + * pane it publishes, so a pane the current graph holds always carries the current sequence and + * is answered without consulting the leaf map at all. That leaves two cases the sequence handles + * on its own, and neither needs a separate "is the graph usable" flag: + * + * - **Nothing has spoken yet.** Before the first sync, and for a surface recorded since the last + * one, the stamp is current. Spawn records the renderer's pane identity *before* the graph + * carrying it arrives, on purpose (#7587), so the only graph that could have spoken since was + * already in flight and its silence is not a retraction. + * - **The graph went away.** Losing it clears the leaf map wholesale without advancing the + * sequence, so every pane it held keeps a current stamp and stays attached. Reading that + * emptiness as "no pane holds this" would report every live terminal orphaned at once — the + * same lie as #18191, pointed the other way. A pane already observed dropped keeps its stale + * stamp and stays named, because losing the ability to re-check is not a reason to un-see it. + */ +import { parsePaneKey } from '../../shared/stable-pane-id' + +export type RecordedPtySurface = { + ptyId: string + tabId: string | null + paneKey: string | null + /** Value of `graphSequence` when this surface was last written. */ + surfaceRecordedAtGraphSequence: number +} + +export type PtySurfaceTopology = { + /** Monotonic count of authoritative graph statements applied so far. */ + graphSequence: number + /** The ptyId the graph currently binds to this pane, or undefined when it holds no such pane. */ + ptyIdHoldingPane: (tabId: string, leafId: string) => string | null | undefined +} + +/** + * True when the record names a pane the graph agrees this PTY occupies, or when nothing has had + * the standing to contradict it yet. False is the reportable state: a live PTY with no surface. + */ +export function ptyHoldsRecordedSurface( + pty: RecordedPtySurface, + topology: PtySurfaceTopology +): boolean { + const pane = parsePaneKey(pty.paneKey ?? '') + if (!pty.tabId || !pane || pane.tabId !== pty.tabId) { + return false + } + if (pty.surfaceRecordedAtGraphSequence >= topology.graphSequence) { + return true + } + return topology.ptyIdHoldingPane(pane.tabId, pane.leafId) === pty.ptyId +} diff --git a/src/main/runtime/runtime-terminal-state-records.ts b/src/main/runtime/runtime-terminal-state-records.ts index 8f54856be0d..82e5124bf40 100644 --- a/src/main/runtime/runtime-terminal-state-records.ts +++ b/src/main/runtime/runtime-terminal-state-records.ts @@ -54,6 +54,12 @@ export type RuntimePtyWorktreeRecord = RuntimeTerminalTailState & { wslDistro: string | null tabId: string | null paneKey: string | null + /** + * `graphSequence` when `paneKey` was last written. A surface recorded since the last graph + * statement has not yet been offered one that could contradict it — see + * pty-recorded-surface-topology.ts. + */ + surfaceRecordedAtGraphSequence: number launchConfig: SleepingAgentLaunchConfig | null launchToken: string | null launchIncarnationId: PtyIncarnationId | null diff --git a/src/main/runtime/terminal-list-surface-lost-orphan.test.ts b/src/main/runtime/terminal-list-surface-lost-orphan.test.ts new file mode 100644 index 00000000000..f9388df59db --- /dev/null +++ b/src/main/runtime/terminal-list-surface-lost-orphan.test.ts @@ -0,0 +1,183 @@ +import { describe, expect, it, vi } from 'vitest' +import { OrcaRuntimeService } from './orca-runtime' +import { getDefaultWorkspaceSession } from '../../shared/constants' +import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' + +// #18191: a terminal whose pane the graph dropped kept reporting `orphaned: false` with a +// `tabId` no tab has — "field-for-field identical to a healthy one", so an operator polling +// `terminal list` had no signal at all. The runtime asked whether the PTY record agreed with +// itself; it never asked the leaf topology whether that pane still exists. + +const WORKTREE_ID = 'repo-1::/tmp/probe-worktree' +const KEPT_LEAF = '11111111-1111-4111-8111-111111111111' +const DROPPED_LEAF = '22222222-2222-4222-8222-222222222222' +const KEPT_PTY = 'pty-ui-created' +const DROPPED_PTY = 'pty-cli-created' + +function makeStore() { + const session: WorkspaceSessionState = getDefaultWorkspaceSession() + return { + getWorkspaceSession: vi.fn(() => session), + setWorkspaceSession: vi.fn(), + getRepos: vi.fn(() => [ + { + id: 'repo-1', + path: '/tmp/probe-worktree', + displayName: 'probe', + badgeColor: '#000000', + addedAt: 0 + } + ]), + getAllWorktreeMeta: vi.fn(() => ({})), + getWorktreeMeta: vi.fn(() => undefined), + setWorktreeMeta: vi.fn(), + removeWorktreeMeta: vi.fn(), + getSettings: vi.fn(() => ({ workspaceDir: '/tmp/workspaces' })), + getProjects: vi.fn(() => []) + } +} + +function leaf(tabId: string, leafId: string, ptyId: string) { + return { + tabId, + worktreeId: WORKTREE_ID, + leafId, + paneRuntimeId: 1, + ptyId, + paneTitle: null, + title: '' + } +} + +function tab(tabId: string, activeLeafId: string) { + return { tabId, worktreeId: WORKTREE_ID, title: '', activeLeafId, layout: null } +} + +/** Both PTYs stay live on the host throughout; only the graph changes. */ +function makeRuntime(): OrcaRuntimeService { + const runtime = new OrcaRuntimeService(makeStore() as never) + runtime.setPtyController({ + spawn: vi.fn(async () => ({ id: 'never' })), + write: () => true, + kill: () => true, + listProcesses: vi.fn(async () => [ + { id: KEPT_PTY, cwd: '/tmp/probe-worktree' }, + { id: DROPPED_PTY, cwd: '/tmp/probe-worktree' } + ]) + } as never) + runtime.attachWindow(1) + runtime.syncWindowGraph(1, { + tabs: [tab('tab-kept', KEPT_LEAF), tab('tab-dropped', DROPPED_LEAF)], + leaves: [leaf('tab-kept', KEPT_LEAF, KEPT_PTY), leaf('tab-dropped', DROPPED_LEAF, DROPPED_PTY)] + }) + return runtime +} + +/** The restart republishes a graph that kept one pane and dropped the other. */ +function dropOnePane(runtime: OrcaRuntimeService): void { + runtime.syncWindowGraph(1, { + tabs: [tab('tab-kept', KEPT_LEAF)], + leaves: [leaf('tab-kept', KEPT_LEAF, KEPT_PTY)] + }) +} + +describe('terminal inventory after a pane is dropped', () => { + it('reports both terminals attached while both panes exist', async () => { + const runtime = makeRuntime() + const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`) + const byPty = new Map(terminals.map((terminal) => [terminal.ptyId, terminal])) + expect(byPty.get(KEPT_PTY)?.orphaned).toBe(false) + expect(byPty.get(DROPPED_PTY)?.orphaned).toBe(false) + }) + + it('distinguishes the surface-lost terminal from the healthy one', async () => { + const runtime = makeRuntime() + dropOnePane(runtime) + + const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`) + const byPty = new Map(terminals.map((terminal) => [terminal.ptyId, terminal])) + const kept = byPty.get(KEPT_PTY) + const dropped = byPty.get(DROPPED_PTY) + + // The whole defect in one line: these two readings used to be identical. + expect(dropped?.orphaned).not.toBe(kept?.orphaned) + expect(dropped?.orphaned).toBe(true) + expect(kept?.orphaned).toBe(false) + }) + + it('stops pointing callers at the tab that no longer exists', async () => { + const runtime = makeRuntime() + dropOnePane(runtime) + + const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`) + const dropped = terminals.find((terminal) => terminal.ptyId === DROPPED_PTY) + // `terminal close --tab tab-dropped` is what returned `tab_not_found` (#18191 §5). + expect(dropped?.tabId).not.toBe('tab-dropped') + expect(dropped?.tabId).toBe(`pty:${DROPPED_PTY}`) + }) + + it('keeps reporting the live PTY rather than dropping it from inventory', async () => { + const runtime = makeRuntime() + dropOnePane(runtime) + + const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`) + // Losing a surface is not evidence the process ended; it must stay listed and connected. + const dropped = terminals.find((terminal) => terminal.ptyId === DROPPED_PTY) + expect(dropped).toBeDefined() + expect(dropped?.connected).toBe(true) + }) + + it('does not call a surface recorded since the last graph statement orphaned', async () => { + const runtime = makeRuntime() + dropOnePane(runtime) + // Spawn records the renderer's pane identity before the graph carrying it arrives (#7587). + // Re-recording the dropped pane stands in for that: the graph has not spoken since, so its + // silence is not a retraction. This also pins that the runtime stamps the record at all — + // orca-runtime-record-pty-worktree.ts is `@ts-nocheck`, so a missing stamp is silent there + // and would leave every freshly spawned terminal reporting orphaned for one graph. + ;( + runtime as unknown as { + recordPtyWorktree: ( + ptyId: string, + worktreeId: string, + state: Record + ) => void + } + ).recordPtyWorktree(DROPPED_PTY, WORKTREE_ID, { + connected: true, + tabId: 'tab-dropped', + paneKey: `tab-dropped:${DROPPED_LEAF}` + }) + + const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`) + const dropped = terminals.find((terminal) => terminal.ptyId === DROPPED_PTY) + expect(dropped?.orphaned).toBe(false) + }) + + it('does not call every pane orphaned when the graph goes away', async () => { + const runtime = makeRuntime() + // Losing the authoritative graph clears the leaf map wholesale + // (transitionGraphReloadToTerminalState). That emptiness says nothing about any individual + // PTY, so reading it as "no pane holds this" would report every live terminal orphaned at + // once — the same lie as #18191, pointed the other way. + runtime.markGraphUnavailable(1) + + const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`) + expect(terminals.length).toBeGreaterThan(0) + for (const terminal of terminals) { + expect(terminal.orphaned).toBe(false) + } + }) + + it('keeps naming the dropped pane after the graph goes away', async () => { + const runtime = makeRuntime() + dropOnePane(runtime) + runtime.markGraphUnavailable(1) + + const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`) + const byPty = new Map(terminals.map((terminal) => [terminal.ptyId, terminal])) + // Losing the graph must not retract an observation already made. + expect(byPty.get(DROPPED_PTY)?.orphaned).toBe(true) + expect(byPty.get(KEPT_PTY)?.orphaned).toBe(false) + }) +})