Skip to content

[Plugin] Fix intermittent re-auth from cross-process refresh-token rotation - #44

Open
pragati-agrawal-glean wants to merge 7 commits into
mainfrom
pragati/fix-plugin-token-rotation-reauth
Open

[Plugin] Fix intermittent re-auth from cross-process refresh-token rotation#44
pragati-agrawal-glean wants to merge 7 commits into
mainfrom
pragati/fix-plugin-token-rotation-reauth

Conversation

@pragati-agrawal-glean

@pragati-agrawal-glean pragati-agrawal-glean commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Problem

The plugin intermittently asks the user to authenticate again — find_skills (and run_tool) return [SETUP_REQUIRED] — even though a valid grant exists. This is not a token-TTL change.

Root cause: per-session in-memory snapshot + refresh-token rotation

MCP servers are spawned per session, so several plugin processes can be alive at once. The OAuth provider reads credentials from disk once at construction and then serves everything from memory:

constructor() { const stored = loadCredentials(); this._tokens = stored?.tokens } // read ONCE
tokens()      { return this._tokens }                                            // never re-reads
saveTokens(t) { this._tokens = t; saveCredentials(t) }                           // last-writer-wins

Refresh is reactive: only on a 401 does the SDK call provider.tokens() (auth.js:272) and refresh with that token. Combined with Ory single-use refresh-token rotation (ory_rt_… are consumed on use; a refresh returns a new one and invalidates the old):

When process A refreshes, it persists a new refresh token to disk and invalidates the old one. Any other live process still holds the old, now-dead refresh token in memory. Its next 401 → refresh → invalid_grant → full re-auth → [SETUP_REQUIRED].

This also explains the observed anomaly of an older token being "last used" after a newer token was minted: a long-running session kept refreshing the token it snapshotted at startup, oblivious to the newer grant on disk.

When invalidation happens before the sibling's write reaches disk, the provider waits up to a 2-second grace window, polling every 100 ms, for the newer grant before clearing credentials. The window is configurable with GLEAN_ROTATION_GRACE_MS.

Fixes (both independent of the PLUGIN_DATA_DIR store split)

1. tokens() re-reads the store when it changed on disk (auth-provider.ts, token-store.ts)
mtime-guarded, so the steady state is a single stat(), not a re-parse. This sits exactly where the SDK reads tokens at the start of its auth flow, so a process picks up a sibling's freshly-rotated grant before attempting a refresh. Conservative on removal: a missing file or a tokens-less rewrite does not evict the in-memory token (a transient stat failure or another process's logout shouldn't drop a token that still works for us).

2. createRemoteClient retries connect once on a sibling refresh (remote-client.ts)
If auth fails and a newer access token has since appeared on disk, rebuild the transport and retry once — turning the rotation race into a silent reconnect. Bounded to a single retry so it can't spin.

Tests

  • tokens() adopts a sibling's newer token; keeps its token when the file vanishes; ignores a tokens-less rewrite.
  • credentialsMtimeMs probe (undefined when absent, advances on rewrite).
  • connect retry fires only when the on-disk token changed; otherwise surfaces AuthRequiredError.

Full suite: 204 passing, typecheck clean.

Manual two-session verification

We also verified the behavior manually using two Claude sessions running an SST MCP server:

  1. Temporarily changed the access-token expiry from 7 days to 2 minutes.
  2. With the pre-fix version, reproduced the error after the token expired: one session refreshed while the other retained the stale token and eventually failed with re-auth / [SETUP_REQUIRED].
  3. Repeated the same test with the changed version in both Claude sessions.
  4. After one session's token expired and it obtained a new rotated token, the other session continued successfully without re-authentication or [SETUP_REQUIRED].

E2E verification (2026-07-29, exp pod 117, real prod /oauth)

Deployed a scio test branch (pragati/test-exp-pod-short-token-age) that makes the exp pod reject access tokens older than 3 minutes (validation-side only — token issuance/refresh stayed on production fosite). Two plugin processes shared one credentials store; both warmed (in-memory snapshot), waited out the age gate, then raced.

Round Build Shape Result
1 main A then B A rotates R→R′ on prod (/oauth/token 200, refresh suffix changed on disk); B refreshes the dead grant → 400 → store wiped → [SETUP_REQUIRED] — the reported bug, on demand
2 this PR identical Both succeed; B's tokens() disk-sync picks up A's rotated grant — never even 401s
3 this PR concurrent B loses the tight race → 400 → grace-poll adopts A's grant → silent retry, succeeds ([auth] Refresh failed but a sibling refreshed — retrying with its token)

