fix(qwen): align auth polling interval and refresh lead#1924
fix(qwen): align auth polling interval and refresh lead#1924shenshuoyaoyouguang wants to merge 3 commits intorouter-for-me:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refines the Qwen authentication process by synchronizing token polling intervals with refresh lead times, which mitigates issues of premature token expiry and refresh timing discrepancies. It introduces a new internal method for handling HTTP POST requests, enabling better control over the HTTP client used during token polling. Additionally, comprehensive unit tests have been added to validate both the internal authentication logic and the SDK's refresh lead configuration. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 aligns Qwen authentication polling timing and adds test coverage. However, a potential path traversal vulnerability was identified in sdk/auth/qwen.go where user-supplied email input is used to construct a file path without sanitization. While the refactoring of an HTTP POST call into a new helper method is a solid improvement for testability and a fix for the refresh lead time is included, consider adding support for context.Context to the new helper method to allow for request cancellation, which would enhance robustness and align with existing patterns.
internal/auth/qwen/qwen_auth.go
Outdated
| func (qa *QwenAuth) postForm(endpoint string, data url.Values) (*http.Response, error) { | ||
| client := qa.httpClient | ||
| if client == nil { | ||
| client = http.DefaultClient | ||
| } | ||
| req, err := http.NewRequest(http.MethodPost, endpoint, strings.NewReader(data.Encode())) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") | ||
| req.Header.Set("Accept", "application/json") | ||
| return client.Do(req) | ||
| } |
There was a problem hiding this comment.
The new postForm helper function is a good abstraction. However, it should accept a context.Context to allow for request cancellation and timeouts. The http.NewRequest call should be replaced with http.NewRequestWithContext.
This change would also imply updating the calling function, PollForToken, to accept and pass down a context. This is a best practice for long-running operations like polling, especially since its caller, sdk/auth/qwen.go:Login, already has a context available that is not being utilized for this operation.
Many other network-calling functions in this package, such as RefreshTokens, already use a context. Aligning PollForToken and postForm with this pattern would improve consistency and robustness.
| func (qa *QwenAuth) postForm(endpoint string, data url.Values) (*http.Response, error) { | |
| client := qa.httpClient | |
| if client == nil { | |
| client = http.DefaultClient | |
| } | |
| req, err := http.NewRequest(http.MethodPost, endpoint, strings.NewReader(data.Encode())) | |
| if err != nil { | |
| return nil, err | |
| } | |
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") | |
| req.Header.Set("Accept", "application/json") | |
| return client.Do(req) | |
| } | |
| func (qa *QwenAuth) postForm(ctx context.Context, endpoint string, data url.Values) (*http.Response, error) { | |
| client := qa.httpClient | |
| if client == nil { | |
| client = http.DefaultClient | |
| } | |
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, strings.NewReader(data.Encode())) | |
| if err != nil { | |
| return nil, err | |
| } | |
| req.Header.Set("Content-Type", "application/x-www-form-urlencoded") | |
| req.Header.Set("Accept", "application/json") | |
| return client.Do(req) | |
| } |
There was a problem hiding this comment.
Fixed in c84bdf7. postForm now accepts context.Context, PollForToken passes the caller context through, and the request is built with http.NewRequestWithContext so cancellation/timeouts propagate correctly.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily refactors the Qwen authentication process to incorporate context.Context into the PollForToken function and its callers, RequestQwenToken and Login, to enable better cancellation and timeout management. Key changes include modifying the PollForToken signature to accept a context, introducing a new postForm helper method to handle HTTP POST requests with context and a configurable HTTP client, and adding new tests for PollForToken's HTTP client usage and the RefreshLead method. Review comments indicate that the PollForToken's polling loop still uses time.Sleep without checking ctx.Done(), which could lead to goroutine leaks, and that the postForm method's fallback to http.DefaultClient lacks a default timeout, potentially causing indefinite hangs.
|
|
||
| // PollForToken polls the token endpoint with the device code to obtain an access token. | ||
| func (qa *QwenAuth) PollForToken(deviceCode, codeVerifier string) (*QwenTokenData, error) { | ||
| func (qa *QwenAuth) PollForToken(ctx context.Context, deviceCode, codeVerifier string) (*QwenTokenData, error) { |
There was a problem hiding this comment.
The PollForToken function's polling loop does not properly handle context.Context cancellation, specifically with time.Sleep calls (e.g., on lines 244, 267, 276). This is a vulnerability that can lead to goroutine leaks and blocking calls even after the context is canceled. To ensure full cancellability, replace time.Sleep(pollInterval) with a context-aware wait using a select statement that checks ctx.Done() and time.After(pollInterval).
internal/auth/qwen/qwen_auth.go
Outdated
| func (qa *QwenAuth) postForm(ctx context.Context, endpoint string, data url.Values) (*http.Response, error) { | ||
| client := qa.httpClient | ||
| if client == nil { | ||
| client = http.DefaultClient |
There was a problem hiding this comment.
The postForm method falls back to http.DefaultClient when qa.httpClient is nil. http.DefaultClient has no default timeout, which, combined with the use of context.Background() in some callers (like RequestQwenToken), can lead to indefinite hangs if the remote server or proxy becomes unresponsive.
| client = http.DefaultClient | |
| client = &http.Client{Timeout: 60 * time.Second} |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces context awareness to the Qwen device authentication flow, which is a good improvement for handling cancellable operations. It also includes a fix for the RefreshLead function and adds relevant test coverage. My review includes one suggestion to complete the context propagation within the polling loop by utilizing the newly introduced waitForNextPoll function.
| resp, err := qa.postForm(ctx, QwenOAuthTokenEndpoint, data) | ||
| if err != nil { | ||
| fmt.Printf("Polling attempt %d/%d failed: %v\n", attempt+1, maxAttempts, err) | ||
| time.Sleep(pollInterval) |
There was a problem hiding this comment.
You've introduced a waitForNextPoll helper function to make sleeping context-aware, which is a great improvement. However, the time.Sleep calls within PollForToken have not been updated to use it. To make the polling loop fully cancellable, you should replace the time.Sleep(pollInterval) calls with waitForNextPoll(ctx, pollInterval) and handle the potential context cancellation error.
This change should be applied to all instances of time.Sleep(pollInterval) in this function (lines 244, 252, 267, and 276).
if err := waitForNextPoll(ctx, pollInterval); err != nil {
return nil, err
}
|
Review Navigation: This PR has a smaller scope, mainly aligning qwen auth polling with refresh lead. Subsequent commits have added context cancellation and default timeout, current CI has passed, suitable for priority or mid-early order review. |
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary
Thanks for the focused fix and the added tests. I found one blocking correctness issue in the cancellation path.
Key findings
- Blocking:
PollForTokenstill retries afterctxcancellation.postFormnow usesNewRequestWithContext, but anyclient.Doerror is still treated as retryable, so the loop continues sleeping and retrying instead of returning promptly withctx.Err(). - Blocking:
waitForNextPollis defined but never used, so cancellation is not honored during the polling backoff either (authorization_pending/slow_downstill usetime.Sleep). - Non-blocking: please add a regression test that cancels the context and verifies
PollForTokenexits immediately.
Test plan
- Reviewed the PR diff and current polling call paths
- Did not run code locally
2 participants
Summary
Business value
Tests