mirror of
https://github.com/stablyai/orca.git
synced 2026-10-20 16:03:20 +00:00
fix(worktree): tell the truth about live PTYs, and offer force for a wedged sweep (#12394)
* fix(worktree): tell the truth about live PTYs, and offer force for a wedged sweep
Two gaps in the #11960 force path:
The delete toast described every unstopped-PTY failure as "could not confirm
every terminal has exited", including the case where verification positively
watched them running. Force Delete proceeds either way, so the user was being
asked to waive a doubt that did not exist while a running agent's uncommitted
work died with it. The live verdict now gets copy that says so.
A sweep that rejects before any per-PTY verdict exists (wedged daemon, dropped
SSH channel) fails with a teardown-timeout message that the force classifier did
not recognise, so no Force Delete button appeared — the exact dead end #11960
set out to remove. That error now carries the shared prefix and classifies.
* fix(worktree): close the sweep-rejection wedge and stop racing the delete
Review of #12394 found the fix covered only half the wedge it named, and
routed users into a force path whose own safety comment was untrue.
1. Only the outer deadline was classifiable. When a provider *rejects* the
sweep — dropped SSH channel, erroring daemon — settleBeforeDeadline
rejects with the provider's original error, which carries no marker, so
classifyWorktreeForceDeleteReason still returned null and no Force Delete
button rendered. That is the exact case #11960 named. A rejected sweep on
the destructive path is now reworded through the existing unstopped-PTY
prefix (provider text preserved, original kept as `cause`), so old and new
clients alike classify it as 'unstopped-pty'.
2. Force could delete files while a sweep was still running. The deadline
rejects without cancelling run(), so allSettled resolved with shutdown()
still in flight — by construction the deadline error can only fire while
something is in flight. Force then deleted the directory a live PTY still
held open (EBUSY / half-delete on Windows and WSL). Sweeps are now tracked
so the forced path waits for the abandoned work, bounded by a 2s grace;
force never wedges, and when the grace expires the warning says handles may
outlive the delete instead of implying the sweep finished.
3. The toast test named for the classifier passed the reason in as a literal,
so it never exercised it. It now derives the reason exactly as the store
does, and fails against main.
4. Added the missing unstoppedPtyLive key to the English catalog.
5. isProvenLivePtyRemovalError anchored the 'still live:' marker to the detail
separator, so a worktree path can no longer spell out a live verdict and
flip the toast to the destructive copy.
---------
Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
(cherry picked from commit 20a2901677)
This commit is contained in:
@@ -0,0 +1,122 @@
|
||||
import { describeError } from './unstopped-pty-verification'
|
||||
|
||||
// Why: an abandoned sweep is one whose provider ignored the deadline it was already given,
|
||||
// so this is a second chance, not a second budget — long enough for a shutdown() that is
|
||||
// mid-kill to finish releasing handles, short enough that Force Delete still feels forced.
|
||||
export const ABANDONED_SWEEP_GRACE_MS = 2_000
|
||||
|
||||
export type WorktreeSweepTracker = {
|
||||
/** Wraps a sweep body so the promise stays observable after its deadline abandons it. */
|
||||
track: <T>(run: () => Promise<T>) => () => Promise<T>
|
||||
/** Resolves when every tracked sweep settled, or the grace expired; true if one is still running. */
|
||||
awaitAbandonedSweeps: (graceMs?: number) => Promise<boolean>
|
||||
}
|
||||
|
||||
/**
|
||||
* Keeps the real sweep promises reachable after `settleBeforeDeadline` stops waiting on them.
|
||||
*
|
||||
* Why: that helper rejects on its timer without cancelling `run()`, so by construction the
|
||||
* deadline error can only fire while work is still in flight — and forced removal deletes
|
||||
* the worktree directory the moment teardown returns.
|
||||
*/
|
||||
export function createWorktreeSweepTracker(): WorktreeSweepTracker {
|
||||
const running = new Set<Promise<unknown>>()
|
||||
return {
|
||||
track:
|
||||
<T>(run: () => Promise<T>) =>
|
||||
() => {
|
||||
const sweep = run()
|
||||
running.add(sweep)
|
||||
// Why: the tracked copy is observed here too, so a rejection this call abandons
|
||||
// never surfaces as an unhandled rejection in whatever ran next.
|
||||
void sweep.catch(() => undefined).finally(() => running.delete(sweep))
|
||||
return sweep
|
||||
},
|
||||
awaitAbandonedSweeps: async (graceMs = ABANDONED_SWEEP_GRACE_MS) => {
|
||||
const pending = [...running]
|
||||
if (pending.length === 0) {
|
||||
return false
|
||||
}
|
||||
let expiry: ReturnType<typeof setTimeout> | undefined
|
||||
const expired = new Promise<true>((resolve) => {
|
||||
expiry = setTimeout(() => resolve(true), graceMs)
|
||||
expiry.unref?.()
|
||||
})
|
||||
const stillRunning = await Promise.race([
|
||||
Promise.allSettled(pending).then(() => false),
|
||||
expired
|
||||
])
|
||||
clearTimeout(expiry)
|
||||
return stillRunning
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
export type WorktreeSweepPromises = {
|
||||
runtime: Promise<{ stopped: number }>
|
||||
provider: Promise<number>
|
||||
registry: Promise<number>
|
||||
}
|
||||
|
||||
export type ForcedSweepSettlement = {
|
||||
stopped: { runtimeStopped: number; providerStopped: number; registryStopped: number }
|
||||
/** A sweep failed outright, so no per-PTY verdict below it can be trusted. */
|
||||
incomplete: boolean
|
||||
}
|
||||
|
||||
/**
|
||||
* Reconciles the three sweeps for a removal carrying the Force Delete waiver (#11960).
|
||||
*
|
||||
* Force goes on to delete files, so every sweep must finish releasing handles first —
|
||||
* `Promise.all` would abandon the siblings of the first rejection while they were still
|
||||
* inside shutdown(). Waiting on the tracked promises rather than only on their deadline
|
||||
* wrappers is what makes that true for the timeout case as well.
|
||||
*/
|
||||
export async function settleSweepsForForcedRemoval(
|
||||
worktreeId: string,
|
||||
sweeps: WorktreeSweepPromises,
|
||||
tracker: WorktreeSweepTracker,
|
||||
deadlineError: Error
|
||||
): Promise<ForcedSweepSettlement> {
|
||||
const settled = await Promise.allSettled([sweeps.runtime, sweeps.provider, sweeps.registry])
|
||||
const [runtimeSettled, providerSettled, registrySettled] = settled
|
||||
// Report what the surviving sweeps actually stopped rather than a flat zero.
|
||||
const stopped = {
|
||||
runtimeStopped: runtimeSettled.status === 'fulfilled' ? runtimeSettled.value.stopped : 0,
|
||||
providerStopped: providerSettled.status === 'fulfilled' ? providerSettled.value : 0,
|
||||
registryStopped: registrySettled.status === 'fulfilled' ? registrySettled.value : 0
|
||||
}
|
||||
const reasons = settled.flatMap((result) =>
|
||||
result.status === 'rejected' ? [result.reason as unknown] : []
|
||||
)
|
||||
const stillRunning = await tracker.awaitAbandonedSweeps()
|
||||
if (reasons.length === 0 && !stillRunning) {
|
||||
return { stopped, incomplete: false }
|
||||
}
|
||||
// Why (#11960): a sweep that cannot even complete — unresponsive daemon, dropped SSH
|
||||
// channel — fails before the unproven-stop gate could offer its escape hatch, so an
|
||||
// explicit Force Delete has to survive it. Report every reason, specific ones first: the
|
||||
// shared deadline sentinel only says that *something* timed out, so leading with it would
|
||||
// bury the provider error that actually explains the failure. This warning is the only
|
||||
// trace of a removal that deleted files without proving a single stop.
|
||||
const detail = [
|
||||
...reasons.filter((candidate) => candidate !== deadlineError),
|
||||
...reasons.filter((candidate) => candidate === deadlineError)
|
||||
].map(describeError)
|
||||
if (stillRunning) {
|
||||
// Force must never wedge, so the wait is bounded — but "we stopped waiting" is not
|
||||
// "handles released", and on Windows/WSL that is the difference between a clean
|
||||
// removal and a half-deleted directory. Say so instead of implying the sweep finished.
|
||||
detail.push(
|
||||
`a sweep was still running after the ${ABANDONED_SWEEP_GRACE_MS}ms grace, so its PTY handles may outlive the delete`
|
||||
)
|
||||
}
|
||||
console.warn(
|
||||
`[worktree-teardown] forcing removal after an incomplete PTY sweep for ${worktreeId} — ${detail.join('; ')}`
|
||||
)
|
||||
// Reporting `incomplete` does skip the per-PTY verdict, which could still have named live
|
||||
// PTYs when only one sweep failed — accepted for now because this path is behind an
|
||||
// explicit Force Delete, deletes either way, and clears no registry rows, so the cost is
|
||||
// diagnosability rather than safety.
|
||||
return { stopped, incomplete: reasons.length > 0 }
|
||||
}
|
||||
@@ -1,5 +1,9 @@
|
||||
import type { IPtyProvider } from '../providers/types'
|
||||
import { UNSTOPPED_PTY_REMOVAL_PREFIX } from '../../shared/worktree-removal'
|
||||
import {
|
||||
UNSTOPPED_PTY_DETAIL_SEPARATOR,
|
||||
UNSTOPPED_PTY_LIVE_DETAIL_PREFIX,
|
||||
UNSTOPPED_PTY_REMOVAL_PREFIX
|
||||
} from '../../shared/worktree-removal'
|
||||
import { settleBeforeDeadline } from './settle-before-deadline'
|
||||
|
||||
// Floor for the verification window when the sweep ran on a very short budget.
|
||||
@@ -60,9 +64,21 @@ export function describeUnstoppedPtys(
|
||||
): string {
|
||||
const detail =
|
||||
verdict.status === 'live'
|
||||
? `still live: ${verdict.ptyIds.join(', ')}`
|
||||
? `${UNSTOPPED_PTY_LIVE_DETAIL_PREFIX} ${verdict.ptyIds.join(', ')}`
|
||||
: `could not verify these exited: ${failedPtyIds.join(', ')} (${verdict.reason})`
|
||||
return `${UNSTOPPED_PTY_REMOVAL_PREFIX} ${worktreeId} — ${detail}`
|
||||
return `${UNSTOPPED_PTY_REMOVAL_PREFIX} ${worktreeId}${UNSTOPPED_PTY_DETAIL_SEPARATOR}${detail}`
|
||||
}
|
||||
|
||||
/**
|
||||
* Words a sweep that never produced a per-PTY verdict — a wedged daemon, a dropped SSH
|
||||
* channel — as the unstopped-PTY failure it is.
|
||||
*
|
||||
* Why (#11960): this rejection carried only the provider's own wording, which the force
|
||||
* classifier cannot recognise, so the wedge the escape hatch exists for was the one case
|
||||
* that never got offered it. The provider's message stays in the text; only the shape changes.
|
||||
*/
|
||||
export function describeFailedPtySweep(worktreeId: string, error: unknown): string {
|
||||
return `${UNSTOPPED_PTY_REMOVAL_PREFIX} ${worktreeId}${UNSTOPPED_PTY_DETAIL_SEPARATOR}the terminal sweep failed: ${describeError(error)}`
|
||||
}
|
||||
|
||||
export function describeError(error: unknown): string {
|
||||
|
||||
@@ -9,6 +9,11 @@ vi.mock('../memory/pty-registry', () => ({
|
||||
}))
|
||||
|
||||
import { killAllProcessesForWorktree, WORKTREE_PROCESS_SWEEP_TIMEOUT_MS } from './worktree-teardown'
|
||||
import { ABANDONED_SWEEP_GRACE_MS } from './forced-sweep-settlement'
|
||||
import {
|
||||
classifyWorktreeForceDeleteReason,
|
||||
isProvenLivePtyRemovalError
|
||||
} from '../../shared/worktree-removal'
|
||||
import type { IPtyProvider, PtyProcessInfo } from '../providers/types'
|
||||
|
||||
// Why: these tests advance fake timers *before* awaiting the teardown, so a
|
||||
@@ -243,6 +248,13 @@ describe('destructive teardown when a PTY stop cannot be proven', () => {
|
||||
providerStopped: 0,
|
||||
registryStopped: 0
|
||||
})
|
||||
// Why: the deadline gave up on this sweep without cancelling it, so the grace
|
||||
// expired with the provider still inside listProcesses. Force deletes the
|
||||
// directory anyway, and this warning is the only record that a PTY handle may
|
||||
// have outlived it — a silent "incomplete sweep" reads as if it had finished.
|
||||
expect(warn).toHaveBeenCalledWith(
|
||||
expect.stringContaining(`still running after the ${ABANDONED_SWEEP_GRACE_MS}ms grace`)
|
||||
)
|
||||
} finally {
|
||||
releaseList([])
|
||||
await vi.advanceTimersByTimeAsync(0)
|
||||
@@ -337,6 +349,79 @@ describe('destructive teardown when a PTY stop cannot be proven', () => {
|
||||
).rejects.toThrow('ssh channel closed')
|
||||
})
|
||||
|
||||
// Why (#11960): the wedge the escape hatch exists for — an unresponsive daemon, a dropped
|
||||
// SSH channel — rejects the sweep in the provider's own words, which the force classifier
|
||||
// cannot recognise. Failing closed with no Force Delete button is the dead end itself.
|
||||
it('offers force delete for a sweep-level failure without losing the provider wording', async () => {
|
||||
listRegisteredPtysMock.mockReturnValue([])
|
||||
const localProvider = createProviderStub(async () => {
|
||||
throw new Error('SSH channel closed while listing processes')
|
||||
})
|
||||
|
||||
const error = await killAllProcessesForWorktree('repo-1::/w', {
|
||||
localProvider,
|
||||
requirePhysicalStop: true
|
||||
}).then(
|
||||
() => new Error('expected a rejection'),
|
||||
(rejection: Error) => rejection
|
||||
)
|
||||
expect(error.message).toContain('SSH channel closed while listing processes')
|
||||
expect(classifyWorktreeForceDeleteReason(error.message)).toBe('unstopped-pty')
|
||||
// Nothing was verified here, so it must not read as the proven-live verdict.
|
||||
expect(isProvenLivePtyRemovalError(error.message)).toBe(false)
|
||||
})
|
||||
|
||||
// Why: the outer deadline rejects without cancelling the sweep it gave up on, so force
|
||||
// could return — and the caller start deleting the directory — while shutdown() was still
|
||||
// killing the PTY holding it open. On Windows that rmdir fails and half-deletes the
|
||||
// workspace, which is exactly what the ordering at the removal call site exists to avoid.
|
||||
it('waits for a shutdown the deadline abandoned before force returns', async () => {
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
let releaseShutdown: () => void = () => {}
|
||||
try {
|
||||
listRegisteredPtysMock.mockReturnValue([])
|
||||
const localProvider = createProviderStub(async () => [
|
||||
{ id: 'w1@@live-1', cwd: '/tmp/w1', title: 'shell' }
|
||||
])
|
||||
let shutdownFinished = false
|
||||
;(localProvider.shutdown as unknown as ReturnType<typeof vi.fn>).mockImplementation(
|
||||
async () =>
|
||||
await new Promise<void>((resolve) => {
|
||||
releaseShutdown = () => {
|
||||
shutdownFinished = true
|
||||
resolve()
|
||||
}
|
||||
})
|
||||
)
|
||||
|
||||
const teardown = settleTeardown(
|
||||
killAllProcessesForWorktree('w1', {
|
||||
localProvider,
|
||||
timeoutMs: 30,
|
||||
requirePhysicalStop: true,
|
||||
allowUnverifiedStop: true
|
||||
})
|
||||
)
|
||||
let returned = false
|
||||
void teardown.then(() => {
|
||||
returned = true
|
||||
})
|
||||
await new Promise((resolve) => setTimeout(resolve, 150))
|
||||
|
||||
expect(shutdownFinished).toBe(false)
|
||||
expect(returned).toBe(false)
|
||||
releaseShutdown()
|
||||
await expect(teardown).resolves.toEqual({
|
||||
runtimeStopped: 0,
|
||||
providerStopped: 0,
|
||||
registryStopped: 0
|
||||
})
|
||||
} finally {
|
||||
releaseShutdown()
|
||||
warn.mockRestore()
|
||||
}
|
||||
})
|
||||
|
||||
it('lets an explicit force removal proceed past PTYs it could not stop', async () => {
|
||||
const warn = vi.spyOn(console, 'warn').mockImplementation(() => {})
|
||||
try {
|
||||
|
||||
@@ -4,10 +4,16 @@ import { listRegisteredPtys } from '../memory/pty-registry'
|
||||
import { isPathInsideOrEqual } from '../../shared/cross-platform-path'
|
||||
import { splitWorktreeId, splitWorktreeIdForFilesystem } from '../../shared/worktree-id'
|
||||
import { mapWithConcurrency } from '../../shared/map-with-concurrency'
|
||||
import { WORKTREE_TEARDOWN_FORCE_HINT } from '../../shared/worktree-removal'
|
||||
import {
|
||||
isUnstoppedPtyRemovalError,
|
||||
WORKTREE_TEARDOWN_FORCE_HINT,
|
||||
WORKTREE_TEARDOWN_TIMEOUT_PREFIX
|
||||
} from '../../shared/worktree-removal'
|
||||
import { settleBeforeDeadline } from './settle-before-deadline'
|
||||
import { createWorktreeSweepTracker, settleSweepsForForcedRemoval } from './forced-sweep-settlement'
|
||||
import {
|
||||
describeError,
|
||||
describeFailedPtySweep,
|
||||
describeUnstoppedPtys,
|
||||
verifyUnstoppedPtys,
|
||||
type UnstoppedPtyVerdict
|
||||
@@ -87,7 +93,10 @@ export async function killAllProcessesForWorktree(
|
||||
): Promise<WorktreeTeardownResult> {
|
||||
const sweepBudgetMs = Math.max(1, deps.timeoutMs ?? WORKTREE_PROCESS_SWEEP_TIMEOUT_MS)
|
||||
const deadline = Date.now() + sweepBudgetMs
|
||||
const deadlineError = new Error(`Timed out waiting for physical PTY teardown: ${worktreeId}`)
|
||||
const deadlineError = new Error(
|
||||
`${WORKTREE_TEARDOWN_TIMEOUT_PREFIX} ${worktreeId}. ${WORKTREE_TEARDOWN_FORCE_HINT}`
|
||||
)
|
||||
const sweeps = createWorktreeSweepTracker()
|
||||
const stopAttempts = new Map<string, Promise<boolean>>()
|
||||
const stopPty = (
|
||||
ptyId: string,
|
||||
@@ -116,7 +125,7 @@ export async function killAllProcessesForWorktree(
|
||||
// destructive removal closed.
|
||||
const runtimeSweep = deps.runtime
|
||||
? settleBeforeDeadline(
|
||||
() =>
|
||||
sweeps.track(() =>
|
||||
deps.runtime!.stopTerminalsForWorktree(worktreeId, {
|
||||
deadline,
|
||||
stopPty,
|
||||
@@ -127,7 +136,8 @@ export async function killAllProcessesForWorktree(
|
||||
...(deps.resolvedRuntimeEnvironmentId
|
||||
? { resolvedRuntimeEnvironmentId: deps.resolvedRuntimeEnvironmentId }
|
||||
: {})
|
||||
}),
|
||||
})
|
||||
),
|
||||
{ stopped: 0 },
|
||||
deadline,
|
||||
deps.requirePhysicalStop ? deadlineError : undefined,
|
||||
@@ -142,7 +152,7 @@ export async function killAllProcessesForWorktree(
|
||||
deps.includeProviderInventory === false
|
||||
? Promise.resolve(0)
|
||||
: settleBeforeDeadline(
|
||||
() =>
|
||||
sweeps.track(() =>
|
||||
sweepProviderByPrefix(
|
||||
worktreeId,
|
||||
deps.localProvider,
|
||||
@@ -150,7 +160,8 @@ export async function killAllProcessesForWorktree(
|
||||
stopPty,
|
||||
deps.onPtyStopped,
|
||||
deps.requirePhysicalStop
|
||||
),
|
||||
)
|
||||
),
|
||||
0,
|
||||
deadline,
|
||||
deps.requirePhysicalStop ? deadlineError : undefined
|
||||
@@ -159,14 +170,15 @@ export async function killAllProcessesForWorktree(
|
||||
deps.includeLocalRegistry === false
|
||||
? Promise.resolve(0)
|
||||
: settleBeforeDeadline(
|
||||
() =>
|
||||
sweeps.track(() =>
|
||||
sweepRegistryForWorktree(
|
||||
worktreeId,
|
||||
deps.localProvider,
|
||||
deadline,
|
||||
stopPty,
|
||||
deps.onPtyStopped
|
||||
),
|
||||
)
|
||||
),
|
||||
0,
|
||||
deadline,
|
||||
deps.requirePhysicalStop ? deadlineError : undefined
|
||||
@@ -180,54 +192,39 @@ export async function killAllProcessesForWorktree(
|
||||
let providerStopped: number
|
||||
let registryStopped: number
|
||||
if (deps.allowUnverifiedStop) {
|
||||
// Why: force goes on to delete files, so every sweep must finish releasing
|
||||
// handles first — Promise.all would abandon the siblings of the first
|
||||
// rejection while they were still inside shutdown(), racing the delete.
|
||||
const settled = await Promise.allSettled([runtimeSweep, providerSweep, registrySweep])
|
||||
const [runtimeSettled, providerSettled, registrySettled] = settled
|
||||
const reasons = settled.flatMap((result) =>
|
||||
result.status === 'rejected' ? [result.reason as unknown] : []
|
||||
const forced = await settleSweepsForForcedRemoval(
|
||||
worktreeId,
|
||||
{ runtime: runtimeSweep, provider: providerSweep, registry: registrySweep },
|
||||
sweeps,
|
||||
deadlineError
|
||||
)
|
||||
if (reasons.length > 0) {
|
||||
// Why (#11960): a sweep that cannot even complete — unresponsive daemon,
|
||||
// dropped SSH channel — fails before the unproven-stop gate below could
|
||||
// offer its escape hatch, so an explicit Force Delete has to survive it.
|
||||
// Report every reason, specific ones first: the shared deadline sentinel
|
||||
// only says that *something* timed out, so leading with it would bury the
|
||||
// provider error that actually explains the failure. This warning is the
|
||||
// only trace of a removal that deleted files without proving a single stop.
|
||||
const detail = [
|
||||
...reasons.filter((candidate) => candidate !== deadlineError),
|
||||
...reasons.filter((candidate) => candidate === deadlineError)
|
||||
]
|
||||
.map(describeError)
|
||||
.join('; ')
|
||||
console.warn(
|
||||
`[worktree-teardown] forcing removal after an incomplete PTY sweep for ${worktreeId} — ${detail}`
|
||||
)
|
||||
// Returning here does skip the verdict below, which could still have named
|
||||
// live PTYs when only one sweep failed — accepted for now because this path
|
||||
// is behind an explicit Force Delete, deletes either way, and clears no
|
||||
// registry rows, so the cost is diagnosability rather than safety.
|
||||
// Report what the surviving sweeps actually stopped rather than a flat zero.
|
||||
return {
|
||||
runtimeStopped: runtimeSettled.status === 'fulfilled' ? runtimeSettled.value.stopped : 0,
|
||||
providerStopped: providerSettled.status === 'fulfilled' ? providerSettled.value : 0,
|
||||
registryStopped: registrySettled.status === 'fulfilled' ? registrySettled.value : 0
|
||||
}
|
||||
if (forced.incomplete) {
|
||||
return forced.stopped
|
||||
}
|
||||
runtimeResult = (runtimeSettled as PromiseFulfilledResult<{ stopped: number }>).value
|
||||
providerStopped = (providerSettled as PromiseFulfilledResult<number>).value
|
||||
registryStopped = (registrySettled as PromiseFulfilledResult<number>).value
|
||||
runtimeResult = { stopped: forced.stopped.runtimeStopped }
|
||||
providerStopped = forced.stopped.providerStopped
|
||||
registryStopped = forced.stopped.registryStopped
|
||||
} else {
|
||||
// Why: without the waiver a rejection aborts the removal and nothing is
|
||||
// deleted, so failing fast is safe — and keeps a dead host reporting
|
||||
// immediately instead of after the full sweep budget.
|
||||
;[runtimeResult, providerStopped, registryStopped] = await Promise.all([
|
||||
runtimeSweep,
|
||||
providerSweep,
|
||||
registrySweep
|
||||
])
|
||||
try {
|
||||
;[runtimeResult, providerStopped, registryStopped] = await Promise.all([
|
||||
runtimeSweep,
|
||||
providerSweep,
|
||||
registrySweep
|
||||
])
|
||||
} catch (error) {
|
||||
// Why (#11960): this rejection is the provider's own wording, which the force
|
||||
// classifier cannot recognise — so the wedge Force Delete exists for was the one
|
||||
// failure that never offered it. Re-word it, keeping the original as the cause.
|
||||
throw deps.requirePhysicalStop && !isUnstoppedPtyRemovalError(describeError(error))
|
||||
? new Error(
|
||||
`${describeFailedPtySweep(worktreeId, error)}. ${WORKTREE_TEARDOWN_FORCE_HINT}`,
|
||||
{ cause: error }
|
||||
)
|
||||
: error
|
||||
}
|
||||
}
|
||||
if (deps.requirePhysicalStop) {
|
||||
const stopResults = await Promise.all(
|
||||
|
||||
@@ -1,5 +1,13 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { getDeleteWorktreeToastCopy } from './delete-worktree-toast'
|
||||
import { classifyWorktreeForceDeleteReason } from '../../../../shared/worktree-removal'
|
||||
|
||||
// Why: production never hands this function a literal reason — the store derives it from
|
||||
// classifyWorktreeForceDeleteReason (store/slices/worktrees.ts). Passing one in would let a
|
||||
// message the classifier rejects still render the force copy, testing a UI that never runs.
|
||||
function toastCopyForRemovalError(worktreeName: string, error: string): unknown {
|
||||
return getDeleteWorktreeToastCopy(worktreeName, classifyWorktreeForceDeleteReason(error), error)
|
||||
}
|
||||
|
||||
describe('getDeleteWorktreeToastCopy', () => {
|
||||
it('uses direct guidance when force delete is available', () => {
|
||||
@@ -14,11 +22,58 @@ describe('getDeleteWorktreeToastCopy', () => {
|
||||
// toast has to actually offer it — a reason of null hides the button entirely.
|
||||
it('uses terminal-teardown guidance when a PTY stop could not be proven', () => {
|
||||
expect(
|
||||
getDeleteWorktreeToastCopy(
|
||||
toastCopyForRemovalError(
|
||||
'feature/foo',
|
||||
'Failed to physically stop every PTY for worktree: repo-1::/w — could not verify these exited: term_a (the process list timed out)'
|
||||
)
|
||||
).toEqual({
|
||||
title: 'Failed to delete workspace feature/foo',
|
||||
description:
|
||||
'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.',
|
||||
isDestructive: false
|
||||
})
|
||||
})
|
||||
|
||||
// Why: Force Delete proceeds on a proven-live PTY too, so the copy must not describe
|
||||
// that as an unconfirmed exit — the user is killing a terminal Orca watched running.
|
||||
it('names the running terminals when verification proved they are still live', () => {
|
||||
expect(
|
||||
toastCopyForRemovalError(
|
||||
'feature/foo',
|
||||
'unstopped-pty',
|
||||
'Failed to physically stop every PTY for worktree: repo-1::/w — still live: term_a'
|
||||
)
|
||||
).toEqual({
|
||||
title: 'Failed to delete workspace feature/foo',
|
||||
description:
|
||||
'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.',
|
||||
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', () => {
|
||||
expect(
|
||||
toastCopyForRemovalError(
|
||||
'feature/foo',
|
||||
'Timed out waiting for physical PTY teardown: repo-1::/w. Retry with force delete (--force) to remove it anyway.'
|
||||
)
|
||||
).toEqual({
|
||||
title: 'Failed to delete workspace feature/foo',
|
||||
description:
|
||||
'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.',
|
||||
isDestructive: false
|
||||
})
|
||||
})
|
||||
|
||||
// Why: the same wedge worded by the provider instead of the deadline — a dropped SSH
|
||||
// channel — must reach the same offer, or the escape hatch misses the case it was for.
|
||||
it('offers force delete when the sweep failed rather than timed out', () => {
|
||||
expect(
|
||||
toastCopyForRemovalError(
|
||||
'feature/foo',
|
||||
'Failed to physically stop every PTY for worktree: repo-1::/w — the terminal sweep failed: SSH channel closed while listing processes. Retry with force delete (--force) to remove it anyway.'
|
||||
)
|
||||
).toEqual({
|
||||
title: 'Failed to delete workspace feature/foo',
|
||||
description:
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
import { translate } from '@/i18n/i18n'
|
||||
import {
|
||||
isLockedWorktreeRemovalError,
|
||||
isProvenLivePtyRemovalError,
|
||||
type WorktreeForceDeleteReason
|
||||
} from '../../../../shared/worktree-removal'
|
||||
export type DeleteWorktreeToastCopy = {
|
||||
@@ -58,10 +59,18 @@ export function getDeleteWorktreeToastCopy(
|
||||
'Failed to delete workspace {{value0}}',
|
||||
{ value0: worktreeName }
|
||||
),
|
||||
description: translate(
|
||||
'auto.components.sidebar.delete.worktree.toast.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.'
|
||||
),
|
||||
// Why: Force Delete proceeds either way, so the copy must say which case this is.
|
||||
// Telling a user "could not confirm" about terminals Orca watched stay alive asks
|
||||
// them to waive a doubt that does not exist, and any running agent's work dies with it.
|
||||
description: isProvenLivePtyRemovalError(error)
|
||||
? translate(
|
||||
'auto.components.sidebar.delete.worktree.toast.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.'
|
||||
)
|
||||
: translate(
|
||||
'auto.components.sidebar.delete.worktree.toast.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.'
|
||||
),
|
||||
isDestructive: false
|
||||
}
|
||||
}
|
||||
|
||||
@@ -4769,7 +4769,8 @@
|
||||
"0899ebdb28": "Git already forgot this workspace, but its directory is still on disk. Use Force Delete to remove the orphaned directory.",
|
||||
"locked": "This workspace is locked by Git. Run git worktree unlock <worktree-path> from its repository, then retry deletion.",
|
||||
"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."
|
||||
"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."
|
||||
}
|
||||
}
|
||||
},
|
||||
|
||||
@@ -1,13 +1,19 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import { classifyWorktreeForceDeleteReason, WORKTREE_TEARDOWN_FORCE_HINT } from './worktree-removal'
|
||||
import {
|
||||
classifyWorktreeForceDeleteReason,
|
||||
isProvenLivePtyRemovalError,
|
||||
UNSTOPPED_PTY_REMOVAL_PREFIX,
|
||||
WORKTREE_TEARDOWN_FORCE_HINT,
|
||||
WORKTREE_TEARDOWN_TIMEOUT_PREFIX
|
||||
} from './worktree-removal'
|
||||
|
||||
const liveError = `${UNSTOPPED_PTY_REMOVAL_PREFIX} repo-1::/w — still live: term_a. ${WORKTREE_TEARDOWN_FORCE_HINT}`
|
||||
const unverifiableError = `${UNSTOPPED_PTY_REMOVAL_PREFIX} repo-1::/w — could not verify these exited: term_a (daemon socket closed). ${WORKTREE_TEARDOWN_FORCE_HINT}`
|
||||
|
||||
// Why (#11960): the desktop Force Delete button renders only when this classifier
|
||||
// returns a reason. The PTY-teardown error tells the user to force-delete, so an
|
||||
// unclassified message leaves them reading advice they cannot act on.
|
||||
describe('classifyWorktreeForceDeleteReason for unstopped PTYs', () => {
|
||||
const liveError = `Failed to physically stop every PTY for worktree: repo-1::/w — still live: term_a. ${WORKTREE_TEARDOWN_FORCE_HINT}`
|
||||
const unverifiableError = `Failed to physically stop every PTY for worktree: repo-1::/w — could not verify these exited: term_a (daemon socket closed). ${WORKTREE_TEARDOWN_FORCE_HINT}`
|
||||
|
||||
it('offers force for a PTY that is still live', () => {
|
||||
expect(classifyWorktreeForceDeleteReason(liveError)).toBe('unstopped-pty')
|
||||
})
|
||||
@@ -29,7 +35,35 @@ describe('classifyWorktreeForceDeleteReason for unstopped PTYs', () => {
|
||||
expect(classifyWorktreeForceDeleteReason(liveError, false, true)).toBeNull()
|
||||
})
|
||||
|
||||
// Why: a wedged provider rejects the sweep before any per-PTY verdict exists, so the
|
||||
// failure arrives worded as a teardown timeout. The waiver clears it exactly like an
|
||||
// unproven stop, so leaving it unclassified hid the button for the wedge #11960 targeted.
|
||||
it('offers force when the teardown sweep itself timed out', () => {
|
||||
expect(
|
||||
classifyWorktreeForceDeleteReason(
|
||||
`${WORKTREE_TEARDOWN_TIMEOUT_PREFIX} repo-1::/w. ${WORKTREE_TEARDOWN_FORCE_HINT}`
|
||||
)
|
||||
).toBe('unstopped-pty')
|
||||
})
|
||||
|
||||
it('leaves unrelated failures unclassified', () => {
|
||||
expect(classifyWorktreeForceDeleteReason('some other failure')).toBeNull()
|
||||
})
|
||||
})
|
||||
|
||||
// The live verdict escalates the delete toast to "Force Delete will kill them", so it must
|
||||
// come from Orca's own detail — not from the worktree id, which is a user-chosen path.
|
||||
describe('isProvenLivePtyRemovalError', () => {
|
||||
it('reads the verdict Orca actually recorded', () => {
|
||||
expect(isProvenLivePtyRemovalError(liveError)).toBe(true)
|
||||
expect(isProvenLivePtyRemovalError(unverifiableError)).toBe(false)
|
||||
})
|
||||
|
||||
it('does not let a worktree path spell out a live verdict', () => {
|
||||
expect(
|
||||
isProvenLivePtyRemovalError(
|
||||
`${UNSTOPPED_PTY_REMOVAL_PREFIX} repo-1::/Users/dev/still live: notes — could not verify these exited: term_a (daemon socket closed)`
|
||||
)
|
||||
).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
@@ -15,8 +15,34 @@ export type WorktreeForceDeleteReason =
|
||||
| 'missing-registration'
|
||||
| 'unstopped-pty'
|
||||
|
||||
// Why: everything before this separator is the worktree id — a user-chosen filesystem path.
|
||||
// Only the detail after it is Orca's own wording, so verdict matchers anchor on the boundary
|
||||
// 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,
|
||||
// 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:'
|
||||
|
||||
// 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
|
||||
// its own matcher the force affordance stayed hidden for the very case it was added for.
|
||||
export const WORKTREE_TEARDOWN_TIMEOUT_PREFIX = 'Timed out waiting for physical PTY teardown:'
|
||||
|
||||
export function isUnstoppedPtyRemovalError(error: string): boolean {
|
||||
return error.includes(UNSTOPPED_PTY_REMOVAL_PREFIX)
|
||||
return (
|
||||
error.includes(UNSTOPPED_PTY_REMOVAL_PREFIX) || error.includes(WORKTREE_TEARDOWN_TIMEOUT_PREFIX)
|
||||
)
|
||||
}
|
||||
|
||||
/** True only when verification positively observed the PTYs still running. */
|
||||
export function isProvenLivePtyRemovalError(error: string): boolean {
|
||||
return (
|
||||
isUnstoppedPtyRemovalError(error) &&
|
||||
error.includes(`${UNSTOPPED_PTY_DETAIL_SEPARATOR}${UNSTOPPED_PTY_LIVE_DETAIL_PREFIX}`)
|
||||
)
|
||||
}
|
||||
|
||||
export function createLockedWorktreeRemovalError(lockReason?: string): Error {
|
||||
|
||||
Reference in New Issue
Block a user