fix(auth): recover from concurrent session refreshes - #213

Merged
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry
Apr 28, 2026
Merged

fix(auth): recover from concurrent session refreshes#213
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry

Conversation

@wyattjoh

@wyattjohwyattjoh commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #205. When two separate clerk processes start from the same expired OAuth session, they can both race into refreshStoredSession(). If one process refreshes successfully and rotates the refresh token, the loser sees invalid_grant and, in the current implementation, deletes the credentials that the winner just rewrote.

This PR keeps the existing refresh flow but hardens the invalid_grant path so a losing process re-reads the credential store before treating the session as dead. If another process already stored a newer session, the loser reuses that session instead of deleting credentials and forcing a re-auth.

Fix

  • Add a small recovery path for invalid_grant in credential-store.ts.
  • Capture the failing session's refresh token, then re-read the store with a short retry window (25ms, 50ms, 100ms) before deleting anything. A different refresh token implies a sibling process already rotated the session.
  • If a newer session appears during that window, treat it as the winner of the race and continue with its access token instead of clearing auth state.
  • Keep the existing cleanup behavior for the real expired-session case where the store never changes.
  • Add regression coverage for both paths and a patch changeset for the follow-up fix.

Test plan

  • bunx tsc -p packages/cli-core/tsconfig.json --noEmit
  • bun test packages/cli-core/src/lib/credential-store.test.ts
  • bun test packages/cli-core/src/lib/plapi.test.ts packages/cli-core/src/lib/token-exchange.test.ts packages/cli-core/src/commands/auth/login.test.ts packages/cli-core/src/commands/doctor/doctor.test.ts packages/cli-core/src/commands/whoami/index.test.ts
  • Added a regression test that simulates another process winning the refresh race and verifies the loser reuses the newer stored session.

@changeset-bot

changeset-botBot commented Apr 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9573196

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
clerkPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 25bf8c6 to 708db0cCompareApril 22, 2026 18:37
Base automatically changed from wyattjoh/oauth-refresh-token-support to mainApril 22, 2026 18:39
@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 708db0c to 6ecad3eCompareApril 22, 2026 18:41
@wyattjoh
wyattjoh marked this pull request as ready for review April 22, 2026 18:41
@coderabbitai

coderabbitaiBot commented Apr 22, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 39af30aa-7498-43c3-aa84-17d55a15adc3

📥 Commits

Reviewing files that changed from the base of the PR and between 0d2667c and 9573196.

📒 Files selected for processing (2)
  • packages/cli-core/src/lib/credential-store.test.ts
  • packages/cli-core/src/lib/credential-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli-core/src/lib/credential-store.ts

📝 Walkthrough

Walkthrough

Introduces getValidAccessToken to centralize token expiry checks and moves expiry/refresh decision logic out of getValidToken. Enhances refresh failure handling: on invalid_grant the code polls the credential store with short delays, re-reads the stored session, and if a newer non-expired session is found (detected via a session fingerprint mismatch) it returns that session's access token; otherwise stored credentials are deleted and a session-expired error is thrown. Adds a test that simulates a concurrent refresh where another process writes a refreshed session during recovery. A Changeset file was added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Detailed explanation

  • Adds getValidAccessToken to centralize expiry detection and refresh behavior; getValidToken delegates to it.
  • Introduces sessionFingerprint() to detect changes between reads of the stored session.
  • Implements recoverFromInvalidGrant() which performs short delays, re-reads stored credentials, and if a fingerprint mismatch reveals a newer valid session, returns that session's access token.
  • Modifies refreshStoredSession to invoke recovery on invalid_grant before deleting stored credentials and throwing a session-expired error.
  • Adds a test that simulates an expired stored session, a refresh call failing with invalid_grant, and a concurrent writer that persists a refreshed session; asserts the resolved token and stored session match the newer session.
  • Adds a Changeset metadata file to trigger a patch release documenting the concurrency behavior for refreshed OAuth credentials.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title 'fix(auth): recover from concurrent session refreshes' directly and clearly summarizes the main change—hardening the invalid_grant handling to recover from race conditions when multiple processes refresh the same expired session.
Description check✅ PassedThe description is well-detailed and clearly related to the changeset, explaining the problem, the fix strategy, implementation details, and test coverage.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


Comment @coderabbitai help to get the list of available commands and usage tips.

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 6ecad3e to 38431d4CompareApril 23, 2026 20:52

@rafa-thaytorafa-thayto left a comment

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.

