diff --git a/.github/dependabot.yml b/.github/dependabot.yml index e45eef52..cdf3668e 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -15,9 +15,22 @@ 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 - dependencies - javascript - semver-patch + groups: + patch-updates: + update-types: + - "patch" + minor-updates: + update-types: + - "minor" 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 c98bb209..cbe34d51 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; @@ -190,6 +211,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) @@ -207,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 @@ -237,32 +265,101 @@ 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; + +// 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; +} 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(); + 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 @@ -444,7 +541,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; @@ -518,14 +615,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 @@ -543,12 +647,31 @@ 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 = CtxWrap::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 // 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; @@ -561,7 +684,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; @@ -575,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); @@ -587,6 +711,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)); @@ -679,13 +806,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/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/bindings/profilers/wall.cc b/bindings/profilers/wall.cc index 95600d3b..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,7 +107,21 @@ void SetContextPtr(ContextPtr& contextPtr, } } -class PersistentContextPtr : public node::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 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 +140,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::Global handle_; + friend class WallProfiler; + static void WeakCallback( + const v8::WeakCallbackInfo& data) { + delete data.GetParameter(); + } + public: PersistentContextPtr(WallProfiler* profiler, Local wrap); @@ -139,14 +163,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; @@ -169,15 +197,6 @@ PersistentContextPtr::~PersistentContextPtr() { } } -inline void* GetAlignedPointerFromInternalField(Object* object, int index) { -#if NODE_MAJOR_VERSION >= 26 - return object->GetAlignedPointerFromInternalField( - index, kEmbedderDataTypeTagDefault); -#else - return object->GetAlignedPointerFromInternalField(index); -#endif -} - // Maximum number of rounds in the GetV8ToEpochOffset static constexpr int MAX_EPOCH_OFFSET_ATTEMPTS = 20; @@ -678,14 +697,18 @@ 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_; + 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/package-lock.json b/package-lock.json index 3f9160cc..bbfc19ac 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,21 +1,21 @@ { "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", - "pprof-format": "^2.2.1", + "pprof-format": "^2.3.1", "source-map": "^0.8.0" }, "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": { @@ -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.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 bcea3076..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", @@ -38,12 +38,12 @@ "license": "Apache-2.0", "dependencies": { "node-gyp-build": "^4.8.4", - "pprof-format": "^2.2.1", + "pprof-format": "^2.3.1", "source-map": "^0.8.0" }, "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", diff --git a/ts/src/otel-thread-ctx.ts b/ts/src/otel-thread-ctx.ts index cbc91c93..f59a976b 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; @@ -120,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; @@ -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/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/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/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/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-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); diff --git a/ts/test/test-otel-thread-ctx.ts b/ts/test/test-otel-thread-ctx.ts index f6229f2c..f4d6683b 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({ @@ -696,6 +733,82 @@ 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('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( @@ -733,7 +846,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'], 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), 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 () {