From 1d5f5de16ae311ecef1282a6f741031ebbe2201a Mon Sep 17 00:00:00 2001 From: spa5k <79936503+spa5k@users.noreply.github.com> Date: Thu, 17 Sep 2026 19:20:32 +0530 Subject: [PATCH 1/2] Replicate the false-expired status and the refresh races behind it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Connections present a red Expired verdict that is either wrong or unrecoverable, and the refresh machinery that produces it is uncoordinated across the surfaces that trigger it. This adds the analysis and the executable repros; it changes no runtime behavior. plans/oauth-refresh-and-expired-status.md ranks eight root causes with file:line evidence and phases the fix. Four are replicated here, each as a pair: a "documents current behavior" test that passes on main (the replication) and a REPRO test asserting the post-fix contract, checked in skipped so the suite stays green and the fix PR un-skips its own anchor. - A refresher that loses a rotation race records a permanent dead grant on a connection whose stored refresh token is valid: every surface then answers expired without probing, and the winner can no longer refresh either. - One transient 4xx from a token endpoint (a 429) ends the grant for good. - The health probe never refreshes reactively, so it persists expired for a credential the next tool call re-mints and heals — the disconnected-then- connected flap. - The MCP liveness probe dials a second connection instead of taking the pooled one, so a single-instance local stdio server fails its own health check while serving tool calls. --- .../src/oauth-expired-status-repro.test.ts | 585 ++++++++++++++++++ .../src/sdk/mcp-liveness-second-spawn.test.ts | 194 ++++++ .../sdk/stdio-single-instance-test-server.ts | 133 ++++ plans/oauth-refresh-and-expired-status.md | 456 ++++++++++++++ 4 files changed, 1368 insertions(+) create mode 100644 packages/core/sdk/src/oauth-expired-status-repro.test.ts create mode 100644 packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts create mode 100644 packages/plugins/mcp/src/sdk/stdio-single-instance-test-server.ts create mode 100644 plans/oauth-refresh-and-expired-status.md diff --git a/packages/core/sdk/src/oauth-expired-status-repro.test.ts b/packages/core/sdk/src/oauth-expired-status-repro.test.ts new file mode 100644 index 0000000000..6e44ce3e6d --- /dev/null +++ b/packages/core/sdk/src/oauth-expired-status-repro.test.ts @@ -0,0 +1,585 @@ +// Reproduction harness for the "Expired" status + refresh defects analysed in +// plans/oauth-refresh-and-expired-status.md. +// +// Each root cause gets TWO tests, with no branching inside either: +// +// "documents current behavior" — passes on main today. This is the +// replication: it pins what a user actually sees, so the defect is not a +// matter of interpretation. +// "REPRO" — asserts the behavior we want. It FAILS on main today, so it is +// checked in skipped; it is the acceptance anchor for the fix phase named +// in its title, and that PR un-skips it green without editing it. +// +// Deployment shape under test: ONE database, ONE credential store, TWO executor +// instances each holding its OWN root db handle. That is cloud (per-request +// `DbService` rebuild + per-session Durable Objects) and any multi-process +// self-host. It is the shape `refreshGateFor`'s own doc block declares out of +// scope, and the shape `oauth-flow.test.ts`'s two-instance test already builds +// — that test asserts the spent token is not written back, but never looks at +// what the loser's `invalid_grant` does to the connection ROW. These do. + +import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; + +import { describe, expect, it } from "@effect/vitest"; +import { Deferred, Effect, Fiber, Option, Schema } from "effect"; +import * as Exit from "effect/Exit"; +import { withQueryContext } from "@executor-js/fumadb/query"; + +import { authToolFailure } from "./auth-tool-failure"; +import { createExecutor } from "./executor"; +import { + AuthTemplateSlug, + ConnectionName, + IntegrationSlug, + OAuthClientSlug, + ProviderKey, + ToolAddress, + ToolName, +} from "./ids"; +import { definePlugin } from "./plugin"; +import type { CredentialProvider } from "./provider"; +import { makeTestConfig } from "./test-config"; +import { serveOAuthTestServer } from "./testing/oauth-test-server"; +import { ToolResult } from "./tool-result"; + +const TENANT = "test-tenant"; +const SUBJECT = "test-subject"; +const INTEG = IntegrationSlug.make("acme"); +const TEMPLATE = AuthTemplateSlug.make("oauth"); +const CLIENT = OAuthClientSlug.make("acme-app"); +const NAME = ConnectionName.make("main"); +const ADDRESS = ToolAddress.make("tools.acme.org.main.whoami"); +const REF = { owner: "org" as const, integration: INTEG, name: NAME }; + +// --------------------------------------------------------------------------- +// Plugin: an upstream that honours every access token except the revoked ones. +// `checkHealth` authenticates the same way `invokeTool` does, so a revoked +// token reads 401 on both paths — the divergence under test is what CORE does +// with each, not what the plugin reports. +// --------------------------------------------------------------------------- + +interface UpstreamState { + readonly revoked: Set; + readonly calls: string[]; + readonly probes: string[]; +} + +const makeUpstreamPlugin = (state: UpstreamState) => + definePlugin(() => ({ + id: "acme" as const, + storage: () => ({}), + resolveTools: () => + Effect.succeed({ tools: [{ name: ToolName.make("whoami"), description: "whoami" }] }), + describeAuthMethods: () => [ + { + id: "oauth", + label: "OAuth2", + kind: "oauth" as const, + template: String(TEMPLATE), + oauth: { scopes: [] }, + }, + ], + invokeTool: ({ credential }) => { + const token = credential.value; + state.calls.push(String(token)); + if (token !== null && !state.revoked.has(token)) { + return Effect.succeed(ToolResult.ok({ token })); + } + return Effect.succeed( + authToolFailure({ + code: "connection_rejected", + status: 401, + message: "Upstream rejected credentials with HTTP 401.", + integration: { id: String(credential.integration) }, + credential: { kind: "upstream", label: String(credential.connection) }, + }), + ); + }, + checkHealth: ({ credential }) => { + const token = credential.value; + state.probes.push(String(token)); + if (token !== null && !state.revoked.has(token)) { + return Effect.succeed({ status: "healthy" as const, checkedAt: Date.now() }); + } + return Effect.succeed({ + status: "expired" as const, + httpStatus: 401, + checkedAt: Date.now(), + detail: "The endpoint rejected the credential with HTTP 401.", + reason: "upstream_status" as const, + }); + }, + extension: (ctx) => ({ + seed: () => ctx.core.integrations.register({ slug: INTEG, description: "Acme", config: {} }), + }), + }))(); + +// --------------------------------------------------------------------------- +// Shared credential store, with a seam that can hold ONE reader between its +// read of the stored refresh token and whatever it does next. That seam is the +// race window; the same one `oauth-flow.test.ts` opens. +// --------------------------------------------------------------------------- + +interface SharedStore { + readonly provider: CredentialProvider; + readonly values: Map; + readonly writes: string[]; + /** Arm the one-shot pause on the next refresh-token read. */ + readonly arm: () => void; +} + +const makeSharedStore = (input: { + readonly pausedAtRead: Deferred.Deferred; + readonly resumeFromRead: Deferred.Deferred; +}): SharedStore => { + const values = new Map(); + const writes: string[] = []; + let pauseNextRefreshRead = false; + return { + values, + writes, + arm: () => { + pauseNextRefreshRead = true; + }, + provider: { + key: ProviderKey.make("shared-memory"), + writable: true, + get: (id) => + Effect.gen(function* () { + const value = values.get(String(id)) ?? null; + if (pauseNextRefreshRead && String(id).endsWith(":refresh")) { + pauseNextRefreshRead = false; + yield* Deferred.succeed(input.pausedAtRead, undefined); + yield* Deferred.await(input.resumeFromRead); + } + return value; + }), + set: (id, value) => + Effect.sync(() => { + writes.push(String(id)); + values.set(String(id), value); + }), + delete: (id) => Effect.sync(() => void values.delete(String(id))), + }, + }; +}; + +// --------------------------------------------------------------------------- +// One database + one store, two executor instances, one completed OAuth +// connection. `expire` forces the next resolve to refresh. +// --------------------------------------------------------------------------- + +const makeRace = (options?: { readonly healthCheck?: boolean }) => + Effect.gen(function* () { + const server = yield* serveOAuthTestServer({ scopes: ["read"] }); + const state: UpstreamState = { revoked: new Set(), calls: [], probes: [] }; + const pausedAtRead = yield* Deferred.make(); + const resumeFromRead = yield* Deferred.make(); + const store = makeSharedStore({ pausedAtRead, resumeFromRead }); + const config = { + ...makeTestConfig({ + plugins: [makeUpstreamPlugin(state)] as const, + tenant: TENANT, + subject: SUBJECT, + }), + providers: [store.provider], + }; + const a = yield* createExecutor(config); + // A SECOND root db handle onto the same database = a second instance. The + // in-flight refresh gate is keyed on handle identity, so this is exactly + // the boundary the gate cannot see across. + const b = yield* createExecutor({ + ...config, + db: withQueryContext(config.testDb.db, { tenant: TENANT, subject: SUBJECT }), + }); + yield* Effect.addFinalizer(() => a.close().pipe(Effect.ignore)); + yield* Effect.addFinalizer(() => b.close().pipe(Effect.ignore)); + yield* Effect.addFinalizer(() => + Effect.promise(() => config.testDb.close()).pipe(Effect.ignore), + ); + + yield* a.acme.seed(); + yield* a.oauth.createClient({ + owner: "org", + slug: CLIENT, + authorizationUrl: server.authorizationEndpoint, + tokenUrl: server.tokenEndpoint, + grant: "authorization_code", + clientId: "test-client", + clientSecret: "test-secret", + }); + if (options?.healthCheck === true) { + // A declared health check puts the connection on the PROBING path rather + // than the credential-only one. + yield* a.integrations.healthCheck.set(INTEG, { operation: "whoami" }); + } + const started = yield* a.oauth.start({ + owner: "org", + client: CLIENT, + clientOwner: "org", + name: NAME, + integration: INTEG, + template: TEMPLATE, + }); + if (started.status !== "redirect") return yield* Effect.die("expected a redirect start"); + const callback = yield* server.completeAuthorizationCodeFlow({ + authorizationUrl: started.authorizationUrl, + }); + yield* a.oauth.complete({ state: started.state, code: callback.code }); + + return { + server, + state, + store, + a, + b, + config, + pausedAtRead, + resumeFromRead, + expire: () => + Effect.promise(() => + config.db.updateMany("connection", { + where: (builder) => builder("name", "=", String(NAME)), + set: { expires_at: Date.now() - 60_000 }, + }), + ), + rawRow: () => + Effect.promise(() => + config.db.findFirst("connection", { + where: (builder) => builder("name", "=", String(NAME)), + }), + ), + refreshItemId: () => [...store.values.keys()].find((key) => key.endsWith(":refresh")), + } as const; + }); + +type Race = Effect.Success>; + +/** Run `use` against a freshly connected two-instance race. */ +const withRace = ( + options: { readonly healthCheck?: boolean }, + use: (race: Race) => Effect.Effect, +) => Effect.scoped(Effect.flatMap(makeRace(options), use)); + +const refreshGrants = (requests: readonly { readonly path: string; readonly body: string }[]) => + requests.filter((r) => r.path === "/token" && r.body.includes("grant_type=refresh_token")); + +const DeadGrantState = Schema.Struct({ oauthReauthRequiredAt: Schema.Number }); +const decodeDeadGrantObject = Schema.decodeUnknownOption(DeadGrantState); +const decodeDeadGrantJson = Schema.decodeUnknownOption(Schema.fromJsonString(DeadGrantState)); + +/** The recorded dead-grant stamp, read off a connection row's + * `provider_state` (a JSON column: an object on some adapters, encoded text + * on others). Takes `unknown` because the row comes back from the raw query + * surface, and normalises it through Schema rather than a cast. */ +const RowProviderState = Schema.Struct({ provider_state: Schema.optional(Schema.Unknown) }); +const decodeRowProviderState = Schema.decodeUnknownOption(RowProviderState); + +const deadGrantStamp = (row: unknown): number | undefined => { + const value = Option.getOrUndefined( + Option.map(decodeRowProviderState(row), (decoded) => decoded.provider_state), + ); + if (value === undefined || value === null) return undefined; + const fromObject = Option.getOrUndefined(decodeDeadGrantObject(value)); + if (fromObject !== undefined) return fromObject.oauthReauthRequiredAt; + return Option.getOrUndefined( + Option.map(decodeDeadGrantJson(value), (state) => state.oauthReauthRequiredAt), + ); +}; + +// --------------------------------------------------------------------------- +// R1 — the loser of a rotation race permanently bricks a healthy connection. +// --------------------------------------------------------------------------- + +/** Run the race: A reads the stored refresh token and stalls, B wins and + * rotates it, A resumes and redeems the consumed token. Shared by both R1 + * tests so they differ only in what they assert about the aftermath. */ +const runRotationRace = (race: Race) => + Effect.gen(function* () { + const refreshItemId = race.refreshItemId(); + expect(refreshItemId, "the connection stored a refresh token").toBeDefined(); + const originalRefreshToken = race.store.values.get(refreshItemId!); + yield* race.expire(); + + race.store.arm(); + const loser = yield* Effect.forkChild(Effect.exit(race.a.execute(ADDRESS, {}))); + yield* Deferred.await(race.pausedAtRead); + + // B wins: it spends that token, the AS rotates it, B stores the successor. + yield* race.b.execute(ADDRESS, {}); + const rotatedRefreshToken = race.store.values.get(refreshItemId!); + expect(rotatedRefreshToken, "the winner rotated the stored refresh token").not.toBe( + originalRefreshToken, + ); + + // A resumes and redeems a token the authorization server already consumed. + yield* Deferred.succeed(race.resumeFromRead, undefined); + yield* Fiber.join(loser); + + // The store still holds B's valid rotated token: this connection is not out + // of credentials, it lost a race. + expect(race.store.values.get(refreshItemId!)).toBe(rotatedRefreshToken); + return { refreshItemId: refreshItemId!, rotatedRefreshToken: rotatedRefreshToken! }; + }); + +describe("R1 — refresh race across two instances", () => { + it.effect("documents current behavior: the loser bricks a connection holding a valid token", () => + withRace({}, (race) => + Effect.gen(function* () { + yield* runRotationRace(race); + + // A's `invalid_grant` recorded a dead grant on a connection whose + // stored refresh token is valid. + expect( + deadGrantStamp(yield* race.rawRow()), + "the loser marked the grant permanently dead", + ).toBeTypeOf("number"); + const health = yield* race.b.connections.checkHealth(REF); + expect(health.status, "every surface now answers expired without probing").toBe("expired"); + + // The rotated token is still perfectly good — nobody is allowed to use + // it again. This is the permanent part. + yield* race.expire(); + yield* race.server.clearRequests; + const next = yield* Effect.exit(race.b.execute(ADDRESS, {})); + expect(Exit.isSuccess(next), "the winner can no longer refresh either").toBe(false); + expect( + refreshGrants(yield* race.server.requests), + "the known-dead gate never sends another grant", + ).toHaveLength(0); + }), + ), + ); + + // Skipped, not deleted: this is the acceptance anchor for Phase 1 of + // plans/oauth-refresh-and-expired-status.md. The PR that lands the fix + // un-skips it and it must go green unchanged. + it.effect.skip("REPRO: a lost rotation race must not record a dead grant (Phase 1)", () => + withRace({}, (race) => + Effect.gen(function* () { + yield* runRotationRace(race); + + // Phase 1 target: the loser notices the rotation and adopts it, so no + // dead grant is ever recorded. + expect( + deadGrantStamp(yield* race.rawRow()), + "a lost race must not record a dead grant", + ).toBeUndefined(); + const health = yield* race.b.connections.checkHealth(REF); + expect(health.status, "and no surface answers expired").not.toBe("expired"); + + yield* race.expire(); + yield* race.server.clearRequests; + const next = yield* Effect.exit(race.b.execute(ADDRESS, {})); + expect(Exit.isSuccess(next), "the winner can still refresh with its own valid token").toBe( + true, + ); + expect( + refreshGrants(yield* race.server.requests).length, + "executor asked the authorization server again", + ).toBeGreaterThan(0); + }), + ), + ); +}); + +// --------------------------------------------------------------------------- +// R2 — one transient 4xx (a 429) permanently kills the grant. +// --------------------------------------------------------------------------- + +interface FlakyEndpoint { + readonly url: string; + readonly attempts: () => number; + readonly close: () => void; +} + +/** Token endpoint that rate-limits the FIRST refresh grant and forwards the + * rest to the real authorization server: one bad minute, then healthy. */ +const serveFlakyTokenEndpoint = (upstream: string) => + Effect.acquireRelease( + Effect.callback((resume) => { + let attempts = 0; + const forward = async ( + req: IncomingMessage, + res: ServerResponse, + body: string, + ): Promise => { + // oxlint-disable-next-line executor/no-raw-fetch -- boundary: test fixture proxying form-encoded token requests to the test authorization server; it must not carry the SDK's own HttpClient layer into the endpoint under test + const response = await fetch(upstream, { + method: req.method, + headers: { + "content-type": req.headers["content-type"] ?? "application/x-www-form-urlencoded", + ...(typeof req.headers["authorization"] === "string" + ? { authorization: req.headers["authorization"] } + : {}), + }, + body: body.length > 0 ? body : undefined, + }); + const text = await response.text(); + res.writeHead(response.status, { + "content-type": response.headers.get("content-type") ?? "application/json", + }); + res.end(text); + }; + const server: Server = createServer((req, res) => { + const chunks: Buffer[] = []; + req.on("data", (chunk: Buffer) => chunks.push(chunk)); + req.on("end", () => { + const body = Buffer.concat(chunks).toString("utf8"); + if (body.includes("grant_type=refresh_token")) { + attempts += 1; + if (attempts === 1) { + res.writeHead(429, { "content-type": "text/plain; charset=utf-8" }); + res.end("Too Many Requests: slow down"); + return; + } + } + // oxlint-disable-next-line executor/no-promise-catch -- boundary: plain node:http handler in a test fixture standing in for a flaky upstream + void forward(req, res, body).catch(() => { + res.writeHead(502, { "content-type": "text/plain" }); + res.end("proxy failed"); + }); + }); + }); + server.listen(0, "127.0.0.1", () => { + const address = server.address(); + const port = typeof address === "object" && address !== null ? address.port : 0; + resume( + Effect.succeed({ + url: `http://127.0.0.1:${port}/token`, + attempts: () => attempts, + close: () => server.close(), + }), + ); + }); + }), + (handle) => Effect.sync(() => handle.close()), + ); + +/** Connect, point the backing app at a token endpoint that rate-limits once, + * and take that first (failing) refresh. Shared by both R2 tests. */ +const withRateLimitedRefresh = ( + use: (input: { + readonly race: Race; + readonly flaky: FlakyEndpoint; + readonly firstCallSucceeded: boolean; + }) => Effect.Effect, +) => + Effect.scoped( + Effect.gen(function* () { + const race = yield* makeRace({}); + const flaky = yield* serveFlakyTokenEndpoint(race.server.tokenEndpoint); + yield* Effect.promise(() => + race.config.db.updateMany("oauth_client", { + where: (builder) => builder("slug", "=", String(CLIENT)), + set: { token_url: flaky.url }, + }), + ); + yield* race.expire(); + const first = yield* Effect.exit(race.a.execute(ADDRESS, {})); + expect(Exit.isSuccess(first), "the rate-limited refresh fails the call").toBe(false); + expect(flaky.attempts(), "the token endpoint was asked once").toBe(1); + return yield* use({ race, flaky, firstCallSucceeded: Exit.isSuccess(first) }); + }), + ); + +describe("R2 — transient 4xx classification", () => { + it.effect("documents current behavior: one 429 permanently disables a working grant", () => + withRateLimitedRefresh(({ race, flaky }) => + Effect.gen(function* () { + const health = yield* race.a.connections.checkHealth(REF); + expect(health.status, "one 429 rendered the connection permanently expired").toBe( + "expired", + ); + + // The endpoint is healthy from here on — every later grant would be + // forwarded to the real authorization server and succeed. Executor + // never sends one. + const attemptsBefore = flaky.attempts(); + const second = yield* Effect.exit(race.a.execute(ADDRESS, {})); + expect(Exit.isSuccess(second), "and it never asks the healthy endpoint again").toBe(false); + expect(flaky.attempts(), "no further grant was attempted").toBe(attemptsBefore); + }), + ), + ); + + // Skipped, not deleted: Phase 1 acceptance anchor (see the note above). + it.effect.skip("REPRO: a 429 must stay retryable (Phase 1)", () => + withRateLimitedRefresh(({ race, flaky }) => + Effect.gen(function* () { + // Phase 1 target: a 429 is retryable, so the next attempt reaches the + // (now healthy) endpoint and the connection keeps working. + const second = yield* Effect.exit(race.a.execute(ADDRESS, {})); + expect(Exit.isSuccess(second), "a 429 does not end the grant").toBe(true); + expect(flaky.attempts(), "executor retried the refresh").toBeGreaterThan(1); + }), + ), + ); +}); + +// --------------------------------------------------------------------------- +// R3 — the health probe never refreshes reactively, so it writes `expired` for +// a credential the tool path would have refreshed, then flips to healthy on the +// next tool call. That flip is the "disconnected, then connected" symptom. +// --------------------------------------------------------------------------- + +/** Connect with a declared health check, then revoke the live access token + * upstream while `expires_at` still says it is good for an hour. */ +const withRevokedToken = (use: (race: Race) => Effect.Effect) => + withRace({ healthCheck: true }, (race) => + Effect.gen(function* () { + for (const token of yield* race.server.issuedAccessTokens) race.state.revoked.add(token); + yield* race.server.clearRequests; + return yield* use(race); + }), + ); + +describe("R3 — probe verdict vs reactive refresh", () => { + it.effect("documents current behavior: probe says expired, the next tool call says healthy", () => + withRevokedToken((race) => + Effect.gen(function* () { + // The probe persists `expired` without ever trying the refresh token + // that would have fixed it … + const verdict = yield* race.a.connections.checkHealth(REF); + expect(verdict.status).toBe("expired"); + expect( + refreshGrants(yield* race.server.requests), + "the probe sent no refresh grant", + ).toHaveLength(0); + const persisted = yield* race.a.connections.get(REF); + expect( + persisted?.lastHealth?.status, + "and the verdict is persisted for every surface to read", + ).toBe("expired"); + + // … then the very next tool call refreshes reactively, succeeds, and + // heals the row. Same connection, seconds apart, no user action: + // "disconnected" then "connected". + yield* race.a.execute(ADDRESS, {}); + expect( + race.state.calls.length, + "the tool call retried with a re-minted token", + ).toBeGreaterThan(1); + const healed = yield* race.a.connections.get(REF); + expect(healed?.lastHealth?.status, "heal-on-use flipped the badge back").toBe("healthy"); + }), + ), + ); + + // Skipped, not deleted: Phase 3 acceptance anchor (see the note above). + it.effect.skip("REPRO: the probe must refresh before concluding expired (Phase 3)", () => + withRevokedToken((race) => + Effect.gen(function* () { + // Phase 3 target: the probe refreshes once before concluding expired. + const verdict = yield* race.a.connections.checkHealth(REF); + expect(verdict.status, "a refreshable revocation is not an expired connection").toBe( + "healthy", + ); + expect( + refreshGrants(yield* race.server.requests).length, + "the probe re-minted the token", + ).toBeGreaterThan(0); + }), + ), + ); +}); diff --git a/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts b/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts new file mode 100644 index 0000000000..7f358f56d0 --- /dev/null +++ b/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts @@ -0,0 +1,194 @@ +// --------------------------------------------------------------------------- +// A liveness probe must not conclude "this connection is broken" from a +// failure its OWN second connection caused. +// +// `checkHealth` dials through `discoverToolsFromInput`, which builds a FRESH +// connector (`plugin.ts` → `discover.ts` → `createMcpConnector`) rather than +// taking the pooled connection tool invocations use (`connection-pool.ts`, +// one idle session per identity, five-minute TTL). For a remote server that +// costs a handshake. For a local stdio server it spawns a SECOND CHILD PROCESS +// — and the common local servers are single-instance: Chrome DevTools MCP owns +// a browser and a debug port, Playwright MCP the same, `docker run -i` a +// container. A second concurrent process cannot start and exits non-zero. +// +// So the probe's verdict describes the probe, not the connection: the server is +// up, it is serving the pooled client, every tool call works — and the accounts +// list says the connection is broken. The next probe (which the UI forces on +// every mount for any non-healthy verdict, `use-connection-health.ts`) runs once +// the pooled child is gone and reports healthy again. That is the +// "disconnected, then connected" flap. +// +// Two tests: the first documents current behavior and passes on main; the +// second asserts what the probe ought to answer, fails on main, and is checked +// in skipped as the fix's acceptance anchor. +// +// `it.live`: this measures real child processes, so it needs the wall clock. +// --------------------------------------------------------------------------- + +import { spawn, type ChildProcess } from "node:child_process"; +import { existsSync, mkdtempSync, readFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { fileURLToPath } from "node:url"; + +import { describe, expect, it } from "@effect/vitest"; +import { Duration, Effect } from "effect"; +import { FetchHttpClient } from "effect/unstable/http"; + +import { mcpPlugin } from "./plugin"; + +const fixture = fileURLToPath(new URL("./stdio-single-instance-test-server.ts", import.meta.url)); + +type Verdict = { readonly status: string; readonly detail?: string; readonly reason?: string }; + +const checkHealth = (config: unknown): Effect.Effect => + Effect.gen(function* () { + const plugin = mcpPlugin({ dangerouslyAllowStdioMCP: true }); + const seam = (plugin as { readonly checkHealth?: unknown }).checkHealth; + if (typeof seam !== "function") { + return yield* Effect.die("mcpPlugin no longer exposes checkHealth"); + } + return yield* ( + seam as (input: { + readonly ctx: { readonly httpClientLayer: typeof FetchHttpClient.layer }; + readonly credential: { + readonly config: unknown; + readonly values: Record; + readonly template: string | null; + readonly connection: string; + readonly integration: string; + }; + }) => Effect.Effect + )({ + ctx: { httpClientLayer: FetchHttpClient.layer }, + credential: { + config, + values: {}, + template: null, + connection: "main", + integration: "single_instance_mcp", + }, + }); + }); + +const waitUntil = (predicate: () => boolean, timeoutMs: number) => + Effect.gen(function* () { + const deadline = Date.now() + timeoutMs; + while (!predicate()) { + if (Date.now() > deadline) return false; + yield* Effect.sleep(Duration.millis(50)); + } + return true; + }); + +const spawnedPids = (log: string): readonly number[] => + existsSync(log) + ? readFileSync(log, "utf8") + .split("\n") + .filter((line) => line.trim().length > 0) + .map((line) => Number(line)) + : []; + +describe("MCP liveness probe against a single-instance local stdio server", () => { + it.live( + "documents current behavior: the probe spawns a second child and reports the live server broken", + () => + Effect.gen(function* () { + const dir = mkdtempSync(join(tmpdir(), "mcp-single-instance-")); + const lockFile = join(dir, "lock"); + const spawnLog = join(dir, "spawns"); + const config = { + transport: "stdio" as const, + command: "bun", + args: ["run", fixture, lockFile, spawnLog], + }; + + // The instance a tool invocation would be holding: the pool keeps at most + // one idle connection per identity for five minutes, so during that window + // the server is up and serving. + let pooled: ChildProcess | undefined; + yield* Effect.acquireUseRelease( + Effect.gen(function* () { + pooled = spawn("bun", ["run", fixture, lockFile, spawnLog], { + stdio: ["pipe", "pipe", "pipe"], + }); + // Keep stdin open: the fixture exits when stdin ends, which is the + // same contract a pooled MCP child has. + pooled.stdin?.on("error", () => {}); + return yield* waitUntil(() => existsSync(lockFile), 20_000); + }), + (started) => + Effect.gen(function* () { + expect(started, "the pooled instance took the lock").toBe(true); + expect(spawnedPids(spawnLog), "one child so far").toHaveLength(1); + + const before = spawnedPids(spawnLog).length; + const verdict = yield* checkHealth(config); + const after = spawnedPids(spawnLog); + + // The probe did not reuse anything: it started another process. + expect(after.length, "the health probe spawned its own child").toBe(before + 1); + // The server is alive and holding the lock the whole time. + expect(existsSync(lockFile), "the pooled server is still running").toBe(true); + + // … and the verdict says the connection is broken, because the + // probe's OWN second instance could not start. + expect(verdict.status, "a live, serving server is reported unhealthy").not.toBe( + "healthy", + ); + return verdict; + }), + () => + Effect.sync(() => { + pooled?.stdin?.end(); + pooled?.kill("SIGTERM"); + }), + ); + void pooled; + }), + ); + + // Skipped, not deleted: this is the acceptance anchor for the R8 fix in + // plans/oauth-refresh-and-expired-status.md (Phase 3). The PR that lands the + // fix un-skips it and it must go green unchanged. + it.live.skip( + "REPRO: a probe must not report the connection broken for its own second spawn", + () => + Effect.gen(function* () { + const dir = mkdtempSync(join(tmpdir(), "mcp-single-instance-")); + const lockFile = join(dir, "lock"); + const spawnLog = join(dir, "spawns"); + const config = { + transport: "stdio" as const, + command: "bun", + args: ["run", fixture, lockFile, spawnLog], + }; + let pooled: ChildProcess | undefined; + yield* Effect.acquireUseRelease( + Effect.sync(() => { + pooled = spawn("bun", ["run", fixture, lockFile, spawnLog], { + stdio: ["pipe", "pipe", "pipe"], + }); + pooled.stdin?.on("error", () => {}); + }), + () => + Effect.gen(function* () { + expect(yield* waitUntil(() => existsSync(lockFile), 20_000)).toBe(true); + const verdict = yield* checkHealth(config); + // Phase 3/5 target: either answer from the live pooled connection, + // or classify "another instance of this server is already running" + // as the non-alarm it is. What it must not do is tell the user this + // credential/connection is broken. + expect(verdict.status, "a server that is up and serving reads healthy").toBe( + "healthy", + ); + }), + () => + Effect.sync(() => { + pooled?.stdin?.end(); + pooled?.kill("SIGTERM"); + }), + ); + }), + ); +}); diff --git a/packages/plugins/mcp/src/sdk/stdio-single-instance-test-server.ts b/packages/plugins/mcp/src/sdk/stdio-single-instance-test-server.ts new file mode 100644 index 0000000000..a752fc15fd --- /dev/null +++ b/packages/plugins/mcp/src/sdk/stdio-single-instance-test-server.ts @@ -0,0 +1,133 @@ +// Fixture for mcp-liveness-second-spawn.test.ts. A stdio MCP server that +// models a SINGLE-INSTANCE local server — the shape Chrome DevTools MCP, +// Playwright MCP and anything else that owns a browser, a debug port or a +// lock file has: a second concurrent process cannot start, and says so on +// stderr before exiting non-zero. +// +// argv[2] is the lock file, argv[3] a spawn log the test reads to count how +// many child processes a code path created. Every spawn appends its PID, so +// "did the health probe reuse a connection or start a new process?" is +// answerable from the file alone. + +import { appendFileSync, existsSync, readFileSync, unlinkSync, writeFileSync } from "node:fs"; + +const lockFile = process.argv[2]; +const spawnLog = process.argv[3]; +if (lockFile === undefined || spawnLog === undefined) { + process.stderr.write("usage: stdio-single-instance-test-server.ts \n"); + process.exit(2); +} + +appendFileSync(spawnLog, `${process.pid}\n`); + +const isAlive = (pid: number): boolean => { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: standalone non-Effect fixture process; kill(pid, 0) reports "gone" only by throwing + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +}; + +if (existsSync(lockFile)) { + const holder = Number(readFileSync(lockFile, "utf8").trim()); + if (Number.isFinite(holder) && holder !== process.pid && isAlive(holder)) { + // Exactly what a single-instance local server does when something already + // owns the resource: refuse to start and exit non-zero. + process.stderr.write( + `single-instance server: another instance (${holder}) is already running\n`, + ); + process.exit(1); + } +} + +writeFileSync(lockFile, String(process.pid)); + +const release = (): void => { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: fixture teardown must not throw on an already-removed lock + try { + if (existsSync(lockFile) && readFileSync(lockFile, "utf8").trim() === String(process.pid)) { + unlinkSync(lockFile); + } + } catch { + // already gone + } +}; +process.on("exit", release); +process.on("SIGTERM", () => { + release(); + process.exit(0); +}); + +const respond = (message: object): void => { + process.stdout.write(`${JSON.stringify(message)}\n`); +}; + +const handle = (line: string): void => { + if (!line.trim()) return; + let request: { + id?: number; + method?: string; + params?: { protocolVersion?: string; name?: string; arguments?: Record }; + }; + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: standalone fixture process; a malformed frame is dropped like a real server would + try { + // oxlint-disable-next-line executor/no-json-parse -- boundary: hand-rolled JSON-RPC framing is the fixture's entire purpose + request = JSON.parse(line); + } catch { + return; + } + if (request.method === "initialize") { + respond({ + jsonrpc: "2.0", + id: request.id, + result: { + protocolVersion: request.params?.protocolVersion, + capabilities: { tools: {} }, + serverInfo: { name: "stdio-single-instance-test-server", version: "0.0.0" }, + }, + }); + } else if (request.method === "tools/list") { + respond({ + jsonrpc: "2.0", + id: request.id, + result: { + tools: [ + { + name: "whoami", + description: "whoami", + inputSchema: { type: "object", properties: {}, additionalProperties: false }, + }, + ], + }, + }); + } else if (request.method === "tools/call") { + respond({ + jsonrpc: "2.0", + id: request.id, + result: { + content: [{ type: "text", text: `served by ${process.pid}` }], + isError: false, + }, + }); + } else if (request.id !== undefined) { + respond({ jsonrpc: "2.0", id: request.id, result: {} }); + } +}; + +let buffer = ""; +process.stdin.setEncoding("utf8"); +process.stdin.on("data", (chunk: string) => { + buffer += chunk; + let newline = buffer.indexOf("\n"); + while (newline >= 0) { + handle(buffer.slice(0, newline)); + buffer = buffer.slice(newline + 1); + newline = buffer.indexOf("\n"); + } +}); +process.stdin.on("end", () => { + release(); + process.exit(0); +}); diff --git a/plans/oauth-refresh-and-expired-status.md b/plans/oauth-refresh-and-expired-status.md new file mode 100644 index 0000000000..f7a0fe28bd --- /dev/null +++ b/plans/oauth-refresh-and-expired-status.md @@ -0,0 +1,456 @@ +# "Expired" is lying, and refresh races cause it — analysis + plan + +Status: analysis complete, plan proposed. No code changed yet. + +The complaint: on executor.sh connections show a red **Expired** badge that is +wrong (or unrecoverable), and token refresh does not behave as if it were +coordinated. Both halves are the same defect family — the health verdict and +the refresh machinery disagree about what is evidence — and one of them +(refresh races across cloud's request/DO boundaries) actively _manufactures_ +the false "Expired". + +Everything below cites current `main` (`a72e51d13`). + +--- + +## 1. How status and refresh work today + +**Refresh triggers** (`packages/core/sdk/src/executor.ts`) + +- Proactive: `resolveConnectionValues` (:2998) refreshes when + `shouldRefreshToken({ expiresAt })` — `expires_at <= now + 60s` + (`oauth-helpers.ts:1726`, `OAUTH2_REFRESH_SKEW_MS = 60_000`). A **null** + `expires_at` never fires proactively, by design. +- Reactive: `executor.execute` retries once on a tool 401 via + `forceRefreshConnectionValues` (:3049, call site :6642-6674). +- Dedup: `refreshInFlight` — a `WeakMap` keyed on the **root db handle object** + (:264, :1986-1990). Its own doc block states the limit: _"dedup reaches + exactly as far as one root DB handle in one process … Multi-instance + deployments are outside it … Both need database-backed coordination + (compare-and-swap on the stored refresh token)."_ +- Failure: a definitive rejection calls `markRefreshGrantDead` (:2294), which + writes `provider_state.oauthReauthRequiredAt` + an `expired` `last_health`. + +**What a dead grant means** — permanent, and derived on every read: + +- `performTokenRefresh` refuses to even send the grant (:2600-2630). +- `connectionCheckHealth` refuses to probe and answers `deadGrantVerdict` + (:5186-5195), including for the manual "Check now". +- `presentedLastHealth` (:1118) re-derives `expired` on **every** API read, so + no writer can bury it and `healPersistedHealthOnUse` (:4966) bails out. +- Only a reconnect (which rewrites `provider_state` wholesale) clears it. + +This gate is deliberate and earned: the Datadog incident (100+ identical +rejections over two days, comment at :2600) plus +`e2e/scenarios/connection-health-verdict.test.ts` and +`e2e/selfhost/mcp-oauth-reconnect-health.test.ts` pin it. **The plan keeps the +gate.** It fixes what feeds it and how little evidence it takes to trigger it. + +--- + +## 2. Root causes, ranked + +### R1 — In cloud, the refresh gate dedups _nothing_, and the loser bricks the connection (severity: critical) + +`apps/cloud/src/api/protected.ts:110-121` + `apps/cloud/src/api/layers.ts:38-46` +rebuild `DbService` **per request** (Cloudflare forbids sharing I/O across +handlers), and `cloudDbProviderLayer` rebuilds the fuma client off it +(`apps/cloud/src/db/fuma.ts:56-73`). So in the HTTP plane every request gets a new +db object → a new `WeakMap` entry → a fresh, empty gate. The MCP plane is +per-session (`session-durable-object.ts:156-160` builds one handle per DO), so +two sessions, or a session plus any HTTP request, are also mutually +undeduped. + +Consequence, with a rotating authorization server (the norm — our own test AS +rotates: `packages/core/sdk/src/testing/oauth-test-server.ts:876-887`): + +1. Surface A and surface B both read refresh token `R1`, both send a grant. +2. A wins, stores `R2` + a fresh access token, `expires_at` updated. +3. B is answered `invalid_grant` (reuse) → `markRefreshGrantDead` → + `provider_state.oauthReauthRequiredAt`. +4. `markRefreshGrantDead` (:2294-2336) is an unconditional `updateMany` — no + CAS, unlike `persistHealthResult` (:4917-4936) which CASes on + `updated_at`/`tools_synced_at`. Nothing ever re-checks that the token we + sent is still the token on the row, and `persistRefreshedToken` (:2363) + never clears the marker. + +Net: **a connection holding a perfectly valid rotated refresh token presents +`expired` forever**, on every surface, unreachable by probe or by use, until a +human re-consents. Worse, providers that treat reuse as theft revoke the whole +token family, so the race can kill the grant for real. + +The trigger surface is broad: at the moment a token goes due, every concurrent +touchpoint refreshes — parallel tool calls across sessions, a tool sync +(`#2028` runs sync in the background), a browser tab loading the accounts page +(use-connection-health probes with **no** freshness window for non-healthy +verdicts), the OAuth callback's catalog sync. + +### R2 — One 4xx is enough to declare a grant permanently dead (severity: high) + +`oauth-helpers.ts:73-91`: + +```ts +isUnusableSuccessTokenResponse = (e) => e.status !== undefined && e.status < 300; +isPermanentTokenRejection = (e) => + isUnusableSuccessTokenResponse(e) || (e.status >= 400 && e.status < 500); +``` + +and `executor.ts:2858-2872` maps that straight to `reauthRequired: true` → +dead grant. So these transient/ambiguous outcomes permanently brick a +connection: + +- **429** — a rate-limited token endpoint (very likely once R1 makes us send + duplicate grants, and likely under an AS incident). 429 is a 4xx. +- **408**, **425**, proxy/WAF **403** or **404** HTML pages, CDN edge errors. +- **2xx that is not a token response** — a captive-portal/challenge page, an + HTML 200 from a misrouted origin: `< 300` ⇒ dead grant. + +The §5.2 `invalid_grant` path (:2833-2857) is genuinely definitive and should +stay one-shot. Everything else is inference from an HTTP status and deserves a +second opinion. + +### R3 — The health probe never refreshes reactively, so it reports `expired` for connections that work (severity: high) + +`connectionCheckHealth` (:5240-5280) resolves credentials (proactive refresh +only) and hands them to the plugin probe. A 401 becomes +`classifyHttpStatus → "expired"` (`health-check.ts:208-213`) and is persisted. +Unlike `executor.execute`, there is **no** forced-refresh-and-retry. + +So for exactly the cases the reactive path was built for — server-side +revocation, an IdP idle timeout shorter than the advertised lifetime, and +**null `expires_at`** (AS omitted `expires_in`; `oauth-flow.test.ts:2508` +records 5 such rows in production) — a page load writes `expired`, the badge +goes red, and it only heals if the user happens to invoke a tool +(`healPersistedHealthOnUse`, :4966). A connection that would refresh fine on +next use is presented as dead. + +### R4 — `healthy` is asserted without evidence (severity: medium) + +For an OAuth connection on an integration with **no** declared `health_check` +spec, the probe is skipped entirely and the verdict is +`oauthCredentialHealthWithoutProbe` (:5045-5056, branch :5242-5250): +`{ status: "healthy", detail: "Credential resolved (no probe configured)." }` +— persisted, which then suppresses revalidation for 5 minutes +(`use-connection-health.ts:HEALTH_REVALIDATE_MS`). Reading a token out of the +vault proves nothing about the upstream. This is pinned by +`e2e/scenarios/google-health-checks.test.ts:381`, so it is intentional, but it +is the mirror image of R3: the same badge is both falsely red and falsely +green. It also skips plugins that _could_ probe without a spec (MCP's +`checkHealth` ignores `spec` and discovers tools: +`packages/plugins/mcp/src/sdk/plugin.ts:1941-1981`). + +### R5 — A refresh that omits `expires_in` erases the expiry (severity: medium) + +`persistRefreshedToken` (:2386-2390): +`expires_at = typeof token.expires_in === "number" ? now + expires_in*1000 : null`. +An AS that advertises a lifetime on the code exchange but omits it on refresh +(RFC 6749 makes it optional) drops the connection to null expiry **forever +after the first refresh** — proactive refresh can never fire again, so every +subsequent call pays a 401 + reactive refresh, and R3 turns each of those into +a red badge between uses. + +### R6 — Scope shortfalls and fuzzy text matching read as `expired` (severity: medium) + +- `classifyHttpStatus` maps **403 → expired**. The invoke path already knows + better: `detectInsufficientScope` (`packages/core/sdk/src/insufficient-scope.ts`, + used at `packages/plugins/openapi/src/sdk/backing.ts:777-800`) distinguishes + RFC 6750 `insufficient_scope` / Google `ACCESS_TOKEN_SCOPE_INSUFFICIENT`. The + probe path only carves out Google's _configuration_ 403s + (`health-check.ts:250-257`), so "you granted too few scopes" is rendered as + a red **Expired** + "reconnect to restore access", when the remedy is + re-consent and the row already carries `missingOAuthScopes`. +- GraphQL classifies on free text: + `packages/plugins/graphql/src/sdk/plugin.ts:118-121` marks `expired` for any + upstream message matching `/permission|credential|api.?key|sign in/i`, + including a 200-body error from an unrelated cause. + +### R7 — 60s skew, no background refresh (severity: low) + +`OAUTH2_REFRESH_SKEW_MS = 60_000` is thin next to a 20s token-request timeout +and an agent turn that can run for minutes; and refresh is call-time only, so +an idle connection's grant can age out (many ASes expire refresh tokens on +inactivity) with nobody looking. Also relevant: the health-probe gate is keyed +the same per-request way as the refresh gate, so the "N tabs collapse to one +probe" claim in `connections/api.ts:244-246` does not hold in cloud either. + +### R8 — the MCP liveness probe dials a SECOND connection, so single-instance local servers fail their own health check (severity: high, local) + +`checkHealth` in `packages/plugins/mcp/src/sdk/plugin.ts:1972-1994` builds a +fresh connector and calls `discoverToolsFromInput`, which creates a new +connection (`discover.ts:142` → `createMcpConnector`) with a 15s deadline. It +never takes the pooled connection that tool invocations use +(`connection-pool.ts`, one idle session per identity, five-minute TTL; +`invoke.ts:468-478`). For a remote server that costs a handshake. **For a +local stdio server it spawns a second child process** — and the common local +servers are single-instance: Chrome DevTools MCP owns a browser and a debug +port, Playwright MCP the same, `docker run -i` a container. The second process +cannot start and exits non-zero, so the probe reports the _connection_ broken +while the server is up and serving the pooled client. + +`mcpLivenessFailureStatus` (`plugin.ts:86-102`) then answers `degraded` for a +spawn failure or a timeout, and `use-connection-health.ts` re-probes every +non-healthy verdict on every mount with no freshness window — so each page load +spawns another child of a server that is already running. The badge goes amber +red, the next probe (once the pooled child is gone) says healthy: the +"local MCPs like Chrome show disconnected" flap. + +This is the one root cause that needs no OAuth, no rotation and no second +instance — it reproduces in a single-process local app, which is where the +symptom was reported. + +--- + +## 2b. Replication (done) + +Four executable repros, each a pair: a **"documents current behavior"** test +that passes on main today (the replication) and a **REPRO** test asserting the +target behavior. Each REPRO test fails on main, so it is checked in **skipped** +and is the acceptance anchor for its phase — that PR un-skips it and it must go +green unedited. + +`packages/core/sdk/src/oauth-expired-status-repro.test.ts` + +```sh +cd packages/core/sdk && npx vitest run src/oauth-expired-status-repro.test.ts +# 3 passed | 3 skipped (the skips are the REPRO targets) +# un-skip one to see it fail: it asserts the post-fix contract +``` + +- **R1** — two executors, two root db handles, one SQLite db, one shared + credential store, rotating test AS. A stalls after reading the stored refresh + token, B wins and rotates it, A resumes and redeems the consumed token. + Current behavior (passing test): `provider_state.oauthReauthRequiredAt` is + recorded, `checkHealth` answers `expired` without probing, and after the next + expiry **B cannot refresh either** — the AS receives zero further grants + while the store still holds B's valid rotated token. REPRO fails on + "a lost race must not record a dead grant". +- **R2** — the backing app's `token_url` is pointed at a fixture endpoint that + answers the first refresh grant with `429 Too Many Requests` and forwards + every later one to the real AS. Current behavior (passing test): one 429 ⇒ + `checkHealth` = `expired`, and the next call sends **no** grant even though + the endpoint is healthy again. REPRO fails on "a 429 does not end the grant". +- **R3** — declared health check, long-lived token, upstream revokes it. The + probe answers `expired` and persists it having sent **zero** refresh grants; + the very next `execute` re-mints reactively, succeeds, and heal-on-use flips + the row back to `healthy`. Same connection, seconds apart, no user action — + the reported "disconnected, then connected". REPRO fails on "a refreshable + revocation is not an expired connection". + +`packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts` (+ the +`stdio-single-instance-test-server.ts` fixture, which refuses to start while a +live process holds its lock, exactly like Chrome DevTools MCP) + +```sh +cd packages/plugins/mcp && npx vitest run src/sdk/mcp-liveness-second-spawn.test.ts +# 1 passed | 1 skipped +``` + +- **R8** — one instance is running and holding the lock. Current behavior + (passing test): the health probe spawns a **second** child (proven from the + fixture's spawn log), that child refuses to start, and the verdict for a + live, serving server is `degraded`. REPRO fails on "a server that is up and + serving reads healthy". + +Both new files are lint-clean (`oxlint -c .oxlintrc.jsonc`), formatted +(`oxfmt`), and typecheck clean (`tsgo --noEmit`) in their packages. + +**Which host sees what.** `apps/local` builds ONE executor over ONE SQLite +handle (`apps/local/src/executor.ts:212-233`, `createExecutorHandle`), so the +refresh gate does hold there: **R1 is cloud/multi-process only.** R3 and R8 +reproduce in a single-process local app, which matches the reported symptom +(Linear flapping disconnected→connected; local MCPs like Chrome reading +disconnected). R2 needs only one instance and a transient 4xx, so it applies +everywhere. + +--- + +## 3. Plan + +Phases are ordered so each lands independently green +(`format:check`, `lint`, `typecheck`, `test`) and the bleeding stops first. + +### Phase 0 — Reproduce and measure (DONE for the repros) + +1. Landed as `packages/core/sdk/src/oauth-expired-status-repro.test.ts` and + `packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts` (see §2b). + Each "documents current behavior" test is the replication; each REPRO test + is the acceptance anchor for its phase and stays red until that phase lands. + The REPRO tests ship skipped; each fix PR un-skips its own. + Note the existing two-instance test in `oauth-flow.test.ts` ("a refresher + paused after reading the stored token never writes it back over a peer's + rotated one") already builds this shape and asserts the _store_ survives — + it never looks at the row, which is why R1 went unnoticed. +2. Add span attributes now so production can size the problem before we change + it: `executor.oauth.refresh.race_suspected` (invalid_grant while the stored + token differs from the one sent — read-only observation), + `executor.oauth.dead_grant.status` (the HTTP status behind the rejection), + `executor.health.source=credential_only` share. Query dead-grant counts per + tenant/integration/reason from existing `executor.oauth.refresh.*` attrs. +3. Record the diagnosis in `MISTAKES.md` (AGENTS.md names it; the file does not + exist yet — create it with this entry). + +### Phase 1 — Stop bricking connections (R1 detection + R2 classification) + +Small, reviewable, and it removes the permanent-damage path even before real +coordination exists. + +1. **Rotation-aware `invalid_grant`** in `performTokenRefresh`: on rejection, + re-read the row and the stored refresh item. If the stored value differs + from the one we sent, a peer rotated it — do **not** mark dead; adopt the + peer's access token (read the primary item) and return it. Span: + `executor.oauth.refresh.outcome=adopted_peer_rotation`. +2. **Fingerprint + CAS on the dead-grant write.** Add + `connection.refresh_token_fp` (SHA-256 prefix of the refresh token, never + the token) written wherever the refresh item is written (the mint paths at + `executor.ts:4509`, `:4565`, `:4729`, fed by + `oauth-service.ts:2344-2430`; and `persistRefreshedToken`). `markRefreshGrantDead` + becomes CAS-guarded on the observed `refresh_token_fp` + `updated_at` + (same idiom as `persistHealthResult`; `updateMany` returns void, so + write-then-re-read decides, and a lost CAS is a silent no-op). A peer's + successful rotation now always beats a stale death certificate. +3. **Narrow `isPermanentTokenRejection`.** Definitive = §5.2 `invalid_grant`, + or an unusable **JSON** 2xx token body carrying an error code. Retryable = + 408, 425, 429, 5xx, transport, non-JSON 2xx (challenge/portal pages). + Other 4xx without a §5.2 code becomes a **strike**: record + `oauthRefreshRejectCount`/`oauthRefreshRejectAt` in `provider_state` and + mark dead on the second strike within a cooldown (e.g. 10 min). This keeps + the Datadog fix (a truly dead grant stops hammering the AS after two + attempts, not 100) without letting one WAF hiccup end a connection. +4. Tests: 429 / 5xx / transport / HTML-200 ⇒ no dead grant; two spaced 400s ⇒ + dead grant; single `invalid_grant` ⇒ dead grant immediately (existing + `oauth-refresh-rejected*.test.ts` must stay green); loser-adopts-rotation + from Phase 0's harness now asserts recovery. + +### Phase 2 — Coordinated refresh across instances (R1 root fix) + +Implement the coordination the `refreshGateFor` comment already prescribes, in +core so selfhost multi-process and cloud both get it. + +1. **DB lease on the connection row**: `refresh_lease_owner`, + `refresh_lease_expires_at` (short, e.g. 30s). Claim with a conditional + `updateMany` (`lease_expires_at IS NULL OR < now`), then re-read to learn + who won — `updateMany` gives no rowcount, so the re-read is the CAS. +2. Winner grants and persists; **losers wait bounded** (poll ~150 ms up to + ~10 s for `expires_at`/`refresh_token_fp` to change) then adopt the stored + access token. A lease that expires mid-grant degrades to today's behavior, + and Phase 1's adoption path catches it. +3. Keep the in-process `WeakMap` gate as the fast path so one executor never + pays a DB round trip for its own concurrency; the lease only arbitrates + _between_ handles. +4. Same treatment for `healthProbeGateFor` (R7's probe-stampede half) — one + lease, N readers adopt the persisted verdict. +5. Tests: two handles ⇒ exactly one grant at the AS (extend Phase 0 harness); + lease expiry ⇒ no deadlock, bounded wait; a crashed winner ⇒ the loser + proceeds after the lease lapses. e2e: `oauth-refresh-cross-instance.test.ts` + (cloud + selfhost) modeled on `oauth-refresh-cross-session.test.ts` but + driving two planes (an HTTP health probe racing an MCP tool call). + +### Phase 3 — Make the probe tell the truth (R3, R6, R8) + +1. **Reactive refresh in `connectionCheckHealth`**: when the probe answers 401 + (or plugin-equivalent auth wall), the connection is OAuth with a refresh + token and no recorded dead grant ⇒ force one refresh and re-probe **once**; + persist the second verdict. Span `executor.health.refresh_retried`. This is + the single change that makes the badge agree with what the next tool call + will do, and it is safe under Phase 2's lease. +2. **Scope-aware 403**: run `detectInsufficientScope` in the probe + classification and emit a distinct outcome (`degraded` + + `reason: insufficient_scope`, feeding the existing `missingOAuthScopes` / + "Reconnect to grant access" UX) instead of red **Expired**. +3. **Narrow GraphQL's `isAuthMessage`**: require an auth signal _and_ a + non-network reason; free-text "permission" alone stops meaning `expired`. +4. **MCP liveness must not dial a second connection (R8).** Take the pooled + connection when one exists for that identity (`connection-pool.ts`) instead + of `discoverToolsFromInput`'s fresh connector, so a probe of a stdio server + does not spawn a second child of a single-instance process. Where a fresh + dial is unavoidable, classify "another instance is already running" / + spawn-because-locked as non-alarm (`unknown`, never `degraded`/`expired`): + the server is up, the credential was never exercised. Add a floor to + non-healthy revalidation in `use-connection-health.ts` (today it sends no + `ifStaleMs` at all, so every mount of every surface re-probes — and for + stdio, re-spawns). +5. Tests: probe-401-then-refresh-then-healthy persists `healthy`; + null-expiry connection heals from a page load alone (today it needs a tool + call); insufficient*scope renders the reconsent affordance, not Expired; + the MCP liveness probe of a live single-instance stdio server answers + healthy and spawns no second child (flip + `mcp-liveness-second-spawn.test.ts`'s REPRO). + e2e: `health-probe-refresh-recovery.test.ts`; keep + `connection-health-verdict.test.ts` green (a \_refused* refresh still ends at + `expired`, persisted, with the freshness window intact). + +### Phase 4 — Honest verdicts and durable expiry (R4, R5) + +1. **Preserve the advertised lifetime**: store the lifetime seen at mint (or + any refresh) in `provider_state.oauthTokenLifetimeMs`; when a refresh + response omits `expires_in`, derive `expires_at` from it instead of writing + null. Null stays only for grants that were never advertised a lifetime. +2. **Evidence-tagged `healthy`**: the credential-only path keeps `healthy` when + it actually refreshed (real evidence) and otherwise answers `unknown` with + detail "Credential present; not verified against the upstream." Also let + plugins that need no spec probe without one (MCP tool discovery), so fewer + connections sit unverified. This changes + `google-health-checks.test.ts:381` deliberately — call it out in the PR. +3. Decide the UX for `unknown`: grey dot, no alarm copy, and a "Check now" + that probes for real (`health-display.ts` already keeps `unknown` neutral). + +### Phase 5 — Recovery affordance and prevention (R2 aftermath, R7) + +1. **"Retry refresh" next to Reconnect** on a dead grant: one re-armed attempt + under the Phase 1 CAS (clears the marker only if the grant succeeds), so a + spuriously bricked connection recovers without re-consent. Keep Reconnect as + the primary action; keep the gate's "no probing while dead" rule for + automatic surfaces — this is an explicit human action. +2. **Copy**: split "Token refresh was rejected — reconnect" from "Upstream + rejected the credential" (`accounts-section.tsx:196`). Show the recorded + reason and when. +3. **Skew**: `max(60s, 10% of the advertised lifetime)`, host-overridable. +4. **Optional, separate decision — background refresh cron** in cloud + (`wrangler.jsonc` already runs a `* * * * *` cron): proactively refresh + tokens for connections used in the last N days. It removes idle-lapse and + makes one coordinated refresher the common path instead of N racing + surfaces. Needs its own design note (cost, org scoping, WorkOS Vault QPS) + — do not fold it into Phases 1-4. +5. **Alert** on dead-grant rate per tenant/integration and on + `race_suspected`, so the next incident is a page rather than a support + thread. + +--- + +## 4. What must not regress + +- The known-dead gate itself: a genuinely dead grant must stop generating + refresh traffic after a bounded number of attempts and must present + `expired` on every read (`connections.test.ts:2810`, `:2985`, `:3084`, + `:3119`). +- Verdict writes stay best-effort and CAS-guarded; a dead grant recorded + mid-probe still survives the probe's write. +- Reactive tool-call retry stays exactly one retry, 401-only, refresh-token + holders only (`oauth-refresh-on-401.test.ts`). +- Single-flight refresh within one process (`oauth-refresh-cross-session.test.ts`). +- Interrupting a dial must still tear down the stdio child (`#1631`, + `stdio-interrupt-cleanup.test.ts`): routing the liveness probe through the + pool changes WHO owns the child, and the pooled child's lifetime is the + pool's — a probe must not close a connection invocations still need, and an + interrupted probe must not strand one. +- The store-writability probe before spending a single-use refresh token + (`#1377`) — and note it writes an item per refresh that is never deleted; + worth a cleanup task, not a blocker. +- Nothing secret-bearing in spans, health `detail`, or the new fingerprint + column (hash only; `redactTokenEndpointBody`'s allowlist governs rendering). + +## 5. Suggested PR boundaries + +1. Phase 0 (tests + telemetry + MISTAKES entry) — no behavior change. +2. Phase 1.1-1.2 (rotation adoption + fingerprint CAS). +3. Phase 1.3 (classification narrowing + strikes). +4. Phase 2 (lease) — the largest; ship behind a config flag defaulting on, with + the flag removed in a follow-up. +5. Phase 3 (probe refresh + scope-aware 403 + GraphQL narrowing + MCP liveness + reusing the pool). R8 is independently shippable and is the one fix that + addresses the reported local symptom on its own — it can lead Phase 3 or + ship before it. +6. Phase 4, then Phase 5. + +Each PR: narrowest meaningful vitest while iterating, one named e2e scenario +when the change is user-visible, `bun run format` before opening. From dc52a0dbcf9a2a9c19ebc1e5c237315bb133c0af Mon Sep 17 00:00:00 2001 From: spa5k <79936503+spa5k@users.noreply.github.com> Date: Thu, 17 Sep 2026 19:32:50 +0530 Subject: [PATCH 2/2] Rewrite the expired-status plan in Simplified Technical English The plan document now follows ASD-STE100. Sentences are short. The voice is active. Each term names one concept. A new Terms section defines them. The metaphors are gone. Paragraphs keep normal prose wrapping; a sentence does not start a new paragraph. The technical content does not change. The eight causes keep their R1 to R8 identifiers, their evidence citations, and their rank order. The six phases, the invariants, and the pull request boundaries are the same work. --- plans/oauth-refresh-and-expired-status.md | 913 ++++++++++++---------- 1 file changed, 502 insertions(+), 411 deletions(-) diff --git a/plans/oauth-refresh-and-expired-status.md b/plans/oauth-refresh-and-expired-status.md index f7a0fe28bd..1db2761860 100644 --- a/plans/oauth-refresh-and-expired-status.md +++ b/plans/oauth-refresh-and-expired-status.md @@ -1,93 +1,129 @@ -# "Expired" is lying, and refresh races cause it — analysis + plan +# The wrong Expired status: analysis and plan -Status: analysis complete, plan proposed. No code changed yet. +Status: the analysis is complete and the plan is proposed. This branch adds +tests and this document. It does not change runtime behavior. -The complaint: on executor.sh connections show a red **Expired** badge that is -wrong (or unrecoverable), and token refresh does not behave as if it were -coordinated. Both halves are the same defect family — the health verdict and -the refresh machinery disagree about what is evidence — and one of them -(refresh races across cloud's request/DO boundaries) actively _manufactures_ -the false "Expired". +## The problem -Everything below cites current `main` (`a72e51d13`). +Connections show the health status **Expired**. The status is sometimes wrong. +The status is sometimes permanent. Token refresh also gives the impression that +nothing coordinates it. These two problems have one origin. The health status +and the refresh mechanism do not agree on what counts as evidence. One defect +in the refresh mechanism also produces the wrong status. ---- - -## 1. How status and refresh work today - -**Refresh triggers** (`packages/core/sdk/src/executor.ts`) - -- Proactive: `resolveConnectionValues` (:2998) refreshes when - `shouldRefreshToken({ expiresAt })` — `expires_at <= now + 60s` - (`oauth-helpers.ts:1726`, `OAUTH2_REFRESH_SKEW_MS = 60_000`). A **null** - `expires_at` never fires proactively, by design. -- Reactive: `executor.execute` retries once on a tool 401 via - `forceRefreshConnectionValues` (:3049, call site :6642-6674). -- Dedup: `refreshInFlight` — a `WeakMap` keyed on the **root db handle object** - (:264, :1986-1990). Its own doc block states the limit: _"dedup reaches - exactly as far as one root DB handle in one process … Multi-instance - deployments are outside it … Both need database-backed coordination - (compare-and-swap on the stored refresh token)."_ -- Failure: a definitive rejection calls `markRefreshGrantDead` (:2294), which - writes `provider_state.oauthReauthRequiredAt` + an `expired` `last_health`. - -**What a dead grant means** — permanent, and derived on every read: - -- `performTokenRefresh` refuses to even send the grant (:2600-2630). -- `connectionCheckHealth` refuses to probe and answers `deadGrantVerdict` - (:5186-5195), including for the manual "Check now". -- `presentedLastHealth` (:1118) re-derives `expired` on **every** API read, so - no writer can bury it and `healPersistedHealthOnUse` (:4966) bails out. -- Only a reconnect (which rewrites `provider_state` wholesale) clears it. - -This gate is deliberate and earned: the Datadog incident (100+ identical -rejections over two days, comment at :2600) plus -`e2e/scenarios/connection-health-verdict.test.ts` and -`e2e/selfhost/mcp-oauth-reconnect-health.test.ts` pin it. **The plan keeps the -gate.** It fixes what feeds it and how little evidence it takes to trigger it. - ---- - -## 2. Root causes, ranked +All citations in this document refer to `main` at commit `a72e51d13`. -### R1 — In cloud, the refresh gate dedups _nothing_, and the loser bricks the connection (severity: critical) +## Terms -`apps/cloud/src/api/protected.ts:110-121` + `apps/cloud/src/api/layers.ts:38-46` -rebuild `DbService` **per request** (Cloudflare forbids sharing I/O across -handlers), and `cloudDbProviderLayer` rebuilds the fuma client off it -(`apps/cloud/src/db/fuma.ts:56-73`). So in the HTTP plane every request gets a new -db object → a new `WeakMap` entry → a fresh, empty gate. The MCP plane is -per-session (`session-durable-object.ts:156-160` builds one handle per DO), so -two sessions, or a session plus any HTTP request, are also mutually -undeduped. +This document uses one term for one concept. -Consequence, with a rotating authorization server (the norm — our own test AS -rotates: `packages/core/sdk/src/testing/oauth-test-server.ts:876-887`): +- **Connection**: one stored credential, identified by owner, integration, and + name. +- **Health status**: the value in `connection.last_health`. The values are + `healthy`, `expired`, `degraded`, `misconfigured`, and `unknown`. +- **Probe**: one run of an integration's health check against the upstream. +- **Refresh grant**: one request to the authorization server (AS) for a new + access token. +- **Permanent rejection record**: the object + `provider_state.oauthReauthRequiredAt`. The system writes it when it decides + that a refresh token is permanently rejected. +- **Instance**: one executor with its own root database handle. Two instances + can run in one process or in two processes. -1. Surface A and surface B both read refresh token `R1`, both send a grant. -2. A wins, stores `R2` + a fresh access token, `expires_at` updated. -3. B is answered `invalid_grant` (reuse) → `markRefreshGrantDead` → - `provider_state.oauthReauthRequiredAt`. -4. `markRefreshGrantDead` (:2294-2336) is an unconditional `updateMany` — no - CAS, unlike `persistHealthResult` (:4917-4936) which CASes on - `updated_at`/`tools_synced_at`. Nothing ever re-checks that the token we - sent is still the token on the row, and `persistRefreshedToken` (:2363) - never clears the marker. - -Net: **a connection holding a perfectly valid rotated refresh token presents -`expired` forever**, on every surface, unreachable by probe or by use, until a -human re-consents. Worse, providers that treat reuse as theft revoke the whole -token family, so the race can kill the grant for real. +--- -The trigger surface is broad: at the moment a token goes due, every concurrent -touchpoint refreshes — parallel tool calls across sessions, a tool sync -(`#2028` runs sync in the background), a browser tab loading the accounts page -(use-connection-health probes with **no** freshness window for non-healthy -verdicts), the OAuth callback's catalog sync. +## 1. How the health status and the token refresh work today + +### Refresh triggers + +The code is in `packages/core/sdk/src/executor.ts`. + +- **Proactive.** `resolveConnectionValues` (:2998) refreshes the token when + `shouldRefreshToken({ expiresAt })` returns true. That function + (`oauth-helpers.ts:1726`) compares `expires_at` with the current time plus a + 60 second skew (`OAUTH2_REFRESH_SKEW_MS = 60_000`). A null `expires_at` + never starts a proactive refresh. This is deliberate. +- **Reactive.** `executor.execute` retries one time when a tool call receives a + 401 response. It calls `forceRefreshConnectionValues` (:3049). The call site + is :6642-6674. +- **Deduplication.** `refreshInFlight` is a `WeakMap` (:264, :1986-1990). The + key is the root database handle object. The documentation of that map states + the limit: deduplication reaches only as far as one root database handle in + one process. It states that multi-instance deployments are outside that + limit. It recommends coordination in the database with a compare-and-set on + the stored refresh token. +- **Failure.** A definitive rejection calls `markRefreshGrantDead` (:2294). + That function writes the permanent rejection record and an `expired` health + status. + +### The permanent rejection record + +The record is permanent. Every read derives the status from it. + +- `performTokenRefresh` does not send the grant (:2600-2630). +- `connectionCheckHealth` does not probe. It answers `deadGrantVerdict` + (:5186-5195). This includes the manual "Check now" action. +- `presentedLastHealth` (:1118) derives `expired` on every API read. No writer + can replace it. `healPersistedHealthOnUse` (:4966) stops when it sees the + record. +- Only a reconnect removes it. A reconnect writes a new `provider_state` + object. + +This gate is deliberate and it has a reason. One incident produced more than +100 identical rejections in two days (the comment at :2600). Two e2e scenarios +pin the behavior: `e2e/scenarios/connection-health-verdict.test.ts` and +`e2e/selfhost/mcp-oauth-reconnect-health.test.ts`. **This plan keeps the gate.** +The plan changes what writes the record and how much evidence the system needs +before it writes it. -### R2 — One 4xx is enough to declare a grant permanently dead (severity: high) +--- -`oauth-helpers.ts:73-91`: +## 2. Causes in rank order + +### R1 — The refresh gate does not work in the cloud app, and the losing instance makes a valid connection permanently Expired (severity: critical) + +`apps/cloud/src/api/protected.ts:110-121` and +`apps/cloud/src/api/layers.ts:38-46` rebuild `DbService` for each request. +Cloudflare Workers forbids one I/O object in two request handlers. +`cloudDbProviderLayer` then rebuilds the fuma client from that service +(`apps/cloud/src/db/fuma.ts:56-73`). Each request therefore gets a new database +object. A new database object gets a new `WeakMap` entry. The gate is empty for +every request in the HTTP plane. The MCP plane is per session: +`session-durable-object.ts:156-160` builds one handle for each Durable Object. +Two sessions do not share a gate. One session and one HTTP request do not share +a gate. + +The consequence follows when the AS rotates refresh tokens. Rotation is the +normal case. The test AS in this repository rotates +(`packages/core/sdk/src/testing/oauth-test-server.ts:876-887`). + +1. Instance A and instance B both read the refresh token `R1`. Both send a + refresh grant. +2. Instance A receives the answer first. It stores `R2` and a new access + token. It updates `expires_at`. +3. Instance B receives `invalid_grant` because the AS consumed `R1`. It calls + `markRefreshGrantDead`. It writes the permanent rejection record. +4. `markRefreshGrantDead` (:2294-2336) is an unconditional `updateMany`. It has + no compare-and-set. Compare this with `persistHealthResult` (:4917-4936), + which uses `updated_at` and `tools_synced_at` as the compare-and-set. No + code examines whether the token that the instance sent is still the token on + the row. `persistRefreshedToken` (:2363) does not remove the record. + +The result is this: **a connection that holds a valid rotated refresh token +shows `expired` forever.** Every surface shows it. No probe and no tool call +can change it. Only a human re-consent removes it. There is a second risk. Some +providers treat token reuse as theft and revoke the whole token family. The +race can then destroy the grant. + +Many surfaces can start the race. At the moment a token becomes due, each +concurrent surface refreshes it. These surfaces exist: parallel tool calls in +two sessions, a background tool sync (`#2028`), a browser tab that loads the +accounts page (`use-connection-health.ts` sends no freshness window for a +non-healthy status), and the catalog sync after an OAuth callback. + +### R2 — One 4xx response is enough to declare a grant permanently rejected (severity: high) + +The classifier is in `oauth-helpers.ts:73-91`: ```ts isUnusableSuccessTokenResponse = (e) => e.status !== undefined && e.status < 300; @@ -95,362 +131,417 @@ isPermanentTokenRejection = (e) => isUnusableSuccessTokenResponse(e) || (e.status >= 400 && e.status < 500); ``` -and `executor.ts:2858-2872` maps that straight to `reauthRequired: true` → -dead grant. So these transient/ambiguous outcomes permanently brick a -connection: - -- **429** — a rate-limited token endpoint (very likely once R1 makes us send - duplicate grants, and likely under an AS incident). 429 is a 4xx. -- **408**, **425**, proxy/WAF **403** or **404** HTML pages, CDN edge errors. -- **2xx that is not a token response** — a captive-portal/challenge page, an - HTML 200 from a misrouted origin: `< 300` ⇒ dead grant. - -The §5.2 `invalid_grant` path (:2833-2857) is genuinely definitive and should -stay one-shot. Everything else is inference from an HTTP status and deserves a -second opinion. - -### R3 — The health probe never refreshes reactively, so it reports `expired` for connections that work (severity: high) - -`connectionCheckHealth` (:5240-5280) resolves credentials (proactive refresh -only) and hands them to the plugin probe. A 401 becomes -`classifyHttpStatus → "expired"` (`health-check.ts:208-213`) and is persisted. -Unlike `executor.execute`, there is **no** forced-refresh-and-retry. - -So for exactly the cases the reactive path was built for — server-side -revocation, an IdP idle timeout shorter than the advertised lifetime, and -**null `expires_at`** (AS omitted `expires_in`; `oauth-flow.test.ts:2508` -records 5 such rows in production) — a page load writes `expired`, the badge -goes red, and it only heals if the user happens to invoke a tool -(`healPersistedHealthOnUse`, :4966). A connection that would refresh fine on -next use is presented as dead. - -### R4 — `healthy` is asserted without evidence (severity: medium) - -For an OAuth connection on an integration with **no** declared `health_check` -spec, the probe is skipped entirely and the verdict is -`oauthCredentialHealthWithoutProbe` (:5045-5056, branch :5242-5250): -`{ status: "healthy", detail: "Credential resolved (no probe configured)." }` -— persisted, which then suppresses revalidation for 5 minutes -(`use-connection-health.ts:HEALTH_REVALIDATE_MS`). Reading a token out of the -vault proves nothing about the upstream. This is pinned by -`e2e/scenarios/google-health-checks.test.ts:381`, so it is intentional, but it -is the mirror image of R3: the same badge is both falsely red and falsely -green. It also skips plugins that _could_ probe without a spec (MCP's -`checkHealth` ignores `spec` and discovers tools: -`packages/plugins/mcp/src/sdk/plugin.ts:1941-1981`). - -### R5 — A refresh that omits `expires_in` erases the expiry (severity: medium) - -`persistRefreshedToken` (:2386-2390): -`expires_at = typeof token.expires_in === "number" ? now + expires_in*1000 : null`. -An AS that advertises a lifetime on the code exchange but omits it on refresh -(RFC 6749 makes it optional) drops the connection to null expiry **forever -after the first refresh** — proactive refresh can never fire again, so every -subsequent call pays a 401 + reactive refresh, and R3 turns each of those into -a red badge between uses. - -### R6 — Scope shortfalls and fuzzy text matching read as `expired` (severity: medium) - -- `classifyHttpStatus` maps **403 → expired**. The invoke path already knows - better: `detectInsufficientScope` (`packages/core/sdk/src/insufficient-scope.ts`, - used at `packages/plugins/openapi/src/sdk/backing.ts:777-800`) distinguishes - RFC 6750 `insufficient_scope` / Google `ACCESS_TOKEN_SCOPE_INSUFFICIENT`. The - probe path only carves out Google's _configuration_ 403s - (`health-check.ts:250-257`), so "you granted too few scopes" is rendered as - a red **Expired** + "reconnect to restore access", when the remedy is - re-consent and the row already carries `missingOAuthScopes`. -- GraphQL classifies on free text: - `packages/plugins/graphql/src/sdk/plugin.ts:118-121` marks `expired` for any - upstream message matching `/permission|credential|api.?key|sign in/i`, - including a 200-body error from an unrelated cause. - -### R7 — 60s skew, no background refresh (severity: low) - -`OAUTH2_REFRESH_SKEW_MS = 60_000` is thin next to a 20s token-request timeout -and an agent turn that can run for minutes; and refresh is call-time only, so -an idle connection's grant can age out (many ASes expire refresh tokens on -inactivity) with nobody looking. Also relevant: the health-probe gate is keyed -the same per-request way as the refresh gate, so the "N tabs collapse to one -probe" claim in `connections/api.ts:244-246` does not hold in cloud either. - -### R8 — the MCP liveness probe dials a SECOND connection, so single-instance local servers fail their own health check (severity: high, local) - -`checkHealth` in `packages/plugins/mcp/src/sdk/plugin.ts:1972-1994` builds a -fresh connector and calls `discoverToolsFromInput`, which creates a new -connection (`discover.ts:142` → `createMcpConnector`) with a 15s deadline. It -never takes the pooled connection that tool invocations use -(`connection-pool.ts`, one idle session per identity, five-minute TTL; -`invoke.ts:468-478`). For a remote server that costs a handshake. **For a -local stdio server it spawns a second child process** — and the common local -servers are single-instance: Chrome DevTools MCP owns a browser and a debug -port, Playwright MCP the same, `docker run -i` a container. The second process -cannot start and exits non-zero, so the probe reports the _connection_ broken -while the server is up and serving the pooled client. - -`mcpLivenessFailureStatus` (`plugin.ts:86-102`) then answers `degraded` for a -spawn failure or a timeout, and `use-connection-health.ts` re-probes every -non-healthy verdict on every mount with no freshness window — so each page load -spawns another child of a server that is already running. The badge goes amber -red, the next probe (once the pooled child is gone) says healthy: the -"local MCPs like Chrome show disconnected" flap. - -This is the one root cause that needs no OAuth, no rotation and no second -instance — it reproduces in a single-process local app, which is where the -symptom was reported. +`executor.ts:2858-2872` maps that result directly to `reauthRequired: true` and +then to the permanent rejection record. These temporary or unclear results +therefore end a connection permanently: + +- **429.** The token endpoint limits the request rate. This is likely when R1 + makes the system send duplicate grants. It is also likely during an incident + at the AS. 429 is in the range 400 to 499. +- **408, 425, a proxy or WAF 403, a 404 HTML page, a CDN edge error.** +- **A 2xx response that is not a token response.** Examples are a + captive-portal page and an HTML 200 response from a wrong origin. The + condition `status < 300` is true, so the system writes the record. + +The §5.2 `invalid_grant` path (:2833-2857) is definitive. It should stay a +one-shot decision. Every other case is an inference from an HTTP status code. +Those cases need a second confirmation. + +### R3 — The probe does not refresh, so it reports `expired` for a connection that works (severity: high) + +`connectionCheckHealth` (:5240-5280) resolves the credential and gives it to +the plugin probe. Resolution performs the proactive refresh only. A 401 +response becomes `expired` through `classifyHttpStatus` +(`health-check.ts:208-213`) and the system persists that status. There is no +forced refresh and no second probe. `executor.execute` has both. + +The affected cases are exactly the cases that the reactive path exists for. +They are: a server-side revocation, an identity provider idle timeout that is +shorter than the advertised lifetime, and a null `expires_at` because the AS +omitted `expires_in` (`oauth-flow.test.ts:2508` records five such rows in +production). In these cases one page load writes `expired`. The indicator turns +red. The status changes to `healthy` only when the user calls a tool, because +`healPersistedHealthOnUse` (:4966) then runs. The user sees a connection that +does not work, and that connection would refresh correctly on the next call. + +### R4 — The system reports `healthy` without evidence (severity: medium) + +An OAuth connection on an integration with no declared `health_check` spec does +not probe. The branch at :5242-5250 selects +`oauthCredentialHealthWithoutProbe` (:5045-5056). The result is +`{ status: "healthy", detail: "Credential resolved (no probe configured)." }`. +The system persists it. A persisted healthy status then suppresses +revalidation for five minutes (`HEALTH_REVALIDATE_MS` in +`use-connection-health.ts`). Reading a token from the credential store says +nothing about the upstream. +`e2e/scenarios/google-health-checks.test.ts:381` pins this behavior, so it is +intentional. It is still the opposite error to R3: the same indicator is +wrongly red in one case and wrongly green in the other. This branch also skips +plugins that could probe without a spec. The MCP `checkHealth` ignores `spec` +and discovers tools (`packages/plugins/mcp/src/sdk/plugin.ts:1941-1981`). + +### R5 — A refresh response without `expires_in` erases the expiry (severity: medium) + +`persistRefreshedToken` (:2386-2390) sets `expires_at` to +`now + expires_in * 1000` when the response has `expires_in`, and to null when +it does not. RFC 6749 makes `expires_in` optional. An AS that sends a lifetime +in the code exchange but omits it in the refresh response therefore sets +`expires_at` to null after the first refresh. Proactive refresh can then never +run again. Every later call receives a 401 and pays a reactive refresh. R3 then +turns each of those calls into a red indicator between uses. + +### R6 — A scope shortfall and a text match report `expired` (severity: medium) + +- `classifyHttpStatus` maps a 403 response to `expired`. The invoke path + already distinguishes this case: `detectInsufficientScope` + (`packages/core/sdk/src/insufficient-scope.ts`) detects RFC 6750 + `insufficient_scope` and the Google `ACCESS_TOKEN_SCOPE_INSUFFICIENT` error. + `packages/plugins/openapi/src/sdk/backing.ts:777-800` uses it. The probe path + carves out only the Google configuration 403 (`health-check.ts:250-257`). A + connection with too few scopes therefore shows red **Expired** and the text + "reconnect to restore access". The correct remedy is a new consent. The row + already carries `missingOAuthScopes`. +- The GraphQL plugin classifies free text. + `packages/plugins/graphql/src/sdk/plugin.ts:118-121` reports `expired` for an + upstream message that matches + `/permission|credential|api.?key|sign in/i`. An unrelated error in a 200 + response body can match that pattern. + +### R7 — The skew is 60 seconds and no background refresh exists (severity: low) + +`OAUTH2_REFRESH_SKEW_MS = 60_000` is short next to a 20 second token request +timeout and an agent turn that can run for minutes. Refresh happens only at +call time. An idle connection can therefore lose its grant, because many +authorization servers expire a refresh token after a period of inactivity. One +more fact is relevant: the health probe gate uses the same per-request key as +the refresh gate. The statement in `connections/api.ts:244-246` — that open +tabs cannot stampede an upstream — is therefore not true in the cloud app. + +### R8 — The MCP probe makes a second connection, so a single-instance local server fails its own health check (severity: high, local) + +`checkHealth` in `packages/plugins/mcp/src/sdk/plugin.ts:1972-1994` builds a new +connector and calls `discoverToolsFromInput`. That function creates a new +connection (`discover.ts:142`, then `createMcpConnector`) with a 15 second +deadline. It does not use the pooled connection that tool calls use +(`connection-pool.ts` keeps one idle session per identity for five minutes; +`invoke.ts:468-478` takes it). For a remote server this costs one handshake. +**For a local stdio server it starts a second child process.** The common local +servers permit one instance only. Chrome DevTools MCP owns a browser and a debug +port. Playwright MCP does the same. `docker run -i` owns a container. The second +process cannot start and exits with a non-zero code. The probe then reports that +the connection does not work, while the server runs and serves the pooled +client. + +`mcpLivenessFailureStatus` (`plugin.ts:86-102`) answers `degraded` for a failed +spawn and for a timeout. `use-connection-health.ts` then probes again on every +mount for a non-healthy status, with no freshness window. Each page load +therefore starts one more child process of a server that already runs. The +indicator turns amber. The next probe runs after the pooled child is gone and +reports `healthy`. This is the reported change between disconnected and +connected for local MCP servers. + +This cause needs no OAuth, no token rotation, and no second instance. It +reproduces in a single-process local app. That is where the user reported the +symptom. --- -## 2b. Replication (done) +## 3. Replication -Four executable repros, each a pair: a **"documents current behavior"** test -that passes on main today (the replication) and a **REPRO** test asserting the -target behavior. Each REPRO test fails on main, so it is checked in **skipped** -and is the acceptance anchor for its phase — that PR un-skips it and it must go -green unedited. +Four causes have executable tests. Each cause has two tests. The first test +shows the behavior on `main` today and passes. The second test gives the +required behavior after the fix and fails on `main`. The test suite therefore +skips the second test. The pull request that makes the fix removes the skip. +The test must then pass without changes. -`packages/core/sdk/src/oauth-expired-status-repro.test.ts` +### The OAuth and health tests + +File: `packages/core/sdk/src/oauth-expired-status-repro.test.ts`. ```sh cd packages/core/sdk && npx vitest run src/oauth-expired-status-repro.test.ts -# 3 passed | 3 skipped (the skips are the REPRO targets) -# un-skip one to see it fail: it asserts the post-fix contract +# 3 passed | 3 skipped (the skipped tests are the fix targets) +# Remove one skip to see that test fail on main. ``` -- **R1** — two executors, two root db handles, one SQLite db, one shared - credential store, rotating test AS. A stalls after reading the stored refresh - token, B wins and rotates it, A resumes and redeems the consumed token. - Current behavior (passing test): `provider_state.oauthReauthRequiredAt` is - recorded, `checkHealth` answers `expired` without probing, and after the next - expiry **B cannot refresh either** — the AS receives zero further grants - while the store still holds B's valid rotated token. REPRO fails on - "a lost race must not record a dead grant". -- **R2** — the backing app's `token_url` is pointed at a fixture endpoint that - answers the first refresh grant with `429 Too Many Requests` and forwards - every later one to the real AS. Current behavior (passing test): one 429 ⇒ - `checkHealth` = `expired`, and the next call sends **no** grant even though - the endpoint is healthy again. REPRO fails on "a 429 does not end the grant". -- **R3** — declared health check, long-lived token, upstream revokes it. The - probe answers `expired` and persists it having sent **zero** refresh grants; - the very next `execute` re-mints reactively, succeeds, and heal-on-use flips - the row back to `healthy`. Same connection, seconds apart, no user action — - the reported "disconnected, then connected". REPRO fails on "a refreshable - revocation is not an expired connection". - -`packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts` (+ the -`stdio-single-instance-test-server.ts` fixture, which refuses to start while a -live process holds its lock, exactly like Chrome DevTools MCP) +- **R1.** The test makes two executors with two root database handles over one + SQLite database and one shared credential store. The test AS rotates refresh + tokens. Instance A stops after it reads the stored refresh token. Instance B + completes a refresh and rotates the token. Instance A then sends the consumed + token. The passing test shows this behavior: the system writes + `provider_state.oauthReauthRequiredAt`; `checkHealth` answers `expired` + without a probe; after the next expiry instance B cannot refresh; the AS + receives zero further grants, although the store holds the valid rotated + token of instance B. The skipped test fails on the assertion "a lost race + must not record a dead grant". +- **R2.** The test points the `token_url` of the backing app at a fixture + endpoint. That endpoint answers the first refresh grant with + `429 Too Many Requests` and forwards every later grant to the real AS. The + passing test shows this behavior: one 429 gives `expired` from `checkHealth`, + and the next call sends no grant, although the endpoint is healthy again. The + skipped test fails on the assertion "a 429 does not end the grant". +- **R3.** The test declares a health check, uses a long-lived token, and then + revokes that token at the upstream. The passing test shows this behavior: the + probe answers `expired` and persists it after zero refresh grants; the next + `execute` refreshes, succeeds, and writes `healthy` to the same row. The + skipped test fails on the assertion "a refreshable revocation is not an + expired connection". + +### The MCP test + +Files: `packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts` and the +fixture `stdio-single-instance-test-server.ts`. The fixture does not start +while a live process holds its lock. Chrome DevTools MCP has the same shape. ```sh cd packages/plugins/mcp && npx vitest run src/sdk/mcp-liveness-second-spawn.test.ts # 1 passed | 1 skipped ``` -- **R8** — one instance is running and holding the lock. Current behavior - (passing test): the health probe spawns a **second** child (proven from the - fixture's spawn log), that child refuses to start, and the verdict for a - live, serving server is `degraded`. REPRO fails on "a server that is up and - serving reads healthy". +- **R8.** One instance runs and holds the lock. The passing test shows this + behavior: the probe starts a second child process, and the spawn log of the + fixture proves it; the second process does not start; the probe answers + `degraded` for a server that runs and serves requests. The skipped test fails + on the assertion "a server that is up and serving reads healthy". + +### Quality gates for the new files -Both new files are lint-clean (`oxlint -c .oxlintrc.jsonc`), formatted -(`oxfmt`), and typecheck clean (`tsgo --noEmit`) in their packages. +Both new test files and the fixture pass `oxlint -c .oxlintrc.jsonc`, pass +`oxfmt`, and give no `tsgo --noEmit` errors in their packages. -**Which host sees what.** `apps/local` builds ONE executor over ONE SQLite -handle (`apps/local/src/executor.ts:212-233`, `createExecutorHandle`), so the -refresh gate does hold there: **R1 is cloud/multi-process only.** R3 and R8 -reproduce in a single-process local app, which matches the reported symptom -(Linear flapping disconnected→connected; local MCPs like Chrome reading -disconnected). R2 needs only one instance and a transient 4xx, so it applies -everywhere. +### Which host shows which cause + +`apps/local` builds one executor over one SQLite handle +(`apps/local/src/executor.ts:212-233`, `createExecutorHandle`). The in-process +refresh gate therefore works in the local app. **R1 occurs in the cloud app and +in multi-process self-hosting only.** R3 and R8 reproduce in a single-process +local app. These two causes match the reported symptoms: an OAuth integration +that changes between disconnected and connected, and local MCP servers that +read as disconnected. R2 needs one instance and one temporary 4xx response, so +it applies to all hosts. --- -## 3. Plan - -Phases are ordered so each lands independently green -(`format:check`, `lint`, `typecheck`, `test`) and the bleeding stops first. - -### Phase 0 — Reproduce and measure (DONE for the repros) - -1. Landed as `packages/core/sdk/src/oauth-expired-status-repro.test.ts` and - `packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts` (see §2b). - Each "documents current behavior" test is the replication; each REPRO test - is the acceptance anchor for its phase and stays red until that phase lands. - The REPRO tests ship skipped; each fix PR un-skips its own. - Note the existing two-instance test in `oauth-flow.test.ts` ("a refresher - paused after reading the stored token never writes it back over a peer's - rotated one") already builds this shape and asserts the _store_ survives — - it never looks at the row, which is why R1 went unnoticed. -2. Add span attributes now so production can size the problem before we change - it: `executor.oauth.refresh.race_suspected` (invalid_grant while the stored - token differs from the one sent — read-only observation), - `executor.oauth.dead_grant.status` (the HTTP status behind the rejection), - `executor.health.source=credential_only` share. Query dead-grant counts per - tenant/integration/reason from existing `executor.oauth.refresh.*` attrs. -3. Record the diagnosis in `MISTAKES.md` (AGENTS.md names it; the file does not - exist yet — create it with this entry). - -### Phase 1 — Stop bricking connections (R1 detection + R2 classification) - -Small, reviewable, and it removes the permanent-damage path even before real -coordination exists. - -1. **Rotation-aware `invalid_grant`** in `performTokenRefresh`: on rejection, - re-read the row and the stored refresh item. If the stored value differs - from the one we sent, a peer rotated it — do **not** mark dead; adopt the - peer's access token (read the primary item) and return it. Span: +## 4. Plan + +The phases are in this order for two reasons. Each phase lands independently +with `format:check`, `lint`, `typecheck`, and `test` green. The phases that +stop permanent damage come first. + +### Phase 0 — Reproduce and measure + +The tests are complete. Two tasks remain. + +1. Add span attributes so production data shows the size of the problem before + the fix. Add `executor.oauth.refresh.race_suspected` for an `invalid_grant` + where the stored token differs from the token that the instance sent. This + attribute is an observation only. Add + `executor.oauth.dead_grant.status` for the HTTP status behind a rejection. + Record the share of `executor.health.source=credential_only`. Then query the + number of permanent rejection records per tenant, integration, and reason + from the existing `executor.oauth.refresh.*` attributes. +2. Record this diagnosis in `MISTAKES.md`. `AGENTS.md` names that file, and the + file does not exist yet. Create it with this entry. + +Note one fact about the existing coverage. The two-instance test in +`oauth-flow.test.ts` ("a refresher paused after reading the stored token never +writes it back over a peer's rotated one") already builds this deployment +shape. It examines the credential store. It does not examine the connection +row. That is the reason nobody found R1. + +### Phase 1 — Stop the permanent damage (R1 detection and R2 classification) + +This phase is small and easy to review. It removes the permanent damage before +the coordination of Phase 2 exists. + +1. **Detect the rotation before the system writes the record.** In + `performTokenRefresh`, read the row and the stored refresh item again after + a rejection. Compare the stored value with the value that the instance sent. + A difference means that another instance rotated the token. Do not write the + permanent rejection record in that case. Read the primary item and return + the access token of the other instance. Add the span attribute `executor.oauth.refresh.outcome=adopted_peer_rotation`. -2. **Fingerprint + CAS on the dead-grant write.** Add - `connection.refresh_token_fp` (SHA-256 prefix of the refresh token, never - the token) written wherever the refresh item is written (the mint paths at - `executor.ts:4509`, `:4565`, `:4729`, fed by - `oauth-service.ts:2344-2430`; and `persistRefreshedToken`). `markRefreshGrantDead` - becomes CAS-guarded on the observed `refresh_token_fp` + `updated_at` - (same idiom as `persistHealthResult`; `updateMany` returns void, so - write-then-re-read decides, and a lost CAS is a silent no-op). A peer's - successful rotation now always beats a stale death certificate. -3. **Narrow `isPermanentTokenRejection`.** Definitive = §5.2 `invalid_grant`, - or an unusable **JSON** 2xx token body carrying an error code. Retryable = - 408, 425, 429, 5xx, transport, non-JSON 2xx (challenge/portal pages). - Other 4xx without a §5.2 code becomes a **strike**: record - `oauthRefreshRejectCount`/`oauthRefreshRejectAt` in `provider_state` and - mark dead on the second strike within a cooldown (e.g. 10 min). This keeps - the Datadog fix (a truly dead grant stops hammering the AS after two - attempts, not 100) without letting one WAF hiccup end a connection. -4. Tests: 429 / 5xx / transport / HTML-200 ⇒ no dead grant; two spaced 400s ⇒ - dead grant; single `invalid_grant` ⇒ dead grant immediately (existing - `oauth-refresh-rejected*.test.ts` must stay green); loser-adopts-rotation - from Phase 0's harness now asserts recovery. - -### Phase 2 — Coordinated refresh across instances (R1 root fix) - -Implement the coordination the `refreshGateFor` comment already prescribes, in -core so selfhost multi-process and cloud both get it. - -1. **DB lease on the connection row**: `refresh_lease_owner`, - `refresh_lease_expires_at` (short, e.g. 30s). Claim with a conditional - `updateMany` (`lease_expires_at IS NULL OR < now`), then re-read to learn - who won — `updateMany` gives no rowcount, so the re-read is the CAS. -2. Winner grants and persists; **losers wait bounded** (poll ~150 ms up to - ~10 s for `expires_at`/`refresh_token_fp` to change) then adopt the stored - access token. A lease that expires mid-grant degrades to today's behavior, - and Phase 1's adoption path catches it. -3. Keep the in-process `WeakMap` gate as the fast path so one executor never - pays a DB round trip for its own concurrency; the lease only arbitrates - _between_ handles. -4. Same treatment for `healthProbeGateFor` (R7's probe-stampede half) — one - lease, N readers adopt the persisted verdict. -5. Tests: two handles ⇒ exactly one grant at the AS (extend Phase 0 harness); - lease expiry ⇒ no deadlock, bounded wait; a crashed winner ⇒ the loser - proceeds after the lease lapses. e2e: `oauth-refresh-cross-instance.test.ts` - (cloud + selfhost) modeled on `oauth-refresh-cross-session.test.ts` but - driving two planes (an HTTP health probe racing an MCP tool call). - -### Phase 3 — Make the probe tell the truth (R3, R6, R8) - -1. **Reactive refresh in `connectionCheckHealth`**: when the probe answers 401 - (or plugin-equivalent auth wall), the connection is OAuth with a refresh - token and no recorded dead grant ⇒ force one refresh and re-probe **once**; - persist the second verdict. Span `executor.health.refresh_retried`. This is - the single change that makes the badge agree with what the next tool call - will do, and it is safe under Phase 2's lease. -2. **Scope-aware 403**: run `detectInsufficientScope` in the probe - classification and emit a distinct outcome (`degraded` + - `reason: insufficient_scope`, feeding the existing `missingOAuthScopes` / - "Reconnect to grant access" UX) instead of red **Expired**. -3. **Narrow GraphQL's `isAuthMessage`**: require an auth signal _and_ a - non-network reason; free-text "permission" alone stops meaning `expired`. -4. **MCP liveness must not dial a second connection (R8).** Take the pooled - connection when one exists for that identity (`connection-pool.ts`) instead - of `discoverToolsFromInput`'s fresh connector, so a probe of a stdio server - does not spawn a second child of a single-instance process. Where a fresh - dial is unavoidable, classify "another instance is already running" / - spawn-because-locked as non-alarm (`unknown`, never `degraded`/`expired`): - the server is up, the credential was never exercised. Add a floor to - non-healthy revalidation in `use-connection-health.ts` (today it sends no - `ifStaleMs` at all, so every mount of every surface re-probes — and for - stdio, re-spawns). -5. Tests: probe-401-then-refresh-then-healthy persists `healthy`; - null-expiry connection heals from a page load alone (today it needs a tool - call); insufficient*scope renders the reconsent affordance, not Expired; - the MCP liveness probe of a live single-instance stdio server answers - healthy and spawns no second child (flip - `mcp-liveness-second-spawn.test.ts`'s REPRO). - e2e: `health-probe-refresh-recovery.test.ts`; keep - `connection-health-verdict.test.ts` green (a \_refused* refresh still ends at - `expired`, persisted, with the freshness window intact). - -### Phase 4 — Honest verdicts and durable expiry (R4, R5) - -1. **Preserve the advertised lifetime**: store the lifetime seen at mint (or - any refresh) in `provider_state.oauthTokenLifetimeMs`; when a refresh - response omits `expires_in`, derive `expires_at` from it instead of writing - null. Null stays only for grants that were never advertised a lifetime. -2. **Evidence-tagged `healthy`**: the credential-only path keeps `healthy` when - it actually refreshed (real evidence) and otherwise answers `unknown` with - detail "Credential present; not verified against the upstream." Also let - plugins that need no spec probe without one (MCP tool discovery), so fewer - connections sit unverified. This changes - `google-health-checks.test.ts:381` deliberately — call it out in the PR. -3. Decide the UX for `unknown`: grey dot, no alarm copy, and a "Check now" - that probes for real (`health-display.ts` already keeps `unknown` neutral). - -### Phase 5 — Recovery affordance and prevention (R2 aftermath, R7) - -1. **"Retry refresh" next to Reconnect** on a dead grant: one re-armed attempt - under the Phase 1 CAS (clears the marker only if the grant succeeds), so a - spuriously bricked connection recovers without re-consent. Keep Reconnect as - the primary action; keep the gate's "no probing while dead" rule for - automatic surfaces — this is an explicit human action. -2. **Copy**: split "Token refresh was rejected — reconnect" from "Upstream - rejected the credential" (`accounts-section.tsx:196`). Show the recorded - reason and when. -3. **Skew**: `max(60s, 10% of the advertised lifetime)`, host-overridable. -4. **Optional, separate decision — background refresh cron** in cloud - (`wrangler.jsonc` already runs a `* * * * *` cron): proactively refresh - tokens for connections used in the last N days. It removes idle-lapse and - makes one coordinated refresher the common path instead of N racing - surfaces. Needs its own design note (cost, org scoping, WorkOS Vault QPS) - — do not fold it into Phases 1-4. -5. **Alert** on dead-grant rate per tenant/integration and on - `race_suspected`, so the next incident is a page rather than a support - thread. +2. **Add a fingerprint and a compare-and-set to the record write.** Add the + column `connection.refresh_token_fp`. Store a SHA-256 prefix of the refresh + token. Never store the token. Write the fingerprint everywhere the system + writes the refresh item: the mint paths at `executor.ts:4509`, `:4565`, and + `:4729`, which `oauth-service.ts:2344-2430` feeds, and + `persistRefreshedToken`. Then guard `markRefreshGrantDead` with a + compare-and-set on the observed `refresh_token_fp` and `updated_at`. Use the + same idiom as `persistHealthResult`. `updateMany` gives no row count, so + write first and read again to decide. A lost compare-and-set does nothing. + A successful rotation by another instance then always wins against an old + rejection. +3. **Narrow `isPermanentTokenRejection`.** Treat these cases as definitive: a + §5.2 `invalid_grant`, and an unusable 2xx response with a JSON token body + that carries an error code. Treat these cases as retryable: 408, 425, 429, + any 5xx, a transport failure, and a non-JSON 2xx response such as a + challenge or portal page. Treat every other 4xx without a §5.2 code as one + strike. Record `oauthRefreshRejectCount` and `oauthRefreshRejectAt` in + `provider_state`. Write the permanent rejection record on the second strike + inside a cooldown period, for example ten minutes. This keeps the benefit of + the existing gate: a truly rejected grant stops sending requests after two + attempts and not after 100. It removes the risk that one wrong answer from a + proxy ends a connection. +4. Add these tests: a 429, a 5xx, a transport failure, and an HTML 200 give no + record; two 400 responses with a gap give the record; one `invalid_grant` + gives the record immediately; the existing `oauth-refresh-rejected*.test.ts` + files stay green; the losing instance in the Phase 0 harness recovers. + +### Phase 2 — Coordinate the refresh between instances (the R1 fix) + +Implement the coordination that the `refreshGateFor` documentation recommends. +Put it in core so that multi-process self-hosting and the cloud app both get +it. + +1. **Add a lease to the connection row.** Add `refresh_lease_owner` and + `refresh_lease_expires_at`. Use a short lease, for example 30 seconds. + Claim the lease with a conditional `updateMany` where the condition is + `lease_expires_at IS NULL OR lease_expires_at < now`. Then read the row + again to learn which instance won. `updateMany` gives no row count, so the + second read is the compare-and-set. +2. **The winner sends the grant and persists the result.** The losers wait for + a bounded time. Poll approximately every 150 ms for a maximum of + approximately ten seconds, and examine `expires_at` and `refresh_token_fp` + for a change. Then read the stored access token and use it. A lease that + expires during a grant gives the behavior of today, and the adoption path of + Phase 1 handles that case. +3. **Keep the in-process `WeakMap` gate as the fast path.** One executor then + never pays a database round trip for its own concurrency. The lease + arbitrates between handles only. +4. **Apply the same design to `healthProbeGateFor`.** This fixes the probe half + of R7. One probe runs, and all readers use the persisted status. +5. Add these tests: two handles give exactly one grant at the AS, with the + Phase 0 harness extended; an expired lease gives no deadlock and a bounded + wait; a winner that crashes lets the loser continue after the lease ends. + Add the e2e scenario `oauth-refresh-cross-instance.test.ts` for the cloud + and self-hosting targets. Model it on `oauth-refresh-cross-session.test.ts` + but drive two planes: one HTTP health probe and one MCP tool call at the + same time. + +### Phase 3 — Make the probe report the truth (R3, R6, R8) + +1. **Add a reactive refresh to `connectionCheckHealth`.** Act when all these + conditions are true: the probe answers 401 or the plugin equivalent; the + connection is OAuth; the connection has a refresh token; no permanent + rejection record exists. Then force one refresh and probe one more time. + Persist the second status. Add the span attribute + `executor.health.refresh_retried`. This change makes the indicator agree + with the next tool call. The lease of Phase 2 makes it safe. +2. **Detect a scope shortfall in a 403.** Run `detectInsufficientScope` in the + probe classification. Report a distinct result: `degraded` with + `reason: insufficient_scope`. Feed the existing `missingOAuthScopes` + mechanism and the "Reconnect to grant access" interface. Do not report red + **Expired**. +3. **Narrow the GraphQL `isAuthMessage` match.** Require an authentication + signal and a reason that is not a network reason. The single word + "permission" in free text must not give `expired`. +4. **Stop the second MCP connection (R8).** Use the pooled connection when one + exists for that identity (`connection-pool.ts`) instead of the new connector + in `discoverToolsFromInput`. A probe of a stdio server then does not start a + second child of a single-instance process. When a new connection is + unavoidable, classify "another instance already runs" as a neutral result. + Report `unknown`. Never report `degraded` or `expired`, because the server + runs and the probe never exercised the credential. Add a minimum interval to + the non-healthy revalidation in `use-connection-health.ts`. Today that code + sends no `ifStaleMs`, so every mount of every surface probes again, and for + stdio it starts another child process. +5. Add these tests: a probe that receives a 401, then refreshes, then receives + a healthy answer persists `healthy`; a connection with a null expiry + recovers from a page load alone, which needs a tool call today; an + insufficient scope shows the new-consent interface and not Expired; the MCP + probe of a live single-instance stdio server answers healthy and starts no + second child, which removes the skip from + `mcp-liveness-second-spawn.test.ts`. Add the e2e scenario + `health-probe-refresh-recovery.test.ts`. Keep + `connection-health-verdict.test.ts` green: a refused refresh still ends at + `expired`, persisted, with the freshness window intact. + +### Phase 4 — Honest status values and a durable expiry (R4, R5) + +1. **Keep the advertised lifetime.** Store the lifetime that the mint or any + refresh reported in `provider_state.oauthTokenLifetimeMs`. When a refresh + response omits `expires_in`, derive `expires_at` from that stored lifetime + instead of writing null. Write null only for a grant that never advertised a + lifetime. +2. **Require evidence for `healthy`.** The credential-only path keeps `healthy` + when it performed a refresh, because that is evidence. Otherwise it answers + `unknown` with the detail "Credential present; not verified against the + upstream." Also let plugins that need no spec probe without one, for example + MCP tool discovery. Fewer connections then stay unverified. This changes + `google-health-checks.test.ts:381` on purpose. State that in the pull + request. +3. Decide the interface for `unknown`. Use a grey indicator, no alarm text, and + a "Check now" action that performs a real probe. `health-display.ts` already + treats `unknown` as neutral. + +### Phase 5 — Recovery for the user and prevention for the system (R2 result, R7) + +1. **Add a "Retry refresh" action next to Reconnect** for a connection with a + permanent rejection record. The action performs one new attempt under the + compare-and-set of Phase 1. It removes the record only when the grant + succeeds. A connection that received a wrong record then recovers without a + new consent. Keep Reconnect as the primary action. Keep the rule "no probe + while the record exists" for automatic surfaces, because this action is an + explicit human action. +2. **Separate the messages.** Distinguish "Token refresh was rejected — + reconnect" from "Upstream rejected the credential" + (`accounts-section.tsx:196`). Show the recorded reason and its time. +3. **Increase the skew.** Use `max(60s, 10% of the advertised lifetime)`. Let + the host override it. +4. **Consider a background refresh cron in the cloud app.** This is a separate + decision and needs its own design note. `wrangler.jsonc` already runs a + `* * * * *` cron. The cron would refresh the tokens of connections that were + used in the last N days. It would remove idle lapse. It would also make one + coordinated refresher the normal path instead of many racing surfaces. The + design note must give the cost, the organization scope, and the WorkOS Vault + request rate. Do not add this work to Phases 1 to 4. +5. **Add alerts.** Alert on the rate of permanent rejection records per tenant + and integration, and on `race_suspected`. The next incident should start with + an alert and not with a support message. --- -## 4. What must not regress - -- The known-dead gate itself: a genuinely dead grant must stop generating - refresh traffic after a bounded number of attempts and must present - `expired` on every read (`connections.test.ts:2810`, `:2985`, `:3084`, - `:3119`). -- Verdict writes stay best-effort and CAS-guarded; a dead grant recorded - mid-probe still survives the probe's write. -- Reactive tool-call retry stays exactly one retry, 401-only, refresh-token - holders only (`oauth-refresh-on-401.test.ts`). -- Single-flight refresh within one process (`oauth-refresh-cross-session.test.ts`). -- Interrupting a dial must still tear down the stdio child (`#1631`, - `stdio-interrupt-cleanup.test.ts`): routing the liveness probe through the - pool changes WHO owns the child, and the pooled child's lifetime is the - pool's — a probe must not close a connection invocations still need, and an - interrupted probe must not strand one. -- The store-writability probe before spending a single-use refresh token - (`#1377`) — and note it writes an item per refresh that is never deleted; - worth a cleanup task, not a blocker. -- Nothing secret-bearing in spans, health `detail`, or the new fingerprint - column (hash only; `redactTokenEndpointBody`'s allowlist governs rendering). - -## 5. Suggested PR boundaries - -1. Phase 0 (tests + telemetry + MISTAKES entry) — no behavior change. -2. Phase 1.1-1.2 (rotation adoption + fingerprint CAS). -3. Phase 1.3 (classification narrowing + strikes). -4. Phase 2 (lease) — the largest; ship behind a config flag defaulting on, with - the flag removed in a follow-up. -5. Phase 3 (probe refresh + scope-aware 403 + GraphQL narrowing + MCP liveness - reusing the pool). R8 is independently shippable and is the one fix that - addresses the reported local symptom on its own — it can lead Phase 3 or - ship before it. +## 5. Invariants to preserve + +- The gate itself. A truly rejected grant must stop refresh traffic after a + bounded number of attempts and must show `expired` on every read + (`connections.test.ts:2810`, `:2985`, `:3084`, `:3119`). +- Status writes stay best effort and keep their compare-and-set. A permanent + rejection record that lands during a probe still survives the write of that + probe. +- The reactive tool call retry stays at one retry, for 401 responses only, for + connections with a refresh token only (`oauth-refresh-on-401.test.ts`). +- One refresh at a time inside one process + (`oauth-refresh-cross-session.test.ts`). +- An interrupted connection attempt must still stop the stdio child process + (`#1631`, `stdio-interrupt-cleanup.test.ts`). Routing the probe through the + pool changes which component owns the child. The pool owns the lifetime of a + pooled child. A probe must not close a connection that tool calls still need. + An interrupted probe must not leave a child process running. +- The store-writability probe before the system spends a single-use refresh + token (`#1377`). Note one defect: that probe writes one item per refresh and + never deletes it. Track it as a cleanup task. It does not block this plan. +- No secret material in spans, in the health `detail`, or in the new + fingerprint column. Store a hash only. The allowlist in + `redactTokenEndpointBody` governs what the system renders. + +## 6. Pull request boundaries + +1. Phase 0: the tests, the telemetry attributes, and the `MISTAKES.md` entry. + No behavior change. +2. Phase 1 items 1 and 2: rotation detection, and the fingerprint with its + compare-and-set. +3. Phase 1 item 3: the narrow classification and the strikes. +4. Phase 2: the lease. This is the largest change. Put it behind a + configuration flag that is on by default, and remove the flag in a later + pull request. +5. Phase 3: the probe refresh, the scope-aware 403, the narrow GraphQL match, + and the pooled MCP probe. R8 can ship on its own. It is the only fix that + addresses the reported local symptom without other changes, and it does not + change OAuth code. It can lead Phase 3 or ship before it. 6. Phase 4, then Phase 5. -Each PR: narrowest meaningful vitest while iterating, one named e2e scenario -when the change is user-visible, `bun run format` before opening. +For each pull request: run the narrowest meaningful vitest selection while you +iterate; add one named e2e scenario when the change is user-visible; run +`bun run format` before you open it.