Skip to content

fix(oauth): rotate a refresh chain only from the token it started from and size the refresh lease for every provider - #8152

Merged
waleedlatif1 merged 2 commits into
stagingfrom
fix/oauth-refresh-conditional-write
Sep 22, 2026
Merged

waleedlatif1 merged 2 commits into
stagingfrom
fix/oauth-refresh-conditional-write

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • The coalesced OAuth refresh serialized concurrent refreshes with a Redis lease, but a lease is not mutual exclusion: it can expire under a slow provider or a paused process while the leader is still running. The rotated token write was unconditional for every provider except Slack, so a refresh that outlived its lease could overwrite a newer rotation with a chain the provider had already retired; the next refresh then fails with invalid_grant, and under reuse detection the whole grant is revoked
  • The write now rotates the chain only from the refresh token the refresh started from (WHERE id = … AND refresh_token = <the one used> with RETURNING). When no row matches, another writer rotated first: the leader logs it, re-reads the row and returns the stored token, and never retries the provider. This generalizes the version-guarded write the Slack installation path already had
  • The lease and follower budgets were sized for the provider call only on the Slack path (30 s / 30 s); every other provider still used the helper defaults (10 s lease, 3 s follower wait) against a 15 s provider timeout. A refresh slower than 3 s made every concurrent caller fail with "no access token" even though the leader succeeded moments later, and a refresh slower than 10 s let a second leader start a competing rotation. Both budgets are now derived from the exported provider timeout plus headroom and apply to every provider

Type of Change

  • Bug fix

Testing

  • New tests: the lease and follower budgets are passed for a non-Slack provider; the rotated write is guarded by the starting refresh token; a lost write returns the stored chain's token without a second provider call. Removing the guard fails two of them, removing the budgets fails the third
  • Existing refresh, Slack, Microsoft, Instagram and QuickBooks tests unchanged in intent; local update-chain mocks gained a returning() shape
  • bun run lint, check:audits (47 audits), docs-manifest:check, type-check pass; 1,482 OAuth and connector tests pass

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…m and size the refresh lease for every provider
@vercel

vercel Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
docs Skipped Skipped Sep 22, 2026 8:21pm UTC

Request Review

@greptile-apps

greptile-apps Bot commented Sep 22, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 5/5

The PR appears safe to merge; both previous concurrency findings are fixed in the current code and no new actionable issue was identified.

Summary

The PR hardens OAuth refresh concurrency and completes the fixes requested in the previous review.

  • Sizes refresh leases and follower waits beyond the provider timeout for every provider.
  • Uses a refresh-token compare-and-swap to prevent stale leaders from overwriting newer token chains.
  • Re-reads the stored chain after lost writes or terminal failures and returns only a usable persisted token.
  • Returns no token if the account was concurrently deleted and avoids marking a newer chain dead.
Diagram
sequenceDiagram
  participant Caller
  participant Lock as Refresh lease
  participant Provider
  participant DB

  Caller->>Lock: Acquire lease
  Lock-->>Caller: Leader
  Caller->>Provider: Refresh using starting token
  Provider-->>Caller: Rotated token chain
  Caller->>DB: "UPDATE ... WHERE refresh_token = starting token"
  alt Update matched
    DB-->>Caller: Rotated row
    Caller-->>Caller: Return refreshed token
  else Newer writer won
    DB-->>Caller: No rows updated
    Caller->>DB: Read current chain
    alt Account exists and token is usable
      DB-->>Caller: Winner chain
      Caller-->>Caller: Return stored winner token
    else Account deleted or token unusable
      Caller-->>Caller: Return no token
    end
  end
Loading

Reviews (2) · Last reviewed commit: "fix(oauth): skip the dead flag when the ..."

Comment thread apps/sim/lib/oauth/credential-service.ts
Comment thread apps/sim/lib/oauth/credential-service.ts Outdated

@cubic-dev-ai cubic-dev-ai Bot 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.

No issues found across 5 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cubic-dev-ai review this PR

@cubic-dev-ai

cubic-dev-ai Bot commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

@cubic-dev-ai review this PR

@waleedlatif1 I have started the AI code review. It will take a few minutes to complete.

@cubic-dev-ai cubic-dev-ai Bot 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.

No issues found across 5 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

@waleedlatif1
waleedlatif1 merged commit 93fdd13 into staging Sep 22, 2026
34 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/oauth-refresh-conditional-write branch September 22, 2026 20:26

This branch was previously deployed

1 inactive deployment
Preview — 2c38245e Deployed Sep 22, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant