From 6c4797ca9f06dd74bd00edb4fb02a91c893a68b4 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Thu, 3 Sep 2026 21:21:22 -0700 Subject: [PATCH] perf(runtime): stop the expired-SSH-lease sweep from rescanning every tab layout (#18409) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(runtime): stop the expired-SSH-lease sweep from rescanning every tab layout The `runtime:syncWindowGraph` IPC handler is the most expensive thing the main process does: measured on a real session it costs 20.7 ms per call at 0.71 calls/sec, which is 1.47% of wall and ~17% of all main-thread JS. 76% of that sits in one subtree: `getHydrationTargets` -> `hasRuntimeOwnedPtyCandidate` -> `getRecentExpiredSshLease` -> `findTerminalTabIdForLeaf`. Three pieces of pure waste, none of which change an answer: 1. `getRecentExpiredSshLease` evaluated its cheapest and most selective filter LAST. `SSH_PANE_RECOVERY_GRACE_MS` is 30 s, so nearly every stored expired lease fails it — but only after the predicate had already resolved the lease's leaf to its current tab, which is the expensive part. The freshness and reattach-eligibility gates now run first; the predicate is otherwise identical and side-effect free, so the selected lease is unchanged. 2. The sweep ran once per tab. `workspaceSessionWorktreeHasRuntimeOwnedPtyCandidate` asked "does a recent expired lease name THIS tab" for every tab in a worktree, and each ask re-read and re-filtered the whole lease list. It now resolves the worktree's recoverable tab ids once, lazily, so a worktree whose first tab already owns a serve/SSH pty still never sweeps. 3. `findTerminalTabIdForLeaf` allocated a `Set` and walked a whole pane tree per tab to answer one leaf lookup. It now reads a leafId -> tabId index built once per layouts record and reused until a layout object is replaced, which keeps first-tab-wins ordering identical. Measured by replaying a real 414-worktree / 801-tab / 137-lease session: 2.51 ms -> 0.27 ms per publish for this subtree, a 9.3x cut. No user-facing trade-off: same leases selected, same tabs reported recoverable, same SSH pane recovery affordance. * fix(runtime): revalidate the leaf membership index on root identity persistPtyBinding grafts a leaf by assigning `layout.root` on the SAME layout object inside the SAME layouts record, so the layout-identity revalidation kept serving an index blind to the grafted leaf and findTerminalTabIdForLeaf answered `undefined` where the pre-index linear scan answered the tab. That fed the SSH reattach fence (restoreReattachedPtyRuntime) and the expired-lease pane recovery resolver, both of which then fall back to the frozen lease tabId. Membership is a pure function of the root tree and no writer mutates a node in place, so root identity is the exact revalidation key — same O(tabs) pointer compare, no new cap, cadence or staleness window. * perf(runtime): resolve a leaf's tab by scan instead of a cached membership index Fix #3 of this PR cached a leafId -> tabId map per layouts record and revalidated it by comparing every root reference on every read. It was the only mutable cross-call state in the change, the only piece carrying a staleness invariant, and it had already needed one follow-up fix (1c23c544) after a layout-identity key turned out to be blind to `persistPtyBinding`'s in-place `layout.root` graft. The index was never what produced the measured win. After fix #1 moves the freshness gate first, the reporter's replay never calls `findTerminalTabIdForLeaf` at all — every stored expired lease is older than the 30 s recovery grace, so the entire 24.9 ms -> 0.9 ms comes from fixes #1 and #2, both of which are unchanged. `findTerminalTabIdForLeaf` is now an allocation-free scan over the existing `layoutContainsLeafId`, which short-circuits on the first matching leaf instead of materialising a Set per tab. Same answers, same first-tab-in-record-order semantics, no revalidation key, nothing for a writer to invalidate. Re-measured on the same 414-worktree / 801-tab / 137-lease replay (process.cpuUsage deltas, median of 3; wall clock is useless on this box): scenario main index scan all leases stale (replay) 24.86 0.87 0.88 ms/publish one lease inside the grace 24.31 1.08 1.04 ms/publish all 137 inside the grace 18.04 3.71 4.31 ms/publish The measured win is unchanged. Only the synthetic worst case — every one of 137 leases expiring inside the same 30 s window — pays for the cache's absence, and even there the two ranges overlap because the index's own revalidation is O(tabs) per lookup. Removes 208 net lines. `terminal-leaf-tab-resolution.test.ts` keeps the parity cases and adds the guard the cache needed: a subtree replaced in place after an earlier read must be visible to the next one. That test fails against the index. * docs(runtime): say why the leaf scan keeps Object.keys 'Allocation-free' overstated it — Object.keys does allocate one key array. A guarded for...in trades that for a hasOwn call per tab and measures slower, so record the reason the next reader does not re-litigate it. --- ...ce-pty-binding-leaf-tab-resolution.test.ts | 86 ++++++++++++++++++ .../expired-ssh-lease-pane-candidacy.test.ts | 47 ++++++++++ ...le-headless-mobile-session-browser-tabs.ts | 90 +++++++++++++------ .../terminal-leaf-tab-resolution.test.ts | 62 +++++++++++++ ...e-session-terminal-membership-authority.ts | 16 +++- 5 files changed, 271 insertions(+), 30 deletions(-) create mode 100644 src/main/persistence-pty-binding-leaf-tab-resolution.test.ts create mode 100644 src/main/runtime/terminal-leaf-tab-resolution.test.ts diff --git a/src/main/persistence-pty-binding-leaf-tab-resolution.test.ts b/src/main/persistence-pty-binding-leaf-tab-resolution.test.ts new file mode 100644 index 00000000000..2f0dc41daaf --- /dev/null +++ b/src/main/persistence-pty-binding-leaf-tab-resolution.test.ts @@ -0,0 +1,86 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest' +import { rmSync, mkdtempSync } from 'node:fs' +import { join } from 'node:path' +import { tmpdir } from 'node:os' +import { getDefaultWorkspaceSession } from '../shared/constants' +import { findTerminalTabIdForLeaf } from './runtime/workspace-session-terminal-membership-authority' +import { testState, createStore, makeTerminalTab } from './persistence-test-harness' +import { TEST_LEAF_1, TEST_LEAF_2 } from './persistence-session-fixtures' + +vi.mock('electron', () => ({ + app: { getPath: () => testState.dir }, + safeStorage: { isEncryptionAvailable: () => false } +})) + +vi.mock('./telemetry/client', () => ({ track: vi.fn() })) +vi.mock('./telemetry/cohort-classifier', () => ({ getCohortAtEmit: () => ({}) })) + +describe('findTerminalTabIdForLeaf after persistPtyBinding grafts a leaf', () => { + beforeEach(() => { + testState.dir = mkdtempSync(join(tmpdir(), 'orca-test-')) + }) + afterEach(() => { + rmSync(testState.dir, { recursive: true, force: true }) + }) + + // `persistPtyBinding` grafts the leaf by assigning `layout.root` on the SAME layout object inside + // the SAME layouts record (pty-binding-persistence.ts), so the resolver has to answer from the + // tree that is there now, not from anything derived on an earlier call. + it('resolves a leaf grafted in place by a split spawn', async () => { + const store = await createStore() + store.setWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-source' })] + }, + terminalLayoutsByTabId: { + tab1: { + root: { type: 'leaf', leafId: TEST_LEAF_1 }, + activeLeafId: TEST_LEAF_1, + expandedLeafId: null, + ptyIdsByLeafId: { [TEST_LEAF_1]: 'pty-source' } + } + } + }) + // A reader runs first, exactly as the syncWindowGraph lease sweep does. + expect(findTerminalTabIdForLeaf(store.getWorkspaceSession(), TEST_LEAF_1)).toBe('tab1') + + expect( + store.persistPtyBinding({ + worktreeId: 'wt1', + tabId: 'tab1', + leafId: TEST_LEAF_2, + ptyId: 'pty-split' + }) + ).toBe(true) + + expect(findTerminalTabIdForLeaf(store.getWorkspaceSession(), TEST_LEAF_2)).toBe('tab1') + expect(findTerminalTabIdForLeaf(store.getWorkspaceSession(), TEST_LEAF_1)).toBe('tab1') + }) + + // The other in-place graft: an empty persisted layout gets its first durable root. + it('resolves the first leaf grafted onto an empty layout', async () => { + const store = await createStore() + store.setWorkspaceSession({ + ...getDefaultWorkspaceSession(), + tabsByWorktree: { + wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: null })] + }, + terminalLayoutsByTabId: { + tab1: { root: null, activeLeafId: null, expandedLeafId: null, ptyIdsByLeafId: {} } + } + }) + expect(findTerminalTabIdForLeaf(store.getWorkspaceSession(), TEST_LEAF_1)).toBeUndefined() + + expect( + store.persistPtyBinding({ + worktreeId: 'wt1', + tabId: 'tab1', + leafId: TEST_LEAF_1, + ptyId: 'pty-first' + }) + ).toBe(true) + + expect(findTerminalTabIdForLeaf(store.getWorkspaceSession(), TEST_LEAF_1)).toBe('tab1') + }) +}) diff --git a/src/main/runtime/expired-ssh-lease-pane-candidacy.test.ts b/src/main/runtime/expired-ssh-lease-pane-candidacy.test.ts index 82986a4b3fd..3d8157c9a28 100644 --- a/src/main/runtime/expired-ssh-lease-pane-candidacy.test.ts +++ b/src/main/runtime/expired-ssh-lease-pane-candidacy.test.ts @@ -12,6 +12,12 @@ const TARGET = 'ssh-target' const TAB_ID = 'tab-candidacy' type LeaseReader = { + workspaceSessionWorktreeHasRuntimeOwnedPtyCandidate: ( + session: { terminalLayoutsByTabId?: Record }, + worktreeId: string, + tabs: { id: string; ptyId: string | null }[] + ) => boolean + collectRecentExpiredSshLeaseTabIds: (worktreeId: string) => ReadonlySet getRecentExpiredSshLease: ( worktreeId: string, tabId: string, @@ -87,4 +93,45 @@ describe('recent expired SSH lease candidacy', () => { ) expect(reader.hasRecentExpiredSshLeasePane(TEST_WORKTREE_ID, pane)).toBe(true) }) + + it('collects the same tabs the per-tab reader reports, in one sweep of the leases', () => { + const leases = [leaseFor('pty-1', { supersededBy: 'pty-2' }), leaseFor('pty-2')] + let sweeps = 0 + const reader = new OrcaRuntimeService({ + ...store, + getSshRemotePtyLeases: () => { + sweeps += 1 + return leases + } + }) as unknown as LeaseReader + const tabs = Array.from({ length: 8 }, (_, index) => ({ + id: index === 7 ? TAB_ID : `tab-${index}`, + ptyId: null + })) + + expect( + reader.workspaceSessionWorktreeHasRuntimeOwnedPtyCandidate( + { terminalLayoutsByTabId: {} }, + TEST_WORKTREE_ID, + tabs + ) + ).toBe(true) + // One sweep answers all eight tabs; the per-tab reader used to sweep once per tab. + expect(sweeps).toBe(1) + expect([...reader.collectRecentExpiredSshLeaseTabIds(TEST_WORKTREE_ID)]).toEqual([TAB_ID]) + }) + + it('reports no candidate when no lease names any of the worktree tabs', () => { + const reader = readerWithLeases([ + leaseFor('pty-1', { tabId: 'somewhere-else', leafId: undefined }) + ]) + + expect( + reader.workspaceSessionWorktreeHasRuntimeOwnedPtyCandidate( + { terminalLayoutsByTabId: {} }, + TEST_WORKTREE_ID, + [{ id: TAB_ID, ptyId: null }] + ) + ).toBe(false) + }) }) diff --git a/src/main/runtime/orca-runtime-reconcile-headless-mobile-session-browser-tabs.ts b/src/main/runtime/orca-runtime-reconcile-headless-mobile-session-browser-tabs.ts index c4687c96d14..a9f377e5a7b 100644 --- a/src/main/runtime/orca-runtime-reconcile-headless-mobile-session-browser-tabs.ts +++ b/src/main/runtime/orca-runtime-reconcile-headless-mobile-session-browser-tabs.ts @@ -82,17 +82,24 @@ export class OrcaRuntimeWithReconcileHeadlessMobileSessionBrowserTabs extends Or worktreeId: string, tabs: WorkspaceSessionState['tabsByWorktree'][string] ): boolean { + // Why resolved lazily and reused: the per-tab question is the same lease sweep with a + // different tabId, so asking it once per worktree answers every tab. Kept lazy so a + // worktree whose first tab already owns a serve/SSH pty never sweeps at all. + let recoverableTabIds: ReadonlySet | undefined return tabs.some((tab) => { if (this.isServeOrSshOwnedPtyId(tab.ptyId)) { return true } const leafPtyIds = session.terminalLayoutsByTabId?.[tab.id]?.ptyIdsByLeafId - return ( - (leafPtyIds && - Object.values(leafPtyIds).some((ptyId) => this.isServeOrSshOwnedPtyId(ptyId))) || - // Why: expiry keeps pane coordinates so paired viewers can request a fresh shell. - this.getRecentExpiredSshLease(worktreeId, tab.id, undefined) !== null - ) + if ( + leafPtyIds && + Object.values(leafPtyIds).some((ptyId) => this.isServeOrSshOwnedPtyId(ptyId)) + ) { + return true + } + // Why: expiry keeps pane coordinates so paired viewers can request a fresh shell. + recoverableTabIds ??= this.collectRecentExpiredSshLeaseTabIds(worktreeId) + return recoverableTabIds.has(tab.id) }) } @@ -128,6 +135,54 @@ export class OrcaRuntimeWithReconcileHeadlessMobileSessionBrowserTabs extends Or ) } + /** + * Why eligibility belongs in the selection, not after it: a pane accumulates leases as it + * re-leases under new relay ids, so `(worktreeId, tabId, leafId)` names several. A superseded or + * relay-id-recycled predecessor is `expired` for a reason that already names its successor, and + * the unqualified callers use this answer to decide a pane is still recoverable — reporting one + * would offer paired viewers a recovery `recoverTerminalPane` then refuses. Picking the first + * ELIGIBLE orphan also keeps a predecessor from shadowing the successor that is genuinely + * reattachable. + */ + private isRecentExpiredSshLeaseForWorktree( + lease: ReturnType>[number], + worktreeId: string, + now: number + ): boolean { + return ( + lease.state === 'expired' && + lease.worktreeId === worktreeId && + sshRemotePtyLeaseAllowsReattach(lease) && + lease.updatedAt <= now && + now - lease.updatedAt <= SSH_PANE_RECOVERY_GRACE_MS + ) + } + + /** + * Leaf is the pane's identity; the frozen tabId is only trustworthy while nothing else can say + * where the leaf actually lives. + */ + private resolveExpiredSshLeaseTabId( + lease: ReturnType>[number] + ): string { + const currentTabId = lease.leafId + ? this.findCurrentTerminalTabIdForLeaf(lease.targetId, lease.leafId) + : undefined + return currentTabId ?? lease.tabId + } + + /** The tabs a recent eligible expired lease still names, resolved in one sweep of the leases. */ + protected collectRecentExpiredSshLeaseTabIds(worktreeId: string): ReadonlySet { + const now = Date.now() + const tabIds = new Set() + for (const lease of this.store?.getSshRemotePtyLeases?.() ?? []) { + if (this.isRecentExpiredSshLeaseForWorktree(lease, worktreeId, now)) { + tabIds.add(this.resolveExpiredSshLeaseTabId(lease)) + } + } + return tabIds + } + protected getRecentExpiredSshLease( worktreeId: string, tabId: string, @@ -137,34 +192,17 @@ export class OrcaRuntimeWithReconcileHeadlessMobileSessionBrowserTabs extends Or const now = Date.now() return ( this.store?.getSshRemotePtyLeases?.().find((lease) => { - if (lease.state !== 'expired' || lease.worktreeId !== worktreeId) { + if (!this.isRecentExpiredSshLeaseForWorktree(lease, worktreeId, now)) { return false } - // Why eligibility belongs in the selection, not after it: a pane accumulates leases as it - // re-leases under new relay ids, so `(worktreeId, tabId, leafId)` names several. A - // superseded or relay-id-recycled predecessor is `expired` for a reason that already names - // its successor, and the unqualified callers use this answer to decide a pane is still - // recoverable — reporting one would offer paired viewers a recovery `recoverTerminalPane` - // then refuses. Picking the first ELIGIBLE orphan also keeps a predecessor from shadowing - // the successor that is genuinely reattachable. - if (!sshRemotePtyLeaseAllowsReattach(lease)) { - return false - } - // Leaf is the pane's identity; the frozen tabId is only trustworthy while nothing else can - // say where the leaf actually lives. - const currentTabId = lease.leafId - ? this.findCurrentTerminalTabIdForLeaf(lease.targetId, lease.leafId) - : undefined return ( - (currentTabId ?? lease.tabId) === tabId && + this.resolveExpiredSshLeaseTabId(lease) === tabId && // Leases store RELAY form (`toStoredPtyId` -> `toRelaySshPtyId`); the runtime hands us // the APP form (`ssh:@@pty-3`). A raw `===` therefore never held for an SSH // pane, which is what kept this reader's only ptyId-qualified caller inert. (ptyId === undefined || lease.ptyId === toComparableRelaySshPtyId(lease.targetId, ptyId)) && - (leafId === undefined || lease.leafId === undefined || lease.leafId === leafId) && - lease.updatedAt <= now && - now - lease.updatedAt <= SSH_PANE_RECOVERY_GRACE_MS + (leafId === undefined || lease.leafId === undefined || lease.leafId === leafId) ) }) ?? null ) diff --git a/src/main/runtime/terminal-leaf-tab-resolution.test.ts b/src/main/runtime/terminal-leaf-tab-resolution.test.ts new file mode 100644 index 00000000000..2fe8a6d0344 --- /dev/null +++ b/src/main/runtime/terminal-leaf-tab-resolution.test.ts @@ -0,0 +1,62 @@ +import { describe, expect, it } from 'vitest' + +import type { TerminalLayoutSnapshot } from '../../shared/terminal-tab-types' +import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' +import { findTerminalTabIdForLeaf } from './workspace-session-terminal-membership-authority' + +function layout(...leafIds: string[]): TerminalLayoutSnapshot { + let root = { type: 'leaf' as const, leafId: leafIds[0] } + for (const leafId of leafIds.slice(1)) { + root = { + type: 'split', + direction: 'row', + first: root, + second: { type: 'leaf' as const, leafId } + } as never + } + return { root, activeLeafId: leafIds[0], ptyIdsByLeafId: {} } as TerminalLayoutSnapshot +} + +function session(layouts: Record): WorkspaceSessionState { + return { terminalLayoutsByTabId: layouts } as WorkspaceSessionState +} + +describe('findTerminalTabIdForLeaf', () => { + it('resolves every leaf of a split tree to its tab', () => { + const state = session({ + 'tab-a': layout('leaf-1', 'leaf-2', 'leaf-3'), + 'tab-b': layout('leaf-4') + }) + expect(findTerminalTabIdForLeaf(state, 'leaf-2')).toBe('tab-a') + expect(findTerminalTabIdForLeaf(state, 'leaf-3')).toBe('tab-a') + expect(findTerminalTabIdForLeaf(state, 'leaf-4')).toBe('tab-b') + }) + + it('keeps the first tab in record order when two layouts claim one leaf', () => { + const layouts = { 'tab-a': layout('shared'), 'tab-b': layout('shared') } + expect(findTerminalTabIdForLeaf(session(layouts), 'shared')).toBe('tab-a') + }) + + it('answers misses, empty sessions and empty layouts with undefined', () => { + expect(findTerminalTabIdForLeaf(undefined, 'leaf-1')).toBeUndefined() + expect(findTerminalTabIdForLeaf(session({}), 'leaf-1')).toBeUndefined() + expect(findTerminalTabIdForLeaf(session({ 'tab-a': layout('leaf-1') }), 'nope')).toBeUndefined() + }) + + // The guard a leafId -> tabId cache needed and this scan does not: membership is read from the + // tree that is there NOW. `persistPtyBinding` grafts leaves by assigning into a layout already in + // the record, so anything memoized across calls has to be revalidated against every mutation + // shape a writer can produce - including one that leaves the root node's identity untouched. + it('reflects a subtree replaced in place after an earlier read', () => { + const tracked = layout('leaf-1', 'leaf-2') + const state = session({ 'tab-a': tracked }) + expect(findTerminalTabIdForLeaf(state, 'leaf-2')).toBe('tab-a') + expect(findTerminalTabIdForLeaf(state, 'leaf-9')).toBeUndefined() + + const root = tracked.root as { second: unknown } + root.second = { type: 'leaf', leafId: 'leaf-9' } + + expect(findTerminalTabIdForLeaf(state, 'leaf-9')).toBe('tab-a') + expect(findTerminalTabIdForLeaf(state, 'leaf-2')).toBeUndefined() + }) +}) diff --git a/src/main/runtime/workspace-session-terminal-membership-authority.ts b/src/main/runtime/workspace-session-terminal-membership-authority.ts index 4c85f46b0c8..2869e3c813c 100644 --- a/src/main/runtime/workspace-session-terminal-membership-authority.ts +++ b/src/main/runtime/workspace-session-terminal-membership-authority.ts @@ -5,6 +5,7 @@ import type { } from '../../shared/terminal-tab-types' import type { WorkspaceSessionState } from '../../shared/workspace-session-state-types' import { getRepoIdFromWorktreeId } from '../../shared/worktree/id' +import { layoutContainsLeafId } from '../persistence/restoring-sessions/terminal-layout-normalization' import { pruneTabGroupLayoutAfterRetirement } from './mobile-session-terminal-retirement' function collectLeafIds(node: TerminalPaneLayoutNode | null, ids: Set): void { @@ -164,15 +165,22 @@ export function advanceTerminalTopologyRevision( * The tab whose live layout holds this leaf. Only the leaf half of a pane key is remint-stable — * `detachTerminalPaneToTab` moves a live pane into a new tab, so a stored tabId names the tab the * pane left. Callers fencing on location must resolve it here rather than trust a frozen tabId. + * + * Stateless on purpose: writers graft leaves by assigning into a layout that is already inside the + * layouts record, so any cache here would need a revalidation key that is itself O(tabs) per read — + * the same cost as this walk, with a staleness invariant to keep. `Object.keys` over a guarded + * `for...in` is deliberate too: the key array is cheaper than a `hasOwn` call per tab (measured). */ export function findTerminalTabIdForLeaf( session: WorkspaceSessionState | undefined, leafId: string ): string | undefined { - for (const [tabId, layout] of Object.entries(session?.terminalLayoutsByTabId ?? {})) { - const leafIds = new Set() - collectLeafIds(layout.root, leafIds) - if (leafIds.has(leafId)) { + const layouts = session?.terminalLayoutsByTabId + if (!layouts) { + return undefined + } + for (const tabId of Object.keys(layouts)) { + if (layoutContainsLeafId(layouts[tabId]?.root ?? null, leafId)) { return tabId } }