Fix heap retention from failed IPC sends and trim oversized AHP logs#320267
Draft
roblourens wants to merge 1 commit into
Draft
Fix heap retention from failed IPC sends and trim oversized AHP logs#320267roblourens wants to merge 1 commit into
roblourens wants to merge 1 commit into
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
Comment on lines
+97
to
111
| const entry = { ...message, _ahpLog: meta }; | ||
| // Fast path: serialize once. The vast majority of messages are small, so | ||
| // we only pay a single stringify and use its length to decide whether the | ||
| // rare oversized-message path below is needed. | ||
| let body = stringifyAhpLogEntry(entry); | ||
| if (body.length > MAX_LOG_LINE_LENGTH) { | ||
| // Slow path (rare): a single message carried very large payloads. Walk | ||
| // the object via a replacer that elides long string values, keeping the | ||
| // line valid JSONL instead of writing/holding the full multi-MB payload. | ||
| meta.truncated = true; | ||
| body = stringifyAhpLogEntryTruncated(entry, MAX_LOGGED_STRING_LENGTH); | ||
| } | ||
| const line = `${body}\n`; | ||
| this._pending.push(VSBuffer.fromString(line)); | ||
| this._scheduleDrain(); |
| * marker. The result is still well-formed JSON, so the log remains valid JSONL. | ||
| * Only used for the rare oversized entry, so the extra per-value work is fine. | ||
| */ | ||
| export function stringifyAhpLogEntryTruncated(value: unknown, maxStringLength: number): string { |
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 leaked request handler (the actual leak)A
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 only removed when a response is leaked for the lifetime of the channel. That leaked entry transitively retains the rejected promise, the error, and the multi-GB serialization buffers.arrives handlerFix: 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.Bug buffer retention via error stacks (defense in depth)B
BufferWriternow implementsIDisposable, and the three serialize/sendsites dispose it in afinally. So even if a thrown serialization error's captured stack pins thesendframe, 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 keeping the output valid JSONL and marking the entrytruncated.elidedTests
BufferWriter releases its buffers on disposereject path)
elides 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)