Add terminal output flow control#4896
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR implements byte-level output acknowledgement and backpressure flow control across the terminal pipeline to prevent heap OOM when sustained PTY output outpaces consumer processing. A renderer writes bytes to xterm, sends an ChangesOutput Acknowledgement & Flow Control
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
|
Ready to review this PR? Stage has broken it down into 6 individual chapters for you: 
Chapters generated by Stage for commit 4a0c19a on May 24, 2026 6:47am UTC. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@packages/pty-daemon/src/flow-control/OutputFlowControl.ts`:
- Around line 30-34: The constructor currently only checks ordering between
options.highWatermarkBytes and options.lowWatermarkBytes; update validation in
OutputFlowControl (constructor handling
options.highWatermarkBytes/lowWatermarkBytes) to also reject NaN, non-finite
(Infinity/-Infinity) and non-positive values: validate both values with
Number.isFinite and ensure they are >= 0 (or > 0 per design) before checking
ordering, and throw a clear Error if any value is invalid (include the offending
value and name in the message) so invalid inputs cannot bypass backpressure
guarantees.
In `@packages/pty-daemon/src/handlers/handlers.ts`:
- Around line 154-156: The subscribe handler currently only adds msg.id to
conn.flowControlledSubscriptions when msg.flowControl is true, so resubscribing
with flowControl: false doesn't remove prior membership; make the operation
idempotent by checking msg.flowControl and calling
conn.flowControlledSubscriptions.add(msg.id) when true and
conn.flowControlledSubscriptions.delete(msg.id) when false (referencing
conn.flowControlledSubscriptions and the subscribe message handling that uses
msg.id and msg.flowControl) so re-subscribe toggles flow-control membership
correctly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e1fe9f3-6b7c-4569-a7a9-c81b7f281308
📒 Files selected for processing (25)
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.test.tsapps/desktop/src/renderer/lib/terminal/terminal-ws-transport.tspackages/host-service/src/terminal/DaemonClient/DaemonClient.node-test.tspackages/host-service/src/terminal/DaemonClient/DaemonClient.tspackages/host-service/src/terminal/DaemonClient/index.tspackages/host-service/src/terminal/terminal-output-acks.test.tspackages/host-service/src/terminal/terminal-output-acks.tspackages/host-service/src/terminal/terminal.tspackages/host-service/test/integration/setup-scripts.integration.test.tspackages/host-service/test/integration/teardown.integration.test.tspackages/host-service/test/integration/terminal.integration.test.tspackages/host-service/test/integration/workspace-cleanup.integration.test.tspackages/pty-daemon/package.jsonpackages/pty-daemon/src/Pty/Pty.tspackages/pty-daemon/src/Server/Server.tspackages/pty-daemon/src/SessionStore/SessionStore.test.tspackages/pty-daemon/src/SessionStore/snapshot.test.tspackages/pty-daemon/src/flow-control/OutputFlowControl.tspackages/pty-daemon/src/flow-control/index.tspackages/pty-daemon/src/handlers/handlers.test.tspackages/pty-daemon/src/handlers/handlers.tspackages/pty-daemon/src/protocol/index.tspackages/pty-daemon/src/protocol/messages.tspackages/pty-daemon/test/byte-fidelity.test.tspackages/pty-daemon/test/flow-control.test.ts
Comment on lines
+30
to
+34
| if (options.highWatermarkBytes <= options.lowWatermarkBytes) { | ||
| throw new Error( | ||
| `highWatermarkBytes must be greater than lowWatermarkBytes (${options.highWatermarkBytes} <= ${options.lowWatermarkBytes})`, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Harden watermark option validation to reject non-finite/negative values.
The constructor currently only checks ordering. NaN/Infinity/negative overrides can bypass intended backpressure guarantees and cause incorrect pause/resume behavior.
Proposed fix
export class OutputFlowControl {
@@
constructor(
target: OutputFlowControlTarget,
options: OutputFlowControlOptions = DEFAULT_OUTPUT_FLOW_CONTROL,
) {
this.target = target;
this.options = options;
+ if (
+ !Number.isFinite(options.highWatermarkBytes) ||
+ !Number.isFinite(options.lowWatermarkBytes) ||
+ !Number.isInteger(options.highWatermarkBytes) ||
+ !Number.isInteger(options.lowWatermarkBytes) ||
+ options.highWatermarkBytes <= 0 ||
+ options.lowWatermarkBytes < 0
+ ) {
+ throw new Error(
+ `flow-control watermarks must be finite integers (high > 0, low >= 0): ` +
+ `high=${String(options.highWatermarkBytes)} low=${String(options.lowWatermarkBytes)}`,
+ );
+ }
if (options.highWatermarkBytes <= options.lowWatermarkBytes) {
throw new Error(
`highWatermarkBytes must be greater than lowWatermarkBytes (${options.highWatermarkBytes} <= ${options.lowWatermarkBytes})`,
);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (options.highWatermarkBytes <= options.lowWatermarkBytes) { | |
| throw new Error( | |
| `highWatermarkBytes must be greater than lowWatermarkBytes (${options.highWatermarkBytes} <= ${options.lowWatermarkBytes})`, | |
| ); | |
| } | |
| constructor( | |
| target: OutputFlowControlTarget, | |
| options: OutputFlowControlOptions = DEFAULT_OUTPUT_FLOW_CONTROL, | |
| ) { | |
| this.target = target; | |
| this.options = options; | |
| if ( | |
| !Number.isFinite(options.highWatermarkBytes) || | |
| !Number.isFinite(options.lowWatermarkBytes) || | |
| !Number.isInteger(options.highWatermarkBytes) || | |
| !Number.isInteger(options.lowWatermarkBytes) || | |
| options.highWatermarkBytes <= 0 || | |
| options.lowWatermarkBytes < 0 | |
| ) { | |
| throw new Error( | |
| `flow-control watermarks must be finite integers (high > 0, low >= 0): ` + | |
| `high=${String(options.highWatermarkBytes)} low=${String(options.lowWatermarkBytes)}`, | |
| ); | |
| } | |
| if (options.highWatermarkBytes <= options.lowWatermarkBytes) { | |
| throw new Error( | |
| `highWatermarkBytes must be greater than lowWatermarkBytes (${options.highWatermarkBytes} <= ${options.lowWatermarkBytes})`, | |
| ); | |
| } | |
| } |
🤖 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 `@packages/pty-daemon/src/flow-control/OutputFlowControl.ts` around lines 30 -
34, The constructor currently only checks ordering between
options.highWatermarkBytes and options.lowWatermarkBytes; update validation in
OutputFlowControl (constructor handling
options.highWatermarkBytes/lowWatermarkBytes) to also reject NaN, non-finite
(Infinity/-Infinity) and non-positive values: validate both values with
Number.isFinite and ensure they are >= 0 (or > 0 per design) before checking
ordering, and throw a clear Error if any value is invalid (include the offending
value and name in the message) so invalid inputs cannot bypass backpressure
guarantees.
Comment on lines
+154
to
+156
| if (msg.flowControl) { | ||
| conn.flowControlledSubscriptions.add(msg.id); | ||
| } |
Contributor
There was a problem hiding this comment.
Make subscribe idempotent for flowControl toggles.
If a client re-subscribes an already-subscribed session with flowControl: false, the previous flow-control membership is retained, so ACK gating can remain enabled unexpectedly.
Proposed fix
conn.subscriptions.add(msg.id);
- if (msg.flowControl) {
- conn.flowControlledSubscriptions.add(msg.id);
- }
+ if (msg.flowControl) {
+ conn.flowControlledSubscriptions.add(msg.id);
+ } else {
+ conn.flowControlledSubscriptions.delete(msg.id);
+ }
if (msg.replay) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (msg.flowControl) { | |
| conn.flowControlledSubscriptions.add(msg.id); | |
| } | |
| conn.subscriptions.add(msg.id); | |
| if (msg.flowControl) { | |
| conn.flowControlledSubscriptions.add(msg.id); | |
| } else { | |
| conn.flowControlledSubscriptions.delete(msg.id); | |
| } | |
| if (msg.replay) { |
🤖 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 `@packages/pty-daemon/src/handlers/handlers.ts` around lines 154 - 156, The
subscribe handler currently only adds msg.id to conn.flowControlledSubscriptions
when msg.flowControl is true, so resubscribing with flowControl: false doesn't
remove prior membership; make the operation idempotent by checking
msg.flowControl and calling conn.flowControlledSubscriptions.add(msg.id) when
true and conn.flowControlledSubscriptions.delete(msg.id) when false (referencing
conn.flowControlledSubscriptions and the subscribe message handling that uses
msg.id and msg.flowControl) so re-subscribe toggles flow-control membership
correctly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
Contributor
Greptile SummaryThis PR introduces an ACK-based, high/low-watermark flow-control system for terminal output, preventing OOM scenarios where a slow renderer could allow PTY output to pile up unboundedly through the daemon → host-service → xterm.js pipeline.
Confidence Score: 4/5The core flow-control logic is correct and well-tested; the main concern is a constructor gap that allows The three-layer ACK pipeline (daemon → host-service aggregator → renderer write-callback) is logically sound: offset-based tracking, minimum-across-all-sockets flushing, and disconnect/exit release all work correctly as demonstrated by the deterministic tests. The packages/pty-daemon/src/flow-control/OutputFlowControl.ts — the watermark validation gap; packages/pty-daemon/src/Server/Server.ts — the shared flow-control accumulation model
|
| Filename | Overview |
|---|---|
| packages/pty-daemon/src/flow-control/OutputFlowControl.ts | New PTY-level flow controller: valid high/low watermark logic, but the constructor permits lowWatermarkBytes: 0 which makes the resume condition unacknowledgedBytes < 0 unsatisfiable, permanently stalling the PTY. |
| packages/pty-daemon/src/Server/Server.ts | Flow-control plumbing in the daemon server: ACK handling, per-connection outstanding-byte accounting, and disconnect/exit release paths all look correct. Has the shared-per-session OutputFlowControl design that inflates byte counts for multiple flow-controlled subscribers, and releaseAllFlowControlForConn misses sessions with zero outstanding bytes. |
| packages/host-service/src/terminal/terminal-output-acks.ts | New offset-based ACK aggregator for renderer sockets: correctly starts new sockets at the current live-output position so replay bytes don't generate upstream ACKs, handles socket removal/disconnect release, and the minimum-across-all-sockets flush logic is sound. |
| packages/host-service/src/terminal/terminal.ts | Integrates TerminalOutputAckTracker into the session lifecycle; transformed-chunk handling (shell-ready scanner) uses ackImmediate correctly for the full raw byte count and skips trackLiveOutput, avoiding double-ack. Socket add/remove hooks are consistently applied across all code paths. |
| apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.ts | Renderer-side ACK: sends output-ack only from the xterm write callback, correctly guarding against reconnects with socket identity and readyState checks before sending. |
| packages/pty-daemon/test/flow-control.test.ts | Integration tests for the new flow-control path: covers pause above high-watermark, resume below low-watermark, non-flow-controlled subscriber isolation, and disconnect-triggered release. Deterministic fake PTY with pauseCalls/resumeCalls counters provides clean assertions. |
| packages/host-service/src/terminal/terminal-output-acks.test.ts | Unit tests for TerminalOutputAckTracker: four focused cases covering single-socket live output, multi-socket wait-for-all, disconnect release, and replay-ack isolation. All cases validate the offset-based aggregation logic correctly. |
| packages/host-service/src/terminal/DaemonClient/DaemonClient.ts | Adds ackOutput fire-and-forget method and threads flowControl opt-in through the subscribe message. Input validation (isFinite, > 0, Math.floor) is consistent with the daemon-side guard. |
| packages/pty-daemon/src/Pty/Pty.ts | Adds pause/resume to the Pty interface and implements them for both NodePtyAdapter (via node-pty) and AdoptedPty (via the reader stream). |
Sequence Diagram
sequenceDiagram
participant PTY
participant Daemon as Daemon Server<br/>(OutputFlowControl)
participant HC as Host-Service<br/>(TerminalOutputAckTracker)
participant R1 as Renderer A<br/>(xterm.js)
participant R2 as Renderer B<br/>(xterm.js)
PTY->>Daemon: onData(chunk)
Daemon->>HC: output frame (bytes)
Daemon->>Daemon: track(bytes) — unacked++
Note over Daemon: unacked > highWatermark?<br/>→ PTY.pause()
HC->>R1: send ArrayBuffer
HC->>R2: send ArrayBuffer
HC->>HC: trackLiveOutput(bytes, [R1, R2])
R1->>R1: xterm.write(data, callback)
R1->>HC: "output-ack {bytes: N}"
HC->>HC: ackSocket(R1, N) — min-offset waits for R2
R2->>R2: xterm.write(data, callback)
R2->>HC: "output-ack {bytes: N}"
HC->>HC: ackSocket(R2, N) — all caught up, flushAckable
HC->>Daemon: "ack-output {bytes: N}"
Daemon->>Daemon: acknowledge(N) — unacked--
Note over Daemon: unacked < lowWatermark?<br/>→ PTY.resume()
Note over HC,Daemon: On socket disconnect:
HC->>HC: removeSocket → flushAckable
Note over Daemon: On conn close:
Daemon->>Daemon: releaseAllFlowControlForConn → PTY.resume()
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
packages/pty-daemon/src/flow-control/OutputFlowControl.ts:30-34
**`lowWatermarkBytes: 0` creates permanent PTY stall**
The constructor validates `high > low` but not `low >= 1`. When `lowWatermarkBytes` is 0 (e.g. `flowControl: { highWatermarkBytes: 100_000, lowWatermarkBytes: 0 }`), the resume condition `this.unacknowledgedBytes < 0` is never satisfiable, leaving the PTY permanently paused once it crosses the high watermark. Adding `|| options.lowWatermarkBytes <= 0` to the guard, or changing the resume check to `<=`, would prevent this deadlock.
### Issue 2 of 3
packages/pty-daemon/src/Server/Server.ts:490-501
**Per-session `OutputFlowControl` tracks bytes N times for N flow-controlled subscribers**
`track(bytes)` on the single shared `OutputFlowControl` instance is called once per flow-controlled connection per chunk. With two flow-controlled subscribers, the effective high-watermark halves: a 60 KB chunk with `highWatermarkBytes=100_000` would accumulate 120 KB of "unacknowledged" bytes, triggering a pause that should not fire. In the current architecture only one host-service acts as the sole flow-controlled subscriber, so this is theoretical, but it's worth noting the model breaks if that assumption changes.
### Issue 3 of 3
packages/pty-daemon/src/Server/Server.ts:539-545
**`releaseAllFlowControlForConn` only iterates sessions with outstanding bytes**
If a flow-controlled subscriber disconnects before any output is sent (or after all bytes have already been fully acked), that session's ID is absent from `flowControlOutstandingBytes` even though it is still present in `flowControlledSubscriptions`. `releaseAllFlowControlForConn` iterates only over `flowControlOutstandingBytes.keys()`, so the `flowControlledSubscriptions` entry is never cleaned up. Since the `conn` object itself is removed from `conns` this is harmless today, but any future code that inspects `flowControlledSubscriptions` on a closing connection would see a stale entry.
Reviews (1): Last reviewed commit: "Add terminal output flow control" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+30
to
+34
| if (options.highWatermarkBytes <= options.lowWatermarkBytes) { | ||
| throw new Error( | ||
| `highWatermarkBytes must be greater than lowWatermarkBytes (${options.highWatermarkBytes} <= ${options.lowWatermarkBytes})`, | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
lowWatermarkBytes: 0 creates permanent PTY stall
The constructor validates high > low but not low >= 1. When lowWatermarkBytes is 0 (e.g. flowControl: { highWatermarkBytes: 100_000, lowWatermarkBytes: 0 }), the resume condition this.unacknowledgedBytes < 0 is never satisfiable, leaving the PTY permanently paused once it crosses the high watermark. Adding || options.lowWatermarkBytes <= 0 to the guard, or changing the resume check to <=, would prevent this deadlock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pty-daemon/src/flow-control/OutputFlowControl.ts
Line: 30-34
Comment:
**`lowWatermarkBytes: 0` creates permanent PTY stall**
The constructor validates `high > low` but not `low >= 1`. When `lowWatermarkBytes` is 0 (e.g. `flowControl: { highWatermarkBytes: 100_000, lowWatermarkBytes: 0 }`), the resume condition `this.unacknowledgedBytes < 0` is never satisfiable, leaving the PTY permanently paused once it crosses the high watermark. Adding `|| options.lowWatermarkBytes <= 0` to the guard, or changing the resume check to `<=`, would prevent this deadlock.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+490
to
+501
| private trackFlowControlledOutput( | ||
| conn: ConnState, | ||
| session: Session, | ||
| bytes: number, | ||
| ): void { | ||
| if (bytes <= 0 || !conn.flowControlledSubscriptions.has(session.id)) return; | ||
| conn.flowControlOutstandingBytes.set( | ||
| session.id, | ||
| (conn.flowControlOutstandingBytes.get(session.id) ?? 0) + bytes, | ||
| ); | ||
| this.getFlowControl(session).track(bytes); | ||
| } |
Contributor
There was a problem hiding this comment.
OutputFlowControl tracks bytes N times for N flow-controlled subscribers
track(bytes) on the single shared OutputFlowControl instance is called once per flow-controlled connection per chunk. With two flow-controlled subscribers, the effective high-watermark halves: a 60 KB chunk with highWatermarkBytes=100_000 would accumulate 120 KB of "unacknowledged" bytes, triggering a pause that should not fire. In the current architecture only one host-service acts as the sole flow-controlled subscriber, so this is theoretical, but it's worth noting the model breaks if that assumption changes.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pty-daemon/src/Server/Server.ts
Line: 490-501
Comment:
**Per-session `OutputFlowControl` tracks bytes N times for N flow-controlled subscribers**
`track(bytes)` on the single shared `OutputFlowControl` instance is called once per flow-controlled connection per chunk. With two flow-controlled subscribers, the effective high-watermark halves: a 60 KB chunk with `highWatermarkBytes=100_000` would accumulate 120 KB of "unacknowledged" bytes, triggering a pause that should not fire. In the current architecture only one host-service acts as the sole flow-controlled subscriber, so this is theoretical, but it's worth noting the model breaks if that assumption changes.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+539
to
+545
| private releaseAllFlowControlForConn(conn: ConnState): void { | ||
| for (const sessionId of Array.from( | ||
| conn.flowControlOutstandingBytes.keys(), | ||
| )) { | ||
| this.releaseFlowControlForConn(conn, sessionId); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
releaseAllFlowControlForConn only iterates sessions with outstanding bytes
If a flow-controlled subscriber disconnects before any output is sent (or after all bytes have already been fully acked), that session's ID is absent from flowControlOutstandingBytes even though it is still present in flowControlledSubscriptions. releaseAllFlowControlForConn iterates only over flowControlOutstandingBytes.keys(), so the flowControlledSubscriptions entry is never cleaned up. Since the conn object itself is removed from conns this is harmless today, but any future code that inspects flowControlledSubscriptions on a closing connection would see a stale entry.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/pty-daemon/src/Server/Server.ts
Line: 539-545
Comment:
**`releaseAllFlowControlForConn` only iterates sessions with outstanding bytes**
If a flow-controlled subscriber disconnects before any output is sent (or after all bytes have already been fully acked), that session's ID is absent from `flowControlOutstandingBytes` even though it is still present in `flowControlledSubscriptions`. `releaseAllFlowControlForConn` iterates only over `flowControlOutstandingBytes.keys()`, so the `flowControlledSubscriptions` entry is never cleaned up. Since the `conn` object itself is removed from `conns` this is harmless today, but any future code that inspects `flowControlledSubscriptions` on a closing connection would see a stale entry.
How can I resolve this? If you propose a fix, please make it concise.
Mirrors VS Code's terminal flow control: the renderer counts bytes xterm has actually parsed and ACKs the daemon directly; the daemon pauses the PTY when unacked output exceeds 100KB and resumes once acks drop below 5KB. node-pty.pause() stops reading the master fd, so the shell eventually blocks on write — real kernel-level back- pressure all the way back to the producer. host-service is a dumb relay for the ack frame: no per-socket tracker, no offset accounting. Subscribers opt in via flowControl on subscribe; conn-drop / unsubscribe releases the counter so a crashed client can't leave a PTY paused.
Kitenite
force-pushed
the
debug-issue-4868-testing
branch
from
701fceb to
51636de
Compare
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/pty-daemon/src/handlers/handlers.ts (1)
159-166:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCount replay bytes when flow control is enabled.
When
replayandflowControlare both on, this can sendsnapimmediately but leavesflowControlUnackedat0. The daemon will then ignoreack-outputcredits for that replay, and a reattach/adoption burst can bypass the new back-pressure path until later live output arrives. Seed the counter with the replay size and re-evaluate pause state right after subscribe.🤖 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 `@packages/pty-daemon/src/handlers/handlers.ts` around lines 159 - 166, When handling replay with flow control enabled, the code currently seeds conn.flowControlUnacked with 0 which ignores the bytes already sent from the snapshot; change the logic in handlers.ts so after taking snap = ctx.store.snapshotBuffer(session) and calling conn.send(out, snap) you set conn.flowControlUnacked.set(msg.id, snap.byteLength) (instead of leaving it at 0) and then immediately re-evaluate the connection back-pressure/pause state by calling the connection's pause/evaluation helper (e.g., the existing conn.maybePause or equivalent) so replay bytes count toward flow-control credits.packages/host-service/src/terminal/terminal.ts (1)
1288-1335:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftTrack ACKs in host-service instead of forwarding renderer ACKs 1:1.
The daemon sees host-service as a single flow-controlled subscriber, but this path only credits bytes when a renderer later ACKs them. Bytes already consumed inside host-service—buffered while no socket is attached, filtered by
scanForShellReady, or fanned out to multiple sockets—are never credited correctly. Detached sessions will eventually hit the watermark and stall, and multi-view attaches can over-credit as soon as the fastest socket ACKs. This layer needs per-socket aggregation/release before forwarding credits upstream.Also applies to: 1607-1609
🤖 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 `@packages/host-service/src/terminal/terminal.ts` around lines 1288 - 1335, The onOutput handler attached via daemon.subscribe currently forwards renderer ACKs 1:1, which miscredits bytes consumed locally (buffered, filtered by scanForShellReady, fanned out) and can under- or over-credit the daemon; change the logic in the onOutput handler (the function passed to daemon.subscribe) to maintain per-socket ACK tracking and a session-level credited counter: when bytes are emitted, increment a session.pendingCredit by the number of bytes produced, then when broadcastBytes(session, bytes) returns the number sent to sockets and/or bufferOutput(session, bytes) retains bytes, record per-socket ACKs into a session.socketsAckMap and aggregate released bytes at the session level, deduct locally-consumed bytes (e.g., output swallowed by scanForShellReady, bytes kept in tail ring) from pendingCredit, and only forward the net released credit upstream once (via the existing daemon credit API) using the difference between totalReleased and session.alreadyForwarded; apply the same aggregation/forwarding pattern to the related code paths mentioned (lines ~1607-1609) so the daemon sees a single aggregated credit per session rather than raw renderer ACKs.
🧹 Nitpick comments (2)
packages/pty-daemon/test/flow-control.test.ts (1)
200-205: ⚡ Quick winReplace fixed sleep with bounded polling to reduce test flakiness.
Line 202 uses a hardcoded delay before a single round-trip. Close propagation can occasionally take longer, causing intermittent failures. Polling until
resumeCountchanges (with timeout) is more deterministic.Proposed change
- // Server learns about the close asynchronously; give it a moment, then - // round-trip via the remaining connection to ensure dropConn has run. - await new Promise((r) => setTimeout(r, 20)); - const reply2 = opener.waitForNext((m) => m.type === "list-reply", 1000); - opener.send({ type: "list" }); - await reply2; + // Server learns about the close asynchronously; poll with a bounded + // timeout to avoid fixed-delay flakes. + const deadline = Date.now() + 1000; + while (Date.now() < deadline && pty.resumeCount === 0) { + const reply2 = opener.waitForNext((m) => m.type === "list-reply", 1000); + opener.send({ type: "list" }); + await reply2; + }🤖 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 `@packages/pty-daemon/test/flow-control.test.ts` around lines 200 - 205, Replace the fixed 20ms sleep with a bounded poll that checks for the expected state change (poll until resumeCount increments or a timeout elapses) instead of sleeping; specifically, after triggering the close, repeatedly read the current resumeCount (or the relevant observable/state used by the test) with short intervals and stop when resumeCount changes or when a configurable timeout is reached, then proceed to perform the round-trip using opener.send and await opener.waitForNext("list-reply"); ensure the poll throws/errors on timeout so the test fails deterministically if close propagation never happens.packages/pty-daemon/src/handlers/handlers.test.ts (1)
51-57: ⚡ Quick winAdd a replay-accounting regression test now that the fake conn exposes
flowControlUnacked.The current replay test only checks that bytes are sent. It does not assert that
subscribe({ replay: true, flowControl: true })seeds the unacked counter, so the replay bypass inhandleSubscribe()would not fail this suite.🤖 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 `@packages/pty-daemon/src/handlers/handlers.test.ts` around lines 51 - 57, Update the replay test to assert that the fake connection's flowControlUnacked map is seeded when subscribing with replay and flow control: use makeConn() to build the Conn, call the code path that issues subscribe({ replay: true, flowControl: true }) (the same call that exercises handleSubscribe), then assert that conn.flowControlUnacked.has(<streamId/substring used in test>) is true and that conn.flowControlUnacked.get(<streamId/substring>) equals the expected initial unacked byte count (matching the bytes the test expects to be replayed); reference makeConn, flowControlUnacked, SentFrame, and handleSubscribe to locate where to add the assertion.
🤖 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.
Outside diff comments:
In `@packages/host-service/src/terminal/terminal.ts`:
- Around line 1288-1335: The onOutput handler attached via daemon.subscribe
currently forwards renderer ACKs 1:1, which miscredits bytes consumed locally
(buffered, filtered by scanForShellReady, fanned out) and can under- or
over-credit the daemon; change the logic in the onOutput handler (the function
passed to daemon.subscribe) to maintain per-socket ACK tracking and a
session-level credited counter: when bytes are emitted, increment a
session.pendingCredit by the number of bytes produced, then when
broadcastBytes(session, bytes) returns the number sent to sockets and/or
bufferOutput(session, bytes) retains bytes, record per-socket ACKs into a
session.socketsAckMap and aggregate released bytes at the session level, deduct
locally-consumed bytes (e.g., output swallowed by scanForShellReady, bytes kept
in tail ring) from pendingCredit, and only forward the net released credit
upstream once (via the existing daemon credit API) using the difference between
totalReleased and session.alreadyForwarded; apply the same
aggregation/forwarding pattern to the related code paths mentioned (lines
~1607-1609) so the daemon sees a single aggregated credit per session rather
than raw renderer ACKs.
In `@packages/pty-daemon/src/handlers/handlers.ts`:
- Around line 159-166: When handling replay with flow control enabled, the code
currently seeds conn.flowControlUnacked with 0 which ignores the bytes already
sent from the snapshot; change the logic in handlers.ts so after taking snap =
ctx.store.snapshotBuffer(session) and calling conn.send(out, snap) you set
conn.flowControlUnacked.set(msg.id, snap.byteLength) (instead of leaving it at
0) and then immediately re-evaluate the connection back-pressure/pause state by
calling the connection's pause/evaluation helper (e.g., the existing
conn.maybePause or equivalent) so replay bytes count toward flow-control
credits.
---
Nitpick comments:
In `@packages/pty-daemon/src/handlers/handlers.test.ts`:
- Around line 51-57: Update the replay test to assert that the fake connection's
flowControlUnacked map is seeded when subscribing with replay and flow control:
use makeConn() to build the Conn, call the code path that issues subscribe({
replay: true, flowControl: true }) (the same call that exercises
handleSubscribe), then assert that
conn.flowControlUnacked.has(<streamId/substring used in test>) is true and that
conn.flowControlUnacked.get(<streamId/substring>) equals the expected initial
unacked byte count (matching the bytes the test expects to be replayed);
reference makeConn, flowControlUnacked, SentFrame, and handleSubscribe to locate
where to add the assertion.
In `@packages/pty-daemon/test/flow-control.test.ts`:
- Around line 200-205: Replace the fixed 20ms sleep with a bounded poll that
checks for the expected state change (poll until resumeCount increments or a
timeout elapses) instead of sleeping; specifically, after triggering the close,
repeatedly read the current resumeCount (or the relevant observable/state used
by the test) with short intervals and stop when resumeCount changes or when a
configurable timeout is reached, then proceed to perform the round-trip using
opener.send and await opener.waitForNext("list-reply"); ensure the poll
throws/errors on timeout so the test fails deterministically if close
propagation never happens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6c51ce0b-e72b-4249-abf1-e17c43251baf
📒 Files selected for processing (14)
apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.tspackages/host-service/src/terminal/DaemonClient/DaemonClient.tspackages/host-service/src/terminal/terminal.tspackages/pty-daemon/package.jsonpackages/pty-daemon/src/Pty/Pty.tspackages/pty-daemon/src/Server/Server.tspackages/pty-daemon/src/SessionStore/SessionStore.test.tspackages/pty-daemon/src/SessionStore/snapshot.test.tspackages/pty-daemon/src/handlers/handlers.test.tspackages/pty-daemon/src/handlers/handlers.tspackages/pty-daemon/src/protocol/index.tspackages/pty-daemon/src/protocol/messages.tspackages/pty-daemon/test/byte-fidelity.test.tspackages/pty-daemon/test/flow-control.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/pty-daemon/src/SessionStore/snapshot.test.ts
handleSubscribe was initializing the per-conn unacked counter to 0 before sending the replay payload, so the daemon under-counted in-flight bytes on attach. The renderer ACKs replay bytes (it can't tell them apart from live output), and those ACKs were silently clamped to zero. Effect: a subscriber attaching to a session with e.g. 200KB of ring-buffer scrollback could chew through that plus the full 100KB high watermark of live data before back- pressure kicked in. One-line fix: charge snap.byteLength to the counter right after the replay frame is sent.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/pty-daemon/src/handlers/handlers.ts (1)
160-160:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake
flowControlsubscription idempotent.Re-subscribing with
flowControl: falsedoes not remove the existing counter entry, so the daemon may continue applying backpressure unexpectedly.Proposed fix
- if (msg.flowControl) conn.flowControlUnacked.set(msg.id, 0); + if (msg.flowControl) { + conn.flowControlUnacked.set(msg.id, 0); + } else { + conn.flowControlUnacked.delete(msg.id); + }🤖 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 `@packages/pty-daemon/src/handlers/handlers.ts` at line 160, The flowControl subscription must be idempotent: update the handler so that when msg.flowControl is true you ensure an entry exists for conn.flowControlUnacked with key msg.id (initialize to 0 if missing), and when msg.flowControl is false you remove any existing counter by deleting conn.flowControlUnacked entry for msg.id; adjust the logic around conn.flowControlUnacked, msg.flowControl, and msg.id in the handlers.ts subscribe flow to perform set-if-missing on true and delete on false.
🤖 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.
Duplicate comments:
In `@packages/pty-daemon/src/handlers/handlers.ts`:
- Line 160: The flowControl subscription must be idempotent: update the handler
so that when msg.flowControl is true you ensure an entry exists for
conn.flowControlUnacked with key msg.id (initialize to 0 if missing), and when
msg.flowControl is false you remove any existing counter by deleting
conn.flowControlUnacked entry for msg.id; adjust the logic around
conn.flowControlUnacked, msg.flowControl, and msg.id in the handlers.ts
subscribe flow to perform set-if-missing on true and delete on false.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2117c9e-ba43-4ee2-839d-8e5f8844d822
📒 Files selected for processing (1)
packages/pty-daemon/src/handlers/handlers.ts
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 29, 2026
Already integrated, superseded by current versions, or net-empty in this fork: superset-sh#3881 superset-sh#3887 superset-sh#3917 superset-sh#3925 superset-sh#3940 superset-sh#3956 superset-sh#3961 superset-sh#3974 superset-sh#4017 superset-sh#4048 superset-sh#4049 superset-sh#4055 superset-sh#4063 superset-sh#4070 superset-sh#4092 superset-sh#4110 superset-sh#4138 superset-sh#4159 superset-sh#4163 superset-sh#4164 superset-sh#4209 superset-sh#4210 superset-sh#4249 superset-sh#4349 superset-sh#4405 superset-sh#4462 superset-sh#4464 superset-sh#4494 superset-sh#4495 superset-sh#4500 superset-sh#4535 superset-sh#4541 superset-sh#4566 superset-sh#4580 superset-sh#4589 superset-sh#4593 superset-sh#4603 superset-sh#4637 superset-sh#4642 superset-sh#4655 superset-sh#4657 superset-sh#4659 superset-sh#4685 superset-sh#4692 superset-sh#4745 superset-sh#4789 superset-sh#4797 superset-sh#4824 superset-sh#4835 superset-sh#4847 superset-sh#4885 superset-sh#4896.
3 tasks
AviPeltz
added a commit
that referenced
this pull request
May 31, 2026
…ached Terminal output flow control (#4896) pauses the PTY when the daemon's unacked-bytes counter crosses the high watermark, and only resumes once renderer acks drain it below the low watermark. But the daemon counts every byte it sends to host-service, while the renderer is the only thing that acks — and only for bytes that reach an open socket. When no renderer socket is open (reconnect gap, pre-attach startup burst, or a session running with no pane showing it), output still streams daemon -> host-service, where it lands in the bounded 64 KB replay buffer (older bytes evicted). Those bytes are never delivered to a renderer, so they are never acked. The daemon's counter climbs past the high watermark and the PTY pauses; on re-attach the renderer can only ack the <=64 KB that survived eviction, leaving the counter stuck above the low watermark. The PTY never resumes — the terminal is permanently frozen until the session is recreated. Fix: when broadcast finds no open socket, host-service has still consumed the bytes into its bounded buffer, so credit the daemon's flow-control counter immediately instead of waiting for a renderer ack that will never come. This is the only path that left a terminal permanently wedged. Note: buffered bytes are also acked again when replayed to a reconnecting renderer (it can't distinguish replay from live). The daemon clamps the counter at >= 0, so the double-ack is harmless beyond briefly loosening back-pressure for one re-attach window. Adds a regression test: ~195 KB produced with no socket attached must not leave the PTY flow-control paused.
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.
Summary
Details
The OOM risk in #4868 is that terminal output could continue flowing from the PTY through the daemon/host-service path without backpressure from the renderer. This implements the same ACK + high/low watermark source-pause pattern used by battle-tested terminal stacks such as VS Code and Tabby, adapted to our byte-native daemon protocol. No third-party code was vendored for this change.
Flow-controlled subscribers now opt in with
subscribe.flowControl; the daemon counts output bytes per connection/session and callspause()on the PTY once outstanding bytes exceed the high watermark. Renderer ACKs are sent only after xterm'swritecallback fires, then host-service aggregates ACKs across attached sockets before forwarding upstream. Disconnects/unsubscribes release outstanding bytes so a dead renderer cannot leave the PTY paused.Fixes #4868.
Validation
bun run lintbun run typecheck --filter=@superset/host-servicebun run typecheck --filter=@superset/pty-daemon(cd packages/pty-daemon && bun run test)(cd packages/pty-daemon && bun run test:integration)- 51 tests passed(cd packages/host-service && bun run test:integration:daemon)- 19 tests passed(cd packages/host-service && bun run test:e2e)- 12 tests passedbun test packages/host-service/test/integration/terminal.integration.test.ts packages/host-service/src/terminal/terminal-mode-tracker.test.ts packages/host-service/src/events/event-bus.test.tsbun test packages/host-service/src/terminal/terminal-output-acks.test.ts apps/desktop/src/renderer/lib/terminal/terminal-ws-transport.test.tsnode --experimental-strip-types --test packages/pty-daemon/test/flow-control.test.ts packages/host-service/src/terminal/DaemonClient/DaemonClient.node-test.tsAdditional E2E/Data Notes
I also looked for an existing checked-in slow-renderer/OOM process-monitoring harness for the new daemon -> host-service -> desktop terminal path and did not find one. The closest data we can collect without adding a new long-running soak test is the deterministic flow-control fixture proving source pause/resume and release-on-disconnect, plus the daemon supervisor and host-service terminal E2E suites above.
Summary by cubic
Adds end-to-end terminal output flow control to prevent runaway PTY output and OOM when the renderer lags. Uses byte ACKs with 100 kB/5 kB watermarks to pause/resume the PTY at the daemon; desktop acks only after
xtermconsumes bytes (fixes #4868).New Features
@superset/pty-daemon): opt-insubscribe.flowControlandack-output; tracks unacked bytes per connection; pauses at 100 kB and resumes below 5 kB; clears counters on unsubscribe/disconnect; implements PTYpause()/resume().@superset/host-service): enables flow control on subscribe and relays rendereroutput-ackto the daemon; no per-socket aggregation.output-ackafterxterm.writecallback to ack only consumed bytes.Bug Fixes
@superset/pty-daemon): counts replayed output bytes toward flow-control so back-pressure applies immediately after attach.Written for commit 4a0c19a. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
Tests