LGTM with two suggestions below. The approach is solid and well-scoped. The fingerprint + retry strategy is a clean way to handle this race.

Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Prevent unbounded recursion when a newer stored session is itself
expired, and ensure recovery failures still fall through to the
delete + session_expired path so the user gets the friendly re-auth
prompt instead of a raw error.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/credential-store.ts`:
- Around line 31-32: The timed-polling invalid_grant fallback currently clears
the credential store unconditionally after the retry budget
(INVALID_GRANT_RETRY_DELAYS_MS) and can delete a freshly written session; change
the fallback to be non-destructive by verifying the store still contains the
original fingerprint before deleting or by avoiding deletion entirely and
instead marking the local attempt as failed. Concretely: capture the original
fingerprint value when you start the retry loop, then before calling the code
that clears the store (the branch that deletes credentials on invalid_grant),
re-read the credential store and compare its current fingerprint to the captured
original; only perform the destructive clear if they match, otherwise skip
deletion (or set a non-destructive failure flag/log). Ensure this check is
applied to the invalid_grant branch that references
INVALID_GRANT_RETRY_DELAYS_MS so the race is eliminated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9ff5427-60ad-42a6-a67d-b3f4650d0923

📥 Commits

Reviewing files that changed from the base of the PR and between 38431d4 and 0d2667c.

📒 Files selected for processing (1)
  • packages/cli-core/src/lib/credential-store.ts

Comment threadpackages/cli-core/src/lib/credential-store.ts
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.test.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Rename recoverFromInvalidGrant to awaitConcurrentRefresh and add a JSDoc
covering the race window, polling budget, and detection rationale. Drop
the sessionFingerprint helper in favor of comparing refresh tokens
directly, since the OAuth server rotates them on every successful
exchange. Update the test description to make the concurrent-refresh
framing explicit.
@wyattjoh
wyattjoh requested a review from jfosheeApril 28, 2026 15:19
@wyattjoh
wyattjoh merged commit 2e6d03b into mainApr 28, 2026
10 checks passed
@wyattjoh
wyattjoh deleted the wyattjoh/oauth-refresh-race-retry branch April 28, 2026 20:00
@github-actionsgithub-actionsBot mentioned this pull request Apr 28, 2026
Sign up for freeto 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.

3 participants

@wyattjoh@jfoshee@rafa-thayto
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

fix(auth): recover from concurrent session refreshes - #213

Merged
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry
Apr 28, 2026
Merged

fix(auth): recover from concurrent session refreshes#213
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry

Conversation

@wyattjoh

@wyattjohwyattjoh commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #205. When two separate clerk processes start from the same expired OAuth session, they can both race into refreshStoredSession(). If one process refreshes successfully and rotates the refresh token, the loser sees invalid_grant and, in the current implementation, deletes the credentials that the winner just rewrote.

This PR keeps the existing refresh flow but hardens the invalid_grant path so a losing process re-reads the credential store before treating the session as dead. If another process already stored a newer session, the loser reuses that session instead of deleting credentials and forcing a re-auth.

Fix

  • Add a small recovery path for invalid_grant in credential-store.ts.
  • Capture the failing session's refresh token, then re-read the store with a short retry window (25ms, 50ms, 100ms) before deleting anything. A different refresh token implies a sibling process already rotated the session.
  • If a newer session appears during that window, treat it as the winner of the race and continue with its access token instead of clearing auth state.
  • Keep the existing cleanup behavior for the real expired-session case where the store never changes.
  • Add regression coverage for both paths and a patch changeset for the follow-up fix.

Test plan

  • bunx tsc -p packages/cli-core/tsconfig.json --noEmit
  • bun test packages/cli-core/src/lib/credential-store.test.ts
  • bun test packages/cli-core/src/lib/plapi.test.ts packages/cli-core/src/lib/token-exchange.test.ts packages/cli-core/src/commands/auth/login.test.ts packages/cli-core/src/commands/doctor/doctor.test.ts packages/cli-core/src/commands/whoami/index.test.ts
  • Added a regression test that simulates another process winning the refresh race and verifies the loser reuses the newer stored session.

@changeset-bot

changeset-botBot commented Apr 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9573196

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
clerkPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 25bf8c6 to 708db0cCompareApril 22, 2026 18:37
Base automatically changed from wyattjoh/oauth-refresh-token-support to mainApril 22, 2026 18:39
@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 708db0c to 6ecad3eCompareApril 22, 2026 18:41
@wyattjoh
wyattjoh marked this pull request as ready for review April 22, 2026 18:41
@coderabbitai

coderabbitaiBot commented Apr 22, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 39af30aa-7498-43c3-aa84-17d55a15adc3

📥 Commits

Reviewing files that changed from the base of the PR and between 0d2667c and 9573196.

📒 Files selected for processing (2)
  • packages/cli-core/src/lib/credential-store.test.ts
  • packages/cli-core/src/lib/credential-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli-core/src/lib/credential-store.ts

📝 Walkthrough

Walkthrough

Introduces getValidAccessToken to centralize token expiry checks and moves expiry/refresh decision logic out of getValidToken. Enhances refresh failure handling: on invalid_grant the code polls the credential store with short delays, re-reads the stored session, and if a newer non-expired session is found (detected via a session fingerprint mismatch) it returns that session's access token; otherwise stored credentials are deleted and a session-expired error is thrown. Adds a test that simulates a concurrent refresh where another process writes a refreshed session during recovery. A Changeset file was added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Detailed explanation

  • Adds getValidAccessToken to centralize expiry detection and refresh behavior; getValidToken delegates to it.
  • Introduces sessionFingerprint() to detect changes between reads of the stored session.
  • Implements recoverFromInvalidGrant() which performs short delays, re-reads stored credentials, and if a fingerprint mismatch reveals a newer valid session, returns that session's access token.
  • Modifies refreshStoredSession to invoke recovery on invalid_grant before deleting stored credentials and throwing a session-expired error.
  • Adds a test that simulates an expired stored session, a refresh call failing with invalid_grant, and a concurrent writer that persists a refreshed session; asserts the resolved token and stored session match the newer session.
  • Adds a Changeset metadata file to trigger a patch release documenting the concurrency behavior for refreshed OAuth credentials.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title 'fix(auth): recover from concurrent session refreshes' directly and clearly summarizes the main change—hardening the invalid_grant handling to recover from race conditions when multiple processes refresh the same expired session.
Description check✅ PassedThe description is well-detailed and clearly related to the changeset, explaining the problem, the fix strategy, implementation details, and test coverage.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


Comment @coderabbitai help to get the list of available commands and usage tips.

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 6ecad3e to 38431d4CompareApril 23, 2026 20:52

@rafa-thaytorafa-thayto left a comment

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.

LGTM with two suggestions below. The approach is solid and well-scoped. The fingerprint + retry strategy is a clean way to handle this race.

Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Prevent unbounded recursion when a newer stored session is itself
expired, and ensure recovery failures still fall through to the
delete + session_expired path so the user gets the friendly re-auth
prompt instead of a raw error.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/credential-store.ts`:
- Around line 31-32: The timed-polling invalid_grant fallback currently clears
the credential store unconditionally after the retry budget
(INVALID_GRANT_RETRY_DELAYS_MS) and can delete a freshly written session; change
the fallback to be non-destructive by verifying the store still contains the
original fingerprint before deleting or by avoiding deletion entirely and
instead marking the local attempt as failed. Concretely: capture the original
fingerprint value when you start the retry loop, then before calling the code
that clears the store (the branch that deletes credentials on invalid_grant),
re-read the credential store and compare its current fingerprint to the captured
original; only perform the destructive clear if they match, otherwise skip
deletion (or set a non-destructive failure flag/log). Ensure this check is
applied to the invalid_grant branch that references
INVALID_GRANT_RETRY_DELAYS_MS so the race is eliminated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9ff5427-60ad-42a6-a67d-b3f4650d0923

📥 Commits

Reviewing files that changed from the base of the PR and between 38431d4 and 0d2667c.

📒 Files selected for processing (1)
  • packages/cli-core/src/lib/credential-store.ts

Comment threadpackages/cli-core/src/lib/credential-store.ts
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.test.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Rename recoverFromInvalidGrant to awaitConcurrentRefresh and add a JSDoc
covering the race window, polling budget, and detection rationale. Drop
the sessionFingerprint helper in favor of comparing refresh tokens
directly, since the OAuth server rotates them on every successful
exchange. Update the test description to make the concurrent-refresh
framing explicit.
@wyattjoh
wyattjoh requested a review from jfosheeApril 28, 2026 15:19
@wyattjoh
wyattjoh merged commit 2e6d03b into mainApr 28, 2026
10 checks passed
@wyattjoh
wyattjoh deleted the wyattjoh/oauth-refresh-race-retry branch April 28, 2026 20:00
@github-actionsgithub-actionsBot mentioned this pull request Apr 28, 2026
Sign up for freeto 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.

3 participants

@wyattjoh@jfoshee@rafa-thayto
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(auth): recover from concurrent session refreshes - #213

Merged
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry
Apr 28, 2026
Merged

