From dc187ce6e81e1982d965d6fd8c6659bec916fe29 Mon Sep 17 00:00:00 2001
From: Marek Siarkowicz <siarkowicz@google.com>
Date: Wed, 10 Apr 2024 20:52:54 +0200
Subject: [PATCH] Validate bookmarkable checks the last event before progress
 notify

Signed-off-by: Marek Siarkowicz <siarkowicz@google.com>
---
 tests/robustness/validate/validate_test.go | 15 ++++-------
 tests/robustness/validate/watch.go         | 31 ++++++++++++++++++----
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/tests/robustness/validate/validate_test.go b/tests/robustness/validate/validate_test.go
index c77797948da..6aa6d0f695f 100644
--- a/tests/robustness/validate/validate_test.go
+++ b/tests/robustness/validate/validate_test.go
@@ -493,8 +493,7 @@ func TestValidateWatch(t *testing.T) {
 				putPersistedEvent("b", "2", 3, true),
 				putPersistedEvent("c", "3", 4, true),
 			},
-			// TODO: Should fail as it should guarantee that all events between requested revision and bookmark are delivered
-			expectError: "",
+			expectError: errBrokeBookmarkable.Error(),
 		},
 		{
 			name: "Bookmarkable - revision non decreasing - pass",
@@ -583,8 +582,7 @@ func TestValidateWatch(t *testing.T) {
 				putPersistedEvent("b", "2", 3, true),
 				putPersistedEvent("c", "3", 4, true),
 			},
-			// TODO: This should fail as bookmark lowered revision
-			expectError: "",
+			expectError: errBrokeBookmarkable.Error(),
 		},
 		{
 			name: "Bookmarkable - progress precedes event - fail",
@@ -653,8 +651,7 @@ func TestValidateWatch(t *testing.T) {
 				putPersistedEvent("b", "2", 3, true),
 				putPersistedEvent("c", "3", 4, true),
 			},
-			// TODO: This should fail as there is a missing event before bookmark
-			expectError: "",
+			expectError: errBrokeBookmarkable.Error(),
 		},
 		{
 			name: "Bookmarkable - missing event matching watch before bookmark - fail",
@@ -685,8 +682,7 @@ func TestValidateWatch(t *testing.T) {
 				putPersistedEvent("a", "2", 3, false),
 				putPersistedEvent("c", "3", 4, true),
 			},
-			// TODO: This should fail as there is a missing event before bookmark
-			expectError: "",
+			expectError: errBrokeBookmarkable.Error(),
 		},
 		{
 			name: "Bookmarkable - missing event matching watch with prefix before bookmark - fail",
@@ -718,8 +714,7 @@ func TestValidateWatch(t *testing.T) {
 				putPersistedEvent("ab", "2", 3, true),
 				putPersistedEvent("cc", "3", 4, true),
 			},
-			// TODO: This should fail as there is a missing event before bookmark
-			expectError: "",
+			expectError: errBrokeBookmarkable.Error(),
 		},
 		{
 			name: "Reliable - all events history - pass",
diff --git a/tests/robustness/validate/watch.go b/tests/robustness/validate/watch.go
index 2822af4e02d..22c3833835f 100644
--- a/tests/robustness/validate/watch.go
+++ b/tests/robustness/validate/watch.go
@@ -17,6 +17,7 @@ package validate
 import (
 	"errors"
 
+	"github.com/google/go-cmp/cmp"
 	"go.uber.org/zap"
 
 	"go.etcd.io/etcd/tests/v3/robustness/model"
@@ -55,11 +56,11 @@ func validateWatch(lg *zap.Logger, cfg Config, reports []report.ClientReport, ev
 		if err != nil {
 			return err
 		}
-		err = validateBookmarkable(lg, r)
-		if err != nil {
-			return err
-		}
 		if eventHistory != nil {
+			err = validateBookmarkable(lg, eventHistory, r)
+			if err != nil {
+				return err
+			}
 			err = validateReliable(lg, eventHistory, r)
 			if err != nil {
 				return err
@@ -95,17 +96,37 @@ func validateFilter(lg *zap.Logger, report report.ClientReport) (err error) {
 	return err
 }
 
-func validateBookmarkable(lg *zap.Logger, report report.ClientReport) (err error) {
+func validateBookmarkable(lg *zap.Logger, eventHistory []model.PersistedEvent, report report.ClientReport) (err error) {
 	for _, op := range report.Watch {
 		var lastProgressNotifyRevision int64
+		var gotEventBeforeProgressNotify *model.PersistedEvent
 		for _, resp := range op.Responses {
 			for _, event := range resp.Events {
 				if event.Revision <= lastProgressNotifyRevision {
 					lg.Error("Broke watch guarantee", zap.String("guarantee", "bookmarkable"), zap.Int("client", report.ClientID), zap.Int64("revision", event.Revision))
 					err = errBrokeBookmarkable
 				}
+				gotEventBeforeProgressNotify = &event.PersistedEvent
 			}
 			if resp.IsProgressNotify {
+				if gotEventBeforeProgressNotify != nil || op.Request.Revision != 0 {
+					var wantEventBeforeProgressNotify *model.PersistedEvent
+					for _, ev := range eventHistory {
+						if ev.Revision < op.Request.Revision {
+							continue
+						}
+						if ev.Revision > resp.Revision {
+							break
+						}
+						if ev.Match(op.Request) {
+							wantEventBeforeProgressNotify = &ev
+						}
+					}
+					if diff := cmp.Diff(wantEventBeforeProgressNotify, gotEventBeforeProgressNotify); diff != "" {
+						lg.Error("Broke watch guarantee", zap.String("guarantee", "bookmarkable"), zap.Int("client", report.ClientID), zap.String("diff", diff))
+						err = errBrokeBookmarkable
+					}
+				}
 				lastProgressNotifyRevision = resp.Revision
 			}
 		}