mirror of
https://github.com/stablyai/orca.git
synced 2026-10-22 00:03:47 +00:00
fix(ssh): arbitrate on the pane, not the tab its lease was written in
Correctness review round 1 on #13326: 5 raised, 2 confirmed, both real. 1. Arbitration looked the durable binding up under the lease's FROZEN tabId. `detachTerminalPaneToTab` moves a live pane and its PTY, and nothing re-keys a lease, so after a break-out the binding lives under the new tab and the lookup found nothing. The bound shell then lost to recency and supersession expired the pane's OWN lease; the follow-on scrub could not clean up either, because it matches lease.tabId against the layout's tabId. Reconnect skipped the expired-but-bound PTY and reattached the stale winner onto the pane. The leaf is the stable half of pane identity, so the lookup now prefers the named tab and falls back to wherever the leaf actually is. 2. Arbitration read the desktop plane only while the scrub reached into `ssh:<target>`, so a headless-plane pane was invisible to the ranking and then lost its live binding to it. Fixed at the reader, not the scrub: local FIRST, headless plane only as a fallback. That is NOT the STA-3077 hedge — that bug was preferring `ssh:<target>`, letting a copy no live writer maintains outvote the real binding. A fallback consulted only when local is silent gives a headless-owned pane a vote without ever outranking a live desktop binding. Proven by mutation: swapping the two back to ssh-first reddens 4 clauses, including the ten-reconnect cardinality one. Also closes two fulfilled-result routes to the duplicate agent resume that the earlier RC2 fix missed — `handleReattachResult` respawned on a result flagged `sessionExpired` and on a result carrying no pty id. A skeptic refuted both as pre-existing rather than PR-caused, which is correct on causation, but they are live routes to the defect this PR claims to fix. Scoped to SSH panes only. The first attempt diverted every pane and broke three daemon tests — correctly: a local provider is authoritative about its own ptys, so "cannot reattach" there is not the ambiguous evidence it is for a relay that may have been replaced. New clause covers both routes; disabling the diversion reddens it. 43,253 pass; the 7 failures are pre-existing on clean main in this environment.
This commit is contained in:
Neil
+19
-2
@@ -7356,14 +7356,31 @@ export class Store {
|
||||
*/
|
||||
/** 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). */
|
||||
* outvote the live binding and silently no-op supersession (STA-3077).
|
||||
*
|
||||
* Keyed on the leaf, falling back across tabs: a lease freezes its tabId at write time, and
|
||||
* `detachTerminalPaneToTab` moves a live pane, so the named tab can be the one the pane left.
|
||||
* Missing the binding there makes it lose to recency and retires the pane's own shell. */
|
||||
private durablyBoundPtyIdForPane(
|
||||
targetId: string,
|
||||
tabId: string,
|
||||
leafId: string
|
||||
): string | undefined {
|
||||
const findLeafBinding = (session: WorkspaceSessionState | undefined): string | undefined => {
|
||||
const layouts = session?.terminalLayoutsByTabId
|
||||
return (
|
||||
layouts?.[tabId]?.ptyIdsByLeafId?.[leafId] ??
|
||||
Object.values(layouts ?? {}).find((layout) => layout?.ptyIdsByLeafId?.[leafId])
|
||||
?.ptyIdsByLeafId?.[leafId]
|
||||
)
|
||||
}
|
||||
// Local FIRST, and only then the headless plane. Local-first is what STA-3077 needed: the old
|
||||
// code preferred `ssh:<target>`, so a copy no live writer maintained outvoted the real binding
|
||||
// and supersession no-opped. The fallback is not that hedge — it only speaks for a pane local
|
||||
// says nothing about, so a headless-owned pane still gets a vote before its lease is retired.
|
||||
const boundPtyId =
|
||||
this.state.workspaceSession?.terminalLayoutsByTabId?.[tabId]?.ptyIdsByLeafId?.[leafId]
|
||||
findLeafBinding(this.state.workspaceSession) ??
|
||||
findLeafBinding(this.state.workspaceSessionsByHostId?.[toSshExecutionHostId(targetId)])
|
||||
return boundPtyId ? this.getRelayPtyIdForSshLeaseComparison(targetId, boundPtyId) : undefined
|
||||
}
|
||||
|
||||
|
||||
@@ -225,6 +225,40 @@ describe('STA-3077 step P: one pane, one live claim across partitions', () => {
|
||||
})
|
||||
})
|
||||
|
||||
describe('STA-3077: arbitration follows the pane, not the tab it was written in', () => {
|
||||
// A lease freezes its tabId when written; `detachTerminalPaneToTab` then moves the live pane and
|
||||
// its PTY into a new tab. Looking the binding up under the frozen tab finds nothing, so the bound
|
||||
// shell loses to recency and supersession retires the pane's OWN shell in favour of a stale one.
|
||||
it('finds the binding after the pane has been moved to another tab', async () => {
|
||||
const MOVED_TAB = 'tab-moved-to'
|
||||
const store = await createStore()
|
||||
// The renderer publishes the pane in its NEW tab; the lease still names the old one.
|
||||
store.setWorkspaceSession(
|
||||
{
|
||||
...getDefaultWorkspaceSession(),
|
||||
tabsByWorktree: { [WORKTREE]: [{ id: MOVED_TAB, worktreeId: WORKTREE }] },
|
||||
terminalLayoutsByTabId: {
|
||||
[MOVED_TAB]: {
|
||||
root: { type: 'leaf' as const, leafId: LEAF },
|
||||
activeLeafId: LEAF,
|
||||
expandedLeafId: null,
|
||||
ptyIdsByLeafId: { [LEAF]: appPtyId('pty-bound') }
|
||||
}
|
||||
}
|
||||
} as never,
|
||||
LOCAL_EXECUTION_HOST_ID
|
||||
)
|
||||
sshSpawnUpsertsLease(store, 'pty-bound')
|
||||
// A newer, unbound lease arrives for the same pane under the lease's frozen tab.
|
||||
sshSpawnUpsertsLease(store, 'pty-newer')
|
||||
|
||||
store.supersedeDuplicatePaneLeases(TARGET)
|
||||
|
||||
// The bound shell must win. Keyed on the frozen tab, it is invisible and recency retires it.
|
||||
expect(liveLeaseIdsForPane(store)).toEqual(['pty-bound'])
|
||||
})
|
||||
})
|
||||
|
||||
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
|
||||
|
||||
@@ -9719,6 +9719,52 @@ describe('connectPanePty', () => {
|
||||
// shells still running under a relay process it replaced. Respawning there starts a second
|
||||
// `--resume` against the same agent session and both processes append to one transcript. This is
|
||||
// the last route to that defect, and it is the reason this pane surfaces as disconnected instead.
|
||||
// The reattach can also FULFIL with a result that says the session expired, or with no usable
|
||||
// pty id at all. Both trace back to the same relay not-found, so neither proves the shell exited —
|
||||
// and both used to clear the binding and cold-restore resume the agent, which is the duplicate
|
||||
// transcript this pane exists to prevent. SSH only: a daemon/local provider is authoritative
|
||||
// about its own ptys and keeps replacing in place.
|
||||
it.each([
|
||||
['a result flagged sessionExpired', { id: 'tab-pty', sessionExpired: true }],
|
||||
['a result carrying no pty id', {}]
|
||||
])('surfaces the pane instead of resuming when an SSH reattach returns %s', async (_l, res) => {
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
const transport = createMockTransport('tab-pty')
|
||||
transport.getPtyId.mockImplementation(() => null)
|
||||
transport.connect.mockImplementation(async ({ sessionId }: { sessionId?: string }) =>
|
||||
sessionId ? res : { id: 'respawned-pty' }
|
||||
)
|
||||
transportFactoryQueue.push(transport)
|
||||
|
||||
// connectionId resolves from the store's repo list, not from deps — an SSH-owned repo is what
|
||||
// makes this pane SSH, and the branch under test is SSH-only by design.
|
||||
mockStoreState = {
|
||||
...mockStoreState,
|
||||
repos: [{ id: 'repo1', connectionId: 'target-a', displayName: 'orca' }]
|
||||
} as StoreState
|
||||
const pane = createPane(1)
|
||||
const manager = createManager(1)
|
||||
const onPtyRecoveryState = vi.fn<(paneId: number, state: RecoveryStateProbe) => void>()
|
||||
const deps = createDeps({
|
||||
restoredLeafId: LEAF_1,
|
||||
restoredPtyIdByLeafId: { [LEAF_1]: 'tab-pty' },
|
||||
onPtyRecoveryStateRef: { current: onPtyRecoveryState }
|
||||
})
|
||||
|
||||
connectPanePty(pane as never, manager as never, deps as never)
|
||||
await flushAsyncTicks(30)
|
||||
|
||||
// Producer pin: the restore ran and returned this shape, or the clauses below are vacuous.
|
||||
expect(transport.connect.mock.calls[0]?.[0]?.sessionId).toBe('tab-pty')
|
||||
expect(
|
||||
onPtyRecoveryState.mock.calls.find(
|
||||
([paneId, state]) => paneId === pane.id && state?.phase === 'disconnected'
|
||||
)?.[1]?.unreachablePane
|
||||
).toBeDefined()
|
||||
expect(transport.connect).toHaveBeenCalledTimes(1)
|
||||
expect(deps.clearTabPtyId).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('does not respawn a pane whose restore was answered with a not-found', async () => {
|
||||
const { connectPanePty } = await import('./pty-connection')
|
||||
const transport = createMockTransport('tab-pty')
|
||||
|
||||
@@ -8126,14 +8126,19 @@ export function connectPanePty(
|
||||
paneId: pane.id,
|
||||
ptyId: staleSessionId ?? null
|
||||
})
|
||||
// Why: a stale restored session can fail reattach after mount; don't leave xterm alive without a backing PTY.
|
||||
if (staleSessionId) {
|
||||
deps.clearExitedPanePtyLayoutBinding(pane.id, staleSessionId)
|
||||
} else {
|
||||
deps.syncPanePtyLayoutBinding(pane.id, null)
|
||||
// For an SSH pane a missing id says the reattach could not answer, not that the shell
|
||||
// exited — a replaced relay reports that for shells still running under its predecessor —
|
||||
// so surface it and let the user choose. A daemon/local pane is not ambiguous that way: its
|
||||
// provider is authoritative about its own ptys, and it keeps replacing in place.
|
||||
if (connectionId && staleSessionId) {
|
||||
publishUnreachablePane(staleSessionId)
|
||||
return false
|
||||
}
|
||||
if (staleSessionId) {
|
||||
deps.clearExitedPanePtyLayoutBinding(pane.id, staleSessionId)
|
||||
deps.clearTabPtyId(deps.tabId, staleSessionId)
|
||||
} else {
|
||||
deps.syncPanePtyLayoutBinding(pane.id, null)
|
||||
}
|
||||
startFreshColdRestoreAgentResume(coldRestoreStartup, {
|
||||
forceBlankRestoredViewport: true
|
||||
@@ -8149,15 +8154,18 @@ export function connectPanePty(
|
||||
: {})
|
||||
})
|
||||
if (connectResult?.sessionExpired) {
|
||||
// For SSH, `sessionExpired` traces back to the relay's not-found, which it also answers for
|
||||
// shells still running under a relay process it replaced — not proof the shell exited.
|
||||
if (connectionId && staleSessionId) {
|
||||
publishUnreachablePane(staleSessionId)
|
||||
return false
|
||||
}
|
||||
if (staleSessionId) {
|
||||
deps.clearExitedPanePtyLayoutBinding(pane.id, staleSessionId)
|
||||
deps.clearTabPtyId(deps.tabId, staleSessionId)
|
||||
} else {
|
||||
deps.syncPanePtyLayoutBinding(pane.id, null)
|
||||
}
|
||||
if (staleSessionId) {
|
||||
deps.clearTabPtyId(deps.tabId, staleSessionId)
|
||||
}
|
||||
// Why: SSH sleep/reconnect can invalidate the relay PTY while the tab stays mounted; replace the dead lease in-place, not a stale overlay.
|
||||
startFreshColdRestoreAgentResume(coldRestoreStartup, {
|
||||
forceBlankRestoredViewport: true
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user