starred/orca
1
0
Fork 0
mirror of https://github.com/stablyai/orca.git synced 2026-10-20 16:03:20 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(ssh): reattach may never create terminal panes (STA-3077, part 1/5)

Browse Source 
Reconnecting an SSH worktree could add terminals the user never opened.
`restoreReattachedPtyRuntime` calls `Store.persistPtyBinding`, which is a
*creating* write: when the lease's leafId is absent from the durable layout
it splits the root and grafts a new leaf, resurrecting a pane as a side
effect of reattaching its PTY.

Add `persistPtyBinding(args, hostId, { mayCreate: false })`. The predicate
already existed inside the function — `terminalMembershipChanged` is set by
the four creating branches and only those — so the mode reuses it and the
function's own `restoreSession()` rollback rather than duplicating a
non-creating writer. Both flush points are guarded, including the legacy
non-terminal-leafId early return.

A refusal is deliberately non-destructive: it skips `registerPty`, leaves the
lease `detached`, and retires nothing. An absence in one durable store can
justify "do not create"; it can never justify "destroy" — the local partition
is not trustworthy as proof of pane deletion, because writes are dropped for
the duration of a reconnect's snapshot apply and a pane can be live on screen
having been restored from the remote host.

Reorder `persistPtyBinding` before `registerPty`, since `registerPty`
publishes a mobile surface and would otherwise create the UI a refusal
exists to prevent. A persistence *throw* still binds: under `mayCreate:
false` the refusal check precedes both flush points, so reaching a flush
proves membership did not change and the pane exists.

Part 1 of 5, ordered by blast radius. This commit can only decline to
create; it destroys nothing and ships safely on its own.
This commit is contained in:
Jinjing
2026-08-05 01:57:50 -07:00
parent d15939c5fd
commit 3fd291ba83
4 changed files with 324 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/main/persistence.test.ts
+246
View File
@@ -9140,6 +9140,252 @@ describe('Store', () => {
})
})
// STA-3077 I2: reattach may never create UI. `mayCreate: false` binds only to panes that already
// exist durably, and a refusal is non-destructive — an absence justifies "do not create", never "destroy".
it('refuses every creating branch under mayCreate:false and leaves the session untouched', async () => {
const cases = [
{
name: 'tab absent',
session: getDefaultWorkspaceSession(),
tabId: 'tab-missing'
},
{
name: 'layout.root absent',
session: {
...getDefaultWorkspaceSession(),
tabsByWorktree: {
wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })]
},
terminalLayoutsByTabId: {
tab1: {
root: null,
activeLeafId: null,
expandedLeafId: null,
ptyIdsByLeafId: {}
}
}
},
tabId: 'tab1'
},
{
name: 'leaf absent from layout',
session: {
...getDefaultWorkspaceSession(),
tabsByWorktree: {
wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })]
},
terminalLayoutsByTabId: {
tab1: {
root: { type: 'leaf', leafId: TEST_LEAF_1 },
activeLeafId: TEST_LEAF_1,
expandedLeafId: null,
ptyIdsByLeafId: { [TEST_LEAF_1]: 'pty-1' }
}
}
},
tabId: 'tab1'
},
{
name: 'layout absent entirely',
session: {
...getDefaultWorkspaceSession(),
tabsByWorktree: {
wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })]
}
},
tabId: 'tab1'
}
]
for (const testCase of cases) {
const store = await createStore()
store.setWorkspaceSession(testCase.session as ReturnType<typeof getDefaultWorkspaceSession>)
const before = structuredClone(store.getWorkspaceSession())
const outcome = store.persistPtyBinding(
{
worktreeId: 'wt1',
tabId: testCase.tabId,
leafId: TEST_LEAF_2,
ptyId: 'pty-2',
incarnationId: 'incarnation-2'
},
undefined,
{ mayCreate: false }
)
expect(outcome, testCase.name).toBe('refused')
// Deep-equal covers the layout, the tab list, the incarnation map and the tombstone clear at once.
expect(store.getWorkspaceSession(), testCase.name).toEqual(before)
}
})
it('binds under mayCreate:false when the pane exists, without bumping topology', async () => {
const store = await createStore()
store.setWorkspaceSession({
...getDefaultWorkspaceSession(),
tabsByWorktree: {
wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })]
},
terminalLayoutsByTabId: {
tab1: {
root: { type: 'leaf', leafId: TEST_LEAF_1 },
activeLeafId: TEST_LEAF_1,
expandedLeafId: null,
ptyIdsByLeafId: { [TEST_LEAF_1]: 'pty-1' }
}
},
terminalTopologyRevisionByRepoId: { wt1: 1 }
})
const outcome = store.persistPtyBinding(
{ worktreeId: 'wt1', tabId: 'tab1', leafId: TEST_LEAF_1, ptyId: 'pty-rebound' },
undefined,
{ mayCreate: false }
)
expect(outcome).toBe('bound')
const session = store.getWorkspaceSession()
expect(session.terminalLayoutsByTabId.tab1.ptyIdsByLeafId).toEqual({
[TEST_LEAF_1]: 'pty-rebound'
})
// A reattach is not a host-admitted spawn after a retirement, so the revision must not move.
expect(session.terminalTopologyRevisionByRepoId?.wt1).toBe(1)
})
it('guards the legacy non-terminal-leafId flush point under mayCreate:false', async () => {
const legacyLeafId = 'pane-legacy-1'
const boundStore = await createStore()
boundStore.setWorkspaceSession({
...getDefaultWorkspaceSession(),
tabsByWorktree: {
wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })]
}
})
expect(
boundStore.persistPtyBinding(
{ worktreeId: 'wt1', tabId: 'tab1', leafId: legacyLeafId, ptyId: 'pty-2' },
undefined,
{ mayCreate: false }
)
).toBe('bound')
const refusedStore = await createStore()
refusedStore.setWorkspaceSession(getDefaultWorkspaceSession())
expect(
refusedStore.persistPtyBinding(
{ worktreeId: 'wt1', tabId: 'tab-missing', leafId: legacyLeafId, ptyId: 'pty-2' },
undefined,
{ mayCreate: false }
)
).toBe('refused')
expect(refusedStore.getWorkspaceSession().tabsByWorktree.wt1).toBeUndefined()
})
it('leaves the default creating mode unchanged when mayCreate is omitted', async () => {
// Issue #217 regression gate: the creating behavior is load-bearing for the spawn path.
const store = await createStore()
store.setWorkspaceSession(getDefaultWorkspaceSession())
expect(
store.persistPtyBinding({
worktreeId: 'wt1',
tabId: 'tab1',
leafId: TEST_LEAF_1,
ptyId: 'pty-1'
})
).toBe('bound')
const session = store.getWorkspaceSession()
expect(session.tabsByWorktree.wt1).toEqual([expect.objectContaining({ id: 'tab1' })])
expect(session.terminalLayoutsByTabId.tab1.root).toEqual({
type: 'leaf',
leafId: TEST_LEAF_1
})
expect(
store.persistPtyBinding({
worktreeId: 'wt1',
tabId: 'tab1',
leafId: TEST_LEAF_2,
ptyId: 'pty-2'
})
).toBe('bound')
expect(store.getWorkspaceSession().terminalLayoutsByTabId.tab1.root).toEqual({
type: 'split',
direction: 'vertical',
first: { type: 'leaf', leafId: TEST_LEAF_1 },
second: { type: 'leaf', leafId: TEST_LEAF_2 }
})
})
// The ruling as an executable assertion: no absence, anywhere, may retire a lease. Two earlier
// revisions of the design proposed the destructive version; this is the gate against re-deriving it.
it('never retires a lease on any absence under mayCreate:false', async () => {
const absences = [
{
name: 'leaf absent from an existing layout',
session: {
...getDefaultWorkspaceSession(),
tabsByWorktree: {
wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })]
},
terminalLayoutsByTabId: {
tab1: {
root: { type: 'leaf', leafId: TEST_LEAF_1 },
activeLeafId: TEST_LEAF_1,
expandedLeafId: null,
ptyIdsByLeafId: { [TEST_LEAF_1]: 'pty-1' }
}
}
},
tabId: 'tab1'
},
{
name: 'tab absent from a hydrated worktree',
session: {
...getDefaultWorkspaceSession(),
tabsByWorktree: {
wt1: [makeTerminalTab({ id: 'tab1', worktreeId: 'wt1', ptyId: 'pty-1' })]
}
},
tabId: 'tab-missing'
},
{
name: 'worktree with no session entry at all',
session: getDefaultWorkspaceSession(),
tabId: 'tab-missing'
}
]
for (const absence of absences) {
const store = await createStore()
store.setWorkspaceSession(absence.session as ReturnType<typeof getDefaultWorkspaceSession>)
store.upsertSshRemotePtyLease({
targetId: 'target-1',
ptyId: 'pty-2',
worktreeId: 'wt1',
tabId: absence.tabId,
leafId: TEST_LEAF_2,
state: 'detached'
})
const sessionBefore = structuredClone(store.getWorkspaceSession())
const outcome = store.persistPtyBinding(
{ worktreeId: 'wt1', tabId: absence.tabId, leafId: TEST_LEAF_2, ptyId: 'pty-2' },
undefined,
{ mayCreate: false }
)
expect(outcome, absence.name).toBe('refused')
expect(
store.getSshRemotePtyLeases().find((lease) => lease.ptyId === 'pty-2')?.state,
absence.name
).toBe('detached')
expect(store.getWorkspaceSession(), absence.name).toEqual(sessionBefore)
}
})
it('keeps worktree deletion authoritative against stale writes and later same-path reuse', async () => {
const store = await createStore()
store.setWorktreeMeta('wt1', { displayName: 'Worktree' })
src/main/persistence.ts
+20 -3
View File
@@ -6619,6 +6619,8 @@ export class Store {
}
// Why: sync-flush the pty binding before pty:spawn returns to close the spawn/persist SIGKILL race (Issue #217).
// `mayCreate: false` binds only to panes that already exist: reattach may never create UI (STA-3077 I2).
// An absence here justifies refusing to create; it never justifies destroying — the caller leaves the lease alone.
persistPtyBinding(
args: {
worktreeId: string
@@ -6628,8 +6630,10 @@ export class Store {
incarnationId?: string
startupCwd?: string
},
hostId?: string | null
): void {
hostId?: string | null,
options?: { mayCreate?: boolean }
): 'bound' | 'refused' {
const mayCreate = options?.mayCreate ?? true
const resolvedHostId = this.resolveHostId(hostId)
const session = this.getWorkspaceSession(resolvedHostId)
if (resolvedHostId !== LOCAL_EXECUTION_HOST_ID) {
@@ -6702,6 +6706,12 @@ export class Store {
}
if (!isTerminalLeafId(args.leafId)) {
// Why: keep legacy renderer-local pane ids out of durable leaf-keyed layout state after the UUID migration.
// Legacy ids have no durable leaf-keyed layout to prove pane existence, so a minted tab is the only
// creation reachable here — refuse it rather than guess.
if (!mayCreate && terminalMembershipChanged) {
restoreSession()
return 'refused'
}
advanceTopologyAfterMembershipChange()
try {
this.flushOrThrow()
@@ -6709,7 +6719,7 @@ export class Store {
restoreSession()
throw err
}
return
return 'bound'
}
const layout = session.terminalLayoutsByTabId?.[args.tabId]
if (layout) {
@@ -6750,6 +6760,12 @@ export class Store {
}
}
}
// Why: the clone at sessionBeforeBinding predates the incarnation write, so refusing also rolls back
// terminalPtyIncarnationsByPaneKey and the tombstone clear — we are declining the bind entirely.
if (!mayCreate && terminalMembershipChanged) {
restoreSession()
return 'refused'
}
advanceTopologyAfterMembershipChange()
try {
this.flushOrThrow()
@@ -6757,6 +6773,7 @@ export class Store {
restoreSession()
throw err
}
return 'bound'
}
// ── SSH Targets ────────────────────────────────────────────────────
src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts
+19 -8
View File
@@ -413,13 +413,22 @@ describe('SshRelaySession reconnect incarnation ordering', () => {
incarnationId
})
expect(runtime.onPtySpawned).not.toHaveBeenCalled()
expect(mockStore.persistPtyBinding).toHaveBeenCalledWith({
worktreeId: 'worktree-1',
tabId: 'tab-1',
leafId: INCARNATION_LEAF_ID,
ptyId: APP_PTY_ID,
incarnationId
})
// Reattach binds non-creatively (STA-3077 I2): it may never mint a tab, layout, or leaf.
expect(mockStore.persistPtyBinding).toHaveBeenCalledWith(
{
worktreeId: 'worktree-1',
tabId: 'tab-1',
leafId: INCARNATION_LEAF_ID,
ptyId: APP_PTY_ID,
incarnationId
},
undefined,
{ mayCreate: false }
)
// The store rules before registerPty publishes a surface, so a refusal creates no UI.
expect(vi.mocked(mockStore.persistPtyBinding).mock.invocationCallOrder[0]).toBeLessThan(
vi.mocked(runtime.registerPty).mock.invocationCallOrder[0]!
)
expect(vi.mocked(mockStore.persistPtyBinding).mock.invocationCallOrder[0]).toBeLessThan(
vi.mocked(mockStore.markSshRemotePtyLeasesAttachedAsync).mock.invocationCallOrder[0]!
)
@@ -527,7 +536,9 @@ describe('SshRelaySession reconnect incarnation ordering', () => {
})
expect(setPtyOwnership).toHaveBeenCalledWith(APP_PTY_ID, 'target-1')
expect(mockStore.persistPtyBinding).toHaveBeenCalledWith(
expect.objectContaining({ ptyId: APP_PTY_ID, incarnationId: currentIncarnationId })
expect.objectContaining({ ptyId: APP_PTY_ID, incarnationId: currentIncarnationId }),
undefined,
{ mayCreate: false }
)
expect(mockWindow.webContents.send).toHaveBeenCalledWith('pty:replay', {
id: APP_PTY_ID,
src/main/ssh/ssh-relay-session.ts
+39 -15
View File
@@ -2124,15 +2124,21 @@ export class SshRelaySession {
return
}
setPtyOwnership(appPtyId, this.targetId)
let restoreOutcome: 'bound' | 'refused' = 'bound'
if (attachResult.incarnationId) {
restorePtyIncarnation(appPtyId, attachResult.incarnationId)
this.restoreReattachedPtyRuntime(
restoreOutcome = this.restoreReattachedPtyRuntime(
appPtyId,
attachResult.incarnationId,
activeLeaseByPtyId.get(ptyId)
)
}
attachedLeaseIds.add(ptyId)
// Why: the relay attach succeeded, so activation bookkeeping proceeds; only the durable bind was
// refused, and a refused bind must not mark the lease attached. It stays detached and is retried
// next reconnect — retirement needs positive evidence, which an absent pane is not.
if (restoreOutcome === 'bound') {
attachedLeaseIds.add(ptyId)
}
pendingReattach.activated = true
recoveryActivationLease?.commit()
recoveryActivationLease = undefined
@@ -2191,31 +2197,49 @@ export class SshRelaySession {
this.runtime?.acceptPtyIncarnationForExit(appPtyId, ptyIncarnation)
}
// Returns 'refused' when the pane no longer exists durably, so the caller leaves the lease detached.
private restoreReattachedPtyRuntime(
appPtyId: string,
incarnationId: string,
lease: SshPtyLease | undefined
): void {
): 'bound' | 'refused' {
if (lease?.worktreeId && lease.tabId && lease.leafId) {
let outcome: 'bound' | 'refused'
try {
// Why: persist decides first — registerPty publishes a mobile surface, so binding before the
// store rules would create the very UI a refusal exists to prevent.
outcome = this.store.persistPtyBinding(
{
worktreeId: lease.worktreeId,
tabId: lease.tabId,
leafId: lease.leafId,
ptyId: appPtyId,
incarnationId
},
undefined,
{ mayCreate: false }
)
} catch (error) {
console.error('[ssh-relay-session] Failed to persist reconnect incarnation:', error)
// Why: under mayCreate:false the refusal check precedes both flush points, so a throw proves
// membership did NOT change — the pane exists and only the disk write failed. Keep the live PTY.
outcome = 'bound'
}
if (outcome === 'refused') {
console.log(
`[ssh-relay-session] Reattach refused for ${appPtyId}: pane ${lease.tabId}:${lease.leafId} is not in durable layout; leaving lease detached`
)
return 'refused'
}
this.runtime?.registerPty(appPtyId, lease.worktreeId, this.targetId, {
tabId: lease.tabId,
leafId: lease.leafId,
incarnationId
})
try {
this.store.persistPtyBinding({
worktreeId: lease.worktreeId,
tabId: lease.tabId,
leafId: lease.leafId,
ptyId: appPtyId,
incarnationId
})
} catch (error) {
console.error('[ssh-relay-session] Failed to persist reconnect incarnation:', error)
}
return
return 'bound'
}
this.runtime?.onPtySpawned(appPtyId, incarnationId, { awaitsRegistration: false })
return 'bound'
}
private async attachPtyWithRetry(