fix(servers): close GET migration race; reverse keychain/disk order (#1356)

Addresses code review:

1. **Concurrency**: GET /api/servers was reading + migrating outside the
   write lock, then writing inside it — a concurrent POST/PUT/DELETE
   between the read and the lock acquisition could be clobbered by the
   migration write. Also affected the first-launch seed write. Fix:
   take a fast unlocked peek; if the file is absent (seed branch) or
   carries plaintext to migrate (migration branch), re-read inside the
   write lock and decide there. Added `hasPlaintextSecrets()` as a
   pure predicate so the common no-migration path stays lock-free.

2. **Write ordering**: POST and PUT wrote disk first, then keychain.
   A keychain `set` failure mid-write left a half-configured entry on
   disk and trapped retries at 409. Reversed to keychain-first so a
   503 leaves both stores in their pre-write state. PUT in-place now
   sets new fields, writes disk, then deletes obsolete fields (a
   failed disk write leaves orphan keychain entries — recoverable —
   rather than premature deletions of fields the user still expects
   to see).

3. **Tolerance simplification**: dropped the no-entry regex in
   `KeyringSecretStore.delete` — both "no entry" and "keychain
   unavailable" collapse to the same desired outcome (the entry isn't
   there anymore), so a uniform swallow is clearer.

4. **Parallelism**: `readKeychainEntriesFor` now `Promise.all`s the
   per-field `get` calls. macOS Keychain round-trips are 10-50ms;
   serializing 5+ env vars per server × N servers is a meaningful
   wall-clock cost on GET.

5. **Spec doc**: reflects the tolerance contract (only `set`
   hard-fails) and the keychain-before-disk write ordering rationale.

New regression test in servers-route.test.ts covers the retry-after-503
contract directly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: cliffhall

clients/web/src/test/integration/mcp/remote/servers-route.test.ts

@@ -1479,6 +1479,54 @@ describe("/api/servers routes", () => {
       }
     });
 
