diff --git a/src/main/runtime/structured-session-worktree-teardown.test.ts b/src/main/runtime/structured-session-worktree-teardown.test.ts index a9bdf6aa45c..f1bb669f770 100644 --- a/src/main/runtime/structured-session-worktree-teardown.test.ts +++ b/src/main/runtime/structured-session-worktree-teardown.test.ts @@ -89,19 +89,28 @@ describe('worktree teardown and structured agent sessions', () => { ]) }) - it('refuses a destructive removal rather than deleting the checkout under a live child', async () => { - // The defect this pins: all three PTY sweeps enumerate leaves, provider sessions and the local - // registry, and a structured session is on NONE of them. Every sweep answered zero, nothing - // errored, and removal proceeded — leaving the provider child running with its `cwd` deleted - // and the dispatch still reporting the worker live and exact. - installHost({ records: [record('s1', WORKTREE)] }) + it('closes a live session on an ordinary removal instead of refusing it', async () => { + // The defect this pins, and the reason the guard is not simply deleted: all three PTY sweeps + // enumerate leaves, provider sessions and the local registry, and a structured session is on + // NONE of them, so removal used to proceed leaving the provider child running with its `cwd` + // deleted. The stop belongs on the ordinary path — the same one that kills a terminal running + // the same agent — so an idle chat is no harder to delete than that terminal. + const host = installHost({ records: [record('s1', WORKTREE)] }) + await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).resolves.toMatchObject({ + structuredStopped: 1 + }) + expect(host.closed).toEqual(['s1']) + }) + + 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/ ) }) it('names the force escape hatch in the refusal, like the unstopped-PTY gate', async () => { - installHost({ records: [record('s1', WORKTREE)] }) + installHost({ records: [record('s1', WORKTREE)], stuck: new Set(['s1']) }) await expect(killAllProcessesForWorktree(WORKTREE, destructiveDeps())).rejects.toThrow(/force/i) }) @@ -109,7 +118,7 @@ describe('worktree teardown and structured agent sessions', () => { // The #11960 dead end, and the shape this file's own comments warn about: the desktop // affordance comes ONLY from the classifier, and an ordinary delete already passes force:true // for the dirty-file skip — so a refusal with no matcher shows raw CLI wording with no button. - installHost({ records: [record('s1', WORKTREE)] }) + installHost({ records: [record('s1', WORKTREE)], stuck: new Set(['s1']) }) const error = await killAllProcessesForWorktree(WORKTREE, destructiveDeps()).catch( (thrown: Error) => thrown.message ) @@ -123,7 +132,7 @@ describe('worktree teardown and structured agent sessions', () => { // A session id is one tab-id hop from the random pane key that gates a worker's mailbox, and // this string reaches CLI output and a desktop toast. A count and the providers are what a // user deciding whether to force actually needs. - installHost({ records: [record('s1', WORKTREE)] }) + installHost({ records: [record('s1', WORKTREE)], stuck: new Set(['s1']) }) const error = await killAllProcessesForWorktree(WORKTREE, destructiveDeps()).catch( (thrown: Error) => thrown.message ) diff --git a/src/main/runtime/structured-session-worktree-teardown.ts b/src/main/runtime/structured-session-worktree-teardown.ts index 226f785f039..1cb43ce5486 100644 --- a/src/main/runtime/structured-session-worktree-teardown.ts +++ b/src/main/runtime/structured-session-worktree-teardown.ts @@ -11,7 +11,12 @@ * 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; only a PROVEN live child is worth refusing a removal over. + * 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 + * refuse over: a removal refuses only on a close that did not settle, exactly as the PTY sweep + * refuses only on a stop it could not verify. */ import { getStructuredAgentSessionHost } from '../native-chat/agent-session-wire/structured-agent-session-registry' @@ -77,8 +82,9 @@ export function describeLiveStructuredSessions( /** * Closes every live structured session in the worktree, and reports what stayed. * - * Force is the documented escape hatch, so it closes rather than orphaning: a child left running - * against a deleted `cwd` is the exact outcome this whole sweep exists to prevent. + * 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. */ export async function closeStructuredSessionsForWorktree( worktreeId: string, diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index 82fa055cc3a..46c657f0893 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -101,8 +101,8 @@ export async function killAllProcessesForWorktree( ) // 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. Refusing costs nothing when there are none, and the check - // is synchronous, so a destructive removal fails fast instead of after the whole sweep budget. + // 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() const stopAttempts = new Map>() @@ -279,17 +279,18 @@ export async function killAllProcessesForWorktree( /** * The fourth sweep: structured agent sessions bound to this worktree. * - * Refuses rather than auto-closing on the ordinary destructive path. `worktree rm` is the verb - * that deletes a user's work, and a running agent session is exactly the thing they would want to - * be told about before it goes — the same bargain the unstopped-PTY gate already strikes, using - * the same `--force` escape hatch. Force closes them properly instead of orphaning a child against - * a `cwd` that is about to disappear. + * 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 + * only for the ones whose exit it could not then verify. Refusing merely because a session is + * attached made an idle chat, which the user is done with, harder to delete than a terminal running + * the same agent. Attachment is lease state, not work in flight, so it was never the right proxy. * - * Two callers participate, for different reasons. A proof-requiring removal (`requirePhysicalStop`) - * refuses, then closes under force. A folder-workspace removal (`closeStructuredSessions`) closes - * best-effort without refusing: 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. + * 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. */ async function sweepStructuredSessions( worktreeId: string, @@ -304,6 +305,19 @@ async function sweepStructuredSessions( 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. + const { closed, unstopped } = await settleBeforeDeadline( + () => closeStructuredSessionsForWorktree(worktreeId, deps.runtime), + { closed: 0, unstopped: live }, + deadline, + deadlineError + ) + 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. @@ -311,25 +325,13 @@ async function sweepStructuredSessions( // 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(live)}. ${WORKTREE_TEARDOWN_FORCE_HINT}` + `${RUNNING_AGENT_SESSION_REMOVAL_PREFIX} ${worktreeId}${UNSTOPPED_PTY_DETAIL_SEPARATOR}${describeLiveStructuredSessions(unstopped)}. ${WORKTREE_TEARDOWN_FORCE_HINT}` ) } - // 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 --force` forever with - // no timeout error at all. On expiry the force path reports the timeout exactly as the PTY - // sweeps do rather than proceeding as if the sessions had closed. - const { closed, unstopped } = await settleBeforeDeadline( - () => closeStructuredSessionsForWorktree(worktreeId, deps.runtime), - { closed: 0, unstopped: live }, - deadline, - deadlineError + // 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` ) - if (unstopped.length > 0) { - // 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` - ) - } return closed } diff --git a/src/renderer/src/components/sidebar/delete-worktree-toast.ts b/src/renderer/src/components/sidebar/delete-worktree-toast.ts index 946e99eadee..86096d1caba 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-toast.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-toast.ts @@ -81,12 +81,12 @@ export function getDeleteWorktreeToastCopy( 'Failed to delete workspace {{value0}}', { value0: worktreeName } ), - // Why this is not the "could not confirm" wording: Orca watched these sessions stay - // attached, so there is no doubt to waive — Force Delete ends a conversation that is - // running right now, and any work it holds goes with it. + // 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', - 'This workspace still has running agent sessions, so Orca stopped before deleting any files. Force Delete will close them and discard any work they hold.' + '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 ad5d2e296b0..e4134b72b15 100644 --- a/src/renderer/src/i18n/locales/en.json +++ b/src/renderer/src/i18n/locales/en.json @@ -5786,7 +5786,7 @@ "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": "This workspace still has running agent sessions, so Orca stopped before deleting any files. Force Delete will close them and discard any 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." } } },