diff --git a/src/main/ipc/worktrees.test.ts b/src/main/ipc/worktrees.test.ts index 62be1f63f..8d576b965 100644 --- a/src/main/ipc/worktrees.test.ts +++ b/src/main/ipc/worktrees.test.ts @@ -8350,6 +8350,7 @@ describe('registerWorktreeHandlers', () => { expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith(worktreeId, { runtime: runtimeStub, + resolvedWorktreeId: worktreeId, localProvider: ptyProvider, onPtyStopped: clearProviderPtyStateMock }) @@ -8364,6 +8365,36 @@ describe('registerWorktreeHandlers', () => { }) }) + // Folder projects can be SSH-backed, and folder workspace ids are `repoId::path::workspace:` + // — reusable across hosts — so the sweep must name the owning connection. + it('fences an SSH folder workspace PTY sweep to the owning connection', async () => { + const sshPtyProvider = { id: 'ssh-pty-provider' } as never + const worktreeId = 'repo-folder::/remote/folder::workspace:child-1' + store.getRepo.mockReturnValue({ + id: 'repo-folder', + path: '/remote/folder', + displayName: 'folder', + badgeColor: '#000', + addedAt: 0, + kind: 'folder', + connectionId: 'conn-1' + }) + getSshPtyProviderMock.mockReturnValue(sshPtyProvider) + + await handlers['worktrees:remove'](null, { worktreeId }) + + expect(getSshPtyProviderMock).toHaveBeenCalledWith('conn-1') + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith(worktreeId, { + runtime: runtimeStub, + resolvedWorktreeId: worktreeId, + resolvedConnectionId: 'conn-1', + localProvider: sshPtyProvider, + onPtyStopped: clearProviderPtyStateMock, + includeProviderInventory: true, + includeLocalRegistry: false + }) + }) + it('runs the archive hook on remove when skipArchive is not set', async () => { mockKnownFeatureWorktree() removeWorktreeMock.mockResolvedValue(undefined) @@ -9950,6 +9981,70 @@ describe('registerWorktreeHandlers', () => { expect(callOrder).toEqual(['preflight', 'kill', 'git']) }) + // Regression: `repoId::path` ids repeat across hosts, so an SSH delete used to reach the + // runtime's same-id local (or other-connection) terminals and stop them. + it('fences an SSH worktree delete PTY sweep to the owning connection', async () => { + const repo = { + id: 'repo-ssh', + path: '/remote/repo', + displayName: 'ssh', + badgeColor: '#000', + addedAt: 0, + connectionId: 'conn-1', + worktreeBaseRef: null + } + const sshPtyProvider = { id: 'ssh-pty-provider' } as never + store.getRepos.mockReturnValue([repo]) + store.getRepo.mockReturnValue(repo) + getSshGitProviderMock.mockReturnValue({ + listWorktrees: vi.fn().mockResolvedValue([ + { path: '/remote/repo', head: 'main', branch: 'main', isBare: false, isMainWorktree: true }, + { + path: '/remote/feature-wt', + head: 'feature', + branch: 'feature', + isBare: false, + isMainWorktree: false + } + ]), + removeWorktree: vi.fn().mockResolvedValue({}), + worktreeIsClean: vi.fn().mockResolvedValue({ clean: true }) + }) + getSshPtyProviderMock.mockReturnValue(sshPtyProvider) + getEffectiveHooksFromConfigMock.mockReturnValue(null) + + await handlers['worktrees:remove'](null, { worktreeId: 'repo-ssh::/remote/feature-wt' }) + + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith('repo-ssh::/remote/feature-wt', { + runtime: runtimeStub, + resolvedWorktreeId: 'repo-ssh::/remote/feature-wt', + resolvedConnectionId: 'conn-1', + localProvider: sshPtyProvider, + onPtyStopped: clearProviderPtyStateMock, + requirePhysicalStop: true, + includeLocalRegistry: false + }) + }) + + // The local counterpart still identifies itself by exact id so a selector that resolves + // two hosts can no longer decide which workspace loses its terminals. + it('pins a local worktree delete PTY sweep to the exact worktree id', async () => { + mockKnownFeatureWorktree() + getEffectiveHooksMock.mockReturnValue(null) + removeWorktreeMock.mockResolvedValue({}) + + await handlers['worktrees:remove'](null, { worktreeId: 'repo-1::/workspace/feature-wt' }) + + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith( + 'repo-1::/workspace/feature-wt', + expect.objectContaining({ resolvedWorktreeId: 'repo-1::/workspace/feature-wt' }) + ) + expect(killAllProcessesForWorktreeMock).toHaveBeenCalledWith( + 'repo-1::/workspace/feature-wt', + expect.not.objectContaining({ resolvedConnectionId: expect.anything() }) + ) + }) + // Why (#11960): the PTY gate previously had no escape hatch at all, so a // workspace with an unprovable PTY was unremovable forever. it('forwards an explicit Force Delete to the PTY gate', async () => { diff --git a/src/main/ipc/worktrees.ts b/src/main/ipc/worktrees.ts index b9f2a57ce..dde33ca76 100644 --- a/src/main/ipc/worktrees.ts +++ b/src/main/ipc/worktrees.ts @@ -169,6 +169,11 @@ async function stopPtysForDestructiveWorktreeRemoval( } const teardownResult = await killAllProcessesForWorktree(worktreeId, { runtime, + // Why: `repoId::path` ids repeat across hosts, so an unfenced sweep stops a same-id + // workspace's terminals on another connection — and the selector lookup this replaces + // throws `selector_ambiguous` the moment two hosts own the id. + resolvedWorktreeId: worktreeId, + ...(connectionId ? { resolvedConnectionId: connectionId } : {}), localProvider: provider, onPtyStopped: clearProviderPtyState, requirePhysicalStop: true, @@ -2290,10 +2295,30 @@ export function registerWorktreeHandlers( } // Why: folder workspaces share one root, so there's no Git remove step to close shells; sweep PTYs before dropping metadata. await withWorktreeRemoveStageSpan('pty_sweep', 'folder', async () => { + // Folder projects can be SSH-backed, so fence the sweep to the owning host exactly + // like the git paths — the local inventory must never reach a remote workspace's id. + const ownerHost = parseExecutionHostId( + resolveWorktreeRemovalOwnerHostId(store, args.worktreeId, repo, args.hostId) + ) + const sshPtyProvider = + ownerHost?.kind === 'ssh' ? getSshPtyProvider(ownerHost.targetId) : undefined + const externalHost = ownerHost?.kind === 'ssh' || ownerHost?.kind === 'runtime' await killAllProcessesForWorktree(args.worktreeId, { runtime, - localProvider: getLocalPtyProvider(), - onPtyStopped: clearProviderPtyState + resolvedWorktreeId: args.worktreeId, + ...(ownerHost?.kind === 'ssh' ? { resolvedConnectionId: ownerHost.targetId } : {}), + ...(ownerHost?.kind === 'runtime' + ? { resolvedRuntimeEnvironmentId: ownerHost.environmentId } + : {}), + localProvider: sshPtyProvider ?? getLocalPtyProvider(), + onPtyStopped: clearProviderPtyState, + ...(externalHost + ? { + includeProviderInventory: + ownerHost?.kind === 'ssh' && Boolean(sshPtyProvider), + includeLocalRegistry: false + } + : {}) }).catch((err) => { console.warn(`[worktree-teardown] failed for ${args.worktreeId}:`, err) }) diff --git a/src/main/runtime/missing-worktree-terminal-reconciliation.test.ts b/src/main/runtime/missing-worktree-terminal-reconciliation.test.ts index f218f3a2c..22cf484a1 100644 --- a/src/main/runtime/missing-worktree-terminal-reconciliation.test.ts +++ b/src/main/runtime/missing-worktree-terminal-reconciliation.test.ts @@ -64,14 +64,23 @@ describe('stopMissingWorktreeTerminals', () => { const localProvider = createProvider([`${deletedId}@@local-session`]) const sshProvider = createProvider([`${deletedId}@@ssh-session`]) const getSshProvider = vi.fn(() => sshProvider) + const runtime = createRuntime() await stopMissingWorktreeTerminals({ ...localRepo, connectionId: 'ssh-1' }, [deletedId], [], { - runtime: createRuntime(), + runtime, getLocalProvider: () => localProvider, getSshProvider }) expect(getSshProvider).toHaveBeenCalledWith('ssh-1') + // The runtime graph holds both hosts' terminals under one id, so the sweep must fence to this one. + expect(runtime.stopTerminalsForWorktree).toHaveBeenCalledWith( + deletedId, + expect.objectContaining({ + resolvedWorktreeId: deletedId, + resolvedConnectionId: 'ssh-1' + }) + ) expect(sshProvider.shutdown).toHaveBeenCalledWith( `${deletedId}@@ssh-session`, expect.objectContaining({ immediate: true }) @@ -95,7 +104,12 @@ describe('stopMissingWorktreeTerminals', () => { ) expect(result).toEqual({ stoppedWorktreeIds: [deletedId] }) - expect(runtime.stopTerminalsForWorktree).toHaveBeenCalledWith(deletedId) + // The graph fallback still names the owning connection: this repo's inventory must not + // stop a same-id workspace's terminals on another host. + expect(runtime.stopTerminalsForWorktree).toHaveBeenCalledWith(deletedId, { + resolvedWorktreeId: deletedId, + resolvedConnectionId: 'ssh-1' + }) }) // Why: an agent cleaning up workspaces deletes many at once. Enumerating the diff --git a/src/main/runtime/missing-worktree-terminal-reconciliation.ts b/src/main/runtime/missing-worktree-terminal-reconciliation.ts index b25c4fc9f..6c4f7f762 100644 --- a/src/main/runtime/missing-worktree-terminal-reconciliation.ts +++ b/src/main/runtime/missing-worktree-terminal-reconciliation.ts @@ -46,6 +46,18 @@ function withSharedProcessSnapshot(provider: IPtyProvider): IPtyProvider { }) } +// Why: `repoId::path` ids repeat across hosts, so a sweep driven by one repo's inventory +// must name its owner or it stops a same-id workspace's terminals on another host. +function hostFence( + repo: Repo, + worktreeId: string +): { resolvedWorktreeId: string; resolvedConnectionId?: string } { + return { + resolvedWorktreeId: worktreeId, + ...(repo.connectionId ? { resolvedConnectionId: repo.connectionId } : {}) + } +} + type MissingWorktreeTerminalReconciliationDeps = { runtime: OrcaRuntimeService getLocalProvider: () => IPtyProvider | null @@ -83,7 +95,7 @@ export async function stopMissingWorktreeTerminals( MISSING_WORKTREE_TEARDOWN_CONCURRENCY, async (worktreeId) => { try { - await deps.runtime.stopTerminalsForWorktree(worktreeId) + await deps.runtime.stopTerminalsForWorktree(worktreeId, hostFence(repo, worktreeId)) return worktreeId } catch { return null @@ -102,6 +114,7 @@ export async function stopMissingWorktreeTerminals( try { await killAllProcessesForWorktree(worktreeId, { runtime: deps.runtime, + ...hostFence(repo, worktreeId), localProvider: provider, onPtyStopped: deps.onPtyStopped, // Why: the shared process snapshot is only valid while nothing needs a diff --git a/src/main/runtime/orca-runtime.test.ts b/src/main/runtime/orca-runtime.test.ts index 6fb0c2386..d8c943670 100644 --- a/src/main/runtime/orca-runtime.test.ts +++ b/src/main/runtime/orca-runtime.test.ts @@ -5923,6 +5923,66 @@ describe('OrcaRuntimeService', () => { expect(deleteWorktreeHistoryDirMock).toHaveBeenCalledWith(`${TEST_REPO_ID}::/remote/feature`) }) + // Regression: `repoId::path` ids repeat across hosts, so the SSH delete's runtime sweep used to + // stop the same-id local workspace's terminals too. + it('leaves a same-id local terminal running when the SSH copy is removed', async () => { + const remoteRepo = { + id: TEST_REPO_ID, + path: '/remote/repo', + displayName: 'repo', + badgeColor: 'blue', + addedAt: 1, + connectionId: 'ssh-1' + } + const remoteStore = { ...store, getRepos: () => [remoteRepo], getRepo: () => remoteRepo } + const gitProvider = { + listWorktrees: vi.fn().mockResolvedValue([ + { + path: '/remote/repo', + head: 'main', + branch: 'refs/heads/main', + isBare: false, + isMainWorktree: true + }, + { + path: '/remote/feature', + head: 'abc', + branch: 'feature/foo', + isBare: false, + isMainWorktree: false + } + ]), + removeWorktree: vi.fn().mockResolvedValue(undefined) + } + registerSshGitProvider('ssh-1', gitProvider as never) + const ptyProvider = { + listProcesses: vi.fn().mockResolvedValue([]), + shutdown: vi.fn().mockResolvedValue(undefined) + } + const runtime = new OrcaRuntimeService(remoteStore as never, undefined, { + getSshProvider: () => ptyProvider as never + }) + const stopAndWait = vi.fn(async () => true) + runtime.setPtyController({ + write: () => true, + kill: vi.fn(() => true), + stopAndWait, + getForegroundProcess: async () => null + }) + syncSinglePty(runtime, null) + runtime.registerPty('pty-remote', `${TEST_REPO_ID}::/remote/feature`, 'ssh-1') + runtime.registerPty('pty-local-same-id', `${TEST_REPO_ID}::/remote/feature`, null) + + try { + await runtime.removeManagedWorktree('path:/remote/feature', true, false) + } finally { + unregisterSshGitProvider('ssh-1') + } + + expect(stopAndWait).toHaveBeenCalledWith('pty-remote', expect.anything()) + expect(stopAndWait).not.toHaveBeenCalledWith('pty-local-same-id', expect.anything()) + }) + it('rejects SSH-backed runtime removal of the main worktree before provider deletion', async () => { const remoteStore = { ...store, @@ -34495,6 +34555,31 @@ describe('OrcaRuntimeService', () => { expect(kill).not.toHaveBeenCalled() }) + it('stops only the owning connection when one worktree id lives on two hosts', async () => { + const runtime = new OrcaRuntimeService(store) + const kill = vi.fn(() => true) + runtime.setPtyController({ + write: () => true, + kill, + stopAndWait: vi.fn(async () => true), + getForegroundProcess: async () => null + }) + syncSinglePty(runtime, null) + // The store keeps one `repoId::path` per host, so deleting the SSH copy must leave the + // local copy's terminals running — the fence the destructive removal paths now supply. + runtime.registerPty('pty-ssh', TEST_WORKTREE_ID, 'ssh-1') + runtime.registerPty('pty-local', TEST_WORKTREE_ID, null) + + await expect( + runtime.stopTerminalsForWorktree(TEST_WORKTREE_ID, { + resolvedWorktreeId: TEST_WORKTREE_ID, + resolvedConnectionId: 'ssh-1' + }) + ).resolves.toEqual({ stopped: 1 }) + expect(kill).toHaveBeenCalledWith('pty-ssh') + expect(kill).not.toHaveBeenCalledWith('pty-local') + }) + it('awaits physical PTY stop when destructive teardown supplies shared dedupe', async () => { const runtime = new OrcaRuntimeService(store) const physicalStop = makeDeferred() diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 95d3cb997..8cca22329 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -3387,6 +3387,10 @@ export class OrcaRuntimeService { } const teardownResult = await killAllProcessesForWorktree(worktreeId, { runtime: this, + // Why: `repoId::path` ids repeat across hosts, so an unfenced sweep stops a same-id + // workspace's terminals on another connection (mirrors the IPC removal path). + resolvedWorktreeId: worktreeId, + ...(connectionId ? { resolvedConnectionId: connectionId } : {}), localProvider: provider, onPtyStopped: this.onPtyStopped ?? undefined, requirePhysicalStop: true, @@ -23438,14 +23442,34 @@ export class OrcaRuntimeService { 'Cannot delete the project root workspace. Remove the folder project instead.' ) } - const localProvider = this.getLocalProvider() - if (localProvider) { + // Folder projects can be SSH-backed, so resolve the owner before sweeping. + const folderHost = parseExecutionHostId( + store.getWorktreeMeta(removalTarget.id)?.hostId ?? getRepoExecutionHostId(repo) + ) + const folderSshPtyProvider = + folderHost?.kind === 'ssh' ? this.getSshProviderFn?.(folderHost.targetId) : undefined + const externalFolderHost = folderHost?.kind === 'ssh' || folderHost?.kind === 'runtime' + const folderPtyProvider = folderSshPtyProvider ?? this.getLocalProvider() + if (folderPtyProvider) { // Why: folder workspace deletion has no Git removal phase where PTYs // would otherwise be swept; tear them down before hiding the workspace. await killAllProcessesForWorktree(removalTarget.id, { runtime: this, - localProvider, - onPtyStopped: this.onPtyStopped ?? undefined + // External host inventories must never sweep a same-id local workspace. + resolvedWorktreeId: removalTarget.id, + ...(folderHost?.kind === 'ssh' ? { resolvedConnectionId: folderHost.targetId } : {}), + ...(folderHost?.kind === 'runtime' + ? { resolvedRuntimeEnvironmentId: folderHost.environmentId } + : {}), + localProvider: folderPtyProvider, + onPtyStopped: this.onPtyStopped ?? undefined, + ...(externalFolderHost + ? { + includeProviderInventory: + folderHost?.kind === 'ssh' && Boolean(folderSshPtyProvider), + includeLocalRegistry: false + } + : {}) }).catch((err) => { console.warn(`[worktree-teardown] failed for ${removalTarget.id}:`, err) })