From d5311219f77c811e618869515f1b0c186eb07f7d Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 13:49:39 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20tests:=20cover=20the=20CoderMCPS?= =?UTF-8?q?erver=20disable=20window=20between=20the=20marker=20write=20and?= =?UTF-8?q?=20the=20PATCH?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two regression tests for the opt-in disable marker that #360 writes before the disable PATCH: - The reconcile writes the marker and stops before the PATCH. While the opt-in stays withdrawn, the next reconcile still sends the enabled: false PATCH, because the decision comes from Coder's answer, not from appliedGeneration. - The PATCH fails without a commit, and the opt-in comes back while the config is still enabled. The generation counts as applied, but the drift sync still sends the spec edit made before the disable. The known limit of that window (review comment 4271304269 on #360) is documented next to sendMCPServerDisable. Fixes #365 Refs #155 Change-Id: I3f259f54aef05ffaf55024edc563504fbc473e0a Signed-off-by: Thomas Kosiewski --- internal/controller/codermcpserver_disable.go | 15 +++- .../controller/codermcpserver_optin_test.go | 82 +++++++++++++++++++ 2 files changed, 96 insertions(+), 1 deletion(-) diff --git a/internal/controller/codermcpserver_disable.go b/internal/controller/codermcpserver_disable.go index 521e9930..4e9e5573 100644 --- a/internal/controller/codermcpserver_disable.go +++ b/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, diff --git a/internal/controller/codermcpserver_optin_test.go b/internal/controller/codermcpserver_optin_test.go index 1ca2172b..61398716 100644 --- a/internal/controller/codermcpserver_optin_test.go +++ b/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) +}