fix(client): propagate refreshed token save errors#2110
Conversation
🦋 Changeset detectedLatest commit: 5e25f0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
|
Pushed Those failures were in Validation before push:
|
There was a problem hiding this comment.
Pull request overview
This PR addresses a client OAuth reliability issue where errors thrown while persisting refreshed tokens (e.g., saveTokens() I/O failures) could be silently swallowed, causing rotated refresh tokens to be lost and forcing unnecessary re-authentication (Fixes #2034).
Changes:
- Refactor
authInternal()to only fall back to re-auth on refresh failures, while propagating post-refreshsaveTokens()errors. - Add a regression test ensuring
saveTokens()failures after a successful refresh are surfaced and do not trigger redirect-based auth. - Add a changeset for a patch release, and improve the Cloudflare Workers integration test’s robustness/diagnostics (dynamic port + richer failure output).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
packages/client/src/client/auth.ts |
Separates refresh errors from token persistence errors so saveTokens() failures propagate. |
packages/client/test/client/auth.test.ts |
Adds coverage for propagating saveTokens() errors after a successful refresh. |
test/integration/test/server/cloudflareWorkers.test.ts |
Makes the Wrangler dev server startup/connect flow more reliable and debuggable. |
.changeset/propagate-token-save-errors.md |
Declares a patch release note for the client behavior change. |
Comments suppressed due to low confidence (1)
packages/client/src/client/auth.ts:782
- The inline comment says errors are "log[ged] out", but this catch block currently only swallows certain refresh failures without logging. Either add logging here (if intended) or update the comment so it matches the actual behavior.
// If this is a ServerError, or an unknown type, log it out and try to continue. Otherwise, escalate so we can fix things and retry.
if (!(error instanceof OAuthError) || error.code === OAuthErrorCode.ServerError) {
// Could not refresh OAuth tokens
} else {
// Refresh failed for another reason, re-throw
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
saveTokens(newTokens)outside the OAuth refresh failure catchFixes #2034.
Validation
pnpm --dir packages/client exec vitest run test/client/auth.test.ts -t "propagates saveTokens errors"pnpm --dir packages/client exec vitest run test/client/auth.test.ts(168 passed)pnpm --filter @modelcontextprotocol/client typecheckpnpm --filter @modelcontextprotocol/client lintpnpm --dir test/integration run lintpnpm --dir test/integration exec prettier --check test/server/cloudflareWorkers.test.tsgit diff --checkNote
The
test/integration/test/server/cloudflareWorkers.test.tschange is intentionally scoped to CI harness reliability: it selects an available local port, cleans up failed Wrangler startups, captures Wrangler output, retries readiness a bit longer, and closes failed client attempts before retrying. The functional OAuth change remains limited to the client auth path plus its regression test and changeset.