Skip to content

🤖 fix: delete Coder providers and MCP configs only by recorded ID (#422 items 1, 4, 5) - #423

Merged
ThomasK33 merged 4 commits into
mainfrom
mcp-provider-id-only-delete
Oct 30, 2026
Merged

ThomasK33 merged 4 commits into
mainfrom
mcp-provider-id-only-delete

Conversation

@ThomasK33

Copy link
Copy Markdown
Member

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 (the CoderAIProvider sweep, the CoderAIProvider live delete, and the CoderMCPServer live delete) deletes nothing, keeps the cleanup record, and the sweep reports it with the gauge coder_k8s_cleanup_records_unmatched and a ManualCleanupNeeded Event. A CoderAIProvider now reports CoderDeploymentChanged instead of DeletedExternally after a Coder database switch. A CoderMCPServer whose status lost its config ID takes its config back by the recorded ID instead of stalling with SlugConflict.

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 (deleteInCoder with 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 DeletedExternally after 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 reported SlugConflict on 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 is sweepKept.
  • CoderAIProviderReconciler.deleteInCoder: without an ID in status or record, it never deletes. It reports unproven for a provider of this name and type. deleteStep then releases the finalizer and keeps the record, as CoderMCPServer already did.
  • CoderAIProviderReconciler.deleteStep: an object without ID and create marker no longer drops an ID-less record. A NameConflict clears 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 MCP SlugConflict.
  • deleteMCPServerInCoder: a config with the slug is always unproven without an ID. The nonce no longer leads to a delete.
  • CoderAIProviderReconciler.providerGone: runs the new recordedDeploymentChanged first, which stalls with CoderDeploymentChanged when the record names another deployment. The stall message moved into aiProviderDeploymentChanged, 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's coderID. The recorded ID counts only after recordedDeploymentChanged confirms that the control plane still reaches the record's deployment. Otherwise the object stalls with CoderDeploymentChanged.
  • Docs: manage-ai-providers.md gets the delete rule, the ManualCleanupNeeded manual cleanup steps, and the CoderDeploymentChanged reason row. manage-mcp-servers.md gets 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, TestCoderAIProviderDeleteRecentCreate
  • TestCleanupSweepAIProviderWithoutProviderID/{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, TestCoderMCPServerRecreateDropsGoneID
  • TestCoderAIProviderDeploymentSwitchKeepsRecord/applied (item 4: got DeletedExternally)
  • TestCoderMCPServerKeepsRecordedIDWithoutStatusID/{create_marker_kept, status_lost_everything} (item 5: got SlugConflict)
  • 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, plus markdownlint-cli2 and cspell on the two changed docs.

Risks

  • Known gap, kept by decision: the create path still adopts a provider or config whose display name is the temporary display name 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 with NameConflict or SlugConflict keeps the window open until the conflict ends. Removing adoption would stall every lost create answer with a false NameConflict or SlugConflict. 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.
  • Records without a Coder ID now stay in more cases (a NameConflict after 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 with NoMatchingDeployment, as MCP records already do since 🤖 fix: keep recorded provider IDs and add cleanup records for MCP servers #419.
  • Behavior change for a rare case: when an object is deleted while its create is still unconfirmed (a lost create answer, normally confirmed by the next reconcile within seconds), the operator no longer deletes the provider or config it most likely created. A person must delete it by hand after the ManualCleanupNeeded Event. 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.
  • A provider sweep now keeps the record of a provider of the same name and type with any display name, where it dropped the record before. A person who owns such a provider sees a ManualCleanupNeeded Event and deletes the ConfigMap.
  • Item 5: a status that lost everything (also appliedGeneration) re-applies the spec to the recognized config, as any status loss with a kept configID already does. A dashboard kill switch that only the lost status remembered is then undone.
  • No change to coderprovisioner* files, API types, RBAC or manifests.

Generated with mux • Model: anthropic:claude-opus-5-5 • Thinking: xhigh

… 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>
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-30T21:17:35.788714Z f0125c2 PR opened
🔒 Security Review ✅ Completed 2026-10-30T21:21:51.011535Z f0125c2 Manual request
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@ThomasK33

Copy link
Copy Markdown
Member Author

@codex security review

@chatgpt-codex-connector

Copy link
Copy Markdown

🛡️ Codex Security Review

Security review completed. No security issues were found in this pull request.

Reviewed commit: f0125c2154

View security finding report

Only the user who started this review can view the report in Codex.

ℹ️ About Codex security reviews in GitHub

This is an experimental Codex feature. Security reviews are triggered when:

  • You comment "@codex security review"
  • A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review

Once complete, Codex will leave suggestions, or a comment if no findings are found.

@ThomasK33
ThomasK33 added this pull request to the merge queue Oct 30, 2026
Merged via the queue into main with commit 526652b Oct 30, 2026
12 of 13 checks passed
@ThomasK33
ThomasK33 deleted the mcp-provider-id-only-delete branch October 30, 2026 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant