fix(remote): resync remote terminal on dropped output frames#8059
fix(remote): resync remote terminal on dropped output frames#8059Jinwoo-H wants to merge 1 commit into
Conversation
The multiplex output path silently drops Output frames when the websocket buffer exceeds MAX_BINARY_BUFFERED_AMOUNT (encryptedBinaryReply returns false). The client never noticed the gap, so a congested link (fast PTY over slow/SSH transport) permanently corrupted the remote terminal with missing/garbled output and no error. Output frame seq is a byte high-water, so a drop is self-describing on the wire. The client now tracks the expected next offset and, on a gap, discards the corrupt tail and pulls a fresh authoritative snapshot (self-healing, no user-facing error). Server-side, seq-less Output chunks now carry the sentinel 0 instead of the control-frame cursor so they don't poison the client's gap detector, and sendFrame propagates the send/drop result end-to-end. Adds an end-to-end repro test driving the real client multiplexer through a fake backpressured transport. Co-authored-by: Orca <help@stably.ai>
📝 WalkthroughWalkthroughTerminal multiplex streaming now accepts and propagates binary-send success results, returns failure when streams are closed, and assigns sequence values differently for output and control frames. The remote terminal multiplexer tracks expected output sequences, detects gaps, requests recovery snapshots, and suppresses output during resynchronization. New integration tests simulate dropped and contiguous frames to verify recovery behavior. Existing terminal tests were structurally reformatted without changing their assertions or control flow. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/runtime/rpc/methods/terminal.ts (1)
1234-1237: 🩺 Stability & Availability | 🔵 TrivialConsider logging/counting dropped
sendBinaryframes for observability.
sendFrame's boolean result is computed but never consumed here, so a dropped Output frame due to backpressure leaves no server-side signal (only the client can infer it via seq gaps). A lightweight counter/log onsent === falsewould help diagnose how often backpressure drops occur in production.src/renderer/src/runtime/remote-runtime-terminal-frame-drop-resync.test.ts (1)
81-95: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a multi-byte/non-ASCII output case to catch byte-vs-code-unit offset bugs.
All fixtures here (
'aaa','bbb','ccc', etc.) are pure ASCII, sotext.lengthhappens to equal the UTF-8 byte length and can't expose a byte-offset vs. string-length mismatch. Given the companion critical finding inremote-runtime-terminal-multiplexer.ts(gap detection usesdata.lengthinstead offrame.payload.byteLength), a test case with a multi-byte character (e.g. an emoji or accented character) straddling a drop would directly validate the fix and prevent regression.Also applies to: 142-160
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 0b14e577-928e-4dcd-aa95-2b673286604d
📒 Files selected for processing (8)
src/main/runtime/rpc/dispatcher.tssrc/main/runtime/rpc/methods/terminal.tssrc/main/runtime/rpc/terminal-multiplex-escape-tail.test.tssrc/main/runtime/rpc/terminal-multiplex.test.tssrc/main/runtime/rpc/terminal-output-batching.test.tssrc/main/runtime/rpc/terminal-subscribe-buffer.test.tssrc/renderer/src/runtime/remote-runtime-terminal-frame-drop-resync.test.tssrc/renderer/src/runtime/remote-runtime-terminal-multiplexer.ts
| // Why: the snapshot is the new authoritative byte high-water; align the | ||
| // gap detector to it and re-open the live path (used by both the initial | ||
| // snapshot and a frame-drop resync, which reuses the 'initial' target). | ||
| if (target === 'initial') { | ||
| stream.expectedSeq = typeof info?.seq === 'number' ? info.seq : undefined | ||
| stream.resyncInFlight = false | ||
| stream.initialSnapshotReceived = true | ||
| stream.callbacks.onSubscribed?.() | ||
| } | ||
| return | ||
| } | ||
| if (frame.opcode === TerminalStreamOpcode.Error) { | ||
| clearSnapshot(stream) | ||
| // Why: a failed resync must re-open the live path or output stalls forever. | ||
| stream.resyncInFlight = false |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -A15 'const sendRequestedSnapshot' src/main/runtime/rpc/methods/terminal.ts
rg -n 'pendingSnapshotRequest|resyncInFlight|snapshotTarget' src/renderer/src/runtime/remote-runtime-terminal-multiplexer.tsRepository: stablyai/orca
Length of output: 2287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- remote-runtime-terminal-multiplexer.ts (snapshot/resync flow) ---'
sed -n '340,520p' src/renderer/src/runtime/remote-runtime-terminal-multiplexer.ts
echo
echo '--- terminal.ts (requested snapshot server path) ---'
sed -n '1346,1415p' src/main/runtime/rpc/methods/terminal.tsRepository: stablyai/orca
Length of output: 10007
Resync can be misclassified by an in-flight manual snapshot
src/renderer/src/runtime/remote-runtime-terminal-multiplexer.ts:365, 462-481 — a gap-triggered SnapshotRequest has no requestId, so if pendingSnapshotRequest is already set, its SnapshotEnd is routed as a normal request response instead of the resync. That resolves the wrong promise and leaves resyncInFlight stuck true, so live output keeps being dropped. Prioritize resyncInFlight in snapshotTarget, or prevent resync while a manual snapshot is pending.
1 participant
Summary
Remote terminals over a congested link (fast PTY such as a build log or
yesover a slow/SSH transport) could silently corrupt — missing or garbled output with no error and no recovery.Root cause: the binary multiplex output path drops
Outputframes when the websocket send buffer exceedsMAX_BINARY_BUFFERED_AMOUNT(8 MiB) —encryptedBinaryReplyreturnsfalseand the frame is never sent. The server ignored that result and kept producing, and the client had no gap check, so a dropped frame vanished forever.Fix (self-healing, no scary UI):
remote-runtime-terminal-multiplexer.ts):Outputseqis a byte high-water, so a drop is self-describing on the wire. The client now tracks the expected next offset per stream. On a gap (seq - rawLength > expectedSeq) it discards the corrupt tail, suppresses live output, and requests a fresh authoritative snapshot (reusing the existing initial-snapshot machinery) that fully re-syncs the terminal.expectedSeqre-aligns to the snapshot's seq.methods/terminal.ts): seq-lessOutputchunks (intermediate split chunks) now carry the sentinelseq: 0(== "no seq") instead of the control-framecursor++value, which would otherwise poison the client's gap detector.sendFramenow returns the send/drop result and the boolean is propagated end-to-end (dispatcher.tssendBinarytype).Cross-version compatibility
seq > 0chunks as gap candidates and never lowersexpectedSeq, so a stray small cursor can't trigger a false resync. Real drops that jump seq forward are still caught.Screenshots
No visual change.
Testing
pnpm typecheck(green)pnpm testfor the affected suites:terminal-multiplex,terminal-multiplex-escape-tail,terminal-output-batching,terminal-subscribe-buffer,runtime-terminal-stream,terminal-stream-protocol, and the new repro — all pass.remote-runtime-terminal-frame-drop-resync.test.ts) that drives the real client multiplexer through a fake backpressured transport.Pre-fix (repro fails on
main):Post-fix:
AI Review Report
Reviewed the seq semantics end-to-end (byte high-water vs. control-frame cursor), the resync state machine (no stuck
resyncInFlighton error/failed-send paths), and interaction with in-flight userserializeBuffersnapshot requests (guarded by theresyncInFlightlatch and independent requestId matching). Cross-platform: change is pure transport/protocol logic (no paths, shells, shortcuts, or Electron platform APIs), so macOS/Linux/Windows behavior is identical; this is squarely the SSH/remote path. No new lint suppressions; nomax-linesbumps; test file named after the behavior.Security Audit
No new input handling, command execution, path handling, auth, secrets, or dependency surface. The resync request reuses the existing authenticated
SnapshotRequestopcode over the already-encrypted e2ee channel; the byte high-waterseqis bounded and only used to compare offsets. Snapshot byte budgets and the existing 2 MiB client cap remain in force, so a malicious/buggy server can't force unbounded buffering via resyncs.Notes
Deliberately out of scope: the server does not additionally pause per-stream enqueue while over the buffer cap (the task listed this as optional). The client-driven resync is the authoritative self-healing mechanism and works across the wire without a server-side drain callback; adding producer-side pausing is a separable optimization and can follow up if backpressure churn proves costly.
Made with Orca 🐋