fix(auth): hold the snapshot lock across the main lock's release (#2082)

Copilot review round 8 — two findings, both upheld. The first is a gap in
round 7's own fix.

**The snapshot was unprotected between two awaits.** Returning from
`withSecretFileLock(filePath, ...)` releases the main lock *before*
`handOffStagedSecrets` acquired the snapshot's, so a second startup could
take the main lock in that window, see the staged file unlocked, and adopt
and re-claim it — leaving this process reading a path that no longer exists.
Narrower than the race round 7 closed, but the same one.

`openSecretFileLock` is now split out of `withSecretFileLock`: it takes the
lock and returns its release, so a caller can hold one *across* another's
release. The claim acquires the snapshot's lock before returning — inside the
main lock — and the hand-off then runs outside the main lock, still holding
it. `withSecretFileLock` is a thin wrapper over the same function, so which
failures refuse and which degrade is decided in one place and the two entry
points cannot drift.

Locking the snapshot degrades rather than refusing (`null` → hand off
anyway): the exposure is one uniquely-named snapshot, which is a better trade
than refusing to migrate at all on a box that cannot lock.

**Both migration scans matched the lock directories themselves.**
`secrets.json.migrating-<pid>-<uuid>.lock` passes the plain prefix test, and
treating it as a snapshot is self-sustaining damage: the liveness probe asks
about a nonexistent `<name>.lock.lock` and so answers "not held", recovery
tries to hard-link a *directory* onto the secrets path, fails, and prints the
orphan warning. Because a liveness *check* never clears a stale lock
directory — only a would-be acquirer does — that false migration repeats on
every startup forever, including long after the real snapshot was recovered.
`isSnapshotName` now excludes `.lock` for both `anythingToMigrate` and
`recoverOrphanedSnapshots`.

New test verified by mutation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JTHVxSu8AUgHRLvo1ntZ8H
Signed-off-by: cliffhall <cliff@futurescale.com>

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<void> {
-  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<void> {
   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-<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?
@@ -715,7 +737,7 @@ async function anythingToMigrate(filePath: string): Promise<boolean> {
   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<boolean> {
  */
 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.
   }