[askalf]

Why

Main went red at 749fc44 on three _test_subject_proxy.mjs cases that had passed on the same commit's PR check, and a rerun passed. The job history explains it: every failed test job overlapped another test job of this repository on the same self-hosted machine, and every job that ran alone passed.

test job	runner	window (UTC)	result
PR #190 attempt 1	pool-e4	14:56:44 to 15:00:29	fail
PR #191 attempt 1	cordon-exec-e1	14:57:01 to 15:00:30	fail
PR #190 attempt 2	cordon-exec-e1	15:05:46 to 15:08:25	pass
PR #191 attempt 2	pool-e4	15:06:29 to 15:08:27	fail
main 749fc44 attempt 1	pool-e1	15:10:41 to 15:12:24	fail
PR #191 attempt 3	pool-e10	15:10:50 to 15:12:26	fail
main 749fc44 attempt 2	cordon-exec-e1	15:17:03 to 15:19:20	pass
PR #191 attempt 4	cordon-exec-e1	15:20:10 to 15:21:57	pass

The battery bound fixed loopback ports (stub 8900, a dead upstream on 8901, instances 8810 to 8813). Self-hosted runner rows share the host's loopback, so two batteries at once shared one stub (one battery's /__calls counted the other's requests: "upstream never called" cases saw a call, a bodies[0] was another battery's request) and the runner's "free the port" step killed whatever listened there, including the other job's processes.

What

• _test_ports.mjs: one table of the battery's ports, read from CORDON_TEST_PORTS=stub,dead,main,nosecret,noadmin,restore, with the historical fixed ports as the default so a suite can still be run by hand against a stub and an instance started by hand.
• _run_tests.mjs: picks six free ports per run (bind 0, read, close), exports the variable to the stub and the suites, logs them, and no longer kills listeners on fixed ports.
• _stub-upstream.mjs listens on STUB_PORT; _test_proxy.mjs, _test_stream.mjs, _test_subject_proxy.mjs take their URLs and the dead port from the table.
• CLAUDE.md's "running one suite" note and a CHANGELOG line under Unreleased / Fixed.

Proof

Two batteries started two seconds apart on one machine both print distinct ports: lines and both end "all suites green". A single npm test is unchanged apart from the ports line. No src change.

[sprayberry-redline]

Verdict: request changes. The per-run port wiring reaches the stub and integration suites, but manually pinned duplicate ports and Windows teardown need correction before the battery can reliably run with its new configuration.

1. Blocking: _test_ports.mjs:18

if (given.length !== NAMES.length || given.some((p) => !Number.isInteger(p) || p < 1 || p > 65535))
throw new Error(CORDON_TEST_PORTS must be ${NAMES.length} ports (${NAMES.join(",")}), got "${value}");

The new environment format accepts the same valid port more than once. For example, CORDON_TEST_PORTS=10000,10001,10002,10002,10004,10005 passes this check, but the computed instances object has two entries keyed by 10002; the later one overwrites the earlier instance configuration. The runner then starts too few instances and suites address a service with the wrong configuration or fail to connect.

Suggested fix:

Reject duplicate values while parsing, for example by requiring `new Set(given).size === NAMES.length` along with the existing numeric validation.

2. Blocking: _run_tests.mjs:5

import { spawn } from "node:child_process";

This removal leaves the Windows branch of die calling execSync without a binding. On Windows, when a child has a PID, that call raises ReferenceError, which the surrounding catch discards, and there is no fallback kill. Every test child can therefore remain running after the runner exits, leaking processes despite the teardown code intended to prevent it.

Suggested fix:

Keep `execSync` imported from `node:child_process`, or replace the Windows teardown implementation with an available API and verify it terminates the process tree.

Minor:

• _run_tests.mjs:11: This comment narrates the incident that led to the change rather than documenting behavior needed to maintain the port-selection code.

rule:none

[askalf]

Fix at edfbbab

