fix(mcp): isolate OAuth ownership from background connects and harden observability

docs/decisions/implemented/bug-fix/2026-08-31-mcp-oauth-ownership-isolation.md

@@ -0,0 +1,40 @@
+# Decision Record: Isolate MCP OAuth ownership and harden observability
+
+Status: implemented
+
+## Problem
+
+`synergy mcp auth <server>` failed with `Authentication failed` / `Authorization cancelled` immediately after opening the browser for any OAuth remote server (reproduced with Notion, 2026-08-31). The CLI process log ended right after `opening browser for oauth` with no callback received and no error line, making the failure unobservable.
+
+Root cause: the MCP supervisor's background auto-connect (default `startup: "eager"`, `MAX_CONCURRENT_STARTS = 3`) shares the process-level `PendingOAuth` registry with the interactive OAuth flow. When a queued background connect for the same server reached 401, `connectPipeline` ran `PendingOAuth.disposeIfIdentity(handle.name, handle.identity, "connection restarted")` or `PendingOAuth.register` (which replaces the existing entry and fires its `onDispose`), and the interactive flow's `onDispose` — `clearPendingOAuthState` — called `McpOAuthCallback.cancelPending`, rejecting the callback wait with `Authorization cancelled`. The CLI then exited via `process.exit()` before the supervisor could write its "requires authentication" log, producing the silent failure.
+
+Additional gaps found along the same ownership line:
+
+1. `McpAuth` kept an in-process cache (`auth.ts`) that was never invalidated in production, so a CLI-authenticated token was invisible to a long-running server process until restart.
+2. Background connects used the interactive `McpOAuthProvider`, whose `saveState`/`saveCodeVerifier`/`saveTokens` wrote into the shared `authMcp` file, overwriting interactive-flow state mid-flight (state-mismatch risk).
+3. `PendingOAuth` entries were created by background 401 probes on every connect attempt, repeatedly re-registering clients and widening the race window.
+4. `cancelPending` rejected silently; CLI auth failures were only printed to the terminal, never logged to file; the fixed callback port 19876 failed with a bare message when already in use.
+
+## Decision
+
+Keep the CLI direct-connect architecture and eliminate the race by separating background probing from interactive ownership:
+
+- **R1 — `McpAuth` reads go to disk every time.** Removed the module cache (and the now-dead `invalidateCache()` shim); `all()` reads `Global.Path.authMcp` directly. CLI-written tokens are visible to the server process on its next read.
+- **R2 — `McpOAuthProvider` gained a `mode: "interactive" | "background"` (default `interactive`).** In background mode, `saveCodeVerifier`, `saveState`, and `saveClientInformation` are no-ops and `codeVerifier()`/`state()` return provider-local memory values, so a background probe never writes PKCE state into the shared `authMcp` file. `saveTokens` is also a no-op while no stored entry exists (probe-only), but persists when the store already has a token entry so SDK refresh-token renewal survives; `tokens()`/`clientInformation()` always read live from disk so an already-authenticated server connects directly.
+- **R3 — supervisor background connects never touch `PendingOAuth`.** Removed the `disposeIfIdentity` call at the top of `connectPipeline`; the 401 branch now closes the failed client and transitions the handle to `NeedsAuth` (with an actionable `lastError`) instead of registering a pending entry. A single `setInterval` (30 s, `unref()`-ed, lazily started when a handle enters `NeedsAuth`, cleared when none remain) checks each `NeedsAuth` handle: reads `McpAuth.getForUrl` locally, reconnects when the stored tokens are valid or expired-but-refreshable (has a refresh token), skips the handle while an interactive `PendingOAuth` entry exists, and never issues network calls without local credentials. `reset()` clears the timer. `checkNeedsAuthNow()` exposes one pass for tests.
+- **R4 — observability.** `cancelPending` logs a WARN with `{ mcpName, reason }` before rejecting; the callback-port-in-use error names `SYNERGY_OAUTH_CALLBACK_PORT` as the escape hatch; the CLI `auth` handler writes `mcp auth failed` to the file log in every failure branch.
+- **R5 — behavioral regression tests** in `test/mcp/oauth.test.ts`: a background 401 during an interactive wait no longer cancels the pending callback and the interactive flow completes end-to-end; background provider never persists state and sees externally written tokens immediately; `NeedsAuth` recovers automatically once credentials appear; callback-port conflict produces an actionable error.
+- **R6 — follow-up fixes from review.** `needs_auth` status now carries an `error` field (mirroring `needs_client_registration`) so the actionable `synergy mcp auth` hint reaches the CLI status line and any status consumer, not just the file log; the SDK/OpenAPI contracts were regenerated to match; the background-provider token-persistence and expired-but-refreshable recovery behaviors above are covered by new tests; the `invalidateCache` no-op shim was removed along with its test call sites.
+
+Behavioral coverage: `test/mcp/oauth.test.ts` (new "MCP OAuth race and recovery" describe) plus the existing `pending-oauth.test.ts`/`supervisor.test.ts` suites staying green unchanged.
+
+## Alternatives considered
+
+- **Server-hosted authentication (CLI delegates OAuth to a running server via `--attach`)** — rejected: changes the CLI behavior contract, depends on a running server, adds a dual-process test matrix, and the fallback path would still need this fix.
+- **Minimal patch: pause background connects for a server while an interactive flow is pending** — rejected: closes one race exit but leaves the stale-cache gap, the cross-process state-file overwrite, the repeated registration probes, and the observability gaps.
+- **Add conditional-replacement/locking semantics to `PendingOAuth`** — rejected: background connections would still own registry entries and write state files; cross-process interference remains.
+- **Database or shared-state service for `authMcp`** — rejected: over-engineering for a small single-file JSON store with no concurrency-volume justification.
+
+## Consequences
+
+`synergy mcp auth <server>` is stable under concurrent background connects; the interactive flow owns `PendingOAuth` exclusively. A server process recovers within ≤30 s after CLI authentication without restart, because reads are live and the NeedsAuth timer reconnects when tokens appear. Unauthenticated servers stop re-registering clients in the background (zero network traffic while `NeedsAuth`). Failures are now visible in both terminal and file logs. The trade-offs: every `McpAuth` read is a disk read (small file, non-hot path — connections/status only); background probes that hit 401 no longer keep the transport alive for a later finishAuth (the interactive flow always creates its own transport, so nothing is lost); the 30 s recovery interval is the worst-case reconnect latency; the callback port remains fixed with a clear conflict error rather than dynamic port selection, keeping `redirect_uris` consistent with server-registered clients.

