fix(ssh): let an expired lease permit a reattach instead of unbinding the pane

`expired` never means the remote shell exited. Every writer records that the
CLIENT lost its route — a superseded sibling, a recycled relay id, a
persistPtyBinding refusal made *after* pty.attach proved the shell alive, a
failed reattach indistinguishable from a relay restart, a relay reset whose
kill may not have landed. docs/reference/ssh-execution-boundary.md grades all
of those `unverifiable`.

Three readers treated it as death, and together they made the pane unable to
reach a process that is still running:

- `isRestorablePtyBinding` / `hasRestorableSshRemotePtyLease` refused to replay
  a durable binding a renderer snapshot had omitted.
- `markSshRemotePtyLease(s)` wiped the persisted pane->pty binding, which is
  what makes `resolvePersistedStablePaneOwner` return null, `adoptStablePane`
  give up, and `createTerminal` cold-spawn a replacement. The user's terminal
  comes back empty and the running job is orphaned and invisible.

Only `terminated` now withdraws a binding: it is the operator-close state
(`ssh:terminateSessions`) and the one written after a host-acknowledged stop.

This authorizes a reattach ATTEMPT, never a respawn, so #17957's gates are
untouched and in fact fire less often — where the pane previously went straight
to a fresh spawn it now attaches first. A genuinely dead shell still converges:
`attachStablePaneOwner` retires the binding on `isPtyAlreadyGoneError` (the
relay's own absence answer, not a message match) and falls through to a fresh
spawn, so no pane retries forever.

Supersession keeps its own binding scrub in `supersedeSiblingLeasesForPane`,
where a NEWER lease for the same pane is the evidence — the 2 -> 19 -> 20
reattach fan-out stays fixed.
This commit is contained in:
Neil
2026-09-01 07:19:05 -07:00
parent ad91a09ac6
commit a321528d6d
7 changed files with 452 additions and 47 deletions
@@ -444,7 +444,32 @@ describe('Store host-partitioned workspace sessions', () => {
).toBeNull()
})
it('clears expired SSH PTY bindings from the SSH partition and legacy local copy', async () => {
it('clears terminated SSH PTY bindings from the SSH partition and legacy local copy', async () => {
const store = await createStore()
const ptyId = 'ssh:ssh-1@@remote-pty'
store.setWorkspaceSession(makeBoundHostSession(ptyId), 'local')
store.setWorkspaceSession(makeBoundHostSession(ptyId), 'ssh:ssh-1')
store.upsertSshRemotePtyLease({
targetId: 'ssh-1',
ptyId: 'remote-pty',
worktreeId: 'repo-1::/worktree',
tabId: 'tab-1',
leafId: TEST_LEAF_1,
state: 'attached'
})
store.markSshRemotePtyLease('ssh-1', ptyId, 'terminated')
for (const hostId of ['local', 'ssh:ssh-1']) {
const session = store.getWorkspaceSession(hostId)
expect(session.tabsByWorktree['repo-1::/worktree'][0]?.ptyId).toBeNull()
expect(session.terminalLayoutsByTabId['tab-1']?.ptyIdsByLeafId).toEqual({})
}
})
// `expired` only records that the client lost its route, so both partitions keep the binding the
// pane needs to reattach to a remote shell nothing has attested is dead.
it('keeps expired SSH PTY bindings in the SSH partition and legacy local copy', async () => {
const store = await createStore()
const ptyId = 'ssh:ssh-1@@remote-pty'
store.setWorkspaceSession(makeBoundHostSession(ptyId), 'local')
@@ -462,8 +487,10 @@ describe('Store host-partitioned workspace sessions', () => {
for (const hostId of ['local', 'ssh:ssh-1']) {
const session = store.getWorkspaceSession(hostId)
expect(session.tabsByWorktree['repo-1::/worktree'][0]?.ptyId).toBeNull()
expect(session.terminalLayoutsByTabId['tab-1']?.ptyIdsByLeafId).toEqual({})
expect(session.tabsByWorktree['repo-1::/worktree'][0]?.ptyId).toBe(ptyId)
expect(session.terminalLayoutsByTabId['tab-1']?.ptyIdsByLeafId).toEqual({
[TEST_LEAF_1]: ptyId
})
}
})
@@ -211,7 +211,9 @@ describe('Store', () => {
expect(leafId).not.toBe(TEST_LEAF_2)
})
it('does not restore cleared SSH bindings after a lease expired', async () => {
// An `expired` lease means reattach gave up, not that the remote shell died. Dropping the
// binding here left the pane unable to re-adopt a process that is still running.
it('restores a cleared SSH binding after a lease expired so the pane can reattach', async () => {
const store = await createStore()
store.upsertSshRemotePtyLease({
targetId: 'ssh-1',
@@ -277,6 +279,79 @@ describe('Store', () => {
}
})
const session = store.getWorkspaceSession()
expect(session.tabsByWorktree.wt1[0].ptyId).toBe('remote-pty')
expect(session.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({
[TEST_LEAF_1]: 'remote-pty'
})
})
it('does not restore cleared SSH bindings after a lease was terminated', async () => {
const store = await createStore()
store.upsertSshRemotePtyLease({
targetId: 'ssh-1',
ptyId: 'remote-pty',
worktreeId: 'wt1',
tabId: 'tab1',
leafId: TEST_LEAF_1,
state: 'terminated'
})
store.setWorkspaceSession({
activeRepoId: 'r1',
activeWorktreeId: 'wt1',
activeTabId: 'tab1',
tabsByWorktree: {
wt1: [
{
id: 'tab1',
worktreeId: 'wt1',
title: 'Terminal',
customTitle: null,
color: null,
sortOrder: 0,
createdAt: 1,
ptyId: 'remote-pty'
}
]
},
terminalLayoutsByTabId: {
tab1: {
root: { type: 'leaf', leafId: TEST_LEAF_1 },
activeLeafId: TEST_LEAF_1,
expandedLeafId: null,
ptyIdsByLeafId: { [TEST_LEAF_1]: 'remote-pty' }
}
}
})
store.setWorkspaceSession({
activeRepoId: 'r1',
activeWorktreeId: 'wt1',
activeTabId: 'tab1',
tabsByWorktree: {
wt1: [
{
id: 'tab1',
worktreeId: 'wt1',
title: 'Terminal',
customTitle: null,
color: null,
sortOrder: 0,
createdAt: 1,
ptyId: null
}
]
},
terminalLayoutsByTabId: {
tab1: {
root: { type: 'leaf', leafId: TEST_LEAF_1 },
activeLeafId: TEST_LEAF_1,
expandedLeafId: null,
ptyIdsByLeafId: {}
}
}
})
const session = store.getWorkspaceSession()
expect(session.tabsByWorktree.wt1[0].ptyId).toBeNull()
expect(session.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({})
@@ -213,7 +213,9 @@ describe('Store', () => {
})
})
it('does not resurrect a host binding after its SSH lease expires', async () => {
// `expired` is the client admitting it lost its route; the remote shell may still be running,
// so the pane keeps the binding it needs to reattach.
it('restores a host binding after its SSH lease expires', async () => {
const store = await createStore()
const hostId = 'ssh:ssh-1'
const session: WorkspaceSessionState = {
@@ -265,6 +267,64 @@ describe('Store', () => {
hostId
)
const persisted = store.getWorkspaceSession(hostId)
expect(persisted.tabsByWorktree.wt1[0]!.ptyId).toBe('ssh:ssh-1@@expired')
expect(persisted.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({
[TEST_LEAF_1]: 'ssh:ssh-1@@expired'
})
})
it('does not resurrect a host binding after its SSH lease was terminated', async () => {
const store = await createStore()
const hostId = 'ssh:ssh-1'
const session: WorkspaceSessionState = {
activeRepoId: 'r1',
activeWorktreeId: 'wt1',
activeTabId: 'tab1',
tabsByWorktree: {
wt1: [
{
id: 'tab1',
worktreeId: 'wt1',
title: 'Terminal',
customTitle: null,
color: null,
sortOrder: 0,
createdAt: 1,
ptyId: 'ssh:ssh-1@@expired'
}
]
},
terminalLayoutsByTabId: {
tab1: {
root: { type: 'leaf', leafId: TEST_LEAF_1 },
activeLeafId: TEST_LEAF_1,
expandedLeafId: null,
ptyIdsByLeafId: { [TEST_LEAF_1]: 'ssh:ssh-1@@expired' }
}
}
}
store.setWorkspaceSession(session, hostId)
store.upsertSshRemotePtyLease({
targetId: 'ssh-1',
ptyId: 'expired',
worktreeId: 'wt1',
tabId: 'tab1',
leafId: TEST_LEAF_1,
state: 'terminated'
})
store.setWorkspaceSession(
{
...session,
tabsByWorktree: {
wt1: [{ ...session.tabsByWorktree.wt1[0]!, ptyId: null }]
},
terminalLayoutsByTabId: {
tab1: { ...session.terminalLayoutsByTabId.tab1!, ptyIdsByLeafId: {} }
}
},
hostId
)
const persisted = store.getWorkspaceSession(hostId)
expect(persisted.tabsByWorktree.wt1[0]!.ptyId).toBeNull()
expect(persisted.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({})
})
@@ -45,6 +45,47 @@ vi.mock('./telemetry/cohort-classifier', () => ({
getCohortAtEmit: getCohortAtEmitMock
}))
/** One SSH pane bound to `ssh:ssh-1@@remote-pty` in both the tab row and the leaf map. */
async function storeWithBoundSshPane(): Promise<Awaited<ReturnType<typeof createStore>>> {
const store = await createStore()
store.upsertSshRemotePtyLease({
targetId: 'ssh-1',
ptyId: 'remote-pty',
worktreeId: 'wt1',
tabId: 'tab1',
leafId: TEST_LEAF_1,
state: 'attached'
})
store.setWorkspaceSession({
activeRepoId: 'r1',
activeWorktreeId: 'wt1',
activeTabId: 'tab1',
tabsByWorktree: {
wt1: [
{
id: 'tab1',
worktreeId: 'wt1',
title: 'Terminal',
customTitle: null,
color: null,
sortOrder: 0,
createdAt: 1,
ptyId: 'ssh:ssh-1@@remote-pty'
}
]
},
terminalLayoutsByTabId: {
tab1: {
root: { type: 'leaf', leafId: TEST_LEAF_1 },
activeLeafId: TEST_LEAF_1,
expandedLeafId: null,
ptyIdsByLeafId: { [TEST_LEAF_1]: 'ssh:ssh-1@@remote-pty' }
}
}
})
return store
}
describe('Store', () => {
beforeEach(() => {
testState.dir = mkdtempSync(join(tmpdir(), 'orca-test-'))
@@ -151,6 +192,87 @@ describe('Store', () => {
})
})
// A partial renderer map is only repaired when a lease says the omitted sibling still belongs to
// this host. `expired` says the client lost its route, not that the sibling died — repair it.
// `terminated` is the operator-close state and must stay refused.
it.each([
['expired', { [TEST_LEAF_1]: 'remote-pty-1', [TEST_LEAF_2]: 'remote-pty-2' }],
['terminated', { [TEST_LEAF_1]: 'remote-pty-1' }]
] as const)(
'repairs a partial renderer snapshot for a %s sibling lease only when it is not terminated',
async (siblingState, expected) => {
const store = await createStore()
store.upsertSshRemotePtyLease({
targetId: 'ssh-1',
ptyId: 'remote-pty-1',
worktreeId: 'wt1',
tabId: 'tab1',
leafId: TEST_LEAF_1,
state: 'detached'
})
store.upsertSshRemotePtyLease({
targetId: 'ssh-1',
ptyId: 'remote-pty-2',
worktreeId: 'wt1',
tabId: 'tab1',
leafId: TEST_LEAF_2,
state: siblingState
})
const layout = {
root: {
type: 'split' as const,
direction: 'horizontal' as const,
first: { type: 'leaf' as const, leafId: TEST_LEAF_1 },
second: { type: 'leaf' as const, leafId: TEST_LEAF_2 },
ratio: 0.5
},
activeLeafId: TEST_LEAF_1,
expandedLeafId: null
}
const tabs = {
wt1: [
{
id: 'tab1',
worktreeId: 'wt1',
title: 'Terminal',
customTitle: null,
color: null,
sortOrder: 0,
createdAt: 1,
ptyId: null
}
]
}
store.setWorkspaceSession({
activeRepoId: 'r1',
activeWorktreeId: 'wt1',
activeTabId: 'tab1',
tabsByWorktree: tabs,
terminalLayoutsByTabId: {
tab1: {
...layout,
ptyIdsByLeafId: { [TEST_LEAF_1]: 'remote-pty-1', [TEST_LEAF_2]: 'remote-pty-2' }
}
}
})
// The renderer republishes only the leaf it still knows about.
store.setWorkspaceSession({
activeRepoId: 'r1',
activeWorktreeId: 'wt1',
activeTabId: 'tab1',
tabsByWorktree: tabs,
terminalLayoutsByTabId: {
tab1: { ...layout, ptyIdsByLeafId: { [TEST_LEAF_1]: 'remote-pty-1' } }
}
})
expect(store.getWorkspaceSession().terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual(
expected
)
}
)
it('does not restore layout bindings for leaves removed from the incoming layout', async () => {
const store = await createStore()
store.upsertSshRemotePtyLease({
@@ -506,43 +628,12 @@ describe('Store', () => {
])
})
it('clears workspace bindings when marking an SSH remote PTY lease expired', async () => {
const store = await createStore()
store.upsertSshRemotePtyLease({
targetId: 'ssh-1',
ptyId: 'remote-pty',
worktreeId: 'wt1',
tabId: 'tab1',
leafId: TEST_LEAF_1,
state: 'attached'
})
store.setWorkspaceSession({
activeRepoId: 'r1',
activeWorktreeId: 'wt1',
activeTabId: 'tab1',
tabsByWorktree: {
wt1: [
{
id: 'tab1',
worktreeId: 'wt1',
title: 'Terminal',
customTitle: null,
color: null,
sortOrder: 0,
createdAt: 1,
ptyId: 'ssh:ssh-1@@remote-pty'
}
]
},
terminalLayoutsByTabId: {
tab1: {
root: { type: 'leaf', leafId: TEST_LEAF_1 },
activeLeafId: TEST_LEAF_1,
expandedLeafId: null,
ptyIdsByLeafId: { [TEST_LEAF_1]: 'ssh:ssh-1@@remote-pty' }
}
}
})
// `expired` never means the shell exited — every writer records that the CLIENT lost its route
// (superseded sibling, recycled relay id, persistPtyBinding refusal, failed reattach, relay
// reset). Wiping the binding here made adoptStablePane return null and forced createTerminal
// into a fresh spawn, stranding a remote process that is still running.
it('keeps workspace bindings when marking an SSH remote PTY lease expired', async () => {
const store = await storeWithBoundSshPane()
store.markSshRemotePtyLease('ssh-1', 'ssh:ssh-1@@remote-pty', 'expired')
@@ -553,10 +644,43 @@ describe('Store', () => {
state: 'expired'
})
])
expect(session.tabsByWorktree.wt1[0].ptyId).toBe('ssh:ssh-1@@remote-pty')
expect(session.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({
[TEST_LEAF_1]: 'ssh:ssh-1@@remote-pty'
})
})
it('clears workspace bindings when marking an SSH remote PTY lease terminated', async () => {
const store = await storeWithBoundSshPane()
store.markSshRemotePtyLease('ssh-1', 'ssh:ssh-1@@remote-pty', 'terminated')
const session = store.getWorkspaceSession()
expect(store.getSshRemotePtyLeases('ssh-1')).toEqual([
expect.objectContaining({
ptyId: 'remote-pty',
state: 'terminated'
})
])
expect(session.tabsByWorktree.wt1[0].ptyId).toBeNull()
expect(session.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({})
})
// The bulk writer takes the same decision; only the operator-close state may unbind a pane.
it('keeps workspace bindings for a bulk expire and clears them for a bulk terminate', async () => {
const expiredStore = await storeWithBoundSshPane()
expiredStore.markSshRemotePtyLeases('ssh-1', 'expired')
expect(expiredStore.getWorkspaceSession().terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({
[TEST_LEAF_1]: 'ssh:ssh-1@@remote-pty'
})
const terminatedStore = await storeWithBoundSshPane()
terminatedStore.markSshRemotePtyLeases('ssh-1', 'terminated')
expect(
terminatedStore.getWorkspaceSession().terminalLayoutsByTabId.tab1.ptyIdsByLeafId
).toEqual({})
})
it('removes SSH remote PTY leases when callers pass scoped app ids', async () => {
const store = await createStore()
store.upsertSshRemotePtyLease({
@@ -89,6 +89,22 @@ function supersedeSiblingLeasesForPane(
}
}
/**
* Only `terminated` unbinds a pane. It is the operator-close state and the one written after a
* host-acknowledged stop; `expired` records that the CLIENT lost its route and says nothing about
* the remote shell (docs/reference/ssh-execution-boundary.md). Wiping the binding on `expired` made
* `resolvePersistedStablePaneOwner` return null, so `adoptStablePane` gave up and `createTerminal`
* spawned a replacement over a process that was still running. Keeping it buys a reattach ATTEMPT
* only — a genuinely dead shell is retired by `attachStablePaneOwner` on the relay's own absence
* answer, which then falls through to a fresh spawn.
*
* Supersession is the one place `expired` still scrubs a binding, and it does so explicitly in
* `supersedeSiblingLeasesForPane`: there a NEWER lease for the same pane is the evidence.
*/
function leaseStateWithdrawsBinding(state: SshRemotePtyLease['state']): boolean {
return state === 'terminated'
}
export function getSshRemotePtyLeases(
state: PersistedState,
targetId?: string
@@ -148,7 +164,7 @@ function updateSshRemotePtyLeaseStates(
): boolean {
const now = Date.now()
let changed = false
const shouldClearBindings = state === 'terminated' || state === 'expired'
const shouldClearBindings = leaseStateWithdrawsBinding(state)
const leasesToClear: SshRemotePtyLease[] = []
operations.state.sshRemotePtyLeases ??= []
for (const lease of operations.state.sshRemotePtyLeases) {
@@ -236,7 +252,7 @@ export function markSshRemotePtyLease(
if (!lease) {
return
}
const shouldClearBindings = state === 'terminated' || state === 'expired'
const shouldClearBindings = leaseStateWithdrawsBinding(state)
if (lease.state === state) {
if (shouldClearBindings && operations.clearBindingsForLeases(targetId, [lease])) {
operations.flush()
@@ -8,6 +8,22 @@ import type { StoreRuntimeState } from './store-runtime-state'
type TerminalBindingRecoveryOperationsRuntime = Pick<StoreRuntimeState, 'state'>
/**
* `terminated` is the only lease state that withdraws a pane binding. It is the operator-close
* state, and the one written after a host-acknowledged stop.
*
* `expired` is deliberately not death: every writer of it records that the CLIENT lost its route —
* a superseded sibling, a recycled relay id, a persistPtyBinding refusal, a failed reattach, a
* relay reset — and `docs/reference/ssh-execution-boundary.md` grades all of those `unverifiable`.
* Refusing the binding there strands a remote shell that is still running behind a pane that can no
* longer reach it. Keeping it authorizes a reattach ATTEMPT, never a respawn: when the shell really
* is gone, `attachStablePaneOwner` retires the binding on the relay's own absence answer and falls
* through to a fresh spawn.
*/
function sshRemotePtyLeaseWithdrawsBinding(lease: SshRemotePtyLease): boolean {
return lease.state === 'terminated'
}
export class TerminalBindingRecoveryOperations {
constructor(private readonly runtime: TerminalBindingRecoveryOperationsRuntime) {}
@@ -40,7 +56,7 @@ export class TerminalBindingRecoveryOperations {
const leases = this.runtime.state.sshRemotePtyLeases?.filter((entry) =>
this.sshRemotePtyLeaseMatchesBinding(entry, binding)
)
return !leases?.some((lease) => lease.state === 'terminated' || lease.state === 'expired')
return !leases?.some(sshRemotePtyLeaseWithdrawsBinding)
}
getRelayPtyIdForSshLeaseComparison(targetId: string, ptyId: string): string {
@@ -91,8 +107,7 @@ export class TerminalBindingRecoveryOperations {
this.runtime.state.sshRemotePtyLeases?.some(
(lease) =>
this.sshRemotePtyLeaseMatchesBinding(lease, binding) &&
lease.state !== 'terminated' &&
lease.state !== 'expired'
!sshRemotePtyLeaseWithdrawsBinding(lease)
) ?? false
)
}
@@ -0,0 +1,88 @@
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'
import { rmSync, mkdtempSync } from 'node:fs'
import { join } from 'node:path'
import { tmpdir } from 'node:os'
import { makePaneKey } from '../shared/stable-pane-id'
import { resolvePersistedStablePaneOwner } from './ipc/pty/pane/stable-owner'
import { testState, createStore, makeTerminalTab } from './persistence-test-harness'
import { TEST_LEAF_1 } from './persistence-session-fixtures'
vi.mock('electron', () => ({
app: { getPath: () => testState.dir },
safeStorage: { isEncryptionAvailable: () => false }
}))
const TARGET = 'ssh-1'
const HOST_ID = 'ssh:ssh-1' as const
const WORKTREE = 'repo1::/worktree'
const TAB = 'tab-1'
const APP_PTY_ID = 'ssh:ssh-1@@remote-pty'
function storeWithBoundRemotePane(): ReturnType<typeof createStore> {
const store = createStore()
store.upsertSshRemotePtyLease({
targetId: TARGET,
ptyId: 'remote-pty',
worktreeId: WORKTREE,
tabId: TAB,
leafId: TEST_LEAF_1,
state: 'attached'
})
store.setWorkspaceSession(
{
activeRepoId: 'repo1',
activeWorktreeId: WORKTREE,
activeTabId: TAB,
tabsByWorktree: {
[WORKTREE]: [makeTerminalTab({ id: TAB, ptyId: APP_PTY_ID, worktreeId: WORKTREE })]
},
terminalLayoutsByTabId: {
[TAB]: {
root: { type: 'leaf', leafId: TEST_LEAF_1 },
activeLeafId: TEST_LEAF_1,
expandedLeafId: null,
ptyIdsByLeafId: { [TEST_LEAF_1]: APP_PTY_ID }
}
}
},
HOST_ID
)
return store
}
/**
* `adoptStablePane` re-adopts a pane only while `resolvePersistedStablePaneOwner` can still name
* its PTY. A null owner is what routes `createTerminal` to a fresh spawn — over a remote shell that
* `expired` never claimed had died.
*/
describe('a pane whose SSH lease expired can still be re-adopted', () => {
beforeEach(() => {
testState.dir = mkdtempSync(join(tmpdir(), 'orca-test-'))
})
afterEach(() => {
rmSync(testState.dir, { recursive: true, force: true })
})
it('keeps the persisted owner after the lease expires, so adoption reattaches', () => {
const store = storeWithBoundRemotePane()
store.markSshRemotePtyLease(TARGET, APP_PTY_ID, 'expired')
expect(
resolvePersistedStablePaneOwner(store, makePaneKey(TAB, TEST_LEAF_1), WORKTREE, TARGET)
).toMatchObject({ tabId: TAB, leafId: TEST_LEAF_1, ptyId: APP_PTY_ID })
})
// Negative control for #17957: an operator close leaves `terminated`, and that must still unbind
// the pane rather than re-adopting a shell the user deliberately stopped.
it('drops the persisted owner after the lease is terminated', () => {
const store = storeWithBoundRemotePane()
store.markSshRemotePtyLease(TARGET, APP_PTY_ID, 'terminated')
expect(
resolvePersistedStablePaneOwner(store, makePaneKey(TAB, TEST_LEAF_1), WORKTREE, TARGET)
).toBeNull()
})
})