Repository navigation
Coalesce and retry SSH PTY resize delivery (#6306) - #6320
Closed
austinywang wants to merge 9 commits into
Closed
austinywang wants to merge 9 commits into
austinywang wants to merge 9 commits into
Conversation
Previously workspace.remote.pty_resize was sent best-effort with try?, so a send that raced a stale/blocked remote-session control path was silently dropped. Output kept flowing so the terminal looked alive while the remote PTY/TUI never received the new size, requiring a manual workspace reconnect to recover (#6306). Add SSHPTYResizeCoordinator which coalesces rapid SIGWINCH bursts into one delivery of the newest size and retries the latest size with bounded exponential backoff when delivery fails, instead of dropping it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Subprocess integration test driving the real ssh-pty-attach CLI against a mock control socket: every workspace.remote.pty_resize fails, and a single SIGWINCH must still produce a retried resize RPC with no further signals. The coordinator lives in the cmux_cli module, which the cmuxTests target (@testable import cmux) cannot import, so coverage uses the repo's standard subprocess-based CLI test harness rather than a unit test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds ChangesSSH PTY Resize Coalescing and Retry
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+107
to
+109
| scheduleAfter: { delay, block in | ||
| queue.asyncAfter(deadline: .now() + delay, execute: block) | ||
| }, |
Contributor
There was a problem hiding this comment.
asyncAfter for retry backoff in a socket/terminal path
The production scheduleAfter closure drops directly into queue.asyncAfter, which is explicitly flagged by the repo's blocking-runtime rule for retry backoff in socket and terminal paths. The rule says retry backoff must use "a real cancellation-aware scheduler, timer abstraction, async sequence, callback, notification, or state transition." The cancelled flag and [weak self] guard against stale fires but asyncAfter itself has no cancellation handle — once scheduled, the closure cannot be recalled, only no-oped. A DispatchSourceTimer configured on queue with setEventHandler / cancel() would give the coordinator an actual cancellation-owning handle and is the idiomatic replacement here.
Rule Used: Flag new blocking or timing-based synchronization ... (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!
Comment on lines
+175
to
+182
| if retryCount < maxRetries { | ||
| retryCount += 1 | ||
| let backoffMs = min(1000, 50 * (1 << min(retryCount - 1, 4))) | ||
| scheduleDelivery(after: .milliseconds(backoffMs)) | ||
| } else { | ||
| log("ssh-pty resize giving up after \(maxRetries) attempts (\(size.cols)x\(size.rows)); awaiting next resize") | ||
| retryCount = 0 | ||
| } |
Contributor
There was a problem hiding this comment.
When retryCount reaches maxRetries the give-up message prints "giving up after \(maxRetries) attempts", but by that point maxRetries + 1 delivery calls have been made (one original call at retryCount == 0 plus maxRetries retried calls). With the default maxRetries = 6 the log will say "6 attempts" while 7 deliveries were actually attempted. Since this is a debug-only log the impact is cosmetic, but the count will mislead anyone reading the log while investigating a stuck resize.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@CLI/CMUXCLI`+SSHCommandSupport.swift:
- Around line 116-124: The issue is that when a retry timer fires after a failed
resize delivery, and then a fresh user resize occurs (noteResize() call), the
old stale retry timer takes precedence over the new coalesced delivery.
Implement a generation token mechanism to fix this: add a generation counter
that increments each time noteResize() is called, then pass this generation
token when scheduling delivery in both the scheduleDelivery(after:
coalesceDelay) call in noteResize() and when scheduling retry backoff. When a
scheduled delivery fires, check that the generation token still matches the
current generation before proceeding with delivery; if it's stale, discard it so
that fresh resize events can properly supersede pending retry timers.
- Around line 62-64: The cached `lastDeliveredSize` is being used to suppress
fresh SIGWINCH delivery when the new size matches a previously delivered size,
but this causes stale remote PTY geometry to persist if the remote session is
rebuilt or reset without updating the local cache. Remove the comparison logic
that checks `lastDeliveredSize` against the current pending size to suppress
delivery (around line 146). Keep only the burst coalescing mechanism using
`deliveryScheduled` to combine rapid consecutive resize events, but ensure that
fresh SIGWINCH signals are always delivered to the remote PTY regardless of
whether they match the cached `lastDeliveredSize`, allowing the remote state to
be revalidated with each 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
Run ID: 6347d0cb-aed2-4f50-868f-155ba4ca3ac0
📒 Files selected for processing (3)
CLI/CMUXCLI+SSHCommandSupport.swiftCLI/cmux.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swift
Comment on lines
+116
to
+124
| func noteResize() { | ||
| guard !cancelled else { return } | ||
| let size = sizeProvider() | ||
| guard size.cols > 0, size.rows > 0 else { return } | ||
| pendingSize = size | ||
| // A fresh, user-driven resize resets the retry budget so we keep trying | ||
| // to deliver the newest size even after a prior burst gave up. | ||
| retryCount = 0 | ||
| scheduleDelivery(after: coalesceDelay) |
There was a problem hiding this comment.
Let fresh resizes replace a pending retry timer.
After a failure schedules backoff, Line 135 makes later noteResize() calls keep the old timer. A user resize during a 400–1000ms retry window updates pendingSize, but delivery is delayed until the stale retry fires instead of being coalesced at coalesceDelay.
Use a generation token so fresh SIGWINCH can supersede retry backoff
private var pendingSize: (cols: Int, rows: Int)?
private var lastDeliveredSize: (cols: Int, rows: Int)?
private var deliveryScheduled = false
+ private var scheduledDeliveryToken = 0
+ private var scheduledDeliveryIsRetry = false
private var retryCount = 0
private var cancelled = false
@@
pendingSize = size
// A fresh, user-driven resize resets the retry budget so we keep trying
// to deliver the newest size even after a prior burst gave up.
retryCount = 0
- scheduleDelivery(after: coalesceDelay)
+ scheduleDelivery(after: coalesceDelay, replacingExistingRetry: true)
@@
- private func scheduleDelivery(after delay: DispatchTimeInterval) {
- guard !cancelled, !deliveryScheduled else { return }
+ private func scheduleDelivery(
+ after delay: DispatchTimeInterval,
+ replacingExistingRetry: Bool = false,
+ isRetry: Bool = false
+ ) {
+ guard !cancelled else { return }
+ if deliveryScheduled {
+ guard replacingExistingRetry, scheduledDeliveryIsRetry else { return }
+ }
+ scheduledDeliveryToken += 1
+ let token = scheduledDeliveryToken
deliveryScheduled = true
+ scheduledDeliveryIsRetry = isRetry
scheduleAfter(delay) { [weak self] in
- self?.deliver()
+ self?.deliver(ifCurrent: token)
}
}
- private func deliver() {
+ private func deliver(ifCurrent token: Int) {
+ guard token == scheduledDeliveryToken else { return }
deliveryScheduled = false
+ scheduledDeliveryIsRetry = false
guard !cancelled, let size = pendingSize else { return }
@@
retryCount += 1
let backoffMs = min(1000, 50 * (1 << min(retryCount - 1, 4)))
- scheduleDelivery(after: .milliseconds(backoffMs))
+ scheduleDelivery(after: .milliseconds(backoffMs), isRetry: true)Also applies to: 134-139, 175-179
🤖 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`+SSHCommandSupport.swift around lines 116 - 124, The issue is
that when a retry timer fires after a failed resize delivery, and then a fresh
user resize occurs (noteResize() call), the old stale retry timer takes
precedence over the new coalesced delivery. Implement a generation token
mechanism to fix this: add a generation counter that increments each time
noteResize() is called, then pass this generation token when scheduling delivery
in both the scheduleDelivery(after: coalesceDelay) call in noteResize() and when
scheduling retry backoff. When a scheduled delivery fires, check that the
generation token still matches the current generation before proceeding with
delivery; if it's stale, discard it so that fresh resize events can properly
supersede pending retry timers.
The retry regression test pushed CLINotifyProcessIntegrationRegressionTests.swift over the Swift file-length budget (workflow-guard-tests). Move it into a new SSHPTYResizeRetryIntegrationTests.swift that extends the same test class, so it still reuses the mock-socket / bundled-CLI harness helpers while keeping the large file unchanged. Wired into the cmuxTests target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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/SSHPTYResizeRetryIntegrationTests.swift`:
- Around line 148-165: The test's retry assertion can false-pass because the
loop sending up to 20 SIGWINCH signals may result in multiple resizes being
queued, and the second resizeReceived.wait could be satisfied by a late resize
from one of those earlier signals rather than from the retry-after-failure
mechanism. To tighten this test, replace the loop at line 148 with a single
SIGWINCH send (not multiple attempts), capture the resize count before
signaling, wait for the first resize after that single signal, then assert that
the count increases again without any additional SIGWINCH being sent, ensuring
the second resize definitively comes from the retry coordinator's retry logic
and not from buffered/delayed events from prior signals.
🪄 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
Run ID: 887fb339-6d9a-4fd7-9e00-e424e072acad
📒 Files selected for processing (2)
cmux.xcodeproj/project.pbxprojcmuxTests/SSHPTYResizeRetryIntegrationTests.swift
Comment on lines
+148
to
+165
| for _ in 0..<20 { | ||
| Darwin.kill(process.processIdentifier, SIGWINCH) | ||
| if resizeReceived.wait(timeout: .now() + 0.2) == .success { | ||
| sawFirstResize = true | ||
| break | ||
| } | ||
| } | ||
| XCTAssertTrue(sawFirstResize, "Expected ssh-pty-attach to issue an initial resize RPC after SIGWINCH") | ||
|
|
||
| // No further SIGWINCH is sent. A second resize attempt can only arrive | ||
| // from the coalesce/retry coordinator re-sending the latest size after | ||
| // the first delivery failed. The pre-fix best-effort send would never | ||
| // retry, so this would time out. | ||
| XCTAssertEqual( | ||
| resizeReceived.wait(timeout: .now() + 3), | ||
| .success, | ||
| "Expected ssh-pty-attach to retry the resize after a delivery failure" | ||
| ) |
There was a problem hiding this comment.
Retry assertion can false-pass because the test may send multiple user SIGWINCH events.
The loop at Line 148 can deliver more than one SIGWINCH before the first resize is observed; then the second resizeReceived.wait at Line 162 may be satisfied by a late resize from those extra signals, not by retry-after-failure. This weakens the regression guarantee.
A tighter pattern is: record resize count before signaling, send exactly one SIGWINCH (or gate to ensure only one post-install signal), wait for first resize, then assert count increases again without any additional signal.
Suggested direction
- var sawFirstResize = false
- for _ in 0..<20 {
- Darwin.kill(process.processIdentifier, SIGWINCH)
- if resizeReceived.wait(timeout: .now() + 0.2) == .success {
- sawFirstResize = true
- break
- }
- }
- XCTAssertTrue(sawFirstResize, "Expected ssh-pty-attach to issue an initial resize RPC after SIGWINCH")
+ // Send one user signal, then prove subsequent resize comes from retry.
+ Darwin.kill(process.processIdentifier, SIGWINCH)
+ XCTAssertEqual(
+ resizeReceived.wait(timeout: .now() + 3),
+ .success,
+ "Expected initial resize RPC after SIGWINCH"
+ )
XCTAssertEqual(
resizeReceived.wait(timeout: .now() + 3),
.success,
"Expected ssh-pty-attach to retry the resize after a delivery failure"
)🤖 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/SSHPTYResizeRetryIntegrationTests.swift` around lines 148 - 165,
The test's retry assertion can false-pass because the loop sending up to 20
SIGWINCH signals may result in multiple resizes being queued, and the second
resizeReceived.wait could be satisfied by a late resize from one of those
earlier signals rather than from the retry-after-failure mechanism. To tighten
this test, replace the loop at line 148 with a single SIGWINCH send (not
multiple attempts), capture the resize count before signaling, wait for the
first resize after that single signal, then assert that the count increases
again without any additional SIGWINCH being sent, ensuring the second resize
definitively comes from the retry coordinator's retry logic and not from
buffered/delayed events from prior signals.
Autoreview flagged that scheduled retry timers could outlive the attach: the DispatchSource cancel handler runs asynchronously, so a retry block already due when the bridge hit EOF could still call deliver() with cancelled==false and send workspace.remote.pty_resize on the shared socket after pty_attach_end (or block EOF cleanup on socketLock). Make SSHPTYResizeCoordinator.cancel() synchronous and thread-safe: guard the cancelled flag with a lock, have the coordinator own the SIGWINCH source, and cancel it directly. Teardown (bridge EOF / defer) now calls coordinator.cancel() synchronously, so any not-yet-started retry observes cancellation and bails before sending. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The lifecycle-fix comment pushed CLI/cmux.swift 3 lines over its tracked budget (workflow-guard-tests). Condense it; the rationale already lives in the SSHPTYResizeCoordinator doc comment. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Autoreview flagged that retrying every sendV2 failure on the shared control socket is unsafe after a transport timeout: SocketClient.send leaves the fd open on timeout and the v2 protocol does not match response ids, so a late resize reply could be misattributed to a later request (pty_sessions / pty_attach_end), desyncing the control protocol. Issue the resize at the raw send(command:) layer so delivery outcome can be classified: send only returns once a complete response line is consumed (socket in sync) — an ok:false there is a protocol rejection and safe to retry (the stale/blocked remote path from #6306). A thrown error (timeout / socket error) means the socket may hold a pending late reply, so surface it as SSHPTYResizeTransportError and stop retrying on that socket; a fresh SIGWINCH recovers once it is healthy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Aziz file-organization policy: a major type should live in its own TypeName.swift rather than appended to CMUXCLI+SSHCommandSupport.swift (which is an extension CMUXCLI of SSH command-string helpers). Relocate the coordinator and its two error types to CLI/SSHPTYResizeCoordinator.swift, wired into the cmux-cli target. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #6306
Problem
In long-lived SSH workspaces, terminal TUI panes could enter a stale resize state where changing pane width no longer reliably caused the remote TUI to reconcile to the visible size. Output kept flowing so the terminal looked alive, but the remote PTY/TUI stayed stuck at the old geometry — only a manual Reconnect Workspace restored self-healing resize.
Root cause
The SSH PTY attach CLI forwards SIGWINCH changes via
workspace.remote.pty_resize, but the send was best-effort and fire-and-forget:A send that raced a stale or blocked remote-session control path was silently dropped. The resize never reached the remote PTY/TUI and was never retried, so the size desynced until the session/controller path was rebuilt by a manual reconnect.
Fix
Add
SSHPTYResizeCoordinator(CLI/CMUXCLI+SSHCommandSupport.swift) and route the SIGWINCH source through it (CLI/cmux.swift):All coordinator state is touched only on the signal source's serial queue, so no extra locking is needed; the existing
socketLockstill guards the shared control socket. The coordinator exposes injectedsend/scheduleAfterseams for deterministic testing.Verification
./scripts/reload.sh --tag issue-6306— clean Debug build of the app + CLI.testSSHPTYAttachRetriesResizeAfterDeliveryFailure(inCLINotifyProcessIntegrationRegressionTests): spawns the realssh-pty-attachCLI against a mock control socket where everyworkspace.remote.pty_resizefails; asserts that a single SIGWINCH still produces a retried resize RPC with no further signals. The pre-fix best-effort send would drop the failed resize and the retry would never arrive.Notes / tradeoffs
cmux_climodule, which thecmuxTeststarget (@testable import cmux) cannot import, so coverage uses the repo's standard subprocess-based CLI test harness rather than an in-process unit test.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Coalesces and retries SSH PTY resize events so remote TUIs stay in sync with the visible terminal, and ties retries to the attach lifecycle to avoid sends after teardown. Fixes #6306 by replacing best‑effort sends with coordinated, protocol‑aware backoff retries and a safe, synchronous cancel.
Bug Fixes
SSHPTYResizeCoordinatorto coalesce rapidSIGWINCHinto the latest size, retry with bounded exponential backoff, supersede/dedupe in‑flight or redundant deliveries, and run work on a serial queue; still usessocketLock.SIGWINCHsource and exposes a synchronous, lock‑guardedcancel(), so pending retries don’t run after bridge EOF.client.send(command:), retry protocol rejections (ok:false/SSHPTYResizeProtocolRejection), and stop on transport failures (SSHPTYResizeTransportError); a freshSIGWINCHlater recovers.ssh-pty-attachresize through the coordinator and added a subprocess regression test (SSHPTYResizeRetryIntegrationTests.swift) that forcesworkspace.remote.pty_resizefailures and asserts a single retry without extra signals.Refactors
SSHPTYResizeCoordinatorinto its own file (CLI/SSHPTYResizeCoordinator.swift); no behavior change.CLI/cmux.swiftto meet the file‑length budget.Written for commit a929814. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests