mirror of
https://github.com/stablyai/orca.git
synced 2026-10-01 00:02:10 +00:00
fix(terminal): truthful handle liveness + no forked resume tabs for hidden restorable panes (#12574)
* fix(runtime): report terminal handles disconnected on controller-proven PTY absence leaf.connected mirrors the renderer graph (ptyId !== null), so a restored surface whose PTY died with a prior process was listed connected/writable forever with empty title/lastOutputAt/preview — the exact signature automation saw on run6 workspaces after a restart. listTerminals now threads the controller inventory it already fetches into buildTerminalSummary and demotes only on proven absence, only for locally-scoped ids; unknown liveness and SSH/remote scopes never demote, and no session or pane is retired. * fix(terminal): stop forking hidden restorable panes into replacement resume tabs paneWillConnectOnActivation still assumed the pre-keep-alive mount model, but every non-parked tab of the active worktree mounts and connects hidden at 0x0. Activation therefore appended a replacement resume tab per non-group-active agent pane and handed it the sleeping record, stranding the hidden pane as a bare shell — or forking two live surfaces onto one provider session when the old PTY survived in the daemon. The predicate now answers "will mount and connect": any non-web-mirror tab of the active worktree qualifies; non-active worktrees still answer false so background wake keeps its append-based resume. Contract change: reverses the hidden-tab expectation from #6800, whose premise (hidden panes never connect) no longer holds; that test is updated in place. * test(terminal): pin the remote-scope exemption and the web-mirror ownership exception CodeRabbit flagged both exclusions as untested: a remote-runtime-scoped leaf absent from the local inventory must stay connected (its inventory lives on the remote host), and a web-mirror tab must not own sleeping-session recovery (it never mounts a local pane), so the appended replacement remains its correct resume path. * fix(terminal): rescue just-spawned ptys from absence demotion; unpark panes owning sleeping records Review (GPT verifier) confirmed two gaps: - listTerminals demoted a live just-spawned PTY when listProcesses snapshotted before session registration (the sweep's hasPty rescue is leaf-gated), and federation reads one connected:false as exited. The summary's proven-absence check now also consults the provider's sync hasPty. - Ordinary per-tab cold parking (30s hidden) kept a non-group-active pane unmounted, so a sleeping record it owns under the new ownership predicate could not cold-restore until the user revealed the tab. Per-tab parks now exempt panes owning a sleeping-session record; worktree-level parks are untouched (they clear on activation). * fix(terminal): reconcile the daemon session cache on inventory; scope the park exemption to consumable records Round-2 review confirmed two holes in the round-1 fixes: - DaemonPtyAdapter.hasPty is cached activeSessionIds membership, and a successful listSessions never removed ids the authoritative inventory omitted — an exit missed while the socket was down kept hasPty true forever, and the new spawn/list-race rescue would trust it, reopening connected-forever for that pty. listProcesses now drops pre-request cached ids the inventory does not list alive (ids spawned mid-flight are snapshot- protected). - The park exemption covered records a pane can never consume (automaticResumeBlockedBy, passive-completed evidence), pinning hidden panes mounted indefinitely. The exemption now lives in sleeping-record-park-exemption.ts and requires a consumable record. Also pins the web-mirror replacement's resume claim and startup command (CodeRabbit round-2). --------- Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
This commit is contained in:
@@ -1670,6 +1670,9 @@ type PtyControllerTerminalIdentity = Readonly<{
|
||||
|
||||
type PtyControllerInventory = Readonly<{
|
||||
livePtyIds: ReadonlySet<string>
|
||||
// Why: livePtyIds is worktree-scoped when a target is given; absence proofs
|
||||
// must consult the unscoped inventory or a misattributed live PTY reads as dead.
|
||||
allLivePtyIds: ReadonlySet<string>
|
||||
terminalIdentityByPtyId: ReadonlyMap<string, PtyControllerTerminalIdentity>
|
||||
}>
|
||||
|
||||
@@ -14955,13 +14958,20 @@ export class OrcaRuntimeService {
|
||||
: targetWorktreeId
|
||||
? []
|
||||
: [...worktreesById.values()]
|
||||
const refreshedPtyLiveness = await this.refreshPtyWorktreeRecordsFromController(
|
||||
const controllerInventory = await this.refreshPtyWorktreeRecordsWithControllerInventory(
|
||||
resolvedWorktrees,
|
||||
targetWorktreeId
|
||||
)
|
||||
const refreshedPtyLiveness = controllerInventory
|
||||
? new Set(controllerInventory.livePtyIds)
|
||||
: null
|
||||
if (opts.requireFreshPtyLiveness && !refreshedPtyLiveness) {
|
||||
throw new Error('terminal_liveness_unavailable')
|
||||
}
|
||||
// Why: a proof of absence, not a proof of liveness — leaves whose PTY the
|
||||
// controller answered for but did not list must not read as connected. An
|
||||
// unavailable inventory (null) proves nothing and demotes nothing.
|
||||
const provenLivePtyIds = controllerInventory?.allLivePtyIds ?? null
|
||||
|
||||
const livePtyWorktreeIds = new Set<string>()
|
||||
for (const pty of this.ptysById.values()) {
|
||||
@@ -14989,7 +14999,7 @@ export class OrcaRuntimeService {
|
||||
if (leaf.ptyId) {
|
||||
ptyIdsFromLeaves.add(leaf.ptyId)
|
||||
}
|
||||
terminals.push(this.buildTerminalSummary(leaf, worktreesById))
|
||||
terminals.push(this.buildTerminalSummary(leaf, worktreesById, provenLivePtyIds))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -28582,6 +28592,7 @@ export class OrcaRuntimeService {
|
||||
if (targetedLiveness !== null) {
|
||||
return {
|
||||
livePtyIds: targetedLiveness,
|
||||
allLivePtyIds: targetedLiveness,
|
||||
terminalIdentityByPtyId: new Map()
|
||||
}
|
||||
}
|
||||
@@ -28797,6 +28808,7 @@ export class OrcaRuntimeService {
|
||||
this.pruneDisconnectedPtyRecords()
|
||||
return {
|
||||
livePtyIds: targetWorktreeId ? selectedLivePtyIds : allLivePtyIds,
|
||||
allLivePtyIds,
|
||||
terminalIdentityByPtyId: controllerIdentityByPtyId
|
||||
}
|
||||
}
|
||||
@@ -29009,12 +29021,28 @@ export class OrcaRuntimeService {
|
||||
|
||||
private buildTerminalSummary(
|
||||
leaf: RuntimeLeafRecord,
|
||||
worktreesById: Map<string, ResolvedWorktree>
|
||||
worktreesById: Map<string, ResolvedWorktree>,
|
||||
provenLivePtyIds: ReadonlySet<string> | null = null
|
||||
): RuntimeTerminalSummary {
|
||||
const worktree = worktreesById.get(leaf.worktreeId)
|
||||
const tab = this.tabs.get(leaf.tabId) ?? null
|
||||
|
||||
const pty = leaf.ptyId ? this.ptysById.get(leaf.ptyId) : undefined
|
||||
// Why: leaf.connected mirrors the renderer graph (`ptyId !== null`), so a
|
||||
// restored surface whose PTY died with a prior run still reads connected.
|
||||
// Demote only on a controller-proven absence, and only for locally-scoped
|
||||
// ids the aggregate inventory authoritatively covers — SSH/remote scopes may
|
||||
// be legitimately missing from it, and unknown liveness never demotes.
|
||||
// The sync hasPty rescue closes the spawn/list race: a just-spawned PTY can
|
||||
// register after the inventory snapshot, and federation reads one
|
||||
// connected:false as exited.
|
||||
const provenAbsent =
|
||||
provenLivePtyIds !== null &&
|
||||
leaf.ptyId !== null &&
|
||||
!provenLivePtyIds.has(leaf.ptyId) &&
|
||||
!leaf.ptyId.startsWith('remote:') &&
|
||||
parseAppSshPtyId(leaf.ptyId) === null &&
|
||||
this.ptyController?.hasPty?.(leaf.ptyId) !== true
|
||||
return {
|
||||
handle: this.issueHandle(leaf),
|
||||
ptyId: leaf.ptyId,
|
||||
@@ -29026,8 +29054,8 @@ export class OrcaRuntimeService {
|
||||
tabId: leaf.tabId,
|
||||
leafId: leaf.leafId,
|
||||
title: getLatestLeafTitle(leaf, tab?.title ?? null),
|
||||
connected: leaf.connected,
|
||||
writable: leaf.writable,
|
||||
connected: provenAbsent ? false : leaf.connected,
|
||||
writable: provenAbsent ? false : leaf.writable,
|
||||
lastOutputAt: leaf.lastOutputAt,
|
||||
preview: leaf.preview
|
||||
}
|
||||
|
||||
@@ -0,0 +1,182 @@
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { OrcaRuntimeService } from './orca-runtime'
|
||||
import { getDefaultWorkspaceSession } from '../../shared/constants'
|
||||
import type { WorkspaceSessionState } from '../../shared/types'
|
||||
|
||||
// run6-review-pr-11959 repro: leaf.connected mirrors the graph (`ptyId !== null`),
|
||||
// so a restored leaf whose PTY no provider owns must be demoted from the
|
||||
// controller inventory or the CLI reports it connected/writable forever.
|
||||
|
||||
const WORKTREE_ID = 'repo-1::/tmp/probe-worktree'
|
||||
const LEAF_ID = '11111111-1111-4111-8111-111111111111'
|
||||
|
||||
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(() => [])
|
||||
}
|
||||
}
|
||||
|
||||
type ControllerSession = { id: string; cwd: string; title?: string }
|
||||
|
||||
function makeRuntimeWithLeaf(options: {
|
||||
leafPtyId: string
|
||||
controllerSessions: ControllerSession[] | 'unavailable'
|
||||
hasPty?: (ptyId: string) => boolean | null
|
||||
}): OrcaRuntimeService {
|
||||
const runtime = new OrcaRuntimeService(makeStore() as never)
|
||||
runtime.setPtyController({
|
||||
spawn: vi.fn(async () => ({ id: 'never' })),
|
||||
write: () => true,
|
||||
kill: () => true,
|
||||
...(options.hasPty ? { hasPty: options.hasPty } : {}),
|
||||
listProcesses:
|
||||
options.controllerSessions === 'unavailable'
|
||||
? vi.fn(async () => {
|
||||
throw new Error('controller unavailable')
|
||||
})
|
||||
: vi.fn(async () => options.controllerSessions)
|
||||
} as never)
|
||||
runtime.attachWindow(1)
|
||||
runtime.syncWindowGraph(1, {
|
||||
tabs: [
|
||||
{
|
||||
tabId: 'tab-1',
|
||||
worktreeId: WORKTREE_ID,
|
||||
title: '',
|
||||
activeLeafId: LEAF_ID,
|
||||
layout: null
|
||||
}
|
||||
],
|
||||
leaves: [
|
||||
{
|
||||
tabId: 'tab-1',
|
||||
worktreeId: WORKTREE_ID,
|
||||
leafId: LEAF_ID,
|
||||
paneRuntimeId: 1,
|
||||
ptyId: options.leafPtyId,
|
||||
paneTitle: null,
|
||||
title: ''
|
||||
}
|
||||
]
|
||||
})
|
||||
return runtime
|
||||
}
|
||||
|
||||
describe('listTerminals liveness truth for restored leaves', () => {
|
||||
it('reports a leaf disconnected when the controller inventory proves its local ptyId absent', async () => {
|
||||
const runtime = makeRuntimeWithLeaf({
|
||||
leafPtyId: 'pty-stale-from-prior-run',
|
||||
controllerSessions: []
|
||||
})
|
||||
|
||||
const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`)
|
||||
|
||||
expect(terminals).toHaveLength(1)
|
||||
expect(terminals[0]).toMatchObject({
|
||||
ptyId: 'pty-stale-from-prior-run',
|
||||
connected: false,
|
||||
writable: false
|
||||
})
|
||||
})
|
||||
|
||||
it('keeps a leaf connected when its ptyId is in the controller inventory', async () => {
|
||||
const runtime = makeRuntimeWithLeaf({
|
||||
leafPtyId: 'pty-live-1',
|
||||
controllerSessions: [{ id: 'pty-live-1', cwd: '/tmp/probe-worktree' }]
|
||||
})
|
||||
|
||||
const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`)
|
||||
|
||||
expect(terminals).toHaveLength(1)
|
||||
expect(terminals[0]).toMatchObject({
|
||||
ptyId: 'pty-live-1',
|
||||
connected: true,
|
||||
writable: true
|
||||
})
|
||||
})
|
||||
|
||||
it('never demotes on an unavailable inventory — unknown liveness is not absence', async () => {
|
||||
const runtime = makeRuntimeWithLeaf({
|
||||
leafPtyId: 'pty-stale-from-prior-run',
|
||||
controllerSessions: 'unavailable'
|
||||
})
|
||||
|
||||
const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`)
|
||||
|
||||
expect(terminals).toHaveLength(1)
|
||||
expect(terminals[0]).toMatchObject({
|
||||
ptyId: 'pty-stale-from-prior-run',
|
||||
connected: true,
|
||||
writable: true
|
||||
})
|
||||
})
|
||||
|
||||
// Why: a just-spawned PTY can register after the inventory snapshot; the
|
||||
// provider's sync hasPty must rescue it or federation reads one
|
||||
// connected:false as exited.
|
||||
it('keeps a leaf connected when the provider synchronously knows a ptyId the snapshot missed', async () => {
|
||||
const runtime = makeRuntimeWithLeaf({
|
||||
leafPtyId: 'pty-just-spawned',
|
||||
controllerSessions: [],
|
||||
hasPty: (ptyId) => ptyId === 'pty-just-spawned'
|
||||
})
|
||||
|
||||
const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`)
|
||||
|
||||
expect(terminals).toHaveLength(1)
|
||||
expect(terminals[0]).toMatchObject({
|
||||
ptyId: 'pty-just-spawned',
|
||||
connected: true,
|
||||
writable: true
|
||||
})
|
||||
})
|
||||
|
||||
it('does not demote remote-runtime-scoped leaves the local inventory never covers', async () => {
|
||||
const runtime = makeRuntimeWithLeaf({
|
||||
leafPtyId: 'remote:env-1@@term_abc',
|
||||
controllerSessions: []
|
||||
})
|
||||
|
||||
const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`)
|
||||
|
||||
expect(terminals).toHaveLength(1)
|
||||
expect(terminals[0]).toMatchObject({
|
||||
ptyId: 'remote:env-1@@term_abc',
|
||||
connected: true,
|
||||
writable: true
|
||||
})
|
||||
})
|
||||
|
||||
it('does not demote SSH-scoped leaves the aggregate inventory may not cover', async () => {
|
||||
const runtime = makeRuntimeWithLeaf({
|
||||
leafPtyId: 'ssh:target-1@@session-9',
|
||||
controllerSessions: []
|
||||
})
|
||||
|
||||
const { terminals } = await runtime.listTerminals(`id:${WORKTREE_ID}`)
|
||||
|
||||
expect(terminals).toHaveLength(1)
|
||||
expect(terminals[0]).toMatchObject({
|
||||
ptyId: 'ssh:target-1@@session-9',
|
||||
connected: true,
|
||||
writable: true
|
||||
})
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user