fix: prevent unbounded PersistentProtocol replay buffer - #324160
fix: prevent unbounded PersistentProtocol replay buffer#324160KevinWang-wpq wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens PersistentProtocol (the reconnection-capable IPC protocol used by the remote agent connection) against unbounded memory growth. The protocol keeps unacknowledged outgoing messages in a replay buffer so they can be re-sent after a reconnection; if a peer stops acknowledging (dead or heavily backpressured), that buffer could grow without limit. The change adds count- and byte-based caps and, when exceeded, emits a socket timeout and drops the buffer.
Changes:
- Track retained payload bytes (
_outgoingUnackMsgBytes) alongside the unacknowledged-message queue, and add configurable/default caps (MaxOutgoingUnacknowledgedMessages= 10000,MaxOutgoingUnacknowledgedBytes= 256MB). - On overflow, fire an
UNACKNOWLEDGED_MESSAGEsocket timeout, clear the replay buffer, and add aQueue.clear()helper; surface retained bytes in theSocketTimeoutEventand timeout logging. - Add a node IPC regression test for the byte-limit path.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/vs/base/parts/ipc/common/ipc.net.ts |
Adds byte tracking, overflow limits/options, Queue.clear(), overflow-check and buffer-clear methods, and the new unacknowledgedMsgBytes event field. |
src/vs/platform/remote/common/remoteAgentConnection.ts |
Includes unacknowledgedMsgBytes in the socket-timeout log line. |
src/vs/base/parts/ipc/test/node/ipc.net.test.ts |
New regression test verifying the replay buffer is dropped when the byte limit is exceeded. |
c85f5d4 to
9166625
Compare
Fixes a memory leak in `PersistentProtocol`. `PersistentProtocol` keeps every regular message in `_outgoingUnackMsg` until the peer acknowledges it. That queue is also the replay buffer used when a connection reconnects. When a connection is dead, congested, or stuck reconnecting, acknowledgements can stop advancing while callers continue to send messages. In that state, `_outgoingUnackMsg` keeps retaining every unacknowledged payload until the protocol is disposed. Bound the replay buffer by both message count and retained payload bytes. When either limit is exceeded, emit a dedicated replay-buffer-overflow timeout reason, clear the buffer, and treat the overflow as unrecoverable for remote connections. Add node IPC regression tests for both byte-limit and count-limit overflow paths.
9166625 to
f0c58de
Compare
|
Adding the anonymized heap-snapshot evidence that motivated this fix. Heap snapshot evidenceIn a renderer heap snapshot, the dominant retained object was the reconnect replay buffer owned by
The relevant retainer chain was: This matches the failure mode addressed here: regular messages are appended to This PR adds the missing bound in two dimensions:
It also treats replay-buffer overflow as unrecoverable for remote connections, so the connection does not reconnect into a stream whose replay messages were already dropped. @Giuspepe could you please take a look when you have a chance? Copilot's latest re-review on the updated commit generated no new comments, and the PR now includes regression coverage for both byte-limit and count-limit overflow paths. |
Fixes a memory leak in
PersistentProtocol.Details
PersistentProtocolkeeps every regular message in_outgoingUnackMsguntil the peer acknowledges it. That queue is also the replay buffer used when a connection reconnects.When a connection is dead, congested, or stuck reconnecting, acknowledgements can stop advancing while callers continue to send messages. In that state,
_outgoingUnackMsgkeeps retaining every unacknowledged payload, including largeVSBufferinstances, until the protocol is disposed.Change
The change bounds the replay buffer by both message count and retained payload bytes. It also tracks the retained bytes so timeout diagnostics can show how much payload memory is currently held by unacknowledged messages.
When either replay-buffer limit is exceeded,
PersistentProtocolemits a dedicatedunacknowledgedMessageReplayBufferOverflowtimeout reason, clears the replay buffer, and advances the local acknowledgement state so the dropped entries are not kept alive.For remote connections, replay-buffer overflow is treated as unrecoverable instead of reconnecting into a stream that is missing replay messages.
Before
A dead or congested peer could stop sending acknowledgements while new messages were still appended to
_outgoingUnackMsg. The replay buffer could then grow without a memory bound and retain all queued message payloads.After
The replay buffer is capped by configurable/default count and byte limits. When the cap is exceeded, the buffer is released, the timeout log includes the retained byte count, and node IPC regression tests cover both byte-limit and count-limit overflow paths.