diff --git a/config/scripts/ci-shard-timings.json b/config/scripts/ci-shard-timings.json index 1e855befdc57..38b5e3af120a 100644 --- a/config/scripts/ci-shard-timings.json +++ b/config/scripts/ci-shard-timings.json @@ -1788,7 +1788,6 @@ "src/main/git/worktree-create-preparation-real-wsl.test.ts": 271, "src/main/git/worktree-created-description-real-git.test.ts": 1340, "src/main/git/worktree-created-disk-witness.test.ts": 149, - "src/main/git/worktree-deferred-removal-real-git.test.ts": 587, "src/main/git/worktree-diff-stamp-guest-gitdir.test.ts": 40, "src/main/git/worktree-diff-stamp-host-paths.test.ts": 55, "src/main/git/worktree-git-capabilities.test.ts": 248, @@ -1805,6 +1804,7 @@ "src/main/git/worktree-preparation-base-oid.test.ts": 99, "src/main/git/worktree-preparation-cancel-latency.bench.test.ts": 248, "src/main/git/worktree-removal-large-tree.bench.test.ts": 291, + "src/main/git/worktree-removal-real-git.test.ts": 5000, "src/main/git/worktree-remove-branch-deletion.test.ts": 194, "src/main/git/worktree-scan-cache-annotation-reuse.test.ts": 241, "src/main/git/worktree-scan-cache-sharing.test.ts": 284, diff --git a/src/cli/handlers/worktree.ts b/src/cli/handlers/worktree.ts index 3541beef20b5..f74f6df79337 100644 --- a/src/cli/handlers/worktree.ts +++ b/src/cli/handlers/worktree.ts @@ -304,6 +304,10 @@ export const WORKTREE_HANDLERS: Record = { }) printHookWarning(result.result, json) printPreservedBranchWarning(result.result, json) - printResult(result, json, (value) => `removed: ${value.removed}`) + printResult(result, json, (value) => + value.removing + ? `removed: ${value.removed}\nOrca is still deleting the checkout in the background.` + : `removed: ${value.removed}` + ) } } diff --git a/src/cli/index.test.ts b/src/cli/index.test.ts index bed4b9f8ab99..2a6c014b984e 100644 --- a/src/cli/index.test.ts +++ b/src/cli/index.test.ts @@ -120,6 +120,20 @@ describe('command aliases dispatch to the canonical handler', () => { ) }) + it('says the checkout is still being deleted when the host accepted a background removal', async () => { + queueFixtures( + callMock, + okFixture('req_show', { worktree: { hostId: 'local' } }), + okFixture('req', { removed: true, removing: true }) + ) + + await main(['worktree', 'rm', '--worktree', 'id:wt-1'], '/tmp/repo') + + expect(logSpy).toHaveBeenCalledWith( + 'removed: true\nOrca is still deleting the checkout in the background.' + ) + }) + it('fails closed when worktree removal cannot resolve a host', async () => { queueFixtures(callMock, okFixture('req_show', { worktree: { id: 'wt-1' } })) const priorExitCode = process.exitCode diff --git a/src/main/git/add-sparse-worktree.test.ts b/src/main/git/add-sparse-worktree.test.ts index 2541b822438e..987fcb5814c7 100644 --- a/src/main/git/add-sparse-worktree.test.ts +++ b/src/main/git/add-sparse-worktree.test.ts @@ -7,26 +7,14 @@ const { translateWslOutputPathsMock, statMock, readFileMock, - resolveGitDirMock, - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock + resolveGitDirMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn(), gitExecFileSyncMock: vi.fn(), translateWslOutputPathsMock: vi.fn((output: string) => output), statMock: vi.fn(), readFileMock: vi.fn(), - resolveGitDirMock: vi.fn(), - moveWorktreeDirectoryToTrashMock: vi.fn(), - restoreWorktreeDirectoryFromTrashMock: vi.fn(), - scheduleWorktreeTrashDeletionMock: vi.fn() -})) - -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrash: restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletion: scheduleWorktreeTrashDeletionMock + resolveGitDirMock: vi.fn() })) vi.mock('./runner', () => ({ @@ -59,11 +47,7 @@ const mockGitCommands = createGitCommandMocker(gitExecFileAsyncMock) const getGitCalls = createGitCallReader(gitExecFileAsyncMock) beforeEach(() => { - resetWorktreeRemovalState({ - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock - }) + resetWorktreeRemovalState() }) describe('addSparseWorktree', () => { diff --git a/src/main/git/command-runner/git-admission-state.ts b/src/main/git/command-runner/git-admission-state.ts index 60b198660cbd..a9a9ce78ddde 100644 --- a/src/main/git/command-runner/git-admission-state.ts +++ b/src/main/git/command-runner/git-admission-state.ts @@ -8,7 +8,8 @@ export const NETWORK_HEADROOM = 1 export const ROUTE_CAP = 2 export const ROUTE_HEADROOM = 1 export const GIT_ADMISSION_AGING_MS = 15_000 -// General 4+2 and network 3+1 are disjoint, so at most ten git children run globally. +// General 4+2 and network 3+1 are disjoint, so at most ten admitted git children run globally; +// worktree deletes bypass admission under their own limit (WORKTREE_DELETE_CONCURRENCY). export const MAX_GIT_CHILDREN = 10 export type AdmissionClass = 'general' | 'network' diff --git a/src/main/git/command-runner/git-exec-admission-exempt.test.ts b/src/main/git/command-runner/git-exec-admission-exempt.test.ts new file mode 100644 index 000000000000..5bbd04cf4479 --- /dev/null +++ b/src/main/git/command-runner/git-exec-admission-exempt.test.ts @@ -0,0 +1,62 @@ +import { EventEmitter } from 'node:events' +import type { ChildProcess } from 'node:child_process' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { execFileMock } = vi.hoisted(() => ({ execFileMock: vi.fn() })) + +vi.mock('node:child_process', async (importOriginal) => ({ + ...(await importOriginal()), + execFile: execFileMock +})) + +import { gitExecFileAsync } from './git-exec-file' +import { + GitAdmissionScheduler, + _gitAdmissionSnapshotForTests, + _resetGitAdmissionForTests +} from './git-subprocess-admission' + +function mockChild(pid: number): ChildProcess { + const child = Object.assign(new EventEmitter(), { + pid, + kill: vi.fn(() => true), + stdin: Object.assign(new EventEmitter(), { end: vi.fn() }), + stdout: new EventEmitter(), + stderr: new EventEmitter() + }) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the exec wrapper only reads pid, kill, stdio and events. + return child as unknown as ChildProcess +} + +describe('worktree deletes and general git admission', () => { + beforeEach(() => { + vi.useFakeTimers() + execFileMock.mockReset() + // The smallest general cap a host can have: deletes holding it would stall every status read. + _resetGitAdmissionForTests(new GitAdmissionScheduler({ generalCap: 2, generalHeadroom: 0 })) + }) + + afterEach(() => { + vi.useRealTimers() + _resetGitAdmissionForTests() + }) + + it('runs a status read while deletes that fill the general cap are still running', async () => { + let pid = 100 + execFileMock.mockImplementation(() => mockChild(pid++)) + + for (const path of ['/wt-a', '/wt-b', '/wt-c']) { + void gitExecFileAsync(['worktree', 'remove', path], { cwd: '/repo', admissionExempt: true }) + } + await vi.advanceTimersByTimeAsync(0) + expect(execFileMock).toHaveBeenCalledTimes(3) + expect(_gitAdmissionSnapshotForTests().budgets.general?.baseUsed ?? 0).toBe(0) + + void gitExecFileAsync(['status', '--porcelain'], { cwd: '/repo' }) + await vi.advanceTimersByTimeAsync(0) + + expect(execFileMock).toHaveBeenCalledTimes(4) + expect(execFileMock.mock.calls[3]?.[1]).toContain('status') + expect(_gitAdmissionSnapshotForTests().queued).toBe(0) + }) +}) diff --git a/src/main/git/command-runner/git-exec-file.ts b/src/main/git/command-runner/git-exec-file.ts index acfeb9f8a3e5..13027d395b25 100644 --- a/src/main/git/command-runner/git-exec-file.ts +++ b/src/main/git/command-runner/git-exec-file.ts @@ -65,13 +65,15 @@ async function gitExecFileAsyncUnlocked( const policy = effectiveOptions.useConfiguredSshCommandForNetwork ? await buildNetworkSshPolicyEnv(effectiveOptions) : { env: nonInteractiveGitEnv(effectiveOptions.env), mode: 'default' as const } - const grant = await acquireGitAdmission({ - args, - cwd: options.cwd, - wslDistro: options.wslDistro, - tier: options.admissionTier, - signal: options.signal - }) + const grant = options.admissionExempt + ? { queueWaitMs: 0, release: () => {} } + : await acquireGitAdmission({ + args, + cwd: options.cwd, + wslDistro: options.wslDistro, + tier: options.admissionTier, + signal: options.signal + }) span?.setAttribute('git.queue_wait_ms', grant.queueWaitMs) const timeoutMs = gitCommandTimeoutMs(args, options.timeout, options.timeoutMsForTest) const terminationState: { current: Promise | null } = { current: null } diff --git a/src/main/git/command-runner/git-exec-options.ts b/src/main/git/command-runner/git-exec-options.ts index 75ced0d30302..93ad47b6520d 100644 --- a/src/main/git/command-runner/git-exec-options.ts +++ b/src/main/git/command-runner/git-exec-options.ts @@ -23,4 +23,6 @@ export type GitExecOptions = { captureWslLoginShellOutput?: boolean /** Scheduler priority for this child; status is the safe default. */ admissionTier?: GitAdmissionTier + /** Skips general admission; only for a caller that bounds its own concurrency (worktree deletes). */ + admissionExempt?: true } diff --git a/src/main/git/remove-worktree-branch-cleanup.test.ts b/src/main/git/remove-worktree-branch-cleanup.test.ts index 6c4885bca75d..6c31b5f393b9 100644 --- a/src/main/git/remove-worktree-branch-cleanup.test.ts +++ b/src/main/git/remove-worktree-branch-cleanup.test.ts @@ -1,5 +1,5 @@ import type * as FsPromises from 'node:fs/promises' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest' const { gitExecFileAsyncMock, @@ -7,26 +7,14 @@ const { translateWslOutputPathsMock, statMock, readFileMock, - resolveGitDirMock, - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock + resolveGitDirMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn(), gitExecFileSyncMock: vi.fn(), translateWslOutputPathsMock: vi.fn((output: string) => output), statMock: vi.fn(), readFileMock: vi.fn(), - resolveGitDirMock: vi.fn(), - moveWorktreeDirectoryToTrashMock: vi.fn(), - restoreWorktreeDirectoryFromTrashMock: vi.fn(), - scheduleWorktreeTrashDeletionMock: vi.fn() -})) - -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrash: restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletion: scheduleWorktreeTrashDeletionMock + resolveGitDirMock: vi.fn() })) vi.mock('./runner', () => ({ @@ -58,12 +46,16 @@ import { forceDeleteLocalBranch, removeWorktree } from './worktree' const mockGitCommands = createGitCommandMocker(gitExecFileAsyncMock) const getGitCalls = createGitCallReader(gitExecFileAsyncMock) +// Why: removal argv carries core.longpaths on Windows; pin a non-Windows default for exact argv. +let platformSpy: MockInstance<() => NodeJS.Platform> + beforeEach(() => { - resetWorktreeRemovalState({ - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock - }) + resetWorktreeRemovalState() + platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') +}) + +afterEach(() => { + platformSpy.mockRestore() }) describe('removeWorktree', () => { @@ -460,4 +452,32 @@ branch refs/heads/feature/test ) expect(getGitCalls()).not.toContain('git config --remove-section branch.feature/test') }) + + it('runs branch cleanup for one repo one removal at a time, not the checkout deletes', async () => { + let releaseFirstBranchDelete: () => void = () => {} + const firstBranchDelete = new Promise((resolve) => { + releaseFirstBranchDelete = resolve + }) + gitExecFileAsyncMock.mockImplementation(async (args: string[]) => { + if (args.join(' ') === 'branch -d -- feature/a') { + await firstBranchDelete + } + return { stdout: '', stderr: '' } + }) + + const first = removeWorktree('/repo', '/repo-a', false, { + knownRemovedWorktree: { branch: 'refs/heads/feature/a', head: 'aaa', locked: false } + }) + await vi.waitFor(() => expect(getGitCalls()).toContain('git branch -d -- feature/a')) + const second = removeWorktree('/repo', '/repo-b', false, { + knownRemovedWorktree: { branch: 'refs/heads/feature/b', head: 'bbb', locked: false } + }) + await vi.waitFor(() => expect(getGitCalls()).toContain('git worktree remove /repo-b')) + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(getGitCalls()).not.toContain('git branch -d -- feature/b') + + releaseFirstBranchDelete() + await Promise.all([first, second]) + expect(getGitCalls()).toContain('git branch -d -- feature/b') + }) }) diff --git a/src/main/git/remove-worktree-clean-preflight.test.ts b/src/main/git/remove-worktree-clean-preflight.test.ts index 190074ed1517..ebaec39cee43 100644 --- a/src/main/git/remove-worktree-clean-preflight.test.ts +++ b/src/main/git/remove-worktree-clean-preflight.test.ts @@ -7,26 +7,14 @@ const { translateWslOutputPathsMock, statMock, readFileMock, - resolveGitDirMock, - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock + resolveGitDirMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn(), gitExecFileSyncMock: vi.fn(), translateWslOutputPathsMock: vi.fn((output: string) => output), statMock: vi.fn(), readFileMock: vi.fn(), - resolveGitDirMock: vi.fn(), - moveWorktreeDirectoryToTrashMock: vi.fn(), - restoreWorktreeDirectoryFromTrashMock: vi.fn(), - scheduleWorktreeTrashDeletionMock: vi.fn() -})) - -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrash: restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletion: scheduleWorktreeTrashDeletionMock + resolveGitDirMock: vi.fn() })) vi.mock('./runner', () => ({ @@ -50,11 +38,7 @@ import { resetWorktreeRemovalState } from './remove-worktree-test-harness' import { assertWorktreeCleanForRemoval, WORKTREE_REMOVAL_PREFLIGHT_TIMEOUT_MS } from './worktree' beforeEach(() => { - resetWorktreeRemovalState({ - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock - }) + resetWorktreeRemovalState() }) describe('assertWorktreeCleanForRemoval', () => { diff --git a/src/main/git/remove-worktree-test-harness.ts b/src/main/git/remove-worktree-test-harness.ts index 1f8be9b7fcc9..509c6b580eae 100644 --- a/src/main/git/remove-worktree-test-harness.ts +++ b/src/main/git/remove-worktree-test-harness.ts @@ -80,23 +80,11 @@ export function mockSparseCheckoutEnabledConfig(readFileMock: Mock): void { }) } -export type WorktreeTrashMocks = { - moveWorktreeDirectoryToTrashMock: Mock - restoreWorktreeDirectoryFromTrashMock: Mock - scheduleWorktreeTrashDeletionMock: Mock -} - -/** Per-test reset shared by every worktree suite: caches cleared, trash rename unavailable. */ -export function resetWorktreeRemovalState(trashMocks: WorktreeTrashMocks): void { +/** Per-test reset shared by every worktree suite: caches cleared. */ +export function resetWorktreeRemovalState(): void { clearGitCapabilityStateForTests() _resetWorktreeScanCacheForTests() __resetSparseCheckoutStateCacheForTests() - // Default: the checkout cannot be renamed aside, so removal deletes it in place. - trashMocks.moveWorktreeDirectoryToTrashMock.mockReset() - trashMocks.moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined) - trashMocks.restoreWorktreeDirectoryFromTrashMock.mockReset() - trashMocks.restoreWorktreeDirectoryFromTrashMock.mockResolvedValue(true) - trashMocks.scheduleWorktreeTrashDeletionMock.mockReset() } export type WorktreeGitMocks = { diff --git a/src/main/git/remove-worktree.test.ts b/src/main/git/remove-worktree.test.ts index 006ee3087bb1..d2246da56ad0 100644 --- a/src/main/git/remove-worktree.test.ts +++ b/src/main/git/remove-worktree.test.ts @@ -1,5 +1,9 @@ +import { existsSync } from 'node:fs' import type * as FsPromises from 'node:fs/promises' -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { mkdir, mkdtemp, rm } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest' const { gitExecFileAsyncMock, @@ -7,26 +11,14 @@ const { translateWslOutputPathsMock, statMock, readFileMock, - resolveGitDirMock, - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock + resolveGitDirMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn(), gitExecFileSyncMock: vi.fn(), translateWslOutputPathsMock: vi.fn((output: string) => output), statMock: vi.fn(), readFileMock: vi.fn(), - resolveGitDirMock: vi.fn(), - moveWorktreeDirectoryToTrashMock: vi.fn(), - restoreWorktreeDirectoryFromTrashMock: vi.fn(), - scheduleWorktreeTrashDeletionMock: vi.fn() -})) - -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrash: restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletion: scheduleWorktreeTrashDeletionMock + resolveGitDirMock: vi.fn() })) vi.mock('./runner', () => ({ @@ -53,7 +45,7 @@ import { resetWorktreeRemovalState } from './remove-worktree-test-harness' -import { removeWorktree, WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS } from './worktree' +import { removeWorktree } from './worktree' import { __getSparseCheckoutStateCacheSizeForTests, detectSparseCheckoutCached @@ -62,12 +54,17 @@ import { const mockGitCommands = createGitCommandMocker(gitExecFileAsyncMock) const getGitCalls = createGitCallReader(gitExecFileAsyncMock) +// Why: removal argv carries core.longpaths on Windows, so pin a non-Windows default or the +// exact-argv assertions below fail for a maintainer running vitest on Windows. +let platformSpy: MockInstance<() => NodeJS.Platform> + beforeEach(() => { - resetWorktreeRemovalState({ - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock - }) + resetWorktreeRemovalState() + platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') +}) + +afterEach(() => { + platformSpy.mockRestore() }) describe('removeWorktree', () => { @@ -223,8 +220,6 @@ branch refs/heads/main const calls = getGitCalls() expect(calls).toEqual([ 'git worktree list --porcelain -z', - // The cleanliness probe that decides whether the checkout may be renamed aside. - 'git status --porcelain --untracked-files=all', 'git worktree remove /repo-feature', 'git branch -d -- feature/test', 'git worktree prune', @@ -232,7 +227,7 @@ branch refs/heads/main ]) }) - it('renames the checkout aside and deregisters the missing path', async () => { + it('lets Git delete the checkout inline with no deadline', async () => { mockGitCommands({ 'git worktree list --porcelain': { stdout: `worktree /repo @@ -251,24 +246,20 @@ branch refs/heads/main ` } }) - moveWorktreeDirectoryToTrashMock.mockResolvedValue('/trash/wt-1-abcdef01') await removeWorktree('/repo', '/repo-feature') - const calls = getGitCalls() - expect(moveWorktreeDirectoryToTrashMock).toHaveBeenCalledWith('/repo-feature') - expect(scheduleWorktreeTrashDeletionMock).toHaveBeenCalledWith('/trash/wt-1-abcdef01') - expect(calls).toContain('git worktree remove --force /repo-feature') - expect(gitExecFileAsyncMock).toHaveBeenCalledWith( - ['worktree', 'remove', '--force', '/repo-feature'], - { cwd: '/repo', timeout: WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS } - ) - expect(calls).not.toContain('git worktree remove /repo-feature') - expect(calls).not.toContain('git worktree prune') - expect(calls).toContain('git branch -d -- feature/test') + // A large checkout takes Git 30 s or more to delete; any timeout here would cut that short. + // Exempt from general admission: deletes queue under their own limit instead. + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'remove', '/repo-feature'], { + cwd: '/repo', + admissionExempt: true + }) + expect(getGitCalls()).not.toContain('git worktree prune') + expect(getGitCalls()).toContain('git branch -d -- feature/test') }) - it('prunes the registration when deregistering the moved checkout fails', async () => { + it('lets a quit stop only the checkout delete, not the branch cleanup that holds ref locks', async () => { mockGitCommands({ 'git worktree list --porcelain': { stdout: `worktree /repo @@ -285,88 +276,76 @@ branch refs/heads/feature/test HEAD abc123 branch refs/heads/main ` - }, - 'git worktree remove --force /repo-feature': { - error: new Error('fatal: validation failed, cannot remove working directory') } }) - moveWorktreeDirectoryToTrashMock.mockResolvedValue('/trash/wt-2-abcdef02') - - await removeWorktree('/repo', '/repo-feature') + const stop = new AbortController() - expect(getGitCalls()).toContain('git worktree prune') - expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { - cwd: '/repo', - timeout: WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS + await removeWorktree('/repo', '/repo-feature', false, { + checkoutDeleteSignal: stop.signal }) - expect( - gitExecFileAsyncMock.mock.calls.filter( - ([args, options]) => - args.join(' ') === 'worktree list --porcelain -z' && - options.timeout === WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS - ) - ).toHaveLength(2) - expect(scheduleWorktreeTrashDeletionMock).toHaveBeenCalledWith('/trash/wt-2-abcdef02') - expect(restoreWorktreeDirectoryFromTrashMock).not.toHaveBeenCalled() - }) - it('restores the moved checkout and removes in place when the registration survives pruning', async () => { - mockGitCommands({ - 'git worktree list --porcelain': { - stdout: `worktree /repo -HEAD abc123 -branch refs/heads/main + const optionsOf = (command: string): { signal?: AbortSignal } | undefined => + gitExecFileAsyncMock.mock.calls.find((call) => call[0].join(' ') === command)?.[1] + expect(optionsOf('worktree remove /repo-feature')?.signal).toBe(stop.signal) + expect(optionsOf('branch -d -- feature/test')).toBeDefined() + expect(optionsOf('branch -d -- feature/test')?.signal).toBeUndefined() + }) -worktree /repo-feature -HEAD def456 -branch refs/heads/feature/test -` - }, - 'git worktree remove --force /repo-feature': { - error: new Error('fatal: validation failed, cannot remove working directory') - } - }) - moveWorktreeDirectoryToTrashMock.mockResolvedValue('/trash/wt-3-abcdef03') + it('never lets Git run an inherited yes/no prompt program during the delete', async () => { + vi.stubEnv('GIT_ASK_YESNO', '/usr/local/bin/prompt') + try { + mockGitCommands({}) - await removeWorktree('/repo', '/repo-feature') + await removeWorktree('/repo', '/repo-feature', false, { + knownRemovedWorktree: { branch: '', head: '', locked: false } + }) - expect(restoreWorktreeDirectoryFromTrashMock).toHaveBeenCalledWith( - '/trash/wt-3-abcdef03', - '/repo-feature' - ) - expect(scheduleWorktreeTrashDeletionMock).not.toHaveBeenCalled() - expect(getGitCalls()).toContain('git worktree remove /repo-feature') + const [, removeOptions] = gitExecFileAsyncMock.mock.calls[0] + expect(removeOptions.env).toBeDefined() + expect(removeOptions.env).not.toHaveProperty('GIT_ASK_YESNO') + } finally { + vi.unstubAllEnvs() + } }) - it('removes in place when the checkout cannot be renamed aside', async () => { - mockGitCommands({ - 'git worktree list --porcelain': { - stdout: `worktree /repo -HEAD abc123 -branch refs/heads/main + it('deletes what Git left of the checkout after a successful remove', async () => { + const scratch = await mkdtemp(join(tmpdir(), 'orca-remove-residue-')) + const worktreePath = join(scratch, 'feature') + // Git for Windows leaves junctions and the directories holding them behind yet exits 0. + await mkdir(join(worktreePath, 'node_modules', 'left-behind'), { recursive: true }) + try { + mockGitCommands({}) -worktree /repo-feature -HEAD def456 -branch refs/heads/feature/test -` - }, - 'git worktree list --porcelain#2': { - stdout: `worktree /repo -HEAD abc123 -branch refs/heads/main -` - } - }) - // Windows open handles and cross-volume renames both surface as an unavailable rename. - moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined) + await removeWorktree('/repo', worktreePath, false, { + knownRemovedWorktree: { branch: '', head: '', locked: false } + }) - await removeWorktree('/repo', '/repo-feature') + expect(getGitCalls()).toEqual([`git worktree remove ${worktreePath}`]) + expect(existsSync(worktreePath)).toBe(false) + } finally { + await rm(scratch, { recursive: true, force: true }) + } + }) - expect(getGitCalls()).toContain('git worktree remove /repo-feature') - expect(scheduleWorktreeTrashDeletionMock).not.toHaveBeenCalled() + it('leaves a WSL checkout that Git reported removed to the distro', async () => { + const scratch = await mkdtemp(join(tmpdir(), 'orca-remove-residue-wsl-')) + const worktreePath = join(scratch, 'feature') + await mkdir(worktreePath, { recursive: true }) + try { + mockGitCommands({}) + + await removeWorktree('/repo', worktreePath, false, { + wslDistro: 'Ubuntu', + knownRemovedWorktree: { branch: '', head: '', locked: false } + }) + + expect(existsSync(worktreePath)).toBe(true) + } finally { + await rm(scratch, { recursive: true, force: true }) + } }) - it('never renames a dirty checkout aside', async () => { + it('leaves a dirty checkout to Git to refuse', async () => { mockGitCommands({ 'git worktree list --porcelain': { stdout: `worktree /repo @@ -387,7 +366,7 @@ branch refs/heads/feature/test await expect(removeWorktree('/repo', '/repo-feature')).rejects.toThrow( 'contains modified or untracked files' ) - expect(moveWorktreeDirectoryToTrashMock).not.toHaveBeenCalled() + expect(getGitCalls()).not.toContain('git worktree remove --force /repo-feature') }) it('deletes WSL-hosted checkouts in place inside the distro', async () => { @@ -412,27 +391,20 @@ branch refs/heads/main await removeWorktree('/repo', '/repo-feature', false, { wslDistro: 'Ubuntu' }) - expect(moveWorktreeDirectoryToTrashMock).not.toHaveBeenCalled() expect(getGitCalls()).not.toContain('git status --porcelain --untracked-files=all') expect(getGitCalls()).toContain('git worktree remove /repo-feature') }) - it('does not rename a WSL checkout configured for a native Windows repo', async () => { - const originalPlatform = process.platform + it('removes a WSL checkout configured for a native Windows repo with Git alone', async () => { const worktreePath = '\\\\wsl.localhost\\Ubuntu\\home\\dev\\feature' - Object.defineProperty(process, 'platform', { configurable: true, value: 'win32' }) - try { - mockGitCommands({}) + platformSpy.mockReturnValue('win32') + mockGitCommands({}) - await removeWorktree('C:\\repo', worktreePath, false, { - knownRemovedWorktree: { branch: '', head: '', locked: false } - }) + await removeWorktree('C:\\repo', worktreePath, false, { + knownRemovedWorktree: { branch: '', head: '', locked: false } + }) - expect(moveWorktreeDirectoryToTrashMock).not.toHaveBeenCalled() - expect(getGitCalls()).toEqual([`git worktree remove ${worktreePath}`]) - } finally { - Object.defineProperty(process, 'platform', { configurable: true, value: originalPlatform }) - } + expect(getGitCalls()).toEqual([`git -c core.longpaths=true worktree remove ${worktreePath}`]) }) it('passes one --force before the worktree path for dirty-file removal', async () => { @@ -574,10 +546,8 @@ branch refs/heads/feature/test 'git worktree remove failed' ) expect(getGitCalls()).not.toContain('git worktree remove --force /repo-feature') - // Only the pre-rename probe ran: an unrelated failure must not re-prove cleanliness. - expect( - getGitCalls().filter((call) => call === 'git status --porcelain --untracked-files=all') - ).toHaveLength(1) + // An unrelated failure must not re-prove cleanliness. + expect(getGitCalls()).not.toContain('git status --porcelain --untracked-files=all') }) it('rejects a locked worktree with stable app-owned copy before invoking remove', async () => { diff --git a/src/main/git/worktree-add-creation-config.test.ts b/src/main/git/worktree-add-creation-config.test.ts index 15bcb335f03b..58b5202ed9b0 100644 --- a/src/main/git/worktree-add-creation-config.test.ts +++ b/src/main/git/worktree-add-creation-config.test.ts @@ -1,17 +1,13 @@ // addWorktree: checkout creation, branch-base/push.autoSetupRemote config writes, ref qualification. import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest' -const { - gitExecFileAsyncMock, - gitExecFileSyncMock, - translateWslOutputPathsMock, - moveWorktreeDirectoryToTrashMock -} = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - gitExecFileSyncMock: vi.fn(), - translateWslOutputPathsMock: vi.fn((output: string) => output), - moveWorktreeDirectoryToTrashMock: vi.fn() -})) +const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + translateWslOutputPathsMock: vi.fn((output: string) => output) + }) +) vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock, @@ -19,13 +15,6 @@ vi.mock('./runner', () => ({ translateWslOutputPaths: translateWslOutputPathsMock })) -// Default: the checkout cannot be renamed aside, so removal deletes it in place. -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined), - restoreWorktreeDirectoryFromTrash: vi.fn().mockResolvedValue(true), - scheduleWorktreeTrashDeletion: vi.fn() -})) - import { addSparseWorktree, addWorktree, WORKTREE_ADD_TIMEOUT_MS } from './worktree' import { registerWorktreeSuiteHooks } from './worktree-test-harness' diff --git a/src/main/git/worktree-add-local-base-refresh.test.ts b/src/main/git/worktree-add-local-base-refresh.test.ts index 1ba2e6c16b88..c5ad0418d279 100644 --- a/src/main/git/worktree-add-local-base-refresh.test.ts +++ b/src/main/git/worktree-add-local-base-refresh.test.ts @@ -1,17 +1,13 @@ // addWorktree: fast-forwarding the local base ref (reset --hard / update-ref) and its safety bailouts. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { - gitExecFileAsyncMock, - gitExecFileSyncMock, - translateWslOutputPathsMock, - moveWorktreeDirectoryToTrashMock -} = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - gitExecFileSyncMock: vi.fn(), - translateWslOutputPathsMock: vi.fn((output: string) => output), - moveWorktreeDirectoryToTrashMock: vi.fn() -})) +const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + translateWslOutputPathsMock: vi.fn((output: string) => output) + }) +) vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock, @@ -19,13 +15,6 @@ vi.mock('./runner', () => ({ translateWslOutputPaths: translateWslOutputPathsMock })) -// Default: the checkout cannot be renamed aside, so removal deletes it in place. -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined), - restoreWorktreeDirectoryFromTrash: vi.fn().mockResolvedValue(true), - scheduleWorktreeTrashDeletion: vi.fn() -})) - import { addWorktree, WORKTREE_ADD_TIMEOUT_MS } from './worktree' import { registerWorktreeSuiteHooks } from './worktree-test-harness' diff --git a/src/main/git/worktree-add-local-base-suggestion.test.ts b/src/main/git/worktree-add-local-base-suggestion.test.ts index 3b449c339f6d..e17581a55f61 100644 --- a/src/main/git/worktree-add-local-base-suggestion.test.ts +++ b/src/main/git/worktree-add-local-base-suggestion.test.ts @@ -1,17 +1,13 @@ // addWorktree: advisory local-base-ref update suggestions when the refresh setting is off. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { - gitExecFileAsyncMock, - gitExecFileSyncMock, - translateWslOutputPathsMock, - moveWorktreeDirectoryToTrashMock -} = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - gitExecFileSyncMock: vi.fn(), - translateWslOutputPathsMock: vi.fn((output: string) => output), - moveWorktreeDirectoryToTrashMock: vi.fn() -})) +const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + translateWslOutputPathsMock: vi.fn((output: string) => output) + }) +) vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock, @@ -19,13 +15,6 @@ vi.mock('./runner', () => ({ translateWslOutputPaths: translateWslOutputPathsMock })) -// Default: the checkout cannot be renamed aside, so removal deletes it in place. -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined), - restoreWorktreeDirectoryFromTrash: vi.fn().mockResolvedValue(true), - scheduleWorktreeTrashDeletion: vi.fn() -})) - import { addWorktree } from './worktree' import { registerWorktreeSuiteHooks } from './worktree-test-harness' diff --git a/src/main/git/worktree-add-pending-removal.test.ts b/src/main/git/worktree-add-pending-removal.test.ts new file mode 100644 index 000000000000..fdb115979246 --- /dev/null +++ b/src/main/git/worktree-add-pending-removal.test.ts @@ -0,0 +1,63 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' + +const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + translateWslOutputPathsMock: vi.fn((output: string) => output) + }) +) + +vi.mock('./runner', () => ({ + gitExecFileAsync: gitExecFileAsyncMock, + gitExecFileSync: gitExecFileSyncMock, + translateWslOutputPaths: translateWslOutputPathsMock +})) + +import { addWorktree } from './worktree' +import { registerWorktreeSuiteHooks } from './worktree-test-harness' +import { + _resetPendingWorktreeRemovalsForTests, + startBackgroundWorktreeRemoval +} from '../worktree-background-removal' + +registerWorktreeSuiteHooks() + +function startDeleting(): void { + void startBackgroundWorktreeRemoval({ + removal: { + worktreeId: 'repo-1::/repo-feature', + repoId: 'repo-1', + repoPath: '/repo', + worktree: { path: '/repo-feature', branch: 'refs/heads/feature/test', head: 'abc' }, + deleteBranch: true, + force: false + }, + run: () => new Promise(() => {}), + publish: () => {} + }) +} + +describe('addWorktree while Orca deletes a checkout in the background', () => { + afterEach(() => { + _resetPendingWorktreeRemovalsForTests() + }) + + it('refuses the same path with a clear message and runs no git', async () => { + startDeleting() + await expect(addWorktree('/repo', '/repo-feature', 'other-branch')).rejects.toThrow( + 'Orca is still deleting the workspace at /repo-feature. Cleanup is pending; try again shortly.' + ) + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('refuses the same branch, which the delete removes when it finishes', async () => { + startDeleting() + await expect( + addWorktree('/repo', '/repo-elsewhere', 'feature/test', undefined, false, false, { + checkoutExistingBranch: true + }) + ).rejects.toThrow('Cleanup is pending; try again shortly.') + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/git/worktree-add-timeout-override.test.ts b/src/main/git/worktree-add-timeout-override.test.ts index 50aa4e59db12..eb67ba880f70 100644 --- a/src/main/git/worktree-add-timeout-override.test.ts +++ b/src/main/git/worktree-add-timeout-override.test.ts @@ -1,17 +1,13 @@ // resolveWorktreeAddTimeoutMs: ORCA_WORKTREE_ADD_TIMEOUT_MS parsing, clamping, and warnings. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { - gitExecFileAsyncMock, - gitExecFileSyncMock, - translateWslOutputPathsMock, - moveWorktreeDirectoryToTrashMock -} = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - gitExecFileSyncMock: vi.fn(), - translateWslOutputPathsMock: vi.fn((output: string) => output), - moveWorktreeDirectoryToTrashMock: vi.fn() -})) +const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + translateWslOutputPathsMock: vi.fn((output: string) => output) + }) +) vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock, @@ -19,13 +15,6 @@ vi.mock('./runner', () => ({ translateWslOutputPaths: translateWslOutputPathsMock })) -// Default: the checkout cannot be renamed aside, so removal deletes it in place. -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined), - restoreWorktreeDirectoryFromTrash: vi.fn().mockResolvedValue(true), - scheduleWorktreeTrashDeletion: vi.fn() -})) - import { resolveWorktreeAddTimeoutMs, WORKTREE_ADD_TIMEOUT_MS } from './worktree' import { registerWorktreeSuiteHooks } from './worktree-test-harness' diff --git a/src/main/git/worktree-add.ts b/src/main/git/worktree-add.ts index 380f3a3cc343..f7658f9c47f3 100644 --- a/src/main/git/worktree-add.ts +++ b/src/main/git/worktree-add.ts @@ -20,6 +20,7 @@ import type { } from './worktree-operation-options' import { gitExecOptions, resolveWorktreeAddTimeoutMs } from './worktree-operation-options' import { bumpWorktreeScanGeneration } from './worktree-scan-cache' +import { assertNoPendingWorktreeRemovalConflict } from '../worktree-background-removal' export type WorktreeAddBaseContext = AddWorktreeResult & { effectiveBase: string @@ -184,6 +185,9 @@ async function performAddWorktree( noCheckout = false, options: AddWorktreeOptions = {} ): Promise { + // Why: Git still owns that path and branch until the background delete finishes; a create now + // would race it, and the branch cleanup that follows would find the branch checked out again. + assertNoPendingWorktreeRemovalConflict(repoPath, { worktreePath, branch }) let localBaseRefRefresh: LocalBaseRefRefreshResult | undefined let localBaseRefUpdateSuggestion: LocalBaseRefUpdateSuggestion | undefined // Why: enable long paths for this Windows checkout without changing user Git config. diff --git a/src/main/git/worktree-deferred-removal-real-git.test.ts b/src/main/git/worktree-deferred-removal-real-git.test.ts deleted file mode 100644 index 06b58b4319ed..000000000000 --- a/src/main/git/worktree-deferred-removal-real-git.test.ts +++ /dev/null @@ -1,164 +0,0 @@ -// Real-binary coverage for deferred worktree deletion: the mocked-runner suite cannot prove that Git -// accepts `worktree remove --force` on a path Orca just renamed away. -import { execFile } from 'node:child_process' -import { existsSync } from 'node:fs' -import { mkdir, mkdtemp, readFile, readdir, realpath, rm, writeFile } from 'node:fs/promises' -import { tmpdir } from 'node:os' -import { join } from 'node:path' -import { promisify } from 'node:util' -import { afterEach, beforeEach, describe, expect, it } from 'vitest' -import { listWorktreesStrict, removeWorktree } from './worktree' -import { isPrunableGitFileWorktree } from '../worktree-prunable-git-file' -import { removeStaleLocalWorktreeRegistration } from '../local-worktree-removal-recovery' -import { - getWorktreeTrashRoot, - isWorktreeTrashEntryName, - sweepStaleWorktreeTrash, - whenWorktreeTrashDeletionsSettled, - WORKTREE_TRASH_DIR_NAME -} from '../worktree-trash' - -const execFileAsync = promisify(execFile) - -let scratchDir = '' -let repoPath = '' -let workspaceRoot = '' -let worktreePath = '' - -async function git(args: string[], cwd: string): Promise { - const { stdout } = await execFileAsync('git', args, { cwd }) - return stdout -} - -beforeEach(async () => { - // realpath: macOS hands out /var/... temp paths while Git reports /private/var/..., and Orca - // matches the worktree it is removing against Git's own list. - scratchDir = await realpath(await mkdtemp(join(tmpdir(), 'orca-deferred-worktree-removal-'))) - repoPath = join(scratchDir, 'repo') - workspaceRoot = join(scratchDir, 'workspaces') - worktreePath = join(workspaceRoot, 'repo', 'feature') - await mkdir(repoPath, { recursive: true }) - await mkdir(join(workspaceRoot, 'repo'), { recursive: true }) - await git(['init', '-q'], repoPath) - await git(['config', 'user.email', 'deferred@example.invalid'], repoPath) - await git(['config', 'user.name', 'Deferred Removal'], repoPath) - await writeFile(join(repoPath, 'seed.txt'), 'seed\n') - // Committed before the worktree exists so its branch stays merged and branch cleanup can run. - await mkdir(join(repoPath, 'node_modules', 'pkg'), { recursive: true }) - await writeFile(join(repoPath, 'node_modules', 'pkg', 'index.js'), 'module.exports = 1\n') - await git(['add', '-A'], repoPath) - await git(['commit', '-qm', 'seed'], repoPath) - await git(['worktree', 'add', '-q', worktreePath, '-b', 'feature'], repoPath) -}) - -afterEach(async () => { - await whenWorktreeTrashDeletionsSettled() - await rm(scratchDir, { recursive: true, force: true }) -}) - -describe('deferred worktree removal against the real Git binary', () => { - it('clears the registration and deletes the renamed checkout in the background', async () => { - const trashRoot = getWorktreeTrashRoot(worktreePath) - - await removeWorktree(repoPath, worktreePath, false, { deleteBranch: false }) - - // The user-visible removal is complete: nothing on disk, nothing registered. - expect(existsSync(worktreePath)).toBe(false) - expect(await git(['worktree', 'list'], repoPath)).not.toContain(worktreePath) - // Only the rename path creates this root, so its presence proves the deletion was deferred. - expect(existsSync(trashRoot)).toBe(true) - expect((await readdir(trashRoot)).every(isWorktreeTrashEntryName)).toBe(true) - - await whenWorktreeTrashDeletionsSettled() - expect(await readdir(trashRoot)).toEqual([]) - }) - - it('leaves sibling worktrees registered', async () => { - const siblingPath = join(workspaceRoot, 'repo', 'sibling') - await git(['worktree', 'add', '-q', siblingPath, '-b', 'sibling'], repoPath) - - await removeWorktree(repoPath, worktreePath, false, { deleteBranch: false }) - - expect(await git(['worktree', 'list'], repoPath)).toContain(siblingPath) - expect(existsSync(siblingPath)).toBe(true) - }) - - it('deletes the branch exactly as the in-place removal did', async () => { - await removeWorktree(repoPath, worktreePath, false) - - expect(await git(['branch', '--list', 'feature'], repoPath)).toBe('') - }) - - it('refuses to move a dirty checkout aside', async () => { - await writeFile(join(worktreePath, 'seed.txt'), 'edited\n') - - await expect(removeWorktree(repoPath, worktreePath, false)).rejects.toThrow() - expect(existsSync(join(worktreePath, 'seed.txt'))).toBe(true) - expect(await git(['worktree', 'list'], repoPath)).toContain(worktreePath) - expect(existsSync(getWorktreeTrashRoot(worktreePath))).toBe(false) - }) - - it('does not rename a malformed registration that points at the checkout git file', async () => { - const markerPath = join(worktreePath, '.git') - const marker = await readFile(markerPath, 'utf8') - const adminPath = marker.trim().replace(/^gitdir: /, '') - await writeFile(join(adminPath, 'gitdir'), `${join(markerPath, '.git')}\n`) - await writeFile(join(worktreePath, 'untracked.txt'), 'keep this work\n') - - await expect( - removeWorktree(repoPath, markerPath, true, { deleteBranch: false }) - ).rejects.toThrow() - await whenWorktreeTrashDeletionsSettled() - - expect(await readFile(markerPath, 'utf8')).toBe(marker) - expect(await readFile(join(worktreePath, 'untracked.txt'), 'utf8')).toBe('keep this work\n') - expect(await git(['branch', '--list', 'feature'], repoPath)).toContain('feature') - expect(existsSync(getWorktreeTrashRoot(markerPath))).toBe(false) - }) - - it('prunes a proven malformed registration while retaining checkout files and its branch', async () => { - const markerPath = join(worktreePath, '.git') - const marker = await readFile(markerPath, 'utf8') - const adminPath = marker.trim().replace(/^gitdir: /, '') - await writeFile(join(adminPath, 'gitdir'), `${join(markerPath, '.git')}\n`) - await writeFile(join(worktreePath, 'untracked.txt'), 'keep this work\n') - const row = (await listWorktreesStrict(repoPath)).find((entry) => entry.path === markerPath) - expect(row).toBeDefined() - if (!row) { - throw new Error('Missing malformed registration') - } - expect(await isPrunableGitFileWorktree(row)).toBe(true) - - const result = await removeStaleLocalWorktreeRegistration({ - canonicalWorktreePath: markerPath, - repoPath, - localWorktreeGitOptions: {}, - registeredWorktree: row, - deleteBranch: true - }) - - expect(result).toEqual({ preservedBranch: { branchName: 'feature', head: row.head } }) - expect(await readFile(markerPath, 'utf8')).toBe(marker) - expect(await readFile(join(worktreePath, 'untracked.txt'), 'utf8')).toBe('keep this work\n') - expect(await git(['rev-parse', 'refs/heads/feature'], repoPath)).toBe(`${row.head}\n`) - expect((await listWorktreesStrict(repoPath)).some((entry) => entry.path === markerPath)).toBe( - false - ) - expect(existsSync(adminPath)).toBe(false) - }) - - it('sweeps trash a previous run left behind', async () => { - const stalePath = join( - workspaceRoot, - 'repo', - WORKTREE_TRASH_DIR_NAME, - 'wt-1700000000000-deadbeef' - ) - await mkdir(join(stalePath, 'node_modules'), { recursive: true }) - - await sweepStaleWorktreeTrash([workspaceRoot]) - - expect(existsSync(stalePath)).toBe(false) - expect(existsSync(worktreePath)).toBe(true) - }) -}) diff --git a/src/main/git/worktree-delete-limit.test.ts b/src/main/git/worktree-delete-limit.test.ts new file mode 100644 index 000000000000..3c6292dccdec --- /dev/null +++ b/src/main/git/worktree-delete-limit.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, it } from 'vitest' +import { + WORKTREE_DELETE_CONCURRENCY, + _worktreeDeleteLimitSnapshotForTests, + runUnderWorktreeDeleteLimit +} from './worktree-delete-limit' + +function deferred(): { promise: Promise; resolve: () => void } { + let resolve!: () => void + const promise = new Promise((settle) => { + resolve = settle + }) + return { promise, resolve } +} + +describe('runUnderWorktreeDeleteLimit', () => { + it('runs at most the limit at once and starts queued deletes in arrival order', async () => { + const gates = Array.from({ length: WORKTREE_DELETE_CONCURRENCY + 2 }, deferred) + const started: number[] = [] + const runs = gates.map((gate, index) => + runUnderWorktreeDeleteLimit(async () => { + started.push(index) + await gate.promise + }) + ) + await Promise.resolve() + + expect(started).toEqual([0, 1]) + expect(_worktreeDeleteLimitSnapshotForTests()).toEqual({ running: 2, waiting: 2 }) + + gates[1]?.resolve() + await runs[1] + await Promise.resolve() + expect(started).toEqual([0, 1, 2]) + + gates[0]?.resolve() + gates[2]?.resolve() + gates[3]?.resolve() + await Promise.all(runs) + expect(started).toEqual([0, 1, 2, 3]) + expect(_worktreeDeleteLimitSnapshotForTests()).toEqual({ running: 0, waiting: 0 }) + }) + + it('frees the slot when a delete fails', async () => { + await expect( + runUnderWorktreeDeleteLimit(async () => { + throw new Error('locked') + }) + ).rejects.toThrow('locked') + expect(_worktreeDeleteLimitSnapshotForTests()).toEqual({ running: 0, waiting: 0 }) + }) +}) diff --git a/src/main/git/worktree-delete-limit.ts b/src/main/git/worktree-delete-limit.ts new file mode 100644 index 000000000000..0304f6efe00b --- /dev/null +++ b/src/main/git/worktree-delete-limit.ts @@ -0,0 +1,34 @@ +/** + * Deletes `git worktree remove` runs at once on this host. + * + * Why its own limit instead of general git admission: a large delete holds its child for 20-35 s, + * and general admission can be as small as two slots, so two deletes there would stall every + * status read. Two concurrent deletes already saturate one disk; more only slow each other. + */ +export const WORKTREE_DELETE_CONCURRENCY = 2 + +let running = 0 +const waiting: (() => void)[] = [] + +export async function runUnderWorktreeDeleteLimit(operation: () => Promise): Promise { + if (running >= WORKTREE_DELETE_CONCURRENCY) { + await new Promise((resolve) => waiting.push(resolve)) + } else { + running += 1 + } + try { + return await operation() + } finally { + const next = waiting.shift() + if (next) { + // Why: hand the slot over directly so a new arrival cannot overtake a queued delete. + next() + } else { + running -= 1 + } + } +} + +export function _worktreeDeleteLimitSnapshotForTests(): { running: number; waiting: number } { + return { running, waiting: waiting.length } +} diff --git a/src/main/git/worktree-git-file-restore.ts b/src/main/git/worktree-git-file-restore.ts new file mode 100644 index 000000000000..616aacb2549b --- /dev/null +++ b/src/main/git/worktree-git-file-restore.ts @@ -0,0 +1,58 @@ +import { readdir, readFile, writeFile } from 'node:fs/promises' +import { isAbsolute, join } from 'node:path' +import type { GitWorktreeExecOptions } from './worktree-operation-options' +import { getErrorCode } from './worktree-operation-options' +import { readRepoCommonDirFromGit } from './worktree-list-reader' +import { areWorktreePathsEqual } from './worktree-path-comparison' + +/** + * Rewrites a registered linked worktree's missing `.git` file from Git's own admin entry, the one + * whose `gitdir` points back at it, so `git worktree remove` can validate and delete the checkout. + * + * Why not `git worktree repair` (2.29+): it also re-points every other registered path's `.git`, + * including a checkout another repository now owns at a reused path. + */ +export async function restoreMissingWorktreeGitFile( + repoPath: string, + worktreePath: string, + options: GitWorktreeExecOptions = {} +): Promise { + const commonDir = await readRepoCommonDirFromGit(repoPath, options) + if (!commonDir) { + return false + } + const adminRoot = join(commonDir, 'worktrees') + let entryNames: string[] + try { + entryNames = (await readdir(adminRoot, { withFileTypes: true })) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + } catch { + return false + } + const gitFilePath = join(worktreePath, '.git') + const owners: string[] = [] + for (const name of entryNames) { + const entryDir = join(adminRoot, name) + const backlink = await readFile(join(entryDir, 'gitdir'), 'utf8').then( + (content) => content.trim(), + () => '' + ) + const resolved = backlink && (isAbsolute(backlink) ? backlink : join(entryDir, backlink)) + if (resolved && areWorktreePathsEqual(resolved, gitFilePath)) { + owners.push(entryDir) + } + } + if (owners.length !== 1) { + return false + } + // Forward slashes are what Git itself writes, and what it compares against on Windows. + const adminDir = process.platform === 'win32' ? owners[0].replaceAll('\\', '/') : owners[0] + try { + await writeFile(gitFilePath, `gitdir: ${adminDir}\n`, { flag: 'wx' }) + } catch (error) { + // Something recreated `.git` meanwhile; Git's own validation judges it. + return getErrorCode(error) === 'EEXIST' + } + return true +} diff --git a/src/main/git/worktree-graph-listing.test.ts b/src/main/git/worktree-graph-listing.test.ts index eecfffe17538..c569ce9e7b86 100644 --- a/src/main/git/worktree-graph-listing.test.ts +++ b/src/main/git/worktree-graph-listing.test.ts @@ -1,17 +1,13 @@ // listWorktreeGraph: -z porcelain listing plus the older-Git usage-error fallback. import { beforeEach, describe, expect, it, vi } from 'vitest' -const { - gitExecFileAsyncMock, - gitExecFileSyncMock, - translateWslOutputPathsMock, - moveWorktreeDirectoryToTrashMock -} = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - gitExecFileSyncMock: vi.fn(), - translateWslOutputPathsMock: vi.fn((output: string) => output), - moveWorktreeDirectoryToTrashMock: vi.fn() -})) +const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + translateWslOutputPathsMock: vi.fn((output: string) => output) + }) +) vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock, @@ -19,13 +15,6 @@ vi.mock('./runner', () => ({ translateWslOutputPaths: translateWslOutputPathsMock })) -// Default: the checkout cannot be renamed aside, so removal deletes it in place. -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined), - restoreWorktreeDirectoryFromTrash: vi.fn().mockResolvedValue(true), - scheduleWorktreeTrashDeletion: vi.fn() -})) - import { listWorktreeGraph, WORKTREE_LIST_TIMEOUT_MS } from './worktree' import { registerWorktreeSuiteHooks } from './worktree-test-harness' diff --git a/src/main/git/worktree-list-porcelain.test.ts b/src/main/git/worktree-list-porcelain.test.ts index 5ef3630639d4..eed9891ac224 100644 --- a/src/main/git/worktree-list-porcelain.test.ts +++ b/src/main/git/worktree-list-porcelain.test.ts @@ -7,26 +7,14 @@ const { translateWslOutputPathsMock, statMock, readFileMock, - resolveGitDirMock, - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock + resolveGitDirMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn(), gitExecFileSyncMock: vi.fn(), translateWslOutputPathsMock: vi.fn((output: string) => output), statMock: vi.fn(), readFileMock: vi.fn(), - resolveGitDirMock: vi.fn(), - moveWorktreeDirectoryToTrashMock: vi.fn(), - restoreWorktreeDirectoryFromTrashMock: vi.fn(), - scheduleWorktreeTrashDeletionMock: vi.fn() -})) - -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrash: restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletion: scheduleWorktreeTrashDeletionMock + resolveGitDirMock: vi.fn() })) vi.mock('./runner', () => ({ @@ -58,11 +46,7 @@ const mockGitCommands = createGitCommandMocker(gitExecFileAsyncMock) const getGitCalls = createGitCallReader(gitExecFileAsyncMock) beforeEach(() => { - resetWorktreeRemovalState({ - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock - }) + resetWorktreeRemovalState() }) describe('listWorktrees', () => { diff --git a/src/main/git/worktree-move.test.ts b/src/main/git/worktree-move.test.ts index 9da3ddbb515f..2832639c1f16 100644 --- a/src/main/git/worktree-move.test.ts +++ b/src/main/git/worktree-move.test.ts @@ -5,13 +5,11 @@ const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock, - moveWorktreeDirectoryToTrashMock, detectSparseCheckoutMock } = vi.hoisted(() => ({ gitExecFileAsyncMock: vi.fn(), gitExecFileSyncMock: vi.fn(), translateWslOutputPathsMock: vi.fn((output: string) => output), - moveWorktreeDirectoryToTrashMock: vi.fn(), detectSparseCheckoutMock: vi.fn() })) @@ -21,13 +19,6 @@ vi.mock('./runner', () => ({ translateWslOutputPaths: translateWslOutputPathsMock })) -// Default: the checkout cannot be renamed aside, so removal deletes it in place. -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined), - restoreWorktreeDirectoryFromTrash: vi.fn().mockResolvedValue(true), - scheduleWorktreeTrashDeletion: vi.fn() -})) - vi.mock('./worktree-sparse-state', () => ({ detectSparseCheckout: detectSparseCheckoutMock, resolveGitCommonDir: vi.fn() diff --git a/src/main/git/worktree-mutation-route-invalidation.test.ts b/src/main/git/worktree-mutation-route-invalidation.test.ts index 351a24a1c98d..785ed3d13916 100644 --- a/src/main/git/worktree-mutation-route-invalidation.test.ts +++ b/src/main/git/worktree-mutation-route-invalidation.test.ts @@ -25,13 +25,6 @@ vi.mock('./status', () => ({ runWithGitReadCacheInvalidation: (run: () => Promise) => run() })) -// Default: the checkout cannot be renamed aside, so removal deletes it in place. -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: vi.fn().mockResolvedValue(undefined), - restoreWorktreeDirectoryFromTrash: vi.fn().mockResolvedValue(true), - scheduleWorktreeTrashDeletion: vi.fn() -})) - vi.mock('./worktree', async (importOriginal) => ({ ...(await importOriginal()), resolveWorktreeAddBaseContext: vi.fn(async () => ({ effectiveBase: 'origin/main' })), diff --git a/src/main/git/worktree-operation-options.ts b/src/main/git/worktree-operation-options.ts index 4bf3d9e94514..82c37a1a679d 100644 --- a/src/main/git/worktree-operation-options.ts +++ b/src/main/git/worktree-operation-options.ts @@ -42,6 +42,8 @@ export type RemoveWorktreeOptions = GitWorktreeExecOptions & { deleteBranch?: boolean forceBranchDelete?: boolean knownRemovedWorktree?: Pick + /** Stops only `git worktree remove`; a killed branch cleanup could strand a ref lock. */ + checkoutDeleteSignal?: AbortSignal } // Why: bound `git worktree add` so a OneDrive cloud-placeholder stall fails fast (STA-1292); ample for an ordinary large checkout, but not one behind a slow content filter (#12696). diff --git a/src/main/git/worktree-porcelain-parsing.test.ts b/src/main/git/worktree-porcelain-parsing.test.ts index ae5c1ed569f4..f9009077f6c4 100644 --- a/src/main/git/worktree-porcelain-parsing.test.ts +++ b/src/main/git/worktree-porcelain-parsing.test.ts @@ -1,17 +1,13 @@ // parseWorktreeList: porcelain block parsing, lock/prunable markers, NUL-delimited output. import { describe, expect, it, vi } from 'vitest' -const { - gitExecFileAsyncMock, - gitExecFileSyncMock, - translateWslOutputPathsMock, - moveWorktreeDirectoryToTrashMock -} = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - gitExecFileSyncMock: vi.fn(), - translateWslOutputPathsMock: vi.fn((output: string) => output), - moveWorktreeDirectoryToTrashMock: vi.fn() -})) +const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + translateWslOutputPathsMock: vi.fn((output: string) => output) + }) +) vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock, @@ -19,13 +15,6 @@ vi.mock('./runner', () => ({ translateWslOutputPaths: translateWslOutputPathsMock })) -// Default: the checkout cannot be renamed aside, so removal deletes it in place. -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined), - restoreWorktreeDirectoryFromTrash: vi.fn().mockResolvedValue(true), - scheduleWorktreeTrashDeletion: vi.fn() -})) - import { parseWorktreeList } from './worktree' import { registerWorktreeSuiteHooks } from './worktree-test-harness' @@ -190,7 +179,6 @@ detached HEAD aaa111 branch refs/heads/main - worktree /repo-b HEAD bbb222 branch refs/heads/dev diff --git a/src/main/git/worktree-posix-path-case-sensitivity.test.ts b/src/main/git/worktree-posix-path-case-sensitivity.test.ts index 7e6204a955e7..3c96acbadc3c 100644 --- a/src/main/git/worktree-posix-path-case-sensitivity.test.ts +++ b/src/main/git/worktree-posix-path-case-sensitivity.test.ts @@ -12,31 +12,14 @@ import type * as FsPromises from 'node:fs/promises' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type * as WslPathTranslation from './command-runner/wsl-path-translation' -const { - gitExecFileAsyncMock, - gitExecFileSyncMock, - statMock, - readFileMock, - resolveGitDirMock, - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock -} = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - gitExecFileSyncMock: vi.fn(), - statMock: vi.fn(), - readFileMock: vi.fn(), - resolveGitDirMock: vi.fn(), - moveWorktreeDirectoryToTrashMock: vi.fn(), - restoreWorktreeDirectoryFromTrashMock: vi.fn(), - scheduleWorktreeTrashDeletionMock: vi.fn() -})) - -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrash: restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletion: scheduleWorktreeTrashDeletionMock -})) +const { gitExecFileAsyncMock, gitExecFileSyncMock, statMock, readFileMock, resolveGitDirMock } = + vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + statMock: vi.fn(), + readFileMock: vi.fn(), + resolveGitDirMock: vi.fn() + })) // Why the real translator: an identity mock removes the Linux -> UNC rewrite that production // always applies, which is the only reason the Linux spelling would ever reach the comparison. @@ -151,11 +134,7 @@ describe('removeWorktree branch selection on a Windows desktop', () => { readFileMock.mockReset() readFileMock.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })) resolveGitDirMock.mockReset() - resetWorktreeRemovalState({ - moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrashMock, - scheduleWorktreeTrashDeletionMock - }) + resetWorktreeRemovalState() }) afterEach(() => { diff --git a/src/main/git/worktree-removal-large-tree.bench.test.ts b/src/main/git/worktree-removal-large-tree.bench.test.ts index debeb6210ed8..02147fdda4a0 100644 --- a/src/main/git/worktree-removal-large-tree.bench.test.ts +++ b/src/main/git/worktree-removal-large-tree.bench.test.ts @@ -1,21 +1,23 @@ -// Manual benchmark for the `worktree.remove.git_remove` stage on a large checkout. -// Opt in (it builds ~100k files): ORCA_WORKTREE_REMOVAL_BENCH=1 pnpm exec vitest run \ -// --config config/vitest.config.ts src/main/git/worktree-removal-large-tree.bench.test.ts +// Manual benchmark: how long removing a large checkout takes, and how slow a store-style write gets +// meanwhile. Opt in (it builds ~100k files): ORCA_WORKTREE_REMOVAL_BENCH=1 ./node_modules/.bin/vitest \ +// run --config config/vitest.config.ts src/main/git/worktree-removal-large-tree.bench.test.ts import { execFile } from 'node:child_process' -import { mkdir, mkdtemp, readdir, rm, writeFile } from 'node:fs/promises' -import { existsSync } from 'node:fs' +import { appendFileSync, existsSync, statSync } from 'node:fs' +import { link, mkdir, mkdtemp, open, readFile, rename, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' +import { performance } from 'node:perf_hooks' import { promisify } from 'node:util' import { afterAll, beforeAll, describe, expect, it } from 'vitest' import { removeWorktree } from './worktree' -import { getWorktreeTrashRoot, whenWorktreeTrashDeletionsSettled } from '../worktree-trash' const execFileAsync = promisify(execFile) const describeBench = process.env.ORCA_WORKTREE_REMOVAL_BENCH ? describe : describe.skip const FIXTURE_DIRECTORIES = 200 const FIXTURE_FILES_PER_DIRECTORY = 500 +const STORE_FILE_BYTES = 2 * 1024 * 1024 +const STORE_TRANSACTION_INTERVAL_MS = 20 describeBench('worktree removal on a large checkout', () => { let scratchDir = '' @@ -55,31 +57,81 @@ describeBench('worktree removal on a large checkout', () => { }, 900_000) afterAll(async () => { - await whenWorktreeTrashDeletionsSettled() if (scratchDir) { await rm(scratchDir, { recursive: true, force: true }) } }) - it('returns from removeWorktree without waiting for the recursive delete', async () => { - const startedAt = Date.now() - await removeWorktree(repoPath, worktreePath, false, { deleteBranch: false }) - const userVisibleMs = Date.now() - startedAt + it('reports removal time and store-write latency during the delete', async () => { + // Outside hard links drop to one link only once the checkout's copies are gone, wherever the + // delete runs, so the window covers the whole delete rather than just the call. + const sentinelRoot = join(scratchDir, 'sentinels') + await mkdir(sentinelRoot) + const sentinels: string[] = [] + for (let dirIndex = 0; dirIndex < FIXTURE_DIRECTORIES; dirIndex += 10) { + const sentinel = join(sentinelRoot, `pkg-${dirIndex}`) + await link(join(worktreePath, 'node_modules', `pkg-${dirIndex}`, 'file-0.js'), sentinel) + sentinels.push(sentinel) + } + const checkoutDeleted = (): boolean => sentinels.every((path) => statSync(path).nlink === 1) + const storePath = join(scratchDir, 'store.json') + await writeFile(storePath, Buffer.alloc(STORE_FILE_BYTES, 1)) - const trashRoot = getWorktreeTrashRoot(worktreePath) - console.log( - `[bench] files=${FIXTURE_DIRECTORIES * FIXTURE_FILES_PER_DIRECTORY} removeWorktree=${userVisibleMs}ms` + // Mirrors a persisted-store write: read, write a temp file, fsync it, rename it into place. + const storeTransaction = async (): Promise => { + const startedAt = performance.now() + const contents = await readFile(storePath) + const tempPath = `${storePath}.tmp` + const handle = await open(tempPath, 'w') + try { + await handle.writeFile(contents) + await handle.sync() + } finally { + await handle.close() + } + await rename(tempPath, storePath) + return performance.now() - startedAt + } + const idleMs: number[] = [] + for (let index = 0; index < 10; index += 1) { + idleMs.push(await storeTransaction()) + } + + let removalReturnedMs = 0 + const startedAt = performance.now() + const removal = removeWorktree(repoPath, worktreePath, false, { deleteBranch: false }).then( + () => { + removalReturnedMs = performance.now() - startedAt + } ) + const duringMs: number[] = [] + while (!removalReturnedMs || !checkoutDeleted()) { + const transactionStartedAt = performance.now() + duringMs.push(await storeTransaction()) + const wait = STORE_TRANSACTION_INTERVAL_MS - (performance.now() - transactionStartedAt) + if (wait > 0) { + await new Promise((resolve) => setTimeout(resolve, wait)) + } + } + const deletedMs = performance.now() - startedAt + await removal + + const percentile = (values: number[], p: number): number => { + const sorted = [...values].sort((a, b) => a - b) + return sorted[Math.min(sorted.length - 1, Math.floor((sorted.length * p) / 100))] + } + const line = + `[bench] files=${FIXTURE_DIRECTORIES * FIXTURE_FILES_PER_DIRECTORY}` + + ` removeWorktree=${removalReturnedMs.toFixed(0)}ms checkoutDeleted=${deletedMs.toFixed(0)}ms` + + ` storeIdle p50=${percentile(idleMs, 50).toFixed(1)}ms` + + ` storeDuring n=${duringMs.length} p50=${percentile(duringMs, 50).toFixed(1)}ms` + + ` p90=${percentile(duringMs, 90).toFixed(1)}ms max=${Math.max(...duringMs).toFixed(1)}ms` + console.log(line) + if (process.env.ORCA_WORKTREE_REMOVAL_BENCH_LOG) { + appendFileSync(process.env.ORCA_WORKTREE_REMOVAL_BENCH_LOG, `${line}\n`) + } expect(existsSync(worktreePath)).toBe(false) const { stdout } = await execFileAsync('git', ['worktree', 'list'], { cwd: repoPath }) expect(stdout).not.toContain(worktreePath) - - const backgroundStartedAt = Date.now() - await whenWorktreeTrashDeletionsSettled() - console.log( - `[bench] background deletion=${Date.now() - backgroundStartedAt}ms (trash root ${trashRoot})` - ) - expect(existsSync(trashRoot)).toBe(true) - expect(await readdir(trashRoot)).toEqual([]) }, 900_000) }) diff --git a/src/main/git/worktree-removal-real-git.test.ts b/src/main/git/worktree-removal-real-git.test.ts new file mode 100644 index 000000000000..2df5f7fa46a1 --- /dev/null +++ b/src/main/git/worktree-removal-real-git.test.ts @@ -0,0 +1,235 @@ +// Real-binary coverage for worktree removal: the mocked-runner suite cannot prove what Git deletes, +// deregisters and refuses, or that deleting a checkout leaves Node's file pool free. +import { execFile } from 'node:child_process' +import { existsSync, statSync } from 'node:fs' +import { link, mkdir, mkdtemp, readFile, realpath, stat, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { performance } from 'node:perf_hooks' +import { promisify } from 'node:util' +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { removeTree } from '../../shared/windows-transient-lock-removal' +import { listWorktreesStrict, removeWorktree } from './worktree' +import { areWorktreePathsEqual } from './worktree-path-comparison' +import { isPrunableGitFileWorktree } from '../worktree-prunable-git-file' +import { removeStaleLocalWorktreeRegistration } from '../local-worktree-removal-recovery' +import { sweepStaleWorktreeTrash, WORKTREE_TRASH_DIR_NAME } from '../worktree-trash' + +const execFileAsync = promisify(execFile) + +let scratchDir = '' +let repoPath = '' +let workspaceRoot = '' +let worktreePath = '' + +async function git(args: string[], cwd: string): Promise { + const { stdout } = await execFileAsync('git', args, { cwd }) + return stdout +} + +// Why parsed: Git prints forward slashes on Windows, so raw text never contains a joined path. +async function isRegistered(path: string): Promise { + return (await listWorktreesStrict(repoPath)).some((worktree) => + areWorktreePathsEqual(worktree.path, path) + ) +} + +beforeEach(async () => { + // realpath: macOS hands out /var/... temp paths while Git reports /private/var/..., and Orca + // matches the worktree it is removing against Git's own list. + scratchDir = await realpath(await mkdtemp(join(tmpdir(), 'orca-worktree-removal-'))) + repoPath = join(scratchDir, 'repo') + workspaceRoot = join(scratchDir, 'workspaces') + worktreePath = join(workspaceRoot, 'repo', 'feature') + await mkdir(repoPath, { recursive: true }) + await mkdir(join(workspaceRoot, 'repo'), { recursive: true }) + await git(['init', '-q'], repoPath) + await git(['config', 'user.email', 'removal@example.invalid'], repoPath) + await git(['config', 'user.name', 'Worktree Removal'], repoPath) + // Why: a commit's detached auto-maintenance can still be writing packs when teardown deletes the repo. + await git(['config', 'maintenance.auto', 'false'], repoPath) + await git(['config', 'gc.auto', '0'], repoPath) + await writeFile(join(repoPath, 'seed.txt'), 'seed\n') + // Committed before the worktree exists so its branch stays merged and branch cleanup can run. + await mkdir(join(repoPath, 'node_modules', 'pkg'), { recursive: true }) + await writeFile(join(repoPath, 'node_modules', 'pkg', 'index.js'), 'module.exports = 1\n') + await git(['add', '-A'], repoPath) + await git(['commit', '-qm', 'seed'], repoPath) + await git(['worktree', 'add', '-q', worktreePath, '-b', 'feature'], repoPath) +}) + +afterEach(async () => { + await removeTree(scratchDir) +}) + +describe('worktree removal against the real Git binary', () => { + it('deletes the checkout and its registration before returning', async () => { + await removeWorktree(repoPath, worktreePath, false, { deleteBranch: false }) + + expect(existsSync(worktreePath)).toBe(false) + expect(await isRegistered(worktreePath)).toBe(false) + expect(existsSync(join(workspaceRoot, 'repo', WORKTREE_TRASH_DIR_NAME))).toBe(false) + }) + + it('leaves sibling worktrees registered', async () => { + const siblingPath = join(workspaceRoot, 'repo', 'sibling') + await git(['worktree', 'add', '-q', siblingPath, '-b', 'sibling'], repoPath) + + await removeWorktree(repoPath, worktreePath, false, { deleteBranch: false }) + + expect(await isRegistered(siblingPath)).toBe(true) + expect(existsSync(siblingPath)).toBe(true) + }) + + it('deletes the branch exactly as the in-place removal did', async () => { + await removeWorktree(repoPath, worktreePath, false) + + expect(await git(['branch', '--list', 'feature'], repoPath)).toBe('') + }) + + it('refuses to delete a dirty checkout', async () => { + await writeFile(join(worktreePath, 'seed.txt'), 'edited\n') + + await expect(removeWorktree(repoPath, worktreePath, false)).rejects.toThrow() + expect(existsSync(join(worktreePath, 'seed.txt'))).toBe(true) + expect(await isRegistered(worktreePath)).toBe(true) + }) + + it('does not delete a checkout through a malformed registration that names its git file', async () => { + const markerPath = join(worktreePath, '.git') + const marker = await readFile(markerPath, 'utf8') + const adminPath = marker.trim().replace(/^gitdir: /, '') + await writeFile(join(adminPath, 'gitdir'), `${join(markerPath, '.git')}\n`) + await writeFile(join(worktreePath, 'untracked.txt'), 'keep this work\n') + + await expect( + removeWorktree(repoPath, markerPath, true, { deleteBranch: false }) + ).rejects.toThrow() + + expect(await readFile(markerPath, 'utf8')).toBe(marker) + expect(await readFile(join(worktreePath, 'untracked.txt'), 'utf8')).toBe('keep this work\n') + expect(await git(['branch', '--list', 'feature'], repoPath)).toContain('feature') + }) + + it('prunes a proven malformed registration while retaining checkout files and its branch', async () => { + const markerPath = join(worktreePath, '.git') + const marker = await readFile(markerPath, 'utf8') + const adminPath = marker.trim().replace(/^gitdir: /, '') + await writeFile(join(adminPath, 'gitdir'), `${join(markerPath, '.git')}\n`) + await writeFile(join(worktreePath, 'untracked.txt'), 'keep this work\n') + const row = (await listWorktreesStrict(repoPath)).find((entry) => + areWorktreePathsEqual(entry.path, markerPath) + ) + expect(row).toBeDefined() + if (!row) { + throw new Error('Missing malformed registration') + } + expect(await isPrunableGitFileWorktree(row)).toBe(true) + + const result = await removeStaleLocalWorktreeRegistration({ + canonicalWorktreePath: row.path, + repoPath, + localWorktreeGitOptions: {}, + registeredWorktree: row, + deleteBranch: true + }) + + expect(result).toEqual({ preservedBranch: { branchName: 'feature', head: row.head } }) + expect(await readFile(markerPath, 'utf8')).toBe(marker) + expect(await readFile(join(worktreePath, 'untracked.txt'), 'utf8')).toBe('keep this work\n') + expect(await git(['rev-parse', 'refs/heads/feature'], repoPath)).toBe(`${row.head}\n`) + expect(await isRegistered(markerPath)).toBe(false) + expect(existsSync(adminPath)).toBe(false) + }) + + it('sweeps trash an older release left behind', async () => { + const stalePath = join( + workspaceRoot, + 'repo', + WORKTREE_TRASH_DIR_NAME, + 'wt-1700000000000-deadbeef' + ) + await mkdir(join(stalePath, 'node_modules'), { recursive: true }) + + await sweepStaleWorktreeTrash([workspaceRoot]) + + expect(existsSync(stalePath)).toBe(false) + expect(existsSync(worktreePath)).toBe(true) + }) +}) + +const POOL_FIXTURE_FILES = 3_000 +const POOL_SENTINEL_EVERY = 100 + +function queuedFsRequests(): number { + return process + .getActiveResourcesInfo() + .filter((resource) => resource === 'FSReqPromise' || resource === 'FSReqCallback').length +} + +describe('worktree removal and the Node file pool', () => { + it('keeps async file I/O responsive while the checkout is deleted', async () => { + // One flat directory: a recursive delete through the pool would queue every entry at once. + const bulkPath = join(worktreePath, 'bulk') + await mkdir(bulkPath) + for (let start = 0; start < POOL_FIXTURE_FILES; start += 500) { + await Promise.all( + Array.from({ length: 500 }, (_unused, offset) => + writeFile( + join(bulkPath, `file-${start + offset}.js`), + `module.exports = ${start + offset}\n` + ) + ) + ) + } + await git(['add', '-A'], worktreePath) + await git(['commit', '-qm', 'bulk'], worktreePath) + // Outside hard links drop to one link only once the checkout's copies are gone, wherever the + // delete runs, so the window below covers the whole delete rather than just the call. + const sentinelRoot = join(scratchDir, 'sentinels') + await mkdir(sentinelRoot) + const sentinels: string[] = [] + for (let index = 0; index < POOL_FIXTURE_FILES; index += POOL_SENTINEL_EVERY) { + const sentinel = join(sentinelRoot, `file-${index}`) + await link(join(bulkPath, `file-${index}.js`), sentinel) + sentinels.push(sentinel) + } + const checkoutDeleted = (): boolean => sentinels.every((path) => statSync(path).nlink === 1) + const probePath = join(repoPath, 'seed.txt') + + let removalSettled = false + const removal = removeWorktree(repoPath, worktreePath, false, { deleteBranch: false }).finally( + () => { + removalSettled = true + } + ) + const startedAt = performance.now() + let maxQueued = 0 + let maxStatMs = 0 + let statCount = 0 + const sampler = setInterval(() => { + maxQueued = Math.max(maxQueued, queuedFsRequests()) + }, 1) + try { + while (!removalSettled || !checkoutDeleted()) { + expect(performance.now() - startedAt).toBeLessThan(60_000) + maxQueued = Math.max(maxQueued, queuedFsRequests()) + const statStartedAt = performance.now() + await stat(probePath) + maxStatMs = Math.max(maxStatMs, performance.now() - statStartedAt) + statCount += 1 + } + } finally { + clearInterval(sampler) + } + const deletionMs = performance.now() - startedAt + await removal + + console.log( + `[pool] files=${POOL_FIXTURE_FILES} delete=${deletionMs.toFixed(0)}ms stats=${statCount} maxStat=${maxStatMs.toFixed(1)}ms maxQueuedFsRequests=${maxQueued}` + ) + expect(existsSync(worktreePath)).toBe(false) + expect(maxQueued).toBeLessThan(32) + expect(maxStatMs).toBeLessThan(deletionMs / 4) + }, 120_000) +}) diff --git a/src/main/git/worktree-removal.ts b/src/main/git/worktree-removal.ts index afe1bf2a9c10..39d2056f708e 100644 --- a/src/main/git/worktree-removal.ts +++ b/src/main/git/worktree-removal.ts @@ -1,30 +1,26 @@ +import { lstat } from 'node:fs/promises' import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' import { assertWorktreeUnlockedForRemoval } from '../../shared/worktree/removal' +import { windowsLongPathGitArgs } from '../../shared/windows-long-path-git-args' import { isSubmoduleWorktreeRemovalRefusal } from '../../shared/worktree/submodule-removal' -import { withWorktreeRemoveStageSpan } from '../observability/instrumentation' +import { removeHostTree } from '../host-tree-removal' import { withSpan } from '../observability/tracer' -import { - moveWorktreeDirectoryToTrash, - restoreWorktreeDirectoryFromTrash, - scheduleWorktreeTrashDeletion -} from '../worktree-trash' import { parseWslPath } from '../wsl' import { gitExecFileAsync } from './runner' import { runWithGitReadCacheInvalidation } from './status' import { deleteBranchAfterWorktreeRemoval } from './worktree-branch-removal' -import { listWorktreesStrict } from './worktree-listing' import { invalidateWslLinkedWorktreeGitRouting } from './wsl-linked-worktree-git-routing' import type { RemoveWorktreeOptions } from './worktree-operation-options' -import { - WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS, - gitExecOptions, - normalizeLocalBranchRef -} from './worktree-operation-options' +import { getErrorCode, gitExecOptions, normalizeLocalBranchRef } from './worktree-operation-options' import { areWorktreePathsEqual } from './worktree-path-comparison' import { assertWorktreeCleanForRemoval } from './worktree-removal-preflight' import { withRepoRefMaintenancePaused } from './local-repo-ref-maintenance' import { bumpWorktreeScanGeneration, listWorktrees } from './worktree-scan-cache' import { invalidateSparseCheckoutState } from './worktree-sparse-checkout-cache' +import { runUnderWorktreeDeleteLimit } from './worktree-delete-limit' +import { runKeyedSerializedOperation } from '../cli/keyed-promise-queue' + +const branchCleanupQueueByRepo = new Map>() /** * Remove a worktree. @@ -69,16 +65,25 @@ async function performRemoveWorktree( // Why: callers outside the IPC/runtime preflight must not bypass Git's lock contract or rely on localized stderr after side effects. assertWorktreeUnlockedForRemoval(removedWorktree) - if ( - !(await tryRemoveWorktreeWithDeferredDirectoryDeletion(repoPath, worktreePath, force, options)) - ) { - const args = ['worktree', 'remove'] - if (force) { - args.push('--force') - } - args.push(worktreePath) + // Why no timeout: this is a write, so none applies by default, and Git deletes the whole checkout + // here (prod p90 29 s); a deadline would kill a legitimate large delete halfway through. + // Why long paths: creation checks out with them on Windows, so deleting without them fails with + // "Filename too long" (#6433) and leaves the branch behind via the Windows recovery. + const longPathArgs = windowsLongPathGitArgs(repoPath) + const execOptions = { + ...gitExecOptions(repoPath, options), + ...(options.checkoutDeleteSignal ? { signal: options.checkoutDeleteSignal } : {}), + ...removalGitEnv(), + admissionExempt: true as const + } + const args = [...longPathArgs, 'worktree', 'remove'] + if (force) { + args.push('--force') + } + args.push(worktreePath) + await runUnderWorktreeDeleteLimit(async () => { try { - await gitExecFileAsync(args, gitExecOptions(repoPath, options)) + await gitExecFileAsync(args, execOptions) } catch (error) { if (force || !isSubmoduleWorktreeRemovalRefusal(error)) { throw error @@ -86,11 +91,12 @@ async function performRemoveWorktree( // Why: Git refuses non-force removal of a worktree with an initialised submodule even when clean; re-prove cleanliness, then --force. await assertWorktreeCleanForRemoval(worktreePath, false, options) await gitExecFileAsync( - ['worktree', 'remove', '--force', worktreePath], - gitExecOptions(repoPath, options) + [...longPathArgs, 'worktree', 'remove', '--force', worktreePath], + execOptions ) } - } + await removeCheckoutLeftByGit(worktreePath, options) + }) if (!branchName) { return {} @@ -99,85 +105,100 @@ async function performRemoveWorktree( return {} } + return deleteBranchOfRemovedWorktree(repoPath, branchName, branchHead, options) +} + +function deleteBranchOfRemovedWorktree( + repoPath: string, + branchName: string, + branchHead: string, + options: RemoveWorktreeOptions +): Promise { // Why its own span: branch cleanup can reach the network (`fetch --prune`), so a stall here reads as // `git worktree remove` being slow unless it is timed separately. - return withSpan('worktree.remove.branch_delete', () => - deleteBranchAfterWorktreeRemoval(repoPath, branchName, branchHead, options) + // Why serialized per repo: concurrent removals in one repo race `packed-refs.lock` and the + // remote-tracking ref locks of `fetch --prune` (#2259); the checkout deletes above need not wait. + return runKeyedSerializedOperation(branchCleanupQueueByRepo, repoPath, () => + withSpan('worktree.remove.branch_delete', () => + deleteBranchAfterWorktreeRemoval(repoPath, branchName, branchHead, options) + ) ) } /** - * Rename the checkout into a sibling trash directory and clear Git's registration for the - * now-missing path, so the multi-GB recursive delete runs after this removal has returned. - * Returns false when the caller must let `git worktree remove` delete the directory inline. + * Finishes a removal whose checkout Git no longer registers (it finished deleting, or an earlier + * run did): leftover files, stale admin records, then the branch. Already-gone parts are done. */ -async function tryRemoveWorktreeWithDeferredDirectoryDeletion( +export async function finishUnregisteredWorktreeRemoval( repoPath: string, worktreePath: string, - force: boolean, + branch: { name: string; head: string } | null, + options: RemoveWorktreeOptions = {} +): Promise { + try { + await runUnderWorktreeDeleteLimit(() => removeCheckoutLeftByGit(worktreePath, options)) + await gitExecFileAsync(['worktree', 'prune'], gitExecOptions(repoPath, options)).catch( + (error: unknown) => console.warn(`[git] worktree prune failed in ${repoPath}`, error) + ) + if (!branch?.name || !(await localBranchExists(repoPath, branch.name, options))) { + return {} + } + return await withRepoRefMaintenancePaused('worktree-remove', () => + deleteBranchOfRemovedWorktree(repoPath, branch.name, branch.head, options) + ) + } finally { + invalidateSparseCheckoutState(repoPath, worktreePath) + bumpWorktreeScanGeneration(repoPath) + } +} + +async function localBranchExists( + repoPath: string, + branchName: string, options: RemoveWorktreeOptions ): Promise { - // Why: WSL-owned checkouts are deleted inside the distro, so Node on Windows must not rename them. - if (options.wslDistro || parseWslPath(worktreePath)) { + try { + await gitExecFileAsync( + ['show-ref', '--verify', '--quiet', '--', `refs/heads/${branchName}`], + gitExecOptions(repoPath, options) + ) + return true + } catch { return false } - if (!force) { - try { - // Why: `git worktree remove` re-checks cleanliness as it removes; prove the same thing here or leave removal to Git. - await assertWorktreeCleanForRemoval(worktreePath, false, options) - } catch { - return false - } - } +} - const trashPath = await withWorktreeRemoveStageSpan('trash_rename', 'local', () => - moveWorktreeDirectoryToTrash(worktreePath) - ) - if (!trashPath) { - return false +// Why: Git for Windows runs $GIT_ASK_YESNO when a file stays locked mid-delete; no prompt program may run here. +function removalGitEnv(): { env?: NodeJS.ProcessEnv } { + const inherited = Object.keys(process.env).filter((key) => key.toUpperCase() === 'GIT_ASK_YESNO') + if (inherited.length === 0) { + return {} } - try { - await clearGitRegistrationForMissingWorktree(repoPath, worktreePath, options) - } catch (error) { - // Why: put the checkout back so the in-place removal below still sees the worktree Git registered. - if (await restoreWorktreeDirectoryFromTrash(trashPath, worktreePath)) { - return false - } - throw error + const env = { ...process.env } + for (const key of inherited) { + delete env[key] } - scheduleWorktreeTrashDeletion(trashPath) - return true + return { env } } -async function clearGitRegistrationForMissingWorktree( - repoPath: string, +// Why: Git for Windows does not descend into junctions and exits 0 with them and their parent +// directories still on disk; finish the delete Git already accepted instead of leaving it behind. +async function removeCheckoutLeftByGit( worktreePath: string, options: RemoveWorktreeOptions ): Promise { - const registrationOptions = { - ...options, - timeout: options.timeout ?? WORKTREE_REMOVAL_REGISTRATION_TIMEOUT_MS + // Why: WSL-owned checkouts are deleted inside the distro, so Node on Windows must not touch them. + if (options.wslDistro || parseWslPath(worktreePath)) { + return } try { - // Removing an already-missing directory is accepted back to the Git 2.25 baseline and touches only this entry. - await gitExecFileAsync( - ['worktree', 'remove', '--force', worktreePath], - gitExecOptions(repoPath, registrationOptions) - ) - return + await lstat(worktreePath) } catch (error) { - console.warn( - `[git] Failed to deregister the moved worktree "${worktreePath}"; pruning instead`, - error - ) - } - - await gitExecFileAsync(['worktree', 'prune'], gitExecOptions(repoPath, registrationOptions)) - // Strict (not the shared scan): an unreadable repo must not read as proof that the row is gone. - const stillRegistered = (await listWorktreesStrict(repoPath, registrationOptions)).some( - (worktree) => areWorktreePathsEqual(worktree.path, worktreePath) - ) - if (stillRegistered) { - throw new Error(`Git still reports a registration for "${worktreePath}" after pruning it.`) + if (getErrorCode(error) === 'ENOENT') { + return + } + throw error } + console.warn(`[git] worktree remove left files at "${worktreePath}"; deleting them`) + await removeHostTree(worktreePath) } diff --git a/src/main/git/worktree-remove-branch-deletion.test.ts b/src/main/git/worktree-remove-branch-deletion.test.ts index e00ee48c0e0f..6469d675e9f0 100644 --- a/src/main/git/worktree-remove-branch-deletion.test.ts +++ b/src/main/git/worktree-remove-branch-deletion.test.ts @@ -1,17 +1,13 @@ // removeWorktree: branch deletion safety after the checkout is removed. -import { beforeEach, describe, expect, it, vi } from 'vitest' - -const { - gitExecFileAsyncMock, - gitExecFileSyncMock, - translateWslOutputPathsMock, - moveWorktreeDirectoryToTrashMock -} = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - gitExecFileSyncMock: vi.fn(), - translateWslOutputPathsMock: vi.fn((output: string) => output), - moveWorktreeDirectoryToTrashMock: vi.fn() -})) +import { afterEach, beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest' + +const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + translateWslOutputPathsMock: vi.fn((output: string) => output) + }) +) vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock, @@ -19,13 +15,6 @@ vi.mock('./runner', () => ({ translateWslOutputPaths: translateWslOutputPathsMock })) -// Default: the checkout cannot be renamed aside, so removal deletes it in place. -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined), - restoreWorktreeDirectoryFromTrash: vi.fn().mockResolvedValue(true), - scheduleWorktreeTrashDeletion: vi.fn() -})) - import { removeWorktree } from './worktree' import { registerWorktreeSuiteHooks } from './worktree-test-harness' @@ -35,15 +24,22 @@ describe('removeWorktree', () => { const beforeRemoval = 'worktree /repo\nHEAD abc123\nbranch refs/heads/main\n\nworktree /repo-feature\nHEAD def456\nbranch refs/heads/feature/test\n' + // Why: removal argv carries core.longpaths on Windows; pin a non-Windows default for exact argv. + let platformSpy: MockInstance<() => NodeJS.Platform> + beforeEach(() => { gitExecFileAsyncMock.mockReset() gitExecFileSyncMock.mockReset() translateWslOutputPathsMock.mockClear() + platformSpy = vi.spyOn(process, 'platform', 'get').mockReturnValue('darwin') + }) + + afterEach(() => { + platformSpy.mockRestore() }) it('uses safe `branch -d` and preserves a branch with unmerged commits', async () => { gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: beforeRemoval }) // list before - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // clean probe before the rename attempt gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // worktree remove // Git refuses to delete an unmerged branch with `-d`. gitExecFileAsyncMock.mockRejectedValueOnce(new Error('not fully merged')) // branch -d @@ -60,7 +56,6 @@ describe('removeWorktree', () => { it('deletes the branch when `branch -d` succeeds (fully merged)', async () => { gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: beforeRemoval }) // list before - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // clean probe before the rename attempt gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // worktree remove gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // branch -d succeeds @@ -75,7 +70,6 @@ describe('removeWorktree', () => { }) it('reuses known removed worktree metadata instead of relisting before removal', async () => { - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // clean probe before the rename attempt gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // worktree remove gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // branch -d succeeds @@ -87,7 +81,6 @@ describe('removeWorktree', () => { }) expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).toEqual([ - ['status', '--porcelain', '--untracked-files=all'], ['worktree', 'remove', '/repo-feature'], ['branch', '-d', '--', 'feature/test'] ]) @@ -95,7 +88,6 @@ describe('removeWorktree', () => { it('prunes and retries branch deletion only when Git reports a checked-out branch', async () => { gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: beforeRemoval }) // list before - gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // clean probe before the rename attempt gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '' }) // worktree remove gitExecFileAsyncMock.mockRejectedValueOnce( new Error("error: cannot delete branch 'feature/test' used by worktree at '/repo-stale'") @@ -107,7 +99,6 @@ describe('removeWorktree', () => { expect(gitExecFileAsyncMock.mock.calls.map((call) => call[0])).toEqual([ ['worktree', 'list', '--porcelain', '-z'], - ['status', '--porcelain', '--untracked-files=all'], ['worktree', 'remove', '/repo-feature'], ['branch', '-d', '--', 'feature/test'], ['worktree', 'prune'], diff --git a/src/main/git/worktree-scan-cache-sharing.test.ts b/src/main/git/worktree-scan-cache-sharing.test.ts index c42bec7e7301..eabe471a1c64 100644 --- a/src/main/git/worktree-scan-cache-sharing.test.ts +++ b/src/main/git/worktree-scan-cache-sharing.test.ts @@ -2,17 +2,13 @@ import { worktreeCreateGit } from './worktree-create-git-executor' // Worktree scan sharing: in-flight coalescing and mutation-generation retirement. import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const { - gitExecFileAsyncMock, - gitExecFileSyncMock, - translateWslOutputPathsMock, - moveWorktreeDirectoryToTrashMock -} = vi.hoisted(() => ({ - gitExecFileAsyncMock: vi.fn(), - gitExecFileSyncMock: vi.fn(), - translateWslOutputPathsMock: vi.fn((output: string) => output), - moveWorktreeDirectoryToTrashMock: vi.fn() -})) +const { gitExecFileAsyncMock, gitExecFileSyncMock, translateWslOutputPathsMock } = vi.hoisted( + () => ({ + gitExecFileAsyncMock: vi.fn(), + gitExecFileSyncMock: vi.fn(), + translateWslOutputPathsMock: vi.fn((output: string) => output) + }) +) vi.mock('./runner', () => ({ gitExecFileAsync: gitExecFileAsyncMock, @@ -20,13 +16,6 @@ vi.mock('./runner', () => ({ translateWslOutputPaths: translateWslOutputPathsMock })) -// Default: the checkout cannot be renamed aside, so removal deletes it in place. -vi.mock('../worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined), - restoreWorktreeDirectoryFromTrash: vi.fn().mockResolvedValue(true), - scheduleWorktreeTrashDeletion: vi.fn() -})) - import { _getWorktreeScanCacheSizesForTests, _resetWorktreeScanCacheForTests, diff --git a/src/main/ipc/desktop-renderer-runtime-capabilities.test.ts b/src/main/ipc/desktop-renderer-runtime-capabilities.test.ts index 4a8fbab955a2..135b0af68f60 100644 --- a/src/main/ipc/desktop-renderer-runtime-capabilities.test.ts +++ b/src/main/ipc/desktop-renderer-runtime-capabilities.test.ts @@ -21,6 +21,7 @@ import { SESSION_TAB_CLOSE_INTENT_RUNTIME_CAPABILITY, SESSION_TABS_RETIREMENT_PROOF_DELTA_RUNTIME_CAPABILITY, STRUCTURED_AGENT_SESSION_RUNTIME_CAPABILITY, + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY, WORKTREE_GITHUB_PR_SUPPRESSION_RUNTIME_CAPABILITY, WORKTREE_VISIBILITY_DEFAULTS_RUNTIME_CAPABILITY, WORKTREE_VISIBILITY_SOURCE_DEFAULTS_RUNTIME_CAPABILITY, @@ -47,7 +48,10 @@ const REMOTE_ONLY_BY_DECISION: readonly RuntimeCapability[] = [ BROWSER_CLIENT_HOST_RUNTIME_CAPABILITY, BROWSER_CLIENT_PAGE_METADATA_RUNTIME_CAPABILITY, // Opts into a delta feed in place of the full tab list — a remote-transport concern. - SESSION_TABS_RETIREMENT_PROOF_DELTA_RUNTIME_CAPABILITY + SESSION_TABS_RETIREMENT_PROOF_DELTA_RUNTIME_CAPABILITY, + // Main marks `removing` on its own worktree IPC listings unconditionally; the renderer never + // lists its own host's worktrees over runtime RPC, where this capability decides mark vs omit. + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY ] /** Gates the renderer must pass against its own main process. The Electron remote list omits all diff --git a/src/main/ipc/worktree-remote.ts b/src/main/ipc/worktree-remote.ts index ce11df4f4f01..0bfb8951b8c8 100644 --- a/src/main/ipc/worktree-remote.ts +++ b/src/main/ipc/worktree-remote.ts @@ -170,6 +170,7 @@ import { retireGeneratedWorktreeName } from '../worktree-name-retirement' import { createRetiredNameLookup } from '../../shared/worktree/retired-name-registry' +import { findPendingWorktreeRemovalConflict } from '../worktree-background-removal' const SSH_WORKTREE_CREATE_FETCH_FRESHNESS_MS = 30_000 const SSH_WORKTREE_CREATE_FETCH_CACHE_MAX = 512 @@ -2627,7 +2628,12 @@ async function performLocalWorktreeCreate( computeWorktreePath(effectiveSanitizedName, repo.path, worktreePathSettings, workspaceRoot), workspaceRoot ) - if (existsSync(worktreePath)) { + // Why the pending check: Git may already have deleted the directory while Orca still owns + // the path until its removal settles; take the next name instead of racing it. + if ( + existsSync(worktreePath) || + findPendingWorktreeRemovalConflict(repo.path, { worktreePath }) + ) { continue } diff --git a/src/main/ipc/worktrees-background-removal.test.ts b/src/main/ipc/worktrees-background-removal.test.ts new file mode 100644 index 000000000000..ead826e7d629 --- /dev/null +++ b/src/main/ipc/worktrees-background-removal.test.ts @@ -0,0 +1,310 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + addWorktreeMock, + getEffectiveHooksMock, + listWorktreesMock, + removeWorktreeMock, + runHookMock +} from './worktrees-test-module-mocks' +import { handlers, setupWorktreeHandlers, store } from './worktrees-test-harness' +import { mockKnownFeatureWorktree } from './worktrees-test-fixtures' +import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' +import { + _resetPendingWorktreeRemovalsForTests, + startBackgroundWorktreeRemoval +} from '../worktree-background-removal' + +vi.mock('electron', async () => + (await import('./worktrees-test-module-mocks')).electronModuleMock() +) +vi.mock('../git/worktree', async () => + (await import('./worktrees-test-module-mocks')).gitWorktreeModuleMock() +) +vi.mock('../git/runner', async () => + (await import('./worktrees-test-module-mocks')).gitRunnerModuleMock() +) +vi.mock('../git/repo', async () => + (await import('./worktrees-test-module-mocks')).gitRepoModuleMock() +) +vi.mock('../git/git-username', async (importOriginal) => ({ + ...(await importOriginal>()), + resolveLocalGitUsername: (await import('./worktrees-test-module-mocks')) + .resolveLocalGitUsernameMock +})) +vi.mock('../github/client', async () => + (await import('./worktrees-test-module-mocks')).githubClientModuleMock() +) +vi.mock('../source-control/hosted-review', async () => + (await import('./worktrees-test-module-mocks')).hostedReviewModuleMock() +) +vi.mock('../providers/ssh-git-dispatch', async () => + (await import('./worktrees-test-module-mocks')).sshGitDispatchModuleMock() +) +vi.mock('../providers/ssh-filesystem-dispatch', async () => + (await import('./worktrees-test-module-mocks')).sshFilesystemDispatchModuleMock() +) +vi.mock('./worktree-symlinks', async () => + (await import('./worktrees-test-module-mocks')).worktreeSymlinksModuleMock() +) +vi.mock('./ssh', async () => (await import('./worktrees-test-module-mocks')).sshModuleMock()) +vi.mock('../ssh/ssh-target-registry', async () => + (await import('./worktrees-test-module-mocks')).sshTargetRegistryModuleMock() +) +vi.mock('../hooks', async () => (await import('./worktrees-test-module-mocks')).hooksModuleMock()) +vi.mock('../setup-runner-script-text', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).setupRunnerScriptTextModuleMock( + (await importOriginal()) as Record + ) +) +vi.mock('../worktree-runner-script', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).worktreeRunnerScriptModuleMock( + (await importOriginal()) as Record + ) +) +vi.mock('../effective-hook-config', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).effectiveHookConfigModuleMock( + (await importOriginal()) as Record + ) +) +vi.mock('../setup-hook-env-vars', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).setupHookEnvVarsModuleMock( + (await importOriginal()) as Record + ) +) +vi.mock('./worktree-logic', async (importOriginal) => + (await import('./worktrees-test-module-mocks')).worktreeLogicModuleMock( + (await importOriginal()) as Record + ) +) +vi.mock('../terminal-history-deletion', async () => + (await import('./worktrees-test-module-mocks')).terminalHistoryDeletionModuleMock() +) +vi.mock('../ports/advertised-url-watcher', async () => + (await import('./worktrees-test-module-mocks')).advertisedUrlWatcherModuleMock() +) +vi.mock('../workspace-cleanup-scan-snapshot', async () => + (await import('./worktrees-test-module-mocks')).workspaceCleanupScanSnapshotModuleMock() +) +vi.mock('../workspace-space-analysis-snapshot', async () => + (await import('./worktrees-test-module-mocks')).workspaceSpaceAnalysisSnapshotModuleMock() +) +vi.mock('../workspace-cleanup-removal-snapshot-prune', async () => + (await import('./worktrees-test-module-mocks')).workspaceCleanupRemovalSnapshotPruneModuleMock() +) +vi.mock('../runtime/worktree-teardown', async () => + (await import('./worktrees-test-module-mocks')).worktreeTeardownModuleMock() +) +vi.mock('./pty', async () => (await import('./worktrees-test-module-mocks')).ptyModuleMock()) + +function remove(args: Record): Promise { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: worktrees:remove resolves a RemoveWorktreeResult. + return handlers['worktrees:remove'](null, args) as Promise +} + +type ListedRow = { id: string; removing?: true } + +async function listRepoRows(): Promise { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: worktrees:list resolves the repo's Worktree rows. + return (await handlers['worktrees:list'](null, { repoId: 'repo-1' })) as ListedRow[] +} + +function blockGitRemove(): { + release: (result?: RemoveWorktreeResult) => void + fail: (error: Error) => void +} { + let release!: (result?: RemoveWorktreeResult) => void + let fail!: (error: Error) => void + removeWorktreeMock.mockImplementation( + () => + new Promise((resolve, reject) => { + release = (result = {}) => resolve(result) + fail = reject + }) + ) + return { + release: (result) => release(result), + fail: (error) => fail(error) + } +} + +const featureId = 'repo-1::/workspace/feature-wt' + +describe('worktrees:remove in the background', () => { + beforeEach(() => { + setupWorktreeHandlers() + }) + + afterEach(() => { + _resetPendingWorktreeRemovalsForTests() + }) + + it('replies once Git finishes, listing the row as removing until then', async () => { + const worktrees = mockKnownFeatureWorktree() + const git = blockGitRemove() + + const reply = remove({ worktreeId: featureId }) + await vi.waitFor(() => expect(removeWorktreeMock).toHaveBeenCalledTimes(1)) + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + + const during = await listRepoRows() + expect(during.find((row) => row.id === featureId)?.removing).toBe(true) + expect(during.find((row) => row.id === 'repo-1::/workspace/repo')?.removing).toBeUndefined() + + listWorktreesMock.mockResolvedValue([worktrees[0]]) + git.release({ preservedBranch: { branchName: 'feature', head: 'feature' } }) + + const result = await reply + expect(result).toMatchObject({ preservedBranch: { branchName: 'feature', head: 'feature' } }) + expect(result.removing).toBeUndefined() + expect(store.removeWorktreeMeta).toHaveBeenCalledWith(featureId, 'local') + const after = await listRepoRows() + expect(after.map((row) => row.id)).not.toContain(featureId) + }) + + it('replies with the delete error and lists the row normally again', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + mockKnownFeatureWorktree() + const git = blockGitRemove() + + const reply = remove({ worktreeId: featureId }) + await vi.waitFor(() => expect(removeWorktreeMock).toHaveBeenCalledTimes(1)) + git.fail(new Error('permission denied')) + + await expect(reply).rejects.toThrow('permission denied') + expect(store.removeWorktreeMeta).not.toHaveBeenCalled() + const rows = await listRepoRows() + expect(rows.find((row) => row.id === featureId)).toBeDefined() + expect(rows.find((row) => row.id === featureId)?.removing).toBeUndefined() + + // A retry is a fresh removal, not a join onto the failed one. + removeWorktreeMock.mockResolvedValue({}) + await expect(remove({ worktreeId: featureId })).resolves.not.toHaveProperty('removing') + expect(removeWorktreeMock).toHaveBeenCalledTimes(2) + }) + + it('joins a repeat delete, with any options, onto the removal Git is running', async () => { + mockKnownFeatureWorktree() + const git = blockGitRemove() + + const first = remove({ worktreeId: featureId }) + await vi.waitFor(() => expect(removeWorktreeMock).toHaveBeenCalledTimes(1)) + const repeat = remove({ worktreeId: featureId, force: true, hostId: 'local' }) + + git.release({ preservedBranch: { branchName: 'feature', head: 'feature' } }) + const preserved = { preservedBranch: { branchName: 'feature', head: 'feature' } } + await expect(first).resolves.toMatchObject(preserved) + await expect(repeat).resolves.toMatchObject(preserved) + expect(removeWorktreeMock).toHaveBeenCalledTimes(1) + }) + + it('runs same-repo archive hooks one at a time while their Git deletes overlap', async () => { + const [main, feature] = mockKnownFeatureWorktree() + const secondId = 'repo-1::/workspace/second-wt' + listWorktreesMock.mockResolvedValue([ + main, + feature, + { ...feature, path: '/workspace/second-wt', branch: 'second', head: 'second' } + ]) + getEffectiveHooksMock.mockReturnValue({ scripts: { archive: 'git update-ref refs/x HEAD' } }) + const hookReleases: (() => void)[] = [] + runHookMock.mockImplementation( + () => + new Promise((resolve) => hookReleases.push(() => resolve({ success: true, output: '' }))) + ) + const gitReleases: (() => void)[] = [] + removeWorktreeMock.mockImplementation( + () => new Promise((resolve) => gitReleases.push(() => resolve({}))) + ) + + const first = remove({ worktreeId: featureId }) + const second = remove({ worktreeId: secondId }) + await vi.waitFor(() => expect(runHookMock).toHaveBeenCalledTimes(1)) + // Let the second request reach the host's acceptance queue before checking it waits there. + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(runHookMock).toHaveBeenCalledTimes(1) + + hookReleases[0]?.() + await vi.waitFor(() => expect(runHookMock).toHaveBeenCalledTimes(2)) + hookReleases[1]?.() + await vi.waitFor(() => expect(removeWorktreeMock).toHaveBeenCalledTimes(2)) + + for (const release of gitReleases) { + release() + } + await expect(first).resolves.not.toHaveProperty('removing') + await expect(second).resolves.not.toHaveProperty('removing') + }) + + it('joins a removal another client got accepted while this request waited its turn', async () => { + const [main, feature] = mockKnownFeatureWorktree() + const secondId = 'repo-1::/workspace/second-wt' + listWorktreesMock.mockResolvedValue([ + main, + feature, + { ...feature, path: '/workspace/second-wt', branch: 'second', head: 'second' } + ]) + getEffectiveHooksMock.mockReturnValue({ scripts: { archive: 'true' } }) + let releaseHook!: () => void + runHookMock.mockResolvedValue({ success: true, output: '' }) + runHookMock.mockImplementationOnce( + () => + new Promise((resolve) => { + releaseHook = () => resolve({ success: true, output: '' }) + }) + ) + removeWorktreeMock.mockResolvedValue({}) + + const holdsTurn = remove({ worktreeId: secondId }) + await vi.waitFor(() => expect(runHookMock).toHaveBeenCalledTimes(1)) + const queued = remove({ worktreeId: featureId }) + await new Promise((resolve) => setTimeout(resolve, 20)) + // Stands in for the runtime path (CLI, paired clients), which coalesces apart from this one. + let finishOther: ((result: RemoveWorktreeResult) => void) | undefined + void startBackgroundWorktreeRemoval({ + removal: { + worktreeId: featureId, + repoId: 'repo-1', + repoPath: '/workspace/repo', + worktree: feature, + deleteBranch: true, + force: false + }, + run: () => + new Promise((resolve) => { + finishOther = resolve + }), + publish: () => {} + }) + releaseHook() + await holdsTurn + await vi.waitFor(() => expect(finishOther).toBeDefined()) + + const preserved = { preservedBranch: { branchName: 'feature', head: 'feature' } } + finishOther?.(preserved) + await expect(queued).resolves.toMatchObject(preserved) + expect(runHookMock).toHaveBeenCalledTimes(1) + expect(removeWorktreeMock).toHaveBeenCalledTimes(1) + }) + + it('gives a create with the same name the next free name while the delete runs', async () => { + mockKnownFeatureWorktree() + blockGitRemove() + void remove({ worktreeId: featureId }).catch(() => {}) + await vi.waitFor(() => expect(removeWorktreeMock).toHaveBeenCalledTimes(1)) + + addWorktreeMock.mockResolvedValue({}) + listWorktreesMock.mockResolvedValue([ + { + path: '/workspace/feature-wt-2', + head: 'abc', + branch: 'feature-wt-2', + isBare: false, + isMainWorktree: false + } + ]) + await handlers['worktrees:create'](null, { repoId: 'repo-1', name: 'feature-wt' }) + + expect(addWorktreeMock).toHaveBeenCalledTimes(1) + expect(addWorktreeMock.mock.calls[0]?.[1]).toBe('/workspace/feature-wt-2') + }) +}) diff --git a/src/main/ipc/worktrees-delete-pty-teardown.test.ts b/src/main/ipc/worktrees-delete-pty-teardown.test.ts index 4326278446eb..7fec88c15ad9 100644 --- a/src/main/ipc/worktrees-delete-pty-teardown.test.ts +++ b/src/main/ipc/worktrees-delete-pty-teardown.test.ts @@ -145,7 +145,11 @@ describe('registerWorktreeHandlers', () => { ]) expect(store.removeWorktreeMeta).toHaveBeenCalledTimes(1) expect(deleteWorktreeHistoryDirMock).toHaveBeenCalledTimes(1) - expect(mainWindow.webContents.send).toHaveBeenCalledTimes(1) + // One notice when the row starts showing as removing, one after it has left the table. + expect(mainWindow.webContents.send).toHaveBeenCalledTimes(2) + expect(mainWindow.webContents.send).toHaveBeenLastCalledWith('worktrees:changed', { + repoId: 'repo-1' + }) }) it('rejects concurrent deletes for the same worktree id with different options', async () => { @@ -155,11 +159,13 @@ describe('registerWorktreeHandlers', () => { const started = new Promise((resolve) => { removalStarted = resolve }) - removeWorktreeMock.mockImplementation( + // Held in acceptance: once accepted, a repeat delete joins the background removal instead. + killAllProcessesForWorktreeMock.mockImplementation( () => - new Promise((resolve) => { + new Promise((resolve) => { removalStarted() - finishRemoval = resolve + finishRemoval = () => + resolve({ runtimeStopped: 0, providerStopped: 0, registryStopped: 0 }) }) ) @@ -176,9 +182,10 @@ describe('registerWorktreeHandlers', () => { }) ).rejects.toThrow('Worktree deletion already in progress') - expect(removeWorktreeMock).toHaveBeenCalledTimes(1) + expect(removeWorktreeMock).not.toHaveBeenCalled() finishRemoval() await expect(first).resolves.toEqual({ catalogVersion: anyCatalogVersion }) + expect(removeWorktreeMock).toHaveBeenCalledTimes(1) }) it('still rejects forced unregistered delete paths that exist on disk', async () => { diff --git a/src/main/ipc/worktrees-removal-recovery.test.ts b/src/main/ipc/worktrees-removal-recovery.test.ts index d3231d50bd87..b1e91daea9d7 100644 --- a/src/main/ipc/worktrees-removal-recovery.test.ts +++ b/src/main/ipc/worktrees-removal-recovery.test.ts @@ -452,9 +452,6 @@ describe('registerWorktreeHandlers', () => { expect.anything() ) expect(store.removeWorktreeMeta).not.toHaveBeenCalled() - expect(mainWindow.webContents.send).not.toHaveBeenCalledWith('worktrees:changed', { - repoId: 'repo-1' - }) } finally { removePathSpy.mockRestore() } diff --git a/src/main/ipc/worktrees-test-harness.ts b/src/main/ipc/worktrees-test-harness.ts index 6e1019d2150f..bbe2a1491610 100644 --- a/src/main/ipc/worktrees-test-harness.ts +++ b/src/main/ipc/worktrees-test-harness.ts @@ -306,7 +306,7 @@ export function setupWorktreeHandlers(): WorktreeRuntimeStub { // Default: no direct-read recovery, so a listing that omits the row still fails the create. describeCreatedWorktreeMock.mockResolvedValue(undefined) forceDeleteLocalBranchMock.mockResolvedValue(undefined) - const runtimeStub = createWorktreeRuntimeStub() + const runtimeStub = createWorktreeRuntimeStub(mainWindow) registerWorktreeHandlers(mainWindow as never, store as never, runtimeStub as never) return runtimeStub } diff --git a/src/main/ipc/worktrees-test-runtime-stub.ts b/src/main/ipc/worktrees-test-runtime-stub.ts index bb2f33cb05e5..68b49754f4a4 100644 --- a/src/main/ipc/worktrees-test-runtime-stub.ts +++ b/src/main/ipc/worktrees-test-runtime-stub.ts @@ -1,3 +1,4 @@ +import { _resetPendingWorktreeRemovalsForTests } from '../worktree-background-removal' import { vi } from 'vitest' export type WorktreeRuntimeStub = { @@ -18,10 +19,15 @@ export type WorktreeRuntimeStub = { closeFileWatchersForRemoval: ReturnType acquireFileWatcherRemoval: ReturnType hydrateInferredWorktreeLineage: ReturnType + publishWorktreeRemovalChange: ReturnType } /** Why: create-flow tests need a minimal runtime; full fetchRemoteWithCache behavior lives in fetch-remote-cache.test.ts. */ -export function createWorktreeRuntimeStub(): WorktreeRuntimeStub { +export function createWorktreeRuntimeStub(mainWindow?: { + webContents: { send: (channel: string, ...args: unknown[]) => unknown } +}): WorktreeRuntimeStub { + // Why here: every harness setup builds a stub, so no removal from an earlier test leaks in. + _resetPendingWorktreeRemovalsForTests() const runtimeStub: WorktreeRuntimeStub = { resolveRemoteTrackingBase: vi.fn().mockResolvedValue(null), hasRemoteTrackingRef: vi.fn().mockResolvedValue(false), @@ -48,7 +54,11 @@ export function createWorktreeRuntimeStub(): WorktreeRuntimeStub { notifyWorktreesChangedForRemoteClients: vi.fn(), closeFileWatchersForRemoval: vi.fn().mockResolvedValue(undefined), acquireFileWatcherRemoval: vi.fn(), - hydrateInferredWorktreeLineage: vi.fn().mockResolvedValue(undefined) + hydrateInferredWorktreeLineage: vi.fn().mockResolvedValue(undefined), + // Mirrors the real runtime notifier, which sends worktrees:changed to the window. + publishWorktreeRemovalChange: vi.fn((repoId: string) => { + mainWindow?.webContents.send('worktrees:changed', { repoId }) + }) } runtimeStub.acquireFileWatcherRemoval.mockImplementation( async (worktreePath: string, connectionId?: string) => { diff --git a/src/main/ipc/worktrees-windows.test.ts b/src/main/ipc/worktrees-windows.test.ts index fb69743c288e..596ff07be52d 100644 --- a/src/main/ipc/worktrees-windows.test.ts +++ b/src/main/ipc/worktrees-windows.test.ts @@ -311,7 +311,10 @@ describe('registerWorktreeHandlers – Windows path handling', () => { closeFileWatchersForRemoval: vi.fn().mockResolvedValue(undefined), acquireFileWatcherRemoval: vi.fn().mockResolvedValue({ finish: vi.fn().mockResolvedValue(undefined) - }) + }), + publishWorktreeRemovalChange: vi.fn((repoId: string) => + mainWindow.webContents.send('worktrees:changed', { repoId }) + ) } registerWorktreeHandlers(mainWindow as never, store as never, runtimeStub as never) }) diff --git a/src/main/ipc/worktrees/listing/detected-provider-listing.ts b/src/main/ipc/worktrees/listing/detected-provider-listing.ts index 975cb5eeb783..8bf5d924b2ce 100644 --- a/src/main/ipc/worktrees/listing/detected-provider-listing.ts +++ b/src/main/ipc/worktrees/listing/detected-provider-listing.ts @@ -1,3 +1,7 @@ +import { + projectPendingWorktreeRemovals, + snapshotPendingWorktreeRemovals +} from '../../../worktree-background-removal' import { getRepoExecutionHostId, getSshTargetIdForExecutionHost @@ -128,6 +132,7 @@ export async function listDetectedWorktreesForCapturedRepo( catalogVersion: getLocalWorktreeCatalogVersion(repo.id) } } + const pendingAtScan = snapshotPendingWorktreeRemovals() const scan = await scanUntilNotOvertaken( repo.id, connectionId && provider @@ -184,7 +189,15 @@ export async function listDetectedWorktreesForCapturedRepo( authoritative: true, source: 'git', catalogVersion: localWorktreeCatalogVersionAt(scan.generation), - worktrees: buildDetectedGitWorktrees(store, repo, gitWorktrees, allMeta) + worktrees: connectionId + ? buildDetectedGitWorktrees(store, repo, gitWorktrees, allMeta) + : // Why always marked: the desktop renderer ships with this main process. + projectPendingWorktreeRemovals( + buildDetectedGitWorktrees(store, repo, gitWorktrees, allMeta), + (worktree) => worktree.id, + true, + pendingAtScan + ) } } catch (err) { const aborted = abortedResult() diff --git a/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts b/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts index 89de29f30e84..c09c1224e88a 100644 --- a/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts +++ b/src/main/ipc/worktrees/listing/register-worktree-catalog-handlers.ts @@ -32,11 +32,25 @@ import { readAllWorktreeMetaForRepo } from '../../../persistence/host-qualified-worktree-meta' import type { WorktreeMeta } from '../../../../shared/worktree/meta-types' +import type { Worktree } from '../../../../shared/worktree/types' +import { + projectPendingWorktreeRemovals, + snapshotPendingWorktreeRemovals, + type PendingWorktreeRemovals +} from '../../../worktree-background-removal' import { getLocalWorktreeScanGeneration } from '../../../local-worktree-scan-generation' import { getRegisteredWorktreeRootsRevision } from '../../registered-worktree-roots-cache' const WORKTREE_LIST_ALL_CONCURRENCY = 8 +// Why always marked: the desktop renderer ships with this main process, so it reads the marker. +function markLocalWorktreesUnderRemoval( + worktrees: T[], + pendingAtScan: PendingWorktreeRemovals +): T[] { + return projectPendingWorktreeRemovals(worktrees, (worktree) => worktree.id, true, pendingAtScan) +} + async function mapWithConcurrency( items: readonly T[], limit: number, @@ -102,6 +116,7 @@ export function registerWorktreeCatalogHandlers(context: WorktreeIpcContext): vo let sideEffectToken: DetectedWorktreeSideEffectToken | undefined let metadataPrune: DetectedWorktreeMetadataPrune | undefined let hygieneDue: boolean | undefined + const pendingAtScan = snapshotPendingWorktreeRemovals() if (isFolderRepo(repo)) { return listVisibleFolderWorkspaces(store, repo) } else if (connectionId) { @@ -152,9 +167,10 @@ export function registerWorktreeCatalogHandlers(context: WorktreeIpcContext): vo } loggedWorktreeListFailures.delete(`${repo.id}:${repo.path}`) const metadata = metadataForRepo(repo) - return buildDetectedGitWorktrees(store, repo, gitWorktrees, metadata) + const worktrees = buildDetectedGitWorktrees(store, repo, gitWorktrees, metadata) .filter((worktree) => worktree.visible) .map((worktree) => stampAndMergeVisibleDetectedWorktree(store, repo, worktree, metadata)) + return connectionId ? worktrees : markLocalWorktreesUnderRemoval(worktrees, pendingAtScan) } catch (err) { warnOnce( loggedWorktreeListFailures, @@ -200,6 +216,7 @@ export function registerWorktreeCatalogHandlers(context: WorktreeIpcContext): vo let sideEffectToken: DetectedWorktreeSideEffectToken | undefined let metadataPrune: DetectedWorktreeMetadataPrune | undefined let hygieneDue: boolean | undefined + const pendingAtScan = snapshotPendingWorktreeRemovals() if (isFolderRepo(repo)) { return listVisibleFolderWorkspaces(store, repo) } else if (connectionId) { @@ -244,9 +261,10 @@ export function registerWorktreeCatalogHandlers(context: WorktreeIpcContext): vo } loggedWorktreeListFailures.delete(`${repo.id}:${repo.path}`) const metadata = allMeta ?? readAllWorktreeMetaForRepo(store, repo) - return buildDetectedGitWorktrees(store, repo, gitWorktrees, metadata) + const worktrees = buildDetectedGitWorktrees(store, repo, gitWorktrees, metadata) .filter((worktree) => worktree.visible) .map((worktree) => stampAndMergeVisibleDetectedWorktree(store, repo, worktree, metadata)) + return connectionId ? worktrees : markLocalWorktreesUnderRemoval(worktrees, pendingAtScan) } catch (err) { warnOnce( loggedWorktreeListFailures, diff --git a/src/main/ipc/worktrees/removal/register-worktree-removal-handlers.ts b/src/main/ipc/worktrees/removal/register-worktree-removal-handlers.ts index edcc0899f730..d91743b01850 100644 --- a/src/main/ipc/worktrees/removal/register-worktree-removal-handlers.ts +++ b/src/main/ipc/worktrees/removal/register-worktree-removal-handlers.ts @@ -12,6 +12,11 @@ import { getWorktreeRemovalOptionsKey } from './worktree-removal-coordinator' import { resolveRepoForExecutionHost } from '../repo-host-ownership' +import { + finishAcceptedWorktreeRemoval, + waitForPendingWorktreeRemoval +} from '../../../worktree-background-removal' +import { runSerializedWorktreeRemovalAcceptance } from '../../../worktree-removal-acceptance-queue' export function registerWorktreeRemovalHandlers(context: WorktreeIpcContext): void { const { store, options, worktreeRemovalsInFlight } = context @@ -26,6 +31,11 @@ export function registerWorktreeRemovalHandlers(context: WorktreeIpcContext): vo } // The resolved repo supplies host ownership when legacy callers omit args.hostId. const removalHostId = getRepoExecutionHostId(repo) + // Why: a retry or a second window asking while Git still deletes joins that removal. + const pending = waitForPendingWorktreeRemoval(args.worktreeId, removalHostId) + if (pending) { + return { ...(await pending), catalogVersion: getLocalWorktreeCatalogVersion(repoId) } + } const inFlightKey = getWorktreeRemovalInFlightKey(args.worktreeId, removalHostId) const optionsKey = getWorktreeRemovalOptionsKey(args) const inFlightRemoval = worktreeRemovalsInFlight.get(inFlightKey) @@ -38,27 +48,30 @@ export function registerWorktreeRemovalHandlers(context: WorktreeIpcContext): vo // Why: concurrent stale-toast/double-click/sidebar races can hit the same worktree; share the op so only one path touches Git and disk. const removal = withWorktreeSpan({ stage: 'remove', path: worktreePath }, async () => { - const result = await executeWorktreeRemoval( - context, - args, - repo, - repoId, - worktreePath, - removalHostId - ) + const accept = async (): Promise => + // Why: another client's removal of this worktree may have been accepted during the wait. + waitForPendingWorktreeRemoval(args.worktreeId, removalHostId) + ? { removing: true } + : executeWorktreeRemoval(context, args, repo, repoId, worktreePath, removalHostId) + const accepted = await (repo.connectionId + ? accept() + : runSerializedWorktreeRemovalAcceptance(repo.path, accept)) + const result = await finishAcceptedWorktreeRemoval(accepted, args.worktreeId, removalHostId) + // A background job reports its own lifecycle when Git finishes. + if (!accepted.removing) { + options?.onWorktreeLifecycle?.({ + kind: 'removed', + worktreeId: args.worktreeId, + path: worktreePath + }) + } // Why stamped inside the shared promise: a coalesced second caller gets the same reply, // naming the catalog this removal produced. return { ...result, catalogVersion: getLocalWorktreeCatalogVersion(repoId) } }) worktreeRemovalsInFlight.set(inFlightKey, { optionsKey, promise: removal }) try { - const result = await removal - options?.onWorktreeLifecycle?.({ - kind: 'removed', - worktreeId: args.worktreeId, - path: parseWorktreeId(args.worktreeId).worktreePath - }) - return result + return await removal } finally { if (worktreeRemovalsInFlight.get(inFlightKey)?.promise === removal) { worktreeRemovalsInFlight.delete(inFlightKey) diff --git a/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts b/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts index 5a36199c8e3d..a75a2607c9e5 100644 --- a/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts +++ b/src/main/ipc/worktrees/removal/remove-registered-local-worktree.ts @@ -1,7 +1,7 @@ import type { Repo } from '../../../../shared/repo-types' import type { ExecutionHostId } from '../../../../shared/execution-host' import type { RemoveWorktreeResult } from '../../../../shared/worktree/create-types' -import type { GitPushTarget } from '../../../../shared/worktree/types' +import type { GitPushTarget, GitWorktreeInfo } from '../../../../shared/worktree/types' import { assertWorktreeUnlockedForRemoval } from '../../../../shared/worktree/removal' import type { LocalProjectWorktreeGitOptions } from '../../../project-runtime-git-options' import { @@ -16,10 +16,7 @@ import { recoverLocalWindowsWorktreeRemoval } from '../../../local-worktree-remo import { withWorktreeRemoveStageSpan } from '../../../observability/instrumentation' import { findRegisteredDeletableWorktree } from '../../../worktree-removal-safety' import { CLIENT_REMOVAL_HOME } from '../../../worktree-removal-home-guard' -import { - cleanupUnusedWorktreePushTargetRemote, - notifyWorktreesChanged -} from '../../worktree-remote' +import { cleanupUnusedWorktreePushTargetRemote } from '../../worktree-remote' import { findExistingWorktreeSymlinkPaths, removeWorktreeLinkedPaths @@ -43,6 +40,10 @@ import { stopPtysForDestructiveWorktreeRemoval } from './worktree-removal-ownership' import { preservedBranchCleanupScopeKey } from '../../../../shared/preserved-branch-cleanup' +import { + removesInBackground, + startBackgroundWorktreeRemoval +} from '../../../worktree-background-removal' export async function removeRegisteredLocalWorktree( context: WorktreeIpcContext, @@ -56,7 +57,7 @@ export async function removeRegisteredLocalWorktree( hasLocalWorktreeGitOptions: boolean, deleteBranch: boolean ): Promise { - const { mainWindow, store, runtime } = context + const { runtime } = context const refreshedWorktrees = hasLocalWorktreeGitOptions ? await listGitWorktreesStrict(repo.path, localWorktreeGitOptions) : await listGitWorktreesStrict(repo.path) @@ -103,11 +104,10 @@ export async function removeRegisteredLocalWorktree( // Why: Git can still classify this as an orphan after preflight; keep strict PTY teardown before any recursive fallback deletion. } - let removalResult: RemoveWorktreeResult | undefined const removalGate = await withWorktreeRemoveStageSpan('watcher_gate', 'local', async () => runtime.acquireFileWatcherRemoval(canonicalWorktreePath) ) - let removalCompleted = false + let accepted = false try { // Why: hold the watcher/terminal gate through Git and any recursive fallback so no late spawn recreates a native handle. // Linked-path deletion is destructive too, so PTYs must release every handle before Windows or WSL filesystem cleanup starts. @@ -121,13 +121,99 @@ export async function removeRegisteredLocalWorktree( if (linkedPaths.length > 0) { await removeWorktreeLinkedPaths(canonicalWorktreePath, linkedPaths) } + accepted = true + } finally { + if (!accepted) { + await removalGate.finish(false) + } + } + const finish = (checkoutDeleteSignal?: AbortSignal): Promise => + finishLocalWorktreeRemoval({ + context, + args, + repo, + repoId, + canonicalWorktreePath, + removalHostId, + removedPushTarget, + localWorktreeGitOptions, + hasLocalWorktreeGitOptions, + deleteBranch, + refreshedRegisteredWorktree, + removalGate, + checkoutDeleteSignal + }) + if (!removesInBackground(canonicalWorktreePath, localWorktreeGitOptions)) { + const result = await finish() + runtime.publishWorktreeRemovalChange(repoId) + return result + } + // Why detached: every refusal above already ran, and Git's 20-35 s delete must finish even when the + // request that asked for it goes away; other views read the host's `removing` marker meanwhile. + void startBackgroundWorktreeRemoval({ + removal: { + worktreeId: args.worktreeId, + repoId, + repoPath: repo.path, + worktree: refreshedRegisteredWorktree, + deleteBranch, + force: args.force ?? false + }, + run: async (stopSignal) => { + const result = await finish(stopSignal) + context.options?.onWorktreeLifecycle?.({ + kind: 'removed', + worktreeId: args.worktreeId, + path: canonicalWorktreePath + }) + return result + }, + publish: () => runtime.publishWorktreeRemovalChange(repoId) + }) + return { removing: true } +} + +async function finishLocalWorktreeRemoval({ + context, + args, + repo, + repoId, + canonicalWorktreePath, + removalHostId, + removedPushTarget, + localWorktreeGitOptions, + hasLocalWorktreeGitOptions, + deleteBranch, + refreshedRegisteredWorktree, + removalGate, + checkoutDeleteSignal +}: { + context: WorktreeIpcContext + args: RemoveWorktreeArgs + repo: Repo + repoId: string + canonicalWorktreePath: string + removalHostId: ExecutionHostId + removedPushTarget: GitPushTarget | undefined + localWorktreeGitOptions: LocalProjectWorktreeGitOptions + hasLocalWorktreeGitOptions: boolean + deleteBranch: boolean + refreshedRegisteredWorktree: GitWorktreeInfo + removalGate: { finish: (removed: boolean) => Promise } + checkoutDeleteSignal?: AbortSignal +}): Promise { + const { store, runtime } = context + let removalResult: RemoveWorktreeResult | undefined + let removalCompleted = false + try { try { const removeOptions = { ...(!deleteBranch ? { deleteBranch } : {}), // Why: reuse the authoritative worktree list already computed here instead of rescanning siblings on the hot delete path. knownRemovedWorktree: refreshedRegisteredWorktree, - ...(hasLocalWorktreeGitOptions ? localWorktreeGitOptions : {}) + ...(hasLocalWorktreeGitOptions ? localWorktreeGitOptions : {}), + checkoutDeleteSignal } removalResult = preserveBranchHeadFallback( await withWorktreeRemoveStageSpan('git_remove', 'local', async () => @@ -187,7 +273,6 @@ export async function removeRegisteredLocalWorktree( }) ) invalidateAuthorizedRootsCache() - notifyWorktreesChanged(mainWindow, repoId) removalCompleted = true return {} } else { @@ -228,7 +313,5 @@ export async function removeRegisteredLocalWorktree( await withWorktreeRemoveStageSpan('cache_invalidation', 'local', async () => { invalidateAuthorizedRootsCache() }) - - notifyWorktreesChanged(mainWindow, repoId) return removalResult ?? {} } diff --git a/src/main/observability/instrumentation.ts b/src/main/observability/instrumentation.ts index 8569ab6ef048..bba248cd663c 100644 --- a/src/main/observability/instrumentation.ts +++ b/src/main/observability/instrumentation.ts @@ -297,7 +297,6 @@ export type WorktreeRemoveStage = | 'git_remove' | 'metadata_purge' | 'pty_sweep' - | 'trash_rename' | 'watcher_gate' /** Wrap one stage of a worktree removal. Children share the parent's `kind` so `kind`-filtered diff --git a/src/main/repo-maintenance-idle-gate.ts b/src/main/repo-maintenance-idle-gate.ts index 78bb25fe68c9..dec9acb549fb 100644 --- a/src/main/repo-maintenance-idle-gate.ts +++ b/src/main/repo-maintenance-idle-gate.ts @@ -6,6 +6,7 @@ import { } from './git/local-repo-ref-maintenance' import { hasWorktreeRemovalsInFlight } from './ipc/worktrees/worktree-ipc-context' import { hasPendingWorktreeCreatePreparations } from './worktree-create-preparation' +import { hasPendingWorktreeRemovals } from './worktree-background-removal' /** * The app-wide "not now" answer for idle repo maintenance. @@ -32,6 +33,7 @@ export function installRepoMaintenanceIdleGate( inputs.getWorkingAgentCount() > 0 || hasPendingWorktreeCreatePreparations() || hasWorktreeRemovalsInFlight() || + hasPendingWorktreeRemovals() || isOnBatteryPower() ) // Do-not-start, never stop-what-is-running. Killing a pack to honour a battery diff --git a/src/main/runtime/orca-runtime-background-removal-test-support.ts b/src/main/runtime/orca-runtime-background-removal-test-support.ts new file mode 100644 index 000000000000..811c31f36fc6 --- /dev/null +++ b/src/main/runtime/orca-runtime-background-removal-test-support.ts @@ -0,0 +1,20 @@ +import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' +import type { RemoveManagedWorktreeOptions } from './runtime-worktree-selection' + +type RemovalRuntimePrototype = { + removeManagedWorktree: ( + selector: string, + options?: RemoveManagedWorktreeOptions + ) => Promise +} + +/** + * Makes `removeManagedWorktree` wait for its background delete, as a current client does, so suites + * about what the delete does keep asserting its end state. A test can still pass `false`. + */ +export function awaitBackgroundRemovalsInRuntimeTests(prototype: RemovalRuntimePrototype): void { + const remove = prototype.removeManagedWorktree + prototype.removeManagedWorktree = function (selector, options = {}) { + return remove.call(this, selector, { waitForBackgroundRemoval: true, ...options }) + } +} diff --git a/src/main/runtime/orca-runtime-notify-ssh-state-changed.ts b/src/main/runtime/orca-runtime-notify-ssh-state-changed.ts index 4a60ab458a46..b321a62f1cce 100644 --- a/src/main/runtime/orca-runtime-notify-ssh-state-changed.ts +++ b/src/main/runtime/orca-runtime-notify-ssh-state-changed.ts @@ -134,6 +134,12 @@ export class OrcaRuntimeWithNotifySshStateChanged extends OrcaRuntimeWithGetStat this.emitClientEvent({ type: 'worktreesChanged', repoId }) } + /** A background removal started or ended; every client refetches and reads the `removing` marker. */ + publishWorktreeRemovalChange(repoId: string): void { + this.invalidateResolvedWorktreeCache() + this.notifyWorktreesChanged(repoId) + } + // Why: structural catalog changes require a fresh Git scan; renderer metadata edits do not. notifyWorktreeCatalogChangedForRemoteClients(repoId: string): void { this.invalidateWorktreeScanCacheForRepo(repoId) diff --git a/src/main/runtime/orca-runtime-remove-managed-worktree.ts b/src/main/runtime/orca-runtime-remove-managed-worktree.ts index 24e733046733..dc065bd82248 100644 --- a/src/main/runtime/orca-runtime-remove-managed-worktree.ts +++ b/src/main/runtime/orca-runtime-remove-managed-worktree.ts @@ -6,13 +6,16 @@ import { } from '../worktree-removal-repo-owner' import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' import { getRepoExecutionHostId, parseExecutionHostId } from '../../shared/execution-host' +import { + finishAcceptedWorktreeRemoval, + waitForPendingWorktreeRemoval +} from '../worktree-background-removal' import { preservedBranchCleanupScopeKey } from '../../shared/preserved-branch-cleanup' import { getRuntimeWorktreeRemovalOptionsKey, type RemoveManagedWorktreeOptions } from './runtime-worktree-selection' import { withWorktreeSpan } from '../observability/instrumentation' -import { invalidateAuthorizedRootsCache } from '../ipc/filesystem-auth' import { resolveWorktreeRemovalHome, resolveWorktreeRemovalRoute @@ -50,6 +53,17 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM const store = this.store const cleanupHostId = parseExecutionHostId(hostId)?.id const removalTarget = await this.resolveWorktreeRemovalTarget(worktreeSelector, cleanupHostId) + // Why: a retry or a second client asking while Git still deletes joins that removal. + const pending = waitForPendingWorktreeRemoval(removalTarget.id, cleanupHostId) + if (pending) { + return options.waitForBackgroundRemoval ? await pending : { removing: true } + } + const emitRemoved = (): void => + this.emitWorktreeLifecycle({ + kind: 'removed', + worktreeId: removalTarget.id, + path: removalTarget.path + }) const cleanupScopeKey = preservedBranchCleanupScopeKey({ worktreeId: removalTarget.id, hostId: cleanupHostId @@ -66,7 +80,9 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM optionsKey ) if (inFlightRemoval) { - return inFlightRemoval + return options.waitForBackgroundRemoval + ? finishAcceptedWorktreeRemoval(await inFlightRemoval, removalTarget.id, cleanupHostId) + : inFlightRemoval } const removal = (async (): Promise => { return withWorktreeSpan({ stage: 'remove', path: removalTarget.path }, async () => { @@ -82,6 +98,8 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM } const repo = repoOwner.kind === 'resolved' ? repoOwner.repo : undefined const removalHostId = repo ? (cleanupHostId ?? getRepoExecutionHostId(repo)) : cleanupHostId + const purgeRemovedWorktree = (): void => + this.purgeRemovedWorktree(store, removalTarget.id, removalTarget.repoId, removalHostId) const orphanOrFolderResult = await removeOrphanOrFolderWorktree({ runtime: this, store, @@ -140,12 +158,8 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM removalTarget.id ), finishRemoval: () => { - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id, removalHostId) this.preservedBranchCleanup.delete(removalTarget.id, cleanupHostId) - this.invalidateResolvedWorktreeCache() - this.invalidateWorktreeScanCacheForRepo(removalTarget.repoId) - invalidateAuthorizedRootsCache() + purgeRemovedWorktree() this.notifyWorktreesChanged(repo.id) } }) @@ -192,11 +206,7 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM registeredWorktree.head, removedPushTarget ) - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id, removalHostId) - this.invalidateResolvedWorktreeCache() - this.invalidateWorktreeScanCacheForRepo(removalTarget.repoId) - invalidateAuthorizedRootsCache() + purgeRemovedWorktree() this.notifyWorktreesChanged(repo.id) return removalResult ?? {} } @@ -235,11 +245,7 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM registeredWorktree.head, removedPushTarget ) - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id, removalHostId) - this.invalidateResolvedWorktreeCache() - this.invalidateWorktreeScanCacheForRepo(removalTarget.repoId) - invalidateAuthorizedRootsCache() + purgeRemovedWorktree() this.notifyWorktreesChanged(repo.id) } }) @@ -277,25 +283,23 @@ export class OrcaRuntimeWithRemoveManagedWorktree extends OrcaRuntimeWithCreateM } else { this.preservedBranchCleanup.delete(removalTarget.id, cleanupHostId) } - this.clearOptimisticReconcileToken(removalTarget.id) - this.removeWorktreeMetadataAndHistory(store, removalTarget.id, removalHostId) - this.invalidateResolvedWorktreeCache() - this.invalidateWorktreeScanCacheForRepo(removalTarget.repoId) - invalidateAuthorizedRootsCache() - this.notifyWorktreesChanged(repo.id) - } + purgeRemovedWorktree() + }, + onRemoved: emitRemoved, + publish: () => this.publishWorktreeRemovalChange(repo.id) }) }) })() this.removeManagedWorktreeInFlight.track(cleanupScopeKey, optionsKey, removal) try { - const result = await removal - this.emitWorktreeLifecycle({ - kind: 'removed', - worktreeId: removalTarget.id, - path: removalTarget.path - }) - return result + const accepted = await removal + // A background removal emits this when Git finishes instead. + if (!accepted.removing) { + emitRemoved() + } + return options.waitForBackgroundRemoval + ? finishAcceptedWorktreeRemoval(accepted, removalTarget.id, cleanupHostId) + : accepted } finally { this.removeManagedWorktreeInFlight.release(cleanupScopeKey, removal) } diff --git a/src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts b/src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts index 0aeacdc0e99e..7fef1bdf9db1 100644 --- a/src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts +++ b/src/main/runtime/orca-runtime-resolve-worktree-removal-target.ts @@ -22,6 +22,10 @@ import { resolveBareAgentLaunchCommand } from './runtime-agent-launch-resolution import { buildAgentStartupPlan } from '../../shared/tui-agent-startup' import { resolveAgentStartupPlanInputs } from '../../shared/agent-startup-plan-inputs' import { agentStartedTelemetry } from '../agent-launch/agent-started-telemetry' +import { LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' +import { invalidateAuthorizedRootsCache } from '../ipc/filesystem-auth' +import { resumeInterruptedWorktreeRemovals } from '../worktree-background-removal' +import { interruptedLocalWorktreeRemovalJob } from './runtime-interrupted-local-worktree-removal' export class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRuntimeWithRemoveManagedWorktree { protected async resolveWorktreeRemovalTarget( @@ -38,6 +42,41 @@ export class OrcaRuntimeWithResolveWorktreeRemovalTarget extends OrcaRuntimeWith }) } + /** Runs the same delete again for each local removal a quit or crash interrupted. */ + finishInterruptedWorktreeRemovals(): void { + const store = this.store + if (!store) { + return + } + resumeInterruptedWorktreeRemovals((record) => + interruptedLocalWorktreeRemovalJob(record, { + store, + acquireWatcherRemoval: this.acquireFileWatcherRemoval, + closeWatchers: (path) => this.closeFileWatchersForRemoval(path), + preservedBranchCleanup: this.preservedBranchCleanup, + purge: ({ worktreeId, repoId }) => + this.purgeRemovedWorktree(store, worktreeId, repoId, LOCAL_EXECUTION_HOST_ID), + onRemoved: ({ worktreeId, worktreePath }) => + this.emitWorktreeLifecycle({ kind: 'removed', worktreeId, path: worktreePath }), + publish: (repoId) => this.publishWorktreeRemovalChange(repoId) + }) + ) + } + + // Host state every removal path drops once Git has let go of the checkout. + protected purgeRemovedWorktree( + store: RuntimeStore, + worktreeId: string, + repoId: string, + removalHostId?: ExecutionHostId + ): void { + this.clearOptimisticReconcileToken(worktreeId) + this.removeWorktreeMetadataAndHistory(store, worktreeId, removalHostId) + this.invalidateResolvedWorktreeCache() + this.invalidateWorktreeScanCacheForRepo(repoId) + invalidateAuthorizedRootsCache() + } + protected removeWorktreeMetadataAndHistory( store: RuntimeStore, worktreeId: string, diff --git a/src/main/runtime/orca-runtime-test-lifecycle.spec.ts b/src/main/runtime/orca-runtime-test-lifecycle.spec.ts index e815599728b2..413035af5094 100644 --- a/src/main/runtime/orca-runtime-test-lifecycle.spec.ts +++ b/src/main/runtime/orca-runtime-test-lifecycle.spec.ts @@ -1,4 +1,6 @@ import * as mocks from './orca-runtime-test-mocks.spec' +import { awaitBackgroundRemovalsInRuntimeTests } from './orca-runtime-background-removal-test-support' +import { _resetPendingWorktreeRemovalsForTests } from '../worktree-background-removal' const { MOCK_GIT_WORKTREES, RuntimeBrowserCommands, _resetTerminalViewAttributesForTest } = mocks const { addGitHubIssueCommentMock, addGitHubPRReviewCommentMock } = mocks @@ -57,7 +59,11 @@ const { sshProviderGenerations, unregisterSshGitProviderMock, updateGitHubIssueM const { updateGitHubPRDetailsMock, updateGitHubPRStateMock, updateGitHubPRTitleMock } = mocks const { updateGitLabIssueMock, updateGitLabMRMock, updateGitLabMRReviewersMock, vi } = mocks +// oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the prototype carries the removal method the wrapper replaces. +awaitBackgroundRemovalsInRuntimeTests(mocks.OrcaRuntimeService.prototype as never) + function resetRuntimeTestMocks(): void { + _resetPendingWorktreeRemovalsForTests() // Why: constructing the browser commands is what pulls the Chromium cluster in, so // production installs this at the Electron entry. A Node host installs none and the // browser RPCs reject rather than silently succeeding. diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts index a9edac0ede7a..96abf6a97cef 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-and-reconciliation-part-03.spec.ts @@ -1,5 +1,6 @@ -import { beforeEach, describe, expect, it, vi } from 'vitest' +import { beforeEach, describe, expect, it, vi, type MockInstance } from 'vitest' import { resetWorktreeTestSshHostHome } from '../../worktree-removal-test-ssh-host-home' +import { _settlePendingWorktreeRemovalsForTests } from '../../worktree-background-removal' import { OrcaRuntimeService, @@ -273,7 +274,7 @@ describe('OrcaRuntimeService', () => { } }) - it('rejects concurrent runtime worktree removals for the same id with different options', async () => { + it('joins a runtime removal Git is already deleting, even with different options', async () => { const runtime = createWorktreeRemovalRuntime() const removeStarted = deferred() const finishRemoval = deferred() @@ -283,16 +284,51 @@ describe('OrcaRuntimeService', () => { return {} }) + const resolveTarget: MockInstance<(...args: unknown[]) => Promise> = vi.spyOn( + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: resolveWorktreeRemovalTarget is a protected runtime method the spy only observes. + runtime as never, + 'resolveWorktreeRemovalTarget' + ) const first = runtime.removeManagedWorktree(TEST_WORKTREE_ID) await removeStarted.promise - await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true })).rejects.toThrow( - 'Worktree deletion already in progress' - ) - + // Accepted means every refusal already ran; a Force Delete retry has nothing left to waive. + const second = runtime.removeManagedWorktree(TEST_WORKTREE_ID, { force: true }) + await vi.waitFor(() => expect(resolveTarget).toHaveBeenCalledTimes(2)) + // The retry checks the removal table right after this resolves, before Git is let finish. + await resolveTarget.mock.results[1]?.value + finishRemoval.resolve() + await expect(Promise.all([first, second])).resolves.toEqual([{}, {}]) expect(removeWorktree).toHaveBeenCalledTimes(1) + }) + + it('answers a client that cannot wait as soon as Git starts deleting', async () => { + const runtime = createWorktreeRemovalRuntime() + const removeStarted = deferred() + const finishRemoval = deferred() + vi.mocked(removeWorktree).mockImplementation(async () => { + removeStarted.resolve() + await finishRemoval.promise + return {} + }) + + const accepted = runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + waitForBackgroundRemoval: false + }) + await removeStarted.promise + await expect(accepted).resolves.toEqual({ removing: true }) finishRemoval.resolve() - await expect(first).resolves.toEqual({}) + await _settlePendingWorktreeRemovalsForTests() + }) + + it('replies to a waiting client with the error of a delete that fails after acceptance', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const runtime = createWorktreeRemovalRuntime() + vi.mocked(removeWorktree).mockRejectedValue(new Error('permission denied')) + + await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID)).rejects.toThrow( + 'permission denied' + ) }) it('treats forced runtime deletion of an already-missing unregistered worktree as cleanup', async () => { diff --git a/src/main/runtime/orca-runtime-tests/worktree-removal-archive-hook-gate.spec.ts b/src/main/runtime/orca-runtime-tests/worktree-removal-archive-hook-gate.spec.ts index 1333d6bd469d..ee5d67745e64 100644 --- a/src/main/runtime/orca-runtime-tests/worktree-removal-archive-hook-gate.spec.ts +++ b/src/main/runtime/orca-runtime-tests/worktree-removal-archive-hook-gate.spec.ts @@ -23,6 +23,9 @@ import { ARCHIVE_HOOK_FAILED_REMOVAL_CODE, asArchiveHookRefusal } from '../../../shared/worktree/archive-hook-removal-gate' +import { runSerializedWorktreeRemovalAcceptance } from '../../worktree-removal-acceptance-queue' +import { startBackgroundWorktreeRemoval } from '../../worktree-background-removal' +import type { RemoveWorktreeResult } from '../../../shared/worktree/create-types' function withArchiveHook(): void { vi.mocked(getEffectiveHooks).mockReturnValue({ @@ -206,6 +209,67 @@ describe('archive hook removal gate', () => { expect(removeWorktree).toHaveBeenCalled() }) + it('holds a desktop removal in the same repo until this one is accepted', async () => { + const runtime = createWorktreeRemovalRuntime() + withArchiveHook() + const hookRun = deferred<{ success: boolean; output: string; exitCode?: number }>() + vi.mocked(runHook).mockReturnValue(hookRun.promise) + vi.mocked(removeWorktree).mockResolvedValue({}) + + const removal = runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: false, + runHooks: true + }) + await vi.waitFor(() => expect(runHook).toHaveBeenCalled()) + // The desktop IPC path queues its acceptance on this same per-repo queue. + const sibling = vi.fn(async () => {}) + const siblingAccepted = runSerializedWorktreeRemovalAcceptance(TEST_REPO_PATH, sibling) + await new Promise((resolve) => setTimeout(resolve, 20)) + expect(sibling).not.toHaveBeenCalled() + + hookRun.resolve({ success: true, output: '', exitCode: 0 }) + await siblingAccepted + expect(sibling).toHaveBeenCalledTimes(1) + await expect(removal).resolves.toEqual({}) + }) + + it('joins a desktop removal of the same worktree accepted while this one waited', async () => { + const runtime = createWorktreeRemovalRuntime() + withArchiveHook() + vi.mocked(runHook).mockResolvedValue({ success: true, output: '' }) + vi.mocked(removeWorktree).mockResolvedValue({}) + const siblingTurn = deferred() + void runSerializedWorktreeRemovalAcceptance(TEST_REPO_PATH, () => siblingTurn.promise) + + const removal = runtime.removeManagedWorktree(TEST_WORKTREE_ID, { + force: false, + runHooks: true + }) + await new Promise((resolve) => setTimeout(resolve, 20)) + // The desktop IPC path coalesces apart from this one; stand in for its accepted removal. + const desktopDelete = deferred() + void startBackgroundWorktreeRemoval({ + removal: { + worktreeId: TEST_WORKTREE_ID, + repoId: 'repo-1', + repoPath: TEST_REPO_PATH, + worktree: { path: TEST_WORKTREE_PATH, branch: 'feature', head: 'feature' }, + deleteBranch: true, + force: false + }, + run: () => desktopDelete.promise, + publish: () => {} + }) + siblingTurn.resolve() + await new Promise((resolve) => setTimeout(resolve, 20)) + const preserved = { preservedBranch: { branchName: 'feature', head: 'feature' } } + desktopDelete.resolve(preserved) + + await expect(removal).resolves.toMatchObject(preserved) + expect(runHook).not.toHaveBeenCalled() + expect(removeWorktree).not.toHaveBeenCalled() + }) + it('does not coalesce an override retry onto the refusal already in flight', async () => { const runtime = createWorktreeRemovalRuntime() withArchiveHook() diff --git a/src/main/runtime/rpc/methods/worktree-catalog-methods.ts b/src/main/runtime/rpc/methods/worktree-catalog-methods.ts index 8b230c169d8e..082d0400f7c8 100644 --- a/src/main/runtime/rpc/methods/worktree-catalog-methods.ts +++ b/src/main/runtime/rpc/methods/worktree-catalog-methods.ts @@ -1,6 +1,11 @@ import { defineMethod } from '../core' import { resolveWorktreeCatalogSnapshot } from '../worktree-catalog-snapshot' import { supportsWorktreeVisibilitySourceDefaults } from '../worktree-visibility-client-capability' +import { + projectWorktreeListRemovals, + projectWorktreePsRemovals +} from '../worktree-removal-marker-projection' +import { snapshotPendingWorktreeRemovals } from '../../../worktree-background-removal' import { WorktreeDetectedListParams, WorktreeListParams, @@ -12,12 +17,17 @@ export const WORKTREE_CATALOG_METHODS = [ name: 'worktree.ps', params: WorktreePsParams, handler: async (params, context) => { - const result = await context.runtime.getWorktreePs( - params.limit, - supportsWorktreeVisibilitySourceDefaults( - context, - params.supportsWorktreeVisibilitySourceDefaults - ) + const pendingAtScan = snapshotPendingWorktreeRemovals() + const result = projectWorktreePsRemovals( + await context.runtime.getWorktreePs( + params.limit, + supportsWorktreeVisibilitySourceDefaults( + context, + params.supportsWorktreeVisibilitySourceDefaults + ) + ), + context, + pendingAtScan ) // Why: callers that never send the field get the byte-exact legacy response. return params.afterSnapshotId === undefined @@ -28,12 +38,18 @@ export const WORKTREE_CATALOG_METHODS = [ defineMethod({ name: 'worktree.list', params: WorktreeListParams, - handler: async (params, context) => - context.runtime.listManagedWorktrees( - params.repo, - params.limit, - supportsWorktreeVisibilitySourceDefaults(context) + handler: async (params, context) => { + const pendingAtScan = snapshotPendingWorktreeRemovals() + return projectWorktreeListRemovals( + await context.runtime.listManagedWorktrees( + params.repo, + params.limit, + supportsWorktreeVisibilitySourceDefaults(context) + ), + context, + pendingAtScan ) + } }), defineMethod({ name: 'worktree.listRetiredNames', @@ -43,11 +59,17 @@ export const WORKTREE_CATALOG_METHODS = [ defineMethod({ name: 'worktree.detectedList', params: WorktreeDetectedListParams, - handler: async (params, context) => - context.runtime.listDetectedManagedWorktrees( - params.repo, - undefined, - supportsWorktreeVisibilitySourceDefaults(context) + handler: async (params, context) => { + const pendingAtScan = snapshotPendingWorktreeRemovals() + return projectWorktreeListRemovals( + await context.runtime.listDetectedManagedWorktrees( + params.repo, + undefined, + supportsWorktreeVisibilitySourceDefaults(context) + ), + context, + pendingAtScan ) + } }) ] diff --git a/src/main/runtime/rpc/methods/worktree-rm-background-reply.test.ts b/src/main/runtime/rpc/methods/worktree-rm-background-reply.test.ts new file mode 100644 index 000000000000..1905d95b2951 --- /dev/null +++ b/src/main/runtime/rpc/methods/worktree-rm-background-reply.test.ts @@ -0,0 +1,61 @@ +import { describe, expect, it, vi } from 'vitest' +import { + ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES, + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY, + type RuntimeCapability +} from '../../../../shared/protocol-version' +import { RpcDispatcher } from '../dispatcher' +import type { OrcaRuntimeService } from '../../orca-runtime' +import { WORKTREE_METHODS } from './worktree' + +function makeRuntime(): OrcaRuntimeService { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: worktree.rm with an explicit host reads only removeManagedWorktree. + return { + getRuntimeId: () => 'test-runtime', + removeManagedWorktree: vi.fn().mockResolvedValue({}) + } as unknown as OrcaRuntimeService +} + +async function dispatchRm( + runtime: OrcaRuntimeService, + clientCapabilities: readonly RuntimeCapability[] | undefined +): Promise { + const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS }) + await dispatcher.dispatch( + { + id: 'req-1', + authToken: 'tok', + method: 'worktree.rm', + params: { worktree: 'id:wt-1', hostId: 'local' } + }, + { clientCapabilities } + ) +} + +// Why: the reply may wait out Git's delete only for a client that shows the `removing` marker; an +// older client is answered on acceptance, as its timeouts and listings expect. +describe('worktree.rm waits for the delete only for clients that can show it', () => { + it('asks the runtime to wait for a client that advertises background removal', async () => { + expect(ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES).toContain( + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY + ) + const runtime = makeRuntime() + await dispatchRm(runtime, ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES) + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( + 'id:wt-1', + expect.objectContaining({ waitForBackgroundRemoval: true }) + ) + }) + + it.each([ + ['a client that negotiated no capabilities', undefined], + ['a client without background removal', []] + ])('answers %s on acceptance', async (_name, capabilities) => { + const runtime = makeRuntime() + await dispatchRm(runtime, capabilities) + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( + 'id:wt-1', + expect.not.objectContaining({ waitForBackgroundRemoval: true }) + ) + }) +}) diff --git a/src/main/runtime/rpc/methods/worktree.ts b/src/main/runtime/rpc/methods/worktree.ts index c7dda776333c..468a1235a33c 100644 --- a/src/main/runtime/rpc/methods/worktree.ts +++ b/src/main/runtime/rpc/methods/worktree.ts @@ -26,6 +26,7 @@ import { WorktreeTeardownMissingTerminalsParams } from './worktree-schemas' import { WORKTREE_CATALOG_METHODS } from './worktree-catalog-methods' +import { readsWorktreeRemovalMarker } from '../worktree-removal-marker-projection' export const WORKTREE_METHODS = [ ...WORKTREE_CATALOG_METHODS, @@ -215,7 +216,8 @@ export const WORKTREE_METHODS = [ defineMethod({ name: 'worktree.rm', params: WorktreeRemove, - handler: async (params, { runtime }) => { + handler: async (params, context) => { + const { runtime } = context // Translate a paired client's runtime-local host spelling before host-qualified reads. let resolvedHostId = resolvePairedCallerHostId( () => runtime.listRepos(), @@ -250,7 +252,10 @@ export const WORKTREE_METHODS = [ runHooks: params.runHooks === true, allowUnverifiedPtyStop: params.allowUnverifiedPtyStop === true, allowFailedArchiveHook: params.allowFailedArchiveHook === true, - ...(resolvedHostId ? { hostId: resolvedHostId } : {}) + ...(resolvedHostId ? { hostId: resolvedHostId } : {}), + // Why: only a client that shows the `removing` marker can wait out Git's delete; older + // clients get the acceptance and never see the row again. + ...(readsWorktreeRemovalMarker(context) ? { waitForBackgroundRemoval: true } : {}) }) return { removed: true, diff --git a/src/main/runtime/rpc/worktree-removal-marker-projection.test.ts b/src/main/runtime/rpc/worktree-removal-marker-projection.test.ts new file mode 100644 index 000000000000..4ce22b0a3600 --- /dev/null +++ b/src/main/runtime/rpc/worktree-removal-marker-projection.test.ts @@ -0,0 +1,123 @@ +import { afterEach, beforeEach, describe, expect, it } from 'vitest' +import { + ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES, + NATIVE_REMOTE_RUNTIME_CLIENT_CAPABILITIES, + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY +} from '../../../shared/protocol-version' +import { remoteRuntimeClientCapabilities } from '../../../shared/remote-runtime-client-capabilities' +import type { + RuntimeWorktreeListResult, + RuntimeWorktreePsResult +} from '../../../shared/runtime-worktree-contracts' +import { + _resetPendingWorktreeRemovalsForTests, + snapshotPendingWorktreeRemovals, + startBackgroundWorktreeRemoval +} from '../../worktree-background-removal' +import { + projectWorktreeListRemovals, + projectWorktreePsRemovals +} from './worktree-removal-marker-projection' + +const removingId = 'repo-1::/work/feature' +const keptId = 'repo-1::/work/other' + +function listResult(): RuntimeWorktreeListResult { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the projection reads only id, hostId and the counts. + return { + worktrees: [ + { id: removingId, hostId: 'local' }, + { id: keptId, hostId: 'local' } + ], + totalCount: 2, + truncated: false + } as unknown as RuntimeWorktreeListResult +} + +function psResult(): RuntimeWorktreePsResult { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the projection reads only worktreeId, hostId and the counts. + return { + worktrees: [ + { worktreeId: removingId, hostId: 'local' }, + { worktreeId: keptId, hostId: 'local' } + ], + totalCount: 2, + truncated: false + } as unknown as RuntimeWorktreePsResult +} + +describe('worktree listings while a checkout is being deleted', () => { + beforeEach(() => { + void startBackgroundWorktreeRemoval({ + removal: { + worktreeId: removingId, + repoId: 'repo-1', + repoPath: '/work/repo', + worktree: { path: '/work/feature', branch: 'refs/heads/feature', head: 'abc' }, + deleteBranch: true, + force: false + }, + run: () => new Promise(() => {}), + publish: () => {} + }) + }) + + afterEach(() => { + _resetPendingWorktreeRemovalsForTests() + }) + + it('marks the row for a desktop or web client that negotiated the marker', () => { + const context = { clientCapabilities: ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES } + expect( + projectWorktreeListRemovals(listResult(), context, snapshotPendingWorktreeRemovals()) + .worktrees + ).toEqual([ + { id: removingId, hostId: 'local', removing: true }, + { id: keptId, hostId: 'local' } + ]) + expect( + projectWorktreePsRemovals(psResult(), context, snapshotPendingWorktreeRemovals()).worktrees + ).toEqual([ + { worktreeId: removingId, hostId: 'local', removing: true }, + { worktreeId: keptId, hostId: 'local' } + ]) + }) + + it('leaves the row out for a client that would show it as a normal workspace', () => { + // An older desktop or web build, the CLI, the phone, and an in-process caller alike. + for (const clientCapabilities of [ + [], + undefined, + NATIVE_REMOTE_RUNTIME_CLIENT_CAPABILITIES, + remoteRuntimeClientCapabilities() + ]) { + const list = projectWorktreeListRemovals( + listResult(), + { clientCapabilities }, + snapshotPendingWorktreeRemovals() + ) + expect(list.worktrees.map((row) => row.id)).toEqual([keptId]) + expect(list.totalCount).toBe(1) + const ps = projectWorktreePsRemovals( + psResult(), + { clientCapabilities }, + snapshotPendingWorktreeRemovals() + ) + expect(ps.worktrees.map((row) => row.worktreeId)).toEqual([keptId]) + expect(ps.totalCount).toBe(1) + } + }) + + it('advertises the marker only from the desktop renderer, never the shared remote defaults', () => { + // Mobile and the CLI send the shared defaults and have no Deleting affordance to show. + expect(ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES).toContain( + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY + ) + expect(remoteRuntimeClientCapabilities()).not.toContain( + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY + ) + expect(NATIVE_REMOTE_RUNTIME_CLIENT_CAPABILITIES).not.toContain( + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY + ) + }) +}) diff --git a/src/main/runtime/rpc/worktree-removal-marker-projection.ts b/src/main/runtime/rpc/worktree-removal-marker-projection.ts new file mode 100644 index 000000000000..f4f13ca9c418 --- /dev/null +++ b/src/main/runtime/rpc/worktree-removal-marker-projection.ts @@ -0,0 +1,63 @@ +import { WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY } from '../../../shared/protocol-version' +import type { DetectedWorktreeListResult } from '../../../shared/worktree/types' +import type { + RuntimeWorktreeListResult, + RuntimeWorktreePsResult +} from '../../../shared/runtime-worktree-contracts' +import { + projectPendingWorktreeRemovals, + type PendingWorktreeRemovals +} from '../../worktree-background-removal' +import type { RpcContext } from './core' + +// Why no in-process default: callers without negotiation (the CLI, host-side readers) print or act +// on rows, and a checkout mid-delete is not a workspace they can use. +export function readsWorktreeRemovalMarker( + context: Pick +): boolean { + return ( + context.clientCapabilities?.includes(WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY) === true + ) +} + +export function projectWorktreeListRemovals< + T extends RuntimeWorktreeListResult | DetectedWorktreeListResult +>( + result: T, + context: Pick, + pendingAtScan: PendingWorktreeRemovals +): T { + const worktrees = projectPendingWorktreeRemovals( + result.worktrees, + (worktree) => worktree.id, + readsWorktreeRemovalMarker(context), + pendingAtScan + ) + if (worktrees === result.worktrees) { + return result + } + const omitted = result.worktrees.length - worktrees.length + return 'totalCount' in result + ? { ...result, worktrees, totalCount: result.totalCount - omitted } + : { ...result, worktrees } +} + +export function projectWorktreePsRemovals( + result: RuntimeWorktreePsResult, + context: Pick, + pendingAtScan: PendingWorktreeRemovals +): RuntimeWorktreePsResult { + const worktrees = projectPendingWorktreeRemovals( + result.worktrees, + (summary) => summary.worktreeId, + readsWorktreeRemovalMarker(context), + pendingAtScan + ) + return worktrees === result.worktrees + ? result + : { + ...result, + worktrees, + totalCount: result.totalCount - (result.worktrees.length - worktrees.length) + } +} diff --git a/src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts b/src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts new file mode 100644 index 000000000000..8fd770875878 --- /dev/null +++ b/src/main/runtime/runtime-interrupted-local-worktree-removal.test.ts @@ -0,0 +1,361 @@ +// Real-binary coverage for finishing a removal a quit or crash interrupted: what is left has to +// come from Git and disk, whatever point the earlier run reached. +import { execFile } from 'node:child_process' +import { existsSync } from 'node:fs' +import { mkdir, mkdtemp, realpath, unlink, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { promisify } from 'node:util' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { Repo } from '../../shared/repo-types' +import { removeTree } from '../../shared/windows-transient-lock-removal' +import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' +import type { Store } from '../persistence' +import type * as HostTreeRemoval from '../host-tree-removal' +import { removeHostTree } from '../host-tree-removal' +import type * as WorktreeGitFileRestore from '../git/worktree-git-file-restore' +import { restoreMissingWorktreeGitFile } from '../git/worktree-git-file-restore' +import { listWorktreesStrict } from '../git/worktree' +import { areWorktreePathsEqual } from '../git/worktree-path-comparison' +import { + acquireWatcherRemovalGate, + beginTerminalInstall, + beginWatcherInstall +} from '../ipc/watcher-removal-gate' +import { + _resetPendingWorktreeRemovalsForTests, + _settlePendingWorktreeRemovalsForTests, + loadWorktreeRemovalRecords, + resumeInterruptedWorktreeRemovals, + waitForPendingWorktreeRemoval +} from '../worktree-background-removal' +import { + readWorktreeRemovalRecords, + writeWorktreeRemovalRecords, + type WorktreeRemovalRecord +} from '../worktree-removal-records' +import { interruptedLocalWorktreeRemovalJob } from './runtime-interrupted-local-worktree-removal' + +vi.mock('../project-runtime-git-options', () => ({ + getLocalProjectWorktreeGitOptions: () => ({}) +})) +vi.mock('../host-tree-removal', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, removeHostTree: vi.fn(actual.removeHostTree) } +}) +vi.mock('../git/worktree-git-file-restore', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + restoreMissingWorktreeGitFile: vi.fn(actual.restoreMissingWorktreeGitFile) + } +}) + +const execFileAsync = promisify(execFile) + +let scratchDir = '' +let recordsDir = '' +let repoPath = '' +let worktreePath = '' +let repo: Repo + +async function git(args: string[], cwd = repoPath): Promise { + const { stdout } = await execFileAsync('git', args, { cwd }) + return stdout +} + +// Why parsed: Git prints forward slashes on Windows, so raw text never contains a joined path. +async function isRegistered(path: string): Promise { + return (await listWorktreesStrict(repoPath)).some((worktree) => + areWorktreePathsEqual(worktree.path, path) + ) +} + +beforeEach(async () => { + // realpath: macOS hands out /var/... temp paths while Git reports /private/var/.... + scratchDir = await realpath(await mkdtemp(join(tmpdir(), 'orca-interrupted-removal-'))) + recordsDir = join(scratchDir, 'profile') + repoPath = join(scratchDir, 'repo') + worktreePath = join(scratchDir, 'workspaces', 'feature') + await mkdir(recordsDir, { recursive: true }) + await mkdir(repoPath, { recursive: true }) + await git(['init', '-q']) + await git(['config', 'user.email', 'removal@example.invalid']) + await git(['config', 'user.name', 'Worktree Removal']) + await writeFile(join(repoPath, 'seed.txt'), 'seed\n') + await git(['add', '-A']) + await git(['commit', '-qm', 'seed']) + await git(['worktree', 'add', '-q', worktreePath, '-b', 'feature']) + repo = { id: 'repo-1', path: repoPath, displayName: 'repo', badgeColor: '', addedAt: 0 } +}) + +afterEach(async () => { + _resetPendingWorktreeRemovalsForTests() + vi.mocked(removeHostTree).mockClear() + await removeTree(scratchDir) +}) + +type FinishOutcome = + | ({ status: 'removed' } & RemoveWorktreeResult) + | { status: 'failed'; error: string } + +async function finishAfterRestart(options: { repoGone?: boolean; head?: string } = {}): Promise<{ + outcome: FinishOutcome + purged: string[] + remember: ReturnType +}> { + const record: WorktreeRemovalRecord = { + worktreeId: `repo-1::${worktreePath}`, + repoId: 'repo-1', + repoPath, + worktreePath, + branch: 'feature', + head: options.head ?? (await git(['rev-parse', 'feature'])).trim(), + deleteBranch: true, + force: false, + requestedAt: 1 + } + await writeWorktreeRemovalRecords(recordsDir, () => [record]) + await loadWorktreeRemovalRecords(recordsDir) + // A request that joins before the finish starts gets the finish's result. + const joined = waitForPendingWorktreeRemoval(record.worktreeId) + expect(joined).toBeDefined() + // Session restore runs before the resume: nothing may open a handle in the half-deleted checkout. + expect(() => beginTerminalInstall(worktreePath)).toThrow(/being removed/) + expect(() => beginWatcherInstall(worktreePath)).toThrow(/being removed/) + + const storeStub = { + getRepo: (id: string) => (id === repo.id && !options.repoGone ? repo : undefined), + getRepos: () => (options.repoGone ? [] : [repo]), + getWorktreeMeta: () => undefined + } + const purged: string[] = [] + const remember = vi.fn() + resumeInterruptedWorktreeRemovals((interrupted) => + interruptedLocalWorktreeRemovalJob(interrupted, { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the finish reads only repos and worktree metadata from the store here; git options and push-target cleanup are stubbed or short-circuit without a push target. + store: storeStub as unknown as Store, + // Takes the real gate synchronously, as the runtime's does. + acquireWatcherRemoval: async (path) => { + const gate = acquireWatcherRemovalGate(path) + expect(() => beginTerminalInstall(worktreePath)).toThrow(/being removed/) + return { finish: async () => gate.release() } + }, + closeWatchers: async () => {}, + preservedBranchCleanup: { + preserveHead: (result) => result ?? {}, + remember + }, + purge: ({ worktreeId }) => purged.push(worktreeId), + onRemoved: () => {}, + publish: () => {} + }) + ) + const outcome: FinishOutcome = await joined!.then( + (result) => ({ status: 'removed' as const, ...result }), + (error: unknown) => ({ status: 'failed' as const, error: String(error) }) + ) + await _settlePendingWorktreeRemovalsForTests() + expect(await readWorktreeRemovalRecords(recordsDir)).toEqual([]) + expect(waitForPendingWorktreeRemoval(record.worktreeId)).toBeUndefined() + // Released on every outcome, including a finish that ended before taking its own gate. + beginTerminalInstall(worktreePath)() + return { outcome, purged, remember } +} + +describe('finishing an interrupted worktree removal after a restart', () => { + it('finishes a checkout Git was still deleting, branch and metadata included', async () => { + // Git stopped partway: part of the checkout is gone, which reads as local changes. + await unlink(join(worktreePath, 'seed.txt')) + + const { outcome, purged } = await finishAfterRestart() + + expect(outcome).toMatchObject({ status: 'removed' }) + expect( + outcome && 'preservedBranch' in outcome ? outcome.preservedBranch : undefined + ).toBeUndefined() + expect(existsSync(worktreePath)).toBe(false) + expect(await isRegistered(worktreePath)).toBe(false) + expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(purged).toEqual([`repo-1::${worktreePath}`]) + }) + + it('lets Git finish a checkout whose .git file it had already deleted', async () => { + // Git deletes in directory order; without `.git` it refuses the checkout ("validation failed"). + await unlink(join(worktreePath, '.git')) + await unlink(join(worktreePath, 'seed.txt')) + + const { outcome, purged } = await finishAfterRestart() + + expect(outcome).toMatchObject({ status: 'removed' }) + expect(existsSync(worktreePath)).toBe(false) + expect(await isRegistered(worktreePath)).toBe(false) + expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(purged).toEqual([`repo-1::${worktreePath}`]) + // Git's process deleted it, not Orca's. + expect(removeHostTree).not.toHaveBeenCalled() + }) + + it('lets Git finish a relative-path checkout whose .git file it had already deleted', async (ctx) => { + await git(['worktree', 'remove', worktreePath]) + try { + await git(['worktree', 'add', '-q', '--relative-paths', worktreePath, 'feature']) + } catch { + // Git before 2.48 has no relative-path worktrees. + ctx.skip() + } + await unlink(join(worktreePath, '.git')) + await unlink(join(worktreePath, 'seed.txt')) + + const { outcome, purged } = await finishAfterRestart() + + expect(outcome).toMatchObject({ status: 'removed' }) + expect(existsSync(worktreePath)).toBe(false) + expect(await isRegistered(worktreePath)).toBe(false) + expect(purged).toEqual([`repo-1::${worktreePath}`]) + expect(removeHostTree).not.toHaveBeenCalled() + }) + + it('deletes the leftover itself only when Git cannot be pointed back at it', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + await unlink(join(worktreePath, '.git')) + vi.mocked(restoreMissingWorktreeGitFile).mockResolvedValueOnce(false) + + const { outcome, purged } = await finishAfterRestart() + + expect(outcome).toMatchObject({ status: 'removed' }) + expect(removeHostTree).toHaveBeenCalledWith(worktreePath) + expect(existsSync(worktreePath)).toBe(false) + expect(await isRegistered(worktreePath)).toBe(false) + expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(purged).toEqual([`repo-1::${worktreePath}`]) + }) + + it('leaves a different checkout created at the same path since the quit', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const head = (await git(['rev-parse', 'feature'])).trim() + await git(['worktree', 'remove', worktreePath]) + await git(['worktree', 'add', '-q', worktreePath, '-b', 'other']) + await writeFile(join(worktreePath, 'unsaved.txt'), 'work\n') + + const { outcome, purged } = await finishAfterRestart({ head }) + + expect(outcome).toMatchObject({ status: 'failed' }) + expect(existsSync(join(worktreePath, 'unsaved.txt'))).toBe(true) + expect(await isRegistered(worktreePath)).toBe(true) + expect(purged).toEqual([]) + }) + + it('leaves a repository created at the path after Git unregistered the checkout', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + await git(['worktree', 'remove', worktreePath]) + await mkdir(worktreePath, { recursive: true }) + await git(['init', '-q'], worktreePath) + await writeFile(join(worktreePath, 'unsaved.txt'), 'work\n') + + const { outcome, purged } = await finishAfterRestart() + + expect(outcome).toMatchObject({ status: 'failed' }) + expect(existsSync(join(worktreePath, 'unsaved.txt'))).toBe(true) + expect(removeHostTree).not.toHaveBeenCalled() + expect(purged).toEqual([]) + }) + + it('leaves the same branch checked out again at a new head', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const head = (await git(['rev-parse', 'feature'])).trim() + await git(['worktree', 'remove', worktreePath]) + await git(['worktree', 'add', '-q', worktreePath, 'feature']) + await writeFile(join(worktreePath, 'work.txt'), 'work\n') + await git(['add', '-A'], worktreePath) + await git(['commit', '-qm', 'work'], worktreePath) + + const { outcome, purged } = await finishAfterRestart({ head }) + + expect(outcome).toMatchObject({ status: 'failed' }) + expect(existsSync(join(worktreePath, 'work.txt'))).toBe(true) + expect(purged).toEqual([]) + }) + + it('leaves a locked checkout alone even when its .git file is gone', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + await git(['worktree', 'lock', worktreePath]) + await unlink(join(worktreePath, '.git')) + + const { outcome, purged } = await finishAfterRestart() + + expect(outcome).toMatchObject({ status: 'failed' }) + expect(existsSync(join(worktreePath, 'seed.txt'))).toBe(true) + expect(purged).toEqual([]) + }) + + it('deletes the branch when Git finished the checkout but the quit came before the branch', async () => { + await git(['worktree', 'remove', worktreePath]) + + const { outcome, purged } = await finishAfterRestart() + + expect(outcome).toMatchObject({ status: 'removed' }) + expect(await git(['branch', '--list', 'feature'])).toBe('') + expect(purged).toEqual([`repo-1::${worktreePath}`]) + }) + + it('keeps an unmerged branch, as a normal removal does', async () => { + await writeFile(join(worktreePath, 'work.txt'), 'work\n') + await git(['add', '-A'], worktreePath) + await git(['commit', '-qm', 'work'], worktreePath) + const head = (await git(['rev-parse', 'feature'])).trim() + await git(['worktree', 'remove', worktreePath]) + + const { outcome, remember } = await finishAfterRestart() + + expect(outcome).toMatchObject({ + status: 'removed', + preservedBranch: { branchName: 'feature', head } + }) + expect((await git(['rev-parse', 'feature'])).trim()).toBe(head) + expect(remember).toHaveBeenCalledWith( + `repo-1::${worktreePath}`, + undefined, + { preservedBranch: { branchName: 'feature', head } }, + head, + undefined + ) + }) + + it('treats a removal that fully finished as done', async () => { + const head = (await git(['rev-parse', 'feature'])).trim() + await git(['worktree', 'remove', worktreePath]) + await git(['branch', '-d', 'feature']) + + const { outcome, purged } = await finishAfterRestart({ head }) + + expect(outcome).toMatchObject({ status: 'removed' }) + expect( + outcome && 'preservedBranch' in outcome ? outcome.preservedBranch : undefined + ).toBeUndefined() + expect(purged).toEqual([`repo-1::${worktreePath}`]) + }) + + it('returns the row live when the finish fails, instead of retrying unseen', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + // Another Git client locked it while Orca was not running. + await git(['worktree', 'lock', worktreePath]) + + const { outcome, purged } = await finishAfterRestart() + + expect(outcome).toMatchObject({ status: 'failed' }) + expect(existsSync(worktreePath)).toBe(true) + expect(await isRegistered(worktreePath)).toBe(true) + expect(purged).toEqual([]) + }) + + it('drops a record whose repo Orca no longer has', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + + const { outcome, purged } = await finishAfterRestart({ repoGone: true }) + + expect(outcome).toMatchObject({ status: 'removed' }) + expect(purged).toEqual([]) + expect(existsSync(worktreePath)).toBe(true) + }) +}) diff --git a/src/main/runtime/runtime-interrupted-local-worktree-removal.ts b/src/main/runtime/runtime-interrupted-local-worktree-removal.ts new file mode 100644 index 000000000000..ca50faf3b8f1 --- /dev/null +++ b/src/main/runtime/runtime-interrupted-local-worktree-removal.ts @@ -0,0 +1,201 @@ +import { lstat } from 'node:fs/promises' +import { join } from 'node:path' +import type { RemoveWorktreeResult } from '../../shared/worktree/create-types' +import type { GitWorktreeInfo } from '../../shared/worktree/types' +import { assertWorktreeUnlockedForRemoval } from '../../shared/worktree/removal' +import { LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' +import type { Store } from '../persistence' +import { + releaseStartupRemovalFence, + type BackgroundWorktreeRemovalJob +} from '../worktree-background-removal' +import { resolveWorktreeRemovalMetadata } from '../worktree-removal-repo-owner' +import type { RuntimePreservedBranchCleanup } from './runtime-preserved-branch-cleanup' +import { listWorktreesStrict } from '../git/worktree' +import { finishUnregisteredWorktreeRemoval } from '../git/worktree-removal' +import { getErrorCode, normalizeLocalBranchRef } from '../git/worktree-operation-options' +import { restoreMissingWorktreeGitFile } from '../git/worktree-git-file-restore' +import { areWorktreePathsEqual } from '../git/worktree-path-comparison' +import { getLocalProjectWorktreeGitOptions } from '../project-runtime-git-options' +import { findRegisteredDeletableWorktree } from '../worktree-removal-safety' +import { CLIENT_REMOVAL_HOME } from '../worktree-removal-home-guard' +import type { WorktreeRemovalRecord } from '../worktree-removal-records' +import { + cleanupRemovedWorktreePushTarget, + finishRuntimeLocalWorktreeRemoval, + type RuntimeLocalWorktreeRemovalFinishArgs +} from './runtime-registered-local-worktree-removal' + +type InterruptedWorktreeRemovalHost = { + store: Store + acquireWatcherRemoval: (path: string) => Promise<{ finish: (removed: boolean) => Promise }> + closeWatchers: (path: string) => Promise + preservedBranchCleanup: Pick + /** Drops the worktree's host state (metadata, history, caches), as every removal path does. */ + purge: (record: WorktreeRemovalRecord) => void + onRemoved: (record: WorktreeRemovalRecord) => void + publish: (repoId: string) => void +} + +/** The background job that finishes one interrupted removal on this host. */ +export function interruptedLocalWorktreeRemovalJob( + record: WorktreeRemovalRecord, + host: InterruptedWorktreeRemovalHost +): BackgroundWorktreeRemovalJob { + return { + run: async (stopSignal) => { + const removedPushTarget = resolveWorktreeRemovalMetadata( + host.store, + record.repoId, + record.worktreeId, + LOCAL_EXECUTION_HOST_ID + )?.pushTarget + const result = await finishInterruptedLocalWorktreeRemoval({ + record, + store: host.store, + stopSignal, + removedPushTarget, + acquireWatcherRemoval: (path) => { + // Why same tick: the gate takes over the loading fence's path with no gap for a spawn. + releaseStartupRemovalFence(record.worktreeId) + return host.acquireWatcherRemoval(path) + }, + closeWatchers: host.closeWatchers, + preserveBranchHead: (result, fallbackHead) => + host.preservedBranchCleanup.preserveHead(result, fallbackHead), + // remember() clears the cleanup target when no branch was preserved. + finishRemoval: (result, _rememberBranch, fallbackHead) => { + host.preservedBranchCleanup.remember( + record.worktreeId, + undefined, + result, + fallbackHead, + removedPushTarget + ) + host.purge(record) + } + }) + host.onRemoved(record) + return result + }, + publish: () => host.publish(record.repoId) + } +} + +type InterruptedLocalWorktreeRemovalArgs = Pick< + RuntimeLocalWorktreeRemovalFinishArgs, + 'removedPushTarget' | 'closeWatchers' | 'preserveBranchHead' | 'finishRemoval' +> & { + store: Store + record: WorktreeRemovalRecord + acquireWatcherRemoval: (path: string) => Promise<{ finish: (removed: boolean) => Promise }> + stopSignal: AbortSignal +} + +/** + * Finishes a removal a quit or crash interrupted. What is left comes from Git and disk, not the + * record, so a delete Git already finished (fully or partly) completes the same way. + */ +async function finishInterruptedLocalWorktreeRemoval( + args: InterruptedLocalWorktreeRemovalArgs +): Promise { + const { record, store } = args + const repo = store.getRepo(record.repoId) + if (!repo) { + console.warn(`[worktrees] dropping removal of ${record.worktreePath}: its repo is gone`) + return {} + } + const localOptions = getLocalProjectWorktreeGitOptions(store, repo) + const finishArgs: RuntimeLocalWorktreeRemovalFinishArgs = { + store, + removedPushTarget: args.removedPushTarget, + closeWatchers: args.closeWatchers, + preserveBranchHead: args.preserveBranchHead, + finishRemoval: args.finishRemoval, + repo, + localOptions, + // Why force: Git already deleted part of the checkout, which reads as local changes. + force: true, + deleteBranch: record.deleteBranch, + target: { id: record.worktreeId } + } + const worktrees = await listWorktreesStrict(repo.path, localOptions) + const registered = worktrees.some((worktree) => + areWorktreePathsEqual(worktree.path, record.worktreePath) + ) + const deletable = registered + ? findRegisteredDeletableWorktree( + repo.path, + record.worktreePath, + worktrees, + CLIENT_REMOVAL_HOME + ) + : undefined + if (registered && !deletable) { + throw new Error( + `Worktree registration changed during deletion: ${record.worktreePath}. Retry deletion.` + ) + } + const gitLink = await readCheckoutGitLink(record.worktreePath) + // Why: the finish forces, so a checkout created at this path since the quit must not be taken. + // Git deletes the checkout, `.git` included, before it drops the registration, so a `.git` at an + // unregistered path belongs to a new checkout. + if (deletable ? !isRecordedCheckout(deletable, record) : gitLink === 'present') { + throw new Error( + `A different checkout is now at ${record.worktreePath}; Orca left it in place. Delete it again to remove it.` + ) + } + // Why: Git deletes `.git` wherever it falls in directory order (early on NTFS) and refuses to + // remove a checkout left without it; restoring the link from Git's admin entry lets Git finish. + let gitCanRemove = !!deletable + if (deletable && gitLink === 'missing') { + assertWorktreeUnlockedForRemoval(deletable) + gitCanRemove = await restoreMissingWorktreeGitFile(repo.path, deletable.path, localOptions) + } + const gate = await args.acquireWatcherRemoval(record.worktreePath) + if (deletable && gitCanRemove) { + return finishRuntimeLocalWorktreeRemoval(finishArgs, deletable, gate, args.stopSignal) + } + // Unregistered, or no admin entry claims the checkout so Git cannot validate it: the leftover is + // deleted in this process as a last resort, then pruned. + let result: RemoveWorktreeResult + let removed = false + try { + result = await finishUnregisteredWorktreeRemoval( + repo.path, + record.worktreePath, + record.deleteBranch && record.branch ? { name: record.branch, head: record.head } : null, + localOptions + ) + removed = true + } finally { + await gate.finish(removed) + } + await cleanupRemovedWorktreePushTarget(finishArgs) + args.finishRemoval(result, true, record.head) + return result +} + +function isRecordedCheckout(worktree: GitWorktreeInfo, record: WorktreeRemovalRecord): boolean { + return ( + normalizeLocalBranchRef(worktree.branch) === record.branch && + (!record.head || worktree.head === record.head) + ) +} + +/** `missing`: the checkout directory is there without its `.git`; unreadable counts as present. */ +async function readCheckoutGitLink( + worktreePath: string +): Promise<'no-checkout' | 'missing' | 'present'> { + try { + await lstat(worktreePath) + } catch { + return 'no-checkout' + } + try { + await lstat(join(worktreePath, '.git')) + return 'present' + } catch (error) { + return getErrorCode(error) === 'ENOENT' ? 'missing' : 'present' + } +} diff --git a/src/main/runtime/runtime-local-worktree-create-candidate.ts b/src/main/runtime/runtime-local-worktree-create-candidate.ts index aacf6ae0b495..7af0f8b99104 100644 --- a/src/main/runtime/runtime-local-worktree-create-candidate.ts +++ b/src/main/runtime/runtime-local-worktree-create-candidate.ts @@ -32,6 +32,7 @@ import { resolveCreateBranchName } from './runtime-worktree-create-git' import { runtimePathExists } from './runtime-worktree-filesystem' +import { findPendingWorktreeRemovalConflict } from '../worktree-background-removal' import type { RuntimeStore } from './runtime-store-contract' import type { HostedReviewExecutionOptions } from '../source-control/hosted-review-git-options' @@ -189,7 +190,11 @@ export async function resolveRuntimeLocalWorktreeCreateCandidate(args: { computeWorktreePath(effectiveSanitizedName, args.repo.path, args.worktreePathSettings), args.workspaceRoot ) - if (!(await runtimePathExists(worktreePath))) { + // Why the pending check: Orca still owns this path until its background removal settles. + if ( + !findPendingWorktreeRemovalConflict(args.repo.path, { worktreePath }) && + !(await runtimePathExists(worktreePath)) + ) { worktreePathResolved = true break } diff --git a/src/main/runtime/runtime-registered-local-worktree-removal.ts b/src/main/runtime/runtime-registered-local-worktree-removal.ts index 011d30207fe2..a5e966b1471a 100644 --- a/src/main/runtime/runtime-registered-local-worktree-removal.ts +++ b/src/main/runtime/runtime-registered-local-worktree-removal.ts @@ -26,8 +26,28 @@ import { findRegisteredDeletableWorktree } from '../worktree-removal-safety' import { CLIENT_REMOVAL_HOME } from '../worktree-removal-home-guard' import type { RuntimeStore } from './runtime-store-contract' import type { RuntimeWorktreeRemovalTarget } from './runtime-worktree-selection' +import { + removesInBackground, + startBackgroundWorktreeRemoval, + waitForPendingWorktreeRemoval +} from '../worktree-background-removal' +import { runSerializedWorktreeRemovalAcceptance } from '../worktree-removal-acceptance-queue' + +/** Runs after the previous same-repo removal was accepted; see runSerializedWorktreeRemovalAcceptance. */ +export function removeRuntimeRegisteredLocalWorktree( + args: Parameters[0] +): Promise { + return runSerializedWorktreeRemovalAcceptance( + args.repo.path, + async (): Promise => + // Why: another client's removal of this worktree may have been accepted during the wait. + waitForPendingWorktreeRemoval(args.target.id) + ? { removing: true } + : acceptRuntimeRegisteredLocalWorktreeRemoval(args) + ) +} -export async function removeRuntimeRegisteredLocalWorktree(args: { +async function acceptRuntimeRegisteredLocalWorktreeRemoval(args: { repo: Repo target: RuntimeWorktreeRemovalTarget registeredWorktree: GitWorktreeInfo @@ -54,6 +74,9 @@ export async function removeRuntimeRegisteredLocalWorktree(args: { // Why: re-read after the archive hook, which can move the branch out from under the pre-hook row. fallbackHead: string | undefined ) => void + /** Fired when Git finished, not on acceptance. */ + onRemoved: () => void + publish: () => void }): Promise { const { repo, registeredWorktree, localOptions } = args const canonicalPath = registeredWorktree.path @@ -121,20 +144,81 @@ export async function removeRuntimeRegisteredLocalWorktree(args: { } } - let removalResult: RemoveWorktreeResult | undefined const gate = await args.acquireWatcherRemoval(canonicalPath) - let completed = false + let accepted = false try { await args.stopPtys() if (linkedPaths.length > 0) { await removeWorktreeLinkedPaths(canonicalPath, linkedPaths) } + accepted = true + } finally { + if (!accepted) { + await gate.finish(false) + } + } + const acceptedFields = { + ...(archiveHookOverride ? { archiveHookOverride } : {}), + ...(warning ? { warning } : {}) + } + if (!removesInBackground(canonicalPath, localOptions)) { + const result = await finishRuntimeLocalWorktreeRemoval(args, refreshed, gate) + args.publish() + return { ...result, ...acceptedFields } + } + // Why detached: every refusal above already ran, and Git's 20-35 s delete must finish even when the + // request that asked for it times out; other views read the host's `removing` marker meanwhile. + void startBackgroundWorktreeRemoval({ + removal: { + worktreeId: args.target.id, + repoId: repo.id, + repoPath: repo.path, + worktree: refreshed, + deleteBranch: args.deleteBranch, + force: args.force + }, + run: async (stopSignal) => { + const result = await finishRuntimeLocalWorktreeRemoval(args, refreshed, gate, stopSignal) + args.onRemoved() + return result + }, + publish: () => args.publish() + }) + return { removing: true, ...acceptedFields } +} + +export type RuntimeLocalWorktreeRemovalFinishArgs = Pick< + Parameters[0], + | 'repo' + | 'removedPushTarget' + | 'store' + | 'localOptions' + | 'force' + | 'deleteBranch' + | 'closeWatchers' + | 'preserveBranchHead' + | 'finishRemoval' +> & { target: { id: string } } + +/** Git's delete and everything after it; the refusals and teardown before it already ran. */ +export async function finishRuntimeLocalWorktreeRemoval( + args: RuntimeLocalWorktreeRemovalFinishArgs, + refreshed: GitWorktreeInfo, + gate: { finish: (removed: boolean) => Promise }, + checkoutDeleteSignal?: AbortSignal +): Promise { + const { repo, localOptions } = args + const canonicalPath = refreshed.path + let removalResult: RemoveWorktreeResult | undefined + let completed = false + try { try { removalResult = args.preserveBranchHead( await removeWorktree(repo.path, canonicalPath, args.force, { ...(!args.deleteBranch ? { deleteBranch: args.deleteBranch } : {}), knownRemovedWorktree: refreshed, - ...localOptions + ...localOptions, + ...(checkoutDeleteSignal ? { checkoutDeleteSignal } : {}) }), refreshed.head ) @@ -162,13 +246,10 @@ export async function removeRuntimeRegisteredLocalWorktree(args: { await gitExecFileAsync(['worktree', 'prune'], { cwd: repo.path, ...localOptions }).catch( () => {} ) - await cleanupPushTarget(args) + await cleanupRemovedWorktreePushTarget(args) args.finishRemoval(undefined, false, refreshed.head) completed = true - return { - ...(archiveHookOverride ? { archiveHookOverride } : {}), - ...(warning ? { warning } : {}) - } + return {} } else { throw new Error(formatWorktreeRemovalError(error, canonicalPath, args.force)) } @@ -179,17 +260,13 @@ export async function removeRuntimeRegisteredLocalWorktree(args: { } finally { await gate.finish(completed) } - await cleanupPushTarget(args) + await cleanupRemovedWorktreePushTarget(args) args.finishRemoval(removalResult, true, refreshed.head) - return { - ...removalResult, - ...(archiveHookOverride ? { archiveHookOverride } : {}), - ...(warning ? { warning } : {}) - } + return removalResult ?? {} } -async function cleanupPushTarget( - args: Parameters[0] +export async function cleanupRemovedWorktreePushTarget( + args: RuntimeLocalWorktreeRemovalFinishArgs ): Promise { await cleanupUnusedWorktreePushTargetRemote( args.repo.path, diff --git a/src/main/runtime/runtime-worktree-selection.ts b/src/main/runtime/runtime-worktree-selection.ts index 230f2952da84..2a00d01ced15 100644 --- a/src/main/runtime/runtime-worktree-selection.ts +++ b/src/main/runtime/runtime-worktree-selection.ts @@ -39,6 +39,8 @@ export type RemoveManagedWorktreeOptions = { /** Waives a FAILED archive hook (#19334). Never implied by `force`, never by `runHooks`. */ allowFailedArchiveHook?: boolean hostId?: string + /** Reply with the finished delete's result; otherwise a background delete replies on acceptance. */ + waitForBackgroundRemoval?: boolean } export function getRuntimeWorktreeRemovalOptionsKey( diff --git a/src/main/startup/main-process-quit.ts b/src/main/startup/main-process-quit.ts index f104a3ec3439..5a89c9e283f6 100644 --- a/src/main/startup/main-process-quit.ts +++ b/src/main/startup/main-process-quit.ts @@ -17,6 +17,7 @@ import { browserManager } from '../browser/browser-manager' import { stopCodexStateDbBackfillRecoveries } from '../codex/codex-state-db-backfill-recovery' import { stopCodexAccountSessionBridges } from '../codex/codex-account-session-bridge' import { awaitPackedRefsLockRelease } from '../git/local-repo-ref-maintenance' +import { stopBackgroundWorktreeRemovals } from '../worktree-background-removal' import { settleTeardownWithinDeadline, settleWithinMs } from '../quit-teardown-deadline' import { quitTeardownStartGate } from '../quit-teardown-start-gate' import { setUnreadDockBadgeCount } from '../dock/unread-badge' @@ -152,6 +153,8 @@ function installWillQuitHandler(): void { REF_MAINTENANCE_QUIT_DEADLINE_MS ).then(() => {}) state.uninstallRepoMaintenanceIdleGate = null + // Why stop, not wait: a delete can run for minutes, and its record makes the next start finish it. + stopBackgroundWorktreeRemovals() agentHookServer.stop() // Why Windows only: POSIX hooks short-circuit on ORCA_PANE_KEY, while Windows must register a // bare script path that cannot express the guard and would otherwise keep spawning after quit. diff --git a/src/main/startup/main-process-ready-runtime.ts b/src/main/startup/main-process-ready-runtime.ts index 26b920652df1..ac8fc8f0d32b 100644 --- a/src/main/startup/main-process-ready-runtime.ts +++ b/src/main/startup/main-process-ready-runtime.ts @@ -32,6 +32,7 @@ import { import { initializeMainProcessAutomations } from './main-process-automations' import { initializeMainProcessPlugins } from './main-process-plugins' import { collectWorktreeTrashSweepRoots, sweepStaleWorktreeTrash } from '../worktree-trash' +import { loadWorktreeRemovalRecords } from '../worktree-background-removal' import { runAfterFirstWindowShown } from './first-window-deferral' import { logStartupMilestone } from './startup-diagnostics' @@ -43,6 +44,8 @@ export async function initializeReadyRuntimeServices(): Promise { if (!store) { throw new Error('Store must be initialized before ready services') } + // Why before any listing: a delete a quit or crash interrupted must show as Deleting from first paint. + await loadWorktreeRemovalRecords(store.getProfileStorageDirectory()) initializeMainProcessObservers() initializeMainProcessAccountServices() const runtime = initializeMainProcessRuntime() @@ -77,16 +80,17 @@ export async function initializeReadyRuntimeServices(): Promise { // Why: externally started serve-sim processes must stay independent — only Orca-managed/attached helpers belong to a workspace. state.emulatorBridge = new EmulatorBridge() runtime.setEmulatorBridge(state.emulatorBridge) - // Why: worktree deletion renames the checkout aside and deletes it in the background, so a quit or - // crash mid-delete can leave the moved directory on disk. Why deferred: the sweep's recursive - // readdir/rm runs on the same libuv threadpool the window's first paint and worktree-catalog - // hydration are reading disk on, and nothing on the startup path consumes its result. + // Why: older releases renamed removed checkouts into a trash root and deleted them in the background, + // so a quit mid-delete left directories on disk; drain them. Removals this version recorded are + // finished by the same delete. Why deferred: both touch disk on the same libuv threadpool the + // window's first paint and worktree-catalog hydration read on, and startup consumes neither. runAfterFirstWindowShown(() => { void sweepStaleWorktreeTrash( collectWorktreeTrashSweepRoots(store.getRepos(), store.getSettings()) ).catch((error) => { console.warn('[worktrees] Failed to sweep leftover worktree directories:', error) }) + runtime.finishInterruptedWorktreeRemovals() }, WORKTREE_TRASH_SWEEP_FALLBACK_MS) nativeTheme.themeSource = store.getSettings().theme ?? 'system' // Why (#16441): the real-home grant runs a codex app-server session. It stays diff --git a/src/main/worktree-background-removal-records.test.ts b/src/main/worktree-background-removal-records.test.ts new file mode 100644 index 000000000000..6707acfe271d --- /dev/null +++ b/src/main/worktree-background-removal-records.test.ts @@ -0,0 +1,257 @@ +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { + _resetPendingWorktreeRemovalsForTests, + _settlePendingWorktreeRemovalsForTests, + loadWorktreeRemovalRecords, + projectPendingWorktreeRemovals, + resumeInterruptedWorktreeRemovals, + snapshotPendingWorktreeRemovals, + startBackgroundWorktreeRemoval, + stopBackgroundWorktreeRemovals, + waitForPendingWorktreeRemoval +} from './worktree-background-removal' +import type * as WorktreeRemovalRecords from './worktree-removal-records' +import { + readWorktreeRemovalRecords, + worktreeRemovalRecordsFile, + writeWorktreeRemovalRecords +} from './worktree-removal-records' + +vi.mock('./worktree-removal-records', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + writeWorktreeRemovalRecords: vi.fn(actual.writeWorktreeRemovalRecords) + } +}) + +const removal = { + worktreeId: 'repo-1::/work/feature', + repoId: 'repo-1', + repoPath: '/work/repo', + worktree: { path: '/work/feature', branch: 'refs/heads/feature', head: 'abc' }, + deleteBranch: true, + force: false +} +const isPending = (): boolean => waitForPendingWorktreeRemoval(removal.worktreeId) !== undefined +let directory = '' + +beforeEach(async () => { + directory = await mkdtemp(join(tmpdir(), 'orca-worktree-removal-records-')) + await loadWorktreeRemovalRecords(directory) +}) + +afterEach(async () => { + vi.useRealTimers() + _resetPendingWorktreeRemovalsForTests() + await rm(directory, { recursive: true, force: true }) +}) + +/** A delete Git never finishes on its own, like one a quit or crash cuts short. */ +function interruptedJob(): { + run: (stopSignal: AbortSignal) => Promise + publish: () => void + started: () => boolean + stopped: () => boolean +} { + let started = false + let stopped = false + return { + run: (stopSignal) => + new Promise((_resolve, reject) => { + started = true + stopSignal.addEventListener('abort', () => { + stopped = true + reject(new Error('The operation was aborted.')) + }) + }), + publish: vi.fn(), + started: () => started, + stopped: () => stopped + } +} + +describe('durable worktree removal records', () => { + it('writes the record before Git starts and clears it once the delete succeeds', async () => { + let recordedAtGitStart: unknown[] = [] + void startBackgroundWorktreeRemoval({ + removal, + run: async () => { + recordedAtGitStart = await readWorktreeRemovalRecords(directory) + return {} + }, + publish: () => {} + }) + await _settlePendingWorktreeRemovalsForTests() + + expect(recordedAtGitStart).toEqual([ + { + worktreeId: removal.worktreeId, + repoId: 'repo-1', + repoPath: '/work/repo', + worktreePath: '/work/feature', + branch: 'feature', + head: 'abc', + deleteBranch: true, + force: false, + requestedAt: expect.any(Number) + } + ]) + expect(await readWorktreeRemovalRecords(directory)).toEqual([]) + }) + + it('clears the record when the delete fails so the row returns live and retryable', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const publish = vi.fn() + const result = startBackgroundWorktreeRemoval({ + removal, + run: async () => { + throw new Error('Permission denied') + }, + publish + }) + await expect(result).rejects.toThrow('Permission denied') + await _settlePendingWorktreeRemovalsForTests() + + expect(await readWorktreeRemovalRecords(directory)).toEqual([]) + expect(isPending()).toBe(false) + // Once on acceptance, once after the row left the table. + expect(publish).toHaveBeenCalledTimes(2) + }) + + it('starts Git after a bounded wait when the record write stalls', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + vi.useFakeTimers({ toFake: ['setTimeout', 'clearTimeout'] }) + vi.mocked(writeWorktreeRemovalRecords).mockReturnValueOnce(new Promise(() => {})) + const run = vi.fn(async () => ({})) + void startBackgroundWorktreeRemoval({ + removal, + run, + publish: () => {} + }) + + await vi.advanceTimersByTimeAsync(1_000) + expect(run).not.toHaveBeenCalled() + await vi.advanceTimersByTimeAsync(1_000) + expect(run).toHaveBeenCalledTimes(1) + vi.useRealTimers() + await _settlePendingWorktreeRemovalsForTests() + }) + + it('replies without waiting for the record to be cleared on disk', async () => { + let finishClear = (): void => {} + const publish = vi.fn() + const result = startBackgroundWorktreeRemoval({ + removal, + run: async () => { + vi.mocked(writeWorktreeRemovalRecords).mockReturnValueOnce( + new Promise((resolve) => { + finishClear = resolve + }) + ) + return {} + }, + publish + }) + + await expect(result).resolves.toEqual({}) + expect(publish).toHaveBeenCalledTimes(2) + finishClear() + await _settlePendingWorktreeRemovalsForTests() + }) + + it('stops Git on quit without waiting and keeps the record for the next start', async () => { + const job = interruptedJob() + void startBackgroundWorktreeRemoval({ removal, ...job }) + await vi.waitFor(() => expect(job.started()).toBe(true)) + + expect(stopBackgroundWorktreeRemovals()).toBeUndefined() + expect(job.stopped()).toBe(true) + await _settlePendingWorktreeRemovalsForTests() + + expect(await readWorktreeRemovalRecords(directory)).toHaveLength(1) + // Only the start was published: a stopped delete is neither removed nor failed. + expect(job.publish).toHaveBeenCalledTimes(1) + }) + + it('marks the row as removing after a restart and finishes it with the same job', async () => { + const job = interruptedJob() + void startBackgroundWorktreeRemoval({ removal, ...job }) + await vi.waitFor(() => expect(job.started()).toBe(true)) + stopBackgroundWorktreeRemovals() + await _settlePendingWorktreeRemovalsForTests() + + // Restart: nothing in memory, only the file. + _resetPendingWorktreeRemovalsForTests() + expect(isPending()).toBe(false) + await loadWorktreeRemovalRecords(directory) + const joined = waitForPendingWorktreeRemoval(removal.worktreeId) + const rows: { id: string; hostId?: undefined }[] = [ + { id: removal.worktreeId }, + { id: 'repo-1::/work/other' } + ] + expect( + projectPendingWorktreeRemovals(rows, (row) => row.id, true, snapshotPendingWorktreeRemovals()) + ).toEqual([{ id: removal.worktreeId, removing: true }, { id: 'repo-1::/work/other' }]) + expect( + projectPendingWorktreeRemovals( + rows, + (row) => row.id, + false, + snapshotPendingWorktreeRemovals() + ) + ).toEqual([{ id: 'repo-1::/work/other' }]) + + const publish = vi.fn() + const resumed: string[] = [] + resumeInterruptedWorktreeRemovals((record) => ({ + run: async () => { + resumed.push(`${record.worktreePath} ${record.branch} ${record.deleteBranch}`) + return {} + }, + publish + })) + await _settlePendingWorktreeRemovalsForTests() + + expect(resumed).toEqual(['/work/feature feature true']) + expect(isPending()).toBe(false) + expect(await readWorktreeRemovalRecords(directory)).toEqual([]) + // A request that joined after the restart gets the resumed delete's result. + await expect(joined).resolves.toEqual({}) + expect(publish).toHaveBeenCalledTimes(1) + }) + + it('does not start a second job for a removal that is still running', async () => { + void startBackgroundWorktreeRemoval({ removal, ...interruptedJob() }) + const jobFor = vi.fn() + resumeInterruptedWorktreeRemovals(jobFor) + expect(jobFor).not.toHaveBeenCalled() + stopBackgroundWorktreeRemovals() + await _settlePendingWorktreeRemovalsForTests() + }) + + it('reads back only well-formed records from the file', async () => { + const valid = { + worktreeId: 'repo-1::/work/a', + repoId: 'repo-1', + repoPath: '/work/repo', + worktreePath: '/work/a', + branch: '', + head: '', + deleteBranch: false, + force: true, + requestedAt: 5 + } + await writeFile( + worktreeRemovalRecordsFile(directory), + JSON.stringify({ version: 1, removals: [valid, { worktreeId: 'x' }, null] }) + ) + expect(await readWorktreeRemovalRecords(directory)).toEqual([valid]) + + await writeFile(worktreeRemovalRecordsFile(directory), '{not json') + expect(await readWorktreeRemovalRecords(directory)).toEqual([]) + }) +}) diff --git a/src/main/worktree-background-removal.test.ts b/src/main/worktree-background-removal.test.ts new file mode 100644 index 000000000000..2ddc1952e8eb --- /dev/null +++ b/src/main/worktree-background-removal.test.ts @@ -0,0 +1,254 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { ExecutionHostId } from '../shared/execution-host' +import { + _resetPendingWorktreeRemovalsForTests, + _settlePendingWorktreeRemovalsForTests, + assertNoPendingWorktreeRemovalConflict, + finishAcceptedWorktreeRemoval, + projectPendingWorktreeRemovals, + removesInBackground, + snapshotPendingWorktreeRemovals, + startBackgroundWorktreeRemoval, + waitForPendingWorktreeRemoval +} from './worktree-background-removal' + +const removal = { + worktreeId: 'repo-1::/work/feature', + repoId: 'repo-1', + repoPath: '/work/repo', + worktree: { path: '/work/feature', branch: 'refs/heads/feature', head: 'abc' }, + deleteBranch: true, + force: false +} +const isPending = (hostId?: ExecutionHostId): boolean => + waitForPendingWorktreeRemoval(removal.worktreeId, hostId) !== undefined + +function deferred(): { promise: Promise; resolve: (value: T) => void } { + let resolve!: (value: T) => void + const promise = new Promise((settle) => { + resolve = settle + }) + return { promise, resolve } +} + +describe('background worktree removal', () => { + afterEach(() => { + _resetPendingWorktreeRemovalsForTests() + }) + + it('resolves with the delete result once Git finishes, after the row has left the table', async () => { + const git = deferred<{ preservedBranch: { branchName: string; head: string } }>() + const pendingAtPublish: boolean[] = [] + const result = startBackgroundWorktreeRemoval({ + removal, + run: () => git.promise, + publish: () => pendingAtPublish.push(isPending()) + }) + + expect(isPending()).toBe(true) + expect(pendingAtPublish).toEqual([true]) + + git.resolve({ preservedBranch: { branchName: 'feature', head: 'abc' } }) + await expect(result).resolves.toEqual({ + preservedBranch: { branchName: 'feature', head: 'abc' } + }) + await _settlePendingWorktreeRemovalsForTests() + expect(isPending()).toBe(false) + // A refetch the end notice triggers must not see the row as still removing. + expect(pendingAtPublish).toEqual([true, false]) + }) + + it('gives a request that joins a running delete that delete’s result', async () => { + const git = deferred<{ preservedBranch: { branchName: string; head: string } }>() + const run = vi.fn(() => git.promise) + void startBackgroundWorktreeRemoval({ removal, run, publish: () => {} }) + + const joined = finishAcceptedWorktreeRemoval( + { removing: true, warning: 'hook skipped' }, + removal.worktreeId + ) + git.resolve({ preservedBranch: { branchName: 'feature', head: 'abc' } }) + + await expect(joined).resolves.toEqual({ + warning: 'hook skipped', + preservedBranch: { branchName: 'feature', head: 'abc' } + }) + expect(run).toHaveBeenCalledTimes(1) + }) + + it('answers a request whose delete a concurrent removal of the same worktree replaced', async () => { + // The desktop IPC and runtime RPC removal paths coalesce separately, so both can be accepted. + const first = deferred>() + const second = deferred<{ preservedBranch: { branchName: string; head: string } }>() + const firstResult = startBackgroundWorktreeRemoval({ + removal, + run: () => first.promise, + publish: () => {} + }) + const secondResult = startBackgroundWorktreeRemoval({ + removal, + run: () => second.promise, + publish: () => {} + }) + + first.resolve({}) + await expect(firstResult).resolves.toEqual({}) + expect(isPending()).toBe(true) + + second.resolve({ preservedBranch: { branchName: 'feature', head: 'abc' } }) + await expect(secondResult).resolves.toEqual({ + preservedBranch: { branchName: 'feature', head: 'abc' } + }) + await _settlePendingWorktreeRemovalsForTests() + expect(isPending()).toBe(false) + }) + + it('rejects with the delete error and clears the row so a retry starts over', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + const result = startBackgroundWorktreeRemoval({ + removal, + run: async () => { + throw new Error('Failed to delete worktree at /work/feature. Permission denied') + }, + publish: () => {} + }) + await expect(result).rejects.toThrow('Permission denied') + await _settlePendingWorktreeRemovalsForTests() + expect(isPending()).toBe(false) + }) + + it('keeps the table consistent when publishing throws', async () => { + vi.spyOn(console, 'error').mockImplementation(() => {}) + void startBackgroundWorktreeRemoval({ + removal, + run: async () => ({}), + publish: () => { + throw new Error('window gone') + } + }) + await _settlePendingWorktreeRemovalsForTests() + expect(isPending()).toBe(false) + }) + + it('refuses a create at the same path or branch while Git deletes', async () => { + const git = deferred>() + void startBackgroundWorktreeRemoval({ + removal, + run: () => git.promise, + publish: () => {} + }) + + expect(() => + assertNoPendingWorktreeRemovalConflict('/work/repo', { worktreePath: '/work/feature' }) + ).toThrow('Cleanup is pending; try again shortly.') + expect(() => + assertNoPendingWorktreeRemovalConflict('/work/repo', { branch: 'refs/heads/feature' }) + ).toThrow('Cleanup is pending; try again shortly.') + expect(() => + assertNoPendingWorktreeRemovalConflict('/work/other-repo', { branch: 'feature' }) + ).not.toThrow() + expect(() => + assertNoPendingWorktreeRemovalConflict('/work/repo', { + worktreePath: '/work/feature-2', + branch: 'feature-2' + }) + ).not.toThrow() + + git.resolve({}) + await _settlePendingWorktreeRemovalsForTests() + expect(() => + assertNoPendingWorktreeRemovalConflict('/work/repo', { worktreePath: '/work/feature' }) + ).not.toThrow() + }) + + it('answers only for this host: a same-id row on an SSH host is not being removed here', () => { + void startBackgroundWorktreeRemoval({ + removal, + run: () => new Promise(() => {}), + publish: () => {} + }) + expect(isPending('local')).toBe(true) + expect(isPending('ssh:box')).toBe(false) + }) + + it('marks rows for clients that read the marker and omits them for clients that do not', () => { + void startBackgroundWorktreeRemoval({ + removal, + run: () => new Promise(() => {}), + publish: () => {} + }) + const rows = [ + { id: removal.worktreeId, hostId: 'local' as const }, + { id: 'repo-1::/work/other' }, + { id: removal.worktreeId, hostId: 'ssh:box' as const } + ] + + expect( + projectPendingWorktreeRemovals(rows, (row) => row.id, true, snapshotPendingWorktreeRemovals()) + ).toEqual([ + { id: removal.worktreeId, hostId: 'local', removing: true }, + { id: 'repo-1::/work/other' }, + { id: removal.worktreeId, hostId: 'ssh:box' } + ]) + expect( + projectPendingWorktreeRemovals( + rows, + (row) => row.id, + false, + snapshotPendingWorktreeRemovals() + ) + ).toEqual([{ id: 'repo-1::/work/other' }, { id: removal.worktreeId, hostId: 'ssh:box' }]) + }) + + it('drops a row a listing read before Git finished deleting it, and keeps one whose delete failed', async () => { + const other = { ...removal, worktreeId: 'repo-1::/work/other' } + const removed = deferred>() + const failed = deferred>() + vi.spyOn(console, 'warn').mockImplementation(() => {}) + void startBackgroundWorktreeRemoval({ removal, run: () => removed.promise, publish: () => {} }) + void startBackgroundWorktreeRemoval({ + removal: other, + run: async () => { + await failed.promise + throw new Error('git failed') + }, + publish: () => {} + }) + // The listing reads Git while both deletes run and replies after both ended. + const pendingAtScan = snapshotPendingWorktreeRemovals() + removed.resolve({}) + failed.resolve({}) + await _settlePendingWorktreeRemovalsForTests() + const rows: { id: string; hostId?: undefined }[] = [ + { id: removal.worktreeId }, + { id: other.worktreeId } + ] + + for (const clientReadsMarker of [true, false]) { + expect( + projectPendingWorktreeRemovals(rows, (row) => row.id, clientReadsMarker, pendingAtScan) + ).toEqual([{ id: other.worktreeId }]) + } + // A listing that began after the delete finished reads Git's current answer as-is. + expect( + projectPendingWorktreeRemovals(rows, (row) => row.id, true, snapshotPendingWorktreeRemovals()) + ).toBe(rows) + }) + + it('keeps WSL checkouts on the inline delete', () => { + expect(removesInBackground('/work/feature', {})).toBe(true) + expect(removesInBackground('/home/me/feature', { wslDistro: 'Ubuntu' })).toBe(false) + }) + + it('returns listings untouched when nothing is being removed', () => { + const rows: { id: string; hostId?: undefined }[] = [{ id: removal.worktreeId }] + expect( + projectPendingWorktreeRemovals( + rows, + (row) => row.id, + false, + snapshotPendingWorktreeRemovals() + ) + ).toBe(rows) + }) +}) diff --git a/src/main/worktree-background-removal.ts b/src/main/worktree-background-removal.ts new file mode 100644 index 000000000000..be6bb365d5ae --- /dev/null +++ b/src/main/worktree-background-removal.ts @@ -0,0 +1,364 @@ +import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../shared/execution-host' +import type { RemoveWorktreeResult } from '../shared/worktree/create-types' +import type { GitWorktreeInfo } from '../shared/worktree/types' +import { normalizeLocalBranchRef } from './git/worktree-operation-options' +import { areWorktreePathsEqual } from './git/worktree-path-comparison' +import { acquireWatcherRemovalGate, type WatcherRemovalGate } from './ipc/watcher-removal-gate' +import { parseWslPath } from './wsl' +import { + readWorktreeRemovalRecords, + writeWorktreeRemovalRecords, + type WorktreeRemovalRecord +} from './worktree-removal-records' + +export type BackgroundWorktreeRemovalJob = { + /** `stopSignal` aborts on an orderly quit; pass it only to the checkout delete. */ + run: (stopSignal: AbortSignal) => Promise + /** Fires when the row starts showing as removing, and again after it has left the table. */ + publish: () => void +} + +/** The removals pending when a listing began to read Git. */ +export type PendingWorktreeRemovals = ReadonlyMap + +type RemovalSettlement = { + result: Promise + resolve: (result: RemoveWorktreeResult) => void + reject: (error: unknown) => void +} + +// The accepted removals, mirrored to disk on every change; listings and joins read only this. +const pendingByWorktreeId = new Map() +const jobsByWorktreeId = new Map>() +// What every request for a pending removal waits on: the first one and any that join it. +const settlementsByWorktreeId = new Map() +const stopControllers = new Set() +// Loaded removals' terminal/watcher fences, held until the resumed job takes its own gate. +const startupFencesByWorktreeId = new Map() +// Why weak: a listing that read Git before a delete finished holds the record until it replies. +const removedRecords = new WeakSet() +const NO_PENDING_REMOVALS: PendingWorktreeRemovals = new Map() +let recordsDirectory: string | null = null + +/** + * Loads removals a quit or crash interrupted, so listings mark them before the first paint and + * session restore cannot open a terminal or watcher in a half-deleted checkout before the resume. + */ +export async function loadWorktreeRemovalRecords(directory: string): Promise { + recordsDirectory = directory + for (const record of await readWorktreeRemovalRecords(directory)) { + if (!pendingByWorktreeId.has(record.worktreeId)) { + addPendingRemoval(record) + try { + startupFencesByWorktreeId.set( + record.worktreeId, + acquireWatcherRemovalGate(record.worktreePath) + ) + } catch { + // An overlapping loaded removal already fences this path. + } + } + } +} + +/** Hands a loaded removal's fence to its resumed job; call in the same tick the job takes its gate. */ +export function releaseStartupRemovalFence(worktreeId: string): void { + startupFencesByWorktreeId.get(worktreeId)?.release() + startupFencesByWorktreeId.delete(worktreeId) +} + +function addPendingRemoval(record: WorktreeRemovalRecord): RemovalSettlement { + let resolve!: RemovalSettlement['resolve'] + let reject!: RemovalSettlement['reject'] + const result = new Promise((settle, fail) => { + resolve = settle + reject = fail + }) + // Why: a removal nobody waits on (an older client's, or one a restart resumed) may still fail. + result.catch(() => {}) + const settlement = { result, resolve, reject } + pendingByWorktreeId.set(record.worktreeId, record) + settlementsByWorktreeId.set(record.worktreeId, settlement) + return settlement +} + +function persistRecords(): Promise { + if (!recordsDirectory) { + return Promise.resolve() + } + return writeWorktreeRemovalRecords(recordsDirectory, () => [ + ...pendingByWorktreeId.values() + ]).catch((error: unknown) => { + // Why: bookkeeping must not gate the delete; a lost write only costs resuming it after a quit. + console.warn('[worktrees] failed to persist worktree removal records', error) + }) +} + +/** + * The result of the removal this host is running for the worktree, for a request that joins it. + * Only this host's local checkouts are removed in the background. + */ +export function waitForPendingWorktreeRemoval( + worktreeId: string, + hostId?: ExecutionHostId +): Promise | undefined { + return (hostId ?? LOCAL_EXECUTION_HOST_ID) === LOCAL_EXECUTION_HOST_ID + ? settlementsByWorktreeId.get(worktreeId)?.result + : undefined +} + +/** Waits for the delete an accepted background removal started; the acceptance's fields ride along. */ +export async function finishAcceptedWorktreeRemoval( + accepted: T, + worktreeId: string, + hostId?: ExecutionHostId +): Promise> { + const { removing, ...acceptance } = accepted + const pending = removing ? waitForPendingWorktreeRemoval(worktreeId, hostId) : undefined + return pending ? { ...acceptance, ...(await pending) } : acceptance +} + +/** WSL checkouts still delete inline, as before; moving them off the request is a follow-up. */ +export function removesInBackground( + worktreePath: string, + options: { wslDistro?: string } +): boolean { + return !options.wslDistro && !parseWslPath(worktreePath) +} + +export function hasPendingWorktreeRemovals(): boolean { + return pendingByWorktreeId.size > 0 +} + +export function findPendingWorktreeRemovalConflict( + repoPath: string, + target: { worktreePath?: string; branch?: string } +): WorktreeRemovalRecord | undefined { + const branch = target.branch?.replace(/^refs\/heads\//, '') + for (const removal of pendingByWorktreeId.values()) { + if (!areWorktreePathsEqual(removal.repoPath, repoPath)) { + continue + } + if ( + (target.worktreePath && areWorktreePathsEqual(removal.worktreePath, target.worktreePath)) || + (branch && removal.branch === branch) + ) { + return removal + } + } + return undefined +} + +export function assertNoPendingWorktreeRemovalConflict( + repoPath: string, + target: { worktreePath?: string; branch?: string } +): void { + const removal = findPendingWorktreeRemovalConflict(repoPath, target) + if (removal) { + throw new Error( + `Orca is still deleting the workspace at ${removal.worktreePath}. Cleanup is pending; try again shortly.` + ) + } +} + +/** + * Records an accepted removal and runs its delete detached from the request that asked for it, so + * the delete finishes even when that request times out or its client goes away. Resolves with the + * delete's result. + */ +export function startBackgroundWorktreeRemoval( + args: { + removal: Pick< + WorktreeRemovalRecord, + 'worktreeId' | 'repoId' | 'repoPath' | 'deleteBranch' | 'force' + > & { worktree: Pick } + } & BackgroundWorktreeRemovalJob +): Promise { + const { worktree, ...accepted } = args.removal + const record: WorktreeRemovalRecord = { + ...accepted, + worktreePath: worktree.path, + branch: normalizeLocalBranchRef(worktree.branch), + head: worktree.head, + requestedAt: Date.now() + } + const settlement = addPendingRemoval(record) + runBackgroundWorktreeRemoval(record, args, persistRecords()) + publishSafely(args.publish) + return settlement.result +} + +/** Runs the same delete again for every record a quit or crash left without a running job. */ +export function resumeInterruptedWorktreeRemovals( + jobFor: (record: WorktreeRemovalRecord) => BackgroundWorktreeRemovalJob +): void { + for (const record of pendingByWorktreeId.values()) { + if (!jobsByWorktreeId.has(record.worktreeId)) { + runBackgroundWorktreeRemoval(record, jobFor(record), Promise.resolve()) + } + } +} + +/** Orderly quit: stops each checkout delete Git is running, without waiting for it to exit. */ +export function stopBackgroundWorktreeRemovals(): void { + for (const controller of stopControllers) { + controller.abort() + } +} + +function runBackgroundWorktreeRemoval( + record: WorktreeRemovalRecord, + job: BackgroundWorktreeRemovalJob, + recorded: Promise +): void { + const controller = new AbortController() + stopControllers.add(controller) + const settled: Promise = settleBackgroundWorktreeRemoval( + record, + settlementsByWorktreeId.get(record.worktreeId), + job, + recorded, + controller.signal + ).finally(() => { + stopControllers.delete(controller) + if (jobsByWorktreeId.get(record.worktreeId) === settled) { + jobsByWorktreeId.delete(record.worktreeId) + } + }) + jobsByWorktreeId.set(record.worktreeId, settled) +} + +async function settleBackgroundWorktreeRemoval( + record: WorktreeRemovalRecord, + settlement: RemovalSettlement | undefined, + job: BackgroundWorktreeRemovalJob, + recorded: Promise, + stopSignal: AbortSignal +): Promise { + await waitForRecordWrite(record, recorded) + let settle: (settlement: RemovalSettlement) => void + try { + if (stopSignal.aborted) { + return + } + const result = await job.run(stopSignal) + removedRecords.add(record) + settle = (settlement) => settlement.resolve(result) + } catch (error) { + if (stopSignal.aborted) { + // Why: quit stopped Git; the record stays so the next start finishes this delete. + return + } + console.warn(`[worktrees] background removal of ${record.worktreePath} failed`, error) + settle = (settlement) => settlement.reject(error) + } finally { + // A resumed job that ended before taking its own gate still holds the fence loading gave it. + releaseStartupRemovalFence(record.worktreeId) + } + // Why clear on failure too: the row returns live and retryable instead of retrying unseen. + const cleared = pendingByWorktreeId.get(record.worktreeId) === record + if (cleared) { + pendingByWorktreeId.delete(record.worktreeId) + settlementsByWorktreeId.delete(record.worktreeId) + } + // Why this run's own settlement: desktop IPC and runtime RPC coalesce separately, so a concurrent + // removal can replace the record, and the request waiting on this delete must still get its reply. + if (settlement) { + settle(settlement) + } + // Why notify before the clear is on disk: the clear is bookkeeping; a crash before it lands only + // re-runs a finish that re-derives what is left from Git. + publishSafely(job.publish) + if (cleared) { + await persistRecords() + } +} + +// Why: the record should be on disk before Git deletes, so a quit resumes the delete, but a disk or +// file pool stall must not hold the user's delete; past this, a crash only loses the resume. +const RECORD_WRITE_WAIT_MS = 2_000 + +async function waitForRecordWrite( + record: WorktreeRemovalRecord, + recorded: Promise +): Promise { + let timer: ReturnType | undefined + const timedOut = await Promise.race([ + recorded.then(() => false), + new Promise((resolve) => { + timer = setTimeout(() => resolve(true), RECORD_WRITE_WAIT_MS) + }) + ]) + clearTimeout(timer) + if (timedOut) { + console.warn( + `[worktrees] removal record for ${record.worktreePath} not on disk after ${RECORD_WRITE_WAIT_MS} ms; deleting anyway` + ) + } +} + +function publishSafely(publish: () => void): void { + try { + publish() + } catch (error) { + // Why: a failed notification must not strand the table entry or reject the detached job. + console.error('[worktrees] failed to publish background removal state', error) + } +} + +/** Taken before a listing reads Git; pass it to projectPendingWorktreeRemovals with the rows. */ +export function snapshotPendingWorktreeRemovals(): PendingWorktreeRemovals { + return pendingByWorktreeId.size === 0 ? NO_PENDING_REMOVALS : new Map(pendingByWorktreeId) +} + +/** + * Marks rows whose checkout this host is deleting, or leaves them out for a client that cannot + * read the marker: such a client already dropped the row on acceptance and would re-show it. + */ +export function projectPendingWorktreeRemovals< + T extends { hostId?: ExecutionHostId; removing?: true } +>( + rows: T[], + idOf: (row: T) => string, + clientReadsMarker: boolean, + pendingAtScan: PendingWorktreeRemovals +): T[] { + if (pendingByWorktreeId.size === 0 && pendingAtScan.size === 0) { + return rows + } + const projected: T[] = [] + for (const row of rows) { + const id = idOf(row) + const local = row.hostId === undefined || row.hostId === LOCAL_EXECUTION_HOST_ID + if (local && pendingByWorktreeId.has(id)) { + if (clientReadsMarker) { + projected.push({ ...row, removing: true }) + } + continue + } + const scanned = local ? pendingAtScan.get(id) : undefined + // Why: Git was read before this delete finished; unmarked, the gone row reads as a failed delete. + if (!scanned || !removedRecords.has(scanned)) { + projected.push(row) + } + } + return projected +} + +export async function _settlePendingWorktreeRemovalsForTests(): Promise { + while (jobsByWorktreeId.size > 0) { + await Promise.all(jobsByWorktreeId.values()) + } +} + +export function _resetPendingWorktreeRemovalsForTests(): void { + pendingByWorktreeId.clear() + jobsByWorktreeId.clear() + settlementsByWorktreeId.clear() + stopControllers.clear() + for (const fence of startupFencesByWorktreeId.values()) { + fence.release() + } + startupFencesByWorktreeId.clear() + recordsDirectory = null +} diff --git a/src/main/worktree-removal-acceptance-queue.ts b/src/main/worktree-removal-acceptance-queue.ts new file mode 100644 index 000000000000..e7248b0b7a05 --- /dev/null +++ b/src/main/worktree-removal-acceptance-queue.ts @@ -0,0 +1,15 @@ +import { runKeyedSerializedOperation } from './cli/keyed-promise-queue' + +const acceptanceQueueByRepo = new Map>() + +/** + * Runs one local removal's archive hook, preflight and teardown only after the previous removal in + * the same repo was accepted, whichever client asked: hooks that write refs race the repo's ref + * locks (#2259). Git's checkout delete runs after acceptance, in parallel under its own limit. + */ +export function runSerializedWorktreeRemovalAcceptance( + repoPath: string, + accept: () => Promise +): Promise { + return runKeyedSerializedOperation(acceptanceQueueByRepo, repoPath, accept) +} diff --git a/src/main/worktree-removal-records.ts b/src/main/worktree-removal-records.ts new file mode 100644 index 000000000000..1fd3558d232a --- /dev/null +++ b/src/main/worktree-removal-records.ts @@ -0,0 +1,93 @@ +// Why a sidecar beside the profile state: the record must be on disk before Git starts deleting, +// and profile-state writes are batched; the sidecar writer fsyncs and renames atomically. It also +// keeps a host-local, in-flight obligation out of profile exports and backups. +import { + readSidecarSnapshot, + sidecarSnapshotFile, + withSidecarSnapshotQueue, + writeSidecarSnapshot +} from './sidecar-snapshot-file' + +const RECORDS_FILE_NAME = 'orca-worktree-removals.json' +const RECORDS_VERSION = 1 + +/** A local worktree removal Orca accepted; everything a restart needs to run the same delete. */ +export type WorktreeRemovalRecord = { + worktreeId: string + repoId: string + repoPath: string + worktreePath: string + /** Short branch name the checkout had; empty when detached. */ + branch: string + /** Branch head when the removal was accepted; empty when unknown. */ + head: string + deleteBranch: boolean + force: boolean + requestedAt: number +} + +type PersistedWorktreeRemovalRecords = { + version: number + removals: WorktreeRemovalRecord[] +} + +function isRecord(value: unknown): value is Record { + return typeof value === 'object' && value !== null && !Array.isArray(value) +} + +function parseRecord(value: unknown): WorktreeRemovalRecord | null { + if ( + !isRecord(value) || + typeof value.worktreeId !== 'string' || + typeof value.repoId !== 'string' || + typeof value.repoPath !== 'string' || + typeof value.worktreePath !== 'string' || + typeof value.branch !== 'string' || + typeof value.head !== 'string' || + typeof value.deleteBranch !== 'boolean' || + typeof value.force !== 'boolean' || + typeof value.requestedAt !== 'number' + ) { + return null + } + return { + worktreeId: value.worktreeId, + repoId: value.repoId, + repoPath: value.repoPath, + worktreePath: value.worktreePath, + branch: value.branch, + head: value.head, + deleteBranch: value.deleteBranch, + force: value.force, + requestedAt: value.requestedAt + } +} + +export function worktreeRemovalRecordsFile(directory: string): string { + return sidecarSnapshotFile(directory, RECORDS_FILE_NAME) +} + +/** Records a previous run left behind; a missing or unreadable file reads as none. */ +export async function readWorktreeRemovalRecords( + directory: string +): Promise { + const parsed = await readSidecarSnapshot(worktreeRemovalRecordsFile(directory)) + if (!isRecord(parsed) || parsed.version !== RECORDS_VERSION || !Array.isArray(parsed.removals)) { + return [] + } + return parsed.removals.flatMap((entry) => parseRecord(entry) ?? []) +} + +/** Writes the current records; `current` is read when the queued write runs, so the latest wins. */ +export function writeWorktreeRemovalRecords( + directory: string, + current: () => WorktreeRemovalRecord[] +): Promise { + const file = worktreeRemovalRecordsFile(directory) + return withSidecarSnapshotQueue(file, () => + writeSidecarSnapshot(file, { + version: RECORDS_VERSION, + removals: current() + } satisfies PersistedWorktreeRemovalRecords) + ) +} diff --git a/src/main/worktree-trash.test.ts b/src/main/worktree-trash.test.ts index 2819c789bf5f..4d5c163e5bf7 100644 --- a/src/main/worktree-trash.test.ts +++ b/src/main/worktree-trash.test.ts @@ -1,18 +1,12 @@ import { existsSync } from 'node:fs' -import { mkdir, mkdtemp, readFile, readdir, rm, symlink, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, rm, symlink, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it } from 'vitest' import type { Repo } from '../shared/repo-types' import { collectWorktreeTrashSweepRoots, - getWorktreeTrashRoot, - isWorktreeTrashEntryName, - moveWorktreeDirectoryToTrash, - restoreWorktreeDirectoryFromTrash, - scheduleWorktreeTrashDeletion, sweepStaleWorktreeTrash, - whenWorktreeTrashDeletionsSettled, WORKTREE_TRASH_DIR_NAME } from './worktree-trash' @@ -23,7 +17,6 @@ beforeEach(async () => { }) afterEach(async () => { - await whenWorktreeTrashDeletionsSettled() await rm(scratchDir, { recursive: true, force: true }) }) @@ -32,112 +25,6 @@ async function createWorktreeDirectory(worktreePath: string): Promise { await writeFile(join(worktreePath, 'node_modules', 'pkg', 'index.js'), 'module.exports = 1\n') } -describe('moveWorktreeDirectoryToTrash', () => { - it('renames the checkout into a hidden sibling trash root', async () => { - const worktreePath = join(scratchDir, 'repo', 'feature') - await createWorktreeDirectory(worktreePath) - - const trashPath = await moveWorktreeDirectoryToTrash(worktreePath) - - expect(trashPath).toBeDefined() - expect(existsSync(worktreePath)).toBe(false) - expect(trashPath!.startsWith(join(scratchDir, 'repo', WORKTREE_TRASH_DIR_NAME))).toBe(true) - expect(existsSync(join(trashPath!, 'node_modules', 'pkg', 'index.js'))).toBe(true) - }) - - it('leaves a file target untouched without creating a trash root', async () => { - const worktreePath = join(scratchDir, '.git') - await writeFile(worktreePath, 'gitdir: /preserved/admin\n') - - expect(await moveWorktreeDirectoryToTrash(worktreePath)).toBeUndefined() - expect(await readFile(worktreePath, 'utf8')).toBe('gitdir: /preserved/admin\n') - expect(existsSync(getWorktreeTrashRoot(worktreePath))).toBe(false) - }) - - it('leaves a directory symlink and its target untouched', async () => { - const target = join(scratchDir, 'target') - const worktreePath = join(scratchDir, 'link') - await createWorktreeDirectory(target) - await symlink(target, worktreePath, process.platform === 'win32' ? 'junction' : 'dir') - - expect(await moveWorktreeDirectoryToTrash(worktreePath)).toBeUndefined() - expect(existsSync(join(worktreePath, 'node_modules', 'pkg', 'index.js'))).toBe(true) - expect(existsSync(getWorktreeTrashRoot(worktreePath))).toBe(false) - }) - - it('generates sweepable, collision-free entry names', async () => { - const first = await moveWorktreeDirectoryToTrash(await seededWorktree('one')) - const second = await moveWorktreeDirectoryToTrash(await seededWorktree('two')) - - expect(first).not.toEqual(second) - expect(await readdir(join(scratchDir, 'repo', WORKTREE_TRASH_DIR_NAME))).toHaveLength(2) - for (const entry of await readdir(join(scratchDir, 'repo', WORKTREE_TRASH_DIR_NAME))) { - expect(isWorktreeTrashEntryName(entry)).toBe(true) - } - }) - - it('reports the rename as unavailable and leaves no trash root when the checkout is missing', async () => { - const worktreePath = join(scratchDir, 'repo', 'gone') - - expect(await moveWorktreeDirectoryToTrash(worktreePath)).toBeUndefined() - expect(existsSync(getWorktreeTrashRoot(worktreePath))).toBe(false) - }) - - it.skipIf(process.platform === 'win32')('refuses a symlinked trash root', async () => { - const worktreePath = join(scratchDir, 'repo', 'feature') - const externalRoot = join(scratchDir, 'external') - await createWorktreeDirectory(worktreePath) - await mkdir(externalRoot) - await symlink(externalRoot, getWorktreeTrashRoot(worktreePath)) - - expect(await moveWorktreeDirectoryToTrash(worktreePath)).toBeUndefined() - expect(existsSync(worktreePath)).toBe(true) - expect(await readdir(externalRoot)).toEqual([]) - }) - - async function seededWorktree(name: string): Promise { - const worktreePath = join(scratchDir, 'repo', name) - await createWorktreeDirectory(worktreePath) - return worktreePath - } -}) - -describe('restoreWorktreeDirectoryFromTrash', () => { - it('puts the checkout back where Git registered it', async () => { - const worktreePath = join(scratchDir, 'repo', 'feature') - await createWorktreeDirectory(worktreePath) - const trashPath = await moveWorktreeDirectoryToTrash(worktreePath) - - expect(await restoreWorktreeDirectoryFromTrash(trashPath!, worktreePath)).toBe(true) - expect(existsSync(join(worktreePath, 'node_modules', 'pkg', 'index.js'))).toBe(true) - expect(existsSync(trashPath!)).toBe(false) - }) - - it('reports failure instead of throwing when the trashed path is gone', async () => { - const trashRoot = getWorktreeTrashRoot(join(scratchDir, 'repo', 'feature')) - expect( - await restoreWorktreeDirectoryFromTrash( - join(trashRoot, 'wt-1-abcdef01'), - join(scratchDir, 'repo', 'feature') - ) - ).toBe(false) - }) -}) - -describe('scheduleWorktreeTrashDeletion', () => { - it('deletes trashed checkouts in the background', async () => { - const worktreePath = join(scratchDir, 'repo', 'feature') - await createWorktreeDirectory(worktreePath) - const trashPath = await moveWorktreeDirectoryToTrash(worktreePath) - - scheduleWorktreeTrashDeletion(trashPath!) - await whenWorktreeTrashDeletionsSettled() - - expect(existsSync(trashPath!)).toBe(false) - expect(existsSync(getWorktreeTrashRoot(worktreePath))).toBe(true) - }) -}) - describe('sweepStaleWorktreeTrash', () => { it('removes leftover entries from both workspace layouts and nothing else', async () => { const nestedTrashRoot = join(scratchDir, 'repo', WORKTREE_TRASH_DIR_NAME) @@ -158,6 +45,9 @@ describe('sweepStaleWorktreeTrash', () => { expect(existsSync(join(nestedTrashRoot, 'unrelated-directory'))).toBe(true) expect(existsSync(join(nestedTrashRoot, 'wt-notes.txt'))).toBe(true) expect(existsSync(join(liveWorktree, 'node_modules', 'pkg', 'index.js'))).toBe(true) + // Nothing refills a trash root, so an emptied one goes; one still holding foreign entries stays. + expect(existsSync(flatTrashRoot)).toBe(false) + expect(existsSync(nestedTrashRoot)).toBe(true) }) it('ignores workspace roots that do not exist', async () => { diff --git a/src/main/worktree-trash.ts b/src/main/worktree-trash.ts index cb4d3d52e03b..088c51bfbc9e 100644 --- a/src/main/worktree-trash.ts +++ b/src/main/worktree-trash.ts @@ -1,10 +1,9 @@ -// Why: `git worktree remove` deletes the whole checkout (usually a multi-GB node_modules) inside the -// remove IPC, so the UI sat on a spinner for 8-35s. Renaming the directory into a sibling trash root -// is a metadata operation, and the recursive delete then runs after the IPC has already returned. +// Why: releases before worktree removal ran inline renamed checkouts into a sibling trash root and +// deleted them in the background; a quit mid-delete left entries behind. Nothing creates trash now, +// so this sweep only drains those legacy entries. -import { randomBytes } from 'node:crypto' -import { lstat, mkdir, readdir, rename, rmdir } from 'node:fs/promises' -import { dirname, join } from 'node:path' +import { lstat, readdir, rmdir } from 'node:fs/promises' +import { join } from 'node:path' import { removeHostTree } from './host-tree-removal' import { isFolderRepo } from '../shared/repo-kind' import { computeWorkspaceRoot, getWorktreePathSettings } from './ipc/worktree-logic' @@ -14,93 +13,13 @@ import { parseWslPath } from './wsl' export const WORKTREE_TRASH_DIR_NAME = '.orca-worktree-trash' -// `-`: the nonce keeps concurrent removals of same-named worktrees from colliding. +// `-`: the only names the retired rename ever generated. const TRASH_ENTRY_PATTERN = /^wt-\d+-[0-9a-f]{8}$/ // Why: the sweep must stay cheap on a workspace root holding many repo containers. const TRASH_SWEEP_MAX_CONTAINERS = 200 -/** Trash root for a worktree: a hidden sibling, so the rename always stays on one volume. */ -export function getWorktreeTrashRoot(worktreePath: string): string { - return join(dirname(worktreePath), WORKTREE_TRASH_DIR_NAME) -} - -export function isWorktreeTrashEntryName(entryName: string): boolean { - return TRASH_ENTRY_PATTERN.test(entryName) -} - -/** - * Move a worktree directory aside so the caller can return before it is deleted. - * Returns the trash path, or `undefined` when the rename is unavailable (a different - * volume, or Windows open handles) and the caller must delete in place instead. - */ -export async function moveWorktreeDirectoryToTrash( - worktreePath: string -): Promise { - const trashRoot = getWorktreeTrashRoot(worktreePath) - const trashPath = join(trashRoot, `wt-${Date.now()}-${randomBytes(4).toString('hex')}`) - try { - // A malformed Git registration can name the checkout's .git file. - const worktreeStat = await lstat(worktreePath) - if (!worktreeStat.isDirectory() || worktreeStat.isSymbolicLink()) { - return undefined - } - await mkdir(trashRoot, { recursive: true }) - const trashRootStat = await lstat(trashRoot) - if (!trashRootStat.isDirectory() || trashRootStat.isSymbolicLink()) { - throw new Error(`Refusing non-directory worktree trash root: ${trashRoot}`) - } - await rename(worktreePath, trashPath) - return trashPath - } catch (error) { - console.warn( - `[worktrees] Deferred deletion unavailable for ${worktreePath}; deleting in place`, - error - ) - // Leave no empty trash root behind when the rename never happened; rmdir keeps queued entries. - await rmdir(trashRoot).catch(() => {}) - return undefined - } -} - -/** Undo a trash rename so a failed registration cleanup leaves the worktree exactly as it was. */ -export async function restoreWorktreeDirectoryFromTrash( - trashPath: string, - worktreePath: string -): Promise { - try { - await rename(trashPath, worktreePath) - return true - } catch (error) { - console.warn(`[worktrees] Failed to restore ${worktreePath} from ${trashPath}`, error) - return false - } -} - -// Why serialized: one background delete at a time keeps a burst of removals from saturating disk I/O -// while the user keeps working. -let queuedTrashDeletions: Promise = Promise.resolve() - -export function scheduleWorktreeTrashDeletion(trashPath: string): void { - queuedTrashDeletions = queuedTrashDeletions.then(async () => { - try { - await removeHostTree(trashPath) - } catch (error) { - // Why only a warning: the directory is already invisible to the user, and the startup sweep retries it. - console.warn(`[worktrees] Failed to delete trashed worktree at ${trashPath}`, error) - } - }) -} - -/** Test/shutdown hook: resolves once every queued background deletion has settled. */ -export function whenWorktreeTrashDeletionsSettled(): Promise { - return queuedTrashDeletions -} - -/** - * Delete trash entries left behind by a previous run (a crash or a kill during background deletion). - * Only entries matching the generated name pattern inside a trash root are removed. - */ +/** Delete legacy trash entries. Only entries matching the generated name pattern inside a trash root are removed. */ export async function sweepStaleWorktreeTrash( workspaceRoots: readonly string[] ): Promise<{ removed: number }> { @@ -117,7 +36,7 @@ export async function sweepStaleWorktreeTrash( continue } for (const entry of entries) { - if (!isWorktreeTrashEntryName(entry)) { + if (!TRASH_ENTRY_PATTERN.test(entry)) { continue } try { @@ -130,6 +49,8 @@ export async function sweepStaleWorktreeTrash( ) } } + // Why: nothing refills the root, so drop it once empty; rmdir keeps anything still inside. + await rmdir(trashRoot).catch(() => {}) } if (removed > 0) { console.log(`[worktrees] Swept ${removed} leftover worktree director(ies) from a previous run`) @@ -158,7 +79,7 @@ async function collectExistingTrashRoots(workspaceRoots: readonly string[]): Pro return [...trashRoots] } -/** Workspace roots of local git repos — the only places Orca creates worktree trash. */ +/** Workspace roots of local git repos — the only places Orca ever created worktree trash. */ export function collectWorktreeTrashSweepRoots( repos: readonly Repo[], settings: Pick diff --git a/src/relay/git-branch-delete-refusal-parity.test.ts b/src/relay/git-branch-delete-refusal-parity.test.ts index 71344bca940c..b71cb20d4f43 100644 --- a/src/relay/git-branch-delete-refusal-parity.test.ts +++ b/src/relay/git-branch-delete-refusal-parity.test.ts @@ -13,18 +13,9 @@ import type * as FsPromises from 'node:fs/promises' import { beforeEach, describe, expect, it, vi } from 'vitest' -const { gitExecFileAsyncMock, resolveGitDirMock, moveWorktreeDirectoryToTrashMock } = vi.hoisted( - () => ({ - gitExecFileAsyncMock: vi.fn(), - resolveGitDirMock: vi.fn(), - moveWorktreeDirectoryToTrashMock: vi.fn() - }) -) - -vi.mock('../main/worktree-trash', () => ({ - moveWorktreeDirectoryToTrash: moveWorktreeDirectoryToTrashMock, - restoreWorktreeDirectoryFromTrash: vi.fn(async () => true), - scheduleWorktreeTrashDeletion: vi.fn() +const { gitExecFileAsyncMock, resolveGitDirMock } = vi.hoisted(() => ({ + gitExecFileAsyncMock: vi.fn(), + resolveGitDirMock: vi.fn() })) vi.mock('../main/git/runner', () => ({ @@ -170,9 +161,6 @@ beforeEach(() => { gitExecFileAsyncMock.mockReset() resolveGitDirMock.mockReset() resolveGitDirMock.mockImplementation(async (worktreePath: string) => `${worktreePath}/.git`) - moveWorktreeDirectoryToTrashMock.mockReset() - // Default: the checkout cannot be renamed aside, so removal runs `worktree remove` in place. - moveWorktreeDirectoryToTrashMock.mockResolvedValue(undefined) }) describe('relay/desktop branch-delete refusal parity', () => { diff --git a/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts b/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts index 8f4b92a8cd20..d050d1c076aa 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-flow.test.ts @@ -230,9 +230,15 @@ describe('delete worktree flow', () => { }) it('revalidates each queued instance immediately before execution', async () => { + // Same-repo deletes queue on a host that does not serialize their branch cleanup itself. setWorktrees([ - { id: 'wt-1', instanceId: 'instance-1', path: '/workspaces/first-longer' }, - { id: 'wt-2', instanceId: 'instance-2', path: '/workspaces/second' } + { + id: 'wt-1', + instanceId: 'instance-1', + path: '/workspaces/first-longer', + hostId: 'ssh:builder' + }, + { id: 'wt-2', instanceId: 'instance-2', path: '/workspaces/second', hostId: 'ssh:builder' } ]) const targets = Array.from(mocks.state.worktreeMap.values()) let finishFirst!: (result: { ok: true }) => void @@ -243,7 +249,7 @@ describe('delete worktree flow', () => { const deletion = runWorktreeDeletesInParallel(targets) await vi.waitFor(() => expect(mocks.state.removeWorktree).toHaveBeenCalledWith( - { id: 'wt-1', executionHostId: null }, + { id: 'wt-1', executionHostId: 'ssh:builder' }, false, { suppressPreservedBranchToast: true @@ -251,14 +257,24 @@ describe('delete worktree flow', () => { ) ) setWorktrees([ - { id: 'wt-1', instanceId: 'instance-1', path: '/workspaces/first-longer' }, - { id: 'wt-2', instanceId: 'replacement-instance', path: '/workspaces/second' } + { + id: 'wt-1', + instanceId: 'instance-1', + path: '/workspaces/first-longer', + hostId: 'ssh:builder' + }, + { + id: 'wt-2', + instanceId: 'replacement-instance', + path: '/workspaces/second', + hostId: 'ssh:builder' + } ]) finishFirst({ ok: true }) - await expect(deletion).resolves.toEqual([{ id: 'wt-1', executionHostId: null }]) + await expect(deletion).resolves.toEqual([{ id: 'wt-1', executionHostId: 'ssh:builder' }]) expect(mocks.state.removeWorktree).not.toHaveBeenCalledWith( - { id: 'wt-2', executionHostId: null }, + { id: 'wt-2', executionHostId: 'ssh:builder' }, false, { suppressPreservedBranchToast: true diff --git a/src/renderer/src/components/sidebar/delete-worktree-parallel-flow.test.ts b/src/renderer/src/components/sidebar/delete-worktree-parallel-flow.test.ts index a211e9cb10d0..6340de503c48 100644 --- a/src/renderer/src/components/sidebar/delete-worktree-parallel-flow.test.ts +++ b/src/renderer/src/components/sidebar/delete-worktree-parallel-flow.test.ts @@ -223,6 +223,57 @@ describe('runWorktreeDeletesInParallel', () => { ) }) + it('starts same-repo deletes on this machine without waiting for earlier ones to finish', async () => { + const first = deferredDeleteResult() + mocks.state.removeWorktree.mockImplementationOnce(() => first.promise) + + const deleted = runDeletesForCurrentWorktrees([ + { id: 'wt-1', displayName: 'one', repoId: 'repo-a', path: '/workspaces/one' }, + { id: 'wt-2', displayName: 'two', repoId: 'repo-a', path: '/workspaces/two' } + ]) + + await vi.waitFor(() => expect(mocks.state.removeWorktree).toHaveBeenCalledTimes(2)) + first.resolve({ ok: true }) + await expect(deleted).resolves.toHaveLength(2) + }) + + it('runs same-repo deletes one at a time on a host that does not serialize them itself', async () => { + const first = deferredDeleteResult() + mocks.state.removeWorktree.mockImplementationOnce(() => first.promise) + const hostId = 'ssh:builder' as const + + const deleted = runDeletesForCurrentWorktrees([ + { id: 'wt-1', displayName: 'one', repoId: 'repo-a', path: '/workspaces/one', hostId }, + { id: 'wt-2', displayName: 'two', repoId: 'repo-a', path: '/workspaces/two', hostId } + ]) + + await vi.waitFor(() => expect(mocks.state.removeWorktree).toHaveBeenCalledTimes(1)) + await Promise.resolve() + await Promise.resolve() + expect(mocks.state.removeWorktree).toHaveBeenCalledTimes(1) + first.resolve({ ok: true }) + await expect(deleted).resolves.toHaveLength(2) + expect(mocks.state.removeWorktree).toHaveBeenCalledTimes(2) + }) + + it('keeps a parent delete waiting for its child to finish on this machine', async () => { + const child = deferredDeleteResult() + mocks.state.removeWorktree.mockImplementationOnce(() => child.promise) + + const deleted = runDeletesForCurrentWorktrees([ + { id: 'parent', displayName: 'parent', repoId: 'repo-a', path: '/workspaces/parent' }, + { id: 'child', displayName: 'child', repoId: 'repo-a', path: '/workspaces/parent/child' } + ]) + + await vi.waitFor(() => expect(mocks.state.removeWorktree).toHaveBeenCalledTimes(1)) + await Promise.resolve() + await Promise.resolve() + expect(mocks.state.removeWorktree).toHaveBeenCalledTimes(1) + child.resolve({ ok: true }) + await expect(deleted).resolves.toHaveLength(2) + expect(mocks.state.removeWorktree).toHaveBeenCalledTimes(2) + }) + it('passes confirmed force to each delete', async () => { await runDeletesForCurrentWorktrees( [ diff --git a/src/renderer/src/components/sidebar/worktree-delete-execution.ts b/src/renderer/src/components/sidebar/worktree-delete-execution.ts index 5554c73d34da..5cd784ff6545 100644 --- a/src/renderer/src/components/sidebar/worktree-delete-execution.ts +++ b/src/renderer/src/components/sidebar/worktree-delete-execution.ts @@ -5,6 +5,7 @@ import { normalizeRuntimePathForComparison } from '../../../../shared/cross-platform-path' import type { Worktree } from '../../../../shared/worktree/types' +import { LOCAL_EXECUTION_HOST_ID } from '../../../../shared/execution-host' import { composeWorktreeHostIdentity, getWorktreeHostIdentity @@ -108,8 +109,20 @@ export async function runWorktreeDeletesInParallel( Array.from(groups.values()).map(async (group) => { const deletedInGroup: WorktreeRemovalTarget[] = [] const failedInGroup: (typeof group)[number][] = [] + const started: { path: string; settled: Promise }[] = [] + // Why only this machine's repos run in parallel: its host serializes the branch cleanup per + // repo and limits concurrent deletes, while SSH and paired (possibly older) hosts race the + // repo's ref locks when one repo deletes in parallel (#2259). + const serialized = (group[0]?.hostId ?? LOCAL_EXECUTION_HOST_ID) !== LOCAL_EXECUTION_HOST_ID for (const target of group) { - await runInWorktreeDeleteTurn(target.id, async () => { + // A descendant's outcome decides whether its ancestor may be deleted at all. + const descendants = started.filter((earlier) => + isStrictDescendantPath(target.path, earlier.path) + ) + if (descendants.length > 0) { + await Promise.all(descendants.map((earlier) => earlier.settled)) + } + const settled = runInWorktreeDeleteTurn(target.id, async () => { // A queued target may be recreated while an earlier repo sibling is deleting. // Why by host (STA-4343): the id-keyed map keeps ONE row per `repoId::path`, // so on a two-host collision it can hand back the other host's row — whose @@ -149,7 +162,12 @@ export async function runWorktreeDeletesInParallel( failedInGroup.push(target) } }) + started.push({ path: target.path, settled }) + if (serialized) { + await settled + } } + await Promise.all(started.map((entry) => entry.settled)) return deletedInGroup }) ) diff --git a/src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts b/src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts index 868a74363549..bbe9899bc694 100644 --- a/src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts +++ b/src/renderer/src/hooks/ipc-events/app-lifetime-ipc-bridge.ts @@ -7,6 +7,7 @@ import { remoteRuntimeTerminalColorPush } from '@/runtime/remote-runtime-termina import { resetAgentHookCompletionNotificationCoordinators } from '../agent-hook-completion-notifications' import { useAppStore } from '../../store' import { registerAgentStatusIpcBridge } from './agent-status-ipc-bridge' +import { registerBackgroundWorktreeRemovalBridge } from './background-worktree-removal-bridge' import { registerBrowserRequestIpcBridge } from './browser-request-ipc-bridge' import { registerBrowserStateIpcBridge } from './browser-state-ipc-bridge' import { registerContentCreationIpcBridge } from './content-creation-ipc-bridge' @@ -85,6 +86,7 @@ export function installAppLifetimeIpcEvents( .catch((error) => console.error('Failed to read runtime status snapshots:', error)) } const unsubscribeRuntimeEnvironmentStore = registerRuntimeClientIpcBridge(unsubs, worktreeRuntime) + registerBackgroundWorktreeRemovalBridge(unsubs) registerProjectCatalogIpcBridge( unsubs, worktreeRuntime.worktreeChangeRefreshQueue, diff --git a/src/renderer/src/hooks/ipc-events/background-worktree-removal-bridge.ts b/src/renderer/src/hooks/ipc-events/background-worktree-removal-bridge.ts new file mode 100644 index 000000000000..ca219bdd03ea --- /dev/null +++ b/src/renderer/src/hooks/ipc-events/background-worktree-removal-bridge.ts @@ -0,0 +1,96 @@ +import type { Worktree } from '../../../../shared/worktree/types' +import { getWorktreeHostIdentity } from '../../../../shared/worktree/host-qualified-identity' +import { getDeleteStateForWorktreeHost } from '@/components/sidebar/worktree-delete-state-host-match' +import { useAppStore } from '../../store' +import { + UNFINISHED_WORKTREE_REMOVAL_ERROR, + settleHostWorktreeRemovals +} from '../../store/slices/worktrees/teardown/host-worktree-removal-state' + +type HostMarkedRow = Pick +type AppStoreApi = Pick + +// Delete states this bridge set from a host marker, keyed like deleteStateByWorktreeId. A state the +// local delete flow set is left to that flow. +const hostMarkedDeleteStates = new Map() + +function deleteStateKey(row: HostMarkedRow): string { + return row.hostId ? getWorktreeHostIdentity(row) : row.id +} + +/** + * Shows the existing Deleting card while the host lists a row as removing, for views that did not + * ask for the delete. The row leaving means it finished; the row returning unmarked means it did not. + */ +export function reconcileHostWorktreeRemovals(store: AppStoreApi = useAppStore): void { + settleHostWorktreeRemovals() + const state = store.getState() + const marked: HostMarkedRow[] = [] + const listed = new Map() + for (const rows of Object.values(state.worktreesByRepo)) { + for (const row of rows) { + const key = deleteStateKey(row) + listed.set(key, row) + if (!row.removing || hostMarkedDeleteStates.has(key)) { + continue + } + const current = getDeleteStateForWorktreeHost(row, state.deleteStateByWorktreeId) + if (current?.isDeleting && current.phase !== 'queued') { + continue + } + hostMarkedDeleteStates.set(key, { id: row.id, hostId: row.hostId }) + marked.push({ id: row.id, hostId: row.hostId }) + } + } + if (marked.length > 0) { + state.markWorktreesDeleting(marked) + } + for (const [key, row] of hostMarkedDeleteStates) { + const listedRow = listed.get(key) + if (listedRow?.removing) { + continue + } + hostMarkedDeleteStates.delete(key) + if (!store.getState().deleteStateByWorktreeId[key]?.isDeleting) { + continue + } + if (!listedRow) { + store.getState().clearWorktreeDeleteState(row.id, row.hostId) + continue + } + store.setState((s) => ({ + deleteStateByWorktreeId: { + ...s.deleteStateByWorktreeId, + [key]: { + isDeleting: false, + ...(row.hostId ? { executionHostId: row.hostId } : {}), + error: UNFINISHED_WORKTREE_REMOVAL_ERROR, + canForceDelete: false, + forceDeleteReason: null + } + } + })) + } +} + +export function registerBackgroundWorktreeRemovalBridge(unsubs: (() => void)[]): void { + let previousRows = useAppStore.getState().worktreesByRepo + let previousDetected = useAppStore.getState().detectedWorktreesByRepo + unsubs.push( + useAppStore.subscribe((state) => { + if ( + state.worktreesByRepo === previousRows && + state.detectedWorktreesByRepo === previousDetected + ) { + return + } + previousRows = state.worktreesByRepo + previousDetected = state.detectedWorktreesByRepo + reconcileHostWorktreeRemovals() + }) + ) +} + +export function _resetBackgroundWorktreeRemovalBridgeForTests(): void { + hostMarkedDeleteStates.clear() +} diff --git a/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts b/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts index d0572bd847d6..3f20b3870ad8 100644 --- a/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts +++ b/src/renderer/src/hooks/useIpcEvents-lifecycle.test.ts @@ -465,7 +465,7 @@ describe('useIpcEvents App-lifetime lifecycle', () => { expect( [...listeners.values()].every((records) => records.filter((item) => item.active).length === 1) ).toBe(true) - expect(storeSubscriptions.filter((item) => item.active)).toHaveLength(2) + expect(storeSubscriptions.filter((item) => item.active)).toHaveLength(3) firstCleanup() const ipcCleanupOrder = cleanupOrder @@ -477,9 +477,11 @@ describe('useIpcEvents App-lifetime lifecycle', () => { 'mobile.disposeHydration', 'store.unsubscribe.0', 'runtimeStore.unsubscribe', - 'store.unsubscribe.1', + 'store.unsubscribe.2', 'agentStore.unsubscribe' ]) + // The background-removal bridge's row subscription, released with the rest of `unsubs`. + expect(cleanupOrder).toContain('store.unsubscribe.1') expect(cleanupOrder.indexOf('runtimeEnvironment.unsubscribe')).toBeGreaterThan( cleanupOrder.indexOf('ipc.ui.onMobileMarkdownRequest') ) @@ -512,7 +514,7 @@ describe('useIpcEvents App-lifetime lifecycle', () => { expect( [...listeners.values()].every((records) => records.filter((item) => item.active).length === 1) ).toBe(true) - expect(storeSubscriptions.filter((item) => item.active)).toHaveLength(2) + expect(storeSubscriptions.filter((item) => item.active)).toHaveLength(3) secondCleanup() expect([...listeners.values()].every((records) => records.every((item) => !item.active))).toBe( diff --git a/src/renderer/src/lib/ensure-hooks-confirmed.test.ts b/src/renderer/src/lib/ensure-hooks-confirmed.test.ts index b21f0f39c4cb..808378e0502a 100644 --- a/src/renderer/src/lib/ensure-hooks-confirmed.test.ts +++ b/src/renderer/src/lib/ensure-hooks-confirmed.test.ts @@ -555,6 +555,36 @@ describe('ensureHooksConfirmed', () => { await expect(second).resolves.toBe('run') }) + it('lets a queued prompt for the same script see the approval recorded by the one before it', async () => { + const created = createTestState() + const { pending } = created + let state = created.state + hooksCheckMock.mockResolvedValue({ + hasHooks: true, + hooks: { scripts: { archive: 'echo bye' } }, + mayNeedUpdate: false + }) + + const first = ensureHooksConfirmed(() => state, 'repo-1', 'archive') + const second = ensureHooksConfirmed(() => state, 'repo-1', 'archive') + + await vi.waitFor(() => expect(pending).toHaveLength(1)) + // The store replaces its state when the dialog records trust, as zustand does. + state = { + ...state, + trustedOrcaHooks: { + 'repo-1': { + archive: { contentHash: await hashOrcaHookScript('echo bye'), approvedAt: 1 } + } + } + } + pending[0].resolve('run') + + await expect(first).resolves.toBe('run') + await expect(second).resolves.toBe('run') + expect(pending).toHaveLength(1) + }) + it('fails closed when window.api.hooks.check throws', async () => { const { state, pending } = createTestState() hooksCheckMock.mockRejectedValue(new Error('boom')) diff --git a/src/renderer/src/lib/ensure-hooks-confirmed.ts b/src/renderer/src/lib/ensure-hooks-confirmed.ts index 0a24c776c629..d392033335c5 100644 --- a/src/renderer/src/lib/ensure-hooks-confirmed.ts +++ b/src/renderer/src/lib/ensure-hooks-confirmed.ts @@ -234,7 +234,8 @@ export async function readAndConfirmRuntimeIssueCommand( } export async function ensureHooksConfirmed( - state: AppState, + // Why a getter: a caller that queues behind an earlier prompt must see the trust it recorded. + stateOrGetter: AppState | (() => AppState), repoId: string, scriptKind: HookScriptKind, hostId?: ExecutionHostId, @@ -245,6 +246,7 @@ export async function ensureHooksConfirmed( if (isCancelled()) { return 'skip' } + const state = typeof stateOrGetter === 'function' ? stateOrGetter() : stateOrGetter if (canUseRepoWideTrust(state, repoId)) { return 'run' } diff --git a/src/renderer/src/store/slices/worktrees-background-removal.test.ts b/src/renderer/src/store/slices/worktrees-background-removal.test.ts new file mode 100644 index 000000000000..2b8d1d8394e1 --- /dev/null +++ b/src/renderer/src/store/slices/worktrees-background-removal.test.ts @@ -0,0 +1,215 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { toast } from 'sonner' +import { createTestStore, makeWorktree, seedStore } from './store-test-helpers' +import { createStoreCascadesMockApi } from './store-cascades-test-harness' +import { clearRuntimeCompatibilityCacheForTests } from '../../runtime/runtime-rpc-client' +import { _resetHostWorktreeRemovalsForTests } from './worktrees/teardown/host-worktree-removal-state' +import { + _resetBackgroundWorktreeRemovalBridgeForTests, + reconcileHostWorktreeRemovals +} from '../../hooks/ipc-events/background-worktree-removal-bridge' +import { getWorktreeHostIdentity } from '../../../../shared/worktree/host-qualified-identity' + +vi.mock('sonner', () => ({ + toast: { info: vi.fn(), success: vi.fn(), error: vi.fn(), warning: vi.fn() } +})) +vi.mock('@/components/terminal-pane/pty-dispatcher', () => ({ + restorePtyDataHandlersAfterFailedShutdown: vi.fn(), + unregisterPtyDataHandlers: vi.fn(() => []) +})) + +const mockApi = createStoreCascadesMockApi() +const worktreeId = 'repo1::/path/wt1' +const hostKey = getWorktreeHostIdentity({ id: worktreeId, hostId: 'local' }) + +function seedRow( + store: ReturnType, + overrides: { removing?: true } = {} +): void { + seedStore(store, { + worktreesByRepo: { + repo1: [ + makeWorktree({ + id: worktreeId, + repoId: 'repo1', + path: '/path/wt1', + hostId: 'local', + ...overrides + }) + ] + } + }) +} + +function deleteState(store: ReturnType) { + const states = store.getState().deleteStateByWorktreeId + return states[hostKey] ?? states[worktreeId] +} + +describe('removing a worktree the host deletes in the background', () => { + let store: ReturnType + + beforeEach(() => { + vi.clearAllMocks() + clearRuntimeCompatibilityCacheForTests() + store = createTestStore() + }) + + afterEach(() => { + _resetHostWorktreeRemovalsForTests() + _resetBackgroundWorktreeRemovalBridgeForTests() + }) + + function deferredRemoval(): { + resolve: (value: unknown) => void + reject: (error: Error) => void + } { + const handle = { resolve: (_value: unknown) => {}, reject: (_error: Error) => {} } + mockApi.worktrees.remove.mockImplementation( + () => + new Promise((resolve, reject) => { + handle.resolve = resolve + handle.reject = reject + }) + ) + return handle + } + + /** Stands in for the listing refresh; `rows` is what the host lists once it lands. */ + function hostListsOnRefresh(rows: 'removed' | 'removing' | 'unmarked' | Error) { + const refresh = vi.fn() + refresh.mockImplementation(async () => { + if (rows instanceof Error) { + throw rows + } + if (rows === 'removed') { + seedStore(store, { worktreesByRepo: { repo1: [] } }) + } else { + seedRow(store, rows === 'removing' ? { removing: true } : {}) + } + return true + }) + store.setState({ fetchWorktrees: refresh }) + return refresh + } + + it('keeps the card Deleting until the host replies that the delete finished', async () => { + seedRow(store) + const host = deferredRemoval() + const removal = store.getState().removeWorktree({ id: worktreeId, executionHostId: null }) + await vi.waitFor(() => expect(mockApi.worktrees.remove).toHaveBeenCalled()) + + expect(deleteState(store)?.isDeleting).toBe(true) + expect(store.getState().worktreesByRepo.repo1?.map((row) => row.id)).toEqual([worktreeId]) + + host.resolve({ preservedBranch: { branchName: 'feature', head: 'abc123' } }) + await expect(removal).resolves.toMatchObject({ + ok: true, + preservedBranch: { branchName: 'feature', head: 'abc123' } + }) + expect(store.getState().worktreesByRepo.repo1).toEqual([]) + // The preserved-branch notice reaches the user through the same path an inline delete used. + expect(toast.warning).toHaveBeenCalledTimes(1) + }) + + it('shows the host error from the reply on the card', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + seedRow(store) + const refresh = hostListsOnRefresh('unmarked') + mockApi.worktrees.remove.mockRejectedValue( + new Error('Failed to delete worktree at /path/wt1. Permission denied') + ) + + await expect( + store.getState().removeWorktree({ id: worktreeId, executionHostId: null }) + ).resolves.toEqual({ + ok: false, + error: 'Failed to delete worktree at /path/wt1. Permission denied' + }) + expect(deleteState(store)).toMatchObject({ + isDeleting: false, + error: 'Failed to delete worktree at /path/wt1. Permission denied' + }) + // A host refusal is an answer, not a lost reply. + expect(refresh).not.toHaveBeenCalled() + }) + + it('finishes a delete whose reply timed out once the host listing drops the row', async () => { + seedRow(store) + const refresh = hostListsOnRefresh('removing') + mockApi.worktrees.remove.mockRejectedValue(new Error('Request timed out: worktree.rm')) + const removal = store.getState().removeWorktree({ id: worktreeId, executionHostId: null }) + + await vi.waitFor(() => expect(refresh).toHaveBeenCalledTimes(1)) + await Promise.resolve() + expect(deleteState(store)).toMatchObject({ isDeleting: true, error: null }) + + seedStore(store, { worktreesByRepo: { repo1: [] } }) + reconcileHostWorktreeRemovals(store) + await expect(removal).resolves.toEqual({ ok: true }) + }) + + it('reports a delete whose reply was lost and that the host lists without the marker', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + seedRow(store, { removing: true }) + hostListsOnRefresh('unmarked') + mockApi.worktrees.remove.mockRejectedValue(new Error('Remote Orca runtime connection closed.')) + + await expect( + store.getState().removeWorktree({ id: worktreeId, executionHostId: null }) + ).resolves.toEqual({ ok: false, error: 'The delete did not finish. Try again.' }) + }) + + it('treats a row the refreshed listing no longer has as deleted', async () => { + seedRow(store) + hostListsOnRefresh('removed') + mockApi.worktrees.remove.mockRejectedValue(new Error('Request timed out: worktree.rm')) + + await expect( + store.getState().removeWorktree({ id: worktreeId, executionHostId: null }) + ).resolves.toEqual({ ok: true }) + }) + + it('reports the lost reply when the host listing cannot be read either', async () => { + vi.spyOn(console, 'warn').mockImplementation(() => {}) + seedRow(store) + hostListsOnRefresh(new Error('Remote Orca runtime is not connected.')) + mockApi.worktrees.remove.mockRejectedValue(new Error('Request timed out: worktree.rm')) + + await expect( + store.getState().removeWorktree({ id: worktreeId, executionHostId: null }) + ).resolves.toEqual({ ok: false, error: 'Request timed out: worktree.rm' }) + }) + + it('shows Deleting on a client that did not start the delete, from the host marker alone', () => { + seedRow(store, { removing: true }) + reconcileHostWorktreeRemovals(store) + expect(deleteState(store)).toMatchObject({ isDeleting: true, phase: 'deleting' }) + + // Git finished: the row leaves the listing. + seedStore(store, { worktreesByRepo: { repo1: [] } }) + reconcileHostWorktreeRemovals(store) + expect(deleteState(store)).toBeUndefined() + }) + + it('shows the existing card error when a row it marked Deleting comes back unmarked', () => { + seedRow(store, { removing: true }) + reconcileHostWorktreeRemovals(store) + + seedRow(store) + reconcileHostWorktreeRemovals(store) + expect(deleteState(store)).toMatchObject({ + isDeleting: false, + error: 'The delete did not finish. Try again.' + }) + }) + + it('leaves a delete this renderer started to that flow', () => { + seedRow(store, { removing: true }) + store.getState().markWorktreesDeleting([{ id: worktreeId, hostId: 'local' }]) + reconcileHostWorktreeRemovals(store) + seedRow(store) + reconcileHostWorktreeRemovals(store) + expect(deleteState(store)?.isDeleting).toBe(true) + }) +}) diff --git a/src/renderer/src/store/slices/worktrees-remote-runtime-removal.test.ts b/src/renderer/src/store/slices/worktrees-remote-runtime-removal.test.ts index dc54b8c7763e..0bbe857ef6a6 100644 --- a/src/renderer/src/store/slices/worktrees-remote-runtime-removal.test.ts +++ b/src/renderer/src/store/slices/worktrees-remote-runtime-removal.test.ts @@ -1,5 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' -import { ARCHIVE_HOOK_TIMEOUT_MS } from '../../../../shared/worktree/archive-hook-removal-gate' +import { worktreeRemovalReplyTimeoutMs } from '../../../../shared/worktree/archive-hook-removal-gate' import type { AppState } from '../types' import type { RuntimeEnvironmentCallRequest } from '../../runtime/runtime-compatibility-test-fixture' import { makeWorktree } from './worktrees-slice-test-fixtures' @@ -66,7 +66,7 @@ describe('worktree remote runtime mutations', () => { runHooks: true }, // Hooks run here, so the client must outlast the host's archive-hook budget (#19334). - timeoutMs: ARCHIVE_HOOK_TIMEOUT_MS + 60_000, + timeoutMs: worktreeRemovalReplyTimeoutMs(true), expectedEnvironmentPairingRevision: undefined, expectedRuntimeId: undefined }) @@ -130,7 +130,7 @@ describe('worktree remote runtime mutations', () => { runHooks: true }, // Hooks run here, so the client must outlast the host's archive-hook budget (#19334). - timeoutMs: ARCHIVE_HOOK_TIMEOUT_MS + 60_000, + timeoutMs: worktreeRemovalReplyTimeoutMs(true), expectedEnvironmentPairingRevision: undefined, expectedRuntimeId: undefined }) @@ -229,7 +229,7 @@ describe('worktree remote runtime mutations', () => { runHooks: true }, // Hooks run here, so the client must outlast the host's archive-hook budget (#19334). - timeoutMs: ARCHIVE_HOOK_TIMEOUT_MS + 60_000 + timeoutMs: worktreeRemovalReplyTimeoutMs(true) }) expect(mockApi.worktrees.remove).not.toHaveBeenCalled() expect(store.getState().worktreesByRepo['repo-ssh']).toEqual([]) diff --git a/src/renderer/src/store/slices/worktrees/teardown/dispatch-worktree-removal.ts b/src/renderer/src/store/slices/worktrees/teardown/dispatch-worktree-removal.ts index 3b2a11da6e4e..fa77425092de 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/dispatch-worktree-removal.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/dispatch-worktree-removal.ts @@ -3,7 +3,12 @@ import type { RemoveWorktreeResult } from '../../../../../../shared/worktree/cre import { callRuntimeRpc, type getActiveRuntimeTarget } from '../../../../runtime/runtime-rpc-client' import { toRuntimeWorktreeSelector } from '../../../../runtime/runtime-worktree-selector' import type { RemoveWorktreeOptions } from '../../worktree-removal-options' -import { ARCHIVE_HOOK_TIMEOUT_MS } from '../../../../../../shared/worktree/archive-hook-removal-gate' +import type { WorktreeSliceGet } from '../listing/worktree-slice-types' +import { + isWorktreeRemovalReplyLost, + settleLostWorktreeRemovalReply +} from './host-worktree-removal-state' +import { worktreeRemovalReplyTimeoutMs } from '../../../../../../shared/worktree/archive-hook-removal-gate' /** * Sends the destructive removal over whichever transport owns this workspace. @@ -17,13 +22,33 @@ export async function dispatchWorktreeRemoval(args: { hostId: ExecutionHostId | undefined force: boolean | undefined skipArchive: boolean - forgetLocalOnly: boolean + get: WorktreeSliceGet target: ReturnType options: RemoveWorktreeOptions | undefined /** Re-checks mid-flight ownership immediately before the destructive call. */ assertCurrent: () => void }): Promise { - const { worktreeId, hostId, force, skipArchive, forgetLocalOnly, target, options } = args + try { + return await requestWorktreeRemoval(args) + } catch (error) { + if (args.options?.mode === 'forget-local' || !isWorktreeRemovalReplyLost(error)) { + throw error + } + // Why: the host may still be deleting; its listing answers what the lost reply would have. + await settleLostWorktreeRemovalReply(args.get, { + worktreeId: args.worktreeId, + hostId: args.hostId, + replyError: error + }) + return {} + } +} + +async function requestWorktreeRemoval( + args: Parameters[0] +): Promise { + const { worktreeId, hostId, force, skipArchive, target, options } = args + const forgetLocalOnly = options?.mode === 'forget-local' const snapshotPruneBatch = options?.snapshotPruneBatchId ? { snapshotPruneBatchId: options.snapshotPruneBatchId } : {} @@ -58,12 +83,9 @@ export async function dispatchWorktreeRemoval(args: { runHooks: !skipArchive }, { - // Why not a flat 60s (#19334): the host may run an archive hook for up to - // ARCHIVE_HOOK_TIMEOUT_MS before it decides anything. A client that gives up first reports a - // failure for a removal that is still in progress — and if the hook then succeeds, the host - // deletes the checkout while the user has been told the delete failed. Outlast the hook when - // one can run; keep the short budget when none will. - timeoutMs: skipArchive ? 60_000 : ARCHIVE_HOOK_TIMEOUT_MS + 60_000 + // Why (#19334): the host may run an archive hook before it decides anything, then waits for + // Git's delete before it replies; outlast both when a hook can run. + timeoutMs: worktreeRemovalReplyTimeoutMs(!skipArchive) } ) } diff --git a/src/renderer/src/store/slices/worktrees/teardown/host-worktree-removal-state.ts b/src/renderer/src/store/slices/worktrees/teardown/host-worktree-removal-state.ts new file mode 100644 index 000000000000..e89fa82ecbb5 --- /dev/null +++ b/src/renderer/src/store/slices/worktrees/teardown/host-worktree-removal-state.ts @@ -0,0 +1,112 @@ +import { + LOCAL_EXECUTION_HOST_ID, + type ExecutionHostId +} from '../../../../../../shared/execution-host' +import { + isRecoverableRemoteRuntimeConnectionError, + isRuntimeRpcQueueOverloadError, + toRemoteRuntimeClientErrorLike +} from '../../../../../../shared/remote-runtime-client-error-classification' +import { getRepoIdFromWorktreeId } from '../../../../../../shared/worktree/id' +import type { Worktree } from '../../../../../../shared/worktree/types' +import type { WorktreeSliceGet } from '../listing/worktree-slice-types' + +export const UNFINISHED_WORKTREE_REMOVAL_ERROR = 'The delete did not finish. Try again.' + +type RemovalRow = Pick + +function rowHostId(row: Pick): ExecutionHostId { + return row.hostId ?? LOCAL_EXECUTION_HOST_ID +} + +/** + * A delete request that failed in transport (a timeout or a dropped connection) says nothing about + * the delete: the host may have accepted it and still be deleting. A queue overload never sent it. + */ +export function isWorktreeRemovalReplyLost(error: unknown): boolean { + const reported = toRemoteRuntimeClientErrorLike(error) + if (isRuntimeRpcQueueOverloadError(reported)) { + return false + } + const message = reported.message.toLowerCase() + return ( + isRecoverableRemoteRuntimeConnectionError(reported) || + message.includes('request timed out') || + message.includes('connection interrupted') + ) +} + +const pendingJudgements = new Set<() => void>() + +/** + * Settles a delete whose reply was lost from the host's listing, as every other view does: the row + * leaving means the delete finished, and the row listed without `removing` means it did not. When + * the listing cannot be read either, rejects with the lost reply's error. + */ +function waitForHostWorktreeRemoval(args: { + hostId: ExecutionHostId | undefined + worktreeId: string + readRows: () => readonly RemovalRow[] + /** Re-reads the host's listing; true only when an authoritative listing was applied. */ + refresh: () => Promise + replyError: unknown +}): Promise { + const hostId = args.hostId ?? LOCAL_EXECUTION_HOST_ID + return new Promise((resolve, reject) => { + const judge = (): void => { + const row = args + .readRows() + .find((candidate) => candidate.id === args.worktreeId && rowHostId(candidate) === hostId) + if (row?.removing) { + return + } + pendingJudgements.delete(judge) + if (row) { + reject(new Error(UNFINISHED_WORKTREE_REMOVAL_ERROR)) + } else { + resolve() + } + } + // Why judge only after a refresh: the rows held when the reply was lost can predate the host + // accepting the delete, and would read as a delete that never started. + const fail = (): void => reject(args.replyError) + args.refresh().then((applied) => { + if (!applied) { + fail() + return + } + pendingJudgements.add(judge) + judge() + }, fail) + }) +} + +/** A lost reply for a delete of `worktreeId`, settled from its repo's listing on `hostId`. */ +export function settleLostWorktreeRemovalReply( + get: WorktreeSliceGet, + args: { worktreeId: string; hostId: ExecutionHostId | undefined; replyError: unknown } +): Promise { + const repoId = getRepoIdFromWorktreeId(args.worktreeId) + return waitForHostWorktreeRemoval({ + ...args, + readRows: () => [ + ...(get().worktreesByRepo[repoId] ?? []), + ...(get().detectedWorktreesByRepo[repoId]?.worktrees ?? []) + ], + refresh: () => + args.hostId + ? get().fetchWorktrees(repoId, { executionHostId: args.hostId }) + : get().fetchWorktrees(repoId) + }) +} + +/** Re-reads every delete waiting on the host's listing; call when listings change. */ +export function settleHostWorktreeRemovals(): void { + for (const judge of pendingJudgements) { + judge() + } +} + +export function _resetHostWorktreeRemovalsForTests(): void { + pendingJudgements.clear() +} diff --git a/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts b/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts index b8174b207cbd..3ca041aeb753 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts @@ -92,10 +92,11 @@ export function createRemoveWorktree( try { // Why: forget-local touches no remote, so there's no archive hook to run or trust prompt needed. + // Why `get`: same-repo local deletes start together, and one approval must cover the rest. const skipArchive = forgetLocalOnly ? true : (await ensureHooksConfirmed( - get(), + get, getRepoIdFromWorktreeId(worktreeId), 'archive', hostId, @@ -135,7 +136,7 @@ export function createRemoveWorktree( hostId, force, skipArchive, - forgetLocalOnly, + get, target, options, assertCurrent: () => removalGenerationGuard?.assertCurrent() diff --git a/src/renderer/src/web/preload-api/web-worktrees-api.ts b/src/renderer/src/web/preload-api/web-worktrees-api.ts index 2e11b258fed2..ee626b187547 100644 --- a/src/renderer/src/web/preload-api/web-worktrees-api.ts +++ b/src/renderer/src/web/preload-api/web-worktrees-api.ts @@ -7,6 +7,7 @@ import type { WorkspaceLineage, WorktreeLineage } from '../../../../shared/workt import { readRetiredNameRegistryForRepo } from '../../../../shared/worktree/retired-name-cache' import { EMPTY_RETIRED_NAME_REGISTRY } from '../../../../shared/worktree/retired-name-registry' import type { Worktree } from '../../../../shared/worktree/types' +import { worktreeRemovalReplyTimeoutMs } from '../../../../shared/worktree/archive-hook-removal-gate' import { toRuntimeWorktreeSelector } from '../../runtime/runtime-worktree-selector' import { callRuntimeResult, @@ -129,15 +130,20 @@ export function createWorktreesApi(): NonNullable['worktrees }), remove: async ({ worktreeId, hostId, force, allowUnverifiedPtyStop, skipArchive }) => { invalidateRuntimeWorktreeCaches() - return callRuntimeResult('worktree.rm', { - worktree: toRuntimeWorktreeSelector(worktreeId), - ...(hostId ? { hostId } : {}), - force, - // Why (#11960): the web client renders the same Force Delete affordances, so - // dropping this field here would leave paired clients permanently wedged. - allowUnverifiedPtyStop, - runHooks: skipArchive !== true - }) + return callRuntimeResult( + 'worktree.rm', + { + worktree: toRuntimeWorktreeSelector(worktreeId), + ...(hostId ? { hostId } : {}), + force, + // Why (#11960): the web client renders the same Force Delete affordances, so + // dropping this field here would leave paired clients permanently wedged. + allowUnverifiedPtyStop, + runHooks: skipArchive !== true + }, + // Why: the host replies once Git has deleted the checkout, past the default 30 s budget. + worktreeRemovalReplyTimeoutMs(skipArchive !== true) + ) }, // Why: forget-locally clears a desktop workspace pinned to a dead SSH host; a paired web client has no such ghost state. forgetLocal: () => { diff --git a/src/renderer/src/web/web-runtime-client.test.ts b/src/renderer/src/web/web-runtime-client.test.ts index 7e17e914433f..db94537297be 100644 --- a/src/renderer/src/web/web-runtime-client.test.ts +++ b/src/renderer/src/web/web-runtime-client.test.ts @@ -17,6 +17,7 @@ import { AGENT_SESSION_BOUNDARY_RUNTIME_CAPABILITY, SESSION_TAB_CLOSE_INTENT_RUNTIME_CAPABILITY, SESSION_TABS_RETIREMENT_PROOF_DELTA_RUNTIME_CAPABILITY, + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY, WORKTREE_GITHUB_PR_SUPPRESSION_RUNTIME_CAPABILITY, WORKTREE_VISIBILITY_DEFAULTS_RUNTIME_CAPABILITY, WORKTREE_VISIBILITY_SOURCE_DEFAULTS_RUNTIME_CAPABILITY @@ -95,7 +96,8 @@ describe('WebRuntimeClient', () => { AGENT_SESSION_BOUNDARY_RUNTIME_CAPABILITY, WORKTREE_GITHUB_PR_SUPPRESSION_RUNTIME_CAPABILITY, WORKTREE_VISIBILITY_DEFAULTS_RUNTIME_CAPABILITY, - WORKTREE_VISIBILITY_SOURCE_DEFAULTS_RUNTIME_CAPABILITY + WORKTREE_VISIBILITY_SOURCE_DEFAULTS_RUNTIME_CAPABILITY, + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY ] }) diff --git a/src/renderer/src/web/web-runtime-connection-frame-router.test.ts b/src/renderer/src/web/web-runtime-connection-frame-router.test.ts index 414fc5c40881..de0854bd532b 100644 --- a/src/renderer/src/web/web-runtime-connection-frame-router.test.ts +++ b/src/renderer/src/web/web-runtime-connection-frame-router.test.ts @@ -1,9 +1,12 @@ import { describe, expect, it, vi } from 'vitest' -import { WORKTREE_GITHUB_PR_SUPPRESSION_RUNTIME_CAPABILITY } from '../../../shared/protocol-version' +import { + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY, + WORKTREE_GITHUB_PR_SUPPRESSION_RUNTIME_CAPABILITY +} from '../../../shared/protocol-version' import { routeWebRuntimeConnectionFrame } from './web-runtime-connection-frame-router' describe('web runtime connection capability advertisement', () => { - it('advertises GitHub PR suppression during E2EE authentication', async () => { + it('advertises GitHub PR suppression and the removing marker during E2EE authentication', async () => { const sendEncrypted = vi.fn(() => true) await routeWebRuntimeConnectionFrame(JSON.stringify({ type: 'e2ee_ready' }), undefined, { @@ -24,7 +27,9 @@ describe('web runtime connection capability advertisement', () => { expect.objectContaining({ type: 'e2ee_auth', clientCapabilities: expect.arrayContaining([ - WORKTREE_GITHUB_PR_SUPPRESSION_RUNTIME_CAPABILITY + WORKTREE_GITHUB_PR_SUPPRESSION_RUNTIME_CAPABILITY, + // The web client runs the desktop renderer, which shows Deleting from the marker. + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY ]) }) ) diff --git a/src/renderer/src/web/web-runtime-connection-frame-router.ts b/src/renderer/src/web/web-runtime-connection-frame-router.ts index e9fa20fa0150..df56f1438920 100644 --- a/src/renderer/src/web/web-runtime-connection-frame-router.ts +++ b/src/renderer/src/web/web-runtime-connection-frame-router.ts @@ -6,6 +6,7 @@ import { AGENT_SESSION_BOUNDARY_RUNTIME_CAPABILITY, SESSION_TAB_CLOSE_INTENT_RUNTIME_CAPABILITY, SESSION_TABS_RETIREMENT_PROOF_DELTA_RUNTIME_CAPABILITY, + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY, WORKTREE_GITHUB_PR_SUPPRESSION_RUNTIME_CAPABILITY, WORKTREE_VISIBILITY_DEFAULTS_RUNTIME_CAPABILITY, WORKTREE_VISIBILITY_SOURCE_DEFAULTS_RUNTIME_CAPABILITY @@ -67,7 +68,8 @@ export async function routeWebRuntimeConnectionFrame( AGENT_SESSION_BOUNDARY_RUNTIME_CAPABILITY, WORKTREE_GITHUB_PR_SUPPRESSION_RUNTIME_CAPABILITY, WORKTREE_VISIBILITY_DEFAULTS_RUNTIME_CAPABILITY, - WORKTREE_VISIBILITY_SOURCE_DEFAULTS_RUNTIME_CAPABILITY + WORKTREE_VISIBILITY_SOURCE_DEFAULTS_RUNTIME_CAPABILITY, + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY ] }) return diff --git a/src/shared/git-binary-compatibility.test.ts b/src/shared/git-binary-compatibility.test.ts index 457dad2d4190..af9dcb9d2af1 100644 --- a/src/shared/git-binary-compatibility.test.ts +++ b/src/shared/git-binary-compatibility.test.ts @@ -1,5 +1,5 @@ import { execFile } from 'node:child_process' -import { mkdir, mkdtemp, readFile, rename, rm, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises' import { tmpdir } from 'node:os' import { join } from 'node:path' import { promisify } from 'node:util' @@ -192,19 +192,6 @@ describeBinaryCompatibility('real Git binary compatibility', () => { } }) - it('deregisters a worktree whose directory was renamed away', async () => { - // Orca renames the checkout into a trash directory and then clears the registration, so every - // supported Git must accept `worktree remove --force` on the now-missing path. - await runGit(['worktree', 'add', '-b', 'compat-deferred', 'deferred-wt']) - await rename(join(repoPath, 'deferred-wt'), join(repoPath, 'deferred-trash')) - - await expect(runGit(['worktree', 'remove', '--force', 'deferred-wt'])).resolves.toBeDefined() - - const remaining = await runGit(['worktree', 'list', '--porcelain']) - expect(remaining.stdout).not.toContain('deferred-wt') - await rm(join(repoPath, 'deferred-trash'), { recursive: true, force: true }) - }) - it('removes locked prepared worktrees without a separate unlock', async () => { await runGit(['worktree', 'add', '--detach', '--no-checkout', 'compat-discard', 'HEAD']) await runGit(['-C', 'compat-discard', 'reset', '--hard', 'HEAD']) diff --git a/src/shared/protocol-version.ts b/src/shared/protocol-version.ts index d3880313897a..5e0df0b25d18 100644 --- a/src/shared/protocol-version.ts +++ b/src/shared/protocol-version.ts @@ -273,6 +273,11 @@ export const WORKTREE_VISIBILITY_DEFAULTS_RUNTIME_CAPABILITY = 'worktree.visibility-defaults.v1' as const export const WORKTREE_VISIBILITY_SOURCE_DEFAULTS_RUNTIME_CAPABILITY = 'worktree.visibility-source-defaults.v1' as const +// Why: Git's checkout delete outlives worktree.rm's older client timeouts. A client with this waits +// for the reply and shows `removing` rows as Deleting; one without it is answered on acceptance and +// would re-show that row as a normal workspace, so the host leaves such rows out of its listings. +export const WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY = + 'worktree.background-removal.v1' as const // Why: older hosts drop automation.list's selector and answer with the whole authority, so a scoped client must not read that as one host's rows. export const AUTOMATION_LIST_HOST_SCOPE_RUNTIME_CAPABILITY = 'automation.list-host-scope.v1' as const @@ -333,7 +338,9 @@ export const ELECTRON_REMOTE_RUNTIME_CLIENT_CAPABILITIES = [ BROWSER_CLIENT_HOST_RUNTIME_CAPABILITY, BROWSER_CLIENT_PAGE_METADATA_RUNTIME_CAPABILITY, // Why: only the renderer runs the retirement-proof ledger; CLI and mobile must keep full lists. - SESSION_TABS_RETIREMENT_PROOF_DELTA_RUNTIME_CAPABILITY + SESSION_TABS_RETIREMENT_PROOF_DELTA_RUNTIME_CAPABILITY, + // Only the renderer shows Deleting for a `removing` row; CLI and mobile get those rows omitted. + WORKTREE_BACKGROUND_REMOVAL_RUNTIME_CAPABILITY ] as const export const ANTIGRAVITY_CONFIGURED_MODEL_RUNTIME_CAPABILITY = diff --git a/src/shared/runtime-rpc-call-queue.test.ts b/src/shared/runtime-rpc-call-queue.test.ts index 0c0219d05f2e..636698bf0e83 100644 --- a/src/shared/runtime-rpc-call-queue.test.ts +++ b/src/shared/runtime-rpc-call-queue.test.ts @@ -44,6 +44,31 @@ describe('runtime RPC call queue', () => { await expect(background2).resolves.toBe('background-2') }) + it('runs foreground calls while worktree deletes wait, still bounding the deletes', async () => { + const queue = new RuntimeRpcCallQueuePool(2, 1) + const started: string[] = [] + const pending: (() => void)[] = [] + const deletes = ['rm-1', 'rm-2', 'rm-3'].map((label) => + queue.enqueue('web-runtime', 'worktree.rm', async () => { + started.push(label) + await new Promise((resolve) => pending.push(resolve)) + return label + }) + ) + await vi.waitFor(() => expect(started).toEqual(['rm-1', 'rm-2'])) + + const listing = queue.enqueue('web-runtime', 'worktree.list', async () => 'listed') + await expect(listing).resolves.toBe('listed') + expect(started).toEqual(['rm-1', 'rm-2']) + + pending.shift()?.() + await expect(deletes[0]).resolves.toBe('rm-1') + await vi.waitFor(() => expect(started).toEqual(['rm-1', 'rm-2', 'rm-3'])) + pending.shift()?.() + pending.shift()?.() + await expect(Promise.all(deletes)).resolves.toEqual(['rm-1', 'rm-2', 'rm-3']) + }) + it('frees the queue slot when a runtime call throws synchronously', async () => { const queue = new RuntimeRpcCallQueuePool(1, 1) const first = queue.enqueue('web-runtime', 'status.get', () => { diff --git a/src/shared/runtime-rpc-call-queue.ts b/src/shared/runtime-rpc-call-queue.ts index 7a3c1842602b..132c80992734 100644 --- a/src/shared/runtime-rpc-call-queue.ts +++ b/src/shared/runtime-rpc-call-queue.ts @@ -51,6 +51,12 @@ export function isBackgroundRuntimeMethod(method: string): boolean { ) } +// Why its own lane: worktree.rm replies only when Git has deleted the checkout, so its calls would +// hold the foreground slots listing refreshes need; the background lane's slots belong to status. +function isLongWaitRuntimeMethod(method: string): boolean { + return method === 'worktree.rm' +} + export class RuntimeRpcCallQueuePool { private readonly queues = new Map() private queuedCallCount = 0 @@ -74,10 +80,12 @@ export class RuntimeRpcCallQueuePool { if (signal?.aborted) { return Promise.reject(abortSignalReason(signal)) } + // Same concurrency bound, counted apart from the selector's other calls; global caps still apply. + const queueKey = isLongWaitRuntimeMethod(method) ? `${selector}\u0000long-wait` : selector if (this.queuedCallCount >= this.maxQueuedTotal) { return Promise.reject(new RuntimeRpcCallQueueOverloadError('global')) } - const existingQueue = this.queues.get(selector) + const existingQueue = this.queues.get(queueKey) if (existingQueue && this.queuedCount(existingQueue) >= this.maxQueuedPerSelector) { return Promise.reject(new RuntimeRpcCallQueueOverloadError('selector')) } @@ -89,7 +97,7 @@ export class RuntimeRpcCallQueuePool { return Promise.reject(new RuntimeRpcCallQueueOverloadError('memory')) } - const queue = this.getQueue(selector) + const queue = this.getQueue(queueKey) return new Promise((resolve, reject) => { const call: QueuedRuntimeCall = { background: isBackgroundRuntimeMethod(method), @@ -105,10 +113,10 @@ export class RuntimeRpcCallQueuePool { this.queuedCallCount += 1 this.retainedCallBytes += retainedBytes if (signal) { - call.onAbort = () => this.cancelQueuedCall(selector, queue, call) + call.onAbort = () => this.cancelQueuedCall(queueKey, queue, call) signal.addEventListener('abort', call.onAbort, { once: true }) } - this.pump(selector, queue) + this.pump(queueKey, queue) }) } diff --git a/src/shared/runtime-worktree-contracts.ts b/src/shared/runtime-worktree-contracts.ts index 3d95fb0a3dc9..68e83c3268ca 100644 --- a/src/shared/runtime-worktree-contracts.ts +++ b/src/shared/runtime-worktree-contracts.ts @@ -76,6 +76,8 @@ export type RuntimeWorktreePsSummary = { /** Optional discriminator for a working workspace; older clients fall back to ordinary working. */ workingMode?: AgentWorkingMode agents: RuntimeWorktreeAgentRow[] + /** See `Worktree.removing`; sent only to clients that advertise background removal. */ + removing?: true } export type RuntimeGitLocalBranches = { diff --git a/src/shared/worktree/archive-hook-removal-gate.ts b/src/shared/worktree/archive-hook-removal-gate.ts index 9dcf270d28e1..d239bc212e22 100644 --- a/src/shared/worktree/archive-hook-removal-gate.ts +++ b/src/shared/worktree/archive-hook-removal-gate.ts @@ -10,6 +10,15 @@ */ export const ARCHIVE_HOOK_TIMEOUT_MS = 120_000 +/** + * How long a client waits for worktree.rm's reply. Git's checkout delete has no timeout (measured + * up to ~60 s under load) and a bulk delete queues behind the host's two-at-a-time limit; a client + * that gives up instead settles the card from the host's listing, so this is not a failure deadline. + */ +export function worktreeRemovalReplyTimeoutMs(runsArchiveHook: boolean): number { + return (runsArchiveHook ? ARCHIVE_HOOK_TIMEOUT_MS : 0) + 180_000 +} + /** RPC/CLI error code for a removal refused because the repo's archive hook did not succeed. */ export const ARCHIVE_HOOK_FAILED_REMOVAL_CODE = 'worktree_archive_hook_failed' diff --git a/src/shared/worktree/create-types.ts b/src/shared/worktree/create-types.ts index 97f268d5b08a..5032df3924c3 100644 --- a/src/shared/worktree/create-types.ts +++ b/src/shared/worktree/create-types.ts @@ -215,6 +215,9 @@ export type RemoveWorktreeResult = { preservedBranch?: PreservedWorktreeBranch /** Present only when a FAILED archive hook was explicitly waived for this removal (#19334). */ archiveHookOverride?: ArchiveHookOverride + /** The host accepted the removal and is still deleting the checkout. Sent only to clients that + * cannot show a removal in progress; the others get the reply when the delete has finished. */ + removing?: true } export type ForceDeleteWorktreeBranchResult = { diff --git a/src/shared/worktree/types.ts b/src/shared/worktree/types.ts index b7f02585a084..ce2bd025e28c 100644 --- a/src/shared/worktree/types.ts +++ b/src/shared/worktree/types.ts @@ -143,6 +143,9 @@ export type Worktree = { mobileDiffReview?: MobileDiffReviewState automationProvenance?: AutomationWorkspaceProvenance cliProvenance?: CliWorkspaceProvenance + /** The host is deleting this checkout in the background; Git lists it until that finishes. + * Sent only to clients that advertise `worktree.background-removal.v1`. */ + removing?: true } & GitWorktreeInfo /** Provenance for workspaces created through `orca worktree create`. Absent on