Repository navigation
feat(ssh): track connection-manager drains, test probes and provider continuations (#16741 T2 P3+P8a) - #24407
Conversation
5b0e356 to
8deda77
Compare
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor suggestion inline. The port is faithful to #16741's head, the tests exercise the new behavior, and I re-ran the seven new/changed test files (37 passing).
Reviewed changes
- Connection-manager bookkeeping — per-target
pendingTargetOperations,unconfirmedTargetTeardowns, and a per-connectionunclosedTransportsByTargetcount driven bySshConnection.subscribeTransportClosure; failed startup now disconnects the half-open connection rather than forgetting it;disconnectAll/disconnectConnectiongain exclusion and drain support. - Per-target test probes —
runSshTestConnectionProbekeyed by target, sotestingTargetsandcredentialRequestedForTargetclear only when a host's last probe settles;awaitSshTestConnectionProbesrechecks after each joined snapshot. - Shutdown drain —
beginSshShutdown(mayDetach)/disconnectAll(mayDetach)with a default that still detaches every host. - Provider continuations — SSH file writes/deletes, drag-in imports, detected-worktree listings, and worktree/folder removals tracked until local settlement via
runSshProviderContinuation. - Misc — owner-based suppression in
broadcastSshState;remotePathExistsextracted to its own module without the oldNodeJS.ErrnoExceptioncast;createMockStoreextracted to a shared fixture.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
| @@ -37,6 +37,16 @@ export function unregisterSshFilesystemProvider(connectionId: string): void { | |||
| sshProviders.delete(connectionId) | |||
| } | |||
|
|
|||
| export function unregisterSshFilesystemProviderIfCurrent( | |||
There was a problem hiding this comment.
unregisterSshFilesystemProviderIfCurrent (and its unregisterSshGitProviderIfCurrent counterpart, plus hasRegisteredDirectSshAuthority with its setDirectSshAuthorityResolver registration) have no callers anywhere in the tree, and none of them has a test. If they're staged for the T3/T8 slices that's reasonable, but note the PR's "left for later" list doesn't name them, so they currently ship as unverified API. Consider either wiring them into the relay teardown path in this slice or dropping them until their consumer lands.
There was a problem hiding this comment.
✅ No new issues found in the incremental changes.
Reviewed changes (since my previous review at 5b0e356)
- Type-assertion lint fixes — line-specific
oxlint-disable ... -- SAFETY:comments added for the remainingascasts in the new test files, matching the repo's checked-types rule. - Test typing tightened —
worktree-removal-continuations.test.tsreturnsPromise<unknown>from a typed arrow instead of casting the handler result;ssh-connection-manager-closure.test.tsdrops the unusedprofile-lifetime-admissionmock, replaces its hoistedas/optional-property state with a typedClosureMockState, and builds a completeSshTargetliteral instead ofas SshTarget. - New test probe —
managerUnclosedTransportTargetsreplaces the inline private-field cast in the closure test.
These are all test-only quality improvements; the production surface is unchanged from the previously reviewed version.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return await operation() | ||
| } catch (error) { | ||
| if (retainFailure) { | ||
| this.unconfirmedTargetTeardowns.add(targetId) |
There was a problem hiding this comment.
unconfirmedTargetTeardowns, but a later successful disconnect does not remove it. Even after the connection physically closes, the manager’s closure probe still reports the target as active. That makes the new bookkeeping unable to recognize a confirmed retry when reset or handoff uses it.
Knowledge Base Used: Remote connectivity and execution
8deda77 to
9bccb33
Compare
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 9bccb33106a517737991ed48478c43bca6329499 and b6f9c64. 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe change adds per-target tracking for SSH provider operations and test-connection probes. Filesystem and worktree handlers use provider continuations. SSH connection management tracks pending operations, failed teardowns, and transport closure, and shutdown accepts a target filter. The change also adds direct SSH authority resolution and suppresses state broadcasts for runtime-owned targets. Priority: ⬇️ Low Merge Risk: 🔵 Low · up to The lifecycle bookkeeping retains stale activity after a teardown failure. Current impact is bounded to activity and closure checks, so merging requires awareness of this unresolved limitation. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected changes improve SSH cleanup and preserve existing file-operation checks. No introduced security vulnerability was established, but access isolation for runtime-managed connections and recovery after failed teardown remain incompletely established. 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: 8f0ec28d-009b-451d-ad15-35153d93ef18
📥 Commits
Reviewing files that changed from the base of the PR and between 4e8edc8 and 9bccb33106a517737991ed48478c43bca6329499.
📒 Files selected for processing (30)
src/main/ipc/filesystem-import-ssh-remote-existence.tssrc/main/ipc/filesystem-import-ssh.tssrc/main/ipc/filesystem-provider-continuations.test.tssrc/main/ipc/filesystem/filesystem-write-handlers.tssrc/main/ipc/ssh-active-relay-sessions.tssrc/main/ipc/ssh-connect-attempt-registry.tssrc/main/ipc/ssh-connection-handlers.tssrc/main/ipc/ssh-ipc-module-mocks.tssrc/main/ipc/ssh-renderer-broadcast.tssrc/main/ipc/ssh-shutdown-drain.tssrc/main/ipc/ssh-state-broadcast-fanout.test.tssrc/main/ipc/ssh-target-lifecycle-queue.tssrc/main/ipc/ssh-test-connection-probe-registry.test.tssrc/main/ipc/ssh-test-probe-presence.test.tssrc/main/ipc/worktrees-ssh-provider-authority.test.tssrc/main/ipc/worktrees/listing/register-detected-worktree-handlers.tssrc/main/ipc/worktrees/removal/register-worktree-removal-handlers.tssrc/main/ipc/worktrees/removal/worktree-removal-continuations.test.tssrc/main/providers/ssh-filesystem-dispatch.tssrc/main/providers/ssh-git-dispatch.tssrc/main/ssh/ssh-connection-disconnect-drain.test.tssrc/main/ssh/ssh-connection-manager-closure.test.tssrc/main/ssh/ssh-connection-manager-registry.test.tssrc/main/ssh/ssh-connection-manager-test-probes.tssrc/main/ssh/ssh-connection-manager.test.tssrc/main/ssh/ssh-connection-manager.tssrc/main/ssh/ssh-connection-store-test-fixture.tssrc/main/ssh/ssh-connection-store.test.tssrc/main/ssh/ssh-provider-continuations.tssrc/main/ssh/ssh-target-registry.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| private async trackTargetOperation<T>( | ||
| targetId: string, | ||
| operation: () => Promise<T>, | ||
| retainFailure = false | ||
| ): Promise<T> { | ||
| this.pendingTargetOperations.set( | ||
| targetId, | ||
| (this.pendingTargetOperations.get(targetId) ?? 0) + 1 | ||
| ) | ||
| try { | ||
| return await operation() | ||
| } catch (error) { | ||
| if (retainFailure) { | ||
| this.unconfirmedTargetTeardowns.add(targetId) | ||
| } | ||
| throw error | ||
| } finally { | ||
| const remaining = (this.pendingTargetOperations.get(targetId) ?? 1) - 1 | ||
| if (remaining === 0) { | ||
| this.pendingTargetOperations.delete(targetId) | ||
| } else { | ||
| this.pendingTargetOperations.set(targetId, remaining) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,260p' src/main/ssh/ssh-connection-manager.ts
rg -n 'unconfirmedTargetTeardowns|managerTargetActive|assertManagerTargetTransportsClosed|hasTargetActivity' src/mainRepository: stablyai/orca
Length of output: 11642
🏁 Script executed:
sed -n '1,120p' src/main/ssh/ssh-connection-manager-test-probes.ts
sed -n '1,125p' src/main/ssh/ssh-connection-manager-closure.test.ts
sed -n '225,270p' src/main/ssh/ssh-connection-disconnect-drain.test.ts
rg -n 'subscribeTransportClosure|disconnectAndDrain|async disconnect|disconnect\\(|unconfirmedTargetTeardowns|unclosedTransportsByTarget' src/main/ssh/ssh-connection.ts src/main/ssh/ssh-connection-manager.ts src/main/ssh --glob '*.ts'Repository: stablyai/orca
Length of output: 7896
🏁 Script executed:
rg -n 'subscribeTransportClosure|disconnectAndDrain|async disconnect|disconnect\\(|emit\\(.close.|transport|close' src/main/ssh/ssh-connection.ts src/main/ssh --glob '*.ts'
sed -n '1,260p' src/main/ssh/ssh-connection.tsRepository: stablyai/orca
Length of output: 10567
🏁 Script executed:
grep -nE 'subscribeTransportClosure|disconnectAndDrain|^[[:space:]]*(async )?disconnect\\b|transportClosure|transportCloseLedger' src/main/ssh/ssh-connection.ts
grep -nE 'disconnectAndAwaitSshTransportClose|subscribe|notify|close' src/main/ssh/ssh-connection-close-drain.ts src/main/ssh/ssh-connection-transport-closure.ts src/main/ssh/ssh-transport-close-ledger.tsRepository: stablyai/orca
Length of output: 3184
🏁 Script executed:
sed -n '1735,1875p' src/main/ssh/ssh-connection.ts
sed -n '1,110p' src/main/ssh/ssh-connection-close-drain.ts
sed -n '1,80p' src/main/ssh/ssh-connection-transport-closure.tsRepository: stablyai/orca
Length of output: 7340
Remove failed-teardown markers only after all transports close.
trackTargetOperation adds targetId after a retained teardown failure, but no current code removes it. The test probes then report stale activity after the final transport closes.
The closure callback alone is not sufficient. disconnectAndDrain can call disconnect(), allow the closure callback to run, and then reject with ssh_connection_close_transport_unproven. The catch block would otherwise add the marker again.
Do not clear the marker on a successful teardown while another transport remains unclosed. managerTargetActive does not check unclosedTransportsByTarget, so that edit would hide outstanding closure debt.
Suggested fix
if (remaining === 0) {
this.unclosedTransportsByTarget.delete(targetId)
+ this.unconfirmedTargetTeardowns.delete(targetId)
} else {
this.unclosedTransportsByTarget.set(targetId, remaining)
}
@@
} catch (error) {
if (retainFailure) {
- this.unconfirmedTargetTeardowns.add(targetId)
+ if (this.unclosedTransportsByTarget.has(targetId)) {
+ this.unconfirmedTargetTeardowns.add(targetId)
+ }
}
throw error
…continuations (#16741 T2 P3+P8a) The SSH connection manager now registers each connection it allocates against the connection's transport-closure notice, and tracks every connect, disconnect, reconnect and teardown per target, so later slices can tell when a target's local work and transports are really gone. Ordinary connect, disconnect and quit behavior is unchanged apart from cleanup. Manager (ssh-connection-manager.ts): - registerConnection counts every connection a target allocated, retired pool entries included, until its transport-closure notice arrives. - connect/disconnect/reconnect/disconnectConnection/disconnectAll run through trackTargetOperation; a failed teardown is remembered as unconfirmed. - A failed startup is now disconnected, not just dropped from the pool. - disconnectAndDrain(targetId, signal) and disconnectConnection(..., drain), bounded by a 10s timeout. - disconnectAll(shouldDisconnect) only tears down targets the filter allows. IPC: - ssh-connect-attempt-registry: runSshTestConnectionProbe publishes a probe per target before it starts and clears testingTargets / credential flags only when the target's last probe settles. ssh:testConnection uses it, so quit still joins an in-flight probe within the shutdown budget. - ssh-target-lifecycle-queue: runTargetLifecycle returns the operation's value. - ssh-shutdown-drain: a mayDetach predicate (default: every target) is plumbed through detach, invalidation and disconnectAll. - ssh-renderer-broadcast: targets owned by a runtime are hidden by owner too, not only by id prefix. - ssh-target-registry: direct-authority resolver, installed by ssh-active-relay-sessions. - Provider dispatch: unregister*IfCurrent for git and filesystem providers. P8a, SSH provider continuations: ssh-provider-continuations tracks local settlement of SSH filesystem writes/deletes, imports, detected-worktree listings and worktree/folder removals per target. It records only local settlement, never remote execution or exit. Porting note (source: #16741 head a68b6f3, merge-base 277c289): - Left for later slices: connectExclusive and assertTargetTransportsClosed (T8), hasTargetActivity (T3), assertProfileLifetimeAdmission (P8b, stubbed out of the manager and continuations), the reset admission checks in ssh-connection-handlers and the reset predicate in the shutdown drain (T3), the network-tunnel resolver (T4), and the managed-orcad provisioning test hunks in ssh-target-registry.test.ts (T6). - Tests read the manager's bookkeeping through ssh-connection-manager-test-probes until T3/T8 expose readers. Two of the four deferred disconnect-drain cases are restored; the two that need connectExclusive, two closure cases that need it and three profile-admission cases stay deferred. - The removal and detected-worktree call sites were rebased onto main's background removal and execution-host routing. filesystem-import-ssh moved its remote-existence helpers out to stay under 300 lines.
9bccb33 to
b6f9c64
Compare
* 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>
This is the P3 slice of the T2 port of #16741 (SSH core), with P8a (SSH provider continuations) folded in. It is stacked on #24401 (P2) and will be retargeted to main once that merges.
ELI5
Orca now keeps per-host notes of what it is still doing over SSH: which connections it opened and whether they have really closed, which connects and disconnects are still running, which "Test connection" checks are in flight, and which file or worktree operations are still finishing. A later change can then wait until a host is truly quiet before resetting or handing it over. Nothing the user does waits on these notes yet, so behavior stays the same.
What Changed
Before: the SSH connection manager forgot a connection as soon as it left the pool, even if its sockets were still closing. A failed connect was dropped without being disconnected. "Test connection" probes were tracked as a flat set, so a second probe on the same host could clear the first one's state.
After: behavior is unchanged for users, apart from cleanup. Underneath:
disconnectAndDrain, and a drain option ondisconnectConnection, for owned connections. The drain is capped at 10 seconds.disconnectAllcan skip hosts.Everything here is local bookkeeping. A host that cannot be reached is never treated as having exited.
Why
Reset (T3) and ownership handoff (T8) need a reliable answer to "is this host's work really finished here?" Recording that at the connection manager and at each SSH provider entry point catches every caller in one place.
Left for later slices:
connectExclusiveandassertTargetTransportsClosed(T8).hasTargetActivity, the reset admission checks, and the shutdown reset predicate (T3).Until T3 and T8 add the public readers, the tests read the manager's internal state through a small test-only probe.
Linked Issue
Part of the Node runtime plan, Phase 3, T2 slices P3 and P8a. Ported from #16741 (head a68b6f3, merge-base 277c289). Stacked on #24401.
Visual Proof
N/A. This is internal SSH bookkeeping with no UI or interaction change.
Testing
Ported tests (123 passing):
Regression: all of
src/main/ssh, the SSH providers, SSH, filesystem and worktree IPC, and the quit path: 430 files, 5,415 passed.Not ported yet: cases that need
connectExclusive(4) or profile admission (3).Tested on macOS 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)