From f99402918d665925e162158a2ea16367f5f83f8c Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 12 Sep 2026 22:00:33 -0400 Subject: [PATCH 1/5] feat: queue concurrent local:gate runs on a machine-wide lease (#2339) `npm run local:gate` now runs its stages under `scripts/gate-lease.mjs`, a `proper-lockfile` lease under `os.tmpdir()` (or `$XDG_RUNTIME_DIR`), so a gate started in a second worktree waits for the first instead of overlapping it. Measured on v2/main @ aa56551b: a quiet gate is 257s green; two started together had one fail at 279s (`smoke:web:chromium` found port 6298 held by the other gate's smoke) and the survivor take 338s. Under the lease, three gates started within seconds ran back to back (259s, then 262s, then the third), each green with zero timeouts. The lease grants rather than polls a threshold, so two waiters cannot deadlock on each other; a holder that dies stops refreshing and is taken over after 30s; a signal to the wrapper stops the whole process group, releases, and exits 128+signal; the wait names the holder's pid and worktree and gives up after 45 minutes. `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it. A lock that cannot be created runs the gate unleased with a warning. `scriptChainRuns` looks through the wrapper (and only it) so the existing vouches about what `local:gate` runs keep reading the real chain. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall --- .claude/skills/pre-push-gate/SKILL.md | 34 ++- AGENTS.md | 1 + docs/quality-gate.md | 2 +- package.json | 3 +- scripts/gate-lease.mjs | 401 ++++++++++++++++++++++++++ scripts/gate-lease.test.mjs | 327 +++++++++++++++++++++ scripts/lib/npm-scripts.mjs | 20 +- scripts/lib/workflow-gate.test.mjs | 30 +- scripts/run-engine-smokes.test.mjs | 4 +- scripts/verify-skills.main.test.mjs | 39 ++- 10 files changed, 849 insertions(+), 12 deletions(-) create mode 100644 scripts/gate-lease.mjs create mode 100644 scripts/gate-lease.test.mjs diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index 4e6a5fca..0edc56ce 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -148,9 +148,37 @@ Vite's `fs.allow`. Do a real `npm install` in the worktree. ### Everything times out at once -⚠️ Two concurrent `npm run local:gate` runs starve each other — ~326 tests time -out at 5s. Run one at a time. (A `pgrep -f "npm run local:gate"` wait loop -matches _itself_ and never exits.) +⚠️ Two `npm run local:gate` runs on one machine starve each other — under four +worktrees ~326 tests timed out at 5s (#2323), and even two collide +deterministically on the web smokes' fixed ports. Since #2339 the gate takes a +**machine-wide lease**, so a second run queues rather than overlapping; if +everything is still timing out at once, look for what is _not_ the gate: a +bypassed lease (`INSPECTOR_SKIP_GATE_LEASE` set in that shell), a bare +`npm run coverage` or `test:storybook` in another session, or Spotlight +indexing a fresh `node_modules` (a `mdworker` storm after `npm install` in a +new worktree pushed the load average to 20 for ten minutes). Do not write a +"wait until the machine is clear" loop — two of them deadlock on each other, +and a `pgrep -f "npm run local:gate"` loop matches _itself_ and never exits. + +### Waiting on the lease + +A gate that starts with + +``` +gate-lease: pid 12345 in /Users/you/Projects/mcp-inspector-wt-1, running for 2m10s holds the gate lease; waiting … +``` + +is queued behind another worktree's gate, and will start the moment it +releases (it re-checks every 2s and prints `still waiting` once a minute). The +holder's pid and worktree are in the line, so you can decide whether to wait +or to stop that gate. A holder that was **killed** — a closed terminal, an +OOM'd session — stops refreshing its lock and is taken over after 30s; nothing +needs cleaning up by hand. The wait gives up after 45 minutes, naming the +holder, which only ever happens against a live gate that has hung. + +`INSPECTOR_SKIP_GATE_LEASE=1 npm run local:gate` runs without the lease. It is +for a measurement that needs contention; it does not get a result sooner, +because the queued run finishes before an overlapped one would. ## Local-only steps diff --git a/AGENTS.md b/AGENTS.md index 49d93a09..3b34e928 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -476,6 +476,7 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race. - **`npm run local:gate` is the mandatory pre-push command.** It runs **every check** `.github/workflows/main.yml` runs, plus two local-only steps, so a green run here is the strongest predictor of a green CI this repo has: every check CI applies has already passed on your machine. It is not a proof — CI runs on a different OS and, since #2341, runs each client's suite bare where the gate runs it only instrumented — so the one residual is a test that passes *only* when slowed down, which is a race (#1596) to fix, never headroom to keep. Expect several minutes. - **The gate runs each client's test suite once, instrumented; CI runs it twice (#2341).** CI's `build` job runs the bare `test` inside `validate` and its parallel `coverage` job runs `test:coverage`, which costs CI no wall clock. Serially in one process the bare pass was ~80s of a ~370s gate, re-running exactly the files the coverage pass runs a few minutes later — so the gate calls **`local:validate`**, which is `validate` minus each client's `test` leg (every client's `validate` is `check && test`, and `local:validate` runs the `check` half). "Every check" is therefore a claim about checks, not invocations, and it holds because `@vitest/coverage-v8` collects coverage from V8's own profiler (`Profiler.takePreciseCoverage`) and rewrites no source: a test file sees identical code either way, and the instrumented run is only *slower*, which makes it the stricter of the two for the failure class this repo actually sees (a correct test cut off under load). A test that passed only *because* it ran slower would be a race — #1596's class, a defect wherever it surfaces — and CI's bare pass still runs it. **`npm run validate` is unchanged**, because CI runs it directly and it is the inner-loop check; `verify:*` guards that ask "is this reachable from `validate`" are unaffected. `local:validate` lives in the `local:` namespace so the workflow guard keeps it out of CI by construction. - **`npm run validate` is the fast inner-loop check and is NOT an acceptable substitute.** It runs `test`, not `test:coverage`, so it does **zero** coverage gating, no smokes, and no Storybook tests. Skipping the gate is how a push passes every fast local check and still fails CI. +- **Concurrent gates queue; they do not overlap.** `local:gate` takes a machine-wide lease (`scripts/gate-lease.mjs`, #2339) so a gate started in a second worktree waits for the first rather than running alongside it. Overlap is not merely slow — the web smokes bind fixed ports, so two gates reaching the same smoke together go red on a diff that cannot have caused it (measured: one of two concurrent gates failed at 279s on port 6298 while a quiet gate passed in 257s). The wait names the holder and its worktree; a holder that dies releases within 30s; `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it, which is for a measurement that _needs_ contention, never for getting a result sooner — the queued run finishes sooner anyway. - There is deliberately **no `npm run ci`** — that name collided with the `npm ci` built-in, which clean-installs from the lockfile and does not run this script. - What each stage covers, and why two of them are local-only, is [`docs/quality-gate.md`](./docs/quality-gate.md); how to diagnose a failing stage is the `pre-push-gate` skill. diff --git a/docs/quality-gate.md b/docs/quality-gate.md index 660f95f9..e6067d68 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -43,7 +43,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | | `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. A **second tier** (#2226) runs alongside it, asking the weaker but broader question the `AGENTS.md` rule actually states: does a package this repo *declares* anywhere resolve to two versions across our installs at all? Its candidate set is every name in any install's `dependencies`/`devDependencies`/`optionalDependencies` — unioned across the root and all four clients, so a copy declared by only one of them still counts — that **more than one install holds a top-level copy of** (17 packages today). Nested copies are excluded: one exists because some dependency asked for a different version, so it is that dependency's range to govern, not ours. Neither tier subsumes the other — the program tier sees a copy no manifest names (`@modelcontextprotocol/sdk`, arriving through another package's `.d.ts`), while the declared tier sees a **transitive** copy no program loads (cli's `@types/node`, hoisted via `@types/express` — the case that motivated it), two **clients** disagreeing with no root copy involved (`@types/react`, web against tui), and the peer shadows `eslint`/`typescript`/`vitest` that never enter a program. Same deny-by-default and same within-a-major rule, against its own `TOLERATED_DECLARED_SKEW` — also empty. Two limits: it reads **lockfiles**, so an uncommitted hand-installed copy is invisible, and it compares only declared names, so a purely transitive package no manifest names stays the first tier's business. Runs in `validate`. | `npm run verify:test-timeouts` | Guards the wall-clock budgets the test gates run under (#2323). The class it encodes against is a budget **nobody chose**: three of the six Vitest projects ran on Vitest's own `testTimeout: 5000` and five on its `hookTimeout`/`teardownTimeout: 10000`, sized for an idle machine rather than the one this team works on — three or four concurrent agent sessions in separate worktrees, each free to run a full `local:gate`, on eight logical cores. A correct, deterministic test cut off by such a budget fails a gate its diff did not break, which is the same "channel nobody trusts" failure [Lint has no warning tier](../AGENTS.md#lint-has-no-warning-tier) describes from the other direction; #2292, #1942 and #1742 were each an instance, found one site at a time. Deliberately **not** cited: #2278 (a missing condition wait around a geometry read) and #2250 (a real race in a test's own timing) were fixed by making the test wait for the right thing, and presenting a race fix as evidence for a larger ceiling would argue against #1596. It asks **Vitest itself** to resolve each of the four configs and reads the number a test actually gets, rather than checking that a key is absent from a config block — which would pass just as happily on a config that had stopped being loaded. A **seventh project** with no row, or a config file it does not discover (it knows all twelve filenames Vitest accepts, not just the two this repo uses), is an error rather than a silent skip. It also checks that every project loads `vitest.setup.shared.mts`. ⚠️ **Everything it reads comes from a resolved project, never from source text** — the two rules no config can report are asserted at runtime instead: `retry` by `vitest.setup.shared.mts` (which reads the value Vitest resolved, so a per-test option, a `describe` option, a project setting and a `--retry` flag are one check) and Testing Library's `asyncUtilTimeout` by `clients/web/src/test/asyncUtilTimeout.test.ts` (which reads what the project's own `waitFor`s use). Both began as source scanning in #2334 and the review found a new valid spelling missed in five consecutive rounds, so the boundary is deliberate: if a rule cannot be answered by asking the tool, assert it at runtime rather than reading the source for it. Observed **red** against the pre-#2323 config before it was trusted. Runs in `validate`; its decision logic has a sibling `verify-test-timeouts.test.mjs` under `test:scripts`. -| `npm run local:gate` | **Mandatory pre-push command.** `local:validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. Every GitHub CI check plus two local-only ones — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. | +| `npm run local:gate` | **Mandatory pre-push command.** `local:validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. Every GitHub CI check plus two local-only ones — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. **Runs under a machine-wide lease** (`scripts/gate-lease.mjs`, [#2339](https://github.com/modelcontextprotocol/inspector/issues/2339)): a second `local:gate` started in another worktree waits for the first to finish instead of running alongside it. Measured on `aa56551b`, a quiet gate took 257s; two started together had one **fail** at 279s on a smoke port collision (`smoke:web:chromium` binds 6298, and so did the other gate's) and the survivor take 338s — so overlapping gates are red by construction, not merely slow, and back to back both finish green in ~2x257s. The wait prints the holder's pid and worktree; a holder that dies without releasing is taken over after 30s (`proper-lockfile` stale detection); `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it. The stages themselves are `local:gate:stages`, which the wrapper runs verbatim. | | `npm run local:validate` | The gate's first stage: `validate` with each client's `test` leg removed (#2341) — the same `validate:guards` and `validate:core`, then every client's `check` — `format:check` + `lint` + `typecheck`, plus `build` for web, tui and launcher, which name one explicitly (cli's only validate-time build was `test`'s `pretest` hook, so it goes with the `test` leg and `coverage:cli` builds the binary once, later) — each client's `validate` being `check && test`. The suites run once, under `coverage`, instead of bare here and instrumented there. In the `local:` namespace so the workflow guard keeps it out of CI, where the bare pass is free. | | `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). | diff --git a/package.json b/package.json index f7042afe..ae019ccc 100644 --- a/package.json +++ b/package.json @@ -38,7 +38,8 @@ "build:tui": "cd clients/tui && npm run build", "build:web": "cd clients/web && npm run build", "build:launcher": "cd clients/launcher && npm run build", - "local:gate": "npm run local:validate && npm run verify:skills:cli && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run smoke:web:firefox && npm run local:storybook", + "local:gate": "node scripts/gate-lease.mjs npm run local:gate:stages", + "local:gate:stages": "npm run local:validate && npm run verify:skills:cli && npm run coverage && npm run verify:build-gate && npm run verify:bundle-externals && npm run smoke && npm run smoke:web:firefox && npm run local:storybook", "local:validate": "npm run validate:guards && npm run validate:core && cd clients/web && npm run check && cd ../cli && npm run check && cd ../tui && npm run check && cd ../launcher && npm run check", "local:storybook": "cd clients/web && npx playwright install chromium && npm run test:storybook", "verify:build-gate": "node scripts/verify-build-gate.mjs", diff --git a/scripts/gate-lease.mjs b/scripts/gate-lease.mjs new file mode 100644 index 00000000..2786c417 --- /dev/null +++ b/scripts/gate-lease.mjs @@ -0,0 +1,401 @@ +#!/usr/bin/env node +/** + * Run a command under a machine-wide lease, so that concurrent `npm run + * local:gate` runs in different worktrees queue instead of contending (#2339). + * + * **Two things go wrong when gates overlap, and both were measured** on + * `v2/main` at `aa56551b` (M3, 8 logical cores). A quiet gate took 257s and + * passed. Two started together in separate worktrees: one **failed** at 279s + * — `smoke:web:chromium` found its fixed port (6298) held by the other gate's + * copy of the same smoke — and the survivor took 338s, 1.3x, with the machine + * half-free for its last minute. So the first problem is deterministic, not a + * load effect: the web smokes bind fixed ports (6296–6299, plus the sandbox + * and app-origin ports), so two gates reaching the same smoke together produce + * a red run whose diff cannot have caused it — exactly the failure #2338 + * exists to stop. The second is load: with three or four sessions (#2323 + * measured sustained load averages of 50–70) every test budget is the wrong + * number. Run back to back, the same two gates finish green in ~2x257s, and + * each is measured against the quiet baseline every other #2338 aspect is + * tuned to. So a second gate waits. + * + * **Why a lease and not a load threshold.** Two sessions each polling "is the + * machine quiet yet?" deadlock on each other — neither ever clears, since each + * is the reason the other is waiting (#2323). A lease *grants*: exactly one + * waiter wins the `mkdir`, runs, and releases; the rest keep asking. There is + * nothing to wait *for* except a release, and a release always comes — see the + * crash case below. + * + * **Why `proper-lockfile`.** `core/auth/node/file-lock.ts` (#2082) records why + * a hand-rolled election loses to it: `mkdir` is atomic, a live holder + * refreshes the lock's mtime at `stale / 2` for as long as it lives, and a + * holder that dies without releasing — a killed terminal, an OOM'd session — + * stops refreshing, goes stale, and is taken over by the next waiter. That is + * the whole "how does a crashed holder release" answer, and it is the + * library's, not ours. The file-lock module's caveats about stale takeover + * being non-single-winner apply here too and matter less: the worst case is + * two gates running at once, which is exactly today's behaviour. + * + * **Why the lease is machine-wide.** The lock lives under `os.tmpdir()` + * (`$XDG_RUNTIME_DIR` where a desktop session sets it), never inside a + * worktree — a lock in the repo would be one per worktree, which is one per + * session, which coordinates nothing. + * + * **Why default-on.** The sessions that need it are the ones that did not + * think to opt in. `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it, and the first + * "waiting" line names the holder, its worktree, and that variable, so a wait + * is never a mystery. It is also never a gate: a lock that cannot be *created* + * (an unwritable tmpdir) runs the command unleased with a warning, because a + * coordination aid must not acquire a new way to fail the gate it wraps. + * + * **Why the whole gate and not its parallel stages.** The serial stages are + * not free — `lint` is type-aware and `build` is a full Vite bundle — and a + * per-stage lease would let two gates interleave into the same 2.5x. One + * lease around the whole run is the smallest thing that keeps the measured + * baseline meaningful. + * + * Usage: `node scripts/gate-lease.mjs [args...]`. The command runs + * with inherited stdio in its own process group, so a signal to this process + * stops the whole tree — `npm run` nests four deep and `sh` forwards nothing — + * and the lease is released before this process exits with `128 + signal`. + */ + +import { spawn } from "node:child_process"; +import { mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { constants as osConstants, tmpdir } from "node:os"; +import path from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; +import { fileURLToPath } from "node:url"; +// CJS-only package; a default import is the shape every loader agrees on. +import properLockfile from "proper-lockfile"; + +/** Set (to anything but `0` or empty) to run without taking the lease. */ +export const SKIP_ENV = "INSPECTOR_SKIP_GATE_LEASE"; + +/** Overrides where the lease lives. Mostly a test seam; also a shared-box escape. */ +export const DIR_ENV = "INSPECTOR_GATE_LEASE_DIR"; + +/** + * How long a lease may go unrefreshed before a waiter may take it over. + * + * `proper-lockfile` refreshes the lock's mtime at `stale / 2`, so this is not + * "how long a gate may run" — it is how long after a holder *dies* the queue + * stays blocked. 30s rather than the library's 10s default, on purpose: the + * refresh is a timer in an otherwise idle process, and the cost of a timer + * firing late (a laptop waking from sleep, an event loop starved by the very + * load this exists to manage) is a false takeover — two gates running at once, + * the thing being prevented — whereas the cost of the longer window is 20 + * more seconds of waiting after a crash, against a gate that takes minutes. + */ +export const STALE_MS = 30_000; + +/** How often a waiter re-asks. The acquire is one `mkdir`; this is not a hot loop. */ +export const POLL_MS = 2_000; + +/** How often a waiter says it is still waiting, so a long queue is visibly alive. */ +export const PROGRESS_MS = 60_000; + +/** + * How long a waiter waits before giving up. + * + * A dead holder releases within {@link STALE_MS}, so this only ever expires + * against a *live* holder that is still running after 45 minutes — four + * queued quiet gates are ~18 minutes, so a holder that long is a hung gate, and + * the right outcome is a loud failure naming it rather than a fifth process + * joining the pile. An unbounded wait would be a task that looks like progress + * and can never succeed. + */ +export const MAX_WAIT_MS = 45 * 60_000; + +/** Where the lease lives. `$XDG_RUNTIME_DIR` is per-user and per-session where it exists. */ +export function leaseDir(env = process.env) { + if (env[DIR_ENV]) return path.resolve(env[DIR_ENV]); + return path.join(env.XDG_RUNTIME_DIR || tmpdir(), "mcp-inspector-gate-lease"); +} + +/** + * The lease target: `proper-lockfile` locks `.lock` beside it and + * never opens the target itself, so the target doubles as the holder record + * (pid, worktree, start time) a waiter prints. It need not exist to be locked + * (`realpath: false` below). + */ +export function leaseTarget(dir) { + return path.join(dir, "local-gate"); +} + +/** The lock directory `proper-lockfile` creates for {@link leaseTarget}. */ +export function lockPathOf(dir) { + return `${leaseTarget(dir)}.lock`; +} + +/** `INSPECTOR_SKIP_GATE_LEASE=0` and an empty value both mean "not skipped". */ +export function isSkipped(env = process.env) { + const value = env[SKIP_ENV]; + return value !== undefined && value !== "" && value !== "0"; +} + +/** `95s` → `1m35s`; sub-minute values stay in seconds. */ +export function formatDuration(ms) { + const s = Math.max(0, Math.round(ms / 1000)); + if (s < 60) return `${s}s`; + return `${Math.floor(s / 60)}m${String(s % 60).padStart(2, "0")}s`; +} + +/** + * The exit code to report for a finished child: its own, or `128 + signal` + * (the shell convention) when a signal ended it. `code` and `signal` are + * mutually exclusive on a real ChildProcess exit. + */ +export function exitCodeFor({ code, signal }) { + if (typeof code === "number") return code; + return 128 + (osConstants.signals[signal] ?? 0); +} + +/** The holder record the current holder wrote, or `null` if unreadable or malformed. */ +export function readHolder(dir) { + try { + const record = JSON.parse(readFileSync(leaseTarget(dir), "utf8")); + if ( + typeof record?.pid === "number" && + typeof record?.cwd === "string" && + typeof record?.startedAt === "number" + ) { + return record; + } + } catch { + // Missing, unreadable, or not JSON — all mean "nothing to report". + } + return null; +} + +/** One line a waiter can act on: who holds it, where, and for how long. */ +export function describeHolder(holder, now = Date.now()) { + if (holder === null) return "another local:gate"; + return `pid ${holder.pid} in ${holder.cwd}, running for ${formatDuration(now - holder.startedAt)}`; +} + +/** + * Send `signal` to the child's whole process group (it was spawned as a group + * leader), falling back to the child alone where groups are unavailable or the + * group is already gone. + */ +function signalTree(child, signal) { + try { + if (process.platform !== "win32") { + process.kill(-child.pid, signal); + return; + } + } catch { + // ESRCH: the group is already gone, or the child never became a leader. + } + try { + child.kill(signal); + } catch { + // Already exited; nothing to signal. + } +} + +/** + * Take the lease, waiting for a holder to release or go stale. + * + * Returns the release function, or `null` when a lock cannot be created here + * at all — the caller then runs unleased. Throws only when the wait budget is + * exhausted against a live holder. + */ +async function acquireLease({ dir, log, pollMs, progressMs, maxWaitMs }) { + const target = leaseTarget(dir); + const startedWaiting = Date.now(); + let lastProgress = startedWaiting; + let announced = false; + for (;;) { + try { + return await properLockfile.lock(target, { + realpath: false, + stale: STALE_MS, + retries: 0, + // The library's default throws from a timer with no caller on the + // stack, which would take the *holder* down mid-gate. A compromised + // lease means another gate is now running alongside this one — worth + // saying, not worth killing a gate that is otherwise fine. + onCompromised: (err) => + log( + `gate-lease: another process took the lease over while this gate was running (${err.message}); continuing without it.`, + ), + }); + } catch (err) { + if (err?.code !== "ELOCKED") { + log( + `gate-lease: could not take the lease at ${lockPathOf(dir)} (${err?.message ?? err}); running without it.`, + ); + return null; + } + } + const waited = Date.now() - startedWaiting; + if (!announced) { + announced = true; + log( + `gate-lease: ${describeHolder(readHolder(dir))} holds the gate lease; waiting for it to finish so the two do not contend. ${SKIP_ENV}=1 runs anyway.`, + ); + } else if (Date.now() - lastProgress >= progressMs) { + lastProgress = Date.now(); + log( + `gate-lease: still waiting (${formatDuration(waited)}) on ${describeHolder(readHolder(dir))}.`, + ); + } + if (waited >= maxWaitMs) { + throw new Error( + `gate-lease: gave up after ${formatDuration(waited)} — ${describeHolder(readHolder(dir))} still holds ${lockPathOf(dir)}. If that gate is hung, stop it; ${SKIP_ENV}=1 runs without the lease.`, + ); + } + await delay(pollMs); + } +} + +/** + * Run `command args` holding the lease, and resolve with the exit code to + * report. Signals to this process stop the child's whole tree first and are + * reported as `128 + signal` after the lease is released. + * + * Every knob is injectable so the tests can drive a real lock, a real child + * and a real signal without waiting real minutes. + * + * @param {object} opts + * @param {string} opts.command + * @param {string[]} opts.args + * @param {string} [opts.dir] lease directory; default {@link leaseDir} + * @param {NodeJS.ProcessEnv} [opts.env] + * @param {(line: string) => void} [opts.log] + * @param {number} [opts.pollMs] + * @param {number} [opts.progressMs] + * @param {number} [opts.maxWaitMs] + * @param {number} [opts.graceMs] SIGTERM → SIGKILL escalation on a signal + * @param {import("node:child_process").StdioOptions} [opts.stdio] + * @returns {Promise} + */ +export async function runUnderLease({ + command, + args, + dir = leaseDir(), + env = process.env, + log = (line) => console.error(line), + pollMs = POLL_MS, + progressMs = PROGRESS_MS, + maxWaitMs = MAX_WAIT_MS, + graceMs = 5_000, + stdio = "inherit", +}) { + let release = null; + let waited = 0; + if (isSkipped(env)) { + log(`gate-lease: ${SKIP_ENV} is set; running without the lease.`); + } else { + let usable = true; + try { + mkdirSync(dir, { recursive: true }); + } catch (err) { + usable = false; + log( + `gate-lease: could not create ${dir} (${err?.message ?? err}); running without the lease.`, + ); + } + if (usable) { + const startedWaiting = Date.now(); + release = await acquireLease({ dir, log, pollMs, progressMs, maxWaitMs }); + waited = Date.now() - startedWaiting; + } + if (release !== null) { + if (waited >= pollMs) { + log(`gate-lease: acquired after ${formatDuration(waited)}.`); + } + try { + writeFileSync( + leaseTarget(dir), + JSON.stringify({ + pid: process.pid, + cwd: process.cwd(), + startedAt: Date.now(), + }), + ); + } catch { + // Best effort: the record only improves a waiter's message. + } + } + } + + const startedRunning = Date.now(); + const child = spawn(command, args, { + stdio, + env, + // Its own group, so one signal reaches every descendant. See the header. + detached: process.platform !== "win32", + }); + + const signals = ["SIGINT", "SIGTERM", "SIGHUP"]; + let stoppedBy = null; + let escalation = null; + const onSignal = (signal) => { + if (stoppedBy !== null) return; + stoppedBy = signal; + log(`gate-lease: received ${signal}; stopping the gate.`); + signalTree(child, signal); + escalation = setTimeout(() => { + log( + `gate-lease: the gate did not exit within ${graceMs}ms of ${signal}; sending SIGKILL.`, + ); + signalTree(child, "SIGKILL"); + }, graceMs); + }; + const handlers = signals.map((signal) => { + const handler = () => onSignal(signal); + process.on(signal, handler); + return [signal, handler]; + }); + + const outcome = await new Promise((resolve, reject) => { + child.once("error", reject); + child.once("exit", (code, signal) => resolve({ code, signal })); + }).finally(() => { + clearTimeout(escalation); + for (const [signal, handler] of handlers) process.off(signal, handler); + }); + + if (release !== null) { + // The record is only meaningful while the lock beside it is held; a + // waiter reading a fresh lock must not be told about the previous holder. + rmSync(leaseTarget(dir), { force: true }); + try { + await release(); + } catch (err) { + log( + `gate-lease: could not release the lease (${err?.message ?? err}); it goes stale on its own after ${formatDuration(STALE_MS)}.`, + ); + } + log( + `gate-lease: released after ${formatDuration(Date.now() - startedRunning)}${waited >= pollMs ? ` (waited ${formatDuration(waited)} first)` : ""}.`, + ); + } + return stoppedBy !== null + ? exitCodeFor({ code: null, signal: stoppedBy }) + : exitCodeFor(outcome); +} + +/** `node scripts/gate-lease.mjs [args...]` */ +export async function main(argv = process.argv.slice(2)) { + const [command, ...args] = argv; + if (!command) { + console.error("usage: node scripts/gate-lease.mjs [args...]"); + return 2; + } + try { + return await runUnderLease({ command, args }); + } catch (err) { + console.error(err?.message ?? err); + return 1; + } +} + +if ( + process.argv[1] && + path.resolve(process.argv[1]) === fileURLToPath(import.meta.url) +) { + process.exitCode = await main(); +} diff --git a/scripts/gate-lease.test.mjs b/scripts/gate-lease.test.mjs new file mode 100644 index 00000000..474c291c --- /dev/null +++ b/scripts/gate-lease.test.mjs @@ -0,0 +1,327 @@ +// Tests for the local:gate lease (#2339). The pure helpers are table-driven; +// the lease itself is exercised against a REAL `proper-lockfile` lock, a real +// child process and — for the signal path — a real signal to a real wrapper +// process, each against a throwaway lease directory so no test can queue +// behind (or block) a developer's actual gate. Run via `npm run test:scripts` +// (node:test; the root has no vitest harness). + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { spawn } from "node:child_process"; +import { + existsSync, + mkdirSync, + mkdtempSync, + readFileSync, + utimesSync, + writeFileSync, +} from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { setTimeout as delay } from "node:timers/promises"; +import { fileURLToPath } from "node:url"; + +import { + DIR_ENV, + SKIP_ENV, + STALE_MS, + describeHolder, + exitCodeFor, + formatDuration, + isSkipped, + leaseDir, + leaseTarget, + lockPathOf, + main, + readHolder, + runUnderLease, +} from "./gate-lease.mjs"; + +const SCRIPT = fileURLToPath(new URL("./gate-lease.mjs", import.meta.url)); + +/** A fresh lease directory per test, so tests never contend with each other. */ +function freshDir() { + return mkdtempSync(join(tmpdir(), "gate-lease-test-")); +} + +function collectLog() { + const lines = []; + return { lines, log: (line) => lines.push(line) }; +} + +/** Run `node -e ` under the lease with quiet stdio and fast knobs. */ +function runNode(source, opts = {}) { + return runUnderLease({ + command: process.execPath, + args: ["-e", source], + stdio: "ignore", + pollMs: 25, + progressMs: 60_000, + ...opts, + }); +} + +/** Poll until `predicate()` holds or `timeoutMs` passes. */ +async function waitFor(predicate, timeoutMs = 5_000) { + const deadline = Date.now() + timeoutMs; + while (!predicate()) { + assert.ok(Date.now() < deadline, "condition not met in time"); + await delay(25); + } +} + +function isAlive(pid) { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +test("leaseDir: explicit override, then XDG_RUNTIME_DIR, then tmpdir", () => { + assert.equal(leaseDir({ [DIR_ENV]: "/x/y" }), "/x/y"); + assert.equal( + leaseDir({ XDG_RUNTIME_DIR: "/run/user/1" }), + join("/run/user/1", "mcp-inspector-gate-lease"), + ); + assert.equal(leaseDir({}), join(tmpdir(), "mcp-inspector-gate-lease")); + // The lock is a sibling of the target, never inside it — a file inside the + // lock directory would make its removal (and so stale takeover) fail. + assert.equal(lockPathOf("/d"), `${leaseTarget("/d")}.lock`); +}); + +test("isSkipped: unset, empty and `0` mean the lease is on", () => { + for (const [env, expected] of [ + [{}, false], + [{ [SKIP_ENV]: "" }, false], + [{ [SKIP_ENV]: "0" }, false], + [{ [SKIP_ENV]: "1" }, true], + [{ [SKIP_ENV]: "true" }, true], + ]) { + assert.equal(isSkipped(env), expected, JSON.stringify(env)); + } +}); + +test("formatDuration: seconds under a minute, m/ss above", () => { + for (const [ms, expected] of [ + [0, "0s"], + [-500, "0s"], + [4_400, "4s"], + [59_499, "59s"], + [60_000, "1m00s"], + [95_000, "1m35s"], + [3_605_000, "60m05s"], + ]) { + assert.equal(formatDuration(ms), expected, String(ms)); + } +}); + +test("exitCodeFor: the child's code, else 128 + signal", () => { + assert.equal(exitCodeFor({ code: 0, signal: null }), 0); + assert.equal(exitCodeFor({ code: 7, signal: null }), 7); + assert.equal(exitCodeFor({ code: null, signal: "SIGTERM" }), 143); + assert.equal(exitCodeFor({ code: null, signal: "SIGINT" }), 130); + assert.equal(exitCodeFor({ code: null, signal: "SIGNOPE" }), 128); +}); + +test("readHolder/describeHolder: a missing or malformed record is not an error", () => { + const dir = freshDir(); + assert.equal(readHolder(dir), null); + assert.equal(describeHolder(null), "another local:gate"); + writeFileSync(leaseTarget(dir), "not json"); + assert.equal(readHolder(dir), null); + writeFileSync(leaseTarget(dir), JSON.stringify({ pid: "12", cwd: "/w" })); + assert.equal(readHolder(dir), null); + const record = { pid: 12, cwd: "/w", startedAt: 1_000 }; + writeFileSync(leaseTarget(dir), JSON.stringify(record)); + assert.deepEqual(readHolder(dir), record); + assert.equal( + describeHolder(record, 96_000), + "pid 12 in /w, running for 1m35s", + ); +}); + +test("two concurrent runs serialize: the second starts after the first ends", async () => { + const dir = freshDir(); + const marks = join(dir, "marks.log"); + const { lines, log } = collectLog(); + const body = (label) => + `const fs=require("fs");const f=${JSON.stringify(marks)};` + + `fs.appendFileSync(f,"${label} start "+Date.now()+"\\n");` + + `setTimeout(()=>fs.appendFileSync(f,"${label} end "+Date.now()+"\\n"),300)`; + const first = runNode(body("a"), { dir, log }); + // Give the first run the lock — and its holder record, which is written + // just after — before the second asks, so which one waits, and what it is + // told, are not themselves races the assertions have to allow for. + await waitFor(() => existsSync(leaseTarget(dir))); + const second = runNode(body("b"), { dir, log }); + assert.deepEqual(await Promise.all([first, second]), [0, 0]); + + const marksByLabel = Object.fromEntries( + readFileSync(marks, "utf8") + .trim() + .split("\n") + .map((line) => line.split(" ")) + .map(([label, phase, at]) => [`${label} ${phase}`, Number(at)]), + ); + assert.ok( + marksByLabel["b start"] >= marksByLabel["a end"], + `b started (${marksByLabel["b start"]}) before a ended (${marksByLabel["a end"]})`, + ); + // The waiter said who it was waiting on, named the bypass, and reported the + // wait when it finally acquired; the uncontended run said nothing on entry. + assert.equal( + lines.filter((l) => l.includes("holds the gate lease")).length, + 1, + ); + assert.match( + lines.find((l) => l.includes("holds the gate lease")), + /pid \d+ in .+, running for/, + ); + assert.match( + lines.find((l) => l.includes("holds the gate lease")), + new RegExp(`${SKIP_ENV}=1`), + ); + assert.equal( + lines.filter((l) => l.startsWith("gate-lease: acquired after")).length, + 1, + ); + assert.equal( + lines.filter((l) => l.startsWith("gate-lease: released after")).length, + 2, + ); + assert.ok(!existsSync(lockPathOf(dir)), "the lock is released at the end"); + assert.ok(!existsSync(leaseTarget(dir)), "the holder record goes with it"); +}); + +test("the child's exit code is the run's exit code", async () => { + const dir = freshDir(); + assert.equal(await runNode("process.exit(7)", { dir, log: () => {} }), 7); +}); + +test("a stale lock (its holder died) is taken over, not waited on", async () => { + const dir = freshDir(); + mkdirSync(lockPathOf(dir), { recursive: true }); + const dead = new Date(Date.now() - STALE_MS - 1_000); + utimesSync(lockPathOf(dir), dead, dead); + const { lines, log } = collectLog(); + assert.equal(await runNode("", { dir, log }), 0); + assert.equal( + lines.filter((l) => l.includes("holds the gate lease")).length, + 0, + ); + assert.ok(!existsSync(lockPathOf(dir))); +}); + +test("a live lock that never releases fails the wait loudly, naming the holder", async () => { + const dir = freshDir(); + // A fresh lock directory nobody refreshes is indistinguishable from a live + // holder for STALE_MS, which is longer than this test is prepared to wait. + mkdirSync(lockPathOf(dir), { recursive: true }); + writeFileSync( + leaseTarget(dir), + JSON.stringify({ + pid: 424242, + cwd: "/some/worktree", + startedAt: Date.now(), + }), + ); + const { lines, log } = collectLog(); + await assert.rejects(runNode("", { dir, log, maxWaitMs: 0 }), (err) => { + assert.match(err.message, /gave up after/); + assert.match(err.message, /pid 424242 in \/some\/worktree/); + assert.match(err.message, new RegExp(`${SKIP_ENV}=1`)); + return true; + }); + assert.equal( + lines.filter((l) => l.includes("holds the gate lease")).length, + 1, + ); + // The lock was somebody else's; giving up must not remove it. + assert.ok(existsSync(lockPathOf(dir))); +}); + +test("a waiter reports progress while it waits", async () => { + const dir = freshDir(); + mkdirSync(lockPathOf(dir), { recursive: true }); + const { lines, log } = collectLog(); + await assert.rejects( + runNode("", { dir, log, maxWaitMs: 120, pollMs: 30, progressMs: 0 }), + /gave up/, + ); + assert.ok( + lines.some((l) => l.startsWith("gate-lease: still waiting")), + lines.join("\n"), + ); +}); + +test(`${SKIP_ENV}=1 runs without touching the lease`, async () => { + const dir = freshDir(); + const { lines, log } = collectLog(); + const env = { ...process.env, [SKIP_ENV]: "1" }; + assert.equal(await runNode("", { dir, log, env }), 0); + assert.match(lines.join("\n"), new RegExp(`${SKIP_ENV} is set`)); + assert.ok(!existsSync(lockPathOf(dir))); +}); + +test("a lease directory that cannot exist degrades to an unleased run", async () => { + const parent = freshDir(); + const file = join(parent, "a-file"); + writeFileSync(file, ""); + const { lines, log } = collectLog(); + assert.equal(await runNode("", { dir: join(file, "nested"), log }), 0); + assert.match( + lines.join("\n"), + /could not create .*; running without the lease/, + ); +}); + +test("main: no command is a usage error", async () => { + const { error } = console; + const lines = []; + console.error = (line) => lines.push(line); + try { + assert.equal(await main([]), 2); + } finally { + console.error = error; + } + assert.match(lines.join("\n"), /usage:/); +}); + +test("a signal to the wrapper stops the whole tree, releases the lease and exits 128+signal", async () => { + const dir = freshDir(); + const pidFile = join(dir, "grandchild.pid"); + // The gate stand-in: records its pid, then idles forever unless signalled. + const idle = + `require("fs").writeFileSync(${JSON.stringify(pidFile)}, String(process.pid));` + + `setInterval(() => {}, 1000)`; + const wrapper = spawn( + process.execPath, + [SCRIPT, process.execPath, "-e", idle], + { + env: { ...process.env, [DIR_ENV]: dir }, + stdio: ["ignore", "ignore", "pipe"], + }, + ); + let stderr = ""; + wrapper.stderr.on("data", (chunk) => (stderr += chunk)); + const exited = new Promise((resolve) => + wrapper.once("exit", (code, signal) => resolve({ code, signal })), + ); + + await waitFor(() => existsSync(pidFile)); + const grandchild = Number(readFileSync(pidFile, "utf8")); + assert.ok(isAlive(grandchild)); + assert.ok(existsSync(lockPathOf(dir)), "the wrapper held the lease"); + + wrapper.kill("SIGTERM"); + const outcome = await exited; + assert.deepEqual(outcome, { code: 143, signal: null }, stderr); + await waitFor(() => !isAlive(grandchild)); + assert.ok( + !existsSync(lockPathOf(dir)), + "the lease was released on the way out", + ); + assert.match(stderr, /received SIGTERM; stopping the gate/); +}); diff --git a/scripts/lib/npm-scripts.mjs b/scripts/lib/npm-scripts.mjs index 91c48ee6..9ea15bf4 100644 --- a/scripts/lib/npm-scripts.mjs +++ b/scripts/lib/npm-scripts.mjs @@ -50,6 +50,16 @@ export function reachableScripts(scripts, entry = "validate") { return reached; } +/** The lease wrapper `local:gate` runs under (#2339); see `scripts/gate-lease.mjs`. */ +export const GATE_LEASE_WRAPPER = "node scripts/gate-lease.mjs "; + +/** `node scripts/gate-lease.mjs npm run X` → `npm run X`; anything else unchanged. */ +function unwrapGateLease(segment) { + return segment.startsWith(GATE_LEASE_WRAPPER) + ? segment.slice(GATE_LEASE_WRAPPER.length).trim() + : segment; +} + /** The command strings of every script reachable from the root `validate`. */ function rootReachedCommands(rootScripts) { return [...reachableScripts(rootScripts)] @@ -108,6 +118,12 @@ export function rootReachesScript(rootScripts, scriptName) { * flags, as are npm's implicit `pre`/`post` hooks. Only the target is matched * exactly, which suits the argument-less scripts a vouch asks about. * + * One wrapper is transparent: `node scripts/gate-lease.mjs npm run X` runs + * `npm run X` under the local:gate lease (#2339) and nothing else, so a vouch + * about what `local:gate` runs looks through it. It is matched by its exact + * leading tokens — a wrapper with flags, or any other wrapper, still hides the + * invocation, because this helper cannot know what an arbitrary wrapper does. + * * @param {Record} scripts * @param {string} entry * @param {string} target @@ -124,7 +140,9 @@ export function scriptChainRuns(scripts, entry, target) { if (typeof scripts?.[hook] === "string") queue.push(hook); const body = scripts?.[name]; if (typeof body !== "string") continue; - for (const segment of body.split(/\n|;|&&/).map((part) => part.trim())) { + for (const segment of body + .split(/\n|;|&&/) + .map((part) => unwrapGateLease(part.trim()))) { if (segment === `npm run ${target}`) return true; const tokens = segment.split(/\s+/); if (tokens[0] === "npm" && tokens[1] === "run" && tokens[2]) { diff --git a/scripts/lib/workflow-gate.test.mjs b/scripts/lib/workflow-gate.test.mjs index 8525c09e..5ddfc395 100644 --- a/scripts/lib/workflow-gate.test.mjs +++ b/scripts/lib/workflow-gate.test.mjs @@ -23,7 +23,11 @@ import { findWorkflowViolations, formatWorkflowViolations, } from "./workflow-gate.mjs"; -import { reachableScripts } from "./npm-scripts.mjs"; +import { + GATE_LEASE_WRAPPER, + reachableScripts, + scriptChainRuns, +} from "./npm-scripts.mjs"; const repoRoot = join(import.meta.dirname, "..", ".."); const workflowDir = join(repoRoot, ".github", "workflows"); @@ -607,7 +611,22 @@ describe("the gate's name", () => { it("is `local:gate`, and runs the local-only Storybook step", () => { assert.ok(scripts["local:gate"], "the pre-push gate must exist"); - assert.match(scripts["local:gate"], /local:storybook/); + assert.match(scripts["local:gate:stages"], /local:storybook/); + }); + + it("runs its stages under the lease wrapper, and nothing else (#2339)", () => { + // The wrapper serializes gates across worktrees. It must be the ONLY thing + // `local:gate` does — a stage placed beside it would run outside the + // lease — and it must run exactly the stages script, so every assertion + // below about what the gate runs can read `local:gate:stages` directly. + assert.equal( + scripts["local:gate"], + `${GATE_LEASE_WRAPPER}npm run local:gate:stages`, + ); + assert.ok( + scriptChainRuns(scripts, "local:gate", "local:storybook"), + "the wrapper is transparent to a chain vouch", + ); }); describe("runs each client's test suite once, not twice (#2341)", () => { @@ -631,8 +650,11 @@ describe("the gate's name", () => { ); it("starts from `local:validate`, and never from `validate`", () => { - assert.match(scripts["local:gate"], /^npm run local:validate && /); - assert.doesNotMatch(scripts["local:gate"], /\brun validate(?=$|[\s&;])/); + assert.match(scripts["local:gate:stages"], /^npm run local:validate && /); + assert.doesNotMatch( + scripts["local:gate:stages"], + /\brun validate(?=$|[\s&;])/, + ); // Nothing reachable from the WHOLE gate — not just its first stage — // may run `validate` or a client's `validate:*`: those are the doors // through which the bare `test` leg would come back, and a later stage diff --git a/scripts/run-engine-smokes.test.mjs b/scripts/run-engine-smokes.test.mjs index 78aff741..6facad60 100644 --- a/scripts/run-engine-smokes.test.mjs +++ b/scripts/run-engine-smokes.test.mjs @@ -17,6 +17,7 @@ import { join } from "node:path"; import { describe, it } from "node:test"; import { ENGINE_SMOKES } from "./run-engine-smokes.mjs"; import { SUPPORTED_BROWSERS } from "./lib/headless-browser.mjs"; +import { scriptChainRuns } from "./lib/npm-scripts.mjs"; const scriptDir = import.meta.dirname; const scripts = JSON.parse( @@ -73,7 +74,8 @@ describe("every engine tier consumes ENGINE_SMOKES", () => { it("the Firefox tier goes through the runner and is in the pre-push gate", () => { assert.match(scripts["smoke:web:firefox"], /run-engine-smokes\.mjs/); - assert.match(scripts["local:gate"], /smoke:web:firefox/); + // Through the #2339 lease wrapper, which `scriptChainRuns` looks past. + assert.ok(scriptChainRuns(scripts, "local:gate", "smoke:web:firefox")); }); it("every supported engine has a `smoke:web:` script", () => { diff --git a/scripts/verify-skills.main.test.mjs b/scripts/verify-skills.main.test.mjs index 7704ec78..78a911e0 100644 --- a/scripts/verify-skills.main.test.mjs +++ b/scripts/verify-skills.main.test.mjs @@ -30,7 +30,7 @@ import { ciRunsUnconditionally, runsCommand, } from "./verify-skills.mjs"; -import { scriptChainRuns } from "./lib/npm-scripts.mjs"; +import { GATE_LEASE_WRAPPER, scriptChainRuns } from "./lib/npm-scripts.mjs"; const SCRIPT = path.join( path.dirname(fileURLToPath(import.meta.url)), @@ -491,6 +491,43 @@ test("scriptChainRuns follows the chain to a real invocation", () => { ); }); +test("scriptChainRuns looks through the local:gate lease wrapper, and only it", () => { + // `local:gate` runs its stages under `scripts/gate-lease.mjs` (#2339). The + // wrapper runs exactly the `npm run …` it is handed, so a vouch sees past + // it; a wrapper carrying flags, or any other wrapper, still hides the chain. + const T = "verify:skills:cli"; + const stages = `npm run validate && npm run ${T}`; + assert.equal( + scriptChainRuns( + { + "local:gate": `${GATE_LEASE_WRAPPER}npm run local:gate:stages`, + "local:gate:stages": stages, + }, + "local:gate", + T, + ), + true, + ); + assert.equal( + scriptChainRuns( + { "local:gate": `${GATE_LEASE_WRAPPER}npm run ${T}` }, + "local:gate", + T, + ), + true, + ); + for (const wrapped of [ + `node scripts/gate-lease.mjs --quiet npm run ${T}`, + `node scripts/other-wrapper.mjs npm run ${T}`, + ]) { + assert.equal( + scriptChainRuns({ "local:gate": wrapped }, "local:gate", T), + false, + wrapped, + ); + } +}); + test("scriptChainRuns is not satisfied by a mention or a longer name", () => { // `reachableScripts` extracts any `npm run …` substring, so both of these // passed the old check while the validator never ran. From 60bbe7675f4d226c3b18df993ea19a1f762a4461 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 12 Sep 2026 22:15:52 -0400 Subject: [PATCH 2/5] fix: address Copilot review round 1 (#2339) - A non-ELOCKED acquire error with the lock directory still present (a stale lock the library could not remove: ENOTEMPTY, EACCES, EROFS) now waits like a held lock instead of degrading to an unleased run; only a lock that cannot be created at all degrades. - Spawn with a shell on Windows (npm is npm.cmd there) and quote the arguments for cmd.exe via the existing winShellArgs; exported as spawnSpec so the branch is unit-tested from POSIX. - Release the lease on every way out, including a spawn failure, via a finally around the child wait. - Guard every lock removal (release and the library's exit handler) on the directory's identity, mirroring core/auth/node/file-lock.ts, so a holder whose lock was taken over never removes the winner's. - The signal test stops the wrapper in a finally, so a failed setup assertion cannot leak its idle grandchild. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall --- scripts/gate-lease.mjs | 177 ++++++++++++++++++++++++++++++------ scripts/gate-lease.test.mjs | 104 +++++++++++++++++++-- 2 files changed, 249 insertions(+), 32 deletions(-) diff --git a/scripts/gate-lease.mjs b/scripts/gate-lease.mjs index 2786c417..393201a4 100644 --- a/scripts/gate-lease.mjs +++ b/scripts/gate-lease.mjs @@ -60,13 +60,22 @@ */ import { spawn } from "node:child_process"; -import { mkdirSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import nodeFs, { + existsSync, + mkdirSync, + readFileSync, + rmSync, + rmdirSync, + statSync, + writeFileSync, +} from "node:fs"; import { constants as osConstants, tmpdir } from "node:os"; import path from "node:path"; import { setTimeout as delay } from "node:timers/promises"; import { fileURLToPath } from "node:url"; // CJS-only package; a default import is the shape every loader agrees on. import properLockfile from "proper-lockfile"; +import { winShellArgs } from "./lib/win-shell-args.mjs"; /** Set (to anything but `0` or empty) to run without taking the lease. */ export const SKIP_ENV = "INSPECTOR_SKIP_GATE_LEASE"; @@ -173,6 +182,90 @@ export function describeHolder(holder, now = Date.now()) { return `pid ${holder.pid} in ${holder.cwd}, running for ${formatDuration(now - holder.startedAt)}`; } +/** + * A lock directory's identity: what changes when it is removed and recreated. + * `ino` and birth time rather than mtime, which the holder's own refresh + * rewrites legitimately; `null` when it cannot be read. + */ +function identify(lockPath) { + try { + const stat = statSync(lockPath); + return { ino: stat.ino, birthtimeMs: stat.birthtimeMs }; + } catch { + return null; + } +} + +/** + * A `proper-lockfile` `fs` shim whose directory removal refuses to delete a + * lock that is no longer the one this process created. + * + * The library's stale takeover is not single-winner (see + * `core/auth/node/file-lock.ts`, whose guard this mirrors): a holder whose + * refresh timer was starved past {@link STALE_MS} can have its directory + * replaced by a waiter, and its `release()` — and the library's `signal-exit` + * handler — would then `rmdir` the *winner's* lock by path, letting a third + * gate in beside the winner. Every removal the library performs goes through + * this object, so guarding here covers both paths. Identity is captured in + * the `mkdir` callback, the moment the directory becomes ours; `owned.id` + * stays `null` until then so a stale directory the library removes on the + * way to acquiring passes through untouched. + */ +function guardedFs(lockPath, owned, onRefused) { + const mine = () => { + if (owned.id === null) return true; + const now = identify(lockPath); + if (now === null) return false; + return now.ino === owned.id.ino && now.birthtimeMs === owned.id.birthtimeMs; + }; + const removeIfMine = () => { + if (!mine()) { + onRefused(); + return; + } + rmdirSync(lockPath); + }; + return { + fs: { + ...nodeFs, + mkdir: (p, cb) => + nodeFs.mkdir(p, (err) => { + if (!err) owned.id = identify(lockPath); + cb(err); + }), + // Reported as success when refused, so the library forgets the lock + // either way rather than handing its exit handler a stale record. + rmdir: (_p, cb) => { + try { + removeIfMine(); + cb(null); + } catch (err) { + cb(err); + } + }, + rmdirSync: () => removeIfMine(), + }, + mine, + }; +} + +/** + * How to spawn `command args` on this platform. Shell-free everywhere but + * Windows, where `npm` is `npm.cmd` and needs `cmd.exe` to start at all + * (Node refuses shell-free `.cmd` spawns), so the arguments are quoted for + * it. The process group that lets one signal reach the whole tree is a POSIX + * notion, hence `detached` only there. + */ +export function spawnSpec(command, args, platform = process.platform) { + const win32 = platform === "win32"; + return { + command, + args: winShellArgs(args, platform), + shell: win32, + detached: !win32, + }; +} + /** * Send `signal` to the child's whole process group (it was spawned as a group * leader), falling back to the child alone where groups are unavailable or the @@ -201,7 +294,7 @@ function signalTree(child, signal) { * at all — the caller then runs unleased. Throws only when the wait budget is * exhausted against a live holder. */ -async function acquireLease({ dir, log, pollMs, progressMs, maxWaitMs }) { +async function acquireLease({ dir, fs, log, pollMs, progressMs, maxWaitMs }) { const target = leaseTarget(dir); const startedWaiting = Date.now(); let lastProgress = startedWaiting; @@ -212,6 +305,7 @@ async function acquireLease({ dir, log, pollMs, progressMs, maxWaitMs }) { realpath: false, stale: STALE_MS, retries: 0, + fs, // The library's default throws from a timer with no caller on the // stack, which would take the *holder* down mid-gate. A compromised // lease means another gate is now running alongside this one — worth @@ -222,7 +316,13 @@ async function acquireLease({ dir, log, pollMs, progressMs, maxWaitMs }) { ), }); } catch (err) { - if (err?.code !== "ELOCKED") { + // `ELOCKED` is not the only "someone holds it": a stale directory the + // library could not remove (`ENOTEMPTY`, `EACCES`, `EROFS`) surfaces as + // an ordinary error, and running unleased beside whatever holds it is + // the overlap this exists to prevent. So the discriminator is the + // directory: if it exists, wait; only a lock that could not be created + // at all is a reason to degrade. + if (err?.code !== "ELOCKED" && !existsSync(lockPathOf(dir))) { log( `gate-lease: could not take the lease at ${lockPathOf(dir)} (${err?.message ?? err}); running without it.`, ); @@ -285,6 +385,13 @@ export async function runUnderLease({ }) { let release = null; let waited = 0; + // Filled in by the shim the moment the lock directory is ours. + const owned = { id: null }; + const guarded = guardedFs(lockPathOf(dir), owned, () => + log( + "gate-lease: the lease was taken over by another gate while this one ran, so its lock was left alone rather than removed.", + ), + ); if (isSkipped(env)) { log(`gate-lease: ${SKIP_ENV} is set; running without the lease.`); } else { @@ -299,7 +406,14 @@ export async function runUnderLease({ } if (usable) { const startedWaiting = Date.now(); - release = await acquireLease({ dir, log, pollMs, progressMs, maxWaitMs }); + release = await acquireLease({ + dir, + fs: guarded.fs, + log, + pollMs, + progressMs, + maxWaitMs, + }); waited = Date.now() - startedWaiting; } if (release !== null) { @@ -322,11 +436,34 @@ export async function runUnderLease({ } const startedRunning = Date.now(); - const child = spawn(command, args, { + // Release on every way out — a spawn failure included, or the lock would + // sit held for STALE_MS with no gate running behind it. + const finishLease = async () => { + if (release === null) return; + // The record is only meaningful while the lock beside it is held; a + // waiter reading a fresh lock must not be told about the previous + // holder — unless the lock is no longer ours, in which case the record + // is the winner's too. + if (guarded.mine()) rmSync(leaseTarget(dir), { force: true }); + try { + await release(); + } catch (err) { + log( + `gate-lease: could not release the lease (${err?.message ?? err}); it goes stale on its own after ${formatDuration(STALE_MS)}.`, + ); + } + log( + `gate-lease: released after ${formatDuration(Date.now() - startedRunning)}${waited >= pollMs ? ` (waited ${formatDuration(waited)} first)` : ""}.`, + ); + }; + + const spec = spawnSpec(command, args); + const child = spawn(spec.command, spec.args, { stdio, env, + shell: spec.shell, // Its own group, so one signal reaches every descendant. See the header. - detached: process.platform !== "win32", + detached: spec.detached, }); const signals = ["SIGINT", "SIGTERM", "SIGHUP"]; @@ -350,28 +487,16 @@ export async function runUnderLease({ return [signal, handler]; }); - const outcome = await new Promise((resolve, reject) => { - child.once("error", reject); - child.once("exit", (code, signal) => resolve({ code, signal })); - }).finally(() => { + let outcome; + try { + outcome = await new Promise((resolve, reject) => { + child.once("error", reject); + child.once("exit", (code, signal) => resolve({ code, signal })); + }); + } finally { clearTimeout(escalation); for (const [signal, handler] of handlers) process.off(signal, handler); - }); - - if (release !== null) { - // The record is only meaningful while the lock beside it is held; a - // waiter reading a fresh lock must not be told about the previous holder. - rmSync(leaseTarget(dir), { force: true }); - try { - await release(); - } catch (err) { - log( - `gate-lease: could not release the lease (${err?.message ?? err}); it goes stale on its own after ${formatDuration(STALE_MS)}.`, - ); - } - log( - `gate-lease: released after ${formatDuration(Date.now() - startedRunning)}${waited >= pollMs ? ` (waited ${formatDuration(waited)} first)` : ""}.`, - ); + await finishLease(); } return stoppedBy !== null ? exitCodeFor({ code: null, signal: stoppedBy }) diff --git a/scripts/gate-lease.test.mjs b/scripts/gate-lease.test.mjs index 474c291c..d6605e5e 100644 --- a/scripts/gate-lease.test.mjs +++ b/scripts/gate-lease.test.mjs @@ -13,6 +13,7 @@ import { mkdirSync, mkdtempSync, readFileSync, + rmSync, utimesSync, writeFileSync, } from "node:fs"; @@ -35,6 +36,7 @@ import { main, readHolder, runUnderLease, + spawnSpec, } from "./gate-lease.mjs"; const SCRIPT = fileURLToPath(new URL("./gate-lease.mjs", import.meta.url)); @@ -125,6 +127,24 @@ test("exitCodeFor: the child's code, else 128 + signal", () => { assert.equal(exitCodeFor({ code: null, signal: "SIGNOPE" }), 128); }); +test("spawnSpec: shell-free and grouped on POSIX; cmd.exe-quoted on Windows", () => { + // `npm` is `npm.cmd` on Windows and cannot be started without a shell, and + // a shell re-parses any argument holding a space — the same pair of rules + // `scripts/install-clients.mjs` and `scripts/lib/win-shell-args.mjs` encode. + const args = ["run", "local:gate:stages", "a b"]; + assert.deepEqual(spawnSpec("npm", args, "darwin"), { + command: "npm", + args, + shell: false, + detached: true, + }); + assert.deepEqual(spawnSpec("npm", args, "linux").detached, true); + const win = spawnSpec("npm", args, "win32"); + assert.equal(win.shell, true); + assert.equal(win.detached, false); + assert.deepEqual(win.args, ["run", "local:gate:stages", '"a b"']); +}); + test("readHolder/describeHolder: a missing or malformed record is not an error", () => { const dir = freshDir(); assert.equal(readHolder(dir), null); @@ -214,6 +234,73 @@ test("a stale lock (its holder died) is taken over, not waited on", async () => assert.ok(!existsSync(lockPathOf(dir))); }); +test("a stale lock that cannot be removed is waited on, not bypassed", async () => { + const dir = freshDir(); + // Stale by mtime, but with a file inside — the library's takeover `rmdir` + // fails ENOTEMPTY, which is not ELOCKED. Something still holds the path, + // so degrading to an unleased run here would be the overlap this prevents. + mkdirSync(lockPathOf(dir), { recursive: true }); + writeFileSync(join(lockPathOf(dir), "stray"), ""); + const dead = new Date(Date.now() - STALE_MS - 1_000); + utimesSync(lockPathOf(dir), dead, dead); + const { lines, log } = collectLog(); + await assert.rejects(runNode("", { dir, log, maxWaitMs: 0 }), /gave up/); + assert.equal( + lines.filter((l) => l.includes("running without")).length, + 0, + lines.join("\n"), + ); + assert.ok(existsSync(lockPathOf(dir))); +}); + +test("a command that cannot be spawned still releases the lease", async () => { + const dir = freshDir(); + const { log } = collectLog(); + await assert.rejects( + runUnderLease({ + command: join(dir, "no-such-binary"), + args: [], + dir, + log, + stdio: "ignore", + pollMs: 25, + }), + { code: "ENOENT" }, + ); + assert.ok( + !existsSync(lockPathOf(dir)), + "the lease must not outlive the failed spawn", + ); + assert.ok(!existsSync(leaseTarget(dir))); +}); + +test("a lock replaced mid-run (stale takeover) is the winner's, and is left alone", async () => { + const dir = freshDir(); + const { lines, log } = collectLog(); + const run = runNode("setTimeout(() => {}, 400)", { dir, log }); + await waitFor(() => existsSync(leaseTarget(dir))); + // What a waiter does after our refresh timer was starved past STALE_MS: + // remove our directory and create its own. A new inode, so the guard can + // tell it is not ours — and must not `rmdir` it on our way out, or a third + // gate would run beside the winner. + rmSync(lockPathOf(dir), { recursive: true }); + mkdirSync(lockPathOf(dir)); + writeFileSync( + leaseTarget(dir), + JSON.stringify({ pid: 1, cwd: "/winner", startedAt: Date.now() }), + ); + assert.equal(await run, 0); + assert.ok( + existsSync(lockPathOf(dir)), + "the winner's lock survived our release", + ); + assert.equal(readHolder(dir)?.cwd, "/winner", "and so did its record"); + assert.ok( + lines.some((l) => l.includes("taken over by another gate")), + lines.join("\n"), + ); +}); + test("a live lock that never releases fails the wait loudly, naming the holder", async () => { const dir = freshDir(); // A fresh lock directory nobody refreshes is indistinguishable from a live @@ -310,12 +397,17 @@ test("a signal to the wrapper stops the whole tree, releases the lease and exits wrapper.once("exit", (code, signal) => resolve({ code, signal })), ); - await waitFor(() => existsSync(pidFile)); - const grandchild = Number(readFileSync(pidFile, "utf8")); - assert.ok(isAlive(grandchild)); - assert.ok(existsSync(lockPathOf(dir)), "the wrapper held the lease"); - - wrapper.kill("SIGTERM"); + let grandchild = null; + try { + await waitFor(() => existsSync(pidFile)); + grandchild = Number(readFileSync(pidFile, "utf8")); + assert.ok(isAlive(grandchild)); + assert.ok(existsSync(lockPathOf(dir)), "the wrapper held the lease"); + } finally { + // On every path — a failed setup assertion included — the wrapper is + // told to stop, so an idle grandchild can never outlive the suite. + wrapper.kill("SIGTERM"); + } const outcome = await exited; assert.deepEqual(outcome, { code: 143, signal: null }, stderr); await waitFor(() => !isAlive(grandchild)); From 2c8dbd3cd0444897f1ac1b70474b5e366eac7b34 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 12 Sep 2026 22:30:05 -0400 Subject: [PATCH 3/5] fix: address Copilot review round 2 (#2339) - The holder-record removal is best-effort and can no longer throw past release(); ERELEASED on release is the library having already dropped a taken-over lock, so it is ignored rather than described as a lock that "goes stale on its own". - The signal test's stand-in now spawns a descendant of its own, so the whole-process-group guarantee is what the test asserts: a regression to child.kill() or a non-detached spawn leaves the descendant alive and fails. - The three docs no longer claim a dead holder is always recovered in 30s: a stale lock directory that cannot be removed runs the wait to its 45-minute cap and names the path. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall --- .claude/skills/pre-push-gate/SKILL.md | 8 ++++++-- AGENTS.md | 2 +- docs/quality-gate.md | 2 +- scripts/gate-lease.mjs | 13 +++++++++++-- scripts/gate-lease.test.mjs | 22 ++++++++++++++++------ 5 files changed, 35 insertions(+), 12 deletions(-) diff --git a/.claude/skills/pre-push-gate/SKILL.md b/.claude/skills/pre-push-gate/SKILL.md index 0edc56ce..c802c24b 100644 --- a/.claude/skills/pre-push-gate/SKILL.md +++ b/.claude/skills/pre-push-gate/SKILL.md @@ -173,8 +173,12 @@ releases (it re-checks every 2s and prints `still waiting` once a minute). The holder's pid and worktree are in the line, so you can decide whether to wait or to stop that gate. A holder that was **killed** — a closed terminal, an OOM'd session — stops refreshing its lock and is taken over after 30s; nothing -needs cleaning up by hand. The wait gives up after 45 minutes, naming the -holder, which only ever happens against a live gate that has hung. +needs cleaning up by hand. The one exception is a dead holder's lock directory +that cannot be removed (a stray file inside it, or permissions): the takeover +fails, the waiter keeps waiting, and the wait runs to its 45-minute cap naming +the path — remove that directory by hand. So the give-up happens against a +live gate that has hung, or a stale lock that would not go away; never on its +own. `INSPECTOR_SKIP_GATE_LEASE=1 npm run local:gate` runs without the lease. It is for a measurement that needs contention; it does not get a result sooner, diff --git a/AGENTS.md b/AGENTS.md index 3b34e928..082d2813 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -476,7 +476,7 @@ free to run a full `local:gate`). Raising a budget nobody chose hides no race. - **`npm run local:gate` is the mandatory pre-push command.** It runs **every check** `.github/workflows/main.yml` runs, plus two local-only steps, so a green run here is the strongest predictor of a green CI this repo has: every check CI applies has already passed on your machine. It is not a proof — CI runs on a different OS and, since #2341, runs each client's suite bare where the gate runs it only instrumented — so the one residual is a test that passes *only* when slowed down, which is a race (#1596) to fix, never headroom to keep. Expect several minutes. - **The gate runs each client's test suite once, instrumented; CI runs it twice (#2341).** CI's `build` job runs the bare `test` inside `validate` and its parallel `coverage` job runs `test:coverage`, which costs CI no wall clock. Serially in one process the bare pass was ~80s of a ~370s gate, re-running exactly the files the coverage pass runs a few minutes later — so the gate calls **`local:validate`**, which is `validate` minus each client's `test` leg (every client's `validate` is `check && test`, and `local:validate` runs the `check` half). "Every check" is therefore a claim about checks, not invocations, and it holds because `@vitest/coverage-v8` collects coverage from V8's own profiler (`Profiler.takePreciseCoverage`) and rewrites no source: a test file sees identical code either way, and the instrumented run is only *slower*, which makes it the stricter of the two for the failure class this repo actually sees (a correct test cut off under load). A test that passed only *because* it ran slower would be a race — #1596's class, a defect wherever it surfaces — and CI's bare pass still runs it. **`npm run validate` is unchanged**, because CI runs it directly and it is the inner-loop check; `verify:*` guards that ask "is this reachable from `validate`" are unaffected. `local:validate` lives in the `local:` namespace so the workflow guard keeps it out of CI by construction. - **`npm run validate` is the fast inner-loop check and is NOT an acceptable substitute.** It runs `test`, not `test:coverage`, so it does **zero** coverage gating, no smokes, and no Storybook tests. Skipping the gate is how a push passes every fast local check and still fails CI. -- **Concurrent gates queue; they do not overlap.** `local:gate` takes a machine-wide lease (`scripts/gate-lease.mjs`, #2339) so a gate started in a second worktree waits for the first rather than running alongside it. Overlap is not merely slow — the web smokes bind fixed ports, so two gates reaching the same smoke together go red on a diff that cannot have caused it (measured: one of two concurrent gates failed at 279s on port 6298 while a quiet gate passed in 257s). The wait names the holder and its worktree; a holder that dies releases within 30s; `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it, which is for a measurement that _needs_ contention, never for getting a result sooner — the queued run finishes sooner anyway. +- **Concurrent gates queue; they do not overlap.** `local:gate` takes a machine-wide lease (`scripts/gate-lease.mjs`, #2339) so a gate started in a second worktree waits for the first rather than running alongside it. Overlap is not merely slow — the web smokes bind fixed ports, so two gates reaching the same smoke together go red on a diff that cannot have caused it (measured: one of two concurrent gates failed at 279s on port 6298 while a quiet gate passed in 257s). The wait names the holder and its worktree; a holder that dies releases within 30s (unless its lock directory cannot be removed, in which case the wait runs to its 45-minute cap and names the path); `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it, which is for a measurement that _needs_ contention, never for getting a result sooner — the queued run finishes sooner anyway. - There is deliberately **no `npm run ci`** — that name collided with the `npm ci` built-in, which clean-installs from the lockfile and does not run this script. - What each stage covers, and why two of them are local-only, is [`docs/quality-gate.md`](./docs/quality-gate.md); how to diagnose a failing stage is the `pre-push-gate` skill. diff --git a/docs/quality-gate.md b/docs/quality-gate.md index e6067d68..a97fe0cd 100644 --- a/docs/quality-gate.md +++ b/docs/quality-gate.md @@ -43,7 +43,7 @@ That is the readable half, and prose rots. The enforced half is `scripts/lib/wor | `npm run verify:typecheck-coverage` | The typecheck-coverage analog of the above (#1791): for each Node client (auto-discovered from disk — enrolled via its `typecheck` script's projects, or for a `tsc -b` client like `clients/web` via its `tsconfig.json` `references`) it runs those projects with `tsc --listFilesOnly`, unions them, and **fails** listing any tracked `.ts`/`.tsx`/`.mts`/`.cts` under the client that lands in no project (so a new top-level config/helper can't silently go untypechecked). It also requires, deny-by-default, the first-party TS no client owns (`test-servers/src`, the root `vitest.shared.mts`, all of `core/`, and any new top-level location) to land in some client project's tsc pass — so a `core` `*.tsx` web's projects don't reach is caught too. Also asserts the gate is wired (each client's typecheck pass — its `typecheck` script, or web's `tsc -b` — is reachable from its `validate`, and the root chain runs each client's `validate`). Runs in `validate`. | | `npm run verify:dep-lockstep` | Guards the "one version per install-crossing dependency" invariant (#1896). v2 is not a workspace, so a client's test project compiles the shared first-party TypeScript — `core/`, `test-servers/src`, and the root-owned `vitest.shared.mts`, all of which resolve their dependencies from the **root** install — alongside the client's own sources, putting the same package in one `tsc` program twice. At the same version that's harmless; skewed, TypeScript must relate two structurally-distinct copies of every type, which for a recursive-generic surface is exponential (zod `4.3.6` vs `4.4.3` exhausted the 4GB tsc heap in `clients/web`). Derives its candidate set from **what actually enters each program** (#1965) — every client tsconfig project listed with `tsc --listFilesOnly` via the shared `scripts/lib/tsc-program.mjs`, each resolved `node_modules` file mapped to its owning install, keeping the packages that reach one program from two installs (a package whose declarations arrive only through another package's `.d.ts`, as `@modelcontextprotocol/sdk`'s do, is invisible to a scan of first-party imports). Prices each copy from the lockfile entry for the exact install path the program resolved, compares only the installs that met in one program, and **fails deny-by-default** on any disagreement not in the annotated `TOLERATED_SKEW` allowlist — empty today — with an allowlisted package tolerated only *within a major version*. A **second tier** (#2226) runs alongside it, asking the weaker but broader question the `AGENTS.md` rule actually states: does a package this repo *declares* anywhere resolve to two versions across our installs at all? Its candidate set is every name in any install's `dependencies`/`devDependencies`/`optionalDependencies` — unioned across the root and all four clients, so a copy declared by only one of them still counts — that **more than one install holds a top-level copy of** (17 packages today). Nested copies are excluded: one exists because some dependency asked for a different version, so it is that dependency's range to govern, not ours. Neither tier subsumes the other — the program tier sees a copy no manifest names (`@modelcontextprotocol/sdk`, arriving through another package's `.d.ts`), while the declared tier sees a **transitive** copy no program loads (cli's `@types/node`, hoisted via `@types/express` — the case that motivated it), two **clients** disagreeing with no root copy involved (`@types/react`, web against tui), and the peer shadows `eslint`/`typescript`/`vitest` that never enter a program. Same deny-by-default and same within-a-major rule, against its own `TOLERATED_DECLARED_SKEW` — also empty. Two limits: it reads **lockfiles**, so an uncommitted hand-installed copy is invisible, and it compares only declared names, so a purely transitive package no manifest names stays the first tier's business. Runs in `validate`. | `npm run verify:test-timeouts` | Guards the wall-clock budgets the test gates run under (#2323). The class it encodes against is a budget **nobody chose**: three of the six Vitest projects ran on Vitest's own `testTimeout: 5000` and five on its `hookTimeout`/`teardownTimeout: 10000`, sized for an idle machine rather than the one this team works on — three or four concurrent agent sessions in separate worktrees, each free to run a full `local:gate`, on eight logical cores. A correct, deterministic test cut off by such a budget fails a gate its diff did not break, which is the same "channel nobody trusts" failure [Lint has no warning tier](../AGENTS.md#lint-has-no-warning-tier) describes from the other direction; #2292, #1942 and #1742 were each an instance, found one site at a time. Deliberately **not** cited: #2278 (a missing condition wait around a geometry read) and #2250 (a real race in a test's own timing) were fixed by making the test wait for the right thing, and presenting a race fix as evidence for a larger ceiling would argue against #1596. It asks **Vitest itself** to resolve each of the four configs and reads the number a test actually gets, rather than checking that a key is absent from a config block — which would pass just as happily on a config that had stopped being loaded. A **seventh project** with no row, or a config file it does not discover (it knows all twelve filenames Vitest accepts, not just the two this repo uses), is an error rather than a silent skip. It also checks that every project loads `vitest.setup.shared.mts`. ⚠️ **Everything it reads comes from a resolved project, never from source text** — the two rules no config can report are asserted at runtime instead: `retry` by `vitest.setup.shared.mts` (which reads the value Vitest resolved, so a per-test option, a `describe` option, a project setting and a `--retry` flag are one check) and Testing Library's `asyncUtilTimeout` by `clients/web/src/test/asyncUtilTimeout.test.ts` (which reads what the project's own `waitFor`s use). Both began as source scanning in #2334 and the review found a new valid spelling missed in five consecutive rounds, so the boundary is deliberate: if a rule cannot be answered by asking the tool, assert it at runtime rather than reading the source for it. Observed **red** against the pre-#2323 config before it was trusted. Runs in `validate`; its decision logic has a sibling `verify-test-timeouts.test.mjs` under `test:scripts`. -| `npm run local:gate` | **Mandatory pre-push command.** `local:validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. Every GitHub CI check plus two local-only ones — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. **Runs under a machine-wide lease** (`scripts/gate-lease.mjs`, [#2339](https://github.com/modelcontextprotocol/inspector/issues/2339)): a second `local:gate` started in another worktree waits for the first to finish instead of running alongside it. Measured on `aa56551b`, a quiet gate took 257s; two started together had one **fail** at 279s on a smoke port collision (`smoke:web:chromium` binds 6298, and so did the other gate's) and the survivor take 338s — so overlapping gates are red by construction, not merely slow, and back to back both finish green in ~2x257s. The wait prints the holder's pid and worktree; a holder that dies without releasing is taken over after 30s (`proper-lockfile` stale detection); `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it. The stages themselves are `local:gate:stages`, which the wrapper runs verbatim. | +| `npm run local:gate` | **Mandatory pre-push command.** `local:validate` → `verify:skills:cli` → `coverage` → `verify:build-gate` → `verify:bundle-externals` → `smoke` → `smoke:web:firefox` → `local:storybook`. Every GitHub CI check plus two local-only ones — see [Two tiers](#two-tiers-github-ci-and-the-local-gate). Named `local:` rather than `ci` on purpose (#2146); there is no `npm run ci` alias. **Runs under a machine-wide lease** (`scripts/gate-lease.mjs`, [#2339](https://github.com/modelcontextprotocol/inspector/issues/2339)): a second `local:gate` started in another worktree waits for the first to finish instead of running alongside it. Measured on `aa56551b`, a quiet gate took 257s; two started together had one **fail** at 279s on a smoke port collision (`smoke:web:chromium` binds 6298, and so did the other gate's) and the survivor take 338s — so overlapping gates are red by construction, not merely slow, and back to back both finish green in ~2x257s. The wait prints the holder's pid and worktree; a holder that dies without releasing is taken over after 30s (`proper-lockfile` stale detection) — unless its lock directory cannot be removed, in which case the wait runs to its 45-minute cap and names the path; `INSPECTOR_SKIP_GATE_LEASE=1` bypasses it. The stages themselves are `local:gate:stages`, which the wrapper runs verbatim. | | `npm run local:validate` | The gate's first stage: `validate` with each client's `test` leg removed (#2341) — the same `validate:guards` and `validate:core`, then every client's `check` — `format:check` + `lint` + `typecheck`, plus `build` for web, tui and launcher, which name one explicitly (cli's only validate-time build was `test`'s `pretest` hook, so it goes with the `test` leg and `coverage:cli` builds the binary once, later) — each client's `validate` being `check && test`. The suites run once, under `coverage`, instead of bare here and instrumented there. In the `local:` namespace so the workflow guard keeps it out of CI, where the bare pass is free. | | `npm run pack:verify` | Publish smoke — see [Publishing](./publishing.md). | diff --git a/scripts/gate-lease.mjs b/scripts/gate-lease.mjs index 393201a4..ca11a3ca 100644 --- a/scripts/gate-lease.mjs +++ b/scripts/gate-lease.mjs @@ -444,12 +444,21 @@ export async function runUnderLease({ // waiter reading a fresh lock must not be told about the previous // holder — unless the lock is no longer ours, in which case the record // is the winner's too. - if (guarded.mine()) rmSync(leaseTarget(dir), { force: true }); + try { + if (guarded.mine()) rmSync(leaseTarget(dir), { force: true }); + } catch { + // Diagnostic only; never let it stand between a finished gate and the + // release below. + } try { await release(); } catch (err) { + // `ERELEASED` means the library's own refresh tick already found the + // lock taken over and dropped it; the directory there now is the + // winner's live lock, and `onCompromised` has already said so. + if (err?.code === "ERELEASED") return; log( - `gate-lease: could not release the lease (${err?.message ?? err}); it goes stale on its own after ${formatDuration(STALE_MS)}.`, + `gate-lease: could not release the lease (${err?.message ?? err}). A waiter takes it over once it is ${formatDuration(STALE_MS)} stale — unless whatever blocked this removal persists, in which case remove ${lockPathOf(dir)} by hand.`, ); } log( diff --git a/scripts/gate-lease.test.mjs b/scripts/gate-lease.test.mjs index d6605e5e..f2b9933a 100644 --- a/scripts/gate-lease.test.mjs +++ b/scripts/gate-lease.test.mjs @@ -379,9 +379,14 @@ test("main: no command is a usage error", async () => { test("a signal to the wrapper stops the whole tree, releases the lease and exits 128+signal", async () => { const dir = freshDir(); const pidFile = join(dir, "grandchild.pid"); - // The gate stand-in: records its pid, then idles forever unless signalled. + // The gate stand-in: starts a descendant of its own (as `npm run` does, + // four levels deep), records both pids, then idles forever unless + // signalled. The descendant is what proves the whole *tree* is stopped — + // a regression to `child.kill()` or a non-detached spawn would leave it + // running and fail below. const idle = - `require("fs").writeFileSync(${JSON.stringify(pidFile)}, String(process.pid));` + + `const d = require("child_process").spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" });` + + `require("fs").writeFileSync(${JSON.stringify(pidFile)}, process.pid + " " + d.pid);` + `setInterval(() => {}, 1000)`; const wrapper = spawn( process.execPath, @@ -397,11 +402,12 @@ test("a signal to the wrapper stops the whole tree, releases the lease and exits wrapper.once("exit", (code, signal) => resolve({ code, signal })), ); - let grandchild = null; + let tree = []; try { await waitFor(() => existsSync(pidFile)); - grandchild = Number(readFileSync(pidFile, "utf8")); - assert.ok(isAlive(grandchild)); + tree = readFileSync(pidFile, "utf8").split(" ").map(Number); + assert.equal(tree.length, 2); + for (const pid of tree) assert.ok(isAlive(pid), `pid ${pid} started`); assert.ok(existsSync(lockPathOf(dir)), "the wrapper held the lease"); } finally { // On every path — a failed setup assertion included — the wrapper is @@ -410,7 +416,11 @@ test("a signal to the wrapper stops the whole tree, releases the lease and exits } const outcome = await exited; assert.deepEqual(outcome, { code: 143, signal: null }, stderr); - await waitFor(() => !isAlive(grandchild)); + for (const pid of tree) + await waitFor(() => !isAlive(pid)).catch(() => { + process.kill(pid, "SIGKILL"); + assert.fail(`pid ${pid} outlived the wrapper — the tree was not stopped`); + }); assert.ok( !existsSync(lockPathOf(dir)), "the lease was released on the way out", From d3e5432532617e268edb8bfed5db52217f873f66 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 12 Sep 2026 22:42:57 -0400 Subject: [PATCH 4/5] fix: address Copilot review round 3 (#2339) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - The spawn itself — spawnSpec and the synchronous argument validation in child_process.spawn — now sits inside the try whose finally releases the lease, so a command that fails before it starts cannot strand the lock until stale takeover. Tested with a non-array args value. - A genuine release failure no longer falls through to the "released after" line; the lock may still be blocking the queue, and the message that says so is the last word. Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall --- scripts/gate-lease.mjs | 59 ++++++++++++++++++++----------------- scripts/gate-lease.test.mjs | 21 +++++++++++++ 2 files changed, 53 insertions(+), 27 deletions(-) diff --git a/scripts/gate-lease.mjs b/scripts/gate-lease.mjs index ca11a3ca..d4eea50c 100644 --- a/scripts/gate-lease.mjs +++ b/scripts/gate-lease.mjs @@ -460,44 +460,49 @@ export async function runUnderLease({ log( `gate-lease: could not release the lease (${err?.message ?? err}). A waiter takes it over once it is ${formatDuration(STALE_MS)} stale — unless whatever blocked this removal persists, in which case remove ${lockPathOf(dir)} by hand.`, ); + // Not "released after": the lock may still be blocking the queue. + return; } log( `gate-lease: released after ${formatDuration(Date.now() - startedRunning)}${waited >= pollMs ? ` (waited ${formatDuration(waited)} first)` : ""}.`, ); }; - const spec = spawnSpec(command, args); - const child = spawn(spec.command, spec.args, { - stdio, - env, - shell: spec.shell, - // Its own group, so one signal reaches every descendant. See the header. - detached: spec.detached, - }); - const signals = ["SIGINT", "SIGTERM", "SIGHUP"]; let stoppedBy = null; let escalation = null; - const onSignal = (signal) => { - if (stoppedBy !== null) return; - stoppedBy = signal; - log(`gate-lease: received ${signal}; stopping the gate.`); - signalTree(child, signal); - escalation = setTimeout(() => { - log( - `gate-lease: the gate did not exit within ${graceMs}ms of ${signal}; sending SIGKILL.`, - ); - signalTree(child, "SIGKILL"); - }, graceMs); - }; - const handlers = signals.map((signal) => { - const handler = () => onSignal(signal); - process.on(signal, handler); - return [signal, handler]; - }); - + let handlers = []; let outcome; + // Everything from here to the child's exit is inside the `try`, so a + // synchronous failure to even start it — an argument `winShellArgs` + // refuses, a spawn option Node rejects — releases the lease the same way + // an asynchronous spawn error does. try { + const spec = spawnSpec(command, args); + const child = spawn(spec.command, spec.args, { + stdio, + env, + shell: spec.shell, + // Its own group, so one signal reaches every descendant. See the header. + detached: spec.detached, + }); + const onSignal = (signal) => { + if (stoppedBy !== null) return; + stoppedBy = signal; + log(`gate-lease: received ${signal}; stopping the gate.`); + signalTree(child, signal); + escalation = setTimeout(() => { + log( + `gate-lease: the gate did not exit within ${graceMs}ms of ${signal}; sending SIGKILL.`, + ); + signalTree(child, "SIGKILL"); + }, graceMs); + }; + handlers = signals.map((signal) => { + const handler = () => onSignal(signal); + process.on(signal, handler); + return [signal, handler]; + }); outcome = await new Promise((resolve, reject) => { child.once("error", reject); child.once("exit", (code, signal) => resolve({ code, signal })); diff --git a/scripts/gate-lease.test.mjs b/scripts/gate-lease.test.mjs index f2b9933a..494c37e3 100644 --- a/scripts/gate-lease.test.mjs +++ b/scripts/gate-lease.test.mjs @@ -274,6 +274,27 @@ test("a command that cannot be spawned still releases the lease", async () => { assert.ok(!existsSync(leaseTarget(dir))); }); +test("a command that fails synchronously before it starts still releases the lease", async () => { + const dir = freshDir(); + const { log } = collectLog(); + // `spawn` validates its arguments synchronously: a non-array `args` throws + // before any child exists, the same class as `winShellArgs` refusing a `%` + // on Windows. That throw must not strand the lock until stale takeover. + await assert.rejects( + runUnderLease({ + command: process.execPath, + args: "-e", + dir, + log, + stdio: "ignore", + pollMs: 25, + }), + { code: "ERR_INVALID_ARG_TYPE" }, + ); + assert.ok(!existsSync(lockPathOf(dir))); + assert.ok(!existsSync(leaseTarget(dir))); +}); + test("a lock replaced mid-run (stale takeover) is the winner's, and is left alone", async () => { const dir = freshDir(); const { lines, log } = collectLog(); From 17e85a8745bc113577c458dd11cf711c1bab06a5 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 12 Sep 2026 22:57:30 -0400 Subject: [PATCH 5/5] fix: address Copilot review round 4 (#2339) Handle SIGQUIT (Ctrl-\) on the same tree-shutdown path as SIGINT, SIGTERM and SIGHUP. The gate runs in its own process group, so a signal the wrapper does not handle ends the wrapper alone and orphans the gate behind a lease that then goes stale under it. The signal test is parametrized over SIGTERM (143) and SIGQUIT (131). Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01PsmUAZ6aLUAFqenjJKSSRp Signed-off-by: cliffhall --- scripts/gate-lease.mjs | 6 +- scripts/gate-lease.test.mjs | 111 ++++++++++++++++++++---------------- 2 files changed, 66 insertions(+), 51 deletions(-) diff --git a/scripts/gate-lease.mjs b/scripts/gate-lease.mjs index d4eea50c..19612857 100644 --- a/scripts/gate-lease.mjs +++ b/scripts/gate-lease.mjs @@ -468,7 +468,11 @@ export async function runUnderLease({ ); }; - const signals = ["SIGINT", "SIGTERM", "SIGHUP"]; + // Every signal a terminal or a supervisor sends to end a run: Ctrl-C, a + // plain kill, a closed terminal, and Ctrl-\ — the child is in its own + // group, so any of these left unhandled would end this process and + // orphan the gate behind a lease that then goes stale under it. + const signals = ["SIGINT", "SIGTERM", "SIGHUP", "SIGQUIT"]; let stoppedBy = null; let escalation = null; let handlers = []; diff --git a/scripts/gate-lease.test.mjs b/scripts/gate-lease.test.mjs index 494c37e3..fe012d3c 100644 --- a/scripts/gate-lease.test.mjs +++ b/scripts/gate-lease.test.mjs @@ -397,54 +397,65 @@ test("main: no command is a usage error", async () => { assert.match(lines.join("\n"), /usage:/); }); -test("a signal to the wrapper stops the whole tree, releases the lease and exits 128+signal", async () => { - const dir = freshDir(); - const pidFile = join(dir, "grandchild.pid"); - // The gate stand-in: starts a descendant of its own (as `npm run` does, - // four levels deep), records both pids, then idles forever unless - // signalled. The descendant is what proves the whole *tree* is stopped — - // a regression to `child.kill()` or a non-detached spawn would leave it - // running and fail below. - const idle = - `const d = require("child_process").spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" });` + - `require("fs").writeFileSync(${JSON.stringify(pidFile)}, process.pid + " " + d.pid);` + - `setInterval(() => {}, 1000)`; - const wrapper = spawn( - process.execPath, - [SCRIPT, process.execPath, "-e", idle], - { - env: { ...process.env, [DIR_ENV]: dir }, - stdio: ["ignore", "ignore", "pipe"], - }, - ); - let stderr = ""; - wrapper.stderr.on("data", (chunk) => (stderr += chunk)); - const exited = new Promise((resolve) => - wrapper.once("exit", (code, signal) => resolve({ code, signal })), - ); +// Ctrl-C and a plain kill are the obvious ones; Ctrl-\ (SIGQUIT) is the one +// that was missed (Copilot), and it matters for the same reason: the child +// is in its own process group, so a signal the wrapper does not handle ends +// the wrapper alone and orphans the gate. +for (const [signal, code] of [ + ["SIGTERM", 143], + ["SIGQUIT", 131], +]) { + test(`${signal} to the wrapper stops the whole tree, releases the lease and exits ${code}`, async () => { + const dir = freshDir(); + const pidFile = join(dir, "grandchild.pid"); + // The gate stand-in: starts a descendant of its own (as `npm run` does, + // four levels deep), records both pids, then idles forever unless + // signalled. The descendant is what proves the whole *tree* is stopped — + // a regression to `child.kill()` or a non-detached spawn would leave it + // running and fail below. + const idle = + `const d = require("child_process").spawn(process.execPath, ["-e", "setInterval(() => {}, 1000)"], { stdio: "ignore" });` + + `require("fs").writeFileSync(${JSON.stringify(pidFile)}, process.pid + " " + d.pid);` + + `setInterval(() => {}, 1000)`; + const wrapper = spawn( + process.execPath, + [SCRIPT, process.execPath, "-e", idle], + { + env: { ...process.env, [DIR_ENV]: dir }, + stdio: ["ignore", "ignore", "pipe"], + }, + ); + let stderr = ""; + wrapper.stderr.on("data", (chunk) => (stderr += chunk)); + const exited = new Promise((resolve) => + wrapper.once("exit", (code, signal) => resolve({ code, signal })), + ); - let tree = []; - try { - await waitFor(() => existsSync(pidFile)); - tree = readFileSync(pidFile, "utf8").split(" ").map(Number); - assert.equal(tree.length, 2); - for (const pid of tree) assert.ok(isAlive(pid), `pid ${pid} started`); - assert.ok(existsSync(lockPathOf(dir)), "the wrapper held the lease"); - } finally { - // On every path — a failed setup assertion included — the wrapper is - // told to stop, so an idle grandchild can never outlive the suite. - wrapper.kill("SIGTERM"); - } - const outcome = await exited; - assert.deepEqual(outcome, { code: 143, signal: null }, stderr); - for (const pid of tree) - await waitFor(() => !isAlive(pid)).catch(() => { - process.kill(pid, "SIGKILL"); - assert.fail(`pid ${pid} outlived the wrapper — the tree was not stopped`); - }); - assert.ok( - !existsSync(lockPathOf(dir)), - "the lease was released on the way out", - ); - assert.match(stderr, /received SIGTERM; stopping the gate/); -}); + let tree = []; + try { + await waitFor(() => existsSync(pidFile)); + tree = readFileSync(pidFile, "utf8").split(" ").map(Number); + assert.equal(tree.length, 2); + for (const pid of tree) assert.ok(isAlive(pid), `pid ${pid} started`); + assert.ok(existsSync(lockPathOf(dir)), "the wrapper held the lease"); + } finally { + // On every path — a failed setup assertion included — the wrapper is + // told to stop, so an idle grandchild can never outlive the suite. + wrapper.kill(signal); + } + const outcome = await exited; + assert.deepEqual(outcome, { code, signal: null }, stderr); + for (const pid of tree) + await waitFor(() => !isAlive(pid)).catch(() => { + process.kill(pid, "SIGKILL"); + assert.fail( + `pid ${pid} outlived the wrapper — the tree was not stopped`, + ); + }); + assert.ok( + !existsSync(lockPathOf(dir)), + "the lease was released on the way out", + ); + assert.match(stderr, new RegExp(`received ${signal}; stopping the gate`)); + }); +}