Skip to content

fix: solve #2968 — skip writes to backpressured terminal sockets#2969

Closed
github-actions[bot] wants to merge 1 commit into
mainfrom
triage/issue-2968-23694985615
Closed

fix: solve #2968 — skip writes to backpressured terminal sockets#2969
github-actions[bot] wants to merge 1 commit into
mainfrom
triage/issue-2968-23694985615

Conversation

@github-actions
Copy link
Copy Markdown
Contributor

@github-actions github-actions Bot commented Mar 28, 2026

Summary

  • Root cause: broadcastEvent in the terminal host session continued calling socket.write() on client sockets that were already backpressured (i.e., socket.write() had previously returned false). This grew Node's internal write buffer without bound. When the socket finally drained, the massive accumulated buffer flushed to the client all at once, causing the visible freeze/catch-up stall reported in the issue — especially for high-repaint TUIs like Droid.
  • Fix: Skip socket.write() for sockets already in the clientSocketsWaitingForDrain set. The terminal emulator still processes all data (preserving snapshot state), so the next TUI repaint naturally resyncs the display. When the socket drains and subprocess stdout resumes, new data flows normally without a flood of stale buffered data.
  • Tests: Added two tests covering the backpressure behavior: one verifying writes are skipped during backpressure, and one verifying writes resume after drain.

Test plan

  • bun test apps/desktop/src/main/terminal-host/session.test.ts — all 6 tests pass
  • Manual: open Droid TUI in Superset terminal, switch tabs/workspaces, switch back — terminal should recover immediately without freeze

Closes #2968


Summary by cubic

Skip writes to backpressured terminal sockets to prevent buffer growth and UI freezes in high-repaint TUIs. Addresses #2968 by letting the terminal recover immediately after drain.

  • Bug Fixes
    • In broadcastEvent, skip socket.write() for sockets in clientSocketsWaitingForDrain.
    • Prevents unbounded Node write buffer and catch-up stalls; emulator still processes data and next repaint resyncs.
    • Added tests for skipping during backpressure and resuming after drain.

Written for commit 5c7a881. Summary will update on new commits.

When a client socket's write buffer was full (socket.write() returned
false), broadcastEvent continued writing to it on subsequent data
frames. This grew Node's internal buffer without bound. When the socket
finally drained, the massive buffer flushed at once, causing a visible
freeze/catch-up stall — especially for high-repaint TUIs like Droid.

Fix: skip socket.write() for sockets already in the drain-wait set.
The terminal emulator still processes all data (preserving snapshot
state), so the next TUI repaint naturally resyncs the display.

Closes #2968
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="apps/desktop/src/main/terminal-host/session.ts">

<violation number="1" location="apps/desktop/src/main/terminal-host/session.ts:1074">
P1: The backpressure skip check drops `exit`/`error` events as well as `data`. Restrict skipping to `data` events so clients still receive terminal lifecycle/error notifications.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Comment on lines +1074 to +1076
if (this.clientSocketsWaitingForDrain.has(socket)) {
continue;
}
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot Mar 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: The backpressure skip check drops exit/error events as well as data. Restrict skipping to data events so clients still receive terminal lifecycle/error notifications.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/terminal-host/session.ts, line 1074:

<comment>The backpressure skip check drops `exit`/`error` events as well as `data`. Restrict skipping to `data` events so clients still receive terminal lifecycle/error notifications.</comment>

