Conversation
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.
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.
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.
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 UsefulSoftwareCo#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.
This was referenced Sep 17, 2026
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.
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.
spa5k
marked this pull request as ready for review
September 17, 2026 18:13
Author
|
@RhysSullivan Ready for review when you have a moment. I hit this in day-to-day use: connections showed an Expired status at random, and local MCP servers (Chrome DevTools MCP) showed as disconnected while their tools still worked. The PR fixes the causes I could reproduce — the cross-isolate refresh race, the permanent rejection written from one rate-limited response, and the probe that answered without refreshing. Two notes for review:
|
Author
2. Clicking linear and coming back shows it logged in.
Supabase MCP as well. |
Author
|
Have seen same things for the chrome devtools mcp. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Connections showed a wrong Expired status — sometimes false, sometimes permanent — and the token refresh behind it was uncoordinated. This fixes the four defects that produced it, across
@executor-js/sdkand the MCP, OpenAPI, and GraphQL plugins.What changes
A lost refresh race no longer kills a connection. The in-flight refresh gate is keyed on the database handle, so two instances can redeem one single-use refresh token. The loser received
invalid_grantand wrote the permanent rejection record while the winner's valid rotated token sat in the store unused — every surface then answeredexpiredwithout probing, and nothing but a re-authorization recovered it. The refusal now re-reads the stored refresh item: 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. The record write is also skipped whenexpires_atmoved forward during the grant.One rate-limited minute no longer ends a grant. 408, 425, and 429 are retryable now, like a 5xx.
The health probe refreshes before it answers
expired. A revoked token, an idle timeout, or a nullexpires_atused to show a working connection as dead until a tool call healed it. The probe now re-mints once and probes again for OAuth connections; a refused refresh keeps the probe's own verdict, so a dead grant still readsexpired.The plugin is asked first, with or without a spec. MCP's probe needs no spec, so its OAuth connections now get a real verdict. Only a plugin that answers
unknownfalls back to the credential-only verdict — computed from the values the probe already resolved, so nothing refreshes twice — and that fallback reportsexpiredwhen a credential value resolves to nothing.The MCP liveness probe reuses the pooled connection. It used to dial a second connection, which for a stdio server started a second child process that single-instance servers (Chrome DevTools MCP, Playwright MCP,
docker run -i) refuse — reporting a live, serving connection as broken on every page mount. The pooled lease is bounded by the same 15s discovery deadline, and an interrupted probe still releases.A scope shortfall reads
degraded, notexpired. Recognised from an RFC 6750WWW-Authenticatechallenge or a body marker; the remedy is a new consent, not a reconnect. The GraphQL probe no longer reads a transport failure's prose as a credential verdict (connect EACCES: permission deniedis not one).A refresh without
expires_inno longer erases the expiry. RFC 6749 makes the field optional; writing null disabled proactive refresh permanently. The mint records the advertised lifetime and a refresh derives from it.Also: the test OAuth server's
/mcpendpoint now speaks JSON-RPC properly (request's own id, empty catalog fortools/list, silence for notifications). The old canned reply used a fixed id, so every completed handshake waited forever fortools/list— invisible until health checks started dialling.What does not change
The permanent rejection gate stays: a genuinely rejected grant still stops refresh traffic and reads
expireduntil a reconnect. No newHealthCheckReasonliteral ships (the set is persisted and needs readers first). The credential-only fallback still reportshealthywhen nothing can probe — changing that turns a large class of connections grey and is a separate decision.Related issues
invalid_grantsurfaces and no verdict flips. The cross-isolate single-flight this issue proposes (the DO or database lease) is the remaining half and is not in this PR. Section 2's smaller item —oauth_scopekeeping the requested set when the provider never echoesscope— is also not addressed here.invalid_grant,lastHealthexpired, re-auth required. Executor does persist rotated refresh tokens (persistRefreshedTokenwrites the rotated token before the access token), so the reported failure matches the cross-isolate race above: the loser wrote the permanent rejection record while a valid rotated token sat in the store. This PR stops that from being written. An affected connection still needs one reconnect to clear an existing record, and the issue's third ask (surface re-auth earlier) is not addressed.Verification
bun run format:check,bun run lint,bun run typecheck(45/45) — clean.bun run test— 39/39 tasks (TURBO_TEST_CONCURRENCY=4).local/oauth-await-longpollandlocal/oauth-token-durabilityon the local target.host-cloudflare's Access test (5sunstable_devboot timeout; reproduced onmainin a clean worktree under the same load; passes 3/3 in isolation here) andmcp-session-cap-eviction(also failed the tests-and-docs-only Diagnose the wrong Expired status and the concurrent refresh defects #2036, which has a fully green run of the same diff).Checklist