fix(auth): recover from concurrent session refreshes#213
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry

Conversation

@wyattjoh

@wyattjohwyattjoh commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #205. When two separate clerk processes start from the same expired OAuth session, they can both race into refreshStoredSession(). If one process refreshes successfully and rotates the refresh token, the loser sees invalid_grant and, in the current implementation, deletes the credentials that the winner just rewrote.

This PR keeps the existing refresh flow but hardens the invalid_grant path so a losing process re-reads the credential store before treating the session as dead. If another process already stored a newer session, the loser reuses that session instead of deleting credentials and forcing a re-auth.

Fix

  • Add a small recovery path for invalid_grant in credential-store.ts.
  • Capture the failing session's refresh token, then re-read the store with a short retry window (25ms, 50ms, 100ms) before deleting anything. A different refresh token implies a sibling process already rotated the session.
  • If a newer session appears during that window, treat it as the winner of the race and continue with its access token instead of clearing auth state.
  • Keep the existing cleanup behavior for the real expired-session case where the store never changes.
  • Add regression coverage for both paths and a patch changeset for the follow-up fix.

Test plan

  • bunx tsc -p packages/cli-core/tsconfig.json --noEmit
  • bun test packages/cli-core/src/lib/credential-store.test.ts
  • bun test packages/cli-core/src/lib/plapi.test.ts packages/cli-core/src/lib/token-exchange.test.ts packages/cli-core/src/commands/auth/login.test.ts packages/cli-core/src/commands/doctor/doctor.test.ts packages/cli-core/src/commands/whoami/index.test.ts
  • Added a regression test that simulates another process winning the refresh race and verifies the loser reuses the newer stored session.

@changeset-bot

changeset-botBot commented Apr 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9573196

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
clerkPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 25bf8c6 to 708db0cCompareApril 22, 2026 18:37
Base automatically changed from wyattjoh/oauth-refresh-token-support to mainApril 22, 2026 18:39
@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 708db0c to 6ecad3eCompareApril 22, 2026 18:41
@wyattjoh
wyattjoh marked this pull request as ready for review April 22, 2026 18:41
@coderabbitai

coderabbitaiBot commented Apr 22, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 39af30aa-7498-43c3-aa84-17d55a15adc3

📥 Commits

Reviewing files that changed from the base of the PR and between 0d2667c and 9573196.

📒 Files selected for processing (2)
  • packages/cli-core/src/lib/credential-store.test.ts
  • packages/cli-core/src/lib/credential-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli-core/src/lib/credential-store.ts

📝 Walkthrough

Walkthrough

Introduces getValidAccessToken to centralize token expiry checks and moves expiry/refresh decision logic out of getValidToken. Enhances refresh failure handling: on invalid_grant the code polls the credential store with short delays, re-reads the stored session, and if a newer non-expired session is found (detected via a session fingerprint mismatch) it returns that session's access token; otherwise stored credentials are deleted and a session-expired error is thrown. Adds a test that simulates a concurrent refresh where another process writes a refreshed session during recovery. A Changeset file was added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Detailed explanation

  • Adds getValidAccessToken to centralize expiry detection and refresh behavior; getValidToken delegates to it.
  • Introduces sessionFingerprint() to detect changes between reads of the stored session.
  • Implements recoverFromInvalidGrant() which performs short delays, re-reads stored credentials, and if a fingerprint mismatch reveals a newer valid session, returns that session's access token.
  • Modifies refreshStoredSession to invoke recovery on invalid_grant before deleting stored credentials and throwing a session-expired error.
  • Adds a test that simulates an expired stored session, a refresh call failing with invalid_grant, and a concurrent writer that persists a refreshed session; asserts the resolved token and stored session match the newer session.
  • Adds a Changeset metadata file to trigger a patch release documenting the concurrency behavior for refreshed OAuth credentials.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title 'fix(auth): recover from concurrent session refreshes' directly and clearly summarizes the main change—hardening the invalid_grant handling to recover from race conditions when multiple processes refresh the same expired session.
Description check✅ PassedThe description is well-detailed and clearly related to the changeset, explaining the problem, the fix strategy, implementation details, and test coverage.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


Comment @coderabbitai help to get the list of available commands and usage tips.

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 6ecad3e to 38431d4CompareApril 23, 2026 20:52

@rafa-thaytorafa-thayto left a comment

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.

