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

desktop/src-tauri/src/commands/link_preview.rs

@@ -14,15 +14,18 @@ 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;
 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 +90,21 @@ 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 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}"));
+            }
+            return Ok(None);
+        }
+        if !is_html_response(&response) {
             return Ok(None);
         }
         let body = read_bytes_prefix(response, MAX_PREVIEW_FETCH_BYTES).await?;
@@ -350,11 +367,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 +664,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 +970,24 @@ mod tests {
         assert_eq!(extract_link_preview_metadata("<title>   </title>"), None);
         assert_eq!(extract_link_preview_metadata("<html></html>"), 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));
+    }
 }

desktop/src-tauri/src/commands/link_preview_rate_limit.rs

@@ -22,6 +22,43 @@ pub(super) fn retry_after_duration(response: &reqwest::Response) -> Option<Durat
         .map(|duration| duration.min(MAX_IMAGE_RETRY_AFTER))
 }
 
+/// Marker prefix the caller matches to distinguish a rate limit (429) from an
+/// ordinary transient blip. When the server supplied a Retry-After, its whole
+/// seconds are appended so the caller can wait exactly that long instead of
+/// blindly retrying or falling back to the default transient window.
+pub(super) const RATE_LIMITED_ERROR_PREFIX: &str = "link preview rate limited";
+
+pub(super) fn rate_limited_error(retry_after: Option<Duration>) -> 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<String> {
+    (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<Duration> {
     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(),
+        );
+    }
+}

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 rejected requests after the negative-cache 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,15 +149,142 @@ test("metadata loader retries rejected requests after the negative-cache TTL", a
     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, 2);
+
+  now += 30_000;
+  assert.deepEqual((await loader.load(preview.href)).metadata, metadata());
+  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. 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 <= 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);
 
-  now += 5 * 60_000;
+  // 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,
+  });
+
+  assert.equal((await loader.load(preview.href)).metadata, null);
+  assert.equal(calls, 1);
+
+  now += 30_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;