Skip to content

Fix paired remote terminal parse backpressure - #9683

Merged
Jinwoo-H merged 2 commits into
mainfrom
OrcaWin/remote-terminal-attach-freeze-corruption
Jul 22, 2026
Merged

Jinwoo-H merged 2 commits into
mainfrom
OrcaWin/remote-terminal-attach-freeze-corruption

Conversation

@OrcaWin

@OrcaWin OrcaWin commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Fix paired desktop terminals that could render stale/blank screens or corrupted full-screen TUI frames while the host PTY remained healthy.
  • Defer ACK credit for remote terminal output until xterm parses or intentionally discards the frame, so per-viewer server backpressure reflects real renderer progress.
  • Preserve ACK credit across snapshot/replay barriers and settle it on stale-generation removal, bounded-queue eviction, replay failure, or pane disposal.
  • Share the existing local and remote delivery-credit contract through a neutral renderer module.

The affected live Windows-hosted terminal remained responsive through the canonical stream while the paired Mac renderer diverged. The root cause was client-side receipt-time ACK, not the Windows PTY or the AttachConsole work in #9638.

Screenshots

No visual change. This is a terminal stream convergence and backpressure fix.

Testing

  • pnpm lint — ordinary oxlint passed; the repository-wide switch-exhaustiveness phase is blocked by two unrelated existing errors in skill-freshness-group.tsx and daemon-server.ts.
  • pnpm typecheck
  • pnpm test — full repository suite not run; focused suites below passed.
  • pnpm build — the Electron E2E global setup completed electron-vite build --mode e2e; the standalone production build was not run.
  • Added or updated high-quality tests that would catch regressions.

Focused evidence:

  • New deterministic remote-frame -> real renderer scheduler regression fails with receipt-time ACK and passes only after the controlled xterm parse callback completes. It includes alternate-screen, synchronized-output, clear-screen, ANSI color, emoji, and wide Unicode.
  • 13 terminal protocol/replay/backpressure test files: 651 tests passed.
  • SSH/remote reattach reliability gate: 575 tests passed.
  • Main renderer-backpressure gate: 312 tests passed.
  • Electron hidden/full-screen TUI E2E: 4 tests passed, including synchronized TUI restore and newer live output after replay.
  • Formatting, scoped lint, max-lines ratchet, reliability-gate manifest, and git diff --check passed.

AI Review Report

Ran two adversarial review/fix passes anchored to origin/main, with the internal elegance, performance, and terminal-session-reliability contracts.

The review checked:

  • ACK settlement on parse, discard, malformed frames, resync, stale generations, replay failure, queue eviction, stream close, and pane disposal.
  • Snapshot/live ordering, sequence gaps, transformed spans, UTF-16 sequence units versus UTF-8 transport bytes, split escape tails, alternate screen, synchronized output, and Unicode.
  • Per-stream and total server budgets, stalled-client snapshot recovery, and isolation of a second viewer.
  • Viewport ownership: passive viewers do not resize the canonical PTY; explicit ClaimViewport remains the desktop driver policy with legacy Resize fallback.
  • Remote ownership, reconnect, hidden/revealed panes, renderer disposal/recreation, and mixed-version capability behavior.
  • Cross-platform compatibility for macOS, Linux, Windows, SSH, WSL, and remote-runtime paths. No shortcut, label, path, shell-command, or OS-specific behavior is introduced. Mobile streams remain on the existing non-desktop ACK path.

Findings addressed:

  1. Live-frame credit retained behind a replay could leak if xterm stalled forever and pane disposal prevented the replay promise from reaching finally. Disposal now settles those credits explicitly.
  2. The first implementation made the runtime multiplexer import a component-local PTY ACK module. The generic credit context was extracted to terminal-delivery-credit.ts, leaving the local IPC gate as a thin adapter.

The second review found no remaining in-scope issues.

Security Audit

  • No command execution, filesystem/path handling, authentication, secrets, dependencies, or public IPC surface was added.
  • Existing remote frame validation, stream identity, sequence-gap detection, snapshot limits, and malformed-span recovery remain intact.
  • ACK callbacks carry only byte counts already tracked by the multiplex stream; terminal payloads are not persisted or added to diagnostics.
  • Delivery state is bounded by existing server and replay queue limits and is released on success, failure, close, replacement, and disposal.
  • No follow-up security work is required.

Notes

  • Production code changes are renderer-side. The remote server already had per-viewer ACK budgets and authoritative snapshot recovery; premature client ACKs prevented those controls from observing real pressure.
  • Applies to ACK-capable paired desktop terminal viewers regardless of host OS, including remote-hosted SSH/WSL terminals viewed through pairing.
  • Direct host terminals, ordinary local PTYs, and mobile viewers are behaviorally unchanged.
  • Older runtimes that ignore the ACK capability continue to degrade through the existing mixed-version path.
  • A patched packaged client was not deployed into the reporter live Mac/Windows pair; that session was inspected read-only and not reset or destroyed.

@coderabbitai

coderabbitai Bot commented Jul 21, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 347b3184-b607-42a6-b4c9-d6d8347f03f9

📥 Commits

Reviewing files that changed from the base of the PR and between 97a3834 and e57b025.

📒 Files selected for processing (2)
  • src/renderer/src/lib/pane-manager/terminal-delivery-credit.test.ts
  • src/renderer/src/lib/pane-manager/terminal-delivery-credit.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/renderer/src/lib/pane-manager/terminal-delivery-credit.test.ts
  • src/renderer/src/lib/pane-manager/terminal-delivery-credit.ts

📝 Walkthrough

Walkthrough

Adds shared deferred terminal-delivery credit tracking with nested restoration and idempotent settlement. PTY acknowledgment handling, remote terminal output delivery, and reattach live-data queues now use the shared mechanism, releasing credits for consumed, discarded, skipped, and disposed data. Adds inspection helpers for processed PTY character totals and tests for nested credits, replay deferral, disposal, and remote output backpressure.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and accurately summarizes the main change: remote terminal parse backpressure handling.
Description check ✅ Passed The description matches the required template with Summary, Screenshots, Testing, AI Review Report, Security Audit, and Notes sections.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 1


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e8293b6a-fa0b-48f8-a403-4e20d5348cc8

📥 Commits

Reviewing files that changed from the base of the PR and between e2e3f82 and 97a3834.

📒 Files selected for processing (7)
  • src/renderer/src/components/terminal-pane/pty-connection.test.ts
  • src/renderer/src/components/terminal-pane/pty-connection.ts
  • src/renderer/src/components/terminal-pane/terminal-pty-ack-gate.ts
  • src/renderer/src/lib/pane-manager/terminal-delivery-credit.test.ts
  • src/renderer/src/lib/pane-manager/terminal-delivery-credit.ts
  • src/renderer/src/runtime/remote-runtime-terminal-multiplexer.ts
  • src/renderer/src/runtime/remote-runtime-terminal-parse-backpressure.test.ts

@AmethystLiang AmethystLiang added the orca-remote-server Remote Orca Server / orca serve / paired remote runtime label Jul 22, 2026
@Jinwoo-H
Jinwoo-H merged commit b606117 into main Jul 22, 2026
6 checks passed
enrick-dev pushed a commit to enrick-dev/orca that referenced this pull request Oct 15, 2026
* fix(terminal): defer remote output ACKs until parse

* test(terminal): document synchronous credit claims

---------

Co-authored-by: OrcaWin <293788423+OrcaWin@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

orca-remote-server Remote Orca Server / orca serve / paired remote runtime

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants