Skip to content

fix: auth refactor — eliminate refresh race, increase buffer, add error semantics - #194

Merged
AjayThorve merged 6 commits into
NVIDIA-AI-Blueprints:developfrom
exactlyallan:aiq_UI-cache-bug
Apr 27, 2026
Merged

fix: auth refactor — eliminate refresh race, increase buffer, add error semantics#194
AjayThorve merged 6 commits into
NVIDIA-AI-Blueprints:developfrom
exactlyallan:aiq_UI-cache-bug

Conversation

@exactlyallan

@exactlyallan exactlyallan commented Apr 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

This PR addresses three of the five root causes of persistent auth failures documented in the auth flow analysis. It is the first PR of a multi-phase auth refactor.

Commit 1: Eliminate dual refresh race + increase buffer (Phases 1 & 2)

  • Eliminate dual refresh race condition: Removed the duplicate setInterval in useAuth that fired concurrently with SessionProvider.refetchInterval. With rotating refresh tokens (e.g., NVIDIA Starfleet SSO), concurrent refreshes consumed an already-invalidated token → invalid_grant → random logouts.
  • Config-driven refresh interval: SessionProvider.refetchInterval now uses config.sessionRefreshIntervalSeconds instead of hardcoded 4 * 60.
  • Increase default refresh buffer: TOKEN_REFRESH_BUFFER_MINUTES default changed from 5 → 15 minutes (covering most deep research operations).

Commit 2: Machine-readable auth error codes + RUM observability (Phase 3)

  • validate_with_error() on TokenValidator: New method returns (user, error_code) instead of just user | None. Default wraps validate() for backward compatibility.
  • JWTValidator override: ExpiredSignatureError"token_expired", InvalidTokenError"token_invalid".
  • Middleware 401 responses now include "error" field: "token_missing", "token_expired", "token_invalid" — frontends can distinguish "refresh" from "reauthenticate".
  • TokenExpiredError / TokenInvalidError subclasses of AuthError with error_code attribute.
  • WebSocket error propagation: AuthError caught in workflow handler → sends typed "auth_error" message over WS.
  • Datadog RUM integration: authenticatedFetch and WebSocket client emit DD_RUM.addError() on auth failures (no-op without Datadog SDK).

Files changed

Frontend (frontends/ui/src/)

File Change
adapters/auth/session.ts Remove duplicate setInterval, add explanatory comment
adapters/auth/config.ts Increase default buffer from 5 → 15 minutes
app/providers.tsx Use config-driven refetchInterval
adapters/api/authenticated-fetch.ts Add RUM error tracking on 401 responses
adapters/api/websocket-client.ts Add RUM tracking for WS auth errors
.env.example, README.md Update defaults
adapters/auth/config.spec.ts Update default expectations
adapters/auth/session.spec.tsx Add regression test (no setInterval)

Backend (frontends/aiq_api/src/aiq_api/)

File Change
auth/base.py Add validate_with_error() default method
auth/jwt_validator.py Override with expired/invalid distinction
auth/middleware.py Return error code in 401 JSON body
auth/errors.py Add TokenExpiredError, TokenInvalidError
auth/__init__.py Export new error types
websocket_reconnect.py Auth error messages over WebSocket
tests/test_auth.py 10 new tests + fix flaky kid cache test

Test plan

  • npm run test:ci — 1183 frontend tests pass
  • pytest test_auth.py — 39 backend tests pass (10 new)
  • Pre-commit hooks pass (ruff, formatting, secrets detection)
  • Manual: verify 401 response includes "error": "token_expired" for expired JWT
  • Manual: verify 401 response includes "error": "token_invalid" for bad JWT
  • Manual: with REQUIRE_AUTH=true, verify single refresh path in Network tab
  • Manual: with REQUIRE_AUTH=false, verify app loads with no auth UI

🤖 Generated with Claude Code

Two independent refresh mechanisms (SessionProvider.refetchInterval and
a setInterval in useAuth) fired concurrently every ~4 minutes, causing
"invalid_grant" failures with OAuth providers that use rotating refresh
tokens (e.g., NVIDIA Starfleet SSO). The second concurrent refresh
consumed an already-invalidated token and killed the session.

Remove the duplicate setInterval from useAuth — session refresh is now
handled solely by SessionProvider's refetchInterval. Make the interval
config-driven (from TOKEN_REFRESH_BUFFER_SECONDS) instead of hardcoded.

Also increase the default TOKEN_REFRESH_BUFFER_MINUTES from 5 to 15.
The previous 5-minute window was insufficient for deployments running
long operations (deep research with ECI runs 20-40+ minutes). Enterprise
deployments should set TOKEN_REFRESH_BUFFER_MINUTES=30 via env var.

