🤖 fix: finish the control plane hold and cleanup record follow-ups of #353

docs/how-to/manage-ai-gateway-keys.md

@@ -139,7 +139,7 @@ The finalizer stays while Coder cannot confirm the deletion:
 
 When the control plane is gone, nothing can delete the keys. The finalizer goes at once, and the [cleanup record](#cleanup-records) stays. The operator logs `Releasing the finalizer without deleting the keys in Coder` with reason `ControlPlaneGone` and the key IDs: delete those keys with `coder ai-gateway keys delete`.
 
-When you delete the control plane while `CoderAIGatewayKey` or `CoderAIProvider` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes: it keeps its finalizer, so Coder keeps running, and the objects delete their keys and providers as usual, with retries. The operator log shows `Waiting for the objects that use this control plane` with their names. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane (or within 30 seconds). After 10 minutes, the objects that remain get at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for their bounded pass below, and then the control plane goes anyway. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait, because Kubernetes then deletes the Coder Deployment first.
+When you delete the control plane while `CoderAIGatewayKey`, `CoderAIProvider` or `CoderMCPServer` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes: it keeps its finalizer, so Coder keeps running, and the objects delete their keys, providers and MCP server configs as usual, with retries. The operator log shows `Waiting for the objects that use this control plane` with their names. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. An object that you do not delete holds the control plane for only 30 seconds, so delete the objects together with the control plane (or within 30 seconds). After 10 minutes, the objects that remain get at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for their bounded pass below, and then the control plane goes anyway. A foreground deletion (`kubectl delete --cascade=foreground`) does not wait, because Kubernetes then deletes the Coder Deployment first.
 
 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 runs one pass of at most 10 seconds without the settle window, then removes the finalizer whatever the answer. A key create that was still in flight at that moment can commit later, and its key stays in Coder. When the pass fails, the log lists the IDs that it did not delete.
 

docs/how-to/manage-ai-providers.md

@@ -124,7 +124,7 @@ Deleting the object deletes the provider in Coder. With only an unconfirmed crea
 
 - When Coder is unreachable, or the control plane has no operator access, the finalizer `coder.com/ai-provider-cleanup` stays and the operator retries every 60 seconds. See [Stuck finalizer](#stuck-finalizer).
 - When the control plane is gone, the operator cannot delete the provider now. The finalizer goes at once, and the operator logs the provider ID. The [cleanup record](#cleanup-records) stays, so a later sweep can delete the provider.
-- When you delete the control plane while `CoderAIProvider` or `CoderAIGatewayKey` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes, then for at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. 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 you delete the control plane while `CoderAIProvider`, `CoderAIGatewayKey` or `CoderMCPServer` objects that use it are being deleted, the control plane delete can take up to about 15 minutes. The control plane waits for the objects for up to 10 minutes, then for at most 5 more minutes (30 seconds plus 10 seconds per object of one kind) for a last bounded attempt. It keeps its finalizer, so Coder keeps running, and the delete above runs with its retries. The operator removes the workspace RBAC of the control plane before it waits, so Coder loses its Kubernetes access at once. 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. Without working access, the finalizer goes without a delete. Either way a namespace deletion finishes. When the delete did not succeed, the operator logs the provider ID: delete the provider in the Coder dashboard. The metric `coder_k8s_cleanup_released_without_delete_total` (label `kind`) counts these releases.
 
 Delete `CoderAIProvider` objects before their namespace: a namespace deletion removes Coder's pods at once, so the control plane cannot wait for them. A sweep deletes the leftover provider only when the namespace comes back with a control plane of the same Coder deployment. Coder deletes a provider even while chat models use it. Those models stop working.

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

@@ -208,7 +208,7 @@ Deleting the object deletes the config in Coder. Coder removes the server from e
 - 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 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.
+- 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.
 - A withdrawn opt-in never blocks a delete.
 
 Delete `CoderMCPServer` objects before their control plane and before their namespace.

internal/controller/coderaigatewaykey_record.go

@@ -74,12 +74,14 @@ func (r *CoderAIGatewayKeyReconciler) recordIssuedKeyID(ctx context.Context, key
 	}
 }
 
-// recordAIGatewayKeyID writes the ID of a current key that Coder lists into
-// the record. It never stops the reconcile: the record already holds the
-// prefix, which finds the key too, so a failure is logged and retried.
+// recordAIGatewayKeyID writes the ID of a current key that Coder lists, and a
+// create time that the observe step stamped later (#353), into the record. It
+// never stops the reconcile: the record already holds the prefix, which
+// finds the key too, so a failure is logged and retried.
 func (r *CoderAIGatewayKeyReconciler) recordAIGatewayKeyID(ctx context.Context, sdk *codersdk.Client, key *coderv1alpha1.CoderAIGatewayKey, prefix string) {
 	record, _, err := readCleanupRecord(ctx, r.APIReader, r.OperatorNamespace, key.UID)
-	if err == nil && record != nil && record.CoderID == key.Status.CurrentKey.ID {
+	if err == nil && record != nil && record.CoderID == key.Status.CurrentKey.ID &&
+		(key.Status.LastCreateAttemptTime == nil || !record.CreateAttemptTime.Before(key.Status.LastCreateAttemptTime.Time)) {
 		return
 	}
 	if err == nil {

internal/controller/coderaigatewaykey_record_test.go

@@ -227,3 +227,21 @@ func TestCoderAIGatewayKeyRecordBackfill(t *testing.T) {
 	requireAIGatewayKeyReady(t, got)
 	require.Equal(t, want, env.aiGatewayKeyRecord(ctx, t, got.UID))
 }
+
+// #353 (#362 follow-up 9): when someone clears status.lastCreateAttemptTime
+// by hand, the controller counts the settle window from now (fail closed),
+// and the record gets that time too, so a sweep does not count from the
+// older one.
+func TestCoderAIGatewayKeyRecordAfterClearedCreateTime(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	env := newAIGatewayKeyEnv(ctx, t, ptrTo(true))
+	got, _ := env.reconcile(ctx, t, env.createKey(ctx, t, "coder", testAIGatewaySecret))
+	requireAIGatewayKeyReady(t, got)
+	env.clock.Step(time.Hour)
+	got.Status.LastCreateAttemptTime = nil
+	require.NoError(t, k8sClient.Status().Update(ctx, got))
+	got, _ = env.reconcile(ctx, t, got)
+	require.Equal(t, env.clock.Now().UTC(), got.Status.LastCreateAttemptTime.UTC())
+	require.Equal(t, env.clock.Now().UTC().Format(time.RFC3339), env.aiGatewayKeyRecord(ctx, t, got.UID)["createAttemptTime"])
+}

internal/controller/coderaiprovider_delete.go

@@ -83,16 +83,23 @@ func (r *CoderAIProviderReconciler) deleteStep(ctx context.Context, provider *co
 		// While the control plane holds for this object, Coder keeps running:
 		// use the normal path, which retries, instead of the bounded pass
 		// (#353, item q).
-		now := time.Now()
-		holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, now)
+		holds, err := controlPlaneHolds(ctx, r.Client, controlPlane, time.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, so the bounded pass after it still fits in the
+			// control plane's grace period (#384 follow-up 4).
+			var cancel context.CancelFunc
+			ctx, cancel = context.WithDeadline(ctx, controlPlaneHoldEnd(controlPlane))
+			defer cancel()
 			defer func() {
 				if step != nil {
-					step.requeue = controlPlaneHoldRequeue(controlPlane, now, step.requeue)
+					// From the end of the pass: a pass that ran until the
+					// deadline retries right after the hold end.
+					step.requeue = controlPlaneHoldRequeue(controlPlane, time.Now(), step.requeue)
 				}
 			}()
 		}

internal/controller/coderaiprovider_delete_internal_test.go

@@ -0,0 +1,69 @@
+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 follow-up 4 for providers: a delete pass under the control plane hold
+// ends at the hold end, also when Coder does not answer, and its retry counts
+// from the end of the pass. Here the hold ends in 5 s and Coder hangs on the
+// DELETE, whose own request timeout is 30 s.
+func TestAIProviderDeleteUnderHoldEndsAtHoldEnd(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", UID: "cp-uid", 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",
+			OwnerReferences: []metav1.OwnerReference{*metav1.NewControllerRef(controlPlane, coderv1alpha1.GroupVersion.WithKind("CoderControlPlane"))},
+		},
+		Data: map[string][]byte{coderv1alpha1.DefaultTokenSecretKey: []byte("token")},
+	}
+	namespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: "ns"}}
+	c := fake.NewClientBuilder().WithScheme(scheme).WithObjects(controlPlane, token, namespace).Build()
+	r := &CoderAIProviderReconciler{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")
+
+	provider := &coderv1alpha1.CoderAIProvider{ObjectMeta: metav1.ObjectMeta{Name: "openai", Namespace: "ns", UID: "provider-uid"}}
+	provider.Spec.ControlPlaneRef.Name = "coder"
+	provider.Status.ProviderID = uuid.NewString()
+	start := time.Now()
+	release, step, err := r.deleteStep(context.Background(), provider)
+	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/coderaiprovider_record.go

@@ -93,7 +93,10 @@ func (r *CoderAIProviderReconciler) ensureAIProviderRecord(
 	if err != nil {
 		return nil, err
 	}
-	if record != nil && record.CoderID == provider.Status.ProviderID {
+	// A record without an ID names the deployment of a pending create, so
+	// the write checks the deployment ID before the create is confirmed in
+	// another deployment (#353).
+	if record != nil && record.CoderID != "" && record.CoderID == provider.Status.ProviderID {
 		return nil, nil
 	}
 	return r.writeAIProviderRecord(ctx, sdk, provider, nil)

internal/controller/coderaiprovider_record_test.go

@@ -288,3 +288,35 @@ func TestCoderAIProviderDeploymentSwitchKeepsRecord(t *testing.T) {
 	require.Zero(t, env.fake.requestCount(routeDeleteAIProvider), "no delete in the new deployment")
 	require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID), "the record stays for a sweep")
 }
+
+// #353 (#362 follow-up 7): a create whose answer was lost leaves a record
+// without a provider ID. When the control plane then reaches another Coder
+// deployment, where someone else's provider has the same name, that name
+// 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 TestCoderAIProviderPendingCreateAfterDeploymentSwitch(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	env, _, provider := newAIProviderCreateEnv(ctx, t)
+	env.fake.failNext(routeCreateAIProvider, fakeFault{AfterCommit: true, Reset: true})
+	got, _ := env.reconcile(ctx, t, provider)
+	requireAIProviderStep(t, got, false, "ConfirmingCreate")
+	before := cleanupRecord(ctx, t, env.namespace, provider.UID)
+	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 _, p := range env.fake.aiProviders {
+		p.displayName = "Someone else's provider"
+	}
+	env.fake.mu.Unlock()
+	got, _ = env.reconcile(ctx, t, got)
+	require.Contains(t, requireAIProviderStep(t, got, true, "CoderDeploymentChanged"), before["deploymentID"])
+	require.NotNil(t, got.Status.CreateAttemptTime, "the marker stays")
+	require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID))
+
+	gone, _ := env.deleteAIProvider(ctx, t, got)
+	require.Nil(t, gone, "the finalizer goes without a delete")
+	require.Zero(t, env.fake.requestCount(routeDeleteAIProvider), "no delete in the new deployment")
+	require.Equal(t, before, cleanupRecord(ctx, t, env.namespace, provider.UID), "the record stays for a sweep")
+}

