Retry restored SSH after boot-time network failure - #9083
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:
📝 WalkthroughWalkthroughForeground SSH authentication now classifies transport and authentication failures, retries transient failures with bounded limits, and cleans up authentication subprocesses during signals and reconnects. Startup and PTY attach tests cover retry limits, signal exits, failure classification, and retry-loop ordering. ChangesSSH foreground authentication retries
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SSHStartupScript
participant SSHForegroundAuthenticationRetryPolicy
participant SSHForegroundAuth
participant SSHPTYAttachRetryScriptBuilder
SSHStartupScript->>SSHForegroundAuthenticationRetryPolicy: wrap foreground authentication
SSHStartupScript->>SSHForegroundAuth: launch tracked authentication subprocess
SSHForegroundAuth-->>SSHForegroundAuthenticationRetryPolicy: return diagnostics and status
SSHForegroundAuthenticationRetryPolicy-->>SSHPTYAttachRetryScriptBuilder: classify mapped status
SSHPTYAttachRetryScriptBuilder->>SSHForegroundAuth: retry transient failure
SSHPTYAttachRetryScriptBuilder-->>SSHStartupScript: exit after limit or permanent failure
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/CMUXCLI`+SSHStartupScripts.swift:
- Around line 360-376: Update the foreground-auth retry flow in the generated
script around cmux_ssh_foreground_auth and cmux_ssh_auth_retry so transient
failures do not increment a shell-managed retry loop or enter the sleep-based
reconnect coordinator. Signal the persistent reconnect owner through the
existing lifecycle/network signaling mechanism, preserving terminal handling for
non-retryable failures and successful authentication.
In
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swift`:
- Around line 24-54: Pin the locale for the wrapped SSH/auth invocation so the
diagnostic text classified by transientFailurePattern and
permanentFailurePattern is deterministic. Update the nestedCommand construction
and execution path around nestedCommand and the classifier invocation to run the
wrapped command with LC_ALL=C (and the appropriate locale environment), while
preserving the existing regex classification behavior.
- Around line 57-72: The classifyingTransientFailure API currently allows
trailing commands to affect classification; constrain it to wrap only the
authentication command. Update its contract and implementation or, preferably,
ensure buildReusableForegroundAuthThenSSHPTYAttachStartupCommand terminates
successfully immediately after ssh -T ... true before appending
localCommandScript, preserving the local command’s independent exit status.
- Line 122: Update the SSH foreground authentication retry command around the
`/usr/bin/script` invocation to avoid the unsupported macOS `-F`/pipe flags. Use
only documented Apple `script(1)` behavior while still streaming classifier
output through `cmux_ssh_auth_classifier_fifo` and preserving the child
command’s exit status.
In
`@Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHForegroundAuthenticationRetryPolicyTests.swift`:
- Around line 8-118: Add coverage for the non-255 exit-status contract in the
SSHForegroundAuthenticationRetryPolicy tests by adding a test near the existing
status-mapping cases that runs exit 3 and asserts the wrapper preserves status 3
and creates no temporary files.
In `@Sources/SSHPTYAttachStartupCommandBuilder.swift`:
- Around line 95-96: Update the startup command construction around
cmux_ssh_attach_foreground_auth and cmux_ssh_attach_auth_pid to use the same
pending-signal handling as the CLI startup loop: record HUP/INT/TERM received
during authentication launch, assign the spawned process PID immediately, then
terminate that recorded PID before continuing when a pending signal exists,
preventing orphaned interactive authentication.
🪄 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: 2c7038f9-0502-4077-80bb-bdabde5373fb
📒 Files selected for processing (12)
CLI/CMUXCLI+SSHStartupScripts.swiftCLI/cmux.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHForegroundAuthenticationRetryPolicyTests.swiftSources/SSHPTYAttachStartupCommandBuilder.swiftcmux.xcodeproj/project.pbxprojcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/SSHDeepSleepReattachTests.swiftcmuxTests/SSHForegroundAuthenticationMarkerCleanupTests.swiftcmuxTests/SSHForegroundAuthenticationSignalTests.swiftcmuxTests/SSHPersistentPTYRetryLifecycleTests.swiftcmuxTests/SSHStartupSignalLifecycleTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHForegroundAuthenticationRetryPolicyTests.swift (1)
151-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace the fixed three-second synchronization delay.
/bin/sleep 3makes every run slower and can let the child exit before a loaded CI worker observes incremental classification. Block the fixture on test-controlled stdin after writingproducer-ready, then release it through aPipeonly after observingtransient\n.As per coding guidelines, tests must use completion signals or deadline-bounded predicate polls rather than fixed-duration waits.
🤖 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/Tests/CmuxFoundationTests/SSHForegroundAuthenticationRetryPolicyTests.swift` around lines 151 - 159, Replace the fixed /bin/sleep 3 synchronization in SSHForegroundAuthenticationRetryPolicyTests with test-controlled stdin blocking after the fixture writes producer-ready. Use a Pipe to release the child only after observing transient\n, and coordinate readiness/release with completion signals or deadline-bounded predicate polling rather than fixed-duration waits.Source: Coding guidelines
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swift (1)
130-131: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClose the launch-to-PID-registration signal race.
A signal between
script ... &andcmux_ssh_auth_command_pid=$!makes the trap see no auth PID, so it exits without terminating the launched authentication process. Mirror the pending-signal/launching state handling used bySources/SSHPTYAttachStartupCommandBuilder.swiftand process any deferred signal immediately after recording$!.As per path instructions, cancellation and cleanup ownership must derive from tracked lifecycle state without staleness windows.
🤖 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/SSHForegroundAuthenticationRetryPolicy.swift` around lines 130 - 131, Update the launch/trap handling in SSHForegroundAuthenticationRetryPolicy so signals arriving between starting the authentication command and recording its PID are deferred rather than handled with a missing PID. Mirror the pending-signal and launching-state lifecycle used by SSHPTYAttachStartupCommandBuilder, record the launched process identifier immediately, then process any deferred signal. Ensure cancellation and cleanup derive from the tracked lifecycle state without stale ownership windows.Source: Path instructions
🤖 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/SSHPTYAttachRetryScriptBuilder.swift`:
- Line 60: Replace the inline blocking sleep in SSHPTYAttachRetryScriptBuilder’s
generated reconnect-delay logic with a lifecycle-aware, signal-interruptible
backoff mechanism so HUP, INT, and TERM terminate the wait immediately. Preserve
the configured delay and retry behavior for normal execution, including
transient authentication failures, and add a regression test proving signals
received during backoff trigger prompt cleanup.
---
Outside diff comments:
In
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swift`:
- Around line 130-131: Update the launch/trap handling in
SSHForegroundAuthenticationRetryPolicy so signals arriving between starting the
authentication command and recording its PID are deferred rather than handled
with a missing PID. Mirror the pending-signal and launching-state lifecycle used
by SSHPTYAttachStartupCommandBuilder, record the launched process identifier
immediately, then process any deferred signal. Ensure cancellation and cleanup
derive from the tracked lifecycle state without stale ownership windows.
In
`@Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHForegroundAuthenticationRetryPolicyTests.swift`:
- Around line 151-159: Replace the fixed /bin/sleep 3 synchronization in
SSHForegroundAuthenticationRetryPolicyTests with test-controlled stdin blocking
after the fixture writes producer-ready. Use a Pipe to release the child only
after observing transient\n, and coordinate readiness/release with completion
signals or deadline-bounded predicate polling rather than fixed-duration waits.
🪄 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: 14d26a2a-6a51-44aa-a896-c7310b05eea6
📒 Files selected for processing (7)
CLI/SSHPTYAttachExitCode.swiftCLI/cmux.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachRetryScriptBuilder.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHForegroundAuthenticationRetryPolicyTests.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHPTYAttachRetryScriptBuilderTests.swiftSources/SSHPTYAttachStartupCommandBuilder.swift
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
`@Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHPTYAttachRetryScriptBuilderTests.swift`:
- Line 143: Remove the fixed Thread.sleep(forTimeInterval:) synchronization wait
from the SSH PTY attach retry test after confirming the marker exists. Send the
signal immediately after the existing real completion check, preserving the
test’s marker-based synchronization and retry assertions.
🪄 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: 08280637-7acb-42b7-bcef-76eb2492c06b
📒 Files selected for processing (2)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachRetryScriptBuilder.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHPTYAttachRetryScriptBuilderTests.swift
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. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLI/CMUXCLI+SSHStartupScripts.swift (1)
361-378: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the auth-retry classification case-pattern into a shared policy helper.
Line 378's
case "$cmux_ssh_status:$cmux_ssh_auth_established" in 254:*|\(authRetryPolicy.unclassifiedFailureExitStatus):1) ... *) ...duplicates, near-verbatim, the classification case-pattern already generated bySSHPTYAttachRetryScriptBuilder.lines(command:reauthenticates:)(254:*|\(authPolicy.unclassifiedFailureExitStatus):1) ... *) ...). The only differences are the variable-name prefix (cmux_ssh_vscmux_ssh_attach_) and the terminal action (breakvsexit 255). This is the exact retry/classification state machine the PR is trying to make robust (issue#9067); if one copy is updated (e.g., a new status code, an off-by-one in the retry-limit check) and the other isn't, initial-connection and reattachment retry behavior will silently diverge.Consider adding a small parameterized method on
SSHForegroundAuthenticationRetryPolicy(e.g. taking the status/established/retry variable names and the "exceeded" action as a shell fragment) that both this file andSSHPTYAttachRetryScriptBuildercall, so the classification logic has one source of truth.🤖 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 `@CLI/CMUXCLI`+SSHStartupScripts.swift around lines 361 - 378, Extract the duplicated authentication retry classification state machine from the reauthentication loop and SSHPTYAttachRetryScriptBuilder.lines(command:reauthenticates:) into a parameterized helper on SSHForegroundAuthenticationRetryPolicy. Have both callers supply their variable prefixes and terminal exceeded action, while preserving their existing retry-limit and status-handling behavior. Replace both inline case-pattern implementations with this shared helper output.CLI/cmux.swift (1)
10252-10328: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winEliminate the zsh-only lock implementation or keep the zsh interpreter wrapper.
zmodload zsh/systemandzsystem flockonly run when the foreground-auth prefix is executed by zsh; the code removes any explicit zsh invocation around this classifiedoneTimeCommand, so the lock path can fail early and drop the single-in-flight-auth invariant. Keep these commands behind a guard or restore the zsh wrapper.🤖 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 `@CLI/cmux.swift` around lines 10252 - 10328, Ensure the locking commands in buildReusableForegroundAuthThenSSHPTYAttachStartupCommand are executed under zsh by restoring the explicit zsh interpreter wrapper for the classified oneTimeCommand, or guard the zmodload/zsystem flock path to run only when zsh is active. Preserve the single-in-flight-auth behavior and existing retry/status handling.
🤖 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 `@cmuxTests/SSHForegroundAuthenticationSignalTests.swift`:
- Around line 47-56: In the test flow surrounding childPIDFile and the result
assertions, validate result.timedOut and result.status before reading or
unwrapping the child PID file so timeout and early-exit diagnostics, including
stderr, are reported first. Keep the existing child-process cleanup via
Darwin.kill after the PID is successfully unwrapped.
In
`@Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHPTYAttachRetryScriptBuilderTests.swift`:
- Around line 258-273: Update the waitForFile helper to perform one final
file-content check after the process-running poll loop exits, before returning
false. Preserve the deadline-bounded polling behavior and return true when the
final read contains expectedContents, including when the process exits
immediately after writing the content.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 10252-10328: Ensure the locking commands in
buildReusableForegroundAuthThenSSHPTYAttachStartupCommand are executed under zsh
by restoring the explicit zsh interpreter wrapper for the classified
oneTimeCommand, or guard the zmodload/zsystem flock path to run only when zsh is
active. Preserve the single-in-flight-auth behavior and existing retry/status
handling.
In `@CLI/CMUXCLI`+SSHStartupScripts.swift:
- Around line 361-378: Extract the duplicated authentication retry
classification state machine from the reauthentication loop and
SSHPTYAttachRetryScriptBuilder.lines(command:reauthenticates:) into a
parameterized helper on SSHForegroundAuthenticationRetryPolicy. Have both
callers supply their variable prefixes and terminal exceeded action, while
preserving their existing retry-limit and status-handling behavior. Replace both
inline case-pattern implementations with this shared helper output.
🪄 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: 945b6ce3-5b43-4e71-9563-541a26e4a0a0
📒 Files selected for processing (9)
CLI/CMUXCLI+SSHStartupScripts.swiftCLI/cmux.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHPTYAttachRetryScriptBuilder.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHForegroundAuthenticationRetryPolicyTests.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHPTYAttachRetryScriptBuilderTests.swiftSources/SSHPTYAttachStartupCommandBuilder.swiftcmuxTests/SSHForegroundAuthenticationMarkerCleanupTests.swiftcmuxTests/SSHForegroundAuthenticationSignalTests.swift
| let childPID = try XCTUnwrap(Int32( | ||
| String(contentsOf: childPIDFile, encoding: .utf8) | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| )) | ||
| defer { | ||
| Darwin.kill(childPID, SIGKILL) | ||
| } | ||
|
|
||
| XCTAssertFalse(result.timedOut, result.stderr) | ||
| XCTAssertEqual(result.status, 130, result.stderr) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the process result before unwrapping the child PID file.
If the wrapper times out or exits early, the child PID file is never written and XCTUnwrap at Line 47 fails first, hiding the result.timedOut/result.status diagnostics (including stderr) that explain why.
🧹 Proposed reorder
- let childPID = try XCTUnwrap(Int32(
- String(contentsOf: childPIDFile, encoding: .utf8)
- .trimmingCharacters(in: .whitespacesAndNewlines)
- ))
- defer {
- Darwin.kill(childPID, SIGKILL)
- }
-
XCTAssertFalse(result.timedOut, result.stderr)
XCTAssertEqual(result.status, 130, result.stderr)
+ let childPID = try XCTUnwrap(Int32(
+ String(contentsOf: childPIDFile, encoding: .utf8)
+ .trimmingCharacters(in: .whitespacesAndNewlines)
+ ))
+ defer {
+ Darwin.kill(childPID, SIGKILL)
+ }
XCTAssertTrue(📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let childPID = try XCTUnwrap(Int32( | |
| String(contentsOf: childPIDFile, encoding: .utf8) | |
| .trimmingCharacters(in: .whitespacesAndNewlines) | |
| )) | |
| defer { | |
| Darwin.kill(childPID, SIGKILL) | |
| } | |
| XCTAssertFalse(result.timedOut, result.stderr) | |
| XCTAssertEqual(result.status, 130, result.stderr) | |
| XCTAssertFalse(result.timedOut, result.stderr) | |
| XCTAssertEqual(result.status, 130, result.stderr) | |
| let childPID = try XCTUnwrap(Int32( | |
| String(contentsOf: childPIDFile, encoding: .utf8) | |
| .trimmingCharacters(in: .whitespacesAndNewlines) | |
| )) | |
| defer { | |
| Darwin.kill(childPID, SIGKILL) | |
| } | |
| XCTAssertTrue( |
🤖 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 `@cmuxTests/SSHForegroundAuthenticationSignalTests.swift` around lines 47 - 56,
In the test flow surrounding childPIDFile and the result assertions, validate
result.timedOut and result.status before reading or unwrapping the child PID
file so timeout and early-exit diagnostics, including stderr, are reported
first. Keep the existing child-process cleanup via Darwin.kill after the PID is
successfully unwrapped.
| private func waitForFile( | ||
| at url: URL, | ||
| containing expectedContents: String, | ||
| while process: Process, | ||
| timeout: TimeInterval | ||
| ) -> Bool { | ||
| let deadline = Date().addingTimeInterval(timeout) | ||
| while process.isRunning, Date() < deadline { | ||
| let contents = (try? String(contentsOf: url, encoding: .utf8)) ?? "" | ||
| if contents.contains(expectedContents) { | ||
| return true | ||
| } | ||
| Thread.sleep(forTimeInterval: 0.01) | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Re-check file contents after the poll loop exits.
while process.isRunning makes the helper return false as soon as the child exits, even when the expected content was already written. That is a live race for the queued-input assertion at Lines 239-245: cmux_test_attach writes input:queued-input and then returns 0, so the shell exits immediately afterward and the poll can observe isRunning == false before it ever reads the final contents. The sibling helper in cmuxTests/SSHStartupSignalLifecycleTests.swift performs a final read for this reason.
🧹 Proposed fix
let deadline = Date().addingTimeInterval(timeout)
while process.isRunning, Date() < deadline {
let contents = (try? String(contentsOf: url, encoding: .utf8)) ?? ""
if contents.contains(expectedContents) {
return true
}
Thread.sleep(forTimeInterval: 0.01)
}
- return false
+ let contents = (try? String(contentsOf: url, encoding: .utf8)) ?? ""
+ return contents.contains(expectedContents)
}As per path instructions, tests must use "deadline-bounded polls of real predicates rather than fixed-duration waits before assertions."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private func waitForFile( | |
| at url: URL, | |
| containing expectedContents: String, | |
| while process: Process, | |
| timeout: TimeInterval | |
| ) -> Bool { | |
| let deadline = Date().addingTimeInterval(timeout) | |
| while process.isRunning, Date() < deadline { | |
| let contents = (try? String(contentsOf: url, encoding: .utf8)) ?? "" | |
| if contents.contains(expectedContents) { | |
| return true | |
| } | |
| Thread.sleep(forTimeInterval: 0.01) | |
| } | |
| return false | |
| } | |
| private func waitForFile( | |
| at url: URL, | |
| containing expectedContents: String, | |
| while process: Process, | |
| timeout: TimeInterval | |
| ) -> Bool { | |
| let deadline = Date().addingTimeInterval(timeout) | |
| while process.isRunning, Date() < deadline { | |
| let contents = (try? String(contentsOf: url, encoding: .utf8)) ?? "" | |
| if contents.contains(expectedContents) { | |
| return true | |
| } | |
| Thread.sleep(forTimeInterval: 0.01) | |
| } | |
| let contents = (try? String(contentsOf: url, encoding: .utf8)) ?? "" | |
| return contents.contains(expectedContents) | |
| } |
🤖 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/Tests/CmuxFoundationTests/SSHPTYAttachRetryScriptBuilderTests.swift`
around lines 258 - 273, Update the waitForFile helper to perform one final
file-content check after the process-running poll loop exits, before returning
false. Preserve the deadline-bounded polling behavior and return true when the
final read contains expectedContents, including when the process exits
immediately after writing the content.
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. |
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/SSHForegroundAuthenticationRetryPolicy.swift`:
- Around line 89-110: Replace the timer-driven poll-and-sleep and repeated PID
rescans in the foreground-auth cancellation path with supervision through a
single owner/process group and explicit child reaping. Update the surrounding
SSHForegroundAuthenticationRetryPolicy teardown flow so cancellation delegates
lifecycle management to that owner, removes the fixed grace loop and escalation
coordination, and preserves reliable cleanup without timing-based
synchronization.
In
`@Packages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHForegroundAuthenticationRetryPolicyTests.swift`:
- Around line 186-201: Bound all process completion in
SSHForegroundAuthenticationRetryPolicyTests: at lines 186-201 replace
waitUntilExit() with a deadline-bounded helper before reading the child PID,
capture or asynchronously drain stderr and report diagnostics on timeout, and
terminate lingering children during cleanup; at lines 282-283 apply the same
bounded completion to the diagnostic-state test; at lines 311-313 update run(_:)
to avoid unbounded pipe-to-EOF reads and process waits while preserving real
completion or deadline-bounded predicate polling.
🪄 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: 0981831e-6107-4a35-8bb5-fe94d0a5b938
📒 Files selected for processing (3)
Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHForegroundAuthenticationRetryPolicy.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHForegroundAuthenticationRetryPolicyTests.swiftcmuxTests/SSHForegroundAuthenticationSignalTests.swift
| cmux_ssh_auth_tree_grace_attempt=0 | ||
| while [ "$cmux_ssh_auth_tree_grace_attempt" -lt 10 ]; do | ||
| cmux_ssh_auth_tree_has_survivor=0 | ||
| for cmux_ssh_auth_tree_pid in $cmux_ssh_auth_tree_initial_pids; do | ||
| if /bin/kill -0 "$cmux_ssh_auth_tree_pid" >/dev/null 2>&1; then | ||
| cmux_ssh_auth_tree_has_survivor=1 | ||
| break | ||
| fi | ||
| done | ||
| if [ "$cmux_ssh_auth_tree_has_survivor" -eq 0 ]; then exit 0; fi | ||
| /bin/sleep 0.02 | ||
| cmux_ssh_auth_tree_grace_attempt=$((cmux_ssh_auth_tree_grace_attempt + 1)) | ||
| done | ||
|
|
||
| cmux_ssh_auth_tree_pids= | ||
| for cmux_ssh_auth_tree_pid in $cmux_ssh_auth_tree_initial_pids; do | ||
| if /bin/kill -0 "$cmux_ssh_auth_tree_pid" >/dev/null 2>&1; then | ||
| cmux_ssh_collect_auth_process_tree "$cmux_ssh_auth_tree_pid" | ||
| fi | ||
| done | ||
| for cmux_ssh_auth_tree_pid in $cmux_ssh_auth_tree_pids; do | ||
| /bin/kill -KILL "$cmux_ssh_auth_tree_pid" >/dev/null 2>&1 || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Remove timer-driven cleanup escalation from the signal path.
This adds a fixed 200 ms poll-and-sleep wait to every foreground-auth cancellation before escalation. Model the authentication subprocess under one supervised owner/process group with explicit reaping instead of using timed rescans to coordinate teardown.
As per coding guidelines, do not introduce timing, blocking, or polling repair paths to paper over lifecycle or shared-state races.
🤖 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/SSHForegroundAuthenticationRetryPolicy.swift`
around lines 89 - 110, Replace the timer-driven poll-and-sleep and repeated PID
rescans in the foreground-auth cancellation path with supervision through a
single owner/process group and explicit child reaping. Update the surrounding
SSHForegroundAuthenticationRetryPolicy teardown flow so cancellation delegates
lifecycle management to that owner, removes the fixed grace loop and escalation
coordination, and preserves reliable cleanup without timing-based
synchronization.
Source: Coding guidelines
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. |
…-9067-ssh-boot-retry # Conflicts: # CLI/SSHPTYAttachExitCode.swift # CLI/cmux.swift # Sources/SSHPTYAttachStartupCommandBuilder.swift # cmuxTests/SSHPersistentPTYRetryLifecycleTests.swift
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. |
1 participant
Summary
Regression coverage
Network is unreachablewith exit 255, then succeedsValidation
git diff --checkPackage.resolvedpolicy, workspace package grouping, and pbxproj normalization/checkissue-9067-ssh-boot-retryon commit68a5a52632ENOSPC; neither produced a source compiler errorCloses #9067
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by cubic
Retries initial SSH foreground authentication inside the reconnect loop and fails closed on unknown errors, so boot-time network outages and server‑alive timeouts retry without hiding prompts. Unifies retry/backoff and signal handling across CLI and restored sessions using
CmuxFoundationbuilders (SSHForegroundAuthenticationRetryPolicy,SSHRetryBackoffScriptBuilder,SSHPTYAttachRetryScriptBuilder) with Sonoma-compatible PTY capture and anchored, bounded cleanup; closes #9067.Bug Fixes
Tests
Written for commit a3573c8. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests