From 44fabbda52f1c38411733ccac7cdc25c85a93201 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 12 Oct 2026 18:43:24 -0700 Subject: [PATCH] fix(runtime): stop a project's terminals before the host forgets the project (#28024) * fix(runtime): stop a project's terminals before the host forgets the project Removing a project on a runtime host (an SSH-managed or paired server) sent repo.rm first; the host resolves terminal.stop through the project's workspace rows, which repo.rm deletes, so the stop failed and the shells kept running on the host. Terminals stop first now. The removal toast also drops the IPC wrapper from its message. * fix(runtime): only stop host terminals when the host will accept the project removal Probe with repo.show (same resolver as repo.rm) so a removal the host rejects, such as an ambiguous project id, no longer kills the shells first. Also poll through terminal_not_writable in the paired-client e2e. * test(e2e): read the host shell pid through sh so non-POSIX login shells work --- src/renderer/src/store/repos/repo-removal.ts | 24 +-- .../repos/runtime-project-terminal-stop.ts | 38 ++++ ...ect-removal-runtime-terminal-order.test.ts | 149 ++++++++++++++ ...oject-removal-stops-host-terminals.spec.ts | 184 ++++++++++++++++++ 4 files changed, 381 insertions(+), 14 deletions(-) create mode 100644 src/renderer/src/store/repos/runtime-project-terminal-stop.ts create mode 100644 src/renderer/src/store/slices/project-removal-runtime-terminal-order.test.ts create mode 100644 tests/e2e/paired-client-project-removal-stops-host-terminals.spec.ts diff --git a/src/renderer/src/store/repos/repo-removal.ts b/src/renderer/src/store/repos/repo-removal.ts index edc55efaf8c..9f39a8f4db6 100644 --- a/src/renderer/src/store/repos/repo-removal.ts +++ b/src/renderer/src/store/repos/repo-removal.ts @@ -9,10 +9,11 @@ import { repoMatchesHostIdentity } from '../slices/repo-host-identity' import { callRuntimeRpc } from '../../runtime/runtime-rpc-client' import { hasRuntimeRpcErrorCode } from '../../runtime/runtime-rpc-result' import { ipcOrRuntimeTargetForHostId } from '../../runtime/runtime-client-target' -import { toRuntimeWorktreeSelector } from '../../runtime/runtime-worktree-selector' import { translate } from '@/i18n/i18n' +import { readIpcErrorMessage } from '@/lib/ipc-error' import { getRepoExecutionHostId, LOCAL_EXECUTION_HOST_ID } from '../../../../shared/execution-host' import { destroyRecipeVmBeforeProjectRemoval } from './recipe-vm-project-removal' +import { stopRuntimeProjectTerminalsBeforeRemoval } from './runtime-project-terminal-stop' import type { RepoSlice } from './repo-state' import { ERROR_TOAST_DURATION } from './repo-state' import { mergeProjectCompatibilityForHostRepoChange } from './repo-catalog-identity' @@ -109,6 +110,13 @@ export function createRepoRemovalActions( executionHostId: ownerHostId })) ) + if (target.kind === 'environment' && !vmServerDestroyed) { + await stopRuntimeProjectTerminalsBeforeRemoval( + target.environmentId, + projectId, + getKnownRepoWorktreeIds(get(), projectId, ownerHostId) + ) + } try { // Why: destroying the VM took its server, and the catalog holding this row, with it. if (!vmServerDestroyed) { @@ -158,18 +166,6 @@ export function createRepoRemovalActions( ] : [] const killedTabIds = new Set() - if (target.kind === 'environment') { - await Promise.allSettled( - worktreeIds.map((worktreeId) => - callRuntimeRpc( - target, - 'terminal.stop', - { worktree: toRuntimeWorktreeSelector(worktreeId) }, - { timeoutMs: 15_000 } - ) - ) - ) - } for (const wId of worktreeIds) { const tabs = get().tabsByWorktree[wId] ?? [] for (const tab of tabs) { @@ -305,7 +301,7 @@ export function createRepoRemovalActions( toast.error( translate('auto.store.slices.repos.removeProjectFailed', 'Failed to remove project'), { - description: err instanceof Error ? err.message : String(err), + description: readIpcErrorMessage(err) ?? String(err), duration: ERROR_TOAST_DURATION } ) diff --git a/src/renderer/src/store/repos/runtime-project-terminal-stop.ts b/src/renderer/src/store/repos/runtime-project-terminal-stop.ts new file mode 100644 index 00000000000..9bd75e14285 --- /dev/null +++ b/src/renderer/src/store/repos/runtime-project-terminal-stop.ts @@ -0,0 +1,38 @@ +import { callRuntimeRpc } from '../../runtime/runtime-rpc-client' +import { toRuntimeWorktreeSelector } from '../../runtime/runtime-worktree-selector' + +/** + * Stops a project's shells on its runtime host ahead of `repo.rm`: the host resolves + * `terminal.stop` through the project's workspace rows, which `repo.rm` deletes. + */ +export async function stopRuntimeProjectTerminalsBeforeRemoval( + environmentId: string, + projectId: string, + worktreeIds: readonly string[] +): Promise { + const target = { kind: 'environment', environmentId } as const + // Why: repo.show shares repo.rm's resolver, so a removal the host will reject (e.g. + // selector_ambiguous) leaves the shells running instead of killing them first. + const hostResolvesProject = await callRuntimeRpc( + target, + 'repo.show', + { repo: projectId }, + { timeoutMs: 15_000 } + ).then( + () => true, + () => false + ) + if (!hostResolvesProject) { + return + } + await Promise.allSettled( + worktreeIds.map((worktreeId) => + callRuntimeRpc( + target, + 'terminal.stop', + { worktree: toRuntimeWorktreeSelector(worktreeId) }, + { timeoutMs: 15_000 } + ) + ) + ) +} diff --git a/src/renderer/src/store/slices/project-removal-runtime-terminal-order.test.ts b/src/renderer/src/store/slices/project-removal-runtime-terminal-order.test.ts new file mode 100644 index 00000000000..441ac3c6dad --- /dev/null +++ b/src/renderer/src/store/slices/project-removal-runtime-terminal-order.test.ts @@ -0,0 +1,149 @@ +/** + * Removing a project on a runtime host (an SSH-managed or paired server) must stop its terminals + * before `repo.rm`: the host resolves `terminal.stop` through the project's workspace rows, which + * `repo.rm` deletes, so stopping afterwards failed and left the shells running on the host. + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { toast } from 'sonner' +import { createTestStore, makeWorktree } from './store-test-helpers' +import type { Repo } from '../../../../shared/repo-types' +import { + createCompatibleRuntimeStatusResponseIfNeeded, + type RuntimeEnvironmentCallRequest +} from '../../runtime/runtime-compatibility-test-fixture' +import { clearRuntimeCompatibilityCacheForTests } from '../../runtime/runtime-rpc-client' + +vi.mock('sonner', () => ({ + toast: { info: vi.fn(), success: vi.fn(), error: vi.fn() } +})) + +const remoteRepo: Repo = { + id: 'app', + path: '/home/me/app', + displayName: 'app', + badgeColor: '#000', + addedAt: 1, + executionHostId: 'runtime:env-1' +} + +const localRepo: Repo = { + id: 'app', + path: '/Users/me/app', + displayName: 'app', + badgeColor: '#000', + addedAt: 1 +} + +const hostCalls: string[] = [] +let hostKnowsProject = true +let hostSelectorAmbiguous = false +let liveHostTerminals = 0 +const removeForHost = vi.fn() + +// Mirrors the host contract: once repo.rm drops the project, its workspace selectors stop resolving. +function answerLikeRuntimeHost(args: RuntimeEnvironmentCallRequest): unknown { + const compatible = createCompatibleRuntimeStatusResponseIfNeeded(args) + if (compatible) { + return compatible + } + hostCalls.push(args.method) + const meta = { runtimeId: 'runtime-remote' } + if (hostSelectorAmbiguous && (args.method === 'repo.show' || args.method === 'repo.rm')) { + const error = { code: 'selector_ambiguous', message: 'selector_ambiguous' } + return { id: args.method, ok: false, error, _meta: meta } + } + if (args.method === 'repo.rm') { + hostKnowsProject = false + return { id: 'repo.rm', ok: true, result: { removed: true }, _meta: meta } + } + if (args.method === 'terminal.stop') { + if (!hostKnowsProject) { + const error = { code: 'selector_not_found', message: 'selector_not_found' } + return { id: 'terminal.stop', ok: false, error, _meta: meta } + } + const stopped = liveHostTerminals + liveHostTerminals = 0 + return { id: 'terminal.stop', ok: true, result: { stopped }, _meta: meta } + } + return { id: args.method, ok: true, result: {}, _meta: meta } +} + +function seedRemoteProject(): ReturnType { + const store = createTestStore() + const worktree = makeWorktree({ id: 'app::/home/me/app', repoId: 'app', hostId: 'runtime:env-1' }) + store.setState({ repos: [remoteRepo], worktreesByRepo: { app: [worktree] } }) + return store +} + +beforeEach(() => { + clearRuntimeCompatibilityCacheForTests() + vi.mocked(toast.error).mockReset() + removeForHost.mockReset() + hostCalls.length = 0 + hostKnowsProject = true + hostSelectorAmbiguous = false + liveHostTerminals = 0 + vi.stubGlobal('window', { + api: { + repos: { remove: vi.fn(), removeForHost }, + pty: { kill: vi.fn() }, + runtimeEnvironments: { call: vi.fn(answerLikeRuntimeHost) }, + ui: { set: vi.fn().mockResolvedValue(undefined) } + } + }) +}) + +describe('project removal on a runtime host', () => { + it('stops the project’s terminals before the host forgets the project', async () => { + liveHostTerminals = 2 + const store = seedRemoteProject() + + await store.getState().removeProject('app', { hostId: 'runtime:env-1' }) + + expect(hostCalls.filter((m) => m === 'terminal.stop' || m === 'repo.rm')).toEqual([ + 'terminal.stop', + 'repo.rm' + ]) + expect(liveHostTerminals).toBe(0) + expect(store.getState().repos).toEqual([]) + }) + + it('still removes the project when a workspace’s terminals cannot be stopped', async () => { + hostKnowsProject = false + const store = seedRemoteProject() + + await store.getState().removeProject('app', { hostId: 'runtime:env-1' }) + + expect(hostCalls).toContain('repo.rm') + expect(store.getState().repos).toEqual([]) + }) + + it('leaves the shells running when the host will reject the removal', async () => { + hostSelectorAmbiguous = true + liveHostTerminals = 2 + const store = seedRemoteProject() + + await store.getState().removeProject('app', { hostId: 'runtime:env-1' }) + + expect(hostCalls).not.toContain('terminal.stop') + expect(liveHostTerminals).toBe(2) + expect(store.getState().repos).toEqual([remoteRepo]) + }) +}) + +describe('project removal error toast', () => { + it('shows the host’s message without the IPC wrapper', async () => { + removeForHost.mockRejectedValue( + new Error("Error invoking remote method 'repos:removeForHost': Error: Project is busy") + ) + const store = createTestStore() + store.setState({ repos: [localRepo] }) + + await store.getState().removeProject('app', { hostId: 'local', errorFeedback: 'toast' }) + + expect(toast.error).toHaveBeenCalledWith( + 'Failed to remove project', + expect.objectContaining({ description: 'Project is busy' }) + ) + }) +}) diff --git a/tests/e2e/paired-client-project-removal-stops-host-terminals.spec.ts b/tests/e2e/paired-client-project-removal-stops-host-terminals.spec.ts new file mode 100644 index 00000000000..782e999d036 --- /dev/null +++ b/tests/e2e/paired-client-project-removal-stops-host-terminals.spec.ts @@ -0,0 +1,184 @@ +/** + * Removing a project from a paired client must stop the project's shells on the host. The host + * resolves `terminal.stop` through the project's workspace rows, so a client that sent `repo.rm` + * first left the shells running on the host after the dialog promised to close them. + * + * Run: + * ORCA_BACKGROUND_LAUNCH=1 pnpm exec playwright test \ + * tests/e2e/paired-client-project-removal-stops-host-terminals.spec.ts \ + * --config tests/playwright.config.ts --project electron-headless --workers=1 + */ +import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import type { ElectronApplication, Page } from '@stablyai/playwright-test' +import { expect, test } from './helpers/orca-app' +import { TEST_REPO_PATH_FILE } from './global-setup' +import { + createRuntimeDesktopPairingOffer, + launchPairedElectronClient, + type PairedElectronClient +} from './helpers/paired-electron-client' +import { createRestartSession } from './helpers/orca-restart' + +function seededRepoPathOrSkip(): string { + const repoPath = existsSync(TEST_REPO_PATH_FILE) + ? readFileSync(TEST_REPO_PATH_FILE, 'utf8').trim() + : '' + test.skip(!repoPath || !existsSync(repoPath), 'Global setup did not produce a seeded test repo') + return repoPath +} + +function isProcessAlive(pid: number): boolean { + try { + process.kill(pid, 0) + return true + } catch { + return false + } +} + +function readShellPid(markerPath: string): number | null { + if (!existsSync(markerPath)) { + return null + } + const pid = Number.parseInt(readFileSync(markerPath, 'utf8').trim(), 10) + return Number.isFinite(pid) && pid > 0 ? pid : null +} + +async function callRuntime( + page: Page, + environmentId: string, + method: string, + params: unknown +): Promise { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: test-only RPC result shape. + return page.evaluate( + async ({ environmentId, method, params }) => { + const response = await window.api.runtimeEnvironments.call({ + selector: environmentId, + method, + params + }) + if (!response.ok) { + throw new Error(`${response.error.code}: ${response.error.message}`) + } + return response.result + }, + { environmentId, method, params } + ) as Promise +} + +test('removing a project from a paired client stops its shells on the host', async (// oxlint-disable-next-line no-empty-pattern -- This test owns its host launch. +{}, testInfo) => { + test.skip(process.platform === 'win32', 'reads the shell pid through POSIX `sh`') + test.setTimeout(240_000) + const repoPath = seededRepoPathOrSkip() + const stage = mkdtempSync(join(tmpdir(), 'orca-project-removal-')) + const markerPath = join(stage, 'shell.pid') + const session = createRestartSession(testInfo) + let host: ElectronApplication | null = null + let client: PairedElectronClient | null = null + let shellPid: number | null = null + try { + const launched = await session.launch() + host = launched.app + client = await launchPairedElectronClient( + await createRuntimeDesktopPairingOffer(launched.page), + testInfo, + 'project-removal-host-terminals' + ) + const paired = client + const added = await callRuntime<{ repo: { id: string } }>( + paired.page, + paired.environmentId, + 'repo.add', + { path: repoPath, kind: 'git' } + ) + const findWorktreeId = (): Promise => + paired.page.evaluate( + (repoId) => + window.__store + ?.getState() + .allWorktrees() + .find((worktree) => worktree.repoId === repoId)?.id ?? null, + added.repo.id + ) + await expect + .poll(findWorktreeId, { timeout: 60_000, message: 'Paired client never saw the worktree' }) + .not.toBeNull() + const worktreeId = (await findWorktreeId()) ?? '' + + await paired.page.evaluate((id) => { + const state = window.__store?.getState() + state?.setActiveView('terminal') + state?.setActiveWorktree(id) + }, worktreeId) + const listHostTerminals = (): Promise => + callRuntime<{ terminals: { handle: string }[] }>( + paired.page, + paired.environmentId, + 'terminal.list', + { worktree: `id:${worktreeId}` } + ).then((listed) => listed.terminals.map((terminal) => terminal.handle)) + await expect + .poll(() => listHostTerminals().then((handles) => handles.length), { + timeout: 45_000, + message: 'Opening the worktree never created a host terminal' + }) + .toBeGreaterThan(0) + const [handle] = await listHostTerminals() + + // Retried because the first send can land before the shell is writable or has its prompt. + await expect + .poll( + async () => { + await callRuntime(paired.page, paired.environmentId, 'terminal.send', { + terminal: handle, + // Why sh: `$$` is not portable to fish; sh's $PPID is the pane shell in any login shell. + text: `sh -c 'echo $PPID > "${markerPath}"'`, + enter: true + }).catch(() => undefined) + return readShellPid(markerPath) + }, + { timeout: 30_000, intervals: [1_000], message: 'The host shell never reported its pid' } + ) + .not.toBeNull() + shellPid = readShellPid(markerPath) + expect(shellPid !== null && isProcessAlive(shellPid)).toBe(true) + + const hostId = await paired.page.evaluate( + (repoId) => + window.__store?.getState().repos.find((repo) => repo.id === repoId)?.executionHostId ?? + null, + added.repo.id + ) + expect(hostId).toMatch(/^runtime:/) + if (!hostId) { + throw new Error('The paired client has no host row for the project') + } + await paired.page.evaluate( + ({ repoId, hostId }) => window.__store?.getState().removeProject(repoId, { hostId }), + { repoId: added.repo.id, hostId } + ) + + await expect + .poll(() => shellPid !== null && isProcessAlive(shellPid), { + timeout: 30_000, + message: 'The host shell kept running after its project was removed' + }) + .toBe(false) + } finally { + if (shellPid !== null && isProcessAlive(shellPid)) { + process.kill(shellPid, 'SIGKILL') + } + if (client) { + await client.dispose() + } + if (host) { + await session.close(host) + } + await session.dispose() + rmSync(stage, { recursive: true, force: true }) + } +})