Two live findings folded into the final commit:

  • Tight-race losers get fosite invalid_request, not invalid_grant — the SDK rethrows it raw without calling invalidateCredentials, so any fix hooked only on the invalid_grant path has a hole. Hence the connect-level waitForSiblingRefresh retry.
  • The losing side's 400 typically arrives before the winner's disk write lands — hence the grace-window poll in front of the destructive clear (which would otherwise poison the shared store for every session).

Client proliferation fixes are stacked separately in #45.

🤖 Generated with Claude Code

— updated via Glean Pi

Comment thread src/auth-provider.ts Outdated
Comment thread src/auth-provider.ts Outdated
pragati-agrawal-glean added a commit to gleanwork/agent-plugins that referenced this pull request Jul 30, 2026
…en rotation

Port of gleanwork/glean-plugins-vnext#44 (squashed; full history and E2E
evidence there).

Each host session runs its own plugin process sharing one credentials
file. The Glean OAuth server rotates refresh tokens on every refresh with
no grace period, so when one session refreshes, every other session's
in-memory copy is revoked; their next refresh gets invalid_grant, the SDK
wipes the SHARED store, and the user sees [SETUP_REQUIRED] — plus every
other live session dies with them.

Fixes (E2E-verified on an experimental pod against real prod /oauth —
bug reproduced on demand with the old build, silent recovery in both
race shapes with this change):

- tokens()/syncTokensFromDisk: mtime-guarded re-read of the shared store
  so a sibling's rotated grant is picked up before the SDK refreshes.
- invalidateCredentials('tokens'): adopt a newer on-disk token instead of
  wiping — with a grace-window poll (GLEAN_ROTATION_GRACE_MS, 2s) because
  the loser's invalid_grant usually lands milliseconds before the
  winner's write.
- Connect-level sibling-refresh retry: concurrent refreshes of the same
  grant make fosite fail the loser with invalid_request (not
  invalid_grant — observed live), which the SDK rethrows raw; recognize
  refresh-shaped failures, wait out the grace window, retry once.
- saveCredentials: temp-file + rename so concurrent writers can't leave
  a torn store that parses as wiped.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@pragati-agrawal-glean

Copy link
Copy Markdown
Contributor Author

Before changes:
Screenshot 2026-08-03 at 1 10 55 AM

After changes:
Screenshot 2026-08-03 at 1 12 43 AM

pragati-agrawal-glean and others added 6 commits August 13, 2026 11:28
…tation

MCP servers are spawned per session, and the OAuth provider reads
credentials from disk once at startup, then serves tokens from that
in-memory snapshot. With Ory single-use refresh-token rotation, when one
process refreshes it persists a new refresh token and invalidates the old
one that every other live process still holds in memory. The next process
to hit a 401 refreshes with its now-dead token -> invalid_grant -> full
re-auth -> [SETUP_REQUIRED]. This is the intermittent "why is it asking me
to auth again", not the (intentional, unchanged) 7-day access-token TTL.

Two fixes, both independent of the PLUGIN_DATA_DIR store split:

1. tokens() re-reads the credentials file when its mtime advances, so a
   process picks up a sibling's freshly-rotated grant before the SDK's
   auth flow reads tokens and attempts a refresh. mtime-guarded so the
   steady state is a single stat(). Conservative on removal: a missing
   file or a tokens-less rewrite does not evict the in-memory token.

2. createRemoteClient retries connect once when, after an auth failure, a
   newer access token has appeared on disk (sibling refresh) -- turning
   the rotation race into a silent reconnect instead of a re-auth. Bounded
   to a single retry.

Tests: provider adopts a sibling's newer token, keeps its token when the
file vanishes, ignores a tokens-less rewrite; credentialsMtimeMs probe;
connect retry fires only when the on-disk token changed.

Follow-ups (not in this PR): collapse the PLUGIN_DATA_DIR / ~/.glean store
split to one canonical path so surfaces share one DCR client; reuse the
DCR client across re-auths to stop orphaned-token pile-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… invalid_grant

The SDK's auth() calls invalidateCredentials("tokens") when a refresh returns
invalid_grant, which wrote {tokens: undefined} to the SHARED credentials file.
But the plugin's OAuth server (legacy /oauth, fosite) rotates refresh tokens
with NO grace period (RevokeRefreshTokenMaybeGracePeriod -> immediate delete),
so invalid_grant is exactly what a sibling's rotation looks like: the sibling
already minted a fresh grant and persisted it, and we only failed because we
refreshed with the now-revoked old token.

Blindly clearing then (a) forced a needless re-auth and (b) clobbered the fresh
token every other session on that store depends on -- one stale session poisoned
the well for all of them. syncFromDisk alone couldn't cover this: the wipe runs
inside connect() (SDK auth() catch), before the connect-retry could re-read.

Now invalidateCredentials("tokens") first checks whether disk holds a token
newer than the one we failed with (mtime-guarded, access_token differs). If so
it adopts that token and keeps it on disk instead of clearing; the SDK's own
post-invalidation retry (authInternal -> refreshAuthorization) then refreshes
with the fresh token and succeeds -- no re-auth, no poisoning. A genuine
invalidation (nothing newer on disk) still clears as before.

Tests: adopt-newer-token-instead-of-wipe (store preserved); clear-when-nothing-
newer. Full suite green, typecheck clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cut the comments added for the cross-process rotation fix down to the
reason/use-case, dropping restatements of what the code already shows.
No behavior change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… retry, atomic writes

E2E-verified on an experimental pod against the real prod /oauth (3-min
token-age gate; two plugin sessions sharing one store):

- OLD build: the session losing a refresh race gets 400, wipes the shared
  store, and surfaces [SETUP_REQUIRED] — reproduced on demand.
- With these changes: the loser recovers silently in both race shapes.

Three additions on top of the existing rails:

1. invalidateCredentials('tokens') now polls the store briefly (2s,
   GLEAN_ROTATION_GRACE_MS) before the destructive clear — the loser's
   invalid_grant usually arrives milliseconds BEFORE the winner's write
   lands, and clearing immediately poisons the shared store for every
   session. Skipped when no refresh token was held (no race possible).

2. Connect-level sibling-refresh retry: when two sessions refresh the same
   grant simultaneously, fosite fails the loser with invalid_request (NOT
   invalid_grant — observed live), which the SDK rethrows raw without
   touching invalidateCredentials. createRemoteClient now recognizes
   refresh-shaped failures, waits out the same grace window for the
   sibling's token, and retries once.

3. saveCredentials writes temp-file + rename so concurrent sibling writers
   can never leave a torn store that parses as wiped.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@pragati-agrawal-glean
pragati-agrawal-glean force-pushed the pragati/fix-plugin-token-rotation-reauth branch from 65c90a9 to 5d53b29 Compare August 13, 2026 05:59
Comment thread src/token-store.ts
encoding: "utf-8",
mode: FILE_MODE,
});
fs.chmodSync(filePath, FILE_MODE);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#clarify why is chmodsync call not needed anymore?

Comment thread src/auth-provider.ts
this._tokens = stored.tokens as OAuthTokens | undefined;
this._clientInfo = stored.clientInfo as OAuthClientInformationMixed | undefined;
}
this._credentialsMtimeMs = credentialsMtimeMs();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#suggest can we get the MTime also from loadCredentials instead of two separate file reads?

Comment thread src/auth-provider.ts

// Re-read the shared store after a sibling process rewrites it, so we use
// the rotated grant instead of a stale in-memory copy.
private syncTokensFromDisk(): void {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading files multiple times in one flow is prone to race conditions. It will be better if we could have a combined function that gives both credentials and mtime.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread src/auth-provider.ts

// On invalid_grant, adopt a sibling's newer on-disk token instead of
// clearing. Returns false when nothing newer exists.
private adoptNewerTokenFromDisk(): boolean {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks exactly the same as syncTokensFromDisk to me.

Comment thread src/auth-provider.ts
if (!this._tokens?.refresh_token) return false;
const deadline = Date.now() + rotationGraceMs();
while (Date.now() < deadline) {
await sleep(ROTATION_POLL_MS);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to define sleep. It must be a standard function?

Comment thread src/auth-provider.ts

// Grace-bounded wait for a sibling's refresh; covers failures the SDK does
// not route through invalidateCredentials (e.g. invalid_request collisions).
async waitForSiblingRefresh(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This again seems very similar to adoptNewerTokenWithGrace

Comment thread src/auth-provider.ts
saveTokens(tokens: OAuthTokens): void {
this._tokens = tokens;
this._authUrlPending = false;
saveCredentials(this._tokens, this._clientInfo);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would prefer to get the Mtime from Save call itself. otherwise, there is again a race condition if two sessions write close to each other. It is possible that a sibling write gets lost as we read the mtime separately after writing.

Comment thread src/auth-provider.ts
break;
case "tokens":
// Usually a sibling's rotation — try adopting before clearing.
if (await this.adoptNewerTokenWithGrace()) return;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We seem to be calling invalidateCredentials only with all scope. Which path does this change impact?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants