refactor: optimize refresh token error handling and add file lock and token file writable checks - #2135
refactor: optimize refresh token error handling and add file lock and token file writable checks#2135kiraWangRuilong wants to merge 7 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe refresh flow now reads tokens under a global lock, classifies structured refresh responses, probes storage writability, and performs generation-safe token updates or removals. ChangesToken refresh flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant refreshWithLock
participant withTokenStorageLock
participant RefreshEndpoint
participant TokenStore
refreshWithLock->>withTokenStorageLock: reload current token generation
withTokenStorageLock-->>refreshWithLock: current token state
refreshWithLock->>RefreshEndpoint: send JSON refresh request
RefreshEndpoint-->>refreshWithLock: return structured response
refreshWithLock->>TokenStore: compare-and-swap or compare-and-delete
TokenStore-->>refreshWithLock: return current or updated token
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@d1b64ecb960ae51d11be89de5a3ea2dd6635d090🧩 Skill updatenpx skills add larksuite/cli#refactor/optimize-refresh-token-flow -y -g |
844c6eb to
8af089a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
internal/auth/uat_client.go (2)
537-575: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
ensureDirWritablemessages match the parameter.The function accepts any
dir, but all four messages name the refresh lock directory. If a second caller appears, the errors will be wrong. Usedirin a neutral phrase, or rename the function toensureLockDirWritable.🤖 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 537 - 575, Update ensureDirWritable error messages to use neutral wording that accurately describes the generic dir parameter instead of referring specifically to the refresh lock directory. Apply this consistently to the MkdirAll, CreateTemp, cleanup, and close failure messages while preserving their existing error details and hints.
183-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ensureDirWritablerepeats thevfs.MkdirAllperformed at line 132.The lock directory already exists at this point, and the flock file is already open. The second
MkdirAllinsideensureDirWritableis redundant work on the refresh path. Consider passing the probe responsibility only, or moving the writeability probe next to theMkdirAllat line 132.🤖 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 183 - 190, Update the refresh-path handling in the UAT client to avoid calling ensureDirWritable after the lock directory has already been created by vfs.MkdirAll and the flock file opened. Reuse a writeability-only probe or move that probe beside the existing directory creation, while preserving the current warning and error return behavior.
🤖 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/uat_client.go`:
- Around line 577-591: The ensureTokenStorageWritable probe creates unnecessary
credential-store writes and can leave orphaned entries. Remove the per-refresh
probe flow from ensureTokenStorageWritable and classify actual SetStoredToken
failures in saveRefreshResponse instead, or replace the timestamp-based
probeUserOpenID with a fixed probe account that is overwritten and cleaned up
consistently.
- Around line 363-370: Wrap the error returned by httpClient.Do in
errs.NewNetworkError before storing it in refreshResult.err. Update the
transport-failure branch in doRefreshToken’s refresh flow, matching the existing
typed error handling in the read-failure branch while preserving the current
retry action selection.
- Around line 386-402: Update the json.Unmarshal failure and missing parsed.Code
branches in the refresh response handling to return refreshRetryAndPreserve
instead of refreshRetryAndClear. Keep refreshRetryAndClear for transport or
body-read failures that indicate possible token rotation, preserving the
existing invalid-response errors and retryability.
- Line 200: Remove the lone trailing tab on line 200 of the Go source and run
gofmt so the file is formatted cleanly with no reported changes.
- Around line 592-597: Update the probe flow containing SetStoredToken and
RemoveStoredToken to wrap each returned storage error with errs.NewInternalError
using errs.SubtypeStorage and an appropriate descriptive hint, rather than
returning the raw keychain error; preserve successful token setup and cleanup
behavior.
---
Nitpick comments:
In `@internal/auth/uat_client.go`:
- Around line 537-575: Update ensureDirWritable error messages to use neutral
wording that accurately describes the generic dir parameter instead of referring
specifically to the refresh lock directory. Apply this consistently to the
MkdirAll, CreateTemp, cleanup, and close failure messages while preserving their
existing error details and hints.
- Around line 183-190: Update the refresh-path handling in the UAT client to
avoid calling ensureDirWritable after the lock directory has already been
created by vfs.MkdirAll and the flock file opened. Reuse a writeability-only
probe or move that probe beside the existing directory creation, while
preserving the current warning and error return behavior.
🪄 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: d986bc1c-1ac1-4d19-9a99-3d6c2bd04ae9
📒 Files selected for processing (4)
internal/auth/token_store.gointernal/auth/uat_client.gointernal/errclass/codemeta.gointernal/errclass/codemeta_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/auth/token_store.go
- internal/errclass/codemeta.go
- internal/errclass/codemeta_test.go
kiraWangRuilong
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2135 +/- ##
==========================================
+ Coverage 75.69% 75.91% +0.22%
==========================================
Files 942 943 +1
Lines 100079 100305 +226
==========================================
+ Hits 75750 76148 +398
+ Misses 18537 18344 -193
- Partials 5792 5813 +21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
5290b58 to
0dc7185
Compare
0dc7185 to
d1b64ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/auth/token_store_test.go (1)
246-253: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the typed metadata of the malformed-payload error.
The test verifies cause preservation with
errors.Asbut not the classification.readStoredTokennow surfaces storage errors, so the category and subtype are part of the new contract. Without a metadata assertion, a change to the classification does not fail this test.Add an
errs.ProblemOfassertion next to the existing cause check.💚 Proposed addition
token, err := readStoredToken(appID, userOpenID) if token != nil { t.Fatalf("readStoredToken() token = %#v, want nil", token) } + requireRefreshProblem(t, err, errs.CategoryInternal, errs.SubtypeStorage, false) var syntaxErr *json.SyntaxError if !errors.As(err, &syntaxErr) { t.Fatalf("readStoredToken() error = %v (%T), want JSON syntax error in cause chain", err, err) }Adjust the expected category, subtype, and retryable flag to match the production classification.
As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings".🤖 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_test.go` around lines 246 - 253, Extend the malformed-payload test for readStoredToken with an errs.ProblemOf assertion alongside the existing errors.As check. Assert the production error’s expected category, subtype, and retryable flag (and metadata parameter as required), while preserving the current JSON syntax-error cause-chain validation.Source: Coding guidelines
internal/auth/token_lock.go (1)
52-56: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueMake
withTokenStorageLocknon-reentrant by design.
withMutexStorageLockis not currently called recursively in production paths, so the deadlock path is not currently reachable. Add a doc comment onwithTokenStorageLockstating thatfnmust not callSetStoredToken,RemoveStoredToken, orrefreshWithLockand must only use lock-holding helpers such aswriteStoredToken,deleteStoredToken, and the CAS helpers.🤖 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_lock.go` around lines 52 - 56, Document withTokenStorageLock as intentionally non-reentrant: state that fn must not call SetStoredToken, RemoveStoredToken, or refreshWithLock, and may only use lock-holding helpers such as writeStoredToken, deleteStoredToken, and the CAS helpers.
🤖 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/uat_client_refresh_test.go`:
- Around line 1086-1106: Rename TestRefreshWithLockReturnsNilWhenTokenWasRemoved
to reflect that setupStoredTokenTest leaves the store empty, such as
TestRefreshWithLockReturnsNilWhenNoTokenIsStored. Keep the test assertions and
behavior unchanged; do not imply coverage of a token-removal race.
---
Nitpick comments:
In `@internal/auth/token_lock.go`:
- Around line 52-56: Document withTokenStorageLock as intentionally
non-reentrant: state that fn must not call SetStoredToken, RemoveStoredToken, or
refreshWithLock, and may only use lock-holding helpers such as writeStoredToken,
deleteStoredToken, and the CAS helpers.
In `@internal/auth/token_store_test.go`:
- Around line 246-253: Extend the malformed-payload test for readStoredToken
with an errs.ProblemOf assertion alongside the existing errors.As check. Assert
the production error’s expected category, subtype, and retryable flag (and
metadata parameter as required), while preserving the current JSON syntax-error
cause-chain validation.
🪄 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: cd9f6313-b515-4694-8265-7fecae538e19
📒 Files selected for processing (7)
internal/auth/token_lock.gointernal/auth/token_lock_test.gointernal/auth/token_store.gointernal/auth/token_store_test.gointernal/auth/uat_client.gointernal/auth/uat_client_refresh_test.gointernal/errclass/codemeta_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/errclass/codemeta_test.go
- internal/auth/token_store.go
- internal/auth/uat_client.go
| func TestRefreshWithLockReturnsNilWhenTokenWasRemoved(t *testing.T) { | ||
| setupStoredTokenTest(t) | ||
| stored := newRefreshTestToken() | ||
| opts := newRefreshTestOptions(stored) | ||
| httpCalled := false | ||
| client := &http.Client{Transport: roundTripFunc(func(*http.Request) (*http.Response, error) { | ||
| httpCalled = true | ||
| return nil, errors.New("unexpected refresh request") | ||
| })} | ||
|
|
||
| refreshed, err := refreshWithLock(client, opts) | ||
| if err != nil { | ||
| t.Fatalf("refreshWithLock() error = %v", err) | ||
| } | ||
| if refreshed != nil { | ||
| t.Fatalf("refreshWithLock() token = %#v, want nil", refreshed) | ||
| } | ||
| if httpCalled { | ||
| t.Fatal("refresh endpoint was called after token removal") | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Rename the test to match its setup.
The test never stores a token, so it does not exercise removal. It asserts the behavior for an empty store. The name TestRefreshWithLockReturnsNilWhenTokenWasRemoved suggests that another actor deleted the token between the caller's read and the lock acquisition. A future reader can conclude that the removal race is covered here when it is not.
Rename to something like TestRefreshWithLockReturnsNilWhenNoTokenIsStored. If you want to cover the removal race directly, store the token first and delete it while the lock is held by a gated hook, in the style of TestDoRefreshTokenCASDoesNotResurrectDeletedGeneration at Lines 663-690.
🤖 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_refresh_test.go` around lines 1086 - 1106, Rename
TestRefreshWithLockReturnsNilWhenTokenWasRemoved to reflect that
setupStoredTokenTest leaves the store empty, such as
TestRefreshWithLockReturnsNilWhenNoTokenIsStored. Keep the test assertions and
behavior unchanged; do not imply coverage of a token-removal race.
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