fix(ui): unblock cold-load 401 refresh (renewer registration race) by chirag-madlani · Pull Request #31644 · open-metadata/OpenMetadata

chirag-madlani · 2026-08-17T13:58:23Z

Summary

Follow-up to #31597. Production users on confidential + Google (and any other provider on a slow first-load) still see 401 Expired token! on cold tab reopen with no /api/v1/auth/refresh call ever firing — they land on /signin after ~10 s hang.

The first hotfix (#31597) added TokenService.awaitRenewerReady so the axios 401 interceptor's refresh call could wait briefly for the renewer to be registered, but it never addressed the underlying race that made the wait pointless.

Root cause

The renewer was registered by a useEffect in AuthProvider.tsx with deps [authenticatorRef.current?.renewIdToken] — a ref access. Ref changes don't schedule React re-renders. If the lazy authenticator chunk finishes loading after AuthProvider's first render pass (which is the norm on cold-load), the effect never re-runs, TokenService.renewToken stays null, awaitRenewerReady times out at 10 s, fetchNewToken returns null, and the interceptor's else branch force-logs the user out. No HTTP /refresh call is ever fired — matches the reported symptom.

The race was timing-dependent. Repeat visits (chunk cached) usually won; first-load / cache-busted deploys lose. Recent perf work that shrank config-fetch latency or grew auth-chunk size likely tipped it into "usually loses" in production.

Fix

Move renewer registration out of AuthProvider.tsx's ref-deps useEffect and into each authenticator wrapper's own mount effect. useImperativeHandle runs before the same component's own useEffect, so registration happens the instant the wrapper mounts — no race, no dependence on the parent re-rendering.

Every authenticator (BasicAuthAuthenticator, GenericAuthenticator, OidcAuthenticator, MsalAuthenticator, OktaAuthenticator, Auth0Authenticator) now does:

useEffect(() => {
  TokenService.getInstance().updateRenewToken(<its own renewer>);
  return () => TokenService.getInstance().updateRenewToken(null);
}, [<memoized renewer>]);

The parent-side effect is gone. updateRefreshSuccessCallback(startTokenExpiryTimer) moves to AuthProvider's main mount effect (that callback lives in its closure).

Files changed

AppAuthenticators/BasicAuthAuthenticator.tsx — added mount effect, useEffect import
AppAuthenticators/GenericAuthenticator.tsx — added mount effect, wrapped handleSilentSignIn in useCallback
AppAuthenticators/OidcAuthenticator.tsx — added mount effect, wrapped signInSilently in useCallback
AppAuthenticators/MsalAuthenticator.tsx — added mount effect, wrapped renewIdToken in useCallback
AppAuthenticators/OktaAuthenticator.tsx — added mount effect, wrapped renewToken in useCallback
AppAuthenticators/Auth0Authenticator.tsx — extracted renewIdToken out of useImperativeHandle inline, wrapped in useCallback, added mount effect
AuthProviders/AuthProvider.tsx — removed the ref-deps effect, moved updateRefreshSuccessCallback into the main mount effect
utils/Auth/TokenService/TokenServiceUtil.ts — widened RenewTokenCallback to () => Promise<unknown> (so each provider's SDK-native return shape fits without a cast) and let updateRenewToken accept null for cleanup

Tests

5 new regression tests (one per authenticator that has a test file on main):

it('registers a renewer with TokenService on mount and unregisters on unmount', ...)

Each mocks TokenService.updateRenewToken, mounts the authenticator, asserts the mount effect registered a function, then unmounts and asserts updateRenewToken(null) fired.

yarn jest src/components/Auth src/utils/Auth → 138/138 pass (5 new + 133 pre-existing)
yarn prettier / yarn eslint — 0 errors
npx tsc --noEmit — 0 new errors (only pre-existing MSAL readonly-tuple and ROUTES literal-type warnings that exist on main)

Non-goals

No changes to the axios interceptor or awaitRenewerReady — those are strict no-ops now on the happy path (renewer already registered when the first 401 lands).
No signup / login / logout flow changes.
No SDK version bumps or storage schema changes.

Manual smoke matrix

Per-provider cold-load / tab-reopen scenarios reviewers can tick off:

Non-regression check

Active-tab mid-session refresh should be unchanged (renewer already registered by first 401). Quick check: log in, wait past id-token TTL while the tab is active, click something — refresh fires and the app stays authenticated.

Follow-up

This fix will be superseded by the AuthCoordinator refactor in the follow-up PR (azure-oidc-session-invalidated-on-restart), which already applies the same fix by construction (Phase 2a of that branch).

🤖 Generated with Claude Code

Greptile Summary

The PR moves token-renewer registration into each authenticator’s lifecycle so cold-load 401 handling can refresh after lazy authenticators mount.

Registers and unregisters renewers in all authenticator wrappers.
Memoizes provider-specific renewal callbacks.
Broadens TokenService’s renewal callback return type and permits cleanup with null.
Relocates refresh-success timer callback registration into AuthProvider’s mount effect.

Confidence Score: 4/5

The PR is not yet safe to merge because the outstanding refresh-success callback still captures initial timer and authentication state.

The mount-only registration retains the first-render startTokenExpiryTimer closure, so later refresh successes can bypass the Basic/LDAP refresh-token guard and create replacement timers without clearing the active timer.

Files Needing Attention: openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx

Important Files Changed

Filename	Overview
openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx	Removes parent ref-based renewer registration and registers the refresh-success timer callback during the main mount effect.
openmetadata-ui/src/main/resources/ui/src/utils/Auth/TokenService/TokenServiceUtil.ts	Generalizes renewer return typing and allows authenticator cleanup to clear the registered callback.
openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.tsx	Registers the Basic/LDAP renewal callback directly from the authenticator lifecycle.
openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/GenericAuthenticator.tsx	Memoizes and lifecycle-registers the confidential and SAML renewal callback.
openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OidcAuthenticator.tsx	Memoizes and lifecycle-registers silent OIDC renewal.
openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx	Memoizes and lifecycle-registers the MSAL renewal callback.
openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OktaAuthenticator.tsx	Memoizes and lifecycle-registers the Okta renewal callback.
openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/Auth0Authenticator.tsx	Extracts, memoizes, and lifecycle-registers the Auth0 renewal callback.

Sequence Diagram

sequenceDiagram
  participant AP as AuthProvider
  participant Auth as Lazy Authenticator
  participant TS as TokenService
  participant API as Auth API
  AP->>TS: Register refresh-success callback
  Auth->>TS: Register provider renewer on mount
  API-->>TS: 401 response
  TS->>Auth: Invoke registered renewer
  Auth->>API: Refresh token
  API-->>Auth: Refreshed credentials
  TS->>AP: Invoke timer restart callback

_{Reviews (2): Last reviewed commit: "Merge branch 'main' into hotfix/renewer-..." | Re-trigger Greptile}

…d 401 refresh Follow-up to #31597. The hotfix added TokenService.awaitRenewerReady so the axios 401 interceptor's refresh call could wait briefly for the renewer to be registered, but the underlying race — the ref-deps useEffect that owned the registration — was never addressed. On any provider where the lazy authenticator finishes mounting AFTER the first /users/loggedInUser call has already 401'd (confidential + Google is where this reliably reproduces, but every provider is susceptible on cold-load with a slow chunk), the useEffect never re-runs (ref changes don't schedule re-renders), TokenService.renewToken stays null, awaitRenewerReady times out at 10s, and the interceptor force-logs the user out — no /api/v1/auth/refresh call is ever fired. Fix: each authenticator wrapper (BasicAuthAuthenticator, GenericAuthenticator, OidcAuthenticator, MsalAuthenticator, OktaAuthenticator, Auth0Authenticator) now calls TokenService.updateRenewToken from its own mount useEffect. That effect runs the moment useImperativeHandle populates the ref, with no dependence on the parent re-rendering — no race, deterministic registration. AuthProvider.tsx: the ref-deps useEffect is gone. updateRefreshSuccessCallback(startTokenExpiryTimer) moves to the main mount effect (that callback lives in AuthProvider's closure). TokenService.updateRenewToken now accepts `null` on cleanup, and RenewTokenCallback widens to `() => Promise<unknown>` so each provider's renewer can return its own SDK-native shape (Auth0 RenewTokenResponse, MSAL AuthenticationResult, oidc-client User, …) without a per-provider cast at the registration call site. Renewers wrapped in useCallback where they weren't already (GenericAuthenticator handleSilentSignIn, MsalAuthenticator renewIdToken, OktaAuthenticator renewToken, Auth0Authenticator renewIdToken extracted from useImperativeHandle) so the register effect's dep is stable and doesn't thrash. Tests: each authenticator's test file now mocks TokenService via jest.mock and asserts (a) the mount effect calls updateRenewToken with a function, and (b) unmount calls updateRenewToken(null). 5 new regression tests, 138/138 total across the auth surface. No behavior change on the happy path (renewer already registered in the same render cycle). Only cold-load / slow-lazy-mount scenarios where the old race lost. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

github-actions · 2026-08-17T13:58:35Z

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically.

Maintainers can bypass this check by adding the skip-pr-checks label.

github-actions · 2026-08-17T14:01:17Z

🔴 Playwright Results — workflow failed

Validated commit 84df794e8cc451cf210c1c1d2cbedfdf3423b4bb in Playwright run 32037373240, attempt 1.

✅ 0 passed · ❌ 0 failed · 🟡 0 flaky · ⏭️ 0 skipped · 🧰 0 lifecycle flaky

Pipeline and setup failures (6)

The build job finished with status failure.
Duration-aware shard planning finished with status skipped.
Fixture cache restoration finished with status skipped.
Seeded fixture preparation finished with status skipped.
The Playwright shard matrix was unexpectedly skipped.
No expected Playwright shards were declared.

Performance

⚪ Performance metrics unavailable; see the CI and reporting failures above.

Shard	Passed	Failed	Flaky	Skipped	Lifecycle failed	Lifecycle flaky

📦 Download artifacts

How to debug locally

# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

greptile-apps · 2026-08-17T14:23:00Z

+    // Timer restart after a successful cross-tab refresh — the callback
+    // itself lives in this component's closure, so we register it here
+    // rather than from each authenticator.
+    tokenService.current.updateRefreshSuccessCallback(startTokenExpiryTimer);


Avoid capturing initial timer state

Registering startTokenExpiryTimer in the mount-only effect retains the initial authConfig and timeoutId. Later refresh successes therefore treat Basic/LDAP as refresh-capable and cannot replace timers created after the first render, resulting in unnecessary refresh requests or overlapping expiry timers.

Copilot

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

github-actions · 2026-08-17T16:28:11Z

⚠️ UI Checkstyle passed — lint findings in changed files

🔍 ESLint findings in this PR's files — 0 error(s), 49 warning(s)

Errors block the build. Warnings do not yet — they are rules whose backlog is still
being worked down, listed so this PR does not add to it. See docs/ui-code-quality-gate.md.

0 error(s), 49 warning(s) across 9 changed file(s).

Count	Rule
22	`i18next/no-literal-string`
11	`react-hooks/exhaustive-deps`
7	`sonarjs/no-duplicate-string`
5	`sonarjs/no-nested-functions`
1	`openmetadata-imports/no-lower-layer-page-imports`
1	`openmetadata-imports/no-internal-barrel-imports`
1	`openmetadata-imports/no-api-calls-in-iteration`
1	`sonarjs/cyclomatic-complexity`

All findings

	Location	Rule	Message
🟡	`src/components/Auth/AppAuthenticators/Auth0Authenticator.test.tsx:27:28`	`sonarjs/no-duplicate-string`	Define a constant instead of duplicating this literal 4 times.
🟡	`src/components/Auth/AppAuthenticators/Auth0Authenticator.test.tsx:64:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/Auth0Authenticator.test.tsx:75:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/Auth0Authenticator.test.tsx:89:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/Auth0Authenticator.test.tsx:107:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/Auth0Authenticator.test.tsx:128:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/Auth0Authenticator.test.tsx:144:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/Auth0Authenticator.test.tsx:158:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.test.tsx:54:29`	`i18next/no-literal-string`	disallow literal string: Loader
🟡	`src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.test.tsx:72:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.test.tsx:85:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.test.tsx:99:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.test.tsx:117:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.test.tsx:137:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.test.tsx:162:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.test.tsx:189:14`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/BasicAuthAuthenticator.tsx:64:10`	`react-hooks/exhaustive-deps`	React Hook useCallback has unnecessary dependencies: 'setOidcToken' and 'setRefreshToken'. Either exclude them or remove the dependency array. Outer scope value
🟡	`src/components/Auth/AppAuthenticators/GenericAuthenticator.test.tsx:65:16`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/GenericAuthenticator.test.tsx:83:16`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/GenericAuthenticator.test.tsx:103:16`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/GenericAuthenticator.test.tsx:122:16`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/GenericAuthenticator.test.tsx:143:16`	`i18next/no-literal-string`	disallow literal string: Child
🟡	`src/components/Auth/AppAuthenticators/MsalAuthenticator.test.tsx:35:15`	`sonarjs/no-duplicate-string`	Define a constant instead of duplicating this literal 3 times.
🟡	`src/components/Auth/AppAuthenticators/MsalAuthenticator.test.tsx:61:18`	`i18next/no-literal-string`	disallow literal string: Test Children
🟡	`src/components/Auth/AppAuthenticators/MsalAuthenticator.test.tsx:84:30`	`sonarjs/no-duplicate-string`	Define a constant instead of duplicating this literal 5 times.
🟡	`src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx:116:8`	`react-hooks/exhaustive-deps`	React Hook useCallback has a missing dependency: 'fetchIdToken'. Either include it or remove the dependency array.
🟡	`src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx:153:8`	`react-hooks/exhaustive-deps`	React Hook useEffect has a missing dependency: 'handleRedirect'. Either include it or remove the dependency array.
🟡	`src/components/Auth/AppAuthenticators/OidcAuthenticator.tsx:30:1`	`openmetadata-imports/no-lower-layer-page-imports`	Pages are route-level composition modules. Move the shared implementation/type to a lower layer instead of importing a page from here.
🟡	`src/components/Auth/AppAuthenticators/OidcAuthenticator.tsx:120:24`	`sonarjs/no-nested-functions`	Refactor this code to not nest functions more than 4 levels deep.
🟡	`src/components/Auth/AppAuthenticators/OidcAuthenticator.tsx:125:30`	`sonarjs/no-nested-functions`	Refactor this code to not nest functions more than 4 levels deep.
🟡	`src/components/Auth/AppAuthenticators/OktaAuthenticator.test.tsx:70:18`	`i18next/no-literal-string`	disallow literal string: Test Children
🟡	`src/components/Auth/AppAuthenticators/OktaAuthenticator.test.tsx:152:38`	`sonarjs/no-duplicate-string`	Define a constant instead of duplicating this literal 3 times.
🟡	`src/components/Auth/AppAuthenticators/OktaAuthenticator.test.tsx:153:46`	`sonarjs/no-duplicate-string`	Define a constant instead of duplicating this literal 4 times.
🟡	`src/components/Auth/AppAuthenticators/OktaAuthenticator.test.tsx:184:38`	`sonarjs/no-duplicate-string`	Define a constant instead of duplicating this literal 3 times.
🟡	`src/components/Auth/AppAuthenticators/OktaAuthenticator.test.tsx:195:51`	`sonarjs/no-duplicate-string`	Define a constant instead of duplicating this literal 3 times.
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:80:1`	`openmetadata-imports/no-internal-barrel-imports`	Import the internal module directly instead of its index barrel so unrelated siblings do not enter the bundle graph.
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:280:9`	`react-hooks/exhaustive-deps`	The 'onLoginHandler' function makes the dependencies of useMemo Hook (at line 941) change on every render. Move it inside the useMemo callback. Alternatively, w
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:357:6`	`react-hooks/exhaustive-deps`	React Hook useCallback has missing dependencies: 'navigate', 'setApplicationLoading', 'setCurrentUser', and 'setIsAuthenticated'. Either include them or remove
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:415:9`	`react-hooks/exhaustive-deps`	The 'resetUserDetails' function makes the dependencies of useMemo Hook (at line 941) change on every render. To fix this, wrap the definition of 'resetUserDetai
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:559:6`	`react-hooks/exhaustive-deps`	React Hook useEffect has missing dependencies: 'getLoggedInUserDetails' and 'startTokenExpiryTimer'. Either include them or remove the dependency array.
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:569:9`	`react-hooks/exhaustive-deps`	The 'handleFailedLogin' function makes the dependencies of useMemo Hook (at line 941) change on every render. Move it inside the useMemo callback. Alternatively
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:627:5`	`react-hooks/exhaustive-deps`	React Hook useCallback has missing dependencies: 'authConfig?.provider', 'handledVerifiedUser', 'navigate', 'resetUserDetails', and 'startTokenExpiryTimer'. Eit
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:672:9`	`react-hooks/exhaustive-deps`	The 'initializeAxiosInterceptors' function makes the dependencies of useMemo Hook (at line 941) change on every render. To fix this, wrap the definition of 'ini
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:740:67`	`sonarjs/no-nested-functions`	Refactor this code to not nest functions more than 4 levels deep.
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:747:23`	`openmetadata-imports/no-api-calls-in-iteration`	Avoid issuing one API request per item. Fetch at the data owner, use a bulk endpoint, or use useQueries with an intentional concurrency policy.
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:759:37`	`sonarjs/no-nested-functions`	Refactor this code to not nest functions more than 4 levels deep.
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:768:27`	`sonarjs/no-nested-functions`	Refactor this code to not nest functions more than 4 levels deep.
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:837:30`	`sonarjs/cyclomatic-complexity`	{"message":"Function has a complexity of 17 which is greater than 10 authorized.","cost":7,"secondaryLocations":[{"line":837,"column":29,"endLine":837,"endColum
🟡	`src/components/Auth/AuthProviders/AuthProvider.tsx:930:6`	`react-hooks/exhaustive-deps`	React Hook useEffect has missing dependencies: 'cleanup', 'fetchAuthConfig', 'initializeAxiosInterceptors', and 'startTokenExpiryTimer'. Either include them or

Fix locally (fast - only checks files changed in this branch):

make ui-checkstyle-changed

sonarqubecloud · 2026-08-17T16:40:07Z

Quality Gate passed for 'open-metadata-ui'

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

gitar-bot · 2026-08-17T21:00:12Z

Code Review ⚠️ Changes requested 0 resolved / 1 findings

Moves renewer registration into individual authenticator mount effects to resolve the cold-load 401 refresh race. However, changes_requested because updateRefreshSuccessCallback captures a stale startTokenExpiryTimer closure in the empty-dependency effect.

⚠️

Bug: refreshSuccessCallback captures stale startTokenExpiryTimer closure

📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx:492-495 📄 openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx:920-930

updateRefreshSuccessCallback(startTokenExpiryTimer) is now registered inside the [] mount effect (line 927), so it permanently captures the render-0 closure of startTokenExpiryTimer. On cold-load authConfig and timeoutId are still undefined at first render, and the callback is never re-registered. When a refresh later succeeds, performRefresh invokes this stale callback: clearTimeout(timeoutId) runs with the mount-time undefined, so it never clears the currently-pending proactive-renewal timer — successive refreshes accumulate orphan timers, each firing its own refreshToken() and risking the exact concurrent /auth/refresh rotation race the surrounding code warns against. It also mis-evaluates the Basic/LDAP shouldStartExpiry branch because authConfig?.provider is undefined. The previous ref-deps effect re-ran and captured fresher closures. Wrap startTokenExpiryTimer in useCallback and register it via an effect keyed on that callback, or read authConfig/timeoutId from refs so the registered callback always sees current state.

Memoize the timer and re-register the success callback so it never captures the stale mount-time closure.

const startTokenExpiryTimer = useCallback(async () => {
  /* ...existing body... */
}, [authConfig, timeoutId]);

// register/refresh the success callback whenever the timer closure changes
useEffect(() => {
  tokenService.current.updateRefreshSuccessCallback(startTokenExpiryTimer);
}, [startTokenExpiryTimer]);

🤖 Prompt for agents

Code Review: Moves renewer registration into individual authenticator mount effects to resolve the cold-load 401 refresh race. However, changes_requested because `updateRefreshSuccessCallback` captures a stale `startTokenExpiryTimer` closure in the empty-dependency effect.

1. ⚠️ Bug: refreshSuccessCallback captures stale startTokenExpiryTimer closure
   Files: openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx:492-495, openmetadata-ui/src/main/resources/ui/src/components/Auth/AuthProviders/AuthProvider.tsx:920-930

   `updateRefreshSuccessCallback(startTokenExpiryTimer)` is now registered inside the `[]` mount effect (line 927), so it permanently captures the render-0 closure of `startTokenExpiryTimer`. On cold-load `authConfig` and `timeoutId` are still `undefined` at first render, and the callback is never re-registered. When a refresh later succeeds, `performRefresh` invokes this stale callback: `clearTimeout(timeoutId)` runs with the mount-time `undefined`, so it never clears the currently-pending proactive-renewal timer — successive refreshes accumulate orphan timers, each firing its own `refreshToken()` and risking the exact concurrent `/auth/refresh` rotation race the surrounding code warns against. It also mis-evaluates the Basic/LDAP `shouldStartExpiry` branch because `authConfig?.provider` is `undefined`. The previous ref-deps effect re-ran and captured fresher closures. Wrap `startTokenExpiryTimer` in `useCallback` and register it via an effect keyed on that callback, or read `authConfig`/`timeoutId` from refs so the registered callback always sees current state.

   Fix (Memoize the timer and re-register the success callback so it never captures the stale mount-time closure.):
   const startTokenExpiryTimer = useCallback(async () => {
     /* ...existing body... */
   }, [authConfig, timeoutId]);
   
   // register/refresh the success callback whenever the timer closure changes
   useEffect(() => {
     tokenService.current.updateRefreshSuccessCallback(startTokenExpiryTimer);
   }, [startTokenExpiryTimer]);

Options

Display: compact → Showing less information.

Comment with these commands to change the behavior for this request:

`Compact`
`gitar display:verbose`

_{Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source}

…d 401 refresh (#31644) Follow-up to #31597. The hotfix added TokenService.awaitRenewerReady so the axios 401 interceptor's refresh call could wait briefly for the renewer to be registered, but the underlying race — the ref-deps useEffect that owned the registration — was never addressed. On any provider where the lazy authenticator finishes mounting AFTER the first /users/loggedInUser call has already 401'd (confidential + Google is where this reliably reproduces, but every provider is susceptible on cold-load with a slow chunk), the useEffect never re-runs (ref changes don't schedule re-renders), TokenService.renewToken stays null, awaitRenewerReady times out at 10s, and the interceptor force-logs the user out — no /api/v1/auth/refresh call is ever fired. Fix: each authenticator wrapper (BasicAuthAuthenticator, GenericAuthenticator, OidcAuthenticator, MsalAuthenticator, OktaAuthenticator, Auth0Authenticator) now calls TokenService.updateRenewToken from its own mount useEffect. That effect runs the moment useImperativeHandle populates the ref, with no dependence on the parent re-rendering — no race, deterministic registration. AuthProvider.tsx: the ref-deps useEffect is gone. updateRefreshSuccessCallback(startTokenExpiryTimer) moves to the main mount effect (that callback lives in AuthProvider's closure). TokenService.updateRenewToken now accepts `null` on cleanup, and RenewTokenCallback widens to `() => Promise<unknown>` so each provider's renewer can return its own SDK-native shape (Auth0 RenewTokenResponse, MSAL AuthenticationResult, oidc-client User, …) without a per-provider cast at the registration call site. Renewers wrapped in useCallback where they weren't already (GenericAuthenticator handleSilentSignIn, MsalAuthenticator renewIdToken, OktaAuthenticator renewToken, Auth0Authenticator renewIdToken extracted from useImperativeHandle) so the register effect's dep is stable and doesn't thrash. Tests: each authenticator's test file now mocks TokenService via jest.mock and asserts (a) the mount effect calls updateRenewToken with a function, and (b) unmount calls updateRenewToken(null). 5 new regression tests, 138/138 total across the auth surface. No behavior change on the happy path (renewer already registered in the same render cycle). Only cold-load / slow-lazy-mount scenarios where the old race lost. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> (cherry picked from commit 1511b12)

Copilot AI lite review requested due to automatic review settings August 17, 2026 13:58

chirag-madlani requested review from ShaileshParmar11 and karanh37 as code owners August 17, 2026 13:58

Copilot AI reviewed Aug 17, 2026

github-actions Bot added safe to test Add this label to run secure Github workflows on PRs UI UI specific issues labels Aug 17, 2026

greptile-apps Bot reviewed Aug 17, 2026

View reviewed changes

Merge branch 'main' into hotfix/renewer-registration-race

c10cd9b

Copilot AI review requested due to automatic review settings August 17, 2026 16:23

Copilot AI reviewed Aug 17, 2026

chirag-madlani enabled auto-merge August 17, 2026 16:24

karanh37 approved these changes Aug 17, 2026

View reviewed changes

chirag-madlani temporarily deployed to test August 17, 2026 16:56 — with GitHub Actions Inactive

chirag-madlani added this pull request to the merge queue Aug 17, 2026

Merged via the queue into main with commit 1511b12 Aug 17, 2026
90 of 93 checks passed

chirag-madlani deleted the hotfix/renewer-registration-race branch August 17, 2026 20:53

chirag-madlani mentioned this pull request Aug 18, 2026

feat(ui): AuthCoordinator refactor + full SSO Playwright matrix (9 scenarios × 8 providers) #31675

Open

5 tasks

Navigation Menu

Uh oh!

fix(ui): unblock cold-load 401 refresh (renewer registration race) - #31644

fix(ui): unblock cold-load 401 refresh (renewer registration race)#31644
chirag-madlani merged 2 commits into
mainfrom
hotfix/renewer-registration-race

chirag-madlani commented Aug 17, 2026 •

edited by greptile-apps Bot

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

greptile-apps Bot Aug 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Aug 17, 2026

Uh oh!

sonarqubecloud Bot commented Aug 17, 2026

Uh oh!

Uh oh!

gitar-bot Bot commented Aug 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

chirag-madlani commented Aug 17, 2026 • edited by greptile-apps Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Root cause

Fix

Files changed

Tests

Non-goals

Manual smoke matrix

Non-regression check

Follow-up

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Sequence Diagram

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 17, 2026

❌ PR checklist incomplete

Uh oh!

github-actions Bot commented Aug 17, 2026

🔴 Playwright Results — workflow failed

Pipeline and setup failures (6)

Performance

Uh oh!

greptile-apps Bot Aug 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 17, 2026

⚠️ UI Checkstyle passed — lint findings in changed files

🔍 ESLint findings in this PR's files — 0 error(s), 49 warning(s)

Uh oh!

sonarqubecloud Bot commented Aug 17, 2026

Quality Gate passed for 'open-metadata-ui'

Uh oh!

Uh oh!

gitar-bot Bot commented Aug 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

chirag-madlani commented Aug 17, 2026 •

edited by greptile-apps Bot

Loading