From 24614bdf6ce97c6db1daca6d091da807090f86fd Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Fri, 28 Aug 2026 01:01:11 -0700 Subject: [PATCH] fix(relay): don't re-run an agent resume against a relay that never held the terminal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a stable pane's reattach fails with positive absence, the fallback re-spawns using the original request's spawn options, replaying `launchAgent` / resume argv / `command`. On a cold restore that is right. After a relay daemon replacement it is not: the binding may name a PTY from a previous daemon whose shell is still alive as an orphan, so the replay starts a SECOND agent against the same worktree. `relay.status` already reports `uptimeMs`. A daemon whose uptime is shorter than the age of the binding it just answered about demonstrably never owned that binding, so its positive absence is `unverifiable`, not `exited`. On that verdict the fallback still spawns a shell — the spawn is never gated — but drops the startup intent and sets the existing `agentResumeUnavailable` flag so the pane shows the resume-unavailable banner instead of claiming the old session came back. Loss of contact produces no verdict: a missing host RPC, a rejected or timed-out `relay.status`, a non-object reply, an absent or unusable `uptimeMs`, and a binding nothing can date all leave today's behavior exactly as-is. Client-side policy only — no new wire field and no new stream opcode. STA-5698 --- src/main/ipc/pty/ipc/spawn-commit.ts | 5 +- .../relay-replacement-resume-suppression.ts | 145 +++++++++++ src/main/ipc/pty/pane/stable-owner.ts | 24 +- ...-pane-relay-age-resume-suppression.test.ts | 243 ++++++++++++++++++ src/main/ipc/pty/runtime/spawn-commit.ts | 5 +- src/main/providers/pty-spawn-result.ts | 3 + 6 files changed, 420 insertions(+), 5 deletions(-) create mode 100644 src/main/ipc/pty/pane/relay-replacement-resume-suppression.ts create mode 100644 src/main/ipc/pty/pane/stable-pane-relay-age-resume-suppression.test.ts diff --git a/src/main/ipc/pty/ipc/spawn-commit.ts b/src/main/ipc/pty/ipc/spawn-commit.ts index 7793ccf17ee..43a4832ebb7 100644 --- a/src/main/ipc/pty/ipc/spawn-commit.ts +++ b/src/main/ipc/pty/ipc/spawn-commit.ts @@ -130,7 +130,7 @@ export async function commitPtyIpcSpawn(ctx: PtyIpcSpawnState): Promise 0 ? age : null +} + +/** + * Does the relay that just answered "that PTY is absent" predate the binding it answered about? + * + * A daemon whose reported uptime is shorter than the binding's age cannot have owned the bound + * process, so its positive absence is `unverifiable` rather than `exited` + * (docs/reference/ssh-execution-boundary.md) — the shell it named may still be alive as an orphan + * of the daemon it replaced. Only an answer that ARRIVES and is demonstrably younger says so: + * a missing provider, a rejected or timed-out `relay.status`, an absent or unusable `uptimeMs`, + * and an undatable binding all return false, leaving the caller on today's behavior. Silence is + * never promoted to a verdict here. + * + * Reads only fields `relay.status` already publishes; adds no request field and no opcode. + */ +export async function answeringRelayPredatesBinding(args: { + provider: IPtyProvider + store?: Store | undefined + connectionId?: string | null + ptyId: string + now?: () => number +}): Promise { + const { provider, connectionId, ptyId } = args + if (!connectionId) { + return false + } + const age = bindingAgeMs(args.store, connectionId, ptyId, (args.now ?? Date.now)()) + if (age === null) { + return false + } + let status: unknown + try { + // Optional call: a provider with no host RPC yields undefined, which is not an answer. + status = await provider.requestHostRpc?.( + 'relay.status', + {}, + { + timeoutMs: RELAY_AGE_STATUS_TIMEOUT_MS + } + ) + } catch { + return false + } + if (!status || typeof status !== 'object') { + return false + } + const uptimeMs = (status as { uptimeMs?: unknown }).uptimeMs + if (typeof uptimeMs !== 'number' || !Number.isFinite(uptimeMs) || uptimeMs < 0) { + return false + } + return age > uptimeMs + RELAY_AGE_CLOCK_SLACK_MS +} + +/** + * The absence fallback's spawn, minus the startup intent a replacement relay never held. + * + * Returns null whenever nothing proves a replacement, leaving the caller on its unchanged path — + * the spawn itself is never gated here, only the intent replayed into it. + */ +export async function spawnWithoutReplayedIntent(args: { + provider: IPtyProvider + store?: Store | undefined + connectionId?: string | null + ptyId: string + spawnOptions: PtySpawnOptions + onFreshSpawn?: (result: PtySpawnResult) => void +}): Promise { + if (!carriesStartupIntent(args.spawnOptions) || !(await answeringRelayPredatesBinding(args))) { + return null + } + const result = await args.provider.spawn(stripStartupIntent(args.spawnOptions)) + args.onFreshSpawn?.(result) + return { ...result, agentResumeUnavailable: true as const } +} diff --git a/src/main/ipc/pty/pane/stable-owner.ts b/src/main/ipc/pty/pane/stable-owner.ts index 9442e914e1f..48bffa1b781 100644 --- a/src/main/ipc/pty/pane/stable-owner.ts +++ b/src/main/ipc/pty/pane/stable-owner.ts @@ -12,6 +12,8 @@ import { } from '../../../daemon/daemon-errors' import { ptyIncarnationById, ptyOwnership } from '../provider/ownership-state' import { isPtyAlreadyGoneError } from '../provider/liveness' +import { isSshPtyAbsentFromRelayError } from '../../../providers/ssh-pty-errors' +import { spawnWithoutReplayedIntent } from './relay-replacement-resume-suppression' import { clearProviderPtyState } from '../provider/state-cleanup' export type StablePaneOwner = { @@ -166,6 +168,8 @@ export type StablePaneSpawnContext = { connectionId?: string | null resolveOwner?: () => StablePaneOwner | null onFreshSpawn?: (result: PtySpawnResult) => void + /** Fired when a reachable relay positively answered this exact PTY id absent. */ + onRelayPositiveAbsence?: () => void } export function stablePanePersistenceFence( @@ -242,6 +246,9 @@ export async function attachStablePaneOwner( if (!isPtyAlreadyGoneError(error)) { throw error } + if (isSshPtyAbsentFromRelayError(error)) { + args.onRelayPositiveAbsence?.() + } const ownerBeforeRetire = args.resolveOwner?.() if ( ownerBeforeRetire && @@ -282,10 +289,25 @@ export async function spawnForStablePane( args: StablePaneSpawnContext ): Promise<{ result: PtySpawnResult; owner: StablePaneOwner | null }> { if (args.owner) { - const attached = await attachStablePaneOwner({ ...args, owner: args.owner }) + let relayAnsweredAbsent = false + const attached = await attachStablePaneOwner({ + ...args, + owner: args.owner, + onRelayPositiveAbsence: () => { + relayAnsweredAbsent = true + } + }) if (attached) { return attached } + // Why: the user still needs a terminal, so the spawn itself is never gated — only the startup + // intent is, and only when the answering relay proves it never held the binding it answered for. + const plain = relayAnsweredAbsent + ? await spawnWithoutReplayedIntent({ ...args, ptyId: args.owner.ptyId }) + : null + if (plain) { + return { result: plain, owner: null } + } } const result = await args.provider.spawn(args.spawnOptions) args.onFreshSpawn?.(result) diff --git a/src/main/ipc/pty/pane/stable-pane-relay-age-resume-suppression.test.ts b/src/main/ipc/pty/pane/stable-pane-relay-age-resume-suppression.test.ts new file mode 100644 index 00000000000..e513a911ffd --- /dev/null +++ b/src/main/ipc/pty/pane/stable-pane-relay-age-resume-suppression.test.ts @@ -0,0 +1,243 @@ +import { describe, expect, it, vi } from 'vitest' +import { + SSH_SESSION_EXPIRED_ERROR, + SshPtyAbsentFromRelayError +} from '../../../providers/ssh-pty-errors' +import type { Store } from '../../../persistence' +import type { IPtyProvider, PtySpawnOptions } from '../../../providers/types' +import { spawnForStablePane, type StablePaneOwner } from './stable-owner' + +const LEAF = '1b3f2c4d-5e6a-4b7c-8d9e-0f1a2b3c4d5e' +const CONNECTION = 'conn-1' +const PTY_ID = `ssh:${CONNECTION}@@pty-1` +const OWNER: StablePaneOwner = { + tabId: 'tab-1', + leafId: LEAF, + ptyId: PTY_ID, + hasPersistedBinding: true +} +const BINDING_AGE_MS = 2 * 60 * 60_000 + +/** Only the lease read matters here; the retirement path is covered by the absence-respawn suite. */ +function leaseStore(createdAt: number | undefined): Store { + return { + getSshRemotePtyLeases: () => + createdAt === undefined + ? [] + : [ + { + targetId: CONNECTION, + ptyId: 'pty-1', + state: 'detached', + createdAt, + updatedAt: createdAt + } + ], + getWorkspaceSession: () => ({}) as never, + setWorkspaceSession: () => {}, + flushOrThrow: () => {} + } as unknown as Store +} + +function runFallback(options: { + spawnOptions: PtySpawnOptions + relayStatus: () => Promise + leaseCreatedAt?: number | undefined + now?: number +}): { + run: () => ReturnType + spawn: ReturnType + requestHostRpc: ReturnType +} { + const now = options.now ?? Date.now() + const spawn = vi + .fn() + .mockRejectedValueOnce(new SshPtyAbsentFromRelayError(`${SSH_SESSION_EXPIRED_ERROR}: pty-1`)) + .mockResolvedValueOnce({ id: `ssh:${CONNECTION}@@pty-2`, isReattach: false }) + const requestHostRpc = vi.fn().mockImplementation(() => options.relayStatus()) + return { + spawn, + requestHostRpc, + run: () => + spawnForStablePane({ + runtime: undefined, + store: leaseStore( + 'leaseCreatedAt' in options ? options.leaseCreatedAt : now - BINDING_AGE_MS + ), + provider: { spawn, requestHostRpc } as unknown as IPtyProvider, + spawnOptions: options.spawnOptions, + owner: OWNER, + worktreeId: 'worktree-1', + connectionId: CONNECTION, + resolveOwner: () => null + }) + } +} + +/** A replacement relay: up 5 minutes against a 2-hour-old binding. */ +const youngerRelay = () => Promise.resolve({ uptimeMs: 5 * 60_000, pid: 4242 }) +/** The relay that could have owned the binding: up 4 hours. */ +const olderRelay = () => Promise.resolve({ uptimeMs: 4 * 60 * 60_000, pid: 4242 }) + +const RESUME_ARGV: PtySpawnOptions = { + cols: 80, + rows: 24, + command: 'claude --resume abc123', + commandDelivery: 'provider' +} +const LAUNCH_AGENT: PtySpawnOptions = { cols: 80, rows: 24, launchAgent: 'claude' } +const BARE_COMMAND: PtySpawnOptions = { cols: 80, rows: 24, command: 'npm run dev' } + +describe('absence fallback against a relay younger than the binding', () => { + it('spawns a shell but drops a resume argv the replacement relay never held', async () => { + const { run, spawn } = runFallback({ spawnOptions: RESUME_ARGV, relayStatus: youngerRelay }) + + const { result } = await run() + + expect(spawn).toHaveBeenCalledTimes(2) + expect(spawn.mock.calls[1]![0]).toMatchObject({ + command: undefined, + commandDelivery: undefined + }) + expect(result.id).toBe(`ssh:${CONNECTION}@@pty-2`) + expect(result.agentResumeUnavailable).toBe(true) + }) + + it('drops launchAgent rather than starting a second agent over the same worktree', async () => { + const { run, spawn } = runFallback({ spawnOptions: LAUNCH_AGENT, relayStatus: youngerRelay }) + + const { result } = await run() + + expect(spawn).toHaveBeenCalledTimes(2) + expect(spawn.mock.calls[1]![0]).toMatchObject({ launchAgent: undefined }) + expect(result.agentResumeUnavailable).toBe(true) + }) + + it('drops a startup command', async () => { + const { run, spawn } = runFallback({ spawnOptions: BARE_COMMAND, relayStatus: youngerRelay }) + + const { result } = await run() + + expect(spawn.mock.calls[1]![0]).toMatchObject({ command: undefined }) + expect(result.agentResumeUnavailable).toBe(true) + }) + + it('never gates the spawn itself — a shell is always produced', async () => { + for (const spawnOptions of [RESUME_ARGV, LAUNCH_AGENT, BARE_COMMAND]) { + const { run, spawn } = runFallback({ spawnOptions, relayStatus: youngerRelay }) + + const { result } = await run() + + expect(result.id).toBe(`ssh:${CONNECTION}@@pty-2`) + expect(spawn).toHaveBeenCalledTimes(2) + } + }) +}) + +describe('absence fallback when nothing proves the relay was replaced', () => { + it('replays the resume when the answering relay predates the binding', async () => { + const { run, spawn } = runFallback({ spawnOptions: RESUME_ARGV, relayStatus: olderRelay }) + + const { result } = await run() + + expect(spawn.mock.calls[1]![0]).toMatchObject({ command: 'claude --resume abc123' }) + expect(result.agentResumeUnavailable).toBeUndefined() + }) + + it('replays the resume when relay.status rejects — silence is not a verdict', async () => { + const { run, spawn } = runFallback({ + spawnOptions: RESUME_ARGV, + relayStatus: () => Promise.reject(new Error('relay status failed')) + }) + + const { result } = await run() + + expect(spawn.mock.calls[1]![0]).toMatchObject({ command: 'claude --resume abc123' }) + expect(result.agentResumeUnavailable).toBeUndefined() + }) + + it('replays the resume when relay.status answers with a non-object', async () => { + const { run, spawn } = runFallback({ + spawnOptions: RESUME_ARGV, + relayStatus: () => Promise.resolve(null) + }) + + const { result } = await run() + + expect(spawn.mock.calls[1]![0]).toMatchObject({ command: 'claude --resume abc123' }) + expect(result.agentResumeUnavailable).toBeUndefined() + }) + + it('replays the resume when relay.status omits uptimeMs', async () => { + const { run, spawn } = runFallback({ + spawnOptions: RESUME_ARGV, + relayStatus: () => Promise.resolve({ pid: 4242 }) + }) + + const { result } = await run() + + expect(spawn.mock.calls[1]![0]).toMatchObject({ command: 'claude --resume abc123' }) + expect(result.agentResumeUnavailable).toBeUndefined() + }) + + it('replays the resume on a cold restore with no lease to date the binding', async () => { + const { run, spawn, requestHostRpc } = runFallback({ + spawnOptions: RESUME_ARGV, + relayStatus: youngerRelay, + leaseCreatedAt: undefined + }) + + const { result } = await run() + + expect(requestHostRpc).not.toHaveBeenCalled() + expect(spawn.mock.calls[1]![0]).toMatchObject({ command: 'claude --resume abc123' }) + expect(result.agentResumeUnavailable).toBeUndefined() + }) + + it('replays the resume when the relay is younger only inside the clock slack', async () => { + // Two independently measured elapsed times; a sub-slack gap is jitter, not a replacement. + const { run, spawn } = runFallback({ + spawnOptions: RESUME_ARGV, + relayStatus: () => Promise.resolve({ uptimeMs: BINDING_AGE_MS - 10_000 }) + }) + + const { result } = await run() + + expect(spawn.mock.calls[1]![0]).toMatchObject({ command: 'claude --resume abc123' }) + expect(result.agentResumeUnavailable).toBeUndefined() + }) + + it('replays the resume for a provider with no host RPC to ask', async () => { + const spawn = vi + .fn() + .mockRejectedValueOnce(new SshPtyAbsentFromRelayError(`${SSH_SESSION_EXPIRED_ERROR}: pty-1`)) + .mockResolvedValueOnce({ id: `ssh:${CONNECTION}@@pty-2`, isReattach: false }) + + const { result } = await spawnForStablePane({ + runtime: undefined, + store: leaseStore(Date.now() - BINDING_AGE_MS), + provider: { spawn } as unknown as IPtyProvider, + spawnOptions: RESUME_ARGV, + owner: OWNER, + worktreeId: 'worktree-1', + connectionId: CONNECTION, + resolveOwner: () => null + }) + + expect(spawn.mock.calls[1]![0]).toMatchObject({ command: 'claude --resume abc123' }) + expect(result.agentResumeUnavailable).toBeUndefined() + }) + + it('asks nothing at all when the request carried no startup intent', async () => { + const { run, spawn, requestHostRpc } = runFallback({ + spawnOptions: { cols: 80, rows: 24 }, + relayStatus: youngerRelay + }) + + const { result } = await run() + + expect(requestHostRpc).not.toHaveBeenCalled() + expect(spawn).toHaveBeenCalledTimes(2) + expect(result.agentResumeUnavailable).toBeUndefined() + }) +}) diff --git a/src/main/ipc/pty/runtime/spawn-commit.ts b/src/main/ipc/pty/runtime/spawn-commit.ts index 38f9ced106e..f0ccee8b9b2 100644 --- a/src/main/ipc/pty/runtime/spawn-commit.ts +++ b/src/main/ipc/pty/runtime/spawn-commit.ts @@ -225,10 +225,11 @@ export async function commitRuntimePtySpawn(ctx: RuntimePtySpawnState) { if (!ctx.stablePaneOwner) { ctx.deps.runtime?.noteTerminalSpawnCommand?.(ctx.result.id, ctx.launchCommand ?? null) } - if (ctx.isClaudeLaunch && !ctx.stablePaneOwner) { + if (ctx.isClaudeLaunch && !ctx.stablePaneOwner && !ctx.result.agentResumeUnavailable) { markClaudePtySpawned(ctx.result.id) } - if (args.telemetry && !ctx.stablePaneOwner) { + // Why: a suppressed resume produced a plain shell, so there is no agent launch to report. + if (args.telemetry && !ctx.stablePaneOwner && !ctx.result.agentResumeUnavailable) { const agentKindParse = agentKindSchema.safeParse(args.telemetry.agent_kind) const launchSourceParse = launchSourceSchema.safeParse(args.telemetry.launch_source) const requestKindParse = requestKindSchema.safeParse(args.telemetry.request_kind) diff --git a/src/main/providers/pty-spawn-result.ts b/src/main/providers/pty-spawn-result.ts index 01cf9193f7e..9dd40fd6f04 100644 --- a/src/main/providers/pty-spawn-result.ts +++ b/src/main/providers/pty-spawn-result.ts @@ -57,6 +57,9 @@ export type PtySpawnResult = { snapshotTerminalOwner?: TerminalOwner /** True when the spawn reattached to an existing daemon session. */ isReattach?: boolean + /** Main dropped this spawn's resume/launch intent, so the pane is a NEW session. Set by main, + * not the provider; the renderer swaps the restored banner for "resume unavailable". */ + agentResumeUnavailable?: true /** Last OSC title tracked by the daemon session the snapshot came from. * Seeds main's terminal title records after a relaunch; never replayed * into a terminal. */