fix(otel-thread-ctx): don't assert a record is valid when growing it

[szegedi]

Reported by @nsavoire on #391. Confirmed, and more serious than "fires in debug builds".

The bug

Append's reallocate path asserts that the record it just copied is valid:

memcpy(new_rec.get(), self->record_, sizeof(OtelThreadCtxRecord) + current_used);
...
// The copy should've preserved valid=1 from the source record.
assert(new_rec->valid == 1);

invalidate() sets that byte to 0, and appending afterwards is supported — there is a test asserting exactly that — so the memcpy faithfully copies a 0 and the assert fires.

It is not debug-only. NDEBUG is never defined for this addon (nothing in dd_pprof.target.mk defines it), so assert() is live in Release as well. Both configurations abort. Reproduced through the public API on Linux:

const ctx = new ThreadContext(traceId, spanId);
ctx.run(() => {
  ctx.invalidate();                        // span finished
  ctx.appendAttributes(['x'.repeat(200)]);  // late attribute
});

node: ../bindings/otel-thread-ctx.cc:625:
  Assertion `new_rec->valid == 1' failed.
Aborted (exit 134)

Only the reallocate path is affected — an append that fits the current capacity is written in place and never copies the header. A fresh record has 36 bytes of attrs_data capacity (64 - sizeof(header)), so an attribute over ~34 bytes on a fresh record is enough to trigger it.

Why the existing test missed it

appendAttributes after invalidate mutates attrs_data but leaves valid=0 appends 'late' — 6 bytes. That fits the initial capacity, takes the in-place path, and never reaches the copy. Right behaviour under test, wrong size.

The fix

Assert what the check was actually for — that the memcpy carried the header across intact — rather than a value the API legitimately changes:

const uint8_t src_valid = self->record_->valid;   // before the copy
...
assert(new_rec->valid == src_valid);

Still catches a genuine copy bug (shortening the memcpy so it no longer covers the header) and is correct whether the record is valid or not.

Verification

The regression test forks, since the failure is an abort that would otherwise take the whole mocha run down — same pattern as the teardown test in this file. Built the pre-fix C++ against it to confirm it bites:

	result
new test vs pre-fix binding	exited with code=null signal=SIGABRT + the assertion
new test vs fixed binding	passing
Node 24 npm test	exit 0, 166 passing
Node 26 npm test	exit 0, 166 passing
Node 20 test:js-asan	exit 0, 101 passing, 0 leaks
public-API repro	exit 0

clang-format -Werror clean, gts check 0 errors.

Follow-up

The same code is vendored in custom-labels js/addon.cpp and needs the same fix; happy to port once this lands.

Worth considering separately: NDEBUG not being defined means every assert() in these bindings is live in production. That is arguably fine as a fail-fast policy, but it should be a deliberate choice — this one was a wrong assertion shipping as a release-build abort.

[github-actions]

Overall package size

Self size: 2.49 MB
Deduped: 3.19 MB
No deduping: 3.19 MB

Dependency sizes

| name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.3.1 | 504.33 kB | 504.33 kB | | source-map | 0.8.0 | 185.66 kB | 185.66 kB | | node-gyp-build | 4.8.4 | 13.86 kB | 13.86 kB |

_{🤖 This report was automatically generated by heaviest-objects-in-the-universe}