From 087f1aa9bf869f1cedd3e6c74294a420a82272ae Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Sat, 16 May 2026 13:52:59 -0700 Subject: [PATCH] fix(worktree): preflight delete before terminal teardown (#2103) Implement delete preflight checks before PTY teardown across git/runtime/ipc flows. Design reference: docs/worktree-delete-preflight.md --- docs/worktree-delete-preflight.md | 78 +++++++++++++++++ src/main/git/remove-worktree.test.ts | 47 ++++++++++- src/main/git/worktree.ts | 23 +++++ src/main/ipc/worktree-logic.test.ts | 33 ++++++++ src/main/ipc/worktree-logic.ts | 19 +++++ src/main/ipc/worktrees.test.ts | 76 ++++++++++++++++- src/main/ipc/worktrees.ts | 66 +++++++++------ src/main/runtime/orca-runtime.test.ts | 83 ++++++++++++++++++- src/main/runtime/orca-runtime.ts | 59 ++++++++----- .../src/store/slices/store-cascades.test.ts | 6 +- src/renderer/src/store/slices/worktrees.ts | 26 +++--- 11 files changed, 450 insertions(+), 66 deletions(-) create mode 100644 docs/worktree-delete-preflight.md diff --git a/docs/worktree-delete-preflight.md b/docs/worktree-delete-preflight.md new file mode 100644 index 000000000..1b8263762 --- /dev/null +++ b/docs/worktree-delete-preflight.md @@ -0,0 +1,78 @@ +# Worktree Delete Preflight + +## Problem + +- Local delete paths kill PTYs before git deletion: +- `worktrees:remove` IPC (`src/main/ipc/worktrees.ts`) +- `removeManagedWorktree` runtime/RPC (`src/main/runtime/orca-runtime.ts`) +- On non-force failures (dirty/untracked is common), the worktree stays on disk but terminals are already gone. +- PTY teardown is intentionally destructive and best-effort (`src/main/runtime/worktree-teardown.ts`), so non-force deletability must be checked first. + +## Ground Truth From Code + +- There is no dry-run remove path currently used in git helpers (`src/main/git/worktree.ts`). +- Errors shown to users are normalized through `formatWorktreeRemovalError` (`src/main/ipc/worktree-logic.ts`). +- SSH-backed repos already delegate deletion to provider APIs and should remain provider-owned. +- Current orphan cleanup (`is not a working tree` handling + prune + metadata cleanup) lives in IPC/runtime remove catch blocks and must not regress. + +## Non-goals + +- Do not change force-delete semantics; force may still kill PTYs before git remove. +- Do not add new renderer confirmation states or copy. +- Do not attempt to predict every possible git remove failure. +- Do not change SSH provider teardown ownership. + +## Design + +1. Add local preflight helper in `src/main/git/worktree.ts`. +- Export `assertWorktreeCleanForRemoval(worktreePath: string, force = false): Promise`. +- If `force`, return immediately. +- Run `git status --porcelain --untracked-files=all` in `cwd = worktreePath`. +- If output is non-empty, throw a dedicated error (dirty/untracked). +- If command fails, rethrow original error. + +2. IPC local delete ordering (`worktrees:remove`). +- Keep canonicalization and protected-path validation first (`getRegisteredDeletableWorktree`). +- Keep SSH provider branch unchanged. +- Keep archive hook and symlink cleanup before preflight, so preflight checks the exact post-hook/post-symlink state that `git worktree remove` will see. +- Run preflight. +- If preflight throws an orphan/missing-worktree style error, continue to existing remove path so current orphan cleanup behavior still executes. + Treat at least these as orphan-compatible for preflight: `is not a working tree`, `not a git repository`, and missing-path (`ENOENT`) failures from running status in a removed directory. +- For other preflight failures, throw via `formatWorktreeRemovalError(...)`. +- Only after successful preflight: run `killAllProcessesForWorktree(...)`, then `removeWorktree(...)`. + +3. Runtime/RPC local delete ordering (`removeManagedWorktree`). +- Keep SSH branch unchanged. +- Keep current hook behavior (archive optional via `--run-hooks`, warning when configured but skipped). +- Run preflight after hook handling. +- Preserve orphan compatibility exactly as in IPC: preflight must not short-circuit existing orphan cleanup semantics (including `not a git repository`/`ENOENT` preflight failures that should fall through to the existing remove/catch path). +- On successful preflight: run PTY teardown, then `removeWorktree`. +- Route failures through existing formatted error surface. + +4. Failure-class behavior contract. +- Dirty/untracked (non-force): fail before PTY teardown. +- Preflight subprocess/tooling failures: fail before PTY teardown, formatted. +- Orphan/missing-worktree conditions (`is not a working tree`, `not a git repository`, `ENOENT`): retain current cleanup-and-metadata-removal behavior by running the existing remove/catch flow without PTY teardown. +- Force deletes: no preflight; keep current teardown-before-remove order. + +5. Tests. +- Update ordering assertions for non-force local deletes to `preflight -> kill -> git`. +- Add IPC/runtime tests proving dirty non-force failures happen before any PTY kill. +- Add IPC/runtime tests proving preflight error formatting uses `formatWorktreeRemovalError` path. +- Add IPC/runtime regression tests proving orphan cleanup still runs when preflight encounters orphan-like failures. +- Keep force ordering tests (`kill -> git`). +- Keep SSH tests proving local PTY teardown is not used for SSH-backed repos. + +## Concurrency, Consistency, Limits + +- Preflight narrows, but does not close, the race window: external edits can occur after preflight and before `git worktree remove`. +- Multi-window and out-of-band mutation races remain possible between canonicalization, hooks/symlink cleanup, preflight, kill, and remove. +- IPC has symlink cleanup before preflight; runtime does not. That asymmetry remains unless runtime gains equivalent cleanup. +- Cost is one additional git subprocess per non-force local delete; this is acceptable for a user-initiated destructive action. + +## Rollout + +1. Implement `assertWorktreeCleanForRemoval` + unit tests in `src/main/git/remove-worktree.test.ts`. +2. Wire IPC ordering and failure mapping in `src/main/ipc/worktrees.ts`; update `src/main/ipc/worktrees.test.ts`. +3. Wire runtime ordering and failure mapping in `src/main/runtime/orca-runtime.ts`; update `src/main/runtime/orca-runtime.test.ts`. +4. Run focused tests, then `pnpm typecheck` and `pnpm lint`. diff --git a/src/main/git/remove-worktree.test.ts b/src/main/git/remove-worktree.test.ts index 4bd5b1db3..c991a3b76 100644 --- a/src/main/git/remove-worktree.test.ts +++ b/src/main/git/remove-worktree.test.ts @@ -32,7 +32,12 @@ vi.mock('fs/promises', async () => { return { ...actual, stat: statMock } }) -import { addSparseWorktree, listWorktrees, removeWorktree } from './worktree' +import { + addSparseWorktree, + assertWorktreeCleanForRemoval, + listWorktrees, + removeWorktree +} from './worktree' type MockResult = { error?: Error @@ -292,6 +297,46 @@ branch refs/heads/main }) }) +describe('assertWorktreeCleanForRemoval', () => { + beforeEach(() => { + gitExecFileAsyncMock.mockReset() + }) + + it('returns without checking git status for force removals', async () => { + await expect(assertWorktreeCleanForRemoval('/repo-feature', true)).resolves.toBeUndefined() + expect(gitExecFileAsyncMock).not.toHaveBeenCalled() + }) + + it('passes when git status output is empty', async () => { + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '', stderr: '' }) + + await expect(assertWorktreeCleanForRemoval('/repo-feature')).resolves.toBeUndefined() + + expect(gitExecFileAsyncMock).toHaveBeenCalledWith( + ['status', '--porcelain', '--untracked-files=all'], + { cwd: '/repo-feature' } + ) + }) + + it('throws a dedicated dirty/untracked error when status output is non-empty', async () => { + gitExecFileAsyncMock.mockResolvedValueOnce({ stdout: '?? scratch.txt\n', stderr: '' }) + + await expect(assertWorktreeCleanForRemoval('/repo-feature')).rejects.toMatchObject({ + message: 'Worktree has uncommitted or untracked changes.', + stdout: '?? scratch.txt\n' + }) + }) + + it('rethrows preflight subprocess failures as-is', async () => { + const error = Object.assign(new Error('fatal: not a git repository'), { + stderr: 'fatal: not a git repository (or any of the parent directories): .git\n' + }) + gitExecFileAsyncMock.mockRejectedValueOnce(error) + + await expect(assertWorktreeCleanForRemoval('/repo-feature')).rejects.toBe(error) + }) +}) + describe('listWorktrees', () => { beforeEach(() => { gitExecFileAsyncMock.mockReset() diff --git a/src/main/git/worktree.ts b/src/main/git/worktree.ts index e26451c76..f5a91b5ba 100644 --- a/src/main/git/worktree.ts +++ b/src/main/git/worktree.ts @@ -378,6 +378,29 @@ export async function removeWorktree( } } +/** + * Assert a worktree is clean enough for non-force removal. + */ +export async function assertWorktreeCleanForRemoval( + worktreePath: string, + force = false +): Promise { + if (force) { + return + } + + const { stdout } = await gitExecFileAsync(['status', '--porcelain', '--untracked-files=all'], { + cwd: worktreePath + }) + if (!stdout.trim()) { + return + } + + const error = new Error('Worktree has uncommitted or untracked changes.') + ;(error as Error & { stdout?: string }).stdout = stdout + throw error +} + function translateWorktreePath(worktreePath: string, repoPath: string): string { const prefix = 'worktree ' const translated = translateWslOutputPaths(`${prefix}${worktreePath}`, repoPath) diff --git a/src/main/ipc/worktree-logic.test.ts b/src/main/ipc/worktree-logic.test.ts index 77073c295..73231eab8 100644 --- a/src/main/ipc/worktree-logic.test.ts +++ b/src/main/ipc/worktree-logic.test.ts @@ -13,6 +13,7 @@ import { mergeWorktree, parseWorktreeId, formatWorktreeRemovalError, + isOrphanCompatiblePreflightError, isOrphanedWorktreeError, areWorktreePathsEqual } from './worktree-logic' @@ -366,3 +367,35 @@ describe('isOrphanedWorktreeError', () => { expect(isOrphanedWorktreeError(null)).toBe(false) }) }) + +describe('isOrphanCompatiblePreflightError', () => { + it('matches not-a-working-tree errors', () => { + const error = Object.assign(new Error('git failed'), { + stderr: "fatal: '/some/path' is not a working tree" + }) + + expect(isOrphanCompatiblePreflightError(error)).toBe(true) + }) + + it('matches status failures from non-repo directories', () => { + const error = Object.assign(new Error('status failed'), { + stderr: 'fatal: not a git repository (or any of the parent directories): .git' + }) + + expect(isOrphanCompatiblePreflightError(error)).toBe(true) + }) + + it('matches missing directories by error code', () => { + const error = Object.assign(new Error('spawn git'), { code: 'ENOENT' }) + + expect(isOrphanCompatiblePreflightError(error)).toBe(true) + }) + + it('does not match unrelated subprocess failures', () => { + const error = Object.assign(new Error('status failed'), { + stderr: 'fatal: unable to read current working directory' + }) + + expect(isOrphanCompatiblePreflightError(error)).toBe(false) + }) +}) diff --git a/src/main/ipc/worktree-logic.ts b/src/main/ipc/worktree-logic.ts index 570c74e21..e689f3ae4 100644 --- a/src/main/ipc/worktree-logic.ts +++ b/src/main/ipc/worktree-logic.ts @@ -239,6 +239,25 @@ export function isOrphanedWorktreeError(error: unknown): boolean { return /is not a working tree/.test(msg) } +export function isOrphanCompatiblePreflightError(error: unknown): boolean { + if (isOrphanedWorktreeError(error)) { + return true + } + if (!(error instanceof Error)) { + return false + } + const errorWithDetails = error as Error & { code?: unknown; stderr?: string; stdout?: string } + const details = [ + errorWithDetails.stderr, + errorWithDetails.stdout, + errorWithDetails.message, + typeof errorWithDetails.code === 'string' ? errorWithDetails.code : undefined + ] + .filter((value): value is string => Boolean(value)) + .join('\n') + return /not a git repository/i.test(details) || /\bENOENT\b/i.test(details) +} + /** * Format a human-readable error message for worktree removal failures. */ diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 3a004a009..9632883b7 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -5,6 +5,7 @@ const { handleMock, removeHandlerMock, listWorktreesMock, + assertWorktreeCleanForRemovalMock, addWorktreeMock, addSparseWorktreeMock, removeWorktreeMock, @@ -31,6 +32,7 @@ const { handleMock: vi.fn(), removeHandlerMock: vi.fn(), listWorktreesMock: vi.fn(), + assertWorktreeCleanForRemovalMock: vi.fn(), addWorktreeMock: vi.fn(), addSparseWorktreeMock: vi.fn(), removeWorktreeMock: vi.fn(), @@ -64,6 +66,7 @@ vi.mock('electron', () => ({ vi.mock('../git/worktree', () => ({ listWorktrees: listWorktreesMock, + assertWorktreeCleanForRemoval: assertWorktreeCleanForRemovalMock, addWorktree: addWorktreeMock, addSparseWorktree: addSparseWorktreeMock, removeWorktree: removeWorktreeMock @@ -176,6 +179,7 @@ describe('registerWorktreeHandlers', () => { handleMock, removeHandlerMock, listWorktreesMock, + assertWorktreeCleanForRemovalMock, addWorktreeMock, addSparseWorktreeMock, removeWorktreeMock, @@ -219,6 +223,7 @@ describe('registerWorktreeHandlers', () => { providerStopped: 0, registryStopped: 0 }) + assertWorktreeCleanForRemovalMock.mockResolvedValue(undefined) getLocalPtyProviderMock.mockReturnValue({} as never) for (const key of Object.keys(handlers)) { @@ -1792,6 +1797,9 @@ describe('registerWorktreeHandlers', () => { mockKnownFeatureWorktree() getEffectiveHooksMock.mockReturnValue(null) const callOrder: string[] = [] + assertWorktreeCleanForRemovalMock.mockImplementation(async () => { + callOrder.push('preflight') + }) killAllProcessesForWorktreeMock.mockImplementation(async () => { callOrder.push('kill') return { runtimeStopped: 1, providerStopped: 0, registryStopped: 0 } @@ -1811,7 +1819,73 @@ describe('registerWorktreeHandlers', () => { }) ) expect(removeWorktreeMock).toHaveBeenCalled() - expect(callOrder).toEqual(['kill', 'git']) + expect(callOrder).toEqual(['preflight', 'kill', 'git']) + }) + + it('fails dirty non-force deletes before PTY teardown', async () => { + mockKnownFeatureWorktree() + getEffectiveHooksMock.mockReturnValue(null) + assertWorktreeCleanForRemovalMock.mockRejectedValue( + Object.assign(new Error('Worktree has uncommitted or untracked changes.'), { + stdout: '?? scratch.txt\n' + }) + ) + + await expect( + handlers['worktrees:remove'](null, { + worktreeId: 'repo-1::/workspace/feature-wt' + }) + ).rejects.toThrow('Failed to delete worktree at /workspace/feature-wt. ?? scratch.txt') + + expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(removeWorktreeMock).not.toHaveBeenCalled() + }) + + it('formats preflight subprocess failures and does not tear down PTYs', async () => { + mockKnownFeatureWorktree() + getEffectiveHooksMock.mockReturnValue(null) + assertWorktreeCleanForRemovalMock.mockRejectedValue( + Object.assign(new Error('status failed'), { + stderr: 'fatal: unable to read current working directory\n' + }) + ) + + await expect( + handlers['worktrees:remove'](null, { + worktreeId: 'repo-1::/workspace/feature-wt' + }) + ).rejects.toThrow( + 'Failed to delete worktree at /workspace/feature-wt. fatal: unable to read current working directory' + ) + + expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(removeWorktreeMock).not.toHaveBeenCalled() + }) + + it('falls through to orphan cleanup when preflight reports missing/non-repo worktree', async () => { + mockKnownFeatureWorktree() + getEffectiveHooksMock.mockReturnValue(null) + assertWorktreeCleanForRemovalMock.mockRejectedValue( + Object.assign(new Error('status failed'), { + stderr: 'fatal: not a git repository (or any of the parent directories): .git\n' + }) + ) + removeWorktreeMock.mockRejectedValue( + Object.assign(new Error('git worktree remove failed'), { + stderr: "fatal: '/workspace/feature-wt' is not a working tree" + }) + ) + gitExecFileAsyncMock.mockResolvedValue({ stdout: '', stderr: '' }) + + await handlers['worktrees:remove'](null, { + worktreeId: 'repo-1::/workspace/feature-wt' + }) + + expect(killAllProcessesForWorktreeMock).not.toHaveBeenCalled() + expect(removeWorktreeMock).toHaveBeenCalled() + expect(gitExecFileAsyncMock).toHaveBeenCalledWith(['worktree', 'prune'], { + cwd: '/workspace/repo' + }) }) it('skips the PTY teardown for SSH-backed repos (design §6 out-of-scope)', async () => { diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index 5f6c9b2b1..1ac4e4821 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -14,7 +14,11 @@ import type { Repo, WorktreeMeta } from '../../shared/types' -import { listWorktrees as listGitWorktrees, removeWorktree } from '../git/worktree' +import { + assertWorktreeCleanForRemoval, + listWorktrees as listGitWorktrees, + removeWorktree +} from '../git/worktree' import { gitExecFileAsync } from '../git/runner' import { getDefaultRemote } from '../git/repo' import { getPullRequestPushTarget, getWorkItem } from '../github/client' @@ -37,6 +41,7 @@ import { parseWorktreeId, areWorktreePathsEqual, formatWorktreeRemovalError, + isOrphanCompatiblePreflightError, isOrphanedWorktreeError } from './worktree-logic' import { @@ -682,31 +687,6 @@ export function registerWorktreeHandlers( registeredWorktrees ).path - // Why: kill every PTY belonging to this worktree BEFORE git-level - // removal. The renderer pre-kills via shutdownWorktreeTerminals, but - // defensive teardown here protects against: (a) a future renderer bug, - // (b) a disconnected window, (c) an out-of-band window.api.worktrees.remove - // caller. Placement is before the SSH early-return so local-host PTYs - // are still reaped for local repos; SSH-backed PTYs are handled by the - // remote provider's own teardown (design §4.3, §6). - if (!repo.connectionId) { - await killAllProcessesForWorktree(args.worktreeId, { - runtime, - localProvider: getLocalPtyProvider() - }) - .then((r) => { - const total = r.runtimeStopped + r.providerStopped + r.registryStopped - if (total > 0) { - console.info( - `[worktree-teardown] ${args.worktreeId} killed runtime=${r.runtimeStopped} provider=${r.providerStopped} registry=${r.registryStopped}` - ) - } - }) - .catch((err) => { - console.warn(`[worktree-teardown] failed for ${args.worktreeId}:`, err) - }) - } - if (repo.connectionId) { await provider!.removeWorktree(canonicalWorktreePath, args.force) runtime.clearOptimisticReconcileToken(args.worktreeId) @@ -734,6 +714,40 @@ export function registerWorktreeHandlers( await removeWorktreeSymlinks(canonicalWorktreePath, repo.symlinkPaths) } + let shouldTearDownPtys = true + try { + await assertWorktreeCleanForRemoval(canonicalWorktreePath, args.force ?? false) + } catch (error) { + if (!isOrphanCompatiblePreflightError(error)) { + throw new Error( + formatWorktreeRemovalError(error, canonicalWorktreePath, args.force ?? false) + ) + } + // Why: orphan cleanup does not need live shells to be killed first, + // and preflight did not prove the worktree is cleanly removable. + shouldTearDownPtys = false + } + + if (shouldTearDownPtys) { + // Why: once preflight proves normal deletion is clean, kill PTYs before + // git-level removal so shells cannot keep the directory busy. + await killAllProcessesForWorktree(args.worktreeId, { + runtime, + localProvider: getLocalPtyProvider() + }) + .then((r) => { + const total = r.runtimeStopped + r.providerStopped + r.registryStopped + if (total > 0) { + console.info( + `[worktree-teardown] ${args.worktreeId} killed runtime=${r.runtimeStopped} provider=${r.providerStopped} registry=${r.registryStopped}` + ) + } + }) + .catch((err) => { + console.warn(`[worktree-teardown] failed for ${args.worktreeId}:`, err) + }) + } + try { await removeWorktree(repo.path, canonicalWorktreePath, args.force ?? false) } catch (error) { diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 1ee72ff3f..cae344efb 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -3,7 +3,12 @@ import { afterEach, describe, expect, it, vi } from 'vitest' import { EventEmitter } from 'events' import { mkdtemp, rm } from 'fs/promises' import type { WorktreeLineage, WorktreeMeta } from '../../shared/types' -import { addWorktree, listWorktrees, removeWorktree } from '../git/worktree' +import { + addWorktree, + assertWorktreeCleanForRemoval, + listWorktrees, + removeWorktree +} from '../git/worktree' import * as gitRunner from '../git/runner' import { createSetupRunnerScript, @@ -67,6 +72,7 @@ const { vi.mock('../git/worktree', () => ({ listWorktrees: vi.fn().mockResolvedValue(MOCK_GIT_WORKTREES), + assertWorktreeCleanForRemoval: vi.fn().mockResolvedValue(undefined), addWorktree: addWorktreeMock, removeWorktree: removeWorktreeMock })) @@ -122,6 +128,8 @@ vi.mock('../git/repo', async (importOriginal) => { afterEach(() => { vi.mocked(listWorktrees).mockResolvedValue(MOCK_GIT_WORKTREES) vi.mocked(addWorktree).mockReset() + vi.mocked(assertWorktreeCleanForRemoval).mockReset() + vi.mocked(assertWorktreeCleanForRemoval).mockResolvedValue(undefined) vi.mocked(removeWorktree).mockReset() sshGitProviders.clear() getSshGitProviderMock.mockReset() @@ -3820,6 +3828,73 @@ describe('OrcaRuntimeService', () => { ) }) + it('fails dirty non-force deletes before PTY teardown', async () => { + const runtime = new OrcaRuntimeService(store) + const killSpy = vi.fn().mockReturnValue(true) + runtime.setPtyController({ + write: () => true, + kill: (id) => killSpy(id), + getForegroundProcess: async () => null + }) + syncSinglePty(runtime, 'pty-1') + vi.mocked(getEffectiveHooks).mockReturnValue(null) + vi.mocked(assertWorktreeCleanForRemoval).mockRejectedValue( + Object.assign(new Error('Worktree has uncommitted or untracked changes.'), { + stdout: '?? scratch.txt\n' + }) + ) + + await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID)).rejects.toThrow( + `Failed to delete worktree at ${TEST_WORKTREE_PATH}. ?? scratch.txt` + ) + + expect(killSpy).not.toHaveBeenCalled() + expect(removeWorktree).not.toHaveBeenCalled() + }) + + it('formats preflight subprocess failures and skips PTY teardown', async () => { + const runtime = new OrcaRuntimeService(store) + const killSpy = vi.fn().mockReturnValue(true) + runtime.setPtyController({ + write: () => true, + kill: (id) => killSpy(id), + getForegroundProcess: async () => null + }) + syncSinglePty(runtime, 'pty-1') + vi.mocked(getEffectiveHooks).mockReturnValue(null) + vi.mocked(assertWorktreeCleanForRemoval).mockRejectedValue( + Object.assign(new Error('status failed'), { + stderr: 'fatal: unable to read current working directory\n' + }) + ) + + await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID)).rejects.toThrow( + `Failed to delete worktree at ${TEST_WORKTREE_PATH}. fatal: unable to read current working directory` + ) + + expect(killSpy).not.toHaveBeenCalled() + expect(removeWorktree).not.toHaveBeenCalled() + }) + + it('falls through to orphan cleanup when preflight reports missing/non-repo worktree', async () => { + const runtime = new OrcaRuntimeService(store) + vi.mocked(getEffectiveHooks).mockReturnValue(null) + vi.mocked(assertWorktreeCleanForRemoval).mockRejectedValue( + Object.assign(new Error('status failed'), { + stderr: 'fatal: not a git repository (or any of the parent directories): .git\n' + }) + ) + vi.mocked(removeWorktree).mockRejectedValue( + Object.assign(new Error('git worktree remove failed'), { + stderr: `fatal: '${TEST_WORKTREE_PATH}' is not a working tree` + }) + ) + vi.spyOn(gitRunner, 'gitExecFileAsync').mockResolvedValue({ stdout: '', stderr: '' }) + + await expect(runtime.removeManagedWorktree(TEST_WORKTREE_ID)).resolves.toEqual({}) + expect(removeWorktree).toHaveBeenCalledWith(TEST_REPO_PATH, TEST_WORKTREE_PATH, false) + }) + it('runs archive hooks for CLI worktree removal when hooks are explicitly enabled', async () => { const runtime = new OrcaRuntimeService(store) vi.mocked(getEffectiveHooks).mockReturnValue({ @@ -4228,6 +4303,9 @@ describe('OrcaRuntimeService', () => { const killSpy = vi.fn().mockReturnValue(true) const localProvider = createProviderStub(async () => []) const callOrder: string[] = [] + vi.mocked(assertWorktreeCleanForRemoval).mockImplementation(async () => { + callOrder.push('preflight') + }) vi.mocked(removeWorktree).mockImplementation(async () => { callOrder.push('git-removeWorktree') }) @@ -4253,8 +4331,11 @@ describe('OrcaRuntimeService', () => { expect(killSpy).toHaveBeenCalledWith('pty-1') // The provider-prefix sweep and the git removal must happen AFTER the // runtime-graph kill. Git removal must NOT happen before any kill. + const preflightIdx = callOrder.indexOf('preflight') const killIdx = callOrder.indexOf('kill:pty-1') const gitIdx = callOrder.indexOf('git-removeWorktree') + expect(preflightIdx).toBeGreaterThanOrEqual(0) + expect(killIdx).toBeGreaterThan(preflightIdx) expect(killIdx).toBeGreaterThanOrEqual(0) expect(gitIdx).toBeGreaterThan(killIdx) }) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 3beb7137f..47dfdeae4 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -204,7 +204,13 @@ import { getRemoteDrift, getRecentDriftSubjects } from '../git/repo' -import { listWorktrees, addWorktree, addSparseWorktree, removeWorktree } from '../git/worktree' +import { + listWorktrees, + addWorktree, + addSparseWorktree, + assertWorktreeCleanForRemoval, + removeWorktree +} from '../git/worktree' import { isENOENT } from '../ipc/filesystem-auth' import { createSetupRunnerScript, @@ -235,6 +241,7 @@ import { computeWorktreePath, ensurePathWithinWorkspace, formatWorktreeRemovalError, + isOrphanCompatiblePreflightError, isOrphanedWorktreeError, mergeWorktree, sanitizeWorktreeName, @@ -6016,15 +6023,36 @@ export class OrcaRuntimeService { return {} } - // Why: kill every PTY belonging to this worktree BEFORE the git-level - // removal. Some shells keep the worktree directory busy, and `git worktree - // remove` throws a confusing error if PTYs still hold it open. This also - // closes the headless-CLI leak (design §2a/§2b): without this call, the - // CLI path runs git removal and never touches PTYs, leaving zombies - // behind. Best-effort: any failure here must not prevent git removal — - // the worst case without the call is the status quo. + const hooks = getEffectiveHooks(repo) + let warning: string | undefined + if (hooks?.scripts.archive && runHooks) { + const result = await runHook('archive', worktree.path, repo) + if (!result.success) { + console.error(`[hooks] archive hook failed for ${worktree.path}:`, result.output) + } + } else if (hooks?.scripts.archive) { + // Runtime RPC calls have no renderer trust prompt, so hooks require explicit CLI opt-in. + warning = `orca.yaml archive hook skipped for ${worktree.path}; pass --run-hooks to run it.` + console.warn(`[hooks] ${warning}`) + } + + let shouldTearDownPtys = true + try { + await assertWorktreeCleanForRemoval(worktree.path, force) + } catch (error) { + if (!isOrphanCompatiblePreflightError(error)) { + throw new Error(formatWorktreeRemovalError(error, worktree.path, force)) + } + // Why: orphan cleanup does not need live shells to be killed first, + // and preflight did not prove the worktree is cleanly removable. + shouldTearDownPtys = false + } + const localProvider = this.getLocalProvider() - if (localProvider) { + if (localProvider && shouldTearDownPtys) { + // Why: once preflight proves normal deletion is clean, kill PTYs before + // git-level removal so shells cannot keep the directory busy. This also + // closes the headless-CLI leak for confirmed-removable worktrees. await killAllProcessesForWorktree(worktree.id, { runtime: this, localProvider @@ -6047,19 +6075,6 @@ export class OrcaRuntimeService { }) } - const hooks = getEffectiveHooks(repo) - let warning: string | undefined - if (hooks?.scripts.archive && runHooks) { - const result = await runHook('archive', worktree.path, repo) - if (!result.success) { - console.error(`[hooks] archive hook failed for ${worktree.path}:`, result.output) - } - } else if (hooks?.scripts.archive) { - // Runtime RPC calls have no renderer trust prompt, so hooks require explicit CLI opt-in. - warning = `orca.yaml archive hook skipped for ${worktree.path}; pass --run-hooks to run it.` - console.warn(`[hooks] ${warning}`) - } - try { await removeWorktree(repo.path, worktree.path, force) } catch (error) { diff --git a/src/renderer/src/store/slices/store-cascades.test.ts b/src/renderer/src/store/slices/store-cascades.test.ts index 6916cd94c..214d75dc5 100644 --- a/src/renderer/src/store/slices/store-cascades.test.ts +++ b/src/renderer/src/store/slices/store-cascades.test.ts @@ -167,6 +167,8 @@ describe('removeWorktree cascade', () => { // State NOT cleaned up expect(s.worktreesByRepo['repo1']).toHaveLength(1) expect(s.tabsByWorktree[worktreeId]).toHaveLength(1) + expect(s.ptyIdsByTabId['tab1']).toEqual(['pty1']) + expect(mockApi.pty.kill).not.toHaveBeenCalled() expect(s.activeWorktreeId).toBe(worktreeId) }) @@ -268,7 +270,7 @@ describe('removeWorktree cascade', () => { expect(s.fileSearchStateByWorktree[wt1]).toBeUndefined() }) - it('shuts down terminals before asking the backend to remove the worktree', async () => { + it('shuts down terminals after the backend confirms worktree removal', async () => { const store = createTestStore() const worktreeId = 'repo1::/path/wt1' const callOrder: string[] = [] @@ -298,7 +300,7 @@ describe('removeWorktree cascade', () => { const result = await store.getState().removeWorktree(worktreeId) expect(result).toEqual({ ok: true }) - expect(callOrder).toEqual(['kill', 'remove']) + expect(callOrder).toEqual(['remove', 'kill']) }) }) diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 04129dc6c..c55e3ee26 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -531,10 +531,19 @@ export const createWorktreeSlice: StateCreator const trustDecision = await ensureHooksConfirmed(get(), repoIdForTrust, 'archive') const skipArchive = trustDecision === 'skip' - // Why: setup-enabled worktrees now commonly have a live shell open as soon as - // they are created. We must tear those PTYs down before asking Git to remove - // the working tree or Windows and some shells can keep the directory in use - // and make delete look broken even though the git state itself is fine. + const target = getActiveRuntimeTarget(get().settings) + await (target.kind === 'local' + ? window.api.worktrees.remove({ worktreeId, force, skipArchive }) + : callRuntimeRpc( + target, + 'worktree.rm', + { worktree: worktreeId, force, runHooks: !skipArchive }, + { timeoutMs: 60_000 } + )) + + // Why: backend delete paths now preflight and kill PTYs only after the + // worktree is cleanly removable. Renderer state follows the successful + // backend result so blocked dirty deletes keep their terminals intact. // // Why browsers first: `shutdownWorktreeTerminals` used to own the // `browserTabsByWorktree[worktreeId]` delete as a side effect, which would @@ -545,15 +554,6 @@ export const createWorktreeSlice: StateCreator // can intercept them. await get().shutdownWorktreeBrowsers(worktreeId) await get().shutdownWorktreeTerminals(worktreeId) - const target = getActiveRuntimeTarget(get().settings) - await (target.kind === 'local' - ? window.api.worktrees.remove({ worktreeId, force, skipArchive }) - : callRuntimeRpc( - target, - 'worktree.rm', - { worktree: worktreeId, force, runHooks: !skipArchive }, - { timeoutMs: 60_000 } - )) const tabs = get().tabsByWorktree[worktreeId] ?? [] const tabIds = new Set(tabs.map((t) => t.id))