Repository navigation
Fix ssh PTY input loss and reordering at reconnect and backpressure seams - #7717
Merged
Merged
Conversation
…essure seams Two deterministic reproductions of #7708 (garbled / out-of-order / lost keystrokes over cmux ssh). Both fail on main by design; the fix lands in the next commit. - TestWebSocketPTYReattachWritesAcceptedOldInputBeforeNew: input accepted by a superseded attachment must reach the PTY, in order, before input from the replacement attachment. On main the replaced-attachment check in writeInputChunk silently drops the queued bytes (times out reading OLDNEW). - "legacy input overflow pauses instead of closing and preserves order": input-window overflow in RemotePTYBridgeSession must backpressure the socket, not close the session. On main the session close(detach:)s and ~1MiB of accepted bytes are lost (delivered 4182004 of 5242880). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughAdds deadline-aware reconnect filtering for SSH PTY stdin and extends PTY input with seq-tagged writes, acknowledgements, and gap reporting across the daemon, Swift bridge, and workspace session flow. ChangesSSH reconnect input filter deadline fix
Sequenced, acknowledged PTY input protocol
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant RemotePTYBridgeServer.Session
participant RemotePTYBridgeRPCClient
participant RemoteDaemonRPCClient
participant cmuxd-remote
RemotePTYBridgeServer.Session->>RemotePTYBridgeRPCClient: supportsInputSeqAck
RemotePTYBridgeServer.Session->>RemoteDaemonRPCClient: attachPTY(inputSeqAck)
RemoteDaemonRPCClient->>cmuxd-remote: pty.attach {input_seq_ack}
RemotePTYBridgeServer.Session->>RemoteDaemonRPCClient: writePTY(data, seq)
RemoteDaemonRPCClient->>cmuxd-remote: pty.write {seq}
cmuxd-remote-->>RemoteDaemonRPCClient: pty.input_ack {seq}
RemoteDaemonRPCClient-->>RemotePTYBridgeServer.Session: .inputAck(seq)
sequenceDiagram
participant RemotePTYBridgeServer.Session
participant RemotePTYBridgeInputFlow
participant RemoteDaemonRPCClient
participant wsPTYHub
RemotePTYBridgeServer.Session->>RemotePTYBridgeInputFlow: enqueue(data)
RemotePTYBridgeInputFlow-->>RemotePTYBridgeServer.Session: write batch or buffer
RemotePTYBridgeServer.Session->>RemoteDaemonRPCClient: writePTY(data, seq)
RemoteDaemonRPCClient->>wsPTYHub: pty.write {seq}
wsPTYHub->>wsPTYHub: validate contiguous seq
wsPTYHub-->>RemoteDaemonRPCClient: pty.input_ack {seq}
RemoteDaemonRPCClient-->>RemotePTYBridgeServer.Session: handleInputAck(seq)
RemotePTYBridgeServer.Session->>RemotePTYBridgeInputFlow: acknowledge(upTo: seq)
Suggested reviewers: 🚥 Pre-merge checks | ✅ 23 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (23 passed)
✨ 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b31099b. Configure here.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/SSHPTYAttachReconnectInputFilter.swift`:
- Around line 201-203: The stop-request branch in
SSHPTYAttachReconnectInputFilter still uses the full pending-probe wait and can
outlive reconnectInputFilter.remainingDeadlineMilliseconds. Update the secondary
pending-input wait in SSHPTYAttachReconnectInputFilter to cap its timeout the
same way the main poll does, using the reconnect deadline and
pendingProbeContinuationTimeoutMilliseconds together. Use the existing
timeout/remainingDeadline logic around reconnectInputFilter,
pendingProbeContinuationTimeoutMilliseconds, and pending bytes handling so the
stop acknowledgement cannot block past the reconnect boundary.
In `@CLI/SSHPTYAttachReconnectInputFilterPumpIO.swift`:
- Around line 36-50: The retry loop in pollStdinPump currently reuses the same
relative timeout after EINTR, which can extend the reconnect window
unexpectedly. Update pollStdinPump to work from an absolute deadline and
recompute the remaining timeout before each Darwin.poll call, so repeated signal
interruptions do not keep the reconnect filter active past the intended cutoff.
Keep the fix localized to pollStdinPump and its timeout calculation path.
In `@daemon/remote/cmd/cmuxd-remote/main.go`:
- Around line 2067-2072: The seq parsing in the request handling logic should
reject present-but-invalid values instead of silently treating them as missing.
Update the seq extraction path around getIntParam in the attachment request flow
so that if the "seq" parameter is present but negative or malformed, the handler
returns invalid_params rather than leaving hasSeq false. Preserve the current
valid behavior for accepted seq values, and make sure the logic in the
seq-ack/legacy attachment handling branches uses the presence of seq to
distinguish absent from invalid input.
In
`@Packages/macOS/CmuxRemoteWorkspace/Tests/CmuxRemoteWorkspaceTests/RemotePTYBridgeServerTests.swift`:
- Line 387: The test currently uses a fixed usleep in
RemotePTYBridgeServerTests, which makes the “no unacked drain” assertion
timing-dependent. Replace that sleep with a causal wait in the relevant test
flow by waiting on a real predicate or completion signal that the input window
has filled before asserting the byte cap and only then emitting ACKs. Use the
existing test helpers and symbols around the assertion site in
RemotePTYBridgeServerTests to gate on readiness instead of wall-clock delay.
🪄 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: 3bd811c5-a6ab-40d3-8755-59e577f05271
📒 Files selected for processing (25)
CLI/SSHPTYAttachReconnectInputFilter.swiftCLI/SSHPTYAttachReconnectInputFilterPumpIO.swiftCLI/SSHPTYAttachReconnectInputFilterState.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Capabilities/RemoteDaemonCapability.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonPTYEvent.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient+Events.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient+RPC.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient+RemotePTYBridgeRPCClient.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/PTYBridge/RemotePTYBridgeEvent.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/PTYBridge/RemotePTYBridgeRPCClient.swiftPackages/macOS/CmuxRemoteDaemon/Tests/CmuxRemoteDaemonTests/RemoteDaemonRPCClientCapabilityTests.swiftPackages/macOS/CmuxRemoteDaemon/Tests/CmuxRemoteDaemonTests/RemoteDaemonStringsTests.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/PTYBridge/RemotePTYBridgeInputFlow.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/PTYBridge/RemotePTYBridgeSession+Input.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/PTYBridge/RemotePTYBridgeSession.swiftPackages/macOS/CmuxRemoteWorkspace/Tests/CmuxRemoteWorkspaceTests/RemotePTYBridgeServerTests.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SSHPTYAttachReconnectInputFilterTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swiftdaemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.godaemon/remote/cmd/cmuxd-remote/ws_pty.godaemon/remote/cmd/cmuxd-remote/ws_pty_test.godaemon/remote/cmuxd-remote
Contributor
Greptile SummaryThis PR closes the input direction of SSH PTY data loss/reordering by adding optional sequenced, cumulatively-acked
Confidence Score: 4/5The change is safe to merge for the vast majority of users; the new seq-ack path is capability-gated and the mixed-version fallback is correct. The two previously-flagged concurrent-write concerns appear to be resolved: inputEnqueueMu serialises every writeInput call for the same session end-to-end, and flushAcceptedInput no longer exists. The core PTY input paths have been substantially rewritten across the Go daemon, Swift bridge session, and CLI reconnect filter. The seq-ack mechanism is new wire protocol with real-time keystroke implications. The changes are well-tested and the key concurrency invariant (inputEnqueueMu scope) is sound, but the cross-language protocol logic and three-layer change warrant an extra pair of eyes before merge. No blocking defects were found in the new code. daemon/remote/cmd/cmuxd-remote/ws_pty.go — the lastAcceptedSeq two-lock pattern and its dependency on inputEnqueueMu scope deserves close reading; see inline comment. Packages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/PTYBridge/RemotePTYBridgeInputFlow.swift — the local-buffer nil-return path versus the daemon-window backpressure path should be documented more precisely. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant App as App (RemotePTYBridgeSession)
participant Flow as RemotePTYBridgeInputFlow
participant Daemon as cmuxd-remote (ws_pty.go)
Note over App,Daemon: Capability handshake
Daemon-->>App: "hello {capabilities: [pty.input.seq_ack, ...]}"
App->>Daemon: "pty.attach {input_seq_ack: true}"
Note over App,Daemon: Seq-ack mode: window-controlled writes
App->>Flow: enqueue(keystroke data)
Flow-->>App: "DrainResult{writes:[{data, seq:1}]}"
App->>Daemon: "pty.write {seq:1, data_base64:...}"
Note right of Daemon: validated under inputEnqueueMu, written to PTY fd
Daemon-->>App: "pty.input_ack {seq:1} (coalesced)"
App->>Flow: acknowledge(upTo: 1)
Flow-->>App: "DrainResult{shouldResumeReads: true}"
App->>App: receiveNext() — resume NWConnection reads
Note over App,Daemon: Seq gap → visible error + detach
App->>Daemon: "pty.write {seq:3} — gap!"
Daemon-->>App: "error {code: pty_input_seq_gap}"
Daemon-->>App: pty.error + detach
Note over App,Daemon: Reconnect seam ordering
Note left of App: OLD attachment bytes stay queued in FIFO
App->>Daemon: pty.attach (new attachment)
Note right of Daemon: OLD bytes drain first, then NEW bytes follow
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant App as App (RemotePTYBridgeSession)
participant Flow as RemotePTYBridgeInputFlow
participant Daemon as cmuxd-remote (ws_pty.go)
Note over App,Daemon: Capability handshake
Daemon-->>App: "hello {capabilities: [pty.input.seq_ack, ...]}"
App->>Daemon: "pty.attach {input_seq_ack: true}"
Note over App,Daemon: Seq-ack mode: window-controlled writes
App->>Flow: enqueue(keystroke data)
Flow-->>App: "DrainResult{writes:[{data, seq:1}]}"
App->>Daemon: "pty.write {seq:1, data_base64:...}"
Note right of Daemon: validated under inputEnqueueMu, written to PTY fd
Daemon-->>App: "pty.input_ack {seq:1} (coalesced)"
App->>Flow: acknowledge(upTo: 1)
Flow-->>App: "DrainResult{shouldResumeReads: true}"
App->>App: receiveNext() — resume NWConnection reads
Note over App,Daemon: Seq gap → visible error + detach
App->>Daemon: "pty.write {seq:3} — gap!"
Daemon-->>App: "error {code: pty_input_seq_gap}"
Daemon-->>App: pty.error + detach
Note over App,Daemon: Reconnect seam ordering
Note left of App: OLD attachment bytes stay queued in FIFO
App->>Daemon: pty.attach (new attachment)
Note right of Daemon: OLD bytes drain first, then NEW bytes follow
Reviews (5): Last reviewed commit: "review: cmux policy cleanups — file-scop..." | Re-trigger Greptile |
#7708) Closes the four input-path holes behind garbled / out-of-order keystrokes over cmux ssh at reconnect and backpressure seams: - pty.write now carries an optional per-attachment monotonic seq (capability "pty.input.seq_ack", opt-in via pty.attach input_seq_ack). The daemon rejects gaps with the wire-pinned rpc error pty_input_seq_gap, which surfaces as a visible pty.error instead of silently writing whatever arrives. Cumulative, coalesced pty.input_ack events flow back after bytes hit the PTY fd. Writes stay async notifications, so the typing-latency win from 719a231 is kept. - Reattach seam is quiesced: superseding an attachment drains every already-accepted input chunk to the PTY through the single input-loop consumer (flush-barrier sentinel under inputEnqueueMu) before the replacement may enqueue, and the supersede slot is re-checked after the barrier so a concurrent attach for the same id is superseded too, never silently overwritten. Old and new bytes can no longer interleave or drop. - RemotePTYBridgeSession no longer close(detach:)s on input-window overflow: a queue-confined flow controller (RemotePTYBridgeInputFlow) pauses socket receives at the window and resumes on drain — acks in seq_ack mode, write completions in legacy mode — so accepted bytes are never dropped in either mode. - The reconnect input filter is bounded by a monotonic deadline (injectable clock, poll timeout capped by the remaining deadline) and flushes pending bytes on every pump exit (EOF, read error, poll failure), so it can never eat real ESC-prefixed keystrokes indefinitely; the fuzz test interleaves keys before/between/after probe replies, including a lone ESC. Mixed versions stay compatible: the capability is optional (never part of the required handshake set), old daemons ignore the attach param and seq, and old apps see no acks and no enforcement. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
force-pushed
the
issue-7708-ssh-input-ordering
branch
from
July 9, 2026 09:08
b31099b to
d71c376
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
CLI/SSHPTYAttachReconnectInputFilter.swift (1)
217-222: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCap the secondary pending-input wait by the reconnect deadline.
Line 220 still waits the full
pendingProbeContinuationTimeoutMillisecondsafter a stop request. If the reconnect deadline is sooner, pending bytes and the stop acknowledgement can still be delayed past the cutoff.Proposed fix
+ let pendingTimeoutMilliseconds: Int32 + if let remaining = reconnectInputFilter?.remainingDeadlineMilliseconds { + let cappedRemaining = Int32(min(Int64(Int32.max), max(Int64(0), remaining))) + pendingTimeoutMilliseconds = min( + pendingProbeContinuationTimeoutMilliseconds, + cappedRemaining + ) + } else { + pendingTimeoutMilliseconds = pendingProbeContinuationTimeoutMilliseconds + } guard let pendingReadiness = pollStdinPump( inputFD: inputFD, stopSignalFD: nil, - timeoutMilliseconds: pendingProbeContinuationTimeoutMilliseconds + timeoutMilliseconds: pendingTimeoutMilliseconds ) else {As per coding guidelines, “In cmux-sensitive Swift paths such as typing, terminal rendering, socket telemetry, and focus handling, blocking or sleep-based coordination should fail CI.”
🤖 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/SSHPTYAttachReconnectInputFilter.swift` around lines 217 - 222, The secondary pending-input wait in SSHPTYAttachReconnectInputFilter should not exceed the reconnect deadline, since pollStdinPump can currently block longer than allowed after a stop request. Update the logic around the pendingReadiness handling to compute the remaining time until the reconnect cutoff and pass that capped timeout instead of always using pendingProbeContinuationTimeoutMilliseconds, while preserving the flushPendingThenShutdown path when no readiness is returned.Sources: Coding guidelines, Path instructions
CLI/SSHPTYAttachReconnectInputFilterPumpIO.swift (1)
36-50: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the absolute timeout across
EINTRretries.
pollStdinPumpretriespollwith the original relative timeout, so repeated signals can extend the reconnect-filter window and keep stripping ESC-prefixed input past the intended cutoff. Compute an absolute end time once, then recompute the remaining timeout before each retry.🤖 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/SSHPTYAttachReconnectInputFilterPumpIO.swift` around lines 36 - 50, The poll retry loop in SSHPTYAttachReconnectInputFilterPumpIO’s polling helper is reusing the original relative timeout after EINTR, which can unintentionally extend the reconnect-filter window. Update the polling logic in pollStdinPump (or the surrounding poll loop) to compute a single absolute deadline before the first poll, then derive the remaining timeout on each retry after EINTR so the total wait time stays bounded. Keep the existing inputReady/stopRequested behavior unchanged while only adjusting how timeoutMilliseconds is calculated across retries.Sources: Coding guidelines, Path instructions
🤖 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/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/PTYBridge/RemotePTYBridgeInputFlow.swift`:
- Around line 68-79: `acknowledge(upTo:)` in `RemotePTYBridgeInputFlow` always
returns a `DrainResult`, so invalid or stale ack values are currently accepted
and the nil-check in `RemotePTYBridgeSession+Input` is unreachable. Add seq
validation inside `acknowledge(upTo:)` against the highest sent/pending sequence
(using the existing `pendingWrites`, `seqAckEnabled`, and `flushBufferedInput`
flow), and return nil or otherwise signal failure for out-of-range
acknowledgements so the caller’s protocol-error handling can trigger.
---
Duplicate comments:
In `@CLI/SSHPTYAttachReconnectInputFilter.swift`:
- Around line 217-222: The secondary pending-input wait in
SSHPTYAttachReconnectInputFilter should not exceed the reconnect deadline, since
pollStdinPump can currently block longer than allowed after a stop request.
Update the logic around the pendingReadiness handling to compute the remaining
time until the reconnect cutoff and pass that capped timeout instead of always
using pendingProbeContinuationTimeoutMilliseconds, while preserving the
flushPendingThenShutdown path when no readiness is returned.
In `@CLI/SSHPTYAttachReconnectInputFilterPumpIO.swift`:
- Around line 36-50: The poll retry loop in
SSHPTYAttachReconnectInputFilterPumpIO’s polling helper is reusing the original
relative timeout after EINTR, which can unintentionally extend the
reconnect-filter window. Update the polling logic in pollStdinPump (or the
surrounding poll loop) to compute a single absolute deadline before the first
poll, then derive the remaining timeout on each retry after EINTR so the total
wait time stays bounded. Keep the existing inputReady/stopRequested behavior
unchanged while only adjusting how timeoutMilliseconds is calculated across
retries.
🪄 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: 8745523a-24f7-432e-8854-28e83dcf27d0
📒 Files selected for processing (25)
CLI/SSHPTYAttachReconnectInputFilter.swiftCLI/SSHPTYAttachReconnectInputFilterPumpIO.swiftCLI/SSHPTYAttachReconnectInputFilterState.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Capabilities/RemoteDaemonCapability.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonPTYEvent.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient+Events.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient+RPC.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient+RemotePTYBridgeRPCClient.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/Client/RemoteDaemonRPCClient.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/PTYBridge/RemotePTYBridgeEvent.swiftPackages/macOS/CmuxRemoteDaemon/Sources/CmuxRemoteDaemon/PTYBridge/RemotePTYBridgeRPCClient.swiftPackages/macOS/CmuxRemoteDaemon/Tests/CmuxRemoteDaemonTests/RemoteDaemonRPCClientCapabilityTests.swiftPackages/macOS/CmuxRemoteDaemon/Tests/CmuxRemoteDaemonTests/RemoteDaemonStringsTests.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/PTYBridge/RemotePTYBridgeInputFlow.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/PTYBridge/RemotePTYBridgeSession+Input.swiftPackages/macOS/CmuxRemoteWorkspace/Sources/CmuxRemoteWorkspace/PTYBridge/RemotePTYBridgeSession.swiftPackages/macOS/CmuxRemoteWorkspace/Tests/CmuxRemoteWorkspaceTests/RemotePTYBridgeServerTests.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SSHPTYAttachReconnectInputFilterTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swiftdaemon/remote/.gitignoredaemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/main_test.godaemon/remote/cmd/cmuxd-remote/ws_pty.godaemon/remote/cmd/cmuxd-remote/ws_pty_test.go
Addresses PR #7717 review feedback (CodeRabbit): - pollStdinPump anchors its timeout to an absolute monotonic deadline so EINTR retries cannot extend the reconnect-filter window under repeated signal delivery. - pty.write with a present-but-malformed seq (non-integer or negative) is rejected with invalid_params instead of being treated as absent, which produced a misleading 'got 0' gap for seq-ack attachments and silently accepted malformed input for legacy ones. - RemotePTYBridgeInputFlow.acknowledge(upTo:) rejects acks for seqs that were never sent, making the session's protocol-violation teardown branch reachable instead of trusting a malformed daemon ack. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
austinywang
enabled auto-merge (squash)
July 9, 2026 09:41
Addresses two structured-review findings on #7717: - prepareAttachment no longer waits on a flush barrier before registering a replacement attachment. The barrier could block the pty.attach RPC indefinitely behind a wedged PTY (stopped foreground process + large paste), holding the attach path hostage. Ordering does not need it: session.input is FIFO with writeInputLoop as its only consumer, whole writes enqueue atomically under inputEnqueueMu, and writeInputChunk does not require the chunk's attachment to still be registered — so accepted old-attachment bytes reach the PTY ahead of the replacement's input by construction. The reattach-seam test now also asserts attach completes while the PTY writer is stalled. - A pty_input_seq_gap notification error now detaches the attachment daemon-side (like pty_input_queue_full) instead of leaving a stale attachment registered and streaming ignored output after the client closed. Co-Authored-By: Claude Fable 5 <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 `@daemon/remote/cmd/cmuxd-remote/ws_pty.go`:
- Around line 828-835: Reword the supersession comment in ws_pty.go so it
clearly documents the OLD→NEW ordering contract without the garbled phrasing.
Keep the explanation tied to the attachment reattach path and the
writeInputLoop/inputEnqueueMu/writeInputChunk behavior: old attachment input
already accepted must remain queued and reach the PTY before any replacement
input, and reattach must never block waiting for a drain. Make the wording
concise and grammatical while preserving the concurrency invariant.
🪄 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: 1d9960bf-6b75-46a5-bc49-4f83d45f6381
📒 Files selected for processing (3)
daemon/remote/cmd/cmuxd-remote/main.godaemon/remote/cmd/cmuxd-remote/ws_pty.godaemon/remote/cmd/cmuxd-remote/ws_pty_test.go
A saturated seq-ack attachment's enqueueInputAck cancels the attachment but left it registered in session.attachments, blocking idle-reaping and leaving stale size/input state under the old token. Mirror the output enqueue path: dropAttachment on ack-queue failure, outside ptyWriteMu (dropAttachment can resize via applyCurrentPTYSize, which takes it). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A pre-attach stdin buffer near the 4 MiB input window became a single pty.write whose base64 payload exceeded the daemon's 4 MiB RPC frame limit; the daemon rejects such frames before parsing attachment identity, so in seq-ack mode the write was never acked and the input window stayed full forever, silently freezing terminal input. RemotePTYBridgeInputFlow now splits enqueued data into <=256 KiB writes (own seq each, buffered pieces stay ordered across the window boundary). Also documents why writeInputChunk is deliberately session-scoped rather than attachment-scoped: input accepted before a detach still executes (persistent-session semantics); discarding it is the silent-loss bug class this PR removes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The deadline stop path (and any pump exit) closes the stop-signal pipe read end while the output-side control can be mid-write, and control deinit closes the acknowledgement read end while the pump acks — either write could raise SIGPIPE and kill the ssh-pty-attach CLI. Set F_SETNOSIGPIPE on both write ends at creation so racing writers surface EPIPE (already handled) instead. Inlined rather than using configureCLIWriteFDNoSIGPIPE because this file also compiles into the cmuxTests target, which does not build CMUXCLI+Process.swift. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…symbols - Move the pure uint64 payload helper to a file-scope private func instead of a private static on the client (static-as-namespace policy). - Add DocC to supportsInputSeqAck (adapter) and the legacy-default protocol extension. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This branch was previously 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
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.
Fixes #7708
Problem
cmux sshkeystrokes could be garbled, reordered, or silently lost at reconnect and backpressure seams. The rendering-side bug was fixed in #6831; this PR closes the input direction, wherepty.writehad no sequencing, no acks, and no loss surfacing. Four root causes, all present on main:CLI/SSHPTYAttachReconnectInputFilter.swift): the post-reattach probe-reply filter window was unbounded until first output, so real ESC-prefixed keystrokes (arrows, alt-combos, bracketed paste) typed in that window could be eaten (prior bugs: Fix remote PTY restore probe reply leak #6070, Fix stale cmux ssh pane resize: reconcile remote PTY size after arming SIGWINCH #5989).daemon/remote/cmd/cmuxd-remote/ws_pty.go): input chunks queued by a superseded attachment were silently dropped at reattach — bytes typed just before a disconnect vanished, and the seam was not quiesced.pty.writeon queue overflow after the bytes were already consumed from local stdin, and the app-sideRemotePTYBridgeSessionclosed the whole session on window overflow — accepted bytes lost either way.pty.writewas fire-and-forget — nothing could detect a gap or reorder.Fix
Sequenced, cumulatively-acked
pty.writewith sender-side windowed flow control, capability-gated for mixed versions:helloadvertisespty.input.seq_ack;pty.attachtakes optionalinput_seq_ack;pty.writetakes optionalseq(strictlylast+1per attachment, gaps rejected with wire-pinnedpty_input_seq_gap→ visiblepty.error+ daemon-side detach, never a silent write; a present-but-malformedseqis rejected withinvalid_params); new coalesced cumulativepty.input_ackevent emitted only to opted-in attachments after bytes hit the PTY fd, with out-of-range acks treated as a protocol violation by the app. Writes stay async notifications — the typing-latency win from 719a231 is preserved.session.inputis FIFO withwriteInputLoopas its only consumer, whole writes enqueue atomically underinputEnqueueMu, andwriteInputChunkno longer drops chunks whose attachment was replaced (the original loss bug). Reattach never waits on that drain, so a wedged PTY (stopped foreground process) cannot hang the attach path; the seam test asserts attach completes while the PTY writer is stalled.RemotePTYBridgeSessiongets a queue-confined flow controller (RemotePTYBridgeInputFlow, new file) that pauses socket receives at the window (4 MiB / 256 writes), splits writes to ≤256 KiB so no RPC frame can exceed the daemon's 4 MiB limit, and resumes on drain — cumulative acks in seq_ack mode, write completions in legacy mode. No accepted byte is ever dropped and the session never closes on overflow, in either mode.Compatibility: the capability is optional (not part of the required handshake set). New app ↔ old daemon falls back to legacy completion-drained windowing; old app ↔ new daemon sees no enforcement and no acks; raw
/terminalwebsocket clients never see ack frames.Two-commit structure (regression policy)
test:) adds only the two RED regression tests, written against main's APIs. Verified failing on main:TestWebSocketPTYReattachWritesAcceptedOldInputBeforeNew: times out — "OLD" bytes dropped at the seam.legacy input overflow pauses instead of closing and preserves order: session closes, 4,182,004 of 5,242,880 bytes delivered.fix:) adds the fix plus the GREEN tests (seq enforcement, seq-gap →pty.error, cumulative/coalesced ack emission, acked-mode flow control,pty.errormid-stream teardown, filter deadline + seeded fuzz with keys before/between/after probe replies including lone ESC, capability/error-code pinning on both sides) and mechanically updates the RED tests to the new signatures.Verification
cd daemon/remote && go test ./cmd/cmuxd-remote/ -count=1✅ (andgo vet ./...; new tests also pass with-race; the one-racefailure,TestPersistentDaemonPTYReattachSurvivesClientDisconnect, reproduces on unmodified main — pre-existing writer-lifetime race, not introduced here)swift test --package-path Packages/macOS/CmuxRemoteWorkspace✅ 51/51swift test --package-path Packages/macOS/CmuxRemoteDaemon✅ 20/20python3 scripts/swift_file_length_budget.py: no TSV touched; all touched files at/under budget (RemotePTYBridgeSession.swift479/506, filter 496/<500,WorkspaceRemoteConnectionTests.swift7312/7312 line-neutral,cmux.swiftuntouched); remaining script failures are pre-existing files this PR does not modify./scripts/lint-pbxproj-test-wiring.sh✅ (newSSHPTYAttachReconnectInputFilterPumpIO.swiftwired into both targets)pty.input.seq_ack,pty_input_seq_gap,pty.input_ack); errors surface through the existing localizedRemotePTYBridgeStringspath. NoLocalizable.xcstringschanges needed.Related (not closed by this PR): #2969, #6082, #6821.
🤖 Generated with Claude Code
Note
High Risk
Touches live SSH stdin forwarding, persistent PTY reconnect semantics, and daemon input ordering—bugs could garble, reorder, or lose keystrokes or hang reattach.
Overview
Adds optional, capability-gated sequenced PTY input end-to-end: daemon advertises
pty.input.seq_ack, attach can opt in withinput_seq_ack,pty.writemay carry monotonicseq(gaps →pty_input_seq_gap, visiblepty.error, detach), andpty.input_ackreports cumulative progress after bytes hit the PTY. Swift RPC/bridge layers plumbinputAck,supportsInputSeqAck, and optionalseqon writes.On the daemon, reattach no longer drops input already accepted from a superseded attachment—
writeInputChunkis session-scoped so queued FIFO input still reaches the PTY (without blocking reattach on a wedged writer). App-sideRemotePTYBridgeInputFlowreplaces naive pending-write counters with windowed flow control: pause socket reads at the cap, split large payloads, resume on write completion (legacy) or cumulative acks (seq mode).SSH reconnect stdin filtering gets a 2s monotonic deadline (poll timeouts capped; EINTR-safe absolute deadlines in extracted pump I/O), flushes pending bytes on pump exit, and
F_SETNOSIGPIPEon stop pipes.Regression tests cover seam ordering, seq enforcement/acks, overflow pause vs close, filter deadline/fuzz, and capability pinning.
Reviewed by Cursor Bugbot for commit 79aa8e8. Bugbot is set up for automated code reviews on this repo. Configure here.