Repository navigation
Conversation
…the relay `updateTaskStatus` ran its live-supervised-worker guard only for terminal statuses, so `task-update --status dispatched` re-opened a Task under a worker that was already stopping. The worker's own report then had nowhere to land: settlement transitioned the worker from a hardcoded `ready`, so it threw `lifecycle_conflict` out of the federated relay import (rolling back the batch and freezing `to_home_imported_sequence`, which the contiguity check then used to refuse every later sequence forever) and out of the coordinator's message sweep (marking the run failed with its batch unread). Three edges, one rule each: - every status the Task guard lets past the active-Dispatch check now clears the same live-worker check, so a genuine re-open cannot happen under a live worker; - settlement reads the worker's actual state and asks the lifecycle graph, then rejects with `worker_not_settleable` instead of throwing, so already-wedged databases drain rather than stall; - `beginWorkerStop` accepts a re-issue from `stopping`, giving the operator a legal exit whose outcome is the honest `stop_unknown` rather than an asserted exit. The state-space test drives the real public operations and proves no reachable (Dispatch, Task, worker) triple can make a worker report throw.
…a listing Legacy worker terminal recovery read `exited` straight off a PTY id missing from one controller inventory. `listProcesses` returns the host's CURRENT session map, so a restarted relay omits every id the previous one minted and a daemon session this process never attached was never cached at all — neither shell necessarily died. On that inference recovery abandoned the worker, failed and circuit-broke its dispatch, closed its questions and reset the Task to ready, so the same Task could be dispatched twice while the original agent kept running. All three listing-omission branches now ask the runtime's existing absence resolver, which routes to the provider that owns the PTY and answers absent only on its readback. Doubt defers to the next sweep instead of certifying an exit, which is what the sibling inventory refresh already does for a restarted relay. The recovery fixtures asserted exits from an empty listing alone; they now supply the owner readback their test names already claimed.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds relay-owned PTY liveness checks to legacy worker recovery. Unverifiable PTY absence now defers reconciliation. Relay PTY ids include mint epochs, and observed exits are tracked for exact liveness responses. Worker settlement now validates lifecycle transitions, blocks task reopening while supervised workers stop, and distinguishes stranded stops by runtime epoch. New tests cover PTY recovery, relay behavior, lifecycle state exploration, stopping workers, federated reports, and repeated stop requests. Merge Risk: 🟠 High · up to The relay can retain oversized PTY identifiers and may certify stale exit evidence, risking relay exhaustion or incorrect worker retirement. Recovery tests can also mask unverifiable PTYs as exited. These issues should be fixed before merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed context, changes, rationale, tests, and gate results. However, it omits required template sections and information, including an explicit Resolution Add the missing template sections. Provide an explicit linked issue using Full details: Docstring CoverageExplanation Docstring coverage is 44.74% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 38 functions across 25 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Worker-report settlement —
settleWorkerReportInTransactionnow asks the lifecycle graph viaisLegalLifecycleTransitionbefore writing and returns a structuredworker_not_settleablerejection instead of throwing; the worker transition'sfromis the worker's actual state, not a hardcoded'ready'. - Task re-open guard —
updateTaskStatusruns the live-supervised-worker check on a genuine re-open todispatched, keeping a no-op re-assert legal. - Stop re-issue escape —
beginWorkerStopaccepts a re-issue fromstopping, completing thestopping→stop_unknown→abandonoperator chain. - Legacy exit certification — all three listing-omission branches in legacy recovery now defer unless the owning provider's readback (
isPtyProvenAbsent) certifies absence. - Tests — a BFS state-space settlement proof, targeted stopping-worker settlement tests, and legacy exit-certification tests, plus updated recovery fixtures.
I traced the load-bearing paths behind the change and found the logic sound. The worker lifecycle edges reused by the new guard are pre-existing in LEGAL_TRANSITIONS, so no new reachable state is introduced; settlement runs inside a single transaction, so the reportingWorker.state read is stable through the write; and neither importFederatedRelayItem nor the coordinator sweep calls updateTaskStatus('dispatched'), so the stricter throw cannot wedge a relay batch. The worker_not_settleable code is string-typed on the wire and is parsed defensively (typeof code === 'string'), so it's safe for mixed-version clients. The pre-existing-wedge-row case (task dispatched under a stopping worker written by an older binary) is correctly handled by rejection rather than a throw.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
… left green A Codex review of #19347 found two mutants the new tests did not kill. Restoring settlement's hardcoded `ready`/`start_unknown` precondition left all ten lifecycle tests green: every old-database case reported success, which the graph guard rejects before the transition runs. The graph does accept stopping -> failed, so a failed report on that same persisted triple must settle; under the hardcoded precondition it throws and wedges the relay batch again. Restoring the unconditional `exited` on the pre-adoption omission branch left the certification suite green: it covered the initial and final listings but not the re-list inside the adoption mutation, which is exactly where a relay restart drops ids whose shells are still running. Also narrows the state-space test's own claim. It adds the missing `taskUpdate:pending` operation and replaces the "state-space proof" wording with what the search actually enumerates: one federated dispatch, settlement called directly rather than through its two batch callers, a memo key of three statuses, and a depth-five cutoff it does not re-derive.
…lers The comment justified the coalescing by a DB refusal that no longer happens: `beginWorkerStop` accepts a re-issue from `stopping`, since that is the operator's escape. The mechanism is now more load-bearing, not less — without it both callers close the same terminal and race to settle a row only one of them can still find — so say that instead of the stale reason.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since the prior review (4caaa6c test + cb0657a docs) widens regression coverage and clarifies docs; there is no production-code change.
- State-space search scope — added a
taskUpdate:pendingoperation so the BFS also reaches task states with no active dispatch, and rewrote the block comment to spell out exactly what the search enumerates and what it leaves to the targeted tests. - Failed-report settlement — added a regression test proving a
failedworker report settles from astoppingworker, the other half of thefrom: reportingWorker.stateguard that the reject case already covered. - Pre-adoption refresh deferral — added a legacy-recovery test for the pre-adoption re-list losing the PTY without an owner absence proof (relay restart between the plan inventory and adoption).
dedupeWorkerStopdoc — corrected the JSDoc to match the now-accepted re-issue fromstopping.
I ran the three changed test files (17 tests; the state-space BFS finishes in ~17s against a 300s timeout). The added tests fail on unpatched main and pass here, so they are real regression coverage rather than theatre. The stopping → failed (legal) versus stopping → succeeded (illegal) split is confirmed against LEGAL_TRANSITIONS in lifecycle-transition.ts, and both comment corrections state the behavior accurately.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 7947cf88-9d80-4ad6-a7c6-be51e919ca05
📒 Files selected for processing (15)
src/main/runtime/orca-runtime-preserved-branch-cleanup.tssrc/main/runtime/orca-runtime-test-scenario-builders.spec.tssrc/main/runtime/orca-runtime-tests/terminal-output-and-worker-recovery-part-04.spec.tssrc/main/runtime/orchestration/db-lifecycle-state-space-settlement.test.tssrc/main/runtime/orchestration/db-stopping-worker-report-settlement.test.tssrc/main/runtime/orchestration/db/dispatch-context/worker-report-settlement.tssrc/main/runtime/orchestration/db/lifecycle-transition.tssrc/main/runtime/orchestration/db/tasks/task-status-transition.tssrc/main/runtime/orchestration/db/worker-dispatch/worker-dispatch-stop.tssrc/main/runtime/orchestration/lifecycle-reconciliation.tssrc/main/runtime/orchestration/types.tssrc/main/runtime/rpc/methods/orchestration/worker/worker-stop.tssrc/main/runtime/runtime-legacy-worker-terminal-exit-certification.test.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-candidate.tssrc/main/runtime/runtime-legacy-worker-terminal-recovery-types.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| getForegroundProcess: async () => null, | ||
| hasPty, | ||
| // Absence is only an exit when the owning provider says so; these fixtures own no other PTY. | ||
| probePtyLiveness: async (candidate: string) => hasPty(candidate) === true, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve unknown PTY liveness
When a harness caller supplies null, return hasPty(candidate) unchanged. The current comparison converts null to false, and the recovery path records that result as proven PTY absence instead of unknown liveness.
Legacy worker recovery now retires a worker only on the owning provider's readback, but `SshPtyProvider` had none, so the runtime controller answered null for every `ssh:` id and absence was permanently unverifiable. An SSH worker whose shell died while Orca was closed has no exit event and recovery is its only settler, so it deferred on a 30 s backoff forever: the pane stayed fenced, the dispatch stayed dispatched, and the Task could not be re-opened. Removing a false `exited` had left no path to a true one. The relay can answer, because a PTY id already carries the mint epoch of the relay process that allocated it. `pty.listProcesses` is that relay's live set: it reaps a torn-down record and re-probes the pid before reporting. So an unlisted id whose mint epoch is the relay's current one is an exit this host observed, while an unlisted id from any other epoch is the restart case that was never evidence. `pty.getCapabilities` gains the epoch as an additive field so the comparison holds even when the relay now lists nothing, which is the reported case; a relay that omits it leaves every absence unverifiable, as today. Deliberately not `pty.attach`, the only refusal carrying the proven-exited marker: a successful attach opens a delivery, retires the previous one, and restores retired pane surfaces, so probing with it would disturb a live consumer in precisely the case where the answer is "live". The certification suite's PTY ids were not parseable SSH ids, so it claimed coverage of the population that regressed and drove none of it. They are real ids now, with two cases running the recovery branch through this probe.
Accepting `stopping` from any caller re-opened the window the worker-stop coalescing exists to close. A second pass over this runtime's own in-flight stop re-inspects a terminal that is already going away and records `stop_unknown`. The exit event that follows claims a clean stop only from `stopping` under its own epoch, so from `stop_unknown` it falls through to a dispatch failure with `workerProcessExited` and posts an "Agent exited unexpectedly" escalation: the operator's own stop reported as a crash. The escape was only ever justified for a row whose owning runtime died, and the runtime id is a per-process value, so the epoch names exactly that population. `failActiveDispatchOnExit` already draws the same line to decide whose stop an exit may settle; both sides now use it, so they cannot disagree about whose stop is in flight.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since the prior review (6dea9aa) is the SSH half of the P1 fix: SshPtyProvider gains a probePtyLiveness readback and the relay publishes its ptyIdMintEpoch, so a legacy SSH worker can finally be certified exited by its own owner instead of deferring forever.
- SSH liveness readback —
SshPtyProvider.probePtyLivenessdelegates to a newprobeSshPtyLiveness, which answerslivewhen the relay lists the id,exitedonly when the same relay that minted it (matchedptyIdMintEpoch) no longer lists it, andnullotherwise. - Relay mint-epoch capability —
pty.getCapabilitiesgains an additiveptyIdMintEpochfield, and id minting is extracted into a sharedtoRelayPtyIdWithMintEpochwhose output is byte-identical to the priorpty2:<epoch>:<seq>format. - Shared epoch module —
relay-pty-mint-epoch.tskeeps the id shape in one place so the minting relay and the parsing client cannot drift. - Recovery integration tests — the legacy recovery suite now drives a real SSH id and asserts both certification (same-epoch absence) and deferral (a restarted relay that merely disowns the id).
I traced the load-bearing claims behind the readback: the epoch is a per-process randomUUID() (production constructs PtyHandler without a persisted epoch), pty.listProcesses reaps disposed/proven-exited PTYs before answering so the listing is genuinely the live set, includeForegroundProcessEvidence: false skips the host process-table work without skipping the reap, and the probe never throws — every doubt case (legacy pty-N id, missing epoch, epoch mismatch, transport failure, non-array listing) resolves to null. An old relay that omits ptyIdMintEpoch leaves every absence unverifiable, preserving the shipped mixed-version behavior. Ran the four changed/new test files (29 tests) — all pass.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
The SSH liveness probe read "absent from pty.listProcesses and minted by this relay's generation" as an observed exit. That implication is false. Relay shutdown waits eight seconds for a physical exit, catches the timeout, and deletes the record anyway; a sibling PTY's failed kill rejects the aggregate, the grace lifecycle defers shutdown, and the relay keeps serving under the same mint epoch. A provider that never attached the PTY then received a death certificate for a running process, and recovery queued its worker as exited. The census of every record removal splits cleanly: node-pty's onExit and the ESRCH pid probe watched the process end; the shutdown teardown and the torn-down-record sweep did not. Only the first two now record the id, at the line that made the observation rather than in the shared reap they both pass through. pty.probeLiveness answers an exact id from that state: a live record, or exited when its pid probes absent or the id is in the ledger. A record mid-teardown, a revive in flight, and every id the relay never watched end are unknown. It fails closed, so a removal path added later without a record defers instead of certifying. SshPtyProvider forwards and maps three statuses; an older relay answers -32601, which arrives as a rejection and maps to null, so no capability negotiation is needed. Two client-side gates went with it. The mint-epoch comparison is subsumed by ledger keys that already embed the generation, and a blanket shutdown-fenced refusal only masked the ledger in the one path where teardown runs.
Reverts 6dea9aa and 1dcae18. The relay-owned probe those commits landed is correct, but it is a relay lifecycle change with its own census, capability surface, and shutdown semantics, and it does not belong in a dispatch-lifecycle PR. It moves to fix/ssh-pty-liveness-owner. What this PR keeps is the safety half: recovery never certifies an exit from a listing omission. SSH legacy workers therefore answer unverifiable and defer, exactly as `remote:` ids already do, because no SSH provider implements the owner readback. That is an availability cost, not a correctness one — a worker whose shell really died is cleared by worker-stop, stop_unknown, then worker-abandon, which this PR makes reachable. Retiring it on a listing omission is what orphaned live agents. The certification suite keeps its parseable SSH ids and gains a case naming the deferral, so the limitation is documented where someone will find it.
The dedupe docstring was rewritten before the epoch gate landed and now says the opposite of what happens: both coalesced callers are in one runtime and share one epoch, so `beginWorkerStop` refuses the second. The coalescing still earns its place by keeping one of them from closing the terminal, which is what it now says. The state-space floor stayed at 50 after `taskUpdate:pending` grew the reachable set from 59 to 71, so the harness-integrity guard tolerated losing a third of the space in silence. Raised to just under the measured count.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since 1e89522 reverts the SSH liveness readback out of the PR, leaving the P0 dispatch fix plus the P1 safety half, and corrects two comments.
- SSH liveness readback removed —
f0f8858964reverts both the relay-ownedpty.probeLivenessledger (1dcae188bf) and the earlierSshPtyProvider.probePtyLiveness/ mint-epoch capability, restoringpty-handler.tsandssh-pty-provider.tsto byte-identical withmain. SSH legacy workers now answer unverifiable and defer, exactly likeremote:ids, and are cleared through the now-reachable stop re-issue →stop_unknown→abandonchain. - Stop re-issue doc corrected —
0be1813781rewrites thededupeWorkerStopdocstring to match the epoch-scoped re-issue (same-runtime callers coalesce; the accepted epoch belongs only to a row a dead runtime stranded). - State-space floor raised — the BFS integrity floor moves to the measured count so a real regression in coverage is visible.
- SSH deferral documented — the recovery cert suite keeps its parseable SSH ids and names the "no owner can certify" deferral case in a comment, recording the limitation next to the code that enforces it.
I verified the revert is clean rather than assuming it: git diff --merge-base main on pty-handler.ts, ssh-pty-provider.ts, and the deleted relay-pty-mint-epoch.ts / ssh-pty-liveness-probe.ts modules is empty, and a repo-wide grep for the removed symbols turns up only the pre-existing pty-handler.ts mint-epoch field (used as authorityGeneration) and two accurate comments. SshPtyProvider omits the optional probePtyLiveness from IPtyProvider, so probePtyLivenessFromRuntimeController returns null for SSH and resolvePtyAbsentFromListing defers to the next sweep — the intended safety behavior rather than a dangling reference.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
This reverts commit f0f8858.
The probe answers `exited` from an observation alone, so the safety argument is the census of every path that makes a record disappear and whether that path watched anything. Three do not: the relay-shutdown teardown that stopped waiting for an uninterruptible child, the listing's torn-down-record sweep, and a worktree removal whose kill nothing confirmed. Each now has a test asserting the id stays unverifiable afterwards, alongside the states that observed nothing either — a revive holding the id while it puts a different process on it, a record mid-teardown, an evicted observation at the ledger cap, a relay that restarted since, an id another generation minted, and every malformed shape. The round-3 reproduction is checked in as it was written: the reachable relay observed the worker's exit while the client was absent, and recovery has to converge instead of deferring every sweep and holding the Task fenced. Its two shutdown controls and the round-2 certification file ride with it, plus a recovery case for a host too old to know the method, which rejects with -32601 and defers with no capability handshake. Two corrections fell out of writing them. The ledger claimed an id another generation minted could never be in it; `pty.revive` re-creates a process under a serialized id, so the true statement is narrower and now says so. And the eviction is the pattern `BoundedMap` already implements and tests.
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: a3d56ddd-4007-44f7-8e28-69a08db1fb04
📒 Files selected for processing (12)
config/reliability-gates.jsoncfix-reports/19347-round2.mdsrc/main/providers/ssh-pty-liveness-probe.test.tssrc/main/providers/ssh-pty-liveness-probe.tssrc/main/providers/ssh-pty-provider-liveness-readback.test.tssrc/main/providers/ssh-pty-provider.tssrc/main/runtime/runtime-legacy-worker-terminal-exit-certification.test.tssrc/relay/pty-handler-liveness-probe.test.tssrc/relay/pty-handler-ssh-exit-certification.test.tssrc/relay/pty-handler-ssh-recovery-convergence.test.tssrc/relay/pty-handler.tssrc/shared/relay-pty-mint-epoch.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| generation, therefore exited". The implication is false. The relay has removal paths that never | ||
| observe the process end: `disposePtyForRelayShutdown` waits `IMMEDIATE_PTY_EXIT_TIMEOUT_MS` for a | ||
| physical exit, catches the timeout, and deletes the record anyway; `disposePtys` settles with | ||
| `Promise.allSettled`, so another PTY's failed kill rejects the aggregate while that record is |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 '\bdisposePtys\b|Promise\.allSettled' srcRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- report ---'
sed -n '1,18p' fix-reports/19347-round2.md
printf '%s\n' '--- implementation ---'
sed -n '3210,3248p' src/relay/pty-handler.tsRepository: stablyai/orca
Length of output: 2398
Correct the Promise.allSettled failure description.
Promise.allSettled fulfills with rejected result entries. disposePtys explicitly throws rejected.reason after settlement, so state that disposePtys rejects rather than the Promise.allSettled aggregate.
There was a problem hiding this comment.
ℹ️ No critical issues — one minor doc/dead-code note inline.
Reviewed changes
The delta since the prior review (0be1813) re-lands the relay-owned SSH liveness readback that was reverted in round 2, completing the owner contract so a supervised SSH worker that exits while the client is disconnected can finally be certified.
- Relay-owned
pty.probeLivenessrestored — the relay now answers an exact id from anobservedPtyExitIdsledger, certifyingexitedonly where it watched the process end (node-ptyonExit, or a pid probe answering ESRCH); every other record disappearance isunknown. - Owner readback forwarder —
SshPtyProvider.probePtyLivenesscarries the question to the relay viaprobeSshPtyLiveness, mappinglive/exited/unknowntotrue/false/null; a foreign target or an old relay (-32601) answersnullwith no capability handshake. - Bounded ledger — the hand-rolled cap-and-evict is replaced with the existing tested
BoundedMap(4,096 entries); eviction fails closed tounknown. - Shared id module — relay id minting extracts to
relay-pty-mint-epoch.tsso the minting relay and any reader cannot drift. - Tests + gate — six new/updated files (relay probe, exit-certification, recovery-convergence, provider readback/forwarder, legacy certification) and a
ssh-liveness.relay-owned-exit-certificationreliability-gate entry.
I traced the load-bearing paths and find the design sound: the two observation sites record the id at the line that observed the end (onExit at pty-handler.ts:1069, pid-probe ESRCH at :2502), not inside the shared reap; the disposed and pendingRevive guards plus ledger eviction all answer unknown; and probeLiveness's reap of a live-but-dead-pid record is the same pre-existing reapPtyProvenExited path listProcesses already drives, so the notification it publishes is correct, not spurious. Ran the six changed/new test files (47 tests) — all pass.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
The observed-exit ledger keyed evidence by id, but pty.revive reuses ids, so an entry did not say which incarnation it described. Admission now clears the id's prior observation and records its owning incarnation, and the node-pty onExit observer records before the disposed guard - a shutdown that stopped waiting is bookkeeping, and the process ending afterwards is still this relay watching it end - but only while that incarnation still owns the id. Ownership outlives the record because the observation can too: an empty pool is not evidence that a late callback still speaks for the id, so a bounded id -> incarnation map answers it after teardown removed the record. Forgetting either map withholds an exit, never invents one. Also drops ptyIdMintEpoch from pty.getCapabilities, restoring the base host exactly: pairing it with pty.listProcesses is the list-absence inference this PR removed from the client, and a host that publishes it hands an older client the second half of it. Renames the probe's doubt status to unverifiable, the execution-boundary vocabulary, and pins FIFO ledger eviction with a read before the overflow.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since the prior review (03af416) binds PTY exit evidence to the incarnation that earned it, closing the three owner-evidence bugs the round-4 review reproduced, and corrects the FIFO-eviction test.
- Incarnation-bound exit evidence —
wireAndStore(the singlethis.ptys.setsite) now clears a reused id's prior observation and records the owningincarnationIdin a second boundedptyIdIncarnationOwnersmap, so apty.revivere-admission no longer lets a predecessor's exit certify the replacement (F1). - Late-exit certification — node-pty
onExitrecords the observation before thedisposedguard, so an exit delivered after a shutdown gave up waiting still certifies instead of leaving the id unverifiable forever (F2). - Ownership gate —
stillOwnsPtyId(this.ptys.get(id) === managed || ptyIdIncarnationOwners.peek(id) === incarnationId) rejects a superseded incarnation's callback once its record left the pool;peek(notget) keeps reads from renewing retention. - Mint epoch unpublished —
pty.getCapabilitiesno longer advertisesptyIdMintEpoch, restoring base behavior exactly and making the rejected round-1 list/epoch inference unreachable for old clients (F3). unknown→unverifiable—pty.probeLivenessadopts the execution-boundary vocabulary; the provider forwarder already mapped non-live/exitedtonull, so the rename is wire-safe in both directions.- Eviction order pinned — the FIFO-eviction test now reads the oldest entry before overflow, so the
BoundedMap.has()-reorders mutation is caught (F4).
I traced the load-bearing paths and find the incarnation binding sound: the incarnationId is a fresh randomUUID() at both admission sites, so the owners map always names the latest incarnation and a late callback from a superseded record cannot write; BoundedMap.has and peek do not reorder, and set overwrites in place; and the divergence between the two bounded maps under eviction fails closed (forgetting withholds an exit, never invents one). The prior inline note about the dead ptyIdMintEpoch field is addressed — the field is unpublished and the parser remains only as the test oracle that proves the old client's inference is unreachable.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/runtime/orca-runtime-test-scenario-builders.spec.ts (1)
241-242: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn the nullable liveness result from the recovery harness
makePostRevealWorkerRecoveryHarnessacceptsboolean | null, butprobePtyLivenessconvertsnulltofalse. Production records absence only for explicitfalse; this conversion can make recovery reportexitedinstead of deferring. ReturnhasPty(candidate)directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Team
Run ID: 6044437f-e2c1-4fe8-bfef-f3939ea74333
📒 Files selected for processing (10)
config/reliability-gates.jsoncfix-reports/19347-round5.mdsrc/main/providers/ssh-pty-liveness-probe.test.tssrc/main/providers/ssh-pty-provider-liveness-readback.test.tssrc/main/runtime/runtime-legacy-worker-terminal-exit-certification.test.tssrc/relay/pty-handler-liveness-probe.test.tssrc/relay/pty-handler-ssh-exit-certification.test.tssrc/relay/pty-handler-ssh-recovery-convergence.test.tssrc/relay/pty-handler.tssrc/shared/relay-pty-mint-epoch.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/shared/relay-pty-mint-epoch.ts
- config/reliability-gates.jsonc
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| 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 |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'ptyIdIncarnationOwners|observedPtyExitIds|OBSERVED_PTY_EXIT_HISTORY|probeLiveness|recordObservedPtyExit|wireAndStore' \
src/relay/pty-handler.ts
rg -n -C 8 \
'OBSERVED_PTY_EXIT_HISTORY|ptyIdIncarnationOwners|observedPtyExitIds|unverifiable|evict' \
src/relay/pty-handler-liveness-probe.test.tsRepository: stablyai/orca
Length of output: 24366
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 12 \
'class BoundedMap|type BoundedMap|new BoundedMap|bounded-map' \
srcRepository: stablyai/orca
Length of output: 47919
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/shared/bounded-map.ts | sed -n '45,180p'Repository: stablyai/orca
Length of output: 5791
Couple owner eviction with exit-ledger eviction.
ptyIdIncarnationOwners and observedPtyExitIds are separate BoundedMap instances and evict independently. After pty-A is observed, admitting more than OBSERVED_PTY_EXIT_HISTORY other ids can evict its owner entry while retaining its exit-ledger entry. After the live record is removed, probeLiveness('pty-A') checks only observedPtyExitIds and can return exited from stale evidence.
Add a regression test for this sequence and assert unverifiable, or remove the corresponding exit-ledger entry when the owner entry is evicted.
| it('defers an SSH candidate the owning relay will not certify', async () => { | ||
| const { pendingResolutions, deferredDispatchIds } = await reconcile({ | ||
| inventory: listingWithoutThePty, | ||
| isPtyProvenAbsent: provenAbsentViaRelay(async () => ({ status: 'unverifiable' })) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Assert the converted relay PTY ID in this recovery test.
The helper test checks pty.probeLiveness separately, but this request stub ignores both arguments. Capture the request here and assert the method and { id: toRelaySshPtyId(CONNECTION_ID, PTY_ID) }. Otherwise, a wrong connection or mint-epoch conversion can still return unverifiable and pass this test.
| private readonly ptyIdIncarnationOwners = new BoundedMap<string, string>({ | ||
| maxEntries: OBSERVED_PTY_EXIT_HISTORY | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- pty-handler map declarations and revive/exit paths ---'
rg -n -C 12 'ptyIdIncarnationOwners|observedPtyExitIds|pty\.revive|revive|entry\.id|unverifiable' src/relay/pty-handler.ts
printf '%s\n' '--- bounded-map options and call sites ---'
rg -n -C 8 'new BoundedMap|interface BoundedMapOptions|type BoundedMapOptions|sizeOf|maxEntryBytes|maxBytes' src/relay src/sharedRepository: stablyai/orca
Length of output: 50370
Denial of Service (CWE-400): Uncontrolled Resource Consumption
Reachability: External · Exploitability: Moderate
Bound retained PTY IDs by bytes.
pty.revive accepts an unbounded entry.id, and both PTY history maps retain that ID after removal. Configure maxBytes, maxEntryBytes, and UTF-8 key measurement through sizeOf for both maps. Add a regression test that rejects an oversized revived ID and reports it as unverifiable.
…eviction The resident disjunct in stillOwnsPtyId let a PTY that outlives 4,096 admissions certify its own exit after its owner entry was evicted; nothing checked in failed when it was removed. Round-5 review found the survivor.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The delta since the prior review (847912e) is a single test commit that pins the live-record ownership fallback across owner-map eviction; no production code changed.
- Owner-map eviction coverage — two new tests drive
ptyIdIncarnationOwnerspast its 4,096 cap and assert that a still-resident record still certifies its own exit via thethis.ptys.get(id) === manageddisjunct, while a late callback with neither the record nor the owner entry present is withheld (unverifiable). - Reliability-gate bookkeeping — the gate entry gains the two assertion names, a corrected summary (58 tests, thirteen mutations), and a budget-scope note.
I ran src/relay/pty-handler-liveness-probe.test.ts (25 tests) — all pass, including the two new cases. Both assertions are exact (live / exited / unverifiable, not truthiness), so they are regression coverage rather than theatre: they exercise the two disjuncts of stillOwnsPtyId in the exact scenario where the live-record and owner-map halves diverge under eviction.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
|
Closing. The lifecycle half (Task re-open guard + worker-stop re-issue) landed as #19551. The relay exit-evidence half is being redone at the root: a single tagged retirement record (exited vs torn-down) replacing the two-meaning tombstone map, instead of a second ledger and a new probe RPC beside it. A fresh PR will replace this one when that is ready. |
Summary
P0 (lifecycle-federation lane) and P1 (mailbox-liveness lane) from the #16904 audit.
P0. Three places assumed a worker state they never rechecked. The Task guard ran its live-supervised-worker check only for terminal statuses, so
task-update --status dispatchedre-opened a Task under astoppingworker. Settlement then transitioned the worker from a hardcodedreadyand threw, and both callers that run it inside a batch (federated relay import, coordinator message sweep) roll the whole batch back: a relay whose cursor can never advance, and a run marked failed with its messages left unread.beginWorkerStoprefused a re-issue fromstopping, so an operator had no CLI-reachable escape.P1. Legacy worker terminal recovery read
exitedoff one controller listing. A restarted relay omits every id the previous one minted, and a daemon session this process never attached was never in the cache, so absence from a listing is not an observation of exit.Fix
dispatchedstays legal.isLegalLifecycleTransitionpredicate, and returns a structuredworker_not_settleablerejection instead of throwing. Relay import and the coordinator sweep already record rejections and advance. Wire-safe: the relay schema types the code as a string.beginWorkerStopaccepts a re-issue fromstopping; the chain re-issue,stop_unknown, abandon is a complete operator exit and never asserts an exit the runtime did not observe.isLeafPtyProvenAbsent(cache proves presence, only the owning provider's readback proves absence,remote:ids answer unverifiable). Doubt defers to the next sweep.Tests
db-lifecycle-state-space-settlement.test.ts(BFS over reachable task/dispatch/worker triples asserts zero throwing settlements; reports the audit's four on main),db-stopping-worker-report-settlement.test.ts,runtime-legacy-worker-terminal-exit-certification.test.ts. Two recovery fixtures that asserted exits from an empty listing now supply the owner readback their names claimed.A/B on unpatched main: 9 of 15 fail (all the bug-characterizing cases), 6 controls pass. 15 of 15 here.
Gates: orchestration + rpc/orchestration + CLI orchestration + legacy recovery + runtime barrel (187 files, 2856 tests),
tc:node,tc:cli, changed-code quality (14 files, 0 findings).Part of the fix-forward set for #16904 with #19337, #19339, #19344. Not to be merged without explicit sign-off.
Round-1 review follow-ups (pushed)
taskUpdate:pendingand an honest scope comment.SshPtyProviderhad noprobePtyLiveness, so recovery could never certify an SSH legacy worker's exit and deferred forever. New probe usespty.listProcesses(the relay re-probes pids before listing) plus the id's mint epoch versus the relay's current epoch frompty.getCapabilities(one additive optional field): listed → live; unlisted with matching epoch → exited; legacy id, epoch mismatch, relay without the field, timeout, wrong target → unverifiable. Deliberately notpty.attach, which would disturb a live consumer. Old relays keep today's behavior (every absence unverifiable).stoppingonly when the row'sruntime_epochdiffers from this runtime's (stranded by a dead runtime). A same-epoch duplicate keeps the refusal, so an in-flight clean stop can no longer be recorded asstop_unknownand then escalated as a crash byfailActiveDispatchOnExit. Both mutation directions are fenced by tests.Round-2 review follow-ups (pushed)
unverifiableand defer, exactly likeremote:ids; the operator exit chain this PR adds (stop re-issue,stop_unknown, abandon) is the escape. A relay-owned exact-id liveness answer is a separate follow-up.dedupeWorkerStopdocstring corrected; BFS integrity floor raised to the measured count.Round-3 review follow-ups (pushed)
Codex round 3 left one P1: with the readback removed, a supervised SSH worker that exits while the
client is disconnected can never be certified. Recovery defers on every sweep, the durable worker
stays active, and the Task refuses both reuse and a replacement worker. The user's decision is to
ship the relay-owned probe, so the round-2 removal is reverted and the owner contract completed.
What the probe certifies.
pty.probeLivenessanswers one exact id from the owning relay's ownlifecycle state, with no side effect on any consumer.
live= this relay holds a non-disposedrecord whose pid still answers signal 0.
exited= this relay watched the process end — exactlytwo lines produce that evidence, node-pty's
onExitand the pid probe that answered ESRCH, and eachrecords the id at the line that made the observation rather than in the shared reap they both pass
through.
SshPtyProvider.probePtyLivenessforwards and maps only those two; anything else is null.What it never certifies. Everything else is
unknown: a shutdown that deleted the record afterits 8 s exit wait timed out (with or without a sibling kill failure keeping the relay serving under
the same mint epoch), the torn-down-record sweep, a record mid-teardown, a
pty.revivein flightholding the id, an id this relay never held (another generation's, a legacy
pty-N, empty,malformed), an observation evicted at the 4,096 cap, and every id after a relay restart — the ledger
is in memory per relay process on purpose, because forgetting is the fail-closed direction. A
worktree removal or a refused force kill leaves the record in place and the answer
live. Nocapability negotiation, no persisted ledger, no client-side epoch comparison, no listing inference.
Old relays. A host predating the method answers JSON-RPC
-32601, which arrives as a rejectionand maps to null — indistinguishable from a timeout or a disposed multiplexer — so recovery defers
and pre-PR behaviour is preserved. Nothing was added to
pty.getCapabilities, so there is no fieldan old host must publish and none a new host publishes that an old client can misread.
Removal-path census.
this.ptys.delete(...)appears at one line, insideremovePty, which hasexactly three callers. That closes the enumeration.
src/relay/pty-handler.tsonExit→removePty(:1070):1069)certifies the exit node-pty reportedreapPtyProvenExited→reapExitedPty('exited')→removePty(:2496→:2503→:2453):2502)certifies an exit it proves by probing the pid, and keeps certifying itlistProcesses→reapExitedPty('record-torn-down')(:2889)stays unverifiable after the listing sweeps a torn-down record awaydisposePtyForRelayShutdown→removePty(:3269)stays unverifiable after a shutdown removes a record it never saw exit; both convergence controlsdisposePtyForRelayShutdownwith a refused force killkeeps a record a shutdown could not kill live, rather than retiring it on paperpty.shutdown(:2275) andshutdownForWorktreePath(:715)never certifies an exit from a worktree removal it could not proveOne claim in the restored implementation was wrong and is corrected: ids embed the mint epoch, but
pty.revivere-creates a process under a serialized id, so a previous generation's id can enterthe ledger. The provable statement is narrower — only ids of records this relay itself held, minted
here or revived here — and the foreign-generation test now runs against a populated ledger.
Tests. The round-3 reproduction is checked in unmodified as
src/relay/pty-handler-ssh-recovery-convergence.test.ts(real handler + real provider + realregistry/controller readback + real recovery candidate + in-memory orchestration DB). It and the
round-2 certification file pass unchanged. New relay cases cover every row above plus revive,
eviction, restart and id shapes; a new recovery case covers the
-32601host. All six files areregistered as reliability gate
ssh-liveness.relay-owned-exit-certification.Six mutations of the probe and its forwarder each fail at least one checked-in test, zero survivors:
record in the shared reap (1/47), dropped
disposedcheck (1/47), dropped pending-revive check(1/47), absent record as exited (14/47), provider
unknownas false (10/47), provider rejection asfalse (4/47).
Gates:
src/relay+src/main/providers+src/main/ipc/pty+src/main/daemon(504 files, 5,121tests),
src/main/runtime(710 files, 7,359 tests),tc:node,tc:cli, changed-code quality (24files, 0 findings), reliability-gate manifest (110 gates), max-lines ratchet.
Also reuses the existing tested
BoundedMapfor the ledger instead of a hand-rolled cap-and-evict.Round-4 review follow-ups
The review reproduced three owner-evidence bugs on top of a recovery path and removal census it
found correct. Full write-up in
fix-reports/19347-round5.md.F1 (P0) and F2 (P1) are one root cause: the ledger keyed exit evidence by id, and
pty.revivereuses ids, so an entry did not say which incarnation it described. Admission (
wireAndStore, thesingle
this.ptys.setsite) now clears the id's prior observation and records the incarnation thatowns the id; the node-pty
onExitobserver records the exit before theif (managed.disposed)return, and only when that incarnation still owns the id. F1 was a stale entry certifying a revived
process that never exited, once a shutdown removed the new record without proving anything. F2 was a
real exit delivered after disposal being dropped, leaving the id unverifiable forever.
The ownership check is
this.ptys.get(id) === managed || ptyIdIncarnationOwners.peek(id) === incarnationId,not the reviewer's
!this.ptys.has(id) || .... An empty pool is not evidence that a late callbackstill speaks for the id: with the empty-pool clause, an id torn down, revived, and torn down again
lets the first process's callback certify the second one's exit.
ptyIdIncarnationOwnersis asecond bounded map, written at the same single admission site; forgetting an owner withholds an
exit, never invents one, and the live-record disjunct keeps eviction off the normal path.
F3 (P1):
ptyIdMintEpochis no longer published bypty.getCapabilities, restoring base9fed61e5c246exactly, andsrc/shared/relay-pty-mint-epoch.tsno longer equates same-epochomission with an exit.
toRelayPtyIdWithMintEpochstays — ids still carry a generation so theycannot collide. The round-two client's list/epoch inference is checked in as an oracle (reproduced
inline, old client source not shipped) and reaches no verdict against this host.
F4 (P2): the ledger-eviction test now reads the oldest entry before the overflow, which kills
the surviving
BoundedMap.has()-reorders mutation.P3:
pty.probeLivenessanswerslive | exited | unverifiable, matching the execution-boundaryvocabulary. No released client consumes the RPC, and the provider maps anything that is not
liveor
exitedto null in either direction.Twelve mutations now each fail at least one checked-in test, zero survivors: the round-4 six, the
BoundedMap.has()reorder, dropped admission invalidation (2/56), dropped incarnation guard (1/56),that guard weakened to the empty-pool form (1/56), recording moved back below the disposed guard
(2/56), and a republished mint epoch (3/56). All four reviewer scripts pass at this head; the owner
probes needed only
'unknown'→'unverifiable'in their appended assertions for the P3 rename.Gates:
src/relay src/main/providers src/main/runtime src/main/daemon src/main/ipc/pty(1,214files, 12,489 tests), the six-file reliability command (56 tests, was 47),
tc:node,tc:cli,changed-code quality (24 files, 0 findings), reliability-gate manifest (110 gates), max-lines
ratchet.