⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.91%. Comparing base (5fd7af7) to head (50c228d).
⚠️ Report is 1 commits behind head on master.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@            Coverage Diff             @@
##           master    #6687      +/-   ##
==========================================
- Coverage   46.92%   46.91%   -0.01%     
==========================================
  Files         416      416              
  Lines       50549    50545       -4     
  Branches     7252     7252              
==========================================
- Hits        23718    23714       -4     
  Misses      25157    25157              
  Partials     1674     1674              

*This pull request uses carry forward flags. Click here to find out more.

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5fd7af7...50c228d. Read the comment docs.

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@franciscojavierarceo
Fixed by storing the token's true expiry and applying each caller's margin on read, the second option you suggested. The cache key is unchanged, so configs sharing credentials still share tokens, but a wider margin can no longer ride a narrower one's deadline. _token_refresh_deadline became _token_expiry and no longer takes a margin.

One behavior change worth flagging: a token already inside the margin now gets cached instead of skipped. Every read still rejects it for that caller so nothing observable changes, and a config with a smaller margin can legitimately use it.

The regression exercises both configs against the shared entry without clearing the cache, and also checks the reverse direction stays cheap, with the narrow-margin config reusing what the wide one wrote. I verified it fails on the old code, with the wide config receiving the narrow config's token. I also dropped the manual cache clear from test_refresh_margin_is_configurable, since that clear is what hid this; it now uses distinct credentials to isolate the two margins.

326 permissions tests pass.

@franciscojavierarceo @ntkathole
Addressed the cache key issue yesterday. The cache now stores the token's true expiry and each config applies its own margin on read, so a config with a wider margin can no longer ride a narrower one's deadline. The regression drives both configs through the shared entry without clearing the cache, and I verified it fails on the pre-fix code.

Green and ready for another look. It also still needs a kind/ label, which I can't add as an outside contributor.

@franciscojavierarceo @ntkathole
Still open and ready whenever you have a moment. The cache-key fix has been in since 5 August: the cache now stores the token's true expiry and each config applies its own margin on read, with a regression that I verified fails against the pre-fix code.

All checks green, no conflicts with current master. It also still needs a kind/ label, which I can't add as an outside contributor.

@larrysingleton007 please resolve the conflicts

@ntkathole done, master merged and pushed.

The only conflict was .secrets.baseline, which is generated: master had picked up newer entries while this branch carried two shifted line numbers for test_oidc_auth_client.py plus a newer generated_at. I took master's baseline and re-ran the detect-secrets hook rather than hand-merging, which reapplied exactly the two shifts this branch needs (29 to 44, 31 to 46). Hand-editing generated line numbers would have been guesswork.

326 permissions tests pass on the merged tree, with ruff and mypy clean. The cache-key fix and its regression are unchanged.

The other two, #6689 and #6690, merge cleanly against current master, so nothing needed there.

@ntkathole both addressed in 2a73cb7.

Pruning. Inserting on a miss now first drops entries whose stored expiry has passed. Keys are credential identities, so the cache was already bounded by the number of distinct configs, but a long-lived process that rotates credentials would have retained every retired one.

Revocation. Added OidcAuthClientManager.invalidate_token() plus a transport-agnostic invalidate_auth_token(auth_config) in client_auth_token.py, and wired it into the gRPC interceptor's existing error path: an UNAUTHENTICATED status drops the cached token, so the next call refetches. That bounds the staleness to the one request that was rejected rather than the remainder of the token's life.

One deliberate deviation from what you asked, and I'd rather flag it than quietly do less. I did not retry the rejected call. All four interceptor methods share _handle_call, and for the stream variants the request_iterator may already be partially consumed, so a retry there could replay a partial stream. Retry seems like it belongs per-transport, where the caller knows whether the request is replayable, rather than in the shared handler. The invalidation is the part that fixes the correctness problem; the retry only saves the caller one visible error. Happy to add it for the unary paths in this PR if you'd prefer, or as a follow-up covering all three transports, since the Arrow Flight and requests wrappers have no 401 handling at all today.

Two new tests: one asserting invalidation forces a refetch and is idempotent, one asserting an expired entry is pruned while an unexpired one survives. 328 permissions tests pass, ruff and mypy clean.

@larrysingleton007 not able to rebase and merge, can you rebase and possibly squash the commit?

@ntkathole rebased onto current master and squashed to a single commit, 202e30d. The branch is no longer behind, and the three merge commits are gone.

One note on how I did it, since it affects what you're reviewing. Master had moved three commits past this branch's last merge, so I brought those in first and then collapsed everything onto upstream/master. A straight squash of the old branch tip would have reverted those three. The net diff is unchanged: 6 files, +414/-9, all of it this PR's own work.

Re-verified after the rebase, since master's newer commits touched permissions/user.py: 329 permissions tests pass, ruff and mypy clean.

@ntkathole both review threads are resolved now. I'd replied in the conversation rather than inside the threads, which is why they stayed open — my mistake.

Summary of where they landed: pruning on cache miss is implemented and covered by a test; eviction on UNAUTHENTICATED is implemented via invalidate_token() and invalidate_auth_token(), wired into the gRPC interceptor. The retry is the one piece I left out, for the stream-replay reason in the thread, and I'm happy to add it either here for the unary paths or as a follow-up across all three transports.

Two things still block the merge, neither of them code in this PR.

The failing check is unit-test-python (3.10, ubuntu-latest) on test_http_session_manager::test_thread_safety, which asserts len(sessions) == 10 and got 9. It can't be this PR: that test builds its session from NoAuthConfig(), which takes the AuthType.NONE branch and never calls get_auth_token, so the token cache isn't in its code path. Its own assert len(errors) == 0 passed, so no thread raised — one thread just didn't record a result, which is a race in the test's own accounting. The test came in with #5895. Could you re-run that job?

@franciscojavierarceo the reviewDecision is still CHANGES_REQUESTED from your 3 August review of the cache key. That was fixed on 5 August: the cache now stores the token's true expiry and applies each config's margin on read, with a regression that I verified fails against the pre-fix code. Would you mind taking another look so the review state clears?

Following up on the red check with evidence rather than just asking for a re-run.

unit-test-python fails intermittently on master, on a different test each time:

Both are on master with no PR involved. The failure here, test_http_session_manager::test_thread_safety, is a third instance of the same class: tests that assert on wall-clock timing or thread scheduling, which are sensitive to how loaded the runner is when 8 xdist workers share it.

For this PR specifically, that test builds its session from NoAuthConfig(), which takes the AuthType.NONE branch and never calls get_auth_token, so nothing in this change is in its code path. I also ran it 88 times locally, 40 in isolation and 48 under deliberate CPU contention, without a single failure — consistent with it needing a loaded shared runner rather than being deterministic.

So a re-run should clear it. Separately, test_performance_bounds_single_entity looks worth fixing on its own: a 0.2s wall-clock bound cannot be made reliable by raising it a little, since the runner's available CPU isn't bounded. Happy to open an issue and PR for that if it's useful.

One more CI follow-up: the DuckDB failure in the latest run is unrelated to this OIDC diff. I found the same extension-install lock race in 2 other PR runs, filed #6743, and put the fix in #6744.

#6744 installs DuckDB's delta extension once before the 8 pytest workers start. The cold-cache suite passes locally with 84 tests passed and 5 skipped, and all 18 ordinary PR checks are green. Its DuckDB CI job is waiting for ok-to-test.

No change is needed in this PR for that failure.