Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion CLI/cmux.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10194,7 +10194,11 @@ struct CMUXCLI {
responseTimeout: waitForReady ? 185 : nil
)
} catch {
throw CLIError(message: "ssh-pty-attach: \(userFacingRemotePTYErrorMessage(error))")
let userFacingMessage = userFacingRemotePTYErrorMessage(error)
throw CLIError(
message: "ssh-pty-attach: \(userFacingMessage)",
exitCode: remotePTYErrorIndicatesMissingSession(error) ? 253 : 1
)
Comment thread
cursor[bot] marked this conversation as resolved.
}
var connectedFD: Int32?
let controlSocketLock = NSLock()
Expand Down Expand Up @@ -10435,6 +10439,21 @@ struct CMUXCLI {
return userFacingRemotePTYErrorMessage(debugString(value) ?? "unknown error")
}

private func remotePTYErrorIndicatesMissingSession(_ value: Any?) -> Bool {
if let error = value as? Error {
return remotePTYErrorIndicatesMissingSession(String(describing: error))
}
return remotePTYErrorIndicatesMissingSession(debugString(value) ?? "")
}

private func remotePTYErrorIndicatesMissingSession(_ message: String) -> Bool {
let lowered = message.trimmingCharacters(in: .whitespacesAndNewlines).lowercased()
guard !lowered.isEmpty else { return false }
return lowered.contains("pty_session_not_found") ||
(lowered.contains("persistent ssh pty session") && lowered.contains("not running")) ||
(lowered.contains("persistent pty session") && lowered.contains("not running"))
}
Comment on lines +10449 to +10455

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Duplicated string patterns between detection and messaging functions

remotePTYErrorIndicatesMissingSession repeats the exact same three contains clauses already present in userFacingRemotePTYErrorMessage (lines 10467–10470). If a new server error pattern is added to userFacingRemotePTYErrorMessage to emit the "no longer running" message but the same pattern is not mirrored here, exit code 253 won't fire, the fallback shell won't trigger, and the regression is silent. Consider refactoring so remotePTYErrorIndicatesMissingSession delegates to (or shares a constant with) userFacingRemotePTYErrorMessage to keep the two in sync.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!


