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.