From f6cc8f67f5e077a2d862519a4badf6dfca9f9099 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Mon, 19 Oct 2026 03:21:32 -0400 Subject: [PATCH] fix(layout): a pane main closed stays closed when its terminal starts late (dark, layout-owner build only) (#30560) --- ...sistence-closed-pane-binding-fence.test.ts | 225 ++++++++++++++++++ .../loading-store/pty-binding-refusals.ts | 15 +- .../closed-pane-binding-fence.test.ts | 102 ++++++++ .../closed-pane-binding-fence.ts | 166 +++++++++++++ .../terminal-topology-commit.ts | 63 +++-- 5 files changed, 549 insertions(+), 22 deletions(-) create mode 100644 src/main/persistence-closed-pane-binding-fence.test.ts create mode 100644 src/main/persistence/terminal-topology/closed-pane-binding-fence.test.ts create mode 100644 src/main/persistence/terminal-topology/closed-pane-binding-fence.ts diff --git a/src/main/persistence-closed-pane-binding-fence.test.ts b/src/main/persistence-closed-pane-binding-fence.test.ts new file mode 100644 index 00000000000..fdb0e51412e --- /dev/null +++ b/src/main/persistence-closed-pane-binding-fence.test.ts @@ -0,0 +1,225 @@ +import { closeTestStores, createStore, testState } from './persistence-test-harness' +import { mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { collectLayoutLeafIdsInOrder } from '../shared/workspace-layout/terminal-pane-tree' +import { TEST_LEAF_1, TEST_LEAF_2 } from './persistence-session-fixtures' +import { _resetClosedPaneFencesForTests } from './persistence/terminal-topology/closed-pane-binding-fence' +import { closeLeafOrTab } from './persistence/terminal-topology/terminal-topology-commit' +import { captureTerminalCloseRequest } from './runtime/terminal-surface-close' +import type { TerminalSurfaceCloseTarget } from '../shared/terminal-surface-close-target' +import { LOCAL_EXECUTION_HOST_ID, type ExecutionHostId } from '../shared/execution-host' + +vi.mock('electron', () => ({ + app: { getPath: () => testState.dir }, + safeStorage: { isEncryptionAvailable: () => false } +})) +vi.mock('../shared/layout-owner-switch', () => ({ ORCA_LAYOUT_OWNER_IS_MAIN: true })) + +const WORKTREE = 'repo1::/worktree' +const TAB = 'host-tab' + +type TestStore = ReturnType + +/** A tab main wrote and whose terminal started, then a split main wrote whose terminal is starting. */ +async function tabWithStartingSplit(store: TestStore): Promise { + expect( + await store.admitTerminalPane({ + type: 'createTerminalTab', + workspace: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1 + }) + ).toBe('admitted') + expect( + await store.persistPtyBinding({ + worktreeId: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + ptyId: 'source-pty', + mayCreate: false + }) + ).toBe(true) + expect( + await store.admitTerminalPane({ + type: 'splitPane', + workspace: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + direction: 'vertical', + newLeafId: TEST_LEAF_2 + }) + ).toBe('admitted') +} + +/** The window's spawn of the split's terminal committing late: it names no expected state. */ +function lateWindowBinding(store: TestStore): Promise { + return store.persistPtyBinding({ + worktreeId: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_2, + ptyId: 'late-pty', + origin: 'spawn' + }) +} + +/** Main's own close of a pane or tab (the CLI's, a phone's), through the one close transaction. */ +async function closeThroughMain( + store: TestStore, + target: TerminalSurfaceCloseTarget +): Promise { + const hostIds: readonly ExecutionHostId[] = [LOCAL_EXECUTION_HOST_ID] + const refusal = await store.runDurableMutation( + closeLeafOrTab({ + worktreeId: WORKTREE, + target, + options: { allowMissing: true }, + ...captureTerminalCloseRequest(WORKTREE, target, hostIds, store, false), + ownerMatches: () => true, + hostIds: () => hostIds, + getSession: (hostId) => store.getWorkspaceSession(hostId), + setSession: (session, hostId) => store.setWorkspaceSession(session, hostId), + onClosed: () => {} + }) + ) + expect(refusal).toBeUndefined() +} + +function leafIds(store: TestStore): string[] { + const root = store.getWorkspaceSession().terminalLayoutsByTabId[TAB]?.root + return root ? collectLayoutLeafIdsInOrder(root) : [] +} + +describe('a pane main closed or took back is not brought back by its late terminal binding', () => { + beforeEach(() => { + testState.dir = mkdtempSync(join(tmpdir(), 'orca-closed-pane-fence-')) + }) + + afterEach(async () => { + vi.useRealTimers() + _resetClosedPaneFencesForTests() + await closeTestStores() + rmSync(testState.dir, { recursive: true, force: true }) + }) + + it('refuses the binding of a split main took back when its terminal did not start in time', async () => { + const store = createStore() + await tabWithStartingSplit(store) + + await store.withdrawTerminalPane({ + type: 'closePane', + workspace: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_2 + }) + + expect(await lateWindowBinding(store)).toBe(false) + expect(leafIds(store)).toEqual([TEST_LEAF_1]) + }) + + it('refuses the binding of a pane the user closed while its terminal was starting', async () => { + const store = createStore() + await tabWithStartingSplit(store) + + const reply = await store.applyLayoutCommand({ + type: 'closePane', + workspace: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_2, + userPaneClose: true + }) + expect(reply.ok).toBe(true) + + expect(await lateWindowBinding(store)).toBe(false) + expect(leafIds(store)).toEqual([TEST_LEAF_1]) + }) + + it('refuses the binding of a pane main closed while its terminal was starting', async () => { + const store = createStore() + await tabWithStartingSplit(store) + + await closeThroughMain(store, { kind: 'pane', tabId: TAB, leafId: TEST_LEAF_2 }) + + expect(await lateWindowBinding(store)).toBe(false) + expect(leafIds(store)).toEqual([TEST_LEAF_1]) + }) + + it('refuses the binding of a pane whose tab the window closed while it was starting', async () => { + const store = createStore() + await tabWithStartingSplit(store) + + const reply = await store.applyLayoutCommand({ + type: 'closeTabs', + workspace: WORKTREE, + tabIds: [TAB], + force: true + }) + expect(reply.ok).toBe(true) + + expect(await lateWindowBinding(store)).toBe(false) + expect(store.getWorkspaceSession().terminalLayoutsByTabId[TAB]).toBeUndefined() + }) + + it('binds a pane the window creates again under the same id', async () => { + const store = createStore() + await tabWithStartingSplit(store) + await store.withdrawTerminalPane({ + type: 'closePane', + workspace: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_2 + }) + + const reply = await store.applyLayoutCommand({ + type: 'splitPane', + workspace: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + direction: 'vertical', + newLeafId: TEST_LEAF_2 + }) + expect(reply.ok).toBe(true) + expect(await lateWindowBinding(store)).toBe(true) + }) + + it('binds a pane a real create wrote again under the same id', async () => { + const store = createStore() + await tabWithStartingSplit(store) + await store.withdrawTerminalPane({ + type: 'closePane', + workspace: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_2 + }) + + expect( + await store.admitTerminalPane({ + type: 'splitPane', + workspace: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_1, + direction: 'vertical', + newLeafId: TEST_LEAF_2 + }) + ).toBe('admitted') + expect(await lateWindowBinding(store)).toBe(true) + expect(leafIds(store)).toEqual([TEST_LEAF_1, TEST_LEAF_2]) + }) + + it('lets a binding create the pane again once the binding window has passed', async () => { + vi.useFakeTimers({ toFake: ['Date'] }) + const store = createStore() + await tabWithStartingSplit(store) + await store.withdrawTerminalPane({ + type: 'closePane', + workspace: WORKTREE, + tabId: TAB, + leafId: TEST_LEAF_2 + }) + + vi.setSystemTime(Date.now() + 5 * 60_000 + 1_000) + expect(await lateWindowBinding(store)).toBe(true) + expect(leafIds(store)).toEqual([TEST_LEAF_1, TEST_LEAF_2]) + }) +}) diff --git a/src/main/persistence/loading-store/pty-binding-refusals.ts b/src/main/persistence/loading-store/pty-binding-refusals.ts index 47c895224a6..7bb43873d93 100644 --- a/src/main/persistence/loading-store/pty-binding-refusals.ts +++ b/src/main/persistence/loading-store/pty-binding-refusals.ts @@ -4,6 +4,8 @@ import type { WorkspaceSessionState } from '../../../shared/workspace-session-st import type { TerminalSessionPartition } from '../../../shared/workspace-layout/terminal-owner-invariants' import { layoutContainsLeafId } from '../restoring-sessions/terminal-layout-normalization' import type { PtyBindingSourceExpectation } from './store' +import { ORCA_LAYOUT_OWNER_IS_MAIN } from '../../../shared/layout-owner-switch' +import { isClosedPaneFenced } from '../terminal-topology/closed-pane-binding-fence' export type PtyBindingRefusalRequest = { tabId: string @@ -15,7 +17,7 @@ export type PtyBindingRefusalRequest = { } /** - * The five fences a binding must clear before anything is mutated, so a refusal leaves nothing + * The fences a binding must clear before anything is mutated, so a refusal leaves nothing * half-written. Order matters: every `false` here is returned before the write path or the * fast lane can run, which is what the relay's lease expiry and the stable-owner throw rely on. */ @@ -74,6 +76,17 @@ export function ptyBindingIsRefused( ) { return true } + // Switch point: a pane main closed or took back is not minted again by a start already under way. + if ( + ORCA_LAYOUT_OWNER_IS_MAIN && + !layoutContainsLeafId( + session.terminalLayoutsByTabId?.[args.tabId]?.root ?? null, + args.leafId + ) && + isClosedPaneFenced(paneKey) + ) { + return true + } // Mirrors the four creating branches of the write path — mint a tab, mint a root leaf, split // the root and graft a leaf, mint a layout — each of which sets `terminalMembershipChanged`. if ( diff --git a/src/main/persistence/terminal-topology/closed-pane-binding-fence.test.ts b/src/main/persistence/terminal-topology/closed-pane-binding-fence.test.ts new file mode 100644 index 00000000000..188b8db9f22 --- /dev/null +++ b/src/main/persistence/terminal-topology/closed-pane-binding-fence.test.ts @@ -0,0 +1,102 @@ +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { ExecutionHostId } from '../../../shared/execution-host' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import { getDefaultWorkspaceSession } from '../../../shared/constants' +import { + _resetClosedPaneFencesForTests, + fencedIfLayoutOwner, + isClosedPaneFenced, + partitionsFence +} from './closed-pane-binding-fence' + +vi.mock('../../../shared/layout-owner-switch', () => ({ ORCA_LAYOUT_OWNER_IS_MAIN: true })) + +const LOCAL: ExecutionHostId = 'local' +const SSH: ExecutionHostId = 'ssh:box' + +function sessionWith(tabId: string, leafIds: string[]): WorkspaceSessionState { + const [first, second] = leafIds + return { + ...getDefaultWorkspaceSession(), + terminalLayoutsByTabId: first + ? { + [tabId]: { + 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 partitions(initial: Partial>) { + const sessions = new Map(Object.entries(initial)) + return { + get: (hostId: ExecutionHostId) => sessions.get(hostId), + set: (hostId: ExecutionHostId, session: WorkspaceSessionState) => sessions.set(hostId, session) + } +} + +describe('the closed-pane fence', () => { + afterEach(() => _resetClosedPaneFencesForTests()) + + it('a write elsewhere that only keeps a fenced pane does not lift its fence', () => { + // An old relay reattach left the tab in both partitions; the SSH copy is taken back. + const store = partitions({ + [SSH]: sessionWith('t', ['a', 'b']), + [LOCAL]: sessionWith('t', ['a', 'b']) + }) + fencedIfLayoutOwner( + () => partitionsFence([SSH], store.get, true), + () => (store.set(SSH, sessionWith('t', ['a'])), { value: null }) + )() + expect(isClosedPaneFenced('t:b')).toBe(true) + + fencedIfLayoutOwner( + () => partitionsFence([LOCAL], store.get, false), + () => (store.set(LOCAL, sessionWith('t', ['a', 'b'])), { value: null }) + )() + expect(isClosedPaneFenced('t:b')).toBe(true) + }) + + it('a rolled-back close lifts the fence it wrote, and a rolled-back create restores the one it lifted', () => { + const store = partitions({ [LOCAL]: sessionWith('t', ['a', 'b']) }) + const close = fencedIfLayoutOwner( + () => partitionsFence([LOCAL], store.get, true), + () => (store.set(LOCAL, sessionWith('t', ['a'])), { value: null, rollback: () => {} }) + )() + expect(isClosedPaneFenced('t:b')).toBe(true) + close.rollback?.() + store.set(LOCAL, sessionWith('t', ['a', 'b'])) + expect(isClosedPaneFenced('t:b')).toBe(false) + + fencedIfLayoutOwner( + () => partitionsFence([LOCAL], store.get, true), + () => (store.set(LOCAL, sessionWith('t', ['a'])), { value: null }) + )() + const create = fencedIfLayoutOwner( + () => partitionsFence([LOCAL], store.get, false), + () => (store.set(LOCAL, sessionWith('t', ['a', 'b'])), { value: null, rollback: () => {} }) + )() + expect(isClosedPaneFenced('t:b')).toBe(false) + create.rollback?.() + expect(isClosedPaneFenced('t:b')).toBe(true) + }) + + it('records nothing for an unfenced command', () => { + const store = partitions({ [LOCAL]: sessionWith('t', ['a', 'b']) }) + fencedIfLayoutOwner( + () => null, + () => (store.set(LOCAL, sessionWith('t', ['a'])), { value: null }) + )() + expect(isClosedPaneFenced('t:b')).toBe(false) + }) +}) diff --git a/src/main/persistence/terminal-topology/closed-pane-binding-fence.ts b/src/main/persistence/terminal-topology/closed-pane-binding-fence.ts new file mode 100644 index 00000000000..4f754a149f0 --- /dev/null +++ b/src/main/persistence/terminal-topology/closed-pane-binding-fence.ts @@ -0,0 +1,166 @@ +// Panes main closed or took back, kept in memory so a terminal start already under way cannot bring +// one back by its late binding (pty-binding-session-update.ts mints a missing pane). Layout-owner +// build only: every caller is behind ORCA_LAYOUT_OWNER_IS_MAIN. + +import type { ExecutionHostId } from '../../../shared/execution-host' +import { ORCA_LAYOUT_OWNER_IS_MAIN } from '../../../shared/layout-owner-switch' +import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' +import { collectLayoutLeafIdsInOrder } from '../../../shared/workspace-layout/terminal-pane-tree' +import type { LayoutCommand } from '../../../shared/workspace-layout/workspace-layout-command-types' +import type { DurableProfileStateMutation } from '../loading-store/store-runtime-state' + +/** + * Why 5 min: past the longest a start can wait before it binds. An SSH start may first wait on a + * credential prompt (SSH_CREDENTIAL_TIMEOUT_MS, 120 s) and a connect (CONNECT_TIMEOUT_MS, 30 s, + * both ssh-connection-utils.ts), then on its relay request (REQUEST_TIMEOUT_MS, 30 s, + * ssh-channel-multiplexer.ts); the local daemon admits a spawn within 12 s + * (MAX_SPAWN_ADMISSION_WINDOW_MS, daemon-terminal-admission.ts). Pane ids are never reused, so a + * longer fence refuses nothing legitimate; the bound only limits memory. + */ +export const CLOSED_PANE_BINDING_FENCE_MS = 5 * 60_000 + +const closedAtByPaneKey = new Map() + +type SessionsRead = { + hostIds: () => readonly ExecutionHostId[] + getSession: (hostId: ExecutionHostId) => WorkspaceSessionState | null | undefined +} + +type Fence = { sessions: SessionsRead; fences: boolean } + +function paneKeysIn( + hostIds: readonly ExecutionHostId[], + getSession: SessionsRead['getSession'] +): Set { + const keys = new Set() + for (const hostId of hostIds) { + const layouts = getSession(hostId)?.terminalLayoutsByTabId ?? {} + for (const [tabId, layout] of Object.entries(layouts)) { + if (layout?.root) { + for (const leafId of collectLayoutLeafIdsInOrder(layout.root)) { + keys.add(`${tabId}:${leafId}`) + } + } + } + } + return keys +} + +/** + * Switch point: in a layout-owner build, `mutate` runs through `fencingRemovedPanes` with what `fence` + * names (null: unfenced). Otherwise it is `mutate` itself; `fence` is never called. + */ +export function fencedIfLayoutOwner( + fence: () => Fence | null, + mutate: () => DurableProfileStateMutation +): () => DurableProfileStateMutation { + if (!ORCA_LAYOUT_OWNER_IS_MAIN) { + return mutate + } + return () => { + const plan = fence() + return plan ? fencingRemovedPanes(plan.sessions, plan.fences, mutate) : mutate() + } +} + +/** A write to these partitions: `fences` when it is a close or take-back, else it only lifts. */ +export function partitionsFence( + hostIds: readonly ExecutionHostId[], + getSession: SessionsRead['getSession'], + fences: boolean +): Fence { + return { sessions: { hostIds: () => hostIds, getSession }, fences } +} + +/** A window or client layout command: its closes fence, its terminal creates lift; others neither. */ +export function layoutCommandFence( + type: LayoutCommand['type'], + hostIds: readonly ExecutionHostId[], + getSession: SessionsRead['getSession'] +): Fence | null { + if (type === 'closePane' || type === 'closeTabs') { + return partitionsFence(hostIds, getSession, true) + } + if (type === 'createTerminalTab' || type === 'splitPane') { + return partitionsFence(hostIds, getSession, false) + } + return null +} + +/** + * Runs a write of main's, then fences the panes it removed when `fences` (a close or take-back) and + * lifts the fence of each pane it newly wrote: a real create under the same id binds as any. A + * rolled-back write puts back the fences it changed. + */ +function fencingRemovedPanes( + sessions: SessionsRead, + fences: boolean, + write: () => DurableProfileStateMutation +): DurableProfileStateMutation { + // The partitions as they were before: a tab's close can drop the partition list it came from. + const hostIds = [...sessions.hostIds()] + const before = paneKeysIn(hostIds, sessions.getSession) + const result = write() + const after = paneKeysIn(hostIds, sessions.getSession) + const now = Date.now() + const changed = new Map() + const set = (paneKey: string, closedAt: number | undefined): void => { + if (!changed.has(paneKey)) { + changed.set(paneKey, closedAtByPaneKey.get(paneKey)) + } + if (closedAt === undefined) { + closedAtByPaneKey.delete(paneKey) + } else { + closedAtByPaneKey.set(paneKey, closedAt) + } + } + for (const [paneKey, closedAt] of closedAtByPaneKey) { + if ( + (after.has(paneKey) && !before.has(paneKey)) || + now - closedAt >= CLOSED_PANE_BINDING_FENCE_MS + ) { + set(paneKey, undefined) + } + } + if (fences) { + for (const paneKey of before) { + if (!after.has(paneKey)) { + set(paneKey, now) + } + } + } + if (changed.size === 0 || !result.rollback) { + return result + } + const rollback = result.rollback + return { + ...result, + rollback: () => { + rollback() + for (const [paneKey, closedAt] of changed) { + if (closedAt === undefined) { + closedAtByPaneKey.delete(paneKey) + } else { + closedAtByPaneKey.set(paneKey, closedAt) + } + } + } + } +} + +/** Whether main closed or took back this pane within the binding window. */ +export function isClosedPaneFenced(paneKey: string): boolean { + const closedAt = closedAtByPaneKey.get(paneKey) + if (closedAt === undefined) { + return false + } + if (Date.now() - closedAt >= CLOSED_PANE_BINDING_FENCE_MS) { + closedAtByPaneKey.delete(paneKey) + return false + } + return true +} + +export function _resetClosedPaneFencesForTests(): void { + closedAtByPaneKey.clear() +} diff --git a/src/main/persistence/terminal-topology/terminal-topology-commit.ts b/src/main/persistence/terminal-topology/terminal-topology-commit.ts index a706d8d50a1..ecb32f34564 100644 --- a/src/main/persistence/terminal-topology/terminal-topology-commit.ts +++ b/src/main/persistence/terminal-topology/terminal-topology-commit.ts @@ -34,6 +34,11 @@ import { writeRestorable } from './restorable-write' import { assignWorkspaceSessionPartition } from './terminal-topology-membership' import { keepWslMoveFrozenSessionsForStore } from '../moving-wsl-projects/wsl-move-session-freeze' import { resolveWindowPaneDetach, type WindowPaneDetachCommand } from './window-pane-detach' +import { + fencedIfLayoutOwner, + layoutCommandFence, + partitionsFence +} from './closed-pane-binding-fence' // The commit boundary for terminal layout (tabs, panes, pane-to-PTY bindings). Wraps the close and // the pane move; the close transform still lives in runtime/ and other writers move here later. @@ -52,7 +57,11 @@ export function closeLeafOrTab( ): () => DurableProfileStateMutation { return traced( commit.target.kind === 'pane' ? 'close_leaf' : 'close_tab', - terminalSurfaceCloseMutation(commit), + // A start under way in a pane main closed must not bring it back. + fencedIfLayoutOwner( + () => ({ sessions: commit, fences: true }), + terminalSurfaceCloseMutation(commit) + ), // Refusals are fixed reason codes, never ids. (refusal) => refusal?.message ) @@ -81,12 +90,16 @@ export function admitPane( ): () => DurableProfileStateMutation { return traced( 'admit_pane', - () => - commitPaneChange( - hostId, - admitPaneToSession(hostId, context.getSession(hostId), admission), - context - ), + // A pane main writes again under a closed pane's id binds as any. + fencedIfLayoutOwner( + () => partitionsFence([hostId], context.getSession, false), + () => + commitPaneChange( + hostId, + admitPaneToSession(hostId, context.getSession(hostId), admission), + context + ) + ), (outcome) => (outcome === 'admitted' || outcome === 'exists' ? undefined : outcome) ) } @@ -99,12 +112,16 @@ export function withdrawPane( ): () => DurableProfileStateMutation { return traced( 'withdraw_pane', - () => - commitPaneChange( - hostId, - withdrawPaneFromSession(hostId, context.getSession(hostId), pane), - context - ), + // A start under way in a pane main took back must not bring it back. + fencedIfLayoutOwner( + () => partitionsFence([hostId], context.getSession, true), + () => + commitPaneChange( + hostId, + withdrawPaneFromSession(hostId, context.getSession(hostId), pane), + context + ) + ), (code) => code ?? undefined ) } @@ -120,14 +137,18 @@ export function applyLayout( ): () => DurableProfileStateMutation { return traced( 'apply_layout', - () => - command.type === 'movePaneToNewTab' && command.windowDetach - ? commitWindowPaneDetach( - hostIds, - { ...command, windowDetach: command.windowDetach }, - context - ) - : commitLayoutCommand(hostIds, command, context), + // Closes fence the panes they removed; creates lift the fence on the ids they write. + fencedIfLayoutOwner( + () => layoutCommandFence(command.type, hostIds, context.getSession), + () => + command.type === 'movePaneToNewTab' && command.windowDetach + ? commitWindowPaneDetach( + hostIds, + { ...command, windowDetach: command.windowDetach }, + context + ) + : commitLayoutCommand(hostIds, command, context) + ), (outcome) => (outcome.ok ? undefined : outcome.code) ) }