🤖 fix: hold a deleted CoderControlPlane while CoderMCPServer objects still use it

docs/how-to/manage-mcp-servers.md

@@ -137,7 +137,7 @@ When a group in `access.groups` does not exist, the sync fails closed:
 1. It removes every entry that `access.groups` no longer names, also user entries.
 2. It adds nothing. Access never widens while a group is missing.
 3. If the same spec generation disables the server (`enabled: false`), it sends one update that holds only `enabled: false`.
-4. It leaves the rest of the generation unapplied. The object stalls with `GroupNotFound` and checks again every 10 minutes.
+4. It leaves the rest of the generation unapplied. The object stalls with `GroupNotFound`. When the name is valid but no such group exists yet, the operator checks again every 10 minutes. The API server refuses a name that is not a valid Coder group name. If such a name reaches the operator anyway, the operator does not check again until the spec changes: fix the name.
 5. Once the missing groups exist, the sync adds them and applies the rest of the generation.
 
 A new server stays disabled in Coder while one of its groups is missing.
@@ -207,7 +207,8 @@ 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.
-- While the control plane is being deleted, the operator sends one delete with a 10-second limit, then removes the finalizer, also when that delete fails. So a namespace deletion finishes, but it can leave the config in Coder.
+- 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), the operator sends one delete with a 10-second limit, then removes the finalizer, also when that delete fails. So a namespace deletion finishes, but it can leave the config in Coder.
 - A withdrawn opt-in never blocks a delete.
 
 Delete `CoderMCPServer` objects before their control plane and before their namespace.

internal/controller/codercontrolplane_controller.go

@@ -623,8 +623,8 @@ func (r *CoderControlPlaneReconciler) finalizeWorkspaceRBAC(
 		return ctrl.Result{}, err
 	}
 	// The finalizer keeps the Coder Deployment (owned by this object)
-	// running while AI Gateway keys and AI providers that use it still
-	// delete their resources in Coder (#353). The grace period after the hold
+	// running while AI Gateway keys, AI providers and MCP servers that use
+	// it still delete their resources in Coder (#353, #382). The grace period after the hold
 	// lets each of them run its bounded pass before Coder goes.
 	now := r.now()
 	dependents, perKind, err := controlPlaneDependents(ctx, r.Client, coderControlPlane, now)
