Avoid choosing parent snapshot newer than time of current snapshot

Currently, `restic backup` (if a `--parent` is not provided)
will choose the most recent matching snapshot as the parent snapshot.
This makes sense in the usual case,
where we tag the snapshot-being-created with the current time.

However, this doesn't make sense if the user has passed `--time`
and is currently creating a snapshot older than the latest snapshot.
Instead, choose the most recent snapshot
which is not newer than the snapshot-being-created's timestamp,
to avoid any time travel.

Impetus for this change:
I'm using restic for the first time!
I have a number of existing BTRFS snapshots
I am backing up via restic to serve as my initial set of backups.
I initially `restic backup`'d the most recent snapshot to test,
then started backing up each of the other snapshots.
I noticed in `restic cat snapshot <id>` output
that all the remaining snapshots have the most recent as the parent.

changelog/unreleased/pull-3619

@@ -0,0 +1,12 @@
+Bugfix: Avoid choosing parent snapshots newer than time of current snapshot
+
+`restic backup` (if a `--parent` is not provided)
+previously chose the most recent matching snapshot as the parent snapshot.
+However, this didn't make sense when the user passed `--time`
+to create a snapshot older than the most recent snapshot.
+
+Instead, `restic backup` now chooses the most recent snapshot
+which is not newer than the snapshot-being-created's timestamp,
+to avoid any time travel.
+
+https://github.com/restic/restic/pull/3619

cmd/restic/cmd_backup.go

