From 6d33bbadf865c0fc384eb6f664e5ac2f03fc3f68 Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Wed, 2 Sep 2026 02:13:56 -0700 Subject: [PATCH] fix(ssh): measure the pane's whole tty before the sweep may stop it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways the orphan sweep could still SIGKILL live user work. `shellIsForeground` was `tpgid === pgid`, a foreground-only predicate. Measured on a real `bash -i` on a real pty: a shell holding `sleep 300 &` and a shell holding a Ctrl-Z'd job both read `pgid == tpgid`, `Ss+` — byte-identical to an idle prompt. Only the job's own row differs. So `pnpm build &` and a suspended editor were each attested idle, and the stop that follows SIGKILLs every process group on the tty. It is now `shellOwnsEveryTtyProcessGroup`, measured over the same set of process groups the kill would reach, and false when anything on the tty is stopped. No new probe: `tpgid` already identifies the terminal, because a process group has one session and a session at most one controlling terminal. `capturedAgeMs` was hardcoded to 0 by a producer that awaited the snapshot once before a loop that may await per entry, and it had zero readers anywhere. It is now stamped from when the capture was taken — deliberately as an upper bound, since the table is TTL-shared — and the sweep refuses evidence older than its own pass budget, counting its own elapsed time since the listing arrived. Stale degrades to "do not sweep", never to "sweep". The display consumer of the same measurement keeps no age budget, now as a stated decision rather than an accident. `pty.shutdown` took no request context while `pty.spawn` and `pty.attach` both did, so the ownership rule was enforced client-side only on the one irreversible call. It gains an optional `expectedOwnerClientInstanceId`, symmetric with the incarnation fence already there; the host refuses unless the connection still authenticates as that identity AND it recorded that identity at spawn. Rule 1: no reader requires it, an ordinary teardown omits it, and an old host ignoring it behaves exactly as today. The load-bearing test joins the real publisher to the real client reader over `ps` captured verbatim from a container. Reverting the predicate fails the backgrounded and suspended rows and nothing else. --- .../agent-foreground-process-batch.test.ts | 6 +- .../agent-foreground-process-batch.ts | 77 +++++- src/main/providers/pty-provider-contract.ts | 6 + src/main/providers/ssh-pty-provider.ts | 8 +- .../ssh/ssh-orphan-relay-pty-sweep.test.ts | 47 +++- src/main/ssh/ssh-orphan-relay-pty-sweep.ts | 30 ++- ...h-orphan-sweep-pane-state-verdicts.test.ts | 238 ++++++++++++++++++ .../ssh-relay-session-orphan-sweep.test.ts | 4 +- ...handler-inventory-process-evidence.test.ts | 44 ++-- .../pty-handler-ownership-attestation.test.ts | 144 +++++++++++ src/relay/pty-handler.ts | 52 +++- src/shared/foreground-process-evidence.ts | 32 ++- src/shared/process-table-snapshot.ts | 5 + .../ssh-relay-pty-ownership-proof.test.ts | 9 +- src/shared/ssh-relay-pty-ownership-proof.ts | 51 +++- 15 files changed, 685 insertions(+), 68 deletions(-) create mode 100644 src/main/ssh/ssh-orphan-sweep-pane-state-verdicts.test.ts diff --git a/src/main/providers/agent-foreground-process-batch.test.ts b/src/main/providers/agent-foreground-process-batch.test.ts index b6f7b81031f..99f9e25138b 100644 --- a/src/main/providers/agent-foreground-process-batch.test.ts +++ b/src/main/providers/agent-foreground-process-batch.test.ts @@ -22,7 +22,7 @@ describe('batched foreground process correlation', () => { resolveAgentForegroundProcessesFromIndex(buildProcessTableIndex(rows), [ { rootPid: 100, fallbackProcess: 'zsh' } ]) - ).toEqual([{ available: true, processName: 'codex', shellIsForeground: false }]) + ).toEqual([{ available: true, processName: 'codex', shellOwnsEveryTtyProcessGroup: false }]) }) it('reports whether the shell itself owns the terminal, named process or not', () => { @@ -41,8 +41,8 @@ describe('batched foreground process correlation', () => { { rootPid: 300, fallbackProcess: 'zsh' } ]) ).toEqual([ - { available: true, processName: null, shellIsForeground: true }, - { available: true, processName: null, shellIsForeground: false } + { available: true, processName: null, shellOwnsEveryTtyProcessGroup: true }, + { available: true, processName: null, shellOwnsEveryTtyProcessGroup: false } ]) }) diff --git a/src/main/providers/agent-foreground-process-batch.ts b/src/main/providers/agent-foreground-process-batch.ts index 73dd07f3afc..66b060c0587 100644 --- a/src/main/providers/agent-foreground-process-batch.ts +++ b/src/main/providers/agent-foreground-process-batch.ts @@ -25,9 +25,9 @@ export type BatchedForegroundProcessResult = { available: boolean processName: string | null reason?: string - /** Set only when the table was readable: the PTY's own shell owns the terminal's foreground - * process group, so nothing is running in the pane. Left absent when we could not observe it. */ - shellIsForeground?: boolean + /** Set only when the table was readable: every process group attached to this PTY's terminal is + * the shell's own, and none of them is stopped. Left absent when we could not observe it. */ + shellOwnsEveryTtyProcessGroup?: boolean } export type BatchedForegroundProcessOptions = { @@ -36,6 +36,48 @@ export type BatchedForegroundProcessOptions = { stats?: ProcessTableIndexStats } +/** Which process groups occupy each controlling terminal, and which terminals hold a stopped + * process. */ +type TtyOccupancy = { + processGroupsByTty: ReadonlyMap> + stoppedTtys: ReadonlySet +} + +const ttyOccupancyByCapture = new WeakMap() + +/** Index the capture by controlling terminal. + * + * Keyed on `tpgid` because the snapshot carries no tty column and does not need one: a process + * group belongs to exactly one session, a session to at most one controlling terminal, so two + * rows reporting the same live `tpgid` are on the same tty. Memoized per capture, since the + * per-pane cadence poll and `pty.listProcesses` share one TTL-cached table. */ +function getTtyOccupancy(rows: readonly ProcessTableRow[]): TtyOccupancy { + const cached = ttyOccupancyByCapture.get(rows) + if (cached) { + return cached + } + const processGroupsByTty = new Map>() + const stoppedTtys = new Set() + for (const row of rows) { + if (row.pgid === undefined || row.tpgid === undefined || row.tpgid <= 0) { + continue + } + let groups = processGroupsByTty.get(row.tpgid) + if (!groups) { + groups = new Set() + processGroupsByTty.set(row.tpgid, groups) + } + groups.add(row.pgid) + // `T` is a job-control stop (Ctrl-Z), `t` a tracing stop. Both are work the pane still holds. + if (row.stat.startsWith('T') || row.stat.startsWith('t')) { + stoppedTtys.add(row.tpgid) + } + } + const occupancy: TtyOccupancy = { processGroupsByTty, stoppedTtys } + ttyOccupancyByCapture.set(rows, occupancy) + return occupancy +} + export async function resolveAgentForegroundProcessesBatch( requests: readonly BatchedForegroundProcessRequest[], options: BatchedForegroundProcessOptions = {} @@ -93,6 +135,7 @@ export function resolveAgentForegroundProcessesFromIndex( } } + const occupancy = getTtyOccupancy(index.rows) return requests.map((request) => { const root = lookupProcessTableIndex(index, (value) => value.byPid.get(request.rootPid)) if (!root) { @@ -116,10 +159,20 @@ export function resolveAgentForegroundProcessesFromIndex( reason: 'no_controlling_tty' } } - // The only host-observable "nothing is running here" signal: the terminal's foreground process - // group is the shell's own. Any foreground command — recognized agent or not — moves tpgid off - // it, so a reader may treat `false` as "busy" and must never treat absence as "idle". - const shellIsForeground = root.tpgid === root.pgid + // The only host-observable "nothing is running here" signal, and it has to be read off the + // whole tty rather than off `tpgid === pgid`. A backgrounded `pnpm build &` and a Ctrl-Z'd + // editor both leave the shell owning the foreground group, byte-identical to an idle prompt; + // what separates them is a second process group attached to the pane's terminal. That is also + // exactly the blast radius of the stop this attests to — `forceKillPosixPtyProcessGroups` + // SIGKILLs every process group on the tty — so the evidence and the kill now measure the same + // thing. A reader may treat `false` as "busy" and must never treat absence as "idle". + const ttyProcessGroups = occupancy.processGroupsByTty.get(root.tpgid) + const shellOwnsEveryTtyProcessGroup = + root.tpgid === root.pgid && + ttyProcessGroups !== undefined && + ttyProcessGroups.size === 1 && + ttyProcessGroups.has(root.pgid) && + !occupancy.stoppedTtys.has(root.tpgid) const allCandidates = rowsByOwner.get(root.pid) ?? [] const foregroundCandidates = allCandidates.filter((row) => row.pgid === root.tpgid) const fallbackProcess = request.fallbackProcess @@ -131,7 +184,7 @@ export function resolveAgentForegroundProcessesFromIndex( ) : foregroundCandidates if (wrapperFallback && candidates.length !== 1) { - return { available: true, processName: null, shellIsForeground } + return { available: true, processName: null, shellOwnsEveryTtyProcessGroup } } let bestCandidate: (ProcessTableRow & { depth: number }) | null = null let bestName: ReturnType = null @@ -150,10 +203,10 @@ export function resolveAgentForegroundProcessesFromIndex( return { available: true, processName: resolveOuterWrapperForegroundProcess(bestName, bestCandidate, allCandidates), - shellIsForeground + shellOwnsEveryTtyProcessGroup } } - return { available: true, processName: null, shellIsForeground } + return { available: true, processName: null, shellOwnsEveryTtyProcessGroup } }) } @@ -166,8 +219,8 @@ export function toForegroundProcessEvidence( ...metadata, verdict: 'live', processName: result.processName, - ...(result.shellIsForeground !== undefined - ? { shellIsForeground: result.shellIsForeground } + ...(result.shellOwnsEveryTtyProcessGroup !== undefined + ? { shellOwnsEveryTtyProcessGroup: result.shellOwnsEveryTtyProcessGroup } : {}) } : { diff --git a/src/main/providers/pty-provider-contract.ts b/src/main/providers/pty-provider-contract.ts index 35fca5b0b34..cec4dbcb30a 100644 --- a/src/main/providers/pty-provider-contract.ts +++ b/src/main/providers/pty-provider-contract.ts @@ -204,6 +204,12 @@ export type IPtyProvider = { keepHistory?: boolean deadlineMs?: number expectedIncarnationId?: PtyIncarnationId + /** Ask the execution host to refuse this stop unless it recorded this exact client identity + * as the PTY's creator AND this connection still authenticates as it. Optional because a + * host that predates it ignores the field, and because most stops are ordinary teardown of a + * pane whose owner the host may never have attested (a revived PTY carries none). Set it + * wherever the caller's authority to destroy comes from that attestation. */ + expectedOwnerClientInstanceId?: string } ): Promise sendSignal(id: string, signal: string): Promise diff --git a/src/main/providers/ssh-pty-provider.ts b/src/main/providers/ssh-pty-provider.ts index f218cc43eca..d48e09bba06 100644 --- a/src/main/providers/ssh-pty-provider.ts +++ b/src/main/providers/ssh-pty-provider.ts @@ -225,15 +225,17 @@ export class SshPtyProvider implements IPtyProvider { } async shutdown(id: string, opts: Parameters[1]): Promise { + // Both fences are omitted rather than sent undefined: a host that predates either must see no + // key at all, and the owner fence in particular must never reach it as a falsy claim. + const { expectedIncarnationId, expectedOwnerClientInstanceId } = opts await this.mux.request( 'pty.shutdown', { id: this.toRelayPtyId(id), immediate: opts.immediate ?? false, keepHistory: opts.keepHistory ?? false, - ...(opts.expectedIncarnationId === undefined - ? {} - : { expectedIncarnationId: opts.expectedIncarnationId }) + ...(expectedIncarnationId === undefined ? {} : { expectedIncarnationId }), + ...(expectedOwnerClientInstanceId === undefined ? {} : { expectedOwnerClientInstanceId }) }, relayTimeoutOptions(opts.deadlineMs) ) diff --git a/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts b/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts index 1d995ea5817..e2d8ef6c9dc 100644 --- a/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts +++ b/src/main/ssh/ssh-orphan-relay-pty-sweep.test.ts @@ -26,7 +26,7 @@ const OBSERVATION = { authorityGeneration: 'gen-1', observationEpoch: 1, capture /** The host looked and saw its own shell owning the terminal: nothing is running in the pane. */ function idleShell(): ForegroundProcessEvidence { - return { ...OBSERVATION, verdict: 'live', processName: null, shellIsForeground: true } + return { ...OBSERVATION, verdict: 'live', processName: null, shellOwnsEveryTtyProcessGroup: true } } function hostEntry(overrides: Partial = {}): PtyProcessInfo { @@ -90,6 +90,49 @@ describe('sweepOrphanedRelayPtys', () => { ) }) + it('asks the host to re-check ownership on the one call that cannot be undone', async () => { + // The stop is the only irreversible step in this flow, and until now the whole nine-condition + // rule was enforced only here, on the client that decided to make it. Naming the owner makes + // the host re-decide where the processes actually live. + const harness = createHarness([hostEntry()]) + + await run(harness) + + expect(harness.shutdown).toHaveBeenCalledWith( + `ssh:${TARGET}@@pty-1`, + expect.objectContaining({ expectedOwnerClientInstanceId: OURS }) + ) + }) + + it('does not act on an observation that aged out between the listing and the plan', async () => { + // The listing answered inside the budget, but this pass then spent longer than the evidence is + // good for. Staleness has to degrade to "leave it running". + let clock = 1_000_000 + const harness = createHarness([hostEntry()]) + harness.provider.listProcesses = vi.fn().mockImplementation(async () => { + clock += 1 + return [hostEntry()] + }) + + const pass = (maximumEvidenceAgeMs: number): Promise => + run(harness, { + now: () => clock, + maximumEvidenceAgeMs, + passBudgetMs: 60_000, + shouldContinue: () => { + clock += 20 + return true + } + }) + + await pass(10) + expect(harness.shutdown).not.toHaveBeenCalled() + + // Positive control: the same entry, the same elapsed time, a budget that covers it. + await pass(10_000) + expect(harness.shutdown).toHaveBeenCalledTimes(1) + }) + it('leaves a PTY the caller just reattached alone', async () => { const harness = createHarness([hostEntry()]) @@ -185,7 +228,7 @@ describe('sweepOrphanedRelayPtys', () => { ...OBSERVATION, verdict: 'live', processName: 'claude', - shellIsForeground: false + shellOwnsEveryTtyProcessGroup: false } }) ]) diff --git a/src/main/ssh/ssh-orphan-relay-pty-sweep.ts b/src/main/ssh/ssh-orphan-relay-pty-sweep.ts index 5d9082acb41..fbc72bf5ec8 100644 --- a/src/main/ssh/ssh-orphan-relay-pty-sweep.ts +++ b/src/main/ssh/ssh-orphan-relay-pty-sweep.ts @@ -3,6 +3,7 @@ import type { IPtyProvider } from '../providers/types' import { toAppSshPtyId, toRelaySshPtyId } from '../providers/ssh-pty-id' import { planRelayPtySweep, + RELAY_PTY_SWEEP_MAX_EVIDENCE_AGE_MS, RELAY_PTY_SWEEP_MIN_AGE_MS, type RelayPtyOwnershipEvidence } from '../../shared/ssh-relay-pty-ownership-proof' @@ -22,6 +23,7 @@ export type SshOrphanRelayPtySweepArgs = { minimumHostAgeMs?: number /** Absolute budget for the whole pass, in ms from its start. */ passBudgetMs?: number + maximumEvidenceAgeMs?: number } /** The two client-side claims the plan needs, read in one pass over the leases. @@ -29,9 +31,15 @@ export type SshOrphanRelayPtySweepArgs = { * `routed` is every relay PTY id this client still has a route to: a live lease, an id this * connect reattached, or a stop it recorded and has not delivered. * - * `expired` is separate on purpose. `expired` is written by four paths — a pane re-leasing under a - * new relay id, a reattach that failed on the transport, a pane surface missing from the layout, - * and a retired reattach — and every one of them deliberately leaves the remote process running. + * `expired` is separate on purpose. It is written by four paths, and every one of them + * deliberately leaves the remote process running: a pane re-leasing under a new relay id, a pane + * surface missing from the layout, a retired reattach, and a reattach the HOST answered "not + * found" for. Only the second of those is reached with the process provably alive — the layout + * refusal runs after `pty.attach` already succeeded (`restoreReattachedPtyRuntime`), which is + * precisely why its own comment reads "topology absence alone is not authority to kill a + * process". A reattach that failed on the transport writes nothing at all: it early-returns as + * `reattachAttemptsExhausted` and the lease stays `attached`, hence routed. + * * Folding it into `routed` would work, but it would also lose the reason in the skip log, and this * is the distinction the sweep most needs to be able to explain. */ function clientClaims(args: SshOrphanRelayPtySweepArgs): { @@ -95,6 +103,9 @@ export async function sweepOrphanedRelayPtys(args: SshOrphanRelayPtySweepArgs): const deadlineMs = now() + (args.passBudgetMs ?? RELAY_PTY_SWEEP_PASS_BUDGET_MS) try { const processes = await args.provider.listProcesses({ deadlineMs }) + // The instant the host's observations reached this client. Every later step — reading the + // leases, planning, issuing the stops — ages them, and the plan has to see that age. + const listedAtMs = now() if (!args.shouldContinue() || now() >= deadlineMs) { return } @@ -106,7 +117,9 @@ export async function sweepOrphanedRelayPtys(args: SshOrphanRelayPtySweepArgs): isSessionOwner: args.isSessionOwner, routedPtyIds: claims.routed, expiredLeasePtyIds: claims.expired, - minimumHostAgeMs: args.minimumHostAgeMs ?? RELAY_PTY_SWEEP_MIN_AGE_MS + minimumHostAgeMs: args.minimumHostAgeMs ?? RELAY_PTY_SWEEP_MIN_AGE_MS, + evidenceAgeSinceListingMs: Math.max(0, now() - listedAtMs), + maximumEvidenceAgeMs: args.maximumEvidenceAgeMs ?? RELAY_PTY_SWEEP_MAX_EVIDENCE_AGE_MS } ) if (plan.sweep.length === 0) { @@ -118,12 +131,15 @@ export async function sweepOrphanedRelayPtys(args: SshOrphanRelayPtySweepArgs): 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. + // Two fences, both enforced by the host that owns the process. The incarnation stops a + // relay that renumbered its ids between the read and this call from hitting a stranger; + // the owner id makes the host re-check the ownership rule itself, so the one irreversible + // call in this flow is not authorized by the client alone. await args.provider.shutdown(toAppSshPtyId(args.targetId, target.ptyId), { immediate: true, deadlineMs, - expectedIncarnationId: target.incarnationId + expectedIncarnationId: target.incarnationId, + expectedOwnerClientInstanceId: args.clientInstanceId }) console.log( `[ssh-orphan-sweep] stopped orphaned relay PTY ${args.targetId}/${target.ptyId}` diff --git a/src/main/ssh/ssh-orphan-sweep-pane-state-verdicts.test.ts b/src/main/ssh/ssh-orphan-sweep-pane-state-verdicts.test.ts new file mode 100644 index 00000000000..ab069240681 --- /dev/null +++ b/src/main/ssh/ssh-orphan-sweep-pane-state-verdicts.test.ts @@ -0,0 +1,238 @@ +// The one test that spans both halves of the sweep. Every other test in this feature asserts on a +// hand-written `ForegroundProcessEvidence` literal, which is exactly how a foreground-only idle +// predicate survived review: the literals said `shellIsForeground: true` for an idle shell because +// that is what the author believed, and nothing ever produced one from a real process table. +// +// So this runs the REAL publisher (`resolveAgentForegroundProcessesBatch` -> +// `toForegroundProcessEvidence`, what `pty.listProcesses` calls) against the REAL client reader +// (`planRelayPtySweep`), over `ps` output captured verbatim from a Linux container driving a real +// `bash -i` on a real pty. The fixtures below are transcripts, not constructions. +// +// Read the shell's own row in each fixture. In `background` and `ctrlz` it is +// `pgid == tpgid`, `Ss+` — byte-identical to `idle`. That is the defect: a foreground-only +// predicate cannot see a job the user backgrounded or suspended, and the stop it authorizes +// SIGKILLs every process group on the tty. +import { describe, expect, it } from 'vitest' +import { + resolveAgentForegroundProcessesBatch, + toForegroundProcessEvidence +} from '../providers/agent-foreground-process-batch' +import { parseStrictProcessTableRows } from '../../shared/process-table-snapshot' +import { + planRelayPtySweep, + RELAY_PTY_SWEEP_MAX_EVIDENCE_AGE_MS, + RELAY_PTY_SWEEP_MIN_AGE_MS, + type RelayPtySweepContext +} from '../../shared/ssh-relay-pty-ownership-proof' + +const OURS = 'client-instance-ours' + +/** `ps -axo pid=,ppid=,pgid=,tpgid=,stat=,command=` on debian:bookworm-slim, one capture per pane + * state, each with a `bash -i` on a pty forked by the harness. */ +const CAPTURES = { + /** Nothing running. The only sweepable state. */ + idle: { + rootPid: 3150, + table: [ + ' 1 0 1 -1 Ss python3 /work/.ptycap.py', + ' 3150 1 3150 3150 Ss+ bash -i', + ' 3151 1 1 -1 R ps -axo pid=,ppid=,pgid=,tpgid=,stat=,command=' + ] + }, + /** `sleep 300` in the foreground. The shell's tpgid moved off its own pgid. */ + foreground: { + rootPid: 3155, + table: [ + ' 1 0 1 -1 Ss python3 /work/.ptycap.py', + ' 3155 1 3155 3156 Ss bash -i', + ' 3156 3155 3156 3156 S+ sleep 300', + ' 3157 1 1 -1 R ps -axo pid=,ppid=,pgid=,tpgid=,stat=,command=' + ] + }, + /** `sleep 300 &`. The shell reads IDENTICALLY to `idle`; only the job's own row differs. */ + background: { + rootPid: 3152, + table: [ + ' 1 0 1 -1 Ss python3 /work/.ptycap.py', + ' 3152 1 3152 3152 Ss+ bash -i', + ' 3153 3152 3153 3152 S sleep 300', + ' 3154 1 1 -1 R ps -axo pid=,ppid=,pgid=,tpgid=,stat=,command=' + ] + }, + /** `sleep 300` then Ctrl-Z. The shell again reads IDENTICALLY to `idle`. */ + ctrlz: { + rootPid: 3158, + table: [ + ' 1 0 1 -1 Ss python3 /work/.ptycap.py', + ' 3158 1 3158 3158 Ss+ bash -i', + ' 3159 3158 3159 3158 T sleep 300', + ' 3160 1 1 -1 R ps -axo pid=,ppid=,pgid=,tpgid=,stat=,command=' + ] + } +} as const + +function context(overrides: Partial = {}): RelayPtySweepContext { + return { + clientInstanceId: OURS, + isSessionOwner: true, + routedPtyIds: new Set(), + expiredLeasePtyIds: new Set(), + minimumHostAgeMs: RELAY_PTY_SWEEP_MIN_AGE_MS, + evidenceAgeSinceListingMs: 0, + maximumEvidenceAgeMs: RELAY_PTY_SWEEP_MAX_EVIDENCE_AGE_MS, + ...overrides + } +} + +/** Everything the host does between reading `ps` and putting a record on the wire. */ +async function publish( + capture: { rootPid: number; table: readonly string[] }, + capturedAgeMs = 0 +): Promise> { + const rows = parseStrictProcessTableRows(capture.table.join('\n')) + const [result] = await resolveAgentForegroundProcessesBatch( + [{ rootPid: capture.rootPid, fallbackProcess: 'bash' }], + { rows } + ) + return toForegroundProcessEvidence(result, { + authorityGeneration: 'relay-generation-1', + observationEpoch: 1, + capturedAgeMs + }) +} + +/** Everything the client does with that record. Returns the plan for one orphan entry. */ +async function planFor( + capture: { rootPid: number; table: readonly string[] }, + overrides: { capturedAgeMs?: number; context?: Partial } = {} +): Promise> { + return planRelayPtySweep( + [ + { + ptyId: 'pty-1', + incarnationId: 'inc-1', + ownerClientInstanceId: OURS, + hostAgeMs: RELAY_PTY_SWEEP_MIN_AGE_MS * 2, + paneBound: true, + foregroundProcessEvidence: await publish(capture, overrides.capturedAgeMs) + } + ], + context(overrides.context) + ) +} + +function skipReason(plan: ReturnType): string | undefined { + return plan.skipped.find((entry) => entry.ptyId === 'pty-1')?.reason +} + +describe('what the host publishes about a pane, read by the sweep', () => { + it('records that a backgrounded and a suspended shell are indistinguishable at tpgid/pgid', () => { + // The premise of the whole file. If this ever fails, the fixtures drifted and every verdict + // below is testing something other than the defect. Pids differ between captures, so the + // comparison is of the shell row's shape: who its parent is, whether it leads its own process + // group, whether that group owns the terminal, and its state flags. + const shellShape = (capture: { rootPid: number; table: readonly string[] }): string => { + const row = parseStrictProcessTableRows(capture.table.join('\n')).find( + (candidate) => candidate.pid === capture.rootPid + )! + return [ + `ppid=${row.ppid}`, + `leadsOwnGroup=${row.pgid === row.pid}`, + `ownsTerminal=${row.tpgid === row.pgid}`, + `stat=${row.stat}` + ].join(' ') + } + + expect(shellShape(CAPTURES.idle)).toBe('ppid=1 leadsOwnGroup=true ownsTerminal=true stat=Ss+') + expect(shellShape(CAPTURES.background)).toBe(shellShape(CAPTURES.idle)) + expect(shellShape(CAPTURES.ctrlz)).toBe(shellShape(CAPTURES.idle)) + expect(shellShape(CAPTURES.foreground)).not.toBe(shellShape(CAPTURES.idle)) + }) + + it('sweeps an idle shell', async () => { + const evidence = await publish(CAPTURES.idle) + expect(evidence).toMatchObject({ + verdict: 'live', + processName: null, + shellOwnsEveryTtyProcessGroup: true + }) + + const plan = await planFor(CAPTURES.idle) + expect(plan.sweep).toEqual([{ ptyId: 'pty-1', incarnationId: 'inc-1' }]) + }) + + it('never sweeps a pane running a foreground job', async () => { + const evidence = await publish(CAPTURES.foreground) + expect(evidence).toMatchObject({ shellOwnsEveryTtyProcessGroup: false }) + + const plan = await planFor(CAPTURES.foreground) + expect(plan.sweep).toEqual([]) + expect(skipReason(plan)).toBe('host does not attest an idle shell') + }) + + it('never sweeps a pane holding a backgrounded job', async () => { + // `sleep 300 &`, i.e. `pnpm build &` or `npm run dev &`. The shell handed the terminal back, + // so the pane looks idle; the job is alive in its own process group on the same tty and a + // stop would SIGKILL it. + const evidence = await publish(CAPTURES.background) + expect(evidence).toMatchObject({ shellOwnsEveryTtyProcessGroup: false }) + + const plan = await planFor(CAPTURES.background) + expect(plan.sweep).toEqual([]) + expect(skipReason(plan)).toBe('host does not attest an idle shell') + }) + + it('never sweeps a pane holding a Ctrl-Z suspended job', async () => { + const evidence = await publish(CAPTURES.ctrlz) + expect(evidence).toMatchObject({ shellOwnsEveryTtyProcessGroup: false }) + + const plan = await planFor(CAPTURES.ctrlz) + expect(plan.sweep).toEqual([]) + expect(skipReason(plan)).toBe('host does not attest an idle shell') + }) + + it('refuses an observation older than the pass it would authorize', async () => { + // Same idle capture that sweeps above; only its age differs. Staleness degrades to "leave it + // running", never to "stop it". + const stale = await planFor(CAPTURES.idle, { + capturedAgeMs: RELAY_PTY_SWEEP_MAX_EVIDENCE_AGE_MS + 1 + }) + expect(stale.sweep).toEqual([]) + expect(skipReason(stale)).toBe('host foreground observation is too old to authorize a stop') + + // And the client's own share of the age counts: a host stamp inside the budget still ages out + // while this pass reads leases and plans. + const agedOnTheClient = await planFor(CAPTURES.idle, { + capturedAgeMs: RELAY_PTY_SWEEP_MAX_EVIDENCE_AGE_MS, + context: { evidenceAgeSinceListingMs: 1 } + }) + expect(agedOnTheClient.sweep).toEqual([]) + expect(skipReason(agedOnTheClient)).toBe( + 'host foreground observation is too old to authorize a stop' + ) + }) + + it('never sweeps when the host itself is too degraded to answer', async () => { + // `main` has since added `recoverRemoteTerminalRuntime`, a self-driven reconnect on relay + // node-pty failure — a sweep trigger that fires exactly when the host is unwell. The publisher + // has to fail closed there: a capture that cannot locate the shell is `unverifiable`, which is + // its own verdict and never collapses into "idle" (docs/reference/ssh-execution-boundary.md). + const evidence = await publish({ rootPid: 999_999, table: CAPTURES.idle.table }) + expect(evidence).toMatchObject({ verdict: 'unverifiable', reason: 'root_missing' }) + + const plan = await planFor({ rootPid: 999_999, table: CAPTURES.idle.table }) + expect(plan.sweep).toEqual([]) + expect(skipReason(plan)).toBe('host could not observe the pane foreground process') + }) + + it('still reclaims a shell whose work really did finish', async () => { + // The feature must not degrade into a no-op. The background fixture's job is gone; what is + // left is the same orphaned shell, and it is swept. + const finished = { + rootPid: CAPTURES.background.rootPid, + table: CAPTURES.background.table.filter((line) => !line.includes('sleep 300')) + } + const plan = await planFor(finished) + expect(plan.sweep).toEqual([{ ptyId: 'pty-1', incarnationId: 'inc-1' }]) + }) +}) diff --git a/src/main/ssh/ssh-relay-session-orphan-sweep.test.ts b/src/main/ssh/ssh-relay-session-orphan-sweep.test.ts index 84198d7a185..fd4cbea8c34 100644 --- a/src/main/ssh/ssh-relay-session-orphan-sweep.test.ts +++ b/src/main/ssh/ssh-relay-session-orphan-sweep.test.ts @@ -132,7 +132,7 @@ function hostEntry( ...OBSERVATION, verdict: 'live', processName: null, - shellIsForeground: true + shellOwnsEveryTtyProcessGroup: true }, ...overrides } @@ -246,7 +246,7 @@ describe('SshRelaySession orphaned relay PTY sweep', () => { ...OBSERVATION, verdict: 'live', processName: 'claude', - shellIsForeground: false + shellOwnsEveryTtyProcessGroup: false } }) ]) diff --git a/src/relay/pty-handler-inventory-process-evidence.test.ts b/src/relay/pty-handler-inventory-process-evidence.test.ts index 17896f67c72..1c8b59bc6de 100644 --- a/src/relay/pty-handler-inventory-process-evidence.test.ts +++ b/src/relay/pty-handler-inventory-process-evidence.test.ts @@ -157,22 +157,32 @@ describe('PtyHandler inventory foreground evidence', () => { expect((await listProcesses())[0].title).toBe('node') }) - it.each([1, 8])('visits the host table exactly once for %s panes', async (paneCount) => { - const table = Array.from({ length: paneCount }, (_, index) => - paneRows(10_000 + index * 10, ['node /opt/codex']) - ).flat() - const { rows, reads } = countingRows(table) - mockGetStrictProcessTableSnapshot.mockResolvedValue(rows) - for (let index = 0; index < paneCount; index += 1) { - await spawnPane(10_000 + index * 10, 'zsh') + // The cost that matters is per-CAPTURE, not per-pane: the defect this guards against is a + // full-table walk for every pane, which is what an O(PTY x rows) inventory looked like. Two + // linear passes build the two indexes the resolver reads — parent/child correlation, and which + // process groups occupy each controlling terminal — and neither grows with the pane count. + const CAPTURE_PASSES = 2 + + it.each([1, 8])( + 'walks the host table a fixed number of times for %s panes', + async (paneCount) => { + const table = Array.from({ length: paneCount }, (_, index) => + paneRows(10_000 + index * 10, ['node /opt/codex']) + ).flat() + const { rows, reads } = countingRows(table) + mockGetStrictProcessTableSnapshot.mockResolvedValue(rows) + for (let index = 0; index < paneCount; index += 1) { + await spawnPane(10_000 + index * 10, 'zsh') + } + + const listed = await listProcesses() + + expect(listed).toHaveLength(paneCount) + expect(listed.every((entry) => entry.title === 'codex')).toBe(true) + expect(mockGetStrictProcessTableSnapshot).toHaveBeenCalledTimes(1) + // Linear in the capture — NOT one full-table walk per pane, which would be + // `table.length * paneCount` here. + expect(reads()).toBe(table.length * CAPTURE_PASSES) } - - const listed = await listProcesses() - - expect(listed).toHaveLength(paneCount) - expect(listed.every((entry) => entry.title === 'codex')).toBe(true) - expect(mockGetStrictProcessTableSnapshot).toHaveBeenCalledTimes(1) - // One linear index pass — NOT one full-table walk per pane. - expect(reads()).toBe(table.length) - }) + ) }) diff --git a/src/relay/pty-handler-ownership-attestation.test.ts b/src/relay/pty-handler-ownership-attestation.test.ts index 34c21f39e57..ec5f1beb5e3 100644 --- a/src/relay/pty-handler-ownership-attestation.test.ts +++ b/src/relay/pty-handler-ownership-attestation.test.ts @@ -33,6 +33,7 @@ import { endPtyHandlerTest, type MockDispatcher } from './pty-handler-test-harness' +import { PROCESS_TABLE_SNAPSHOT_MAX_STALENESS_MS } from '../shared/process-table-snapshot' const PANE_KEY = 'tab-agent:22222222-2222-4222-8222-222222222222' @@ -41,6 +42,7 @@ type Summary = { paneBound?: boolean hostAgeMs?: number ownerClientInstanceId?: string + foregroundProcessEvidence?: { capturedAgeMs: number } } describe('PtyHandler publishes host-attested PTY ownership', () => { @@ -136,4 +138,146 @@ describe('PtyHandler publishes host-attested PTY ownership', () => { expect(entry?.paneBound).toBe(false) expect(entry?.ownerClientInstanceId).toBe('client-A') }) + + it('dates the foreground observation instead of stamping it fresh', async () => { + // `capturedAgeMs` used to be a hardcoded 0 with no reader anywhere, so the one field that + // exists to bound staleness asserted the evidence was never stale. It now carries the + // worst-case age of the TTL-shared capture the record was derived from. + const { id } = await spawnFrom(7, { env: { ORCA_PANE_KEY: PANE_KEY } }) + + const entry = (await listProcesses()).find((process) => process.id === id) + + expect(entry?.foregroundProcessEvidence?.capturedAgeMs).toBe( + PROCESS_TABLE_SNAPSHOT_MAX_STALENESS_MS + ) + }) +}) + +// CodeRabbit's unaddressed note, and the asymmetry behind it: `pty.spawn` and `pty.attach` both +// take a request context and both check it, while `pty.shutdown` — the one call that irreversibly +// destroys a user's running process — took none, so the entire ownership rule was enforced only on +// the client that decided to make the call. +describe('PtyHandler authorizes a fenced stop against its own attestation', () => { + let dispatcher: MockDispatcher + let handler: PtyHandler + let originalPlatform: PropertyDescriptor | undefined + + async function spawnFrom(clientId: number): Promise<{ id: string }> { + mockPtySpawn.mockReturnValue({ ...mockPtyInstance, onData: vi.fn(), onExit: vi.fn() }) + return (await dispatcher.callRequest('pty.spawn', { env: { ORCA_PANE_KEY: PANE_KEY } }, { + clientId, + isStale: () => false + } as never)) as { id: string } + } + + async function isStillHeld(id: string): Promise { + const entries = (await dispatcher.callRequest('pty.listProcesses', {})) as Summary[] + return entries.some((entry) => entry.id === id) + } + + /** What actually reaches the process. A refusal has to leave this untouched — the point of the + * check is the process, not the error. */ + function killSignals(): unknown[][] { + return mockPtyInstance.kill.mock.calls + } + + function stop(id: string, params: Record, clientId: number): Promise { + return dispatcher.callRequest('pty.shutdown', { id, immediate: false, ...params }, { + clientId, + isStale: () => false + } as never) + } + + beforeEach(() => { + ;({ dispatcher, handler, originalPlatform } = beginPtyHandlerTest({ + mockPtySpawn, + mockPtyInstance, + mockCreateShellPromptReadinessProbe + })) + handler.setConsumerIdentityResolver((clientId) => + clientId === 7 ? 'client-A' : clientId === 8 ? 'client-B' : null + ) + }) + + afterEach(async () => { + await endPtyHandlerTest(handler, originalPlatform) + }) + + it('stops a PTY when the connection and the host agree on the owner', async () => { + const { id } = await spawnFrom(7) + + await expect( + stop(id, { expectedOwnerClientInstanceId: 'client-A' }, 7) + ).resolves.toBeUndefined() + expect(killSignals()).toEqual([['SIGTERM']]) + }) + + it('refuses when another client asserts our identity, and leaves the process running', async () => { + // The claim is a parameter, so a confused or displaced client can send any value it likes. + // What it cannot do is authenticate as that identity on this connection. + const { id } = await spawnFrom(7) + + await expect(stop(id, { expectedOwnerClientInstanceId: 'client-A' }, 8)).rejects.toThrow( + /requester is not the attested owner/ + ) + expect(killSignals()).toEqual([]) + expect(await isStillHeld(id)).toBe(true) + }) + + it('refuses when the connection holds no grant at all', async () => { + const { id } = await spawnFrom(7) + + await expect(stop(id, { expectedOwnerClientInstanceId: 'client-A' }, 99)).rejects.toThrow( + /requester is not the attested owner/ + ) + expect(killSignals()).toEqual([]) + expect(await isStillHeld(id)).toBe(true) + }) + + it('refuses a PTY this host never attested, even to the client that asked', async () => { + // The revived-PTY case. Both sweep preconditions a client can see are satisfied — pane-bound, + // old enough — and only the host knows it never recorded a creator for it. + const revivedId = 'pty-revived-fence' + await dispatcher.callRequest( + 'pty.revive', + { + state: JSON.stringify([ + { + id: revivedId, + pid: process.pid, + cwd: process.cwd(), + paneKey: PANE_KEY, + cols: 80, + rows: 24 + } + ]) + }, + { clientId: 7, isStale: () => false } as never + ) + + await expect(stop(revivedId, { expectedOwnerClientInstanceId: 'client-A' }, 7)).rejects.toThrow( + /this host attested no such owner/ + ) + expect(killSignals()).toEqual([]) + expect(await isStillHeld(revivedId)).toBe(true) + }) + + it('leaves an ordinary teardown that names no owner exactly as it was', async () => { + // Rule 1's obligation: an old client, and every non-sweep caller on a current one, omits the + // field. The host must not start refusing a stop it is obliged to honour. + const { id } = await spawnFrom(7) + + await expect(stop(id, {}, 7)).resolves.toBeUndefined() + expect(killSignals()).toEqual([['SIGTERM']]) + }) + + it('rejects a malformed owner claim rather than ignoring it', async () => { + const { id } = await spawnFrom(7) + + await expect(stop(id, { expectedOwnerClientInstanceId: '' }, 7)).rejects.toThrow( + /Invalid expectedOwnerClientInstanceId/ + ) + expect(killSignals()).toEqual([]) + expect(await isStillHeld(id)).toBe(true) + }) }) diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index d8fe370b50c..25945203878 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -70,6 +70,7 @@ import { } from '../main/providers/agent-foreground-process' import { getStrictProcessTableSnapshot, + PROCESS_TABLE_SNAPSHOT_MAX_STALENESS_MS, type ProcessTableRow } from '../shared/process-table-snapshot' import type { ForegroundProcessEvidence } from '../shared/foreground-process-evidence' @@ -1013,7 +1014,7 @@ export class PtyHandler { private registerHandlers(): void { this.dispatcher.onRequest('pty.spawn', (p, context) => this.spawn(p, context)) this.dispatcher.onRequest('pty.attach', (p, context) => this.attach(p, context)) - this.dispatcher.onRequest('pty.shutdown', (p) => this.shutdown(p)) + this.dispatcher.onRequest('pty.shutdown', (p, context) => this.shutdown(p, context)) this.dispatcher.onRequest('pty.sendSignal', (p) => this.sendSignal(p)) this.dispatcher.onRequest('pty.getCwd', (p) => this.getCwd(p)) this.dispatcher.onRequest('pty.getInitialCwd', (p) => this.getInitialCwd(p)) @@ -2123,7 +2124,7 @@ export class PtyHandler { return { cols: managed.pty.cols, rows: managed.pty.rows } } - private async shutdown(params: Record): Promise { + private async shutdown(params: Record, context?: RequestContext): Promise { const id = params.id as string const immediate = params.immediate as boolean const expectedIncarnationId = params.expectedIncarnationId @@ -2133,6 +2134,14 @@ export class PtyHandler { ) { throw new Error('Invalid expectedIncarnationId') } + const expectedOwnerClientInstanceId = params.expectedOwnerClientInstanceId + if ( + expectedOwnerClientInstanceId !== undefined && + (typeof expectedOwnerClientInstanceId !== 'string' || + expectedOwnerClientInstanceId.length === 0) + ) { + throw new Error('Invalid expectedOwnerClientInstanceId') + } const managed = this.ptys.get(id) if (!managed) { return @@ -2140,6 +2149,9 @@ export class PtyHandler { if (expectedIncarnationId !== undefined && expectedIncarnationId !== managed.incarnationId) { throw new Error(`PTY incarnation mismatch for ${id}`) } + if (expectedOwnerClientInstanceId !== undefined) { + this.assertShutdownOwnership(id, managed, expectedOwnerClientInstanceId, context) + } // Why: `pty.shutdown` is the only authoritative statement this host ever gets that a tab is // gone. Record it before the kill request, because the kill is the part that can fail: an agent // that survives teardown otherwise keeps posting hooks the relay forwards as a live agent pane @@ -2164,6 +2176,34 @@ export class PtyHandler { } } + /** Re-decide, on the host, whether the caller may destroy this PTY. + * + * `pty.shutdown` is irreversible and its siblings `pty.spawn`/`pty.attach` already take a + * request context; without this the whole ownership rule lived on the client, on the one call + * that cannot be taken back. Both halves are checked here because either alone is an echo: the + * connection must still authenticate as that consumer identity (so a claim cannot be asserted), + * and this host must have recorded that same identity as the PTY's creator at spawn (so the + * caller cannot reach a PTY it never made). + * + * Only callers that opt in are checked. An ordinary pane teardown does not pass the field, and + * must not: a revived PTY carries no attested owner at all, and a host predating the attestation + * would refuse stops it is obliged to honour. */ + private assertShutdownOwnership( + id: string, + managed: ManagedPty, + expectedOwnerClientInstanceId: string, + context: RequestContext | undefined + ): void { + const requester = + context === undefined ? null : (this.consumerIdentityResolver?.(context.clientId) ?? null) + if (requester !== expectedOwnerClientInstanceId) { + throw new Error(`PTY "${id}" stop refused: requester is not the attested owner`) + } + if (managed.ownerClientInstanceId !== expectedOwnerClientInstanceId) { + throw new Error(`PTY "${id}" stop refused: this host attested no such owner`) + } + } + /** Record that this pane's client surface is gone, and tell the hook server so the pane's cached * agent status stops being replayed to reconnecting clients. Returns false when there is no pane * surface to retire. */ @@ -2432,9 +2472,15 @@ export class PtyHandler { let evidenceRows: readonly ProcessTableRow[] | null = null let evidenceResults: BatchedForegroundProcessResult[] = [] const evidenceEpoch = ++this.foregroundEvidenceEpoch + // Worst-case capture time for the snapshot below, not the instant its await settled: the + // reader may serve a TTL-cached table, so the observation can already be one window old. The + // loop that follows can await per entry, so each record is stamped against this rather than + // carrying a shared constant. + let evidenceCapturedAtMs = Date.now() if (process.platform !== 'win32' && managedEntries.length > 0) { try { evidenceRows = await getStrictProcessTableSnapshot() + evidenceCapturedAtMs = Date.now() - PROCESS_TABLE_SNAPSHOT_MAX_STALENESS_MS evidenceResults = await resolveAgentForegroundProcessesBatch( managedEntries.map(([, managed]) => ({ rootPid: managed.pty.pid, @@ -2468,7 +2514,7 @@ export class PtyHandler { { authorityGeneration: this.ptyIdMintEpoch, observationEpoch: evidenceEpoch, - capturedAgeMs: 0 + capturedAgeMs: Math.max(0, Date.now() - evidenceCapturedAtMs) } ) : undefined diff --git a/src/shared/foreground-process-evidence.ts b/src/shared/foreground-process-evidence.ts index bda28686453..bbcfd091d60 100644 --- a/src/shared/foreground-process-evidence.ts +++ b/src/shared/foreground-process-evidence.ts @@ -2,7 +2,14 @@ export type ForegroundEvidenceObservation = { authorityGeneration: string observationEpoch: number - /** Age at serialization; receivers rebase this onto their monotonic clock. */ + /** How old the underlying process-table capture was when this record was serialized, measured on + * the OBSERVING host's clock so no clock skew enters it. Receivers rebase it onto their own + * monotonic clock by adding the time since the carrying response arrived. + * + * It is an upper bound, not an estimate: the capture is TTL-shared, so a reader may be served + * one up to `PROCESS_TABLE_SNAPSHOT_MAX_STALENESS_MS` older than its own await, and the + * producer stamps for that worst case. Erring old is the safe direction for every consumer — + * the only one that acts destructively refuses stale evidence. */ capturedAgeMs: number } @@ -10,11 +17,19 @@ export type ForegroundProcessEvidence = | ({ verdict: 'live' processName: string | null - /** True only when the host observed the PTY's own shell owning the terminal's foreground - * process group — i.e. nothing is running in the pane. False means something IS running, - * named or not. Absent from a host that predates the field, which is neither: a reader - * deciding whether the pane is idle must require `true` and defer on anything else. */ - shellIsForeground?: boolean + /** True only when the host observed every process group attached to this PTY's terminal to be + * the shell's own, with none of them stopped — i.e. nothing is running in the pane, in the + * foreground OR the background, and nothing sits suspended. + * + * Deliberately not `tpgid === pgid`: a job the user backgrounded with `&` and a job the user + * suspended with Ctrl-Z both hand the terminal back to the shell, so a foreground-only + * predicate reads them as idle. This one is measured against the same set of process groups + * a forced stop would SIGKILL. + * + * False means something IS running, named or not. Absent from a host that predates the + * field, which is neither: a reader deciding whether the pane is idle must require `true` + * and defer on anything else. */ + shellOwnsEveryTtyProcessGroup?: boolean } & ForegroundEvidenceObservation) | ({ verdict: 'unverifiable'; reason: string } & ForegroundEvidenceObservation) @@ -38,7 +53,10 @@ export function isForegroundProcessEvidence(value: unknown): value is Foreground return false } if (input.verdict === 'live') { - if (input.shellIsForeground !== undefined && typeof input.shellIsForeground !== 'boolean') { + if ( + input.shellOwnsEveryTtyProcessGroup !== undefined && + typeof input.shellOwnsEveryTtyProcessGroup !== 'boolean' + ) { return false } return input.processName === null || typeof input.processName === 'string' diff --git a/src/shared/process-table-snapshot.ts b/src/shared/process-table-snapshot.ts index 2d65fc3f324..4ef6bf0f024 100644 --- a/src/shared/process-table-snapshot.ts +++ b/src/shared/process-table-snapshot.ts @@ -364,6 +364,11 @@ const processTableReader = createProcessTableSnapshotReader now: () => Date.now() }) +/** How much older than its own await a snapshot from the shared reader may be. The reader serves a + * capture from its TTL cache, so a caller that needs to state the observation's age must assume + * this whole window rather than the instant its await settled. */ +export const PROCESS_TABLE_SNAPSHOT_MAX_STALENESS_MS = DEFAULT_SNAPSHOT_TTL_MS + /** * Run (or reuse a recent) `ps -axo` process-table scan and return * its parsed rows. Per-process singleton: the relay and local main processes diff --git a/src/shared/ssh-relay-pty-ownership-proof.test.ts b/src/shared/ssh-relay-pty-ownership-proof.test.ts index 2764658fdc9..1abfc0ef7f5 100644 --- a/src/shared/ssh-relay-pty-ownership-proof.test.ts +++ b/src/shared/ssh-relay-pty-ownership-proof.test.ts @@ -4,6 +4,7 @@ import { describe, expect, it } from 'vitest' import type { ForegroundProcessEvidence } from './foreground-process-evidence' import { planRelayPtySweep, + RELAY_PTY_SWEEP_MAX_EVIDENCE_AGE_MS, RELAY_PTY_SWEEP_MAX_PER_PASS, RELAY_PTY_SWEEP_MIN_AGE_MS, type RelayPtyOwnershipEvidence, @@ -16,7 +17,7 @@ const OBSERVATION = { authorityGeneration: 'gen-1', observationEpoch: 1, capture /** The host looked at the pane and saw its own shell owning the terminal: nothing is running. */ function idleShell(): ForegroundProcessEvidence { - return { ...OBSERVATION, verdict: 'live', processName: null, shellIsForeground: true } + return { ...OBSERVATION, verdict: 'live', processName: null, shellOwnsEveryTtyProcessGroup: true } } function orphan(overrides: Partial = {}): RelayPtyOwnershipEvidence { @@ -38,6 +39,8 @@ function context(overrides: Partial = {}): RelayPtySweepCo routedPtyIds: new Set(), expiredLeasePtyIds: new Set(), minimumHostAgeMs: RELAY_PTY_SWEEP_MIN_AGE_MS, + evidenceAgeSinceListingMs: 0, + maximumEvidenceAgeMs: RELAY_PTY_SWEEP_MAX_EVIDENCE_AGE_MS, ...overrides } } @@ -80,7 +83,7 @@ describe('planRelayPtySweep', () => { ...OBSERVATION, verdict: 'live', processName: 'claude', - shellIsForeground: false + shellOwnsEveryTtyProcessGroup: false } }) ], @@ -101,7 +104,7 @@ describe('planRelayPtySweep', () => { ...OBSERVATION, verdict: 'live', processName: null, - shellIsForeground: false + shellOwnsEveryTtyProcessGroup: false } }) ], diff --git a/src/shared/ssh-relay-pty-ownership-proof.ts b/src/shared/ssh-relay-pty-ownership-proof.ts index 03f7adc05ef..ff929cf4d81 100644 --- a/src/shared/ssh-relay-pty-ownership-proof.ts +++ b/src/shared/ssh-relay-pty-ownership-proof.ts @@ -51,14 +51,25 @@ export type RelayPtySweepContext = { * * Separate from {@link routedPtyIds} because it is a different fact with the same verdict: an * expired lease records that THIS CLIENT lost its handle — a pane re-leased under a new relay - * id, a reattach that failed on the transport, a pane surface that is no longer in the layout. - * Every one of those writers deliberately declines to stop the remote process, and client-side - * absence is `unverifiable` by construction (`docs/reference/ssh-execution-boundary.md`). So an - * expired lease is the record of a process left running on purpose, never a licence to kill it. */ + * id, a pane surface that is no longer in the layout, a retired reattach. The layout case is the + * one that matters most, because it is reached only AFTER `pty.attach` succeeded: the process is + * not merely unproven, it is known to be alive. Every one of those writers deliberately declines + * to stop it, and client-side absence is `unverifiable` by construction + * (`docs/reference/ssh-execution-boundary.md`). So an expired lease is the record of a process + * left running on purpose, never a licence to kill it. */ expiredLeasePtyIds: 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 + /** How long ago, on THIS client's clock, the listing that carried the evidence arrived. Added to + * each entry's host-stamped `capturedAgeMs` so {@link maximumEvidenceAgeMs} bounds staleness at + * the moment of the decision rather than at the moment of serialization. The transit itself is + * unmeasured — the two clocks are not synchronized — but it is bounded by the listing's own RPC + * deadline, and both halves that ARE measurable are counted. */ + evidenceAgeSinceListingMs: number + /** Oldest foreground observation that may authorize a stop. Stale evidence degrades to "do not + * sweep", never to "sweep". */ + maximumEvidenceAgeMs: number } export type RelayPtySweepTarget = { ptyId: string; incarnationId: string } @@ -79,15 +90,36 @@ export const RELAY_PTY_SWEEP_MIN_AGE_MS = 30_000 * 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 +/** The oldest foreground observation this sweep will treat as authorization to SIGKILL. + * + * Sized to the pass budget rather than to the 30s spawn floor: those answer different questions. + * The floor guards a concurrent spawn this client has not recorded yet; this one guards the pane + * the user started working in AFTER the host looked. An observation older than the whole pass it + * is meant to authorize cannot have been taken for this pass, so it is not evidence about now. + * + * It does not remove the race — nothing can, the host cannot re-check between the answer and the + * signal — it bounds it. The display consumer of the same measurement deliberately keeps NO age + * budget: a stale pane title costs a redraw and self-corrects on the next poll, so one truthful + * number carries two explicit budgets rather than one implicit one. */ +export const RELAY_PTY_SWEEP_MAX_EVIDENCE_AGE_MS = 5_000 + /** The host's own answer to "is anything running in this pane?". Only a positive "no" clears the * sweep; every other shape — an older host, an unreadable process table, a named foreground * process, a busy foreground group — is a reason to leave the process alone. */ -function foregroundSkipReason(evidence: ForegroundProcessEvidence | undefined): string | null { +function foregroundSkipReason( + evidence: ForegroundProcessEvidence | undefined, + context: RelayPtySweepContext +): string | null { if (evidence === undefined) { // A host that never published it, or a Windows host where it is not collected. Absence of the // observation is not the observation of absence. return 'host published no foreground-process observation' } + // Before anything is read out of it: an observation is only a claim about the instant it was + // taken. Age is checked on both verdicts because a stale `unverifiable` is no better. + if (evidence.capturedAgeMs + context.evidenceAgeSinceListingMs > context.maximumEvidenceAgeMs) { + return 'host foreground observation is too old to authorize a stop' + } if (evidence.verdict !== 'live') { return 'host could not observe the pane foreground process' } @@ -96,9 +128,10 @@ function foregroundSkipReason(evidence: ForegroundProcessEvidence | undefined): // exactly the hand-launched `claude`/`codex` case agentSessionOwners cannot see. return 'host observes a named foreground process' } - if (evidence.shellIsForeground !== true) { - // Either the terminal's foreground group is not the shell's (something unrecognized is - // running - a build, an editor, a test run), or this host predates the field. + if (evidence.shellOwnsEveryTtyProcessGroup !== true) { + // Something other than the shell's own process group is attached to the pane's terminal — a + // foreground command, a job backgrounded with `&`, a Ctrl-Z'd editor — or this host predates + // the field. The stop would SIGKILL that group, so none of those is a pane to reclaim. return 'host does not attest an idle shell' } return null @@ -129,7 +162,7 @@ function skipReason( // agent. Reaping it converts a recoverable session into a destroyed one. return 'host still advertises an adoptable agent session' } - const foregroundSkip = foregroundSkipReason(entry.foregroundProcessEvidence) + const foregroundSkip = foregroundSkipReason(entry.foregroundProcessEvidence, context) if (foregroundSkip !== null) { return foregroundSkip }