LGTM with two suggestions below. The approach is solid and well-scoped. The fingerprint + retry strategy is a clean way to handle this race.

Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Prevent unbounded recursion when a newer stored session is itself
expired, and ensure recovery failures still fall through to the
delete + session_expired path so the user gets the friendly re-auth
prompt instead of a raw error.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/credential-store.ts`:
- Around line 31-32: The timed-polling invalid_grant fallback currently clears
the credential store unconditionally after the retry budget
(INVALID_GRANT_RETRY_DELAYS_MS) and can delete a freshly written session; change
the fallback to be non-destructive by verifying the store still contains the
original fingerprint before deleting or by avoiding deletion entirely and
instead marking the local attempt as failed. Concretely: capture the original
fingerprint value when you start the retry loop, then before calling the code
that clears the store (the branch that deletes credentials on invalid_grant),
re-read the credential store and compare its current fingerprint to the captured
original; only perform the destructive clear if they match, otherwise skip
deletion (or set a non-destructive failure flag/log). Ensure this check is
applied to the invalid_grant branch that references
INVALID_GRANT_RETRY_DELAYS_MS so the race is eliminated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9ff5427-60ad-42a6-a67d-b3f4650d0923

📥 Commits

Reviewing files that changed from the base of the PR and between 38431d4 and 0d2667c.

📒 Files selected for processing (1)
  • packages/cli-core/src/lib/credential-store.ts

Comment threadpackages/cli-core/src/lib/credential-store.ts
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.test.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Rename recoverFromInvalidGrant to awaitConcurrentRefresh and add a JSDoc
covering the race window, polling budget, and detection rationale. Drop
the sessionFingerprint helper in favor of comparing refresh tokens
directly, since the OAuth server rotates them on every successful
exchange. Update the test description to make the concurrent-refresh
framing explicit.
@wyattjoh
wyattjoh requested a review from jfosheeApril 28, 2026 15:19
@wyattjoh
wyattjoh merged commit 2e6d03b into mainApr 28, 2026
10 checks passed
@wyattjoh
wyattjoh deleted the wyattjoh/oauth-refresh-race-retry branch April 28, 2026 20:00
@github-actionsgithub-actionsBot mentioned this pull request Apr 28, 2026
Sign up for freeto 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.

3 participants

@wyattjoh@jfoshee@rafa-thayto
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(auth): recover from concurrent session refreshes - #213

Merged
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry
Apr 28, 2026
Merged

fix(auth): recover from concurrent session refreshes#213
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry

Conversation

@wyattjoh

@wyattjohwyattjoh commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #205. When two separate clerk processes start from the same expired OAuth session, they can both race into refreshStoredSession(). If one process refreshes successfully and rotates the refresh token, the loser sees invalid_grant and, in the current implementation, deletes the credentials that the winner just rewrote.

This PR keeps the existing refresh flow but hardens the invalid_grant path so a losing process re-reads the credential store before treating the session as dead. If another process already stored a newer session, the loser reuses that session instead of deleting credentials and forcing a re-auth.

Fix

  • Add a small recovery path for invalid_grant in credential-store.ts.
  • Capture the failing session's refresh token, then re-read the store with a short retry window (25ms, 50ms, 100ms) before deleting anything. A different refresh token implies a sibling process already rotated the session.
  • If a newer session appears during that window, treat it as the winner of the race and continue with its access token instead of clearing auth state.
  • Keep the existing cleanup behavior for the real expired-session case where the store never changes.
  • Add regression coverage for both paths and a patch changeset for the follow-up fix.

Test plan

  • bunx tsc -p packages/cli-core/tsconfig.json --noEmit
  • bun test packages/cli-core/src/lib/credential-store.test.ts
  • bun test packages/cli-core/src/lib/plapi.test.ts packages/cli-core/src/lib/token-exchange.test.ts packages/cli-core/src/commands/auth/login.test.ts packages/cli-core/src/commands/doctor/doctor.test.ts packages/cli-core/src/commands/whoami/index.test.ts
  • Added a regression test that simulates another process winning the refresh race and verifies the loser reuses the newer stored session.

@changeset-bot

changeset-botBot commented Apr 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9573196

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
clerkPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 25bf8c6 to 708db0cCompareApril 22, 2026 18:37
Base automatically changed from wyattjoh/oauth-refresh-token-support to mainApril 22, 2026 18:39
@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 708db0c to 6ecad3eCompareApril 22, 2026 18:41
@wyattjoh
wyattjoh marked this pull request as ready for review April 22, 2026 18:41
@coderabbitai

coderabbitaiBot commented Apr 22, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 39af30aa-7498-43c3-aa84-17d55a15adc3

📥 Commits

Reviewing files that changed from the base of the PR and between 0d2667c and 9573196.

📒 Files selected for processing (2)
  • packages/cli-core/src/lib/credential-store.test.ts
  • packages/cli-core/src/lib/credential-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli-core/src/lib/credential-store.ts

📝 Walkthrough

Walkthrough

Introduces getValidAccessToken to centralize token expiry checks and moves expiry/refresh decision logic out of getValidToken. Enhances refresh failure handling: on invalid_grant the code polls the credential store with short delays, re-reads the stored session, and if a newer non-expired session is found (detected via a session fingerprint mismatch) it returns that session's access token; otherwise stored credentials are deleted and a session-expired error is thrown. Adds a test that simulates a concurrent refresh where another process writes a refreshed session during recovery. A Changeset file was added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Detailed explanation

  • Adds getValidAccessToken to centralize expiry detection and refresh behavior; getValidToken delegates to it.
  • Introduces sessionFingerprint() to detect changes between reads of the stored session.
  • Implements recoverFromInvalidGrant() which performs short delays, re-reads stored credentials, and if a fingerprint mismatch reveals a newer valid session, returns that session's access token.
  • Modifies refreshStoredSession to invoke recovery on invalid_grant before deleting stored credentials and throwing a session-expired error.
  • Adds a test that simulates an expired stored session, a refresh call failing with invalid_grant, and a concurrent writer that persists a refreshed session; asserts the resolved token and stored session match the newer session.
  • Adds a Changeset metadata file to trigger a patch release documenting the concurrency behavior for refreshed OAuth credentials.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title 'fix(auth): recover from concurrent session refreshes' directly and clearly summarizes the main change—hardening the invalid_grant handling to recover from race conditions when multiple processes refresh the same expired session.
Description check✅ PassedThe description is well-detailed and clearly related to the changeset, explaining the problem, the fix strategy, implementation details, and test coverage.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


Comment @coderabbitai help to get the list of available commands and usage tips.

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 6ecad3e to 38431d4CompareApril 23, 2026 20:52

@rafa-thaytorafa-thayto left a comment

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.

LGTM with two suggestions below. The approach is solid and well-scoped. The fingerprint + retry strategy is a clean way to handle this race.

Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Prevent unbounded recursion when a newer stored session is itself
expired, and ensure recovery failures still fall through to the
delete + session_expired path so the user gets the friendly re-auth
prompt instead of a raw error.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/credential-store.ts`:
- Around line 31-32: The timed-polling invalid_grant fallback currently clears
the credential store unconditionally after the retry budget
(INVALID_GRANT_RETRY_DELAYS_MS) and can delete a freshly written session; change
the fallback to be non-destructive by verifying the store still contains the
original fingerprint before deleting or by avoiding deletion entirely and
instead marking the local attempt as failed. Concretely: capture the original
fingerprint value when you start the retry loop, then before calling the code
that clears the store (the branch that deletes credentials on invalid_grant),
re-read the credential store and compare its current fingerprint to the captured
original; only perform the destructive clear if they match, otherwise skip
deletion (or set a non-destructive failure flag/log). Ensure this check is
applied to the invalid_grant branch that references
INVALID_GRANT_RETRY_DELAYS_MS so the race is eliminated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9ff5427-60ad-42a6-a67d-b3f4650d0923

📥 Commits

Reviewing files that changed from the base of the PR and between 38431d4 and 0d2667c.

📒 Files selected for processing (1)
  • packages/cli-core/src/lib/credential-store.ts

Comment threadpackages/cli-core/src/lib/credential-store.ts
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.test.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Rename recoverFromInvalidGrant to awaitConcurrentRefresh and add a JSDoc
covering the race window, polling budget, and detection rationale. Drop
the sessionFingerprint helper in favor of comparing refresh tokens
directly, since the OAuth server rotates them on every successful
exchange. Update the test description to make the concurrent-refresh
framing explicit.
@wyattjoh
wyattjoh requested a review from jfosheeApril 28, 2026 15:19
@wyattjoh
wyattjoh merged commit 2e6d03b into mainApr 28, 2026
10 checks passed
@wyattjoh
wyattjoh deleted the wyattjoh/oauth-refresh-race-retry branch April 28, 2026 20:00
@github-actionsgithub-actionsBot mentioned this pull request Apr 28, 2026
Sign up for freeto 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.

3 participants

@wyattjoh@jfoshee@rafa-thayto
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

fix(auth): recover from concurrent session refreshes - #213

Merged
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry
Apr 28, 2026
Merged

fix(auth): recover from concurrent session refreshes#213
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry

Conversation

@wyattjoh

@wyattjohwyattjoh commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #205. When two separate clerk processes start from the same expired OAuth session, they can both race into refreshStoredSession(). If one process refreshes successfully and rotates the refresh token, the loser sees invalid_grant and, in the current implementation, deletes the credentials that the winner just rewrote.

This PR keeps the existing refresh flow but hardens the invalid_grant path so a losing process re-reads the credential store before treating the session as dead. If another process already stored a newer session, the loser reuses that session instead of deleting credentials and forcing a re-auth.

Fix

  • Add a small recovery path for invalid_grant in credential-store.ts.
  • Capture the failing session's refresh token, then re-read the store with a short retry window (25ms, 50ms, 100ms) before deleting anything. A different refresh token implies a sibling process already rotated the session.
  • If a newer session appears during that window, treat it as the winner of the race and continue with its access token instead of clearing auth state.
  • Keep the existing cleanup behavior for the real expired-session case where the store never changes.
  • Add regression coverage for both paths and a patch changeset for the follow-up fix.

Test plan

  • bunx tsc -p packages/cli-core/tsconfig.json --noEmit
  • bun test packages/cli-core/src/lib/credential-store.test.ts
  • bun test packages/cli-core/src/lib/plapi.test.ts packages/cli-core/src/lib/token-exchange.test.ts packages/cli-core/src/commands/auth/login.test.ts packages/cli-core/src/commands/doctor/doctor.test.ts packages/cli-core/src/commands/whoami/index.test.ts
  • Added a regression test that simulates another process winning the refresh race and verifies the loser reuses the newer stored session.

@changeset-bot

changeset-botBot commented Apr 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9573196

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
clerkPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 25bf8c6 to 708db0cCompareApril 22, 2026 18:37
Base automatically changed from wyattjoh/oauth-refresh-token-support to mainApril 22, 2026 18:39
@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 708db0c to 6ecad3eCompareApril 22, 2026 18:41
@wyattjoh
wyattjoh marked this pull request as ready for review April 22, 2026 18:41
@coderabbitai

coderabbitaiBot commented Apr 22, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 39af30aa-7498-43c3-aa84-17d55a15adc3

📥 Commits

Reviewing files that changed from the base of the PR and between 0d2667c and 9573196.

📒 Files selected for processing (2)
  • packages/cli-core/src/lib/credential-store.test.ts
  • packages/cli-core/src/lib/credential-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli-core/src/lib/credential-store.ts

📝 Walkthrough

Walkthrough

Introduces getValidAccessToken to centralize token expiry checks and moves expiry/refresh decision logic out of getValidToken. Enhances refresh failure handling: on invalid_grant the code polls the credential store with short delays, re-reads the stored session, and if a newer non-expired session is found (detected via a session fingerprint mismatch) it returns that session's access token; otherwise stored credentials are deleted and a session-expired error is thrown. Adds a test that simulates a concurrent refresh where another process writes a refreshed session during recovery. A Changeset file was added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Detailed explanation

  • Adds getValidAccessToken to centralize expiry detection and refresh behavior; getValidToken delegates to it.
  • Introduces sessionFingerprint() to detect changes between reads of the stored session.
  • Implements recoverFromInvalidGrant() which performs short delays, re-reads stored credentials, and if a fingerprint mismatch reveals a newer valid session, returns that session's access token.
  • Modifies refreshStoredSession to invoke recovery on invalid_grant before deleting stored credentials and throwing a session-expired error.
  • Adds a test that simulates an expired stored session, a refresh call failing with invalid_grant, and a concurrent writer that persists a refreshed session; asserts the resolved token and stored session match the newer session.
  • Adds a Changeset metadata file to trigger a patch release documenting the concurrency behavior for refreshed OAuth credentials.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title 'fix(auth): recover from concurrent session refreshes' directly and clearly summarizes the main change—hardening the invalid_grant handling to recover from race conditions when multiple processes refresh the same expired session.
Description check✅ PassedThe description is well-detailed and clearly related to the changeset, explaining the problem, the fix strategy, implementation details, and test coverage.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


Comment @coderabbitai help to get the list of available commands and usage tips.

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 6ecad3e to 38431d4CompareApril 23, 2026 20:52

@rafa-thaytorafa-thayto left a comment

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.

