fix(msgpack): cap MsgpackChunk growth at the agent intake limit#8672
Conversation
Overall package sizeSelf size: 6.4 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.2.0 | 104.26 kB | 843.44 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | dc-polyfill | 0.1.11 | 25.74 kB | 25.74 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: bc3a6c5 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-06-27 19:03:03 Comparing candidate commit bc3a6c5 in PR branch Found 39 performance improvements and 0 performance regressions! Performance is the same for 2174 metrics, 73 unstable metrics.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8672 +/- ##
=======================================
Coverage 93.69% 93.69%
=======================================
Files 889 889
Lines 50856 50895 +39
Branches 11830 11837 +7
=======================================
+ Hits 47647 47688 +41
+ Misses 3209 3207 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
ae5f111 to
39df4fa
Compare
2f63db1 to
80e8f34
Compare
39df4fa to
53f9732
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53f97323d6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
juan-fernandez
left a comment
There was a problem hiding this comment.
I want to be careful with this because test optimization does not need an agent and should not be limited to agent limits
53f9732 to
e237690
Compare
`MsgpackChunk.reserve` used to double the backing buffer with no upper bound, so a single pathological span (multi-MB stack trace, unsanitized meta tag the size of a media file) could grow the chunk until allocation failed and took the host process with it. The agent's trace intake caps payloads at 50 MiB, so anything above that is rejected downstream anyway. `reserve` now refuses growth past `MAX_SIZE = 50 MiB` with an `OverflowError` (a `RangeError` subclass carrying `code: 'ERR_MSGPACK_CHUNK_OVERFLOW'` as an own property); the cursor stays put so the caller's recovery path can decide what to do. A single shared class keeps the `code` identical across every throw site so every writer recognises it. A single chunk is not the only way to cross the cap: two chunks each capped under `MAX_SIZE` can sum past it once stitched together, and that sum only exists inside `makePayload`, after `encode` already returned — so the encode-time catch never sees it. Each path that can raise the tag drops the queued payload rather than emit a partial buffer — rolling back just the in-flight entry is unsafe because the string cache may already point at offsets we'd invalidate: 1. `AgentEncoder.encode` calls the virtual `reset()` so subclasses can clear their own per-payload state. `AgentlessCiVisibilityEncoder. _eventCount` would otherwise stay primed after the bytes were wiped and the next `makePayload` would patch the `events` array prefix with a count larger than the payload that survived. 2. `CoverageCIVisibilityEncoder` writes into its own `_coverageBytes` chunk, so the parent catch never sees its overflow. It catches the tag locally and drops the queued coverage form. 3. `DataStreamsWriter.flush` catches the tag from the standalone `msgpack.encode` and drops the pipeline-stats payload rather than propagating the `RangeError` out of the writer. 4. The v0.5 `makePayload` concatenates the string table and the trace bytes; the CI Visibility `makePayload` builds its metadata prefix in a fresh chunk and concatenates the events. Both now check the assembled size and raise the tag before allocating the fused buffer. 5. The shared `Writer.flush` wraps `makePayload`, so the assembled-size overflow from (4) resets the encoder and drops the payload instead of escaping as an uncaught `RangeError` in the host application.
e237690 to
bc3a6c5
Compare
There was a problem hiding this comment.
More details
This PR introduces a critical safety cap on MsgpackChunk growth at 50 MiB (the agent intake limit), preventing pathological traces/metadata from causing unbounded buffer growth and host process OOM. The fix is architecturally sound: chunk-level enforcement via reserve(), encoder-level assembled-payload checks, and writer-level catch handlers for both single chunks and multi-chunk assemblies. All 79 existing tests pass, plus extensive adversarial validation confirms the implementation handles boundary conditions atomically and leaves chunks in recoverable states after overflow attempts.
📊 Validated against 13 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit bc3a6c5 · What is Autotest? · Any feedback? Reach out in #autotest
`MsgpackChunk.reserve` used to double the backing buffer with no upper bound, so a single pathological span (multi-MB stack trace, unsanitized meta tag the size of a media file) could grow the chunk until allocation failed and took the host process with it. The agent's trace intake caps payloads at 50 MiB, so anything above that is rejected downstream anyway. `reserve` now refuses growth past `MAX_SIZE = 50 MiB` with an `OverflowError` (a `RangeError` subclass carrying `code: 'ERR_MSGPACK_CHUNK_OVERFLOW'` as an own property); the cursor stays put so the caller's recovery path can decide what to do. A single shared class keeps the `code` identical across every throw site so every writer recognises it. A single chunk is not the only way to cross the cap: two chunks each capped under `MAX_SIZE` can sum past it once stitched together, and that sum only exists inside `makePayload`, after `encode` already returned — so the encode-time catch never sees it. Each path that can raise the tag drops the queued payload rather than emit a partial buffer — rolling back just the in-flight entry is unsafe because the string cache may already point at offsets we'd invalidate: 1. `AgentEncoder.encode` calls the virtual `reset()` so subclasses can clear their own per-payload state. `AgentlessCiVisibilityEncoder. _eventCount` would otherwise stay primed after the bytes were wiped and the next `makePayload` would patch the `events` array prefix with a count larger than the payload that survived. 2. `CoverageCIVisibilityEncoder` writes into its own `_coverageBytes` chunk, so the parent catch never sees its overflow. It catches the tag locally and drops the queued coverage form. 3. `DataStreamsWriter.flush` catches the tag from the standalone `msgpack.encode` and drops the pipeline-stats payload rather than propagating the `RangeError` out of the writer. 4. The v0.5 `makePayload` concatenates the string table and the trace bytes; the CI Visibility `makePayload` builds its metadata prefix in a fresh chunk and concatenates the events. Both now check the assembled size and raise the tag before allocating the fused buffer. 5. The shared `Writer.flush` wraps `makePayload`, so the assembled-size overflow from (4) resets the encoder and drops the payload instead of escaping as an uncaught `RangeError` in the host application.
`MsgpackChunk.reserve` used to double the backing buffer with no upper bound, so a single pathological span (multi-MB stack trace, unsanitized meta tag the size of a media file) could grow the chunk until allocation failed and took the host process with it. The agent's trace intake caps payloads at 50 MiB, so anything above that is rejected downstream anyway. `reserve` now refuses growth past `MAX_SIZE = 50 MiB` with an `OverflowError` (a `RangeError` subclass carrying `code: 'ERR_MSGPACK_CHUNK_OVERFLOW'` as an own property); the cursor stays put so the caller's recovery path can decide what to do. A single shared class keeps the `code` identical across every throw site so every writer recognises it. A single chunk is not the only way to cross the cap: two chunks each capped under `MAX_SIZE` can sum past it once stitched together, and that sum only exists inside `makePayload`, after `encode` already returned — so the encode-time catch never sees it. Each path that can raise the tag drops the queued payload rather than emit a partial buffer — rolling back just the in-flight entry is unsafe because the string cache may already point at offsets we'd invalidate: 1. `AgentEncoder.encode` calls the virtual `reset()` so subclasses can clear their own per-payload state. `AgentlessCiVisibilityEncoder. _eventCount` would otherwise stay primed after the bytes were wiped and the next `makePayload` would patch the `events` array prefix with a count larger than the payload that survived. 2. `CoverageCIVisibilityEncoder` writes into its own `_coverageBytes` chunk, so the parent catch never sees its overflow. It catches the tag locally and drops the queued coverage form. 3. `DataStreamsWriter.flush` catches the tag from the standalone `msgpack.encode` and drops the pipeline-stats payload rather than propagating the `RangeError` out of the writer. 4. The v0.5 `makePayload` concatenates the string table and the trace bytes; the CI Visibility `makePayload` builds its metadata prefix in a fresh chunk and concatenates the events. Both now check the assembled size and raise the tag before allocating the fused buffer. 5. The shared `Writer.flush` wraps `makePayload`, so the assembled-size overflow from (4) resets the encoder and drops the payload instead of escaping as an uncaught `RangeError` in the host application.
`MsgpackChunk.reserve` used to double the backing buffer with no upper bound, so a single pathological span (multi-MB stack trace, unsanitized meta tag the size of a media file) could grow the chunk until allocation failed and took the host process with it. The agent's trace intake caps payloads at 50 MiB, so anything above that is rejected downstream anyway. `reserve` now refuses growth past `MAX_SIZE = 50 MiB` with an `OverflowError` (a `RangeError` subclass carrying `code: 'ERR_MSGPACK_CHUNK_OVERFLOW'` as an own property); the cursor stays put so the caller's recovery path can decide what to do. A single shared class keeps the `code` identical across every throw site so every writer recognises it. A single chunk is not the only way to cross the cap: two chunks each capped under `MAX_SIZE` can sum past it once stitched together, and that sum only exists inside `makePayload`, after `encode` already returned — so the encode-time catch never sees it. Each path that can raise the tag drops the queued payload rather than emit a partial buffer — rolling back just the in-flight entry is unsafe because the string cache may already point at offsets we'd invalidate: 1. `AgentEncoder.encode` calls the virtual `reset()` so subclasses can clear their own per-payload state. `AgentlessCiVisibilityEncoder. _eventCount` would otherwise stay primed after the bytes were wiped and the next `makePayload` would patch the `events` array prefix with a count larger than the payload that survived. 2. `CoverageCIVisibilityEncoder` writes into its own `_coverageBytes` chunk, so the parent catch never sees its overflow. It catches the tag locally and drops the queued coverage form. 3. `DataStreamsWriter.flush` catches the tag from the standalone `msgpack.encode` and drops the pipeline-stats payload rather than propagating the `RangeError` out of the writer. 4. The v0.5 `makePayload` concatenates the string table and the trace bytes; the CI Visibility `makePayload` builds its metadata prefix in a fresh chunk and concatenates the events. Both now check the assembled size and raise the tag before allocating the fused buffer. 5. The shared `Writer.flush` wraps `makePayload`, so the assembled-size overflow from (4) resets the encoder and drops the payload instead of escaping as an uncaught `RangeError` in the host application.
`MsgpackChunk.reserve` used to double the backing buffer with no upper bound, so a single pathological span (multi-MB stack trace, unsanitized meta tag the size of a media file) could grow the chunk until allocation failed and took the host process with it. The agent's trace intake caps payloads at 50 MiB, so anything above that is rejected downstream anyway. `reserve` now refuses growth past `MAX_SIZE = 50 MiB` with an `OverflowError` (a `RangeError` subclass carrying `code: 'ERR_MSGPACK_CHUNK_OVERFLOW'` as an own property); the cursor stays put so the caller's recovery path can decide what to do. A single shared class keeps the `code` identical across every throw site so every writer recognises it. A single chunk is not the only way to cross the cap: two chunks each capped under `MAX_SIZE` can sum past it once stitched together, and that sum only exists inside `makePayload`, after `encode` already returned — so the encode-time catch never sees it. Each path that can raise the tag drops the queued payload rather than emit a partial buffer — rolling back just the in-flight entry is unsafe because the string cache may already point at offsets we'd invalidate: 1. `AgentEncoder.encode` calls the virtual `reset()` so subclasses can clear their own per-payload state. `AgentlessCiVisibilityEncoder. _eventCount` would otherwise stay primed after the bytes were wiped and the next `makePayload` would patch the `events` array prefix with a count larger than the payload that survived. 2. `CoverageCIVisibilityEncoder` writes into its own `_coverageBytes` chunk, so the parent catch never sees its overflow. It catches the tag locally and drops the queued coverage form. 3. `DataStreamsWriter.flush` catches the tag from the standalone `msgpack.encode` and drops the pipeline-stats payload rather than propagating the `RangeError` out of the writer. 4. The v0.5 `makePayload` concatenates the string table and the trace bytes; the CI Visibility `makePayload` builds its metadata prefix in a fresh chunk and concatenates the events. Both now check the assembled size and raise the tag before allocating the fused buffer. 5. The shared `Writer.flush` wraps `makePayload`, so the assembled-size overflow from (4) resets the encoder and drops the payload instead of escaping as an uncaught `RangeError` in the host application.
Summary
MsgpackChunk.reserveused to grow the backing buffer without bound. Apathological span (multi-MB stack trace, unsanitized meta tag the size
of a media file) could grow the chunk until allocation failed and took
the host process with it.
reservenow refuses growth past 50 MiB (theagent's trace intake ceiling) with a tagged
RangeError(
code: 'ERR_MSGPACK_CHUNK_OVERFLOW').Every direct
MsgpackChunkconsumer catches the tag and drops the queuedpayload — rolling back just the in-flight entry is unsafe because the
string cache may already hold offsets pointing at bytes we'd invalidate:
AgentEncoder.encodecalls the virtualreset()so subclasses canclear their own per-payload state (
AgentlessCiVisibilityEncoder. _eventCountwould otherwise stay primed and the nextmakePayloadwould patch the
eventsarray prefix with a count larger than thepayload that survived).
CoverageCIVisibilityEncoderwrites into its own_coverageByteschunk and catches the tag locally; without the catch the parent
AgentEncoder.encodecatch path never sees it.DataStreamsWriter.flushcatches the tag from the standalonemsgpack.encodeand drops the pipeline-stats payload.Stacked on top of #8504; merges into that branch.