fix(heap): don't assume a per-isolate state exists - #384
Merged
Conversation
szegedi
requested review from
IlyasShabi,
nsavoire and
r1viollet
as code owners
August 6, 2026 14:38
Overall package sizeSelf size: 2.48 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | pprof-format | 2.3.0 | 503.97 kB | 503.97 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 |
|
🔗 Commit SHA: 85f19f4 | Docs | Datadog PR Page | Give us feedback! |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
nsavoire
reviewed
Aug 6, 2026
There was a problem hiding this comment.
Might be worth checking for null profile
Author
There was a problem hiding this comment.
Fair enough. It's probably reachable with the callback still installed between HeapProfilerCleanupHook and the isolate going away; we're registered with nothing to sample. I only skipped the profile-dependent work so the heap-limit extension still happens. I couldn't find a non-flaky way to test this (it needs teardown plus heap pressure).
nsavoire
reviewed
Aug 6, 2026
| auto state = PerIsolateData::For(isolate)->GetHeapProfilerState(); | ||
|
|
||
| if (!state) { | ||
| // StopSamplingHeapProfiler() resets the state but cannot uninstall this |
There was a problem hiding this comment.
I read the comment several times, but I cannot make sense of it.
the object that tracked its installation is what it just dropped
StopSamplingHeapProfiler resets the state shared_ptr, which destroys it and uninstalls the callback
Author
There was a problem hiding this comment.
Yup, you're right about destructor uninstalling the callback. I moved uninstalling to happen first inside ~HeapProfilerState though. It previously called V8's StopSamplingHeapProfiler before uninstalling, and by then the PerIsolateData slot is already empty, so a GC in that window would have reached NearHeapLimit with no state. The nonsensical comment is gone too :-)
szegedi
added a commit
that referenced
this pull request
Aug 7, 2026
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>
IlyasShabi
previously approved these changes
Aug 7, 2026
nsavoire
reviewed
Aug 7, 2026
nsavoire
reviewed
Aug 7, 2026
| state->profile.reset(); | ||
| } | ||
| } else { | ||
| state->profile.reset(); |
There was a problem hiding this comment.
nit: keep state->profile.reset();, it should not hurt
There was a problem hiding this comment.
nit: reset() may not trigger destructor if a shared_ptr copy keep the state alive (eg. in NearHeapLimit or InterruptCallback), V8 could call NearHeapLimit with an empty state. To be safe we could always uninstall the callback at this point (idempotent: it clears callbackInstalled).
Suggested change
| auto& state = PerIsolateData::For(isolate)->GetHeapProfilerState(); | |
| if (state) { | |
| state->UninstallNearHeapLimitCallback(); | |
| } | |
| state.reset(); |
Author
There was a problem hiding this comment.
Got it.
Claude tells me that an analogous copy-outlives-slot logic now applies to NearHeapLimit.
Should we re-add the null guard we removed in 0ede06e? The rationale is different, though and instead of calling RemoveNearHeapLimitCallback we'd just return current_heap_limit. This should avoid a very narrow abort during shutdown, but we might still avoid getting a customer issue about it sometime. I'd add it but since you once raised it as something to remove I'd rather run it by you. Basically,
if (!state) {
return current_heap_limit;
}
in NearHeapLimit before trying to dereference state.
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>
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>
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>
… 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.
szegedi
force-pushed
the
szegedi/fix-heap-profiler-null-state
branch
from
August 7, 2026 11:26
c1bd24c to
b7ba01c
Compare
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.
nsavoire
approved these changes
Aug 7, 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
3 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.
What
HeapProfiler::GetAllocationProfileandHeapProfiler::MapAllocationProfiledereference the per-isolateHeapProfilerStateafter checking only that V8 returned a profile:A non-null profile only proves V8's sampling heap profiler is running — not that 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 ownStartSamplingHeapProfilercreates the state. The guard then passes with an emptyshared_ptrand we segfault.Why it regressed
Both sites were null-checked through 5.14.4. In 5.15.0
MonitorOutOfMemorychanged from unconditionally replacing the state to reusing an existing one, which made "state already exists" the normal case; the checks were dropped along the way.MapAllocationProfilestill null-checksstateone line above the unguardedOnNewProfile()call, which is a good hint that this was accidental.Restores the pre-5.15.0 behaviour of serving the profile without allocation stats rather than throwing — V8's profiler genuinely is enabled, so
"Heap profiler is not enabled."would be the wrong error.Also fixed
Two pre-existing instances of the same assumption, both reachable because
StopSamplingHeapProfiler()resets the state:NearHeapLimitreadstate->insideCallbackunguarded. The state that recorded the callback's installation is exactly what got dropped, so nothing could uninstall it. Now removes the callback and leaves the heap limit alone so V8 does its normal OOM handling.InterruptCallbackis requested fromNearHeapLimitbut runs later, so the state can disappear in between.Testing
New
ts/test/heap-foreign-sampler.ts, forked fromtest-heap-profiler.tsbecause the failure mode is a SIGSEGV that would otherwise take the whole mocha run down — same pattern as the existingOOMMonitoringtests. It enables V8 heap sampling via the inspector and then calls both entry points.Built the unfixed native code first to confirm the test bites:
Full suite 113 passing / 0 failing,
clang-format -Werrorclean,gts check0 errors.Context
Found while investigating a customer crash on Node.js 20.18.3 / Alpine musl x64 with dd-trace 5.117.0 (
@datadog/pprof5.17.0), reported as appearing in 5.15.0+ but not 5.14.4. This is the only defect in the 5.15.0 delta that produces a SIGSEGV, is reachable on Node 20, and still exists in 5.17.0 —#341's PCP list needs CPED (Node >= 22.9) and#343's allocation profiler needs Node >= 26.It is not confirmed to be that customer's crash: it requires a co-tenant enabling V8 heap sampling, and dd-trace never does so itself. The fix stands on its own regardless.
Jira: PROF-15670