* 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:<uuid>` 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
This commit is contained in:
parent
da19a9beda
commit
381e81e7bd
|
|
@ -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:<uuid>` 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<typeof vi.fn>).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' },
|
||||
|
|
|
|||
|
|
@ -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<string, Promise<boolean>>()
|
||||
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<T>(
|
|||
|
||||
async function sweepProviderByPrefix(
|
||||
worktreeId: string,
|
||||
worktreePath: string | undefined,
|
||||
provider: IPtyProvider,
|
||||
deadline: number,
|
||||
stopPty: (
|
||||
|
|
@ -240,6 +237,16 @@ async function sweepProviderByPrefix(
|
|||
failClosed = false
|
||||
): Promise<number> {
|
||||
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:<uuid>` 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;
|
||||
|
|
|
|||
Loading…
Reference in New Issue