fix(desktop): remove terminal backpressure warning#2996
Merged
Conversation
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRemoved a warning log emitted when a client socket's write returned false in the terminal host session; backpressure handling via pause/resume and drain events remains unchanged. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/main/terminal-host/session.test.ts (3)
296-305: IncompleteAttachedClientstructure injected.The test directly injects into
attachedClientsbut only includes{ socket }. The actualAttachedClientinterface requiresattachedAtandattachTokenfields. While this works for the currentbroadcastEvent()implementation (which only usessocket), it may cause issues if future code accesses other fields.♻️ Suggested improvement for test robustness
// Directly inject the socket as an attached client ( session as unknown as { attachedClients: Map< import("node:net").Socket, - { socket: import("node:net").Socket } + { socket: import("node:net").Socket; attachedAt: number; attachToken: symbol } >; } - ).attachedClients.set(fakeSocket, { socket: fakeSocket }); + ).attachedClients.set(fakeSocket, { + socket: fakeSocket, + attachedAt: Date.now(), + attachToken: Symbol("test-attach"), + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.test.ts` around lines 296 - 305, The test injects an incomplete AttachedClient into session.attachedClients — it only sets { socket } but the real AttachedClient type also includes attachedAt and attachToken; update the injected value in the test for the Map used by attachedClients (referencing attachedClients, AttachedClient, and broadcastEvent()) to include valid attachedAt (e.g., a Date or timestamp) and attachToken (e.g., a test token string) so the object matches the real interface and prevents future breaks if other fields are accessed.
367-382: Test primarily verifies skip behavior, not rate-limiting.Since
writeFnalways returnsfalse, the socket becomes backpressured on the first write. Subsequent broadcasts skip the socket entirely (due to theclientSocketsWaitingForDraincheck), sowarnBackpressure()is only called once. The test passes, but it doesn't directly exercise the rate-limiting logic—only the skip logic. The next test ("includes suppressed count") properly tests rate-limiting by draining between broadcasts.Consider updating the test name or comment to clarify what behavior it actually validates:
- it("emits only one backpressure warning within the rate-limit window", () => { + it("emits only one backpressure warning when socket stays backpressured", () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.test.ts` around lines 367 - 382, The test currently uses createSessionWithSocket(() => false) so writeFn always returns false, causing the socket to be marked in clientSocketsWaitingForDrain and subsequent broadcasts are skipped—so the test actually verifies the skip behavior rather than rate-limiting; rename the test (or update its description/comment) from "emits only one backpressure warning within the rate-limit window" to something like "emits one backpressure warning when socket is marked waiting-for-drain (skip behavior)" and/or adjust the setup to exercise rate-limiting by using a writeFn that allows multiple writes/drains (e.g., a function that returns false once then true or toggles) so warnBackpressure and rate-limit logic are exercised; refer to createSessionWithSocket, writeFn, clientSocketsWaitingForDrain, warnBackpressure, and broadcast when making the change.
237-244: Consider saving and restoring the originalconsole.warn.The current approach replaces
console.warnwithout saving the original. Whilemock()in Bun returns a spy, it doesn't inherently supportmockRestore()to restore the original function. If tests run in sequence, this could affect subsequent tests.♻️ Suggested improvement
describe("Terminal Host Session backpressure", () => { let warnSpy: ReturnType<typeof mock>; + let originalWarn: typeof console.warn; beforeEach(() => { + originalWarn = console.warn; warnSpy = mock(); console.warn = warnSpy; }); afterEach(() => { - warnSpy.mockRestore?.(); + console.warn = originalWarn; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.test.ts` around lines 237 - 244, The tests overwrite console.warn without saving the original, which can leak state between tests; update the beforeEach/afterEach setup to save the original console.warn into a variable (e.g., originalWarn) before assigning warnSpy = mock() and restoring originalWarn in afterEach instead of calling warnSpy.mockRestore; reference the existing beforeEach, afterEach, warnSpy, console.warn and mock() to locate and change the setup so the real console.warn is preserved and reliably restored after each test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/terminal-host/session.test.ts`:
- Around line 296-305: The test injects an incomplete AttachedClient into
session.attachedClients — it only sets { socket } but the real AttachedClient
type also includes attachedAt and attachToken; update the injected value in the
test for the Map used by attachedClients (referencing attachedClients,
AttachedClient, and broadcastEvent()) to include valid attachedAt (e.g., a Date
or timestamp) and attachToken (e.g., a test token string) so the object matches
the real interface and prevents future breaks if other fields are accessed.
- Around line 367-382: The test currently uses createSessionWithSocket(() =>
false) so writeFn always returns false, causing the socket to be marked in
clientSocketsWaitingForDrain and subsequent broadcasts are skipped—so the test
actually verifies the skip behavior rather than rate-limiting; rename the test
(or update its description/comment) from "emits only one backpressure warning
within the rate-limit window" to something like "emits one backpressure warning
when socket is marked waiting-for-drain (skip behavior)" and/or adjust the setup
to exercise rate-limiting by using a writeFn that allows multiple writes/drains
(e.g., a function that returns false once then true or toggles) so
warnBackpressure and rate-limit logic are exercised; refer to
createSessionWithSocket, writeFn, clientSocketsWaitingForDrain,
warnBackpressure, and broadcast when making the change.
- Around line 237-244: The tests overwrite console.warn without saving the
original, which can leak state between tests; update the beforeEach/afterEach
setup to save the original console.warn into a variable (e.g., originalWarn)
before assigning warnSpy = mock() and restoring originalWarn in afterEach
instead of calling warnSpy.mockRestore; reference the existing beforeEach,
afterEach, warnSpy, console.warn and mock() to locate and change the setup so
the real console.warn is preserved and reliably restored after each test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bd58080-b539-4c40-b791-6e5768fa4d7b
📒 Files selected for processing (2)
apps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/session.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
2 issues 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.test.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/session.test.ts:238">
P2: `console.warn` is globally overwritten in `beforeEach` but not actually restored, causing cross-test global state leakage. Use a method spy (`mock.method`) or explicitly restore the original `console.warn`.</violation>
</file>
<file name="apps/desktop/src/main/terminal-host/session.ts">
<violation number="1" location="apps/desktop/src/main/terminal-host/session.ts:1081">
P1: Skipping backpressured sockets for every event type can drop critical `exit`/`error` events. Restrict this skip to `data` events so lifecycle/error notifications are still delivered.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…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
force-pushed
the
fix/terminal-backpressure-combined
branch
from
e377392 to
d98affb
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/terminal-host/session.ts (1)
136-137: Consider resetting backpressure warning state inresetProcessState().The
backpressureWarnLastAtandbackpressureWarnSuppressedfields are not reset when the process state is reset (lines 984-1013). If a session's subprocess exits and respawns, stale warning state from the previous subprocess could affect warning behavior for the new one.This is minor since the 5-second window is short and the state would naturally expire, but for consistency with other state fields it may be worth resetting.
♻️ Proposed fix
private resetProcessState(): void { this.subprocess = null; this.subprocessReady = false; this.subprocessDecoder = null; + this.backpressureWarnLastAt = 0; + this.backpressureWarnSuppressed = 0; const shellName = this.shell.split("/").pop() || this.shell;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.ts` around lines 136 - 137, Reset the backpressure warning state inside resetProcessState(): clear the fields backpressureWarnLastAt and backpressureWarnSuppressed when resetting process state so stale values from a previous subprocess don't affect a respawned one; locate resetProcessState() in session.ts and add assignments to set backpressureWarnLastAt = 0 and backpressureWarnSuppressed = 0 alongside the other state resets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/terminal-host/session.ts`:
- Around line 136-137: Reset the backpressure warning state inside
resetProcessState(): clear the fields backpressureWarnLastAt and
backpressureWarnSuppressed when resetting process state so stale values from a
previous subprocess don't affect a respawned one; locate resetProcessState() in
session.ts and add assignments to set backpressureWarnLastAt = 0 and
backpressureWarnSuppressed = 0 alongside the other state resets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f40f3f30-ed6a-4ce0-b2f7-115d7e6339e8
📒 Files selected for processing (2)
apps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/session.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/terminal-host/session.test.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/main/terminal-host/session.test.ts (2)
267-339: Cover the mixed healthy/backpressured-client case.
createSessionWithSocket()hardcodes a single attached socket, so this suite would still pass ifbroadcastEvent()accidentally stopped broadcasting after the first backpressured client. Since the fix is supposed to skip only the affected socket, add a two-client test where one socket is waiting fordrainand another keeps receiving frames.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.test.ts` around lines 267 - 339, create a new test using createSessionWithSocket that attaches two FakeSocket instances to the session (use the same attachedClients injection pattern) where one socket's writeFn returns false to simulate backpressure and the other returns true so it keeps receiving frames; call (session as ...).broadcastEvent / broadcast repeatedly and assert the non-backpressured socket receives frames while the backpressured socket is skipped, then emit a 'drain' on the backpressured FakeSocket and assert it resumes receiving frames; reference createSessionWithSocket, FakeSocket, attachedClients, broadcastEvent and broadcast when locating where to add the test.
441-444: Prefer advancing time through a seam instead of mutatingSessioninternals.Overwriting
backpressureWarnLastAtmakes this assertion depend on the current implementation of the rate limiter rather than the 5-second contract. Injecting a clock or stubbingDate.now()would keep the test resilient to internal refactors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.test.ts` around lines 441 - 444, The test directly mutates the Session internal field backpressureWarnLastAt; instead, replace that mutation with a time seam (e.g., stub Date.now() or use a fake timer) or use an injected clock on the Session so the test can advance time by >5 seconds without touching internals; update the failing test in session.test.ts to call the seam (or advance the fake clock) to simulate Date.now() being 5+ seconds earlier and assert behavior, referencing the Session instance and the backpressure rate-limit contract rather than the private backpressureWarnLastAt field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/terminal-host/session.test.ts`:
- Around line 267-339: create a new test using createSessionWithSocket that
attaches two FakeSocket instances to the session (use the same attachedClients
injection pattern) where one socket's writeFn returns false to simulate
backpressure and the other returns true so it keeps receiving frames; call
(session as ...).broadcastEvent / broadcast repeatedly and assert the
non-backpressured socket receives frames while the backpressured socket is
skipped, then emit a 'drain' on the backpressured FakeSocket and assert it
resumes receiving frames; reference createSessionWithSocket, FakeSocket,
attachedClients, broadcastEvent and broadcast when locating where to add the
test.
- Around line 441-444: The test directly mutates the Session internal field
backpressureWarnLastAt; instead, replace that mutation with a time seam (e.g.,
stub Date.now() or use a fake timer) or use an injected clock on the Session so
the test can advance time by >5 seconds without touching internals; update the
failing test in session.test.ts to call the seam (or advance the fake clock) to
simulate Date.now() being 5+ seconds earlier and assert behavior, referencing
the Session instance and the backpressure rate-limit contract rather than the
private backpressureWarnLastAt field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a80c4e90-66ae-4b87-a86b-b5d2c421caa4
📒 Files selected for processing (2)
apps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/session.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/terminal-host/session.ts
Kitenite
changed the title
Collaborator
Author
|
Updated summary for the final branch state:
Why this changed: The earlier branch went through a few narrower versions while backing out the lossy streaming idea. The final PR is now intentionally minimal: remove the noisy backpressure warning and leave the rest of the behavior alone. Validation: bun test apps/desktop/src/main/terminal-host/session.test.ts |
Kitenite
changed the title
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/terminal-host/session.test.ts (1)
15-22: Assert the stdout pause/resume side effect here.The production path is valuable because it pauses
subprocess.stdouton backpressure and resumes it ondrain, but this suite only checksclientSocketsWaitingForDrain. A regression that stopped callingpause()/resume()would still pass, because Line 396 keeps writing even while backpressured.🔧 Tighten the fake and assert the contract
class FakeStdout extends EventEmitter { + paused = false; + pauseCalls = 0; + resumeCalls = 0; + pause() { + this.paused = true; + this.pauseCalls++; return this; } resume() { + this.paused = false; + this.resumeCalls++; return this; } }Then return
childfromcreateSessionWithSocket()and assertchild.stdout.pauseCalls === 1after the first backpressured write andchild.stdout.resumeCalls === 1afterdrain.Also applies to: 267-339, 365-400
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/terminal-host/session.test.ts` around lines 15 - 22, The test must verify that the session pauses and resumes subprocess stdout on backpressure: enhance FakeStdout to track pause/resume call counts (e.g., add pauseCalls and resumeCalls incremented in pause() and resume()), have createSessionWithSocket return the mocked child process, then in the test assert that after the first backpressured write child.stdout.pauseCalls === 1 and after emitting 'drain' child.stdout.resumeCalls === 1; also add similar assertions in the other test blocks mentioned (around lines 267-339 and 365-400) to ensure pause/resume behavior is covered alongside clientSocketsWaitingForDrain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/desktop/src/main/terminal-host/session.test.ts`:
- Around line 15-22: The test must verify that the session pauses and resumes
subprocess stdout on backpressure: enhance FakeStdout to track pause/resume call
counts (e.g., add pauseCalls and resumeCalls incremented in pause() and
resume()), have createSessionWithSocket return the mocked child process, then in
the test assert that after the first backpressured write child.stdout.pauseCalls
=== 1 and after emitting 'drain' child.stdout.resumeCalls === 1; also add
similar assertions in the other test blocks mentioned (around lines 267-339 and
365-400) to ensure pause/resume behavior is covered alongside
clientSocketsWaitingForDrain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0578043f-07e5-4715-ac7b-325205fb1704
📒 Files selected for processing (2)
apps/desktop/src/main/terminal-host/session.test.tsapps/desktop/src/main/terminal-host/session.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/desktop/src/main/terminal-host/session.ts
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)
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
Remove the terminal-host backpressure warning while keeping the existing socket backpressure handling unchanged.
What Changed
console.warnemitted when a client socket returnsfalsefromwrite()Why
The warning was noisy and no longer worth carrying forward as a dedicated change. This leaves the existing behavior in place without logging repeated backpressure messages.
Impact
Validation
bun test apps/desktop/src/main/terminal-host/session.test.tsSummary by CodeRabbit