fix: ensure Qwen refreshed tokens are persisted to disk#2726
fix: ensure Qwen refreshed tokens are persisted to disk#2726aeddk-bnh wants to merge 1 commit intorouter-for-me:devfrom
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the QwenExecutor by ensuring that refreshed tokens are persisted to the underlying QwenTokenStorage and introduces dependency injection for the token refresh process to improve testability. New unit tests have been added to verify these updates. Key feedback includes a recommendation to update the LastRefreshedAt and UpdatedAt fields on the Auth struct for consistency, and a warning regarding potential data loss in UpdateTokenStorage if the refresh response contains empty fields.
| // Update the underlying storage object so FileTokenStore persists the | ||
| // refreshed tokens (especially the rotated refresh_token) to disk. | ||
| if storage, ok := auth.Storage.(*qwenauth.QwenTokenStorage); ok { | ||
| svc.UpdateTokenStorage(storage, td) |
There was a problem hiding this comment.
The UpdateTokenStorage method (defined in internal/auth/qwen/qwen_auth.go) appears to unconditionally overwrite storage.RefreshToken and storage.ResourceURL. If the refresh response from the server does not include these fields (which is common if the refresh token has not rotated), the existing credentials in storage will be wiped. This will prevent subsequent refreshes once the current access token expires. Please update UpdateTokenStorage to only update these fields if they are non-empty in the td object, matching the logic used for auth.Metadata in lines 667-672.
| now := time.Now().Format(time.RFC3339) | ||
| auth.Metadata["last_refresh"] = now |
There was a problem hiding this comment.
In addition to updating the metadata, it is important to update the LastRefreshedAt and UpdatedAt fields on the Auth struct itself. This ensures that the system's internal state is correctly maintained for components that rely on these timestamps (such as the AuthManager or UI). Using UTC is recommended for consistency with the struct's documentation.
| now := time.Now().Format(time.RFC3339) | |
| auth.Metadata["last_refresh"] = now | |
| now := time.Now().UTC() | |
| auth.Metadata["last_refresh"] = now.Format(time.RFC3339) | |
| auth.LastRefreshedAt = now | |
| auth.UpdatedAt = now |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee288f50f8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if storage, ok := auth.Storage.(*qwenauth.QwenTokenStorage); ok { | ||
| svc.UpdateTokenStorage(storage, td) |
There was a problem hiding this comment.
Refresh now always calls svc.UpdateTokenStorage(storage, td), but that helper unconditionally assigns RefreshToken and ResourceURL; this conflicts with the guarded metadata updates just above (td.RefreshToken != "", td.ResourceURL != "") and can erase previously valid values when the refresh response omits optional fields. In deployments using GitTokenStore/PostgresStore/ObjectTokenStore (which persist auth.Storage directly), this can write an empty refresh_token to disk and cause the next refresh cycle to fail after restart or cache miss.
Useful? React with 👍 / 👎.
aeddk-bnh
force-pushed
the
fix/qwen-token-persistence
branch
from
ee288f5 to
bf70c15
Compare
|
Thank you for the detailed feedback! I've updated the implementation to address the concerns:
Force-pushed the updated commit. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bf70c15375
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if storage, ok := auth.Storage.(*qwenauth.QwenTokenStorage); ok { | ||
| svc.UpdateTokenStorage(storage, td) |
There was a problem hiding this comment.
Refresh runs on per-request auth clones, but Auth.Clone() only shallow-copies Storage, so this write mutates the same *QwenTokenStorage instance held by the manager. In concurrent traffic (e.g., two requests on one credential where one hits 429 and refreshes while another is being persisted via MarkResult), this unsynchronized cross-goroutine mutation can race with store serialization and persist inconsistent token state. Keep refresh mutations request-local (or deep-copy/synchronize storage updates) before writing through the shared pointer.
Useful? React with 👍 / 👎.
Updated QwenExecutor.Refresh to synchronize the new access_token and rotated refresh_token into the underlying storage object. This ensures that FileTokenStore correctly persists the latest credentials to disk, preventing "invalid token" errors after server restarts or cache misses. Added unit tests to verify both Metadata and Storage updates. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
aeddk-bnh
force-pushed
the
fix/qwen-token-persistence
branch
from
bf70c15 to
c225da9
Compare
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary:
This PR improves the local QwenExecutor.Refresh() state update logic, but it still does not fix the persistence path described in the PR.
Blocking findings:
- The 429 immediate-retry flow refreshes only the cloned
Authpassed intoexecutor.Execute(). That refreshed copy is never written back throughManager.Update(), so the latest tokens are still not persisted for the long-lived manager state or for restart recovery. - After restart,
FileTokenStore.readAuthFile()reconstructs Qwen auths fromMetadataonly and leavesStorageunset. Updating/cloningauth.StorageinsideQwenExecutor.Refresh()therefore does not address the restart/cache-miss scenario called out in the PR description.
Non-blocking findings:
FileTokenStore.Save()already callsSetMetadata(auth.Metadata), andMergeMetadata()applies metadata after serializing the storage struct. That means metadata already wins during file writes, so the new storage mutation is not the critical missing piece.- The new tests validate executor-local storage mutation, but they do not cover the manager/store integration path where persistence actually happens.
Test plan:
- Not run; review based on code inspection of the refresh, manager, and filestore paths.
3 participants
Summary
Updated
QwenExecutor.Refreshto synchronize the newaccess_tokenand rotatedrefresh_tokeninto the underlying storage object. This ensures thatFileTokenStorecorrectly persists the latest credentials to disk, preventing "invalid token" errors after server restarts or cache misses.Key Changes
auth.Storage.refreshTokensfunction toQwenExecutorfor easier unit testing.qwen_executor_test.goto verify storage updates and graceful fallback when storage is nil.Test plan
go test -v ./internal/runtime/executor/qwen_executor_test.go- PASS.QwenTokenStoragestruct is updated when present.auth.Storageis nil.🤖 Generated with Claude Code