@@ -3309,6 +3309,8 @@ func (r *CoderControlPlaneReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Watches(&coderv1alpha1.CoderAIGatewayKey{}, handler.EnqueueRequestsFromMapFunc(requestForDependentControlPlane),
 			ctrlbuilder.WithPredicates(dependentRemoved)).
 		Watches(&coderv1alpha1.CoderAIProvider{}, handler.EnqueueRequestsFromMapFunc(requestForDependentControlPlane),
+			ctrlbuilder.WithPredicates(dependentRemoved)).
+		Watches(&coderv1alpha1.CoderMCPServer{}, handler.EnqueueRequestsFromMapFunc(requestForDependentControlPlane),
 			ctrlbuilder.WithPredicates(dependentRemoved))
 
 	// Gateway API is optional; only watch HTTPRoutes when the CRD is installed.

internal/controller/codermcpserver_delete.go

@@ -49,7 +49,33 @@ func (r *CoderMCPServerReconciler) deleteStep(ctx context.Context, server *coder
 		return false, nil, err
 	}
 	deleting := !controlPlane.DeletionTimestamp.IsZero()
-	sdk, _, wait, err := controlPlaneCoderClientFor(ctx, r.Client, ref.Namespace, ref.Name, deleting)
+	if deleting {
+		// While the control plane holds for this object, Coder keeps running:
+		// use the normal path, which retries, instead of the bounded pass
+		// (#382, #353).
+		now := time.Now()
+		holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, now)
+		if err != nil {
+			return false, nil, err
+		}
+		deleting = !holds
+		if holds {
+			// A pass under the hold ends at the hold end, however slowly
+			// Coder answers. The bounded pass after it then still fits in
+			// the control plane's grace period.
+			var cancel context.CancelFunc
+			ctx, cancel = context.WithDeadline(ctx, controlPlaneHoldEnd(controlPlane))
+			defer cancel()
+			defer func() {
+				if step != nil {
+					// From the time the pass ended: a pass that ran until the
+					// deadline retries right after the hold end.
+					step.requeue = controlPlaneHoldRequeue(controlPlane, time.Now(), step.requeue)
+				}
+			}()
+		}
+	}
+	sdk, _, wait, err := controlPlaneCoderClientFor(ctx, r.Client, ref.Namespace, ref.Name, !controlPlane.DeletionTimestamp.IsZero())
 	switch {
 	case err != nil && deleting:
 		// One attempt only, also when the access setup fails.

internal/controller/codermcpserver_delete_internal_test.go

@@ -0,0 +1,67 @@
+package controller
+
+import (
+	"context"
+	"net/http"
+	"net/http/httptest"
+	"testing"
+	"time"
+
+	"github.com/google/uuid"
+	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	clientgoscheme "k8s.io/client-go/kubernetes/scheme"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1"
+)
+
+// #384 review: a delete pass under the control plane hold ends at the hold
+// end, also when Coder does not answer, so the bounded pass after it fits in
+// the grace period. Here the hold ends in 5 s and Coder hangs on the DELETE,
+// whose own request timeout is 30 s.
+func TestMCPServerDeleteUnderHoldEndsAtHoldEnd(t *testing.T) {
+	t.Parallel()
+	coder := httptest.NewServer(http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
+		<-r.Context().Done()
+	}))
+	t.Cleanup(coder.Close)
+
+	scheme := runtime.NewScheme()
+	require.NoError(t, clientgoscheme.AddToScheme(scheme))
+	require.NoError(t, coderv1alpha1.AddToScheme(scheme))
+	deleted := metav1.NewTime(time.Now().Add(5*time.Second - controlPlaneDependentHold))
+	controlPlane := &coderv1alpha1.CoderControlPlane{ObjectMeta: metav1.ObjectMeta{
+		Name: "coder", Namespace: "ns", DeletionTimestamp: &deleted, Finalizers: []string{workspaceRBACFinalizer},
+	}}
+	controlPlane.Status.URL = coder.URL
+	controlPlane.Status.OperatorAccessReady = true
+	controlPlane.Status.OperatorTokenSecretRef = &coderv1alpha1.SecretKeySelector{Name: "coder-operator-token"}
+	token := &corev1.Secret{
+		ObjectMeta: metav1.ObjectMeta{Name: "coder-operator-token", Namespace: "ns"},
+		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}
+	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.Spec.ControlPlaneRef.Name = "coder"
+	server.Spec.Slug = "github"
+	server.Status.OrganizationID = uuid.NewString()
+	server.Status.ConfigID = uuid.NewString()
+	start := time.Now()
+	release, step, err := r.deleteStep(context.Background(), server)
+	require.NoError(t, err)
+	require.Less(t, time.Since(start), 15*time.Second, "the pass ends at the hold end, not at the 30 s request timeout")
+	require.False(t, release, "a pass under the hold never releases without a delete")
+	require.NotNil(t, step)
+	require.Equal(t, "CoderUnavailable", step.reason)
+	// The requeue counts from the end of the pass: the time left to the hold
+	// end, which is about 0 here, plus 1 s (controlPlaneHoldRequeue).
+	require.LessOrEqual(t, step.requeue, 2*time.Second, "the next pass runs right after the hold end")
+}

internal/controller/codermcpserver_delete_test.go

@@ -218,6 +218,44 @@ func TestCoderMCPServerDeleteWhileControlPlaneDeletes(t *testing.T) {
 	}
 }
 
