From e192b623c47c786cd9a050b7022d98fdd98c7a79 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Thu, 10 Sep 2026 19:27:18 -0700 Subject: [PATCH] fix(worktrees): retire the chat tab of a chat with no child when its workspace goes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deleting a workspace left a chat tab behind for every structured session that had no attached provider child at the time, and that tab came back at the next launch pointing at a workspace that no longer exists. A provider child is scoped to a VISIBLE pane, not to a tab: the hold that keeps one is `enabled: isVisible && isWorktreeActive`, and dropping the last hold evicts the child after the release grace. So the sweep's liveness predicate selected only "the chat that is the visible pane in the active workspace, or was moments ago" — which means deleting a workspace from the sidebar while a different one is active left every chat in the target invisible to the sweep, and the `live.length === 0` early return did nothing at all. The durable reference is `visibleSessionIds` in the agent-session record store. Both purges a removal already performs miss it: the renderer drops `unifiedTabsByWorktree` and the main process drops the workspace metadata, and neither touches that index. Startup replays it, restores the session from it and republishes the tab. Worktree ids are path-derived, so a later workspace created at the same path inherits the old chat. Splits the two concerns the sweep conflated in one list. Liveness still decides what to CLOSE and what to refuse over, unchanged. Membership — the same fenced record filter minus the liveness clause — decides what to RETIRE, and covers exactly the complement of the close list so each session's tab is handled once. Retirement runs from `killAllProcessesForWorktree`, past every point that can refuse, not from the structured sweep itself: that sweep is joined BEFORE the unstopped-PTY verdict so a structured refusal can outrank a terminal one, and a tab retired there would still be ahead of a gate that can refuse the whole removal — leaving the workspace in place with its chats gone. --- ...ructured-session-worktree-teardown.test.ts | 190 +++++++++++++++++- .../structured-session-worktree-teardown.ts | 80 ++++++-- src/main/runtime/worktree-teardown.ts | 55 ++++- 3 files changed, 292 insertions(+), 33 deletions(-) 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 } }