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 trailing slashes, query strings and names that need
escaping all work.

Model: opus-5-5

internal/cli/fetch_test.go

@@ -4,16 +4,24 @@ package cli
 import (
 	"bytes"
 	"context"
+	"flag"
+	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"strconv"
+	"sync"
+	"sync/atomic"
 	"testing"
+	"time"
 
 	"github.com/spf13/afero"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	urfcli "github.com/urfave/cli/v2"
 	"sneak.berlin/go/mfer/mfer"
 )
 
@@ -214,6 +222,41 @@ func fetchTestHandler(
 	}
 }
 
+// manifestOf scans a tree holding files and returns its manifest bytes.
+func manifestOf(t *testing.T, files map[string][]byte) []byte {
+	t.Helper()
+
+	sourceFs := afero.NewMemMapFs()
+	for p, content := range files {
+		require.NoError(t, sourceFs.MkdirAll(filepath.Dir("/"+p), 0o755))
+		require.NoError(t, afero.WriteFile(sourceFs, "/"+p, content, 0o644))
+	}
+
+	return scanToManifest(t, sourceFs)
+}
+
+// testClient returns the client tests download with. Its retries wait
+// milliseconds rather than seconds.
+func testClient() retryingClient {
+	return retryingClient{
+		client:     &http.Client{Timeout: 10 * time.Second},
+		firstDelay: time.Millisecond,
+	}
+}
+
+// getNothing calls client.get for rawURL with a use that reads nothing,
+// under a context that expires after timeout. It holds runMu, since get
+// logs each retry to the process-global logger, and starts the timeout
+// only once it has the lock.
+func getNothing(client retryingClient, rawURL string, timeout time.Duration) error {
+	return runLocked(func() error {
+		ctx, cancel := context.WithTimeout(context.Background(), timeout)
+		defer cancel()
+
+		return client.get(ctx, rawURL, func(*http.Response) error { return nil })
+	})
+}
+
 //nolint:paralleltest // changes the process-global working directory
 func TestFetchFromHTTP(t *testing.T) {
 	// Create source filesystem with test files
@@ -266,7 +309,8 @@ func TestFetchFromHTTP(t *testing.T) {
 		require.NoError(t, err)
 
 		fileURL := baseURL + f.GetPath()
-		err = downloadFile(context.Background(), fileURL, localPath, f, progress)
+		err = downloadFile(context.Background(), testClient(),
+			fileURL, localPath, f, progress)
 		require.NoError(t, err, "failed to download %s", f.GetPath())
 	}
 
@@ -313,7 +357,7 @@ func TestFetchHashMismatch(t *testing.T) {
 	chdirTemp(t)
 
 	// Try to download - should fail with hash mismatch
-	err = downloadFile(context.Background(),
+	err = downloadFile(context.Background(), testClient(),
 		server.URL+"/file.txt", testFileTxt, files[0], nil)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "mismatch")
@@ -359,7 +403,7 @@ func TestFetchSizeMismatch(t *testing.T) {
 	chdirTemp(t)
 
 	// Try to download - should fail with size mismatch
-	err = downloadFile(context.Background(),
+	err = downloadFile(context.Background(), testClient(),
 		server.URL+"/file.txt", testFileTxt, files[0], nil)
 	require.Error(t, err)
 	assert.Contains(t, err.Error(), "size mismatch")
@@ -417,7 +461,7 @@ func TestFetchProgress(t *testing.T) {
 	}()
 
 	// Download
-	err = downloadFile(context.Background(),
+	err = downloadFile(context.Background(), testClient(),
 		server.URL+"/large.txt", "large.txt", files[0], progress)
 	close(progress)
 	<-done
@@ -523,3 +567,343 @@ func TestFetchReplacesHardLinkAtTempName(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, "outside", string(outside), "fetch wrote outside the destination")
 }
+
+// TestGetRetriesTransientStatusesOnly answers every request with one
+// status and counts the requests: a 5xx or 429 is retried until
+// fetchAttempts runs out, and any other status fails at once.
+func TestGetRetriesTransientStatusesOnly(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		status   int
+		requests int32
+	}{
+		{http.StatusNotFound, 1},
+		{http.StatusForbidden, 1},
+		{http.StatusInternalServerError, fetchAttempts},
+		{http.StatusServiceUnavailable, fetchAttempts},
+		{http.StatusTooManyRequests, fetchAttempts},
+	}
+
+	for _, tt := range tests {
+		t.Run(http.StatusText(tt.status), func(t *testing.T) {
+			t.Parallel()
+
+			var requests atomic.Int32
+
+			server := httptest.NewServer(
+				http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+					requests.Add(1)
+					w.WriteHeader(tt.status)
+				}))
+			defer server.Close()
+
+			err := getNothing(testClient(), server.URL, 10*time.Second)
+			require.ErrorIs(t, err, errHTTPStatus)
+			require.EqualError(t, err, fmt.Sprintf("HTTP %d", tt.status))
+			assert.Equal(t, tt.requests, requests.Load())
+		})
+	}
+}
+
+// TestGetTimesOutOnStalledServer points get at a server that takes a
+// request and never answers it. The try must give up at the client's
+// timeout and be retried; when every try stalls, get must return a
+// timeout error rather than hang.
+func TestGetTimesOutOnStalledServer(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name       string
+		stallEvery bool
+	}{
+		{"first request stalls", false},
+		{"every request stalls", true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			var requests atomic.Int32
+
+			stop := make(chan struct{})
+			server := httptest.NewServer(
+				http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
+					if requests.Add(1) == 1 || tt.stallEvery {
+						select {
+						case <-r.Context().Done():
+						case <-stop:
+						}
+					}
+				}))
+
+			defer server.Close()
+			defer close(stop)
+
+			client := testClient()
+			client.client.Timeout = 50 * time.Millisecond
+
+			err := getNothing(client, server.URL, 10*time.Second)
+			if !tt.stallEvery {
+				require.NoError(t, err)
+
+				return
+			}
+
+			var netErr net.Error
+			require.ErrorAs(t, err, &netErr)
+			assert.True(t, netErr.Timeout(), "want a timeout, got %v", err)
+		})
+	}
+}
+
+// TestGetHonorsRetryAfter answers the first request with a 503 and a
+// Retry-After header, and any later one with 200 OK. The client's own
+// wait before a retry is an hour, so get finishes in time only if it
+// waits as long as Retry-After says instead. A Retry-After longer than
+// maxRetryAfter must fail at once.
+func TestGetHonorsRetryAfter(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		value    string
+		requests int32
+		wantErr  bool
+	}{
+		{"seconds", "0", 2, false},
+		{"HTTP date", time.Now().Add(-time.Minute).UTC().Format(http.TimeFormat), 2, false},
+		{"too long", strconv.Itoa(int((maxRetryAfter + time.Second).Seconds())), 1, true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+
+			var requests atomic.Int32
+
+			server := httptest.NewServer(
+				http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+					if requests.Add(1) == 1 {
+						w.Header().Set("Retry-After", tt.value)
+						w.WriteHeader(http.StatusServiceUnavailable)
+					}
+				}))
+			defer server.Close()
+
+			client := testClient()
+			client.firstDelay = time.Hour
+
+			err := getNothing(client, server.URL, 10*time.Second)
+			if tt.wantErr {
+				require.ErrorIs(t, err, errHTTPStatus)
+			} else {
+				require.NoError(t, err)
+			}
+
+			assert.Equal(t, tt.requests, requests.Load())
+		})
+	}
+}
+
+// TestGetStopsWaitingWhenCanceled gives get a server that always answers
+// 503 and an hour's wait before each retry, then lets the context expire
+// during that wait. get returning at all shows it stopped waiting.
+func TestGetStopsWaitingWhenCanceled(t *testing.T) {
+	t.Parallel()
+
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			w.WriteHeader(http.StatusServiceUnavailable)
+		}))
+	defer server.Close()
+
+	client := testClient()
+	client.firstDelay = time.Hour
+
+	err := getNothing(client, server.URL, 50*time.Millisecond)
+	require.ErrorIs(t, err, context.DeadlineExceeded)
+}
+
+// TestDownloadFileRetriesToSuccess serves a file that fails twice before
+// it succeeds: first with a 503, then with a body cut off halfway. The
+// download must end with the whole, verified file and no temp file.
+//
+//nolint:paralleltest // changes the process-global working directory
+func TestDownloadFileRetriesToSuccess(t *testing.T) {
+	content := []byte("the whole file, every byte of it")
+
+	manifest, err := mfer.NewManifestFromReader(bytes.NewReader(
+		manifestOf(t, map[string][]byte{testFileTxt: content})))
+	require.NoError(t, err)
+
+	var requests atomic.Int32
+
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+			switch requests.Add(1) {
+			case 1:
+				w.WriteHeader(http.StatusServiceUnavailable)
+			case 2:
+				// Promise the whole file but send half of it; the server
+				// then closes the connection.
+				w.Header().Set("Content-Length", strconv.Itoa(len(content)))
+				_, _ = w.Write(content[:len(content)/2])
+			default:
+				_, _ = w.Write(content)
+			}
+		}))
+	defer server.Close()
+
+	chdirTemp(t)
+
+	err = downloadFile(context.Background(), testClient(),
+		server.URL+"/"+testFileTxt, testFileTxt, manifest.Files()[0], nil)
+	require.NoError(t, err)
+	assert.Equal(t, int32(3), requests.Load())
+
+	fetched, err := os.ReadFile(testFileTxt)
+	require.NoError(t, err)
+	assert.Equal(t, content, fetched)
+
+	_, err = os.Stat(".file.txt.tmp")
+	assert.True(t, os.IsNotExist(err), "temp file left behind")
+}
+
+// TestFetchBaseURLForms fetches one tree by its directory URL with and
+// without a trailing slash, with a query string, and by its manifest URL.
+// Each must request the same manifest and file paths.
+//
+//nolint:paralleltest // changes the process-global working directory
+func TestFetchBaseURLForms(t *testing.T) {
+	files := map[string][]byte{"one.txt": []byte("1"), "sub/two.txt": []byte("2")}
+	tree := http.StripPrefix("/tree", fetchTestHandler(manifestOf(t, files), files))
+
+	var (
+		mu        sync.Mutex
+		requested []string
+	)
+
+	server := httptest.NewServer(
+		http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+			mu.Lock()
+
+			requested = append(requested, r.URL.Path)
+
+			mu.Unlock()
+
+			tree.ServeHTTP(w, r)
+		}))
+	defer server.Close()
+
+	inputs := []string{"/tree", "/tree/", "/tree?key=value", "/tree/index.mf"}
+	for _, input := range inputs {
+		t.Run(input, func(t *testing.T) {
+			mu.Lock()
+			requested = nil
+			mu.Unlock()
+
+			chdirTemp(t)
+
+			opts := testOpts([]string{testApp, cmdFetch, "-q", server.URL + input},
+				afero.NewOsFs())
+			require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+
+			mu.Lock()
+			defer mu.Unlock()
+
+			assert.ElementsMatch(t,
+				[]string{"/tree/index.mf", "/tree/one.txt", "/tree/sub/two.txt"}, requested)
+		})
+	}
+}
+
+// TestFetchEscapedPaths fetches files whose names need escaping in a URL,
+// one of them a name that is itself an escape sequence. Each must arrive
+// under its own name with its own content.
+//
+//nolint:paralleltest // changes the process-global working directory
+func TestFetchEscapedPaths(t *testing.T) {
+	files := map[string][]byte{
+		"a b.txt":                []byte("space"),
+		"a%20b.txt":              []byte("percent sign, two, zero"),
+		"100%.txt":               []byte("percent sign"),
+		"q?x=1#frag.txt":         []byte("question mark and hash"),
+		"dir with space/sub.txt": []byte("directory with a space"),
+	}
+
+	server := httptest.NewServer(fetchTestHandler(manifestOf(t, files), files))
+	defer server.Close()
+
+	chdirTemp(t)
+
+	opts := testOpts([]string{testApp, cmdFetch, "-q", server.URL}, afero.NewOsFs())
+	require.Equal(t, 0, runCLI(opts), testStderr(t, opts))
+
+	for name, content := range files {
+		fetched, err := os.ReadFile(name) //nolint:gosec // test-controlled path
+		require.NoError(t, err)
+		assert.Equal(t, content, fetched, name)
+	}
+}
+
+// TestFetchTimeoutFlag runs fetch with --timeout against a server that
+// never answers. Without the flag's limit the request would wait forever;
+// once fetch gives up on it, the server cancels fetch's context so that
+// fetch returns instead of retrying.
+func TestFetchTimeoutFlag(t *testing.T) {
+	t.Parallel()
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
+	server := httptest.NewServer(
+		http.HandlerFunc(func(_ http.ResponseWriter, r *http.Request) {
+			<-r.Context().Done() // fetch gave up on this request
+			cancel()
+		}))
+	defer server.Close()
+
+	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{"--" + flagTimeout, "100ms", server.URL}))
+
+	cliCtx := urfcli.NewContext(nil, set, nil)
+	cliCtx.Context = ctx
+
+	// fetchManifestOperation logs to the process-global logger.
+	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")
+}