fix: retry Codex refresh on token invalidation#4088
Conversation
|
This pull request targeted The base branch has been automatically changed to |
![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 introduces a token refresh and retry mechanism for the Codex executor when an authentication token is invalidated (indicated by a 401 response with a token invalidation message). This retry logic is implemented across the Execute, executeCompact, and ExecuteStream methods, and is accompanied by comprehensive unit tests. The review feedback highlights a critical issue where a failed retry request could overwrite the response variable with nil, leading to a nil pointer dereference panic in deferred body close operations. It is recommended to use temporary variables to safely handle the retry response before reassignment.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| httpResp, err = httpClient.Do(retryReq) | ||
| if err != nil { | ||
| helps.RecordAPIResponseError(ctx, e.cfg, err) | ||
| return resp, err | ||
| } |
There was a problem hiding this comment.
If httpClient.Do(retryReq) returns an error, httpResp will be assigned nil. Since the deferred function registered at line 830 calls httpResp.Body.Close() without a nil check, this will result in a nil pointer dereference panic.
To prevent this, use a temporary variable to capture the result of the retry request, and only reassign httpResp if the request succeeds.
| httpResp, err = httpClient.Do(retryReq) | |
| if err != nil { | |
| helps.RecordAPIResponseError(ctx, e.cfg, err) | |
| return resp, err | |
| } | |
| retryResp, retryErr := httpClient.Do(retryReq) | |
| if retryErr != nil { | |
| helps.RecordAPIResponseError(ctx, e.cfg, retryErr) | |
| return resp, retryErr | |
| } | |
| httpResp = retryResp |
| httpResp, err = httpClient.Do(retryReq) | ||
| if err != nil { | ||
| helps.RecordAPIResponseError(ctx, e.cfg, err) | ||
| return resp, err | ||
| } |
There was a problem hiding this comment.
If httpClient.Do(retryReq) returns an error, httpResp will be assigned nil. Since the deferred function registered at line 1056 calls httpResp.Body.Close() without a nil check, this will result in a nil pointer dereference panic.
To prevent this, use a temporary variable to capture the result of the retry request, and only reassign httpResp if the request succeeds.
| httpResp, err = httpClient.Do(retryReq) | |
| if err != nil { | |
| helps.RecordAPIResponseError(ctx, e.cfg, err) | |
| return resp, err | |
| } | |
| retryResp, retryErr := httpClient.Do(retryReq) | |
| if retryErr != nil { | |
| helps.RecordAPIResponseError(ctx, e.cfg, retryErr) | |
| return resp, retryErr | |
| } | |
| httpResp = retryResp |
| httpResp, err = httpClient.Do(retryReq) | ||
| if err != nil { | ||
| helps.RecordAPIResponseError(ctx, e.cfg, err) | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
For consistency and defensive programming, use a temporary variable to capture the result of the retry request here as well. This avoids reassigning the outer httpResp and err variables before we know if the call succeeded.
| httpResp, err = httpClient.Do(retryReq) | |
| if err != nil { | |
| helps.RecordAPIResponseError(ctx, e.cfg, err) | |
| return nil, err | |
| } | |
| retryResp, retryErr := httpClient.Do(retryReq) | |
| if retryErr != nil { | |
| helps.RecordAPIResponseError(ctx, e.cfg, retryErr) | |
| return nil, retryErr | |
| } | |
| httpResp = retryResp |
![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: 7457833980
ℹ️ 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 errClose := httpResp.Body.Close(); errClose != nil { | ||
| log.Errorf("codex executor: close response body error: %v", errClose) | ||
| } | ||
| httpResp, err = httpClient.Do(retryReq) |
There was a problem hiding this comment.
When the retry request fails at the transport layer after the initial token_invalidated 401 (for example a proxy error or context cancellation), this assignment replaces the successful first httpResp with the nil response returned by Do. The deferred close above then executes httpResp.Body.Close() and panics instead of returning the retry error; use a separate retry response variable or close the first response before reassigning. The compact retry path has the same pattern.
Useful? React with 👍 / 👎.
| if httpResp.StatusCode >= 200 && httpResp.StatusCode < 300 { | ||
| goto codexReadExecuteResponse | ||
| } | ||
| b, _ = io.ReadAll(httpResp.Body) |
There was a problem hiding this comment.
When the refreshed retry returns a non-2xx invalid_encrypted_content or invalid thinking-signature error, this branch reads and returns the retry body without running clearCodexReasoningReplayOnInvalidSignature, unlike the initial error path above. That leaves the bad cached reasoning replay in place after a token-invalidated first attempt, so subsequent requests can keep replaying the same invalid block instead of self-healing; run the same replay-clear check on retry error bodies before returning.
Useful? React with 👍 / 👎.
1 participant
Summary
401 token_invalidatedresponsesCodexExecutor.Refresh()path before suspending/falling backCloses #4087
Test plan
go test ./internal/runtime/executor -run 'TestCodexExecutorExecuteRefreshesAndRetriesTokenInvalidated|TestCodexExecutorExecuteStreamRefreshesAndRetriesTokenInvalidated|TestIsCodexTokenInvalidatedResponse' -count=1go test ./internal/runtime/executor -count=1go test ./... -count=1token_invalidatedimmediately suspended/fell backtoken_invalidatedcalled refresh first, then fell back only after refresh returnedrefresh_token_invalidated