fetch: client timeout, retry with backoff, url.JoinPath (closes #63)

fetch now makes every request through an http.Client with a time
limit, ten minutes by default and set with --timeout, which must be
greater than zero. A connection error, a timeout, or a 5xx or 429
response is retried, up to five tries in all, after a random wait
whose limit doubles from one second, or after the wait the server's
Retry-After asks for, up to one minute. Each try of a file starts a
new temp file, so a retry never keeps a partial file; the size and
hash checks are unchanged. Manifest and file URLs are built with
URL.JoinPath, so a base URL with or without a trailing slash or with
a query string, and names that need escaping, all work.

Model: opus-5-5

internal/cli/errmsg_test.go

@@ -272,10 +272,15 @@ func TestFetchManifestHTTPStatusMessage(t *testing.T) {
 		}))
 	defer server.Close()
 
-	set := flag.NewFlagSet("fetch", flag.ContinueOnError)
+	mfa := &CLIApp{Fs: afero.NewMemMapFs()}
+
+	set := flag.NewFlagSet(cmdFetch, flag.ContinueOnError)
+	for _, f := range mfa.fetchCommand().Flags {
+		require.NoError(t, f.Apply(set))
+	}
+
 	require.NoError(t, set.Parse([]string{server.URL}))
 
-	mfa := &CLIApp{Fs: afero.NewMemMapFs()}
 	ctx := urfcli.NewContext(nil, set, nil)
 
 	// fetchManifestOperation logs to the process-global logger.

internal/cli/fetch.go

@@ -71,6 +71,9 @@ var (
 	// errURLRequired indicates the fetch command was run without a URL
 	// argument.
 	errURLRequired = errors.New("URL argument required")
+	// errInvalidTimeout indicates a fetch --timeout of zero or less, which
+	// http.Client would take as no time limit at all.
+	errInvalidTimeout = errors.New("--timeout must be greater than zero")
 	// errEmptyPath indicates an empty file path in the manifest.
 	errEmptyPath = errors.New("empty path")
 	// errAbsolutePath indicates an absolute file path in the manifest.
@@ -273,6 +276,11 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error {
 		return errURLRequired
 	}
 
+	timeout := ctx.Duration(flagTimeout)
+	if timeout <= 0 {
+		return errInvalidTimeout
+	}
+
 	inputURL := ctx.Args().Get(0)
 
 	manifestURL, err := resolveManifestURL(inputURL)
@@ -281,7 +289,7 @@ func (mfa *CLIApp) fetchManifestOperation(ctx *cli.Context) error {
 	}
 
 	client := retryingClient{
-		client:     &http.Client{Timeout: ctx.Duration(flagTimeout)},
+		client:     &http.Client{Timeout: timeout},
 		firstDelay: firstRetryDelay,
 	}
 

internal/cli/fetch_test.go

@@ -881,3 +881,29 @@ func TestFetchTimeoutFlag(t *testing.T) {
 	err := runLocked(func() error { return mfa.fetchManifestOperation(cliCtx) })
 	require.Error(t, err)
 }
+
+// TestFetchRejectsTimeoutOfZeroOrLess runs fetch with a --timeout of zero
+// and of less than zero. http.Client takes either as no time limit at all,
+// so fetch must refuse it before making any request.
+func TestFetchRejectsTimeoutOfZeroOrLess(t *testing.T) {
+	t.Parallel()
+
+	var requests atomic.Int32
+
+	server := httptest.NewServer(
+		http.HandlerFunc(func(http.ResponseWriter, *http.Request) {
+			requests.Add(1)
+		}))
+	defer server.Close()
+
+	for _, timeout := range []string{"0", "-1s"} {
+		opts := testOpts(
+			[]string{testApp, cmdFetch, "--" + flagTimeout + "=" + timeout, server.URL},
+			afero.NewMemMapFs())
+
+		assert.Equal(t, 1, runCLI(opts), timeout)
+		assert.Contains(t, testStderr(t, opts), errInvalidTimeout.Error(), timeout)
+	}
+
+	assert.Zero(t, requests.Load(), "fetch made a request")
+}

internal/cli/freshen.go

@@ -629,7 +629,15 @@ func addExistingToBuilder(b *mfer.Builder, entry *mfer.MFFilePath) error {
 		return nil
 	}
 
-	return b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()),
+	err := b.AddFileWithHash(mfer.RelFilePath(entry.GetPath()),
 		mfer.FileSize(entry.GetSize()), mfer.ModTime(mtime),
 		entry.GetHashes()[0].GetMultiHash())
