Conversation
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.
Closed
8 tasks
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.
7 tasks
Owner
Author
|
Superseded by UsefulSoftwareCo#2037, which carries all commits (diagnosis + fixes) in one pull request. |
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
This pull request fixes three causes of a wrong Expired status in
@executor-js/sdk. It is the first fix in the stack. The analysis and the reproductions are in the parent pull request.The work is Phase 1 items 1 to 3 and Phase 3 item 1 of
plans/oauth-refresh-and-expired-status.md. The plan document in this branch records what landed and what remains open.Stack
This pull request is open in the fork, because a stacked pull request needs its
base branch in the same repository, and the base branch exists only in the
fork. Retarget it to
UsefulSoftwareCo/executor:mainafter UsefulSoftwareCo#2036 merges, ormerge it into the fork branch and open one upstream pull request for both.
The four changes
1. A refresher that loses a rotation race no longer ends the connection.
The in-flight refresh gate is keyed on the database handle. Two instances can therefore redeem one single-use refresh token. The loser received
invalid_grantand wrote the permanent rejection recordoauthReauthRequiredAt. The winner's valid rotated token stayed in the store, unused. Every surface then answeredexpiredwithout a probe, and no tool call could refresh again.The refusal now reads the stored refresh item again before it records anything. A value that changed during the request is a peer's rotation. The call then adopts the access token that the peer persisted and records nothing. Adoption runs before the record write. An adopted token is never persisted again, because that would erase the expiry the peer wrote.
2. The record write examines the row again.
An
expires_atthat moved forward during the grant is the same peer success, read from the row. The write is then 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. The write also merges into the freshprovider_state, not the one this grant read before it went to the network.3.
isPermanentTokenRejectionno longer reads 408, 425, or 429 as definitive.One rate-limited minute at a token endpoint ended a grant permanently. Those three statuses now behave like a 5xx response: the failure stays retryable and the next call tries again.
4.
connections.checkHealthrefreshes before it answersexpired.The probe answered from the credential it was handed. A revoked token, an idle timeout shorter than the advertised lifetime, or a null
expires_attherefore showed a working connection as dead. The status changed to healthy only when the user called a tool. The probe now forces one refresh and probes one more time for an OAuth connection. A refused refresh keeps the first verdict, so the persisted-expiredcontract for a dead grant does not change.Tests
The parent branch had two tests for each cause: one that pinned the old behavior, and one skipped test that gave the required behavior. This branch replaces each pair with one test that asserts the required behavior. The three skipped tests are gone, and their assertions pass.
oauth-helpers.test.tsalso pins the new classification: 408, 425, and 429 stay transient, and a text/plain 400 or 404 stays definitive.What this pull request does not change
expiredon every read until a reconnect.OAuth2Error.Linked issue
None. The parent pull request proposes one issue per phase.
Verification
bun run format:check— all files have the correct format.bun run lint— 0 warnings and 0 errors in 1869 files. The changelog stub check passes.bun run typecheck— 45 of 45 tasks pass.bun run test— 39 of 39 tasks pass withTURBO_TEST_CONCURRENCY=4. Two packages failed under full parallel load and pass alone:host-cloudflarestarts a fixed-port PGlite server, andruntime-quickjspassed on its own run. Neither package touches this code.e2e — selfhost target, 10 scenario files, 34 tests, all pass:
These are the scenarios that pin the contracts this change touches: a refused
refresh still presents
expiredand is persisted (oauth-refresh-rejected,oauth-refresh-rejected-non-json,connection-health-verdict), the reactive401 retry still re-mints once (
oauth-refresh-on-401), one refresh perconnection across stacks (
oauth-refresh-cross-session), the freshness gatestill serves a cached verdict and probes once
(
health-checks-ifstalems-...), and the reconnect journey still refreshes theverdict without a page reload (
mcp-oauth-reconnect-health).Per-scenario artifacts are on the machine that ran them, under
e2e/runs/selfhost/<scenario-slug>/(result.json,test.ts,traces.json;the browser scenario also records video and a trace). That directory is
gitignored.
One note on the environment: the first run of
connection-health-verdict.test.tsfailed in its browser half because thepinned Playwright headless shell was not installed.
npx playwright install chromiumfixed the environment, and the scenario then passed. The failure wasnot related to this change.
Run the changed tests:
Checklist
.changeset/oauth-refresh-evidence.mdfor@executor-js/sdk(patch).