diff --git a/src/main/runtime/structured-agent-session-close.test.ts b/src/main/runtime/structured-agent-session-close.test.ts index 187dd79c12c..531ca0aa129 100644 --- a/src/main/runtime/structured-agent-session-close.test.ts +++ b/src/main/runtime/structured-agent-session-close.test.ts @@ -163,6 +163,21 @@ describe('closeStructuredAgentSessionChild tab-visibility rollback', () => { expect(host.setSessionTabVisibility.mock.calls).toEqual([[SESSION, false]]) }) + it('does not put the tab back when the caller is discarding the workspace anyway', async () => { + // Worktree teardown passes this off for a removal that cannot refuse — force, and the + // folder-workspace paths. A tab put back there is a durable reference to a workspace that is + // about to be gone, so it republishes the chat at the next launch pointing at it. + const host = installHost({ stuck: true }) + + const outcome = await closeStructuredAgentSessionChild(SESSION, { + restoreTabOnUnprovenClose: false + }) + + expect(outcome.stopped).toBe(false) + expect(host.visible.has(SESSION)).toBe(false) + expect(host.setSessionTabVisibility.mock.calls).toEqual([[SESSION, false]]) + }) + it('does not publish a tab for a session that was already hidden', async () => { const host = installHost({ closeThrows: new Error('provider round trip failed'), visible: [] }) diff --git a/src/main/runtime/structured-agent-session-close.ts b/src/main/runtime/structured-agent-session-close.ts index 17157839b55..f65d1204db9 100644 --- a/src/main/runtime/structured-agent-session-close.ts +++ b/src/main/runtime/structured-agent-session-close.ts @@ -36,6 +36,15 @@ export type StructuredAgentSessionCloseOptions = { * keep the child un-evictable for the life of the app. Every settlement has to reach it. */ afterClose?: () => void + /** + * Whether an unproven close may put the chat tab back in the durable restore index. + * + * On by default, which is the retryable case: a stop that refused and still took the user's tab + * away is the loss the rollback exists to undo. A caller that will discard the WORKSPACE + * whatever this close reports passes false — a tab put back there is a durable reference to a + * workspace about to be gone, and it republishes the chat at the next launch pointing at it. + */ + restoreTabOnUnprovenClose?: boolean } export async function closeStructuredAgentSessionChild( @@ -54,7 +63,8 @@ export async function closeStructuredAgentSessionChild( // Read BEFORE the hide, so a rollback puts the tab back exactly as it was. Restoring // unconditionally would publish a tab for a session that was already hidden — a worker started // without a chat tab, or one the user had closed — which is a new side effect, not an undo. - const tabWasVisible = readPersistedTabVisibility(host, sessionId) + const restoreTabIfCloseFails = + options.restoreTabOnUnprovenClose !== false && readPersistedTabVisibility(host, sessionId) // Set only once the close is actually issued: `setSessionTabVisibility` throwing first leaves a // running child, and a receipt that still said `closed_agent_terminal` for it would be the // close-that-never-happened this flag exists to rule out. @@ -67,7 +77,7 @@ export async function closeStructuredAgentSessionChild( // Only `closeAttempted` proves the hide landed: the store transaction restores its own state on // failure, so a `setSessionTabVisibility` that threw hid nothing and has nothing to undo. if (closeAttempted) { - await restorePersistedTabVisibility(host, sessionId, tabWasVisible) + await restorePersistedTabVisibility(host, sessionId, restoreTabIfCloseFails) } return { stopped: false, @@ -78,7 +88,7 @@ export async function closeStructuredAgentSessionChild( options.afterClose?.() const observation = observeStructuredWorker({ sessionId }) if (observation.status !== 'exited') { - await restorePersistedTabVisibility(host, sessionId, tabWasVisible) + await restorePersistedTabVisibility(host, sessionId, restoreTabIfCloseFails) return { stopped: false, closeAttempted: true, @@ -113,6 +123,11 @@ function readPersistedTabVisibility(host: StructuredAgentSessionHost, sessionId: * counting such a session closed and retiring its tab. Republishing there would resurrect a tab for * a session that is demonstrably gone, at the next launch, pointing at a deleted workspace. * + * That observation NARROWS the window; it does not close it. This one and the sweep's are taken a + * store write apart, so a child that dies in between is unverifiable here and exited there — which + * is why the sweep re-drops the tab reference when it takes that proof. Do not delete either half + * on the strength of the other. + * * Never throws: the caller's `reason` is what the user is asked to act on, and a rollback failure * must not replace it. `agent_session_identity_required` is the expected one — the record can be * gone by now, which is itself the exit this restore is declining to undo. @@ -120,9 +135,9 @@ function readPersistedTabVisibility(host: StructuredAgentSessionHost, sessionId: async function restorePersistedTabVisibility( host: StructuredAgentSessionHost, sessionId: string, - tabWasVisible: boolean + restoreTab: boolean ): Promise { - if (!tabWasVisible || observeStructuredWorker({ sessionId }).status === 'exited') { + if (!restoreTab || observeStructuredWorker({ sessionId }).status === 'exited') { return } try { diff --git a/src/main/runtime/structured-session-worktree-teardown.test.ts b/src/main/runtime/structured-session-worktree-teardown.test.ts index 5cdae7dcd4f..84977aad4eb 100644 --- a/src/main/runtime/structured-session-worktree-teardown.test.ts +++ b/src/main/runtime/structured-session-worktree-teardown.test.ts @@ -56,13 +56,40 @@ function installHost(options: { closeGate?: Promise /** Blocks ONE session's close, so the serial loop can be caught part-way through. */ closeGates?: Record> -}): { closed: string[] } { + /** Sessions in the persisted visible-tab index, so a rollback has something to put back. */ + visible?: string[] + /** + * Sessions whose death evidence lands DURING the close's tab-restore write. + * + * `setSessionTabVisibility` is a store transaction — a real disk write — so the close's own + * observation and the sweep's re-read straddle it and can disagree about the same session. + */ + exitsDuringTabRestore?: Set +}): { closed: string[]; visible: Set } { const held = new Set(options.records.map((entry) => entry.sessionId)) const closed: string[] = [] + const visible = new Set(options.visible ?? []) + const recordExit = (sessionId: string): void => { + const entry = options.records.find((candidate) => candidate.sessionId === sessionId) + if (entry) { + entry.lease.claimStatus = 'released' + entry.lease.deathEvidence = { kind: 'exit-observed', detail: 'closed', observedAt: 1 } + } + } hostRef.current = { deps: { store: { listRecords: () => options.records, getRecord: () => null } }, hasSession: (sessionId: string) => held.has(sessionId), - setSessionTabVisibility: async () => {}, + getPersistedVisibleSessionTabIndex: () => ({ present: true, sessionIds: [...visible] }), + setSessionTabVisibility: async (sessionId: string, isVisible: boolean) => { + if (!isVisible) { + visible.delete(sessionId) + return + } + if (options.exitsDuringTabRestore?.has(sessionId)) { + recordExit(sessionId) + } + visible.add(sessionId) + }, close: async (sessionId: string) => { closed.push(sessionId) await options.closeGate @@ -74,10 +101,8 @@ function installHost(options: { if (options.unverifiable?.has(sessionId)) { return } - const record = options.records.find((entry) => entry.sessionId === sessionId) - if (record) { - record.lease.claimStatus = 'released' - record.lease.deathEvidence = { kind: 'exit-observed', detail: 'closed', observedAt: 1 } + if (!options.exitsDuringTabRestore?.has(sessionId)) { + recordExit(sessionId) } if (options.settledThenThrows?.has(sessionId)) { throw new Error('the event sink could not be flushed') @@ -89,7 +114,7 @@ function installHost(options: { hostRef.current as { deps: { store: { getRecord: (id: string) => unknown } } } ).deps.store.getRecord = (sessionId: string) => options.records.find((entry) => entry.sessionId === sessionId) ?? null - return { closed } + return { closed, visible } } const localProvider = { @@ -148,6 +173,71 @@ describe('worktree teardown and structured agent sessions', () => { ) }) + it('puts the chat tab back when the removal refuses over the session', async () => { + // The workspace survives a refusal, so the tab has to survive it too: a destructive operation + // that refused and still took the user's chat tab away is the loss the rollback exists to undo. + const host = installHost({ + records: [record('s1', WORKTREE)], + stuck: new Set(['s1']), + visible: ['s1'] + }) + await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).rejects.toThrow( + /still live: 1 agent session \(claude\)/ + ) + expect([...host.visible]).toEqual(['s1']) + }) + + it('leaves the chat tab dropped when a forced removal deletes the workspace anyway', async () => { + // The other half of the same rollback. Force does not refuse — it warns and goes on to delete + // the checkout — so putting the tab back leaves a DURABLE reference to a workspace that is + // about to be gone, which republishes the chat at the next launch pointing at a deleted + // worktree: the exact outcome this whole sweep exists to remove. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const host = installHost({ + records: [record('s1', WORKTREE)], + stuck: new Set(['s1']), + visible: ['s1'] + }) + await killAllProcessesForWorktree(WORKTREE, destructiveDeps({ allowUnverifiedStop: true })) + expect([...host.visible]).toEqual([]) + warn.mockRestore() + }) + + it('leaves the chat tab dropped for a folder-workspace removal, which never refuses', async () => { + // Same reasoning without the force waiver: this caller cannot refuse at all, so the workspace + // is forgotten whatever the close reports. + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + const host = installHost({ + records: [record('s1', WORKTREE)], + stuck: new Set(['s1']), + visible: ['s1'] + }) + await killAllProcessesForWorktree(WORKTREE, { + localProvider, + includeProviderInventory: false as const, + includeLocalRegistry: false as const, + closeStructuredSessions: true + }) + expect([...host.visible]).toEqual([]) + warn.mockRestore() + }) + + it('drops the chat tab for a session the sweep proves exited after the close gave up', async () => { + // `host.close` can return BEFORE the child's exit is recorded, so the close's own observation + // reads unverifiable and puts the tab back — and the sweep's re-read, one store write later, + // proves the exit and counts the session closed. The two observations straddle that write and + // can disagree; the tab must not survive the disagreement, because this removal proceeds. + const host = installHost({ + records: [record('s1', WORKTREE)], + visible: ['s1'], + exitsDuringTabRestore: new Set(['s1']) + }) + await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).resolves.toMatchObject({ + structuredStopped: 1 + }) + expect([...host.visible]).toEqual([]) + }) + it('names the force escape hatch in the refusal, like the unstopped-PTY gate', async () => { installHost({ records: [record('s1', WORKTREE)], stuck: new Set(['s1']) }) await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).rejects.toThrow(/force/i) diff --git a/src/main/runtime/structured-session-worktree-teardown.ts b/src/main/runtime/structured-session-worktree-teardown.ts index 609ff3ef23f..8b161e4f5a8 100644 --- a/src/main/runtime/structured-session-worktree-teardown.ts +++ b/src/main/runtime/structured-session-worktree-teardown.ts @@ -209,15 +209,28 @@ export function unclosedStructuredSessions( * the outcome this whole sweep exists to prevent, and closing is how you prevent it. What stayed is * the only thing worth refusing over. * - * Takes the list rather than re-deriving it, so the sessions reported as unclosed are exactly the - * ones a close was attempted on — re-enumerating would run every liveness observation twice and - * let the refusal name a session this call never touched. + * Takes the list rather than re-deriving it, so the refusal can only ever name a session out of + * the set this sweep was handed — re-enumerating would run every liveness observation twice and + * let it name one this call never touched. Not every one of them is a session a close was + * attempted on: the deadline check below can leave the tail of the list unasked, and + * `unclosedStructuredSessions` reports those as `unverifiable` precisely because nobody looked. */ export async function closeStructuredSessionsForWorktree( progress: StructuredSweepProgress, deadline: number, - runtime?: StructuredWorktreeSweepRuntime + options: { + runtime?: StructuredWorktreeSweepRuntime + /** + * Whether this removal can still refuse over an unclosed session. + * + * It is the only case where the workspace — and therefore its chat tabs — survives, so it is + * the only case where an unproven close may put a tab back. Force and the folder-workspace + * paths discard the workspace whatever the sweep reports. + */ + mayRefuse?: boolean + } = {} ): Promise { + const { runtime, mayRefuse } = options // No `afterClose` for a dispatched worker: `host.close` drops the holds, so nothing keeps a // provider child un-evictable, but the dispatch's redrive subscription and registry entry do // survive until it settles by another verb. That is a bounded leak, not a hazard — and passing @@ -231,10 +244,10 @@ export async function closeStructuredSessionsForWorktree( if (Date.now() >= deadline) { return } - const outcome = await closeStructuredAgentSessionChild( - session.sessionId, - runtime ? { runtime } : {} - ) + const outcome = await closeStructuredAgentSessionChild(session.sessionId, { + ...(runtime ? { runtime } : {}), + restoreTabOnUnprovenClose: mayRefuse === true + }) if (outcome.stopped) { progress.closed += 1 } else { @@ -246,6 +259,11 @@ export async function closeStructuredSessionsForWorktree( // observation, or the record's death evidence landed after it read. Refusing on a child // that is demonstrably gone is the defect this sweep exists to remove, so take the proof // and run the retirement `closeStructuredAgentSessionChild` skipped when it gave up. + // + // Including the hide it UNDID: its rollback ran against an observation taken one store + // write before this one, so a child that died in between left the tab republished for a + // session this sweep is about to count closed. Taking the proof has to take that back. + await dropDurableChatTabReference(session.sessionId) retireSettledStructuredWorkerTab(session.sessionId, runtime) progress.closed += 1 } else { @@ -257,3 +275,20 @@ export async function closeStructuredSessionsForWorktree( progress.settled += 1 } } + +/** + * Drops a settled session's durable chat-tab reference, and cannot fail the settlement. + * + * The close's own hide is the ordinary path; this is only for the session whose exit this sweep + * proved after that close had already rolled the hide back. + */ +async function dropDurableChatTabReference(sessionId: string): Promise { + try { + await getStructuredAgentSessionHost()?.setSessionTabVisibility?.(sessionId, false) + } catch (error) { + console.warn( + `[worktree-teardown] could not drop the chat tab reference for ${sessionId}`, + error + ) + } +} diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 1d16baf2fdd..84275b40eed 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -339,7 +339,13 @@ async function sweepStructuredSessions( // guess — it named every session, including the ones already closed, and reported zero closes. const progress = createStructuredSweepProgress(live) await settleBeforeDeadline( - sweeps.track(() => closeStructuredSessionsForWorktree(progress, deadline, deps.runtime)), + sweeps.track(() => + closeStructuredSessionsForWorktree(progress, deadline, { + ...(deps.runtime ? { runtime: deps.runtime } : {}), + // The only shape of removal that can leave this workspace — and its chat tabs — in place. + mayRefuse: Boolean(deps.requirePhysicalStop) && !deps.allowUnverifiedStop + }) + ), undefined, deadline )