Includes a regression test that verifies useAuth never creates its own
setInterval, preventing re-introduction of the race condition.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Apr 16, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR is the first phase of an auth refactor addressing three root causes of auth failures: it removes the duplicate setInterval in useAuth that raced with SessionProvider.refetchInterval on rotating-token providers, increases the default token refresh buffer from 5 → 15 minutes, and adds machine-readable error codes ("token_missing", "token_expired", "token_invalid") to 401 responses, WebSocket error messages, and Datadog RUM events. The validate() abstract method now returns (user_dict, error_code) tuples with full backward-compatibility handling in the middleware.

Confidence Score: 5/5

Safe to merge; only P2 findings, all on non-critical observability paths

The core auth fixes (race condition elimination, buffer increase, error code propagation) are well-implemented and thoroughly tested. Both findings are P2: one is a narrow RUM tracking guard that misses specific subclass codes (no functional regression), the other is dead code complexity. No P0/P1 issues found.

No files require special attention. middleware.py has the dead validate_with_error branch worth cleaning up, but it has no runtime impact.

Important Files Changed

Filename Overview
frontends/aiq_api/src/aiq_api/auth/middleware.py Refactored to propagate machine-readable error codes in 401 responses; backward-compat validator detection includes a dead validate_with_error branch that is never reached with current validators
frontends/ui/src/adapters/api/websocket-client.ts Adds RUM tracking for WS auth errors, but the guard only matches the base 'auth_error' code, silently missing 'token_expired' and 'token_invalid' subclass codes
frontends/aiq_api/src/aiq_api/auth/jwt_validator.py All return paths updated to tuples; ExpiredSignatureError"token_expired", InvalidTokenError"token_invalid", consistent with the new contract
frontends/aiq_api/src/aiq_api/auth/errors.py Adds TokenExpiredError and TokenInvalidError subclasses with error_code class attribute; clean hierarchy
frontends/aiq_api/src/aiq_api/websocket_reconnect.py Auth errors during workflow are now caught and surfaced as typed WS error messages; non-auth exceptions still log and return as before
frontends/ui/src/adapters/auth/session.ts Duplicate setInterval removed; explanatory comment added; RUM error emitted before signOut() redirect
frontends/ui/src/app/providers.tsx refetchInterval now uses config-driven sessionRefreshIntervalSeconds instead of hardcoded 4 * 60
frontends/ui/src/shared/utils/rum.ts New RUM helper; correctly guards for window existence (SSR safe) and optional DD_RUM global; no-op without Datadog SDK
frontends/ui/src/adapters/api/authenticated-fetch.ts Uses response.clone().json() correctly to read body without consuming the original response; RUM tracking on 401 is well-guarded
frontends/aiq_api/tests/test_auth.py 10 new tests covering error subclasses, error code propagation through middleware, and missing/expired/invalid token scenarios; flaky kid-cache test fixed
frontends/ui/src/adapters/auth/config.ts Default TOKEN_REFRESH_BUFFER_MINUTES increased from 5 → 15; change is reflected consistently in spec, .env.example, and README

Sequence Diagram

sequenceDiagram
    participant FE as Frontend
    participant SP as SessionProvider
    participant MW as AuthMiddleware
    participant JV as JWTValidator
    participant RUM as Datadog RUM

    Note over SP: refetchInterval = config.sessionRefreshIntervalSeconds<br/>(no duplicate setInterval in useAuth)
    SP->>MW: GET /session (Bearer token)
    MW->>JV: validate_token_with_error(token)
    alt Token valid
        JV-->>MW: (user_dict, None)
        MW-->>SP: 200 OK
    else Token expired
        JV-->>MW: (None, "token_expired")
        MW-->>FE: 401 {error: "token_expired"}
        FE->>RUM: trackRumError("Auth failure: token_expired")
    else Token invalid
        JV-->>MW: (None, "token_invalid")
        MW-->>FE: 401 {error: "token_invalid"}
        FE->>RUM: trackRumError("Auth failure: token_invalid")
    else No token
        MW-->>FE: 401 {error: "token_missing"}
    end

    Note over FE: WebSocket path
    FE->>MW: WS connect (AuthError raised in workflow)
    MW-->>FE: ERROR msg {message: exc.error_code}
    FE->>RUM: trackRumError("WebSocket auth failure")
Loading

Reviews (6): Last reviewed commit: "Merge upstream develop into auth refacto..." | Re-trigger Greptile

