From 0e1d0ad4a8380e2a618ef42c75bffe4e08cb0d17 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Mon, 19 Oct 2026 05:21:06 -0400 Subject: [PATCH] A CLI split that times out leaves no pane (dark, layout-owner build only) (#30563) * fix(layout): a pane main closed stays closed when its terminal starts late (dark, layout-owner build only) * fix(layout): main takes back a CLI split pane whose terminal does not start within 10 s (dark, layout-owner build only) * fix(layout): the timeout take-back keeps a pane whose terminal started or is starting (dark, layout-owner build only) * fix(layout): one take-back call keeps the layout-writer count * fix(layout): a split take-back is decided in the write queue, and a spawn for a taken-back pane starts nothing (dark, layout-owner build only) * test(layout): a spawn for a taken-back pane is refused before the provider spawns * test: typed store fake in the closed-pane spawn test, no casts --- .../ipc/pty-closed-pane-spawn-refusal.test.ts | 113 +++++++++++++++++ .../pty/ipc/closed-pane-spawn-refusal.test.ts | 69 ++++++++++ .../ipc/pty/ipc/closed-pane-spawn-refusal.ts | 28 +++++ src/main/ipc/pty/ipc/spawn-run.ts | 6 + src/main/ipc/pty/pane/spawn-reservation.ts | 11 ++ .../loading-store/pty-binding-persistence.ts | 15 ++- .../unstarted-pane-withdraw.ts | 30 +++++ .../runtime/runtime-durable-store-fixture.ts | 19 ++- .../runtime/runtime-pane-admission.test.ts | 1 + src/main/runtime/runtime-pane-admission.ts | 24 ++-- .../terminal-split-layout-owner.test.ts | 118 ++++++++++++++++-- .../launch/terminal-split-source-authority.ts | 3 +- .../launch/terminal-split-wait-timeout.ts | 2 + .../runtime/terminal/launch/terminal-split.ts | 57 ++++++++- ...inal-cli-split-closed-before-mount.spec.ts | 68 +++++++++- 15 files changed, 533 insertions(+), 31 deletions(-) create mode 100644 src/main/ipc/pty-closed-pane-spawn-refusal.test.ts create mode 100644 src/main/ipc/pty/ipc/closed-pane-spawn-refusal.test.ts create mode 100644 src/main/ipc/pty/ipc/closed-pane-spawn-refusal.ts create mode 100644 src/main/persistence/terminal-topology/unstarted-pane-withdraw.ts create mode 100644 src/main/runtime/terminal/launch/terminal-split-wait-timeout.ts diff --git a/src/main/ipc/pty-closed-pane-spawn-refusal.test.ts b/src/main/ipc/pty-closed-pane-spawn-refusal.test.ts new file mode 100644 index 00000000000..75965d120bb --- /dev/null +++ b/src/main/ipc/pty-closed-pane-spawn-refusal.test.ts @@ -0,0 +1,113 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { getDefaultWorkspaceSession } from '../../shared/constants' +import { LOCAL_EXECUTION_HOST_ID } from '../../shared/execution-host' +import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' +import { + _resetClosedPaneFencesForTests, + fencedIfLayoutOwner, + partitionsFence +} from '../persistence/terminal-topology/closed-pane-binding-fence' +import { Store } from '../persistence' +import { spawnMock } from './pty-ipc-mock-registry' +import { setupPtyIpcSuite } from './pty-ipc-test-harness' +import { registerPtyHandlers } from './pty/register-handlers' + +vi.mock('electron', () => import('./pty-ipc-mock-registry').then((m) => m.electronModuleMock())) +vi.mock('fs', () => import('./pty-ipc-mock-registry').then((m) => m.fsModuleMock())) +vi.mock('node-pty', () => import('./pty-ipc-mock-registry').then((m) => m.nodePtyModuleMock())) +vi.mock('node:child_process', async (importOriginal) => + (await import('./pty-ipc-mock-registry')).childProcessModuleMock(await importOriginal()) +) +vi.mock('../opencode/hook-service', () => + import('./pty-ipc-mock-registry').then((m) => m.openCodeHookServiceModuleMock()) +) +vi.mock('../mimo/hook-service', () => + import('./pty-ipc-mock-registry').then((m) => m.mimoHookServiceModuleMock()) +) +vi.mock('../agent-hooks/server', () => + import('./pty-ipc-mock-registry').then((m) => m.agentHookServerModuleMock()) +) +vi.mock('../pi/titlebar-extension-service', () => + import('./pty-ipc-mock-registry').then((m) => m.piTitlebarExtensionModuleMock()) +) +vi.mock('../pwsh', () => import('./pty-ipc-mock-registry').then((m) => m.pwshModuleMock())) +vi.mock('../wsl', async (importOriginal) => + (await import('./pty-ipc-mock-registry')).wslModuleMock(await importOriginal()) +) +vi.mock('../telemetry/client', () => + import('./pty-ipc-mock-registry').then((m) => m.telemetryClientModuleMock()) +) +vi.mock('../telemetry/classify-error', () => + import('./pty-ipc-mock-registry').then((m) => m.classifyErrorModuleMock()) +) +vi.mock('../cli/linux-terminal-orca-cli-shim', () => + import('./pty-ipc-mock-registry').then((m) => m.linuxCliShimModuleMock()) +) +vi.mock('../memory/pty-registry', () => + import('./pty-ipc-mock-registry').then((m) => m.ptyRegistryModuleMock()) +) +vi.mock('../agent-hooks/migration-unsupported-pty-state', () => + import('./pty-ipc-mock-registry').then((m) => m.migrationUnsupportedPtyModuleMock()) +) +vi.mock('../codex/codex-pane-account-registry', () => + import('./pty-ipc-mock-registry').then((m) => m.codexPaneAccountRegistryModuleMock()) +) +vi.mock('../codex/codex-state-db-backfill-recovery', () => + import('./pty-ipc-mock-registry').then((m) => m.codexBackfillRecoveryModuleMock()) +) +vi.mock('../../shared/layout-owner-switch', () => ({ ORCA_LAYOUT_OWNER_IS_MAIN: true })) + +const TAB = 'tab-1' +const KEPT = '11111111-1111-4111-8111-111111111111' +const TAKEN = '22222222-2222-4222-8222-222222222222' + +function sessionWith(leafIds: string[]): WorkspaceSessionState { + const [first, second] = leafIds + return { + ...getDefaultWorkspaceSession(), + terminalLayoutsByTabId: { + [TAB]: { + root: second + ? { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: first! }, + second: { type: 'leaf', leafId: second } + } + : { type: 'leaf', leafId: first! }, + activeLeafId: first!, + expandedLeafId: null + } + } + } +} + +describe('a window spawn for a pane main took back while it waited', () => { + const { handlers, mainWindow } = setupPtyIpcSuite() + afterEach(() => _resetClosedPaneFencesForTests()) + + it('is refused before the provider spawns anything', async () => { + const current = { value: sessionWith([KEPT, TAKEN]) } + fencedIfLayoutOwner( + () => partitionsFence(['local'], () => current.value, true), + () => ((current.value = sessionWith([KEPT])), { value: null }) + )() + const store: Store = Object.create(Store.prototype) + vi.spyOn(store, 'getWorkspaceSessionHostIds').mockReturnValue([LOCAL_EXECUTION_HOST_ID]) + vi.spyOn(store, 'getWorkspaceSession').mockImplementation(() => current.value) + handlers.clear() + registerPtyHandlers(mainWindow, undefined, undefined, undefined, undefined, store) + spawnMock.mockClear() + + await expect( + handlers.get('pty:spawn')!(null, { + cols: 80, + rows: 24, + command: 'touch ran', + tabId: TAB, + leafId: TAKEN + }) + ).rejects.toThrow('terminal_pane_closed') + expect(spawnMock).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/ipc/pty/ipc/closed-pane-spawn-refusal.test.ts b/src/main/ipc/pty/ipc/closed-pane-spawn-refusal.test.ts new file mode 100644 index 00000000000..ce480041153 --- /dev/null +++ b/src/main/ipc/pty/ipc/closed-pane-spawn-refusal.test.ts @@ -0,0 +1,69 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import { getDefaultWorkspaceSession } from '../../../../shared/constants' +import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' +import type { Store } from '../../../persistence' +import { + _resetClosedPaneFencesForTests, + fencedIfLayoutOwner, + partitionsFence +} from '../../../persistence/terminal-topology/closed-pane-binding-fence' +import { refuseClosedPaneSpawn } from './closed-pane-spawn-refusal' + +vi.mock('../../../../shared/layout-owner-switch', () => ({ ORCA_LAYOUT_OWNER_IS_MAIN: true })) + +function session(leafIds: string[]): WorkspaceSessionState { + const [first, second] = leafIds + return { + ...getDefaultWorkspaceSession(), + terminalLayoutsByTabId: { + t: { + root: second + ? { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: first! }, + second: { type: 'leaf', leafId: second } + } + : { type: 'leaf', leafId: first! }, + activeLeafId: first!, + expandedLeafId: null + } + } + } +} + +function storeHolding(current: { value: WorkspaceSessionState }): Store { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: refuseClosedPaneSpawn reads only these two Store members. + return { + getWorkspaceSessionHostIds: () => ['local'], + getWorkspaceSession: () => current.value + } as unknown as Store +} + +describe('a spawn for a pane main closed while it waited', () => { + afterEach(() => _resetClosedPaneFencesForTests()) + + it('starts nothing once main took the pane back', () => { + const current = { value: session(['a', 'b']) } + fencedIfLayoutOwner( + () => partitionsFence(['local'], () => current.value, true), + () => ((current.value = session(['a'])), { value: null }) + )() + + expect(() => refuseClosedPaneSpawn(storeHolding(current), 't', 'b')).toThrow( + 'terminal_pane_closed' + ) + }) + + it('starts a pane main never closed, or one it holds again', () => { + const current = { value: session(['a', 'b']) } + expect(() => refuseClosedPaneSpawn(storeHolding(current), 't', 'b')).not.toThrow() + + fencedIfLayoutOwner( + () => partitionsFence(['local'], () => current.value, true), + () => ((current.value = session(['a'])), { value: null }) + )() + current.value = session(['a', 'b']) + expect(() => refuseClosedPaneSpawn(storeHolding(current), 't', 'b')).not.toThrow() + }) +}) diff --git a/src/main/ipc/pty/ipc/closed-pane-spawn-refusal.ts b/src/main/ipc/pty/ipc/closed-pane-spawn-refusal.ts new file mode 100644 index 00000000000..ab594040552 --- /dev/null +++ b/src/main/ipc/pty/ipc/closed-pane-spawn-refusal.ts @@ -0,0 +1,28 @@ +import { isClosedPaneFenced } from '../../../persistence/terminal-topology/closed-pane-binding-fence' +import type { Store } from '../../../persistence' +import { layoutContainsLeafId } from '../../../../shared/workspace-layout/terminal-pane-tree' + +/** + * Layout-owner build: a spawn for a pane main closed or took back, which main's layout no longer + * holds, starts nothing. Its binding would be refused anyway (pty-binding-refusals.ts); refusing + * first means its startup command never runs. + */ +export function refuseClosedPaneSpawn( + store: Store | undefined, + tabId: string | undefined, + leafId: string | null | undefined +): void { + // The fence's own key: legacy non-UUID leaf ids are matched, never thrown on. + if (!tabId || !leafId || !isClosedPaneFenced(`${tabId}:${leafId}`)) { + return + } + const held = (store?.getWorkspaceSessionHostIds() ?? []).some((hostId) => + layoutContainsLeafId( + store?.getWorkspaceSession(hostId)?.terminalLayoutsByTabId?.[tabId]?.root ?? null, + leafId + ) + ) + if (!held) { + throw new Error('terminal_pane_closed') + } +} diff --git a/src/main/ipc/pty/ipc/spawn-run.ts b/src/main/ipc/pty/ipc/spawn-run.ts index 56075d02fa6..fd25968b02d 100644 --- a/src/main/ipc/pty/ipc/spawn-run.ts +++ b/src/main/ipc/pty/ipc/spawn-run.ts @@ -17,6 +17,8 @@ import { commitPtyIpcSpawn } from './spawn-commit' import { createPtyIpcSpawnState, type PtyIpcSpawnState } from './spawn-state' import { triggerPtySpawnPushTargetMaterialization } from './spawn-push-target-materialization' import type { PtySpawnIpcArgs, PtySpawnIpcDeps } from './spawn-types' +import { ORCA_LAYOUT_OWNER_IS_MAIN } from '../../../../shared/layout-owner-switch' +import { refuseClosedPaneSpawn } from './closed-pane-spawn-refusal' function releaseAbandonedAgentTeamsLeader(ctx: PtyIpcSpawnState): void { if (!ctx.agentTeamsLeaderHandle) { @@ -76,6 +78,10 @@ export async function runPtyIpcSpawn(deps: PtySpawnIpcDeps, args: PtySpawnIpcArg } // Right before the provider spawn, with nothing awaited in between: a closed pane starts nothing. ctx.paneAgentLaunch?.refuseIfClosed() + // Switch point: a pane main closed or took back while this spawn waited starts nothing. + if (ORCA_LAYOUT_OWNER_IS_MAIN) { + refuseClosedPaneSpawn(deps.store, args.tabId, args.leafId) + } await executePtyIpcSpawn(ctx) const committed = await commitPtyIpcSpawn(ctx) endPaneAgentLaunch( diff --git a/src/main/ipc/pty/pane/spawn-reservation.ts b/src/main/ipc/pty/pane/spawn-reservation.ts index 6a760512d7c..8f8c7a0bca7 100644 --- a/src/main/ipc/pty/pane/spawn-reservation.ts +++ b/src/main/ipc/pty/pane/spawn-reservation.ts @@ -82,6 +82,17 @@ export function clearPaneSpawnReservation( } } +/** Whether a window or runtime spawn of this pane has begun and not yet bound or failed. */ +export function isPaneSpawnInFlight(worktreeId: string, paneKey: string): boolean { + for (const ownerKey of paneSpawnReservationsByOwnerKey.keys()) { + const parsed: unknown = JSON.parse(ownerKey) + if (Array.isArray(parsed) && parsed[1] === worktreeId && parsed[2] === paneKey) { + return true + } + } + return false +} + export function makePaneSpawnReservationKey( worktreeId: string | undefined, connectionId: string | null | undefined, diff --git a/src/main/persistence/loading-store/pty-binding-persistence.ts b/src/main/persistence/loading-store/pty-binding-persistence.ts index c274a883acc..ea835138ae0 100644 --- a/src/main/persistence/loading-store/pty-binding-persistence.ts +++ b/src/main/persistence/loading-store/pty-binding-persistence.ts @@ -11,6 +11,10 @@ import type { PtyBindingSourceExpectation } from './store' import type { StoreRuntimeState } from './store-runtime-state' import type { SessionHostPartitionOperations } from './session-host-partitions' +import { + withdrawUnstartedPane, + type UnstartedPaneKept +} from '../terminal-topology/unstarted-pane-withdraw' import { resolveHostId } from './session-host-partitions' import { evaluatePtyBindingFastLane } from './pty-binding-fast-lane' import { ptyBindingIsRefused } from './pty-binding-refusals' @@ -244,12 +248,17 @@ export class PtyBindingPersistenceOperations { ) } + /** With `stillStarting`, a pane whose terminal bound or is starting stays (UnstartedPaneKept). */ withdrawTerminalPane( pane: CommandOf<'closePane'>, - hostId?: string | null - ): Promise { + hostId?: string | null, + stillStarting?: () => boolean + ): Promise { + const resolved = resolveHostId(hostId) return this[ptyBindingPersistenceOperationsContext].runtime.runDurableMutation( - withdrawPane(resolveHostId(hostId), pane, this.topology()) + stillStarting + ? withdrawUnstartedPane(resolved, pane, this.topology(), stillStarting) + : withdrawPane(resolved, pane, this.topology()) ) } diff --git a/src/main/persistence/terminal-topology/unstarted-pane-withdraw.ts b/src/main/persistence/terminal-topology/unstarted-pane-withdraw.ts new file mode 100644 index 00000000000..60cc61707b7 --- /dev/null +++ b/src/main/persistence/terminal-topology/unstarted-pane-withdraw.ts @@ -0,0 +1,30 @@ +import type { ExecutionHostId } from '../../../shared/execution-host' +import type { + CommandOf, + LayoutRefusalCode +} from '../../../shared/workspace-layout/workspace-layout-command-types' +import type { DurableProfileStateMutation } from '../loading-store/store-runtime-state' +import { withdrawPane, type TerminalTopologyCommitContext } from './terminal-topology-commit' + +/** Settled with bindings in the write queue: what keeps a pane whose terminal never started. */ +export type UnstartedPaneKept = 'bound' | 'starting' + +/** + * `withdrawPane` for a pane with no terminal: one that bound since ('bound') or whose start is under + * way ('starting') stays, and nothing is written. + */ +export function withdrawUnstartedPane( + hostId: ExecutionHostId, + pane: CommandOf<'closePane'>, + context: TerminalTopologyCommitContext, + stillStarting: () => boolean +): () => DurableProfileStateMutation { + const withdraw = withdrawPane(hostId, pane, context) + return () => { + const layout = context.getSession(hostId).terminalLayoutsByTabId?.[pane.tabId] + if (layout?.ptyIdsByLeafId?.[pane.leafId]) { + return { value: 'bound', persist: false } + } + return stillStarting() ? { value: 'starting', persist: false } : withdraw() + } +} diff --git a/src/main/runtime/runtime-durable-store-fixture.ts b/src/main/runtime/runtime-durable-store-fixture.ts index c566d35cc9b..c65c3a86a82 100644 --- a/src/main/runtime/runtime-durable-store-fixture.ts +++ b/src/main/runtime/runtime-durable-store-fixture.ts @@ -7,7 +7,10 @@ import { type PaneLayoutChange, type TerminalPaneAdmission } from '../persistence/terminal-topology/terminal-pane-admission' -import type { CommandOf } from '../../shared/workspace-layout/workspace-layout-command-types' +import type { + CommandOf, + LayoutRefusalCode +} from '../../shared/workspace-layout/workspace-layout-command-types' import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' import { clearReplacedPaneBinding } from '../persistence/loading-store/replaced-pane-binding' @@ -102,7 +105,17 @@ export function withRuntimePaneAdmission< return Object.assign(store, { admitTerminalPane: async (admission: TerminalPaneAdmission, hostId?: string) => commit(hostId, (session) => admitPaneToSession(resolveHostId(hostId), session, admission)), - withdrawTerminalPane: async (pane: CommandOf<'closePane'>, hostId?: string) => - commit(hostId, (session) => withdrawPaneFromSession(resolveHostId(hostId), session, pane)) + withdrawTerminalPane: async ( + pane: CommandOf<'closePane'>, + hostId?: string, + stillStarting?: () => boolean + ): Promise => + commit(hostId, (session) => + stillStarting && session.terminalLayoutsByTabId?.[pane.tabId]?.ptyIdsByLeafId?.[pane.leafId] + ? { value: 'bound' as const, session: null } + : stillStarting?.() + ? { value: 'starting' as const, session: null } + : withdrawPaneFromSession(resolveHostId(hostId), session, pane) + ) }) } diff --git a/src/main/runtime/runtime-pane-admission.test.ts b/src/main/runtime/runtime-pane-admission.test.ts index ad6fe476afa..662be255e88 100644 --- a/src/main/runtime/runtime-pane-admission.test.ts +++ b/src/main/runtime/runtime-pane-admission.test.ts @@ -49,6 +49,7 @@ describe('startInAdmittedPane', () => { ).rejects.toThrow('mount timed out') expect(store.withdrawTerminalPane).toHaveBeenCalledWith( { type: 'closePane', workspace: 'wt-1', tabId: 'tab-1', leafId: 'leaf-new' }, + undefined, undefined ) }) diff --git a/src/main/runtime/runtime-pane-admission.ts b/src/main/runtime/runtime-pane-admission.ts index d358e540fdd..1bf3d6ba9a4 100644 --- a/src/main/runtime/runtime-pane-admission.ts +++ b/src/main/runtime/runtime-pane-admission.ts @@ -3,6 +3,7 @@ import type { TerminalPaneAdmission, TerminalPaneAdmissionOutcome } from '../persistence/terminal-topology/terminal-pane-admission' +import type { LayoutRefusalCode } from '../../shared/workspace-layout/workspace-layout-command-types' import type { RuntimePtyController } from './runtime-pty-controller-contract' import type { RuntimeStore } from './runtime-store-contract' @@ -66,17 +67,24 @@ export async function withdrawRuntimePane( await withdrawAdmittedPane(store, admissionFor(args), hostIdOf(args)) } -/** Never the user's close: main stops no launch for it, unlike a window's closePane. */ -async function withdrawAdmittedPane( +/** + * Never the user's close: main stops no launch for it, unlike a window's closePane. With + * `stillStarting`, a pane whose terminal bound or is starting stays ('bound' / 'starting'). + */ +export async function withdrawAdmittedPane( store: RuntimeStore | null | undefined, admission: TerminalPaneAdmission, - hostId: string | undefined -): Promise { + hostId: string | undefined, + stillStarting?: () => boolean +): Promise { const leafId = admission.type === 'createTerminalTab' ? admission.leafId : admission.newLeafId - await store?.withdrawTerminalPane?.( - { type: 'closePane', workspace: admission.workspace, tabId: admission.tabId, leafId }, - hostId - ) + const pane = { + type: 'closePane' as const, + workspace: admission.workspace, + tabId: admission.tabId, + leafId + } + return store?.withdrawTerminalPane?.(pane, hostId, stillStarting) } /** Writes a pane the runtime asks for into main's copy; what fills it is the caller's. */ diff --git a/src/main/runtime/terminal/launch/terminal-split-layout-owner.test.ts b/src/main/runtime/terminal/launch/terminal-split-layout-owner.test.ts index d7a4fac5527..770175c23c9 100644 --- a/src/main/runtime/terminal/launch/terminal-split-layout-owner.test.ts +++ b/src/main/runtime/terminal/launch/terminal-split-layout-owner.test.ts @@ -8,6 +8,11 @@ import { } from '../../orca-runtime-test-fixtures.spec' import { createMobileCreateTestNotifier } from '../../orca-runtime-test-scenario-builders.spec' import { collectLayoutLeafIdsInOrder } from '../../../../shared/workspace-layout/terminal-pane-tree' +import { + clearPaneSpawnReservation, + makePaneSpawnReservationKey, + reservePaneSpawn +} from '../../../ipc/pty/pane/spawn-reservation' vi.mock('../../../../shared/layout-owner-switch', () => ({ ORCA_LAYOUT_OWNER_IS_MAIN: true })) @@ -35,6 +40,36 @@ function graph(leafIds: string[], unboundLeafIds: string[] = []) { } } +type RuntimeInternals = { + recordPtyWorktree: ( + ptyId: string, + worktreeId: string, + opts: { connected: boolean; tabId: string; paneKey: string } + ) => void +} + +function internalsOf(runtime: OrcaRuntimeService): RuntimeInternals { + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: recordPtyWorktree is declared on OrcaRuntimeService's inheritance chain. + return runtime as unknown as RuntimeInternals +} + +function graphWithSplit(newLeafId: string) { + const next = graph([HEADLESS_LEAF_ID, newLeafId]) + return { + ...next, + tabs: next.tabs.map((tab) => ({ + ...tab, + layout: { + type: 'split' as const, + direction: 'vertical' as const, + ratio: 0.5, + first: { type: 'leaf' as const, leafId: HEADLESS_LEAF_ID }, + second: { type: 'leaf' as const, leafId: newLeafId } + } + })) + } +} + describe('a split of a pane the window draws, in a layout-owner build', () => { function setup() { const store = makeRuntimeStoreWithWorkspaceSession(makeWorkspaceSessionWithHeadlessTerminal()) @@ -51,7 +86,7 @@ describe('a split of a pane the window draws, in a layout-owner build', () => { written.add(listener) return () => written.delete(listener) }) - const notifier = createMobileCreateTestNotifier(vi.fn()) + const notifier = { ...createMobileCreateTestNotifier(vi.fn()), closeTerminalPane: vi.fn() } const runtime = new OrcaRuntimeService(store.runtimeStore) runtime.setNotifier(notifier) runtime.syncWindowGraph(1, graph([HEADLESS_LEAF_ID])) @@ -60,6 +95,7 @@ describe('a split of a pane the window draws, in a layout-owner build', () => { collectLayoutLeafIdsInOrder(store.getSession().terminalLayoutsByTabId[TAB_ID]!.root!) return { ...store, + notifier, runtime, handle: handle!, splitTerminal: vi.mocked(notifier.splitTerminal), @@ -67,30 +103,92 @@ describe('a split of a pane the window draws, in a layout-owner build', () => { } } - it('writes the new pane in main before the window mounts it, and keeps it however the start goes', async () => { - const { runtime, runtimeStore, handle, splitTerminal, leafIds } = setup() - const withdraw = vi.fn(async () => null) - Reflect.set(runtimeStore, 'withdrawTerminalPane', withdraw) + it('writes the new pane in main before the window mounts it, and takes it back when its terminal never starts', async () => { + const { runtime, handle, splitTerminal, leafIds, notifier } = setup() vi.useFakeTimers() try { const split = runtime.splitTerminal(handle, { direction: 'vertical', command: 'make test' }) - const rejected = expect(split).rejects.toThrow('Timed out waiting for split pane handle') + const rejected = expect(split).rejects.toThrow( + /^split_pane_not_started: .*did not start within 10 s.*closed/ + ) await vi.waitFor(() => expect(splitTerminal).toHaveBeenCalledTimes(1)) const notice = splitTerminal.mock.calls[0]![2]! expect(notice).toMatchObject({ command: 'make test', sourceLeafId: HEADLESS_LEAF_ID }) expect(leafIds()).toEqual([HEADLESS_LEAF_ID, notice.newLeafId]) - // Its terminal never starts: main keeps a dead pane, as any, and takes nothing back. + // Its terminal never starts: main takes the pane back, and the window drops it too. await vi.advanceTimersByTimeAsync(11_000) await rejected - expect(leafIds()).toEqual([HEADLESS_LEAF_ID, notice.newLeafId]) - expect(withdraw).not.toHaveBeenCalled() + expect(leafIds()).toEqual([HEADLESS_LEAF_ID]) + expect(notifier.closeTerminalPane).toHaveBeenCalledWith(TAB_ID, notice.newLeafId) } finally { vi.useRealTimers() } }) + it('keeps a pane whose terminal bound before the wait ran out, though the window reported it late', async () => { + const { runtime, runtimeStore, handle, splitTerminal, leafIds, notifier } = setup() + + vi.useFakeTimers() + try { + const startedAt = Date.now() + const split = runtime.splitTerminal(handle, { direction: 'vertical' }) + await vi.waitFor(() => expect(splitTerminal).toHaveBeenCalledTimes(1)) + const newLeafId = splitTerminal.mock.calls[0]![2]!.newLeafId! + await vi.advanceTimersByTimeAsync(9_900 - (Date.now() - startedAt)) + // Main binds the started terminal at 9.9 s; the busy window's report lands after 10 s. + await runtimeStore.persistPtyBinding({ + worktreeId: TEST_WORKTREE_ID, + tabId: TAB_ID, + leafId: newLeafId, + ptyId: 'pty-1' + }) + await vi.advanceTimersByTimeAsync(600) + internalsOf(runtime).recordPtyWorktree('pty-1', TEST_WORKTREE_ID, { + connected: true, + tabId: TAB_ID, + paneKey: makePaneKey(TAB_ID, newLeafId) + }) + runtime.syncWindowGraph(1, graphWithSplit(newLeafId)) + await vi.advanceTimersByTimeAsync(0) + await expect(split).resolves.toMatchObject({ tabId: TAB_ID, leafId: newLeafId }) + expect(leafIds()).toEqual([HEADLESS_LEAF_ID, newLeafId]) + expect(notifier.closeTerminalPane).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('keeps the pane of a start still under way when the wait runs out, with the plain timeout', async () => { + const { runtime, handle, splitTerminal, leafIds, notifier } = setup() + const reservations: (() => void)[] = [] + + vi.useFakeTimers() + try { + const split = runtime.splitTerminal(handle, { direction: 'vertical' }) + const rejected = expect(split).rejects.toThrow(/^Timed out waiting for split pane handle$/) + await vi.waitFor(() => expect(splitTerminal).toHaveBeenCalledTimes(1)) + const newLeafId = splitTerminal.mock.calls[0]![2]!.newLeafId! + // A slow (SSH-style) start of the pane's terminal is under way and has not bound yet. + const reservationKey = makePaneSpawnReservationKey( + TEST_WORKTREE_ID, + 'ssh-1', + makePaneKey(TAB_ID, newLeafId) + )! + const reservation = reservePaneSpawn(reservationKey) + reservations.push(() => clearPaneSpawnReservation(reservationKey, reservation)) + + await vi.advanceTimersByTimeAsync(11_000) + await rejected + expect(leafIds()).toEqual([HEADLESS_LEAF_ID, newLeafId]) + expect(notifier.closeTerminalPane).not.toHaveBeenCalled() + } finally { + reservations.forEach((release) => release()) + vi.useRealTimers() + } + }) + it('closes a CLI split before its terminal starts, and the CLI hears so at once', async () => { const { runtime, handle, splitTerminal, leafIds } = setup() @@ -147,7 +245,7 @@ describe('a split of a pane the window draws, in a layout-owner build', () => { vi.useFakeTimers() try { const split = runtime.splitTerminal(handle, { direction: 'vertical' }) - const rejected = expect(split).rejects.toThrow('Timed out waiting for split pane handle') + const rejected = expect(split).rejects.toThrow('split_pane_not_started') await vi.waitFor(() => expect(splitTerminal).toHaveBeenCalledTimes(1)) // An unrelated write while the pane waits for its terminal. runtimeStore.setWorkspaceSession(runtimeStore.getWorkspaceSession()) diff --git a/src/main/runtime/terminal/launch/terminal-split-source-authority.ts b/src/main/runtime/terminal/launch/terminal-split-source-authority.ts index 9903891552d..50c516b9a88 100644 --- a/src/main/runtime/terminal/launch/terminal-split-source-authority.ts +++ b/src/main/runtime/terminal/launch/terminal-split-source-authority.ts @@ -13,6 +13,7 @@ import { } from '../../claude-agent-teams-shim-env' import { applyClaudeEnvPatch } from '../../../claude-accounts/environment' import { worktreeIdsEqual } from '../../../../shared/worktree/id' +import { SPLIT_PANE_WAIT_TIMEOUT_MESSAGE } from './terminal-split-wait-timeout' export abstract class OrcaRuntimeWithResolveTerminalSplitSourceAuthority extends OrcaRuntimeWithSplitPtyBackedTerminal { abstract getTerminalHandleForPaneKey( @@ -185,7 +186,7 @@ export abstract class OrcaRuntimeWithResolveTerminalSplitSourceAuthority extends if (idx !== -1) { this.graphSyncCallbacks.splice(idx, 1) } - reject(new Error('Timed out waiting for split pane handle')) + reject(new Error(SPLIT_PANE_WAIT_TIMEOUT_MESSAGE)) }, timeoutMs) const check = (): void => { diff --git a/src/main/runtime/terminal/launch/terminal-split-wait-timeout.ts b/src/main/runtime/terminal/launch/terminal-split-wait-timeout.ts new file mode 100644 index 00000000000..a9175c3e438 --- /dev/null +++ b/src/main/runtime/terminal/launch/terminal-split-wait-timeout.ts @@ -0,0 +1,2 @@ +/** waitForLeafInTab's timeout; layout-owner splits tell it apart from their own failures. */ +export const SPLIT_PANE_WAIT_TIMEOUT_MESSAGE = 'Timed out waiting for split pane handle' diff --git a/src/main/runtime/terminal/launch/terminal-split.ts b/src/main/runtime/terminal/launch/terminal-split.ts index 68ec8925a9d..22e58ed6432 100644 --- a/src/main/runtime/terminal/launch/terminal-split.ts +++ b/src/main/runtime/terminal/launch/terminal-split.ts @@ -6,10 +6,15 @@ import type { RuntimeTerminalSplit } from '../../../../shared/runtime-types' import { randomUUID } from 'node:crypto' import { parseExecutionHostId, type ExecutionHostId } from '../../../../shared/execution-host' import { ORCA_LAYOUT_OWNER_IS_MAIN } from '../../../../shared/layout-owner-switch' -import { admitRuntimePane } from '../../runtime-pane-admission' +import { admitRuntimePane, withdrawAdmittedPane } from '../../runtime-pane-admission' import type { RuntimeStore } from '../../runtime-store-contract' import type { Worktree } from '../../../../shared/worktree/types' import { collectLeafIds } from '../../../../shared/terminal-pane-layout-tree' +import { makePaneKey } from '../../../../shared/stable-pane-id' +import { isPaneSpawnInFlight } from '../../../ipc/pty/pane/spawn-reservation' +import { SPLIT_PANE_WAIT_TIMEOUT_MESSAGE } from './terminal-split-wait-timeout' + +const SPLIT_PANE_START_TIMEOUT_MS = 10_000 export abstract class OrcaRuntimeWithSplitTerminal extends OrcaRuntimeWithStopExplicitlyClosedTabPtys { protected abstract splitPtyBackedTerminal( @@ -52,8 +57,8 @@ export abstract class OrcaRuntimeWithSplitTerminal extends OrcaRuntimeWithStopEx const [hostId] = this.getWorkspaceSessionHostIdsForTab(leaf.worktreeId, leaf.tabId) if (hostId && parseExecutionHostId(hostId)?.kind !== 'runtime') { const newLeafId = randomUUID() - // Main keeps the pane however its start goes: one whose terminal never starts is a dead - // pane, as any. No pane written, no split: nothing else would put it in main's copy. + // No pane written, no split: nothing else would put it in main's copy. A pane whose + // terminal never starts is taken back below, so the CLI's failure is the truth. const outcome = await admitRuntimePane( this.store, { @@ -82,8 +87,50 @@ export abstract class OrcaRuntimeWithSplitTerminal extends OrcaRuntimeWithStopEx () => this.getWorkspaceSessionHostIdsForTab(leaf.worktreeId, leaf.tabId), leaf.tabId, newLeafId, - this.waitForLeafInTab(leaf.tabId, newLeafId) - ) + this.waitForLeafInTab(leaf.tabId, newLeafId, SPLIT_PANE_START_TIMEOUT_MS) + ).catch(async (error: unknown) => { + if ( + outcome !== 'admitted' || + !(error instanceof Error) || + error.message !== SPLIT_PANE_WAIT_TIMEOUT_MESSAGE + ) { + throw error + } + const taken = await withdrawAdmittedPane( + this.store, + { + type: 'splitPane', + workspace: leaf.worktreeId, + tabId: leaf.tabId, + leafId: leaf.leafId, + direction, + newLeafId + }, + hostId, + // Read in the write queue: a start under way (a slow SSH host) keeps its pane. + () => isPaneSpawnInFlight(leaf.worktreeId, makePaneKey(leaf.tabId, newLeafId)) + ).catch((withdrawError: unknown) => { + console.warn('[layout] an unstarted split pane stayed in main:', withdrawError) + return 'threw' as const + }) + if (taken === 'bound') { + // Bound just before the take-back: the window's late report brings its handle. + return settledByPaneClose( + this.store, + () => this.getWorkspaceSessionHostIdsForTab(leaf.worktreeId, leaf.tabId), + leaf.tabId, + newLeafId, + this.waitForLeafInTab(leaf.tabId, newLeafId, SPLIT_PANE_START_TIMEOUT_MS) + ) + } + if (taken !== null) { + throw error + } + this.notifier?.closeTerminalPane?.(leaf.tabId, newLeafId) + throw new Error( + `split_pane_not_started: the new pane's terminal did not start within ${SPLIT_PANE_START_TIMEOUT_MS / 1000} s, so the pane was closed${opts.command ? ' and its command was not run' : ''}` + ) + }) return { handle: newHandle, tabId: leaf.tabId, diff --git a/tests/e2e/terminal-cli-split-closed-before-mount.spec.ts b/tests/e2e/terminal-cli-split-closed-before-mount.spec.ts index 5a2eccc89a2..3a9676e7773 100644 --- a/tests/e2e/terminal-cli-split-closed-before-mount.spec.ts +++ b/tests/e2e/terminal-cli-split-closed-before-mount.spec.ts @@ -1,5 +1,5 @@ import { execFile } from 'node:child_process' -import { existsSync } from 'node:fs' +import { existsSync, mkdirSync } from 'node:fs' import path from 'node:path' import { promisify } from 'node:util' import type { Page } from '@stablyai/playwright-test' @@ -138,3 +138,69 @@ test('a CLI split whose tab closes before it mounts answers promptly', async ({ expect(outcome.ok ? '' : outcome.error).toContain('split_pane_closed') expect(elapsedMs).toBeLessThan(6_000) }) + +// The window is too busy to start the split's terminal within the CLI's 10 s: main takes the +// pane back, so the CLI's failure is the truth and the startup command never runs. +test('a CLI split whose terminal does not start within 10 s leaves no pane', async ({ + electronApp, + orcaPage +}, testInfo) => { + test.skip(!LAYOUT_OWNER_BUILD, 'layout-owner build only') + test.setTimeout(180_000) + await waitForSessionReady(orcaPage) + const worktreeId = await waitForActiveWorktree(orcaPage) + await ensureTerminalVisible(orcaPage) + await waitForActiveTerminalManager(orcaPage, 30_000) + const userDataDir = await electronApp.evaluate(({ app }) => app.getPath('userData')) + const client = new RuntimeClient(userDataDir, 30_000) + const tabId = await createActiveTab(orcaPage, worktreeId) + const source = await handleFor(client, worktreeId, tabId) + await parkHiddenTabBehindDecoy(orcaPage, worktreeId, tabId, { parkDelayMs: PARKING_DELAY_MS }) + await orcaPage.evaluate(() => { + window.addEventListener( + 'orca-background-mount-terminal-worktree', + () => { + const until = performance.now() + 11_000 + while (performance.now() < until) { + // Busy renderer. + } + }, + { once: true, capture: true } + ) + }) + // A file the command writes: a terminal that ran it and was then killed still leaves it. + const marker = testInfo.outputPath(`split-ran-${Date.now()}`) + mkdirSync(path.dirname(marker), { recursive: true }) + const outcome = await runCliSplit(userDataDir, source, `touch '${marker}'`) + // Past the stall: the window has handled the split and main's take-back. + await new Promise((resolve) => setTimeout(resolve, 5_000)) + const tabTerminals = await client + .call('terminal.list', { worktree: `id:${worktreeId}`, limit: 200 }) + .then((listed) => listed.result.terminals.filter((t) => t.tabId === tabId)) + const windowLeafIds = await orcaPage.evaluate((id) => { + const collect = (node: unknown): string[] => { + if (typeof node !== 'object' || node === null) { + return [] + } + if ('leafId' in node && typeof node.leafId === 'string') { + return [node.leafId] + } + return [ + ...('first' in node ? collect(node.first) : []), + ...('second' in node ? collect(node.second) : []) + ] + } + return collect(window.__store?.getState().terminalLayoutsByTabId[id]?.root) + }, tabId) + const report = { outcome, tabTerminals: tabTerminals.map((t) => t.leafId), windowLeafIds } + await testInfo.attach('split-taken-back.json', { + body: JSON.stringify(report, null, 2), + contentType: 'application/json' + }) + expect(outcome.ok ? '' : outcome.error).toContain( + "split_pane_not_started: the new pane's terminal did not start within 10 s, so the pane was closed and its command was not run" + ) + expect(tabTerminals).toHaveLength(1) + expect(windowLeafIds).toEqual([tabTerminals[0]!.leafId]) + expect(existsSync(marker)).toBe(false) +})