rickub
nandi/gleanpublic Fork
⑂ fdcbe24
Branches
claude/modal-container-min-76527a main View all branches → View all tags →
Commits
⬇ Clone ▾
git clone https://git.rickub.com/nandi/glean.git
git clone ssh://git@rickub.com/nandi/glean.git

Host key fingerprint (ed25519): SHA256:iycHnxEyq0Q7uyVpB7JlznP0G7JrTPXLYRcAU5CSLhc — verify it before your first connect.

Download ZIP

Fix the feed fetcher's retry policy

executeRequest returned its *http.Response only on success, so in Fetch the
guard

    if resp != nil && !httpclient.IsRetryable(resp.StatusCode)

could never be true when err != nil: the response was always nil on the
error path. Three things followed.

Every failure retried the full four attempts whatever the status, so a
permanently-404 feed cost four requests and about seven seconds of backoff
on every refresh cycle. retryBackoff's Retry-After handling was unreachable
for the same reason -- lastResp only became non-nil once the function had
already returned -- so a rate-limited server's own requested delay was never
honoured. And the status was never checked at all, so an error page was
handed to the parser and a 410 surfaced in the logs as "unable to detect
feed format", which is why the real cause was not obvious.

The fix returns the response on the error paths too, checks the status, and
marks a body that arrived intact and was not a feed as not worth retrying.
The response body is closed by the existing defer; StatusCode and Header
stay readable, which is all the caller needs. That also makes the explicit
lastResp.Body.Close() in the loop redundant, so it goes.

