Repository navigation
fix(daemon): pause producers when stream backlogs grow - #20947
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe daemon stream pipeline now accounts retained entry bytes and centralizes session flushing. A backpressure coordinator tracks queued data, metadata, and pending writes with hysteresis, ownership checks, and stale-callback protection. Deep sockets hold eligible output and use refill coordination. Stream pauses integrate with client pauses. Tests and audit artifacts cover TCP backpressure, producer control, cleanup, and retention measurements. Priority: ➖ Normal 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 12 functions across 17 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 6599f59f-9159-4458-bd08-6a409b764ae8
📒 Files selected for processing (21)
docs/audits/daemon-stream-retention/README.mddocs/audits/daemon-stream-retention/after.jsondocs/audits/daemon-stream-retention/before-1mib-chunks.jsondocs/audits/daemon-stream-retention/before-64kib-chunks.jsondocs/audits/daemon-stream-retention/reproduce.mjssrc/main/daemon/daemon-client-connections.tssrc/main/daemon/daemon-server.tssrc/main/daemon/daemon-stream-backpressure-socket.test.tssrc/main/daemon/daemon-stream-backpressure.test.tssrc/main/daemon/daemon-stream-backpressure.tssrc/main/daemon/daemon-stream-data-batcher.test.tssrc/main/daemon/daemon-stream-data-batcher.tssrc/main/daemon/daemon-stream-data-entry.tssrc/main/daemon/daemon-stream-data-split.tssrc/main/daemon/daemon-stream-entry-accounting.tssrc/main/daemon/daemon-stream-held-refill.tssrc/main/daemon/daemon-stream-keep-tail-drop.tssrc/main/daemon/session-producer-pause.tssrc/main/daemon/session-terminal-control.test.tssrc/main/daemon/session.tssrc/main/daemon/terminal-host.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one documentation nitpick inline and one design question below.
Reviewed changes
- Producer backpressure tracker: new
DaemonStreamBackpressureaggregates queued strings, per-entry metadata, and outstanding socket writes across clients, pauses a session's producer above a per-session high water while the global total exceeds 4 MiB, and resumes with hysteresis. - Batcher integration:
DaemonStreamDataBatcherfeeds backlog state into the tracker on every enqueue/flush/write completion; all socket writes route through it. Held-refill and entry accounting are extracted intoDaemonStreamHeldRefillanddaemon-stream-entry-accounting.ts;flushSessionmoves todaemon-stream-data-entry.ts. - Pause ownership:
SessionProducerPausegains an independentstreamBackpressuredflag so the client pause/failsafe and daemon stream pressure cannot release each other; threaded throughSessionandTerminalHost, skipped when the session has no attached clients or is terminating. - Connection lifecycle: stream-socket replacement flushes the pending batch to the replacement socket; control/stream disconnect clears the batcher plus backpressure.
- Tests + audit: real blocked-socket test (visible/hidden, 64 KiB and 1 KiB), tracker unit tests, metadata-accounting test, session pause-ownership tests, and a reproduction script with before/after measurements.
ℹ️ Stream-triggered pauses have no liveness bound
SessionProducerPause.pause('stream', …) deliberately skips the 5s failsafe, so the producer is only resumed by refresh() dropping below the low watermarks or by a clear/detach/termination. If a peer stops draining without closing the socket (half-open relay/TCP), the socket write callbacks stop firing, refresh() never observes a drop, and the producer can stay paused until the transport finally closes. Since the client here is main and the SSH-relay path is a supported deployment, please confirm there is a heartbeat/reconnect that bounds this, or document the accepted behavior.
Technical details
# Unbounded stream pause when the peer stops reading without closing
## Affected sites
- `src/main/daemon/session-producer-pause.ts:16` — `pause('stream', …)` returns after
`setStreamBackpressured(true)` and never arms `failsafeTimer`.
- `src/main/daemon/daemon-stream-backpressure.ts:96` — the only `setPaused(sessionId, false)` paths
require a `refresh()` where the per-session bytes fall to `SESSION_LOW_WATER_BYTES` or `pressured`
clears; `refresh()` is driven by enqueues and socket write completions only.
- `src/main/daemon/daemon-server.ts:77` — wires the pause directly to `host.pauseProducer(id, 'stream')`.
## Required outcome
- The producer pause must be released within a bounded time even when a live socket stops draining,
or the absence of such a bound must be a documented, deliberate tradeoff.
## Open questions for the human
- Does the stream transport (direct or SSH relay) guarantee close/error detection within a bounded
window after main stalls, so `onStreamDisconnected` reliably clears the backpressure?
- If not, is a max-pause or periodic reconciliation refresh warranted?DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Incremental re-review of the commits since the prior pullfrog review (ca5aea1):
- Stream-socket replacement resets per-socket-generation state:
DaemonClientConnections.installStreamSocketnow callsstreamDataBatcher.replaceStream(clientId)before re-flushing, so a replaced socket's stale pending-write bytes and armed held-refill symbol can no longer keep the backpressure tracker pressured or block a re-arm on the replacement. DaemonStreamDataBatcher.replaceStream: clears the batch timer,heldRefill, and the backpressure client entry without dropping queued payloads; the immediately followingflushre-accounts them onto the new socket. This addresses the CodeRabbit stability finding on the replacement path.- Retention audit tightened: the README reproducer command now points at
reproduce.mjsand marks the Node 26 measurements as historical (addressing the prior inline nit);reproduce.mjsasserts the visible producers paused and the hidden producer processed the full 8 MiB before emitting JSON. - Test-title fix:
daemon-stream-data-batcher.test.tsuses%sinstead of%ifor the string payload placeholder.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Stream backpressure pauses a session's PTY with no deadline: the only un-pause comes from the consumer draining, so a half-open peer that stops reading without closing freezes the shell for the rest of the session. Arm a 60s watchdog on the false->true stream-pause transition (not on the re-assertions refresh() makes for neighbouring sessions). On fire, mark the session stall-released: it becomes keep-tail droppable, its backlog is thinned behind a dataGap, and the producer runs again. The existing dataGap path makes the renderer restore that pane from the daemon's snapshot, so the user sees the terminal jump to current rather than sit frozen. The mark clears once the session's last byte leaves the daemon, restoring ordinary pausing. Nothing here reports a process exit - loss of contact with a consumer is not evidence about the child. Also enable TCP keepalive on the stream socket so a genuinely dead peer closes and onStreamDisconnected clears the pause.
`oxlint-disable-next-line` covers only the line directly after it, so a rationale wrapped onto a second comment line suppressed nothing and the casts failed the changed-code quality gate. Drop the remaining JSON.parse cast for an annotated binding.
2 participants
ELI5
A terminal can produce text faster than a connected viewer can read it. Previously, unread text could keep piling up in memory. This change pauses the producer until the viewer catches up.
What Changed
Pause terminal producers when queued output crosses the existing pressure thresholds. Resume after the viewer drains the queue or its ownership ends. Output keeps its original order and is not intentionally discarded.
Why
Pausing preserves output while limiting memory used by a slow viewer. Dropping output would lose terminal text; disconnecting would interrupt the viewer. Disk spooling would require a separate storage and recovery design.
Tradeoff
A viewer that never catches up can keep its producer paused indefinitely. Providers without pause support retain their existing write-through fallback.
Linked Issue
Related to #19831 as memory hardening; this PR does not prove that incident was caused by stream retention.
Visual Proof
No before/after UI capture is attached; this is a behavioral change. Automated behavior evidence is linked under Testing.
Testing
The audit covers real TCP pressure, visible and hidden output, viewer detachment, resumption, and daemon lifecycle controls.
Audit evidence
Review
The intended behavior is backpressure, not data loss. Review provider pause support and the permanent-stall behavior before merging.