node:diagnostics_channel: sync with Node 26 + subsystem channels - #32628
node:diagnostics_channel: sync with Node 26 + subsystem channels#32628cirospaciari wants to merge 59 commits into
Code review found 3 potential issues
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | src/js/internal/module_tracing.ts:29-45 |
Require-swap can infinite-loop after subscribe → user monkey-patch → unsubscribe → resubscribe |
| 🟡 Nit | test/js/node/async_hooks/AsyncLocalStorage.test.ts:990-992 |
Stale timeout-rationale comment: 'net/_http_server eagerly load node:diagnostics_channel' |
| 🟡 Nit | test/js/node/test/parallel/test-diagnostics-channel-http.js:1-12 |
test-diagnostics-channel-http.js vendored despite PR description saying it is excluded |
Annotations
Check warning on line 45 in src/js/internal/module_tracing.ts
claude / Claude Code Review
Require-swap can infinite-loop after subscribe → user monkey-patch → unsubscribe → resubscribe
`onRequireSubscribersChanged()` re-reads `baseRequire = Module.prototype.require` on every false→true transition (line 34), not just the first — contradicting the comment on lines 10-11. If a user monkey-patches `Module.prototype.require` while tracing is active (capturing `tracingRequire` as its `orig`), then unsubscribes (restore is skipped since `M.p.require !== tracingRequire`), then resubscribes, `baseRequire` becomes the user's wrapper and any `require()` recurses `tracingRequire → wrap →
Check warning on line 992 in test/js/node/async_hooks/AsyncLocalStorage.test.ts
claude / Claude Code Review
Stale timeout-rationale comment: 'net/_http_server eagerly load node:diagnostics_channel'
This comment says "net/_http_server eagerly load node:diagnostics_channel", but commit 078e4bbb made both lazy via `initNetChannels()` / `initHttpServerChannels()` per Jarred's "Do not load node:diagnostics_channel on start" review. The rationale for the 15s bumps is still valid — three of these subprocesses `require("http2")` (http2.ts:51 still does `const dc = require("node:diagnostics_channel")` at module scope on main) and the fourth loads `_http_client.ts` (same pattern at line 26) — so the
Check warning on line 12 in test/js/node/test/parallel/test-diagnostics-channel-http.js
claude / Claude Code Review
test-diagnostics-channel-http.js vendored despite PR description saying it is excluded
The description's "not vendored" table lists `test-diagnostics-channel-http` as intentionally excluded (net.Socket assertion) and says replacement coverage was added to `diagnostics_channel.test.ts` because it was dropped — but the diff adds it as a new file with no expectations.txt entry. Given `NodeHTTPServerSocket` now `extends net.Socket` on main and robobun's later comment counts "65" passing (vs the body's "64"), the test likely passes now and the description is just stale; either way the