mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 08:03:12 +00:00
Harden worktree creation cancellation rollback
Review follow-ups on the pending-creation cancellation path: - Refuse a replaced-instance rollback inside beginHostQualifiedRemoval, next to the sibling fail-closed refusals and before the row is marked deleting, so the refusal cannot pin a permanent error on a live replacement's delete state. An absent row no longer counts as proof of replacement. - Treat a replaced instance as nothing left to roll back rather than a cleanup failure, so it stops raising a never-dismissing toast. - Fail closed when a deferred rollback cannot identify its workspace: a host too old to stamp instanceId leaves no proof, so leave the workspace in place instead of force-deleting a possible replacement. - Say that a retained runtime is still running and that deleting the workspace releases it. - Warn instead of reporting a clean rollback when a dispatched create failed without proving the host did not create the workspace. - Keep the attempt map private behind register/release/reset so the identity-checked release has one implementation. - Share the creation-flow store stand-in from the fixtures module, keeping both touched files under their max-lines caps.
This commit is contained in:
@@ -1,18 +1,52 @@
|
||||
import type { Worktree } from '../../../shared/worktree/types'
|
||||
|
||||
/** Refused before the create reached the host, so nothing can have been created. */
|
||||
export class WorktreeCreationCancelledError extends Error {}
|
||||
|
||||
export type WorktreeCreationAttempt = {
|
||||
completed: boolean
|
||||
cancelled: boolean
|
||||
cleanupAfterSettlement?: () => Promise<boolean>
|
||||
cleanupRuntime?: () => Promise<void>
|
||||
worktree?: Worktree
|
||||
/** The create call failed without proving the host did not create the workspace. */
|
||||
createOutcomeUnknown?: boolean
|
||||
isCancelled: () => boolean
|
||||
}
|
||||
|
||||
export const activeWorktreeCreationAttempts = new Map<string, WorktreeCreationAttempt>()
|
||||
// Why private: these hold cleanup closures, which a zustand slice cannot serialise,
|
||||
// so attempt bookkeeping lives here while `pendingWorktreeCreations` stays the store's.
|
||||
// Every mutation goes through the functions below so the identity-checked release
|
||||
// (a newer attempt must not be evicted by an older one) has exactly one implementation.
|
||||
const activeAttempts = new Map<string, WorktreeCreationAttempt>()
|
||||
|
||||
export function getActiveWorktreeCreation(creationId: string): WorktreeCreationAttempt | undefined {
|
||||
return activeAttempts.get(creationId)
|
||||
}
|
||||
|
||||
export function registerActiveWorktreeCreation(
|
||||
creationId: string,
|
||||
attempt: WorktreeCreationAttempt
|
||||
): void {
|
||||
activeAttempts.set(creationId, attempt)
|
||||
}
|
||||
|
||||
/** Drop `attempt` only while it is still the registered one. */
|
||||
export function releaseActiveWorktreeCreation(
|
||||
creationId: string,
|
||||
attempt: WorktreeCreationAttempt
|
||||
): void {
|
||||
if (activeAttempts.get(creationId) === attempt) {
|
||||
activeAttempts.delete(creationId)
|
||||
}
|
||||
}
|
||||
|
||||
export function resetActiveWorktreeCreations(): void {
|
||||
activeAttempts.clear()
|
||||
}
|
||||
|
||||
export function cancelActiveWorktreeCreation(creationId: string): boolean {
|
||||
const attempt = activeWorktreeCreationAttempts.get(creationId)
|
||||
const attempt = activeAttempts.get(creationId)
|
||||
if (!attempt || attempt.completed) {
|
||||
return false
|
||||
}
|
||||
@@ -24,9 +58,9 @@ export function cancelActiveWorktreeCreation(creationId: string): boolean {
|
||||
}
|
||||
|
||||
export function completeActiveWorktreeCreation(creationId: string): void {
|
||||
const attempt = activeWorktreeCreationAttempts.get(creationId)
|
||||
const attempt = activeAttempts.get(creationId)
|
||||
if (attempt) {
|
||||
attempt.completed = true
|
||||
}
|
||||
activeWorktreeCreationAttempts.delete(creationId)
|
||||
activeAttempts.delete(creationId)
|
||||
}
|
||||
|
||||
@@ -3,13 +3,14 @@ import { makeWorktree } from '@/store/slices/worktrees-slice-test-fixtures'
|
||||
import { toast } from 'sonner'
|
||||
import { cancelActiveWorktreeCreation } from './worktree-creation-attempt'
|
||||
import { withWorktreeCreationCancellation } from './worktree-creation-cancellation'
|
||||
import { WORKTREE_INSTANCE_REPLACED_ERROR } from '@/store/slices/worktree-removal-options'
|
||||
|
||||
const state = vi.hoisted(() => ({
|
||||
pendingWorktreeCreations: {} as Record<string, unknown>,
|
||||
removeWorktree: vi.fn()
|
||||
}))
|
||||
vi.mock('@/store', () => ({ useAppStore: { getState: () => state } }))
|
||||
vi.mock('sonner', () => ({ toast: { error: vi.fn() } }))
|
||||
vi.mock('sonner', () => ({ toast: { error: vi.fn(), warning: vi.fn() } }))
|
||||
|
||||
function deferred() {
|
||||
let resolve!: () => void
|
||||
@@ -18,7 +19,12 @@ function deferred() {
|
||||
})
|
||||
return { promise, resolve }
|
||||
}
|
||||
const worktree = makeWorktree({ id: 'repo::/workspace', repoId: 'repo', hostId: 'ssh:owner' })
|
||||
const worktree = makeWorktree({
|
||||
id: 'repo::/workspace',
|
||||
repoId: 'repo',
|
||||
hostId: 'ssh:owner',
|
||||
instanceId: 'instance-1'
|
||||
})
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks()
|
||||
state.pendingWorktreeCreations = { creation: {}, other: {} }
|
||||
@@ -65,7 +71,11 @@ describe('worktree creation cancellation', () => {
|
||||
expect(state.removeWorktree).toHaveBeenCalledExactlyOnceWith(
|
||||
{ id: worktree.id, executionHostId: 'ssh:owner' },
|
||||
true,
|
||||
{ skipArchiveHooks: true, suppressPreservedBranchToast: true }
|
||||
{
|
||||
skipArchiveHooks: true,
|
||||
suppressPreservedBranchToast: true,
|
||||
expectedInstanceId: 'instance-1'
|
||||
}
|
||||
)
|
||||
expect(cleanup).toHaveBeenCalledOnce()
|
||||
expect(state.pendingWorktreeCreations.other).toBeDefined()
|
||||
@@ -171,6 +181,95 @@ describe('worktree creation cancellation', () => {
|
||||
expect.stringContaining('Host unavailable'),
|
||||
expect.objectContaining({ duration: Infinity })
|
||||
)
|
||||
// The retained runtime is invisible otherwise, so the toast must say how to release it.
|
||||
expect(toast.error).toHaveBeenCalledWith(
|
||||
expect.stringContaining('deleting the workspace releases it'),
|
||||
expect.anything()
|
||||
)
|
||||
expect(cancelActiveWorktreeCreation('creation')).toBe(false)
|
||||
})
|
||||
|
||||
it('omits the runtime hint when the cancelled creation had no runtime', async () => {
|
||||
state.removeWorktree.mockResolvedValue({ ok: false, error: 'Host unavailable' })
|
||||
await withWorktreeCreationCancellation('creation', async (attempt) => {
|
||||
attempt.worktree = worktree
|
||||
cancelActiveWorktreeCreation('creation')
|
||||
})
|
||||
expect(toast.error).toHaveBeenCalledWith(
|
||||
expect.not.stringContaining('runtime is still running'),
|
||||
expect.anything()
|
||||
)
|
||||
})
|
||||
|
||||
it('warns instead of claiming a clean rollback when the host never confirmed', async () => {
|
||||
await withWorktreeCreationCancellation('creation', async (attempt) => {
|
||||
attempt.createOutcomeUnknown = true
|
||||
cancelActiveWorktreeCreation('creation')
|
||||
})
|
||||
expect(state.removeWorktree).not.toHaveBeenCalled()
|
||||
expect(toast.warning).toHaveBeenCalledWith(expect.stringContaining('delete it manually'))
|
||||
expect(toast.error).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('stays silent when cancellation is known to have preceded the create', async () => {
|
||||
await withWorktreeCreationCancellation('creation', async () => {
|
||||
cancelActiveWorktreeCreation('creation')
|
||||
})
|
||||
expect(toast.warning).not.toHaveBeenCalled()
|
||||
expect(toast.error).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('treats a replaced instance as nothing left to roll back, not a cleanup failure', async () => {
|
||||
const cleanup = vi.fn()
|
||||
state.removeWorktree.mockResolvedValue({
|
||||
ok: false,
|
||||
error: WORKTREE_INSTANCE_REPLACED_ERROR
|
||||
})
|
||||
await withWorktreeCreationCancellation('creation', async (attempt) => {
|
||||
attempt.worktree = worktree
|
||||
attempt.cleanupRuntime = cleanup
|
||||
cancelActiveWorktreeCreation('creation')
|
||||
})
|
||||
expect(cleanup).toHaveBeenCalledOnce()
|
||||
expect(toast.error).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('refuses a deferred rollback the host left unidentifiable rather than force-deleting', async () => {
|
||||
const unstamped = makeWorktree({ id: 'repo::/workspace', repoId: 'repo', hostId: 'ssh:owner' })
|
||||
await withWorktreeCreationCancellation('creation', async (attempt) => {
|
||||
attempt.worktree = unstamped
|
||||
})
|
||||
expect(cancelActiveWorktreeCreation('creation')).toBe(true)
|
||||
await vi.waitFor(() =>
|
||||
expect(toast.error).toHaveBeenCalledWith(
|
||||
expect.stringContaining('Delete it manually'),
|
||||
expect.objectContaining({ duration: Infinity })
|
||||
)
|
||||
)
|
||||
expect(state.removeWorktree).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('still rolls back an unidentifiable workspace when cancelled in flight', async () => {
|
||||
const unstamped = makeWorktree({ id: 'repo::/workspace', repoId: 'repo', hostId: 'ssh:owner' })
|
||||
await withWorktreeCreationCancellation('creation', async (attempt) => {
|
||||
attempt.worktree = unstamped
|
||||
cancelActiveWorktreeCreation('creation')
|
||||
})
|
||||
// In-flight cancellation cannot race a replacement, so it must not fail closed.
|
||||
expect(state.removeWorktree).toHaveBeenCalledOnce()
|
||||
expect(toast.error).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('lets retry proceed after the rollback found its workspace already replaced', async () => {
|
||||
await withWorktreeCreationCancellation('creation', async (attempt) => {
|
||||
attempt.worktree = worktree
|
||||
})
|
||||
state.removeWorktree.mockResolvedValueOnce({
|
||||
ok: false,
|
||||
error: WORKTREE_INSTANCE_REPLACED_ERROR
|
||||
})
|
||||
const retry = vi.fn()
|
||||
await withWorktreeCreationCancellation('creation', retry)
|
||||
expect(retry).toHaveBeenCalledOnce()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -2,15 +2,18 @@ import { toast } from 'sonner'
|
||||
import { useAppStore } from '@/store'
|
||||
|
||||
import {
|
||||
activeWorktreeCreationAttempts as activeAttempts,
|
||||
getActiveWorktreeCreation,
|
||||
registerActiveWorktreeCreation,
|
||||
releaseActiveWorktreeCreation,
|
||||
type WorktreeCreationAttempt
|
||||
} from './worktree-creation-attempt'
|
||||
import { WORKTREE_INSTANCE_REPLACED_ERROR } from '@/store/slices/worktree-removal-options'
|
||||
|
||||
export async function withWorktreeCreationCancellation(
|
||||
creationId: string,
|
||||
execute: (attempt: WorktreeCreationAttempt) => Promise<void>
|
||||
): Promise<void> {
|
||||
const previous = activeAttempts.get(creationId)
|
||||
const previous = getActiveWorktreeCreation(creationId)
|
||||
if (previous && !previous.completed && !previous.cleanupAfterSettlement) {
|
||||
return
|
||||
}
|
||||
@@ -29,33 +32,45 @@ export async function withWorktreeCreationCancellation(
|
||||
isCancelled: () =>
|
||||
attempt.cancelled || !useAppStore.getState().pendingWorktreeCreations[creationId]
|
||||
}
|
||||
activeAttempts.set(creationId, attempt)
|
||||
registerActiveWorktreeCreation(creationId, attempt)
|
||||
try {
|
||||
if (!attempt.isCancelled()) {
|
||||
await execute(attempt)
|
||||
}
|
||||
} finally {
|
||||
const cleanup = (): Promise<boolean> =>
|
||||
removeCancelledCreation(attempt).finally(() => {
|
||||
if (activeAttempts.get(creationId) === attempt) {
|
||||
activeAttempts.delete(creationId)
|
||||
}
|
||||
})
|
||||
const cleanup = (deferred: boolean): Promise<boolean> =>
|
||||
removeCancelledCreation(attempt, deferred).finally(() =>
|
||||
releaseActiveWorktreeCreation(creationId, attempt)
|
||||
)
|
||||
if (!attempt.completed && attempt.isCancelled()) {
|
||||
await cleanup()
|
||||
await cleanup(false)
|
||||
} else if (!attempt.completed && attempt.worktree) {
|
||||
// Failed post-create startup still owns a workspace when its error panel is dismissed.
|
||||
attempt.cleanupAfterSettlement = cleanup
|
||||
} else if (activeAttempts.get(creationId) === attempt) {
|
||||
activeAttempts.delete(creationId)
|
||||
attempt.cleanupAfterSettlement = () => cleanup(true)
|
||||
} else {
|
||||
releaseActiveWorktreeCreation(creationId, attempt)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async function removeCancelledCreation(attempt: WorktreeCreationAttempt): Promise<boolean> {
|
||||
/**
|
||||
* `deferred` marks a rollback that outlived its attempt, waiting behind an error
|
||||
* panel. Only that one can sit long enough for the user to delete and recreate at
|
||||
* the same path, so only it refuses to force-delete a workspace it cannot prove is
|
||||
* the one it created — a host too old to stamp `instanceId` leaves no other proof.
|
||||
*/
|
||||
async function removeCancelledCreation(
|
||||
attempt: WorktreeCreationAttempt,
|
||||
deferred: boolean
|
||||
): Promise<boolean> {
|
||||
const { worktree } = attempt
|
||||
try {
|
||||
if (worktree) {
|
||||
if (deferred && !worktree.instanceId) {
|
||||
throw new Error(
|
||||
'it could not be identified on the host, so it was left in place. Delete it manually if unwanted.'
|
||||
)
|
||||
}
|
||||
const result = await useAppStore
|
||||
.getState()
|
||||
.removeWorktree({ id: worktree.id, executionHostId: worktree.hostId ?? 'local' }, true, {
|
||||
@@ -63,16 +78,31 @@ async function removeCancelledCreation(attempt: WorktreeCreationAttempt): Promis
|
||||
suppressPreservedBranchToast: true,
|
||||
...(worktree.instanceId ? { expectedInstanceId: worktree.instanceId } : {})
|
||||
})
|
||||
if (!result.ok) {
|
||||
// A replaced instance means this attempt's workspace is already gone, so
|
||||
// there is nothing left to roll back — not a cleanup failure.
|
||||
if (!result.ok && result.error !== WORKTREE_INSTANCE_REPLACED_ERROR) {
|
||||
throw new Error(result.error)
|
||||
}
|
||||
}
|
||||
// Why: cancelling before the host answered leaves no row to remove, but the
|
||||
// host may still have finished. Say so rather than reporting a clean rollback.
|
||||
if (!worktree && attempt.createOutcomeUnknown) {
|
||||
toast.warning(
|
||||
'Cancelled before the host confirmed the workspace. If it appears, delete it manually.'
|
||||
)
|
||||
}
|
||||
await attempt.cleanupRuntime?.()
|
||||
return true
|
||||
} catch (error) {
|
||||
const message = error instanceof Error ? error.message : String(error)
|
||||
console.error('worktree create: cancellation cleanup failed', worktree?.id, error)
|
||||
toast.error(`Could not remove the cancelled workspace: ${message}`, {
|
||||
// Why: the runtime is deliberately left running — tearing down a VM whose
|
||||
// workspace deletion never confirmed could destroy a workspace that survived.
|
||||
// Deleting the still-visible row is what releases both, so say so.
|
||||
const runtimeHint = attempt.cleanupRuntime
|
||||
? ' Its runtime is still running; deleting the workspace releases it.'
|
||||
: ''
|
||||
toast.error(`Could not remove the cancelled workspace: ${message}${runtimeHint}`, {
|
||||
duration: Infinity
|
||||
})
|
||||
return false
|
||||
|
||||
@@ -1,7 +1,10 @@
|
||||
import { dispatchWorktreeCreation } from './worktree-creation-dispatch'
|
||||
import { toast } from 'sonner'
|
||||
import { withWorktreeCreationCancellation } from './worktree-creation-cancellation'
|
||||
import type { WorktreeCreationAttempt } from './worktree-creation-attempt'
|
||||
import {
|
||||
WorktreeCreationCancelledError,
|
||||
type WorktreeCreationAttempt
|
||||
} from './worktree-creation-attempt'
|
||||
import { useAppStore } from '@/store'
|
||||
import { preflightAgentTrust } from '@/lib/agent-trust-preflight'
|
||||
import { activateAndRevealWorktree, type ActivateAndRevealResult } from '@/lib/worktree-activation'
|
||||
@@ -70,6 +73,9 @@ async function executeWorktreeCreationAttempt(
|
||||
// Why: a missing entry means the user cancelled mid-flight — abandon
|
||||
// silently rather than surfacing an error for work they already dismissed.
|
||||
if (!useAppStore.getState().pendingWorktreeCreations[creationId]) {
|
||||
// A dispatched call that failed proves nothing about the host: it may have
|
||||
// finished and lost the response, so cleanup must not claim the path is clear.
|
||||
attempt.createOutcomeUnknown = !(error instanceof WorktreeCreationCancelledError)
|
||||
return
|
||||
}
|
||||
if (preparedRequest.ephemeralVmRuntimeId) {
|
||||
|
||||
@@ -1,52 +1,19 @@
|
||||
import { activeWorktreeCreationAttempts } from './worktree-creation-attempt'
|
||||
import { makeRequest, makePendingCreation } from './worktree-creation-test-fixtures'
|
||||
import {
|
||||
resetActiveWorktreeCreations,
|
||||
WorktreeCreationCancelledError
|
||||
} from './worktree-creation-attempt'
|
||||
import {
|
||||
makeRequest,
|
||||
makePendingCreation,
|
||||
makeCreationFlowStore
|
||||
} from './worktree-creation-test-fixtures'
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { PendingWorktreeCreation } from '@/lib/pending-worktree-creation'
|
||||
|
||||
const { prepareEphemeralVmWorkspaceTargetMock } = vi.hoisted(() => ({
|
||||
prepareEphemeralVmWorkspaceTargetMock: vi.fn()
|
||||
}))
|
||||
|
||||
type TestActiveView = 'terminal' | 'tasks'
|
||||
|
||||
const store = {
|
||||
settings: {
|
||||
activeRuntimeEnvironmentId: null as string | null,
|
||||
experimentalNativeChat: undefined as boolean | undefined,
|
||||
openAgentTabsInChatByDefault: undefined as boolean | undefined
|
||||
},
|
||||
activeView: 'terminal' as TestActiveView,
|
||||
activePendingCreationId: 'creation-1' as string | null,
|
||||
repos: [{ id: 'repo-runtime', connectionId: null }],
|
||||
pendingWorktreeCreations: {} as Record<string, PendingWorktreeCreation>,
|
||||
beginPendingWorktreeCreation: vi.fn((entry: PendingWorktreeCreation) => {
|
||||
store.pendingWorktreeCreations[entry.creationId] = entry
|
||||
store.activePendingCreationId = entry.creationId
|
||||
}),
|
||||
updatePendingWorktreeCreation: vi.fn(
|
||||
(creationId: string, patch: Partial<PendingWorktreeCreation>) => {
|
||||
const entry = store.pendingWorktreeCreations[creationId]
|
||||
if (entry) {
|
||||
store.pendingWorktreeCreations[creationId] = { ...entry, ...patch }
|
||||
}
|
||||
}
|
||||
),
|
||||
removePendingWorktreeCreation: vi.fn((creationId: string) => {
|
||||
delete store.pendingWorktreeCreations[creationId]
|
||||
}),
|
||||
updateWorktreeMeta: vi.fn(),
|
||||
removeWorktree: vi.fn().mockResolvedValue({ ok: true }),
|
||||
setActivePendingWorktreeCreation: vi.fn(),
|
||||
setActiveView: vi.fn(),
|
||||
setSidebarOpen: vi.fn(),
|
||||
createWorktree: vi.fn(() => new Promise(() => {})),
|
||||
setupProjectExistingFolder: vi.fn(),
|
||||
refreshRuntimeEnvironmentStatus: vi.fn(),
|
||||
seedNativeChatLaunchDraft: vi.fn(),
|
||||
setTabViewMode: vi.fn(),
|
||||
tabsByWorktree: {} as Record<string, { id: string; launchAgent?: string }[]>,
|
||||
unifiedTabsByWorktree: {}
|
||||
}
|
||||
const store = makeCreationFlowStore()
|
||||
|
||||
vi.mock('@/store', () => ({
|
||||
useAppStore: {
|
||||
@@ -76,7 +43,8 @@ vi.mock('@/lib/new-workspace', () => ({
|
||||
|
||||
vi.mock('sonner', () => ({
|
||||
toast: {
|
||||
error: vi.fn()
|
||||
error: vi.fn(),
|
||||
warning: vi.fn()
|
||||
}
|
||||
}))
|
||||
|
||||
@@ -96,7 +64,7 @@ import {
|
||||
} from './worktree-creation-flow'
|
||||
|
||||
beforeEach(() => {
|
||||
activeWorktreeCreationAttempts.clear()
|
||||
resetActiveWorktreeCreations()
|
||||
vi.clearAllMocks()
|
||||
store.settings.activeRuntimeEnvironmentId = null
|
||||
store.settings.experimentalNativeChat = undefined
|
||||
@@ -594,6 +562,33 @@ describe('staged background worktree creation', () => {
|
||||
expect(activateAndRevealWorktree).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// Why: a dispatched create that throws proves nothing (the host may have finished
|
||||
// and lost the response), while a pre-dispatch refusal proves nothing was created.
|
||||
it.each([
|
||||
{ what: 'never reported back', err: () => new Error('socket hang up'), warnings: 1 },
|
||||
{
|
||||
what: 'was refused before dispatch',
|
||||
err: () => new WorktreeCreationCancelledError('Worktree creation cancelled.'),
|
||||
warnings: 0
|
||||
}
|
||||
])('cancelled create whose call $what', async ({ err, warnings }) => {
|
||||
store.repos = [{ id: 'repo-1', connectionId: null }]
|
||||
store.createWorktree.mockImplementationOnce(async () => {
|
||||
delete store.pendingWorktreeCreations['creation-1']
|
||||
throw err()
|
||||
})
|
||||
|
||||
continueBackgroundWorktreeCreation('creation-1', makeRequest(), {
|
||||
revealCreationSurface: false
|
||||
})
|
||||
|
||||
await vi.waitFor(() => expect(store.createWorktree).toHaveBeenCalled())
|
||||
await flushAsyncWorktreeCreation()
|
||||
expect(toast.warning).toHaveBeenCalledTimes(warnings)
|
||||
expect(store.removeWorktree).not.toHaveBeenCalled()
|
||||
expect(toast.error).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
// Why: one-click "Start workspace from issue" commonly backgrounds, so the
|
||||
// user-moved-on path is the common delivery for the repo's issue command; it
|
||||
// must thread through as the 5th positional arg, not be dropped to undefined.
|
||||
|
||||
@@ -1,3 +1,4 @@
|
||||
import { vi, type Mock } from 'vitest'
|
||||
import type { PendingWorktreeCreation, WorktreeCreationRequest } from './pending-worktree-creation'
|
||||
|
||||
export function makeRequest(
|
||||
@@ -28,3 +29,75 @@ export function makePendingCreation(request: WorktreeCreationRequest): PendingWo
|
||||
request
|
||||
}
|
||||
}
|
||||
|
||||
type TestActiveView = 'terminal' | 'tasks'
|
||||
|
||||
export type CreationFlowStore = {
|
||||
settings: {
|
||||
activeRuntimeEnvironmentId: string | null
|
||||
experimentalNativeChat: boolean | undefined
|
||||
openAgentTabsInChatByDefault: boolean | undefined
|
||||
}
|
||||
activeView: TestActiveView
|
||||
activePendingCreationId: string | null
|
||||
repos: { id: string; connectionId: string | null }[]
|
||||
pendingWorktreeCreations: Record<string, PendingWorktreeCreation>
|
||||
beginPendingWorktreeCreation: Mock
|
||||
updatePendingWorktreeCreation: Mock
|
||||
removePendingWorktreeCreation: Mock
|
||||
updateWorktreeMeta: Mock
|
||||
removeWorktree: Mock
|
||||
setActivePendingWorktreeCreation: Mock
|
||||
setActiveView: Mock
|
||||
setSidebarOpen: Mock
|
||||
createWorktree: Mock
|
||||
setupProjectExistingFolder: Mock
|
||||
refreshRuntimeEnvironmentStatus: Mock
|
||||
seedNativeChatLaunchDraft: Mock
|
||||
setTabViewMode: Mock
|
||||
tabsByWorktree: Record<string, { id: string; launchAgent?: string }[]>
|
||||
unifiedTabsByWorktree: Record<string, unknown>
|
||||
}
|
||||
|
||||
/** The `@/store` stand-in shared by the creation-flow suites. */
|
||||
export function makeCreationFlowStore(): CreationFlowStore {
|
||||
const store: CreationFlowStore = {
|
||||
settings: {
|
||||
activeRuntimeEnvironmentId: null,
|
||||
experimentalNativeChat: undefined,
|
||||
openAgentTabsInChatByDefault: undefined
|
||||
},
|
||||
activeView: 'terminal',
|
||||
activePendingCreationId: 'creation-1',
|
||||
repos: [{ id: 'repo-runtime', connectionId: null }],
|
||||
pendingWorktreeCreations: {},
|
||||
beginPendingWorktreeCreation: vi.fn((entry: PendingWorktreeCreation) => {
|
||||
store.pendingWorktreeCreations[entry.creationId] = entry
|
||||
store.activePendingCreationId = entry.creationId
|
||||
}),
|
||||
updatePendingWorktreeCreation: vi.fn(
|
||||
(creationId: string, patch: Partial<PendingWorktreeCreation>) => {
|
||||
const entry = store.pendingWorktreeCreations[creationId]
|
||||
if (entry) {
|
||||
store.pendingWorktreeCreations[creationId] = { ...entry, ...patch }
|
||||
}
|
||||
}
|
||||
),
|
||||
removePendingWorktreeCreation: vi.fn((creationId: string) => {
|
||||
delete store.pendingWorktreeCreations[creationId]
|
||||
}),
|
||||
updateWorktreeMeta: vi.fn(),
|
||||
removeWorktree: vi.fn().mockResolvedValue({ ok: true }),
|
||||
setActivePendingWorktreeCreation: vi.fn(),
|
||||
setActiveView: vi.fn(),
|
||||
setSidebarOpen: vi.fn(),
|
||||
createWorktree: vi.fn(() => new Promise(() => {})),
|
||||
setupProjectExistingFolder: vi.fn(),
|
||||
refreshRuntimeEnvironmentStatus: vi.fn(),
|
||||
seedNativeChatLaunchDraft: vi.fn(),
|
||||
setTabViewMode: vi.fn(),
|
||||
tabsByWorktree: {},
|
||||
unifiedTabsByWorktree: {}
|
||||
}
|
||||
return store
|
||||
}
|
||||
|
||||
@@ -1,5 +1,9 @@
|
||||
import type { ExecutionHostId } from '../../../../shared/execution-host'
|
||||
|
||||
/** The rollback target was replaced, so its workspace is already gone. */
|
||||
export const WORKTREE_INSTANCE_REPLACED_ERROR =
|
||||
'Workspace instance changed before cancellation cleanup.'
|
||||
|
||||
export type RemoveWorktreeOptions = {
|
||||
// 'forget-local' drops the workspace from Orca only (no remote Git/FS work)
|
||||
// for workspaces pinned to a removed/disconnected SSH host. Reuses the same
|
||||
|
||||
@@ -54,6 +54,24 @@ describe('removeWorktree state cleanup', () => {
|
||||
})
|
||||
expect(mockApi.worktrees.remove).not.toHaveBeenCalled()
|
||||
expect(store.getState().worktreesByRepo.repo1).toEqual([replacement])
|
||||
// The refused rollback must leave no trace on the live replacement's delete state.
|
||||
expect(store.getState().deleteStateByWorktreeId).toEqual({})
|
||||
})
|
||||
|
||||
it('does not read an absent row as a replacement during cancellation cleanup', async () => {
|
||||
const store = createTestStore()
|
||||
store.setState({ worktreesByRepo: { repo1: [] } } as Partial<AppState>)
|
||||
const result = await store
|
||||
.getState()
|
||||
.removeWorktree({ id: 'repo1::/path/gone', executionHostId: 'local' }, true, {
|
||||
skipArchiveHooks: true,
|
||||
expectedInstanceId: 'original-instance'
|
||||
})
|
||||
// Absent locally is not proof the checkout is gone, so this must not report
|
||||
// the benign "replaced" verdict that tells cleanup there is nothing to do.
|
||||
if (!result.ok) {
|
||||
expect(result.error).not.toBe('Workspace instance changed before cancellation cleanup.')
|
||||
}
|
||||
})
|
||||
|
||||
beforeEach(() => {
|
||||
|
||||
@@ -32,6 +32,7 @@ import {
|
||||
} from './worktree-create-parent-pick'
|
||||
|
||||
import { repoHostId, withRepoHostOwnership } from '../listing/worktree-host-ownership'
|
||||
import { WorktreeCreationCancelledError } from '@/lib/worktree-creation-attempt'
|
||||
|
||||
type RuntimeTarget = ReturnType<typeof getActiveRuntimeTarget>
|
||||
|
||||
@@ -193,7 +194,7 @@ export function createCreateWorktree(
|
||||
for (let attempt = 0; attempt < CLIENT_WORKTREE_CREATE_MAX_ATTEMPTS; attempt += 1) {
|
||||
try {
|
||||
if (options?.isCancelled?.()) {
|
||||
throw new Error('Worktree creation cancelled.')
|
||||
throw new WorktreeCreationCancelledError('Worktree creation cancelled.')
|
||||
}
|
||||
const outcome = await runCreateAttempt(
|
||||
request,
|
||||
|
||||
+24
-6
@@ -10,6 +10,7 @@ import type { WorktreeSlice } from '../../worktree-helpers'
|
||||
import type { getActiveRuntimeTarget } from '../../../../runtime/runtime-rpc-client'
|
||||
import type { Worktree } from '../../../../../../shared/worktree/types'
|
||||
import type { ExecutionHostId } from '../../../../../../shared/execution-host'
|
||||
import { WORKTREE_INSTANCE_REPLACED_ERROR } from '../../worktree-removal-options'
|
||||
import {
|
||||
getWorktreeOperationOwnerHostIds,
|
||||
resolveWorktreeOperationRoute,
|
||||
@@ -73,10 +74,18 @@ export function beginHostQualifiedRemoval(
|
||||
worktreeId: string,
|
||||
requiredExecutionHostId: ExecutionHostId | null,
|
||||
forgetLocalOnly: boolean,
|
||||
ignoreWorkspaceCleanupScanSurvivors = false
|
||||
ignoreWorkspaceCleanupScanSurvivors = false,
|
||||
expectedInstanceId?: string
|
||||
): HostQualifiedRemovalStart {
|
||||
const resolveRemovalRoute = (): WorktreeOperationRoute | null =>
|
||||
resolveHostQualifiedRemovalRoute(get, worktreeId, requiredExecutionHostId)
|
||||
const replacedInstance = refuseReplacedWorktreeInstance(
|
||||
findWorktreeOnConfirmedHost(get, worktreeId, requiredExecutionHostId),
|
||||
expectedInstanceId
|
||||
)
|
||||
if (replacedInstance) {
|
||||
return { ok: false, error: replacedInstance }
|
||||
}
|
||||
const removalRoute = resolveRemovalRoute()
|
||||
if (!removalRoute && (!forgetLocalOnly || !requiredExecutionHostId)) {
|
||||
// Why: callers mark rows deleting up front for immediate sidebar feedback
|
||||
@@ -159,16 +168,25 @@ export function refuseUnprovableRemoteHostRouting(
|
||||
)
|
||||
}
|
||||
|
||||
/** The row on the confirmed host only — a same-id row elsewhere must not stand in for it. */
|
||||
export function assertWorktreeRemovalInstance(
|
||||
/**
|
||||
* Refuse a delayed cancellation rollback whose path was reused by another
|
||||
* instance. Returns a reason instead of throwing so the caller can bail out
|
||||
* before marking a row deleting — the id now belongs to a live workspace whose
|
||||
* delete state must not carry this refusal.
|
||||
*/
|
||||
export function refuseReplacedWorktreeInstance(
|
||||
worktree: Worktree | undefined,
|
||||
expectedInstanceId?: string
|
||||
): void {
|
||||
if (expectedInstanceId && worktree?.instanceId !== expectedInstanceId) {
|
||||
throw new Error('Workspace instance changed before cancellation cleanup.')
|
||||
): string | null {
|
||||
// An absent row is not proof of a replacement — the checkout may simply not be
|
||||
// in local state yet — so leave that to the ordinary host-routing refusals.
|
||||
if (!expectedInstanceId || !worktree) {
|
||||
return null
|
||||
}
|
||||
return worktree.instanceId === expectedInstanceId ? null : WORKTREE_INSTANCE_REPLACED_ERROR
|
||||
}
|
||||
|
||||
/** The row on the confirmed host only — a same-id row elsewhere must not stand in for it. */
|
||||
export function findWorktreeOnConfirmedHost(
|
||||
get: WorktreeSliceGet,
|
||||
worktreeId: string,
|
||||
|
||||
@@ -15,7 +15,6 @@ import {
|
||||
} from '@/lib/worktree-operation-route'
|
||||
import {
|
||||
beginHostQualifiedRemoval,
|
||||
assertWorktreeRemovalInstance,
|
||||
completeSameIdHostScopedRemoval,
|
||||
findWorktreeOnConfirmedHost,
|
||||
prepareHostScopedRemovalCompletion,
|
||||
@@ -59,7 +58,8 @@ export function createRemoveWorktree(
|
||||
worktreeId,
|
||||
requiredExecutionHostId,
|
||||
forgetLocalOnly,
|
||||
options?.ignoreWorkspaceCleanupScanSurvivors === true
|
||||
options?.ignoreWorkspaceCleanupScanSurvivors === true,
|
||||
options?.expectedInstanceId
|
||||
)
|
||||
if (!start.ok) {
|
||||
return { ok: false, error: start.error }
|
||||
@@ -107,7 +107,6 @@ export function createRemoveWorktree(
|
||||
worktreeId,
|
||||
requiredExecutionHostId
|
||||
)
|
||||
assertWorktreeRemovalInstance(worktreeBeforeRemoval, options?.expectedInstanceId)
|
||||
const terminalPtyIdsBeforeRemoval = (get().tabsByWorktree[worktreeId] ?? []).flatMap(
|
||||
(tab) => get().ptyIdsByTabId[tab.id] ?? []
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user