Handle partial volume creation failures

cmd/ateapi/internal/controlapi/create_actor.go

@@ -59,15 +59,9 @@ func (s *Service) CreateActor(ctx context.Context, req *ateapipb.CreateActorRequ
 		return nil, status.Errorf(codes.FailedPrecondition, "Atespace %s not found", atespace)
 	}
 
-	actorRef := &ateapipb.ObjectRef{
-		Atespace: atespace,
-		Name:     name,
-	}
-
-	volumes, err := s.createActorVolumes(ctx, actorRef, template)
-	if err != nil {
-		return nil, err
-	}
+	// Assuming CreateActor() will not become idempotent so we won't override volume state
+	// if the actor already exists.
+	initVols := initialActorVolumes(template)
 
 	actor := &ateapipb.Actor{
 		Metadata: &ateapipb.ResourceMetadata{
@@ -78,12 +72,10 @@ func (s *Service) CreateActor(ctx context.Context, req *ateapipb.CreateActorRequ
 		ActorTemplateNamespace: templateNamespace,
 		ActorTemplateName:      templateName,
 		WorkerSelector:         in.GetWorkerSelector(),
-		ActorVolumes:           volumes,
+		ActorVolumes:           initVols,
 	}
 	stored, err := s.persistence.CreateActor(ctx, actor)
 	if err != nil {
-		// Cleanup created volumes if DB write fails
-		_ = s.deleteActorVolumes(ctx, actorRef, volumes)
 		if errors.Is(err, store.ErrAlreadyExists) {
 			return nil, status.Errorf(codes.AlreadyExists, "Actor %s already exists", name)
 		}

cmd/ateapi/internal/controlapi/delete_actor.go

@@ -16,10 +16,7 @@ package controlapi
 
 import (
 	"context"
-	"errors"
-	"fmt"
 
-	"github.com/agent-substrate/substrate/cmd/ateapi/internal/store"
 	"github.com/agent-substrate/substrate/internal/resources"
 	"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
 	"google.golang.org/grpc/codes"
@@ -34,35 +31,9 @@ func (s *Service) DeleteActor(ctx context.Context, req *ateapipb.DeleteActorRequ
 	actorRef := resources.ActorRefFromObjectRef(req.GetActor())
 	setSpanActorRefAttributes(ctx, actorRef)
 
-	actor, err := s.persistence.GetActor(ctx, actorRef)
+	deleted, err := s.actorWorkflow.DeleteActor(ctx, req.GetActor().GetAtespace(), req.GetActor().GetName())
 	if err != nil {
-		if errors.Is(err, store.ErrNotFound) {
-			return nil, status.Errorf(codes.NotFound, "Actor %s not found", actorRef)
-		}
-		return nil, fmt.Errorf("while fetching actor: %w", err)
-	}
-
-	// Delete associated volumes
-	if err := s.deleteActorVolumes(ctx, req.GetActor(), actor.GetActorVolumes()); err != nil {
-		return nil, status.Errorf(codes.Internal, "while deleting actor volumes: %v", err)
-	}
-
-	deleted, err := s.persistence.DeleteActor(ctx, actorRef)
-	if err != nil {
-		if errors.Is(err, store.ErrNotFound) {
-			return nil, status.Errorf(codes.NotFound, "Actor %s not found", actorRef)
-		}
-		if errors.Is(err, store.ErrFailedPrecondition) {
-			current, getErr := s.persistence.GetActor(ctx, actorRef)
-			if getErr == nil {
-				return nil, status.Errorf(codes.FailedPrecondition, "Actor %s is not suspended (status: %v)", actorRef, current.GetStatus())
-			}
-			return nil, status.Errorf(codes.FailedPrecondition, "Actor %s is not suspended", actorRef)
-		}
-		if errors.Is(err, store.ErrVersionConflict) {
-			return nil, status.Error(codes.Aborted, "concurrent update conflict, please retry")
-		}
-		return nil, fmt.Errorf("while deleting actor from DB: %w", err)
+		return nil, err
 	}
 
 	return deleted, nil

cmd/ateapi/internal/controlapi/delete_actor_test.go

@@ -16,9 +16,15 @@ package controlapi
 
 import (
 	"context"
+	"fmt"
+	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
+
 	"github.com/agent-substrate/substrate/internal/ateattr"
+	"github.com/agent-substrate/substrate/internal/resources"
+	"github.com/agent-substrate/substrate/internal/volume"
 	"github.com/agent-substrate/substrate/pkg/proto/ateapipb"
 )
 
@@ -50,3 +56,116 @@ func TestDeleteActor_StampsRefSpanIdentity(t *testing.T) {
 	assertSpanStr(t, attrs, ateattr.AtespaceKey, testAtespace)
 	assertSpanStr(t, attrs, ateattr.ActorNameKey, testActorID)
 }
+
+func TestDeleteActor_StatusDeleting(t *testing.T) {
+	ns := namespaceForTest("ns-delete-deleting")
+	tc := setupTest(t, ns)
+	defer tc.cleanup()
+	createTemplate(t, tc, ns)
+
+	deletingActor := &ateapipb.Actor{
+		Metadata: &ateapipb.ResourceMetadata{
+			Atespace: testAtespace,
+			Name:     "deleting-actor",
+		},
+		Status:                 ateapipb.Actor_STATUS_DELETING,
+		ActorTemplateNamespace: ns,
+		ActorTemplateName:      "tmpl1",
+	}
+	if _, err := tc.persistence.CreateActor(context.Background(), deletingActor); err != nil {
+		t.Fatalf("CreateActor: %v", err)
+	}
+
+	if _, err := tc.service.DeleteActor(context.Background(), &ateapipb.DeleteActorRequest{
+		Actor: &ateapipb.ObjectRef{Atespace: testAtespace, Name: "deleting-actor"},
+	}); err != nil {
+		t.Fatalf("DeleteActor on STATUS_DELETING actor failed: %v", err)
+	}
+
+	if _, err := tc.persistence.GetActor(context.Background(), resources.ActorRef{Atespace: testAtespace, Name: "deleting-actor"}); err == nil {
+		t.Errorf("expected actor to be deleted, but it still exists")
+	}
+}
+
+func TestDeleteActor_WrongStatus(t *testing.T) {
+	ns := namespaceForTest("ns-delete-wrong-status")
+	tc := setupTest(t, ns)
+	defer tc.cleanup()
+	createTemplate(t, tc, ns)
+
+	runningActor := &ateapipb.Actor{
+		Metadata: &ateapipb.ResourceMetadata{
+			Atespace: testAtespace,
+			Name:     "running-actor",
+		},
+		Status:                 ateapipb.Actor_STATUS_RUNNING,
+		ActorTemplateNamespace: ns,
+		ActorTemplateName:      "tmpl1",
+	}
+	if _, err := tc.persistence.CreateActor(context.Background(), runningActor); err != nil {
+		t.Fatalf("CreateActor: %v", err)
+	}
+
+	_, err := tc.service.DeleteActor(context.Background(), &ateapipb.DeleteActorRequest{
+		Actor: &ateapipb.ObjectRef{Atespace: testAtespace, Name: "running-actor"},
+	})
+	if err == nil {
+		t.Fatalf("expected DeleteActor on STATUS_RUNNING actor to fail, but it succeeded")
+	}
+}
+
+type failingVolumePlugin struct {
+	volume.VolumePluginControlPlane
+	deletedIDs []string
+}
+
+func (f *failingVolumePlugin) DeleteVolume(ctx context.Context, volumeID string) error {
+	f.deletedIDs = append(f.deletedIDs, volumeID)
+	return fmt.Errorf("simulated delete error for %s", volumeID)
+}
+
+func TestDeleteActor_MultipleVolumeDeletionFailures(t *testing.T) {
+	ns := namespaceForTest("ns-delete-multivol-fail")
+	tc := setupTest(t, ns)
+	defer tc.cleanup()
+	createTemplate(t, tc, ns)
+
+	plugin := &failingVolumePlugin{}
+	oldGlobalPlugin := globalVolumePlugin
+	globalVolumePlugin = plugin
+	defer func() { globalVolumePlugin = oldGlobalPlugin }()
+
+	actor := &ateapipb.Actor{
+		Metadata: &ateapipb.ResourceMetadata{
+			Atespace: testAtespace,
+			Name:     "multi-vol-actor",
+		},
+		Status:                 ateapipb.Actor_STATUS_SUSPENDED,
+		ActorTemplateNamespace: ns,
+		ActorTemplateName:      "tmpl1",
+		ActorVolumes: []*ateapipb.ExternalVolume{
+			{VolumeName: "vol1", StorageVolumeId: "storage-vol-1", Status: ateapipb.ExternalVolume_CREATED},
+			{VolumeName: "vol2", StorageVolumeId: "storage-vol-2", Status: ateapipb.ExternalVolume_CREATED},
+		},
+	}
+	if _, err := tc.persistence.CreateActor(context.Background(), actor); err != nil {
+		t.Fatalf("CreateActor: %v", err)
+	}
+
+	_, err := tc.service.DeleteActor(context.Background(), &ateapipb.DeleteActorRequest{
+		Actor: &ateapipb.ObjectRef{Atespace: testAtespace, Name: "multi-vol-actor"},
+	})
+	if err == nil {
+		t.Fatalf("expected DeleteActor to fail when volume deletion fails, but it succeeded")
+	}
+
+	wantDeleted := []string{"storage-vol-1", "storage-vol-2"}
+	if diff := cmp.Diff(wantDeleted, plugin.deletedIDs); diff != "" {
+		t.Errorf("deletedIDs mismatch (-want +got):\n%s", diff)
+	}
+
+	errMsg := err.Error()
+	if !strings.Contains(errMsg, "storage-vol-1") || !strings.Contains(errMsg, "storage-vol-2") {
+		t.Errorf("expected error message to contain both volume failure details, got: %v", errMsg)
+	}
+}