<file context>
@@ -1065,10 +1065,18 @@ export class Session {
+				// buffer flush causes a visible freeze / catch-up stall (#2968).
+				// The data is still processed by the emulator, so snapshot state
+				// stays consistent and the next TUI repaint naturally resyncs.
+				if (this.clientSocketsWaitingForDrain.has(socket)) {
+					continue;
+				}
</file context>
Suggested change
if (this.clientSocketsWaitingForDrain.has(socket)) {
continue;
}
if (
eventType === "data" &&
this.clientSocketsWaitingForDrain.has(socket)
) {
continue;
}
Fix with Cubic

Kitenite added a commit that referenced this pull request Mar 29, 2026
…limit warnings

Combines the fixes from #2969 and #2962 into a single changeset:

1. Skip writes to backpressured sockets (#2969): When a client socket
   signals backpressure (write returns false), subsequent broadcastEvent
   calls skip that socket entirely instead of growing Node's internal
   write buffer without bound. The terminal emulator still processes all
   data so snapshot state stays consistent — the next TUI repaint after
   drain naturally resyncs the display.

2. Rate-limit backpressure warnings (#2962): Replace unbounded
   console.warn on every backpressure event with a rate-limited
   warnBackpressure() method. Only one warning is emitted per 5-second
   window; subsequent occurrences are counted and reported in the next
   warning (e.g. '247 similar warnings suppressed'). Under sustained
   high-output commands, a single pane could previously generate 400k+
   identical warnings flooding the daemon log.

Tests cover: writes skipped during backpressure, writes resume after
drain, warning rate-limiting within the 5s window, and suppressed
count reporting after the window elapses.

Closes #2969
Closes #2962

Co-Authored-By: Mastra Code (anthropic/claude-opus-4-6) <noreply@mastra.ai>
Kitenite added a commit that referenced this pull request Mar 29, 2026
…limit warnings

Combines the fixes from #2969 and #2962 into a single changeset:

1. Skip writes to backpressured sockets (#2969): When a client socket
   signals backpressure (write returns false), subsequent broadcastEvent
   calls skip that socket entirely instead of growing Node's internal
   write buffer without bound. The terminal emulator still processes all
   data so snapshot state stays consistent — the next TUI repaint after
   drain naturally resyncs the display.

2. Rate-limit backpressure warnings (#2962): Replace unbounded
   console.warn on every backpressure event with a rate-limited
   warnBackpressure() method. Only one warning is emitted per 5-second
   window; subsequent occurrences are counted and reported in the next
   warning (e.g. '247 similar warnings suppressed'). Under sustained
   high-output commands, a single pane could previously generate 400k+
   identical warnings flooding the daemon log.

Tests cover: writes skipped during backpressure, writes resume after
drain, warning rate-limiting within the 5s window, and suppressed
count reporting after the window elapses.

Closes #2969
Closes #2962

Co-Authored-By: Mastra Code (anthropic/claude-opus-4-6) <noreply@mastra.ai>
@Kitenite Kitenite closed this in 8ff299c Mar 30, 2026
siarhei-belavus pushed a commit to siarhei-belavus/localset that referenced this pull request Mar 30, 2026
* fix(desktop): skip writes to backpressured terminal sockets and rate-limit warnings

Combines the fixes from superset-sh#2969 and superset-sh#2962 into a single changeset:

1. Skip writes to backpressured sockets (superset-sh#2969): When a client socket
   signals backpressure (write returns false), subsequent broadcastEvent
   calls skip that socket entirely instead of growing Node's internal
   write buffer without bound. The terminal emulator still processes all
   data so snapshot state stays consistent — the next TUI repaint after
   drain naturally resyncs the display.

2. Rate-limit backpressure warnings (superset-sh#2962): Replace unbounded
   console.warn on every backpressure event with a rate-limited
   warnBackpressure() method. Only one warning is emitted per 5-second
   window; subsequent occurrences are counted and reported in the next
   warning (e.g. '247 similar warnings suppressed'). Under sustained
   high-output commands, a single pane could previously generate 400k+
   identical warnings flooding the daemon log.

Tests cover: writes skipped during backpressure, writes resume after
drain, warning rate-limiting within the 5s window, and suppressed
count reporting after the window elapses.

Closes superset-sh#2969
Closes superset-sh#2962

Co-Authored-By: Mastra Code (anthropic/claude-opus-4-6) <noreply@mastra.ai>

* fix(desktop): preserve terminal lifecycle events under backpressure

* fix(desktop): preserve terminal output under backpressure

* test(desktop): trim terminal host session coverage

* fix(desktop): remove terminal backpressure warning

---------

Co-authored-by: Mastra Code (anthropic/claude-opus-4-6) <noreply@mastra.ai>
(cherry picked from commit 8ff299c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[bug] Desktop terminal temporarily freezes after tab/workspace switch with Droid TUI

0 participants