docs/reference/configuration-layout.md

@@ -281,6 +281,31 @@ Feishu/Lark account configuration may pair an explicit `model` with `variant`. T
 
 A Feishu/Lark account may also set `projectDir` to bind the account's sessions to a project Scope. Resolution rules and error behavior are documented in the [Channels reference](../product/connections.md).
 
+## MCP Server Authentication
+
+Remote MCP servers (`type: "remote"` in `40-mcp.jsonc`) use OAuth 2.0 authorization-code flows with PKCE and dynamic client registration by default. Credentials (tokens, registered client info, and in-flight PKCE state) live in the auth store, never in `40-mcp.jsonc`; the `oauth` object in the server config only carries a pre-registered `clientId`/`clientSecret`/`scope` when a server does not support dynamic registration.
+
+```jsonc
+{
+  "mcp": {
+    "notion": {
+      "type": "remote",
+      "url": "https://mcp.notion.com/mcp",
+      "oauth": {},
+      "startup": "eager",
+    },
+  },
+}
+```
+
+Authenticate a server with `synergy mcp auth <name>` (or `synergy mcp auth` to pick from a list). The command opens the provider's authorization page in a browser, waits for the local callback, exchanges the code for tokens, and saves them to the auth store. A long-running server process picks up CLI-authenticated credentials automatically within ~30 seconds (the supervisor re-checks servers in the `needs_auth` state on an interval and reconnects when valid tokens appear), so no restart is required after authenticating from the CLI.
+
+Background supervision never writes PKCE state: an unauthenticated server is marked `needs_auth` (with an actionable `synergy mcp auth <name>` error on the status) and left idle (no network probes) until credentials exist. Once credentials exist, the supervisor reconnects within ~30 seconds even if the stored access token already expired, as long as a refresh token is present — refreshed tokens are persisted back to the auth store. This keeps background auto-connect from interfering with an interactive `mcp auth` flow for the same server.
+
+`startup` controls when a server connects: `"eager"` (default) connects at runtime start, `"lazy"` connects on first tool use, and `"manual"` never auto-connects (use `synergy mcp connect <name>`). Set `"manual"` for a server you only authenticate on demand.
+
+The OAuth callback listener runs on `127.0.0.1:19876` by default. If another Synergy process is already running an OAuth flow on that port, `mcp auth` fails with an actionable error; set `SYNERGY_OAUTH_CALLBACK_PORT` to a free port to run a flow in the second process.
+
 ## Feishu/Lark Channel Settings
 
 `90-channels.jsonc` owns the built-in Feishu/Lark Channel provider under `channel.feishu`. A minimal configuration is:

