diff --git a/internal/cli/fetch.go b/internal/cli/fetch.go index 57b8d27..d2497f2 100644 --- a/internal/cli/fetch.go +++ b/internal/cli/fetch.go @@ -126,8 +126,9 @@ type retryingClient struct { // to a limit that doubles each time, unless the server's Retry-After // header says how long to wait. Any other failure, an error from use // included, is returned at once and unwrapped; a non-OK status is -// returned as errHTTPStatus. use must start over each time it is called, -// since a retry calls it again with a new response. +// returned as errHTTPStatus. If ctx ends before a retry, the error wraps +// both ctx's error and the last try's error. use must start over each +// time it is called, since a retry calls it again with a new response. func (c retryingClient) get( ctx context.Context, rawURL string, use func(*http.Response) error, ) error { @@ -169,15 +170,19 @@ func (c retryingClient) get( } } - if !retry || attempt == fetchAttempts || ctx.Err() != nil { + if !retry || attempt == fetchAttempts { return err } + if ctx.Err() != nil { + return fmt.Errorf("%w: %w", ctx.Err(), err) + } + log.Warnf("%s: %s, retrying in %s", rawURL, err, wait.Round(time.Millisecond)) select { case <-ctx.Done(): - return ctx.Err() + return fmt.Errorf("%w: %w", ctx.Err(), err) case <-time.After(wait): } diff --git a/internal/cli/fetch_test.go b/internal/cli/fetch_test.go index 975f333..e174aeb 100644 --- a/internal/cli/fetch_test.go +++ b/internal/cli/fetch_test.go @@ -916,6 +916,34 @@ func TestGetStopsWaitingWhenCanceled(t *testing.T) { require.ErrorIs(t, err, context.DeadlineExceeded) } +// late503Transport answers every request with a 503, but only once the +// request's context has ended. A real server cannot stand in for it: the +// client drops a response still on its way when the context ends. +type late503Transport struct{} + +func (late503Transport) RoundTrip(req *http.Request) (*http.Response, error) { + <-req.Context().Done() + + return &http.Response{ + StatusCode: http.StatusServiceUnavailable, + Body: http.NoBody, + }, nil +} + +// TestGetStopsWhenContextEndsDuringRequest lets the context expire while +// the first request is still waiting for its 503. get must return the +// deadline error and keep the 503 with it. +func TestGetStopsWhenContextEndsDuringRequest(t *testing.T) { + t.Parallel() + + client := testClient() + client.client.Transport = late503Transport{} + + err := getNothing(client, "http://example.invalid/", 50*time.Millisecond) + require.ErrorIs(t, err, context.DeadlineExceeded) + require.ErrorIs(t, err, errHTTPStatus) +} + // 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.