Skip to content

src: do not enable the inspector during snapshot building - #67154

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
joyeecheung:fix-snapshot-inspector
Nov 5, 2026
Merged

nodejs-github-bot merged 1 commit into
nodejs:mainfrom
joyeecheung:fix-snapshot-inspector

Conversation

@joyeecheung

@joyeecheung joyeecheung commented Oct 31, 2026 •

Copy link
Copy Markdown
Member

V8 no longer allows attaching a debuger or setting a debug context ID during snapshot building. Avoid enabling the inspector in this case, and reject the inspector options when the user-land --build-snapshot is used.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/8679045

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/config
  • @nodejs/inspector
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues and PRs related to Node.js configuration and feature settings. inspector Issues and PRs related to the V8 inspector protocol. needs-ci PRs that need a full CI run. labels Oct 31, 2026
@codecov

codecov Bot commented Oct 31, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.67%. Comparing base (3b209b2) to head (e6e89f9).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/inspector_agent.cc 50.00% 2 Missing and 1 partial ⚠️
src/node_options.cc 93.75% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #67154      +/-   ##
==========================================
- Coverage   90.68%   90.67%   -0.02%     
==========================================
  Files         797      797              
  Lines      279367   279381      +14     
  Branches    53737    53741       +4     
==========================================
- Hits       253344   253324      -20     
- Misses      16319    16354      +35     
+ Partials     9704     9703       -1     
Files with missing lines Coverage Δ
src/node_options.h 95.83% <100.00%> (+0.12%) ⬆️
src/node_options.cc 88.20% <93.75%> (+0.01%) ⬆️
src/inspector_agent.cc 83.01% <50.00%> (-0.28%) ⬇️

... and 28 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@legendecas legendecas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but linter is complaining.

@joyeecheung
joyeecheung force-pushed the fix-snapshot-inspector branch from b0271b9 to 7feef72 Compare November 2, 2026 17:14
@joyeecheung

Copy link
Copy Markdown
Member Author

Force-pushed to sign it off and fixed the linter complaint.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@legendecas legendecas added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Nov 3, 2026
V8 no longer allows attaching a debuger or setting a debug context ID
during snapshot building. Avoid enabling the inspector in this case,
and reject the inspector options when the user-land --build-snapshot
is used.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/8679045
Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com>
@joyeecheung
joyeecheung force-pushed the fix-snapshot-inspector branch from d475908 to e6e89f9 Compare November 4, 2026 16:14
@joyeecheung

Copy link
Copy Markdown
Member Author

Not sure why linter is failing at ib/internal/vfs/watcher.js, rebased to see if it goes away..

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@joyeecheung

Copy link
Copy Markdown
Member Author

CI is finally green. @legendecas can you take a look again?

Also need another approval to land maybe @nodejs/build @nodejs/v8-update would be interested in taking a look..

@richardlau

Copy link
Copy Markdown
Member

Should this have semver/dont-land labels?

@joyeecheung

joyeecheung commented Nov 5, 2026 •

Copy link
Copy Markdown
Member Author

Should this have semver/dont-land labels?

It's a very niche use case and I doubt this was actually even working before, even if people did manage to make it work somehow it's unlikely to be used in automated tests since it's for debugging the snapshot building process with the inspector. I think it should be safe to land in older versions.

@joyeecheung joyeecheung added the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 5, 2026
@nodejs-github-bot
nodejs-github-bot merged commit 5e893ce into nodejs:main Nov 5, 2026
60 checks passed
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 5e893ce

@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Nov 5, 2026
aduh95 pushed a commit that referenced this pull request Nov 11, 2026
V8 no longer allows attaching a debuger or setting a debug context ID
during snapshot building. Avoid enabling the inspector in this case,
and reject the inspector options when the user-land --build-snapshot
is used.

Refs: https://chromium-review.googlesource.com/c/v8/v8/+/8679045
Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #67154
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Richard Lau <richard.lau@ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. config Issues and PRs related to Node.js configuration and feature settings. inspector Issues and PRs related to the V8 inspector protocol. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants