mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 08:03:37 +00:00
fix(worktrees): stop host-scoped removal leaking VMs and stale-row deletes
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.
This commit is contained in:
@@ -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()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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 }
|
||||
}
|
||||
}),
|
||||
|
||||
+15
-2
@@ -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<typeof getActiveRuntimeTarget>
|
||||
worktreeBeforeRemoval: PreservedBranchWorktree
|
||||
suppressPreservedBranchToast: boolean
|
||||
}): RemoveWorktreeSliceResult {
|
||||
}): Promise<Awaited<RemoveWorktreeSliceResult>> {
|
||||
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) {
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user