🤖 tests: cover the CoderMCPServer disable window between the marker write and the PATCH

internal/controller/codermcpserver_disable.go

@@ -118,7 +118,20 @@ func readMCPServerToDisable(
 // before the PATCH: Coder can commit the PATCH while its answer or the next
 // status write is lost, and a pending generation must not enable the config
 // once consent or the opt-in comes back. A failed status write sends no
-// PATCH.
+// PATCH. Whether the config is disabled comes from Coder's answer, not from
+// appliedGeneration, so a reconcile after a stop between the status write
+// and the PATCH still sends the PATCH while consent or the opt-in stays
+// withdrawn.
+//
+// Known limit (#365): if the process stops between the status write and the
+// PATCH, and consent or the opt-in comes back before the next reconcile, the
+// sync applies the current spec without a later spec edit. Coder never
+// disabled the config in that window, and the sync checks every credential
+// first, so no credential goes to an origin without consent. The same holds
+// when the PATCH fails without a commit and consent or the opt-in comes back
+// while the config is still enabled: the generation counts as applied, but
+// the sync compares Coder with the spec on every reconcile, so it still
+// sends a spec edit made before the disable.
 func (r *CoderMCPServerReconciler) sendMCPServerDisable(
 	ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, before *coderv1alpha1.CoderMCPServerStatus,
 	orgID, id uuid.UUID, reason string, unchecked func(format string, args ...any) *mcpServerStep,

internal/controller/codermcpserver_optin_test.go

@@ -367,3 +367,85 @@ func TestCoderMCPServerOptInGoneConfigHolds(t *testing.T) {
 	requireMCPServerApplied(t, got)
 	require.Equal(t, 2, env.posts())
 }
+
+// #365: the process stops after the status write that records the disable
+// and before the disable PATCH. While the opt-in stays withdrawn, the next
+// reconcile still sends the PATCH: whether the config is disabled comes from
+// Coder's answer, not from appliedGeneration.
+func TestCoderMCPServerOptInDisableAfterStopBeforePatch(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	env := newMCPServerEnv(ctx, t, true)
+	env.keySecret(ctx, t, testMCPKey)
+	got, _ := env.applied(ctx, t, nil)
+	require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got))
+	got.Spec.Description = "pending"
+	require.NoError(t, k8sClient.Update(ctx, got))
+	env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway})
+	got, _ = env.reconcile(ctx, t, got)
+	requireMCPServerStep(t, got, false, "CoderUnavailable")
+	env.setOptIn(ctx, t, nil)
+
+	// The status write succeeds, then the process stops: the context ends
+	// before the PATCH can go out.
+	stopCtx, stop := context.WithCancel(ctx)
+	base, err := client.NewWithWatch(cfg, client.Options{Scheme: scheme})
+	require.NoError(t, err)
+	r := &controller.CoderMCPServerReconciler{Client: interceptor.NewClient(base, interceptor.Funcs{
+		SubResourceUpdate: func(ctx context.Context, c client.Client, sub string, obj client.Object, opts ...client.SubResourceUpdateOption) error {
+			err := c.SubResource(sub).Update(ctx, obj, opts...)
+			stop()
+			return err
+		},
+	}), Scheme: scheme}
+	mark := len(env.writeRoutes())
+	_, _ = r.Reconcile(stopCtx, ctrl.Request{NamespacedName: client.ObjectKeyFromObject(got)})
+	require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got))
+	require.Equal(t, got.Generation, got.Status.AppliedGeneration, "the marker is written")
+	require.Empty(t, env.writesSince(mark), "the PATCH never went out")
+	require.True(t, env.stored(t, got).view.Enabled)
+
+	got, _ = env.reconcile(ctx, t, got)
+	require.Contains(t, requireMCPServerStep(t, got, true, "MCPServersNotAllowed"), "The controller disabled MCP server config")
+	writes := env.writesSince(mark)
+	require.Len(t, writes, 1)
+	require.JSONEq(t, `{"enabled":false}`, string(writes[0].body))
+	require.False(t, env.stored(t, got).view.Enabled)
+}
+
+// #365, second case: the controller writes the disable marker, the PATCH
+// fails without a commit, and the opt-in comes back while the config is
+// still enabled. The pending generation then counts as applied, but the
+// sync compares Coder with the spec on every reconcile, so it sends the
+// pending spec edit as drift: the edit is not lost.
+func TestCoderMCPServerOptInFailedDisableKeepsPendingEdit(t *testing.T) {
+	t.Parallel()
+	ctx := context.Background()
+	env := newMCPServerEnv(ctx, t, true)
+	env.keySecret(ctx, t, testMCPKey)
+	got, _ := env.applied(ctx, t, nil)
+	require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got))
+	got.Spec.Description = "pending"
+	require.NoError(t, k8sClient.Update(ctx, got))
+	env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway})
+	got, _ = env.reconcile(ctx, t, got)
+	requireMCPServerStep(t, got, false, "CoderUnavailable")
+
+	env.setOptIn(ctx, t, nil)
+	env.fake.failNext(routeUpdateMCPServer, fakeFault{Status: http.StatusBadGateway})
+	got, _ = env.reconcile(ctx, t, got)
+	require.Contains(t, requireMCPServerStep(t, got, false, "MCPServersNotAllowed"), "the disable failed")
+	require.Equal(t, got.Generation, got.Status.AppliedGeneration, "the marker is written")
+	stored := env.stored(t, got)
+	require.True(t, stored.view.Enabled, "the PATCH did not commit")
+	require.Empty(t, stored.view.Description)
+
+	env.setOptIn(ctx, t, &coderv1alpha1.MCPServersSpec{Organizations: []string{"coder"}})
+	mark := len(env.writeRoutes())
+	got, _ = env.reconcile(ctx, t, got)
+	requireMCPServerApplied(t, got)
+	writes := env.writesSince(mark)
+	require.Len(t, writes, 1)
+	require.JSONEq(t, `{"description":"pending"}`, string(writes[0].body), "the drift sync sends the pending edit")
+	require.Equal(t, "pending", env.stored(t, got).view.Description)
+}