Repository navigation
🤖 fix: hold a deleted CoderControlPlane while CoderMCPServer objects still use it - #384
Conversation
|
@codex security 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. |
![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: a4409ed173
ℹ️ 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".
🛡️ 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. |
Change-Id: Id1e2e4a197291d2db02e0bea3e729702cc882a80 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: 05a47b6e2b
ℹ️ 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".
…at the hold end Change-Id: Ic019bddbfdcbe5c7e323f5dfe9b4409ac63d260e Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cf8651d124
ℹ️ 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".
🛡️ 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. |
… of the pass Change-Id: I8de8cb92e215f1879c2192913ffe49c83a84f4ee Signed-off-by: Thomas Kosiewski <tk@coder.com>
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. 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. |
1 participant
Summary
A deleted
CoderControlPlanenow waits for theCoderMCPServerobjects that use it, as #362 made it wait forCoderAIGatewayKeyandCoderAIProviderobjects. Coder keeps running while it waits, so each MCP server delete reaches Coder and retries on failure. Before this PR the control plane went at once. The MCP server then made one 10-second delete attempt or released withControlPlaneGone, and its config could stay in Coder, enabled and with its stored credential.What changes
controlPlaneDependents(internal/controller/controlplane_hold.go) also listsCoderMCPServerobjects that reference the control plane and are being deleted with their cleanup finalizer, plus live ones for the first 30 seconds, the same rule as the other two kinds. The per-kind count for the grace period covers the new kind, and the grace period uses the largest bounded delete timeout of the three kinds (all are 10 s).requestForDependentControlPlanemaps aCoderMCPServerto its control plane, and the control plane controller watchesCoderMCPServerremovals (dependentRemoved), so a waiting control plane goes at once.CoderMCPServerdelete path checkscontrolPlaneHolds, as theCoderAIProviderdelete path does. While the control plane holds for it, it takes the normal path (retry every 60 s, capped at the hold end) instead of one bounded attempt and a release. Without this step, the hold would keep Coder running but a failed attempt would still release at once.Under the hold, the pass runs with a context deadline at the hold end, so a slow Coder cannot stretch it past the hold. The bounded pass after the hold then fits in the grace period. The retry counts from the end of the pass, so it lands right after the hold end (review rounds 2 and 3).
access.groupsthat is not a valid Coder group name stalls withGroupNotFoundand no requeue (codermcpserver_acl.go:147). The doc said that it checks again every 10 minutes. It now says that the object is not checked again until the spec changes. The API server refuses such names, so this applies only when admission was bypassed.Validation
TestCoderMCPServerDeleteUnderControlPlaneHold(envtest): with a control plane that is being deleted and an appliedCoderMCPServerthat is being deleted, the control plane keeps its finalizer. A failed DELETE (502) keeps the MCP finalizer withCoderUnavailableand a 60 s requeue, and the control plane still waits. The retry deletes the config, the MCP object goes, and then the control plane goes. Red before the fix: "the control plane waits for the MCP server" failed.TestMCPServerDeleteUnderHoldEndsAtHoldEnd(unit, fake client): with a hold that ends in 5 s and a Coder that hangs, the pass ends near the hold end withCoderUnavailable, keeps the finalizer, and requeues right after the hold end. Without the deadline it waits for the 30 s request timeout (mutation checked).TestRequestForDependentControlPlane(unit): each of the three dependent kinds maps to its control plane, and another kind maps to nothing.CoderMCPServercase of the mapper fails the unit test, forcing the bounded path in the MCP delete fails the envtest test, and dropping the hold-end deadline fails the unit test above.TestCoderMCPServerDeleteWhileControlPlaneDeletes,TestControlPlane*,TestCoderAIProviderDelete*andTestCoderAIGatewayKeyDelete*tests pass unchanged.859d6bc7):make verify-vendor,make build,make lint,GOFLAGS=-mod=vendor go tool govulncheck ./...,make test,make test-integration,go test -race ./internal/controller/, andmake docs-checkpass.make codegen,make manifests,make build-installerandmake docs-referenceleave no diff. RBAC does not change: the manager role already grantsget,listandwatchoncodermcpservers.internal/controller/codermcpserver*andcontrolplane_hold.go).Not in this PR
docs/how-to/manage-ai-providers.mdnames onlyCoderAIProviderandCoderAIGatewayKeyin its hold sentence. That page describes providers, and 🤖 feat: write a cleanup record before each CoderAIProvider create (#353 part 2a) #374 edits it, so I left it.coder_k8s_cleanup_released_without_delete_total, and Namespace deletion leaves AI Gateway keys and AI providers in Coder #353 part 2 (cleanup records and the sweep, 🤖 feat: write a cleanup record before each CoderAIProvider create (#353 part 2a) #374 and 🤖 feat: sweep the cleanup records of CoderAIProvider objects that went without a delete (#353 part 2b) #375) covers onlyCoderAIProvider. Both are follow-ups for Namespace deletion leaves AI Gateway keys and AI providers in Coder #353.Size
199 changed lines: 65 in the controllers, 129 in tests and 5 in the how-to.
Fixes #382
Refs #353
Refs #155
Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$224.47