Propagate tombstone info through protocol serialization

For older versions that don't understand the explicit presence
of tombstones for GetReply, make it appear as if the reply has
simply returned a Not Found response. This makes behavior unchanged
in an upgrade scenario.

6 files changed, 72 insertions, 14 deletions

diff --git a/storageapi/src/tests/mbusprot/storageprotocoltest.cpp b/storageapi/src/tests/mbusprot/storageprotocoltest.cpp
index 0f628f59aac..37159ab0011 100644
--- a/storageapi/src/tests/mbusprot/storageprotocoltest.cpp
+++ b/storageapi/src/tests/mbusprot/storageprotocoltest.cpp
@@ -291,6 +291,7 @@ TEST_P(StorageProtocolTest, get) {
     EXPECT_EQ(_testDoc->getId(), reply2->getDocumentId());
     EXPECT_EQ(Timestamp(123), reply2->getBeforeTimestamp());
     EXPECT_EQ(Timestamp(100), reply2->getLastModifiedTimestamp());
+    EXPECT_FALSE(reply2->is_tombstone());
     EXPECT_NO_FATAL_FAILURE(assert_bucket_info_reply_fields_propagated(*reply2));
 }
 
@@ -316,6 +317,40 @@ TEST_P(StorageProtocolTest, can_set_internal_read_consistency_on_get_commands) {
     EXPECT_EQ(cmd2->internal_read_consistency(), InternalReadConsistency::Strong);
 }
 
+TEST_P(StorageProtocolTest, tombstones_propagated_for_gets) {
+    // Only supported on protocol version 7+.
+    if (GetParam().getMajor() < 7) {
+        return;
+    }
+    auto cmd = std::make_shared<GetCommand>(_bucket, _testDocId, "foo,bar", 123);
+    auto reply = std::make_shared<GetReply>(*cmd, std::shared_ptr<Document>(), 100, false, true);
+    set_dummy_bucket_info_reply_fields(*reply);
+    auto reply2 = copyReply(reply);
+
+    EXPECT_TRUE(reply2->getDocument().get() == nullptr);
+    EXPECT_EQ(_testDoc->getId(), reply2->getDocumentId());
+    EXPECT_EQ(Timestamp(123), reply2->getBeforeTimestamp());
+    EXPECT_EQ(Timestamp(100), reply2->getLastModifiedTimestamp()); // In this case, the tombstone timestamp.
+    EXPECT_TRUE(reply2->is_tombstone());
+}
+
+// TODO remove this once pre-protobuf serialization is removed
+TEST_P(StorageProtocolTest, old_serialization_format_treats_tombstone_get_replies_as_not_found) {
+    if (GetParam().getMajor() >= 7) {
+        return;
+    }
+    auto cmd = std::make_shared<GetCommand>(_bucket, _testDocId, "foo,bar", 123);
+    auto reply = std::make_shared<GetReply>(*cmd, std::shared_ptr<Document>(), 100, false, true);
+    set_dummy_bucket_info_reply_fields(*reply);
+    auto reply2 = copyReply(reply);
+
+    EXPECT_TRUE(reply2->getDocument().get() == nullptr);
+    EXPECT_EQ(_testDoc->getId(), reply2->getDocumentId());
+    EXPECT_EQ(Timestamp(123), reply2->getBeforeTimestamp());
+    EXPECT_EQ(Timestamp(0), reply2->getLastModifiedTimestamp());
+    EXPECT_FALSE(reply2->is_tombstone()); // Protocol version doesn't understand explicit tombstones.
+}
+
 TEST_P(StorageProtocolTest, remove) {
     auto cmd = std::make_shared<RemoveCommand>(_bucket, _testDocId, 159);
     auto cmd2 = copyCommand(cmd);
diff --git a/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto b/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto
index 12dbaf59146..f38c5bfb56a 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto
+++ b/storageapi/src/vespa/storageapi/mbusprot/protobuf/feed.proto
@@ -73,6 +73,9 @@ message GetResponse {
     uint64     last_modified_timestamp = 2;
     BucketInfo bucket_info             = 3;
     BucketId   remapped_bucket_id      = 4;
+    // Note: last_modified_timestamp and tombstone_timestamp are mutually exclusive.
+    // Tracked separately (rather than being a flag bool) to avoid issues during rolling upgrades.
+    uint64     tombstone_timestamp     = 5;
 }
 
 message RevertRequest {
diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp
index aeb6d382997..8a0204c8fc9 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp
+++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization5_0.cpp
@@ -168,7 +168,8 @@ ProtocolSerialization5_0::onDecodeUpdateReply(const SCmd& cmd, BBuf& buf) const
 void ProtocolSerialization5_0::onEncode(GBBuf& buf, const api::GetReply& msg) const
 {
     SH::putDocument(msg.getDocument().get(), buf);
-    buf.putLong(msg.getLastModifiedTimestamp());
+    // Old protocol version doesn't understand tombstones. Make it appear as Not Found.
+    buf.putLong(msg.is_tombstone() ? api::Timestamp(0) : msg.getLastModifiedTimestamp());
     onEncodeBucketInfoReply(buf, msg);
 }
 
diff --git a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp
index 90c8d1c7d2a..8ea946eede4 100644
--- a/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp
+++ b/storageapi/src/vespa/storageapi/mbusprot/protocolserialization7.cpp
@@ -567,7 +567,17 @@ void ProtocolSerialization7::onEncode(GBBuf& buf, const api::GetReply& msg) cons
         if (msg.getDocument()) {
             set_document(*res.mutable_document(), *msg.getDocument());
         }
-        res.set_last_modified_timestamp(msg.getLastModifiedTimestamp());
+        if (!msg.is_tombstone()) {
+            res.set_last_modified_timestamp(msg.getLastModifiedTimestamp());
+        } else {
+            // This field will be ignored by older versions, making the behavior as if
+            // a timestamp of zero was returned for tombstones, as it the legacy behavior.
+            res.set_tombstone_timestamp(msg.getLastModifiedTimestamp());
+            // Will not be encoded onto the wire, but we include it here to hammer down the
+            // point that it's intentional to have the last modified time appear as a not
+            // found document for older versions.
+            res.set_last_modified_timestamp(0);
+        }
     });
 }
 
