CBL-7841: Handle conflict when revision history appears broken by jianminzhao · Pull Request #2489 · couchbase/couchbase-lite-core

jianminzhao · 2026-05-15T21:34:58Z

This is a fallback for cases where the general comparison algorithm yields a Conflict while applying a remote revision to the local document. The target scenario is a deleted document being resurrected in CBS -- when that happens, the resurrected document has no history prior to the deletion.

Copilot

Pull request overview

Adds a fallback in putExistingRevision to recover from cases where a pulled revision conflicts with the local revision because the remote's history appears to have been reset (e.g. a deleted doc being resurrected in Couchbase Server with the same ID). When the general VV comparison yields kConflicting, the code loads the last revision known to be shared with that remote and, if that landmark is the same as or newer than the current local revision, treats the incoming revision as kNewer instead of a conflict.

Changes:

Detect the "broken history" case after a kConflicting result from VersionVecWithLegacy::compare.
Use _doc.loadRemoteRevision(remote) as a landmark; promote order to kNewer when the landmark is same/newer than curVers.
Add an extensive comment block documenting the reasoning behind the fallback.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

This is a fallback for cases where the general comparison algorithm yields a Conflict while applying a remote revision to the local document. The target scenario is a deleted document being resurrected in CBS -- when that happens, the resurrected document has no history prior to the deletion.

cbl-bot · 2026-05-15T22:11:19Z

Code Coverage Results:

Type	Percentage
branches	64.61
functions	77.37
instantiations	70.95
lines	76.01
regions	72.3

snej · 2026-05-18T18:15:24Z

+                // CBL-7841: Handle the case where revision history may appear broken.
+                //


Having a detailed explanation is good, but this is already quite a long method. One thing I've done before is to make the explanation a "footnote" at the end of the file, with a one-line reference to it inline.

I will put the comments in the CBL ticket, and leave a short note in the source.

snej · 2026-05-18T18:32:02Z

+                        if ( orderWithLandmark == kSame || orderWithLandmark == kNewer ) order = kNewer;
+                    }


As I understand it, this says "if the last known rev at the remote was the same or newer than the local rev, treat the remote's new rev as also newer than the local rev, instead of as a conflict."

I think this is too general. This situation isn't the same as the rev-tree "branch switch". It's specifically that the remote has an entirely new history, because Couchbase Server recreated the doc from scratch after it was deleted. We can detect that by checking whether rev and newVers have no Versions in common, and also the local revision is a tombstone. What do you think?

I did not find specific check of "the local revision is a tombstone". Did I miss the check somewhere?

I think whether we accept the newVers only if the curVers is a tombstone is a decision by its own right, provided we already decided that "Couchbase Server recreated the doc from scratch." I tried to create the the doc in Couchbase Server after I deleted in Lite and syned to the SGW. There is no friction in the way. I also tried to create the doc in CB after the doc is syned to SGW before deleted. In this case CB won't let me do it because the doc already exists. I did not try to create the doc in CB first before CB sees it. This would simulate the case where a doc with same docID is created in Lite and CB concurrently, and we will encounter the same situation where the latestSynedInRemote shares no versions at at all. The only difference is whether curVers is a tombstone or not.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.