The existing tests exercised retryBackoff in isolation with a hand-built
response, which is how the policy came to be wrong while its unit tests
passed: nothing checked that retryBackoff was ever reached with a non-nil
response. The new tests drive Fetch against an httptest server and assert
request counts. Confirmed to fail against the previous code -- 404 and
unparseable bodies were each requested 4 times, and the 410 case reported a
parse failure.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
nandithebull committed 2026-09-21T18:27:49-07:00 Browse files
fdcbe24 parent: 3e65fa0
modified internal/feed/fetcher.go +22 -5
@@ -2,6 +2,7 @@ package feed
22
33 import (
44 "context"
5+ "errors"
56 "fmt"
67 "log/slog"
78 "net/http"
@@ -19,6 +20,10 @@ const (
1920 baseRetryDelay = 1 * time.Second
2021 )
2122
23+// errUnparseable marks a response that arrived intact and was not a feed, as
24+// opposed to one that never arrived. Only the latter is worth retrying.
25+var errUnparseable = errors.New("response was not a feed")
26+
2227 type ATProtoFetcher interface {
2328 FetchFeed(ctx context.Context, feedURL string) (*ParseResult, error)
2429 }
@@ -60,20 +65,23 @@ func (f *Fetcher) Fetch(ctx context.Context, feedURL string) (*ParseResult, erro
6065 }
6166 }
6267
63- if lastResp != nil {
64- lastResp.Body.Close()
65- }
66-
6768 result, resp, err := f.executeRequest(ctx, feedURL)
6869 lastResp = resp
6970 if err == nil {
7071 return result, nil
7172 }
7273
74+ // A status the server has settled on will not change on a retry.
7375 if resp != nil && !httpclient.IsRetryable(resp.StatusCode) {
7476 return nil, err
7577 }
7678
79+ // Nor will a body that did not parse. Retrying it costs three more
80+ // requests to reach the same conclusion.
81+ if errors.Is(err, errUnparseable) {
82+ return nil, err
83+ }
84+
7785 lastErr = err
7886 }
7987
@@ -119,9 +127,18 @@ func (f *Fetcher) executeRequest(ctx context.Context, feedURL string) (*ParseRes
119127 }
120128 defer resp.Body.Close()
121129
130+ // The response is returned on the error paths too, not just on success.
131+ // It is what lets the caller tell a 503 worth retrying from a 404 that is
132+ // not; returning nil here made that check unreachable and every failure
133+ // cost the full retry budget. The body is closed by the defer above, but
134+ // StatusCode and Header stay readable, which is all the caller needs.
135+ if resp.StatusCode < 200 || resp.StatusCode >= 300 {
136+ return nil, resp, fmt.Errorf("fetching feed: unexpected status %s", resp.Status)
137+ }
138+
122139 result, err := Parse(resp.Body, feedURL)
123140 if err != nil {
124- return nil, nil, fmt.Errorf("parsing feed: %w", err)
141+ return nil, resp, fmt.Errorf("%w: %v", errUnparseable, err)
125142 }
126143
127144 return result, resp, nil
@@ -2,6 +2,7 @@ package feed
2 2
3 import (3 import (
4 "context"4 "context"
5+ "errors"
5 "fmt"6 "fmt"
6 "log/slog"7 "log/slog"
7 "net/http"8 "net/http"
@@ -19,6 +20,10 @@ const (
19 baseRetryDelay = 1 * time.Second20 baseRetryDelay = 1 * time.Second
20 )21 )
21 22
23+// errUnparseable marks a response that arrived intact and was not a feed, as
24+// opposed to one that never arrived. Only the latter is worth retrying.
25+var errUnparseable = errors.New("response was not a feed")
26+
22 type ATProtoFetcher interface {27 type ATProtoFetcher interface {
23 FetchFeed(ctx context.Context, feedURL string) (*ParseResult, error)28 FetchFeed(ctx context.Context, feedURL string) (*ParseResult, error)
24 }29 }
@@ -60,20 +65,23 @@ func (f *Fetcher) Fetch(ctx context.Context, feedURL string) (*ParseResult, erro
60 }65 }
61 }66 }
62 67
63- if lastResp != nil {
64- lastResp.Body.Close()
65- }
66-
67 result, resp, err := f.executeRequest(ctx, feedURL)68 result, resp, err := f.executeRequest(ctx, feedURL)
68 lastResp = resp69 lastResp = resp
69 if err == nil {70 if err == nil {
70 return result, nil71 return result, nil
71 }72 }
72 73
74+ // A status the server has settled on will not change on a retry.
73 if resp != nil && !httpclient.IsRetryable(resp.StatusCode) {75 if resp != nil && !httpclient.IsRetryable(resp.StatusCode) {
74 return nil, err76 return nil, err
75 }77 }
76 78
79+ // Nor will a body that did not parse. Retrying it costs three more
80+ // requests to reach the same conclusion.
81+ if errors.Is(err, errUnparseable) {
82+ return nil, err
83+ }
84+
77 lastErr = err85 lastErr = err
78 }86 }
79 87
@@ -119,9 +127,18 @@ func (f *Fetcher) executeRequest(ctx context.Context, feedURL string) (*ParseRes
119 }127 }
120 defer resp.Body.Close()128 defer resp.Body.Close()
121 129
130+ // The response is returned on the error paths too, not just on success.
131+ // It is what lets the caller tell a 503 worth retrying from a 404 that is
132+ // not; returning nil here made that check unreachable and every failure
133+ // cost the full retry budget. The body is closed by the defer above, but
134+ // StatusCode and Header stay readable, which is all the caller needs.
135+ if resp.StatusCode < 200 || resp.StatusCode >= 300 {
136+ return nil, resp, fmt.Errorf("fetching feed: unexpected status %s", resp.Status)
137+ }
138+
122 result, err := Parse(resp.Body, feedURL)139 result, err := Parse(resp.Body, feedURL)
123 if err != nil {140 if err != nil {
124- return nil, nil, fmt.Errorf("parsing feed: %w", err)141+ return nil, resp, fmt.Errorf("%w: %v", errUnparseable, err)
125 }142 }
126 143
127 return result, resp, nil144 return result, resp, nil
modified internal/feed/fetcher_test.go +93 -0
@@ -2,6 +2,9 @@ package feed
22
33 import (
44 "net/http"
5+ "net/http/httptest"
6+ "strings"
7+ "sync/atomic"
58 "testing"
69 "time"
710 )
@@ -49,3 +52,93 @@ func TestRetryBackoff_RetryAfterIgnoredForNon429(t *testing.T) {
4952 t.Fatalf("got %v, want 1s", got)
5053 }
5154 }
55+
56+// The tests above exercise retryBackoff in isolation, with a hand-built
57+// response. That is how the retry policy came to be wrong while its unit
58+// tests passed: nothing checked that retryBackoff was ever reached with a
59+// non-nil response, or that Fetch consulted the status at all.
60+//
61+// These drive Fetch against a real server instead, and assert request counts.
62+
63+func newTestFetcher() *Fetcher {
64+ return &Fetcher{httpClient: &http.Client{Timeout: 5 * time.Second}}
65+}
66+
67+const testFeed = `<?xml version="1.0"?><rss version="2.0"><channel>` +
68+ `<title>T</title><link>https://x.test</link>` +
69+ `<item><title>One</title><link>https://x.test/1</link></item>` +
70+ `</channel></rss>`
71+
72+func TestFetch_DoesNotRetryPermanentStatus(t *testing.T) {
73+ var calls int32
74+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
75+ atomic.AddInt32(&calls, 1)
76+ http.Error(w, "nope", http.StatusNotFound)
77+ }))
78+ defer srv.Close()
79+
80+ if _, err := newTestFetcher().Fetch(t.Context(), srv.URL); err == nil {
81+ t.Fatal("expected an error for a 404")
82+ }
83+ if got := atomic.LoadInt32(&calls); got != 1 {
84+ t.Fatalf("a 404 was requested %d times, want 1", got)
85+ }
86+}
87+
88+func TestFetch_RetriesTransientStatus(t *testing.T) {
89+ var calls int32
90+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
91+ if atomic.AddInt32(&calls, 1) < 3 {
92+ http.Error(w, "later", http.StatusServiceUnavailable)
93+ return
94+ }
95+ w.Header().Set("Content-Type", "application/xml")
96+ _, _ = w.Write([]byte(testFeed))
97+ }))
98+ defer srv.Close()
99+
100+ result, err := newTestFetcher().Fetch(t.Context(), srv.URL)
101+ if err != nil {
102+ t.Fatalf("expected success after retries: %v", err)
103+ }
104+ if len(result.Articles) != 1 {
105+ t.Fatalf("got %d articles, want 1", len(result.Articles))
106+ }
107+ if got := atomic.LoadInt32(&calls); got != 3 {
108+ t.Fatalf("made %d requests, want 3", got)
109+ }
110+}
111+
112+func TestFetch_DoesNotRetryUnparseableBody(t *testing.T) {
113+ var calls int32
114+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
115+ atomic.AddInt32(&calls, 1)
116+ _, _ = w.Write([]byte("<html><body>not a feed</body></html>"))
117+ }))
118+ defer srv.Close()
119+
120+ // The bytes will be identical on a second read, so one attempt is enough.
121+ if _, err := newTestFetcher().Fetch(t.Context(), srv.URL); err == nil {
122+ t.Fatal("expected an error for a non-feed body")
123+ }
124+ if got := atomic.LoadInt32(&calls); got != 1 {
125+ t.Fatalf("an unparseable body was requested %d times, want 1", got)
126+ }
127+}
128+
129+func TestFetch_ReportsStatusRatherThanParseFailure(t *testing.T) {
130+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
131+ http.Error(w, "gone", http.StatusGone)
132+ }))
133+ defer srv.Close()
134+
135+ _, err := newTestFetcher().Fetch(t.Context(), srv.URL)
136+ if err == nil {
137+ t.Fatal("expected an error")
138+ }
139+ // Before the fix an error page was handed to the parser, so a 410 was
140+ // reported as a parse failure and the real cause never reached the logs.
141+ if !strings.Contains(err.Error(), "410") {
142+ t.Fatalf("error should name the status, got: %v", err)
143+ }
144+}
@@ -2,6 +2,9 @@ package feed
2 2
3 import (3 import (
4 "net/http"4 "net/http"
5+ "net/http/httptest"
6+ "strings"
7+ "sync/atomic"
5 "testing"8 "testing"
6 "time"9 "time"
7 )10 )
@@ -49,3 +52,93 @@ func TestRetryBackoff_RetryAfterIgnoredForNon429(t *testing.T) {
49 t.Fatalf("got %v, want 1s", got)52 t.Fatalf("got %v, want 1s", got)
50 }53 }
51 }54 }
55+
56+// The tests above exercise retryBackoff in isolation, with a hand-built
57+// response. That is how the retry policy came to be wrong while its unit
58+// tests passed: nothing checked that retryBackoff was ever reached with a
59+// non-nil response, or that Fetch consulted the status at all.
60+//
61+// These drive Fetch against a real server instead, and assert request counts.
62+
63+func newTestFetcher() *Fetcher {
64+ return &Fetcher{httpClient: &http.Client{Timeout: 5 * time.Second}}
65+}
66+
67+const testFeed = `<?xml version="1.0"?><rss version="2.0"><channel>` +
68+ `<title>T</title><link>https://x.test</link>` +
69+ `<item><title>One</title><link>https://x.test/1</link></item>` +
70+ `</channel></rss>`
71+
72+func TestFetch_DoesNotRetryPermanentStatus(t *testing.T) {
73+ var calls int32
74+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
75+ atomic.AddInt32(&calls, 1)
76+ http.Error(w, "nope", http.StatusNotFound)
77+ }))
78+ defer srv.Close()
79+
80+ if _, err := newTestFetcher().Fetch(t.Context(), srv.URL); err == nil {
81+ t.Fatal("expected an error for a 404")
82+ }
83+ if got := atomic.LoadInt32(&calls); got != 1 {
84+ t.Fatalf("a 404 was requested %d times, want 1", got)
85+ }
86+}
87+
88+func TestFetch_RetriesTransientStatus(t *testing.T) {
89+ var calls int32
90+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
91+ if atomic.AddInt32(&calls, 1) < 3 {
92+ http.Error(w, "later", http.StatusServiceUnavailable)
93+ return
94+ }
95+ w.Header().Set("Content-Type", "application/xml")
96+ _, _ = w.Write([]byte(testFeed))
97+ }))
98+ defer srv.Close()
99+
100+ result, err := newTestFetcher().Fetch(t.Context(), srv.URL)
101+ if err != nil {
102+ t.Fatalf("expected success after retries: %v", err)
103+ }
104+ if len(result.Articles) != 1 {
105+ t.Fatalf("got %d articles, want 1", len(result.Articles))
106+ }
107+ if got := atomic.LoadInt32(&calls); got != 3 {
108+ t.Fatalf("made %d requests, want 3", got)
109+ }
110+}
111+
112+func TestFetch_DoesNotRetryUnparseableBody(t *testing.T) {
113+ var calls int32
114+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
115+ atomic.AddInt32(&calls, 1)
116+ _, _ = w.Write([]byte("<html><body>not a feed</body></html>"))
117+ }))
118+ defer srv.Close()
119+
120+ // The bytes will be identical on a second read, so one attempt is enough.
121+ if _, err := newTestFetcher().Fetch(t.Context(), srv.URL); err == nil {
122+ t.Fatal("expected an error for a non-feed body")
123+ }
124+ if got := atomic.LoadInt32(&calls); got != 1 {
125+ t.Fatalf("an unparseable body was requested %d times, want 1", got)
126+ }
127+}
128+
129+func TestFetch_ReportsStatusRatherThanParseFailure(t *testing.T) {
130+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
131+ http.Error(w, "gone", http.StatusGone)
132+ }))
133+ defer srv.Close()
134+
135+ _, err := newTestFetcher().Fetch(t.Context(), srv.URL)
136+ if err == nil {
137+ t.Fatal("expected an error")
138+ }
139+ // Before the fix an error page was handed to the parser, so a 410 was
140+ // reported as a parse failure and the real cause never reached the logs.
141+ if !strings.Contains(err.Error(), "410") {
142+ t.Fatalf("error should name the status, got: %v", err)
143+ }
144+}