@@ -585,8 +595,12 @@ api::StorageReply::UP ProtocolSerialization7::onDecodeGetReply(const SCmd& cmd,
     return decode_bucket_info_response<protobuf::GetResponse>(buf, [&](auto& res) {
         try {
             auto document = get_document(res.document(), type_repo());
+            const bool is_tombstone = (res.tombstone_timestamp() != 0);
+            const auto effective_timestamp = (is_tombstone ? res.tombstone_timestamp()
+                                                           : res.last_modified_timestamp());
             return std::make_unique<api::GetReply>(static_cast<const api::GetCommand&>(cmd),
-                                                   std::move(document), res.last_modified_timestamp());
+                                                   std::move(document), effective_timestamp,
+                                                   false, is_tombstone);
         } catch (std::exception& e) {
             auto reply = std::make_unique<api::GetReply>(static_cast<const api::GetCommand&>(cmd),
                                                          std::shared_ptr<document::Document>(), 0u);
diff --git a/storageapi/src/vespa/storageapi/message/persistence.cpp b/storageapi/src/vespa/storageapi/message/persistence.cpp
index 7fd789a8c81..af0b7ae0288 100644
--- a/storageapi/src/vespa/storageapi/message/persistence.cpp
+++ b/storageapi/src/vespa/storageapi/message/persistence.cpp
@@ -211,14 +211,16 @@ GetCommand::print(std::ostream& out, bool verbose, const std::string& indent) co
 GetReply::GetReply(const GetCommand& cmd,
                    const DocumentSP& doc,
                    Timestamp lastModified,
-                   bool had_consistent_replicas)
+                   bool had_consistent_replicas,
+                   bool is_tombstone)
     : BucketInfoReply(cmd),
       _docId(cmd.getDocumentId()),
       _fieldSet(cmd.getFieldSet()),
       _doc(doc),
       _beforeTimestamp(cmd.getBeforeTimestamp()),
       _lastModifiedTime(lastModified),
-      _had_consistent_replicas(had_consistent_replicas)
+      _had_consistent_replicas(had_consistent_replicas),
+      _is_tombstone(is_tombstone)
 {
 }
 
diff --git a/storageapi/src/vespa/storageapi/message/persistence.h b/storageapi/src/vespa/storageapi/message/persistence.h
index e6fe2b6dae5..24601803266 100644
--- a/storageapi/src/vespa/storageapi/message/persistence.h
+++ b/storageapi/src/vespa/storageapi/message/persistence.h
@@ -224,24 +224,27 @@ class GetReply : public BucketInfoReply {
     Timestamp _beforeTimestamp;
     Timestamp _lastModifiedTime;
     bool _had_consistent_replicas;
-
+    bool _is_tombstone;
 public:
-    GetReply(const GetCommand& cmd,
-             const DocumentSP& doc = DocumentSP(),
-             Timestamp lastModified = 0,
-             bool had_consistent_replicas = false);
+    explicit GetReply(const GetCommand& cmd,
+                      const DocumentSP& doc = DocumentSP(),
+                      Timestamp lastModified = 0,
+                      bool had_consistent_replicas = false,
+                      bool is_tombstone = false);
+
     ~GetReply() override;
 
     const DocumentSP& getDocument() const { return _doc; }
     const document::DocumentId& getDocumentId() const { return _docId; }
     const vespalib::string& getFieldSet() const { return _fieldSet; }
 
-    Timestamp getLastModifiedTimestamp() const { return _lastModifiedTime; }
-    Timestamp getBeforeTimestamp() const { return _beforeTimestamp; }
+    Timestamp getLastModifiedTimestamp() const noexcept { return _lastModifiedTime; }
+    Timestamp getBeforeTimestamp() const noexcept { return _beforeTimestamp; }
 
-    bool had_consistent_replicas() const noexcept { return _had_consistent_replicas; }
+    [[nodiscard]] bool had_consistent_replicas() const noexcept { return _had_consistent_replicas; }
+    [[nodiscard]] bool is_tombstone() const noexcept { return _is_tombstone; }
 
-    bool wasFound() const { return (_doc.get() != 0); }
+    bool wasFound() const { return (_doc.get() != nullptr); }
     void print(std::ostream& out, bool verbose, const std::string& indent) const override;
     DECLARE_STORAGEREPLY(GetReply, onGetReply)
 };