internal/controller/codermcpserver_controller.go

@@ -175,13 +175,18 @@ func (r *CoderMCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		if !controllerutil.ContainsFinalizer(server, coderv1alpha1.CoderMCPServerCleanupFinalizer) {
 			return ctrl.Result{}, nil
 		}
+		forgetReleasedWithoutDelete(server.UID)
 		release, step, err := r.deleteStep(ctx, server)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
 		if release {
 			r.sentCredentials.Delete(req.NamespacedName)
-			return ctrl.Result{}, r.patchFinalizer(ctx, server, false)
+			if err := r.patchFinalizer(ctx, server, false); err != nil {
+				return ctrl.Result{}, err
+			}
+			releasedWithoutDeleteDone(server.UID)
+			return ctrl.Result{}, nil
 		}
 		applyMCPServerStep(server, step)
 		return ctrl.Result{RequeueAfter: step.requeue}, r.writeStatus(ctx, server, before)

internal/controller/codermcpserver_delete.go

@@ -165,6 +165,7 @@ func deleteMCPServerInCoder(ctx context.Context, sdk *codersdk.Client, server *c
 // releaseWithoutDelete logs what can stay in Coder, because nothing can
 // delete it any more, and returns true (ControlPlaneGone).
 func (r *CoderMCPServerReconciler) releaseWithoutDelete(ctx context.Context, server *coderv1alpha1.CoderMCPServer, why string) bool {
+	countReleasedWithoutDelete(server.UID, "CoderMCPServer")
 	log.FromContext(ctx).Info("Releasing the finalizer without a delete in Coder: the MCP server config can stay in the Coder database",
 		"reason", "ControlPlaneGone", "why", why, "slug", server.Spec.Slug, "configID", server.Status.ConfigID,
 		"createPending", server.Status.CreateAttemptTime != nil)