Repository navigation
Conversation
|
@boolafish is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
ChangesSSH PTY Resize Resilience
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (17 passed)
✨ Finishing Touches🧪 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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| queue.asyncAfter(deadline: .now() + delay) { | ||
| guard generation == resizeGeneration else { return } | ||
| sendLatestResize(attempt: attempt + 1, generation: generation) | ||
| } |
There was a problem hiding this comment.
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)
| 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) | ||
| } |
There was a problem hiding this comment.
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.
| throw CLIError(message: "ssh-pty-attach: bridge status exceeded \(maxStatusBytes) bytes") | ||
| } | ||
|
|
||
| private static let sshPTYResizeRetryDelays: [TimeInterval] = [0.05, 0.15, 0.35] |
There was a problem hiding this comment.
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!
1 participant
Fixes #6306.
Related to #5700.
Summary
SSH PTY attach currently forwards pane resize changes through
workspace.remote.pty_resizefrom the SIGWINCH handler, but delivery is best-effort and failures are silently dropped. If that send races a stale or temporarily unavailable remote-session control path, the remote PTY can miss the current size while output continues to flow.This change adds a small coalesced retry loop for SSH PTY resize delivery:
Validation
xcrun swiftc -parse CLI/cmux.swiftgit diff --checkNeed help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Make SSH PTY resize delivery reliable by adding a coalesced retry loop so the remote PTY doesn’t miss size updates during brief control-path hiccups.
sendV2timeout.NSLockfor thread-safe sends; includeallow_moved_surfacewhensurface_idis present.Written for commit 24b7d7e. Summary will update on new commits.
Summary by CodeRabbit