Repository navigation
Fix SSH reconnect churn after persistent daemon respawn - #9019
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe Swift client now admits keepalive RPCs only when no application calls are pending and centralizes payload encoding. The Go persistent-daemon authentication path returns explicit errors, logs rejected connections, and propagates authentication response write failures. ChangesIdle RPC and transport keepalive
Persistent-daemon authentication handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)Idle keepalive admissionsequenceDiagram
participant ApplicationRPC
participant RemoteDaemonRPCClient
participant PendingCallRegistry
participant RemoteDaemon
ApplicationRPC->>RemoteDaemonRPCClient: start application RPC
RemoteDaemonRPCClient->>PendingCallRegistry: register pending call
RemoteDaemonRPCClient->>RemoteDaemon: send application request
RemoteDaemonRPCClient->>PendingCallRegistry: registerIfIdle()
PendingCallRegistry-->>RemoteDaemonRPCClient: nil while application call is pending
RemoteDaemon-->>RemoteDaemonRPCClient: application response
RemoteDaemonRPCClient->>PendingCallRegistry: resolve pending call
RemoteDaemonRPCClient->>PendingCallRegistry: registerIfIdle()
PendingCallRegistry-->>RemoteDaemonRPCClient: admitted keepalive call
RemoteDaemonRPCClient->>RemoteDaemon: send hello probe
Persistent authenticationsequenceDiagram
participant persistentDaemonAcceptLoop
participant handlePersistentDaemonConn
participant authenticatePersistentDaemonConn
participant stderr
persistentDaemonAcceptLoop->>handlePersistentDaemonConn: pass accepted connection and stderr
handlePersistentDaemonConn->>authenticatePersistentDaemonConn: authenticate connection
authenticatePersistentDaemonConn-->>handlePersistentDaemonConn: success or error
handlePersistentDaemonConn->>stderr: log rejection error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
…ejoin-respawned-daemon
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
daemon/remote/cmd/cmuxd-remote/main.go (1)
1165-1174: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDifferentiate missing and invalid authentication methods.
A nonempty unsupported method also reaches this branch, but the emitted diagnostic always says it is missing. Log the actual rejection reason without echoing the supplied method.
Proposed fix
-if req.Method != persistentDaemonAuthMethod { +if req.Method == "" { + // write unauthorized response + return errors.New("authentication method is missing") +} +if req.Method != persistentDaemonAuthMethod { // write unauthorized response - return errors.New("authentication method is missing") + return errors.New("authentication method is invalid") }Based on the PR objective to log authentication rejection reasons.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@daemon/remote/cmd/cmuxd-remote/main.go` around lines 1165 - 1174, Update the authentication check in the request handling flow around persistentDaemonAuthMethod to distinguish an empty method from a nonempty unsupported method. Preserve the unauthorized response, but return a diagnostic stating whether authentication is missing or invalid, without including the supplied method value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@daemon/remote/cmd/cmuxd-remote/main.go`:
- Around line 1165-1174: Update the authentication check in the request handling
flow around persistentDaemonAuthMethod to distinguish an empty method from a
nonempty unsupported method. Preserve the unauthorized response, but return a
diagnostic stating whether authentication is missing or invalid, without
including the supplied method value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d2893842-27f8-4438-9a7a-936b719390b1
📒 Files selected for processing (7)
Packages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient+RPC.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient+TransportKeepalive.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Registry/RemoteDaemonPendingCallRegistry.swiftPackages/macOS/CmuxRemoteDaemon/Tests/CmuxRemoteDaemonTests/RemoteDaemonPendingCallRegistryTests.swiftPackages/macOS/CmuxRemoteDaemon/Tests/CmuxRemoteDaemonTests/RemoteDaemonRPCClientKeepaliveTests.swiftdaemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.go
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Contributor
Author
|
Addressed the outside-diff auth-method diagnostic finding in b9cbd0d. Empty methods now log |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
daemon/remote/cmd/cmuxd-remote/main.go (1)
1170-1178: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPropagate failed rejection-response writes.
This branch still discards
writer.writeResponseerrors, so a failed auth reply is logged as a method rejection rather than a response-write failure. Apply the same error propagation used by the success response to every unauthorized/invalid-request response path.Proposed fix
- _ = writer.writeResponse(rpcResponse{ + if err := writer.writeResponse(rpcResponse{ ID: req.ID, OK: false, Error: &rpcError{ Code: "unauthorized", Message: "persistent daemon authentication required", }, - }) + }); err != nil { + return fmt.Errorf("authentication response write failed: %w", err) + } return errors.New(reason)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@daemon/remote/cmd/cmuxd-remote/main.go` around lines 1170 - 1178, Update the unauthorized and invalid-request response paths in the request handler to return or propagate the error from writer.writeResponse, matching the existing success-response handling instead of discarding it. Preserve the rejection response payload while ensuring response-write failures are surfaced as the returned error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@daemon/remote/cmd/cmuxd-remote/main.go`:
- Around line 1170-1178: Update the unauthorized and invalid-request response
paths in the request handler to return or propagate the error from
writer.writeResponse, matching the existing success-response handling instead of
discarding it. Preserve the rejection response payload while ensuring
response-write failures are surfaced as the returned error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 72025f39-25f2-44e3-a129-6eca4d354b1f
📒 Files selected for processing (3)
Packages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient+RPC.swiftdaemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.go
Contributor
Author
|
Regarding the automated Cmux Swift Blocking Runtime warning: we are consciously retaining these three queue boundaries. |
Contributor
Author
|
Addressed CodeRabbit’s refreshed rejection-write finding in 3e01767. All authentication rejection/invalid-request branches now surface a generic wrapped response-write error when the rejection frame cannot be sent, while preserving the existing wire payloads and secret-safe diagnostics. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@daemon/remote/cmd/cmuxd-remote/main_test.go`:
- Around line 31-37: Rename the test-local errorWriter type in main_test.go to a
distinct fixture name such as authErrorWriter, and update all references to it
in the affected tests; do not redeclare the existing package-level errorWriter
from main.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b75cbc84-7094-4626-8b8e-85850f3a9428
📒 Files selected for processing (2)
daemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.go
added 2 commits
July 27, 2026 17:57
…-9015-ssh-slot-rejoin-respawned-daemon
This was referenced Aug 1, 2026
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
1 participant
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.
Summary
Root cause
The stdio keepalive started after five seconds without an inbound frame and killed the transport after ten more seconds. Valid RPCs can take longer, and the daemon can queue the heartbeat behind cold-start work after a respawn. The heartbeat watchdog then killed a healthy SSH proxy while its application RPC was still progressing, producing the reported 20–40 second reconnect cycle.
Heartbeat admission is now atomic with write ordering. An earlier application call defers the probe; a probe that wins is written before any later application call. The existing idle-wedge watchdog behavior remains intact.
Tests
The regression is deterministic transport-level coverage of the liveness race; it does not stand up a real SSH host or kill a real remote daemon.
Closes #9015
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Fixes the 20–40s SSH reconnect loop after a persistent daemon respawn (issue #9015) by sending keepalives only when the transport is idle and arming the watchdog only after admission. Also improves persistent-daemon auth diagnostics and surfaces response-write failures, logging clear rejection reasons without exposing user input.
CmuxRemoteDaemon: AddedregisterIfIdle/callIfIdle; probe admission and write are atomic onwriteQueue; watchdog arms post-admission; active RPCs keep their own timeout.cmuxd-remote: Auth now returns explicit errors, logs rejection reasons to stderr, and reports wrapped errors when the auth response write fails; supplied tokens/methods are never logged.Written for commit c2f7bd6. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests