mirror of
https://github.com/stablyai/orca.git
synced 2026-10-21 16:03:26 +00:00
fix(ssh): a supersession decided from one plane only mutates that plane
Correctness round 2 confirmed finding. Arbitration ranks leases using the desktop plane's binding, then handed its losers to a scrub that walked BOTH planes — so the headless/CLI/mobile plane's binding was deleted for a lease it never got to vote on. Its owner then has no durable record to reattach that shell by, and can fall back to recency or orphan a shell that is still running. `clearSshRemotePtyBindingsForLeases` now takes `arbitratedFrom`. A decision reached by reading one plane may only mutate that plane. An explicit expiry or termination is plane-agnostic — the pty is gone for everyone — so those callers pass nothing and still scrub both, which is what the existing `markSshRemotePtyLease` oracle pins. I initially assessed this as not-a-defect, reasoning that local winning IS the STA-3077 fix. That was about RANKING and did not justify DELETING the other plane's record; the round-2 verdict was right and I was wrong. It did also catch a comment of mine that had gone stale — a clause claimed the other plane was "deliberately left alone" while the code cleared it. The comment now says what the code does and why. Mutation: letting arbitration scrub both planes again reddens the new clause. 43,256 pass; the 7 failures are pre-existing on clean main in this environment.
This commit is contained in:
+15
-7
@@ -7344,7 +7344,7 @@ export class Store {
|
||||
}
|
||||
// Why: matching on lease ptyId first means this scrubs only the predecessor's
|
||||
// stale binding — the winner's own binding cannot match and is left intact.
|
||||
this.clearSshRemotePtyBindingsForLeases(winner.targetId, superseded)
|
||||
this.clearSshRemotePtyBindingsForLeases(winner.targetId, superseded, 'local')
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -7453,7 +7453,7 @@ export class Store {
|
||||
return 0
|
||||
}
|
||||
const sessionsBefore = this.cloneSshLeaseBindingSessions(targetId)
|
||||
this.clearSshRemotePtyBindingsForLeases(targetId, superseded)
|
||||
this.clearSshRemotePtyBindingsForLeases(targetId, superseded, 'local')
|
||||
try {
|
||||
// Why flushOrThrow: flush() swallows write errors, which would leave these
|
||||
// leases retired in memory but attached on disk for the rest of the session.
|
||||
@@ -7632,19 +7632,27 @@ export class Store {
|
||||
this.clearSshRemotePtyBindingsForLeases(targetId, leases ?? [])
|
||||
}
|
||||
|
||||
/** `arbitratedFrom` names the plane a supersession DECISION was made from. A decision reached by
|
||||
* reading one plane may only mutate that plane: the other plane's binding never got a vote, and
|
||||
* deleting it strands a shell its owner can still be using. An explicit expiry or termination is
|
||||
* plane-agnostic — the pty is gone for everyone — so those callers pass nothing. */
|
||||
private clearSshRemotePtyBindingsForLeases(
|
||||
targetId: string,
|
||||
leases: SshRemotePtyLease[]
|
||||
leases: SshRemotePtyLease[],
|
||||
arbitratedFrom?: 'local'
|
||||
): boolean {
|
||||
if (!leases?.length) {
|
||||
return false
|
||||
}
|
||||
let changed = false
|
||||
const sessions = new Set(
|
||||
[
|
||||
this.state.workspaceSession,
|
||||
this.state.workspaceSessionsByHostId?.[toSshExecutionHostId(targetId)]
|
||||
].filter((session): session is WorkspaceSessionState => Boolean(session))
|
||||
(arbitratedFrom === 'local'
|
||||
? [this.state.workspaceSession]
|
||||
: [
|
||||
this.state.workspaceSession,
|
||||
this.state.workspaceSessionsByHostId?.[toSshExecutionHostId(targetId)]
|
||||
]
|
||||
).filter((session): session is WorkspaceSessionState => Boolean(session))
|
||||
)
|
||||
for (const session of sessions) {
|
||||
for (const [worktreeId, tabs] of Object.entries(session.tabsByWorktree ?? {})) {
|
||||
|
||||
@@ -225,6 +225,29 @@ describe('STA-3077 step P: one pane, one live claim across partitions', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('STA-3077: a decision made from one plane only mutates that plane', () => {
|
||||
// Arbitration ranks using the desktop plane's binding. The headless plane's binding never gets a
|
||||
// vote, so deleting it here would strand a shell its owner may still be using — and that owner
|
||||
// would then have no durable record to reattach by. An explicit expiry is different: the pty is
|
||||
// gone for everyone, and that path still scrubs both.
|
||||
it('leaves the headless plane binding intact when supersession retires its lease', async () => {
|
||||
const sshSession = paneSession('pty-old') as unknown as Record<string, unknown>
|
||||
const store = await createStore({
|
||||
workspaceSession: paneSession('pty-new'),
|
||||
workspaceSessionsByHostId: { [SSH_PARTITION]: sshSession }
|
||||
})
|
||||
sshSpawnUpsertsLease(store, 'pty-old')
|
||||
|
||||
// Local names pty-new, so arbitration expires pty-old without the other plane being consulted.
|
||||
sshSpawnUpsertsLease(store, 'pty-new')
|
||||
|
||||
expect(liveLeaseIdsForPane(store)).toEqual(['pty-new'])
|
||||
expect(
|
||||
store.getWorkspaceSession(SSH_PARTITION).terminalLayoutsByTabId?.[TAB]?.ptyIdsByLeafId?.[LEAF]
|
||||
).toBe(appPtyId('pty-old'))
|
||||
})
|
||||
})
|
||||
|
||||
describe('STA-3077: a pane moved between tabs still supersedes its own predecessor', () => {
|
||||
// The reported cardinality growth, in its surviving form. A lease freezes its tabId at write
|
||||
// time. Break the pane out into a new tab and its next lease carries the NEW tab, so matching
|
||||
@@ -314,7 +337,10 @@ describe('STA-3077 step P: the desktop plane resolves from its own home', () =>
|
||||
|
||||
rendererPublishesPane(store, 'pty-1')
|
||||
expect(relayReattachBindsPane(store, 'pty-2')).toBe(true)
|
||||
// The other plane still names the predecessor, and is deliberately left alone.
|
||||
// The other plane names the predecessor. It does not get a vote here — local speaking about
|
||||
// this leaf IS the desktop plane's live view, and letting the other copy outrank it is the
|
||||
// STA-3077 defect. Its binding to the loser is then scrubbed with every other reference to
|
||||
// that retired lease, so the two planes do not drift further apart.
|
||||
somethingRewritesTheSshPartition(store, 'pty-1')
|
||||
sshSpawnUpsertsLease(store, 'pty-2')
|
||||
|
||||
|
||||
Reference in New Issue
Block a user