From 6384f17d431956ff898e1a8821de5463816cfc18 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Sun, 4 Oct 2026 19:13:01 +0200 Subject: [PATCH] engine: honour Retry-After when retrying a rate-limited module fetch A module answered 429 was retried after a fixed 150ms and 300ms, inside the window the origin asked us to wait, and could be dropped, breaking the page. Reuse the image path's Retry-After parsing so the wait matches the origin's request. Stash-verified: the old code drops the module at 0.46s; the fix fetches it after the 1s the origin asked for. Co-Authored-By: Claude Sonnet 5 --- FIDELITY.md | 9 ++++++++ modules.go | 26 ++++++++++++++-------- modules_retryafter_test.go | 45 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 9 deletions(-) create mode 100644 modules_retryafter_test.go diff --git a/FIDELITY.md b/FIDELITY.md index 07d220d..c8ccd2a 100644 --- a/FIDELITY.md +++ b/FIDELITY.md @@ -38,6 +38,15 @@ What limits the network is `maxPerHostConcurrency` (`transport.go`): every Tailw - **Correction (round 153)**: the 429 comparison above used two runs at cap 2 (0 429s) and two at cap 4 (3 each). Eight runs at the baseline cap of 2 produced 429s in six of them (3–5 each), so Wikimedia's 429s are not a function of this cap, and the cap-4 "regression" was a sampling artifact. The cap is not the lever for Wikimedia's limit. Its current baseline rate needs a separate investigation (retry/backoff behaviour, Retry-After handling, and whether the retried images still render). - **Adaptive limiter tried in round 153, not shipped**: start at 2, grow to 4 after a run of clean responses, drop back on a 429. Growing after 10 clean responses still produced 429s (5 in one of three runs); growing after 40 never grows on a single Wikipedia page, so it cannot help there, and its Tailwind gain was within noise. Parked, not merged. +## 2026-10-04 (round 160) — module fetches ignored a rate-limited origin's Retry-After and retried inside its window, which could drop a module and break the page (engine#260, patch) + +A survey of how other renderers accelerate loading recommended honouring Retry-After in module fetches. Checked against the code before acting, because the survey also claimed that no Retry-After parsing existed and that image fetches had no 429 retry. Both claims were wrong: `doWithRateLimitRetry` and `retryAfterDelay` in `images.go` already honour Retry-After for images and fonts (three retries). The real gap was narrower: `fetchModuleSource` in `modules.go` retried after a fixed 150ms then 300ms, ignoring the header. + +- **Why it matters beyond latency**: a module refused with `Retry-After: 1` was retried 150ms later, drew another 429, and was dropped after the attempts ran out. A dropped module is a broken page, not a slow one. +- **Fixed**: `fetchModuleOnce` returns the Retry-After delay for a 429, and `fetchModuleSource` waits that long when present, keeping the fixed backoff otherwise. The same helper as the image path is reused. +- **Test**: `TestFetchModuleSourceHonoursRetryAfter` serves 429 with `Retry-After: 1` for the first second. With the fix it succeeds after about 1.0s. Stash-verified: the old code drops the module at 0.46s. +- **Corpus effect**: none measured. The saved Tailwind, Wikipedia and Hacker News renders are pixel-identical before and after, because no module on those pages was rate-limited. The change corrects a case the corpus does not exercise. + ## 2026-10-04 (round 158) — the settle loop is a small lever: about 0.26s of CPU on tailwindcss.com, all of it full re-cascades, bounded by the network wait (docs only, no behaviour change) Third candidate after the cascade and the image path: the settle-then-render loop. Profiling one Tailwind render shows `settle` at about 12% of CPU samples, and of that the cascade (`CascadeMediaContainers`) is almost all of it, about 0.26s. JavaScript execution is about 0.07s. A render performs five full cascades. Each is a whole-document pass. diff --git a/modules.go b/modules.go index 8110ec7..7943716 100644 --- a/modules.go +++ b/modules.go @@ -304,48 +304,56 @@ func (e *Engine) fetchModuleSource(ctx context.Context, abs string) (string, boo return "", false } for attempt := 0; ; attempt++ { - src, retry, ok := e.fetchModuleOnce(ctx, abs) + src, retry, ok, after := e.fetchModuleOnce(ctx, abs) if ok { return src, true } if !retry || attempt >= moduleFetchRetries || ctx.Err() != nil { return "", false } - // Small backoff to let a rate-limited origin recover, bounded by ctx. + // A rate-limited origin says how long to wait (Retry-After); retrying + // sooner just draws another 429. Otherwise a small backoff bounded by ctx. + wait := after + if wait == 0 { + wait = time.Duration(attempt+1) * 150 * time.Millisecond + } select { case <-ctx.Done(): return "", false - case <-time.After(time.Duration(attempt+1) * 150 * time.Millisecond): + case <-time.After(wait): } } } // fetchModuleOnce does a single fetch attempt. retry reports whether the failure // looks transient (worth another attempt): a network error or a 429/5xx. -func (e *Engine) fetchModuleOnce(ctx context.Context, abs string) (body string, retry, ok bool) { +func (e *Engine) fetchModuleOnce(ctx context.Context, abs string) (body string, retry, ok bool, after time.Duration) { rctx, cancel := context.WithTimeout(ctx, moduleFetchTimeout) defer cancel() req, err := http.NewRequestWithContext(rctx, http.MethodGet, abs, nil) if err != nil { - return "", false, false + return "", false, false, 0 } if e.UserAgent != "" { req.Header.Set("User-Agent", e.UserAgent) } resp, err := e.Client.Do(req) if err != nil { - return "", true, false // network/timeout: transient + return "", true, false, 0 // network/timeout: transient } defer resp.Body.Close() if resp.StatusCode < 200 || resp.StatusCode >= 300 { transient := resp.StatusCode == http.StatusTooManyRequests || resp.StatusCode >= 500 - return "", transient, false + if resp.StatusCode == http.StatusTooManyRequests && resp.Header.Get("Retry-After") != "" { + after = retryAfterDelay(resp.Header.Get("Retry-After")) + } + return "", transient, false, after } raw, err := io.ReadAll(io.LimitReader(resp.Body, maxModuleSourceBytes)) if err != nil { - return "", true, false + return "", true, false, 0 } - return string(raw), false, true + return string(raw), false, true, 0 } // awaitBounded runs fn in its own goroutine and returns its result with ok diff --git a/modules_retryafter_test.go b/modules_retryafter_test.go new file mode 100644 index 0000000..2675a5b --- /dev/null +++ b/modules_retryafter_test.go @@ -0,0 +1,45 @@ +// Copyright (c) the go-webengine/engine authors. +// SPDX-License-Identifier: BSD-3-Clause + +package engine + +import ( + "context" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" +) + +// A module answered 429 with Retry-After: 1 must be fetched again only after +// that second has passed. The fixed 150ms/300ms backoff retried inside the +// window, drew another 429, ran out of attempts and dropped the module. +func TestFetchModuleSourceHonoursRetryAfter(t *testing.T) { + var mu sync.Mutex + var first time.Time + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + defer mu.Unlock() + if first.IsZero() { + first = time.Now() + } + if time.Since(first) < time.Second { + w.Header().Set("Retry-After", "1") + w.WriteHeader(http.StatusTooManyRequests) + return + } + _, _ = w.Write([]byte("export const x = 1;")) + })) + defer srv.Close() + e := New() + start := time.Now() + src, ok := e.fetchModuleSource(context.Background(), srv.URL+"/chunk.js") + elapsed := time.Since(start) + if !ok || src != "export const x = 1;" { + t.Fatalf("module not fetched: ok=%v src=%q", ok, src) + } + if elapsed < 900*time.Millisecond { + t.Errorf("retried after %v, want to wait for Retry-After (about 1s)", elapsed) + } +}