v5.18.0 - #394
Merged
Merged
Conversation
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 typescript-eslint/typescript-eslint#10940 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit aac1e50)
) * 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) <noreply@anthropic.com> * 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 f66e514)
…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] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> (cherry picked from commit 11415b4)
) 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 cbcfa11)
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 ee8559d)
(cherry picked from commit 53e2899)
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 9c00d10)
* 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<v8::AllocationProfile> 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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
(cherry picked from commit 5478bf2)
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 a19664d)
…392) 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 cfa8cd1)
* 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 6c74108)
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 93531f4)
szegedi
requested review from
IlyasShabi,
nsavoire and
r1viollet
as code owners
August 11, 2026 10:07
Overall package sizeSelf size: 2.49 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 |
|
szegedi
force-pushed
the
v5.18.0-proposal
branch
from
August 11, 2026 10:30
b7ae547 to
5da183f
Compare
IlyasShabi
approved these changes
Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
New features
Bug fixes
Other (build, dev)
Minor bump: #382 and #383 are
semver-minor; the remaining eleven aresemver-patch.Some earlier 5.x releases were squash-merged rather than rebased, so
branch-diffreports false positives for older commits. The cutoff used here is #378 (73d1050on main,ba1cabbon v5.x) — the newest main commit whose PR is already present on v5.x; everything older was treated as already released.That heuristic was verified rather than assumed: diffing this branch against
mainshowed the only real gap it hid was #352, whose grouped patch/minor Dependabot config had never landed on v5.x. It is included above, sogit diff main v5.18.0-proposalis now empty apart from the version inpackage.json/package-lock.json.