private func userFacingRemotePTYErrorMessage(_ message: String) -> String {
let trimmed = message.trimmingCharacters(in: .whitespacesAndNewlines)
guard !trimmed.isEmpty else { return "remote PTY operation failed" }
Expand Down
39 changes: 33 additions & 6 deletions Sources/Workspace.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7062,10 +7062,22 @@ final class WorkspaceRemoteSessionController {
process: process,
stderrPipe: stderrPipe
) {
let retryDelay = 2.0
let retryDelay: TimeInterval
let didCleanStaleListener: Bool
if Self.reverseRelayStartupFailureIndicatesStaleRemoteListener(
startupFailure,
relayPort: relayPort
) {
didCleanStaleListener = cleanupStaleRemoteRelayListenerLocked(relayPort: relayPort)
retryDelay = didCleanStaleListener ? 0.25 : 2.0
} else {
didCleanStaleListener = false
retryDelay = 2.0
}
let retrySeconds = max(1, Int(retryDelay.rounded()))
debugLog(
"remote.relay.startFailed relayPort=\(relayPort) " +
"cleanedStaleListener=\(didCleanStaleListener ? 1 : 0) " +
"error=\(startupFailure)"
)
if let relayServer {
Expand All @@ -7074,10 +7086,12 @@ final class WorkspaceRemoteSessionController {
cliRelayServer = nil
}
}
publishDaemonStatus(
.error,
detail: "Remote SSH relay unavailable: \(startupFailure) (retry in \(retrySeconds)s)"
)
if !didCleanStaleListener {
publishDaemonStatus(
.error,
detail: "Remote SSH relay unavailable: \(startupFailure) (retry in \(retrySeconds)s)"
)
}
scheduleReverseRelayRestartLocked(remotePath: remotePath, delay: retryDelay)
Comment on lines +7089 to 7095

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Silent error suppression when cleanup finds nothing to kill

cleanupStaleRemoteRelayListenerLocked returns true for two distinct cases: (1) it killed a stale process, and (2) the SSH exec succeeded but found no process to kill (noop — empty stdout, logged as remoteListener.cleanupNoop). In the noop case didCleanStaleListener is still true, so publishDaemonStatus(.error, ...) is skipped and the 0.25 s retry fires. On the next attempt the same "remote port forwarding failed for listen port X" error triggers reverseRelayStartupFailureIndicatesStaleRemoteListener again → cleanup runs again → noop again → error suppressed again. There is no retry limit in scheduleReverseRelayRestartLocked, so a port that stays occupied by something the cleanup script can’t find produces an infinite silent retry loop: the relay never connects but the user sees no error and has no way to diagnose the failure.

The fix is to distinguish the noop case — only suppress status publication (and use the fast 0.25 s delay) when the cleanup actually terminated a process (non-empty stdout / a richer boolean signal from the helper), and fall back to publishing the status normally for the noop case.

return
}
Expand Down Expand Up @@ -9253,6 +9267,16 @@ final class WorkspaceRemoteSessionController {
return bestErrorLine(stderr: stderr) ?? "status=\(process.terminationStatus)"
}

static func reverseRelayStartupFailureIndicatesStaleRemoteListener(
_ detail: String,
relayPort: Int
) -> Bool {
guard relayPort > 0 else { return false }
let lowered = detail.lowercased()
return lowered.contains("remote port forwarding failed")
&& lowered.contains("listen port \(relayPort)")
}

private static func meaningfulErrorLine(in text: String) -> String? {
let lines = text
.split(separator: "\n")
Expand Down Expand Up @@ -13941,7 +13965,10 @@ final class Workspace: Identifiable, ObservableObject {
)
return SSHPTYAttachStartupCommandBuilder.command(
sessionID: sessionID,
foregroundAuth: foregroundAuth
foregroundAuth: foregroundAuth,
remoteCommand: remoteConfiguration.relayPort.map(
SSHPTYAttachStartupCommandBuilder.restoredRemoteShellCommand(relayPort:)
)
)
}

Expand Down
24 changes: 21 additions & 3 deletions Sources/WorkspaceRemoteConfiguration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,10 @@ nonisolated enum SSHPTYAttachStartupCommandBuilder {
" --command-b64 \(shellQuote(Data($0.utf8).base64EncodedString()))"
} ?? ""
let attachCommand = "\"$cmux_ssh_attach_cli\" --socket \"$CMUX_SOCKET_PATH\" ssh-pty-attach --wait\(requireExistingFlag) --workspace \"$CMUX_WORKSPACE_ID\" --session-id \"$cmux_ssh_attach_session_id\" --attachment-id \"${CMUX_SURFACE_ID:-}\"\(commandB64Flag)"
lines += retryingAttachLines(command: attachCommand)
let fallbackCommand = requireExisting && !commandB64Flag.isEmpty
? "\"$cmux_ssh_attach_cli\" --socket \"$CMUX_SOCKET_PATH\" ssh-pty-attach --wait --workspace \"$CMUX_WORKSPACE_ID\" --session-id \"$cmux_ssh_attach_session_id\" --attachment-id \"${CMUX_SURFACE_ID:-}\"\(commandB64Flag)"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep recovered PTY surfaces tracked after fallback

When a restored attach exits 253, the first ssh-pty-attach process still runs its failed-attach cleanup and sends workspace.remote.pty_attach_end; that path calls Workspace.markRemotePTYAttachEnded, which removes the surface from activeRemoteTerminalSurfaceIds and clears remotePTYSessionIDsByPanelId. This fallback then starts a new PTY on the same surface, but there is no corresponding path that re-adds the restored session ID, so the recovered terminal is no longer treated as a remote PTY and the next session snapshot will drop the persistent PTY state.

Useful? React with 👍 / 👎.

: nil
lines += retryingAttachLines(command: attachCommand, missingSessionFallbackCommand: fallbackCommand)
return "/bin/sh -c \(shellQuote(lines.joined(separator: "\n")))"
}

Expand All @@ -169,8 +172,11 @@ nonisolated enum SSHPTYAttachStartupCommandBuilder {
)
}

private static func retryingAttachLines(command: String) -> [String] {
[
private static func retryingAttachLines(
command: String,
missingSessionFallbackCommand: String? = nil
) -> [String] {
var lines = [
"cmux_ssh_attach_reconnect_limit=\"${CMUX_SSH_RECONNECT_LIMIT:-20}\"",
"case \"$cmux_ssh_attach_reconnect_limit\" in ''|*[!0-9]*) cmux_ssh_attach_reconnect_limit=20 ;; esac",
"cmux_ssh_attach_reconnect_delay=\"${CMUX_SSH_RECONNECT_DELAY_SECONDS:-2}\"",
Expand All @@ -179,13 +185,25 @@ nonisolated enum SSHPTYAttachStartupCommandBuilder {
"while :; do",
" \(command)",
" cmux_ssh_attach_status=$?",
]
if let missingSessionFallbackCommand {
lines += [
" if [ \"$cmux_ssh_attach_status\" -eq 253 ]; then",
" if [ -t 2 ]; then printf '\\n\\033[33m[cmux] persisted SSH PTY session is gone; starting a new remote shell.\\033[0m\\n' >&2 || true; fi",
" \(missingSessionFallbackCommand)",
" exit \"$?\"",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fallback attach skips bridge retries

Medium Severity

After exit code 253, the stale-session fallback runs a second ssh-pty-attach and immediately exits with its status. That bypasses the surrounding retry loop that handles exit codes 254 and 255 for the primary attach. A recovered remote shell can therefore stop reattaching on transient bridge closes that the main path would retry.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit bdf5944. Configure here.

" fi",
]
}
lines += [
" case \"$cmux_ssh_attach_status\" in 254|255) ;; *) exit \"$cmux_ssh_attach_status\" ;; esac",
" if [ \"$cmux_ssh_attach_retry\" -ge \"$cmux_ssh_attach_reconnect_limit\" ]; then exit \"$cmux_ssh_attach_status\"; fi",
" cmux_ssh_attach_retry=$((cmux_ssh_attach_retry + 1))",
" if [ -t 2 ]; then printf '\\n\\033[33m[cmux] remote PTY bridge closed; reattaching (attempt %s/%s).\\033[0m\\n' \"$cmux_ssh_attach_retry\" \"$cmux_ssh_attach_reconnect_limit\" >&2 || true; fi",
" if [ \"$cmux_ssh_attach_reconnect_delay\" -gt 0 ]; then sleep \"$cmux_ssh_attach_reconnect_delay\"; fi",
"done",
]
return lines
}

private static func foregroundAuthLines(_ auth: ForegroundAuth) -> [String] {
Expand Down
2 changes: 1 addition & 1 deletion cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -3835,7 +3835,7 @@ final class CLINotifyProcessIntegrationRegressionTests: XCTestCase {

wait(for: [socketHandled], timeout: 3)
XCTAssertFalse(result.timedOut, result.stderr)
XCTAssertEqual(result.status, 1, result.stderr)
XCTAssertEqual(result.status, 253, result.stderr)
XCTAssertTrue(result.stderr.contains("persistent SSH PTY session is no longer running"), result.stderr)
let methods = state.snapshot().compactMap { self.jsonObject($0)?["method"] as? String }
XCTAssertEqual(methods, [
Expand Down
12 changes: 10 additions & 2 deletions cmuxTests/TabManagerSessionSnapshotTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -2259,9 +2259,16 @@ final class TabManagerSessionSnapshotTests: XCTestCase {
XCTAssertTrue(restoredInitialCommand.contains(restoredForegroundAuthToken), restoredInitialCommand)
XCTAssertTrue(restoredInitialCommand.contains("--require-existing"), restoredInitialCommand)
XCTAssertTrue(restoredInitialCommand.contains("254|255"), restoredInitialCommand)
XCTAssertTrue(restoredInitialCommand.contains("\"$cmux_ssh_attach_status\" -eq 253"), restoredInitialCommand)
XCTAssertTrue(restoredInitialCommand.contains("persisted SSH PTY session is gone"), restoredInitialCommand)
XCTAssertTrue(restoredInitialCommand.contains(expectedSessionID), restoredInitialCommand)
XCTAssertTrue(restoredInitialCommand.contains("CMUX_SURFACE_ID"), restoredInitialCommand)
XCTAssertFalse(restoredInitialCommand.contains("--command-b64 "), restoredInitialCommand)
XCTAssertTrue(restoredInitialCommand.contains("--command-b64 "), restoredInitialCommand)
let restoredInitialRemoteCommand = try XCTUnwrap(Self.decodedSSHPTYCommandB64(in: restoredInitialCommand))
XCTAssertTrue(
restoredInitialRemoteCommand.contains("export CMUX_SOCKET_PATH=127.0.0.1:64003"),
restoredInitialRemoteCommand
)

let roundTrip = restoredWorkspace.sessionSnapshot(includeScrollback: false)
XCTAssertEqual(roundTrip.remote?.preserveAfterTerminalExit, true)
Expand Down Expand Up @@ -2334,7 +2341,8 @@ final class TabManagerSessionSnapshotTests: XCTestCase {
let command = try XCTUnwrap(panel.surface.debugInitialCommand())
XCTAssertTrue(command.contains("ssh-pty-attach"), command)
XCTAssertTrue(command.contains("--require-existing"), command)
XCTAssertFalse(command.contains("--command-b64 "), command)
XCTAssertTrue(command.contains("--command-b64 "), command)
XCTAssertTrue(command.contains("\"$cmux_ssh_attach_status\" -eq 253"), command)
XCTAssertTrue(
expectedSessionIDs.contains { command.contains($0) },
command
Expand Down
21 changes: 21 additions & 0 deletions cmuxTests/WorkspaceRemoteConnectionTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -986,6 +986,27 @@ final class WorkspaceRemoteConnectionTests: XCTestCase {
XCTAssertEqual(detail, "remote port forwarding failed for listen port 64009")
}

func testReverseRelayStartupFailureDetectsExactStaleRemoteListenerPort() {
XCTAssertTrue(
WorkspaceRemoteSessionController.reverseRelayStartupFailureIndicatesStaleRemoteListener(
"remote port forwarding failed for listen port 64009",
relayPort: 64009
)
)
XCTAssertFalse(
WorkspaceRemoteSessionController.reverseRelayStartupFailureIndicatesStaleRemoteListener(
"remote port forwarding failed for listen port 64010",
relayPort: 64009
)
)
XCTAssertFalse(
WorkspaceRemoteSessionController.reverseRelayStartupFailureIndicatesStaleRemoteListener(
"Permission denied (publickey)",
relayPort: 64009
)
)
}

func testExecutableSearchPathsIncludesHomebrewAndHomeFallbacks() {
let paths = WorkspaceRemoteSessionController.executableSearchPaths(
environment: [
Expand Down
30 changes: 28 additions & 2 deletions tests_v2/test_ssh_remote_detachable_pty.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,25 @@ def _resolve_workspace_id(client: cmux, payload: dict, *, before_workspace_ids:
raise cmuxError(f"Unable to resolve workspace_id from payload: {payload}")


def _resolve_surface_id(client: cmux, workspace_id: str, payload: dict, *, before_surface_ids: set[str]) -> str:
surface_id = str(payload.get("surface_id") or "")
if surface_id:
return surface_id

surface_ref = str(payload.get("surface_ref") or "")
current_surfaces = client.list_surfaces(workspace_id)
if surface_ref.startswith("surface:"):
for index, resolved_surface_id, _focused in current_surfaces:
if f"surface:{index}" == surface_ref:
return resolved_surface_id

new_ids = sorted({surface_id for _index, surface_id, _focused in current_surfaces} - before_surface_ids)
if len(new_ids) == 1:
return new_ids[0]

raise cmuxError(f"Unable to resolve surface_id from payload: {payload}")


def _workspace_row(client: cmux, workspace_id: str) -> dict:
rows = (client._call("workspace.list", {}) or {}).get("workspaces") or []
for row in rows:
Expand Down Expand Up @@ -233,12 +252,19 @@ def detached_session_is_listed() -> bool:
f"detached session should keep bounded scrollback metadata: {row_after_detach}",
)

before_attach_surface_ids = {
surface_id for _index, surface_id, _focused in client.list_surfaces(workspace_id)
}
attach_payload = _run_cli_json(
cli,
["ssh-session-attach", "--workspace", workspace_id, "--session-id", session_id],
)
reattached_surface = str(attach_payload.get("surface_id") or "")
_must(reattached_surface, f"ssh-session-attach output missing surface_id: {attach_payload}")
reattached_surface = _resolve_surface_id(
client,
workspace_id,
attach_payload,
before_surface_ids=before_attach_surface_ids,
)

second_probe = _run_surface_probe(
client,
Expand Down
Loading