+                    // 6. curVers.legacy[0] is not in newVers.legacy (RevTree parent)
+                    auto isCurVersLegacyParentOfNewVers = [&]() {
+                        if ( curVers.legacy.empty() ) return false;
+                        return std::ranges::find(newVers.legacy, curVers.legacy[0]) != curVers.legacy.end();


+                // CBL-7841: Handle the case where revision history may appear broken.
+                //
+                // Concrete scenario for a single document ID:
+                //   1. Local DB deletes the doc.
+                //   2. Local pushes the tombstone to a remote SGW.
+                //   3. In the target CB database, a new doc is created with the same ID.
+                //      (In CBS, a deleted doc is effectively purged, so the ID is free
+                //      for reuse by an unrelated document.)
+                //   4. Local DB performs a pull.
+                //
+                // At step 4 the BLIP message carries a document that starts at a new
+                // revision with no history. In this case, this new revision will be in
+                // conflict with any revisions that exist before its birth. However, we
+                // would like to make it new current, replacing the already deleted Local.
+


The criteria of this comparion is curVers.legacy[0] is-not-contained-in newVers.legacy meaning to assert that curVers is not a parent of newVers.

snej · 2026-05-22T16:12:27Z

+                // 5. curVers.vector shares no version with newVers.vector
+                // 6. curVers.legacy[0] is not in newVers.legacy (RevTree parent)
+
+                for ( bool goOn = order == kConflicting && remote != RemoteID::Local; goOn; goOn = false ) {


This is confusing. It is effectively an if, since it never runs more than once, but I'm guessing you made it a for so you could use break to exit it.
In my experience, when you have a block with multiple exits like this, it's time to turn it into a function. Or if that would add too many parameters to pass, use a do{...}while(false) statement along with a comment that this is just to allowbreak.

snej · 2026-05-22T16:17:08Z

+                    VersionVecWithLegacy lastSynced{rev->revID};
+                    if ( curVers.legacy != lastSynced.legacy || curVers.vector != lastSynced.vector ) break;
+
+                    // 5. curVers.vector shares no version with newVers.vector


This whole thing through line 488 should be a method on VersionVector, not inlined here.

snej · 2026-05-22T16:17:53Z

+
+                    // 5. curVers.vector shares no version with newVers.vector
+                    unordered_map<SourceID, logicalTime>       newVersions;
+                    optional<std::pair<SourceID, logicalTime>> extra;


This pair is identical to Version, so use that instead.

snej · 2026-05-22T16:19:43Z

+                    auto isCurVersLegacyParentOfNewVers = [&]() {
+                        if ( curVers.legacy.empty() ) return false;
+                        return std::ranges::find(newVers.legacy, curVers.legacy[0]) != newVers.legacy.end();
+                    };
+                    if ( isCurVersLegacyParentOfNewVers() ) break;


Why is this a lambda and not just a plain if statement?

CBL-8057: Better stack trace logging and crash handling (#2490) CBL-7841: Handle conflict when revision history appears broken (#2489) CBL-8229: Upgrade SQLite to 3.53.0 (#2473) CBL-8180: Upgrade mbedTLS to 3.6.6 (#2469) EE: c32b01c : Fix build of peer-o-matic tool (#143) CBL-8326: Remove temp error log in TLSCodec auth callback (#142) CBL-8229: Upgrade SQLite to 3.53.0 (#138)

jianminzhao force-pushed the cbl-7841 branch from e9e5484 to e660ebd Compare May 15, 2026 21:39

jianminzhao requested a review from Copilot May 15, 2026 21:56

Copilot started reviewing on behalf of jianminzhao May 15, 2026 21:57 View session

Copilot AI reviewed May 15, 2026

View reviewed changes

Comment thread LiteCore/Database/VectorDocument.cc Outdated

jianminzhao force-pushed the cbl-7841 branch from e660ebd to 139b660 Compare May 15, 2026 22:00

jianminzhao requested review from pasin and snej May 18, 2026 17:02

snej requested changes May 18, 2026

View reviewed changes

jianminzhao force-pushed the cbl-7841 branch from aa77617 to 4343eff Compare May 21, 2026 18:53

jianminzhao requested a review from Copilot May 21, 2026 19:14

Copilot started reviewing on behalf of jianminzhao May 21, 2026 19:15 View session

Copilot AI reviewed May 21, 2026

View reviewed changes

Added comparison between the legacy part of curVers and newVers.

d832baa

The criteria of this comparion is curVers.legacy[0] is-not-contained-in newVers.legacy meaning to assert that curVers is not a parent of newVers.

jianminzhao force-pushed the cbl-7841 branch from 4343eff to d832baa Compare May 21, 2026 19:22

jianminzhao requested a review from snej May 21, 2026 23:18

snej requested changes May 22, 2026

View reviewed changes

incorporate review feedback

1438a10

jianminzhao requested a review from snej May 22, 2026 21:42

snej approved these changes May 22, 2026

View reviewed changes

jianminzhao merged commit ed06469 into master May 22, 2026
11 of 12 checks passed

jianminzhao deleted the cbl-7841 branch May 22, 2026 22:59

		// CBL-7841: Handle the case where revision history may appear broken.
		//

		if ( orderWithLandmark == kSame \|\| orderWithLandmark == kNewer ) order = kNewer;
		}

Conversation

jianminzhao commented May 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

cbl-bot commented May 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cbl-bot commented May 15, 2026 •

edited

Loading