Merge pull request #582 from MerginMaps/fix-rank-merges-on-delta-vibe

Fix rank merges on delta vibe

Author: MarcelGeo

server/mergin/sync/models.py

@@ -69,10 +69,12 @@ class ChangeComparisonAction(Enum):
     """Actions to take when comparing two changes"""
 
     REPLACE = "replace"
-    DELETE = "delete"
-    UPDATE = "update"
-    UPDATE_DIFF = "update_diff"
+    UPDATE_METADATA = "update_metadata"  # Update metadata and keep diffs (used for update + update sequence)
+    REPLACE_DIFFS = "replace_diffs"  # Replace diffs but keep metadata (used for update + update sequence when only diffs are changed)
     EXCLUDE = "exclude"  # Return None to exclude the file
+    FORCE_UPDATE = (
+        "force_update"  # Force update even if it looks like a delete + create sequence
+    )
 
 
 class Project(db.Model):
@@ -1089,9 +1091,11 @@ def merge_changes(
                 continue
 
             # Compare and merge with previous change for this file
-            can_delete = path in updating_files
+            # If file did not exist before this range (first change was CREATE),
+            # a CREATE+DELETE sequence is transparent — exclude the pair.
+            exclude_delete = path not in updating_files
             new_change = ProjectVersionDelta._compare_changes(
-                result[path], current, can_delete
+                result[path], current, exclude_delete
             )
 
             # Update result (or remove if no change is detected)
@@ -1106,45 +1110,54 @@ def merge_changes(
     def _compare_changes(
         previous: DeltaChangeMerged,
         new: DeltaChangeMerged,
-        prevent_delete_change: bool,
+        exclude_delete: bool,
     ) -> Optional[DeltaChangeMerged]:
         """
         Compare and merge two changes for the same file.
 
         Args:
             previous: Previously accumulated change
             new: New change to compare
-            prevent_delete_change: Whether the change can be deleted when resolving create+delete sequences
+            exclude_delete: If True, a CREATE+DELETE pair is excluded (file was
+                created and deleted within the same range — no net effect).
+                If False, the DELETE is kept (file existed before this range
+                and the client needs to remove it).
 
         Returns:
             Merged change or None if file should be excluded
         """
 
         # Map change type pairs to actions
         action_map = {
-            # create + delete = file is transparent for current changes -> delete it
+            # CREATE + DELETE: if file didn't exist before (exclude_delete=True),
+            # the pair cancels out (EXCLUDE). If it did exist, keep the DELETE
+            # so the client removes its local copy.
             (
                 PushChangeType.CREATE,
                 PushChangeType.DELETE,
-            ): ChangeComparisonAction.DELETE,
+            ): (
+                ChangeComparisonAction.EXCLUDE
+                if exclude_delete
+                else ChangeComparisonAction.REPLACE
+            ),
             # create + update = create with updated info
             (
                 PushChangeType.CREATE,
                 PushChangeType.UPDATE,
-            ): ChangeComparisonAction.UPDATE,
+            ): ChangeComparisonAction.UPDATE_METADATA,
             (
                 PushChangeType.CREATE,
                 PushChangeType.UPDATE_DIFF,
-            ): ChangeComparisonAction.UPDATE,
+            ): ChangeComparisonAction.UPDATE_METADATA,
             (
                 PushChangeType.CREATE,
                 PushChangeType.CREATE,
-            ): ChangeComparisonAction.EXCLUDE,
+            ): ChangeComparisonAction.REPLACE,
             # update + update_diff = update with latest info
             (
                 PushChangeType.UPDATE,
                 PushChangeType.UPDATE_DIFF,
-            ): ChangeComparisonAction.UPDATE,
+            ): ChangeComparisonAction.UPDATE_METADATA,
             (
                 PushChangeType.UPDATE,
                 PushChangeType.UPDATE,
@@ -1161,7 +1174,7 @@ def _compare_changes(
             (
                 PushChangeType.UPDATE_DIFF,
                 PushChangeType.UPDATE_DIFF,
-            ): ChangeComparisonAction.UPDATE_DIFF,
+            ): ChangeComparisonAction.REPLACE_DIFFS,
             (
                 PushChangeType.UPDATE_DIFF,
                 PushChangeType.UPDATE,
@@ -1173,44 +1186,45 @@ def _compare_changes(
             (
                 PushChangeType.UPDATE_DIFF,
                 PushChangeType.CREATE,
-            ): ChangeComparisonAction.EXCLUDE,
+            ): ChangeComparisonAction.REPLACE,
             (
                 PushChangeType.DELETE,
                 PushChangeType.CREATE,
-            ): ChangeComparisonAction.REPLACE,
-            # delete + update = invalid sequence
+            ): ChangeComparisonAction.FORCE_UPDATE,
+            # delete + update = replace it (used for multicheckpoint ranges when we want to keep file in history even if it was deleted in the middle, so we keep delete but update metadata and diffs)
             (
                 PushChangeType.DELETE,
                 PushChangeType.UPDATE,
-            ): ChangeComparisonAction.EXCLUDE,
+            ): ChangeComparisonAction.REPLACE,
             (
                 PushChangeType.DELETE,
                 PushChangeType.UPDATE_DIFF,
             ): ChangeComparisonAction.EXCLUDE,
             (
                 PushChangeType.DELETE,
                 PushChangeType.DELETE,
-            ): ChangeComparisonAction.EXCLUDE,
+            ): ChangeComparisonAction.REPLACE,
         }
 
         action = action_map.get((previous.change, new.change))
