Skip to content

Retry SSH PTY resize relay #6307

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
boolafish wants to merge 1 commit into from
Closed

Retry SSH PTY resize relay #6307

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
81 changes: 64 additions & 17 deletions CLI/cmux.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11898,6 +11898,40 @@ struct CMUXCLI {
throw CLIError(message: "ssh-pty-attach: bridge status exceeded \(maxStatusBytes) bytes")
}

private static let sshPTYResizeRetryDelays: [TimeInterval] = [0.05, 0.15, 0.35]

@greptile-apps greptile-apps Bot Jun 17, 2026

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 Timing repair papers over the underlying socket race (architectural rethink)

The swift-architectural-rethink rule flags asyncAfter used to paper over socket races and requires the fix to name the invariant and state transition that makes the whole class impossible. The retry loop assumes the send will succeed within 0.55 s of accumulated wall-clock delay, but the actual failure condition — a stale or temporarily unavailable remote-session control path — is still representable after this change. If sendV2 continues to fail beyond the third attempt, the PTY size is silently wrong with no diagnostic. A more durable fix would surface send errors through the session's existing error or reconnection channel, so the invariant "the remote PTY always reflects the current terminal size" is owned by the session lifecycle rather than a blind retry counter.

Rule Used: Flag Swift fixes that patch symptoms while leaving... (source)

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 sendSSHPTYResize(
client: SocketClient,
workspaceId: String,
surfaceID: String?,
sessionID: String,
attachmentID: String,
attachmentToken: String,
cols: Int,
rows: Int,
socketLock: NSLock
) throws {
socketLock.lock()
defer { socketLock.unlock() }
var params: [String: Any] = [
"workspace_id": workspaceId,
"session_id": sessionID,
"attachment_id": attachmentID,
"attachment_token": attachmentToken,
"cols": cols,
"rows": rows,
]
if let surfaceID {
params["surface_id"] = surfaceID
params["allow_moved_surface"] = true
}
_ = try client.sendV2(
method: "workspace.remote.pty_resize",
params: params,
responseTimeout: 2.0
)
}

private func startSSHPTYResizeSource(
client: SocketClient,
workspaceId: String,
Expand All @@ -11908,27 +11942,40 @@ struct CMUXCLI {
socketLock: NSLock
) -> DispatchSourceSignal {
signal(SIGWINCH, SIG_IGN)
let queue = DispatchQueue(label: "com.cmux.ssh-pty.resize")
var resizeGeneration: UInt64 = 0

func sendLatestResize(attempt: Int, generation: UInt64) {
let size = self.currentCLITerminalSize()
do {
try self.sendSSHPTYResize(
client: client,
workspaceId: workspaceId,
surfaceID: surfaceID,
sessionID: sessionID,
attachmentID: attachmentID,
attachmentToken: attachmentToken,
cols: size.cols,
rows: size.rows,
socketLock: socketLock
)
} catch {
guard attempt < Self.sshPTYResizeRetryDelays.count else { return }
let delay = Self.sshPTYResizeRetryDelays[attempt]
queue.asyncAfter(deadline: .now() + delay) {
guard generation == resizeGeneration else { return }
sendLatestResize(attempt: attempt + 1, generation: generation)
}
Comment on lines +11965 to +11968

@greptile-apps greptile-apps Bot Jun 17, 2026

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 asyncAfter timing-based retry violates swift-blocking-runtime rule

queue.asyncAfter with hardcoded delays [0.05, 0.15, 0.35] is explicitly prohibited by the cmux blocking-runtime rule: "DispatchQueue.asyncAfter … treat these as failures by default, even when the delay is small. Retry backoff … still need a real cancellation-aware scheduler, timer abstraction, async sequence, callback, notification, or state transition." The socket send path is also called out as latency-sensitive in the rule. The real event here is acknowledgment or error from sendV2 — the fix should use Swift concurrency (async/await with a cancellation-aware Task and structured backoff via an AsyncSequence or a CancellableTimer abstraction) rather than wall-clock polling via asyncAfter.

Rule Used: Flag new blocking or timing-based synchronization ... (source)

Comment on lines +11946 to +11968

@greptile-apps greptile-apps Bot Jun 17, 2026

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 Pending retries survive session teardown

The generation counter prevents a stale retry from scheduling the next attempt, but it does not cancel an already-dispatched asyncAfter block. When the SSH session ends and the caller cancels the DispatchSourceSignal, no new SIGWINCH events fire, so resizeGeneration is never incremented. Any in-flight asyncAfter block will pass guard generation == resizeGeneration and call sendLatestResize, which acquires socketLock and calls client.sendV2 with a 2-second responseTimeout on what may already be a closed socket. This can hold socketLock for up to 2 seconds after teardown. The fix is to expose a cancellation path — for example, incrementing resizeGeneration in a cancellation/deinit hook, or returning a handle whose cancel() sets an isCancelled flag that each pending block checks before proceeding.

}
}

let source = DispatchSource.makeSignalSource(
signal: SIGWINCH,
queue: DispatchQueue(label: "com.cmux.ssh-pty.resize")
queue: queue
)
source.setEventHandler {
let size = self.currentCLITerminalSize()
socketLock.lock()
defer { socketLock.unlock() }
var params: [String: Any] = [
"workspace_id": workspaceId,
"session_id": sessionID,
"attachment_id": attachmentID,
"attachment_token": attachmentToken,
"cols": size.cols,
"rows": size.rows,
]
if let surfaceID {
params["surface_id"] = surfaceID
params["allow_moved_surface"] = true
}
_ = try? client.sendV2(method: "workspace.remote.pty_resize", params: params)
resizeGeneration &+= 1
sendLatestResize(attempt: 0, generation: resizeGeneration)
}
source.resume()
return source
Expand Down