Prevent stalled remote PTY starts and bound wedged reattach loops - #9111
Merged
Merged
Conversation
|
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:
📝 WalkthroughWalkthroughThe change adds SSH PTY bridge exit-code classification and bounded no-progress retries, centralizes retry-loop generation, updates bridge reconciliation, and adds localization and regression coverage. It also coordinates concurrent remote PTY session startup, RPC cancellation, and shutdown handling. ChangesSSH PTY retry handling
Remote PTY hub coordination
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PTYBridge
participant CMUXCLI
participant RetryLoop
participant RemotePTY
PTYBridge->>CMUXCLI: report EOF with output and uptime
CMUXCLI->>RetryLoop: classify closure and retry budget
RetryLoop-->>CMUXCLI: return exit code
CMUXCLI->>RemotePTY: reconcile or retry attach
sequenceDiagram
participant AttachRequest
participant rpcRequestDispatcher
participant rpcServer
participant wsPTYHub
AttachRequest->>rpcRequestDispatcher: dispatch pty.attach
rpcRequestDispatcher->>rpcServer: handle attachment with context
rpcServer->>wsPTYHub: prepareAttachment
wsPTYHub-->>rpcServer: publish or reuse session
rpcServer-->>rpcRequestDispatcher: write attach response
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 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 |
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/SSHPTYAttachStartupCommandBuilder.swift (1)
206-212: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThird divergent
shellQuotecopy introduced by this extraction.This file's
shellQuote(fast-path regex + single-quote escape) is duplicated inCLI/cmux.swift, and the newSSHPTYAttachRetryLoop.swift(created as part of this refactor) adds a third, slightly different implementation that always wraps in quotes without the safe-pattern fast path. Consider extracting one shared shell-quoting utility that all three call sites use, so quoting behavior can't drift.🤖 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/SSHPTYAttachStartupCommandBuilder.swift` around lines 206 - 212, Extract the shell-quoting logic from SSHPTYAttachStartupCommandBuilder.shellQuote, CLI/cmux.swift, and SSHPTYAttachRetryLoop.swift into one shared utility, then update all three call sites to use it. Preserve the existing safe-pattern fast path and single-quote escaping behavior consistently across every caller, removing the duplicated implementations.
🤖 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 `@CLI/SSHPTYAttachRetryLoop.swift`:
- Around line 59-64: Update the case branches generated by SSHPTYAttachRetryLoop
to replace the hardcoded 254 and 255 literals with the raw values of
SSHPTYAttachExitCode.bridgeClosedSessionRunning and
SSHPTYAttachExitCode.retryableTransient, matching the existing noProgressStatus
enum-derived branch.
In `@cmuxTests/SSHPTYAttachNoProgressRetryTests.swift`:
- Around line 47-169: Extract the duplicated temporary-directory, fake CLI/fake
sleep, executable-permission, and base environment setup from
repeatedZeroProgressClosuresStop and
normalRetryableClosureResetsNoProgressStreak into a private helper. Parameterize
the helper with the fake CLI’s per-attempt decision behavior and return the
shared test result/artifacts needed for assertions, while keeping each test’s
distinct expected attempts, policy log, status, and stderr assertions in the
tests.
In `@daemon/remote/cmd/cmuxd-remote/ws_pty_test.go`:
- Around line 256-273: Update the concurrent attach test around attach and
releaseStart so it waits until the second caller has entered the in-flight-start
waiting branch before closing releaseStart. Add or use a hub-owned waiter
counter or test hook channel, then apply a deadline-bounded wait and preserve
the existing assertions for both attach results and a single PTY start.
---
Outside diff comments:
In `@Sources/SSHPTYAttachStartupCommandBuilder.swift`:
- Around line 206-212: Extract the shell-quoting logic from
SSHPTYAttachStartupCommandBuilder.shellQuote, CLI/cmux.swift, and
SSHPTYAttachRetryLoop.swift into one shared utility, then update all three call
sites to use it. Preserve the existing safe-pattern fast path and single-quote
escaping behavior consistently across every caller, removing the duplicated
implementations.
🪄 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: 7d20e041-1a7f-496b-aec2-7ca485123958
📒 Files selected for processing (11)
CLI/CMUXCLI+SSHPTYAttachBridge.swiftCLI/SSHPTYAttachExitCode.swiftCLI/SSHPTYAttachRetryLoop.swiftCLI/cmux.swiftResources/Localizable.xcstringsSources/SSHPTYAttachStartupCommandBuilder.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/SSHPTYAttachNoProgressRetryTests.swiftdaemon/remote/cmd/cmuxd-remote/ws_pty.godaemon/remote/cmd/cmuxd-remote/ws_pty_test.go
austinywang
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachExitCode.swift`:
- Line 112: Remove the elapsed-time sleep from the reconnect command assembled
in SSHPTYAttachExitCode. Replace delay-based retry coordination with an explicit
lifecycle/readiness result that determines when the next attach attempt may
proceed, preserving reconnect behavior without timing-based backoff.
- Around line 70-73: Update the noProgressFormat construction in
SSHPTYAttachExitCode to use ICU pluralization, selecting explicit .one and
.other localized catalog entries based on the retry limit so singular output
says “1 attempt” and other counts say “N attempts.” Add matching .one and .other
entries to the localization catalog while preserving remoteCommandShellQuoted
handling.
🪄 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: 4c2f5e34-ada1-4a8a-8ec4-1be1684adb1b
📒 Files selected for processing (8)
CLI/CMUXCLI+SSHPTYAttachBridge.swiftCLI/SSHPTYAttachExitCode.swiftCLI/cmux.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachExitCode.swiftSources/SSHPTYAttachStartupCommandBuilder.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SSHPTYAttachExitCodeClassifierTests.swiftcmuxTests/SSHPTYAttachNoProgressRetryTests.swift
💤 Files with no reviewable changes (2)
- CLI/SSHPTYAttachExitCode.swift
- cmux.xcodeproj/project.pbxproj
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)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachExitCode.swift (1)
48-56: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the no-progress retry boundary.
With
limit == 3,currentRetry == 2represents two prior retries and should allow the third attempt, butcurrentRetry + 1 < limitreturnsfalse. It can also overflow forInt.max.Proposed fix
- currentRetry >= 0 && limit > 0 && currentRetry + 1 < limit + currentRetry >= 0 && limit > 0 && currentRetry < limit🤖 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 `@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachExitCode.swift` around lines 48 - 56, Update SSHPTYAttachExitCode.hasNoProgressRetryRemaining so a valid currentRetry allows attempts while it is below limit, including currentRetry == limit - 1; avoid adding 1 to currentRetry to prevent Int.max overflow, while preserving the existing nonnegative retry and positive limit validation.
🤖 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
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachExitCode.swift`:
- Around line 48-56: Update SSHPTYAttachExitCode.hasNoProgressRetryRemaining so
a valid currentRetry allows attempts while it is below limit, including
currentRetry == limit - 1; avoid adding 1 to currentRetry to prevent Int.max
overflow, while preserving the existing nonnegative retry and positive limit
validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b5130ebe-1666-4de9-b410-84ba8886b3e3
📒 Files selected for processing (3)
CLI/cmux.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachExitCode.swiftcmuxTests/SSHPTYAttachNoProgressRetryTests.swift
There was a problem hiding this comment.
Actionable comments posted: 3
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/ws_pty.go (1)
872-886: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftOwner-scoped cancellation is propagated to unrelated waiters, and the started session is discarded even when live waiters want it.
At Line 913-914 the start is failed with the owner's
ctx.Err()and the freshly started session is discarded. Waiters that joined this start (Line 876-879) then receivecontext.Canceledverbatim, even though their own connections are healthy — so one client disconnecting during allocation turns into a spurious failure for every other client attaching the same session, and the just-allocated PTY is torn down and must be re-allocated.Given this PR also bounds client-side retries after consecutive no-progress attaches, these owner-scoped failures can count against unrelated clients' retry budgets.
Suggest distinguishing owner-scoped errors (owner ctx cancellation) from session-scoped errors: on owner cancellation, either hand ownership to a waiter (keep the session and let the loop republish it) or signal waiters to retry the loop instead of returning the owner's error.
♻️ Sketch: let waiters retry instead of inheriting owner cancellation
if start := h.startingSessions[sessionKey]; start != nil { closedCh := h.closedCh h.mu.Unlock() select { case <-start.done: - if start.err != nil { + // Owner-scoped cancellation is not a failure for this caller; + // retry the loop and start the session ourselves. + if start.err != nil && !start.ownerCanceled { return nil, nil, nil, start.err }with
ownerCanceledset alongsidestart.errin thecase ctx.Err() != nil:branch.Also applies to: 908-921
🤖 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/ws_pty.go` around lines 872 - 886, Update the starting-session wait path around startingSessions and the owner-cancellation handling near the session allocation failure branch to distinguish owner ctx cancellation from session-scoped errors. When the allocating owner cancels, do not return that owner’s context error to unrelated waiters; signal them to retry the session-start loop, while preserving and republishing a successfully allocated PTY when live waiters remain. Continue propagating genuine session errors and each waiter’s own ctx cancellation normally.
🤖 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.go`:
- Around line 388-398: Update rpcRequestDispatcher to track dispatched
goroutines with a sync.WaitGroup, incrementing before each dispatch goroutine
and calling Done on completion, then expose dispatcher.wait(). In
daemon/remote/cmd/cmuxd-remote/main.go lines 388-398, and apply the same
ordering at lines 1318-1321 in runRPCServerWithReader, cancel the connection,
wait for in-flight handlers, and only then call writer.flush(); ensure deferred
teardown does not flush before cancellation and waiting.
- Around line 2264-2274: Update the context-error response in the attachment
flow around ctx.Err() to use the same product-level message as the sibling
connection-loss branch near the subsequent error handling, rather than exposing
err.Error(). Keep the existing cleanup, error code, and response structure
unchanged.
- Around line 143-180: Update rpcRequestDispatcher.dispatch and the async PTY
attach handling to use the existing request-response path for id-less requests,
preserving notification semantics: id-less pty.attach must route through
handleNotificationResponse and emit a pty.error event rather than writing an
unconditional response frame, while requests with IDs retain normal responses
and existing concurrency/error behavior.
---
Outside diff comments:
In `@daemon/remote/cmd/cmuxd-remote/ws_pty.go`:
- Around line 872-886: Update the starting-session wait path around
startingSessions and the owner-cancellation handling near the session allocation
failure branch to distinguish owner ctx cancellation from session-scoped errors.
When the allocating owner cancels, do not return that owner’s context error to
unrelated waiters; signal them to retry the session-start loop, while preserving
and republishing a successfully allocated PTY when live waiters remain. Continue
propagating genuine session errors and each waiter’s own ctx cancellation
normally.
🪄 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: a9ab740c-412a-4d10-b7fd-e01906eff32e
📒 Files selected for processing (4)
daemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.godaemon/remote/cmd/cmuxd-remote/ws_pty.godaemon/remote/cmd/cmuxd-remote/ws_pty_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. |
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. |
This was referenced Aug 3, 2026
4 of 6 tasks
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.
Closes #8750
Issue: #8750
Root cause
The persistent PTY path had several lifecycle hazards that compose into the reported affected-slot-only wedge:
wsPTYHub.prepareAttachmentallocated the PTY and started the shell while holding the hub-global mutex. One stalled OS-level start therefore serialized every new attach/session operation in that daemon, while already-running PTY pumps could keep streaming.pty.attachshared the synchronous RPC read loop. A request that stopped making progress could retain per-request state and block unrelated work on that transport; timed-out callers had no protocol operation that canceled the in-flight server request.These mechanisms match the forensic split in #8750: existing attachments stay usable, but new attachment establishment on one old persistent server stops progressing.
Fix
Isolated, bounded server lifecycle
pty.attachasynchronously and bound each RPC connection to 32 in-flight attaches.unavailableinstead of creating unbounded goroutines.Cancellable attachment protocol
pty.attach.cancel, keyed by request plus session/attachment identity, so a timed-out attach is canceled without destroying healthy calls on the same transport.Diagnosable, bounded client recovery
unavailablemaps to status 251 (retry without reauthentication) instead of the generic fatal path.The bounded fatal path intentionally does not kill the entire remote daemon automatically, because an affected slot can contain unrelated non-tmux work. It prevents the infinite loop while the server changes remove and contain the shared lifecycle failure modes.
Reproduction steps (verbatim from #8750)
Regression coverage
The commit history keeps the regressions and fixes separate. Behavior coverage includes:
Local validation on final HEAD
0a68c39c07:CmuxFoundation: 155/155 testsCmuxRemoteDaemon: 25/25 testsCmuxRemoteSession: 111/111 testsCmuxRemoteWorkspace: 87/87 testsxcodebuild testor XCUITest, per issue instructionsExact-HEAD CI: 18/18 jobs passed in https://github.com/manaflow-ai/cmux/actions/runs/30450715809, including remote-daemon, Swift-package, app-host, release-build, and workflow guard lanes. CodeRabbit and Socket checks passed; all review threads are resolved.
Localization audit: the terminal diagnostics use
String(localized:); English and Japanese entries are present inResources/Localizable.xcstrings. There is no web-facing string change. The Swift warning budget was untouched; the repository's current file/package guards passed.Tagged dev-build verification
Cloud build https://github.com/manaflow-ai/cmux/actions/runs/30454380014 succeeded at the exact final HEAD. In the isolated
issue-8750-cmuxd-remote-attach-wedgeapp, usingcmux ssh austinwang@127.0.0.1 --port 2296 --ssh-option ClearAllForwardings=yeswithzsh -il:Residual uncertainty
The original multi-day Ubuntu 24.04 state was not available for a goroutine dump, and its probabilistic ~3.5-day trigger cannot be recreated within this run. The verification therefore covers the identified failure mechanisms deterministically and stress-tests reattach/transport/server recovery, but does not claim an end-to-end reproduction of the exact multi-day host state.