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 0f10bc5b..6d045328 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 @@ -493,5 +493,13 @@ describe("withSecretFileLock reports what it cannot clean up", () => { { 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 21a6ea3d..f3d3fa70 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 @@ -788,6 +788,46 @@ describe("absorbFileSecretsIntoKeyring", () => { }, ); + 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("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 f9f8f5f5..eceed45e 100644 --- a/core/auth/node/file-lock.ts +++ b/core/auth/node/file-lock.ts @@ -367,6 +367,30 @@ function acquire( }); } +/** + * 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`. * @@ -449,15 +473,29 @@ export async function withSecretFileLock( // 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. - warnOnce( - // No branch on the error code, deliberately. Our own guard `stat`s - // before removing, so anything 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 for "everything else" 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.`, - ); + // + // **`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.`, + ); + } } } } diff --git a/core/auth/node/secret-store-selection.ts b/core/auth/node/secret-store-selection.ts index e35a5fac..8b2add61 100644 --- a/core/auth/node/secret-store-selection.ts +++ b/core/auth/node/secret-store-selection.ts @@ -54,7 +54,7 @@ import { readSecretFilePermissions, tightenSecretFilePermissions, } from "./file-secret-store.js"; -import { withSecretFileLock } from "./file-lock.js"; +import { isFileLockHeld, withSecretFileLock } from "./file-lock.js"; import { KeyringSecretStore, parseAccount, @@ -546,21 +546,18 @@ export async function absorbFileSecretsIntoKeyring( // after the claim below. Everything that *acts* re-checks under the lock. if (!(await anythingToMigrate(filePath))) return; - // Orphan adoption and the claim below both move the live path around, so - // they 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 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. + // 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 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. // - // 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 @@ -619,8 +616,45 @@ export async function absorbFileSecretsIntoKeyring( return null; }); if (claimed === null) return; - const staged = claimed; + await handOffStagedSecrets(claimed, filePath, keyring); +} +/** + * 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 { + 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 // something to move — the one case where the snapshot is redundant. @@ -717,6 +751,14 @@ async function recoverOrphanedSnapshots(filePath: string): Promise { } 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 });