Add DeletePreconditions to all Delete<Resource> methods (agent-substrate#1807)

Allow clients to do optimistic locking on deletes, as well as prevent
unintended deletes on name reuse.

agent-substrate#866

Author: juli4n

benchmarking/locust/common/ateapi_pb2.py

@@ -434,16 +434,16 @@ func (s *RPCService) DeleteActor(ctx context.Context, req *ateapipb.DeleteActorR
 	actorRef := resources.ActorRefFromObjectRef(req.GetActor())
 	setSpanActorRefAttributes(ctx, actorRef)
 
-	deleted, err = s.actorWorkflow.DeleteActor(ctx, actorRef, req.GetAnyState())
+	deleted, err = s.actorWorkflow.DeleteActor(ctx, actorRef, req.GetAnyState(), toDeletePreconditions(req.GetOptions()))
 	if err != nil {
 		return nil, err
 	}
 
 	return deleted, nil
 }
 
-func (s *ServiceImpl) DeleteActor(ctx context.Context, actorRef resources.ActorRef) (*ateapipb.Actor, error) {
-	return s.store.DeleteActor(ctx, actorRef)
+func (s *ServiceImpl) DeleteActor(ctx context.Context, actorRef resources.ActorRef, precondition store.DeletePreconditions) (*ateapipb.Actor, error) {
+	return s.store.DeleteActor(ctx, actorRef, precondition)
 }
 
 func validateDeleteActorRequest(ctx context.Context, req *ateapipb.DeleteActorRequest) field.ErrorList {

cmd/ateapi/internal/controlapi/actor.go

@@ -156,45 +156,12 @@ func (s *RPCService) DeleteActorTemplate(ctx context.Context, req *ateapipb.Dele
 	if errs := validateDeleteActorTemplateRequest(ctx, req); len(errs) > 0 {
 		return nil, toGRPCStatusError(errs)
 	}
-
-	templateRef := resources.ActorTemplateRefFromObjectRef(req.GetActorTemplate())
-	// Serialize cleanup against golden actor/tag creation by the reconciler.
-	ctx, lease, err := acquireLease(ctx, s.impl, "lease:actortemplate:"+templateRef.Atespace+":"+templateRef.Name, "ActorTemplate "+templateRef.String())
-	if err != nil {
-		return nil, err
-	}
-	defer lease.Close()
-	tmpl, err := s.impl.GetActorTemplate(ctx, templateRef)
-	if errors.Is(err, store.ErrNotFound) {
-		return nil, status.Errorf(codes.NotFound, "ActorTemplate %s not found", templateRef)
-	}
-	if err != nil {
-		return nil, err
-	}
-	goldenRef := &ateapipb.ObjectRef{Atespace: resources.GoldenActorAtespace, Name: tmpl.GetMetadata().GetUid()}
-	if _, err := s.DeleteActor(ctx, &ateapipb.DeleteActorRequest{Actor: goldenRef, AnyState: true}); err != nil && status.Code(err) != codes.NotFound {
-		return nil, fmt.Errorf("while deleting golden actor: %w", err)
-	}
-	if _, err := s.DeleteTag(ctx, &ateapipb.DeleteTagRequest{Tag: goldenRef}); err != nil && status.Code(err) != codes.NotFound {
-		return nil, fmt.Errorf("while deleting golden tag: %w", err)
-	}
-	deleted, err := s.impl.DeleteActorTemplate(ctx, templateRef)
-	if err != nil {
-		if errors.Is(err, store.ErrNotFound) {
-			return nil, status.Errorf(codes.NotFound, "ActorTemplate %s not found", templateRef)
-		}
-		if errors.Is(err, store.ErrFailedPrecondition) {
-			return nil, status.Error(codes.FailedPrecondition, err.Error())
-		}
-		return nil, fmt.Errorf("while deleting actor template from DB: %w", err)
-	}
-
-	return deleted, nil
+	return s.actorWorkflow.DeleteActorTemplate(ctx, resources.ActorTemplateRefFromObjectRef(req.GetActorTemplate()), toDeletePreconditions(req.GetOptions()))
 }
 
-func (s *ServiceImpl) DeleteActorTemplate(ctx context.Context, templateRef resources.ActorTemplateRef) (*ateapipb.ActorTemplate, error) {
+func (s *ServiceImpl) DeleteActorTemplate(ctx context.Context, templateRef resources.ActorTemplateRef, precondition store.DeletePreconditions) (*ateapipb.ActorTemplate, error) {
 	// TODO: implement this
-	return s.store.DeleteActorTemplate(ctx, templateRef)
+	return s.store.DeleteActorTemplate(ctx, templateRef, precondition)
 }
 
 func validateDeleteActorTemplateRequest(ctx context.Context, req *ateapipb.DeleteActorTemplateRequest) field.ErrorList {

cmd/ateapi/internal/controlapi/actor_template.go

@@ -359,6 +359,7 @@ func TestDeleteActorTemplate(t *testing.T) {
 		// failPrefix makes object storage fail cleanup for this resource kind.
 		failPrefix            string
 		wantActorAfterFailure bool
+		staleGuard            bool
 	}{
 		{name: "golden actor and tag"},
 		{name: "golden actor already deleted", actorDeleted: true},
@@ -367,6 +368,7 @@ func TestDeleteActorTemplate(t *testing.T) {
 		{name: "incomplete golden tag", pendingTag: true},
 		{name: "actor cleanup failure", failPrefix: "/actors/", wantActorAfterFailure: true},
 		{name: "tag cleanup failure", failPrefix: "/tags/"},
+		{name: "stale guard refused before cleanup", staleGuard: true},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -406,7 +408,7 @@ func TestDeleteActorTemplate(t *testing.T) {
 				s.State = ateapipb.ActorState_ACTOR_STATE_RUNNING
 			})
 			if tt.actorDeleted {
-				if _, err := workflow.DeleteActor(ctx, goldenRef, true); err != nil {
+				if _, err := workflow.DeleteActor(ctx, goldenRef, true, store.DeletePreconditions{}); err != nil {
 					t.Fatal(err)
 				}
 			}
@@ -423,6 +425,22 @@ func TestDeleteActorTemplate(t *testing.T) {
 					return nil
 				}
 			}
+			if tt.staleGuard {
+				current, err := persistence.GetActorTemplate(ctx, templateRef)
+				if err != nil {
+					t.Fatal(err)
+				}
+				stale := store.DeletePreconditions{UID: current.GetMetadata().GetUid(), Version: current.GetMetadata().GetVersion() + 1}
+				if _, err := workflow.DeleteActorTemplate(ctx, templateRef, stale); status.Code(err) != codes.Aborted {
+					t.Fatalf("DeleteActorTemplate with a stale version = %v, want code Aborted", err)
+				}
+				if _, err := persistence.GetActor(ctx, goldenRef); err != nil {
+					t.Fatalf("golden actor after the refused delete: %v", err)
+				}
+				if _, err := persistence.GetTag(ctx, tagRef); err != nil {
+					t.Fatalf("golden tag after the refused delete: %v", err)
+				}
+			}
 			req := &ateapipb.DeleteActorTemplateRequest{ActorTemplate: templateRef.ToObjectRef()}
 			deleted, err := svc.DeleteActorTemplate(ctx, req)
 			if tt.failPrefix != "" {

cmd/ateapi/internal/controlapi/actor_template_test.go

@@ -1117,7 +1117,7 @@ func TestUpdateActor_DeleteRecreateRace(t *testing.T) {
 			}); err != nil {
 				t.Fatalf("racing writer: mark deleting: %v", err)
 			}
-			if _, err := persistence.DeleteActor(ctx, actorRef); err != nil {
+			if _, err := persistence.DeleteActor(ctx, actorRef, store.DeletePreconditions{}); err != nil {
 				t.Fatalf("racing writer: DeleteActor: %v", err)
 			}
 			recreated, err = persistence.CreateActor(ctx, &ateapipb.Actor{

cmd/ateapi/internal/controlapi/actor_test.go

@@ -128,18 +128,24 @@ func (s *RPCService) DeleteAtespace(ctx context.Context, req *ateapipb.DeleteAte
 		return nil, toGRPCStatusError(errs)
 	}
 
-	return s.impl.DeleteAtespace(ctx, req.Atespace.Name)
+	return s.impl.DeleteAtespace(ctx, req.Atespace.Name, toDeletePreconditions(req.GetOptions()))
 }
 
-func (s *ServiceImpl) DeleteAtespace(ctx context.Context, name string) (*ateapipb.Atespace, error) {
-	deleted, err := s.store.DeleteAtespace(ctx, name)
+func (s *ServiceImpl) DeleteAtespace(ctx context.Context, name string, precondition store.DeletePreconditions) (*ateapipb.Atespace, error) {
+	deleted, err := s.store.DeleteAtespace(ctx, name, precondition)
 	if err != nil {
 		if errors.Is(err, store.ErrNotFound) {
 			return nil, status.Errorf(codes.NotFound, "Atespace %s not found", name)
 		}
 		if errors.Is(err, store.ErrFailedPrecondition) {
 			return nil, status.Errorf(codes.FailedPrecondition, "Atespace %s is not empty", name)
 		}
+		if errors.Is(err, store.ErrUIDConflict) {
+			return nil, status.Errorf(codes.Aborted, "Atespace %s does not have uid %s", name, precondition.UID)
+		}
+		if errors.Is(err, store.ErrVersionConflict) {
+			return nil, status.Error(codes.Aborted, "concurrent update conflict, please retry")
+		}
 		return nil, fmt.Errorf("while deleting atespace from DB: %w", err)
 	}
 

cmd/ateapi/internal/controlapi/atespace.go

@@ -0,0 +1,24 @@
+// Copyright 2026 Google LLC
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package controlapi
+
+import (
+	"github.com/agent-substrate/substrate/cmd/ateapi/internal/store"
+	"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
+)
+
+func toDeletePreconditions(opts *ateapipb.DeleteOptions) store.DeletePreconditions {
+	return store.DeletePreconditions{UID: opts.GetUid(), Version: opts.GetVersion()}
+}

cmd/ateapi/internal/controlapi/delete_options.go

@@ -69,7 +69,7 @@ func (s *RPCService) GetActorEgressPolicy(ctx context.Context, req *ateapipb.Get
 func (s *ServiceImpl) GetEgressPolicy(ctx context.Context, actorRef resources.ActorRef) (*ateapipb.EgressPolicy, error) {
 	policy, err := s.store.GetEgressPolicy(ctx, actorRef)
 	if errors.Is(err, store.ErrNotFound) {
-		return nil, status.Error(codes.NotFound, "EgressPolicy not found")
+		return nil, status.Errorf(codes.NotFound, "EgressPolicy for actor %s not found", actorRef)
 	}
 	if err != nil {
 		return nil, fmt.Errorf("while getting Actor egress policy: %w", err)
@@ -128,11 +128,11 @@ func (s *RPCService) DeleteActorEgressPolicy(ctx context.Context, req *ateapipb.
 		return nil, toGRPCStatusError(errs)
 	}
 
-	return s.impl.DeleteEgressPolicy(ctx, resources.ActorRefFromObjectRef(req.GetActor()))
+	return s.impl.DeleteEgressPolicy(ctx, resources.ActorRefFromObjectRef(req.GetActor()), toDeletePreconditions(req.GetOptions()))
 }
 
-func (s *ServiceImpl) DeleteEgressPolicy(ctx context.Context, actorRef resources.ActorRef) (*ateapipb.EgressPolicy, error) {
-	deleted, err := s.store.DeleteEgressPolicy(ctx, actorRef)
+func (s *ServiceImpl) DeleteEgressPolicy(ctx context.Context, actorRef resources.ActorRef, precondition store.DeletePreconditions) (*ateapipb.EgressPolicy, error) {
+	deleted, err := s.store.DeleteEgressPolicy(ctx, actorRef, precondition)
 	return mapEgressPolicyWrite(deleted, err)
 }
 

cmd/ateapi/internal/controlapi/egress_policy.go

@@ -16,12 +16,13 @@ package functionaltest
 
 import (
 	"context"
+	"testing"
+	"time"
+
 	"github.com/agent-substrate/substrate/cmd/ateapi/internal/controlapi"
 	"github.com/agent-substrate/substrate/internal/resources"
 	"google.golang.org/grpc/status"
 	"k8s.io/apimachinery/pkg/util/wait"
-	"testing"
-	"time"
 
 	"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
 	"github.com/google/go-cmp/cmp"
@@ -245,3 +246,41 @@ func TestListActorTemplates_InvalidPageToken(t *testing.T) {
 		&ateapipb.ListActorTemplatesRequest{PageToken: "%%%"})
 	assertGrpcError(t, err, codes.InvalidArgument, "invalid page_token")
 }
+
+func TestDeleteActorTemplate_Preconditions(t *testing.T) {
+	ns := namespaceForTest("ns-delete-template-preconditions")
+	tc := setupTest(t, ns)
+	defer tc.cleanup()
+	ctx := context.Background()
+	tmpl := createTemplateWithSelector(t, tc, "tmpl1", nil)
+	templateRef := resources.ActorTemplateRefFromActorTemplate(tmpl)
+	ref := templateRef.ToObjectRef()
+	del := func(opts *ateapipb.DeleteOptions) error {
+		_, err := tc.client.DeleteActorTemplate(ctx, &ateapipb.DeleteActorTemplateRequest{ActorTemplate: ref, Options: opts})
+		return err
+	}
+
+	uid, version := tmpl.GetMetadata().GetUid(), tmpl.GetMetadata().GetVersion()
+
+	assertGrpcError(t, del(&ateapipb.DeleteOptions{Version: version + 1}), codes.Aborted, "concurrent update conflict, please retry")
+	assertGrpcError(t, del(&ateapipb.DeleteOptions{Uid: uid, Version: version + 1}), codes.Aborted, "concurrent update conflict, please retry")
+	assertGrpcError(t, del(&ateapipb.DeleteOptions{Uid: foreignUID}), codes.Aborted, "ActorTemplate "+templateRef.String()+" does not have uid "+foreignUID)
+	assertGrpcError(t, del(&ateapipb.DeleteOptions{Uid: foreignUID, Version: version}), codes.Aborted, "ActorTemplate "+templateRef.String()+" does not have uid "+foreignUID)
+	if _, err := tc.client.GetActorTemplate(ctx, &ateapipb.GetActorTemplateRequest{ActorTemplate: ref}); err != nil {
+		t.Fatalf("a refused delete removed the template: %v", err)
+	}
+
+	if err := del(&ateapipb.DeleteOptions{Version: version}); err != nil {
+		t.Fatalf("DeleteActorTemplate with the matching version: %v", err)
+	}
+	tmpl = createTemplateWithSelector(t, tc, "tmpl1", nil)
+	if err := del(&ateapipb.DeleteOptions{Uid: tmpl.GetMetadata().GetUid()}); err != nil {
+		t.Fatalf("DeleteActorTemplate with the matching uid: %v", err)
+	}
+	tmpl = createTemplateWithSelector(t, tc, "tmpl1", nil)
+	if err := del(&ateapipb.DeleteOptions{Uid: tmpl.GetMetadata().GetUid(), Version: tmpl.GetMetadata().GetVersion()}); err != nil {
+		t.Fatalf("DeleteActorTemplate with both guards: %v", err)
+	}
+	_, err := tc.client.GetActorTemplate(ctx, &ateapipb.GetActorTemplateRequest{ActorTemplate: ref})
+	assertGrpcError(t, err, codes.NotFound, "ActorTemplate "+templateRef.String()+" not found")
+}

cmd/ateapi/internal/controlapi/functionaltest/actor_template_test.go

@@ -3806,3 +3806,56 @@ func TestRevertActor_NotFound(t *testing.T) {
 		t.Fatalf("RevertActor = %v, want NotFound", err)
 	}
 }
+
+func TestDeleteActor_Preconditions(t *testing.T) {
+	ns := namespaceForTest("ns-delete-preconditions")
+	tc := setupTest(t, ns)
+	defer tc.cleanup()
+	ctx := context.Background()
+	tmpl := createTemplate(t, tc, ns)
+	actorRef := resources.ActorRef{Atespace: testAtespace, Name: "id1"}
+	ref := actorRef.ToObjectRef()
+	create := func() *ateapipb.Actor {
+		created, err := tc.client.CreateActor(ctx, &ateapipb.CreateActorRequest{Actor: &ateapipb.Actor{
+			Metadata:      &ateapipb.ResourceMetadata{Atespace: actorRef.Atespace, Name: actorRef.Name},
+			ActorTemplate: resources.ActorTemplateRefFromActorTemplate(tmpl).ToObjectRef(),
+		}})
+		if err != nil {
+			t.Fatalf("CreateActor failed: %v", err)
+		}
+		return created
+	}
+	del := func(opts *ateapipb.DeleteOptions) error {
+		_, err := tc.client.DeleteActor(ctx, &ateapipb.DeleteActorRequest{Actor: ref, Options: opts})
+		return err
+	}
+
+	actor := create()
+	uid, version := actor.GetMetadata().GetUid(), actor.GetMetadata().GetVersion()
+
+	assertGrpcError(t, del(&ateapipb.DeleteOptions{Version: version + 1}), codes.Aborted, "concurrent update conflict, please retry")
+	assertGrpcError(t, del(&ateapipb.DeleteOptions{Uid: uid, Version: version + 1}), codes.Aborted, "concurrent update conflict, please retry")
+	assertGrpcError(t, del(&ateapipb.DeleteOptions{Uid: foreignUID}), codes.Aborted, "Actor "+actorRef.String()+" does not have uid "+foreignUID)
+	assertGrpcError(t, del(&ateapipb.DeleteOptions{Uid: foreignUID, Version: version}), codes.Aborted, "Actor "+actorRef.String()+" does not have uid "+foreignUID)
+	got, err := tc.client.GetActor(ctx, &ateapipb.GetActorRequest{Actor: ref})
+	if err != nil {
+		t.Fatalf("a refused delete removed the actor: %v", err)
+	}
+	if state := got.GetStatus().GetState(); state != ateapipb.ActorState_ACTOR_STATE_SUSPENDED {
+		t.Fatalf("state after the refused deletes = %v, want SUSPENDED", state)
+	}
+
+	if err := del(&ateapipb.DeleteOptions{Version: version}); err != nil {
+		t.Fatalf("DeleteActor with the matching version: %v", err)
+	}
+	actor = create()
+	if err := del(&ateapipb.DeleteOptions{Uid: actor.GetMetadata().GetUid()}); err != nil {
+		t.Fatalf("DeleteActor with the matching uid: %v", err)
+	}
+	actor = create()
+	if err := del(&ateapipb.DeleteOptions{Uid: actor.GetMetadata().GetUid(), Version: actor.GetMetadata().GetVersion()}); err != nil {
+		t.Fatalf("DeleteActor with both guards: %v", err)
+	}
+	_, err = tc.client.GetActor(ctx, &ateapipb.GetActorRequest{Actor: ref})
+	assertGrpcError(t, err, codes.NotFound, "Actor "+actorRef.String()+" not found")
+}