fix(sandboxcr): bind Sandbox mutations to the authorized incarnation

pkg/sandbox-manager/infra/sandboxcr/sandbox.go

@@ -23,6 +23,7 @@ import (
 	"net/http"
 	"time"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/client-go/util/retry"
 	"k8s.io/klog/v2"
@@ -79,8 +80,37 @@ func (s *Sandbox) GetRoute() (sandboxroute.Route, error) {
 
 var DefaultDeleteSandbox = deleteSandbox
 
-func deleteSandbox(ctx context.Context, sbx *agentsv1alpha1.Sandbox, client client.Client) error {
-	return client.Delete(ctx, sbx)
+func deleteSandbox(ctx context.Context, sbx *agentsv1alpha1.Sandbox, c client.Client) error {
+	// Bind the delete to the incarnation the caller was authorized for. The API
+	// server otherwise resolves this by namespace and name alone, so a Sandbox
+	// recreated under the same name after authorization would be deleted in its
+	// place. A synthetic object without a UID keeps the old behaviour.
+	if sbx.UID == "" {
+		return c.Delete(ctx, sbx)
+	}
+	return c.Delete(ctx, sbx, client.Preconditions{UID: &sbx.UID})
+}
+
+// assertSameIncarnation guards a re-read that resolved by namespace and name.
+// Authorization is performed against one specific Sandbox, but every re-read
+// below looks the object up by ObjectKey, so a same-name Sandbox created in the
+// window between the two would otherwise be adopted and mutated in its place.
+//
+// A mismatch is reported as NotFound: from the caller's point of view the
+// Sandbox it was authorized for no longer exists, which is what the API layer
+// should surface rather than acting on the replacement.
+//
+// An empty UID on either side means the caller is holding a synthetic object
+// that was never read from the API server, so there is nothing to compare and
+// the check is skipped.
+func assertSameIncarnation(want, got *agentsv1alpha1.Sandbox) error {
+	if want.UID == "" || got.UID == "" || want.UID == got.UID {
+		return nil
+	}
+	return apierrors.NewNotFound(
+		agentsv1alpha1.GroupVersion.WithResource("sandboxes").GroupResource(),
+		want.Name,
+	)
 }
 
 func (s *Sandbox) GetTemplate() string {
@@ -105,6 +135,9 @@ func (s *Sandbox) InplaceRefresh(ctx context.Context, deepcopy bool) error {
 			return err
 		}
 	}
+	if err = assertSameIncarnation(s.Sandbox, newSbx); err != nil {
+		return err
+	}
 	if expectations.IsResourceVersionReallyNewer(s.Sandbox.GetResourceVersion(), newSbx.GetResourceVersion()) {
 		if deepcopy {
 			s.Sandbox = newSbx.DeepCopy()
@@ -153,6 +186,9 @@ func (s *Sandbox) retryUpdate(ctx context.Context, modifier ModifierFunc) (bool,
 		if err != nil {
 			return err
 		}
+		if err = assertSameIncarnation(s.Sandbox, latest); err != nil {
+			return err
+		}
 
 		copied := latest.DeepCopy()
 		shouldUpdate, err := modifier(copied)

pkg/sandbox-manager/infra/sandboxcr/sandbox_test.go

@@ -1509,3 +1509,74 @@ func TestSandbox_TriggerRecycle(t *testing.T) {
 		})
 	}
 }
+
+// TestSandbox_IncarnationBinding covers the window in #792: authorization
+// happens against one Sandbox, but the re-reads below resolve by namespace and
+// name, so a same-name Sandbox created in between must not be adopted or
+// mutated in place of the authorized one.
+func TestSandbox_IncarnationBinding(t *testing.T) {
+	newSandbox := func(uid types.UID, resourceVersion string) *v1alpha1.Sandbox {
+		return &v1alpha1.Sandbox{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:            "demo",
+				Namespace:       "team-a",
+				UID:             uid,
+				ResourceVersion: resourceVersion,
+			},
+		}
+	}
+
+	t.Run("assertSameIncarnation", func(t *testing.T) {
+		tests := []struct {
+			name        string
+			want, got   *v1alpha1.Sandbox
+			expectError bool
+		}{
+			{name: "same UID passes", want: newSandbox("uid-a", "1"), got: newSandbox("uid-a", "2")},
+			{name: "replacement is rejected", want: newSandbox("uid-a", "1"), got: newSandbox("uid-b", "1"), expectError: true},
+			{name: "synthetic caller object is skipped", want: newSandbox("", "1"), got: newSandbox("uid-b", "1")},
+			{name: "synthetic read object is skipped", want: newSandbox("uid-a", "1"), got: newSandbox("", "1")},
+		}
+		for _, tt := range tests {
+			t.Run(tt.name, func(t *testing.T) {
+				err := assertSameIncarnation(tt.want, tt.got)
+				if !tt.expectError {
+					assert.NoError(t, err)
+					return
+				}
+				require.Error(t, err)
+				// NotFound so the API layer reports the authorized Sandbox as
+				// gone rather than acting on its replacement.
+				assert.True(t, apierrors.IsNotFound(err), "expected NotFound, got %v", err)
+			})
+		}
+	})
+
+	t.Run("delete carries a UID precondition", func(t *testing.T) {
+		scheme := k8sruntime.NewScheme()
+		utilruntime.Must(clientgoscheme.AddToScheme(scheme))
+		utilruntime.Must(v1alpha1.AddToScheme(scheme))
+
+		var gotPreconditionUID types.UID
+		fc := fake.NewClientBuilder().WithScheme(scheme).
+			WithInterceptorFuncs(interceptor.Funcs{
+				Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error {
+					options := &client.DeleteOptions{}
+					for _, opt := range opts {
+						opt.ApplyToDelete(options)
+					}
+					if options.Preconditions != nil && options.Preconditions.UID != nil {
+						gotPreconditionUID = *options.Preconditions.UID
+					}
+					return nil
+				},
+			}).Build()
+
+		require.NoError(t, deleteSandbox(context.Background(), newSandbox("uid-a", "1"), fc))
+		assert.Equal(t, types.UID("uid-a"), gotPreconditionUID)
+
+		gotPreconditionUID = ""
+		require.NoError(t, deleteSandbox(context.Background(), newSandbox("", "1"), fc))
+		assert.Empty(t, gotPreconditionUID, "a synthetic object sends no precondition")
+	})
+}