diff --git a/internal/db/sqlite/db_global_test.go b/internal/db/sqlite/db_global_test.go index c565668f6..b9ed25762 100644 --- a/internal/db/sqlite/db_global_test.go +++ b/internal/db/sqlite/db_global_test.go @@ -568,3 +568,59 @@ func TestNeedPagination(t *testing.T) { t.Error("bad need") } } + +func TestDeletedAfterConflict(t *testing.T) { + t.Parallel() + + // A delete that comes after a conflict should be applied, not lose the + // conflict and suddenly cause an old conflict version to become + // promoted. + + // D:\syncthing-windows-amd64-v2.0.0-rc.22.dev.11.gff88430e>syncthing --home=c:\PortableApp\SyncTrayzorPortable-x64\data\syncthing debug database-file tnhbr-gxtuf TreeSizeFreeSetup.exe + // DEVICE TYPE NAME SEQUENCE DELETED MODIFIED SIZE FLAGS VERSION BLOCKLIST + // -local- FILE TreeSizeFreeSetup.exe 499 del 2025-07-04T11:52:36.2804841Z 0 ------- HZJYWFM:1751507473,OMKHRPB:1751629956 -nil- + // J5WNYJ6 FILE TreeSizeFreeSetup.exe 500 del 2025-07-04T11:52:36.2804841Z 0 ------- HZJYWFM:1751507473,OMKHRPB:1751629956 -nil- + // 23NHXGS FILE TreeSizeFreeSetup.exe 445 --- 2025-06-23T03:16:10.2804841Z 13832808 -nG---- HZJYWFM:1751507473 7B4kLitF + // JKX6ZDN FILE TreeSizeFreeSetup.exe 320 --- 2025-06-23T03:16:10.2804841Z 13832808 ------- JKX6ZDN:1750992570 7B4kLitF + + db, err := OpenTemp() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { + if err := db.Close(); err != nil { + t.Fatal(err) + } + }) + + // A file, updated by some remote device. This file is an old, conflicted copy. + file := genFile("test1", 1, 101) + file.ModifiedS = 1750992570 + file.Version = protocol.Vector{Counters: []protocol.Counter{{ID: 5 << 60, Value: 1750992570}}} + if err := db.Update(folderID, protocol.DeviceID{5}, []protocol.FileInfo{file}); err != nil { + t.Fatal(err) + } + + // The file, updated by a newer remote device. This file is the newer, conflict-winning copy. + file.ModifiedS = 1751507473 + file.Version = protocol.Vector{Counters: []protocol.Counter{{ID: 2 << 60, Value: 1751507473}}} + if err := db.Update(folderID, protocol.DeviceID{2}, []protocol.FileInfo{file}); err != nil { + t.Fatal(err) + } + + // The file, deleted locally after syncing the file from the remote above.. + file.SetDeleted(4) + if err := db.Update(folderID, protocol.LocalDeviceID, []protocol.FileInfo{file}); err != nil { + t.Fatal(err) + } + + // The delete should be the global version + f, _, err := db.GetGlobalFile(folderID, "test1") + if err != nil { + t.Fatal(err) + } + if !f.IsDeleted() { + t.Log(f) + t.Error("should be deleted") + } +} diff --git a/internal/db/sqlite/folderdb_update.go b/internal/db/sqlite/folderdb_update.go index 0490da974..a02484a1f 100644 --- a/internal/db/sqlite/folderdb_update.go +++ b/internal/db/sqlite/folderdb_update.go @@ -458,12 +458,6 @@ func (e fileRow) Compare(other fileRow) int { } return -1 // they are invalid, we win } - if e.Deleted != other.Deleted { - if e.Deleted { // we are deleted, we lose - return 1 - } - return -1 // they are deleted, we win - } if d := cmp.Compare(e.Modified, other.Modified); d != 0 { return -d // positive d means we were newer, so we win (negative return) } diff --git a/lib/model/folder_sendrecv.go b/lib/model/folder_sendrecv.go index ac84fd832..7a4d2e88f 100644 --- a/lib/model/folder_sendrecv.go +++ b/lib/model/folder_sendrecv.go @@ -896,18 +896,20 @@ func (f *sendReceiveFolder) deleteFileWithCurrent(file, cur protocol.FileInfo, h return } - if f.inConflict(cur.Version, file.Version) { - // There is a conflict here, which shouldn't happen as deletions - // always lose. Merge the version vector of the file we have - // locally and commit it to db to resolve the conflict. - cur.Version = cur.Version.Merge(file.Version) - dbUpdateChan <- dbUpdateJob{cur, dbUpdateHandleFile} - return - } + switch { + case f.inConflict(cur.Version, file.Version) && !cur.IsSymlink(): + // If the delete constitutes winning a conflict, we move the file to + // a conflict copy instead of doing the delete + err = f.inWritableDir(func(name string) error { + return f.moveForConflict(name, file.ModifiedBy.String(), scanChan) + }, cur.Name) - if f.versioner != nil && !cur.IsSymlink() { + case f.versioner != nil && !cur.IsSymlink(): + // If we have a versioner, use that to move the file away err = f.inWritableDir(f.versioner.Archive, file.Name) - } else { + + default: + // Delete the file err = f.inWritableDir(f.mtimefs.Remove, file.Name) } diff --git a/lib/model/requests_test.go b/lib/model/requests_test.go index 700423d8b..d51982746 100644 --- a/lib/model/requests_test.go +++ b/lib/model/requests_test.go @@ -903,13 +903,24 @@ func TestRequestDeleteChanged(t *testing.T) { t.Fatal("timed out") } - // Check outcome - if _, err := tfs.Lstat(a); err != nil { - if fs.IsNotExist(err) { - t.Error(`Modified file "a" was removed`) - } else { - t.Error(`Error stating file "a":`, err) + // Check outcome. The file may have been moved to a conflict copy. + remains := false + files, err := tfs.Glob("a*") + if err != nil { + t.Fatal(err) + } + for _, file := range files { + if file == "a" { + remains = true + break } + if strings.HasPrefix(file, "a.sync-conflict-") { + remains = true + break + } + } + if !remains { + t.Error(`Modified file "a" was removed`) } } diff --git a/lib/protocol/bep_fileinfo.go b/lib/protocol/bep_fileinfo.go index 168481275..2fd6dbfa1 100644 --- a/lib/protocol/bep_fileinfo.go +++ b/lib/protocol/bep_fileinfo.go @@ -200,15 +200,6 @@ func (f *FileInfo) WinsConflict(other FileInfo) bool { return !f.IsInvalid() } - // If a modification is in conflict with a delete, we pick the - // modification. - if !f.IsDeleted() && other.IsDeleted() { - return true - } - if f.IsDeleted() && !other.IsDeleted() { - return false - } - // The one with the newer modification time wins. if f.ModTime().After(other.ModTime()) { return true diff --git a/lib/protocol/conflict_test.go b/lib/protocol/conflict_test.go index b68382252..4b0ff0e63 100644 --- a/lib/protocol/conflict_test.go +++ b/lib/protocol/conflict_test.go @@ -14,7 +14,7 @@ func TestWinsConflict(t *testing.T) { testcases := [][2]FileInfo{ // The first should always win over the second {{ModifiedS: 42}, {ModifiedS: 41}}, - {{ModifiedS: 41}, {ModifiedS: 42, Deleted: true}}, + {{ModifiedS: 42, Deleted: true}, {ModifiedS: 41}}, {{Deleted: true}, {ModifiedS: 10, LocalFlags: FlagLocalRemoteInvalid}}, {{ModifiedS: 41, Version: Vector{Counters: []Counter{{ID: 42, Value: 2}, {ID: 43, Value: 1}}}}, {ModifiedS: 41, Version: Vector{Counters: []Counter{{ID: 42, Value: 1}, {ID: 43, Value: 2}}}}}, } diff --git a/relnotes/v2.0.md b/relnotes/v2.0.md index 4a0bff505..c422f60db 100644 --- a/relnotes/v2.0.md +++ b/relnotes/v2.0.md @@ -36,3 +36,7 @@ - netbsd/* - openbsd/386 and openbsd/arm - windows/arm + +- The handling of conflict resolution involving deleted files has changed. A + delete can now be the winning outcome of conflict resolution, resulting in + the deleted file being moved to a conflict copy.