From 27da04d50d4a9e27a672a6f76f264abaeeeab3ce Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Tue, 4 Aug 2026 17:20:09 -0700 Subject: [PATCH] fix(terminal): truthful handle liveness + no forked resume tabs for hidden restorable panes (#12574) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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> --- src/main/daemon/daemon-pty-adapter.test.ts | 15 + src/main/daemon/daemon-pty-adapter.ts | 13 + src/main/runtime/orca-runtime.ts | 38 ++- .../terminal-list-stale-leaf-liveness.test.ts | 182 ++++++++++ .../sleeping-record-park-exemption.ts | 29 ++ .../use-terminal-tab-cold-parking.test.ts | 86 ++++- .../use-terminal-tab-cold-parking.ts | 18 +- .../lib/resume-sleeping-agent-session.test.ts | 17 +- .../src/lib/sleeping-agent-pane-ownership.ts | 21 +- ...ivation-preserved-pane-replacement.test.ts | 321 ++++++++++++++++++ 10 files changed, 712 insertions(+), 28 deletions(-) create mode 100644 src/main/runtime/terminal-list-stale-leaf-liveness.test.ts create mode 100644 src/renderer/src/components/terminal-pane/sleeping-record-park-exemption.ts create mode 100644 src/renderer/src/lib/worktree-reactivation-preserved-pane-replacement.test.ts diff --git a/src/main/daemon/daemon-pty-adapter.test.ts b/src/main/daemon/daemon-pty-adapter.test.ts index 84147ddb8..4c5d39184 100644 --- a/src/main/daemon/daemon-pty-adapter.test.ts +++ b/src/main/daemon/daemon-pty-adapter.test.ts @@ -1657,6 +1657,21 @@ describe('DaemonPtyAdapter (IPtyProvider)', () => { }) describe('listProcesses', () => { + // Why: hasPty reads the activeSessionIds cache; an exit missed while the + // socket was down must not survive an authoritative inventory, or absence + // proofs (terminal list demotion, send guard) are defeated forever. + it('drops cached session ids an authoritative inventory omits', async () => { + const { id } = await adapter.spawn({ cols: 80, rows: 24 }) + const staleId = 'repo::/repo/stale@@deadbeef' + ;(adapter as unknown as { activeSessionIds: Set }).activeSessionIds.add(staleId) + expect(adapter.hasPty(staleId)).toBe(true) + + await adapter.listProcesses() + + expect(adapter.hasPty(staleId)).toBe(false) + expect(adapter.hasPty(id)).toBe(true) + }) + it('returns active sessions', async () => { await adapter.spawn({ cols: 80, diff --git a/src/main/daemon/daemon-pty-adapter.ts b/src/main/daemon/daemon-pty-adapter.ts index b9473c3de..0b0c0c41e 100644 --- a/src/main/daemon/daemon-pty-adapter.ts +++ b/src/main/daemon/daemon-pty-adapter.ts @@ -1382,6 +1382,9 @@ export class DaemonPtyAdapter implements IPtyProvider { } async listProcesses(opts?: { deadlineMs?: number }): Promise { + // Why: snapshotted before the request so ids spawned mid-flight can never + // be reconciled away below. + const preRequestActiveIds = new Set(this.activeSessionIds) try { // Why: connect + listSessions share the caller's one absolute deadline so a // wedged handshake cannot burn the whole teardown budget before the list issues. @@ -1393,10 +1396,12 @@ export class DaemonPtyAdapter implements IPtyProvider { ) const admission = new PtyProcessListAdmission() const processes: PtyProcessInfo[] = [] + const aliveSessionIds = new Set() for (const session of result.sessions) { if (!session.isAlive) { continue } + aliveSessionIds.add(session.sessionId) const { worktreeId } = parsePtySessionId(session.sessionId) processes.push( admission.admit({ @@ -1412,6 +1417,14 @@ export class DaemonPtyAdapter implements IPtyProvider { }) ) } + // Why: hasPty reads activeSessionIds, and an exit missed while the socket + // was disconnected otherwise survives an authoritative inventory forever — + // defeating every absence proof built on the cache. + for (const id of preRequestActiveIds) { + if (!aliveSessionIds.has(id)) { + this.activeSessionIds.delete(id) + } + } this.publishAuditObservation( recordAuthenticatedInventory(this.auditContext, this.exactDaemonIncarnation) ) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index bc72387da..a1d90d2cf 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -1670,6 +1670,9 @@ type PtyControllerTerminalIdentity = Readonly<{ type PtyControllerInventory = Readonly<{ livePtyIds: ReadonlySet + // 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 terminalIdentityByPtyId: ReadonlyMap }> @@ -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() 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 + worktreesById: Map, + provenLivePtyIds: ReadonlySet | 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 } diff --git a/src/main/runtime/terminal-list-stale-leaf-liveness.test.ts b/src/main/runtime/terminal-list-stale-leaf-liveness.test.ts new file mode 100644 index 000000000..5cbb967da --- /dev/null +++ b/src/main/runtime/terminal-list-stale-leaf-liveness.test.ts @@ -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 + }) + }) +}) diff --git a/src/renderer/src/components/terminal-pane/sleeping-record-park-exemption.ts b/src/renderer/src/components/terminal-pane/sleeping-record-park-exemption.ts new file mode 100644 index 000000000..e65dcf5bb --- /dev/null +++ b/src/renderer/src/components/terminal-pane/sleeping-record-park-exemption.ts @@ -0,0 +1,29 @@ +import type { SleepingAgentSessionRecord } from '../../../../shared/agent-session-resume' +import { isPassiveCompletedHibernationEvidence } from '../../lib/sleeping-agent-pane-ownership' + +const EMPTY_TAB_IDS: ReadonlySet = new Set() + +/** Tab ids whose panes own a sleeping record a mount can actually consume. + * Why: a parked pane can never cold-restore, so per-tab parks must exempt + * these — but only these: blocked and passive-completed records never resume, + * and exempting them would pin a hidden pane mounted indefinitely. */ +export function selectSleepingRecordParkExemptTabIds( + sleepingAgentSessionsByPaneKey: Record | undefined, + worktreeId: string +): ReadonlySet { + let owned: Set | null = null + for (const record of Object.values(sleepingAgentSessionsByPaneKey ?? {})) { + if (record.worktreeId !== worktreeId) { + continue + } + if (record.automaticResumeBlockedBy || isPassiveCompletedHibernationEvidence(record)) { + continue + } + const tabId = record.tabId ?? record.paneKey.slice(0, record.paneKey.indexOf(':')) + if (tabId) { + owned ??= new Set() + owned.add(tabId) + } + } + return owned ?? EMPTY_TAB_IDS +} diff --git a/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts b/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts index 7138edb37..99aabd719 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.test.ts @@ -8,7 +8,11 @@ const mocks = vi.hoisted(() => ({ pendingStartupByTabId: {} as Record, runtimeStatusByEnvironmentId: new Map(), settings: {} as Record, - terminalLayoutsByTabId: {} as Record }> + terminalLayoutsByTabId: {} as Record }>, + sleepingAgentSessionsByPaneKey: {} as Record< + string, + { paneKey: string; tabId?: string; worktreeId: string } + > }, exemptTabIds: new Set(), exemptSelectCalls: 0 @@ -76,6 +80,7 @@ describe('useTerminalTabColdParking measure-clock contract', () => { mocks.exemptTabIds = new Set() mocks.exemptSelectCalls = 0 mocks.storeState.terminalLayoutsByTabId = {} + mocks.storeState.sleepingAgentSessionsByPaneKey = {} mocks.storeState.runtimeStatusByEnvironmentId = new Map() }) @@ -240,4 +245,83 @@ describe('useTerminalTabColdParking measure-clock contract', () => { expect(mocks.exemptSelectCalls).toBe(callsAfterFirstRender) expect(result.current).toEqual(new Set(['tab-2'])) }) + + // Why: a parked pane can never cold-restore, so a per-tab park holding a + // sleeping-session record would strand the agent's resume until tab reveal. + it('unparks a per-tab-parked pane once it owns a sleeping-session record', () => { + const { result, rerender } = renderHook( + (args: ReturnType) => useTerminalTabColdParking(args), + { initialProps: hookArgs(false) } + ) + + act(() => { + vi.advanceTimersByTime(TERMINAL_TAB_HOT_RETAIN_MS + 1) + }) + expect(result.current).toEqual(new Set(['tab-2'])) + + mocks.storeState.sleepingAgentSessionsByPaneKey = { + 'tab-2:22222222-2222-4222-8222-222222222222': { + paneKey: 'tab-2:22222222-2222-4222-8222-222222222222', + tabId: 'tab-2', + worktreeId: WORKTREE_ID + } + } + act(() => { + rerender(hookArgs(false)) + }) + expect(result.current.size).toBe(0) + + // Records for other worktrees change nothing. + mocks.storeState.sleepingAgentSessionsByPaneKey = { + 'tab-2:22222222-2222-4222-8222-222222222222': { + paneKey: 'tab-2:22222222-2222-4222-8222-222222222222', + tabId: 'tab-2', + worktreeId: 'other-worktree' + } + } + act(() => { + rerender(hookArgs(false)) + }) + expect(result.current).toEqual(new Set(['tab-2'])) + }) + + // Why: blocked and passive-completed records never auto-resume, so exempting + // them would pin a hidden pane mounted indefinitely for nothing. + it('keeps parking panes whose records cannot be consumed', () => { + const { result, rerender } = renderHook( + (args: ReturnType) => useTerminalTabColdParking(args), + { initialProps: hookArgs(false) } + ) + act(() => { + vi.advanceTimersByTime(TERMINAL_TAB_HOT_RETAIN_MS + 1) + }) + expect(result.current).toEqual(new Set(['tab-2'])) + + mocks.storeState.sleepingAgentSessionsByPaneKey = { + 'tab-2:22222222-2222-4222-8222-222222222222': { + paneKey: 'tab-2:22222222-2222-4222-8222-222222222222', + tabId: 'tab-2', + worktreeId: WORKTREE_ID, + automaticResumeBlockedBy: 'legacy-orchestration-worker' + } as never + } + act(() => { + rerender(hookArgs(false)) + }) + expect(result.current).toEqual(new Set(['tab-2'])) + + mocks.storeState.sleepingAgentSessionsByPaneKey = { + 'tab-2:22222222-2222-4222-8222-222222222222': { + paneKey: 'tab-2:22222222-2222-4222-8222-222222222222', + tabId: 'tab-2', + worktreeId: WORKTREE_ID, + origin: 'worktree-sleep', + state: 'done' + } as never + } + act(() => { + rerender(hookArgs(false)) + }) + expect(result.current).toEqual(new Set(['tab-2'])) + }) }) diff --git a/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts b/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts index c339f00fe..dfde5d65c 100644 --- a/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts +++ b/src/renderer/src/components/terminal-pane/use-terminal-tab-cold-parking.ts @@ -29,6 +29,7 @@ import { selectEvictionExemptTerminalTabIds, selectEvictionExemptTerminalTabLayoutKey } from './terminal-eviction-exempt-tabs' +import { selectSleepingRecordParkExemptTabIds } from './sleeping-record-park-exemption' import { canWatcherCoverParkedTerminalTab, disposeParkedTerminalWatchersForWorktree, @@ -96,6 +97,13 @@ export function useTerminalTabColdParking(args: { () => selectPairedRuntimeParkingEnvironmentIds(runtimeStatusByEnvironmentId), [runtimeStatusByEnvironmentId] ) + const sleepingAgentSessionsByPaneKey = useAppStore( + (state) => state.sleepingAgentSessionsByPaneKey + ) + const sleepingRecordOwnedTabIds = useMemo( + () => selectSleepingRecordParkExemptTabIds(sleepingAgentSessionsByPaneKey, worktreeId), + [sleepingAgentSessionsByPaneKey, worktreeId] + ) const terminalTabHiddenSinceRef = useRef(new Map()) // Why (shared measure-clock contract with Terminal.tsx): tab hiddenSince // survives a background-measure window so per-tab park deadlines stay in @@ -288,7 +296,14 @@ export function useTerminalTabColdParking(args: { tabId: terminalTab.id }) !== null if ( - (coldParkTerminalPanes || (!isVisible && coldParkedTerminalTabIds.has(terminalTab.id))) && + (coldParkTerminalPanes || + (!isVisible && + coldParkedTerminalTabIds.has(terminalTab.id) && + // Why: a pane owning a sleeping-session record must stay mountable + // on an active worktree — parked it can never cold-restore, so the + // agent's resume strands until the user reveals the tab. Scoped to + // per-tab parks: the worktree-level park clears on activation. + !sleepingRecordOwnedTabIds.has(terminalTab.id))) && !hasActivityTerminalPortal && // Why: a force-parked worktree's eviction-exempt tabs keep their // mounted panes — a remount would orphan their live pty. Scoped to @@ -322,6 +337,7 @@ export function useTerminalTabColdParking(args: { evictionExemptTerminalTabIds, isWorktreeActive, shouldMeasureHiddenWorktree, + sleepingRecordOwnedTabIds, terminalTabs, worktreeId ]) diff --git a/src/renderer/src/lib/resume-sleeping-agent-session.test.ts b/src/renderer/src/lib/resume-sleeping-agent-session.test.ts index 51f921797..c4f219f15 100644 --- a/src/renderer/src/lib/resume-sleeping-agent-session.test.ts +++ b/src/renderer/src/lib/resume-sleeping-agent-session.test.ts @@ -196,7 +196,12 @@ describe('resumeSleepingAgentSessionsForWorktree', () => { expect(state.sleepingAgentSessionsByPaneKey[record.paneKey]).toBe(record) }) - it('resumes active worktree-sleep stable-pane records when the preserved tab is hidden during activation', () => { + // Why: keep-alive mounts every tab of the active worktree and pane connect is + // not visibility-gated, so a hidden restorable pane cold-restores in place. + // Appending a resume tab here (the pre-keep-alive contract from #6800) forked + // a second surface onto the same provider session and stranded the hidden + // pane as a bare shell. + it('leaves a hidden restorable stable-pane record for in-place cold restore during activation', () => { const paneKey = makePaneKey('tab-1', LEAF_ID) const record = makeRecord({ paneKey, origin: 'worktree-sleep' }) useAppStore.setState({ @@ -214,13 +219,9 @@ describe('resumeSleepingAgentSessionsForWorktree', () => { const launched = resumeSleepingAgentSessionsForWorktree('wt-1') const state = useAppStore.getState() - const resumedTab = state.tabsByWorktree['wt-1']?.find( - (tab) => tab.id !== 'tab-1' && tab.id !== 'tab-2' - ) - expect(launched).toBe(1) - expect(resumedTab?.launchAgent).toBe('claude') - expect(state.pendingStartupByTabId[resumedTab!.id]?.showSessionRestoredBanner).toBe(true) - expect(state.sleepingAgentSessionsByPaneKey[record.paneKey]).toBeUndefined() + expect(launched).toBe(0) + expect(state.tabsByWorktree['wt-1']).toHaveLength(2) + expect(state.sleepingAgentSessionsByPaneKey[record.paneKey]).toBe(record) }) it('rechecks pane ownership after an earlier fresh resume activates a new terminal', () => { diff --git a/src/renderer/src/lib/sleeping-agent-pane-ownership.ts b/src/renderer/src/lib/sleeping-agent-pane-ownership.ts index a1e0f7b74..868ec9582 100644 --- a/src/renderer/src/lib/sleeping-agent-pane-ownership.ts +++ b/src/renderer/src/lib/sleeping-agent-pane-ownership.ts @@ -6,6 +6,7 @@ import type { TerminalTab } from '../../../shared/types' import { parseLegacyNumericPaneKey, parsePaneKey } from '../../../shared/stable-pane-id' +import { isWebTerminalSurfaceTabId } from '../../../shared/terminal-surface-id' type AppStoreState = ReturnType @@ -110,19 +111,13 @@ function paneWillConnectOnActivation( if (state.activeWorktreeId !== worktreeId) { return false } - if (state.activeTabType === 'terminal' && state.activeTabId === tabId) { - return true - } - // Why: split groups can show multiple terminal tabs at once; each group's - // active terminal mounts and connects even when another group has focus. - const groups = state.groupsByWorktree[worktreeId] ?? [] - const unifiedTabs = state.unifiedTabsByWorktree[worktreeId] ?? [] - return groups.some((group) => { - const tab = group.activeTabId - ? unifiedTabs.find((candidate) => candidate.id === group.activeTabId) - : null - return tab?.contentType === 'terminal' && tab.entityId === tabId - }) + // Why: keep-alive mounts every terminal tab of the active worktree and pane + // connect is not visibility-gated (cold-activation deferral delays a mount, + // never cancels it), so any preserved restorable pane cold-restores in place. + // Gating on the visible tab forked a second live surface onto the same + // provider session for every non-group-active agent tab. Web-mirror tabs are + // the exception: they never mount a local pane, so they cannot own recovery. + return !isWebTerminalSurfaceTabId(tabId) } export function recordPaneIsOwnedByPreservedPane( diff --git a/src/renderer/src/lib/worktree-reactivation-preserved-pane-replacement.test.ts b/src/renderer/src/lib/worktree-reactivation-preserved-pane-replacement.test.ts new file mode 100644 index 000000000..7b2d8bda8 --- /dev/null +++ b/src/renderer/src/lib/worktree-reactivation-preserved-pane-replacement.test.ts @@ -0,0 +1,321 @@ +import path from 'node:path' +import { afterEach, describe, expect, it, vi } from 'vitest' +import { useAppStore, type AppState } from '@/store' +import { activateAndRevealWorktree } from './worktree-activation' +import { makeCreatedAgentWorktree as makeWorktree } from '@/lib/worktree-activation-created-agent-test-state' +import { makePaneKey } from '../../../shared/stable-pane-id' + +// Pins the activation contract behind the run6-review-pr-11959 incident shape: +// a persisted (husk) tab whose pane cannot resume in place gets ONE appended +// replacement tab per provider session — the husk is retained for scrollback — +// while a pane that is live or will cold-restore in place gets NO replacement. + +const initialAppStoreState = useAppStore.getState() + +const LEAF_ID = '22222222-2222-4222-8222-222222222222' +const HUSK_TAB_ID = 'husk-tab-1' + +function baseState(worktree: ReturnType): Partial { + return { + repos: [ + { + id: 'repo-1', + path: path.join(path.sep, 'workspace', 'repo'), + displayName: 'repo', + badgeColor: '#000000', + addedAt: 0 + } + ], + worktreesByRepo: { 'repo-1': [worktree] }, + activeRepoId: 'repo-1', + activeView: 'terminal', + tabsByWorktree: {}, + unifiedTabsByWorktree: {}, + groupsByWorktree: {}, + layoutByWorktree: {}, + activeGroupIdByWorktree: {}, + openFiles: [], + browserTabsByWorktree: {}, + activeFileIdByWorktree: {}, + activeBrowserTabIdByWorktree: {}, + activeTabTypeByWorktree: {}, + activeTabIdByWorktree: {}, + tabBarOrderByWorktree: {}, + pendingStartupByTabId: {}, + automaticAgentResumeClaimsByTabId: {}, + agentStatusByPaneKey: {}, + sleepingAgentSessionsByPaneKey: {}, + ptyIdsByTabId: {}, + terminalLayoutsByTabId: {}, + settings: { + agentCmdOverrides: {}, + setupScriptLaunchMode: 'new-tab' + } as unknown as ReturnType['settings'], + markWorktreeVisited: vi.fn(), + recordWorktreeVisit: vi.fn(), + refreshGitHubForWorktreeIfStale: vi.fn(), + revealWorktreeInSidebar: vi.fn() + } +} + +function seedHuskTab( + state: Partial, + worktreeId: string, + ptyBinding: string | null +): void { + state.tabsByWorktree = { + [worktreeId]: [{ id: HUSK_TAB_ID, title: 'Codex', ptyId: null } as never] + } + state.unifiedTabsByWorktree = { + [worktreeId]: [ + { + id: `unified-${HUSK_TAB_ID}`, + contentType: 'terminal', + entityId: HUSK_TAB_ID, + groupId: 'group-1' + } as never + ] + } + state.groupsByWorktree = { + [worktreeId]: [ + { + id: 'group-1', + activeTabId: `unified-${HUSK_TAB_ID}`, + tabOrder: [`unified-${HUSK_TAB_ID}`], + recentTabIds: [] + } as never + ] + } + state.activeGroupIdByWorktree = { [worktreeId]: 'group-1' } + state.terminalLayoutsByTabId = { + [HUSK_TAB_ID]: { + root: { type: 'leaf', leafId: LEAF_ID }, + activeLeafId: LEAF_ID, + ...(ptyBinding ? { ptyIdsByLeafId: { [LEAF_ID]: ptyBinding } } : {}) + } as never + } +} + +function seedSleepingRecord(worktreeId: string, sessionId: string): void { + const paneKey = makePaneKey(HUSK_TAB_ID, LEAF_ID) + useAppStore.setState((s) => ({ + sleepingAgentSessionsByPaneKey: { + ...s.sleepingAgentSessionsByPaneKey, + [paneKey]: { + paneKey, + tabId: HUSK_TAB_ID, + worktreeId, + agent: 'codex' as const, + providerSession: { key: 'session_id' as const, id: sessionId }, + prompt: 'resume prior task', + state: 'working' as const, + origin: 'quit' as const, + capturedAt: 1000, + updatedAt: 1000, + terminalTitle: 'Codex' + } + } + })) +} + +afterEach(() => { + useAppStore.setState(initialAppStoreState, true) +}) + +describe('preserved-pane replacement contract on workspace activation', () => { + it('appends exactly one replacement tab for a husk pane and retains the husk across repeats', () => { + const worktree = { ...makeWorktree(), createdWithAgent: undefined } + const state = baseState(worktree) + // Hibernation cleared the pane's PTY binding: the husk cannot resume in place. + seedHuskTab(state, worktree.id, null) + useAppStore.setState(state) + seedSleepingRecord(worktree.id, 'codex-session-A') + + activateAndRevealWorktree(worktree.id) + + const afterFirst = useAppStore.getState() + const tabsAfterFirst = afterFirst.tabsByWorktree[worktree.id] ?? [] + expect(tabsAfterFirst.map((tab) => tab.id)).toContain(HUSK_TAB_ID) + expect(tabsAfterFirst).toHaveLength(2) + const replacement = tabsAfterFirst.find((tab) => tab.id !== HUSK_TAB_ID)! + expect(afterFirst.automaticAgentResumeClaimsByTabId[replacement.id]?.providerSession).toEqual({ + key: 'session_id', + id: 'codex-session-A' + }) + expect(afterFirst.consumeTabStartupCommand(replacement.id)?.resumeProviderSession).toEqual({ + key: 'session_id', + id: 'codex-session-A' + }) + + // Reopening must not fork more tabs or launch another resume. + activateAndRevealWorktree(worktree.id) + activateAndRevealWorktree(worktree.id) + const afterRepeats = useAppStore.getState() + expect(afterRepeats.tabsByWorktree[worktree.id]).toHaveLength(2) + }) + + it('does not append a replacement while the preserved pane still has a live PTY', () => { + const worktree = { ...makeWorktree(), createdWithAgent: undefined } + const state = baseState(worktree) + seedHuskTab(state, worktree.id, 'pty-live-1') + state.ptyIdsByTabId = { [HUSK_TAB_ID]: ['pty-live-1'] } + useAppStore.setState(state) + seedSleepingRecord(worktree.id, 'codex-session-B') + + activateAndRevealWorktree(worktree.id) + + const after = useAppStore.getState() + expect(after.tabsByWorktree[worktree.id]).toHaveLength(1) + // The record stays with its live pane instead of forking a duplicate. + expect(after.sleepingAgentSessionsByPaneKey[makePaneKey(HUSK_TAB_ID, LEAF_ID)]).toBeDefined() + }) + + it('does not fork a NON-group-active restorable pane into a replacement tab', () => { + const worktree = { ...makeWorktree(), createdWithAgent: undefined } + const state = baseState(worktree) + seedHuskTab(state, worktree.id, 'pty-old-1') + // A second tab holds the group-active slot; the husk is hidden but will + // still mount keep-alive and cold-restore in place on activation. + state.tabsByWorktree = { + [worktree.id]: [ + { id: HUSK_TAB_ID, title: 'Codex', ptyId: null } as never, + { id: 'other-tab-1', title: 'shell', ptyId: null } as never + ] + } + state.unifiedTabsByWorktree = { + [worktree.id]: [ + { + id: `unified-${HUSK_TAB_ID}`, + contentType: 'terminal', + entityId: HUSK_TAB_ID, + groupId: 'group-1' + } as never, + { + id: 'unified-other-tab-1', + contentType: 'terminal', + entityId: 'other-tab-1', + groupId: 'group-1' + } as never + ] + } + state.groupsByWorktree = { + [worktree.id]: [ + { + id: 'group-1', + activeTabId: 'unified-other-tab-1', + tabOrder: [`unified-${HUSK_TAB_ID}`, 'unified-other-tab-1'], + recentTabIds: [] + } as never + ] + } + state.activeTabIdByWorktree = { [worktree.id]: 'other-tab-1' } + state.activeTabTypeByWorktree = { [worktree.id]: 'terminal' } + useAppStore.setState(state) + seedSleepingRecord(worktree.id, 'codex-session-D') + + activateAndRevealWorktree(worktree.id) + activateAndRevealWorktree(worktree.id) + + const after = useAppStore.getState() + expect(after.tabsByWorktree[worktree.id]?.map((tab) => tab.id)).toEqual([ + HUSK_TAB_ID, + 'other-tab-1' + ]) + // The record stays with the pane that will cold-restore in place — + // consuming it here would resume the session twice (pane + replacement). + expect(after.sleepingAgentSessionsByPaneKey[makePaneKey(HUSK_TAB_ID, LEAF_ID)]).toBeDefined() + }) + + it('does not append a replacement when the preserved pane will cold-restore in place', () => { + const worktree = { ...makeWorktree(), createdWithAgent: undefined } + const state = baseState(worktree) + // Restorable binding persists, no live PTY: pane-level cold restore owns recovery. + seedHuskTab(state, worktree.id, 'pty-old-1') + state.activeTabIdByWorktree = { [worktree.id]: HUSK_TAB_ID } + state.activeTabTypeByWorktree = { [worktree.id]: 'terminal' } + useAppStore.setState(state) + seedSleepingRecord(worktree.id, 'codex-session-C') + + activateAndRevealWorktree(worktree.id) + + const after = useAppStore.getState() + expect(after.tabsByWorktree[worktree.id]).toHaveLength(1) + expect(after.sleepingAgentSessionsByPaneKey[makePaneKey(HUSK_TAB_ID, LEAF_ID)]).toBeDefined() + }) + + // Why: web-mirror tabs never mount a local pane, so they cannot own recovery + // — the appended replacement stays the correct resume path for them. + it('still appends a replacement for a web-mirror tab that cannot mount a local pane', () => { + const webTabId = 'web-terminal-host-tab' + const worktree = { ...makeWorktree(), createdWithAgent: undefined } + const state = baseState(worktree) + state.tabsByWorktree = { + [worktree.id]: [{ id: webTabId, title: 'Codex', ptyId: null } as never] + } + state.unifiedTabsByWorktree = { + [worktree.id]: [ + { + id: `unified-${webTabId}`, + contentType: 'terminal', + entityId: webTabId, + groupId: 'group-1' + } as never + ] + } + state.groupsByWorktree = { + [worktree.id]: [ + { + id: 'group-1', + activeTabId: `unified-${webTabId}`, + tabOrder: [`unified-${webTabId}`], + recentTabIds: [] + } as never + ] + } + state.activeGroupIdByWorktree = { [worktree.id]: 'group-1' } + state.terminalLayoutsByTabId = { + [webTabId]: { + root: { type: 'leaf', leafId: LEAF_ID }, + activeLeafId: LEAF_ID, + ptyIdsByLeafId: { [LEAF_ID]: 'pty-old-1' } + } as never + } + useAppStore.setState(state) + const paneKey = makePaneKey(webTabId, LEAF_ID) + useAppStore.setState((s) => ({ + sleepingAgentSessionsByPaneKey: { + ...s.sleepingAgentSessionsByPaneKey, + [paneKey]: { + paneKey, + tabId: webTabId, + worktreeId: worktree.id, + agent: 'codex' as const, + providerSession: { key: 'session_id' as const, id: 'codex-session-E' }, + prompt: 'resume prior task', + state: 'working' as const, + origin: 'quit' as const, + capturedAt: 1000, + updatedAt: 1000, + terminalTitle: 'Codex' + } + } + })) + + activateAndRevealWorktree(worktree.id) + + const after = useAppStore.getState() + const tabs = after.tabsByWorktree[worktree.id] ?? [] + expect(tabs.map((tab) => tab.id)).toContain(webTabId) + expect(tabs).toHaveLength(2) + expect(after.sleepingAgentSessionsByPaneKey[paneKey]).toBeUndefined() + const replacement = tabs.find((tab) => tab.id !== webTabId)! + expect(after.automaticAgentResumeClaimsByTabId[replacement.id]?.providerSession).toEqual({ + key: 'session_id', + id: 'codex-session-E' + }) + expect(after.consumeTabStartupCommand(replacement.id)?.resumeProviderSession).toEqual({ + key: 'session_id', + id: 'codex-session-E' + }) + }) +})