Skip to content

OUT-3621: proactive token refresh with race-safe invalid_grant handling - #231

Merged
SandipBajracharya merged 4 commits into
masterfrom
OUT-3621
Apr 22, 2026
Merged

OUT-3621: proactive token refresh with race-safe invalid_grant handling#231
SandipBajracharya merged 4 commits into
masterfrom
OUT-3621

Conversation

@SandipBajracharya

@SandipBajracharya SandipBajracharya commented Apr 21, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Replaces two duplicated isExpired blocks with a single getValidQbTokens(portalId) helper that proactively refreshes when the stored token is within REFRESH_BUFFER_SECONDS (15 min) of expiry.
  • On invalid_grant, getRefreshedQbTokenInfo distinguishes a cross-worker refresh race (DB tokenSetTime advanced) from genuine revocation by re-reading the row — clock-skew-free.
  • Genuine revocation throws a typed QBReconnectRequiredError without mutating state. AuthService.getQBPortalConnection owns the paired disable-sync + notify-IU side effects so they only fire under a Copilot user context.

Test plan

  • yarn test — 50 tests across 3 files, all passing (10 new in tokenRefresh.test.ts).
  • yarn tsc --noEmit — clean (pre-existing unrelated errors filtered).
  • Manual: connect a sandbox portal, force an expired token (or wait 1h), confirm the next webhook proactively refreshes without 401 noise.
  • Manual: revoke a sandbox portal in Intuit admin, fire a webhook, confirm syncFlag flips to false and the IU receives the reconnect notification once.
  • Manual: load the dashboard for a portal whose token is revoked — UI should not crash; checkForNonUsCompany returns false silently.

Linear

OUT-3621

🤖 Generated with Claude Code

…andling

Replaces the reactive 401 approach with proactive refresh via a 10-min buffer
(`getValidQbTokens`), removing the two duplicated `isExpired` blocks. On
`invalid_grant`, `getRefreshedQbTokenInfo` distinguishes a cross-worker race
(via `tokenSetTime` re-read) from genuine revocation and throws a typed
`QBReconnectRequiredError` without mutating state. `AuthService` owns the
paired disable-sync + notify-IU side effects so they stay together under a
user context. Also renames the CI workflow file.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@linear-code

linear-code Bot commented Apr 21, 2026

Copy link
Copy Markdown

@vercel

vercel Bot commented Apr 21, 2026

Copy link
Copy Markdown

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

Project Deployment Actions Updated (UTC)
quickbooks-sync Ready Ready Preview, Comment Apr 22, 2026 9:49am
quickbooks-sync (dev) Ready Ready Preview, Comment Apr 22, 2026 9:49am

Request Review

@greptile-apps

greptile-apps Bot commented Apr 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR consolidates duplicated token-expiry checks into a single getValidQbTokens helper with a proactive 10-minute refresh buffer, and adds a handleInvalidGrant function that distinguishes a cross-worker refresh race (DB tokenSetTime advanced) from genuine revocation using a clock-skew-free DB comparison. QBReconnectRequiredError cleanly separates the signal from the side effects, keeping the disable-sync + IU-notify pair exclusively in AuthService.getQBPortalConnection.

Confidence Score: 5/5

Safe to merge — all findings are P2 style/efficiency suggestions; the race-detection and revocation-handling logic is correct and well-tested.

The core handleInvalidGrant race-vs-revocation logic is sound across all edge cases (null tokenSetTime, same time, advanced time). The isTokenFresh buffer comparison is directionally correct. Tests cover the critical state-mutation invariant. The two flagged items (double DB read and info-level revocation log) are non-blocking style improvements.

No files require special attention; tokenRefresh.ts has the double-read efficiency note worth a follow-up but it does not affect correctness.

Important Files Changed

