test(browser): test servers bind an OS-chosen port themselves

tests/browser/server.py accepts --port 0: it binds 127.0.0.1:0 before
building the app, holds that socket for its whole life and prints
PM_TEST_SERVER_PORT=<n> to stdout. startBrowserServer defaults to port 0
and reads the announced port, so no port is reserved and released before
the server binds and two launchers can no longer race for one. The
"address already in use" retry and free-port.mjs are gone; --port N still
works and is checked against the announcement.

TESTING.md

@@ -40,8 +40,9 @@ npm run --prefix tools/vendor test:workflows -- --project firefox --project webk
 ```
 
 `PM_TEST_PYTHON` selects the Python executable; set it when Python is outside
-PATH. Every Playwright worker starts its own `tests/browser/server.py` on a port the
-OS picks, with its own temp root and database (`tests/browser/fixtures.mjs`), so
+PATH. Every Playwright worker starts its own `tests/browser/server.py --port 0`,
+which binds a port the OS picks and prints it as `PM_TEST_SERVER_PORT=<n>`, with
+its own temp root and database (`tests/browser/fixtures.mjs`), so
 parallel workers and concurrent runs never share state. `PM_TEST_WORKERS` sets the
 worker count (default 1 under `CI`, otherwise a quarter of the CPUs, at most 8).
 Test servers are never reused across runs. On unsupported Linux

tests/browser/fixtures.mjs

@@ -1,17 +1,16 @@
 // Shared `test` for the page workflows: every Playwright worker owns one
-// tests/browser/server.py process on an OS-chosen port, with its own temp root
+// tests/browser/server.py process on a port the OS chose for it, with its own temp root
 // and database. `baseURL` (and so `page`, `request` and `browser.newContext`)
 // points at that worker's server. A server that dies fails the running test
 // with its stderr tail, and the next test in the worker gets a fresh server.
 import { test as base, expect } from '../../tools/vendor/node_modules/@playwright/test/index.mjs';
-import { freePort } from '../../tools/vendor/free-port.mjs';
 import { createServerSupervisor, startBrowserServer } from '../../tools/vendor/start-browser-server.mjs';
 
 const SERVER_FIXTURE_TIMEOUT_MS = 90_000;
 
 export const test = base.extend({
     testServer: [async ({}, use) => {
-        const supervisor = createServerSupervisor({ start: async () => startBrowserServer({ port: await freePort() }) });
+        const supervisor = createServerSupervisor({ start: () => startBrowserServer() });
         try {
             await use(supervisor);
         } finally {

tests/browser/server.py

@@ -4,6 +4,7 @@ import argparse
 import json
 import os
 import shutil
+import socket
 from pathlib import Path
 import sys
 import tempfile
@@ -515,14 +516,41 @@ def create_app(root: Path) -> web.Application:
     return app
 
 
-def main() -> None:
+HOST = "127.0.0.1"
+PORT_ANNOUNCEMENT = "PM_TEST_SERVER_PORT="
+
+
+def bind_listener(port: int) -> socket.socket:
+    """Bind the server's socket before the app is built.
+
+    ``port`` 0 lets the OS choose a free port, which this process then holds,
+    so parallel launchers never race for one between choosing and binding.
+    """
+    listener = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+    try:
+        if port and os.name != "nt":
+            # aiohttp's own default on POSIX; on Windows it would let a
+            # second process share the port.
+            listener.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+        listener.bind((HOST, port))
+        listener.listen(128)
+    except OSError:
+        listener.close()
+        raise
+    return listener
+
+
+def main(argv: list[str] | None = None) -> None:
     parser = argparse.ArgumentParser()
-    parser.add_argument("--port", type=int, default=8765)
-    args = parser.parse_args()
+    parser.add_argument(
+        "--port", type=int, default=8765, help="0 binds an OS-chosen free port"
+    )
+    args = parser.parse_args(argv)
+    listener = bind_listener(args.port)
+    # One machine-readable line tells the launcher which port this process holds.
+    print(f"{PORT_ANNOUNCEMENT}{listener.getsockname()[1]}", flush=True)
     with tempfile.TemporaryDirectory(prefix="pm-browser-") as directory:
-        web.run_app(
-            create_app(Path(directory).resolve()), host="127.0.0.1", port=args.port
-        )
+        web.run_app(create_app(Path(directory).resolve()), sock=listener)
 
 
 if __name__ == "__main__":

tests/browser/worker-server.spec.mjs

@@ -2,7 +2,6 @@
 // `browser.newContext()` all reach it through `baseURL`, and another server
 // process never sees its database.
 import { test, expect } from './fixtures.mjs';
-import { freePort } from '../../tools/vendor/free-port.mjs';
 import { startBrowserServer } from '../../tools/vendor/start-browser-server.mjs';
 
 const search = text => `/prompt_manager/search?text=${encodeURIComponent(text)}`;
@@ -26,7 +25,7 @@ test('every fixture reaches this worker\'s own server and another server keeps i
         await context.close();
     }
 
-    const other = await startBrowserServer({ port: await freePort() });
+    const other = await startBrowserServer();
     try {
         expect(other.url).not.toBe(baseURL);
         expect((await (await fetch(`${other.url}${search(marker)}`)).json()).count).toBe(0);

tests/vendor/browser-server-fixture.test.mjs

@@ -2,12 +2,12 @@ import test from 'node:test';
 import assert from 'node:assert/strict';
 import { createServer } from 'node:net';
 import { fileURLToPath } from 'node:url';
-import { freePort } from '../../tools/vendor/free-port.mjs';
 import { startBrowserServer, createServerSupervisor } from '../../tools/vendor/start-browser-server.mjs';
 import { workflowWorkers } from '../../tools/vendor/test-workers.mjs';
 
 const fakeServer = fileURLToPath(new URL('./fake-browser-server.mjs', import.meta.url));
 const fake = (...extra) => ({ command: process.execPath, args: port => [fakeServer, '--port', String(port), ...extra] });
+const REAL_SERVER_TIMEOUT_MS = 90_000;
 
 function listen(port = 0) {
     return new Promise((resolve, reject) => {
@@ -17,24 +17,42 @@ function listen(port = 0) {
     });
 }
 
+async function releasedPort() {
+    const probe = await listen();
+    const { port } = probe.address();
+    await new Promise(resolve => probe.close(resolve));
+    return port;
+}
+
 test('free port allocation never reuses a bound port', async t => {
     const held = await listen();
     t.after(() => held.close());
     const heldPort = held.address().port;
-    const seen = [];
-    for (let index = 0; index < 20; index += 1) {
-        const port = await freePort();
-        assert.ok(Number.isInteger(port) && port > 0 && port < 65536, `port ${port} is a TCP port`);
-        assert.notEqual(port, heldPort, 'a port another process holds is never handed out');
-        const probe = await listen(port);
-        seen.push(port);
-        await new Promise(resolve => probe.close(resolve));
+    const servers = await Promise.all(Array.from({ length: 6 }, () => startBrowserServer(fake())));
+    t.after(() => Promise.all(servers.map(server => server.stop())));
+    const ports = servers.map(server => server.port);
+    for (const server of servers) {
+        assert.ok(Number.isInteger(server.port) && server.port > 0 && server.port < 65536, `port ${server.port} is a TCP port`);
+        assert.equal(server.url, `http://127.0.0.1:${server.port}`);
+        assert.equal(await (await fetch(`${server.url}/prompt_manager/test`)).text(), 'ready');
     }
-    assert.ok(new Set(seen).size > 1, 'the OS hands out more than one port');
+    assert.ok(!ports.includes(heldPort), 'a port another process holds is never handed out');
+    assert.equal(new Set(ports).size, ports.length, 'concurrent servers never share a port');
 });
 
-test('startBrowserServer serves the readiness page on the given port and stops cleanly', async () => {
-    const port = await freePort();
+test('startBrowserServer asks the server to bind port 0 and reads the port it announces', async () => {
+    const asked = [];
+    const server = await startBrowserServer({ command: process.execPath, args: port => { asked.push(port); return fake().args(port); } });
+    try {
+        assert.deepEqual(asked, [0], 'no port is reserved and released before the server binds');
+        assert.ok(server.port > 0);
+    } finally {
+        await server.stop();
+    }
+});
+
+test('startBrowserServer serves the readiness page on a requested port and stops cleanly', async () => {
+    const port = await releasedPort();
     const server = await startBrowserServer({ port, ...fake() });
     assert.equal(server.port, port);
     assert.equal(server.url, `http://127.0.0.1:${port}`);
@@ -44,22 +62,20 @@ test('startBrowserServer serves the readiness page on the given port and stops c
 });
 
 test('startBrowserServer reports a startup failure with the server\'s stderr tail', async () => {
-    const port = await freePort();
-    await assert.rejects(startBrowserServer({ port, ...fake('--fail') }), error => {
+    await assert.rejects(startBrowserServer(fake('--fail')), error => {
         assert.match(error.message, /exited with code 4 before it was ready/);
         assert.match(error.message, /last fake stderr line/);
         return true;
     });
 });
 
-test('startBrowserServer gives up on a server that never becomes ready', async () => {
-    const port = await freePort();
+test('startBrowserServer gives up on a server that never announces a port or becomes ready', async () => {
     const silent = { command: process.execPath, args: () => ['-e', 'setInterval(() => {}, 1000)'] };
-    await assert.rejects(startBrowserServer({ port, ...silent, timeoutMs: 500 }), /not ready .* within 500 ms/);
+    await assert.rejects(startBrowserServer({ ...silent, timeoutMs: 500 }), /not ready .* within 500 ms/);
 });
 
 test('worker server restarts after an unexpected exit', async t => {
-    const supervisor = createServerSupervisor({ start: async () => startBrowserServer({ port: await freePort(), ...fake() }) });
+    const supervisor = createServerSupervisor({ start: () => startBrowserServer(fake()) });
     t.after(() => supervisor.stop());
     const first = await supervisor.ensure();
     assert.equal(supervisor.crash(first), null, 'a running server reports no crash');
@@ -74,29 +90,41 @@ test('worker server restarts after an unexpected exit', async t => {
     assert.equal(await supervisor.ensure(), second, 'a healthy server is reused');
 });
 
-test('supervisor retries a port that was taken between reservation and bind', async t => {
-    const ports = [];
+test('supervisor reports a startup failure once and tries again on the next ensure', async t => {
     let attempts = 0;
     const supervisor = createServerSupervisor({
         start: async () => {
             attempts += 1;
-            if (attempts === 1) throw new Error('OSError: [Errno 98] error while attempting to bind: address already in use');
-            const port = await freePort();
-            ports.push(port);
-            return startBrowserServer({ port, ...fake() });
+            if (attempts === 1) throw new Error('ImportError: no module');
+            return startBrowserServer(fake());
         },
     });
     t.after(() => supervisor.stop());
+    await assert.rejects(supervisor.ensure(), /ImportError/);
+    assert.equal(attempts, 1, 'a failed startup is not retried behind the caller\'s back');
     const server = await supervisor.ensure();
     assert.equal(attempts, 2);
-    assert.equal(server.port, ports[0]);
+    assert.equal(await (await fetch(`${server.url}/prompt_manager/test`)).text(), 'ready');
 });
 
-test('supervisor does not retry a startup failure that is not a port collision', async () => {
-    let attempts = 0;
-    const supervisor = createServerSupervisor({ start: async () => { attempts += 1; throw new Error('ImportError: no module'); } });
-    await assert.rejects(supervisor.ensure(), /ImportError/);
-    assert.equal(attempts, 1);
+test('tests/browser/server.py binds an OS-chosen port for --port 0 and still honours --port N', { timeout: 2 * REAL_SERVER_TIMEOUT_MS }, async t => {
+    const held = await listen();
+    t.after(() => held.close());
+    const chosen = await startBrowserServer({ timeoutMs: REAL_SERVER_TIMEOUT_MS });
+    try {
+        assert.notEqual(chosen.port, held.address().port);
+        assert.equal((await fetch(`${chosen.url}/prompt_manager/test`)).status, 200);
+    } finally {
+        await chosen.stop();
+    }
+    const port = await releasedPort();
+    const fixed = await startBrowserServer({ port, timeoutMs: REAL_SERVER_TIMEOUT_MS });
+    try {
+        assert.equal(fixed.port, port);
+        assert.equal((await fetch(`http://127.0.0.1:${port}/prompt_manager/test`)).status, 200);
+    } finally {
+        await fixed.stop();
+    }
 });
 
 test('worker count follows PM_TEST_WORKERS, then one in CI, then a quarter of the CPUs up to eight', () => {

tests/vendor/fake-browser-server.mjs

@@ -1,6 +1,8 @@
-// A stand-in for tests/browser/server.py: `--port N` serves the readiness page,
-// `--fail` writes to stderr and exits before listening, and GET /__exit makes
-// the process die unexpectedly after writing a last stderr line.
+// A stand-in for tests/browser/server.py: `--port N` (0 = an OS-chosen port)
+// serves the readiness page and announces the bound port on stdout as
+// `PM_TEST_SERVER_PORT=<n>`, `--fail` writes to stderr and exits before
+// listening, and GET /__exit makes the process die unexpectedly after writing a
+// last stderr line.
 import { createServer } from 'node:http';
 
 const args = process.argv.slice(2);
@@ -17,5 +19,7 @@ const server = createServer((request, response) => {
     }
     response.end(request.url === '/prompt_manager/test' ? 'ready' : `pid ${process.pid}`);
 });
-server.listen(port, '127.0.0.1');
+server.listen(port, '127.0.0.1', () => {
+    process.stdout.write(`fake server: listening\nPM_TEST_SERVER_PORT=${server.address().port}\n`);
+});
 for (const signal of ['SIGTERM', 'SIGINT']) process.once(signal, () => server.close(() => process.exit(0)));

tools/vendor/free-port.mjs

@@ -1,17 +0,0 @@
-import { createServer } from 'node:net';
-
-// Ask the OS for a port nobody holds: bind port 0 on loopback, read it back and
-// release it. A port another process has bound is never returned. The caller
-// binds it next; the short window in between is covered by retrying a startup
-// that fails with "address already in use" (see createServerSupervisor).
-export function freePort(host = '127.0.0.1') {
-    return new Promise((resolve, reject) => {
-        const server = createServer();
-        server.unref();
-        server.once('error', reject);
-        server.listen(0, host, () => {
-            const { port } = server.address();
-            server.close(error => (error ? reject(error) : resolve(port)));
-        });
-    });
-}

tools/vendor/start-browser-server.mjs

@@ -10,8 +10,9 @@ const STOP_TIMEOUT_MS = 5_000;
 const POLL_INTERVAL_MS = 100;
 const PROBE_TIMEOUT_MS = 1_000;
 const STDERR_TAIL_BYTES = 8_192;
-const START_ATTEMPTS = 3;
-const PORT_COLLISION = /address already in use|errno 98\b|errno 48\b|10048|EADDRINUSE/i;
+const HOST = '127.0.0.1';
+// tests/browser/server.py prints this once it holds its listening socket.
+const PORT_ANNOUNCEMENT = /^PM_TEST_SERVER_PORT=(\d+)\s*$/m;
 
 const pythonServer = port => ['-m', 'tests.browser.server', '--port', String(port)];
 
@@ -19,28 +20,41 @@ function describeExit({ code, signal }) {
     return signal ? `signal ${signal}` : `code ${code}`;
 }
 
-// Start one disposable tests/browser/server.py on `port` and wait until it
-// serves the readiness page. Resolves to {port, url, exited, exitStatus,
+// Start one disposable tests/browser/server.py and wait until it serves the
+// readiness page. `port` 0 (the default) lets the server bind an OS-chosen port
+// itself and announce it on stdout, so nothing can take the port between
+// choosing and binding. Resolves to {port, url, exited, exitStatus,
 // stderrTail(), stop()}; rejects with the server's stderr tail when it exits
 // or stays unready.
 export async function startBrowserServer({
-    port,
+    port = 0,
     command = process.env.PM_TEST_PYTHON || 'python',
     args = pythonServer,
     cwd = repositoryRoot,
     timeoutMs = STARTUP_TIMEOUT_MS,
 } = {}) {
-    if (!Number.isInteger(port) || port <= 0) throw new Error(`startBrowserServer needs a port, got ${port}`);
-    const url = `http://127.0.0.1:${port}`;
-    const child = spawn(command, args(port), { cwd, stdio: ['ignore', 'ignore', 'pipe'] });
+    if (!Number.isInteger(port) || port < 0 || port > 65535) {
+        throw new Error(`startBrowserServer needs a port from 0 to 65535, got ${port}`);
+    }
+    const child = spawn(command, args(port), { cwd, stdio: ['ignore', 'pipe', 'pipe'] });
     let stderr = '';
     child.stderr.setEncoding('utf8');
     child.stderr.on('data', chunk => { stderr = (stderr + chunk).slice(-STDERR_TAIL_BYTES); });
+    let stdout = '';
+    let announced = null;
+    child.stdout.setEncoding('utf8');
+    // Keep draining stdout for the server's whole life so it never blocks on a full pipe.
+    child.stdout.on('data', chunk => {
+        if (announced !== null) return;
+        stdout = (stdout + chunk).slice(-STDERR_TAIL_BYTES);
+        const match = PORT_ANNOUNCEMENT.exec(stdout);
+        if (match) announced = Number(match[1]);
+    });
     const killOnExit = () => child.kill('SIGKILL');
     process.once('exit', killOnExit);
     const server = {
-        port,
-        url,
+        port: port || null,
+        url: port ? `http://${HOST}:${port}` : null,
         exitStatus: null,
         stopping: false,
         stderrTail: () => stderr.trimEnd(),
@@ -72,15 +86,27 @@ export async function startBrowserServer({
     const failure = async reason => {
         await server.stop();
         const tail = server.stderrTail();
-        return new Error(`test server on port ${port} ${reason}${tail ? `\n--- server stderr tail ---\n${tail}` : ''}`);
+        return new Error(`test server on port ${server.port ?? '(not yet announced)'} ${reason}`
+            + (tail ? `\n--- server stderr tail ---\n${tail}` : ''));
     };
     const deadline = Date.now() + timeoutMs;
     while (Date.now() < deadline) {
         if (server.exitStatus !== null) {
             throw await failure(`exited with ${describeExit(server.exitStatus)} before it was ready`);
         }
+        if (announced !== null && server.url === null) {
+            server.port = announced;
+            server.url = `http://${HOST}:${announced}`;
+        }
+        if (announced !== null && announced !== server.port) {
+            throw await failure(`announced port ${announced} instead`);
+        }
+        if (server.url === null) {
+            await delay(POLL_INTERVAL_MS);
+            continue;
+        }
         try {
-            const response = await fetch(`${url}${READY_PATH}`, { signal: AbortSignal.timeout(PROBE_TIMEOUT_MS) });
+            const response = await fetch(`${server.url}${READY_PATH}`, { signal: AbortSignal.timeout(PROBE_TIMEOUT_MS) });
             await response.arrayBuffer();
             if (response.ok) return server;
         } catch {
@@ -92,23 +118,17 @@ export async function startBrowserServer({
 }
 
 // Keep one live server for a Playwright worker. ensure() returns the running
-// server, or starts a fresh one when the last one died; crash(server) turns an
-// unexpected exit into an error carrying the stderr tail; a startup that lost
-// its port to another process between reservation and bind is retried.
-export function createServerSupervisor({ start, attempts = START_ATTEMPTS }) {
+// server, or starts a fresh one when the last one died (a failed start is
+// reported, not retried); crash(server) turns an unexpected exit into an error
+// carrying the stderr tail.
+export function createServerSupervisor({ start }) {
     let current = null;
     return {
         async ensure() {
             if (current && current.exitStatus === null) return current;
             current = null;
-            for (let attempt = 1; ; attempt += 1) {
-                try {
-                    current = await start();
-                    return current;
-                } catch (error) {
-                    if (attempt >= attempts || !PORT_COLLISION.test(error.message)) throw error;
-                }
-            }
+            current = await start();
+            return current;
         },
         crash(server) {
             if (!server || server.stopping || server.exitStatus === null) return null;