Repository navigation
feat(ssh): wire SshConnection through the work and transport close ledgers (#16741 T2 P2) - #24401
Conversation
…dgers (#16741 T2 P2) Every operation SshConnection admits (exec, shell, sftp, file transfers, upload sessions, forwarded channels and sockets, system-SSH commands) now runs through the connection's work ledger, and every ssh2 client and proxy process it allocates is tracked until it physically closes. Ordinary connect, reconnect and disconnect behavior is unchanged. Adds: - subscribeTransportClosure: one-shot notice once the connection is disposed, every allocated transport has emitted 'close' and tracked work has drained. System-SSH startup is never proven closed from here. - disconnectAndDrain(signal): for owned single-lifetime transports; fences new work, disconnects, and waits for physical close of the client, proxy, every allocated client and all fenced work. Refuses (after cleaning up) when the transport cannot be proven, e.g. system SSH or a connect still in flight. - getExecutionDestination: the ssh2 endpoint, accepted host-key fingerprint and proxy-route digest proven by the current handshake (ssh-connection-destination). - getTransportGeneration, prepareForwardRoute, openForwardSocket, forwardOut, forwardStreamLocal for later forwarding callers. - An automaticReconnect constructor option (default on). Channel close is local lifetime evidence only, never a remote-exit verdict. Porting note (source: #16741 head a68b6f3, merge-base 277c289): - Taken: the ledger hunks of ssh-connection.ts, ssh-connection-destination, ssh-forward-channel-lifetime, ssh-upload-session-lifetime, the system-SSH facade EOF hunk, and their tests. - Adapted: operation bodies became private *Untracked methods called through the ledger instead of being re-indented; closure gating and the close drain moved to ssh-connection-transport-closure / ssh-connection-close-drain; the destination parser uses type guards instead of a cast. disconnectAndDrain fences through the ledger directly. Main's plain-SSH shell() goes through the ledger too. execCommand takes Pick<SshConnection, 'exec' | 'usesSystemSshTransport'>; its string-stdin/maxOutputBytes hunk is not taken because main already streams stdin. Work-drain tests fence the private ledger until T3 adds the public fence; system-SSH drain cases split into their own file. - Left for later slices: fenceWorkForReset (T3), isEphemeralRuntimeSshOwner (T6), assertProfileLifetimeAdmission (P8b), and the four manager drain cases in ssh-connection-disconnect-drain.test.ts (P3).
…channel errors ssh-connection.ts no longer grows under its max-lines exemption: it is 1872 lines, below main's 1917. The public API is unchanged. - ssh-channel-open.ts: the channel-open waiter and session-limit retry. - ssh-connection-file-transfers.ts: the uploadDirectory, downloadFile, upload session, writeFile and writeBuffer bodies, reading the connection through a small getter-based host so each read still sees the live transport. - ssh-forward-channel-lifetime.ts: the forward client and stream-local checks. The lifetime tracker's 'error' listener no longer hides errors. When it is a channel's only error listener, the error is reported: SshConnection logs "[ssh] Unhandled <kind> channel error for <target>: <message>", and other callers fall back to a generic [ssh] warning. Nothing throws, so an orphaned channel error still cannot crash the process.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — two defer-able observations to confirm before this connects up with later slices.
Reviewed changes
- Work-ledger wiring in
SshConnection—exec,shell,sftp,uploadDirectory,downloadFile,writeFile,writeBuffer, upload sessions and system-ssh commands now run throughworkLedger, with each original body kept as a private*Untrackedmethod. - Transport-close ledger — every ssh2
Clientand proxy process is tracked until itsclose, including failed startups. - New, not-yet-called surface —
subscribeTransportClosure,disconnectAndDrain,getExecutionDestination,getTransportGeneration,prepareForwardRoute/openForwardSocket/forwardOut/forwardStreamLocal, plus anautomaticReconnectconstructor option (defaulttrue) and aresetConnectionFencedlatch. - System-ssh EOF — the exec-channel
Duplexnow forwardsfinaltoproc.stdin.end, so EOF reaches the remote reader. - Destination capture — the accepted host-key fingerprint, endpoint and proxy-route digest from a successful ssh2 handshake are published as a frozen snapshot.
- Tests — new drain/closure/destination/upload-session/forward-lifetime suites; updates to channel-open, direct-startup, file-transfer, host-key-store, reconnect-ladder, MFA and system-transport tests.
All 3,175 tests in src/main/ssh pass locally.
ℹ️ Post-disconnect operations now reject with AbortError
The ledger's admission hook was wired as if (this.disposed) throw createSshOperationAbortError(), so once disconnect() runs, every new operation — exec, sftp, uploadDirectory, writeFile, upload sessions, forwarding — rejects with AbortError instead of the previous 'Not connected' / 'Connection disposed'. That is a wider contract change than the PR's "behavior is the same for users" framing, and only the upload-session path has a test pinning it. Worth confirming that callers which distinguish "transport lost → reconnect" from "user cancelled" do not now read a disposed connection as a cancellation.
Technical details
# Disposed-connection operations change error type
## Affected sites
- `src/main/ssh/ssh-connection.ts` — `workLedger` constructor passes `assertAdmission` that throws `createSshOperationAbortError()` when `this.disposed`; `admit()` calls it for `run`, `beginSession` and `beginChannelOpen`.
- `src/main/ssh/ssh-connection.ts` (`exec`/`sftp`/`uploadDirectory`/`downloadFile`/`writeFile`/`writeBuffer`/`openFileUploadSession`/`forwardOut`/`forwardStreamLocal`) — previously reached `execUntracked` etc. and threw `'Not connected'` after disconnect.
- `src/main/ssh/ssh-connection-file-transfer.test.ts:185` — only coverage of the new behavior (underlying transfer not called; rejects `AbortError`).
## Required outcome
- Confirm that treating a disposed connection as "operation aborted" is intended for all callers, or keep the prior `'Not connected'` classification for non-abort cases.
## Suggested approach (optional)
- If the change is intended, add a direct test for `exec`/`sftp` after `disconnect()` so the contract is pinned, and note the taxonomy change in the PR description.
## Open questions for the human
- Do any relay/reconnect paths key on `'Not connected'` or `'Connection disposed'` to decide recovery?ℹ️ unprovenStartupTransport is sticky even when the connection ends up on ssh2
unprovenStartupTransport is set in doSystemSshProbe, spawnAndWaitForSystemSsh and connectTrackedViaSystemSsh and never cleared. The GSSAPI path can run the system-SSH probe, catch its failure, reset useSystemSshTransport = false, and then connect successfully over ssh2 — leaving the flag permanently true for that instance. From then on disconnectAndDrain always refuses with ssh_connection_close_transport_unproven and subscribeTransportClosure never fires, even though the live transport is provable ssh2. Nothing consumes these yet, but the first later slice that does will silently refuse to drain such a host.
Technical details
# Sticky unproven flag after a failed system-SSH probe
## Affected sites
- `src/main/ssh/ssh-connection.ts:1274` (`doSystemSshProbe`), `:1435` (`spawnAndWaitForSystemSsh`), `:2078` (`connectTrackedViaSystemSsh`) — set `unprovenStartupTransport = true`; no assignment back to `false` exists.
- `src/main/ssh/ssh-connection.ts:1029-1048` — GSSAPI attempt probes system SSH, and on a non-disposed failure resets `useSystemSshTransport` and falls through to the ssh2 path.
- `src/main/ssh/ssh-connection.ts:2135-2147` (`disconnectAndDrain`) and `:229-238` (`transportClosure` predicate) read the flag.
## Required outcome
- Decide whether a cleaned-up, failed system-SSH probe should permanently disqualify the connection from proven closure, or whether the flag can be scoped to a currently-live system transport.
## Open questions for the human
- Is "any system-ssh child ever spawned" the intended granularity, or only "a system-ssh transport is live at drain time"?ℹ️ Nitpicks
getTransportGeneration()'s doc says it is "for callers that compare transport incarnations rather than connect attempts", but it returnsconnectGeneration, which also advances on failed attempts and ondisconnect(). A caller using it as a transport-incarnation key would see a new value with no new transport.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Important
Two CI gates are red on 15329c7: pnpm run typecheck and pnpm run check:code-quality:changed. Both look mechanical, but the PR cannot merge green until they are fixed.
Reviewed changes (delta since the prior review at a3e2cee)
- Channel-open extraction —
openSshSessionChannelWithRetryandwaitForSshChannelOpenmoved out ofssh-connection.tsintossh-channel-open.ts; the logic is a faithful move, now taking the ledger work item and anonUnhandledErrorreporter. - File-transfer extraction — the five transfer bodies moved into
ssh-connection-file-transfers.tsbehind aSshFileTransferHostgetter interface; theSshConnectionmethods are thinworkLedger.runwrappers.ssh-connection.tsis now 1,872 lines. - Forward guards —
assertSshForwardClient/assertSshStreamLocalForwardAllowedextracted intossh-forward-channel-lifetime.ts. - Unhandled channel errors —
trackSshConnectionChannelLifetimenow reports via anonUnhandledErrorcallback when the tracker's listener is the channel's onlyerrorlistener;SshConnectionsupplies a kind-labelled reporter ([ssh] Unhandled exec channel error for <target>: …). Newssh-connection-channel-error.test.tscovers the handled and unhandled cases. - Doc fix —
getTransportGenerationno longer claims to be a distinct transport-incarnation key (addresses an earlier nitpick).
The refactor is behavior-preserving and I found no new logic bug in it. The full src/main/ssh suite (3,180 tests) passes.
⚠️ pnpm run typecheck fails with a consistent-type-assertions-adjacent type error
pnpm tc:node fails on a new test file; pnpm run typecheck runs the same config/tsconfig.node.json project in CI (.github/workflows/pr.yml:387), so this blocks the typecheck job.
Technical details
# TS2739 in ssh-upload-session-lifetime.test.ts
## Affected sites
- `src/main/ssh/ssh-upload-session-lifetime.test.ts:79` — `channel.bind({})` passes `{}` where `bind(resource: SshConnectionWorkChannel)` requires `on`/`once`/`removeListener`.
- `src/main/ssh/ssh-upload-session-lifetime.ts` — `beginChannelOpen().bind` parameter type comes from `SshConnectionWorkChannel` in `ssh-connection-work-ledger.ts`.
## Required outcome
- Pass a value satisfying `SshConnectionWorkChannel` (e.g. `new EventEmitter()` with `import { EventEmitter } from 'node:events'`) so `pnpm tc:node` passes.
## Open questions for the human
- None; behavior is unchanged (the resource is only stored, never invoked).⚠️ pnpm run check:code-quality:changed fails with 17 new type-assertion findings
CI runs this gate at .github/workflows/pr.yml:225; it reports 22 typescript(consistent-type-assertions) findings (17 unique lines) across six test files added or changed by this PR. Per AGENTS.md, unavoidable casts need a line-specific // oxlint-disable-next-line typescript/consistent-type-assertions -- SAFETY: … rationale, or the cast should be removed.
Technical details
# Consistent type assertions in new SSH test files
## Affected sites
- `src/main/ssh/ssh-connection-channel-error.test.ts:52`
- `src/main/ssh/ssh-connection-disconnect-drain.test.ts:47,193,205`
- `src/main/ssh/ssh-connection-reconnect-ladder.test.ts:271`
- `src/main/ssh/ssh-connection-transport-closure.test.ts:69,87,154`
- `src/main/ssh/ssh-connection-work-drain.test.ts:79,88,116,129,163,175`
- `src/main/ssh/ssh-forward-channel-lifetime.test.ts:10,21,70`
## Required outcome
- The changed-code quality gate passes.
## Suggested approach (optional)
- Add the SAFETY-commented `oxlint-disable-next-line` where the cast is genuinely unavoidable, or replace the cast with a typed stub (`new EventEmitter()`, a small `ClientChannel` fixture) where it is not.deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| forwardOut(client: SshClient, localSocket: object, ...args: Parameters<SshClient['forwardOut']>) { | ||
| assertSshForwardClient(this.client, client) | ||
| const report = this.reportUnhandledChannelError('forward') | ||
| forwardTrackedSshChannel(this.workLedger, client, localSocket, report, ...args) |
There was a problem hiding this comment.
client.forwardOut directly instead of using this tracked wrapper. If the SSH client closes while one of those forwards still has an open socket or a pending channel callback, the work ledger cannot see it. subscribeTransportClosure or disconnectAndDrain can then report completion before that forwarding work is finished.
Knowledge Base Used: Remote connectivity and execution
| const markOpenUnconfirmed = (error: Error): Error => { | ||
| work.markUnverifiable(error) | ||
| if (!trackRemoteCommandTermination) { | ||
| return error | ||
| } | ||
| unconfirmedOpenError = Object.assign(error, { sshChannelCloseConfirmed: false }) | ||
| return unconfirmedOpenError |
There was a problem hiding this comment.
Knowledge Base Used: Remote connectivity and execution
This comment has been minimized.
This comment has been minimized.
| openForwardSocket<T extends NodeJS.EventEmitter>(open: () => T): T { | ||
| return openTrackedSshSocket(this.workLedger, open, this.reportUnhandledChannelError('socket')) | ||
| } | ||
| forwardOut(client: SshClient, localSocket: object, ...args: Parameters<SshClient['forwardOut']>) { |
There was a problem hiding this comment.
client.forwardOut directly instead of this tracked method. If a forward is opening or active during disconnectAndDrain, its socket and channel are absent from the work ledger. The drain can therefore report completion without accounting for that forward.
Knowledge Base Used: Remote connectivity and execution
| } | ||
| } | ||
| const rejectAfterClose = (value: T): void => { | ||
| const abortError = markOpenUnconfirmed(createSshOperationAbortError()) |
There was a problem hiding this comment.
disconnectAndDrain, this call marks it unverifiable before waiting for close. The fenced ledger retains that failure permanently, so the drain rejects even when the channel closes within the grace period and its closure is confirmed.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 📝 WalkthroughWalkthroughThe changes add tracked lifetimes for SSH channel opens, forwarded channels, upload sessions, and file-transfer operations. They add work fencing and transport-close draining, plus validated execution-destination snapshots for successful SSH2 connections. System SSH command streams now end child stdin when input ends. Tests cover channel errors, reconnects, cancellation, transport closure, and work draining. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to A forwarded-socket close race can leave the new drain API waiting on a stale ledger entry. Fix the tracking before relying on drain completion. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change affects SSH shutdown guarantees and authenticated destination identity. Identity checks restrict the new destination information to the current successful connection. No newly introduced security vulnerability was verified, but complete shutdown coverage is not established for every forwarding path. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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: Advanced
Run ID: ded1e127-679b-4c73-a52c-0d4a67576cc5
📒 Files selected for processing (30)
src/main/ssh/ssh-channel-open.tssrc/main/ssh/ssh-connection-channel-error.test.tssrc/main/ssh/ssh-connection-channel-lifetime.test.tssrc/main/ssh/ssh-connection-channel-lifetime.tssrc/main/ssh/ssh-connection-channel-open.test.tssrc/main/ssh/ssh-connection-close-drain.tssrc/main/ssh/ssh-connection-destination-capture.test.tssrc/main/ssh/ssh-connection-destination.test.tssrc/main/ssh/ssh-connection-destination.tssrc/main/ssh/ssh-connection-direct-startup.test.tssrc/main/ssh/ssh-connection-disconnect-drain.test.tssrc/main/ssh/ssh-connection-file-transfer.test.tssrc/main/ssh/ssh-connection-file-transfers.tssrc/main/ssh/ssh-connection-host-key-store-wiring.test.tssrc/main/ssh/ssh-connection-reconnect-ladder.test.tssrc/main/ssh/ssh-connection-system-work-drain.test.tssrc/main/ssh/ssh-connection-test-fixtures.tssrc/main/ssh/ssh-connection-transport-closure.test.tssrc/main/ssh/ssh-connection-transport-closure.tssrc/main/ssh/ssh-connection-work-drain.test.tssrc/main/ssh/ssh-connection.tssrc/main/ssh/ssh-forward-channel-lifetime.test.tssrc/main/ssh/ssh-forward-channel-lifetime.tssrc/main/ssh/ssh-multi-factor-authentication.test.tssrc/main/ssh/ssh-relay-exec-command.tssrc/main/ssh/ssh-relay-exec-input.test.tssrc/main/ssh/ssh-system-transport.integration.test.tssrc/main/ssh/ssh-upload-session-lifetime.test.tssrc/main/ssh/ssh-upload-session-lifetime.tssrc/main/ssh/system-ssh-command.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| export function forwardTrackedSshChannel( | ||
| ledger: SshConnectionWorkLedger, | ||
| client: Client, | ||
| localSocket: object, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Type the forwarded localSocket as NodeJS.EventEmitter instead of object. In both signatures, object fails the anti-slop(no-object-parameters) static-analysis check. That makes CI fail. The type also hides the real contract: the lifetime tracker needs on, once, and removeListener. If a caller passes a value without them, the tracker marks the value unverifiable and never settles its ledger entry.
src/main/ssh/ssh-forward-channel-lifetime.ts#L55-L55: change theforwardTrackedSshChannelparameterlocalSocket: objecttolocalSocket: NodeJS.EventEmitter.src/main/ssh/ssh-connection.ts#L296-L296: change theSshConnection.forwardOutparameterlocalSocket: objecttolocalSocket: NodeJS.EventEmitter.
🧰 Tools
🪛 GitHub Check: static analysis
[failure] 55-55: anti-slop(no-object-parameters)
src/main/ssh/ssh-forward-channel-lifetime.ts:55:16: Parameter localSocket uses the broad object type. Accept a named owner type; parse external input at its boundary before calling this function.
📍 Affects 2 files
src/main/ssh/ssh-forward-channel-lifetime.ts#L55-L55(this comment)src/main/ssh/ssh-connection.ts#L296-L296
Source: Linters/SAST tools
…ssion test stub
The forwarded local socket now takes SshConnectionWorkChannel, the event
surface the ledger tracks, instead of a broad object. The upload-session
lifetime test binds an EventEmitter rather than an untyped {}.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes (delta since the prior Pullfrog review at 15329c7)
- Typecheck fix —
ssh-upload-session-lifetime.test.tsnow binds a realEventEmitter(channel.bind(new EventEmitter()), plus thenode:eventsimport) instead of{}. - Cast cleanup — every
typescript/consistent-type-assertionsfinding from the prior review now carries a line-specific// oxlint-disable-next-line … -- SAFETY:rationale. - Forward socket typing —
forwardOutandforwardTrackedSshChannelnow takelocalSocket: SshConnectionWorkChannelrather thanobject;forwardOutalso declares itsvoidreturn. Production-only type tightening with no behavioral change. - Test stub typing — the reconnect-ladder DNS failure is built via
Object.assign(new Error(…), { code: 'EAI_AGAIN' })instead of aNodeJS.ErrnoExceptioncast.
I re-ran both gates the prior review left red: pnpm tc:node exits 0 and pnpm run check:code-quality:changed reports 0 new findings across all 30 changed files. The changes are mechanical and introduce no new logic. The two earlier deferrable observations (the post-disconnect AbortError taxonomy and the sticky unprovenStartupTransport flag) remain open design questions with no production consumer yet; they are intentionally not re-raised here.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Preserve closure state across repeated socket tracking. · ssh-forward-channel-lifetime.ts:55-78
src/main/ssh/ssh-forward-channel-lifetime.ts:55-78
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPreserve closure state across repeated socket tracking.
openForwardSockettracks the socket once, thenforwardOutcreates a second ledger entry for the same socket. If the socket emitsclosebetween these calls, the second tracker subscribes too late. Thecloseevent is not replayed, so the new entry can remain pending and makedisconnectAndDrainwait for a close that cannot occur. Check the socket state and retain closure state across tracking calls.Suggested fix
type TrackableChannel = SshConnectionWorkChannel & { closed?: unknown + destroyed?: unknown } +const closedChannels = new WeakSet<object>() + ... } const channel = value work.bind(channel) - if (channel.closed === true) { + if ( + channel.closed === true || + channel.destroyed === true || + closedChannels.has(channel) + ) { work.close() return } ... channel.once('close', () => { + closedChannels.add(channel) channel.removeListener('error', onError) work.close() })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 67610fd9-7139-4a08-990e-2d5a3e5af3c5
📒 Files selected for processing (9)
src/main/ssh/ssh-connection-channel-error.test.tssrc/main/ssh/ssh-connection-disconnect-drain.test.tssrc/main/ssh/ssh-connection-reconnect-ladder.test.tssrc/main/ssh/ssh-connection-transport-closure.test.tssrc/main/ssh/ssh-connection-work-drain.test.tssrc/main/ssh/ssh-connection.tssrc/main/ssh/ssh-forward-channel-lifetime.test.tssrc/main/ssh/ssh-forward-channel-lifetime.tssrc/main/ssh/ssh-upload-session-lifetime.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/ssh/ssh-connection-channel-error.test.ts
- src/main/ssh/ssh-forward-channel-lifetime.test.ts
- src/main/ssh/ssh-connection-transport-closure.test.ts
- src/main/ssh/ssh-connection-disconnect-drain.test.ts
- src/main/ssh/ssh-connection-work-drain.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
GitHub reported a conflict after main moved SshConnection's file transfers into ssh-connection-file-transfers.ts (stablyai#24401). Conflict, in ssh-connection.ts: took main's delegation through fileTransferHost() and the work ledger, keeping this branch's download observer type on downloadFile. This branch's transfer behavior moved into main's new module: - downloadSshFile: SFTP downloads go through downloadFileViaSftp, and system-SSH downloads use a signal linked to the caller's cancel, both reporting bytes. - createSshFileUploadSession: uploads link the caller's cancel signal and pass onRemoteCreated / onBytesTransferred; sessions expose removeCreatedEntry (SFTP, and system SSH except on Windows hosts). sftp-upload.ts auto-merged past its 300-line limit, since main split uploadDirectory while this branch added upload observers. The two observer listeners moved unchanged into sftp-upload-observers.ts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Main's openTrackedSshUploadSession (stablyai#24401) returned only uploadFile and close, so the optional removeCreatedEntry that SFTP and POSIX system-SSH sessions provide was dropped. With it gone, a cancelled SSH import's rollback kept every folder it created and deleted files by path after the identity check, instead of the remote's guarded non-recursive removal. Type checking missed it because the method is optional. The wrapper now forwards removeCreatedEntry when the session has one, inside the session lifetime so the connection's reset fence waits for it. When the session has none (Windows hosts), it adds no stub, which keeps the rollback's Windows fallback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* Revert "feat(orcad): source-side dormant export of a relay-hosted SSH target (#16741 T6-8) (#24519)" This reverts commit 783101b. * Revert "feat(ssh): update, roll back, recover and stop a managed orcad server (#16741 T6-5 follow-up) (#24463)" This reverts commit 38c2d1d. * Revert "feat(ssh): deploy and pair an empty managed orcad server over SSH (#16741 T6-5) (#24453)" This reverts commit 8b76683. * Revert "fix(ssh): orcad GC honors the activation journal; readiness requires proven daemon coverage (#16741 T6 follow-up) (#24451)" This reverts commit d3f8c50. * Revert "feat(ssh): remote orcad stop by request file and journaled decommission (#16741 T6-4) (#24449)" This reverts commit 43d9b43. * Revert "feat(orcad): supervisable server: stop requests, managed stop receipts and a lifetime that keeps its lock on failed teardown (#16741 T6-3) (#24433)" This reverts commit b093d3a. * Revert "feat(ssh): crash-safe orcad activation, rollback and recovery (#16741 T6-2) (#24423)" This reverts commit 1a9ac0e. * Revert "feat(runtime): SSH access links for paired servers in a downgrade-safe sidecar (#16741 T5-1+T5-2) (#24420)" This reverts commit 99db2bf. * Revert "feat(relay): capability-gated owner reset with a durable preparation journal (#16741 T3 R1) (#24418)" This reverts commit 34a582b. * Revert "feat(ssh): track connection-manager drains, test probes and provider continuations (#16741 T2 P3+P8a) (#24407)" This reverts commit d53063d. * Revert "feat(daemon): idle retirement, session census and recovery-only provider (#16741 T2 P4b) (#24409)" This reverts commit ff212db. * Revert "feat(ssh): add pty.resumeClient and split SSH PTY process listing (#16741 T2 P5+P6) (#24414)" This reverts commit 92cb717. * Revert "feat(relay): await owned watcher and agent children on shutdown (#16741 T2 P1) (#24400)" This reverts commit 6b36e4f. * Revert "feat(session): retry failed renderer session writes and verify local folder PTYs (#16741 T2 P9) (#24406)" This reverts commit d23ecef. * Revert "feat(ssh): remote orcad primitives on the pinned Node runtime (#16741 T6-1) (#24419)" This reverts commit dd87ae5. * Revert "fix(runtime): fence runtime-environment subscriptions and status probes by identity (#16741 T5-3) (#24421)" This reverts commit ece9e4d. * Revert "feat(orcad): migration manifest and dormant-state contracts (#16741 T6-7) (#24422)" This reverts commit 3fbdaba. * Revert "feat(ssh): wire SshConnection through the work and transport close ledgers (#16741 T2 P2) (#24401)" This reverts commit 4e8edc8. * Revert "feat(profiles): carry markdown frontmatter visibility in project transfers (#16741 T2 P7) (#24405)" This reverts commit 60c9326. * Revert "fix(runtime): project the PTY incarnation onto mobile session tabs (#24413)" This reverts commit 99e0303. * Revert "feat(daemon): tag daemon stream data with the PTY incarnation id (#16741 T2 P4a) (#24402)" This reverts commit 817af76. * Revert "feat(ssh): port the SSH connection work ledger and transport close ledger (#16741 T2) (#24210)" This reverts commit c991893. * Revert "feat(relay): fence and drain file and git response streams on shutdown (#24185)" This reverts commit dc08ffe. * Revert "refactor(runtime-rpc): extract the Node WebSocket lifecycle; opt-in pinned port (#24186)" This reverts commit a789233. * Revert "feat(relay): route relay handlers through work admission; producer publication drain (#24181)" This reverts commit 0b812bd. * Revert "feat(relay): land the #16741 T1 seam (work drain, publication drain, release gate) (#24156)" This reverts commit 3aa2d3a. --------- Co-authored-by: m4air <m4air@Mac.localdomain>
Main reverted the 26 Phase 3 PRs (stablyai#24559), including stablyai#24401, which had moved SshConnection's file transfers into ssh-connection-file-transfers.ts behind a work ledger. This branch had moved its transfer behavior into that module, and added the forwarding of removeCreatedEntry through its tracked upload session. Resolution: - ssh-connection-file-transfers.ts, ssh-upload-session-lifetime.ts and its test: take main's deletion. - ssh-connection.ts: git auto-merged downloadFile and openFileUploadSession to main's plain bodies with no conflict marker, silently dropping this branch's download progress and cancel, upload cancel signal and callbacks, and removeCreatedEntry. Both methods are restored to this branch's in-class version from before stablyai#24401 (identical to 03a44db), with their imports; main's other changes in the file are kept. The 26 tests that fail in native-chat, cross-version e2e and the agent session import also fail on main itself at 66799f7. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 participant
This is the P2 slice of the T2 port of #16741 (SSH core), building on #24210, which added the work ledger and transport close ledger without using them. This PR has
SshConnectionactually use them.ELI5
An SSH connection now keeps a list of everything it has opened (commands, file transfers, terminals, forwarded sockets) and crosses each item off only when it has really closed. A later caller can then wait for "everything on this connection is closed" instead of guessing. Nothing a user does today waits on that yet, so behavior does not change.
What Changed
Before:
SshConnectionopened channels and transports, but nothing could tell when they had all physically closed. Closing a connection was fire-and-forget.After: behavior is the same for users. Underneath:
close, including channels that open after their caller gave up. That covers exec,shell(the new plain-SSH terminals), SFTP, directory and file uploads, downloads, file writes, upload sessions and system-SSH commands.close, including clients whose login failed.subscribeTransportClosure: a one-shot notice once the connection is disconnected, every tracked transport has closed and all work has drained. A system-SSH connection never gets this notice, because its child process's closure cannot be proven from here.disconnectAndDrain(signal): for single-lifetime connections. It stops new work, disconnects, and waits until everything above has closed. If closure cannot be proven, for example on system SSH or with a connect still in progress, it cleans up and then refuses rather than claiming success.getExecutionDestination: the host, port, user, accepted host-key fingerprint and proxy route that the current ssh2 login actually reached.getTransportGeneration, plusprepareForwardRoute/openForwardSocket/forwardOut/forwardStreamLocalfor forwarding callers in later slices.automaticReconnectconstructor option, on by default.sshchild process, as an ssh2 channel already does.errorlistener. When that is the channel's onlyerrorlistener, the error is logged as[ssh] Unhandled <kind> channel error for <target>: <message>instead of disappearing. Before this, such an error crashed the process.Channel or client closure is treated as local evidence only. It is never reported as proof that a remote process exited, so the
live/unverifiable/exitedrule is unchanged.Why
Later T2 and T3 work (connection-manager drain, reset, ownership moves) needs to know when a connection's work and transports are really gone. Tracking at the
SshConnectionboundary catches every caller without changing any of them, including the plain-SSH mode added in #24147, whoseshell()and SFTP calls go through the same ledger.ssh-connection.tsends at 1,872 lines, below main's 1,917, so it does not grow under its max-lines exemption. The public API is unchanged.ssh-connection-transport-closure.tsandssh-connection-close-drain.ts: the new closure gating and close drain.ssh-channel-open.ts: the existing channel-open waiter and session-limit retry.ssh-connection-file-transfers.ts: the existing transfer bodies, reading the connection through getters so each read sees the live transport.ssh-forward-channel-lifetime.ts: the forward-route checks.fenceWorkForReset(T3);disconnectAndDrainfences through the ledger directly instead;isEphemeralRuntimeSshOwner(T6);assertProfileLifetimeAdmission(P8b);execCommand, because main already streams stdin.Linked Issue
Part of the Node runtime plan, Phase 3, T2 slice P2. It ports from #16741 (head a68b6f3, merge-base 277c289) and builds on #24210.
Visual Proof
N/A. This is internal SSH connection bookkeeping with no UI or interaction change.
Testing
New tests: an unhandled channel error is reported, an owner-handled one is not, and
SshConnectionlogs an unhandled exec channel error with the target and channel kind.Ported tests: destination parsing and capture, disconnect drain, transport closure, work drain (system-SSH cases split into their own file), forward-channel and upload-session lifetimes, system-SSH command input. Also the feat(orcad): consolidate Bun runtime and SSH/Remote migration #16741 updates to the channel-open, direct-startup, file-transfer, host-key-store-wiring and reconnect-ladder tests, and to the multi-factor authentication and system-transport integration tests.
Regression: every
ssh-connection*andssh-relay-session*test, plus the rest ofsrc/main/ssh, the SSH providers and the SSH IPC tests (300+ files) pass. That includes the plain-SSH session, shell and SFTP provider tests.The work-drain tests use the ledger's own fence until T3 adds the public one.
macOS only, locally; CI covers Linux and Windows.
I manually tested these changes locally
Automated tests added/updated, or explained why not below
AI Disclosure
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)