LGTM with two suggestions below. The approach is solid and well-scoped. The fingerprint + retry strategy is a clean way to handle this race.

Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Prevent unbounded recursion when a newer stored session is itself
expired, and ensure recovery failures still fall through to the
delete + session_expired path so the user gets the friendly re-auth
prompt instead of a raw error.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/credential-store.ts`:
- Around line 31-32: The timed-polling invalid_grant fallback currently clears
the credential store unconditionally after the retry budget
(INVALID_GRANT_RETRY_DELAYS_MS) and can delete a freshly written session; change
the fallback to be non-destructive by verifying the store still contains the
original fingerprint before deleting or by avoiding deletion entirely and
instead marking the local attempt as failed. Concretely: capture the original
fingerprint value when you start the retry loop, then before calling the code
that clears the store (the branch that deletes credentials on invalid_grant),
re-read the credential store and compare its current fingerprint to the captured
original; only perform the destructive clear if they match, otherwise skip
deletion (or set a non-destructive failure flag/log). Ensure this check is
applied to the invalid_grant branch that references
INVALID_GRANT_RETRY_DELAYS_MS so the race is eliminated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9ff5427-60ad-42a6-a67d-b3f4650d0923

📥 Commits

Reviewing files that changed from the base of the PR and between 38431d4 and 0d2667c.

📒 Files selected for processing (1)
  • packages/cli-core/src/lib/credential-store.ts

Comment threadpackages/cli-core/src/lib/credential-store.ts
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.test.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Rename recoverFromInvalidGrant to awaitConcurrentRefresh and add a JSDoc
covering the race window, polling budget, and detection rationale. Drop
the sessionFingerprint helper in favor of comparing refresh tokens
directly, since the OAuth server rotates them on every successful
exchange. Update the test description to make the concurrent-refresh
framing explicit.
@wyattjoh
wyattjoh requested a review from jfosheeApril 28, 2026 15:19
@wyattjoh
wyattjoh merged commit 2e6d03b into mainApr 28, 2026
10 checks passed
@wyattjoh
wyattjoh deleted the wyattjoh/oauth-refresh-race-retry branch April 28, 2026 20:00
@github-actionsgithub-actionsBot mentioned this pull request Apr 28, 2026
Sign up for freeto 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.

3 participants

@wyattjoh@jfoshee@rafa-thayto
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(auth): recover from concurrent session refreshes - #213

Merged
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry
Apr 28, 2026
Merged

fix(auth): recover from concurrent session refreshes#213
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry

Conversation

@wyattjoh

@wyattjohwyattjoh commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #205. When two separate clerk processes start from the same expired OAuth session, they can both race into refreshStoredSession(). If one process refreshes successfully and rotates the refresh token, the loser sees invalid_grant and, in the current implementation, deletes the credentials that the winner just rewrote.

This PR keeps the existing refresh flow but hardens the invalid_grant path so a losing process re-reads the credential store before treating the session as dead. If another process already stored a newer session, the loser reuses that session instead of deleting credentials and forcing a re-auth.

Fix

  • Add a small recovery path for invalid_grant in credential-store.ts.
  • Capture the failing session's refresh token, then re-read the store with a short retry window (25ms, 50ms, 100ms) before deleting anything. A different refresh token implies a sibling process already rotated the session.
  • If a newer session appears during that window, treat it as the winner of the race and continue with its access token instead of clearing auth state.
  • Keep the existing cleanup behavior for the real expired-session case where the store never changes.
  • Add regression coverage for both paths and a patch changeset for the follow-up fix.

Test plan

  • bunx tsc -p packages/cli-core/tsconfig.json --noEmit
  • bun test packages/cli-core/src/lib/credential-store.test.ts
  • bun test packages/cli-core/src/lib/plapi.test.ts packages/cli-core/src/lib/token-exchange.test.ts packages/cli-core/src/commands/auth/login.test.ts packages/cli-core/src/commands/doctor/doctor.test.ts packages/cli-core/src/commands/whoami/index.test.ts
  • Added a regression test that simulates another process winning the refresh race and verifies the loser reuses the newer stored session.

@changeset-bot

changeset-botBot commented Apr 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9573196

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
clerkPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 25bf8c6 to 708db0cCompareApril 22, 2026 18:37
Base automatically changed from wyattjoh/oauth-refresh-token-support to mainApril 22, 2026 18:39
@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 708db0c to 6ecad3eCompareApril 22, 2026 18:41
@wyattjoh
wyattjoh marked this pull request as ready for review April 22, 2026 18:41
@coderabbitai

coderabbitaiBot commented Apr 22, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 39af30aa-7498-43c3-aa84-17d55a15adc3

📥 Commits

Reviewing files that changed from the base of the PR and between 0d2667c and 9573196.

📒 Files selected for processing (2)
  • packages/cli-core/src/lib/credential-store.test.ts
  • packages/cli-core/src/lib/credential-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli-core/src/lib/credential-store.ts

📝 Walkthrough

Walkthrough

Introduces getValidAccessToken to centralize token expiry checks and moves expiry/refresh decision logic out of getValidToken. Enhances refresh failure handling: on invalid_grant the code polls the credential store with short delays, re-reads the stored session, and if a newer non-expired session is found (detected via a session fingerprint mismatch) it returns that session's access token; otherwise stored credentials are deleted and a session-expired error is thrown. Adds a test that simulates a concurrent refresh where another process writes a refreshed session during recovery. A Changeset file was added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Detailed explanation

  • Adds getValidAccessToken to centralize expiry detection and refresh behavior; getValidToken delegates to it.
  • Introduces sessionFingerprint() to detect changes between reads of the stored session.
  • Implements recoverFromInvalidGrant() which performs short delays, re-reads stored credentials, and if a fingerprint mismatch reveals a newer valid session, returns that session's access token.
  • Modifies refreshStoredSession to invoke recovery on invalid_grant before deleting stored credentials and throwing a session-expired error.
  • Adds a test that simulates an expired stored session, a refresh call failing with invalid_grant, and a concurrent writer that persists a refreshed session; asserts the resolved token and stored session match the newer session.
  • Adds a Changeset metadata file to trigger a patch release documenting the concurrency behavior for refreshed OAuth credentials.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title 'fix(auth): recover from concurrent session refreshes' directly and clearly summarizes the main change—hardening the invalid_grant handling to recover from race conditions when multiple processes refresh the same expired session.
Description check✅ PassedThe description is well-detailed and clearly related to the changeset, explaining the problem, the fix strategy, implementation details, and test coverage.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


Comment @coderabbitai help to get the list of available commands and usage tips.

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 6ecad3e to 38431d4CompareApril 23, 2026 20:52

@rafa-thaytorafa-thayto left a comment

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.

LGTM with two suggestions below. The approach is solid and well-scoped. The fingerprint + retry strategy is a clean way to handle this race.

Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Prevent unbounded recursion when a newer stored session is itself
expired, and ensure recovery failures still fall through to the
delete + session_expired path so the user gets the friendly re-auth
prompt instead of a raw error.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/credential-store.ts`:
- Around line 31-32: The timed-polling invalid_grant fallback currently clears
the credential store unconditionally after the retry budget
(INVALID_GRANT_RETRY_DELAYS_MS) and can delete a freshly written session; change
the fallback to be non-destructive by verifying the store still contains the
original fingerprint before deleting or by avoiding deletion entirely and
instead marking the local attempt as failed. Concretely: capture the original
fingerprint value when you start the retry loop, then before calling the code
that clears the store (the branch that deletes credentials on invalid_grant),
re-read the credential store and compare its current fingerprint to the captured
original; only perform the destructive clear if they match, otherwise skip
deletion (or set a non-destructive failure flag/log). Ensure this check is
applied to the invalid_grant branch that references
INVALID_GRANT_RETRY_DELAYS_MS so the race is eliminated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9ff5427-60ad-42a6-a67d-b3f4650d0923

📥 Commits

Reviewing files that changed from the base of the PR and between 38431d4 and 0d2667c.

📒 Files selected for processing (1)
  • packages/cli-core/src/lib/credential-store.ts

Comment threadpackages/cli-core/src/lib/credential-store.ts
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.test.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Rename recoverFromInvalidGrant to awaitConcurrentRefresh and add a JSDoc
covering the race window, polling budget, and detection rationale. Drop
the sessionFingerprint helper in favor of comparing refresh tokens
directly, since the OAuth server rotates them on every successful
exchange. Update the test description to make the concurrent-refresh
framing explicit.
@wyattjoh
wyattjoh requested a review from jfosheeApril 28, 2026 15:19
@wyattjoh
wyattjoh merged commit 2e6d03b into mainApr 28, 2026
10 checks passed
@wyattjoh
wyattjoh deleted the wyattjoh/oauth-refresh-race-retry branch April 28, 2026 20:00
@github-actionsgithub-actionsBot mentioned this pull request Apr 28, 2026
Sign up for freeto 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.

3 participants

@wyattjoh@jfoshee@rafa-thayto
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

fix(auth): recover from concurrent session refreshes - #213

Merged
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry
Apr 28, 2026
Merged

fix(auth): recover from concurrent session refreshes#213
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry

Conversation

@wyattjoh

@wyattjohwyattjoh commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #205. When two separate clerk processes start from the same expired OAuth session, they can both race into refreshStoredSession(). If one process refreshes successfully and rotates the refresh token, the loser sees invalid_grant and, in the current implementation, deletes the credentials that the winner just rewrote.