+// #382 (#353): a control plane that is being deleted waits for a
+// CoderMCPServer that is being deleted, so Coder keeps running. Under that
+// hold the object takes the normal delete path: a failed DELETE keeps the
+// finalizer and retries, instead of one bounded attempt and a release. Then
+// the control plane goes.
+func TestCoderMCPServerDeleteUnderControlPlaneHold(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	env := newMCPServerEnv(ctx, t, true)
+	env.keySecret(ctx, t, testMCPKey)
+	got, _ := env.applied(ctx, t, nil)
+	deleteFinalizedControlPlane(ctx, t, env.namespace, "coder")
+	require.NoError(t, k8sClient.Delete(ctx, got))
+
+	controlPlaneGone := func() bool {
+		t.Helper()
+		r := &controller.CoderControlPlaneReconciler{Client: k8sClient, Scheme: scheme}
+		_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: client.ObjectKey{Namespace: env.namespace, Name: "coder"}})
+		require.NoError(t, err)
+		err = k8sClient.Get(ctx, client.ObjectKey{Namespace: env.namespace, Name: "coder"}, &coderv1alpha1.CoderControlPlane{})
+		return apierrors.IsNotFound(err)
+	}
+	require.False(t, controlPlaneGone(), "the control plane waits for the MCP server")
+
+	env.fake.failNext(routeDeleteMCPServer, fakeFault{Status: http.StatusBadGateway})
+	kept, result := env.reconcileDeleting(ctx, t, got)
+	require.NotNil(t, kept, "a failed delete keeps the finalizer while the control plane waits")
+	requireMCPServerStep(t, kept, false, "CoderUnavailable")
+	require.Equal(t, 60*time.Second, result.RequeueAfter)
+	require.False(t, controlPlaneGone(), "the control plane still waits")
+
+	gone, _ := env.reconcileDeleting(ctx, t, kept)
+	require.Nil(t, gone)
+	require.Empty(t, env.fake.mcpServers(), "the retry deleted the config")
+	require.Equal(t, 2, env.fake.requestCount(routeDeleteMCPServer))
+	require.True(t, controlPlaneGone(), "the control plane goes after the MCP server")
+}
+
 // From the #329 final assessment: a released object's entry in the
 // reconciler's credential hash map goes, so the map does not grow without
 // bound.

internal/controller/controlplane_hold.go

@@ -22,9 +22,10 @@ import (
 )
 
 // controlPlaneDependentHold is how long a CoderControlPlane that is being
-// deleted waits for the CoderAIGatewayKey and CoderAIProvider objects that
-// use it (#353). Its Coder Deployment keeps running while it waits, so they
-// can delete their keys and providers in Coder. It covers the 5-minute
+// deleted waits for the CoderAIGatewayKey, CoderAIProvider and CoderMCPServer
+// objects that use it (#353, #382). Its Coder Deployment keeps running while
+// it waits, so they can delete their keys, providers and MCP server configs
+// in Coder. It covers the 5-minute
 // settle window of a key deletion plus retries. After it, the control plane
 // goes, and they release with ControlPlaneGone.
 const controlPlaneDependentHold = 10 * time.Minute
@@ -93,7 +94,7 @@ func controlPlaneHoldEnd(controlPlane *coderv1alpha1.CoderControlPlane) time.Tim
 // controlPlaneDependentGraceFor is the grace period after the hold when
 // perKind dependents of one kind remain (controlPlaneDependentGrace).
 func controlPlaneDependentGraceFor(perKind int) time.Duration {
-	return min(controlPlaneDependentGrace+time.Duration(perKind)*max(aiGatewayKeyBoundedDeleteTimeout, aiProviderBoundedDeleteTimeout), controlPlaneDependentGraceMax)
+	return min(controlPlaneDependentGrace+time.Duration(perKind)*max(aiGatewayKeyBoundedDeleteTimeout, aiProviderBoundedDeleteTimeout, mcpServerBoundedDeleteTimeout), controlPlaneDependentGraceMax)
 }
 
 // controlPlaneHoldRequeue caps a dependent's requeue under the hold, so it
@@ -126,8 +127,8 @@ func controlPlaneHolds(ctx context.Context, reader client.Reader, controlPlane *
 	return namespace.DeletionTimestamp.IsZero() && namespace.Status.Phase != corev1.NamespaceTerminating, nil
 }
 
-// controlPlaneDependents returns the CoderAIGatewayKey and CoderAIProvider
-// objects in controlPlane's namespace that reference it and hold it at now,
+// controlPlaneDependents returns the CoderAIGatewayKey, CoderAIProvider and
+// CoderMCPServer objects in controlPlane's namespace that reference it and hold it at now,
 // as kind/name, and the larger count of one kind. A dependent holds it while
 // it is being deleted and still has its cleanup finalizer (another finalizer
 // does not hold it), and a live one only for controlPlaneLiveDependentWait
