From cbe91f0899fa1b7d55ea0caa6cc3fb6699203a85 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Mon, 31 Aug 2026 23:28:40 -0700 Subject: [PATCH] fix(ssh): reap relay PTYs the host attests this client orphaned (#9819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing reconciled the relay's live PTY list against what the app still tracks, so a pane whose id the client lost held a relay slot until the daemon died — the "Maximum number of PTY sessions reached (50)" wall. #9819 proposed sweeping any pane-bound PTY "the app no longer owns or leases". That is not proof of orphanhood: absence from a client-side set is `unverifiable` by construction, and a second machine on the same build attaches to the SAME relay, where its live agents are missing from this client's store for exactly the same reason a real orphan is. So the host attests instead. PtyHandler records the authenticated consumer identity behind the connection that asked it to spawn each PTY, read from the live grant rather than from a spawn parameter, and publishes it on pty.listProcesses alongside a host-measured age and whether the PTY is pane-bound. A client may stop a PTY only when the owning host names THIS client as its creator, this client holds the negotiated session-owner grant, the PTY is pane-bound, older than 30s, carries no adoptable agent session, and this client has no lease, live route or pending stop for it. Every stop is fenced on the incarnation the same listing published. All three published fields are optional (Rule 1), and every absence reads as unknown: against a host that predates them the sweep stops nothing. Revived PTYs are deliberately left unattested. --- src/main/providers/pty-process-info.ts | 9 + .../ssh/ssh-orphan-relay-pty-sweep.test.ts | 162 ++++++++++++++ src/main/ssh/ssh-orphan-relay-pty-sweep.ts | 117 ++++++++++ .../ssh-relay-session-orphan-sweep.test.ts | 203 ++++++++++++++++++ src/main/ssh/ssh-relay-session.ts | 12 ++ .../pty-handler-ownership-attestation.test.ts | 107 +++++++++ src/relay/pty-handler.ts | 35 +++ src/relay/relay-runtime-services.ts | 5 + src/relay/ssh-pty-consumer-session-adapter.ts | 6 + src/shared/pty-consumer-session.ts | 9 + .../ssh-relay-pty-ownership-proof.test.ts | 159 ++++++++++++++ src/shared/ssh-relay-pty-ownership-proof.ts | 144 +++++++++++++ 12 files changed, 968 insertions(+) create mode 100644 src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts create mode 100644 src/main/ssh/ssh-orphan-relay-pty-sweep.ts create mode 100644 src/main/ssh/ssh-relay-session-orphan-sweep.test.ts create mode 100644 src/relay/pty-handler-ownership-attestation.test.ts create mode 100644 src/shared/ssh-relay-pty-ownership-proof.test.ts create mode 100644 src/shared/ssh-relay-pty-ownership-proof.ts diff --git a/src/main/providers/pty-process-info.ts b/src/main/providers/pty-process-info.ts index 848ff07c78a..942cab84f73 100644 --- a/src/main/providers/pty-process-info.ts +++ b/src/main/providers/pty-process-info.ts @@ -18,4 +18,13 @@ export type PtyProcessInfo = { /** Optional host-side process evidence attached to an inventory seed. */ foregroundProcessEvidence?: ForegroundProcessEvidence agentSessionOwners?: AgentSessionOwnerBinding[] + /** Age measured on the OWNING host's clock. Absent means the host did not measure it, which is + * not the same as "new" or "old" — a reader that needs an age must defer instead of assuming. */ + hostAgeMs?: number + /** True when the host spawned this PTY for an Orca pane, false for a bare host shell. Absent from + * a host that never published it; absence is neither value. */ + paneBound?: boolean + /** The client identity the OWNING host recorded as having asked it to create this PTY. Absent + * whenever the host could not attest one, and absence must never be read as "unowned". */ + ownerClientInstanceId?: string } diff --git a/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts b/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts new file mode 100644 index 00000000000..841a8ff21f3 --- /dev/null +++ b/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts @@ -0,0 +1,162 @@ +// #9819, the client half: what the sweep actually asks the store and the host, and what it does +// with the answers. The rule itself is covered in ssh-relay-pty-ownership-proof.test.ts. +import { describe, expect, it, vi } from 'vitest' +import type { Store } from '../persistence' +import type { IPtyProvider } from '../providers/types' +import type { PtyProcessInfo } from '../providers/pty-process-info' +import type { SshRemotePtyLease } from '../../shared/ssh-types' +import { sweepOrphanedRelayPtys } from './ssh-orphan-relay-pty-sweep' +import { RELAY_PTY_SWEEP_MIN_AGE_MS } from '../../shared/ssh-relay-pty-ownership-proof' + +const TARGET = 'target-1' +const OURS = 'client-instance-ours' + +function hostEntry(overrides: Partial = {}): PtyProcessInfo { + return { + id: `ssh:${TARGET}@@pty-1`, + incarnationId: 'inc-1', + cwd: '/home/user', + title: 'zsh', + ownerClientInstanceId: OURS, + hostAgeMs: RELAY_PTY_SWEEP_MIN_AGE_MS * 2, + paneBound: true, + ...overrides + } +} + +function createHarness( + processes: PtyProcessInfo[], + leases: SshRemotePtyLease[] = [] +): { provider: IPtyProvider; store: Store; shutdown: ReturnType } { + const shutdown = vi.fn().mockResolvedValue(undefined) + const provider = { + listProcesses: vi.fn().mockResolvedValue(processes), + shutdown + } as unknown as IPtyProvider + const store = { + getSshRemotePtyLeases: vi.fn().mockReturnValue(leases) + } as unknown as Store + return { provider, store, shutdown } +} + +function run( + harness: ReturnType, + overrides: Partial[0]> = {} +): Promise { + return sweepOrphanedRelayPtys({ + targetId: TARGET, + store: harness.store, + provider: harness.provider, + clientInstanceId: OURS, + isSessionOwner: true, + routedPtyIds: [], + shouldContinue: () => true, + ...overrides + }) +} + +function lease(ptyId: string, state: SshRemotePtyLease['state']): SshRemotePtyLease { + return { ptyId, state } as SshRemotePtyLease +} + +describe('sweepOrphanedRelayPtys', () => { + it('stops an attested orphan, fenced on the incarnation the same listing published', async () => { + const harness = createHarness([hostEntry()]) + + await run(harness) + + expect(harness.shutdown).toHaveBeenCalledWith(`ssh:${TARGET}@@pty-1`, { + immediate: true, + expectedIncarnationId: 'inc-1' + }) + }) + + it('leaves a PTY the caller just reattached alone', async () => { + const harness = createHarness([hostEntry()]) + + await run(harness, { routedPtyIds: ['pty-1'] }) + + expect(harness.shutdown).not.toHaveBeenCalled() + }) + + it.each([['attached'], ['detached']] as const)( + 'leaves a PTY holding a live %s lease alone', + async (state) => { + const harness = createHarness([hostEntry()], [lease('pty-1', state)]) + + await run(harness) + + expect(harness.shutdown).not.toHaveBeenCalled() + } + ) + + it('leaves a PTY with an undelivered stop to the replay pass', async () => { + // The kill-intent journal owns those: it re-fences and retries them, and a second stop issued + // from here would race that decision with weaker evidence. + const tombstoned = { + ...lease('pty-1', 'terminated'), + pendingKill: { requestedAt: 1, incarnationId: 'inc-1', attempts: 0 } + } as SshRemotePtyLease + const harness = createHarness([hostEntry()], [tombstoned]) + + await run(harness) + + expect(harness.shutdown).not.toHaveBeenCalled() + }) + + it('does sweep a PTY whose lease this client already tombstoned without an order', async () => { + const harness = createHarness([hostEntry()], [lease('pty-1', 'terminated')]) + + await run(harness) + + expect(harness.shutdown).toHaveBeenCalledTimes(1) + }) + + it('asks the host nothing when this connection is not the session owner', async () => { + const harness = createHarness([hostEntry()]) + + await run(harness, { isSessionOwner: false }) + + expect(harness.provider.listProcesses).not.toHaveBeenCalled() + expect(harness.shutdown).not.toHaveBeenCalled() + }) + + it('stops nothing against a host that publishes no attestation', async () => { + const legacy = hostEntry() + delete legacy.ownerClientInstanceId + delete legacy.hostAgeMs + delete legacy.paneBound + const harness = createHarness([legacy]) + + await run(harness) + + expect(harness.shutdown).not.toHaveBeenCalled() + }) + + it('swallows a failed listing rather than failing the connect it runs on', async () => { + const harness = createHarness([]) + vi.mocked(harness.provider.listProcesses).mockRejectedValue(new Error('relay went away')) + + await expect(run(harness)).resolves.toBeUndefined() + }) + + it('swallows a failed stop and leaves the order to the next connect', async () => { + const harness = createHarness([hostEntry()]) + harness.shutdown.mockRejectedValue(new Error('connection lost')) + + await expect(run(harness)).resolves.toBeUndefined() + }) + + it('abandons the pass when the attempt is superseded mid-flight', async () => { + const harness = createHarness([hostEntry()]) + let alive = true + vi.mocked(harness.provider.listProcesses).mockImplementation(async () => { + alive = false + return [hostEntry()] + }) + + await run(harness, { shouldContinue: () => alive }) + + expect(harness.shutdown).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/ssh/ssh-orphan-relay-pty-sweep.ts b/src/main/ssh/ssh-orphan-relay-pty-sweep.ts new file mode 100644 index 00000000000..964864ca2cf --- /dev/null +++ b/src/main/ssh/ssh-orphan-relay-pty-sweep.ts @@ -0,0 +1,117 @@ +import type { Store } from '../persistence' +import type { IPtyProvider } from '../providers/types' +import { toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id' +import { + planRelayPtySweep, + RELAY_PTY_SWEEP_MIN_AGE_MS, + type RelayPtyOwnershipEvidence +} from '../../shared/ssh-relay-pty-ownership-proof' + +export type SshOrphanRelayPtySweepArgs = { + targetId: string + store: Store + provider: IPtyProvider + /** This client's persisted consumer identity for the target. */ + clientInstanceId: string + /** True only when the relay granted this connection the negotiated `session-owner` role. */ + isSessionOwner: boolean + /** Relay PTY ids this connect just reattached, plus any the caller otherwise knows are live. */ + routedPtyIds: Iterable + shouldContinue: () => boolean + now?: () => number + minimumHostAgeMs?: number +} + +/** Every relay PTY id this client still has a route to. Read across all lease states except the + * tombstones, plus the undelivered stops, which belong to the replay pass and not to this one. */ +function routedIds(args: SshOrphanRelayPtySweepArgs): Set { + const routed = new Set(args.routedPtyIds) + for (const lease of args.store.getSshRemotePtyLeases(args.targetId)) { + if (lease.state !== 'terminated' && lease.state !== 'expired') { + routed.add(lease.ptyId) + } + if (lease.pendingKill) { + routed.add(lease.ptyId) + } + } + return routed +} + +function toEvidence( + targetId: string, + process: Awaited>[number] +): RelayPtyOwnershipEvidence { + return { + ptyId: toRelaySshPtyId(targetId, process.id), + ...(process.incarnationId ? { incarnationId: process.incarnationId } : {}), + ...(process.ownerClientInstanceId + ? { ownerClientInstanceId: process.ownerClientInstanceId } + : {}), + ...(typeof process.hostAgeMs === 'number' ? { hostAgeMs: process.hostAgeMs } : {}), + ...(typeof process.paneBound === 'boolean' ? { paneBound: process.paneBound } : {}), + ...(process.agentSessionOwners ? { agentSessionOwners: process.agentSessionOwners } : {}) + } +} + +/** Stops the relay PTYs this client can prove it created and has since lost every route to. + * + * Runs after reattach, so a PTY this connect reclaimed is already routed and can never be a + * candidate. Best-effort and never throws: it is opportunistic cleanup on the connect path, and a + * failed connection is a much worse outcome than a slot left leaked for another session. + * + * Costs one `pty.listProcesses` per connect. That is the price of reconciling at all — there is no + * cheaper question than asking the authoritative host what it is holding. */ +export async function sweepOrphanedRelayPtys(args: SshOrphanRelayPtySweepArgs): Promise { + if (!args.isSessionOwner || !args.clientInstanceId || !args.shouldContinue()) { + return + } + try { + const processes = await args.provider.listProcesses() + if (!args.shouldContinue()) { + return + } + const plan = planRelayPtySweep( + processes.map((process) => toEvidence(args.targetId, process)), + { + clientInstanceId: args.clientInstanceId, + isSessionOwner: args.isSessionOwner, + routedPtyIds: routedIds(args), + minimumHostAgeMs: args.minimumHostAgeMs ?? RELAY_PTY_SWEEP_MIN_AGE_MS + } + ) + if (plan.sweep.length === 0) { + return + } + await Promise.all( + plan.sweep.map(async (target) => { + if (!args.shouldContinue()) { + return + } + try { + // Fenced on the incarnation the same listing published, so a relay that renumbered its + // ids between the read and this call refuses the stop instead of hitting a stranger. + await args.provider.shutdown(toAppSshPtyId(args.targetId, target.ptyId), { + immediate: true, + expectedIncarnationId: target.incarnationId + }) + console.log( + `[ssh-orphan-sweep] stopped orphaned relay PTY ${args.targetId}/${target.ptyId}` + ) + } catch (err) { + // Unverifiable, not failed: the next connect re-reads the inventory and decides again. + console.warn( + `[ssh-orphan-sweep] stop for ${args.targetId}/${target.ptyId} is unverifiable: ${ + err instanceof Error ? err.message : String(err) + }` + ) + } + }) + ) + } catch (err) { + console.warn( + `[ssh-orphan-sweep] pass on ${args.targetId} stopped early: ${ + err instanceof Error ? err.message : String(err) + }` + ) + } +} diff --git a/src/main/ssh/ssh-relay-session-orphan-sweep.test.ts b/src/main/ssh/ssh-relay-session-orphan-sweep.test.ts new file mode 100644 index 00000000000..960cbd58a4f --- /dev/null +++ b/src/main/ssh/ssh-relay-session-orphan-sweep.test.ts @@ -0,0 +1,203 @@ +// #9819 end to end on the client: the sweep runs only after reattach, only under a negotiated +// session-owner grant, and only against PTYs this relay itself attributes to this client. +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { SshRelaySession } from './ssh-relay-session' +import { createMockDeps, mockDeploySuccess } from './ssh-relay-session-test-fixtures' + +const { muxRequestMock, openConsumerSessionMock } = vi.hoisted(() => ({ + muxRequestMock: vi.fn(), + openConsumerSessionMock: vi.fn(async (_mux: unknown, options: { clientInstanceId: string }) => ({ + state: { + mode: 'negotiated' as const, + clientInstanceId: options.clientInstanceId, + clientGeneration: 1, + ownerGeneration: 1, + ownerLease: 'test-owner-lease' + }, + resumed: false + })) +})) + +vi.mock('./ssh-relay-deploy', () => ({ deployAndLaunchRelay: vi.fn() })) +vi.mock('./ssh-pty-consumer-session', () => ({ + openSshPtyConsumerSession: openConsumerSessionMock +})) +vi.mock('../ipc/ssh-pty-output-intake-registry', () => ({ + acceptSshPtyOutputData: vi.fn().mockResolvedValue(undefined), + acceptSshPtyOutputExit: vi.fn().mockResolvedValue(undefined), + allocateSshPtyProviderGeneration: vi.fn(() => 17), + beginSshPtyOutputGenerationMigration: vi.fn(() => ({ + byPty: new Map(), + completion: Promise.resolve() + })), + closeSshPtyOutputGeneration: vi.fn(), + getSshPtyAcceptedSourceCheckpoints: vi.fn(() => []), + applySshPtySourceCancellationProof: vi.fn(() => true), + applySshPtySourceRecoveryCancellationProof: vi.fn(() => true), + installSshPtySourceAckPublisher: vi.fn(() => () => {}), + installSshPtySourceCancellationPublisher: vi.fn(() => () => {}) +})) +vi.mock('./ssh-relay-deploy-helpers', () => ({ execCommand: vi.fn().mockResolvedValue('') })) +vi.mock('./ssh-remote-orca-cli', () => ({ + runRemoteOrcaCli: vi.fn().mockResolvedValue({ exitCode: 0, stdout: '', stderr: '' }) +})) +vi.mock('./ssh-channel-multiplexer', () => ({ + SshChannelMultiplexer: class MockSshChannelMultiplexer { + notify = vi.fn() + notifyWithSettlement = vi.fn() + request = muxRequestMock + onNotification = vi.fn().mockReturnValue(() => {}) + onNotificationByMethod = vi.fn().mockReturnValue(() => {}) + onRequest = vi.fn().mockReturnValue(() => {}) + onDispose = vi.fn().mockReturnValue(() => {}) + dispose = vi.fn() + isDisposed = vi.fn().mockReturnValue(false) + } +})) +vi.mock('../agent-hooks/remote-managed-hook-installers', () => ({ + installRemoteManagedAgentHooks: vi.fn() +})) +vi.mock('../providers/ssh-pty-provider', () => ({ + SshPtyProvider: class MockSshPtyProvider { + onData = vi.fn().mockReturnValue(() => {}) + onReplay = vi.fn().mockReturnValue(() => {}) + onExit = vi.fn().mockReturnValue(() => {}) + attach = vi.fn().mockResolvedValue(undefined) + attachForReconnect = vi.fn().mockResolvedValue({}) + dispose = vi.fn() + } +})) +vi.mock('../providers/ssh-filesystem-provider', () => ({ + SshFilesystemProvider: class MockSshFilesystemProvider { + dispose = vi.fn() + } +})) +vi.mock('../providers/ssh-git-provider', () => ({ + SshGitProvider: class MockSshGitProvider {} +})) +vi.mock('../ipc/pty', () => ({ + registerSshPtyProvider: vi.fn(), + unregisterSshPtyProvider: vi.fn(), + getSshPtyProvider: vi.fn(), + getPtyIdsForConnection: vi.fn().mockReturnValue([]), + clearPtyOwnershipForConnection: vi.fn(), + clearProviderPtyState: vi.fn(), + deletePtyOwnership: vi.fn(), + setPtyOwnership: vi.fn(), + restorePtyIncarnation: vi.fn(), + isCurrentPtyExit: vi.fn(() => true), + answerStartupTerminalColorQueriesForPty: vi.fn((_id: string, data: string) => data) +})) +vi.mock('../providers/ssh-filesystem-dispatch', () => ({ + registerSshFilesystemProvider: vi.fn(), + unregisterSshFilesystemProvider: vi.fn(), + getSshFilesystemProvider: vi.fn().mockReturnValue({ dispose: vi.fn() }) +})) +vi.mock('../providers/ssh-git-dispatch', () => ({ + registerSshGitProvider: vi.fn(), + unregisterSshGitProvider: vi.fn() +})) + +const { getSshPtyProvider, getPtyIdsForConnection } = await import('../ipc/pty') + +const TARGET = 'target-1' +const OUR_CLIENT = 'client-instance-1' + +function hostEntry(overrides: Record = {}): Record { + return { + id: `ssh:${TARGET}@@pty-orphan`, + incarnationId: 'inc-orphan', + cwd: '/home/user', + title: 'zsh', + // The relay stamps this from the live consumer grant, so it names THIS client. + ownerClientInstanceId: OUR_CLIENT, + hostAgeMs: 120_000, + paneBound: true, + ...overrides + } +} + +describe('SshRelaySession orphaned relay PTY sweep', () => { + beforeEach(() => { + vi.clearAllMocks() + muxRequestMock.mockReset() + muxRequestMock.mockResolvedValue([]) + mockDeploySuccess() + vi.mocked(getPtyIdsForConnection).mockReturnValue([]) + }) + + async function establish( + processes: Record[], + leases: { ptyId: string; state: string }[] = [] + ): Promise<{ shutdown: ReturnType }> { + const deps = createMockDeps() + // Why the recovery row: it pins this session's clientInstanceId, and the comparison is + // meaningless unless the id it uses is the persisted one. + vi.mocked(deps.mockStore.getSshPtyConsumerRecovery).mockReturnValue({ + targetId: TARGET, + clientInstanceId: OUR_CLIENT, + serverBuildId: 'build-1', + clientGeneration: 1, + ownerGeneration: 1, + ownerLease: 'test-owner-lease' + } as ReturnType) + vi.mocked(deps.mockStore.getSshRemotePtyLeases).mockReturnValue( + leases.map((lease) => ({ targetId: TARGET, ...lease })) as ReturnType< + typeof deps.mockStore.getSshRemotePtyLeases + > + ) + const shutdown = vi.fn().mockResolvedValue(undefined) + vi.mocked(getSshPtyProvider).mockReturnValue({ + attachForReconnect: vi.fn().mockResolvedValue({}), + listProcesses: vi.fn().mockResolvedValue(processes), + shutdown, + dispose: vi.fn() + } as unknown as ReturnType) + + const session = new SshRelaySession( + TARGET, + deps.getMainWindow, + deps.mockStore, + deps.mockPortForward + ) + await session.establish(deps.mockConn) + return { shutdown } + } + + it('stops an attested orphan the client has no lease for', async () => { + const { shutdown } = await establish([hostEntry()]) + + expect(shutdown).toHaveBeenCalledWith(`ssh:${TARGET}@@pty-orphan`, { + immediate: true, + expectedIncarnationId: 'inc-orphan' + }) + }) + + it('never stops a PTY that still holds a live lease', async () => { + const { shutdown } = await establish( + [hostEntry({ id: `ssh:${TARGET}@@pty-live`, incarnationId: 'inc-live' })], + [{ ptyId: 'pty-live', state: 'detached' }] + ) + + expect(shutdown).not.toHaveBeenCalled() + }) + + it('never stops a PTY this relay attributes to a different client', async () => { + const { shutdown } = await establish([ + hostEntry({ ownerClientInstanceId: 'someone-elses-laptop' }) + ]) + + expect(shutdown).not.toHaveBeenCalled() + }) + + it('never stops anything a relay predating the attestation lists', async () => { + const legacy = hostEntry() + delete legacy.ownerClientInstanceId + delete legacy.hostAgeMs + delete legacy.paneBound + + const { shutdown } = await establish([legacy]) + + expect(shutdown).not.toHaveBeenCalled() + }) +}) diff --git a/src/main/ssh/ssh-relay-session.ts b/src/main/ssh/ssh-relay-session.ts index 68254fbb108..04fe6020749 100644 --- a/src/main/ssh/ssh-relay-session.ts +++ b/src/main/ssh/ssh-relay-session.ts @@ -10,6 +10,7 @@ import { isRelayEndpointHeldError } from './ssh-relay-endpoint-incumbent' import { forgetRelayNodePtyRepairs, recoverRelayNodePtyForSpawn } from './ssh-relay-node-pty-repair' import type { TerminalUnavailableCause } from '../../shared/terminal-unavailable-cause' import { replayPendingSshPtyKills } from './ssh-pending-pty-kill-replay' +import { sweepOrphanedRelayPtys } from './ssh-orphan-relay-pty-sweep' import { SshChannelMultiplexer } from './ssh-channel-multiplexer' import { SshPtyProvider } from '../providers/ssh-pty-provider' import type { SshPtyAttachResult } from '../providers/ssh-pty-session-reattach' @@ -2398,6 +2399,17 @@ export class SshRelaySession { Array.from(attachedLeaseIds) ) } + // Why last: reclaiming comes first, so every PTY this connect could route to is routed before + // anything asks which ones are unreachable (#9819). + await sweepOrphanedRelayPtys({ + targetId: this.targetId, + store: this.store, + provider: ptyProvider, + clientInstanceId: this.ptyConsumerClientInstanceId, + isSessionOwner: this.activePtyConsumerOwner() !== null, + routedPtyIds: ptyIds, + shouldContinue + }) } private async reattachKnownPty(args: { diff --git a/src/relay/pty-handler-ownership-attestation.test.ts b/src/relay/pty-handler-ownership-attestation.test.ts new file mode 100644 index 00000000000..7ab726fd533 --- /dev/null +++ b/src/relay/pty-handler-ownership-attestation.test.ts @@ -0,0 +1,107 @@ +// The host half of #9819: a client may only reap a relay PTY it can prove it created, so the relay +// has to say who created each one. The attestation is read from the live consumer grant, never from +// a spawn parameter — otherwise it would just echo the caller's claim back at it. +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest' + +const { mockPtySpawn, mockPtyInstance, mockCreateShellPromptReadinessProbe } = vi.hoisted(() => ({ + mockPtySpawn: vi.fn(), + mockCreateShellPromptReadinessProbe: vi.fn(), + mockPtyInstance: { + pid: process.pid, + onData: vi.fn(), + onExit: vi.fn(), + write: vi.fn(), + resize: vi.fn(), + kill: vi.fn(), + clear: vi.fn(), + pause: vi.fn(), + resume: vi.fn() + } +})) + +vi.mock('node-pty', () => ({ spawn: mockPtySpawn })) +vi.mock('../main/pty/posix-pty-process-groups', () => ({ + forceKillPosixPtyProcessGroups: vi.fn((_pid: number, fallback: () => void) => fallback()) +})) +vi.mock('../main/shell-prompt-readiness-probe', () => ({ + createShellPromptReadinessProbe: mockCreateShellPromptReadinessProbe +})) + +import type { PtyHandler } from './pty-handler' +import { + beginPtyHandlerTest, + endPtyHandlerTest, + type MockDispatcher +} from './pty-handler-test-harness' + +const PANE_KEY = 'tab-agent:22222222-2222-4222-8222-222222222222' + +type Summary = { + id: string + paneBound?: boolean + hostAgeMs?: number + ownerClientInstanceId?: string +} + +describe('PtyHandler publishes host-attested PTY ownership', () => { + let dispatcher: MockDispatcher + let handler: PtyHandler + let originalPlatform: PropertyDescriptor | undefined + + async function spawnFrom( + clientId: number, + params: Record = {} + ): Promise<{ id: string }> { + mockPtySpawn.mockReturnValue({ ...mockPtyInstance, onData: vi.fn(), onExit: vi.fn() }) + return (await dispatcher.callRequest('pty.spawn', params, { + clientId, + isStale: () => false + } as never)) as { id: string } + } + + async function listProcesses(): Promise { + return (await dispatcher.callRequest('pty.listProcesses', {})) as Summary[] + } + + beforeEach(() => { + ;({ dispatcher, handler, originalPlatform } = beginPtyHandlerTest({ + mockPtySpawn, + mockPtyInstance, + mockCreateShellPromptReadinessProbe + })) + handler.setConsumerIdentityResolver((clientId) => (clientId === 7 ? 'client-A' : null)) + }) + + afterEach(async () => { + await endPtyHandlerTest(handler, originalPlatform) + }) + + it('attributes a pane spawn to the identity the consumer grant names', async () => { + const { id } = await spawnFrom(7, { env: { ORCA_PANE_KEY: PANE_KEY } }) + vi.advanceTimersByTime(45_000) + + const entry = (await listProcesses()).find((process) => process.id === id) + + expect(entry?.ownerClientInstanceId).toBe('client-A') + expect(entry?.paneBound).toBe(true) + expect(entry?.hostAgeMs).toBeGreaterThanOrEqual(45_000) + }) + + it('omits the attestation entirely when the connection holds no active grant', async () => { + const { id } = await spawnFrom(9, { env: { ORCA_PANE_KEY: PANE_KEY } }) + + const entry = (await listProcesses()).find((process) => process.id === id) + + // Absent, not empty-string or null: a reader must be able to tell "unattested" from any value. + expect(entry).not.toHaveProperty('ownerClientInstanceId') + }) + + it('reports a bare shell as not pane-bound', async () => { + const { id } = await spawnFrom(7, {}) + + const entry = (await listProcesses()).find((process) => process.id === id) + + expect(entry?.paneBound).toBe(false) + expect(entry?.ownerClientInstanceId).toBe('client-A') + }) +}) diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index f8bd2351cf2..d8fe370b50c 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -190,6 +190,13 @@ type ManagedPty = { startupIngressIntent?: ReturnType ownerBackend: PtyOwnerBackend agentSessionOwners?: AgentSessionOwnerBinding[] + /** Host clock, host-relative only: published as an age so no client has to trust our wall clock. */ + createdAt: number + /** The authenticated consumer identity that asked this host to create this PTY, read from the + * live grant rather than from a spawn parameter. Absent whenever the host could not attest one + * (no consumer session, or a revive replaying state some other client serialized), and absence + * must never be read as "nobody owns it". */ + ownerClientInstanceId?: string } type RelayAgentSessionCreateResult = { @@ -369,6 +376,14 @@ type PtyProcessSummary = { terminalHandle?: string foregroundProcessEvidence?: ForegroundProcessEvidence agentSessionOwners?: AgentSessionOwnerBinding[] + /** Age on the HOST's clock. Published instead of a creation timestamp so a client with a skewed + * clock cannot compute a negative or enormous age and act on it. */ + hostAgeMs?: number + /** True when this PTY was spawned for an Orca pane (`ORCA_PANE_KEY`). False means a bare relay + * shell. Absent from a host that predates the field — which is neither. */ + paneBound?: boolean + /** See {@link ManagedPty.ownerClientInstanceId}. Omitted when this host cannot attest one. */ + ownerClientInstanceId?: string } type SerializedPtyEntry = { @@ -456,6 +471,7 @@ export class PtyHandler { private consumerPausedOutputPtys = new Set() private removeLegacyCapacityListener: (() => void) | null = null private sourcePublication: RelayPtySourcePublication | null = null + private consumerIdentityResolver: ((clientId: number) => string | null) | null = null private lastInputAtByPty = new Map() private interactiveOutputCharsByPty = new Map() private pendingSpawnCount = 0 @@ -510,6 +526,12 @@ export class PtyHandler { this.sourcePublication = publication } + /** Supplies the authenticated client identity behind a transport connection, so a spawn can be + * attributed to the consumer session that requested it. */ + setConsumerIdentityResolver(resolve: ((clientId: number) => string | null) | null): void { + this.consumerIdentityResolver = resolve + } + handleSourceCreditAvailable(id: string): void { this.sourcePublication?.onCreditAvailable(id) } @@ -1871,11 +1893,15 @@ export class PtyHandler { params.startupIngressVersion === PTY_STARTUP_INGRESS_VERSION ? parsePtyStartupIngressIntent(params.startupIngress) : undefined + const ownerClientInstanceId = + context === undefined ? null : (this.consumerIdentityResolver?.(context.clientId) ?? null) const managed: ManagedPty = { id, incarnationId: randomUUID(), pty: term, initialCwd: cwd, + createdAt: Date.now(), + ...(ownerClientInstanceId ? { ownerClientInstanceId } : {}), buffered: new RecentPtyOutputBuffer({ preserveChunkBoundaries: false, limit: REPLAY_BUFFER_MAX @@ -2451,6 +2477,11 @@ export class PtyHandler { incarnationId: managed.incarnationId, cwd: managed.initialCwd, title, + hostAgeMs: Math.max(0, Date.now() - managed.createdAt), + paneBound: Boolean(managed.paneKey ?? managed.attachIdentity?.paneKey), + ...(managed.ownerClientInstanceId + ? { ownerClientInstanceId: managed.ownerClientInstanceId } + : {}), ...(managed.worktreeId ? { worktreeId: managed.worktreeId } : {}), ...(managed.terminalHandle ? { terminalHandle: managed.terminalHandle } : {}), ...(foregroundProcessEvidence ? { foregroundProcessEvidence } : {}), @@ -2624,6 +2655,10 @@ export class PtyHandler { incarnationId: randomUUID(), pty: term, initialCwd: entry.cwd, + createdAt: Date.now(), + // Deliberately no ownerClientInstanceId: revive replays state a client serialized, which is + // not this host observing who asked for the shell. Unattested means never swept. + buffered: new RecentPtyOutputBuffer({ preserveChunkBoundaries: false, limit: REPLAY_BUFFER_MAX diff --git a/src/relay/relay-runtime-services.ts b/src/relay/relay-runtime-services.ts index 36276ed9b70..8fa5dc9d1d0 100644 --- a/src/relay/relay-runtime-services.ts +++ b/src/relay/relay-runtime-services.ts @@ -44,6 +44,11 @@ export class RelayRuntimeServices { (id, paused) => this.ptyHandler.setConsumerDeliveryPaused(id, paused), (id) => this.ptyHandler.handleSourceCreditAvailable(id) ) + // Why wired after construction: the handler is built first, but PTY ownership has to be + // attested from the consumer grant the adapter holds. + this.ptyHandler.setConsumerIdentityResolver((clientId) => + this.ptyConsumerSessionAdapter.clientInstanceIdFor(clientId) + ) this.ptySourcePublication = new RelayPtySourcePublication( dispatcher, this.ptyConsumerSessionAdapter, diff --git a/src/relay/ssh-pty-consumer-session-adapter.ts b/src/relay/ssh-pty-consumer-session-adapter.ts index 83e61f17fa3..906edd47ff8 100644 --- a/src/relay/ssh-pty-consumer-session-adapter.ts +++ b/src/relay/ssh-pty-consumer-session-adapter.ts @@ -112,6 +112,12 @@ export class SshPtyConsumerSessionAdapter { }) } + /** The authenticated client identity behind a transport connection, or null when it holds no + * active grant. Used to stamp host-attested ownership on a PTY at spawn. */ + clientInstanceIdFor(clientId: number): string | null { + return this.session.activeClientInstanceId(String(clientId)) + } + openDelivery( clientId: number, id: string, diff --git a/src/shared/pty-consumer-session.ts b/src/shared/pty-consumer-session.ts index 6b040be47cf..7c5b29aacfd 100644 --- a/src/shared/pty-consumer-session.ts +++ b/src/shared/pty-consumer-session.ts @@ -153,6 +153,15 @@ export class PtyConsumerSession { return client?.state === 'active' ? client.grant : null } + /** The authenticated client identity behind an active connection, or null. + * + * Why the host reads it here instead of taking a spawn parameter: this is what makes a later + * "this PTY belongs to you" attestation evidence rather than an echo of what a caller claimed. */ + activeClientInstanceId(connectionId: string): string | null { + const client = this.clients.get(connectionId) + return client?.state === 'active' ? client.clientInstanceId : null + } + private admissionFor( client: ClientRecord, displacedOwner?: Readonly diff --git a/src/shared/ssh-relay-pty-ownership-proof.test.ts b/src/shared/ssh-relay-pty-ownership-proof.test.ts new file mode 100644 index 00000000000..4dc8cb1ce14 --- /dev/null +++ b/src/shared/ssh-relay-pty-ownership-proof.test.ts @@ -0,0 +1,159 @@ +// #9819. Every case here is the same question asked from a different angle: can this client PROVE +// the host is holding a process nobody can reach? A "no" has to mean "leave it running". +import { describe, expect, it } from 'vitest' +import { + planRelayPtySweep, + RELAY_PTY_SWEEP_MAX_PER_PASS, + RELAY_PTY_SWEEP_MIN_AGE_MS, + type RelayPtyOwnershipEvidence, + type RelayPtySweepContext +} from './ssh-relay-pty-ownership-proof' + +const OURS = 'client-instance-ours' + +function orphan(overrides: Partial = {}): RelayPtyOwnershipEvidence { + return { + ptyId: 'pty-1', + incarnationId: 'inc-1', + ownerClientInstanceId: OURS, + hostAgeMs: RELAY_PTY_SWEEP_MIN_AGE_MS * 2, + paneBound: true, + ...overrides + } +} + +function context(overrides: Partial = {}): RelayPtySweepContext { + return { + clientInstanceId: OURS, + isSessionOwner: true, + routedPtyIds: new Set(), + minimumHostAgeMs: RELAY_PTY_SWEEP_MIN_AGE_MS, + ...overrides + } +} + +function reasonFor(plan: ReturnType, ptyId: string): string | undefined { + return plan.skipped.find((entry) => entry.ptyId === ptyId)?.reason +} + +describe('planRelayPtySweep', () => { + it('sweeps a pane PTY this host attests we created and we have lost every route to', () => { + const plan = planRelayPtySweep([orphan()], context()) + + expect(plan.sweep).toEqual([{ ptyId: 'pty-1', incarnationId: 'inc-1' }]) + }) + + it('never sweeps a PTY this client still routes to', () => { + const plan = planRelayPtySweep([orphan()], context({ routedPtyIds: new Set(['pty-1']) })) + + expect(plan.sweep).toEqual([]) + expect(reasonFor(plan, 'pty-1')).toBe('this client still has a route to it') + }) + + it('never sweeps a PTY the host attributes to another client instance', () => { + // The case that makes local absence useless as evidence: a second machine on the same build + // connects to the same relay, and its live agents are missing from our store exactly like an + // orphan is. + const plan = planRelayPtySweep( + [orphan({ ownerClientInstanceId: 'client-instance-theirs' })], + context() + ) + + expect(plan.sweep).toEqual([]) + expect(reasonFor(plan, 'pty-1')).toBe('host attests another client created it') + }) + + it('never sweeps a PTY younger than the floor', () => { + const plan = planRelayPtySweep( + [orphan({ hostAgeMs: RELAY_PTY_SWEEP_MIN_AGE_MS - 1 })], + context() + ) + + expect(plan.sweep).toEqual([]) + expect(reasonFor(plan, 'pty-1')).toBe('younger than the sweep floor') + }) + + it('never sweeps a bare host shell', () => { + const plan = planRelayPtySweep([orphan({ paneBound: false })], context()) + + expect(plan.sweep).toEqual([]) + expect(reasonFor(plan, 'pty-1')).toBe('not a pane-bound PTY') + }) + + it('never sweeps a PTY whose agent session the host still advertises as adoptable', () => { + const plan = planRelayPtySweep( + [orphan({ agentSessionOwners: [{ ptyId: 'pty-1' }] })], + context() + ) + + expect(plan.sweep).toEqual([]) + expect(reasonFor(plan, 'pty-1')).toBe('host still advertises an adoptable agent session') + }) + + it('never sweeps without the negotiated session-owner grant', () => { + const plan = planRelayPtySweep([orphan()], context({ isSessionOwner: false })) + + expect(plan.sweep).toEqual([]) + expect(reasonFor(plan, 'pty-1')).toBe('this client does not hold the relay session-owner grant') + }) + + it('refuses a pass larger than the per-pass ceiling instead of truncating it', () => { + const entries = Array.from({ length: RELAY_PTY_SWEEP_MAX_PER_PASS + 1 }, (_, index) => + orphan({ ptyId: `pty-${index}`, incarnationId: `inc-${index}` }) + ) + + const plan = planRelayPtySweep(entries, context()) + + expect(plan.sweep).toEqual([]) + expect(plan.skipped).toHaveLength(entries.length) + }) + + describe('against a host that predates the attestation', () => { + // Mixed versions: every new field is optional, and an older host publishes none of them. The + // sweep has to read each absence as "unknown", never as a permissive default. + it('skips an entry with no owner attestation', () => { + const { ownerClientInstanceId: _absent, ...legacy } = orphan() + + const plan = planRelayPtySweep([legacy], context()) + + expect(plan.sweep).toEqual([]) + expect(reasonFor(plan, 'pty-1')).toBe('host attested no owning client') + }) + + it('skips an entry with no published age', () => { + const { hostAgeMs: _absent, ...legacy } = orphan() + + const plan = planRelayPtySweep([legacy], context()) + + expect(plan.sweep).toEqual([]) + expect(reasonFor(plan, 'pty-1')).toBe('host published no age') + }) + + it('skips an entry with no paneBound field', () => { + const { paneBound: _absent, ...legacy } = orphan() + + const plan = planRelayPtySweep([legacy], context()) + + expect(plan.sweep).toEqual([]) + expect(reasonFor(plan, 'pty-1')).toBe('not a pane-bound PTY') + }) + + it('skips an entry with no incarnation, so no stop is ever unfenced', () => { + const { incarnationId: _absent, ...legacy } = orphan() + + const plan = planRelayPtySweep([legacy], context()) + + expect(plan.sweep).toEqual([]) + expect(reasonFor(plan, 'pty-1')).toBe('host published no PTY incarnation') + }) + + it('sweeps nothing at all when the whole listing predates the fields', () => { + const legacy = [ + { ptyId: 'pty-1', incarnationId: 'inc-1' }, + { ptyId: 'pty-2', incarnationId: 'inc-2' } + ] + + expect(planRelayPtySweep(legacy, context()).sweep).toEqual([]) + }) + }) +}) diff --git a/src/shared/ssh-relay-pty-ownership-proof.ts b/src/shared/ssh-relay-pty-ownership-proof.ts new file mode 100644 index 00000000000..1f9505db03e --- /dev/null +++ b/src/shared/ssh-relay-pty-ownership-proof.ts @@ -0,0 +1,144 @@ +/** Which relay PTYs a client may prove it orphaned, and therefore may stop (#9819). + * + * A relay PTY is a child of the detached relay daemon. Stopping one destroys a running process — + * often a running agent — on the user's remote machine, and the relay's 50-slot cap is a far + * cheaper failure than that. So every rule below is written to answer "can this client PROVE + * nobody owns this?" and to answer "no" whenever it cannot. + * + * The rule #9819 proposed — "pane-bound and the app no longer owns or leases it" — is not that + * proof. Absence from a client-side set is `unverifiable` by construction + * (`docs/reference/ssh-execution-boundary.md`): a second machine running the same Orca build + * connects to the SAME relay and displaces the session owner, and its PTYs are missing from THIS + * client's store for exactly the same reason a genuine orphan is. Sweeping on local absence alone + * would let one laptop reap another laptop's live agents. + * + * What replaces it: the host itself records which authenticated consumer identity asked it to + * create each PTY, and publishes that back. A PTY is sweepable only when the OWNING HOST names + * this client as its creator and this client's own durable state has no route to it. Both halves + * are required; either alone is a guess. + */ + +/** One `pty.listProcesses` entry, as far as this decision is concerned. Every field a host may + * omit is optional here, because a host predating it publishes nothing rather than a default. */ +export type RelayPtyOwnershipEvidence = { + /** Relay-scoped PTY id. */ + ptyId: string + incarnationId?: string + ownerClientInstanceId?: string + hostAgeMs?: number + paneBound?: boolean + /** Non-empty when the host still advertises an adoptable agent session on this PTY. */ + agentSessionOwners?: readonly unknown[] +} + +export type RelayPtySweepContext = { + /** This client's persisted consumer identity for the target. */ + clientInstanceId: string + /** Whether the relay granted THIS connection the `session-owner` role. A subscriber, or a client + * that fell back to the unnegotiated legacy path, never sweeps. */ + isSessionOwner: boolean + /** Every relay PTY id this client still has any route to: a live provider PTY, a lease it has + * not tombstoned, an id it just reattached, or a stop it has recorded and not yet delivered. */ + routedPtyIds: ReadonlySet + /** Host-measured age a PTY must exceed. Guards a spawn that is in flight from another window of + * this same client and has not written its lease yet. */ + minimumHostAgeMs: number +} + +export type RelayPtySweepTarget = { ptyId: string; incarnationId: string } + +export type RelayPtySweepSkip = { ptyId: string; reason: string } + +export type RelayPtySweepPlan = { + sweep: RelayPtySweepTarget[] + skipped: RelayPtySweepSkip[] +} + +/** Deliberately longer than any single connect round trip. A PTY younger than this is never worth + * the risk: the leak it represents costs one slot for 30 more seconds, and reaping a shell that a + * concurrent spawn is still recording costs the user a terminal. */ +export const RELAY_PTY_SWEEP_MIN_AGE_MS = 30_000 + +/** Bounds one pass. A relay is capped at 50 PTYs, so a pass that wants to stop more than this is + * not reclaiming a leak — it is a disagreement about ownership, and stopping is the wrong move. */ +export const RELAY_PTY_SWEEP_MAX_PER_PASS = 8 + +function skipReason( + entry: RelayPtyOwnershipEvidence, + context: RelayPtySweepContext +): string | null { + if (typeof entry.incarnationId !== 'string' || entry.incarnationId.length === 0) { + // Without the host's own incarnation there is no fence, and an unfenced stop aimed at a relay + // id can hit whatever holds that id by the time it lands. + return 'host published no PTY incarnation' + } + if (typeof entry.ownerClientInstanceId !== 'string' || entry.ownerClientInstanceId.length === 0) { + return 'host attested no owning client' + } + if (entry.ownerClientInstanceId !== context.clientInstanceId) { + return 'host attests another client created it' + } + if (entry.paneBound !== true) { + // Covers both a bare host shell (a remote CLI terminal nobody's pane owns) and a host that + // never published the field. Neither is a pane this client lost. + return 'not a pane-bound PTY' + } + if (entry.agentSessionOwners !== undefined && entry.agentSessionOwners.length > 0) { + // The host still advertises this session as adoptable, so a later spawn can reclaim the running + // agent. Reaping it converts a recoverable session into a destroyed one. + return 'host still advertises an adoptable agent session' + } + if (typeof entry.hostAgeMs !== 'number' || !Number.isFinite(entry.hostAgeMs)) { + return 'host published no age' + } + if (entry.hostAgeMs < context.minimumHostAgeMs) { + return 'younger than the sweep floor' + } + if (context.routedPtyIds.has(entry.ptyId)) { + return 'this client still has a route to it' + } + return null +} + +/** Plans one sweep pass. Pure: every input is evidence the caller already gathered, so the rule can + * be tested without a relay, and the irreversible call sits with the caller. */ +export function planRelayPtySweep( + entries: readonly RelayPtyOwnershipEvidence[], + context: RelayPtySweepContext +): RelayPtySweepPlan { + if (!context.isSessionOwner || !context.clientInstanceId) { + return { + sweep: [], + skipped: entries.map((entry) => ({ + ptyId: entry.ptyId, + reason: 'this client does not hold the relay session-owner grant' + })) + } + } + const sweep: RelayPtySweepTarget[] = [] + const skipped: RelayPtySweepSkip[] = [] + for (const entry of entries) { + const reason = skipReason(entry, context) + if (reason !== null) { + skipped.push({ ptyId: entry.ptyId, reason }) + } else { + sweep.push({ ptyId: entry.ptyId, incarnationId: entry.incarnationId as string }) + } + } + if (sweep.length > RELAY_PTY_SWEEP_MAX_PER_PASS) { + // Why refuse rather than truncate: at this size the disagreement is about ownership, not about + // a handful of leaked slots, and a truncated pass would work through the same list one connect + // at a time and destroy it all anyway. + return { + sweep: [], + skipped: [ + ...skipped, + ...sweep.map((target) => ({ + ptyId: target.ptyId, + reason: `refusing a ${sweep.length}-PTY sweep; over the ${RELAY_PTY_SWEEP_MAX_PER_PASS} per-pass ceiling` + })) + ] + } + } + return { sweep, skipped } +}