fix(auth): refuse on a stuck lock, capture identity at the mkdir seam (#2082)

Copilot review round 6 — five findings (two inline, three suppressed), all
valid.

**A stale lock that cannot be cleared caused unlocked writes.** `acquireLock`
does not only *create* directories: on finding a stale one it removes it and
retries, and that removal can fail — `ENOTEMPTY` for a lock with anything
inside it, `EACCES`/`EROFS` for one we may not touch. Those surface as
ordinary non-`ELOCKED` errors, which the probe read as "locks do not work
here" and degraded on. So every Inspector on the box quietly bypassed the
*same* stuck lock and raced its writes — while the release-failure message
was telling the operator saves would keep failing until they cleared it.

`isStuckOrHeld` now discriminates on **whether the lock directory exists**
rather than on an errno taxonomy: if it is there, something holds it and we
refuse; if it is not, we genuinely could not create one and degrading is the
documented trade (#1848, #1905). Reading the state the decision is about
avoids enumerating error codes per platform — and an enumeration is exactly
what let `ENOTEMPTY` through.

**Identity was captured after the acquire promise settled.** That span
includes the library's own `utimes`/`stat` probe, so a waiter replacing our
directory inside it would have us record *the winner's* identity as our own,
after which the removal guard would accept and delete their lock. It is now
recorded in the `mkdir` callback — the moment the directory becomes ours, and
synchronous with respect to this process, so there is no window.

**The expiry advice was still wrong for `EACCES`/`EPERM`/`EROFS`.** Since the
guard `stat`s before removing, everything reaching the release catch is an
`rmdir` that was refused — and stale takeover reclaims through that same
`rmdir`. The `ENOENT` arm was unreachable for the same reason, so the branch
was dead code: removed, and the advice is now unconditional.

**`proper-lockfile` was missing from `NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE`.** A
fourth externalization surface — the tsup lists configure the production
bundles, not `vite dev`, whose dep scanner would otherwise walk its
CJS/`graceful-fs`/`signal-exit` graph. Added, and `vite-base-config.test.ts`
now asserts it along with `chokidar` and `@napi-rs/keyring`, which were also
absent from that assertion.

**A test was named for something it did not exercise.** "does not treat an
unlistable directory as an empty one" was driving `ENOTDIR`, which is
deliberately treated *as* the fresh-install case, so the actual new behaviour
was untested. Split in two: one for `ENOTDIR`, and a real `EACCES` one using
a `--x` directory — `readdir` denied, a known name still reachable, which is
the asymmetry that makes "unlistable" different from "empty". Driven with a
real mode rather than a stub, since `vi.spyOn` cannot redefine an ESM
namespace export; skipped as root and on Windows, where the mode would not
bite and the test would assert nothing.

Both new tests 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>

clients/web/server/vite-base-config.ts

@@ -34,6 +34,14 @@ const NODE_ONLY_OPTIMIZE_DEPS_EXCLUDE = [
   // excluding it keeps Vite's dep scanner from chasing into the
   // platform-specific binaries during dev startup.
   "@napi-rs/keyring",
+  // `proper-lockfile` is reached only through `core/auth/node/file-lock.ts`
+  // — the secrets file's cross-process lock (#2082) — which the Hono
+  // `/api/servers` handlers pull in via `core/auth/node/file-secret-store.ts`.
+  // Same node-only import chain as `atomically` above, and the same reason:
+  // it is CJS with a `graceful-fs`/`signal-exit`/`retry` graph that Vite's
+  // dev scanner has no business walking. Note the tsup `external` lists do
+  // **not** cover this — they configure the production bundles, not `vite dev`.
+  "proper-lockfile",
 ] as const;
 
 export function getViteBaseConfig() {

clients/web/src/test/integration/auth/node/file-lock.test.ts

@@ -305,6 +305,37 @@ describe("withSecretFileLock degrades rather than failing", () => {
     expect(existsSync(target)).toBe(false);
   }, 90_000);
 
+  it("refuses rather than degrading when a stale lock cannot be cleared", async () => {
+    // `acquireLock` does not only *create* directories — on finding a stale
+    // one it removes it and retries, and that removal can fail. A stale lock
+    // with anything inside it fails `ENOTEMPTY`, which is not `ELOCKED`, and
+    // treating every non-`ELOCKED` error as "locks do not work here" meant
+    // every Inspector on the box quietly bypassed the *same* stuck lock and
+    // raced its writes — while the release-failure message was telling the
+    // operator saves would keep failing until they cleared it.
+    const target = filePath();
+    const lockPath = `${target}.lock`;
+    await fs.mkdir(path.dirname(target), { recursive: true });
+    await fs.mkdir(lockPath);
+    await fs.writeFile(`${lockPath}/stray`, "", "utf-8");
+    // Backdated so it reads as stale — which is what sends `acquireLock` down
+    // the remove-and-retry path rather than straight to `ELOCKED`.
+    const longDead = new Date(Date.now() - 60_000);
+    await fs.utimes(lockPath, longDead, longDead);
+
+    const store = new FileSecretStore({ filePath: target });
+    const err = await store
+      .set("srv", "env:MINE", "1")
+      .catch((e: unknown) => e);
+
+    expect(err).toBeInstanceOf(SecretStoreUnavailableError);
+    expect((err as Error).message).toMatch(/was not saved/);
+    // Nothing written behind the stuck lock, and no "unprotected" warning:
+    // this is a refusal, not a degrade.
+    expect(existsSync(target)).toBe(false);
+    expect(warnings()).not.toContain("not protected");
+  }, 90_000);
+
   it("takes over the lock of a holder that died, rather than failing the save", async () => {
     // The invariant behind refusing on `ELOCKED`: refusing is only defensible
     // because a *crashed* holder resolves on its own first. `RETRY` therefore

clients/web/src/test/integration/auth/node/secret-store-selection.test.ts

@@ -735,13 +735,11 @@ 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.
+  it("treats ENOTDIR as the fresh-install case, like ENOENT (#2082)", async () => {
+    // A path whose parent is a *file* answers `readdir` with ENOTDIR — the
+    // other shape of "there is no directory here" — so it takes the same
+    // silent path as a missing one. The *unlistable* case is different and is
+    // covered by the test below.
     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;
@@ -754,6 +752,42 @@ describe("absorbFileSecretsIntoKeyring", () => {
     expect(warn).not.toHaveBeenCalled();
   });
 
+  // Root bypasses POSIX permission checks, and Windows does not model them
+  // the same way — in either case the directory below stays listable and the
+  // test would assert nothing.
+  const canDenyListing =
+    process.platform !== "win32" && process.getuid?.() !== 0;
+
+  it.skipIf(!canDenyListing)(
+    "does not treat an unlistable directory as an empty one (#2082)",
+    async () => {
+      // The case ENOTDIR does *not* cover, and the reason the catch narrowed
+      // to ENOENT/ENOTDIR rather than swallowing everything. A directory with
+      // `--x` permission denies `readdir` with EACCES while still permitting
+      // access to a *known* name inside it — so reading EACCES as "nothing to
+      // migrate" selects the keychain and leaves those secrets invisible with
+      // nothing said. That asymmetry is real POSIX behaviour, which is why
+      // this drives it with a real mode rather than a stubbed `readdir`.
+      const filePath = await seedFile({ "srv:oauthClientSecret": "from-file" });
+      process.env.MCP_INSPECTOR_SECRET_FILE = filePath;
+      vi.spyOn(console, "warn").mockImplementation(() => {});
+      // Write + execute, no read: `readdir` fails, `stat`/`open`/`rename` of a
+      // known name still work — which is exactly what the migration needs.
+      await fs.chmod(tmpDir, 0o300);
+      try {
+        const mod = await loadWithProbe(true);
+        const keyring = new InMemorySecretStore();
+        await mod.absorbFileSecretsIntoKeyring(keyring);
+
+        // It went on and migrated, rather than silently skipping.
+        expect(await keyring.get("srv", "oauthClientSecret")).toBe("from-file");
+      } finally {
+        // Restore before `afterEach`, which needs to list it to remove it.
+        await fs.chmod(tmpDir, 0o700);
+      }
+    },
+  );
+
   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

clients/web/src/test/integration/server/vite-base-config.test.ts

@@ -21,8 +21,14 @@ describe("getViteBaseConfig", () => {
       expect.arrayContaining([
         "@modelcontextprotocol/client/stdio",
         "atomically",
+        "chokidar",
         "cross-spawn",
         "which",
+        "@napi-rs/keyring",
+        // #2082 — reached through `core/auth/node/file-lock.ts`. The tsup
+        // `external` lists cover the production bundles, not `vite dev`, so
+        // a node-only dependency has to be named in both places.
+        "proper-lockfile",
       ]),
     );
   });

core/auth/node/file-lock.ts

@@ -162,6 +162,32 @@ export function resetFileLockWarnings(): void {
   warned.clear();
 }
 
+/**
+ * Is this failure "the lock is there and we could not have it", as opposed to
+ * "locks do not work here"?
+ *
+ * `ELOCKED` is the obvious member, but not the only one, and the difference
+ * decides whether a save **refuses** or **degrades to an unlocked write** —
+ * so getting it wrong is not cosmetic. `acquireLock` does not only *create*
+ * directories: on finding a stale one it removes it and retries, and that
+ * removal can fail — `ENOTEMPTY` for a lock with anything inside it, `EACCES`
+ * or `EROFS` for one we may not touch. Those surface as ordinary non-`ELOCKED`
+ * errors, and treating them as infrastructure failures meant every Inspector
+ * on the box quietly bypassed the *same* stuck lock and raced its writes —
+ * while the release-failure message was telling the operator saves would keep
+ * failing until they cleared it.
+ *
+ * The discriminator is the lock directory itself rather than an errno
+ * taxonomy: **if it exists, something holds it and we must not proceed**; if
+ * it does not, we genuinely could not create one and degrading is the
+ * documented trade (#1848, #1905). That reads the state the decision is
+ * actually about, instead of enumerating error codes per platform and
+ * filesystem — which is the enumeration that let `ENOTEMPTY` through.
+ */
+function isStuckOrHeld(err: unknown, lockPath: string): boolean {
+  return isHeldElsewhere(err) || identifySync(lockPath) !== null;
+}
+
 /**
  * Did `proper-lockfile` decline because someone else holds the lock?
  *
@@ -266,6 +292,23 @@ function guardedFs(
   };
   return {
     ...nodeFs,
+    // **Identity is captured here, not after `lock()` resolves.** `mkdir` is
+    // the moment the directory becomes ours, and it is synchronous with
+    // respect to this process: recording it in the callback, before yielding,
+    // leaves no window. Capturing after the acquire promise settled — as this
+    // did originally — spans the library's own `utimes`/`stat` probe, and a
+    // waiter replacing our directory inside that span would have us record
+    // *the winner's* identity as our own, after which the guard below would
+    // cheerfully delete their lock.
+    mkdir: (
+      p: string,
+      cb: (err: NodeJS.ErrnoException | null) => void,
+    ): void => {
+      nodeFs.mkdir(p, (err) => {
+        if (!err) owned.id = identifySync(lockPath);
+        cb(err);
+      });
+    },
     // Reported as success when refused: the library's bookkeeping should
     // forget this lock either way. Leaving it registered would hand the
     // `signal-exit` handler a record pointing at the winner's directory.
@@ -286,26 +329,6 @@ function guardedFs(
   };
 }
 
-/**
- * What to tell an operator about a lock we could not remove.
- *
- * The blanket "it expires on its own" is **false for `ENOTEMPTY`**, and that
- * is the reachable case rather than a hypothetical: stale takeover reclaims a
- * lock through the same `rmdir`, which also cannot remove a non-empty
- * directory. So a lock directory with anything inside it is not cleaned up by
- * the staleness mechanism, by us, or by the next writer — it stays until
- * somebody deletes it, and every later save fails `ELOCKED` against it.
- *
- * Any other failure leaves a directory the refresher has stopped touching, so
- * the stale path really does reclaim it after {@link STALE_MS}.
- */
-function releaseAdvice(err: unknown, target: string): string {
-  const code = (err as NodeJS.ErrnoException).code;
-  return code === "ENOTEMPTY"
-    ? `It has something inside it, which stale takeover cannot clear either, so saves will keep failing until you remove ${lockPathOf(target)} by hand.`
-    : `It expires on its own after ${STALE_MS / 1000}s.`;
-}
-
 /**
  * One `proper-lockfile` acquire against `target`, with the given retry policy.
  *
@@ -390,7 +413,7 @@ export async function withSecretFileLock<T>(
       // time, on every save, before degrading. Probing once with no retries
       // separates the two answers at the cost of one syscall: `ELOCKED` is
       // worth waiting out, an infrastructure failure is not.
-      if (!isHeldElsewhere(err)) throw err;
+      if (!isStuckOrHeld(err, lockPathOf(target))) throw err;
       return acquire(target, RETRY, fsShim);
     });
   } catch (err) {
@@ -402,9 +425,9 @@ export async function withSecretFileLock<T>(
     // {@link RETRY}), a holder still there is not one that crashed; it is one
     // that is stuck. Refusing loses nothing — `set` reports it and the user
     // retries — whereas proceeding can lose a secret while reporting success.
-    if (isHeldElsewhere(err)) {
+    if (isStuckOrHeld(err, lockPathOf(target))) {
       throw new SecretStoreUnavailableError(
-        `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.`,
+        `Could not save to the secrets file at ${target}: its lock (${lockPathOf(target)}) was still held after 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 that lock and try again.`,
       );
     }
     // Everything else is the lock being *unavailable* rather than held — a
@@ -417,9 +440,6 @@ export async function withSecretFileLock<T>(
     );
     return fn();
   }
-  // Now that we hold it, record which directory is ours so the guard above
-  // can refuse to delete anyone else's.
-  owned.id = identifySync(lockPathOf(target));
   try {
     return await fn();
   } finally {
@@ -428,10 +448,15 @@ export async function withSecretFileLock<T>(
     } catch (err) {
       // 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, and the two reasons need
-      // different advice — see `releaseAdvice`.
+      // But a lock left behind is worth explaining.
       warnOnce(
-        `Could not release the lock on the secrets file at ${target} (${describeError(err)}). ${releaseAdvice(err, target)}`,
+        // 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.`,
       );
     }
   }