@@ -163,7 +164,17 @@ func controlPlaneDependents(ctx context.Context, reader client.Reader, controlPl
 			dependents = append(dependents, "CoderAIProvider/"+p.Name)
 		}
 	}
-	return dependents, max(keyCount, len(dependents)-keyCount), nil
+	providerCount := len(dependents) - keyCount
+	servers := &coderv1alpha1.CoderMCPServerList{}
+	if err := reader.List(ctx, servers, client.InNamespace(controlPlane.Namespace)); err != nil {
+		return nil, 0, fmt.Errorf("list MCP servers of namespace %s: %w", controlPlane.Namespace, err)
+	}
+	for i := range servers.Items {
+		if m := &servers.Items[i]; m.Spec.ControlPlaneRef.Name == controlPlane.Name && holds(m, coderv1alpha1.CoderMCPServerCleanupFinalizer) {
+			dependents = append(dependents, "CoderMCPServer/"+m.Name)
+		}
+	}
+	return dependents, max(keyCount, providerCount, len(dependents)-keyCount-providerCount), nil
 }
 
 // dependentRemoved passes only the removal of a dependent object, the event
@@ -175,15 +186,17 @@ var dependentRemoved = predicate.Funcs{
 	GenericFunc: func(event.GenericEvent) bool { return false },
 }
 
-// requestForDependentControlPlane maps a CoderAIGatewayKey or CoderAIProvider
-// to the control plane that it references.
+// requestForDependentControlPlane maps a CoderAIGatewayKey, CoderAIProvider or
+// CoderMCPServer to the control plane that it references.
 func requestForDependentControlPlane(_ context.Context, obj client.Object) []reconcile.Request {
 	var name string
 	switch o := obj.(type) {
 	case *coderv1alpha1.CoderAIGatewayKey:
 		name = o.Spec.ControlPlaneRef.Name
 	case *coderv1alpha1.CoderAIProvider:
 		name = o.Spec.ControlPlaneRef.Name
+	case *coderv1alpha1.CoderMCPServer:
+		name = o.Spec.ControlPlaneRef.Name
 	}
 	if name == "" {
 		return nil

internal/controller/controlplane_hold_internal_test.go

@@ -1,10 +1,17 @@
 package controller
 
 import (
+	"context"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	coderv1alpha1 "github.com/coder/coder-k8s/api/v1alpha1"
 )
 
 // #353: the grace period after the hold grows by one bounded pass per
@@ -16,3 +23,20 @@ func TestControlPlaneDependentGraceFor(t *testing.T) {
 	require.Equal(t, time.Minute, controlPlaneDependentGraceFor(3))
 	require.Equal(t, 5*time.Minute, controlPlaneDependentGraceFor(1000), "the grace period has a cap")
 }
+
+// #353, #382: the removal of each dependent kind requeues the control plane
+// that it references, so a waiting control plane goes at once.
+func TestRequestForDependentControlPlane(t *testing.T) {
+	t.Parallel()
+	meta := metav1.ObjectMeta{Name: "dependent", Namespace: "coder"}
+	ref := coderv1alpha1.CoderControlPlaneReference{Name: "coder"}
+	want := []reconcile.Request{{NamespacedName: types.NamespacedName{Namespace: "coder", Name: "coder"}}}
+	for _, obj := range []client.Object{
+		&coderv1alpha1.CoderAIGatewayKey{ObjectMeta: meta, Spec: coderv1alpha1.CoderAIGatewayKeySpec{ControlPlaneRef: ref}},
+		&coderv1alpha1.CoderAIProvider{ObjectMeta: meta, Spec: coderv1alpha1.CoderAIProviderSpec{ControlPlaneRef: ref}},
+		&coderv1alpha1.CoderMCPServer{ObjectMeta: meta, Spec: coderv1alpha1.CoderMCPServerSpec{ControlPlaneRef: ref}},
+	} {
+		require.Equal(t, want, requestForDependentControlPlane(context.Background(), obj), "%T", obj)
+	}
+	require.Nil(t, requestForDependentControlPlane(context.Background(), &coderv1alpha1.CoderTemplateTest{ObjectMeta: meta}), "not a dependent kind")
+}