fix: scope the Playwright server state to a single run

src/Playwright/Servers/AlreadyStartedPlaywrightServer.php

@@ -13,6 +13,23 @@
  */
 final readonly class AlreadyStartedPlaywrightServer implements PlaywrightServer
 {
+    /**
+     * The environment variable carrying the identifier of the current test run.
+     *
+     * The main process mints it and exports it before spawning any worker, so
+     * every worker resolves the same state file as the run that spawned it,
+     * while a second, unrelated test run in the same project resolves its own.
+     */
+    private const string RUN_ID_ENV = 'PEST_BROWSER_RUN_ID';
+
+    /**
+     * How long a state file may go untouched before it is considered abandoned.
+     *
+     * A run killed by a stall watchdog or a CI timeout never reaches its
+     * teardown, so its file would otherwise stay behind forever.
+     */
+    private const int STALE_AFTER_SECONDS = 21600;
+
     /**
      * Creates a new already started playwright server instance.
      */
@@ -32,14 +49,14 @@ public static function fromPersisted(): self
     {
         $path = self::path();
 
-        $path = file_get_contents($path);
+        $contents = file_get_contents($path);
 
-        if ($path === false) {
+        if ($contents === false) {
             throw new RuntimeException('Could not read Playwright server data from file.');
         }
 
         // @phpstan-ignore-next-line
-        ['host' => $host, 'port' => $port] = json_decode($path, true, 512, JSON_THROW_ON_ERROR);
+        ['host' => $host, 'port' => $port] = json_decode($contents, true, 512, JSON_THROW_ON_ERROR);
 
         assert(is_string($host) && is_numeric($port), 'Invalid Playwright server data persisted.');
 
@@ -61,16 +78,36 @@ public static function persist(string $host, int $port): void
         ];
 
         $path = self::path();
+        $directory = dirname($path);
+
+        // Two runs may reach this at the same moment, so the result is checked
+        // rather than the precondition -- "is_dir" before "mkdir" is a race.
+        if (! is_dir($directory) && ! mkdir($directory, 0755, true) && ! is_dir($directory)) {
+            throw new RuntimeException(sprintf('Could not create the directory [%s].', $directory));
+        }
+
+        self::removeAbandonedStateFiles($path);
 
-        if (! file_exists(dirname($path))) {
-            mkdir(dirname($path), 0755, true);
+        // Written through a temporary file and moved into place, so a worker
+        // can never observe a half-written description.
+        $temporary = $path.'.'.getmypid().'.tmp';
+
+        if (file_put_contents($temporary, json_encode($data, JSON_THROW_ON_ERROR)) === false) {
+            throw new RuntimeException(sprintf('Could not write Playwright server data to [%s].', $temporary));
         }
 
-        file_put_contents($path, json_encode($data, JSON_THROW_ON_ERROR));
+        if (! rename($temporary, $path)) {
+            @unlink($temporary);
+
+            throw new RuntimeException(sprintf('Could not move Playwright server data into [%s].', $path));
+        }
     }
 
     /**
      * Marks the Playwright server a stopped by removing the persisted state file.
+     *
+     * The file is scoped to the current run, so a test run that finishes while
+     * another one is still going can only ever remove its own description.
      */
     public static function markAsStopped(): void
     {
@@ -114,10 +151,75 @@ public function url(): string
     }
 
     /**
-     * Returns the state file of the Playwright server.
+     * Returns the state file of the Playwright server for the current run.
      */
     private static function path(): string
     {
-        return dirname(__DIR__, 3).'/.temp/playwright-server.json';
+        return dirname(__DIR__, 3).'/.temp/playwright-server-'.self::runId().'.json';
+    }
+
+    /**
+     * Returns the identifier of the current test run, minting one if this is
+     * the process that starts the server.
+     */
+    private static function runId(): string
+    {
+        foreach ([$_SERVER, $_ENV] as $source) {
+            if (isset($source[self::RUN_ID_ENV]) && is_string($source[self::RUN_ID_ENV]) && $source[self::RUN_ID_ENV] !== '') {
+                return self::sanitize($source[self::RUN_ID_ENV]);
+            }
+        }
+
+        $inherited = getenv(self::RUN_ID_ENV);
+
+        if (is_string($inherited) && $inherited !== '') {
+            return self::sanitize($inherited);
+        }
+
+        // The random suffix matters as much as the process id: a killed run can
+        // leave a state file behind, and process ids are reused.
+        $runId = getmypid().'-'.bin2hex(random_bytes(4));
+
+        putenv(self::RUN_ID_ENV.'='.$runId);
+        $_SERVER[self::RUN_ID_ENV] = $runId;
+        $_ENV[self::RUN_ID_ENV] = $runId;
+
+        return $runId;
+    }
+
+    /**
+     * Reduces the run identifier to characters that are safe in a file name.
+     *
+     * It arrives from the environment, so it is not assumed to be well-formed.
+     */
+    private static function sanitize(string $runId): string
+    {
+        $sanitized = preg_replace('/[^A-Za-z0-9_-]/', '', $runId);
+
+        return match (true) {
+            $sanitized === null, $sanitized === '' => 'default',
+            default => mb_substr($sanitized, 0, 64),
+        };
+    }
+
+    /**
+     * Removes state files left behind by runs that never reached their teardown.
+     */
+    private static function removeAbandonedStateFiles(string $currentPath): void
+    {
+        // A missing directory answers with an empty array rather than false.
+        $files = glob(dirname($currentPath).'/playwright-server-*.json');
+
+        foreach ($files === false ? [] : $files as $file) {
+            if ($file === $currentPath) {
+                continue;
+            }
+
+            $modifiedAt = @filemtime($file);
+
+            if ($modifiedAt !== false && (time() - $modifiedAt) > self::STALE_AFTER_SECONDS) {
+                @unlink($file);
+            }
+        }
     }
 }

src/ServerManager.php

@@ -59,21 +59,28 @@ public function playwright(): PlaywrightServer
             return AlreadyStartedPlaywrightServer::fromPersisted();
         }
 
