debugger: wait for target startup before initialization - #64304

Closed
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest
Closed

debugger: wait for target startup before initialization#64304
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest

Conversation

@Archkon

@ArchkonArchkon commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Trying to fix the test error that triggered at previous pr when run github action ci/cd

https://github.com/nodejs/node/actions/runs/28730483133/job/85195106458?pr=64301

Fixes: #64116
Fixes: #61762
Fixes: #64005

@nodejs-github-botnodejs-github-bot added debugger Issues and PRs related to the Node.js command-line debugger. needs-ci PRs that need a full CI run. labels Jul 5, 2026
@trivikr

Copy link
Copy Markdown
Member

Several previous attempts of deflaking were unsuccessful.
Please refer #64116 for prior discussions and links to other PRs.

@Archkon

This comment was marked as spam.

@Archkon
Archkon marked this pull request as draft July 6, 2026 05:26
@Archkon
Archkonforce-pushed the fixtest branch 3 times, most recently from 88fe3e7 to 1e6cde7CompareJuly 29, 2026 23:48
@ArchkonArchkon changed the title debugger: defer pause to avoid pause foreverdebugger: wait for target startup before initializationJul 29, 2026
@Archkon
Archkon marked this pull request as ready for review July 29, 2026 23:58
@codecov

codecovBot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.49123% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (c18fd90) to head (aa1cfd8).
⚠️ Report is 12 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/debugger/inspect_helpers.js95.23%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #64304 +/- ##
==========================================
- Coverage 90.31% 90.31% -0.01% 
==========================================
Files 759 759 Lines 247646 248350 +704 Branches 46697 46871 +174 ==========================================
+ Hits 223654 224288 +634 - Misses 15463 15473 +10 - Partials 8529 8589 +60 
Files with missing linesCoverage Δ
lib/internal/debugger/inspect_probe.js82.05% <100.00%> (-0.09%)⬇️
lib/internal/debugger/inspect_repl.js91.36% <100.00%> (+0.05%)⬆️
lib/internal/debugger/inspect_helpers.js97.00% <95.23%> (-0.41%)⬇️

... and 40 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.

@Archkon

This comment was marked as spam.

@avivkeller
avivkeller requested review from joyeecheung and trivikr and removed request for joyeecheungAugust 5, 2026 02:05
@Archkon

This comment was marked as spam.

@joyeecheung

Copy link
Copy Markdown
Member

I think at the minimum the commit should explain why the previous flakes happen, and why the change is supposed to fix it?

The debugger endpoint can accept a connection before the target has
entered its startup wait. In that window,Runtime.runIfWaitingForDebugger
may be handled too early, leaving the target waiting indefinitely.
Use NodeRuntime.waitingForDebugger as a readiness handshake before
initializing the debugger domains and releasing the target. Apply the
handshake to both the interactive debugger and probe mode,and reject the
wait if the session closes.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Debugger tests spawn a debugger client and target process, then wait for
asynchronous CLI output through a shared test helper.Under high parallel
load on macOS, process scheduling and inspector communication can delay
progress beyond the existing 15-second timeout, causing intermittent
failures across multiple debugger tests.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Run the debugger CLI in the per-test temporary directory so concurrent
runs do not overwrite or remove the same node.cpuprofile file.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@Archkon

This comment was marked as spam.

@Archkon
Archkonforce-pushed the fixtest branch 2 times, most recently from b85de2c to aa1cfd8CompareAugust 7, 2026 04:08
@Archkon

This comment was marked as spam.

Comment on lines +13 to +15
if (common.isMacOS) {
TIMEOUT = common.platformTimeout(30000);
} else if (common.isWindows) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon

Thanks for working on this!

Could you clarify why the timeout needs to be increased? If waitForDebugger() addresses the race condition, I would expect the timeout increase to be unnecessary.

Also, I think keeping the original timeout would make the stress test results more convincing, since otherwise it’s difficult to tell whether the improvement comes from waitForDebugger() or simply from the increased timeout.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks for clarifying. If Debugger.paused was emitted but not received by the JavaScript layer within 15 seconds, could you share a failing run or trace showing that?

Because the current stress runs include both changes, it is difficult to tell whether waitForDebugger() alone resolves the issue.

Comment on lines +15 to +19
const cli = startCLI(
[fixtures.path('debugger/empty.js')],
[],
{ cwd: tmpdir.path },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Could you remove this change from the PR?

It appears to be needed only for the --repeat 1000 -j16 stress run, where concurrent instances of this test share the same node.cpuprofile. It is unrelated to the debugger startup race and is not needed in the normal CI run.

If it is needed for stress validation, it can remain only in the stress-test branch or workflow.

This comment was marked as spam.

This comment was marked as spam.

@inoway46inoway46Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks. My concern is the PR scope, not the -j4 setting.

The linked run still executes this test 1000 times. Normal CI executes it only once, so the node.cpuprofile collision shown there is specific to the stress run. I think this change should be removed from this PR and kept only in the stress-test setup if needed.

Also, please leave this thread unresolved while this concern is still under discussion.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debuggerIssues and PRs related to the Node.js command-line debugger.needs-ciPRs that need a full CI run.

Projects

None yet

6 participants

@Archkon@trivikr@joyeecheung@inoway46@aduh95@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

debugger: wait for target startup before initialization - #64304

Closed
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest
Closed

debugger: wait for target startup before initialization#64304
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest

Conversation

@Archkon

@ArchkonArchkon commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Trying to fix the test error that triggered at previous pr when run github action ci/cd

https://github.com/nodejs/node/actions/runs/28730483133/job/85195106458?pr=64301

Fixes: #64116
Fixes: #61762
Fixes: #64005

@nodejs-github-botnodejs-github-bot added debugger Issues and PRs related to the Node.js command-line debugger. needs-ci PRs that need a full CI run. labels Jul 5, 2026
@trivikr

Copy link
Copy Markdown
Member

Several previous attempts of deflaking were unsuccessful.
Please refer #64116 for prior discussions and links to other PRs.

@Archkon

This comment was marked as spam.

@Archkon
Archkon marked this pull request as draft July 6, 2026 05:26
@Archkon
Archkonforce-pushed the fixtest branch 3 times, most recently from 88fe3e7 to 1e6cde7CompareJuly 29, 2026 23:48
@ArchkonArchkon changed the title debugger: defer pause to avoid pause foreverdebugger: wait for target startup before initializationJul 29, 2026
@Archkon
Archkon marked this pull request as ready for review July 29, 2026 23:58
@codecov

codecovBot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.49123% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (c18fd90) to head (aa1cfd8).
⚠️ Report is 12 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/debugger/inspect_helpers.js95.23%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #64304 +/- ##
==========================================
- Coverage 90.31% 90.31% -0.01% 
==========================================
Files 759 759 Lines 247646 248350 +704 Branches 46697 46871 +174 ==========================================
+ Hits 223654 224288 +634 - Misses 15463 15473 +10 - Partials 8529 8589 +60 
Files with missing linesCoverage Δ
lib/internal/debugger/inspect_probe.js82.05% <100.00%> (-0.09%)⬇️
lib/internal/debugger/inspect_repl.js91.36% <100.00%> (+0.05%)⬆️
lib/internal/debugger/inspect_helpers.js97.00% <95.23%> (-0.41%)⬇️

... and 40 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.

@Archkon

This comment was marked as spam.

@avivkeller
avivkeller requested review from joyeecheung and trivikr and removed request for joyeecheungAugust 5, 2026 02:05
@Archkon

This comment was marked as spam.

@joyeecheung

Copy link
Copy Markdown
Member

I think at the minimum the commit should explain why the previous flakes happen, and why the change is supposed to fix it?

The debugger endpoint can accept a connection before the target has
entered its startup wait. In that window,Runtime.runIfWaitingForDebugger
may be handled too early, leaving the target waiting indefinitely.
Use NodeRuntime.waitingForDebugger as a readiness handshake before
initializing the debugger domains and releasing the target. Apply the
handshake to both the interactive debugger and probe mode,and reject the
wait if the session closes.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Debugger tests spawn a debugger client and target process, then wait for
asynchronous CLI output through a shared test helper.Under high parallel
load on macOS, process scheduling and inspector communication can delay
progress beyond the existing 15-second timeout, causing intermittent
failures across multiple debugger tests.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Run the debugger CLI in the per-test temporary directory so concurrent
runs do not overwrite or remove the same node.cpuprofile file.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@Archkon

This comment was marked as spam.

@Archkon
Archkonforce-pushed the fixtest branch 2 times, most recently from b85de2c to aa1cfd8CompareAugust 7, 2026 04:08
@Archkon

This comment was marked as spam.

Comment on lines +13 to +15
if (common.isMacOS) {
TIMEOUT = common.platformTimeout(30000);
} else if (common.isWindows) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon

Thanks for working on this!

Could you clarify why the timeout needs to be increased? If waitForDebugger() addresses the race condition, I would expect the timeout increase to be unnecessary.

Also, I think keeping the original timeout would make the stress test results more convincing, since otherwise it’s difficult to tell whether the improvement comes from waitForDebugger() or simply from the increased timeout.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks for clarifying. If Debugger.paused was emitted but not received by the JavaScript layer within 15 seconds, could you share a failing run or trace showing that?

Because the current stress runs include both changes, it is difficult to tell whether waitForDebugger() alone resolves the issue.

Comment on lines +15 to +19
const cli = startCLI(
[fixtures.path('debugger/empty.js')],
[],
{ cwd: tmpdir.path },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Could you remove this change from the PR?

It appears to be needed only for the --repeat 1000 -j16 stress run, where concurrent instances of this test share the same node.cpuprofile. It is unrelated to the debugger startup race and is not needed in the normal CI run.

If it is needed for stress validation, it can remain only in the stress-test branch or workflow.

This comment was marked as spam.

This comment was marked as spam.

@inoway46inoway46Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks. My concern is the PR scope, not the -j4 setting.

The linked run still executes this test 1000 times. Normal CI executes it only once, so the node.cpuprofile collision shown there is specific to the stress run. I think this change should be removed from this PR and kept only in the stress-test setup if needed.

Also, please leave this thread unresolved while this concern is still under discussion.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debuggerIssues and PRs related to the Node.js command-line debugger.needs-ciPRs that need a full CI run.

Projects

None yet

6 participants

@Archkon@trivikr@joyeecheung@inoway46@aduh95@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

debugger: wait for target startup before initialization - #64304

Closed
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest
Closed

debugger: wait for target startup before initialization#64304
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest

Conversation

@Archkon

@ArchkonArchkon commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Trying to fix the test error that triggered at previous pr when run github action ci/cd

https://github.com/nodejs/node/actions/runs/28730483133/job/85195106458?pr=64301

Fixes: #64116
Fixes: #61762
Fixes: #64005

@nodejs-github-botnodejs-github-bot added debugger Issues and PRs related to the Node.js command-line debugger. needs-ci PRs that need a full CI run. labels Jul 5, 2026
@trivikr

Copy link
Copy Markdown
Member

Several previous attempts of deflaking were unsuccessful.
Please refer #64116 for prior discussions and links to other PRs.

@Archkon

This comment was marked as spam.

@Archkon
Archkon marked this pull request as draft July 6, 2026 05:26
@Archkon
Archkonforce-pushed the fixtest branch 3 times, most recently from 88fe3e7 to 1e6cde7CompareJuly 29, 2026 23:48
@ArchkonArchkon changed the title debugger: defer pause to avoid pause foreverdebugger: wait for target startup before initializationJul 29, 2026
@Archkon
Archkon marked this pull request as ready for review July 29, 2026 23:58
@codecov

codecovBot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.49123% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (c18fd90) to head (aa1cfd8).
⚠️ Report is 12 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/debugger/inspect_helpers.js95.23%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #64304 +/- ##
==========================================
- Coverage 90.31% 90.31% -0.01% 
==========================================
Files 759 759 Lines 247646 248350 +704 Branches 46697 46871 +174 ==========================================
+ Hits 223654 224288 +634 - Misses 15463 15473 +10 - Partials 8529 8589 +60 
Files with missing linesCoverage Δ
lib/internal/debugger/inspect_probe.js82.05% <100.00%> (-0.09%)⬇️
lib/internal/debugger/inspect_repl.js91.36% <100.00%> (+0.05%)⬆️
lib/internal/debugger/inspect_helpers.js97.00% <95.23%> (-0.41%)⬇️

... and 40 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.

@Archkon

This comment was marked as spam.

@avivkeller
avivkeller requested review from joyeecheung and trivikr and removed request for joyeecheungAugust 5, 2026 02:05
@Archkon

This comment was marked as spam.

@joyeecheung

Copy link
Copy Markdown
Member

I think at the minimum the commit should explain why the previous flakes happen, and why the change is supposed to fix it?

The debugger endpoint can accept a connection before the target has
entered its startup wait. In that window,Runtime.runIfWaitingForDebugger
may be handled too early, leaving the target waiting indefinitely.
Use NodeRuntime.waitingForDebugger as a readiness handshake before
initializing the debugger domains and releasing the target. Apply the
handshake to both the interactive debugger and probe mode,and reject the
wait if the session closes.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Debugger tests spawn a debugger client and target process, then wait for
asynchronous CLI output through a shared test helper.Under high parallel
load on macOS, process scheduling and inspector communication can delay
progress beyond the existing 15-second timeout, causing intermittent
failures across multiple debugger tests.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Run the debugger CLI in the per-test temporary directory so concurrent
runs do not overwrite or remove the same node.cpuprofile file.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@Archkon

This comment was marked as spam.

@Archkon
Archkonforce-pushed the fixtest branch 2 times, most recently from b85de2c to aa1cfd8CompareAugust 7, 2026 04:08
@Archkon

This comment was marked as spam.

Comment on lines +13 to +15
if (common.isMacOS) {
TIMEOUT = common.platformTimeout(30000);
} else if (common.isWindows) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon

Thanks for working on this!

Could you clarify why the timeout needs to be increased? If waitForDebugger() addresses the race condition, I would expect the timeout increase to be unnecessary.

Also, I think keeping the original timeout would make the stress test results more convincing, since otherwise it’s difficult to tell whether the improvement comes from waitForDebugger() or simply from the increased timeout.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks for clarifying. If Debugger.paused was emitted but not received by the JavaScript layer within 15 seconds, could you share a failing run or trace showing that?

Because the current stress runs include both changes, it is difficult to tell whether waitForDebugger() alone resolves the issue.

Comment on lines +15 to +19
const cli = startCLI(
[fixtures.path('debugger/empty.js')],
[],
{ cwd: tmpdir.path },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Could you remove this change from the PR?

It appears to be needed only for the --repeat 1000 -j16 stress run, where concurrent instances of this test share the same node.cpuprofile. It is unrelated to the debugger startup race and is not needed in the normal CI run.

If it is needed for stress validation, it can remain only in the stress-test branch or workflow.

This comment was marked as spam.

This comment was marked as spam.

@inoway46inoway46Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks. My concern is the PR scope, not the -j4 setting.

The linked run still executes this test 1000 times. Normal CI executes it only once, so the node.cpuprofile collision shown there is specific to the stress run. I think this change should be removed from this PR and kept only in the stress-test setup if needed.

Also, please leave this thread unresolved while this concern is still under discussion.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debuggerIssues and PRs related to the Node.js command-line debugger.needs-ciPRs that need a full CI run.

Projects

None yet

6 participants

@Archkon@trivikr@joyeecheung@inoway46@aduh95@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

debugger: wait for target startup before initialization - #64304

Closed
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest
Closed

debugger: wait for target startup before initialization#64304
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest

Conversation

@Archkon

@ArchkonArchkon commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Trying to fix the test error that triggered at previous pr when run github action ci/cd

https://github.com/nodejs/node/actions/runs/28730483133/job/85195106458?pr=64301

Fixes: #64116
Fixes: #61762
Fixes: #64005

@nodejs-github-botnodejs-github-bot added debugger Issues and PRs related to the Node.js command-line debugger. needs-ci PRs that need a full CI run. labels Jul 5, 2026
@trivikr

Copy link
Copy Markdown
Member

Several previous attempts of deflaking were unsuccessful.
Please refer #64116 for prior discussions and links to other PRs.

@Archkon

This comment was marked as spam.

@Archkon
Archkon marked this pull request as draft July 6, 2026 05:26
@Archkon
Archkonforce-pushed the fixtest branch 3 times, most recently from 88fe3e7 to 1e6cde7CompareJuly 29, 2026 23:48
@ArchkonArchkon changed the title debugger: defer pause to avoid pause foreverdebugger: wait for target startup before initializationJul 29, 2026
@Archkon
Archkon marked this pull request as ready for review July 29, 2026 23:58
@codecov

codecovBot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.49123% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (c18fd90) to head (aa1cfd8).
⚠️ Report is 12 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/debugger/inspect_helpers.js95.23%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #64304 +/- ##
==========================================
- Coverage 90.31% 90.31% -0.01% 
==========================================
Files 759 759 Lines 247646 248350 +704 Branches 46697 46871 +174 ==========================================
+ Hits 223654 224288 +634 - Misses 15463 15473 +10 - Partials 8529 8589 +60 
Files with missing linesCoverage Δ
lib/internal/debugger/inspect_probe.js82.05% <100.00%> (-0.09%)⬇️
lib/internal/debugger/inspect_repl.js91.36% <100.00%> (+0.05%)⬆️
lib/internal/debugger/inspect_helpers.js97.00% <95.23%> (-0.41%)⬇️

... and 40 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.

@Archkon

This comment was marked as spam.

@avivkeller
avivkeller requested review from joyeecheung and trivikr and removed request for joyeecheungAugust 5, 2026 02:05
@Archkon

This comment was marked as spam.

@joyeecheung

Copy link
Copy Markdown
Member

I think at the minimum the commit should explain why the previous flakes happen, and why the change is supposed to fix it?

The debugger endpoint can accept a connection before the target has
entered its startup wait. In that window,Runtime.runIfWaitingForDebugger
may be handled too early, leaving the target waiting indefinitely.
Use NodeRuntime.waitingForDebugger as a readiness handshake before
initializing the debugger domains and releasing the target. Apply the
handshake to both the interactive debugger and probe mode,and reject the
wait if the session closes.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Debugger tests spawn a debugger client and target process, then wait for
asynchronous CLI output through a shared test helper.Under high parallel
load on macOS, process scheduling and inspector communication can delay
progress beyond the existing 15-second timeout, causing intermittent
failures across multiple debugger tests.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Run the debugger CLI in the per-test temporary directory so concurrent
runs do not overwrite or remove the same node.cpuprofile file.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@Archkon

This comment was marked as spam.

@Archkon
Archkonforce-pushed the fixtest branch 2 times, most recently from b85de2c to aa1cfd8CompareAugust 7, 2026 04:08
@Archkon

This comment was marked as spam.

Comment on lines +13 to +15
if (common.isMacOS) {
TIMEOUT = common.platformTimeout(30000);
} else if (common.isWindows) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon

Thanks for working on this!

Could you clarify why the timeout needs to be increased? If waitForDebugger() addresses the race condition, I would expect the timeout increase to be unnecessary.

Also, I think keeping the original timeout would make the stress test results more convincing, since otherwise it’s difficult to tell whether the improvement comes from waitForDebugger() or simply from the increased timeout.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks for clarifying. If Debugger.paused was emitted but not received by the JavaScript layer within 15 seconds, could you share a failing run or trace showing that?

Because the current stress runs include both changes, it is difficult to tell whether waitForDebugger() alone resolves the issue.

Comment on lines +15 to +19
const cli = startCLI(
[fixtures.path('debugger/empty.js')],
[],
{ cwd: tmpdir.path },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Could you remove this change from the PR?

It appears to be needed only for the --repeat 1000 -j16 stress run, where concurrent instances of this test share the same node.cpuprofile. It is unrelated to the debugger startup race and is not needed in the normal CI run.

If it is needed for stress validation, it can remain only in the stress-test branch or workflow.

This comment was marked as spam.

This comment was marked as spam.

@inoway46inoway46Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks. My concern is the PR scope, not the -j4 setting.

The linked run still executes this test 1000 times. Normal CI executes it only once, so the node.cpuprofile collision shown there is specific to the stress run. I think this change should be removed from this PR and kept only in the stress-test setup if needed.

Also, please leave this thread unresolved while this concern is still under discussion.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debuggerIssues and PRs related to the Node.js command-line debugger.needs-ciPRs that need a full CI run.

Projects

None yet

6 participants

@Archkon@trivikr@joyeecheung@inoway46@aduh95@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

debugger: wait for target startup before initialization - #64304

Closed
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest
Closed

debugger: wait for target startup before initialization#64304
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest

Conversation

@Archkon

@ArchkonArchkon commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Trying to fix the test error that triggered at previous pr when run github action ci/cd

https://github.com/nodejs/node/actions/runs/28730483133/job/85195106458?pr=64301

Fixes: #64116
Fixes: #61762
Fixes: #64005

@nodejs-github-botnodejs-github-bot added debugger Issues and PRs related to the Node.js command-line debugger. needs-ci PRs that need a full CI run. labels Jul 5, 2026
@trivikr

Copy link
Copy Markdown
Member

Several previous attempts of deflaking were unsuccessful.
Please refer #64116 for prior discussions and links to other PRs.

@Archkon

This comment was marked as spam.

@Archkon
Archkon marked this pull request as draft July 6, 2026 05:26
@Archkon
Archkonforce-pushed the fixtest branch 3 times, most recently from 88fe3e7 to 1e6cde7CompareJuly 29, 2026 23:48
@ArchkonArchkon changed the title debugger: defer pause to avoid pause foreverdebugger: wait for target startup before initializationJul 29, 2026
@Archkon
Archkon marked this pull request as ready for review July 29, 2026 23:58
@codecov

codecovBot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.49123% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (c18fd90) to head (aa1cfd8).
⚠️ Report is 12 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/debugger/inspect_helpers.js95.23%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #64304 +/- ##
==========================================
- Coverage 90.31% 90.31% -0.01% 
==========================================
Files 759 759 Lines 247646 248350 +704 Branches 46697 46871 +174 ==========================================
+ Hits 223654 224288 +634 - Misses 15463 15473 +10 - Partials 8529 8589 +60 
Files with missing linesCoverage Δ
lib/internal/debugger/inspect_probe.js82.05% <100.00%> (-0.09%)⬇️
lib/internal/debugger/inspect_repl.js91.36% <100.00%> (+0.05%)⬆️
lib/internal/debugger/inspect_helpers.js97.00% <95.23%> (-0.41%)⬇️

... and 40 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.

@Archkon

This comment was marked as spam.

@avivkeller
avivkeller requested review from joyeecheung and trivikr and removed request for joyeecheungAugust 5, 2026 02:05
@Archkon

This comment was marked as spam.

@joyeecheung

Copy link
Copy Markdown
Member

I think at the minimum the commit should explain why the previous flakes happen, and why the change is supposed to fix it?

The debugger endpoint can accept a connection before the target has
entered its startup wait. In that window,Runtime.runIfWaitingForDebugger
may be handled too early, leaving the target waiting indefinitely.
Use NodeRuntime.waitingForDebugger as a readiness handshake before
initializing the debugger domains and releasing the target. Apply the
handshake to both the interactive debugger and probe mode,and reject the
wait if the session closes.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Debugger tests spawn a debugger client and target process, then wait for
asynchronous CLI output through a shared test helper.Under high parallel
load on macOS, process scheduling and inspector communication can delay
progress beyond the existing 15-second timeout, causing intermittent
failures across multiple debugger tests.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Run the debugger CLI in the per-test temporary directory so concurrent
runs do not overwrite or remove the same node.cpuprofile file.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@Archkon

This comment was marked as spam.

@Archkon
Archkonforce-pushed the fixtest branch 2 times, most recently from b85de2c to aa1cfd8CompareAugust 7, 2026 04:08
@Archkon

This comment was marked as spam.

Comment on lines +13 to +15
if (common.isMacOS) {
TIMEOUT = common.platformTimeout(30000);
} else if (common.isWindows) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon

Thanks for working on this!

Could you clarify why the timeout needs to be increased? If waitForDebugger() addresses the race condition, I would expect the timeout increase to be unnecessary.

Also, I think keeping the original timeout would make the stress test results more convincing, since otherwise it’s difficult to tell whether the improvement comes from waitForDebugger() or simply from the increased timeout.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks for clarifying. If Debugger.paused was emitted but not received by the JavaScript layer within 15 seconds, could you share a failing run or trace showing that?

Because the current stress runs include both changes, it is difficult to tell whether waitForDebugger() alone resolves the issue.

Comment on lines +15 to +19
const cli = startCLI(
[fixtures.path('debugger/empty.js')],
[],
{ cwd: tmpdir.path },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Could you remove this change from the PR?

It appears to be needed only for the --repeat 1000 -j16 stress run, where concurrent instances of this test share the same node.cpuprofile. It is unrelated to the debugger startup race and is not needed in the normal CI run.

If it is needed for stress validation, it can remain only in the stress-test branch or workflow.

This comment was marked as spam.

This comment was marked as spam.

@inoway46inoway46Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks. My concern is the PR scope, not the -j4 setting.

The linked run still executes this test 1000 times. Normal CI executes it only once, so the node.cpuprofile collision shown there is specific to the stress run. I think this change should be removed from this PR and kept only in the stress-test setup if needed.

Also, please leave this thread unresolved while this concern is still under discussion.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debuggerIssues and PRs related to the Node.js command-line debugger.needs-ciPRs that need a full CI run.

Projects

None yet

6 participants

@Archkon@trivikr@joyeecheung@inoway46@aduh95@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

debugger: wait for target startup before initialization - #64304

Closed
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest
Closed

debugger: wait for target startup before initialization#64304
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest

Conversation

@Archkon

@ArchkonArchkon commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Trying to fix the test error that triggered at previous pr when run github action ci/cd

https://github.com/nodejs/node/actions/runs/28730483133/job/85195106458?pr=64301

Fixes: #64116
Fixes: #61762
Fixes: #64005

@nodejs-github-botnodejs-github-bot added debugger Issues and PRs related to the Node.js command-line debugger. needs-ci PRs that need a full CI run. labels Jul 5, 2026
@trivikr

Copy link
Copy Markdown
Member

Several previous attempts of deflaking were unsuccessful.
Please refer #64116 for prior discussions and links to other PRs.

@Archkon

This comment was marked as spam.

@Archkon
Archkon marked this pull request as draft July 6, 2026 05:26
@Archkon
Archkonforce-pushed the fixtest branch 3 times, most recently from 88fe3e7 to 1e6cde7CompareJuly 29, 2026 23:48
@ArchkonArchkon changed the title debugger: defer pause to avoid pause foreverdebugger: wait for target startup before initializationJul 29, 2026
@Archkon
Archkon marked this pull request as ready for review July 29, 2026 23:58
@codecov

codecovBot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.49123% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (c18fd90) to head (aa1cfd8).
⚠️ Report is 12 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/debugger/inspect_helpers.js95.23%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #64304 +/- ##
==========================================
- Coverage 90.31% 90.31% -0.01% 
==========================================
Files 759 759 Lines 247646 248350 +704 Branches 46697 46871 +174 ==========================================
+ Hits 223654 224288 +634 - Misses 15463 15473 +10 - Partials 8529 8589 +60 
Files with missing linesCoverage Δ
lib/internal/debugger/inspect_probe.js82.05% <100.00%> (-0.09%)⬇️
lib/internal/debugger/inspect_repl.js91.36% <100.00%> (+0.05%)⬆️
lib/internal/debugger/inspect_helpers.js97.00% <95.23%> (-0.41%)⬇️

... and 40 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.

@Archkon

This comment was marked as spam.

@avivkeller
avivkeller requested review from joyeecheung and trivikr and removed request for joyeecheungAugust 5, 2026 02:05
@Archkon

This comment was marked as spam.

@joyeecheung

Copy link
Copy Markdown
Member

I think at the minimum the commit should explain why the previous flakes happen, and why the change is supposed to fix it?

The debugger endpoint can accept a connection before the target has
entered its startup wait. In that window,Runtime.runIfWaitingForDebugger
may be handled too early, leaving the target waiting indefinitely.
Use NodeRuntime.waitingForDebugger as a readiness handshake before
initializing the debugger domains and releasing the target. Apply the
handshake to both the interactive debugger and probe mode,and reject the
wait if the session closes.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Debugger tests spawn a debugger client and target process, then wait for
asynchronous CLI output through a shared test helper.Under high parallel
load on macOS, process scheduling and inspector communication can delay
progress beyond the existing 15-second timeout, causing intermittent
failures across multiple debugger tests.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Run the debugger CLI in the per-test temporary directory so concurrent
runs do not overwrite or remove the same node.cpuprofile file.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@Archkon

This comment was marked as spam.

@Archkon
Archkonforce-pushed the fixtest branch 2 times, most recently from b85de2c to aa1cfd8CompareAugust 7, 2026 04:08
@Archkon

This comment was marked as spam.

Comment on lines +13 to +15
if (common.isMacOS) {
TIMEOUT = common.platformTimeout(30000);
} else if (common.isWindows) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon

Thanks for working on this!

Could you clarify why the timeout needs to be increased? If waitForDebugger() addresses the race condition, I would expect the timeout increase to be unnecessary.

Also, I think keeping the original timeout would make the stress test results more convincing, since otherwise it’s difficult to tell whether the improvement comes from waitForDebugger() or simply from the increased timeout.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks for clarifying. If Debugger.paused was emitted but not received by the JavaScript layer within 15 seconds, could you share a failing run or trace showing that?

Because the current stress runs include both changes, it is difficult to tell whether waitForDebugger() alone resolves the issue.

Comment on lines +15 to +19
const cli = startCLI(
[fixtures.path('debugger/empty.js')],
[],
{ cwd: tmpdir.path },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Could you remove this change from the PR?

It appears to be needed only for the --repeat 1000 -j16 stress run, where concurrent instances of this test share the same node.cpuprofile. It is unrelated to the debugger startup race and is not needed in the normal CI run.

If it is needed for stress validation, it can remain only in the stress-test branch or workflow.

This comment was marked as spam.

This comment was marked as spam.

@inoway46inoway46Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks. My concern is the PR scope, not the -j4 setting.

The linked run still executes this test 1000 times. Normal CI executes it only once, so the node.cpuprofile collision shown there is specific to the stress run. I think this change should be removed from this PR and kept only in the stress-test setup if needed.

Also, please leave this thread unresolved while this concern is still under discussion.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debuggerIssues and PRs related to the Node.js command-line debugger.needs-ciPRs that need a full CI run.

Projects

None yet

6 participants

@Archkon@trivikr@joyeecheung@inoway46@aduh95@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

debugger: wait for target startup before initialization - #64304

Closed
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest
Closed

debugger: wait for target startup before initialization#64304
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest

Conversation

@Archkon

@ArchkonArchkon commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Trying to fix the test error that triggered at previous pr when run github action ci/cd

https://github.com/nodejs/node/actions/runs/28730483133/job/85195106458?pr=64301

Fixes: #64116
Fixes: #61762
Fixes: #64005

@nodejs-github-botnodejs-github-bot added debugger Issues and PRs related to the Node.js command-line debugger. needs-ci PRs that need a full CI run. labels Jul 5, 2026
@trivikr

Copy link
Copy Markdown
Member

Several previous attempts of deflaking were unsuccessful.
Please refer #64116 for prior discussions and links to other PRs.

@Archkon

This comment was marked as spam.

@Archkon
Archkon marked this pull request as draft July 6, 2026 05:26
@Archkon
Archkonforce-pushed the fixtest branch 3 times, most recently from 88fe3e7 to 1e6cde7CompareJuly 29, 2026 23:48
@ArchkonArchkon changed the title debugger: defer pause to avoid pause foreverdebugger: wait for target startup before initializationJul 29, 2026
@Archkon
Archkon marked this pull request as ready for review July 29, 2026 23:58
@codecov

codecovBot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.49123% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (c18fd90) to head (aa1cfd8).
⚠️ Report is 12 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/debugger/inspect_helpers.js95.23%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #64304 +/- ##
==========================================
- Coverage 90.31% 90.31% -0.01% 
==========================================
Files 759 759 Lines 247646 248350 +704 Branches 46697 46871 +174 ==========================================
+ Hits 223654 224288 +634 - Misses 15463 15473 +10 - Partials 8529 8589 +60 
Files with missing linesCoverage Δ
lib/internal/debugger/inspect_probe.js82.05% <100.00%> (-0.09%)⬇️
lib/internal/debugger/inspect_repl.js91.36% <100.00%> (+0.05%)⬆️
lib/internal/debugger/inspect_helpers.js97.00% <95.23%> (-0.41%)⬇️

... and 40 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.

@Archkon

This comment was marked as spam.

@avivkeller
avivkeller requested review from joyeecheung and trivikr and removed request for joyeecheungAugust 5, 2026 02:05
@Archkon

This comment was marked as spam.

@joyeecheung

Copy link
Copy Markdown
Member

I think at the minimum the commit should explain why the previous flakes happen, and why the change is supposed to fix it?

The debugger endpoint can accept a connection before the target has
entered its startup wait. In that window,Runtime.runIfWaitingForDebugger
may be handled too early, leaving the target waiting indefinitely.
Use NodeRuntime.waitingForDebugger as a readiness handshake before
initializing the debugger domains and releasing the target. Apply the
handshake to both the interactive debugger and probe mode,and reject the
wait if the session closes.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Debugger tests spawn a debugger client and target process, then wait for
asynchronous CLI output through a shared test helper.Under high parallel
load on macOS, process scheduling and inspector communication can delay
progress beyond the existing 15-second timeout, causing intermittent
failures across multiple debugger tests.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Run the debugger CLI in the per-test temporary directory so concurrent
runs do not overwrite or remove the same node.cpuprofile file.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@Archkon

This comment was marked as spam.

@Archkon
Archkonforce-pushed the fixtest branch 2 times, most recently from b85de2c to aa1cfd8CompareAugust 7, 2026 04:08
@Archkon

This comment was marked as spam.

Comment on lines +13 to +15
if (common.isMacOS) {
TIMEOUT = common.platformTimeout(30000);
} else if (common.isWindows) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon

Thanks for working on this!

Could you clarify why the timeout needs to be increased? If waitForDebugger() addresses the race condition, I would expect the timeout increase to be unnecessary.

Also, I think keeping the original timeout would make the stress test results more convincing, since otherwise it’s difficult to tell whether the improvement comes from waitForDebugger() or simply from the increased timeout.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks for clarifying. If Debugger.paused was emitted but not received by the JavaScript layer within 15 seconds, could you share a failing run or trace showing that?

Because the current stress runs include both changes, it is difficult to tell whether waitForDebugger() alone resolves the issue.

Comment on lines +15 to +19
const cli = startCLI(
[fixtures.path('debugger/empty.js')],
[],
{ cwd: tmpdir.path },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Could you remove this change from the PR?

It appears to be needed only for the --repeat 1000 -j16 stress run, where concurrent instances of this test share the same node.cpuprofile. It is unrelated to the debugger startup race and is not needed in the normal CI run.

If it is needed for stress validation, it can remain only in the stress-test branch or workflow.

This comment was marked as spam.

This comment was marked as spam.

@inoway46inoway46Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks. My concern is the PR scope, not the -j4 setting.

The linked run still executes this test 1000 times. Normal CI executes it only once, so the node.cpuprofile collision shown there is specific to the stress run. I think this change should be removed from this PR and kept only in the stress-test setup if needed.

Also, please leave this thread unresolved while this concern is still under discussion.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debuggerIssues and PRs related to the Node.js command-line debugger.needs-ciPRs that need a full CI run.

Projects

None yet

6 participants

@Archkon@trivikr@joyeecheung@inoway46@aduh95@nodejs-github-bot
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

debugger: wait for target startup before initialization - #64304

Closed
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest
Closed

debugger: wait for target startup before initialization#64304
Archkon wants to merge 3 commits into
nodejs:mainfrom
Archkon:fixtest

Conversation

@Archkon

@ArchkonArchkon commented Jul 5, 2026

Copy link
Copy Markdown
Contributor

Trying to fix the test error that triggered at previous pr when run github action ci/cd

https://github.com/nodejs/node/actions/runs/28730483133/job/85195106458?pr=64301

Fixes: #64116
Fixes: #61762
Fixes: #64005

@nodejs-github-botnodejs-github-bot added debugger Issues and PRs related to the Node.js command-line debugger. needs-ci PRs that need a full CI run. labels Jul 5, 2026
@trivikr

Copy link
Copy Markdown
Member

Several previous attempts of deflaking were unsuccessful.
Please refer #64116 for prior discussions and links to other PRs.

@Archkon

This comment was marked as spam.

@Archkon
Archkon marked this pull request as draft July 6, 2026 05:26
@Archkon
Archkonforce-pushed the fixtest branch 3 times, most recently from 88fe3e7 to 1e6cde7CompareJuly 29, 2026 23:48
@ArchkonArchkon changed the title debugger: defer pause to avoid pause foreverdebugger: wait for target startup before initializationJul 29, 2026
@Archkon
Archkon marked this pull request as ready for review July 29, 2026 23:58
@codecov

codecovBot commented Jul 30, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.49123% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.31%. Comparing base (c18fd90) to head (aa1cfd8).
⚠️ Report is 12 commits behind head on main.

Files with missing linesPatch %Lines
lib/internal/debugger/inspect_helpers.js95.23%2 Missing ⚠️
Additional details and impacted files
@@ Coverage Diff @@## main #64304 +/- ##
==========================================
- Coverage 90.31% 90.31% -0.01% 
==========================================
Files 759 759 Lines 247646 248350 +704 Branches 46697 46871 +174 ==========================================
+ Hits 223654 224288 +634 - Misses 15463 15473 +10 - Partials 8529 8589 +60 
Files with missing linesCoverage Δ
lib/internal/debugger/inspect_probe.js82.05% <100.00%> (-0.09%)⬇️
lib/internal/debugger/inspect_repl.js91.36% <100.00%> (+0.05%)⬆️
lib/internal/debugger/inspect_helpers.js97.00% <95.23%> (-0.41%)⬇️

... and 40 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.

@Archkon

This comment was marked as spam.

@avivkeller
avivkeller requested review from joyeecheung and trivikr and removed request for joyeecheungAugust 5, 2026 02:05
@Archkon

This comment was marked as spam.

@joyeecheung

Copy link
Copy Markdown
Member

I think at the minimum the commit should explain why the previous flakes happen, and why the change is supposed to fix it?

The debugger endpoint can accept a connection before the target has
entered its startup wait. In that window,Runtime.runIfWaitingForDebugger
may be handled too early, leaving the target waiting indefinitely.
Use NodeRuntime.waitingForDebugger as a readiness handshake before
initializing the debugger domains and releasing the target. Apply the
handshake to both the interactive debugger and probe mode,and reject the
wait if the session closes.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Debugger tests spawn a debugger client and target process, then wait for
asynchronous CLI output through a shared test helper.Under high parallel
load on macOS, process scheduling and inspector communication can delay
progress beyond the existing 15-second timeout, causing intermittent
failures across multiple debugger tests.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
Run the debugger CLI in the per-test temporary directory so concurrent
runs do not overwrite or remove the same node.cpuprofile file.
Signed-off-by: Archkon <180910180+Archkon@users.noreply.github.com>
@Archkon

This comment was marked as spam.

@Archkon
Archkonforce-pushed the fixtest branch 2 times, most recently from b85de2c to aa1cfd8CompareAugust 7, 2026 04:08
@Archkon

This comment was marked as spam.

Comment on lines +13 to +15
if (common.isMacOS) {
TIMEOUT = common.platformTimeout(30000);
} else if (common.isWindows) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon

Thanks for working on this!

Could you clarify why the timeout needs to be increased? If waitForDebugger() addresses the race condition, I would expect the timeout increase to be unnecessary.

Also, I think keeping the original timeout would make the stress test results more convincing, since otherwise it’s difficult to tell whether the improvement comes from waitForDebugger() or simply from the increased timeout.

This comment was marked as spam.

This comment was marked as spam.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks for clarifying. If Debugger.paused was emitted but not received by the JavaScript layer within 15 seconds, could you share a failing run or trace showing that?

Because the current stress runs include both changes, it is difficult to tell whether waitForDebugger() alone resolves the issue.

Comment on lines +15 to +19
const cli = startCLI(
[fixtures.path('debugger/empty.js')],
[],
{ cwd: tmpdir.path },
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Could you remove this change from the PR?

It appears to be needed only for the --repeat 1000 -j16 stress run, where concurrent instances of this test share the same node.cpuprofile. It is unrelated to the debugger startup race and is not needed in the normal CI run.

If it is needed for stress validation, it can remain only in the stress-test branch or workflow.

This comment was marked as spam.

This comment was marked as spam.

@inoway46inoway46Aug 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Archkon
Thanks. My concern is the PR scope, not the -j4 setting.

The linked run still executes this test 1000 times. Normal CI executes it only once, so the node.cpuprofile collision shown there is specific to the stress run. I think this change should be removed from this PR and kept only in the stress-test setup if needed.

Also, please leave this thread unresolved while this concern is still under discussion.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

debuggerIssues and PRs related to the Node.js command-line debugger.needs-ciPRs that need a full CI run.

Projects

None yet

6 participants

@Archkon@trivikr@joyeecheung@inoway46@aduh95@nodejs-github-bot