Skip to content

fix: avoid Blob upload body to prevent native ArrayBuffer leak#311

Open
gabrielbryk wants to merge 1 commit into
grafana:mainfrom
gabrielbryk:fix/avoid-blob-upload-body
Open

fix: avoid Blob upload body to prevent native ArrayBuffer leak#311
gabrielbryk wants to merge 1 commit into
grafana:mainfrom
gabrielbryk:fix/avoid-blob-upload-body

Conversation

@gabrielbryk

Copy link
Copy Markdown

Summary

  • The profile uploader builds each request body as a FormData containing a Blob, which undici's fetch serializes via Blob.prototype.stream(). On Node 24.16.0–24.18.0 that pins the source ArrayBuffer in a handle that's never released, so every flush leaks one payload-sized ArrayBuffer. This builds the multipart/form-data body by hand into a plain Uint8Array instead, which never touches Blob.stream().
  • Also drains the response body on the success path — previously the HTTP 200 body was left unconsumed and only read on the !ok path.

Why a hand-rolled body instead of FormData

The leaked bytes are native/off-heap arrayBuffers, so they don't show up in a V8 heap snapshot — RSS just climbs monotonically until the process is OOM-killed. With the wall and heap profilers sharing one uploader we saw ~45–80 MB/day per instance in production, never plateauing. It's a Node core bug (nodejs/node#63574), fixed on main (#63577) but not yet backported to 24.x (#64105) — and users only pick that up on a Node upgrade.

Passing a typed array sidesteps Blob.stream() on every Node version, and also drops a Buffer -> Blob -> stream round-trip on each flush regardless of version. The emitted bytes are wire-identical to what undici produces for the equivalent FormData/Blob — same part framing, Content-Disposition/Content-Type, and CRLF placement; only the random boundary token differs. This also looks like a plausible current-Node cause behind #28.

Test plan

  • yarn lint, yarn build, and yarn test (51/51) pass.
  • New test/build-profile-multipart-body.test.ts parses the serialized body back with busboy (the same parser the existing profiler tests use) and asserts the profile part is byte-equal to the input, with the expected name/filename, an application/octet-stream part content-type, and a multipart/form-data; boundary=----pyroscope… header.
  • The existing profiler integration tests, which decode real uploads with busboy, pass unchanged — the wire format is still accepted end to end.
  • Accelerated A/B on Node 24.16.0 (5s flush interval): the Blob body climbs monotonically, the Uint8Array body stays flat. Controls isolate it to the fetch-time blob.stream() call — the same payload sent as a typed array is flat, and a Blob that's built but never fetched is flat.

No public API or dependency change.

The profile uploader built the request body as a FormData containing a
Blob. undici's fetch serializes Blob parts via Blob.prototype.stream(),
which on Node 24.16.0-24.18.0 pins the source ArrayBuffer in an eternal
handle (nodejs/node#63574). Each flush leaks one payload-sized
ArrayBuffer, producing a steady ~45-80 MB/day native-memory leak in
production for a long-lived process running two profilers.

Serialize the multipart/form-data body by hand into a plain Uint8Array
instead. The wire bytes are identical to what undici emits for the
equivalent FormData/Blob (same part framing, headers, CRLF placement);
only the arbitrary boundary token differs. A typed-array body never
touches Blob.stream(), so the leak cannot occur on any Node version, and
it also skips a wasteful Buffer -> Blob -> stream round-trip on every
flush.

Also always drain the response body (response.text()) on the success
path, which previously left the 200-response body unconsumed.

Refs: nodejs/node#63574, nodejs/node#63577, nodejs/node#64105
@gabrielbryk
gabrielbryk requested review from a team as code owners July 21, 2026 23:42
@cla-assistant

cla-assistant Bot commented Jul 21, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@jake-kramer

Copy link
Copy Markdown
Contributor

Thanks for your contribution @gabrielbryk. This does appear to fix a memory leak caused by node. I'll provide a thorough review tomorrow.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses a native memory leak on Node 24.16.0–24.18.0 by avoiding FormData + Blob upload bodies (which trigger Blob.stream() in undici fetch), replacing them with a manually serialized multipart/form-data payload built into a Uint8Array. It also ensures the HTTP response body is always consumed so connections/buffers are released promptly.

Changes:

  • Add buildProfileMultipartBody() utility to serialize the profile part as a fully materialized Uint8Array multipart payload (no Blob.stream()).
  • Update PyroscopeApiExporter to upload the typed-array multipart body and set the correct Content-Type header.
  • Add a new unit test that round-trips the serialized multipart body through busboy and asserts part metadata + byte equality.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/utils/build-profile-multipart-body.ts Introduces a hand-rolled multipart serializer that returns { body: Uint8Array, contentType } to avoid Blob.stream() usage.
src/pyroscope-api-exporter.ts Switches the uploader to use the new multipart builder and drains the response body to release resources.
test/build-profile-multipart-body.test.ts Adds tests that parse the serialized multipart payload with busboy and validate headers/bytes/type.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

it('serializes the profile part with bytes intact (byte-equal to input)', async () => {
// Random bytes exercise binary content, including sequences that could
// resemble CRLF/boundary framing.
const content = new Uint8Array(randomBytes(4096).buffer);
Comment on lines +131 to +135
// Always drain the response body. The success path (HTTP 200) previously
// left the body unconsumed, which keeps the connection from being
// released back to the pool promptly and retains the response buffer.
const responseText: string = await response.text();

// WHY this is a hand-rolled buffer rather than `FormData`:
//
// When the request body is a `FormData` containing a `Blob`, undici's `fetch`
// serializes each blob part by calling `blob.stream()`. On Node 24.16.0 -

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of v26.0.0–v26.5.x is affected too, not just 24.16–24.18. The upstream fix (nodejs/node#63577) is still not released or backported as of today.

@@ -104,30 +108,34 @@ export class PyroscopeApiExporter implements ProfileExporter {
const arrayBuffer: Uint8Array<ArrayBuffer> =

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Buffer.concat always copies into a fresh ArrayBuffer-backed Buffer, so the SharedArrayBuffer narrowing no longer serves a purpose so arrayBuffer can be removed and profileBuffer can be passed directly to buildProfileMultipartBody

Readable.from(Buffer.from(body)).pipe(bb);
});

describe('buildProfileMultipartBody', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest another test to protect against underlying undici changes in the future

it('is byte-identical to the FormData/Blob serialization it replaces, modulo the boundary', async () => {
  // Payload deliberately contains CRLFs and a leading "--" sequence so the
  // comparison also covers framing-adjacent content, not just plain bytes.
  const content = Buffer.from('some profile bytes\r\n--tricky\r\n');

  // What fetch used to send: a FormData holding one Blob field.
  // Response(formData) runs undici's real multipart serializer — the same
  // code path fetch uses — without any network I/O. (The one-off
  // blob.stream() call this triggers in-test is harmless.)
  const formData = new FormData();
  formData.append('profile', new Blob([content]), 'profile');
  const response = new Response(formData);
  const undiciContentType = response.headers.get('content-type') as string;
  const undiciBoundary = undiciContentType.split('boundary=')[1];
  const undiciBody = Buffer.from(await response.arrayBuffer());
  // TODO change to `buildProfileMultipartBody(content)` when buffer is removed
  const { body, contentType } = buildProfileMultipartBody(
    new Uint8Array(content)
  );
  const boundary = contentType.split('boundary=')[1];

  // Substitute each side's (random) boundary token, then require the full
  // byte strings — headers, casing, CRLF placement, epilogue — to match.
  // latin1 keeps the comparison byte-exact for non-UTF-8 content.
  const normalize = (buffer: Buffer, from: string): string =>
    buffer.toString('latin1').replaceAll(from, '<boundary>');

  // If undici ever changes its serialization in a future Node release,
  // this failing is a feature: it flags that the "wire-identical to the
  // old FormData path" guarantee needs to be re-verified against servers.
  assert.equal(
    normalize(Buffer.from(body), boundary),
    normalize(undiciBody, undiciBoundary)
  );
});

@jake-kramer

Copy link
Copy Markdown
Contributor

@gabrielbryk Note we now require commit signing for our repos, more information here: https://community.grafana.com/t/action-required-signed-commits-mandatory-for-all-grafana-repositories/163404


const CRLF = '\r\n';

export interface MultipartRequestBody {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend dropping multipart altogether, we can just ingest with POST /ingest?format=pprof

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion! Much simpler: #316

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants