Node v22.15.1 nsolid v5.7.2 release#310
Conversation
Always call uv_fs_req_cleanup after calling uv_fs_open instead of just when uv_fs_open returns a negative result. I referenced ReadFileSync from node:js2c when making this change. https://github.com/bnoordhuis made the same suggestion based on the PR nodejs/node#49691. Fixes: nodejs/node#57800 PR-URL: nodejs/node#57811 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> CVE-ID: CVE-2025-23165
Added a unit test for testing the memory usage of readFileSync. Test is looking specifically for the the issue caused by failing to free the filepath buffer in fs::ReadFileUtf8(), but it will also catch other significant memory leaks in readFileSync() as well. Refs: nodejs/node#57800 PR-URL: nodejs/node#57811 Fixes: nodejs/node#57800 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Fixes: https://hackerone.com/reports/2817648 Co-Authored-By: Filip Skokan <panva.ip@gmail.com> Co-Authored-By: Tobias Nießen <tniessen@tnie.de> Backport-PR-URL: nodejs-private/node-private#688 CVE-ID: CVE-2025-23166 PR-URL: nodejs-private/node-private#709
This is a security release. Notable changes: src: * (CVE-2025-23166) fix error handling on async crypto operation fs: * (CVE-2025-23165) add missing call to uv\_fs\_req\_cleanup PR-URL: nodejs-private/node-private#712
This reverts commit 3cc36a4.
This reverts commit c2bc3e4.
This reverts commit 24fe19d.
This reverts commit 6566a01.
This reverts commit 4488fb5.
This reverts commit 6f4153c.
This reverts commit 62a40a6.
This reverts commit 1ac895b.
This reverts commit de4bf79.
This reverts commit 74bb085.
2025-05-14 Node.js v22.15.1 'Jod' (LTS) Release Git-EVTag-v0-SHA512: 142fde9dd6f318ca3d53d5cafe996d0e90c9a755b7f824acc02fd6dd57cacdbe90b9d272f1ec8da49dca5d0e10aa02ca7ac3a6cbf456cfbaf32ccadf2cd8c278
|
This PR has too many files to review (>50 files). |
WalkthroughThis set of changes removes all support for continuous CPU profiling from the codebase, including related configuration options, gRPC APIs, protobuf definitions, build files, and test suites. It also introduces a patch-level version update, security fixes, and a new memory leak test for UTF-8 file reads. Several cryptographic "DeriveBits" methods are updated to accept a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant NodeApp
participant NSolid
participant gRPCServer
Note over User,NodeApp: Previous Flow (Continuous Profiling)
User->>NodeApp: Enable continuous CPU profiling
NodeApp->>NSolid: Start continuous profiler
NSolid->>gRPCServer: Stream continuous profile data (ExportContinuousProfile)
gRPCServer-->>NSolid: Ack / EventResponse
Note over User,NodeApp: New Flow (After Change)
User->>NodeApp: (No continuous profiling support)
NodeApp--xNSolid: No continuous profiler logic
NSolid--xgRPCServer: No ExportContinuousProfile RPC
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
agents/grpc/src/grpc_agent.h (1)
119-125:⚠️ Potential issue
timestampis added but never default-initialised – potential UBThe newly-introduced
timestampmember inProfileStordoes not get an in-class initialiser, unlikedone.
Whenever aProfileStoris default-constructed (e.g. asProfileStor ps{}or when it is value-initialised inside a standard container) the field will remain indeterminate, leading to undefined behaviour when it is later compared, logged or serialised.struct ProfileStor { std::string req_id; - uint64_t timestamp; + // Monotonic time when the profile was started (µs since epoch). + // Default-initialised to 0 to avoid indeterminate values. + uint64_t timestamp{0}; AssetStream* stream; ProfileOptions options; bool done = false; };If the timestamp must always be set by the creator, consider enforcing this through a non-default constructor instead.
agents/grpc/src/grpc_agent.cc (1)
371-374:⚠️ Potential issue
statsdTagsmapped to the wrong protobuf field
set_blockedloopthreshold()is invoked for thestatsdtagsconfiguration entry – clearly a copy-paste oversight. This results in the tags silently landing in the wrong place and the realstatsdtagsfield never being populated.- body->set_blockedloopthreshold(*it); + body->set_statsdtags(*it);Please update and add a quick unit test / integration check to ensure every JSON key is routed to the expected proto field.
agents/grpc/src/proto/nsolid_service.grpc.pb.h (1)
1-8: 🛠️ Refactor suggestionGenerated headers should be recreated, not hand-edited
All the
~-marked changes appear inside a file produced by the gRPC C++ code-gen plugin.
Keeping generated artifacts under version control is perfectly fine, but editing them (or committing a version that was hand-tweaked) is brittle – the nextprotoc --grpc_outrun will overwrite these edits, and any missed index renumbering will surface as hard-to-debug runtime errors.Recommended workflow
- Commit only the updated
nsolid_service.proto(and any other.protos).- Add a small
make protoornpm/gypstep to regenerate the C++ stubs as part of the build pipeline.- If the repository must keep generated files, regenerate them locally and commit the untouched output to avoid accidental drift.
This guarantees that:
• AllMarkMethod*indices stay consistent across the seven code-paths (Async,Callback,Generic,Raw,RawCallback,StreamedUnary,SplitStreamed).
• Future removals / additions of RPCs don’t require risky manual edits.
• Reviewers can focus on the authoritative source (.proto) instead of ∼1 700 lines of boilerplate.
🧹 Nitpick comments (5)
test/pummel/test-fs-read-file-sync-utf8-memory.js (2)
1-4: Consider removing redundant 'use strict' directive.JavaScript modules are automatically in strict mode, making this directive unnecessary. However, if this is maintained for consistency with other Node.js files, it can be retained.
-'use strict';🧰 Tools
🪛 Biome (1.9.4)
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
51-54: Clarify assertion logic.The assertion checks that memory growth is LESS than the expected leak threshold, which is counterintuitive at first glance. This is correct since we're verifying the absence of a leak (if fixed, memory growth should be minimal compared to the calculated leak size).
Consider adding a brief comment explaining that the test passes when memory growth is significantly below what would be leaked if the bug were present.
// Use 90% of the expected memory leak as the threshold just to be safe. const memoryDifference = endMemory.rss - startMemory.rss; +// If the bug is fixed, actual memory growth should be far less than what would leak if the bug were present assert.ok(memoryDifference < (minExpectedMemoryLeak * 0.9), `Unexpected memory overhead: ${util.inspect([startMemory, endMemory])}`);doc/changelogs/CHANGELOG_V22.md (1)
70-71: Optional: reorder CVE listings for clarity.For chronological consistency, you might list CVE-2025-23165 before CVE-2025-23166 in the “Notable Changes” section.
agents/grpc/src/asset_stream.h (1)
40-45: Constructor signature: avoid unnecessarystd::stringcopiesWith the removal of the
rpc_typeparameter the constructor now takes:explicit AssetStream(..., const std::string& agent_id, const std::string& saas);Both parameters are read-only. Passing them as
std::string_view(or evenconst char*foragent_id, which is a UUID) would:
- Eliminate the hidden cost of constructing temporary
std::strings at the call-site.- Decouple the API from
std::string, making it usable withabsl::string_view,folly::fbstring, literals, etc.Example diff (header only; implementer must update the
.ccfile accordingly):-explicit AssetStream(..., - const std::string& agent_id, - const std::string& saas); +explicit AssetStream(..., + std::string_view agent_id, + std::string_view saas);Not critical for correctness, but a small win for efficiency and API ergonomics.
agents/grpc/src/grpc_agent.cc (1)
574-581: Potential indefinite wait & minor typo
- If
profile_stopped == trueandpending_profiles()never returnsfalse(e.g. a profile stream hangs), this thread blocks forever. Consider adding a timeout or a fail-safe.- Comment typo: “the are no” → “there are no”.
Purely advisory – no blocker, but worth tightening.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (57)
CHANGELOG.md(1 hunks)agents/grpc/proto/asset.proto(1 hunks)agents/grpc/proto/nsolid_service.proto(0 hunks)agents/grpc/proto/reconfigure.proto(0 hunks)agents/grpc/src/asset_stream.cc(1 hunks)agents/grpc/src/asset_stream.h(1 hunks)agents/grpc/src/grpc_agent.cc(5 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/proto/asset.pb.cc(1 hunks)agents/grpc/src/proto/asset.pb.h(0 hunks)agents/grpc/src/proto/nsolid_service.grpc.pb.cc(9 hunks)agents/grpc/src/proto/nsolid_service.grpc.pb.h(73 hunks)agents/grpc/src/proto/nsolid_service.pb.cc(2 hunks)agents/grpc/src/proto/reconfigure.pb.cc(9 hunks)agents/grpc/src/proto/reconfigure.pb.h(0 hunks)agents/src/profile_collector.h(0 hunks)doc/changelogs/CHANGELOG_V22.md(2 hunks)doc/changelogs/NSOLID_CHANGELOG_V5_NODE_V22.md(1 hunks)lib/nsolid.js(0 hunks)node.gyp(1 hunks)src/crypto/crypto_dh.cc(1 hunks)src/crypto/crypto_dh.h(1 hunks)src/crypto/crypto_ec.cc(1 hunks)src/crypto/crypto_ec.h(1 hunks)src/crypto/crypto_hash.cc(1 hunks)src/crypto/crypto_hash.h(1 hunks)src/crypto/crypto_hkdf.cc(1 hunks)src/crypto/crypto_hkdf.h(1 hunks)src/crypto/crypto_hmac.cc(1 hunks)src/crypto/crypto_hmac.h(1 hunks)src/crypto/crypto_pbkdf2.cc(1 hunks)src/crypto/crypto_pbkdf2.h(1 hunks)src/crypto/crypto_random.cc(3 hunks)src/crypto/crypto_random.h(3 hunks)src/crypto/crypto_scrypt.cc(1 hunks)src/crypto/crypto_scrypt.h(1 hunks)src/crypto/crypto_sig.cc(5 hunks)src/crypto/crypto_sig.h(1 hunks)src/crypto/crypto_util.h(1 hunks)src/node_file.cc(1 hunks)src/node_version.h(2 hunks)src/nsolid/continuous_profiler.cc(0 hunks)src/nsolid/continuous_profiler.h(0 hunks)src/nsolid/nsolid_api.cc(1 hunks)src/nsolid/nsolid_api.h(0 hunks)test/addons/nsolid-dispatchqueue/binding.gyp(0 hunks)test/addons/nsolid-eventloop-cmd/binding.gyp(0 hunks)test/addons/nsolid-log-hooks/binding.gyp(0 hunks)test/addons/nsolid-statsdagent/binding.gyp(0 hunks)test/agents/test-grpc-continuous-profile.mjs(0 hunks)test/agents/test-grpc-reconfigure.mjs(0 hunks)test/common/nsolid-grpc-agent/server.mjs(0 hunks)test/fixtures/test-nsolid-config-continuous-profiling-env-script.js(0 hunks)test/parallel/test-crypto-async-sign-verify.js(2 hunks)test/parallel/test-nsolid-config-continuous-profiling-env.js(0 hunks)test/parallel/test-nsolid-config-continuous-profiling.js(0 hunks)test/pummel/test-fs-read-file-sync-utf8-memory.js(1 hunks)
💤 Files with no reviewable changes (19)
- agents/src/profile_collector.h
- test/addons/nsolid-eventloop-cmd/binding.gyp
- agents/grpc/proto/nsolid_service.proto
- test/addons/nsolid-statsdagent/binding.gyp
- agents/grpc/proto/reconfigure.proto
- test/fixtures/test-nsolid-config-continuous-profiling-env-script.js
- test/agents/test-grpc-reconfigure.mjs
- test/addons/nsolid-log-hooks/binding.gyp
- test/addons/nsolid-dispatchqueue/binding.gyp
- lib/nsolid.js
- test/parallel/test-nsolid-config-continuous-profiling.js
- test/parallel/test-nsolid-config-continuous-profiling-env.js
- src/nsolid/nsolid_api.h
- agents/grpc/src/proto/reconfigure.pb.h
- test/agents/test-grpc-continuous-profile.mjs
- agents/grpc/src/proto/asset.pb.h
- test/common/nsolid-grpc-agent/server.mjs
- src/nsolid/continuous_profiler.h
- src/nsolid/continuous_profiler.cc
🧰 Additional context used
🧬 Code Graph Analysis (4)
src/crypto/crypto_dh.h (4)
src/crypto/crypto_ec.h (11)
env(24-24)env(27-29)env(81-84)env(86-88)env(144-146)env(148-150)env(152-155)env(157-159)params(107-107)mode(75-79)mode(109-113)src/crypto/crypto_hash.h (4)
env(17-17)env(73-76)env(78-80)mode(67-71)src/crypto/crypto_hmac.h (4)
env(18-18)env(76-79)env(81-83)mode(70-74)src/crypto/crypto_random.h (5)
env(35-38)env(40-42)mode(29-33)mode(64-68)mode(98-102)
src/node_file.cc (1)
deps/uv/src/win/fs.c (1)
uv_fs_req_cleanup(2988-3017)
src/crypto/crypto_util.h (5)
src/crypto/crypto_dh.cc (2)
DeriveBits(526-533)DeriveBits(526-529)src/crypto/crypto_hmac.cc (2)
DeriveBits(224-255)DeriveBits(224-227)src/crypto/crypto_hash.cc (2)
DeriveBits(505-535)DeriveBits(505-508)src/crypto/crypto_ec.cc (2)
DeriveBits(451-515)DeriveBits(451-454)src/crypto/crypto_sig.cc (2)
DeriveBits(706-812)DeriveBits(706-709)
agents/grpc/src/proto/reconfigure.pb.cc (1)
agents/grpc/src/proto/reconfigure.pb.h (9)
reinterpret_cast(107-110)reinterpret_cast(436-439)ReconfigureBody(67-67)ReconfigureBody(67-67)ReconfigureBody(68-68)ReconfigureBody(69-69)ReconfigureBody(71-71)ReconfigureBody(72-75)ReconfigureBody(171-172)
🪛 Biome (1.9.4)
test/pummel/test-fs-read-file-sync-utf8-memory.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.
(lint/suspicious/noRedundantUseStrict)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: lint-js-and-md
- GitHub Check: test-linux (ubuntu-24.04-arm)
- GitHub Check: coverage-linux-without-intl
- GitHub Check: coverage-windows
- GitHub Check: test-linux (ubuntu-24.04)
- GitHub Check: coverage-linux
- GitHub Check: test-macOS
🔇 Additional comments (56)
test/pummel/test-fs-read-file-sync-utf8-memory.js (4)
6-7: Well-documented test purpose with issue reference.Excellent documentation that clearly explains what this test is checking for and provides a GitHub issue reference for context.
14-28: Good defensive code for path length handling.The code properly handles potential path length restrictions by checking against MAX_PATH and providing a fallback for platforms with shorter path limits. The detailed comments explaining the memory leak mechanics are very helpful.
30-34: Platform-specific memory calculation is well-explained.The code accounts for different WCHAR sizes across platforms (Windows: 2, Mac: 2, Linux: 4) when calculating the expected memory leak. Using the minimum size (2 bytes per character) provides a conservative baseline for testing.
40-49: Effective baseline establishment before measurement.The warm-up phase helps establish a stable baseline memory usage before the actual test. This improves the reliability of memory leak detection by minimizing the impact of initial allocations.
src/node_file.cc (1)
2606-2607: Good resource management improvement.Moving the
uv_fs_req_cleanup(&req)call before the return statement ensures proper cleanup prior to exception throwing, rather than relying on scope-based cleanup. This addresses a potential resource leak in the error path.src/node_version.h (2)
27-27: Version bump for patch release.Incrementing the patch version from 0 to 1 correctly reflects the Node.js v22.15.1 release mentioned in the PR title.
39-39: Release flag set for NSolid.Setting
NSOLID_VERSION_IS_RELEASEto 1 properly marks NSolid v5.7.2 as a release build rather than a pre-release.doc/changelogs/NSOLID_CHANGELOG_V5_NODE_V22.md (1)
5-9: Changelog entry added for new release.This changelog entry appropriately documents the new version 22.15.1-nsolid-v5.7.2 with the merge commit information.
CHANGELOG.md (1)
41-42: Version update correctly added to changelog.The new version 22.15.1 is properly added as the most recent supported release (with bold formatting), and version 22.15.0 is moved down without bold formatting.
src/crypto/crypto_pbkdf2.cc (1)
114-115: Function signature updated to include CryptoJobMode parameter.The DeriveBits function signature has been extended to include a CryptoJobMode parameter, matching similar changes across other cryptographic methods. While this parameter isn't used within the function implementation yet, this change maintains consistency with the updated interface.
src/crypto/crypto_ec.cc (1)
453-454: Function signature updated to include CryptoJobMode parameter.The ECDHBitsTraits::DeriveBits method has been modified to include the CryptoJobMode parameter, consistent with similar changes across the cryptographic codebase. While the parameter isn't used in the implementation yet, this standardizes the interface.
src/crypto/crypto_dh.cc (1)
526-529: Function signature updated to include CryptoJobMode parameter.The DHBitsTraits::DeriveBits method now includes a CryptoJobMode parameter, maintaining consistency with the broader interface changes across cryptographic derivation methods. The implementation itself remains unchanged.
src/crypto/crypto_dh.h (1)
106-109: Signature update aligns with crypto module standardizationThe addition of the
CryptoJobMode modeparameter to theDeriveBitsmethod signature is part of a consistent pattern across the crypto module, matching similar changes in other crypto trait structs (ECDHBitsTraits,HashTraits,HmacTraits, etc.). This standardization allows for different error handling behavior between synchronous and asynchronous operations.Please ensure that the implementation in
src/crypto/crypto_dh.ccproperly handles this new parameter and that error handling is consistent with other crypto implementations, particularly around suppressing exceptions in asynchronous mode.doc/changelogs/CHANGELOG_V22.md (2)
12-15: Navigation table updated correctly.The new version link for 22.15.1 is inserted above the previous entries and follows the same
<a href="#...">pattern.
62-68: Release entry added correctly.The anchor ID, date, version header, and "security release" designation match the existing changelog style.
src/crypto/crypto_hkdf.cc (1)
99-102: Function signature updated to include job mode parameter.The
DeriveBitsmethod signature has been updated to include theCryptoJobMode modeparameter, aligning with similar changes across multiple crypto trait classes. While the implementation doesn't currently use this parameter, the change ensures a consistent interface across all cryptographic derivation functions.src/crypto/crypto_pbkdf2.h (1)
58-61: Function signature updated to include job mode parameter.The
DeriveBitsstatic method signature now includes theCryptoJobMode modeparameter, as part of a systematic update across all cryptographic trait classes. This change maintains interface consistency in the crypto subsystem.src/crypto/crypto_util.h (1)
470-474: Improved error handling and mode parameter passing.Two key improvements have been made:
- Added
ClearErrorOnReturnto ensure OpenSSL errors are properly cleared on function exit- Now passing the job mode to the
DeriveBitscall, supporting the updated function signaturesThese changes enhance error state management and ensure proper job mode context is available to all crypto operations.
src/crypto/crypto_ec.h (1)
81-84: Function signature updated to include job mode parameter.The
DeriveBitsmethod signature has been modified to include theCryptoJobMode modeparameter, consistent with similar changes across the crypto subsystem. This standardizes the interface for all cryptographic operations that derive bits.src/crypto/crypto_sig.h (1)
149-152: Function signature updated for consistencyThe
SignTraits::DeriveBitsmethod signature has been updated to include aCryptoJobMode modeparameter, consistent with other crypto trait classes. According to implementation details, this parameter is used to conditionally suppress exceptions in asynchronous mode.src/crypto/crypto_hmac.h (1)
76-79: Function signature updated for consistencyThe
HmacTraits::DeriveBitsmethod signature has been updated to include aCryptoJobMode modeparameter, aligning with the standardized interface being implemented across all cryptographic trait classes.src/crypto/crypto_hash.h (1)
73-76: Function signature updated to support job mode contextThe method signature has been updated to include
CryptoJobMode modeas a parameter, which is part of a systematic enhancement across the cryptographic subsystem to support job mode awareness in bit derivation operations.src/crypto/crypto_scrypt.h (1)
60-63: Updated function signature with newCryptoJobModeparameterThe
DeriveBitsdeclaration has been updated to include aCryptoJobMode modeparameter, consistent with similar interface changes across the crypto module.src/crypto/crypto_hkdf.h (1)
45-48: Updated function signature with newCryptoJobModeparameterThe
DeriveBitsdeclaration has been updated to include aCryptoJobMode modeparameter, consistent with similar interface changes across the crypto module.src/crypto/crypto_scrypt.cc (1)
116-119:Details
✅ Verification successful
Updated function signature with new
CryptoJobModeparameterThe implementation of
ScryptTraits::DeriveBitshas been updated to include theCryptoJobMode modeparameter to match the updated declaration in the header file. While the parameter isn't currently used in the function body, this change maintains consistency with the standardized interface across the crypto module.
🏁 Script executed:
#!/bin/bash # Check if the util.h file has been updated to pass this parameter # Look for DeriveBitsJob::DoThreadPoolWork implementation rg --no-filename "DoThreadPoolWork.*DeriveBits" src/crypto/crypto_util.hLength of output: 74
Let’s locate all calls to
DeriveBitsin the crypto utility layer to confirm whether the newmodeparameter is being propagated:
🏁 Script executed:
#!/bin/bash # Search for DeriveBits usage in crypto_util.h and crypto_util.cc echo "== Occurrences in crypto_util.h ==" rg -n "DeriveBits" src/crypto/crypto_util.h || echo " (none found)" echo echo "== Occurrences in crypto_util.cc ==" rg -n "DeriveBits" src/crypto/crypto_util.cc || echo " (none found)"Length of output: 1453
Let’s inspect the actual invocation of
DeriveBitsinside theDeriveBitsJobto confirm the newmodeis forwarded:
🏁 Script executed:
#!/bin/bash # Extract the DeriveBits invocation in DeriveBitsJob’s DoThreadPoolWork rg -n -C3 "DeriveBitsTraits::DeriveBits" src/crypto/crypto_util.hLength of output: 420
Verified mode parameter is correctly forwarded
The additional
CryptoJobMode modeinScryptTraits::DeriveBitsis indeed passed through from the job invocation:
- In
src/crypto/crypto_util.h(lines 468–474),DeriveBitsTraits::DeriveBits(..., this->mode())confirms the new parameter is propagated.No further changes are needed.
src/crypto/crypto_sig.cc (2)
706-710: Implementation of error handling based on job mode looks good.The addition of the
CryptoJobMode modeparameter and thecan_throwvariable to conditionally handle errors is well-designed. This prevents exceptions from being thrown during asynchronous operations while maintaining the behavior for synchronous operations.
720-721: Consistent conditional error handling throughout the function.The conditional error handling is consistently applied throughout the function where errors might occur, making sure exceptions are only thrown in synchronous mode. This is a good practice for asynchronous code.
Also applies to: 727-728, 741-742, 756-757, 776-778, 783-785
test/parallel/test-crypto-async-sign-verify.js (3)
6-6: Good addition of hasOpenSSL3 for conditional testing.This addition allows the test to handle differences in error messages between OpenSSL versions.
146-160: Well-structured test for undefined digest error handling.This test properly verifies that the crypto.verify function handles errors correctly in async mode when given an undefined digest. The conditional error message based on OpenSSL version is a good approach.
162-170: Good test coverage for digest too big error case.This test ensures that the crypto.sign function correctly handles the error case when the digest is too large for the RSA key size in async mode.
src/crypto/crypto_random.cc (3)
67-70: API signature update for consistency.The addition of the
CryptoJobMode modeparameter to the RandomBytesTraits::DeriveBits method signature aligns with similar changes across the crypto module.
155-158: API signature update for consistency.The RandomPrimeTraits::DeriveBits method signature has been updated to include the CryptoJobMode parameter, maintaining consistency with other crypto module methods.
194-197: API signature update for consistency.The CheckPrimeTraits::DeriveBits method signature has been updated to include the CryptoJobMode parameter, maintaining consistency with other crypto module methods.
src/crypto/crypto_random.h (3)
35-38: Header signature update for consistency.The RandomBytesTraits::DeriveBits method declaration has been updated to include the CryptoJobMode parameter, properly matching the implementation.
70-73: Header signature update for consistency.The RandomPrimeTraits::DeriveBits method declaration has been updated to include the CryptoJobMode parameter, properly matching the implementation.
104-107: Header signature update for consistency.The CheckPrimeTraits::DeriveBits method declaration has been updated to include the CryptoJobMode parameter, properly matching the implementation.
node.gyp (1)
417-524: Continuous profiler files removed from buildThis change removes the continuous profiler source files (
src/nsolid/continuous_profiler.ccandsrc/nsolid/continuous_profiler.h) from the build, which aligns with the broader removal of continuous CPU profiling support throughout the codebase.agents/grpc/proto/asset.proto (1)
8-15: Timestamp fields removed from Asset messageThe
start_tsandend_tsfields have been removed from the Asset protobuf message, consistent with the removal of continuous CPU profiling functionality. This properly maintains backward compatibility by keeping field numbers intact for remaining fields.agents/grpc/src/proto/nsolid_service.pb.cc (2)
65-86: ExportContinuousProfile RPC method removedThe
ExportContinuousProfileRPC method has been removed from theNSolidServiceservice definition, aligning with the overall removal of continuous CPU profiling support. This change is correctly reflected in the protobuf descriptor.
102-102: Descriptor table size updatedThe descriptor table size has been properly reduced to reflect the removal of the
ExportContinuousProfileRPC method.agents/grpc/src/asset_stream.cc (2)
18-33: AssetStream constructor simplifiedThe constructor has been simplified by:
- Removing the
rpc_typeparameter- Removing conditional logic that selected between
ExportContinuousProfileandExportAsset- Now always using
ExportAssetThis change is consistent with the removal of continuous CPU profiling support throughout the codebase.
87-91: Assertion removed from WritesDone methodAn assertion that checked
write_state_.write_done_calledhas been removed from theWritesDonemethod. The state is still being set to true and used inNextWrite(), so functionality should remain intact.agents/grpc/src/proto/asset.pb.cc (2)
70-76: Removal ofstart_ts/end_tsfields – confirm wire-compatibility expectations
Assetno longer carriesstart_tsandend_ts. As long as the field numbers (tags) of the removed fields are not re-used for different purposes this is wire-compatible – unknown fields sent by older agents will be silently ignored.Please double-check:
- The removed fields’ tag numbers (presumably 5 & 6) are now unused and won’t be re-assigned in the
.proto.- Any downstream consumers (DB schemas, analytics jobs) that relied on the timestamps have been updated.
No code change needed if the above is already guaranteed.
84-85: Descriptor size hard-coded – regenerate other language stubsThe descriptor table size changed (now
241bytes). Ensure all language bindings (JS/TS, Python, Go, etc.) are regenerated from the updated.protoso every component shares the same schema and descriptor metadata.src/nsolid/nsolid_api.cc (1)
1144-1144: Code looks good after continuous profiling feature removal.This line remains unchanged but is part of the configuration update logic that previously handled continuous CPU profiling configuration. The surrounding code has been modified to remove the continuous profiling feature, leaving this comparison logic intact to detect other configuration changes.
agents/grpc/src/proto/reconfigure.pb.cc (8)
38-38: RemovedcontCpuProfilefield from constructor initialization list.This change correctly removes the initialization for the
contCpuProfileboolean field that was previously part of theReconfigureBodymessage, keeping only thetracingmodulesblacklist_initialization. This change aligns with the overall removal of the continuous CPU profiling functionality.
106-107: Updated schema layout after field removal.The migration schema sizes were adjusted after removing the
contCpuProfilefield from theReconfigureBodymessage. This is automatically generated code that reflects the updated structure of the protobuf message.
117-132: Updated protobuf definition string after removing continuous profiling field.The embedded protobuf definition string has been updated to remove the
contCpuProfilefield from theReconfigureBodymessage. This string represents the canonical definition of the protobuf structure used for runtime reflection.
139-139: Updated descriptor table size after field removal.The descriptor table size has been reduced from a larger value to 661 bytes, which is consistent with removing the
contCpuProfilefield and its associated metadata from theReconfigureBodymessage.
241-242: Updated field memory layout calculations after field removal.These lines calculate memory boundaries for field copying operations. The adjustment reflects the removal of the
contCpuProfilefield, ensuring that memory operations work correctly with the updated struct layout.
324-328: Updated bit manipulation for field clearing.This section handles clearing of field bits when the
Clear()method is called. The logic has been updated to account for the removal of thecontCpuProfilefield's presence bit, maintaining proper bit manipulation with the remaining fields.
692-700: Updated merge logic after field removal.This section of code handles merging two
ReconfigureBodymessages. The conditional logic for handling the bits has been updated to account for the removal of thecontCpuProfilefield while maintaining proper handling of the remaining fields.
734-740: Updated memory swapping boundaries after field removal.These lines define the memory boundaries used during the swap operation between two
ReconfigureBodyinstances. The calculation has been updated to reflect the removal of thecontCpuProfilefield, ensuring that memory swapping works correctly with the new struct layout.agents/grpc/src/proto/nsolid_service.grpc.pb.cc (1)
45-56: Generated file – no manual review required
nsolid_service.grpc.pb.ccis generated by the gRPC plugin. Assuming the generation script is run in CI, committing these diffs is fine. Otherwise consider adding the generated artefacts to.gitignoreand regenerating them as part of the build to avoid accidental divergence.agents/grpc/src/grpc_agent.cc (2)
1398-1403: Duration unit may no longer match server expectations
uv_now()returns milliseconds (since the lastuv_update_time()call) whereas the removedstart_ts / end_tsfields were nanoseconds. Make sure the backend now expects ms; otherwise convert:asset.set_duration((uv_now(&loop_) - prof_stor.timestamp) * 1'000'000); // nsAlso ensure
uv_update_time(&loop_)has been called on the same loop before eachuv_now()to avoid stale timestamps.
1961-1966: ConfirmAssetStream’s memory ownership
new AssetStream(...)is stored only insideprof_stor.stream.
EnsureAssetStreamself-deletes onFinish()or is deleted in every early-exit path; otherwise the change in constructor signature might introduce a leak.
| bool HashTraits::DeriveBits(Environment* env, | ||
| const HashConfig& params, | ||
| ByteSource* out, | ||
| CryptoJobMode mode) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Function signature updated with new parameter
The DeriveBits method signature has been updated to include a CryptoJobMode mode parameter, but the implementation doesn't currently use this parameter. This change aligns with a broader refactoring across cryptographic trait classes.
🏁 Script executed:
#!/bin/bash
# Check if other DeriveBits implementations use the CryptoJobMode parameter
echo "Checking DeriveBits implementations that use the mode parameter:"
rg -A 5 "DeriveBits.*CryptoJobMode mode\)\s*\{[^}]*mode" --type cpp src/crypto/Length of output: 220
I’d like to confirm that no other DeriveBits variants are using the new mode parameter. Let’s list every declaration and definition across the crypto modules:
🏁 Script executed:
#!/bin/bash
echo "Listing all DeriveBits occurrences (declarations & definitions) in src/crypto/:"
rg -n "DeriveBits" -C 2 --type cpp --type h src/crypto/Length of output: 14110
Let’s confirm the exact signature in crypto_hash.cc around the reported lines:
🏁 Script executed:
#!/bin/bash
echo "Inspecting HashTraits::DeriveBits signature and parameters in crypto_hash.cc:"
# Show signature and next few lines to see if CryptoJobMode is present
rg -n "HashTraits::DeriveBits" -A5 -B2 src/crypto/crypto_hash.cc
# Dump a broader context if needed
sed -n '495,515p' src/crypto/crypto_hash.ccLength of output: 1205
HashTraits::DeriveBits has an unused mode parameter
The newly added CryptoJobMode mode parameter in HashTraits::DeriveBits isn’t referenced, which will trigger an unused‐parameter warning. Suppress it by adding (void)mode;, marking it [[maybe_unused]], or incorporating mode‐specific behavior.
Also, to match the generic DeriveBitsJob API, ensure all other trait implementations adopt the four-argument signature:
• src/crypto/crypto_hash.cc: annotate or use the mode parameter in HashTraits::DeriveBits.
• src/crypto/*Traits::{DeriveBits} (e.g. ScryptTraits, PBKDF2Traits, HmacTraits, HkdfTraits, DHBitsTraits, ECDHBitsTraits, SignTraits, RandomBytesTraits, RandomPrimeTraits, CheckPrimeTraits): update both declarations and definitions to include CryptoJobMode mode.
🤖 Prompt for AI Agents
In src/crypto/crypto_hash.cc around lines 505 to 508, the DeriveBits method has
a new CryptoJobMode mode parameter that is currently unused, causing an
unused-parameter warning. Fix this by explicitly marking the parameter as unused
with (void)mode; or [[maybe_unused]] in the function body. Additionally, ensure
that all other trait classes in src/crypto/*Traits update their DeriveBits
declarations and definitions to include the CryptoJobMode mode parameter to
maintain consistent four-argument signatures.
| bool HmacTraits::DeriveBits(Environment* env, | ||
| const HmacConfig& params, | ||
| ByteSource* out, | ||
| CryptoJobMode mode) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Updated function signature with new CryptoJobMode parameter
The function signature has been updated to include a CryptoJobMode mode parameter. This change is consistent with similar signature updates across multiple cryptographic trait classes in the codebase to standardize interfaces.
🏁 Script executed:
#!/bin/bash
# Verify that all callers of DeriveBits have been updated to pass the new parameter
# Find all places where the DeriveBits functions are called
rg --no-filename "DeriveBits\s*\(\s*.*?,\s*.*?,\s*.*?(,\s*.*?)?\s*\)" src/crypto/Length of output: 83
To ensure we catch every call site, let’s broaden the search for any invocation of DeriveBits(...):
This will list all call sites so you can verify that each now passes the new mode argument.
🏁 Script executed:
#!/bin/bash
# Broad search for any occurrence of DeriveBits(…) in the crypto directory
rg -n 'DeriveBits\(' src/crypto/Length of output: 1777
Ensure all DeriveBits calls include the new mode parameter
The signature for DeriveBits was extended to:
bool DeriveBits(Environment* env,
const XConfig& params,
ByteSource* out,
CryptoJobMode mode);However, there are still call sites invoking it with only three arguments. In particular:
- src/crypto/crypto_util.h:471
if (!DeriveBitsTraits::DeriveBits(AsyncWrap::env(), params, out)) {
Please update this (and any similar invocations) to pass the appropriate CryptoJobMode. For example, if using the enclosing function’s mode parameter:
- if (!DeriveBitsTraits::DeriveBits(AsyncWrap::env(), params, out)) {
+ if (!DeriveBitsTraits::DeriveBits(AsyncWrap::env(), params, out, mode)) {—or, if this is a synchronous helper—use a default:
- if (!DeriveBitsTraits::DeriveBits(AsyncWrap::env(), params, out)) {
+ if (!DeriveBitsTraits::DeriveBits(AsyncWrap::env(), params, out, CryptoJobMode::kSync)) {Please audit all DeriveBits(...) calls across the src/crypto/ directory and ensure each now includes the fourth CryptoJobMode argument.
🤖 Prompt for AI Agents
In src/crypto/crypto_hmac.cc around lines 224 to 227, the DeriveBits function
signature was updated to include a new CryptoJobMode mode parameter, but some
call sites still invoke it with only three arguments. You need to find all calls
to DeriveBits in the src/crypto/ directory and update them to pass the
appropriate fourth argument for mode. Use the enclosing function's mode
parameter if available, or a suitable default if not. Ensure every call to
DeriveBits now includes this fourth CryptoJobMode argument to match the updated
signature.
| 'src/nsolid/nsolid_heap_snapshot.cc', | ||
| 'src/nsolid.h', | ||
| 'src/nsolid/continuous_profiler.h', | ||
| 'src/nsolid.h' |
There was a problem hiding this comment.
Fix syntax error: missing comma after 'src/nsolid.h'
There's a missing comma after 'src/nsolid.h' which will cause a syntax error in the build configuration. This needs to be fixed for the build to work properly.
- 'src/nsolid.h'
+ 'src/nsolid.h',📝 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.
| 'src/nsolid.h' | |
| 'src/nsolid.h', |
🤖 Prompt for AI Agents
In node.gyp at line 519, there is a missing comma after the entry 'src/nsolid.h'
which causes a syntax error in the build configuration. Add a comma immediately
after 'src/nsolid.h' to correctly separate list items and fix the syntax error.
| ProfileStor stor{ req.requestid(), | ||
| uv_now(&loop_), | ||
| nullptr, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Timestamp captured from a non-loop thread can be stale
uv_now(&loop_) is evaluated while pushing the ProfileStor from any thread (JS or agent).
uv_now() is only updated by uv_run() or an explicit uv_update_time(&loop_).
If this call happens outside the agent’s event-loop thread the stored timestamp might lag behind, producing negative or wildly large durations later.
Suggestion:
- uv_now(&loop_),
+ /* timestamp populated in agent thread to guarantee fresh clock */
+ 0, // placeholder – to be filled in do_start_prof_end()and immediately before assigning prof_stor.stream in do_start_prof_end():
prof_stor.timestamp = uv_now(&loop_);This guarantees the call runs on the agent’s loop thread after a recent uv_update_time().
🏁 Script executed:
#!/bin/bash
# Locate the definition of do_start_prof_end and inspect ProfileStor usage context
rg -n "do_start_prof_end" -C3 agents/grpc/src/grpc_agent.cc
rg -n "ProfileStor" -C3 agents/grpc/src/grpc_agent.cc
sed -n '1900,1960p' agents/grpc/src/grpc_agent.ccLength of output: 5898
🏁 Script executed:
#!/bin/bash
# Locate definition of StartProfStor to understand fields and usage
rg -n "StartProfStor" -C3 agents/grpc/src/grpc_agent.ccLength of output: 259
🏁 Script executed:
#!/bin/bash
# Search for usage of timestamp in grpc_agent.cc to see where it's used
rg -n "timestamp" -C3 agents/grpc/src/grpc_agent.ccLength of output: 358
Ensure timestamps are captured on the agent’s loop thread to avoid stale values
Capturing uv_now(&loop_) in do_start_prof_init may run off the loop thread—uv_now() is only refreshed by uv_run() or an explicit uv_update_time(). This can lead to negative or unexpectedly large profile durations. Defer timestamping to the loop thread in do_start_prof_end().
Locations to update:
do_start_prof_initinitializer (agents/grpc/src/grpc_agent.cc:1932–1934)- Duration computation in
do_start_prof_end(agents/grpc/src/grpc_agent.cc:1402)
Suggested diff:
--- a/agents/grpc/src/grpc_agent.cc
+++ b/agents/grpc/src/grpc_agent.cc
@@ -1932,7 +1932,8 @@ ErrorType GrpcAgent::do_start_prof_init(const grpcagent::CommandRequest& req,
nsuv::ns_mutex::scoped_lock lock(profile_state_lock_);
ProfileState& profile_state = profile_state_[type];
- ProfileStor stor{ req.requestid(),
- uv_now(&loop_),
+ // timestamp will be set later on the loop thread to guarantee freshness
+ ProfileStor stor{ req.requestid(),
0,
nullptr,
options };
@@ -1400,7 +1400,10 @@ ErrorType GrpcAgent::do_start_prof_end(ErrorType err,
asset.set_thread_id(thread_id);
asset.mutable_metadata()->CopyFrom(metadata);
asset.set_complete(true);
- asset.set_duration(uv_now(&loop_) - prof_stor.timestamp);
+ // capture a fresh timestamp now that we’re on the loop thread
+ prof_stor.timestamp = uv_now(&loop_);
+ asset.set_duration(uv_now(&loop_) - prof_stor.timestamp);
prof_stor.stream->Write(std::move(asset));
prof_stor.stream->WritesDone();This change ensures uv_now() is invoked immediately after an uv_update_time() in the loop, preventing stale timestamps.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In agents/grpc/src/grpc_agent.cc around lines 1932 to 1934, the timestamp is
captured using uv_now(&loop_) potentially off the agent's event-loop thread,
which can cause stale or incorrect timing data. To fix this, remove the
uv_now(&loop_) call from the ProfileStor initialization in do_start_prof_init
and instead assign the timestamp inside do_start_prof_end on the loop thread
immediately after uv_update_time(&loop_) is called. This ensures the timestamp
is fresh and accurate, preventing negative or large duration calculations.
santigimeno
force-pushed
the
node-v22.15.1-nsolid-v5.7.2-release
branch
from
165b0a0 to
2065869
Compare
5 participants
Summary by CodeRabbit
New Features
fs.readFileSyncdoes not leak memory when reading UTF-8 files.Bug Fixes
Removals
Documentation