Patch MCPServer spec instead of Update

Fixes #4767.

The controller writes finalizers, finalizer removal, and the
restart-processed annotation via r.Update. Update is a full PUT, so
any spec field the operator does not track — most importantly
spec.authzConfig, which a separate authorization controller will soon
own — is zeroed on every reconcile.

Replace the three Update call sites with an optimistic-lock merge
patch. The merge-patch body carries only fields the caller changed,
so untouched fields never hit the wire and cannot be clobbered.
MergeFromWithOptimisticLock sends resourceVersion as a precondition,
giving 409-on-collision semantics for concurrent writers and
defending metadata.finalizers (which has no array-merge semantics
under merge-patch) against wholesale replacement when another
controller is mid-flight adding its own entry.

Tests:

- Envtest suite writes spec.authzConfig out-of-band and asserts it
  survives both the finalizer-add reconcile and the
  restart-annotation reconcile.
- Unit suite uses a patch-recording client to assert each migrated
  call site emits a body carrying the resourceVersion precondition
  — a deterministic wire-level signal that
  MergeFromWithOptimisticLock is in effect. A regression to plain
  MergeFrom would drop the precondition and fail the assertion
  independent of the higher-level survival test.

Also:

- .claude/rules/operator.md: new "Spec / metadata patching" section
  documenting the pattern for future CR writes. Status patching is
  a separate follow-up (#4633).
- Rename the mock-client flag failOnMCPServerUpdate →
  failOnMCPServerWrite; it now intercepts both Update and Patch on
  MCPServer, so the name matches reality.

Author: jhrozek

.claude/rules/operator.md

@@ -67,3 +67,43 @@ task operator-test            # Run unit tests
 task operator-e2e-test        # Run e2e tests
 task crdref-gen              # Generate CRD API docs (run from cmd/thv-operator/)
 ```
+
+## Spec / metadata patching
+
+Never use `r.Update` on a CR spec or metadata: `Update` is a full PUT,
+so any field our local copy does not track (e.g. `spec.authzConfig`
+written by a separate authorization controller) gets zeroed on every
+reconcile.
+
+Use an **optimistic-lock merge patch** instead. The merge-patch body
+only contains fields the caller changed, and `MergeFromWithOptimisticLock`
+sends `resourceVersion` as a precondition: if the server moved between
+our Get and Patch, the apiserver returns 409 and controller-runtime
+requeues with a fresh Get.
+
+This is what protects `metadata.finalizers`. Merge-patch has no
+array-append semantics — arrays are replaced wholesale — so when our
+diff includes `finalizers` (e.g. an `AddFinalizer` call) it must have
+been computed from an up-to-date snapshot. The 409 + requeue is what
+guarantees that: any concurrent finalizer added by another controller
+fails our precondition, and the next reconcile observes it via a fresh
+Get before recomputing the diff.
+
+```go
+original := mcpServer.DeepCopy()
+controllerutil.AddFinalizer(mcpServer, MCPServerFinalizerName)
+if err := r.Patch(ctx, mcpServer,
+    client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})); err != nil {
+    return ctrl.Result{}, err
+}
+```
+
+Do not put unrelated work between the `DeepCopy` and the `Patch`: the
+wire diff is computed from that snapshot, so anything you mutate in
+between leaks into the patch body.
+
+Expect 409s as routine log noise once the external controller lands —
+the guard doing its job, not a bug.
+
+Status-subresource patching follows the same "never `Update`" rule and
+is covered separately once the shared helper lands (#4633).

cmd/thv-operator/controllers/mcpexternalauthconfig_controller.go

@@ -177,14 +177,20 @@ func (r *MCPExternalAuthConfigReconciler) handleConfigHashChange(
 		logger.Info("Triggering reconciliation of MCPServer due to MCPExternalAuthConfig change",
 			"mcpserver", server.Name, "externalAuthConfig", externalAuthConfig.Name)
 
+		// Use an optimistic-lock merge patch rather than Update so we do not
+		// silently overwrite spec fields (e.g. spec.authzConfig) that are owned
+		// by another controller. The resourceVersion is sent, so concurrent
+		// writers cause a 409 Conflict and a clean requeue.
+		original := server.DeepCopy()
 		// Add an annotation to the MCPServer to trigger reconciliation
 		if server.Annotations == nil {
 			server.Annotations = make(map[string]string)
 		}
 		server.Annotations["toolhive.stacklok.dev/externalauthconfig-hash"] = configHash
 
-		if err := r.Update(ctx, &server); err != nil {
-			logger.Error(err, "Failed to update MCPServer annotation", "mcpserver", server.Name)
+		if err := r.Patch(ctx, &server,
+			client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})); err != nil {
+			logger.Error(err, "Failed to patch MCPServer annotation", "mcpserver", server.Name)
 			// Continue with other servers even if one fails
 		}
 	}

cmd/thv-operator/controllers/mcpserver_controller.go

@@ -105,6 +105,9 @@ var remoteProxyRBACRules = []rbacv1.PolicyRule{
 // mcpContainerName is the name of the mcp container used in pod templates
 const mcpContainerName = "mcp"
 
+// MCPServerFinalizerName is the name of the finalizer for MCPServer
+const MCPServerFinalizerName = "mcpserver.toolhive.stacklok.dev/finalizer"
+
 // Restart annotation keys for triggering pod restart
 const (
 	RestartedAtAnnotationKey          = "mcpserver.toolhive.stacklok.dev/restarted-at"
@@ -182,25 +185,35 @@ func (r *MCPServerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 	// Check if the MCPServer instance is marked to be deleted — do this before
 	// any validation or external API calls to avoid unnecessary work during deletion
 	if mcpServer.GetDeletionTimestamp() != nil {
-		if controllerutil.ContainsFinalizer(mcpServer, "mcpserver.toolhive.stacklok.dev/finalizer") {
+		if controllerutil.ContainsFinalizer(mcpServer, MCPServerFinalizerName) {
 			if err := r.finalizeMCPServer(ctx, mcpServer); err != nil {
 				return ctrl.Result{}, err
 			}
 
-			controllerutil.RemoveFinalizer(mcpServer, "mcpserver.toolhive.stacklok.dev/finalizer")
-			err := r.Update(ctx, mcpServer)
-			if err != nil {
+			// Use an optimistic-lock merge patch rather than Update so we do not
+			// silently overwrite spec fields (e.g. spec.authzConfig) that are owned
+			// by another controller. The resourceVersion is sent, so concurrent
+			// writers cause a 409 Conflict and a clean requeue.
+			original := mcpServer.DeepCopy()
+			controllerutil.RemoveFinalizer(mcpServer, MCPServerFinalizerName)
+			if err := r.Patch(ctx, mcpServer,
+				client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})); err != nil {
 				return ctrl.Result{}, err
 			}
 		}
 		return ctrl.Result{}, nil
 	}
 
 	// Add finalizer for this CR
-	if !controllerutil.ContainsFinalizer(mcpServer, "mcpserver.toolhive.stacklok.dev/finalizer") {
-		controllerutil.AddFinalizer(mcpServer, "mcpserver.toolhive.stacklok.dev/finalizer")
-		err = r.Update(ctx, mcpServer)
-		if err != nil {
+	if !controllerutil.ContainsFinalizer(mcpServer, MCPServerFinalizerName) {
+		// Use an optimistic-lock merge patch rather than Update so we do not
+		// silently overwrite spec fields (e.g. spec.authzConfig) that are owned
+		// by another controller. The resourceVersion is sent, so concurrent
+		// writers cause a 409 Conflict and a clean requeue.
+		original := mcpServer.DeepCopy()
+		controllerutil.AddFinalizer(mcpServer, MCPServerFinalizerName)
+		if err := r.Patch(ctx, mcpServer,
+			client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})); err != nil {
 			return ctrl.Result{}, err
 		}
 	}
@@ -745,13 +758,18 @@ func (r *MCPServerReconciler) handleRestartAnnotation(ctx context.Context, mcpSe
 		return false, fmt.Errorf("failed to perform restart: %w", err)
 	}
 
-	// Update the last processed restart timestamp in annotations
+	// Update the last processed restart timestamp in annotations.
+	// Use an optimistic-lock merge patch rather than Update so we do not
+	// silently overwrite spec fields (e.g. spec.authzConfig) that are owned
+	// by another controller. The resourceVersion is sent, so concurrent
+	// writers cause a 409 Conflict and a clean requeue.
+	original := mcpServer.DeepCopy()
 	if mcpServer.Annotations == nil {
 		mcpServer.Annotations = make(map[string]string)
 	}
 	mcpServer.Annotations[LastProcessedRestartAnnotationKey] = currentRestartedAt
-	err = r.Update(ctx, mcpServer)
-	if err != nil {
+	if err := r.Patch(ctx, mcpServer,
+		client.MergeFromWithOptions(original, client.MergeFromWithOptimisticLock{})); err != nil {
 		return false, fmt.Errorf("failed to update MCPServer with last processed restart annotation: %w", err)
 	}
 

cmd/thv-operator/controllers/mcpserver_restart_test.go

@@ -564,10 +564,10 @@ func TestHandleRestartAnnotation_ErrorPaths(t *testing.T) {
 		testCtx.createDeployment()
 		testCtx.setRestartAnnotation("2023-01-01T12:00:00Z", "rolling")
 
-		// Mock a client that fails only on MCPServer Update operations
+		// Mock a client that fails only on MCPServer write operations
 		mockClient := &mockFailingClient{
-			Client:                testCtx.client,
-			failOnMCPServerUpdate: true,
+			Client:               testCtx.client,
+			failOnMCPServerWrite: true,
 		}
 		testCtx.reconciler.Client = mockClient
 
@@ -698,14 +698,20 @@ func TestReconcile_HandleRestartAnnotation_ErrorPaths(t *testing.T) {
 	})
 }
 
-// mockFailingClient is a test helper that wraps a real client and can be configured to fail on specific operations
+// mockFailingClient is a test helper that wraps a real client and can be configured to fail on specific operations.
+//
+// failOnMCPServerWrite triggers a mock error on any write (Update or Patch)
+// whose target is a *mcpv1beta1.MCPServer. "Write" is used because the
+// #4767 migration replaced MCPServer spec Updates with optimistic-lock
+// Patches, so a single flag covers both code paths that can mutate the
+// resource.
 type mockFailingClient struct {
 	client.Client
-	failOnGet             bool
-	failOnList            bool
-	failOnUpdate          bool
-	failOnDelete          bool
-	failOnMCPServerUpdate bool
+	failOnGet            bool
+	failOnList           bool
+	failOnUpdate         bool
+	failOnDelete         bool
+	failOnMCPServerWrite bool
 }
 
 func (m *mockFailingClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error {
@@ -726,7 +732,7 @@ func (m *mockFailingClient) Update(ctx context.Context, obj client.Object, opts
 	if m.failOnUpdate {
 		return fmt.Errorf("mock error: Update operation failed")
 	}
-	if m.failOnMCPServerUpdate {
+	if m.failOnMCPServerWrite {
 		// Check if the object being updated is an MCPServer
 		if _, isMCPServer := obj.(*mcpv1beta1.MCPServer); isMCPServer {
 			return fmt.Errorf("mock error: MCPServer Update operation failed")
@@ -735,6 +741,17 @@ func (m *mockFailingClient) Update(ctx context.Context, obj client.Object, opts
 	return m.Client.Update(ctx, obj, opts...)
 }
 
+func (m *mockFailingClient) Patch(
+	ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption,
+) error {
+	if m.failOnMCPServerWrite {
+		if _, isMCPServer := obj.(*mcpv1beta1.MCPServer); isMCPServer {
+			return fmt.Errorf("mock error: MCPServer Patch operation failed")
+		}
+	}
+	return m.Client.Patch(ctx, obj, patch, opts...)
+}
+
 func (m *mockFailingClient) Delete(ctx context.Context, obj client.Object, opts ...client.DeleteOption) error {
 	if m.failOnDelete {
 		return fmt.Errorf("mock error: Delete operation failed")

cmd/thv-operator/controllers/mcpserver_spec_patch_test.go

@@ -0,0 +1,184 @@
+// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+package controllers
+
+import (
+	"context"
+	"strings"
+	"sync"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	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/fake"
+
+	mcpv1beta1 "github.com/stacklok/toolhive/cmd/thv-operator/api/v1beta1"
+	"github.com/stacklok/toolhive/pkg/container/kubernetes"
+)
+
+// patchRecordingClient wraps a client.Client and records the marshaled body
+// of every Patch call. Tests use it to assert the wire-level flavor of a
+// patch — in particular, an optimistic-lock merge patch stamps the
+// resourceVersion into the body, so its presence in the recorded body is a
+// deterministic signal that MergeFromWithOptimisticLock was in effect.
+//
+// Patches issued via .Status().Patch do not pass through this wrapper:
+// controller-runtime's subresource client is obtained from the embedded
+// client.Client and has its own Patch implementation, so the recorder only
+// observes spec/metadata patches on the root client.
+type patchRecordingClient struct {
+	client.Client
+	mu      sync.Mutex
+	patches []recordedPatch
+}
+
+type recordedPatch struct {
+	obj  client.Object
+	body string
+}
+
+func (c *patchRecordingClient) Patch(
+	ctx context.Context, obj client.Object, patch client.Patch, opts ...client.PatchOption,
+) error {
+	// err ignored: patch.Data is json.Marshal of a typed MCPServer, which
+	// has no channels/funcs/cyclic pointers and cannot fail in practice.
+	// A failure here would also break the production controller's own
+	// Patch call and fire other assertions before this one.
+	if data, err := patch.Data(obj); err == nil {
+		c.mu.Lock()
+		c.patches = append(c.patches, recordedPatch{
+			obj:  obj.DeepCopyObject().(client.Object),
+			body: string(data),
+		})
+		c.mu.Unlock()
+	}
+	return c.Client.Patch(ctx, obj, patch, opts...)
+}
+
+// lastMCPServerPatchBody returns the body of the most recent recorded
+// Patch call whose target was an *mcpv1beta1.MCPServer. Returns empty
+// string if none was recorded.
+func (c *patchRecordingClient) lastMCPServerPatchBody() string {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	for i := len(c.patches) - 1; i >= 0; i-- {
+		if _, ok := c.patches[i].obj.(*mcpv1beta1.MCPServer); ok {
+			return c.patches[i].body
+		}
+	}
+	return ""
+}
+
+// TestMCPServerSpecPatchesAreOptimisticLock asserts that each of the three
+// MCPServer spec Patch call sites introduced in #4767 emits a merge-patch
+// whose body carries the resourceVersion precondition. A regression from
+// client.MergeFromWithOptions(orig, client.MergeFromWithOptimisticLock{})
+// to plain client.MergeFrom(orig) would drop the precondition and fail
+// these assertions, independent of whether the higher-level field-
+// clobber survival test still passes.
+func TestMCPServerSpecPatchesAreOptimisticLock(t *testing.T) {
+	t.Parallel()
+
+	const namespace = "default"
+
+	tests := []struct {
+		name string
+		// seed returns the MCPServer fixture placed in the fake client
+		// before the action runs. Returning a distinct name per case
+		// keeps parallel subtests from colliding on the shared fake.
+		seed func() *mcpv1beta1.MCPServer
+		// action triggers the reconcile path that should emit the
+		// optimistic-lock Patch under test. It is invoked with a
+		// recorder-backed reconciler.
+		action func(t *testing.T, r *MCPServerReconciler, key types.NamespacedName)
+	}{
+		{
+			name: "AddFinalizer",
+			seed: func() *mcpv1beta1.MCPServer {
+				s := createTestMCPServer("optlock-add", namespace)
+				// No finalizer yet — Reconcile should add it.
+				return s
+			},
+			action: func(t *testing.T, r *MCPServerReconciler, key types.NamespacedName) {
+				t.Helper()
+				_, _ = r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: key})
+			},
+		},
+		{
+			name: "RemoveFinalizer",
+			seed: func() *mcpv1beta1.MCPServer {
+				s := createTestMCPServer("optlock-remove", namespace)
+				s.Finalizers = []string{MCPServerFinalizerName}
+				// DeletionTimestamp forces Reconcile into the
+				// finalize branch. The fake client accepts an
+				// already-set timestamp on created objects.
+				now := metav1.Now()
+				s.DeletionTimestamp = &now
+				return s
+			},
+			action: func(t *testing.T, r *MCPServerReconciler, key types.NamespacedName) {
+				t.Helper()
+				_, _ = r.Reconcile(context.TODO(), ctrl.Request{NamespacedName: key})
+			},
+		},
+		{
+			name: "RestartAnnotation",
+			seed: func() *mcpv1beta1.MCPServer {
+				s := createTestMCPServer("optlock-restart", namespace)
+				s.Finalizers = []string{MCPServerFinalizerName}
+				if s.Annotations == nil {
+					s.Annotations = map[string]string{}
+				}
+				s.Annotations[RestartedAtAnnotationKey] = "2026-01-01T00:00:00Z"
+				s.Annotations[RestartStrategyAnnotationKey] = "immediate"
+				return s
+			},
+			action: func(t *testing.T, r *MCPServerReconciler, key types.NamespacedName) {
+				t.Helper()
+				got := &mcpv1beta1.MCPServer{}
+				require.NoError(t, r.Get(context.TODO(), key, got))
+				// handleRestartAnnotation is the innermost
+				// function that issues the Patch under test;
+				// calling it directly avoids exercising the
+				// rest of Reconcile, which would issue many
+				// unrelated writes.
+				_, _ = r.handleRestartAnnotation(context.TODO(), got)
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			seeded := tc.seed()
+			testScheme := createTestScheme()
+			fakeClient := fake.NewClientBuilder().
+				WithScheme(testScheme).
+				WithObjects(seeded).
+				WithStatusSubresource(&mcpv1beta1.MCPServer{}).
+				Build()
+			recorder := &patchRecordingClient{Client: fakeClient}
+			reconciler := newTestMCPServerReconciler(
+				recorder, testScheme, kubernetes.PlatformKubernetes)
+
+			tc.action(t, reconciler, types.NamespacedName{
+				Name:      seeded.Name,
+				Namespace: namespace,
+			})
+
+			body := recorder.lastMCPServerPatchBody()
+			require.NotEmpty(t, body,
+				"no MCPServer Patch was recorded; the reconcile path did not emit the expected write")
+			assert.True(t,
+				strings.Contains(body, `"resourceVersion"`),
+				"MCPServer spec patch body did not include a resourceVersion precondition; "+
+					"MergeFromWithOptimisticLock regression? body=%s", body)
+		})
+	}
+}