From 6a3f532c8d0163576ccfb2761b5c6f6714a0e8c0 Mon Sep 17 00:00:00 2001
From: clawbot <35+clawbot@noreply.example.org>
Date: Wed, 14 Oct 2026 01:55:41 +0200
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.

A new test covers the context ending during the request, and
TestGetStopsWaitingWhenCanceled now covers it ending during the wait.
Both answer through a transport rather than a server, so neither depends
on how fast the 503 arrives.

Model: opus-5-5
---
 internal/cli/fetch.go      | 13 ++++++---
 internal/cli/fetch_test.go | 56 +++++++++++++++++++++++++++++++-------
 2 files changed, 55 insertions(+), 14 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..bb4b053 100644
--- a/internal/cli/fetch_test.go
+++ b/internal/cli/fetch_test.go
@@ -897,23 +897,59 @@ func TestGetHonorsRetryAfter(t *testing.T) {
 	}
 }
 
-// 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.
+// instant503Transport answers every request with a 503 at once, so the
+// 503 is in hand before any deadline can pass.
+type instant503Transport struct{}
+
+func (instant503Transport) RoundTrip(*http.Request) (*http.Response, error) {
+	return &http.Response{
+		StatusCode: http.StatusServiceUnavailable,
+		Body:       http.NoBody,
+	}, nil
+}
+
+// TestGetStopsWaitingWhenCanceled answers every request with a 503 and
+// sets an hour's wait before each retry, then lets the context expire
+// during that wait. get returning at all shows it stopped waiting; it must
+// return the deadline error and keep the 503 with it.
 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.client.Transport = instant503Transport{}
 	client.firstDelay = time.Hour
 
-	err := getNothing(client, server.URL, 50*time.Millisecond)
+	err := getNothing(client, "http://example.invalid/", 50*time.Millisecond)
 	require.ErrorIs(t, err, context.DeadlineExceeded)
+	require.ErrorIs(t, err, errHTTPStatus)
+}
+
+// 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