Make partial cancellelation state part of the API

Also rename factory function to avoid explicit coupling to higher
level semantics.

9 files changed, 16 insertions, 13 deletions

diff --git a/storage/src/tests/distributor/check_condition_test.cpp b/storage/src/tests/distributor/check_condition_test.cpp
index 92c3b120da2..617401dd271 100644
--- a/storage/src/tests/distributor/check_condition_test.cpp
+++ b/storage/src/tests/distributor/check_condition_test.cpp
@@ -231,7 +231,7 @@ TEST_F(CheckConditionTest, failed_gets_completes_check_with_error_outcome) {
 TEST_F(CheckConditionTest, check_fails_if_condition_explicitly_cancelled) {
     test_cond_with_2_gets_sent([&](auto& cond) {
         cond.handle_reply(_sender, make_matched_reply(0));
-        cond.cancel(_sender, CancelScope::of_ownership_change());
+        cond.cancel(_sender, CancelScope::of_fully_cancelled());
         cond.handle_reply(_sender, make_matched_reply(1));
     }, [&](auto& outcome) {
         EXPECT_FALSE(outcome.matched_condition());
diff --git a/storage/src/tests/distributor/garbagecollectiontest.cpp b/storage/src/tests/distributor/garbagecollectiontest.cpp
index c3cec32b279..b1cf1cbc636 100644
--- a/storage/src/tests/distributor/garbagecollectiontest.cpp
+++ b/storage/src/tests/distributor/garbagecollectiontest.cpp
@@ -177,7 +177,7 @@ TEST_F(GarbageCollectionOperationTest, no_replica_bucket_info_added_to_db_if_ope
     ASSERT_EQ(2, _sender.commands().size());
 
     reply_to_nth_request(*op, 0, 1234, 70);
-    op->cancel(_sender, CancelScope::of_ownership_change());
+    op->cancel(_sender, CancelScope::of_fully_cancelled());
     reply_to_nth_request(*op, 1, 4567, 60);
 
     // DB state is unchanged. Note that in a real scenario, the DB entry will have been removed
@@ -408,7 +408,7 @@ TEST_F(GarbageCollectionOperationPhase1FailureTest, no_second_phase_if_bucket_in
 }
 
 TEST_F(GarbageCollectionOperationPhase1FailureTest, no_second_phase_if_operation_fully_cancelled_between_phases) {
-    _op->cancel(_sender, CancelScope::of_ownership_change());
+    _op->cancel(_sender, CancelScope::of_fully_cancelled());
     receive_phase1_replies_and_assert_no_phase_2_started();
 }
 
diff --git a/storage/src/tests/distributor/putoperationtest.cpp b/storage/src/tests/distributor/putoperationtest.cpp
index 431b7595571..ee87fe84df6 100644
--- a/storage/src/tests/distributor/putoperationtest.cpp
+++ b/storage/src/tests/distributor/putoperationtest.cpp
@@ -217,7 +217,7 @@ TEST_F(PutOperationTest, failed_CreateBucket_does_not_send_RequestBucketInfo_if_
 
     ASSERT_EQ("Create bucket => 1,Create bucket => 0,Put => 1,Put => 0", _sender.getCommands(true));
 
-    op->cancel(_sender, CancelScope::of_ownership_change());
+    op->cancel(_sender, CancelScope::of_fully_cancelled());
     sendReply(0, api::ReturnCode::TIMEOUT, api::BucketInfo()); // CreateBucket to node 1
 
     // DB is not touched (note: normally node 1 would be removed at the cancel-edge).
@@ -320,7 +320,7 @@ TEST_F(PutOperationTest, return_success_if_op_acked_on_all_replicas_even_if_oper
               "id:test:testdoctype1::, timestamp 100, size 45) => 1",
               _sender.getCommands(true, true));
 
-    op->cancel(_sender, CancelScope::of_ownership_change());
+    op->cancel(_sender, CancelScope::of_fully_cancelled());
 
     sendReply(0);
     sendReply(1);
@@ -659,7 +659,7 @@ TEST_F(PutOperationTest, db_not_updated_if_operation_cancelled_by_ownership_chan
     ASSERT_EQ("Put => 1,Put => 2,Put => 0", _sender.getCommands(true));
 
     operation_context().remove_nodes_from_bucket_database(makeDocumentBucket(bucket), {0, 1, 2});
-    op->cancel(_sender, CancelScope::of_ownership_change());
+    op->cancel(_sender, CancelScope::of_fully_cancelled());
 
     // Normally DB updates triggered by replies don't _create_ buckets in the DB, unless
     // they're remapped buckets. Use a remapping to ensure we hit a create-if-missing DB path.
@@ -886,7 +886,7 @@ TEST_F(PutOperationTest, ownership_cancellation_during_condition_probe_fails_ope
     ASSERT_NO_FATAL_FAILURE(set_up_tas_put_with_2_inconsistent_replica_nodes());
 
     op->receive(_sender, make_get_reply(*sent_get_command(0), 0, false, false));
-    op->cancel(_sender, CancelScope::of_ownership_change());
+    op->cancel(_sender, CancelScope::of_fully_cancelled());
     op->receive(_sender, make_get_reply(*sent_get_command(1), 0, false, false));
 
     ASSERT_EQ("Get => 1,Get => 0", _sender.getCommands(true));
diff --git a/storage/src/tests/distributor/removeoperationtest.cpp b/storage/src/tests/distributor/removeoperationtest.cpp
index 762f9857fce..3fad2c194a2 100644
--- a/storage/src/tests/distributor/removeoperationtest.cpp
+++ b/storage/src/tests/distributor/removeoperationtest.cpp
@@ -315,7 +315,7 @@ TEST_F(ExtRemoveOperationTest, cancellation_during_condition_probe_fails_operati
     ASSERT_NO_FATAL_FAILURE(set_up_tas_remove_with_2_nodes(ReplicaState::INCONSISTENT));
 
     reply_with(make_get_reply(0, 50, false, true));
-    op->cancel(_sender, CancelScope::of_ownership_change());
+    op->cancel(_sender, CancelScope::of_fully_cancelled());
     reply_with(make_get_reply(1, 50, false, true));
 
     ASSERT_EQ("Get => 1,Get => 0", _sender.getCommands(true));
diff --git a/storage/src/vespa/storage/distributor/operations/cancel_scope.cpp b/storage/src/vespa/storage/distributor/operations/cancel_scope.cpp
index ef4d99aa58b..3a05ce3975d 100644
--- a/storage/src/vespa/storage/distributor/operations/cancel_scope.cpp
+++ b/storage/src/vespa/storage/distributor/operations/cancel_scope.cpp
@@ -41,7 +41,7 @@ void CancelScope::merge(const CancelScope& other) {
     }
 }
 
-CancelScope CancelScope::of_ownership_change() noexcept {
+CancelScope CancelScope::of_fully_cancelled() noexcept {
     return CancelScope(ownership_change_ctor_tag{});
 }
 
diff --git a/storage/src/vespa/storage/distributor/operations/cancel_scope.h b/storage/src/vespa/storage/distributor/operations/cancel_scope.h
index e7da26c600a..20998375a71 100644
--- a/storage/src/vespa/storage/distributor/operations/cancel_scope.h
+++ b/storage/src/vespa/storage/distributor/operations/cancel_scope.h
@@ -44,6 +44,8 @@ public:
     void merge(const CancelScope& other);
 
     [[nodiscard]] bool fully_cancelled() const noexcept { return _ownership_lost; }
+    // Note: partially_cancelled() does not subsume fully_cancelled(); it must be checked independently
+    [[nodiscard]] bool partially_cancelled() const noexcept { return !_cancelled_nodes.empty(); }
     [[nodiscard]] bool node_is_cancelled(uint16_t node) const noexcept {
         return (fully_cancelled() || _cancelled_nodes.contains(node));
     }
@@ -52,7 +54,7 @@ public:
         return _cancelled_nodes;
     }
 
-    static CancelScope of_ownership_change() noexcept;
+    static CancelScope of_fully_cancelled() noexcept;
     static CancelScope of_node_subset(CancelledNodeSet nodes) noexcept;
 };
 
diff --git a/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.cpp b/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.cpp
index 5d47fb4c337..842657fdf9c 100644
--- a/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.cpp
+++ b/storage/src/vespa/storage/distributor/operations/idealstate/garbagecollectionoperation.cpp
@@ -268,7 +268,7 @@ void GarbageCollectionOperation::merge_received_bucket_info_into_db() {
     if (_cancel_scope) {
         if (_cancel_scope->fully_cancelled()) {
             return;
-        } else {
+        } else if (_cancel_scope->partially_cancelled()) {
             _replica_info = prune_cancelled_nodes(_replica_info, *_cancel_scope);
         }
     }
diff --git a/storage/src/vespa/storage/distributor/operations/operation.h b/storage/src/vespa/storage/distributor/operations/operation.h
index 1154cead61f..64caacfc642 100644
--- a/storage/src/vespa/storage/distributor/operations/operation.h
+++ b/storage/src/vespa/storage/distributor/operations/operation.h
@@ -63,7 +63,8 @@ public:
     /**
      * Explicitly cancel the operation. Cancelled operations may or may not (depending on
      * the operation implementation) be immediately aborted, but they should either way
-     * never insert any bucket information into the bucket DB after cancel() has been called.
+     * never insert any bucket information _for cancelled nodes_ into the bucket DB after
+     * cancel() has been called.
      */
     void cancel(DistributorStripeMessageSender& sender, const CancelScope& cancel_scope);
 
diff --git a/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp b/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp
index bcb849ac840..8a60f4c787a 100644
--- a/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp
+++ b/storage/src/vespa/storage/distributor/persistencemessagetracker.cpp
@@ -64,7 +64,7 @@ PersistenceMessageTrackerImpl::updateDB()
     if (_cancel_scope) {
         if (_cancel_scope->fully_cancelled()) {
             return; // Fully cancelled ops cannot mutate the DB at all
-        } else {
+        } else if (_cancel_scope->partially_cancelled()) {
             prune_cancelled_nodes_if_present(_bucketInfo, *_cancel_scope);
             prune_cancelled_nodes_if_present(_remapBucketInfo, *_cancel_scope);
         }