Repository navigation
Stop SSH auto-reconnect when host is unreachable; add manual Reconnect control (#5734) - #5767
Conversation
The SSH remote auto-reconnect loop retries forever while the host is unreachable. Introduce the WorkspaceRemoteReconnectPolicy seam encoding today's never-suspend behavior, plus tests asserting the desired policy: suspend the loop after consecutive failed reachability probes so the user controls when reconnection happens. The suspend assertions fail (red) until the policy change lands in the follow-up commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an SSH host reachability probe and a reconnect policy that suspends automatic reconnect after three consecutive unreachable probes; wires suspend state into workspace daemon, UI snapshot and sidebar affordance, CLI verbs, localization, tests, and Xcode project build entries. ChangesSSH Remote Reconnect Feature
Sequence Diagram(s)sequenceDiagram
participant Scheduler as ReconnectScheduler
participant Probe as WorkspaceRemoteHostReachabilityProbe
participant Policy as WorkspaceRemoteReconnectPolicy
participant Workspace as WorkspaceDaemon
Scheduler->>Probe: schedule probe
Probe->>Policy: WorkspaceRemoteHostProbeOutcome
Policy->>Workspace: Decision (scheduleRetry / suspend)
alt suspend
Workspace->>Scheduler: cancel pending retries
Workspace->>UI: publish .suspended state
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related issues
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning, 2 inconclusive)
✅ Passed checks (14 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 |
Greptile SummaryThis PR stops the indefinite SSH auto-reconnect loop by adding a reachability probe (
Confidence Score: 5/5The change is safe to merge: core policy logic, state-machine resets, and probe lifecycle are all correctly implemented, and the previously-flagged issues are resolved at HEAD. The suspension threshold, generation-latch, retain-cycle fix, and reconnectSuspended reset on .ready are all present and correct. No new blocking logic issues or state-machine gaps were found beyond the already-addressed threads. Sources/ContentView.swift — the safeHelp call passes the optional remoteWorkspaceSidebarText directly to String(format:locale:) rather than using a nil-coalesced value; safe today but fragile if conditions diverge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[connection fails] --> B[scheduleReconnectLocked]
B --> C{isStopping or reconnectSuspended?}
C -- yes --> D[return early, no retry scheduled]
C -- no --> E[arm reconnectWorkItem with backoff delay]
E --> F[evaluateReconnectPolicyLocked]
F --> G[bump reachabilityProbeGeneration, launch TCP probe on global utility queue]
G -->|probe result| H{generation still current?}
H -- no --> I[discard stale outcome]
H -- yes --> J{reconnectWorkItem still pending?}
J -- no --> I
J -- yes --> K[WorkspaceRemoteReconnectPolicy.evaluate]
K --> L{decision?}
L -- scheduleRetry --> M[streak++ keep work item armed]
L -- suspend streak >= 3 --> N[suspendAutoReconnectLocked]
N --> O[cancel reconnectWorkItem, reconnectSuspended=true, publishState .suspended]
O --> P[sidebar Reconnect button + notification]
P --> Q{user clicks Reconnect?}
Q -- yes --> R[reconnectRemoteConnection, new controller, all state reset]
R --> A
E -->|retryDelay expires| S[connectLocked]
S -->|success| T[handleProxyBrokerUpdateLocked .ready, clears reconnectSuspended, bumps generation]
S -->|failure| A
Reviews (9): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
…t control Fixes #5734. Every scheduled reconnect retry now kicks a quick reachability probe (ssh -G endpoint resolution + short-timeout TCP connect; first ProxyJump hop when present, indeterminate for ProxyCommand). Three consecutive unreachable probes suspend the loop instead of retrying indefinitely: the controller cancels the pending retry and publishes the new .suspended connection state with a localized detail. Manual control shares the existing Workspace.reconnectRemoteConnection path: an inline sidebar Reconnect button on the (still visible) SSH row, the workspace context menu, and new "cmux workspace reconnect" / "cmux workspace disconnect" CLI verbs over the existing workspace.remote.reconnect/disconnect socket methods. Suspended workspaces are exempt from remote demotion, surface a status entry, sidebar log line, and notification, and reattach to persistent PTY sessions on manual reconnect where the relay layer supports it. New strings localized in en/ja/ko/uk, matching their sibling families; previously uncataloged context-menu reconnect/disconnect labels added in the same locales. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ect-manual-control # Conflicts: # Resources/Localizable.xcstrings # cmux.xcodeproj/project.pbxproj
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 `@docs/cli-contract.md`:
- Line 98: Update the markdown table cell describing the workspace verbs to
escape the pipe characters inside the `--workspace <id|ref|index>` fragment so
it reads `--workspace <id\|ref\|index>`; locate the table row for the
`workspace` namespace (the cell mentioning verbs `list, create, close, rename,
select, reconnect, disconnect, group` and the `workspace reconnect`/`workspace
disconnect` text) and replace the unescaped `<id|ref|index>` with
`<id\|ref\|index>` to maintain table consistency (same pattern used elsewhere
for `--workspace`).
🪄 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
Run ID: 2498f341-232a-494e-b727-768586cf5667
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (11)
CLI/cmux.swiftResources/Localizable.xcstringsSources/ContentView.swiftSources/Sidebar/SidebarWorkspaceSnapshotRefreshPolicy.swiftSources/Workspace.swiftSources/WorkspaceRemoteHostReachabilityProbe.swiftSources/WorkspaceRemoteReconnectPolicy.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swiftcmuxTests/WorkspaceRemoteReconnectPolicyTests.swiftdocs/cli-contract.md
💤 Files with no reviewable changes (1)
- Resources/Localizable.xcstrings
…ble + manual Reconnect (manaflow-ai#5734) # Conflicts: # .github/swift-file-length-budget.tsv # ghostty
…ests - "cmux workspace reconnect/disconnect" no longer falls back to the caller's CMUX_WORKSPACE_ID when an explicit --window is supplied, so the server resolves that window's selected workspace instead of a workspace from a different window (matches the rename command's convention). - WorkspaceRemoteHostReachabilityProbe.resolveEndpoint gains an sshConfigFile test seam (ssh -F) so resolver tests pin /dev/null and stay hermetic against the ambient ~/.ssh/config; production callers keep honoring the user's real config. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| let deadline = Date().addingTimeInterval(sshResolveTimeout) | ||
| while process.isRunning, Date() < deadline { | ||
| usleep(20_000) | ||
| } | ||
| if process.isRunning { | ||
| process.terminate() | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Blocking poll loop on shared
probeQueue thread
runSSHConfigResolution is dispatched onto probeQueue (the same queue used by probeTCP and its NWConnection callbacks). The usleep(20_000) busy-poll loop can block that thread for up to the full sshResolveTimeout (3 s). Any other probe dispatched to probeQueue during that window — including NWConnection state updates from a concurrent TCP probe — is stalled behind it. For users with multiple suspended SSH workspaces whose probes fire near-simultaneously, each probe can delay the next by a full 3 s sleep cycle.
The fix is to use process.terminationHandler (a real signal from the owning subsystem) and post back via a DispatchWorkItem cancel for the timeout side, instead of a polling sleep loop.
There was a problem hiding this comment.
Fixed in 2fe454e — ssh -G resolution now runs on the global utility pool, so concurrent probes don't serialize on probeQueue and NWConnection callbacks stay unblocked; only the connection state updates and the timeout remain on the serial queue.
— Claude Code
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)
Sources/WorkspaceRemoteHostReachabilityProbe.swift (1)
24-27:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon't serialize every workspace probe behind one blocking queue.
runSSHConfigResolutioncan holdprobeQueuefor up tosshResolveTimeout, and that same serial queue also drives every other probe'sNWConnectioncallbacks and timeout block. A single slowssh -Gtherefore delays unrelated workspaces' probes past their configured deadlines, so suspension timing starts depending on queue backlog instead of the host being checked. Split the blocking resolution work off this shared queue, or make the queue concurrent and keep synchronization local to each probe.Also applies to: 39-55, 196-219, 261-264
🤖 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 `@Sources/WorkspaceRemoteHostReachabilityProbe.swift` around lines 24 - 27, The serial probeQueue used by WorkspaceRemoteHostReachabilityProbe serializes long blocking work (notably runSSHConfigResolution which can hold the queue for up to sshResolveTimeout) and thus delays other probes' NWConnection callbacks and timeout handlers; change the design so blocking SSH resolution does not run on the shared serial queue — either make probeQueue concurrent and confine synchronization to per-probe state, or create a separate DispatchQueue (e.g., sshResolutionQueue) and dispatch runSSHConfigResolution (and any blocking file/exec work) onto that queue while keeping NWConnection callback and short-lived probe state updates on the original probeQueue; update all uses (including runSSHConfigResolution, NWConnection callback dispatches, and timeout handlers) to ensure only non-blocking operations run on the shared probeQueue and that per-probe locking/synchronization is local to each probe.
🤖 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 `@Sources/WorkspaceRemoteHostReachabilityProbe.swift`:
- Around line 24-27: The serial probeQueue used by
WorkspaceRemoteHostReachabilityProbe serializes long blocking work (notably
runSSHConfigResolution which can hold the queue for up to sshResolveTimeout) and
thus delays other probes' NWConnection callbacks and timeout handlers; change
the design so blocking SSH resolution does not run on the shared serial queue —
either make probeQueue concurrent and confine synchronization to per-probe
state, or create a separate DispatchQueue (e.g., sshResolutionQueue) and
dispatch runSSHConfigResolution (and any blocking file/exec work) onto that
queue while keeping NWConnection callback and short-lived probe state updates on
the original probeQueue; update all uses (including runSSHConfigResolution,
NWConnection callback dispatches, and timeout handlers) to ensure only
non-blocking operations run on the shared probeQueue and that per-probe
locking/synchronization is local to each probe.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8cbbe7c5-7c5f-4cd7-a3ce-e92642f5ce76
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
CLI/cmux.swiftSources/WorkspaceRemoteHostReachabilityProbe.swiftcmuxTests/WorkspaceRemoteReconnectPolicyTests.swift
- probeTCP clears the NWConnection stateUpdateHandler before cancel so the handler/connection cycle can't leak one connection per backoff retry while the host stays reachable but bootstrap keeps failing. - "cmux workspace --help" now lists the reconnect/disconnect verbs with their targeting rules and examples, keeping CLI discovery coherent with docs/cli-contract.md. - Refresh the CLI/cmux.swift length budget for the help-text growth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…be lock - Restore the ghostty submodule pointer to origin/main's commit (34cbf18): the main-merge conflict resolution accidentally staged a stale local submodule checkout via git add -A. No ghostty change belongs to this PR. - Move WorkspaceRemoteHostProbeOutcome into its own file so the policy file carries one top-level type (Aziz file-organization policy). - probeTCP's first-finisher latch needs no NSLock: both finish paths (NWConnection state updates and the timeout) already run on the serial probeQueue, so plain queue confinement replaces the lock (Aziz concurrency policy). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Autoreview triage for the Aziz policy findings (canonical helper, --mode branch): Fixed:
Rejected (with rationale):
|
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
| consecutiveUnreachableProbeCount = 0 | ||
| reachabilityProbeGeneration &+= 1 | ||
| guard proxyEndpoint != endpoint else { |
There was a problem hiding this comment.
reconnectSuspended is not cleared when the proxy broker self-heals while the controller is already suspended. If WorkspaceRemoteProxyBroker succeeds through its own internal short retries during the suspended window, handleProxyBrokerUpdateLocked(.ready) fires, the workspace transitions to .connected, and reconnectSuspended stays true. The next connection failure calls scheduleReconnectLocked, which hits guard !reconnectSuspended and returns silently — no retry is scheduled and no UI affordance appears, leaving the workspace durably stuck in .error with no automatic or visible recovery path. Compare with stopAllLocked, which correctly resets all three counters together.
| consecutiveUnreachableProbeCount = 0 | |
| reachabilityProbeGeneration &+= 1 | |
| guard proxyEndpoint != endpoint else { | |
| consecutiveUnreachableProbeCount = 0 | |
| reconnectSuspended = false | |
| reachabilityProbeGeneration &+= 1 | |
| guard proxyEndpoint != endpoint else { |
There was a problem hiding this comment.
Fixed in 2fe454e — the proxy .ready handler now clears reconnectSuspended alongside the retry/streak resets, so a connection established while suspended can't strand a later failure behind the suspended guard. (Today suspension implies no live broker lease, so the path was unreachable, but the state machine is now self-consistent either way.)
— Claude Code
…ect-manual-control # Conflicts: # .github/swift-file-length-budget.tsv # Resources/Localizable.xcstrings
- Run ssh -G endpoint resolution on the global utility pool instead of the serial probeQueue so simultaneous probes from multiple suspended workspaces don't serialize behind its bounded blocking wait, and NWConnection callbacks stay unblocked (Greptile P1). - Clear reconnectSuspended in the proxy .ready handler alongside the other policy resets so a connection established while suspended can't leave a later failure unable to reschedule retries (Greptile P1). - Refresh the Workspace.swift length budget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The CLI target already localizes usage blocks (cli.claude-teams.usage et al) and error strings (cli.error.*), so the new workspace reconnect/disconnect help and the touched subcommand error messages follow the same pattern: cli.workspace.usage, cli.error.workspaceSubcommandRequired, and cli.error.workspaceSubcommandUnknown, each with en/ja/ko/uk entries in Localizable.xcstrings. This also corrects the PR's earlier audit note that claimed CLI text was conventionally unlocalized. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ect-manual-control # Conflicts: # .github/swift-file-length-budget.tsv
|
Autoreview triage, round 3:
Rejected (with rationale):
|
…ect-manual-control # Conflicts: # Resources/Localizable.xcstrings
…evert The revert of PR #5767 also unwrapped three live user-facing workspace CLI strings (subcommand-required error, unknown-subcommand error, and the `workspace` usage text) back to bare literals, and deleted the `workspace` row from the CLI contract docs. The `workspace` command and its remaining verbs (list, create, close, rename, select, group) still exist post-revert, so: - Re-wrap the three strings in String(localized:) using the existing catalog keys, dropping the removed reconnect/disconnect verbs. - Update en/ja/ko/uk catalog values for those keys to match. - Restore the `workspace` contract row with the reduced verb set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* Revert PR 5767 remote reconnect suspension This reverts commit 8efa28b. * Keep workspace CLI strings localized and restore contract row after revert The revert of PR #5767 also unwrapped three live user-facing workspace CLI strings (subcommand-required error, unknown-subcommand error, and the `workspace` usage text) back to bare literals, and deleted the `workspace` row from the CLI contract docs. The `workspace` command and its remaining verbs (list, create, close, rename, select, group) still exist post-revert, so: - Re-wrap the three strings in String(localized:) using the existing catalog keys, dropping the removed reconnect/disconnect verbs. - Update en/ja/ko/uk catalog values for those keys to match. - Restore the `workspace` contract row with the reduced verb set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Fixes #5734
Problem
Switching networks (plane wifi → hotspot → office, captive portals) drops SSH remote workspaces, and the auto-reconnect loop in
WorkspaceRemoteSessionControllerretries indefinitely while the host is unreachable —scheduleReconnectLockedhas no halt condition, and the user has no way to say "stop — I'll reconnect when I'm ready."Pre-fix repro (local route-drop simulation)
Configured an SSH remote workspace against a connection-refused endpoint (
nobody@127.0.0.1:49999) on a Debug build of the pre-fix HEAD and polledworkspace.remote.statusfor 5 minutes:Controller debug log shows
remote.session.connect.begin retry=0…8with 60s-capped exponential backoff and no bound.Fix
Reconnect policy (built into the existing state machine, not a parallel one):
WorkspaceRemoteReconnectPolicy— a pure decision function: every scheduled retry kicks a quick reachability probe; consecutive unreachable probes build a streak, and at 3 the loop suspends instead of rescheduling. Reachable/indeterminate probes reset the streak, so transient blips (sleep/wake, wifi handoff) keep today's backoff behavior (Make Cloud VM SSH sessions resilient to sleep and reconnects #3776 unaffected).WorkspaceRemoteHostReachabilityProbe— resolves the effective endpoint withssh -G(honors~/.ssh/configaliases,HostNameoverrides, and probes the firstProxyJumphop), then attempts a short-timeout TCP connect viaNWConnection.ProxyCommandtransports can't be probed directly and report indeterminate, which never suspends — the policy only halts on positive evidence of unreachability.remote.session.reconnect.suspended, and publishes the newWorkspaceRemoteConnectionState.suspendedwith a localized detail. Suspended workspaces are exempt from remote-workspace demotion so the configuration (and the reconnect affordance) survives the last terminal session dying.Manual control (one shared action path —
Workspace.reconnectRemoteConnection()— for every entrypoint):cmux workspace reconnect/cmux workspace disconnectverbs calling the existingworkspace.remote.reconnect/workspace.remote.disconnectsocket methods. Targets a positional/--workspacehandle, then the caller's workspace, then the selected one.On manual reconnect the existing
configureRemoteConnectionpath preserves persistent PTY session identity, so terminals reattach to live remote sessions where the relay/cmuxd-remote layer supports it; when the remote session is gone, the existing ended-session banner ("falling back to a local shell…") reports it rather than silently spawning a fresh shell.Two-commit red/green structure
WorkspaceRemoteReconnectPolicyencoding the current never-suspend behavior plus tests asserting the desired halt-on-unreachable policy → the suspend assertions fail (verified locally: 3 failed / 3 passed).Post-fix verification (local sshd + severable TCP proxy)
User-level
sshdon127.0.0.1:2299behind a kill-able TCP proxy on:2300(kill = route drop):state=connected, persistent PTY session listedstate=suspended, retry loop stops (debug log goes quiet)cmux workspace reconnect→state=connected, same persistent PTY session reattached(Exact transcripts in the PR comments if needed.)
Scope notes
CMUX_SSH_RECONNECT_LIMIT, default 20) and prints attempts in-terminal; unchanged here. In-terminal reconnect UI is Show SSH reconnection attempts in terminal #1530; the persistent-remote-daemon architecture is Persistent SSH relay daemon that survives SSH disconnects #2696.WorkspaceRemoteProxyBrokerretains its internal short retries; sustained transport failures escalate to the controller's re-bootstrap path, where this policy applies.KeyboardShortcutSettingschanges needed).Localization audit
New user-facing strings (
remote.status.suspended,sidebar.remote.help.suspended,sidebar.remote.reconnect.button,sidebar.remote.reconnect.help,remote.state.suspended.detail,remote.statusEntry.suspended,remote.notification.suspendedTitle) are added toResources/Localizable.xcstringsin en/ja/ko/uk, matching the locale coverage of theirremote.status.*/sidebar.remote.*sibling families. The previously-uncataloged workspace context-menu reconnect/disconnect labels (contextMenu.{re,dis}connectWorkspace{,s}) were also added in the same four locales. CLI help/error text follows the existing unlocalized CLI conventions;docs/cli-contract.mdis an English-only doc. No web message catalogs are affected.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Pause SSH auto-reconnect when an SSH host is unreachable and add a manual Reconnect control in the UI and CLI. This stops endless retry loops and lets users reconnect when ready (fixes #5734).
New Features
ssh -G(honors aliases/HostName; probes firstProxyJumphop;ProxyCommand→ indeterminate) and runs a short TCP check; includes ansshConfigFileseam for hermetic tests.cmux workspace reconnect/disconnectcallworkspace.remote.reconnect/workspace.remote.disconnect; with--window, target that window’s selected workspace (noCMUX_WORKSPACE_IDfallback);cmux workspace --helpanddocs/cli-contract.mdupdated; CLI help and subcommand errors localized (en/ja/ko/uk).Bug Fixes
NWConnectionstateUpdateHandlerbeforecancel()to avoid a retain cycle and per-retry leaks; first-finisher latch relies on the serial probe queue.ssh -Gendpoint resolution on a concurrent utility queue so simultaneous probes don’t block NW callbacks..readyso later failures can reschedule retries.Written for commit e93aa48. Summary will update on new commits.
Summary by CodeRabbit
New Features
Connectivity
UI
CLI
Tests
Localization