From 9e399b34f9c406684b6542ea417fe76c42a3937f Mon Sep 17 00:00:00 2001
From: Julien Perrochet <julien.perro@gmail.com>
Date: Mon, 29 Jul 2024 14:50:11 +0200
Subject: [PATCH] [scd] inline cleanup of implicit subscription into CRDB call
 when removing OIR

---
 pkg/scd/operational_intents_handler.go        | 14 ------
 .../store/cockroach/operational_intents.go    | 45 +++++++++++++++++--
 2 files changed, 41 insertions(+), 18 deletions(-)

diff --git a/pkg/scd/operational_intents_handler.go b/pkg/scd/operational_intents_handler.go
index 86b8732e5..c6494125b 100644
--- a/pkg/scd/operational_intents_handler.go
+++ b/pkg/scd/operational_intents_handler.go
@@ -111,11 +111,6 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest
 			}
 		}
 
-		removeImplicitSubscription, err := subscriptionIsImplicitAndOnlyAttachedToOIR(ctx, r, id, previousSubscription)
-		if err != nil {
-			return stacktrace.Propagate(err, "Could not determine if Subscription can be removed")
-		}
-
 		// Gather the subscriptions that need to be notified
 		notifyVolume := &dssmodels.Volume4D{
 			StartTime: old.StartTime,
@@ -138,15 +133,6 @@ func (a *Server) DeleteOperationalIntentReference(ctx context.Context, req *rest
 			return stacktrace.Propagate(err, "Unable to delete OperationalIntent from repo")
 		}
 
-		// removeImplicitSubscription is only true if the OIR had a subscription defined
-		if removeImplicitSubscription {
-			// Automatically remove a now-unused implicit Subscription
-			err = r.DeleteSubscription(ctx, previousSubscription.ID)
-			if err != nil {
-				return stacktrace.Propagate(err, "Unable to delete associated implicit Subscription")
-			}
-		}
-
 		// Return response to client
 		response = &restapi.ChangeOperationalIntentReferenceResponse{
 			OperationalIntentReference: *old.ToRest(),
diff --git a/pkg/scd/store/cockroach/operational_intents.go b/pkg/scd/store/cockroach/operational_intents.go
index 90b3082c7..d3d2a7dea 100644
--- a/pkg/scd/store/cockroach/operational_intents.go
+++ b/pkg/scd/store/cockroach/operational_intents.go
@@ -169,10 +169,32 @@ func (s *repo) GetOperationalIntent(ctx context.Context, id dssmodels.ID) (*scdm
 func (s *repo) DeleteOperationalIntent(ctx context.Context, id dssmodels.ID) error {
 	var (
 		deleteOperationQuery = `
-			DELETE FROM
+			WITH deleted_oir AS (
+            DELETE FROM
 				scd_operations
 			WHERE
 				id = $1
+            RETURNING
+                id, subscription_id
+            ),
+            exists_and_is_implicit AS (
+                SELECT subscription_id
+                FROM deleted_oir
+                JOIN scd_subscriptions ON scd_subscriptions.id = deleted_oir.subscription_id
+                WHERE scd_subscriptions.implicit = true
+            ),
+            dependent_oirs AS ( -- NOTE: this sub-query will still return the OIR being deleted (!)
+                SELECT id
+                FROM scd_operations
+                WHERE subscription_id = (SELECT subscription_id FROM deleted_oir)
+            ),
+            deleted_subscription_id AS (
+                DELETE FROM scd_subscriptions
+                WHERE id = (SELECT subscription_id FROM exists_and_is_implicit)
+                AND (SELECT COUNT(*) FROM dependent_oirs) = 1 -- NOTE: see above, the OIR being removed is still counted here, hence a value of 1
+                RETURNING id
+            )
+            SELECT id FROM deleted_oir
 		`
 	)
 
@@ -180,13 +202,28 @@ func (s *repo) DeleteOperationalIntent(ctx context.Context, id dssmodels.ID) err
 	if err != nil {
 		return stacktrace.Propagate(err, "Failed to convert id to PgUUID")
 	}
-	res, err := s.q.Exec(ctx, deleteOperationQuery, uid)
+	res, err := s.q.Query(ctx, deleteOperationQuery, uid)
 	if err != nil {
 		return stacktrace.Propagate(err, "Error in query: %s", deleteOperationQuery)
 	}
+	defer res.Close()
 
-	if res.RowsAffected() == 0 {
-		return stacktrace.NewError("Could not delete Operation that does not exist")
+	// Check that the deleted OIR ID was returned:
+	var opID dssmodels.ID
+	if res.Next() {
+		err = res.Scan(&opID)
+		if err != nil {
+			return stacktrace.Propagate(err, "Error scanning deleted Operation ID")
+		}
+	} else if resErr := res.Err(); resErr != nil {
+		// Note: typically, res.Next() will be false and res.Err() non-nil when the query fails
+		// because it collided with another query and the whole transaction needs to be retried.
+		// This situation is extremely likely to occur when the DSS is under concurrent load.
+		return stacktrace.Propagate(resErr, "Error in query: %s", deleteOperationQuery)
+	}
+
+	if opID == "" || opID != id {
+		return stacktrace.NewError("Could not delete Operation that does not exist. Delete: %s, returned opID: %s", id, opID)
 	}
 
 	return nil