Repository navigation
🤖 fix: delete Coder providers and MCP configs only by recorded ID (#422 items 1, 4, 5) - #423
Merged
Merged
Conversation
… items 4 and 5 Refs #422 Change-Id: I9e7ac05664d59ab6e7b8edadadff1d7517d3884e Signed-off-by: Thomas Kosiewski <tk@coder.com>
…ymentChanged for providers Without a recorded ID, the CoderAIProvider sweep and both live delete paths no longer delete a provider or MCP server config whose display name is the temporary nonce: a person can edit display names. The record stays and the sweep reports it with ManualCleanupNeeded. A CoderAIProvider whose control plane reaches another Coder deployment now reports CoderDeploymentChanged instead of DeletedExternally, and a CoderMCPServer whose status lost its config ID recognizes its config by the recorded ID instead of stalling with SlugConflict. Refs #422 Change-Id: I5346eac9b37454579e102d5a8b52c550fb808bde Signed-off-by: Thomas Kosiewski <tk@coder.com>
…yment before recorded-ID recovery Refs #422 Change-Id: Iaf70ca73c8056032dcb3a7c18a63dd838781a251 Signed-off-by: Thomas Kosiewski <tk@coder.com>
…eployment before recorded-ID recovery Refs #422 Change-Id: I320c0b1544475654f0a085c8ffd32e98a5003899 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. |
Member
Author
|
@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. |
This was referenced Oct 30, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The operator no longer deletes a Coder AI provider or MCP server config because its display name is the operator's temporary display name (
coder-k8s:<object UID>). Without a recorded Coder ID, every delete path (theCoderAIProvidersweep, theCoderAIProviderlive delete, and theCoderMCPServerlive delete) deletes nothing, keeps the cleanup record, and the sweep reports it with the gaugecoder_k8s_cleanup_records_unmatchedand aManualCleanupNeededEvent. ACoderAIProvidernow reportsCoderDeploymentChangedinstead ofDeletedExternallyafter a Coder database switch. ACoderMCPServerwhose status lost its config ID takes its config back by the recorded ID instead of stalling withSlugConflict.Refs #422: items 1, 4 and 5 are done. Items 2, 3 and 6 stay documented limits or decisions.
Background
Coder lets a person edit the display name of a provider and the slug and display name of an MCP server config. So a resource that a person created, or renamed, with the temporary display name could be deleted by the operator. #413 (provisioner keys) and #419 (MCP sweep) adopted the rule "a record without a recorded ID never deletes". This PR applies it to the remaining paths. Issue #422 lists the provider sweep and the MCP live delete. The provider live delete (
deleteInCoderwith only a create marker) did the same nonce match, so it gets the same rule.Item 4: #419 taught the MCP sync to check the record's deployment on Coder's 404. The provider sync did not, so an applied provider reported
DeletedExternallyafter the control plane reached a new database.Item 5: after a status loses
configID, the config's display name is the spec's, not the nonce, so the slug lookup reportedSlugConflicton the object's own config, although the cleanup record still held its ID.Implementation
sweepAIProvider(cleanup_sweep.go): without a recorded ID, Coder's 404 for the name drops the record. A provider of that name with another type also drops it (Coder cannot change a provider's name or type). Any other provider of that name issweepKept.CoderAIProviderReconciler.deleteInCoder: without an ID in status or record, it never deletes. It reportsunprovenfor a provider of this name and type.deleteStepthen releases the finalizer and keeps the record, asCoderMCPServeralready did.CoderAIProviderReconciler.deleteStep: an object without ID and create marker no longer drops an ID-less record. ANameConflictclears the marker also when a person edited the display name of the provider of a lost create answer, so the record stays and the sweep decides, as 🤖 fix: keep recorded provider IDs and add cleanup records for MCP servers #419 did for the MCPSlugConflict.deleteMCPServerInCoder: a config with the slug is alwaysunprovenwithout an ID. The nonce no longer leads to a delete.CoderAIProviderReconciler.providerGone: runs the newrecordedDeploymentChangedfirst, which stalls withCoderDeploymentChangedwhen the record names another deployment. The stall message moved intoaiProviderDeploymentChanged, shared with the record write.createOrRecord/recordMCPServer: a config with the slug is this object's when its display name is the nonce or its ID is the record'scoderID. The recorded ID counts only afterrecordedDeploymentChangedconfirms that the control plane still reaches the record's deployment. Otherwise the object stalls withCoderDeploymentChanged.manage-ai-providers.mdgets the delete rule, theManualCleanupNeededmanual cleanup steps, and theCoderDeploymentChangedreason row.manage-mcp-servers.mdgets the live delete rule.Validation
The test commits come before their fixes. On
ae231a98(main) these fail:TestCoderAIProviderTemporaryDisplayNameDeletesNothing(live delete, sweep, gauge, manual cleanup end to end)TestCoderAIProviderDeletePendingCreate/a_person's_provider_with_the_temporary_display_name,TestCoderAIProviderDeleteRecentCreateTestCleanupSweepAIProviderWithoutProviderID/{lost_create_answer, lost_create_answer,_display_name_edited, a_person's_provider_with_the_temporary_display_name}TestCoderMCPServerDeletePendingCreate/a_person's_config_with_the_temporary_display_name,TestCoderMCPServerRecreateDropsGoneIDTestCoderAIProviderDeploymentSwitchKeepsRecord/applied(item 4: gotDeletedExternally)TestCoderMCPServerKeepsRecordedIDWithoutStatusID/{create_marker_kept, status_lost_everything}(item 5: gotSlugConflict)TestCoderAIProviderDeleteKeepsRecordAfterNameConflict(the record was dropped while the provider stayed)TestCoderMCPServerRecordedIDNeedsRecordedDeployment(fails on the first fix commit: the recorded ID was taken in another deployment)Controls that pass on main and after:
.../create_never_committed,.../a_person's_provider_of_another_type,TestCoderAIProviderDeploymentSwitchKeepsRecord/pending_spec_edit. All pass on the head.Local gates:
make verify-vendor,make build,make lint,make test,make test-integration,nix develop -c make docs-check, plusmarkdownlint-cli2andcspellon the two changed docs.Risks
coder-k8s:<object UID>. Adoption is the only way to learn the ID of a create whose answer was lost (🤖 feat: create the Coder provider for a CoderAIProvider #253), or of a create that committed after its marker was cleared. The operator records that ID, applies the spec to the resource, and deletes it by that ID when the object goes. So a person who gives a resource that exact display name (with the object's provider name or MCP slug) while the object has no Coder ID in its status can get it adopted and later deleted. After a lost create answer, that window normally ends at the next successful read, about 5 seconds later. An object that stalls withNameConflictorSlugConflictkeeps the window open until the conflict ends. Removing adoption would stall every lost create answer with a falseNameConflictorSlugConflict. Planned follow-up: make the temporary display name a random secret that the operator stores in the cleanup record, instead of the object UID, so that a person cannot produce it by accident or from the object's metadata.NameConflictafter a create, a create that Coder rejected). When a control plane of the record's deployment is ready, the sweep removes them after its 5-minute settle window once Coder holds no matching resource. After a namespace deletion they wait withNoMatchingDeployment, as MCP records already do since 🤖 fix: keep recorded provider IDs and add cleanup records for MCP servers #419.ManualCleanupNeededEvent. This is the trade 🤖 fix: revoke a CoderProvisioner key created at an intermediate control plane endpoint #413 and 🤖 fix: keep recorded provider IDs and add cleanup records for MCP servers #419 already made.ManualCleanupNeededEvent and deletes the ConfigMap.appliedGeneration) re-applies the spec to the recognized config, as any status loss with a keptconfigIDalready does. A dashboard kill switch that only the lost status remembered is then undone.coderprovisioner*files, API types, RBAC or manifests.Generated with
mux• Model:anthropic:claude-opus-5-5• Thinking:xhigh