packages/app/test/components/session/session-connection-stats.test.ts

@@ -59,6 +59,8 @@ function mcp(status: McpStatus["status"]): McpStatus {
       return { status, attempt: 1, maxAttempts: 3 }
     case "needs_client_registration":
       return { status, error: "register" }
+    case "needs_auth":
+      return { status, error: "auth" }
     default:
       return { status }
   }

packages/sdk/js/src/gen/types.gen.ts

@@ -4493,6 +4493,7 @@ export type McpStatusDisabled = {
 
 export type McpStatusNeedsAuth = {
   status: "needs_auth"
+  error: string
 }
 
 export type McpStatusNeedsClientRegistration = {

packages/sdk/openapi.json

@@ -35519,9 +35519,12 @@
           "status": {
             "type": "string",
             "const": "needs_auth"
+          },
+          "error": {
+            "type": "string"
           }
         },
-        "required": ["status"]
+        "required": ["status", "error"]
       },
       "MCPStatusNeedsClientRegistration": {
         "type": "object",

packages/synergy/src/cli/cmd/mcp.ts

@@ -1,4 +1,5 @@
 import { formatLocalDateTime } from "../../util/time-format"
+import { Log } from "../../util/log"
 import { cmd } from "./cmd"
 import { Client } from "@modelcontextprotocol/sdk/client/index.js"
 import { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js"
@@ -97,6 +98,7 @@ export const McpListCommand = cmd({
           } else if (status.status === "needs_auth") {
             statusIcon = "⚠"
             statusText = "needs authentication"
+            hint = "\n    " + status.error
           } else if (status.status === "needs_client_registration") {
             statusIcon = "✗"
             statusText = "needs client registration"
@@ -229,6 +231,7 @@ export const McpAuthCommand = cmd({
             spinner.stop("Authentication successful!")
           } else if (status.status === "needs_client_registration") {
             spinner.stop("Authentication failed", 1)
+            Log.Default.error("mcp auth failed: needs client registration", { serverName, error: status.error })
             prompts.log.error(status.error)
             prompts.log.info("Add clientId to your MCP server config:")
             prompts.log.info(`
@@ -244,12 +247,17 @@ export const McpAuthCommand = cmd({
   }`)
           } else if (status.status === "failed") {
             spinner.stop("Authentication failed", 1)
+            Log.Default.error("mcp auth failed", { serverName, error: status.error })
             prompts.log.error(status.error)
           } else {
             spinner.stop("Unexpected status: " + status.status, 1)
           }
         } catch (error) {
           spinner.stop("Authentication failed", 1)
+          Log.Default.error("mcp auth failed", {
+            serverName,
+            error: error instanceof Error ? error.message : String(error),
+          })
           prompts.log.error(error instanceof Error ? error.message : String(error))
         }
 

packages/synergy/src/mcp/auth.ts

@@ -32,7 +32,6 @@ export namespace McpAuth {
     isCurrent?: () => boolean
   }
 
-  let cache: { filepath: string; data: Record<string, Entry> } | undefined
   let mutation: Promise<void> = Promise.resolve()
 
   function serialize<T>(fn: () => Promise<T>): Promise<T> {
@@ -61,16 +60,9 @@ export namespace McpAuth {
     })
   }
 
-  export function invalidateCache() {
-    cache = undefined
-  }
-
   export async function all(): Promise<Record<string, Entry>> {
-    const filepath = Global.Path.authMcp
-    if (cache?.filepath === filepath) return cache.data
-    const file = Bun.file(filepath)
+    const file = Bun.file(Global.Path.authMcp)
     const data = (await file.json().catch(() => ({}))) as Record<string, Entry>
-    cache = { filepath, data }
     return data
   }
 

packages/synergy/src/mcp/oauth-callback.ts

@@ -159,7 +159,9 @@ export namespace McpOAuthCallback {
     const port = getOAuthCallbackPort()
     const portFreed = await waitForPortInUse(false)
     if (!portFreed) {
-      throw new Error(`OAuth callback port ${port} is already in use`)
+      throw new Error(
+        `OAuth callback port ${port} is already in use — another Synergy process is running an OAuth flow. Complete it there or set SYNERGY_OAUTH_CALLBACK_PORT to a free port.`,
+      )
     }
 
     server = Bun.serve({
@@ -171,7 +173,7 @@ export namespace McpOAuthCallback {
   }
 
   export function waitForCallback(oauthState: string, mcpName = oauthState): Promise<string> {
-    cancelPending(mcpName)
+    cancelPending(mcpName, undefined, "superseded by a new OAuth flow")
     return new Promise((resolve, reject) => {
       const timeout = setTimeout(() => {
         const pending = removePending(oauthState)
@@ -184,11 +186,14 @@ export namespace McpOAuthCallback {
     })
   }
 
-  export function cancelPending(mcpName: string, expectedState?: string): void {
+  export function cancelPending(mcpName: string, expectedState?: string, reason = "cancelled"): void {
     const oauthState = stateByMcpName.get(mcpName)
     if (!oauthState || (expectedState !== undefined && oauthState !== expectedState)) return
     const pending = removePending(oauthState)
-    pending?.reject(new Error("Authorization cancelled"))
+    if (pending) {
+      log.warn("oauth callback cancelled", { mcpName, reason })
+      pending.reject(new Error("Authorization cancelled"))
+    }
   }
 
   export async function isPortInUse(): Promise<boolean> {

packages/synergy/src/mcp/oauth-provider.ts

@@ -25,6 +25,8 @@ function getOAuthCallbackPort(): number {
   return port
 }
 
+export type McpOAuthMode = "interactive" | "background"
+
 export interface McpOAuthConfig {
   clientId?: string
   clientSecret?: string
@@ -41,6 +43,9 @@ export interface McpOAuthCallbacks {
 // Local adaptation: implements the SDK's OAuthClientProvider over Synergy-owned token storage;
 // public client by default (token_endpoint_auth_method "none") unless a client secret is configured.
 export class McpOAuthProvider implements OAuthClientProvider {
+  private memoryCodeVerifier: string | undefined
+  private memoryState: string | undefined
+
   private get mutationOptions(): McpAuth.MutationOptions {
     return { isCurrent: this.callbacks.isCurrent }
   }
@@ -49,6 +54,7 @@ export class McpOAuthProvider implements OAuthClientProvider {
     private serverUrl: string,
     private config: McpOAuthConfig,
     private callbacks: McpOAuthCallbacks,
+    private mode: McpOAuthMode = "interactive",
   ) {}
 
   get redirectUrl(): string {
@@ -96,6 +102,7 @@ export class McpOAuthProvider implements OAuthClientProvider {
   }
 
   async saveClientInformation(info: OAuthClientInformationFull): Promise<void> {
+    if (this.mode === "background") return
     await McpAuth.updateClientInfo(
       this.mcpName,
       {
@@ -130,6 +137,12 @@ export class McpOAuthProvider implements OAuthClientProvider {
   }
 
   async saveTokens(tokens: OAuthTokens): Promise<void> {
+    if (this.mode === "background") {
+      // Persist refreshed tokens so later requests send the new access token.
+      // Probe-only connects (no stored entry) still never write shared state.
+      const existing = await McpAuth.getForUrl(this.mcpName, this.serverUrl)
+      if (!existing) return
+    }
     await McpAuth.updateTokens(
       this.mcpName,
       {
@@ -154,10 +167,20 @@ export class McpOAuthProvider implements OAuthClientProvider {
   }
 
   async saveCodeVerifier(codeVerifier: string): Promise<void> {
+    if (this.mode === "background") {
+      this.memoryCodeVerifier = codeVerifier
+      return
+    }
     await McpAuth.updateCodeVerifier(this.mcpName, codeVerifier, this.mutationOptions)
   }
 
   async codeVerifier(): Promise<string> {
+    if (this.mode === "background") {
+      if (!this.memoryCodeVerifier) {
+        throw new Error(`No code verifier saved for MCP server: ${this.mcpName}`)
+      }
+      return this.memoryCodeVerifier
+    }
     const entry = await McpAuth.get(this.mcpName)
     if (!entry?.codeVerifier) {
       throw new Error(`No code verifier saved for MCP server: ${this.mcpName}`)
@@ -166,10 +189,18 @@ export class McpOAuthProvider implements OAuthClientProvider {
   }
 
   async saveState(state: string): Promise<void> {
+    if (this.mode === "background") {
+      this.memoryState = state
+      return
+    }
     await McpAuth.updateOAuthState(this.mcpName, state, this.mutationOptions)
   }
 
   async state(): Promise<string> {
+    if (this.mode === "background") {
+      if (!this.memoryState) this.memoryState = crypto.randomUUID()
+      return this.memoryState
+    }
     const entry = await McpAuth.get(this.mcpName)
     if (entry?.oauthState) return entry.oauthState
     const state = crypto.randomUUID()