mirror of
https://github.com/stablyai/orca.git
synced 2026-09-23 16:02:24 +00:00
fix(ssh): stop reconnect from grafting panes and stacking remote leases
Reconnecting an SSH-backed workspace added terminal panes the user never opened, and the remote host accumulated shells nobody was using — one report went from 2 to 19 to 20 relay PTYs across three reconnects (STA-3077). Two root causes, both in the store. Reattach could create UI. `persistPtyBinding` has four creating branches — mint a tab, mint a root leaf, split the root and graft a leaf, mint a layout. All four are load-bearing for `pty:spawn`, which can beat the renderer's debounced layout writer, but none of them is appropriate on reattach, where the pane either already exists or is gone for good. Add `mayCreate`, defaulting true so the spawn path is untouched; every creating branch already sets `terminalMembershipChanged`, so refusing is a check rather than a new code path. Lease identity had no pane key. `upsertSshRemotePtyLease` matched on `(targetId, ptyId)` alone, so a pane that re-leased under a new relay id left its predecessor live with nothing to retire it, and the next reattach fanned out over both. One pane now keeps at most one live lease. Superseded leases are marked `expired` rather than terminated: losing a lease is not proof the shell died, so the remote process is deliberately left running. Tests assert observable behavior rather than mechanism, so they stay valid under any implementation that fixes this. Co-authored-by: Orca <help@stably.ai>
This commit is contained in:
Neil
Orca
@@ -6711,9 +6711,13 @@ export class Store {
|
||||
incarnationId?: string
|
||||
startupCwd?: string
|
||||
expectedBinding?: { ptyId: string; incarnationId?: string }
|
||||
/** Reattach passes false: an absent durable pane never authorizes creating UI.
|
||||
* Defaults true so the spawn path keeps its force-quit-race branches. */
|
||||
mayCreate?: boolean
|
||||
},
|
||||
hostId?: string | null
|
||||
): boolean {
|
||||
const mayCreate = args.mayCreate ?? true
|
||||
const resolvedHostId = this.resolveHostId(hostId)
|
||||
const session = this.getWorkspaceSession(resolvedHostId)
|
||||
const paneKey = `${args.tabId}:${args.leafId}`
|
||||
@@ -6800,6 +6804,10 @@ export class Store {
|
||||
[args.worktreeId]: session.activeTabIdByWorktree?.[args.worktreeId] ?? args.tabId
|
||||
}
|
||||
}
|
||||
if (!mayCreate && terminalMembershipChanged) {
|
||||
restoreSession()
|
||||
return false
|
||||
}
|
||||
if (!isTerminalLeafId(args.leafId)) {
|
||||
// Why: keep legacy renderer-local pane ids out of durable leaf-keyed layout state after the UUID migration.
|
||||
advanceTopologyFence()
|
||||
@@ -6850,6 +6858,10 @@ export class Store {
|
||||
}
|
||||
}
|
||||
}
|
||||
if (!mayCreate && terminalMembershipChanged) {
|
||||
restoreSession()
|
||||
return false
|
||||
}
|
||||
advanceTopologyFence()
|
||||
try {
|
||||
this.flushOrThrow()
|
||||
@@ -7176,9 +7188,48 @@ export class Store {
|
||||
} else {
|
||||
this.state.sshRemotePtyLeases.push(next)
|
||||
}
|
||||
this.supersedeSiblingLeasesForPane(next, now)
|
||||
this.flush()
|
||||
}
|
||||
|
||||
/**
|
||||
* One pane owns at most one live remote PTY. Without this, lease identity is
|
||||
* `(targetId, ptyId)` alone, so a pane that re-leases under a new relay id
|
||||
* leaves its predecessor live forever — reattach then fans out over both and
|
||||
* grafts a pane the user never opened (STA-3077: 2 -> 19 -> 20 across three
|
||||
* reconnects).
|
||||
*
|
||||
* Superseded leases are marked `expired`, not terminated: the remote shell is
|
||||
* deliberately left running, because losing a lease is not proof the shell died.
|
||||
*/
|
||||
private supersedeSiblingLeasesForPane(winner: SshRemotePtyLease, now: number): void {
|
||||
if (!winner.worktreeId || !winner.tabId || !winner.leafId) {
|
||||
return
|
||||
}
|
||||
if (winner.state === 'terminated' || winner.state === 'expired') {
|
||||
return
|
||||
}
|
||||
const superseded: SshRemotePtyLease[] = []
|
||||
for (const lease of this.state.sshRemotePtyLeases ?? []) {
|
||||
if (
|
||||
lease.ptyId !== winner.ptyId &&
|
||||
lease.targetId === winner.targetId &&
|
||||
lease.worktreeId === winner.worktreeId &&
|
||||
lease.tabId === winner.tabId &&
|
||||
lease.leafId === winner.leafId &&
|
||||
lease.state !== 'terminated' &&
|
||||
lease.state !== 'expired'
|
||||
) {
|
||||
lease.state = 'expired'
|
||||
lease.updatedAt = now
|
||||
superseded.push(lease)
|
||||
}
|
||||
}
|
||||
// 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)
|
||||
}
|
||||
|
||||
markSshRemotePtyLeases(targetId: string, state: SshRemotePtyLease['state']): void {
|
||||
if (this.updateSshRemotePtyLeaseStates(targetId, state)) {
|
||||
this.flush()
|
||||
|
||||
@@ -0,0 +1,185 @@
|
||||
/**
|
||||
* STA-3077 behavioral oracles: reconnecting an SSH workspace must not add panes
|
||||
* the user never opened, and must not accumulate remote shells.
|
||||
*
|
||||
* These assert observable behavior, not a mechanism, so they stay valid under
|
||||
* any implementation that fixes the defect. Each case names the root cause it
|
||||
* pins from the #12264 diagnosis.
|
||||
*
|
||||
* Reported symptom: relay PTY count went 2 -> 19 -> 20 across three reconnects.
|
||||
*/
|
||||
import { beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs'
|
||||
import { join } from 'node:path'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { getDefaultPersistedState } from '../shared/constants'
|
||||
|
||||
const testState = { dir: '' }
|
||||
|
||||
vi.mock('electron', () => ({
|
||||
app: { getPath: () => testState.dir },
|
||||
safeStorage: { isEncryptionAvailable: () => false }
|
||||
}))
|
||||
vi.mock('./telemetry/client', () => ({ track: vi.fn() }))
|
||||
vi.mock('./telemetry/cohort-classifier', () => ({ getCohortAtEmit: vi.fn() }))
|
||||
|
||||
const TARGET = 'ssh-target-1'
|
||||
const WORKTREE = 'repo-1:wt-1'
|
||||
const TAB = 'tab-1'
|
||||
/** Must be a real layout leaf UUID — the store drops any other spelling. */
|
||||
const LEAF = '3f1c9a2e-7b4d-4e1a-9c8f-2d5e6a7b8c90'
|
||||
|
||||
async function createStore(state: Record<string, unknown> = {}) {
|
||||
mkdirSync(testState.dir, { recursive: true })
|
||||
writeFileSync(
|
||||
join(testState.dir, 'orca-data.json'),
|
||||
JSON.stringify({ ...getDefaultPersistedState(testState.dir), ...state }),
|
||||
'utf-8'
|
||||
)
|
||||
vi.resetModules()
|
||||
const { Store, initDataPath } = await import('./persistence')
|
||||
initDataPath()
|
||||
return new Store()
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
testState.dir = mkdtempSync(join(tmpdir(), 'orca-sta3077-'))
|
||||
})
|
||||
|
||||
/** One pane's lease, as the reattach path records it. */
|
||||
function leaseFor(ptyId: string, updatedAt: number) {
|
||||
return {
|
||||
targetId: TARGET,
|
||||
ptyId,
|
||||
worktreeId: WORKTREE,
|
||||
tabId: TAB,
|
||||
leafId: LEAF,
|
||||
state: 'attached' as const,
|
||||
updatedAt
|
||||
}
|
||||
}
|
||||
|
||||
function liveLeasesForPane(store: {
|
||||
getSshRemotePtyLeases: (targetId?: string) => readonly {
|
||||
ptyId: string
|
||||
tabId?: string
|
||||
leafId?: string
|
||||
state: string
|
||||
}[]
|
||||
}) {
|
||||
return store
|
||||
.getSshRemotePtyLeases(TARGET)
|
||||
.filter(
|
||||
(lease) =>
|
||||
lease.tabId === TAB &&
|
||||
lease.leafId === LEAF &&
|
||||
lease.state !== 'terminated' &&
|
||||
lease.state !== 'expired'
|
||||
)
|
||||
}
|
||||
|
||||
describe('STA-3077: one pane owns at most one live remote PTY lease', () => {
|
||||
// RC1: lease uniqueness keys on (targetId, ptyId) only, so a pane whose PTY id
|
||||
// changes leaves its predecessor behind with nothing to retire it.
|
||||
it('does not accumulate a second live lease when one pane re-leases a new PTY id', async () => {
|
||||
const store = await createStore()
|
||||
|
||||
store.upsertSshRemotePtyLease(leaseFor('relay-pty-a', 1))
|
||||
store.upsertSshRemotePtyLease(leaseFor('relay-pty-b', 2))
|
||||
|
||||
expect(liveLeasesForPane(store)).toHaveLength(1)
|
||||
})
|
||||
|
||||
// RC1: the reported 2 -> 19 -> 20 growth. Lease count must not scale with
|
||||
// reconnect count for a fixed set of panes.
|
||||
it('keeps live lease count flat across repeated reconnects of one pane', async () => {
|
||||
const store = await createStore()
|
||||
|
||||
for (let reconnect = 0; reconnect < 10; reconnect += 1) {
|
||||
store.upsertSshRemotePtyLease(leaseFor(`relay-pty-${reconnect}`, reconnect + 1))
|
||||
}
|
||||
|
||||
expect(liveLeasesForPane(store)).toHaveLength(1)
|
||||
})
|
||||
})
|
||||
|
||||
describe('STA-3077: superseding a lease scrubs only the predecessor binding', () => {
|
||||
it('drops the stale pane binding without disturbing the successor', async () => {
|
||||
const store = await createStore()
|
||||
|
||||
// Spawn-shaped setup: the creating branches are intentionally allowed here.
|
||||
store.persistPtyBinding({
|
||||
worktreeId: WORKTREE,
|
||||
tabId: TAB,
|
||||
leafId: LEAF,
|
||||
ptyId: 'relay-pty-a',
|
||||
incarnationId: 'inc-a'
|
||||
})
|
||||
store.upsertSshRemotePtyLease(leaseFor('relay-pty-a', 1))
|
||||
store.upsertSshRemotePtyLease(leaseFor('relay-pty-b', 2))
|
||||
|
||||
const layout = store.getWorkspaceSession().terminalLayoutsByTabId?.[TAB]
|
||||
expect(layout?.ptyIdsByLeafId?.[LEAF]).not.toBe('relay-pty-a')
|
||||
expect(liveLeasesForPane(store).map((lease) => lease.ptyId)).toEqual(['relay-pty-b'])
|
||||
})
|
||||
})
|
||||
|
||||
describe('STA-3077: reattach binds panes, it never creates them', () => {
|
||||
// RC3: persistPtyBinding has four creating branches (mint tab, mint root leaf,
|
||||
// split root and graft leaf, mint layout). They are load-bearing for spawn and
|
||||
// wrong for reattach, where the pane either exists or is gone for good.
|
||||
it('does not mint a tab for a pane that no longer exists', async () => {
|
||||
const store = await createStore()
|
||||
|
||||
store.persistPtyBinding({
|
||||
worktreeId: WORKTREE,
|
||||
tabId: TAB,
|
||||
leafId: LEAF,
|
||||
ptyId: 'relay-pty-a',
|
||||
incarnationId: 'inc-a',
|
||||
// A reattach must not grow topology. Expressed as the caller's intent so
|
||||
// this oracle survives whatever option name the fix introduces.
|
||||
mayCreate: false
|
||||
} as Parameters<typeof store.persistPtyBinding>[0])
|
||||
|
||||
const session = store.getWorkspaceSession()
|
||||
expect(session.tabsByWorktree?.[WORKTREE] ?? []).toHaveLength(0)
|
||||
expect(session.terminalLayoutsByTabId?.[TAB]).toBeUndefined()
|
||||
})
|
||||
|
||||
// Unknown is not dead: failing to resolve a pane must not be reported as
|
||||
// success, and must not terminate anything.
|
||||
it('reports an unresolved reattach rather than silently succeeding', async () => {
|
||||
const store = await createStore()
|
||||
|
||||
const bound = store.persistPtyBinding({
|
||||
worktreeId: WORKTREE,
|
||||
tabId: TAB,
|
||||
leafId: LEAF,
|
||||
ptyId: 'relay-pty-a',
|
||||
incarnationId: 'inc-a',
|
||||
mayCreate: false
|
||||
} as Parameters<typeof store.persistPtyBinding>[0])
|
||||
|
||||
expect(bound).toBe(false)
|
||||
})
|
||||
})
|
||||
|
||||
describe('STA-3077: exact-binding compare-and-swap', () => {
|
||||
// Already correct at HEAD. Pinned so a fix cannot regress it: a stale renderer
|
||||
// replay must not overwrite a binding the host has since re-admitted.
|
||||
it('refuses a write whose expected binding no longer matches', async () => {
|
||||
const store = await createStore()
|
||||
|
||||
const stale = store.persistPtyBinding({
|
||||
worktreeId: WORKTREE,
|
||||
tabId: TAB,
|
||||
leafId: LEAF,
|
||||
ptyId: 'relay-pty-b',
|
||||
incarnationId: 'inc-b',
|
||||
expectedBinding: { ptyId: 'relay-pty-a', incarnationId: 'inc-a' }
|
||||
})
|
||||
|
||||
expect(stale).toBe(false)
|
||||
})
|
||||
})
|
||||
Reference in New Issue
Block a user