Repository navigation
test(ssh): dockerized relay fault injection with verdict assertions - #18017
Merged
Merged
Conversation
…shapes The existing SSH reconnect specs all reconnect by calling ssh.disconnect() then ssh.connect() - a clean cycle the client knows is coming. Nothing covered the faults the reconnect machinery exists for. Four shapes, each documented with why it is not the others: killing sshd's per-connection forks (transport dies, relay survives), `docker pause` (silence with TCP still established), SIGKILLing every relay.js (the only fault where `exited` is the correct verdict), and a 48MB flood with nobody attached. The relay-kill case is the one that makes the rest meaningful: every other case asserts the session survived, which only means something if a genuinely dead session is distinguishable. It is the only case where replacing the pane is correct, so it pins the boundary in docs/reference/ssh-execution-boundary.md rather than just testing reconnection. The `docker pause` case pins the other side of that boundary: after 30s of silence from a healthy host the pane keeps its PTY and its scrollback, because loss of contact is never evidence of death. No network-blackhole fault: reconnecting the fixture does not restore its published port mapping, so that fault is not reversible on this container and would strand the worker it ran on.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughAdds Docker SSH relay fault helpers and a gated Playwright spec. The tests cover transport drops, output flooding, relay daemon termination, paused targets, and a known paused-host input failure. The spec is added to SSH source routing, the gate contract mapping, and the SSH Docker E2E execution list. Merge Risk: ⚪ Minimal · up to This change adds Docker-based SSH fault-injection coverage and explicit assertions distinguishing session resumption from replacement, without changing production runtime behavior or public interfaces. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and explains the scope, rationale, test coverage, defects found, deliberate reductions, gate wiring, and user-facing impact. It references related issues and provides test results. It does not use all template headings, does not explicitly state visual proof as N/A, and omits the checklist sections, but the core required information is present.
✨ 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}
Contributor
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: 38a7e86b-11a6-41c3-a09d-b04f8db71050
📒 Files selected for processing (5)
config/scripts/pr-e2e-gate-contract.test.mjsconfig/scripts/pr-e2e-source-routing.mjsconfig/scripts/run-ssh-docker-e2e.mjstests/e2e/helpers/docker-ssh-relay-faults.tstests/e2e/ssh-docker-transport-drop-recovery.spec.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
| .toBe('connected') | ||
|
|
||
| await waitForActiveTerminalManager(orcaPage, 60_000) | ||
| await waitForActivePanePtyId(orcaPage, 60_000) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the original PTY after transport recovery.
Both tests record the original ptyId, but neither compares it after recovery. A replacement pane can retain the old scrollback marker, and later commands run against the replacement PTY, so a cold start passes as a resumed session.
tests/e2e/ssh-docker-transport-drop-recovery.spec.ts#L93-L93: assert that the recovered active PTY equalsptyId.tests/e2e/ssh-docker-transport-drop-recovery.spec.ts#L181-L181: assert that the active PTY equalsptyIdbefore sending the post-flood command.
📍 Affects 1 file
tests/e2e/ssh-docker-transport-drop-recovery.spec.ts#L93-L93(this comment)tests/e2e/ssh-docker-transport-drop-recovery.spec.ts#L181-L181
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ℹ️ Solid, well-documented test-only PR — two inline comments worth addressing, nothing blocking.
Reviewed changes
tests/e2e/helpers/docker-ssh-relay-faults.ts— a new fault-injection helper with four fault shapes (sshd-fork transport drop,docker pausefreeze, relay SIGKILL, and a fault-clearing helper), each documented with why it is distinct from the others.tests/e2e/ssh-docker-transport-drop-recovery.spec.ts— five specs exercising those faults (four passing, onetest.fixmereproduction of the "live-looking but deaf" post-freeze defect), asserting theresumedvscold-startverdicts through terminal content and pty stability rather than connection status.- Three one-line gate entries wiring the new spec into
pr-e2e-gate-contract.test.mjs,pr-e2e-source-routing.mjs, andrun-ssh-docker-e2e.mjs.
ℹ️ Nitpicks
- The
test.fixmereproduction atssh-docker-transport-drop-recovery.spec.ts:313documents the defect it found in depth but carries no tracking-issue number; linking one would make the deliberate skip traceable for whoever picks up the fix.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
Contributor
|
Two inline comments that belong with the review above (the review API deduped a second submission, so posting here): 1. The verdict here is 2.
Neither is blocking — both are
|
4 of 5 tasks
Contributor
Author
|
Merge-blocking request before this lands: Please mark that one case |
It fails in CI on its first real run: the pane keeps its PTY and repaints, but a command run after the flood produces no output within the poll budget. Same shape as #18018 and not caused by this spec. The three verdict assertions around it stay enforced.
Contributor
There was a problem hiding this comment.
Important
Marking the flood spec test.fixme also disables its RSS OOM guard, which is the test's stated purpose and appears to still pass — see inline.
Reviewed changes
test.fixmeon the flood spec — the "stays bounded when a disconnected shell floods its pty" spec was markedtest.fixme(tracked in #18018) after its first CI run failed on the trailing post-flood usability assertion, leaving the three verdict specs enforced.
ℹ️ Nitpicks
- The new comment attributes the flood failure to #18018 ("deaf pane after a stalled host resumes"), but the sibling
test.fixmeatssh-docker-transport-drop-recovery.spec.ts:317("accepts input again after a frozen host resumes") is the canonical reproduction of that exact defect and still carries no tracking-issue reference; linking #18018 there would close the loop flagged in the prior review.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| // run after the flood produces no output within the poll budget. Same shape as #18018 (deaf pane | ||
| // after a stalled host resumes), and not caused by this spec. Tracked there; the three verdict | ||
| // assertions around it stay enforced. | ||
| test.fixme('stays bounded when a disconnected shell floods its pty', async ({ orcaPage }, testInfo) => { |
Contributor
There was a problem hiding this comment.
test.fixme disables the entire flood spec, including the RSS OOM guard at line 178 (.toBeLessThan(200_000)) — which is the test's stated purpose and appears to still pass, since the CI failure is only the trailing "command produces output" check. Split the passing RSS assertion from the failing usability check the way the stall verdict was split into specs 4 and 5, so the memory guard stays enforced in CI.
Technical details
# Preserve the flood spec's RSS OOM guard
## Affected sites
- tests/e2e/ssh-docker-transport-drop-recovery.spec.ts:120 — `test.fixme` now disables the whole flood spec.
- tests/e2e/ssh-docker-transport-drop-recovery.spec.ts:175-178 — the RSS bound assertion (`.toBeLessThan(200_000)`), the test's OOM guard.
- tests/e2e/ssh-docker-transport-drop-recovery.spec.ts:180-188 — the trailing `FLOOD_AFTER` usability assertion that actually fails in CI.
## Required outcome
- The RSS memory bound stays enforced in CI.
- Only the currently-failing usability check is `test.fixme`.
## Suggested approach (optional)
- Split into two specs, mirroring the stall split into specs 4/5: one passing spec that floods, drops transport, and asserts `afterRssKb - baselineRssKb < 200_000`; one `test.fixme` that re-runs the flood/drop and asserts the `FLOOD_AFTER` marker.
- Re-running the flood doubles the ~60s runtime; if that cost is unacceptable, keep one spec and hoist the RSS assertion so it still executes before the failing check — but note `test.fixme` suppresses failures even for assertions that pass today.
nwparker
added a commit
that referenced
this pull request
Sep 2, 2026
Gate 3 refused to respawn a pane whose records still named a PTY, which is right for a transport drop and wrong for a killed relay: after the relay is SIGKILLed and comes back, the leaf map still holds `pty2:<dead-epoch>:1` while the new relay answers that it has no such id. #18017's "replaces the pane only when the host proves the session is gone" regressed on exactly that. The gap was not the predicate, it was its inputs. `handlePtyReattachFailure` already distinguishes the three reattach outcomes and only its not-found branch publishes anything — a lost link and an identity mismatch send nothing. But it published `pty:exit { code: -1 }`, and `-1` is the sentinel every reader resolves to `stop_unverified`, so the one branch holding positive host evidence of absence arrived looking exactly like loss of contact. The renderer had no host answer at all, which the gate's own comment conceded. The exit now carries `livenessVerdict: 'exited'` beside the unchanged `-1`, so the code keeps meaning "no provable status" for every existing reader while the verdict rides its own field. A store bridge records those ids in `hostAttestedAbsentPtyIds` regardless of whether a pane is mounted to hear it — during reconnect none is — and the gate stops counting a recorded id the host has disowned. Settled when a PTY answers to that id again, because a redeployed relay renumbers from pty-1. This narrows #17963, which pinned the same exit as unverified on the grounds that not-found cannot separate "verified the pid is dead" from "my session map never had this id". Everything #17963 protects is untouched: `-1` still fails isProvenProcessExit, so the tab is not closed, the pane's leaf binding is not dropped on exit, and markUnverifiedPtyLoss still fires. Only the reconnect respawn gate reads the new field, and only for an id whose sole channel — the relay that answered — has disowned it, which no client can reach again under any verdict. That is the reading ssh-pty-relay-absence-verdict.test.ts already pins for the spawn path; the reconnect path now agrees with it. Rejected: parsing the relay's mint epoch out of `pty2:<epoch>:<n>`. It needs the current epoch on the wire (a capability-negotiated relay change), it has no answer for legacy `pty-N` ids, and a relay that comes back with zero PTYs gives the client no epoch to compare against. Rejected: clearing the leaf record outright, because the remote workspace snapshot re-hydrates those ids after the clear and the gate would refuse again.
Contributor
|
thanks for reporting. looking into it |
nwparker
added a commit
that referenced
this pull request
Sep 3, 2026
Gate 3 refused to respawn a pane whose records still named a PTY, which is right for a transport drop and wrong for a killed relay: after the relay is SIGKILLed and comes back, the leaf map still holds `pty2:<dead-epoch>:1` while the new relay answers that it has no such id. #18017's "replaces the pane only when the host proves the session is gone" regressed on exactly that. The gap was not the predicate, it was its inputs. `handlePtyReattachFailure` already distinguishes the three reattach outcomes and only its not-found branch publishes anything — a lost link and an identity mismatch send nothing. But it published `pty:exit { code: -1 }`, and `-1` is the sentinel every reader resolves to `stop_unverified`, so the one branch holding positive host evidence of absence arrived looking exactly like loss of contact. The renderer had no host answer at all, which the gate's own comment conceded. The exit now carries `livenessVerdict: 'exited'` beside the unchanged `-1`, so the code keeps meaning "no provable status" for every existing reader while the verdict rides its own field. A store bridge records those ids in `hostAttestedAbsentPtyIds` regardless of whether a pane is mounted to hear it — during reconnect none is — and the gate stops counting a recorded id the host has disowned. Settled when a PTY answers to that id again, because a redeployed relay renumbers from pty-1. This narrows #17963, which pinned the same exit as unverified on the grounds that not-found cannot separate "verified the pid is dead" from "my session map never had this id". Everything #17963 protects is untouched: `-1` still fails isProvenProcessExit, so the tab is not closed, the pane's leaf binding is not dropped on exit, and markUnverifiedPtyLoss still fires. Only the reconnect respawn gate reads the new field, and only for an id whose sole channel — the relay that answered — has disowned it, which no client can reach again under any verdict. That is the reading ssh-pty-relay-absence-verdict.test.ts already pins for the spawn path; the reconnect path now agrees with it. Rejected: parsing the relay's mint epoch out of `pty2:<epoch>:<n>`. It needs the current epoch on the wire (a capability-negotiated relay change), it has no answer for legacy `pty-N` ids, and a relay that comes back with zero PTYs gives the client no epoch to compare against. Rejected: clearing the leaf record outright, because the remote workspace snapshot re-hydrates those ids after the clear and the gate would refuse again.
nwparker
added a commit
that referenced
this pull request
Sep 3, 2026
…17957) * fix(ssh): stop respawning panes on client-side-only absence evidence Three respawn gates acted on evidence weaker than host-attested exit. Per docs/reference/ssh-execution-boundary.md, loss of contact, a failed reattach, an identity mismatch and absence from a client map are all `unverifiable`, never `exited`. Gate 1 (ipc-pty-connect.ts): "belongs to SSH connection" is minted by the id router from a pure client-side string compare, before any relay is asked, and still returned `sessionExpired: true` -> fresh PTY + agent resume. After an SSH target re-adoption the "other" connection is the same machine, so that puts a second `claude --resume` on the transcript the surviving PTY still owns. Now returns undefined with no error, which routes the pane to recoverUnverifiableDirectSshReattach (remount + reattach, no shell restart) and keeps #7661's no-red-toast outcome. Gate 3 (ssh-reconnect-pane-retry.ts): `!tabPtyId` read `tab.ptyId`, which is only the single-pane fallback for legacy attach. It diverges from the real records deterministically: workspace-terminal-reconnect fills ptyIdsByTabId from the leaf map but writes tab.ptyId only when a tab-level id survives, and clearTransientTerminalState nulls tab.ptyId on every hydrated row. Both leave live leaf PTYs with a null fallback field, arming a generation bump onto the fresh-spawn path. Now consults ptyIdsByTabId and the layout leaf map too; a tab with no PTY in any record still retries. Gate 2 (recoverTerminalPane): an `expired` lease plus `!pty.connected` authorized createTerminal. Every writer of `expired` records that the CLIENT lost its route, not that the shell died. Now also requires the runtime's own liveness verdict to be neither `live` nor `unverifiable`, and ssh-relay-session records markPtyLivenessLive at the persistPtyBinding refusal, which is reached only after pty.attach succeeded. See the report for why this branch is currently unreachable for SSH panes. * fix(ssh): let the respawn gate see the relay's own absence answer Gate 3 refused to respawn a pane whose records still named a PTY, which is right for a transport drop and wrong for a killed relay: after the relay is SIGKILLed and comes back, the leaf map still holds `pty2:<dead-epoch>:1` while the new relay answers that it has no such id. #18017's "replaces the pane only when the host proves the session is gone" regressed on exactly that. The gap was not the predicate, it was its inputs. `handlePtyReattachFailure` already distinguishes the three reattach outcomes and only its not-found branch publishes anything — a lost link and an identity mismatch send nothing. But it published `pty:exit { code: -1 }`, and `-1` is the sentinel every reader resolves to `stop_unverified`, so the one branch holding positive host evidence of absence arrived looking exactly like loss of contact. The renderer had no host answer at all, which the gate's own comment conceded. The exit now carries `livenessVerdict: 'exited'` beside the unchanged `-1`, so the code keeps meaning "no provable status" for every existing reader while the verdict rides its own field. A store bridge records those ids in `hostAttestedAbsentPtyIds` regardless of whether a pane is mounted to hear it — during reconnect none is — and the gate stops counting a recorded id the host has disowned. Settled when a PTY answers to that id again, because a redeployed relay renumbers from pty-1. This narrows #17963, which pinned the same exit as unverified on the grounds that not-found cannot separate "verified the pid is dead" from "my session map never had this id". Everything #17963 protects is untouched: `-1` still fails isProvenProcessExit, so the tab is not closed, the pane's leaf binding is not dropped on exit, and markUnverifiedPtyLoss still fires. Only the reconnect respawn gate reads the new field, and only for an id whose sole channel — the relay that answered — has disowned it, which no client can reach again under any verdict. That is the reading ssh-pty-relay-absence-verdict.test.ts already pins for the spawn path; the reconnect path now agrees with it. Rejected: parsing the relay's mint epoch out of `pty2:<epoch>:<n>`. It needs the current epoch on the wire (a capability-negotiated relay change), it has no answer for legacy `pty-N` ids, and a relay that comes back with zero PTYs gives the client no epoch to compare against. Rejected: clearing the leaf record outright, because the remote workspace snapshot re-hydrates those ids after the clear and the gate would refuse again. * refactor(ssh): name the relay-disowned signal for disownership, not exit
paidaxingyo666
pushed a commit
to paidaxingyo666/Manta
that referenced
this pull request
Sep 3, 2026
…tablyai#18017) * test(ssh): add a dockerized SSH fault-injection lane with four fault shapes The existing SSH reconnect specs all reconnect by calling ssh.disconnect() then ssh.connect() - a clean cycle the client knows is coming. Nothing covered the faults the reconnect machinery exists for. Four shapes, each documented with why it is not the others: killing sshd's per-connection forks (transport dies, relay survives), `docker pause` (silence with TCP still established), SIGKILLing every relay.js (the only fault where `exited` is the correct verdict), and a 48MB flood with nobody attached. The relay-kill case is the one that makes the rest meaningful: every other case asserts the session survived, which only means something if a genuinely dead session is distinguishable. It is the only case where replacing the pane is correct, so it pins the boundary in docs/reference/ssh-execution-boundary.md rather than just testing reconnection. The `docker pause` case pins the other side of that boundary: after 30s of silence from a healthy host the pane keeps its PTY and its scrollback, because loss of contact is never evidence of death. No network-blackhole fault: reconnecting the fixture does not restore its published port mapping, so that fault is not reversible on this container and would strand the worker it ran on. * test(ssh): fixme the flood case pending stablyai#18018 It fails in CI on its first real run: the pane keeps its PTY and repaints, but a command run after the flood produces no output within the poll budget. Same shape as stablyai#18018 and not caused by this spec. The three verdict assertions around it stay enforced. Mirror-Of: 0dbe9d0
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
…tablyai#18017) * test(ssh): add a dockerized SSH fault-injection lane with four fault shapes The existing SSH reconnect specs all reconnect by calling ssh.disconnect() then ssh.connect() - a clean cycle the client knows is coming. Nothing covered the faults the reconnect machinery exists for. Four shapes, each documented with why it is not the others: killing sshd's per-connection forks (transport dies, relay survives), `docker pause` (silence with TCP still established), SIGKILLing every relay.js (the only fault where `exited` is the correct verdict), and a 48MB flood with nobody attached. The relay-kill case is the one that makes the rest meaningful: every other case asserts the session survived, which only means something if a genuinely dead session is distinguishable. It is the only case where replacing the pane is correct, so it pins the boundary in docs/reference/ssh-execution-boundary.md rather than just testing reconnection. The `docker pause` case pins the other side of that boundary: after 30s of silence from a healthy host the pane keeps its PTY and its scrollback, because loss of contact is never evidence of death. No network-blackhole fault: reconnecting the fixture does not restore its published port mapping, so that fault is not reversible on this container and would strand the worker it ran on. * test(ssh): fixme the flood case pending stablyai#18018 It fails in CI on its first real run: the pane keeps its PTY and repaints, but a command run after the flood produces no output within the poll budget. Same shape as stablyai#18018 and not caused by this spec. The three verdict assertions around it stay enforced.
enrick-dev
pushed a commit
to enrick-dev/orca
that referenced
this pull request
Oct 15, 2026
…tablyai#17957) * fix(ssh): stop respawning panes on client-side-only absence evidence Three respawn gates acted on evidence weaker than host-attested exit. Per docs/reference/ssh-execution-boundary.md, loss of contact, a failed reattach, an identity mismatch and absence from a client map are all `unverifiable`, never `exited`. Gate 1 (ipc-pty-connect.ts): "belongs to SSH connection" is minted by the id router from a pure client-side string compare, before any relay is asked, and still returned `sessionExpired: true` -> fresh PTY + agent resume. After an SSH target re-adoption the "other" connection is the same machine, so that puts a second `claude --resume` on the transcript the surviving PTY still owns. Now returns undefined with no error, which routes the pane to recoverUnverifiableDirectSshReattach (remount + reattach, no shell restart) and keeps stablyai#7661's no-red-toast outcome. Gate 3 (ssh-reconnect-pane-retry.ts): `!tabPtyId` read `tab.ptyId`, which is only the single-pane fallback for legacy attach. It diverges from the real records deterministically: workspace-terminal-reconnect fills ptyIdsByTabId from the leaf map but writes tab.ptyId only when a tab-level id survives, and clearTransientTerminalState nulls tab.ptyId on every hydrated row. Both leave live leaf PTYs with a null fallback field, arming a generation bump onto the fresh-spawn path. Now consults ptyIdsByTabId and the layout leaf map too; a tab with no PTY in any record still retries. Gate 2 (recoverTerminalPane): an `expired` lease plus `!pty.connected` authorized createTerminal. Every writer of `expired` records that the CLIENT lost its route, not that the shell died. Now also requires the runtime's own liveness verdict to be neither `live` nor `unverifiable`, and ssh-relay-session records markPtyLivenessLive at the persistPtyBinding refusal, which is reached only after pty.attach succeeded. See the report for why this branch is currently unreachable for SSH panes. * fix(ssh): let the respawn gate see the relay's own absence answer Gate 3 refused to respawn a pane whose records still named a PTY, which is right for a transport drop and wrong for a killed relay: after the relay is SIGKILLed and comes back, the leaf map still holds `pty2:<dead-epoch>:1` while the new relay answers that it has no such id. stablyai#18017's "replaces the pane only when the host proves the session is gone" regressed on exactly that. The gap was not the predicate, it was its inputs. `handlePtyReattachFailure` already distinguishes the three reattach outcomes and only its not-found branch publishes anything — a lost link and an identity mismatch send nothing. But it published `pty:exit { code: -1 }`, and `-1` is the sentinel every reader resolves to `stop_unverified`, so the one branch holding positive host evidence of absence arrived looking exactly like loss of contact. The renderer had no host answer at all, which the gate's own comment conceded. The exit now carries `livenessVerdict: 'exited'` beside the unchanged `-1`, so the code keeps meaning "no provable status" for every existing reader while the verdict rides its own field. A store bridge records those ids in `hostAttestedAbsentPtyIds` regardless of whether a pane is mounted to hear it — during reconnect none is — and the gate stops counting a recorded id the host has disowned. Settled when a PTY answers to that id again, because a redeployed relay renumbers from pty-1. This narrows stablyai#17963, which pinned the same exit as unverified on the grounds that not-found cannot separate "verified the pid is dead" from "my session map never had this id". Everything stablyai#17963 protects is untouched: `-1` still fails isProvenProcessExit, so the tab is not closed, the pane's leaf binding is not dropped on exit, and markUnverifiedPtyLoss still fires. Only the reconnect respawn gate reads the new field, and only for an id whose sole channel — the relay that answered — has disowned it, which no client can reach again under any verdict. That is the reading ssh-pty-relay-absence-verdict.test.ts already pins for the spawn path; the reconnect path now agrees with it. Rejected: parsing the relay's mint epoch out of `pty2:<epoch>:<n>`. It needs the current epoch on the wire (a capability-negotiated relay change), it has no answer for legacy `pty-N` ids, and a relay that comes back with zero PTYs gives the client no epoch to compare against. Rejected: clearing the leaf record outright, because the remote workspace snapshot re-hydrates those ids after the clear and the gate would refuse again. * refactor(ssh): name the relay-disowned signal for disownership, not exit
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.
Salvaged from #17786 (see the review there). Test-only, independently mergeable, and it found a real defect on
main.What this is
A Docker SSH fault-injection harness with four genuinely distinct fault shapes, each documented with why it is not the others:
sshdandsshd-session(the OpenSSH ≥9.8 rename).docker pause— silence with TCP still established. The laptop-lid shape; the only fault that exposes a liveness timeout firing on a healthy session.relay.js— the only fault where an explicit session-expired state is correct rather than resuming.Verbatim final run
Tests 3 and 4 are new. #17786's spec used one fault three ways and never asserted the verdict vocabulary at all — it read
sshConnectionStates.status, a connection status, not a PTY verdict.killDockerSshRelayDaemonand asserts theexitedverdict (pane bound to a different PTY).docker pauseand assertsunverifiable(same PTY, scrollback intact).That distinction — resumed vs silently started over — is what this whole sweep turns on, and it was previously untestable.
A real defect found, and deliberately left failing
Test 4's last assertion fails on
main: after a 30s freeze the pane keeps its PTY and repaints its scrollback, but a command run afterwards produces no output within 60s. Live-looking and deaf.Cherry-checked against #18016 applied:
✘ Error: Terminal did not contain "STALL_AFTER_1788297964282"— identical failure, so the delivery-ownership fix is not the cause.I split the passing verdict assertions from the failing one, so the
unverifiableverdict stays enforced in CI rather than being masked by a broken sibling, and left the reproduction astest.fixmewith the measurements attached.Deliberate reductions from #17786's version
docker network connectdoes not restore the fixture's published port mapping, so that fault is not reversible on this container and would strand the worker. That also moots the hardcoded-bridgebug inclearDockerSshRelayFaults: the helper reduces tounpause, which is exactly what the surviving faults need.releaseUndeliverableOutput, which is not onmainand is part of fix(ssh): declare a wedged relay link lost, and stop reading silence as a verdict #17817's relay-drain work.docs/ssh-v3-redesign.html, which does not exist.config/reliability-gates.jsoncuntouched — fix(ssh): fence reattach restore and relay backpressure #17786 rewrites an unrelated historical ws-transport heartbeat gate record there. Confirmed and excluded.Every remaining export is now exercised. #17786 shipped 9 exports with 2 used.
Gate wiring
Three one-line additions (
pr-e2e-gate-contract,pr-e2e-source-routing,run-ssh-docker-e2e);pr-e2e-gate-contract.test.mjs42 passed.Worth knowing: the spec was already wired to run in #17786 — the source-routing gate matches its paths and
e2e.ymlauto-setsORCA_E2E_SSH_DOCKER=1— butreliability-gates.jsoncrecords no run evidence, so it appears never to have executed. The run above is its first real signal.User-facing regressions
None — test-only.