mirror of
https://github.com/stablyai/orca.git
synced 2026-09-25 16:02:38 +00:00
fix(runtime): name a terminal whose pane the graph dropped
`buildPtyTerminalSummary` decided `orphaned` from the PTY record's agreement with itself — `!pty.tabId || !pane || pane.tabId !== pty.tabId`. A record whose `paneKey` still parses to its own `tabId` passes that forever, including long after the session graph dropped the pane, so a terminal that had lost its surface reported `orphaned: false, connected: true, writable: true` and a `tabId` no tab has: field-for-field identical to a healthy one (#18191). Consult the leaf topology instead, gated on a graph statement having had the standing to contradict the record. `graphSequence` counts authoritative graph statements; every statement re-records the surface of every pane it publishes, so a pane the current graph holds carries the current stamp and is answered without touching the leaf map. That covers the two absences that are not evidence, without a second flag: a surface recorded since the last statement (spawn records the pane before the graph carrying it arrives, #7587), and a lost graph clearing every leaf at once without advancing the sequence. 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. `orphaned: true` is shipped vocabulary that both consumers already read, so no capability gate is needed: adoption keys on it (`hasStrongOrphanIdentity`) and now reaches this population, and the duplicate-surface index (`indexLiveTerminalSurfaceOwners`) stops recording a destroyed pane as a PTY's live owner.
This commit is contained in:
@@ -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<string, ResolvedWorktree>
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -277,6 +277,10 @@ export class OrcaRuntimeWithRuntimeId {
|
||||
/** One-shot delivery retries, keyed by leaf. See checkDeliverySettledAndArmRecheck. */
|
||||
protected deliveryRecheckTimersByLeafKey = new Map<string, ReturnType<typeof setTimeout>>()
|
||||
|
||||
// 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<string, RuntimeLeafRecord>()
|
||||
|
||||
// Why: PTY output is a per-keystroke hot path. Looking up affected leaves by
|
||||
|
||||
@@ -84,6 +84,9 @@ export class OrcaRuntimeWithSyncWindowGraph extends OrcaRuntimeWithAttachWindow
|
||||
)
|
||||
const nextLeaves = new Map<string, RuntimeLeafRecord>()
|
||||
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.
|
||||
|
||||
@@ -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<Parameters<typeof ptyHoldsRecordedSurface>[0]> = {}) {
|
||||
return {
|
||||
ptyId: 'pty-1',
|
||||
tabId: TAB,
|
||||
paneKey: `${TAB}:${LEAF}`,
|
||||
surfaceRecordedAtGraphSequence: 0,
|
||||
...overrides
|
||||
}
|
||||
}
|
||||
|
||||
function topology(overrides: Partial<PtySurfaceTopology> = {}): 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)
|
||||
})
|
||||
})
|
||||
@@ -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
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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<string, unknown>
|
||||
) => 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)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user