From 539e283c0fc54790ea58473dae4e0b2367147d0f Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Tue, 15 Sep 2026 15:30:01 -0700 Subject: [PATCH] fix(agent-launch): dedupe complete launch and cancel setup wait --- ...ntime-start-tui-idle-visible-read-probe.ts | 14 ++- ...nal-creation-and-readiness-part-06.spec.ts | 31 ++++++ .../methods/agent-launch-worktree-creation.ts | 94 +++++++++--------- .../runtime/rpc/methods/agent-launch.test.ts | 96 ++++++++++++++++++- src/main/runtime/rpc/methods/agent-launch.ts | 21 ++-- 5 files changed, 200 insertions(+), 56 deletions(-) diff --git a/src/main/runtime/orca-runtime-start-tui-idle-visible-read-probe.ts b/src/main/runtime/orca-runtime-start-tui-idle-visible-read-probe.ts index adaa5fde72f..aa630277078 100644 --- a/src/main/runtime/orca-runtime-start-tui-idle-visible-read-probe.ts +++ b/src/main/runtime/orca-runtime-start-tui-idle-visible-read-probe.ts @@ -96,7 +96,10 @@ export class OrcaRuntimeWithStartTuiIdleVisibleReadProbe extends OrcaRuntimeWith : buildTerminalWaitResult(handle, 'tui-idle', leaf) } - async waitForSetupTerminalCompletion(handle: string): Promise<{ exitCode: number | null }> { + async waitForSetupTerminalCompletion( + handle: string, + signal?: AbortSignal + ): Promise<{ exitCode: number | null }> { const ptyId = this.getLivePtyForHandle(handle)?.pty.ptyId if (!ptyId) { throw new Error('terminal_handle_stale') @@ -106,9 +109,13 @@ export class OrcaRuntimeWithStartTuiIdleVisibleReadProbe extends OrcaRuntimeWith return await new Promise<{ exitCode: number | null }>((resolve, reject) => { let settled = false let unsubscribe: (() => void) | null = null + const onAbort = (): void => { + fail(signal?.reason ?? new Error('request_aborted')) + } const cleanup = (): void => { unsubscribe?.() exitAbort.abort() + signal?.removeEventListener('abort', onAbort) } const finish = (exitCode: number | null): void => { if (settled) { @@ -127,6 +134,11 @@ export class OrcaRuntimeWithStartTuiIdleVisibleReadProbe extends OrcaRuntimeWith cleanup() reject(error) } + if (signal?.aborted) { + onAbort() + return + } + signal?.addEventListener('abort', onAbort, { once: true }) const scanner = completionToken ? createSetupCompletionScanner(completionToken, finish) : null if (scanner) { diff --git a/src/main/runtime/orca-runtime-tests/terminal-creation-and-readiness-part-06.spec.ts b/src/main/runtime/orca-runtime-tests/terminal-creation-and-readiness-part-06.spec.ts index 6d9326b37f2..1d0841b5e1c 100644 --- a/src/main/runtime/orca-runtime-tests/terminal-creation-and-readiness-part-06.spec.ts +++ b/src/main/runtime/orca-runtime-tests/terminal-creation-and-readiness-part-06.spec.ts @@ -103,6 +103,37 @@ describe('OrcaRuntimeService', () => { await expect(waiting).resolves.toEqual({ exitCode: 9 }) }) + it('cancels setup completion observation when the caller aborts', async () => { + const runtime = new OrcaRuntimeService(store) + runtime.setPtyController({ + spawn: vi.fn().mockResolvedValue({ id: 'pty-cancelled-setup' }), + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + runtime.attachWindow(1) + runtime.syncWindowGraph(1, { tabs: [], leaves: [] }) + const { handle } = await runtime.createTerminal(`path:${TEST_WORKTREE_PATH}`) + ;( + runtime as unknown as { setupCompletionTokenByPtyId: Map } + ).setupCompletionTokenByPtyId.set('pty-cancelled-setup', 'token-cancelled') + const unsubscribe = vi.fn() + vi.spyOn(runtime, 'subscribeToTerminalData').mockReturnValue(unsubscribe) + const controller = new AbortController() + + const waiting = runtime.waitForSetupTerminalCompletion(handle, controller.signal) + expect(runtime.subscribeToTerminalData).toHaveBeenCalledWith( + 'pty-cancelled-setup', + expect.any(Function) + ) + + const reason = new Error('cancelled') + controller.abort(reason) + + await expect(waiting).rejects.toBe(reason) + expect(unsubscribe).toHaveBeenCalledOnce() + }) + it('keeps observing after an uncertain setup terminal status', async () => { const runtime = new OrcaRuntimeService(store) runtime.setPtyController({ diff --git a/src/main/runtime/rpc/methods/agent-launch-worktree-creation.ts b/src/main/runtime/rpc/methods/agent-launch-worktree-creation.ts index 18842b06089..edf8dade6c1 100644 --- a/src/main/runtime/rpc/methods/agent-launch-worktree-creation.ts +++ b/src/main/runtime/rpc/methods/agent-launch-worktree-creation.ts @@ -39,51 +39,49 @@ export function agentLaunchWorkspaceFactory( // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: already validated by `AgentLaunch`; the executor only removed the reserved agent fields, so the rest of the payload is the parsed shape. const params = create as WorktreeCreateParams const { runtime } = context - return runtime.dedupeWorktreeCreate(params.repo, params.clientMutationId, async () => { - const repo = await runtime.showRepo(params.repo) - const automationProvenance = resolveAutomationWorkspaceProvenance({ - authority: runtime, - repoSelector: params.repo, - repo, - request: params.automationProvenanceRequest - }) - // Reserved before creation so a retry can recover; a failed attempt has to release it. - try { - const result = await runtime.createManagedWorktree({ - ...buildManagedWorktreeCreateArgs( - { ...params, ...(startupAgent ? { startupAgent } : {}) }, - { - automationProvenance, - cliProvenance: buildCliWorkspaceProvenance(params.cliProvenanceRequest, { - startupAgent: agent, - createdAt: Date.now() - }), - creatorProvenance: resolveRpcWorkspaceCreatorProvenance(context) - }, - context.clientKind ? { clientKind: context.clientKind } : {} - ), - // The launch owns the agent whichever surface it settles on, so the workspace records - // it even when no startup terminal was created for it. - createdWithAgent: agent, - // Structured sessions have no startup command to sequence behind setup. Provision the - // setup terminal synchronously and attach a completion token so the launch can wait - // before creating the chat surface. - awaitTerminalProvisioning: true, - observeSetupCompletion: true - }) - if (!startupAgent) { - await waitForStructuredSetup(runtime, result.setupReceipt) - } - finishAutomationWorkspaceProvenanceRequest(params.automationProvenanceRequest) - return { - worktreeId: result.worktree.id, - startupTerminalHandle: result.startupTerminal?.handle - } - } catch (error) { - releaseAutomationWorkspaceProvenanceRequest(params.automationProvenanceRequest) - throw error - } + const repo = await runtime.showRepo(params.repo) + const automationProvenance = resolveAutomationWorkspaceProvenance({ + authority: runtime, + repoSelector: params.repo, + repo, + request: params.automationProvenanceRequest }) + // Reserved before creation so a retry can recover; a failed attempt has to release it. + try { + const result = await runtime.createManagedWorktree({ + ...buildManagedWorktreeCreateArgs( + { ...params, ...(startupAgent ? { startupAgent } : {}) }, + { + automationProvenance, + cliProvenance: buildCliWorkspaceProvenance(params.cliProvenanceRequest, { + startupAgent: agent, + createdAt: Date.now() + }), + creatorProvenance: resolveRpcWorkspaceCreatorProvenance(context) + }, + context.clientKind ? { clientKind: context.clientKind } : {} + ), + // The launch owns the agent whichever surface it settles on, so the workspace records + // it even when no startup terminal was created for it. + createdWithAgent: agent, + // Structured sessions have no startup command to sequence behind setup. Provision the + // setup terminal synchronously and attach a completion token so the launch can wait + // before creating the chat surface. + awaitTerminalProvisioning: true, + observeSetupCompletion: true + }) + if (!startupAgent) { + await waitForStructuredSetup(runtime, result.setupReceipt) + } + finishAutomationWorkspaceProvenanceRequest(params.automationProvenanceRequest) + return { + worktreeId: result.worktree.id, + startupTerminalHandle: result.startupTerminal?.handle + } + } catch (error) { + releaseAutomationWorkspaceProvenanceRequest(params.automationProvenanceRequest) + throw error + } } } } @@ -100,12 +98,16 @@ async function waitForStructuredSetup( ) { return } + const abort = new AbortController() let timer: ReturnType | undefined try { await Promise.race([ - runtime.waitForSetupTerminalCompletion(receipt.terminalHandle), + runtime.waitForSetupTerminalCompletion(receipt.terminalHandle, abort.signal), new Promise((resolve) => { - timer = setTimeout(resolve, STRUCTURED_SETUP_WAIT_TIMEOUT_MS) + timer = setTimeout(() => { + abort.abort(new Error('structured_setup_wait_timeout')) + resolve() + }, STRUCTURED_SETUP_WAIT_TIMEOUT_MS) }) ]) } catch { diff --git a/src/main/runtime/rpc/methods/agent-launch.test.ts b/src/main/runtime/rpc/methods/agent-launch.test.ts index 7c6c8bf6784..aaf99d79629 100644 --- a/src/main/runtime/rpc/methods/agent-launch.test.ts +++ b/src/main/runtime/rpc/methods/agent-launch.test.ts @@ -42,14 +42,32 @@ function runtimeStub( } } = {} ) { - const waitForSetupTerminalCompletion = vi.fn(async () => ({ exitCode: 0 })) + const worktreeCreateResults = new Map>() + const waitForSetupTerminalCompletion = vi.fn( + async (_handle: string, _signal?: AbortSignal): Promise<{ exitCode: number | null }> => ({ + exitCode: 0 + }) + ) return { getClientSettings: vi.fn(() => options.settings ?? STRUCTURED_PREFERENCE), getStructuredAgentSessionCreateSupport: vi.fn( async () => options.createSupport ?? { supported: true } ), dedupeWorktreeCreate: vi.fn( - (_repo: string, _key: string | undefined, run: () => Promise) => run() + (repo: string, key: string | undefined, run: () => Promise) => { + if (!key) { + return run() + } + const compositeKey = `${repo}\0${key}` + const existing = worktreeCreateResults.get(compositeKey) + if (existing) { + return existing + } + const result = run() + worktreeCreateResults.set(compositeKey, result) + void result.catch(() => worktreeCreateResults.delete(compositeKey)) + return result + } ), showRepo: vi.fn(async () => ({ id: 'repo-1' })), createManagedWorktree: vi.fn(async (args: Record) => ({ @@ -126,6 +144,14 @@ const CREATE_LAUNCH = { target: { kind: 'create-worktree', create: { repo: 'id:repo-1', name: 'task' } } } +const IDEMPOTENT_CREATE_LAUNCH = { + agent: 'claude', + target: { + kind: 'create-worktree' as const, + create: { repo: 'id:repo-1', name: 'task', clientMutationId: 'launch-1' } + } +} + beforeEach(() => { createStructuredSession.mockClear() }) @@ -247,6 +273,67 @@ describe('the worktree factory', () => { expect(result.outcome.kind).toBe('structured') }) + it('deduplicates concurrent launches through surface creation', async () => { + const runtime = runtimeStub() + + const results = await Promise.all([ + launch(IDEMPOTENT_CREATE_LAUNCH, runtime), + launch(IDEMPOTENT_CREATE_LAUNCH, runtime) + ]) + + expect(results[0]).toEqual(results[1]) + expect(runtime.dedupeWorktreeCreate).toHaveBeenCalledTimes(2) + expect(runtime.dedupeWorktreeCreate.mock.calls).toEqual([ + ['id:repo-1', 'agent.launch:launch-1', expect.any(Function)], + ['id:repo-1', 'agent.launch:launch-1', expect.any(Function)] + ]) + expect(runtime.createManagedWorktree).toHaveBeenCalledTimes(1) + expect(createStructuredSession).toHaveBeenCalledTimes(1) + }) + + it('reuses a completed launch result for a sequential retry', async () => { + const runtime = runtimeStub() + + const first = await launch(IDEMPOTENT_CREATE_LAUNCH, runtime) + const retried = await launch(IDEMPOTENT_CREATE_LAUNCH, runtime) + + expect(retried).toEqual(first) + expect(runtime.createManagedWorktree).toHaveBeenCalledTimes(1) + expect(createStructuredSession).toHaveBeenCalledTimes(1) + }) + + it('aborts the setup wait when its bounded timeout expires', async () => { + vi.useFakeTimers() + try { + const runtime = runtimeStub({ + setupReceipt: { + startupPolicy: 'wait-for-setup', + state: 'running', + terminalHandle: 'setup-1' + } + }) + let setupSignal: AbortSignal | undefined + runtime.waitForSetupTerminalCompletion.mockImplementation( + (_handle, signal) => + new Promise<{ exitCode: number | null }>((_resolve, reject) => { + setupSignal = signal + signal?.addEventListener('abort', () => reject(signal.reason), { once: true }) + }) + ) + + const result = await (async () => { + const pending = launch(CREATE_LAUNCH, runtime) + await vi.runAllTimersAsync() + return pending + })() + + expect(result.outcome.kind).toBe('structured') + expect(setupSignal?.aborted).toBe(true) + } finally { + vi.useRealTimers() + } + }) + it('waits for a setup-gated structured workspace before creating its session', async () => { const runtime = runtimeStub({ setupReceipt: { @@ -268,7 +355,10 @@ describe('the worktree factory', () => { await launch(CREATE_LAUNCH, runtime) expect(order).toEqual(['setup-complete', 'structured-create']) - expect(runtime.waitForSetupTerminalCompletion).toHaveBeenCalledWith('setup-1') + expect(runtime.waitForSetupTerminalCompletion).toHaveBeenCalledWith( + 'setup-1', + expect.any(AbortSignal) + ) }) it('keeps agent-first creation for a launch the user wants as a terminal', async () => { diff --git a/src/main/runtime/rpc/methods/agent-launch.ts b/src/main/runtime/rpc/methods/agent-launch.ts index e1ece033ff6..54e0e8a76f6 100644 --- a/src/main/runtime/rpc/methods/agent-launch.ts +++ b/src/main/runtime/rpc/methods/agent-launch.ts @@ -96,12 +96,21 @@ export const AGENT_LAUNCH_METHODS = [ } const intent = await agentLaunchIntent(params, context.runtime) await validateReusedTerminal(intent, context.runtime) - return executeAgentLaunch({ - runtime: context.runtime, - intent, - surfaces: agentLaunchSurfaceFactory(context), - workspaces: agentLaunchWorkspaceFactory(context, intent.agent) - }) + const execute = () => + executeAgentLaunch({ + runtime: context.runtime, + intent, + surfaces: agentLaunchSurfaceFactory(context), + workspaces: agentLaunchWorkspaceFactory(context, intent.agent) + }) + if (params.target.kind === 'create-worktree' && params.target.create.clientMutationId) { + return context.runtime.dedupeWorktreeCreate( + params.target.create.repo, + `agent.launch:${params.target.create.clientMutationId}`, + execute + ) + } + return execute() } }) ]