From f0c71437ea2038c7d2200649ca1dde4eccbfe866 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Mon, 10 Aug 2026 11:17:24 -0700 Subject: [PATCH] fix(link-preview): stop a transient fetch failure from blanking a valid card A valid link (e.g. a GitHub PR page) could render no preview card for five minutes after a single unlucky fetch, then quietly work again. One slow handshake, a rate-limit blip, or a 5xx would poison the card. Two paths funneled a transient failure into the same sticky negative cache as a genuine "no metadata" miss: - The resolver's fetch loader caught any rejection (timeout, network error) as a plain null and cached it under NULL_METADATA_RETRY_MS (5 minutes). - The native fetcher returned Ok(None) for a retryable page status (429, 408, 425, 5xx), which the resolver also cached for the full miss TTL. Distinguish transient failures from hard misses: - The loader now records a rejected fetch as transient and caches it for the short transient TTL (30s) instead of the 5-minute miss TTL. A resolved null (200 + HTML + no metadata) still holds the full miss TTL. - The native fetcher surfaces a retryable page status as an error (via a shared is_transient_status helper, reused by the image path) so it takes the short transient TTL rather than the sticky negative cache. Also widen the per-request timeout from 4s to 6s (total 10s to 15s) so slow networks stop tripping the transient path in the first place. Tests: JS regression that a rejected fetch clears well before the 5-minute miss TTL while a genuine null miss stays cached; the rejected-retry test now asserts the 30s transient boundary; Rust unit test that retryable statuses are transient and permanent ones (404/403/401/410) are hard misses. Full desktop unit suite 4580 passed; link_preview Rust suite 22 passed. Co-authored-by: Taylor Ho Signed-off-by: Taylor Ho --- .../src-tauri/src/commands/link_preview.rs | 59 +++++++++++++++---- .../lib/useResolvedLinkPreviews.test.mjs | 54 ++++++++++++++++- .../src/shared/lib/useResolvedLinkPreviews.ts | 20 ++++++- 3 files changed, 117 insertions(+), 16 deletions(-) diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index 5ada1b3884..68f7f94501 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -21,8 +21,8 @@ const MAX_IMAGE_FETCH_BYTES: usize = 2 * 1024 * 1024; const MAX_IMAGE_DIMENSION: u32 = 4096; const MAX_IMAGE_PIXELS: u64 = 16_000_000; const MAX_SANITIZED_DIMENSION: u32 = 1200; -const PREVIEW_FETCH_TIMEOUT: Duration = Duration::from_secs(4); -const PREVIEW_TOTAL_TIMEOUT: Duration = Duration::from_secs(10); +const PREVIEW_FETCH_TIMEOUT: Duration = Duration::from_secs(6); +const PREVIEW_TOTAL_TIMEOUT: Duration = Duration::from_secs(15); const MAX_REDIRECTS: usize = 3; const MAX_METADATA_CHARS: usize = 180; const MAX_METADATA_DESCRIPTION_CHARS: usize = 280; @@ -87,7 +87,18 @@ async fn fetch_link_preview_metadata_inner( continue; } - if !response.status().is_success() || !is_html_response(&response) { + if !response.status().is_success() { + // A retryable status (rate limit, request timeout, too early, or a + // server error) is transient: surface it as an error so the caller + // 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 is_transient_status(status) { + return Err(format!("link preview request failed: HTTP {status}")); + } + return Ok(None); + } + if !is_html_response(&response) { return Ok(None); } let body = read_bytes_prefix(response, MAX_PREVIEW_FETCH_BYTES).await?; @@ -215,6 +226,16 @@ 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() @@ -350,11 +371,7 @@ async fn fetch_sanitized_image( } if !response.status().is_success() { let status = response.status(); - if status == reqwest::StatusCode::TOO_MANY_REQUESTS - || status == reqwest::StatusCode::REQUEST_TIMEOUT - || status == reqwest::StatusCode::TOO_EARLY - || status.is_server_error() - { + if is_transient_status(status) { let retry_after = retry_after_duration(&response); if let Some(retry_after) = retry_after { set_image_host_cooldown(&url, retry_after); @@ -651,9 +668,9 @@ mod tests { use super::rate_limit::MAX_IMAGE_RETRY_AFTER; use super::{ apply_image_result, declares_animation, extract_favicon_url, extract_image_url, - extract_link_preview_metadata, is_html_response, read_bytes_prefix, retry_after_duration, - sanitize_image, ImageFetchError, LinkPreviewImageFetchState, LinkPreviewMetadata, - MAX_METADATA_DESCRIPTION_CHARS, + extract_link_preview_metadata, is_html_response, is_transient_status, read_bytes_prefix, + retry_after_duration, sanitize_image, ImageFetchError, LinkPreviewImageFetchState, + LinkPreviewMetadata, MAX_METADATA_DESCRIPTION_CHARS, }; use axum::{body::Body, http::Response, routing::get, Router}; use base64::Engine as _; @@ -957,4 +974,24 @@ mod tests { assert_eq!(extract_link_preview_metadata(" "), None); assert_eq!(extract_link_preview_metadata(""), None); } + + #[test] + fn transient_statuses_are_distinguished_from_hard_misses() { + use reqwest::StatusCode; + // Retryable: surfaced as an error so the caller retries soon instead of + // caching an empty card for the full miss TTL. + assert!(is_transient_status(StatusCode::TOO_MANY_REQUESTS)); + assert!(is_transient_status(StatusCode::REQUEST_TIMEOUT)); + assert!(is_transient_status(StatusCode::TOO_EARLY)); + assert!(is_transient_status(StatusCode::INTERNAL_SERVER_ERROR)); + assert!(is_transient_status(StatusCode::BAD_GATEWAY)); + assert!(is_transient_status(StatusCode::SERVICE_UNAVAILABLE)); + assert!(is_transient_status(StatusCode::GATEWAY_TIMEOUT)); + // Genuine hard misses: cached for the full miss TTL, not retried early. + assert!(!is_transient_status(StatusCode::NOT_FOUND)); + assert!(!is_transient_status(StatusCode::FORBIDDEN)); + assert!(!is_transient_status(StatusCode::UNAUTHORIZED)); + assert!(!is_transient_status(StatusCode::GONE)); + assert!(!is_transient_status(StatusCode::OK)); + } } diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs index bf7e393612..998202c499 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs @@ -133,7 +133,7 @@ test("metadata loader retries transient images after the server cooldown", async assert.equal(calls, 2); }); -test("metadata loader retries rejected requests after the negative-cache TTL", async () => { +test("metadata loader retries a rejected request after the short transient TTL", async () => { let now = 1_000; let calls = 0; const loader = __linkPreviewMetadataTest.createMetadataLoader({ @@ -149,11 +149,61 @@ test("metadata loader retries rejected requests after the negative-cache TTL", a assert.equal((await loader.load(preview.href)).metadata, null); assert.equal(calls, 1); - now += 5 * 60_000; + now += 30_000; assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); assert.equal(calls, 2); }); +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. + let now = 1_000; + let calls = 0; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + fetcher: async () => { + calls += 1; + if (calls === 1) throw new Error("temporary failure"); + return metadata(); + }, + now: () => now, + }); + + 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; + assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.equal(calls, 2); +}); + +test("a genuine no-metadata miss stays cached for the full miss TTL", async () => { + let now = 1_000; + let calls = 0; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + fetcher: async () => { + calls += 1; + return null; + }, + now: () => now, + }); + + assert.equal((await loader.load(preview.href)).metadata, null); + assert.equal(calls, 1); + + // A resolved null (200 + HTML + no metadata) is a hard miss: it must not be + // refetched inside the transient window. + now += 60_000; + assert.equal((await loader.load(preview.href)).metadata, null); + assert.equal(calls, 1); + + now += 5 * 60_000; + assert.equal((await loader.load(preview.href)).metadata, null); + assert.equal(calls, 2); +}); + test("metadata loader coalesces fragment variants and bounds concurrency", async () => { let active = 0; let maxActive = 0; diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.ts b/desktop/src/shared/lib/useResolvedLinkPreviews.ts index 136f8f4b1c..f955ac2cf8 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.ts +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.ts @@ -33,6 +33,9 @@ export type LinkPreviewMetadata = { type MetadataCacheEntry = { expiresAt: number | null; metadata: LinkPreviewMetadata | null; + /** A transient fetch failure (timeout/429/5xx/network) rather than a genuine + * "no metadata" result; retried soon instead of cached for the full miss TTL. */ + transient?: boolean; }; type MetadataLoadResult = MetadataCacheEntry & { @@ -80,7 +83,12 @@ function metadataCacheKey(href: string): string { function metadataExpiry( metadata: LinkPreviewMetadata | null, now: number, + transient = false, ): 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; if (metadata === null) return now + NULL_METADATA_RETRY_MS; if (metadata.imageFetchState !== "transient_failure") return null; const retryAfterMs = @@ -158,11 +166,17 @@ function createMetadataLoader({ const requestGeneration = generation; const promise = schedule(() => fetcher(href)) - .catch(() => null) - .then((metadata) => { + .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 }) => { const entry = { - expiresAt: metadataExpiry(metadata, now()), + expiresAt: metadataExpiry(metadata, now(), transient), metadata, + transient, }; if (requestGeneration === generation) { cache.set(key, entry);