diff --git a/orca-persistence-design-assessment.md b/orca-persistence-design-assessment.md index 86981873731..7101b492e05 100644 --- a/orca-persistence-design-assessment.md +++ b/orca-persistence-design-assessment.md @@ -16,8 +16,6 @@ The binding write now has one flush and rollback boundary. Session mutation does - Validation: 931 tests passed and one opt-in metadata benchmark skipped; a subsequent targeted run passed 80 tests, including SSH reattach and terminal-close continuity. Node typecheck, targeted lint, formatting, and whitespace checks passed. - Coverage: host partition behavior is tested for local, SSH, and paired runtimes; folder-workspace and binding-recovery tests are included in the persistence suite. PTY I/O, WSL process execution, platform launch policy, and wire formats are unchanged by the review fixes. No live platform matrix or before/after typing-latency measurement was collected. -Concurrent edits began adding call-origin instrumentation after this validation. That work is separate from these review fixes; implementation descriptions below about omitted origin metadata need reconciliation when those edits settle. - ## Problem Every terminal pane that mounts or remounts calls `Store.persistPtyBinding`, which clones the workspace session, mutates it, and calls `flushOrThrow`. The flush serializes the whole persisted state (9.2 MB on the measured install) on the Electron main thread, then compares its hash to the last written hash and usually skips the disk write. The serialization is paid whether or not the write happens. @@ -38,20 +36,26 @@ The check runs after the four existing refusal checks (`expectedSourceBinding`, All of the following must hold. Any miss falls through to the existing code unchanged. -| Condition | Why | -| ------------------------------------------------------------------------------------------------------------------------------------ | ---------------------------------------------------------------------------------- | -| `args.expectedSourceBinding === undefined` | The split path always changes membership and arms the topology fence. | -| `isTerminalLeafId(args.leafId)` | Legacy leaf ids take the early flush branch and never write layout state. | -| Tab exists in `session.tabsByWorktree[bindingWorktreeId]` with `tab.id === args.tabId` and `tab.ptyId === args.ptyId` | Otherwise the call mints a tab. | -| `session.terminalLayoutsByTabId[args.tabId]` exists, `layout.root` is non-null, and `layoutContainsLeafId(layout.root, args.leafId)` | Otherwise the call mints or splits the layout. | -| `layout.ptyIdsByLeafId?.[args.leafId] === args.ptyId` | The load-bearing binding. | -| `session.terminalPtyIncarnationsByPaneKey?.[paneKey] === args.incarnationId` | Strict equality: undefined on both sides is a match; undefined on one side is not. | -| `args.expectedBinding === undefined \|\| args.expectedBinding.incarnationId === args.incarnationId` | A reconciled incarnation must still bump the topology fence. | -| `!session.terminalSurfaceTombstonesByPaneKey?.[paneKey]` | A tombstone is cleared by the write path; it is state the call would change. | -| Binding is durable (next section) | In-memory equality alone can match a binding still waiting in the debounced save. | +| Condition | Why | +| --------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------- | +| `args.expectedSourceBinding === undefined` | The split path always changes membership and arms the topology fence. | +| `isTerminalLeafId(args.leafId)` | Legacy leaf ids take the early flush branch and never write layout state. | +| Tab exists in `session.tabsByWorktree[bindingWorktreeId]` with `tab.id === args.tabId`, and `tab.ptyId` already equals what `tabRowPtyIdAfterLeafBinding` would write | Otherwise the call mints a tab or rewrites the row. See "The tab row" below. | +| `session.terminalLayoutsByTabId[args.tabId]` exists, `layout.root` is non-null, and `layoutContainsLeafId(layout.root, args.leafId)` | Otherwise the call mints or splits the layout. | +| `layout.ptyIdsByLeafId?.[args.leafId] === args.ptyId` | The load-bearing binding. | +| `session.terminalPtyIncarnationsByPaneKey?.[paneKey] === args.incarnationId` | Strict equality: undefined on both sides is a match; undefined on one side is not. | +| `args.expectedBinding === undefined \|\| args.expectedBinding.incarnationId === args.incarnationId` | A reconciled incarnation must still bump the topology fence. | +| `!session.terminalSurfaceTombstonesByPaneKey?.[paneKey]` | A tombstone is cleared by the write path; it is state the call would change. | +| Binding is durable (next section) | In-memory equality alone can match a binding still waiting in the debounced save. | When the predicate holds, `return true`. The `true` return matters: `persistAdmittedStablePaneBinding` in `src/main/ipc/pty/pane/stable-owner.ts` throws `terminal_pane_owner_changed` on `false`, and `spawn-commit-persist.ts` uses the `true` result to suppress its second binding write. +### The tab row + +A tab row names one PTY, but a split tab holds several panes. The renderer keeps the row on the first pane and refuses to let later split-pane spawns steal it, because a remount reattaches the tab to whatever the row says. Until this change the main-process write path overwrote the row with whichever pane was binding, and the renderer's next session publish put the first pane back. On the dev profile that ping-pong was the sole reason all four reattach-shaped calls in the first 22-span capture fell through: they matched on layout, leaf PTY, and incarnation and missed only on `tab_pty`. On the real profile 310 of 1,424 terminal tabs are split, holding 674 of 1,764 panes, so 38% of remounts could never have hit the fast lane. + +`terminal-tab-pty-ownership.ts` holds the rule both sides now follow. The row is rewritten only when it names nothing useful: it is null, it points at the PTY this leaf is replacing, or it names a PTY no leaf of the layout holds. A sibling pane's bind leaves it alone. The predicate compares the row against what that rule would write, so a sibling reattach counts as a match. Every main-process reader of the row already falls back to the per-leaf map, so none depends on it naming the most recent pane. The two existing tests that pin a null row being filled stay valid. + ### Durability check ```ts diff --git a/src/main/persistence-flush-and-save-scheduling.test.ts b/src/main/persistence-flush-and-save-scheduling.test.ts index 05ec95edeb7..d054d6180ae 100644 --- a/src/main/persistence-flush-and-save-scheduling.test.ts +++ b/src/main/persistence-flush-and-save-scheduling.test.ts @@ -14,7 +14,7 @@ import { makeRepo, makeTerminalTab } from './persistence-test-harness' -import { TEST_LEAF_1 } from './persistence-session-fixtures' +import { TEST_LEAF_1, TEST_LEAF_2 } from './persistence-session-fixtures' import { getDefaultPersistedState, getDefaultWorkspaceSession } from '../shared/constants' import type { WorkspaceSessionState } from '../shared/workspace-session-state-types' import { _resetTracerForTests, setActiveSink } from './observability/tracer' @@ -582,6 +582,40 @@ describe('Store', () => { expect(flushSpy).not.toHaveBeenCalled() }) + it('lets every pane of a split tab hit the fast lane', async () => { + const store = await createStore() + store.setWorkspaceSession( + boundSession({ + terminalLayoutsByTabId: { + tab1: { + root: { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: TEST_LEAF_1 }, + second: { type: 'leaf', leafId: TEST_LEAF_2 } + }, + activeLeafId: TEST_LEAF_2, + expandedLeafId: null, + ptyIdsByLeafId: { [TEST_LEAF_1]: 'pty-1', [TEST_LEAF_2]: 'pty-2' } + } + } + }) + ) + const sibling = { ...binding, leafId: TEST_LEAF_2, ptyId: 'pty-2' } + // First remount after a cold park: both panes reattach back to back. + expect(store.persistPtyBinding(binding)).toBe(true) + expect(store.persistPtyBinding(sibling)).toBe(true) + expect(store.getWorkspaceSession().tabsByWorktree?.[WORKTREE]?.[0]?.ptyId).toBe('pty-1') + const flushSpy = vi.spyOn(store, 'flushOrThrow') + + // Second remount: neither pane may rewrite the tab row, so neither flushes. + expect(store.persistPtyBinding(sibling)).toBe(true) + expect(store.persistPtyBinding(binding)).toBe(true) + + expect(flushSpy).not.toHaveBeenCalled() + expect(store.getWorkspaceSession().tabsByWorktree?.[WORKTREE]?.[0]?.ptyId).toBe('pty-1') + }) + it('resolves the SSH partition without re-pointing it', async () => { const store = await createStore() const hostId = 'ssh:ssh-1' diff --git a/src/main/persistence/loading-store/pty-binding-fast-lane.test.ts b/src/main/persistence/loading-store/pty-binding-fast-lane.test.ts index 52306a1bb78..8a1ca3788d7 100644 --- a/src/main/persistence/loading-store/pty-binding-fast-lane.test.ts +++ b/src/main/persistence/loading-store/pty-binding-fast-lane.test.ts @@ -100,6 +100,33 @@ describe('evaluatePtyBindingFastLane', () => { ).toEqual(['tombstone']) }) + it('accepts a sibling pane whose tab row names the first pane', () => { + const LEAF_B = '22222222-2222-4222-8222-222222222222' + const state = session({ + terminalLayoutsByTabId: { + tab1: { + root: { + type: 'split', + direction: 'vertical', + first: { type: 'leaf', leafId: LEAF }, + second: { type: 'leaf', leafId: LEAF_B } + }, + activeLeafId: LEAF_B, + expandedLeafId: null, + ptyIdsByLeafId: { [LEAF]: 'pty-1', [LEAF_B]: 'pty-2' } + } + } + }) + expect( + evaluatePtyBindingFastLane( + { ...request, leafId: LEAF_B, ptyId: 'pty-2' }, + state, + WORKTREE, + true + ) + ).toEqual({ eligible: true, misses: [] }) + }) + it('accepts a matching incarnation and a reconciled-to-same expected binding', () => { const state = session({ terminalPtyIncarnationsByPaneKey: { [paneKey]: 'a' } }) expect( diff --git a/src/main/persistence/loading-store/pty-binding-fast-lane.ts b/src/main/persistence/loading-store/pty-binding-fast-lane.ts index 142c5726131..287a9412885 100644 --- a/src/main/persistence/loading-store/pty-binding-fast-lane.ts +++ b/src/main/persistence/loading-store/pty-binding-fast-lane.ts @@ -1,6 +1,7 @@ import { isTerminalLeafId } from '../../../shared/stable-pane-id' import type { WorkspaceSessionState } from '../../../shared/workspace-session-state-types' import { layoutContainsLeafId } from '../restoring-sessions/terminal-layout-normalization' +import { tabRowPtyIdAfterLeafBinding } from './terminal-tab-pty-ownership' /** * Why a reattach can be ineligible. `not_durable` alone means memory already matched but the @@ -56,12 +57,14 @@ export function evaluatePtyBindingFastLane( const tab = session.tabsByWorktree?.[bindingWorktreeId]?.find( (candidate) => candidate.id === args.tabId ) + const layout = session.terminalLayoutsByTabId?.[args.tabId] if (!tab) { misses.push('tab_missing') - } else if (tab.ptyId !== args.ptyId) { + } else if ( + tab.ptyId !== tabRowPtyIdAfterLeafBinding(tab, layout?.ptyIdsByLeafId, args.leafId, args.ptyId) + ) { misses.push('tab_pty') } - const layout = session.terminalLayoutsByTabId?.[args.tabId] if (!layout || !layout.root) { misses.push('layout_missing') } else { diff --git a/src/main/persistence/loading-store/pty-binding-persistence.ts b/src/main/persistence/loading-store/pty-binding-persistence.ts index 3ae036fe44d..27d62c8602b 100644 --- a/src/main/persistence/loading-store/pty-binding-persistence.ts +++ b/src/main/persistence/loading-store/pty-binding-persistence.ts @@ -19,6 +19,7 @@ import { resolveHostId } from './session-host-partitions' import { evaluatePtyBindingFastLane } from './pty-binding-fast-lane' import { ptyBindingIsRefused } from './pty-binding-refusals' import { startPtyBindingSpan, type PtyBindingOrigin } from './pty-binding-span' +import { tabRowPtyIdAfterLeafBinding } from './terminal-tab-pty-ownership' type PtyBindingPersistenceOperationsRuntime = Pick< StoreRuntimeState, @@ -188,7 +189,12 @@ function applyPtyBinding( const tabs = session.tabsByWorktree?.[bindingWorktreeId] const tab = tabs?.find((t) => t.id === args.tabId) if (tab) { - tab.ptyId = args.ptyId + tab.ptyId = tabRowPtyIdAfterLeafBinding( + tab, + session.terminalLayoutsByTabId?.[args.tabId]?.ptyIdsByLeafId, + args.leafId, + args.ptyId + ) } else { terminalMembershipChanged = true hostAdmittedTabCreated = args.hostAdmittedMembership === true diff --git a/src/main/persistence/loading-store/terminal-tab-pty-ownership.test.ts b/src/main/persistence/loading-store/terminal-tab-pty-ownership.test.ts new file mode 100644 index 00000000000..cb266d1b16a --- /dev/null +++ b/src/main/persistence/loading-store/terminal-tab-pty-ownership.test.ts @@ -0,0 +1,42 @@ +import { describe, expect, it } from 'vitest' +import { tabRowPtyIdAfterLeafBinding } from './terminal-tab-pty-ownership' + +const LEAF_A = 'leaf-a' +const LEAF_B = 'leaf-b' + +describe('tabRowPtyIdAfterLeafBinding', () => { + it('fills a null row', () => { + expect(tabRowPtyIdAfterLeafBinding({ ptyId: null }, undefined, LEAF_A, 'pty-1')).toBe('pty-1') + expect(tabRowPtyIdAfterLeafBinding({ ptyId: null }, {}, LEAF_A, 'pty-1')).toBe('pty-1') + }) + + it('follows a respawn of the leaf the row already names', () => { + expect( + tabRowPtyIdAfterLeafBinding({ ptyId: 'pty-1' }, { [LEAF_A]: 'pty-1' }, LEAF_A, 'pty-1b') + ).toBe('pty-1b') + }) + + it('leaves the row on the first pane when a sibling pane binds', () => { + expect( + tabRowPtyIdAfterLeafBinding( + { ptyId: 'pty-1' }, + { [LEAF_A]: 'pty-1', [LEAF_B]: 'pty-2' }, + LEAF_B, + 'pty-2' + ) + ).toBe('pty-1') + // The sibling's first bind, before its leaf is in the map, must not steal the row either. + expect( + tabRowPtyIdAfterLeafBinding({ ptyId: 'pty-1' }, { [LEAF_A]: 'pty-1' }, LEAF_B, 'pty-2') + ).toBe('pty-1') + }) + + it('reclaims a row that names a PTY no leaf holds', () => { + expect( + tabRowPtyIdAfterLeafBinding({ ptyId: 'pty-gone' }, { [LEAF_A]: 'pty-1' }, LEAF_B, 'pty-2') + ).toBe('pty-2') + expect(tabRowPtyIdAfterLeafBinding({ ptyId: 'pty-gone' }, undefined, LEAF_A, 'pty-1')).toBe( + 'pty-1' + ) + }) +}) diff --git a/src/main/persistence/loading-store/terminal-tab-pty-ownership.ts b/src/main/persistence/loading-store/terminal-tab-pty-ownership.ts new file mode 100644 index 00000000000..1004dd27593 --- /dev/null +++ b/src/main/persistence/loading-store/terminal-tab-pty-ownership.ts @@ -0,0 +1,28 @@ +import type { TerminalTab } from '../../../shared/terminal-tab-types' + +type LeafPtyIds = Readonly> | undefined + +/** + * A tab row names one PTY, but a split tab holds several panes. The renderer keeps the row on + * the first pane and refuses to let later split-pane spawns steal it (see terminal-pty-bindings.ts), + * because a remount reattaches the tab to whatever the row says. Main must agree, or every + * sibling pane's reattach rewrites the row and the two sides ping-pong forever. + * + * The row is rewritten only when it names nothing useful: it is null, it points at the PTY this + * very leaf is replacing, or it names a PTY no leaf of the layout holds any more. + */ +export function tabRowPtyIdAfterLeafBinding( + tab: Pick, + ptyIdsByLeafId: LeafPtyIds, + leafId: string, + ptyId: string +): string { + const current = tab.ptyId + if (current === null || current === ptyIdsByLeafId?.[leafId]) { + return ptyId + } + const heldByAnotherLeaf = Object.entries(ptyIdsByLeafId ?? {}).some( + ([otherLeafId, otherPtyId]) => otherLeafId !== leafId && otherPtyId === current + ) + return heldByAnotherLeaf ? current : ptyId +}