diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 9e8e8033..e6a22c98 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -80,6 +80,7 @@ Both exist and do different jobs. Theme files (`src/theme/.ts`) custo - Dependencies reached only through **root-owned code with no manifest** (`test-servers/src`, `core/`) are declared at the root and aliased to the **repo root** in `vitest.shared.mts` — as `express` and `yaml` are — not to `/node_modules` like the other pins there. - **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed it in the *consumer's* tree, beside a React satisfying *its* peer range — looser than ours, which is all it takes to split React. `ink-form`/`ink-scroll-view` declare `">=18"`, so a consumer's React 18 satisfies them, the TUI ends up with two React instances, and it crashes on the first hook (#1952). Both are inlined via `noExternal` in `clients/tui/tsup.config.ts`. **`ink` is the one exemption, justified by cost (~1.4MB) — never by a peer range**: flag any claim that `">=19"` keeps npm from misplacing it, which is false and was in this repo once. What keeps it safe is the **root `react` range staying open to the whole major (`^19.0.0`)** so npm can dedupe with a consumer's pinned React 19; treat narrowing that range as reopening the bug. `clients/tui/__tests__/tsupConfig.test.ts` enforces the split, the root-declaration of exempt packages, and that range. - **Which section is a separate question from which manifest.** A package `core/` imports at runtime must be in root **`dependencies`**: client builds externalize npm packages, so a published install resolves them from the root manifest and devDependencies are absent there. Only test/build-only packages (`express`) belong in `devDependencies`. Flag a runtime `core/` import added to `devDependencies` — it passes every local check and breaks the published package. +- **A root-declared package that `core/` imports at runtime must also be named in each client's bundler `external` list** — `clients/{cli,tui}/tsup.config.ts` and `clients/web/tsup.runner.config.ts`, all three. Bundlers externalize what the *client's* manifest declares, and these packages are root-only by rule, so omitting them means they get bundled. For a CJS package inlined into an ESM bundle that is fatal: esbuild's `Dynamic require of "path" is not supported` shim throws at import time and the binary dies before parsing a flag (#2082, `proper-lockfile`). Flag a new root runtime dependency that is not added to all three. ## Tests and the coverage gate diff --git a/AGENTS.md b/AGENTS.md index d68142c1..67cd1596 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,7 +68,28 @@ v2/main/ │ │ │ # throws and the routes turn into a 503, and the keychain │ │ │ # probe), file-secret-store.ts (0600 JSON, AES-256-GCM when │ │ │ # MCP_INSPECTOR_SECRET_KEY is set — refuses to overwrite a -│ │ │ # file it cannot decrypt rather than destroying it), and +│ │ │ # file it cannot decrypt rather than destroying it), +│ │ │ # file-lock.ts (withSecretFileLock: the cross-process +│ │ │ # mutual exclusion #2082 settled on — proper-lockfile, +│ │ │ # borrowed rather than hand-rolled. Read its header before +│ │ │ # citing it: it makes two LIVE Inspectors exclusive, and +│ │ │ # does NOT make stale takeover single-winner. proper-lockfile +│ │ │ # detects a takeover only on its 5s refresh tick, which an +│ │ │ # ordinary sub-second mutation never reaches, and its release +│ │ │ # is an unconditional rmdir (as is its signal-exit handler) — +│ │ │ # so withSecretFileLock passes a GUARDED options.fs, the one +│ │ │ # seam both removal paths share, refusing to delete a lock +│ │ │ # that is no longer ours (inode+birthtime). That NARROWS the +│ │ │ # window, it does not close it — still check-then-act across +│ │ │ # processes. BEST-EFFORT throughout: do not write that a +│ │ │ # compromised holder is always told, or that the winner's +│ │ │ # lock is always preserved. +│ │ │ # DEGRADES when no lock CAN be taken (read-only $HOME etc), +│ │ │ # since this store exists for boxes missing the usual +│ │ │ # mechanism; but THROWS on ELOCKED — a lock held by a live +│ │ │ # writer is evidence the lock works, not licence to bypass +│ │ │ # it — after waiting past the stale window), +│ │ │ # and │ │ │ # secret-store-selection.ts (the POLICY: explicit │ │ │ # MCP_INSPECTOR_SECRET_STORE wins, else probe the keychain, │ │ │ # else fall back LOUDLY — to memory in a container with @@ -185,7 +206,13 @@ v2/main/ │ # pack-and-verify.mjs, and lib/ shared helpers │ # (tsc-program.mjs is the `tsc --listFilesOnly` │ # measurement both coverage guards read a program -│ # through — #1965; resolve-node-bin.mjs resolves a +│ # through — #1965; announced-child.mjs owns the +│ # spawn-and-wait-for-a-readiness-line step, publishing +│ # the child via `onSpawn` BEFORE the wait so the +│ # caller's teardown reaches it on every throw path — +│ # the readiness timeout included, which is what used +│ # to orphan a live server holding its port — #2000; +│ # resolve-node-bin.mjs resolves a │ # package's bin through its own package.json, so the │ # verify/smoke scripts spawn it with `process.execPath` │ # rather than the `npx` `.cmd` shim Node refuses to @@ -222,6 +249,8 @@ The same **placement** rule covers anything reached only through **root-owned co - A package only the tests, the test servers, or the build tooling need belongs in **`devDependencies`** — `express`, added there by #1970. - `yaml` is in `dependencies` today even though its only importer is `test-servers/src/load-config.ts`. Left as-is deliberately (moving it changes what ships, which is not a docs change); if you touch it, confirm no published path reads YAML first. +**A root-declared package that `core/` imports at runtime must also be named in each client's bundler `external` list.** tsup and Vite externalize what the *client's* `package.json` declares, and a root-only dependency is in none of them — so it gets **bundled**, silently, and the placement rule above is what guarantees every such package is root-only. For a CJS package inlined into an ESM bundle that is fatal rather than merely wasteful: esbuild leaves a `Dynamic require of "path" is not supported` shim that throws at *import* time, so the binary dies before it parses a flag. `proper-lockfile` hit exactly that in #2082; `@napi-rs/keyring` is listed in all three for the same reason. The three lists are `clients/cli/tsup.config.ts`, `clients/tui/tsup.config.ts`, and `clients/web/tsup.runner.config.ts` — add a new package to **all** of them, since which client reaches it is a function of what `core/` imports, not of what the client's own code names. + **A dependency that renders React components must be bundled into the client that uses it, and is then not a root dependency.** An externalized package resolves its own `react` from wherever npm placed **it** in the consumer's tree, and npm places a package beside a React satisfying *that package's* peer range — looser than ours in every case here, which is all it takes to split React. `ink-form` and `ink-scroll-view` declare `">=18"`, satisfied by a consumer's React 18 while our React 19 nests underneath: the bundle renders through one React, those packages call hooks on another, and the TUI crashes on the first hook (#1952). Both are inlined by `clients/tui/tsup.config.ts` (`noExternal`) and declared only in `clients/tui/package.json`, where the build resolves them — declaring an inlined package at the root would just make consumers install a second, unused copy. **`ink` is the single exemption, and it is justified by cost, not by safety.** Bundling it works but adds ~1.4MB (`react-reconciler` + `yoga-layout`, plus a `createRequire` banner, since inlined CJS calls `require` at runtime and esbuild's ESM interop rejects that without a real `require` in scope). **Never justify an exemption by a peer range** — `ink` briefly carried "its `">=19"` peer keeps npm honest", which is false: a consumer pinning React 19.0 satisfies `">=19"` while a narrower range of ours nests underneath. What actually makes the exemption safe is a *different* lever: the **root `react` range stays open to the whole major (`^19.0.0`)**, so npm can dedupe our React with whatever React 19 a consumer pins and an external `ink` lands on the same copy the bundle uses. Narrowing it (e.g. back to `^19.2.4`) silently reopens the crash for the renderer itself, which breaks TUI *startup*, not just its forms. `clients/tui/__tests__/tsupConfig.test.ts` enforces all of it: React-rendering deps inlined, each exempt package both external and root-declared, and the root range pinned to `ink`'s peer floor. @@ -773,7 +802,7 @@ The ⚠️ option-deletion hazard, the snapshot rule, and the recovery recipe ab **Every web smoke runs against a throwaway catalog (#1977).** The helper mints a temp dir per run and passes it as `MCP_CATALOG_PATH`; without it the web backend falls back to the developer's real `~/.mcp-inspector/mcp.json`, which made these smokes both destructive and non-deterministic — `smoke:web:app`'s deep link persists a `deep-link` server row, so a *second* run found it already on disk, raced hydration, and drew a spurious (swallowed, non-fatal) 409 that was really just residue from the previous run. CI never saw it: a fresh `HOME` per run made every CI run look like a first run. This matches `smoke:cli` / `smoke:tui`, which have always driven a temp `--catalog`. Only the **catalog** is redirected — other per-user state under `~/.mcp-inspector` (OAuth tokens, `storage/`) stays shared, because isolating it means redirecting `HOME` wholesale, which would also move the npx and Playwright caches these smokes depend on. Teardown uses **both** halves of `scripts/lib/child-cleanup.mjs` (`stopChild` to await the child's exit, then `removeSafe` to delete the dir) — a bare `kill()` only *delivers* the signal, so removing synchronously re-enters the #1801 ENOTEMPTY race. That makes `stop()` **async**, so every caller must `await` it (and a caller's own `fail()`/`shutdown()` becomes async in turn, or execution runs past the intended exit). The isolation contract is unit-tested in `scripts/lib/prod-web-server.test.mjs` via `test:scripts`, since the smokes exit immediately after teardown and so cannot detect a regression that silently reshared the catalog or stopped cleaning up: `createTempCatalog` and `buildWebServerEnv` cover *which* catalog the server gets, and `teardownWebServer` — extracted from `stop()` for exactly this reason — is driven against a stand-in child process so the teardown asserts on the real directory rather than a spy. - `smoke:web:browser` (`scripts/smoke-web-browser.mjs`, #1615) goes a step further than `smoke:web`: it boots the same prod `--web` server and then actually **runs** the bundle in headless Chromium (Playwright — already a `clients/web` devDependency for the Storybook tests), asserting the app renders its first meaningful frame (the "Add Servers" control) with **no uncaught error**. `smoke:web` only checks the served HTML, so a Node built-in reaching the browser bundle slipped through it; this smoke catches that regression as a _class_ (e.g. #1612). The mechanism is the uncaught error, not a magic string: under Vite the excluded module becomes an empty stub and the first _call_ into it (e.g. `fs.readFileSync(...)` during a transitive module's init) throws a `TypeError` that aborts app mount. A _synchronous_ such throw fires `pageerror`; its _async_ twin (the same `TypeError` via `await`/`.then()`, or a failed dynamic import) is logged on the console channel as `Uncaught (in promise) …` / `Failed to fetch dynamically imported module` — the smoke hard-fails on both. The literal `Module "…" has been externalized` text is, **in a prod build**, a build-time warning (`vite build` / `npm run build`), not a runtime message, so the browser never sees it (under `npm run dev` Vite's stub is instead a `Proxy` that `console.warn`s that string at runtime); and an externalized import that is never _called_ ships a harmless `{}` and is invisible here by design. Every _other_ console error is printed as a diagnostic, not a failure (so a benign font-CDN or React-warning `console.error` doesn't flake CI). Playwright is resolved via `createRequire` based at `clients/web/package.json` — a bare `import("playwright")` would resolve relative to `scripts/`, not the cwd, so it can't be reached that way (it only appears to work when an ancestor `node_modules` carries playwright, and fails in CI, which has none). The npm script's `cd clients/web` exists only so `npx playwright install chromium` finds the local playwright bin (a no-op when already installed). -- `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=&openApp=…&appArgs=…&autoOpen=`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout. +- `smoke:web:app` (`scripts/smoke-web-app.mjs`, #1859) goes one step further again: `smoke:web:browser` stops at first paint and never connects to a server, so the Apps tab, the sandbox controller, and the UI-protocol bridge were unexercised by any smoke. This one boots the same prod `--web` server, spawns the `mcp-app-http.json` composable test server (the `mcp_app_demo` tool + its `mcp_app_demo_widget` UI resource), and drives the whole **connect → open app → widget ready** chain through a single deep-link navigate (`?serverUrl=…&autoConnect=&openApp=…&appArgs=…&autoOpen=`). The assertion is the `data-app-status="ready"` contract from [clients/web/README.md](clients/web/README.md) — the renderer reports `ready` only once the widget has loaded inside the sandbox iframe _and_ fired `notifications/initialized` back through the bridge, so one attribute covers the sandbox proxy being served, the UI resource loading, and the handshake completing. Two mechanics are load-bearing and easy to get wrong: the test server announces readiness on **stderr** (`console.error` in `server-composable.ts`), so both child streams are piped and scanned — watching stdout alone times out with an empty diagnostic; and its bound port is **not** the config's, because `createTestServerHttp` resolves through `findAvailablePort()`, which walks upward when the configured port is taken — so the smoke parses the announced URL rather than assuming `3130`. Both mechanics now live in `scripts/lib/announced-child.mjs` rather than in the smoke, so the failure path is testable: this smoke's happy path always receives the announcement, so nothing it could assert would prove that a child alive *through* the 30s timeout is still reachable by `shutdown()` — the case that orphaned a live server (#2000). The helper publishes the child via `onSpawn` before waiting, and `scripts/lib/announced-child.test.mjs` drives real `node -e` children (not spies) to assert it is published, still alive when the throw lands, and actually killable. Same reason `teardownWebServer` was extracted from `prod-web-server.mjs`'s `stop()`. **Scope note:** this runs against the repo build tree like every other smoke, so it would _not_ have caught #1859 itself (a packaging failure — the file is always present in-repo); `pack:verify` owns that dimension. It does carry a cheap structural pre-check that the proxy page exists at the path `sandbox-controller.ts` resolves, so a move/rename fails fast with a clear cause instead of an opaque render timeout. - `smoke:web:elicit` (`scripts/smoke-web-elicitation.mjs`, #1854) is the app-rendered **elicitation** counterpart of `smoke:web:app`: same prod `--web` server and the same deep-link connect, but it then calls `app_choose_option` from the Tools tab, waits for `[data-testid="app-elicitation"][data-app-elicitation-status="ready"]`, clicks a choice **inside the sandboxed app** (two `frameLocator` hops — the trusted sandbox proxy, then the untrusted app), and asserts the app's standard `ElicitResult` comes back in the *tool result*, i.e. that it reached the server rather than merely the host. It then repeats against `app-elicitation-native-http.json` — the same tool and app on a server that never advertised the nested MCP Apps `elicitation` capability — and asserts the **native** elicitation dialog takes it and no app modal is rendered. That second half is the more valuable one: the failure this feature can produce is not "the app doesn't render" but "an app renders when it should not have been offered one", which strands every user of a server that never opted in. Set `SMOKE_SCREENSHOT_DIR` to capture PNGs of the three states (used for PR proof); unset, it asserts only. Two mechanics worth knowing: the main-view tabs are a Mantine `SegmentedControl`, so there is no `role="tab"` — the clickable element is the sibling `label[for$="-Tools"]`; and the prompt string also appears in the (hidden) Protocol-tab payload, so the fallback assertion is scoped to the dialog rather than a bare text lookup. - **The build gate for the browser-externalized-builtin class (#1769)** is the earlier, more complete companion to `smoke:web:browser`. A Vite plugin in `clients/web/vite.config.ts` (logic in `clients/web/server/browser-externalized-builtin-gate.ts`, unit-tested) turns Vite 8's _browser-externalization warning_ (`Module "node:*" has been externalized for browser compatibility`) into a hard `vite build` error, so a Node built-in in the browser graph now **fails `npm run build` / `validate`** instead of shipping a `{}` stub. This catches **both** the _called-at-init_ case (which `smoke:web:browser` also catches, but later/at runtime) **and** the _imported-but-never-called_ case (the `{}` stub that is invisible to the runtime smoke "by design" — see above). Because rolldown **swallows a throw inside `onLog`** (the one hook where a thrown error doesn't abort — verified against vite@8.0.0), the plugin _records_ the warning in `onLog` and re-throws in `buildEnd`. There is **no stable log `code`**, so the gate keys off the documented message phrasing; `npm run verify:build-gate` (`scripts/verify-build-gate.mjs`, in `npm run ci` and the GitHub workflow) runs a real build with a `node:fs` probe forced into `src/main.tsx` and asserts the build fails via the gate — the only check that catches the message phrasing **drifting** in a future Vite bump and silently disabling the gate. The gate is scoped to `vite build` (`apply: 'build'`) — never `vite dev` or the vitest projects — **and** to the browser (`client`) environment (`applyToEnvironment`), so a future SSR/node environment built from this config isn't failed for a legitimate `node:*` import; the Node runner build (tsup, `build:runner`) is a separate config where built-ins are legitimate. `smoke:web:browser` stays as the runtime backstop for crashes the build can't reason about. diff --git a/README.md b/README.md index 672d86a6..49528454 100644 --- a/README.md +++ b/README.md @@ -368,7 +368,7 @@ Each client self-validates from its own folder; the root scripts chain them. The | `npm run smoke` | End-to-end smokes through the built launcher (`--help` dispatch + prod cli/tui/web), plus three headless-Chromium smokes: a boot smoke that runs the prod web bundle and asserts a clean first render (no uncaught error — sync exception or unhandled rejection, how a Node built-in reaching the browser bundle manifests), and an **MCP Apps** smoke (`smoke:web:app`) that drives connect → open app → `data-app-status="ready"` against a composable App server, covering the sandbox proxy and UI-protocol bridge, and an **app-rendered elicitation** smoke (`smoke:web:elicit`) that drives one end to end — call the tool, answer inside the sandboxed app, see the app's `ElicitResult` reach the server — and then the same tool against a server that never advertised the capability, which must fall back to the native elicitation form. | | `npm run verify:build-gate` | Runs a real `vite build` with a Node built-in forced into the browser graph and asserts the build **fails** via the #1769 gate (which turns Vite's browser-externalization warning into a hard error). Guards against the warning phrasing drifting in a Vite bump and silently disabling the gate. Part of `npm run ci`. | | `npm run verify:format-coverage` | Parses the `format:check` globs out of every `package.json` (only those reachable from `validate`), enumerates all tracked source files, and **fails** listing any not covered by a glob — the durable guard for the "every first-party source file is format-gated" invariant (#1792). Runs first in `validate`. | -| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus `scripts/lib/resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | +| `npm run test:scripts` | Table-driven unit tests (`node --test`) for the guard's own pure parsers (`scripts/lib/npm-scripts.mjs`, `scripts/lib/tsc-program.mjs` + the exported helpers of `verify-typecheck-coverage.mjs` and `verify-dep-lockstep.mjs`), one case per rule they encode, plus two suites over shared `scripts/lib` helpers that no smoke can check itself: `resolve-node-bin.test.mjs` — the cross-platform bin resolver (#1939), pinned against the real `bin`/`exports` shapes of the packages the scripts actually spawn — and `announced-child.test.mjs` — the spawn/readiness ownership helper (#2000), which drives real `node -e` children to prove a child that never announces is still published to the caller before the timeout throws, and so is reachable by teardown rather than orphaned. Runs in `validate` — and `verify:typecheck-coverage` guards *this* gate in turn (reachable from `validate`, non-empty test set, every test file matched by the `test:scripts` glob), since `node --test` silently skips a file its glob misses and still exits 0. | | `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*. Runs in `validate`. | `npm run ci` | **Mandatory pre-push command.** `validate` → `coverage` → `verify:build-gate` → `smoke` → Storybook. A true superset of GitHub CI. | @@ -504,9 +504,17 @@ Setting the passphrase later is safe — the next write upgrades an existing pla The Inspector writes the file `0600` and re-tightens it at startup if something loosened it. If it _cannot_ — the file belongs to another user, or the mount is read-only — it says so in the log rather than continuing to describe the file as protected, since on that box the mode claim above is not true. -**Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — there is deliberately **no lock**: writers are allowed to collide and the loser is made to notice. Each mutation reads the file, applies its change, writes, then reads back and compares the whole map; if another process wrote in between, it re-applies onto what they left and retries, and after five lost rounds it fails loudly rather than returning as though the value were saved. +**Two Inspectors, one file.** Within a process, mutations are serialized per file path, so a web session's own concurrent saves cannot lose each other. Across processes — a CLI run beside a web session — each mutation takes an exclusive lock on `secrets.json.lock` for the whole read-modify-write, using [`proper-lockfile`](https://github.com/moxystudio/node-proper-lockfile) (the same library npm itself locks with). The lock expires 10 seconds after its holder stops refreshing it, so an Inspector that is killed mid-save does not leave the file unwritable. -This is **not mutual exclusion**, and the residual case is worth stating: the verify only catches a clobber that has already landed, so if one Inspector reads back *before* the other's write arrives, both report success and one value is gone. That needs two Inspectors writing the same file within the gap between one's write and its read-back — narrow, but not only a crash. An earlier build did take a lock (`secrets.json.lock`); it was removed because making a `mkdir` lock single-winner on a stale takeover needs a compare-and-swap on a directory entry that Node does not expose, so it had the same class of failure with several hundred more lines and no way to close it. +Two running Inspectors are therefore genuinely serialized. What a lock file cannot make single-winner is the *takeover of a lock whose holder died* — that needs a compare-and-swap on a directory entry (`renameat2`) which Node does not expose, and it is what an earlier hand-rolled attempt failed three review rounds on. `proper-lockfile` does not close that race either. The window opens only after a holder dies without releasing. + +The Inspector adds one thing on top: every lock-directory removal the library makes on its behalf — on release, and from its exit handler — is guarded by a check that the directory is still the one it created (by inode and birth time, which survive the library's own refresh but not a delete-and-recreate). That matters because those removals are otherwise unconditional, so a holder whose lock had been replaced would delete the *winner's* lock on the way out, turning one compromised writer into two unprotected ones. It also surfaces the takeover as a warning. Treat all of this as **best-effort**: the guard is still a check followed by an act, so it makes the destructive case rare rather than impossible, and it rests on filesystem metadata that not every filesystem reports. + +Which is why, underneath the lock, each mutation still reads the file, applies its change, writes, then reads back and compares the whole map; if something wrote in between it re-applies onto what was left and retries, failing loudly after five lost rounds rather than returning as though the value were saved. That check is what still catches a clobber inside that window — and it covers what no lock can, since a lock only orders the writers that *take* it: an editor, a restored backup, or an Inspector older than this release. + +If another process holds the lock and will not let go, the save **fails** rather than going ahead unlocked — waiting past the stale window first, so a crashed Inspector resolves itself rather than failing everyone else's saves. Writing alongside a writer you can see is the one case where degrading would lose the secret it was trying to protect. + +It is also what covers the lock being unavailable. This store exists for boxes where the usual mechanism isn't there, so a directory that can't hold a lock file — a read-only `$HOME`, a mount owned by another uid — makes the save proceed unlocked with a warning, rather than turning every `set` into a failure on exactly the deployments the store was written for. Three env vars affect where the file lands. `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` picks the store outright, bypassing the probe. `MCP_INSPECTOR_SECRET_FILE` names the file. Failing both, the file follows `MCP_STORAGE_DIR` — the same variable that relocates OAuth tokens and `client.json` — so mounting a volume at your configured storage directory is enough to make secrets durable there. diff --git a/clients/cli/tsup.config.ts b/clients/cli/tsup.config.ts index 03cf7431..c37897ca 100644 --- a/clients/cli/tsup.config.ts +++ b/clients/cli/tsup.config.ts @@ -21,6 +21,13 @@ export default defineConfig({ noExternal: [/^@inspector\/core/], external: [ "@napi-rs/keyring", + // Root-declared (see the repo's dependency-placement rule) and CJS, which + // is the combination that bites: tsup externalizes what the *client's* + // package.json declares, so a root-only dependency is bundled unless named + // here — and inlining a CJS module into an ESM bundle leaves esbuild's + // `Dynamic require of "path" is not supported` shim, which throws at + // import time and takes the whole binary down before it parses a flag. + "proper-lockfile", "@modelcontextprotocol/client", "@modelcontextprotocol/core", "commander", diff --git a/clients/tui/tsup.config.ts b/clients/tui/tsup.config.ts index 2c0197a9..109011ba 100644 --- a/clients/tui/tsup.config.ts +++ b/clients/tui/tsup.config.ts @@ -120,6 +120,13 @@ export default defineConfig({ "@modelcontextprotocol/client", "@modelcontextprotocol/core", "@napi-rs/keyring", + // Root-declared (see the repo's dependency-placement rule) and CJS, which + // is the combination that bites: tsup externalizes what the *client's* + // package.json declares, so a root-only dependency is bundled unless named + // here — and inlining a CJS module into an ESM bundle leaves esbuild's + // `Dynamic require of "path" is not supported` shim, which throws at + // import time and takes the whole binary down before it parses a flag. + "proper-lockfile", ], esbuildPlugins: [inkFormLabelPatch], esbuildOptions(options) { diff --git a/clients/web/.npmignore b/clients/web/.npmignore index 133cadc2..ca99459a 100644 --- a/clients/web/.npmignore +++ b/clients/web/.npmignore @@ -8,11 +8,15 @@ # load. (The other clients don't hit this because none of them ship a nested # .gitignore.) # -# Crucially this file does NOT list `build` or `dist`, so both the prod runner -# (build/) and the SPA (dist/) are packed. The root "files" allowlist already -# restricts publishing to those two directories, so everything else in -# clients/web (src, configs, node_modules, coverage, storybook-static) stays out -# regardless — the entries below are just belt-and-suspenders. +# Crucially this file does NOT list `build`, `dist`, or `static`, so all three +# are packed: the prod runner (build/), the SPA (dist/), and the MCP Apps +# sandbox proxy page (static/sandbox_proxy.html — a committed source file, read +# from disk at runtime by server/sandbox-controller.ts as +# `/../static/sandbox_proxy.html`, so it must ship at exactly that +# path; #1859). The root "files" allowlist names those three directories and +# nothing else under clients/web, so the rest (src, configs, node_modules, +# coverage, storybook-static) stays out regardless — the entries below are just +# belt-and-suspenders. node_modules coverage storybook-static diff --git a/clients/web/server/vite-base-config.ts b/clients/web/server/vite-base-config.ts index c351f473..3d9c1a90 100644 --- a/clients/web/server/vite-base-config.ts +++ b/clients/web/server/vite-base-config.ts @@ -34,6 +34,14 @@ const NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE = [ // excluding it keeps Vite's dep scanner from chasing into the // platform-specific binaries during dev startup. "@napi-rs/keyring", + // `proper-lockfile` is reached only through `core/auth/node/file-lock.ts` + // — the secrets file's cross-process lock (#2082) — which the Hono + // `/api/servers` handlers pull in via `core/auth/node/file-secret-store.ts`. + // Same node-only import chain as `atomically` above, and the same reason: + // it is CJS with a `graceful-fs`/`signal-exit`/`retry` graph that Vite's + // dev scanner has no business walking. Note the tsup `external` lists do + // **not** cover this — they configure the production bundles, not `vite dev`. + "proper-lockfile", ] as const; export function getViteBaseConfig() { diff --git a/clients/web/src/App.test.tsx b/clients/web/src/App.test.tsx index 032f46e9..beb1ba55 100644 --- a/clients/web/src/App.test.tsx +++ b/clients/web/src/App.test.tsx @@ -1655,6 +1655,9 @@ describe("App task wiring", () => { type RootsFakeClient = EventTarget & { setRoots: ReturnType; getRoots: ReturnType; + // Close also pushes the settings it decided to apply, which is what the + // failed-save case asserts on (#2089). + setServerSettings: ReturnType; }; const settingsWithRoots = ( @@ -1745,6 +1748,99 @@ describe("App roots live-apply on settings-dialog close", () => { ); expect(client.setRoots).not.toHaveBeenCalled(); }); + + it("applies the last persisted settings on close after a save failed (#2089)", async () => { + // `useSettingsDraft` keeps the draft when a save rejects, so closing the + // modal would push a value that never reached disk into the live client — + // undoing the rollback that reported the failure. Close has to apply what + // landed instead. + const user = userEvent.setup(); + updateServerSettingsSpy.mockClear(); + const failedDraft: InspectorServerSettings = { + ...settingsWithRoots([{ uri: "file:///rejected" }]), + paginatedLists: true, + }; + const client = await openSettingsForConnectedServer(failedDraft); + client.getRoots.mockReturnValue([]); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + // One save lands, so the tracker has an account of disk… + const persistedSettings: InspectorServerSettings = { + ...settingsWithRoots([]), + paginatedLists: false, + }; + await act(async () => { + await draftOptions.onPersist("A", persistedSettings); + }); + // …then the next one rejects, leaving the draft holding an unsaved edit. + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await act(async () => { + await draftOptions.onPersist("A", failedDraft).catch(() => undefined); + }); + client.setServerSettings.mockClear(); + + await closeModal(user); + + await waitFor(() => expect(client.setServerSettings).toHaveBeenCalled()); + expect(client.setServerSettings).toHaveBeenLastCalledWith( + persistedSettings, + ); + // …and the rejected roots edit is not advertised either. + expect(client.setRoots).not.toHaveBeenCalled(); + }); + + it("restores the roots and log size too when a flushed save rejects (#2089)", async () => { + // Close flushes a pending save and live-applies the draft immediately — + // roots and the Network log size, not just the settings object. If that + // save then rejects, reconciling only `setServerSettings` would leave the + // server advertising roots that were never persisted. + const user = userEvent.setup(); + updateServerSettingsSpy.mockClear(); + const rejectedDraft: InspectorServerSettings = { + ...settingsWithRoots([{ uri: "file:///rejected" }]), + maxFetchRequests: 10, + }; + const client = await openSettingsForConnectedServer(rejectedDraft); + client.getRoots.mockReturnValue([]); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + const persistedSettings = settingsWithRoots([]); + await act(async () => { + await draftOptions.onPersist("A", persistedSettings); + }); + + // Close applies the draft live — the state the flush leaves behind. + const fetchLog = fetchLogInstances.at(-1) as EventTarget & { + setMaxFetchRequests: ReturnType; + }; + await closeModal(user); + await waitFor(() => + expect(client.setRoots).toHaveBeenCalledWith([ + { uri: "file:///rejected" }, + ]), + ); + expect(fetchLog.setMaxFetchRequests).toHaveBeenLastCalledWith(10); + client.getRoots.mockReturnValue([{ uri: "file:///rejected" }]); + client.setRoots.mockClear(); + + // …and only now does the flushed save reject. + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await act(async () => { + await draftOptions.onPersist("A", rejectedDraft).catch(() => undefined); + }); + + // The whole live surface goes back to what landed — settings, roots, and + // the log size, each of which close had moved to the rejected draft. + await waitFor(() => expect(client.setRoots).toHaveBeenCalledWith([])); + expect(client.setServerSettings).toHaveBeenLastCalledWith( + persistedSettings, + ); + expect(fetchLog.setMaxFetchRequests).toHaveBeenLastCalledWith( + persistedSettings.maxFetchRequests, + ); + }); }); describe("App history pin/replay", () => { @@ -2451,6 +2547,485 @@ describe("App paginated list pagination toggle (#1721)", () => { expect(lastPush?.paginatedLists).toBeFalsy(); }); + it("rolls back to the last write that landed, not to a stale list entry (#2089)", async () => { + // Two toggles. The first PUT lands but the list reload behind it fails, so + // the `servers` entry keeps describing disk as it was *before* it — which + // this suite models exactly, because `useServers` is mocked to return the + // same entry forever. The second PUT fails outright and rolls back. + // + // Reverting to the `servers` entry there lands on `false`, the value from + // before the first toggle, contradicting what is on disk. The baseline has + // to come from the write that landed instead. + // + // One failed toggle cannot distinguish the two baselines — they only differ + // once a write has landed that the list never caught up with — so the test + // needs the full sequence. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("paginated-on")); + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledWith( + "A", + expect.objectContaining({ paginatedLists: true }), + ), + ); + + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await user.click(screen.getByText("paginated-off")); + + // Back to the value the first toggle put on disk, not to the pre-toggle one. + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + const client = clientInstances[0] as EventTarget & { + setServerSettings: ReturnType; + }; + const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as { + paginatedLists?: boolean; + }; + expect(lastPush?.paginatedLists).toBe(true); + }); + + it("rolls back to a write that landed while this one was in flight (#2089)", async () => { + // Overlapping toggles. The baseline cannot be captured when a write is + // issued: toggle 1 is still in flight when toggle 2 reads it, so toggle 2 + // would close over `false` — and by the time toggle 2 fails, toggle 1 has + // landed `true` on disk. Rolling back to the captured value contradicts + // disk exactly as the stale-list case does; the baseline has to be resolved + // at failure time. The two writes are driven by deferred promises so the + // order — issue 1, issue 2, land 1, fail 2 — is exact rather than timed. + const user = userEvent.setup(); + let landFirst: (() => void) | undefined; + let failSecond: ((err: Error) => void) | undefined; + const first = new Promise((resolve) => { + landFirst = resolve; + }); + const second = new Promise((_resolve, reject) => { + failSecond = (err) => { + reject(err); + }; + }); + updateServerSettingsSpy + .mockImplementationOnce(() => first) + .mockImplementationOnce(() => second); + + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("paginated-on")); + await user.click(screen.getByText("paginated-off")); + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledTimes(2), + ); + + await act(async () => { + landFirst?.(); + await first; + }); + await act(async () => { + failSecond?.(new Error("disk full")); + await second.catch(() => undefined); + }); + + // `true` is what the landed write put on disk — not the `false` the second + // toggle read before it got there. + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + const client = clientInstances[0] as EventTarget & { + setServerSettings: ReturnType; + }; + const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as { + paginatedLists?: boolean; + }; + expect(lastPush?.paginatedLists).toBe(true); + }); + + it("re-applies a write that lands after an overlapping one failed (#2089)", async () => { + // The mirror of the test above, in the other settlement order: toggle 1 + // (`true`) is still in flight when toggle 2 (`false`) fails, so the rollback + // resolves to the stale entry's `false` — correct at that instant, since + // nothing had landed. Toggle 1 then lands `true`. If its list reload failed + // too, no render will ever correct the UI, so the write that settles has to + // re-apply itself. + const user = userEvent.setup(); + let landFirst: (() => void) | undefined; + let failSecond: ((err: Error) => void) | undefined; + const first = new Promise((resolve) => { + landFirst = resolve; + }); + const second = new Promise((_resolve, reject) => { + failSecond = (err) => { + reject(err); + }; + }); + updateServerSettingsSpy + .mockImplementationOnce(() => first) + .mockImplementationOnce(() => second); + + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("paginated-on")); + await user.click(screen.getByText("paginated-off")); + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledTimes(2), + ); + + // Fail the second write first, then land the first. + await act(async () => { + failSecond?.(new Error("disk full")); + await second.catch(() => undefined); + }); + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("false"); + await act(async () => { + landFirst?.(); + await first; + }); + + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + const client = clientInstances[0] as EventTarget & { + setServerSettings: ReturnType; + }; + const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as { + paginatedLists?: boolean; + }; + expect(lastPush?.paginatedLists).toBe(true); + }); + + it("re-applies a settled modal save after a toggle failed first (#2089)", async () => { + // The mixed-writer version of the order above: the modal's flush is in + // flight with `paginatedLists: true` when the sidebar toggle fails and + // rolls back to the stale entry's `false`. The modal save then lands, so it + // is the settled state — and it has to re-apply itself for the same reason + // the toggle does, or disk holds `true` while the UI and the live client + // sit on `false`. + const user = userEvent.setup(); + let landModal: (() => void) | undefined; + const modalWrite = new Promise((resolve) => { + landModal = resolve; + }); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + updateServerSettingsSpy.mockImplementationOnce(() => modalWrite); + const persisted = draftOptions.onPersist("A", { + ...settingsWithRoots([]), + paginatedLists: true, + }); + + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await user.click(screen.getByText("paginated-off")); + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("false"), + ); + + await act(async () => { + landModal?.(); + await persisted; + }); + + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + const client = clientInstances[0] as EventTarget & { + setServerSettings: ReturnType; + }; + const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as { + paginatedLists?: boolean; + }; + expect(lastPush?.paginatedLists).toBe(true); + }); + + it("applies a settled write to the current client, not the one it started on (#2089)", async () => { + // A write can outlive a disconnect/reconnect to the *same* server: the id + // guard still passes, but the client captured when the write was issued has + // been destroyed and replaced by one built from the stale list entry. The + // settled value has to reach the live instance. + const user = userEvent.setup(); + let landWrite: (() => void) | undefined; + const write = new Promise((resolve) => { + landWrite = resolve; + }); + updateServerSettingsSpy.mockImplementationOnce(() => write); + + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("paginated-on")); + // Reconnect to the same server while that write is in flight. The transport + // drops, then the user reconnects — and `onToggleConnection` always + // rebuilds the client so the latest saved settings are picked up. + act(() => { + clientInstances[0].dispatchEvent(new Event("disconnect")); + }); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(2)); + const replacement = clientInstances[1] as EventTarget & { + setServerSettings: ReturnType; + }; + replacement.setServerSettings.mockClear(); + + await act(async () => { + landWrite?.(); + await write; + }); + + await waitFor(() => + expect(replacement.setServerSettings).toHaveBeenCalledWith( + expect.objectContaining({ paginatedLists: true }), + ), + ); + }); + + it("reconciles after a modal save fails onto a write that landed first (#2089)", async () => { + // Reverse of the mixed-writer case above: the toggle lands while the modal + // save is still pending, so it is told it is not settled and applies + // nothing. The modal save then fails, which makes the toggle's value the + // account of disk — and nothing else is left to apply it. + const user = userEvent.setup(); + let failModal: ((err: Error) => void) | undefined; + const modalWrite = new Promise((_resolve, reject) => { + failModal = (err) => { + reject(err); + }; + }); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + updateServerSettingsSpy.mockImplementationOnce(() => modalWrite); + const persisted = draftOptions + .onPersist("A", { ...settingsWithRoots([]), paginatedLists: false }) + .catch(() => undefined); + + // The toggle lands `true` while the modal save is still in flight. + await user.click(screen.getByText("paginated-on")); + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledTimes(2), + ); + const client = clientInstances[0] as EventTarget & { + setServerSettings: ReturnType; + }; + client.setServerSettings.mockClear(); + + await act(async () => { + failModal?.(new Error("disk full")); + await persisted; + }); + + // The failure is what reconciles: a push carrying the landed value arrives + // only after it, since the landed write itself was not settled at the time. + await waitFor(() => + expect(client.setServerSettings).toHaveBeenCalledWith( + expect.objectContaining({ paginatedLists: true }), + ), + ); + }); + + it("seeds the settings modal from the last write, not the stale entry (#2089)", async () => { + // The modal draft is the input to the *next* write, so seeding it from a + // `servers` entry that a failed list reload froze would send the superseded + // value back to disk the moment the user saves any unrelated field. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + await user.click(screen.getByText("paginated-on")); + await waitFor(() => + expect(updateServerSettingsSpy).toHaveBeenCalledWith( + "A", + expect.objectContaining({ paginatedLists: true }), + ), + ); + + // `servers` is mocked to a fixed entry here, so it still reports the + // pre-toggle value — exactly what a failed reload leaves behind. + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + expect(draftOptions.resolveInitial("A")).toEqual( + expect.objectContaining({ paginatedLists: true }), + ); + }); + + it("builds the next client from the last write, not the stale entry (#2089)", async () => { + // Connecting is where the whole connection is configured, and it read the + // `servers` entry directly — so a save that landed while list reads were + // failing (including one made from the modal for a server that was not + // connected at the time) was undone by the next connect, which rebuilt the + // client from the frozen entry. + const user = userEvent.setup(); + renderWithMantine(); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + const saved: InspectorServerSettings = { + ...settingsWithRoots([{ uri: "file:///saved" }]), + maxFetchRequests: 42, + }; + await act(async () => { + await draftOptions.onPersist("A", saved); + }); + + // `servers` still reports the entry with no settings at all — what a failed + // reload leaves. The client must be built from the write that landed. + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + const clientOptions = vi + .mocked(McpIndex.InspectorClient) + .mock.calls.at(-1)?.[1] as { roots?: { uri: string }[] } | undefined; + expect(clientOptions?.roots).toEqual([{ uri: "file:///saved" }]); + const fetchLogOptions = vi + .mocked(FetchLogModule.FetchRequestLogState) + .mock.calls.at(-1)?.[1] as { maxFetchRequests?: number } | undefined; + expect(fetchLogOptions?.maxFetchRequests).toBe(42); + }); + + it("lets a successful list read supersede a concrete rollback override (#2089)", async () => { + // A rollback sets the override to a value rather than clearing it, so the + // effect that drops it cannot key on the persisted boolean alone: if a + // later read reports the value the override replaced — an edit made outside + // the Inspector overtaking the write — neither that boolean nor the server + // id changes, and the UI would stay stuck on the override forever. A read + // rebuilds the list, so the entry object is the signal that supersedes it. + const user = userEvent.setup(); + const previousUseServers = vi.mocked(useServers).getMockImplementation(); + try { + const { rerender } = renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + // A write lands `true` while list reads are stale, then a toggle fails — + // leaving the override on `true` with the entry still reporting `false`. + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + await act(async () => { + await draftOptions.onPersist("A", { + ...settingsWithRoots([]), + paginatedLists: true, + }); + }); + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await user.click(screen.getByText("paginated-off")); + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + + // Now a list read succeeds and still says `false` — same boolean, same + // server id, new entry object. It is authoritative and must win. + vi.mocked(useServers).mockReturnValue({ + servers: [{ ...SERVER_A } as ServerEntry], + loading: false, + error: undefined, + refresh: vi.fn().mockResolvedValue(undefined), + addServer: vi.fn(), + updateServer: vi.fn(), + updateServerSettings: updateServerSettingsSpy, + removeServer: vi.fn(), + reorderServers: vi.fn(), + importSource: vi.fn().mockResolvedValue({ servers: {} }), + }); + rerender(); + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent( + "false", + ), + ); + } finally { + if (previousUseServers) { + vi.mocked(useServers).mockImplementation(previousUseServers); + } + } + }); + + it("rolls back to a settings-modal save that landed, not just a toggle (#2089)", async () => { + // The two settings writers feed one record, and this is the half the + // toggle-driven tests cannot reach: `useSettingsDraft` is mocked for this + // file, so its `onPersist` never runs on its own and a regression that + // dropped `begin`/`landed` from the modal path would go unnoticed. The + // callback the App handed the hook is invoked directly instead. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + const saved: InspectorServerSettings = { + ...settingsWithRoots([]), + paginatedLists: true, + }; + // The save lands; the list read behind it is what stays stale, so `servers` + // (mocked to a fixed entry here) never reports it. + await act(async () => { + await draftOptions.onPersist("A", saved); + }); + expect(updateServerSettingsSpy).toHaveBeenCalledWith("A", saved); + + updateServerSettingsSpy.mockRejectedValueOnce(new Error("disk full")); + await user.click(screen.getByText("paginated-off")); + + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + const client = clientInstances[0] as EventTarget & { + setServerSettings: ReturnType; + }; + const lastPush = client.setServerSettings.mock.calls.at(-1)?.[0] as { + paginatedLists?: boolean; + }; + expect(lastPush?.paginatedLists).toBe(true); + }); + + it("treats a modal save whose list reload failed as landed (#1914 + #2089)", async () => { + // A `ServerListReloadError` rejects *after* the PUT landed, so the save is + // on disk. It has to be recorded as the last landed write and applied like + // any other settled one — the failure path would roll the UI, the live + // client and the modal's seed back to a value disk no longer holds. + const user = userEvent.setup(); + renderWithMantine(); + await user.click(screen.getByText("connect")); + await waitFor(() => expect(clientInstances).toHaveLength(1)); + + const draftOptions = vi.mocked(useSettingsDraft).mock.calls.at(-1)?.[0]; + if (!draftOptions) throw new Error("useSettingsDraft was never called"); + updateServerSettingsSpy.mockRejectedValueOnce( + new ServerListReloadError( + "The server was saved, but the server list could not be reloaded: disk full", + ), + ); + const saved: InspectorServerSettings = { + ...settingsWithRoots([]), + paginatedLists: true, + }; + await act(async () => { + await draftOptions.onPersist("A", saved).catch(() => undefined); + }); + + // `servers` is mocked to a fixed entry here — exactly what a failed reload + // leaves — so both of these read the record rather than the entry. + expect(draftOptions.resolveInitial("A")).toEqual( + expect.objectContaining({ paginatedLists: true }), + ); + await waitFor(() => + expect(screen.getByTestId("tools-paginated")).toHaveTextContent("true"), + ); + }); + it("keeps the optimistic toggle when only the list reload failed (#1914)", async () => { // The mirror of the test above. `refreshAfterWrite` rejects with a // `ServerListReloadError` when the PUT landed and only reading the list diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index a99f212e..f308c477 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -113,6 +113,7 @@ import { usePagedTools } from "@inspector/core/react/usePagedTools.js"; import { usePagedPrompts } from "@inspector/core/react/usePagedPrompts.js"; import { usePagedResources } from "@inspector/core/react/usePagedResources.js"; import { usePaginatedList } from "./hooks/usePaginatedList"; +import { useLastPersistedSettings } from "./hooks/useLastPersistedSettings"; import { useValueChange } from "./hooks/useValueChange"; import type { ListPaginationControlsProps } from "./components/elements/ListPaginationControls/ListPaginationControls"; import { useManagedResourceTemplates } from "@inspector/core/react/useManagedResourceTemplates.js"; @@ -1163,19 +1164,34 @@ function App() { error: pagedResourcesLoadError, loadPage: loadResourcesPage, } = usePagedResources(inspectorClient, pagedResourcesState); + // What every settings write that landed on disk actually wrote, so a later + // failed write can be rolled back to that rather than to a `servers` entry + // frozen at the last successful list read (#2089). + const lastPersistedSettings = useLastPersistedSettings(servers); // The active server's persisted paginated setting drives the display mode. // The sidebar toggle edits it (optimistically, below) and persists it. + const activeServerEntry = servers.find((s) => s.id === activeServerId); const persistedPaginatedLists = - servers.find((s) => s.id === activeServerId)?.settings?.paginatedLists ?? - false; + activeServerEntry?.settings?.paginatedLists ?? false; const [paginatedListsOverride, setPaginatedListsOverride] = useState< boolean | null >(null); // Drop the optimistic override once the persisted value catches up (or the // active server changes), so the persisted setting is the resting source. - useEffect(() => { - setPaginatedListsOverride(null); - }, [persistedPaginatedLists, activeServerId]); + // + // Keyed on the entry object rather than on the boolean it carries: the + // override can hold a *concrete* value (a rollback sets one instead of + // clearing), and if a later successful read reports the value that override + // replaced — an outside edit overtaking the write — the boolean and the + // server id are both unchanged and the UI stays stuck on it. Every successful + // read rebuilds the list, so the entry is the signal that supersedes it, and + // it subsumes the other two: the value is read off this entry, and switching + // servers selects a different one (#2089). + // + // Adjusted during render rather than in an effect, per this repo's rule + // against resetting state from render data in `useEffect` — the entry is + // referentially stable between reads, which is what `useValueChange` needs. + useValueChange(activeServerEntry, () => setPaginatedListsOverride(null)); const paginatedLists = paginatedListsOverride ?? persistedPaginatedLists; // The malformed-entry report is written by the aggregate walk's salvage. In // paginated mode the tools/prompts/resources panels render the paged stores @@ -1719,11 +1735,18 @@ function App() { const activeServerNameRef = useRef(undefined); const activeServerIdRef = useRef(activeServerId); const serversRef = useRef(servers); + // The live client, mirrored for the same reason: a settings write that + // outlives a disconnect/reconnect to the *same* server would otherwise push + // its value into the instance captured when it was issued — which has since + // been destroyed — while the replacement client, built from the stale list + // entry, keeps the value the write replaced (#2089). + const inspectorClientRef = useRef(inspectorClient); useEffect(() => { if (activeServer) activeServerNameRef.current = activeServer.name; activeServerIdRef.current = activeServerId; serversRef.current = servers; - }, [activeServer, activeServerId, servers]); + inspectorClientRef.current = inspectorClient; + }, [activeServer, activeServerId, servers, inspectorClient]); // Last connection-level error message, surfaced as `data-error-message` on // the InspectorView header so an automated driver can read *why* a connect @@ -1910,6 +1933,43 @@ function App() { // rebound every time the active server (and its log state) changes. const fetchLogRef = useRef(null); + // Make the live client reflect one settings value, in full. Every path that + // decides what the active server's settings *are* goes through this: the + // settings modal's close (#1444), and each write's settled / rolled-back + // reconciliation (#2089). It is one function because the live surface is more + // than `setServerSettings` — a partial re-application would leave the Network + // log sized for, and the server advertised roots from, a value that is not on + // disk. Reads the client through its ref so a write outliving a reconnect + // applies to the instance that exists now. + const applyLiveServerSettings = useCallback( + (settings: InspectorServerSettings) => { + const client = inspectorClientRef.current; + if (!client) return; + // Settings the managed state reads at notification time + // (auto-refresh-on-list-changed) take effect without a reconnect (#1444). + // Connection-time inputs (transport, OAuth, timeouts) still only apply on + // the next connect. + client.setServerSettings(settings); + // Resize the Network log buffer live so a maxFetchRequests edit takes + // effect without a reconnect (shrinking trims immediately). + fetchLogRef.current?.setMaxFetchRequests(settings.maxFetchRequests); + // Root edits are diffed against what the client currently advertises + // (both cleaned) and notified only when they differ: `setRoots` fires + // `notifications/roots/list_changed`, which makes the server re-request + // `roots/list`. + const nextRoots = cleanRoots(settings.roots); + const currentRoots = cleanRoots(client.getRoots()); + if (JSON.stringify(nextRoots) !== JSON.stringify(currentRoots)) { + void client.setRoots(nextRoots).catch(() => { + // setRoots swallows notification failures internally; a throw here + // only means the client is mid-teardown — the persisted roots will + // re-advertise on the next connect, so nothing to surface. + }); + } + }, + [], + ); + // Flush the pre-redirect Network log to backend storage, keyed by the OAuth // authId carried in the authorization URL's `state`. Runs synchronously from // `BrowserNavigation` right before `window.location.href`, so the keepalive @@ -2465,7 +2525,16 @@ function App() { ); // The settings node persisted in mcp.json for this server — distinct // from the InspectorClient options we're about to derive from it. - const savedSettings = server.settings; + // + // Through the tracker, not `server.settings` directly: the entry only + // advances on a successful list read, so a write that landed while reads + // were failing — including one made from the settings modal for a server + // that was not connected at the time — would otherwise be undone at the + // next connect, which builds the client from that frozen entry. This is + // the one place the whole connection is configured, so every construction + // path (connect, reconnect, OAuth resume) goes through it (#2089). + const savedSettings = + lastPersistedSettings.resolve(server.id) ?? server.settings; const activeIdp = getActiveEnterpriseManagedAuthIdp(clientConfig); const activeCimdUrl = getActiveCimdClientMetadataUrl(clientConfig); // Flatten the persisted settings into the InspectorClient options shape. @@ -2637,6 +2706,7 @@ function App() { onBeforeOAuthRedirect, clientConfig, newAppElicitationSession, + lastPersistedSettings, ], ); @@ -3734,9 +3804,12 @@ function App() { setPaginatedListsOverride(value); const current = servers.find((s) => s.id === activeServerId); if (!current || activeServerId === undefined) return; - const prevSettings = current.settings ?? EMPTY_SETTINGS; + // Not `current.settings` directly: that entry only advances on a + // successful list read, so once one has failed it describes disk as it + // was *before* the writes made since. Build on the last write known to + // have landed while that is the fresher account (#2089). const next: InspectorServerSettings = { - ...prevSettings, + ...(lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS), paginatedLists: value, }; inspectorClient?.setServerSettings(next); @@ -3761,35 +3834,91 @@ function App() { // server's rehydrated secrets, so it can trigger the pending // plaintext-to-encrypted upgrade even though the user only toggled // pagination (#1950 review r22). + // Announced before the request goes out, so two toggles in flight at once + // are ordered by when they were issued rather than by which one's list + // reload finished first (#2089). + const write = lastPersistedSettings.begin(activeServerId); + // This value is on disk now. Remember it as the rollback baseline for + // whatever is written next, since the `servers` entry it was derived + // from will keep describing the old value if the reload behind this + // write — or any later one — fails (#2089). + // + // Re-apply it when this write is the settled one: an overlapping toggle + // that failed *first* rolled the UI and the live client back to a + // baseline this write has since replaced, and if the list read behind + // this write failed too, nothing else would ever correct them. Through + // the *current* client, not this continuation's closure: a reconnect to + // the same server passes the id check while the captured instance is + // already destroyed. + const settlePaginationWrite = () => { + const settled = write.landed(next); + if (settled && activeServerIdRef.current === activeServerId) { + setPaginatedListsOverride(next.paginatedLists ?? false); + applyLiveServerSettings(next); + } + }; void refreshingPersist(updateServerSettings, refreshInitialConfig)( activeServerId, next, - ).catch((err: unknown) => { - // A ServerListReloadError means the PUT landed and only reading the - // list back failed, so the new setting IS on disk (#1914). Rolling - // back here would put the UI and the live client on the *old* value - // and contradict it — report it and leave the optimistic state alone. - const reloadFailed = err instanceof ServerListReloadError; - if (!reloadFailed) { - // Persist failed: revert the optimistic override (the effect only - // clears it when the persisted value changes, which won't happen here) - // and roll the live client setting back, so the UI and client reflect - // the value that's actually on disk rather than the failed edit (#1721). - setPaginatedListsOverride(null); - inspectorClient?.setServerSettings(prevSettings); - } - notifications.show({ - title: reloadFailed - ? "Pagination setting saved, but the server list did not reload" - : "Failed to save pagination setting", - message: err instanceof Error ? err.message : String(err), - color: "red", + ) + .then(settlePaginationWrite) + .catch((err: unknown) => { + // A `ServerListReloadError` means the PUT landed and only reading the + // list back failed, so the new setting IS on disk (#1914). That is a + // landed write, not a failed one: rolling back would put the UI and + // the live client on the *old* value and contradict disk. Settle it + // exactly as the success path does and report only the failed reload. + if (err instanceof ServerListReloadError) { + settlePaginationWrite(); + notifications.show({ + title: + "Pagination setting saved, but the server list did not reload", + message: err.message, + color: "red", + }); + return; + } + // This write is over and never reached disk, so it stops counting as + // in flight: an earlier write still running is the settled state once + // it lands, and is what re-applies the UI this rollback is about to + // set (#2089). + write.failed(); + // Persist failed: revert the optimistic override and roll the live + // client setting back, so the UI and client reflect the value that's + // actually on disk rather than the failed edit (#1721). + // + // The baseline is resolved *here*, not captured when this write was + // issued: another toggle can land in between, and its value is what + // disk holds by the time this one fails. The override is set to that + // baseline rather than cleared, because clearing it falls back to + // `persistedPaginatedLists` — read from a `servers` entry that may be + // stale, showing the same wrong value from the other side (#2089). + // + // Applied only while this write's server is still the active one: the + // override is a single app-wide boolean and the live client belongs to + // whichever server is connected now, so a rejection arriving after the + // user switched would render this server's value on another one. + // The client is taken from the ref for the same reason the success + // path does: a reconnect to the same server passes the id check while + // this continuation's captured instance is already destroyed. + const baseline = + lastPersistedSettings.resolve(activeServerId) ?? EMPTY_SETTINGS; + if (activeServerIdRef.current === activeServerId) { + setPaginatedListsOverride(baseline.paginatedLists ?? false); + applyLiveServerSettings(baseline); + } + notifications.show({ + title: "Failed to save pagination setting", + message: err instanceof Error ? err.message : String(err), + color: "red", + }); }); - }); }, [ servers, activeServerId, + lastPersistedSettings, + applyLiveServerSettings, inspectorClient, updateServerSettings, refreshInitialConfig, @@ -4197,8 +4326,12 @@ function App() { flush: flushSettingsDraft, } = useSettingsDraft({ targetId: settingsModalTargetId, - resolveInitial: (id) => - servers.find((s) => s.id === id)?.settings ?? EMPTY_SETTINGS, + // Through the tracker rather than off `servers` directly: after a write + // lands whose list reload failed, that entry still describes the previous + // value, so seeding from it and saving an unrelated field would write the + // superseded value straight back to disk (#2089). `resolve` falls back to + // the entry when nothing has landed, so the ordinary case is unchanged. + resolveInitial: (id) => lastPersistedSettings.resolve(id) ?? EMPTY_SETTINGS, // The saved settings may include an OAuth client secret or a stdio `env:` // value, and writing one can change what the secrets file *is* — the first // save under a newly-set passphrase re-encrypts a pre-existing plaintext @@ -4206,7 +4339,60 @@ function App() { // unit rather than an inline `await …; refresh()` because this file is // outside the coverage gate, so wiring written here is tested by nothing // (#1950 review r17). - onPersist: refreshingPersist(updateServerSettings, refreshInitialConfig), + onPersist: async (id: string, value: InspectorServerSettings) => { + // Announced before the request, like the toggle's own write: the debounced + // flush can put two saves for one server in flight, and the later-issued + // one describes disk however the two happen to finish (#2089). + const write = lastPersistedSettings.begin(id); + const settleSettingsWrite = () => { + const settled = write.landed(value); + if (settled && activeServerIdRef.current === id) { + setPaginatedListsOverride(value.paginatedLists ?? false); + applyLiveServerSettings(value); + } + }; + try { + await refreshingPersist(updateServerSettings, refreshInitialConfig)( + id, + value, + ); + } catch (err) { + // A `ServerListReloadError` means the PUT landed and only reading the + // list back failed, so this write did reach disk (#1914): record it + // like the success path below instead of rolling anything back, and + // rethrow so `onError` can say the reload — not the save — failed. + if (err instanceof ServerListReloadError) { + settleSettingsWrite(); + throw err; + } + // Stop counting as in flight before the rejection goes on to + // `onError` — an earlier write still running settles the state once it + // lands, and cannot know that while this one looks pending (#2089). + write.failed(); + // A write that landed while this one was pending was told it was not + // settled and so applied nothing; this save never reached disk, so that + // write is the account of it and nothing else will re-apply it. Same + // reconciliation the toggle's own failure path does. + const baseline = lastPersistedSettings.resolve(id); + if (baseline && activeServerIdRef.current === id) { + setPaginatedListsOverride(baseline.paginatedLists ?? false); + applyLiveServerSettings(baseline); + } + throw err; + } + // Recorded for the same reason the pagination toggle records its own + // write: this is what disk holds now, and the `servers` entry it was + // edited from will keep describing the previous value for as long as list + // reads keep failing. Both settings writers feed the same per-server + // record, so a rollback in either takes the most recent write for that + // server, not just the most recent write *of its own kind*. + // + // And the reconciliation is shared too, for the mixed-writer overlap: a + // toggle failing while this save is in flight rolls the UI and the live + // client back to a baseline this save then replaces on disk, so a settled + // save has to re-apply itself exactly as a settled toggle does (#2089). + settleSettingsWrite(); + }, // Surface failures via toast — the modal usually closes // immediately on user dismiss, so a silent fail-on-flush would // leave the user thinking their last edits saved when they @@ -4346,25 +4532,20 @@ function App() { settingsModalTargetId === activeServerId && settingsDraft ) { - // Push the edited settings onto the live client so settings the managed - // state reads at notification time (auto-refresh-on-list-changed) take - // effect without a reconnect (#1444). Connection-time inputs (transport, - // OAuth, timeouts) still only apply on the next connect. - inspectorClient.setServerSettings(settingsDraft); - // Resize the Network log buffer live so a maxFetchRequests edit takes - // effect without a reconnect (shrinking trims immediately). Connect-time - // construction also reads this, so a reconnect would apply it anyway — - // this just makes the toast→adjust flow responsive. - fetchLogRef.current?.setMaxFetchRequests(settingsDraft.maxFetchRequests); - const nextRoots = cleanRoots(settingsDraft.roots); - const currentRoots = cleanRoots(inspectorClient.getRoots()); - if (JSON.stringify(nextRoots) !== JSON.stringify(currentRoots)) { - void inspectorClient.setRoots(nextRoots).catch(() => { - // setRoots swallows notification failures internally; a throw here - // only means the client is mid-teardown — the persisted roots will - // re-advertise on the next connect, so nothing to surface. - }); - } + // Normally the draft is what the user just saved, so it is also what is + // on disk. When this server's last write *rejected* it is not: the draft + // still holds the failed edit, and applying it here would contradict disk + // and undo the rollback that reported the failure — so apply what landed + // instead. `EMPTY_SETTINGS`, not the draft, when nothing has ever landed + // for this server: falling back to the draft would re-apply the very + // values that failed (#2089). + const applied = lastPersistedSettings.lastWriteFailed( + settingsModalTargetId, + ) + ? (lastPersistedSettings.resolve(settingsModalTargetId) ?? + EMPTY_SETTINGS) + : settingsDraft; + applyLiveServerSettings(applied); } setSettingsModalTargetId(undefined); }, [ @@ -4373,6 +4554,8 @@ function App() { settingsModalTargetId, activeServerId, settingsDraft, + lastPersistedSettings, + applyLiveServerSettings, ]); // The Resources screen needs `isSubscribed` to flip the Subscribe button diff --git a/clients/web/src/hooks/useLastPersistedSettings.test.tsx b/clients/web/src/hooks/useLastPersistedSettings.test.tsx new file mode 100644 index 00000000..4141fe22 --- /dev/null +++ b/clients/web/src/hooks/useLastPersistedSettings.test.tsx @@ -0,0 +1,225 @@ +import { describe, it, expect } from "vitest"; +import type { + InspectorServerSettings, + ServerEntry, +} from "@inspector/core/mcp/types.js"; +import { renderWithMantine } from "../test/renderWithMantine"; +import { + useLastPersistedSettings, + type LastPersistedSettings, +} from "./useLastPersistedSettings"; + +const settings = ( + overrides: Partial = {}, +): InspectorServerSettings => ({ + headers: [], + env: [], + metadata: [], + connectionTimeout: 0, + requestTimeout: 0, + taskTtl: 0, + autoRefreshOnListChanged: false, + paginatedLists: false, + maxFetchRequests: 0, + roots: [], + ...overrides, +}); + +/** + * One `servers` entry, a fresh object per call — most of these tests turn on + * object identity, so a shared fixture would defeat them. A list rebuilt from + * fresh calls is what a successful `GET /api/servers` produces. + */ +const entry = ( + id: string, + settingsNode?: InspectorServerSettings, +): ServerEntry => ({ + id, + name: id, + config: { type: "stdio", command: "node" }, + connection: { status: "disconnected" }, + settings: settingsNode, +}); + +interface Harness { + api: LastPersistedSettings; + /** Deliver a new server list, as a successful list read would. */ + setServers: (next: ServerEntry[]) => void; +} + +/** + * Hands the hook's API back to the test. The hook holds refs, so it has to be + * read from inside a rendered component; `setServers` re-renders it with a new + * list the way `useServers` does after a read. + */ +function harness(initial: ServerEntry[]): Harness { + let api: LastPersistedSettings | undefined; + function Probe({ servers }: { servers: ServerEntry[] }) { + api = useLastPersistedSettings(servers); + return null; + } + const { rerender } = renderWithMantine(); + if (!api) throw new Error("hook did not render"); + return { + api, + setServers: (next) => rerender(), + }; +} + +describe("useLastPersistedSettings", () => { + it("reads the list entry when no write has landed for that server", () => { + const onDisk = settings({ paginatedLists: true }); + const { api } = harness([entry("A", onDisk)]); + expect(api.resolve("A")).toBe(onDisk); + }); + + it("returns nothing when the list has no settings and nothing landed", () => { + const { api } = harness([entry("A")]); + expect(api.resolve("A")).toBeUndefined(); + }); + + it("returns the recorded write while the entry has not been re-read", () => { + const { api } = harness([entry("A", settings())]); + const written = settings({ paginatedLists: true }); + api.begin("A").landed(written); + expect(api.resolve("A")).toBe(written); + }); + + it("ignores a record made for a different server", () => { + const onDiskB = settings(); + const { api } = harness([entry("A"), entry("B", onDiskB)]); + api.begin("A").landed(settings({ paginatedLists: true })); + expect(api.resolve("B")).toBe(onDiskB); + }); + + it("keeps each server's record, so a save on B doesn't discard A's", () => { + // The settings modal can be opened for any server, connected or not, so + // this sequence is reachable: A's write lands while list reads are failing, + // B is saved from the modal, and only then does a write on A fail. A single + // slot would have lost A's baseline and reproduced #2089. + const { api } = harness([entry("A", settings()), entry("B", settings())]); + const writtenA = settings({ paginatedLists: true }); + api.begin("A").landed(writtenA); + api.begin("B").landed(settings({ autoRefreshOnListChanged: true })); + expect(api.resolve("A")).toBe(writtenA); + }); + + it("stops trusting the record once a fresh list read replaces the entry", () => { + const { api, setServers } = harness([entry("A", settings())]); + api.begin("A").landed(settings({ paginatedLists: true })); + // A successful `GET /api/servers` rebuilds the list, so the entry is a new + // object even when its values are unchanged. That is the signal the list + // has caught up, and it is authoritative again from here. + const fresh = settings(); + setServers([entry("A", fresh)]); + expect(api.resolve("A")).toBe(fresh); + }); + + it("pairs the record with the entry as of completion, not of issue", () => { + // A list read that lands while the write is in flight must not be mistaken + // for one that happened after it. The pairing is taken in `landed`, so the + // read below — which the write outlives — does not invalidate the record. + const { api, setServers } = harness([entry("A", settings())]); + const write = api.begin("A"); + setServers([entry("A", settings())]); + const written = settings({ paginatedLists: true }); + write.landed(written); + expect(api.resolve("A")).toBe(written); + }); + + it("keeps only the most recent write for a server", () => { + const { api } = harness([entry("A", settings())]); + api.begin("A").landed(settings({ paginatedLists: true })); + const later = settings({ autoRefreshOnListChanged: true }); + api.begin("A").landed(later); + expect(api.resolve("A")).toBe(later); + }); + + it("lets the later-issued write win however the two finish", () => { + // `updateServerSettings` waits for a list read after its PUT, so an older + // write with a slow read can complete last. Ordering by completion would + // then record a value that a newer write has already replaced on disk, and + // the next failed write would roll back to it. + const { api } = harness([entry("A", settings())]); + const first = api.begin("A"); + const second = api.begin("A"); + const newest = settings({ paginatedLists: true }); + second.landed(newest); + first.landed(settings({ paginatedLists: false })); + expect(api.resolve("A")).toBe(newest); + }); + + it("reports settled only for the last write issued", () => { + // The flag is what tells a caller it is safe to re-apply the value to the + // UI: while a later write is still in flight, that write will describe disk + // when it reports, and re-applying now would fight it. + const { api } = harness([entry("A", settings())]); + const first = api.begin("A"); + expect(first.landed(settings({ paginatedLists: true }))).toBe(true); + const second = api.begin("A"); + const third = api.begin("A"); + expect(second.landed(settings({ paginatedLists: false }))).toBe(false); + expect(third.landed(settings({ paginatedLists: true }))).toBe(true); + // A superseded straggler is neither recorded nor settled. + expect(second.landed(settings({ paginatedLists: false }))).toBe(false); + }); + + it("treats an earlier write as settled once the later one has failed", () => { + // The order that leaves the UI wrong: the later write fails first, rolling + // the UI back to a baseline the earlier write then replaces on disk. Once + // the failed write stops counting as in flight, the earlier one settles and + // its caller can re-apply the value. + const { api } = harness([entry("A", settings())]); + const first = api.begin("A"); + const second = api.begin("A"); + second.failed(); + const written = settings({ paginatedLists: true }); + expect(first.landed(written)).toBe(true); + expect(api.resolve("A")).toBe(written); + }); + + it("does not let a straggler resurrect a record a fresh read dropped", () => { + // The high-water mark outlives the record, so an earlier write reporting + // after `resolve` invalidated things cannot reinstate a superseded value. + const { api, setServers } = harness([entry("A", settings())]); + const first = api.begin("A"); + api.begin("A").landed(settings({ paginatedLists: true })); + const fresh = settings(); + setServers([entry("A", fresh)]); + expect(api.resolve("A")).toBe(fresh); + first.landed(settings({ paginatedLists: false })); + expect(api.resolve("A")).toBe(fresh); + }); + + it("reports a failed last write per server, ordered by issue", () => { + // The settings modal's draft survives a rejected save, so its owner has to + // know whether what the user last tried to save is on disk. A failure on B + // says nothing about A, and a straggling failure from an older write does + // not overrule a newer write that succeeded. + const { api } = harness([entry("A", settings()), entry("B", settings())]); + expect(api.lastWriteFailed("A")).toBe(false); + + const firstA = api.begin("A"); + const secondA = api.begin("A"); + secondA.landed(settings({ paginatedLists: true })); + firstA.failed(); + expect(api.lastWriteFailed("A")).toBe(false); + + api.begin("B").failed(); + expect(api.lastWriteFailed("B")).toBe(true); + expect(api.lastWriteFailed("A")).toBe(false); + + // …and a newer failure on A does count, until a newer write lands. + api.begin("A").failed(); + expect(api.lastWriteFailed("A")).toBe(true); + api.begin("A").landed(settings()); + expect(api.lastWriteFailed("A")).toBe(false); + }); + + it("returns nothing when the server has left the list entirely", () => { + const { api, setServers } = harness([entry("A", settings())]); + api.begin("A").landed(settings({ paginatedLists: true })); + setServers([]); + expect(api.resolve("A")).toBeUndefined(); + }); +}); diff --git a/clients/web/src/hooks/useLastPersistedSettings.ts b/clients/web/src/hooks/useLastPersistedSettings.ts new file mode 100644 index 00000000..86e7649c --- /dev/null +++ b/clients/web/src/hooks/useLastPersistedSettings.ts @@ -0,0 +1,216 @@ +import { useCallback, useEffect, useRef } from "react"; +import type { + InspectorServerSettings, + ServerEntry, +} from "@inspector/core/mcp/types.js"; + +/** + * Remembers, per server, the last settings write known to have reached disk, so + * a *later* failed write can be rolled back to what is actually persisted + * rather than to a `servers` entry that has gone stale (#2089). + * + * The `servers` list only advances when a `GET /api/servers` succeeds. Once a + * list read fails, every entry in it keeps describing disk *as of the last + * successful read* — while writes issued since then have changed disk. A + * rollback that reads its baseline from such an entry reverts to a value + * nothing on disk holds, and the UI contradicts the file until the next + * successful read. The reproduction is two toggles: the first write lands but + * its list reload fails, the second write fails outright and reverts to the + * pre-first-toggle value. + * + * Records are kept **per server id**, not in a single slot. The settings modal + * can be opened for any server, independently of which one is connected, so a + * single slot would let a save on B discard the baseline a later failed write + * on A still needs — reproducing the very bug this hook exists to prevent. + * + * ## Which of two overlapping writes wins + * + * A write is announced with `begin` when it is *issued* and confirmed with + * `landed` when it completes, and a confirmation is ignored if a later-issued + * write for that server has already confirmed. Completion order cannot be used + * for this: `updateServerSettings` waits for a list read after its PUT, so an + * older write with a slow read can finish last and would otherwise overwrite + * the record with a value that is no longer on disk. + * + * Issue order is a proxy for disk order, not a proof of it — HTTP does not + * promise that two in-flight PUTs arrive in the order they were sent. It is the + * best signal available on this side: the backend serializes writes to the + * catalog, but neither the route nor `useServers` returns a revision the client + * could order by. If one is added later, this is where it replaces the counter. + * + * ## When a record stops being believed + * + * A record is paired with the `servers` entry for that server **as of when the + * write completed**, and is dropped once the list carries a different entry + * object: a successful read rebuilds the list, so a fresh object is the signal + * that the entry has caught up with — or been overtaken by (an edit made + * outside the Inspector) — our write, and from there the entry is authoritative + * again. The pairing is taken at completion rather than at issue because a read + * that lands *while the write is in flight* would otherwise be mistaken for one + * that happened after it. + * + * One case the client cannot decide is left: a background read whose response + * was generated before the write landed but which arrives after it still looks + * newer here, and the record is dropped in its favour. That needs the same + * missing revision as above. What the fallback yields in that case is the + * freshest successful read — the same value the rest of the UI renders from — + * rather than an arbitrary one. + * + * The list and the records are held in refs rather than state deliberately: + * nothing renders from either, they are read only inside callbacks at the + * moment a write is issued, completes, or fails, and holding them in state + * would re-render the whole composition root on every settings save. + */ +export interface LastPersistedSettings { + /** + * Announce a settings write for `serverId` at the moment it is **issued**. + * Confirm it with the returned handle once it completes. + */ + begin: (serverId: string) => SettingsWrite; + /** + * The best available answer to "what is on disk for this server", **as of the + * moment it is called**. Returns the recorded write while the list still + * carries the entry that write was paired with, otherwise that server's + * current entry settings, and `undefined` when the list has no such entry (or + * one carrying no settings) and no write has landed for it this session. + * + * Both the record and the list are read live, so calling this in a failure + * handler answers for the state at failure time. Do not hoist the result into + * a variable at the point the write is issued and reuse it on rejection: an + * overlapping write that landed in between would make it stale, which is the + * same class of bug as reading the baseline from a stale `servers` entry. + */ + resolve: (serverId: string) => InspectorServerSettings | undefined; + /** + * Whether the most recently **issued** write that has reported for this + * server ended in failure — i.e. whatever the user last tried to save is not + * what disk holds. A caller holding an edit buffer for that server (the + * settings modal's draft) uses it to know its buffer is unpersisted, and to + * apply `resolve` instead of the buffer. + * + * Ordered by issue, not by arrival, for the same reason the record is: a + * straggling failure from an older write does not overwrite the outcome of a + * newer one, and outcomes are kept per server so a failure on B says nothing + * about A. + */ + lastWriteFailed: (serverId: string) => boolean; +} + +export interface SettingsWrite { + /** + * Record that this write reached disk. A no-op if a write issued after this + * one has already been confirmed for the same server. + * + * Returns whether this write is now the **settled** state for that server — + * recorded, with no later write still in flight to describe disk after it. + * Callers that own UI derived from the setting use it to re-apply the value: + * an overlapping write that failed *first* will have rolled that UI back to a + * baseline this write has since replaced, and if the list read behind this + * one failed too, nothing else would ever correct it. + */ + landed: (written: InspectorServerSettings) => boolean; + /** + * Report that this write is over without reaching disk, so it stops counting + * as in flight. An earlier write that lands afterwards is then the settled + * state — which is exactly the order that leaves the UI on a rolled-back + * value nothing on disk holds. + */ + failed: () => void; +} + +interface WriteRecord { + written: InspectorServerSettings; + entry: ServerEntry | undefined; +} + +/** + * @param servers the current server list, as rendered + */ +export function useLastPersistedSettings( + servers: ServerEntry[], +): LastPersistedSettings { + const recordsRef = useRef>(new Map()); + const nextSequenceRef = useRef(0); + // Highest issue order confirmed per server. Kept apart from the records so a + // record dropped by `resolve` can't be resurrected by a straggler that was + // issued earlier: the high-water mark outlives the record it produced. + const confirmedRef = useRef>(new Map()); + // Issue orders still in flight per server, so a write that lands can tell + // whether it is the last word or whether something later will still report. + // A write leaves this set through `landed` or `failed` alike. + const pendingRef = useRef>>(new Map()); + // Outcome of the latest-issued write that has reported, per server. Kept + // apart from the record because a *failed* write leaves no record but is + // still the answer to "is the user's last attempt on disk?". + const outcomesRef = useRef>( + new Map(), + ); + // Read the list through a ref so a write pairs with the entry as it stands + // when it finishes, not the one captured when it was issued. Mirrored in an + // effect rather than assigned during render: writing a ref mid-render is an + // error under `react-hooks/refs`, and every reader runs from a settled + // promise or an event handler, long after the commit. + const serversRef = useRef(servers); + useEffect(() => { + serversRef.current = servers; + }, [servers]); + + const begin = useCallback((serverId: string): SettingsWrite => { + const sequence = ++nextSequenceRef.current; + const pending = pendingRef.current.get(serverId) ?? new Set(); + pending.add(sequence); + pendingRef.current.set(serverId, pending); + const settle = (ok: boolean) => { + pending.delete(sequence); + // Keep the outcome of the latest-issued write that has reported, so an + // older straggler cannot overwrite a newer one's verdict. + const previous = outcomesRef.current.get(serverId); + if (!previous || previous.sequence <= sequence) { + outcomesRef.current.set(serverId, { sequence, ok }); + } + }; + return { + landed: (written: InspectorServerSettings) => { + settle(true); + const confirmed = confirmedRef.current.get(serverId) ?? 0; + // A write issued after this one already reported; it describes disk + // more recently, whichever of the two happened to finish first. + if (confirmed > sequence) return false; + confirmedRef.current.set(serverId, sequence); + const entry = serversRef.current.find((s) => s.id === serverId); + recordsRef.current.set(serverId, { written, entry }); + // Settled only while nothing later is still in flight — such a write + // will describe disk once it reports, so re-applying this value in the + // meantime would fight it. + for (const other of pending) if (other > sequence) return false; + return true; + }, + failed: () => settle(false), + }; + }, []); + + const lastWriteFailed = useCallback( + (serverId: string) => outcomesRef.current.get(serverId)?.ok === false, + [], + ); + + const resolve = useCallback( + (serverId: string): InspectorServerSettings | undefined => { + const entry = serversRef.current.find((s) => s.id === serverId); + const current = recordsRef.current.get(serverId); + if (!current) return entry?.settings; + // Identity, not deep equality: the question is whether the list has been + // re-read since the write, not whether the values happen to match. + if (current.entry !== entry) { + // Superseded — drop it rather than re-checking a dead record on every + // later write for this server. + recordsRef.current.delete(serverId); + return entry?.settings; + } + return current.written; + }, + [], + ); + + return { begin, resolve, lastWriteFailed }; +} diff --git a/clients/web/src/test/core/react/useServers.test.tsx b/clients/web/src/test/core/react/useServers.test.tsx index 2ca1d334..a54de4f1 100644 --- a/clients/web/src/test/core/react/useServers.test.tsx +++ b/clients/web/src/test/core/react/useServers.test.tsx @@ -878,7 +878,9 @@ describe("useServers", () => { fetchFn: async (input, init) => { const url = input instanceof Request ? input.url : String(input); if (url.endsWith("/api/servers/events")) { - return { ok: true, body: null } as unknown as Response; + // A real Response constructed from `null` has a null `.body`, + // so the guard is exercised through the actual Response API. + return new Response(null, { status: 200 }); } return h.fetchFn(url, init); }, @@ -897,12 +899,14 @@ describe("useServers", () => { fetchFn: async (input, init) => { const url = input instanceof Request ? input.url : String(input); if (url.endsWith("/api/servers/events")) { - const body = { - getReader: () => ({ - read: () => Promise.reject(new Error("stream broke")), - }), - }; - return { ok: true, body } as unknown as Response; + // A real stream whose first pull throws — `reader.read()` then + // rejects exactly as a broken network body would, with no cast. + const body = new ReadableStream({ + pull() { + throw new Error("stream broke"); + }, + }); + return new Response(body, { status: 200 }); } return h.fetchFn(url, init); }, @@ -931,22 +935,18 @@ describe("useServers", () => { fetchFn: async (input, init) => { const url = input instanceof Request ? input.url : String(input); if (url.endsWith("/api/servers/events")) { - const body = { - getReader: () => ({ - read: async () => { - reads += 1; - if (reads === 1) { - // Two frames in one chunk → one background refresh. - return { - done: false, - value: encoder.encode("event: change\n\n\n\n"), - }; - } - return { done: true, value: undefined }; - }, - }), - }; - return { ok: true, body } as unknown as Response; + const body = new ReadableStream({ + pull(controller) { + reads += 1; + if (reads === 1) { + // Two frames in one chunk → one background refresh. + controller.enqueue(encoder.encode("event: change\n\n\n\n")); + return; + } + controller.close(); + }, + }); + return new Response(body, { status: 200 }); } return h.fetchFn(url, init); }, @@ -1141,22 +1141,24 @@ describe("useServers", () => { const fetchFn: typeof fetch = async (input, init) => { const url = input instanceof Request ? input.url : String(input); if (url.endsWith("/api/servers/events")) { - const body = { - getReader: () => ({ - read: async () => { - reads += 1; - // Priming comment only — no `event:` / `data:` line. - if (reads === 1) { - return { done: false, value: encoder.encode(":\n\n") }; - } - // Hold the stream open so the loop can't end and let a - // teardown-time settle hide a queued refresh. - await secondRead; - return { done: true, value: undefined }; - }, - }), - }; - return { ok: true, body } as unknown as Response; + // `pull` runs once per read, so the counting and the blocking + // second read work the same way they would on a hand-rolled reader + // double — while staying type-checked against the Response API. + const body = new ReadableStream({ + async pull(controller) { + reads += 1; + // Priming comment only — no `event:` / `data:` line. + if (reads === 1) { + controller.enqueue(encoder.encode(":\n\n")); + return; + } + // Hold the stream open so the loop can't end and let a + // teardown-time settle hide a queued refresh. + await secondRead; + controller.close(); + }, + }); + return new Response(body, { status: 200 }); } if (url.endsWith("/api/servers")) listGets += 1; return h.fetchFn(url, init); diff --git a/clients/web/src/test/integration/auth/node/file-lock.test.ts b/clients/web/src/test/integration/auth/node/file-lock.test.ts new file mode 100644 index 00000000..bd4d5929 --- /dev/null +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -0,0 +1,514 @@ +/** + * `withSecretFileLock` against a real filesystem and a real second process + * (#2082). + * + * The property under test is cross-process mutual exclusion, and the + * existing suite structurally cannot reach it: `FileSecretStore.serialize` + * is one process-wide queue per path, so two in-process callers are ordered + * before the lock ever sees them. A child process is not scaffolding here — + * it is the only participant that can produce the interleaving. + */ +import { + describe, + it, + expect, + beforeEach, + afterEach, + vi, + type MockInstance, +} from "vitest"; +import { execFile } from "node:child_process"; +import { createRequire } from "node:module"; +import { existsSync } from "node:fs"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { promisify } from "node:util"; +import { + withSecretFileLock, + resetFileLockWarnings, +} from "@inspector/core/auth/node/file-lock.js"; +import { FileSecretStore } from "@inspector/core/auth/node/file-secret-store.js"; +import { SecretStoreUnavailableError } from "@inspector/core/auth/node/secret-store.js"; + +const run = promisify(execFile); +const require_ = createRequire(import.meta.url); +/** + * Resolved in the parent and handed to the child. The child's cwd is not + * this repo, and `proper-lockfile` lives in the *root* install rather than + * `clients/web`'s, so a bare `require("proper-lockfile")` there resolves + * against whatever happens to be above the temp directory — usually nothing. + */ +const LOCKFILE_MODULE = require_.resolve("proper-lockfile"); + +let tmpDir: string; +let warn: MockInstance; +const filePath = (): string => path.join(tmpDir, "secrets.json"); + +beforeEach(async () => { + tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "inspector-lock-")); + resetFileLockWarnings(); + // These paths warn by design; asserting on the text is the point, and + // letting it reach the real console would bury the suite's output. + warn = vi.spyOn(console, "warn").mockImplementation(() => {}); +}); + +afterEach(async () => { + warn.mockRestore(); + await fs.rm(tmpDir, { recursive: true, force: true }); +}); + +/** Everything `console.warn` was handed this test, as one string. */ +const warnings = (): string => + warn.mock.calls.map((c) => String(c[0])).join("\n"); + +/** + * Hold the lock on `target` in a **real second process** for `holdMs`, and + * resolve once that child confirms it has it. + * + * Resolving on the child's confirmation rather than on a sleep is what makes + * the ordering assertions below meaningful: the parent starts contending + * only once the lock is provably held elsewhere, so a pass cannot come from + * the parent simply getting there first. + */ +async function holdLockInChildProcess( + target: string, + holdMs: number, + /** + * Secrets the child writes *while holding the lock*, before it announces + * itself. A parent that honours the lock therefore reads a map that already + * contains them, which is what lets the caller assert on the merged result + * rather than only on timing. + */ + writeWhileHeld?: Record, +): Promise<{ ready: Promise; done: Promise }> { + const script = ` + const lockfile = require(${JSON.stringify(LOCKFILE_MODULE)}); + const fs = require("node:fs"); + const path = require("node:path"); + // A second Inspector reaches the lock through \`withSecretFileLock\`, which + // creates the storage directory first. Mirror that, or the fresh-install + // case below would be testing the child's omission rather than the parent. + fs.mkdirSync(path.dirname(${JSON.stringify(target)}), { recursive: true }); + lockfile + .lock(${JSON.stringify(target)}, { realpath: false, stale: 10000 }) + .then(async (release) => { + const secrets = ${JSON.stringify(writeWhileHeld ?? null)}; + if (secrets) { + fs.writeFileSync( + ${JSON.stringify(target)}, + JSON.stringify({ version: 1, encryption: "none", secrets }), + ); + } + process.stdout.write("acquired\\n"); + await new Promise((r) => setTimeout(r, ${holdMs})); + await release(); + process.stdout.write("released\\n"); + }) + .catch((err) => { + process.stdout.write("failed:" + err.code + "\\n"); + process.exitCode = 1; + }); + `; + const child = run(process.execPath, ["-e", script]); + let seenReady = false; + const done = child.then(({ stdout }) => { + expect(stdout).toContain("acquired"); + expect(stdout).toContain("released"); + }); + // `execFile` buffers, so the "acquired" line is only readable off the + // stream. Subscribe before awaiting anything, or the line is missed. + const ready = new Promise((resolve, reject) => { + child.child.stdout?.on("data", (chunk: Buffer) => { + if (!seenReady && chunk.toString().includes("acquired")) { + seenReady = true; + resolve(); + } + }); + child.catch(reject); + }); + return { ready, done }; +} + +describe("withSecretFileLock across processes", () => { + it("waits for a lock another process holds, then runs", async () => { + const target = filePath(); + const { ready, done } = await holdLockInChildProcess(target, 400); + await ready; + + const startedAt = Date.now(); + let ranAt = 0; + await withSecretFileLock(target, async () => { + ranAt = Date.now(); + }); + + // It waited rather than barging in. The child holds for 400ms and the + // retry schedule's first sleeps are tens of milliseconds, so anything + // above a floor well under 400 proves contention without pinning the + // assertion to the scheduler's exact wake-up. + expect(ranAt - startedAt).toBeGreaterThan(200); + // …and having waited, it did not report a degraded write. + expect(warnings()).toBe(""); + await done; + }, 20_000); + + it("locks a file that does not exist yet", async () => { + // The first `set` on a fresh install has no `secrets.json` — and + // `proper-lockfile` resolves its target through `fs.realpath` by + // default, which is `ENOENT` there. `realpath: false` is what makes the + // very first write lockable; without it the one call with nothing to + // fall back on is the one that runs unprotected. + const target = filePath(); + await expect(fs.stat(target)).rejects.toThrow(); + + let ran = false; + await withSecretFileLock(target, async () => { + ran = true; + }); + + expect(ran).toBe(true); + expect(warnings()).toBe(""); + }); + + it("makes FileSecretStore.set wait on a lock another process holds", async () => { + // The end-to-end shape from the issue: a CLI run beside a web session. + // + // Deliberately asserts that the parent's `set` has *not finished* while + // the child holds the lock. A test that only checks both keys survive + // afterwards passes with the lock removed from `mutate` entirely — the + // optimistic verify would repair the clobber and hide the regression. + // Not-yet-resolved is the observation only a real lock can produce. + const target = filePath(); + const { ready, done } = await holdLockInChildProcess(target, 700, { + "srv:env:FROM_CHILD": "1", + }); + await ready; + + let settled = false; + const store = new FileSecretStore({ filePath: target }); + const pending = store.set("srv", "env:FROM_PARENT", "2").then(() => { + settled = true; + }); + + // Comfortably inside the child's hold, and comfortably outside the few + // milliseconds an unlocked read-modify-write would take. + await new Promise((r) => setTimeout(r, 300)); + expect(settled).toBe(false); + + await done; + await pending; + + // Having waited, the parent read the map the child left, so its own entry + // landed *on top of* the child's rather than replacing it. + const reader = new FileSecretStore({ filePath: target }); + expect(await reader.get("srv", "env:FROM_CHILD")).toBe("1"); + expect(await reader.get("srv", "env:FROM_PARENT")).toBe("2"); + expect(warnings()).toBe(""); + }, 20_000); + + it("creates the storage directory so the very first save is locked too", async () => { + // `writeStoreFile` creates the parent directory, but from *inside* the + // locked section — so without the `mkdir` in `withSecretFileLock` the + // first save on a fresh install fails `ENOENT` on the lock and degrades + // to an unlocked write. That is the save most likely to be racing + // another, since two Inspectors started together both reach it. + const target = path.join(tmpDir, "fresh-install", "secrets.json"); + const { ready, done } = await holdLockInChildProcess(target, 700); + await ready; + + let settled = false; + const store = new FileSecretStore({ filePath: target }); + const pending = store.set("srv", "env:FIRST_EVER", "1").then(() => { + settled = true; + }); + + await new Promise((r) => setTimeout(r, 300)); + expect(settled).toBe(false); + + await done; + await pending; + + const reader = new FileSecretStore({ filePath: target }); + expect(await reader.get("srv", "env:FIRST_EVER")).toBe("1"); + // No degrade warning: the lock was genuinely held, not skipped. + expect(warnings()).toBe(""); + }, 20_000); +}); + +describe("withSecretFileLock degrades rather than failing", () => { + it("runs the body anyway when the lock cannot be created, and says so once", async () => { + // A path whose parent is a *file* stands in for every real variant — + // read-only `$HOME`, a mount owned by another uid, a filesystem without + // `mkdir` semantics — and unlike a permissions-based setup it fails the + // same way for root, so it cannot pass locally and flake in a container. + // Note a merely *missing* directory is no longer this case: + // `withSecretFileLock` creates it. This store exists for boxes where the + // usual mechanism is missing, so it must not gain a new way to be + // unavailable. + await fs.writeFile(path.join(tmpDir, "not-a-dir"), "", "utf-8"); + const target = path.join(tmpDir, "not-a-dir", "secrets.json"); + + let ran = 0; + await withSecretFileLock(target, async () => { + ran += 1; + }); + await withSecretFileLock(target, async () => { + ran += 1; + }); + + expect(ran).toBe(2); + expect(warnings()).toContain("Could not take a lock on the secrets file"); + // Once per reason per process — a warning on every save would be noise + // on precisely the deployment that cannot act on it. + expect(warn).toHaveBeenCalledTimes(1); + }); + + it("refuses the save rather than writing alongside a live holder", async () => { + // `ELOCKED` is evidence the lock is *working*, so degrading here would + // enter the exact interleaving the lock exists to prevent — and enter it + // knowing another writer is there. Held from this process, which is + // indistinguishable to `proper-lockfile` from a remote holder (it is not + // reentrant); the in-process queue is what keeps that out of the way in + // production. + // + // The wait is real: the retry budget deliberately outlasts the 10s stale + // window so a *crashed* holder resolves by takeover instead of failing + // everyone else's saves. This holder is alive and refreshing, so it never + // goes stale and the budget is spent in full. + const target = filePath(); + const lockfile = require_(LOCKFILE_MODULE) as { + lock: (f: string, o: object) => Promise<() => Promise>; + }; + // The **same** `stale` production uses, and that is not incidental: + // `isLockStale` is evaluated against the *waiter's* threshold while the + // holder refreshes on its own `stale / 2`. A holder configured looser + // (say 60s) refreshes every 30s and is therefore declared stale by a + // 10s waiter after 10s — the waiter takes over and the save succeeds, + // quietly testing the opposite of what this test claims. + const release = await lockfile.lock(target, { + realpath: false, + stale: 10_000, + }); + + const store = new FileSecretStore({ filePath: target }); + // One call, both assertions off the same rejection: each attempt spends + // the full retry budget, so a second would double the test's runtime to + // re-prove the same thing. + const err = await store + .set("srv", "env:MINE", "1") + .catch((e: unknown) => e); + expect(err).toBeInstanceOf(SecretStoreUnavailableError); + expect((err as Error).message).toMatch(/was not saved/); + await release(); + + // Nothing was written behind the holder's back. + expect(existsSync(target)).toBe(false); + }, 90_000); + + it("refuses rather than degrading when a stale lock cannot be cleared", async () => { + // `acquireLock` does not only *create* directories — on finding a stale + // one it removes it and retries, and that removal can fail. A stale lock + // with anything inside it fails `ENOTEMPTY`, which is not `ELOCKED`, and + // treating every non-`ELOCKED` error as "locks do not work here" meant + // every Inspector on the box quietly bypassed the *same* stuck lock and + // raced its writes — while the release-failure message was telling the + // operator saves would keep failing until they cleared it. + const target = filePath(); + const lockPath = `${target}.lock`; + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.mkdir(lockPath); + await fs.writeFile(`${lockPath}/stray`, "", "utf-8"); + // Backdated so it reads as stale — which is what sends `acquireLock` down + // the remove-and-retry path rather than straight to `ELOCKED`. + const longDead = new Date(Date.now() - 60_000); + await fs.utimes(lockPath, longDead, longDead); + + const store = new FileSecretStore({ filePath: target }); + const err = await store + .set("srv", "env:MINE", "1") + .catch((e: unknown) => e); + + expect(err).toBeInstanceOf(SecretStoreUnavailableError); + expect((err as Error).message).toMatch(/was not saved/); + // Nothing written behind the stuck lock, and no "unprotected" warning: + // this is a refusal, not a degrade. + expect(existsSync(target)).toBe(false); + expect(warnings()).not.toContain("not protected"); + }, 90_000); + + it("takes over the lock of a holder that died, rather than failing the save", async () => { + // The invariant behind refusing on `ELOCKED`: refusing is only defensible + // because a *crashed* holder resolves on its own first. `RETRY` therefore + // has to outlast `STALE_MS` — if the budget were the shorter of the two, + // one Inspector killed mid-save would make every later save on the box + // fail until someone deleted the lock by hand. + // + // A dead holder is exactly a lock directory nobody is refreshing, so it + // is staged directly: no child to race, and no dependence on how quickly + // a killed process is reaped. + const target = filePath(); + const lockPath = `${target}.lock`; + await fs.mkdir(path.dirname(target), { recursive: true }); + await fs.mkdir(lockPath); + const longDead = new Date(Date.now() - 60_000); + await fs.utimes(lockPath, longDead, longDead); + + const store = new FileSecretStore({ filePath: target }); + await store.set("srv", "env:AFTER_CRASH", "1"); + + const reader = new FileSecretStore({ filePath: target }); + expect(await reader.get("srv", "env:AFTER_CRASH")).toBe("1"); + // Took the lock over — did not fall through to an unlocked write. + expect(warnings()).toBe(""); + }, 60_000); + + it("stays silent per the delete contract when the lock is held", async () => { + // `delete` reports nothing by contract — only `set` hard-fails — so the + // refusal above must not turn a delete into a throw. + const target = filePath(); + const store = new FileSecretStore({ filePath: target }); + await store.set("srv", "env:A", "1"); + + const lockfile = require_(LOCKFILE_MODULE) as { + lock: (f: string, o: object) => Promise<() => Promise>; + }; + // The **same** `stale` production uses, and that is not incidental: + // `isLockStale` is evaluated against the *waiter's* threshold while the + // holder refreshes on its own `stale / 2`. A holder configured looser + // (say 60s) refreshes every 30s and is therefore declared stale by a + // 10s waiter after 10s — the waiter takes over and the save succeeds, + // quietly testing the opposite of what this test claims. + const release = await lockfile.lock(target, { + realpath: false, + stale: 10_000, + }); + await expect(store.delete("srv", "env:A")).resolves.toBeUndefined(); + await release(); + + // …and the entry it could not delete is still there, not half-removed. + expect(await store.get("srv", "env:A")).toBe("1"); + }, 90_000); +}); + +describe("withSecretFileLock reports what it cannot clean up", () => { + it("does not delete the winner's lock after being taken over", async () => { + // The destructive half of the stale-takeover race. `proper-lockfile`'s + // removal is an unconditional `rmdir`, so a holder whose lock was + // replaced deletes the *winner's* directory on the way out — one + // compromised holder becoming two unprotected writers. Its own detection + // cannot prevent that: it runs on the refresh tick (5s here) while an + // ordinary mutation finishes in well under a second. + // + // No waiting here, deliberately: this is the fast case the tick misses. + const target = filePath(); + const lockPath = `${target}.lock`; + + let winner: { ino: number; birthtimeMs: number } | undefined; + const result = await withSecretFileLock(target, async () => { + await fs.rm(lockPath, { recursive: true, force: true }); + await fs.mkdir(lockPath); + const stat = await fs.stat(lockPath); + winner = { ino: stat.ino, birthtimeMs: stat.birthtimeMs }; + return "saved"; + }); + + expect(result).toBe("saved"); + // Same directory, not a recreated one — it was left alone, not deleted. + const after = await fs.stat(lockPath); + expect(after.ino).toBe(winner?.ino); + expect(after.birthtimeMs).toBe(winner?.birthtimeMs); + expect(warnings()).toContain("was taken over by another process"); + }); + + it("the exit handler this guards against really does delete a lock", async () => { + // The other lifecycle window, and the reason the guard lives in + // `options.fs` rather than in a check before `release()`: + // `proper-lockfile` registers a `signal-exit` handler that `rmdirSync`s + // every lock it still has registered, with no ownership check of its own. + // A guard placed only around release would leave that free to delete the + // winner's directory if the process exits at the wrong moment. + // + // Shown in a real child, because the handler only runs on a real exit, + // and with an **empty** replacement directory — a non-empty one makes + // `rmdirSync` fail `ENOTEMPTY` and would "pass" for the wrong reason, + // which is exactly how the first draft of this test fooled itself. + // + // `withSecretFileLock` routes that same handler through the shim's + // `rmdirSync`, which shares `removeIfMine` with the release path proven + // by the test above. + const target = filePath(); + const lockPath = `${target}.lock`; + await fs.mkdir(path.dirname(target), { recursive: true }); + + const script = ` + const lockfile = require(${JSON.stringify(LOCKFILE_MODULE)}); + const fs = require("node:fs"); + const lockPath = ${JSON.stringify(lockPath)}; + lockfile + .lock(${JSON.stringify(target)}, { realpath: false, stale: 10000 }) + .then(() => { + // Taken over while we hold it, then exit *without* releasing. + fs.rmSync(lockPath, { recursive: true, force: true }); + fs.mkdirSync(lockPath); + process.exit(0); + }); + `; + await run(process.execPath, ["-e", script]); + + // Unguarded, the winner's directory is gone. + expect(existsSync(lockPath)).toBe(false); + }, 30_000); + + it("tells the operator to clear a lock that cannot expire on its own", async () => { + // `rmdir` refuses a non-empty directory — and so does stale takeover, + // which reclaims through the same call. So an `ENOTEMPTY` lock is not + // cleaned up by the staleness mechanism, by us, or by the next writer: + // every later save fails `ELOCKED` against it until somebody deletes it. + // Promising it "expires on its own after 10s" would be false. + const target = filePath(); + const result = await withSecretFileLock(target, async () => { + await fs.writeFile(`${target}.lock/stray`, "", "utf-8"); + return "saved"; + }); + + // The body's result is returned regardless: a save that completed must + // not be turned into a failure by its own teardown. + expect(result).toBe("saved"); + expect(warnings()).toContain("Could not release the lock"); + // Conditional guidance, not an instruction. `proper-lockfile` forwards + // whatever the filesystem returned and nothing here can tell a permanent + // refusal from a transient one — and if it was transient, this path may + // by then hold a *different, live* Inspector's lock, so an unconditional + // "remove it" would destroy the exclusion of a process that did nothing + // wrong. Both conditions must be stated. + expect(warnings()).toContain("If saves keep failing"); + expect(warnings()).toContain("no other Inspector is running"); + // …and it must not promise the lock clears by itself either, which is + // false whenever the same `rmdir` also blocks stale takeover. + expect(warnings()).not.toMatch(/It expires on its own/); + }); + + it("warns rather than crashing the process when the library detects the takeover", async () => { + // `proper-lockfile`'s default `onCompromised` *throws*, from a timer with + // no caller on the stack — an uncaught exception that takes an Inspector + // session down. Replaced with a warning. This is the slow path: the lock + // is removed and the body stays alive past the refresh tick, so the + // library's own detection fires rather than the removal guard. + const target = filePath(); + await withSecretFileLock(target, async () => { + await fs.rm(`${target}.lock`, { recursive: true, force: true }); + await vi.waitFor( + () => expect(warnings()).toContain("was taken over by another process"), + { timeout: 20_000, interval: 250 }, + ); + }); + + // And it must NOT go on to tell the operator to delete the lock. Once the + // tick has fired, `release()` answers `ERELEASED` without touching the + // filesystem — the directory sitting at that path is then whoever took + // over, so "remove it by hand" would destroy the exclusion of a process + // that did nothing wrong. `onCompromised` has already said what happened. + expect(warnings()).not.toContain("by hand"); + expect(warnings()).not.toContain("Could not release the lock"); + }, 30_000); +}); diff --git a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts index 7fb03da4..9cc2b065 100644 --- a/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts +++ b/clients/web/src/test/integration/auth/node/secret-store-selection.test.ts @@ -684,6 +684,197 @@ describe("absorbFileSecretsIntoKeyring", () => { return filePath; } + it("does not throw when another process holds the lock (#2082)", async () => { + // `withSecretFileLock` throws on a lock held past its retry budget, which + // is right for a `set` — the user is waiting on that value — and wrong + // here. This function is awaited directly by both `resolveSecretStore` + // branches, so an escaping error fails store resolution and with it the + // whole session: a stuck writer elsewhere on the box would stop the + // Inspector from starting. Leaving the file for the next run is the only + // acceptable outcome. + const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + + const properLockfile = (await import("proper-lockfile")).default; + const release = await properLockfile.lock(filePath, { + realpath: false, + stale: 10_000, + }); + + await expect( + mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()), + ).resolves.toBeUndefined(); + await release(); + + // Left exactly as it was, and said so — asserting the *lock* message + // specifically, since the pre-existing claim-failure warning also ends in + // "left in place" and would let this pass without the lock ever being hit. + expect(existsSync(filePath)).toBe(true); + expect(warn).toHaveBeenCalledWith( + expect.stringContaining("Could not lock the secrets file"), + ); + }, 60_000); + + it("takes no lock when there is nothing to migrate (#2082)", async () => { + // The overwhelmingly common startup: a keychain is available and no file + // was ever written. Locking first would create and remove a lock + // directory on every run — and on a box whose storage directory does not + // exist yet the lock cannot be created at all, so the degrade path would + // warn about unprotected writes on every single run, with nothing to + // protect. + const filePath = path.join(tmpDir, "no-such-dir", "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + + await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()); + + expect(warn).not.toHaveBeenCalled(); + expect(existsSync(`${filePath}.lock`)).toBe(false); + }); + + it("treats ENOTDIR as the fresh-install case, like ENOENT (#2082)", async () => { + // A path whose parent is a *file* answers `readdir` with ENOTDIR — the + // other shape of "there is no directory here" — so it takes the same + // silent path as a missing one. The *unlistable* case is different and is + // covered by the test below. + await fs.writeFile(path.join(tmpDir, "not-a-dir"), "", "utf-8"); + const filePath = path.join(tmpDir, "not-a-dir", "secrets.json"); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + + await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()); + + // ENOTDIR is a fresh-install shape: nothing said, nothing locked. + expect(warn).not.toHaveBeenCalled(); + }); + + // Root bypasses POSIX permission checks, and Windows does not model them + // the same way — in either case the directory below stays listable and the + // test would assert nothing. + const canDenyListing = + process.platform !== "win32" && process.getuid?.() !== 0; + + it.skipIf(!canDenyListing)( + "does not treat an unlistable directory as an empty one (#2082)", + async () => { + // The case ENOTDIR does *not* cover, and the reason the catch narrowed + // to ENOENT/ENOTDIR rather than swallowing everything. A directory with + // `--x` permission denies `readdir` with EACCES while still permitting + // access to a *known* name inside it — so reading EACCES as "nothing to + // migrate" selects the keychain and leaves those secrets invisible with + // nothing said. That asymmetry is real POSIX behaviour, which is why + // this drives it with a real mode rather than a stubbed `readdir`. + const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); + process.env.MCP_INSPECTOR_SECRET_FILE = filePath; + vi.spyOn(console, "warn").mockImplementation(() => {}); + // Write + execute, no read: `readdir` fails, `stat`/`open`/`rename` of a + // known name still work — which is exactly what the migration needs. + await fs.chmod(tmpDir, 0o300); + try { + const mod = await loadWithProbe(true); + const keyring = new InMemorySecretStore(); + await mod.absorbFileSecretsIntoKeyring(keyring); + + // It went on and migrated, rather than silently skipping. + expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file"); + } finally { + // Restore before `afterEach`, which needs to list it to remove it. + await fs.chmod(tmpDir, 0o700); + } + }, + ); + + it("does not adopt a snapshot another migration is still using (#2082)", async () => { + // A `*.migrating-*` file is not automatically an orphan: the process that + // staged it may still be reading it. Adopting one mid-hand-off links it + // back to the live path and re-claims it under a new name, so its owner's + // hand-off fails ENOENT — and if we exit before copying, that healthy + // session starts with none of those secrets. + // + // Liveness is the snapshot's own lock rather than the pid in its name: a + // pid outlives its process and recurs (pid 1 on every container start), + // whereas the lock expires by itself if the owner dies. + const inProgress = path.join(tmpDir, "secrets.json.migrating-999-abc"); + await fs.writeFile( + inProgress, + JSON.stringify({ + version: 1, + encryption: "none", + secrets: { "srv:oauthClientSecret": "still-migrating" }, + }), + "utf-8", + ); + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + vi.spyOn(console, "warn").mockImplementation(() => {}); + + const properLockfile = (await import("proper-lockfile")).default; + const release = await properLockfile.lock(inProgress, { + realpath: false, + stale: 10_000, + }); + + const mod = await loadWithProbe(true); + const keyring = new InMemorySecretStore(); + await mod.absorbFileSecretsIntoKeyring(keyring); + await release(); + + // Left exactly where its owner put it, and not migrated from under it. + expect(existsSync(inProgress)).toBe(true); + expect(existsSync(path.join(tmpDir, "secrets.json"))).toBe(false); + expect(await keyring.get("srv", "oauthClientSecret")).toBe(null); + }); + + it("ignores a snapshot's own lock directory (#2082)", async () => { + // `secrets.json.migrating--.lock` matches the plain prefix + // test, and treating it as a snapshot is self-sustaining damage: the + // liveness probe asks about a nonexistent `.lock.lock` and answers + // "not held", recovery tries to hard-link a *directory* onto the secrets + // path, fails, and prints the orphan warning — every startup, forever, + // since a liveness *check* never clears a stale lock directory. + const strayLock = path.join(tmpDir, "secrets.json.migrating-999-abc.lock"); + await fs.mkdir(strayLock); + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + + await mod.absorbFileSecretsIntoKeyring(new InMemorySecretStore()); + + // Not mistaken for a snapshot: nothing said, nothing linked, and the + // directory left exactly where it was. + expect(warn).not.toHaveBeenCalled(); + expect(existsSync(strayLock)).toBe(true); + expect(existsSync(path.join(tmpDir, "secrets.json"))).toBe(false); + }); + + it("still adopts an orphan when the live file is absent (#2082)", async () => { + // The fast path above must not be a `stat` of `secrets.json`: an + // interrupted migration leaves *only* the snapshot, which is precisely + // the case where there is everything to migrate and no live file. + const orphan = path.join(tmpDir, "secrets.json.migrating-123-abc"); + await fs.writeFile( + orphan, + JSON.stringify({ + version: 1, + encryption: "none", + secrets: { "srv:oauthClientSecret": "from-orphan" }, + }), + "utf-8", + ); + process.env.MCP_INSPECTOR_SECRET_FILE = path.join(tmpDir, "secrets.json"); + vi.spyOn(console, "warn").mockImplementation(() => {}); + const mod = await loadWithProbe(true); + const keyring = new InMemorySecretStore(); + + await mod.absorbFileSecretsIntoKeyring(keyring); + + expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-orphan"); + expect(existsSync(orphan)).toBe(false); + }); + it("moves the file's secrets into the keychain and removes the file", async () => { const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" }); process.env.MCP_INSPECTOR_SECRET_FILE = filePath; diff --git a/clients/web/src/test/integration/server/vite-base-config.test.ts b/clients/web/src/test/integration/server/vite-base-config.test.ts index fc305223..90c39abd 100644 --- a/clients/web/src/test/integration/server/vite-base-config.test.ts +++ b/clients/web/src/test/integration/server/vite-base-config.test.ts @@ -21,8 +21,14 @@ describe("getViteBaseConfig", () => { expect.arrayContaining([ "@modelcontextprotocol/client/stdio", "atomically", + "chokidar", "cross-spawn", "which", + "@napi-rs/keyring", + // #2082 — reached through `core/auth/node/file-lock.ts`. The tsup + // `external` lists cover the production bundles, not `vite dev`, so + // a node-only dependency has to be named in both places. + "proper-lockfile", ]), ); }); diff --git a/clients/web/tsup.runner.config.ts b/clients/web/tsup.runner.config.ts index 863c0e29..a6512a48 100644 --- a/clients/web/tsup.runner.config.ts +++ b/clients/web/tsup.runner.config.ts @@ -27,6 +27,13 @@ export default defineConfig({ "atomically", "chokidar", "@napi-rs/keyring", + // Root-declared (see the repo's dependency-placement rule) and CJS, which + // is the combination that bites: tsup externalizes what the *client's* + // package.json declares, so a root-only dependency is bundled unless named + // here — and inlining a CJS module into an ESM bundle leaves esbuild's + // `Dynamic require of "path" is not supported` shim, which throws at + // import time and takes the whole binary down before it parses a flag. + "proper-lockfile", "@modelcontextprotocol/client", "@modelcontextprotocol/core", ], diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts new file mode 100644 index 00000000..aaf566d6 --- /dev/null +++ b/core/auth/node/file-lock.ts @@ -0,0 +1,527 @@ +/** + * Cross-process mutual exclusion for the secrets file (#2082). + * + * **Why a library and not a hand-rolled election.** #1950 shipped without a + * lock on purpose. An earlier revision of it *did* take one — a `mkdir` + * election with an owner stamp, a heartbeat and a stale-takeover — and three + * consecutive review rounds found a real race in it. The last one is not + * closable with what Node exposes: claiming a stale lock atomically needs + * compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`), and + * without it a waiter that loses the race can move the winner's *fresh* lock + * aside and enter alongside it. + * + * So the choice #2082 settles is not "lock versus no lock" but "hand-rolled + * versus borrowed". `proper-lockfile` is the borrowed one — what npm itself + * locks with — and it is worth being exact about what it does and does not + * buy, because the temptation is to overclaim it. + * + * **What it makes exclusive.** `mkdir` is atomic, and a live holder refreshes + * the lock's mtime at `stale / 2` for as long as it lives, so its lock never + * becomes eligible for takeover. Two running Inspectors are therefore + * genuinely serialized: one holds, the other waits. That is the case #1950 + * lost updates in, and it is closed. + * + * **What it does not.** Stale takeover is still not single-winner. Reading + * `lib/lockfile.js@4.1.2`: on `EEXIST` it `stat`s, and if stale it `rmdir`s + * and re-`mkdir`s — without checking that the directory it removed is the + * one it found stale. So a slow waiter can delete a fast waiter's *fresh* + * lock and claim a replacement, and both proceed. That is the identical race + * the hand-rolled version could not close, and it cannot be closed with what + * Node exposes (`renameat2(RENAME_EXCHANGE)`). + * + * The library detects it only on its refresh tick — `updateLock` compares + * the lock's mtime against the value recorded at acquire and fires + * `onCompromised`. That tick runs at `stale / 2`, i.e. every 5s here, while + * an ordinary mutation is a read, an scrypt derivation and an atomic write: + * comfortably under a second. **So in the common case the tick never runs + * and the library tells nobody.** Worse, its `release` path calls `rmdir` + * unconditionally, with no ownership check — so a holder whose lock was + * replaced goes on to delete the *winner's* lock on the way out, silently + * ending the winner's exclusion too. + * + * {@link withSecretFileLock} therefore guards every removal the library makes + * on its behalf — see `guardedFs`, which sits in `options.fs` so it covers + * the release path *and* the `signal-exit` handler. It refuses to delete a + * directory that is no longer the one we created, and surfaces the compromise + * in the fast-mutation case the tick misses. + * + * **That narrows the destructive window; it does not close it.** The guard is + * a `statSync` immediately followed by an `rmdirSync`, so nothing in this + * process can interleave — but it is still check-then-act against other + * processes, and closing that needs the same compare-and-swap Node does not + * expose. It also rests on inode and birth-time identity, which some + * filesystems do not report. Best-effort throughout: it makes the destructive + * case rare, not impossible. + * + * The window is at least narrow and conditional: it opens only after a + * holder *dies without releasing*, since nothing else lets a lock go stale. + * + * **Which is why the optimistic verify in {@link FileSecretStore.mutate} + * stays, and is not belt-and-braces.** It is what still catches a clobber + * inside that window. It also covers what no lock can: an advisory + * convention orders Inspector against Inspector and says nothing about an + * editor, a backup restore, or an Inspector old enough to predate this file + * — and it covers {@link withSecretFileLock} being unable to lock at all, + * see below. + * + * **Degrading rather than failing is deliberate.** The store's whole reason + * for existing is a box where the usual mechanism is unavailable (no + * keychain, #1848/#1905), so it must not acquire a *new* way to be + * unavailable. A directory that cannot hold a lock file — a read-only + * `$HOME`, a filesystem without `mkdir` semantics, a container mount owned + * by another uid — would otherwise turn every `set` into a hard failure on + * exactly the deployments this store was written for. So a lock that cannot + * be taken runs the body anyway, with the #1950 optimistic behaviour + * underneath it, and says so once. + */ + +import nodeFs, { statSync, rmdirSync } from "node:fs"; +import * as fs from "node:fs/promises"; +import * as path from "node:path"; +// CJS-only package. A default import is the shape that survives every +// bundler this repo runs core/ through (tsup for cli/tui, vite's SSR/node +// graph for the web runner); named imports off a CJS module depend on +// lexer detection that esbuild and rollup disagree about. +import properLockfile from "proper-lockfile"; +import { SecretStoreUnavailableError } from "./secret-store.js"; + +/** + * How long a lock may go untouched before another process may claim it. + * + * `proper-lockfile` refreshes the lock's mtime at `stale / 2` for as long as + * the holder is alive, so this is not "how long a mutation may take" — it is + * how long after a holder *dies* the file stays unwritable. 10s is the + * library's own default and the value npm ships with; a mutation is a read, + * an scrypt derivation and an atomic write, so the margin is enormous. + */ +const STALE_MS = 10_000; + +/** + * How long a waiter keeps trying before giving up. + * + * This schedule sums to roughly 15s, and the number that matters is that it + * is comfortably **longer than {@link STALE_MS}**. A waiter that gives up + * first would abandon the save while the lock still belonged to a process + * that had already died — the takeover that resolves it becomes possible + * only once the lock goes stale, so a budget under 10s would turn a crashed + * Inspector into a failed save for every other one on the box. + * + * An uncontended acquire is one `mkdir` and pays none of this; the first + * retries are tens of milliseconds, so ordinary contention (a mutation is a + * read, an scrypt derivation and an atomic write) resolves imperceptibly. + */ +const RETRY = { + retries: 20, + factor: 2, + minTimeout: 20, + maxTimeout: 1_000, +} as const; + +/** + * What {@link RETRY} actually sums to, in milliseconds. + * + * Computed rather than written down. `retries * maxTimeout` overstates it by + * a third — the early attempts are the exponential ramp, not the cap — and a + * hand-maintained constant is one edit away from disagreeing with the + * schedule it describes, in a message whose whole job is to tell a user how + * long the Inspector waited. `retry` applies no jitter by default + * (`randomize` is off), so this is exact rather than an estimate. + * + * It must stay **above {@link STALE_MS}**: see {@link RETRY}. + */ +const RETRY_BUDGET_MS = ((): number => { + let total = 0; + let delay = RETRY.minTimeout; + for (let i = 0; i < RETRY.retries; i++) { + total += Math.min(delay, RETRY.maxTimeout); + delay *= RETRY.factor; + } + return total; +})(); + +/** Emitted once per process, not once per call — see {@link warnOnce}. */ +const warned = new Set(); + +/** + * Say why locking is unavailable here, once per reason per process. + * + * Once per *reason* rather than once overall: "the directory is read-only" + * and "the lock is held by something that never releases it" are different + * problems with different fixes, and collapsing them would print whichever + * happened first and hide the other for the life of the process. Keyed on + * the message, which already encodes the reason. + */ +function warnOnce(message: string): void { + if (warned.has(message)) return; + warned.add(message); + console.warn(`[mcp-inspector] ${message}`); +} + +/** Test seam: forget which warnings have been emitted. */ +export function resetFileLockWarnings(): void { + warned.clear(); +} + +/** + * Is this failure "the lock is there and we could not have it", as opposed to + * "locks do not work here"? + * + * `ELOCKED` is the obvious member, but not the only one, and the difference + * decides whether a save **refuses** or **degrades to an unlocked write** — + * so getting it wrong is not cosmetic. `acquireLock` does not only *create* + * directories: on finding a stale one it removes it and retries, and that + * removal can fail — `ENOTEMPTY` for a lock with anything inside it, `EACCES` + * or `EROFS` for one we may not touch. Those surface as ordinary non-`ELOCKED` + * errors, and treating them as infrastructure failures meant every Inspector + * on the box quietly bypassed the *same* stuck lock and raced its writes — + * while the release-failure message was telling the operator saves would keep + * failing until they cleared it. + * + * The discriminator is the lock directory itself rather than an errno + * taxonomy: **if it exists, something holds it and we must not proceed**; if + * it does not, we genuinely could not create one and degrading is the + * documented trade (#1848, #1905). That reads the state the decision is + * actually about, instead of enumerating error codes per platform and + * filesystem — which is the enumeration that let `ENOTEMPTY` through. + */ +function isStuckOrHeld(err: unknown, lockPath: string): boolean { + return isHeldElsewhere(err) || identifySync(lockPath) !== null; +} + +/** + * Did `proper-lockfile` decline because someone else holds the lock? + * + * A bare cast rather than a guarded narrowing: the only caller is the `catch` + * around `properLockfile.lock`, and the library rejects with a real `Error` + * carrying a `code` on every path. Guarding would add branches nothing can + * exercise, which is a worse trade than an assertion whose one caller is two + * lines away. + */ +const isHeldElsewhere = (err: unknown): boolean => + (err as NodeJS.ErrnoException).code === "ELOCKED"; + +/** + * The message from whatever was thrown. + * + * Same reasoning as above for the non-`Error` arm — `proper-lockfile` does not + * reject with one — except that this is used where a thrown non-`Error` would + * otherwise be reported as `[object Object]`, so the fallback earns its place + * even though nothing can provoke it. + */ +/* v8 ignore next 2 -- @preserve: the non-Error arm is unreachable via + proper-lockfile, which rejects only with Errors. */ +const describeError = (err: unknown): string => + err instanceof Error ? err.message : String(err); + +/** Where `proper-lockfile` puts the lock for `target` — its documented default. */ +const lockPathOf = (target: string): string => `${target}.lock`; + +/** A lock directory's identity: what changes on delete-and-recreate. */ +interface LockIdentity { + ino: number; + birthtimeMs: number; +} + +/** + * Identify a lock directory, or `null` if it cannot be read. + * + * `ino` and `birthtimeMs` together, and **not the mtime** the library + * compares: a lock held longer than one refresh tick has its mtime rewritten + * by `utimes` legitimately, so an mtime comparison would call our own healthy + * lock compromised. Both of these survive `utimes` and both change when a + * directory is removed and recreated, which is the event to detect. + * + * Not every filesystem reports either (Windows shares, some network mounts, + * older kernels report `0`). There the comparison trivially succeeds and the + * guard below concludes the lock is ours — the behaviour without the guard at + * all, which is the right way to fail: best-effort, never a false alarm on a + * healthy lock. + */ +function identifySync(lockPath: string): LockIdentity | null { + 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 we created. + * + * **Why this sits in `options.fs` rather than in a check before `release()`.** + * Every deletion the library performs on our behalf goes through this object + * — the `release()` path (`removeLock` → `fs.rmdir`) *and* its `signal-exit` + * handler (`rmdirSync` over every registered lock, with no ownership check of + * its own). A check placed before `release()` covers only the first, and + * leaves the second free to delete the winner's lock if the process exits at + * the wrong moment. Guarding at the single point where a directory is + * actually removed covers both, and there is nowhere narrower to put it. + * + * **It narrows the window; it does not close it.** The guard is + * `statSync` immediately followed by `rmdirSync`, with no `await` between + * them — so nothing else *in this process* can interleave, and the gap is as + * small as this platform allows. It is still a check-then-act against other + * processes, and closing that needs compare-and-swap on a directory entry + * (`renameat2(RENAME_EXCHANGE)`), which is exactly what Node does not expose + * and what the whole stale-takeover problem reduces to. Treat this as making + * the destructive case rare, not impossible. + * + * `owned.id` stays `null` until we have acquired, which is deliberate: during + * `acquireLock` the library removes *another* holder's stale directory + * through this same seam, and that removal must go through untouched. + */ +function guardedFs( + lockPath: string, + owned: { id: LockIdentity | null }, + onRefused: () => void, +): unknown { + const mine = (): boolean => { + if (owned.id === null) return true; // Not ours yet — see above. + const now = identifySync(lockPath); + if (now === null) return false; // Already gone; nothing of ours to remove. + return now.ino === owned.id.ino && now.birthtimeMs === owned.id.birthtimeMs; + }; + const removeIfMine = (): void => { + if (!mine()) { + onRefused(); + return; + } + rmdirSync(lockPath); + }; + return { + ...nodeFs, + // **Identity is captured here, not after `lock()` resolves.** `mkdir` is + // the moment the directory becomes ours, and it is synchronous with + // respect to this process: recording it in the callback, before yielding, + // leaves no window. Capturing after the acquire promise settled — as this + // did originally — spans the library's own `utimes`/`stat` probe, and a + // waiter replacing our directory inside that span would have us record + // *the winner's* identity as our own, after which the guard below would + // cheerfully delete their lock. + mkdir: ( + p: string, + cb: (err: NodeJS.ErrnoException | null) => void, + ): void => { + nodeFs.mkdir(p, (err) => { + if (!err) owned.id = identifySync(lockPath); + cb(err); + }); + }, + // Reported as success when refused: the library's bookkeeping should + // forget this lock either way. Leaving it registered would hand the + // `signal-exit` handler a record pointing at the winner's directory. + rmdir: (_p: string, cb: (err: NodeJS.ErrnoException | null) => void) => { + try { + removeIfMine(); + cb(null); + } catch (err) { + cb(err as NodeJS.ErrnoException); + } + }, + /* v8 ignore next 3 -- @preserve: only reachable from proper-lockfile's + signal-exit handler, i.e. at real process exit, which no in-process + test can drive. Its logic is `removeIfMine`, covered via `rmdir`. */ + rmdirSync: () => { + removeIfMine(); + }, + }; +} + +/** + * One `proper-lockfile` acquire against `target`, with the given retry policy. + * + * `realpath: false` is load-bearing: by default the library resolves the + * target through `fs.realpath`, which fails `ENOENT` on a secrets file that + * does not exist yet — i.e. on the very first `set`, the one call that has + * nothing to fall back on. Resolving the path lexically instead lets a file + * be locked into existence. The cost is that two paths reaching one file + * through different symlinks take different locks; the store resolves its + * path once at construction and every caller goes through it, so that is a + * shape this codebase does not produce. + */ +function acquire( + target: string, + retries: number | typeof RETRY, + fsShim: unknown, +): Promise<() => Promise> { + return properLockfile.lock(target, { + realpath: false, + stale: STALE_MS, + retries, + // Every directory removal the library performs — on release and from its + // exit handler — routes through here. See `guardedFs`. + fs: fsShim, + // The library's default `onCompromised` *throws* — from a timer, with no + // caller on the stack, so it lands as an uncaught exception and takes the + // process down. This is also the library's *only* signal for the + // stale-takeover race described at the top of this file — someone + // declared our lock stale and replaced it — so it is the one place a user + // learns the guarantee was lost. Worth saying; not worth killing an + // Inspector session over. + onCompromised: (err) => + warnOnce( + `The lock on the secrets file at ${target} was taken over by another process while a write was in progress (${err.message}). If a secret you just saved is missing, save it again.`, + ), + }); +} + +/** + * Is someone holding the lock on `filePath` right now? + * + * Liveness, not ownership: `check` reports a *stale* lock as unheld, so a + * holder that died stops counting on its own after {@link STALE_MS} with no + * bookkeeping to clean up. That is what makes this usable as an + * "is this in progress" test — a pid stamp cannot say it, since a pid both + * outlives its process and recurs (pid 1 on every container start). + * + * Answers `false` when it cannot tell. The callers use this to decide whether + * to *leave something alone*, and the alternative to a wrong `false` is + * refusing to ever recover an abandoned file. + */ +export async function isFileLockHeld(filePath: string): Promise { + try { + return await properLockfile.check(path.resolve(filePath), { + realpath: false, + stale: STALE_MS, + }); + } catch { + return false; + } +} + +/** + * Run `fn` holding an exclusive cross-process lock on `filePath`. + * + * The lock is `.lock`, a directory beside the secrets file rather + * than inside it — `proper-lockfile` never opens or truncates the file it + * guards, so a lock that outlives its holder can only ever block a write, + * never damage one. + * + * Returns whatever `fn` returns. `fn` runs exactly once either way — the + * lock's absence changes the guarantee, never whether the work happens. + */ +/** + * Take the lock and hand back its release, or `null` when locking is + * unavailable here and the caller should proceed unprotected. + * + * Split out of {@link withSecretFileLock} so a caller that must hold a lock + * **across** another lock's release can do so — the keychain hand-off needs + * exactly that, see `absorbFileSecretsIntoKeyring`. Everything about *which* + * failures refuse and which degrade lives here, so both entry points cannot + * drift on that question. + * + * Throws {@link SecretStoreUnavailableError} when the lock is held or stuck; + * returns `null` when it could not be created at all. + */ +export async function openSecretFileLock( + filePath: string, +): Promise<(() => Promise) | null> { + const target = path.resolve(filePath); + // Create the parent directory before locking, not after. `writeStoreFile` + // creates it on the way to writing the secrets file, but that runs *inside* + // the locked section — so on a fresh install, where `~/.mcp-inspector` does + // not exist yet, `proper-lockfile` would fail `ENOENT` and every first save + // would degrade to an unlocked write with a warning. That is the one save + // most likely to be racing another: two Inspectors started together both + // reach it, and it is exactly the interleaving this lock exists to close. + // + // Failure is deliberately swallowed rather than reported here. A directory + // that cannot be created is the same condition as a lock that cannot be + // taken, and the catch below already says so with the right message — one + // that mentions the lock rather than a `mkdir` the caller never asked for. + await fs.mkdir(path.dirname(target), { recursive: true }).catch(() => {}); + // Filled in once we actually hold the lock; see `guardedFs`. + const owned: { id: LockIdentity | null } = { id: null }; + const fsShim = guardedFs(lockPathOf(target), owned, () => + warnOnce( + `The lock on the secrets file at ${target} was taken over by another process while a write was in progress, so it was left alone rather than removed. If a secret you just saved is missing, save it again.`, + ), + ); + let release: () => Promise; + try { + release = await acquire(target, 0, fsShim).catch((err: unknown) => { + // **Retries are for contention, and only for contention.** + // `proper-lockfile` drives its whole acquire through `retry`, which + // re-attempts on *any* error — so a read-only `$HOME` would spend the + // full ~15s budget re-issuing an `mkdir` that fails identically every + // time, on every save, before degrading. Probing once with no retries + // separates the two answers at the cost of one syscall: `ELOCKED` is + // worth waiting out, an infrastructure failure is not. + if (!isStuckOrHeld(err, lockPathOf(target))) throw err; + return acquire(target, RETRY, fsShim); + }); + } catch (err) { + // **`ELOCKED` is not a reason to degrade — it is the opposite.** It means + // the lock is working and something else demonstrably holds it right now, + // so running the body anyway would write alongside a *known* concurrent + // writer: the precise interleaving this exists to prevent, entered + // deliberately. Having already waited past the stale window (see + // {@link RETRY}), a holder still there is not one that crashed; it is one + // that is stuck. Refusing loses nothing — `set` reports it and the user + // retries — whereas proceeding can lose a secret while reporting success. + if (isStuckOrHeld(err, lockPathOf(target))) { + throw new SecretStoreUnavailableError( + `Could not save to the secrets file at ${target}: its lock (${lockPathOf(target)}) was still held after the ${Math.round(RETRY_BUDGET_MS / 1000)} seconds this save waited. Its secrets are intact; the value you just entered was not saved. If no other Inspector is running, remove that lock and try again.`, + ); + } + // Everything else is the lock being *unavailable* rather than held — a + // read-only `$HOME`, a mount owned by another uid, a filesystem without + // `mkdir` semantics. There the choice is between degrading and refusing + // every save on a box that has no other way to keep a secret, and this + // store exists for exactly those boxes (#1848, #1905). + warnOnce( + `Could not take a lock on the secrets file at ${target} (${describeError(err)}), so writes to it are not protected against another process writing at the same moment.`, + ); + return null; + } + return async () => { + try { + await release(); + } catch (err) { + // The body already ran and its result is being returned; turning a + // completed save into a failure at teardown would be the wrong trade. + // But a lock left behind is worth explaining. + // + // **`ERELEASED` is not a lock left behind.** When the library's refresh + // tick detects a takeover it marks the lock released and drops its + // registry entry *before* calling `onCompromised`, so this returns + // `ERELEASED` without touching the filesystem. The directory sitting + // there is then the **winner's live lock** — and the message below + // would tell the operator to delete it, destroying the exclusion of a + // process that did nothing wrong. `onCompromised` already said what + // happened, so there is nothing to add. + if ((err as NodeJS.ErrnoException).code === "ERELEASED") return; + warnOnce( + // **Conditional, not an instruction.** Two blanket versions of this + // have now been wrong in opposite directions: "it expires on its own" + // (false whenever the same `rmdir` that blocked us also blocks stale + // takeover — `ENOTEMPTY`, `EACCES`, `EPERM`, `EROFS`), and "remove it + // by hand" (false whenever the failure was transient, since the path + // may by then hold a *different, live* Inspector's lock, and deleting + // that destroys the exclusion of a process that did nothing wrong). + // + // `proper-lockfile` forwards whatever the filesystem returned, and + // nothing here can tell a permanent refusal from a passing one. So + // this reports what happened and states the two conditions the + // operator can check for themselves, rather than asserting an outcome + // this code does not know. + `Could not release the lock on the secrets file at ${target} (${describeError(err)}). It may clear on its own — stale takeover reclaims a lock through the same directory removal, so it will not if whatever blocked this persists. If saves keep failing against this file and no other Inspector is running, remove ${lockPathOf(target)} by hand.`, + ); + } + }; +} + +export async function withSecretFileLock( + filePath: string, + fn: () => Promise, +): Promise { + const release = await openSecretFileLock(filePath); + if (release === null) return fn(); + try { + return await fn(); + } finally { + await release(); + } +} diff --git a/core/auth/node/file-secret-store.ts b/core/auth/node/file-secret-store.ts index edff72c1..4d812751 100644 --- a/core/auth/node/file-secret-store.ts +++ b/core/auth/node/file-secret-store.ts @@ -62,6 +62,7 @@ import * as crypto from "node:crypto"; import * as fs from "node:fs/promises"; import * as path from "node:path"; import { readStoreFile, writeStoreFile } from "../../storage/store-io.js"; +import { withSecretFileLock } from "./file-lock.js"; import { SecretStoreUnavailableError, type SecretBulkRequest, @@ -597,30 +598,33 @@ export class FileSecretStore implements SecretStore { } /** - * Run `fn` with the file to itself across *processes* as well. + * Apply a mutation to the file, under a cross-process lock, and confirm it + * survived. * - * The in-process queue is necessary and not sufficient: a durable secrets - * file is shared state, and a second Inspector on the same box — a CLI run - * beside a web session is the ordinary case, not a contrived one — reads - * the same "before" map and then atomically replaces the file, dropping - * whatever the first process had just added. Both writes report success; - * one secret is simply gone. + * **Two mechanisms, and they answer different questions** — this is not + * belt-and-braces. * - * Apply a mutation to the file, and confirm it survived. + * The lock ({@link withSecretFileLock}, #2082) is the one that provides + * mutual exclusion: a durable secrets file is shared state, and a second + * Inspector on the same box — a CLI run beside a web session is the + * ordinary case, not a contrived one — otherwise reads the same "before" + * map and then atomically replaces the file, dropping whatever the first + * process had just added. Both writes report success; one secret is simply + * gone. Held across the read *and* the write, so no other holder can + * observe or replace the map in between. * - * **There is no cross-process lock.** There was one — a `mkdir` election - * with an owner stamp, a heartbeat and a stale-takeover — and three - * consecutive review rounds found a real race in it. The last one is - * unfixable with what Node exposes: claiming a stale lock needs - * compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`), - * and without it a waiter that loses the race can still move the winner's - * *fresh* lock aside and enter alongside it. + * The verify covers what a lock structurally cannot. A lock is an advisory + * convention between the processes that take it, so it says nothing about + * a writer that does not: an editor, a restored backup, a `jq` one-liner, + * or an Inspector predating #2082. It is also the fallback when the lock + * is unavailable at all — `withSecretFileLock` runs the body anyway on a + * read-only or lock-hostile directory rather than failing a `set` on + * exactly the deployments this store exists for (#1848, #1905). * - * So this does the opposite: it lets writers collide and makes the loser - * notice. Read `M0`, apply the mutation to get `M1`, write it, then read - * back `M2`. If `M2` equals `M1` nothing interleaved. If it does not, - * someone wrote between our write and our read — so re-apply onto what - * they left and try again. + * So: read `M0`, apply the mutation to get `M1`, write it, then read back + * `M2`. If `M2` equals `M1` nothing interleaved. If it does not, someone + * wrote between our write and our read — so re-apply onto what they left + * and try again. * * The comparison is over the **whole map**, not just the entry we touched. * Checking only our own key would pass in precisely the case that loses @@ -629,32 +633,29 @@ export class FileSecretStore implements SecretStore { * repairs it — A writes `MA`, B writes `MB` over it, B verifies `MB` * correctly, A verifies, sees `M2 !== MA`, re-applies onto `MB`. * - * **This is not mutual exclusion, and the gap is wider than a crash.** - * The verify only catches a clobber that has already landed. Order the - * same two writers as write-A, verify-A, write-B, verify-B and both - * succeed while A's entry is gone: A's verify ran before B's write, so - * there was nothing yet to see, and B did nothing wrong. Nothing detects - * it afterwards. A crash between write and verify is one instance of the - * same shape, not the whole of it — an earlier version of this comment - * said otherwise, which understated it. - * - * What that buys, stated without overselling: two processes must write the - * same file within the window between one's write and its read-back, and - * the loss is one secret that reported success. The lock this replaced - * lost updates in a *wider* set of interleavings, with every participant - * alive, and could not be closed without a primitive Node does not expose - * (`renameat2(RENAME_EXCHANGE)`); this costs ~220 fewer lines and converges - * in every interleaving where the clobber lands before the verify. If the - * residual matters for a deployment, the answer is a real lock — an - * OS-backed one from a dedicated library — not another hand-rolled - * election. + * **What is left, stated without overselling.** Against an unlocked writer + * the verify is not mutual exclusion: it only catches a clobber that has + * already landed, so ordering the two as write-A, verify-A, write-B, + * verify-B loses A's entry with both reporting success. That residual is + * now confined to a writer outside this codebase, or to a directory where + * no lock could be taken — and the latter announces itself, once, on the + * console. * * Reads deliberately do not participate: `writeStoreFile` is atomic * (write-temp-then-rename), so a reader sees either the old file or the - * new one, never a torn one. + * new one, never a torn one. Taking the lock for them would serialize + * every `GET /api/servers` behind whatever else holds it, to prevent + * nothing. */ private async mutate( apply: (map: Record) => Record | null, + ): Promise { + await withSecretFileLock(this.filePath, () => this.mutateLocked(apply)); + } + + /** {@link mutate}'s body, with the lock already held. */ + private async mutateLocked( + apply: (map: Record) => Record | null, ): Promise { for (let attempt = 0; attempt < MAX_WRITE_ATTEMPTS; attempt++) { let current: Record; @@ -713,9 +714,15 @@ export class FileSecretStore implements SecretStore { * Measured, not theorised — two instances racing `set` lost an entry on * the first run of the convergence test below. * - * So the in-process case is made correct by construction here, and the - * verify-and-retry in {@link mutate} covers what this cannot see: a + * So the in-process case is made correct by construction here, and + * {@link mutate}'s cross-process lock covers what this cannot see: a * second Inspector process. + * + * The queue is *also* what keeps that lock usable. `proper-lockfile` is + * not reentrant — a second `lock()` on a path this process already holds + * fails `ELOCKED`, which is indistinguishable from a genuine remote + * holder. Serializing here means the lock is only ever contended between + * processes, which is the only contention it is asked to arbitrate. */ private serialize(fn: () => Promise): Promise { const key = path.resolve(this.filePath); diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index cb468af9..3a55ec1f 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -54,6 +54,11 @@ import { readSecretFilePermissions, tightenSecretFilePermissions, } from "./file-secret-store.js"; +import { + isFileLockHeld, + openSecretFileLock, + withSecretFileLock, +} from "./file-lock.js"; import { KeyringSecretStore, parseAccount, @@ -532,52 +537,130 @@ export async function absorbFileSecretsIntoKeyring( ): Promise { const filePath = defaultSecretFilePath(); - // A crash between the claim and the delete leaves only - // `secrets.json.migrating-`. Checking the canonical path alone then - // reports "nothing to migrate", the keychain is selected, and every stored - // credential silently disappears — the claim protecting the delete having - // introduced a way to lose everything. Adopt any orphan first. - await recoverOrphanedSnapshots(filePath); - - if (!fsSync.existsSync(filePath)) return; - - // **Claim the file atomically before reading it.** Comparing its contents - // immediately before `rm` narrowed the window and could not close it: a - // writer that completes a `set` after the comparison and before the delete - // has its write verified, reports success, and then loses it — a *later* - // successful write destroyed, which is worse than the optimistic-write - // residual and, unlike that one, fixable here. + // Cheap, lock-free "is there anything at all to do". Taking the lock first + // would mean every startup on the overwhelmingly common path — a keychain + // is available and no file was ever written — creates and removes a lock + // directory, and on a box whose storage dir does not exist yet the lock + // cannot be created at all, so `withSecretFileLock` would warn about + // unprotected writes on every single run with nothing to protect. // - // `rename` is atomic and leaves the live path free. Everything after this - // point operates on a snapshot nobody else can reach, and a writer that - // recreates `secrets.json` in the meantime is simply untouched — its file - // is a different one, and the next run migrates it. + // Racy by construction, and that is fine: it can only be wrong by saying + // "nothing here" about a file created a moment later, which is a file the + // next run migrates — the same outcome as a writer that recreates the path + // after the claim below. Everything that *acts* re-checks under the lock. + if (!(await anythingToMigrate(filePath))) return; + + // Orphan adoption and the claim below run under the same cross-process lock + // a `set` takes (#2082) — otherwise a concurrent writer's atomic rename can + // land between the two and be adopted, claimed, or clobbered depending on + // the interleaving. // - // The staged name carries the pid so two Inspectors starting together - // cannot claim the same destination; whichever wins the rename does the - // migration and the loser sees ENOENT and returns. - // A per-attempt nonce, not just the pid. `recoverOrphanedSnapshots` - // deliberately leaves an orphan in place when a live `secrets.json` also - // exists — and a pid-only name is reusable across restarts (pid 1 on every - // container start), so the next claim would `rename` straight over that - // orphan and permanently discard secrets it may uniquely hold. - const staged = `${filePath}.migrating-${process.pid}-${randomUUID()}`; - try { - await fs.rename(filePath, staged); - } catch (err) { - const code = (err as NodeJS.ErrnoException).code; - if (code !== "ENOENT") { - // Only ENOENT means someone else claimed it. Anything else — EACCES on - // the directory, EROFS — leaves the file exactly where it is, and - // returning quietly would select the keychain while file-backed - // secrets sit there unreadable by anything, with nothing said. - console.warn( - `\n[mcp-inspector] Could not claim the secrets file at ${filePath} for migration into the OS keychain (${code ?? "unknown error"}), so it has been left in place. Its secrets are not visible to this session.`, - ); + // The hand-off itself deliberately runs **outside** it, under a lock on the + // *snapshot* instead (see `handOffStagedSecrets`). Holding the main lock + // across a keychain round-trip per secret blocks every ordinary writer for + // the duration, and past the retry budget fails their save outright — which + // is a worse regression than the race it would close, since #1950 + // guarantees a write completing after the claim survives. + // + const claimed = await withSecretFileLock(filePath, async () => { + // A crash between the claim and the delete leaves only + // `secrets.json.migrating-`. Checking the canonical path alone then + // reports "nothing to migrate", the keychain is selected, and every stored + // credential silently disappears — the claim protecting the delete having + // introduced a way to lose everything. Adopt any orphan first. + await recoverOrphanedSnapshots(filePath); + + if (!fsSync.existsSync(filePath)) return null; + + // **Claim the file atomically before reading it.** Comparing its contents + // immediately before `rm` narrowed the window and could not close it: a + // writer that completes a `set` after the comparison and before the delete + // has its write verified, reports success, and then loses it — a *later* + // successful write destroyed, which is worse than the optimistic-write + // residual and, unlike that one, fixable here. + // + // `rename` is atomic and leaves the live path free. Everything after this + // point operates on a snapshot nobody else can reach, and a writer that + // recreates `secrets.json` in the meantime is simply untouched — its file + // is a different one, and the next run migrates it. + // + // The staged name carries the pid so two Inspectors starting together + // cannot claim the same destination; whichever wins the rename does the + // migration and the loser sees ENOENT and returns. + // A per-attempt nonce, not just the pid. `recoverOrphanedSnapshots` + // deliberately leaves an orphan in place when a live `secrets.json` also + // exists — and a pid-only name is reusable across restarts (pid 1 on every + // container start), so the next claim would `rename` straight over that + // orphan and permanently discard secrets it may uniquely hold. + const staged = `${filePath}.migrating-${process.pid}-${randomUUID()}`; + try { + await fs.rename(filePath, staged); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + if (code !== "ENOENT") { + // Only ENOENT means someone else claimed it. Anything else — EACCES on + // the directory, EROFS — leaves the file exactly where it is, and + // returning quietly would select the keychain while file-backed + // secrets sit there unreadable by anything, with nothing said. + console.warn( + `\n[mcp-inspector] Could not claim the secrets file at ${filePath} for migration into the OS keychain (${code ?? "unknown error"}), so it has been left in place. Its secrets are not visible to this session.`, + ); + } + return null; } - return; + // **Acquire the snapshot's lock before returning**, i.e. before the main + // lock is released. Taking it afterwards leaves a gap in which a second + // startup can take the main lock, see the staged file unlocked, and adopt + // and re-claim it — leaving this process reading a path that no longer + // exists, which is the very race the snapshot lock exists to prevent. + // + // `null` means locking is unavailable here; the hand-off still runs. The + // exposure is one unique nonce-named snapshot, which is a far better + // trade than refusing to migrate at all on a box that cannot lock. + const releaseSnapshot = await openSecretFileLock(staged).catch(() => null); + return { staged, releaseSnapshot }; + // `withSecretFileLock` rejects only with the `SecretStoreUnavailableError` + // it constructs itself (a lock it could not create degrades instead of + // throwing), so the cast is over a value produced one call away rather + // than an assumption about arbitrary throwables. + }).catch((err: Error) => { + console.warn( + `\n[mcp-inspector] Could not lock the secrets file at ${filePath} to migrate it into the OS keychain (${err.message}), so it has been left in place. Its secrets are not visible to this session; the next run will try again.`, + ); + return null; + }); + if (claimed === null) return; + // Outside the main lock, still holding the snapshot's: a keychain + // round-trip per secret must not block ordinary writers (#1950 guarantees a + // write completing after the claim survives). + try { + await handOffStagedSecrets(claimed.staged, filePath, keyring); + } finally { + await claimed.releaseSnapshot?.(); } +} +/** + * Move a claimed snapshot's secrets into the keychain, then dispose of it. + * + * Runs under a lock on the **snapshot**, not on `secrets.json`. That is what + * stops a second Inspector's {@link recoverOrphanedSnapshots} from adopting a + * migration still in progress — it link/unlinks the snapshot back to the live + * path and re-claims it, our hand-off then fails `ENOENT`, and if that second + * process exits before copying, the healthy first session starts with none of + * those secrets. Locking the snapshot says "in progress" in a way a filename + * cannot, and expires by itself if this process dies. + * + * Holding the *main* lock here instead would also close it, and was tried: + * it blocks every ordinary writer for the whole migration and fails their + * save past the retry budget, breaking #1950's guarantee that a write + * completing after the claim survives. + */ +async function handOffStagedSecrets( + staged: string, + filePath: string, + keyring: SecretStore, +): Promise { const file = new FileSecretStore({ filePath: staged }); // True only when every value reached the keychain *and* there was // something to move — the one case where the snapshot is redundant. @@ -625,6 +708,52 @@ export async function absorbFileSecretsIntoKeyring( } } +/** + * Is `name` a migration snapshot, as opposed to the lock directory beside one? + * + * The `.lock` exclusion is load-bearing, not tidiness. `secrets.json.lock` + * and `secrets.json.migrating--.lock` both match the plain prefix + * test, and treating the latter as a snapshot is self-sustaining damage: the + * liveness probe asks about a nonexistent `.lock.lock` and so answers + * "not held", recovery then tries to hard-link a *directory* onto the secrets + * path, fails, and prints the orphan warning. Because a stale lock directory + * is never removed by a liveness *check* — only a would-be acquirer clears + * one — that false migration repeats on every startup forever, including + * after the real snapshot has long since been recovered. + */ +const isSnapshotName = (name: string, base: string): boolean => + name.startsWith(`${base}.migrating-`) && !name.endsWith(".lock"); + +/** + * Is there a `secrets.json`, or a snapshot orphaned by an interrupted + * migration, worth taking the lock for? + * + * One `readdir` rather than a `stat` of the canonical path: an orphan is the + * case where the live file is *absent* and there is still everything to + * migrate, so checking only `secrets.json` would skip the recovery that + * exists because skipping it loses every stored credential. + */ +async function anythingToMigrate(filePath: string): Promise { + const base = path.basename(filePath); + try { + return (await fs.readdir(path.dirname(filePath))).some( + (name) => name === base || isSnapshotName(name, base), + ); + } catch (err) { + const code = (err as NodeJS.ErrnoException).code; + // Only "there is no directory" proves there is nothing to migrate — the + // first run on a fresh install, and the path this check exists to keep + // quiet. Every other failure (EACCES, EPERM, EMFILE) means the directory + // is there and could not be *listed*, which is not the same as empty: a + // directory can deny listing while still permitting access to the known + // `secrets.json` path, so returning false would select the keychain and + // leave those secrets invisible with nothing said. Fall through instead + // and let the under-lock `existsSync` and claim decide, which is what + // happened before this fast path existed. + return code !== "ENOENT" && code !== "ENOTDIR"; + } +} + /** * Adopt a snapshot left behind by a process that died mid-migration. * @@ -635,15 +764,23 @@ export async function absorbFileSecretsIntoKeyring( */ async function recoverOrphanedSnapshots(filePath: string): Promise { const dir = path.dirname(filePath); - const prefix = `${path.basename(filePath)}.migrating-`; + const base = path.basename(filePath); let names: string[]; try { - names = (await fs.readdir(dir)).filter((n) => n.startsWith(prefix)); + names = (await fs.readdir(dir)).filter((n) => isSnapshotName(n, base)); } catch { return; // No directory yet, or unreadable — nothing to recover. } for (const name of names) { const orphan = path.join(dir, name); + // Not an orphan at all — another Inspector is migrating it right now, and + // adopting it would link it back to the live path and re-claim it under a + // new name while its owner is still reading it. That owner's hand-off + // then fails `ENOENT`, and if we exit before copying, its healthy session + // starts with none of those secrets. The lock expires on its own if that + // process dies, so a genuinely abandoned snapshot becomes adoptable + // without anything to clean up. + if (await isFileLockHeld(orphan)) continue; try { await fs.link(orphan, filePath); await fs.rm(orphan, { force: true }); diff --git a/package-lock.json b/package-lock.json index d934e96e..d2950988 100644 --- a/package-lock.json +++ b/package-lock.json @@ -26,6 +26,7 @@ "ink": "^6.0.0", "open": "^10.2.0", "pino": "^9.14.0", + "proper-lockfile": "^4.1.2", "react": "^19.0.0", "undici": "^8.5.0", "vite": "^8.1.5", @@ -37,6 +38,7 @@ }, "devDependencies": { "@eslint/js": "^10.0.1", + "@types/proper-lockfile": "^4.1.4", "eslint": "^10.8.0", "express": "^5.2.1", "globals": "^17.7.0", @@ -993,6 +995,23 @@ "dev": true, "license": "MIT" }, + "node_modules/@types/proper-lockfile": { + "version": "4.1.4", + "resolved": "https://registry.npmjs.org/@types/proper-lockfile/-/proper-lockfile-4.1.4.tgz", + "integrity": "sha512-uo2ABllncSqg9F1D4nugVl9v93RmjxF6LJzQLMLDdPaXCUIDPeOJ21Gbqi43xNKzBi/WQ0Q0dICqufzQbMjipQ==", + "dev": true, + "license": "MIT", + "dependencies": { + "@types/retry": "*" + } + }, + "node_modules/@types/retry": { + "version": "0.12.5", + "resolved": "https://registry.npmjs.org/@types/retry/-/retry-0.12.5.tgz", + "integrity": "sha512-3xSjTp3v03X/lSQLkczaN9UIEwJMoMCA1+Nb5HfbJEQWogdeQIyVtTvxPXDQjZ5zws8rFQfVfRdz03ARihPJgw==", + "dev": true, + "license": "MIT" + }, "node_modules/@typescript-eslint/eslint-plugin": { "version": "8.65.0", "resolved": "https://registry.npmjs.org/@typescript-eslint/eslint-plugin/-/eslint-plugin-8.65.0.tgz", @@ -2394,6 +2413,12 @@ "url": "https://github.com/sponsors/ljharb" } }, + "node_modules/graceful-fs": { + "version": "4.2.11", + "resolved": "https://registry.npmjs.org/graceful-fs/-/graceful-fs-4.2.11.tgz", + "integrity": "sha512-RbJ5/jmFcNNCcDV5o9eTnBLJ/HszWV0P73bc+Ff4nS/rJj+YaS6IGyiOL0VoBYX+l1Wrl3k63h/KrH+nhJ0XvQ==", + "license": "ISC" + }, "node_modules/has-symbols": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/has-symbols/-/has-symbols-1.1.0.tgz", @@ -3453,6 +3478,17 @@ ], "license": "MIT" }, + "node_modules/proper-lockfile": { + "version": "4.1.2", + "resolved": "https://registry.npmjs.org/proper-lockfile/-/proper-lockfile-4.1.2.tgz", + "integrity": "sha512-TjNPblN4BwAWMXU8s9AEz4JmQxnD1NNL7bNOY/AKUzyamc379FWASUhc/K1pL2noVb+XmZKLL68cjzLsiOAMaA==", + "license": "MIT", + "dependencies": { + "graceful-fs": "^4.2.4", + "retry": "^0.12.0", + "signal-exit": "^3.0.2" + } + }, "node_modules/proxy-addr": { "version": "2.0.7", "resolved": "https://registry.npmjs.org/proxy-addr/-/proxy-addr-2.0.7.tgz", @@ -3593,6 +3629,15 @@ "url": "https://github.com/sponsors/sindresorhus" } }, + "node_modules/retry": { + "version": "0.12.0", + "resolved": "https://registry.npmjs.org/retry/-/retry-0.12.0.tgz", + "integrity": "sha512-9LkiTwjUh6rT555DtE9rTX+BKByPfrMzEAtnlEtdEwr3Nkffwiihqe2bWADg+OQRjt9gl6ICdmB/ZFDCGAtSow==", + "license": "MIT", + "engines": { + "node": ">= 4" + } + }, "node_modules/rolldown": { "version": "1.2.1", "resolved": "https://registry.npmjs.org/rolldown/-/rolldown-1.2.1.tgz", diff --git a/package.json b/package.json index 4545060a..99113852 100644 --- a/package.json +++ b/package.json @@ -94,6 +94,7 @@ "ink": "^6.0.0", "open": "^10.2.0", "pino": "^9.14.0", + "proper-lockfile": "^4.1.2", "react": "^19.0.0", "undici": "^8.5.0", "vite": "^8.1.5", @@ -105,6 +106,7 @@ }, "devDependencies": { "@eslint/js": "^10.0.1", + "@types/proper-lockfile": "^4.1.4", "eslint": "^10.8.0", "express": "^5.2.1", "globals": "^17.7.0", diff --git a/scripts/lib/announced-child.mjs b/scripts/lib/announced-child.mjs new file mode 100644 index 00000000..11303a19 --- /dev/null +++ b/scripts/lib/announced-child.mjs @@ -0,0 +1,94 @@ +/** + * Spawn a child process and wait for it to announce readiness on its output. + * + * Extracted from `scripts/smoke-web-app.mjs` so the failure path can be tested + * (#2000). A smoke script only ever exercises its own happy path: the real MCP + * test server always announces, so nothing in `npm run ci` proved that a child + * which stays alive *through* the readiness timeout is still reachable by the + * caller's teardown. That is precisely the case that used to orphan a live + * server holding its port, and it is invisible to the smoke itself. + * + * The ownership rule this encodes: the child is handed to `onSpawn` the moment + * it exists, **before** the readiness wait, so every throw path below leaves + * the caller holding a stoppable handle. Returning it only on success is what + * made the timeout leak — `spawnError` and `exited` were never affected (the + * child is already gone there), but they cost nothing to cover the same way. + * + * Both stdio channels are piped and scanned: a child that announces with + * `console.error` is missed entirely by a stdout-only scan, which then times + * out with an empty diagnostic. Piping both also keeps the child's noise out of + * the caller's output while still making it available in the failure message. + */ + +import { spawn } from "node:child_process"; +import { setTimeout as delay } from "node:timers/promises"; + +/** + * @param {object} opts + * @param {string} opts.command Executable to spawn. + * @param {string[]} opts.args Arguments for it. + * @param {string} [opts.cwd] Working directory. + * @param {RegExp} opts.pattern Matched against the accumulated output; + * the first match ends the wait. + * @param {(child: import("node:child_process").ChildProcess) => void} opts.onSpawn + * Called synchronously with the child immediately after spawn, before any + * waiting. This is the caller's teardown handle. + * @param {string} opts.what Noun used in error messages ("MCP test server"). + * @param {number} [opts.timeoutMs] Readiness budget (default 30s). + * @param {number} [opts.pollMs] Poll interval (default 250ms). + * @returns {Promise<{ child: import("node:child_process").ChildProcess, match: RegExpMatchArray }>} + */ +export async function startAnnouncedChild({ + command, + args, + cwd, + pattern, + onSpawn, + what, + timeoutMs = 30_000, + pollMs = 250, +}) { + const child = spawn(command, args, { + cwd, + stdio: ["ignore", "pipe", "pipe"], + }); + // Publish before waiting — this is the whole point of the helper. + onSpawn(child); + + let out = ""; + child.stdout.on("data", (d) => (out += d)); + child.stderr.on("data", (d) => (out += d)); + let exited = false; + let spawnError = null; + // A spawn failure (e.g. an unbuilt/renamed entry) emits `error`, NOT `exit` — + // and with no `error` listener Node throws it uncaught, replacing the caller's + // diagnostic with a raw stack. `close` is listened to alongside `exit` for the + // same reason: it fires in cases `exit` does not, so a child that dies without + // an exit event can't leave the poll below spinning for the full budget. + child.on("error", (err) => (spawnError = err)); + child.on("exit", () => (exited = true)); + child.on("close", () => (exited = true)); + + // A deadline loop, not a fixed attempt count: state is re-read *after* every + // wait, including the last one. Counting attempts and throwing straight after + // the final `delay` leaves the whole last polling interval unobserved, so an + // announcement landing at 29.9s of a 30s budget is reported as a timeout — + // and a spawn error or early exit in that window is misattributed the same + // way. The final wait is also clamped to the deadline so the budget is a + // real bound rather than one poll longer. + const deadline = Date.now() + timeoutMs; + for (;;) { + const match = out.match(pattern); + if (match) return { child, match }; + if (spawnError) { + throw new Error(`could not spawn the ${what}: ${spawnError.message}`); + } + if (exited) throw new Error(`${what} exited early:\n${out}`); + const remaining = deadline - Date.now(); + if (remaining <= 0) break; + await delay(Math.min(pollMs, remaining)); + } + throw new Error( + `${what} did not start within ${Math.round(timeoutMs / 1000)}s:\n${out}`, + ); +} diff --git a/scripts/lib/announced-child.test.mjs b/scripts/lib/announced-child.test.mjs new file mode 100644 index 00000000..e6404c80 --- /dev/null +++ b/scripts/lib/announced-child.test.mjs @@ -0,0 +1,163 @@ +/** + * Tests for the spawn/readiness ownership helper (#2000). + * + * The invariant under test is the one a smoke script can never check itself: + * a child that stays alive *through* the readiness timeout must still be + * reachable by the caller's teardown. `smoke:web:app`'s happy path always gets + * the announcement, so a regression here would be silent in `npm run ci` and + * would surface only as a stray process holding a port on a later run. + * + * Every case drives a real `node -e` child rather than a spy, so the assertion + * is about a real process being killable, not about a mock's call log. + */ + +import test from "node:test"; +import assert from "node:assert/strict"; +import { startAnnouncedChild } from "./announced-child.mjs"; + +/** Wait for a real child to exit (or resolve immediately if it already has). */ +function waitForExit(child) { + if (child.exitCode !== null || child.signalCode !== null) + return Promise.resolve(); + return new Promise((resolve) => child.once("close", resolve)); +} + +/** + * Build an `onSpawn` that registers teardown the moment the child exists, so it + * is killed on every path out of the test — a failed assertion included. + * + * This is the helper's own ownership rule applied to the suite: killing only on + * the success path would leak a 60-second child into the runner precisely when + * the regression being guarded has come back, which is the one run where a leak + * matters most. Killing an already-exited child is a no-op, so the early-exit + * cases pay nothing for it. + */ +function killOnTeardown(t, capture = () => {}) { + return (child) => { + capture(child); + t.after(async () => { + child.kill("SIGKILL"); + await waitForExit(child); + }); + }; +} + +test("hands the child to onSpawn before waiting, and returns the match", async (t) => { + let seen = null; + const { child, match } = await startAnnouncedChild({ + command: process.execPath, + args: ["-e", 'console.error("listening at http://127.0.0.1:9999")'], + pattern: /listening at (http:\/\/\S+)/i, + onSpawn: killOnTeardown(t, (c) => (seen = c)), + what: "probe", + timeoutMs: 10_000, + pollMs: 25, + }); + assert.equal(seen, child, "onSpawn got the child that was returned"); + assert.equal(match[1], "http://127.0.0.1:9999"); +}); + +test("scans stdout as well as stderr", async (t) => { + const { match } = await startAnnouncedChild({ + command: process.execPath, + args: ["-e", 'console.log("ready on 4242")'], + pattern: /ready on (\d+)/, + onSpawn: killOnTeardown(t), + what: "probe", + timeoutMs: 10_000, + pollMs: 25, + }); + assert.equal(match[1], "4242"); +}); + +test("sees an announcement that lands inside the final polling interval", async (t) => { + // The off-by-one this guards: a fixed attempt count checks the output and + // then sleeps, so nothing ever observes the last interval — an announcement + // at 29.9s of a 30s budget reads as a timeout. + // + // Setting pollMs === timeoutMs makes that deterministic rather than a race: + // the attempt-counted loop gets exactly one check, at t≈0 before the child + // has even finished starting, and then throws. The deadline loop re-reads + // after the wait, so *any* announcement before the deadline is seen. The + // child announces immediately, ~2s inside the window on either side, so a + // loaded runner cannot flip the outcome. (Verified: this fails against the + // attempt-counted loop and passes against the deadline loop.) + const { match } = await startAnnouncedChild({ + command: process.execPath, + args: ["-e", 'console.error("listening at http://127.0.0.1:8080")'], + pattern: /listening at (http:\/\/\S+)/i, + onSpawn: killOnTeardown(t), + what: "probe", + timeoutMs: 2000, + pollMs: 2000, + }); + assert.equal(match[1], "http://127.0.0.1:8080"); +}); + +test("a non-announcing child is still reachable and killable after the timeout", async (t) => { + // The regression: a child that is alive but never announces. The throw must + // not be the only thing that happens — the caller must already hold the + // handle, or `process.exit(1)` leaves this running. + let published = null; + await assert.rejects( + startAnnouncedChild({ + // Sleeps well past the budget and prints nothing matching. + command: process.execPath, + args: ["-e", "setTimeout(() => {}, 60000)"], + pattern: /never going to match/, + onSpawn: killOnTeardown(t, (c) => (published = c)), + what: "probe", + timeoutMs: 300, + pollMs: 25, + }), + /probe did not start within/, + ); + + assert.ok(published, "the child was published before the readiness wait"); + assert.equal( + published.exitCode, + null, + "and it is still alive, as in the bug", + ); + + published.kill("SIGKILL"); + await waitForExit(published); + assert.ok( + published.exitCode !== null || published.signalCode !== null, + "teardown could actually stop it", + ); +}); + +test("reports a child that exits before announcing, with its output", async (t) => { + let published = null; + await assert.rejects( + startAnnouncedChild({ + command: process.execPath, + args: ["-e", 'console.error("boom: bad config"); process.exit(3)'], + pattern: /never going to match/, + onSpawn: killOnTeardown(t, (c) => (published = c)), + what: "probe", + timeoutMs: 10_000, + pollMs: 25, + }), + (err) => + /probe exited early/.test(err.message) && + /boom: bad config/.test(err.message), + ); + assert.ok(published, "published even on the early-exit path"); +}); + +test("reports a spawn failure rather than throwing it uncaught", async (t) => { + await assert.rejects( + startAnnouncedChild({ + command: "definitely-not-an-executable-2000", + args: [], + pattern: /never/, + onSpawn: killOnTeardown(t), + what: "probe", + timeoutMs: 10_000, + pollMs: 25, + }), + /could not spawn the probe:/, + ); +}); diff --git a/scripts/smoke-web-app.mjs b/scripts/smoke-web-app.mjs index 4dafa18f..b849f357 100644 --- a/scripts/smoke-web-app.mjs +++ b/scripts/smoke-web-app.mjs @@ -46,13 +46,13 @@ * demand if missing, as in smoke:cli. */ -import { spawn, spawnSync } from "node:child_process"; +import { spawnSync } from "node:child_process"; import { existsSync } from "node:fs"; import { createRequire } from "node:module"; -import { setTimeout as delay } from "node:timers/promises"; import { join, resolve } from "node:path"; import { startProdWebServer } from "./lib/prod-web-server.mjs"; import { stopChild } from "./lib/child-cleanup.mjs"; +import { startAnnouncedChild } from "./lib/announced-child.mjs"; import { resolveNodeBin } from "./lib/resolve-node-bin.mjs"; const repoRoot = resolve(import.meta.dirname, ".."); @@ -164,45 +164,30 @@ function ensureTestServer() { /** * Spawn the MCP App test server and wait for it to announce its URL. * - * Both stdio channels are piped and scanned: server-composable.ts announces - * readiness with `console.error`, so watching stdout alone never matches and - * this times out with an empty diagnostic. Piping both also keeps the child's - * noise out of the smoke's own output while still making it available in the - * failure message. + * The spawn/readiness ownership lives in `lib/announced-child.mjs` so the + * failure path is testable: this smoke's happy path always gets the + * announcement, so nothing here could prove that a child which stays alive + * through the readiness timeout is still reachable by `shutdown()` — the case + * that used to orphan a live server holding its port (#2000). `onSpawn` + * publishes the child to `mcpServer` before the wait, so every throw path is + * covered by teardown. + * + * The announced URL is authoritative rather than the config's port: + * createTestServerHttp resolves through findAvailablePort(), which walks upward + * when the configured value is taken. */ async function startMcpServer() { - const child = spawn( - process.execPath, - [composableServer, "--config", appConfig], - { cwd: repoRoot, stdio: ["ignore", "pipe", "pipe"] }, - ); - let out = ""; - child.stdout.on("data", (d) => (out += d)); - child.stderr.on("data", (d) => (out += d)); - let exited = false; - let spawnError = null; - // A spawn failure (e.g. an unbuilt/renamed entry) emits `error`, NOT `exit` — - // and with no `error` listener Node throws it uncaught, replacing this smoke's - // diagnostic with a raw stack. `close` is listened to alongside `exit` for the - // same reason: it fires in cases `exit` does not, so a child that dies without - // an exit event can't leave the poll below spinning for the full 30s. - child.on("error", (err) => (spawnError = err)); - child.on("exit", () => (exited = true)); - child.on("close", () => (exited = true)); - - for (let attempt = 0; attempt < 120; attempt++) { - // Take the port the server actually bound, not the one we asked for. - const announced = out.match(/listening at (http:\/\/\S+)/i); - if (announced) return { child, url: announced[1] }; - if (spawnError) { - throw new Error( - `could not spawn the MCP test server (${composableServer}): ${spawnError.message}`, - ); - } - if (exited) throw new Error(`MCP test server exited early:\n${out}`); - await delay(250); - } - throw new Error(`MCP test server did not start within 30s:\n${out}`); + const { match } = await startAnnouncedChild({ + command: process.execPath, + args: [composableServer, "--config", appConfig], + cwd: repoRoot, + pattern: /listening at (http:\/\/\S+)/i, + onSpawn: (child) => { + mcpServer = child; + }, + what: `MCP test server (${composableServer})`, + }); + return { url: match[1] }; } /** base64url(JSON) — the appArgs encoding the deep link expects. */ @@ -247,7 +232,9 @@ try { } ensureTestServer(); - ({ child: mcpServer, url: mcpUrl } = await startMcpServer()); + // `startMcpServer` publishes the child to `mcpServer` itself, so teardown + // reaches it even when this throws before returning. + ({ url: mcpUrl } = await startMcpServer()); await server.waitForReady(); browser = await loadChromium(); const page = await browser.newPage(); diff --git a/specification/v2_servers_file.md b/specification/v2_servers_file.md index 53a7a0a2..b9b09f0a 100644 --- a/specification/v2_servers_file.md +++ b/specification/v2_servers_file.md @@ -261,11 +261,11 @@ Each server entry may carry these Inspector-extension fields at the top level: - **Secret store selection (#1950)**: #1356 assumed a keychain. On a host without one — the published container (no D-Bus session, #1848), Android/Termux (no prebuilt binary, #1905), a minimal Linux install — `set` hard-failed, so those users could not persist an OAuth client secret or a stdio `env:` value **at all**. #1950 adds the two missing implementations plus the policy that picks one, and the surfaces that say which was picked. - **Selection**: `MCP_INSPECTOR_SECRET_STORE=keyring|file|memory` wins outright. Otherwise the keychain is _probed_ — an `AsyncEntry` construction plus a real read, because the container that motivated #1848 imports the package fine and only fails when it reaches for a Secret Service that isn't there. A read and not a write, deliberately: probing by writing would deposit a value in the user's login keyring at every startup for a store they may never use. If the probe fails the fallback is **`memory`** in a container whose secrets directory is not on a mount, and **`file`** otherwise — so mounting the volume the README already recommends for the catalog flips the same run to durable storage with no configuration, and an unmounted container gets the honest answer rather than a file `docker run --rm` will discard. Resolution is cached per process so the startup banner, `GET /api/config`, and the store doing the writing cannot disagree. - **`FileSecretStore` at rest**: one JSON document at `~/.mcp-inspector/secrets.json` (or `MCP_INSPECTOR_SECRET_FILE`, else under `MCP_STORAGE_DIR`), written `0600` and re-tightened at startup. With `MCP_INSPECTOR_SECRET_KEY` set it is AES-256-GCM with a scrypt-derived key and a per-write random salt; without it the values are in the clear, and _that_ is what the loud banner and the warning-toned modal footer are about. The **whole map is encrypted as a unit**, not value-by-value, so the account names (`serverId:field`) are hidden too — a per-value scheme would leave a readable index of which servers you hold a client secret for. Upgrading is lazy: setting the passphrase later re-encrypts on the next write rather than rewriting the file during a run that may never touch a secret, and the descriptor reports `pendingEncryption` until it does. A file that can no longer be decrypted reads as empty but **refuses to be written**, because replacing a file of still-valid secrets to satisfy an additive request destroys data. - - **Concurrency, and what it does not promise**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes there is **no lock**. An earlier iteration had a `mkdir` election with an owner stamp, heartbeat and stale-takeover; three review rounds each found a real race, and the last is not closable with what Node exposes — claiming a stale lock needs compare-and-swap on a directory entry (`renameat2(RENAME_EXCHANGE)`). It was replaced with optimistic concurrency: read `M0`, apply, write `M1`, read back `M2`, and re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. **This is not mutual exclusion.** The verify only catches a clobber that has already landed, so ordering the two writers write-A / verify-A / write-B / verify-B leaves both reporting success with A's entry gone — A's verify ran before there was anything to see. A crash between write and verify is one instance of that shape, not the whole of it. The window is narrower than the lock's (which lost updates across a wider set of interleavings, with every participant alive) and needs no primitive Node lacks, but it is a real residual and is documented as one. If a deployment needs the guarantee, the answer is an OS-backed lock from a dedicated library, not another hand-rolled election. + - **Concurrency**: within a process, mutations are serialized per **resolved file path** (not per store instance — two `FileSecretStore`s on one file are ordinary, since the resolved store holds one and the keychain hand-off builds another). Across processes, each mutation holds an exclusive `proper-lockfile` lock on `secrets.json.lock` for the whole read-modify-write (#2082). The in-process queue is what keeps that usable: `proper-lockfile` is not reentrant, so a second `lock()` from the same process fails `ELOCKED` and would be indistinguishable from a genuine remote holder. An earlier iteration hand-rolled the lock — a `mkdir` election with an owner stamp, heartbeat and stale-takeover — and three review rounds each found a real race, the last not closable with what Node exposes (claiming a stale lock needs compare-and-swap on a directory entry, `renameat2(RENAME_EXCHANGE)`). #2082 settled that as "borrow, don't hand-roll" — but the borrowed lock is not claimed to close that race, because it does not. `proper-lockfile@4.1.2` `rmdir`s a stale lock and re-`mkdir`s without checking the directory it removed is the one it found stale, so a slow waiter can still delete a fast waiter's fresh lock; and its own detection is weaker than it looks: `updateLock` compares mtime only on the refresh tick (`stale / 2`, 5s here), while an ordinary mutation finishes in well under a second — so in the common case the tick never runs and nobody is told. Its `release` is also an unconditional `rmdir` with no ownership check, so a holder whose lock was replaced deletes the _winner's_ lock on the way out, turning one compromised writer into two unprotected ones. `withSecretFileLock` therefore supplies a guarded `options.fs` whose directory removal refuses to delete a lock that is no longer the one it created (inode + birth time, which survive the library's `utimes` refresh but not a delete-and-recreate). It sits in `options.fs` rather than around `release()` because that is the one seam **both** removal paths route through — the release path and the library's `signal-exit` handler, which `rmdirSync`s every registered lock with no ownership check of its own; a guard around release alone leaves an exit at the wrong moment free to delete the winner's directory. This **narrows** the destructive window (the check is a `statSync` immediately followed by an `rmdirSync`, so nothing in-process interleaves) and surfaces the takeover in the fast case the tick misses — but it does not close it: it is still check-then-act across processes, which needs the same CAS Node does not expose. Best-effort throughout, and where a filesystem reports neither field it degrades to the library's unaided behaviour rather than to a false alarm. What **is** exclusive is the case that matters: `mkdir` is atomic and a live holder refreshes its mtime, so its lock never goes stale and two running Inspectors are genuinely serialized. The residual window opens only after a holder dies without releasing. A waiter also waits past the stale window before giving up (so a crashed holder resolves by takeover rather than failing everyone else's saves), and a lock still held after that makes `set` **fail** rather than write alongside a visible concurrent writer — `ELOCKED` is evidence the lock is working, not a reason to bypass it. **The optimistic verify stays underneath it**, and is not redundant: read `M0`, apply, write `M1`, read back `M2`, re-apply onto whatever a concurrent writer left if they differ, bounded, with `set` throwing on non-convergence rather than returning as though the value were saved. The comparison is over the **whole map** — checking only your own entry passes in exactly the case that loses data, because yours is present and the other writer's is gone. A lock is advisory between the processes that take it, so the verify is what covers a writer outside this codebase (an editor, a restored backup, an Inspector predating #2082) and what covers the lock being _unavailable_: `withSecretFileLock` runs the body anyway, warning once, on a directory that cannot hold a lock file — this store exists for boxes where the usual mechanism is missing (#1848, #1905) and must not acquire a new way to be unavailable. Reads take no lock; `writeStoreFile` is atomic, so a reader sees the old file or the new one, never a torn one. - **Strict reads (`getStrict`)**: `get` is tolerant by contract, and that tolerance is wrong for exactly one caller — a migration that treats `null` as proof of absence and then _writes_. A transient read failure would look like "nothing there", and the write would replace a newer stored value with an older on-disk copy, inverting the keychain-wins rule the migration is built on. `getStrict` throws instead, and both plaintext migrations plus the keychain hand-off use it. Two traps this hit on the way in, both worth remembering: the seam must be forwarded by `DeferredSecretStore` (the store production actually uses, so without forwarding the strictness existed only in tests that injected a concrete store), and it must be implemented by _every_ store rather than falling back to `get` for the one it was introduced for. - **Bulk reads (`getMany`)**: rehydration asked field by field, and `FileSecretStore.get` reads and decrypts the entire file per call. scrypt at `N=16384` measures ~23ms, so an encrypted catalog spent ~450ms of pure key derivation on every `GET /api/servers` — a visible stall rather than a micro-optimization. The seam takes a **list of `{ serverId, fields }` across servers**, and both rehydration callers pass the whole catalog in one request, so `FileSecretStore` decrypts once per rehydration; stores for which per-field reads are already cheap (the keychain) fall back to parallel `get`. The cross-server shape is the point: a per-server version shipped first and left a 20-server catalog paying 20 serialized derivations, because both callers iterate servers — the same stall reached one server at a time instead of one field at a time. - **Durability gate on migration**: the plaintext-stripping migrations only delete the disk copy once the value is somewhere that outlives the process (`isDurable`). Against a session-scoped store, stripping would trade a secret that survives restarts for one that dies with the process — and it runs on an ordinary `GET`, so merely opening the app would destroy it. - - **Hand-off when a keychain appears**: install libsecret after storing secrets in a file, and the next run selects the keychain and stops seeing them — still on disk, read by nothing, nothing visibly broken. `absorbFileSecretsIntoKeyring` copies them over on that run, under the same keychain-wins rule, and deletes the file **only** on complete success. Since the store takes no lock, the source is **claimed atomically** first: the live `secrets.json` is renamed to a unique snapshot (pid plus a per-attempt nonce, so a staging path can never be reused — pid 1 recurs on every container start), the migration reads only that snapshot, and a writer that recreates the live path is untouched and migrated on the next run. The snapshot is deleted only on a complete hand-off; otherwise it is restored with `link` + `unlink` rather than `rename`, since POSIX `rename` silently replaces its destination and would overwrite a newer live file. A snapshot left behind by a process that died mid-migration is adopted at startup — checking only the canonical path would otherwise report "nothing to migrate" while every stored credential quietly disappeared. + - **Hand-off when a keychain appears**: install libsecret after storing secrets in a file, and the next run selects the keychain and stops seeing them — still on disk, read by nothing, nothing visibly broken. `absorbFileSecretsIntoKeyring` copies them over on that run, under the same keychain-wins rule, and deletes the file **only** on complete success. The claim runs under the same cross-process lock a `set` takes, and the source is **claimed atomically** within it: the live `secrets.json` is renamed to a unique snapshot (pid plus a per-attempt nonce, so a staging path can never be reused — pid 1 recurs on every container start), the migration reads only that snapshot, and a writer that recreates the live path is untouched and migrated on the next run. The snapshot is deleted only on a complete hand-off; otherwise it is restored with `link` + `unlink` rather than `rename`, since POSIX `rename` silently replaces its destination and would overwrite a newer live file. A snapshot left behind by a process that died mid-migration is adopted at startup — checking only the canonical path would otherwise report "nothing to migrate" while every stored credential quietly disappeared. - **Surfacing it**: the active store rides `GET /api/config` as a `secretStorage` descriptor and is stated in a permanent footer at the bottom of every dialog that accepts a secret: Client Settings (the enterprise IdP client secret), Server Settings (the per-server OAuth client secret and stdio `env:` values), and Server Config (stdio `env:` values). That third one was missed at first, which made it the one dialog taking secrets with no disclosure at all — so the count here is load-bearing rather than descriptive. A startup banner is seen once by whoever started the process; a toast is seen once; a dismissible banner is by design the thing a user dismisses before doing the work it describes. The descriptor is re-derived per request rather than cached, because `plaintext`/`pendingEncryption` describe bytes this very process changes. - **Hard-cutover legacy behavior (per #1358 decision 4)**: files written by the one pre-#1358 build of v2/main have a nested `settings` block. `normalizeMcpServers` drops the node on read and logs a one-line warn including the server id; the persisted headers / metadata / timeouts / OAuth credentials are intentionally lost on first read. Users re-enter them via the settings form (or hand-edit the file into the flat shape). v2 has not shipped a stable release with the nested shape, so the blast radius is the small set of v2/main dogfooders who edited per-server settings between #1353 merging and this change. @@ -281,7 +281,6 @@ Each server entry may carry these Inspector-extension fields at the top level: ## Out of scope (follow-ups) -- Import-from-Claude-Desktop button (read `~/Library/Application Support/Claude/claude_desktop_config.json` or the Windows/Linux equivalent, merge into our file). - File watching for hot reload of external edits. - Per-server tags / folders / groups. - Export current list as JSON. diff --git a/vitest.shared.mts b/vitest.shared.mts index 1ebbc96a..8ad914b5 100644 --- a/vitest.shared.mts +++ b/vitest.shared.mts @@ -101,6 +101,17 @@ export function vitestSharedPaths(clientDir: string) { find: /^yaml$/, replacement: path.resolve(repoRoot, "node_modules/yaml"), }, + // Same reasoning, one layer in: `proper-lockfile` is reached only through + // `core/` (the secrets file's cross-process lock, #2082), which is the + // other root-owned tree with no manifest of its own. Resolution finds the + // root copy on its own today — nothing declares it in a client — and this + // pin is what keeps that from depending on nothing ever arriving as some + // client's transitive dependency, which would otherwise give a test two + // copies of a module whose whole job is a single registry of held locks. + { + find: /^proper-lockfile$/, + replacement: path.resolve(repoRoot, "node_modules/proper-lockfile"), + }, ]; const projectResolve = {