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 }) + } +})