From abf9686bd7ae757b1a6e8e55a2ae73897ddde3dd Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Sun, 16 Aug 2026 20:33:13 -0700 Subject: [PATCH] fix(worktrees): stop host-scoped removal leaking VMs and stale-row deletes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fail-open seams the readiness review found. Ephemeral VM teardown was skipped on the host-scoped path. That early return is justified by SHARED renderer state (tabs, terminals, browsers) belonging to the surviving host — but an ephemeral VM is not shared, it belongs to the workspace just deleted. The likeliest real collision is one machine reachable as both `runtime:env` and `ssh:target`, so removing the runtime row left the VM running and billing. Teardown moves into completeSameIdHostScopedRemoval, which is where that path's cleanup belongs; the full path keeps its own call AFTER terminal teardown so a still-mounted pane cannot hit a gone relay. The host-scoped path tears down no terminals, so it has no such race. worktree.rm pre-resolved the host via showManagedWorktree, which THROWS once Git no longer lists the path. Deletes legitimately arrive in that state and removeManagedWorktree already handles it, so this made a stale workspace undeletable by any client that sends no hostId — mobile could delete it before. Only 'selector_not_found' falls through now; 'selector_ambiguous' still fails closed, because two owners plus an unqualified client is precisely the case that must refuse. Both directions are pinned by tests. --- .../worktree-rm-host-qualification.test.ts | 37 +++++++++++++++++++ src/main/runtime/rpc/methods/worktree.ts | 25 ++++++++++--- .../host-qualified-worktree-removal.ts | 17 ++++++++- .../worktrees/teardown/remove-worktree.ts | 2 + 4 files changed, 74 insertions(+), 7 deletions(-) diff --git a/src/main/runtime/rpc/methods/worktree-rm-host-qualification.test.ts b/src/main/runtime/rpc/methods/worktree-rm-host-qualification.test.ts index c706c3a05be..25ecd00a471 100644 --- a/src/main/runtime/rpc/methods/worktree-rm-host-qualification.test.ts +++ b/src/main/runtime/rpc/methods/worktree-rm-host-qualification.test.ts @@ -82,4 +82,41 @@ describe('worktree.rm host qualification', () => { }) expect(runtime.removeManagedWorktree).not.toHaveBeenCalled() }) + // Why: a delete legitimately arrives after Git stops listing the path (the + // caller is clearing a stale row). Pre-resolving the host must not turn that + // into a hard failure for a client that sends no hostId — mobile could delete + // such a row before host qualification and must still be able to. + it('still deletes a stale workspace for an old client once Git no longer lists it', async () => { + const runtime = makeRuntime() + vi.mocked(runtime.showManagedWorktree).mockRejectedValue(new Error('selector_not_found')) + const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS }) + + const response = await dispatcher.dispatch( + makeRequest({ worktree: 'id:wt-gone', force: true, runHooks: false }) + ) + + expect(response).toMatchObject({ ok: true, result: { removed: true } }) + // Unqualified on purpose: removeManagedWorktree owns the stale-row path and + // still refuses on its own if the id turns out to have two owners. + expect(runtime.removeManagedWorktree).toHaveBeenCalledWith( + 'id:wt-gone', + true, + false, + false, + undefined + ) + }) + + it('propagates a non-missing lookup failure instead of deleting unqualified', async () => { + const runtime = makeRuntime() + vi.mocked(runtime.showManagedWorktree).mockRejectedValue(new Error('selector_ambiguous')) + const dispatcher = new RpcDispatcher({ runtime, methods: WORKTREE_METHODS }) + + const response = await dispatcher.dispatch( + makeRequest({ worktree: 'id:wt-1', force: true, runHooks: false }) + ) + + expect(response).toMatchObject({ ok: false, error: { message: 'selector_ambiguous' } }) + expect(runtime.removeManagedWorktree).not.toHaveBeenCalled() + }) }) diff --git a/src/main/runtime/rpc/methods/worktree.ts b/src/main/runtime/rpc/methods/worktree.ts index 5630868365b..2dd7ef3f46f 100644 --- a/src/main/runtime/rpc/methods/worktree.ts +++ b/src/main/runtime/rpc/methods/worktree.ts @@ -201,10 +201,25 @@ export const WORKTREE_METHODS: RpcMethod[] = [ name: 'worktree.rm', params: WorktreeRemove, handler: async (params, { runtime }) => { - // Older mobile clients omit hostId, so resolve through the ambiguity gate before pinning removal. - const hostId = params.hostId ?? (await runtime.showManagedWorktree(params.worktree)).hostId - if (!hostId) { - throw new Error('worktree.rm could not resolve the workspace host') + // Older mobile clients omit hostId, so resolve through the ambiguity gate + // before pinning removal. An ambiguous selector still fails closed: two + // hosts own the id and an unqualified client cannot say which it meant. + let resolvedHostId = params.hostId + if (!resolvedHostId) { + try { + resolvedHostId = (await runtime.showManagedWorktree(params.worktree)).hostId + if (!resolvedHostId) { + throw new Error('worktree.rm could not resolve the workspace host') + } + } catch (error) { + // 'selector_not_found' is not a failure to attribute — Git simply no + // longer lists the path. A delete legitimately arrives in that state and + // `removeManagedWorktree` handles it, so a stale workspace stays + // deletable by a client that sends no host. Anything else propagates. + if (!(error instanceof Error) || error.message !== 'selector_not_found') { + throw error + } + } } const removalArgs = [ params.worktree, @@ -212,7 +227,7 @@ export const WORKTREE_METHODS: RpcMethod[] = [ params.runHooks === true, params.allowUnverifiedPtyStop === true ] as const - const result = await runtime.removeManagedWorktree(...removalArgs, hostId) + const result = await runtime.removeManagedWorktree(...removalArgs, resolvedHostId) return { removed: true, ...result } } }), diff --git a/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts b/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts index 544bd8beec3..ad46ef61455 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/host-qualified-worktree-removal.ts @@ -23,6 +23,8 @@ import { WORKTREE_REMOVAL_HOST_CHANGED_ERROR } from '../listing/worktree-slice-constants' import { requestVirtualizedScrollAnchorRecord } from '@/hooks/requestVirtualizedScrollAnchorRecord' +import { cleanupEphemeralVmRuntimesForDeleted } from '@/lib/ephemeral-vm-runtime-cleanup' +import { purgeOrphanedRuntimeSshProjects } from './orphaned-runtime-ssh-project-purge' import { showPreservedBranchToast } from '@/components/sidebar/preserved-branch-toast' import { preservedBranchCleanupKey } from '../../../../../../shared/preserved-branch-cleanup' @@ -199,8 +201,15 @@ function dropConfirmedHostRow( /** * Finish a removal whose id still exists on another host: prune just the * confirmed host's row and keep the preserved-branch follow-up pinned to it. + * + * Tears down the ephemeral VM even though the shared renderer state survives. A + * VM is NOT shared: it belongs to the workspace just deleted. The likeliest real + * collision is one machine reachable as both `runtime:env` and `ssh:target`, so + * skipping it left the VM running and billing after its row was removed. Safe to + * do here without the full path's ordering care, because this path tears down no + * terminals — there is no still-mounted pane to race a disposed relay. */ -export function completeSameIdHostScopedRemoval(args: { +export async function completeSameIdHostScopedRemoval(args: { set: WorktreeSliceSet get: WorktreeSliceGet worktreeId: string @@ -210,7 +219,7 @@ export function completeSameIdHostScopedRemoval(args: { target: ReturnType worktreeBeforeRemoval: PreservedBranchWorktree suppressPreservedBranchToast: boolean -}): RemoveWorktreeSliceResult { +}): Promise> { const { set, get, @@ -222,6 +231,10 @@ export function completeSameIdHostScopedRemoval(args: { worktreeBeforeRemoval, suppressPreservedBranchToast } = args + const runtimeCleanup = await cleanupEphemeralVmRuntimesForDeleted({ + workspaceIds: [worktreeId] + }) + await purgeOrphanedRuntimeSshProjects(get, runtimeCleanup.destroyedSshTargetIds) dropConfirmedHostRow(set, worktreeId, requiredExecutionHostId) const preservedBranch = removalResult?.preservedBranch if (!preservedBranch) { 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 fae6ddf6472..cb921befdab 100644 --- a/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts +++ b/src/renderer/src/store/slices/worktrees/teardown/remove-worktree.ts @@ -193,6 +193,8 @@ export function createRemoveWorktree( // Why (STA-4343): another host still owns this id, so the shared renderer // state (tabs, terminals, browsers) belongs to that live workspace — drop // only the confirmed host's row instead of tearing all of it down. + // + // The ephemeral VM is the exception — see completeSameIdHostScopedRemoval. if (sameIdSurvivesOnAnotherHost && requiredExecutionHostId) { return completeSameIdHostScopedRemoval({ set,