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/6] 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/6] 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. From 47be1c1b2baa123febb192bb12056d78af5cb01f Mon Sep 17 00:00:00 2001 From: spa5k <79936503+spa5k@users.noreply.github.com> Date: Thu, 17 Sep 2026 19:56:50 +0530 Subject: [PATCH 3/6] Require evidence before a connection is marked permanently expired Three causes of a wrong Expired status, fixed in the SDK. The analysis and the reproductions are in the parent branch; this branch lands Phase 1 items 1 to 3 and Phase 3 item 1 of plans/oauth-refresh-and-expired-status.md. 1. A refresher that loses a rotation race no longer ends the connection. The in-flight gate is keyed per database handle, so two instances can redeem one single-use refresh token. The loser received invalid_grant and wrote the permanent rejection record, while the winner's valid rotated token sat in the store unused. The refusal now re-reads the refresh item first. A value that changed during the request is a peer's rotation, so the call adopts the access token that peer persisted and records nothing. Adoption runs before the record write, and an adopted token is never re-persisted, because that would erase the expiry the peer wrote. 2. The record write also re-reads the row. An expires_at that moved forward during the grant is the same peer success seen from the row, so the write is skipped. Only a mint or a refresh writes that column, so an unrelated write such as a tool sync does not suppress a legitimate record. 3. isPermanentTokenRejection no longer reads 408, 425, or 429 as definitive. One rate-limited minute at a token endpoint ended a grant permanently. Those statuses now behave like a 5xx response and the next call retries. 4. connections.checkHealth re-mints once and probes again before it answers expired for an OAuth connection. The probe previously answered from the credential it was handed, so a revoked token, an idle timeout shorter than the advertised lifetime, or a null expires_at showed a working connection as dead until a tool call healed it. The three skipped acceptance tests from the parent branch are un-skipped and pass unchanged in intent; each cause now has one regression test instead of a pair. oauth-helpers.test.ts pins the new transient-status classification. --- .changeset/oauth-refresh-evidence.md | 11 + packages/core/sdk/src/executor.ts | 206 ++++++++++++++---- .../src/oauth-expired-status-repro.test.ts | 206 +++++++----------- packages/core/sdk/src/oauth-helpers.test.ts | 42 ++++ packages/core/sdk/src/oauth-helpers.ts | 28 ++- plans/oauth-refresh-and-expired-status.md | 87 +++++--- 6 files changed, 381 insertions(+), 199 deletions(-) create mode 100644 .changeset/oauth-refresh-evidence.md diff --git a/.changeset/oauth-refresh-evidence.md b/.changeset/oauth-refresh-evidence.md new file mode 100644 index 0000000000..9e84592a30 --- /dev/null +++ b/.changeset/oauth-refresh-evidence.md @@ -0,0 +1,11 @@ +--- +"@executor-js/sdk": patch +--- + +Require evidence before a connection is marked permanently expired, and let the health probe refresh before it answers `expired`. + +A refresher whose grant is refused now reads the stored refresh token again. When a peer instance rotated that token while the request was in flight, the call adopts the access token the peer persisted and records no rejection. Previously the loser of a concurrent refresh wrote `oauthReauthRequiredAt` onto a connection that still held a valid rotated refresh token. Every surface then answered `expired` without probing, and no tool call could refresh it again: only a re-authorization recovered it. The record is also skipped when `expires_at` moved forward during the grant, which is the same peer success read from the row. + +`isPermanentTokenRejection` no longer reads 408, 425, or 429 as a definitive refusal. One rate-limited minute at a token endpoint therefore no longer ends a grant. Those statuses now behave like a 5xx response, and the next call retries. + +`connections.checkHealth` re-mints the token once and probes again before it answers `expired` for an OAuth connection. A revoked token, an idle timeout shorter than the advertised lifetime, or a null `expires_at` therefore no longer shows a working connection as dead. diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index cd89ab9075..5e1abb6858 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -2284,6 +2284,14 @@ export const createExecutor = => { - const existingState = decodeJsonColumn(row.provider_state); - const mergedState = - existingState != null && typeof existingState === "object" && !Array.isArray(existingState) - ? (existingState as Record) - : {}; - const health: HealthCheckResult = { - status: "expired", - checkedAt: Date.now(), - detail, - reason, + const ref: ConnectionRef = { + owner: row.owner as Owner, + integration: IntegrationSlug.make(row.integration), + name: ConnectionName.make(row.name), }; - return core - .updateMany("connection", { + const observedExpiry = row.expires_at == null ? null : Number(row.expires_at); + const record = (target: ConnectionRow): Effect.Effect => { + const existingState = decodeJsonColumn(target.provider_state); + const mergedState = + existingState != null && + typeof existingState === "object" && + !Array.isArray(existingState) + ? (existingState as Record) + : {}; + const health: HealthCheckResult = { + status: "expired", + checkedAt: Date.now(), + detail, + reason, + }; + return core.updateMany("connection", { where: (b: AnyCb) => b.and( - byOwner(row.owner as Owner)(b), - b("integration", "=", String(row.integration)), - b("name", "=", String(row.name)), + byOwner(target.owner as Owner)(b), + b("integration", "=", String(target.integration)), + b("name", "=", String(target.name)), ), set: { provider_state: { @@ -2333,8 +2349,36 @@ export const createExecutor = { + if (fresh === null) return record(row); + const freshExpiry = fresh.expires_at == null ? null : Number(fresh.expires_at); + const peerRefreshed = + freshExpiry !== null && (observedExpiry === null || freshExpiry > observedExpiry); + return peerRefreshed + ? Effect.annotateCurrentSpan({ + "executor.oauth.dead_grant.skipped_peer_refresh": true, + }) + : record(fresh); + }), + Effect.ignore, + ); }; /** Write a re-minted token back: a ROTATED refresh token into the refresh @@ -2739,7 +2783,45 @@ export const createExecutor = => + Effect.gen(function* () { + if (error.reauthRequired !== true) return yield* Effect.fail(error); + const sent = storedRefreshToken; + if (sent === undefined || row.refresh_item_id == null) { + return yield* Effect.fail(error); + } + const current = yield* provider.get(ProviderItemId.make(row.refresh_item_id)); + if (current === null || current === sent) return yield* Effect.fail(error); + const tokenItemId = + connectionItemIds(row)[PRIMARY_INPUT_VARIABLE] ?? + `connection:${row.owner}:${row.integration}:${row.name}:${PRIMARY_INPUT_VARIABLE}`; + const access = yield* provider.get(ProviderItemId.make(tokenItemId)); + if (access === null) return yield* Effect.fail(error); + return access; + }); + const token: OAuth2TokenResponse | AdoptedAccessToken = clientRow.grant === "client_credentials" ? yield* exchangeClientCredentials({ tokenUrl, @@ -2874,6 +2956,13 @@ export const createExecutor = + adoptPeerRotatedToken(error), + ), // Persist the definitive verdict so the NEXT refresh skips // the doomed grant (see the known-dead gate above) and the // connection shows `expired` without waiting for a probe. @@ -2887,6 +2976,14 @@ export const createExecutor = , + ): Effect.Effect => { + const credential: ToolInvocationCredential = { + owner: connectionRow.owner as Owner, + integration: ref.integration, + connection: ConnectionName.make(connectionRow.name), + template: AuthTemplateSlug.make(connectionRow.template), + value: values[PRIMARY_INPUT_VARIABLE] ?? null, + values, + config: record.config, + ...(grantedScopes ? { grantedScopes } : {}), + }; + // Core resolves the declared spec (its own column) and + // hands it to the plugin; plugins no longer read it out of + // their config. + return foldPluginFailure( + check({ + ctx: runtime.ctx, + integration: record, + credential, + spec, + }), + `Health check for connection "${ref.name}" failed.`, + ); }; - // Core resolves the declared spec (its own column) and - // hands it to the plugin; plugins no longer read it out of - // their config. - return yield* foldPluginFailure( - check({ - ctx: runtime.ctx, - integration: record, - credential, - spec, - }), - `Health check for connection "${ref.name}" failed.`, + const values = yield* resolveConnectionValues(connectionRow); + const first = yield* probe(values); + // A probe answers from the credential it was handed, so its + // `expired` is only as good as that credential. The invoke + // path knows this and re-mints once on a 401 + // (`forceRefreshConnectionValues`); the probe did not, which + // persisted `expired` for exactly the connections the + // reactive refresh exists for — a server-side revocation, an + // idle timeout shorter than the advertised lifetime, a null + // `expires_at` the proactive check can never fire on. The + // badge then said "reconnect" for a connection that worked + // on its next call, and only heal-on-use corrected it. + // + // One forced refresh and one re-probe, for an OAuth + // connection only. A refusal keeps the probe's own verdict: + // it is the more informative of the two, and the refresh + // path has already recorded a dead grant if there is one. + if (first.status !== "expired" || connectionRow.oauth_client == null) { + return first; + } + const refreshed = yield* forceRefreshConnectionValues(connectionRow).pipe( + Effect.catchTag("CredentialResolutionError", () => Effect.succeed(null)), ); + if (refreshed === null) return first; + yield* Effect.annotateCurrentSpan({ + "executor.health.refresh_retried": true, + }); + return yield* probe(refreshed); }), ).pipe( // Persist the verdict on the connection row so the accounts diff --git a/packages/core/sdk/src/oauth-expired-status-repro.test.ts b/packages/core/sdk/src/oauth-expired-status-repro.test.ts index 6e44ce3e6d..0cd834b8e8 100644 --- a/packages/core/sdk/src/oauth-expired-status-repro.test.ts +++ b/packages/core/sdk/src/oauth-expired-status-repro.test.ts @@ -1,22 +1,25 @@ -// Reproduction harness for the "Expired" status + refresh defects analysed in -// plans/oauth-refresh-and-expired-status.md. +// Regression coverage for the three causes of a wrong **Expired** status that +// plans/oauth-refresh-and-expired-status.md ranks R1, R2, and R3. This file +// started as the reproduction harness for that analysis: each cause had a test +// that pinned the behavior on `main` and a skipped test that gave the required +// behavior. The fix landed, so each cause now has one test, and it asserts the +// required behavior. // -// 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. +// R1: a refresher that loses a rotation race adopts the peer's token. It does +// not write the permanent rejection record. +// R2: one temporary 4xx response (a 429) does not end the grant. +// R3: the health probe refreshes before it answers `expired`. // // 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. +// instances, and one root database handle for each instance. That is the cloud +// app (a per-request `DbService` rebuild plus per-session Durable Objects) and +// any multi-process self-hosting. It is the shape that the `refreshGateFor` +// documentation declares out of scope for the in-process gate. +// +// `oauth-flow.test.ts` already builds this shape in "a refresher paused after +// reading the stored token never writes it back over a peer's rotated one". +// That test examines the credential store. These tests examine the connection +// row, which is where the wrong status was written. import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; @@ -288,12 +291,13 @@ const deadGrantStamp = (row: unknown): number | undefined => { }; // --------------------------------------------------------------------------- -// R1 — the loser of a rotation race permanently bricks a healthy connection. +// R1 — a refresher that loses the rotation race adopts the peer's token. // --------------------------------------------------------------------------- /** 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. */ + * rotates it, A resumes and redeems the consumed token. Returns the rotated + * token and A's own outcome, so a test can assert what the loser did with the + * refusal. */ const runRotationRace = (race: Race) => Effect.gen(function* () { const refreshItemId = race.refreshItemId(); @@ -314,69 +318,43 @@ const runRotationRace = (race: Race) => // A resumes and redeems a token the authorization server already consumed. yield* Deferred.succeed(race.resumeFromRead, undefined); - yield* Fiber.join(loser); + const loserExit = 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! }; + return { + refreshItemId: refreshItemId!, + rotatedRefreshToken: rotatedRefreshToken!, + loserExit, + }; }); -describe("R1 — refresh race across two instances", () => { - it.effect("documents current behavior: the loser bricks a connection holding a valid token", () => +describe("R1 — a lost rotation race is not a dead grant", () => { + it.effect("the loser adopts the peer's token and the connection keeps refreshing", () => 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); - }), - ), - ); + const { loserExit } = yield* runRotationRace(race); - // 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); + // The loser's own call recovered: it read the refresh item back, saw + // that a peer had replaced the value it sent, and used the access token + // that peer persisted. + expect(Exit.isSuccess(loserExit), "the losing refresher still served its call").toBe(true); - // 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(); + // No permanent rejection record, because the grant is alive. + expect(deadGrantStamp(yield* race.rawRow()), "no dead grant is recorded").toBeUndefined(); const health = yield* race.b.connections.checkHealth(REF); expect(health.status, "and no surface answers expired").not.toBe("expired"); + // The winner still refreshes with its own rotated token when the access + // token next expires. 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(Exit.isSuccess(next), "the winner refreshes again on the next expiry").toBe(true); expect( refreshGrants(yield* race.server.requests).length, - "executor asked the authorization server again", + "the authorization server received that grant", ).toBeGreaterThan(0); }), ), @@ -384,7 +362,7 @@ describe("R1 — refresh race across two instances", () => { }); // --------------------------------------------------------------------------- -// R2 — one transient 4xx (a 429) permanently kills the grant. +// R2 — one temporary 4xx does not end a grant. // --------------------------------------------------------------------------- interface FlakyEndpoint { @@ -457,7 +435,7 @@ const serveFlakyTokenEndpoint = (upstream: string) => ); /** Connect, point the backing app at a token endpoint that rate-limits once, - * and take that first (failing) refresh. Shared by both R2 tests. */ + * and take that first failing refresh. */ const withRateLimitedRefresh = ( use: (input: { readonly race: Race; @@ -483,44 +461,30 @@ const withRateLimitedRefresh = ( }), ); -describe("R2 — transient 4xx classification", () => { - it.effect("documents current behavior: one 429 permanently disables a working grant", () => +describe("R2 — a rate-limited refresh stays retryable", () => { + it.effect("a 429 from the token endpoint does not end the 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(); + // The endpoint forwards every grant after the first to the real + // authorization server, so it is healthy from here on. The next call + // must reach it. 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); - }), - ), - ); + expect(Exit.isSuccess(second), "the retry refreshed and the call succeeded").toBe(true); + expect(flaky.attempts(), "executor asked the token endpoint again").toBeGreaterThan(1); - // 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); + expect( + deadGrantStamp(yield* race.rawRow()), + "a rate limit is not a permanent rejection", + ).toBeUndefined(); + const health = yield* race.a.connections.checkHealth(REF); + expect(health.status, "and the connection does not read as expired").not.toBe("expired"); }), ), ); }); // --------------------------------------------------------------------------- -// 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. +// R3 — the probe refreshes before it answers expired. // --------------------------------------------------------------------------- /** Connect with a declared health check, then revoke the live access token @@ -534,51 +498,39 @@ const withRevokedToken = (use: (race: Race) => Effect.Effect) => }), ); -describe("R3 — probe verdict vs reactive refresh", () => { - it.effect("documents current behavior: probe says expired, the next tool call says healthy", () => +describe("R3 — the probe refreshes before it answers expired", () => { + it.effect("a revoked token that the refresh can replace probes 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(verdict.status, "the probe re-minted instead of reporting expired").toBe("healthy"); expect( - refreshGrants(yield* race.server.requests), - "the probe sent no refresh grant", - ).toHaveLength(0); + refreshGrants(yield* race.server.requests).length, + "the probe sent a refresh grant", + ).toBeGreaterThan(0); + + // The persisted verdict agrees, so every surface reads healthy without + // waiting for a tool call to heal it. 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"); + expect(persisted?.lastHealth?.status, "the healthy verdict is persisted").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) => + it.effect("a refused refresh still answers expired from the probe", () => + withRace({ healthCheck: true }, (race) => Effect.gen(function* () { - // Phase 3 target: the probe refreshes once before concluding expired. + // No revocation and no expiry: the probe answers from the credential it + // resolved. A refusal is what the persisted-expired contract in + // `connection-health-verdict.test.ts` covers; this asserts the retry + // did not turn the probe into a second grant on a healthy connection. + yield* race.server.clearRequests; const verdict = yield* race.a.connections.checkHealth(REF); - expect(verdict.status, "a refreshable revocation is not an expired connection").toBe( - "healthy", - ); + expect(verdict.status).toBe("healthy"); expect( - refreshGrants(yield* race.server.requests).length, - "the probe re-minted the token", - ).toBeGreaterThan(0); + refreshGrants(yield* race.server.requests), + "a healthy probe sends no refresh grant", + ).toHaveLength(0); }), ), ); diff --git a/packages/core/sdk/src/oauth-helpers.test.ts b/packages/core/sdk/src/oauth-helpers.test.ts index d94e8c34ad..1ecfb6e7b0 100644 --- a/packages/core/sdk/src/oauth-helpers.test.ts +++ b/packages/core/sdk/src/oauth-helpers.test.ts @@ -1781,6 +1781,48 @@ describe("refreshAccessToken", () => { expect(isPermanentTokenRejection(error)).toBe(false); }), ); + + // A 4xx that describes THIS MINUTE rather than this grant must stay + // retryable: the identical request can succeed moments later. 429 is the + // authorization server asking us to come back, 408 is its own request + // timeout, 425 is a transport-level replay refusal. Reading any of them as + // definitive ended connections permanently on one rate-limited minute — and + // rate limiting is likelier the more refreshers race for one grant. + for (const status of [408, 425, 429] as const) { + it.effect(`keeps a ${status} response transient`, () => + withTokenEndpoint( + () => HttpServerResponse.text("slow down", { status }), + ({ tokenUrl }) => + Effect.gen(function* () { + const error = yield* Effect.flip( + refreshAccessToken({ tokenUrl, clientId: "cid", refreshToken: "old" }), + ); + expect(error.status).toBe(status); + expect(error.error).toBeUndefined(); + expect(isPermanentTokenRejection(error)).toBe(false); + }), + ), + ); + } + + // The definitive 4xx stay definitive: §5.2 mandates 400 for a grant the AS + // will not honour, and a token endpoint answering 404 does not start + // existing on the next attempt. + for (const status of [400, 404] as const) { + it.effect(`keeps a text/plain ${status} response definitive`, () => + withTokenEndpoint( + () => HttpServerResponse.text("your session has expired", { status }), + ({ tokenUrl }) => + Effect.gen(function* () { + const error = yield* Effect.flip( + refreshAccessToken({ tokenUrl, clientId: "cid", refreshToken: "old" }), + ); + expect(error.status).toBe(status); + expect(isPermanentTokenRejection(error)).toBe(true); + }), + ), + ); + } }); describe("shouldRefreshToken", () => { diff --git a/packages/core/sdk/src/oauth-helpers.ts b/packages/core/sdk/src/oauth-helpers.ts index c6e3209fe7..a5c1517086 100644 --- a/packages/core/sdk/src/oauth-helpers.ts +++ b/packages/core/sdk/src/oauth-helpers.ts @@ -73,22 +73,34 @@ export class OAuth2Error extends Data.TaggedError("OAuth2Error")<{ export const isUnusableSuccessTokenResponse = (error: OAuth2Error): boolean => error.status !== undefined && error.status < 300; +/** 4xx statuses that describe THIS MINUTE rather than this grant. A 429 is the + * authorization server asking us to come back, a 408 is its own request + * timeout, and a 425 is a transport-level replay refusal — none of them is a + * verdict on the refresh token, and re-sending the identical grant later can + * succeed. Treating them as definitive ended connections permanently on one + * rate-limited minute, which is likelier the more refreshers race. */ +const TRANSIENT_4XX_STATUSES: ReadonlySet = new Set([408, 425, 429]); + /** * Did the token endpoint answer in a way that re-sending the identical grant * cannot change? * - * Yes for a 4xx — §5.2 mandates 400 for a grant the authorization server will - * not honour, 401/403 are refusals, and a token endpoint answering 404 does not - * start existing on the next attempt — and yes for a 2xx that carried no usable - * token, because the server called it a success and still issued nothing. + * Yes for a 4xx that is not one of {@link TRANSIENT_4XX_STATUSES} — §5.2 + * mandates 400 for a grant the authorization server will not honour, 401/403 + * are refusals, and a token endpoint answering 404 does not start existing on + * the next attempt — and yes for a 2xx that carried no usable token, because + * the server called it a success and still issued nothing. * - * No for a 5xx (the AS is having a bad minute) and no when there is no response - * at all (transport). Those are exactly the failures a later attempt survives, - * so they must stay retryable. + * No for a 5xx (the AS is having a bad minute), no for a rate-limited or + * timed-out 4xx, and no when there is no response at all (transport). Those are + * exactly the failures a later attempt survives, so they must stay retryable. */ export const isPermanentTokenRejection = (error: OAuth2Error): boolean => isUnusableSuccessTokenResponse(error) || - (error.status !== undefined && error.status >= 400 && error.status < 500); + (error.status !== undefined && + error.status >= 400 && + error.status < 500 && + !TRANSIENT_4XX_STATUSES.has(error.status)); // --------------------------------------------------------------------------- // Token response shape (RFC 6749 §5.1) diff --git a/plans/oauth-refresh-and-expired-status.md b/plans/oauth-refresh-and-expired-status.md index 1db2761860..e62270d29e 100644 --- a/plans/oauth-refresh-and-expired-status.md +++ b/plans/oauth-refresh-and-expired-status.md @@ -1,7 +1,8 @@ # The wrong Expired status: analysis and plan -Status: the analysis is complete and the plan is proposed. This branch adds -tests and this document. It does not change runtime behavior. +Status: the analysis is complete and the plan is proposed. The diagnosis branch +adds tests and this document only. The fix branch +(`fix/oauth-refresh-evidence`) lands Phase 1 items 1 to 3 and Phase 3 item 1. ## The problem @@ -248,11 +249,15 @@ symptom. ## 3. Replication -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. +Four causes have executable tests. Each cause has two tests on the diagnosis +branch. 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. + +R1, R2, and R3 are fixed on `fix/oauth-refresh-evidence`. That branch replaces +each pair with one test that asserts the required behavior, so the file is now +regression coverage. R8 still ships its skipped test. ### The OAuth and health tests @@ -352,26 +357,41 @@ 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. +the coordination of Phase 2 exists. Items 1 and 2 landed on +`fix/oauth-refresh-evidence`. Item 3 landed in the narrow form below: the +transient statuses are excluded, and the strike counter is deferred. 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. **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. + `performTokenRefresh`, read 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. + + **Landed.** `adoptPeerRotatedToken` runs between the classification and the + record write, so the record is only considered after adoption failed. The + span attribute is `executor.oauth.refresh.peer_rotation_adopted=true`, and + `executor.oauth.refresh.outcome` stays `ok`, because the call did succeed. + The caller skips `persistRefreshedToken` for an adopted token: the peer + persisted it already, and persisting from a token response this instance + never received would erase the expiry the peer wrote. + +2. **Guard the record write against a peer's success.** Read the row again + before `markRefreshGrantDead` writes. When `expires_at` moved forward, or + went from null to a value, a peer refreshed this grant successfully while + our request was in flight. Skip the record then. Only a mint or a refresh + writes `expires_at`, so this signal does not fire for an unrelated write + such as a tool sync. Write against the fresh row, so the merge base is the + current `provider_state`. + + **Landed in this form.** The plan first proposed a new + `connection.refresh_token_fp` column with a compare-and-set on it. That + needs a schema migration in four hosts, and the re-read of the stored + refresh item in item 1 already gives the precise signal. The `expires_at` + guard is the second net for the case where adoption itself fails, for + example when the primary item is unreadable. Revisit the fingerprint column + only if Phase 2's lease needs a stable token identity. + 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, @@ -383,6 +403,15 @@ the coordination of Phase 2 exists. 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. + + **Landed in part.** The transient statuses 408, 425, and 429 are excluded + and behave like a 5xx response. The strike counter is not implemented: it + needs the cooldown semantics decided first, and excluding the transient + statuses removes the case that motivated it. The non-JSON 2xx case is + unchanged, because `oauth-helpers.test.ts` pins a malformed JSON 200 as + definitive and the HTML-200 variant needs a structural "the body was JSON" + flag on `OAuth2Error`. Both remain open. + 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` @@ -421,6 +450,8 @@ it. ### Phase 3 — Make the probe report the truth (R3, R6, R8) +Item 1 landed on `fix/oauth-refresh-evidence`. Items 2 to 5 are open. + 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 @@ -428,6 +459,12 @@ it. 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. + + **Landed.** The probe builds its credential through one local `probe` + function, runs it, and on an `expired` answer for an OAuth connection it + calls `forceRefreshConnectionValues` and runs the probe one more time. A + refused refresh keeps the first verdict. + 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` From e29cd5d9acf062fc47d7ad81531369f7ceeebd47 Mon Sep 17 00:00:00 2001 From: spa5k <79936503+spa5k@users.noreply.github.com> Date: Thu, 17 Sep 2026 22:15:40 +0530 Subject: [PATCH 4/6] Fix the remaining wrong-status causes and make the MCP probe honest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Continues the same branch; the causes are ranked in plans/oauth-refresh-and-expired-status.md. This lands Phase 3 items 2 to 4 and Phase 4 items 1 and 2, and R8 from the reported local symptom. The MCP liveness probe takes the invocation pool's lease instead of dialling a second connection, built from the same identity the invoke path uses, so a probe of a stdio server no longer starts a second child of a single-instance process (Chrome DevTools MCP, Playwright MCP, docker run -i). The whole lease is bounded by the shared discovery deadline — the pool's own dial has none — and an interrupted probe still releases, so #1631 holds. A probe is now asked of the plugin even when the integration declares no health-check operation. A plugin that can answer without one (MCP lists tools) gives a real verdict for its OAuth connections, which the credential-only branch never reached. Only a plugin that answers unknown falls back to the credential-only verdict, computed from the values the probe already resolved so nothing refreshes twice; that fallback now reports expired when a credential value resolved to nothing, matching the plugins and heal-on-use. A 403 scope shortfall on a probe reads degraded instead of expired, from either an RFC 6750 WWW-Authenticate challenge (classifyProbeResponse now takes the response headers) or a body marker. GraphQL no longer reads a transport failure's prose as a dead credential: `connect EACCES: permission denied` on a socket is not an authentication verdict. A refresh response that omits expires_in no longer erases expires_at. The mint records the advertised lifetime in provider_state.oauthTokenLifetimeMs and a refresh derives the expiry from it; RFC 6749 makes the field optional, and writing null disabled proactive refresh for the rest of the connection's life. The test authorization server's /mcp resource endpoint now speaks JSON-RPC honestly: the request's own id, an empty catalog for tools/list, and silence for notifications. The old canned reply used a fixed id, so any client that completed the handshake waited forever for tools/list and every sync or probe against the endpoint timed out at the discovery deadline — invisible while OAuth health checks never dialled, exposed once they do. --- .changeset/graphql-network-prose.md | 5 + .changeset/mcp-liveness-pooled-probe.md | 5 + .changeset/oauth-refresh-evidence.md | 8 + .changeset/probe-scope-shortfall.md | 5 + packages/core/sdk/src/connections.test.ts | 34 ++- packages/core/sdk/src/executor.ts | 281 +++++++++++------- packages/core/sdk/src/health-check.test.ts | 32 ++ packages/core/sdk/src/health-check.ts | 34 ++- .../src/oauth-expired-status-repro.test.ts | 112 +++++++ packages/core/sdk/src/oauth-flow.test.ts | 25 +- .../core/sdk/src/testing/oauth-test-server.ts | 39 ++- .../src/sdk/health-classification.test.ts | 48 +++ packages/plugins/graphql/src/sdk/plugin.ts | 24 +- packages/plugins/mcp/src/sdk/discover.ts | 169 +++++++---- .../src/sdk/mcp-liveness-second-spawn.test.ts | 241 ++++++--------- packages/plugins/mcp/src/sdk/plugin.ts | 63 +++- packages/plugins/openapi/src/sdk/backing.ts | 10 +- plans/oauth-refresh-and-expired-status.md | 146 +++++++-- 18 files changed, 896 insertions(+), 385 deletions(-) create mode 100644 .changeset/graphql-network-prose.md create mode 100644 .changeset/mcp-liveness-pooled-probe.md create mode 100644 .changeset/probe-scope-shortfall.md create mode 100644 packages/plugins/graphql/src/sdk/health-classification.test.ts diff --git a/.changeset/graphql-network-prose.md b/.changeset/graphql-network-prose.md new file mode 100644 index 0000000000..d018b46762 --- /dev/null +++ b/.changeset/graphql-network-prose.md @@ -0,0 +1,5 @@ +--- +"@executor-js/plugin-graphql": patch +--- + +A GraphQL liveness probe no longer reads a transport failure's prose as a dead credential. Classification matched any upstream message containing "permission", which an operating-system refusal also carries — `connect EACCES: permission denied` on a socket reported the connection as `expired` and asked the user to re-enter a secret that was never the problem. Prose is now consulted only when the failure is not a transport failure; an HTTP 401 or 403 still classifies on its status. diff --git a/.changeset/mcp-liveness-pooled-probe.md b/.changeset/mcp-liveness-pooled-probe.md new file mode 100644 index 0000000000..d2d612111a --- /dev/null +++ b/.changeset/mcp-liveness-pooled-probe.md @@ -0,0 +1,5 @@ +--- +"@executor-js/plugin-mcp": patch +--- + +The MCP liveness probe now takes the invocation pool's connection instead of dialling a second one. A probe of a local stdio server previously started a second child process, and the common local servers permit one instance only — Chrome DevTools MCP owns a browser and a debug port, Playwright MCP the same, `docker run -i` a container. The second child could not start, so the health check reported the connection broken while the server was up and serving tool calls. Because the UI re-probes every non-healthy verdict on every mount, each page load started one more child. A probe now reuses the pooled session or child, and an interrupted probe still tears down whatever it acquired. diff --git a/.changeset/oauth-refresh-evidence.md b/.changeset/oauth-refresh-evidence.md index 9e84592a30..3d9741f836 100644 --- a/.changeset/oauth-refresh-evidence.md +++ b/.changeset/oauth-refresh-evidence.md @@ -9,3 +9,11 @@ A refresher whose grant is refused now reads the stored refresh token again. Whe `isPermanentTokenRejection` no longer reads 408, 425, or 429 as a definitive refusal. One rate-limited minute at a token endpoint therefore no longer ends a grant. Those statuses now behave like a 5xx response, and the next call retries. `connections.checkHealth` re-mints the token once and probes again before it answers `expired` for an OAuth connection. A revoked token, an idle timeout shorter than the advertised lifetime, or a null `expires_at` therefore no longer shows a working connection as dead. + +A connection whose integration declares no probe operation is now asked of the plugin first. A plugin that can answer without a spec — MCP lists its tools — gives a real verdict for its OAuth connections, which the credential-only branch never reached. Only when the plugin itself answers `unknown` does the credential-only verdict replace it, and that verdict is now computed from the values the probe already resolved, so nothing refreshes twice. A credential that resolves to nothing reads as `expired` there too, matching the plugins and heal-on-use. + +A refresh response that omits `expires_in` no longer erases `expires_at`. RFC 6749 makes the field optional, so an authorization server that advertised a lifetime on the code exchange and omitted it on refresh used to disable proactive refresh for the rest of the connection's life. The mint now records the advertised lifetime in `provider_state.oauthTokenLifetimeMs`, and a refresh without `expires_in` derives the expiry from it. + +A 403 scope shortfall on a probe now reads as `degraded` rather than `expired`: the credential authenticated, the grant is too narrow, and the remedy is a new consent rather than a reconnect. + +The test authorization server's MCP resource endpoint (`serveOAuthTestServer` at `/mcp`) now speaks the JSON-RPC protocol honestly: it answers the request's own id, answers `tools/list` with an empty catalog, and stays silent for notifications. The previous canned reply used a fixed id, so any client that completed the handshake waited forever for its `tools/list` response and every catalog sync or liveness probe against the endpoint timed out at the discovery deadline — a limitation invisible while OAuth health checks never dialled, and exposed once they do. diff --git a/.changeset/probe-scope-shortfall.md b/.changeset/probe-scope-shortfall.md new file mode 100644 index 0000000000..dfe0dae7b7 --- /dev/null +++ b/.changeset/probe-scope-shortfall.md @@ -0,0 +1,5 @@ +--- +"@executor-js/plugin-openapi": patch +--- + +A health probe that meets a 403 scope shortfall now reads as `degraded` rather than `expired`. The credential authenticated; the grant is narrower than the probe operation needs, and the remedy is a new consent with wider scope — which the connection's missing-scope affordance already offers. The old verdict told the user the connection was dead and sent them through a reconnect that could not widen the grant. Classification passes the response headers as well as the body, so an RFC 6750 `WWW-Authenticate: Bearer error="insufficient_scope"` challenge is recognised too. diff --git a/packages/core/sdk/src/connections.test.ts b/packages/core/sdk/src/connections.test.ts index 51c7f8d110..92fe43586c 100644 --- a/packages/core/sdk/src/connections.test.ts +++ b/packages/core/sdk/src/connections.test.ts @@ -2575,6 +2575,11 @@ const makeHealthHarness = (options?: { * `counters.probes`, so a Deferred-gated probe lets a test hold every * in-flight health check open and count how many actually started. */ readonly probe?: Effect.Effect; + /** Answer `unknown` when core passes no declared spec, the way the protocol + * plugins do: they have no operation to dial, so core falls back to the + * credential-only verdict. Without this the harness plugin answers every + * check, and the fallback is unreachable. */ + readonly declineWithoutSpec?: boolean; }) => { const counters = { probes: 0, resolves: 0 }; const hooks = { @@ -2660,9 +2665,16 @@ const makeHealthHarness = (options?: { ? ToolResult.fail({ code: "upstream_error", message: "boom" }) : { ran: toolRow.name, value: credential.value }, ), - checkHealth: () => + checkHealth: ({ spec }) => Effect.suspend(() => { counters.probes += 1; + if (options?.declineWithoutSpec === true && spec === undefined) { + return Effect.succeed({ + status: "unknown" as const, + checkedAt: Date.now(), + detail: "No health check configured.", + }); + } return ( options?.probe ?? Effect.succeed({ status: "healthy" as const, checkedAt: Date.now(), detail: "probe ok" }) @@ -3298,12 +3310,15 @@ describe("credential-only health path", () => { // parallel suite load the forked checks may not have finished their row // loads yet, and the counter reads 0. const entered = yield* Deferred.make(); - const { executor, counters, stamp, persisted, hooks } = yield* makeHealthHarness(); - // No declared probe spec + an OAuth client on the row routes checkHealth - // down the credential-only path: the verdict is "the credential - // resolved", produced without invoking the plugin probe. That path runs - // behind the same in-flight gate as probing, so concurrent checks must - // collapse to ONE resolution. + const { executor, counters, stamp, persisted, hooks } = yield* makeHealthHarness({ + declineWithoutSpec: true, + }); + // No declared probe spec + an OAuth client on the row: the plugin is + // asked first, declines for want of an operation to dial, and core falls + // back to the credential-only verdict — "the credential resolved", + // produced from the SAME resolution the plugin's probe used, so nothing + // refreshes twice. That path runs behind the same in-flight gate as + // probing, so concurrent checks must collapse to ONE resolution. yield* stamp({ oauth_client: "acme", expires_at: null }); hooks.onResolve = Deferred.succeed(entered, void 0).pipe( Effect.andThen(Deferred.await(gate)), @@ -3329,7 +3344,10 @@ describe("credential-only health path", () => { expect(first.status).toBe("healthy"); expect(second.status).toBe("healthy"); expect(counters.resolves).toBe(1); - expect(counters.probes).toBe(0); + // Both checks shared ONE gate entry, so the plugin was asked once and + // declined once; the verdict both callers received is the fallback. + expect(counters.probes).toBe(1); + expect(first.detail).toBe("Credential resolved (no probe configured)."); const row = yield* persisted(); expect(row?.lastHealth?.status).toBe("healthy"); diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 5e1abb6858..61934a0f5c 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -1013,6 +1013,33 @@ const decodeOAuthReauthRequiredProviderState = Schema.decodeUnknownOption( const oauthReauthRequiredFromProviderState = (value: unknown) => Option.getOrNull(decodeOAuthReauthRequiredProviderState(decodeJsonColumn(value))); +/** `provider_state` as a merge base: the object it holds, or an empty one. Every + * writer merges rather than replaces, so a concurrent record survives. */ +const providerStateRecord = (value: unknown): Record => { + const decoded = decodeJsonColumn(value); + return decoded != null && typeof decoded === "object" && !Array.isArray(decoded) + ? (decoded as Record) + : {}; +}; + +const decodeTokenLifetimeState = Schema.decodeUnknownOption( + Schema.Struct({ oauthTokenLifetimeMs: Schema.Number }), +); + +/** The access-token lifetime this grant advertised, in ms, when it ever + * advertised one. RFC 6749 makes `expires_in` OPTIONAL: an authorization + * server that sends it on the code exchange may omit it on refresh, and + * writing a null `expires_at` from such a response erased the only input the + * proactive refresh has — permanently, for the rest of the connection's life. + * Remembering the lifetime lets the next refresh derive an expiry from the + * grant it already knows. */ +const rememberedTokenLifetimeMs = (value: unknown): number | null => { + const decoded = Option.getOrNull(decodeTokenLifetimeState(decodeJsonColumn(value))); + return decoded === null || !Number.isFinite(decoded.oauthTokenLifetimeMs) + ? null + : decoded.oauthTokenLifetimeMs; +}; + type OAuthReauthRequiredState = NonNullable< ReturnType >; @@ -2314,13 +2341,7 @@ export const createExecutor = => { - const existingState = decodeJsonColumn(target.provider_state); - const mergedState = - existingState != null && - typeof existingState === "object" && - !Array.isArray(existingState) - ? (existingState as Record) - : {}; + const mergedState = providerStateRecord(target.provider_state); const health: HealthCheckResult = { status: "expired", checkedAt: Date.now(), @@ -2427,13 +2448,37 @@ export const createExecutor = = { - expires_at: nextExpiresAt, + expires_at: lifetimeMs === null ? null : Date.now() + lifetimeMs, updated_at: new Date(), }; if (token.scope !== undefined) set.oauth_scope = token.scope; + if (advertisedLifetimeMs !== null && advertisedLifetimeMs !== rememberedLifetimeMs) { + // Merge into the CURRENT `provider_state`, read fresh: this write must + // not bury a concurrent one — a dead-grant record, a missing-scope + // set — under the copy this refresh started from. + const ref: ConnectionRef = { + owner: row.owner as Owner, + integration: IntegrationSlug.make(row.integration), + name: ConnectionName.make(row.name), + }; + const fresh = yield* findConnectionRow(ref); + set.provider_state = { + ...providerStateRecord((fresh ?? row).provider_state), + oauthTokenLifetimeMs: advertisedLifetimeMs, + }; + } yield* core.updateMany("connection", { where: (b: AnyCb) => b.and( @@ -4523,6 +4568,12 @@ export const createExecutor = => - foldCredentialResolutionIntoVerdict( - resolveConnectionValues(row).pipe( - Effect.as({ - status: "healthy" as const, + /** The verdict for an OAuth connection whose integration declares no probe + * operation and whose plugin cannot invent one: "the credential resolved + * (refreshing if due)" is the only signal this path can produce, and a + * refresh failure reaches the caller as a folded + * CredentialResolutionError instead of through here. A null value means the + * stored credential is GONE — the same case the plugins and heal-on-use + * refuse to call healthy, because rendering omits that placement and an + * upstream that answers unauthenticated would otherwise look alive. */ + const credentialOnlyVerdict = (values: Record): HealthCheckResult => + Object.values(values).some((value) => value == null) + ? { + status: "expired", + checkedAt: Date.now(), + detail: "Connection has no resolvable credential value.", + reason: "credential_missing", + } + : { + status: "healthy", checkedAt: Date.now(), detail: "Credential resolved (no probe configured).", - }), - ), - ); + }; // Resolve an in-flight credential's value map (key-first validation) without // saving anything. Mirrors `resolveConnectionValues` for the saved-row path: @@ -5331,95 +5391,102 @@ export const createExecutor = = {}; const freshVerdict: Effect.Effect = - spec === undefined && connectionRow.oauth_client != null - ? // No probe operation is declared, so "healthy" here means only - // "the credential resolved (refreshing if due)" — a refresh - // failure is the one real signal this path can produce, and it - // must not hide inside a green span. - oauthCredentialHealthWithoutProbe(connectionRow).pipe( - Effect.tap((result) => persistProbeHealthResult(ref, result)), - Effect.map((result) => ({ - source: "credential_only" as const, - result, - })), - ) - : foldCredentialResolutionIntoVerdict( - Effect.gen(function* () { - const record = rowToIntegrationRecord( - integrationRow, - yield* describeAuthMethodsForRow(integrationRow), - ); - const grantedScopes = grantedScopesFromRow(connectionRow); - const probe = ( - values: Record, - ): Effect.Effect => { - const credential: ToolInvocationCredential = { - owner: connectionRow.owner as Owner, - integration: ref.integration, - connection: ConnectionName.make(connectionRow.name), - template: AuthTemplateSlug.make(connectionRow.template), - value: values[PRIMARY_INPUT_VARIABLE] ?? null, - values, - config: record.config, - ...(grantedScopes ? { grantedScopes } : {}), - }; - // Core resolves the declared spec (its own column) and - // hands it to the plugin; plugins no longer read it out of - // their config. - return foldPluginFailure( - check({ - ctx: runtime.ctx, - integration: record, - credential, - spec, - }), - `Health check for connection "${ref.name}" failed.`, - ); - }; - const values = yield* resolveConnectionValues(connectionRow); - const first = yield* probe(values); - // A probe answers from the credential it was handed, so its - // `expired` is only as good as that credential. The invoke - // path knows this and re-mints once on a 401 - // (`forceRefreshConnectionValues`); the probe did not, which - // persisted `expired` for exactly the connections the - // reactive refresh exists for — a server-side revocation, an - // idle timeout shorter than the advertised lifetime, a null - // `expires_at` the proactive check can never fire on. The - // badge then said "reconnect" for a connection that worked - // on its next call, and only heal-on-use corrected it. - // - // One forced refresh and one re-probe, for an OAuth - // connection only. A refusal keeps the probe's own verdict: - // it is the more informative of the two, and the refresh - // path has already recorded a dead grant if there is one. - if (first.status !== "expired" || connectionRow.oauth_client == null) { - return first; - } - const refreshed = yield* forceRefreshConnectionValues(connectionRow).pipe( - Effect.catchTag("CredentialResolutionError", () => Effect.succeed(null)), - ); - if (refreshed === null) return first; - yield* Effect.annotateCurrentSpan({ - "executor.health.refresh_retried": true, - }); - return yield* probe(refreshed); - }), - ).pipe( - // Persist the verdict on the connection row so the accounts - // list shows alive/expired at a glance, AND so the freshness - // gate above has something to serve. A probe that could not - // resolve its credential persists too: it is the connection - // most likely to be re-probed by every surface on every - // mount, so leaving it unwritten is what turns one broken - // connection into unbounded upstream and error traffic. - Effect.tap((result) => persistProbeHealthResult(ref, result)), - Effect.map((result) => ({ - source: "probe" as const, - result, - })), + foldCredentialResolutionIntoVerdict( + Effect.gen(function* () { + const record = rowToIntegrationRecord( + integrationRow, + yield* describeAuthMethodsForRow(integrationRow), ); + const grantedScopes = grantedScopesFromRow(connectionRow); + const probe = ( + values: Record, + ): Effect.Effect => { + const credential: ToolInvocationCredential = { + owner: connectionRow.owner as Owner, + integration: ref.integration, + connection: ConnectionName.make(connectionRow.name), + template: AuthTemplateSlug.make(connectionRow.template), + value: values[PRIMARY_INPUT_VARIABLE] ?? null, + values, + config: record.config, + ...(grantedScopes ? { grantedScopes } : {}), + }; + // Core resolves the declared spec (its own column) and + // hands it to the plugin; plugins no longer read it out of + // their config. + return foldPluginFailure( + check({ + ctx: runtime.ctx, + integration: record, + credential, + spec, + }), + `Health check for connection "${ref.name}" failed.`, + ); + }; + const values = yield* resolveConnectionValues(connectionRow); + resolvedValues = values; + const first = yield* probe(values); + // A probe answers from the credential it was handed, so its + // `expired` is only as good as that credential. The invoke + // path knows this and re-mints once on a 401 + // (`forceRefreshConnectionValues`); the probe did not, which + // persisted `expired` for exactly the connections the + // reactive refresh exists for — a server-side revocation, an + // idle timeout shorter than the advertised lifetime, a null + // `expires_at` the proactive check can never fire on. The + // badge then said "reconnect" for a connection that worked + // on its next call, and only heal-on-use corrected it. + // + // One forced refresh and one re-probe, for an OAuth + // connection only. A refusal keeps the probe's own verdict: + // it is the more informative of the two, and the refresh + // path has already recorded a dead grant if there is one. + if (first.status !== "expired" || connectionRow.oauth_client == null) { + return first; + } + const refreshed = yield* forceRefreshConnectionValues(connectionRow).pipe( + Effect.catchTag("CredentialResolutionError", () => Effect.succeed(null)), + ); + if (refreshed === null) return first; + yield* Effect.annotateCurrentSpan({ + "executor.health.refresh_retried": true, + }); + return yield* probe(refreshed); + }), + ).pipe( + // Ask the plugin FIRST, even with no declared spec: a plugin + // whose `checkHealth` ignores the spec (MCP lists tools) now + // gives a real verdict for its OAuth connections, which the + // old credential-only branch never reached. Only when the + // plugin itself answers `unknown` — it cannot invent a probe + // operation — does the credential-only verdict replace it, + // computed from the values the probe already resolved so + // nothing refreshes twice. + Effect.map((result) => + spec === undefined && + connectionRow.oauth_client != null && + result.status === "unknown" + ? { + source: "credential_only" as const, + result: credentialOnlyVerdict(resolvedValues), + } + : { source: "probe" as const, result }, + ), + // Persist the verdict on the connection row so the accounts + // list shows alive/expired at a glance, AND so the freshness + // gate above has something to serve. A probe that could not + // resolve its credential persists too: it is the connection + // most likely to be re-probed by every surface on every + // mount, so leaving it unwritten is what turns one broken + // connection into unbounded upstream and error traffic. + Effect.tap((outcome) => persistProbeHealthResult(ref, outcome.result)), + ); const run = freshVerdict.pipe( Effect.exit, Effect.flatMap((exit) => Deferred.done(deferred, exit)), diff --git a/packages/core/sdk/src/health-check.test.ts b/packages/core/sdk/src/health-check.test.ts index 61b652267e..c3f650912b 100644 --- a/packages/core/sdk/src/health-check.test.ts +++ b/packages/core/sdk/src/health-check.test.ts @@ -137,4 +137,36 @@ describe("classifyProbeResponse", () => { ); } }); + + // A scope shortfall authenticated: the credential works, the grant is + // narrower than this operation needs, and the remedy is a NEW CONSENT — which + // the connection's `missingOAuthScopes` already offers. Reporting it as + // `expired` told the user the connection was dead and sent them through a + // reconnect that could not widen the grant. + it("classifies an RFC 6750 insufficient_scope challenge as degraded", () => { + expect( + classifyProbeResponse(403, undefined, { + "www-authenticate": 'Bearer error="insufficient_scope", scope="read write"', + }), + ).toBe("degraded"); + }); + + it("classifies a body-named insufficient_scope as degraded", () => { + expect(classifyProbeResponse(403, { error: "insufficient_scope" })).toBe("degraded"); + expect( + classifyProbeResponse(403, { + error: { code: 403, details: [{ reason: "ACCESS_TOKEN_SCOPE_INSUFFICIENT" }] }, + }), + ).toBe("degraded"); + }); + + it("keeps the configuration carve-out ahead of the scope one", () => { + expect( + classifyProbeResponse( + 403, + { error: { errors: [{ reason: "accessNotConfigured" }], code: 403 } }, + { "www-authenticate": 'Bearer error="insufficient_scope"' }, + ), + ).toBe("misconfigured"); + }); }); diff --git a/packages/core/sdk/src/health-check.ts b/packages/core/sdk/src/health-check.ts index a4d6f1cb20..892c5408d7 100644 --- a/packages/core/sdk/src/health-check.ts +++ b/packages/core/sdk/src/health-check.ts @@ -17,6 +17,8 @@ import { Schema } from "effect"; +import { detectInsufficientScope } from "./insufficient-scope"; + // --------------------------------------------------------------------------- // Status: the five states a connection can be in. `expired` is the one this // whole feature exists for (Google's 7-day dev-token revocation): the credential @@ -242,16 +244,32 @@ const errorReasonMarkers = (body: unknown): string[] => { }; /** Classify a probe response from its status AND body. Everything is - * `classifyHttpStatus` except one carve-out: a 403 whose error body carries a - * known configuration reason (Google `accessNotConfigured` / - * `SERVICE_DISABLED`) is `misconfigured`, not `expired`: the credential - * authenticated; the upstream API is disabled in the OAuth client's project, - * and only enabling it there (not reconnecting) fixes it. */ -export const classifyProbeResponse = (status: number, body: unknown): HealthStatus => { + * `classifyHttpStatus` except two carve-outs on a 403: + * + * - A known configuration reason (Google `accessNotConfigured` / + * `SERVICE_DISABLED`) is `misconfigured`: the credential authenticated, the + * upstream API is disabled in the OAuth client's project, and only enabling + * it there (not reconnecting) fixes it. + * - A scope shortfall (RFC 6750 `insufficient_scope` in `WWW-Authenticate`, + * `error: insufficient_scope` in the body, Google's + * `ACCESS_TOKEN_SCOPE_INSUFFICIENT`) is `degraded`: the credential + * authenticated too, and the remedy is a NEW CONSENT with wider scope — + * which the connection's `missingOAuthScopes` already offers — not a + * reconnect. Reporting it as `expired` told the user the connection was dead + * and sent them through a flow that could not fix it. `headers` is optional + * so a caller that only kept the body still gets the body-based detection. */ +export const classifyProbeResponse = ( + status: number, + body: unknown, + headers?: Record, +): HealthStatus => { const byStatus = classifyHttpStatus(status); if (status !== 403 || byStatus !== "expired") return byStatus; - return errorReasonMarkers(body).some((reason) => CONFIGURATION_403_REASONS.has(reason)) - ? "misconfigured" + if (errorReasonMarkers(body).some((reason) => CONFIGURATION_403_REASONS.has(reason))) { + return "misconfigured"; + } + return detectInsufficientScope({ body, ...(headers === undefined ? {} : { headers }) }) !== null + ? "degraded" : "expired"; }; diff --git a/packages/core/sdk/src/oauth-expired-status-repro.test.ts b/packages/core/sdk/src/oauth-expired-status-repro.test.ts index 0cd834b8e8..76846f456a 100644 --- a/packages/core/sdk/src/oauth-expired-status-repro.test.ts +++ b/packages/core/sdk/src/oauth-expired-status-repro.test.ts @@ -483,6 +483,70 @@ describe("R2 — a rate-limited refresh stays retryable", () => { ); }); +// --------------------------------------------------------------------------- +// R5 — a refresh response without `expires_in` keeps the advertised lifetime. +// --------------------------------------------------------------------------- + +interface StrippingEndpoint { + readonly url: string; + readonly attempts: () => number; + readonly close: () => void; +} + +/** A token endpoint that answers a refresh grant with a valid token response + * that OMITS `expires_in`, which RFC 6749 permits, and rotates the refresh + * token like the real one does. It stands in for an authorization server that + * advertised a lifetime on the code exchange and then stopped repeating it. */ +const serveExpiresInStrippingEndpoint = () => + Effect.acquireRelease( + Effect.callback((resume) => { + let attempts = 0; + 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")) { + res.writeHead(400, { "content-type": "text/plain" }); + res.end("this fixture answers refresh grants only"); + return; + } + attempts += 1; + res.writeHead(200, { "content-type": "application/json", "cache-control": "no-store" }); + res.end( + `{"access_token":"at_stripped_${attempts}","refresh_token":"rt_stripped_${attempts}","token_type":"Bearer"}`, + ); + void req; + }); + }); + 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()), + ); + +/** `connection.expires_at` off the raw row, which adapters return as a number or + * a string. */ +const RowExpiry = Schema.Struct({ expires_at: Schema.optional(Schema.Unknown) }); +const decodeRowExpiry = Schema.decodeUnknownOption(RowExpiry); +const rowExpiresAt = (row: unknown): number | null => { + const value = Option.getOrUndefined(decodeRowExpiry(row))?.expires_at; + // A bigint column: adapters hand back a number, a string, or a BigInt. + if (typeof value === "number") return value; + if (typeof value === "bigint") return Number(value); + if (typeof value === "string") return Number(value); + return null; +}; + // --------------------------------------------------------------------------- // R3 — the probe refreshes before it answers expired. // --------------------------------------------------------------------------- @@ -535,3 +599,51 @@ describe("R3 — the probe refreshes before it answers expired", () => { ), ); }); + +// --------------------------------------------------------------------------- +// R5 — a refresh response without `expires_in` must not erase the expiry. +// --------------------------------------------------------------------------- + +describe("R5 — a refresh response that omits expires_in", () => { + it.effect("keeps the advertised lifetime, so proactive refresh survives", () => + Effect.scoped( + Effect.gen(function* () { + const race = yield* makeRace({}); + const mintedExpiry = rowExpiresAt(yield* race.rawRow()); + expect(mintedExpiry, "the mint recorded the advertised expiry").not.toBeNull(); + expect( + (mintedExpiry ?? 0) - Date.now(), + "and the test authorization server advertised an hour", + ).toBeGreaterThan(30 * 60_000); + + const stripping = yield* serveExpiresInStrippingEndpoint(); + yield* Effect.promise(() => + race.config.db.updateMany("oauth_client", { + where: (builder) => builder("slug", "=", String(CLIENT)), + set: { token_url: stripping.url }, + }), + ); + yield* race.expire(); + + const first = yield* Effect.exit(race.a.execute(ADDRESS, {})); + expect(Exit.isSuccess(first), "the refresh succeeded").toBe(true); + expect(stripping.attempts(), "and it went to the endpoint that omits expires_in").toBe(1); + + // This wrote null before the fix, which disabled the proactive check + // for the rest of the connection's life and left every later call to + // the reactive 401 path. + const refreshedExpiry = rowExpiresAt(yield* race.rawRow()); + expect(refreshedExpiry, "the expiry survived a response without expires_in").not.toBeNull(); + expect( + (refreshedExpiry ?? 0) - Date.now(), + "and it carries the lifetime the grant advertised", + ).toBeGreaterThan(30 * 60_000); + + // The proactive path still works, so the next call needs no grant. + const second = yield* Effect.exit(race.a.execute(ADDRESS, {})); + expect(Exit.isSuccess(second), "the next call used the stored token").toBe(true); + expect(stripping.attempts(), "and sent no second grant").toBe(1); + }), + ), + ); +}); diff --git a/packages/core/sdk/src/oauth-flow.test.ts b/packages/core/sdk/src/oauth-flow.test.ts index 7496ce1efd..a3f82ae5e6 100644 --- a/packages/core/sdk/src/oauth-flow.test.ts +++ b/packages/core/sdk/src/oauth-flow.test.ts @@ -54,11 +54,17 @@ const oauthPlugin = definePlugin(() => ({ }, // Echo the resolved credential value (the OAuth access token) back out. invokeTool: ({ credential }) => Effect.succeed({ token: credential.value }), - checkHealth: ({ credential }) => - Effect.succeed({ - status: credential.value === null ? "expired" : "healthy", - checkedAt: Date.now(), - }), + // Mirrors the protocol plugins: with no declared probe operation there is + // nothing to dial, so the plugin answers `unknown` and core falls back to the + // credential-only verdict. A plugin that CAN answer without a spec (MCP lists + // tools) is asked first and gives a real verdict. + checkHealth: ({ credential, spec }) => + spec === undefined + ? Effect.succeed({ status: "unknown" as const, checkedAt: Date.now() }) + : Effect.succeed({ + status: credential.value === null ? ("expired" as const) : ("healthy" as const), + checkedAt: Date.now(), + }), extension: (ctx) => ({ seed: (scopes: readonly string[] = []) => ctx.core.integrations.register({ @@ -2085,7 +2091,7 @@ describe("oauth token refresh in resolveConnectionValue", () => { where: (b) => b("name", "=", "main"), }), ); - expect(row?.provider_state).toEqual({ missingOAuthScopes: ["write"] }); + expect(row?.provider_state).toMatchObject({ missingOAuthScopes: ["write"] }); const listed = yield* executor.connections.list({ integration: INTEG }); expect(listed[0]?.missingOAuthScopes).toEqual(["write"]); }), @@ -2136,7 +2142,12 @@ describe("oauth token refresh in resolveConnectionValue", () => { where: (b) => b("name", "=", "main"), }), ); - expect(row?.provider_state).toBeNull(); + // No missing-scope record. `provider_state` itself is not null: the + // mint records the advertised token lifetime there so a later refresh + // whose response omits `expires_in` can still derive an expiry. + expect( + (row?.provider_state as { missingOAuthScopes?: unknown } | null)?.missingOAuthScopes, + ).toBeUndefined(); }), ), ); diff --git a/packages/core/sdk/src/testing/oauth-test-server.ts b/packages/core/sdk/src/testing/oauth-test-server.ts index 0380e389f0..bd68d826c9 100644 --- a/packages/core/sdk/src/testing/oauth-test-server.ts +++ b/packages/core/sdk/src/testing/oauth-test-server.ts @@ -1076,10 +1076,45 @@ export const serveOAuthTestServer = ( }, ); } + // A minimal but honest MCP resource server. The old canned reply used + // a FIXED json-rpc id and answered notifications too, so a client + // that completed the handshake waited forever for its `tools/list` + // response: every catalog sync and every liveness probe against this + // endpoint timed out at the discovery deadline. Answer the request's + // OWN id, answer `tools/list` with an empty catalog, and stay silent + // for notifications, which is the protocol. + const decodeMcpFrame = Schema.decodeUnknownOption(Schema.fromJsonString(Schema.Unknown)); + const frame = Option.getOrUndefined(decodeMcpFrame(body)); + const frameIsRecord = frame !== null && typeof frame === "object"; + const frameRecord = frameIsRecord ? (frame as Record) : {}; + const frameMethod = + typeof frameRecord["method"] === "string" ? frameRecord["method"] : ""; + const hasId = "id" in frameRecord && frameRecord["id"] !== undefined; + if (!hasId) { + // Accepted with no body: a notification has no reply. + return HttpServerResponse.empty({ status: 202 }); + } + const params = + frameRecord["params"] !== null && typeof frameRecord["params"] === "object" + ? (frameRecord["params"] as Record) + : {}; + const reply = + frameMethod === "tools/list" + ? { tools: [] } + : frameMethod === "initialize" + ? { + protocolVersion: + typeof params["protocolVersion"] === "string" + ? params["protocolVersion"] + : "2025-06-18", + capabilities: { tools: {} }, + serverInfo: { name: "oauth-test-server", version: "0.0.0" }, + } + : {}; return jsonResponse(200, { jsonrpc: "2.0", - id: 1, - result: { protocolVersion: "2025-06-18", capabilities: {} }, + id: frameRecord["id"], + result: reply, }); } diff --git a/packages/plugins/graphql/src/sdk/health-classification.test.ts b/packages/plugins/graphql/src/sdk/health-classification.test.ts new file mode 100644 index 0000000000..eb1d0b5489 --- /dev/null +++ b/packages/plugins/graphql/src/sdk/health-classification.test.ts @@ -0,0 +1,48 @@ +// The prose a transport failure carries is the operating system's or the HTTP +// client's, not the upstream's verdict on a credential. Reading it as one sent +// users to re-enter a secret that was never the problem: `EACCES: permission +// denied` on a socket matches any pattern looking for the word "permission". + +import { describe, expect, it } from "@effect/vitest"; + +import { GraphqlIntrospectionError } from "./errors"; +import { healthFromIntrospectionError } from "./plugin"; + +const classify = (input: { + readonly reason?: "network" | "graphql-errors" | "invalid-json"; + readonly status?: number; + readonly upstreamMessage?: string; +}) => + healthFromIntrospectionError( + new GraphqlIntrospectionError({ + message: "introspection failed", + ...(input.reason === undefined ? {} : { reason: input.reason }), + ...(input.status === undefined ? {} : { status: input.status }), + ...(input.upstreamMessage === undefined ? {} : { upstreamMessage: input.upstreamMessage }), + }), + Date.now(), + ); + +describe("GraphQL liveness classification", () => { + it("does not read a transport failure's prose as a dead credential", () => { + const verdict = classify({ + reason: "network", + upstreamMessage: "connect EACCES: permission denied /run/graphql.sock", + }); + expect(verdict.status).not.toBe("expired"); + }); + + it("still reads an authentication failure the upstream named as expired", () => { + const verdict = classify({ + reason: "graphql-errors", + upstreamMessage: "Authentication required: invalid token", + }); + expect(verdict.status).toBe("expired"); + }); + + it("classifies an HTTP 401 on its status whatever else is said", () => { + const verdict = classify({ status: 401, upstreamMessage: "nope" }); + expect(verdict.status).toBe("expired"); + expect(verdict.httpStatus).toBe(401); + }); +}); diff --git a/packages/plugins/graphql/src/sdk/plugin.ts b/packages/plugins/graphql/src/sdk/plugin.ts index 176e6d7721..dfe3f77f44 100644 --- a/packages/plugins/graphql/src/sdk/plugin.ts +++ b/packages/plugins/graphql/src/sdk/plugin.ts @@ -114,6 +114,14 @@ const appendUpstreamMessage = (detail: string, message?: string): string => ? `${detail} Upstream said: ${truncateHealthDetail(message)}` : detail; +/** Whether an upstream's own prose names an authentication failure. + * + * Text matching, so it is deliberately the WEAKER signal: it is consulted only + * when no HTTP status classified the failure, and never for a transport + * failure. An OS-level refusal carries the word "permission" too — `EACCES: + * permission denied` on a socket or a binary — and reading that as a dead + * credential sent the user to re-enter a secret that was never the problem. + * The caller excludes `reason: "network"` for exactly that case. */ const isAuthMessage = (message: string | undefined): boolean => message !== undefined && /authoriz|authenticat|forbidden|permission|credential|api.?key|access denied|access token|invalid token|token expired|logged in|sign in/i.test( @@ -141,7 +149,14 @@ const missingCredentialVariables = ( }); }; -const healthFromIntrospectionError = ( +/** Classify one introspection failure as a health verdict. + * + * Exported for tests (not re-exported from `sdk/index.ts`, so this widens no + * public API): the classification rules — which prose counts as an + * authentication failure, and which reason may never be read from prose — are + * the whole behavior under test, and reaching them through a live introspection + * would test the transport instead. */ +export const healthFromIntrospectionError = ( error: GraphqlIntrospectionError, checkedAt: number, ): HealthCheckResult => { @@ -160,7 +175,12 @@ const healthFromIntrospectionError = ( }; } - if (httpStatus === 401 || httpStatus === 403 || isAuthMessage(upstream)) { + // A transport failure never classifies from prose: its message is the OS's or + // the HTTP client's, and "permission denied" there names a socket, not a + // credential (see `isAuthMessage`). An HTTP 401/403 still classifies on its + // own status whatever the reason. + const proseSaysAuth = error.reason !== "network" && isAuthMessage(upstream); + if (httpStatus === 401 || httpStatus === 403 || proseSaysAuth) { const statusDetail = httpStatus === 401 || httpStatus === 403 ? `The endpoint rejected the credential with HTTP ${httpStatus}.` diff --git a/packages/plugins/mcp/src/sdk/discover.ts b/packages/plugins/mcp/src/sdk/discover.ts index d3c672361d..900a5a5f0b 100644 --- a/packages/plugins/mcp/src/sdk/discover.ts +++ b/packages/plugins/mcp/src/sdk/discover.ts @@ -5,7 +5,11 @@ import { Duration, Effect, Option, Predicate, Schema } from "effect"; import { hasNestedOAuthReauthorization, type McpConnection, type McpConnector } from "./connection"; -import { McpToolDiscoveryError } from "./errors"; +import { + type McpConnectionError, + type McpOAuthReauthorizationRequired, + McpToolDiscoveryError, +} from "./errors"; import { createMcpConnector, type ConnectorInput } from "./connection"; import { httpStatusFromCause } from "./http-status"; import { @@ -29,7 +33,8 @@ const MAX_LIST_TOOLS_PAGES = 100; // (`probeMcpEndpointShape`'s `timeoutMs = 8_000`) at a slightly longer // bound since a real handshake + listTools round-trip is heavier than the // shape probe's single unauth POST. -const DEFAULT_DISCOVER_TIMEOUT = Duration.seconds(15); +/** The shared deadline for one discovery: dial plus list. */ +export const DEFAULT_DISCOVER_TIMEOUT = Duration.seconds(15); // Teardown is best-effort and paid for by the request that performed discovery. // A remote transport may accept close and then never settle, so use the same @@ -160,6 +165,84 @@ export const discoverToolsFromInput = ( }), ); +/** Turn a connection failure into the discovery failure every caller of this + * module handles. Exported because a caller that takes its connection from the + * invocation pool meets the raw connector errors itself: the pool dials, this + * module only lists. Keeping the mapping here is what makes a pooled liveness + * probe classify a 401, a 403, and a connect timeout exactly as a dialling one + * does. + * + * Preserves the handshake HTTP status (401/403 = auth wall) and a + * connect-level timeout so the liveness health check can classify + * structurally — dropping `failureKind: "timeout"` here is what made a timed-out + * handshake read as a generic probe failure. */ +export const connectionFailureToDiscoveryError = ( + failure: McpConnectionError | McpOAuthReauthorizationRequired, +): McpToolDiscoveryError => { + const httpStatus = Predicate.isTagged(failure, "McpConnectionError") + ? failure.httpStatus + : undefined; + const reauthorizationRequired = Predicate.isTagged(failure, "McpOAuthReauthorizationRequired"); + const timedOut = + Predicate.isTagged(failure, "McpConnectionError") && failure.failureKind === "timeout"; + return new McpToolDiscoveryError({ + stage: "connect", + message: `Failed connecting to MCP server: ${failure.message}`, + ...(httpStatus !== undefined ? { httpStatus } : {}), + ...(reauthorizationRequired ? { reauthorizationRequired: true } : {}), + ...(timedOut ? { timedOut } : {}), + }); +}; + +/** Bound a discovery step with the shared deadline and the shared timeout error. + * + * One definition so every path answers a wedged server identically — and every + * path is bounded. The pool's `withConnection` dials through its own acquire + * with no deadline of its own, so a caller that takes its connection from the + * pool must wrap the WHOLE lease in this deadline: without it, a server that + * never completes its handshake hangs the health check that used to time out + * at fifteen seconds. On timeout the lease releases (the pool closes a + * connection its lease failed on), so no child or session is left behind. */ +export const withDiscoveryTimeout = ( + effect: Effect.Effect, + timeoutMs: number, +): Effect.Effect => + effect.pipe( + Effect.timeoutOrElse({ + duration: Duration.millis(timeoutMs), + orElse: () => + Effect.fail( + new McpToolDiscoveryError({ + stage: "connect", + message: `MCP discovery timed out after ${timeoutMs}ms`, + timedOut: true, + }), + ), + }), + ); + +/** The listing half of discovery, over a connection the CALLER owns. + * + * `discoverTools` dials, lists, and closes. A caller that already holds an + * open connection must not close it: the liveness health check takes a lease + * from the invocation pool, and tool calls still need that session afterwards. + * This is the same listing work — same deadline, same elicitation refusal — + * with no teardown. */ +export const discoverToolsFromConnection = ( + connection: McpConnection, + timeoutMs: number = Duration.toMillis(DEFAULT_DISCOVER_TIMEOUT), +): Effect.Effect => + withDiscoveryTimeout( + Effect.gen(function* () { + // Decline elicitation explicitly; see the same call in `discoverTools`. + connection.client.setRequestHandler("elicitation/create", () => + Promise.resolve({ action: "decline" }), + ); + return yield* listAllTools(connection); + }), + timeoutMs, + ); + /** * Connect to an MCP server and discover all available tools. * Returns the parsed manifest containing server metadata and tool entries. @@ -184,68 +267,34 @@ export const discoverTools = ( connector: McpConnector, timeoutMs: number = Duration.toMillis(DEFAULT_DISCOVER_TIMEOUT), ): Effect.Effect => - Effect.uninterruptibleMask((restore) => - Effect.gen(function* () { - // Acquire connection - const connection = yield* restore( - connector.pipe( - Effect.mapError((failure) => { - // Preserve the handshake HTTP status (401/403 = auth wall) and a - // connect-level timeout so the liveness health check can classify - // structurally — dropping `failureKind: "timeout"` here is what - // made a timed-out handshake read as a generic probe failure. - const httpStatus = Predicate.isTagged(failure, "McpConnectionError") - ? failure.httpStatus - : undefined; - const reauthorizationRequired = Predicate.isTagged( - failure, - "McpOAuthReauthorizationRequired", - ); - const timedOut = - Predicate.isTagged(failure, "McpConnectionError") && - failure.failureKind === "timeout"; - return new McpToolDiscoveryError({ - stage: "connect", - message: `Failed connecting to MCP server: ${failure.message}`, - ...(httpStatus !== undefined ? { httpStatus } : {}), - ...(reauthorizationRequired ? { reauthorizationRequired: true } : {}), - ...(timedOut ? { timedOut } : {}), - }); - }), - ), - ); + withDiscoveryTimeout( + Effect.uninterruptibleMask((restore) => + Effect.gen(function* () { + // Acquire connection + const connection = yield* restore( + connector.pipe(Effect.mapError(connectionFailureToDiscoveryError)), + ); - // The connection advertises the elicitation capability (connection.ts), - // so a server may elicit mid-listTools — the Codex desktop plugins do - // this for first-use approvals. Discovery has no user to route the - // request to (unlike the invoke path's bridge in invoke.ts), and a - // handler-less request would surface as a method-not-found error on the - // server's side of an otherwise healthy sync. Decline explicitly: the - // server completes the list with whatever it allows unapproved. - connection.client.setRequestHandler("elicitation/create", () => - Promise.resolve({ action: "decline" }), - ); + // The connection advertises the elicitation capability (connection.ts), + // so a server may elicit mid-listTools — the Codex desktop plugins do + // this for first-use approvals. Discovery has no user to route the + // request to (unlike the invoke path's bridge in invoke.ts), and a + // handler-less request would surface as a method-not-found error on the + // server's side of an otherwise healthy sync. Decline explicitly: the + // server completes the list with whatever it allows unapproved. + connection.client.setRequestHandler("elicitation/create", () => + Promise.resolve({ action: "decline" }), + ); - const manifest = yield* restore(listAllTools(connection)).pipe( - Effect.onExit(() => closeConnection(connection)), - ); + const manifest = yield* restore(listAllTools(connection)).pipe( + Effect.onExit(() => closeConnection(connection)), + ); - return manifest; - }), - ).pipe( - Effect.timeoutOrElse({ - duration: Duration.millis(timeoutMs), - orElse: () => - Effect.fail( - new McpToolDiscoveryError({ - stage: "connect", - message: `MCP discovery timed out after ${timeoutMs}ms`, - timedOut: true, - }), - ), - }), + return manifest; + }), + ), + timeoutMs, ); - const closeConnection = (connection: { readonly close: () => Promise; }): Effect.Effect => 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 index 7f358f56d0..1f88c11650 100644 --- a/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts +++ b/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts @@ -1,38 +1,32 @@ // --------------------------------------------------------------------------- -// A liveness probe must not conclude "this connection is broken" from a -// failure its OWN second connection caused. +// A liveness probe must not dial a second connection when the invocation pool +// already holds one. // -// `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. +// `checkHealth` used to call `discoverToolsFromInput`, which builds a FRESH +// connector (`discover.ts` → `createMcpConnector`) instead of taking the pooled +// connection that tool calls 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 starts a SECOND CHILD PROCESS — and the common local +// servers permit one instance only: Chrome DevTools MCP owns a browser and a +// debug port, Playwright MCP the same, `docker run -i` a container. The second +// child could not start, so the probe reported the connection broken while the +// server was up and serving the pooled client. The UI re-probes every +// non-healthy verdict on every mount, so each page load started one more child. // -// 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. +// The fixture makes that failure deterministic: it refuses to start while a +// live process holds its lock. Two probes therefore pass only if the second one +// reuses the first one's child. // // `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 { Effect } from "effect"; import { FetchHttpClient } from "effect/unstable/http"; import { mcpPlugin } from "./plugin"; @@ -41,45 +35,27 @@ const fixture = fileURLToPath(new URL("./stdio-single-instance-test-server.ts", 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", - }, - }); - }); +type CheckHealth = (input: { + readonly ctx: { readonly httpClientLayer: typeof FetchHttpClient.layer }; + readonly credential: { + readonly config: unknown; + readonly values: Record; + readonly template: string | null; + readonly owner: string; + readonly connection: string; + readonly integration: string; + }; +}) => Effect.Effect; -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; - }); +/** One plugin instance, so both probes share its connection pool — the same + * lifetime the pool has in a host. */ +const pluginCheckHealth = (): CheckHealth => { + const plugin = mcpPlugin({ dangerouslyAllowStdioMCP: true }); + const seam = (plugin as { readonly checkHealth?: CheckHealth }).checkHealth; + // The seam is part of the plugin contract. A build without it cannot run this + // scenario at all, so die rather than invent a verdict. + return seam ?? ((() => Effect.die("mcpPlugin no longer exposes checkHealth")) as CheckHealth); +}; const spawnedPids = (log: string): readonly number[] => existsSync(log) @@ -89,106 +65,59 @@ const spawnedPids = (log: string): readonly number[] => .map((line) => Number(line)) : []; +/** Stop every child the fixture logged. The pool keeps an idle child alive by + * design, and a test must not leave one behind. */ +const stopSpawned = (log: string): Effect.Effect => + Effect.sync(() => { + for (const pid of spawnedPids(log)) { + // oxlint-disable-next-line executor/no-try-catch-or-throw -- boundary: kill throws ESRCH when the child already exited, which is the desired state + try { + process.kill(pid, "SIGTERM"); + } catch { + // already gone + } + } + }); + 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], - }; + it.live("reuses the pooled child, so a second probe starts no second process", () => + 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], + }; + const checkHealth = pluginCheckHealth(); + const credential = { + config, + values: {}, + template: null, + owner: "user", + connection: "main", + integration: "single_instance_mcp", + }; + const ctx = { httpClientLayer: FetchHttpClient.layer }; - // 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( + yield* Effect.acquireUseRelease( + Effect.void, + () => 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; - }), - ); + const first = yield* checkHealth({ ctx, credential }); + expect(first.status, "the first probe dials and the server answers").toBe("healthy"); + expect(spawnedPids(spawnLog), "and it started exactly one child").toHaveLength(1); - // 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", () => {}); + // The pooled child is alive and still holds the lock, so a second + // dial could not start. This probe passes only by reuse. + expect(existsSync(lockFile), "the first child is still running").toBe(true); + const second = yield* checkHealth({ ctx, credential }); + expect(second.status, "the second probe reads the same live server").toBe("healthy"); + expect(spawnedPids(spawnLog), "and it started no second child").toHaveLength(1); }), - () => - 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"); - }), - ); - }), + () => stopSpawned(spawnLog), + ); + }), ); }); diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index 079b89dce5..a3f6af8a25 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -1,4 +1,4 @@ -import { Effect, Layer, Option, Result, Schema } from "effect"; +import { Duration, Effect, Layer, Option, Predicate, Result, Schema } from "effect"; import type { HttpClient } from "effect/unstable/http"; import type { OAuthClientProvider } from "@modelcontextprotocol/client"; @@ -46,7 +46,13 @@ import { import type { CodexPluginEntry } from "./codex-plugins"; import { createMcpConnector, type ConnectorInput, type McpConnector } from "./connection"; import { createMcpConnectionPool } from "./connection-pool"; -import { discoverToolsFromInput } from "./discover"; +import { + connectionFailureToDiscoveryError, + DEFAULT_DISCOVER_TIMEOUT, + discoverToolsFromConnection, + discoverToolsFromInput, + withDiscoveryTimeout, +} from "./discover"; import { McpConnectionError, type McpConnectionFailureKind, @@ -1969,7 +1975,7 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { } } } - const connector = yield* buildConnectorInput( + const connectorInput = yield* buildConnectorInput( parsed, credential.values, credential.template === null ? null : String(credential.template), @@ -1977,7 +1983,56 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { options?.httpClientLayer ?? ctx.httpClientLayer, ); - return yield* discoverToolsFromInput(connector).pipe( + // Take the invocation pool's lease when this connection is poolable, so + // the probe REUSES the session or child process that tool calls already + // hold. Dialling a second connection made the probe the author of its + // own failure on a single-instance local server: Chrome DevTools MCP + // owns a browser and a debug port, Playwright MCP the same, `docker run + // -i` a container, so the second child could not start and the liveness + // check reported a connection broken while the server was up and + // serving. The UI re-probes every non-healthy verdict on every mount, + // so each page load started one more child. The key is built exactly as + // the invoke path builds it, which is what makes the lease hit the same + // entry. + const poolKey = isPoolableConnectorInput(connectorInput) + ? yield* connectionPoolKey( + connectorInput, + String(credential.template), + credential.values, + { + owner: String(credential.owner), + connection: String(credential.connection), + }, + ) + : undefined; + const discovery: Effect.Effect = + poolKey === undefined + ? Effect.asVoid(discoverToolsFromInput(connectorInput)) + : // The whole LEASE is bounded: the pool dials through its own + // acquire with no deadline, and a server that never completes its + // handshake would otherwise hang this probe where a dialling one + // timed out at fifteen seconds. + withDiscoveryTimeout( + connectionPool.withConnection( + poolKey, + createMcpConnector(connectorInput), + (connection) => discoverToolsFromConnection(connection), + ), + Duration.toMillis(DEFAULT_DISCOVER_TIMEOUT), + ).pipe( + Effect.asVoid, + // The pool dials, so the raw connector failures surface here + // instead of inside `discoverTools`. Map them through the same + // function that path uses, so a pooled probe classifies a 401, + // a 403, and a connect timeout exactly as a dialling one does. + Effect.mapError((error) => + Predicate.isTagged(error, "McpToolDiscoveryError") + ? error + : connectionFailureToDiscoveryError(error), + ), + ); + + return yield* discovery.pipe( Effect.map( () => ({ status: "healthy" as const, checkedAt: Date.now() }) satisfies HealthCheckResult, diff --git a/packages/plugins/openapi/src/sdk/backing.ts b/packages/plugins/openapi/src/sdk/backing.ts index 82cd6dcdba..d336556afc 100644 --- a/packages/plugins/openapi/src/sdk/backing.ts +++ b/packages/plugins/openapi/src/sdk/backing.ts @@ -1013,8 +1013,14 @@ export const checkHealthOpenApi = (input: { } // Body-aware: a configuration 403 (Google accessNotConfigured / - // SERVICE_DISABLED) reads misconfigured, not expired. - const status = classifyProbeResponse(probe.result.status, probe.result.error); + // SERVICE_DISABLED) reads misconfigured, and a scope shortfall reads + // degraded, not expired — both authenticated, and neither is fixed by a + // reconnect. + const status = classifyProbeResponse( + probe.result.status, + probe.result.error, + probe.result.headers, + ); const rawIdentity = status === "healthy" ? extractIdentity(probe.result.data, spec.identityField) : undefined; // The identity is read straight off the raw body, so unlike the sample it diff --git a/plans/oauth-refresh-and-expired-status.md b/plans/oauth-refresh-and-expired-status.md index e62270d29e..9c73c3e974 100644 --- a/plans/oauth-refresh-and-expired-status.md +++ b/plans/oauth-refresh-and-expired-status.md @@ -1,8 +1,8 @@ # The wrong Expired status: analysis and plan -Status: the analysis is complete and the plan is proposed. The diagnosis branch -adds tests and this document only. The fix branch -(`fix/oauth-refresh-evidence`) lands Phase 1 items 1 to 3 and Phase 3 item 1. +Status: the analysis is complete. Causes R1, R2 in part, R3, R4, R5, R6, and +R8 are fixed on this branch. Open: R7, the Phase 2 database lease, the strike +counter, and the non-JSON 2xx case. ## The problem @@ -255,9 +255,10 @@ 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. -R1, R2, and R3 are fixed on `fix/oauth-refresh-evidence`. That branch replaces -each pair with one test that asserts the required behavior, so the file is now -regression coverage. R8 still ships its skipped test. +R1, R2, R3, and R8 are fixed on this branch. Each pair of tests became one +test that asserts the required behavior, so both files are now regression +coverage. R5 and R6 have new tests of their own. R4 changed two pinned +expectations, and Phase 4 records them. ### The OAuth and health tests @@ -303,11 +304,10 @@ cd packages/plugins/mcp && npx vitest run src/sdk/mcp-liveness-second-spawn.test # 1 passed | 1 skipped ``` -- **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". +- **R8.** The fixture refuses to start while a live process holds its lock, so + two probes pass only when the second one reuses the child the first one + started. The test asserts one child for two probes, and a healthy verdict for + both. ### Quality gates for the new files @@ -452,6 +452,71 @@ it. Item 1 landed on `fix/oauth-refresh-evidence`. Items 2 to 5 are open. +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. + + **Landed.** The probe builds its credential through one local `probe` + function, runs it, and on an `expired` answer for an OAuth connection it + calls `forceRefreshConnectionValues` and runs the probe one more time. A + refused refresh keeps the first verdict. + +2. **Detect a scope shortfall in a 403.** Run `detectInsufficientScope` in the + probe classification and report `degraded` instead of red **Expired**. Feed + the existing `missingOAuthScopes` mechanism and the "Reconnect to grant + access" interface. + + **Landed, with one difference.** `classifyProbeResponse` takes the response + headers as an optional third argument, so an RFC 6750 challenge is recognised + as well as a body. The result is `degraded` with the existing + `upstream_status` reason: `HealthCheckReason` is a closed set persisted + inside `last_health`, and its own comment requires a new literal to ship in a + separate deploy, readers first. A new `insufficient_scope` literal stays + open. + +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`. + + **Landed.** Prose is consulted only when `error.reason` is not `"network"`. + An HTTP 401 or 403 still classifies on its status. + `healthFromIntrospectionError` is exported for tests, and + `health-classification.test.ts` pins both halves: `connect EACCES: +permission denied` is not `expired`, and an upstream that names an + authentication failure still is. + +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. + + **Landed, in part.** The probe takes the pool lease, built from the same + identity the invoke path uses, so it reuses the session or child that tool + calls hold. `discoverToolsFromConnection` is the listing half of discovery + with no teardown, and `connectionFailureToDiscoveryError` maps the pooled + dial failures through the classification the dialling path uses. An + interrupted probe still releases its lease, and the pool closes a connection + its lease failed on, so `#1631` holds. Two parts stay open: a neutral + classification for the case where a process OUTSIDE executor holds the + resource, and a minimum interval for the non-healthy revalidation in + `use-connection-health.ts`, which still sends no `ifStaleMs`. + +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) + +Item 1 landed on `fix/oauth-refresh-evidence`. Items 2 to 5 are open. + 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 @@ -501,13 +566,32 @@ Item 1 landed on `fix/oauth-refresh-evidence`. Items 2 to 5 are open. 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. + + **Landed.** The mint records it, `persistRefreshedToken` falls back to it, + and a refresh that reports a new lifetime updates it. That update merges into + a freshly read `provider_state`, so it cannot bury a concurrent dead-grant + record under the copy the refresh started from. Two pinned expectations + changed with it: `oauth-flow.test.ts` asserted an exact `provider_state` + object and a null one, and both now carry the lifetime. + +2. **Require evidence for `healthy`.** Also let plugins that need no spec probe + without one, for example MCP tool discovery. Fewer connections then stay + unverified. + + **Landed, in the narrower form.** The probe is asked first, with or without a + spec. A plugin that can answer without one — MCP lists tools — gives a real + verdict, which the old branch never reached. Only a plugin that answers + `unknown` falls back to the credential-only verdict, and that verdict is + computed from the values the probe already resolved, so nothing refreshes + twice. The fallback also reports `expired` when a credential value resolved + to null, which the plugins and heal-on-use already did. + + The wider proposal — replacing the fallback's `healthy` with `unknown` — is + NOT implemented. It turns a large class of connections from green to grey, + which is a product decision rather than a defect fix. The + `google-health-checks` scenario still passes unchanged, because the OpenAPI + plugin declines without a spec and the fallback still produces that detail. + 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. @@ -565,19 +649,23 @@ Item 1 landed on `fix/oauth-refresh-evidence`. Items 2 to 5 are open. ## 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 +This branch ships Phase 0, Phase 1 items 1 to 3, Phase 3 items 1 to 4, and +Phase 4 items 1 and 2 as ONE pull request: the causes share code paths, and +each fix on its own leaves a wrong status reachable. The remaining work keeps +these boundaries: + +1. 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. +2. Phase 1 item 3 remainder: the strike counter, and the structural "the body + was JSON" flag on `OAuth2Error` that the 2xx case needs. +3. Phase 3 item 4 remainder: a neutral classification when a process outside + executor holds a single-instance resource, and a minimum interval for the + non-healthy revalidation. +4. Phase 4 item 2 remainder and item 3: the evidence-tagged `healthy`, which is + a product decision, and the interface for `unknown`. +5. Phase 5: the retry action, the message split, the skew, the optional + background refresh, and the alerts. 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 From db320815b5d74d1a6e5b04a29ba3bc270ae0bf66 Mon Sep 17 00:00:00 2001 From: spa5k <79936503+spa5k@users.noreply.github.com> Date: Thu, 17 Sep 2026 22:24:01 +0530 Subject: [PATCH 5/6] Hoist the MCP resource frame decoder to module scope The CI oxlint build flags the inline Schema.decodeUnknownOption in the test server's /mcp handler: the compiled decoder was rebuilt on every request. Move it beside the file's other module-level decoders. --- packages/core/sdk/src/testing/oauth-test-server.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/core/sdk/src/testing/oauth-test-server.ts b/packages/core/sdk/src/testing/oauth-test-server.ts index bd68d826c9..3887ae6985 100644 --- a/packages/core/sdk/src/testing/oauth-test-server.ts +++ b/packages/core/sdk/src/testing/oauth-test-server.ts @@ -512,6 +512,9 @@ const SUPPORTED_SUBJECT_TOKEN_TYPES = new Set([ const JwksUriMetadata = Schema.Struct({ jwks_uri: Schema.String }); const decodeJwksUriMetadata = Schema.decodeUnknownOption(JwksUriMetadata); +/** One JSON-RPC frame off the `/mcp` resource endpoint's body. */ +const decodeMcpResourceFrame = Schema.decodeUnknownOption(Schema.fromJsonString(Schema.Unknown)); + /** Resolve a trusted IdP's signing keys the way a Resource Authorization Server * does: read its RFC 8414 metadata, follow `jwks_uri`, fetch the key set. Any * failure yields `None`, which the caller reports as `invalid_grant` — the @@ -1083,8 +1086,7 @@ export const serveOAuthTestServer = ( // endpoint timed out at the discovery deadline. Answer the request's // OWN id, answer `tools/list` with an empty catalog, and stay silent // for notifications, which is the protocol. - const decodeMcpFrame = Schema.decodeUnknownOption(Schema.fromJsonString(Schema.Unknown)); - const frame = Option.getOrUndefined(decodeMcpFrame(body)); + const frame = Option.getOrUndefined(decodeMcpResourceFrame(body)); const frameIsRecord = frame !== null && typeof frame === "object"; const frameRecord = frameIsRecord ? (frame as Record) : {}; const frameMethod = From af7ae91cd027fbe717a2a2ca89e44c791bbd1645 Mon Sep 17 00:00:00 2001 From: spa5k <79936503+spa5k@users.noreply.github.com> Date: Thu, 17 Sep 2026 23:43:32 +0530 Subject: [PATCH 6/6] Clean up for review: one changeset, no plan document, trimmed comments The plan document and the phase annotations were working notes for the diagnosis; the code and its tests now carry the reasoning. The four changesets become one that covers all four packages. The regression test file is renamed to what it now is (oauth-refresh-evidence.test.ts) and its header describes the invariants instead of the history of this branch. The long narrative comments keep their rationale and drop the storytelling. --- .changeset/graphql-network-prose.md | 5 - .changeset/mcp-liveness-pooled-probe.md | 5 - .changeset/oauth-refresh-evidence.md | 19 +- .changeset/probe-scope-shortfall.md | 5 - packages/core/sdk/src/executor.ts | 139 ++-- packages/core/sdk/src/health-check.ts | 22 +- ...test.ts => oauth-refresh-evidence.test.ts} | 49 +- packages/plugins/mcp/src/sdk/discover.ts | 38 +- .../src/sdk/mcp-liveness-second-spawn.test.ts | 26 +- packages/plugins/mcp/src/sdk/plugin.ts | 27 +- plans/oauth-refresh-and-expired-status.md | 672 ------------------ 11 files changed, 111 insertions(+), 896 deletions(-) delete mode 100644 .changeset/graphql-network-prose.md delete mode 100644 .changeset/mcp-liveness-pooled-probe.md delete mode 100644 .changeset/probe-scope-shortfall.md rename packages/core/sdk/src/{oauth-expired-status-repro.test.ts => oauth-refresh-evidence.test.ts} (92%) delete mode 100644 plans/oauth-refresh-and-expired-status.md diff --git a/.changeset/graphql-network-prose.md b/.changeset/graphql-network-prose.md deleted file mode 100644 index d018b46762..0000000000 --- a/.changeset/graphql-network-prose.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@executor-js/plugin-graphql": patch ---- - -A GraphQL liveness probe no longer reads a transport failure's prose as a dead credential. Classification matched any upstream message containing "permission", which an operating-system refusal also carries — `connect EACCES: permission denied` on a socket reported the connection as `expired` and asked the user to re-enter a secret that was never the problem. Prose is now consulted only when the failure is not a transport failure; an HTTP 401 or 403 still classifies on its status. diff --git a/.changeset/mcp-liveness-pooled-probe.md b/.changeset/mcp-liveness-pooled-probe.md deleted file mode 100644 index d2d612111a..0000000000 --- a/.changeset/mcp-liveness-pooled-probe.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@executor-js/plugin-mcp": patch ---- - -The MCP liveness probe now takes the invocation pool's connection instead of dialling a second one. A probe of a local stdio server previously started a second child process, and the common local servers permit one instance only — Chrome DevTools MCP owns a browser and a debug port, Playwright MCP the same, `docker run -i` a container. The second child could not start, so the health check reported the connection broken while the server was up and serving tool calls. Because the UI re-probes every non-healthy verdict on every mount, each page load started one more child. A probe now reuses the pooled session or child, and an interrupted probe still tears down whatever it acquired. diff --git a/.changeset/oauth-refresh-evidence.md b/.changeset/oauth-refresh-evidence.md index 3d9741f836..826bf98814 100644 --- a/.changeset/oauth-refresh-evidence.md +++ b/.changeset/oauth-refresh-evidence.md @@ -1,19 +1,18 @@ --- "@executor-js/sdk": patch +"@executor-js/plugin-mcp": patch +"@executor-js/plugin-openapi": patch +"@executor-js/plugin-graphql": patch --- -Require evidence before a connection is marked permanently expired, and let the health probe refresh before it answers `expired`. +Stop recording a permanent **Expired** verdict without evidence, and stop the refresh races that produced one. -A refresher whose grant is refused now reads the stored refresh token again. When a peer instance rotated that token while the request was in flight, the call adopts the access token the peer persisted and records no rejection. Previously the loser of a concurrent refresh wrote `oauthReauthRequiredAt` onto a connection that still held a valid rotated refresh token. Every surface then answered `expired` without probing, and no tool call could refresh it again: only a re-authorization recovered it. The record is also skipped when `expires_at` moved forward during the grant, which is the same peer success read from the row. +A refresher whose grant is refused now reads the stored refresh token again; when a peer instance rotated it during the request, the call adopts the access token that peer persisted and records nothing. Previously the loser of a concurrent refresh wrote `oauthReauthRequiredAt` onto a connection that still held a valid rotated refresh token, and every surface then answered `expired` without probing — no tool call could refresh it again, only a re-authorization recovered it. The record write is also skipped when `expires_at` moved forward during the grant, which is the same peer success read from the row. -`isPermanentTokenRejection` no longer reads 408, 425, or 429 as a definitive refusal. One rate-limited minute at a token endpoint therefore no longer ends a grant. Those statuses now behave like a 5xx response, and the next call retries. +`isPermanentTokenRejection` no longer reads 408, 425, or 429 as a definitive refusal, so one rate-limited minute at a token endpoint no longer ends a grant; those statuses behave like a 5xx and the next call retries. A refresh response that omits `expires_in` (RFC 6749 makes it optional) no longer erases `expires_at`: the mint records the advertised lifetime in `provider_state.oauthTokenLifetimeMs` and a refresh derives the expiry from it, instead of disabling proactive refresh for the rest of the connection's life. -`connections.checkHealth` re-mints the token once and probes again before it answers `expired` for an OAuth connection. A revoked token, an idle timeout shorter than the advertised lifetime, or a null `expires_at` therefore no longer shows a working connection as dead. +`connections.checkHealth` re-mints once and probes again before it answers `expired` for an OAuth connection, so a revoked token, an idle timeout shorter than the advertised lifetime, or a null `expires_at` no longer shows a working connection as dead until a tool call heals it. The plugin is asked first with or without a declared health-check spec, so a plugin whose probe needs no spec (MCP lists tools) gives its OAuth connections a real verdict; only a plugin that answers `unknown` falls back to the credential-only verdict, and that verdict now reports `expired` when a credential value resolves to nothing. -A connection whose integration declares no probe operation is now asked of the plugin first. A plugin that can answer without a spec — MCP lists its tools — gives a real verdict for its OAuth connections, which the credential-only branch never reached. Only when the plugin itself answers `unknown` does the credential-only verdict replace it, and that verdict is now computed from the values the probe already resolved, so nothing refreshes twice. A credential that resolves to nothing reads as `expired` there too, matching the plugins and heal-on-use. +The MCP liveness probe takes the invocation pool's lease instead of dialling a second connection, bounded by the shared 15s discovery deadline; a probe of a stdio server no longer starts a second child process, which single-instance servers (Chrome DevTools MCP, Playwright MCP, `docker run -i`) refused — reporting a live, serving connection as broken on every page mount. A 403 scope shortfall on a probe reads `degraded` instead of `expired`, from either an RFC 6750 `WWW-Authenticate` challenge or a body marker. The GraphQL probe no longer reads a transport failure's prose as a dead credential (`connect EACCES: permission denied` on a socket is not an authentication verdict). -A refresh response that omits `expires_in` no longer erases `expires_at`. RFC 6749 makes the field optional, so an authorization server that advertised a lifetime on the code exchange and omitted it on refresh used to disable proactive refresh for the rest of the connection's life. The mint now records the advertised lifetime in `provider_state.oauthTokenLifetimeMs`, and a refresh without `expires_in` derives the expiry from it. - -A 403 scope shortfall on a probe now reads as `degraded` rather than `expired`: the credential authenticated, the grant is too narrow, and the remedy is a new consent rather than a reconnect. - -The test authorization server's MCP resource endpoint (`serveOAuthTestServer` at `/mcp`) now speaks the JSON-RPC protocol honestly: it answers the request's own id, answers `tools/list` with an empty catalog, and stays silent for notifications. The previous canned reply used a fixed id, so any client that completed the handshake waited forever for its `tools/list` response and every catalog sync or liveness probe against the endpoint timed out at the discovery deadline — a limitation invisible while OAuth health checks never dialled, and exposed once they do. +The test authorization server's `/mcp` resource endpoint now speaks JSON-RPC honestly — the request's own id, an empty catalog for `tools/list`, silence for notifications — where the old canned reply used a fixed id and left every completed handshake waiting forever for its `tools/list` response. diff --git a/.changeset/probe-scope-shortfall.md b/.changeset/probe-scope-shortfall.md deleted file mode 100644 index dfe0dae7b7..0000000000 --- a/.changeset/probe-scope-shortfall.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@executor-js/plugin-openapi": patch ---- - -A health probe that meets a 403 scope shortfall now reads as `degraded` rather than `expired`. The credential authenticated; the grant is narrower than the probe operation needs, and the remedy is a new consent with wider scope — which the connection's missing-scope affordance already offers. The old verdict told the user the connection was dead and sent them through a reconnect that could not widen the grant. Classification passes the response headers as well as the body, so an RFC 6750 `WWW-Authenticate: Bearer error="insufficient_scope"` challenge is recognised too. diff --git a/packages/core/sdk/src/executor.ts b/packages/core/sdk/src/executor.ts index 61934a0f5c..4af36d4e07 100644 --- a/packages/core/sdk/src/executor.ts +++ b/packages/core/sdk/src/executor.ts @@ -1027,12 +1027,9 @@ const decodeTokenLifetimeState = Schema.decodeUnknownOption( ); /** The access-token lifetime this grant advertised, in ms, when it ever - * advertised one. RFC 6749 makes `expires_in` OPTIONAL: an authorization - * server that sends it on the code exchange may omit it on refresh, and - * writing a null `expires_at` from such a response erased the only input the - * proactive refresh has — permanently, for the rest of the connection's life. - * Remembering the lifetime lets the next refresh derive an expiry from the - * grant it already knows. */ + * advertised one. RFC 6749 makes `expires_in` optional: writing a null + * `expires_at` from a refresh that omits it erased the only input the + * proactive refresh has, permanently. */ const rememberedTokenLifetimeMs = (value: unknown): number | null => { const decoded = Option.getOrNull(decodeTokenLifetimeState(decodeJsonColumn(value))); return decoded === null || !Number.isFinite(decoded.oauthTokenLifetimeMs) @@ -2311,12 +2308,9 @@ export const createExecutor = { if (fresh === null) return record(row); @@ -2448,13 +2434,9 @@ export const createExecutor = adoptPeerRotatedToken(error), ), @@ -3022,8 +2992,7 @@ export const createExecutor = ): HealthCheckResult => Object.values(values).some((value) => value == null) ? { @@ -5432,21 +5397,12 @@ export const createExecutor = spec === undefined && connectionRow.oauth_client != null && diff --git a/packages/core/sdk/src/health-check.ts b/packages/core/sdk/src/health-check.ts index 892c5408d7..088012d13c 100644 --- a/packages/core/sdk/src/health-check.ts +++ b/packages/core/sdk/src/health-check.ts @@ -243,21 +243,13 @@ const errorReasonMarkers = (body: unknown): string[] => { return markers; }; -/** Classify a probe response from its status AND body. Everything is - * `classifyHttpStatus` except two carve-outs on a 403: - * - * - A known configuration reason (Google `accessNotConfigured` / - * `SERVICE_DISABLED`) is `misconfigured`: the credential authenticated, the - * upstream API is disabled in the OAuth client's project, and only enabling - * it there (not reconnecting) fixes it. - * - A scope shortfall (RFC 6750 `insufficient_scope` in `WWW-Authenticate`, - * `error: insufficient_scope` in the body, Google's - * `ACCESS_TOKEN_SCOPE_INSUFFICIENT`) is `degraded`: the credential - * authenticated too, and the remedy is a NEW CONSENT with wider scope — - * which the connection's `missingOAuthScopes` already offers — not a - * reconnect. Reporting it as `expired` told the user the connection was dead - * and sent them through a flow that could not fix it. `headers` is optional - * so a caller that only kept the body still gets the body-based detection. */ +/** Classify a probe response from its status, body, and (optionally) headers. + * Everything is `classifyHttpStatus` except two 403 carve-outs, both of which + * authenticated: a known configuration reason (Google `accessNotConfigured` / + * `SERVICE_DISABLED`) is `misconfigured`, and a scope shortfall (RFC 6750 + * `insufficient_scope`) is `degraded` — the remedy is a new consent, not a + * reconnect, so `expired` would send the user through a flow that cannot fix + * it. */ export const classifyProbeResponse = ( status: number, body: unknown, diff --git a/packages/core/sdk/src/oauth-expired-status-repro.test.ts b/packages/core/sdk/src/oauth-refresh-evidence.test.ts similarity index 92% rename from packages/core/sdk/src/oauth-expired-status-repro.test.ts rename to packages/core/sdk/src/oauth-refresh-evidence.test.ts index 76846f456a..bf27aa4644 100644 --- a/packages/core/sdk/src/oauth-expired-status-repro.test.ts +++ b/packages/core/sdk/src/oauth-refresh-evidence.test.ts @@ -1,25 +1,14 @@ -// Regression coverage for the three causes of a wrong **Expired** status that -// plans/oauth-refresh-and-expired-status.md ranks R1, R2, and R3. This file -// started as the reproduction harness for that analysis: each cause had a test -// that pinned the behavior on `main` and a skipped test that gave the required -// behavior. The fix landed, so each cause now has one test, and it asserts the -// required behavior. +// Regression coverage for the connection status and OAuth refresh defects that +// produced a permanent, wrong **Expired**: a refresher that loses a rotation +// race, a rate-limited token endpoint, a probe that answered without +// refreshing, and a refresh response that omits `expires_in`. // -// R1: a refresher that loses a rotation race adopts the peer's token. It does -// not write the permanent rejection record. -// R2: one temporary 4xx response (a 429) does not end the grant. -// R3: the health probe refreshes before it answers `expired`. -// -// Deployment shape under test: ONE database, ONE credential store, TWO executor -// instances, and one root database handle for each instance. That is the cloud -// app (a per-request `DbService` rebuild plus per-session Durable Objects) and -// any multi-process self-hosting. It is the shape that the `refreshGateFor` -// documentation declares out of scope for the in-process gate. -// -// `oauth-flow.test.ts` already builds this shape in "a refresher paused after -// reading the stored token never writes it back over a peer's rotated one". -// That test examines the credential store. These tests examine the connection -// row, which is where the wrong status was written. +// One database, one credential store, two executor instances, one root database +// handle each. The in-flight refresh gate is keyed on the handle, so two +// instances do not share a gate — the cloud app's per-request `DbService` +// rebuild and any multi-process self-host both have this shape. The tests +// assert on the connection ROW, which is where the wrong status was written: +// `oauth-flow.test.ts` already covers this shape for the credential store. import { createServer, type IncomingMessage, type Server, type ServerResponse } from "node:http"; @@ -291,7 +280,7 @@ const deadGrantStamp = (row: unknown): number | undefined => { }; // --------------------------------------------------------------------------- -// R1 — a refresher that loses the rotation race adopts the peer's token. +// A refresher that loses the rotation race adopts the peer's token. // --------------------------------------------------------------------------- /** Run the race: A reads the stored refresh token and stalls, B wins and @@ -330,7 +319,7 @@ const runRotationRace = (race: Race) => }; }); -describe("R1 — a lost rotation race is not a dead grant", () => { +describe("a lost rotation race is not a dead grant", () => { it.effect("the loser adopts the peer's token and the connection keeps refreshing", () => withRace({}, (race) => Effect.gen(function* () { @@ -362,7 +351,7 @@ describe("R1 — a lost rotation race is not a dead grant", () => { }); // --------------------------------------------------------------------------- -// R2 — one temporary 4xx does not end a grant. +// One temporary 4xx response does not end a grant. // --------------------------------------------------------------------------- interface FlakyEndpoint { @@ -461,7 +450,7 @@ const withRateLimitedRefresh = ( }), ); -describe("R2 — a rate-limited refresh stays retryable", () => { +describe("a rate-limited refresh stays retryable", () => { it.effect("a 429 from the token endpoint does not end the grant", () => withRateLimitedRefresh(({ race, flaky }) => Effect.gen(function* () { @@ -484,7 +473,7 @@ describe("R2 — a rate-limited refresh stays retryable", () => { }); // --------------------------------------------------------------------------- -// R5 — a refresh response without `expires_in` keeps the advertised lifetime. +// A refresh response without `expires_in` keeps the advertised lifetime. // --------------------------------------------------------------------------- interface StrippingEndpoint { @@ -548,7 +537,7 @@ const rowExpiresAt = (row: unknown): number | null => { }; // --------------------------------------------------------------------------- -// R3 — the probe refreshes before it answers expired. +// The probe refreshes before it answers expired. // --------------------------------------------------------------------------- /** Connect with a declared health check, then revoke the live access token @@ -562,7 +551,7 @@ const withRevokedToken = (use: (race: Race) => Effect.Effect) => }), ); -describe("R3 — the probe refreshes before it answers expired", () => { +describe("the probe refreshes before it answers expired", () => { it.effect("a revoked token that the refresh can replace probes healthy", () => withRevokedToken((race) => Effect.gen(function* () { @@ -601,10 +590,10 @@ describe("R3 — the probe refreshes before it answers expired", () => { }); // --------------------------------------------------------------------------- -// R5 — a refresh response without `expires_in` must not erase the expiry. +// A refresh response without `expires_in` must not erase the expiry. // --------------------------------------------------------------------------- -describe("R5 — a refresh response that omits expires_in", () => { +describe("a refresh response that omits expires_in", () => { it.effect("keeps the advertised lifetime, so proactive refresh survives", () => Effect.scoped( Effect.gen(function* () { diff --git a/packages/plugins/mcp/src/sdk/discover.ts b/packages/plugins/mcp/src/sdk/discover.ts index 900a5a5f0b..19756bf37f 100644 --- a/packages/plugins/mcp/src/sdk/discover.ts +++ b/packages/plugins/mcp/src/sdk/discover.ts @@ -166,16 +166,10 @@ export const discoverToolsFromInput = ( ); /** Turn a connection failure into the discovery failure every caller of this - * module handles. Exported because a caller that takes its connection from the - * invocation pool meets the raw connector errors itself: the pool dials, this - * module only lists. Keeping the mapping here is what makes a pooled liveness - * probe classify a 401, a 403, and a connect timeout exactly as a dialling one - * does. - * - * Preserves the handshake HTTP status (401/403 = auth wall) and a - * connect-level timeout so the liveness health check can classify - * structurally — dropping `failureKind: "timeout"` here is what made a timed-out - * handshake read as a generic probe failure. */ + * module handles. A caller that takes its connection from the invocation pool + * meets the raw connector errors itself (the pool dials, this module only + * lists), so the mapping is shared. Preserves the handshake HTTP status and a + * connect-level timeout, which the liveness health check classifies on. */ export const connectionFailureToDiscoveryError = ( failure: McpConnectionError | McpOAuthReauthorizationRequired, ): McpToolDiscoveryError => { @@ -194,15 +188,11 @@ export const connectionFailureToDiscoveryError = ( }); }; -/** Bound a discovery step with the shared deadline and the shared timeout error. - * - * One definition so every path answers a wedged server identically — and every - * path is bounded. The pool's `withConnection` dials through its own acquire - * with no deadline of its own, so a caller that takes its connection from the - * pool must wrap the WHOLE lease in this deadline: without it, a server that - * never completes its handshake hangs the health check that used to time out - * at fifteen seconds. On timeout the lease releases (the pool closes a - * connection its lease failed on), so no child or session is left behind. */ +/** Bound a discovery step with the shared deadline and the shared timeout + * error, so every path answers a wedged server identically — and every path is + * bounded: the pool's own dial has no deadline, so a pooled caller must wrap + * the whole lease. On timeout the lease releases and the pool closes the + * connection, so nothing is left behind. */ export const withDiscoveryTimeout = ( effect: Effect.Effect, timeoutMs: number, @@ -221,13 +211,9 @@ export const withDiscoveryTimeout = ( }), ); -/** The listing half of discovery, over a connection the CALLER owns. - * - * `discoverTools` dials, lists, and closes. A caller that already holds an - * open connection must not close it: the liveness health check takes a lease - * from the invocation pool, and tool calls still need that session afterwards. - * This is the same listing work — same deadline, same elicitation refusal — - * with no teardown. */ +/** The listing half of discovery, over a connection the caller owns. + * `discoverTools` dials, lists, and closes; a caller holding a pool lease must + * not close it. Same listing work and deadline, no teardown. */ export const discoverToolsFromConnection = ( connection: McpConnection, timeoutMs: number = Duration.toMillis(DEFAULT_DISCOVER_TIMEOUT), 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 index 1f88c11650..5ff0744ee4 100644 --- a/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts +++ b/packages/plugins/mcp/src/sdk/mcp-liveness-second-spawn.test.ts @@ -1,24 +1,14 @@ -// --------------------------------------------------------------------------- -// A liveness probe must not dial a second connection when the invocation pool -// already holds one. +// A liveness probe must reuse the invocation pool's connection instead of +// dialling a second one. For a stdio server a fresh dial starts a second child +// process, and the common local servers permit one instance only (Chrome +// DevTools MCP, Playwright MCP, `docker run -i`): the second child cannot +// start, so the probe reported a live, serving server as broken — once per page +// mount, because the UI re-probes every non-healthy verdict. // -// `checkHealth` used to call `discoverToolsFromInput`, which builds a FRESH -// connector (`discover.ts` → `createMcpConnector`) instead of taking the pooled -// connection that tool calls 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 starts a SECOND CHILD PROCESS — and the common local -// servers permit one instance only: Chrome DevTools MCP owns a browser and a -// debug port, Playwright MCP the same, `docker run -i` a container. The second -// child could not start, so the probe reported the connection broken while the -// server was up and serving the pooled client. The UI re-probes every -// non-healthy verdict on every mount, so each page load started one more child. -// -// The fixture makes that failure deterministic: it refuses to start while a -// live process holds its lock. Two probes therefore pass only if the second one -// reuses the first one's child. +// The fixture refuses to start while a live process holds its lock, so two +// probes pass only when the second reuses the first one's child. // // `it.live`: this measures real child processes, so it needs the wall clock. -// --------------------------------------------------------------------------- import { existsSync, mkdtempSync, readFileSync } from "node:fs"; import { tmpdir } from "node:os"; diff --git a/packages/plugins/mcp/src/sdk/plugin.ts b/packages/plugins/mcp/src/sdk/plugin.ts index a3f6af8a25..cf7dd12303 100644 --- a/packages/plugins/mcp/src/sdk/plugin.ts +++ b/packages/plugins/mcp/src/sdk/plugin.ts @@ -1984,16 +1984,12 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { ); // Take the invocation pool's lease when this connection is poolable, so - // the probe REUSES the session or child process that tool calls already - // hold. Dialling a second connection made the probe the author of its - // own failure on a single-instance local server: Chrome DevTools MCP - // owns a browser and a debug port, Playwright MCP the same, `docker run - // -i` a container, so the second child could not start and the liveness - // check reported a connection broken while the server was up and - // serving. The UI re-probes every non-healthy verdict on every mount, - // so each page load started one more child. The key is built exactly as - // the invoke path builds it, which is what makes the lease hit the same - // entry. + // the probe reuses the session or child process tool calls already hold. + // A fresh dial starts a second child on a stdio server, and the common + // local servers permit one instance only (Chrome DevTools MCP, + // Playwright MCP, `docker run -i`) — the probe then failed a live, + // serving server, once per page mount. The key matches the invoke + // path's, which is what makes the lease hit the same entry. const poolKey = isPoolableConnectorInput(connectorInput) ? yield* connectionPoolKey( connectorInput, @@ -2008,10 +2004,8 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { const discovery: Effect.Effect = poolKey === undefined ? Effect.asVoid(discoverToolsFromInput(connectorInput)) - : // The whole LEASE is bounded: the pool dials through its own - // acquire with no deadline, and a server that never completes its - // handshake would otherwise hang this probe where a dialling one - // timed out at fifteen seconds. + : // The whole lease is bounded: the pool's own dial has no + // deadline. withDiscoveryTimeout( connectionPool.withConnection( poolKey, @@ -2022,9 +2016,8 @@ export const mcpPlugin = definePlugin((options?: McpPluginOptions) => { ).pipe( Effect.asVoid, // The pool dials, so the raw connector failures surface here - // instead of inside `discoverTools`. Map them through the same - // function that path uses, so a pooled probe classifies a 401, - // a 403, and a connect timeout exactly as a dialling one does. + // instead of inside `discoverTools`; map them through the same + // classification that path uses. Effect.mapError((error) => Predicate.isTagged(error, "McpToolDiscoveryError") ? error diff --git a/plans/oauth-refresh-and-expired-status.md b/plans/oauth-refresh-and-expired-status.md deleted file mode 100644 index 9c73c3e974..0000000000 --- a/plans/oauth-refresh-and-expired-status.md +++ /dev/null @@ -1,672 +0,0 @@ -# The wrong Expired status: analysis and plan - -Status: the analysis is complete. Causes R1, R2 in part, R3, R4, R5, R6, and -R8 are fixed on this branch. Open: R7, the Phase 2 database lease, the strike -counter, and the non-JSON 2xx case. - -## The problem - -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. - -All citations in this document refer to `main` at commit `a72e51d13`. - -## Terms - -This document uses one term for one concept. - -- **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. 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. - ---- - -## 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; -isPermanentTokenRejection = (e) => - isUnusableSuccessTokenResponse(e) || (e.status >= 400 && e.status < 500); -``` - -`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. - ---- - -## 3. Replication - -Four causes have executable tests. Each cause has two tests on the diagnosis -branch. 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. - -R1, R2, R3, and R8 are fixed on this branch. Each pair of tests became one -test that asserts the required behavior, so both files are now regression -coverage. R5 and R6 have new tests of their own. R4 changed two pinned -expectations, and Phase 4 records them. - -### 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 skipped tests are the fix targets) -# Remove one skip to see that test fail on main. -``` - -- **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.** The fixture refuses to start while a live process holds its lock, so - two probes pass only when the second one reuses the child the first one - started. The test asserts one child for two probes, and a healthy verdict for - both. - -### Quality gates for the new files - -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 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. - ---- - -## 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. Items 1 and 2 landed on -`fix/oauth-refresh-evidence`. Item 3 landed in the narrow form below: the -transient statuses are excluded, and the strike counter is deferred. - -1. **Detect the rotation before the system writes the record.** In - `performTokenRefresh`, read 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. - - **Landed.** `adoptPeerRotatedToken` runs between the classification and the - record write, so the record is only considered after adoption failed. The - span attribute is `executor.oauth.refresh.peer_rotation_adopted=true`, and - `executor.oauth.refresh.outcome` stays `ok`, because the call did succeed. - The caller skips `persistRefreshedToken` for an adopted token: the peer - persisted it already, and persisting from a token response this instance - never received would erase the expiry the peer wrote. - -2. **Guard the record write against a peer's success.** Read the row again - before `markRefreshGrantDead` writes. When `expires_at` moved forward, or - went from null to a value, a peer refreshed this grant successfully while - our request was in flight. Skip the record then. Only a mint or a refresh - writes `expires_at`, so this signal does not fire for an unrelated write - such as a tool sync. Write against the fresh row, so the merge base is the - current `provider_state`. - - **Landed in this form.** The plan first proposed a new - `connection.refresh_token_fp` column with a compare-and-set on it. That - needs a schema migration in four hosts, and the re-read of the stored - refresh item in item 1 already gives the precise signal. The `expires_at` - guard is the second net for the case where adoption itself fails, for - example when the primary item is unreadable. Revisit the fingerprint column - only if Phase 2's lease needs a stable token identity. - -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. - - **Landed in part.** The transient statuses 408, 425, and 429 are excluded - and behave like a 5xx response. The strike counter is not implemented: it - needs the cooldown semantics decided first, and excluding the transient - statuses removes the case that motivated it. The non-JSON 2xx case is - unchanged, because `oauth-helpers.test.ts` pins a malformed JSON 200 as - definitive and the HTML-200 variant needs a structural "the body was JSON" - flag on `OAuth2Error`. Both remain open. - -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) - -Item 1 landed on `fix/oauth-refresh-evidence`. Items 2 to 5 are open. - -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. - - **Landed.** The probe builds its credential through one local `probe` - function, runs it, and on an `expired` answer for an OAuth connection it - calls `forceRefreshConnectionValues` and runs the probe one more time. A - refused refresh keeps the first verdict. - -2. **Detect a scope shortfall in a 403.** Run `detectInsufficientScope` in the - probe classification and report `degraded` instead of red **Expired**. Feed - the existing `missingOAuthScopes` mechanism and the "Reconnect to grant - access" interface. - - **Landed, with one difference.** `classifyProbeResponse` takes the response - headers as an optional third argument, so an RFC 6750 challenge is recognised - as well as a body. The result is `degraded` with the existing - `upstream_status` reason: `HealthCheckReason` is a closed set persisted - inside `last_health`, and its own comment requires a new literal to ship in a - separate deploy, readers first. A new `insufficient_scope` literal stays - open. - -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`. - - **Landed.** Prose is consulted only when `error.reason` is not `"network"`. - An HTTP 401 or 403 still classifies on its status. - `healthFromIntrospectionError` is exported for tests, and - `health-classification.test.ts` pins both halves: `connect EACCES: -permission denied` is not `expired`, and an upstream that names an - authentication failure still is. - -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. - - **Landed, in part.** The probe takes the pool lease, built from the same - identity the invoke path uses, so it reuses the session or child that tool - calls hold. `discoverToolsFromConnection` is the listing half of discovery - with no teardown, and `connectionFailureToDiscoveryError` maps the pooled - dial failures through the classification the dialling path uses. An - interrupted probe still releases its lease, and the pool closes a connection - its lease failed on, so `#1631` holds. Two parts stay open: a neutral - classification for the case where a process OUTSIDE executor holds the - resource, and a minimum interval for the non-healthy revalidation in - `use-connection-health.ts`, which still sends no `ifStaleMs`. - -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) - -Item 1 landed on `fix/oauth-refresh-evidence`. Items 2 to 5 are open. - -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. - - **Landed.** The probe builds its credential through one local `probe` - function, runs it, and on an `expired` answer for an OAuth connection it - calls `forceRefreshConnectionValues` and runs the probe one more time. A - refused refresh keeps the first verdict. - -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. - - **Landed.** The mint records it, `persistRefreshedToken` falls back to it, - and a refresh that reports a new lifetime updates it. That update merges into - a freshly read `provider_state`, so it cannot bury a concurrent dead-grant - record under the copy the refresh started from. Two pinned expectations - changed with it: `oauth-flow.test.ts` asserted an exact `provider_state` - object and a null one, and both now carry the lifetime. - -2. **Require evidence for `healthy`.** Also let plugins that need no spec probe - without one, for example MCP tool discovery. Fewer connections then stay - unverified. - - **Landed, in the narrower form.** The probe is asked first, with or without a - spec. A plugin that can answer without one — MCP lists tools — gives a real - verdict, which the old branch never reached. Only a plugin that answers - `unknown` falls back to the credential-only verdict, and that verdict is - computed from the values the probe already resolved, so nothing refreshes - twice. The fallback also reports `expired` when a credential value resolved - to null, which the plugins and heal-on-use already did. - - The wider proposal — replacing the fallback's `healthy` with `unknown` — is - NOT implemented. It turns a large class of connections from green to grey, - which is a product decision rather than a defect fix. The - `google-health-checks` scenario still passes unchanged, because the OpenAPI - plugin declines without a spec and the fallback still produces that detail. - -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. - ---- - -## 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 - -This branch ships Phase 0, Phase 1 items 1 to 3, Phase 3 items 1 to 4, and -Phase 4 items 1 and 2 as ONE pull request: the causes share code paths, and -each fix on its own leaves a wrong status reachable. The remaining work keeps -these boundaries: - -1. 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. -2. Phase 1 item 3 remainder: the strike counter, and the structural "the body - was JSON" flag on `OAuth2Error` that the 2xx case needs. -3. Phase 3 item 4 remainder: a neutral classification when a process outside - executor holds a single-instance resource, and a minimum interval for the - non-healthy revalidation. -4. Phase 4 item 2 remainder and item 3: the evidence-tagged `healthy`, which is - a product decision, and the interface for `unknown`. -5. Phase 5: the retry action, the message split, the skew, the optional - background refresh, and the alerts. - -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.