From 9bfabff0caf0f23de0ee2fe1c09972d3aeeb1ccc Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 26 Oct 2026 15:14:23 -0700 Subject: [PATCH] fix(runtime-environments): drop a recipe VM's rows and session partition when cleanup forgets its server (#33805) (#34101) Recipe cleanup removed the saved server without the retirement a Settings removal runs, so the host's session partition stayed (listings kept it under omittedHostIds) and the sidebar kept the VM's primary checkout until the next catalog read. Cleanup now retires the server through the same routine as a Settings removal, the renderer re-reads the catalog once cleanup returns, a catalog read that started before a change is followed by one more, and registry edits made outside Settings (CLI environment rm, recipe cleanup) also drop the removed host's session partitions. --- .../runtime-environment-release.test.ts | 33 +++++---- .../runtime-environment-release.ts | 9 +-- ...hemeral-vm-runtime-handler-cleanup.test.ts | 68 +++++++++++++++++++ src/main/ipc/ephemeral-vm-runtime-handlers.ts | 10 ++- .../runtime-environments.ts | 10 +-- .../lib/ephemeral-vm-runtime-cleanup.test.ts | 19 ++++++ .../src/lib/ephemeral-vm-runtime-cleanup.ts | 8 +++ ...untime-environment-pairing-refresh.test.ts | 29 ++++++++ .../runtime-environment-pairing-refresh.ts | 11 ++- 9 files changed, 173 insertions(+), 24 deletions(-) diff --git a/src/main/ephemeral-vm/runtime-environment-release.test.ts b/src/main/ephemeral-vm/runtime-environment-release.test.ts index efba558d388..d74f0a034db 100644 --- a/src/main/ephemeral-vm/runtime-environment-release.test.ts +++ b/src/main/ephemeral-vm/runtime-environment-release.test.ts @@ -8,10 +8,6 @@ const mocks = vi.hoisted(() => ({ closeTunnel: vi.fn(async () => {}) })) -vi.mock('../runtime/desktop-chat-creation-history', () => ({ - desktopChatCreationHistory: () => ({ removeEnvironment: mocks.retire }) -})) - vi.mock('../../shared/runtime-environment-store', () => ({ listEnvironments: mocks.list, removeEnvironment: mocks.remove @@ -35,10 +31,11 @@ describe('releaseEphemeralVmRuntimeEnvironments', () => { { id: 'env-vm', orcadDeployment: { sshTargetId: 'runtime-ssh-vm' } }, { id: 'env-other', orcadDeployment: { sshTargetId: 'ssh-builder' } } ]) - await releaseEphemeralVmRuntimeEnvironments('/user-data', { - runtimeEnvironmentId: 'env-vm', - sshTargetId: 'runtime-ssh-vm' - }) + await releaseEphemeralVmRuntimeEnvironments( + '/user-data', + { runtimeEnvironmentId: 'env-vm', sshTargetId: 'runtime-ssh-vm' }, + mocks.retire + ) expect(mocks.closeTunnel).toHaveBeenCalledTimes(1) expect(mocks.closeTunnel).toHaveBeenCalledWith('env-vm') expect(mocks.removeManaged).toHaveBeenCalledTimes(1) @@ -54,15 +51,21 @@ describe('releaseEphemeralVmRuntimeEnvironments', () => { mocks.list.mockReturnValue([ { id: 'env-converted', orcadDeployment: { sshTargetId: 'runtime-ssh-vm' } } ]) - await releaseEphemeralVmRuntimeEnvironments('/user-data', { sshTargetId: 'runtime-ssh-vm' }) + await releaseEphemeralVmRuntimeEnvironments( + '/user-data', + { sshTargetId: 'runtime-ssh-vm' }, + mocks.retire + ) expect(mocks.removeManaged).toHaveBeenCalledWith('/user-data', 'env-converted') }) it('removes a paired recipe server the ordinary way', async () => { mocks.list.mockReturnValue([{ id: 'env-paired' }]) - await releaseEphemeralVmRuntimeEnvironments('/user-data', { - runtimeEnvironmentId: 'env-paired' - }) + await releaseEphemeralVmRuntimeEnvironments( + '/user-data', + { runtimeEnvironmentId: 'env-paired' }, + mocks.retire + ) expect(mocks.remove).toHaveBeenCalledWith('/user-data', 'env-paired') expect(mocks.retire).toHaveBeenCalledWith('env-paired') expect(mocks.remove.mock.invocationCallOrder[0]).toBeLessThan( @@ -73,7 +76,11 @@ describe('releaseEphemeralVmRuntimeEnvironments', () => { it('ignores a server that is already gone', async () => { mocks.list.mockReturnValue([]) - await releaseEphemeralVmRuntimeEnvironments('/user-data', { runtimeEnvironmentId: 'env-gone' }) + await releaseEphemeralVmRuntimeEnvironments( + '/user-data', + { runtimeEnvironmentId: 'env-gone' }, + mocks.retire + ) expect(mocks.remove).not.toHaveBeenCalled() expect(mocks.removeManaged).not.toHaveBeenCalled() expect(mocks.retire).not.toHaveBeenCalled() diff --git a/src/main/ephemeral-vm/runtime-environment-release.ts b/src/main/ephemeral-vm/runtime-environment-release.ts index 31f3299703c..c2dbe4ce7e5 100644 --- a/src/main/ephemeral-vm/runtime-environment-release.ts +++ b/src/main/ephemeral-vm/runtime-environment-release.ts @@ -1,4 +1,3 @@ -import { desktopChatCreationHistory } from '../runtime/desktop-chat-creation-history' import type { EphemeralVmRuntimeRecord } from '../../shared/ephemeral-vm-runtimes' import { removeManagedOrcadEnvironment } from '../../shared/runtime-environment-managed-orcad-store' import { listEnvironments, removeEnvironment } from '../../shared/runtime-environment-store' @@ -7,7 +6,9 @@ import { getOrcadSshDeployment } from '../../shared/runtime-environments' /** Remove destroyed VM servers so managed tunnels stop reconnecting. */ export async function releaseEphemeralVmRuntimeEnvironments( userDataPath: string, - runtime: Pick + runtime: Pick, + // Same retirement as a Settings removal, so the host's session partition and transport go too. + retireRemovedEnvironment: (environmentId: string) => void ): Promise { // Relay-era conversions may have recorded only the SSH target, not the server ID. const environments = listEnvironments(userDataPath).filter( @@ -19,13 +20,13 @@ export async function releaseEphemeralVmRuntimeEnvironments( for (const environment of environments) { if (!environment.orcadDeployment) { removeEnvironment(userDataPath, environment.id) - void desktopChatCreationHistory(userDataPath).removeEnvironment(environment.id) + retireRemovedEnvironment(environment.id) continue } const { closeOrcadManagedTunnel } = await import('../managed-server/lifecycle/orcad-managed-tunnel') await closeOrcadManagedTunnel(environment.id).catch(() => undefined) removeManagedOrcadEnvironment(userDataPath, environment.id) - void desktopChatCreationHistory(userDataPath).removeEnvironment(environment.id) + retireRemovedEnvironment(environment.id) } } 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 0dbef129556..2a271058a9b 100644 --- a/src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts +++ b/src/main/ipc/ephemeral-vm-runtime-handler-cleanup.test.ts @@ -3,6 +3,7 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, expect, it, vi } from 'vitest' import { upsertEphemeralVmRuntime } from '../../shared/ephemeral-vm-runtime-store' +import type * as RuntimeEnvironmentStore from '../../shared/runtime-environment-store' const handlers = new Map unknown>() const { getPathMock, handleMock, removeRuntimeOwnedSshTargetMock, removeHandlerMock } = vi.hoisted( @@ -19,6 +20,21 @@ vi.mock('electron', () => ({ ipcMain: { handle: handleMock, removeHandler: removeHandlerMock } })) +const { listEnvironmentsMock, removeEnvironmentMock } = vi.hoisted(() => ({ + listEnvironmentsMock: vi.fn((): { id: string }[] => []), + removeEnvironmentMock: vi.fn() +})) + +vi.mock('../../shared/runtime-environment-store', async (importOriginal) => ({ + ...(await importOriginal()), + listEnvironments: listEnvironmentsMock, + removeEnvironment: removeEnvironmentMock +})) + +vi.mock('./runtime-environments/runtime-environments', () => ({ + invalidateRuntimeEnvironmentTransport: vi.fn(async () => {}) +})) + vi.mock('../ephemeral-vm/runtime-ssh', () => ({ connectRuntimeOwnedSshTarget: vi.fn(), disconnectRuntimeOwnedSshTarget: vi.fn(), @@ -148,3 +164,55 @@ it('stops in-flight cleanup and retains the runtime for retry', async () => { }) await expect(cleanup).resolves.toMatchObject({ status: 'cleanup_failed' }) }) + +it("retires a recipe server's session partition once its VM is destroyed (#33805)", async () => { + const userDataPath = mkdtempSync(join(tmpdir(), 'orca-vm-runtime-handler-')) + const repoPath = mkdtempSync(join(tmpdir(), 'orca-vm-runtime-repo-')) + tempDirs.push(userDataPath, repoPath) + getPathMock.mockReturnValue(userDataPath) + const destroyPath = join(repoPath, 'destroy.js') + writeFileSync(destroyPath, 'process.exit(0)') + listEnvironmentsMock.mockReturnValue([{ id: 'env-recipe' }]) + upsertEphemeralVmRuntime(userDataPath, { + id: 'runtime-paired', + recipeId: 'cloud-sandbox', + recipe: { + id: 'cloud-sandbox', + name: 'Cloud Sandbox', + create: 'unused', + destroy: nodeCommand(destroyPath) + }, + repoId: 'repo-1', + workspaceId: 'test1', + runtimeEnvironmentId: 'env-recipe', + status: 'running', + cleanupStatus: 'not_started', + createdAt: 1, + updatedAt: 1, + recipeResult: { + schemaVersion: 1, + connection: { type: 'orca-server', pairingCode: 'unused', projectRoot: '/workspace/repo' } + } + }) + const removeWorkspaceSessionHost = vi.fn() + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: cleanup reads only getRepo and the removed-server store methods this fake implements. + registerEphemeralVmRuntimeHandlers({ + getRepo: vi.fn(() => ({ + id: 'repo-1', + path: repoPath, + displayName: 'Repo', + badgeColor: '#000', + addedAt: 0 + })), + removeWorkspaceSessionHost, + getRemoteLayoutSnapshot: vi.fn(), + setRemoteLayoutSnapshot: vi.fn() + } as never) + + await expect( + handlers.get('ephemeralVm:cleanup')?.(null, { runtimeId: 'runtime-paired' }) + ).resolves.toMatchObject({ cleanupStatus: 'succeeded' }) + + expect(removeEnvironmentMock).toHaveBeenCalledWith(userDataPath, 'env-recipe') + expect(removeWorkspaceSessionHost).toHaveBeenCalledWith('runtime:env-recipe') +}) diff --git a/src/main/ipc/ephemeral-vm-runtime-handlers.ts b/src/main/ipc/ephemeral-vm-runtime-handlers.ts index 898e997259a..fada16a9e16 100644 --- a/src/main/ipc/ephemeral-vm-runtime-handlers.ts +++ b/src/main/ipc/ephemeral-vm-runtime-handlers.ts @@ -27,6 +27,7 @@ import { } from '../ephemeral-vm/runtime-ssh' import { getRuntimeRecipeContext } from './ephemeral-vm-recipe-context' import { invalidateRuntimeEnvironmentTransport } from './runtime-environments/runtime-environments' +import { retireRemovedRuntimeEnvironment } from './runtime-environments/runtime-environment-removal-cleanup' import { attachEphemeralVmRuntimeToWorkspace } from '../ephemeral-vm/runtime-attachment' export type EphemeralVmCleanupCommandResult = { @@ -102,7 +103,14 @@ export function registerEphemeralVmRuntimeHandlers(store: Store): void { } if (result.ok) { try { - await releaseEphemeralVmRuntimeEnvironments(userDataPath, runtime) + await releaseEphemeralVmRuntimeEnvironments(userDataPath, runtime, (environmentId) => { + void retireRemovedRuntimeEnvironment( + environmentId, + invalidateRuntimeEnvironmentTransport, + store, + userDataPath + ) + }) } catch { // Cleanup of provider resources matters more than hiding a stale local // environment row; users can still remove that manually. diff --git a/src/main/ipc/runtime-environments/runtime-environments.ts b/src/main/ipc/runtime-environments/runtime-environments.ts index 253e18fb639..118ab3ab940 100644 --- a/src/main/ipc/runtime-environments/runtime-environments.ts +++ b/src/main/ipc/runtime-environments/runtime-environments.ts @@ -117,11 +117,11 @@ let stopPreferenceWatch: (() => void) | undefined export function registerRuntimeEnvironmentHandlers(store: Store): void { stopPreferenceWatch?.() - stopPreferenceWatch = watchRuntimeEnvironmentPreference( - store, - getUserDataPath(), - publishRuntimeEnvironmentCatalogChanged - ) + stopPreferenceWatch = watchRuntimeEnvironmentPreference(store, getUserDataPath(), () => { + // CLI and recipe removals skip the Settings path; drop their session partitions here too. + reconcileOrphanedRuntimeSessions(store, getUserDataPath()) + publishRuntimeEnvironmentCatalogChanged() + }) for (const pending of pendingSubscriptions.values()) { pending.close() } 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 86f8e4c6dbf..5fb01b2febc 100644 --- a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts +++ b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts @@ -8,6 +8,7 @@ globalThis.window = { api: { ephemeralVm: { listRuntimes, cleanup } } } import { cleanupEphemeralVmRuntimesForDeleted } from './ephemeral-vm-runtime-cleanup' import { toSshExecutionHostId } from '../../../shared/execution-host' +import { setRuntimeEnvironmentCatalogRefresher } from '@/runtime/runtime-environment-pairing-refresh' function runtime(overrides: Record): Record { return { id: 'rt', cleanupStatus: 'not_started', ...overrides } @@ -35,6 +36,24 @@ describe('cleanupEphemeralVmRuntimesForDeleted', () => { }) }) + it("re-reads the server catalog once a cleanup may have forgotten the VM's server (#33805)", async () => { + listRuntimes.mockResolvedValue([ + runtime({ id: 'rt-a', workspaceId: 'wt-1', runtimeEnvironmentId: 'env-a' }) + ]) + const refresher = vi.fn(async () => {}) + setRuntimeEnvironmentCatalogRefresher(refresher) + try { + await cleanupEphemeralVmRuntimesForDeleted({ workspaceIds: ['wt-1'] }) + } finally { + setRuntimeEnvironmentCatalogRefresher(null) + } + + expect(refresher).toHaveBeenCalledTimes(1) + expect(refresher.mock.invocationCallOrder[0]).toBeGreaterThan( + cleanup.mock.invocationCallOrder[0] + ) + }) + it('cleans only the confirmed host runtime when another host owns the same workspace id', async () => { listRuntimes.mockResolvedValue([ runtime({ diff --git a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts index 70d3204dc72..f03ecc82a88 100644 --- a/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts +++ b/src/renderer/src/lib/ephemeral-vm-runtime-cleanup.ts @@ -5,6 +5,7 @@ import { type ExecutionHostId } from '../../../shared/execution-host' import { composeWorktreeHostIdentity } from '../../../shared/worktree/host-qualified-identity' +import { refreshRuntimeEnvironmentCatalog } from '@/runtime/runtime-environment-pairing-refresh' /** Destroy runtimes for removed workspaces and runtime-owned SSH targets. Share with project removal because SSH-mode main worktrees delete through that path; classify targets so callers purge only confirmed destruction. */ export type EphemeralVmCleanupSummary = { @@ -56,9 +57,12 @@ export async function cleanupEphemeralVmRuntimesForDeleted(args: { )))) || (runtime.sshTargetId !== undefined && sshTargetIdSet.has(runtime.sshTargetId))) ) + let releasedServer = false for (const runtime of matchingRuntimes) { try { const cleaned = await window.api.ephemeralVm.cleanup({ runtimeId: runtime.id }) + releasedServer ||= + runtime.runtimeEnvironmentId !== undefined || runtime.sshTargetId !== undefined if (runtime.sshTargetId) { const targetIds = cleaned.sshTargetId ? retainedSshTargetIds : destroyedSshTargetIds targetIds.add(runtime.sshTargetId) @@ -70,6 +74,10 @@ export async function cleanupEphemeralVmRuntimesForDeleted(args: { } } } + if (releasedServer) { + // Cleanup can forget the VM's server; purge its rows now instead of on the next catalog read. + await refreshRuntimeEnvironmentCatalog() + } } catch (error) { console.error('Failed to clean up ephemeral VM runtime for deleted workspace:', error) for (const targetId of args.runtimeOwnedSshTargetIds ?? []) { diff --git a/src/renderer/src/runtime/runtime-environment-pairing-refresh.test.ts b/src/renderer/src/runtime/runtime-environment-pairing-refresh.test.ts index f8789d69284..8487f805d57 100644 --- a/src/renderer/src/runtime/runtime-environment-pairing-refresh.test.ts +++ b/src/renderer/src/runtime/runtime-environment-pairing-refresh.test.ts @@ -6,6 +6,7 @@ import { replaceRuntimeEnvironmentRevisions } from './runtime-environment-revision' import { + refreshRuntimeEnvironmentCatalog, setRuntimeEnvironmentCatalogRefresher, subscribeRuntimeEnvironment } from './runtime-environment-pairing-refresh' @@ -93,3 +94,31 @@ describe('a pairing rotation under a live subscriber', () => { await expect(request()).resolves.toMatchObject({ ok: true }) }) }) + +describe('catalog refresh coalescing', () => { + it('reads again after an in-flight read that may predate the change', async () => { + let catalog = ['env-1', 'env-recipe'] + const published: string[][] = [] + const releaseFirstRead = Promise.withResolvers() + let reads = 0 + setRuntimeEnvironmentCatalogRefresher(async () => { + const snapshot = [...catalog] + reads += 1 + if (reads === 1) { + await releaseFirstRead.promise + } + published.push(snapshot) + }) + + const first = refreshRuntimeEnvironmentCatalog() + // Recipe cleanup forgets the server while the earlier read is still out. + catalog = ['env-1'] + const afterRemoval = refreshRuntimeEnvironmentCatalog() + const sameBurst = refreshRuntimeEnvironmentCatalog() + releaseFirstRead.resolve() + await Promise.all([first, afterRemoval, sameBurst]) + + expect(reads).toBe(2) + expect(published.at(-1)).toEqual(['env-1']) + }) +}) diff --git a/src/renderer/src/runtime/runtime-environment-pairing-refresh.ts b/src/renderer/src/runtime/runtime-environment-pairing-refresh.ts index b27bcfb80c2..56c82192840 100644 --- a/src/renderer/src/runtime/runtime-environment-pairing-refresh.ts +++ b/src/renderer/src/runtime/runtime-environment-pairing-refresh.ts @@ -5,6 +5,7 @@ type CatalogRefresher = () => Promise let refreshCatalog: CatalogRefresher | null = null let inFlight: Promise | null = null +let trailing: Promise | null = null export function setRuntimeEnvironmentCatalogRefresher(refresher: CatalogRefresher | null): void { refreshCatalog = refresher @@ -31,7 +32,15 @@ export function refreshRuntimeEnvironmentCatalog(): Promise { if (!refreshCatalog) { return Promise.resolve() } - inFlight ??= refreshCatalog() + if (inFlight) { + // The running read may predate this caller's change, so queue one more for the whole burst. + trailing ??= inFlight.then(() => { + trailing = null + return refreshRuntimeEnvironmentCatalog() + }) + return trailing + } + inFlight = refreshCatalog() .catch((refreshError: unknown) => { console.warn( '[runtime-environments] Could not re-read a re-paired environment:',