mirror of
https://github.com/stablyai/orca.git
synced 2026-09-29 00:02:56 +00:00
fix(worktrees): close an idle structured chat on delete instead of refusing
`worktree rm` refused whenever any structured chat session was attached to the workspace, so an idle Codex/Claude chat that had already answered was harder to delete than a terminal actively running the same agent. The PTY sweep stops every terminal it owns and refuses only for the ones whose exit it could not verify. The structured sweep refused on `live` alone and never attempted the close, which ran only under force. `live` is lease state — a provider child is attached — not work in flight, so it was never the right proxy for "you would lose something". Close first, refuse only on what did not settle. The refusal now means the same thing the unverified-PTY one does, so the toast takes that wording.
This commit is contained in:
committed by
Merge Sim
parent
7dd183d82d
commit
5dc2df91cb
@@ -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
|
||||
)
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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<string, Promise<boolean>>()
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
@@ -5786,7 +5786,7 @@
|
||||
"lockedReason": "This workspace is locked by Git. Git reported: {{value0}}. Run git worktree unlock <worktree-path> 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."
|
||||
}
|
||||
}
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user