[BUG] Node 20: Test timeouts do not exit gracefully

[gtm-nayan]

System info

• Playwright Version: v1.32.3
• Operating System: Ubuntu 22
• Browser: Chromium, Firefox
• Other info:

Source code

• I provided exact source code that allows reproducing the issue locally.

// playwright.config.ts
import { defineConfig, devices } from '@playwright/test';

export default defineConfig({
  webServer: {
    command: 'node webserver.js',
    port: 4173,
  },
  projects: [
    {
      name: 'chromium',
      use: { ...devices['Desktop Chrome'], },
    },
  ]
});

// webserver.js
import { createServer } from 'node:http';
createServer((req, res) => {}).listen(4173);

Test file (self-contained)

it('should check the box using setChecked', async ({ page }) => {
  await new Promise(r => setTimeout(r, 25000));
});

Steps

• Run the test

Expected
test failure is reported, webserver is closed

Actual
exits without any message, test remains unmarked, webserver remains open

[gtm-nayan]

Narrowed it down to a node bug nodejs/node#47566 that's been fixed in nightly with nodejs/node#47620

Going to keep this issue open because

playwright/packages/playwright-test/src/cli.ts

Lines 265 to 268 in 44e56d2

	innerProcess.on('close', (code: number | null) => {
	if (code !== 0 && code !== null)
	process.exit(code);
	});

should likely be adjusted to make it easier to diagnose any similar regressions in the future and to exit any other child processes started by the cli (eg. webserver).

[dgozman]

This specific place does not really spawn child processes like webserver or test worker. It restarts itself with some more options, and mirrors the exit. I don't see what should we do differently here.

@gtm-nayan Do you have a particular improvement in mind?

[aslushnikov]

This is actually working as intended; if the inner process crashes here, than the program is in invalid state. Killing the whole process tree would only hide the behavior under the carpet.

Since we already exit with a non-zero code, the error should be surfaced well-enough for us to debug later.

I'll close this since we believe the current behavior is optimal.

[gtm-nayan]

This specific place does not really spawn child processes like webserver or test worker.

Ahh, sorry I have misunderstood that part then.

There's a race condition somewhere still, sometimes the worker process will receive a SIGKILL, it'll log that the worker process exited unexpectedly, the webserver will get cleaned up and that forked process will exit with a 1 code.

Other times, the forked process from the linked snippet will be closed before anything is logged. The close event handler receives a code of null and a SIGKILL signal. And in that case, the webserver will leak as well.

The setup with the loop from #22643 is more reliable in reproducing that case.