From e0cf50167a38ea2cf704ea1088a43329193d4a49 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 17:32:11 +0000 Subject: [PATCH 01/11] test: a status without the provider ID keeps the recorded ID (#353) Change-Id: I12b6b4f617c2057de880b3800b8cc9cf08502174 Signed-off-by: Thomas Kosiewski --- .../controller/coderaiprovider_record_test.go | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/internal/controller/coderaiprovider_record_test.go b/internal/controller/coderaiprovider_record_test.go index 0a21b382..f4840478 100644 --- a/internal/controller/coderaiprovider_record_test.go +++ b/internal/controller/coderaiprovider_record_test.go @@ -228,6 +228,30 @@ func TestCoderAIProviderDeleteUsesRecordedID(t *testing.T) { require.Nil(t, cleanupRecord(ctx, t, env.namespace, provider.UID)) } +// #410 review (r4426047301): a status without the provider ID, but with a +// create marker, must not drop the recorded ID. After the sync the display +// name is no longer the nonce, so only the recorded ID still finds the +// provider for a delete or a sweep. +func TestCoderAIProviderKeepsRecordedIDWithoutStatusID(t *testing.T) { + t.Parallel() + ctx := context.Background() + env, _, provider := newReadyAIProvider(ctx, t) + recordedID := provider.Status.ProviderID + provider.Status.ProviderID = "" + provider.Status.CreateAttemptTime = &metav1.Time{Time: time.Now().Add(-time.Hour)} + require.NoError(t, k8sClient.Status().Update(ctx, provider)) + + got, _ := env.reconcile(ctx, t, provider) + requireAIProviderStep(t, got, true, "NameConflict") + require.Equal(t, recordedID, cleanupRecord(ctx, t, env.namespace, got.UID)["coderID"], "the record keeps the provider ID") + + gone, _ := env.deleteAIProvider(ctx, t, got) + require.Nil(t, gone) + _, ok := env.fake.aiProvider("openai") + require.False(t, ok, "the delete used the recorded ID") + require.Nil(t, cleanupRecord(ctx, t, env.namespace, got.UID)) +} + // #374 review round 2: when the control plane now reaches another Coder // deployment (a new database), a delete there proves nothing. The finalizer // goes without a delete, and the record stays for a sweep. From 52c35cf69aecacc5465e5741d1a6c6965916faab Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 17:33:17 +0000 Subject: [PATCH 02/11] fix: keep the recorded provider ID when the status has none (#353) Change-Id: If109ee2e3ac425d7c6462a06f67997a5f3fcd96e Signed-off-by: Thomas Kosiewski --- internal/controller/coderaiprovider_record.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/internal/controller/coderaiprovider_record.go b/internal/controller/coderaiprovider_record.go index 40b0a116..ddcfd47b 100644 --- a/internal/controller/coderaiprovider_record.go +++ b/internal/controller/coderaiprovider_record.go @@ -42,6 +42,13 @@ func (r *CoderAIProviderReconciler) writeAIProviderRecord( Kind: cleanupRecordKindAIProvider, UID: provider.UID, Namespace: provider.Namespace, Name: provider.Name, DeploymentID: deploymentID, Nonce: aiProviderNonce(provider), CoderID: provider.Status.ProviderID, Type: provider.Spec.Type, } + if record.CoderID == "" && createAttempt == nil && previous != nil { + // A status without the ID does not prove that the provider is gone, + // and after the sync only the recorded ID finds it. A new create + // drops the ID: it follows Coder's 404 for the name, and Coder + // cannot rename a provider. + record.CoderID = previous.CoderID + } if createAttempt != nil { record.CreateAttemptTime = *createAttempt } else if provider.Status.CreateAttemptTime != nil { From 16e47073082f1c5e49b21018e6881237fd010237 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 17:47:05 +0000 Subject: [PATCH 03/11] feat: cleanup records and sweep for CoderMCPServer (#353) Change-Id: Ia2e08b94b5c2e258d8eec9e0078f9442b641a6ff Signed-off-by: Thomas Kosiewski --- internal/app/controllerapp/controllerapp.go | 6 +- internal/controller/cleanup_record.go | 12 +- internal/controller/cleanup_sweep.go | 69 +++++- internal/controller/cleanup_sweep_mcp_test.go | 138 +++++++++++ .../controller/codermcpserver_apply_test.go | 2 +- .../controller/codermcpserver_controller.go | 27 ++- .../codermcpserver_controller_test.go | 9 +- internal/controller/codermcpserver_create.go | 30 ++- .../controller/codermcpserver_create_test.go | 4 +- internal/controller/codermcpserver_delete.go | 60 +++-- .../codermcpserver_delete_internal_test.go | 5 +- .../controller/codermcpserver_delete_test.go | 8 +- .../controller/codermcpserver_marker_test.go | 2 +- .../controller/codermcpserver_optin_test.go | 4 +- internal/controller/codermcpserver_record.go | 92 ++++++++ .../controller/codermcpserver_record_test.go | 222 ++++++++++++++++++ .../controller/codermcpserver_sync_test.go | 2 +- 17 files changed, 638 insertions(+), 54 deletions(-) create mode 100644 internal/controller/cleanup_sweep_mcp_test.go create mode 100644 internal/controller/codermcpserver_record.go create mode 100644 internal/controller/codermcpserver_record_test.go diff --git a/internal/app/controllerapp/controllerapp.go b/internal/app/controllerapp/controllerapp.go index 30222b2c..d3121b25 100644 --- a/internal/app/controllerapp/controllerapp.go +++ b/internal/app/controllerapp/controllerapp.go @@ -161,8 +161,10 @@ func SetupControllers(mgr manager.Manager) error { } mcpServerReconciler := &controller.CoderMCPServerReconciler{ - Client: client, - Scheme: managerScheme, + Client: client, + Scheme: managerScheme, + APIReader: mgr.GetAPIReader(), + OperatorNamespace: aiProviderReconciler.OperatorNamespace, } if err := mcpServerReconciler.SetupWithManager(mgr); err != nil { return fmt.Errorf("unable to create MCP server controller: %w", err) diff --git a/internal/controller/cleanup_record.go b/internal/controller/cleanup_record.go index 7465269a..d8508429 100644 --- a/internal/controller/cleanup_record.go +++ b/internal/controller/cleanup_record.go @@ -37,6 +37,8 @@ const ( cleanupRecordNonce = "nonce" cleanupRecordCoderID = "coderID" cleanupRecordType = "type" + cleanupRecordOrganization = "organizationID" + cleanupRecordSlug = "slug" cleanupRecordCreateSentAt = "createAttemptTime" ) @@ -51,13 +53,17 @@ type cleanupRecord struct { // database, so it names the database that holds the resource. DeploymentID string // Nonce finds a resource whose Coder ID is not known yet: the display - // name of a provider. + // name of a provider or of an MCP server config. Nonce string // CoderID is the Coder ID of the resource once the operator knows it. CoderID string // Type is the provider type that the create sent. With the nonce it // proves that a provider found by name is the object's. Type string + // Organization and Slug locate an MCP server config: its Coder + // organization ID and its slug in that organization. + Organization string + Slug string // CreateAttemptTime is when the operator last sent a create. A sweep // waits for the settle window after it. CreateAttemptTime time.Time @@ -111,6 +117,7 @@ func parseCleanupRecord(configMap *corev1.ConfigMap) (*cleanupRecord, error) { record := &cleanupRecord{ Kind: d[cleanupRecordKind], UID: types.UID(d[cleanupRecordUID]), Namespace: d[cleanupRecordNamespace], Name: d[cleanupRecordName], DeploymentID: d[cleanupRecordDeploymentID], Nonce: d[cleanupRecordNonce], CoderID: d[cleanupRecordCoderID], Type: d[cleanupRecordType], + Organization: d[cleanupRecordOrganization], Slug: d[cleanupRecordSlug], } if configMap.Labels[cleanupRecordLabel] == "" || configMap.Labels[cleanupRecordLabel] != record.Kind || record.UID == "" || configMap.Name != cleanupRecordObjectName(record.UID) || record.DeploymentID == "" { @@ -136,6 +143,9 @@ func writeCleanupRecord(ctx context.Context, c client.Client, namespace string, cleanupRecordKind: record.Kind, cleanupRecordUID: string(record.UID), cleanupRecordNamespace: record.Namespace, cleanupRecordName: record.Name, cleanupRecordDeploymentID: record.DeploymentID, cleanupRecordNonce: record.Nonce, cleanupRecordCoderID: record.CoderID, cleanupRecordType: record.Type, } + if record.Organization != "" || record.Slug != "" { + data[cleanupRecordOrganization], data[cleanupRecordSlug] = record.Organization, record.Slug + } if !record.CreateAttemptTime.IsZero() { data[cleanupRecordCreateSentAt] = record.CreateAttemptTime.UTC().Format(time.RFC3339) } diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go index accc677d..8d9b2e89 100644 --- a/internal/controller/cleanup_sweep.go +++ b/internal/controller/cleanup_sweep.go @@ -63,8 +63,9 @@ func init() { // CleanupRecordReconciler sweeps the cleanup records of objects that went // without a confirmed delete in Coder (#353). It acts only through a ready // control plane whose current Coder deployment ID equals the record's, and -// it deletes only the recorded provider ID, the provider with the recorded -// nonce, or the AI Gateway keys whose names start with the recorded prefix. +// it deletes only the recorded provider or MCP server config ID, the provider +// or config with the recorded nonce, or the AI Gateway keys whose names +// start with the recorded prefix. type CleanupRecordReconciler struct { client.Client // APIReader reads the object of a record live, so a stale cache never @@ -78,9 +79,9 @@ type CleanupRecordReconciler struct { } // SetupWithManager watches the records in the operator namespace. It wakes a -// record when its CoderAIProvider or CoderAIGatewayKey is deleted, and every -// record when a control plane gets operator access, since that control plane -// can match them. +// record when its CoderAIProvider, CoderAIGatewayKey or CoderMCPServer is +// deleted, and every record when a control plane gets operator access, since +// that control plane can match them. func (r *CleanupRecordReconciler) SetupWithManager(mgr ctrl.Manager) error { if mgr == nil || r.Client == nil || r.APIReader == nil || r.OperatorNamespace == "" || r.Clock == nil || r.Recorder == nil { return errors.New("assertion failed: the cleanup record reconciler needs a manager, a client, an API reader, the operator namespace, a clock and an event recorder") @@ -95,6 +96,7 @@ func (r *CleanupRecordReconciler) SetupWithManager(mgr ctrl.Manager) error { For(&corev1.ConfigMap{}, builder.WithPredicates(isRecord)). Watches(&coderv1alpha1.CoderAIProvider{}, recordOf, builder.WithPredicates(dependentRemoved)). Watches(&coderv1alpha1.CoderAIGatewayKey{}, recordOf, builder.WithPredicates(dependentRemoved)). + Watches(&coderv1alpha1.CoderMCPServer{}, recordOf, builder.WithPredicates(dependentRemoved)). Watches(&coderv1alpha1.CoderControlPlane{}, handler.EnqueueRequestsFromMapFunc(r.allRecords), builder.WithPredicates(operatorAccessGained)). Named("cleanuprecord"). Complete(r) @@ -229,6 +231,9 @@ var cleanupRecordKinds = map[string]cleanupSweepKind{ cleanupRecordKindAIGatewayKey: { object: func() client.Object { return &coderv1alpha1.CoderAIGatewayKey{} }, what: "the AI Gateway keys", sweep: sweepAIGatewayKeys, }, + cleanupRecordKindMCPServer: { + object: func() client.Object { return &coderv1alpha1.CoderMCPServer{} }, what: "the MCP server config", sweep: sweepMCPServer, + }, } // controlPlaneMatch is a control plane whose Coder deployment is a record's. @@ -367,6 +372,58 @@ func sweepAIGatewayKeys(ctx context.Context, sdk *codersdk.Client, record *clean return false, "Coder still lists keys of the object after their delete", nil } +// sweepMCPServer deletes the MCP server config of record in Coder. It +// returns gone when Coder confirms that it holds no config of the record: a +// delete answered with success or Coder's own 404, or a list by a site owner +// that shows no config of the record's slug with the record's nonce. It +// deletes only the recorded ID or the config whose display name is the +// recorded nonce (invariant 5). +func sweepMCPServer(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (gone bool, why string, err error) { + if record.Nonce != mcpServerNoncePrefix+string(record.UID) { + return false, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce) + } + orgID, err := uuid.Parse(record.Organization) + if err != nil || orgID == uuid.Nil || orgID.String() != record.Organization || record.Slug == "" { + return false, "", fmt.Errorf("assertion failed: cleanup record of %s has organization ID %q and slug %q", record.UID, record.Organization, record.Slug) + } + if step := mcpServerRequireSiteOwner(ctx, sdk); step != nil { + // Only a site owner sees every config in the list. + return false, step.message, nil + } + var id uuid.UUID + if record.CoderID != "" { + if id, err = uuid.Parse(record.CoderID); err != nil || id == uuid.Nil || id.String() != record.CoderID { + return false, "", fmt.Errorf("assertion failed: cleanup record of %s has coderID %q, which is not a canonical UUID", record.UID, record.CoderID) + } + } else { + configs, err := sdk.MCPServerConfigs(ctx, orgID) + switch { + case isCoderResourceNotFound(err): + return true, "the organization is gone", nil + case err != nil: + return false, "list MCP server configs: " + coderErrorSummary(err), nil + } + found, step := mcpServerWithSlug(configs, record.Slug, orgID) + switch { + case step != nil: + return false, step.message, nil + case found == nil: + return true, "no config with that slug", nil + case found.DisplayName != record.Nonce: + // Another config holds the slug, so no create of this object + // committed after the settle window. + return true, "the config with that slug is not this object's", nil + } + id = found.ID + } + if err := sdk.DeleteMCPServerConfig(ctx, orgID, id); err != nil && !isCoderResourceNotFound(err) { + return false, "delete MCP server config " + id.String() + ": " + coderErrorSummary(err), nil + } else if err != nil { + return true, "MCP server config " + id.String() + " was already gone", nil //nolint:nilerr // Coder's own 404 confirms the delete. + } + return true, "deleted MCP server config " + id.String(), nil +} + // setUnmatched keeps the unmatched gauge equal to the records that no // deployment reaches. func (r *CleanupRecordReconciler) setUnmatched(uid types.UID, kind string, unmatched bool) { @@ -375,7 +432,7 @@ func (r *CleanupRecordReconciler) setUnmatched(uid types.UID, kind string, unmat } else { r.unmatched.Delete(uid) } - counts := map[string]float64{cleanupRecordKindAIProvider: 0, cleanupRecordKindAIGatewayKey: 0} + counts := map[string]float64{cleanupRecordKindAIProvider: 0, cleanupRecordKindAIGatewayKey: 0, cleanupRecordKindMCPServer: 0} r.unmatched.Range(func(_, v any) bool { counts[v.(string)]++ return true diff --git a/internal/controller/cleanup_sweep_mcp_test.go b/internal/controller/cleanup_sweep_mcp_test.go new file mode 100644 index 00000000..19b1f29e --- /dev/null +++ b/internal/controller/cleanup_sweep_mcp_test.go @@ -0,0 +1,138 @@ +package controller_test + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/tools/events" + clocktesting "k8s.io/utils/clock/testing" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" + "github.com/coder/coder-k8s/internal/controller" +) + +// mcpSweep is a sweeper for the records of an mcpServerEnv. +type mcpSweep struct { + env mcpServerEnv + clock *clocktesting.FakeClock + recorder *events.FakeRecorder + sweeper *controller.CleanupRecordReconciler +} + +func newMCPSweep(t *testing.T, env mcpServerEnv) mcpSweep { + t.Helper() + s := mcpSweep{env: env, clock: clocktesting.NewFakeClock(time.Now()), recorder: events.NewFakeRecorder(10)} + s.sweeper = &controller.CleanupRecordReconciler{Client: watchClient(t), APIReader: k8sClient, OperatorNamespace: env.namespace, Clock: s.clock, Recorder: s.recorder} + return s +} + +func (s mcpSweep) sweep(ctx context.Context, t *testing.T, uid types.UID) ctrl.Result { + t.Helper() + result, err := s.sweeper.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKey{Namespace: s.env.namespace, Name: "ck8s-cleanup-" + string(uid)}}) + require.NoError(t, err) + return result +} + +// leaveMCPServer deletes the control plane, then the object, as a namespace +// deletion can: the finalizer goes without a delete, and the record stays. +func (env mcpServerEnv) leaveMCPServer(ctx context.Context, t *testing.T, server *coderv1alpha1.CoderMCPServer) { + t.Helper() + require.NoError(t, k8sClient.Delete(ctx, env.controlPlane)) + gone, _ := env.deleteMCPServer(ctx, t, server) + require.Nil(t, gone) + require.NotNil(t, env.mcpServerRecord(ctx, t, server.UID)) +} + +// #353 (#384 follow-up 3): after a namespace deletion, a new control plane +// that reaches the same Coder deployment deletes the leftover MCP server +// config, once the settle window has passed. Not parallel: it reads +// process-wide metrics. +func TestCleanupSweepMCPServer(t *testing.T) { + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + env.leaveMCPServer(ctx, t, got) + require.Len(t, env.fake.mcpServers(), 1, "the config is left in Coder") + s := newMCPSweep(t, env) + swept := kindMetric(t, "coder_k8s_cleanup_records_swept_total", "CoderMCPServer") + + requests := env.fake.totalRequests() + result := s.sweep(ctx, t, got.UID) + require.Equal(t, requests, env.fake.totalRequests(), "no Coder call inside the settle window") + require.Greater(t, result.RequeueAfter, 4*time.Minute) + s.clock.Step(6 * time.Minute) + require.Equal(t, 10*time.Minute, s.sweep(ctx, t, got.UID).RequeueAfter, "no control plane reaches the deployment") + require.Contains(t, <-s.recorder.Events, "NoMatchingDeployment") + require.Equal(t, 1.0, kindMetric(t, "coder_k8s_cleanup_records_unmatched", "CoderMCPServer")) + require.Len(t, env.fake.mcpServers(), 1) + + createTestControlPlane(ctx, t, env.namespace, "coder-b", env.fake.server.URL) + require.Zero(t, s.sweep(ctx, t, got.UID).RequeueAfter) + require.Empty(t, env.fake.mcpServers(), "the sweep deleted the config") + require.Equal(t, 1, env.fake.requestCount(routeDeleteMCPServer)) + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID), "Coder confirmed the delete, so the record goes") + require.Equal(t, swept+1, kindMetric(t, "coder_k8s_cleanup_records_swept_total", "CoderMCPServer")) + require.Zero(t, kindMetric(t, "coder_k8s_cleanup_records_unmatched", "CoderMCPServer")) +} + +// #353 invariants 5 and 6: a sweep never touches the config of a living +// object, finds a config without a recorded ID only by its slug and nonce, +// and leaves a config with the slug that the object did not create. +func TestCleanupSweepMCPServerCases(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("living object", func(t *testing.T) { + t.Parallel() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + s := newMCPSweep(t, env) + s.clock.Step(time.Hour) + requests := env.fake.totalRequests() + require.Equal(t, 30*time.Minute, s.sweep(ctx, t, got.UID).RequeueAfter) + require.Equal(t, requests, env.fake.totalRequests(), "no Coder call for a living object") + require.NotNil(t, env.mcpServerRecord(ctx, t, got.UID)) + }) + for name, tc := range map[string]struct { + replace bool // Someone else's config takes the slug. + }{ + "pending create, found by its nonce": {}, + "slug taken by another config": {replace: true}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.failNext(routeCreateMCPServer, fakeFault{AfterCommit: true, Reset: true}) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, false, "ConfirmingCreate") + env.leaveMCPServer(ctx, t, got) + require.Empty(t, env.mcpServerRecord(ctx, t, got.UID)["coderID"]) + if tc.replace { + for _, c := range env.fake.mcpServers() { + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, c.view.ID)) + } + createOther(ctx, t, env, got) + } + deletes := env.fake.requestCount(routeDeleteMCPServer) + createTestControlPlane(ctx, t, env.namespace, "coder-b", env.fake.server.URL) + s := newMCPSweep(t, env) + s.clock.Step(6 * time.Minute) + require.Zero(t, s.sweep(ctx, t, got.UID).RequeueAfter) + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID)) + if tc.replace { + require.Equal(t, deletes, env.fake.requestCount(routeDeleteMCPServer), "the other config is never deleted") + require.Len(t, env.fake.mcpServers(), 1) + return + } + require.Equal(t, deletes+1, env.fake.requestCount(routeDeleteMCPServer)) + require.Empty(t, env.fake.mcpServers(), "the config with the nonce is deleted") + }) + } +} diff --git a/internal/controller/codermcpserver_apply_test.go b/internal/controller/codermcpserver_apply_test.go index 498d46b2..a97c8068 100644 --- a/internal/controller/codermcpserver_apply_test.go +++ b/internal/controller/codermcpserver_apply_test.go @@ -309,7 +309,7 @@ func TestCoderMCPServerApplyLostStatusWrite(t *testing.T) { env := newMCPServerEnv(ctx, t, true) env.keySecret(ctx, t, testMCPKey) server := env.create(ctx, t, nil) - r := &controller.CoderMCPServerReconciler{Client: appliedStatusFailClient{k8sClient}, Scheme: scheme} + r := &controller.CoderMCPServerReconciler{Client: appliedStatusFailClient{k8sClient}, Scheme: scheme, APIReader: k8sClient, OperatorNamespace: env.namespace} _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(server)}) require.ErrorContains(t, err, "lost status write") require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(server), server)) diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index df1e7ff7..50a2fb15 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -58,6 +58,10 @@ const ( type CoderMCPServerReconciler struct { client.Client Scheme *runtime.Scheme + // APIReader reads the cleanup records live, and OperatorNamespace holds + // them (#353). + APIReader client.Reader + OperatorNamespace string // sentCredentials maps types.NamespacedName -> sentCredentialSums: the // object's UID, and credential field -> SHA-256 of the value that Coder @@ -91,8 +95,8 @@ func mcpServerStalled(requeue time.Duration, reason, format string, args ...any) // A change of the referenced control plane or of a credential Secret requeues // the object. func (r *CoderMCPServerReconciler) SetupWithManager(mgr ctrl.Manager) error { - if mgr == nil || r.Client == nil || r.Scheme == nil { - return fmt.Errorf("assertion failed: MCP server reconciler needs a manager, a client, and a scheme") + if mgr == nil || r.Client == nil || r.Scheme == nil || r.APIReader == nil || r.OperatorNamespace == "" { + return fmt.Errorf("assertion failed: MCP server reconciler needs a manager, a client, a scheme, an API reader and the operator namespace") } indexes := map[string]func(*coderv1alpha1.CoderMCPServer) []string{ mcpServerControlPlaneRefIndex: func(s *coderv1alpha1.CoderMCPServer) []string { return []string{s.Spec.ControlPlaneRef.Name} }, @@ -153,8 +157,8 @@ func (r *CoderMCPServerReconciler) serversWith(ctx context.Context, namespace, i // Reconcile runs the checks in order and reports the first one that fails. func (r *CoderMCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - if r.Client == nil || r.Scheme == nil { - return ctrl.Result{}, fmt.Errorf("assertion failed: MCP server reconciler needs a client and a scheme") + if r.Client == nil || r.Scheme == nil || r.APIReader == nil || r.OperatorNamespace == "" { + return ctrl.Result{}, fmt.Errorf("assertion failed: MCP server reconciler needs a client, a scheme, an API reader and the operator namespace") } server := &coderv1alpha1.CoderMCPServer{} if err := r.Get(ctx, req.NamespacedName, server); apierrors.IsNotFound(err) { @@ -254,6 +258,11 @@ func (r *CoderMCPServerReconciler) check( if step != nil || err != nil { return step, err } + // The record carries the config ID before the sync replaces the nonce, + // so a later sweep can still find the config (#353). + if step, err := r.ensureMCPServerRecord(ctx, sdk, server); step != nil || err != nil { + return step, err + } // A config that is gone before a full apply is created again once. for range 2 { if server.Status.ConfigID == "" { @@ -506,21 +515,21 @@ func findMCPServerSlug( if err != nil { return nil, mcpServerListFailed(err) } - return mcpServerWithSlug(configs, server, orgID) + return mcpServerWithSlug(configs, server.Spec.Slug, orgID) } func mcpServerListFailed(err error) *mcpServerStep { return mcpServerWait(mcpServerRetry, "CoderUnavailable", "list MCP server configs: %s", coderErrorSummary(err)) } -// mcpServerWithSlug returns the config with the spec's slug from configs, the -// list of the organization orgID. -func mcpServerWithSlug(configs []codersdk.MCPServerConfig, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID) (*codersdk.MCPServerConfig, *mcpServerStep) { +// mcpServerWithSlug returns the config with slug from configs, the list of +// the organization orgID. +func mcpServerWithSlug(configs []codersdk.MCPServerConfig, slug string, orgID uuid.UUID) (*codersdk.MCPServerConfig, *mcpServerStep) { for i, config := range configs { if err := coderAnswerFor("MCP server config organization", orgID.String(), config.OrganizationID.String()); err != nil { return nil, mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "%s", err.Error()) } - if config.Slug != server.Spec.Slug { + if config.Slug != slug { continue } if config.ID == uuid.Nil { diff --git a/internal/controller/codermcpserver_controller_test.go b/internal/controller/codermcpserver_controller_test.go index 016a160d..2f74b513 100644 --- a/internal/controller/codermcpserver_controller_test.go +++ b/internal/controller/codermcpserver_controller_test.go @@ -51,7 +51,7 @@ func newMCPServerEnv(ctx context.Context, t *testing.T, withControlPlane bool) m t.Helper() env := mcpServerEnv{namespace: createTestNamespace(ctx, t, "mcp"), fake: newFakeCoder(t)} env.orgID = env.fake.addOrganization("coder") - env.r = &controller.CoderMCPServerReconciler{Client: k8sClient, Scheme: scheme} + env.r = &controller.CoderMCPServerReconciler{Client: k8sClient, Scheme: scheme, APIReader: k8sClient, OperatorNamespace: env.namespace} if withControlPlane { env.controlPlane = createTestControlPlane(ctx, t, env.namespace, "coder", env.fake.server.URL) env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}}) @@ -117,9 +117,10 @@ func (env mcpServerEnv) reconcile(ctx context.Context, t *testing.T, server *cod return got, result } -// mcpRequests counts the fake's requests other than organization lookups. +// mcpRequests counts the fake's requests other than organization lookups +// and deployment ID reads. func (env mcpServerEnv) mcpRequests() int { - return env.fake.totalRequests() - env.fake.requestCount(routeOrganization) + return env.fake.totalRequests() - env.fake.requestCount(routeOrganization) - env.fake.requestCount(routeBuildInfo) } // requireMCPServerStep checks the kstatus conditions of a wait, a stall, or @@ -582,7 +583,7 @@ func TestCoderMCPServerWatches(t *testing.T) { Controller: config.Controller{SkipNameValidation: ptrTo(true)}, }) require.NoError(t, err) - require.NoError(t, (&controller.CoderMCPServerReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme()}).SetupWithManager(mgr)) + require.NoError(t, (&controller.CoderMCPServerReconciler{Client: mgr.GetClient(), Scheme: mgr.GetScheme(), APIReader: mgr.GetAPIReader(), OperatorNamespace: env.namespace}).SetupWithManager(mgr)) mgrCtx, cancel := context.WithCancel(ctx) mgrDone := make(chan error, 1) go func() { mgrDone <- mgr.Start(mgrCtx) }() diff --git a/internal/controller/codermcpserver_create.go b/internal/controller/codermcpserver_create.go index 04f1d49f..519bc162 100644 --- a/internal/controller/codermcpserver_create.go +++ b/internal/controller/codermcpserver_create.go @@ -55,7 +55,11 @@ func (r *CoderMCPServerReconciler) createOrRecord( return keepRejectedMCPServerCreate(step, rejected), nil } if found != nil { - return recordMCPServer(server, *found), nil + if step := recordMCPServer(server, *found); step != nil { + return step, nil + } + // Store the ID before the sync replaces the nonce (#353). + return r.ensureMCPServerRecord(ctx, sdk, server) } if server.Status.CreateAttemptTime != nil { // A create of this object may have been sent. A missing config proves @@ -68,7 +72,10 @@ func (r *CoderMCPServerReconciler) createOrRecord( return mcpServerStalled(0, "CoderRejected", "Coder rejected the create request of MCP server config %s, and a read confirmed that nothing was created.", server.Spec.Slug), nil } } - return r.createMCPServer(ctx, sdk, server, orgID, values, before) + if step, err := r.createMCPServer(ctx, sdk, server, orgID, values, before); step != nil || err != nil { + return step, err + } + return r.ensureMCPServerRecord(ctx, sdk, server) } // keepRejectedMCPServerCreate keeps the memory of a rejected create in the @@ -110,16 +117,23 @@ func recordMCPServer(server *coderv1alpha1.CoderMCPServer, found codersdk.MCPSer return nil } -// createMCPServer writes the create marker and the change tokens, then sends -// the only create request of this attempt. A conflict on the marker write -// ends the reconcile before any request. The config starts disabled with the -// nonce as its display name, so members never see it and chats never use it -// (plan F6) before the apply step. +// createMCPServer writes the cleanup record, then the create marker and the +// change tokens, then sends the only create request of this attempt. A failed +// record write or a conflict on the marker write ends the reconcile before +// any request. The config starts disabled with the nonce as its display name, +// so members never see it and chats never use it (plan F6) before the apply +// step. func (r *CoderMCPServerReconciler) createMCPServer( ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, values map[string]credentialsecret.Value, before *coderv1alpha1.CoderMCPServerStatus, ) (*mcpServerStep, error) { - server.Status.CreateAttemptTime = &metav1.Time{Time: time.Now()} + now := time.Now() + // Without the record, a config that a namespace deletion leaves behind + // cannot be found again (#353 invariant 1). + if step, err := r.writeMCPServerRecord(ctx, sdk, server, &now); step != nil || err != nil { + return step, err + } + server.Status.CreateAttemptTime = &metav1.Time{Time: now} server.Status.Credentials = mcpServerCredentialStatus(server.Spec.Auth, values) applyMCPServerStep(server, mcpServerWait(0, "CreatingConfig", "Creating MCP server config %s in Coder.", server.Spec.Slug)) if err := r.writeStatus(ctx, server, before); err != nil { diff --git a/internal/controller/codermcpserver_create_test.go b/internal/controller/codermcpserver_create_test.go index 82d21d96..f0b2d9db 100644 --- a/internal/controller/codermcpserver_create_test.go +++ b/internal/controller/codermcpserver_create_test.go @@ -292,7 +292,7 @@ func TestCoderMCPServerCreateRecovery(t *testing.T) { u.Roles = []codersdk.SlimRole{{Name: codersdk.RoleOwner, OrganizationID: env.orgID.String()}, {Name: codersdk.RoleMember}} }) } - env.r = &controller.CoderMCPServerReconciler{Client: k8sClient, Scheme: scheme} + env.r = &controller.CoderMCPServerReconciler{Client: k8sClient, Scheme: scheme, APIReader: k8sClient, OperatorNamespace: env.namespace} base := env.posts() // Creates of the test setup. got, _ := env.reconcile(ctx, t, server) if tc.reason == "Applied" { @@ -331,7 +331,7 @@ func TestCoderMCPServerMarkerConflictSendsNoCreate(t *testing.T) { env := newMCPServerEnv(ctx, t, true) env.keySecret(ctx, t, testMCPKey) server := env.create(ctx, t, nil) - r := &controller.CoderMCPServerReconciler{Client: markerConflictMCPClient{k8sClient}, Scheme: scheme} + r := &controller.CoderMCPServerReconciler{Client: markerConflictMCPClient{k8sClient}, Scheme: scheme, APIReader: k8sClient, OperatorNamespace: env.namespace} _, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(server)}) require.True(t, apierrors.IsConflict(err), "%v", err) require.Zero(t, env.posts()) diff --git a/internal/controller/codermcpserver_delete.go b/internal/controller/codermcpserver_delete.go index df3cd298..d99dee3d 100644 --- a/internal/controller/codermcpserver_delete.go +++ b/internal/controller/codermcpserver_delete.go @@ -1,7 +1,9 @@ package controller import ( + "cmp" "context" + "errors" "fmt" "time" @@ -32,9 +34,16 @@ const ( // returns release when the finalizer can go. Otherwise the step says why the // finalizer stays. The scope gate never blocks it. func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coderv1alpha1.CoderMCPServer) (release bool, step *mcpServerStep, err error) { - if server.Status.ConfigID == "" && server.Status.CreateAttemptTime == nil { - // Nothing of this object can exist in Coder. - return true, nil, nil + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, server.UID) + if errors.Is(err, errNotACleanupRecord) { + record, err = nil, nil // Not the operator's ConfigMap: it proves nothing either way. + } else if err != nil { + return false, nil, err + } + if server.Status.ConfigID == "" && server.Status.CreateAttemptTime == nil && (record == nil || record.CoderID == "") { + // Nothing of this object can exist in Coder. A record with a config + // ID still locates a config, for example after a lost status write. + return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, server.UID) } orgID, err := uuid.Parse(server.Status.OrganizationID) if err != nil || orgID == uuid.Nil { @@ -94,7 +103,28 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder ctx, cancel = context.WithTimeout(ctx, mcpServerBoundedDeleteTimeout) defer cancel() } - step, err = deleteMCPServerInCoder(ctx, sdk, server, orgID) + recordedID := "" + if record != nil { + // The record names the deployment that holds the config. When the + // control plane now reaches another deployment, a delete there + // proves nothing, so the record stays for a sweep (#353 invariant 7). + id, err := coderDeploymentID(ctx, sdk) + switch { + case err != nil && deleting: + return r.releaseWithoutDelete(ctx, server, "the deployment ID read while CoderControlPlane is being deleted failed: "+coderErrorSummary(err)), nil, nil + case err != nil: + return false, mcpServerWait(mcpServerDeletePoll, "CoderUnavailable", "read the Coder deployment ID: %s", coderErrorSummary(err)), nil + case id != record.DeploymentID: + return r.releaseWithoutDelete(ctx, server, fmt.Sprintf("CoderControlPlane now reaches Coder deployment %s, but the config is in deployment %s", id, record.DeploymentID)), nil, nil + } + recordedID = record.CoderID + } + step, err = deleteMCPServerInCoder(ctx, sdk, server, orgID, recordedID) + if err == nil && step == nil { + // Coder confirmed that it holds no config of this object. Only then + // does the record go (#353 invariant 7). + return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, server.UID) + } switch { case err != nil && deleting: // The attempt ended with an impossible answer. Log it, and still @@ -106,19 +136,21 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder case step != nil && deleting: return r.releaseWithoutDelete(ctx, server, "the one delete attempt while CoderControlPlane is being deleted failed: "+step.message), nil, nil } - return step == nil, step, nil + return false, step, nil } // deleteMCPServerInCoder deletes the config of this object in Coder and -// returns nil when Coder holds none. With only a create marker, a read -// decides first: only the nonce proves that the config is this object's. -// Only Coder's own 404 proves absence (#337). -func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID) (*mcpServerStep, error) { +// returns nil when Coder holds none. Without status.configID it deletes +// recordedID, the config ID of the cleanup record: a failed status write +// after the create can leave the ID only there. With only a create marker, a +// read decides first: only the nonce proves that the config is this +// object's. Only Coder's own 404 proves absence (#337). +func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, recordedID string) (*mcpServerStep, error) { var id uuid.UUID - if server.Status.ConfigID != "" { - parsed, err := uuid.Parse(server.Status.ConfigID) - if err != nil || parsed == uuid.Nil || parsed.String() != server.Status.ConfigID { - return nil, fmt.Errorf("assertion failed: MCP server %s/%s has status.configID %q, which is not a canonical UUID", server.Namespace, server.Name, server.Status.ConfigID) + if configID := cmp.Or(server.Status.ConfigID, recordedID); configID != "" { + parsed, err := uuid.Parse(configID) + if err != nil || parsed == uuid.Nil || parsed.String() != configID { + return nil, fmt.Errorf("assertion failed: MCP server %s/%s has config ID %q, which is not a canonical UUID", server.Namespace, server.Name, configID) } id = parsed } else { @@ -130,7 +162,7 @@ func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *c } // Coder's own 404 means that the pinned organization is gone, and no // config of it can be reached any more (#378). - found, step := mcpServerWithSlug(configs, server, orgID) + found, step := mcpServerWithSlug(configs, server.Spec.Slug, orgID) switch { case step != nil: step.requeue = max(step.requeue, mcpServerDeletePoll) diff --git a/internal/controller/codermcpserver_delete_internal_test.go b/internal/controller/codermcpserver_delete_internal_test.go index 1de7efc4..8bb24602 100644 --- a/internal/controller/codermcpserver_delete_internal_test.go +++ b/internal/controller/codermcpserver_delete_internal_test.go @@ -47,12 +47,13 @@ func TestMCPServerDeleteUnderHoldEndsAtHoldEnd(t *testing.T) { Data: map[string][]byte{coderv1alpha1.DefaultTokenSecretKey: []byte("token")}, } namespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns"}} - r := &CoderMCPServerReconciler{Client: fake.NewClientBuilder().WithScheme(scheme).WithObjects(controlPlane, token, namespace).Build(), Scheme: scheme} + c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(controlPlane, token, namespace).Build() + r := &CoderMCPServerReconciler{Client: c, APIReader: c, Scheme: scheme, OperatorNamespace: "coder-system"} held, err := controlPlaneHolds(context.Background(), r.Client, controlPlane, time.Now()) require.NoError(t, err) require.True(t, held, "assertion failed: the test needs a control plane that holds") - server := &coderv1alpha1.CoderMCPServer{ObjectMeta: metav1.ObjectMeta{Name: "github", Namespace: "ns"}} + server := &coderv1alpha1.CoderMCPServer{ObjectMeta: metav1.ObjectMeta{Name: "github", Namespace: "ns", UID: "mcp-uid"}} server.Spec.ControlPlaneRef.Name = "coder" server.Spec.Slug = "github" server.Status.OrganizationID = uuid.NewString() diff --git a/internal/controller/codermcpserver_delete_test.go b/internal/controller/codermcpserver_delete_test.go index 2366b060..61b76ea5 100644 --- a/internal/controller/codermcpserver_delete_test.go +++ b/internal/controller/codermcpserver_delete_test.go @@ -9,6 +9,7 @@ import ( "github.com/coder/coder/v2/codersdk" "github.com/google/uuid" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" @@ -198,6 +199,11 @@ func (env mcpServerEnv) pendingCreate( server.Status.ConfigID = "" server.Status.CreateAttemptTime = &metav1.Time{Time: time.Now().Add(-age)} require.NoError(t, k8sClient.Status().Update(ctx, server)) + // The record of a pending create has no config ID yet. + record := &corev1.ConfigMap{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(server.UID)}, record)) + delete(record.Data, "coderID") + require.NoError(t, k8sClient.Update(ctx, record)) return server } @@ -320,7 +326,7 @@ func TestCoderMCPServerReleasedWithoutDeleteMetric(t *testing.T) { base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme}) require.NoError(t, err) failed := false - env.r = &controller.CoderMCPServerReconciler{Scheme: scheme, Client: interceptor.NewClient(base, interceptor.Funcs{ + env.r = &controller.CoderMCPServerReconciler{Scheme: scheme, APIReader: k8sClient, OperatorNamespace: env.namespace, Client: interceptor.NewClient(base, interceptor.Funcs{ Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { if _, ok := obj.(*coderv1alpha1.CoderMCPServer); ok && !failed { failed = true diff --git a/internal/controller/codermcpserver_marker_test.go b/internal/controller/codermcpserver_marker_test.go index 1c35af71..48f2816d 100644 --- a/internal/controller/codermcpserver_marker_test.go +++ b/internal/controller/codermcpserver_marker_test.go @@ -93,7 +93,7 @@ func TestCoderMCPServerDisableNeedsTheMarker(t *testing.T) { SubResourceUpdate: func(context.Context, client.Client, string, client.Object, ...client.SubResourceUpdateOption) error { return errors.New("lost status write") }, - }), Scheme: scheme} + }), Scheme: scheme, APIReader: k8sClient, OperatorNamespace: env.namespace} mark := len(env.writeRoutes()) _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) require.ErrorContains(t, err, "lost status write") diff --git a/internal/controller/codermcpserver_optin_test.go b/internal/controller/codermcpserver_optin_test.go index 61398716..26b8f0cc 100644 --- a/internal/controller/codermcpserver_optin_test.go +++ b/internal/controller/codermcpserver_optin_test.go @@ -255,7 +255,7 @@ func TestCoderMCPServerOptInDisableMarkerFirst(t *testing.T) { SubResourceUpdate: func(context.Context, client.Client, string, client.Object, ...client.SubResourceUpdateOption) error { return errors.New("lost status write") }, - }), Scheme: scheme} + }), Scheme: scheme, APIReader: k8sClient, OperatorNamespace: env.namespace} mark := len(env.writeRoutes()) _, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) require.ErrorContains(t, err, "lost status write") @@ -397,7 +397,7 @@ func TestCoderMCPServerOptInDisableAfterStopBeforePatch(t *testing.T) { stop() return err }, - }), Scheme: scheme} + }), Scheme: scheme, APIReader: k8sClient, OperatorNamespace: env.namespace} mark := len(env.writeRoutes()) _, _ = r.Reconcile(stopCtx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)}) require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) diff --git a/internal/controller/codermcpserver_record.go b/internal/controller/codermcpserver_record.go new file mode 100644 index 00000000..e6b5f59c --- /dev/null +++ b/internal/controller/codermcpserver_record.go @@ -0,0 +1,92 @@ +package controller + +import ( + "context" + "fmt" + "time" + + "github.com/coder/coder/v2/codersdk" + "github.com/google/uuid" + + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" +) + +// cleanupRecordKindMCPServer is the record kind of a CoderMCPServer. Its +// record holds the nonce, the pinned organization ID and the slug, which +// find a config whose ID is not known yet, and the config ID once it is. +const cleanupRecordKindMCPServer = "CoderMCPServer" + +// writeMCPServerRecord writes the cleanup record of server with the +// deployment ID that Coder answers now. createAttempt is the time of a create +// that the caller sends next, or nil. A failed read or write returns a wait +// or an error, and the caller then sends no create (#353 invariant 1). +func (r *CoderMCPServerReconciler) writeMCPServerRecord( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, createAttempt *time.Time, +) (*mcpServerStep, error) { + if orgID, err := uuid.Parse(server.Status.OrganizationID); err != nil || orgID == uuid.Nil || orgID.String() != server.Status.OrganizationID { + return nil, fmt.Errorf("assertion failed: MCP server %s/%s needs a pinned organization ID for its cleanup record, but has %q", server.Namespace, server.Name, server.Status.OrganizationID) + } + if id := server.Status.ConfigID; id != "" { + // A sweep deletes the recorded ID, so only a canonical UUID goes in. + if parsed, err := uuid.Parse(id); err != nil || parsed == uuid.Nil || parsed.String() != id { + return nil, fmt.Errorf("assertion failed: MCP server %s/%s has status.configID %q, which is not a canonical UUID", server.Namespace, server.Name, id) + } + } + previous, existing, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, server.UID) + if err != nil { + return nil, err + } + deploymentID, err := coderDeploymentID(ctx, sdk) + if err != nil { + return mcpServerWait(mcpServerRetry, "CoderUnavailable", "read the Coder deployment ID for the cleanup record: %s", coderErrorSummary(err)), nil + } + if previous != nil && previous.DeploymentID != deploymentID { + // The record is the only locator of what this object made in the old + // Coder database, so the controller neither overwrites it nor creates + // in the new one. + return mcpServerStalled(mcpServerResync, "CoderDeploymentChanged", + "MCP server config %s (ID %q) is in Coder deployment %s, but CoderControlPlane %s now reaches deployment %s. Delete the config there, then delete ConfigMap %s/%s. Until then the controller creates nothing.", + server.Spec.Slug, previous.CoderID, previous.DeploymentID, server.Spec.ControlPlaneRef.Name, deploymentID, r.OperatorNamespace, cleanupRecordObjectName(server.UID)), nil + } + record := cleanupRecord{ + Kind: cleanupRecordKindMCPServer, UID: server.UID, Namespace: server.Namespace, Name: server.Name, + DeploymentID: deploymentID, Nonce: mcpServerNonce(server), CoderID: server.Status.ConfigID, + Organization: server.Status.OrganizationID, Slug: server.Spec.Slug, + } + if record.CoderID == "" && createAttempt == nil && previous != nil { + // A status without the ID does not prove that the config is gone, and + // after the sync only the recorded ID finds it. A new create drops the + // ID: the controller clears it only after Coder's 404 for that ID. + record.CoderID = previous.CoderID + } + switch { + case createAttempt != nil: + record.CreateAttemptTime = *createAttempt + case server.Status.CreateAttemptTime != nil: + record.CreateAttemptTime = server.Status.CreateAttemptTime.Time + case previous != nil: + // The marker went after a confirmed create. The sweep still waits + // for the settle window after that create. + record.CreateAttemptTime = previous.CreateAttemptTime + } + return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) +} + +// ensureMCPServerRecord makes sure that a server which can exist in Coder has +// a record with its current config ID before the sync replaces the nonce. +// Objects from before records existed get one here too. With only a create +// marker, the write checks the deployment ID before a slug conflict in another +// deployment can clear the marker (#353). +func (r *CoderMCPServerReconciler) ensureMCPServerRecord(ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer) (*mcpServerStep, error) { + if server.Status.ConfigID == "" && server.Status.CreateAttemptTime == nil { + return nil, nil // Nothing of this object can exist in Coder yet. The create writes the record. + } + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, server.UID) + if err != nil { + return nil, err + } + if record != nil && record.CoderID != "" && record.CoderID == server.Status.ConfigID { + return nil, nil + } + return r.writeMCPServerRecord(ctx, sdk, server, nil) +} diff --git a/internal/controller/codermcpserver_record_test.go b/internal/controller/codermcpserver_record_test.go new file mode 100644 index 00000000..a5cb127d --- /dev/null +++ b/internal/controller/codermcpserver_record_test.go @@ -0,0 +1,222 @@ +package controller_test + +import ( + "context" + "errors" + "net/http" + "testing" + "time" + + "github.com/google/uuid" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + + "github.com/coder/coder-k8s/internal/controller" +) + +// mcpServerRecord returns the data of the cleanup record of uid, or nil when +// there is none. The env namespace is also the operator namespace. +func (env mcpServerEnv) mcpServerRecord(ctx context.Context, t *testing.T, uid types.UID) map[string]string { + t.Helper() + configMap := &corev1.ConfigMap{} + err := k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(uid)}, configMap) + if apierrors.IsNotFound(err) { + return nil + } + require.NoError(t, err) + require.Equal(t, "CoderMCPServer", configMap.Labels["coder.com/cleanup-record"]) + return configMap.Data +} + +// #353 (#384 follow-up 3): before its create, the controller records which +// Coder deployment holds the config, and the config ID before the sync +// replaces the nonce. The record holds no credential. A confirmed delete in +// Coder removes it. +func TestCoderMCPServerCleanupRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + + // Stop after the create, before the sync replaces the nonce. + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, false, "CoderUnavailable") + record := env.mcpServerRecord(ctx, t, got.UID) + require.NotEmpty(t, record["createAttemptTime"]) + delete(record, "createAttemptTime") + require.Equal(t, map[string]string{ + "kind": "CoderMCPServer", "uid": string(got.UID), "namespace": env.namespace, "name": got.Name, + "deploymentID": env.fake.deploymentID, "nonce": "coder-k8s:" + string(got.UID), "coderID": env.requireCreated(t, got).view.ID.String(), + "type": "", "organizationID": env.orgID.String(), "slug": got.Spec.Slug, + }, record, "the config ID is recorded before the sync replaces the nonce") + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + for _, v := range env.mcpServerRecord(ctx, t, got.UID) { + require.NotContains(t, v, testMCPKey, "a record holds no credential") + } + + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone) + require.Empty(t, env.fake.mcpServers()) + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID), "Coder confirmed the delete, so the record goes") +} + +// #353 invariant 1: without a record, the controller sends no create. +func TestCoderMCPServerNoCreateWithoutRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("record write fails", func(t *testing.T) { + t.Parallel() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + server := env.create(ctx, t, nil) + env.r.Client = interceptor.NewClient(watchClient(t), interceptor.Funcs{ + Create: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.CreateOption) error { + if _, ok := obj.(*corev1.ConfigMap); ok { + return errors.New("injected record write failure") + } + return c.Create(ctx, obj, opts...) + }, + }) + _, err := env.r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(server)}) + require.ErrorContains(t, err, "injected record write failure") + require.Zero(t, env.posts(), "no create without a record") + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(server), server)) + require.Nil(t, server.Status.CreateAttemptTime, "no create marker either") + }) + t.Run("deployment ID unknown", func(t *testing.T) { + t.Parallel() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.failNext(routeBuildInfo, fakeFault{Status: http.StatusServiceUnavailable}) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, false, "CoderUnavailable") + require.Zero(t, env.posts()) + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID)) + }) +} + +// #353: a release without a delete in Coder keeps the record, so a later +// sweep can delete the config. When the control plane reaches another Coder +// deployment, a delete there proves nothing, so the record stays too. +func TestCoderMCPServerRecordKeptWithoutDelete(t *testing.T) { + t.Parallel() + ctx := context.Background() + for name, leave := range map[string]func(env mcpServerEnv){ + "control plane gone": func(env mcpServerEnv) { require.NoError(t, k8sClient.Delete(ctx, env.controlPlane)) }, + "deployment switch": func(env mcpServerEnv) { + env.fake.mu.Lock() + env.fake.deploymentID = uuid.NewString() // A new database behind the same control plane. + env.fake.mu.Unlock() + }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + leave(env) + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone, "the finalizer goes without a delete") + require.Zero(t, env.fake.requestCount(routeDeleteMCPServer)) + require.Len(t, env.fake.mcpServers(), 1, "the config stays in Coder") + require.Equal(t, id.String(), env.mcpServerRecord(ctx, t, got.UID)["coderID"], "the record stays for a sweep") + }) + } +} + +// #353 (#362 follow-up 7, for MCP servers): a create whose answer was lost +// leaves a record without a config ID. When the control plane then reaches +// another Coder deployment, where someone else's config has the same slug, +// that slug conflict proves nothing about the old deployment. The controller +// keeps the marker and the record, so a delete keeps the record for a sweep. +func TestCoderMCPServerPendingCreateAfterDeploymentSwitch(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.failNext(routeCreateMCPServer, fakeFault{AfterCommit: true, Reset: true}) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, false, "ConfirmingCreate") + before := env.mcpServerRecord(ctx, t, got.UID) + require.NotNil(t, before) + require.Empty(t, before["coderID"], "the create answer was lost") + + env.fake.mu.Lock() + env.fake.deploymentID = uuid.NewString() // A new database behind the same control plane. + for _, s := range env.fake.mcp.servers { + s.view.DisplayName = "Someone else's server" + } + env.fake.mu.Unlock() + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, "CoderDeploymentChanged"), before["deploymentID"]) + require.NotNil(t, got.Status.CreateAttemptTime, "the marker stays") + require.Equal(t, before, env.mcpServerRecord(ctx, t, got.UID)) + + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone, "the finalizer goes without a delete") + require.Zero(t, env.fake.requestCount(routeDeleteMCPServer), "no delete in the new deployment") + require.Equal(t, before, env.mcpServerRecord(ctx, t, got.UID), "the record stays for a sweep") +} + +// #353: a status without the config ID, but with a create marker, keeps the +// recorded ID. After the sync the display name is no longer the nonce, so +// only the recorded ID still finds the config for a delete. +func TestCoderMCPServerKeepsRecordedIDWithoutStatusID(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + got.Status.ConfigID = "" + got.Status.CreateAttemptTime = &metav1.Time{Time: time.Now().Add(-time.Hour)} + require.NoError(t, k8sClient.Status().Update(ctx, got)) + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "SlugConflict") + require.Equal(t, id.String(), env.mcpServerRecord(ctx, t, got.UID)["coderID"], "the record keeps the config ID") + + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone) + require.Empty(t, env.fake.mcpServers(), "the delete used the recorded ID") + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID)) +} + +// #353: a config from before records existed gets one, with the deployment +// ID that Coder answers now. +func TestCoderMCPServerRecordBackfill(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + require.NoError(t, k8sClient.Delete(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(got.UID)}})) + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + record := env.mcpServerRecord(ctx, t, got.UID) + require.Equal(t, id.String(), record["coderID"]) + require.Equal(t, env.fake.deploymentID, record["deploymentID"]) +} + +// The reconciler refuses to start without its cleanup record dependencies. +func TestCoderMCPServerSetupNeedsRecordDependencies(t *testing.T) { + t.Parallel() + for name, r := range map[string]*controller.CoderMCPServerReconciler{ + "no operator namespace": {Client: k8sClient, Scheme: scheme, APIReader: k8sClient}, + "no API reader": {Client: k8sClient, Scheme: scheme, OperatorNamespace: "coder-system"}, + } { + mgr, err := ctrl.NewManager(cfg, ctrl.Options{Scheme: scheme, Metrics: metricsserver.Options{BindAddress: "0"}, HealthProbeBindAddress: "0"}) + require.NoError(t, err) + require.ErrorContains(t, r.SetupWithManager(mgr), "assertion failed: MCP server reconciler needs", name) + } +} diff --git a/internal/controller/codermcpserver_sync_test.go b/internal/controller/codermcpserver_sync_test.go index e260da3d..1003e2d7 100644 --- a/internal/controller/codermcpserver_sync_test.go +++ b/internal/controller/codermcpserver_sync_test.go @@ -330,7 +330,7 @@ func TestCoderMCPServerSyncSecretMetadataChange(t *testing.T) { require.Empty(t, env.writesSince(mark)) require.Equal(t, secret.ResourceVersion, got.Status.Credentials[0].SecretResourceVersion) - env.r = &controller.CoderMCPServerReconciler{Client: k8sClient, Scheme: scheme} + env.r = &controller.CoderMCPServerReconciler{Client: k8sClient, Scheme: scheme, APIReader: k8sClient, OperatorNamespace: env.namespace} touch("two") mark = len(env.writeRoutes()) got, _ = env.reconcile(ctx, t, got) From 687d0cab0d93636b5ed96d65a13ac656d9148eec Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 17:48:43 +0000 Subject: [PATCH 04/11] docs: cleanup records for MCP servers (#353) Change-Id: I312bf9f47741f74373c895cc0640d237a621a60f Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-mcp-servers.md | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/docs/how-to/manage-mcp-servers.md b/docs/how-to/manage-mcp-servers.md index 5487e3bb..6a3744ec 100644 --- a/docs/how-to/manage-mcp-servers.md +++ b/docs/how-to/manage-mcp-servers.md @@ -117,6 +117,7 @@ The object has the conditions `Ready`, `Reconciling` and `Stalled`. A stalled ob | `MCPServersNotAllowed`, `ForceOnNotAllowed` | The control plane does not allow this object. See [step 1](#1-allow-organizations-on-the-control-plane). | | `CredentialSecretNotFound`, `CredentialNotOptedIn`, `CredentialDestinationNotAllowed`, `CredentialRefused`, `CredentialKeyMissing`, `CredentialMalformed` | A credential Secret fails the contract. See [Credential kill switch](#credential-kill-switch). | | `SlugConflict` | The organization has a config with this slug that the operator did not create. | +| `CoderDeploymentChanged` | The control plane now reaches another Coder deployment than the one that holds the config. See [Cleanup records](#cleanup-records). | | `CreatingConfig`, `ConfirmingCreate`, `CreateRetrying` | A create is in progress, or the operator reads Coder to learn whether a create committed. | | `GroupNotFound` | A name in `access.groups` is not a Coder group yet. See [Access](#access). | | `CoderRejected` | Coder rejected the request. Fix the spec. | @@ -206,12 +207,20 @@ Deleting the object deletes the config in Coder. Coder removes the server from e - The operator deletes only a config that this object created. With only an unconfirmed create, it reads Coder first. It deletes a config that carries this object's temporary display name, and it never touches another config with the slug. A create can still commit for a short time, so within 2 minutes of the create request the operator reads again before it trusts that the config is absent. - When Coder is unreachable, or the control plane has no operator access, the finalizer stays and the operator retries every 60 seconds. The operator trusts only Coder's own 404 as proof that the config is gone. A 404 from a proxy keeps the finalizer. -- When the control plane is gone, nothing can delete the config. The finalizer goes at once. The operator logs the slug and the config ID with the reason `ControlPlaneGone`. Delete the config in the Coder dashboard. +- When the control plane is gone, the operator cannot delete the config now. The finalizer goes at once. The operator logs the slug and the config ID with the reason `ControlPlaneGone`. The [cleanup record](#cleanup-records) stays, so a later sweep can delete the config. - When you delete the control plane while `CoderMCPServer` objects that use it are being deleted, the control plane waits for them for up to 10 minutes, plus a grace period for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. -- When the control plane does not wait (its namespace is being deleted, it is deleted in the foreground, or the 10 minutes have passed) and its operator access still works, the operator sends one delete with a 10-second limit, then removes the finalizer, also when that delete fails. Without working access, the finalizer goes without a delete. So a namespace deletion finishes, but it can leave the config in Coder. The metric `coder_k8s_cleanup_released_without_delete_total` (label `kind`) counts each release without a delete. +- When the control plane does not wait (its namespace is being deleted, it is deleted in the foreground, or the 10 minutes have passed) and its operator access still works, the operator sends one delete with a 10-second limit, then removes the finalizer, also when that delete fails. Without working access, the finalizer goes without a delete. So a namespace deletion finishes, but it can leave the config in Coder, and its cleanup record stays. The metric `coder_k8s_cleanup_released_without_delete_total` (label `kind`) counts each release without a delete. - A withdrawn opt-in never blocks a delete. -Delete `CoderMCPServer` objects before their control plane and before their namespace. +Delete `CoderMCPServer` objects before their control plane and before their namespace. A sweep deletes a leftover config only when the namespace comes back with a control plane of the same Coder deployment. + +### Cleanup records + +Before the operator creates a config in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderMCPServer` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the config. The record holds no credential: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the config's temporary display name, the pinned organization ID, the slug, and the config ID once the operator knows it. + +When the control plane later reaches another Coder deployment (for example after its database Secret changed), the record still locates the config in the old one. The operator then stalls the object with `CoderDeploymentChanged`, keeps the record and creates nothing in the new deployment. Delete the config in the old deployment, then delete the ConfigMap. + +The operator removes the record when Coder confirms that the config is deleted. When the finalizer goes without a delete, the record stays, and the operator sweeps it as for [AI providers](manage-ai-providers.md#cleanup-records): once the object is gone and 5 minutes have passed since the last create, through a ready control plane in the object's namespace whose Coder answers the record's deployment ID. The sweep deletes the recorded config ID. Without a recorded ID, it deletes only the config with the recorded slug whose display name is still the temporary one. A record that no control plane reaches stays, with the gauge `coder_k8s_cleanup_records_unmatched` and a `NoMatchingDeployment` Warning Event. When that Coder deployment never comes back, delete the config in the Coder dashboard, then delete the ConfigMap. To release a stuck object by hand, remove the finalizer with a JSON patch whose `test` operation checks the index first, as for [AI providers](manage-ai-providers.md#stuck-finalizer). Then check **AI settings > MCP servers** in the dashboard for a leftover config with the slug. From 9d8274a1f18fcb871f3ed851f9323dbf127e85ba Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 18:04:18 +0000 Subject: [PATCH 05/11] fix: backfill MCP server records on stalled objects and cover review cases (#353) Change-Id: I7090bb024a2cc861f5b30e1bee9e6cfe448e9030 Signed-off-by: Thomas Kosiewski --- .../controller/codermcpserver_controller.go | 1 + internal/controller/codermcpserver_delete.go | 6 +- internal/controller/codermcpserver_record.go | 14 ++ .../controller/codermcpserver_record_test.go | 121 ++++++++++++++++++ 4 files changed, 141 insertions(+), 1 deletion(-) diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 50a2fb15..d62ce163 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -227,6 +227,7 @@ func (r *CoderMCPServerReconciler) check( // operator token Secret that appears later. return mcpServerWait(mcpServerRetry, wait.reason, "%s", wait.message), nil } + r.backfillMCPServerRecord(ctx, sdk, server) org, step, err := resolveMCPServerOrganization(ctx, sdk, server.Spec.Organization) if err != nil { return nil, err diff --git a/internal/controller/codermcpserver_delete.go b/internal/controller/codermcpserver_delete.go index d99dee3d..7eb6df4c 100644 --- a/internal/controller/codermcpserver_delete.go +++ b/internal/controller/codermcpserver_delete.go @@ -45,7 +45,11 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder // ID still locates a config, for example after a lost status write. return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, server.UID) } - orgID, err := uuid.Parse(server.Status.OrganizationID) + orgText := server.Status.OrganizationID + if orgText == "" && record != nil { + orgText = record.Organization // A status edit cannot strand the recorded config. + } + orgID, err := uuid.Parse(orgText) if err != nil || orgID == uuid.Nil { // The organization is pinned before the first MCP write. return false, nil, fmt.Errorf("assertion failed: MCP server %s/%s can exist in Coder but has status.organizationID %q", server.Namespace, server.Name, server.Status.OrganizationID) diff --git a/internal/controller/codermcpserver_record.go b/internal/controller/codermcpserver_record.go index e6b5f59c..27f3beb0 100644 --- a/internal/controller/codermcpserver_record.go +++ b/internal/controller/codermcpserver_record.go @@ -7,6 +7,7 @@ import ( "github.com/coder/coder/v2/codersdk" "github.com/google/uuid" + "sigs.k8s.io/controller-runtime/pkg/log" coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" ) @@ -72,6 +73,19 @@ func (r *CoderMCPServerReconciler) writeMCPServerRecord( return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) } +// backfillMCPServerRecord writes the missing record of a config that exists +// in Coder from before records existed. It runs before every other check, so +// an object that a check stalls or disables gets its record too. It never +// stops the reconcile: a failure is logged and retried. +func (r *CoderMCPServerReconciler) backfillMCPServerRecord(ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer) { + if server.Status.ConfigID == "" { + return // The create path writes the record. + } + if step, err := r.ensureMCPServerRecord(ctx, sdk, server); err != nil || step != nil { + log.FromContext(ctx).Info("Cannot write the cleanup record of an MCP server config that exists in Coder", "error", err, "step", step) + } +} + // ensureMCPServerRecord makes sure that a server which can exist in Coder has // a record with its current config ID before the sync replaces the nonce. // Objects from before records existed get one here too. With only a create diff --git a/internal/controller/codermcpserver_record_test.go b/internal/controller/codermcpserver_record_test.go index a5cb127d..e5b88429 100644 --- a/internal/controller/codermcpserver_record_test.go +++ b/internal/controller/codermcpserver_record_test.go @@ -18,6 +18,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client/interceptor" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1" "github.com/coder/coder-k8s/internal/controller" ) @@ -208,6 +209,126 @@ func TestCoderMCPServerRecordBackfill(t *testing.T) { require.Equal(t, env.fake.deploymentID, record["deploymentID"]) } +// #353: a config from before records gets one also while the controller +// disables it because the control plane withdrew its opt-in. +func TestCoderMCPServerRecordBackfillOnDisable(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + require.NoError(t, k8sClient.Delete(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(got.UID)}})) + env.setOptIn(ctx, t, nil) + + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "MCPServersNotAllowed") + require.False(t, env.stored(t, got).view.Enabled, "the disable still runs") + require.Equal(t, id.String(), env.mcpServerRecord(ctx, t, got.UID)["coderID"]) +} + +// #353: a create answer that was lost is confirmed by the next reconcile, +// which records the config ID before the sync replaces the nonce. +func TestCoderMCPServerRecordAfterLostCreateAnswer(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.failNext(routeCreateMCPServer, fakeFault{AfterCommit: true, Reset: true}) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, false, "ConfirmingCreate") + + env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "CoderUnavailable") + require.Equal(t, env.requireCreated(t, got).view.ID.String(), env.mcpServerRecord(ctx, t, got.UID)["coderID"]) +} + +// #353: a config that is gone before a full apply is created again. The +// record write before that create drops the old ID, so a delete finds the new +// config by its nonce instead of trusting the 404 of the old ID. +func TestCoderMCPServerRecreateDropsGoneID(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, old := env.applied(ctx, t, nil) + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, old)) + + env.fake.failNext(routeCreateMCPServer, fakeFault{AfterCommit: true, Reset: true}) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, false, "ConfirmingCreate") + require.Len(t, env.fake.mcpServers(), 1, "the create committed") + require.Empty(t, env.mcpServerRecord(ctx, t, got.UID)["coderID"], "the gone ID left the record") + + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone) + require.Empty(t, env.fake.mcpServers(), "the delete found the new config by its nonce") + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID)) +} + +// #353: when the control plane reaches another Coder deployment while the +// object lives, the config looks gone, but the record still locates it in +// the old deployment. The controller keeps the record and creates nothing in +// the new deployment, and a delete keeps the record too. +func TestCoderMCPServerDeploymentSwitchKeepsRecord(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + before := env.mcpServerRecord(ctx, t, got.UID) + env.fake.mu.Lock() + env.fake.deploymentID = uuid.NewString() // A new database behind the same control plane. + env.fake.mcp.servers = nil + env.fake.mu.Unlock() + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, "CoderDeploymentChanged"), before["deploymentID"]) + require.Equal(t, 1, env.posts(), "no create in the new deployment") + require.Equal(t, before, env.mcpServerRecord(ctx, t, got.UID), "the record keeps the old locator") + + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone, "the finalizer goes without a delete") + require.Zero(t, env.fake.requestCount(routeDeleteMCPServer), "no delete in the new deployment") + require.Equal(t, before, env.mcpServerRecord(ctx, t, got.UID), "the record stays for a sweep") +} + +// #353: a delete never removes a ConfigMap of the record's name that is not +// this object's record. With a status that lost everything, the record still +// names the organization, so the delete of the recorded config finishes. +func TestCoderMCPServerDeleteRecordEdgeCases(t *testing.T) { + t.Parallel() + ctx := context.Background() + t.Run("foreign ConfigMap", func(t *testing.T) { + t.Parallel() + env := newMCPServerEnv(ctx, t, true) + server := env.create(ctx, t, nil) + server.Finalizers = []string{coderv1alpha1.CoderMCPServerCleanupFinalizer} + require.NoError(t, k8sClient.Update(ctx, server)) + foreign := &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(server.UID)}, Data: map[string]string{"owner": "someone else"}} + require.NoError(t, k8sClient.Create(ctx, foreign)) + gone, _ := env.deleteMCPServer(ctx, t, server) + require.Nil(t, gone, "nothing of the object exists in Coder, so the finalizer goes") + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(foreign), foreign), "the foreign ConfigMap stays") + }) + t.Run("status lost everything", func(t *testing.T) { + t.Parallel() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + got.Status = coderv1alpha1.CoderMCPServerStatus{} + require.NoError(t, k8sClient.Status().Update(ctx, got)) + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone) + require.Empty(t, env.fake.mcpServers(), "the delete used the recorded ID and organization") + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID)) + }) +} + // The reconciler refuses to start without its cleanup record dependencies. func TestCoderMCPServerSetupNeedsRecordDependencies(t *testing.T) { t.Parallel() From ece19e1fc987ea3bccf4f541be79707dbd4b38ee Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 18:07:29 +0000 Subject: [PATCH 06/11] docs: note MCP server records after an upgrade (#353) Change-Id: If70ce36eb7a6c8eab57523150a32ff7c9f8064bd Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-mcp-servers.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/how-to/manage-mcp-servers.md b/docs/how-to/manage-mcp-servers.md index 6a3744ec..160ff477 100644 --- a/docs/how-to/manage-mcp-servers.md +++ b/docs/how-to/manage-mcp-servers.md @@ -216,7 +216,7 @@ Delete `CoderMCPServer` objects before their control plane and before their name ### Cleanup records -Before the operator creates a config in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderMCPServer` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the config. The record holds no credential: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the config's temporary display name, the pinned organization ID, the slug, and the config ID once the operator knows it. +Before the operator creates a config in Coder, it writes a cleanup record: a ConfigMap named `ck8s-cleanup-` with the label `coder.com/cleanup-record=CoderMCPServer` in the operator's namespace (`coder-system`). If it cannot write the record, it does not create the config. The record holds no credential: it holds the object's namespace, name and UID, Coder's deployment ID from `/api/v2/buildinfo`, the config's temporary display name, the pinned organization ID, the slug, and the config ID once the operator knows it. A config that an earlier operator version created gets its record on the next reconcile. A config that was left in Coder before that has no record: delete it in the dashboard. When the control plane later reaches another Coder deployment (for example after its database Secret changed), the record still locates the config in the old one. The operator then stalls the object with `CoderDeploymentChanged`, keeps the record and creates nothing in the new deployment. Delete the config in the old deployment, then delete the ConfigMap. From 4c543839b91a9d32a5414b5353562390c2a3af28 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 18:43:37 +0000 Subject: [PATCH 07/11] fix: report CoderDeploymentChanged when an MCP config looks deleted after a deployment switch (#353) Change-Id: I08581faa886e871b8928e4b763b5657bad43653a Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_apply.go | 16 +++++-- internal/controller/codermcpserver_record.go | 32 +++++++++++-- .../controller/codermcpserver_record_test.go | 46 +++++++++++-------- 3 files changed, 67 insertions(+), 27 deletions(-) diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index ac70378d..d443b7a1 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -50,12 +50,18 @@ func (r *CoderMCPServerReconciler) syncMCPServer( switch { // Only Coder's own 404 proves that the config is gone. A proxy page or // "Route not found." waits below and changes nothing (#337). - case isCoderResourceNotFound(err) && applied: - server.Status.Enabled = nil - return mcpServerStalled(mcpServerResync, "DeletedExternally", - "MCP server config %s was deleted in Coder. The controller does not create it again until the spec changes.", id), false, nil case isCoderResourceNotFound(err): - server.Status.ConfigID, server.Status.Credentials, server.Status.Enabled = "", nil, nil + server.Status.Enabled = nil + // The control plane can reach another Coder deployment now, which + // answers 404 for a config of the old one (#353). + if step, err := r.recordedDeploymentChanged(ctx, sdk, server); step != nil || err != nil { + return step, false, err + } + if applied { + return mcpServerStalled(mcpServerResync, "DeletedExternally", + "MCP server config %s was deleted in Coder. The controller does not create it again until the spec changes.", id), false, nil + } + server.Status.ConfigID, server.Status.Credentials = "", nil r.sentCredentials.Delete(client.ObjectKeyFromObject(server)) return nil, true, nil case err != nil: diff --git a/internal/controller/codermcpserver_record.go b/internal/controller/codermcpserver_record.go index 27f3beb0..eb9ae177 100644 --- a/internal/controller/codermcpserver_record.go +++ b/internal/controller/codermcpserver_record.go @@ -45,9 +45,7 @@ func (r *CoderMCPServerReconciler) writeMCPServerRecord( // The record is the only locator of what this object made in the old // Coder database, so the controller neither overwrites it nor creates // in the new one. - return mcpServerStalled(mcpServerResync, "CoderDeploymentChanged", - "MCP server config %s (ID %q) is in Coder deployment %s, but CoderControlPlane %s now reaches deployment %s. Delete the config there, then delete ConfigMap %s/%s. Until then the controller creates nothing.", - server.Spec.Slug, previous.CoderID, previous.DeploymentID, server.Spec.ControlPlaneRef.Name, deploymentID, r.OperatorNamespace, cleanupRecordObjectName(server.UID)), nil + return r.mcpServerDeploymentChanged(server, previous, deploymentID), nil } record := cleanupRecord{ Kind: cleanupRecordKindMCPServer, UID: server.UID, Namespace: server.Namespace, Name: server.Name, @@ -73,6 +71,34 @@ func (r *CoderMCPServerReconciler) writeMCPServerRecord( return nil, writeCleanupRecord(ctx, r.Client, r.OperatorNamespace, existing, record) } +// mcpServerDeploymentChanged is the stall of an object whose record names +// another Coder deployment than the one that the control plane reaches now. +func (r *CoderMCPServerReconciler) mcpServerDeploymentChanged(server *coderv1alpha1.CoderMCPServer, record *cleanupRecord, deploymentID string) *mcpServerStep { + return mcpServerStalled(mcpServerResync, "CoderDeploymentChanged", + "MCP server config %s (ID %q) is in Coder deployment %s, but CoderControlPlane %s now reaches deployment %s. Delete the config there, then delete ConfigMap %s/%s. Until then the controller creates nothing.", + server.Spec.Slug, record.CoderID, record.DeploymentID, server.Spec.ControlPlaneRef.Name, deploymentID, r.OperatorNamespace, cleanupRecordObjectName(server.UID)) +} + +// recordedDeploymentChanged returns the CoderDeploymentChanged stall when the +// record names another Coder deployment than the one that the control plane +// reaches now, or a wait when the deployment ID read fails. Without a record +// it returns nil. A config that looks deleted can then still exist in the +// old deployment. +func (r *CoderMCPServerReconciler) recordedDeploymentChanged(ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer) (*mcpServerStep, error) { + record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, server.UID) + if err != nil || record == nil { + return nil, err + } + deploymentID, err := coderDeploymentID(ctx, sdk) + if err != nil { + return mcpServerWait(mcpServerRetry, "CoderUnavailable", "read the Coder deployment ID: %s", coderErrorSummary(err)), nil + } + if deploymentID != record.DeploymentID { + return r.mcpServerDeploymentChanged(server, record, deploymentID), nil + } + return nil, nil +} + // backfillMCPServerRecord writes the missing record of a config that exists // in Coder from before records existed. It runs before every other check, so // an object that a check stalls or disables gets its record too. It never diff --git a/internal/controller/codermcpserver_record_test.go b/internal/controller/codermcpserver_record_test.go index e5b88429..bc38eccc 100644 --- a/internal/controller/codermcpserver_record_test.go +++ b/internal/controller/codermcpserver_record_test.go @@ -270,31 +270,39 @@ func TestCoderMCPServerRecreateDropsGoneID(t *testing.T) { // #353: when the control plane reaches another Coder deployment while the // object lives, the config looks gone, but the record still locates it in -// the old deployment. The controller keeps the record and creates nothing in +// the old deployment. With or without a pending spec edit, the controller +// stalls with CoderDeploymentChanged, keeps the record and creates nothing in // the new deployment, and a delete keeps the record too. func TestCoderMCPServerDeploymentSwitchKeepsRecord(t *testing.T) { t.Parallel() ctx := context.Background() - env := newMCPServerEnv(ctx, t, true) - env.keySecret(ctx, t, testMCPKey) - got, _ := env.applied(ctx, t, nil) - before := env.mcpServerRecord(ctx, t, got.UID) - env.fake.mu.Lock() - env.fake.deploymentID = uuid.NewString() // A new database behind the same control plane. - env.fake.mcp.servers = nil - env.fake.mu.Unlock() - got.Spec.Description = "pending" - require.NoError(t, k8sClient.Update(ctx, got)) + for name, edit := range map[string]bool{"applied": false, "pending spec edit": true} { + t.Run(name, func(t *testing.T) { + t.Parallel() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + before := env.mcpServerRecord(ctx, t, got.UID) + env.fake.mu.Lock() + env.fake.deploymentID = uuid.NewString() // A new database behind the same control plane. + env.fake.mcp.servers = nil + env.fake.mu.Unlock() + if edit { + got.Spec.Description = "pending" + require.NoError(t, k8sClient.Update(ctx, got)) + } - got, _ = env.reconcile(ctx, t, got) - require.Contains(t, requireMCPServerStep(t, got, true, "CoderDeploymentChanged"), before["deploymentID"]) - require.Equal(t, 1, env.posts(), "no create in the new deployment") - require.Equal(t, before, env.mcpServerRecord(ctx, t, got.UID), "the record keeps the old locator") + got, _ = env.reconcile(ctx, t, got) + require.Contains(t, requireMCPServerStep(t, got, true, "CoderDeploymentChanged"), before["deploymentID"]) + require.Equal(t, 1, env.posts(), "no create in the new deployment") + require.Equal(t, before, env.mcpServerRecord(ctx, t, got.UID), "the record keeps the old locator") - gone, _ := env.deleteMCPServer(ctx, t, got) - require.Nil(t, gone, "the finalizer goes without a delete") - require.Zero(t, env.fake.requestCount(routeDeleteMCPServer), "no delete in the new deployment") - require.Equal(t, before, env.mcpServerRecord(ctx, t, got.UID), "the record stays for a sweep") + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone, "the finalizer goes without a delete") + require.Zero(t, env.fake.requestCount(routeDeleteMCPServer), "no delete in the new deployment") + require.Equal(t, before, env.mcpServerRecord(ctx, t, got.UID), "the record stays for a sweep") + }) + } } // #353: a delete never removes a ConfigMap of the record's name that is not From 62da960e301ec4af28bb709f096f76f28e0936f4 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 19:31:04 +0000 Subject: [PATCH 08/11] test: an MCP record without a config ID deletes nothing and stays while a config can be its own (#353) Change-Id: I8ad87b4d6a2ccdb9a4d21cc6287b928d99d613ce Signed-off-by: Thomas Kosiewski --- internal/controller/cleanup_sweep_mcp_test.go | 137 +++++++++++++----- .../controller/codermcpserver_delete_test.go | 20 ++- 2 files changed, 116 insertions(+), 41 deletions(-) diff --git a/internal/controller/cleanup_sweep_mcp_test.go b/internal/controller/cleanup_sweep_mcp_test.go index 19b1f29e..be25bab1 100644 --- a/internal/controller/cleanup_sweep_mcp_test.go +++ b/internal/controller/cleanup_sweep_mcp_test.go @@ -5,10 +5,12 @@ import ( "testing" "time" + "github.com/coder/coder/v2/codersdk" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/events" clocktesting "k8s.io/utils/clock/testing" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -81,58 +83,117 @@ func TestCleanupSweepMCPServer(t *testing.T) { require.Zero(t, kindMetric(t, "coder_k8s_cleanup_records_unmatched", "CoderMCPServer")) } -// #353 invariants 5 and 6: a sweep never touches the config of a living -// object, finds a config without a recorded ID only by its slug and nonce, -// and leaves a config with the slug that the object did not create. -func TestCleanupSweepMCPServerCases(t *testing.T) { +// #353 invariant 6: a sweep never touches the config of a living object. +func TestCleanupSweepMCPServerLivingObject(t *testing.T) { t.Parallel() ctx := context.Background() - t.Run("living object", func(t *testing.T) { - t.Parallel() - env := newMCPServerEnv(ctx, t, true) - env.keySecret(ctx, t, testMCPKey) - got, _ := env.applied(ctx, t, nil) - s := newMCPSweep(t, env) - s.clock.Step(time.Hour) - requests := env.fake.totalRequests() - require.Equal(t, 30*time.Minute, s.sweep(ctx, t, got.UID).RequeueAfter) - require.Equal(t, requests, env.fake.totalRequests(), "no Coder call for a living object") - require.NotNil(t, env.mcpServerRecord(ctx, t, got.UID)) - }) + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, _ := env.applied(ctx, t, nil) + s := newMCPSweep(t, env) + s.clock.Step(time.Hour) + requests := env.fake.totalRequests() + require.Equal(t, 30*time.Minute, s.sweep(ctx, t, got.UID).RequeueAfter) + require.Equal(t, requests, env.fake.totalRequests(), "no Coder call for a living object") + require.NotNil(t, env.mcpServerRecord(ctx, t, got.UID)) +} + +// #353 invariant 5: without a recorded config ID, a sweep deletes nothing. A +// person can edit the slug and the display name of a config, so neither +// proves that the object created it. The record goes only when the +// organization holds no config with the recorded slug and none with the +// temporary display name. Otherwise it stays for manual cleanup. +func TestCleanupSweepMCPServerWithoutConfigID(t *testing.T) { + t.Parallel() + ctx := context.Background() + editConfig := func(req codersdk.UpdateMCPServerConfigRequest) func(context.Context, *testing.T, mcpServerEnv, *coderv1alpha1.CoderMCPServer) { + return func(ctx context.Context, t *testing.T, env mcpServerEnv, _ *coderv1alpha1.CoderMCPServer) { + t.Helper() + configs := env.fake.mcpServers() + require.Len(t, configs, 1) + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, configs[0].view.ID, req) + require.NoError(t, err) + } + } for name, tc := range map[string]struct { - replace bool // Someone else's config takes the slug. + committed bool // The create commits, and its answer is lost. + edit func(context.Context, *testing.T, mcpServerEnv, *coderv1alpha1.CoderMCPServer) // A person's change in Coder after the object went. + kept bool }{ - "pending create, found by its nonce": {}, - "slug taken by another config": {replace: true}, + "create never committed": {}, + "lost create answer, display name edited": {committed: true, edit: editConfig(codersdk.UpdateMCPServerConfigRequest{DisplayName: ptr.To("Renamed by a person")}), kept: true}, + "lost create answer, slug edited": {committed: true, edit: editConfig(codersdk.UpdateMCPServerConfigRequest{Slug: ptr.To("renamed")}), kept: true}, + "a person's config with the slug": {edit: createOther, kept: true}, + "a person's config with the temporary display name": {edit: createWithNonce, kept: true}, } { t.Run(name, func(t *testing.T) { t.Parallel() env := newMCPServerEnv(ctx, t, true) - env.keySecret(ctx, t, testMCPKey) - env.fake.failNext(routeCreateMCPServer, fakeFault{AfterCommit: true, Reset: true}) - got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) - requireMCPServerStep(t, got, false, "ConfirmingCreate") - env.leaveMCPServer(ctx, t, got) - require.Empty(t, env.mcpServerRecord(ctx, t, got.UID)["coderID"]) - if tc.replace { - for _, c := range env.fake.mcpServers() { - require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, c.view.ID)) - } - createOther(ctx, t, env, got) + got := env.leaveWithoutConfigID(ctx, t, tc.committed) + if tc.edit != nil { + tc.edit(ctx, t, env, got) } - deletes := env.fake.requestCount(routeDeleteMCPServer) + configs, deletes := len(env.fake.mcpServers()), env.fake.requestCount(routeDeleteMCPServer) createTestControlPlane(ctx, t, env.namespace, "coder-b", env.fake.server.URL) s := newMCPSweep(t, env) s.clock.Step(6 * time.Minute) - require.Zero(t, s.sweep(ctx, t, got.UID).RequeueAfter) - require.Nil(t, env.mcpServerRecord(ctx, t, got.UID)) - if tc.replace { - require.Equal(t, deletes, env.fake.requestCount(routeDeleteMCPServer), "the other config is never deleted") - require.Len(t, env.fake.mcpServers(), 1) + + result := s.sweep(ctx, t, got.UID) + require.Equal(t, deletes, env.fake.requestCount(routeDeleteMCPServer), "a record without a config ID deletes nothing") + require.Len(t, env.fake.mcpServers(), configs) + if !tc.kept { + require.Zero(t, result.RequeueAfter) + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID), "Coder holds no config that can be the object's") + require.Empty(t, s.recorder.Events) return } - require.Equal(t, deletes+1, env.fake.requestCount(routeDeleteMCPServer)) - require.Empty(t, env.fake.mcpServers(), "the config with the nonce is deleted") + require.Equal(t, 10*time.Minute, result.RequeueAfter) + require.NotNil(t, env.mcpServerRecord(ctx, t, got.UID), "a config can still be the object's, so the record stays") + require.Len(t, s.recorder.Events, 1) + require.Contains(t, <-s.recorder.Events, "ManualCleanupNeeded") }) } } + +// #353: a record kept for manual cleanup counts in the unmatched gauge until +// a person deletes the config in Coder. The next sweep then removes it. Not +// parallel: it reads process-wide metrics. +func TestCleanupSweepMCPServerManualCleanup(t *testing.T) { + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + got := env.leaveWithoutConfigID(ctx, t, true) + createTestControlPlane(ctx, t, env.namespace, "coder-b", env.fake.server.URL) + s := newMCPSweep(t, env) + s.clock.Step(6 * time.Minute) + + require.Equal(t, 10*time.Minute, s.sweep(ctx, t, got.UID).RequeueAfter) + require.Len(t, env.fake.mcpServers(), 1, "the config of the lost create answer stays") + require.NotNil(t, env.mcpServerRecord(ctx, t, got.UID)) + require.Equal(t, 1.0, kindMetric(t, "coder_k8s_cleanup_records_unmatched", "CoderMCPServer")) + + for _, c := range env.fake.mcpServers() { + require.NoError(t, env.dashboard(t).DeleteMCPServerConfig(ctx, env.orgID, c.view.ID)) + } + require.Zero(t, s.sweep(ctx, t, got.UID).RequeueAfter) + require.Nil(t, env.mcpServerRecord(ctx, t, got.UID), "the config is gone, so the record goes") + require.Zero(t, kindMetric(t, "coder_k8s_cleanup_records_unmatched", "CoderMCPServer")) +} + +// leaveWithoutConfigID leaves the record of an object whose create answer was +// lost, so the record has no config ID. With committed, the create committed +// in Coder. Otherwise Coder holds no config. +func (env mcpServerEnv) leaveWithoutConfigID(ctx context.Context, t *testing.T, committed bool) *coderv1alpha1.CoderMCPServer { + t.Helper() + env.keySecret(ctx, t, testMCPKey) + env.fake.failNext(routeCreateMCPServer, fakeFault{AfterCommit: committed, Reset: true}) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, false, "ConfirmingCreate") + env.leaveMCPServer(ctx, t, got) + require.Empty(t, env.mcpServerRecord(ctx, t, got.UID)["coderID"]) + if committed { + require.Len(t, env.fake.mcpServers(), 1) + } else { + require.Empty(t, env.fake.mcpServers()) + } + return got +} diff --git a/internal/controller/codermcpserver_delete_test.go b/internal/controller/codermcpserver_delete_test.go index 61b76ea5..f30f99a5 100644 --- a/internal/controller/codermcpserver_delete_test.go +++ b/internal/controller/codermcpserver_delete_test.go @@ -144,7 +144,8 @@ func TestCoderMCPServerDeleteWithoutOperatorAccess(t *testing.T) { // Plan 4.10 item 2: with only a create marker, the slug read decides. A // config with this object's nonce is deleted. Someone else's config with the // slug is never touched. With no config, the controller waits until a late -// create cannot commit any more, then releases. +// create cannot commit any more, then releases. #353: the record goes only +// when no config can still be the object's, as in a sweep. func TestCoderMCPServerDeletePendingCreate(t *testing.T) { t.Parallel() for _, tc := range []struct { @@ -153,10 +154,12 @@ func TestCoderMCPServerDeletePendingCreate(t *testing.T) { age time.Duration // Age of the create marker. release bool deletes int + record bool // The record stays. }{ {name: "nonce found", create: createWithNonce, release: true, deletes: 1}, - {name: "someone else's config", create: createOther, release: true}, - {name: "no config, recent create", age: time.Second}, + {name: "someone else's config", create: createOther, release: true, record: true}, + {name: "nonce under another slug", create: createWithNonceUnderOtherSlug, age: 3 * time.Minute, release: true, record: true}, + {name: "no config, recent create", age: time.Second, record: true}, {name: "no config, old create", age: 3 * time.Minute, release: true}, } { t.Run(tc.name, func(t *testing.T) { @@ -168,6 +171,7 @@ func TestCoderMCPServerDeletePendingCreate(t *testing.T) { kept, result := env.deleteMCPServer(ctx, t, server) require.Equal(t, tc.deletes, env.fake.requestCount(routeDeleteMCPServer)-deletes) + require.Equal(t, tc.record, env.mcpServerRecord(ctx, t, server.UID) != nil, "record stays") if tc.release { require.Nil(t, kept) require.Len(t, env.fake.mcpServers(), before-tc.deletes) @@ -271,6 +275,16 @@ func createWithNonce(ctx context.Context, t *testing.T, env mcpServerEnv, server require.NoError(t, err) } +// createWithNonceUnderOtherSlug is the config of a lost create answer after a +// person changed its slug in Coder. +func createWithNonceUnderOtherSlug(ctx context.Context, t *testing.T, env mcpServerEnv, server *coderv1alpha1.CoderMCPServer) { + t.Helper() + req := apiKeyMCPRequest("renamed") + req.DisplayName = "coder-k8s:" + string(server.UID) + _, err := env.dashboard(t).CreateMCPServerConfig(ctx, env.orgID, req) + require.NoError(t, err) +} + func createOther(ctx context.Context, t *testing.T, env mcpServerEnv, server *coderv1alpha1.CoderMCPServer) { t.Helper() _, err := env.dashboard(t).CreateMCPServerConfig(ctx, env.orgID, apiKeyMCPRequest(server.Spec.Slug)) From 6474666aa5386d890d204b88fd9509afc2a531e9 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 19:35:06 +0000 Subject: [PATCH 09/11] fix: an MCP record without a config ID deletes nothing and stays for manual cleanup while a config can be its own (#353) Change-Id: I62170dd522143a8ea677de9a62e78ee6d00ecfda Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-mcp-servers.md | 17 ++- internal/controller/cleanup_sweep.go | 143 ++++++++++-------- .../controller/codermcpserver_controller.go | 12 ++ internal/controller/codermcpserver_delete.go | 37 +++-- 4 files changed, 130 insertions(+), 79 deletions(-) diff --git a/docs/how-to/manage-mcp-servers.md b/docs/how-to/manage-mcp-servers.md index 34061a75..38bfd38d 100644 --- a/docs/how-to/manage-mcp-servers.md +++ b/docs/how-to/manage-mcp-servers.md @@ -207,7 +207,7 @@ Disabling stops every chat from using the server, also chats that selected it ea Deleting the object deletes the config in Coder. Coder removes the server from every chat and deletes the users' OAuth2 tokens for it. The finalizer `coder.com/mcp-server-cleanup` keeps the object until then. -- The operator deletes only a config that this object created. With only an unconfirmed create, it reads Coder first. It deletes a config that carries this object's temporary display name, and it never touches another config with the slug. A create can still commit for a short time, so within 2 minutes of the create request the operator reads again before it trusts that the config is absent. +- The operator deletes only a config that this object created. With only an unconfirmed create, it reads Coder first. It deletes a config that carries this object's temporary display name, and it never touches another config with the slug. A create can still commit for a short time, so within 2 minutes of the create request the operator reads again before it trusts that the config is absent. When another config holds the slug, or a config with the temporary display name has another slug, the finalizer goes without a delete, and the [cleanup record](#cleanup-records) stays. - When Coder is unreachable, or the control plane has no operator access, the finalizer stays and the operator retries every 60 seconds. The operator trusts only Coder's own 404 as proof that the config is gone. A 404 from a proxy keeps the finalizer. - When the control plane is gone, the operator cannot delete the config now. The finalizer goes at once. The operator logs the slug and the config ID with the reason `ControlPlaneGone`. The [cleanup record](#cleanup-records) stays, so a later sweep can delete the config. - When you delete the control plane while `CoderMCPServer` objects that use it are being deleted, the control plane waits for them for up to 10 minutes, plus a grace period for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. @@ -222,7 +222,20 @@ Before the operator creates a config in Coder, it writes a cleanup record: a Con When the control plane later reaches another Coder deployment (for example after its database Secret changed), the record still locates the config in the old one. The operator then stalls the object with `CoderDeploymentChanged`, keeps the record and creates nothing in the new deployment. Delete the config in the old deployment, then delete the ConfigMap. -The operator removes the record when Coder confirms that the config is deleted. When the finalizer goes without a delete, the record stays, and the operator sweeps it as for [AI providers](manage-ai-providers.md#cleanup-records): once the object is gone and 5 minutes have passed since the last create, through a ready control plane in the object's namespace whose Coder answers the record's deployment ID. The sweep deletes the recorded config ID. Without a recorded ID, it deletes only the config with the recorded slug whose display name is still the temporary one. A record that no control plane reaches stays, with the gauge `coder_k8s_cleanup_records_unmatched` and a `NoMatchingDeployment` Warning Event. When that Coder deployment never comes back, delete the config in the Coder dashboard, then delete the ConfigMap. +The operator removes the record when Coder confirms that the config is deleted. When the finalizer goes without a delete, the record stays, and the operator sweeps it as for [AI providers](manage-ai-providers.md#cleanup-records): once the object is gone and 5 minutes have passed since the last create, through a ready control plane in the object's namespace whose Coder answers the record's deployment ID. The sweep deletes the recorded config ID. A record that no control plane reaches stays, with the gauge `coder_k8s_cleanup_records_unmatched` and a `NoMatchingDeployment` Warning Event. When that Coder deployment never comes back, delete the config in the Coder dashboard, then delete the ConfigMap. + +A record without a config ID is left by a create whose answer the operator never got. The sweep deletes nothing for such a record: a person can edit the slug and the display name of any config, so neither proves that the operator created it. When the organization has no config with the recorded slug and none with the temporary display name, the sweep removes the record. Otherwise the record stays, the gauge `coder_k8s_cleanup_records_unmatched` counts it, and the operator writes a `ManualCleanupNeeded` Warning Event on the ConfigMap that names the config. Clean up by hand: + +1. Read the record and its Event: + + ```bash + kubectl -n coder-system get configmap ck8s-cleanup- -o yaml + kubectl -n coder-system get events --field-selector reason=ManualCleanupNeeded + ``` + +2. In the dashboard, open **AI settings > MCP servers** in the recorded organization and find the config that the Event names. +3. When that config is a leftover of the deleted object, delete it. The next sweep, within 10 minutes, then removes the record. +4. When a person owns that config, keep it and delete the ConfigMap. To release a stuck object by hand, remove the finalizer with a JSON patch whose `test` operation checks the index first, as for [AI providers](manage-ai-providers.md#stuck-finalizer). Then check **AI settings > MCP servers** in the dashboard for a leftover config with the slug. diff --git a/internal/controller/cleanup_sweep.go b/internal/controller/cleanup_sweep.go index 8d9b2e89..5604811f 100644 --- a/internal/controller/cleanup_sweep.go +++ b/internal/controller/cleanup_sweep.go @@ -50,7 +50,7 @@ var ( }, []string{"kind"}) cleanupRecordsUnmatched = prometheus.NewGaugeVec(prometheus.GaugeOpts{ Name: "coder_k8s_cleanup_records_unmatched", - Help: "Cleanup records of gone objects that no ready control plane of their Coder deployment reaches.", + Help: "Cleanup records of gone objects that no ready control plane of their Coder deployment reaches, or that wait for manual cleanup because nothing proves which Coder resource is theirs.", }, []string{"kind"}) ) @@ -64,8 +64,8 @@ func init() { // without a confirmed delete in Coder (#353). It acts only through a ready // control plane whose current Coder deployment ID equals the record's, and // it deletes only the recorded provider or MCP server config ID, the provider -// or config with the recorded nonce, or the AI Gateway keys whose names -// start with the recorded prefix. +// with the recorded nonce, or the AI Gateway keys whose names start with the +// recorded prefix. type CleanupRecordReconciler struct { client.Client // APIReader reads the object of a record live, so a stale cache never @@ -75,7 +75,7 @@ type CleanupRecordReconciler struct { Clock clock.Clock Recorder events.EventRecorder - unmatched sync.Map // types.UID -> kind of a record that no deployment reaches. + unmatched sync.Map // types.UID -> kind of a record that no deployment reaches, or that waits for manual cleanup. } // SetupWithManager watches the records in the operator namespace. It wakes a @@ -191,22 +191,29 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques record.Namespace, record.DeploymentID, kind.what, record.Kind, record.Namespace, record.Name, kind.what) return ctrl.Result{RequeueAfter: cleanupUnmatchedRecheck}, nil } - r.setUnmatched(record.UID, record.Kind, false) logger := log.FromContext(ctx).WithValues("record", configMap.Name, "deploymentID", record.DeploymentID, "object", record.Kind+" "+record.Namespace+"/"+record.Name) // Each matching control plane has its own operator token, so a failure // through one tries the next. - gone, why, through := false, "", logger + outcome, why, through := sweepRetry, "", logger for _, m := range matches { through = logger.WithValues("controlPlane", types.NamespacedName{Namespace: m.controlPlane.Namespace, Name: m.controlPlane.Name}) - if gone, why, err = kind.sweep(ctx, m.sdk, record); err != nil { + if outcome, why, err = kind.sweep(ctx, m.sdk, record); err != nil { return ctrl.Result{}, err - } else if gone { + } else if outcome != sweepRetry { break } through.Info("The cleanup of a deleted object in Coder failed", "why", why) } - if !gone { + r.setUnmatched(record.UID, record.Kind, outcome == sweepKept) + switch outcome { + case sweepRetry: return ctrl.Result{RequeueAfter: cleanupRetry}, nil + case sweepKept: + r.Recorder.Eventf(configMap, nil, corev1.EventTypeWarning, "ManualCleanupNeeded", "Sweep", + "Coder deployment %s holds %s, and nothing proves that deleted %s %s/%s created it, so the sweep deletes nothing. If it is a leftover of that object, delete it in Coder. Then delete this ConfigMap.", + record.DeploymentID, why, record.Kind, record.Namespace, record.Name) + through.Info("Kept the cleanup record of a deleted object for manual cleanup", "why", why) + return ctrl.Result{RequeueAfter: cleanupUnmatchedRecheck}, nil } // Coder confirmed the delete in the matching deployment (invariant 7). if err := deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, record.UID); err != nil { @@ -217,11 +224,25 @@ func (r *CleanupRecordReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, nil } +// sweepOutcome is what a sweep of one record found in Coder. +type sweepOutcome int + +const ( + // sweepRetry: Coder confirmed nothing. The sweep tries again. + sweepRetry sweepOutcome = iota + // sweepGone: Coder holds nothing of the record, so the record goes. + sweepGone + // sweepKept: Coder holds a resource that can be the record's, but + // nothing proves it. The sweep deletes nothing, and the record stays + // for a person. + sweepKept +) + // cleanupSweepKind is how a sweep handles the records of one object kind. type cleanupSweepKind struct { object func() client.Object what string // What the object holds in Coder, for the Event. - sweep func(context.Context, *codersdk.Client, *cleanupRecord) (gone bool, why string, err error) + sweep func(context.Context, *codersdk.Client, *cleanupRecord) (outcome sweepOutcome, why string, err error) } var cleanupRecordKinds = map[string]cleanupSweepKind{ @@ -291,47 +312,47 @@ func (r *CleanupRecordReconciler) matchingControlPlanes(ctx context.Context, rec // a site owner, or a provider of that name without the record's nonce. It // deletes only the recorded ID or the provider whose display name is the // recorded nonce (invariant 5). -func sweepAIProvider(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (gone bool, why string, err error) { +func sweepAIProvider(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (outcome sweepOutcome, why string, err error) { if record.Nonce != aiProviderNoncePrefix+string(record.UID) { - return false, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce) + return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce) } if step := requireSiteOwner(ctx, sdk); step != nil { // Only a site owner can trust Coder's 404 for a provider. - return false, step.message, nil + return sweepRetry, step.message, nil } id := record.CoderID if id != "" { if parsed, err := uuid.Parse(id); err != nil || parsed == uuid.Nil || parsed.String() != id { - return false, "", fmt.Errorf("assertion failed: cleanup record of %s has coderID %q, which is not a canonical UUID", record.UID, id) + return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has coderID %q, which is not a canonical UUID", record.UID, id) } } else { found, err := sdk.AIProvider(ctx, record.Name) switch { case isCoderResourceNotFound(err): - return true, "no provider of that name", nil + return sweepGone, "no provider of that name", nil case err != nil: - return false, "get provider: " + coderErrorSummary(err), nil + return sweepRetry, "get provider: " + coderErrorSummary(err), nil } if err := coderAnswerFor("AI provider", record.Name, found.Name); err != nil { - return false, err.Error(), nil //nolint:nilerr // A wrong answer retries the sweep. + return sweepRetry, err.Error(), nil //nolint:nilerr // A wrong answer retries the sweep. } if found.DisplayName != record.Nonce || string(found.Type) != record.Type { // Another provider holds the name, so no create of this object // committed after the settle window. The nonce and the type // prove ownership, as on the controller's own paths. - return true, "the provider of that name is not this object's", nil + return sweepGone, "the provider of that name is not this object's", nil } if found.ID == uuid.Nil { - return false, "", errors.New("assertion failed: Coder answered an AI provider without an ID") + return sweepRetry, "", errors.New("assertion failed: Coder answered an AI provider without an ID") } id = found.ID.String() } if err := sdk.DeleteAIProvider(ctx, id); err != nil && !isCoderResourceNotFound(err) { - return false, "delete provider " + id + ": " + coderErrorSummary(err), nil + return sweepRetry, "delete provider " + id + ": " + coderErrorSummary(err), nil } else if err != nil { - return true, "provider " + id + " was already gone", nil //nolint:nilerr // Coder's own 404 confirms the delete. + return sweepGone, "provider " + id + " was already gone", nil //nolint:nilerr // Coder's own 404 confirms the delete. } - return true, "deleted provider " + id, nil + return sweepGone, "deleted provider " + id, nil } // sweepAIGatewayKeys deletes the keys of record in Coder: every key whose @@ -339,93 +360,91 @@ func sweepAIProvider(ctx context.Context, sdk *codersdk.Client, record *cleanupR // object is gone, so no serial bound applies: the settle window has passed, // and no create of the object can still commit. It returns gone only when a // LIST, read by a site owner, shows no such key. -func sweepAIGatewayKeys(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (gone bool, why string, err error) { +func sweepAIGatewayKeys(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (outcome sweepOutcome, why string, err error) { prefix, err := aiGatewayKeyNamePrefix(record.UID) if err != nil { - return false, "", err + return sweepRetry, "", err } if record.Nonce != prefix { - return false, "", fmt.Errorf("assertion failed: cleanup record of %s has key prefix %q, not the prefix of its UID", record.UID, record.Nonce) + return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has key prefix %q, not the prefix of its UID", record.UID, record.Nonce) } if step := requireSiteOwner(ctx, sdk); step != nil { // Only a site owner's LIST shows every key. - return false, step.message, nil + return sweepRetry, step.message, nil } deleted := 0 for range 2 { listed, err := sdk.ListAIGatewayKeys(ctx) if err != nil { - return false, "list AI Gateway keys: " + coderErrorSummary(err), nil + return sweepRetry, "list AI Gateway keys: " + coderErrorSummary(err), nil } own := ownAIGatewayKeys(prefix, listed) if len(own) == 0 { - return true, fmt.Sprintf("deleted %d keys, and a list shows none left", deleted), nil + return sweepGone, fmt.Sprintf("deleted %d keys, and a list shows none left", deleted), nil } for _, k := range own { // Only Coder's own 404 proves that the key is gone (#325). if err := sdk.DeleteAIGatewayKey(ctx, k.ID); err != nil && !isCoderResourceNotFound(err) { - return false, "delete key " + k.Name + ": " + coderErrorSummary(err), nil + return sweepRetry, "delete key " + k.Name + ": " + coderErrorSummary(err), nil } deleted++ } } - return false, "Coder still lists keys of the object after their delete", nil + return sweepRetry, "Coder still lists keys of the object after their delete", nil } -// sweepMCPServer deletes the MCP server config of record in Coder. It -// returns gone when Coder confirms that it holds no config of the record: a -// delete answered with success or Coder's own 404, or a list by a site owner -// that shows no config of the record's slug with the record's nonce. It -// deletes only the recorded ID or the config whose display name is the -// recorded nonce (invariant 5). -func sweepMCPServer(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (gone bool, why string, err error) { +// sweepMCPServer deletes the recorded config ID in Coder (invariant 5), and +// returns gone when Coder answers that delete with success or its own 404. +// Without a recorded ID it deletes nothing: a person can edit the slug and +// the display name of a config, so neither proves that this object created +// it. It then returns gone when a list by a site owner shows no config with +// the record's slug and none with its nonce, and kept otherwise. +func sweepMCPServer(ctx context.Context, sdk *codersdk.Client, record *cleanupRecord) (outcome sweepOutcome, why string, err error) { if record.Nonce != mcpServerNoncePrefix+string(record.UID) { - return false, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce) + return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has nonce %q, not the nonce of its UID", record.UID, record.Nonce) } orgID, err := uuid.Parse(record.Organization) if err != nil || orgID == uuid.Nil || orgID.String() != record.Organization || record.Slug == "" { - return false, "", fmt.Errorf("assertion failed: cleanup record of %s has organization ID %q and slug %q", record.UID, record.Organization, record.Slug) + return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has organization ID %q and slug %q", record.UID, record.Organization, record.Slug) } if step := mcpServerRequireSiteOwner(ctx, sdk); step != nil { // Only a site owner sees every config in the list. - return false, step.message, nil + return sweepRetry, step.message, nil } - var id uuid.UUID - if record.CoderID != "" { - if id, err = uuid.Parse(record.CoderID); err != nil || id == uuid.Nil || id.String() != record.CoderID { - return false, "", fmt.Errorf("assertion failed: cleanup record of %s has coderID %q, which is not a canonical UUID", record.UID, record.CoderID) - } - } else { + if record.CoderID == "" { configs, err := sdk.MCPServerConfigs(ctx, orgID) switch { case isCoderResourceNotFound(err): - return true, "the organization is gone", nil + return sweepGone, "the organization is gone", nil case err != nil: - return false, "list MCP server configs: " + coderErrorSummary(err), nil + return sweepRetry, "list MCP server configs: " + coderErrorSummary(err), nil } found, step := mcpServerWithSlug(configs, record.Slug, orgID) - switch { - case step != nil: - return false, step.message, nil - case found == nil: - return true, "no config with that slug", nil - case found.DisplayName != record.Nonce: - // Another config holds the slug, so no create of this object - // committed after the settle window. - return true, "the config with that slug is not this object's", nil + if step != nil { + return sweepRetry, step.message, nil } - id = found.ID + if found == nil { + found = mcpServerWithDisplayName(configs, record.Nonce) + } + if found == nil { + return sweepGone, "no config with that slug or display name", nil + } + return sweepKept, fmt.Sprintf("MCP server config %s (slug %q) in organization %s", found.ID, found.Slug, orgID), nil + } + id, err := uuid.Parse(record.CoderID) + if err != nil || id == uuid.Nil || id.String() != record.CoderID { + return sweepRetry, "", fmt.Errorf("assertion failed: cleanup record of %s has coderID %q, which is not a canonical UUID", record.UID, record.CoderID) } if err := sdk.DeleteMCPServerConfig(ctx, orgID, id); err != nil && !isCoderResourceNotFound(err) { - return false, "delete MCP server config " + id.String() + ": " + coderErrorSummary(err), nil + return sweepRetry, "delete MCP server config " + id.String() + ": " + coderErrorSummary(err), nil } else if err != nil { - return true, "MCP server config " + id.String() + " was already gone", nil //nolint:nilerr // Coder's own 404 confirms the delete. + return sweepGone, "MCP server config " + id.String() + " was already gone", nil //nolint:nilerr // Coder's own 404 confirms the delete. } - return true, "deleted MCP server config " + id.String(), nil + return sweepGone, "deleted MCP server config " + id.String(), nil } // setUnmatched keeps the unmatched gauge equal to the records that no -// deployment reaches. +// deployment reaches or that wait for manual cleanup. func (r *CleanupRecordReconciler) setUnmatched(uid types.UID, kind string, unmatched bool) { if unmatched { r.unmatched.Store(uid, kind) diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index ae756d2d..6bb9ce1f 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -541,6 +541,18 @@ func mcpServerWithSlug(configs []codersdk.MCPServerConfig, slug string, orgID uu return nil, nil } +// mcpServerWithDisplayName returns a config with displayName from configs, or +// nil. A person can change the slug of the config of a lost create answer, so +// its nonce can sit under another slug. +func mcpServerWithDisplayName(configs []codersdk.MCPServerConfig, displayName string) *codersdk.MCPServerConfig { + for i := range configs { + if configs[i].DisplayName == displayName { + return &configs[i] + } + } + return nil +} + // patchFinalizer adds or removes the cleanup finalizer with a merge patch. The // optimistic lock turns a concurrent finalizer change into a conflict, not a // lost update. diff --git a/internal/controller/codermcpserver_delete.go b/internal/controller/codermcpserver_delete.go index 7eb6df4c..116b9654 100644 --- a/internal/controller/codermcpserver_delete.go +++ b/internal/controller/codermcpserver_delete.go @@ -123,7 +123,12 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder } recordedID = record.CoderID } - step, err = deleteMCPServerInCoder(ctx, sdk, server, orgID, recordedID) + unproven, step, err := deleteMCPServerInCoder(ctx, sdk, server, orgID, recordedID) + if err == nil && step == nil && unproven { + // A config that can be this object's stays in Coder, so the record + // stays for a sweep, which reports it for manual cleanup (#353). + return true, nil, nil + } if err == nil && step == nil { // Coder confirmed that it holds no config of this object. Only then // does the record go (#353 invariant 7). @@ -144,17 +149,19 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder } // deleteMCPServerInCoder deletes the config of this object in Coder and -// returns nil when Coder holds none. Without status.configID it deletes +// returns a nil step when it is done. Without status.configID it deletes // recordedID, the config ID of the cleanup record: a failed status write // after the create can leave the ID only there. With only a create marker, a // read decides first: only the nonce proves that the config is this -// object's. Only Coder's own 404 proves absence (#337). -func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, recordedID string) (*mcpServerStep, error) { +// object's. Only Coder's own 404 proves absence (#337). unproven reports a +// config that it leaves although it can be this object's: another config +// with the slug, or a config with the nonce under another slug. +func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID uuid.UUID, recordedID string) (unproven bool, step *mcpServerStep, err error) { var id uuid.UUID if configID := cmp.Or(server.Status.ConfigID, recordedID); configID != "" { parsed, err := uuid.Parse(configID) if err != nil || parsed == uuid.Nil || parsed.String() != configID { - return nil, fmt.Errorf("assertion failed: MCP server %s/%s has config ID %q, which is not a canonical UUID", server.Namespace, server.Name, configID) + return false, nil, fmt.Errorf("assertion failed: MCP server %s/%s has config ID %q, which is not a canonical UUID", server.Namespace, server.Name, configID) } id = parsed } else { @@ -162,7 +169,7 @@ func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *c if err != nil && !isCoderResourceNotFound(err) { step := mcpServerListFailed(err) step.requeue = mcpServerDeletePoll - return step, nil + return false, step, nil } // Coder's own 404 means that the pinned organization is gone, and no // config of it can be reached any more (#378). @@ -170,32 +177,32 @@ func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *c switch { case step != nil: step.requeue = max(step.requeue, mcpServerDeletePoll) - return step, nil + return false, step, nil case found == nil: // A missing config proves absence only for a site owner. A role // change in Coder has no watch, so the wait uses the delete poll. if step := mcpServerRequireSiteOwner(ctx, sdk); step != nil { step.requeue = mcpServerDeletePoll - return step, nil + return false, step, nil } if wait := mcpServerCreateSettle - time.Since(server.Status.CreateAttemptTime.Time); wait > 0 { // A create sent moments ago can still commit after this read. - return mcpServerWait(wait, "ConfirmingCreate", + return false, mcpServerWait(wait, "ConfirmingCreate", "MCP server config %s is not in Coder, but its create request was sent at %s and can still commit. The controller reads again before it releases the finalizer.", server.Spec.Slug, server.Status.CreateAttemptTime.UTC().Format(time.RFC3339)), nil } - return nil, nil + return mcpServerWithDisplayName(configs, mcpServerNonce(server)) != nil, nil, nil case found.DisplayName != mcpServerNonce(server): - // Someone else's config holds the slug, so no create of this - // object committed. The controller never deletes it. - return nil, nil + // Someone else's config holds the slug, or a person renamed this + // object's config. The controller never deletes it. + return true, nil, nil } id = found.ID } if err := sdk.DeleteMCPServerConfig(ctx, orgID, id); err != nil && !isCoderResourceNotFound(err) { - return mcpServerWait(mcpServerDeletePoll, "CoderUnavailable", "delete MCP server config %s (ID %s): %s", server.Spec.Slug, id, coderErrorSummary(err)), nil + return false, mcpServerWait(mcpServerDeletePoll, "CoderUnavailable", "delete MCP server config %s (ID %s): %s", server.Spec.Slug, id, coderErrorSummary(err)), nil } - return nil, nil + return false, nil, nil } // releaseWithoutDelete logs what can stay in Coder, because nothing can From 5c4eaddacd917cb8e9ac36df0f17a2553dd4ab7f Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 19:51:09 +0000 Subject: [PATCH 10/11] test: cover MCP records on delete after a slug conflict, for legacy configs, and an organization stall after a deployment switch (#353) Change-Id: I31648be19fc5faaf5555f56916188df5c819b43b Signed-off-by: Thomas Kosiewski --- .../controller/codermcpserver_delete_test.go | 26 ++++++++++++++ .../controller/codermcpserver_record_test.go | 35 +++++++++++++++++-- 2 files changed, 59 insertions(+), 2 deletions(-) diff --git a/internal/controller/codermcpserver_delete_test.go b/internal/controller/codermcpserver_delete_test.go index f30f99a5..e7021ca2 100644 --- a/internal/controller/codermcpserver_delete_test.go +++ b/internal/controller/codermcpserver_delete_test.go @@ -185,6 +185,32 @@ func TestCoderMCPServerDeletePendingCreate(t *testing.T) { } } +// #353: when a person renames the config of a lost create answer, the +// controller reports SlugConflict and drops its create marker. A delete then +// keeps the record, so a sweep can report the config for manual cleanup. +func TestCoderMCPServerDeleteKeepsRecordAfterSlugConflict(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + env.fake.failNext(routeCreateMCPServer, fakeFault{AfterCommit: true, Reset: true}) + got, _ := env.reconcile(ctx, t, env.create(ctx, t, nil)) + requireMCPServerStep(t, got, false, "ConfirmingCreate") + configs := env.fake.mcpServers() + require.Len(t, configs, 1) + displayName := "Renamed by a person" + _, err := env.dashboard(t).UpdateMCPServerConfig(ctx, env.orgID, configs[0].view.ID, codersdk.UpdateMCPServerConfigRequest{DisplayName: &displayName}) + require.NoError(t, err) + got, _ = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "SlugConflict") + require.Nil(t, got.Status.CreateAttemptTime) + + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone) + require.Len(t, env.fake.mcpServers(), 1, "the controller never deletes a config that it cannot prove its own") + require.NotNil(t, env.mcpServerRecord(ctx, t, got.UID), "the record stays for a sweep") +} + // pendingCreate returns an object with only a create marker of the given age. // Coder holds no config of it, except one that create adds. func (env mcpServerEnv) pendingCreate( diff --git a/internal/controller/codermcpserver_record_test.go b/internal/controller/codermcpserver_record_test.go index bc38eccc..1f97ae5e 100644 --- a/internal/controller/codermcpserver_record_test.go +++ b/internal/controller/codermcpserver_record_test.go @@ -226,6 +226,27 @@ func TestCoderMCPServerRecordBackfillOnDisable(t *testing.T) { require.Equal(t, id.String(), env.mcpServerRecord(ctx, t, got.UID)["coderID"]) } +// #353: a config from before records gets one also when its object is +// already being deleted, so a release without a confirmed delete leaves a +// record for a sweep. +func TestCoderMCPServerRecordBackfillOnDelete(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + got, id := env.applied(ctx, t, nil) + require.NoError(t, k8sClient.Delete(ctx, &corev1.ConfigMap{ObjectMeta: metav1.ObjectMeta{Namespace: env.namespace, Name: "ck8s-cleanup-" + string(got.UID)}})) + holdControlPlane(ctx, t, env.namespace) + env.fake.failNext(routeDeleteMCPServer, fakeFault{Status: http.StatusBadGateway}) + + gone, _ := env.deleteMCPServer(ctx, t, got) + require.Nil(t, gone, "released after one attempt") + require.Len(t, env.fake.mcpServers(), 1) + record := env.mcpServerRecord(ctx, t, got.UID) + require.Equal(t, id.String(), record["coderID"], "the record stays for a sweep") + require.Equal(t, env.fake.deploymentID, record["deploymentID"]) +} + // #353: a create answer that was lost is confirmed by the next reconcile, // which records the config ID before the sync replaces the nonce. func TestCoderMCPServerRecordAfterLostCreateAnswer(t *testing.T) { @@ -276,7 +297,14 @@ func TestCoderMCPServerRecreateDropsGoneID(t *testing.T) { func TestCoderMCPServerDeploymentSwitchKeepsRecord(t *testing.T) { t.Parallel() ctx := context.Background() - for name, edit := range map[string]bool{"applied": false, "pending spec edit": true} { + for name, tc := range map[string]struct { + edit bool // A spec edit is pending. + noOrgID bool // The new database has no organization with the pinned ID. + }{ + "applied": {}, + "pending spec edit": {edit: true}, + "organization not in the new one": {noOrgID: true}, + } { t.Run(name, func(t *testing.T) { t.Parallel() env := newMCPServerEnv(ctx, t, true) @@ -286,8 +314,11 @@ func TestCoderMCPServerDeploymentSwitchKeepsRecord(t *testing.T) { env.fake.mu.Lock() env.fake.deploymentID = uuid.NewString() // A new database behind the same control plane. env.fake.mcp.servers = nil + if tc.noOrgID { + delete(env.fake.orgs, env.orgID) + } env.fake.mu.Unlock() - if edit { + if tc.edit { got.Spec.Description = "pending" require.NoError(t, k8sClient.Update(ctx, got)) } From 22aa163ecaae64da4b8af5b2085a3de542f34660 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Fri, 30 Oct 2026 19:52:25 +0000 Subject: [PATCH 11/11] fix: keep MCP records after a slug conflict, backfill them on delete, and report a deployment switch before an organization stall (#353) Change-Id: I409d537e4e4ee56806d712721808ab8c9ea9d324 Signed-off-by: Thomas Kosiewski --- docs/how-to/manage-mcp-servers.md | 4 ++-- .../controller/codermcpserver_controller.go | 5 +++++ internal/controller/codermcpserver_delete.go | 20 ++++++++++++++++--- 3 files changed, 24 insertions(+), 5 deletions(-) diff --git a/docs/how-to/manage-mcp-servers.md b/docs/how-to/manage-mcp-servers.md index 38bfd38d..5f98561f 100644 --- a/docs/how-to/manage-mcp-servers.md +++ b/docs/how-to/manage-mcp-servers.md @@ -207,7 +207,7 @@ Disabling stops every chat from using the server, also chats that selected it ea Deleting the object deletes the config in Coder. Coder removes the server from every chat and deletes the users' OAuth2 tokens for it. The finalizer `coder.com/mcp-server-cleanup` keeps the object until then. -- The operator deletes only a config that this object created. With only an unconfirmed create, it reads Coder first. It deletes a config that carries this object's temporary display name, and it never touches another config with the slug. A create can still commit for a short time, so within 2 minutes of the create request the operator reads again before it trusts that the config is absent. When another config holds the slug, or a config with the temporary display name has another slug, the finalizer goes without a delete, and the [cleanup record](#cleanup-records) stays. +- The operator deletes only a config that this object created. With only an unconfirmed create, it reads Coder first. It deletes a config that carries this object's temporary display name, and it never touches another config with the slug. A create can still commit for a short time, so within 2 minutes of the create request the operator reads again before it trusts that the config is absent. When another config holds the slug, or a config with the temporary display name has another slug, the finalizer goes without a delete, and the [cleanup record](#cleanup-records) stays. A record without a config ID also stays when the object reports `SlugConflict` after a create: a person can have renamed the config of that create. - When Coder is unreachable, or the control plane has no operator access, the finalizer stays and the operator retries every 60 seconds. The operator trusts only Coder's own 404 as proof that the config is gone. A 404 from a proxy keeps the finalizer. - When the control plane is gone, the operator cannot delete the config now. The finalizer goes at once. The operator logs the slug and the config ID with the reason `ControlPlaneGone`. The [cleanup record](#cleanup-records) stays, so a later sweep can delete the config. - When you delete the control plane while `CoderMCPServer` objects that use it are being deleted, the control plane waits for them for up to 10 minutes, plus a grace period for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait. @@ -224,7 +224,7 @@ When the control plane later reaches another Coder deployment (for example after The operator removes the record when Coder confirms that the config is deleted. When the finalizer goes without a delete, the record stays, and the operator sweeps it as for [AI providers](manage-ai-providers.md#cleanup-records): once the object is gone and 5 minutes have passed since the last create, through a ready control plane in the object's namespace whose Coder answers the record's deployment ID. The sweep deletes the recorded config ID. A record that no control plane reaches stays, with the gauge `coder_k8s_cleanup_records_unmatched` and a `NoMatchingDeployment` Warning Event. When that Coder deployment never comes back, delete the config in the Coder dashboard, then delete the ConfigMap. -A record without a config ID is left by a create whose answer the operator never got. The sweep deletes nothing for such a record: a person can edit the slug and the display name of any config, so neither proves that the operator created it. When the organization has no config with the recorded slug and none with the temporary display name, the sweep removes the record. Otherwise the record stays, the gauge `coder_k8s_cleanup_records_unmatched` counts it, and the operator writes a `ManualCleanupNeeded` Warning Event on the ConfigMap that names the config. Clean up by hand: +A record without a config ID is left by a create that the operator could not confirm. The sweep deletes nothing for such a record: a person can edit the slug and the display name of any config, so neither proves that the operator created it. When the organization has no config with the recorded slug and none with the temporary display name, the sweep removes the record. Otherwise the record stays, the gauge `coder_k8s_cleanup_records_unmatched` counts it, and the operator writes a `ManualCleanupNeeded` Warning Event on the ConfigMap that names the config. Clean up by hand: 1. Read the record and its Event: diff --git a/internal/controller/codermcpserver_controller.go b/internal/controller/codermcpserver_controller.go index 6bb9ce1f..5f5ecb6f 100644 --- a/internal/controller/codermcpserver_controller.go +++ b/internal/controller/codermcpserver_controller.go @@ -245,6 +245,11 @@ func (r *CoderMCPServerReconciler) check( return optInStep, err } } + // A new Coder database can miss the organization too. The recovery + // then is the deployment's, so that stall goes first. + if switched, err := r.recordedDeploymentChanged(ctx, sdk, server); switched != nil || err != nil { + return switched, err + } return step, nil } server.Status.OrganizationID = org.ID.String() diff --git a/internal/controller/codermcpserver_delete.go b/internal/controller/codermcpserver_delete.go index 116b9654..e5ebc58c 100644 --- a/internal/controller/codermcpserver_delete.go +++ b/internal/controller/codermcpserver_delete.go @@ -41,9 +41,13 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder return false, nil, err } if server.Status.ConfigID == "" && server.Status.CreateAttemptTime == nil && (record == nil || record.CoderID == "") { - // Nothing of this object can exist in Coder. A record with a config - // ID still locates a config, for example after a lost status write. - return true, nil, deleteCleanupRecord(ctx, r.Client, r.APIReader, r.OperatorNamespace, server.UID) + // The status shows nothing of this object in Coder. A record with a + // config ID still locates a config, for example after a lost status + // write. A record without one stays too: a SlugConflict clears the + // marker also when a person renamed the config of a lost create + // answer. The sweep removes it once Coder holds no config with the + // slug or the nonce. + return true, nil, nil } orgText := server.Status.OrganizationID if orgText == "" && record != nil { @@ -107,6 +111,16 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder ctx, cancel = context.WithTimeout(ctx, mcpServerBoundedDeleteTimeout) defer cancel() } + if record == nil && server.Status.ConfigID != "" { + // A config from before records existed gets its record before a + // delete that can release without a confirmation. + r.backfillMCPServerRecord(ctx, sdk, server) + if record, _, err = readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, server.UID); errors.Is(err, errNotACleanupRecord) { + record = nil + } else if err != nil { + return false, nil, err + } + } recordedID := "" if record != nil { // The record names the deployment that holds the config. When the