-
Notifications
You must be signed in to change notification settings - Fork 961
auth: refresh user tokens earlier and clarify refresh failures#1416
auth: refresh user tokens earlier and clarify refresh failures #1416ArthurNie wants to merge 3 commits into
Conversation
albertnusouo
commented
Jun 12, 2026
Overall: Solid, well-tested change addressing a real pain point — the 5-min window is easy for scheduled/intermittent CLI
usage to miss. Code compiles, meta/metaOK are correctly in scope, and the new test meaningfully guards the constant
(59/61 straddle the boundary and would fail under the old value). A few things worth considering before merge.
Main concern — widening the window enlarges the blast radius of clearing a still-valid token
In GetValidAccessToken, the needs_refresh path clears the token and returns NeedAuthorizationError on any non-retryable
refresh failure. With the old 5-min window, the in-hand access token had ≤5 min left, so discarding it was cheap. At 60 min,
a transient refresh-side hiccup (or an errclass misclassification) can now discard an access token with up to ~59 min of
remaining validity and force a re-login. This trades availability for earlier refresh, and is strictly more fragile when the
refresh endpoint is flaky.
Suggestion: on a non-retryable refresh failure, check now < stored.ExpiresAt first — if the access token is still
actually valid, keep using it (warn only) instead of clearing. If immediate invalidation on revoked/reused codes is an
intentional security policy, please state that explicitly, as it's a deliberate tradeoff.
Implicit assumption — access token TTL must exceed the refresh-ahead window
This relies on TTL > 60 min (currently 7200s default). If the server ever issues tokens with TTL ≤ 60 min, ExpiresAt - 60min is always in the past and every CLI invocation triggers a refresh. Worth a one-line comment at the constant
documenting this dependency.
Minor
- Refresh call frequency roughly doubles (one extra rotation per token lifetime) — negligible at current scale, but a noted
cost. - Nit: the PR body mentions running
go test ./internal/errclass, but no errclass code/tests are touched here
(regression-only, no issue).
|
No actionable comments were generated in the recent review. 🎉 i️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR increases the refresh-ahead window from 5 to 60 minutes and updates refresh failure handling so doRefreshToken preserves still-valid access tokens unless the refresh token is classified as unusable; tests exercise window boundaries and multiple refresh-failure scenarios. ChangesToken Refresh and Failure Handling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/auth/token_store_test.go`:
- Around line 74-82: The test currently only exercises the helper
shouldClearTokenAfterRefreshFailure; add a unit test that directly invokes
doRefreshToken (or the exported function that performs the refresh flow) to
simulate an unrecoverable refresh failure and assert it both clears the stored
token in the TokenStore and emits the expected "auth login" guidance message;
mock or stub the refresh RPC to return an errclass with
Category=errs.CategoryAuthentication and
Subtype=errs.SubtypeRefreshTokenRevoked, call doRefreshToken with a TokenStore
pre-populated with a refresh token, then verify the TokenStore no longer
contains the token and that the logger/output contains the auth login guidance.
In `@internal/auth/uat_client.go`:
- Around line 271-281: The code drops the session or returns on callEndpoint()
transport/retry failures without applying the same "keep valid token until
expiry" fallback used for parsed OAuth errors; update the error handling around
callEndpoint() and the retry branch to check stored.ExpiresAt and
shouldClearTokenAfterRefreshFailure(meta, metaOK) (using meta/metaOK when
available) and, when the access token is still valid and
shouldClearTokenAfterRefreshFailure returns false, log a warning (including
opts.UserOpenId and the error/transport info) and return the existing stored
token instead of clearing it; ensure the same subtype/log message pattern used
in the parsed-error path is applied for transport/retry failures so transient
outages don’t force re-login.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
i️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7c24f21f-2e3d-41df-8b65-0628b19301b5
📒 Files selected for processing (3)
internal/auth/token_store.gointernal/auth/token_store_test.gointernal/auth/uat_client.go
🚀 PR Preview Install Guide
🧰 CLI update
npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d6c0c0351c92933e4b7d8d45da7743bb8580f216
🧩 Skill update
npx skills add ArthurNie/cli#codex/auth-refresh-ahead-diagnostics -y -g
Uh oh!
There was an error while loading. Please reload this page.
Summary
Why
User access tokens are short-lived, and the previous 5-minute refresh-ahead window is easy for scheduled or intermittent CLI usage to miss. If refresh later fails with a non-retryable OAuth business code such as 20064, the CLI clears the stored user token and subsequent commands only see missing authorization. A wider refresh window makes normal CLI usage more likely to rotate tokens before expiry.
The fallback avoids widening the blast radius of refresh-side failures: when the current access token is still valid and the failure does not prove the refresh token is unusable, the CLI keeps using the existing token until expiry. Revoked, reused, invalid, or expired refresh-token classifications still clear the stored token and require reauthorization.
Test
No raw tokens, user IDs, log IDs, or Feishu response bodies are included in this PR.
Summary by CodeRabbit
New Features
Tests