Repository navigation
fix(ssh): stop reporting an unresumable delivery as a dead session - #14974
Closed
brennanb2025 wants to merge 43 commits into
Closed
brennanb2025 wants to merge 43 commits into
brennanb2025 wants to merge 43 commits into
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughSSH PTY handling now distinguishes restore-required errors, unverifiable liveness, ambiguous exits, and confirmed PTY exits. Restore and liveness failures preserve ownership and mark leases detached so reattachment can continue. Confirmed exits still expire leases and clear ownership. Provider state tracks live, unverifiable, ambiguous, and exited PTYs. Renderer transport preserves existing bindings and reports delivery-unresumable results. Localized messages explain the terminal state. Tests and reliability gates cover these lifecycle paths. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
Contributor
Author
|
SSH / Electron QA evidence (gate review)
|
brennanb2025
force-pushed
the
brennanb2025/ssh-restorerequired-fix-2
branch
from
August 17, 2026 03:07
fb7d40d to
245d7a2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/ipc/pty.ts (1)
5153-5164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth locations implement the identical SSH lease-detachment rule for restore-required/liveness-unverifiable spawn failures. One root cause: the runtime-controller spawn path and the
ipcMain.handle('pty:spawn')path duplicate this safety-relevant check instead of sharing one implementation.
src/main/ipc/pty.ts#L5153-L5164: extract this block into a shared helper (for examplemarkSshLeaseDetachedOnUnresumableFailure(store, connectionId, relayId, spawnError, rawMessage)) and call it here.src/main/ipc/pty.ts#L6719-L6730: call the same shared helper here instead of repeating the condition and action.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 162a5de4-94c0-498e-8d9a-08674cd4a045
📒 Files selected for processing (18)
config/reliability-gates.jsoncsrc/main/ipc/pty-daemon-ssh-lease-lifecycle.test.tssrc/main/ipc/pty.tssrc/main/providers/ssh-pty-errors.tssrc/main/providers/ssh-pty-liveness-state.tssrc/main/providers/ssh-pty-provider.tssrc/main/providers/ssh-pty-restore-required-reattach.test.tssrc/main/providers/ssh-pty-session-reattach.tssrc/main/ssh/ssh-relay-session-reconnect-incarnation.test.tssrc/main/ssh/ssh-relay-session.tssrc/renderer/src/components/terminal-pane/TerminalErrorToast.test.tssrc/renderer/src/components/terminal-pane/TerminalErrorToast.tsxsrc/renderer/src/components/terminal-pane/pty-connection-direct-ssh-reattach-retry.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/pty-transport-spawn-errors.test.tssrc/renderer/src/components/terminal-pane/pty-transport-types.tssrc/renderer/src/components/terminal-pane/pty-transport.tssrc/renderer/src/i18n/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/renderer/src/i18n/locales/en.json
- src/main/ipc/pty-daemon-ssh-lease-lifecycle.test.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 3 remain after this review.
Treat restoreRequired as a delivery condition, retry one fresh attach, and keep the remote lease detached after repeated delivery failure. Preserve exited classification only for authoritative PTY absence, humanize the live-session recovery path, and pin the invariant in the reliability gate.
Surface repeated output-delivery recovery failure without retiring the remote lease or cold-starting a duplicate agent. Keep renderer retry authority bounded and explain that the remote session remains live.
A first restore-required response proves the remote PTY is live. If the bounded fresh-delivery attach then loses transport, retain the PTY and renderer binding; only authoritative absence or an observed exit may allow replacement.
Distinguish host-confirmed live delivery failure from a transport loss during verification. Preserve the detached lease and pane binding for both, but reserve provider live state and live-session copy for an observed host response.
Track a PTY whose verification lost transport without classifying it live or exited. Keep provider routing ownership, but return null through the existing tri-state liveness probe until a later inventory or exit supplies evidence.
Record the transport-loss branch in the terminal resilience gate so future refactors cannot collapse the tri-state probe back to a false absence.
The reviewing agent died with this uncommitted while implementing the P1 fix for probePtyLiveness returning false for PTYs absent from both liveness sets. Introduces ssh-pty-liveness-state.ts. Not reviewed, not verified, not complete.
brennanb2025
force-pushed
the
brennanb2025/ssh-restorerequired-fix-2
branch
from
August 17, 2026 06:46
245d7a2 to
bd4c391
Compare
Contributor
Author
|
Verdict: clean at exact SHA bd4c391. No evidence-backed P0, P1, or P2 findings. This was reviewed and pushed, but not merged. What is proved:
Validation:
Exact-SHA Electron/CDP evidence:
Unverified gaps:
Reliability record:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/providers/ssh-pty-provider.ts (1)
286-288: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNormalize IDs in
hasPtyandprobePtyLiveness.
SshPtyLivenessStatestores app-scoped IDs, but these methods compare raw IDs. Relay-scoped queries therefore returnfalseornull. Current production callers use app-scoped IDs, so this remains an optional consistency refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e0dd80cf-68c2-4b65-bdcb-e0b51fa97d59
📒 Files selected for processing (28)
config/reliability-gates.jsoncsrc/main/ipc/pty.tssrc/main/providers/pty-process-inspection.test.tssrc/main/providers/pty-process-inspection.tssrc/main/providers/ssh-agent-session-create-operation.tssrc/main/providers/ssh-pty-attach-liveness.tssrc/main/providers/ssh-pty-liveness-state.tssrc/main/providers/ssh-pty-notification-rejection-routing.test.tssrc/main/providers/ssh-pty-notification-routing.test.tssrc/main/providers/ssh-pty-notification-routing.tssrc/main/providers/ssh-pty-process-list-liveness.tssrc/main/providers/ssh-pty-provider-exit-race.test.tssrc/main/providers/ssh-pty-provider-output-state.tssrc/main/providers/ssh-pty-provider-process-events.test.tssrc/main/providers/ssh-pty-provider-reattach-incarnation.test.tssrc/main/providers/ssh-pty-provider.test.tssrc/main/providers/ssh-pty-provider.tssrc/main/providers/ssh-pty-restore-required-reattach.test.tssrc/main/providers/ssh-pty-session-reattach.tssrc/main/providers/ssh-pty-shutdown.tssrc/main/providers/ssh-pty-source-recovery-result.tssrc/main/providers/ssh-pty-spawn-exit-race.tssrc/main/ssh/ssh-relay-session-incarnation.test.tssrc/main/ssh/ssh-relay-session-reconnect-incarnation.test.tssrc/main/ssh/ssh-relay-session-recovery-races.test.tssrc/main/ssh/ssh-relay-session.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/i18n/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/providers/ssh-pty-provider-reattach-incarnation.test.ts
- src/renderer/src/i18n/locales/en.json
- src/main/ipc/pty.ts
- src/renderer/src/components/terminal-pane/pty-connection.ts
Included review availability: Your plan includes up to 10 reviews per rolling hour; 0 remain after this review.
# Conflicts: # src/main/providers/ssh-pty-provider.test.ts
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d3993f96-be17-4c40-81a9-095791fe0c17
📒 Files selected for processing (51)
config/reliability-gates.jsoncconfig/scripts/wsl-git-shell-benchmark.mjssrc/main/codex/codex-trust-grant-host.test.tssrc/main/ipc/pty-daemon-ssh-lease-lifecycle.test.tssrc/main/ipc/pty.tssrc/main/ipc/ssh-handler-reregistration.test.tssrc/main/ipc/ssh-ipc-mock-shapes.tssrc/main/ipc/ssh-ipc-module-mocks.tssrc/main/ipc/ssh-state-broadcast-fanout.test.tssrc/main/local-worktree-filesystem-wsl-banner.wsl.test.tssrc/main/local-worktree-filesystem.test.tssrc/main/providers/local-pty-provider-windows-shell-launch.test.tssrc/main/providers/pty-process-inspection.test.tssrc/main/providers/pty-process-inspection.tssrc/main/providers/ssh-agent-session-create-operation.tssrc/main/providers/ssh-pty-attach-liveness.tssrc/main/providers/ssh-pty-errors.tssrc/main/providers/ssh-pty-liveness-state.tssrc/main/providers/ssh-pty-notification-rejection-routing.test.tssrc/main/providers/ssh-pty-notification-routing.test.tssrc/main/providers/ssh-pty-notification-routing.tssrc/main/providers/ssh-pty-process-list-liveness.tssrc/main/providers/ssh-pty-provider-exit-race.test.tssrc/main/providers/ssh-pty-provider-output-state.tssrc/main/providers/ssh-pty-provider-process-events.test.tssrc/main/providers/ssh-pty-provider-reattach-incarnation.test.tssrc/main/providers/ssh-pty-provider-spawn.test.tssrc/main/providers/ssh-pty-provider.test.tssrc/main/providers/ssh-pty-provider.tssrc/main/providers/ssh-pty-restore-required-reattach.test.tssrc/main/providers/ssh-pty-session-reattach.tssrc/main/providers/ssh-pty-shell-metadata.tssrc/main/providers/ssh-pty-shutdown.tssrc/main/providers/ssh-pty-source-recovery-result.tssrc/main/providers/ssh-pty-spawn-exit-race.tssrc/main/ssh/ssh-relay-session-incarnation.test.tssrc/main/ssh/ssh-relay-session-reconnect-incarnation.test.tssrc/main/ssh/ssh-relay-session-recovery-races.test.tssrc/main/ssh/ssh-relay-session.test.tssrc/main/ssh/ssh-relay-session.tssrc/renderer/src/components/settings/CliSkillRuntimeSetup.test.tsxsrc/renderer/src/components/settings/CliSkillRuntimeSetup.tsxsrc/renderer/src/components/terminal-pane/TerminalErrorToast.test.tssrc/renderer/src/components/terminal-pane/TerminalErrorToast.tsxsrc/renderer/src/components/terminal-pane/pty-connection-direct-ssh-reattach-retry.test.tssrc/renderer/src/components/terminal-pane/pty-connection.tssrc/renderer/src/components/terminal-pane/pty-transport-spawn-errors.test.tssrc/renderer/src/components/terminal-pane/pty-transport-types.tssrc/renderer/src/components/terminal-pane/pty-transport.tssrc/renderer/src/i18n/locales/en.jsonsrc/shared/wsl-exec-mode-separator.test.ts
💤 Files with no reviewable changes (1)
- src/main/codex/codex-trust-grant-host.test.ts
🚧 Files skipped from review as they are similar to previous changes (38)
- src/main/ipc/ssh-ipc-module-mocks.ts
- src/main/ipc/ssh-handler-reregistration.test.ts
- src/renderer/src/components/terminal-pane/pty-transport-types.ts
- src/main/providers/ssh-pty-provider-process-events.test.ts
- src/main/ipc/ssh-ipc-mock-shapes.ts
- src/renderer/src/components/terminal-pane/pty-connection-direct-ssh-reattach-retry.test.ts
- src/renderer/src/i18n/locales/en.json
- src/main/providers/pty-process-inspection.ts
- src/main/providers/pty-process-inspection.test.ts
- src/main/ipc/pty-daemon-ssh-lease-lifecycle.test.ts
- src/main/providers/ssh-pty-shutdown.ts
- src/renderer/src/components/terminal-pane/pty-transport-spawn-errors.test.ts
- src/main/providers/ssh-pty-provider-reattach-incarnation.test.ts
- src/main/ssh/ssh-relay-session.test.ts
- src/main/providers/ssh-pty-provider-exit-race.test.ts
- src/main/providers/ssh-pty-shell-metadata.ts
- src/main/providers/ssh-pty-errors.ts
- src/main/providers/ssh-pty-restore-required-reattach.test.ts
- src/renderer/src/components/terminal-pane/TerminalErrorToast.test.ts
- src/renderer/src/components/terminal-pane/pty-transport.ts
- src/main/ssh/ssh-relay-session-recovery-races.test.ts
- src/main/providers/ssh-pty-process-list-liveness.ts
- src/main/providers/ssh-pty-notification-routing.test.ts
- src/main/ipc/pty.ts
- src/main/providers/ssh-pty-source-recovery-result.ts
- src/renderer/src/components/terminal-pane/TerminalErrorToast.tsx
- src/main/providers/ssh-pty-attach-liveness.ts
- src/renderer/src/components/terminal-pane/pty-connection.ts
- src/main/ipc/ssh-state-broadcast-fanout.test.ts
- src/main/providers/ssh-agent-session-create-operation.ts
- src/main/ssh/ssh-relay-session-reconnect-incarnation.test.ts
- src/main/providers/ssh-pty-notification-rejection-routing.test.ts
- src/main/ssh/ssh-relay-session-incarnation.test.ts
- src/main/providers/ssh-pty-provider-output-state.ts
- src/main/providers/ssh-pty-spawn-exit-race.ts
- src/main/providers/ssh-pty-liveness-state.ts
- src/main/providers/ssh-pty-provider.ts
- src/main/ssh/ssh-relay-session.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Comment on lines
+247
to
+251
| const canceled = await unresumable.sourceActivationLease.rollback() | ||
| if (!canceled) { | ||
| throw new Error( | ||
| `${SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR}: ${toRelaySshPtyId(args.connectionId, unresumable.id)}` | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Classify failed source cancellation as unverifiable.
If rollback() returns false, line 250 creates SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR. Line 272 then maps that marker to acceptAmbiguousExitPty at line 274. This path has no observed exit. It bypasses acceptUnverifiablePty and can apply ambiguous-exit handling to a delivery-cancellation failure.
Track this condition separately. Call acceptUnverifiablePty for this path.
Proposed fix
let result: SshPtyReattachResult | undefined
+ let sourceCancellationUnverifiable = false
try {
@@
const canceled = await unresumable.sourceActivationLease.rollback()
if (!canceled) {
+ sourceCancellationUnverifiable = true
throw new Error(
`${SSH_PTY_LIVENESS_UNVERIFIABLE_ERROR}: ${toRelaySshPtyId(args.connectionId, unresumable.id)}`
)
@@
- const ambiguousExit = isSshPtyLivenessUnverifiableError(error)
+ const ambiguousExit =
+ !sourceCancellationUnverifiable && isSshPtyLivenessUnverifiableError(error)Also applies to: 272-280
brennanb2025
added a commit
that referenced
this pull request
Aug 20, 2026
…bulary (#14971) * docs(ssh): state the SSH execution boundary and pin the liveness vocabulary Nothing under docs/ described how work splits between the client and an SSH host, so agents and humans inferred it from error strings and got it wrong: loss of contact was repeatedly reported as process death, which orphaned live remote agents and cold-started duplicates over the same worktree. Pins the vocabulary to the incumbent live/unverifiable/exited verdict from unstopped-pty-verification so no synonym is introduced, records the one real discriminator (all of a host's terminals drop together on link loss; one alone means process exit), and lists the outstanding gaps with citations. Tracked via the docs allow-list and linked from AGENTS.md, per the convention in .gitignore. * docs(ssh): cite the live restoreRequired site after it moved The throw now lives in reattachSshPtySessionForSpawn; ssh-pty-provider.ts no longer contains it. Caught by the worker fixing it, against a newer main than the audit ran on. * docs(ssh): require host evidence for liveness verdicts * docs(ssh): keep boundary references stable * docs(ssh): fence liveness evidence to its host identity * docs(ssh): state replay and environment boundaries precisely * docs(ssh): correct replay and platform boundary claims * docs(ssh): describe headless runtime continuity accurately * docs(ssh): distinguish authority from client metadata * docs(ssh): describe pending fixes accurately * docs(ssh): date the gap list and name the PR that closes each entry The Known gaps section was accurate when written and becomes actively misleading as its fixes land: it told a reader to go fix restoreRequired, the missing unverifiable verdict, and the absent terminal-list host field, three things now addressed by #14974, #14977 and #14973. Mark the section as dated, require verification against current code before acting on any entry, name the PR per entry, and move landed items out. Also correct the two body claims that the landed fixes invalidated. The rules above are durable; only this section rots. * docs(ssh): make the boundary doc a durable ruleset, not an incident record The Known gaps section was 18 of 93 lines enumerating specific defects from one investigation, several already fixed by sibling PRs in the same batch. A reference doc that needs a 'this section rots' warning is telling you the section belongs somewhere else; those entries belong in issues. Replace the six-row table of currently-lying signals with the method that outlands any particular bug: ask whether the owning host produced the signal, whether every PTY on the target went quiet together, whether the termination event matches the current incarnation and generation, and whether a returned status is actually a claim. Same for artifacts - state what ls-remote and a PR head each do and do not prove, rather than listing which command is currently wrong. Nothing here goes stale when the open fixes land.
Contributor
Author
Electron SSH visual proof (head
|
| Check | Result |
|---|---|
| PTY id | ssh:ssh-1787214764400-bd86w0@@pty-1 before and after |
| Remote marker PID | 2377 throughout (ticks n=1 → n=58+) |
| Remote shell PID | 2219 throughout |
| Output after reconnect | Marker ticks continued on the same pane |
| Git checkout | Clean (GIT_DIRTY_BYTES=0) |
| Duplicate session | One original shell (2219) and one marker (2377). Detached relay stayed PID 1963. The second shell (3275) is a user-opened inspection tab, not a cold-start duplicate of pty-1. Resource Manager stayed at 1 session until that tab was opened. |
Relay log after the drop: socket client closed with ptys=1, then handshake accepted. The live PTY was preserved across transport loss.
restoreRequired honesty
A real transport drop was injected (relay --connect killed; remote PTY left alive). I did not observe a restoreRequired / checkpointUnavailable line in the relay log: the detached relay still had the live PTY and the client reattached inside the 60s grace window with a usable delivery. Do not treat this run as proof of the restoreRequired delivery-layer path specifically; it is proof that a live remote PTY/session is not retired and not duplicated across transport loss.
Screenshots
SSH terminal live on the Ubuntu 20.04 host:
Before drop — same PTY id, shell 2219, marker 2377, clean git:
After --connect kill — ticks still on PID 2377 (transport already reattached):
After reconnect — still pty-1, still PID 2377, output continuing:
Host inventory from a second (user-opened) tab: marker file 2377, git clean, original shell 2219 still unique:
Original pane still ticking on 2377:
SHA tested: 4b34fdc116. Disposable container/key/profile torn down after this comment.
A pty.exit that arrived while pty.attach was in flight was held back for the reattach to classify. If that attach then rejected — a timeout on a healthy connection, say — the operation was discarded without ever classifying, and the held exit went with it: no pty:exit reached the renderer, the lease never went 'terminated', and the pane kept showing a dead process as live. Release a held exit when the last operation that could still claim it finishes without a verdict. An operation that did classify keeps ownership, so a reattach that fenced a stale incarnation is not overridden by a sibling's failure.
…s unproven On the restoreRequired retry path the first attach already answered with an incarnation, so the remote PTY is proven alive and only its delivery record was retired. Filing that as ambiguous exit evidence recorded a live agent as possibly dead. Report it as restore-required and accept the PTY as live. Both errors already map to deliveryUnresumable in the renderer and to a 'detached' lease, so the pane behaves exactly as before.
…the host Filling the ambiguous-exit set tore down the whole SSH channel, so one busy host could drop every terminal and agent on it at once. The set is only ever counted, never read, so bound it the way the exit tombstone set is bounded: evict the oldest entry and leave the channel alone.
…n id A host exit notification is positive evidence of death; a missing incarnation id only means we cannot attribute it. Treating every incarnation-less exit as ambiguous meant that against an older relay — one that never sends the field — panes never showed "Process exited" and the lease never reached 'terminated'. Mixed client/host versions are the normal state, so restore delivery when no incarnation was recorded, and keep the fence only where one was.
On the restoreRequired path the retry cancels the old delivery, and the exit fence is already closed by then. A pty.exit arriving during that round trip was published and tombstoned, and the unconditional acceptLivePty that followed erased it — probePtyLiveness then answered "live" forever for a process the owning host had reported dead, so no terminal_gone was raised and the renderer kept a dead pane bound. Open live evidence before the cancel round trip and settle it after, so an exit observed in that window invalidates the evidence and refuses the promotion.
… death A spawn that begins before the relay assigns its pty id was handed a pending exit but no quarantine pointer, so the verdict it reached never claimed the exit and a sibling operation's finish() published it anyway. finish() was also not idempotent: a second call decremented the holder count again and released an exit another operation was still fencing. Both paths deliver a pty:exit for a PTY that was never confirmed dead. Give an unbound operation the quarantine pointer without making it a holder, and make finish() a no-op after the first call.
The double invalidated every pending live evidence on any exit, while the real SshPtyLivenessState invalidates only the exiting PTY's. Nothing is vacuous today because each test uses one PTY, but a two-PTY test would pass here and fail in production.
…pane live The restoreRequired retry closes the exit fence between its two attach attempts, so a pty.exit arriving in that gap is published and tombstoned against no pending operation. Only the unproven-cancel branch consulted that evidence; when the delivery cancellation was PROVEN, both remaining promotions ran unconditionally and erased the tombstone. probePtyLiveness then answered "live" forever for a process the owning host had reported dead, so no terminal_gone was raised and the renderer kept a dead pane bound. Open one live-evidence window for the whole reattach and route every promotion through it, so there is no path out of this function to `live` that skips the check. Scope window invalidation to exit evidence: a sibling operation losing contact is not evidence of death and must not stop a later attach that does reach the host from proving the PTY live.
classifyPendingExit stamped every same-id exit as classified before deciding its verdict, including the 'unverifiable' returns where the operation said it could not attribute the exit at all. On relays that send no incarnation ids, an operation that bound to the PTY after the exit was recorded could stamp the quarantine it does not hold; the real holder's finish() then refused to release it and zero pty:exit reached the renderer for a PTY the host reported dead. Claim an exit only when the operation actually attributed it: it owned the PTY when the exit arrived, or both incarnations are known and comparable. recordExit also returned false for an unheld exit and let the notification router publish it directly, bypassing the quarantine's published latch, so a late binder that later attributed the same exit published a second pane death. Publish it through the latch instead and report that the tracker owns it.
…h the host A rejected delivery frame drives a targeted reattach. When that reattach failed for any reason other than a host-confirmed absence, the relay recorded the PTY as unverifiable and then gated the retry on hasPty, which reads false for `unverifiable` and `exited` alike. The pane's output delivery was abandoned outright: no retry, no channel-drop escalation, no user-facing signal, while the remote process kept running. Gate the retry on the tri-state verdict instead, so only a positive `exited` verdict stops recovery and an unverifiable reattach still escalates to the recoverable relay reconnect that restores the pane. The suite could not see this: the provider doubles answered hasPty only, so they read every verdict as two-state. They now use a real SshPtyProvider.
…ust opened On the plain-reconnect path finishSourceRecovery opens live passthrough after draining the recovered output, and activation then reassigned the flag from the targeted-recovery mode, closing it again. Today the pending entry is torn down in the same synchronous run so nothing observes it, but any await introduced between the two would quarantine output that nothing drains.
The set was written, deleted, cleared and capped, but nothing ever read it, so its bound guarded nothing and its guarding test passed with the whole set gone. acceptUnverifiableExit keeps what does matter — refusing the live claims an open evidence window could still settle after the exit — and the test now asserts the verdict itself stays unverifiable rather than counting entries.
…tates subscribeSshPtyNotifications no longer takes provider liveness, so every assertion on the harness's livePtyIds read a set the test alone wrote to, including the exit test's "without changing liveness" clause, which could not fail. The routing unit does not touch liveness; the remaining assertions cover what it actually does.
… arrays The two entries added for this fix left config/reliability-gates.jsonc failing oxfmt --check. Nothing in CI runs oxfmt over .jsonc: no workflow invokes it and lint-staged's glob only matches *.json and *.css.
Contributor
Author
Electron SSH visual proof (head
|
| Check | Result |
|---|---|
| PTY id | ssh:ssh-1787222043484-m1sm35@@pty-1 before and after |
| Remote marker PID | 5848 throughout |
| Remote shell PID | 5760 throughout |
| Detached relay | 5503 throughout |
| Output after reconnect | Marker ticks continued on the same pane |
| Git checkout | Clean (GIT_DIRTY_BYTES=0) |
| Duplicate session | One original shell (5760) and one marker (5848) |
restoreRequired honesty
A real transport drop was injected, and this run did hold the client off the relay for ~74s (past the 30s delivery-owner reconnect grace, while the live PTY stayed pooled). I still did not observe a restoreRequired / checkpointUnavailable / deliveryUnavailable line in the relay log: those statuses are not logged there. Do not treat the log as proof of the named delivery-layer status. What this run does prove is that a live remote PTY/session is not retired and not duplicated when reconnect happens after delivery grace, not only inside it.
Scenario B — remote process exits while the client is disconnected
- Opened a second SSH pane (
pty-2) and ranexec sleep 999(PID 8358). Resource Manager: 2 sessions. - Dropped transport again (kill
--connect+ inbound SSH blocked). While the client wasConnecting…, the host confirmed PID 8358 was gone. Marker 5848 / shell 5760 stayed alive. - On reconnect:
listSessionscontained onlypty-1;hasPty(pty-2)was false; inspect reportedunavailable. Resource Manager: 1 session. - The dead pane showed: Orca couldn't reattach to this pane's terminal session on the host. Open a new terminal to continue. It was not left looking live.
pty-1kept ticking on PID 5848.
The overlay copy is “couldn't reattach”, not the word “exited”. Host-side the process was gone, the session was dropped from the live PTY list, and the sibling live pane was preserved.
Screenshots
Isolated empty app (identity brennanb2025/qa-14974-electron):
SSH terminal live on Ubuntu 20.04 — marker PID 5848:
After the ~74s blocked disconnect — still pty-1, still PID 5848, ticks continuing:
Second pane before disconnect — exec sleep 999, PID 8358 (Resource Manager 2):
Client disconnected while 8358 was killed on the host:
After reconnect — dead pane is not live; overlay asks to open a new terminal (Resource Manager 1):
Original pane still ticking on 5848 after the sibling exit:
SHA tested: 0b2e7a23443b68eaa451075a461d8b7b0e86128a. Disposable container/key/profile torn down after this comment.
Contributor
Author
Review status —
|
| # | Defect | Found by |
|---|---|---|
| 1 | A host-authoritative pty.exit was permanently swallowed when the reattach that quarantined it failed before classifying it — pane kept its binding over a dead process |
round 1 |
| 2 | An unproven delivery cancellation was filed as ambiguous exit evidence despite positive evidence the PTY was live | round 1 |
| 3 | Ambiguous-exit overflow disposed the whole SSH channel, dropping every pane on the host, for a set nothing read | round 1 |
| 4 | Incarnation-less host exits stopped being delivered — on an older relay, panes would never report "Process exited" | round 1 |
| 5 | Promotion to live ran outside the exit fence, erasing the tombstone of a host-confirmed dead PTY |
round 2 |
| 6 | That fix covered only 1 of 3 promotion branches; the proven-cancel path still resurrected a dead pane | round 3 |
| 7 | classified was stamped by operations that explicitly could not attribute an exit, re-opening #1 |
round 3 |
| 8 | One exit could publish twice, bypassing the quarantine latch | round 3 |
| 9 | Delivery-recovery retry read unverifiable as exited, silently dropping a live pane's output with no retry, no escalation, and no user-facing signal |
end readiness |
| 10–13 | livePassthrough clobber (latent), dead write-only state, 11 assertions on a set production no longer receives, and an oxfmt regression nothing in CI catches |
end readiness |
Two structural notes. #9 escaped five review rounds because the guarding double was { hasPty: () => true } — hard-coding the exact value this PR changed, so the escalation test was incapable of failing. Those doubles now use a real provider. And after #6 and #7, the fixes stopped being spot patches: acceptLivePty was removed from the reattach's argument surface entirely so an ungated promotion is unrepresentable, and classified was given a single definition — an operation may claim an exit only if it owned the PTY when the exit arrived, or both incarnations are known and comparable.
Verification
- Main-process suites: 7,070 tests passing, 0 failures. Typechecks clean on node/cli/web.
oxlint,oxfmt, reliability gates (85), max-lines ratchet, localization — all pass. - Every new regression test was proven to fail without its production change. One added test is characterization-only and is labelled as such, not counted as coverage.
- Rendered-app SSH validation on a throwaway Ubuntu 20.04 host, at exact head: a live PTY survived a 74-second disconnect — past the delivery-reconnect grace — with the same PTY id, same remote PID, output resuming, and no duplicate; and separately, a process killed while the client was disconnected was correctly reported gone on reconnect rather than left looking live, with its sibling pane preserved.
- Behaviour was checked against precedent from comparable terminal and remote-execution tools. The tri-state model matches the consensus: process death may only be concluded from positive evidence produced by the owning host, and loss of contact must land in a preserved, retryable state. No part of this design is without precedent, with one exception now removed — the overflow that disposed the channel (Add Shift+Enter to insert newline in terminal #3).
Known gaps — not addressed here
- Pre-existing, worth a follow-up: an exit tombstone can be erased by live-evidence begun after a confirmed exit, flipping a dead pane back to
live. Confirmed reachable end-to-end on relays without the output-flow-control capability, via two writers that set a tombstone without sealing the intake. Verified bit-identical before and after this PR — not caused or worsened here. Cheap containment would be forbeginLiveEvidenceto refuse when a tombstone exists. - Pre-existing: on reveal, a short main-model snapshot can be painted once and never refreshed, leaving a prefix-only scrollback. Surfaced by a flaky
ssh-terminal-parkingrun at this head; ruled out as a regression — the paths those commits touch did not execute in the failing run, and the same SHA passed on re-run. - Pre-existing: the headless emulator is built with a 5,000-row default while the renderer may request up to 50,000, capping what a large scrollback can restore.
- Unmeasured: performance. This adds two map/set operations and a promise continuation per data frame on the SSH output hot path. Reasoned about, never benchmarked.
- Not exercised: Windows, WSL, and Linux desktop. All validation ran on macOS plus a Linux SSH target.
restoreRequireditself was never observed directly in live QA — those statuses are not logged by the relay, so the log cannot confirm or deny them. The deterministic tests cover that classification; the live runs prove the surrounding invariant.- Left deliberately in place: the unproven rollback before retry in
attachPtyWithDeadline, and two bareacceptLivePtycalls after awaits inSshRelaySession— same shape as Fix MaxListenersExceededWarning with singleton PTY dispatcher #6, out of scope here. - Five files in the diff carry pure formatting reflow with no semantic change.
paidaxingyo666
pushed a commit
to paidaxingyo666/Manta
that referenced
this pull request
Aug 21, 2026
…bulary (stablyai#14971) * docs(ssh): state the SSH execution boundary and pin the liveness vocabulary Nothing under docs/ described how work splits between the client and an SSH host, so agents and humans inferred it from error strings and got it wrong: loss of contact was repeatedly reported as process death, which orphaned live remote agents and cold-started duplicates over the same worktree. Pins the vocabulary to the incumbent live/unverifiable/exited verdict from unstopped-pty-verification so no synonym is introduced, records the one real discriminator (all of a host's terminals drop together on link loss; one alone means process exit), and lists the outstanding gaps with citations. Tracked via the docs allow-list and linked from AGENTS.md, per the convention in .gitignore. * docs(ssh): cite the live restoreRequired site after it moved The throw now lives in reattachSshPtySessionForSpawn; ssh-pty-provider.ts no longer contains it. Caught by the worker fixing it, against a newer main than the audit ran on. * docs(ssh): require host evidence for liveness verdicts * docs(ssh): keep boundary references stable * docs(ssh): fence liveness evidence to its host identity * docs(ssh): state replay and environment boundaries precisely * docs(ssh): correct replay and platform boundary claims * docs(ssh): describe headless runtime continuity accurately * docs(ssh): distinguish authority from client metadata * docs(ssh): describe pending fixes accurately * docs(ssh): date the gap list and name the PR that closes each entry The Known gaps section was accurate when written and becomes actively misleading as its fixes land: it told a reader to go fix restoreRequired, the missing unverifiable verdict, and the absent terminal-list host field, three things now addressed by stablyai#14974, stablyai#14977 and stablyai#14973. Mark the section as dated, require verification against current code before acting on any entry, name the PR per entry, and move landed items out. Also correct the two body claims that the landed fixes invalidated. The rules above are durable; only this section rots. * docs(ssh): make the boundary doc a durable ruleset, not an incident record The Known gaps section was 18 of 93 lines enumerating specific defects from one investigation, several already fixed by sibling PRs in the same batch. A reference doc that needs a 'this section rots' warning is telling you the section belongs somewhere else; those entries belong in issues. Replace the six-row table of currently-lying signals with the method that outlands any particular bug: ask whether the owning host produced the signal, whether every PTY on the target went quiet together, whether the termination event matches the current incarnation and generation, and whether a returned status is actually a claim. Same for artifacts - state what ls-remote and a PR head each do and do not prove, rather than listing which command is currently wrong. Nothing here goes stale when the open fixes land.
dallascrilley
pushed a commit
to dallascrilley/orca
that referenced
this pull request
Aug 27, 2026
…bulary (stablyai#14971) * docs(ssh): state the SSH execution boundary and pin the liveness vocabulary Nothing under docs/ described how work splits between the client and an SSH host, so agents and humans inferred it from error strings and got it wrong: loss of contact was repeatedly reported as process death, which orphaned live remote agents and cold-started duplicates over the same worktree. Pins the vocabulary to the incumbent live/unverifiable/exited verdict from unstopped-pty-verification so no synonym is introduced, records the one real discriminator (all of a host's terminals drop together on link loss; one alone means process exit), and lists the outstanding gaps with citations. Tracked via the docs allow-list and linked from AGENTS.md, per the convention in .gitignore. * docs(ssh): cite the live restoreRequired site after it moved The throw now lives in reattachSshPtySessionForSpawn; ssh-pty-provider.ts no longer contains it. Caught by the worker fixing it, against a newer main than the audit ran on. * docs(ssh): require host evidence for liveness verdicts * docs(ssh): keep boundary references stable * docs(ssh): fence liveness evidence to its host identity * docs(ssh): state replay and environment boundaries precisely * docs(ssh): correct replay and platform boundary claims * docs(ssh): describe headless runtime continuity accurately * docs(ssh): distinguish authority from client metadata * docs(ssh): describe pending fixes accurately * docs(ssh): date the gap list and name the PR that closes each entry The Known gaps section was accurate when written and becomes actively misleading as its fixes land: it told a reader to go fix restoreRequired, the missing unverifiable verdict, and the absent terminal-list host field, three things now addressed by stablyai#14974, stablyai#14977 and stablyai#14973. Mark the section as dated, require verification against current code before acting on any entry, name the PR per entry, and move landed items out. Also correct the two body claims that the landed fixes invalidated. The rules above are durable; only this section rots. * docs(ssh): make the boundary doc a durable ruleset, not an incident record The Known gaps section was 18 of 93 lines enumerating specific defects from one investigation, several already fixed by sibling PRs in the same batch. A reference doc that needs a 'this section rots' warning is telling you the section belongs somewhere else; those entries belong in issues. Replace the six-row table of currently-lying signals with the method that outlands any particular bug: ask whether the owning host produced the signal, whether every PTY on the target went quiet together, whether the termination event matches the current incarnation and generation, and whether a returned status is actually a claim. Same for artifacts - state what ls-remote and a PR head each do and do not prove, rather than listing which command is currently wrong. Nothing here goes stale when the open fixes land.
Collaborator
|
Thank you @brennanb2025. The core of this landed in #17951 (a restore-required reattach is treated as a live PTY, not a dead session), and the relay that produced that response has since been removed in favor of Orca's managed remote server (#27214/#27215). Closing as fixed. |
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
…bulary (stablyai#14971) * docs(ssh): state the SSH execution boundary and pin the liveness vocabulary Nothing under docs/ described how work splits between the client and an SSH host, so agents and humans inferred it from error strings and got it wrong: loss of contact was repeatedly reported as process death, which orphaned live remote agents and cold-started duplicates over the same worktree. Pins the vocabulary to the incumbent live/unverifiable/exited verdict from unstopped-pty-verification so no synonym is introduced, records the one real discriminator (all of a host's terminals drop together on link loss; one alone means process exit), and lists the outstanding gaps with citations. Tracked via the docs allow-list and linked from AGENTS.md, per the convention in .gitignore. * docs(ssh): cite the live restoreRequired site after it moved The throw now lives in reattachSshPtySessionForSpawn; ssh-pty-provider.ts no longer contains it. Caught by the worker fixing it, against a newer main than the audit ran on. * docs(ssh): require host evidence for liveness verdicts * docs(ssh): keep boundary references stable * docs(ssh): fence liveness evidence to its host identity * docs(ssh): state replay and environment boundaries precisely * docs(ssh): correct replay and platform boundary claims * docs(ssh): describe headless runtime continuity accurately * docs(ssh): distinguish authority from client metadata * docs(ssh): describe pending fixes accurately * docs(ssh): date the gap list and name the PR that closes each entry The Known gaps section was accurate when written and becomes actively misleading as its fixes land: it told a reader to go fix restoreRequired, the missing unverifiable verdict, and the absent terminal-list host field, three things now addressed by stablyai#14974, stablyai#14977 and stablyai#14973. Mark the section as dated, require verification against current code before acting on any entry, name the PR per entry, and move landed items out. Also correct the two body claims that the landed fixes invalidated. The rules above are durable; only this section rots. * docs(ssh): make the boundary doc a durable ruleset, not an incident record The Known gaps section was 18 of 93 lines enumerating specific defects from one investigation, several already fixed by sibling PRs in the same batch. A reference doc that needs a 'this section rots' warning is telling you the section belongs somewhere else; those entries belong in issues. Replace the six-row table of currently-lying signals with the method that outlands any particular bug: ask whether the owning host produced the signal, whether every PTY on the target went quiet together, whether the termination event matches the current incarnation and generation, and whether a returned status is actually a claim. Same for artifacts - state what ls-remote and a PR head each do and do not prove, rather than listing which command is currently wrong. Nothing here goes stale when the open fixes land.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Invariant
SSH PTY liveness has three distinct outcomes:
live,unverifiable, andexited. Only positive host evidence may produceexited; delivery or transport failure remainsunverifiableand must not trigger a replacement PTY.Defect
The relay's
restoreRequiredresponse means its output-delivery record cannot be resumed. It does not terminate the managed PTY. Orca relabelled that response asSSH_SESSION_EXPIRED, expired the detached lease, cleared the renderer binding, and could cold-start a duplicate agent over the same worktree while the original remained live.Fix
restoreRequired, after rolling back the stale source-activation lease.restoreRequiredaslive; keep the remote lease detached and the renderer binding intact.unverifiablethrough a distinct error token and tri-state liveness probe. Preserve routing ownership and never cold-spawn a replacement.exitedonly for authoritative PTY absence or an observed exit.unverifiableuntil reattach proves their state.unverifiable.No stream opcode, RPC schema, persisted schema, mobile-facing surface, dependency, polling loop, or platform-specific shell/path behavior changed.
Verification
pnpm lintpnpm typecheckgit diff --checkThe 12-file renderer reliability batch remains sensitive to shared-machine contention: its latest combined run passed 100/110 and timed out or cascaded in four files, while every failing file passed in isolation.
UI and real SSH QA
Electron was exercised over CDP against a real key-only Ubuntu 20.04 ARM64 SSH target using the built relay. The rendered remote workspace was opened, terminal input executed on the host, a remote marker was read back, the exact review worktree identity was verified, and the remote Git worktree stayed clean. Screenshot and command evidence are attached in the QA comment.
The exact live
restoreRequiredfault was not injected on the SSH rig; that branch is covered deterministically by provider, IPC, relay, and renderer tests.