From 21e2707235b54ddb39ed9de6f31f986493a79266 Mon Sep 17 00:00:00 2001 From: Attila Szegedi Date: Thu, 20 Aug 2026 15:37:35 +0200 Subject: [PATCH] fix(wall): don't touch V8 handles in ~WallProfiler MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Terminating a worker thread with profiling enabled aborts the process: FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope 4: v8::HandleScope::CreateHandle(v8::internal::Isolate*, unsigned long) 5: dd::WallProfiler::~WallProfiler() [dd_pprof.node] 6: dd::WallProfiler::CleanupHook(void*) [dd_pprof.node] 7: node::CleanupQueue::Drain() 8: node::Environment::RunCleanup() 9: node::FreeEnvironment(node::Environment*) 10: node::worker::Worker::Run() Reported as DataDog/dd-trace-js#9891: a terminated worker (worker.terminate(), or process.exit() with a live worker) kills the process with SIGABRT/SIGSEGV. It bit drizzle-kit migrate, which runs migrations in a worker and terminates it, so deploy containers started failing after a successful migration. The fatal prints twice because OnFatalError -> DumpJavaScriptBacktrace -> GetCurrentStackTrace opens an EscapableHandleScope, which fails the same way and re-enters OnFatalError. Introduced by the "Add the zero-out-internal-field logic to PCP too" commit of #391, first shipped in 5.18.0. #391 nulled the holder's internal field in DrainLiveCtxWraps, which is right there: that slot is the OTEP-4947 reader's contract, so leaving it pointing at freed memory aims a loaded gun at a consumer we do not control. Copying the same logic into ~WallProfiler was symmetry, and the symmetry does not hold in either direction. It is not safe. node::FreeEnvironment wraps RunCleanup in a SealHandleScope, so handle_.Get() needs an inner HandleScope of its own — that much is fixable. The rest is not: WallProfiler is a Nan::ObjectWrap, whose weak callback is first-pass and deletes the wrap inline, and V8 documents that no API call may be made from a first-pass weak callback. That path is real, not theoretical (instrumenting the destructor shows it reached with a non-empty live list after dropping a profiler without dispose()), and where the holders die in the same GC nothing orders their weak callbacks before the profiler's, so Get() can materialize a Local to an already-condemned object. It is also not needed. A PCP holder is reachable only through that profiler's own cpedKey_: another WallProfiler in the same isolate uses a different key object, JS cannot read internal fields, and the out-of-process reader keys off the otel ALS. The slot dies with the profiler that could have read it. So drop the zero-out rather than scope it, which restores 5.17.0 behaviour for PCP. Verified against the reporter's Docker repro on node:24-alpine/arm64: 5/5 clean on terminate() and 3/3 on process.exit(), against 3/3 aborts before. Contrary to the report, glibc is affected too — node:24-bookworm-slim aborts 3/3 (exit 133 rather than 139, and with a populated stack trace, which is where the trace above comes from). It is a plain ApiCheck; libc has nothing to do with it. worker2 now establishes a sample context, because the existing "should not crash when worker is terminated" test missed all of this: it never set one, so liveContextPtrHead_ was always empty and the faulting walk never executed. With a context set, reinstating the unscoped zero-out fails the test. DrainLiveCtxWraps keeps its HandleScope and gains a comment saying why it is there, since that is exactly what was not obvious. --- bindings/otel-thread-ctx.cc | 5 +++++ bindings/profilers/wall.cc | 12 ++++++++---- ts/test/worker2.ts | 22 ++++++++++++++++++---- 3 files changed, 31 insertions(+), 8 deletions(-) diff --git a/bindings/otel-thread-ctx.cc b/bindings/otel-thread-ctx.cc index 3d061891..ddee80cf 100644 --- a/bindings/otel-thread-ctx.cc +++ b/bindings/otel-thread-ctx.cc @@ -301,7 +301,12 @@ thread_local CtxWrap* g_live_ctx_wraps = nullptr; // fires exactly once, at teardown, while the Environment is still alive. void DrainLiveCtxWraps(void* arg) { auto* isolate = static_cast(arg); + // We must allocate our own HandleScope here as node::FreeEnvironment wraps + // RunCleanup in a SealHandleScope, so handle_.Get() below has to allocate + // inside a scope of our own or V8 aborts with "Cannot create a handle without + // a HandleScope". v8::HandleScope scope(isolate); + CtxWrap* p = g_live_ctx_wraps; while (p != nullptr) { CtxWrap* next = p->next_; diff --git a/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index fe85c876..0187af47 100644 --- a/bindings/profilers/wall.cc +++ b/bindings/profilers/wall.cc @@ -700,15 +700,19 @@ WallProfiler::~WallProfiler() { // 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.) + // + // While it'd be tempting to do the same "zero out internal field logic" here + // as in otel-thread-ctx.cc's DrainLiveCtxWraps, we shouldn't. That one only + // ever runs as an environment cleanup hook, while this can also get here from + // Nan::ObjectWrap's weak callback, and V8 forbids the API in a first-pass + // weak callback. The holders' internal fields therefore keep pointing at the + // PCPs we free, but since they are only ever read back through our own + // cpedKey_ that dies with us it is not an issue. 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; } diff --git a/ts/test/worker2.ts b/ts/test/worker2.ts index 041a4461..284811b8 100644 --- a/ts/test/worker2.ts +++ b/ts/test/worker2.ts @@ -24,8 +24,22 @@ time.start({ useCPED: useCPED, }); -parentPort?.on('message', () => { - void delay(50).then(() => { - parentPort?.postMessage('hello'); +function listen() { + parentPort?.on('message', () => { + void delay(50).then(() => { + parentPort?.postMessage('hello'); + }); }); -}); +} + +// Establish a sample context, and do it around the listener registration so +// the async context frame holding it stays reachable until we are terminated. +// That leaves a live PersistentContextPtr for ~WallProfiler to walk when it +// runs from the environment cleanup hook; with an empty list the walk is a +// no-op and the teardown path goes untested. +if (useCPED) { + time.runWithContext({worker: 'worker2'}, listen); +} else { + time.setContext({worker: 'worker2'}); + listen(); +}