Limit and retry the module download in the Docker build (closes #351)

Each `go mod download` in the Dockerfile now runs under a five-minute
time limit and is tried up to three times. Without a limit, a stalled
connection to the module proxy held the step, and every build on the
host sharing it, until the kernel dropped the connection. Five minutes
is well above the three minutes a working download can take on a busy
build host. When all three attempts fail, the step prints "module
download failed" and exits 1, so the build stops there instead of
failing as a test or lint failure. A test checks every download step
for the limit, the retries, the message and the final `exit 1`.

Model: opus-5-5

Dockerfile

@@ -5,7 +5,16 @@
 FROM golangci/golangci-lint:v2.14.0@sha256:ad862ba6b3798cbe0fd9fd7408d498fd74fbd2623a92406b2fd3898faf0bf98f AS lint
 WORKDIR /src
 COPY go.mod go.sum ./
-RUN go mod download
+# Without the time limit a stalled connection to the module proxy holds
+# this step, and every build on the host sharing it, until the kernel
+# drops the connection, which can take an hour. The limit is well above
+# the three minutes a working download can take on a busy build host.
+RUN for attempt in 1 2 3; do \
+        timeout 300 go mod download && exit 0; \
+        echo "go mod download attempt $attempt failed or ran over 300s" >&2; \
+    done; \
+    echo "module download failed: giving up after 3 attempts" >&2; \
+    exit 1
 COPY . .
 # `golangci-lint run` silently ignores an unknown top-level key in
 # .golangci.yml, such as a misspelt `linters:`; `config verify` fails on it.
@@ -18,7 +27,13 @@ RUN golangci-lint run --config .golangci.yml ./...
 FROM golang:1.26.1@sha256:cd78d88e00afadbedd272f977d375a6247455f3a4b1178f8ae8bbcb201743a8a AS test
 WORKDIR /src
 COPY go.mod go.sum ./
-RUN go mod download
+# Limited and retried as in the lint phase above.
+RUN for attempt in 1 2 3; do \
+        timeout 300 go mod download && exit 0; \
+        echo "go mod download attempt $attempt failed or ran over 300s" >&2; \
+    done; \
+    echo "module download failed: giving up after 3 attempts" >&2; \
+    exit 1
 COPY . .
 RUN go test -timeout 90s -race -cover ./... || \
     { echo "--- Rerunning with -v for details ---"; \
@@ -36,7 +51,13 @@ RUN apk add --no-cache git
 RUN git config --system --add safe.directory /src
 WORKDIR /src
 COPY go.mod go.sum ./
-RUN go mod download
+# Limited and retried as in the lint phase above.
+RUN for attempt in 1 2 3; do \
+        timeout 300 go mod download && exit 0; \
+        echo "go mod download attempt $attempt failed or ran over 300s" >&2; \
+    done; \
+    echo "module download failed: giving up after 3 attempts" >&2; \
+    exit 1
 COPY . .
 
 # The VERSION build arg when one is given, otherwise

README.md

@@ -372,9 +372,7 @@ on the destination in one go, use `vaultik remote nuke --force`.
   store — its remote key (or an unambiguous leading part of it) as shown
   by `snapshot list`. See
   [restoring on another machine](#restoring-on-another-machine).
-* Optional path arguments to restore specific files/directories (default: all).
+* Optional path arguments to restore specific files/directories (default: all)
-  A path that matches nothing in the snapshot is named in a warning, and the
-  command exits non-zero after restoring the paths that match.
 * Preserves file permissions, timestamps, ownership (ownership requires root),
   symlinks, and empty directories
 * `--verify`: After restoring, verify every restored file's chunk hashes

cmd/vaultik/dockermoddownload_test.go

@@ -0,0 +1,45 @@
+package main_test
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestDockerfileBoundsEveryModuleDownload fails unless every
+// `go mod download` in the Dockerfile runs under a time limit, is tried
+// a fixed number of times, and then fails the build with a message that
+// names the module download (issue #351). Like the version tests, this
+// parses the Dockerfile; that a stalled download really is cut off and
+// retried was checked by hand.
+func TestDockerfileBoundsEveryModuleDownload(t *testing.T) {
+	t.Parallel()
+
+	var downloads []string
+
+	for _, instruction := range instructions(t, productDockerfile) {
+		if strings.Contains(instruction, "go mod download") {
+			downloads = append(downloads, instruction)
+		}
+	}
+
+	require.NotEmpty(t, downloads, "%s must download modules",
+		productDockerfile)
+
+	for _, download := range downloads {
+		assert.Contains(t, download, "timeout 300 go mod download",
+			"%s: a module download must have a time limit: %s",
+			productDockerfile, download)
+		assert.Contains(t, download, "for attempt in 1 2 3",
+			"%s: a module download must be retried: %s",
+			productDockerfile, download)
+		assert.Contains(t, download, "module download failed",
+			"%s: a module download that never succeeds must say so: %s",
+			productDockerfile, download)
+		assert.True(t, strings.HasSuffix(download, "exit 1"),
+			"%s: a module download that never succeeds must fail the"+
+				" build: %s", productDockerfile, download)
+	}
+}

internal/cli/snapshot_restore.go

@@ -30,8 +30,6 @@ This command will restore files from the specified snapshot to the
 target directory.
 If no paths are specified, all files are restored.
 If paths are specified, only matching files/directories are restored.
-A path that matches nothing in the snapshot is named in a warning, and
-the command exits non-zero after restoring the paths that match.
 
 The snapshot may be named by its ID or, when restoring on a host with no
 local index, by the remote key that 'snapshot list' prints for a

internal/vaultik/restore.go

@@ -27,7 +27,6 @@ import (
 // Sentinel errors for restore failures.
 var (
 	errFilesFailedRestore    = errors.New("file(s) failed to restore")
-	errPathsNotInSnapshot    = errors.New("requested path(s) not in the snapshot")
 	errFilesFailedVerify     = errors.New("files failed verification")
 	errDecryptionKeyRequired = errors.New(
 		"decryption key required for restore\n\n" +
@@ -151,20 +150,16 @@ func (v *Vaultik) Restore(opts *RestoreOptions) error {
 	repos := database.NewRepositories(tempDB)
 
 	// Step 2: Get list of files to restore
-	files, unmatched, err := v.getFilesToRestore(v.ctx, repos, opts.Paths)
+	files, err := v.getFilesToRestore(v.ctx, repos, opts.Paths)
 	if err != nil {
 		return fmt.Errorf("getting files to restore: %w", err)
 	}
 
-	// Named before anything is restored, so the names are shown even
-	// when the restore stops on an error.
-	unmatchedErr := v.reportUnmatchedPaths(unmatched)
-
 	if len(files) == 0 {
 		log.Warn("No files found to restore")
 		v.UI.Warningf("No files found to restore.")
 
-		return unmatchedErr
+		return nil
 	}
 
 	log.Info("Found files to restore", "count", len(files))
@@ -204,17 +199,14 @@ func (v *Vaultik) Restore(opts *RestoreOptions) error {
 		v.UI.Duration(result.Duration),
 	)
 
-	return v.finishRestore(repos, files, unmatchedErr, opts, result)
+	return v.finishRestore(repos, files, opts, result)
 }
 
 // finishRestore emits the post-restore warnings, runs optional
 // verification, and converts any failed-file count into an error.
-// Otherwise it returns unmatchedErr, the error for the requested paths
-// that matched nothing (nil when there are none).
 func (v *Vaultik) finishRestore(
 	repos *database.Repositories,
 	files []*database.File,
-	unmatchedErr error,
 	opts *RestoreOptions,
 	result *RestoreResult,
 ) error {
@@ -244,23 +236,7 @@ func (v *Vaultik) finishRestore(
 		return fmt.Errorf("%d %w", result.FilesFailed, errFilesFailedRestore)
 	}
 
-	return unmatchedErr
+	return nil
-}
-
-// reportUnmatchedPaths warns, by name, about each requested path that
-// matched nothing in the snapshot, and returns an error counting them,
-// or nil when there are none.
-func (v *Vaultik) reportUnmatchedPaths(unmatched []string) error {
-	if len(unmatched) == 0 {
-		return nil
-	}
-
-	for _, path := range unmatched {
-		log.Warn("Requested path matches nothing in the snapshot", "path", path)
-		v.UI.Warningf("%s matches nothing in the snapshot.", v.UI.Path(path))
-	}
-
-	return fmt.Errorf("%d %w", len(unmatched), errPathsNotInSnapshot)
 }
 
 // restoreIdentities parses the configured age secret key once into every
@@ -864,34 +840,27 @@ func (v *Vaultik) materializeSnapshotDB(
 	return db, tempDir, nil
 }
 
-// getFilesToRestore returns the list of files to restore based on path
+// getFilesToRestore returns the list of files to restore based on path filters
-// filters, and the filters, as given, that matched no file.
 func (v *Vaultik) getFilesToRestore(
 	ctx context.Context, repos *database.Repositories, pathFilters []string,
-) ([]*database.File, []string, error) {
+) ([]*database.File, error) {
 	// If no filters, get all files
 	if len(pathFilters) == 0 {
-		files, err := repos.Files.ListAll(ctx)
+		return repos.Files.ListAll(ctx)
-
-		return files, nil, err
 	}
 
 	// Get files matching the path filters
-	var (
+	var result []*database.File
-		result    []*database.File
-		unmatched []string
-	)
 
 	seen := make(map[string]bool)
 
 	for _, filter := range pathFilters {
-		files, err := repos.Files.ListUnderPath(ctx, filepath.Clean(filter))
+		// Normalize the filter path
-		if err != nil {
+		filter = filepath.Clean(filter)
-			return nil, nil, fmt.Errorf("listing files under %s: %w", filter, err)
-		}
 
-		if len(files) == 0 {
+		files, err := repos.Files.ListUnderPath(ctx, filter)
-			unmatched = append(unmatched, filter)
+		if err != nil {
+			return nil, fmt.Errorf("listing files under %s: %w", filter, err)
 		}
 
 		for _, file := range files {
@@ -902,7 +871,7 @@ func (v *Vaultik) getFilesToRestore(
 		}
 	}
 
-	return result, unmatched, nil
+	return result, nil
 }
 
 // buildChunkToBlobMap creates a mapping from chunk hash to blob information

internal/vaultik/restore_unmatched_path_test.go

@@ -1,202 +0,0 @@
-package vaultik_test
-
-import (
-	"bytes"
-	"context"
-	"os"
-	"path/filepath"
-	"strings"
-	"testing"
-
-	"github.com/spf13/afero"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
-	"sneak.berlin/go/vaultik/internal/config"
-	"sneak.berlin/go/vaultik/internal/database"
-	"sneak.berlin/go/vaultik/internal/log"
-	"sneak.berlin/go/vaultik/internal/storage"
-	"sneak.berlin/go/vaultik/internal/storage/faultstore"
-	"sneak.berlin/go/vaultik/internal/ui"
-	"sneak.berlin/go/vaultik/internal/vaultik"
-)
-
-// These tests cover https://git.eeqj.de/sneak/vaultik/issues/309: a path
-// given to restore that matches nothing in the snapshot is named in a
-// warning and makes the restore fail, after the paths that do match are
-// restored.
-
-func TestRestoreNamesUnmatchedPathAndRestoresTheMatchingOne(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	fs := afero.NewOsFs()
-	tempDir := t.TempDir()
-	dataDir := filepath.Join(tempDir, "src")
-	storeDir := filepath.Join(tempDir, "remote")
-	restoreDir := filepath.Join(tempDir, "restored")
-	dbPath := filepath.Join(tempDir, "index.sqlite")
-
-	ctx := context.Background()
-	cfg := faultTestConfig()
-	files := writeFaultSourceTree(t, fs, dataDir)
-
-	store, err := storage.NewFileStorer(storeDir)
-	require.NoError(t, err)
-
-	db, err := database.New(ctx, dbPath)
-	require.NoError(t, err)
-
-	id := fullFaultBackup(ctx, t, fs, store, cfg,
-		database.NewRepositories(db), dataDir, dbPath, "unmatched")
-	require.NoError(t, db.Close())
-
-	matched := filepath.Join(dataDir, "small.txt")
-	unmatched := filepath.Join(dataDir, "typo.txt")
-
-	var out bytes.Buffer
-
-	v := newReaderVaultik(ctx, cfg, store, nil, fs)
-	v.UI = ui.NewWithColor(&out, false)
-
-	err = v.Restore(&vaultik.RestoreOptions{
-		SnapshotID: id,
-		TargetDir:  restoreDir,
-		Paths:      []string{matched, unmatched},
-	})
-
-	require.Error(t, err, "a path that matches nothing must fail the restore")
-	assert.Contains(t, out.String(), unmatched,
-		"the path that matches nothing must be named")
-	assert.NotContains(t, out.String(), matched,
-		"the path that matches must not be reported")
-
-	got, err := afero.ReadFile(fs, filepath.Join(restoreDir, matched))
-	require.NoError(t, err, "the path that matches must still be restored")
-	assert.Equal(t, files[matched], got)
-}
-
-// When no requested path matches, nothing is restored and each path is
-// named. One path is relative. The other goes through the symlink that
-// is the snapshot's configured path, while the entries are stored under
-// the directory the symlink points to.
-func TestRestoreNamesEveryPathWhenNothingMatches(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	const snapshotName = "linked"
-
-	fs := afero.NewOsFs()
-	tempDir := t.TempDir()
-	dataDir := filepath.Join(tempDir, "src")
-	linkDir := filepath.Join(tempDir, "srclink")
-	storeDir := filepath.Join(tempDir, "remote")
-	restoreDir := filepath.Join(tempDir, "restored")
-	dbPath := filepath.Join(tempDir, "index.sqlite")
-
-	ctx := context.Background()
-
-	writeFaultSourceTree(t, fs, dataDir)
-	require.NoError(t, os.Symlink(dataDir, linkDir))
-
-	cfg := faultTestConfig()
-	cfg.IndexPath = dbPath
-	cfg.ChunkSize = config.Size(faultChunkSize)
-	cfg.Snapshots = map[string]config.SnapshotConfig{
-		snapshotName: {Paths: []string{linkDir}},
-	}
-
-	store, err := storage.NewFileStorer(storeDir)
-	require.NoError(t, err)
-
-	db, err := database.New(ctx, dbPath)
-	require.NoError(t, err)
-
-	repos := database.NewRepositories(db)
-	v := newBackupVaultik(ctx, cfg, store, repos, db, fs)
-
-	require.NoError(t, backUp(v, snapshotName))
-
-	id := localSnapshotID(ctx, t, repos, snapshotName)
-	require.NoError(t, db.Close())
-
-	throughLink := filepath.Join(linkDir, "sub")
-	relative := strings.TrimPrefix(filepath.Join(dataDir, "small.txt"), "/")
-
-	var out bytes.Buffer
-
-	reader := newReaderVaultik(ctx, cfg, store, nil, fs)
-	reader.UI = ui.NewWithColor(&out, false)
-
-	err = reader.Restore(&vaultik.RestoreOptions{
-		SnapshotID: id,
-		TargetDir:  restoreDir,
-		Paths:      []string{throughLink, relative},
-	})
-
-	require.Error(t, err, "a restore that matches nothing must fail")
-	assert.Contains(t, out.String(), throughLink,
-		"the path through the symlink must be named")
-	assert.Contains(t, out.String(), relative,
-		"the relative path must be named")
-
-	_, err = fs.Stat(restoreDir)
-	assert.True(t, os.IsNotExist(err), "nothing must be restored")
-}
-
-// Without --skip-errors a damaged blob stops the restore part way. The
-// path that matches nothing must still be named.
-func TestRestoreNamesUnmatchedPathWhenADamagedBlobStopsIt(t *testing.T) {
-	log.Initialize(log.Config{})
-	t.Parallel()
-
-	fs := afero.NewOsFs()
-	tempDir := t.TempDir()
-	dataDir := filepath.Join(tempDir, "src")
-	storeDir := filepath.Join(tempDir, "remote")
-	restoreDir := filepath.Join(tempDir, "restored")
-	dbPath := filepath.Join(tempDir, "index.sqlite")
-
-	ctx := context.Background()
-	cfg := faultTestConfig()
-
-	writeFaultSourceTree(t, fs, dataDir)
-
-	inner, err := storage.NewFileStorer(storeDir)
-	require.NoError(t, err)
-
-	db, err := database.New(ctx, dbPath)
-	require.NoError(t, err)
-
-	id := fullFaultBackup(ctx, t, fs, inner, cfg,
-		database.NewRepositories(db), dataDir, dbPath, "damaged")
-	require.NoError(t, db.Close())
-
-	damaged := faultstore.New(inner)
-	damaged.OnGet = func(key string) faultstore.GetFault {
-		if strings.HasPrefix(key, "blobs/") {
-			return faultstore.GetCorrupt
-		}
-
-		return faultstore.GetNormal
-	}
-
-	matched := filepath.Join(dataDir, "small.txt")
-	unmatched := filepath.Join(dataDir, "typo.txt")
-
-	var out bytes.Buffer
-
-	v := newReaderVaultik(ctx, cfg, damaged, nil, fs)
-	v.UI = ui.NewWithColor(&out, false)
-
-	err = v.Restore(&vaultik.RestoreOptions{
-		SnapshotID: id,
-		TargetDir:  restoreDir,
-		Paths:      []string{matched, unmatched},
-	})
-
-	require.Error(t, err, "a damaged blob must stop the restore")
-	assert.Contains(t, err.Error(), "blob",
-		"the restore must stop on the damaged blob")
-	assert.Contains(t, out.String(), unmatched,
-		"the path that matches nothing must be named")
-}