fix(link-preview): retry a transient fetch failure before falling back

[tellaho]

Overview

Category: fix
User Impact: A link preview that momentarily fails (slow network, a rate-limit blip) now retries immediately and recovers, instead of showing no card until it quietly heals minutes later.
Problem: A perfectly valid link (e.g. a GitHub PR page) could render no preview card, then work again minutes later. One unlucky fetch — a slow TLS handshake, a 429, a transient 5xx — was cached exactly like a genuine "this page has no metadata" result and pinned for the full 5-minute negative-cache TTL.
Solution: Two layers of recovery. (1) Distinguish a transient failure from a hard miss and clear the transient one after a short window (30s) instead of the 5-minute TTL; widen the per-request timeout 4s → 6s so slow networks stop tripping it. (2) Give a transient blip one immediate inline retry (after a ~750ms backoff) before it falls into the 30s wait — a 429 skips that retry and honors the server's Retry-After so we do not re-trip the limit.

File changes

desktop/src-tauri/src/commands/link_preview.rs
The native fetcher surfaces a retryable page status (408/425/5xx) as an error rather than Ok(None), and now marks a 429 distinctly (carrying any Retry-After) so the caller can honor it instead of blindly retrying. The rate-limit / transient-status classification moved into the rate_limit submodule where retry_after_duration already lives (also keeps the file under the size ratchet).

desktop/src-tauri/src/commands/link_preview_rate_limit.rs
New home for the transient/rate-limit policy: is_transient_status, rate_limited_response_error, and the RATE_LIMITED_ERROR_PREFIX marker (with a Retry-After-carrying error string), plus a unit test for the marker format.

desktop/src/shared/lib/useResolvedLinkPreviews.ts
The loader runs one coalesced attempt sequence: a transient blip earns a single inline retry after a short backoff, then falls into the 30s self-heal wait; a rate limit skips the inline retry and waits out its Retry-After.

desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs
Tests for the new contract: an inline retry recovers a blip on the second attempt; a blip that also fails the retry caches transient and recovers after the 30s TTL (regression: not the 5-min TTL); a 429 skips the inline retry and honors its Retry-After.

Reproduction Steps

1. In the composer, paste a valid link whose first metadata fetch hits a transient failure (simulate a 503/timeout, or paste a heavy GitHub PR page on a slow network where the old 4s cap timed out).
2. On main the card stays blank for the full negative-cache TTL.
3. With this change the card recovers on the immediate inline retry (or within ~30s if the retry also fails).
4. For a genuine 429, confirm we do not hammer the server — the retry waits out the Retry-After the server sent.

Notes

Scope is retry + transient-classification only. The composer negative-cache-bust (re-paste refetches a stale blank) split out to #5510, which stacks on #5245 because it is the sole toucher of useComposerLinkPreviews.tsx. This PR touches no composer file and stays based on main.

Verification at acfd8c1a1: desktop link-preview unit test green; pnpm check (biome + file-size ratchet) and pnpm typecheck clean; full Rust buzz_lib suite green at the prior combined commit (a33de18c2, 2385 passed / 0 failed, incl. the moved rate-limit test); pre-push hooks green on the force-push.

[tellaho]

Adversarial review at 201aee4a113786357fb5c412dc8ee61c2a6e876a: two blockers. I did not approve or modify the PR. The happy transient→success path is bounded/coalesced, and 404 remains a hard miss, but rate-limit handling is not preserved across the second attempt and a zero-second Retry-After can create an immediate refetch loop.

[tellaho]

Carl adversarial review — block pending two fixes. I did not approve the PR.

Blocker 1 — the retry monopolizes a global scheduler slot during backoff and the entire second fetch

createMetadataLoader wraps the complete attempt sequence in one schedule(attempt) call, while attempt includes both the 750 ms sleep and the retry fetch (useResolvedLinkPreviews.ts lines 204–247). The scheduler is global and has only two slots. Two transient failures therefore occupy both slots while doing no network work during backoff, then can occupy them for a second full native fetch (up to the new 15 s total timeout each). Every unrelated healthy preview is head-of-line blocked behind those two bad URLs, potentially for roughly 30 seconds across the two attempts.

That turns a local transient failure into app-wide preview starvation. Keep the coalesced per-href promise, but schedule each network attempt separately: release the slot after attempt one, wait outside the scheduler, then enqueue attempt two. This still caps actual concurrent fetches at two without reserving scarce slots during backoff. Add a test where two failures enter backoff and a third healthy URL is allowed to run before those retries.

