diff --git a/src/main/runtime/structured-session-worktree-teardown.test.ts b/src/main/runtime/structured-session-worktree-teardown.test.ts index 84977aad4eb..5869fb4e28e 100644 --- a/src/main/runtime/structured-session-worktree-teardown.test.ts +++ b/src/main/runtime/structured-session-worktree-teardown.test.ts @@ -13,8 +13,7 @@ const { isProvenLiveStructuredSessionRemovalError, isUnstoppedPtyRemovalError } = await import('../../shared/worktree/removal') -const { listLiveStructuredSessionsForWorktree } = - await import('./structured-session-worktree-teardown') +const { listStructuredSessionsForWorktree } = await import('./structured-session-worktree-teardown') const WORKTREE = 'repo_1::/tmp/wt-a' const OTHER_WORKTREE = 'repo_1::/tmp/wt-b' @@ -58,6 +57,13 @@ function installHost(options: { closeGates?: Record> /** Sessions in the persisted visible-tab index, so a rollback has something to put back. */ visible?: string[] + /** + * Sessions this host is not holding, so they observe `unverifiable` rather than `live`. + * + * The everyday shape, not an edge case: the provider child belongs to the VISIBLE pane, so any + * chat outside the active workspace has already been evicted by the release clock. + */ + detached?: Set /** * Sessions whose death evidence lands DURING the close's tab-restore write. * @@ -66,7 +72,11 @@ function installHost(options: { */ exitsDuringTabRestore?: Set }): { closed: string[]; visible: Set } { - const held = new Set(options.records.map((entry) => entry.sessionId)) + const held = new Set( + options.records + .map((entry) => entry.sessionId) + .filter((sessionId) => !options.detached?.has(sessionId)) + ) const closed: string[] = [] const visible = new Set(options.visible ?? []) const recordExit = (sessionId: string): void => { @@ -148,9 +158,21 @@ describe('worktree teardown and structured agent sessions', () => { it('finds sessions by workspace, and ignores a sibling worktree', () => { installHost({ records: [record('s1', WORKTREE), record('s2', OTHER_WORKTREE)] }) - expect(listLiveStructuredSessionsForWorktree(WORKTREE, {})).toEqual([ - { sessionId: 's1', agent: 'claude' } - ]) + expect(listStructuredSessionsForWorktree(WORKTREE, {})).toEqual({ + members: [{ sessionId: 's1', agent: 'claude' }], + live: [{ sessionId: 's1', agent: 'claude' }] + }) + }) + + it('counts a chat with no attached child as a member but never as live', () => { + // The split this file's two lists exist for. A provider child is scoped to a VISIBLE pane, so + // every chat in a workspace the user is not currently looking at observes non-live — and a + // liveness-only list therefore saw nothing at all to act on for the commonest delete there is. + installHost({ records: [record('s1', WORKTREE)], detached: new Set(['s1']) }) + expect(listStructuredSessionsForWorktree(WORKTREE, {})).toEqual({ + members: [{ sessionId: 's1', agent: 'claude' }], + live: [] + }) }) it('closes a live session on an ordinary removal instead of refusing it', async () => { @@ -365,11 +387,14 @@ describe('worktree teardown and structured agent sessions', () => { installHost({ records: [record('s1', WORKTREE, { executionHostId: 'ssh:host-a' }), record('s2', WORKTREE)] }) - const local = [{ sessionId: 's2', agent: 'claude' }] - expect(listLiveStructuredSessionsForWorktree(WORKTREE, { resolvedConnectionId: null })).toEqual( + const local = { + members: [{ sessionId: 's2', agent: 'claude' }], + live: [{ sessionId: 's2', agent: 'claude' }] + } + expect(listStructuredSessionsForWorktree(WORKTREE, { resolvedConnectionId: null })).toEqual( local ) - expect(listLiveStructuredSessionsForWorktree(WORKTREE, {})).toEqual(local) + expect(listStructuredSessionsForWorktree(WORKTREE, {})).toEqual(local) }) it('closes only the session on the host the removal resolved to', async () => { @@ -599,6 +624,153 @@ describe('worktree teardown and structured agent sessions', () => { await expect(removal).resolves.toMatchObject({ structuredStopped: 1 }) }) + it('retires the chat tab of a chat that had no child to close', async () => { + // The orphan. Deleting a workspace from the sidebar while a different one is active leaves + // every chat in the target non-live — the provider child belongs to the VISIBLE pane — so the + // close list was empty and the sweep returned early. The durable `visibleSessionIds` reference + // survived both purges a removal already performs, and startup replayed it: the chat tab came + // back at the next launch pointing at a workspace that no longer exists. + const host = installHost({ + records: [record('s1', WORKTREE)], + detached: new Set(['s1']), + visible: ['s1'] + }) + await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).resolves.toMatchObject({ + runtimeStopped: 0 + }) + expect(host.closed).toEqual([]) + expect([...host.visible]).toEqual([]) + }) + + it('retires it from the live tab snapshot as well as the durable index', async () => { + // `setSessionTabVisibility(false)` only clears the restore index; the chat tab published on + // screen survives it for the rest of the app session and re-attaches the session when opened. + const retired: string[] = [] + const runtime = { + stopTerminalsForWorktree: async () => ({ stopped: 0 }), + retireStructuredAgentSessionTabFromSnapshot: (sessionId: string) => { + retired.push(sessionId) + return true + } + } as never + installHost({ + records: [record('s1', WORKTREE)], + detached: new Set(['s1']), + visible: ['s1'] + }) + await killAllProcessesForWorktree(WORKTREE, { ...destructiveDeps(), runtime }) + expect(retired).toEqual(['s1']) + }) + + it('keeps every chat tab when the removal refuses over a session it could not close', async () => { + // The load-bearing interaction. A refusal leaves the workspace — and its chat tabs — exactly + // where they were, so retirement must not have run: a destructive operation that refused and + // still took the user's chats away is the very harm this sweep's rollback exists to prevent. + // Both members are covered, the stuck one and the detached bystander beside it. + const host = installHost({ + records: [record('s1', WORKTREE), record('s2', WORKTREE)], + stuck: new Set(['s1']), + detached: new Set(['s2']), + visible: ['s1', 's2'] + }) + await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).rejects.toThrow( + /still live: 1 agent session \(claude\)/ + ) + expect([...host.visible].sort()).toEqual(['s1', 's2']) + }) + + it('keeps every chat tab when the unstopped-PTY gate refuses the removal', async () => { + // The reason retirement is NOT done inside the structured sweep. That sweep is joined BEFORE + // the per-PTY verdict so a structured refusal can outrank a terminal one — which means a tab + // retired at the end of it would still be ahead of a gate that can refuse the whole removal, + // and this workspace survives with its chats gone. + const runtime = { + stopTerminalsForWorktree: async ( + _worktreeId: string, + options: { stopPty: (ptyId: string, stop: () => Promise) => Promise } + ) => { + await options.stopPty('pty-1', async () => false) + return { stopped: 0 } + } + } as never + const livePtyProvider = { + listProcesses: async () => [{ id: 'pty-1' }], + shutdown: async () => {} + } as never + const host = installHost({ + records: [record('s1', WORKTREE)], + detached: new Set(['s1']), + visible: ['s1'] + }) + await expect( + killAllProcessesForWorktree(WORKTREE, { + localProvider: livePtyProvider, + requirePhysicalStop: true, + includeProviderInventory: false, + includeLocalRegistry: false, + runtime + }) + ).rejects.toThrow(/still live: pty-1/) + expect([...host.visible]).toEqual(['s1']) + }) + + it('retires the chat tab under force, which deletes the workspace anyway', async () => { + const host = installHost({ + records: [record('s1', WORKTREE)], + detached: new Set(['s1']), + visible: ['s1'] + }) + await killAllProcessesForWorktree(WORKTREE, destructiveDeps({ allowUnverifiedStop: true })) + expect([...host.visible]).toEqual([]) + }) + + it('retires the chat tab for a folder-workspace removal too', async () => { + // No checkout vanishes there, but the workspace metadata does, so a republished tab at the + // next launch points at a workspace Orca has forgotten. That path already drops the tab for + // the sessions it DID close, so leaving the detached ones is the inconsistency being fixed. + const host = installHost({ + records: [record('s1', WORKTREE)], + detached: new Set(['s1']), + visible: ['s1'] + }) + await killAllProcessesForWorktree(WORKTREE, { + localProvider, + includeProviderInventory: false as const, + includeLocalRegistry: false as const, + closeStructuredSessions: true + }) + expect([...host.visible]).toEqual([]) + }) + + it('retires nothing on a reconciliation sweep, which deletes no workspace', async () => { + // Those callers repair state. They close no session, so they must retire no tab either — + // the workspace and its checkout are both still there. + const host = installHost({ + records: [record('s1', WORKTREE)], + detached: new Set(['s1']), + visible: ['s1'] + }) + await killAllProcessesForWorktree(WORKTREE, { + localProvider, + includeProviderInventory: false, + includeLocalRegistry: false + }) + expect([...host.visible]).toEqual(['s1']) + }) + + it('leaves a same-id workspace on another host holding its chat tab', async () => { + // The membership filter is fenced for the same reason the close list is: `repoId::path` names + // a different workspace on every host, and retiring a tab for one host's workspace takes a + // chat tab from another's. + const host = installHost({ + records: [record('s1', WORKTREE, { executionHostId: 'ssh:host-a' })], + detached: new Set(['s1']), + visible: ['s1'] + }) + await killAllProcessesForWorktree(WORKTREE, destructiveDeps()) + expect([...host.visible]).toEqual(['s1']) + }) + it('does not block removal when no structured host is installed', async () => { // Not being able to look is not evidence a child is there, and reading the persisted store // directly would force-install the host as a side effect of a teardown. diff --git a/src/main/runtime/structured-session-worktree-teardown.ts b/src/main/runtime/structured-session-worktree-teardown.ts index 8b161e4f5a8..46265a16111 100644 --- a/src/main/runtime/structured-session-worktree-teardown.ts +++ b/src/main/runtime/structured-session-worktree-teardown.ts @@ -33,12 +33,12 @@ import { retireSettledStructuredWorkerTab } from './structured-agent-session-tab import type { WorktreePtyHostFence } from './worktree-pty-host-fence' import type { OrcaRuntimeService } from './orca-runtime' -export type LiveStructuredSessionInWorkspace = { +export type StructuredSessionInWorkspace = { sessionId: string agent: 'claude' | 'codex' } -export type UnclosedStructuredSession = LiveStructuredSessionInWorkspace & { +export type UnclosedStructuredSession = StructuredSessionInWorkspace & { /** Read AFTER the close: `live` is a child watched stay attached, not merely one left unproven. */ status: 'live' | 'unverifiable' } @@ -83,36 +83,61 @@ export function structuredSessionTeardownHostId( } /** - * Structured sessions with a proven-live child in this worktree, on the fenced host only. + * The workspace's structured sessions, split into what belongs to it and what is attached. + * + * MEMBERSHIP and LIVENESS answer different questions, and folding them into one list is what let + * a chat tab outlive its workspace. A provider child is scoped to a VISIBLE pane — the hold that + * keeps one is `enabled: isVisible && isWorktreeActive`, and dropping the last hold evicts the + * child after the release grace — so `live` really means "this chat is the visible pane in the + * active workspace, or was moments ago". Deleting a workspace from the sidebar while a different + * one is active makes every chat in the target non-live. Those are exactly the sessions a + * liveness-only list never saw. + */ +export type StructuredSessionsForWorktree = { + /** Every session bound to this workspace on the fenced host, attached or not. */ + members: StructuredSessionInWorkspace[] + /** The subset with a proven-live provider child: what the sweep closes and may refuse over. */ + live: StructuredSessionInWorkspace[] +} + +/** + * Structured sessions in this worktree, on the fenced host only. * * An uninstalled host answers empty rather than throwing: no host in this generation means no * provider child was started by this process, and the three PTY sweeps fall through the same way * when their surface is unavailable. It is deliberately NOT read through the persisted store * directly — that would force-install the host, which is itself a side effect on a teardown path. + * + * One enumeration and one observation per member, because both answers are read by the same + * caller: re-deriving the live subset separately would run every liveness observation twice. */ -export function listLiveStructuredSessionsForWorktree( +export function listStructuredSessionsForWorktree( worktreeId: string, fence: StructuredSessionHostFence -): LiveStructuredSessionInWorkspace[] { +): StructuredSessionsForWorktree { const host = getStructuredAgentSessionHost() if (!host) { - return [] + return { members: [], live: [] } } let records: ReturnType try { records = host.deps.store.listRecords() } catch { - return [] + return { members: [], live: [] } } const hostId = structuredSessionTeardownHostId(fence) - return records + const members = records .filter( (record) => - record.location.workspaceId === worktreeId && - record.location.executionHostId === hostId && - observeStructuredWorker({ sessionId: record.sessionId }).status === 'live' + record.location.workspaceId === worktreeId && record.location.executionHostId === hostId ) .map((record) => ({ sessionId: record.sessionId, agent: record.provider })) + return { + members, + live: members.filter( + (session) => observeStructuredWorker({ sessionId: session.sessionId }).status === 'live' + ) + } } /** @@ -166,7 +191,7 @@ export function describeUnclosedStructuredSessions( */ export type StructuredSweepProgress = { /** The sessions this sweep closes, in the order the loop reaches them. */ - readonly sessions: readonly LiveStructuredSessionInWorkspace[] + readonly sessions: readonly StructuredSessionInWorkspace[] /** Sessions no longer attached after their close — the count this sweep reports. */ closed: number /** Attempted closes that did not settle, each carrying the verdict re-read after the attempt. */ @@ -176,7 +201,7 @@ export type StructuredSweepProgress = { } export function createStructuredSweepProgress( - sessions: readonly LiveStructuredSessionInWorkspace[] + sessions: readonly StructuredSessionInWorkspace[] ): StructuredSweepProgress { return { sessions, closed: 0, unstopped: [], settled: 0 } } @@ -276,6 +301,35 @@ export async function closeStructuredSessionsForWorktree( } } +/** + * Retires the chat tabs of every structured session in a workspace this removal is discarding. + * + * The close path already does this for a session it CLOSED. This is the complement: the members + * with no attached child, which the close list never contained and nothing else will hide. Their + * durable reference survives every purge a removal already performs: the renderer drops `unifiedTabsByWorktree` and the main + * process drops the workspace metadata, and neither touches `visibleSessionIds`. Startup replays + * that index, restores the session from it and republishes the tab, so the chat comes back at the + * next launch pointing at a workspace that is gone. Worktree ids are path-derived and can be + * recreated, so a later workspace at the same path inherits the tab — which is the hazard + * `removeWorktreeMetadataAndHistory` purges everything else to prevent. + * + * The caller owns WHEN: this must only be reached once the removal can no longer refuse, because + * a refusal leaves the workspace and its tabs in place. Same reasoning as the close's own + * `restoreTabOnUnprovenClose` gate, one level up. + * + * Serial, and structurally unable to fail the teardown: each step is a store transaction that + * serializes per session anyway, and a removal must not be turned back by tab bookkeeping. + */ +export async function retireStructuredSessionTabsForWorktree( + sessions: readonly StructuredSessionInWorkspace[], + runtime?: StructuredWorktreeSweepRuntime +): Promise { + for (const session of sessions) { + await dropDurableChatTabReference(session.sessionId) + retireSettledStructuredWorkerTab(session.sessionId, runtime) + } +} + /** * Drops a settled session's durable chat-tab reference, and cannot fail the settlement. * diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 84275b40eed..eeafb0ea6f3 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -17,8 +17,10 @@ import { closeStructuredSessionsForWorktree, createStructuredSweepProgress, describeUnclosedStructuredSessions, - listLiveStructuredSessionsForWorktree, - unclosedStructuredSessions + listStructuredSessionsForWorktree, + retireStructuredSessionTabsForWorktree, + unclosedStructuredSessions, + type StructuredSessionInWorkspace } from './structured-session-worktree-teardown' import { createWorktreeSweepTracker, @@ -208,7 +210,8 @@ export async function killAllProcessesForWorktree( for (const sweep of [runtimeSweep, providerSweep, registrySweep]) { void sweep.catch(() => undefined) } - const structuredStopped = await structuredSweep + const structured = await structuredSweep + const structuredStopped = structured.closed let runtimeResult: { stopped: number } let providerStopped: number let registryStopped: number @@ -222,7 +225,9 @@ export async function killAllProcessesForWorktree( if (forced.incomplete) { // Carries the structured count out too: this early return skips the PTY verdict, not the // sweep that already closed a user's chats, and dropping it makes the log say `structured=0` - // for a removal that closed some. + // for a removal that closed some. Force deletes whatever the sweeps reported, so the chat + // tabs go with the workspace here as well. + await retireStructuredSessionTabsForWorktree(structured.retirable, deps.runtime) return { ...forced.stopped, ...(structuredStopped > 0 ? { structuredStopped } : {}) } } runtimeResult = { stopped: forced.stopped.runtimeStopped } @@ -284,6 +289,11 @@ export async function killAllProcessesForWorktree( } } + // Past every refusal, and only here. A removal that refuses — over an unclosed session above, a + // sweep that failed outright, or the unstopped-PTY gate just now — leaves the workspace and its + // chat tabs exactly where they were, so retiring a tab before this point would take the user's + // chat away on a delete that never happened. + await retireStructuredSessionTabsForWorktree(structured.retirable, deps.runtime) return { runtimeStopped: runtimeResult.stopped, providerStopped, @@ -292,6 +302,23 @@ export async function killAllProcessesForWorktree( } } +type StructuredSweepOutcome = { + /** Sessions this sweep proved closed — the count the removal log and result report. */ + closed: number + /** + * The workspace's sessions the close loop never had anything to close: no attached child, so + * nothing hid their chat tab and nothing will. Exactly the complement of the close list, so each + * session's tab is handled once, by one mechanism. + * + * Carried out to the caller rather than acted on here, because this sweep is joined BEFORE the + * unstopped-PTY verdict: retiring a tab at the end of this function would still be ahead of a + * gate that can refuse the whole removal. + */ + retirable: readonly StructuredSessionInWorkspace[] +} + +const NO_STRUCTURED_SWEEP: StructuredSweepOutcome = { closed: 0, retirable: [] } + /** * The fourth sweep: structured agent sessions bound to this worktree, on this host. * @@ -313,16 +340,22 @@ async function sweepStructuredSessions( deps: WorktreeTeardownDeps, deadline: number, sweeps: WorktreeSweepTracker -): Promise { +): Promise { if (!deps.requirePhysicalStop && !deps.closeStructuredSessions) { - return 0 + return NO_STRUCTURED_SWEEP } // `deps` carries the same two host fields the PTY sweeps fence on, and a `repoId::path` id names // a different workspace on every host — so an unfenced list would close a live chat belonging to - // an SSH or paired-runtime copy of the id being removed here. - const live = listLiveStructuredSessionsForWorktree(worktreeId, deps) + // an SSH or paired-runtime copy of the id being removed here. The same fence carries the + // membership half: a tab retired for one host's workspace is a tab taken from another's. + const { members, live } = listStructuredSessionsForWorktree(worktreeId, deps) + const liveIds = new Set(live.map((session) => session.sessionId)) + // A chat with no attached child is precisely the one this removal used to leave a durable tab + // reference for, and it is invisible to every list below — so it comes out even when the close + // loop below is skipped entirely, which is the common case for a delete from the sidebar. + const retirable = members.filter((session) => !liveIds.has(session.sessionId)) if (live.length === 0) { - return 0 + return { closed: 0, retirable } } // Raced against the same sweep budget every PTY surface is bounded by, because `host.close` // awaits a provider round trip whose own eviction steps are each bounded well past this budget. @@ -352,7 +385,7 @@ async function sweepStructuredSessions( const closed = progress.closed const unstopped = unclosedStructuredSessions(progress) if (unstopped.length === 0) { - return closed + return { closed, retirable } } // Only a proof-requiring removal may refuse. A folder-workspace removal shares its root, so no // checkout disappears under the child — the harm is a session left pointing at a workspace Orca @@ -372,5 +405,5 @@ async function sweepStructuredSessions( console.warn( `[worktree-teardown] forcing removal of ${worktreeId}${UNSTOPPED_PTY_DETAIL_SEPARATOR}${describeUnclosedStructuredSessions(unstopped)}` ) - return closed + return { closed, retirable } }