+	if err != nil {
+		return fmt.Errorf(
+			"manifest entry %s: %w (regenerate the manifest with mfer generate)",
+			entry.GetPath(), err,
+		)
+	}
+
+	return nil
 }

internal/cli/freshen_test.go

@@ -4,11 +4,13 @@ package cli
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
 	"os"
 	"path/filepath"
 	"testing"
 	"time"
 
+	"github.com/multiformats/go-multihash"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -234,21 +236,50 @@ func TestFreshenRecordEntryMtimePresence(t *testing.T) {
 func TestFreshenAddExistingRejectsMissingMtime(t *testing.T) {
 	t.Parallel()
 
+	hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
+
 	b := mfer.NewBuilder()
 	entry := &mfer.MFFilePath{
 		Path:  "file1.txt",
 		Size:  8,
 		Mtime: nil,
 		Hashes: []*mfer.MFFileChecksum{
-			{MultiHash: []byte{0x12, 0x20}},
+			{MultiHash: hash},
 		},
 	}
 
-	err := addExistingToBuilder(b, entry)
+	err = addExistingToBuilder(b, entry)
 	require.ErrorIs(t, err, errEntryMissingMtime)
 	assert.Contains(t, err.Error(), "file1.txt")
 }
 
+// TestFreshenAddExistingRejectsShortHash pins that an existing manifest
+// entry whose hash the builder refuses is reported as a problem with the
+// manifest, with the command that fixes it.
+func TestFreshenAddExistingRejectsShortHash(t *testing.T) {
+	t.Parallel()
+
+	sha1Hash, err := multihash.Encode(make([]byte, 20), multihash.SHA1)
+	require.NoError(t, err)
+
+	b := mfer.NewBuilder()
+	entry := &mfer.MFFilePath{
+		Path:  "old.txt",
+		Size:  8,
+		Mtime: &mfer.Timestamp{Seconds: 1_700_000_000},
+		Hashes: []*mfer.MFFileChecksum{
+			{MultiHash: sha1Hash},
+		},
+	}
+
+	err = addExistingToBuilder(b, entry)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "manifest entry old.txt")
+	assert.Contains(t, err.Error(), "mfer generate")
+	assert.Zero(t, b.FileCount())
+}
+
 // TestEntryMtime pins the presence semantics the callers depend on.
 func TestEntryMtime(t *testing.T) {
 	t.Parallel()

internal/cli/manifest_loader.go

@@ -7,10 +7,14 @@ import (
 	"io"
 	"net/http"
 	"strings"
+	"time"
 
 	"github.com/urfave/cli/v2"
 )
 
+// manifestFetchTimeout bounds HTTP requests made to fetch a manifest.
+const manifestFetchTimeout = 30 * time.Second
+
 // errHTTPStatus indicates an HTTP response with a non-OK status code.
 //
 // Its text is the literal "HTTP" prefix of the rendered "HTTP <code>"
@@ -28,7 +32,7 @@ func isHTTPURL(s string) bool {
 // The caller must close the returned reader.
 func (mfa *CLIApp) openManifestReader(pathOrURL string) (io.ReadCloser, error) {
 	if isHTTPURL(pathOrURL) {
-		client := &http.Client{Timeout: httpTimeout}
+		client := &http.Client{Timeout: manifestFetchTimeout}
 
 		req, err := http.NewRequestWithContext(
 			context.Background(), http.MethodGet, pathOrURL, nil,

mfer/builder.go

@@ -35,7 +35,8 @@ var (
 	errPathDotDot       = errors.New("contains '..' segment")
 	errSizeMismatch     = errors.New("size mismatch")
 	errNegativeSize     = errors.New("size cannot be negative")
-	errEmptyHash        = errors.New("hash cannot be nil or empty")
+	errHashNotMultihash = errors.New("hash is not a valid multihash")
+	errHashTooShort     = errors.New("hash digest is too short")
 )
 
 // ValidatePath checks that a file path conforms to manifest path invariants:
@@ -228,7 +229,8 @@ func (b *Builder) FileCount() int {
 
 // AddFileWithHash adds a file entry with a pre-computed hash.
 // This is useful when the hash is already known (e.g., from an existing manifest).
-// Returns an error if path is empty, size is negative, or hash is nil/empty.
+// Returns an error if path is invalid, size is negative, or hash is not a
+// multihash with a digest of at least 32 bytes, as long as SHA-256's.
 func (b *Builder) AddFileWithHash(
 	path RelFilePath,
 	size FileSize,
@@ -244,8 +246,19 @@ func (b *Builder) AddFileWithHash(
 		return errNegativeSize
 	}
 
-	if len(hash) == 0 {
-		return errEmptyHash
+	decoded, err := multihash.Decode(hash)
+	if err != nil {
+		return fmt.Errorf("%w: %w", errHashNotMultihash, err)
+	}
+
+	// The reader's limit on decoding cost (maxDecodedGrowth) assumes every
+	// hash is at least as long as a SHA-256 multihash, so a manifest of
+	// shorter ones could fail to load.
+	if len(decoded.Digest) < sha256.Size {
+		return fmt.Errorf(
+			"%w: %d bytes, at least %d needed",
+			errHashTooShort, len(decoded.Digest), sha256.Size,
+		)
 	}
 
 	entry := &MFFilePath{

mfer/builder_test.go

@@ -4,12 +4,14 @@ package mfer
 import (
 	"bytes"
 	"context"
+	"crypto/sha256"
 	"fmt"
 	"path/filepath"
 	"strings"
 	"testing"
 	"time"
 
+	"github.com/multiformats/go-multihash"
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -44,9 +46,10 @@ func TestBuilderAddFileWithHash(t *testing.T) {
 	t.Parallel()
 
 	b := NewBuilder()
-	hash := make([]byte, 34) // SHA256 multihash is 34 bytes
+	hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
 
-	err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), hash)
+	err = b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), hash)
 	require.NoError(t, err)
 	assert.Equal(t, 1, b.FileCount())
 }
@@ -54,12 +57,14 @@ func TestBuilderAddFileWithHash(t *testing.T) {
 func TestBuilderAddFileWithHashValidation(t *testing.T) {
 	t.Parallel()
 
+	sha256Hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
+
 	t.Run("empty path", func(t *testing.T) {
 		t.Parallel()
 
 		b := NewBuilder()
-		hash := make([]byte, 34)
-		err := b.AddFileWithHash("", 100, ModTime(time.Now()), hash)
+		err := b.AddFileWithHash("", 100, ModTime(time.Now()), sha256Hash)
 		require.Error(t, err)
 		assert.Contains(t, err.Error(), "path")
 	})
@@ -68,41 +73,58 @@ func TestBuilderAddFileWithHashValidation(t *testing.T) {
 		t.Parallel()
 
 		b := NewBuilder()
-		hash := make([]byte, 34)
-		err := b.AddFileWithHash("test.txt", -1, ModTime(time.Now()), hash)
+		err := b.AddFileWithHash("test.txt", -1, ModTime(time.Now()), sha256Hash)
 		require.Error(t, err)
 		assert.Contains(t, err.Error(), "size")
 	})
 
-	t.Run("nil hash", func(t *testing.T) {
-		t.Parallel()
-
-		b := NewBuilder()
-		err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), nil)
-		require.Error(t, err)
-		assert.Contains(t, err.Error(), "hash")
-	})
-
-	t.Run("empty hash", func(t *testing.T) {
-		t.Parallel()
-
-		b := NewBuilder()
-		err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), []byte{})
-		require.Error(t, err)
-		assert.Contains(t, err.Error(), "hash")
-	})
-
 	t.Run("valid inputs", func(t *testing.T) {
 		t.Parallel()
 
 		b := NewBuilder()
-		hash := make([]byte, 34)
-		err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), hash)
+		err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), sha256Hash)
 		require.NoError(t, err)
 		assert.Equal(t, 1, b.FileCount())
 	})
 }
 
