Skip to content

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

Merged
yzxoi merged 4 commits into
devfrom
synergy/session-fa950b
Aug 31, 2026
Merged

fix(mcp): isolate OAuth ownership from background connects and harden observability#1292
yzxoi merged 4 commits into
devfrom
synergy/session-fa950b

Conversation

@EricSanchezok

Copy link
Copy Markdown
Contributor

Summary

Fixes synergy mcp auth <server> failing with Authentication failed / Authorization cancelled immediately after opening the browser for OAuth remote servers (reproduced with Notion).

Root cause: background supervisor auto-connects (default startup: "eager") shared the process-level PendingOAuth registry with interactive OAuth flows. A queued background connect for the same server reaching 401 disposed the interactive pending entry, which rejected the callback wait (cancelPending → "Authorization cancelled"). The CLI then exited before the supervisor logged anything, making the failure silent.

Changes

  • McpAuth — removed the never-invalidated in-process cache; reads go to disk every time, so CLI-authenticated tokens are visible to a running server without restart.
  • McpOAuthProvider — added background mode; background probe connects no longer write PKCE state/tokens into the shared auth store.
  • supervisor — background 401s no longer touch PendingOAuth; servers enter NeedsAuth and a 30s local check reconnects once credentials exist (zero network probes while unauthenticated, interactive flow skipped while pending).
  • ObservabilitycancelPending logs WARN with reason; callback-port conflicts name SYNERGY_OAUTH_CALLBACK_PORT; CLI auth failures write to the file log.
  • Tests — race regression (background 401 during interactive wait does not cancel the pending callback), live token visibility, NeedsAuth auto-recovery, port-conflict error.

Verification

cd packages/synergy
bun test test/mcp/            # 107 pass (was 103, +4 new)
bun run quality:quick         # 15/15 gates pass
bun run decision:check        # pass

E2E (isolated home): mcp auth with the callback port occupied prints the actionable error and writes mcp auth failed to the file log.

Notes

  • No config schema, route/OpenAPI, SDK contract, or authMcp format changes.
  • New decision record: docs/decisions/implemented/bug-fix/2026-08-31-mcp-oauth-ownership-isolation.md.

EricSanchezok and others added 2 commits August 31, 2026 18:16
… observability

Background supervisor auto-connects shared the PendingOAuth registry with
interactive `mcp auth` flows: a queued 401 connect for the same server
disposed the interactive pending entry, rejecting the callback wait with
"Authorization cancelled" and failing authentication with no log trail.

- McpAuth: drop the never-invalidated in-process cache so CLI-written
  tokens are visible to a running server without restart.
- McpOAuthProvider: add background mode; probe connects no longer write
  PKCE state or tokens into the shared auth store.
- supervisor: background 401s never register PendingOAuth entries; servers
  enter NeedsAuth and a 30s local check reconnects once credentials exist,
  with zero network probes while unauthenticated.
- observability: cancelPending logs WARN with reason, callback port
  conflicts name SYNERGY_OAUTH_CALLBACK_PORT, CLI auth failures write to
  the file log.
- tests: race regression (background 401 during interactive wait), live
  token visibility, NeedsAuth auto-recovery, port-conflict error.

Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com>
The supervisor's background connect path now transitions a server to
needs_auth without creating a PendingOAuth entry; only the interactive
startAuth flow registers one. Update the declarative plugin OAuth
integration assertion to match.

Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com>
@synergy-agent

synergy-agent Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Verification is complete. Here is my review.

Review: #1292 — fix(mcp): isolate OAuth ownership from background connects and harden observability

Summary

The fix is sound and the reported race is genuinely closed: background connects no longer touch PendingOAuth, so a queued 401 probe can't cancel an interactive wait. The new tests exercise the exact failure mode end-to-end. I found no blocking issues, but there is one real functional regression worth addressing (refresh-token renewal is broken in background mode), plus two minor nits.

