refactor: optimize refresh token error handling and enhance file lock and token file checks - #2135
refactor: optimize refresh token error handling and enhance file lock and token file checks#2135kiraWangRuilong wants to merge 2 commits into
Conversation
📝 WalkthroughWalkthroughThe token refresh flow now reloads state under global locks, classifies JSON refresh responses, handles retries and policy errors, checks directory writeability, and conditionally persists or removes tokens by generation. Error metadata and tests cover additional authentication, authorization, API, and configuration codes. ChangesToken refresh flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant refreshWithLock
participant TokenStore
participant RefreshEndpoint
participant ErrorClassifier
refreshWithLock->>TokenStore: reload token after acquiring global lock
TokenStore-->>refreshWithLock: current token generation
refreshWithLock->>RefreshEndpoint: send JSON refresh request
RefreshEndpoint-->>refreshWithLock: traced response
refreshWithLock->>ErrorClassifier: classify response and retry action
ErrorClassifier-->>refreshWithLock: retry or preservation decision
refreshWithLock->>TokenStore: conditionally update or remove token
TokenStore-->>refreshWithLock: current or persisted token
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
kiraWangRuilong
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
internal/auth/token_store.go (1)
47-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider returning typed errors from
readStoredToken.
readStoredTokenreturns raw keychain andjson.Unmarshalerrors. These errors now reach callers such asrefreshWithLockand surface untyped. Wrap them with the prescribed typed constructor so the CLI emits a classified envelope.♻️ Proposed refactor
func readStoredToken(appId, userOpenId string) (*StoredUAToken, error) { jsonStr, err := keychain.Get(keychain.LarkCliService, accountKey(appId, userOpenId)) if err != nil { - return nil, err + return nil, errs.NewInternalError(errs.SubtypeStorage, + "failed to read stored user token: %v", err).WithCause(err) } if jsonStr == "" { return nil, nil } var token StoredUAToken if err := json.Unmarshal([]byte(jsonStr), &token); err != nil { - return nil, err + return nil, errs.NewInternalError(errs.SubtypeStorage, + "stored user token is not valid JSON: %v", err).WithCause(err) } return &token, nil }As per coding guidelines: "Use the prescribed typed error constructors for validation, failed preconditions, API failures, network failures, file I/O failures, and unknown lower-layer errors".
🤖 Prompt for 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. In `@internal/auth/token_store.go` around lines 47 - 59, Update readStoredToken to wrap both keychain.Get and json.Unmarshal failures with the prescribed typed error constructor before returning them, while preserving nil-token behavior for an empty keychain value and successful token parsing.Source: Coding guidelines
internal/auth/uat_client.go (1)
248-259: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: remove the duplicated refresh-expiry check.
refreshWithLockalready handles theexpiredstatus under the lock at Lines 169-181, includingremoveStoredTokenIfCurrentand the same log line. This block repeats that logic for the same token snapshot. Consider keeping the check in one place, or add a comment that states which callers can reachdoRefreshTokenwithout the locked check.🤖 Prompt for 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. In `@internal/auth/uat_client.go` around lines 248 - 259, Remove the duplicated refresh-expiry handling from doRefreshToken, relying on refreshWithLock’s existing locked check and cleanup flow. If the check must remain for callers that bypass refreshWithLock, document that caller path clearly instead of duplicating the logic.internal/errclass/codemeta.go (1)
56-62: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the comment typos.
Line 60 has
staus. Line 62 readsnot allows for refresh token.✏️ Proposed fix
- 20066: {Category: errs.CategoryAuthorization, Subtype: errs.SubtypeUserUnauthorized}, // user staus is not normal + 20066: {Category: errs.CategoryAuthorization, Subtype: errs.SubtypeUserUnauthorized}, // user status is not normal - 20074: {Category: errs.CategoryAuthorization, Subtype: errs.SubtypeAppUnavailable}, // app specified not allows for refresh token + 20074: {Category: errs.CategoryAuthorization, Subtype: errs.SubtypeAppUnavailable}, // app is not allowed to use refresh tokens🤖 Prompt for 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. In `@internal/errclass/codemeta.go` around lines 56 - 62, Correct the inline comments in the error-code mapping: change “staus” to “status” on the 20066 entry and fix the grammar of the 20074 comment to state that the app does not allow refresh tokens.
🤖 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.go`:
- Around line 77-80: Update the comment above isSameStoredTokenGeneration to use
the exact function name, remove the trailing whitespace after “does not,” and
run gofmt so the repository produces no formatting violations.
In `@internal/auth/uat_client.go`:
- Around line 469-481: Update refreshActionForCode so the !ok branch for
unmapped codes preserves the stored token while retaining retry behavior. Keep
token clearing limited to explicitly classified terminal credential failures,
without changing the existing policy, retryable, or default classified-code
handling.
- Around line 396-416: Update the policy-error construction in the refresh
result branch to ensure Problem.Message is non-empty when
parsed.ErrorDescription is absent. Reuse the established errclass fallback
behavior or generated-message helper used by errclass.BuildAPIError, while
preserving the endpoint description when provided; only adjust the Message
assignment in the errs.SecurityPolicyError path.
In `@internal/errclass/codemeta.go`:
- Around line 44-46: Update the codemeta entry for error code 20072 to set
Retryable: true, matching the temporary refresh-server behavior and the 20050
entry. Update the corresponding expected retryability assertion in the codemeta
tests to true.
---
Nitpick comments:
In `@internal/auth/token_store.go`:
- Around line 47-59: Update readStoredToken to wrap both keychain.Get and
json.Unmarshal failures with the prescribed typed error constructor before
returning them, while preserving nil-token behavior for an empty keychain value
and successful token parsing.
In `@internal/auth/uat_client.go`:
- Around line 248-259: Remove the duplicated refresh-expiry handling from
doRefreshToken, relying on refreshWithLock’s existing locked check and cleanup
flow. If the check must remain for callers that bypass refreshWithLock, document
that caller path clearly instead of duplicating the logic.
In `@internal/errclass/codemeta.go`:
- Around line 56-62: Correct the inline comments in the error-code mapping:
change “staus” to “status” on the 20066 entry and fix the grammar of the 20074
comment to state that the app does not allow refresh tokens.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 78bff60f-ccc0-4b45-b223-ad1e1e9fffa0
📒 Files selected for processing (4)
internal/auth/token_store.gointernal/auth/uat_client.gointernal/errclass/codemeta.gointernal/errclass/codemeta_test.go
| // sameStoredTokenGeneration reports whether two snapshots represent the same | ||
| // refresh-token generation. Access tokens are used only for case that does not | ||
| // contain a refresh token. | ||
| func isSameStoredTokenGeneration(current, expected *StoredUAToken) bool { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the doc comment name and remove the trailing space.
The comment names sameStoredTokenGeneration, but the function is isSameStoredTokenGeneration. Line 78 also ends with a trailing space after does not , which gofmt removes. The repository requires no gofmt -l . output.
♻️ Proposed fix
-// sameStoredTokenGeneration reports whether two snapshots represent the same
-// refresh-token generation. Access tokens are used only for case that does not
-// contain a refresh token.
+// isSameStoredTokenGeneration reports whether two snapshots represent the same
+// refresh-token generation. Access tokens are compared only when neither
+// snapshot contains a refresh token.
func isSameStoredTokenGeneration(current, expected *StoredUAToken) bool {As per coding guidelines: "Run gofmt; Go code must be formatted with no gofmt -l . output."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // sameStoredTokenGeneration reports whether two snapshots represent the same | |
| // refresh-token generation. Access tokens are used only for case that does not | |
| // contain a refresh token. | |
| func isSameStoredTokenGeneration(current, expected *StoredUAToken) bool { | |
| // isSameStoredTokenGeneration reports whether two snapshots represent the same | |
| // refresh-token generation. Access tokens are compared only when neither | |
| // snapshot contains a refresh token. | |
| func isSameStoredTokenGeneration(current, expected *StoredUAToken) bool { |
🤖 Prompt for 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.
In `@internal/auth/token_store.go` around lines 77 - 80, Update the comment above
isSameStoredTokenGeneration to use the exact function name, remove the trailing
whitespace after “does not,” and run gofmt so the repository produces no
formatting violations.
Source: Coding guidelines
| if code != 0 { | ||
| if meta, ok := errclass.LookupCodeMeta(code); ok && meta.Category == errs.CategoryPolicy { | ||
| var policyFields struct { | ||
| ChallengeURL string `json:"challenge_url"` | ||
| CLIHint string `json:"cli_hint"` | ||
| } | ||
| // Retry succeeded, fall through to parse token below. | ||
| } else { | ||
| // All other errors: clear token, require re-authorization. | ||
| fmt.Fprintf(errOut, "[lark-cli] [WARN] uat-client: refresh failed (code=%d), clearing token for %s\n", code, opts.UserOpenId) | ||
| if err := RemoveStoredToken(opts.AppId, opts.UserOpenId); err != nil { | ||
| fmt.Fprintf(errOut, "[lark-cli] [WARN] uat-client: failed to remove token: %v\n", err) | ||
| _ = json.Unmarshal(body, &policyFields) | ||
| return refreshResult{ | ||
| action: refreshStopAndPreserve, | ||
| err: &errs.SecurityPolicyError{ | ||
| Problem: errs.Problem{ | ||
| Category: errs.CategoryPolicy, | ||
| Subtype: meta.Subtype, | ||
| Code: code, | ||
| Message: parsed.ErrorDescription, | ||
| Hint: policyFields.CLIHint, | ||
| }, | ||
| ChallengeURL: policyFields.ChallengeURL, | ||
| }, | ||
| } | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Keep Problem.Message non-empty for policy errors.
This branch sets Message from parsed.ErrorDescription only. The OAuth endpoint can omit error_description. Message is then empty on the wire envelope. errclass.BuildAPIError avoids this by falling back to a generated message (see internal/errclass/classify.go:54-59). Apply the same fallback here.
🛠️ Proposed fix
+ message := parsed.ErrorDescription
+ if message == "" {
+ message = parsed.Error
+ }
+ if message == "" {
+ message = fmt.Sprintf("token refresh rejected by policy: [%d]", code)
+ }
return refreshResult{
action: refreshStopAndPreserve,
err: &errs.SecurityPolicyError{
Problem: errs.Problem{
Category: errs.CategoryPolicy,
Subtype: meta.Subtype,
Code: code,
- Message: parsed.ErrorDescription,
+ Message: message,
Hint: policyFields.CLIHint,
},
ChallengeURL: policyFields.ChallengeURL,
},
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if code != 0 { | |
| if meta, ok := errclass.LookupCodeMeta(code); ok && meta.Category == errs.CategoryPolicy { | |
| var policyFields struct { | |
| ChallengeURL string `json:"challenge_url"` | |
| CLIHint string `json:"cli_hint"` | |
| } | |
| // Retry succeeded, fall through to parse token below. | |
| } else { | |
| // All other errors: clear token, require re-authorization. | |
| fmt.Fprintf(errOut, "[lark-cli] [WARN] uat-client: refresh failed (code=%d), clearing token for %s\n", code, opts.UserOpenId) | |
| if err := RemoveStoredToken(opts.AppId, opts.UserOpenId); err != nil { | |
| fmt.Fprintf(errOut, "[lark-cli] [WARN] uat-client: failed to remove token: %v\n", err) | |
| _ = json.Unmarshal(body, &policyFields) | |
| return refreshResult{ | |
| action: refreshStopAndPreserve, | |
| err: &errs.SecurityPolicyError{ | |
| Problem: errs.Problem{ | |
| Category: errs.CategoryPolicy, | |
| Subtype: meta.Subtype, | |
| Code: code, | |
| Message: parsed.ErrorDescription, | |
| Hint: policyFields.CLIHint, | |
| }, | |
| ChallengeURL: policyFields.ChallengeURL, | |
| }, | |
| } | |
| return nil, nil | |
| } | |
| if code != 0 { | |
| if meta, ok := errclass.LookupCodeMeta(code); ok && meta.Category == errs.CategoryPolicy { | |
| var policyFields struct { | |
| ChallengeURL string `json:"challenge_url"` | |
| CLIHint string `json:"cli_hint"` | |
| } | |
| _ = json.Unmarshal(body, &policyFields) | |
| message := parsed.ErrorDescription | |
| if message == "" { | |
| message = parsed.Error | |
| } | |
| if message == "" { | |
| message = fmt.Sprintf("token refresh rejected by policy: [%d]", code) | |
| } | |
| return refreshResult{ | |
| action: refreshStopAndPreserve, | |
| err: &errs.SecurityPolicyError{ | |
| Problem: errs.Problem{ | |
| Category: errs.CategoryPolicy, | |
| Subtype: meta.Subtype, | |
| Code: code, | |
| Message: message, | |
| Hint: policyFields.CLIHint, | |
| }, | |
| ChallengeURL: policyFields.ChallengeURL, | |
| }, | |
| } | |
| } |
🤖 Prompt for 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.
In `@internal/auth/uat_client.go` around lines 396 - 416, Update the policy-error
construction in the refresh result branch to ensure Problem.Message is non-empty
when parsed.ErrorDescription is absent. Reuse the established errclass fallback
behavior or generated-message helper used by errclass.BuildAPIError, while
preserving the endpoint description when provided; only adjust the Message
assignment in the errs.SecurityPolicyError path.
| func refreshActionForCode(code int) refreshAction { | ||
| meta, ok := errclass.LookupCodeMeta(code) | ||
| switch { | ||
| case !ok: | ||
| return refreshRetryAndClear | ||
| case meta.Category == errs.CategoryPolicy: | ||
| return refreshStopAndPreserve | ||
| case meta.Retryable: | ||
| return refreshRetryAndPreserve | ||
| default: | ||
| return refreshStopAndClear | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reconsider clearing the stored token for unmapped server codes.
refreshActionForCode returns refreshRetryAndClear when LookupCodeMeta has no entry for the code. An unrecognized code then deletes the stored refresh token after the last attempt, and the user must log in again. A new or undocumented server code is not evidence that the refresh token is invalid. Prefer preserving the token for unmapped codes, and clear it only for codes that are classified as terminal credential failures.
♻️ Proposed change
switch {
case !ok:
- return refreshRetryAndClear
+ return refreshRetryAndPreserve
case meta.Category == errs.CategoryPolicy:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func refreshActionForCode(code int) refreshAction { | |
| meta, ok := errclass.LookupCodeMeta(code) | |
| switch { | |
| case !ok: | |
| return refreshRetryAndClear | |
| case meta.Category == errs.CategoryPolicy: | |
| return refreshStopAndPreserve | |
| case meta.Retryable: | |
| return refreshRetryAndPreserve | |
| default: | |
| return refreshStopAndClear | |
| } | |
| } | |
| func refreshActionForCode(code int) refreshAction { | |
| meta, ok := errclass.LookupCodeMeta(code) | |
| switch { | |
| case !ok: | |
| return refreshRetryAndPreserve | |
| case meta.Category == errs.CategoryPolicy: | |
| return refreshStopAndPreserve | |
| case meta.Retryable: | |
| return refreshRetryAndPreserve | |
| default: | |
| return refreshStopAndClear | |
| } | |
| } |
🤖 Prompt for 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.
In `@internal/auth/uat_client.go` around lines 469 - 481, Update
refreshActionForCode so the !ok branch for unmapped codes preserves the stored
token while retaining retry behavior. Keep token clearing limited to explicitly
classified terminal credential failures, without changing the existing policy,
retryable, or default classified-code handling.
| 20050: {Category: errs.CategoryAuthentication, Subtype: errs.SubtypeRefreshServerError, Retryable: true}, // refresh endpoint transient error | ||
| 20064: {Category: errs.CategoryAuthentication, Subtype: errs.SubtypeRefreshTokenRevoked}, // refresh_token revoked | ||
| 20072: {Category: errs.CategoryAuthentication, Subtype: errs.SubtypeRefreshServerError}, // refresh endpoint temporarily unavailable |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Lark open platform OAuth error code 20072 refresh token retryable
💡 Result:
The Lark Open Platform error code 20072 indicates that the server is temporarily unavailable [1]. According to official Lark documentation, this error is categorized as a transient server-side issue, and you are advised to retry your request [1]. Key details regarding this error: * Meaning: The server is temporarily unable to process the request [1]. * Status: It is considered a retryable error [1]. * Implementation: In the context of the Lark Open Platform SDK, transient server-side errors (such as those represented by HTTP 503-like statuses) are explicitly handled as retryable to ensure robust token management [2][3]. If you encounter this error while refreshing a token, you should implement an exponential backoff or a brief delay before retrying the operation to avoid overwhelming the server [1]. If the error persists after multiple retries, it may indicate a broader service disruption, and you should contact Lark technical support or monitor official status channels [1].
Citations:
- 1: https://open.larksuite.com/document/uAjLw4CM/ukTMukTMukTM/authentication-management/access-token/refresh-user-access-token
- 2: https://github.com/larksuite/cli/blob/d4d4f32e/internal/auth/errors.go
- 3: https://github.com/larksuite/cli/blob/main/errs/subtypes.go
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)(codemeta\.go|codemeta_test\.go|refreshActionForCode|errs|subtypes)\.go$' || true
echo
echo "== codemeta.go relevant lines =="
if [ -f internal/errclass/codemeta.go ]; then
nl -ba internal/errclass/codemeta.go | sed -n '1,120p'
fi
echo
echo "== codemeta_test.go relevant context =="
if [ -f internal/errclass/codemeta_test.go ]; then
nl -ba internal/errclass/codemeta_test.go | sed -n '1,120p'
fi
echo
echo "== search refreshActionForCode and SubtypeRefreshServerError =="
rg -n "refreshActionForCode|SubtypeRefreshServerError|20072|20050|Retryable" -S .Repository: larksuite/cli
Length of output: 265
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | grep -E '(^|/)(codemeta\.go|codemeta_test\.go|refreshActionForCode|errs|subtypes)\.go$' || true
echo
echo "== codemeta.go relevant lines =="
if command -v nl >/dev/null 2>&1; then
nl -ba internal/errclass/codemeta.go | sed -n '1,120p'
elif command -v awk >/dev/null 2>&1; then
awk '{printf "%6d\t%s\n", NR, $0}' internal/errclass/codemeta.go | sed -n '1,120p'
fi
echo
echo "== codemeta_test.go relevant context =="
if [ -f internal/errclass/codemeta_test.go ]; then
awk '{printf "%6d\t%s\n", NR, $0}' internal/errclass/codemeta_test.go | sed -n '1,120p'
fi
echo
echo "== search refreshActionForCode and SubtypeRefreshServerError =="
grep -RIn "refreshActionForCode|SubtypeRefreshServerError|20072|20050|Retryable" . || trueRepository: larksuite/cli
Length of output: 14267
🌐 Web query:
Lark open platform refresh-user-access-token error 20072 retryable
💡 Result:
In the Lark Open Platform API, the error code 20072 indicates that the server is temporarily unavailable [1][2]. The official recommendation for this error is to retry your request [1][2]. Because this is a transient server-side error, it is considered retryable [1][2][3]. If you continue to encounter this error after retrying, it suggests a persistent issue with the server's availability, and you may need to wait before attempting further requests [1][2].
Citations:
- 1: https://open.larksuite.com/document/uAjLw4CM/ukTMukTMukTM/authentication-management/access-token/get-user-access-token
- 2: https://open.larksuite.com/document/uAjLw4CM/ukTMukTMukTM/authentication-management/access-token/refresh-user-access-token
- 3: https://open.larkenterprise.com/document/authentication-management/access-token/get-user-access-token
Mark refresh error 20072 as retryable.
20072 is documented as a temporary refresh server unavailable error, so it should use Retryable: true like 20050, and the expected retryability in internal/errclass/codemeta_test.go line 31 should also change to true.
🤖 Prompt for 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.
In `@internal/errclass/codemeta.go` around lines 44 - 46, Update the codemeta
entry for error code 20072 to set Retryable: true, matching the temporary
refresh-server behavior and the 20050 entry. Update the corresponding expected
retryability assertion in the codemeta tests to true.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@844c6eb30fc394727a4a0e6ab83e4d195408dbcb🧩 Skill updatenpx skills add larksuite/cli#refactor/optimize-refresh-token-flow -y -g |
1 participant
Summary
This PR contains two related follow-up changes to user access token refresh reliability:
Changes
/open-apis/authen/v2/oauth/tokenrefresh flow:internal/errclass/codemeta.gofor OAuth refresh-related and validation/client config/user/app-state codes, with retryability updated where applicable.internal/auth/token_store.go:internal/auth/uat_client.go:Test Plan
Related Issues
Summary by CodeRabbit