Repository navigation
fix(ssh): stop pane adoption certifying a death from the relay's not-found union - #18531
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between a23adea2fe8ff1982247bb0ce156422a39295e33 and 2b8b2ba. 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe relay now marks attach refusals only when it proves that the PTY process exited. SSH reattach code maps marked refusals to Merge Risk: 🟡 Moderate · up to SSH PTY exit classification is covered by updated fixtures, but the relay deployment bundle still needs confirmation that its shared-module import resolves correctly before merge. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides a detailed defect explanation, fix rationale, scope boundaries, tests, and verification results. However, it omits required template content, including the Linked Issue, Visual Proof or an explicit N/A statement, and several checklist sections. ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![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: b58dab9d-f3fd-4147-b7a7-ed924efcc879
📥 Commits
Reviewing files that changed from the base of the PR and between c79e1c0 and a23adea2fe8ff1982247bb0ce156422a39295e33.
📒 Files selected for processing (11)
docs/reference/ssh-execution-boundary.mdsrc/main/ipc/pty/pane/stable-owner.tssrc/main/ipc/pty/pane/stable-pane-absence-death-certificate.test.tssrc/main/ipc/pty/provider/liveness.tssrc/main/providers/ssh-pty-errors.tssrc/main/providers/ssh-pty-reattach-absence-discrimination.test.tssrc/main/providers/ssh-pty-session-reattach.tssrc/main/ssh-expired-lease-pane-readoption.test.tssrc/relay/pty-handler-attach-replay.test.tssrc/relay/pty-handler.tssrc/shared/pty-attach-absence-evidence.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
| @@ -35,6 +35,7 @@ import { | |||
| type RelaySpawnCwdResolution | |||
| } from './pty-spawn-cwd' | |||
| import { PhysicalExitTracker } from '../shared/physical-exit-tracker' | |||
| import { PTY_ATTACH_PROVEN_EXITED_MARKER } from '../shared/pty-attach-absence-evidence' | |||
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep the relay marker local.
src/relay is deployed as a separate bundle. This import can make the remote relay bundle resolve an app-side shared module. Define PTY_ATTACH_PROVEN_EXITED_MARKER locally in src/relay/pty-handler.ts. Keep the test assertion against the shared value so it detects literal drift.
Proposed fix
-import { PTY_ATTACH_PROVEN_EXITED_MARKER } from '../shared/pty-attach-absence-evidence'
+const PTY_ATTACH_PROVEN_EXITED_MARKER = 'process exited'Based on learnings: src/relay is bundled separately for remote deploy, so it must not import app-side modules, including shared constants.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { PTY_ATTACH_PROVEN_EXITED_MARKER } from '../shared/pty-attach-absence-evidence' | |
| const PTY_ATTACH_PROVEN_EXITED_MARKER = 'process exited' |
Source: Learnings
Local verification (in progress → final)
Five other failures in that run are unrelated and pre-existing/load-sensitive, verified individually:
Docker SSH laneNot run. Worth stating precisely why rather than implying coverage: no spec in that lane restarts a relay. Related seam left alone
|
343da17 to
2b8b2ba
Compare
![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
- Marker token for the probed-dead relay refusal —
src/relay/pty-handler.tsappendsPTY_ATTACH_PROVEN_EXITED_MARKERto thereapPtyProvenExitednot-found answer;src/shared/pty-attach-absence-evidence.tsdefines the marker andisProvenExitedPtyAttachRefusal. - Typed subclass at the reattach boundary —
ssh-pty-session-reattach.tsmintsSshPtyProvenExitedOnRelayError(subclass ofSshPtyAbsentFromRelayError) for the marked half, leaving everyisSshPtyAbsentFromRelayErrorconsumer untouched. - No more death certificate from the union —
stable-owner.tspublishes-1and passeshostExitConfirmed: trueonly for observed evidence, keying the retire gate onisHostReportedPtyAbsenceError(type) instead of the text-matchingisPtyAlreadyGoneError. - Test-deed alignment — the follow-up commit throws what the real providers throw (
SessionNotFoundError,SshPtyProvenExitedOnRelayError) and assertsonPtyExit(-1, …, { hostExitConfirmed: true }).
The fix is well-reasoned and wire-safe in both directions: a new relay's marker is additive (an older client keeps its old, already-buggy behavior), and a missing marker — including an older relay's — is never read as evidence, so the certifying branch degrades to unverifiable. I traced the three seams independently (relay refusal → onPtyExit/verdict register → retirement) and ran the affected suites: pty-dead-owner-respawn, pty-persisted-incarnation-repair, pty-pane-reservation-settlement, and stable-pane-absence-death-certificate all pass (17/17), as does the PR's own SSH/relay suite (32/32).
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
…found union `attachStablePaneOwner` was the last reader that synthesised a runtime exit from a reattach refusal, and it published code `0` — which `orca-runtime-on-pty-exit` records as `rememberPtyLivenessVerdict(exited)`, a death certificate whose only legitimate writer is a host-delivered exit frame. The refusal it acted on is a union. `pty.attach` answers `PTY "<id>" not found` both for a pid the relay probed with `isProcessAlive` and for an id its session map simply never had — which, because ids carry a per-start mint epoch, is every id minted before a relay restart, checked against nothing. So a relay restart plus a reconnect certified a shell that was still running under the old daemon's orphaned process tree, retired the pane binding, and cold-started a second agent onto the same transcript. The sibling `handlePtyReattachFailure` has always refused to certify from that union; this path did not. - The relay marks the one refusal it backed with a liveness check (`PTY_ATTACH_PROVEN_EXITED_MARKER`). The marker is additive, so an unmarked answer — including an older relay's — stays ambiguous, which is the safe direction. - The client mints that half as `SshPtyProvenExitedOnRelayError`, a subclass so every existing `isSshPtyAbsentFromRelayError` consumer is unchanged. - Pane adoption publishes `UNVERIFIED_PROCESS_EXIT_CODE` (-1), the sentinel its sibling publishes, and passes `hostExitConfirmed` only for evidence that observed the process: the marked relay refusal, or `SessionNotFoundError` from the registry that owns the PTY. The ambiguous half now records `unverifiable` instead of `exited`. - The gone-branch keys on the error type rather than the bare `PTY ".+" not found` text, so an untyped string can no longer authorise abandoning a binding — the discriminator `pty-connect-limits.ts` already documented. Refs docs/reference/ssh-execution-boundary.md
…throw
These four fixtures rejected with bare `new Error('Session not found: ...')` and
`new Error('PTY "..." not found')`. No provider produces either untyped:
`local-pty-spawn` and `decodeDaemonResponseError` both mint
`SessionNotFoundError`, and the SSH reattach path types the relay's wire text
before any pane sees it. Fixtures that skip the type were the reason a
message-shaped gate looked adequate.
The exit-code expectations move with it: the pane path now publishes the -1
stop sentinel plus `hostExitConfirmed`, so a certificate follows the evidence
rather than a synthesized zero.
2b8b2ba to
6fbd093
Compare
CI: all 8 test shards pass
Introduced by Rebased onto Local
|
Local
|
Final CI: 21 pass / 2 fail — both pre-existing on
|
| lane | result |
|---|---|
test / tests node 24 1..8 (all 8 shards) |
pass |
e2e / ssh docker watcher isolation |
pass (19m46s) |
e2e / changed e2e specs |
pass (9m28s) |
typecheck, managed hooks on Node 18, root directory guard, test vs non-test LoC, packaging (incl. Windows) |
pass |
static analysis |
fail — verify:localization-runtime-catalog |
verify |
fail — only because it needs: static analysis |
The two failures are the onboarding-i18n catalog drift from c79e1c097b7, reproduced verbatim on a clean detached origin/main checkout. Nothing in this PR touches an i18n file.
The SSH Docker lane passing is a regression check on the reattach-succeeds path, not proof of the fix — as noted above, no spec in that lane restarts a relay, so it cannot reach the not-found union. That remains a recorded coverage gap rather than something this PR quietly claims.
…found union (stablyai#18531) * fix(ssh): stop pane adoption certifying a death from the relay's not-found union `attachStablePaneOwner` was the last reader that synthesised a runtime exit from a reattach refusal, and it published code `0` — which `orca-runtime-on-pty-exit` records as `rememberPtyLivenessVerdict(exited)`, a death certificate whose only legitimate writer is a host-delivered exit frame. The refusal it acted on is a union. `pty.attach` answers `PTY "<id>" not found` both for a pid the relay probed with `isProcessAlive` and for an id its session map simply never had — which, because ids carry a per-start mint epoch, is every id minted before a relay restart, checked against nothing. So a relay restart plus a reconnect certified a shell that was still running under the old daemon's orphaned process tree, retired the pane binding, and cold-started a second agent onto the same transcript. The sibling `handlePtyReattachFailure` has always refused to certify from that union; this path did not. - The relay marks the one refusal it backed with a liveness check (`PTY_ATTACH_PROVEN_EXITED_MARKER`). The marker is additive, so an unmarked answer — including an older relay's — stays ambiguous, which is the safe direction. - The client mints that half as `SshPtyProvenExitedOnRelayError`, a subclass so every existing `isSshPtyAbsentFromRelayError` consumer is unchanged. - Pane adoption publishes `UNVERIFIED_PROCESS_EXIT_CODE` (-1), the sentinel its sibling publishes, and passes `hostExitConfirmed` only for evidence that observed the process: the marked relay refusal, or `SessionNotFoundError` from the registry that owns the PTY. The ambiguous half now records `unverifiable` instead of `exited`. - The gone-branch keys on the error type rather than the bare `PTY ".+" not found` text, so an untyped string can no longer authorise abandoning a binding — the discriminator `pty-connect-limits.ts` already documented. Refs docs/reference/ssh-execution-boundary.md * test(pty): make the pane-adoption fixtures throw what real providers throw These four fixtures rejected with bare `new Error('Session not found: ...')` and `new Error('PTY "..." not found')`. No provider produces either untyped: `local-pty-spawn` and `decodeDaemonResponseError` both mint `SessionNotFoundError`, and the SSH reattach path types the relay's wire text before any pane sees it. Fixtures that skip the type were the reason a message-shaped gate looked adequate. The exit-code expectations move with it: the pane path now publishes the -1 stop sentinel plus `hostExitConfirmed`, so a certificate follows the evidence rather than a synthesized zero.
The defect
attachStablePaneOwner(src/main/ipc/pty/pane/stable-owner.ts) was the last reader that synthesised a runtime exit out of a reattach refusal, and it published exit code0:orca-runtime-on-pty-exit.tsreadsexitCode >= 0asrememberPtyLivenessVerdict(ptyId, { status: 'exited' })— a death certificate whose own doc comment says "Its only writer is a host-delivered exit frame; nothing weaker may reach it."The refusal it acts on is a union.
src/relay/pty-handler.tsattachthrowsPTY "<id>" not foundfor two unrelated situations:isProcessAliveand found gone (reapPtyProvenExited) —exitedis correct;pty2:<ptyIdMintEpoch>:<n>, is every previously minted id after a relay restart.So: relay restarts → user reconnects → pane adoption runs
attachOnlyfor a lease whose shell is still alive under the old daemon's orphaned process tree →exitedis recorded →retirePersistedStablePaneOwnerclears the binding → fresh spawn → duplicate agent on one transcript.The codebase already knew this.
handlePtyReattachFailure(ssh-relay-session.ts) carries a paragraph explaining exactly this union and deliberately does not callonPtyExit, publishing-1instead.stable-owner.tswas the one remaining path that did — with0.Blame is
3bc13f7b8cb(Aug 24); pre-existing, not from today's sweep. The existing test (ssh-expired-lease-pane-readoption.test.ts) passedruntime: undefined, soonPtyExitwas never observable.The fix
The two relay situations were genuinely indistinguishable at the client — the same string produced both, and
SshPtyAbsentFromRelayErrorwas minted for the whole union. Following therestoreRequiredprecedent from #17951, the narrow case now gets its own token:pty-handler.ts): thereapPtyProvenExitedrefusal carriesPTY_ATTACH_PROVEN_EXITED_MARKER. Additive on purpose — an unmarked answer, including an older relay's, stays ambiguous, so a missing marker is never evidence.ssh-pty-errors.ts,ssh-pty-session-reattach.ts): that half is minted asSshPtyProvenExitedOnRelayError, a subclass ofSshPtyAbsentFromRelayError, so every existingisSshPtyAbsentFromRelayErrorconsumer (spawn-execute.ts×2 lease bookkeeping) is unchanged.stable-owner.ts: publishesUNVERIFIED_PROCESS_EXIT_CODE(-1), the sentinel its sibling publishes, and passeshostExitConfirmed: trueonly for evidence that observed the process — the marked relay refusal, orSessionNotFoundErrorfrom the registry that owns the PTY. The ambiguous half now recordsunverifiable, notexited.isHostReportedPtyAbsenceError(classes) instead ofisPtyAlreadyGoneError, whose bare/PTY ".+" not found/match let an untyped string abandon a binding. This is the discriminatorpty-connect-limits.tsalready documented on the renderer side.isPtyAlreadyGoneErroris untouched for the kill/teardown callers, where leniency is correct.What is deliberately not changed
The ambiguous case still retires the client's pane binding and falls through to a fresh spawn. That is client routing bookkeeping, not a claim about the process: after a relay change the old id is genuinely unroutable, and
docs/reference/ssh-execution-boundary.mdalready documents that fallback. Refusing to retire would also strand the pane —resolveStablePaneOwnerthrowsterminal_pane_owner_conflictonce the runtime's pane→pty and persistence disagree. The destructive artifact was the certificate, and that is what is removed: the orphan now staysunverifiable, so the sweep, stop verdicts andgetPtyLivenessVerdictkeep failing closed over it.Tests
src/main/ipc/pty/pane/stable-pane-absence-death-certificate.test.ts(new) — a realOrcaRuntimeService, soonPtyExitand the verdict register are observable. Covers the relay-restart case (unverifiable, no certificate), the pid-probed case (exited, binding retired — a genuinely dead PTY must still retire), the owning-registry case, and an untypednot foundstring (no verdict, persistence untouched).src/relay/pty-handler-attach-replay.test.ts— the same spec now asserts the probed refusal carries the marker and the "id the map never had" refusal does not.src/main/providers/ssh-pty-reattach-absence-discrimination.test.ts— the reattach path maps only the marked refusal to the proven class.ssh-expired-lease-pane-readoption.test.tsnow throws the typed error the real SSH provider raises rather than raw wire text.Failing before the fix (verified by reverting each half in place):
Verification
pnpm tc✅pnpm run check:code-quality:changed✅ (0 new findings across 10 files;max-linesproved live with a 320-line positive control)oxfmt --check✅pnpm test src— see comment below