Stop reporting recovered remote reconnects as errors - #9664
Closed
austinywang wants to merge 3 commits into
Closed
austinywang wants to merge 3 commits into
austinywang wants to merge 3 commits into
Conversation
Quitting or restarting cmux while a remote PTY is attached surfaced two failures for an outage that healed itself seconds later: the terminal printed a red "Error: ssh-pty-attach: bridge closed before remote PTY exit could be confirmed: remote daemon is not ready", and the sidebar kept a red "Remote daemon error" row on a workspace that was connected. The CLI now leaves reporting to the persistent attach wrapper whenever that wrapper will retry the failure itself; the wrapper already prints its own reattaching notice, so the error line was duplicate and made a successful reconnect look broken. Failures the wrapper will not retry still print. The sidebar now retracts every daemon error once the daemon reports ready, instead of only the proxy-only subset. Relay bounces publish details that say "retrying in 2 seconds" and always schedule a reconnect, but did not match the proxy-only substrings, so they stayed red forever. Errors the daemon never recovers from still persist, because retraction only runs on ready. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe CLI suppresses duplicate errors from the persistent SSH PTY attach wrapper. Remote workspace readiness clears resolved remote-daemon errors when active remote terminals exist. Tests cover successful recovery and retained errors. ChangesRemote SSH recovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (23 passed)
✨ Finishing Touches 💡 1📝 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 |
active_terminal_sessions=0 on a workspace that still reports Connected is the signature of a remote terminal that fell back to a local shell: the sidebar row, the tab title, and the connection state all still name the remote host while the prompt is local. Retracting daemon errors there would erase the only visible sign the workspace is not remote, so retraction now requires a live remote terminal session as well as a ready daemon. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![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
🤖 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 `@Sources/Workspace.swift`:
- Around line 2760-2768: Update clearResolvedRemoteDaemonSidebarArtifacts so
daemon error entries are removed only when an authoritative remote terminal
session is in the .connected state, not merely when
hasActiveRemoteTerminalSessions is true; alternatively, invoke the cleanup again
when RemoteTerminalLiveness records the later .connected transition.
🪄 Autofix
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: 3d5c2510-359f-440f-8cbc-2ffd44bdf8ed
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/WorkspaceRemoteDaemonRecoveryTests.swift
Comment on lines
+2760
to
+2768
| // A ready daemon only proves the management lane recovered. When the | ||
| // workspace has no remote terminal session, its terminal has fallen | ||
| // back to a local shell while the row still reads "Connected", and | ||
| // retracting here would erase the only sign that anything went wrong. | ||
| // Keep the errors visible until a remote terminal is actually alive. | ||
| guard hasActiveRemoteTerminalSessions else { return } | ||
| logEntries.removeAll { entry in | ||
| entry.source == "remote-daemon" && entry.level == .error | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 --glob '*.swift' \
'hasActiveRemoteTerminalSessions|activeRemoteTerminalSessionCount|activeRemoteTerminalSurfaceIds|trackRemoteTerminalSurface|markRemoteTerminalSessionLaunching|untrackRemoteTerminalSurface|applyRemoteDaemonStatusUpdate|clearResolvedRemoteDaemonSidebarArtifacts|clearProxyOnlyRemoteSidebarArtifacts' \
Sources cmuxTestsRepository: manaflow-ai/cmux
Length of output: 50373
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
# Inspect focused declarations, lifecycle transitions, and cleanup call sites.
sed -n '1,220p' Sources/Workspace+RemoteTerminalLiveness.swift
printf '\n--- Workspace line ranges near declarations and helper ---\n'
sed -n '2650,2775p;5640,5652p;6050,6185p' Sources/Workspace.swift
printf '\n--- focused call sites ---\n'
rg -n -C 6 --glob '*.swift' \
'clearResolvedRemoteDaemonSidebarArtifacts\(|clearProxyOnlyRemoteDaemonSidebarArtifacts\(|clearProxyOnlyRemoteSidebarArtifacts\(|markRemoteTerminalSessionLaunching\(|markRemoteTerminalSessionConnected\(|trackRemoteTerminalSurface\(|activeRemoteTerminalSessionCount =|hasActiveRemoteTerminalSessions' \
SourcesRepository: manaflow-ai/cmux
Length of output: 50372
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
cat -n Sources/Workspace.swift | sed -n '6967,7005p'
printf '\n--- all Workspace calls to daemon/sidebar cleanup helpers ---\n'
rg -n -C 4 --glob '*.swift' \
'clearResolvedRemoteDaemonSidebarArtifacts\(|clearProxyOnlyRemoteDaemonSidebarArtifacts\(\)|clearProxyOnlyRemoteSidebarArtifacts\(\)' Sources | sed -n '1,260p'
printf '\n--- active session count and surface phase update context ---\n'
cat -n Sources/Workspace.swift | sed -n '6660,6820p;6470,6535p'
printf '\n--- remote daemon status transition call sites ---\n'
rg -n -C 3 --glob '*.swift' 'remoteDaemonStatus \(.\)|WorkspaceRemoteDaemonStatus|remoteDaemonStatusUpdate|applyRemoteDaemonStatusUpdate' SourcesRepository: manaflow-ai/cmux
Length of output: 25510
Gate resolved daemon-error cleanup on a connected remote terminal, not just an active session count.
trackRemoteTerminalSurface adds the panel to activeRemoteTerminalSurfaceIds and increments activeRemoteTerminalSessionCount before markRemoteTerminalSessionLaunching records .launching. If remoteDaemonStatus changes to .ready before RemoteTerminalLiveness records .connected, clearResolvedRemoteDaemonSidebarArtifacts() removes daemon .error entries while no remote PTY is live. Use the authoritative .connected session state for this cleanup, or reapply cleanup on the later connect transition.
🤖 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/Workspace.swift` around lines 2760 - 2768, Update
clearResolvedRemoteDaemonSidebarArtifacts so daemon error entries are removed
only when an authoritative remote terminal session is in the .connected state,
not merely when hasActiveRemoteTerminalSessions is true; alternatively, invoke
the cleanup again when RemoteTerminalLiveness records the later .connected
transition.
Sources: Path instructions, MCP tools
This was referenced Aug 11, 2026
lawrencecchen
added
the
stale-revisit
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
3 participants
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
Restarting cmux while a remote workspace is attached reported two failures for an outage that had already healed.
cmux ssh-pty-attachexits with a retryable status when its bridge closes, and the CLI's top-level handler printedError: ssh-pty-attach: bridge closed before remote PTY exit could be confirmed: remote daemon is not readyeven though the persistent attach wrapper immediately retried and printed its own yellow "reattaching (attempt n/∞)" notice. The CLI now leaves reporting to the wrapper whenever the wrapper will retry the failure itself. Failures the wrapper will not retry, and every non-attach command, still print normally..errorwith details likeRemote SSH relay unavailable; retrying in 2 secondsand always schedule a reconnect, but retraction-on-recovery only matched the "proxy-only" substrings, so those entries stayed red forever on a workspace that wasConnectedand working. Retraction now covers every daemon error once the daemon reports.ready— a ready daemon has by definition resolved the errors recorded before it. Outages the daemon never recovers from still persist, because retraction only runs on.ready.Testing
cmuxTests/WorkspaceRemoteDaemonRecoveryTests.swift: a recovered relay bounce retracts its sidebar error (fails before this change); an outage that never reaches.readykeeps its entry. The pre-existing Recovered daemon-transport bounce leaves a permanent error on the workspace row #8917 proxy-bounce test still passes.cmux-mac-mini), identical protocol on both builds — one fresh remote session, one app restart, no other cmux instance running:[remote-daemon] [error] Remote daemon error (cmux-mac-mini): Remote SSH relay unavailable; retrying in 2 seconds.No log entries, daemonready, workspaceconnected.Error:line no longer appears in the terminal on a bridge close the wrapper retries.Known gap (pre-existing, not from this change)
Reattaching a remote PTY after an app restart is itself unreliable, and reproduces on a build without this change: the control build ended the restored session with
^D, and the fixed build reportedssh-pty-attach: remote PTY attach failed. That is a separate defect in the reattach path; this PR only stops a recovered reconnect from being reported as a failure. Filed separately.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Stops showing errors for remote reconnects that recover on their own. The CLI suppresses duplicate errors for retryable
ssh-pty-attachdisconnects, and the sidebar clears resolved daemon errors once the daemon is ready and a remote terminal is active..readywith an active remote terminal, retract all priorremote-daemonerrors (including relay bounces); outages that never reach ready or lack a remote terminal keep their entries.Written for commit d3ebc23. Summary will update on new commits.
Summary by CodeRabbit