@exactlyallan
exactlyallan requested a review from AjayThorve April 16, 2026 19:27
@exactlyallan exactlyallan self-assigned this Apr 16, 2026
@exactlyallan exactlyallan added the bug Something isn't working label Apr 16, 2026
@exactlyallan exactlyallan changed the title fix: eliminate dual token refresh race and increase refresh buffer fix: eliminate dual token refresh race and increase refresh buffer - part 1 of 5 Apr 16, 2026
The backend previously returned the same 401 "Invalid or expired auth
token" for all failure types. Frontends had no way to distinguish
"refresh your token" from "reauthenticate completely", contributing to
silent failure modes.

Backend changes:
- Add validate_with_error() to TokenValidator base class (backward-
  compatible default wraps validate())
- Override in JWTValidator: ExpiredSignatureError -> "token_expired",
  InvalidTokenError -> "token_invalid"
- Middleware 401 responses now include an "error" field with machine-
  readable codes: "token_missing", "token_expired", "token_invalid"
- Add TokenExpiredError/TokenInvalidError subclasses of AuthError
- WebSocket handler catches AuthError and sends typed "auth_error"
  message before closing, so frontends get actionable feedback

Frontend changes:
- authenticatedFetch emits Datadog RUM custom errors on 401 responses
  with the error code as a facet (no-op without Datadog SDK)
- WebSocket client emits RUM events for auth_error messages

Also fixes a pre-existing flaky test (test_matches_key_by_kid) caused
by stale JWKS cache timestamps on long-uptime machines.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@exactlyallan exactlyallan changed the title fix: eliminate dual token refresh race and increase refresh buffer - part 1 of 5 fix: auth refactor — eliminate refresh race, increase buffer, add error semantics Apr 16, 2026
TypeScript strict mode rejects `window as Record<string, unknown>`
because Window's type doesn't have an index signature. Cast through
`unknown` first: `window as unknown as Record<string, unknown>`.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Comment thread frontends/aiq_api/src/aiq_api/auth/jwt_validator.py Outdated
exactlyallan and others added 3 commits April 16, 2026 16:16
Per review feedback, consolidate the two-method pattern into a single
validate() that returns (user_dict, error_code) directly. There are no
external consumers of the old dict|None signature — StarfleetValidator
and NVAuthValidator in aiq-bp-internal will be updated in a coordinated
internal MR.

- TokenValidator.validate() now returns tuple[dict | None, str | None]
- Remove validate_with_error() from base class and JWTValidator
- Middleware calls validate() directly (no _validate_token_with_error)
- Update all tests for new return type

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

The DD_RUM.addError() calls added in the previous commit were dead code
for the primary auth failure path: token refresh failures are detected
server-side in the JWT callback, propagated via the session object, and
then signOut() immediately redirects — no RUM emission happened before
the page unloaded.

Add trackRumError() call in the useAuth hook BEFORE handleSignOut() so
the event is buffered before navigation starts. The Datadog SDK's
sendBeacon flushes buffered events on page unload.

Also extract a shared trackRumError() utility to DRY up the DD_RUM
access pattern (was duplicated with type casts in 3 files).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve conflicts with the current request-auth helper extraction. Preserve machine-readable auth error codes through shared HTTP/WebSocket identity resolution and keep SessionProvider as the single refresh scheduler.

@AjayThorve AjayThorve left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, tested locally, no regressions to the middleware

@AjayThorve
AjayThorve merged commit 0d53241 into NVIDIA-AI-Blueprints:develop Apr 27, 2026
12 of 13 checks passed
taylorjordanNC pushed a commit to taylorjordanNC/rh-research that referenced this pull request May 27, 2026
…or semantics (NVIDIA-AI-Blueprints#194)

* fix: eliminate dual token refresh race and increase refresh buffer

Two independent refresh mechanisms (SessionProvider.refetchInterval and a setInterval in useAuth) fired concurrently every ~4 minutes, causing "invalid_grant" failures with OAuth providers that use rotating refresh tokens (e.g., NVIDIA Starfleet SSO). The second concurrent refresh consumed an already-invalidated token and killed the session.

Remove the duplicate setInterval from useAuth — session refresh is now handled solely by SessionProvider's refetchInterval. Make the interval config-driven (from TOKEN_REFRESH_BUFFER_SECONDS) instead of hardcoded.

Also increase the default TOKEN_REFRESH_BUFFER_MINUTES from 5 to 15. The previous 5-minute window was insufficient for deployments running long operations (deep research with ECI runs 20-40+ minutes). Enterprise
deployments should set TOKEN_REFRESH_BUFFER_MINUTES=30 via env var.

Includes a regression test that verifies useAuth never creates its own setInterval, preventing re-introduction of the race condition.

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants