Node v22.21.1 nsolid v6.1.0 release#393
Conversation
Add comprehensive runtime control for asset collection (CPU/heap
profiling, snapshots) with multiple configuration methods.
Implements assetsEnabled flag with full control surface:
- NSOLID_ASSETS_ENABLED environment variable
- nsolid.start({ assetsEnabled: true/false }) configuration
- Runtime helpers: enableAssets() and disableAssets()
- Dynamic updates via gRPC reconfigure protocol
- Validation in both gRPC and ZMQ agents with proper errors
- Continuous profiler integration respects assetsEnabled state
This allows operators to disable expensive profiling operations
at runtime without process restarts, improving resource management
and production safety.
Also adds missing enableTraces()/disableTraces() helper methods
to complete the tracing API surface.
Comprehensive test coverage added for all asset types across gRPC,
ZMQ, and parallel test suites.
PR-URL: #377
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
WalkthroughThis pull request introduces a comprehensive restructuring of the project to establish N|Solid branding, adding Windows build automation via DSC configurations, a new gRPC-based agent subsystem with protobuf messaging, and extensive updates to CI/CD workflows, documentation, and build system configuration to reflect the N|Solid namespace and terminology. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (8)
.github/workflows/major-release.yml-39-39 (1)
39-39: Remove extra leading whitespace on thegh issue createline.Line 39 has an extra space at the start, causing indentation inconsistency within the script block.
Apply this diff:
- gh issue create --repo "${GITHUB_REPOSITORY}" \ + gh issue create --repo "${GITHUB_REPOSITORY}" \agents/.eslintrc.yaml-1-258 (1)
1-258: Fix non-existent module references in no-restricted-globals rules.The configuration contains restrictions on two globals with references to modules that don't exist in this codebase:
- Line 64-65 (FormData): References
internal/deps/undici/undici— this path does not exist- Line 174-175 (structuredClone): References
internal/structured_clone— this path does not existWhile these specific globals are not used in the agents code, the error messages will point developers to non-existent modules. Either update the restrictions to reference correct internal paths (if these modules are available under different names) or remove them if agents should not enforce these restrictions.
.github/ISSUE_TEMPLATE/config.yml-6-11 (1)
6-11: Fix the typo: "seperate" should be "separate".The word "seperate" is misspelled in both the npm and undici contact link descriptions (lines 8 and 11).
Apply this diff to fix the typos:
- name: 📦 Have an issue with npm? url: https://github.com/npm/cli/issues - about: npm has a seperate issue tracker. + about: npm has a separate issue tracker. - name: 📡 Have an issue with undici? (`WebSocket`, `fetch`, etc.) url: https://github.com/nodejs/undici/issues - about: Undici has a seperate issue tracker. + about: Undici has a separate issue tracker..github/label-pr-config.yml-131-132 (1)
131-132: Fix regex pattern inconsistency in test/module-hooks labeling.Line 132 contains a regex error:
/^test\/fixtures/module-hooks\//appears to have incorrect path structure. The pattern should likely be/^test\/module-hooks\//(without "fixtures" in the middle), consistent with line 131's intent.- /^test\/fixtures/module-hooks\//: test, module, loaders + /^test\/module-hooks\//: test, module, loadersBUILDING.md-887-887 (1)
887-887: Fix grammar: "using a ABI" should be "using an ABI".An article error on Line 887 in the licensing discussion.
Apply this diff:
-incompatible with the official N|Solid builds (e.g. using a ABI incompatible +incompatible with the official N|Solid builds (e.g. using an ABI incompatible.github/workflows/lint-release-proposal.yml-34-34 (1)
34-34: Fix debug artifact in lint script.Line 34 prints
"Actual: $ACTUAL"but the variable is namedCOMMIT_SUBJECT. Either use the correct variable name or remove this debug line if it's not needed.- echo "Actual: $ACTUAL" + echo "Actual: $COMMIT_SUBJECT"agents/grpc/src/grpc_errors.h-19-22 (1)
19-22: Add missing standard library includes.
ErrorStorusesstd::stringanduint32_tbut the header doesn't include<string>or<cstdint>. This may compile if other headers happen to include them first, but it's fragile.#ifndef AGENTS_GRPC_SRC_GRPC_ERRORS_H_ #define AGENTS_GRPC_SRC_GRPC_ERRORS_H_ +#include <cstdint> +#include <string> + #define GRPC_ERRORS(X) \Committable suggestion skipped: line range outside the PR's diff.
agents/grpc/src/grpc_agent.cc-72-73 (1)
72-73: Fix misleading GRPC_MAX_SIZE comment (value is 4 MiB, not 4 GiB)
GRPC_MAX_SIZEis4L * 1024 * 1024(4 MiB), but the comment says// 4GB. This is confusing when reasoning about chunking ingot_profile/got_continuous_profile. Update the comment (or the value, if 4 GiB is actually intended) so they match.
🧹 Nitpick comments (30)
.github/workflows/daily.yml (1)
16-16: Consider the trade-off of pinning to ubuntu-24.04.Switching from
ubuntu-latesttoubuntu-24.04provides version stability but requires manual updates when newer Ubuntu versions are released. Ensure this aligns with your maintenance strategy.CHANGELOG_NSOLID.md (1)
7-7: Fix compound adjective formatting.Use a hyphen in "Long-term Support" to properly form the compound adjective.
-* [N|Solid 5 | Node.js 20](doc/changelogs/NSOLID_CHANGELOG_V5_NODE_V20.md) **Long Term Support** +* [N|Solid 5 | Node.js 20](doc/changelogs/NSOLID_CHANGELOG_V5_NODE_V20.md) **Long-term Support**.configurations/configuration.vsProfessional.dsc.yaml (1)
1-54: VS Professional DSC configuration looks consistent;moduledirective nitThe resource graph (Python → VS Professional → VSComponents plus Git/NASM) is well-structured and
vsComponentscorrectly depends onvsPackagewith the expected product/channel IDs. As a tiny consistency nit, you might either drop themodule: Microsoft.WinGet.DSCdirective frompythonPackageor add it to the otherWinGetPackageresources so they all follow the same pattern, but it’s not functionally required..github/workflows/notify-on-review-wanted.yml (1)
1-44: Review-wanted Slack notifications are correctly scoped; PR/issue detection could be made clearerThe workflow is narrowly scoped (only
nodejs/nodeand thereview wantedlabel), usespull_request_targetsafely for metadata-only access, and the Slack step is properly pinned and parameterized from thedefine-messageoutputs. As a minor clarity improvement, you could skip referencinggithub.event.pull_request.numberinside the shell and instead branch on something like anIS_PRenv (${{ github.event_name == 'pull_request_target' }}) or a boolean output, but the current logic should still behave correctly..github/workflows/linters.yml (1)
70-70: Consider removing hardcoded && false condition; use explicit conditional or removal.The "Get release version numbers" step uses
&& falseto disable functionality. While functional, this pattern is a code smell. Either define a proper conditional (e.g.,if: ${{ false }}as a comment) or remove the step entirely if it's no longer needed for N|Solid.As noted in static analysis, this pattern should be replaced with explicit intent:
- if: ${{ github.event.pull_request && github.event.pull_request.base.ref == github.event.pull_request.base.repo.default_branch && false }} + if: false # Disabled: fetch released N|Solid versions when needed.github/workflows/coverage-linux.yml (1)
50-52: Job intentionally disabled withif: false.The comment on line 50 explains this is temporary due to "Report JS" step crashing. While the static analysis correctly flags that this condition is always false, this appears intentional.
Consider adding a tracking issue reference to make it easier to follow up on re-enabling this workflow.
- # Disabled because "Report JS" step was crashing. + # Disabled because "Report JS" step was crashing. See: <issue-link> if: falseCONTRIBUTING.md (1)
143-147: Clarifyncu-configsetup for non-Node.js contributors.The git node land command (Line 146) assumes familiarity with
node-core-utilstooling originally designed for Node.js core. For N|Solid-specific contributors, briefly clarify what this tool does or link to its N|Solid-specific documentation..github/workflows/create-release-proposal.yml (2)
1-3: Add secret validation or documentation.The workflow depends on
GH_USER_TOKENsecret (comment line 2, used line 81). Ensure this secret is documented in repository security/configuration documentation or add a helpful error message if the secret is missing.
74-77: Document the create-release-proposal.sh script.The workflow curls and pipes a shell script from GitHub (line 76-77). Ensure the
tools/actions/create-release-proposal.shscript is documented in CONTRIBUTING.md or a release guide for maintainers who may need to debug or customize the release process..github/workflows/lint-release-proposal.yml (1)
69-75: Verify lint-release-proposal-commit-list.mjs exists and is committed.The workflow pipes output to
tools/actions/lint-release-proposal-commit-list.mjs(line 69). Ensure this script:
- Is committed to the repository
- Exists at the correct path
- Is executable and handles the piped JSON input correctly
.github/workflows/build-tarball.yml (2)
74-75: Document sparse-checkout usage for clang installation.The test job uses sparse-checkout to selectively download only
.github/actions/install-clang(lines 74-75). Document why this optimization is needed or consider removing it if the performance gain is minimal, as it adds complexity.
93-94: Verify tarball extraction TAR_DIR inference is robust.Line 94 infers the directory name from the tarball glob:
TAR_DIR=$RUNNER_TEMP/$(basename tarballs/*.tar.gz .tar.gz). This assumes:
- Exactly one tarball is in
tarballs/- Tarball is named consistently (format:
<name>.tar.gz)If tarballs change format or multiple tarballs exist, extraction will fail. Consider adding explicit error handling.
# More robust: verify exactly one tarball exists TARBALL=$(ls tarballs/*.tar.gz | wc -l) if [ "$TARBALL" -ne 1 ]; then echo "Error: Expected exactly 1 tarball, found $TARBALL" exit 1 fi TAR_DIR=$RUNNER_TEMP/$(basename tarballs/*.tar.gz .tar.gz)agents/grpc/proto/BUILD (1)
1-8: Make the proto generation command portable and self‑describing.Hard‑coding
~/software/protobuf/src/protocmeans thisBUILDfile will only work on your local setup. Consider using a relative path or relying onprotocfrom$PATH, and (if this is meant as a helper script) add a shebang/comment or rename it so it’s clear it isn’t a Bazel BUILD file.agents/grpc/lib/nsolid.js (1)
1-6: Thin gRPC agent entrypoint looks correct; optional explicitinternalBindingimport.This is a clean wrapper around the
nsolid_grpc_agentbinding and the internal agent factory. If there’s any chance this module might be loaded outside the core/builtin loader context, you could explicitly importinternalBinding(e.g., viainternal/bootstrap/loaders) to make the dependency clearer, but otherwise this looks good.agents/grpc/proto/command.proto (1)
1-5: Consider removing or updating the design comment.This comment appears to be a leftover design note that doesn't reflect the current implementation. Either remove it or expand it into proper documentation explaining the
CommandArgsoneof pattern.-// I want to define here RuntimeResponse message which will be oneOf many other types of messages -// For example: -// - InfoResponse -// - PackagesResponse - syntax = "proto3";agents/grpc/lib/agent.js (1)
3-27: Composable factory looks good; optional require placement tweakThe composition with
internal/nsolid_assetsplusstart/status/stopis straightforward and clear. If you want to squeeze a tiny bit more clarity and avoid re-evaluating therequireexpression on the first call, you could move therequire('internal/nsolid_assets')to module scope and reuse it in the factory, but behavior is already correct andrequireis cached anyway.agents/grpc/src/grpc_utils.h (1)
4-50: Avoid leaking protobuf symbols via globalusingin a headerPlacing
using google::protobuf::Message; using google::protobuf::util::MessageToJsonString;at global scope in a public header can pollute consumers with a very generic
Messageidentifier and potentially cause name clashes. It’s safer to either:
- fully qualify these types/functions at use sites, or
- move the
usingdeclarations insidenamespace node::nsolid::grpc(or a detail namespace) so their scope is limited.The debug helpers themselves look fine for a debug-only path; truncated prefixes from the 512-byte buffer in
DebugProtobufMsgare acceptable if you don’t need full messages in logs.agents/grpc/proto/info.proto (1)
7-29: Double‑checkkernelVersionasuint32Most OS kernel versions are dotted strings (e.g.,
5.10.0-xyz), so modelingkernelVersionasuint32could be limiting if you ever need the full version string or semver-like comparison. If you only care about a single numeric component it’s fine; otherwise, consider making this astring kernelVersion = 16;while the wire format is still new.agents/grpc/proto/packages.proto (1)
1-28: Packages schema is clear; optional comment cleanupThe
PackagesEvent/PackagesBody/Packagehierarchy is straightforward and matches the rest of the event schema style. The introductory comment about a futureRuntimeResponseoneof is fine as a TODO, but if that design isn’t planned anymore, you might want to trim it to avoid confusion for future readers.agents/grpc/proto/profile.proto (1)
1-35: Align Buf package‑directory rule withgrpcagentprotosBuf is flagging that files with
package grpcagent;should live under agrpcagent/directory relative to the module root, but this file sits inagents/grpc/proto/. If Buf linting is part of CI, you’ll likely need to either:
- point the Buf module root at
agents/grpc/proto(sogrpcagentmatches that tree), or- move these protos under a
grpcagent/subdirectory and adjust imports accordingly.The
ProfileArgs+ oneof structure itself looks good and gives you a flexible envelope for the various profile types.agents/grpc/src/grpc_client.cc (2)
1-4: Unused include detected.The
debug_utils-inl.hheader is included but no debug utilities appear to be used in this file. Consider removing it to reduce compilation dependencies.#include "grpc_client.h" -#include "debug_utils-inl.h" #include "opentelemetry/exporters/otlp/otlp_grpc_client_options.h"
41-42: Minor: Extra blank line.There's a redundant blank line here that could be removed for consistency.
} - SslCredentialsOptions ssl_opts;agents/grpc/src/asset_stream.cc (1)
75-107: Stream statistics are only updated when debug logging is enabled.The
write_countandtotal_bytesstatistics at lines 93-94 are only updated inside the debug-enabled block. If these stats are needed for non-debug purposes (e.g., metrics, monitoring), they should be tracked unconditionally.If stats are needed regardless of debug mode:
if (!ok) { write_state_.done = true; if (!write_state_.writes_done) { write_state_.writes_done = true; StartWritesDone(); RemoveHold(); } } else { + // Update stream statistics unconditionally + stream_stats_.write_count++; + stream_stats_.total_bytes += write_state_.asset.data().length(); NextWrite(); }agents/grpc/src/asset_stream.h (2)
7-7: Consider using a more robust include path.The relative path
../../src/profile_collector.his fragile and depends on the specific directory structure. Consider using an include path relative to a well-known root or configuring include directories in the build system.
71-74: Duplicateprivate:access specifier.There are two consecutive
private:labels at lines 71 and 74. The second one is redundant.private: void NextWrite(); - private: std::weak_ptr<AssetStreamObserver> observer_;agents/grpc/src/grpc_client.h (1)
44-46: Uninitializedstartmember could cause issues in debug logging.The
startfield is uninitialized inGrpcAsyncCallData. WhenNSOLID_GRPC_AGENTdebug is not enabled at call time but becomes enabled later in the callback, the conditioncall_data->start > 0(line 129) may evaluate unpredictably.EventType* event = nullptr; grpcagent::EventResponse* event_response = nullptr; - uint64_t start; + uint64_t start = 0;agents/grpc/src/grpc_agent.h (1)
108-111: Thread-safe but consider simplifying thesaas()accessor.The static local empty string is thread-safe (C++11 magic statics), but returning a reference to it when
saas_is null could be cleaner. Consider returningstd::string_viewor simply returning by value for a small string.This is a minor style preference; the current implementation is correct and safe.
agents/grpc/src/grpc_agent.cc (3)
916-937: Ensure metrics exporters are always initialized before metrics callbacks fire
metrics_msg_cb_unconditionally buildsResourceMetricsand calls:auto result = agent->metrics_exporter_->Export(data);Similarly,
metrics_timer_cb_always callsagent->got_proc_metrics(), which usesmetrics_exporter_as well. This assumes that:
config()has already processed a/grpcchange and initializedmetrics_exporter_, and- No metrics timer/messages can fire after
do_stop()has resetmetrics_exporter_.If either invariant can be broken (e.g., a config without
/grpc, or a stray timer firing after partial shutdown), this would dereference a null exporter.At minimum, consider a null check:
if (!agent->metrics_exporter_) { return; }near these call sites, or assert at configuration time that metrics cannot be enabled without a configured exporter.
1310-1315: Guard trace exporter usage ingot_spans
got_spansassumestrace_exporter_is non‑null:auto result = trace_exporter_->Export(const_cast<UniqRecordables&>(recordables));Given
update_tracer()resetsspan_collector_and will only (re)create it whentrace_flags_is non‑zero andtrace_exporter_exists, this is probably safe today. Still, to make the coupling explicit and future‑proof against configuration changes, it would be safer to either:
ASSERT(trace_exporter_)at the top ofgot_spans, or- Early‑return if
!trace_exporter_.This avoids accidental null dereferences if the tracing configuration path evolves.
1382-1492: Check size_t/uint64_t printf formats in debug logsSeveral debug statements here use
%ldfor values that aresize_toruint64_t, e.g.:Debug("got_profile. len: %ld, status: %d. thread_id: %ld. req_id: %s\n", stor.profile.length(), stor.status, thread_id, ...); ... Debug("Asset size larger than supported (%ld > %ld): ...\n", asset_size, GRPC_MAX_SIZE);On typical platforms
size_t/uint64_tare unsigned types; passing them to%ldis technically undefined behavior and can misprint or break on other ABIs.When you touch this area, prefer the correct format specifiers (
%zuforsize_t,%llu/%juforuint64_t, or cast explicitly) to keep diagnostics well‑defined across platforms.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
benchmark/fixtures/node_modules/error-stack/simple-error-stack.jsis excluded by!**/node_modules/**benchmark/fixtures/node_modules/error-stack/simple-error-stack.tsis excluded by!**/node_modules/**
📒 Files selected for processing (106)
.configurations/configuration.dsc.yaml(1 hunks).configurations/configuration.vsEnterprise.dsc.yaml(1 hunks).configurations/configuration.vsProfessional.dsc.yaml(1 hunks).cpplint(1 hunks).devcontainer.json(1 hunks).devcontainer/devcontainer.json(0 hunks).editorconfig(1 hunks).eslintignore(0 hunks).eslintrc.js(0 hunks).gitattributes(1 hunks).github/CODEOWNERS(5 hunks).github/ISSUE_TEMPLATE/1-bug-report.yml(2 hunks).github/ISSUE_TEMPLATE/4-report-a-flaky-test.yml(1 hunks).github/ISSUE_TEMPLATE/config.yml(1 hunks).github/SUPPORT.md(1 hunks).github/codeql-config.yml(1 hunks).github/dependabot.yml(1 hunks).github/label-pr-config.yml(7 hunks).github/workflows/auto-start-ci.yml(2 hunks).github/workflows/build-tarball.yml(1 hunks).github/workflows/build-windows.yml(0 hunks).github/workflows/close-stale-feature-requests.yml(1 hunks).github/workflows/close-stalled.yml(1 hunks).github/workflows/codeql.yml(1 hunks).github/workflows/commit-lint.yml(1 hunks).github/workflows/commit-queue.yml(4 hunks).github/workflows/coverage-linux-without-intl.yml(3 hunks).github/workflows/coverage-linux.yml(3 hunks).github/workflows/coverage-windows.yml(3 hunks).github/workflows/create-release-proposal.yml(1 hunks).github/workflows/daily-wpt-fyi.yml(6 hunks).github/workflows/daily.yml(1 hunks).github/workflows/doc.yml(1 hunks).github/workflows/find-inactive-collaborators.yml(2 hunks).github/workflows/find-inactive-tsc.yml(1 hunks).github/workflows/label-flaky-test-issue.yml(1 hunks).github/workflows/license-builder.yml(1 hunks).github/workflows/lint-release-proposal.yml(1 hunks).github/workflows/linters.yml(4 hunks).github/workflows/major-release.yml(1 hunks).github/workflows/notify-on-push.yml(3 hunks).github/workflows/notify-on-review-wanted.yml(1 hunks).github/workflows/scorecard.yml(2 hunks).github/workflows/test-asan.yml(0 hunks).github/workflows/test-internet.yml(3 hunks).github/workflows/test-linux.yml(3 hunks).github/workflows/test-macos.yml(3 hunks).github/workflows/test-ubsan.yml(0 hunks).github/workflows/timezone-update.yml(1 hunks).github/workflows/tools.yml(9 hunks).github/workflows/update-openssl.yml(2 hunks).github/workflows/update-v8.yml(2 hunks).github/workflows/update-wpt.yml(1 hunks).gitignore(4 hunks).mailmap(7 hunks).npmrc(1 hunks)BUILDING.md(24 hunks)CHANGELOG.md(2 hunks)CHANGELOG_NSOLID.md(1 hunks)CODE_OF_CONDUCT.md(1 hunks)CONTRIBUTING.md(2 hunks)GOVERNANCE.md(6 hunks)LICENSE(9 hunks)LICENSE_NSOLID(1 hunks)Makefile(52 hunks)README.md(1 hunks)SECURITY.md(2 hunks)agents/.eslintrc.yaml(1 hunks)agents/grpc/lib/agent.js(1 hunks)agents/grpc/lib/nsolid.js(1 hunks)agents/grpc/proto/BUILD(1 hunks)agents/grpc/proto/asset.proto(1 hunks)agents/grpc/proto/blocked_loop.proto(1 hunks)agents/grpc/proto/command.proto(1 hunks)agents/grpc/proto/common.proto(1 hunks)agents/grpc/proto/exit.proto(1 hunks)agents/grpc/proto/info.proto(1 hunks)agents/grpc/proto/metrics.proto(1 hunks)agents/grpc/proto/nsolid_service.proto(1 hunks)agents/grpc/proto/packages.proto(1 hunks)agents/grpc/proto/profile.proto(1 hunks)agents/grpc/proto/reconfigure.proto(1 hunks)agents/grpc/proto/source_code.proto(1 hunks)agents/grpc/proto/startup_times.proto(1 hunks)agents/grpc/src/asset_stream.cc(1 hunks)agents/grpc/src/asset_stream.h(1 hunks)agents/grpc/src/binding.cc(1 hunks)agents/grpc/src/command_stream.cc(1 hunks)agents/grpc/src/command_stream.h(1 hunks)agents/grpc/src/grpc_agent.cc(1 hunks)agents/grpc/src/grpc_agent.h(1 hunks)agents/grpc/src/grpc_client.cc(1 hunks)agents/grpc/src/grpc_client.h(1 hunks)agents/grpc/src/grpc_errors.h(1 hunks)agents/grpc/src/grpc_utils.h(1 hunks)agents/grpc/src/proto/asset.grpc.pb.cc(1 hunks)agents/grpc/src/proto/asset.grpc.pb.h(1 hunks)agents/grpc/src/proto/asset.pb.cc(1 hunks)agents/grpc/src/proto/asset.pb.h(1 hunks)agents/grpc/src/proto/blocked_loop.grpc.pb.cc(1 hunks)agents/grpc/src/proto/blocked_loop.grpc.pb.h(1 hunks)agents/grpc/src/proto/blocked_loop.pb.cc(1 hunks)agents/grpc/src/proto/blocked_loop.pb.h(1 hunks)agents/grpc/src/proto/command.grpc.pb.cc(1 hunks)agents/grpc/src/proto/command.grpc.pb.h(1 hunks)agents/grpc/src/proto/command.pb.cc(1 hunks)
💤 Files with no reviewable changes (6)
- .github/workflows/test-asan.yml
- .github/workflows/test-ubsan.yml
- .github/workflows/build-windows.yml
- .eslintignore
- .devcontainer/devcontainer.json
- .eslintrc.js
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 0
File: :0-0
Timestamp: 2025-10-27T13:06:17.506Z
Learning: In the nodesource/nsolid repository, when reviewing PRs that add or modify V8 patches (e.g., in deps/v8/patches/), focus review comments on the patch management infrastructure (configure.py, apply-patches.sh, README.md, etc.) and not on the V8 code changes contained within the .patch files themselves, as the patch content is out of scope for the patch management PR.
📚 Learning: 2025-10-14T10:18:23.123Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 377
File: test/parallel/test-nsolid-heap-profile-stream.js:81-88
Timestamp: 2025-10-14T10:18:23.123Z
Learning: In nsolid.heapProfileStream() and nsolid.heapSamplingStream(), synchronous throws occur for argument validation errors and immediate state validation checks (like assets being disabled), while async error events are emitted for runtime errors that manifest during execution (like invalid threadId).
Applied to files:
agents/grpc/lib/agent.js
📚 Learning: 2025-10-27T13:06:17.506Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 0
File: :0-0
Timestamp: 2025-10-27T13:06:17.506Z
Learning: In the nodesource/nsolid repository, when reviewing PRs that add or modify V8 patches (e.g., in deps/v8/patches/), focus review comments on the patch management infrastructure (configure.py, apply-patches.sh, README.md, etc.) and not on the V8 code changes contained within the .patch files themselves, as the patch content is out of scope for the patch management PR.
Applied to files:
CONTRIBUTING.md.github/workflows/commit-queue.yml.github/workflows/update-v8.ymlBUILDING.mdREADME.md.github/CODEOWNERS.github/label-pr-config.yml
📚 Learning: 2025-07-08T14:46:47.806Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:13-14
Timestamp: 2025-07-08T14:46:47.806Z
Learning: In the nsolid test suite, native addon bindings are expected to be built beforehand; tests do not add fs.existsSync guards to skip when bindings are missing.
Applied to files:
BUILDING.mdMakefile
📚 Learning: 2025-07-08T14:47:34.724Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/nsolid-elf-utils.js:16-18
Timestamp: 2025-07-08T14:47:34.724Z
Learning: In the nsolid test suite, external tool invocations (e.g., execSync to run `readelf`) are intentionally left uncaught so that any failure causes the test to fail rather than being skipped.
Applied to files:
BUILDING.md
📚 Learning: 2025-07-08T14:48:04.827Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: test/addons/nsolid-elf-utils/binding.cc:13-28
Timestamp: 2025-07-08T14:48:04.827Z
Learning: In nsolid test native addons (e.g., `test/addons/*/binding.cc`), additional JavaScript-facing argument validation is typically omitted because the tests supply well-formed inputs.
Applied to files:
BUILDING.mdMakefileagents/grpc/src/binding.cc
📚 Learning: 2025-07-08T16:05:45.341Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 339
File: src/nsolid/nsolid_elf_utils.cc:18-18
Timestamp: 2025-07-08T16:05:45.341Z
Learning: In the NSolid project, the `GetBuildId` function in `src/nsolid/nsolid_elf_utils.cc` is designed to be called only from the NSolid thread, so the static cache doesn't require thread safety mechanisms like mutex protection.
Applied to files:
BUILDING.md
📚 Learning: 2025-06-10T09:52:49.401Z
Learnt from: santigimeno
Repo: nodesource/nsolid PR: 327
File: test/addons/nsolid-code-event-hook/binding.cc:64-78
Timestamp: 2025-06-10T09:52:49.401Z
Learning: In the nsolid codebase, test files in test/addons/ consistently use assert() for input validation and runtime checks, not CHECK() macros. This is the established testing convention.
Applied to files:
Makefile
🧬 Code graph analysis (7)
agents/grpc/src/command_stream.h (1)
agents/grpc/src/proto/nsolid_service.grpc.pb.h (3)
NSolidService(31-44)StubInterface(36-41)stub(306-306)
agents/grpc/src/grpc_utils.h (1)
src/debug_utils.h (1)
DebugCategory(70-74)
agents/grpc/src/command_stream.cc (1)
agents/grpc/src/proto/nsolid_service.grpc.pb.h (2)
NSolidService(31-44)stub(306-306)
agents/grpc/src/grpc_client.cc (1)
agents/grpc/src/proto/nsolid_service.grpc.pb.h (1)
NSolidService(31-44)
agents/grpc/src/grpc_agent.h (5)
agents/grpc/src/grpc_client.h (4)
namespace(14-19)node(25-152)nsolid(26-151)grpc(27-149)agents/grpc/src/asset_stream.h (4)
node(11-85)nsolid(12-83)grpc(13-45)AssetStreamObserver(30-35)agents/grpc/src/grpc_utils.h (3)
node(11-54)nsolid(12-53)grpc(13-52)agents/grpc/src/asset_stream.cc (2)
AssetStream(16-39)AssetStream(41-42)agents/grpc/src/command_stream.h (1)
CommandStreamObserver(13-19)
agents/grpc/src/proto/asset.pb.cc (1)
agents/grpc/src/proto/asset.pb.h (25)
protobuf(42-47)protobuf(64-65)protobuf(108-115)protobuf(117-119)protobuf(120-122)protobuf(123-125)Asset(91-91)Asset(92-93)Asset(381-394)Asset(395-408)Asset(452-471)Asset(474-476)Asset(483-487)Asset(488-511)Asset(512-525)Asset(569-588)Asset(591-595)Asset(641-653)Asset(656-660)Asset(665-669)Asset(670-688)Asset(689-693)grpcagent(57-62)grpcagent(68-427)default_instance(126-129)
agents/grpc/src/asset_stream.h (4)
agents/grpc/src/grpc_client.h (3)
node(25-152)nsolid(26-151)grpc(27-149)agents/grpc/src/grpc_agent.h (1)
grpc(39-65)agents/grpc/src/asset_stream.cc (12)
AssetStream(16-39)AssetStream(41-42)OnDone(44-73)OnDone(44-44)OnWriteDone(75-107)OnWriteDone(75-75)Write(132-136)Write(132-132)WritesDone(138-143)WritesDone(138-138)NextWrite(109-130)NextWrite(109-109)agents/grpc/src/grpc_agent.cc (2)
on_asset_stream_done(988-991)on_asset_stream_done(988-988)
🪛 actionlint (1.7.9)
.github/workflows/coverage-linux.yml
51-51: condition "false" is always evaluated to false. remove the if: section
(if-cond)
.github/workflows/linters.yml
118-118: condition "false" is always evaluated to false. remove the if: section
(if-cond)
.github/workflows/coverage-linux-without-intl.yml
51-51: condition "false" is always evaluated to false. remove the if: section
(if-cond)
🪛 Buf (1.61.0)
agents/grpc/proto/metrics.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/asset.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/info.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/blocked_loop.proto
8-8: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/reconfigure.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/exit.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/startup_times.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/source_code.proto
3-3: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/command.proto
8-8: import "profile.proto": file does not exist
(COMPILE)
agents/grpc/proto/packages.proto
8-8: import "common.proto": file does not exist
(COMPILE)
agents/grpc/proto/common.proto
3-3: Files with package "grpcagent" must be within a directory "grpcagent" relative to root but were in directory "agents/grpc/proto".
(PACKAGE_DIRECTORY_MATCH)
agents/grpc/proto/profile.proto
5-5: Files with package "grpcagent" must be within a directory "grpcagent" relative to root but were in directory "agents/grpc/proto".
(PACKAGE_DIRECTORY_MATCH)
agents/grpc/proto/nsolid_service.proto
3-3: import "asset.proto": file does not exist
(COMPILE)
🪛 checkmake (0.2.2)
Makefile
[warning] 220-220: Target body for "distclean" exceeds allowed length of 5 (8).
(maxbodylength)
[warning] 314-314: Target body for "jstest" exceeds allowed length of 5 (6).
(maxbodylength)
[warning] 335-335: Target body for "test" exceeds allowed length of 5 (8).
(maxbodylength)
🪛 LanguageTool
.github/SUPPORT.md
[style] ~4-~4: Consider using a different verb to strengthen your wording.
Context: ...dress general support questions. Please make sure you are using a currently supported ver...
(MAKE_SURE_ENSURE)
CONTRIBUTING.md
[style] ~12-~12: Consider a more concise word here.
Context: .../github.com/nodesource/nsolid/issues>. In order to build this project, you will need a bui...
(IN_ORDER_TO_PREMIUM)
[grammar] ~97-~97: Use a hyphen to join words.
Context: ...TS version. For the initial N|Solid open source release, this branch was `nsolid-...
(QB_NEW_EN_HYPHEN)
[style] ~98-~98: ‘Prior to’ might be wordy. Consider a shorter alternative.
Context: ...release, this branch was nsolid-v5.x. Prior to releases, relevant changes will be port...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[style] ~216-~216: As an alternative to the over-used intensifier ‘quite’, consider replacing this phrase.
Context: ... of the types of Node.js API changes is quite wide, but it is also the least likely to lan...
(EN_WEAK_ADJECTIVE)
[style] ~222-~222: ‘prior to’ might be wordy. Consider a shorter alternative.
Context: ... zero-filling Buffers during allocation prior to the Buffer.alloc upstream Node.js API...
(EN_WORDINESS_PREMIUM_PRIOR_TO)
[grammar] ~293-~293: Use a hyphen to join words.
Context: ...MAJOR version increments in N|Solid. Pre v5.0.0 versions of N|Solid are propr...
(QB_NEW_EN_HYPHEN)
CODE_OF_CONDUCT.md
[style] ~27-~27: Try using a synonym here to strengthen your wording.
Context: ...vances * Trolling, insulting/derogatory comments, and personal or political attacks * Pu...
(COMMENT_REMARK)
[style] ~62-~62: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...s obligated to maintain confidentiality with regard to the reporter of an incident. Further de...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
CHANGELOG.md
[uncategorized] ~5-~5: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ... 22](doc/changelogs/CHANGELOG_V22.md) Long Term Support * [Node.js 21](doc/changelogs...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...js 20](doc/changelogs/CHANGELOG_V20.md) Long Term Support * [Node.js 19](doc/changelogs/C...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
BUILDING.md
[grammar] ~887-~887: Use a hyphen to join words.
Context: ...fficial N|Solid builds (e.g. using a ABI incompatible version of a dependency), p...
(QB_NEW_EN_HYPHEN)
GOVERNANCE.md
[style] ~149-~149: Consider a more concise word here.
Context: ...ree to the recording and/or transcript, in order to comply to privacy regulations. All dis...
(IN_ORDER_TO_PREMIUM)
[style] ~215-~215: The phrase ‘have the ability to’ might be wordy. Consider using “can”.
Context: ...their judgement be trusted, and do they have the ability to work well with others. #### The Authen...
(HAS_THE_ABILITY_TO)
[style] ~223-~223: Consider a more concise word here.
Context: ...n commit access to open-source projects in order to inject malicious code or for other nefa...
(IN_ORDER_TO_PREMIUM)
[style] ~241-~241: Try using a more formal synonym here to elevate your writing.
Context: ...in the private discussion stating you're planning on opening a public issue, e.g. "I see a number...
(PLAN_ON_INTEND)
[grammar] ~279-~279: Use a hyphen to join words.
Context: ...TSC may, following typical TSC consensus seeking processes, choose to advance a n...
(QB_NEW_EN_HYPHEN)
CHANGELOG_NSOLID.md
[uncategorized] ~7-~7: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...logs/NSOLID_CHANGELOG_V5_NODE_V20.md) Long Term Support
(EN_COMPOUND_ADJECTIVE_INTERNAL)
README.md
[uncategorized] ~11-~11: The official name of this software platform is spelled with a capital “H”.
Context: ...ut the instructions for getting support. ### Download Binaries, i...
(GITHUB)
[style] ~48-~48: Consider using a more concise synonym.
Context: ...rnal libraries that are available under a variety of licenses. See LICENSE and [L...
(A_VARIETY_OF)
🪛 markdownlint-cli2 (0.18.1)
BUILDING.md
33-33: Link fragments should be valid
(MD051, link-fragments)
37-37: Link fragments should be valid
(MD051, link-fragments)
38-38: Link fragments should be valid
(MD051, link-fragments)
| } | ||
| } | ||
| }, | ||
| "image": "nodejs/devcontainer:nightly" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
nodejs/devcontainer:nightly image Docker Hub maintenance status
💡 Result:
Short answer: The Docker Hub image nodejs/devcontainer:nightly is not the actively maintained source for Dev Container images. The official devcontainer images were moved to the devcontainers/images project and are published from Microsoft registries (MCR / ghcr) (see below). Many projects that previously referenced nodejs/devcontainer:nightly now use mcr.microsoft.com/devcontainers/... or ghcr.io/devcontainers/... instead. [1][2][3]
Sources:
- Announcement / tracking that images moved to devcontainers/images (repository archived → images moved). [1]
- Official devcontainer images on Microsoft Container Registry (examples: mcr.microsoft.com/devcontainers/javascript-node). [2]
- Example showing older configs that referenced nodejs/devcontainer:nightly (illustrates historical usage). [3]
Use official Microsoft Container Registry devcontainer image instead of deprecated nodejs/devcontainer:nightly.
The nodejs/devcontainer:nightly image source has been archived and is no longer maintained. Official devcontainer images were migrated to the devcontainers/images project and are now published on Microsoft Container Registry (MCR) and GitHub Container Registry. Use mcr.microsoft.com/devcontainers/javascript-node or ghcr.io/devcontainers/javascript-node instead to ensure you receive active maintenance, security updates, and ongoing support.
🤖 Prompt for AI Agents
In .devcontainer.json around line 24, replace the deprecated
"nodejs/devcontainer:nightly" image with the official maintained devcontainer
image (either "mcr.microsoft.com/devcontainers/javascript-node" or
"ghcr.io/devcontainers/javascript-node") and pick an explicit supported tag (for
example a Node LTS tag like ":18" or ":current" per your project's needs);
update the "image" field to use one of these MCR/GHCR URIs so the devcontainer
uses the actively maintained Microsoft Container Registry image.
| - Windows ARM64 | ||
| - Windows x64 |
There was a problem hiding this comment.
Update the label extraction workflow to handle the new Windows platform options.
The flaky test template now distinguishes between "Windows ARM64" and "Windows x64", but the label extraction workflow at .github/workflows/label-flaky-test-issue.yml (line 36) only maps "Windows" to the windows label. The new options won't be recognized.
Update the workflow to add mappings for both new options:
#!/bin/bash
# Verify the current mappings in label-flaky-test-issue.yml
rg -n "platform2label\[\"Windows" .github/workflows/label-flaky-test-issue.yml🤖 Prompt for AI Agents
.github/workflows/label-flaky-test-issue.yml around line 36: the workflow
currently maps only "Windows" to the windows label so the new template options
"Windows ARM64" and "Windows x64" are not recognized; update the platform2label
mapping to add entries for "Windows ARM64" -> "windows" and "Windows x64" ->
"windows" (preserve existing casing and quoting), then run the provided ripgrep
command to verify the new keys are present.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/stale@28ca1036281a5e5922ead5184a1bbf96e5fc984e # v9.0.0 | ||
| - uses: actions/stale@3a9db7e6a41a89f618792c92c0e97cc736e1b13f # v10.0.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
actions/stale v10.0.0 breaking changes remove-stale-when-updated default value
💡 Result:
The default value for remove-stale-when-updated in actions/stale v10.0.0 is true. [1][2]
Sources:
- actions/stale README / inputs (shows "Default value:
true" for remove-stale-when-updated). [1] - v10.0.0 release notes (breaking-change details for v10). [2]
🌐 Web query:
actions/stale v10.0.0 release notes breaking changes migration
💡 Result:
Summary of breaking change in actions/stale v10.0.0 and migration steps
- Breaking change: runtime upgraded from Node.js 20 to Node.js 24. Make sure your environment supports Node 24. [1][2]
- Runner requirement: use GitHub runner version v2.327.1 or later to guarantee compatibility. [1][2]
- Migration steps:
- Update workflows to use actions/stale@v10 (or v10.x). [2]
- Ensure your self-hosted runners (or GitHub-hosted runner image) are on runner v2.327.1+ (or otherwise provide Node 24). [1]
- Test your stale workflow in a non-production repo/branch to catch any Node-related behavior changes (e.g., dependency or runtime differences). [1]
Sources: GitHub releases and repository README. [1] GitHub releases (v10.0.0). [2] actions/stale README / Marketplace.
Verify that the label removal behavior change aligns with stale issue management strategy and ensure runner compatibility.
The removal of remove-stale-when-updated: false changes behavior significantly: the default value in v10.0.0 is true, meaning the "stalled" label will now automatically be removed when issues or PRs are updated (previously it was preserved). Confirm this aligns with your workflow.
Additionally, v10.0.0 upgrades the Node.js runtime from Node 20 to Node 24 and requires GitHub runners v2.327.1 or later. Verify your infrastructure supports these requirements.
🤖 Prompt for AI Agents
.github/workflows/close-stalled.yml around line 23: the workflow switched the
stale action to v10.0.0 which changes default label behavior
(remove-stale-when-updated now true) and upgrades Node to v24 with runner
requirement v2.327.1+. If you want to preserve the old behavior, explicitly add
remove-stale-when-updated: false to the action configuration (or pin to an
earlier action version that uses the old default); otherwise confirm this
automatic removal of the "stalled" label is intended. Also verify your
self-hosted or GitHub-hosted runners are at least v2.327.1 (or stay on a prior
action/runtime) and run the workflow in a staging repo to validate label
handling and runner compatibility before merging.
| # Disabled because "Report JS" step was crashing. | ||
| if: false |
There was a problem hiding this comment.
Address disabled coverage job.
The coverage job is disabled due to a crashing "Report JS" step. This workflow should either:
- Fix the crash and remove the
if: falsecondition, or - Remove the job entirely and file a tracking issue if it's deferred
Leaving a disabled, unmaintained job increases technical debt. Clarify the plan in the PR description or as a follow-up issue.
🧰 Tools
🪛 actionlint (1.7.9)
51-51: condition "false" is always evaluated to false. remove the if: section
(if-cond)
🤖 Prompt for AI Agents
.github/workflows/coverage-linux-without-intl.yml lines 50-51: the coverage job
is currently disabled with "if: false" due to a crashing "Report JS" step;
either (A) fix the underlying crash and remove the `if: false` so the job runs
again — reproduce the crash locally or in CI, capture the failing step logs,
correct the script or tooling (e.g., update report-js version, fix paths/env
vars, or guard against missing artifacts), add assertions to prevent regression,
and confirm the workflow completes; or (B) remove the entire job from the
workflow and create a tracking issue (link it in the PR description) documenting
why it was removed and next steps, ensuring no leftover disabled blocks remain.
| - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 | ||
| with: | ||
| persist-credentials: false | ||
| - name: Use Node.js ${{ env.NODE_VERSION }} | ||
| uses: actions/setup-node@60edb5dd545a775178f52524783378180af0d1f8 # v4.0.2 | ||
| uses: actions/setup-node@a0853c24544627f65ddf259abe73b1d18a591444 # v5.0.0 |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
actions/checkout v5.0.0 breaking changes GitHub
💡 Result:
actions/checkout v5.0.0 breaking change: the action runtime was upgraded to Node.js 24 — this requires GitHub Actions Runner v2.327.1 or newer to run. [1][2]
Sources:
[1] actions/checkout repository "Checkout v5" notes. [2] CHANGELOG / v5.0.0 release entry.
🌐 Web query:
actions/setup-node v5.0.0 breaking changes GitHub
💡 Result:
Short summary — actions/setup-node v5.0.0 breaking changes
- Automatic package-manager caching enabled when package.json has a valid packageManager field (caching on by default). To disable: set package-manager-cache: false in the action step. [1]
- Action runtime upgraded to Node.js 24 — runners must be GitHub Actions runner v2.327.1 or later for compatibility. [1][2]
Quick mitigation examples
- Disable automatic caching:
- uses: actions/setup-node@v5
with:
package-manager-cache: false
(keep actions/checkout as usual). [1]
- uses: actions/setup-node@v5
Sources
- actions/setup-node v5.0.0 release notes. [GitHub Releases]. [1]
- actions/setup-node README (breaking-changes summary). [GitHub repo]. [2]
Ensure runner compatibility and configure setup-node caching behavior for v5.0.0 upgrades.
Both actions/checkout and actions/setup-node v5.0.0 have breaking changes:
- Runner version requirement: Both actions now require GitHub Actions Runner v2.327.1 or later due to Node.js 24 runtime upgrade.
- Auto-caching in setup-node: Automatic package-manager caching is now enabled by default when
package.jsoncontains a validpackageManagerfield. If this behavior is undesired, addpackage-manager-cache: falseto theactions/setup-nodestep.
Verify that your hosted runners meet the minimum runner version, and decide whether to disable auto-caching if it conflicts with your build strategy.
🤖 Prompt for AI Agents
.github/workflows/daily.yml around lines 18 to 22: update the workflow to ensure
runner compatibility and control setup-node caching for the v5.0.0 actions;
verify your runners are at GitHub Actions Runner v2.327.1 or later (upgrade
hosted/self-hosted runners as needed) and, unless you want automatic
package-manager caching, add the setup-node input package-manager-cache: false
to the actions/setup-node step so auto-caching is disabled.
| void GrpcAgent::command_stream_closed(const ::grpc::Status& status) { | ||
| const ::grpc::StatusCode code = status.error_code(); | ||
| if (code == ::grpc::StatusCode::UNAUTHENTICATED) { | ||
| Debug("Error Authenticating. Retrying in %ld ms\n", auth_timer_interval); | ||
| auth_retries_++; | ||
| if (auth_retries_ == MAX_AUTH_RETRIES) { | ||
| fprintf(stderr, | ||
| "N|Solid warning: %s Unable to authenticate, " | ||
| "please verify your token and network connection!\n", | ||
| agent_id_.c_str()); | ||
| unauthorized_ = true; | ||
| QueueCallback([](WeakGrpcAgent agent_wp) { | ||
| SharedGrpcAgent agent = agent_wp.lock(); | ||
| if (agent == nullptr) { | ||
| return; | ||
| } | ||
|
|
||
| agent->stop(); | ||
| delete agent.get(); | ||
| }, weak_from_this()); | ||
| return; |
There was a problem hiding this comment.
Avoid manually deleting a SharedGrpcAgent‑managed instance (double‑free risk)
Inside command_stream_closed, after hitting MAX_AUTH_RETRIES, the queued callback does:
SharedGrpcAgent agent = agent_wp.lock();
...
agent->stop();
delete agent.get();SharedGrpcAgent is a shared_ptr with a custom deleter (see GrpcAgent::Inst()), so calling delete agent.get() will free the object while the shared_ptr control block still believes it owns it. When the last shared_ptr reference is later released, the custom deleter will run again, causing a double‑free/use‑after‑free.
You should not manually delete the raw pointer here; rely on the shared_ptr lifetime instead, e.g.:
- agent->stop();
- delete agent.get();
+ agent->stop();
+ // Let shared_ptr ownership control the GrpcAgent lifetime.If you truly need the singleton to be torn down, consider restructuring Inst() (e.g., adding an explicit Reset() path) rather than bypassing shared_ptr ownership.
🤖 Prompt for AI Agents
In agents/grpc/src/grpc_agent.cc around lines 501 to 521, the queued callback
manually calls delete on agent.get(), which will double‑free the object because
SharedGrpcAgent is a shared_ptr with a custom deleter; remove the manual delete
and rely on the shared_ptr to manage lifetime (call agent->stop() only), and if
you need explicit teardown of the singleton provide and call a dedicated
reset/Release/Shutdown path from GrpcAgent::Inst() instead of deleting the raw
pointer.
| DebugJSON("Old Config: \n%s\n", old_config); | ||
| DebugJSON("NewConfig: \n%s\n", config_); | ||
| DebugJSON("Diff: \n%s\n", diff); |
There was a problem hiding this comment.
Redact /saas in config debug logging to avoid leaking tokens
config() logs full old_config, config_, and diff via DebugJSON. If the config contains a "/saas" field (as handled just above), those logs will include the raw SaaS token. That token is effectively a secret and shouldn’t appear in debug logs or crash dumps.
Consider redacting or removing sensitive fields before logging, e.g., overwrite config_["saas"] (and corresponding entries in old_config/diff) with a constant string like "<redacted>" before calling DebugJSON.
🤖 Prompt for AI Agents
In agents/grpc/src/grpc_agent.cc around lines 1006 to 1008, the DebugJSON calls
print raw old_config, config_, and diff which may include a "/saas" token;
create redacted copies of those objects (or shallow copies if they are JSON-like
maps) and replace any top-level "saas" entry (and any corresponding entries
inside the diff) with a constant string like "<redacted>" before calling
DebugJSON, then log the redacted copies so the original objects remain
unchanged.
| bool insecure = false; | ||
| auto insecure_str = | ||
| per_process::system_environment->Get(kNSOLID_GRPC_INSECURE); | ||
| // Only parse the insecure flag in non SaaS mode. | ||
| if (insecure_str.has_value() && (!saas_ || saas_->testing)) { | ||
| // insecure = std::stoull(insecure_str.value()); | ||
| insecure = std::stoi(insecure_str.value()); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file at the specified lines
wc -l agents/grpc/src/grpc_agent.ccRepository: nodesource/nsolid
Length of output: 95
🏁 Script executed:
# Read the specific section mentioned in the review
sed -n '1023,1030p' agents/grpc/src/grpc_agent.ccRepository: nodesource/nsolid
Length of output: 425
🏁 Script executed:
# Get broader context around these lines to understand the function
sed -n '1000,1050p' agents/grpc/src/grpc_agent.ccRepository: nodesource/nsolid
Length of output: 1904
🏁 Script executed:
# Check if there's any exception handling around this code
sed -n '1000,1100p' agents/grpc/src/grpc_agent.cc | grep -A 5 -B 5 "try\|catch\|stoi"Repository: nodesource/nsolid
Length of output: 556
🏁 Script executed:
# Check if there's a try-catch block wrapping the entire config() function
sed -n '1000,1200p' agents/grpc/src/grpc_agent.cc | head -100Repository: nodesource/nsolid
Length of output: 3606
🏁 Script executed:
# Search for similar environment variable parsing patterns in the file
rg "system_environment->Get" agents/grpc/src/grpc_agent.cc -A 3 -B 1Repository: nodesource/nsolid
Length of output: 581
🏁 Script executed:
# Check how std::stoi is used elsewhere in the codebase and if exceptions are handled
rg "std::stoi|std::stoull|std::stol" --type cc -A 2 -B 2 | head -60Repository: nodesource/nsolid
Length of output: 87
🏁 Script executed:
# Verify Debug function is accessible in this scope
rg "Debug\(" agents/grpc/src/grpc_agent.cc | head -5Repository: nodesource/nsolid
Length of output: 320
🏁 Script executed:
# Check if there's any higher-level exception handling that might catch this
rg "try|catch" agents/grpc/src/grpc_agent.cc | head -20Repository: nodesource/nsolid
Length of output: 1259
🏁 Script executed:
# Look at the function signature and return type to understand error handling pattern
sed -n '1000,1010p' agents/grpc/src/grpc_agent.ccRepository: nodesource/nsolid
Length of output: 401
Guard NSOLID_GRPC_INSECURE parsing against invalid values
std::stoi() throws an exception on non-numeric input. If NSOLID_GRPC_INSECURE is set to a non-numeric value (e.g., "true", "yes", or a typo), the process crashes without graceful error handling.
Replace the exception-prone parsing with explicit value checks:
- if (insecure_str.has_value() && (!saas_ || saas_->testing)) {
- // insecure = std::stoull(insecure_str.value());
- insecure = std::stoi(insecure_str.value());
- }
+ if (insecure_str.has_value() && (!saas_ || saas_->testing)) {
+ const auto &v = insecure_str.value();
+ if (v == "1" || v == "true" || v == "TRUE") {
+ insecure = true;
+ } else if (v == "0" || v == "false" || v == "FALSE") {
+ insecure = false;
+ } else {
+ Debug("Ignoring invalid %s value: %s\n", kNSOLID_GRPC_INSECURE, v.c_str());
+ }
+ }This safely handles invalid values while supporting common boolean representations.
🤖 Prompt for AI Agents
agents/grpc/src/grpc_agent.cc around lines 1023 to 1030: the code uses std::stoi
on NSOLID_GRPC_INSECURE which can throw on non-numeric input; replace this with
a safe, exception-free parse by lowercasing the string and comparing against
accepted boolean representations ("1","0","true","false","yes","no"); set
insecure=true for "1","true","yes", insecure=false for "0","false","no", and
treat any other value as invalid by leaving insecure=false and emitting a
warning/log entry; keep the existing SaaS/testing guard and ensure no exceptions
are possible during parsing.
| Debug("Parsing SaaS token: %s\n", token.c_str()); | ||
| std::string pubKey = token.substr(0, PUB_KEY_SIZE); | ||
| std::replace(pubKey.begin(), pubKey.end(), ',', '!'); | ||
| std::string saasUrl = token.substr(PUB_KEY_SIZE, token.length()); | ||
| std::string baseUrl; | ||
| std::string basePort; | ||
| std::istringstream saasStream(saasUrl); | ||
| std::getline(saasStream, baseUrl, ':'); | ||
| std::getline(saasStream, basePort, ':'); | ||
|
|
||
| if (baseUrl.empty() || basePort.empty() || pubKey.length() != PUB_KEY_SIZE) { | ||
| Debug("Invalid SaaS token: %s\n", token.c_str()); | ||
| return; | ||
| } | ||
|
|
||
| std::string console_id = baseUrl.substr(0, baseUrl.find('.')); | ||
| if (console_id.size() != CONSOLE_ID_SIZE) { | ||
| Debug("Invalid SaaS token: %s\n", token.c_str()); | ||
| return; | ||
| } | ||
|
|
||
| std::string endpoint; | ||
| bool is_testing = false; | ||
| bool is_staging = token.find("staging") != std::string::npos; | ||
| if (is_staging) { | ||
| endpoint = console_id + ".grpc.staging.nodesource.io:443"; | ||
| } else { | ||
| is_testing = token.find("testing") != std::string::npos; | ||
| if (is_testing) { | ||
| // For testing, set endpoint to the string after the last dot in the token | ||
| size_t last_dot = token.rfind('.'); | ||
| if (last_dot != std::string::npos && last_dot + 1 < token.size()) { | ||
| endpoint = token.substr(last_dot + 1); | ||
| } else { | ||
| endpoint = "localhost:50051"; // fallback if no dot is found | ||
| } | ||
| } else { | ||
| endpoint = console_id + ".grpc.nodesource.io:443"; | ||
| } | ||
| } | ||
|
|
||
| saas_ = std::make_unique<SaaSInfo>( | ||
| SaaSInfo{token, std::move(endpoint), is_testing}); | ||
| } |
There was a problem hiding this comment.
Do not log raw SaaS tokens (secrets) and harden token parsing
parse_saas_token currently does:
Debug("Parsing SaaS token: %s\n", token.c_str());
...
Debug("Invalid SaaS token: %s\n", token.c_str());This logs the entire SaaS token on both success and failure paths, which is a secret leakage risk (logs, support bundles, etc.). The token should be treated as confidential.
Suggested changes:
- Debug("Parsing SaaS token: %s\n", token.c_str());
+ Debug("Parsing SaaS token\n");
...
- Debug("Invalid SaaS token: %s\n", token.c_str());
+ Debug("Invalid SaaS token format\n");
...
- Debug("Invalid SaaS token: %s\n", token.c_str());
+ Debug("Invalid SaaS token console id\n");If you need diagnostics, log only non‑sensitive derived values (e.g. whether it’s staging/testing and the computed endpoint), not the original token.
🤖 Prompt for AI Agents
In agents/grpc/src/grpc_agent.cc around lines 1645 to 1688, remove any Debug
calls that print the raw token and replace them with non‑sensitive diagnostics
(e.g., log only is_staging/is_testing and the computed endpoint), and harden
parsing by validating token length and delimiter positions before calling substr
or rfind (check token.size() >= PUB_KEY_SIZE, ensure ':' separators exist and
console_id length matches CONSOLE_ID_SIZE) to avoid out‑of‑bounds substrings;
never include token.c_str() or the full pubKey in logs—only log redacted or
derived values.
| // For the Assets JS API | ||
| std::map<SharedEnvInst, v8::Global<v8::Function>> asset_cb_map_; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for asset_cb_map_ usage
echo "=== asset_cb_map_ usage ==="
rg -n 'asset_cb_map_' agents/grpc/src/
echo ""
echo "=== env_deletion_cb_ implementation ==="
rg -n -A 20 'env_deletion_cb_' agents/grpc/src/grpc_agent.ccRepository: nodesource/nsolid
Length of output: 1632
🏁 Script executed:
#!/bin/bash
# Search for asset_cb_map_ cleanup/erase operations
echo "=== asset_cb_map_ erase/cleanup ==="
rg -n 'asset_cb_map_\.erase|asset_cb_map_\.clear' agents/grpc/src/
echo ""
echo "=== env_msg_cb_ full implementation ==="
rg -n -A 50 'void GrpcAgent::env_msg_cb_' agents/grpc/src/grpc_agent.cc | head -80
echo ""
echo "=== Full context of asset_cb_map_ emplace at line 496 ==="
sed -n '490,510p' agents/grpc/src/grpc_agent.ccRepository: nodesource/nsolid
Length of output: 2814
🏁 Script executed:
#!/bin/bash
# Search for all asset_cb_map_ usages
echo "=== All asset_cb_map_ references ==="
rg -n -B 3 -A 3 'asset_cb_map_' agents/grpc/src/
echo ""
echo "=== Check for any cleanup in destructors or stop methods ==="
rg -n -A 30 '~GrpcAgent|do_stop' agents/grpc/src/grpc_agent.cc | grep -A 30 'do_stop'Repository: nodesource/nsolid
Length of output: 5121
Remove asset_cb_map_ entries when environments are deleted.
The asset_cb_map_ has no cleanup mechanism. When an environment is destroyed, its entry remains in the map indefinitely, causing a memory leak and holding references to the deleted environment. Unlike env_metrics_map_, which is properly erased during environment deletion (in env_msg_cb_), asset_cb_map_ should erase the corresponding entry when env_deletion_cb_ is invoked.
🤖 Prompt for AI Agents
In agents/grpc/src/grpc_agent.h around lines 381-382, asset_cb_map_ is never
cleaned up when environments are deleted; update the environment deletion path
(env_deletion_cb_) to look up the SharedEnvInst key in asset_cb_map_, if found
call .Reset() on the stored v8::Global<v8::Function> to release the V8 handle
and then erase the map entry (or use asset_cb_map_.erase(it)); ensure this is
done under the same lock/guard used for other env maps to avoid races.
santigimeno
changed the base branch from
node-v24.x-nsolid-v6.x
to
node-v22.x-nsolid-v6.x
1 participant
Summary by CodeRabbit
Release Notes
New Features
Infrastructure & Configuration
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.