From 381e81e7bd20ff98e47f76e408286cd3cb2e47d1 Mon Sep 17 00:00:00 2001 From: Jinjing <6427696+AmethystLiang@users.noreply.github.com> Date: Thu, 23 Jul 2026 19:43:34 -0700 Subject: [PATCH] fix(worktree): don't path-sweep sibling sessions when deleting a folder workspace (#10252) (#10268) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(worktree): don't path-sweep sibling sessions when deleting a folder workspace (#10252) Deleting one folder-workspace instance could kill terminal/agent sessions in OTHER workspaces sharing the same checkout path — sibling instances, and even worktrees of a different repo rooted under that directory. Both pi and Claude Code agent sessions died at once with no recovery. The `cwdOwned` path fallback in killAllProcessesForWorktree() derives its match path via splitWorktreeIdForFilesystem(), which strips the `::workspace:` suffix and collapses a folder instance's path to the shared checkout dir. Every untagged session under that dir then path-matched and got swept (worst case: a home directory registered as a folder repo). Disable the path fallback for folder-workspace instances — their filesystem path can't identify a single instance. The exact `${worktreeId}@@` prefix and authoritative `session.worktreeId` matches (both carrying the instance uuid) still tear down the deleted instance's own sessions; normal git worktrees (unique paths) keep the fallback. The runtime and registry sweeps already matched by exact worktreeId. Adds isFolderWorkspaceInstanceId() and regression tests. See docs/delete-workspace-cwd-owned-sibling-kill.md. * rm design doc --- src/main/runtime/worktree-teardown.test.ts | 96 ++++++++++++++++++++++ src/main/runtime/worktree-teardown.ts | 19 +++-- 2 files changed, 109 insertions(+), 6 deletions(-) diff --git a/src/main/runtime/worktree-teardown.test.ts b/src/main/runtime/worktree-teardown.test.ts index 606b62d06..55f33f9d0 100644 --- a/src/main/runtime/worktree-teardown.test.ts +++ b/src/main/runtime/worktree-teardown.test.ts @@ -154,6 +154,102 @@ describe('killAllProcessesForWorktree', () => { expect(result.providerStopped).toBe(0) }) + it('does not path-sweep untagged siblings when deleting a folder-workspace instance', async () => { + // Regression for #10252: folder-workspace instances share one checkout dir, + // so splitWorktreeIdForFilesystem() strips the `::workspace:` suffix + // down to the shared path. An untagged session under that shared path must + // NOT be swept — it may belong to a sibling workspace or another repo. + const deletedInstance = + 'repo-1::/Users/dev/project::workspace:11111111-1111-1111-1111-111111111111' + const siblingInstance = + 'repo-1::/Users/dev/project::workspace:22222222-2222-2222-2222-222222222222' + const localProvider = createProviderStub(async () => [ + // Untagged session whose cwd is the shared checkout dir (sibling's live agent). + { id: 'floating-sibling', cwd: '/Users/dev/project', title: 'shell' }, + // Properly tagged session owned by a sibling instance. + { + id: `${siblingInstance}@@sib00000`, + cwd: '/Users/dev/project', + title: 'shell', + worktreeId: siblingInstance + } + ]) + listRegisteredPtysMock.mockReturnValue([]) + + const result = await killAllProcessesForWorktree(deletedInstance, { + localProvider, + requirePhysicalStop: true + }) + + expect(localProvider.shutdown).not.toHaveBeenCalled() + expect(result.providerStopped).toBe(0) + }) + + it('still tears down the deleted folder-workspace instance own sessions', async () => { + // The fix disables only the shared-path fallback; exact prefix and + // authoritative worktreeId matches for THIS instance must still fire. + const deletedInstance = + 'repo-1::/Users/dev/project::workspace:11111111-1111-1111-1111-111111111111' + const localProvider = createProviderStub(async () => [ + { id: `${deletedInstance}@@own00001`, cwd: '/Users/dev/project', title: 'shell' }, + { + id: 'tagged-own', + cwd: '/Users/dev/project', + title: 'shell', + worktreeId: deletedInstance + } + ]) + listRegisteredPtysMock.mockReturnValue([]) + + const result = await killAllProcessesForWorktree(deletedInstance, { + localProvider, + requirePhysicalStop: true + }) + + expect(localProvider.shutdown).toHaveBeenCalledWith( + `${deletedInstance}@@own00001`, + expect.objectContaining({ immediate: true }) + ) + expect(localProvider.shutdown).toHaveBeenCalledWith( + 'tagged-own', + expect.objectContaining({ immediate: true }) + ) + expect(result.providerStopped).toBe(2) + }) + + it('kills only the deleted instance own sessions when siblings share the list', async () => { + // One provider list spanning all four quadrants: the deleted instance's own + // prefix + tagged sessions must die; the untagged and tagged sibling sessions + // on the shared checkout path must survive. + const deletedInstance = + 'repo-1::/Users/dev/project::workspace:11111111-1111-1111-1111-111111111111' + const siblingInstance = + 'repo-1::/Users/dev/project::workspace:22222222-2222-2222-2222-222222222222' + const localProvider = createProviderStub(async () => [ + { id: `${deletedInstance}@@own00001`, cwd: '/Users/dev/project', title: 'shell' }, + { id: 'own-tagged', cwd: '/Users/dev/project', title: 'shell', worktreeId: deletedInstance }, + { id: 'floating-sibling', cwd: '/Users/dev/project', title: 'shell' }, + { + id: `${siblingInstance}@@sib00000`, + cwd: '/Users/dev/project', + title: 'shell', + worktreeId: siblingInstance + } + ]) + listRegisteredPtysMock.mockReturnValue([]) + + const result = await killAllProcessesForWorktree(deletedInstance, { + localProvider, + requirePhysicalStop: true + }) + + const killed = (localProvider.shutdown as unknown as ReturnType).mock.calls + .map((call) => call[0] as string) + .sort() + expect(killed).toEqual([`${deletedInstance}@@own00001`, 'own-tagged'].sort()) + expect(result.providerStopped).toBe(2) + }) + it('uses authoritative remote worktree ownership without sweeping the local registry', async () => { const remoteProvider = createProviderStub(async () => [ { id: 'pty-remote', cwd: '/remote/w1', title: 'shell', worktreeId: 'w1' }, diff --git a/src/main/runtime/worktree-teardown.ts b/src/main/runtime/worktree-teardown.ts index db6c0f5d6..7f25d6793 100644 --- a/src/main/runtime/worktree-teardown.ts +++ b/src/main/runtime/worktree-teardown.ts @@ -2,7 +2,7 @@ import type { IPtyProvider } from '../providers/types' import type { OrcaRuntimeService } from './orca-runtime' import { listRegisteredPtys } from '../memory/pty-registry' import { isPathInsideOrEqual } from '../../shared/cross-platform-path' -import { splitWorktreeIdForFilesystem } from '../../shared/worktree-id' +import { splitWorktreeId, splitWorktreeIdForFilesystem } from '../../shared/worktree-id' import { mapWithConcurrency } from '../../shared/map-with-concurrency' // Why: normal inventories still coalesce into one process scan, while a stale @@ -69,7 +69,6 @@ export async function killAllProcessesForWorktree( } const deadline = Date.now() + Math.max(1, deps.timeoutMs ?? WORKTREE_PROCESS_SWEEP_TIMEOUT_MS) const deadlineError = new Error(`Timed out waiting for physical PTY teardown: ${worktreeId}`) - const worktreePath = splitWorktreeIdForFilesystem(worktreeId)?.worktreePath const stopAttempts = new Map>() const stopPty = ( ptyId: string, @@ -113,7 +112,6 @@ export async function killAllProcessesForWorktree( () => sweepProviderByPrefix( worktreeId, - worktreePath, deps.localProvider, deadline, stopPty, @@ -229,7 +227,6 @@ async function settleBeforeDeadline( async function sweepProviderByPrefix( worktreeId: string, - worktreePath: string | undefined, provider: IPtyProvider, deadline: number, stopPty: ( @@ -240,6 +237,16 @@ async function sweepProviderByPrefix( failClosed = false ): Promise { const prefix = `${worktreeId}@@` + // Why (#10252): the cwd fallback only proves ownership when the filesystem path + // is the *whole* worktree path. A folder-workspace instance strips its + // `::workspace:` suffix to a checkout dir shared with sibling instances, + // so leave the fallback unset whenever stripping shortened the path — else + // deleting one instance would sweep the others. + const fullWorktreePath = splitWorktreeId(worktreeId)?.worktreePath + const cwdFallbackPath = + splitWorktreeIdForFilesystem(worktreeId)?.worktreePath === fullWorktreePath + ? fullWorktreePath + : undefined const rpcDeadline = teardownRpcDeadline(deadline) const sessions = failClosed ? await provider.listProcesses({ deadlineMs: rpcDeadline }) @@ -248,11 +255,11 @@ async function sweepProviderByPrefix( // Why: older daemon/relay process rows may omit cwd; their established ID // and authoritative worktree ownership must remain usable during teardown. const cwdOwned = - worktreePath !== undefined && + cwdFallbackPath !== undefined && session.worktreeId === undefined && typeof session.cwd === 'string' && session.cwd.length > 0 && - isPathInsideOrEqual(worktreePath, session.cwd) + isPathInsideOrEqual(cwdFallbackPath, session.cwd) return session.id.startsWith(prefix) || session.worktreeId === worktreeId || cwdOwned }) // Why: agent shutdown snapshots coalesce only when requests begin together;