From 58d9f09065eecd53b5955ee8a5cb088e5c5b2806 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 7 Aug 2026 09:02:12 +0200 Subject: [PATCH 01/14] chore: ignore TypeScript major-version bumps in dependabot (#380) TypeScript 7 (the new native port) is not yet supported by typescript-eslint, and therefore not by gts, so a major bump breaks the lint job's `gts check` step. The latest published typescript-eslint still declares a `typescript >=4.8.4 <6.1.0` peer dependency. Ignore typescript major-version updates until the ecosystem catches up. See https://github.com/typescript-eslint/typescript-eslint/issues/10940 Co-authored-by: Claude Opus 4.8 (1M context) (cherry picked from commit aac1e50023eb87ac523cd56664b810aa4939dcb1) --- .github/dependabot.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index e45eef52..0f9d0319 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -15,6 +15,12 @@ updates: ignore: # Internal dependencies that we update manually - dependency-name: "pprof-format" + # TypeScript 7 (the native port) is not yet supported by typescript-eslint + # (and therefore gts), so major bumps break `npm run lint`. + # See https://github.com/typescript-eslint/typescript-eslint/issues/10940 + # Re-enable once typescript-eslint ships TS >=7 support. + - dependency-name: "typescript" + update-types: ["version-update:semver-major"] versioning-strategy: "increase" labels: - dependabot From 0f162a2cfe83920ca32e337f7d32822e49461a26 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 7 Aug 2026 09:50:12 +0200 Subject: [PATCH 02/14] fix(wall): don't derive PersistentContextPtr from node::ObjectWrap (#385) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(wall): don't derive PersistentContextPtr from node::ObjectWrap node::ObjectWrap registers a per-instance environment cleanup hook in its constructor and calls RemoveEnvironmentCleanupHook from its destructor. That teardown path CHECKs the Environment is still alive: node[650]: void node::RemoveEnvironmentCleanupHook( v8::Isolate*, CleanupHook, void*) at ../src/api/hooks.cc:142 Assertion failed: (env) != nullptr 3: node::RemoveEnvironmentCleanupHook(...) 4: node::ObjectWrap::RemoveCleanupHook() 5: node::ObjectWrap::~ObjectWrap() 6: dd::PersistentContextPtr::~PersistentContextPtr() 8: node::ObjectWrap::WeakCallback(...) A PCP is owned by a weak V8 handle, so V8 decides when it dies — and V8 runs weak callbacks during isolate teardown, after the Environment is gone. The CHECK then aborts the process with SIGABRT. The wrapper only ever needed two things from the base class: the internal-field pointer that GetContextPtrSignalSafe reads, and a weak handle to hang the object's lifetime on. Neither needs a cleanup hook — ~WallProfiler already walks the live list and deletes any PCP V8 has not collected, which is what keeps LSAN quiet at exit. So hold the weak Persistent directly and drop the base class. ~PersistentContextPtr resets the handle, which cancels the weak callback when ~WallProfiler is the one doing the deleting and is a no-op when we arrived from the callback itself. Reproduced on main under ASAN (which perturbs GC timing enough to make it deterministic) as an abort during teardown after the Time Profiler tests; the full ASAN suite goes from exit 134 to 158 passing with no leaks reported. Co-Authored-By: Claude Opus 5 (1M context) * fix(wall): tag the internal-field store for Node 26 Node 26 requires an EmbedderDataTypeTag on Object::SetAlignedPointerInInternalField: error: no matching function for call to 'v8::Object::SetAlignedPointerInInternalField(int, dd::PersistentContextPtr*)' note: candidate: 'void v8::Object::SetAlignedPointerInInternalField( int, void*, v8::EmbedderDataTypeTag)' note: candidate expects 3 arguments, 2 provided node::ObjectWrap::Wrap hid this: its header handles the tag internally, so taking over the store exposed the version difference. Add the setter counterpart to the existing GetAlignedPointerFromInternalField helper and use it, so both ends of the internal-field access agree on kEmbedderDataTypeTagDefault. Verified on Node 20, 24 and 26 (the last is where AsyncContextFrame is on by default, so it actually exercises the PCP path): builds clean, 158 passing, ASAN exit 0 with no leaks or aborts. (cherry picked from commit f66e514bf31786d8a0b0a387bbb5cda350a689f7) --- bindings/profilers/wall.cc | 73 ++++++++++++++++++++++++++++++-------- 1 file changed, 58 insertions(+), 15 deletions(-) diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index 95600d3b..dee88687 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -106,7 +106,41 @@ void SetContextPtr(ContextPtr& contextPtr, } } -class PersistentContextPtr : public node::ObjectWrap { +inline void* GetAlignedPointerFromInternalField(Object* object, int index) { +#if NODE_MAJOR_VERSION >= 26 + return object->GetAlignedPointerFromInternalField( + index, kEmbedderDataTypeTagDefault); +#else + return object->GetAlignedPointerFromInternalField(index); +#endif +} + +inline void SetAlignedPointerInInternalField(Local object, + int index, + void* value) { +#if NODE_MAJOR_VERSION >= 26 + object->SetAlignedPointerInInternalField( + index, value, kEmbedderDataTypeTagDefault); +#else + object->SetAlignedPointerInInternalField(index, value); +#endif +} + +// Deliberately not a node::ObjectWrap. That base registers a per-instance +// environment cleanup hook in its constructor and calls +// RemoveEnvironmentCleanupHook from its destructor, which CHECKs that the +// Environment is still alive: +// +// node[650]: void node::RemoveEnvironmentCleanupHook(...) hooks.cc:142 +// Assertion failed: (env) != nullptr +// +// A PCP is owned by a weak V8 handle, and V8 runs weak callbacks during +// isolate teardown — after the Environment has been torn down — so that CHECK +// fires and aborts the process. All we need from a wrapper is the +// internal-field pointer and the weak handle, and ~WallProfiler already +// deletes whatever is still on the live list, so the cleanup hook the base +// class installs has nothing left to do. +class PersistentContextPtr { ContextPtr context; // Back-pointer to the WallProfiler that created this PCP. Guaranteed to be // a valid pointer whenever pprev_ != nullptr — ~WallProfiler nulls pprev_ @@ -125,8 +159,17 @@ class PersistentContextPtr : public node::ObjectWrap { PersistentContextPtr** pprev_ = nullptr; PersistentContextPtr* next_ = nullptr; + // Weak handle on the holder object. Owns this PCP: when V8 collects the + // holder, WeakCallback deletes us. + v8::Persistent handle_; + friend class WallProfiler; + static void WeakCallback( + const v8::WeakCallbackInfo& data) { + delete data.GetParameter(); + } + public: PersistentContextPtr(WallProfiler* profiler, Local wrap); @@ -139,14 +182,18 @@ class PersistentContextPtr : public node::ObjectWrap { ContextPtr Get() const { return context; } static PersistentContextPtr* Unwrap(Local wrap) { - return node::ObjectWrap::Unwrap(wrap); + return static_cast( + GetAlignedPointerFromInternalField(*wrap, 0)); } }; PersistentContextPtr::PersistentContextPtr(WallProfiler* profiler, Local wrap) : profiler_(profiler) { - Wrap(wrap); + auto* isolate = Isolate::GetCurrent(); + SetAlignedPointerInInternalField(wrap, 0, this); + handle_.Reset(isolate, wrap); + handle_.SetWeak(this, &WeakCallback, v8::WeakCallbackType::kParameter); // Splice ourselves at the head of profiler's live list. auto** headSlot = profiler->liveContextPtrHeadSlot(); next_ = *headSlot; @@ -167,15 +214,11 @@ PersistentContextPtr::~PersistentContextPtr() { if (next_ != nullptr) next_->pprev_ = pprev_; profiler_->recordContextRelease(); } -} - -inline void* GetAlignedPointerFromInternalField(Object* object, int index) { -#if NODE_MAJOR_VERSION >= 26 - return object->GetAlignedPointerFromInternalField( - index, kEmbedderDataTypeTagDefault); -#else - return object->GetAlignedPointerFromInternalField(index); -#endif + // Cancels the weak callback when we're deleted by ~WallProfiler rather than + // by V8; a no-op when we got here from WeakCallback itself. The holder + // object's internal field is left dangling either way, but nothing reads it + // once the owning profiler is gone. + handle_.Reset(); } // Maximum number of rounds in the GetV8ToEpochOffset @@ -678,9 +721,9 @@ WallProfiler::~WallProfiler() { // Delete every PCP still live in the CPED map. ~PCP would normally unlink // itself via pprev_/next_, but we're tearing down the list we point into — // so null pprev_ first to signal "already detached" and let ~PCP skip the - // unlink. (~ObjectWrap will still clear V8's weak callback during delete, - // so the dangling internal-field pointer in the wrap object stays inert - // even if V8 later GCs the wrap.) + // unlink. (~PCP still resets its weak handle during delete, so the dangling + // internal-field pointer in the wrap object stays inert even if V8 later + // GCs the wrap.) auto* p = liveContextPtrHead_; while (p != nullptr) { auto* next = p->next_; From 118ad934c5ea64f55648475067f4e865935d1825 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 7 Aug 2026 07:57:29 +0000 Subject: [PATCH 03/14] build(deps-dev): bump @types/node from 26.1.1 to 26.1.2 in the patch-updates group across 1 directory (#381) Bumps the patch-updates group with 1 update in the / directory: [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node). Updates `@types/node` from 26.1.1 to 26.1.2 - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node) --- updated-dependencies: - dependency-name: "@types/node" dependency-version: 26.1.2 dependency-type: direct:development update-type: version-update:semver-patch dependency-group: patch-updates ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 11415b4599f6ef1996ebe54274e9c766082a2650) --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index 3f9160cc..53097e0a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15,7 +15,7 @@ }, "devDependencies": { "@types/mocha": "^10.0.1", - "@types/node": "26.1.1", + "@types/node": "26.1.2", "@types/semver": "^7.5.8", "@types/sinon": "^22.0.0", "@types/tmp": "^0.2.3", @@ -962,9 +962,9 @@ "license": "MIT" }, "node_modules/@types/node": { - "version": "26.1.1", - "resolved": "https://registry.npmjs.org/@types/node/-/node-26.1.1.tgz", - "integrity": "sha512-nxAkRSVkN1Y0JC1W8ky/fTfkGsMmcrRsbx+3XoZE+rMOX71kLYTV7fLXpqud1GpbpP5TuffXFqfX7fH2GgZREw==", + "version": "26.1.2", + "resolved": "https://registry.npmjs.org/@types/node/-/node-26.1.2.tgz", + "integrity": "sha512-Vu4a5UFA9rIIFJ7rB/Vaafh9lrCQszopTCx6KjFboXTGQbPNasehVR5TEiithSDGyd1DEiUByggTZsg8jukeIg==", "dev": true, "license": "MIT", "dependencies": { diff --git a/package.json b/package.json index bcea3076..2e86680a 100644 --- a/package.json +++ b/package.json @@ -43,7 +43,7 @@ }, "devDependencies": { "@types/mocha": "^10.0.1", - "@types/node": "26.1.1", + "@types/node": "26.1.2", "@types/semver": "^7.5.8", "@types/sinon": "^22.0.0", "@types/tmp": "^0.2.3", From 162312c5e93ef8761bf4e49d73de18ae0b3097d5 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 7 Aug 2026 10:00:32 +0200 Subject: [PATCH 04/14] otel-thread-ctx: add ThreadContext.invalidate() to flip valid byte (#383) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When an SDK finishes a span, calling clearContext() on the current async-context frame detaches the ThreadContext only from that frame. Sibling and detached-continuation frames that already inherited the reference keep holding the same JS object — and with it the same underlying native record — so an out-of-process reader sampling those threads still sees the finished span's trace / span IDs as active. invalidate() writes 0 to the record's `valid` header byte in place, using the same volatile+atomic_signal_fence protocol the constructor and Append() use for header bytes readers may race with. Because every async-context frame holding this ThreadContext reference observes the same shared record buffer, a single invalidate() drops the record out of scope for every such frame at once — readers see valid=0 and MUST ignore the record per OTEP-4947. The method is idempotent, safe under repeated calls, and orthogonal to attrs_data mutation: appendAttributes after invalidate is still observable in the record bytes, but readers honor the valid=0 flag regardless. (cherry picked from commit cbcfa11e05412238bb70688cb3e2247a5fb70b25) --- bindings/otel-thread-ctx.cc | 23 ++++++++++++++++++++ ts/src/otel-thread-ctx.ts | 15 +++++++++++++ ts/test/test-otel-thread-ctx.ts | 37 +++++++++++++++++++++++++++++++++ 3 files changed, 75 insertions(+) diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index c98bb209..2aaea7ba 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -190,6 +190,7 @@ class CtxWrap : public ObjectWrap { static void New(const FunctionCallbackInfo& args); static void DebugBytes(const FunctionCallbackInfo& args); static void Append(const FunctionCallbackInfo& args); + static void Invalidate(const FunctionCallbackInfo& args); static void IsTruncated(const FunctionCallbackInfo& args); // Encode the JS array at `attrs_val` into `out` as packed (key, len, value) @@ -543,6 +544,25 @@ void CtxWrap::Append(const FunctionCallbackInfo& args) { free(old_rec); } +// Mark this record's `valid` byte as 0 in place. Every async-context +// frame that holds this ThreadContext reference — including those that +// merely inherited it verbatim from a parent frame — will subsequently +// present the same shared record to a reader, so this one write drops +// the record out of scope for every such frame at once. Intended for +// span-finish, where clearing the current frame's context via +// `clearContext()` alone leaves sibling / detached-continuation frames +// still exposing the finished span. Idempotent; safe to call multiple +// times. +void CtxWrap::Invalidate(const FunctionCallbackInfo& args) { + CtxWrap* self = ObjectWrap::Unwrap(args.This()); + if (!self) { + args.GetIsolate()->ThrowError("not a ThreadContext"); + return; + } + std::atomic_signal_fence(std::memory_order_release); + *reinterpret_cast(&self->record_->valid) = 0; +} + // Returns true if any attribute was ever dropped from this wrapper's // record because it would have pushed attrs_data past the cap — set during // CtxWrap::New() if the initial set didn't fit, or by any subsequent @@ -587,6 +607,9 @@ void CtxWrap::Init(Local exports) { tpl->PrototypeTemplate()->Set( String::NewFromUtf8Literal(isolate, "appendAttributes"), FunctionTemplate::New(isolate, Append)); + tpl->PrototypeTemplate()->Set( + String::NewFromUtf8Literal(isolate, "invalidate"), + FunctionTemplate::New(isolate, Invalidate)); tpl->PrototypeTemplate()->Set( String::NewFromUtf8Literal(isolate, "isTruncated"), FunctionTemplate::New(isolate, IsTruncated)); diff --git a/ts/src/otel-thread-ctx.ts b/ts/src/otel-thread-ctx.ts index cbc91c93..b5d9e75b 100644 --- a/ts/src/otel-thread-ctx.ts +++ b/ts/src/otel-thread-ctx.ts @@ -61,6 +61,20 @@ export interface ThreadContext { appendAttributes( attributes: Array | undefined, ): void; + + /** + * Mark this context's underlying record `valid` byte as 0 in place. + * Every async-context frame that still holds this `ThreadContext` + * reference (including those that inherited it verbatim from a + * parent frame) will subsequently present a record with `valid = 0` + * to a reader, so this one call drops the record out of scope for + * every such frame at once. Intended for the span-finish path, where + * clearing only the current frame's context via {@link clearContext} + * would leave sibling and detached-continuation frames still exposing + * the finished span's trace / span IDs. Idempotent. + */ + invalidate(): void; + isTruncated(): boolean; /** Debug-only: returns the on-the-wire record bytes. Not stable. */ debugBytes(): Uint8Array; @@ -224,6 +238,7 @@ if (process.platform === 'linux') { // AsyncLocalStorage. class NoopThreadContext implements ThreadContext { appendAttributes(): void {} + invalidate(): void {} isTruncated(): boolean { return false; } diff --git a/ts/test/test-otel-thread-ctx.ts b/ts/test/test-otel-thread-ctx.ts index f6229f2c..d19167d1 100644 --- a/ts/test/test-otel-thread-ctx.ts +++ b/ts/test/test-otel-thread-ctx.ts @@ -696,6 +696,43 @@ function captureBytes(opts: { }); }); + describe('invalidate', () => { + it('flips the record valid byte to 0 in place', () => { + // Verified through the shared record: same ThreadContext reference + // observed by any async-context frame that inherits it sees the + // new valid=0 the moment we call invalidate() on any of them. + const ctx = new ThreadContext(TRACE_ID_BYTES, SPAN_ID_BYTES); + ctx.run(() => { + strictAssert.equal(decodeHeader(_currentRecordBytes()!).valid, 1); + ctx.invalidate(); + strictAssert.equal(decodeHeader(_currentRecordBytes()!).valid, 0); + }); + }); + + it('is idempotent', () => { + const ctx = new ThreadContext(TRACE_ID_BYTES, SPAN_ID_BYTES); + ctx.run(() => { + ctx.invalidate(); + ctx.invalidate(); + strictAssert.equal(decodeHeader(_currentRecordBytes()!).valid, 0); + }); + }); + + it('appendAttributes after invalidate mutates attrs_data but leaves valid=0', () => { + // valid is a separate byte from attrs_data — an invalidated record + // can still grow via appendAttributes; readers MUST honor + // valid==0 and ignore the record regardless. + const ctx = new ThreadContext(TRACE_ID_BYTES, SPAN_ID_BYTES); + ctx.run(() => { + ctx.invalidate(); + ctx.appendAttributes([, 'late']); + const hdr = decodeHeader(_currentRecordBytes()!); + strictAssert.equal(hdr.valid, 0); + strictAssert.equal(hdr.attrsDataSize, 6); // key(1) + len(1) + 'late'(4) + }); + }); + }); + describe('getProcessContextAttributes', () => { it('rejects non-array keys', () => { strictAssert.throws( From d2116f1bee7324de1620def21147607c339f323f Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 7 Aug 2026 10:03:13 +0200 Subject: [PATCH 05/14] test(worker-threads): reap the spawned child after a timeout (#386) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A mocha timeout rejects the test but does not kill the process the test spawned, and `npm test` runs mocha without --exit, so mocha waits for the event loop to drain before exiting. A live child keeps its process handle on that loop, so a child outliving its test holds the whole run open. When the child is merely slow this is invisible — mocha waits the extra couple of seconds and exits. When the child is wedged, the run never ends and the CI job burns a runner until the job limit instead of failing in seconds. Seen on win32-test-22: the suite printed its epilogue inside the first minute, then the job sat in_progress for half an hour on `Run ./.prebuildify/test` after `should work` timed out. Capture the ChildProcess (promisify(execFile) exposes it on the returned promise) and kill it from afterEach, which mocha still runs after a timeout. Measured with the child replaced by a 10-minute sleep and the test timeout forced low: with the reap: 1.19s total without the reap: never exits (killed by a 30s watchdog) This only bounds the damage; it does not address why that child wedges on win32 in the first place, which is a separate investigation. Reaping is preferred over adding --exit to the mocha invocation: --exit would paper over any handle leak, and this suite exists partly to catch worker threads that fail to exit. (cherry picked from commit ee8559d9d70032df24df76529cdf660b9b52574c) --- ts/test/test-worker-threads.ts | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/ts/test/test-worker-threads.ts b/ts/test/test-worker-threads.ts index 93d8e04b..54059eb5 100644 --- a/ts/test/test-worker-threads.ts +++ b/ts/test/test-worker-threads.ts @@ -1,15 +1,32 @@ -import {execFile} from 'child_process'; +import {execFile, ChildProcess} from 'child_process'; import {promisify} from 'util'; import {Worker} from 'worker_threads'; const exec = promisify(execFile); describe('Worker Threads', () => { + let child: ChildProcess | undefined; + + afterEach(() => { + // A mocha timeout rejects the test but leaves the spawned process running, + // and `npm test` runs mocha without --exit, so mocha waits for the event + // loop to drain before exiting. A live child keeps its process handle on + // the loop, so a child that outlives its test holds the entire run open — + // if that child is itself wedged, until the CI job limit rather than until + // the test times out. Hooks still run after a timeout, so reap it here. + if (child?.exitCode === null && child.signalCode === null) { + child.kill(); + } + child = undefined; + }); + // eslint-ignore-next-line prefer-array-callback it('should work', function () { this.timeout(20000); const nbWorkers = 2; - return exec('node', ['./out/test/worker.js', String(nbWorkers)]); + const running = exec('node', ['./out/test/worker.js', String(nbWorkers)]); + child = running.child; + return running; }); it('should not crash when worker is terminated', async function () { From e607646229f1365952457a18143621184f3646e8 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 7 Aug 2026 10:05:19 +0200 Subject: [PATCH 06/14] add 'emit' column-numbers mode using pprof-format Line.column (#382) (cherry picked from commit 53e28991c1065ea508f5fd77fcc57a9036129fc4) --- package-lock.json | 8 +++--- package.json | 2 +- ts/src/profile-serializer.ts | 39 ++++++++++++++++++--------- ts/src/time-profiler.ts | 5 ++-- ts/test/test-profile-serializer.ts | 42 ++++++++++++++++++++++++++++++ 5 files changed, 77 insertions(+), 19 deletions(-) diff --git a/package-lock.json b/package-lock.json index 53097e0a..ec4d7ae6 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,7 +10,7 @@ "license": "Apache-2.0", "dependencies": { "node-gyp-build": "^4.8.4", - "pprof-format": "^2.2.1", + "pprof-format": "^2.3.0", "source-map": "^0.8.0" }, "devDependencies": { @@ -5062,9 +5062,9 @@ } }, "node_modules/pprof-format": { - "version": "2.2.1", - "resolved": "https://registry.npmjs.org/pprof-format/-/pprof-format-2.2.1.tgz", - "integrity": "sha512-p4tVN7iK19ccDqQv8heyobzUmbHyds4N2FI6aBMcXz6y99MglTWDxIyhFkNaLeEXs6IFUEzT0zya0icbSLLY0g==", + "version": "2.3.0", + "resolved": "https://registry.npmjs.org/pprof-format/-/pprof-format-2.3.0.tgz", + "integrity": "sha512-ovChRLoV4H3k0zKWq0AXewtnuPGskzBLjBPmF2N0DZu+H65PYuo51+6e/1GTb8Olm7oC0xvhhLdqPL4/XhCl4A==", "license": "MIT" }, "node_modules/prelude-ls": { diff --git a/package.json b/package.json index 2e86680a..ab06b461 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "license": "Apache-2.0", "dependencies": { "node-gyp-build": "^4.8.4", - "pprof-format": "^2.2.1", + "pprof-format": "^2.3.0", "source-map": "^0.8.0" }, "devDependencies": { diff --git a/ts/src/profile-serializer.ts b/ts/src/profile-serializer.ts index 5f74d7a3..5d5ebb36 100644 --- a/ts/src/profile-serializer.ts +++ b/ts/src/profile-serializer.ts @@ -87,12 +87,18 @@ function isGeneratedLocation( * (`column << 32 | line`) — the encoding the Datadog deobfuscation backend * decodes (the same one the Chrome profile intake uses). Required to * deobfuscate single-line (bundled/minified) frames, where the column is the - * only discriminator between functions. - * - * The enum leaves room for a future `'emit'` mode that would populate a real - * pprof `Line.column` field once the backend consumes it. + * only discriminator between functions. Only frames whose source map was + * declared but missing locally are packed. + * - `'emit'`: like `'pack'`, but records the column in the dedicated pprof + * `Line.column` field instead of packing it into the line field, so the line + * field stays standards-compliant and the two concerns are cleanly separated. + * Scoped to the same frames as `'pack'` — only those whose source map was + * declared but missing locally, i.e. the frames bound for server-side + * unminification. The Datadog backend performs the `column << 32 | line` + * packing itself (for Node.js profiles) when it consumes the column field. + * Requires pprof-format >= 2.3.0, the version that added `Line.column`. */ -export type ColumnNumbers = 'drop' | 'pack'; +export type ColumnNumbers = 'drop' | 'pack' | 'emit'; export const DEFAULT_COLUMN_NUMBERS: ColumnNumbers = 'drop'; @@ -158,6 +164,7 @@ function serialize( const functionIdMap = new Map(); const locationIdMap = new Map(); const packColumns = columnNumbers === 'pack'; + const emitColumns = columnNumbers === 'emit'; let hasMissingMapFiles = false; @@ -230,15 +237,23 @@ function serialize( } function getLine(loc: SourceLocation, scriptId?: number): Line { - // Only pack the column for frames whose source map was declared but missing - // locally — i.e. exactly the frames that will be sent for server-side - // unminification (the same condition that sets dd:has-missing-map-files). - // Locally-resolved frames keep their plain line, so packed values never - // reach profiles that skip server-side unminification. - const packColumn = packColumns && loc.missingMapFile === true; + // Both 'pack' and 'emit' carry the column only for frames whose source map + // was declared but missing locally — i.e. exactly the frames bound for + // server-side unminification (the same condition that sets + // dd:has-missing-map-files). Locally-resolved frames keep a plain line and + // no column, so column data never reaches profiles that skip server-side + // unminification. + const carryColumn = loc.missingMapFile === true; return new Line({ functionId: getFunction(loc, scriptId).id, - line: packColumn ? packLineAndColumn(loc.line, loc.column) : loc.line, + // 'pack' encodes the column into the high 32 bits of the line field. + line: + packColumns && carryColumn + ? packLineAndColumn(loc.line, loc.column) + : loc.line, + // 'emit' records the column in the dedicated pprof Line.column field + // instead; the backend does the line/column packing itself. + column: emitColumns && carryColumn ? loc.column : undefined, }); } diff --git a/ts/src/time-profiler.ts b/ts/src/time-profiler.ts index 5c95e04f..d57b94fb 100644 --- a/ts/src/time-profiler.ts +++ b/ts/src/time-profiler.ts @@ -115,8 +115,9 @@ export interface TimeProfilerOptions { * Controls how frame column numbers are represented in the serialized * profile. Defaults to `'drop'` (column omitted) to preserve the historical * line-number semantics for existing consumers. Set to `'pack'` to pack the - * column into the high 32 bits of the line field for backends that support - * it (e.g. Datadog's JS/Node deobfuscation). See {@link ColumnNumbers}. + * column into the high 32 bits of the line field, or `'emit'` to populate the + * dedicated pprof `Line.column` field, for backends that support it (e.g. + * Datadog's JS/Node deobfuscation). See {@link ColumnNumbers}. */ columnNumbers?: ColumnNumbers; } diff --git a/ts/test/test-profile-serializer.ts b/ts/test/test-profile-serializer.ts index c2c0b085..5ad6d488 100644 --- a/ts/test/test-profile-serializer.ts +++ b/ts/test/test-profile-serializer.ts @@ -497,6 +497,48 @@ describe('profile-serializer', () => { ); }); + it('emits the real column in the pprof Line.column field and keeps the line plain when columnNumbers is "emit"', () => { + // 'emit' never packs: the line field stays plain and the generated column + // (1) is written to the dedicated pprof Line.column field. The backend + // performs the line/column packing itself for Node.js profiles. + const profile = serializeTimeProfile( + makeSingleNodeTimeProfile(missingJsPath), + 1000, + sourceMapper, + false, + undefined, + [], + 'emit', + ); + assertHasMissingMapToken(profile); + const line = profile.location![0].line![0]; + assert.strictEqual(BigInt(line.line), 1n); + assert.strictEqual(BigInt(line.column), 1n); + }); + + it('does not carry a column under "emit" for a frame with no missing map', () => { + // 'emit' is scoped to the same frames as 'pack': only those whose map was + // declared but missing locally. With no source mapper the frame is neither + // resolved nor flagged missing, so no column is emitted (Line.column + // defaults to 0) and the line stays plain. + const profile = serializeTimeProfile( + makeSingleNodeTimeProfile(missingJsPath), + 1000, + undefined, + false, + undefined, + [], + 'emit', + ); + assert.ok( + !profile.comment || profile.comment.length === 0, + 'expected no missing-map token without a source mapper', + ); + const line = profile.location![0].line![0]; + assert.strictEqual(BigInt(line.line), 1n); + assert.strictEqual(BigInt(line.column), 0n); + }); + it('leaves a missing-map frame line plain under the default "drop"', () => { const profile = serializeTimeProfile( makeSingleNodeTimeProfile(missingJsPath), From d8c17bdabfb9125e34657539e2288bb6343fde0f Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 7 Aug 2026 11:57:45 +0200 Subject: [PATCH 07/14] fix(wall): hold the PCP holder in a Global, not a Persistent (#387) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-up on #385. v8::Persistent has no destruction behaviour: the handle leaks unless every path clears it by hand. v8::Global releases it in its own destructor, and is what every other handle in this file already uses — ContextPtr, cpedKey_, wrapObjectTemplate_, jsArray_. The Persistent introduced in #385 was the odd one out. Nothing was leaking in practice, since ~PersistentContextPtr always reset the handle explicitly, but relying on that is exactly the footgun the V8 docs warn about. Switching to Global makes the release structural, so the explicit Reset goes away with it. Historically the manual handle was justified: before #261 removed instance reuse, PersistentContextPtr recycled itself through a freelist and needed ClearWeak/Reset to unregister and re-register the same object. With reuse gone a handle lives exactly as long as its PCP, so there is nothing left for Persistent's manual semantics to buy. Verified on Node 20, 24 and 26 — the last is where AsyncContextFrame is on by default and PCPs are actually created. 163 passing, ASAN exit 0 with no leaks and no aborts on 20 and 24. (cherry picked from commit 9c00d101907c0a4548ed67cada7f5b0933997c99) --- bindings/profilers/wall.cc | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index dee88687..6f33179b 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -161,7 +161,7 @@ class PersistentContextPtr { // Weak handle on the holder object. Owns this PCP: when V8 collects the // holder, WeakCallback deletes us. - v8::Persistent handle_; + v8::Global handle_; friend class WallProfiler; @@ -214,11 +214,6 @@ PersistentContextPtr::~PersistentContextPtr() { if (next_ != nullptr) next_->pprev_ = pprev_; profiler_->recordContextRelease(); } - // Cancels the weak callback when we're deleted by ~WallProfiler rather than - // by V8; a no-op when we got here from WeakCallback itself. The holder - // object's internal field is left dangling either way, but nothing reads it - // once the owning profiler is gone. - handle_.Reset(); } // Maximum number of rounds in the GetV8ToEpochOffset From a6828f41cf2131f1a35f4fdf1e7dbe09af3d1174 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Fri, 7 Aug 2026 15:59:37 +0200 Subject: [PATCH 08/14] fix(heap): don't assume a per-isolate state exists (#384) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(heap): don't assume a per-isolate state exists GetAllocationProfile and MapAllocationProfile dereference the per-isolate HeapProfilerState after only checking that V8 returned a profile: auto& state = PerIsolateData::For(isolate)->GetHeapProfilerState(); std::unique_ptr profile( isolate->GetHeapProfiler()->GetAllocationProfile()); if (!profile) { return Nan::ThrowError("Heap profiler is not enabled."); } const bool allocations = state->allocations; // <- state may be null A non-null profile only proves V8's sampling heap profiler is running. It does not prove we started it: anything else in the process can enable it out of band — the inspector's HeapProfiler.startSampling, DevTools, a second agent — and only our own StartSamplingHeapProfiler creates the state. In that case the guard passes and we dereference an empty shared_ptr, which segfaults. Both call sites were null-checked until 5.15.0, when MonitorOutOfMemory switched from unconditionally replacing the state to reusing an existing one. That made "state already exists" the normal case and the checks were dropped along the way — MapAllocationProfile still null-checks `state` one line above the unguarded OnNewProfile() call. Restore the checks, keeping the pre-5.15.0 behaviour of serving the profile without allocation stats rather than throwing: V8's profiler really is enabled, so "Heap profiler is not enabled." would be wrong. Fix two pre-existing instances of the same assumption while here, both reachable because StopSamplingHeapProfiler() resets the state: - NearHeapLimit ran `state->insideCallback` unguarded. The state that recorded the callback's installation is the one that was dropped, so nothing could uninstall it. Remove the callback and leave the heap limit alone so V8 does its normal OOM handling. - InterruptCallback is requested from NearHeapLimit but runs later, so the state can disappear in between. The regression test forks a child process, since the failure mode is a SIGSEGV that would otherwise take the whole mocha run down with it. Co-Authored-By: Claude Opus 5 (1M context) * test(heap): keep the forked child out of LeakSanitizer's reach Under the asan CI job the forked child inherits LD_PRELOAD=libasan and LSAN_OPTIONS, so LeakSanitizer runs when it exits. The child ends via process.exit(), which skips V8 heap teardown, so every live object is reported as leaked and the child exits non-zero — failing the test for a reason unrelated to what it checks. Seen on asan (20): 1) foreign heap sampler should not crash when V8 heap sampling was enabled outside of pprof: Error: heap-foreign-sampler exited with code=1 signal=null Pass LSAN_OPTIONS=detect_leaks=0 to the child. ASAN itself stays active, so a real memory error in the code under test is still caught; only the exit-time leak sweep is suppressed, and only for this child. Two things made this harder to diagnose than it should have been, both fixed here: - The failure message came through empty because the promise settled on 'exit', which can fire before the piped stdio has drained. Settle on 'close' instead, so the captured output is complete. - Drop the retained allocation from 200k objects to 20k and keep it function-scoped rather than parking it on globalThis. The profile only needs a non-empty sample set. Co-Authored-By: Claude Opus 5 (1M context) * fix(heap): guard NearHeapLimit's profile, drop its bogus state check Addresses review feedback on #384. Check GetAllocationProfile for null before dereferencing it. It returns null when V8's sampling heap profiler isn't running, and that is reachable with this callback still installed: HeapProfilerCleanupHook stops V8's sampler without touching our state, so between that hook running and the isolate going away we stay registered with nothing to sample. The heap-limit bookkeeping still has to happen in that case, so only the profile-dependent work is skipped. Also remove the null-state check this branch had added to NearHeapLimit. Its justification was simply wrong: it claimed StopSamplingHeapProfiler could not uninstall the callback, but resetting the state shared_ptr destroys HeapProfilerState, whose destructor calls UninstallNearHeapLimitCallback. The callback cannot fire after the state is gone, so the check was dead code resting on a false premise. The one hole in that argument was ordering inside ~HeapProfilerState: it called V8's StopSamplingHeapProfiler before uninstalling, and by then the shared_ptr in PerIsolateData is already empty, so a GC in that window would have reached NearHeapLimit with no state. Fixed at the source by uninstalling first, which is where the invariant belongs. Node 20 ASAN: exit 0, 99 passing, no leaks, both OOM tests green. Node 24 still aborts on the pre-existing ~PersistentContextPtr teardown CHECK (#385), unrelated to this file. Co-Authored-By: Claude Opus 5 (1M context) * fix(heap): uninstall the near-heap-limit callback before dropping the state Two review nits from #384. StopSamplingHeapProfiler relied on ~HeapProfilerState to uninstall the near-heap-limit callback, but reset() only destroys the state when it holds the last reference — and it need not. Both NearHeapLimit and InterruptCallback take a shared_ptr copy for the duration of the call, so a stop() reached from inside one of them (the near-heap-limit JS callback calling heapProfiler.stop(), say) leaves the state alive, the destructor unrun, and the callback still registered with V8 while the per-isolate slot is already empty. The next near-heap-limit GC would then enter NearHeapLimit with no state at all — exactly the crash this branch is about. Uninstall explicitly instead; it is idempotent, clearing callbackInstalled. Also keep clearing state->profile when GetAllocationProfile returns null. Any profile retained from an earlier invocation is stale at that point and nothing below is going to consume or replace it. * fix(heap): re-add NearHeapLimit's null-state guard, with a real reason The version of this check removed earlier on this branch rested on a false premise — that StopSamplingHeapProfiler could not uninstall the callback — and deserved to go. There is a genuine reason for it, though, which only became apparent from the shared_ptr-copy problem in the previous commit. StopSamplingHeapProfiler now uninstalls before dropping the state, so that path is covered. The other destruction path is not: a shared_ptr copy taken by an in-flight NearHeapLimit or InterruptCallback can outlive the per-isolate slot. If the OOM JS callback calls process.exit(), PerIsolateData is erased while InterruptCallback still holds a reference, ~HeapProfilerState never runs, and the callback stays registered with an empty slot behind it. A teardown GC reaching the heap limit then enters NearHeapLimit with no state and dereferences null. Decline and let V8 do its normal OOM handling. Deliberately no RemoveNearHeapLimitCallback: the state that tracked the installation is already unreachable, so callbackInstalled cannot be cleared, and the only way to reach this is a process on its way out. Kept as its own commit because it partially reverses a change made earlier on this branch, and because it is defence in depth rather than a fix for anything reproducible — the trigger needs process.exit() from inside the OOM callback plus a teardown GC that hits the limit, which I could not turn into a non-flaky test. The branch it adds is therefore uncovered. --------- Co-authored-by: Claude Opus 5 (1M context) (cherry picked from commit 5478bf25ee6629f41affb77d536f8d5d6a71642c) --- bindings/profilers/heap.cc | 101 ++++++++++++++++++++++++++------ ts/test/heap-foreign-sampler.ts | 79 +++++++++++++++++++++++++ ts/test/test-heap-profiler.ts | 46 +++++++++++++++ 3 files changed, 207 insertions(+), 19 deletions(-) create mode 100644 ts/test/heap-foreign-sampler.ts diff --git a/bindings/profilers/heap.cc b/bindings/profilers/heap.cc index e4ee8cf3..6106625d 100644 --- a/bindings/profilers/heap.cc +++ b/bindings/profilers/heap.cc @@ -67,12 +67,17 @@ struct HeapProfilerState { explicit HeapProfilerState(v8::Isolate* isolate) : isolate(isolate) {} ~HeapProfilerState() { + // Uninstall first. By the time we run, the shared_ptr in PerIsolateData is + // already empty (that is what destroyed us), so NearHeapLimit would find no + // state to work with; anything below that can trigger a GC must not be able + // to reach it. + UninstallNearHeapLimitCallback(); + auto profiler = isolate->GetHeapProfiler(); if (profiler) { profiler->StopSamplingHeapProfiler(); } - UninstallNearHeapLimitCallback(); if (async) { // defer deletion of async when uv_close callback is invoked uv_close(reinterpret_cast(async), [](uv_handle_t* handle) { @@ -365,6 +370,21 @@ size_t NearHeapLimit(void* data, auto isolate = v8::Isolate::GetCurrent(); auto state = PerIsolateData::For(isolate)->GetHeapProfilerState(); + if (!state) { + // StopSamplingHeapProfiler uninstalls us before dropping the state, so + // normally this cannot happen. The gap is the other destruction path: a + // shared_ptr copy taken by an in-flight NearHeapLimit or InterruptCallback + // can outlive the per-isolate slot — the OOM JS callback calling + // process.exit() erases PerIsolateData while InterruptCallback still holds + // a reference, so ~HeapProfilerState never runs to uninstall us. Decline + // and let V8 do its normal OOM handling. + // + // Deliberately no RemoveNearHeapLimitCallback here: the state that tracked + // the installation is already unreachable, so callbackInstalled cannot be + // cleared, and the only way to get here is a process on its way out. + return current_heap_limit; + } + if (state->insideCallback) { // Reentrant call detected, try to increase heap limit a bit so that // previous callback can proceed @@ -401,26 +421,41 @@ size_t NearHeapLimit(void* data, stats.object_count()); } } + // GetAllocationProfile returns null when V8's sampling heap profiler isn't + // running, and that can happen while this callback is still installed: + // HeapProfilerCleanupHook stops V8's sampler without touching our state, so + // between that hook and the isolate actually going away we stay registered + // with nothing to sample. The heap-limit bookkeeping below still has to run, + // so skip only the profile-dependent work. std::unique_ptr profile{ isolate->GetHeapProfiler()->GetAllocationProfile()}; - state->profile = TranslateAllocationProfileToCpp(profile->GetRootNode()); - if (state->dumpProfileOnStderr) { - dumpAllocationProfile(stderr, state->profile.get()); - } - - if (!state->export_command.empty()) { - ExportProfile(*state); - } + if (profile) { + state->profile = TranslateAllocationProfileToCpp(profile->GetRootNode()); + if (state->dumpProfileOnStderr) { + dumpAllocationProfile(stderr, state->profile.get()); + } - if (!state->callback.IsEmpty()) { - if (state->callbackMode & kInterruptCallback) { - isolate->RequestInterrupt(InterruptCallback, nullptr); + if (!state->export_command.empty()) { + ExportProfile(*state); } - if (state->callbackMode & kAsyncCallback) { - uv_async_send(state->async); + + if (!state->callback.IsEmpty()) { + if (state->callbackMode & kInterruptCallback) { + isolate->RequestInterrupt(InterruptCallback, nullptr); + } + if (state->callbackMode & kAsyncCallback) { + uv_async_send(state->async); + } + } else { + state->profile.reset(); } } else { + // Drop any profile retained from an earlier invocation: it is stale, and + // nothing below is going to consume or replace it. state->profile.reset(); + fprintf(stderr, + "NearHeapLimit: heap profiler is not enabled, no allocation " + "profile to report\n"); } if (!state->isMainThread) { @@ -518,7 +553,21 @@ NAN_METHOD(HeapProfiler::StartSamplingHeapProfiler) { NAN_METHOD(HeapProfiler::StopSamplingHeapProfiler) { auto isolate = info.GetIsolate(); isolate->GetHeapProfiler()->StopSamplingHeapProfiler(); - PerIsolateData::For(isolate)->GetHeapProfilerState().reset(); + + // Uninstall explicitly rather than leaving it to ~HeapProfilerState. reset() + // only destroys the state if this is the last reference, and it need not be: + // NearHeapLimit and InterruptCallback both take a shared_ptr copy for the + // duration of the call, so a stop() reached from inside one of them (the + // near-heap-limit JS callback calling heapProfiler.stop(), say) would leave + // the state alive, the destructor unrun, and this callback still registered + // with V8 while the per-isolate slot is already empty. The next + // near-heap-limit GC would then enter NearHeapLimit with no state at all. + // Idempotent: it clears callbackInstalled. + auto& state = PerIsolateData::For(isolate)->GetHeapProfilerState(); + if (state) { + state->UninstallNearHeapLimitCallback(); + } + state.reset(); // Remove cleanup hook since profiler is explicitly stopped { @@ -541,14 +590,22 @@ NAN_METHOD(HeapProfiler::GetAllocationProfile) { if (!profile) { return Nan::ThrowError("Heap profiler is not enabled."); } - const bool allocations = state->allocations; + // A non-null profile only proves V8's sampling heap profiler is running; it + // does not imply we are the one who started it. Anything else in the process + // (the inspector's HeapProfiler.startSampling, another agent) can enable it + // without ever going through StartSamplingHeapProfiler, in which case there + // is no per-isolate state. Serve the profile without allocation stats rather + // than dereferencing an empty shared_ptr. + const bool allocations = state && state->allocations; v8::AllocationProfile::Node* root = profile->GetRootNode(); AllocationProfileNodeStatsMap allocation_stats; if (allocations) { allocation_stats = BuildAllocationStatsByNodeId(profile->GetSamples()); } - state->OnNewProfile(); + if (state) { + state->OnNewProfile(); + } info.GetReturnValue().Set(TranslateAllocationProfile( root, allocations ? &allocation_stats : nullptr)); } @@ -573,7 +630,11 @@ NAN_METHOD(HeapProfiler::MapAllocationProfile) { return Nan::ThrowError("Heap profiler is not enabled."); } - state->OnNewProfile(); + // As in GetAllocationProfile: V8's profiler may be running without us having + // started it, so there may be no per-isolate state to update. + if (state) { + state->OnNewProfile(); + } auto root = AllocationProfileNodeView::New(profile->GetRootNode()); v8::Local argv[] = {root}; @@ -668,7 +729,9 @@ NAN_MODULE_INIT(HeapProfiler::Init) { void InterruptCallback(v8::Isolate* isolate, void* data) { v8::HandleScope scope(isolate); auto state = PerIsolateData::For(isolate)->GetHeapProfilerState(); - if (!state->profile) { + // The interrupt is requested from NearHeapLimit but runs later, so + // StopSamplingHeapProfiler() may have dropped the state in between. + if (!state || !state->profile) { return; } v8::Local argv[1] = { diff --git a/ts/test/heap-foreign-sampler.ts b/ts/test/heap-foreign-sampler.ts new file mode 100644 index 00000000..e331bba1 --- /dev/null +++ b/ts/test/heap-foreign-sampler.ts @@ -0,0 +1,79 @@ +/* + * Copyright 2026 Datadog, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +// Runs in a forked process because the failure mode under test is a SIGSEGV, +// which would take the whole mocha run down with it. +// +// V8's sampling heap profiler can be enabled by anything in the process - here +// the inspector's HeapProfiler.startSampling, but equally DevTools or another +// agent. That leaves getAllocationProfile()/mapAllocationProfile() with a live +// V8 profile but no per-isolate HeapProfilerState, since only +// startSamplingHeapProfiler() creates one. Both used to dereference that empty +// shared_ptr. + +import * as inspector from 'inspector'; + +import * as v8HeapProfiler from '../src/heap-profiler-bindings'; + +function post( + session: inspector.Session, + method: string, + params?: object, +): Promise { + return new Promise((resolve, reject) => { + session.post(method, params, err => (err ? reject(err) : resolve())); + }); +} + +async function main() { + const session = new inspector.Session(); + session.connect(); + + await post(session, 'HeapProfiler.enable'); + await post(session, 'HeapProfiler.startSampling', {samplingInterval: 16384}); + + // Allocate so the sampler actually has samples to report. Kept modest and + // scoped to this function: the profile only needs a non-empty sample set. + const retained: Array<{i: number; s: string}> = []; + for (let i = 0; i < 20000; i++) { + retained.push({i, s: 'x'.repeat(16)}); + } + + // pprof never started the heap profiler, so there is no state for this + // isolate. Both of these must return rather than crash. + const profile = v8HeapProfiler.getAllocationProfile(); + if (!profile || typeof profile.name !== 'string') { + throw new Error('getAllocationProfile returned an unusable profile'); + } + + const mapped = v8HeapProfiler.mapAllocationProfile(node => node.name); + if (typeof mapped !== 'string') { + throw new Error('mapAllocationProfile did not invoke the callback'); + } + + await post(session, 'HeapProfiler.stopSampling'); + session.disconnect(); +} + +main().then( + () => process.exit(0), + err => { + console.error(err); + process.exit(1); + }, +); diff --git a/ts/test/test-heap-profiler.ts b/ts/test/test-heap-profiler.ts index 2bb2c3e4..20b62b80 100644 --- a/ts/test/test-heap-profiler.ts +++ b/ts/test/test-heap-profiler.ts @@ -330,6 +330,52 @@ describe('HeapProfiler', () => { }); }); +describe('foreign heap sampler', () => { + // Regression test: V8's sampling heap profiler can be enabled by something + // other than pprof (inspector, DevTools, another agent). getAllocationProfile + // and mapAllocationProfile then see a live V8 profile with no per-isolate + // state, and used to dereference an empty shared_ptr. Forked because the + // failure is a SIGSEGV. + it('should not crash when V8 heap sampling was enabled outside of pprof', async function () { + this.timeout(30000); + + const proc = fork(path.join(__dirname, 'heap-foreign-sampler.js'), { + silent: true, + // Under the asan CI job the child inherits LD_PRELOAD=libasan and runs + // LeakSanitizer at exit. The child ends on process.exit(), so V8's heap + // is never torn down and every live object is reported as leaked, + // failing the child for reasons that have nothing to do with what this + // test checks. ASAN itself stays on, so a genuine memory error in the + // code under test is still caught. + env: {...process.env, LSAN_OPTIONS: 'detect_leaks=0'}, + }); + let output = ''; + proc.stdout?.on('data', chunk => { + output += chunk; + }); + proc.stderr?.on('data', chunk => { + output += chunk; + }); + + await new Promise((resolve, reject) => { + proc.on('error', reject); + // 'close' rather than 'exit': it fires once the piped stdio has been + // drained, so `output` is complete when it lands in the failure message. + proc.on('close', (code, signal) => { + if (code === 0) { + resolve(); + } else { + reject( + new Error( + `heap-foreign-sampler exited with code=${code} signal=${signal}\n${output}`, + ), + ); + } + }); + }); + }); +}); + describe('OOMMonitoring', () => { it('should restore heap limit after v8 recovers from OOM', async function () { this.timeout(30000); From 26e0ca503f8fb807b714369cf18e6ed1ad539c40 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Mon, 10 Aug 2026 14:22:15 +0200 Subject: [PATCH 09/14] fix(otel-thread-ctx): don't derive CtxWrap from node::ObjectWrap (#388) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CtxWrap has the same defect #385 fixed in the wall profiler's PersistentContextPtr. node::ObjectWrap registers a per-instance environment cleanup hook in its constructor and calls RemoveEnvironmentCleanupHook from its destructor, which CHECKs that an Environment is current. A CtxWrap is owned by a weak V8 handle, so V8 picks the moment it dies, and weak callbacks run during isolate teardown with no context entered: Assertion failed: (env) != nullptr 2: node::RemoveEnvironmentCleanupHook(...) 3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap() This one is not subtle: create a few thousand ThreadContexts and exit normally and it aborts every time, on a plain release build. No ASAN needed, unlike the PCP case. Nothing below ~1000 instances reproduces it — V8 has to still have some left to collect at teardown. Note the CHECK guards something real, so it must not be worked around by skipping the removal. Environment::GetCurrent(isolate) returns null on `!isolate->InContext()` alone, so the Environment may well still be alive; leaving a hook behind whose arg is a freed pointer would turn the abort into a use-after-free when CleanupQueue::Drain later calls it. The fix is to not register the per-instance hook at all. Dropping the base loses what that hook provided: deletion at teardown even when V8 never collects the object. PCP could rely on ~WallProfiler walking its live list; CtxWrap has no such owner and owns a malloc'd record, so without a replacement this would trade an abort for a leak. Add the equivalent: a thread-local list of live CtxWraps drained by a single per-isolate cleanup hook, registered from Wrap() — inside a JS constructor call, where a context is entered, so AddEnvironmentCleanupHook is satisfied honestly — and never removed, since it fires once at teardown while the Environment is alive. One hook per isolate instead of one per instance, with removal timing we control rather than V8. With no base class, `record_` becomes CtxWrap's first member, so the published threadlocal.native_wrap_fields_offset goes from 24 to 0 and is now computed with offsetof rather than sizeof() of a foreign type. That is a reader-contract change, made now because no readers exist yet. Losing the base also makes CtxWrap standard-layout — no base subobject, no virtuals, all data members in one access section — so offsetof on it is now unconditionally valid and the two -Winvalid-offsetof suppressions the inheriting version needed are gone. A static_assert on is_standard_layout keeps it that way, since the reader contract depends on offsetof(record_) being well-defined. The two internal-field accessors move to a new internal-field.hh: Node 26 requires an EmbedderDataTypeTag on both the get and the set, and having the pair in one place stops them drifting when only one is exercised on the version you build against. wall.cc keeps its own copies for now to avoid conflicting with in-flight work there; folding those in is a follow-up. Verified on Node 20, 24 and 26, with both clang and gcc. New regression test fails with signal=SIGABRT against the pre-fix binding and passes after; ASAN exit 0 with zero leaks on 20 and 24, which is the check that the drain hook really does replace what ObjectWrap was doing. (cherry picked from commit a19664d1b8cc99d8cd54c81000fb562386f44500) --- bindings/internal-field.hh | 47 ++++++++++ bindings/otel-thread-ctx.cc | 155 +++++++++++++++++++++++++------- ts/src/otel-thread-ctx.ts | 2 +- ts/test/otel-ctx-teardown.ts | 67 ++++++++++++++ ts/test/test-otel-thread-ctx.ts | 41 ++++++++- 5 files changed, 279 insertions(+), 33 deletions(-) create mode 100644 bindings/internal-field.hh create mode 100644 ts/test/otel-ctx-teardown.ts diff --git a/bindings/internal-field.hh b/bindings/internal-field.hh new file mode 100644 index 00000000..c7f5f48c --- /dev/null +++ b/bindings/internal-field.hh @@ -0,0 +1,47 @@ +/* + * Copyright 2026 Datadog, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include +#include + +namespace dd { + +// Read and write the embedder pointer stored in an object's internal field. +// Node 26 requires an EmbedderDataTypeTag on both ends. + +inline void* GetAlignedPointerFromInternalField(v8::Object* object, int index) { +#if NODE_MAJOR_VERSION >= 26 + return object->GetAlignedPointerFromInternalField( + index, v8::kEmbedderDataTypeTagDefault); +#else + return object->GetAlignedPointerFromInternalField(index); +#endif +} + +inline void SetAlignedPointerInInternalField(v8::Local object, + int index, + void* value) { +#if NODE_MAJOR_VERSION >= 26 + object->SetAlignedPointerInInternalField( + index, value, v8::kEmbedderDataTypeTagDefault); +#else + object->SetAlignedPointerInInternalField(index, value); +#endif +} + +} // namespace dd diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index 2aaea7ba..69d7aa43 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -32,9 +32,9 @@ #include "otel-thread-ctx.hh" #include "defer.hh" +#include "internal-field.hh" #include -#include #include #include @@ -44,6 +44,7 @@ #include #include +#include #include // Single thread-local read from outside the process via TLSDESC. It @@ -105,7 +106,6 @@ static_assert(offsetof(otel_thread_ctx_nodejs_v1_t, undefined_addr) == namespace dd { namespace { -using node::ObjectWrap; using v8::Array; using v8::Context; using v8::Function; @@ -173,12 +173,33 @@ constexpr size_t MAX_ATTRS_DATA_SIZE = 640 - sizeof(OtelThreadCtxRecord); // // Layout note for the reader: `record_` is private to C++ but its byte // position within CtxWrap is part of the reader contract. It is the first -// field after the node::ObjectWrap base subobject. `capacity_` and +// field of the class, at offset zero. `capacity_` and // `truncated_` sit after `record_` purely for the writer's own // bookkeeping — the reader never touches them. -class CtxWrap : public ObjectWrap { +// Deliberately not a node::ObjectWrap. That base registers a per-instance +// environment cleanup hook in its constructor and calls +// RemoveEnvironmentCleanupHook from its destructor, which CHECKs that an +// Environment is current: +// +// node[107]: void node::RemoveEnvironmentCleanupHook(...) hooks.cc:142 +// Assertion failed: (env) != nullptr +// 3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap() +// +// A CtxWrap is owned by a weak V8 handle, so V8 chooses when it dies, and +// weak callbacks run during isolate teardown with no context entered — +// Environment::GetCurrent(isolate) returns null on `!isolate->InContext()` +// alone — so the CHECK fires and aborts. Reproducible today by creating a few +// thousand ThreadContexts and exiting normally; see the regression test. +// +// Note the CHECK is guarding something real, so this must not be worked +// around by skipping the removal: the Environment may well still be alive, +// and leaving a hook behind whose arg is a freed pointer turns an abort into +// a use-after-free at Drain(). The fix is to never register the per-instance +// hook, and to provide the teardown deletion it was giving us (see +// g_live_ctx_wraps below). +class CtxWrap { public: - ~CtxWrap() override; + ~CtxWrap(); static void Init(Local exports); CtxWrap(const CtxWrap&) = delete; @@ -208,7 +229,13 @@ class CtxWrap : public ObjectWrap { CtxWrap(OtelThreadCtxRecord* record, size_t capacity, bool truncated); - // The three fields are kept in one access section because C++ leaves + // Attach to the holder JSObject: store `this` in internal field 0 and take + // a weak handle on the holder, so V8 deletes us once it collects it. + void Wrap(Local holder); + static CtxWrap* Unwrap(Local holder); + static void WeakCallback(const v8::WeakCallbackInfo& data); + + // The fields are kept in one access section because C++ leaves // the relative layout of fields in different access controls // implementation-defined. `record_` must come first — its offset // within CtxWrap is part of the reader contract (see the @@ -238,32 +265,100 @@ class CtxWrap : public ObjectWrap { // call instead. New() doesn't need the guard because a freshly constructed // CtxWrap isn't observable to JS until New() returns. bool encoding_; + // Intrusive doubly-linked list of the CtxWraps still alive on this thread, + // threaded through g_live_ctx_wraps. `pprev_` is the address of the pointer + // currently referencing us, so unlinking needs no head/non-head branch; + // `pprev_ == nullptr` is the "already detached" sentinel set by the drain + // hook before it deletes us. Same shape as WallProfiler's PCP list. + CtxWrap** pprev_; + CtxWrap* next_; + // Weak handle on the holder object; owns this CtxWrap. + v8::Global handle_; }; // Pin the offset of `record_` — the field the reader walks to from the -// JSObject's internal field 0. We document it as "the first field after -// the node::ObjectWrap base subobject", so equality with -// sizeof(node::ObjectWrap) is the invariant. `offsetof` on a non- -// standard-layout type (CtxWrap has private fields and inherits from -// ObjectWrap) is conditionally supported per the standard but accepted -// by every compiler this addon targets; suppress -Winvalid-offsetof so -// the static_assert compiles cleanly under strict warning flags. -#pragma GCC diagnostic push -#pragma GCC diagnostic ignored "-Winvalid-offsetof" -static_assert(offsetof(CtxWrap, record_) == sizeof(node::ObjectWrap), - "record_ must be the first field after the ObjectWrap base " - "subobject"); -#pragma GCC diagnostic pop +// JSObject's internal field 0. With no base class it is simply the first +// member, so the offset is zero and the published +// `threadlocal.native_wrap_fields_offset` is computed from this. +static_assert(std::is_standard_layout::value, + "CtxWrap must stay standard-layout: the reader contract depends " + "on offsetof(record_) being well-defined"); +static_assert(offsetof(CtxWrap, record_) == 0, + "record_ must be the first field of CtxWrap"); + +// Head of the live-CtxWrap list for this thread. Node pins each isolate to a +// thread, and CtxWraps are only ever constructed and destroyed on their own +// isolate's thread, so a thread-local needs no lock — the same reasoning the +// wall profiler uses for its active-profiler pointer. +// `otel_thread_ctx_nodejs_v1` above is thread-local for the same reason. +thread_local CtxWrap* g_live_ctx_wraps = nullptr; +// Whether DrainLiveCtxWraps is registered for the current isolate. Cleared by +// the drain itself so an isolate torn down and re-created on the same thread +// re-registers, matching how `undefined_addr` gates ResetDiscoveryStruct. +thread_local bool g_drain_hook_registered = false; + +// Delete every CtxWrap V8 has not collected yet. This is the teardown +// deletion that node::ObjectWrap's per-instance cleanup hook used to provide; +// without it the records would simply leak at exit. Registered once per +// isolate from Wrap(), which runs inside a JS constructor call where a +// context is entered, so AddEnvironmentCleanupHook's own CHECK is satisfied, +// and never removed — it fires exactly once, at teardown, while the +// Environment is still alive. +void DrainLiveCtxWraps(void* /*arg*/) { + CtxWrap* p = g_live_ctx_wraps; + while (p != nullptr) { + CtxWrap* next = p->next_; + p->pprev_ = nullptr; + p->next_ = nullptr; + delete p; + p = next; + } + g_live_ctx_wraps = nullptr; + g_drain_hook_registered = false; +} CtxWrap::~CtxWrap() { + // pprev_ != nullptr means we are still on the live list, i.e. V8 collected + // the holder and we got here from WeakCallback. If it is null the drain hook + // is walking the list and has already detached us. + if (pprev_ != nullptr) { + *pprev_ = next_; + if (next_ != nullptr) next_->pprev_ = pprev_; + } free(record_); } +void CtxWrap::WeakCallback(const v8::WeakCallbackInfo& data) { + delete data.GetParameter(); +} + +void CtxWrap::Wrap(Local holder) { + Isolate* isolate = Isolate::GetCurrent(); + if (!g_drain_hook_registered) { + node::AddEnvironmentCleanupHook(isolate, DrainLiveCtxWraps, nullptr); + g_drain_hook_registered = true; + } + SetAlignedPointerInInternalField(holder, 0, this); + handle_.Reset(isolate, holder); + handle_.SetWeak(this, &WeakCallback, v8::WeakCallbackType::kParameter); + next_ = g_live_ctx_wraps; + pprev_ = &g_live_ctx_wraps; + if (next_ != nullptr) next_->pprev_ = &next_; + g_live_ctx_wraps = this; +} + +CtxWrap* CtxWrap::Unwrap(Local holder) { + if (holder->InternalFieldCount() < 1) return nullptr; + return static_cast(GetAlignedPointerFromInternalField(*holder, 0)); +} + CtxWrap::CtxWrap(OtelThreadCtxRecord* record, size_t capacity, bool truncated) : record_(record), capacity_(capacity), truncated_(truncated), - encoding_(false) {} + encoding_(false), + pprev_(nullptr), + next_(nullptr) {} // Copy exactly `expected_bytes` bytes out of a JS Uint8Array (or subclass // such as Buffer) into `out`. Returns false if the value isn't a @@ -445,7 +540,7 @@ void CtxWrap::Append(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); Local context = isolate->GetCurrentContext(); - CtxWrap* self = ObjectWrap::Unwrap(args.This()); + CtxWrap* self = CtxWrap::Unwrap(args.This()); if (!self) { isolate->ThrowError("not a ThreadContext"); return; @@ -554,7 +649,7 @@ void CtxWrap::Append(const FunctionCallbackInfo& args) { // still exposing the finished span. Idempotent; safe to call multiple // times. void CtxWrap::Invalidate(const FunctionCallbackInfo& args) { - CtxWrap* self = ObjectWrap::Unwrap(args.This()); + CtxWrap* self = CtxWrap::Unwrap(args.This()); if (!self) { args.GetIsolate()->ThrowError("not a ThreadContext"); return; @@ -568,7 +663,7 @@ void CtxWrap::Invalidate(const FunctionCallbackInfo& args) { // CtxWrap::New() if the initial set didn't fit, or by any subsequent // CtxWrap::Append() call. void CtxWrap::IsTruncated(const FunctionCallbackInfo& args) { - CtxWrap* self = ObjectWrap::Unwrap(args.This()); + CtxWrap* self = CtxWrap::Unwrap(args.This()); if (!self) { args.GetIsolate()->ThrowError("not a ThreadContext"); return; @@ -581,7 +676,7 @@ void CtxWrap::IsTruncated(const FunctionCallbackInfo& args) { // API; intended for tests and out-of-process-reader development. void CtxWrap::DebugBytes(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); - CtxWrap* self = ObjectWrap::Unwrap(args.This()); + CtxWrap* self = CtxWrap::Unwrap(args.This()); if (!self) { isolate->ThrowError("not a ThreadContext"); return; @@ -702,13 +797,13 @@ constexpr int WRAPPED_OBJECT_OFFSET = 0; #endif constexpr int TAGGED_SIZE = v8::internal::kApiTaggedSize; -// sizeof(node::ObjectWrap). Given a pointer to a CtxWrap — or any other -// ObjectWrap-derived C++ object attached to a JSObject via the V8 -// wrapped-object slot — add this offset to reach the derived class's own -// fields. For CtxWrap, that's `record_` (see the static_assert on its -// offset above). +// Given a pointer to a CtxWrap — reached from the JSObject's V8 +// wrapped-object slot — add this offset to arrive at `record_`. CtxWrap has +// no base class, so `record_` is its first member and the offset is zero; +// computing it with offsetof keeps the published value correct if the layout +// ever changes again. constexpr int NATIVE_WRAP_FIELDS_OFFSET = - static_cast(sizeof(node::ObjectWrap)); + static_cast(offsetof(CtxWrap, record_)); // V8 JSMap layout: kTableOffset within the JSMap object holds a tagged // pointer to the backing OrderedHashMap table. Not exposed in V8's diff --git a/ts/src/otel-thread-ctx.ts b/ts/src/otel-thread-ctx.ts index b5d9e75b..f59a976b 100644 --- a/ts/src/otel-thread-ctx.ts +++ b/ts/src/otel-thread-ctx.ts @@ -134,7 +134,7 @@ const SCHEMA_VERSION = 'nodejs_v1_dev'; // consistent in shape. let WRAPPED_OBJECT_OFFSET = 24; let TAGGED_SIZE = 8; -let NATIVE_WRAP_FIELDS_OFFSET = 24; +let NATIVE_WRAP_FIELDS_OFFSET = 0; let JS_MAP_TABLE_OFFSET = 0x18; let ORDERED_HASH_MAP_HEADER_SIZE = 0x10; diff --git a/ts/test/otel-ctx-teardown.ts b/ts/test/otel-ctx-teardown.ts new file mode 100644 index 00000000..1780b73f --- /dev/null +++ b/ts/test/otel-ctx-teardown.ts @@ -0,0 +1,67 @@ +/* + * Copyright 2026 Datadog, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +// Runs in a forked process: the failure mode under test is a SIGABRT, which +// would take the whole mocha run down. +// +// When CtxWrap derived from node::ObjectWrap, a CtxWrap collected during +// isolate teardown ran ~ObjectWrap -> RemoveEnvironmentCleanupHook, which +// CHECKs that an Environment is current. It is not, during teardown, so the +// process aborted: +// +// Assertion failed: (env) != nullptr +// 3: otel_thread_ctx_nodejs::CtxWrap::~CtxWrap() +// +// It needs enough instances that V8 still has some left to collect at +// teardown — nothing below ~1000 reproduced it — hence the count here. + +import {otelThreadCtx} from '../src/index'; + +const N = 3000; + +function id(n: number, len: number): Uint8Array { + const b = new Uint8Array(len); + b[0] = (n >> 24) & 0xff; + b[1] = (n >> 16) & 0xff; + b[2] = (n >> 8) & 0xff; + b[3] = n & 0xff; + return b; +} + +const retained: unknown[] = []; + +for (let i = 0; i < N; i++) { + const ctx = new otelThreadCtx.ThreadContext(id(i, 16), id(i, 8), [ + 'k', + String(i), + ]); + if (i % 4 === 0) { + // Still strongly reachable at exit. + retained.push(ctx); + } else { + // Reachable only through the async context frame; collectable whenever + // V8 decides, including during teardown. + ctx.enter(); + } +} + +(globalThis as unknown as {__retained: unknown}).__retained = retained; + +// Exit through the normal path, so the Environment is torn down and the +// isolate disposed. That is where the weak callbacks in question fire. +console.log(`created ${N}, retained ${retained.length}`); diff --git a/ts/test/test-otel-thread-ctx.ts b/ts/test/test-otel-thread-ctx.ts index d19167d1..f26af9db 100644 --- a/ts/test/test-otel-thread-ctx.ts +++ b/ts/test/test-otel-thread-ctx.ts @@ -26,7 +26,7 @@ import assert from 'assert'; import {strict as strictAssert} from 'assert'; -import {spawnSync} from 'node:child_process'; +import {fork, spawnSync} from 'node:child_process'; import {existsSync} from 'node:fs'; import {join} from 'node:path'; @@ -151,6 +151,43 @@ function captureBytes(opts: { (isLinux && isAsyncContextFrameAvailable ? describe : describe.skip)( 'OTEP-4947 thread context (Linux-only)', () => { + describe('isolate teardown', () => { + // Regression test: CtxWrap used to derive from node::ObjectWrap, whose + // destructor calls RemoveEnvironmentCleanupHook. A CtxWrap collected + // during isolate teardown hit that function's CHECK that an Environment + // is current and aborted the process. Forked, because the failure is a + // SIGABRT rather than a test failure. + it('should not abort when contexts are collected during teardown', async function () { + this.timeout(60000); + + const proc = fork(join(__dirname, 'otel-ctx-teardown.js'), { + silent: true, + }); + let output = ''; + proc.stdout?.on('data', chunk => { + output += chunk; + }); + proc.stderr?.on('data', chunk => { + output += chunk; + }); + + await new Promise((resolve, reject) => { + proc.on('error', reject); + proc.on('close', (code, signal) => { + if (code === 0) { + resolve(); + } else { + reject( + new Error( + `otel-ctx-teardown exited with code=${code} signal=${signal}\n${output}`, + ), + ); + } + }); + }); + }); + }); + describe('ThreadContext construction', () => { it('accepts Uint8Array trace and span IDs', () => { const bytes = captureBytes({ @@ -770,7 +807,7 @@ function captureBytes(opts: { strictAssert.deepEqual(pca['threadlocal.attribute_key_map'], keys); strictAssert.equal(pca['threadlocal.wrapped_object_offset'], 24); strictAssert.equal(pca['threadlocal.tagged_size'], 8); - strictAssert.equal(pca['threadlocal.native_wrap_fields_offset'], 24); + strictAssert.equal(pca['threadlocal.native_wrap_fields_offset'], 0); strictAssert.equal(pca['threadlocal.js_map_table_offset'], 0x18); strictAssert.equal( pca['threadlocal.ordered_hash_map_header_size'], From 2435668e940828b195b4e07d06a09ff049d541f5 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Tue, 11 Aug 2026 11:03:03 +0200 Subject: [PATCH 10/14] fix(otel-thread-ctx): don't assert a record is valid when growing it (#392) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Append's reallocate path asserted that the record it had just copied was valid: memcpy(new_rec.get(), self->record_, ...); ... assert(new_rec->valid == 1); invalidate() sets that byte to 0, and appending afterwards is supported — there is a test for it — so the assert fires on any append too large to fit in place: Assertion `new_rec->valid == 1' failed. Aborted (exit 134) This is not debug-only. NDEBUG is never defined for this addon, so assert() is live in Release too; both configurations abort. Reproduced through the public API on Linux with invalidate() followed by a 200-byte attribute. 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, so an attribute over ~34 bytes on a fresh record is enough. The existing 'appendAttributes after invalidate' test appends 6 bytes, takes the in-place path, and so never reached the copy — right behaviour, wrong size. Assert what the check was actually for — that the memcpy carried the header across intact — by capturing the source's valid byte first and comparing against that. Still catches a genuine copy bug, such as shortening the memcpy so it no longer covers the header, and is correct whether the record is valid or not. The regression test forks, since the failure is an abort that would otherwise take the whole mocha run down. Verified it bites: against the pre-fix binding it reports signal=SIGABRT with the assertion above, and passes after. Reported by @nsavoire on #391. (cherry picked from commit cfa8cd11504d4aaa2581fa61d83fbeb4ec3fa12d) --- bindings/otel-thread-ctx.cc | 11 ++++-- ts/test/otel-invalidate-append.ts | 57 +++++++++++++++++++++++++++++++ ts/test/test-otel-thread-ctx.ts | 39 +++++++++++++++++++++ 3 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 ts/test/otel-invalidate-append.ts diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index 69d7aa43..15477a11 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -614,14 +614,21 @@ void CtxWrap::Append(const FunctionCallbackInfo& args) { isolate->ThrowError("allocation failed"); return; } + // Capture before the copy: the point of the assert below is that the memcpy + // carried the header across intact, not that the record is valid. It used to + // assert `valid == 1`, which invalidate() legitimately makes false — and + // since NDEBUG is not defined for this addon, that aborted release builds + // too, not just debug ones. + const uint8_t src_valid = self->record_->valid; // Copy the existing record (header + already-written attrs_data). memcpy( new_rec.get(), self->record_, sizeof(OtelThreadCtxRecord) + current_used); // Append the new entries and update attrs_data_size. memcpy(&new_rec->attrs_data[current_used], appended.data(), appended.size()); new_rec->attrs_data_size = static_cast(new_used); - // The copy should've preserved valid=1 from the source record. - assert(new_rec->valid == 1); + // The copy should've carried the source record's header across verbatim, + // whatever its validity was. + assert(new_rec->valid == src_valid); // Publish: the pointer swap is the atomic boundary the reader sees. The // first fence keeps the new_rec content writes ordered before the pointer diff --git a/ts/test/otel-invalidate-append.ts b/ts/test/otel-invalidate-append.ts new file mode 100644 index 00000000..9d12dc16 --- /dev/null +++ b/ts/test/otel-invalidate-append.ts @@ -0,0 +1,57 @@ +/* + * Copyright 2026 Datadog, Inc + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +'use strict'; + +// Runs in a forked process because the failure mode is an abort, which would +// take the whole mocha run down with it. +// +// Append's reallocate path used to assert that the copied record had +// `valid == 1`. invalidate() sets that byte to 0 and appending afterwards is +// supported, so an append too large to fit in place aborted the process: +// +// Assertion `new_rec->valid == 1' failed. +// +// 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 the value below is +// comfortably past it. + +import assert from 'assert'; + +import {otelThreadCtx} from '../src/index'; + +const VALUE = 'x'.repeat(200); + +const ctx = new otelThreadCtx.ThreadContext( + Buffer.alloc(16, 1), + Buffer.alloc(8, 2), +); + +ctx.run(() => { + ctx.invalidate(); + ctx.appendAttributes([VALUE]); + + const bytes = ctx.debugBytes(); + const attrsDataSize = bytes[26] | (bytes[27] << 8); + + // invalidate() must stick: growing the record does not resurrect it. + assert.strictEqual(bytes[24], 0, 'valid byte should still be 0'); + // key index (1) + length (1) + the value itself. + assert.strictEqual(attrsDataSize, VALUE.length + 2, 'attrs_data_size'); +}); + +console.log('ok'); diff --git a/ts/test/test-otel-thread-ctx.ts b/ts/test/test-otel-thread-ctx.ts index f26af9db..f4d6683b 100644 --- a/ts/test/test-otel-thread-ctx.ts +++ b/ts/test/test-otel-thread-ctx.ts @@ -770,6 +770,45 @@ function captureBytes(opts: { }); }); + describe('invalidate then grow', () => { + // Regression test: Append's reallocate path asserted that the copied + // record had valid == 1, which invalidate() legitimately makes false, so + // an append too large to fit in place aborted the process. NDEBUG is not + // defined for this addon, so that hit release builds too. The sibling + // test above appends 6 bytes, which fits the initial 36-byte capacity and + // is written in place, so it never reached the copy. Forked, because the + // failure is an abort rather than a test failure. + it('should survive an append that reallocates after invalidate', async function () { + this.timeout(30000); + + const proc = fork(join(__dirname, 'otel-invalidate-append.js'), { + silent: true, + }); + let output = ''; + proc.stdout?.on('data', chunk => { + output += chunk; + }); + proc.stderr?.on('data', chunk => { + output += chunk; + }); + + await new Promise((resolve, reject) => { + proc.on('error', reject); + proc.on('close', (code, signal) => { + if (code === 0) { + resolve(); + } else { + reject( + new Error( + `otel-invalidate-append exited with code=${code} signal=${signal}\n${output}`, + ), + ); + } + }); + }); + }); + }); + describe('getProcessContextAttributes', () => { it('rejects non-array keys', () => { strictAssert.throws( From 795dd05d10ca3be46b99c58283542dbdc030efb1 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Tue, 11 Aug 2026 11:03:19 +0200 Subject: [PATCH 11/14] Follow up on #388 review comments (#391) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Follow up on #388 review comments Three review nits from #388, all valid. Deduplicate the internal-field accessors. wall.cc had its own copies of GetAlignedPointerFromInternalField / SetAlignedPointerInInternalField; #388 added the same pair in internal-field.hh and deliberately left wall.cc alone to avoid conflicting with #387, which was in flight. #387 has landed, so wall.cc now includes the header and its copies are gone. Same namespace and names, so every call site is unchanged. Register the drain hook in Init() rather than lazily on first Wrap(), which removes g_drain_hook_registered entirely. Module initialisation always runs with a context entered, so AddEnvironmentCleanupHook's CHECK is satisfied there too, and Init() runs exactly once per isolate — which is the lifetime the hook should match. The flag existed only to make the lazy registration idempotent and to re-arm after an isolate was torn down and recreated on the same thread; Init() running again on the new isolate covers that by construction. Clear the holder's internal field before freeing the CtxWrap it points at. The drain hook now nulls slot 0 on its way through the list. This matters more than a tidiness nit: that slot is exactly what the out-of-process OTEP-4947 reader walks, so leaving it pointing at freed memory is a loaded gun aimed at a consumer we do not control. Being on the live list means V8 has not collected the holder, so reading the handle there is safe; the WeakCallback path cannot do this and does not need to, since there the holder is the object being collected. Verified on Node 20, 24 and 26: ASAN exit 0 with zero leaks and zero aborts on 20 and 24, 165 passing on 24 and 26, the teardown regression test passing where the OTEP block runs, the original repro clean at N=3000 and N=10000, and the published native_wrap_fields_offset still 0. * Add the zero-out-internal-field logic to PCP too (cherry picked from commit 6c741081ae8360394b8721205bd8ddbc10eacdc8) --- bindings/otel-thread-ctx.cc | 38 +++++++++++++++++++------------------ bindings/profilers/wall.cc | 25 +++++------------------- 2 files changed, 25 insertions(+), 38 deletions(-) diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index 15477a11..cbe34d51 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -292,29 +292,34 @@ static_assert(offsetof(CtxWrap, record_) == 0, // wall profiler uses for its active-profiler pointer. // `otel_thread_ctx_nodejs_v1` above is thread-local for the same reason. thread_local CtxWrap* g_live_ctx_wraps = nullptr; -// Whether DrainLiveCtxWraps is registered for the current isolate. Cleared by -// the drain itself so an isolate torn down and re-created on the same thread -// re-registers, matching how `undefined_addr` gates ResetDiscoveryStruct. -thread_local bool g_drain_hook_registered = false; - -// Delete every CtxWrap V8 has not collected yet. This is the teardown -// deletion that node::ObjectWrap's per-instance cleanup hook used to provide; -// without it the records would simply leak at exit. Registered once per -// isolate from Wrap(), which runs inside a JS constructor call where a -// context is entered, so AddEnvironmentCleanupHook's own CHECK is satisfied, -// and never removed — it fires exactly once, at teardown, while the -// Environment is still alive. -void DrainLiveCtxWraps(void* /*arg*/) { + +// Delete every CtxWrap V8 has not collected yet. This is the teardown deletion +// that node::ObjectWrap's per-instance cleanup hook used to provide; without it +// the records would simply leak at exit. Registered once per isolate from +// Init(), which runs at module initialisation with a context entered, so +// AddEnvironmentCleanupHook's own CHECK is satisfied, and never removed — it +// fires exactly once, at teardown, while the Environment is still alive. +void DrainLiveCtxWraps(void* arg) { + auto* isolate = static_cast(arg); + v8::HandleScope scope(isolate); CtxWrap* p = g_live_ctx_wraps; while (p != nullptr) { CtxWrap* next = p->next_; p->pprev_ = nullptr; p->next_ = nullptr; + // Clear the holder's internal field (containing p as pointer value), so + // nothing can reach a dangling CtxWrap through it including the + // out-of-process reader, which walks this slot. Being on the live list + // means V8 has not collected the holder, so the handle is safe to read + // here; the WeakCallback path cannot do this and does not need to, + // since there the holder is the thing being collected. + if (!p->handle_.IsEmpty()) { + SetAlignedPointerInInternalField(p->handle_.Get(isolate), 0, nullptr); + } delete p; p = next; } g_live_ctx_wraps = nullptr; - g_drain_hook_registered = false; } CtxWrap::~CtxWrap() { @@ -334,10 +339,6 @@ void CtxWrap::WeakCallback(const v8::WeakCallbackInfo& data) { void CtxWrap::Wrap(Local holder) { Isolate* isolate = Isolate::GetCurrent(); - if (!g_drain_hook_registered) { - node::AddEnvironmentCleanupHook(isolate, DrainLiveCtxWraps, nullptr); - g_drain_hook_registered = true; - } SetAlignedPointerInInternalField(holder, 0, this); handle_.Reset(isolate, holder); handle_.SetWeak(this, &WeakCallback, v8::WeakCallbackType::kParameter); @@ -697,6 +698,7 @@ void CtxWrap::DebugBytes(const FunctionCallbackInfo& args) { void CtxWrap::Init(Local exports) { Isolate* isolate = Isolate::GetCurrent(); + node::AddEnvironmentCleanupHook(isolate, DrainLiveCtxWraps, isolate); Local context = isolate->GetCurrentContext(); Local tpl = FunctionTemplate::New(isolate, New); diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index 6f33179b..fe85c876 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -27,6 +27,7 @@ #include #include +#include "internal-field.hh" #include "map-get.hh" #include "per-isolate-data.hh" #include "translate-time-profile.hh" @@ -106,26 +107,6 @@ void SetContextPtr(ContextPtr& contextPtr, } } -inline void* GetAlignedPointerFromInternalField(Object* object, int index) { -#if NODE_MAJOR_VERSION >= 26 - return object->GetAlignedPointerFromInternalField( - index, kEmbedderDataTypeTagDefault); -#else - return object->GetAlignedPointerFromInternalField(index); -#endif -} - -inline void SetAlignedPointerInInternalField(Local object, - int index, - void* value) { -#if NODE_MAJOR_VERSION >= 26 - object->SetAlignedPointerInInternalField( - index, value, kEmbedderDataTypeTagDefault); -#else - object->SetAlignedPointerInInternalField(index, value); -#endif -} - // Deliberately not a node::ObjectWrap. That base registers a per-instance // environment cleanup hook in its constructor and calls // RemoveEnvironmentCleanupHook from its destructor, which CHECKs that the @@ -720,10 +701,14 @@ WallProfiler::~WallProfiler() { // internal-field pointer in the wrap object stays inert even if V8 later // GCs the wrap.) auto* p = liveContextPtrHead_; + auto isolate = Isolate::GetCurrent(); while (p != nullptr) { auto* next = p->next_; p->pprev_ = nullptr; p->next_ = nullptr; + if (isolate != nullptr && !p->handle_.IsEmpty()) { + SetAlignedPointerInInternalField(p->handle_.Get(isolate), 0, nullptr); + } delete p; p = next; } From 96b1722f7f4dd650667771ca26d5ca8ed57d77ec Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Tue, 11 Aug 2026 11:54:03 +0200 Subject: [PATCH 12/14] chore(deps): bump pprof-format to 2.3.1 (#393) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Note this is a metadata-only release: diffing the published 2.3.0 and 2.3.1 tarballs, package.json is the only file that differs. pprof-format has no runtime dependencies, and 2.3.1 carries a version bump, two devDependency bumps, and an `overrides` block patching brace-expansion and linkify-it in its own dev tree. A dependency's `overrides` are ignored by npm — only the root project's apply — so none of that reaches consumers. So this changes no shipped code and fixes no vulnerability we are exposed to. It keeps us on the current release and off tooling's "behind latest" reports, which is the whole of it. `^2.3.0` already admitted 2.3.1, so the substantive part is the lockfile pin; the range is moved in step so the declared floor matches what we test against. Suite: 115 passing. (cherry picked from commit 93531f4d10e695c87e7d948d7bea0493e67b7b5c) --- package-lock.json | 8 ++++---- package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index ec4d7ae6..49b0cbd7 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,7 +10,7 @@ "license": "Apache-2.0", "dependencies": { "node-gyp-build": "^4.8.4", - "pprof-format": "^2.3.0", + "pprof-format": "^2.3.1", "source-map": "^0.8.0" }, "devDependencies": { @@ -5062,9 +5062,9 @@ } }, "node_modules/pprof-format": { - "version": "2.3.0", - "resolved": "https://registry.npmjs.org/pprof-format/-/pprof-format-2.3.0.tgz", - "integrity": "sha512-ovChRLoV4H3k0zKWq0AXewtnuPGskzBLjBPmF2N0DZu+H65PYuo51+6e/1GTb8Olm7oC0xvhhLdqPL4/XhCl4A==", + "version": "2.3.1", + "resolved": "https://registry.npmjs.org/pprof-format/-/pprof-format-2.3.1.tgz", + "integrity": "sha512-y51Z83qG2vEQBACPu6lkGFREVkHwQaCaNDdSFEMLIqSo3bmpADsbP6J3F2SSk7tYB741oTQ9Kt5YAdQsmiCRkA==", "license": "MIT" }, "node_modules/prelude-ls": { diff --git a/package.json b/package.json index ab06b461..88a06b10 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,7 @@ "license": "Apache-2.0", "dependencies": { "node-gyp-build": "^4.8.4", - "pprof-format": "^2.3.0", + "pprof-format": "^2.3.1", "source-map": "^0.8.0" }, "devDependencies": { From 4345be531967e94beff8a8ec44d953818e049761 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Tue, 16 Jun 2026 11:42:33 +0200 Subject: [PATCH 13/14] Group patch and minor Dependabot updates for easier approval/merging (#352) (cherry picked from commit 0089e1e83c688edcbe64edbe20d4811b95a677d1) --- .github/dependabot.yml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 0f9d0319..cdf3668e 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -27,3 +27,10 @@ updates: - dependencies - javascript - semver-patch + groups: + patch-updates: + update-types: + - "patch" + minor-updates: + update-types: + - "minor" From 5da183fe7dcb52aa38110180afe748a22ff8f9dc Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Tue, 11 Aug 2026 12:27:26 +0200 Subject: [PATCH 14/14] v5.18.0 --- package-lock.json | 4 ++-- package.json | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package-lock.json b/package-lock.json index 49b0cbd7..bbfc19ac 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "@datadog/pprof", - "version": "5.17.0", + "version": "5.18.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "@datadog/pprof", - "version": "5.17.0", + "version": "5.18.0", "license": "Apache-2.0", "dependencies": { "node-gyp-build": "^4.8.4", diff --git a/package.json b/package.json index 88a06b10..9092580a 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "@datadog/pprof", - "version": "5.17.0", + "version": "5.18.0", "description": "pprof support for Node.js", "repository": { "type": "git",