The proactive expiry check is the only thing that has ever triggered a refresh, and expires_at is only the AS's advertised lifetime. Tokens die earlier than that routinely: server-side revocation, an identity provider idle-timeout policy, or an AS that omits expires_in entirely (null expiry, which shouldRefreshToken returns false for forever). Treat an upstream 401 as authoritative: re-mint once and retry. Excludes 403 (authenticated but not permitted) and connections with no refresh token, and keeps the upstream's own failure when the retry also fails. Also drop pooled MCP sessions on 401 — a session is bound to the bearer it was dialled with, so a retained one would serve the dead credential for the full idle window and the refreshed token could never reach the server. Adds tracing to the refresh path, which had no span, log, or metric.
Unified · Split
| @@ -0,0 +1,5 @@ | ||
| 1 | +--- | |
| 2 | +"executor": patch | |
| 3 | +--- | |
| 4 | + | |
| 5 | +Refresh OAuth tokens when the upstream rejects them with HTTP 401, not only when the stored expiry says they are due. Connections whose authorization server omits `expires_in` can now recover without a manual reconnect, and the refresh path is traced. | |
| @@ -1,4 +1,4 @@ | ||
| 1 | −import { ToolResult, type ToolError } from "./tool-result"; | |
| 1 | +import { isToolResult, ToolResult, type ToolError } from "./tool-result"; | |
| 2 | 2 | |
| 3 | 3 | export type AuthToolFailureCode = |
| 4 | 4 | | "connection_value_missing" |
| @@ -84,3 +84,21 @@ export const authToolFailure = <T = never>(input: AuthToolFailureInput): ToolRes · 79 unmodified lines | ||
| 84 | 84 | }; |
| 85 | 85 | return ToolResult.fail(error); |
| 86 | 86 | }; |
| 87 | + | |
| 88 | +/** | |
| 89 | + * Whether a tool result is the upstream saying "this credential is not valid" | |
| 90 | + * — an HTTP 401 that every protocol plugin reports as `connection_rejected`. | |
| 91 | + * | |
| 92 | + * This is the trigger for a reactive token refresh, so it is deliberately | |
| 93 | + * exact. `oauth_scope_insufficient` and any 403 are excluded: those mean | |
| 94 | + * authenticated-but-not-permitted, and re-minting the same grant returns the | |
| 95 | + * identical answer (a retry loop that burns a refresh-token rotation per call | |
| 96 | + * and never converges). `oauth_reauth_required` and `oauth_refresh_failed` are | |
| 97 | + * excluded too — they are raised by the refresh path itself, so retrying on | |
| 98 | + * them would mean refreshing in response to a failed refresh. | |
| 99 | + */ | |
| 100 | +export const isUnauthorizedToolFailure = (value: unknown): boolean => | |
| 101 | + isToolResult(value) && | |
| 102 | + !value.ok && | |
| 103 | + value.error.code === "connection_rejected" && | |
| 104 | + value.error.status === 401; | |
| @@ -151,6 +151,7 @@ import { · 150 unmodified lines | ||
| 151 | 151 | } from "./oauth-helpers"; |
| 152 | 152 | import { connectionIdentifier } from "./connection-name-identifier"; |
| 153 | 153 | import { annotateToolResultOutcome } from "./tool-result"; |
| 154 | +import { isUnauthorizedToolFailure } from "./auth-tool-failure"; | |
| 154 | 155 | |
| 155 | 156 | const PLUGIN_STORAGE_DELETE_KEY_BATCH_SIZE = 90; |
| 156 | 157 | const PLUGIN_STORAGE_CREATE_ROW_BATCH_SIZE = 90; |
| @@ -1500,10 +1501,15 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 1343 unmodified lines | ||
| 1500 | 1501 | where: (b: AnyCb) => b.and(byOwner(owner)(b), b("slug", "=", slug)), |
| 1501 | 1502 | }); |
| 1502 | 1503 | |
| 1504 | + /** What drove a refresh: the pre-call expiry check (`proactive`), or an | |
| 1505 | + * upstream 401 on a token we believed was still valid (`reactive`). */ | |
| 1506 | + type RefreshTrigger = "proactive" | "reactive"; | |
| 1507 | + | |
| 1503 | 1508 | // Perform the actual refresh-token grant and persist the rotated material. |
| 1504 | 1509 | const performTokenRefresh = ( |
| 1505 | 1510 | row: ConnectionRow, |
| 1506 | 1511 | provider: CredentialProvider, |
| 1512 | + trigger: RefreshTrigger, | |
| 1507 | 1513 | ): Effect.Effect<string | null, StorageFailure | CredentialResolutionError> => |
| 1508 | 1514 | Effect.gen(function* () { |
| 1509 | 1515 | const owner = row.owner as Owner; |
| @@ -1653,11 +1659,45 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 143 unmodified lines | ||
| 1653 | 1659 | }); |
| 1654 | 1660 | |
| 1655 | 1661 | return token.access_token; |
| 1656 | − }); | |
| 1662 | + }).pipe( | |
| 1663 | + // The refresh path was previously invisible to telemetry: no span, no | |
| 1664 | + // log, no metric. When a customer reported "my OAuth just died", there | |
| 1665 | + // was no way to answer "did a refresh even fire, and what did the AS | |
| 1666 | + // say?" without a repro. Stamp the outcome and the AS's own error code | |
| 1667 | + // (an enumerable RFC 6749 §5.2 identifier, never user content or token | |
| 1668 | + // material) so refresh health is greppable in traces. | |
| 1669 | + Effect.tap(() => Effect.annotateCurrentSpan({ "executor.oauth.refresh.outcome": "ok" })), | |
| 1670 | + Effect.tapError((error: StorageFailure | CredentialResolutionError) => | |
| 1671 | + Effect.annotateCurrentSpan({ | |
| 1672 | + "executor.oauth.refresh.outcome": "fail", | |
| 1673 | + "executor.oauth.refresh.error": Predicate.isTagged(error, "CredentialResolutionError") | |
| 1674 | + ? "CredentialResolutionError" | |
| 1675 | + : "StorageFailure", | |
| 1676 | + // Whether the AS's refusal was definitive (RFC 6749 invalid_grant → | |
| 1677 | + // the refresh token itself is dead) or a transient failure the next | |
| 1678 | + // invoke can retry. The split is the actionable half of the signal. | |
| 1679 | + ...(Predicate.isTagged(error, "CredentialResolutionError") | |
| 1680 | + ? { "executor.oauth.refresh.reauth_required": error.reauthRequired === true } | |
| 1681 | + : {}), | |
| 1682 | + }), | |
| 1683 | + ), | |
| 1684 | + Effect.withSpan("executor.oauth.refresh", { | |
| 1685 | + attributes: { | |
| 1686 | + "executor.integration": String(row.integration), | |
| 1687 | + "executor.connection": String(row.name), | |
| 1688 | + // Which path drove this refresh: the expiry check ahead of a call, | |
| 1689 | + // or an upstream 401 on a token we believed was still good. The | |
| 1690 | + // ratio is the health signal — a rising `reactive` share means | |
| 1691 | + // tokens are dying earlier than their advertised expiry. | |
| 1692 | + "executor.oauth.refresh.trigger": trigger, | |
| 1693 | + }, | |
| 1694 | + }), | |
| 1695 | + ); | |
| 1657 | 1696 | |
| 1658 | 1697 | const refreshConnectionToken = ( |
| 1659 | 1698 | row: ConnectionRow, |
| 1660 | 1699 | provider: CredentialProvider, |
| 1700 | + trigger: RefreshTrigger = "proactive", | |
| 1661 | 1701 | ): Effect.Effect<string | null, StorageFailure | CredentialResolutionError> => |
| 1662 | 1702 | // Share a single refresh per connection so concurrent resolves of the same |
| 1663 | 1703 | // connection all await one refresh-token grant (the AS rotates the refresh |
| @@ -1666,11 +1706,16 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 2 unmodified lines | ||
| 1666 | 1706 | // expiry can refresh again. |
| 1667 | 1707 | Effect.gen(function* () { |
| 1668 | 1708 | const key = connectionKey(row); |
| 1709 | + // Joining an in-flight grant is correct for BOTH triggers: whatever | |
| 1710 | + // that peer mints is newer than the token this fiber just saw rejected, | |
| 1711 | + // which is exactly what a reactive retry wants. The gate is cleared on | |
| 1712 | + // settle, so a 401 arriving after a refresh completed starts a fresh | |
| 1713 | + // grant rather than replaying the stale memoized one. | |
| 1669 | 1714 | const existing = refreshInFlight.get(key); |
| 1670 | 1715 | if (existing) return yield* existing; |
| 1671 | 1716 | // `Effect.cached` memoizes the grant onto a deferred: it runs once and |
| 1672 | 1717 | // replays to every awaiter sharing this entry. |
| 1673 | − const memoized = yield* Effect.cached(performTokenRefresh(row, provider)); | |
| 1718 | + const memoized = yield* Effect.cached(performTokenRefresh(row, provider, trigger)); | |
| 1674 | 1719 | const gated = memoized.pipe( |
| 1675 | 1720 | Effect.ensuring(Effect.sync(() => refreshInFlight.delete(key))), |
| 1676 | 1721 | ); |
| @@ -1720,6 +1765,29 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 43 unmodified lines | ||
| 1720 | 1765 | ), |
| 1721 | 1766 | ); |
| 1722 | 1767 | |
| 1768 | + /** Re-mint an OAuth connection's access token unconditionally, ignoring the | |
| 1769 | + * stored expiry. Drives the reactive path: the upstream just rejected the | |
| 1770 | + * token we sent, which is authoritative regardless of what `expires_at` | |
| 1771 | + * claims (revoked server-side, an idle-timeout policy shorter than the | |
| 1772 | + * advertised lifetime, or an expiry the AS never advertised at all). | |
| 1773 | + * | |
| 1774 | + * Returns null when the connection can't be re-minted without a human — | |
| 1775 | + * not OAuth-backed, or holding no refresh token — so the caller keeps the | |
| 1776 | + * upstream's own auth failure instead of inventing one. */ | |
| 1777 | + const forceRefreshConnectionValues = ( | |
| 1778 | + row: ConnectionRow, | |
| 1779 | + ): Effect.Effect< | |
| 1780 | + Record<string, string | null> | null, | |
| 1781 | + StorageFailure | CredentialResolutionError | |
| 1782 | + > => | |
| 1783 | + Effect.gen(function* () { | |
| 1784 | + if (row.oauth_client == null || row.refresh_item_id == null) return null; | |
| 1785 | + const provider = credentialProviders.get(row.provider); | |
| 1786 | + if (!provider) return null; | |
| 1787 | + const access = yield* refreshConnectionToken(row, provider, "reactive"); | |
| 1788 | + return { [PRIMARY_INPUT_VARIABLE]: access }; | |
| 1789 | + }); | |
| 1790 | + | |
| 1723 | 1791 | /** The primary (`token`) value — the public seam for OAuth + single-input |
| 1724 | 1792 | * callers that only ever need one value. */ |
| 1725 | 1793 | const resolveConnectionValue = ( |
| @@ -3798,27 +3866,58 @@ export const createExecutor = <const TPlugins extends readonly AnyPlugin[] = rea · 2072 unmodified lines | ||
| 3798 | 3866 | const values = yield* resolveConnectionValues(connectionRow); |
| 3799 | 3867 | const integrationRow = yield* findIntegrationRow(parsed.integration); |
| 3800 | 3868 | const grantedScopes = grantedScopesFromRow(connectionRow); |
| 3801 | − const credential: ToolInvocationCredential = { | |
| 3802 | − owner: parsed.owner, | |
| 3803 | − integration: parsed.integration, | |
| 3804 | − connection: parsed.connection, | |
| 3805 | − template: AuthTemplateSlug.make(connectionRow.template), | |
| 3806 | − value: values[PRIMARY_INPUT_VARIABLE] ?? null, | |
| 3807 | − values, | |
| 3808 | − config: integrationRow ? decodeJsonColumn(integrationRow.config) : undefined, | |
| 3809 | − ...(grantedScopes ? { grantedScopes } : {}), | |
| 3869 | + const invokeTool = runtime.plugin.invokeTool; | |
| 3870 | + const invokeWith = ( | |
| 3871 | + resolved: Record<string, string | null>, | |
| 3872 | + ): Effect.Effect<unknown, ToolInvocationError> => { | |
| 3873 | + const credential: ToolInvocationCredential = { | |
| 3874 | + owner: parsed.owner, | |
| 3875 | + integration: parsed.integration, | |
| 3876 | + connection: parsed.connection, | |
| 3877 | + template: AuthTemplateSlug.make(connectionRow.template), | |
| 3878 | + value: resolved[PRIMARY_INPUT_VARIABLE] ?? null, | |
| 3879 | + values: resolved, | |
| 3880 | + config: integrationRow ? decodeJsonColumn(integrationRow.config) : undefined, | |
| 3881 | + ...(grantedScopes ? { grantedScopes } : {}), | |
| 3882 | + }; | |
| 3883 | + return wrapInvocationError( | |
| 3884 | + invokeTool({ | |
| 3885 | + ctx: runtime.ctx, | |
| 3886 | + toolRow: row, | |
| 3887 | + credential, | |
| 3888 | + args, | |
| 3889 | + elicit: buildElicit(address, args, handler), | |
| 3890 | + invokeOptions: options, | |
| 3891 | + }), | |
| 3892 | + ); | |
| 3810 | 3893 | }; |
| 3811 | 3894 | |
| 3812 | − return yield* wrapInvocationError( | |
| 3813 | − runtime.plugin.invokeTool({ | |
| 3814 | − ctx: runtime.ctx, | |
| 3815 | − toolRow: row, | |
| 3816 | − credential, | |
| 3817 | − args, | |
| 3818 | − elicit: buildElicit(address, args, handler), | |
| 3819 | − invokeOptions: options, | |
| 3820 | − }), | |
| 3895 | + const first = yield* invokeWith(values); | |
| 3896 | + // Reactive refresh. `expires_at` is only ever the AS's ADVERTISED | |
| 3897 | + // lifetime; the upstream rejecting the token is the authoritative word | |
| 3898 | + // on whether it is still good. The two diverge routinely: server-side | |
| 3899 | + // revocation, an identity provider's idle-timeout policy shorter than | |
| 3900 | + // the token lifetime, and connections whose AS omitted `expires_in` | |
| 3901 | + // entirely (null expiry → the proactive check never fires, so this is | |
| 3902 | + // their ONLY route back to a working token short of a reconnect). | |
| 3903 | + // | |
| 3904 | + // Deliberately narrow: exactly one retry, only on the 401 that means | |
| 3905 | + // "this credential is not valid", and only for a connection holding a | |
| 3906 | + // refresh token. A 403 is excluded — it means authenticated-but-not- | |
| 3907 | + // permitted, and re-minting the same grant returns the same answer. | |
| 3908 | + // If the retry also fails its result stands, so a genuinely dead grant | |
| 3909 | + // still surfaces the upstream's own auth failure and its reconnect | |
| 3910 | + // guidance rather than a masked one. | |
| 3911 | + if (!isUnauthorizedToolFailure(first)) return first; | |
| 3912 | + const refreshed = yield* forceRefreshConnectionValues(connectionRow).pipe( | |
| 3913 | + // A failed re-mint is not this call's failure to report: the upstream | |
| 3914 | + // already produced an auth failure with recovery guidance, which is | |
| 3915 | + // strictly more actionable than a refresh-plumbing error. Keep it. | |
| 3916 | + Effect.catchTag("CredentialResolutionError", () => Effect.succeed(null)), | |
| 3821 | 3917 | ); |
| 3918 | + if (!refreshed) return first; | |
| 3919 | + yield* Effect.annotateCurrentSpan({ "executor.oauth.refresh.retried": true }); | |
| 3920 | + return yield* invokeWith(refreshed); | |
| 3822 | 3921 | }).pipe( |
| 3823 | 3922 | // Expected tool failures (`ToolResult.fail`) resolve through the |
| 3824 | 3923 | // success channel, so the tracer alone would record them as healthy |
| @@ -404,6 +404,7 @@ export { · 403 unmodified lines | ||
| 404 | 404 | } from "./sqlite-oauth-client-gc-migration"; |
| 405 | 405 | export { |
| 406 | 406 | authToolFailure, |
| 407 | + isUnauthorizedToolFailure, | |
| 407 | 408 | type AuthToolFailureCode, |
| 408 | 409 | type AuthToolFailureInput, |
| 409 | 410 | } from "./auth-tool-failure"; |
| @@ -10,11 +10,13 @@ import { · 9 unmodified lines | ||
| 10 | 10 | ToolAddress, |
| 11 | 11 | ToolName, |
| 12 | 12 | } from "./ids"; |
| 13 | +import { authToolFailure } from "./auth-tool-failure"; | |
| 13 | 14 | import { decodeOAuthCallbackState } from "./oauth"; |
| 14 | 15 | import { OAuthStartError } from "./oauth-client"; |
| 15 | 16 | import { missingGrantedOAuthScopes } from "./oauth-service"; |
| 16 | 17 | import { definePlugin } from "./plugin"; |
| 17 | 18 | import { makeTestWorkspaceHarness, memoryCredentialsPlugin } from "./test-config"; |
| 19 | +import { ToolResult } from "./tool-result"; | |
| 18 | 20 | import { serveOAuthTestServer } from "./testing/oauth-test-server"; |
| 19 | 21 | |
| 20 | 22 | // Milestone 2: prove the v2 `oauth.start` / `oauth.complete` token-minting flow |
| @@ -1220,3 +1222,332 @@ describe("missingGrantedOAuthScopes canonicalization", () => { · 1199 unmodified lines | ||
| 1220 | 1222 | expect(missing).toEqual([]); |
| 1221 | 1223 | }); |
| 1222 | 1224 | }); |
| 1225 | + | |
| 1226 | +// --------------------------------------------------------------------------- | |
| 1227 | +// Reactive refresh: an upstream 401 re-mints the access token and retries once. | |
| 1228 | +// | |
| 1229 | +// `expires_at` is only ever the authorization server's ADVERTISED lifetime. The | |
| 1230 | +// upstream rejecting the token is the authoritative word on whether it is still | |
| 1231 | +// good, and the two diverge routinely: server-side revocation, an identity | |
| 1232 | +// provider's idle-timeout policy shorter than the token lifetime, and the case | |
| 1233 | +// these tests pin hardest — an AS that omits `expires_in` entirely, leaving a | |
| 1234 | +// null expiry that the proactive check can never fire on. | |
| 1235 | +// --------------------------------------------------------------------------- | |
| 1236 | + | |
| 1237 | +/** A plugin whose tool authenticates for real: any token in `revoked` gets the | |
| 1238 | + * same `connection_rejected` / HTTP 401 shape every protocol plugin emits; | |
| 1239 | + * anything else succeeds. Modelling revocation (rather than "only the newest | |
| 1240 | + * token works") is what lets a test assert that the RETRY succeeded — the | |
| 1241 | + * re-minted token is simply one the upstream never revoked. */ | |
| 1242 | +const makeRejectingPlugin = (state: { | |
| 1243 | + revoked: Set<string>; | |
| 1244 | + /** Reject every token, even a freshly minted one — a dead grant. */ | |
| 1245 | + rejectEverything?: boolean; | |
| 1246 | + calls: string[]; | |
| 1247 | +}) => | |
| 1248 | + definePlugin(() => ({ | |
| 1249 | + id: "acme" as const, | |
| 1250 | + storage: () => ({}), | |
| 1251 | + resolveTools: () => | |
| 1252 | + Effect.succeed({ tools: [{ name: ToolName.make("whoami"), description: "whoami" }] }), | |
| 1253 | + describeAuthMethods: () => [ | |
| 1254 | + { | |
| 1255 | + id: "oauth", | |
| 1256 | + label: "OAuth2", | |
| 1257 | + kind: "oauth" as const, | |
| 1258 | + template: String(TEMPLATE), | |
| 1259 | + oauth: { scopes: [] }, | |
| 1260 | + }, | |
| 1261 | + ], | |
| 1262 | + invokeTool: ({ credential }) => { | |
| 1263 | + const token = credential.value; | |
| 1264 | + state.calls.push(String(token)); | |
| 1265 | + if (token !== null && !state.rejectEverything && !state.revoked.has(token)) { | |
| 1266 | + return Effect.succeed(ToolResult.ok({ token })); | |
| 1267 | + } | |
| 1268 | + return Effect.succeed( | |
| 1269 | + authToolFailure({ | |
| 1270 | + code: "connection_rejected", | |
| 1271 | + status: 401, | |
| 1272 | + message: "Upstream rejected credentials with HTTP 401.", | |
| 1273 | + integration: { id: String(credential.integration) }, | |
| 1274 | + credential: { kind: "upstream", label: String(credential.connection) }, | |
| 1275 | + }), | |
| 1276 | + ); | |
| 1277 | + }, | |
| 1278 | + extension: (ctx) => ({ | |
| 1279 | + seed: () => ctx.core.integrations.register({ slug: INTEG, description: "Acme", config: {} }), | |
| 1280 | + }), | |
| 1281 | + }))(); | |
| 1282 | + | |
| 1283 | +/** Mint a connection against `server` and return the harness + call log. */ | |
| 1284 | +const connectRejecting = (options?: { readonly tokenExpiresInSeconds?: number }) => | |
| 1285 | + Effect.gen(function* () { | |
| 1286 | + const server = yield* serveOAuthTestServer({ | |
| 1287 | + scopes: ["read"], | |
| 1288 | + ...(options?.tokenExpiresInSeconds !== undefined | |
| 1289 | + ? { tokenExpiresInSeconds: options.tokenExpiresInSeconds } | |
| 1290 | + : {}), | |
| 1291 | + }); | |
| 1292 | + const state = { | |
| 1293 | + revoked: new Set<string>(), | |
| 1294 | + rejectEverything: false, | |
| 1295 | + calls: [] as string[], | |
| 1296 | + }; | |
| 1297 | + const issuedLatest = Effect.gen(function* () { | |
| 1298 | + const issued = yield* server.issuedAccessTokens; | |
| 1299 | + return issued.length > 0 ? issued[issued.length - 1]! : null; | |
| 1300 | + }); | |
| 1301 | + const harness = yield* makeTestWorkspaceHarness({ | |
| 1302 | + plugins: [memoryCredentialsPlugin(), makeRejectingPlugin(state)] as const, | |
| 1303 | + }); | |
| 1304 | + const { executor, config } = harness; | |
| 1305 | + yield* executor.acme.seed(); | |
| 1306 | + yield* executor.oauth.createClient({ | |
| 1307 | + owner: "org", | |
| 1308 | + slug: CLIENT, | |
| 1309 | + authorizationUrl: server.authorizationEndpoint, | |
| 1310 | + tokenUrl: server.tokenEndpoint, | |
| 1311 | + grant: "authorization_code", | |
| 1312 | + clientId: "test-client", | |
| 1313 | + clientSecret: "test-secret", | |
| 1314 | + }); | |
| 1315 | + const started = yield* executor.oauth.start({ | |
| 1316 | + owner: "org", | |
| 1317 | + client: CLIENT, | |
| 1318 | + clientOwner: "org", | |
| 1319 | + name: ConnectionName.make("mine"), | |
| 1320 | + integration: INTEG, | |
| 1321 | + template: TEMPLATE, | |
| 1322 | + }); | |
| 1323 | + if (started.status !== "redirect") { | |
| 1324 | + return yield* Effect.die("expected a redirect-status OAuth start"); | |
| 1325 | + } | |
| 1326 | + const callback = yield* server.completeAuthorizationCodeFlow({ | |
| 1327 | + authorizationUrl: started.authorizationUrl, | |
| 1328 | + }); | |
| 1329 | + yield* executor.oauth.complete({ state: started.state, code: callback.code }); | |
| 1330 | + return { server, state, executor, config, issuedLatest }; | |
| 1331 | + }); | |
| 1332 | + | |
| 1333 | +const ADDRESS = ToolAddress.make("tools.acme.org.mine.whoami"); | |
| 1334 | + | |
| 1335 | +/** Mark every token the AS has issued so far as no longer honoured upstream — | |
| 1336 | + * the state a revocation or an idle-timeout policy leaves behind. */ | |
| 1337 | +const revokeIssuedSoFar = ( | |
| 1338 | + server: { readonly issuedAccessTokens: Effect.Effect<readonly string[]> }, | |
| 1339 | + state: { revoked: Set<string> }, | |
| 1340 | +) => | |
| 1341 | + Effect.gen(function* () { | |
| 1342 | + for (const token of yield* server.issuedAccessTokens) state.revoked.add(token); | |
| 1343 | + }); | |
| 1344 | + | |
| 1345 | +const refreshGrants = (requests: readonly { readonly path: string; readonly body: string }[]) => | |
| 1346 | + requests.filter((r) => r.path === "/token" && r.body.includes("grant_type=refresh_token")); | |
| 1347 | + | |
| 1348 | +describe("reactive OAuth refresh on upstream 401", () => { | |
| 1349 | + it.effect( | |
| 1350 | + "re-mints and retries once when the upstream rejects a token it still believes valid", | |
| 1351 | + () => | |
| 1352 | + Effect.scoped( | |
| 1353 | + Effect.gen(function* () { | |
| 1354 | + const { server, state, executor, issuedLatest } = yield* connectRejecting(); | |
| 1355 | + | |
| 1356 | + // Simulate the divergence: the AS says the token is good for an hour, | |
| 1357 | + // but the upstream has already stopped honouring it (revoked / idle | |
| 1358 | + // timeout). Nothing about `expires_at` reflects this. | |
| 1359 | + yield* revokeIssuedSoFar(server, state); | |
| 1360 | + yield* server.clearRequests; | |
| 1361 | + | |
| 1362 | + const result = (yield* executor.execute(ADDRESS, {})) as { | |
| 1363 | + data: { token: string }; | |
| 1364 | + }; | |
| 1365 | + | |
| 1366 | + // The retry ran against a token the upstream would accept, so the call | |
| 1367 | + // succeeded rather than surfacing the 401 to the user. | |
| 1368 | + const latest = yield* issuedLatest; | |
| 1369 | + expect(result.data.token).toBe(latest); | |
| 1370 | + expect(state.calls).toHaveLength(2); | |
| 1371 | + expect(state.calls[0]).not.toBe(state.calls[1]); | |
| 1372 | + | |
| 1373 | + // Exactly one refresh grant — the retry is single-shot, not a loop. | |
| 1374 | + expect(refreshGrants(yield* server.requests)).toHaveLength(1); | |
| 1375 | + }), | |
| 1376 | + ), | |
| 1377 | + ); | |
| 1378 | + | |
| 1379 | + it.effect("recovers a null-expiry connection, which the proactive check can never fire on", () => | |
| 1380 | + Effect.scoped( | |
| 1381 | + Effect.gen(function* () { | |
| 1382 | + const { server, state, executor, config, issuedLatest } = yield* connectRejecting(); | |
| 1383 | + | |
| 1384 | + // An AS that omits `expires_in` leaves expires_at null. `shouldRefreshToken` | |
| 1385 | + // returns false for null forever, so before the reactive path these | |
| 1386 | + // connections could never refresh at all — they died silently and only a | |
| 1387 | + // manual reconnect brought them back. 5 such rows existed in production. | |
| 1388 | + yield* Effect.promise(() => | |
| 1389 | + config.db.updateMany("connection", { | |
| 1390 | + where: (b) => b("name", "=", "mine"), | |
| 1391 | + set: { expires_at: null }, | |
| 1392 | + }), | |
| 1393 | + ); | |
| 1394 | + yield* revokeIssuedSoFar(server, state); | |
| 1395 | + yield* server.clearRequests; | |
| 1396 | + | |
| 1397 | + const result = (yield* executor.execute(ADDRESS, {})) as { | |
| 1398 | + data: { token: string }; | |
| 1399 | + }; | |
| 1400 | + | |
| 1401 | + expect(result.data.token).toBe(yield* issuedLatest); | |
| 1402 | + expect(refreshGrants(yield* server.requests)).toHaveLength(1); | |
| 1403 | + }), | |
| 1404 | + ), | |
| 1405 | + ); | |
| 1406 | + | |
| 1407 | + it.effect("persists the re-minted token so the next call needs no second refresh", () => | |
| 1408 | + Effect.scoped( | |
| 1409 | + Effect.gen(function* () { | |
| 1410 | + const { server, state, executor, issuedLatest } = yield* connectRejecting(); | |
| 1411 | + yield* revokeIssuedSoFar(server, state); | |
| 1412 | + yield* executor.execute(ADDRESS, {}); | |
| 1413 | + | |
| 1414 | + // The re-minted token is not revoked, so a second call must reuse the | |
| 1415 | + // stored value rather than refreshing again. | |
| 1416 | + const current = yield* issuedLatest; | |
| 1417 | + yield* server.clearRequests; | |
| 1418 | + | |
| 1419 | + const result = (yield* executor.execute(ADDRESS, {})) as { | |
| 1420 | + data: { token: string }; | |
| 1421 | + }; | |
| 1422 | + expect(result.data.token).toBe(current); | |
| 1423 | + expect(refreshGrants(yield* server.requests)).toHaveLength(0); | |
| 1424 | + }), | |
| 1425 | + ), | |
| 1426 | + ); | |
| 1427 | + | |
| 1428 | + it.effect("surfaces the upstream's own 401 when the re-minted token is also rejected", () => | |
| 1429 | + Effect.scoped( | |
| 1430 | + Effect.gen(function* () { | |
| 1431 | + const { server, state, executor } = yield* connectRejecting(); | |
| 1432 | + | |
| 1433 | + // The upstream rejects everything, including whatever the refresh mints | |
| 1434 | + // — a genuinely dead grant, not a stale token. The user must see the | |
| 1435 | + // upstream's auth failure and its reconnect guidance, NOT a masked | |
| 1436 | + // error or a retry loop. | |
| 1437 | + state.rejectEverything = true; | |
| 1438 | + yield* server.clearRequests; | |
| 1439 | + | |
| 1440 | + const result = yield* executor.execute(ADDRESS, {}); | |
| 1441 | + | |
| 1442 | + expect(result).toMatchObject({ | |
| 1443 | + ok: false, | |
| 1444 | + error: { code: "connection_rejected", status: 401 }, | |
| 1445 | + }); | |
| 1446 | + // Tried twice, refreshed once, then stopped. | |
| 1447 | + expect(state.calls).toHaveLength(2); | |
| 1448 | + expect(refreshGrants(yield* server.requests)).toHaveLength(1); | |
| 1449 | + }), | |
| 1450 | + ), | |
| 1451 | + ); | |
| 1452 | + | |
| 1453 | + it.effect("does not retry a 403 — re-minting the same grant returns the same answer", () => | |
| 1454 | + Effect.scoped( | |
| 1455 | + Effect.gen(function* () { | |
| 1456 | + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); | |
| 1457 | + const calls: string[] = []; | |
| 1458 | + const forbiddenPlugin = definePlugin(() => ({ | |
| 1459 | + id: "acme" as const, | |
| 1460 | + storage: () => ({}), | |
| 1461 | + resolveTools: () => | |
| 1462 | + Effect.succeed({ tools: [{ name: ToolName.make("whoami"), description: "whoami" }] }), | |
| 1463 | + describeAuthMethods: () => [ | |
| 1464 | + { | |
| 1465 | + id: "oauth", | |
| 1466 | + label: "OAuth2", | |
| 1467 | + kind: "oauth" as const, | |
| 1468 | + template: String(TEMPLATE), | |
| 1469 | + oauth: { scopes: [] }, | |
| 1470 | + }, | |
| 1471 | + ], | |
| 1472 | + invokeTool: ({ credential }) => { | |
| 1473 | + calls.push(String(credential.value)); | |
| 1474 | + return Effect.succeed( | |
| 1475 | + authToolFailure({ | |
| 1476 | + code: "connection_rejected", | |
| 1477 | + status: 403, | |
| 1478 | + message: "Upstream rejected credentials with HTTP 403.", | |
| 1479 | + integration: { id: String(credential.integration) }, | |
| 1480 | + credential: { kind: "upstream", label: String(credential.connection) }, | |
| 1481 | + }), | |
| 1482 | + ); | |
| 1483 | + }, | |
| 1484 | + extension: (ctx) => ({ | |
| 1485 | + seed: () => | |
| 1486 | + ctx.core.integrations.register({ slug: INTEG, description: "Acme", config: {} }), | |
| 1487 | + }), | |
| 1488 | + }))(); | |
| 1489 | + | |
| 1490 | + const { executor } = yield* makeTestWorkspaceHarness({ | |
| 1491 | + plugins: [memoryCredentialsPlugin(), forbiddenPlugin] as const, | |
| 1492 | + }); | |
| 1493 | + yield* executor.acme.seed(); | |
| 1494 | + yield* executor.oauth.createClient({ | |
| 1495 | + owner: "org", | |
| 1496 | + slug: CLIENT, | |
| 1497 | + authorizationUrl: server.authorizationEndpoint, | |
| 1498 | + tokenUrl: server.tokenEndpoint, | |
| 1499 | + grant: "authorization_code", | |
| 1500 | + clientId: "test-client", | |
| 1501 | + clientSecret: "test-secret", | |
| 1502 | + }); | |
| 1503 | + const started = yield* executor.oauth.start({ | |
| 1504 | + owner: "org", | |
| 1505 | + client: CLIENT, | |
| 1506 | + clientOwner: "org", | |
| 1507 | + name: ConnectionName.make("mine"), | |
| 1508 | + integration: INTEG, | |
| 1509 | + template: TEMPLATE, | |
| 1510 | + }); | |
| 1511 | + if (started.status !== "redirect") return; | |
| 1512 | + const callback = yield* server.completeAuthorizationCodeFlow({ | |
| 1513 | + authorizationUrl: started.authorizationUrl, | |
| 1514 | + }); | |
| 1515 | + yield* executor.oauth.complete({ state: started.state, code: callback.code }); | |
| 1516 | + yield* server.clearRequests; | |
| 1517 | + | |
| 1518 | + const result = yield* executor.execute(ADDRESS, {}); | |
| 1519 | + | |
| 1520 | + // A 403 is authenticated-but-not-permitted. Retrying would burn a | |
| 1521 | + // refresh-token rotation per call and never converge. | |
| 1522 | + expect(result).toMatchObject({ ok: false, error: { status: 403 } }); | |
| 1523 | + expect(calls).toHaveLength(1); | |
| 1524 | + expect(refreshGrants(yield* server.requests)).toHaveLength(0); | |
| 1525 | + }), | |
| 1526 | + ), | |
| 1527 | + ); | |
| 1528 | + | |
| 1529 | + it.effect("does not retry when the connection holds no refresh token", () => | |
| 1530 | + Effect.scoped( | |
| 1531 | + Effect.gen(function* () { | |
| 1532 | + const { server, state, executor, config } = yield* connectRejecting(); | |
| 1533 | + | |
| 1534 | + // Nothing to re-mint from: the only recovery is a human reconnect, so | |
| 1535 | + // the upstream's failure must reach the caller on the first try. | |
| 1536 | + yield* Effect.promise(() => | |
| 1537 | + config.db.updateMany("connection", { | |
| 1538 | + where: (b) => b("name", "=", "mine"), | |
| 1539 | + set: { refresh_item_id: null }, | |
| 1540 | + }), | |
| 1541 | + ); | |
| 1542 | + yield* revokeIssuedSoFar(server, state); | |
| 1543 | + yield* server.clearRequests; | |
| 1544 | + | |
| 1545 | + const result = yield* executor.execute(ADDRESS, {}); | |
| 1546 | + | |
| 1547 | + expect(result).toMatchObject({ ok: false, error: { status: 401 } }); | |
| 1548 | + expect(state.calls).toHaveLength(1); | |
| 1549 | + expect(refreshGrants(yield* server.requests)).toHaveLength(0); | |
| 1550 | + }), | |
| 1551 | + ), | |
| 1552 | + ); | |
| 1553 | +}); | |
| @@ -746,6 +746,19 @@ export const refreshAccessToken = ( · 745 unmodified lines | ||
| 746 | 746 | // Refresh-needed predicate |
| 747 | 747 | // --------------------------------------------------------------------------- |
| 748 | 748 | |
| 749 | +/** Whether the stored access token is close enough to its expiry to be | |
| 750 | + * re-minted BEFORE the next call goes out (the proactive path). | |
| 751 | + * | |
| 752 | + * A null `expiresAt` means the authorization server never told us when the | |
| 753 | + * token dies (`expires_in` omitted from the token response). That is not the | |
| 754 | + * same as "never expires": the token may well be revoked or time out | |
| 755 | + * upstream. We deliberately do NOT refresh on every call for those — that | |
| 756 | + * would hammer the AS on connections whose tokens are genuinely long-lived, | |
| 757 | + * and there is no expiry to be "close to". Instead the reactive path owns | |
| 758 | + * them: an upstream 401 is the only truthful signal that an unknown-expiry | |
| 759 | + * token is dead, and `executor.execute` re-mints and retries once on that | |
| 760 | + * signal. Keep the two paths in sync — narrowing the reactive retry strands | |
| 761 | + * every null-expiry connection with no way to recover short of a reconnect. */ | |
| 749 | 762 | export const shouldRefreshToken = (input: { |
| 750 | 763 | readonly expiresAt: number | null; |
| 751 | 764 | readonly now?: number; |
| @@ -153,4 +153,45 @@ describe("MCP connection pool", () => { · 152 unmodified lines | ||
| 153 | 153 | }), |
| 154 | 154 | ), |
| 155 | 155 | ); |
| 156 | + | |
| 157 | + it.effect("drops a pooled session whose bearer the server has stopped accepting", () => | |
| 158 | + Effect.scoped( | |
| 159 | + Effect.gen(function* () { | |
| 160 | + const server = yield* serveMcpServer(() => makeEchoMcpServer()); | |
| 161 | + const pool = createMcpConnectionPool(); | |
| 162 | + | |
| 163 | + yield* invoke({ | |
| 164 | + endpoint: server.endpoint, | |
| 165 | + pool, | |
| 166 | + toolName: "echo", | |
| 167 | + args: { value: "before" }, | |
| 168 | + }); | |
| 169 | + | |
| 170 | + // A 401 means the bearer this session was dialled with is no longer | |
| 171 | + // accepted. The session is bound to that token for its lifetime, so | |
| 172 | + // retaining it would serve the same dead credential for the full idle | |
| 173 | + // window — and would defeat core's post-401 refresh, whose new token can | |
| 174 | + // only reach the server over a NEW session. | |
| 175 | + yield* server.rejectNextSessionRequest(401); | |
| 176 | + yield* invoke({ | |
| 177 | + endpoint: server.endpoint, | |
| 178 | + pool, | |
| 179 | + toolName: "echo", | |
| 180 | + args: { value: "unauthorized" }, | |
| 181 | + }).pipe(Effect.flip); | |
| 182 | + | |
| 183 | + const after = yield* invoke({ | |
| 184 | + endpoint: server.endpoint, | |
| 185 | + pool, | |
| 186 | + toolName: "echo", | |
| 187 | + args: { value: "after" }, | |
| 188 | + }); | |
| 189 | + | |
| 190 | + expect(after).toMatchObject({ content: [{ type: "text", text: "after" }] }); | |
| 191 | + // A second session was dialled rather than the dead one being reused. | |
| 192 | + expect(server.sessionCount()).toBe(2); | |
| 193 | + yield* pool.close(); | |
| 194 | + }), | |
| 195 | + ), | |
| 196 | + ); | |
| 156 | 197 | }); |
| @@ -27,6 +27,13 @@ const isDeadConnectionFailure = (error: unknown): boolean => { · 26 unmodified lines | ||
| 27 | 27 | return ( |
| 28 | 28 | error.transportFailure === true || |
| 29 | 29 | error.status === 400 || |
| 30 | + // A 401 means the bearer this session was dialled with is no longer | |
| 31 | + // accepted. The session is bound to that token for its lifetime, so | |
| 32 | + // retaining it would hand the same dead credential to every caller for the | |
| 33 | + // full idle window — and would defeat core's post-401 token refresh, whose | |
| 34 | + // freshly minted token can only reach the server over a NEW session. Drop | |
| 35 | + // it so the retry dials with the new credential. | |
| 36 | + error.status === 401 || | |
| 30 | 37 | error.status === 404 || |
| 31 | 38 | error.status === 408 |
| 32 | 39 | ); |