fix(test runner): project.workers is sometimes exceeded (#39539)

Author: dgozman

packages/playwright/src/runner/dispatcher.ts

@@ -40,7 +40,8 @@ import type { RegisteredListener } from 'playwright-core/lib/utils';
 export type EnvByProjectId = Map<string, Record<string, string | undefined>>;
 
 export class Dispatcher {
-  private _workerSlots: { busy: boolean, worker?: WorkerHost, jobDispatcher?: JobDispatcher }[] = [];
+  // Worker slot is claimed when it has jobDispatcher assigned.
+  private _workerSlots: { worker?: WorkerHost, jobDispatcher?: JobDispatcher }[] = [];
   private _queue: TestGroup[] = [];
   private _workerLimitPerProjectId = new Map<string, number>();
   private _queuedOrRunningHashCount = new Map<string, number>();
@@ -71,7 +72,7 @@ export class Dispatcher {
       const projectIdWorkerLimit = this._workerLimitPerProjectId.get(job.projectId);
       if (!projectIdWorkerLimit)
         return index;
-      const runningWorkersWithSameProjectId = this._workerSlots.filter(w => w.busy && w.worker && w.worker.projectId() === job.projectId).length;
+      const runningWorkersWithSameProjectId = this._workerSlots.filter(w => w.jobDispatcher?.job.projectId === job.projectId).length;
       if (runningWorkersWithSameProjectId < projectIdWorkerLimit)
         return index;
     }
@@ -92,9 +93,9 @@ export class Dispatcher {
     const job = this._queue[jobIndex];
 
     // 2. Find a worker with the same hash, or just some free worker.
-    let workerIndex = this._workerSlots.findIndex(w => !w.busy && w.worker && w.worker.hash() === job.workerHash && !w.worker.didSendStop());
+    let workerIndex = this._workerSlots.findIndex(w => !w.jobDispatcher && w.worker && w.worker.hash() === job.workerHash && !w.worker.didSendStop());
     if (workerIndex === -1)
-      workerIndex = this._workerSlots.findIndex(w => !w.busy);
+      workerIndex = this._workerSlots.findIndex(w => !w.jobDispatcher);
     if (workerIndex === -1) {
       // No workers available, bail out.
       return;
@@ -103,15 +104,13 @@ export class Dispatcher {
     // 3. Claim both the job and the worker slot.
     this._queue.splice(jobIndex, 1);
     const jobDispatcher = new JobDispatcher(job, this._config, this._reporter, this._failureTracker, () => this.stop().catch(() => {}));
-    this._workerSlots[workerIndex].busy = true;
     this._workerSlots[workerIndex].jobDispatcher = jobDispatcher;
 
     // 4. Run the job. This is the only async operation.
     void this._runJobInWorker(workerIndex, jobDispatcher).then(() => {
 
       // 5. Release the worker slot.
       this._workerSlots[workerIndex].jobDispatcher = undefined;
-      this._workerSlots[workerIndex].busy = false;
 
       // 6. Check whether we are done or should schedule another job.
       this._checkFinished();
@@ -178,7 +177,7 @@ export class Dispatcher {
       return;
 
     // Make sure all workers have finished the current job.
-    if (this._workerSlots.some(w => w.busy))
+    if (this._workerSlots.some(w => !!w.jobDispatcher))
       return;
 
     this._finished.resolve();
@@ -209,7 +208,7 @@ export class Dispatcher {
       void this.stop();
     // 1. Allocate workers.
     for (let i = 0; i < this._config.config.workers; i++)
-      this._workerSlots.push({ busy: false });
+      this._workerSlots.push({});
     // 2. Schedule enough jobs.
     for (let i = 0; i < this._workerSlots.length; i++)
       this._scheduleJob();

tests/playwright-test/worker-index.spec.ts

@@ -280,6 +280,47 @@ test('should respect project.workers=1', async ({ runInlineTest }) => {
   ]);
 });
 
+test('should respect project.workers=1 on startup', async ({ runInlineTest }) => {
+  const result = await runInlineTest({
+    'playwright.config.ts': `
+      export default {
+        workers: 2,
+        projects: [
+          { name: 'project1' },
+          { name: 'project2', workers: 1 },
+        ],
+      };
+    `,
+    'a.test.js': `
+      import { test, expect } from '@playwright/test';
+      test('test1', async ({}, testInfo) => {
+        console.log('%%test1-begin:' + testInfo.project.name);
+        await new Promise(f => setTimeout(f, 2000));
+        console.log('%%test1-end:' + testInfo.project.name);
+      });
+    `,
+    'b.test.js': `
+      import { test, expect } from '@playwright/test';
+      test('test2', async ({}, testInfo) => {
+        console.log('%%test2-begin:' + testInfo.project.name);
+        await new Promise(f => setTimeout(f, 2000));
+        console.log('%%test2-end:' + testInfo.project.name);
+      });
+    `,
+  }, { workers: 2 });
+  expect(result.passed).toBe(4);
+  expect(result.exitCode).toBe(0);
+
+  // Once both tests from the first project finish apporximately at the same time,
+  // tests from the second project should run sequentially.
+  expect(result.outputLines.slice(4, 8)).toEqual([
+    'test1-begin:project2',
+    'test1-end:project2',
+    'test2-begin:project2',
+    'test2-end:project2',
+  ]);
+});
+
 test('should respect project.workers>1', async ({ runInlineTest }) => {
   const result = await runInlineTest({
     'playwright.config.ts': `