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 f7317d2e..e9cb8f7e 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,39 @@ 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 @@ -702,6 +735,25 @@ describe("absorbFileSecretsIntoKeyring", () => { expect(existsSync(`${filePath}.lock`)).toBe(false); }); + it("does not treat an unlistable directory as an empty one (#2082)", async () => { + // Only "there is no directory" proves the fresh-install case. A path + // whose parent is a *file* answers `readdir` with ENOTDIR — the other + // shape of "nothing there" — and must be treated the same, whereas an + // EACCES directory that denies listing while still permitting access to + // the known `secrets.json` must not be, or the keychain is selected and + // those secrets go invisible with nothing said. + 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(); + }); + 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 diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index d61108d0..56d7782c 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -96,6 +96,28 @@ const RETRY = { 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(); @@ -231,7 +253,7 @@ export async function withSecretFileLock( // retries — whereas proceeding can lose a secret while reporting success. if (isHeldElsewhere(err)) { throw new SecretStoreUnavailableError( - `Could not save to the secrets file at ${target}: another process has held the lock on it for over ${Math.round((RETRY.retries * RETRY.maxTimeout) / 1000)} seconds. Its secrets are intact; the value you just entered was not saved. If no other Inspector is running, remove ${target}.lock and try again.`, + `Could not save to the secrets file at ${target}: another process has held the lock on it for 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 ${target}.lock and try again.`, ); } // Everything else is the lock being *unavailable* rather than held — a diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index ce1704cc..e35a5fac 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -553,6 +553,14 @@ export async function absorbFileSecretsIntoKeyring( // The lock is released before the hand-off: that part reads a snapshot // nobody else can reach, and holding it across a keychain round-trip per // secret would block every writer for the duration of a migration. + // + // Wrapped because `withSecretFileLock` **throws** when another process + // holds the lock past its retry budget, and this function's contract is + // that it never throws: it is awaited directly by both `resolveSecretStore` + // branches, so a stuck writer would fail store resolution and with it the + // whole session. Refusing is the right answer for a `set` — the user is + // waiting on that value — and the wrong one here, where the file simply + // stays put and the next run migrates it. 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 @@ -600,6 +608,15 @@ export async function absorbFileSecretsIntoKeyring( return null; } return staged; + // `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; const staged = claimed;