Verification (run in this checkout)

  • bun test test/mcp/107 pass, 0 fail (matches the PR claim; all 4 new "MCP OAuth race and recovery" tests pass)
  • bun run quality:quick — 14/15 gates pass; secrets:check fails only because gitleaks isn't installed in this environment (not a code issue)
  • bun run decision:check — pass; tsgo --noEmit — clean
  • Confirmed against the pinned SDK (@modelcontextprotocol/sdk@1.29.0): the provider-method ordering the background mode relies on (state()saveCodeVerifierredirectToAuthorization; codeVerifier() only read later in fetchToken) holds, so the memory-only verifier/state in oauth-provider.ts:161-200 is safe
  • docs/reference/configuration-layout.md is hand-maintained (not generated), and the new section is accurate (40-mcp.jsonc per config/domain.ts:78, startup default "eager" per config/config.ts:168, port 19876 per oauth-provider.ts:13)

Findings

P2 — Background mode breaks refresh-token renewal for expired-but-refreshable tokens (regression vs. pre-PR behavior).

  • packages/synergy/src/mcp/oauth-provider.ts:136saveTokens is a no-op in background mode. When an already-authenticated server's access token expires, the SDK's auth() refreshes via refresh_token and calls saveTokens(newTokens) (SDK client/auth.js:272-285), which now discards the refreshed token.
  • The transport then rebuilds headers from provider.tokens() — a fresh disk read (streamableHttp.js:61) — so the next request still sends the stale token, gets 401, and hits the _hasCompletedAuthFlow guard (streamableHttp.js:322-329), which throws. The connection fails.
  • The new recovery gate then excludes the server permanently: supervisor.ts:842 only schedules a reconnect when !expiresAt || expiresAt > now, so an expired entry never retries. End state: needs_auth until the user manually re-runs mcp auth.

Pre-PR, background connects used the default interactive provider and persisted refreshed tokens, so this scenario self-healed. The new suite doesn't cover it (the fixture returns expires_in: 3600 with no refresh_token).

Smallest fix: in background mode, persist saveTokens when the store already has a token entry for this server (probe-only no-op when no entry exists), and let checkNeedsAuthHandles treat expired-but-refreshable entries as reconnectable. That preserves the race fix while restoring renewal.

P3 — handle.lastError set on NeedsAuth is never surfaced. supervisor.ts:940 writes "Server requires OAuth authentication. Run: synergy mcp auth <name>", but MCPStatusNeedsAuth has no error field (supervisor.ts:119, mapStatus at 360-361), unlike needs_client_registration. The actionable hint only reaches the file log. Either surface it (contract change, which the PR says it avoided) or drop the assignment.

P3 — McpAuth.invalidateCache() is now a dead no-op kept only for test call sites (test/mcp/auth.test.ts:13, test/mcp/oauth.test.ts:57,71,698,711, test/plugin/mcp-declarative-oauth.test.ts:119,289). Harmless, but dropping the test calls and the shim would be cleaner than preserving a cache-shaped API that no longer caches.

Notes

  • The CLI observability fix works: clack's spinner.stop(msg, 1) renders and sets exit state without a synchronous process.exit (0 occurrences in the package), so the new Log.Default.error calls in cli/cmd/mcp.ts:233,249,256 do reach the file log.
  • Environment note: bun install regenerated packages/sdk/js/src/gen/types.gen.ts (unrelated drift on dev); I restored it, so the tree is clean and the PR diff is untouched.

Synergy: review complete — 0 blocking, 1 should-fix (background refresh renewal), 2 suggestions

yzxoi and others added 2 commits August 31, 2026 19:25
…auth error

Address review findings on PR #1292:
- Background saveTokens now persists when a stored entry exists, so SDK
  refresh-token renewal survives instead of being discarded (probe-only
  connects still never write shared state).
- NeedsAuth recovery treats expired-but-refreshable entries as reconnectable,
  restoring self-healing for long-running daemons.
- needs_auth status now carries the actionable error (mirrors
  needs_client_registration); SDK/OpenAPI regenerated to match.
- Remove the dead invalidateCache no-op shim and its test call sites.
- Fixture supports refresh_token grant; new regression tests cover token
  persistence and expired-refreshable recovery.
- Declarative plugin test now asserts background 401 no longer creates a
  PendingOAuth entry (was stale pre-PR behavior).

Co-authored-by: synergy-agent <299070056+synergy-agent@users.noreply.github.com>
@yzxoi
yzxoi merged commit 77f359e into dev Aug 31, 2026
8 of 10 checks passed
@yzxoi
yzxoi deleted the synergy/session-fa950b branch August 31, 2026 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants