Fix SSH relay deadlock after app restart - #9105
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:
📝 WalkthroughWalkthroughReverse-relay SSH transport now resolves effective ControlMaster options, tracks cross-process socket ownership, recovers inherited forwards, and falls back to a dedicated SSH process. Foreground authentication, cleanup retries, lifecycle handling, localized statuses, and associated tests were updated. ChangesSSH relay ownership and recovery
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 2 warnings)
✅ 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 |
austinywang
marked this pull request as ready for review
July 29, 2026 00:25
![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 `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 1975-1985: Strengthen this test by adding a positive assertion
that reverse-relay startup was reached before asserting controlOperations is
empty. Use a relay-specific signal captured by the scripted runner, such as the
relay metadata installation command or relay-scoped SSH invocation, and retain
the existing negative ControlMaster assertion.
🪄 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: 668c81e2-5d3c-4021-adf8-99323026d8c6
📒 Files selected for processing (7)
Packages/macOS/CmuxCore/Sources/CmuxCore/Remote/WorkspaceRemoteConfiguration+SSHBatchCommands.swiftPackages/macOS/CmuxCore/Tests/CmuxCoreTests/WorkspaceRemoteConfigurationSSHBatchCommandsTests.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+RelayProvisioning.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelay.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionSSHRemoteCommandOverrideTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
💤 Files with no reviewable changes (4)
- Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swift
- Packages/macOS/CmuxCore/Tests/CmuxCoreTests/WorkspaceRemoteConfigurationSSHBatchCommandsTests.swift
- Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+RelayProvisioning.swift
- Packages/macOS/CmuxCore/Sources/CmuxCore/Remote/WorkspaceRemoteConfiguration+SSHBatchCommands.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. |
…controlmaster-lease
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 `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 1952-1959: Update the relay startup test around the relay
invocation and relayStatusObservation to capture SSH arguments specifically for
the dedicated reverse-relay launch, then use that invocation as the startup
expectation. Assert the captured arguments include “-S none” and exclude the
configured shared ControlPath, while retaining the existing controlOperations
assertion only as supplemental coverage; do not use the generic “Remote SSH
relay unavailable” status string as the startup signal.
🪄 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: d3d42549-853a-40c8-8042-593c311c892b
📒 Files selected for processing (1)
cmuxTests/WorkspaceRemoteConnectionTests.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: 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/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator`+ReverseRelay.swift:
- Around line 234-261: Refactor cancelInheritedReverseRelayForwardLocked so it
does not synchronously call sshExec on the locked coordinator path; move
relay-cancellation SSH work into a coordinator-owned cancellable asynchronous
phase, preserving the existing success and ignored-failure logging. Ensure any
dedicated-process launch is enqueued only after cancellation completes, and
allow termination, stop, and later relay state transitions to proceed without
waiting on the SSH timeout.
🪄 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: eaf226b0-c883-4828-9003-98c7bb3479e2
📒 Files selected for processing (3)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelay.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
💤 Files with no reviewable changes (1)
- Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.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/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteProcessCancellationOperation.swift`:
- Around line 6-8: Add a one-line explanation to the documentation above
RemoteProcessCancellationOperation stating that installCancellationHandler and
cancel are invoked synchronously from non-isolated contexts that cannot await,
so actor-backed synchronization is not viable.
🪄 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: 1fa0c6a6-0fa7-43de-8ff6-a03974a29f7e
📒 Files selected for processing (6)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteProcessCancellationOperation.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelay.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelayStartup.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Values/ReverseRelayStartupPhase.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionReverseRelayStartupTests.swift
Contributor
Author
|
@coderabbitai resume |
@austinywang I can't start this review because your workspace has reached its free monthly review limit. cubic has reviewed 241,631 of the 240,000 allowed lines of code this month. Reviews resume on 1 August 2026 (in 3 days). Paid plans include much higher monthly review limits. Upgrade now to resume reviews. To help optimise your usage, you can tune cubic to get the most out of your usage limits:
|
|
To use Codex here, create a Codex account and connect to github. |
|
✅ Action performedReviews resumed. Review finished.
|
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator`+ReverseRelayStartup.swift:
- Around line 131-141: Update the reverse relay startup status construction in
the coordinator to avoid placing raw startupFailure text in user-facing detail;
use a concise sanitized relay-unavailable summary while retaining the raw
failure only in debug logging. Replace the hardcoded English status text with
String(localized:defaultValue:) using a stable localization key, preserving the
retry timing information.
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Values/ReverseRelayStartupPhase.swift`:
- Around line 6-10: Update the cancellation path in
RemoteSessionCoordinator+ReverseRelayStartup.swift so a cancelled exit task
still invokes finishConflictedControlMasterExitLocked through the queue-confined
state transition, rather than returning while the phase is
.exitingConflictedControlMaster. Preserve the normal completion behavior and add
coverage for cancellation followed by a new relay start, verifying recovery and
launch are no longer blocked.
In
`@Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionReverseRelayStartupTests.swift`:
- Around line 136-157: Update the coordinator setup in both reverse-relay
startup tests to generate a unique temporary manifest home and socket path for
each test invocation, rather than using FileManager.default.temporaryDirectory
and the fixed cancellation socket. Pass these per-invocation paths through each
coordinator’s configuration while preserving the existing test behavior and
cleanup.
🪄 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: 8fe0fed9-8296-4abf-873c-e8c7221878c2
📒 Files selected for processing (7)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteProcessCancellationOperation.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelay.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelayStartup.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Values/ReverseRelayStartupPhase.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionReverseRelayStartupTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/WorkspaceRemoteConnectionTests.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/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionReverseRelayStartupTests.swift`:
- Around line 30-97: Add a test covering the successful recovery path in
beginConflictedControlMasterExitIfNeededLocked: configure RecordingProcessRunner
to return status 0 for the -O exit request and succeed for the subsequent -N -R
relay launch, then assert finishConflictedControlMasterExitLocked triggers
launchReverseRelayLocked and retries the relay bind. Preserve existing
assertions for the ignored and cancellation paths.
🪄 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: 8fe0fed9-8296-4abf-873c-e8c7221878c2
📒 Files selected for processing (7)
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteProcessCancellationOperation.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelay.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelayStartup.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Values/ReverseRelayStartupPhase.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionReverseRelayStartupTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/WorkspaceRemoteConnectionTests.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. |
…controlmaster-lease
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionReverseRelayStartupTests.swift (1)
145-177: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRace: handlers are stored after the stream yield, not before.
launchContinuation.yield(...)fires (potentially waking an awaitingTaskon another thread) beforeself.startupHandler/self.terminationHandlerare assigned under the lock a few lines later. A test that doeslet launch = try#require(await launches.next())and then immediately callsemitStartupReady()/emitTermination()can race the producer and read a still-nil handler, silently no-op'ing the callback. This affects downstream consumers such asunresolvedOwnedControlPathUsesStandaloneFallbackandstandaloneConflictFailsClosedinRemoteSessionReverseRelayTransportTests.swift, which callemitStartupReady()/emitTermination()right after consuming the stream element.Store the handlers under the lock before yielding to the stream so any awakened consumer is guaranteed to see them set.
🐛 Proposed fix
let localRelayPort = try `#require`( Int(reverseArgument.split(separator: ":").last ?? "") ) - launchContinuation.yield(RecordedReverseRelayLaunch( - arguments: arguments, - localRelayPort: localRelayPort, - startupMarker: startupMarker - )) lock.withLock { _launchCount += 1 self.startupHandler = startupHandler self.terminationHandler = terminationHandler } + launchContinuation.yield(RecordedReverseRelayLaunch( + arguments: arguments, + localRelayPort: localRelayPort, + startupMarker: startupMarker + )) return process🤖 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/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionReverseRelayStartupTests.swift` around lines 145 - 177, Move the lock-protected assignments to startupHandler, terminationHandler, and _launchCount in launch before launchContinuation.yield(RecordedReverseRelayLaunch(...)). Preserve the existing launch record contents and return behavior, ensuring consumers awakened by the yield can immediately observe both handlers.Packages/macOS/CmuxCore/Sources/CmuxCore/Remote/WorkspaceRemoteConfiguration+SSHBatchCommands.swift (1)
71-75: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDuplicate "first SSH option value" parser — delegate to
SSHAgentSocketResolver.
SSHAgentSocketResolver.optionValue(named:in:)already implements this exact first-value-wins parsing (andSSHConnectionSharingOptions.swiftremoved its own equivalent helper in this same PR to consolidate on it). This file still keeps a private duplicate (firstSSHOptionValue), used byreverseRelayControlMasterArguments. SinceSSHAgentSocketResolveris already imported here (used inbackgroundSSHOptions), delegating avoids two independently-maintained copies of security/behavior-sensitive SSH option parsing drifting apart.♻️ Proposed refactor
- private static func firstSSHOptionValue( - named key: String, - in options: [String] - ) -> String? { - let loweredKey = key.lowercased() - for option in trimmedSSHOptions(options) { - let parts = option.split( - maxSplits: 1, - omittingEmptySubsequences: true, - whereSeparator: { $0 == "=" || $0.isWhitespace } - ) - guard parts.count == 2, parts[0].lowercased() == loweredKey else { - continue - } - let value = parts[1].trimmingCharacters(in: .whitespacesAndNewlines) - if !value.isEmpty { - return value - } - } - return nil - } + private static func firstSSHOptionValue( + named key: String, + in options: [String] + ) -> String? { + SSHAgentSocketResolver().optionValue(named: key, in: options) + }Also applies to: 153-174
🤖 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/CmuxCore/Sources/CmuxCore/Remote/WorkspaceRemoteConfiguration`+SSHBatchCommands.swift around lines 71 - 75, Remove the private firstSSHOptionValue parser and update reverseRelayControlMasterArguments to use SSHAgentSocketResolver.optionValue(named:in:) for ControlMaster and related SSH option lookups. Preserve first-value-wins behavior and the existing handling of disabled ControlMaster values.Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+Workspace.swift (1)
795-813: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse empty-to-nil trimming for
control_path.
control_pathis a new optional SSH controlpath, not legacyv2RawStringdata. Parsing""or whitespace as a non-nilresolvedControlPathcurrently forwards it intobeginControlMasterAdoption(...)instead of being treated as "not provided"; useoptionalTrimmedRawString(params, "control_path")like the nearbysession_idparsing path.🤖 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/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator`+Workspace.swift around lines 795 - 813, Update workspaceRemoteForegroundAuthReady to parse control_path with optionalTrimmedRawString instead of rawString plus trimming, while preserving the existing foreground_auth_token parsing. Ensure empty or whitespace-only control_path values become nil before passing resolvedControlPath to controlWorkspaceRemoteForegroundAuthReady.Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHConnectionBroker.swift (2)
41-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the three initializers onto the designated one.
Both public inits now repeat the same
SSHConnectionSharingOptions()/ jitter / registry construction; the third init already takes every dependency. Delegating removes the drift risk when another dependency is added.♻️ Proposed refactor
public nonisolated init(clock: any RemoteProxyRetryClock = SystemRemoteProxyRetryClock()) { - self.sharingOptions = SSHConnectionSharingOptions() - self.clock = clock - self.jitterMilliseconds = { Int.random(in: 100...350) } - self.cleanupLauncherOverride = nil - let ownershipRegistry = - NativeSSHControlMasterOwnershipRegistry( - sharingOptions: sharingOptions - ) - self.controlMasterOwnershipRegistry = ownershipRegistry + self.init(clock: clock, cleanupLauncher: nil) }...and give the
cleanupLauncher-taking init an optional parameter that forwards to the designated initializer with a freshly builtSSHConnectionSharingOptions()and registry.🤖 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/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHConnectionBroker.swift` around lines 41 - 90, Collapse the three initializers in NativeSSHConnectionBroker onto the dependency-injecting initializer. Make the cleanupLauncher parameter optional there, and have both public initializers delegate to it with freshly constructed SSHConnectionSharingOptions, jitter behavior, and NativeSSHControlMasterOwnershipRegistry while preserving their existing defaults and cleanup-launcher behavior.
121-137: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHonor the cross-process ownership gate before reinstating the lease.
retainsWorkspace(_:)drops thecontrolMasterOwnershipRegistry.retain(...)result, then installs the lease and owner mapping beforecancelCleanup(for:). SincebeginCleanup(controlPath:)only authorizes recovery for an unowned socket, a live sibling process can prevent cleanup while the broker still records ownership of that socket. Fail closed when retained ownership is not acquired, or expose the registry result through adoption/cleanup handling if better-for-now stale cleanup is intentional.🤖 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/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHConnectionBroker.swift` around lines 121 - 137, The retainsWorkspace(_:) flow must honor the result of controlMasterOwnershipRegistry.retain before recording ownership. Capture and validate that result, and fail closed without reinstating ownerLeases or ownersByControlMaster when retention is denied; ensure cleanup cancellation and lease restoration occur only after successful cross-process ownership acquisition.Source: Path instructions
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelay.swift (1)
207-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRoute binding conflicts through the port-conflict status string.
RemoteSessionCoordinator+ReverseRelay.swift:97andhandleReverseRelayTerminationLocked:204both callpublishReverseRelayFailureLocked, but that always publishesstrings.reverseRelayUnavailableRetrying. Since.bindingConflictis produced for relay-port conflicts, pass/usestrings.reverseRelayPortUnavailableRetryingfor that path so the added string is not dead/localization-only copy unless port conflicts are intentionally hidden behind the generic detail.🤖 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/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator`+ReverseRelay.swift around lines 207 - 216, Update publishReverseRelayFailureLocked to accept or select the status detail based on the failure reason, using strings.reverseRelayPortUnavailableRetrying for .bindingConflict cases and strings.reverseRelayUnavailableRetrying for other failures. Ensure the reverse-relay failure callers, including handleReverseRelayTerminationLocked, pass the conflict-specific path when appropriate while preserving the existing retry scheduling.
🤖 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/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHConnectionBroker`+Cleanup.swift:
- Around line 93-100: Preserve a single authentication-lock exit-status
contract: in NativeSSHConnectionBroker.launchCleanup, pass
request.authenticationLockPath into NativeSSHControlMasterCleanupRequest so the
zsh wrapper and its retry/no-op statuses remain observable, and retain the
related stale-pid cleanup; then remove any unused
resetSkippedExitStatus/noOpExitStatus API from
NativeSSHControlMasterCleanupRequest, or add the caller logic that consumes it.
Update both affected files accordingly:
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHConnectionBroker+Cleanup.swift
lines 93-100 and
Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterCleanupRequest.swift
lines 33-49.
- Around line 7-10: Document the intentional timing-based coordination around
the cleanup delay constants and the cleanup retry/termination logic near the
referenced cleanup flow. State that the cross-process lock holder provides no
completion signal, so retries use capped delays and stop after
cleanupMaximumRetryCount; make clear this is intentional staggering rather than
polling.
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHConnectionBroker`+ControlMasterOwnership.swift:
- Around line 17-30: Validate controlPath in beginControlMasterAdoption before
creating the lease or calling controlMasterOwnershipRegistry.retain, using the
same template and cmux-owned resolved-path checks as
beginReverseForwardRecovery. Return nil for any missing, template-bearing, or
non-cmux-owned path, and only proceed with registry retention for an exact
validated path.
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterAdoptionHandoff.swift`:
- Around line 19-39: Update the default expiration behavior in
NativeSSHControlMasterAdoptionHandoff to match the authentication lock’s
staleness budget, or re-arm expirationTask while the authentication lock remains
held; ensure legitimate interactive authentication cannot lose the handoff
before durable-lease installation completes.
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterCleanupRequest.swift`:
- Around line 33-49: Remove the unused resetSkippedExitStatus constant and the
noOpExitStatus parameterized processInvocation overload, retaining only the
active processInvocation path and its current behavior. Verify no remaining
callers or references depend on these exit codes.
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterOwnershipRegistry.swift`:
- Around line 196-206: Update the exclusive-ownership path in
NativeSSHControlMasterOwnershipRegistry so it refuses conversion when
entry.leases is non-empty, preventing active workspace leases from being
stranded. Preserve the existing shared-lock reacquisition behavior for entries
without leases, and do not call removeEntryLocked(controlPath) while live leases
remain; if lease reporting is already supported, propagate any lost leases to
NativeSSHConnectionBroker.ownerLeases instead.
In
`@Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/FoundationRemoteReverseRelayProcessTests.swift`:
- Around line 72-82: Remove the wall-clock timing assertions from the three
tests terminationBoundsInheritedStderr, missingForwardConfirmationTerminates,
and startupDeadlineForceKillsAfterGracePeriod, including their startedAt Date()
setup and elapsed-time checks. Keep the existing asynchronous completion and
termination assertions unchanged.
In
`@Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/NativeSSHControlMasterAdoptionHandoffTests.swift`:
- Around line 24-30: The release assertion in the handoff test relies on
Task.yield() rather than guaranteed synchronization. Update the
releaseHandler/recorder flow to emit an explicit completion signal, such as an
AsyncStream continuation, and await that signal after handoff.release() before
asserting recorder.count == 1; remove the heuristic Task.yield().
In
`@Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/NativeSSHControlMasterOwnershipRegistryTests.swift`:
- Around line 252-254: Update resolvedControlPath(userID:) to place the
generated control path inside each test’s UUID-scoped temporary directory, using
the existing per-test scratch-directory symbol and preserving the deterministic
suffix. Ensure ownership locks derived through
sharingOptions.resolvedControlMasterOwnershipLockPath(controlPath:) no longer
resolve under shared /tmp across parallel tests.
In `@Sources/TerminalController`+ControlWorkspaceContext.swift:
- Around line 437-459: Update controlWorkspaceRemoteForegroundAuthReady to
detect a nil or blank foregroundAuthToken before calling
notifyRemoteForegroundAuthenticationReady and return the existing
invalid_params-style result for missing input. Only invoke
notifyRemoteForegroundAuthenticationReady with a non-empty normalized token,
preserving the unavailable ownership result for genuine adoption refusal.
- Around line 671-685: Update the shared RemoteSessionStrings
controlMasterOwnershipUnavailable accessor to cover this localized message, then
replace the inline String(localized:) construction in both error paths of the
workspace control flow with that accessor, preserving the existing error code,
data, and return behavior.
In `@Sources/Workspace.swift`:
- Around line 5530-5534: Remove the discarded-result behavior from
configureRemoteConnection and update reconnectRemoteConnection to consume its
Bool result, returning false when configuration or ControlMaster adoption fails
so callers receive the failure. Alternatively, ensure the failure branch in
configureRemoteConnection updates remoteConnectionState to .error before
returning false; preserve successful reconnection behavior.
In `@Sources/Workspace`+RemoteSessionLifecycle.swift:
- Around line 210-223: Normalize resolvedControlPath so an empty or
whitespace-only value becomes nil before the adoption branch in the readiness
notification flow. Update the logic surrounding
beginControlMasterAdoption(controlPath:ownerWorkspaceID:) to skip adoption and
preserve the existing nil-path behavior for malformed control_path input, while
continuing to adopt valid paths.
---
Outside diff comments:
In
`@Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator`+Workspace.swift:
- Around line 795-813: Update workspaceRemoteForegroundAuthReady to parse
control_path with optionalTrimmedRawString instead of rawString plus trimming,
while preserving the existing foreground_auth_token parsing. Ensure empty or
whitespace-only control_path values become nil before passing
resolvedControlPath to controlWorkspaceRemoteForegroundAuthReady.
In
`@Packages/macOS/CmuxCore/Sources/CmuxCore/Remote/WorkspaceRemoteConfiguration`+SSHBatchCommands.swift:
- Around line 71-75: Remove the private firstSSHOptionValue parser and update
reverseRelayControlMasterArguments to use
SSHAgentSocketResolver.optionValue(named:in:) for ControlMaster and related SSH
option lookups. Preserve first-value-wins behavior and the existing handling of
disabled ControlMaster values.
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHConnectionBroker.swift`:
- Around line 41-90: Collapse the three initializers in
NativeSSHConnectionBroker onto the dependency-injecting initializer. Make the
cleanupLauncher parameter optional there, and have both public initializers
delegate to it with freshly constructed SSHConnectionSharingOptions, jitter
behavior, and NativeSSHControlMasterOwnershipRegistry while preserving their
existing defaults and cleanup-launcher behavior.
- Around line 121-137: The retainsWorkspace(_:) flow must honor the result of
controlMasterOwnershipRegistry.retain before recording ownership. Capture and
validate that result, and fail closed without reinstating ownerLeases or
ownersByControlMaster when retention is denied; ensure cleanup cancellation and
lease restoration occur only after successful cross-process ownership
acquisition.
In
`@Packages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator`+ReverseRelay.swift:
- Around line 207-216: Update publishReverseRelayFailureLocked to accept or
select the status detail based on the failure reason, using
strings.reverseRelayPortUnavailableRetrying for .bindingConflict cases and
strings.reverseRelayUnavailableRetrying for other failures. Ensure the
reverse-relay failure callers, including handleReverseRelayTerminationLocked,
pass the conflict-specific path when appropriate while preserving the existing
retry scheduling.
In
`@Packages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionReverseRelayStartupTests.swift`:
- Around line 145-177: Move the lock-protected assignments to startupHandler,
terminationHandler, and _launchCount in launch before
launchContinuation.yield(RecordedReverseRelayLaunch(...)). Preserve the existing
launch record contents and return behavior, ensuring consumers awakened by the
yield can immediately observe both handlers.
🪄 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: 66b97f62-8421-4c02-8973-0b866c0a88bc
📒 Files selected for processing (57)
Packages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlCommandCoordinator+Workspace.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceContext.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/Workspace/ControlWorkspaceRemoteResolution.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandContextTestStubs.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/ControlCommandCoordinatorWorkspaceTests.swiftPackages/macOS/CmuxControlSocket/Tests/CmuxControlSocketTests/FakeWorkspaceControlCommandContext.swiftPackages/macOS/CmuxCore/Sources/CmuxCore/Remote/WorkspaceRemoteConfiguration+SSHBatchCommands.swiftPackages/macOS/CmuxCore/Sources/CmuxCore/Remote/WorkspaceRemoteConfiguration+SSHControlPath.swiftPackages/macOS/CmuxCore/Tests/CmuxCoreTests/WorkspaceRemoteConfigurationSSHBatchCommandsTests.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHAgentSocketResolver.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/SSHConnectionSharingOptions.swiftPackages/macOS/CmuxFoundation/Tests/CmuxFoundationTests/SSHConnectionSharingOptionsTests.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHConnectionBroker+Cleanup.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHConnectionBroker+ControlMasterOwnership.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHConnectionBroker.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterAdoptionHandoff.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterCleanupRequest.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterExclusiveUseAuthorization.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterLeaseIdentity.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterOwnershipRegistry.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterOwnershipTracking.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlMasterPendingCleanup.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Connection/NativeSSHControlPathResolver.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Hosting/RemoteSessionStrings.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/FoundationRemoteReverseRelayProcess.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteReverseRelayLauncher.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Process/RemoteReverseRelayLaunching.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+RelayProvisioning.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelay.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator+ReverseRelayControlMaster.swiftPackages/macOS/CmuxRemoteSession/Sources/CmuxRemoteSession/Session/RemoteSessionCoordinator.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/CleanupBlockingNativeSSHControlMasterOwnershipRegistry.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/FoundationRemoteReverseRelayProcessTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/NativeSSHConnectionBrokerTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/NativeSSHControlMasterAdoptionHandoffTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/NativeSSHControlMasterOwnershipRecoveryTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/NativeSSHControlMasterOwnershipRegistryTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/PermissiveNativeSSHControlMasterOwnershipRegistry.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteDaemonUploadTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemotePTYIntentionalCleanupTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemotePortScanGatingTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteReconnectPolicyTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteRelaySlotTeardownTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionInheritedForwardRecoveryTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionReverseRelayStartupTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionReverseRelayTransportTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/RemoteSessionSSHRemoteCommandOverrideTests.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/ResolvedControlPathProcessRunner.swiftPackages/macOS/CmuxRemoteSession/Tests/CmuxRemoteSessionTests/SynchronousEventRecorder.swiftResources/Localizable.xcstringsSources/RemoteSessionStrings+App.swiftSources/SSHPTYAttachStartupCommandBuilder.swiftSources/TerminalController+ControlWorkspaceContext.swiftSources/Workspace+RemoteSessionLifecycle.swiftSources/Workspace.swiftcmuxTests/SSHForegroundAuthenticationMarkerCleanupTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.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. |
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. |
kernelalex
added a commit
to Enigma-Labs-Technology/zerocmux
that referenced
this pull request
Aug 2, 2026
The unit-test target did not compile on the merged tree. Three distinct causes, fixed here; `xcodebuild -scheme zerocmux-unit build-for-testing` now succeeds locally. 1. Collateral from the scrub's block parser. Its member detection kept scanning past one-line declarations (`private let x = ...`, no braces) and swallowed the following member, so removing mobile tests also deleted unrelated neighbours. Restored NotificationFeedHistoryTests from the merge commit and re-removed ONLY its three mobile-host RPC tests plus responsePayload (12 persistence tests are back, along with the waitUntil/notification/write helpers); restored recoveryURL in BrowserWebContentProcessTests. Removals now use a parser anchored on the func keyword that cannot cross member boundaries. 2. Merge truncation and dropped adjacent lines. SSHStartupSignalLifecycleTests lost its file tail, so its own call sites had no generatedVMSSHInitialStartupCommand / waitForSSHSignalLifecycleLog definitions; restored from upstream. WorkspaceUnitTests lost three `let fallbackCwd` declarations that upstream had added next to lines whose conflict resolved to ours. 3. Tests outliving their subject. Removed the four SidebarWidthPolicy cases exercising AppWebThemeSnapshot (it lives in the excluded ProWelcomeChecklist) and the seven WorkspaceRemoteConnection cases for remoteStaleRelayListenerCleanupScript / remoteReverseRelayControlOperation — verified those existed at the previous sync point and that upstream itself deleted the API and its tests in 'Fix SSH relay deadlock after app restart (manaflow-ai#9105)', so this adopts upstream's deletion rather than dropping fork behaviour. Also: guarded eight sidebar test files' bare '@testable import cmux_DEV' with the fork's canImport pattern (the module is zerocmux_DEV); rewrote RemoteTmuxPaneSeedTransport's grid-wait helpers onto libghostty's ghostty_surface_grid_metrics instead of the removed mobile render-grid exporter, keeping the four tests that use them; dropped CodexCodeModeRolloutIdentityTests (subject AgentChatSessionRegistry is excluded) and swept its pbxproj entries. cmux-tui: write the graphics_writer PTY sentinel before TCOOFF (CI showed write() returning -1/EAGAIN against a stopped pty) and widen the cancellable-connect deadline so two 10ms cancellation polls fit on a loaded runner.
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.
Summary
ControlPath, the retained workspace generation, exclusive cross-process ownership, and matching remote relay metadata before sendingssh -O exitCloses #8894.
Issue: #8894
Reproduction
From #8894:
Architecture
The reverse
-Rlistener belongs to the SSH connection, so a ControlPersist master inherited from the previous app instance also inherits the stale listener. Reusing that master keeps the listener alive and guarantees that retrying the pinned port cannot succeed.OpenSSH cannot reliably cancel this inherited forward from the listen address alone.
ssh -O cancel -Rmust identify the original full forward, including the old app's ephemeral local relay target port, and that target is intentionally not persisted after a crash. A listen-only cancel therefore cannot be the recovery primitive. The authorized operation isssh -O exitagainst the exact cmux-owned socket.The broker now owns the disruptive recovery transaction:
ssh -O exit;The remote persistent daemon metadata is deliberately preserved. This lets the replacement SSH master bind the same lease port and lets the existing remote PTY survive the local transport restart. Blocking SSH probe/exit work runs outside the main actor through an
@concurrentboundary, and stopping a workspace detaches promptly while an already-authorized shared reap completes.Verification
Final pushed head:
3b6d7c268c235120e1a5c12a4213f884f4c43df0.swift test --package-path Packages/macOS/CmuxRemoteSession— 145 tests passed in 25 suites-O exit, custom ControlPaths, and live foreign owners fail closed./scripts/lint-pbxproj-test-wiring.sh— passed for 626 test filespython3 scripts/check-package-resolved-policy.py— passedpython3 scripts/check-workspace-package-groups.py— passedgit diff --check— passedscripts/swift_file_length_budget.pyis absent on this branch)Tagged dev-build restart dogfood
Built and launched with:
Against an isolated native-SSH workspace and real local sshd:
C2EFB5A3-F621-4053-B023-611C021E95FCconnected through master PID 76234, with sshd listening on pinned port 53150 and returning a relay-auth challenge.CMUX_8894_PERSIST_MARKER=alivein persistent shell PID 45603.SIGKILLonly to the tagged app.CMUX_8894_PERSIST_MARKER=alive.No local
xcodebuild testor XCUITest was run, per repository policy.Review
origin/main, but the Codex engine was blocked by the account's hard usage limit on all three required retries. This is recorded as review-infrastructure unavailability, not as a clean structured-review result; no finding was suppressed.