From 9d2b378eb39d5f2292eb7a0d13bf95fecd637681 Mon Sep 17 00:00:00 2001 From: Merge Sim Date: Tue, 1 Sep 2026 16:12:50 -0700 Subject: [PATCH] fix: retain relay epoch owner across spawn retries --- src/main/ipc/pty/pane/relay-pty-mint-epoch.ts | 29 +++++- .../pty/pane/stable-owner-relay-epoch.test.ts | 89 ++++++++++++++++++- src/main/ipc/pty/pane/stable-owner.ts | 24 ++--- sta5698-epoch-gate-plan.md | 47 ++++++++++ 4 files changed, 170 insertions(+), 19 deletions(-) create mode 100644 sta5698-epoch-gate-plan.md diff --git a/src/main/ipc/pty/pane/relay-pty-mint-epoch.ts b/src/main/ipc/pty/pane/relay-pty-mint-epoch.ts index ad88364610c..569a62a1301 100644 --- a/src/main/ipc/pty/pane/relay-pty-mint-epoch.ts +++ b/src/main/ipc/pty/pane/relay-pty-mint-epoch.ts @@ -68,7 +68,7 @@ export function rememberRetiredRelayEpochOwner(args: { retiredRelayOwnerByPane.set(key, args.ownerPtyId) } -export function takeRetiredRelayEpochOwner( +export function peekRetiredRelayEpochOwner( connectionId: string | null | undefined, paneKey: string | null | undefined ): string | undefined { @@ -76,8 +76,31 @@ export function takeRetiredRelayEpochOwner( return undefined } const key = retiredRelayOwnerKey(connectionId, paneKey) - const ownerPtyId = retiredRelayOwnerByPane.get(key) - retiredRelayOwnerByPane.delete(key) + return retiredRelayOwnerByPane.get(key) +} + +/** Remove a retired owner only after the replacement PTY has been committed. */ +export function consumeRetiredRelayEpochOwner( + connectionId: string | null | undefined, + paneKey: string | null | undefined, + ownerPtyId: string | undefined +): void { + if (!connectionId || !paneKey || !ownerPtyId) { + return + } + const key = retiredRelayOwnerKey(connectionId, paneKey) + if (retiredRelayOwnerByPane.get(key) === ownerPtyId) { + retiredRelayOwnerByPane.delete(key) + } +} + +/** @deprecated Use peek + consume after a successful spawn. */ +export function takeRetiredRelayEpochOwner( + connectionId: string | null | undefined, + paneKey: string | null | undefined +): string | undefined { + const ownerPtyId = peekRetiredRelayEpochOwner(connectionId, paneKey) + consumeRetiredRelayEpochOwner(connectionId, paneKey, ownerPtyId) return ownerPtyId } diff --git a/src/main/ipc/pty/pane/stable-owner-relay-epoch.test.ts b/src/main/ipc/pty/pane/stable-owner-relay-epoch.test.ts index 86b71ecd015..608f65c719d 100644 --- a/src/main/ipc/pty/pane/stable-owner-relay-epoch.test.ts +++ b/src/main/ipc/pty/pane/stable-owner-relay-epoch.test.ts @@ -1,10 +1,7 @@ import { describe, expect, it, vi } from 'vitest' import type { IPtyProvider, PtySpawnOptions, PtySpawnResult } from '../../../providers/types' import { toAppSshPtyId } from '../../../providers/ssh-pty-id' -import { - rememberRetiredRelayEpochOwner, - takeRetiredRelayEpochOwner -} from './relay-pty-mint-epoch' +import { rememberRetiredRelayEpochOwner, takeRetiredRelayEpochOwner } from './relay-pty-mint-epoch' import { spawnForStablePane, type StablePaneOwner } from './stable-owner' type EpochAwareSpawnOptions = PtySpawnOptions & { resumeProviderSession?: unknown } @@ -241,6 +238,90 @@ describe('spawnForStablePane relay epoch gate', () => { expect(harness.spawns[0]).toEqual(spawnOptions) }) + it('does not let a retired owner gate an unrelated later restore of the same pane', async () => { + const paneKey = 'tab-lifetime:55555555-5555-4555-8555-555555555555' + const retiredPtyId = toAppSshPtyId('remote', 'pty2:previous:1') + const provider = { + requestHostRpc: vi.fn(async () => ({ ptyIdMintEpoch: 'current' })), + spawn: vi.fn(async (options: EpochAwareSpawnOptions) => + options.attachOnly + ? { id: retiredPtyId, isReattach: true } + : { id: toAppSshPtyId('remote', 'pty2:current:2') } + ) + } as unknown as IPtyProvider + rememberRetiredRelayEpochOwner({ + connectionId: 'remote', + paneKey, + ownerPtyId: retiredPtyId + }) + + await spawnForStablePane({ + runtime: undefined, + provider, + spawnOptions: { cols: 80, rows: 24 }, + owner: owner('pty2:previous:1'), + connectionId: 'remote', + paneKey + }) + + const secondOptions = agentSpawnOptions() + await spawnForStablePane({ + runtime: undefined, + provider, + spawnOptions: secondOptions, + owner: null, + connectionId: 'remote', + paneKey + }) + expect(provider.spawn).toHaveBeenLastCalledWith(secondOptions) + }) + + it('retains the retired owner when the first replacement spawn fails', async () => { + const paneKey = 'tab-retry:66666666-6666-4666-8666-666666666666' + const requestHostRpc = vi.fn(async () => ({ ptyIdMintEpoch: 'current' })) + let attempts = 0 + const provider = { + requestHostRpc, + spawn: vi.fn(async (options: EpochAwareSpawnOptions) => { + if (options.attachOnly) { + throw new Error('not found') + } + attempts += 1 + if (attempts === 1) { + throw new Error('relay replacement in progress') + } + return { id: toAppSshPtyId('remote', 'pty2:current:2') } + }) + } as unknown as IPtyProvider + rememberRetiredRelayEpochOwner({ + connectionId: 'remote', + paneKey, + ownerPtyId: toAppSshPtyId('remote', 'pty2:previous:1') + }) + + await expect( + spawnForStablePane({ + runtime: undefined, + provider, + spawnOptions: agentSpawnOptions(), + owner: null, + connectionId: 'remote', + paneKey + }) + ).rejects.toThrow('relay replacement in progress') + + const retried = await spawnForStablePane({ + runtime: undefined, + provider, + spawnOptions: agentSpawnOptions(), + owner: null, + connectionId: 'remote', + paneKey + }) + expect(retried.result.agentResumeUnavailable).toBe(true) + expect(provider.spawn).toHaveBeenCalledTimes(2) + }) + it('does not treat a new agent launch as a retired-owner resume', async () => { const harness = createProvider({ ptyIdMintEpoch: 'current' }) const paneKey = 'tab-new-agent:44444444-4444-4444-8444-444444444444' diff --git a/src/main/ipc/pty/pane/stable-owner.ts b/src/main/ipc/pty/pane/stable-owner.ts index 862048745ef..0db78e11683 100644 --- a/src/main/ipc/pty/pane/stable-owner.ts +++ b/src/main/ipc/pty/pane/stable-owner.ts @@ -13,10 +13,7 @@ import { import { ptyIncarnationById, ptyOwnership } from '../provider/ownership-state' import { isPtyAlreadyGoneError } from '../provider/liveness' import { clearProviderPtyState } from '../provider/state-cleanup' -import { - deriveStablePaneFreshSpawnOptions, - takeRetiredRelayEpochOwner -} from './relay-pty-mint-epoch' +import * as relayEpoch from './relay-pty-mint-epoch' export type StablePaneOwner = { handle?: string @@ -292,19 +289,22 @@ export async function spawnForStablePane( return attached } } - const retiredRelayOwnerPtyId = takeRetiredRelayEpochOwner(args.connectionId, args.paneKey) - const relayEpochOwnerPtyId = - args.owner?.ptyId ?? - (args.spawnOptions.resumeProviderSession || args.spawnOptions.agentSessionEnsure - ? retiredRelayOwnerPtyId - : undefined) - const freshSpawn = await deriveStablePaneFreshSpawnOptions({ + const retiredRelayOwnerPtyId = relayEpoch.peekRetiredRelayEpochOwner( + args.connectionId, + args.paneKey + ) + const freshSpawn = await relayEpoch.deriveStablePaneFreshSpawnOptions({ provider: args.provider, - ownerPtyId: relayEpochOwnerPtyId, + ownerPtyId: + args.owner?.ptyId ?? + (args.spawnOptions.resumeProviderSession || args.spawnOptions.agentSessionEnsure + ? retiredRelayOwnerPtyId + : undefined), connectionId: args.connectionId, spawnOptions: args.spawnOptions }) const providerResult = await args.provider.spawn(freshSpawn.options) + relayEpoch.consumeRetiredRelayEpochOwner(args.connectionId, args.paneKey, retiredRelayOwnerPtyId) const result = freshSpawn.agentResumeDeclined ? { ...providerResult, agentResumeUnavailable: true as const } : providerResult diff --git a/sta5698-epoch-gate-plan.md b/sta5698-epoch-gate-plan.md new file mode 100644 index 00000000000..ac4c476a0a4 --- /dev/null +++ b/sta5698-epoch-gate-plan.md @@ -0,0 +1,47 @@ +# STA-5698 relay epoch gate lifetime fix + +## Diagnosis to verify + +The runtime failures share the retired-owner side table's identity/lifetime bug. A +`(connectionId, paneKey)` tombstone is consumed by whichever later spawn happens to +use that pane, so a later same-relay restore can compare against an unrelated old +epoch. Conversely, a failed fresh spawn consumes the only tombstone before a relay +replacement retries the same absence, making that retry `unknown` and allowing the +resume. + +## Design + +The first proposal to key by `effectiveSessionAppId` is rejected: fresh resume paths +intentionally omit `sessionId`, so that value cannot identify the absence. The fix is +scoped to direct IPC/SSH paths that reproduce STA-5698; paired-runtime methods and +stream opcodes remain unchanged. Relay retirement (including the synthetic +`webContents.send('pty:exit', {id, code:-1})` in +`ssh-relay-session.handlePtyReattachFailure`) records `{connectionId,paneKey,ownerPtyId}` +in the bounded main-process map. No renderer wire shape changes: this map is consulted +only by the main process when a fresh IPC/SSH spawn already carries the pane key. + +Presentation is transactional: lookup peeks the retired owner, and the entry is removed +only after provider spawn succeeds (including a successful rebind or shell replacement). +A provider rejection leaves the entry for the next relay incarnation retry; a later +restore after successful rebind cannot see it. A restore with no entry remains `unknown`; +known-different still declines agent resume and known-same still resumes. Concurrent +lookups are serialized by the spawn path, and the map remains bounded at 256 with +recency refresh. + +## Tests and verification + +Add red-before-fix tests in stable-owner and SSH relay suites: synthetic relay exit records +the exact owner; first provider rejection leaves that owner for a successful retry; success +removes it and blocks replay; an attach/rebind followed by an unrelated restore of the same +pane is unknown; same-epoch, legacy/no-entry, and concurrent presentation controls remain +unchanged. Run focused tests, typecheck, oxlint/format, relay build, then both runtime +scenarios in separate and back-to-back app sessions against `openclaw`. + +## OSS precedent + +The synced desktop terminal manager keeps killed-session tombstones separate from +live-resource maps, bounds them, refreshes recency on re-record, and consumes them on +matching reuse. It clears the tombstone before create and does not roll it back on +create failure; rollback-on-provider-failure is an Orca requirement demonstrated by +STA-5698, not OSS precedent. This change borrows only separation, bounding, recency, +and explicit reuse.