Repository navigation
🤖 fix: map uncertain aggregated write answers to 504 and list unconfigured namespaces as empty - #400
Conversation
…cts (#242) Change-Id: Ia77616f5949c1930986af2594cf3e69bc22a302c Signed-off-by: Thomas Kosiewski <tk@coder.com>
…215) Change-Id: I2d537eae9e3e647dfad15af6254d8085d822b967 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Ia37d86a95b5f36db95709ce44e9634f8e5c6e33d Signed-off-by: Thomas Kosiewski <tk@coder.com>
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. |
![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: b805079eb8
ℹ️ 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".
…ctivation (#242) Change-Id: I85e0bc17b3bfee10045dcab28cfdfda54964bc47 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…as uncertain (#242) Change-Id: Ia6c1baa2a623621d15d164aebcee59e61d110260 Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Ia39194d4f638f04bc41b3aa96a2c561a2a717d7d 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: 129100d5f7
ℹ️ 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: I6de82486d0068fd4926491e4c034628c8961d47f Signed-off-by: Thomas Kosiewski <tk@coder.com>
…data updates (#242) Change-Id: Ica84f8d8d1278380b31468136cd4cd69732fec05 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@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. |
…242) Change-Id: I0031e0f8010e79857ecedb00ed336c5083b313ec 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: 46fa605e79
ℹ️ 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".
…in (#242) Change-Id: I95cee7af345dee1e705b0c25450e311c19c11572 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: 0afaf7bf28
ℹ️ 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".
… 500 (#242) Change-Id: I759db7afb294e606fe1a562a962db311544ac22e Signed-off-by: Thomas Kosiewski <tk@coder.com>
Change-Id: Ifad7d7460c03785f2397b1aabcccb4d6474eceda 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: 0251b23ff6
ℹ️ 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".
…ain-writes Change-Id: Ic53ba17ad92131a37074dbf921b6c2132c192d21 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ 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. |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ 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. |
…itten (#242) Remove the TLS alert classification from notSent: a Go TLS server can send protocol_version after it read the whole request. Mark the error of a round trip that wrote request headers on any attempt, so an HTTP/2 retry that fails to redial returns the uncertain 504. Change-Id: Ic5276b3952575ba9c21f4e61ffffb53d29849863 Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ 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. |
Summary
Aggregated writes that get an answer which leaves the result open now return the uncertain
504("re-read the object with kubectl get before you retry") without the Coder URL. Coder objects without the fields of their name now return a clean500instead of a panic. An unconfigured standalone server now returns empty namespaced lists, so namespace deletion does not hang on it.Fixes #242
Fixes #215
Background
Users want every open issue fixed and landed on
main. This PR closes two of them.502/503, a2xxbody that breaks off (read response body: ...), malformed JSON on an unexpected200(decode body: ...), or a connection that closes after the send returned500 InternalError. Coder can have applied the write, so clients that retry can create duplicates. A201with{}ornullmadeassertNameSegmentpanic in the converters (case 5). The same panic hit GET and LIST answers.errClientProvider(standalone mode without--coder-urland--coder-session-token) returned a plain503for namespaced LISTs, so the namespace controller could not finish a namespace deletion.Terminating", step 2): both are already onmain. Step 2 says "If the message namesaggregation.coder.com" and links themultiple eligible CoderControlPlaneentry. No change needed.Implementation
coder.MapCoderWriteError(err, write, resource, name)replacesMapCoderCreateErrorand maps the error of every Coder mutation call:CreateUserWorkspace,CreateWorkspaceBuild(update, delete),CreateTemplate, the activation PATCH of a files-changing template update,UpdateTemplateMeta,DeleteTemplate.patchActiveTemplateVersionhelper of the promote subresource, becausecodersdk.UpdateActiveTemplateVersionreturnsnilon transport errors.201 {}) returns the uncertain504. So does a template metadata update whose answer is not the updated template.504: a*codersdk.Errorwhose status passescoder.IsUncertainStatus(2xx, 502, 503; moved fromworkspace_transition.go, which now calls it), and every error without a status that the request can have caused after the send.MapCoderError): timeouts and a Coder504(already a504), statuses that leave no doubt, and errors that prove the request never reached Coder (notSent): a*net.OpErrorwithOpdialorproxyconnect(covers DNS and refused connections), a proxy that refuses theCONNECTtunnel (the Coder transport now setsOnProxyConnectResponseto mark it witherrProxyConnectRefused, because Go reports it with an untyped error), a TLS certificate error, a TLS record-header error for a first record that is not a TLS handshake (crypto/tlssetsRecordHeaderError.Connonly then, before the server hello; a malformed record after the handshake can follow a request that Coder read, so it returns the uncertain504),http.ErrSchemeMismatch(anhttpsURL answered by a plain HTTP server), and a request that thesameoriginround tripper refused before the send. An error of a request that followed a redirect is nevernotSent: the Coder HTTP client marks it witherrAfterRedirect, because the server that redirected got the write. An error of a round trip that wrote request headers on any attempt is nevernotSenteither: the client marks it witherrAfterWrite(anhttptraceWroteHeadershook). A received TLS alert is nevernotSent, because a Go TLS server can sendprotocol_versionafter it read the whole request. These keep the500"could not be reached". A refused cross-origin redirect is uncertain: the server that sent the redirect got the write.MapCoderWriteError, and a version answer without an ID returns the uncertain504instead of an assertion500.504instead of a500.504instead of passing the read error through as a500.codertemplateversionsLIST checks each template of the templates answer (ID and name fields) before it requests that template's versions. A malformed entry such as{}now returns the500instead of an empty list: Coder answers404for the zero ID, which the LIST skipped as a deleted template.coderworkspaces/start|stop: a build answer without a build ID (for example201 {}) now takes the existing confirming re-read instead of answeringQueuedwith an empty build.convert.TemplateToK8s,WorkspaceToK8sandTemplateVersionToK8snow return(obj, error). They reject a zero ID, a name field that is empty or contains.(via the newcoder.NameSegmentError), and invalid label values, with an error that starts withthe Coder API answered. Storage maps that error to500on reads, and to the uncertain504on a create answer. Workspace create checks the answer before the stop build, which needs the workspace ID.errClientProvidermarks namespaced errors with the now exportedcoder.NewNamespaceNotServedError. GET and other verbs keep the503. The all-namespaces LIST keeps the503.docs/reference/aggregated-api-behavior.md("Coder timeouts" and "Namespaces without a Coder backend") and a504entry indocs/how-to/troubleshooting.md.Validation
Failing tests land first (
430c9ccb46,a48c06dfc6), then the fix.internal/aggregated/storage/write_outcome_test.go: table test of 7 writes (workspace create/update/delete, template create/metadata update/activation of new files/delete) against a proxy in front of the mock Coder that applies the write, then answers 502, 503, closes the connection, cuts the body, answers200with malformed JSON,201with{}/null, or Coder's answer without itsid. Each returns the uncertain504without the Coder URL. A refused connection before the send returns500"could not be reached".internal/aggregated/storage/coder_answer_test.go: workspace LIST, template LIST, template GET, template version LIST with nameless objects return500, no panic.internal/app/apiserverapp/coder_answer_route_test.go: the same outcomes through the production handler chain (DELETE, PUT, POST, LIST, GET over HTTPS). Before the fix, the template create with{}killed the connection (EOF).internal/app/apiserverapp/unconfigured_namespace_test.go: unconfigured standalone server, namespaced LIST of all three resources returns an empty list, GET and the all-namespaces LIST keep503.TestMapCoderWriteError(error shapes),TestConvertersRejectAnswersWithoutNames.internal/aggregated/coder/errors_transport_test.go: real SDK writes through the production Coder transport. A refused connection, a rejected certificate, a refused proxy connection, a refused cross-origin request, a proxyCONNECTanswered407, and anhttpsURL answered by plain HTTP never reach the handler and return500. A server that refuses the TLS version or the cipher suites also never gets the request, but the alert does not prove that, so it returns the uncertain504. A connection closed after the handler got the request returns the uncertain504. The test landed first (1d21ac3844): the four new cases failed with the uncertain504. A same-origin redirect whose next connection is refused returns the uncertain504(test first ind7e4c097).write_outcome_test.gogains the template version POST of a files update, and of a template create (failed first inbcf31d31).TestWorkspaceTransitionUncertainPostgains201with{}(confirmed by the re-read) and201withnull(uncertain504). Both landed failing ind7e4c097.TestCoderAnswersWithoutNamesFailCleanlygains a version LIST whose templates answer is[{}](failed first in264287e2: it returned an empty list).TestTemplateFilesUpdateReReadAfterActivation: after the activation, a re-read answered502, closed, refused, or with another active version returns the uncertain504(failed first in7ed56662).e1e1ce4b, fix ina8ad8ebd):TestMapCoderWriteErrorRealTransportgains a TLS server that reads the whole POST after the handshake and answers with a record of the wrong version or an oversized record; both returned the500and now return the uncertain504.TestTemplateUpdateReadAfterChange: after a metadata PATCH or a confirmed activation, a502on the final read returned the500and now returns the uncertain504.8aa903d0):TestMapCoderWriteErrorRealTransportgains TLS 1.2 and TLS 1.3 servers that read the whole POST, then send aprotocol_versionalert (the verifier's probe), and an HTTP/2 server that reads the whole POST, then sends GOAWAY andRST_STREAM(PROTOCOL_ERROR). Go's HTTP/2 client sends the POST again on a new connection, and the refused dial returned the500. All three now return the uncertain504. Negative controls: with the oldnotSent, the two alert cases and the two TLS refusal cases fail; without theerrAfterWritecheck, the HTTP/2 case fails withdial tcp ...: connection refusedmapped to500. A temporary log confirmed thatWroteHeadersfires for HTTP/1.1 and HTTP/2 and that no pre-send case is marked.make verify-vendor,make test,make build,make lint,make docs-check(innix develop): all pass.Risks
encode body) now returns the uncertain504instead of500. These errors do not happen with the request types used here. A false "uncertain" costs one re-read. A false "certain" can cause a duplicate, so the default is uncertain.notSentdoes not recognize (for example every received TLS alert, including a TLS version or cipher refusal, or a proxy that breaks theCONNECTexchange) return the uncertain504, even when the request never reached Coder. The docs say so.3xxwithout aLocationheader reaches the SDK as a*codersdk.Errorwith a 3xx status, and a write maps it to the500, not the uncertain504. Rare, and it reverses the current3xxmapping, so the maintainer files a follow-up issue after merge.504does not write the original error to thecoder-errorslog. Operators lose the status or transport detail. The maintainer files a follow-up issue after merge.spec.running=falseinstalls the stop build answer without checking its ID, so a201 {}from a proxy reports an empty latest build. The fix is a nonzero build-ID check, like the start/stop path. The maintainer files a follow-up issue after merge.coderworkspaces/start|stop) keep their own confirming re-read. Only the answer without a build ID changed.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:medium