Skip to content

debugger: wait for target startup before initialization #64304

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Archkon wants to merge 3 commits into from
Closed

debugger: wait for target startup before initialization #64304

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2cc24e5
debugger: wait for target startup before initialization
Archkon Jul 29, 2026
c31b790
test: extend debugger startup timeout on macOS
Archkon Jul 29, 2026
aa1cfd8
test: isolate debugger CPU profile output
Archkon Jul 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 42 additions & 1 deletion lib/internal/debugger/inspect_helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ const {
ArrayPrototypePushApply,
Number,
Promise,
PromiseWithResolvers,
RegExpPrototypeExec,
SafePromiseRace,
StringPrototypeEndsWith,
} = primordials;

Expand All @@ -18,7 +20,10 @@ const {
AbortController,
} = require('internal/abort_controller');

const { ERR_DEBUGGER_STARTUP_ERROR } = require('internal/errors').codes;
const {
ERR_DEBUGGER_ERROR,
ERR_DEBUGGER_STARTUP_ERROR,
} = require('internal/errors').codes;
const {
exitCodes: {
kInvalidCommandLineArgument,
Expand Down Expand Up @@ -61,6 +66,41 @@ function ensureTrailingNewline(text) {
return StringPrototypeEndsWith(text, '\n') ? text : `${text}\n`;
}

async function waitForDebugger(
client,
callMethod = (method) => client.callMethod(method),
) {
const {
promise: waitingPromise,
resolve: resolveWaiting,
} = PromiseWithResolvers();
const {
promise: closedPromise,
reject: rejectClosed,
} = PromiseWithResolvers();
const onWaiting = () => resolveWaiting();
const onClose = () => {
rejectClosed(new ERR_DEBUGGER_ERROR(
'Debugger session ended while waiting for target startup'));
};

client.once('NodeRuntime.waitingForDebugger', onWaiting);
client.once('close', onClose);
try {
await SafePromiseRace([
callMethod('NodeRuntime.enable'),
closedPromise,
]);
await SafePromiseRace([
waitingPromise,
closedPromise,
]);
} finally {
client.removeListener('NodeRuntime.waitingForDebugger', onWaiting);
client.removeListener('close', onClose);
}
}

function writeInspectUsageAndExit(invokedAs, message, exitCode) {
const code = exitCode ?? (message ? kInvalidCommandLineArgument : 0);
const out = code === 0 ? process.stdout : process.stderr;
Expand Down Expand Up @@ -189,5 +229,6 @@ async function launchChildProcess(childArgs, inspectHost, inspectPort,
module.exports = {
ensureTrailingNewline,
launchChildProcess,
waitForDebugger,
writeInspectUsageAndExit,
};
6 changes: 6 additions & 0 deletions lib/internal/debugger/inspect_probe.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ const InspectClient = require('internal/debugger/inspect_client');
const {
ensureTrailingNewline,
launchChildProcess,
waitForDebugger,
} = require('internal/debugger/inspect_helpers');

const { ERR_DEBUGGER_STARTUP_ERROR } = require('internal/errors').codes;
Expand Down Expand Up @@ -1044,11 +1045,16 @@ class ProbeInspectorSession {
this.connected = true;

try {
await waitForDebugger(
this.client,
(method) => this.callCdp(method),
);
await this.callCdp('Runtime.enable');
await this.callCdp('Debugger.enable');
await this.bindBreakpoints();
this.started = true;
this.startTimeout();
await this.callCdp('NodeRuntime.disable');
await this.callCdp('Runtime.runIfWaitingForDebugger');
} catch (err) {
if (err !== kInspectorFailedSentinel) { throw err; }
Expand Down
10 changes: 9 additions & 1 deletion lib/internal/debugger/inspect_repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ const { fileURLToPath } = require('internal/url');
const { customInspectSymbol, SideEffectFreeRegExpPrototypeSymbolReplace } = require('internal/util');
const { inspect: utilInspect } = require('internal/util/inspect');
const { isObjectLiteral } = require('internal/repl/utils');
const { waitForDebugger } = require('internal/debugger/inspect_helpers');
const debuglog = require('internal/util/debuglog').debuglog('inspect');

const SHORTCUTS = {
Expand Down Expand Up @@ -1204,9 +1205,13 @@ function createRepl(inspector) {
}

async function initAfterStart() {
const waitForDebuggerOnStart = !!inspector.options?.script;
waitForInitialBreakRender =
!!inspector.options?.script &&
waitForDebuggerOnStart &&
process.env.NODE_INSPECT_RESUME_ON_START !== '1';
if (waitForDebuggerOnStart) {
await waitForDebugger(inspector.client);
}
await Runtime.enable();
await Profiler.enable();
await Profiler.setSamplingInterval({ interval: 100 });
Expand All @@ -1215,6 +1220,9 @@ function createRepl(inspector) {
await Debugger.setBlackboxPatterns({ patterns: [] });
await Debugger.setPauseOnExceptions({ state: pauseOnExceptionState });
await restoreBreakpoints();
if (waitForDebuggerOnStart) {
await inspector.client.callMethod('NodeRuntime.disable');
}
await Runtime.runIfWaitingForDebugger();
await PromiseResolve();
waitForInitialBreakRender = false;
Expand Down
4 changes: 3 additions & 1 deletion test/common/debugger.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,9 @@ const BREAK_MESSAGE = new RegExp('(?:' + [
let TIMEOUT = common.platformTimeout(10000);
// Some macOS and Windows machines require more time to receive the outputs from the client.
// https://github.com/nodejs/build/issues/3014
if (common.isWindows || common.isMacOS) {
if (common.isMacOS) {
TIMEOUT = common.platformTimeout(30000);
} else if (common.isWindows) {
Comment on lines +13 to +15

@inoway46 inoway46 Aug 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 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.

Sign in to view

This comment was marked as spam.

Sign in to view

@inoway46 inoway46 Aug 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 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.

TIMEOUT = common.platformTimeout(15000);
}

Expand Down
12 changes: 8 additions & 4 deletions test/parallel/test-debugger-profile-command.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,18 @@ common.skipIfInspectorDisabled();

const fixtures = require('../common/fixtures');
const startCLI = require('../common/debugger');
const tmpdir = require('../common/tmpdir');

const assert = require('assert');
const fs = require('fs');
const path = require('path');

const cli = startCLI([fixtures.path('debugger/empty.js')]);
tmpdir.refresh();

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

@inoway46 inoway46 Aug 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
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.

Sign in to view

This comment was marked as spam.

Sign in to view

@inoway46 inoway46 Aug 9, 2026 •
edited
Loading

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.


(async () => {
await cli.waitForInitialBreak();
Expand All @@ -25,7 +29,7 @@ const rootDir = path.resolve(__dirname, '..', '..');
await cli.command('profiles[0].save()');
assert.match(cli.output, /Saved profile to .*node\.cpuprofile/);

const cpuprofile = path.resolve(rootDir, 'node.cpuprofile');
const cpuprofile = tmpdir.resolve('node.cpuprofile');
const data = JSON.parse(fs.readFileSync(cpuprofile, 'utf8'));
assert.strictEqual(Array.isArray(data.nodes), true);

Expand Down
43 changes: 42 additions & 1 deletion test/parallel/test-debugger-run-restart-init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,27 @@ async function assertCommandWaitsForInit(repl, command, gate, calls) {
const runGate = createGate();
const restartGate = createGate();
const gates = [null, runGate, restartGate];
const client = new EventEmitter();
let nodeRuntimeEnableCount = 0;
client.callMethod = async (method) => {
calls.push(method);
if (method === 'NodeRuntime.enable') {
const emitWaiting = () => {
calls.push('NodeRuntime.waitingForDebugger');
client.emit('NodeRuntime.waitingForDebugger');
};
// Cover notifications arriving both before and after the enable reply.
if (nodeRuntimeEnableCount++ % 2 === 0) {
emitWaiting();
} else {
setImmediate(emitWaiting);
}
}
};
const inspector = {
client: new EventEmitter(),
client,
domainNames: ['Debugger', 'HeapProfiler', 'Profiler', 'Runtime'],
options: { script: 'debugger-target.js' },
stdin: new PassThrough(),
stdout: new PassThrough(),
run: common.mustCall(async () => {
Expand All @@ -101,6 +119,29 @@ async function assertCommandWaitsForInit(repl, command, gate, calls) {
await assertCommandWaitsForInit(repl, 'run', runGate, calls);
await assertCommandWaitsForInit(repl, 'restart', restartGate, calls);

assert.deepStrictEqual(
calls.filter((call) => (
call === 'NodeRuntime.enable' ||
call === 'NodeRuntime.waitingForDebugger' ||
call === 'NodeRuntime.disable' ||
call === 'Runtime.runIfWaitingForDebugger'
)),
[
'NodeRuntime.enable',
'NodeRuntime.waitingForDebugger',
'NodeRuntime.disable',
'Runtime.runIfWaitingForDebugger',
'NodeRuntime.enable',
'NodeRuntime.waitingForDebugger',
'NodeRuntime.disable',
'Runtime.runIfWaitingForDebugger',
'NodeRuntime.enable',
'NodeRuntime.waitingForDebugger',
'NodeRuntime.disable',
'Runtime.runIfWaitingForDebugger',
],
);

assert.deepStrictEqual(
calls.filter((call) => (
call === 'inspector.run' ||
Expand Down
Loading