Commit d9f4e8e
test: fix flaky test-watch-mode-inspect timeout
This test randomly times out (~120s) on CI due to a race condition
between child-process restart (triggered by touching the watched file)
and the second inspector-session connection.
The old code used an interval-based restart (write every 500ms) and
a 'gettingDebuggedPid' flag to pause writes during a session. This
still left a race window where getDebuggedPid() would attempt to
connect the inspector via HTTP GET /json/list + WebSocket upgrade
either before the new child was ready (empty target list) or after
the old session was being destroyed, causing the promise to hang.
Fix: Replace the interval with a single write that triggers exactly
one restart, then wait for the restarted child's 'safe to debug now'
stdout line before connecting the second inspector session. This
eliminates the race by ensuring the new child process and its
inspector session are fully ready before any connection attempt.
Removes the now-unused gettingDebuggedPid flag and the pending
setTimeout delay that was needed as a backstop for the interval.
Fixes: #44898
Signed-off-by: Matteo Collina <hello@matteocollina.com>
PR-URL: #63361
Reviewed-By: Moshe Atlow <moshe@atlow.co.il>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>1 parent df183d7 commit d9f4e8e
1 file changed
Lines changed: 12 additions & 15 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
@@ -3,7 +3,6 @@ import * as fixtures from '../common/fixtures.mjs'; | |||
| 3 | 3 | import assert from 'node:assert'; | |
| 4 | 4 | import { describe, it } from 'node:test'; | |
| 5 | 5 | import { writeFileSync, readFileSync } from 'node:fs'; | |
| 6 | - import { setTimeout } from 'node:timers/promises'; | ||
| 7 | 6 | import { NodeInstance } from '../common/inspector-helper.js'; | |
| 8 | 7 | ||
| 9 | 8 | ||
@@ -12,10 +11,7 @@ if (common.isIBMi) | |||
| 12 | 11 | ||
| 13 | 12 | common.skipIfInspectorDisabled(); | |
| 14 | 13 | ||
| 15 | - let gettingDebuggedPid = false; | ||
| 16 | - | ||
| 17 | 14 | async function getDebuggedPid(instance, waitForLog = true) { | |
| 18 | - gettingDebuggedPid = true; | ||
| 19 | 15 | const session = await instance.connectInspectorSession(); | |
| 20 | 16 | await session.send({ method: 'Runtime.enable' }); | |
| 21 | 17 | if (waitForLog) { | |
@@ -25,20 +21,23 @@ async function getDebuggedPid(instance, waitForLog = true) { | |||
| 25 | 21 | 'method': 'Runtime.evaluate', 'params': { 'expression': 'process.pid' }, | |
| 26 | 22 | })).result; | |
| 27 | 23 | session.disconnect(); | |
| 28 | - gettingDebuggedPid = false; | ||
| 29 | 24 | return innerPid; | |
| 30 | 25 | } | |
| 31 | 26 | ||
| 32 | - function restart(file) { | ||
| 27 | + // Triggers a single restart and resolves when the restarted child prints "safe to debug now". | ||
| 28 | + function restartAndWaitForReady(file, instance) { | ||
| 29 | + const ready = new Promise((resolve) => { | ||
| 30 | + instance.on('stdout', (data) => { | ||
| 31 | + if (data?.includes('safe to debug now')) { | ||
| 32 | + resolve(); | ||
| 33 | + } | ||
| 34 | + }); | ||
| 35 | + }); | ||
| 33 | 36 | writeFileSync(file, readFileSync(file)); | |
| 34 | - const interval = setInterval(() => { | ||
| 35 | - if (!gettingDebuggedPid) { | ||
| 36 | - writeFileSync(file, readFileSync(file)); | ||
| 37 | - } | ||
| 38 | - }, common.platformTimeout(500)); | ||
| 39 | - return () => clearInterval(interval); | ||
| 37 | + return ready; | ||
| 40 | 38 | } | |
| 41 | 39 | ||
| 40 | + | ||
| 42 | 41 | describe('watch mode - inspect', () => { | |
| 43 | 42 | it('should start debugger on inner process', async () => { | |
| 44 | 43 | const file = fixtures.path('watch-mode/inspect.js'); | |
@@ -51,11 +50,9 @@ describe('watch mode - inspect', () => { | |||
| 51 | 50 | const pids = [instance.pid]; | |
| 52 | 51 | pids.push(await getDebuggedPid(instance)); | |
| 53 | 52 | instance.resetPort(); | |
| 54 | - const stopRestarting = restart(file); | ||
| 53 | + await restartAndWaitForReady(file, instance); | ||
| 55 | 54 | pids.push(await getDebuggedPid(instance)); | |
| 56 | - stopRestarting(); | ||
| 57 | 55 | ||
| 58 | - await setTimeout(common.platformTimeout(500)); | ||
| 59 | 56 | await instance.kill(); | |
| 60 | 57 | ||
| 61 | 58 | // There should be a process per restart and one per parent process. | |
0 commit comments