fix(terminal): verify provider shutdown before purge

This commit is contained in:
Brennan Benson
2026-07-12 14:45:25 -07:00
parent d87c8fa9af
commit 8024166e6d
10 changed files with 226 additions and 36 deletions
+37 -5
View File
@@ -3600,7 +3600,7 @@
"providers": ["ssh", "remote-runtime"],
"coveredPlatforms": ["macos"],
"coveredProviders": ["ssh"],
"coverageNotes": "A macOS unit harness loads the installed patched node-pty WindowsTerminal prototype without constructing native state, proving pre-ready kill/destroy dispatch immediately. A relay artifact harness reproduces vanilla npm node-pty retaining one deferred kill with zero native close calls, then proves both Windows relay artifacts hash, ship, apply, include the local ConPTY console-list fallback, and idempotently verify the fix. Relay tests simulate win32 shutdown, stale-spawn fallback ownership, one-shot ownership, and retry after native failure. Renderer tests retain exact local/SSH kill identity across rejection and retry, while project removal keeps local or remote ownership state until provider shutdown succeeds. The same relay contract serves paired web/mobile clients; no live Windows ConPTY or Windows-over-SSH run is recorded yet.",
"coverageNotes": "A macOS unit harness loads the installed patched node-pty WindowsTerminal prototype without constructing native state, proving pre-ready kill/destroy dispatch immediately. A relay artifact harness reproduces vanilla npm node-pty retaining one deferred kill with zero native close calls, then proves both Windows relay artifacts hash, ship, apply, include the local ConPTY console-list fallback, and idempotently verify the fix. Relay tests simulate win32 shutdown, repeated stale-spawn native failure, one-shot ownership, and retry after native failure. Renderer tests retain exact local/SSH kill identity across rejection and retry, while project removal keeps local or remote ownership state until provider-verified shutdown and VM cleanup succeed. Runtime tests prove terminal.stop awaits stopAndWait plus a fresh post-stop liveness check. The same relay contract serves paired web/mobile clients; no live Windows ConPTY or Windows-over-SSH run is recorded yet.",
"motivatingLinks": [
"https://github.com/stablyai/orca/issues/8260",
"https://github.com/stablyai/orca/pull/8279"
@@ -3608,7 +3608,8 @@
"invariant": "A Windows relay PTY closed before its first output executes exactly one no-argument node-pty shutdown immediately; Orca does not delete ownership for a native kill failure, and a later retry can still target the same managed PTY.",
"oracle": "Load the installed patched WindowsTerminal prototype in a pre-ready state and assert kill and destroy each close the terminal and native agent once with zero deferred work, while signal arguments still throw. Build both Windows SSH relay artifacts, reproduce vanilla npm node-pty with close=0/native-kill=0/deferred=1, apply the shipped patch, and assert close=1/native-kill=1/deferred=0, AttachConsole failure falls back to the shell pid, and the artifact version derives from all patch bytes. Assert remote npm install executes patch enforcement before its runtime probe and never finalizes a failed patch. In the relay, assert immediate, graceful, stale-spawn fallback, and handler-dispose paths preserve ownership through native throw and issue at most one successful no-argument shutdown. In the renderer, reject the first kill, retain exact identity, and assert the next lifecycle/removal retry succeeds before ownership state is purged.",
"commands": [
"pnpm exec vitest run --config config/vitest.config.ts config/scripts/build-relay-windows-node-pty-patch.test.mjs src/main/ssh/ssh-relay-native-deps-install.test.ts src/relay/node-pty-windows-pre-ready-shutdown.test.ts src/relay/pty-handler.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts src/renderer/src/store/slices/repos.test.ts"
"pnpm exec vitest run --config config/vitest.config.ts config/scripts/build-relay-windows-node-pty-patch.test.mjs src/main/ssh/ssh-relay-native-deps-install.test.ts src/relay/node-pty-windows-pre-ready-shutdown.test.ts src/relay/pty-handler.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts src/renderer/src/store/slices/repos.test.ts src/renderer/src/store/slices/repos-runtime-vm-removal.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts",
"pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts"
],
"testFiles": [
"config/scripts/build-relay-windows-node-pty-patch.test.mjs",
@@ -3617,7 +3618,10 @@
"src/relay/pty-handler.test.ts",
"src/renderer/src/components/terminal-pane/pty-transport.test.ts",
"src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts",
"src/renderer/src/store/slices/repos.test.ts"
"src/renderer/src/store/slices/repos.test.ts",
"src/renderer/src/store/slices/repos-runtime-vm-removal.test.ts",
"src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts",
"src/main/runtime/orca-runtime.test.ts"
],
"assertionRefs": [
{
@@ -3667,6 +3671,25 @@
"assertions": [
"remote-runtime project removal retains terminal ownership after stop rejection and purges after retry succeeds"
]
},
{
"file": "src/renderer/src/store/slices/repos-runtime-vm-removal.test.ts",
"assertions": [
"runtime-owned SSH project removal retains the project when VM cleanup fails"
]
},
{
"file": "src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts",
"assertions": [
"project-removal cleanup propagates VM teardown failure instead of orphaning a hidden runtime"
]
},
{
"file": "src/main/runtime/orca-runtime.test.ts",
"assertions": [
"terminal.stop awaits stopAndWait and fresh post-stop liveness before acknowledging provider shutdown",
"provider shutdown rejection fails terminal.stop closed"
]
}
],
"evidenceRuns": [
@@ -3674,10 +3697,19 @@
"date": "2026-07-12",
"runner": "local",
"platform": "macos",
"command": "pnpm exec vitest run --config config/vitest.config.ts config/scripts/build-relay-windows-node-pty-patch.test.mjs src/main/ssh/ssh-relay-native-deps-install.test.ts src/relay/node-pty-windows-pre-ready-shutdown.test.ts src/relay/pty-handler.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts src/renderer/src/store/slices/repos.test.ts",
"command": "pnpm exec vitest run --config config/vitest.config.ts config/scripts/build-relay-windows-node-pty-patch.test.mjs src/main/ssh/ssh-relay-native-deps-install.test.ts src/relay/node-pty-windows-pre-ready-shutdown.test.ts src/relay/pty-handler.test.ts src/renderer/src/components/terminal-pane/pty-transport.test.ts src/renderer/src/store/slices/repos-remove-project-purge-leak.test.ts src/renderer/src/store/slices/repos.test.ts src/renderer/src/store/slices/repos-runtime-vm-removal.test.ts src/renderer/src/lib/ephemeral-vm-runtime-cleanup.test.ts",
"result": "passed",
"durationSeconds": 2,
"summary": "193 tests passed, including built Windows relay red/green and ConPTY fallback parity, strict remote install ordering/failure/upgrade-repair handling, installed-dependency pre-ready assertions, stale-spawn fallback ownership, renderer retained retry identity, and local/remote project removal fail-closed ownership coverage."
"summary": "200 tests passed, including built Windows relay red/green and ConPTY fallback parity, repeated stale-spawn throw containment, renderer retained retry identity, local/remote project removal fail-closed ownership, and VM cleanup propagation."
},
{
"date": "2026-07-12",
"runner": "local",
"platform": "macos",
"command": "pnpm exec vitest run --config config/vitest.config.ts src/main/runtime/orca-runtime.test.ts",
"result": "passed",
"durationSeconds": 10,
"summary": "662 runtime tests passed, including provider-verified terminal.stop acceptance and post-stop liveness coverage."
}
],
"runtimeBudget": {
+49 -7
View File
@@ -22202,6 +22202,45 @@ describe('OrcaRuntimeService', () => {
expect(killed).toBe(false)
})
it('awaits provider shutdown and verifies liveness before terminal.stop returns', async () => {
const runtime = new OrcaRuntimeService(store)
const stopped: string[] = []
const processLists = [[{ id: 'pty-1', cwd: '/tmp/worktree-a', title: 'Shell' }], []]
runtime.setPtyController({
write: () => true,
kill: () => false,
stopAndWait: async (ptyId) => {
stopped.push(ptyId)
runtime.onPtyExit(ptyId, -1)
return true
},
getForegroundProcess: async () => null,
listProcesses: async () => processLists.shift() ?? []
})
syncSinglePty(runtime, 'pty-1')
await expect(runtime.stopTerminalsForWorktree('id:repo-1::/tmp/worktree-a')).resolves.toEqual({
stopped: 1
})
expect(stopped).toEqual(['pty-1'])
})
it('rejects terminal.stop when provider shutdown is not accepted', async () => {
const runtime = new OrcaRuntimeService(store)
runtime.setPtyController({
write: () => true,
kill: () => false,
stopAndWait: async () => false,
getForegroundProcess: async () => null,
listProcesses: async () => [{ id: 'pty-1', cwd: '/tmp/worktree-a', title: 'Shell' }]
})
syncSinglePty(runtime, 'pty-1')
await expect(runtime.stopTerminalsForWorktree('id:repo-1::/tmp/worktree-a')).rejects.toThrow(
'terminal_verified_stop_failed'
)
})
it('stops exactly the expected live PTYs for a worktree', async () => {
const runtime = new OrcaRuntimeService(store)
const stopped: string[] = []
@@ -29226,7 +29265,8 @@ describe('OrcaRuntimeService', () => {
it('RPC-initiated delete kills matching PTYs before git', async () => {
// Seed the runtime with a live leaf whose worktreeId matches the target.
const killSpy = vi.fn().mockReturnValue(true)
const stopSpy = vi.fn().mockResolvedValue(true)
const processLists = [[{ id: 'pty-1', cwd: TEST_WORKTREE_PATH, title: 'Shell' }], []]
const localProvider = createProviderStub(async () => [])
const callOrder: string[] = []
vi.mocked(assertWorktreeCleanForRemoval).mockImplementation(async () => {
@@ -29245,21 +29285,23 @@ describe('OrcaRuntimeService', () => {
})
runtime.setPtyController({
write: () => true,
kill: (id) => {
callOrder.push(`kill:${id}`)
return killSpy(id) as boolean
kill: () => false,
stopAndWait: async (id) => {
callOrder.push(`stop:${id}`)
return stopSpy(id) as Promise<boolean>
},
getForegroundProcess: async () => null
getForegroundProcess: async () => null,
listProcesses: async () => processLists.shift() ?? []
})
syncSinglePty(runtime, 'pty-1')
await runtime.removeManagedWorktree(TEST_WORKTREE_ID)
expect(killSpy).toHaveBeenCalledWith('pty-1')
expect(stopSpy).toHaveBeenCalledWith('pty-1')
// The provider-prefix sweep and the git removal must happen AFTER the
// runtime-graph kill. Git removal must NOT happen before any kill.
const preflightIdx = callOrder.indexOf('preflight')
const killIdx = callOrder.indexOf('kill:pty-1')
const killIdx = callOrder.indexOf('stop:pty-1')
const gitIdx = callOrder.indexOf('git-removeWorktree')
expect(preflightIdx).toBeGreaterThanOrEqual(0)
expect(killIdx).toBeGreaterThan(preflightIdx)
+28 -14
View File
@@ -18734,25 +18734,39 @@ export class OrcaRuntimeService {
const graphEpoch = this.captureReadyGraphEpoch()
const worktree = await this.resolveWorktreeSelector(worktreeSelector)
this.assertStableReadyGraph(graphEpoch)
const ptyIds = new Set<string>()
for (const leaf of this.leaves.values()) {
if (leaf.worktreeId === worktree.id && leaf.ptyId) {
ptyIds.add(leaf.ptyId)
}
const resolvedWorktrees = [...(await this.getResolvedWorktreeMap()).values()]
this.assertStableReadyGraph(graphEpoch)
const refreshedPtyLiveness =
await this.refreshPtyWorktreeRecordsFromController(resolvedWorktrees)
if (!refreshedPtyLiveness) {
throw new Error('terminal_liveness_unavailable')
}
for (const pty of this.ptysById.values()) {
if (pty.worktreeId === worktree.id && pty.connected) {
ptyIds.add(pty.ptyId)
}
const ptyIds = this.getLivePtyIdsForWorktree(worktree.id, refreshedPtyLiveness)
if (ptyIds.size === 0) {
return { stopped: 0 }
}
if (!this.ptyController?.stopAndWait) {
throw new Error('terminal_verified_stop_unavailable')
}
let stopped = 0
for (const ptyId of ptyIds) {
if (this.ptyController?.kill(ptyId)) {
stopped += 1
const stoppedPtyIds: string[] = []
for (const ptyId of [...ptyIds].sort()) {
if (!(await this.ptyController.stopAndWait(ptyId))) {
throw Object.assign(new Error('terminal_verified_stop_failed'), { ptyId })
}
stoppedPtyIds.push(ptyId)
}
return { stopped }
const postStopLiveness = await this.refreshPtyWorktreeRecordsFromController(resolvedWorktrees)
if (!postStopLiveness) {
throw new Error('terminal_stop_postcheck_unavailable')
}
const remainingLivePtyIds = this.getLivePtyIdsForWorktree(worktree.id, postStopLiveness)
if (remainingLivePtyIds.size > 0) {
throw Object.assign(new Error('terminal_stop_still_live'), {
remainingLivePtyIds: [...remainingLivePtyIds].sort()
})
}
return { stopped: stoppedPtyIds.length }
}
async stopExactTerminalsForWorktree(
+23
View File
@@ -712,6 +712,29 @@ describe('PtyHandler', () => {
expect(handler.activePtyCount).toBe(0)
})
it('contains repeated stale-spawn native kill throws and still releases ownership', async () => {
const killSpy = vi.fn(() => {
throw new Error('native ConPTY kill failed')
})
const stderr = vi.spyOn(process.stderr, 'write').mockImplementation(() => true)
const term = { ...mockPtyInstance, kill: killSpy, onData: vi.fn(), onExit: vi.fn() }
mockPtySpawn.mockReturnValue(term)
try {
await expect(
dispatcher.callRequest('pty.spawn', {}, { isStale: () => true })
).rejects.toThrow('native ConPTY kill failed')
expect(() => vi.advanceTimersByTime(5000)).not.toThrow()
expect(killSpy).toHaveBeenCalledTimes(2)
expect(handler.activePtyCount).toBe(0)
expect(stderr).toHaveBeenCalledWith(
expect.stringContaining('stale-spawn fallback kill failed')
)
} finally {
stderr.mockRestore()
}
})
it('does not submit provider-delivered commands for stale spawn responses', async () => {
const killSpy = vi.fn()
const term = { ...mockPtyInstance, kill: killSpy, onData: vi.fn(), onExit: vi.fn() }
+15 -8
View File
@@ -731,14 +731,21 @@ export class PtyHandler {
managed.killTimer = setTimeout(() => {
const still = this.ptys.get(id)
if (still && !still.disposed) {
killPtyForShutdown(still, 'SIGKILL')
// Why: stale-spawn cleanup has no client who will ever attach. If
// SIGKILL's onExit is missed (kernel edge case, uninterruptible
// sleep), the managed entry + ptmx fd would leak forever. Dispose
// synchronously so the entry is gone regardless of onExit timing.
this.notifyExitListener(still)
disposeManagedPty(still)
this.ptys.delete(id)
try {
killPtyForShutdown(still, 'SIGKILL')
} catch (error) {
process.stderr.write(
`[pty-handler] stale-spawn fallback kill failed: ${error instanceof Error ? error.message : String(error)}\n`
)
} finally {
// Why: stale-spawn cleanup has no client who will ever attach. Even
// a repeated native throw must release ownership without escaping
// the timer into relay.ts uncaughtException process termination.
this.notifyExitListener(still)
disposeManagedPty(still)
this.ptys.delete(id)
this.clearPtyFlowState(id)
}
}
}, 5000)
// Why: arm fallback ownership before native shutdown. node-pty can throw
@@ -67,4 +67,13 @@ describe('cleanupEphemeralVmRuntimesForDeleted', () => {
[]
)
})
it('fails closed when project removal requests cleanup ownership', async () => {
const error = new Error('VM cleanup unavailable')
listRuntimes.mockRejectedValue(error)
await expect(
cleanupEphemeralVmRuntimesForDeleted({ workspaceIds: ['wt-1'], throwOnError: true })
).rejects.toBe(error)
})
})
@@ -18,6 +18,8 @@ export async function cleanupEphemeralVmRuntimesForDeleted(args: {
// 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[]
/** Project removal must retain UI ownership when VM teardown fails. */
throwOnError?: boolean
}): Promise<string[]> {
const destroyedSshTargetIds: string[] = []
try {
@@ -40,6 +42,9 @@ export async function cleanupEphemeralVmRuntimesForDeleted(args: {
}
} catch (error) {
console.error('Failed to clean up ephemeral VM runtime for deleted workspace:', error)
if (args.throwOnError) {
throw error
}
}
return destroyedSshTargetIds
}
@@ -54,6 +54,8 @@ export const ptyKill: Mock = vi.fn()
export const runtimeEnvironmentCall: Mock = vi.fn()
export const runtimeEnvironmentTransportCall: Mock = vi.fn()
export const orcaProfileFindProjectProfiles: 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 {
@@ -81,6 +83,8 @@ export function installReposRuntimeRoutingHarness(): void {
orcaProfileFindProjectProfiles.mockReset()
runtimeEnvironmentCall.mockReset()
runtimeEnvironmentTransportCall.mockReset()
ephemeralVmListRuntimes.mockReset().mockResolvedValue([])
ephemeralVmCleanup.mockReset().mockResolvedValue({})
runtimeEnvironmentTransportCall.mockImplementation((args: RuntimeEnvironmentCallRequest) => {
return createCompatibleRuntimeStatusResponseIfNeeded(args) ?? runtimeEnvironmentCall(args)
})
@@ -110,7 +114,8 @@ export function installReposRuntimeRoutingHarness(): void {
findProjectProfiles: orcaProfileFindProjectProfiles
},
pty: { kill: ptyKill },
runtimeEnvironments: { call: runtimeEnvironmentTransportCall }
runtimeEnvironments: { call: runtimeEnvironmentTransportCall },
ephemeralVm: { listRuntimes: ephemeralVmListRuntimes, cleanup: ephemeralVmCleanup }
}
})
})
@@ -0,0 +1,52 @@
import { describe, expect, it, vi } from 'vitest'
import { createTestStore, makeWorktree } from './store-test-helpers'
import {
ephemeralVmCleanup,
ephemeralVmListRuntimes,
installReposRuntimeRoutingHarness,
reposRemove,
sshRepo
} from './repos-runtime-routing-fixture'
vi.mock('sonner', () => ({
toast: {
error: vi.fn(),
info: vi.fn(),
success: vi.fn(),
warning: vi.fn()
}
}))
installReposRuntimeRoutingHarness()
describe('runtime-owned project VM removal', () => {
it('retains the SSH project when VM cleanup fails', async () => {
const runtimeOwnedRepo = {
...sshRepo,
id: 'runtime-repo',
connectionId: 'runtime-ssh-orca-1'
}
const worktreeId = `${runtimeOwnedRepo.id}::/runtime/wt`
ephemeralVmListRuntimes.mockResolvedValue([
{
id: 'runtime-1',
workspaceId: worktreeId,
sshTargetId: runtimeOwnedRepo.connectionId,
cleanupStatus: 'not_started'
}
])
ephemeralVmCleanup.mockRejectedValue(new Error('VM cleanup unavailable'))
const store = createTestStore()
store.setState({
repos: [runtimeOwnedRepo],
worktreesByRepo: {
[runtimeOwnedRepo.id]: [makeWorktree({ id: worktreeId, repoId: runtimeOwnedRepo.id })]
}
})
await store.getState().removeProject(runtimeOwnedRepo.id)
expect(store.getState().repos).toEqual([runtimeOwnedRepo])
expect(reposRemove).not.toHaveBeenCalled()
})
})
+2 -1
View File
@@ -2812,7 +2812,8 @@ export const createRepoSlice: StateCreator<AppState, [], [], RepoSlice> = (set,
if (isRuntimeOwnedSshTargetId(ownerRepo.connectionId)) {
await cleanupEphemeralVmRuntimesForDeleted({
workspaceIds: worktreeIds,
runtimeOwnedSshTargetIds: [ownerRepo.connectionId as string]
runtimeOwnedSshTargetIds: [ownerRepo.connectionId as string],
throwOnError: true
})
}