Skip to content

Commit 3b49fd2

Browse files
nielashncw
authored andcommitted
bisync: fix listings missing concurrent modifications - fixes rclone#8359
Before this change, there was a bug affecting listing files when: - a given bisync run had changes in the 2to1 direction AND - the run had NO changes in the 1to2 direction AND - at least one of the changed files changed AGAIN during the run (specifically, after the initial march and before the transfers.) In this situation, the listings on one side would still retain the prior version of the changed file, potentially causing conflicts or errors. This change fixes the issue by making sure that if we're updating the listings on one side, we must also update the other. (We previously tried to skip it for efficiency, but this failed to account for the possibility that a changed file could change again during the run.)
1 parent c0515a5 commit 3b49fd2

31 files changed

+181
-24
lines changed

cmd/bisync/bisync_test.go

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -746,6 +746,16 @@ func (b *bisyncTest) runTestStep(ctx context.Context, line string) (err error) {
746746
case "test-func":
747747
b.TestFn = testFunc
748748
return
749+
case "concurrent-func":
750+
b.TestFn = func() {
751+
src := filepath.Join(b.dataDir, "file7.txt")
752+
dst := "file1.txt"
753+
err := b.copyFile(ctx, src, b.replaceHex(b.path2), dst)
754+
if err != nil {
755+
fs.Errorf(src, "error copying file: %v", err)
756+
}
757+
}
758+
return
749759
case "fix-names":
750760
// in case the local os converted any filenames
751761
ci.NoUnicodeNormalization = true

cmd/bisync/deltas.go

Lines changed: 7 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -286,7 +286,7 @@ func (b *bisyncRun) findDeltas(fctx context.Context, f fs.Fs, oldListing string,
286286
}
287287

288288
// applyDeltas
289-
func (b *bisyncRun) applyDeltas(ctx context.Context, ds1, ds2 *deltaSet) (changes1, changes2 bool, results2to1, results1to2 []Results, queues queues, err error) {
289+
func (b *bisyncRun) applyDeltas(ctx context.Context, ds1, ds2 *deltaSet) (results2to1, results1to2 []Results, queues queues, err error) {
290290
path1 := bilib.FsPath(b.fs1)
291291
path2 := bilib.FsPath(b.fs2)
292292

@@ -367,7 +367,7 @@ func (b *bisyncRun) applyDeltas(ctx context.Context, ds1, ds2 *deltaSet) (change
367367
}
368368
}
369369

370-
//if there are potential conflicts to check, check them all here (outside the loop) in one fell swoop
370+
// if there are potential conflicts to check, check them all here (outside the loop) in one fell swoop
371371
matches, err := b.checkconflicts(ctxCheck, filterCheck, b.fs1, b.fs2)
372372

373373
for _, file := range ds1.sort() {
@@ -392,7 +392,7 @@ func (b *bisyncRun) applyDeltas(ctx context.Context, ds1, ds2 *deltaSet) (change
392392
} else if d2.is(deltaOther) {
393393
b.indent("!WARNING", file, "New or changed in both paths")
394394

395-
//if files are identical, leave them alone instead of renaming
395+
// if files are identical, leave them alone instead of renaming
396396
if (dirs1.has(file) || dirs1.has(alias)) && (dirs2.has(file) || dirs2.has(alias)) {
397397
fs.Infof(nil, "This is a directory, not a file. Skipping equality check and will not rename: %s", file)
398398
ls1.getPut(file, skippedDirs1)
@@ -486,7 +486,6 @@ func (b *bisyncRun) applyDeltas(ctx context.Context, ds1, ds2 *deltaSet) (change
486486

487487
// Do the batch operation
488488
if copy2to1.NotEmpty() && !b.InGracefulShutdown {
489-
changes1 = true
490489
b.indent("Path2", "Path1", "Do queued copies to")
491490
ctx = b.setBackupDir(ctx, 1)
492491
results2to1, err = b.fastCopy(ctx, b.fs2, b.fs1, copy2to1, "copy2to1")
@@ -498,12 +497,11 @@ func (b *bisyncRun) applyDeltas(ctx context.Context, ds1, ds2 *deltaSet) (change
498497
return
499498
}
500499

501-
//copy empty dirs from path2 to path1 (if --create-empty-src-dirs)
500+
// copy empty dirs from path2 to path1 (if --create-empty-src-dirs)
502501
b.syncEmptyDirs(ctx, b.fs1, copy2to1, dirs2, &results2to1, "make")
503502
}
504503

505504
if copy1to2.NotEmpty() && !b.InGracefulShutdown {
506-
changes2 = true
507505
b.indent("Path1", "Path2", "Do queued copies to")
508506
ctx = b.setBackupDir(ctx, 2)
509507
results1to2, err = b.fastCopy(ctx, b.fs1, b.fs2, copy1to2, "copy1to2")
@@ -515,23 +513,23 @@ func (b *bisyncRun) applyDeltas(ctx context.Context, ds1, ds2 *deltaSet) (change
515513
return
516514
}
517515

518-
//copy empty dirs from path1 to path2 (if --create-empty-src-dirs)
516+
// copy empty dirs from path1 to path2 (if --create-empty-src-dirs)
519517
b.syncEmptyDirs(ctx, b.fs2, copy1to2, dirs1, &results1to2, "make")
520518
}
521519

522520
if delete1.NotEmpty() && !b.InGracefulShutdown {
523521
if err = b.saveQueue(delete1, "delete1"); err != nil {
524522
return
525523
}
526-
//propagate deletions of empty dirs from path2 to path1 (if --create-empty-src-dirs)
524+
// propagate deletions of empty dirs from path2 to path1 (if --create-empty-src-dirs)
527525
b.syncEmptyDirs(ctx, b.fs1, delete1, dirs1, &results2to1, "remove")
528526
}
529527

530528
if delete2.NotEmpty() && !b.InGracefulShutdown {
531529
if err = b.saveQueue(delete2, "delete2"); err != nil {
532530
return
533531
}
534-
//propagate deletions of empty dirs from path1 to path2 (if --create-empty-src-dirs)
532+
// propagate deletions of empty dirs from path1 to path2 (if --create-empty-src-dirs)
535533
b.syncEmptyDirs(ctx, b.fs2, delete2, dirs2, &results1to2, "remove")
536534
}
537535

cmd/bisync/operations.go

Lines changed: 3 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -359,8 +359,6 @@ func (b *bisyncRun) runLocked(octx context.Context) (err error) {
359359

360360
// Determine and apply changes to Path1 and Path2
361361
noChanges := ds1.empty() && ds2.empty()
362-
changes1 := false // 2to1
363-
changes2 := false // 1to2
364362
results2to1 := []Results{}
365363
results1to2 := []Results{}
366364

@@ -370,7 +368,7 @@ func (b *bisyncRun) runLocked(octx context.Context) (err error) {
370368
fs.Infof(nil, "No changes found")
371369
} else {
372370
fs.Infof(nil, "Applying changes")
373-
changes1, changes2, results2to1, results1to2, queues, err = b.applyDeltas(octx, ds1, ds2)
371+
results2to1, results1to2, queues, err = b.applyDeltas(octx, ds1, ds2)
374372
if err != nil {
375373
if b.InGracefulShutdown && (err == context.Canceled || err == accounting.ErrorMaxTransferLimitReachedGraceful || strings.Contains(err.Error(), "context canceled")) {
376374
fs.Infof(nil, "Ignoring sync error due to Graceful Shutdown: %v", err)
@@ -395,21 +393,11 @@ func (b *bisyncRun) runLocked(octx context.Context) (err error) {
395393
}
396394
b.saveOldListings()
397395
// save new listings
398-
// NOTE: "changes" in this case does not mean this run vs. last run, it means start of this run vs. end of this run.
399-
// i.e. whether we can use the March lst-new as this side's lst without modifying it.
400396
if noChanges {
401397
b.replaceCurrentListings()
402398
} else {
403-
if changes1 || b.InGracefulShutdown { // 2to1
404-
err1 = b.modifyListing(fctx, b.fs2, b.fs1, results2to1, queues, false)
405-
} else {
406-
err1 = bilib.CopyFileIfExists(b.newListing1, b.listing1)
407-
}
408-
if changes2 || b.InGracefulShutdown { // 1to2
409-
err2 = b.modifyListing(fctx, b.fs1, b.fs2, results1to2, queues, true)
410-
} else {
411-
err2 = bilib.CopyFileIfExists(b.newListing2, b.listing2)
412-
}
399+
err1 = b.modifyListing(fctx, b.fs2, b.fs1, results2to1, queues, false) // 2to1
400+
err2 = b.modifyListing(fctx, b.fs1, b.fs2, results1to2, queues, true) // 1to2
413401
}
414402
if b.DebugName != "" {
415403
l1, _ := b.loadListing(b.listing1)
Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
"file1.txt"
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# bisync listing v1 from test
2+
- 109 - - 2000-01-01T00:00:00.000000000+0000 "RCLONE_TEST"
3+
- 19 - - 2023-08-26T00:00:00.000000000+0000 "file1.txt"
4+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file2.txt"
5+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file3.txt"
6+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file4.txt"
7+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file5.txt"
8+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file6.txt"
9+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file7.txt"
10+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file8.txt"
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# bisync listing v1 from test
2+
- 109 - - 2000-01-01T00:00:00.000000000+0000 "RCLONE_TEST"
3+
- 19 - - 2023-08-26T00:00:00.000000000+0000 "file1.txt"
4+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file2.txt"
5+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file3.txt"
6+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file4.txt"
7+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file5.txt"
8+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file6.txt"
9+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file7.txt"
10+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file8.txt"
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# bisync listing v1 from test
2+
- 109 - - 2000-01-01T00:00:00.000000000+0000 "RCLONE_TEST"
3+
- 19 - - 2023-08-26T00:00:00.000000000+0000 "file1.txt"
4+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file2.txt"
5+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file3.txt"
6+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file4.txt"
7+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file5.txt"
8+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file6.txt"
9+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file7.txt"
10+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file8.txt"
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# bisync listing v1 from test
2+
- 109 - - 2000-01-01T00:00:00.000000000+0000 "RCLONE_TEST"
3+
- 19 - - 2023-08-26T00:00:00.000000000+0000 "file1.txt"
4+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file2.txt"
5+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file3.txt"
6+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file4.txt"
7+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file5.txt"
8+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file6.txt"
9+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file7.txt"
10+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file8.txt"
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# bisync listing v1 from test
2+
- 109 - - 2000-01-01T00:00:00.000000000+0000 "RCLONE_TEST"
3+
- 19 - - 2023-08-26T00:00:00.000000000+0000 "file1.txt"
4+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file2.txt"
5+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file3.txt"
6+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file4.txt"
7+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file5.txt"
8+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file6.txt"
9+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file7.txt"
10+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file8.txt"
Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
# bisync listing v1 from test
2+
- 109 - - 2000-01-01T00:00:00.000000000+0000 "RCLONE_TEST"
3+
- 19 - - 2023-08-26T00:00:00.000000000+0000 "file1.txt"
4+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file2.txt"
5+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file3.txt"
6+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file4.txt"
7+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file5.txt"
8+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file6.txt"
9+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file7.txt"
10+
- 0 - - 2000-01-01T00:00:00.000000000+0000 "file8.txt"

0 commit comments

Comments
 (0)