Bug fix: ref conflict can cause tags to be temporarily removed on a replica

go/libraries/doltcore/sqle/read_replica_database.go

@@ -18,6 +18,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"sort"
 	"strings"
 	"sync"
 
@@ -509,9 +510,20 @@ func getReplicationRefs(ctx *sql.Context, rrd ReadReplicaDatabase) (
 func refsToDelete(remRefs, localRefs []doltdb.RefWithHash) []doltdb.RefWithHash {
 	toDelete := make([]doltdb.RefWithHash, 0, len(localRefs))
 	var i, j int
+
+	// Before we map remote refs to local refs to determine which refs to delete, we need to sort them
+	// by Ref.String() – this ensures a unique identifier that does not conflict with other refs, unlike
+	// Ref.GetPath(), which can conflict if a branch or tag has the same name.
+	sort.Slice(remRefs, func(i, j int) bool {
+		return remRefs[i].Ref.String() < remRefs[j].Ref.String()
+	})
+	sort.Slice(localRefs, func(i, j int) bool {
+		return localRefs[i].Ref.String() < localRefs[j].Ref.String()
+	})
+
 	for i < len(remRefs) && j < len(localRefs) {
-		rem := remRefs[i].Ref.GetPath()
-		local := localRefs[j].Ref.GetPath()
+		rem := remRefs[i].Ref.String()
+		local := localRefs[j].Ref.String()
 		if rem == local {
 			i++
 			j++

go/libraries/doltcore/sqle/replication_test.go

@@ -80,12 +80,17 @@ func TestReplicationBranches(t *testing.T) {
 			local:       []string{"feature4", "feature5", "feature6", "feature7", "feature8", "feature9"},
 			expToDelete: []string{"feature4", "feature5", "feature6", "feature7", "feature8", "feature9"},
 		},
+		{
+			remote:      []string{"main", "new1", "a1"},
+			local:       []string{"main", "a1"},
+			expToDelete: []string{},
+		},
 	}
 
 	for _, tt := range tests {
 		remoteRefs := make([]doltdb.RefWithHash, len(tt.remote))
 		for i := range tt.remote {
-			remoteRefs[i] = doltdb.RefWithHash{Ref: ref.NewRemoteRef("", tt.remote[i])}
+			remoteRefs[i] = doltdb.RefWithHash{Ref: ref.NewBranchRef(tt.remote[i])}
 		}
 		localRefs := make([]doltdb.RefWithHash, len(tt.local))
 		for i := range tt.local {
@@ -96,6 +101,6 @@ func TestReplicationBranches(t *testing.T) {
 		for i := range diff {
 			diffNames[i] = diff[i].Ref.GetPath()
 		}
-		assert.Equal(t, diffNames, tt.expToDelete)
+		assert.Equal(t, tt.expToDelete, diffNames)
 	}
 }

integration-tests/bats/replication.bats

@@ -201,6 +201,48 @@ teardown() {
     [[ "$output" =~ "b1" ]] || false
 }
 
+# When a replica pulls refs, the remote refs are compared with the local refs to identify which local refs
+# need to be deleted. Branches, tags, and remotes all share the ref space and previous versions of Dolt could
+# incorrectly map remote refs and local refs, resulting in local refs being incorrectly removed, until future
+# runs of replica synchronization.
+@test "replication: local tag refs are not deleted" {
+    # Configure repo1 to push changes on commit and create tag a1
+    cd repo1
+    dolt config --local --add sqlserver.global.dolt_replicate_to_remote remote1
+    dolt sql -q "call dolt_tag('a1');"
+
+    # Configure repo2 to pull changes on read
+    cd ..
+    dolt clone file://./rem1 repo2
+    cd repo2
+    dolt config --local --add sqlserver.global.dolt_read_replica_remote origin
+    dolt config --local --add sqlserver.global.dolt_replicate_all_heads 1
+    run dolt sql -q "select tag_name from dolt_tags;"
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ "| tag_name |" ]] || false
+    [[ "$output" =~ "| a1       |" ]] || false
+
+    # Create branch new1 in repo1 – "new1" sorts after "main", but before "a1", and previous
+    # versions of Dolt had problems computing which local refs to delete in this case.
+    cd ../repo1
+    dolt sql -q "call dolt_branch('new1');"
+
+    # Confirm that tag a1 has not been deleted. Note that we need to check for this immediately after
+    # creating branch new1 (i.e. before looking at branches), because the bug in the previous versions
+    # of Dolt would only manifest in the next command, and would be fixed by later remote pulls.
+    cd ../repo2
+    run dolt sql -q "select tag_name from dolt_tags;"
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ "| tag_name |" ]] || false
+    [[ "$output" =~ "| a1       |" ]] || false
+
+    # Try again to make sure the results are stable
+    run dolt sql -q "select tag_name from dolt_tags;"
+    [ "$status" -eq 0 ]
+    [[ "$output" =~ "| tag_name |" ]] || false
+    [[ "$output" =~ "| a1       |" ]] || false
+}
+
 @test "replication: pull branch delete current branch" {
     skip "broken by latest transaction changes"
 
@@ -627,7 +669,6 @@ SQL
 }
 
 @test "replication: pull all heads pulls tags" {
-
     dolt clone file://./rem1 repo2
     cd repo2
     dolt checkout -b new_feature