-        $port = Port::find();
-        $host = Playwright::host() ?? self::DEFAULT_HOST;
+        // Finding the port and persisting the description belong inside the
+        // guard, not beside it. Only the first call creates the server, but
+        // every call used to allocate a fresh port and then persist THAT one --
+        // so the second call described the running server by a port nothing was
+        // listening on.
+        if (! $this->playwright instanceof PlaywrightServer) {
+            $port = Port::find();
+            $host = Playwright::host() ?? self::DEFAULT_HOST;
 
-        $this->playwright ??= PlaywrightNpmServer::create(
-            PackageJsonDirectory::find(),
-            '.'.DIRECTORY_SEPARATOR.'node_modules'.DIRECTORY_SEPARATOR.'.bin'.DIRECTORY_SEPARATOR.'playwright run-server --host %s --port %d --mode launchServer',
-            $host,
-            $port,
-            'Listening on',
-        );
+            $this->playwright = PlaywrightNpmServer::create(
+                PackageJsonDirectory::find(),
+                '.'.DIRECTORY_SEPARATOR.'node_modules'.DIRECTORY_SEPARATOR.'.bin'.DIRECTORY_SEPARATOR.'playwright run-server --host %s --port %d --mode launchServer',
+                $host,
+                $port,
+                'Listening on',
+            );
 
-        AlreadyStartedPlaywrightServer::persist(
-            $host,
-            $port,
-        );
+            AlreadyStartedPlaywrightServer::persist(
+                $host,
+                $port,
+            );
+        }
 
         return $this->playwright;
     }

tests/Unit/Playwright/AlreadyStartedPlaywrightServerTest.php

@@ -0,0 +1,79 @@
+<?php
+
+declare(strict_types=1);
+
+use Pest\Browser\Playwright\Servers\AlreadyStartedPlaywrightServer;
+
+/*
+ * The state file used to be one path per project, so a second test run in the
+ * same checkout overwrote it and then deleted it on its way out. These cover the
+ * two halves of that: the description has to survive a foreign teardown, and a
+ * worker has to resolve the run it belongs to.
+ */
+
+afterEach(function (): void {
+    putenv('PEST_BROWSER_RUN_ID');
+    unset($_SERVER['PEST_BROWSER_RUN_ID'], $_ENV['PEST_BROWSER_RUN_ID']);
+});
+
+/** Runs a closure under a given run id, the way a worker inherits one. */
+function underRunId(string $runId, Closure $work): mixed
+{
+    putenv('PEST_BROWSER_RUN_ID='.$runId);
+    $_SERVER['PEST_BROWSER_RUN_ID'] = $runId;
+    $_ENV['PEST_BROWSER_RUN_ID'] = $runId;
+
+    try {
+        return $work();
+    } finally {
+        putenv('PEST_BROWSER_RUN_ID');
+        unset($_SERVER['PEST_BROWSER_RUN_ID'], $_ENV['PEST_BROWSER_RUN_ID']);
+    }
+}
+
+it('keeps one run\'s server when another run tears its own down', function (): void {
+    underRunId('run-a', function (): void {
+        AlreadyStartedPlaywrightServer::persist('127.0.0.1', 1111);
+    });
+
+    underRunId('run-b', function (): void {
+        AlreadyStartedPlaywrightServer::persist('127.0.0.1', 2222);
+        AlreadyStartedPlaywrightServer::markAsStopped();
+    });
+
+    $server = underRunId('run-a', fn (): AlreadyStartedPlaywrightServer => AlreadyStartedPlaywrightServer::fromPersisted());
+
+    expect($server->port)->toBe(1111)
+        ->and($server->host)->toBe('127.0.0.1');
+
+    underRunId('run-a', function (): void {
+        AlreadyStartedPlaywrightServer::markAsStopped();
+    });
+});
+
+it('resolves the server belonging to the run it was given', function (): void {
+    underRunId('run-a', fn () => AlreadyStartedPlaywrightServer::persist('127.0.0.1', 1111));
+    underRunId('run-b', fn () => AlreadyStartedPlaywrightServer::persist('127.0.0.1', 2222));
+
+    $a = underRunId('run-a', fn (): AlreadyStartedPlaywrightServer => AlreadyStartedPlaywrightServer::fromPersisted());
+    $b = underRunId('run-b', fn (): AlreadyStartedPlaywrightServer => AlreadyStartedPlaywrightServer::fromPersisted());
+
+    expect($a->port)->toBe(1111)
+        ->and($b->port)->toBe(2222);
+
+    underRunId('run-a', fn () => AlreadyStartedPlaywrightServer::markAsStopped());
+    underRunId('run-b', fn () => AlreadyStartedPlaywrightServer::markAsStopped());
+});
+
+it('exports the run id it mints, so a worker inherits it', function (): void {
+    AlreadyStartedPlaywrightServer::persist('127.0.0.1', 3333);
+
+    // putenv() alone is not enough here. Symfony's Process builds the inherited
+    // environment as array_intersect_key(getenv(), $_SERVER), so a variable that
+    // exists only in getenv() is dropped before a worker ever sees it.
+    expect(getenv('PEST_BROWSER_RUN_ID'))->not->toBeFalse()
+        ->and($_SERVER)->toHaveKey('PEST_BROWSER_RUN_ID')
+        ->and($_ENV)->toHaveKey('PEST_BROWSER_RUN_ID');
+
+    AlreadyStartedPlaywrightServer::markAsStopped();
+});