From 48b879d31bf504f594b96227a05bc1e576797c03 Mon Sep 17 00:00:00 2001 From: Gregory Newman-Smith Date: Fri, 15 Aug 2025 11:40:51 +0100 Subject: [PATCH 1/5] CBG-4806: move conflict detection to common function to be shared between areas --- db/crud.go | 6 ++-- db/hybrid_logical_vector.go | 49 +++++++++++++++++++++++++++ rest/utilities_testing_blip_client.go | 28 +++------------ 3 files changed, 57 insertions(+), 26 deletions(-) diff --git a/db/crud.go b/db/crud.go index 171f2f5406..8c4ab80c7a 100644 --- a/db/crud.go +++ b/db/crud.go @@ -1267,6 +1267,7 @@ func (db *DatabaseCollectionWithUser) PutExistingCurrentVersion(ctx context.Cont revTreeConflictChecked := false var parent string var currentRevIndex int + var isConflict bool // Conflict check here // if doc has no HLV defined this is a new doc we haven't seen before, skip conflict check @@ -1277,11 +1278,12 @@ func (db *DatabaseCollectionWithUser) PutExistingCurrentVersion(ctx context.Cont return nil, nil, false, nil, addNewerVersionsErr } } else { - if doc.HLV.isDominating(newDocHLV) { + isConflict, err = IsInConflict(ctx, doc.HLV, newDocHLV) + if err != nil && errors.Is(err, ErrNoNewVersionsToAdd) { base.DebugfCtx(ctx, base.KeyCRUD, "PutExistingCurrentVersion(%q): No new versions to add. existing: %#v new:%#v", base.UD(newDoc.ID), doc.HLV, newDocHLV) return nil, nil, false, nil, base.ErrUpdateCancel // No new revisions to add } - if newDocHLV.isDominating(doc.HLV) { + if !isConflict { // update hlv for all newer incoming source version pairs addNewerVersionsErr := doc.HLV.AddNewerVersions(newDocHLV) if addNewerVersionsErr != nil { diff --git a/db/hybrid_logical_vector.go b/db/hybrid_logical_vector.go index 761eb4ae2d..ef6a53d5df 100644 --- a/db/hybrid_logical_vector.go +++ b/db/hybrid_logical_vector.go @@ -9,7 +9,9 @@ package db import ( + "context" "encoding/base64" + "errors" "fmt" "maps" "sort" @@ -689,3 +691,50 @@ func (hlv *HybridLogicalVector) UnmarshalJSON(inputjson []byte) error { func (hlv HybridLogicalVector) GoString() string { return fmt.Sprintf("HybridLogicalVector{CurrentVersionCAS:%d, SourceID:%s, Version:%d, PreviousVersions:%#+v, MergeVersions:%#+v}", hlv.CurrentVersionCAS, hlv.SourceID, hlv.Version, hlv.PreviousVersions, hlv.MergeVersions) } + +// ErrNoNewVersionsToAdd will be thrown when there are no new versions from incoming HLV to be added to HLV that is local +var ErrNoNewVersionsToAdd = errors.New("no new versions to add to HLV") + +// IsInConflict is used to identify if two HLV's are in conflict or not. Will return boolean to indicate if in conflict +// or not and will error for the following cases: +// - Local HLV dominates incoming HLV (meaning local version is a newer version that the incoming one) +// - Local CV matches incoming CV, so no new versions to add +func IsInConflict(ctx context.Context, localHLV, incomingHLV *HybridLogicalVector) (bool, error) { + incomingCV := incomingHLV.ExtractCurrentVersionFromHLV() + localCV := localHLV.ExtractCurrentVersionFromHLV() + + // check if incoming CV and local CV are the same. This is needed here given that if both CV's are the same the + // below check to check local revision is newer than incoming revision will pass given the check and will add the + // incoming versions even if CV's are the same + if localCV.Equal(*incomingCV) { + base.DebugfCtx(ctx, base.KeyCRUD, "incoming CV %#+v is equal to local revision %#+v", incomingCV, localCV) + return false, ErrNoNewVersionsToAdd + } + + // standard no conflict case. In the simple case, this happens when: + // - SG writes document 1@cbs1 + // - CBL pulls document 1@cbs1 + // - SG writes document 2@cbs1 + if incomingHLV.DominatesSource(*localCV) { + return false, nil + } + + // local revision is newer than incoming revision. Common case: + // - CBL writes document 1@cbl1 + // - CBL pushes to SG as 1@cbl1 + // - CBL pulls document 1@cbl1 + // + // NOTE: without P2P replication, this should not be the case and we would not get this revision, since CBL + // would respond to a SG changes message that CBL does not need this revision + if localHLV.DominatesSource(*incomingCV) { + return false, ErrNoNewVersionsToAdd + } + // Check if conflict has been previously resolved. + // - If merge versions are empty, then it has not be resolved. + // - If merge versions do not match, then it has not been resolved. + if len(incomingHLV.MergeVersions) != 0 && len(localHLV.MergeVersions) != 0 && maps.Equal(incomingHLV.MergeVersions, localHLV.MergeVersions) { + base.DebugfCtx(ctx, base.KeyVV, "merge versions match between local HLV %#v and incoming HLV %#v, conflict previously resolved", localHLV, incomingCV) + return false, nil + } + return true, nil +} diff --git a/rest/utilities_testing_blip_client.go b/rest/utilities_testing_blip_client.go index bd29626ed5..a8a92733e7 100644 --- a/rest/utilities_testing_blip_client.go +++ b/rest/utilities_testing_blip_client.go @@ -16,7 +16,6 @@ import ( "encoding/base64" "fmt" "iter" - "maps" "net/http" "slices" "strconv" @@ -325,35 +324,16 @@ func (cd *clientDoc) _hasConflict(t testing.TB, incomingHLV *db.HybridLogicalVec incomingCV := incomingHLV.ExtractCurrentVersionFromHLV() localCV := localHLV.ExtractCurrentVersionFromHLV() // safety check - ensure SG is not sending a rev that we already had - ensures changes feed messaging is working correctly to prevent + // this check is also performed in the function below but we should keep this here for extra context on FailNow call if localCV.Equal(*incomingCV) { require.FailNow(t, fmt.Sprintf("incoming CV %#+v is equal to local revision %#+v - this should've been filtered via changes response before ending up as a rev. This is only true if there is a single replication occurring, two simultaneous replications (e.g. P2P) could cause this. If there are multiple replications, modify code.", incomingCV, latestRev)) } - // standard no conflict case. In the simple case, this happens when: - // - SG writes document 1@cbs1 - // - CBL pulls document 1@cbs1 - // - SG writes document 2@cbs1 - if incomingHLV.DominatesSource(*localCV) { - return false - } - // local revision is newer than incoming revision. Common case: - // - CBL writes document 1@cbl1 - // - CBL pushes to SG as 1@cbl1 - // - CBL pulls document 1@cbl1 - // - // NOTE: without P2P replication, this should not be the case and we would not get this revision, since CBL - // would respond to a SG changes message that CBL does not need this revision - if localHLV.DominatesSource(*incomingCV) { + inConflict, err := db.IsInConflict(nil, &localHLV, incomingHLV) + if err != nil { require.FailNow(t, fmt.Sprintf("incoming CV %#+v has lower version than the local revision %#+v - this should've been filtered via changes response before ending up as a rev. blip tester would reply that to Sync Gateway that it doesn't need this revision", incomingCV, localHLV)) - return false - } - // Check if conflict has been previously resolved. - // - If merge versions are empty, then it has not be resolved. - // - If merge versions do not match, then it has not been resolved. - if len(incomingHLV.MergeVersions) != 0 && len(localHLV.MergeVersions) != 0 && maps.Equal(incomingHLV.MergeVersions, localHLV.MergeVersions) { - return false } - return true + return inConflict } func (btcc *BlipTesterCollectionClient) _resolveConflict(incomingHLV *db.HybridLogicalVector, incomingBody []byte, localDoc *clientDocRev) (body []byte, hlv db.HybridLogicalVector) { From 8bcf2b79c52de4100e144241dcd62a81030909bb Mon Sep 17 00:00:00 2001 From: Gregory Newman-Smith Date: Fri, 15 Aug 2025 13:01:39 +0100 Subject: [PATCH 2/5] add test for IsInConflict function --- db/hybrid_logical_vector_test.go | 48 ++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/db/hybrid_logical_vector_test.go b/db/hybrid_logical_vector_test.go index b57a0a54e4..ae58d9e944 100644 --- a/db/hybrid_logical_vector_test.go +++ b/db/hybrid_logical_vector_test.go @@ -1413,3 +1413,51 @@ func TestAddVersion(t *testing.T) { }) } } + +func TestIsInConflict(t *testing.T) { + testCases := []struct { + name string + localHLV string + incomingHLV string + expectedInConflict bool + expectedError bool + }{ + { + name: "CV equal", + localHLV: "111@abc;123@def", + incomingHLV: "111@abc;123@ghi", + expectedError: true, + }, + { + name: "no conflict case", + localHLV: "111@abc;123@def", + incomingHLV: "112@abc;123@ghi", + }, + { + name: "local revision is newer", + localHLV: "111@abc;123@def", + incomingHLV: "100@abc;123@ghi", + expectedError: true, + }, + { + name: "merge versions match", + localHLV: "130@abc,123@def,100@ghi;50@jkl", + incomingHLV: "150@mno,123@def,100@ghi;50@jkl", + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + localHLV, _, err := extractHLVFromBlipString(tc.localHLV) + require.NoError(t, err) + incomingHLV, _, err := extractHLVFromBlipString(tc.incomingHLV) + + inConflict, err := IsInConflict(t.Context(), localHLV, incomingHLV) + if tc.expectedError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + require.Equal(t, tc.expectedInConflict, inConflict) + }) + } +} From 954d5a5c83797adfee5239e12bffee8bd220b6e7 Mon Sep 17 00:00:00 2001 From: Gregory Newman-Smith Date: Fri, 15 Aug 2025 13:10:48 +0100 Subject: [PATCH 3/5] lint issue --- db/hybrid_logical_vector.go | 6 +++--- db/hybrid_logical_vector_test.go | 11 ++++++----- rest/utilities_testing_blip_client.go | 2 +- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/db/hybrid_logical_vector.go b/db/hybrid_logical_vector.go index ef6a53d5df..a2eed9e25a 100644 --- a/db/hybrid_logical_vector.go +++ b/db/hybrid_logical_vector.go @@ -693,12 +693,12 @@ func (hlv HybridLogicalVector) GoString() string { } // ErrNoNewVersionsToAdd will be thrown when there are no new versions from incoming HLV to be added to HLV that is local -var ErrNoNewVersionsToAdd = errors.New("no new versions to add to HLV") +var ErrNoNewVersionsToAdd = errors.New("no new versions to add to HLV") // IsInConflict is used to identify if two HLV's are in conflict or not. Will return boolean to indicate if in conflict // or not and will error for the following cases: -// - Local HLV dominates incoming HLV (meaning local version is a newer version that the incoming one) -// - Local CV matches incoming CV, so no new versions to add +// - Local HLV dominates incoming HLV (meaning local version is a newer version that the incoming one) +// - Local CV matches incoming CV, so no new versions to add func IsInConflict(ctx context.Context, localHLV, incomingHLV *HybridLogicalVector) (bool, error) { incomingCV := incomingHLV.ExtractCurrentVersionFromHLV() localCV := localHLV.ExtractCurrentVersionFromHLV() diff --git a/db/hybrid_logical_vector_test.go b/db/hybrid_logical_vector_test.go index ae58d9e944..7ea04d8c87 100644 --- a/db/hybrid_logical_vector_test.go +++ b/db/hybrid_logical_vector_test.go @@ -1434,14 +1434,14 @@ func TestIsInConflict(t *testing.T) { incomingHLV: "112@abc;123@ghi", }, { - name: "local revision is newer", - localHLV: "111@abc;123@def", - incomingHLV: "100@abc;123@ghi", + name: "local revision is newer", + localHLV: "111@abc;123@def", + incomingHLV: "100@abc;123@ghi", expectedError: true, }, { - name: "merge versions match", - localHLV: "130@abc,123@def,100@ghi;50@jkl", + name: "merge versions match", + localHLV: "130@abc,123@def,100@ghi;50@jkl", incomingHLV: "150@mno,123@def,100@ghi;50@jkl", }, } @@ -1450,6 +1450,7 @@ func TestIsInConflict(t *testing.T) { localHLV, _, err := extractHLVFromBlipString(tc.localHLV) require.NoError(t, err) incomingHLV, _, err := extractHLVFromBlipString(tc.incomingHLV) + require.NoError(t, err) inConflict, err := IsInConflict(t.Context(), localHLV, incomingHLV) if tc.expectedError { diff --git a/rest/utilities_testing_blip_client.go b/rest/utilities_testing_blip_client.go index a8a92733e7..b60e221605 100644 --- a/rest/utilities_testing_blip_client.go +++ b/rest/utilities_testing_blip_client.go @@ -329,7 +329,7 @@ func (cd *clientDoc) _hasConflict(t testing.TB, incomingHLV *db.HybridLogicalVec require.FailNow(t, fmt.Sprintf("incoming CV %#+v is equal to local revision %#+v - this should've been filtered via changes response before ending up as a rev. This is only true if there is a single replication occurring, two simultaneous replications (e.g. P2P) could cause this. If there are multiple replications, modify code.", incomingCV, latestRev)) } - inConflict, err := db.IsInConflict(nil, &localHLV, incomingHLV) + inConflict, err := db.IsInConflict(t.Context(), &localHLV, incomingHLV) if err != nil { require.FailNow(t, fmt.Sprintf("incoming CV %#+v has lower version than the local revision %#+v - this should've been filtered via changes response before ending up as a rev. blip tester would reply that to Sync Gateway that it doesn't need this revision", incomingCV, localHLV)) } From fe8e72d112b8f534ef8b57d21e34c45f04b614a5 Mon Sep 17 00:00:00 2001 From: Gregory Newman-Smith Date: Fri, 15 Aug 2025 14:16:38 +0100 Subject: [PATCH 4/5] updates based off review --- db/hybrid_logical_vector.go | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/db/hybrid_logical_vector.go b/db/hybrid_logical_vector.go index a2eed9e25a..170b5fc162 100644 --- a/db/hybrid_logical_vector.go +++ b/db/hybrid_logical_vector.go @@ -705,24 +705,25 @@ func IsInConflict(ctx context.Context, localHLV, incomingHLV *HybridLogicalVecto // check if incoming CV and local CV are the same. This is needed here given that if both CV's are the same the // below check to check local revision is newer than incoming revision will pass given the check and will add the - // incoming versions even if CV's are the same + // incoming versions even if CVs are the same if localCV.Equal(*incomingCV) { - base.DebugfCtx(ctx, base.KeyCRUD, "incoming CV %#+v is equal to local revision %#+v", incomingCV, localCV) + base.TracefCtx(ctx, base.KeyVV, "incoming CV %#+v is equal to local revision %#+v", incomingCV, localCV) return false, ErrNoNewVersionsToAdd } // standard no conflict case. In the simple case, this happens when: - // - SG writes document 1@cbs1 - // - CBL pulls document 1@cbs1 - // - SG writes document 2@cbs1 + // - Client A writes document 1@cbs1 + // - Client B pulls document 1@cbs1 from Client A + // - Client A writes document 2@cbs1 + // - Client B pulls document 2@cbs1 from Client A if incomingHLV.DominatesSource(*localCV) { return false, nil } // local revision is newer than incoming revision. Common case: - // - CBL writes document 1@cbl1 - // - CBL pushes to SG as 1@cbl1 - // - CBL pulls document 1@cbl1 + // - Client A writes document 1@cbl1 + // - Client A pushes to Client B as 1@cbl1 + // - Client A pulls document 1@cbl1 from Client B // // NOTE: without P2P replication, this should not be the case and we would not get this revision, since CBL // would respond to a SG changes message that CBL does not need this revision From 4feffe78da4324473d7a5815546b0107c56db464 Mon Sep 17 00:00:00 2001 From: Gregory Newman-Smith Date: Fri, 15 Aug 2025 14:34:12 +0100 Subject: [PATCH 5/5] update comments to remove reference to CBL and SG peers --- db/hybrid_logical_vector.go | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/db/hybrid_logical_vector.go b/db/hybrid_logical_vector.go index 170b5fc162..7dc4068565 100644 --- a/db/hybrid_logical_vector.go +++ b/db/hybrid_logical_vector.go @@ -712,21 +712,21 @@ func IsInConflict(ctx context.Context, localHLV, incomingHLV *HybridLogicalVecto } // standard no conflict case. In the simple case, this happens when: - // - Client A writes document 1@cbs1 - // - Client B pulls document 1@cbs1 from Client A - // - Client A writes document 2@cbs1 - // - Client B pulls document 2@cbs1 from Client A + // - Client A writes document 1@srcA + // - Client B pulls document 1@srcA from Client A + // - Client A writes document 2@srcA + // - Client B pulls document 2@srcA from Client A if incomingHLV.DominatesSource(*localCV) { return false, nil } // local revision is newer than incoming revision. Common case: - // - Client A writes document 1@cbl1 - // - Client A pushes to Client B as 1@cbl1 - // - Client A pulls document 1@cbl1 from Client B + // - Client A writes document 1@srcA + // - Client A pushes to Client B as 1@srcA + // - Client A pulls document 1@srcA from Client B // - // NOTE: without P2P replication, this should not be the case and we would not get this revision, since CBL - // would respond to a SG changes message that CBL does not need this revision + // NOTE: without P2P replication, this should not be the case and we would not get this revision, since Client A + // would respond to a Client B changes message that Client A does not need this revision if localHLV.DominatesSource(*incomingCV) { return false, ErrNoNewVersionsToAdd }