Repository navigation
Retract recovered daemon transport errors from the workspace sidebar - #9472
Merged
austinywang merged 5 commits intoAug 3, 2026
Merged
Conversation
…r error Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe workspace now removes proxy-only remote-daemon sidebar errors when daemon status returns to ChangesRemote daemon recovery
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant RemoteDaemon
participant Workspace
participant SidebarLog
RemoteDaemon->>Workspace: Report ready status
Workspace->>Workspace: Clear proxy-only daemon artifacts
Workspace->>SidebarLog: Remove remote-daemon error
🚥 Pre-merge checks | ✅ 25✅ Passed checks (25 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 |
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 6922-6927: In Sources/Workspace.swift lines 6922-6927, update the
ready-state recovery logic to clear sidebar artifacts only when the
authoritative typed lifecycle state is .connected, handling either event order
through one guarded cleanup path and failing closed when unavailable. In
cmuxTests/WorkspaceRemoteConnectionTests.swift lines 3271-3319, establish
.connected before the positive recovery sequence and add a disconnected-state
case verifying cleanup does not occur.
🪄 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: 2b543ab3-d5fb-406d-8278-89a0d645f1ac
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
Comment on lines
+6922
to
+6927
| if status.state == .ready { | ||
| // #8917: a transport bounce that re-bootstrapped successfully is | ||
| // not a workspace failure, so retract its sidebar error the same | ||
| // way the connection-state path retracts on `.connected`. | ||
| clearProxyOnlyRemoteDaemonSidebarArtifacts() | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce and test the connected-state recovery precondition.
The production code clears the daemon error on .ready alone, while the regression test starts with a .disconnected connection. This permits the test to pass for an implementation that hides an error before the connection recovers.
Sources/Workspace.swift#L6922-L6927: Require the authoritative connection state to be.connectedbefore clearing. Handle either event order through one guarded cleanup path.cmuxTests/WorkspaceRemoteConnectionTests.swift#L3271-L3319: Establish.connectedbefore the positive sequence and add a negative disconnected-state case.
As per path instructions, use the authoritative typed lifecycle state and fail closed when it is unavailable.
📍 Affects 2 files
Sources/Workspace.swift#L6922-L6927(this comment)cmuxTests/WorkspaceRemoteConnectionTests.swift#L3271-L3319
🤖 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 6922 - 6927, In Sources/Workspace.swift
lines 6922-6927, update the ready-state recovery logic to clear sidebar
artifacts only when the authoritative typed lifecycle state is .connected,
handling either event order through one guarded cleanup path and failing closed
when unavailable. In cmuxTests/WorkspaceRemoteConnectionTests.swift lines
3271-3319, establish .connected before the positive recovery sequence and add a
disconnected-state case verifying cleanup does not occur.
Source: Path instructions
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. |
austinywang
deleted the
issue-8917-recovered-daemon-transport-bounce-leaves
branch
August 3, 2026 22:32
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.
A transient daemon-transport drop on an SSH remote workspace escalates to a
.errordaemon status, andapplyRemoteDaemonStatusUpdateappendsRemote daemon error (<host>): Remote daemon transport needs re-bootstrap after proxy failure ...to the sidebar log. When the transport re-bootstraps a few seconds later the handler only reset the dedup fingerprint, so that entry stayed the workspace's last log entry, which is exactly what the sidebar row renders (SidebarWorkspaceSnapshotFactory->latestLog), and it persisted across restarts. The connection-state path already self-heals on.connected; the daemon path did not.The daemon-status handler now retracts its own proxy-only artifacts when the daemon returns to
.ready, via a newclearProxyOnlyRemoteDaemonSidebarArtifacts()that removes onlyremote-daemon-sourced entries matchingisProxyOnlyRemoteError. It deliberately does not reuseclearProxyOnlyRemoteSidebarArtifacts(), which also drops the connection-state status entry and proxy notifications owned byapplyRemoteConnectionStateUpdate; a healthy daemon does not imply a healthy connection.Fixes #8917
Testing
WorkspaceRemoteConnectionTests.testRecoveredDaemonTransportBounceClearsSidebarDaemonErrordrives an ordinary SSH workspace with persistent PTY through.ready->.error(proxy-only re-bootstrap message) ->.readyand asserts noremote-daemonentry remains and the last log entry is not an error.scripts/reload.sh --tag sym8917) succeeded, launched, and answered on its debug socket; the app was then quit. A live end-to-end repro was not run because it needs a real SSH remote workspace plus a ~40s network interruption, which this machine has no sshd or remote host for. The state transition itself is covered deterministically by the unit test.Summary by CodeRabbit
Bug Fixes
Tests