fix(test runner): regular worker termination finishes long fixtures (#…

…30769)

Previously, terminating worker always had a 30 seconds force exit.

Now, regular worker termination assumes that process will eventually
finish tearing down all the fixtures and exits. However, the
self-destruction routine keeps the 30 seconds timeout to avoid zombies.

Fixes #30504.

packages/playwright/src/common/process.ts

@@ -44,11 +44,12 @@ export class ProcessRunner {
   }
 }
 
-let closed = false;
+let gracefullyCloseCalled = false;
+let forceExitInitiated = false;
 
 sendMessageToParent({ method: 'ready' });
 
-process.on('disconnect', gracefullyCloseAndExit);
+process.on('disconnect', () => gracefullyCloseAndExit(true));
 process.on('SIGINT', () => {});
 process.on('SIGTERM', () => {});
 
@@ -76,7 +77,7 @@ process.on('message', async (message: any) => {
     const keys = new Set([...Object.keys(process.env), ...Object.keys(startingEnv)]);
     const producedEnv: EnvProducedPayload = [...keys].filter(key => startingEnv[key] !== process.env[key]).map(key => [key, process.env[key] ?? null]);
     sendMessageToParent({ method: '__env_produced__', params: producedEnv });
-    await gracefullyCloseAndExit();
+    await gracefullyCloseAndExit(false);
     return;
   }
   if (message.method === '__dispatch__') {
@@ -92,19 +93,24 @@ process.on('message', async (message: any) => {
   }
 });
 
-async function gracefullyCloseAndExit() {
-  if (closed)
-    return;
-  closed = true;
-  // Force exit after 30 seconds.
-  // eslint-disable-next-line no-restricted-properties
-  setTimeout(() => process.exit(0), 30000);
-  // Meanwhile, try to gracefully shutdown.
-  await processRunner?.gracefullyClose().catch(() => {});
-  if (processName)
-    await stopProfiling(processName).catch(() => {});
-  // eslint-disable-next-line no-restricted-properties
-  process.exit(0);
+const kForceExitTimeout = +(process.env.PWTEST_FORCE_EXIT_TIMEOUT || 30000);
+
+async function gracefullyCloseAndExit(forceExit: boolean) {
+  if (forceExit && !forceExitInitiated) {
+    forceExitInitiated = true;
+    // Force exit after 30 seconds.
+    // eslint-disable-next-line no-restricted-properties
+    setTimeout(() => process.exit(0), kForceExitTimeout);
+  }
+  if (!gracefullyCloseCalled) {
+    gracefullyCloseCalled = true;
+    // Meanwhile, try to gracefully shutdown.
+    await processRunner?.gracefullyClose().catch(() => {});
+    if (processName)
+      await stopProfiling(processName).catch(() => {});
+    // eslint-disable-next-line no-restricted-properties
+    process.exit(0);
+  }
 }
 
 function sendMessageToParent(message: { method: string, params?: any }) {

packages/playwright/src/worker/workerMain.ts

@@ -100,12 +100,17 @@ export class WorkerMain extends ProcessRunner {
   override async gracefullyClose() {
     try {
       await this._stop();
+      // Ignore top-level errors, they are already inside TestInfo.errors.
+      const fakeTestInfo = new TestInfoImpl(this._config, this._project, this._params, undefined, 0, () => {}, () => {}, () => {});
+      const runnable = { type: 'teardown' } as const;
       // We have to load the project to get the right deadline below.
-      await this._loadIfNeeded();
-      await this._teardownScopes();
+      await fakeTestInfo._runAsStage({ title: 'worker cleanup', runnable }, () => this._loadIfNeeded()).catch(() => {});
+      await this._fixtureRunner.teardownScope('test', fakeTestInfo, runnable).catch(() => {});
+      await this._fixtureRunner.teardownScope('worker', fakeTestInfo, runnable).catch(() => {});
       // Close any other browsers launched in this process. This includes anything launched
       // manually in the test/hooks and internal browsers like Playwright Inspector.
-      await gracefullyCloseAll();
+      await fakeTestInfo._runAsStage({ title: 'worker cleanup', runnable }, () => gracefullyCloseAll()).catch(() => {});
+      this._fatalErrors.push(...fakeTestInfo.errors);
     } catch (e) {
       this._fatalErrors.push(serializeError(e));
     }
@@ -144,15 +149,6 @@ export class WorkerMain extends ProcessRunner {
     }
   }
 
-  private async _teardownScopes() {
-    const fakeTestInfo = new TestInfoImpl(this._config, this._project, this._params, undefined, 0, () => {}, () => {}, () => {});
-    const runnable = { type: 'teardown' } as const;
-    // Ignore top-level errors, they are already inside TestInfo.errors.
-    await this._fixtureRunner.teardownScope('test', fakeTestInfo, runnable).catch(() => {});
-    await this._fixtureRunner.teardownScope('worker', fakeTestInfo, runnable).catch(() => {});
-    this._fatalErrors.push(...fakeTestInfo.errors);
-  }
-
   unhandledError(error: Error | any) {
     // No current test - fatal error.
     if (!this._currentTest) {

tests/playwright-test/timeout.spec.ts

@@ -514,3 +514,47 @@ test('should report up to 3 timeout errors', async ({ runInlineTest }) => {
   expect(result.output).toContain('Test timeout of 1000ms exceeded while running "afterEach" hook.');
   expect(result.output).toContain('Worker teardown timeout of 1000ms exceeded while tearing down "autoWorker".');
 });
+
+test('should complain when worker fixture times out during worker cleanup', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      import { test as base, expect } from '@playwright/test';
+      const test = base.extend({
+        slowTeardown: [async ({}, use) => {
+          await use('hey');
+          await new Promise(f => setTimeout(f, 2000));
+        }, { scope: 'worker', auto: true, timeout: 400 }],
+      });
+      test('test ok', async ({ slowTeardown }) => {
+        expect(slowTeardown).toBe('hey');
+      });
+    `
+  });
+  expect(result.exitCode).toBe(1);
+  expect(result.passed).toBe(1);
+  expect(result.output).toContain(`Fixture "slowTeardown" timeout of 400ms exceeded during teardown.`);
+});
+
+test('should allow custom worker fixture timeout longer than force exit cap', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'a.spec.ts': `
+      import { test as base, expect } from '@playwright/test';
+      const test = base.extend({
+        slowTeardown: [async ({}, use) => {
+          await use('hey');
+          await new Promise(f => setTimeout(f, 1500));
+          console.log('output from teardown');
+          throw new Error('Oh my!');
+        }, { scope: 'worker', auto: true, timeout: 2000 }],
+      });
+      test('test ok', async ({ slowTeardown }) => {
+        expect(slowTeardown).toBe('hey');
+      });
+    `
+  }, {}, { PWTEST_FORCE_EXIT_TIMEOUT: '400' });
+  expect(result.exitCode).toBe(1);
+  expect(result.passed).toBe(1);
+  expect(result.output).toContain(`output from teardown`);
+  expect(result.output).toContain(`Error: Oh my!`);
+  expect(result.output).toContain(`1 error was not a part of any test, see above for details`);
+});