diff --git a/src/main/ipc/worktrees/removal/worktree-removal-ownership.ts b/src/main/ipc/worktrees/removal/worktree-removal-ownership.ts index ec2198bcdb0..ecc982076c5 100644 --- a/src/main/ipc/worktrees/removal/worktree-removal-ownership.ts +++ b/src/main/ipc/worktrees/removal/worktree-removal-ownership.ts @@ -40,11 +40,17 @@ export async function stopPtysForDestructiveWorktreeRemoval( ...(allowUnverifiedStop ? { allowUnverifiedStop: true } : {}), ...(connectionId ? { includeLocalRegistry: false } : {}) }) + // Structured sessions are counted here too: closing a user's chat is now an ordinary outcome + // of this verb, and a removal that closed one but no PTY would otherwise log nothing at all. + const structuredStopped = teardownResult.structuredStopped ?? 0 const total = - teardownResult.runtimeStopped + teardownResult.providerStopped + teardownResult.registryStopped + teardownResult.runtimeStopped + + teardownResult.providerStopped + + teardownResult.registryStopped + + structuredStopped if (total > 0) { console.info( - `[worktree-teardown] ${worktreeId} killed runtime=${teardownResult.runtimeStopped} provider=${teardownResult.providerStopped} registry=${teardownResult.registryStopped}` + `[worktree-teardown] ${worktreeId} killed runtime=${teardownResult.runtimeStopped} provider=${teardownResult.providerStopped} registry=${teardownResult.registryStopped} structured=${structuredStopped}` ) } } diff --git a/src/main/runtime/orca-runtime-pty-foreground-process-reads.ts b/src/main/runtime/orca-runtime-pty-foreground-process-reads.ts index a7e10247fed..84161b64789 100644 --- a/src/main/runtime/orca-runtime-pty-foreground-process-reads.ts +++ b/src/main/runtime/orca-runtime-pty-foreground-process-reads.ts @@ -149,13 +149,17 @@ export class OrcaRuntimeWithPtyForegroundProcessReads extends OrcaRuntimeWithSta ...(allowUnverifiedStop ? { allowUnverifiedStop: true } : {}), ...(connectionId ? { includeLocalRegistry: false } : {}) }) + // Structured sessions are counted here too, mirroring the IPC path: closing a user's chat is + // now an ordinary outcome of this verb, and a removal that closed one but no PTY logged nothing. + const structuredStopped = teardownResult.structuredStopped ?? 0 const total = teardownResult.runtimeStopped + teardownResult.providerStopped + - teardownResult.registryStopped + teardownResult.registryStopped + + structuredStopped if (total > 0) { console.info( - `[worktree-teardown] ${worktreeId} killed runtime=${teardownResult.runtimeStopped} provider=${teardownResult.providerStopped} registry=${teardownResult.registryStopped}` + `[worktree-teardown] ${worktreeId} killed runtime=${teardownResult.runtimeStopped} provider=${teardownResult.providerStopped} registry=${teardownResult.registryStopped} structured=${structuredStopped}` ) } } diff --git a/src/main/runtime/structured-session-worktree-teardown.test.ts b/src/main/runtime/structured-session-worktree-teardown.test.ts index f1bb669f770..8ec87f7f9c3 100644 --- a/src/main/runtime/structured-session-worktree-teardown.test.ts +++ b/src/main/runtime/structured-session-worktree-teardown.test.ts @@ -8,18 +8,31 @@ vi.mock('../native-chat/agent-session-wire/structured-agent-session-registry', ( })) const { killAllProcessesForWorktree } = await import('./worktree-teardown') -const { classifyWorktreeForceDeleteReason } = await import('../../shared/worktree/removal') +const { + classifyWorktreeForceDeleteReason, + isProvenLiveStructuredSessionRemovalError, + isUnstoppedPtyRemovalError +} = await import('../../shared/worktree/removal') const { listLiveStructuredSessionsForWorktree } = await import('./structured-session-worktree-teardown') const WORKTREE = 'repo_1::/tmp/wt-a' const OTHER_WORKTREE = 'repo_1::/tmp/wt-b' -function record(sessionId: string, workspaceId: string): AgentSessionRecord { +function record( + sessionId: string, + workspaceId: string, + options: { provider?: 'claude' | 'codex'; executionHostId?: string } = {} +): AgentSessionRecord { return { sessionId, - provider: 'claude', - location: { executionHostId: 'local', wslDistro: null, workspaceId, workspaceKind: 'folder' }, + provider: options.provider ?? 'claude', + location: { + executionHostId: options.executionHostId ?? 'local', + wslDistro: null, + workspaceId, + workspaceKind: 'folder' + }, lease: { sessionId, runtimeKind: 'native', @@ -33,8 +46,12 @@ function record(sessionId: string, workspaceId: string): AgentSessionRecord { function installHost(options: { records: AgentSessionRecord[] - /** Sessions the host still holds; a close removes one unless it is listed as stuck. */ + /** Sessions the host keeps holding through a close, so the post-close observation is `live`. */ stuck?: Set + /** Sessions the host drops without death evidence, so the observation is `unverifiable`. */ + unverifiable?: Set + /** Blocks every close, to exercise the shared sweep budget without fake timers. */ + closeGate?: Promise }): { closed: string[] } { const held = new Set(options.records.map((entry) => entry.sessionId)) const closed: string[] = [] @@ -44,13 +61,18 @@ function installHost(options: { setSessionTabVisibility: async () => {}, close: async (sessionId: string) => { closed.push(sessionId) - if (!options.stuck?.has(sessionId)) { - held.delete(sessionId) - 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 } - } + await options.closeGate + if (options.stuck?.has(sessionId)) { + return + } + held.delete(sessionId) + 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 } } } } @@ -67,7 +89,7 @@ const localProvider = { shutdown: async () => {} } as never -function destructiveDeps(extra: { allowUnverifiedStop?: boolean } = {}) { +function destructiveDeps(extra: { allowUnverifiedStop?: boolean; timeoutMs?: number } = {}) { return { localProvider, requirePhysicalStop: true, @@ -84,7 +106,7 @@ 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([ + expect(listLiveStructuredSessionsForWorktree(WORKTREE, {})).toEqual([ { sessionId: 's1', agent: 'claude' } ]) }) @@ -105,7 +127,7 @@ describe('worktree teardown and structured agent sessions', () => { it('refuses only when the close does not settle', async () => { installHost({ records: [record('s1', WORKTREE)], stuck: new Set(['s1']) }) await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).rejects.toThrow( - /1 running agent session/ + /still live: 1 agent session \(claude\)/ ) }) @@ -137,7 +159,7 @@ describe('worktree teardown and structured agent sessions', () => { (thrown: Error) => thrown.message ) expect(error).not.toContain('s1') - expect(error).toContain('1 running agent session') + expect(error).toContain('1 agent session (claude)') }) it('closes best-effort for a folder-workspace removal, which requires no stop proof', async () => { @@ -191,6 +213,116 @@ describe('worktree teardown and structured agent sessions', () => { ).resolves.toMatchObject({ runtimeStopped: 0 }) }) + it('leaves a same-id workspace on another execution host alone', async () => { + // A workspace id is `repoId::path` with no host component, so the local, SSH and paired-runtime + // copies of one id are DIFFERENT workspaces. Unfenced, deleting the local one closed a chat + // running on somebody else's machine — a destructive cross-host act, not a spurious refusal. + const host = installHost({ + records: [record('s1', WORKTREE, { executionHostId: 'ssh:host-a' })] + }) + await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).resolves.toMatchObject({ + runtimeStopped: 0 + }) + expect(host.closed).toEqual([]) + }) + + it('closes only the session on the host the removal resolved to', async () => { + const host = installHost({ + records: [record('s1', WORKTREE, { executionHostId: 'ssh:host-a' }), record('s2', WORKTREE)] + }) + await expect( + killAllProcessesForWorktree(WORKTREE, { + ...destructiveDeps(), + resolvedConnectionId: 'host-a' + }) + ).resolves.toMatchObject({ structuredStopped: 1 }) + expect(host.closed).toEqual(['s1']) + }) + + it('names only the sessions that stayed, and every provider still there', async () => { + installHost({ + records: [ + record('s1', WORKTREE), + record('s2', WORKTREE, { provider: 'codex' }), + record('s3', WORKTREE) + ], + stuck: new Set(['s2', 's3']) + }) + const error = await killAllProcessesForWorktree(WORKTREE, destructiveDeps()).catch( + (thrown: Error) => thrown.message + ) + expect(error).toContain('still live: 2 agent sessions (claude, codex)') + }) + + it('separates a close it could not confirm from one it watched stay attached', async () => { + // `src/shared/worktree/removal.ts` keeps these two apart on purpose: a user waiving "we could + // not confirm" is making a different decision than one discarding a conversation Orca just saw + // running. The toast branches on this marker, so flattening them makes one of the two a lie. + installHost({ records: [record('s1', WORKTREE)], unverifiable: new Set(['s1']) }) + const unconfirmed = await killAllProcessesForWorktree(WORKTREE, destructiveDeps()).catch( + (thrown: Error) => thrown.message + ) + expect(unconfirmed).toContain('could not confirm these closed: 1 agent session (claude)') + expect(isProvenLiveStructuredSessionRemovalError(unconfirmed as string)).toBe(false) + + installHost({ records: [record('s1', WORKTREE)], stuck: new Set(['s1']) }) + const live = await killAllProcessesForWorktree(WORKTREE, destructiveDeps()).catch( + (thrown: Error) => thrown.message + ) + expect(isProvenLiveStructuredSessionRemovalError(live as string)).toBe(true) + }) + + it('refuses in agent-session wording when the close outlives the sweep budget', async () => { + // A structured close that runs out of time used to reject with the PTY timeout sentinel, which + // the classifier reads FIRST — so the toast blamed terminals, and the Force Delete meant to + // clear the wedge hit the same rejection again (#11960). + installHost({ records: [record('s1', WORKTREE)], closeGate: new Promise(() => {}) }) + const error = await killAllProcessesForWorktree( + WORKTREE, + destructiveDeps({ timeoutMs: 5 }) + ).catch((thrown: Error) => thrown.message) + expect(error).toContain('could not confirm these closed: 1 agent session (claude)') + expect(isUnstoppedPtyRemovalError(error as string)).toBe(false) + expect(classifyWorktreeForceDeleteReason(error as string, true)).toBe('running-agent-session') + }) + + it('never wedges Force Delete on a close that will not settle', async () => { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}) + installHost({ records: [record('s1', WORKTREE)], closeGate: new Promise(() => {}) }) + await expect( + killAllProcessesForWorktree( + WORKTREE, + destructiveDeps({ allowUnverifiedStop: true, timeoutMs: 5 }) + ) + ).resolves.toMatchObject({ runtimeStopped: 0 }) + expect(warn).toHaveBeenCalledWith(expect.stringContaining('still attached')) + warn.mockRestore() + }) + + it('starts the terminal sweeps while the structured close is still in flight', async () => { + // The close is serial and each one waits on a provider round trip. Awaiting it before the + // sweeps exist spends the shared budget head-first, and the sweeps then report a timeout for + // a stop they never attempted. + let releaseClose: () => void = () => {} + const closeGate = new Promise((resolve) => { + releaseClose = resolve + }) + installHost({ records: [record('s1', WORKTREE)], closeGate }) + let terminalSweepStarted = false + const runtime = { + stopTerminalsForWorktree: async () => { + terminalSweepStarted = true + return { stopped: 0 } + } + } as never + const removal = killAllProcessesForWorktree(WORKTREE, { ...destructiveDeps(), runtime }) + await vi.waitFor(() => { + expect(terminalSweepStarted).toBe(true) + }) + releaseClose() + await expect(removal).resolves.toMatchObject({ structuredStopped: 1 }) + }) + 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 1cb43ce5486..81876f46285 100644 --- a/src/main/runtime/structured-session-worktree-teardown.ts +++ b/src/main/runtime/structured-session-worktree-teardown.ts @@ -8,10 +8,10 @@ * kept running with its `cwd` gone, the durable record and chat tab survived to republish at the * next launch pointing at a deleted worktree, and `worker-show` still reported the worker live. * - * Membership is `location.workspaceId`, which every structured session carries — so this covers a - * plain chat session in the worktree as well as a dispatched worker. Liveness is - * `observeStructuredWorker`, the same `live` / `unverifiable` / `exited` vocabulary the rest of the - * structured surface uses. + * Membership is `location.workspaceId` PLUS the host fence below, and every structured session + * carries both — so this covers a plain chat session in the worktree as well as a dispatched + * worker. Liveness is `observeStructuredWorker`, the same `live` / `unverifiable` / `exited` + * vocabulary the rest of the structured surface uses. * * `live` here is lease state — a provider child is attached — not work in flight, so it says * nothing about whether the user would lose anything. It selects what to CLOSE, never what to @@ -19,6 +19,13 @@ * refuses only on a stop it could not verify. */ +import { + LOCAL_EXECUTION_HOST_ID, + toRuntimeExecutionHostId, + toSshExecutionHostId, + type ExecutionHostId +} from '../../shared/execution-host' +import { STILL_LIVE_DETAIL_PREFIX } from '../../shared/worktree/removal' import { getStructuredAgentSessionHost } from '../native-chat/agent-session-wire/structured-agent-session-registry' import { observeStructuredWorker } from './structured-worker-authority' import { closeStructuredAgentSessionChild } from './structured-agent-session-close' @@ -29,13 +36,45 @@ export type LiveStructuredSessionInWorkspace = { agent: 'claude' | 'codex' } +export type UnclosedStructuredSession = LiveStructuredSessionInWorkspace & { + /** Read AFTER the close: `live` is a child watched stay attached, not merely one left unproven. */ + status: 'live' | 'unverifiable' +} + export type StructuredWorktreeSweepRuntime = Pick< OrcaRuntimeService, 'forgetStructuredSessionMail' | 'retireStructuredAgentSessionTabFromSnapshot' > +/** The two fields every teardown caller already resolves to fence its PTY sweeps to one host. */ +export type StructuredSessionHostFence = { + resolvedConnectionId?: string + resolvedRuntimeEnvironmentId?: string +} + /** - * Structured sessions with a proven-live child in this worktree. + * The one execution host this teardown may touch. + * + * A workspace id is `repoId::path` with no host component, so the local machine, an SSH host and a + * paired runtime can all publish the SAME id and each names a DIFFERENT workspace (STA-4343). The + * PTY sweeps fence on exactly these two fields; a structured session records its host directly, so + * the comparison is on `location.executionHostId` instead of on a pty-id shape — but the + * precedence is the same. Neither field set means the removal targets this machine, which is also + * the safe default: a caller that resolved no host closes nothing on anyone else's. + */ +export function structuredSessionTeardownHostId( + fence: StructuredSessionHostFence +): ExecutionHostId { + if (fence.resolvedRuntimeEnvironmentId !== undefined) { + return toRuntimeExecutionHostId(fence.resolvedRuntimeEnvironmentId) + } + return fence.resolvedConnectionId === undefined + ? LOCAL_EXECUTION_HOST_ID + : toSshExecutionHostId(fence.resolvedConnectionId) +} + +/** + * Structured sessions with a proven-live child 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 @@ -43,7 +82,8 @@ export type StructuredWorktreeSweepRuntime = Pick< * directly — that would force-install the host, which is itself a side effect on a teardown path. */ export function listLiveStructuredSessionsForWorktree( - worktreeId: string + worktreeId: string, + fence: StructuredSessionHostFence ): LiveStructuredSessionInWorkspace[] { const host = getStructuredAgentSessionHost() if (!host) { @@ -55,48 +95,63 @@ export function listLiveStructuredSessionsForWorktree( } catch { return [] } + const hostId = structuredSessionTeardownHostId(fence) return records .filter( (record) => record.location.workspaceId === worktreeId && + record.location.executionHostId === hostId && observeStructuredWorker({ sessionId: record.sessionId }).status === 'live' ) .map((record) => ({ sessionId: record.sessionId, agent: record.provider })) } /** - * Counts and providers, never session ids. + * Counts, providers and the post-close verdict — never session ids. * * A session id is one tab-id hop from the random pane key that gates a worker's mailbox, and this * string reaches agent-readable CLI output and a desktop toast. The count and the providers are * what a user deciding whether to force actually needs; the ids identify nothing they can act on. + * + * The verdict is here for the reason `describeUnstoppedPtys` carries one: "we watched it stay + * attached" and "we could not confirm it went" are different decisions to waive, and the delete + * toast branches on this marker. Any proven-live session makes the whole refusal a live one, as it + * does for PTYs — that is the stronger warning, and the one whose work is about to be discarded. */ -export function describeLiveStructuredSessions( - sessions: readonly LiveStructuredSessionInWorkspace[] +export function describeUnclosedStructuredSessions( + sessions: readonly UnclosedStructuredSession[] ): string { - const noun = sessions.length === 1 ? 'agent session' : 'agent sessions' - const providers = [...new Set(sessions.map((session) => session.agent))].sort().join(', ') - return `${sessions.length} running ${noun} (${providers})` + const stillLive = sessions.filter((session) => session.status === 'live') + const named = stillLive.length > 0 ? stillLive : sessions + const noun = named.length === 1 ? 'agent session' : 'agent sessions' + const providers = [...new Set(named.map((session) => session.agent))].sort().join(', ') + const summary = `${named.length} ${noun} (${providers})` + return stillLive.length > 0 + ? `${STILL_LIVE_DETAIL_PREFIX} ${summary}` + : `could not confirm these closed: ${summary}` } /** - * Closes every live structured session in the worktree, and reports what stayed. + * Closes the given structured sessions, and reports what stayed. * * Runs on the ordinary removal too, not just force: a child left running against a deleted `cwd` is * 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. */ export async function closeStructuredSessionsForWorktree( - worktreeId: string, + sessions: readonly LiveStructuredSessionInWorkspace[], runtime?: StructuredWorktreeSweepRuntime -): Promise<{ closed: number; unstopped: LiveStructuredSessionInWorkspace[] }> { +): Promise<{ closed: number; unstopped: UnclosedStructuredSession[] }> { // 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 // one here would mean resolving a dispatch id per session on a teardown path that must stay // inside the sweep deadline. - const sessions = listLiveStructuredSessionsForWorktree(worktreeId) - const unstopped: LiveStructuredSessionInWorkspace[] = [] + const unstopped: UnclosedStructuredSession[] = [] let closed = 0 for (const session of sessions) { const outcome = await closeStructuredAgentSessionChild( @@ -105,9 +160,12 @@ export async function closeStructuredSessionsForWorktree( ) if (outcome.stopped) { closed += 1 - } else { - unstopped.push(session) + continue } + // Re-observed rather than reusing the close's own reason string: what the user is asked to + // waive is the state AFTER the attempt, and a close that threw never reached an observation. + const status = observeStructuredWorker({ sessionId: session.sessionId }).status + unstopped.push({ ...session, status: status === 'live' ? 'live' : 'unverifiable' }) } return { closed, unstopped } } diff --git a/src/main/runtime/unstopped-pty-verification.ts b/src/main/runtime/unstopped-pty-verification.ts index a2cd83b8eec..7cfd5907797 100644 --- a/src/main/runtime/unstopped-pty-verification.ts +++ b/src/main/runtime/unstopped-pty-verification.ts @@ -2,7 +2,7 @@ import type { IPtyProvider } from '../providers/types' import type { OrcaRuntimeService } from './orca-runtime' import { UNSTOPPED_PTY_DETAIL_SEPARATOR, - UNSTOPPED_PTY_LIVE_DETAIL_PREFIX, + STILL_LIVE_DETAIL_PREFIX, UNSTOPPED_PTY_REMOVAL_PREFIX } from '../../shared/worktree/removal' import { @@ -105,7 +105,7 @@ export function describeUnstoppedPtys( ): string { const detail = verdict.status === 'live' - ? `${UNSTOPPED_PTY_LIVE_DETAIL_PREFIX} ${verdict.ptyIds.join(', ')}` + ? `${STILL_LIVE_DETAIL_PREFIX} ${verdict.ptyIds.join(', ')}` : `could not verify these exited: ${failedPtyIds.join(', ')} (${verdict.reason})` return `${UNSTOPPED_PTY_REMOVAL_PREFIX} ${worktreeId}${UNSTOPPED_PTY_DETAIL_SEPARATOR}${detail}` } diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 46c657f0893..e6cab8fb053 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -15,10 +15,14 @@ import { } from './worktree-pty-surface-sweeps' import { closeStructuredSessionsForWorktree, - describeLiveStructuredSessions, + describeUnclosedStructuredSessions, listLiveStructuredSessionsForWorktree } from './structured-session-worktree-teardown' -import { createWorktreeSweepTracker, settleSweepsForForcedRemoval } from './forced-sweep-settlement' +import { + createWorktreeSweepTracker, + settleSweepsForForcedRemoval, + type WorktreeSweepTracker +} from './forced-sweep-settlement' import { describeError, describeFailedPtySweep, @@ -57,7 +61,7 @@ export type WorktreeTeardownResult = { runtimeStopped: number providerStopped: number registryStopped: number - /** Structured agent sessions closed by the force path; absent when none were found. */ + /** Structured agent sessions this teardown closed; absent when it closed none. */ structuredStopped?: number } @@ -99,12 +103,18 @@ export async function killAllProcessesForWorktree( const deadlineError = new Error( `${WORKTREE_TEARDOWN_TIMEOUT_PREFIX} ${worktreeId}. ${WORKTREE_TEARDOWN_FORCE_HINT}` ) - // FIRST, and before a single PTY sweep starts: a structured agent session is registered on none - // of the three surfaces below, so all three answered zero and removal deleted the checkout out - // from under a running provider child. It returns immediately when there are none, and its own - // close is bounded by the same deadline the PTY sweeps share. - const structuredStopped = await sweepStructuredSessions(worktreeId, deps, deadline, deadlineError) const sweeps = createWorktreeSweepTracker() + // ISSUED first, before a single PTY is touched: a structured agent session is registered on none + // of the three surfaces below, so all three answered zero and removal deleted the checkout out + // from under a running provider child. Asking the agent plane ahead of the terminal plane also + // keeps an intentional stop from reading as a failed process exit. + // + // Not AWAITED first, though. Its close is serial and each one waits on a provider round trip, so + // awaiting here would spend the shared budget before a single PTY was asked — and the sweeps + // would then report a timeout for a stop they never attempted. It is joined below, ahead of the + // PTY verdict, so a structured refusal still outranks one. + const structuredSweep = sweepStructuredSessions(worktreeId, deps, deadline, sweeps) + void structuredSweep.catch(() => undefined) const stopAttempts = new Map>() const stopPty = ( ptyId: string, @@ -196,6 +206,7 @@ export async function killAllProcessesForWorktree( for (const sweep of [runtimeSweep, providerSweep, registrySweep]) { void sweep.catch(() => undefined) } + const structuredStopped = await structuredSweep let runtimeResult: { stopped: number } let providerStopped: number let registryStopped: number @@ -277,7 +288,7 @@ export async function killAllProcessesForWorktree( } /** - * The fourth sweep: structured agent sessions bound to this worktree. + * The fourth sweep: structured agent sessions bound to this worktree, on this host. * * Stops first and refuses only on unproven stops, which is the bargain the unstopped-PTY gate * actually strikes: that gate kills every PTY — a terminal running an agent included — and refuses @@ -288,50 +299,60 @@ export async function killAllProcessesForWorktree( * Two callers participate. A proof-requiring removal (`requirePhysicalStop`) refuses when a close * does not settle, so nothing deletes a checkout out from under a child that is still there. A * folder-workspace removal (`closeStructuredSessions`) never refuses: it shares its root so no - * checkout vanishes under the child, and one of those paths is a never-throw forget that a refusal - * would wedge. Reconciliation sweeps set neither — they repair state, delete nothing, and must - * never close a session. + * checkout vanishes under the child, and every one of those call sites discards a rejection, so a + * refusal there would be words nobody reads. Reconciliation sweeps set neither — they repair state, + * delete nothing, and must never close a session. */ async function sweepStructuredSessions( worktreeId: string, deps: WorktreeTeardownDeps, deadline: number, - deadlineError: Error + sweeps: WorktreeSweepTracker ): Promise { if (!deps.requirePhysicalStop && !deps.closeStructuredSessions) { return 0 } - const live = listLiveStructuredSessionsForWorktree(worktreeId) + // `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) if (live.length === 0) { return 0 } - // Raced against the same sweep budget every PTY surface is bounded by: `host.close` awaits a - // provider round trip, and a wedged one would otherwise hang `worktree rm` forever with no - // timeout error at all. On expiry the sweep reports the timeout exactly as the PTY sweeps do - // rather than proceeding as if the sessions had closed. + // 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. + // + // Deliberately NOT fail-closed, unlike the PTY sweeps: their timeout sentinel carries the PTY + // timeout prefix, which the desktop classifier reads as a TERMINAL failure — so a wedged session + // close would refuse in terminal wording, and refuse identically again under the Force Delete + // that is meant to clear it (#11960). A close that ran out of time is a session this removal + // could not confirm closed, which is exactly what the branch below already words. Tracked so a + // forced removal still waits out the abandoned-sweep grace before it deletes files. const { closed, unstopped } = await settleBeforeDeadline( - () => closeStructuredSessionsForWorktree(worktreeId, deps.runtime), - { closed: 0, unstopped: live }, - deadline, - deadlineError + sweeps.track(() => closeStructuredSessionsForWorktree(live, deps.runtime)), + { + closed: 0, + unstopped: live.map((session) => ({ ...session, status: 'unverifiable' as const })) + }, + deadline ) if (unstopped.length === 0) { return closed } // 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 - // has forgotten — and one of those paths is a never-throw forget, which a refusal would wedge. + // has forgotten — and every one of those callers discards a rejection anyway. if (deps.requirePhysicalStop && !deps.allowUnverifiedStop) { // The prefix is what the desktop classifier matches on; without it the toast shows raw CLI // wording and hides the Force Delete button — the #11960 dead end this file already documents. throw new Error( - `${RUNNING_AGENT_SESSION_REMOVAL_PREFIX} ${worktreeId}${UNSTOPPED_PTY_DETAIL_SEPARATOR}${describeLiveStructuredSessions(unstopped)}. ${WORKTREE_TEARDOWN_FORCE_HINT}` + `${RUNNING_AGENT_SESSION_REMOVAL_PREFIX} ${worktreeId}${UNSTOPPED_PTY_DETAIL_SEPARATOR}${describeUnclosedStructuredSessions(unstopped)}. ${WORKTREE_TEARDOWN_FORCE_HINT}` ) } // Force is the documented escape hatch, so removal continues — but say so, because the child // outliving its `cwd` is the failure this sweep exists to make visible. console.warn( - `[worktree-teardown] forcing removal of ${worktreeId} with ${describeLiveStructuredSessions(unstopped)} still attached` + `[worktree-teardown] forcing removal of ${worktreeId} with ${describeUnclosedStructuredSessions(unstopped)} still attached` ) return closed } diff --git a/src/renderer/src/components/sidebar/delete-worktree-toast.test.ts b/src/renderer/src/components/sidebar/delete-worktree-toast.test.ts index dffedf06337..c63b92cc174 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-toast.test.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-toast.test.ts @@ -50,6 +50,39 @@ describe('getDeleteWorktreeToastCopy', () => { }) }) + // Why: the structured sweep now CLOSES an attached session on the ordinary delete, so reaching + // this toast means the close was attempted and did not settle — not that Orca declined to try. + it('offers force delete when an agent session could not be confirmed closed', () => { + expect( + toastCopyForRemovalError( + 'feature/foo', + 'Refusing to remove worktree with running agent sessions: repo-1::/w — could not confirm these closed: 1 agent session (claude). Retry with force delete (--force) to remove it anyway.' + ) + ).toEqual({ + title: 'Failed to delete workspace feature/foo', + description: + 'Orca could not confirm every agent session in this workspace has closed, so it stopped before deleting any files. Use Force Delete to remove it anyway.', + isDestructive: false + }) + }) + + // Why: the same split the PTY pair above draws. Force Delete proceeds either way, and telling a + // user "could not confirm" about a conversation Orca watched stay attached asks them to waive a + // doubt that does not exist — the work in that conversation goes with the delete. + it('names the running agent sessions when the close left them attached', () => { + expect( + toastCopyForRemovalError( + 'feature/foo', + 'Refusing to remove worktree with running agent sessions: repo-1::/w — still live: 2 agent sessions (claude, codex). Retry with force delete (--force) to remove it anyway.' + ) + ).toEqual({ + title: 'Failed to delete workspace feature/foo', + description: + 'This workspace still has running agent sessions that Orca could not close, so it stopped before deleting any files. Force Delete will discard any work they hold.', + isDestructive: false + }) + }) + // Why: a sweep that never answered wedges removal the same way, and the waiver clears // both — so it must reach the same force affordance instead of a dead end. it('offers force delete when the teardown sweep itself timed out', () => { diff --git a/src/renderer/src/components/sidebar/delete-worktree-toast.ts b/src/renderer/src/components/sidebar/delete-worktree-toast.ts index 86096d1caba..dc32abfc40f 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-toast.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-toast.ts @@ -2,6 +2,7 @@ import { translate } from '@/i18n/i18n' import { isLockedWorktreeRemovalError, isProvenLivePtyRemovalError, + isProvenLiveStructuredSessionRemovalError, type WorktreeForceDeleteReason } from '../../../../shared/worktree/removal' export type DeleteWorktreeToastCopy = { @@ -81,13 +82,19 @@ export function getDeleteWorktreeToastCopy( 'Failed to delete workspace {{value0}}', { value0: worktreeName } ), - // Why this is the "could not confirm" wording: an ordinary delete already closed these - // sessions, so reaching here means the close did not settle — the same doubt the - // unverified-PTY case asks the user to waive, not a session Orca declined to close. - description: translate( - 'auto.components.sidebar.delete.worktree.toast.runningAgentSession', - 'Orca could not confirm every agent session in this workspace has closed, so it stopped before deleting any files. Use Force Delete to remove it anyway.' - ), + // Why two branches, like the PTY pair above: an ordinary delete already tried to close + // these sessions, and only the observation AFTER that attempt separates one Orca watched + // stay attached from one it simply could not reach. Telling the first user "could not + // confirm" asks them to waive a doubt that does not exist, and a conversation dies with it. + description: isProvenLiveStructuredSessionRemovalError(error) + ? translate( + 'auto.components.sidebar.delete.worktree.toast.runningAgentSessionLive', + 'This workspace still has running agent sessions that Orca could not close, so it stopped before deleting any files. Force Delete will discard any work they hold.' + ) + : translate( + 'auto.components.sidebar.delete.worktree.toast.runningAgentSession', + 'Orca could not confirm every agent session in this workspace has closed, so it stopped before deleting any files. Use Force Delete to remove it anyway.' + ), isDestructive: false } } diff --git a/src/renderer/src/i18n/locales/en.json b/src/renderer/src/i18n/locales/en.json index e4134b72b15..4fd2203c462 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -5786,7 +5786,8 @@ "lockedReason": "This workspace is locked by Git. Git reported: {{value0}}. Run git worktree unlock from its repository, then retry deletion.", "unstoppedPty": "Orca could not confirm every terminal in this workspace has exited, so it stopped before deleting any files. Use Force Delete to remove it anyway.", "unstoppedPtyLive": "This workspace still has running terminals, so Orca stopped before deleting any files. Force Delete will kill them and discard any uncommitted work they hold.", - "runningAgentSession": "Orca could not confirm every agent session in this workspace has closed, so it stopped before deleting any files. Use Force Delete to remove it anyway." + "runningAgentSession": "Orca could not confirm every agent session in this workspace has closed, so it stopped before deleting any files. Use Force Delete to remove it anyway.", + "runningAgentSessionLive": "This workspace still has running agent sessions that Orca could not close, so it stopped before deleting any files. Force Delete will discard any work they hold." } } }, diff --git a/src/shared/worktree/removal.ts b/src/shared/worktree/removal.ts index 59e5803eddb..237d73630df 100644 --- a/src/shared/worktree/removal.ts +++ b/src/shared/worktree/removal.ts @@ -22,11 +22,12 @@ export type WorktreeForceDeleteReason = // rather than scanning the whole message and letting a path spell out a verdict. export const UNSTOPPED_PTY_DETAIL_SEPARATOR = ' — ' -// Why: verification distinguishes a PTY it watched stay alive from one it could not reach, +// Why: verification distinguishes a process it watched stay alive from one it could not reach, // and the delete toast must not flatten the two — a user waiving "we could not confirm" is -// making a different decision than one killing a terminal Orca just saw running. The marker -// and its matcher stay together for the same reason the force hint does. -export const UNSTOPPED_PTY_LIVE_DETAIL_PREFIX = 'still live:' +// making a different decision than one killing something Orca just saw running. The marker +// and its matcher stay together for the same reason the force hint does. Shared by the PTY +// sweep and the structured-session sweep, which both re-observe after their stop. +export const STILL_LIVE_DETAIL_PREFIX = 'still live:' // Why (#11960): a sweep that never answers wedges removal exactly like a stop that could not // be proven, and the waiver clears both — but this error carries different words, so without @@ -54,7 +55,15 @@ export function isUnstoppedPtyRemovalError(error: string): boolean { export function isProvenLivePtyRemovalError(error: string): boolean { return ( isUnstoppedPtyRemovalError(error) && - error.includes(`${UNSTOPPED_PTY_DETAIL_SEPARATOR}${UNSTOPPED_PTY_LIVE_DETAIL_PREFIX}`) + error.includes(`${UNSTOPPED_PTY_DETAIL_SEPARATOR}${STILL_LIVE_DETAIL_PREFIX}`) + ) +} + +/** True only when the observation AFTER the close found the session still attached. */ +export function isProvenLiveStructuredSessionRemovalError(error: string): boolean { + return ( + isRunningAgentSessionRemovalError(error) && + error.includes(`${UNSTOPPED_PTY_DETAIL_SEPARATOR}${STILL_LIVE_DETAIL_PREFIX}`) ) }