From 897932c4848ae4eb4a27a399ac165fc5efe264a2 Mon Sep 17 00:00:00 2001
From: Ugaitz Urien <ugaitz.urien@datadoghq.com>
Date: Fri, 19 Sep 2025 09:28:39 +0200
Subject: [PATCH] diagnostics_channel: fix race condition with
 diagnostics_channel and GC
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

PR-URL: https://github.com/nodejs/node/pull/59910
Reviewed-By: Stephen Belanger <admin@stephenbelanger.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
---
 lib/diagnostics_channel.js                    |  8 ++++++-
 .../source_map_assert_source_line.snapshot    |  2 +-
 ...stics-channel-gc-maintains-subcriptions.js | 21 +++++++++++++++++
 ...t-diagnostics-channel-gc-race-condition.js | 23 +++++++++++++++++++
 4 files changed, 52 insertions(+), 2 deletions(-)
 create mode 100644 test/parallel/test-diagnostics-channel-gc-maintains-subcriptions.js
 create mode 100644 test/parallel/test-diagnostics-channel-gc-race-condition.js

diff --git a/lib/diagnostics_channel.js b/lib/diagnostics_channel.js
index 312bd258f5..3deb301e7f 100644
--- a/lib/diagnostics_channel.js
+++ b/lib/diagnostics_channel.js
@@ -37,7 +37,9 @@ const { WeakReference } = require('internal/util');
 // Only GC can be used as a valid time to clean up the channels map.
 class WeakRefMap extends SafeMap {
   #finalizers = new SafeFinalizationRegistry((key) => {
-    this.delete(key);
+    // Check that the key doesn't have any value before deleting, as the WeakRef for the key
+    // may have been replaced since finalization callbacks aren't synchronous with GC.
+    if (!this.has(key)) this.delete(key);
   });
 
   set(key, value) {
@@ -49,6 +51,10 @@ class WeakRefMap extends SafeMap {
     return super.get(key)?.get();
   }
 
+  has(key) {
+    return !!this.get(key);
+  }
+
   incRef(key) {
     return super.get(key)?.incRef();
   }
diff --git a/test/fixtures/source-map/output/source_map_assert_source_line.snapshot b/test/fixtures/source-map/output/source_map_assert_source_line.snapshot
index fe11794e9c..9def6eb4d7 100644
--- a/test/fixtures/source-map/output/source_map_assert_source_line.snapshot
+++ b/test/fixtures/source-map/output/source_map_assert_source_line.snapshot
@@ -7,7 +7,7 @@ AssertionError [ERR_ASSERTION]: The expression evaluated to a falsy value:
     *
     *
     *
-    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
+    at TracingChannel.traceSync (node:diagnostics_channel:328:14)
     *
     *
     *
diff --git a/test/parallel/test-diagnostics-channel-gc-maintains-subcriptions.js b/test/parallel/test-diagnostics-channel-gc-maintains-subcriptions.js
new file mode 100644
index 0000000000..3de30a63d4
--- /dev/null
+++ b/test/parallel/test-diagnostics-channel-gc-maintains-subcriptions.js
@@ -0,0 +1,21 @@
+// Flags: --expose-gc
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const { channel } = require('diagnostics_channel');
+
+function test() {
+  function subscribe() {
+    channel('test-gc').subscribe(function noop() {});
+  }
+
+  subscribe();
+
+  setTimeout(() => {
+    global.gc();
+    assert.ok(channel('test-gc').hasSubscribers, 'Channel must have subscribers');
+  });
+}
+
+test();
diff --git a/test/parallel/test-diagnostics-channel-gc-race-condition.js b/test/parallel/test-diagnostics-channel-gc-race-condition.js
new file mode 100644
index 0000000000..6a46325d4d
--- /dev/null
+++ b/test/parallel/test-diagnostics-channel-gc-race-condition.js
@@ -0,0 +1,23 @@
+// Flags: --expose-gc
+'use strict';
+
+require('../common');
+const assert = require('assert');
+const { channel } = require('diagnostics_channel');
+
+function test() {
+  const testChannel = channel('test-gc');
+
+  setTimeout(() => {
+    const testChannel2 = channel('test-gc');
+
+    assert.ok(testChannel === testChannel2, 'Channel instances must be the same');
+  });
+}
+
+test();
+
+setTimeout(() => {
+  global.gc();
+  test();
+}, 10);