diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 01724348ba6..f3bb174b085 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -656,6 +656,7 @@ jobs: src/main/cli/wsl-cli-powershell-boundary.test.ts src/main/orca-profiles/profile-index-store.test.ts src/main/runtime/repo-worktree-admin-fingerprint.test.ts + src/main/runtime/runtime-rpc-worktree-rm-liveness.test.ts src/main/runtime/worktree-scan-admin-fingerprint-gate.test.ts src/shared/secure-file-fsync-flags.test.ts diff --git a/src/cli/format-recovery.test.ts b/src/cli/format-recovery.test.ts index fea52395cd8..700eee19941 100644 --- a/src/cli/format-recovery.test.ts +++ b/src/cli/format-recovery.test.ts @@ -70,4 +70,21 @@ describe('CLI error recovery', () => { expect(output).toContain('--retry-request mutation_1') expect(output).not.toContain('orca open') }) + + it('prefers worktree response-loss recovery over runtime startup advice', () => { + const error = new RuntimeClientError( + 'runtime_unavailable', + 'The workspace removal may still complete.', + { + requestPhase: 'awaiting_response', + method: 'worktree.rm', + nextSteps: ['Run: orca worktree list --json and verify state.'] + } + ) + + const output = formatCliError(error) + + expect(output).toContain('worktree list --json') + expect(output).not.toContain('orca open') + }) }) diff --git a/src/cli/format.ts b/src/cli/format.ts index 2b6b67f05e9..21743c56d9f 100644 --- a/src/cli/format.ts +++ b/src/cli/format.ts @@ -78,6 +78,10 @@ export function printResult( export function formatCliError(error: unknown, context: CliErrorContext = {}): string { const message = error instanceof Error ? error.message : String(error) if (error instanceof RuntimeClientError && error.code === 'runtime_unavailable') { + const nextSteps = nextStepsFromData(error.data) + if (nextSteps.length > 0) { + return formatMessageWithNextSteps(message, nextSteps) + } if (hasOrchestrationRequestId(error.data)) { return message } diff --git a/src/cli/runtime-client.test.ts b/src/cli/runtime-client.test.ts index 2fa6130ce3d..971aec37f14 100644 --- a/src/cli/runtime-client.test.ts +++ b/src/cli/runtime-client.test.ts @@ -441,7 +441,10 @@ describe.skipIf(process.platform === 'win32')('RuntimeClient', () => { expect((failure as RuntimeClientError).message).toBe( 'The Orca runtime closed the connection before responding. Restart Orca and try again.' ) - expect((failure as RuntimeClientError).data).toBeUndefined() + expect((failure as RuntimeClientError).data).toEqual({ + requestPhase: 'awaiting_response', + method: 'orchestration.workerShow' + }) expect(request).toMatchObject({ method: 'orchestration.workerShow' }) expect(request).not.toHaveProperty('orchestrationRequestId') }) diff --git a/src/main/runtime/rpc/transport.ts b/src/main/runtime/rpc/transport.ts index 9b762cf84f3..c786d336945 100644 --- a/src/main/runtime/rpc/transport.ts +++ b/src/main/runtime/rpc/transport.ts @@ -9,11 +9,16 @@ // Why: per-message hook bag owned by the Unix transport. `signal` aborts // when the underlying connection terminates so long-poll handlers can bail -// out. `startKeepalive` is opt-in per request — only long-poll dispatches -// call it, so short RPCs pay no timer overhead. See design doc §3.1. +// out. `startKeepalive` is opt-in per request; bounded slow dispatches keep +// admission and abort semantics separate from long polls. See design doc §3.1. +export type RpcKeepaliveBound = { + maxDurationMs: number + timeoutResponse: string +} + export type RpcMessageContext = { signal: AbortSignal - startKeepalive: (maxDurationMs?: number) => void + startKeepalive: (bounded?: RpcKeepaliveBound) => void } export type RpcTransport = { diff --git a/src/main/runtime/rpc/unix-socket-transport.test.ts b/src/main/runtime/rpc/unix-socket-transport.test.ts index 612989c0d01..91bbc927cd2 100644 --- a/src/main/runtime/rpc/unix-socket-transport.test.ts +++ b/src/main/runtime/rpc/unix-socket-transport.test.ts @@ -75,39 +75,46 @@ describe('UnixSocketTransport', () => { expect(socket.writes).toHaveLength(1) }) - it.each(['unix', 'named-pipe'] as const)( - 'returns one terminal frame when bounded %s keepalive expires', - (kind) => { - const transport = new UnixSocketTransport({ - endpoint: '/tmp/orca-runtime-rpc-test.sock', - kind, - keepaliveIntervalMs: 10 - }) - const socket = new FakeSocket() - let reply: ((response: string) => void) | undefined + it('returns one terminal frame when bounded keepalive expires', () => { + const transport = new UnixSocketTransport({ + endpoint: '/tmp/orca-runtime-rpc-test.sock', + kind: 'unix', + keepaliveIntervalMs: 10 + }) + const socket = new FakeSocket() + let reply: ((response: string) => void) | undefined - transport.onMessage((_msg, respond, context) => { - reply = respond - context?.startKeepalive(25) + transport.onMessage((_msg, respond, context) => { + reply = respond + context?.startKeepalive({ + maxDurationMs: 25, + timeoutResponse: JSON.stringify({ + id: 'slow-rm', + ok: false, + error: { + code: 'runtime_timeout', + data: { requestPhase: 'awaiting_response', method: 'worktree.rm' } + } + }) }) - ;(transport as unknown as UnixSocketTransportInternals).handleConnection( - socket as unknown as Socket - ) - socket.emit('data', '{"id":"slow-rm","method":"worktree.rm"}\n') + }) + ;(transport as unknown as UnixSocketTransportInternals).handleConnection( + socket as unknown as Socket + ) + socket.emit('data', '{"id":"slow-rm","method":"worktree.rm"}\n') - vi.advanceTimersByTime(25) - reply?.('{"id":"slow-rm","ok":true}') + vi.advanceTimersByTime(25) + reply?.('{"id":"slow-rm","ok":true}') - const terminalWrites = socket.writes.filter((write) => !write.includes('_keepalive')) - expect(terminalWrites).toHaveLength(1) - expect(JSON.parse(terminalWrites[0]!.trim())).toMatchObject({ - id: 'slow-rm', - ok: false, - error: { - code: 'runtime_timeout', - data: { requestPhase: 'awaiting_response', method: 'worktree.rm' } - } - }) - } - ) + const terminalWrites = socket.writes.filter((write) => !write.includes('_keepalive')) + expect(terminalWrites).toHaveLength(1) + expect(JSON.parse(terminalWrites[0]!.trim())).toMatchObject({ + id: 'slow-rm', + ok: false, + error: { + code: 'runtime_timeout', + data: { requestPhase: 'awaiting_response', method: 'worktree.rm' } + } + }) + }) }) diff --git a/src/main/runtime/rpc/unix-socket-transport.ts b/src/main/runtime/rpc/unix-socket-transport.ts index dd1966601e3..55ef0340350 100644 --- a/src/main/runtime/rpc/unix-socket-transport.ts +++ b/src/main/runtime/rpc/unix-socket-transport.ts @@ -6,7 +6,7 @@ // away. See design doc §3.1. import { createServer, type Server, type Socket } from 'node:net' import { chmodSync, existsSync, rmSync } from 'node:fs' -import type { RpcMessageContext, RpcTransport } from './transport' +import type { RpcKeepaliveBound, RpcMessageContext, RpcTransport } from './transport' const MAX_RUNTIME_RPC_MESSAGE_BYTES = 1024 * 1024 export const RUNTIME_RPC_SOCKET_IDLE_TIMEOUT_MS = 30_000 @@ -167,19 +167,6 @@ export class UnixSocketTransport implements RpcTransport { let replied = false let keepaliveTimer: NodeJS.Timeout | null = null let keepaliveExpiryTimer: NodeJS.Timeout | null = null - let requestId = 'unknown' - let requestMethod = 'unknown' - try { - const request = JSON.parse(rawMessage) as { id?: unknown; method?: unknown } - if (typeof request.id === 'string' && request.id.length > 0) { - requestId = request.id - } - if (typeof request.method === 'string' && request.method.length > 0) { - requestMethod = request.method - } - } catch { - // Request validation remains owned by the RPC server. - } // Why: each dispatch needs its own abort signal and keepalive timer // cleanup. Socket close runs every cleanup without touching sibling // dispatches that already replied on the same connection. @@ -217,7 +204,7 @@ export class UnixSocketTransport implements RpcTransport { } } - const startKeepalive = (maxDurationMs?: number): void => { + const startKeepalive = (bounded?: RpcKeepaliveBound): void => { if (keepaliveTimer || replied) { return } @@ -232,21 +219,11 @@ export class UnixSocketTransport implements RpcTransport { if (typeof keepaliveTimer.unref === 'function') { keepaliveTimer.unref() } - if (maxDurationMs !== undefined) { + if (bounded) { keepaliveExpiryTimer = setTimeout(() => { keepaliveExpiryTimer = null - reply( - JSON.stringify({ - id: requestId, - ok: false, - error: { - code: 'runtime_timeout', - message: `The runtime stopped waiting for ${requestMethod} before it completed.`, - data: { requestPhase: 'awaiting_response', method: requestMethod } - } - }) - ) - }, maxDurationMs) + reply(bounded.timeoutResponse) + }, bounded.maxDurationMs) keepaliveExpiryTimer.unref?.() } } diff --git a/src/main/runtime/runtime-rpc-worktree-rm-liveness.test.ts b/src/main/runtime/runtime-rpc-worktree-rm-liveness.test.ts index b956ed2113a..4a3b5481e31 100644 --- a/src/main/runtime/runtime-rpc-worktree-rm-liveness.test.ts +++ b/src/main/runtime/runtime-rpc-worktree-rm-liveness.test.ts @@ -4,7 +4,7 @@ import { join } from 'node:path' import { describe, expect, it, vi } from 'vitest' import { OrcaRuntimeService } from './orca-runtime' import { readRuntimeMetadata } from './runtime-metadata' -import { openFramedSession, waitFor } from './runtime-rpc-test-harness' +import { openFramedSession, sleep, waitFor } from './runtime-rpc-test-harness' import { OrcaRuntimeRpcServer } from './runtime-rpc' function createRemovalGate(runtime: OrcaRuntimeService) { @@ -49,9 +49,9 @@ describe('worktree.rm runtime RPC liveness', () => { const server = new OrcaRuntimeRpcServer({ runtime, userDataPath, - socketIdleTimeoutMs: 50, + socketIdleTimeoutMs: 100, keepaliveIntervalMs: 10, - worktreeRemoveKeepaliveMaxMs: 200 + worktreeRemoveKeepaliveMaxMs: 400 }) await server.start() @@ -62,7 +62,15 @@ describe('worktree.rm runtime RPC liveness', () => { } expect(metadata.transports[0].kind).toBe(process.platform === 'win32' ? 'named-pipe' : 'unix') const session = startRemoval(metadata.transports[0].endpoint, metadata.authToken) - await waitFor(() => session.frames.filter((frame) => frame._keepalive === true).length >= 2) + await waitFor(() => session.frames.some((frame) => frame._keepalive === true)) + const keepalivesBeforeIdleWindow = session.frames.filter( + (frame) => frame._keepalive === true + ).length + await sleep(150) + expect(session.frames.filter((frame) => frame.ok !== undefined)).toHaveLength(0) + expect(session.frames.filter((frame) => frame._keepalive === true).length).toBeGreaterThan( + keepalivesBeforeIdleWindow + ) removal.release() await session.done @@ -97,6 +105,7 @@ describe('worktree.rm runtime RPC liveness', () => { throw new Error('runtime metadata was not written') } const session = startRemoval(metadata.transports[0].endpoint, metadata.authToken) + await waitFor(() => session.frames.filter((frame) => frame.ok !== undefined).length === 1) await session.done expect(session.frames.filter((frame) => frame._keepalive === true).length).toBeGreaterThan(1) diff --git a/src/main/runtime/runtime-rpc.ts b/src/main/runtime/runtime-rpc.ts index ce3986a67a9..296b77aba03 100644 --- a/src/main/runtime/runtime-rpc.ts +++ b/src/main/runtime/runtime-rpc.ts @@ -75,7 +75,7 @@ type OrcaRuntimeRpcServerOptions = { // Only `orca serve` (explicit remote opt-in) and E2E set this; the desktop app widens lazily on pairing. exposeNetworkByDefault?: boolean webClientRoot?: string - // Why: test-only overrides for the two constants below; production must not pass these (defaults set by §3.1). + // Why: test-only timing overrides; production uses the bounded policy defaults. keepaliveIntervalMs?: number socketIdleTimeoutMs?: number worktreeRemoveKeepaliveMaxMs?: number @@ -1550,7 +1550,6 @@ export class OrcaRuntimeRpcServer { return this.buildError(request.id, 'runtime_busy', rejection) } if (longPoll) { - // Why: arm keepalive only for long-polls; short RPCs never create the setInterval. See §3.1. context?.startKeepalive() } else { const maxKeepaliveMs = boundedDispatchKeepaliveMs( @@ -1558,7 +1557,17 @@ export class OrcaRuntimeRpcServer { this.worktreeRemoveKeepaliveMaxMs ) if (maxKeepaliveMs !== null) { - context?.startKeepalive(maxKeepaliveMs) + context?.startKeepalive({ + maxDurationMs: maxKeepaliveMs, + timeoutResponse: JSON.stringify( + this.buildError( + request.id, + 'runtime_timeout', + `The runtime stopped waiting for ${request.method} before it completed.`, + { requestPhase: 'awaiting_response', method: request.method } + ) + ) + }) } } @@ -1751,8 +1760,8 @@ export class OrcaRuntimeRpcServer { } } - private buildError(id: string, code: string, message: string): RpcResponse { - return errorResponse(id, { runtimeId: this.runtime.getRuntimeId() }, code, message) + private buildError(id: string, code: string, message: string, data?: unknown): RpcResponse { + return errorResponse(id, { runtimeId: this.runtime.getRuntimeId() }, code, message, data) } private writeMetadata(): void {