diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index a390b1b3bd3..f41bab6f557 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -12,12 +12,17 @@ "gates": [ { "id": "ephemeral-vm-runtime.rollback-readable-sidecar", - "title": "VM lifecycle records remain readable across provisioned-root rollback", + "title": "VM lifecycle stays rollback-readable and cleanup-retryable", "maturity": "experimental", "protection": "partial", "owner": "ephemeral-vm-runtime-store", "layer": "desktop-main-persistence", - "surfaces": ["VM runtime sidecar", "recipe lifecycle", "provisioned-root direct SSH"], + "surfaces": [ + "VM runtime sidecar", + "recipe lifecycle", + "cleanup retry", + "provisioned-root direct SSH" + ], "platforms": ["macos", "linux", "windows"], "providers": ["orca-server", "direct-ssh"], "coveredPlatforms": ["macos", "linux", "windows"], @@ -28,11 +33,11 @@ "https://github.com/stablyai/orca/pull/14352", "https://github.com/stablyai/orca/issues/13044" ], - "invariant": "A sidecar written by the new build cannot make the previous production build lose otherwise compatible VM runtimes. Downgrade lifecycle writes remain authoritative after re-upgrade, provisioned-root metadata remains recoverable, and corrupt or future feature metadata is preserved without poisoning the rollback-readable v1 store.", - "oracle": "Write one ordinary schema-v1 runtime and one provisioned-root runtime. Require the exact pre-#14352 reader to return both, run the old destroy lifecycle against the projected provisioned record, then require the candidate to retain the cleaned state while restoring schema-v2 and checkout-mode metadata. The same old reader must reject affected-main and fix-reverted bytes. Malformed, oversized, unknown-record, and future-version feature sidecars must leave rollback-compatible records accessible and must not be overwritten during compatible lifecycle mutations.", + "invariant": "A sidecar written by the new build cannot make the previous production build lose otherwise compatible VM runtimes. Downgrade lifecycle writes remain authoritative after re-upgrade, provisioned-root metadata remains recoverable, and corrupt or future feature metadata is preserved without poisoning the rollback-readable v1 store. Destroy attempts settle at a bounded deadline, retry starts fresh, and provider failure retains the SSH/project context needed for safe cleanup.", + "oracle": "Write one ordinary schema-v1 runtime and one provisioned-root runtime. Require the exact pre-#14352 reader to return both, run the old destroy lifecycle against the projected provisioned record, then require the candidate to retain the cleaned state while restoring schema-v2 and checkout-mode metadata. The same old reader must reject affected-main and fix-reverted bytes. Malformed, oversized, unknown-record, and future-version feature sidecars must leave rollback-compatible records accessible and must not be overwritten during compatible lifecycle mutations. Force destroy to outlive its deadline, require cleanup_failed plus a fresh successful retry, and prove provider or project-host rollback failure retains every ownership record required to retry.", "commands": [ "node config/scripts/run-ephemeral-vm-runtime-store-rollback-repro.mjs config/scripts/ephemeral-vm-runtime-store-cross-version.test.ts", - "pnpm exec vitest run --config config/vitest.config.ts src/shared/ephemeral-vm-runtime-store.test.ts src/shared/ephemeral-vm-runtime-store-rollback.test.ts src/main/ephemeral-vm-runtime-service.test.ts src/main/ephemeral-vm-recipe-runner.test.ts src/main/ephemeral-vm-runtime-ssh-cleanup.test.ts src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts src/main/ipc/ephemeral-vm.test.ts src/main/provisioned-root-ssh-adoption.test.ts", + "pnpm exec vitest run --config config/vitest.config.ts src/shared/ephemeral-vm-runtime-store.test.ts src/shared/ephemeral-vm-runtime-store-rollback.test.ts src/shared/ephemeral-vm-recipe-process.test.ts src/main/ephemeral-vm-runtime-service.test.ts src/main/ephemeral-vm-recipe-runner.test.ts src/main/ephemeral-vm-runtime-ssh-cleanup.test.ts src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts src/main/ipc/ephemeral-vm.test.ts src/main/provisioned-root-ssh-adoption.test.ts src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts", "ORCA_E2E_SSH_DOCKER=1 pnpm exec playwright test tests/e2e/ephemeral-vm-provisioned-root.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1", "SKIP_BUILD=1 ORCA_E2E_SSH_DOCKER=1 pnpm exec playwright test tests/e2e/ephemeral-vm-provisioned-root.spec.ts --config tests/playwright.config.ts --project electron-headless --workers=1" ], @@ -41,12 +46,15 @@ "config/scripts/run-ephemeral-vm-runtime-store-rollback-repro.mjs", "src/shared/ephemeral-vm-runtime-store-rollback.test.ts", "src/shared/ephemeral-vm-runtime-store.test.ts", + "src/shared/ephemeral-vm-recipe-process.test.ts", "src/main/ephemeral-vm-runtime-service.test.ts", "src/main/ephemeral-vm-recipe-runner.test.ts", "src/main/ephemeral-vm-runtime-ssh-cleanup.test.ts", "src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts", "src/main/ipc/ephemeral-vm.test.ts", "src/main/provisioned-root-ssh-adoption.test.ts", + "src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.test.ts", + "src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts", "tests/e2e/ephemeral-vm-provisioned-root.spec.ts" ], "assertionRefs": [ @@ -74,6 +82,23 @@ "post-create persistence failure destroys the provider resource when possible", "failed destroy persists a durable rollback-readable cleanup recovery record" ] + }, + { + "file": "src/shared/ephemeral-vm-recipe-process.test.ts", + "assertions": [ + "a destroy process that never closes is killed and settled at its bounded deadline" + ] + }, + { + "file": "src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts", + "assertions": [ + "provider failure retains runtime-owned SSH targets and confirmed success releases them", + "unknown runtime inventory retains an explicitly requested target" + ] + }, + { + "file": "src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.test.ts", + "assertions": ["failed or unconfirmed project-host rollback prevents provider destroy"] } ], "evidenceRuns": [ @@ -90,10 +115,10 @@ "date": "2026-08-14", "runner": "local", "platform": "macos", - "command": "pnpm exec vitest run --config config/vitest.config.ts src/shared/ephemeral-vm-runtime-store.test.ts src/shared/ephemeral-vm-runtime-store-rollback.test.ts src/main/ephemeral-vm-runtime-service.test.ts src/main/ephemeral-vm-recipe-runner.test.ts src/main/ephemeral-vm-runtime-ssh-cleanup.test.ts src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts src/main/ipc/ephemeral-vm.test.ts src/main/provisioned-root-ssh-adoption.test.ts", + "command": "pnpm exec vitest run --config config/vitest.config.ts src/shared/ephemeral-vm-runtime-store.test.ts src/shared/ephemeral-vm-runtime-store-rollback.test.ts src/shared/ephemeral-vm-recipe-process.test.ts src/main/ephemeral-vm-runtime-service.test.ts src/main/ephemeral-vm-recipe-runner.test.ts src/main/ephemeral-vm-runtime-ssh-cleanup.test.ts src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts src/main/ipc/ephemeral-vm.test.ts src/main/provisioned-root-ssh-adoption.test.ts src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts", "result": "passed", "durationSeconds": 2, - "summary": "Sixty-two focused store, recipe, lifecycle, cleanup-handler, provisioning-recovery, and provisioned-root adoption tests passed." + "summary": "Seventy-nine focused store, recipe-process, lifecycle, cleanup-handler, retry-context, provisioning-recovery, and provisioned-root adoption tests passed." }, { "date": "2026-08-14", @@ -119,18 +144,19 @@ }, "performanceBudget": { "required": true, - "evidence": "Ordinary writes remain byte-identical and create no feature sidecar. Reads add one bounded feature-file existence check; sorted feature comparisons avoid unchanged rewrites, and durable companion writes occur only when compatibility metadata changes. There is no polling, timer, renderer work, RPC traffic, or remote fanout." + "evidence": "Ordinary writes remain byte-identical and create no feature sidecar. Reads add one bounded feature-file existence check; sorted feature comparisons avoid unchanged rewrites, and durable companion writes occur only when compatibility metadata changes. Cleanup adds one unrefed timer per active destroy process and clears it on settlement; there is no polling, background scan, or remote fanout." }, "promotionCriteria": [ "Keep the pinned baseline/latest/candidate/revert oracle green in CI.", "Keep the Docker provisioned-root create/adopt/terminal/remove/destroy journey green.", + "Keep hung destroy, fresh retry, and retained-context failure tests green.", "Collect soak history without unexplained lifecycle or sidecar flakes." ], "knownGaps": [ "No packaged-build downgrade installer journey; the exact released source reader and service are executed in-process.", "Live Docker SSH evidence runs locally on macOS and in the changed-file PR E2E lane on Ubuntu; Windows desktop behavior is covered by the platform-neutral filesystem codec and package job." ], - "demotionRule": "Demote if a new write fails the pinned rollback reader, a downgrade lifecycle mutation is lost after re-upgrade, malformed/future metadata overwrites recoverable bytes, ordinary v1 bytes change, or the focused gate flakes without an identified harness defect." + "demotionRule": "Demote if a new write fails the pinned rollback reader, a downgrade lifecycle mutation is lost after re-upgrade, malformed/future metadata overwrites recoverable bytes, ordinary v1 bytes change, a destroy can remain permanently in flight, cleanup loses retry ownership, or the focused gate flakes without an identified harness defect." }, { "id": "terminal-session.shell-ready-exec-prompt-fallback", diff --git a/src/main/ephemeral-vm-runtime-service.test.ts b/src/main/ephemeral-vm-runtime-service.test.ts index 9280be2ba90..f2185aaaade 100644 --- a/src/main/ephemeral-vm-runtime-service.test.ts +++ b/src/main/ephemeral-vm-runtime-service.test.ts @@ -159,6 +159,61 @@ describe('ephemeral VM runtime service', () => { expect(readFileSync(join(repoPath, 'cleanup-count.txt'), 'utf8')).toBe('x') }) + it('times out a hung destroy and starts a fresh retry', async () => { + const userDataPath = makeDir('orca-ephemeral-vm-service-user-data-') + const repoPath = makeDir('orca-ephemeral-vm-service-repo-') + const cleanupPath = join(repoPath, 'cleanup.js') + const countPath = join(repoPath, 'cleanup-count.txt') + writeFileSync( + cleanupPath, + `require('fs').appendFileSync(${JSON.stringify(countPath)}, 'x'); setInterval(() => {}, 1000)` + ) + const recipe: OrcaVmRecipe = { + id: 'cloud-sandbox', + name: 'Cloud Sandbox', + create: 'unused', + destroy: nodeCommand(cleanupPath) + } + upsertEphemeralVmRuntime(userDataPath, { + id: 'runtime-1', + recipeId: recipe.id, + recipe, + status: 'running', + cleanupStatus: 'not_started', + createdAt: 1_000, + updatedAt: 1_000, + recipeResult: { + schemaVersion: 1, + connection: { + type: 'ssh', + projectRoot: '/workspace/repo', + target: { label: 'VM', host: 'host', port: 22, username: 'orca' } + } + } + }) + const cleanupArgs = { + userDataPath, + repoPath, + recipe, + runtimeId: 'runtime-1', + destroyTimeoutMs: 100 + } + + await expect(cleanupEphemeralVmRuntime(cleanupArgs)).resolves.toMatchObject({ + ok: false, + runtime: { status: 'cleanup_failed', cleanupStatus: 'failed' }, + error: expect.stringContaining('timed out') + }) + writeFileSync(cleanupPath, `require('fs').appendFileSync(${JSON.stringify(countPath)}, 'x')`) + await expect( + cleanupEphemeralVmRuntime({ ...cleanupArgs, destroyTimeoutMs: 2_000 }) + ).resolves.toMatchObject({ + ok: true, + runtime: { status: 'cleaned', cleanupStatus: 'succeeded' } + }) + expect(readFileSync(countPath, 'utf8')).toBe('xx') + }) + it('does not persist a runtime when recipe output cannot be parsed', async () => { const userDataPath = makeDir('orca-ephemeral-vm-service-user-data-') const repoPath = makeDir('orca-ephemeral-vm-service-repo-') diff --git a/src/main/ephemeral-vm-runtime-service.ts b/src/main/ephemeral-vm-runtime-service.ts index a6dd1933f89..c9ce5f0c346 100644 --- a/src/main/ephemeral-vm-runtime-service.ts +++ b/src/main/ephemeral-vm-runtime-service.ts @@ -55,6 +55,7 @@ export type CleanupEphemeralVmRuntimeArgs = { recipe: OrcaVmRecipe runtimeId: string now?: number + destroyTimeoutMs?: number signal?: AbortSignal onStdout?: (chunk: string) => void onStderr?: (chunk: string) => void @@ -184,6 +185,7 @@ async function cleanupEphemeralVmRuntimeOnce( recipe: args.recipe, context: contextFromRuntime(args.repoPath, running), recipeResult: running.recipeResult, + timeoutMs: args.destroyTimeoutMs, signal: args.signal, onStdout: args.onStdout, onStderr: args.onStderr diff --git a/src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts b/src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts index b5b2100d4d2..85320c8a0ad 100644 --- a/src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts +++ b/src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts @@ -47,7 +47,7 @@ afterEach(() => { } }) -it('removes the hidden SSH target when recipe context is unavailable', async () => { +it('removes the hidden SSH target when provider cleanup cannot start', async () => { const userDataPath = mkdtempSync(join(tmpdir(), 'orca-vm-runtime-handler-')) tempDirs.push(userDataPath) getPathMock.mockReturnValue(userDataPath) @@ -79,6 +79,7 @@ it('removes the hidden SSH target when recipe context is unavailable', async () expect(cleaned).toMatchObject({ status: 'cleanup_failed', cleanupStatus: 'failed', + connectionMode: undefined, sshTargetId: undefined }) expect(removeRuntimeOwnedSshTargetMock).toHaveBeenCalledWith('runtime-ssh-missing-context') diff --git a/src/main/ipc/ephemeral-vm-runtime-handlers.ts b/src/main/ipc/ephemeral-vm-runtime-handlers.ts index 3175680a923..69fdae1cebc 100644 --- a/src/main/ipc/ephemeral-vm-runtime-handlers.ts +++ b/src/main/ipc/ephemeral-vm-runtime-handlers.ts @@ -111,8 +111,9 @@ export function registerEphemeralVmRuntimeHandlers(store: Store): void { // environment row; users can still remove that manually. } } - // Remove even on cleanup_failed (removal is idempotent via the deterministic - // id) so a terminal cleanup never orphans the hidden SSH target. + if (!result.ok) { + return result.runtime + } return removeEphemeralVmRuntimeSshTarget({ userDataPath, runtime: result.runtime, diff --git a/src/main/ipc/ephemeral-vm.test.ts b/src/main/ipc/ephemeral-vm.test.ts index d1cf54d26e0..4f71dde0f9f 100644 --- a/src/main/ipc/ephemeral-vm.test.ts +++ b/src/main/ipc/ephemeral-vm.test.ts @@ -498,7 +498,7 @@ describe('registerEphemeralVmHandlers', () => { expect(listEnvironments(userDataPath)).toEqual([]) }) - it('removes the runtime-owned SSH target on cleanup even when destroy fails', async () => { + it('retains the runtime-owned SSH target when destroy fails', async () => { const userDataPath = makeDir('orca-ephemeral-vm-ipc-user-data-') const repoPath = makeDir('orca-ephemeral-vm-ipc-repo-') getPathMock.mockReturnValue(userDataPath) @@ -523,8 +523,6 @@ describe('registerEphemeralVmHandlers', () => { '}))' ].join('\n') ) - // Why: a failing destroy drives the cleanup_failed branch; the runtime-owned - // SSH target must still be torn down so it never orphans (see Fix D). writeFileSync(destroyPath, 'process.exit(1)') writeFileSync( join(repoPath, 'orca.yaml'), @@ -554,9 +552,9 @@ describe('registerEphemeralVmHandlers', () => { expect(cleaned).toEqual(expect.objectContaining({ status: 'cleanup_failed' })) expect(cleaned.cleanupLastError).toBeTruthy() - expect(removeRuntimeOwnedSshTargetMock).toHaveBeenCalledWith('runtime-ssh-orca-instance-1') - expect(cleaned.connectionMode).toBeUndefined() - expect(cleaned.sshTargetId).toBeUndefined() + expect(removeRuntimeOwnedSshTargetMock).not.toHaveBeenCalled() + expect(cleaned.connectionMode).toBe('ssh') + expect(cleaned.sshTargetId).toBe('runtime-ssh-orca-instance-1') }) it('retries hidden SSH teardown without rerunning completed provider cleanup', async () => { diff --git a/src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.test.ts b/src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.test.ts index 5b6137747f6..8faa4bac982 100644 --- a/src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.test.ts +++ b/src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.test.ts @@ -32,6 +32,7 @@ describe('cleanupFailedEphemeralVmWorkspace', () => { await cleanupFailedEphemeralVmWorkspace(request(), { deleteProjectHostSetup: vi.fn(async () => { order.push('setup') + return { id: 'setup-1' } }), cleanupRuntime: vi.fn(async () => { order.push('runtime') @@ -43,7 +44,7 @@ describe('cleanupFailedEphemeralVmWorkspace', () => { expect(order).toEqual(['setup', 'runtime']) }) - it('still destroys the VM when setup deletion fails', async () => { + it('retains the VM when setup deletion throws', async () => { const cleanupRuntime = vi.fn().mockResolvedValue(undefined) const reportSetupError = vi.fn() await cleanupFailedEphemeralVmWorkspace(request(), { @@ -54,6 +55,20 @@ describe('cleanupFailedEphemeralVmWorkspace', () => { }) expect(reportSetupError).toHaveBeenCalledOnce() - expect(cleanupRuntime).toHaveBeenCalledWith('runtime-1') + expect(cleanupRuntime).not.toHaveBeenCalled() + }) + + it('retains the VM when setup deletion reports no result', async () => { + const cleanupRuntime = vi.fn().mockResolvedValue(undefined) + const reportSetupError = vi.fn() + await cleanupFailedEphemeralVmWorkspace(request(), { + deleteProjectHostSetup: vi.fn().mockResolvedValue(null), + cleanupRuntime, + reportSetupError, + reportRuntimeError: vi.fn() + }) + + expect(reportSetupError).toHaveBeenCalledOnce() + expect(cleanupRuntime).not.toHaveBeenCalled() }) }) diff --git a/src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.ts b/src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.ts index e51bcf7d29c..2c71e66c834 100644 --- a/src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.ts +++ b/src/renderer/src/lib/ephemeral-vm-failed-create-cleanup.ts @@ -19,9 +19,16 @@ export async function cleanupFailedEphemeralVmWorkspace( request.workspaceRunContext?.projectHostSetupId ) { try { - await actions.deleteProjectHostSetup(request.workspaceRunContext.projectHostSetupId) + const deleted = await actions.deleteProjectHostSetup( + request.workspaceRunContext.projectHostSetupId + ) + if (deleted == null) { + actions.reportSetupError(new Error('Could not confirm project host rollback.')) + return + } } catch (error) { actions.reportSetupError(error) + return } } try { diff --git a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts index 664c2ff656a..78c9fafb424 100644 --- a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts +++ b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts @@ -28,7 +28,10 @@ describe('cleanupEphemeralVmRuntimesForDeleted', () => { expect(cleanup).toHaveBeenCalledTimes(1) expect(cleanup).toHaveBeenCalledWith({ runtimeId: 'rt-1' }) - expect(destroyed).toEqual(['runtime-ssh-a']) + expect(destroyed).toEqual({ + destroyedSshTargetIds: ['runtime-ssh-a'], + retainedSshTargetIds: [] + }) }) it('cleans a runtime matched only by its runtime-owned SSH target id', async () => { @@ -43,7 +46,10 @@ describe('cleanupEphemeralVmRuntimesForDeleted', () => { }) expect(cleanup).toHaveBeenCalledWith({ runtimeId: 'rt-1' }) - expect(destroyed).toEqual(['runtime-ssh-orca-1']) + expect(destroyed).toEqual({ + destroyedSshTargetIds: ['runtime-ssh-orca-1'], + retainedSshTargetIds: [] + }) }) it('ignores non-runtime-owned target ids and already-cleaned runtimes', async () => { @@ -58,7 +64,7 @@ describe('cleanupEphemeralVmRuntimesForDeleted', () => { }) expect(cleanup).not.toHaveBeenCalled() - expect(destroyed).toEqual([]) + expect(destroyed).toEqual({ destroyedSshTargetIds: [], retainedSshTargetIds: [] }) }) it('retries a completed provider cleanup while its SSH target remains', async () => { @@ -74,7 +80,7 @@ describe('cleanupEphemeralVmRuntimesForDeleted', () => { cleanup.mockResolvedValue({ status: 'cleaned', cleanupStatus: 'succeeded' }) await expect(cleanupEphemeralVmRuntimesForDeleted({ workspaceIds: ['wt-1'] })).resolves.toEqual( - ['runtime-ssh-a'] + { destroyedSshTargetIds: ['runtime-ssh-a'], retainedSshTargetIds: [] } ) expect(cleanup).toHaveBeenCalledWith({ runtimeId: 'rt-1' }) }) @@ -90,14 +96,27 @@ describe('cleanupEphemeralVmRuntimesForDeleted', () => { }) await expect(cleanupEphemeralVmRuntimesForDeleted({ workspaceIds: ['wt-1'] })).resolves.toEqual( - [] + { destroyedSshTargetIds: [], retainedSshTargetIds: ['runtime-ssh-a'] } ) }) it('swallows listRuntimes failures', async () => { listRuntimes.mockRejectedValue(new Error('boom')) await expect(cleanupEphemeralVmRuntimesForDeleted({ workspaceIds: ['wt-1'] })).resolves.toEqual( - [] + { destroyedSshTargetIds: [], retainedSshTargetIds: [] } ) }) + + it('retains an explicit runtime target when runtime listing fails', async () => { + listRuntimes.mockRejectedValue(new Error('boom')) + + await expect( + cleanupEphemeralVmRuntimesForDeleted({ + runtimeOwnedSshTargetIds: ['runtime-ssh-a'] + }) + ).resolves.toEqual({ + destroyedSshTargetIds: [], + retainedSshTargetIds: ['runtime-ssh-a'] + }) + }) }) diff --git a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts index 71c0231cb8b..817cd8e5f72 100644 --- a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts +++ b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts @@ -10,16 +10,21 @@ import { isRuntimeOwnedSshTargetId } from '../../../shared/execution-host' * routes through project removal, which must not leak the live Docker/VM and its * hidden SSH target. * - * Returns the runtime-owned SSH target ids that were destroyed so the caller can - * purge the now-orphaned project that pointed at them. + * Classifies runtime-owned SSH targets so callers purge only confirmed-destroyed projects. */ +export type EphemeralVmCleanupSummary = { + destroyedSshTargetIds: string[] + retainedSshTargetIds: string[] +} + export async function cleanupEphemeralVmRuntimesForDeleted(args: { workspaceIds?: readonly string[] // Raw runtime-owned SSH target ids (e.g. a removed repo's connectionId) whose // backing runtime should also be torn down, even if no workspace id matched. runtimeOwnedSshTargetIds?: readonly string[] -}): Promise { - const destroyedSshTargetIds: string[] = [] +}): Promise { + const destroyedSshTargetIds = new Set() + const retainedSshTargetIds = new Set() try { const workspaceIdSet = new Set(args.workspaceIds ?? []) const sshTargetIdSet = new Set( @@ -33,13 +38,29 @@ export async function cleanupEphemeralVmRuntimesForDeleted(args: { (runtime.sshTargetId !== undefined && sshTargetIdSet.has(runtime.sshTargetId))) ) for (const runtime of matchingRuntimes) { - const cleaned = await window.api.ephemeralVm.cleanup({ runtimeId: runtime.id }) - if (runtime.sshTargetId && !cleaned.sshTargetId) { - destroyedSshTargetIds.push(runtime.sshTargetId) + try { + const cleaned = await window.api.ephemeralVm.cleanup({ runtimeId: runtime.id }) + if (runtime.sshTargetId) { + const targetIds = cleaned.sshTargetId ? retainedSshTargetIds : destroyedSshTargetIds + targetIds.add(runtime.sshTargetId) + } + } catch (error) { + console.error('Failed to clean up ephemeral VM runtime for deleted workspace:', error) + if (runtime.sshTargetId) { + retainedSshTargetIds.add(runtime.sshTargetId) + } } } } catch (error) { console.error('Failed to clean up ephemeral VM runtime for deleted workspace:', error) + for (const targetId of args.runtimeOwnedSshTargetIds ?? []) { + if (isRuntimeOwnedSshTargetId(targetId)) { + retainedSshTargetIds.add(targetId) + } + } + } + return { + destroyedSshTargetIds: [...destroyedSshTargetIds], + retainedSshTargetIds: [...retainedSshTargetIds] } - return destroyedSshTargetIds } diff --git a/src/renderer/src/store/slices/repos-ephemeral-vm-cleanup-retention.test.ts b/src/renderer/src/store/slices/repos-ephemeral-vm-cleanup-retention.test.ts new file mode 100644 index 00000000000..afc54b88af1 --- /dev/null +++ b/src/renderer/src/store/slices/repos-ephemeral-vm-cleanup-retention.test.ts @@ -0,0 +1,49 @@ +import { expect, it, vi } from 'vitest' +import { toast } from 'sonner' +import type { Repo } from '../../../../shared/repo-types' +import { + ephemeralVmCleanup, + ephemeralVmListRuntimes, + installReposRuntimeRoutingHarness, + reposRemove, + sshRepo +} from './repos-runtime-routing-fixture' +import { createTestStore } from './store-test-helpers' + +vi.mock('sonner', () => ({ + toast: { + error: vi.fn(), + info: vi.fn(), + success: vi.fn(), + warning: vi.fn() + } +})) + +installReposRuntimeRoutingHarness() + +it('retains a runtime-owned SSH project when VM cleanup fails', async () => { + const runtimeRepo: Repo = { ...sshRepo, connectionId: 'runtime-ssh-runtime-1' } + ephemeralVmListRuntimes.mockResolvedValue([ + { + id: 'runtime-1', + cleanupStatus: 'not_started', + sshTargetId: runtimeRepo.connectionId + } + ]) + ephemeralVmCleanup.mockResolvedValue({ + status: 'cleanup_failed', + cleanupStatus: 'failed', + sshTargetId: runtimeRepo.connectionId + }) + const store = createTestStore() + store.setState({ repos: [runtimeRepo], activeRepoId: runtimeRepo.id }) + + await store.getState().removeProject(runtimeRepo.id, { errorFeedback: 'toast' }) + + expect(store.getState().repos).toEqual([runtimeRepo]) + expect(reposRemove).not.toHaveBeenCalled() + expect(toast.error).toHaveBeenCalledWith( + expect.any(String), + expect.objectContaining({ description: expect.stringContaining('Retry cleanup') }) + ) +}) diff --git a/src/renderer/src/store/slices/repos-runtime-routing-fixture.ts b/src/renderer/src/store/slices/repos-runtime-routing-fixture.ts index 5df2606b057..f7dca6594ae 100644 --- a/src/renderer/src/store/slices/repos-runtime-routing-fixture.ts +++ b/src/renderer/src/store/slices/repos-runtime-routing-fixture.ts @@ -56,6 +56,8 @@ export const runtimeEnvironmentCall: Mock = vi.fn() export const runtimeEnvironmentTransportCall: Mock = vi.fn() export const orcaProfileFindProjectProfiles: Mock = vi.fn() export const uiSet: Mock = vi.fn() +export const ephemeralVmListRuntimes: Mock = vi.fn() +export const ephemeralVmCleanup: Mock = vi.fn() // Registers the per-test reset + window stub. Call once inside the suite's module scope. export function installReposRuntimeRoutingHarness(): void { @@ -86,6 +88,8 @@ export function installReposRuntimeRoutingHarness(): void { runtimeEnvironmentTransportCall.mockReset() uiSet.mockReset() uiSet.mockResolvedValue(undefined) + ephemeralVmListRuntimes.mockReset().mockResolvedValue([]) + ephemeralVmCleanup.mockReset() runtimeEnvironmentTransportCall.mockImplementation((args: RuntimeEnvironmentCallRequest) => { return createCompatibleRuntimeStatusResponseIfNeeded(args) ?? runtimeEnvironmentCall(args) }) @@ -117,6 +121,10 @@ export function installReposRuntimeRoutingHarness(): void { }, pty: { kill: ptyKill }, runtimeEnvironments: { call: runtimeEnvironmentTransportCall }, + ephemeralVm: { + listRuntimes: ephemeralVmListRuntimes, + cleanup: ephemeralVmCleanup + }, ui: { set: uiSet } } }) diff --git a/src/renderer/src/store/slices/repos.ts b/src/renderer/src/store/slices/repos.ts index a1e78967da5..2a802bc57b2 100644 --- a/src/renderer/src/store/slices/repos.ts +++ b/src/renderer/src/store/slices/repos.ts @@ -3602,12 +3602,16 @@ export const createRepoSlice: StateCreator = (set, return } const ownerHostId = getRepoExecutionHostId(ownerRepo) + const runtimeSshTargetId = ownerRepo.connectionId // Why: an SSH per-workspace-env's workspace is the repo's main worktree, so removal routes here; tear down its ephemeral runtime first so it doesn't leak. - if (isRuntimeOwnedSshTargetId(ownerRepo.connectionId)) { - await cleanupEphemeralVmRuntimesForDeleted({ + if (runtimeSshTargetId && isRuntimeOwnedSshTargetId(runtimeSshTargetId)) { + const cleanup = await cleanupEphemeralVmRuntimesForDeleted({ workspaceIds: getKnownRepoWorktreeIds(get(), projectId, ownerHostId), - runtimeOwnedSshTargetIds: [ownerRepo.connectionId as string] + runtimeOwnedSshTargetIds: [runtimeSshTargetId] }) + if (cleanup.retainedSshTargetIds.includes(runtimeSshTargetId)) { + throw new Error('The cloud VM could not be destroyed. Retry cleanup before removing it.') + } } // Why: derive the target from the owner's settings (via options.hostId) so an SSH host removal never routes repo.rm to the focused runtime. const target = getActiveRuntimeTarget(settingsForRepoOwner(get(), projectId, options?.hostId)) diff --git a/src/renderer/src/store/slices/worktrees.test.ts b/src/renderer/src/store/slices/worktrees.test.ts index b865fe22ffa..aeacf05ab21 100644 --- a/src/renderer/src/store/slices/worktrees.test.ts +++ b/src/renderer/src/store/slices/worktrees.test.ts @@ -10573,6 +10573,7 @@ describe('pending worktree creation state', () => { it('removePendingWorktreeCreation cleans up a provisioned-root setup and VM runtime', async () => { const store = createTestStore() const deleteProjectHostSetup = vi.mocked(store.getState().deleteProjectHostSetup) + deleteProjectHostSetup.mockResolvedValue({ setup: { id: 'setup-1' } } as never) store.getState().beginPendingWorktreeCreation( makePendingCreation('c1', { phase: 'fetching', diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 25ac9ffcd72..2c0f2528666 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -4388,11 +4388,11 @@ export const createWorktreeSlice: StateCreator backendOwnsPtyTeardown: true }) // Why: dispose the SSH relay AFTER terminal teardown so a still-mounted pane can't hit a gone relay and toast "SSH not active". - const destroyedRuntimeSshTargetIds = await cleanupEphemeralVmRuntimesForDeleted({ + const runtimeCleanup = await cleanupEphemeralVmRuntimesForDeleted({ workspaceIds: [worktreeId] }) // Remove the orphaned project for the destroyed SSH target so it can't surface as a dead project in the composer. - await purgeOrphanedRuntimeSshProjects(get, destroyedRuntimeSshTargetIds) + await purgeOrphanedRuntimeSshProjects(get, runtimeCleanup.destroyedSshTargetIds) const tabs = get().tabsByWorktree[worktreeId] ?? [] const tabIds = new Set(tabs.map((t) => t.id)) diff --git a/src/shared/ephemeral-vm-recipe-destroy-result.ts b/src/shared/ephemeral-vm-recipe-destroy-result.ts new file mode 100644 index 00000000000..32f55b5ca1d --- /dev/null +++ b/src/shared/ephemeral-vm-recipe-destroy-result.ts @@ -0,0 +1,32 @@ +import type { ProcessRunResult } from './ephemeral-vm-recipe-process' + +export const EPHEMERAL_VM_RECIPE_DESTROY_TIMEOUT_MS = 5 * 60 * 1000 + +type FailedEphemeralVmRecipeDestroy = { + ok: false + skipped: false + error: string +} & ProcessRunResult + +export function getEphemeralVmRecipeDestroyFailure( + result: ProcessRunResult, + timeoutMs: number +): FailedEphemeralVmRecipeDestroy | null { + if (result.timedOut) { + return { + ok: false, + skipped: false, + error: `Destroy timed out after ${timeoutMs}ms.`, + ...result + } + } + if (result.exitCode !== 0) { + return { + ok: false, + skipped: false, + error: `Destroy exited with code ${result.exitCode ?? 'unknown'}.`, + ...result + } + } + return null +} diff --git a/src/shared/ephemeral-vm-recipe-process.test.ts b/src/shared/ephemeral-vm-recipe-process.test.ts index 118847ab251..6995e683a89 100644 --- a/src/shared/ephemeral-vm-recipe-process.test.ts +++ b/src/shared/ephemeral-vm-recipe-process.test.ts @@ -1,12 +1,15 @@ +import { EventEmitter } from 'node:events' import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { PassThrough } from 'node:stream' import { tmpdir } from 'node:os' import { join } from 'node:path' -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { runRecipeCommand } from './ephemeral-vm-recipe-process' const tmpRoots: string[] = [] afterEach(() => { + vi.useRealTimers() for (const root of tmpRoots.splice(0)) { rmSync(root, { recursive: true, force: true }) } @@ -23,6 +26,33 @@ function nodeCommand(scriptPath: string): string { } describe('runRecipeCommand', () => { + it('settles and kills a destroy process at its deadline even if close never arrives', async () => { + vi.useFakeTimers() + const child = Object.assign(new EventEmitter(), { + pid: undefined, + stdin: new PassThrough(), + stdout: new PassThrough(), + stderr: new PassThrough(), + kill: vi.fn(), + unref: vi.fn() + }) + const resultPromise = runRecipeCommand({ + command: 'destroy', + repoPath: makeRepo(), + mode: 'destroy', + resultSchemaVersion: 1, + context: { recipeId: 'cloud-sandbox', repoPath: makeRepo() }, + timeoutMs: 1_000, + spawnCommand: vi.fn(() => child) as never + }) + + await vi.advanceTimersByTimeAsync(1_000) + + await expect(resultPromise).resolves.toMatchObject({ timedOut: true, exitCode: null }) + expect(child.kill).toHaveBeenCalledWith('SIGKILL') + expect(child.unref).toHaveBeenCalledOnce() + }) + it.each([ { output: 'abcdef', maxCaptureBytes: 4, expected: 'cdef' }, { output: 'A😀B', maxCaptureBytes: 5, expected: '😀B' }, diff --git a/src/shared/ephemeral-vm-recipe-process.ts b/src/shared/ephemeral-vm-recipe-process.ts index d2ecc5b83b8..dbd64ad58c3 100644 --- a/src/shared/ephemeral-vm-recipe-process.ts +++ b/src/shared/ephemeral-vm-recipe-process.ts @@ -8,6 +8,7 @@ export type ProcessRunResult = { stderr: string exitCode: number | null signal: NodeJS.Signals | null + timedOut?: true } export function quoteShellToken(value: string): string { @@ -29,6 +30,7 @@ export async function runRecipeCommand(args: { stdin?: string env?: NodeJS.ProcessEnv maxCaptureBytes?: number + timeoutMs?: number signal?: AbortSignal onStdout?: (chunk: string) => void onStderr?: (chunk: string) => void @@ -36,6 +38,9 @@ export async function runRecipeCommand(args: { }): Promise { const maxBytes = args.maxCaptureBytes ?? DEFAULT_MAX_CAPTURE_BYTES const spawnCommand = args.spawnCommand ?? spawn + if (args.timeoutMs !== undefined && (!Number.isFinite(args.timeoutMs) || args.timeoutMs <= 0)) { + throw new Error('Recipe command timeout must be a positive finite number.') + } return new Promise((resolve, reject) => { let child: ChildProcessWithoutNullStreams @@ -55,6 +60,29 @@ export async function runRecipeCommand(args: { let stdout = '' let stderr = '' let settled = false + let timeout: ReturnType | undefined + const finish = (result: ProcessRunResult): void => { + if (settled) { + return + } + settled = true + if (timeout) { + clearTimeout(timeout) + } + args.signal?.removeEventListener('abort', abort) + resolve(result) + } + const fail = (error: Error): void => { + if (settled) { + return + } + settled = true + if (timeout) { + clearTimeout(timeout) + } + args.signal?.removeEventListener('abort', abort) + reject(error) + } const abort = (): void => { if (settled) { return @@ -62,7 +90,11 @@ export async function runRecipeCommand(args: { killRecipeProcess(child) } - args.signal?.addEventListener('abort', abort, { once: true }) + if (args.signal?.aborted) { + abort() + } else { + args.signal?.addEventListener('abort', abort, { once: true }) + } child.stdout.setEncoding('utf8') child.stderr.setEncoding('utf8') @@ -75,16 +107,24 @@ export async function runRecipeCommand(args: { args.onStderr?.(chunk) }) child.on('error', (error) => { - settled = true - args.signal?.removeEventListener('abort', abort) - reject(error) + fail(error) }) child.on('close', (exitCode, signal) => { - settled = true - args.signal?.removeEventListener('abort', abort) - resolve({ stdout, stderr, exitCode, signal }) + finish({ stdout, stderr, exitCode, signal }) }) + if (args.timeoutMs !== undefined) { + timeout = setTimeout(() => { + finish({ stdout, stderr, exitCode: null, signal: null, timedOut: true }) + killRecipeProcess(child, true) + child.stdin.destroy() + child.stdout.destroy() + child.stderr.destroy() + child.unref() + }, args.timeoutMs) + timeout.unref() + } + if (args.stdin) { child.stdin.end(args.stdin) } else { @@ -93,7 +133,8 @@ export async function runRecipeCommand(args: { }) } -function killRecipeProcess(child: ChildProcessWithoutNullStreams): void { +function killRecipeProcess(child: ChildProcessWithoutNullStreams, force = false): void { + const signal = force ? 'SIGKILL' : 'SIGTERM' if (process.platform === 'win32') { // Recipes run through `cmd.exe /c` (shell: true), so child.kill() would only // terminate the wrapper and orphan the actual recipe subprocess (e.g. a cloud @@ -103,22 +144,22 @@ function killRecipeProcess(child: ChildProcessWithoutNullStreams): void { windowsHide: true, stdio: 'ignore' }) - killer.on('error', () => child.kill()) + killer.on('error', () => child.kill(signal)) return } - child.kill() + child.kill(signal) return } if (child.pid) { try { // Recipes run through a shell; kill the process group so shell children do not linger. - process.kill(-child.pid, 'SIGTERM') + process.kill(-child.pid, signal) return } catch { // Fall back to killing the direct child if the process group is already gone. } } - child.kill() + child.kill(signal) } function buildRecipeEnv( diff --git a/src/shared/ephemeral-vm-recipe-runner.ts b/src/shared/ephemeral-vm-recipe-runner.ts index 60d6545abf7..88512ceed62 100644 --- a/src/shared/ephemeral-vm-recipe-runner.ts +++ b/src/shared/ephemeral-vm-recipe-runner.ts @@ -8,6 +8,10 @@ import { getEphemeralVmRecipeResultSchemaVersion } from './ephemeral-vm-recipe-checkout-mode' import { runRecipeCommand } from './ephemeral-vm-recipe-process' +import { + EPHEMERAL_VM_RECIPE_DESTROY_TIMEOUT_MS, + getEphemeralVmRecipeDestroyFailure +} from './ephemeral-vm-recipe-destroy-result' import { buildEphemeralVmRecipeCleanupPayload, buildEphemeralVmRecipeLifecyclePayload @@ -74,6 +78,7 @@ export type EphemeralVmRecipeCleanupArgs = { recipeResult: EphemeralVmRecipeResult env?: NodeJS.ProcessEnv maxCaptureBytes?: number + timeoutMs?: number signal?: AbortSignal onStdout?: (chunk: string) => void onStderr?: (chunk: string) => void @@ -169,6 +174,7 @@ export async function runEphemeralVmRecipeCleanup( } const payload = buildEphemeralVmRecipeCleanupPayload(args) + const timeoutMs = args.timeoutMs ?? EPHEMERAL_VM_RECIPE_DESTROY_TIMEOUT_MS const processResult = await runRecipeCommand({ command: args.recipe.destroy, repoPath: args.repoPath, @@ -178,19 +184,16 @@ export async function runEphemeralVmRecipeCleanup( stdin: `${JSON.stringify(payload)}\n`, env: args.env, maxCaptureBytes: args.maxCaptureBytes, + timeoutMs, signal: args.signal, onStdout: args.onStdout, onStderr: args.onStderr, spawnCommand: args.spawnCommand }) - if (processResult.exitCode !== 0) { - return { - ok: false, - skipped: false, - error: `Destroy exited with code ${processResult.exitCode ?? 'unknown'}.`, - ...processResult - } + const failure = getEphemeralVmRecipeDestroyFailure(processResult, timeoutMs) + if (failure) { + return failure } return { ok: true, skipped: false, ...processResult }