From e9fa0cf95c6f0ad21a84badb2fba910c82f6cf11 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Mon, 19 Oct 2026 15:41:18 -0400 Subject: [PATCH] fix: a tab close by the handle a split returned finds the new pane (dark, layout-owner build only) (#30786) * fix(renderer): list a pane main holds before the window mounts it, so a tab close by a split's handle finds it (dark, layout-owner build only) Co-Authored-By: Claude * test(e2e): the split-close spec polls through the host's "can't answer yet" terminal list Co-Authored-By: Claude * fix(runtime): a pane the window lists before main binds its terminal keeps main's terminal (dark, layout-owner build only) Co-Authored-By: Claude * fix(runtime): a tab close by a main-side split's handle stands on main's own record of the pane (dark, layout-owner build only) Replaces the window-side listing and the binding fill: the window's list lags main in three ways (built before main wrote the pane, from it before the binding, or before the window mounts it), and only main's own layout answers all three. Co-Authored-By: Claude * fix(runtime): main's relisting for a close tells no clients (dark, layout-owner build only) Co-Authored-By: Claude --------- Co-authored-by: E2E Test Co-authored-by: Claude --- .github/workflows/pr.yml | 2 +- .../pr-layout-owner-switch-on-route.mjs | 2 + ...close-by-split-handle-layout-owner.test.ts | 255 ++++++++++++++++++ .../view/layout-owner-close-relisting.ts | 66 +++++ .../terminal/view/mobile-terminal-surface.ts | 14 + ...inal-tab-close-restart-persistence.spec.ts | 23 +- 6 files changed, 355 insertions(+), 7 deletions(-) create mode 100644 src/main/runtime/session-lifecycle/close/close-by-split-handle-layout-owner.test.ts create mode 100644 src/main/runtime/terminal/view/layout-owner-close-relisting.ts diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 99408bd9dc5..ded057125a9 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -1594,7 +1594,7 @@ jobs: with: ref: ${{ github.sha }} # LAYOUT_OWNER_SWITCH_ON_E2E_SPECS in config/scripts/pr-layout-owner-switch-on-route.mjs. - test_files: '["tests/e2e/workspace-layout-oracle-headless.spec.ts","tests/e2e/workspace-layout-oracle.spec.ts","tests/e2e/cli-file-open-position.spec.ts","tests/e2e/relaunch-first-terminal.spec.ts","tests/e2e/relaunch-sleeping-agents-resume-on-view.spec.ts","tests/e2e/terminal-cold-activation-deferral.spec.ts","tests/e2e/cli-first-worktree-single-terminal.spec.ts","tests/e2e/unopened-workspace-agent-single-resume.spec.ts","tests/e2e/terminal-pane-close-layout-consistency.spec.ts","tests/e2e/golden-file-open-edit-save.spec.ts","tests/e2e/golden-source-control-open-diff.spec.ts","tests/e2e/paired-cli-terminal-graph-sync-tab-retention.spec.ts","tests/e2e/floating-workspace-pane-height.spec.ts","tests/e2e/editor-model-host-isolation.spec.ts","tests/e2e/editor-tab-selection-restore.spec.ts","tests/e2e/finished-agent-ghost-resume.spec.ts","tests/e2e/live-background-terminal-mount-authority.spec.ts","tests/e2e/workspace-back-forward-navigation.spec.ts","tests/e2e/worktree-lineage-toggle-shortcut.spec.ts","tests/e2e/local-cli-terminal-retention.spec.ts","tests/e2e/default-tabs-mark-crash-relaunch.spec.ts","tests/e2e/terminal-cli-split-closed-before-mount.spec.ts","tests/e2e/layout-stream-loss-reopen.spec.ts","tests/e2e/artificial-opencode-terminal-load.spec.ts","tests/e2e/ssh-orcad-"]' + test_files: '["tests/e2e/workspace-layout-oracle-headless.spec.ts","tests/e2e/workspace-layout-oracle.spec.ts","tests/e2e/cli-file-open-position.spec.ts","tests/e2e/relaunch-first-terminal.spec.ts","tests/e2e/relaunch-sleeping-agents-resume-on-view.spec.ts","tests/e2e/terminal-cold-activation-deferral.spec.ts","tests/e2e/cli-first-worktree-single-terminal.spec.ts","tests/e2e/unopened-workspace-agent-single-resume.spec.ts","tests/e2e/terminal-pane-close-layout-consistency.spec.ts","tests/e2e/golden-file-open-edit-save.spec.ts","tests/e2e/golden-source-control-open-diff.spec.ts","tests/e2e/paired-cli-terminal-graph-sync-tab-retention.spec.ts","tests/e2e/floating-workspace-pane-height.spec.ts","tests/e2e/editor-model-host-isolation.spec.ts","tests/e2e/editor-tab-selection-restore.spec.ts","tests/e2e/finished-agent-ghost-resume.spec.ts","tests/e2e/live-background-terminal-mount-authority.spec.ts","tests/e2e/workspace-back-forward-navigation.spec.ts","tests/e2e/worktree-lineage-toggle-shortcut.spec.ts","tests/e2e/local-cli-terminal-retention.spec.ts","tests/e2e/default-tabs-mark-crash-relaunch.spec.ts","tests/e2e/terminal-cli-split-closed-before-mount.spec.ts","tests/e2e/layout-stream-loss-reopen.spec.ts","tests/e2e/terminal-tab-close-restart-persistence.spec.ts","tests/e2e/artificial-opencode-terminal-load.spec.ts","tests/e2e/ssh-orcad-"]' run_changed_e2e: true needs_build: true artifact_suffix: -layout-owner-switch-on diff --git a/config/scripts/pr-layout-owner-switch-on-route.mjs b/config/scripts/pr-layout-owner-switch-on-route.mjs index 1b329223028..fc9fbc4fcc5 100644 --- a/config/scripts/pr-layout-owner-switch-on-route.mjs +++ b/config/scripts/pr-layout-owner-switch-on-route.mjs @@ -32,6 +32,8 @@ export const LAYOUT_OWNER_SWITCH_ON_E2E_SPECS = [ 'tests/e2e/default-tabs-mark-crash-relaunch.spec.ts', 'tests/e2e/terminal-cli-split-closed-before-mount.spec.ts', 'tests/e2e/layout-stream-loss-reopen.spec.ts', + // A tab close by the handle a split returned. + 'tests/e2e/terminal-tab-close-restart-persistence.spec.ts', // A split main first sees at its terminal binding keeps its direction (fails only at CI's window size). 'tests/e2e/artificial-opencode-terminal-load.spec.ts', // Every managed-SSH spec, in e2e.yml's Docker lane; its orcad template stays switch-off. diff --git a/src/main/runtime/session-lifecycle/close/close-by-split-handle-layout-owner.test.ts b/src/main/runtime/session-lifecycle/close/close-by-split-handle-layout-owner.test.ts new file mode 100644 index 00000000000..5725250eca2 --- /dev/null +++ b/src/main/runtime/session-lifecycle/close/close-by-split-handle-layout-owner.test.ts @@ -0,0 +1,255 @@ +import { describe, expect, it, vi } from 'vitest' +import { + withDurableRuntimeStore, + withRuntimePaneAdmission +} from '../../runtime-durable-store-fixture' +import { getDefaultWorkspaceSession } from '../../../../shared/constants' +import type { + RuntimeMobileSessionTabsSnapshot, + RuntimeMobileSessionTerminalTab +} from '../../../../shared/runtime-types' +import type { TerminalLayoutSnapshot } from '../../../../shared/terminal-tab-types' +import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' +import { OrcaRuntimeService } from '../../orca-runtime' + +vi.mock('../../../../shared/layout-owner-switch', () => ({ ORCA_LAYOUT_OWNER_IS_MAIN: true })) + +const REPO_ID = 'repo-1' +const WORKTREE_ID = `${REPO_ID}::/workspace` +const TAB_ID = 'tab-1' +const SOURCE_LEAF_ID = '11111111-1111-4111-8111-111111111111' +const SOURCE_PTY_ID = 'pty-source' +const SPLIT_PTY_ID = 'pty-split' + +function sourceLayout(): TerminalLayoutSnapshot { + return { + root: { type: 'leaf', leafId: SOURCE_LEAF_ID }, + activeLeafId: SOURCE_LEAF_ID, + expandedLeafId: null, + ptyIdsByLeafId: { [SOURCE_LEAF_ID]: SOURCE_PTY_ID } + } +} + +function persistedSession(): WorkspaceSessionState { + return { + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + [WORKTREE_ID]: [ + { + id: TAB_ID, + ptyId: SOURCE_PTY_ID, + worktreeId: WORKTREE_ID, + title: 'Terminal', + customTitle: null, + color: null, + sortOrder: 0, + createdAt: 1 + } + ] + }, + terminalLayoutsByTabId: { [TAB_ID]: sourceLayout() } + } +} + +function surface( + leafId: string, + ptyId: string | null, + parentLayout: TerminalLayoutSnapshot +): RuntimeMobileSessionTerminalTab { + return { + type: 'terminal', + id: `${TAB_ID}::${leafId}`, + parentTabId: TAB_ID, + leafId, + ptyId, + title: 'Terminal', + parentLayout, + isActive: leafId === SOURCE_LEAF_ID + } +} + +/** The window's own publication: one surface per pane it has mounted. */ +function windowSnapshot( + version: number, + surfaces: RuntimeMobileSessionTerminalTab[] +): RuntimeMobileSessionTabsSnapshot { + return { + worktree: WORKTREE_ID, + publicationEpoch: 'window', + snapshotVersion: version, + activeGroupId: 'group-1', + activeTabId: `${TAB_ID}::${SOURCE_LEAF_ID}`, + activeTabType: 'terminal', + tabGroups: [{ id: 'group-1', activeTabId: TAB_ID, tabOrder: [TAB_ID] }], + tabs: surfaces + } +} + +function windowGraph( + leaves: { leafId: string; ptyId: string | null }[], + snapshot: RuntimeMobileSessionTabsSnapshot +) { + return { + tabs: [ + { + tabId: TAB_ID, + worktreeId: WORKTREE_ID, + title: 'Terminal', + activeLeafId: SOURCE_LEAF_ID, + layout: null + } + ], + leaves: leaves.map(({ leafId, ptyId }, index) => ({ + tabId: TAB_ID, + worktreeId: WORKTREE_ID, + leafId, + paneRuntimeId: index + 1, + ptyId + })), + mobileSessionTabs: [snapshot] + } +} + +/** What the window's list holds for main's new pane when it lands, after main's own. */ +type WindowListing = 'pane-without-terminal' | 'no-new-pane' + +function setup(listing: WindowListing) { + let session = persistedSession() + const repo = { + id: REPO_ID, + path: '/workspace', + displayName: 'repo', + badgeColor: 'blue', + addedAt: 1 + } + const store = withDurableRuntimeStore( + withRuntimePaneAdmission({ + getRepos: () => [repo], + getRepo: (id: string) => (id === REPO_ID ? repo : undefined), + getWorkspaceSession: () => session, + setWorkspaceSession: (next: WorkspaceSessionState) => { + session = next + }, + persistPtyBinding: () => true, + flushOrThrow: () => undefined + }) + ) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the fake carries every store method a split and a tab close reach. + const runtime = new OrcaRuntimeService(store as never) + Object.assign(runtime, { + resolveTerminalWorkspaceLaunchScope: vi.fn(async () => ({ + id: WORKTREE_ID, + path: '/workspace', + connectionId: null, + repo, + folderWorkspace: null + })) + }) + runtime.setPtyController({ + // The real spawn writes the new pane's binding into main's session; the fake does the same. + spawn: vi.fn(async (args: { tabId?: string; leafId?: string }) => { + const layout = session.terminalLayoutsByTabId[args.tabId!]! + session = { + ...session, + terminalLayoutsByTabId: { + ...session.terminalLayoutsByTabId, + [args.tabId!]: { + ...layout, + ptyIdsByLeafId: { ...layout.ptyIdsByLeafId, [args.leafId!]: SPLIT_PTY_ID } + } + } + } + return { id: SPLIT_PTY_ID } + }), + write: () => true, + kill: () => true, + getForegroundProcess: async () => null + }) + // The window's list lags main: built before main's first frame (no new pane), or from it (the + // pane, but not yet its terminal). Either lands after main's own listing. + const revealTerminalSession = vi.fn(async (_worktreeId: string, args: { leafId?: string }) => { + const newLeafId = args.leafId! + const twoPanes = { + ...sourceLayout(), + root: { + type: 'split' as const, + direction: 'vertical' as const, + first: { type: 'leaf' as const, leafId: SOURCE_LEAF_ID }, + second: { type: 'leaf' as const, leafId: newLeafId } + } + } + runtime.syncWindowGraph( + 1, + windowGraph( + [{ leafId: SOURCE_LEAF_ID, ptyId: SOURCE_PTY_ID }], + windowSnapshot( + 2, + listing === 'no-new-pane' + ? [surface(SOURCE_LEAF_ID, SOURCE_PTY_ID, sourceLayout())] + : [surface(SOURCE_LEAF_ID, SOURCE_PTY_ID, twoPanes), surface(newLeafId, null, twoPanes)] + ) + ) + ) + return { tabId: TAB_ID } + }) + const notifier = { + revealTerminalSession, + splitTerminal: vi.fn(), + closeTerminalTab: vi.fn(async () => undefined) + } + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: the split and the tab close call only these notices. + runtime.setNotifier(notifier as never) + runtime.syncWindowGraph( + 1, + windowGraph( + [{ leafId: SOURCE_LEAF_ID, ptyId: SOURCE_PTY_ID }], + windowSnapshot(1, [surface(SOURCE_LEAF_ID, SOURCE_PTY_ID, sourceLayout())]) + ) + ) + runtime.registerPty(SOURCE_PTY_ID, WORKTREE_ID, null, { tabId: TAB_ID, leafId: SOURCE_LEAF_ID }) + // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: both members are declared on OrcaRuntimeService's inheritance chain. + const internals = runtime as unknown as { + issuePtyHandle: (pty: unknown) => string + ptysById: Map + } + // A terminal listing can name the source by its terminal, so the split runs main-side. + const handle = internals.issuePtyHandle(internals.ptysById.get(SOURCE_PTY_ID)) + // Main's layout drops the pane, as a close of it would. + const dropPane = (leafId: string): void => { + const layout = session.terminalLayoutsByTabId[TAB_ID]! + const { [leafId]: _dropped, ...ptyIdsByLeafId } = layout.ptyIdsByLeafId ?? {} + session = { + ...session, + terminalLayoutsByTabId: { + ...session.terminalLayoutsByTabId, + [TAB_ID]: { ...layout, root: sourceLayout().root, ptyIdsByLeafId } + } + } + } + return { runtime, handle, dropPane } +} + +describe("a tab close by a main-side split's handle while the window's list lags, in a layout-owner build", () => { + it.each(['pane-without-terminal', 'no-new-pane'])( + "closes the tab on main's own record of the pane (%s)", + async (listing) => { + const { runtime, handle } = setup(listing) + + const split = await runtime.splitTerminal(handle, { direction: 'vertical' }) + + await expect(runtime.closeTerminalTab(split.handle)).resolves.toMatchObject({ + tabId: TAB_ID, + closeMode: 'tab' + }) + } + ) + + it('still refuses once main no longer holds the pane', async () => { + const { runtime, handle, dropPane } = setup('no-new-pane') + + const split = await runtime.splitTerminal(handle, { direction: 'vertical' }) + dropPane(split.leafId!) + + await expect(runtime.closeTerminalTab(split.handle)).rejects.toThrow('terminal_handle_stale') + }) +}) diff --git a/src/main/runtime/terminal/view/layout-owner-close-relisting.ts b/src/main/runtime/terminal/view/layout-owner-close-relisting.ts new file mode 100644 index 00000000000..7a409d6f5a4 --- /dev/null +++ b/src/main/runtime/terminal/view/layout-owner-close-relisting.ts @@ -0,0 +1,66 @@ +import type { + RuntimePtyTabCloseAuthority, + RuntimePtyWorktreeRecord +} from '../../runtime-terminal-state-records' +import type { WorkspaceSessionState } from '../../../../shared/workspace-session-state-types' +import { makePaneKey, parsePaneKey } from '../../../../shared/stable-pane-id' +import { terminalLayoutContainsLeaf } from '../../../../shared/workspace-session-pane-ownership' +import type { ExecutionHostId } from '../../../../shared/execution-host' + +type RelistedPane = { + tabId: string + leafId: string + title: null + activate: false + selectIfNoActiveTab: false + /** The close's own record: clients keep the window's list, which catches up on its own. */ + notify: false +} + +/** + * Layout-owner builds: the pane main's own layout binds this close's terminal to, when the window's + * list lacks it. That list lags main (a split written, not yet drawn or bound), and a close by the + * terminal's handle must not depend on it. + */ +export function relistPaneMainBinds( + authority: RuntimePtyTabCloseAuthority, + pty: RuntimePtyWorktreeRecord | undefined, + host: { + hostId: (tabId: string) => ExecutionHostId | undefined + session: (hostId: ExecutionHostId) => WorkspaceSessionState | null | undefined + relist: (pty: RuntimePtyWorktreeRecord, pane: RelistedPane) => void + } +): void { + if ( + !pty?.connected || + pty.ptyId !== authority.ptyId || + pty.incarnationId !== authority.incarnationId || + pty.worktreeId !== authority.worktreeId + ) { + return + } + const pane = parsePaneKey(pty.paneKey ?? '') + const hostId = pane && pane.tabId === pty.tabId ? host.hostId(pane.tabId) : undefined + if (!pane || !hostId) { + return + } + const session = host.session(hostId) + const layout = session?.terminalLayoutsByTabId?.[pane.tabId] + const incarnationId = + session?.terminalPtyIncarnationsByPaneKey?.[makePaneKey(pane.tabId, pane.leafId)] + if ( + layout?.ptyIdsByLeafId?.[pane.leafId] !== pty.ptyId || + !terminalLayoutContainsLeaf(layout.root, pane.leafId) || + (incarnationId && incarnationId !== pty.incarnationId) + ) { + return + } + host.relist(pty, { + tabId: pane.tabId, + leafId: pane.leafId, + title: null, + activate: false, + selectIfNoActiveTab: false, + notify: false + }) +} diff --git a/src/main/runtime/terminal/view/mobile-terminal-surface.ts b/src/main/runtime/terminal/view/mobile-terminal-surface.ts index de9633613e9..6338637cb15 100644 --- a/src/main/runtime/terminal/view/mobile-terminal-surface.ts +++ b/src/main/runtime/terminal/view/mobile-terminal-surface.ts @@ -1,4 +1,6 @@ /* eslint-disable unicorn/no-useless-spread */ +import { ORCA_LAYOUT_OWNER_IS_MAIN } from '../../../../shared/layout-owner-switch' +import { relistPaneMainBinds } from './layout-owner-close-relisting' import { OrcaRuntimeWithCreateRuntimeOwnedMobileSessionTerminal } from '../launch/mobile-terminal-owned-create' import type { OrcaRuntimeWithResolveTerminalSplitSourceAuthority } from '../launch/terminal-split-source-authority' import type { @@ -135,6 +137,18 @@ export abstract class OrcaRuntimeWithWaitForMobileTerminalSurface extends OrcaRu ) { return null } + // Switch point: main binds the pane, so its own record stands while the window's list lags. + if ( + ORCA_LAYOUT_OWNER_IS_MAIN && + !this.findMobileTerminalSurfaceForPty(authority.worktreeId, authority.ptyId) + ) { + relistPaneMainBinds(authority, live.pty, { + hostId: (tabId) => this.getWorkspaceSessionHostIdsForTab(authority.worktreeId, tabId)[0], + session: (hostId) => this.store?.getWorkspaceSession?.(hostId), + relist: (pty, pane) => + this.publishPtyBackedMobileSessionTerminal(authority.worktreeId, pty, pane) + }) + } const surface = this.findMobileTerminalSurfaceForPty(authority.worktreeId, authority.ptyId) if (!surface) { return null diff --git a/tests/e2e/terminal-tab-close-restart-persistence.spec.ts b/tests/e2e/terminal-tab-close-restart-persistence.spec.ts index 20421dd1730..287317ec8cf 100644 --- a/tests/e2e/terminal-tab-close-restart-persistence.spec.ts +++ b/tests/e2e/terminal-tab-close-restart-persistence.spec.ts @@ -12,6 +12,7 @@ import { } from './helpers/store' import { attachRepoAndOpenTerminal, createRestartSession } from './helpers/orca-restart' import { worktreeRowSurface } from './worktree-row-locators' +import { readFreshTerminalInventory } from './helpers/terminal-inventory-observation' import { RuntimeClient } from '../../src/cli/runtime/client' import { RuntimeRpcFailureError } from '../../src/cli/runtime/types' import type { @@ -81,10 +82,17 @@ test('durable whole-tab close removes a split tab across restart', async (// oxl await expect .poll( async () => { - const listed = await client.call('terminal.list', { - worktree: `id:${worktreeId}` + // The pane's terminal can still be binding; the host then says it can't answer yet. + const listed = await readFreshTerminalInventory(async () => { + const response = await client.call('terminal.list', { + worktree: `id:${worktreeId}` + }) + return response.result }) - const matching = listed.result.terminals.filter( + if (!listed) { + return null + } + const matching = listed.terminals.filter( (terminal) => terminal.worktreeId === worktreeId && terminal.tabId === closedTabId ) activeHandle = matching.length === 1 ? (matching[0]?.handle ?? null) : null @@ -120,10 +128,13 @@ test('durable whole-tab close removes a split tab across restart', async (// oxl await expect .poll( async () => { - const afterClose = await client.call('terminal.list', { - worktree: `id:${worktreeId}` + const afterClose = await readFreshTerminalInventory(async () => { + const response = await client.call('terminal.list', { + worktree: `id:${worktreeId}` + }) + return response.result }) - return afterClose.result.terminals + return afterClose?.terminals .filter((terminal) => terminal.tabId === closedTabId) .map((terminal) => terminal.handle) },