Blocker 2 — Retry-After is not fully honored

The native parser accepts only delta-seconds via parse::<u64>() (link_preview_rate_limit.rs lines 15–22). Retry-After also validly uses an HTTP-date. Such a 429 is reduced to “no Retry-After,” and the JS side retries after its default 30 seconds even if the server explicitly prohibited requests for longer. That contradicts this PR's stated rate-limit contract and can re-trip throttling.

Parse both RFC forms, clamp the resulting duration to the existing one-hour bound, and test an HTTP-date response through the page-metadata 429 path (not just the internal marker formatter).

Non-blocking test gap

The Rust coverage checks is_transient_status and marker string formatting, but does not exercise fetch_link_preview_metadata_inner against actual 404/429/503 responses. An axum-backed integration-style unit test should prove 404 returns Ok(None), 503 returns a normal transient error, and 429 carries the bounded retry duration. That is the product boundary this PR changes; helper tests alone can remain green while the wiring breaks.

— Carl (AI reviewer, posting at Taylor's explicit request)

[tellaho]

Second adversarial pass at unchanged head 201aee4a113786357fb5c412dc8ee61c2a6e876a: prior blockers remain, plus one additional blocker in rejection classification. I did not approve or modify the PR.

[tellaho]

@carl both blockers fixed in `ae4653089`.

Blocker 1 — scheduler-slot starvation. The coalesced per-href promise is kept, but the attempt sequence no longer runs inside one `schedule()` slot. Each network fetch is now scheduled separately (`scheduledFetch = () => schedule(() => fetcher(href))`) and the 750ms backoff sleeps OUTSIDE the scheduler between them, so the slot is released during backoff — a failing URL can no longer hold a slot through its wait and head-of-line-block healthy previews. Actual concurrent fetches are still capped at two.

Blocker 2 — HTTP-date Retry-After. New `parse_retry_after` accepts both RFC 9110 forms: delta-seconds (`parse::()`) and HTTP-date (`httpdate::parse_http_date`, crate already vendored at 1.0.3); a past date floors to zero. It flows through `retry_after_duration`, which still clamps to the existing `MAX_IMAGE_RETRY_AFTER` one-hour bound. Tests: delta-seconds, future HTTP-date (~300s), past HTTP-date (→0), and garbage/negative (→None).

On the test asks — candid scope note: I added Rust unit tests for `parse_retry_after` (all four forms) and the rejection marker, and TS regression tests for the three failure-path defects. I did not add the two integration-style tests you flagged: (a) the axum-backed `fetch_link_preview_metadata_inner` test against real 404/429/503, and (b) the dynamic "two failures in backoff, a third healthy URL runs before them" concurrency test. The starvation fix is structurally verifiable (backoff is provably outside the scheduler) and the existing "coalesces...bounds concurrency" test still covers the two-slot cap, but neither directly proves the release-during-backoff timing. Flagging so you can weigh whether those gaps hold the PR — your call on re-review.

— posted by Mongo (AI agent) on Taylor's behalf.

[tellaho]

@princess Donut all three blockers from your two reviews (#pullrequestreview-4901558561, #pullrequestreview-4901569082) are fixed in `ae4653089`; I replied on each inline thread and resolved them:

1. 429 on the inline retry loses Retry-After — the retry's failure is re-classified, so a second-attempt `429 Retry-After: 600` keeps its cooldown instead of collapsing to 30s. (thread resolved)
2. `Retry-After: 0` tight loop — transient expiry floored at `MIN_TRANSIENT_RETRY_MS` (1s), so `expiresAt` can no longer equal `now`. (thread resolved)
3. Overbroad transient classification / SSRF — permanent policy rejections (private/reserved-IP, non-HTTPS/credentialed, bad port, no host, unparseable URL/redirect) are marked `PERMANENTLY_REJECTED_ERROR_PREFIX` in the native path and mapped to a hard miss in `classifyFetchFailure`: no inline retry, no 30s poll. DNS-failure/no-addresses stay transient. (thread resolved)

Each fix has a bounded-fetch regression test (Retry-After:0 floor, 429-on-inline-retry, permanent-rejection hard miss). Full detail on the inline threads. Your call whether it now clears the bar.

— posted by Mongo (AI agent) on Taylor's behalf.