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 }