Fix heap retention from failed IPC sends and trim oversized AHP logs#320267
Merged
Conversation
Investigating an Agents window heap snapshot (7.7GB, mostly native memory) revealed a multi-GB retention rooted at a failed "Export Agent Host Debug Logs" operation. Two IPC bugs and one logging issue conspired: Bug A (the leak): in `ChannelClient.requestPromise`, the response handler is registered in `this.handlers` before the request is serialized/sent. If serialization throws synchronously (e.g. an oversized argument makes `VSBuffer.concat` throw `RangeError: Array buffer allocation failed`), the handler entry is never it's only deleted on a response that neverremoved arrives. The leaked handler retains the rejected promise, the error, and (via the error's captured stack) the serialization buffers, for the lifetime of the channel. The heap snapshot confirmed this was the sole retainer of the error. Fix: clean up the handler and reject if `sendRequest` throws. This also settles the promise on the uninitialized (`then`) path, which previously hung forever. Bug B (defense in depth): `BufferWriter` now implements `IDisposable` and the `send`/serialize sites dispose it in a `finally`, so a thrown serialization error's captured stack can't pin the intermediate buffers. This protects the server-side and other send sites that have no handler-map cleanup. AHP log trimming: a single AHP protocol log line could reach ~157MB (e.g. a `resourceRead` carrying a base64 file). `AhpJsonlLogger.log` now stringifies once (fast path) and, only when a line exceeds 1MB, re-serializes with oversized string values elided so the line stays valid JSONL. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a heap-retention scenario in the IPC layer (leaked request handlers when synchronous serialization fails) and reduces memory/disk pressure from Agent Host Protocol (AHP) transport logging by truncating oversized log lines while keeping JSONL valid.
Changes:
- Ensure
ChannelClient.requestPromisecleans up its handler map entry and rejects whensendRequestthrows synchronously during serialization. - Make
BufferWriterdisposable and ensure all IPC send sites dispose it infinallyblocks to avoid retaining intermediate buffers via error stacks. - Add AHP JSONL log-line truncation for oversized payloads, plus unit tests covering truncation and
BufferWriterdisposal semantics.
Show a summary per file
| File | Description |
|---|---|
| src/vs/platform/agentHost/test/common/ahpJsonlLogger.test.ts | Adds a unit test ensuring oversized string payloads are elided while maintaining valid JSONL. |
| src/vs/platform/agentHost/common/ahpJsonlLogger.ts | Implements log-line truncation logic and records truncation metadata on oversized entries. |
| src/vs/base/parts/ipc/test/common/ipc.test.ts | Adds tests asserting BufferWriter releases buffers on dispose and that deferred request serialization failures reject properly. |
| src/vs/base/parts/ipc/common/ipc.ts | Fixes handler leak on synchronous send/serialization failure and disposes BufferWriter at send sites. |
Copilot's findings
- Files reviewed: 4/4 changed files
- Comments generated: 2
It has no cross-module consumer (not even tests), so per the repo guideline to only export APIs used across components, drop the export. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
kycutler
approved these changes
Jun 7, 2026
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
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.
What & why
Investigating an Agents window heap snapshot (7.7GB heap, ~97% native memory) traced a multi-GB retention back to a failed Export Agent Host Debug Logs operation. Two IPC bugs and one logging issue conspired to pin the memory. Retainer analysis of the snapshot confirmed the root cause precisely: the failed request's rejected promise was retained by the live channel's
handlersmap, and through the error's captured stack it retained the serialization buffers + log strings.Changes
Bug A — leaked request handler (the actual leak)
In
ChannelClient.requestPromise, the response handler is registered inthis.handlersbefore the request is serialized and sent. If serialization throws synchronously (e.g. an oversized argument makesVSBuffer.concatthrowRangeError: Array buffer allocation failed), no request ever goes out and the handler — only removed when a response arrives — is leaked for the lifetime of the channel. That leaked entry transitively retains the rejected promise, the error, and the multi-GB serialization buffers.Fix: wrap
sendRequestso that on a synchronous failure we delete the handler and reject. This also settles the promise on the uninitialized (then) path, which previously hung forever on such a failure.Why one dangling entry pins so much (the retention chain)
This isn't obvious from reading the code, so concretely, here is the chain the snapshot shows from a GC root down to the payload:
ChannelClientholds itsthis.handlersMapfor the channel's whole lifetime.handlerclosure. It is only ever deleted when a response with thatidarrives — and since the send threw, none ever will.c/e) callbacks, which internally reference the rejectedPromiseitself.[[PromiseResult]]slot keeps theErroralive. This is the counter-intuitive hop: a rejected promise retains its rejection value for as long as the promise is reachable — so the error is not eligible for GC even though nothingawaits it.Error's lazily-captured.stackpins the stack frames that were live at throw time, including thesend/serialize frame whose localwriter(aBufferWriter) is still in scope.writer.buffersthen holds the multi-GB serialized payload (and, in the AHP case, the ~157MB log string).In the snapshot the
RangeErrorhad exactly one incoming edge (the rejected promise), and the only path from that whole cluster to a GC root washandler closure → Map → ChannelClient.handlers. So the danglinghandlersentry is both necessary and sufficient to pin everything, which is why deleting it on a synchronous send failure releases the entire graph.Bug B — buffer retention via error stacks (defense in depth)
BufferWriternow implementsIDisposable, and the three serialize/sendsites dispose it in afinally. So even if a thrown serialization error's captured stack pins thesendframe (step 5 above), it can't drag the intermediate buffers along. This covers the server-side and other send sites that have no handler-map cleanup of their own.AHP log line trimming
A single AHP protocol log line could reach ~157MB (e.g. a
resourceReadcarrying a base64-encoded file).AhpJsonlLogger.lognow stringifies once (fast path) and, only when a line exceeds 1MB, re-serializes with oversized string values elided — keeping the output valid JSONL and marking the entrytruncated.Tests
BufferWriter releases its buffers on disposerequest rejects (and cleans up) when serialization throws on the deferred pathelides oversized string payloads while keeping the line valid JSONLFull unit suite passes (10812 passing).
Notes
requestEvent(handler registered up front, removed on last-listener-removed); left untouched as it's far lower risk (small event args). Happy to harden it too if desired.(Written by Copilot)