mirror of
https://github.com/stablyai/orca.git
synced 2026-09-26 16:02:43 +00:00
fix(persistence): keep the tab row on its first pane when a sibling pane binds
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, since a remount reattaches the tab to whatever the row says. Main overwrote it with whichever pane was binding, and the renderer's next publish put it back, so every sibling reattach was a state change and could never take the fast lane. On the real profile that is 38% of panes. Rewrite the row only when it names nothing useful: null, the PTY this leaf is replacing, or a PTY no leaf holds. The fast-lane predicate compares against the same rule.
This commit is contained in:
Jinjing
Jinwoo-H
@@ -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
|
||||
|
||||
@@ -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'
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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'
|
||||
)
|
||||
})
|
||||
})
|
||||
@@ -0,0 +1,28 @@
|
||||
import type { TerminalTab } from '../../../shared/terminal-tab-types'
|
||||
|
||||
type LeafPtyIds = Readonly<Record<string, string>> | 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<TerminalTab, 'ptyId'>,
|
||||
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
|
||||
}
|
||||
Reference in New Issue
Block a user