This PR keeps the existing refresh flow but hardens the invalid_grant path so a losing process re-reads the credential store before treating the session as dead. If another process already stored a newer session, the loser reuses that session instead of deleting credentials and forcing a re-auth.

Fix

  • Add a small recovery path for invalid_grant in credential-store.ts.
  • Capture the failing session's refresh token, then re-read the store with a short retry window (25ms, 50ms, 100ms) before deleting anything. A different refresh token implies a sibling process already rotated the session.
  • If a newer session appears during that window, treat it as the winner of the race and continue with its access token instead of clearing auth state.
  • Keep the existing cleanup behavior for the real expired-session case where the store never changes.
  • Add regression coverage for both paths and a patch changeset for the follow-up fix.

Test plan

  • bunx tsc -p packages/cli-core/tsconfig.json --noEmit
  • bun test packages/cli-core/src/lib/credential-store.test.ts
  • bun test packages/cli-core/src/lib/plapi.test.ts packages/cli-core/src/lib/token-exchange.test.ts packages/cli-core/src/commands/auth/login.test.ts packages/cli-core/src/commands/doctor/doctor.test.ts packages/cli-core/src/commands/whoami/index.test.ts
  • Added a regression test that simulates another process winning the refresh race and verifies the loser reuses the newer stored session.

@changeset-bot

changeset-botBot commented Apr 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9573196

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
clerkPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 25bf8c6 to 708db0cCompareApril 22, 2026 18:37
Base automatically changed from wyattjoh/oauth-refresh-token-support to mainApril 22, 2026 18:39
@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 708db0c to 6ecad3eCompareApril 22, 2026 18:41
@wyattjoh
wyattjoh marked this pull request as ready for review April 22, 2026 18:41
@coderabbitai

coderabbitaiBot commented Apr 22, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 39af30aa-7498-43c3-aa84-17d55a15adc3

📥 Commits

Reviewing files that changed from the base of the PR and between 0d2667c and 9573196.

📒 Files selected for processing (2)
  • packages/cli-core/src/lib/credential-store.test.ts
  • packages/cli-core/src/lib/credential-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli-core/src/lib/credential-store.ts

📝 Walkthrough

Walkthrough

Introduces getValidAccessToken to centralize token expiry checks and moves expiry/refresh decision logic out of getValidToken. Enhances refresh failure handling: on invalid_grant the code polls the credential store with short delays, re-reads the stored session, and if a newer non-expired session is found (detected via a session fingerprint mismatch) it returns that session's access token; otherwise stored credentials are deleted and a session-expired error is thrown. Adds a test that simulates a concurrent refresh where another process writes a refreshed session during recovery. A Changeset file was added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Detailed explanation

  • Adds getValidAccessToken to centralize expiry detection and refresh behavior; getValidToken delegates to it.
  • Introduces sessionFingerprint() to detect changes between reads of the stored session.
  • Implements recoverFromInvalidGrant() which performs short delays, re-reads stored credentials, and if a fingerprint mismatch reveals a newer valid session, returns that session's access token.
  • Modifies refreshStoredSession to invoke recovery on invalid_grant before deleting stored credentials and throwing a session-expired error.
  • Adds a test that simulates an expired stored session, a refresh call failing with invalid_grant, and a concurrent writer that persists a refreshed session; asserts the resolved token and stored session match the newer session.
  • Adds a Changeset metadata file to trigger a patch release documenting the concurrency behavior for refreshed OAuth credentials.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title 'fix(auth): recover from concurrent session refreshes' directly and clearly summarizes the main change—hardening the invalid_grant handling to recover from race conditions when multiple processes refresh the same expired session.
Description check✅ PassedThe description is well-detailed and clearly related to the changeset, explaining the problem, the fix strategy, implementation details, and test coverage.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


Comment @coderabbitai help to get the list of available commands and usage tips.

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 6ecad3e to 38431d4CompareApril 23, 2026 20:52

@rafa-thaytorafa-thayto left a comment

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.