• [1] Duplicate ports (blocking): parsePorts in _test_ports.mjs now rejects a CORDON_TEST_PORTS value that names the same port twice. Its check now also requires new Set(given).size === 6, and the error message says the ports must be distinct. Before this, a pinned value such as 10000,10001,10002,10002,10004,10005 was accepted, and two instances ended up keyed by the same port. New parsePorts cases in _test_unit.mjs cover a duplicate, a wrong count, an out-of-range port, the defaults, and an ordinary value. The duplicate case fails without this change.
• [2] Windows teardown (blocking): _run_tests.mjs imports execSync from node:child_process again. On Windows, die calls taskkill /T to stop each child's whole process tree. Without the import, that call threw a ReferenceError that the surrounding catch swallowed, so no process was stopped.
• [3] Comment (minor): The comment above freePort in _run_tests.mjs now says what the code does: each run takes six free loopback ports, so batteries running at the same time on one host get their own stub and instances, and setting CORDON_TEST_PORTS pins the ports instead. It no longer tells the story of the incident.

Files: _run_tests.mjs, _test_ports.mjs, _test_unit.mjs

Tests: npm test exited 0 (5 pass, 0 fail)

[sprayberry-redline]

Verdict: request changes. The duplicate-port validation and Windows teardown from the prior review are corrected, but the new allocation still does not reserve ports across concurrent batteries.

1. Blocking: _run_tests.mjs:15

const freePort = () =>
new Promise((resolve, reject) => {
const srv = createServer();
srv.once("error", reject);
srv.listen(0, "127.0.0.1", () => {
const { port } = srv.address();
srv.close(() => resolve(port));
});
});

Closing the probe socket before any child binds the selected port leaves a race between batteries. For example, battery A can obtain and close port 40000, then battery B can obtain and close that same port before A launches its stub or instance. One battery then fails with EADDRINUSE, or can attach to the other battery's service if its request arrives first. Thus simultaneous runs are not guaranteed distinct ports, which is the failure this change is intended to prevent.

Suggested fix:

Use a cross-process allocation mechanism that retains ownership until the corresponding service is ready, or make the runner and children coordinate a reservation/port assignment that cannot be reused by another battery during startup. Add a concurrency-focused test or harness check that demonstrates two simultaneous allocations cannot return an overlapping set.

rule:none

[askalf]

Fix at 59613df

• [1] _run_tests.mjs: a probed port could be taken by another battery before a child binds it. The runner now calls a new reservePorts() in _test_ports.mjs. It still gets a free port by binding port 0 and closing, but only takes that port after creating <port>.lock with an exclusive create (O_EXCL) in a lock directory every battery on the host shares (/tmp/cordon-test-ports on POSIX, under the temp directory on Windows; CORDON_TEST_PORT_LOCKS overrides it). If two batteries get the same port before either binds it, exactly one gets the lock and the other asks for a new port. The runner holds all six locks until it exits, including on SIGINT, SIGTERM and SIGHUP. That covers the stub, the instances, and the dead port, which is only bound when the proxy suite runs, so no other battery can take or attach to them while this one starts up or runs. Setting CORDON_TEST_PORTS by hand still pins the ports and skips the reservation. A lock left by a battery killed with SIGKILL is skipped rather than reclaimed, because checking that its owner is gone and deleting the file cannot be done as one step; each stale lock costs only that one port.
• Concurrency test in _test_unit.mjs. Two reservations run at the same time against a fake port source that hands out every port twice. The checks are that the two sets of six never overlap, that all twelve locks stay in place until released, that a later reservation skips ports another run still holds, and that release removes the lock files.

Files: _run_tests.mjs, _test_ports.mjs, _test_unit.mjs

Tests: npm test exited 0 (5 pass, 0 fail)

[sprayberry-redline]

Verdict: approve. The test runner now allocates and retains host-wide ownership records for six distinct per-run ports, passes that configuration through to every stub, instance, and affected suite, and restores Windows process-tree teardown. The parser and concurrent-reservation coverage exercise duplicate rejection, non-overlapping allocations, retained locks, and release behavior.