@@ -103,7 +103,7 @@ func init() {
 	cmdRoot.AddCommand(cmdBackup)
 
 	f := cmdBackup.Flags()
-	f.StringVar(&backupOptions.Parent, "parent", "", "use this parent `snapshot` (default: last snapshot in the repo that has the same target files/directories)")
+	f.StringVar(&backupOptions.Parent, "parent", "", "use this parent `snapshot` (default: last snapshot in the repo that has the same target files/directories, and is not newer than the snapshot time)")
 	f.BoolVarP(&backupOptions.Force, "force", "f", false, `force re-reading the target files/directories (overrides the "parent" flag)`)
 	f.StringArrayVarP(&backupOptions.Excludes, "exclude", "e", nil, "exclude a `pattern` (can be specified multiple times)")
 	f.StringArrayVar(&backupOptions.InsensitiveExcludes, "iexclude", nil, "same as --exclude `pattern` but ignores the casing of filenames")
@@ -472,7 +472,7 @@ func collectTargets(opts BackupOptions, args []string) (targets []string, err er
 
 // parent returns the ID of the parent snapshot. If there is none, nil is
 // returned.
-func findParentSnapshot(ctx context.Context, repo restic.Repository, opts BackupOptions, targets []string) (parentID *restic.ID, err error) {
+func findParentSnapshot(ctx context.Context, repo restic.Repository, opts BackupOptions, targets []string, timeStamp time.Time) (parentID *restic.ID, err error) {
 	// Force using a parent
 	if !opts.Force && opts.Parent != "" {
 		id, err := restic.FindSnapshot(ctx, repo, opts.Parent)
@@ -485,7 +485,7 @@ func findParentSnapshot(ctx context.Context, repo restic.Repository, opts Backup
 
 	// Find last snapshot to set it as parent, if not already set
 	if !opts.Force && parentID == nil {
-		id, err := restic.FindLatestSnapshot(ctx, repo, targets, []restic.TagList{}, []string{opts.Host})
+		id, err := restic.FindLatestSnapshot(ctx, repo, targets, []restic.TagList{}, []string{opts.Host}, &timeStamp)
 		if err == nil {
 			parentID = &id
 		} else if err != restic.ErrNoSnapshotFound {
@@ -579,7 +579,7 @@ func runBackup(opts BackupOptions, gopts GlobalOptions, term *termstatus.Termina
 		return err
 	}
 
-	parentSnapshotID, err := findParentSnapshot(gopts.ctx, repo, opts, targets)
+	parentSnapshotID, err := findParentSnapshot(gopts.ctx, repo, opts, targets, timeStamp)
 	if err != nil {
 		return err
 	}

cmd/restic/cmd_dump.go

@@ -152,7 +152,7 @@ func runDump(opts DumpOptions, gopts GlobalOptions, args []string) error {
 	var id restic.ID
 
 	if snapshotIDString == "latest" {
-		id, err = restic.FindLatestSnapshot(ctx, repo, opts.Paths, opts.Tags, opts.Hosts)
+		id, err = restic.FindLatestSnapshot(ctx, repo, opts.Paths, opts.Tags, opts.Hosts, nil)
 		if err != nil {
 			Exitf(1, "latest snapshot for criteria not found: %v Paths:%v Hosts:%v", err, opts.Paths, opts.Hosts)
 		}

cmd/restic/cmd_restore.go

@@ -118,7 +118,7 @@ func runRestore(opts RestoreOptions, gopts GlobalOptions, args []string) error {
 	var id restic.ID
 
 	if snapshotIDString == "latest" {
-		id, err = restic.FindLatestSnapshot(ctx, repo, opts.Paths, opts.Tags, opts.Hosts)
+		id, err = restic.FindLatestSnapshot(ctx, repo, opts.Paths, opts.Tags, opts.Hosts, nil)
 		if err != nil {
 			Exitf(1, "latest snapshot for criteria not found: %v Paths:%v Hosts:%v", err, opts.Paths, opts.Hosts)
 		}

cmd/restic/find.go

@@ -23,7 +23,7 @@ func FindFilteredSnapshots(ctx context.Context, repo *repository.Repository, hos
 			for _, s := range snapshotIDs {
 				if s == "latest" {
 					usedFilter = true
-					id, err = restic.FindLatestSnapshot(ctx, repo, paths, tags, hosts)
+					id, err = restic.FindLatestSnapshot(ctx, repo, paths, tags, hosts, nil)
 					if err != nil {
 						Warnf("Ignoring %q, no snapshot matched given filter (Paths:%v Tags:%v Hosts:%v)\n", s, paths, tags, hosts)
 						continue

internal/restic/snapshot_find.go

@@ -13,8 +13,8 @@ import (
 // ErrNoSnapshotFound is returned when no snapshot for the given criteria could be found.
 var ErrNoSnapshotFound = errors.New("no snapshot found")
 
-// FindLatestSnapshot finds latest snapshot with optional target/directory, tags and hostname filters.
-func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, tagLists []TagList, hostnames []string) (ID, error) {
+// FindLatestSnapshot finds latest snapshot with optional target/directory, tags, hostname, and timestamp filters.
+func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string, tagLists []TagList, hostnames []string, timeStamp *time.Time) (ID, error) {
 	var err error
 	absTargets := make([]string, 0, len(targets))
 	for _, target := range targets {
@@ -38,6 +38,10 @@ func FindLatestSnapshot(ctx context.Context, repo Repository, targets []string,
 			return errors.Errorf("Error loading snapshot %v: %v", id.Str(), err)
 		}
 
+		if timeStamp != nil && snapshot.Time.After(*timeStamp) {
+			return nil
+		}
+
 		if snapshot.Time.Before(latest) {
 			return nil
 		}

internal/restic/snapshot_find_test.go

@@ -0,0 +1,47 @@
+package restic_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/restic/restic/internal/repository"
+	"github.com/restic/restic/internal/restic"
+)
+
+func TestFindLatestSnapshot(t *testing.T) {
+	repo, cleanup := repository.TestRepository(t)
+	defer cleanup()
+
+	restic.TestCreateSnapshot(t, repo, parseTimeUTC("2015-05-05 05:05:05"), 1, 0)
+	restic.TestCreateSnapshot(t, repo, parseTimeUTC("2017-07-07 07:07:07"), 1, 0)
+	latestSnapshot := restic.TestCreateSnapshot(t, repo, parseTimeUTC("2019-09-09 09:09:09"), 1, 0)
+
+	id, err := restic.FindLatestSnapshot(context.TODO(), repo, []string{}, []restic.TagList{}, []string{"foo"}, nil)
+	if err != nil {
+		t.Fatalf("FindLatestSnapshot returned error: %v", err)
+	}
+
+	if id != *latestSnapshot.ID() {
+		t.Errorf("FindLatestSnapshot returned wrong snapshot ID: %v", id)
+	}
+}
+
+func TestFindLatestSnapshotWithMaxTimestamp(t *testing.T) {
+	repo, cleanup := repository.TestRepository(t)
+	defer cleanup()
+
+	restic.TestCreateSnapshot(t, repo, parseTimeUTC("2015-05-05 05:05:05"), 1, 0)
+	desiredSnapshot := restic.TestCreateSnapshot(t, repo, parseTimeUTC("2017-07-07 07:07:07"), 1, 0)
+	restic.TestCreateSnapshot(t, repo, parseTimeUTC("2019-09-09 09:09:09"), 1, 0)
+
+	maxTimestamp := parseTimeUTC("2018-08-08 08:08:08")
+
+	id, err := restic.FindLatestSnapshot(context.TODO(), repo, []string{}, []restic.TagList{}, []string{"foo"}, &maxTimestamp)
+	if err != nil {
+		t.Fatalf("FindLatestSnapshot returned error: %v", err)
+	}
+
+	if id != *desiredSnapshot.ID() {
+		t.Errorf("FindLatestSnapshot returned wrong snapshot ID: %v", id)
+	}
+}