diff --git a/config/reliability-gates.jsonc b/config/reliability-gates.jsonc index 687de8008a0..9d5ed04ec7d 100644 --- a/config/reliability-gates.jsonc +++ b/config/reliability-gates.jsonc @@ -18721,7 +18721,7 @@ "https://github.com/stablyai/orca/pull/16904" ], "invariant": "A PTY id is certified exited only by the relay that owned the record and watched the process end: node-pty's onExit, or a pid probe that answered ESRCH. Every other disappearance of a record - a shutdown that stopped waiting for an uninterruptible child, the torn-down-record sweep, an evicted or forgotten observation, an id this relay never held, a revive in flight, a record mid-teardown, and a host too old to answer - is unverifiable, and recovery defers instead of retiring the worker.", - "oracle": "The relay probe answers live/exited/unknown for each of those states directly. The provider maps only live and exited, and maps a JSON-RPC -32601 rejection to null with no capability handshake. At the recovery seam, an observed exit converges to an exited resolution that releases the Task, while the shutdown-timeout case (with and without a sibling kill failure) and the old-relay case produce no resolution and a deferred dispatch, with a real orchestration DB refusing Task reuse.", + "oracle": "The relay probe answers live/exited/unverifiable for each of those states directly, and binds the answer to one incarnation: admission under a reused id clears the prior observation, and only the incarnation that currently owns the id may record one, including a callback that arrives after teardown removed the record. The provider maps only live and exited, and maps a JSON-RPC -32601 rejection to null with no capability handshake. At the recovery seam, an observed exit converges to an exited resolution that releases the Task, while the shutdown-timeout case (with and without a sibling kill failure) and the old-relay case produce no resolution and a deferred dispatch, with a real orchestration DB refusing Task reuse.", "commands": [ "ORCA_BACKGROUND_LAUNCH=1 node_modules/.bin/vitest run --config config/vitest.config.ts src/relay/pty-handler-liveness-probe.test.ts src/relay/pty-handler-ssh-exit-certification.test.ts src/relay/pty-handler-ssh-recovery-convergence.test.ts src/main/providers/ssh-pty-liveness-probe.test.ts src/main/providers/ssh-pty-provider-liveness-readback.test.ts src/main/runtime/runtime-legacy-worker-terminal-exit-certification.test.ts" ], @@ -18738,9 +18738,12 @@ "file": "src/relay/pty-handler-liveness-probe.test.ts", "assertions": [ "separates a real exit from a teardown removal in the same shutdown", + "certifies an exit node-pty reported after the shutdown wait had given up", "stays unverifiable after the listing sweeps a torn-down record away", "stays unverifiable while a revive holds the id, and answers for the new record after", - "forgets its oldest observation at the ledger cap instead of growing without bound", + "does not certify a revived process with the exit of the one it replaced", + "does not let a superseded incarnation certify the id once both records are gone", + "evicts observations in the order it made them, not the order it was asked", "forgets every observation when the relay restarts, even under the same mint epoch" ] }, @@ -18748,6 +18751,8 @@ "file": "src/relay/pty-handler-ssh-recovery-convergence.test.ts", "assertions": [ "recovers a worker after the connected relay observed its physical exit while the client was absent", + "recovers a worker whose exit the relay observed after its shutdown wait gave up", + "does not retire a revived worker with the exit of the process it replaced", "does not certify an unexited shutdown record; sibling kill failure=%s" ] }, @@ -18755,7 +18760,9 @@ "file": "src/relay/pty-handler-ssh-exit-certification.test.ts", "assertions": [ "does not certify exit after shutdown bookkeeping removes an unexited PTY", - "keeps missing shutdown records unverifiable when another failed kill leaves the relay serving" + "keeps missing shutdown records unverifiable when another failed kill leaves the relay serving", + "publishes nothing that names the generation which minted its PTY ids", + "leaves a shutdown-removed id unverifiable for a client that infers from listings" ] }, { @@ -18782,13 +18789,13 @@ "platform": "macos", "command": "ORCA_BACKGROUND_LAUNCH=1 node_modules/.bin/vitest run --config config/vitest.config.ts src/relay/pty-handler-liveness-probe.test.ts src/relay/pty-handler-ssh-exit-certification.test.ts src/relay/pty-handler-ssh-recovery-convergence.test.ts src/main/providers/ssh-pty-liveness-probe.test.ts src/main/providers/ssh-pty-provider-liveness-readback.test.ts src/main/runtime/runtime-legacy-worker-terminal-exit-certification.test.ts", "result": "passed", - "durationSeconds": 1.43, - "summary": "6 files, 47 tests passed. The convergence file fails 1/3 without the owner readback; six mutations of the probe and its forwarder each fail at least one of these tests." + "durationSeconds": 1.27, + "summary": "6 files, 56 tests passed. The convergence file fails 1/3 without the owner readback; twelve mutations of the probe, its incarnation binding, the published capabilities, the bounded map and the forwarder each fail at least one of these tests." } ], "runtimeBudget": { "p95Seconds": 30, - "scope": "Six unit/service-level files; the ledger-eviction case drives 4,097 spawn-and-exit cycles through the real handler and dominates the run at roughly 0.8 s." + "scope": "Six unit/service-level files; the ledger-eviction case drives 4,097 spawn-and-exit cycles through the real handler and reads the oldest entry before the overflow and dominates the run at roughly 0.8 s." }, "flakeHistory": { "status": "not-started", @@ -18796,7 +18803,7 @@ }, "redGreenEvidence": { "status": "complete", - "evidence": "Red: the archived round-3 reproduction failed 1/3 at head 0be1813781 with an empty resolution list, and the round-2 reproduction failed 2/4 against the removed client-side epoch inference. Green at this head. Six independent mutations (record in the shared reap, dropped disposed check, dropped pending-revive check, absent-record-as-exited, provider unknown-as-exited, provider rejection-as-exited) each fail a checked-in test." + "evidence": "Red: the archived round-3 reproduction failed 1/3 at head 0be1813781 with an empty resolution list, and the round-2 reproduction failed 2/4 against the removed client-side epoch inference. The round-4 review reproduced three more at head 03af416c38 - a revived id certified from its predecessor's exit, an exit observed after disposal lost forever, and an old client's inference restored by a published mint epoch. Green at this head. Twelve independent mutations (record in the shared reap, dropped disposed check, dropped pending-revive check, absent-record-as-exited, provider unknown-as-exited, provider rejection-as-exited, read-reordered ledger eviction, dropped admission invalidation, dropped incarnation guard, incarnation guard weakened to an empty pool, recording moved back below the disposed guard, republished mint epoch) each fail a checked-in test." }, "performanceBudget": { "required": false, @@ -18810,7 +18817,7 @@ "knownGaps": [ "No live SSH server, real relay process restart, or kernel-uninterruptible child; the shutdown-timeout and restart cases are simulated at the handler seam.", "The observed-exit ledger is in-memory and per relay process; a relay that restarts between the observation and the question answers unverifiable forever for that id, and only manual stop/abandon releases the worker.", - "A disposed record left in the pool is not reachable from any current teardown, so that guard is exercised by constructing the state rather than by a production path.", + "A disposed record left in the pool is not reachable from any current teardown, so that guard, and the twice-superseded-id ordering built on top of it, are exercised by constructing the state rather than by a production path.", "Local and daemon providers keep their existing readback; this gate makes no claim about them." ], "demotionRule": "Any failure that shows an exit certified without an observation is a P0 revert of the probe, not a test relaxation; unverifiable answers that merely defer stay experimental while the live SSH journey is missing." diff --git a/fix-reports/19347-round5.md b/fix-reports/19347-round5.md new file mode 100644 index 00000000000..3e898902bb3 --- /dev/null +++ b/fix-reports/19347-round5.md @@ -0,0 +1,186 @@ +# PR #19347 round 5 — exit evidence bound to the incarnation, mint epoch unpublished + +Round 4 found the recovery path and the removal census correct, and three narrow owner-evidence +bugs on top of them. All three are in `src/relay/pty-handler.ts`. F1 and F2 are one root cause. + +## The root cause: the ledger keyed evidence by id, and ids are reusable + +`observedPtyExitIds` answers "did this relay watch the process behind id X end". An id is not a +process. `pty.revive` re-creates a process under an id this relay may already have watched end, so a +single id names a sequence of incarnations, and an entry keyed by the id alone does not say which +one it describes. + +Two transitions were unguarded: + +- **Admission (F1, P0).** `wireAndStore` stored a new record without touching the ledger. While the + new record was in `this.ptys` the probe answered from it, so the stale entry was invisible. The + moment a shutdown that stopped waiting removed that record — the exact case this whole PR exists + for — the old entry became authoritative again and certified a process that never exited. +- **Observation (F2, P1).** The node-pty `onExit` callback returned at `if (managed.disposed)` + before recording. Disposal is bookkeeping: teardown gave up waiting and deleted the record. The + process ending afterwards is still this relay watching this process end, and dropping it left the + id unverifiable forever — the round-3 convergence bug in a narrower ordering that needs no + restart, eviction or revive. + +### The fix + +Admission is the single site where an id changes hands, so it is where ownership is established: + +```ts +// wireAndStore — the one this.ptys.set site +this.observedPtyExitIds.delete(managed.id) +this.ptyIdIncarnationOwners.set(managed.id, managed.incarnationId) +this.ptys.set(managed.id, managed) +``` + +Observation happens before the disposed guard, and only for the incarnation that still speaks for +the id: + +```ts +managed.pty.onExit(({ exitCode }) => { + managed.physicalExit?.markExited() + if (this.stillOwnsPtyId(managed)) { + this.recordObservedPtyExit(managed.id) + } + if (managed.disposed) { + return + } + ... +``` + +`stillOwnsPtyId` is `this.ptys.get(id) === managed || ptyIdIncarnationOwners.peek(id) === incarnationId`. +The redundant write further down the callback was removed; there is now exactly one write per +observation route (`onExit`, and `reapPtyProvenExited` for the ESRCH probe). + +### Why the incarnation map, and not the reviewer's two-clause guard + +The counterfactual in `run-counterfactuals.py` records when `!this.ptys.has(id) || this.ptys.get(id) === managed`. +That closes both reproduced cases and the reviewer said so explicitly — and also said it is not a +complete patch for every repeated-revive/late-callback ordering. It is not, and the gap is the same +class as F1: an empty pool is not evidence that this callback still speaks for the id. Sequence: + +1. incarnation A is admitted under `pty-7`; teardown removes it without observing an exit; +2. `pty.revive` admits incarnation B under `pty-7`; teardown removes B the same way; +3. A's `onExit` finally fires. `this.ptys.has('pty-7')` is false, so the empty-pool clause records — + and the id now belongs to B, whose process is still running. That is a false death certificate + with the admission invalidation in place. + +`this.ptys` can only answer the ownership question while the record is in it, and the observation +this ledger exists for is precisely the one that arrives after it left. So ownership needs to +outlive the record: `ptyIdIncarnationOwners` is a bounded map from id to the incarnation admitted +most recently under it, written at the same single admission site. `peek`, not `get`, so asking does +not renew an owner ahead of older ones. + +Fail-closed in both directions of forgetting. An unrecognised incarnation records nothing, so the id +stays unverifiable; and the owners map is bounded by the same `OBSERVED_PTY_EXIT_HISTORY` entry cap +as the ledger, so eviction can only withhold an exit, never invent one. The live-record disjunct +(`this.ptys.get(id) === managed`) keeps eviction from touching the normal path at all: a PTY that +outlives 4,096 admissions still certifies its own exit, because its record is in the pool when the +callback fires. + +Checked in as `does not certify a revived process with the exit of the one it replaced` (F1), +`certifies an exit node-pty reported after the shutdown wait had given up` (F2) and +`does not let a superseded incarnation certify the id once both records are gone` (the ordering +above), plus the reviewer's revived-id, foreign-epoch and failed-revive cases, in +`pty-handler-liveness-probe.test.ts`; and the two service-level compositions +(`recovers a worker whose exit the relay observed after its shutdown wait gave up`, +`does not retire a revived worker with the exit of the process it replaced`) in +`pty-handler-ssh-recovery-convergence.test.ts`. + +## F3 (P1) — the mint epoch is no longer published + +`pty.getCapabilities` no longer returns `ptyIdMintEpoch`, and the comment inviting list/epoch +inference is gone. Base `9fed61e5c246` published no such field, so this restores base behaviour +exactly; the epoch was added by this PR and read by nobody outside the relay. + +Mixed client and host versions are the normal state, and a host cannot fix an old client — it can +only decline to hand it the second half of an unsound inference. The round-two client's rule was +"id absent from `pty.listProcesses`, and its mint epoch equals the host's published epoch, therefore +exited". Removing the publication makes the second clause unsatisfiable, so that client answers +`null` and defers, which is its own fail-closed path. + +The oracle is checked in without shipping the old client: `pty-handler-ssh-exit-certification.test.ts` +reproduces the inference in ~10 lines against the real handler and asserts it reaches no verdict for +a shutdown-removed id, with and without a sibling kill failure, plus +`publishes nothing that names the generation which minted its PTY ids`, which compares every +published capability value against the epoch parsed from a real id rather than against one field +name. The reviewer's `run-mixed-version.py`, which does load the round-two client source, still runs +and passes unchanged. + +`toRelayPtyIdWithMintEpoch` and `parseRelayPtyMintEpoch` stay — ids are still minted with an epoch +so generations cannot collide — and `src/shared/relay-pty-mint-epoch.ts`'s comments no longer equate +same-epoch omission with an exit. Note the epoch is not secret: `authorityGeneration` in +`pty.inspectProcess` foreground evidence has carried it since before this PR and still does. The +change is that the relay no longer advertises a *contract* whose only purpose was list-absence +inference. + +## F4 (P2) — FIFO eviction is now pinned by a read before the overflow + +The eviction test filled 4,097 entries before its first read, so it never observed a read altering +the next eviction and the `BoundedMap.has()` → `get()` mutation survived the whole gate. +`evicts observations in the order it made them, not the order it was asked` fills exactly +`OBSERVED_PTY_EXIT_HISTORY`, probes the oldest entry, records one more exit and requires the oldest +to be `unverifiable`. The mutant keeps it and fails. + +## P3 — `unknown` → `unverifiable` + +`pty.probeLiveness` now answers `live | exited | unverifiable`, the execution-boundary vocabulary +(`docs/reference/ssh-execution-boundary.md`), which has no synonym for doubt. No released client +consumes this RPC, and the provider maps anything that is not `live` or `exited` to `null` anyway, +so the rename is safe in both directions across versions. Provider, forwarder and runtime +certification tests were renamed with it. + +## Mutations + +Twelve isolated mutations, each run against the six-file reliability command (56 tests). Every one +fails at least one checked-in test; the round-4 survivor is killed. + +| Mutation | Result | First test that fails | +| --- | --- | --- | +| Record the observation in the shared reap | 2/56 fail | separates a real exit from a teardown removal in the same shutdown | +| Drop the disposed guard in `probeLiveness` | 1/56 fails | stays unverifiable while a record is mid-teardown | +| Drop the pending-revive guard | 1/56 fails | stays unverifiable while a revive holds the id | +| Absent record → `exited` | 22/56 fail | stays unverifiable after a shutdown removes a record it never saw exit | +| Provider: unverifiable → `false` | 12/56 fail | maps the owner verdict unverifiable | +| Provider: rejection → `false` | 4/56 fail | fails closed on a relay too old to know the method | +| `BoundedMap.has()` reorders via `get()` | 1/56 fails | evicts observations in the order it made them, not the order it was asked | +| **Drop the admission invalidation** | 2/56 fail | does not certify a revived process with the exit of the one it replaced | +| **Drop the incarnation guard in `onExit`** | 1/56 fails | does not let a superseded incarnation certify the id once both records are gone | +| **Weaken that guard to the empty-pool form** | 1/56 fails | does not let a superseded incarnation certify the id once both records are gone | +| **Move the recording back below the disposed guard** | 2/56 fail | certifies an exit node-pty reported after the shutdown wait had given up | +| **Republish `ptyIdMintEpoch`** | 3/56 fail | publishes nothing that names the generation which minted its PTY ids | + +## Reviewer scripts + +All four pass at this head (run from copies whose evidence directory points at scratch, so the +round-4 archive is not overwritten): + +| Script | Result | Adaptation | +| --- | --- | --- | +| `run-owner-probes.py` | 29 passed, exit 0 | its appended assertions spell doubt `'unknown'`; replaced with `'unverifiable'` for the P3 rename | +| `run-recovery-edge-probes.py` | 7 passed, exit 0 | none | +| `run-mixed-version.py` | 5 passed, exit 0 | none; it still loads the round-two client from `1e89522a9d` | +| `run-counterfactuals.py` | 4 runs, exit 0 each | none needed, but its source patches are now no-ops or identity at this head, so its first two runs measure the shipped code rather than a counterfactual | +| `run-fifo-mutation-proof.py` | superseded by the `has-reorders` row above, which runs against the checked-in suite | | + +## Gates + +| Gate | Result | +| --- | --- | +| `src/relay src/main/providers src/main/runtime src/main/daemon src/main/ipc/pty` | 1,214 files passed, 8 skipped; 12,489 tests passed, 46 skipped | +| Six-file reliability command | 56 passed (was 47) | +| `tsc --noEmit -p config/tsconfig.node.json` / `config/tsconfig.tc.cli.json` | both pass | +| `node config/scripts/check-changed-code-quality.mjs` | 0 findings across 24 changed files (native, type-aware, React Doctor) | +| `node config/scripts/check-reliability-gates.mjs` | 110 gates pass | +| `node config/scripts/check-max-lines-ratchet.mjs` | 12 grandfathered suppressions, no new bypasses | + +## Known gaps + +- The twice-superseded-id ordering is built by constructing a disposed record and letting the + listing sweep it, the same synthetic state the existing suite documents; no current teardown + leaves a disposed record in the pool. +- `ptyIdIncarnationOwners` adds a second bounded map. Both caps are entry counts, not time or + retention guarantees: past 4,096 admissions an older id is forgotten and answers unverifiable, + which defers recovery rather than retiring a worker. +- The epoch removal is proven against the round-two client implementation, not against a public + release; no released client read that capability. diff --git a/src/main/providers/ssh-pty-liveness-probe.test.ts b/src/main/providers/ssh-pty-liveness-probe.test.ts index 688ae044538..52a630a3176 100644 --- a/src/main/providers/ssh-pty-liveness-probe.test.ts +++ b/src/main/providers/ssh-pty-liveness-probe.test.ts @@ -18,7 +18,7 @@ describe('SSH PTY liveness forwarder', () => { it.each([ ['live', true], ['exited', false], - ['unknown', null] + ['unverifiable', null] ])('maps the owner verdict %s', async (status, expected) => { const request = relay({ status }) diff --git a/src/main/providers/ssh-pty-provider-liveness-readback.test.ts b/src/main/providers/ssh-pty-provider-liveness-readback.test.ts index d6d10e967a7..84d4f6e1b96 100644 --- a/src/main/providers/ssh-pty-provider-liveness-readback.test.ts +++ b/src/main/providers/ssh-pty-provider-liveness-readback.test.ts @@ -15,7 +15,7 @@ const APP_PTY_ID = toAppSshPtyId(CONNECTION_ID, RELAY_PTY_ID) * for every SSH id and no SSH worker can ever be certified exited. The verdict itself belongs to * the relay; this class only carries the question to it. */ -function makeProvider(status: 'live' | 'exited' | 'unknown') { +function makeProvider(status: 'live' | 'exited' | 'unverifiable') { const request = vi.fn(async (method: string) => { if (method === 'pty.probeLiveness') { return { status } @@ -32,7 +32,7 @@ function makeProvider(status: 'live' | 'exited' | 'unknown') { describe('SshPtyProvider liveness readback', () => { it('exposes the readback the liveness rule asks the owning provider for', () => { - const { provider } = makeProvider('unknown') + const { provider } = makeProvider('unverifiable') expect(typeof provider.probePtyLiveness).toBe('function') }) diff --git a/src/main/runtime/runtime-legacy-worker-terminal-exit-certification.test.ts b/src/main/runtime/runtime-legacy-worker-terminal-exit-certification.test.ts index d25f7cdc406..315d872b9f8 100644 --- a/src/main/runtime/runtime-legacy-worker-terminal-exit-certification.test.ts +++ b/src/main/runtime/runtime-legacy-worker-terminal-exit-certification.test.ts @@ -175,7 +175,7 @@ describe('legacy worker recovery: certifying that a worker PTY exited', () => { it('defers an SSH candidate the owning relay will not certify', async () => { const { pendingResolutions, deferredDispatchIds } = await reconcile({ inventory: listingWithoutThePty, - isPtyProvenAbsent: provenAbsentViaRelay(async () => ({ status: 'unknown' })) + isPtyProvenAbsent: provenAbsentViaRelay(async () => ({ status: 'unverifiable' })) }) expect(pendingResolutions).toEqual([]) diff --git a/src/relay/pty-handler-liveness-probe.test.ts b/src/relay/pty-handler-liveness-probe.test.ts index 232e37ea3f5..ca5154c8c7c 100644 --- a/src/relay/pty-handler-liveness-probe.test.ts +++ b/src/relay/pty-handler-liveness-probe.test.ts @@ -89,6 +89,31 @@ describe('PtyHandler.probeLiveness', () => { managed.disposed = true } + /** Tear a record down and let the listing sweep it away; nothing watched the process end. */ + async function forgetRecordWithoutObservingItsExit(id: string): Promise { + tearDownRecordWithoutRemovingIt(id) + await dispatcher.callRequest('pty.listProcesses', { includeForegroundProcessEvidence: false }) + } + + /** The state `pty.revive` re-creates a process from, for an id this relay did not mint. */ + function serializedState(id: string): string { + return JSON.stringify([{ id, pid: process.pid, cols: 80, rows: 24, cwd: process.cwd() }]) + } + + /** A shutdown whose sibling refuses to die: the aggregate fails and the relay keeps serving. */ + async function failedShutdownThatKeepsTheRelayServing(): Promise { + const failedKill = vi.fn<() => void>(() => { + throw new Error('host refused kill') + }) + mockPtySpawn.mockReturnValueOnce({ ...mockPtyInstance, kill: failedKill }) + await spawnPty() + const disposal = handler.dispose().catch((error: Error) => error) + await vi.advanceTimersByTimeAsync(8_001) + expect(await disposal).toMatchObject({ message: 'host refused kill' }) + // Let the fixture be cleaned up now that the refusal has been asserted. + failedKill.mockImplementation(() => {}) + } + describe('observed exits', () => { it('answers live while the relay owns a running record', async () => { const { id } = await spawnPty() @@ -117,6 +142,22 @@ describe('PtyHandler.probeLiveness', () => { // the recovery sweep that drives this asks again on every pass. expect(await probe(id)).toBe('exited') }) + + it('certifies an exit node-pty reported after the shutdown wait had given up', async () => { + const { id } = await spawnPty() + const exiting = mockPtyInstance.onExit.mock.calls.at(-1)?.[0] as (event: { + exitCode: number + }) => void + + await failedShutdownThatKeepsTheRelayServing() + + // The record was removed on a timeout, which watched nothing. + expect(await probe(id)).toBe('unverifiable') + exiting({ exitCode: 0 }) + // Disposal is bookkeeping; the callback that follows it is still this relay watching this + // process end, and losing it is what left these workers unverifiable forever. + expect(await probe(id)).toBe('exited') + }) }) describe('bookkeeping removals and states that observed nothing', () => { @@ -127,11 +168,11 @@ describe('PtyHandler.probeLiveness', () => { await disposal expect(handler.activePtyCount).toBe(0) - expect(await probe(id)).toBe('unknown') + expect(await probe(id)).toBe('unverifiable') }) it('stays unverifiable for an id this relay never minted', async () => { - expect(await probe(testPtyId(99))).toBe('unknown') + expect(await probe(testPtyId(99))).toBe('unverifiable') }) it('stays unverifiable for an id another relay generation minted', async () => { @@ -141,13 +182,13 @@ describe('PtyHandler.probeLiveness', () => { // Ledger keys are the whole ids of records this relay held, so no number of exits it has // watched can put another generation's id in it. - expect(await probe('pty2:some-other-generation:1')).toBe('unknown') + expect(await probe('pty2:some-other-generation:1')).toBe('unverifiable') }) it.each(['pty-7', '', 'not-a-pty-id'])( 'stays unverifiable for the id shape %s, which names no generation', async (id) => { - expect(await probe(id)).toBe('unknown') + expect(await probe(id)).toBe('unverifiable') } ) @@ -162,7 +203,7 @@ describe('PtyHandler.probeLiveness', () => { // Both records are gone from the map; only one of them was ever watched ending. expect(await probe(exiting.id)).toBe('exited') - expect(await probe(tornDown.id)).toBe('unknown') + expect(await probe(tornDown.id)).toBe('unverifiable') }) it('keeps a record a shutdown could not kill live, rather than retiring it on paper', async () => { @@ -186,7 +227,7 @@ describe('PtyHandler.probeLiveness', () => { tearDownRecordWithoutRemovingIt(id) // The pid is this test process, so without the guard the record reads as a live PTY. - expect(await probe(id)).toBe('unknown') + expect(await probe(id)).toBe('unverifiable') }) it('stays unverifiable after the listing sweeps a torn-down record away', async () => { @@ -196,7 +237,7 @@ describe('PtyHandler.probeLiveness', () => { await dispatcher.callRequest('pty.listProcesses', { includeForegroundProcessEvidence: false }) expect(handler.activePtyCount).toBe(0) - expect(await probe(id)).toBe('unknown') + expect(await probe(id)).toBe('unverifiable') }) it('never certifies an exit from a worktree removal it could not prove', async () => { @@ -219,25 +260,32 @@ describe('PtyHandler.probeLiveness', () => { // The revive is about to put a different process on this id, so the observation it would // otherwise be certified from describes a process that no longer occupies it. const revived = dispatcher.callRequest('pty.revive', { state }) - expect(await probe(id)).toBe('unknown') + expect(await probe(id)).toBe('unverifiable') await revived expect(await probe(id)).toBe('live') }) - it('forgets its oldest observation at the ledger cap instead of growing without bound', async () => { + it('evicts observations in the order it made them, not the order it was asked', async () => { let oldest = '' let newest = '' - for (let index = 0; index <= OBSERVED_PTY_EXIT_HISTORY; index++) { + for (let index = 0; index < OBSERVED_PTY_EXIT_HISTORY; index++) { const { id } = await spawnPty() reportExitOfLatestPty() oldest ||= id newest = id } + // Asking about the oldest entry at the cap must not renew it: retention is observation + // order, so the next observation still pushes exactly this one out. + expect(await probe(oldest)).toBe('exited') expect(await probe(newest)).toBe('exited') + const overflowing = await spawnPty() + reportExitOfLatestPty() + + expect(await probe(overflowing.id)).toBe('exited') // Evicted, not remembered as absent: a forgotten observation is unverifiable, never exited. - expect(await probe(oldest)).toBe('unknown') + expect(await probe(oldest)).toBe('unverifiable') }) it('forgets every observation when the relay restarts, even under the same mint epoch', async () => { @@ -250,10 +298,80 @@ describe('PtyHandler.probeLiveness', () => { const restartedDispatcher = createMockDispatcher() const restarted = createTestPtyHandler(restartedDispatcher) try { - expect(await probe(id, restartedDispatcher)).toBe('unknown') + expect(await probe(id, restartedDispatcher)).toBe('unverifiable') } finally { await restarted.dispose({ waitForPhysicalExit: false }).catch(() => {}) } }) }) + + describe('incarnations sharing one id, which `pty.revive` re-creates a process under', () => { + it.each(['pty-7', 'pty2:previous-generation:99'])( + 'certifies the exit of the process it revived under %s, and only for itself', + async (id) => { + await dispatcher.callRequest('pty.revive', { state: serializedState(id) }) + expect(await probe(id)).toBe('live') + + reportExitOfLatestPty() + + expect(await probe(id)).toBe('exited') + // Reviving is what put the id in this relay's hands; whatever its shape, it names nothing + // on a relay that never held it. + const otherDispatcher = createMockDispatcher() + const other = createTestPtyHandler(otherDispatcher) + try { + expect(await probe(id, otherDispatcher)).toBe('unverifiable') + } finally { + await other.dispose({ waitForPhysicalExit: false }).catch(() => {}) + } + } + ) + + it('stays unverifiable for an id whose revive never created a record', async () => { + mockPtySpawn.mockImplementationOnce(() => { + throw new Error('spawn refused') + }) + + await expect( + dispatcher.callRequest('pty.revive', { state: serializedState('pty-77') }) + ).rejects.toThrow('spawn refused') + + expect(await probe('pty-77')).toBe('unverifiable') + }) + + it('does not certify a revived process with the exit of the one it replaced', async () => { + const id = 'pty-7' + await dispatcher.callRequest('pty.revive', { state: serializedState(id) }) + reportExitOfLatestPty() + expect(await probe(id)).toBe('exited') + + // Admitting a second incarnation clears that observation: it described the process this one + // replaced, so the id starts again with nothing to certify from. + await dispatcher.callRequest('pty.revive', { state: serializedState(id) }) + expect(await probe(id)).toBe('live') + + await failedShutdownThatKeepsTheRelayServing() + + // The revived record left the pool on a timeout, and its process is still running. + expect(() => process.kill(process.pid, 0)).not.toThrow() + expect(await probe(id)).toBe('unverifiable') + }) + + it('does not let a superseded incarnation certify the id once both records are gone', async () => { + const id = 'pty-7' + await dispatcher.callRequest('pty.revive', { state: serializedState(id) }) + const supersededExit = mockPtyInstance.onExit.mock.calls.at(-1)?.[0] as (event: { + exitCode: number + }) => void + await forgetRecordWithoutObservingItsExit(id) + await dispatcher.callRequest('pty.revive', { state: serializedState(id) }) + await forgetRecordWithoutObservingItsExit(id) + + // The first process really did end. The id belongs to a later one that nothing watched end, + // so an empty pool is not permission for this callback to answer for it. + supersededExit({ exitCode: 0 }) + + expect(await probe(id)).toBe('unverifiable') + }) + }) }) diff --git a/src/relay/pty-handler-ssh-exit-certification.test.ts b/src/relay/pty-handler-ssh-exit-certification.test.ts index dd676335c41..dfbdfc42352 100644 --- a/src/relay/pty-handler-ssh-exit-certification.test.ts +++ b/src/relay/pty-handler-ssh-exit-certification.test.ts @@ -60,24 +60,62 @@ describe('SSH exit certification across relay shutdown', () => { await endPtyHandlerTest(handler, originalPlatform) }) - async function capabilities(): Promise<{ ptyIdMintEpoch?: unknown }> { - return (await dispatcher.callRequest('pty.getCapabilities', {})) as { ptyIdMintEpoch?: unknown } + async function capabilities(): Promise> { + return (await dispatcher.callRequest('pty.getCapabilities', {})) as Record } - it('names the generation that minted its PTY ids', async () => { + /** + * The rule an older client applied, reproduced here rather than shipped: an id missing from + * `pty.listProcesses` was an exit whenever its mint epoch matched one the host published. Mixed + * client and host versions are the normal state (docs/reference/remote-wire-compatibility.md), + * so what this host publishes has to keep that client unable to reach a verdict — a host cannot + * fix an old client, only decline to hand it the second half of the inference. + */ + async function inferLivenessFromListingAndEpoch(relayPtyId: string): Promise { + const listed = (await dispatcher.callRequest('pty.listProcesses', { + includeForegroundProcessEvidence: false + })) as { id?: unknown }[] + if (listed.some((session) => session.id === relayPtyId)) { + return true + } + const mintEpoch = parseRelayPtyMintEpoch(relayPtyId) + if (!mintEpoch) { + return null + } + return Object.values(await capabilities()).includes(mintEpoch) ? false : null + } + + it('publishes nothing that names the generation which minted its PTY ids', async () => { const { id } = await spawnPty() const mintEpoch = parseRelayPtyMintEpoch(id) expect(mintEpoch).toBeTruthy() - expect((await capabilities()).ptyIdMintEpoch).toBe(mintEpoch) + expect(Object.values(await capabilities())).not.toContain(mintEpoch) }) - it('keeps that generation stable across ids and reads', async () => { - const first = await spawnPty() - const second = await spawnPty() + it('leaves a shutdown-removed id unverifiable for a client that infers from listings', async () => { + const { id } = await spawnPty() + const disposal = handler.dispose() + await vi.advanceTimersByTimeAsync(8_001) + await disposal - expect(parseRelayPtyMintEpoch(second.id)).toBe(parseRelayPtyMintEpoch(first.id)) - expect((await capabilities()).ptyIdMintEpoch).toBe((await capabilities()).ptyIdMintEpoch) + expect(await inferLivenessFromListingAndEpoch(id)).toBeNull() + }) + + it('keeps that inference unreachable when a failed kill leaves the relay serving', async () => { + const { id } = await spawnPty() + const failedKill = vi.fn<() => void>(() => { + throw new Error('host refused kill') + }) + mockPtySpawn.mockReturnValueOnce({ ...mockPtyInstance, kill: failedKill }) + await spawnPty() + const disposal = handler.dispose().catch((error: Error) => error) + await vi.advanceTimersByTimeAsync(8_001) + expect(await disposal).toMatchObject({ message: 'host refused kill' }) + failedKill.mockImplementation(() => {}) + + expect(() => process.kill(process.pid, 0)).not.toThrow() + expect(await inferLivenessFromListingAndEpoch(id)).toBeNull() }) it('does not certify exit after shutdown bookkeeping removes an unexited PTY', async () => { const { id } = await spawnPty() diff --git a/src/relay/pty-handler-ssh-recovery-convergence.test.ts b/src/relay/pty-handler-ssh-recovery-convergence.test.ts index 5a1c993827c..4cf750cde8b 100644 --- a/src/relay/pty-handler-ssh-recovery-convergence.test.ts +++ b/src/relay/pty-handler-ssh-recovery-convergence.test.ts @@ -149,4 +149,61 @@ describe('SSH recovery convergence', () => { db.close() } }) + it('recovers a worker whose exit the relay observed after its shutdown wait gave up', async () => { + const { id } = (await dispatcher.callRequest('pty.spawn', {})) as { id: string } + const exit = mockPtyInstance.onExit.mock.calls.at(-1)![0] as (event: { + exitCode: number + }) => void + const failedKill = vi.fn<() => void>(() => { + throw new Error('host refused kill') + }) + mockPtySpawn.mockReturnValueOnce({ ...mockPtyInstance, kill: failedKill }) + await dispatcher.callRequest('pty.spawn', {}) + const disposal = handler.dispose().catch((error: Error) => error) + await vi.advanceTimersByTimeAsync(8001) + expect(await disposal).toMatchObject({ message: 'host refused kill' }) + failedKill.mockImplementation(() => {}) + const appId = toAppSshPtyId(connection, id) + expect(await probePtyLivenessFromRuntimeController(deps, appId)).toBeNull() + + exit({ exitCode: 0 }) + + // The owner watched this process end, after the timeout that removed its record. Recovery has + // to see that, or the sweep defers this worker forever. + const result = await candidateResult(appId) + expect(result.pendingResolutions).toEqual([ + { candidate: { dispatchId: 'review-worker', ptyId: appId }, resolution: 'exited' } + ]) + }) + it('does not retire a revived worker with the exit of the process it replaced', async () => { + const id = 'pty-7' + const state = JSON.stringify([{ id, pid: process.pid, cols: 80, rows: 24, cwd: process.cwd() }]) + await dispatcher.callRequest('pty.revive', { state }) + const supersededExit = mockPtyInstance.onExit.mock.calls.at(-1)![0] as (event: { + exitCode: number + }) => void + supersededExit({ exitCode: 0 }) + await dispatcher.callRequest('pty.revive', { state }) + const appId = toAppSshPtyId(connection, id) + expect(await probePtyLivenessFromRuntimeController(deps, appId)).toBe(true) + const failedKill = vi.fn<() => void>(() => { + throw new Error('host refused kill') + }) + mockPtySpawn.mockReturnValueOnce({ ...mockPtyInstance, kill: failedKill }) + await dispatcher.callRequest('pty.spawn', {}) + const exitsBefore = dispatcher._notifications.filter((n) => n.method === 'pty.exit').length + const disposal = handler.dispose().catch((error: Error) => error) + await vi.advanceTimersByTimeAsync(8001) + expect(await disposal).toMatchObject({ message: 'host refused kill' }) + failedKill.mockImplementation(() => {}) + + // Nothing watched the revived process end, and its pid is this live test process. + expect(() => process.kill(process.pid, 0)).not.toThrow() + expect(dispatcher._notifications.filter((n) => n.method === 'pty.exit')).toHaveLength( + exitsBefore + ) + const result = await candidateResult(appId) + expect(result.pendingResolutions).toEqual([]) + expect([...result.deferredDispatchIds]).toEqual(['review-worker']) + }) }) diff --git a/src/relay/pty-handler.ts b/src/relay/pty-handler.ts index 9c0bb923ad0..9a98a2176c7 100644 --- a/src/relay/pty-handler.ts +++ b/src/relay/pty-handler.ts @@ -551,10 +551,27 @@ export class PtyHandler { * fail-closed direction, where persisting it would have to survive the ambiguity of the crash * that lost the records. Bounded for the same reason: an evicted id answers unverifiable * (docs/reference/ssh-execution-boundary.md). + * + * An entry describes ONE incarnation. `pty.revive` re-creates a process under an id this relay + * already watched end, so admission clears the id's entry and only the incarnation that currently + * owns the id may write one; see {@link ptyIdIncarnationOwners}. */ private readonly observedPtyExitIds = new BoundedMap({ maxEntries: OBSERVED_PTY_EXIT_HISTORY }) + /** + * The incarnation that most recently took each id. `this.ptys` answers that question only while + * the record is still in the pool, and the observation this ledger needs can arrive after it left + * — a shutdown that stopped waiting removes the record, and the process ends afterwards. Without + * this, that late callback could not be told apart from one belonging to a process a `pty.revive` + * has since replaced under the same id, and would certify the replacement's exit. + * + * Bounded like the ledger, and forgetting fails closed: an unrecognized incarnation records + * nothing, so the id stays unverifiable. + */ + private readonly ptyIdIncarnationOwners = new BoundedMap({ + maxEntries: OBSERVED_PTY_EXIT_HISTORY + }) private creationFenced = false private pendingCreationDrainResolvers = new Set<() => void>() private worktreeRemovalCoordinator: RelayPtyWorktreeRemovalCoordinator | null = null @@ -792,6 +809,18 @@ export class PtyHandler { this.observedPtyExitIds.set(id, true) } + /** + * Whether this record still speaks for its id. An exit observed for a superseded incarnation is + * evidence about the process that ended, never about the one occupying the id now. `peek` rather + * than `get` so asking does not extend an owner's retention past older ones. + */ + private stillOwnsPtyId(managed: ManagedPty): boolean { + return ( + this.ptys.get(managed.id) === managed || + this.ptyIdIncarnationOwners.peek(managed.id) === managed.incarnationId + ) + } + // Why: the sole removal path, so the three exit routes can't drift on who announces an empty pool. private removePty(id: string): void { this.ptys.delete(id) @@ -968,6 +997,11 @@ export class PtyHandler { /** Wire onData/onExit listeners for a managed PTY and store it. */ private wireAndStore(managed: ManagedPty): void { managed.physicalExit = new PhysicalExitTracker() + // Why here: this is the single admission site, so it is where an id changes hands. A new + // incarnation inherits no exit evidence — the observation described the process it replaced — + // and becomes the only one whose exit may be recorded for this id. + this.observedPtyExitIds.delete(managed.id) + this.ptyIdIncarnationOwners.set(managed.id, managed.incarnationId) this.ptys.set(managed.id, managed) // Why: a PTY joining the pool under this paneKey means the surface exists again (reopened pane // or revive), so a prior retirement no longer describes anything and must not mute its hooks. @@ -1034,6 +1068,12 @@ export class PtyHandler { }) managed.pty.onExit(({ exitCode }: { exitCode: number }) => { managed.physicalExit?.markExited() + // Why before the disposed guard: this callback IS the observation. Teardown that stopped + // waiting removed the record and marked it disposed, but the process ending afterwards is + // still this relay watching it end, and dropping it left the worker unverifiable forever. + if (this.stillOwnsPtyId(managed)) { + this.recordObservedPtyExit(managed.id) + } if (managed.disposed) { return } @@ -1066,7 +1106,6 @@ export class PtyHandler { this.publishPendingExit(managed.id) this.notifyExitListener(managed) this.agentSessionOwners.release(managed.id) - this.recordObservedPtyExit(managed.id) this.removePty(managed.id) this.clearPtyInputState(managed.id) // Why: release the ptmx fd on natural exit, else the master fd leaks until GC (docs/fix-pty-fd-leak.md). @@ -1120,11 +1159,10 @@ export class PtyHandler { agentSessionCreateOperationVersion: AGENT_SESSION_CREATE_OPERATION_PROTOCOL_VERSION, // Additive capability: clients may request the no-process-table inventory // projection and consume fenced inspect evidence on this host. - foregroundProcessEvidenceVersion: 1, - // Additive: names the generation that minted this relay's ids, so a client can read an id's - // absence from `pty.listProcesses` as an exit this host observed rather than as a restart. - // A relay that omits it leaves every absence unverifiable, which is the shipped behaviour. - ptyIdMintEpoch: this.ptyIdMintEpoch + foregroundProcessEvidenceVersion: 1 + // Deliberately publishes no mint epoch. Pairing one with `pty.listProcesses` is how a client + // used to read an id's absence as an exit, and absence is also what a shutdown that stopped + // waiting for an uninterruptible child leaves behind. `pty.probeLiveness` is the only answer. })) this.dispatcher.onRequest('pty.probeLiveness', async (params) => this.probeLiveness(params)) this.dispatcher.onRequest('pty.listProcesses', (params) => this.listProcesses(params)) @@ -2643,22 +2681,23 @@ export class PtyHandler { * (docs/reference/ssh-execution-boundary.md). * * `exited` requires an observation — a live record whose pid probes absent, or an id in - * {@link observedPtyExitIds}. Everything else is `unknown`: an id this relay never held or never - * saw end, a revive in flight, and a record mid-teardown. + * {@link observedPtyExitIds}. Everything else is `unverifiable`, the execution boundary's word + * for doubt: an id this relay never held or never saw end, a revive in flight, and a record + * mid-teardown. */ private async probeLiveness( params: Record - ): Promise<{ status: 'live' | 'exited' | 'unknown' }> { + ): Promise<{ status: 'live' | 'exited' | 'unverifiable' }> { const id = typeof params.id === 'string' ? params.id : '' const managed = this.ptys.get(id) if (managed) { if (managed.disposed) { - return { status: 'unknown' } + return { status: 'unverifiable' } } return { status: this.reapPtyProvenExited(managed) ? 'exited' : 'live' } } if (this.pendingReviveIds.has(id)) { - return { status: 'unknown' } + return { status: 'unverifiable' } } // The ledger is the only gate, deliberately. A blanket "shutdown has started" refusal would // also mask a wrong entry in it, since teardown only ever runs behind that fence — and it @@ -2666,7 +2705,7 @@ export class PtyHandler { // held: ids minted with this generation's epoch, plus any id `pty.revive` re-created a process // under. An id another generation minted and this one never revived is therefore absent, and // absent is unverifiable. - return { status: this.observedPtyExitIds.has(id) ? 'exited' : 'unknown' } + return { status: this.observedPtyExitIds.has(id) ? 'exited' : 'unverifiable' } } private async hasChildProcesses(params: Record): Promise { diff --git a/src/shared/relay-pty-mint-epoch.ts b/src/shared/relay-pty-mint-epoch.ts index a6fbd854365..77b140c169c 100644 --- a/src/shared/relay-pty-mint-epoch.ts +++ b/src/shared/relay-pty-mint-epoch.ts @@ -1,10 +1,13 @@ /** - * A relay PTY id carries the mint epoch of the relay process that allocated it, so a client can - * tell the two halves of "this relay does not list that id" apart: the relay minted it and no - * longer has it, which is an exit the host observed, versus the relay never had it, which is every - * id minted before a restart and is evidence of nothing (docs/reference/ssh-execution-boundary.md). + * A relay PTY id carries the mint epoch of the relay process that allocated it, so ids from + * different generations of the same relay cannot collide. The shape lives here so the relay that + * mints it and anything that reads it back cannot drift. * - * The id shape lives here so the relay that mints and the client that reads it cannot drift. + * The epoch certifies nothing on its own, and this relay no longer publishes it. Pairing it with + * `pty.listProcesses` used to read an id's absence from a same-epoch listing as an exit, which is + * unsound: the relay also removes a record it never watched end, so a shutdown that gave up waiting + * for an uninterruptible child produced that same absence. Only the owner's exact-id + * `pty.probeLiveness` answers whether a process ended (docs/reference/ssh-execution-boundary.md). */ const MINT_EPOCH_PTY_ID_PREFIX = 'pty2:' @@ -12,7 +15,7 @@ export function toRelayPtyIdWithMintEpoch(mintEpoch: string, sequence: number): return `${MINT_EPOCH_PTY_ID_PREFIX}${encodeURIComponent(mintEpoch)}:${sequence}` } -/** Null for a legacy `pty-N` id, which names no epoch and so can never certify an exit. */ +/** Null for a legacy `pty-N` id, which names no generation. */ export function parseRelayPtyMintEpoch(relayPtyId: string): string | null { if (!relayPtyId.startsWith(MINT_EPOCH_PTY_ID_PREFIX)) { return null