mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 08:03:37 +00:00
refactor(ssh): delete the pane-binding fold; its premise was false
The migration moved legacy `ssh:<target>` pane bindings into `local` and cleared them, on the theory that the ssh partition was a stale spill of the desktop plane's state. Investigation of both hypotheses the team raised disproved that: - NOT cross-version compat. The introducing commit says it is for CONCURRENT multi-host. Nothing about the partition crosses the wire (zero references under src/main/runtime/rpc/), and the payload that does cross the SSH boundary — RemoteWorkspaceSnapshot — is projected from `local`. The only downgrade-compat comment protects `local`, the other direction. - NOT multi-client. PersistedState is one file on one machine; phones and CLI are RPC clients into that same process. Orca's real per-client state is `mobileClientTabSelectionsByDeviceId`, 13 lines above in the same struct, and it carries selections only — never a ptyId. What it actually is: the headless/CLI/mobile plane's OWN home, written and read deliberately across several tickets (STA-3463, STA-3465), with tests that assert that partition by name. So the fold was not tidying a spill — it was erasing another plane's live state. Measured symptom: a split SSH tab would disappear from mobile while its shell kept running. The desktop plane's fix never needed it. Supersession multiplied panes because `durablyBoundPtyIdForPane` hedged into the other plane's copy; reading `local` alone is the fix, and it stands without any migration. Deleting rather than redesigning, because the redesign had no target: five review rounds each found a defect in that function, three of them inside the previous round's fix, and every one was a cell of a merge matrix that only existed to serve a premise that was false. The `boundPtyIdsAcrossPartitions` clause is INVERTED, not dropped. It required the two partitions to AGREE after load — which encoded the false premise, and is what made a migration look necessary. It now asserts the narrower, stronger property: what the desktop plane resolves follows `local` alone, whatever the other plane holds. Also proven, and the reason unification was NOT attempted: worktreeId is `<repoId>::<path>` where repoId is a randomUUID minted client-side at repo add (orca-runtime.ts:18722), so two servers cannot collide on one. Host scoping was not protecting against that here — but the two planes' opposite choices are each deliberate and each test-pinned, so choosing a winner is an architecture call, not a cleanup. Net production: -75 lines.
This commit is contained in:
Neil
+4
-79
@@ -2859,8 +2859,6 @@ export class Store {
|
||||
const loaded = this.load()
|
||||
const normalized = normalizePersistedPaneIdentityState(loaded)
|
||||
this.state = normalized.state
|
||||
// After the leaf remap, so every folded binding keys on a UUID.
|
||||
const foldedSshPaneBindings = this.foldSshPaneBindingsIntoLocalPartition()
|
||||
// Why: activeView is a frequent, tiny preference; keeping it beside the
|
||||
// profile avoids serializing the multi-MB recovery store on navigation.
|
||||
this.activeViewPreference = new ActiveViewPreference(this.dataFile, this.state.ui?.activeView)
|
||||
@@ -2879,7 +2877,7 @@ export class Store {
|
||||
this.state.legacyPaneKeyAliasEntries = entries
|
||||
this.scheduleSave()
|
||||
})
|
||||
if (normalized.changed || this.loadNeedsSave || adaptedProjectGroups || foldedSshPaneBindings) {
|
||||
if (normalized.changed || this.loadNeedsSave || adaptedProjectGroups) {
|
||||
// Why: rewrite legacy pane:1 leaves so older renderer writes can't revive them; other migrations also set loadNeedsSave.
|
||||
this.scheduleSave()
|
||||
}
|
||||
@@ -7356,82 +7354,9 @@ export class Store {
|
||||
*
|
||||
* Returns the number of leases retired, for logging.
|
||||
*/
|
||||
/**
|
||||
* One-time fold: SSH pane bindings written to an `ssh:<target>` partition by an older build move
|
||||
* into `local`, the partition the renderer has always published pane membership to. Two homes is
|
||||
* the STA-3077 defect itself — the reattach writer updated one and supersession read the other.
|
||||
* `local` wins on conflict because it is the live publisher; the loser is dropped rather than
|
||||
* demoted to an orphan candidate, since relay pty ids recycle and cannot identify a shell alone.
|
||||
*/
|
||||
private foldSshPaneBindingsIntoLocalPartition(): boolean {
|
||||
const partitions = this.state.workspaceSessionsByHostId ?? {}
|
||||
const local = this.state.workspaceSession
|
||||
if (!local) {
|
||||
// Nothing to fold into, and defaulting one here would mutate state on a path that reports no
|
||||
// change — so the save would never be scheduled for it.
|
||||
return false
|
||||
}
|
||||
let changed = false
|
||||
for (const hostId of Object.keys(partitions)) {
|
||||
if (parseExecutionHostId(hostId)?.kind !== 'ssh') {
|
||||
continue
|
||||
}
|
||||
const host: WorkspaceSessionState | undefined = partitions[hostId]
|
||||
const incarnations = host?.terminalPtyIncarnationsByPaneKey
|
||||
for (const [tabId, layout] of Object.entries(host?.terminalLayoutsByTabId ?? {})) {
|
||||
const bindings = layout.ptyIdsByLeafId
|
||||
if (!bindings || Object.keys(bindings).length === 0) {
|
||||
continue
|
||||
}
|
||||
const localLayout = local.terminalLayoutsByTabId?.[tabId]
|
||||
if (!localLayout) {
|
||||
// No local layout owns this tab, so there is no second home to disagree with and nothing
|
||||
// to fold. Clearing here would delete the only record of the binding.
|
||||
continue
|
||||
}
|
||||
const localBindings = localLayout.ptyIdsByLeafId ?? {}
|
||||
// An incarnation is the other half of its binding's fence, so no binding is cleared without
|
||||
// its fence being resolved in the same pass — one of three ways, by which pty ends up
|
||||
// bound. (An incarnation whose binding was already missing before the fold is left alone:
|
||||
// it is a pre-existing orphan, isolated in this partition, and not ours to reinterpret.)
|
||||
for (const [leafId, ptyId] of Object.entries(bindings)) {
|
||||
const paneKey = `${tabId}:${leafId}`
|
||||
const incarnationId = incarnations?.[paneKey]
|
||||
const localPtyId = localBindings[leafId]
|
||||
if (!localPtyId) {
|
||||
// This binding moves, so local's fence for the leaf becomes this partition's — whatever
|
||||
// it is, INCLUDING absent. A local incarnation with no local binding is a leftover, and
|
||||
// leaving it in place would fence the arriving pty with a value never written beside it.
|
||||
local.terminalPtyIncarnationsByPaneKey ??= {}
|
||||
if (incarnationId) {
|
||||
local.terminalPtyIncarnationsByPaneKey[paneKey] = incarnationId
|
||||
} else {
|
||||
delete local.terminalPtyIncarnationsByPaneKey[paneKey]
|
||||
}
|
||||
} else if (incarnationId && localPtyId === ptyId) {
|
||||
// Same shell from both sides; keep whichever fence local already holds.
|
||||
local.terminalPtyIncarnationsByPaneKey ??= {}
|
||||
local.terminalPtyIncarnationsByPaneKey[paneKey] ??= incarnationId
|
||||
}
|
||||
// The remaining case is local winning with a DIFFERENT pty. This incarnation belongs to
|
||||
// the binding that just lost, so it is dropped rather than moved: pairing it with local's
|
||||
// pty would synthesize a pair no writer produced, and leaving it here would orphan it
|
||||
// from a binding that no longer exists.
|
||||
if (incarnationId) {
|
||||
delete incarnations![paneKey]
|
||||
}
|
||||
}
|
||||
localLayout.ptyIdsByLeafId = { ...bindings, ...localBindings }
|
||||
layout.ptyIdsByLeafId = {}
|
||||
changed = true
|
||||
}
|
||||
}
|
||||
return changed
|
||||
}
|
||||
|
||||
/** The PTY a pane is durably bound to. SSH pane bindings live in the local partition only —
|
||||
* see `foldSshPaneBindingsIntoLocalPartition`. Hedging across two partitions is what let a
|
||||
* stale `ssh:<target>` copy outvote the live binding and no-op supersession (STA-3077). */
|
||||
/** The PTY a pane is durably bound to. The desktop plane's home is `local` — the renderer is its
|
||||
* only publisher of pane membership. Hedging into `ssh:<target>` let the headless plane's copy
|
||||
* outvote the live binding and silently no-op supersession (STA-3077). */
|
||||
private durablyBoundPtyIdForPane(
|
||||
targetId: string,
|
||||
tabId: string,
|
||||
|
||||
@@ -136,19 +136,6 @@ function liveLeaseIdsForPane(store: TestStore): string[] {
|
||||
.sort()
|
||||
}
|
||||
|
||||
/** Every pty id this one pane is durably bound to, across every partition. */
|
||||
function boundPtyIdsAcrossPartitions(store: TestStore): string[] {
|
||||
const partitions = [LOCAL_EXECUTION_HOST_ID, SSH_PARTITION]
|
||||
return Array.from(
|
||||
new Set(
|
||||
partitions
|
||||
.map((hostId) => store.getWorkspaceSession(hostId).terminalLayoutsByTabId?.[TAB])
|
||||
.map((layout) => layout?.ptyIdsByLeafId?.[LEAF])
|
||||
.filter((ptyId): ptyId is string => typeof ptyId === 'string')
|
||||
)
|
||||
).sort()
|
||||
}
|
||||
|
||||
/** The argument text of every `callee(...)` call in a production source file. */
|
||||
function callArgumentsIn(source: string, callee: string): string[] {
|
||||
const calls: string[] = []
|
||||
@@ -238,129 +225,26 @@ describe('STA-3077 step P: one pane, one live claim across partitions', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('STA-3077 step P: the pane binding has one home', () => {
|
||||
// Loading legacy state must fold the SSH partition's pane bindings into the
|
||||
// one partition every reader consults; two homes is the defect itself.
|
||||
it('resolves one pane to one durable pty id after loading legacy state', async () => {
|
||||
describe('STA-3077 step P: the desktop plane resolves from its own home', () => {
|
||||
// INVERTED. This clause used to require the two partitions to AGREE after load, which assumed
|
||||
// `ssh:<target>` was a stale spill of this plane's state. It is not — it is the headless/CLI
|
||||
// plane's own home, which that plane writes and reads deliberately (STA-3463, STA-3465). The
|
||||
// property that actually matters is narrower and stronger: what the desktop plane resolves must
|
||||
// follow `local` alone, whatever the other plane holds. Asserting agreement made a migration look
|
||||
// necessary that in fact erased another plane's live state.
|
||||
it('resolves to the local binding regardless of what the ssh partition holds', async () => {
|
||||
const store = await createStore(diskAfterEarlierSession('pty-1'))
|
||||
|
||||
rendererPublishesPane(store, 'pty-1')
|
||||
expect(relayReattachBindsPane(store, 'pty-2')).toBe(true)
|
||||
|
||||
expect(boundPtyIdsAcrossPartitions(store)).toEqual([appPtyId('pty-2')])
|
||||
})
|
||||
})
|
||||
|
||||
describe('STA-3077 step P: the fold never destroys a binding it cannot move', () => {
|
||||
// A tab that exists only in the ssh partition has no second home to disagree with, so there is
|
||||
// nothing to fold. Clearing it anyway would delete the only record that pane was ever bound.
|
||||
it('leaves an ssh-only tab binding intact', async () => {
|
||||
const store = await createStore({
|
||||
workspaceSession: { ...getDefaultWorkspaceSession(), terminalLayoutsByTabId: {} },
|
||||
workspaceSessionsByHostId: { [SSH_PARTITION]: paneSession('pty-1') }
|
||||
})
|
||||
// The other plane still names the predecessor, and is deliberately left alone.
|
||||
somethingRewritesTheSshPartition(store, 'pty-1')
|
||||
sshSpawnUpsertsLease(store, 'pty-2')
|
||||
|
||||
expect(
|
||||
store.getWorkspaceSession(SSH_PARTITION).terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF]
|
||||
).toBe(appPtyId('pty-1'))
|
||||
})
|
||||
})
|
||||
|
||||
describe('STA-3077 step P: the fold carries the whole fence', () => {
|
||||
// `persistPtyBinding` writes the binding and its incarnation into the SAME partition, and the
|
||||
// incarnation is what its CAS compares. Folding only the binding would move the guard's subject
|
||||
// to `local` while the guard itself stayed in the partition nothing reads — the CAS would then
|
||||
// compare undefined against undefined and pass for any incarnation.
|
||||
it('folds the pane incarnation along with the binding it fences', async () => {
|
||||
const sshSession = paneSession('pty-1') as unknown as {
|
||||
terminalPtyIncarnationsByPaneKey: Record<string, string>
|
||||
}
|
||||
sshSession.terminalPtyIncarnationsByPaneKey = { [`${TAB}:${LEAF}`]: 'inc-pty-1' }
|
||||
const store = await createStore({
|
||||
workspaceSession: paneSession('pty-1'),
|
||||
workspaceSessionsByHostId: { [SSH_PARTITION]: sshSession }
|
||||
})
|
||||
|
||||
expect(store.getWorkspaceSession().terminalPtyIncarnationsByPaneKey?.[`${TAB}:${LEAF}`]).toBe(
|
||||
'inc-pty-1'
|
||||
)
|
||||
expect(
|
||||
store.getWorkspaceSession(SSH_PARTITION).terminalPtyIncarnationsByPaneKey?.[`${TAB}:${LEAF}`]
|
||||
).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
describe('STA-3077 step P: the fold never synthesizes a pair that was never written', () => {
|
||||
// The two partitions can name DIFFERENT ptys for one leaf — that divergence is the defect being
|
||||
// migrated. Local wins the binding, so the losing partition's incarnation must not follow it:
|
||||
// (local pty, superseded incarnation) is a pair no writer ever produced, and persistPtyBinding's
|
||||
// CAS compares both, so it would refuse the pane's next legitimate update.
|
||||
it('leaves the superseded incarnation behind when local wins with a different pty', async () => {
|
||||
const sshSession = paneSession('pty-1') as unknown as {
|
||||
terminalPtyIncarnationsByPaneKey: Record<string, string>
|
||||
}
|
||||
sshSession.terminalPtyIncarnationsByPaneKey = { [`${TAB}:${LEAF}`]: 'inc-pty-1' }
|
||||
const store = await createStore({
|
||||
workspaceSession: paneSession('pty-2'),
|
||||
workspaceSessionsByHostId: { [SSH_PARTITION]: sshSession }
|
||||
})
|
||||
|
||||
const local = store.getWorkspaceSession()
|
||||
expect(local.terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF]).toBe(appPtyId('pty-2'))
|
||||
expect(local.terminalPtyIncarnationsByPaneKey?.[`${TAB}:${LEAF}`]).not.toBe('inc-pty-1')
|
||||
// Not merely "not copied" — the losing binding is cleared, so leaving its incarnation here
|
||||
// would strand a fence with nothing to fence.
|
||||
expect(
|
||||
store.getWorkspaceSession(SSH_PARTITION).terminalPtyIncarnationsByPaneKey?.[`${TAB}:${LEAF}`]
|
||||
).toBeUndefined()
|
||||
})
|
||||
|
||||
// Case 7 of the fold matrix, and the one two independent reviewers both landed on. A moving
|
||||
// binding with NO incarnation of its own must not inherit local's leftover: (arriving pty,
|
||||
// unrelated incarnation) is a pair no writer produced, and persistPtyBinding's CAS compares both,
|
||||
// so it would refuse this pane's next legitimate update.
|
||||
it('does not pair a moving binding with a local incarnation when it has none of its own', async () => {
|
||||
const paneKey = `${TAB}:${LEAF}`
|
||||
const localSession = paneSession('pty-1') as unknown as {
|
||||
terminalLayoutsByTabId: Record<string, { ptyIdsByLeafId: Record<string, string> }>
|
||||
terminalPtyIncarnationsByPaneKey: Record<string, string>
|
||||
}
|
||||
localSession.terminalLayoutsByTabId[TAB]!.ptyIdsByLeafId = {}
|
||||
localSession.terminalPtyIncarnationsByPaneKey = { [paneKey]: 'inc-orphaned' }
|
||||
const store = await createStore({
|
||||
workspaceSession: localSession,
|
||||
// The ssh partition carries the binding but never an incarnation for it.
|
||||
workspaceSessionsByHostId: { [SSH_PARTITION]: paneSession('pty-9') }
|
||||
})
|
||||
|
||||
const local = store.getWorkspaceSession()
|
||||
expect(local.terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF]).toBe(appPtyId('pty-9'))
|
||||
expect(local.terminalPtyIncarnationsByPaneKey?.[paneKey]).toBeUndefined()
|
||||
})
|
||||
|
||||
// A local incarnation with no local binding is a leftover, not a fence. When the ssh binding is
|
||||
// the one that survives, its own incarnation must come with it and outrank that leftover —
|
||||
// otherwise the surviving pty is fenced by a value never written beside it.
|
||||
it('lets a moving binding overwrite a local incarnation that fences nothing', async () => {
|
||||
const paneKey = `${TAB}:${LEAF}`
|
||||
const sshSession = paneSession('pty-1') as unknown as {
|
||||
terminalPtyIncarnationsByPaneKey: Record<string, string>
|
||||
}
|
||||
sshSession.terminalPtyIncarnationsByPaneKey = { [paneKey]: 'inc-pty-1' }
|
||||
const localSession = paneSession('pty-1') as unknown as {
|
||||
terminalLayoutsByTabId: Record<string, { ptyIdsByLeafId: Record<string, string> }>
|
||||
terminalPtyIncarnationsByPaneKey: Record<string, string>
|
||||
}
|
||||
localSession.terminalLayoutsByTabId[TAB]!.ptyIdsByLeafId = {}
|
||||
localSession.terminalPtyIncarnationsByPaneKey = { [paneKey]: 'inc-orphaned' }
|
||||
const store = await createStore({
|
||||
workspaceSession: localSession,
|
||||
workspaceSessionsByHostId: { [SSH_PARTITION]: sshSession }
|
||||
})
|
||||
|
||||
const local = store.getWorkspaceSession()
|
||||
expect(local.terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF]).toBe(appPtyId('pty-1'))
|
||||
expect(local.terminalPtyIncarnationsByPaneKey?.[paneKey]).toBe('inc-pty-1')
|
||||
store.getWorkspaceSession().terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF]
|
||||
).toBe(appPtyId('pty-2'))
|
||||
expect(liveLeaseIdsForPane(store)).toEqual(['pty-2'])
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
Reference in New Issue
Block a user