modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

Merge branch 'v2/main' into v2/fix/1914-surface-post-add-refresh-failure

Browse source 
Both lines changed the same two settings-write paths in App.tsx:

- #2089 (v2/main) rewrote the pagination toggle and the settings-draft
  flush around `useLastPersistedSettings`, so each write announces itself
  with `begin` and confirms with `landed`/`failed`.
- #1914 (this branch) made a failed post-write list reload reject with
  `ServerListReloadError` — the PUT landed, only reading the list back
  failed.

Resolved by treating a `ServerListReloadError` as a *landed* write in the
new model rather than a failed one: both paths now settle it exactly as
the success path does (so it becomes the rollback baseline and re-applies
itself when settled) and only the toast says the reload failed. Rolling
it back would put the UI, the live client and the modal's seed on a value
disk no longer holds — the very contradiction #2089 exists to prevent.

The two suites' new tests were interleaved by the conflict and are both
kept, plus one covering the combination: a modal save whose reload failed
is recorded as the landed write instead of rolled back.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UL9retfvAXRgvi6EWk4SY4
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-23 19:37:27 -04:00
commit b1996d1093
3 changed files with 3185 additions and 230 deletions
Download patch file Download diff file Expand all files Collapse all files

527
core/auth/node/file-lock.ts Normal file
View file

@ -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<string>();
/**
* 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<void>> {
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<boolean> {
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 `<filePath>.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<void>) | 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<void>;
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<T>(
filePath: string,
fn: () => Promise<T>,
): Promise<T> {
const release = await openSecretFileLock(filePath);
if (release === null) return fn();
try {
return await fn();
} finally {
await release();
}
}

91
core/auth/node/file-secret-store.ts
View file

@ -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<string, string>) => Record<string, string> | null,
): Promise<void> {
await withSecretFileLock(this.filePath, () => this.mutateLocked(apply));
}
/** {@link mutate}'s body, with the lock already held. */
private async mutateLocked(
apply: (map: Record<string, string>) => Record<string, string> | null,
): Promise<void> {
for (let attempt = 0; attempt < MAX_WRITE_ATTEMPTS; attempt++) {
let current: Record<string, string>;
@ -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<T>(fn: () => Promise<T>): Promise<T> {
const key = path.resolve(this.filePath);

223
core/auth/node/secret-store-selection.ts
View file

@ -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<void> {
const filePath = defaultSecretFilePath();
// A crash between the claim and the delete leaves only
// `secrets.json.migrating-<pid>`. 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-<pid>`. 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<void> {
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-<pid>-<uuid>.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 `<name>.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<boolean> {
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<void> {
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 });