Filename Overview
src/utils/tokenRefresh.ts Core of the PR — introduces getValidQbTokens, QBReconnectRequiredError, and race-vs-revocation logic in handleInvalidGrant. Logic is sound; one P2: getRefreshedQbTokenInfo redundantly re-reads the DB row that getValidQbTokens already fetched.
src/action/quickbooks.action.ts Replaces duplicated inline token-expiry logic with getValidQbTokens; wraps checkForNonUsCompany in a try/catch that swallows QBReconnectRequiredError. One P2: revocation logged at info rather than warn.
src/app/api/quickbooks/auth/auth.service.ts Replaces inline expiry check in getQBPortalConnection with getValidQbTokens; adds correct QBReconnectRequiredError handler that queues IU notification and disables sync. Side-effect ordering and error-handling contract look correct.
test/unit/utils/tokenRefresh.test.ts 10 new unit tests covering proactive buffer, race recovery, genuine revocation (no-state-mutation contract), non-invalid_grant propagation, and missing connection. Well-commented; covers the critical invariants.
.github/workflows/test.yml Renamed from ci.yml to test.yml — no content changes.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant getValidQbTokens
    participant DB
    participant getRefreshedQbTokenInfo
    participant Intuit
    participant handleInvalidGrant

    Caller->>getValidQbTokens: portalId
    getValidQbTokens->>DB: getPortalConnection
    DB-->>getValidQbTokens: row

    alt More than 10 min remaining on token
        getValidQbTokens-->>Caller: stored tokens (no refresh)
    else Within 10-min buffer or no tokenSetTime
        getValidQbTokens->>getRefreshedQbTokenInfo: portalId
        getRefreshedQbTokenInfo->>DB: getPortalConnection (2nd read, captures T1)
        getRefreshedQbTokenInfo->>Intuit: getRefreshedQBToken

        alt Intuit responds with new tokens
            Intuit-->>getRefreshedQbTokenInfo: new access + refresh tokens
            getRefreshedQbTokenInfo->>DB: UPDATE set tokenSetTime to now
            getRefreshedQbTokenInfo-->>Caller: new tokens
        else Intuit responds with invalid_grant
            Intuit-->>getRefreshedQbTokenInfo: invalid_grant
            getRefreshedQbTokenInfo->>handleInvalidGrant: portalId, startingTime T1
            handleInvalidGrant->>DB: getPortalConnection (re-read, gets T2)

            alt T2 is after T1 — concurrent worker already refreshed
                handleInvalidGrant-->>Caller: winner tokens from DB, no write
            else T2 equals T1 — genuine revocation
                handleInvalidGrant-->>Caller: throws QBReconnectRequiredError
            end
        end
    end

    Note over Caller: AuthService.getQBPortalConnection catches QBReconnectRequiredError
    Note over Caller: Queues IU notification via afterIfAvailable
    Note over Caller: Awaits turnOffSync, returns empty tokens
Loading

Reviews (1): Last reviewed commit: "fix(OUT-3621): proactive token refresh w..." | Re-trigger Greptile

Comment thread src/utils/tokenRefresh.ts
Comment on lines +85 to +96
export async function getValidQbTokens(
portalId: string,
): Promise<IntuitAPITokensType> {
const row = await getPortalConnection(portalId)
if (!row) {
throw new Error(
`getValidQbTokens | Portal connection not found for portalId: ${portalId}`,
)
}
if (isTokenFresh(row)) return extractTokens(row)
return getRefreshedQbTokenInfo(portalId)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Extra DB read on the stale-token path

getValidQbTokens already fetches and checks the row on line 88, but when the token is stale it discards that result and calls getRefreshedQbTokenInfo(portalId) (line 95), which immediately does a second getPortalConnection call (line 111) to populate startingTokenSetTime. The two reads create a small race window: if another worker refreshes between them, the second read will capture that worker's tokenSetTime as startingTokenSetTime, causing this worker to attempt a redundant Intuit API call with the freshly rotated refresh token. The race-detection logic will eventually recover correctly, but the extra round-trip and the unnecessary Intuit call could be avoided entirely by accepting the pre-fetched row as a parameter and skipping the second DB query.

Comment thread src/action/quickbooks.action.ts
…loop

intiateSync runs inside trigger.dev (no Vercel function timeout), so a long
backlog of failed logs for one portal can outrun the 15-min proactive refresh
buffer when we fetch tokens once upfront. Calling getQBPortalConnection per
iteration keeps each log's processing inside a fresh-token window, and
breaking on emptyTokens halts cleanly if a prior iteration's invalid_grant
cascade disabled sync.

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

getRefreshedQbTokenInfo used to re-read the portal row to snapshot
startingTokenSetTime. If a concurrent worker refreshed between the caller's
freshness check and this re-read, the snapshot captured the winner's
tokenSetTime — and a subsequent invalid_grant from Intuit would find
tokenSetTime unchanged on re-read, misdiagnosing the race as a genuine
revocation (wrongly flipping syncFlag=false and notifying the IU).

Accept an optional prefetchedConnection parameter so callers that already
read the row pass it through; the race-detection baseline now matches what
they observed. Direct callers (CLI) fall back to a fresh read unchanged.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@SandipBajracharya SandipBajracharya changed the title fix(OUT-3621): proactive token refresh with race-safe invalid_grant handling OUT-3621: proactive token refresh with race-safe invalid_grant handling Apr 21, 2026

@priosshrsth priosshrsth left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lgtm

Widens the proactive-refresh window to better absorb long-running sync
jobs, queue latency, and clock skew against Intuit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@SandipBajracharya
SandipBajracharya merged commit 2bfb253 into master Apr 22, 2026
4 checks passed
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.

2 participants