LGTM with two suggestions below. The approach is solid and well-scoped. The fingerprint + retry strategy is a clean way to handle this race.

Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Prevent unbounded recursion when a newer stored session is itself
expired, and ensure recovery failures still fall through to the
delete + session_expired path so the user gets the friendly re-auth
prompt instead of a raw error.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/credential-store.ts`:
- Around line 31-32: The timed-polling invalid_grant fallback currently clears
the credential store unconditionally after the retry budget
(INVALID_GRANT_RETRY_DELAYS_MS) and can delete a freshly written session; change
the fallback to be non-destructive by verifying the store still contains the
original fingerprint before deleting or by avoiding deletion entirely and
instead marking the local attempt as failed. Concretely: capture the original
fingerprint value when you start the retry loop, then before calling the code
that clears the store (the branch that deletes credentials on invalid_grant),
re-read the credential store and compare its current fingerprint to the captured
original; only perform the destructive clear if they match, otherwise skip
deletion (or set a non-destructive failure flag/log). Ensure this check is
applied to the invalid_grant branch that references
INVALID_GRANT_RETRY_DELAYS_MS so the race is eliminated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9ff5427-60ad-42a6-a67d-b3f4650d0923

📥 Commits

Reviewing files that changed from the base of the PR and between 38431d4 and 0d2667c.

📒 Files selected for processing (1)
  • packages/cli-core/src/lib/credential-store.ts

Comment threadpackages/cli-core/src/lib/credential-store.ts
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.test.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Rename recoverFromInvalidGrant to awaitConcurrentRefresh and add a JSDoc
covering the race window, polling budget, and detection rationale. Drop
the sessionFingerprint helper in favor of comparing refresh tokens
directly, since the OAuth server rotates them on every successful
exchange. Update the test description to make the concurrent-refresh
framing explicit.
@wyattjoh
wyattjoh requested a review from jfosheeApril 28, 2026 15:19
@wyattjoh
wyattjoh merged commit 2e6d03b into mainApr 28, 2026
10 checks passed
@wyattjoh
wyattjoh deleted the wyattjoh/oauth-refresh-race-retry branch April 28, 2026 20:00
@github-actionsgithub-actionsBot mentioned this pull request Apr 28, 2026
Sign up for freeto 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.

3 participants

@wyattjoh@jfoshee@rafa-thayto
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

fix(auth): recover from concurrent session refreshes - #213

Merged
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry
Apr 28, 2026
Merged

fix(auth): recover from concurrent session refreshes#213
wyattjoh merged 4 commits into
mainfrom
wyattjoh/oauth-refresh-race-retry

Conversation

@wyattjoh

@wyattjohwyattjoh commented Apr 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #205. When two separate clerk processes start from the same expired OAuth session, they can both race into refreshStoredSession(). If one process refreshes successfully and rotates the refresh token, the loser sees invalid_grant and, in the current implementation, deletes the credentials that the winner just rewrote.

This PR keeps the existing refresh flow but hardens the invalid_grant path so a losing process re-reads the credential store before treating the session as dead. If another process already stored a newer session, the loser reuses that session instead of deleting credentials and forcing a re-auth.

Fix

  • Add a small recovery path for invalid_grant in credential-store.ts.
  • Capture the failing session's refresh token, then re-read the store with a short retry window (25ms, 50ms, 100ms) before deleting anything. A different refresh token implies a sibling process already rotated the session.
  • If a newer session appears during that window, treat it as the winner of the race and continue with its access token instead of clearing auth state.
  • Keep the existing cleanup behavior for the real expired-session case where the store never changes.
  • Add regression coverage for both paths and a patch changeset for the follow-up fix.

Test plan

  • bunx tsc -p packages/cli-core/tsconfig.json --noEmit
  • bun test packages/cli-core/src/lib/credential-store.test.ts
  • bun test packages/cli-core/src/lib/plapi.test.ts packages/cli-core/src/lib/token-exchange.test.ts packages/cli-core/src/commands/auth/login.test.ts packages/cli-core/src/commands/doctor/doctor.test.ts packages/cli-core/src/commands/whoami/index.test.ts
  • Added a regression test that simulates another process winning the refresh race and verifies the loser reuses the newer stored session.

@changeset-bot

changeset-botBot commented Apr 22, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 9573196

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
clerkPatch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 25bf8c6 to 708db0cCompareApril 22, 2026 18:37
Base automatically changed from wyattjoh/oauth-refresh-token-support to mainApril 22, 2026 18:39
@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 708db0c to 6ecad3eCompareApril 22, 2026 18:41
@wyattjoh
wyattjoh marked this pull request as ready for review April 22, 2026 18:41
@coderabbitai

coderabbitaiBot commented Apr 22, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 39af30aa-7498-43c3-aa84-17d55a15adc3

📥 Commits

Reviewing files that changed from the base of the PR and between 0d2667c and 9573196.

📒 Files selected for processing (2)
  • packages/cli-core/src/lib/credential-store.test.ts
  • packages/cli-core/src/lib/credential-store.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli-core/src/lib/credential-store.ts

📝 Walkthrough

Walkthrough

Introduces getValidAccessToken to centralize token expiry checks and moves expiry/refresh decision logic out of getValidToken. Enhances refresh failure handling: on invalid_grant the code polls the credential store with short delays, re-reads the stored session, and if a newer non-expired session is found (detected via a session fingerprint mismatch) it returns that session's access token; otherwise stored credentials are deleted and a session-expired error is thrown. Adds a test that simulates a concurrent refresh where another process writes a refreshed session during recovery. A Changeset file was added.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~35 minutes

Detailed explanation

  • Adds getValidAccessToken to centralize expiry detection and refresh behavior; getValidToken delegates to it.
  • Introduces sessionFingerprint() to detect changes between reads of the stored session.
  • Implements recoverFromInvalidGrant() which performs short delays, re-reads stored credentials, and if a fingerprint mismatch reveals a newer valid session, returns that session's access token.
  • Modifies refreshStoredSession to invoke recovery on invalid_grant before deleting stored credentials and throwing a session-expired error.
  • Adds a test that simulates an expired stored session, a refresh call failing with invalid_grant, and a concurrent writer that persists a refreshed session; asserts the resolved token and stored session match the newer session.
  • Adds a Changeset metadata file to trigger a patch release documenting the concurrency behavior for refreshed OAuth credentials.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 16.67% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Title check✅ PassedThe title 'fix(auth): recover from concurrent session refreshes' directly and clearly summarizes the main change—hardening the invalid_grant handling to recover from race conditions when multiple processes refresh the same expired session.
Description check✅ PassedThe description is well-detailed and clearly related to the changeset, explaining the problem, the fix strategy, implementation details, and test coverage.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

Warning

Review ran into problems

🔥 Problems

Timed out fetching pipeline failures after 30000ms


Comment @coderabbitai help to get the list of available commands and usage tips.

@wyattjoh
wyattjohforce-pushed the wyattjoh/oauth-refresh-race-retry branch from 6ecad3e to 38431d4CompareApril 23, 2026 20:52

@rafa-thaytorafa-thayto left a comment

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.

LGTM with two suggestions below. The approach is solid and well-scoped. The fingerprint + retry strategy is a clean way to handle this race.

Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Prevent unbounded recursion when a newer stored session is itself
expired, and ensure recovery failures still fall through to the
delete + session_expired path so the user gets the friendly re-auth
prompt instead of a raw error.

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli-core/src/lib/credential-store.ts`:
- Around line 31-32: The timed-polling invalid_grant fallback currently clears
the credential store unconditionally after the retry budget
(INVALID_GRANT_RETRY_DELAYS_MS) and can delete a freshly written session; change
the fallback to be non-destructive by verifying the store still contains the
original fingerprint before deleting or by avoiding deletion entirely and
instead marking the local attempt as failed. Concretely: capture the original
fingerprint value when you start the retry loop, then before calling the code
that clears the store (the branch that deletes credentials on invalid_grant),
re-read the credential store and compare its current fingerprint to the captured
original; only perform the destructive clear if they match, otherwise skip
deletion (or set a non-destructive failure flag/log). Ensure this check is
applied to the invalid_grant branch that references
INVALID_GRANT_RETRY_DELAYS_MS so the race is eliminated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c9ff5427-60ad-42a6-a67d-b3f4650d0923

📥 Commits

Reviewing files that changed from the base of the PR and between 38431d4 and 0d2667c.

📒 Files selected for processing (1)
  • packages/cli-core/src/lib/credential-store.ts

Comment threadpackages/cli-core/src/lib/credential-store.ts
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.test.ts Outdated
Comment threadpackages/cli-core/src/lib/credential-store.ts Outdated
Rename recoverFromInvalidGrant to awaitConcurrentRefresh and add a JSDoc
covering the race window, polling budget, and detection rationale. Drop
the sessionFingerprint helper in favor of comparing refresh tokens
directly, since the OAuth server rotates them on every successful
exchange. Update the test description to make the concurrent-refresh
framing explicit.
@wyattjoh
wyattjoh requested a review from jfosheeApril 28, 2026 15:19
@wyattjoh
wyattjoh merged commit 2e6d03b into mainApr 28, 2026
10 checks passed
@wyattjoh
wyattjoh deleted the wyattjoh/oauth-refresh-race-retry branch April 28, 2026 20:00
@github-actionsgithub-actionsBot mentioned this pull request Apr 28, 2026
Sign up for freeto 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.

3 participants

@wyattjoh@jfoshee@rafa-thayto