Repository navigation
🤖 fix: report an unreadable 2xx create answer as an uncertain 504 - #241
Conversation
#163) Change-Id: Idb95ec07d36db8f82ba80ead3a0bcdc6f6c233f8 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![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: aebc7778d1
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…reate docs Change-Id: I43e7fae2c7dde08c7ac7be4dfb2cb1fedd0009db Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0350910c0d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Change-Id: I58136a816d42bfbae8247eaf8b1492d1213474b3 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52d51bc5ec
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1 participant
Summary
When a create of a
coderworkspaceor acodertemplategets a 2xx answer from Coder that the SDK cannot read, the aggregated API server now returns504with an "uncertain, re-read before you retry" message. It used to return a plain500that contained the Coder URL. This is note (b) from the final review of #163.Refs #163
Background
Since codersdk v2.37 (#163), a 2xx response whose body is not JSON is a
*codersdk.Errorwith that 2xx status. For example, a proxy in front of Coder can send an HTML page. A non-201 2xx on a POST that expects 201 was already such an error before.MapCoderErrorsends any 5xx-or-other status to itsdefaultbranch, so a create answered this way became500 InternalError, witherr.Error()as its text (POST https://<coder>/api/v2/...: invalid API response (status code 200) ...).But a 2xx means the POST got through. Coder can have created the object. A plain
500does not tell the client to check that before it retries. The docs already promise "After a504on a write, the result is not certain. Re-read the object withkubectl getbefore you retry". Workspace start and stop already answer an uncertain504for the same 2xx case (isUncertainStatusinworkspace_transition.go, #163).Note (a) from the same review is not in this PR. It does not apply, because go-sdk v1.7.0 marks
StreamableHTTPOptions.CrossOriginProtectionDeprecatedand recommends the outerhttp.NewCrossOriginProtection().Handler(...)wrapper thatinternal/app/mcpapp/http.goalready uses (vendor/github.com/modelcontextprotocol/go-sdk/mcp/streamable.go:184-196).Implementation
coder.MapCoderCreateError(internal/aggregated/coder/errors.go). A*codersdk.Errorwith a 2xx status becomesapierrors.NewTimeoutError(..., 0):504, reasonTimeout. The message names the resource, the object and the status, and says that the result is uncertain and that the client must re-read the object before it retries. It leaves out the Coder URL.RetryAfterSecondsis0, so the server sends noRetry-Afterheader and client-go does not send the create again on its own. Every other error maps exactly as inMapCoderError.CreateUserWorkspaceand bothCreateTemplatecalls (with and withoutspec.files). Server-side apply create-on-update calls the sameCreate, so it is covered too.CreateTemplateare unchanged. When they fail, the template does not exist yet, so a retry cannot duplicate it.docs/reference/aggregated-api-behavior.mddescribe the new504.Validation
TestMapCoderCreateError: 200, 201 and 204 map to504with the uncertain message, without the URL and without Retry-After. 409, 400, 502, 300 and a transport error map the same asMapCoderError.TestCreateWithUnreadable2xxAnswerIsUncertain: a test proxy lets the mock Coder create the object, then answers 200 or 201 with an HTML page. For a workspace, a template without files and a template with files, the create returns the uncertain504, and the object exists in the mock Coder. Onmain, each case fails withInternal error occurred: POST http://127.0.0.1:<port>/api/v2/.... I also reverted each of the three call sites alone and saw its case fail.Risks
Low. Only creates whose Coder answer is a 2xx
*codersdk.Errorchange, from500to504. Both are server errors, and neither carriesRetry-After. The status reason changes fromInternalErrortoTimeout.Generated with
mux• Model:anthropic:claude-opus-5-5• Thinking:high