+    it("POST 503 leaves no disk entry — retry is not trapped at 409", async () => {
+      // Regression for the write-ordering review comment. If keychain
+      // set ran AFTER the disk write, a failed `set` would leave the
+      // entry on disk and the user's retry would hit 409. With the
+      // reversed order, a failed `set` leaves disk untouched and the
+      // retry can proceed.
+      const u = await startUnavailableHarness();
+      try {
+        const first = await fetch(`${u.baseUrl}/api/servers`, {
+          method: "POST",
+          headers: { "Content-Type": "application/json" },
+          body: JSON.stringify({
+            id: "retry-test",
+            config: { type: "streamable-http", url: "https://x.test/mcp" },
+            settings: {
+              headers: [],
+              metadata: [],
+              connectionTimeout: 0,
+              requestTimeout: 0,
+              oauthClientId: "cid",
+              oauthClientSecret: "shh",
+            },
+          }),
+        });
+        expect(first.status).toBe(503);
+
+        // Disk: no entry persisted. A GET should not see "retry-test".
+        const cfg = existsSync(u.configPath)
+          ? (JSON.parse(readFileSync(u.configPath, "utf-8")) as MCPConfig)
+          : { mcpServers: {} };
+        expect(cfg.mcpServers).not.toHaveProperty("retry-test");
+
+        // Retry with no secret should now succeed (no 409).
+        const second = await fetch(`${u.baseUrl}/api/servers`, {
+          method: "POST",
+          headers: { "Content-Type": "application/json" },
+          body: JSON.stringify({
+            id: "retry-test",
+            config: { type: "stdio", command: "node" },
+          }),
+        });
+        expect(second.status).toBe(200);
+      } finally {
+        await new Promise<void>((r) => u.server.close(() => r()));
+        rmSync(u.tempDir, { recursive: true });
+      }
+    });
+
     it("DELETE succeeds (sweep silently no-ops)", async () => {
       const u = await startUnavailableHarness();
       try {

core/auth/node/secret-store.ts

@@ -121,12 +121,14 @@ export class KeyringSecretStore implements SecretStore {
     const entry = new AsyncEntry(SERVICE_NAME, buildAccount(serverId, field));
     try {
       await entry.deleteCredential();
-    } catch (err) {
-      // `deleteCredential` throws NoEntry for missing entries — treat as success.
-      const msg = err instanceof Error ? err.message : String(err);
-      if (/no entry|no matching|not found/i.test(msg)) return;
-      // Keychain unavailable → silently no-op. We couldn't have written
-      // anything either, so there's nothing to clean up.
+    } catch {
+      // Both reasons for a throw collapse to the same desired outcome
+      // ("the entry isn't there anymore"): `deleteCredential` raises
+      // NoEntry for a missing credential, and the native binding
+      // raises a runtime error when the keychain itself is unavailable.
+      // We treat both as success — there's no value to lose either
+      // way, and `set` is the operation that hard-fails when the
+      // keychain is actually down.
     }
   }
 

core/mcp/remote/node/server.ts

@@ -1162,18 +1162,38 @@ export function createRemoteApp(
   // stripped shape into the shape today's browser code expects. Missing
   // fields are simply absent from the result (so `mergeSecretsIntoStored`
   // leaves the disk value untouched).
+  // Issue many keychain reads in parallel. On macOS each round-trip to
+  // Keychain Services is 10-50ms; a server with 20 entries × 5 env vars
+  // each would otherwise serialize to ~5s of rehydration on every GET.
+  // `@napi-rs/keyring`'s async APIs release the event loop, so
+  // Promise.all is a real win, not just stylistic.
   const readKeychainEntriesFor = async (
     id: string,
     fields: string[],
   ): Promise<Record<string, string>> => {
+    const values = await Promise.all(
+      fields.map(async (field) => [field, await secretStore.get(id, field)] as const),
+    );
     const out: Record<string, string> = {};
-    for (const field of fields) {
-      const v = await secretStore.get(id, field);
+    for (const [field, v] of values) {
       if (v !== null) out[field] = v;
     }
     return out;
   };
 
+  // Cheap structural check used by the GET handler to decide whether
+  // to enter the write lock for migration. Pure — no keychain or disk
+  // access. The actual migration (`migratePlaintextSecrets`) writes
+  // to the keychain, so we must not run it speculatively outside the
+  // lock; this predicate lets us keep the fast path lock-free.
+  const hasPlaintextSecrets = (config: MCPConfig): boolean => {
+    for (const stored of Object.values(config.mcpServers)) {
+      const { secrets } = extractSecretsFromStored(stored);
+      if (Object.keys(secrets).length > 0) return true;
+    }
+    return false;
+  };
+
   // Migrate plaintext secrets in a freshly-read mcp.json into the
   // keychain. Idempotent: when the keychain already has a value for
   // `(id, field)`, that value wins and the disk plaintext is dropped
@@ -1238,24 +1258,31 @@ export function createRemoteApp(
     return out;
   };
 
-  // Reconcile keychain state for a single server on the write path:
-  // delete any field the prior entry held but the new entry doesn't
-  // (e.g. an env key the user just removed), then write the new
-  // secrets. Order matters — we delete obsolete first so that a
-  // pathological case where a field is "renamed" (delete + add a
-  // different one) doesn't leave the old value behind.
-  const reconcileKeychainEntry = async (
-    id: string,
+  // Fields the previous entry held that the new entry doesn't —
+  // candidates for deletion after the disk write succeeds. Returned
+  // as an array so the caller can decide where in the write sequence
+  // to perform the destructive operation (we want it after the disk
+  // write so a failed disk write doesn't leave the user with their
+  // old config on disk but missing keychain entries).
+  const computeObsoleteFields = (
     previousFields: Set<string>,
     nextSecrets: Record<string, string>,
-  ): Promise<void> => {
+  ): string[] => {
     const nextFieldSet = new Set(Object.keys(nextSecrets));
+    const obsolete: string[] = [];
     for (const field of previousFields) {
-      if (!nextFieldSet.has(field)) {
-        await secretStore.delete(id, field);
-      }
+      if (!nextFieldSet.has(field)) obsolete.push(field);
+    }
+    return obsolete;
+  };
+
+  const deleteKeychainFields = async (
+    id: string,
+    fields: string[],
+  ): Promise<void> => {
+    for (const field of fields) {
+      await secretStore.delete(id, field);
     }
-    await writeKeychainEntriesFor(id, nextSecrets);
   };
 
   const keychainErrorResponse = (
@@ -1270,33 +1297,55 @@ export function createRemoteApp(
 
   app.get("/api/servers", async (c) => {
     try {
+      // Fast path: peek at the file without taking the write lock. Most
+      // GETs land here — file exists, no plaintext to migrate. The
+      // unlocked read is safe because we don't write anything in this
+      // branch; a concurrent mutation just means the response is a
+      // snapshot from a moment ago.
       const raw = await readStoreFile(mcpConfigPath);
-      let onDisk: MCPConfig;
-      if (raw === null) {
-        // First-launch seed. The seed has no secrets so no migration to
-        // run; persist verbatim and respond. We rehydrate anyway so the
-        // GET path stays uniform (no-op on a seed without env values).
-        await writeMcpAndTrackMtime(serializeStore(DEFAULT_SEED_CONFIG));
-        onDisk = DEFAULT_SEED_CONFIG;
-      } else {
+      if (raw !== null) {
         const parsed = parseStore(raw) as { mcpServers?: unknown } | null;
-        onDisk = { mcpServers: normalizeMcpServers(parsed?.mcpServers) };
+        const onDisk: MCPConfig = {
+          mcpServers: normalizeMcpServers(parsed?.mcpServers),
+        };
+        if (!hasPlaintextSecrets(onDisk)) {
+          return c.json(await rehydrateConfig(onDisk));
+        }
       }
 
-      // Migrate plaintext secrets that may have been written by an
-      // older Inspector build or hand-edited into mcp.json. Idempotent:
-      // on a clean file with no plaintext secrets it's a no-op. The
-      // rewrite happens inside the same write lock as POST/PUT/DELETE
-      // so concurrent mutations can't race with our cleanup.
-      const { migrated, changed } = await migratePlaintextSecrets(onDisk);
-      if (changed) {
-        await withWriteLock(async () => {
+      // Slow path: either the file is missing (first-launch seed) or it
+      // contains plaintext we need to migrate. Both branches write the
+      // file, so the entire read + decide + write sequence must happen
+      // inside the write lock — otherwise a concurrent POST/PUT/DELETE
+      // could land between our unlocked read and our locked write, and
+      // we'd clobber it. Re-read once we hold the lock so the decision
+      // is based on the same snapshot we're about to mutate.
+      const settled = await withWriteLock(async () => {
+        const rawInside = await readStoreFile(mcpConfigPath);
+        if (rawInside === null) {
+          // Still absent after taking the lock → no concurrent POST
+          // beat us to it; seed the file ourselves. The seed has no
+          // secrets so nothing else to do.
+          await writeMcpAndTrackMtime(serializeStore(DEFAULT_SEED_CONFIG));
+          return DEFAULT_SEED_CONFIG;
+        }
+        const parsedInside = parseStore(rawInside) as
+          | { mcpServers?: unknown }
+          | null;
+        const inside: MCPConfig = {
+          mcpServers: normalizeMcpServers(parsedInside?.mcpServers),
+        };
+        // A concurrent POST may have run between the unlocked peek and
+        // the lock acquisition — if the file now has no plaintext to
+        // migrate, leave it alone and return the latest snapshot.
+        if (!hasPlaintextSecrets(inside)) return inside;
+        const { migrated, changed } = await migratePlaintextSecrets(inside);
+        if (changed) {
           await writeMcpAndTrackMtime(serializeStore(migrated));
-        });
-      }
-
-      const rehydrated = await rehydrateConfig(migrated);
-      return c.json(rehydrated);
+        }
+        return migrated;
+      });
+      return c.json(await rehydrateConfig(settled));
     } catch (error) {
       const keychainResp = keychainErrorResponse(c, error);
       if (keychainResp) return keychainResp;
@@ -1346,15 +1395,19 @@ export function createRemoteApp(
         }
         const built = buildStoredEntry(id, body.config, postSettings);
         // Split secret values out of the new entry — the stripped shape
-        // goes to disk, the values go to the keychain. We sweep any
-        // leftover keychain entries for this id first to handle the
-        // edge case where a previous DELETE failed midway and left
-        // orphans under the same id the user is now reusing.
+        // goes to disk, the values go to the keychain.
         const { stripped, secrets } = extractSecretsFromStored(built);
+        // Order: sweep → keychain → disk. The keychain write is the
+        // only step that can hard-fail (KeychainUnavailableError on
+        // `set`); doing it before the disk write means a 503 leaves no
+        // disk entry behind, so a retry POST isn't trapped at 409. The
+        // initial sweep handles the case where a previous DELETE failed
+        // midway and left orphans under the same id the user is now
+        // reusing; it's a silent no-op when the keychain is unavailable.
         await secretStore.deleteAllForServer(id);
+        await writeKeychainEntriesFor(id, secrets);
         current.mcpServers[id] = stripped;
         await writeMcpAndTrackMtime(serializeStore(current));
-        await writeKeychainEntriesFor(id, secrets);
         return c.json({ ok: true });
       });
     } catch (error) {
@@ -1486,17 +1539,35 @@ export function createRemoteApp(
             next.mcpServers[key] = val;
           }
         }
-        await writeMcpAndTrackMtime(serializeStore(next));
-        // Keychain reconcile. On rename, every field under the old id
-        // is obsolete by definition — sweep then write under the new
-        // id. Otherwise diff against the previous field set so we only
-        // touch entries that actually need to change.
+        // Ordering: write the new keychain entries first, then the
+        // disk file, then clean up obsolete keychain entries.
+        //
+        // - Keychain set is the only hard-fail step (it raises 503 on
+        //   `KeychainUnavailableError`). Doing it first means a failed
+        //   set leaves both disk and keychain in their pre-PUT state;
+        //   the user retries and nothing is half-applied.
+        // - The disk write happens after the keychain is fully primed,
+        //   so a successful disk write is also a fully-consistent end
+        //   state.
+        // - Obsolete deletion comes last because it's destructive: if
+        //   we deleted first and then the disk write failed, the user
+        //   would still see the old config on disk but with missing
+        //   keychain values. With the current order a failed disk
+        //   write leaves orphan keychain entries — recoverable on the
+        //   next reconcile or `deleteAllForServer` sweep.
         if (newId !== originalId) {
-          await secretStore.deleteAllForServer(originalId);
           await writeKeychainEntriesFor(newId, secrets);
+          await writeMcpAndTrackMtime(serializeStore(next));
+          await secretStore.deleteAllForServer(originalId);
         } else {
+          // In-place update: same id, possibly different fields. Set
+          // the new values first, then write disk, then drop obsolete
+          // fields (env keys the user removed, OAuth secret cleared).
           const previousFields = new Set(expectedSecretFields(existing));
-          await reconcileKeychainEntry(newId, previousFields, secrets);
+          const obsolete = computeObsoleteFields(previousFields, secrets);
+          await writeKeychainEntriesFor(newId, secrets);
+          await writeMcpAndTrackMtime(serializeStore(next));
+          await deleteKeychainFields(newId, obsolete);
         }
         return c.json({ ok: true });
       });

specification/v2_servers_file.md

@@ -245,7 +245,9 @@ Each server entry may carry these Inspector-extension fields at the top level:
 - **First-connect contract**: settings apply on the *first* outbound request after the entry loads from disk — no need to open the settings form. The browser sends `settings` to the backend in the `/api/mcp/connect` body; the backend reads it from `RemoteConnectRequest` and threads it into `createTransportNode`.
 - **Secret storage (#1356)**: `oauth.clientSecret` and stdio `env` values are persisted in the OS keychain (macOS Keychain Services / Windows Credential Manager / Linux libsecret via `@napi-rs/keyring`), keyed by `(serverId, field)` under the service name `mcp-inspector`. Field names: `oauth-client-secret`, `env:<KEY>` (one per stdio env variable). The on-disk `mcp.json` is stripped of these values — `oauth.clientSecret` is omitted entirely, stdio env keys are preserved with empty-string placeholders (`"env": { "API_KEY": "" }`) so the file still documents the env interface the server expects. The wire shape returned by `GET /api/servers` is unchanged from before #1356: the handler rehydrates values from the keychain so browser code sees the same JSON it has always seen. The keychain interactions live in `core/auth/node/secret-store.ts` behind a `SecretStore` interface; `KeyringSecretStore` is the production impl and `InMemorySecretStore` is the test double the integration suite injects via `RemoteServerOptions.secretStore`.
   - **Migration**: on every `GET /api/servers`, the handler walks the freshly-read config and, for any entry that still carries plaintext secrets (older Inspector builds, hand-edited files, files imported from another tool), lifts each value into the keychain and rewrites the file with the stripped shape. The migration is idempotent — when the keychain already holds a value for `(serverId, field)`, the keychain wins and the disk plaintext is dropped unread. After the rewrite the disk file no longer contains the secret material.
-  - **Linux without libsecret**: `@napi-rs/keyring` returns a hard error on any operation. The handlers translate that to a `503` response with a message pointing at libsecret/gnome-keyring install. macOS and Windows always have a working keychain so this only realistically fires on minimal Linux installs.
+  - **Linux without libsecret**: `KeyringSecretStore` is *tolerant* — only the `set` operation throws `KeychainUnavailableError` (translated to a `503` by the handlers); `get` returns `null` and the destructive operations silently no-op. The result is that no-secret flows (creating a stdio server with no env values, deleting an entry, reading the list, the defensive sweep on POST) all work normally on a minimal Linux box without libsecret. Only the moments where a secret would actually be lost — saving an OAuth client secret, saving a stdio env value, or migrating a plaintext value into the keychain — surface a clear error. macOS and Windows always have a working keychain so this only matters on minimal Linux installs.
+  - **Migration tolerance**: when migration encounters `KeychainUnavailableError`, the GET handler logs a warning, leaves the on-disk plaintext untouched, and serves the (still-plaintext) response. Subsequent reads retry — installing libsecret later lifts the secrets on the next GET without any user action.
+  - **Write ordering on POST/PUT**: keychain writes happen before the disk write, and obsolete-field deletions happen after. The intent is that a `set` failure (the only hard-fail path) leaves both stores in their pre-write state — no half-applied entry on disk that would trap a retry POST at `409`, and no premature deletion of an obsolete field whose disk write later fails.
   - **Out of scope for this PR**: the OAuth handshake itself still runs in the browser via the MCP SDK, so during the token exchange the secret transits the wire (browser → MCP SDK → OAuth provider's token endpoint). The on-disk win this PR delivers is that the secret is no longer in the shareable / symlinked `mcp.json` and is no longer the source-of-truth on the filesystem. Moving the token exchange to the Node side is tracked separately.
 - **Hard-cutover legacy behavior (per #1358 decision 4)**: files written by the one pre-#1358 build of v2/main have a nested `settings` block. `normalizeMcpServers` drops the node on read and logs a one-line warn including the server id; the persisted headers / metadata / timeouts / OAuth credentials are intentionally lost on first read. Users re-enter them via the settings form (or hand-edit the file into the flat shape). v2 has not shipped a stable release with the nested shape, so the blast radius is the small set of v2/main dogfooders who edited per-server settings between #1353 merging and this change.
 - **UI**: `ServerSettingsModal` is opened from the server card's settings affordance. Saving routes through `useServers.updateServerSettings(id, settings)` which issues a settings-only `PUT /api/servers/:id` with `{ id, settings }` — the route preserves the on-disk transport config inside its write lock. Conversely, `useServers.updateServer` (driven by the basic-config modal) issues a config-only PUT with `{ id, config }` and the route preserves the on-disk settings fields. Edits in either modal cannot silently wipe the other half.