From 07ce546909a2108f65be18bcf3500342e9017b15 Mon Sep 17 00:00:00 2001
From: sneak <sneak@sneak.berlin>
Date: Tue, 13 Oct 2026 22:39:29 +0000
Subject: [PATCH] Return the context's error when it ends during a fetch
 request (closes #221)

retryingClient.get checked its context right after each request and
returned that request's error, so a deadline that passed while a 503 was
on its way came back as the 503. TestGetStopsWaitingWhenCanceled failed
on that now and then on a loaded host. When the context ends before a
retry, during the request or during the wait, get now returns an error
wrapping both the context's error and the last try's error.

The new test answers through a transport that holds the 503 until the
context has ended. A real server cannot do that: the client drops a
response still on its way when the context ends.

Model: opus-5-5
---
 internal/cli/fetch.go      | 13 +++++++++----
 internal/cli/fetch_test.go | 28 ++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)

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.