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 f3d3fa70..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 @@ -828,6 +828,28 @@ describe("absorbFileSecretsIntoKeyring", () => { 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 diff --git a/core/auth/node/file-lock.ts b/core/auth/node/file-lock.ts index eceed45e..73772031 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -402,10 +402,22 @@ export async function isFileLockHeld(filePath: string): Promise { * Returns whatever `fn` returns. `fn` runs exactly once either way — the * lock's absence changes the guarantee, never whether the work happens. */ -export async function withSecretFileLock( +/** + * 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, - fn: () => Promise, -): Promise { +): 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* @@ -427,7 +439,7 @@ export async function withSecretFileLock( `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) | undefined; + let release: () => Promise; try { release = await acquire(target, 0, fsShim).catch((err: unknown) => { // **Retries are for contention, and only for contention.** @@ -462,11 +474,9 @@ export async function withSecretFileLock( 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 fn(); + return null; } - try { - return await fn(); - } finally { + return async () => { try { await release(); } catch (err) { @@ -476,26 +486,35 @@ export async function withSecretFileLock( // // **`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 our `release()` - // 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` has already said - // what happened, so there is nothing to add. - // - // A conditional rather than an early `return`: a `return` inside - // `finally` discards whatever the body was returning or throwing. - if ((err as NodeJS.ErrnoException).code !== "ERELEASED") { - warnOnce( - // No branch on the *removal* error code, deliberately. Our own - // guard `stat`s before removing, so anything else reaching here is - // an `rmdir` that was refused — and **stale takeover reclaims - // through that same `rmdir`**, so whatever blocked ours blocks that - // too. `ENOTEMPTY` is the reachable one; `EACCES`, `EPERM` and - // `EROFS` behave identically. Promising expiry was wrong for all. - `Could not release the lock on the secrets file at ${target} (${describeError(err)}). Stale takeover reclaims a lock through the same directory removal, so whatever blocked this blocks that too: saves will keep failing until you remove ${lockPathOf(target)} by hand.`, - ); - } + // 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( + // No branch on the *removal* error code, deliberately. Our own guard + // `stat`s before removing, so anything else reaching here is an + // `rmdir` that was refused — and **stale takeover reclaims through + // that same `rmdir`**, so whatever blocked ours blocks that too. + // `ENOTEMPTY` is the reachable one; `EACCES`, `EPERM` and `EROFS` + // behave identically. Promising expiry was wrong for all of them. + `Could not release the lock on the secrets file at ${target} (${describeError(err)}). Stale takeover reclaims a lock through the same directory removal, so whatever blocked this blocks that too: saves will keep failing until you 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/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index 8b2add61..3a55ec1f 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -54,7 +54,11 @@ import { readSecretFilePermissions, tightenSecretFilePermissions, } from "./file-secret-store.js"; -import { isFileLockHeld, withSecretFileLock } from "./file-lock.js"; +import { + isFileLockHeld, + openSecretFileLock, + withSecretFileLock, +} from "./file-lock.js"; import { KeyringSecretStore, parseAccount, @@ -604,7 +608,17 @@ export async function absorbFileSecretsIntoKeyring( } return null; } - return staged; + // **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 @@ -616,7 +630,14 @@ export async function absorbFileSecretsIntoKeyring( return null; }); if (claimed === null) return; - await handOffStagedSecrets(claimed, filePath, keyring); + // 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?.(); + } } /** @@ -639,21 +660,6 @@ async function handOffStagedSecrets( staged: string, filePath: string, keyring: SecretStore, -): Promise { - await withSecretFileLock(staged, () => - handOffStagedSecretsLocked(staged, filePath, keyring), - ).catch((err: Error) => { - console.warn( - `\n[mcp-inspector] Could not lock ${staged} to migrate it into the OS keychain (${err.message}), so it has been left in place. The next run will try again.`, - ); - }); -} - -/** {@link handOffStagedSecrets}'s body, with the snapshot's lock held. */ -async function handOffStagedSecretsLocked( - staged: string, - filePath: string, - keyring: SecretStore, ): Promise { const file = new FileSecretStore({ filePath: staged }); // True only when every value reached the keychain *and* there was @@ -702,6 +708,22 @@ async function handOffStagedSecretsLocked( } } +/** + * 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? @@ -715,7 +737,7 @@ async function anythingToMigrate(filePath: string): Promise { const base = path.basename(filePath); try { return (await fs.readdir(path.dirname(filePath))).some( - (name) => name === base || name.startsWith(`${base}.migrating-`), + (name) => name === base || isSnapshotName(name, base), ); } catch (err) { const code = (err as NodeJS.ErrnoException).code; @@ -742,10 +764,10 @@ async function anythingToMigrate(filePath: string): Promise { */ 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. }