docs: address Copilot review round 1 (#2354)

The process-group signal guarantee is POSIX-only; the 45-minute wait is a
total queueing budget, not a hung-holder detector; the fixed-port range is
6295–6299 including smoke:web:tabs.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-09-13 17:58:06 -04:00
commit 09f625c197

View file

@ -91,13 +91,13 @@ The machine this repo is worked on runs three or four agent sessions at once, ea
`npm run local:gate` is `node scripts/gate-lease.mjs npm run local:gate:stages`: the stage chain runs as the child of a wrapper that holds **one machine-wide lease for the whole run**. The lock lives under `os.tmpdir()` — or `$XDG_RUNTIME_DIR` where a desktop session sets it — and never inside a worktree, because a lock in the repo would be one per worktree, which is one per session, which coordinates nothing. A second gate started in another worktree while one holds the lease **waits**: it prints one line naming the holder's pid, worktree and elapsed time, re-asks every 2s, says `still waiting` once a minute, and starts the moment the holder releases.
The lease is released on every way out — a normal exit, a spawn failure, or a signal. The wrapper runs the chain in its own process group, so Ctrl-C, a plain `kill`, a closed terminal or Ctrl-\ stop the whole tree (`npm run` nests four deep and `sh` forwards nothing), release, and exit `128 + signal`. A holder that dies without releasing — a killed terminal, an OOM'd session — stops refreshing its lock and is taken over after **30s** (`proper-lockfile`'s stale detection, at 30s rather than the library's 10s default: a refresh timer firing late under the very load this exists to manage would otherwise cause a false takeover, which is two gates overlapping — the thing being prevented — whereas the cost of the longer window is 20 more seconds after a crash, against a gate that takes minutes). A waiter gives up after **45 minutes**, naming the holder and the lock path; that only ever happens against a live gate that has hung, or a dead holder's lock directory that cannot be removed. `INSPECTOR_SKIP_GATE_LEASE=1` bypasses the lease — for a measurement that *needs* contention, never for a sooner result, since the queued run finishes before an overlapped one would. And the lease can never fail the gate it wraps: a lock that cannot be *created* (an unwritable tmpdir) runs the gate unleased with a warning.
The lease is released on every way out — a normal exit, a spawn failure, or a signal. On POSIX the wrapper runs the chain in its own process group, so Ctrl-C, a plain `kill`, a closed terminal or Ctrl-\ stop the whole tree (`npm run` nests four deep and `sh` forwards nothing), release, and exit `128 + signal`; on Windows there is no process group (`spawnSpec` spawns through `cmd.exe`, not detached), the signal is delivered to the immediate child only, and the lease is released just the same — what is not guaranteed there is that every descendant of that child has stopped. A holder that dies without releasing — a killed terminal, an OOM'd session — stops refreshing its lock and is taken over after **30s** (`proper-lockfile`'s stale detection, at 30s rather than the library's 10s default: a refresh timer firing late under the very load this exists to manage would otherwise cause a false takeover, which is two gates overlapping — the thing being prevented — whereas the cost of the longer window is 20 more seconds after a crash, against a gate that takes minutes). A waiter gives up after **45 minutes**, naming the holder and the lock path. That budget is a **total queueing budget**: it counts from the waiter's first attempt and is not reset when the holder ahead releases and another queued gate takes the lease, so it expires against a live gate that has hung, a dead holder's lock directory that cannot be removed, or a queue deeper than the budget covers — about ten gates at ~4.5 minutes each (see [capacity](#what-it-means-for-capacity)). A give-up behind a queue of healthy gates is that third case, not a hung holder; the line it prints names whichever gate holds the lease at that moment. `INSPECTOR_SKIP_GATE_LEASE=1` bypasses the lease — for a measurement that *needs* contention, never for a sooner result, since the queued run finishes before an overlapped one would. And the lease can never fail the gate it wraps: a lock that cannot be *created* (an unwritable tmpdir) runs the gate unleased with a warning.
### Why a queue, and not a load-based wait or a worker cap
Each alternative was ruled out by a measurement rather than an argument:
- **Overlapping gates are red by construction, so no budget or worker cap could make two of them green.** The web smokes bind fixed ports (6296–6299, plus the sandbox and app-origin ports). Measured for #2339 on `aa56551b`: a quiet gate took 257s and passed; two started together in separate worktrees had one **fail** at 279s — `smoke:web:chromium` found port 6298 held by the other gate's copy of the same smoke — while the survivor took 338s, ~1.3x, with zero test timeouts. That is a red run on a diff that cannot have caused it, the exact failure #2338 exists to stop, and only not overlapping fixes it. (The corollary corrected the effort's own framing: at two sessions the load inflation is ~1.3x; the 2.5x recorded on [#2323](https://github.com/modelcontextprotocol/inspector/issues/2323), with ~326 tests timing out at 5s, was the four-worktree number.)
- **Overlapping gates are red by construction, so no budget or worker cap could make two of them green.** The web smokes bind fixed ports — 6295–6299 by default (`smoke:web:tabs` 6295, `smoke:web:elicit` 6296, `smoke:web:app` 6297, `smoke:web:browser` 6298, `smoke:web` 6299), plus the sandbox and app-origin ports. Measured for #2339 on `aa56551b`: a quiet gate took 257s and passed; two started together in separate worktrees had one **fail** at 279s — `smoke:web:chromium` found port 6298 held by the other gate's copy of the same smoke — while the survivor took 338s, ~1.3x, with zero test timeouts. That is a red run on a diff that cannot have caused it, the exact failure #2338 exists to stop, and only not overlapping fixes it. (The corollary corrected the effort's own framing: at two sessions the load inflation is ~1.3x; the 2.5x recorded on [#2323](https://github.com/modelcontextprotocol/inspector/issues/2323), with ~326 tests timing out at 5s, was the four-worktree number.)
- **A load-threshold wait deadlocks.** Two sessions each polling "is the machine quiet yet?" never clear, because each is the reason the other is waiting — the trap #2339 recorded before any code existed. A lease *grants* instead: exactly one waiter wins the `mkdir`, runs, and releases; the rest keep asking, and a release always comes (see the stale takeover above). A `pgrep -f "npm run local:gate"` loop is the same trap from another side — it matches itself and never exits.
- **A worker cap is a permanent solo-run cost bought against a failure that does not occur.** Measured for #2336 (PR [#2345](https://github.com/modelcontextprotocol/inspector/pull/2345)) at the leased baseline, arms interleaved over three rounds: `maxWorkers: 4` cost +14% wall on web `unit` and +39% on `coverage:web` — ~35s on every `coverage:web`, a tenth of the whole gate — for zero fewer failures (0 across all 18 runs). With gates serialized, the oversubscription a cap would mitigate comes only from non-gate work in a sibling session, and against a concurrent bare unit run the total throughput was the same with both sides capped as with both at the default, because eight cores are saturated either way. So `maxWorkers` stays unset, stated as a choice beside `TIMEOUTS` in `vitest.shared.mts`.