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 index eb9b4dd1..714432bb 100644 --- a/clients/web/src/test/integration/auth/node/file-lock.test.ts +++ b/clients/web/src/test/integration/auth/node/file-lock.test.ts @@ -72,12 +72,32 @@ const warnings = (): string => 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(); @@ -148,32 +168,83 @@ describe("withSecretFileLock across processes", () => { expect(warnings()).toBe(""); }); - it("serializes two FileSecretStores in different processes", async () => { + 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. - // The child holds the lock while the parent's `set` is in flight, so the - // parent's whole read-modify-write happens after the child is gone. + // + // 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 store = new FileSecretStore({ filePath: target }); - await store.set("srv", "env:FIRST", "1"); - - const { ready, done } = await holdLockInChildProcess(target, 300); + const { ready, done } = await holdLockInChildProcess(target, 700, { + "srv:env:FROM_CHILD": "1", + }); await ready; - await store.set("srv", "env:SECOND", "2"); + + 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")).toBe("1"); - expect(await reader.get("srv", "env:SECOND")).toBe("2"); + 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 directory that does not exist stands in for every real variant — + // 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. This store exists for boxes where the usual - // mechanism is missing, so it must not gain a new way to be unavailable. - const target = path.join(tmpDir, "no-such-dir", "secrets.json"); + // `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 () => { diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index 66dcaea7..21d89835 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -35,6 +35,7 @@ * underneath it, and says so once. */ +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 @@ -140,6 +141,19 @@ export async function withSecretFileLock( fn: () => Promise, ): Promise { 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(() => {}); let release: (() => Promise) | undefined; try { release = await properLockfile.lock(target, { diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 4cd69375..ce1704cc 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -666,10 +666,18 @@ async function anythingToMigrate(filePath: string): Promise { return (await fs.readdir(path.dirname(filePath))).some( (name) => name === base || name.startsWith(`${base}.migrating-`), ); - } catch { - // No storage directory yet — the first run on a fresh install, and the - // path this check exists to keep quiet. - return false; + } 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"; } } 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 = {