Node v24.13.0 nsolid v6.2.0 release#409
Node v24.13.0 nsolid v6.2.0 release#409santigimeno merged 147 commits intonode-v24.x-nsolid-v6.xfrom
Conversation
Original commit message:
Fix scratch registers passed to mtvsrdd
`ra` cannot be r0 as it will be interpreted as Operand(0)
Change-Id: Idce58191f9d3578dc91dc4aa3872a0bf2939d8b3
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6936113
Commit-Queue: Milad Farazmand <mfarazma@ibm.com>
Reviewed-by: Junliang Yan <junyan1@ibm.com>
Cr-Commit-Position: refs/heads/main@{#102388}
Refs: v8/v8@2abc613
PR-URL: nodejs/node#60177
Refs: nodejs/undici#4530
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs/node#60125 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
PR-URL: nodejs/node#60150 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
PR-URL: nodejs/node#60175 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
There can be a race from eagerly shutting down the servers and severing two pipes at the same time but for the purpose of this test, we only care about whether the requests are initiated from the client as expected, not how the upstream/proxy servers behave. Ignore EPIPE errors from them. PR-URL: nodejs/node#60269 Refs: nodejs/node#59741 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
PR-URL: nodejs/node#60142 Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
`performance.eventLoopUtilization` and `performance.timerify` are not part of the Web API. Expose these two functions directly on the `perf_hooks` module. PR-URL: nodejs/node#60370 Fixes: nodejs/node#60368 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#60391 Refs: nodejs/node#49605 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
The test has been flaky for years and new platforms keep popping up. As it squeezes too many independent test cases into one file, split them into individual files to avoid masking regressions and help only mark the real flaky ones as flaky. This might also help with the flakiness itself by avoiding updating a shared tmpdir being watched by differet tests and avoiding running all these time-consuming tests in one file, which can cause a timeout on slow machines. PR-URL: nodejs/node#60391 Refs: nodejs/node#49605 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Jake Yuesong Li <jake.yuesong@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
PR-URL: nodejs/node#60190 Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: nodejs/node#60366 Refs: nodejs/nodejs.org#8231 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Claudio Wunder <cwunder@gnome.org> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Fixes: nodejs/node#56666 PR-URL: nodejs/node#56829 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
PR-URL: nodejs/node#60387 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Filip Skokan <panva.ip@gmail.com>
PR-URL: nodejs/node#60388 Reviewed-By: Chengzhong Wu <legendecas@gmail.com> Reviewed-By: Kohei Ueno <kohei.ueno119@gmail.com>
PR-URL: nodejs/node#60411 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
PR-URL: nodejs/node#60412 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Node.js only checks trust settings of additional certificates from the system store. It does not, however, support revocation/distrust of existing certificates. PR-URL: nodejs/node#60340 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
PR-URL: nodejs/node#60444 Refs: nodejs/undici#4644 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This addresses late review comments for the recently landed cfbfc1b05080 and aligns the new code with the pre-existing V8 fast call counters. Refs: nodejs/node#60434 (review) PR-URL: nodejs/node#60447 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Avoid unnecessary nextTick. PR-URL: nodejs/node#60454 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
PR-URL: nodejs/node#60345 Fixes: nodejs/node#60311 Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Vladimir Morozov <vmorozov@microsoft.com>
PR-URL: nodejs/node#59394 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com> Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
PR-URL: nodejs/node#60465 Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
PR-URL: nodejs/node#60474 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Previously, formatExtraProperties applied ctx.stylize to the entire '[key]: value' string. This caused the colon and space to be styled, making the output inconsistent with normal object properties. Now, only the key itself is stylized, and the colon and space remain unstyled, aligning with the formatting of regular properties. Refs: nodejs/node#60131 PR-URL: nodejs/node#60479 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Jordan Harband <ljharb@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Otherwise when the expected prompt does not show up in the session and the debugger tests time out, there is not enough information to figure out exactly which line in the test is causing the timeout. PR-URL: nodejs/node#60457 Reviewed-By: Ethan Arrowood <ethan@arrowood.dev> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
The existing description is outdated, and exposes too many details that are subject to change. - There is no point conceptualizing "two module loaders", in reality the boundary is blurred since the two invoke each other to support require(esm) and import(cjs). The distinction lies not in what kind of module is being requested/which loader is used, but only in how the the module request is initiated (via `require()` or `import()`). The inner working of the loaders are subject to change and not suitable to be documented. - It should not mention monkey patching in the documentation, as publicly supported universal hooks are already provided through `module.registerHooks()`, and so there's no need to single out any of them in terms of loader hooks support either. - Remove the description about whether they are asynchronous or synchronous, which is also implementation detail subject to change. - Add missing descriptions about how .ts, .mts and .cts are treated, and `.node` is also supported in import now. - There is no need to specially mention .node treatment in cli.md, link to the explanations about loading from `import` in packages.md instead. PR-URL: nodejs/node#60346 Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
ESM resolution and loading is now always synchronous from a non-loader-hook thread. If no asynchrnous loader hooks are registered, the resolution/loading is entirely synchronous. If asynchronous loader hooks are registered, these would be synchronous on the non-loader-hook thread, and asynchronous on the loader hook thread. This avoids several races caused by async/sync loading sharing the same cache. In particular, asynchronous loader hooks now works with `require(esm)` - previously it tends to break due to races. In addition, when an asynchronous loader hook returns a promise that never settles, the main thread no longer silently exits with exit code 13, leaving the code below any module loading calls silently ignored without being executed. Instead, it now throws ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED which can be caught and handled by the main thread. If the module request comes from `import()`, the never-settling promise is now relayed to the result returned by `import()`. Drive-by: when annotating the error about importing undetectable named exports from CommonJS, it now no longer reload the source code of the CommonJS module, and instead reuses format information cached when the module was loaded for linking. PR-URL: nodejs/node#60380 Fixes: nodejs/node#59666 Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Jacob Smith <jacob@frende.me>
PR-URL: nodejs/node#60481 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com>
Original commit message:
[regexp] Rename "greedy loops" to "fixed length".
Some greedy quantifiers are code-generated in a more efficient
way, but far from all greedy quantifiers are generated
in this way. This change renames the specially optimized loops
from "greedy loops" to "fixed length loops", which should be
clearer.
Going forward, we can probably reuse much of the code for
fixed length loops when code-generating simple possessive
quantifiers.
Change-Id: I13b9d14beac430e2d05d0feaf887fc0566bc4103
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6508846
Reviewed-by: Patrick Thier <pthier@chromium.org>
Commit-Queue: Erik Corry <erikcorry@chromium.org>
Cr-Commit-Position: refs/heads/main@{#100062}
Refs: v8/v8@df20105
PR-URL: nodejs/node#60732
Fixes: nodejs/node#60030
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Original commit message:
[regexp] Remove DeferredAction class.
We can just chain up the traces and use the regular
ActionNodes that we already have to represent the
deferred actions. Also simplifies the ActionNodes
a little. No functional change intended.
Reduce the size of the Trace from 128->120 bytes.
This is a stack allocated struct so to avoid stack
overflows after adding the next_ field I am reducing
it back down to 120 bytes by rearranging and shrinking
fields.
Change-Id: I6dca9946e035e9b22798e160b8fadaeca61f4955
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6512931
Reviewed-by: Patrick Thier <pthier@chromium.org>
Commit-Queue: Erik Corry <erikcorry@chromium.org>
Cr-Commit-Position: refs/heads/main@{#100092}
Refs: v8/v8@0dd2318
PR-URL: nodejs/node#60732
Fixes: nodejs/node#60030
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Original commit message:
[regexp] Clean up state for fixed length loop.
Also reduces the on-stack Trace size by one word,
adds some comments, renames some variables for
more clarity.
Change-Id: I9ec105cd9cebbaba65e9801c47dd0574cc81f967
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6512896
Reviewed-by: Patrick Thier <pthier@chromium.org>
Commit-Queue: Erik Corry <erikcorry@chromium.org>
Cr-Commit-Position: refs/heads/main@{#100117}
Refs: v8/v8@6bb32bd
PR-URL: nodejs/node#60732
Fixes: nodejs/node#60030
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Original commit message:
[regexp] Fix modifiers for ChoiceNodes
Each alternative might modify flags when their sub-graph is emitted.
We need to restore flags to the value at the beginning of a ChoiceNode
for each alternative.
Drive-by: Move regexp-modifiers test out of harmony/
Fixed: 447583670
Change-Id: I9f41e51f34df7659461da0a4fcd28b7e157f52e1
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/6995181
Reviewed-by: Jakob Linke <jgruber@chromium.org>
Commit-Queue: Patrick Thier <pthier@chromium.org>
Cr-Commit-Position: refs/heads/main@{#102838}
Refs: v8/v8@72b0e27
Fixes: nodejs/node#60030
PR-URL: nodejs/node#60732
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
Notable changes: http: * (SEMVER-MINOR) add optimizeEmptyRequests server option (Rafael Gonzaga) nodejs/node#59778 lib: * (SEMVER-MINOR) add options to util.deprecate (Rafael Gonzaga) nodejs/node#59982 module: * (SEMVER-MINOR) mark type stripping as stable (Marco Ippolito) nodejs/node#60600 node-api: * (SEMVER-MINOR) add napi_create_object_with_properties (Miguel Marcondes Filho) nodejs/node#59953 sqlite: * (SEMVER-MINOR) allow setting defensive flag (Bart Louwers) nodejs/node#60217 src: * (SEMVER-MINOR) add watch config namespace (Marco Ippolito) nodejs/node#60178 * (SEMVER-MINOR) add an option to make compile cache portable (Aditi) nodejs/node#58797 src,permission: * (SEMVER-MINOR) add --allow-inspector ability (Rafael Gonzaga) nodejs/node#59711 v8: * (SEMVER-MINOR) add cpu profile (theanarkh) nodejs/node#59807 PR-URL: nodejs/node#61001
Refs: https://hackerone.com/reports/3390084 PR-URL: nodejs-private/node-private#748 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> CVE-ID: CVE-2025-55132
Refs: https://hackerone.com/reports/3417819 Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com> PR-URL: nodejs-private/node-private#760 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> CVE-ID: CVE-2025-55130
When a stack overflow exception occurs during async_hooks callbacks (which use TryCatchScope::kFatal), detect the specific "Maximum call stack size exceeded" RangeError and re-throw it instead of immediately calling FatalException. This allows user code to catch the exception with try-catch blocks instead of requiring uncaughtException handlers. The implementation adds IsStackOverflowError() helper to detect stack overflow RangeErrors and re-throws them in TryCatchScope destructor instead of calling FatalException. This fixes the issue where async_hooks would cause stack overflow exceptions to exit with code 7 (kExceptionInFatalExceptionHandler) instead of being catchable. Fixes: nodejs/node#37989 Ref: https://hackerone.com/reports/3456295 PR-URL: nodejs-private/node-private#773 Refs: https://hackerone.com/reports/3456295 Reviewed-By: Robert Nagy <ronagy@icloud.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> CVE-ID: CVE-2025-59466
Wrap pskCallback and ALPNCallback invocations in try-catch blocks to route exceptions through owner.destroy() instead of letting them become uncaught exceptions. This prevents remote attackers from crashing TLS servers or causing resource exhaustion. Fixes: https://hackerone.com/reports/3473882 PR-URL: nodejs-private/node-private#796 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> CVE-ID: CVE-2026-21637
This prevents the server from crashing due to an unhandled rejection
when a TLSSocket connection is abruptly destroyed during initialization
and the user has not attached an error handler to the socket.
e.g:
```js
const server = http2.createSecureServer({ ... })
server.on('secureConnection', socket => {
socket.on('error', err => {
console.log(err)
})
})
```
PR-URL: nodejs-private/node-private#797
Fixes: nodejs/node#44751
Refs: https://hackerone.com/bugs?subject=nodejs&report_id=3262404
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
CVE-ID: CVE-2025-59465
PR-URL: nodejs/node#61283 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
PR-URL: nodejs/node#60997 Reviewed-By: Richard Lau <richard.lau@ibm.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Ethan Arrowood <ethan@arrowood.dev>
This removes the zero-fill toggle mechanism that allowed JavaScript to control ArrayBuffer initialization via shared memory. Instead, unsafe buffer creation now uses a dedicated C++ API. Refs: https://hackerone.com/reports/3405778 Co-Authored-By: Rafael Gonzaga <rafael.nunu@hotmail.com> Signed-off-by: RafaelGSS <rafael.nunu@hotmail.com> PR-URL: nodejs-private/node-private#759 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> CVE-ID: CVE-2025-55131
This is a security release. Notable changes: lib: * (CVE-2025-59465) add TLSSocket default error handler (RafaelGSS) <nodejs-private/node-private#797> * (CVE-2025-55132) disable futimes when permission model is enabled (RafaelGSS) <nodejs-private/node-private#748> lib,permission: * (CVE-2025-55130) require full read and write to symlink APIs (RafaelGSS) <nodejs-private/node-private#760> src: * (CVE-2025-59466) rethrow stack overflow exceptions in async\_hooks (Matteo Collina) <nodejs-private/node-private#773> src,lib: * (CVE-2025-55131) refactor unsafe buffer creation to remove zero-fill toggle (Сковорода Никита Андреевич) <nodejs-private/node-private#759> tls: * (CVE-2026-21637) route callback exceptions through error handlers (Matteo Collina) <nodejs-private/node-private#796> PR-URL: nodejs-private/node-private#800
2026-01-13 Node.js v24.13.0 'Krypton' (LTS) Release Git-EVTag-v0-SHA512: f6fd214d8fef7bb59d5ca9692798b55f4ac592c11fbaf66e18cafb8dbbfb20ed8787324e69843402e11eb0b6a39f584d0497416ff14f2512b2ab3b6778d9b8ef
WalkthroughRestructures devcontainer configuration, adds a base devcontainer, and updates CODEOWNERS and .gitignore. Updates numerous GitHub Actions versions and ignores. Bumps multiple dependencies (c-ares, nghttp2, undici, corepack, amaro, v8). Adds runtime feature detection and new undici features (dedupe, RoundRobinPool, DNS storage). Many V8 and Node core API/internal changes and docs edits. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Application Client
participant Dedup as Deduplicate Interceptor
participant Map as PendingRequests Map
participant Primary as Primary Dispatch (network)
participant Handler as DeduplicationHandler
participant Waiting as Waiting Handler(s)
participant Response as Network Response
Client->>Dedup: dispatch(request)
Dedup->>Map: compute dedupeKey
alt key exists
Map-->>Handler: add Waiting Handler
Dedup->>Waiting: attach to existing handler
else new key
Dedup->>Handler: create primary handler, register in Map
Dedup->>Primary: forward actual dispatch
end
Primary->>Handler: onResponseStart/status/headers
Handler->>Handler: buffer chunks via onResponseData
Primary->>Handler: onResponseEnd
Handler->>Waiting: replay buffered response to waiting handlers
Handler->>Map: remove dedupeKey
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (21)📓 Common learnings📚 Learning: 2025-12-03T14:35:32.707ZApplied to files:
📚 Learning: 2025-12-03T14:29:25.191ZApplied to files:
📚 Learning: 2025-12-03T14:35:32.707ZApplied to files:
📚 Learning: 2025-12-03T14:34:55.521ZApplied to files:
📚 Learning: 2025-12-03T14:29:25.191ZApplied to files:
📚 Learning: 2025-12-03T14:34:52.246ZApplied to files:
📚 Learning: 2025-12-03T14:35:32.707ZApplied to files:
📚 Learning: 2025-12-03T14:35:32.707ZApplied to files:
📚 Learning: 2025-12-03T14:34:55.521ZApplied to files:
📚 Learning: 2025-12-03T14:35:32.707ZApplied to files:
📚 Learning: 2025-12-03T14:34:55.521ZApplied to files:
📚 Learning: 2025-12-03T14:34:47.986ZApplied to files:
📚 Learning: 2025-12-03T14:34:52.246ZApplied to files:
📚 Learning: 2025-12-03T14:30:16.146ZApplied to files:
📚 Learning: 2025-12-03T14:29:16.068ZApplied to files:
📚 Learning: 2025-12-03T14:29:16.068ZApplied to files:
📚 Learning: 2025-12-03T14:29:05.695ZApplied to files:
📚 Learning: 2025-12-03T14:29:08.569ZApplied to files:
📚 Learning: 2025-12-03T14:29:05.695ZApplied to files:
📚 Learning: 2025-12-03T14:29:05.695ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
🔇 Additional comments (4)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
doc/api/child_process.md (1)
1484-1503: Remove duplicate close event handling.This example contains both the original event-based handler (lines 1493-1499) and the new
await once()pattern (lines 1500-1502). Both handlers would execute in the same code, causing the close message to be logged twice. This is inconsistent with the earlier examples which cleanly replaced the event handler.Either remove the old event handler to match the pattern used in the earlier examples, or clearly separate these as distinct alternative approaches.
📝 Suggested fix
ls.on('close', (code) => { console.log(`child process close all stdio with code ${code}`); }); ls.on('exit', (code) => { console.log(`child process exited with code ${code}`); }); - -const [code] = await once(ls, 'close'); -console.log(`child process close all stdio with code ${code}`);Or, if both approaches should be demonstrated, they should be in separate code blocks with explanatory text distinguishing them.
deps/undici/src/lib/mock/snapshot-utils.js (1)
43-45: Fallback does not actually hash the value—it only encodes.The fallback path
Buffer.from(value).toString('base64url')performs base64url encoding without hashing. Thecrypto.hashAPI is only available in Node.js 19.1.0+; older versions fall back to the encoding-only path.This creates a semantic mismatch: the JSDoc states the function returns "the base64url encoded hash," but the fallback implementation contradicts this. The practical implications are:
- Output length differs between the two paths (fixed 43 chars for SHA-256 vs. variable for base64url)
- If the crypto.hash availability differs between environments, generated snapshot IDs won't be stable
Since this is upstream undici code, this fallback is likely intentional for broad Node.js version compatibility. However, the JSDoc should clarify the fallback behavior rather than implying all code paths perform hashing.
deps/undici/src/docs/docs/api/Client.md (1)
33-38: Documentation inconsistency: Notes section contradicts the newuseH2coption.Line 33 documents the new
useH2coption that "Enforces h2c for non-https connections", but line 37 in the notes section still states "It only works under TLS connections. h2c is not supported." This note should be updated to reflect the new h2c support whenuseH2cis enabled.doc/api/http.md (1)
3569-3684: Fix pr-url and soften "never emit" wording foroptimizeEmptyRequests.The
pr-urlshould point to this PR (#409), not upstream nodejs/node PR#59778—the lint-pr-url check validates that pr-url matches the current pull request URL.Also, "never emit any stream events" overstates the guarantee. Since the stream is already-ended when initialized, new listeners won't fire, but the phrasing could be softened to say "will not emit stream events to listeners added after initialization" or similar.
deps/cares/src/lib/ares_init.c (1)
485-507: Lock(*dest)while copying callbacks to prevent event thread from reading partially-initialized pointers.The event thread for
(*dest)starts inares_init_options()before the callbacks are copied at lines 488–501. Since the event thread readsnotify_pending_write_cbandquery_enqueue_cbwithout synchronization (seeares_process.c:1253andares_process.c:1390), wrap the callback assignments withares_channel_lock(*dest)andares_channel_unlock(*dest)to ensure thread safety.deps/nghttp2/lib/nghttp2_session.c (2)
432-614:map_seedfallback being0may negate hash/DoS hardening whenrand_callbackis unset.
Ifnghttp2_map_init()’s seed is intended to reduce collision attacks, consider using a non-deterministic fallback whencallbacks->rand_callbackis NULL (or at least initializemap_seedbefore invoking the callback to avoid reliance on “must fill all bytes” behavior).Proposed adjustment (minimal)
- if (callbacks->rand_callback) { - callbacks->rand_callback((uint8_t *)&map_seed, sizeof(map_seed)); - } else { - map_seed = 0; - } + map_seed = 0; + if (callbacks->rand_callback) { + callbacks->rand_callback((uint8_t *)&map_seed, sizeof(map_seed)); + } else { + /* Fallback: avoid deterministic seed where possible */ + map_seed = (uint32_t)nghttp2_time_now_sec(); + }
5471-5528: Rate limiter is enabled by default (not opt-in); verify if default rates should be relaxed.The glitch rate limiter is always initialized (not opt-in) with a burst of 1000 and a rate of 33 tokens/second. The
NGHTTP2_OPT_GLITCH_RATE_LIMIToption allows overriding these defaults, not disabling the limiter entirely. This means valid-but-rare patterns (PRIORITY frames, empty DATA without END_STREAM, unknown extension frames) will trigger rate limiting on every session by default.At the default rate of 33 events/second, clients legitimately sending PRIORITY frames or extension frames in rapid succession could exceed the limit and receive an unexpected GOAWAY (ENHANCE_YOUR_CALM). If this is a new feature, older clients with these patterns may encounter unexpected disconnections.
Consider: either relax the default rates (e.g.,
rate=0to disable by default), or document this as a behavioral change requiring opt-out configuration.Also applies to: 5602–5648, 5813–5832, 5908–5960, 6067–6083, 6615–6631
deps/v8/src/regexp/regexp-compiler.cc (3)
129-141: Comment tweak: “simple greedy loops” vs “fixed length loops” phrasing is slightly confusing
Line 129 says “Simple greedy loops…” and Line 130 says “these fixed length loops”. Consider rewording to make it clear you’re describing the optimization category (fixed-length loop optimization) rather than renaming the loop itself.
297-318: MakeGetStoredPositionmore robust against caller misuse
DCHECK_EQ(0, *cp_offset)(Line 305) requires callers to pre-init the out-param. It’s easy to violate and doesn’t buy much. Prefer setting*cp_offset = 0at function entry and dropping the DCHECK (or DCHECK thatcp_offset != nullptr).Proposed diff
bool Trace::GetStoredPosition(int reg, int* cp_offset) const { - DCHECK_EQ(0, *cp_offset); + DCHECK_NOT_NULL(cp_offset); + *cp_offset = 0; for (auto trace : *this) { if (trace->has_action() && trace->action()->Mentions(reg)) { if (trace->action_->action_type() == ActionNode::CLEAR_POSITION || trace->action_->action_type() == ActionNode::RESTORE_POSITION) { *cp_offset = trace->next_->cp_offset(); return true; } else { return false; } } } return false; }
2628-2679: Potential int overflow in fixed-length loop length accumulation
length += node_length;(Line 2666) can overflow signedintbefore the final bounds check (Lines 2674-2676). If it wraps, it can accidentally pass the min/max CP offset check and misclassify a too-large alternative as valid.Proposed diff (checked add)
int ChoiceNode::FixedLengthLoopLengthForAlternative( GuardedAlternative* alternative) { int length = 0; RegExpNode* node = alternative->node(); int recursion_depth = 0; while (node != this) { if (recursion_depth++ > RegExpCompiler::kMaxRecursion) { return kNodeIsTooComplexForFixedLengthLoops; } int node_length = node->FixedLengthLoopLength(); if (node_length == kNodeIsTooComplexForFixedLengthLoops) { return kNodeIsTooComplexForFixedLengthLoops; } - length += node_length; + if (V8_UNLIKELY(base::bits::SignedAddOverflow32(length, node_length, + &length))) { + return kNodeIsTooComplexForFixedLengthLoops; + } node = node->AsSeqRegExpNode()->on_success(); } if (read_backward()) { length = -length; } if (length < RegExpMacroAssembler::kMinCPOffset || length > RegExpMacroAssembler::kMaxCPOffset) { return kNodeIsTooComplexForFixedLengthLoops; } return length; }deps/v8/src/regexp/regexp-compiler.h (1)
243-353: Trace copy ctor likely missingquick_check_performed_copy
Trace(const Trace& other)copies most state but does not initializequick_check_performed_, so it defaults tocharacters_ == 0. That drops quick-check context on everyTrace new_trace(*trace)and can change codegen decisions/optimizations.Proposed diff
Trace(const Trace& other) V8_NOEXCEPT : cp_offset_(other.cp_offset_), flush_budget_(other.flush_budget_), at_start_(other.at_start_), has_any_actions_(other.has_any_actions_), action_(nullptr), backtrack_(other.backtrack_), fixed_length_loop_state_(other.fixed_length_loop_state_), characters_preloaded_(other.characters_preloaded_), bound_checked_up_to_(other.bound_checked_up_to_), + quick_check_performed_(other.quick_check_performed_), next_(&other) {}deps/v8/src/regexp/regexp-nodes.h (2)
329-420: Add direct include forbase::IsInRange()and guard register access methods against misuse.
Mentions()usesbase::IsInRange()which is defined insrc/base/bounds.h, but that header is not directly included inregexp-nodes.h. Add direct include to avoid relying on transitive includes.register_to()lacks theDCHECK(IsSimpleAction())guard thatregister_from()has, creating asymmetry and risking accidental calls on non-simple actions. Add matching DCHECK.- Constructor should validate
from <= towhento != -1to prevent creating inverted ranges that would breakMentions()and related logic. Add defensive DCHECKs.Proposed diff
--- a/deps/v8/src/regexp/regexp-nodes.h +++ b/deps/v8/src/regexp/regexp-nodes.h @@ -1,6 +1,7 @@ `#include` "src/codegen/label.h" +#include "src/base/bounds.h" `#include` "src/regexp/regexp-macro-assembler.h" `#include` "src/zone/zone.h" int register_from() const { DCHECK(IsSimpleAction()); return data_.u_simple.register_from; } - int register_to() const { return data_.u_simple.register_to; } + int register_to() const { + DCHECK(IsSimpleAction()); + return data_.u_simple.register_to; + } ActionNode(ActionType action_type, RegExpNode* on_success, int from, int to = -1, int value = 0) : SeqRegExpNode(on_success), action_type_(action_type) { + DCHECK_GE(from, 0); + DCHECK(to == -1 || to >= from); data_.u_simple.register_from = from; data_.u_simple.register_to = to == -1 ? from : to; data_.u_simple.value = value; DCHECK(IsSimpleAction()); }
688-716: Flags are not consistently reset before all alternatives in EmitFixedLengthLoop and the early-return path.The code at line 3222-3224 explicitly acknowledges that "flags might be modified when emitting an alternative" and need to be reset, but this is inconsistently applied:
- Early return (line 3195): When
choice_count == 1with no guards, the alternative'sEmitis called without preserving flags at all.- EmitFixedLengthLoop (line 3282): The first alternative's
Emitis called withoutcompiler->set_flags(flags)beforehand, unlikeEmitChoiceswhich sets flags before each alternative.While
EmitChoicesproperly callscompiler->set_flags(flags)at the start of each iteration before callingEmit, and out-of-line continuations also preserve flags, these two paths bypass the flag reset mechanism. Ensure all code paths that emit alternatives reset flags to the captured state first.
🤖 Fix all issues with AI agents
In `@benchmark/esm/import-meta.js`:
- Around line 25-52: In main(), change how fixtureDir is resolved: replace
path.resolve(__filename, '../../fixtures') with path.resolve(__dirname,
'../fixtures') (or use __dirname + '/.. /fixtures') so fixtureDir points to
benchmark/fixtures and fixtureFile/fixtureFileURL use that correct dir; also
handle import errors from load(...) by appending a .catch(err => { throw err; })
(or otherwise rethrow/log and process.exit) after the .then(...) to ensure
promise rejections propagate instead of silently failing; reference the main
function, variables fixtureDir, fixtureFileURL, and the load(...) call when
applying the changes.
In `@deps/cares/src/lib/ares_init.c`:
- Around line 607-617: The setter ares_set_query_enqueue_cb currently updates
channel->query_enqueue_cb and channel->query_enqueue_cb_data without holding the
channel lock; make it thread-safe by acquiring the channel lock used elsewhere
in this file before assigning those fields and releasing it afterward (follow
the same lock/unlock calls used by other setters in this file), while keeping
the early return when channel == NULL.
In `@deps/cares/src/lib/ares_sysconfig_win.c`:
- Around line 115-118: The early return when WideCharToMultiByte returns 0 leaks
the buffer pointed to by val; before returning ARES_FALSE you must free val
using the same allocator used at its allocation (e.g., free(val) or
ares_free(val) depending on how val was allocated), and then return ARES_FALSE;
ensure any other early-return paths in the function also free val to avoid
similar leaks.
In `@deps/undici/src/lib/interceptor/dns.js`:
- Around line 24-26: Update the DNSStorage.set signature to accept the third
options parameter (e.g., set(hostname, records, options)) so it matches the
TypeScript contract and callers that pass { ttl }. Modify the implementation of
DNSStorage.set to include the options parameter (even if unused) and forward or
ignore it safely, and update any internal references to call sites like the call
at line where set is invoked with { ttl: minTTL } to ensure no parameter is
dropped; keep the existing behavior of storing hostname/records in this.#records
while matching the declared function signature in interceptors.d.ts.
In `@doc/api/packages.md`:
- Around line 1164-1179: Update the two broken Markdown link definitions so they
point to the actual anchor IDs: change the WebAssembly modules reference from
esm.md#wasm-modules to esm.md#esm_experimental_wasm_modules and change
customization hooks from module.md#customization-hooks to
module.md#module_customization_hooks; leave the error-code links as-is (their
case-insensitive anchors work) and ensure you update the entries in the same
link-definition block that contains symbols like [`"exports"`], [`"imports"`],
and [entry points].
In `@doc/api/worker_threads.md`:
- Around line 1833-1834: Restore the full description for the worker.performance
property by replacing the truncated sentence with a complete sentence such as:
"An object that can be used to query performance information from a worker
instance. Similar to perf_hooks.performance." Ensure the reference link for
`perf_hooks.performance` (the `[perf_hooks.performance][]` anchor) is re-added
at the end of the document if it was removed so the inline reference resolves
correctly; update the `worker.performance` documentation block to include the
restored text and the reference link.
In `@lib/fs.js`:
- Around line 1278-1286: The fsync function sets req.oncomplete =
makeCallback(callback) but calls the raw callback on the permission-denied path;
change the error path to invoke the wrapped callback (i.e., use req.oncomplete
or call makeCallback(callback) result) so the callback is always the wrapped
version—update the permission check in fsync to call req.oncomplete(new
ERR_ACCESS_DENIED(...)) or assign and call the wrapped callback instead of
calling the original callback directly (refer to fsync, FSReqCallback,
makeCallback, permission.isEnabled, ERR_ACCESS_DENIED, binding.fsync).
- Around line 1246-1255: The permission-denied path uses the raw callback
instead of the wrapped one; set req.oncomplete = makeCallback(callback) first
(already done) and invoke that wrapped callback with the error instead of
calling callback directly — i.e., replace callback(new ERR_ACCESS_DENIED(...))
with req.oncomplete(new ERR_ACCESS_DENIED('fdatasync API is disabled when
Permission Model is enabled.')); keep the early return.
🟡 Minor comments (22)
deps/undici/src/lib/dispatcher/dispatcher-base.js-19-20 (1)
19-20: JSDoc type annotation has a syntax error.The generic type is missing its closing angle bracket:
Array<Function|null}should beArray<Function|null>}or more likelyArray<Function>|null}to match the semantics used on line 25.Since this is vendored undici code, consider reporting this upstream if not already fixed.
Suggested fix
- /** `@type` {Array<Function|null} */ + /** `@type` {Array<Function>|null} */ [kOnDestroyed] = null;deps/undici/src/docs/docs/api/WebSocket.md-48-48 (1)
48-48: MissingWebSocketimport in the example.The code example imports only
Agentbut usesWebSocketon line 52 without importing it. This will cause aReferenceErrorwhen users try to run the example.📝 Proposed fix
-import { Agent } from 'undici' +import { WebSocket, Agent } from 'undici'doc/api/v8.md-1403-1419 (1)
1403-1419: Fix grammar in method descriptions.The method descriptions use present participle ("Stopping") instead of present tense. This is inconsistent with Node.js documentation conventions where API method descriptions typically use present tense.
📝 Proposed fix
-Stopping collecting the profile and return the profile data. +Stops collecting the profile and returns the profile data.-Stopping collecting the profile and the profile will be discarded. +Stops collecting the profile and discards the profile data.doc/api/v8.md-1516-1531 (1)
1516-1531: Fix grammar and addusingsyntax example.Line 1524 has a grammar issue similar to the class methods. Additionally, while the documentation claims support for
usingsyntax (line 1525), the example only demonstrates manual resource management withstop(). SinceSymbol.disposeis implemented, an example showing theusingsyntax would be valuable.📝 Proposed fixes
Fix the grammar:
-Starting a CPU profile then return a `SyncCPUProfileHandle` object. +Starts a CPU profile and returns a `SyncCPUProfileHandle` object.Add an additional example demonstrating the
usingsyntax:// Using the 'using' syntax for automatic disposal using handle = v8.startCpuProfile(); // Profile collection stops automatically when 'handle' goes out of scopedoc/api/sqlite.md-989-997 (1)
989-997: Fix the description inconsistency.The heading indicates
size()is a method (note the parentheses), but the description text calls it "a read-only property." This is inconsistent.📝 Proposed fix
### `sqlTagStore.size()` <!-- YAML added: v24.9.0 --> * Returns: {integer} The number of prepared statements currently in the cache. -A read-only property that returns the number of prepared statements currently in the cache. +Returns the number of prepared statements currently in the cache.doc/api/cli.md-4093-4093 (1)
4093-4093: Verify the link fragment.The link reference
[environment_variables]:#environment-variables_1`` may have an incorrect fragment. The static analysis tool flagged this as a potentially invalid link fragment.The section at line 3273 is
## Environment variables, which would typically create the anchor#environment-variables(with a hyphen, not underscore). Please verify that the_1suffix is needed and that the anchor actually uses an underscore.Based on static analysis hints.
deps/undici/src/README.md-194-210 (1)
194-210: Missing import forsetGlobalDispatcher.The example uses
setGlobalDispatcher(client)but doesn't import it. Users copying this example will encounter aReferenceError.📝 Proposed fix
-import { fetch, Agent, interceptors, cacheStores } from 'undici'; +import { fetch, Agent, interceptors, cacheStores, setGlobalDispatcher } from 'undici';deps/cares/RELEASE-NOTES.md-27-27 (1)
27-27: Fix PR reference mismatch.The link text shows
#996but the URL points to pull/997. This inconsistency should be corrected.📝 Proposed fix
- [PR `#996`](https://github.com/c-ares/c-ares/pull/997) + [PR `#997`](https://github.com/c-ares/c-ares/pull/997)deps/cares/RELEASE-NOTES.md-24-24 (1)
24-24: Fix capitalization."Windows" should be capitalized as it refers to the Microsoft operating system. As per static analysis hints, this is a proper noun.
📝 Proposed fix
-* Fix clang build on windows. +* Fix clang build on Windows.deps/cares/RELEASE-NOTES.md-6-7 (1)
6-7: Wrap bare URL in angle brackets for markdown compliance.The CVE number and GitHub Security Advisory reference are correct. However, the bare URL on line 7 should be wrapped in angle brackets to comply with markdown linting rules:
Markdown fix
-* CVE-2025-62408. A use-after-free bug has been uncovered in read_answers() that - was introduced in v1.32.3. Please see https://github.com/c-ares/c-ares/security/advisories/GHSA-jq53-42q6-pqr5 +* CVE-2025-62408. A use-after-free bug has been uncovered in read_answers() that + was introduced in v1.32.3. Please see <https://github.com/c-ares/c-ares/security/advisories/GHSA-jq53-42q6-pqr5>deps/undici/src/lib/handler/cache-handler.js-199-203 (1)
199-203: Address potential async store handling in 304 Not Modified path.While the current built-in store implementations (
MemoryCacheStoreandSqliteCacheStore) return synchronously, the cache interceptor's design (cache.js lines 426-428) explicitly accounts for stores that return Promises viatypeof result.then === 'function'checks. The 304 path at line 199 callsthis.#store.get()without handling this case, making it incompatible with async store implementations. For consistency and future extensibility, this should either handle the Promise case or document that custom stores must be synchronous.deps/undici/src/lib/web/fetch/formdata-parser.js-537-555 (1)
537-555: Typo in comment:// +should be// =The comment on line 553 says
// +but0x3dis the ASCII code for=(equals sign), not+(plus sign, which is0x2b).📝 Proposed fix
- char === 0x3d // + + char === 0x3d // =deps/undici/src/lib/interceptor/deduplicate.js-14-23 (1)
14-23: Validation ofoptsoccurs after destructuring.The
typeof opts !== 'object'check on line 21 happens after destructuringoptson lines 15-19. Ifoptsis a non-object primitive (e.g., a string or number), the destructuring will succeed but yield unexpectedundefinedvalues rather than throwing the intendedTypeError.Consider reordering validation before destructuring:
🛠️ Suggested fix
module.exports = (opts = {}) => { + if (typeof opts !== 'object' || opts === null) { + throw new TypeError(`expected type of opts to be an Object, got ${opts === null ? 'null' : typeof opts}`) + } + const { methods = ['GET'], skipHeaderNames = [], excludeHeaderNames = [] } = opts - if (typeof opts !== 'object' || opts === null) { - throw new TypeError(`expected type of opts to be an Object, got ${opts === null ? 'null' : typeof opts}`) - }deps/undici/src/types/round-robin-pool.d.ts-14-15 (1)
14-15: Minor typo in JSDoc comment.The comment references
pool.destroyed()but the method ispool.destroy(). The property itself (destroyed) is correctly a boolean.📝 Suggested fix
- /** `true` after `pool.destroyed()` has been called or `pool.close()` has been called and the pool shutdown has completed. */ + /** `true` after `pool.destroy()` has been called or `pool.close()` has been called and the pool shutdown has completed. */deps/undici/undici.js-2141-2141 (1)
2141-2141: Minor JSDoc typo.The closing angle bracket is missing in the type annotation.
📝 Suggested fix
- /** `@type` {Array<Function|null} */ + /** `@type` {Array<Function>|null} */doc/changelogs/CHANGELOG_V24.md-68-82 (1)
68-82: Fix Markdown list formatting in 24.13.0 “Notable Changes” (currently nests subsystem labels into the previous bullet).
Lines 74/76/78/80 are indented, so they’ll render as part of the previous list item instead of as their own headings/grouping, which makes the section hard to read (and possibly lint-failing depending on rules).Proposed formatting fix
### Notable Changes -lib: - -* (CVE-2025-59465) add TLSSocket default error handler (RafaelGSS) <https://github.com/nodejs-private/node-private/pull/797> -* (CVE-2025-55132) disable futimes when permission model is enabled (RafaelGSS) <https://github.com/nodejs-private/node-private/pull/748> - lib,permission: -* (CVE-2025-55130) require full read and write to symlink APIs (RafaelGSS) <https://github.com/nodejs-private/node-private/pull/760> - src: -* (CVE-2025-59466) rethrow stack overflow exceptions in async\_hooks (Matteo Collina) <https://github.com/nodejs-private/node-private/pull/773> - src,lib: -* (CVE-2025-55131) refactor unsafe buffer creation to remove zero-fill toggle (Сковорода Никита Андреевич) <https://github.com/nodejs-private/node-private/pull/759> - tls: -* (CVE-2026-21637) route callback exceptions through error handlers (Matteo Collina) <https://github.com/nodejs-private/node-private/pull/796> +* (CVE-2025-59465) **lib**: add TLSSocket default error handler (RafaelGSS) <https://github.com/nodejs-private/node-private/pull/797> +* (CVE-2025-55132) **lib**: disable futimes when permission model is enabled (RafaelGSS) <https://github.com/nodejs-private/node-private/pull/748> +* (CVE-2025-55130) **lib,permission**: require full read and write to symlink APIs (RafaelGSS) <https://github.com/nodejs-private/node-private/pull/760> +* (CVE-2025-59466) **src**: rethrow stack overflow exceptions in async\_hooks (Matteo Collina) <https://github.com/nodejs-private/node-private/pull/773> +* (CVE-2025-55131) **src,lib**: refactor unsafe buffer creation to remove zero-fill toggle (Сковорода Никita Андреевич) <https://github.com/nodejs-private/node-private/pull/759> +* (CVE-2026-21637) **tls**: route callback exceptions through error handlers (Matteo Collina) <https://github.com/nodejs-private/node-private/pull/796>doc/node.1-729-735 (1)
729-735: Minor grammar fix inNODE_COMPILE_CACHE_PORTABLEdescription.
“location … remain consistent” → “location … remains consistent”.Proposed fix
- will be hit as long as the location of the modules relative to the cache directory remain + will be hit as long as the location of the modules relative to the cache directory remainsdoc/api/n-api.md-2640-2675 (1)
2640-2675: Use C terminology (NULL) and acfence for consistency with Node-API documentation.The code fence should be
c instead ofcpp (the similarnapi_create_objectfunction above usesc, and the Node-API documentation predominantly usesc, appearing 214 times vs. only 6 times for ```cpp).Additionally, the parameter description should use
NULLinstead ofnullptr—nullptris C++ terminology, whereas the Node-API is a C API and consistently usesNULLin documentation. The phrase "or anullptrthat will be converted tonull" should be "orNULL(which will be treated asnull)".The version metadata (
added: v24.12.0) could not be definitively verified with available repository history.doc/node-config-schema.json-732-760 (1)
732-760: FixminItemsplacement for array schemas (currently not enforced).
minItemsshould be at the array schema level, not nested underitems. This affects multiple array properties generated by the schema builder includingwatch-path.Example fix
{ "items": { - "type": "string", - "minItems": 1 + "type": "string" }, + "minItems": 1, "type": "array" }Regarding
watchappearing in bothnodeOptionsand as a top-level namespace: this is intentional. ThenodeOptions.watchboolean is for backward compatibility (from CLI environment options), while the top-levelwatchobject is a separate namespace for detailed watch configuration (watch-path, watch-kill-signal, etc.).doc/contributing/using-devcontainer.md-16-16 (1)
16-16: Fix typo: "popuplar" → "popular".Proposed fix
-Among them, [Visual Studio Code (VS Code)](https://code.visualstudio.com/) is a very popuplar option. +Among them, [Visual Studio Code (VS Code)](https://code.visualstudio.com/) is a very popular option.doc/contributing/using-devcontainer.md-5-5 (1)
5-5: Fix typo: "pre-genreated" → "pre-generated".Proposed fix
-development container that comes with pre-installed build dependencies and pre-genreated build cache. +development container that comes with pre-installed build dependencies and pre-generated build cache.deps/nghttp2/lib/includes/nghttp2/nghttp2.h-2042-2055 (1)
2042-2055: Fix typo and grammar in docs: "which includes upper cased latter" → "which include upper case letter".Appears at lines 2042-2055 and 2080-2092.
Proposed doc fix
- * Also header fields which includes upper cased latter are also + * Also header fields which include upper case letter are also * treated as error without passing them to this callback.
🧹 Nitpick comments (34)
deps/undici/src/eslint.config.js (1)
19-20: LGTM! Good additions for enforcing Node.js best practices.Both rules are sensible additions:
n/prefer-node-protocolensures clarity by requiring thenode:prefix for built-in modules.n/no-process-exitencourages proper error handling over abrupt termination.Nitpick: Minor style inconsistency—line 19 uses
['error'](array) while line 20 uses'error'(string). Both are functionally equivalent, but using the same format would be slightly more consistent.doc/api/errors.md (1)
706-712: Consider improving description clarity and verify version metadata.The new error code entry is properly formatted, but the description could be more direct and clear:
Current: "An operation related to module loading is customized by an asynchronous loader hook that never settled the promise before the loader thread exits."
Suggested: "An asynchronous loader hook customizing module loading never settled its promise before the loader thread exited."
Additionally, verify whether YAML version metadata should be added. Some error codes in this file include version information (see lines 656-657 for
ABORT_ERR, lines 840-842 forERR_CPU_PROFILE_ALREADY_STARTED), while others don't. Since this is part of the v24.13.0 release, confirm whether the following metadata should be added:<!-- YAML added: v24.13.0 -->deps/nghttp2/lib/nghttp2_map.c (1)
118-121: Minor: Redundant hash computation in debug output.
map_hash(map, bkt->key)is computed twice - once foridxand again in thefprintf. This is debug code (non-WIN32 only), so the performance impact is negligible.♻️ Optional: Reuse computed idx
idx = map_hash(map, bkt->key); - fprintf(stderr, "@%zu hash=%zu key=%d base=%zu distance=%u\n", i, - map_hash(map, bkt->key), bkt->key, idx, bkt->psl); + fprintf(stderr, "@%zu hash=%zu key=%d base=%zu distance=%u\n", i, + idx, bkt->key, idx, bkt->psl);deps/v8/src/logging/log.cc (1)
2306-2323: Minor: Static assert messages could be updated for accuracy.The static_assert messages on lines 2317-2322 still reference "non-Linux platforms," but Darwin is now explicitly supported via the expanded guard. Consider updating to "non-Linux/Darwin platforms" for clarity.
📝 Suggested message update
`#else` static_assert( !v8_flags.perf_prof.value(), - "--perf-prof should be statically disabled on non-Linux platforms"); + "--perf-prof should be statically disabled on non-Linux/Darwin platforms"); static_assert( !v8_flags.perf_basic_prof.value(), - "--perf-basic-prof should be statically disabled on non-Linux platforms"); + "--perf-basic-prof should be statically disabled on non-Linux/Darwin platforms"); `#endif`glossary.md (1)
11-14: LGTM! Helpful additions to the glossary.Both new entries are technically accurate and well-formatted. The Backport definition is particularly relevant for the Node.js LTS workflow.
Optional: Consider making the ASLR definition more specific
The current definition mentions "certain attacks" but could be more informative for readers:
-* **ASLR**: Address Space Layout Randomization. A security technique that randomizes memory addresses - to prevent certain attacks. +* **ASLR**: Address Space Layout Randomization. A security technique that randomizes memory addresses + to prevent exploitation of memory corruption vulnerabilities.This clarifies what type of attacks ASLR specifically mitigates, though the current definition is still correct.
deps/undici/src/docs/docs/best-practices/crawling.md (1)
11-33: Example format and code are clear and correct.The user agent format example and JavaScript code are well-structured. The privacy guidance regarding role-based contacts is excellent.
Static analysis flagged two minor formatting issues that you may optionally address:
📝 Optional: Address linter hints
Line 13: Add a language identifier to the code fence for consistency:
-``` +```text AcmeCo Crawler - acme.co - contact@acme.co**Line 17:** The bare URL warning for `crawler-team@company.com` is a false positive (it's an example email in prose, not a link), so you can safely ignore it or wrap it in backticks if preferred: ```diff -Instead, use a role-based or organizational contact (e.g., crawler-team@company.com) to protect individual privacy while still enabling communication. +Instead, use a role-based or organizational contact (e.g., `crawler-team@company.com`) to protect individual privacy while still enabling communication.doc/api/util.md (2)
247-250: Improve clarity of themodifyPrototypeoption description.The description "When false do not change the prototype of object while emitting the deprecation warning" could be more specific:
- Specify which object's prototype is being modified (e.g., "the returned wrapper function's prototype")
- Explain what prototype modification occurs by default when
modifyPrototypeistrue- Consider adding a usage example to demonstrate when and why users would set this to
falseThis will help users understand the practical implications of this option.
📝 Suggested documentation improvement
Consider revising to be more explicit:
* `options` {Object} - * `modifyPrototype` {boolean} When false do not change the prototype of object - while emitting the deprecation warning. + * `modifyPrototype` {boolean} When `false`, the returned wrapper function's + prototype will not be modified to match the deprecated function's prototype. **Default:** `true`.And add an example after the existing examples (around line 297) demonstrating this option:
To deprecate a function without modifying its prototype: \`\`\`mjs import { deprecate } from 'node:util'; const oldFunc = () => { /* ... */ }; const newFunc = deprecate(oldFunc, 'Use newShinyFunc instead', 'DEP0XYZ', { modifyPrototype: false }); \`\`\`
228-228: Add example demonstrating theoptionsparameter and clarify how to use it when a deprecation code is not needed.The documentation introduces the
modifyPrototypeoption but provides no examples showing its usage. Since the function signature requires passingcodebeforeoptions, users wanting to use the option without a deprecation code must passundefinedforcode, which should be clarified with an example:import { deprecate } from 'node:util'; // Using options without a deprecation code const fn = deprecate(myFunction, 'msg', undefined, { modifyPrototype: false }); // Or with a deprecation code const fn = deprecate(myFunction, 'msg', 'DEP1234', { modifyPrototype: false });lib/fs.js (1)
1803-1809: Consider wrapping callback before the permission check.When
callbackis provided as the 4th argument (not inferred fromtype), it's used directly without being wrapped bymakeCallback(). This follows the pre-existing behavior in this function, but for the new permission error path, it would be safer to ensure the callback is validated and runs in the proper context.Suggested improvement
function symlink(target, path, type, callback) { if (callback === undefined) { callback = makeCallback(type); type = undefined; } else { validateOneOf(type, 'type', ['dir', 'file', 'junction', null, undefined]); + callback = makeCallback(callback); } // Due to the nature of Node.js runtime, symlinks has different edge cases that can bypass // the permission model security guarantees. Thus, this API is disabled unless fs.read // and fs.write permission has been given. if (permission.isEnabled() && !permission.has('fs')) { callback(new ERR_ACCESS_DENIED('fs.symlink API requires full fs.read and fs.write permissions.')); return; }deps/undici/src/lib/web/infra/index.js (1)
126-131: Consider using a clearer constant for the chunk size.The expression
(2 << 15) - 1equals 65535, but the bit-shift notation is non-standard for this value. Typically(1 << 16) - 1or simply65535would be more readable.Suggested improvement
+const MAX_ARGS_LENGTH = 65535 + function isomorphicDecode (input) { const length = input.length - if ((2 << 15) - 1 > length) { + if (MAX_ARGS_LENGTH > length) { return String.fromCharCode.apply(null, input) } let result = '' let i = 0 - let addition = (2 << 15) - 1 + let addition = MAX_ARGS_LENGTH while (i < length) {deps/undici/src/lib/web/fetch/formdata-parser.js (1)
274-276: Consider catchingdecodeURIComponenterrors.If the percent-encoded value in the extended attribute is malformed (e.g.,
%GG),decodeURIComponentwill throw aURIError. This would propagate uncaught instead of being converted to a structured parsing error.♻️ Proposed fix
// Skip utf-8'' and decode the rest - value = decodeURIComponent(decoder.decode(headerValue.subarray(7))) + try { + value = decodeURIComponent(decoder.decode(headerValue.subarray(7))) + } catch { + throw parsingError('invalid percent-encoding in extended attribute') + }deps/undici/src/lib/util/cache.js (1)
374-392: Header casing may cause deduplication key inconsistency.
Object.keys(cacheKey.headers).sort()performs a case-sensitive sort, so headers like"Accept"vs"accept"would produce different key orderings and values. SincemakeCacheKeypasses headers through without normalization, identical requests with differently-cased headers could generate different deduplication keys, causing missed deduplication opportunities.Consider normalizing header keys to lowercase before sorting, or ensure all callers normalize headers before invoking this function.
♻️ Suggested fix
function makeDeduplicationKey (cacheKey, excludeHeaders) { // Create a deterministic string key from the cache key // Include origin, method, path, and sorted headers let key = `${cacheKey.origin}:${cacheKey.method}:${cacheKey.path}` if (cacheKey.headers) { - const sortedHeaders = Object.keys(cacheKey.headers).sort() + const sortedHeaders = Object.keys(cacheKey.headers) + .map(h => h.toLowerCase()) + .sort() for (const header of sortedHeaders) { // Skip excluded headers - if (excludeHeaders?.has(header.toLowerCase())) { + if (excludeHeaders?.has(header)) { continue } - const value = cacheKey.headers[header] + // Use case-insensitive header lookup + const originalKey = Object.keys(cacheKey.headers).find(k => k.toLowerCase() === header) + const value = cacheKey.headers[originalKey] key += `:${header}=${Array.isArray(value) ? value.join(',') : value}` } } return key }deps/undici/src/lib/dispatcher/h2c-client.js (1)
18-19: Unusedconnectvariable extracted from options.The
connectoption is destructured but never used since the baseClientclass now handles connection building internally withuseH2c: true. Consider omitting it from destructuring to avoid confusion:- const { connect, maxConcurrentStreams, pipelining, ...opts } = - clientOpts ?? {} + const { maxConcurrentStreams, pipelining, ...opts } = + clientOpts ?? {}Alternatively, if
connectshould be passed to super, include it inopts.deps/undici/src/lib/dispatcher/round-robin-pool.js (1)
78-84: TTL annotation overwrites existing properties on connect event.Using
Object.assign(target, { ttl: Date.now() })works but could be clearer with direct property assignment:- Object.assign(target, { ttl: Date.now() }) + target.ttl = Date.now()This is a minor readability improvement.
deps/undici/src/lib/util/runtime-features.js (1)
35-38: Consider: Missing error handling for module loading.
detectRuntimeFeatureByExportedPropertycallslazyLoaders[moduleName]()directly without a try/catch. If the module is unavailable (unlikely for core modules likenode:zlibandnode:worker_threads), this would throw instead of returningfalse.Since this is upstream undici code and these are Node.js core modules, this is likely acceptable, but worth noting for edge cases.
deps/undici/src/lib/handler/deduplication-handler.js (2)
147-161: Synthetic controller'sabortedgetter always returnsfalse.The
waitingControllerobject hasget aborted() { return false }(line 151), so the check on lines 159-161 will never trigger. This appears intentional since the response is being replayed from a buffer (the original request already completed successfully), but the dead code could be confusing.If this is intentional defensive coding for interface compliance, consider adding a brief comment. Otherwise, the abort checks can be removed.
179-181: Empty catch blocks are intentional but could benefit from a comment.The empty catch blocks silently ignore errors from waiting handlers. This is correct behavior (one handler's failure shouldn't affect others), but the intent could be clearer.
📝 Suggested improvement
} catch { - // Ignore errors from waiting handlers + // Ignore errors from waiting handlers - one handler's failure shouldn't affect others }Also applies to: 206-208
deps/v8/src/objects/heap-object.h (1)
51-52: Nice utility API; consider documenting that this is the raw map-word slot.
A brief comment like “raw slot for map word (usable during GC / for barriers)” would help prevent accidental misuse.Also applies to: 161-162
deps/v8/src/heap/free-list.cc (1)
35-48:PickNodeFromList()failure path now returns the canonical null sentinel.Setting
*node_size = 0and returningTagged<FreeSpace>()is consistent with callers that gate onis_null().Possible consistency tweak
Consider using the same accessor family for sizing (
size(kRelaxedLoad)vsSize()) in this method vsSearchForNodeInList()to reduce future layout surprises.deps/v8/src/diagnostics/objects-debug.cc (1)
369-376: FreeSpace verifier is a good start; consider expanding invariants if not covered elsewhere.If not already verified by Torque verifiers / other checks, it may be worth asserting basic sanity (e.g., non-negative size, alignment, and (if applicable)
nextstructure).deps/v8/src/heap/heap.cc (1)
6250-6258: HardenHeap::IsFreeSpaceValid()against null + size_t/int compare warnings
objectis immediately dereferenced; adding aDCHECK_NOT_NULL(object)would make misuse fail fast in debug. Also consider normalizing types in theCHECK_LEto avoidsize_tvsint/uint32_tcomparison warnings (often-Werrorin V8 builds).Proposed tweak
bool Heap::IsFreeSpaceValid(const FreeSpace* object) { + DCHECK_NOT_NULL(object); Heap* heap = HeapUtils::GetOwnerHeap(object); Tagged<Object> free_space_map = heap->isolate()->root(RootIndex::kFreeSpaceMap); CHECK(!heap->deserialization_complete() || object->map_slot().contains_map_value(free_space_map.ptr())); - CHECK_LE(offsetof(FreeSpace, next_) + kTaggedSize, - object->size(kRelaxedLoad)); + CHECK_LE(offsetof(FreeSpace, next_) + kTaggedSize, + static_cast<size_t>(object->size(kRelaxedLoad))); return true; }Verification to run: ensure this matches upstream V8 API expectations for
HeapUtils::GetOwnerHeap(...),FreeSpace::map_slot(), and the return type ofFreeSpace::size(...)in this branch.doc/contributing/writing-and-running-benchmarks.md (1)
598-600: Minor grammar: use "teardown" (one word) instead of "tear down"."Teardown" as a noun is conventionally written as one word in technical contexts.
📝 Suggested fix
- If tear down is necessary, register a listener for the `exit` event on + If teardown is necessary, register a listener for the `exit` event ondoc/api/http.md (1)
1188-1228: Example should consume the response viares.resume()(and ideally show'error'handling).
The current “Do nothing” data handler works but trains a non-idiomatic pattern.Proposed example tweak
http.get('http://localhost:3000', { agent }, (res) => { - res.on('data', (data) => { - // Do nothing - }); + res.resume(); // Consume data to free up memory }); }, 5000); // Sending request on 5s interval so it's easy to hit idle timeoutdeps/cares/src/lib/ares_getaddrinfo.c (1)
578-638: Nice deduplication; please confirm edge-case parsing behavior matches previous semantics.
numeric_service_to_port()centralizes validation well (errno, full-consume via*end == '\0', range check). The remaining question is whether inputs like""," 80", or"+80"should be accepted/rejected (strtoul accepts leading whitespace and+).Optional stricter parsing (only if desired)
static ares_bool_t numeric_service_to_port(const char *service, unsigned short *port) { char *end; unsigned long val; + if (service == NULL || port == NULL || *service == '\0') { + return ARES_FALSE; + } + errno = 0; val = strtoul(service, &end, 10); - if (errno == 0 && *end == '\0' && val <= 65535) { + if (errno == 0 && end != service && *end == '\0' && val <= 65535UL) { *port = (unsigned short)val; return ARES_TRUE; }If there are existing resolver/getaddrinfo tests in this repo, it’d be ideal to add/adjust one for the accepted numeric-service grammar (especially around whitespace and empty-string handling).
doc/api/module.md (1)
386-447: Fix a few grammar/wording nits to keep the docs crisp (and reduce ambiguity).
Notably: “a ECMAScript Module” → “an ECMAScript module”, “can not” → “cannot”.Proposed doc tweak
- whenever Node.js compiles a CommonJS, a ECMAScript Module, or a TypeScript module, it will + whenever Node.js compiles a CommonJS module, an ECMAScript module, or a TypeScript module, it will use on-disk [V8 code cache][] persisted in the specified directory to speed up the compilation. @@ -Compilation cache generated by one version of Node.js can not be reused by a different +Compilation cache generated by one version of Node.js cannot be reused by a different version of Node.js. Cache generated by different versions of Node.js will be stored.github/CODEOWNERS (1)
236-236: Use a recursive glob to cover nested devcontainer files.The pattern
/.devcontainer/*only matches files directly within.devcontainer/, but.devcontainer/base/devcontainer.jsonexists in a nested subdirectory and won't be covered by this pattern. Change to/.devcontainer/**to ensure the@nodejs/devcontainerteam is pinged for all devcontainer-related changes, including nested files.Suggested change
-/.devcontainer/* `@nodejs/devcontainer` +/.devcontainer/** `@nodejs/devcontainer`.devcontainer/base/devcontainer.json (1)
1-11: Consider pinning the devcontainer image version for reproducibility.Using
nodejs/devcontainer:nightlymay cause inconsistent development environments as the image changes. Consider using a versioned tag or SHA digest if reproducible builds are important for the team.The rest of the configuration looks appropriate - the volume mount for
/outand thegit restore-mtimepost-create command are sensible choices for build caching and avoiding timestamp-based rebuilds.doc/contributing/using-devcontainer.md (1)
50-50: Consider: "drop down" → "dropdown".Proposed fix
-* Select the appropriate Dev Container configuration from the drop down. The base configuration in this +* Select the appropriate Dev Container configuration from the dropdown. The base configuration in thisbenchmark/fixtures/import-meta-dirname-and-filename.mjs (1)
1-1: Consider exporting the value directly to avoid extra per-iteration work in the benchmark.Right now each
default()call allocates a new array; if you want the benchmark to focus onimport.meta.*access, prefer a constant export (and adjust the benchmark accordingly).benchmark/esm/import-meta.js (1)
17-23: Avoid serial dynamic imports (optional), and consider pre-sizing the array.This is setup work (not timed), but
await import()in a loop makes the benchmark startup significantly slower for largern.Proposed tweak (parallelize imports)
async function load(n, fixtureFileURL) { - const array = []; - for (let i = 0; i < n; i++) { - array[i] = await import(`${fixtureFileURL}?i=${i}`); - } - return array; + const imports = new Array(n); + for (let i = 0; i < n; i++) { + imports[i] = import(`${fixtureFileURL}?i=${i}`); + } + return Promise.all(imports); }deps/nghttp2/lib/nghttp2_session.c (1)
3386-3398: Add a GOAWAY reason for glitch-limiter termination (debuggability).
Right now termination isNGHTTP2_ENHANCE_YOUR_CALMwith no reason string, which makes it hard to attribute disconnects in the field.Proposed change
- return nghttp2_session_terminate_session(session, NGHTTP2_ENHANCE_YOUR_CALM); + return nghttp2_session_terminate_session_with_reason( + session, NGHTTP2_ENHANCE_YOUR_CALM, "glitch rate limit exceeded");deps/v8/src/regexp/regexp-compiler.cc (2)
353-366: Guard against inverted register ranges when collecting affected registers
for (int i = action->register_from(); i <= to; i++)assumesregister_to() >= register_from(). A defensiveDCHECK_GE(to, action->register_from())would make failures clearer if a new ActionNode type accidentally violates the invariant.
3205-3256: Flags reset strategy is good; consider restoring entry flags before returning
CapturingRegExpFlags flags = compiler->flags();(Line 3226) and resetting per alternative (Lines 3241, 3359) is solid. One small safety improvement: restorecompiler->set_flags(flags);just before returning fromChoiceNode::Emit, so outer emitters don’t observe a flags value modified by the last-emitted alternative.deps/v8/src/regexp/regexp-compiler.h (1)
321-324:set_flush_budgetshould defensively clamp in release builds
DCHECK(to <= UINT16_MAX)won’t prevent truncation in non-debug builds. If callers can ever pass larger values, preferto = std::min(to, int{UINT16_MAX});.
| function main({ n, valuesToRead }) { | ||
| const array = []; | ||
| for (let i = 0; i < n; ++i) { | ||
| array.push({ dirname: '', filename: '', i: 0 }); | ||
| } | ||
| const fixtureDir = path.resolve(__filename, '../../fixtures'); | ||
| const fixtureFile = path.join(fixtureDir, `import-meta-${valuesToRead}.mjs`); | ||
| const fixtureFileURL = pathToFileURL(fixtureFile); | ||
|
|
||
| bench.start(); | ||
| load(array, n, valuesToRead).then((arr) => { | ||
| load(n, fixtureFileURL).then((array) => { | ||
| const results = new Array(n); | ||
| bench.start(); | ||
| for (let i = 0; i < n; i++) { | ||
| results[i] = array[i].default(); | ||
| } | ||
| bench.end(n); | ||
| if (valuesToRead.includes('dirname')) assert.strictEqual(arr[n - 1].dirname, fixtureDir); | ||
| if (valuesToRead.includes('filename')) assert.strictEqual(arr[n - 1].filename, path.join(fixtureDir, `import-meta-${valuesToRead}.mjs`)); | ||
|
|
||
| switch (valuesToRead) { | ||
| case 'dirname-and-filename': | ||
| assert.deepStrictEqual(results[n - 1], [fixtureDir, fixtureFile]); | ||
| break; | ||
| case 'dirname': | ||
| assert.strictEqual(results[n - 1], fixtureDir); | ||
| break; | ||
| case 'filename': | ||
| assert.strictEqual(results[n - 1], fixtureFile); | ||
| break; | ||
| case 'url': | ||
| assert.strictEqual(results[n - 1], `${fixtureFileURL}?i=${n - 1}`); | ||
| break; | ||
| } | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and see the actual code context
cat -n benchmark/esm/import-meta.js | head -60Repository: nodesource/nsolid
Length of output: 1813
🏁 Script executed:
# Check if __dirname is used elsewhere in ESM benchmarks
fd '\.mjs$|\.js$' benchmark/esm --type f -x grep -l "__dirname" {} +Repository: nodesource/nsolid
Length of output: 340
🏁 Script executed:
# Check if fixtures directory structure matches the path resolution
find benchmark -type d -name 'fixtures' 2>/dev/null | head -20Repository: nodesource/nsolid
Length of output: 79
🏁 Script executed:
# Look for other benchmarks using path.resolve with __filename
rg "path\.resolve\(__filename" benchmark/Repository: nodesource/nsolid
Length of output: 154
🏁 Script executed:
# Check how load() is defined and if it handles errors in similar benchmarks
rg -A 5 "function load\(" benchmark/esm/import-meta.jsRepository: nodesource/nsolid
Length of output: 230
🏁 Script executed:
# Check if fixtures directory exists at repo root
find . -maxdepth 2 -type d -name 'fixtures' 2>/dev/nullRepository: nodesource/nsolid
Length of output: 97
🏁 Script executed:
# Check other benchmarks for .then() error handling patterns
rg "\.then\(" benchmark/ -A 2 | head -40Repository: nodesource/nsolid
Length of output: 1907
🏁 Script executed:
# Verify the fixture files referenced actually exist
ls -la benchmark/fixtures/import-meta-*.mjs 2>/dev/null || echo "Fixture files not found at benchmark/fixtures/"Repository: nodesource/nsolid
Length of output: 430
🏁 Script executed:
# Check if there's a root fixtures directory with these files
ls -la fixtures/import-meta-*.mjs 2>/dev/null || echo "No fixtures at repo root"Repository: nodesource/nsolid
Length of output: 85
Use __dirname and add error handling to prevent silent benchmark failures.
- Line 26:
path.resolve(__filename, '../../fixtures')incorrectly resolves to the repo root instead ofbenchmark/fixtures/. Use__dirnamewith../fixturesto reach the correct directory. - Line 30: Unhandled promise rejection on import failure. Add
.catch((err) => { throw err; })to propagate errors instead of silently failing.
Proposed fix
function main({ n, valuesToRead }) {
- const fixtureDir = path.resolve(__filename, '../../fixtures');
+ const fixtureDir = path.resolve(__dirname, '../fixtures');
const fixtureFile = path.join(fixtureDir, `import-meta-${valuesToRead}.mjs`);
const fixtureFileURL = pathToFileURL(fixtureFile);
- load(n, fixtureFileURL).then((array) => {
+ load(n, fixtureFileURL).then((array) => {
const results = new Array(n);
bench.start();
for (let i = 0; i < n; i++) {
results[i] = array[i].default();
}
bench.end(n);
switch (valuesToRead) {
case 'dirname-and-filename':
assert.deepStrictEqual(results[n - 1], [fixtureDir, fixtureFile]);
break;
case 'dirname':
assert.strictEqual(results[n - 1], fixtureDir);
break;
case 'filename':
assert.strictEqual(results[n - 1], fixtureFile);
break;
case 'url':
assert.strictEqual(results[n - 1], `${fixtureFileURL}?i=${n - 1}`);
break;
}
- });
+ }).catch((err) => { throw err; });
}🤖 Prompt for AI Agents
In `@benchmark/esm/import-meta.js` around lines 25 - 52, In main(), change how
fixtureDir is resolved: replace path.resolve(__filename, '../../fixtures') with
path.resolve(__dirname, '../fixtures') (or use __dirname + '/.. /fixtures') so
fixtureDir points to benchmark/fixtures and fixtureFile/fixtureFileURL use that
correct dir; also handle import errors from load(...) by appending a .catch(err
=> { throw err; }) (or otherwise rethrow/log and process.exit) after the
.then(...) to ensure promise rejections propagate instead of silently failing;
reference the main function, variables fixtureDir, fixtureFileURL, and the
load(...) call when applying the changes.
| static napi_value global_names[20]; | ||
| static napi_value global_values[20]; | ||
| static bool global_properties_initialized = false; | ||
|
|
||
| // Creating with many options because complains are when ~20 properties | ||
| static void InitializeTestProperties(napi_env env) { | ||
| if (global_properties_initialized) return; | ||
|
|
||
| for (int i = 0; i < 20; i++) { | ||
| std::string name = "foo" + std::to_string(i); | ||
| napi_create_string_utf8( | ||
| env, name.c_str(), NAPI_AUTO_LENGTH, &global_names[i]); | ||
| napi_create_string_utf8( | ||
| env, name.c_str(), NAPI_AUTO_LENGTH, &global_values[i]); | ||
| } | ||
| global_properties_initialized = true; | ||
| } |
There was a problem hiding this comment.
Global napi_value handles may become stale across benchmark iterations.
napi_value handles are scoped to the current callback invocation and may be garbage collected between calls. Storing them in globals means subsequent benchmark runs could use invalid handles, causing undefined behavior or crashes.
Consider one of these approaches:
- Use
napi_create_referenceto prevent GC of the values - Re-initialize properties on each call (acceptable overhead for benchmarks since it's outside the timed section)
- Create a persistent reference that you retrieve on each call
🔧 Suggested fix using napi_ref for persistent storage
-static napi_value global_names[20];
-static napi_value global_values[20];
-static bool global_properties_initialized = false;
+static napi_ref global_names_refs[20];
+static napi_ref global_values_refs[20];
+static bool global_properties_initialized = false;
// Creating with many options because complains are when ~20 properties
-static void InitializeTestProperties(napi_env env) {
+static void InitializeTestProperties(napi_env env,
+ napi_value* names,
+ napi_value* values) {
if (global_properties_initialized) return;
for (int i = 0; i < 20; i++) {
std::string name = "foo" + std::to_string(i);
+ napi_value name_val, value_val;
napi_create_string_utf8(
- env, name.c_str(), NAPI_AUTO_LENGTH, &global_names[i]);
+ env, name.c_str(), NAPI_AUTO_LENGTH, &name_val);
napi_create_string_utf8(
- env, name.c_str(), NAPI_AUTO_LENGTH, &global_values[i]);
+ env, name.c_str(), NAPI_AUTO_LENGTH, &value_val);
+ napi_create_reference(env, name_val, 1, &global_names_refs[i]);
+ napi_create_reference(env, value_val, 1, &global_values_refs[i]);
}
global_properties_initialized = true;
+
+ // Retrieve values for this call
+ for (int i = 0; i < 20; i++) {
+ napi_get_reference_value(env, global_names_refs[i], &names[i]);
+ napi_get_reference_value(env, global_values_refs[i], &values[i]);
+ }
}| void ares_set_query_enqueue_cb(ares_channel_t *channel, | ||
| ares_query_enqueue_cb callback, | ||
| void *user_data) | ||
| { | ||
| if (channel == NULL) { | ||
| return; | ||
| } | ||
|
|
||
| channel->query_enqueue_cb = callback; | ||
| channel->query_enqueue_cb_data = user_data; | ||
| } |
There was a problem hiding this comment.
Make ares_set_query_enqueue_cb thread-safe (missing channel lock).
Other setters in this file lock the channel; this one should too.
Proposed fix
void ares_set_query_enqueue_cb(ares_channel_t *channel,
ares_query_enqueue_cb callback,
void *user_data)
{
if (channel == NULL) {
return;
}
- channel->query_enqueue_cb = callback;
- channel->query_enqueue_cb_data = user_data;
+ ares_channel_lock(channel);
+ channel->query_enqueue_cb = callback;
+ channel->query_enqueue_cb_data = user_data;
+ ares_channel_unlock(channel);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| void ares_set_query_enqueue_cb(ares_channel_t *channel, | |
| ares_query_enqueue_cb callback, | |
| void *user_data) | |
| { | |
| if (channel == NULL) { | |
| return; | |
| } | |
| channel->query_enqueue_cb = callback; | |
| channel->query_enqueue_cb_data = user_data; | |
| } | |
| void ares_set_query_enqueue_cb(ares_channel_t *channel, | |
| ares_query_enqueue_cb callback, | |
| void *user_data) | |
| { | |
| if (channel == NULL) { | |
| return; | |
| } | |
| ares_channel_lock(channel); | |
| channel->query_enqueue_cb = callback; | |
| channel->query_enqueue_cb_data = user_data; | |
| ares_channel_unlock(channel); | |
| } |
🤖 Prompt for AI Agents
In `@deps/cares/src/lib/ares_init.c` around lines 607 - 617, The setter
ares_set_query_enqueue_cb currently updates channel->query_enqueue_cb and
channel->query_enqueue_cb_data without holding the channel lock; make it
thread-safe by acquiring the channel lock used elsewhere in this file before
assigning those fields and releasing it afterward (follow the same lock/unlock
calls used by other setters in this file), while keeping the early return when
channel == NULL.
| len = WideCharToMultiByte(CP_UTF8, 0, val, -1, NULL, 0, NULL, NULL); | ||
| if (len == 0) { | ||
| return ARES_FALSE; | ||
| } |
There was a problem hiding this comment.
Memory leak: val is not freed when WideCharToMultiByte fails.
If WideCharToMultiByte returns 0 on line 115, the function returns ARES_FALSE without freeing the val buffer allocated on line 102.
🐛 Proposed fix
/* Convert to UTF8 */
len = WideCharToMultiByte(CP_UTF8, 0, val, -1, NULL, 0, NULL, NULL);
if (len == 0) {
+ ares_free(val);
return ARES_FALSE;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| len = WideCharToMultiByte(CP_UTF8, 0, val, -1, NULL, 0, NULL, NULL); | |
| if (len == 0) { | |
| return ARES_FALSE; | |
| } | |
| len = WideCharToMultiByte(CP_UTF8, 0, val, -1, NULL, 0, NULL, NULL); | |
| if (len == 0) { | |
| ares_free(val); | |
| return ARES_FALSE; | |
| } |
🤖 Prompt for AI Agents
In `@deps/cares/src/lib/ares_sysconfig_win.c` around lines 115 - 118, The early
return when WideCharToMultiByte returns 0 leaks the buffer pointed to by val;
before returning ARES_FALSE you must free val using the same allocator used at
its allocation (e.g., free(val) or ares_free(val) depending on how val was
allocated), and then return ARES_FALSE; ensure any other early-return paths in
the function also free val to avoid similar leaks.
| set (hostname, records) { | ||
| this.#records.set(hostname, records) | ||
| } |
There was a problem hiding this comment.
Signature mismatch: set ignores the options parameter.
The DNSStorage.set method accepts only (hostname, records), but it's called at line 275 with a third { ttl: minTTL } argument. The TypeScript definition in interceptors.d.ts (line 26) expects set(origin, records, options: { ttl: number }).
This means the ttl option—intended for external storage implementations—is silently ignored by the built-in storage. Update the signature to accept the options parameter for consistency with the TypeScript contract.
🔧 Proposed fix
- set (hostname, records) {
+ set (hostname, records, options) {
this.#records.set(hostname, records)
}Even if the built-in storage doesn't use ttl, accepting the parameter ensures signature compatibility with external implementations and the TypeScript types.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| set (hostname, records) { | |
| this.#records.set(hostname, records) | |
| } | |
| set (hostname, records, options) { | |
| this.#records.set(hostname, records) | |
| } |
🤖 Prompt for AI Agents
In `@deps/undici/src/lib/interceptor/dns.js` around lines 24 - 26, Update the
DNSStorage.set signature to accept the third options parameter (e.g.,
set(hostname, records, options)) so it matches the TypeScript contract and
callers that pass { ttl }. Modify the implementation of DNSStorage.set to
include the options parameter (even if unused) and forward or ignore it safely,
and update any internal references to call sites like the call at line where set
is invoked with { ttl: minTTL } to ensure no parameter is dropped; keep the
existing behavior of storing hostname/records in this.#records while matching
the declared function signature in interceptors.d.ts.
| [TypeScript]: typescript.md | ||
| [WebAssembly modules]: esm.md#wasm-modules | ||
| [WinterCG]: https://wintercg.org/ | ||
| [`"exports"`]: #exports | ||
| [`"imports"`]: #imports | ||
| [`"main"`]: #main | ||
| [`"name"`]: #name | ||
| [`"type"`]: #type | ||
| [`--conditions` / `-C` flag]: #resolving-user-conditions | ||
| [`--experimental-addon-modules`]: cli.md#--experimental-addon-modules | ||
| [`--no-addons` flag]: cli.md#--no-addons | ||
| [`ERR_PACKAGE_PATH_NOT_EXPORTED`]: errors.md#err_package_path_not_exported | ||
| [`ERR_UNKNOWN_FILE_EXTENSION`]: errors.md#err_unknown_file_extension | ||
| [`package.json`]: #nodejs-packagejson-field-definitions | ||
| [customization hooks]: module.md#customization-hooks | ||
| [entry points]: #package-entry-points |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the referenced doc files exist and anchors are present somewhere in-file
set -euo pipefail
echo "=== Checking file existence ==="
for f in doc/api/typescript.md doc/api/esm.md doc/api/module.md doc/api/errors.md; do
if test -f "$f"; then
echo "✓ $f exists"
else
echo "✗ $f missing"
fi
done
echo -e "\n=== Checking anchors ==="
echo "Checking wasm-modules in doc/api/esm.md:"
rg -n "wasm-modules" doc/api/esm.md || echo " (not found)"
echo -e "\nChecking customization-hooks in doc/api/module.md:"
rg -n "customization.hooks|customization-hooks" doc/api/module.md || echo " (not found)"
echo -e "\nChecking err_unknown_file_extension in doc/api/errors.md:"
rg -n "err_unknown_file_extension" doc/api/errors.md -i || echo " (not found)"
echo -e "\nChecking err_package_path_not_exported in doc/api/errors.md:"
rg -n "err_package_path_not_exported" doc/api/errors.md -i || echo " (not found)"Repository: nodesource/nsolid
Length of output: 1207
🏁 Script executed:
#!/bin/bash
# Get context around the anchor references to verify they are properly defined
echo "=== Context around wasm-modules in esm.md (line 715) ==="
sed -n '710,720p' doc/api/esm.md
echo -e "\n=== Context around customization-hooks in module.md (line 1815) ==="
sed -n '1810,1825p' doc/api/module.md
echo -e "\n=== Check for wasm-modules anchor ID specifically ==="
rg -n 'id=".*wasm' doc/api/esm.md || echo " (not found)"
echo -e "\n=== Check for customization-hooks anchor ID in module.md ==="
rg -n 'id=".*customization' doc/api/module.md || echo " (not found)"
echo -e "\n=== Check reference link definitions in module.md ==="
rg -n '\[customization hooks\]' doc/api/module.mdRepository: nodesource/nsolid
Length of output: 1524
🏁 Script executed:
#!/bin/bash
# Check if HTML anchors are case-insensitive by examining both the anchor and link references
echo "=== Full context for ERR_UNKNOWN_FILE_EXTENSION anchor ==="
sed -n '3173,3180p' doc/api/errors.md
echo -e "\n=== Looking for the link reference format in packages.md ==="
rg -n "ERR_UNKNOWN_FILE_EXTENSION|err_unknown_file_extension" doc/api/packages.md
echo -e "\n=== Summary of anchor ID formats found ==="
echo "ESM file uses: esm_experimental_wasm_modules (line 707)"
echo "Module file uses: module_customization_hooks (line 581)"
echo "Expected by links: wasm-modules and customization-hooks"Repository: nodesource/nsolid
Length of output: 779
Fix anchor references for WebAssembly modules and customization hooks links.
All referenced files exist, but two cross-reference anchors are broken:
[WebAssembly modules]: esm.md#wasm-modulestargets#wasm-modules, but the actual anchor ID in esm.md (line 707) isesm_experimental_wasm_modules[customization hooks]: module.md#customization-hookstargets#customization-hooks, but the actual anchor ID in module.md (line 581) ismodule_customization_hooks
Update the link definitions to reference the correct anchor IDs. The error code links use case-insensitive HTML anchors (ERR_UNKNOWN_FILE_EXTENSION vs err_unknown_file_extension) which function but are not exact matches.
🤖 Prompt for AI Agents
In `@doc/api/packages.md` around lines 1164 - 1179, Update the two broken Markdown
link definitions so they point to the actual anchor IDs: change the WebAssembly
modules reference from esm.md#wasm-modules to
esm.md#esm_experimental_wasm_modules and change customization hooks from
module.md#customization-hooks to module.md#module_customization_hooks; leave the
error-code links as-is (their case-insensitive anchors work) and ensure you
update the entries in the same link-definition block that contains symbols like
[`"exports"`], [`"imports"`], and [entry points].
| An object that can be used to query performance information from a worker | ||
| instance. Similar to [`perf_hooks.performance`][]. | ||
| instance. |
There was a problem hiding this comment.
Restore the complete description for worker.performance.
The description for worker.performance has been truncated to just "instance." which is incomplete and uninformative. The original description provided useful context:
"An object that can be used to query performance information from a worker instance. Similar to perf_hooks.performance."
Please restore the complete description or provide an equivalent full description of this property.
📝 Suggested fix
An object that can be used to query performance information from a worker
-instance.
+instance. Similar to [`perf_hooks.performance`][].Note: You may also need to verify whether the reference link [perf_hooks.performance][] should be restored at the end of the file if it was removed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| An object that can be used to query performance information from a worker | |
| instance. Similar to [`perf_hooks.performance`][]. | |
| instance. | |
| An object that can be used to query performance information from a worker | |
| instance. Similar to [`perf_hooks.performance`][]. |
🤖 Prompt for AI Agents
In `@doc/api/worker_threads.md` around lines 1833 - 1834, Restore the full
description for the worker.performance property by replacing the truncated
sentence with a complete sentence such as: "An object that can be used to query
performance information from a worker instance. Similar to
perf_hooks.performance." Ensure the reference link for `perf_hooks.performance`
(the `[perf_hooks.performance][]` anchor) is re-added at the end of the document
if it was removed so the inline reference resolves correctly; update the
`worker.performance` documentation block to include the restored text and the
reference link.
| function fdatasync(fd, callback) { | ||
| const req = new FSReqCallback(); | ||
| req.oncomplete = makeCallback(callback); | ||
|
|
||
| if (permission.isEnabled()) { | ||
| callback(new ERR_ACCESS_DENIED('fdatasync API is disabled when Permission Model is enabled.')); | ||
| return; | ||
| } | ||
| binding.fdatasync(fd, req); | ||
| } |
There was a problem hiding this comment.
Bug: Permission check uses raw callback instead of the wrapped callback.
The callback is wrapped at line 1248 (req.oncomplete = makeCallback(callback)), but the permission denial error path at line 1251 uses the raw callback. This is inconsistent with other permission-gated APIs like fchmod, fchown, and futimes which wrap the callback first and then use it consistently.
makeCallback() validates the callback is a function and ensures it runs in the proper context. Using the raw callback bypasses these guarantees.
Proposed fix
function fdatasync(fd, callback) {
+ callback = makeCallback(callback);
+
+ if (permission.isEnabled()) {
+ callback(new ERR_ACCESS_DENIED('fdatasync API is disabled when Permission Model is enabled.'));
+ return;
+ }
+
const req = new FSReqCallback();
- req.oncomplete = makeCallback(callback);
-
- if (permission.isEnabled()) {
- callback(new ERR_ACCESS_DENIED('fdatasync API is disabled when Permission Model is enabled.'));
- return;
- }
+ req.oncomplete = callback;
binding.fdatasync(fd, req);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function fdatasync(fd, callback) { | |
| const req = new FSReqCallback(); | |
| req.oncomplete = makeCallback(callback); | |
| if (permission.isEnabled()) { | |
| callback(new ERR_ACCESS_DENIED('fdatasync API is disabled when Permission Model is enabled.')); | |
| return; | |
| } | |
| binding.fdatasync(fd, req); | |
| } | |
| function fdatasync(fd, callback) { | |
| callback = makeCallback(callback); | |
| if (permission.isEnabled()) { | |
| callback(new ERR_ACCESS_DENIED('fdatasync API is disabled when Permission Model is enabled.')); | |
| return; | |
| } | |
| const req = new FSReqCallback(); | |
| req.oncomplete = callback; | |
| binding.fdatasync(fd, req); | |
| } |
🤖 Prompt for AI Agents
In `@lib/fs.js` around lines 1246 - 1255, The permission-denied path uses the raw
callback instead of the wrapped one; set req.oncomplete = makeCallback(callback)
first (already done) and invoke that wrapped callback with the error instead of
calling callback directly — i.e., replace callback(new ERR_ACCESS_DENIED(...))
with req.oncomplete(new ERR_ACCESS_DENIED('fdatasync API is disabled when
Permission Model is enabled.')); keep the early return.
| function fsync(fd, callback) { | ||
| const req = new FSReqCallback(); | ||
| req.oncomplete = makeCallback(callback); | ||
| if (permission.isEnabled()) { | ||
| callback(new ERR_ACCESS_DENIED('fsync API is disabled when Permission Model is enabled.')); | ||
| return; | ||
| } | ||
| binding.fsync(fd, req); | ||
| } |
There was a problem hiding this comment.
Bug: Same issue as fdatasync - permission check uses raw callback.
The callback is wrapped at line 1280 but the error path at line 1282 uses the raw callback.
Proposed fix
function fsync(fd, callback) {
+ callback = makeCallback(callback);
+
+ if (permission.isEnabled()) {
+ callback(new ERR_ACCESS_DENIED('fsync API is disabled when Permission Model is enabled.'));
+ return;
+ }
+
const req = new FSReqCallback();
- req.oncomplete = makeCallback(callback);
- if (permission.isEnabled()) {
- callback(new ERR_ACCESS_DENIED('fsync API is disabled when Permission Model is enabled.'));
- return;
- }
+ req.oncomplete = callback;
binding.fsync(fd, req);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function fsync(fd, callback) { | |
| const req = new FSReqCallback(); | |
| req.oncomplete = makeCallback(callback); | |
| if (permission.isEnabled()) { | |
| callback(new ERR_ACCESS_DENIED('fsync API is disabled when Permission Model is enabled.')); | |
| return; | |
| } | |
| binding.fsync(fd, req); | |
| } | |
| function fsync(fd, callback) { | |
| callback = makeCallback(callback); | |
| if (permission.isEnabled()) { | |
| callback(new ERR_ACCESS_DENIED('fsync API is disabled when Permission Model is enabled.')); | |
| return; | |
| } | |
| const req = new FSReqCallback(); | |
| req.oncomplete = callback; | |
| binding.fsync(fd, req); | |
| } |
🤖 Prompt for AI Agents
In `@lib/fs.js` around lines 1278 - 1286, The fsync function sets req.oncomplete =
makeCallback(callback) but calls the raw callback on the permission-denied path;
change the error path to invoke the wrapped callback (i.e., use req.oncomplete
or call makeCallback(callback) result) so the callback is always the wrapped
version—update the permission check in fsync to call req.oncomplete(new
ERR_ACCESS_DENIED(...)) or assign and call the wrapped callback instead of
calling the original callback directly (refer to fsync, FSReqCallback,
makeCallback, permission.isEnabled, ERR_ACCESS_DENIED, binding.fsync).
Don't set `OtlpGrpcClientOptions.use_ssl_credentials` when also setting `OtlpGrpcClientOptions.credentials`.
Specifically in `EnvInst::CustomCommandReqWeakCallback()` when trying to access `EnvInst` associated with the `Isolate`. Observed when load-testing the `test/addons/nsolid-custom-command` test.
santigimeno
force-pushed
the
node-v24.13.0-nsolid-v6.2.0-release
branch
from
61f223c to
5554e72
Compare
20 participants
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.