Fix workspace cleanup removal review findings

- Fail a queued removal that now needs a force the user never approved
  (confirm-time approvedCandidates snapshot compared in preflight)
- Reword the 120s removal timeout to say removal continues in background
- Wire suppressPreservedBranchToast into cleanup removals
- Stop statting a repo after the first activity metadata timeout
- Document the WSL 9P best-effort stat gap; drop unused locale key

Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
brennanb2025
2026-07-09 13:35:24 -07:00
committed by Brennan Benson
co-authored by Orca
parent cfe4976759
commit e82971e34b
12 changed files with 208 additions and 38 deletions
@@ -15,6 +15,14 @@ export function getPersistedWorkspaceCleanupActivityAt(
return Math.max(persistedActivityAt, createdAt)
}
export function resolvePersistedWorkspaceCleanupActivityWorktree(worktree: Worktree): Worktree {
const persistedActivityAt = getPersistedWorkspaceCleanupActivityAt(worktree)
if (persistedActivityAt <= worktree.lastActivityAt) {
return worktree
}
return { ...worktree, lastActivityAt: persistedActivityAt }
}
export async function resolveWorkspaceCleanupActivityWorktree(
repo: Repo,
worktree: Worktree,
@@ -56,6 +64,9 @@ async function resolveWorkspaceCleanupActivityAt(
return Math.max(persistedActivityAt, filesystemActivityAt)
}
// Why: best-effort only. Win32 stat over \\wsl.localhost (9P) can falsely
// report ENOENT (see wslUncDirectoryExists), so a failed stat degrades to the
// persisted activity timestamp instead of blocking or mislabeling the row.
async function getNewestLocalWorktreeStatMtime(
worktreePath: string,
statPath: StatPath,
+14 -3
View File
@@ -15,6 +15,7 @@ import type {
} from '../../shared/workspace-cleanup'
import {
getPersistedWorkspaceCleanupActivityAt,
resolvePersistedWorkspaceCleanupActivityWorktree,
resolveWorkspaceCleanupActivityWorktree
} from './workspace-cleanup-activity'
import {
@@ -138,13 +139,21 @@ async function scanRepoWorkspaces(
: mergedWorktrees.filter((worktree) =>
shouldResolveBroadWorkspaceCleanupActivity(repoIsFolder, worktree, scannedAt)
)
// Why: fs stat has no cancellation, so on a hung network/WSL mount every
// timed-out row would abandon more threadpool work. After the first timeout,
// stop statting this repo and use persisted activity only.
let activityStatsUnavailable = false
const candidatesWithSkipped = await mapWorkspaceCleanupWithConcurrency(
candidateWorktrees,
WORKTREE_SCAN_CONCURRENCY,
async (worktree) => {
// Why: externally-created worktrees can miss Orca activity stamps; local
// filesystem metadata is a conservative guard before suggesting deletion.
const worktreeWithActivity = await resolveCleanupActivityWithTimeout(repo, worktree)
const worktreeWithActivity = activityStatsUnavailable
? resolvePersistedWorkspaceCleanupActivityWorktree(worktree)
: await resolveCleanupActivityWithTimeout(repo, worktree, () => {
activityStatsUnavailable = true
})
if (!targetWorktreeId && !isWorkspaceInactiveForCleanup(worktreeWithActivity, scannedAt)) {
return null
}
@@ -173,7 +182,8 @@ async function scanRepoWorkspaces(
async function resolveCleanupActivityWithTimeout(
repo: Repo,
worktree: Worktree
worktree: Worktree,
onActivityStatsUnavailable: () => void
): Promise<Worktree> {
try {
return await withWorkspaceCleanupTimeout(
@@ -182,8 +192,9 @@ async function resolveCleanupActivityWithTimeout(
'Timed out reading worktree activity.'
)
} catch (error) {
onActivityStatsUnavailable()
console.warn('Workspace cleanup activity scan failed', error)
return worktree
return resolvePersistedWorkspaceCleanupActivityWorktree(worktree)
}
}
+21
View File
@@ -574,6 +574,27 @@ describe('workspace cleanup scan', () => {
})
})
it('stops statting a repo after the first activity metadata timeout', async () => {
listRepoWorktreesMock.mockResolvedValue(
['/repo-a', '/repo-b', '/repo-c', '/repo-d'].map((worktreePath, index) => ({
path: worktreePath,
head: `head-${index}`,
branch: `refs/heads/branch-${index}`,
isBare: false,
isMainWorktree: false
}))
)
lstatMock.mockReturnValue(new Promise(() => undefined))
const scanPromise = scanWorkspaceCleanup(makeStore())
await vi.advanceTimersByTimeAsync(8_000)
const result = await scanPromise
expect(result.candidates).toHaveLength(4)
expect(lstatMock).not.toHaveBeenCalledWith('/repo-d')
expect(lstatMock).not.toHaveBeenCalledWith(path.join('/repo-d', '.git'))
})
it('includes focused remove preflight rows even when they are recent', async () => {
const result = await scanWorkspaceCleanup(
makeStore({
@@ -71,7 +71,9 @@ describe('startWorkspaceCleanupBackgroundRemoval', () => {
onResult
})
expect(removeCandidates).toHaveBeenCalledWith([candidate.worktreeId])
expect(removeCandidates).toHaveBeenCalledWith([candidate.worktreeId], {
approvedCandidates: [candidate]
})
expect(onProgress).toHaveBeenCalledWith({
totalCount: 1,
processedCount: 0,
@@ -114,8 +116,12 @@ describe('startWorkspaceCleanupBackgroundRemoval', () => {
})
await settleBackgroundRemoval()
expect(removeCandidates).toHaveBeenNthCalledWith(1, [first.worktreeId])
expect(removeCandidates).toHaveBeenNthCalledWith(2, [second.worktreeId])
expect(removeCandidates).toHaveBeenNthCalledWith(1, [first.worktreeId], {
approvedCandidates: [first]
})
expect(removeCandidates).toHaveBeenNthCalledWith(2, [second.worktreeId], {
approvedCandidates: [second]
})
expect(onProgress).toHaveBeenLastCalledWith({
totalCount: 2,
processedCount: 2,
@@ -149,8 +155,12 @@ describe('startWorkspaceCleanupBackgroundRemoval', () => {
})
await settleBackgroundRemoval()
expect(removeCandidates).toHaveBeenNthCalledWith(1, [child.worktreeId])
expect(removeCandidates).toHaveBeenNthCalledWith(2, [parent.worktreeId])
expect(removeCandidates).toHaveBeenNthCalledWith(1, [child.worktreeId], {
approvedCandidates: [child]
})
expect(removeCandidates).toHaveBeenNthCalledWith(2, [parent.worktreeId], {
approvedCandidates: [parent]
})
})
it('skips an ancestor after a nested workspace removal fails', async () => {
@@ -182,7 +192,9 @@ describe('startWorkspaceCleanupBackgroundRemoval', () => {
await settleBackgroundRemoval()
expect(removeCandidates).toHaveBeenCalledTimes(1)
expect(removeCandidates).toHaveBeenCalledWith([child.worktreeId])
expect(removeCandidates).toHaveBeenCalledWith([child.worktreeId], {
approvedCandidates: [child]
})
expect(onProgress).toHaveBeenLastCalledWith({
totalCount: 2,
processedCount: 2,
@@ -242,8 +254,12 @@ describe('startWorkspaceCleanupBackgroundRemoval', () => {
})
await settleBackgroundRemoval()
expect(removeCandidates).toHaveBeenNthCalledWith(1, [failedChild.worktreeId])
expect(removeCandidates).toHaveBeenNthCalledWith(2, [unrelatedParent.worktreeId])
expect(removeCandidates).toHaveBeenNthCalledWith(1, [failedChild.worktreeId], {
approvedCandidates: [failedChild]
})
expect(removeCandidates).toHaveBeenNthCalledWith(2, [unrelatedParent.worktreeId], {
approvedCandidates: [unrelatedParent]
})
expect(onResult).toHaveBeenCalledWith({
removedIds: [unrelatedParent.worktreeId],
failures: [{ worktreeId: failedChild.worktreeId, displayName: 'child', message: 'busy' }]
@@ -305,7 +321,8 @@ describe('startWorkspaceCleanupBackgroundRemoval', () => {
{
worktreeId: candidate.worktreeId,
displayName: candidate.displayName,
message: 'Timed out removing alpha.'
message:
'Removing alpha is taking longer than expected. It will keep running in the background.'
}
]
})
@@ -6,6 +6,7 @@ import {
} from '../../../../shared/cross-platform-path'
import type {
WorkspaceCleanupFailure,
WorkspaceCleanupRemoveOptions,
WorkspaceCleanupRemoveResult
} from '@/store/slices/workspace-cleanup'
import { translate } from '@/i18n/i18n'
@@ -21,7 +22,10 @@ export type WorkspaceCleanupRemovalProgress = {
export type WorkspaceCleanupBackgroundRemovalArgs = {
candidates: readonly WorkspaceCleanupCandidate[]
removeCandidates: (worktreeIds: readonly string[]) => Promise<WorkspaceCleanupRemoveResult>
removeCandidates: (
worktreeIds: readonly string[],
options?: WorkspaceCleanupRemoveOptions
) => Promise<WorkspaceCleanupRemoveResult>
onProgress: (progress: WorkspaceCleanupRemovalProgress) => void
onResult?: (result: WorkspaceCleanupRemoveResult) => void
onError?: (error: unknown) => void
@@ -88,7 +92,7 @@ export function startWorkspaceCleanupBackgroundRemoval({
}
try {
const result = await withWorkspaceCleanupRemovalTimeout(
removeCandidates([candidate.worktreeId]),
removeCandidates([candidate.worktreeId], { approvedCandidates: [candidate] }),
candidate,
removalTimeoutMs
)
@@ -172,11 +176,14 @@ async function withWorkspaceCleanupRemovalTimeout(
promise,
new Promise<WorkspaceCleanupRemoveResult>((_resolve, reject) => {
timeout = setTimeout(() => {
// Why: the underlying removal cannot be cancelled from the renderer,
// so the row stays "Deleting" and this message must not claim the
// removal stopped.
reject(
new Error(
translate(
'auto.components.workspace.cleanup.backgroundRemoval.timedOut',
'Timed out removing {{value0}}.',
'Removing {{value0}} is taking longer than expected. It will keep running in the background.',
{ value0: candidate.displayName }
)
)
+3 -3
View File
@@ -210,7 +210,8 @@
},
"workspace": {
"cleanup": {
"9d6e531da6": "Workspace no longer exists."
"9d6e531da6": "Workspace no longer exists.",
"changedSinceConfirmation": "Workspace changed after confirmation. Refresh to review it before removing."
}
},
"worktrees": {
@@ -2340,7 +2341,6 @@
"contextFilter": "Context",
"sortBy": "Sort by",
"sortDirection": "Direction",
"e50c4da067": "Deleting",
"1d3503357d": "You can close this and come back while deletion continues.",
"4c2990886e": "{{value0}}/{{value1}} deleted",
"86ba852118": "{{value0}}, {{value1}} failed",
@@ -2357,7 +2357,7 @@
"failed": "Workspaces not removed: {{value0}}",
"error": "Workspace cleanup failed",
"skippedAncestor": "Skipped because a nested workspace could not be removed.",
"timedOut": "Timed out removing {{value0}}."
"timedOut": "Removing {{value0}} is taking longer than expected. It will keep running in the background."
},
"candidateRow": {
"gitLabel": "Git",
+3 -3
View File
@@ -210,7 +210,8 @@
},
"workspace": {
"cleanup": {
"9d6e531da6": "El espacio de trabajo ya no existe."
"9d6e531da6": "El espacio de trabajo ya no existe.",
"changedSinceConfirmation": "El espacio de trabajo cambió después de la confirmación. Actualiza para revisarlo antes de eliminarlo."
}
},
"worktrees": {
@@ -2340,7 +2341,6 @@
"contextFilter": "Contexto",
"sortBy": "Ordenar por",
"sortDirection": "Dirección",
"e50c4da067": "Eliminando",
"1d3503357d": "Puedes cerrar esto y volver mientras la eliminación continúa.",
"4c2990886e": "{{value0}}/{{value1}} eliminados",
"86ba852118": "{{value0}}, {{value1}} fallidos",
@@ -2357,7 +2357,7 @@
"failed": "Espacios de trabajo no eliminados: {{value0}}",
"error": "Error en la limpieza del espacio de trabajo",
"skippedAncestor": "Se omitió porque no se pudo eliminar un espacio de trabajo anidado.",
"timedOut": "Se agotó el tiempo al eliminar {{value0}}."
"timedOut": "La eliminación de {{value0}} está tardando más de lo esperado. Continuará ejecutándose en segundo plano."
},
"candidateRow": {
"gitLabel": "Git",
+3 -3
View File
@@ -210,7 +210,8 @@
},
"workspace": {
"cleanup": {
"9d6e531da6": "ワークスペースはもう存在しません。"
"9d6e531da6": "ワークスペースはもう存在しません。",
"changedSinceConfirmation": "確認後にワークスペースが変更されました。削除する前に更新して確認してください。"
}
},
"worktrees": {
@@ -2340,7 +2341,6 @@
"contextFilter": "コンテクスト",
"sortBy": "並べ替え",
"sortDirection": "方向",
"e50c4da067": "削除中",
"1d3503357d": "削除の続行中にこれを閉じて後で戻れます。",
"4c2990886e": "{{value0}}/{{value1}} 件を削除済み",
"86ba852118": "{{value0}}、{{value1}} 件が失敗",
@@ -2357,7 +2357,7 @@
"failed": "削除できなかったワークスペース: {{value0}}",
"error": "ワークスペースのクリーンアップに失敗しました",
"skippedAncestor": "ネストされたワークスペースを削除できなかったためスキップしました。",
"timedOut": "{{value0}} の削除がタイムアウトしました。"
"timedOut": "{{value0}} の削除に予想より時間がかかっています。バックグラウンドで継続されます。"
},
"candidateRow": {
"gitLabel": "Git",
+3 -3
View File
@@ -210,7 +210,8 @@
},
"workspace": {
"cleanup": {
"9d6e531da6": "워크스페이스가 더 이상 존재하지 않습니다."
"9d6e531da6": "워크스페이스가 더 이상 존재하지 않습니다.",
"changedSinceConfirmation": "확인 후 워크스페이스가 변경되었습니다. 제거하기 전에 새로 고쳐 검토하세요."
}
},
"worktrees": {
@@ -2340,7 +2341,6 @@
"contextFilter": "문맥",
"sortBy": "정렬 기준",
"sortDirection": "방향",
"e50c4da067": "삭제 중",
"1d3503357d": "삭제가 계속되는 동안 이 창을 닫고 나중에 돌아올 수 있습니다.",
"4c2990886e": "{{value0}}/{{value1}}개 삭제됨",
"86ba852118": "{{value0}}, {{value1}}개 실패",
@@ -2357,7 +2357,7 @@
"failed": "삭제되지 않은 워크스페이스: {{value0}}",
"error": "워크스페이스 정리 실패",
"skippedAncestor": "중첩된 워크스페이스를 제거할 수 없어 건너뛰었습니다.",
"timedOut": "{{value0}} 삭제 시간이 초과되었습니다."
"timedOut": "{{value0}} 제거가 예상보다 오래 걸리고 있습니다. 백그라운드에서 계속 실행됩니다."
},
"candidateRow": {
"gitLabel": "Git",
+3 -3
View File
@@ -210,7 +210,8 @@
},
"workspace": {
"cleanup": {
"9d6e531da6": "工作区不再存在。"
"9d6e531da6": "工作区不再存在。",
"changedSinceConfirmation": "工作区在确认后发生了更改。请刷新并在删除前重新检查。"
}
},
"worktrees": {
@@ -2340,7 +2341,6 @@
"contextFilter": "上下文",
"sortBy": "排序依据",
"sortDirection": "方向",
"e50c4da067": "正在删除",
"1d3503357d": "删除继续期间,你可以关闭此窗口并稍后回来。",
"4c2990886e": "已删除 {{value0}}/{{value1}}",
"86ba852118": "{{value0}},{{value1}} 个失败",
@@ -2357,7 +2357,7 @@
"failed": "未删除工作区:{{value0}}",
"error": "工作区清理失败",
"skippedAncestor": "由于无法删除嵌套工作区,已跳过。",
"timedOut": "删除 {{value0}} 超时。"
"timedOut": "删除 {{value0}} 的时间比预期长。它将在后台继续运行。"
},
"candidateRow": {
"gitLabel": "Git",
@@ -767,7 +767,9 @@ describe('workspace cleanup viewed rows', () => {
'repo-a::/repo/parent',
'repo-c::/other'
])
expect(removeWorktree).toHaveBeenCalledWith('repo-c::/other', true)
expect(removeWorktree).toHaveBeenCalledWith('repo-c::/other', true, {
suppressPreservedBranchToast: true
})
expect(store.getState().workspaceCleanupScan?.candidates).toEqual([])
})
@@ -880,7 +882,9 @@ describe('workspace cleanup viewed rows', () => {
removedIds: [WORKTREE_ID],
failures: []
})
expect(removeWorktree).toHaveBeenCalledWith(WORKTREE_ID, false)
expect(removeWorktree).toHaveBeenCalledWith(WORKTREE_ID, false, {
suppressPreservedBranchToast: true
})
})
it('defers git checks for locally active workspaces on initial scans', async () => {
@@ -986,7 +990,67 @@ describe('workspace cleanup viewed rows', () => {
failures: []
}
)
expect(removeWorktree).toHaveBeenCalledWith(WORKTREE_ID, false)
expect(removeWorktree).toHaveBeenCalledWith(WORKTREE_ID, false, {
suppressPreservedBranchToast: true
})
})
it('fails a queued removal that now needs a force the user never approved', async () => {
const approvedCandidate = makeCandidate()
const dirtySinceConfirmation = makeCandidate({
tier: 'review',
blockers: ['dirty-files'],
git: { clean: false, upstreamAhead: 0, upstreamBehind: 0, checkedAt: NOW }
})
const scan = vi.fn().mockResolvedValue({
scannedAt: NOW,
candidates: [dirtySinceConfirmation],
errors: []
} satisfies WorkspaceCleanupScanResult)
installWorkspaceCleanupApi(scan)
const removeWorktree = vi.fn().mockResolvedValue({ ok: true })
const store = createCleanupTestStore(removeWorktree)
await expect(
store.getState().removeWorkspaceCleanupCandidates([WORKTREE_ID], {
approvedCandidates: [approvedCandidate]
})
).resolves.toEqual({
removedIds: [],
failures: [
{
worktreeId: WORKTREE_ID,
displayName: 'old-workspace',
message: 'Workspace changed after confirmation. Refresh to review it before removing.'
}
]
})
expect(removeWorktree).not.toHaveBeenCalled()
})
it('still force-removes rows whose approved candidate already carried git risk', async () => {
const approvedCandidate = makeCandidate({
tier: 'review',
blockers: ['dirty-files'],
git: { clean: false, upstreamAhead: 0, upstreamBehind: 0, checkedAt: NOW }
})
const scan = vi.fn().mockResolvedValue({
scannedAt: NOW,
candidates: [approvedCandidate],
errors: []
} satisfies WorkspaceCleanupScanResult)
installWorkspaceCleanupApi(scan)
const removeWorktree = vi.fn().mockResolvedValue({ ok: true })
const store = createCleanupTestStore(removeWorktree)
await expect(
store.getState().removeWorkspaceCleanupCandidates([WORKTREE_ID], {
approvedCandidates: [approvedCandidate]
})
).resolves.toEqual({ removedIds: [WORKTREE_ID], failures: [] })
expect(removeWorktree).toHaveBeenCalledWith(WORKTREE_ID, true, {
suppressPreservedBranchToast: true
})
})
it('protects old workspaces when an agent process is still foregrounded', async () => {
@@ -35,6 +35,12 @@ export type WorkspaceCleanupRemoveResult = {
failures: WorkspaceCleanupFailure[]
}
export type WorkspaceCleanupRemoveOptions = {
// Why: rows are removed long after the confirm click; the confirm-time
// candidate records how much git risk the user actually approved.
approvedCandidates?: readonly WorkspaceCleanupCandidate[]
}
type WorkspaceCleanupViewedCandidate = {
viewedAt: number
fingerprint: string
@@ -55,7 +61,8 @@ export type WorkspaceCleanupSlice = {
) => Promise<void>
resetWorkspaceCleanupDismissals: () => Promise<void>
removeWorkspaceCleanupCandidates: (
worktreeIds: readonly string[]
worktreeIds: readonly string[],
options?: WorkspaceCleanupRemoveOptions
) => Promise<WorkspaceCleanupRemoveResult>
}
@@ -285,14 +292,22 @@ export const createWorkspaceCleanupSlice: StateCreator<AppState, [], [], Workspa
await window.api.workspaceCleanup.clearDismissals()
},
removeWorkspaceCleanupCandidates: async (worktreeIds) => {
removeWorkspaceCleanupCandidates: async (worktreeIds, options) => {
const removedIds: string[] = []
const failures: WorkspaceCleanupFailure[] = []
const approvedCandidatesByWorktreeId = new Map(
(options?.approvedCandidates ?? []).map((candidate) => [candidate.worktreeId, candidate])
)
const preflights = await mapWithConcurrency(
worktreeIds,
WORKSPACE_CLEANUP_PREFLIGHT_CONCURRENCY,
(worktreeId) => preflightWorkspaceCleanupCandidate(worktreeId, get)
(worktreeId) =>
preflightWorkspaceCleanupCandidate(
worktreeId,
get,
approvedCandidatesByWorktreeId.get(worktreeId)
)
)
const candidatesToRemove: WorkspaceCleanupCandidate[] = []
@@ -309,7 +324,10 @@ export const createWorkspaceCleanupSlice: StateCreator<AppState, [], [], Workspa
for (const candidate of [...candidatesToRemove].sort((a, b) => b.path.length - a.path.length)) {
const result = await get().removeWorktree(
candidate.worktreeId,
shouldForceWorkspaceCleanupRemoval(candidate)
shouldForceWorkspaceCleanupRemoval(candidate),
// Why: cleanup reports outcomes in its own summary toasts; per-row
// preserved-branch warnings would stack one toast per removed row.
{ suppressPreservedBranchToast: true }
)
if (result.ok) {
removedIds.push(candidate.worktreeId)
@@ -801,7 +819,8 @@ function applyDismissal(
async function preflightWorkspaceCleanupCandidate(
worktreeId: string,
getState: () => AppState
getState: () => AppState,
approvedCandidate?: WorkspaceCleanupCandidate
): Promise<
| { ok: true; candidate: WorkspaceCleanupCandidate }
| { ok: false; failure: WorkspaceCleanupFailure }
@@ -835,6 +854,26 @@ async function preflightWorkspaceCleanupCandidate(
}
}
}
// Why: this row may be removed minutes after the confirm click. If it now
// needs a force removal the user never approved (new dirt, unpushed work,
// or a git error since confirmation), fail it instead of force-deleting.
if (
approvedCandidate &&
shouldForceWorkspaceCleanupRemoval(candidate) &&
!shouldForceWorkspaceCleanupRemoval(approvedCandidate)
) {
return {
ok: false,
failure: {
worktreeId,
displayName: candidate.displayName,
message: translate(
'auto.store.slices.workspace.cleanup.changedSinceConfirmation',
'Workspace changed after confirmation. Refresh to review it before removing.'
)
}
}
}
return { ok: true, candidate }
}