+
         result = None
         if action == ChangeComparisonAction.REPLACE:
             result = new
 
-        elif action == ChangeComparisonAction.DELETE:
-            # if change is create + delete, we can just remove the change from accumulated changes
-            # only if this action is allowed (file existed before)
-            if prevent_delete_change:
-                result = new
+        elif action == ChangeComparisonAction.FORCE_UPDATE:
+            # handle force update case, when previous change was delete and new change is create - just revert to update with new metadata
+            new.change = PushChangeType.UPDATE
+            new.diffs = []
+            result = new
 
-        elif action == ChangeComparisonAction.UPDATE:
+        elif action == ChangeComparisonAction.UPDATE_METADATA:
             # handle update case, when previous change was create - just revert to create with new metadata
             new.change = previous.change
             new.diffs = []
             result = new
 
-        elif action == ChangeComparisonAction.UPDATE_DIFF:
+        elif action == ChangeComparisonAction.REPLACE_DIFFS:
             new.diffs = (previous.diffs or []) + (new.diffs or [])
             result = new
 
@@ -1269,8 +1283,12 @@ def create_checkpoint(
         changes = []
         for delta in delta_range:
             changes.extend(DeltaChangeSchema(many=True).load(delta.changes))
+
+        # Merge changes for compact storage and FileDiff checkpoint decisions.
+        merged_changes = cls.merge_changes(changes)
+
         merged_delta_items: List[DeltaChange] = [
-            d.to_data_delta() for d in cls.merge_changes(changes)
+            d.to_data_delta() for d in merged_changes
         ]
 
         # Pre-fetch data for all versioned files to create FileDiff checkpoints where it makes sense

server/mergin/tests/test_public_api_v2.py

@@ -25,6 +25,7 @@
 
 from .utils import (
     add_user,
+    create_blank_version,
     create_project,
     create_workspace,
     diffs_are_equal,
@@ -493,6 +494,106 @@ def test_delta_merge_changes():
     assert merged[0].checksum == delete8.checksum
 
 
+def test_delta_cross_checkpoint_create_delete_recreate(client):
+    """
+    Setup (rank-1 chunks cover 4 versions each, 4^1=4):
+        v1–v4:  tracked.gpkg does NOT exist (client at v4 has never seen it)
+        v5–v8:  tracked.gpkg is CREATED at v5  → rank-1 stores [CREATE(v5)]
+        v9–v12: tracked.gpkg is DELETED at v9, RE-CREATED at v10
+                → rank-1 stores [CREATE(v10)] after pre-merging DELETE+CREATE
+
+    get_delta_changes(4, 12) then loads [CREATE(v5), CREATE(v10)] and the outer
+    merge_changes hits CREATE+CREATE → EXCLUDE → returns [].
+
+    Expected: [CREATE(v10)] so the client downloads the re-created file.
+    """
+    project = Project.query.filter_by(
+        workspace_id=test_workspace_id, name=test_project
+    ).first()
+
+    tracked_gpkg = os.path.join(TMP_DIR, "tracked.gpkg")
+    shutil.copy(os.path.join(test_project_dir, "base.gpkg"), tracked_gpkg)
+
+    # advance to v4 with blank versions (project starts at v1)
+    create_blank_version(project)  # v2
+    create_blank_version(project)  # v3
+    create_blank_version(project)  # v4
+    assert project.latest_version == 4
+
+    # rank-1 chunk [v5–v8]: file is born at v5
+    push_change(project, "added", "tracked.gpkg", TMP_DIR)  # v5
+    create_blank_version(project)  # v6
+    create_blank_version(project)  # v7
+    create_blank_version(project)  # v8
+    assert project.latest_version == 8
+
+    # rank-1 chunk [v9–v12]: file is deleted then re-created in the same chunk
+    push_change(project, "removed", "tracked.gpkg", TMP_DIR)  # v9
+    push_change(project, "added", "tracked.gpkg", TMP_DIR)  # v10
+    create_blank_version(project)  # v11
+    create_blank_version(project)  # v12
+    assert project.latest_version == 12
+
+    # client at v4 never had tracked.gpkg; it was created at v5, deleted at v9, re-created at v10
+    delta = project.get_delta_changes(4, 12)
+
+    assert delta is not None
+    tracked = next((d for d in delta if d.path == "tracked.gpkg"), None)
+    # DELETE(v9)+CREATE(v10) was collapsed to CREATE inside checkpoint(v9-v12)
+    # then CREATE(v5)+CREATE(v10) hit CREATE+CREATE→EXCLUDE in the outer merge
+    assert tracked is not None
+    assert tracked.change == PushChangeType.CREATE
+    assert tracked.version == 10
+
+
+def test_delta_cross_checkpoint_recreate_then_delete(client):
+    """
+    Setup:
+        v1–v4:  base.gpkg exists (client at v4 has the file, exclude_delete should be False)
+        v5–v8:  base.gpkg DELETED at v5, RE-CREATED at v6
+                → rank-1 stores [CREATE(v6)] after pre-merging DELETE+CREATE
+        v9–v12: base.gpkg DELETED at v9 (permanently gone)
+                → rank-1 stores [DELETE(v9)]
+
+    get_delta_changes(4, 12) loads [CREATE(v6), DELETE(v9)]. The stored CREATE
+    from chunk A did not set updating_files, so exclude_delete=True and
+    CREATE+DELETE → EXCLUDE → returns [].
+
+    Expected: [DELETE] so the client removes the stale local copy of base.gpkg.
+    """
+    project = Project.query.filter_by(
+        workspace_id=test_workspace_id, name=test_project
+    ).first()
+
+    # advance to v4; base.gpkg is present throughout (client at v4 has it)
+    create_blank_version(project)  # v2
+    create_blank_version(project)  # v3
+    create_blank_version(project)  # v4
+    assert project.latest_version == 4
+
+    # rank-1 chunk [v5–v8]: delete then immediately re-create in the same chunk
+    push_change(project, "removed", "base.gpkg", TMP_DIR)  # v5
+    push_change(project, "added", "base.gpkg", test_project_dir)  # v6
+    create_blank_version(project)  # v7
+    create_blank_version(project)  # v8
+    assert project.latest_version == 8
+
+    # rank-1 chunk [v9–v12]: file is permanently deleted
+    push_change(project, "removed", "base.gpkg", TMP_DIR)  # v9
+    create_blank_version(project)  # v10
+    create_blank_version(project)  # v11
+    create_blank_version(project)  # v12
+    assert project.latest_version == 12
+
+    # client at v4 has base.gpkg; server deleted it at v9
+    delta = project.get_delta_changes(4, 12)
+
+    assert delta is not None
+    base_gpkg = next((d for d in delta if d.path == "base.gpkg"), None)
+    assert base_gpkg is not None
+    assert base_gpkg.change == PushChangeType.DELETE
+
+
 def test_project_version_delta_changes(client, diff_project: Project):
     """Test that get_delta_changes and its schema work as expected"""
     latest_version = diff_project.get_latest_version()
@@ -524,15 +625,15 @@ def test_project_version_delta_changes(client, diff_project: Project):
     # delete + create version
     delta = diff_project.get_delta_changes(1, 3)
     assert len(delta) == 1
-    assert delta[0].change == PushChangeType.CREATE
+    assert delta[0].change == PushChangeType.UPDATE
     # file was created in v3
     assert delta[0].version == 3
     assert delta[0].checksum == deltas[3].changes[0]["checksum"]
 
     # get_delta with update diff
     delta = diff_project.get_delta_changes(1, 4)
     assert len(delta) == 1
-    assert delta[0].change == PushChangeType.CREATE
+    assert delta[0].change == PushChangeType.UPDATE
     assert ProjectVersionDelta.query.filter_by(rank=1).count() == 0
 
     # create rank 1 checkpoint for v4