diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index 68f7f9450..c8919bf79 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -14,7 +14,10 @@ use url::Url; #[path = "link_preview_rate_limit.rs"] mod rate_limit; -use rate_limit::{image_host_cooldown_remaining, retry_after_duration, set_image_host_cooldown}; +use rate_limit::{ + image_host_cooldown_remaining, is_transient_status, rate_limited_response_error, + retry_after_duration, set_image_host_cooldown, +}; const MAX_PREVIEW_FETCH_BYTES: usize = 256 * 1024; const MAX_IMAGE_FETCH_BYTES: usize = 2 * 1024 * 1024; @@ -93,6 +96,9 @@ async fn fetch_link_preview_metadata_inner( // retries soon instead of caching an empty card for the full miss // TTL. Any other non-success (e.g. 404) is a genuine hard miss. let status = response.status(); + if let Some(error) = rate_limited_response_error(&response) { + return Err(error); + } if is_transient_status(status) { return Err(format!("link preview request failed: HTTP {status}")); } @@ -226,16 +232,6 @@ async fn send_pinned_request(url: &Url, accept: &str) -> Result bool { - status == reqwest::StatusCode::TOO_MANY_REQUESTS - || status == reqwest::StatusCode::REQUEST_TIMEOUT - || status == reqwest::StatusCode::TOO_EARLY - || status.is_server_error() -} - fn is_html_response(response: &reqwest::Response) -> bool { response .headers() diff --git a/desktop/src-tauri/src/commands/link_preview_rate_limit.rs b/desktop/src-tauri/src/commands/link_preview_rate_limit.rs index c3f7ed718..eff4c87c4 100644 --- a/desktop/src-tauri/src/commands/link_preview_rate_limit.rs +++ b/desktop/src-tauri/src/commands/link_preview_rate_limit.rs @@ -22,6 +22,43 @@ pub(super) fn retry_after_duration(response: &reqwest::Response) -> Option) -> String { + match retry_after { + Some(retry_after) => { + format!( + "{RATE_LIMITED_ERROR_PREFIX}: retry-after {}", + retry_after.as_secs() + ) + } + None => RATE_LIMITED_ERROR_PREFIX.to_string(), + } +} + +/// A 429 is a genuine rate limit: an immediate retry would just be throttled +/// again, so surface it distinctly (carrying any Retry-After) rather than as an +/// ordinary transient blip. Returns `None` for every other status so the caller +/// keeps its usual transient/hard-miss handling. +pub(super) fn rate_limited_response_error(response: &reqwest::Response) -> Option { + (response.status() == reqwest::StatusCode::TOO_MANY_REQUESTS) + .then(|| rate_limited_error(retry_after_duration(response))) +} + +/// A retryable HTTP status: rate limit, request timeout, too early, or any +/// server error. These are transient and should be retried soon rather than +/// cached as a genuine "no metadata" miss. +pub(super) fn is_transient_status(status: reqwest::StatusCode) -> bool { + status == reqwest::StatusCode::TOO_MANY_REQUESTS + || status == reqwest::StatusCode::REQUEST_TIMEOUT + || status == reqwest::StatusCode::TOO_EARLY + || status.is_server_error() +} + pub(super) fn image_host_cooldown_remaining(url: &Url) -> Option { let host = url.host_str()?; let cooldowns = IMAGE_HOST_COOLDOWNS.get_or_init(|| Mutex::new(HashMap::new())); @@ -60,3 +97,30 @@ pub(super) fn set_image_host_cooldown(url: &Url, retry_after: Duration) { cooldowns.insert(host.to_string(), expires_at); } } + +#[cfg(test)] +mod tests { + use super::{rate_limited_error, RATE_LIMITED_ERROR_PREFIX}; + use std::time::Duration; + + #[test] + fn rate_limited_error_carries_retry_after_seconds() { + // A rate limit surfaces a distinct marker so the caller skips the fast + // inline retry; when the server gave a Retry-After we append its whole + // seconds so the caller can wait exactly that long. + assert_eq!( + rate_limited_error(Some(Duration::from_secs(45))), + format!("{RATE_LIMITED_ERROR_PREFIX}: retry-after 45"), + ); + // Sub-second remainders truncate to whole seconds. + assert_eq!( + rate_limited_error(Some(Duration::from_millis(1_500))), + format!("{RATE_LIMITED_ERROR_PREFIX}: retry-after 1"), + ); + // No Retry-After: bare marker, caller falls back to its default window. + assert_eq!( + rate_limited_error(None), + RATE_LIMITED_ERROR_PREFIX.to_string(), + ); + } +} diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs index 998202c49..21afd9f33 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs @@ -133,10 +133,14 @@ test("metadata loader retries transient images after the server cooldown", async assert.equal(calls, 2); }); -test("metadata loader retries a rejected request after the short transient TTL", async () => { - let now = 1_000; +test("a transient blip earns one immediate inline retry before caching", async () => { + // A single unlucky fetch (timeout/5xx/network) should not blank the card: the + // loader retries once inline after a short backoff. If that second attempt + // succeeds, the card resolves right away with no wait at all. + const now = 1_000; let calls = 0; const loader = __linkPreviewMetadataTest.createMetadataLoader({ + delay: () => Promise.resolve(), fetcher: async () => { calls += 1; if (calls === 1) throw new Error("temporary failure"); @@ -145,25 +149,104 @@ test("metadata loader retries a rejected request after the short transient TTL", now: () => now, }); + assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.equal(calls, 2); +}); + +test("a blip that also fails the inline retry caches transient, recovering after the short TTL", async () => { + // When both the initial fetch and its inline retry fail, the loader gives up + // and caches a transient entry (short 30s TTL), not a full 5-min hard miss. + let now = 1_000; + let calls = 0; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + delay: () => Promise.resolve(), + fetcher: async () => { + calls += 1; + // Both the initial attempt and its inline retry fail; success afterward. + if (calls <= 2) throw new Error("temporary failure"); + return metadata(); + }, + now: () => now, + }); + assert.equal((await loader.load(preview.href)).metadata, null); assert.equal((await loader.load(preview.href)).metadata, null); - assert.equal(calls, 1); + assert.equal(calls, 2); now += 30_000; assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); - assert.equal(calls, 2); + assert.equal(calls, 3); }); test("a rejected fetch does not poison the cache for the full miss TTL", async () => { // Regression: a single transient failure (timeout/429/5xx/network) used to be // cached like a genuine "no metadata" miss, blanking a perfectly valid link - // for 5 minutes. It must clear well before the miss TTL so the card recovers. + // for 5 minutes. Even when the inline retry also fails, the transient entry + // must clear well before the miss TTL so the card recovers. let now = 1_000; let calls = 0; const loader = __linkPreviewMetadataTest.createMetadataLoader({ + delay: () => Promise.resolve(), fetcher: async () => { calls += 1; - if (calls === 1) throw new Error("temporary failure"); + if (calls <= 2) throw new Error("temporary failure"); + return metadata(); + }, + now: () => now, + }); + + assert.equal((await loader.load(preview.href)).metadata, null); + assert.equal(calls, 2); + + // Well before NULL_METADATA_RETRY_MS (5 min) the transient entry has expired + // and the retry succeeds — a hard miss would still be cached here. + now += 60_000; + assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.equal(calls, 3); +}); + +test("a rate limit skips the inline retry and waits out its Retry-After", async () => { + // A 429 must NOT get the fast inline retry (an immediate retry would just be + // throttled again). It caches transient and honors the server's Retry-After + // as the wait, then refetches once that window elapses. + let now = 1_000; + let calls = 0; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + delay: () => Promise.resolve(), + fetcher: async () => { + calls += 1; + if (calls === 1) { + throw new Error("link preview rate limited: retry-after 45"); + } + return metadata(); + }, + now: () => now, + }); + + assert.equal((await loader.load(preview.href)).metadata, null); + // No inline retry on a rate limit: exactly one fetch so far. + assert.equal(calls, 1); + + // The default transient window has passed, but the server asked for 45s — the + // entry is still cached, no refetch yet. + now += 30_000; + assert.equal((await loader.load(preview.href)).metadata, null); + assert.equal(calls, 1); + + // Past the honored Retry-After the entry expires and the refetch succeeds. + now += 15_000; + assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.equal(calls, 2); +}); + +test("a rate limit without Retry-After falls back to the default transient window", async () => { + let now = 1_000; + let calls = 0; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + delay: () => Promise.resolve(), + fetcher: async () => { + calls += 1; + if (calls === 1) throw new Error("link preview rate limited"); return metadata(); }, now: () => now, @@ -172,9 +255,7 @@ test("a rejected fetch does not poison the cache for the full miss TTL", async ( assert.equal((await loader.load(preview.href)).metadata, null); assert.equal(calls, 1); - // Well before NULL_METADATA_RETRY_MS (5 min) the transient entry has expired - // and the retry succeeds — a hard miss would still be cached here. - now += 60_000; + now += 30_000; assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); assert.equal(calls, 2); }); diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.ts b/desktop/src/shared/lib/useResolvedLinkPreviews.ts index f955ac2cf..f322e6e86 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.ts +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.ts @@ -45,6 +45,38 @@ type MetadataLoadResult = MetadataCacheEntry & { const DEFAULT_TRANSIENT_RETRY_MS = 30_000; const NULL_METADATA_RETRY_MS = 5 * 60_000; const MAX_CONCURRENT_METADATA_FETCHES = 2; +/** Backoff before the single inline retry that a transient blip earns before + * it falls into the longer {@link DEFAULT_TRANSIENT_RETRY_MS} self-heal wait. */ +const INLINE_RETRY_BACKOFF_MS = 750; +/** Prefix the native fetcher uses to mark a 429. A rate limit must NOT get the + * fast inline retry (we would just be throttled again); it waits instead. Kept + * in sync with RATE_LIMITED_ERROR_PREFIX in link_preview.rs. */ +const RATE_LIMITED_ERROR_PREFIX = "link preview rate limited"; + +type TransientFailure = { + /** A 429 rate limit: skip the inline retry and wait it out. */ + rateLimited: boolean; + /** Server-supplied Retry-After, in ms, when present on a rate limit. */ + retryAfterMs?: number; +}; + +/** Classify a rejected native fetch. Any rejection is transient (a genuine + * "no metadata" result resolves, it does not reject); we only distinguish a + * rate limit — which must not be retried immediately — from an ordinary blip. */ +function classifyTransientFailure(reason: unknown): TransientFailure { + const message = reason instanceof Error ? reason.message : String(reason); + if (!message.startsWith(RATE_LIMITED_ERROR_PREFIX)) { + return { rateLimited: false }; + } + const match = /retry-after (\d+)/.exec(message); + const retryAfterSeconds = match ? Number.parseInt(match[1], 10) : NaN; + return { + rateLimited: true, + retryAfterMs: Number.isFinite(retryAfterSeconds) + ? retryAfterSeconds * 1_000 + : undefined, + }; +} /** * React may flush an interaction-triggered effect before the browser paints. @@ -84,11 +116,13 @@ function metadataExpiry( metadata: LinkPreviewMetadata | null, now: number, transient = false, + transientRetryMs = DEFAULT_TRANSIENT_RETRY_MS, ): number | null { // A transient failure (timeout/429/5xx/network) must not stick for the full // miss TTL: one unlucky fetch would otherwise blank the card for 5 minutes - // even though the link is perfectly valid. Retry it soon instead. - if (transient) return now + DEFAULT_TRANSIENT_RETRY_MS; + // even though the link is perfectly valid. Retry it soon instead — after the + // server's Retry-After when it gave us one, otherwise the default window. + if (transient) return now + transientRetryMs; if (metadata === null) return now + NULL_METADATA_RETRY_MS; if (metadata.imageFetchState !== "transient_failure") return null; const retryAfterMs = @@ -128,10 +162,13 @@ function createTaskScheduler(concurrency: number) { function createMetadataLoader({ concurrency = MAX_CONCURRENT_METADATA_FETCHES, + delay = (ms: number) => + new Promise((resolve) => setTimeout(resolve, ms)), fetcher, now = Date.now, }: { concurrency?: number; + delay?: (ms: number) => Promise; fetcher: (href: string) => Promise; now?: () => number; }) { @@ -165,16 +202,57 @@ function createMetadataLoader({ } const requestGeneration = generation; - const promise = schedule(() => fetcher(href)) - .then( - (metadata) => ({ metadata, transient: false }), - // A rejected fetch is a transient failure (timeout, network error, or a - // retryable status surfaced as an error), not a genuine "no metadata". - () => ({ metadata: null, transient: true }), - ) - .then(({ metadata, transient }) => { + // One coalesced attempt sequence. A transient blip earns a single inline + // retry after a short backoff before we give up and fall into the longer + // self-heal wait; a rate limit (429) skips that retry — an immediate retry + // would just be throttled again — and waits out its Retry-After instead. + const attempt = (): Promise<{ + metadata: LinkPreviewMetadata | null; + transient: boolean; + transientRetryMs: number; + }> => + fetcher(href).then( + (metadata) => ({ + metadata, + transient: false, + transientRetryMs: DEFAULT_TRANSIENT_RETRY_MS, + }), + (reason) => { + const failure = classifyTransientFailure(reason); + if (failure.rateLimited) { + return { + metadata: null, + transient: true, + transientRetryMs: + failure.retryAfterMs ?? DEFAULT_TRANSIENT_RETRY_MS, + }; + } + return delay(INLINE_RETRY_BACKOFF_MS) + .then(() => fetcher(href)) + .then( + (metadata) => ({ + metadata, + transient: false, + transientRetryMs: DEFAULT_TRANSIENT_RETRY_MS, + }), + () => ({ + metadata: null, + transient: true, + transientRetryMs: DEFAULT_TRANSIENT_RETRY_MS, + }), + ); + }, + ); + + const promise = schedule(attempt).then( + ({ metadata, transient, transientRetryMs }) => { const entry = { - expiresAt: metadataExpiry(metadata, now(), transient), + expiresAt: metadataExpiry( + metadata, + now(), + transient, + transientRetryMs, + ), metadata, transient, }; @@ -182,7 +260,8 @@ function createMetadataLoader({ cache.set(key, entry); } return { key, ...entry }; - }); + }, + ); cache.set(key, promise); return promise; };