+func TestBuilderAddFileWithHashRejectsBadHashes(t *testing.T) {
+	t.Parallel()
+
+	sha1Hash, err := multihash.Encode(make([]byte, 20), multihash.SHA1)
+	require.NoError(t, err)
+
+	truncatedHash, err := multihash.Encode(make([]byte, sha256.Size-1), multihash.SHA2_256)
+	require.NoError(t, err)
+
+	tests := []struct {
+		name string
+		hash Multihash
+		want error
+	}{
+		{"nil hash", nil, errHashNotMultihash},
+		{"empty hash", []byte{}, errHashNotMultihash},
+		{"one-byte hash", []byte{0x12}, errHashNotMultihash},
+		// A SHA-256 code and 32-byte length, then only two bytes of digest.
+		{"malformed multihash", []byte{0x12, 0x20, 0x01, 0x02}, errHashNotMultihash},
+		// A valid multihash, but its 20-byte SHA-1 digest is too short.
+		{"SHA-1 multihash", sha1Hash, errHashTooShort},
+		// A valid multihash whose 31-byte digest is one byte short.
+		{"31-byte digest", truncatedHash, errHashTooShort},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			b := NewBuilder()
+			err := b.AddFileWithHash("test.txt", 100, ModTime(time.Now()), tt.hash)
+			require.ErrorIs(t, err, tt.want)
+			assert.Equal(t, 0, b.FileCount())
+		})
+	}
+}
+
 func TestBuilderBuild(t *testing.T) {
 	t.Parallel()
 
@@ -358,7 +380,8 @@ func TestBuilderBuildRoundTrip(t *testing.T) {
 func TestBuilderBuildRoundTripLargeManifest(t *testing.T) {
 	t.Parallel()
 
-	hash := make([]byte, 34) // multihash: 2-byte prefix + 32-byte SHA-256
+	hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
 
 	b := NewBuilder()
 

mfer/constants.go

@@ -36,7 +36,8 @@ const (
 	decodedMIMETypeSize  = 16
 
 	// Each file entry mfer writes holds a path of at least one byte, a
-	// 34-byte SHA-256 multihash and a modification time: at least 47 bytes,
+	// multihash at least as long as SHA-256's 34 bytes (AddFileWithHash
+	// refuses shorter ones) and a modification time: at least 47 bytes,
 	// counted at 336. So its manifests add up to at most about 7.15 times
 	// their size, and this limit is about 12% above that.
 	maxDecodedGrowth = 8

mfer/deserialize_path_test.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/google/uuid"
 	"github.com/klauspost/compress/zstd"
+	"github.com/multiformats/go-multihash"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/protobuf/encoding/protowire"
@@ -205,7 +206,8 @@ func TestDeserializeDropsUnknownFields(t *testing.T) {
 func TestDeserializeLoadsDensestManifest(t *testing.T) {
 	t.Parallel()
 
-	hash := make([]byte, 34) // multihash: 2-byte prefix + 32-byte SHA-256
+	hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
 
 	b := NewBuilder()
 	b.SetIncludeTimestamps(true)
@@ -227,7 +229,8 @@ func TestDeserializeLoadsDensestManifest(t *testing.T) {
 func TestDeserializeValidManifestRoundTrips(t *testing.T) {
 	t.Parallel()
 
-	hash := make([]byte, 34) // multihash: 2-byte prefix + 32-byte SHA-256
+	hash, err := multihash.Encode(make([]byte, sha256.Size), multihash.SHA2_256)
+	require.NoError(t, err)
 
 	b := NewBuilder()
 	require.NoError(t, b.AddFileWithHash("dir/file.txt", 123, ModTime{}, hash))