From 24f63174d71bd751c271487deaa746994f1a5ec5 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Tue, 1 Sep 2026 10:33:25 -0700 Subject: [PATCH 01/11] fix(link-preview): keep composer metadata fetches user-paced Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../src-tauri/src/commands/link_preview.rs | 72 ++++++++++--------- .../src/commands/link_preview_youtube.rs | 16 +---- 2 files changed, 43 insertions(+), 45 deletions(-) diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index b781f8d9e68..a272cdf7c99 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -1,4 +1,4 @@ -use std::{io::Cursor, net::IpAddr, time::Duration}; +use std::{future::Future, io::Cursor, net::IpAddr, time::Duration}; use base64::{engine::general_purpose::STANDARD as BASE64_STANDARD, Engine as _}; use futures_util::StreamExt; @@ -24,8 +24,6 @@ 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 MAX_REDIRECTS: usize = 3; const MAX_METADATA_CHARS: usize = 180; const MAX_METADATA_DESCRIPTION_CHARS: usize = 280; @@ -56,15 +54,23 @@ pub struct LinkPreviewMetadata { pub async fn fetch_link_preview_metadata( href: String, ) -> Result, String> { - tokio::time::timeout( - PREVIEW_TOTAL_TIMEOUT, - fetch_link_preview_metadata_inner(href), - ) - .await - .map_err(|_| "link preview request timed out".to_string())? + fetch_link_preview_metadata_with(href, fetch_link_preview_metadata_for_url).await +} + +/// Run metadata fetching without a native deadline. The renderer owns the +/// finite budget after a composer generation is promoted into a send. +async fn fetch_link_preview_metadata_with( + href: String, + fetch: F, +) -> Result, String> +where + F: FnOnce(String) -> Fut, + Fut: Future, String>>, +{ + fetch(href).await } -async fn fetch_link_preview_metadata_inner( +async fn fetch_link_preview_metadata_for_url( href: String, ) -> Result, String> { let mut url = Url::parse(href.trim()).map_err(|error| format!("invalid URL: {error}"))?; @@ -107,28 +113,15 @@ async fn fetch_link_preview_metadata_inner( let (image_result, favicon_result) = tokio::join!( async { match image_url { - Some(image_url) => Some( - tokio::time::timeout( - PREVIEW_FETCH_TIMEOUT, - fetch_sanitized_image_with_retry(image_url, false), - ) - .await - .unwrap_or(Err(ImageFetchError::Transient { - retry_after: None, - retry_inline: false, - })), - ), + Some(image_url) => { + Some(fetch_sanitized_image_with_retry(image_url, false).await) + } None => None, } }, async { match favicon_url { - Some(favicon_url) => tokio::time::timeout( - PREVIEW_FETCH_TIMEOUT, - fetch_sanitized_image(favicon_url, true), - ) - .await - .ok(), + Some(favicon_url) => Some(fetch_sanitized_image(favicon_url, true).await), None => None, } } @@ -219,9 +212,9 @@ async fn send_pinned_request(url: &Url, accept: &str) -> Result diff --git a/desktop/src-tauri/src/commands/link_preview_youtube.rs b/desktop/src-tauri/src/commands/link_preview_youtube.rs index a0a5a753dcd..b759046d8f0 100644 --- a/desktop/src-tauri/src/commands/link_preview_youtube.rs +++ b/desktop/src-tauri/src/commands/link_preview_youtube.rs @@ -5,8 +5,8 @@ use url::Url; use super::{ apply_image_result, fetch_sanitized_image, normalize_metadata_description, - normalize_metadata_text, read_limited_bytes, send_pinned_request, ImageFetchError, - LinkPreviewImageFetchState, LinkPreviewMetadata, PREVIEW_FETCH_TIMEOUT, + normalize_metadata_text, read_limited_bytes, send_pinned_request, LinkPreviewImageFetchState, + LinkPreviewMetadata, }; const MAX_OEMBED_FETCH_BYTES: usize = 64 * 1024; @@ -54,17 +54,7 @@ pub(super) async fn fetch_oembed_metadata( return Ok(None); }; let image_result = match thumbnail_url { - Some(thumbnail_url) => Some( - tokio::time::timeout( - PREVIEW_FETCH_TIMEOUT, - fetch_sanitized_image(thumbnail_url, false), - ) - .await - .unwrap_or(Err(ImageFetchError::Transient { - retry_after: None, - retry_inline: false, - })), - ), + Some(thumbnail_url) => Some(fetch_sanitized_image(thumbnail_url, false).await), None => None, }; apply_image_result(&mut metadata, image_result); From b4b88d70a984855f20602afd3aa0c9d5c3c7dc61 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Tue, 1 Sep 2026 11:34:29 -0700 Subject: [PATCH 02/11] chore(link-preview): log metadata fetch outcomes in development Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../src/shared/lib/useResolvedLinkPreviews.ts | 30 ++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.ts b/desktop/src/shared/lib/useResolvedLinkPreviews.ts index 4591d54e7dd..b02aa26650c 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.ts +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.ts @@ -222,12 +222,40 @@ function createMetadataLoader({ function fetchLinkPreviewMetadata( href: string, ): Promise { + const startedAt = performance.now(); + const logResult = ( + result: LinkPreviewMetadata | null, + ): LinkPreviewMetadata | null => { + if (import.meta.env?.DEV) { + console.info("[link-preview] metadata fetch completed", { + href, + elapsedMs: Math.round(performance.now() - startedAt), + result: result === null ? "miss" : "hit", + imageFetchState: result?.imageFetchState ?? "none", + imageRetryAfterMs: result?.imageRetryAfterMs ?? null, + hasImage: Boolean(result?.imageDataUrl && result.imageDomain), + hasFavicon: Boolean(result?.faviconDataUrl), + }); + } + return result; + }; + const logFailure = (error: unknown): never => { + if (import.meta.env?.DEV) { + console.warn("[link-preview] metadata fetch failed", { + href, + elapsedMs: Math.round(performance.now() - startedAt), + error, + }); + } + throw error; + }; + return invokeTauri( "fetch_link_preview_metadata", { href, }, - ); + ).then(logResult, logFailure); } const metadataLoader = createMetadataLoader({ From 1593cca9325707a158c48ad6d9641e8297847b79 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Tue, 1 Sep 2026 12:07:10 -0700 Subject: [PATCH 03/11] fix(link-preview): pace image requests by host Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../src-tauri/src/commands/link_preview.rs | 358 ++---------------- .../src/commands/link_preview_rate_limit.rs | 19 +- .../src/commands/link_preview_tests.rs | 325 ++++++++++++++++ 3 files changed, 367 insertions(+), 335 deletions(-) create mode 100644 desktop/src-tauri/src/commands/link_preview_tests.rs diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index a272cdf7c99..f091b4572a1 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -17,7 +17,9 @@ mod rate_limit; #[path = "link_preview_youtube.rs"] mod youtube; -use rate_limit::{image_host_cooldown_remaining, retry_after_duration, set_image_host_cooldown}; +use rate_limit::{ + image_host_cooldown_remaining, image_host_gate, retry_after_duration, set_image_host_cooldown, +}; const MAX_PREVIEW_FETCH_BYTES: usize = 256 * 1024; const MAX_IMAGE_FETCH_BYTES: usize = 2 * 1024 * 1024; @@ -340,12 +342,25 @@ async fn fetch_sanitized_image( validate_public_https_url(&url) .await .map_err(|_| ImageFetchError::Rejected)?; - for redirect_count in 0..=MAX_REDIRECTS { + let mut redirect_count = 0; + let mut waited_for_cooldown = false; + while redirect_count <= MAX_REDIRECTS { if let Some(retry_after) = image_host_cooldown_remaining(&url) { - return Err(ImageFetchError::Transient { - retry_after: Some(retry_after), - retry_inline: false, - }); + if waited_for_cooldown { + return Err(ImageFetchError::Transient { + retry_after: Some(retry_after), + retry_inline: false, + }); + } + waited_for_cooldown = true; + tokio::time::sleep(retry_after).await; + continue; + } + + let host_gate = image_host_gate(&url); + let _host_guard = host_gate.lock().await; + if image_host_cooldown_remaining(&url).is_some() { + continue; } let response = send_pinned_request(&url, "image/jpeg,image/png,image/webp") .await @@ -366,6 +381,8 @@ async fn fetch_sanitized_image( validate_public_https_url(&url) .await .map_err(|_| ImageFetchError::Rejected)?; + redirect_count += 1; + waited_for_cooldown = false; continue; } if !response.status().is_success() { @@ -670,330 +687,5 @@ fn decode_html_entities(value: &str) -> String { } #[cfg(test)] -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, fetch_link_preview_metadata_with, is_html_response, - 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 _; - use bytes::Bytes; - use futures_util::stream; - use image::{DynamicImage, ImageFormat, Rgb, RgbImage, Rgba, RgbaImage}; - use std::{convert::Infallible, io::Cursor}; - use url::Url; - - async fn test_response(router: Router, path: &str) -> reqwest::Response { - let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); - let address = listener.local_addr().unwrap(); - tokio::spawn(async move { - axum::serve(listener, router).await.unwrap(); - }); - reqwest::get(format!("http://{address}{path}")) - .await - .unwrap() - } - - #[tokio::test(start_paused = true)] - async fn metadata_fetch_has_no_native_deadline() { - let fetch = - fetch_link_preview_metadata_with("https://example.com".to_string(), |_| async { - tokio::time::sleep(std::time::Duration::from_secs(60)).await; - Ok(None) - }); - tokio::pin!(fetch); - - tokio::time::advance(std::time::Duration::from_secs(59)).await; - assert!(futures_util::poll!(&mut fetch).is_pending()); - tokio::time::advance(std::time::Duration::from_secs(1)).await; - assert_eq!(fetch.await, Ok(None)); - } - - #[test] - fn metadata_prefers_open_graph_and_reads_site_name() { - let html = r#" - - - Fallback"#; - assert_eq!( - extract_link_preview_metadata(html), - Some(LinkPreviewMetadata { - title: "Rich previews & cards".to_string(), - site_name: Some("Buzz".to_string()), - description: Some("Safe & useful previews".to_string()), - image_data_url: None, - image_domain: None, - image_fetch_state: LinkPreviewImageFetchState::None, - image_retry_after_ms: None, - favicon_data_url: None, - }) - ); - } - - #[test] - fn image_results_preserve_absence_and_classify_recovery() { - let mut metadata = extract_link_preview_metadata("Preview result").unwrap(); - apply_image_result(&mut metadata, None); - assert_eq!(metadata.image_fetch_state, LinkPreviewImageFetchState::None); - - apply_image_result( - &mut metadata, - Some(Err(ImageFetchError::Transient { - retry_after: Some(std::time::Duration::from_secs(15)), - retry_inline: false, - })), - ); - assert_eq!( - metadata.image_fetch_state, - LinkPreviewImageFetchState::TransientFailure - ); - assert_eq!(metadata.image_retry_after_ms, Some(15_000)); - - apply_image_result( - &mut metadata, - Some(Ok(( - "data:image/jpeg;base64,abc".to_string(), - "images.example.com".to_string(), - ))), - ); - assert_eq!( - metadata.image_fetch_state, - LinkPreviewImageFetchState::Image - ); - assert_eq!(metadata.image_domain.as_deref(), Some("images.example.com")); - } - - #[test] - fn metadata_falls_back_to_twitter_then_title() { - assert_eq!( - extract_link_preview_metadata("") - .map(|metadata| metadata.title), - Some("Tweet title".to_string()) - ); - assert_eq!( - extract_link_preview_metadata(" Plain title ") - .map(|metadata| metadata.title), - Some("Plain title".to_string()) - ); - } - - #[test] - fn metadata_preserves_description_line_breaks() { - let html = r#" - "#; - assert_eq!( - extract_link_preview_metadata(html).and_then(|metadata| metadata.description), - Some("First paragraph.\n\nAgents:\n- One\n- Two".to_string()) - ); - } - - #[test] - fn metadata_description_supports_standard_x_posts() { - let description = "x".repeat(MAX_METADATA_DESCRIPTION_CHARS + 1); - let html = format!( - r#""# - ); - let extracted = extract_link_preview_metadata(&html) - .and_then(|metadata| metadata.description) - .unwrap(); - assert_eq!(extracted.chars().count(), MAX_METADATA_DESCRIPTION_CHARS); - } - - #[test] - fn favicon_metadata_resolves_relative_icon_links() { - let page = Url::parse("https://example.com/articles/one").unwrap(); - let html = r#" - "#; - assert_eq!( - extract_favicon_url(html, &page).unwrap().as_str(), - "https://example.com/favicon.png" - ); - } - - #[test] - fn favicon_metadata_prefers_a_supported_raster_candidate() { - let page = Url::parse("https://github.com/block/buzz").unwrap(); - let html = r#" - - "#; - assert_eq!( - extract_favicon_url(html, &page).unwrap().as_str(), - "https://assets.example/favicon.png" - ); - } - - #[test] - fn favicon_metadata_uses_touch_icon_before_unsupported_ico() { - let page = Url::parse("https://twitter.com/tellaho").unwrap(); - let html = r#" - "#; - assert_eq!( - extract_favicon_url(html, &page).unwrap().as_str(), - "https://twitter.com/apple-touch-icon.png" - ); - } - - #[test] - fn image_metadata_resolves_relative_urls_and_prefers_open_graph() { - let page = Url::parse("https://example.com/articles/one").unwrap(); - let html = r#" - "#; - assert_eq!( - extract_image_url(html, &page).unwrap().as_str(), - "https://example.com/preview.png" - ); - } - - #[tokio::test] - async fn oversized_html_uses_metadata_within_the_bounded_prefix() { - const LIMIT: usize = 256; - let metadata = r#""#; - let body = format!("{metadata}{}", "x".repeat(LIMIT)); - let response = test_response( - Router::new().route( - "/declared", - get(move || { - let body = body.clone(); - async move { - Response::builder() - .header("content-type", "text/html") - .body(Body::from(body)) - .unwrap() - } - }), - ), - "/declared", - ) - .await; - assert!(response - .content_length() - .is_some_and(|size| size > LIMIT as u64)); - assert!(is_html_response(&response)); - - let prefix = read_bytes_prefix(response, LIMIT).await.unwrap(); - assert_eq!(prefix.len(), LIMIT); - let html = String::from_utf8_lossy(&prefix); - assert_eq!( - extract_link_preview_metadata(&html).map(|metadata| metadata.title), - Some("Prefix title".to_string()) - ); - assert!(extract_image_url(&html, &Url::parse("https://example.com").unwrap()).is_some()); - } - - #[tokio::test] - async fn image_retry_after_uses_bounded_delta_seconds() { - let response = test_response( - Router::new().route( - "/rate-limited", - get(|| async { - Response::builder() - .status(429) - .header("retry-after", "900") - .body(Body::empty()) - .unwrap() - }), - ), - "/rate-limited", - ) - .await; - assert_eq!( - retry_after_duration(&response), - Some(std::time::Duration::from_secs(900)) - ); - - let response = test_response( - Router::new().route( - "/excessive", - get(|| async { - Response::builder() - .status(429) - .header("retry-after", "7200") - .body(Body::empty()) - .unwrap() - }), - ), - "/excessive", - ) - .await; - assert_eq!(retry_after_duration(&response), Some(MAX_IMAGE_RETRY_AFTER)); - } - - #[tokio::test] - async fn oversized_chunked_html_ignores_metadata_beyond_the_bounded_prefix() { - const LIMIT: usize = 256; - let response = test_response( - Router::new().route( - "/chunked", - get(|| async { - let chunks = stream::iter([ - Ok::<_, Infallible>(Bytes::from(vec![b'x'; LIMIT])), - Ok(Bytes::from_static( - br#""#, - )), - ]); - Response::builder() - .header("content-type", "text/html") - .body(Body::from_stream(chunks)) - .unwrap() - }), - ), - "/chunked", - ) - .await; - assert_eq!(response.content_length(), None); - - let prefix = read_bytes_prefix(response, LIMIT).await.unwrap(); - assert_eq!(prefix.len(), LIMIT); - let html = String::from_utf8_lossy(&prefix); - assert_eq!(extract_link_preview_metadata(&html), None); - assert_eq!( - extract_image_url(&html, &Url::parse("https://example.com").unwrap()), - None - ); - } - - #[test] - fn sanitizer_rejects_mime_mismatch_and_outputs_static_jpeg() { - let source = DynamicImage::ImageRgb8(RgbImage::from_pixel(2, 2, Rgb([10, 20, 30]))); - let mut png = Cursor::new(Vec::new()); - source.write_to(&mut png, ImageFormat::Png).unwrap(); - assert!(sanitize_image(png.get_ref(), "image/jpeg", false).is_err()); - let sanitized = sanitize_image(png.get_ref(), "image/png", false).unwrap(); - assert!(sanitized.starts_with("data:image/jpeg;base64,")); - } - - #[test] - fn favicon_sanitizer_preserves_png_transparency() { - let source = DynamicImage::ImageRgba8(RgbaImage::from_pixel(2, 2, Rgba([36, 41, 47, 0]))); - let mut png = Cursor::new(Vec::new()); - source.write_to(&mut png, ImageFormat::Png).unwrap(); - - let sanitized = sanitize_image(png.get_ref(), "image/png", true).unwrap(); - assert!(sanitized.starts_with("data:image/png;base64,")); - let encoded = sanitized.split_once(',').unwrap().1; - let bytes = base64::engine::general_purpose::STANDARD - .decode(encoded) - .unwrap(); - assert!(image::load_from_memory(&bytes).unwrap().color().has_alpha()); - } - - #[test] - fn animation_markers_are_rejected_before_decode() { - let mut apng = b"\x89PNG\r\n\x1a\n".to_vec(); - apng.extend_from_slice(b"junkacTLjunk"); - assert!(declares_animation(&apng, ImageFormat::Png)); - - let mut webp = b"RIFF\x00\x00\x00\x00WEBPVP8X\x0a\x00\x00\x00".to_vec(); - webp.push(0x02); - assert!(declares_animation(&webp, ImageFormat::WebP)); - } - - #[test] - fn metadata_requires_a_non_empty_title() { - assert_eq!(extract_link_preview_metadata(" "), None); - assert_eq!(extract_link_preview_metadata(""), None); - } -} +#[path = "link_preview_tests.rs"] +mod tests; 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 c3f7ed7188c..8a997b3cd8f 100644 --- a/desktop/src-tauri/src/commands/link_preview_rate_limit.rs +++ b/desktop/src-tauri/src/commands/link_preview_rate_limit.rs @@ -1,16 +1,31 @@ use std::{ - collections::HashMap, - sync::{Mutex, OnceLock}, + collections::{hash_map::DefaultHasher, HashMap}, + hash::{Hash, Hasher}, + sync::{LazyLock, Mutex, OnceLock}, time::{Duration, Instant}, }; use reqwest::header::RETRY_AFTER; +use tokio::sync::Mutex as AsyncMutex; use url::Url; pub(super) const MAX_IMAGE_RETRY_AFTER: Duration = Duration::from_secs(60 * 60); const MAX_IMAGE_HOST_COOLDOWNS: usize = 128; +const IMAGE_HOST_GATE_COUNT: usize = 64; static IMAGE_HOST_COOLDOWNS: OnceLock>> = OnceLock::new(); +// A bounded stripe table serializes image requests by host without retaining an +// unbounded attacker-controlled hostname map. Hash collisions only make two +// unrelated hosts wait for one another; they never weaken the host boundary. +static IMAGE_HOST_GATES: LazyLock<[AsyncMutex<()>; IMAGE_HOST_GATE_COUNT]> = + LazyLock::new(|| std::array::from_fn(|_| AsyncMutex::new(()))); + +pub(super) fn image_host_gate(url: &Url) -> &'static AsyncMutex<()> { + let mut hasher = DefaultHasher::new(); + url.host_str().unwrap_or_default().hash(&mut hasher); + let index = (hasher.finish() as usize) % IMAGE_HOST_GATE_COUNT; + &IMAGE_HOST_GATES[index] +} pub(super) fn retry_after_duration(response: &reqwest::Response) -> Option { response diff --git a/desktop/src-tauri/src/commands/link_preview_tests.rs b/desktop/src-tauri/src/commands/link_preview_tests.rs new file mode 100644 index 00000000000..3477d659024 --- /dev/null +++ b/desktop/src-tauri/src/commands/link_preview_tests.rs @@ -0,0 +1,325 @@ + +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, fetch_link_preview_metadata_with, is_html_response, + 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 _; +use bytes::Bytes; +use futures_util::stream; +use image::{DynamicImage, ImageFormat, Rgb, RgbImage, Rgba, RgbaImage}; +use std::{convert::Infallible, io::Cursor}; +use url::Url; + +async fn test_response(router: Router, path: &str) -> reqwest::Response { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + tokio::spawn(async move { + axum::serve(listener, router).await.unwrap(); + }); + reqwest::get(format!("http://{address}{path}")) + .await + .unwrap() +} + +#[tokio::test(start_paused = true)] +async fn metadata_fetch_has_no_native_deadline() { + let fetch = fetch_link_preview_metadata_with("https://example.com".to_string(), |_| async { + tokio::time::sleep(std::time::Duration::from_secs(60)).await; + Ok(None) + }); + tokio::pin!(fetch); + + tokio::time::advance(std::time::Duration::from_secs(59)).await; + assert!(futures_util::poll!(&mut fetch).is_pending()); + tokio::time::advance(std::time::Duration::from_secs(1)).await; + assert_eq!(fetch.await, Ok(None)); +} + +#[test] +fn metadata_prefers_open_graph_and_reads_site_name() { + let html = r#" + + + Fallback"#; + assert_eq!( + extract_link_preview_metadata(html), + Some(LinkPreviewMetadata { + title: "Rich previews & cards".to_string(), + site_name: Some("Buzz".to_string()), + description: Some("Safe & useful previews".to_string()), + image_data_url: None, + image_domain: None, + image_fetch_state: LinkPreviewImageFetchState::None, + image_retry_after_ms: None, + favicon_data_url: None, + }) + ); +} + +#[test] +fn image_results_preserve_absence_and_classify_recovery() { + let mut metadata = extract_link_preview_metadata("Preview result").unwrap(); + apply_image_result(&mut metadata, None); + assert_eq!(metadata.image_fetch_state, LinkPreviewImageFetchState::None); + + apply_image_result( + &mut metadata, + Some(Err(ImageFetchError::Transient { + retry_after: Some(std::time::Duration::from_secs(15)), + retry_inline: false, + })), + ); + assert_eq!( + metadata.image_fetch_state, + LinkPreviewImageFetchState::TransientFailure + ); + assert_eq!(metadata.image_retry_after_ms, Some(15_000)); + + apply_image_result( + &mut metadata, + Some(Ok(( + "data:image/jpeg;base64,abc".to_string(), + "images.example.com".to_string(), + ))), + ); + assert_eq!( + metadata.image_fetch_state, + LinkPreviewImageFetchState::Image + ); + assert_eq!(metadata.image_domain.as_deref(), Some("images.example.com")); +} + +#[test] +fn metadata_falls_back_to_twitter_then_title() { + assert_eq!( + extract_link_preview_metadata("") + .map(|metadata| metadata.title), + Some("Tweet title".to_string()) + ); + assert_eq!( + extract_link_preview_metadata(" Plain title ") + .map(|metadata| metadata.title), + Some("Plain title".to_string()) + ); +} + +#[test] +fn metadata_preserves_description_line_breaks() { + let html = r#" + "#; + assert_eq!( + extract_link_preview_metadata(html).and_then(|metadata| metadata.description), + Some("First paragraph.\n\nAgents:\n- One\n- Two".to_string()) + ); +} + +#[test] +fn metadata_description_supports_standard_x_posts() { + let description = "x".repeat(MAX_METADATA_DESCRIPTION_CHARS + 1); + let html = format!( + r#""# + ); + let extracted = extract_link_preview_metadata(&html) + .and_then(|metadata| metadata.description) + .unwrap(); + assert_eq!(extracted.chars().count(), MAX_METADATA_DESCRIPTION_CHARS); +} + +#[test] +fn favicon_metadata_resolves_relative_icon_links() { + let page = Url::parse("https://example.com/articles/one").unwrap(); + let html = r#" + "#; + assert_eq!( + extract_favicon_url(html, &page).unwrap().as_str(), + "https://example.com/favicon.png" + ); +} + +#[test] +fn favicon_metadata_prefers_a_supported_raster_candidate() { + let page = Url::parse("https://github.com/block/buzz").unwrap(); + let html = r#" + + "#; + assert_eq!( + extract_favicon_url(html, &page).unwrap().as_str(), + "https://assets.example/favicon.png" + ); +} + +#[test] +fn favicon_metadata_uses_touch_icon_before_unsupported_ico() { + let page = Url::parse("https://twitter.com/tellaho").unwrap(); + let html = r#" + "#; + assert_eq!( + extract_favicon_url(html, &page).unwrap().as_str(), + "https://twitter.com/apple-touch-icon.png" + ); +} + +#[test] +fn image_metadata_resolves_relative_urls_and_prefers_open_graph() { + let page = Url::parse("https://example.com/articles/one").unwrap(); + let html = r#" + "#; + assert_eq!( + extract_image_url(html, &page).unwrap().as_str(), + "https://example.com/preview.png" + ); +} + +#[tokio::test] +async fn oversized_html_uses_metadata_within_the_bounded_prefix() { + const LIMIT: usize = 256; + let metadata = r#""#; + let body = format!("{metadata}{}", "x".repeat(LIMIT)); + let response = test_response( + Router::new().route( + "/declared", + get(move || { + let body = body.clone(); + async move { + Response::builder() + .header("content-type", "text/html") + .body(Body::from(body)) + .unwrap() + } + }), + ), + "/declared", + ) + .await; + assert!(response + .content_length() + .is_some_and(|size| size > LIMIT as u64)); + assert!(is_html_response(&response)); + + let prefix = read_bytes_prefix(response, LIMIT).await.unwrap(); + assert_eq!(prefix.len(), LIMIT); + let html = String::from_utf8_lossy(&prefix); + assert_eq!( + extract_link_preview_metadata(&html).map(|metadata| metadata.title), + Some("Prefix title".to_string()) + ); + assert!(extract_image_url(&html, &Url::parse("https://example.com").unwrap()).is_some()); +} + +#[tokio::test] +async fn image_retry_after_uses_bounded_delta_seconds() { + let response = test_response( + Router::new().route( + "/rate-limited", + get(|| async { + Response::builder() + .status(429) + .header("retry-after", "900") + .body(Body::empty()) + .unwrap() + }), + ), + "/rate-limited", + ) + .await; + assert_eq!( + retry_after_duration(&response), + Some(std::time::Duration::from_secs(900)) + ); + + let response = test_response( + Router::new().route( + "/excessive", + get(|| async { + Response::builder() + .status(429) + .header("retry-after", "7200") + .body(Body::empty()) + .unwrap() + }), + ), + "/excessive", + ) + .await; + assert_eq!(retry_after_duration(&response), Some(MAX_IMAGE_RETRY_AFTER)); +} + +#[tokio::test] +async fn oversized_chunked_html_ignores_metadata_beyond_the_bounded_prefix() { + const LIMIT: usize = 256; + let response = test_response( + Router::new().route( + "/chunked", + get(|| async { + let chunks = stream::iter([ + Ok::<_, Infallible>(Bytes::from(vec![b'x'; LIMIT])), + Ok(Bytes::from_static( + br#""#, + )), + ]); + Response::builder() + .header("content-type", "text/html") + .body(Body::from_stream(chunks)) + .unwrap() + }), + ), + "/chunked", + ) + .await; + assert_eq!(response.content_length(), None); + + let prefix = read_bytes_prefix(response, LIMIT).await.unwrap(); + assert_eq!(prefix.len(), LIMIT); + let html = String::from_utf8_lossy(&prefix); + assert_eq!(extract_link_preview_metadata(&html), None); + assert_eq!( + extract_image_url(&html, &Url::parse("https://example.com").unwrap()), + None + ); +} + +#[test] +fn sanitizer_rejects_mime_mismatch_and_outputs_static_jpeg() { + let source = DynamicImage::ImageRgb8(RgbImage::from_pixel(2, 2, Rgb([10, 20, 30]))); + let mut png = Cursor::new(Vec::new()); + source.write_to(&mut png, ImageFormat::Png).unwrap(); + assert!(sanitize_image(png.get_ref(), "image/jpeg", false).is_err()); + let sanitized = sanitize_image(png.get_ref(), "image/png", false).unwrap(); + assert!(sanitized.starts_with("data:image/jpeg;base64,")); +} + +#[test] +fn favicon_sanitizer_preserves_png_transparency() { + let source = DynamicImage::ImageRgba8(RgbaImage::from_pixel(2, 2, Rgba([36, 41, 47, 0]))); + let mut png = Cursor::new(Vec::new()); + source.write_to(&mut png, ImageFormat::Png).unwrap(); + + let sanitized = sanitize_image(png.get_ref(), "image/png", true).unwrap(); + assert!(sanitized.starts_with("data:image/png;base64,")); + let encoded = sanitized.split_once(',').unwrap().1; + let bytes = base64::engine::general_purpose::STANDARD + .decode(encoded) + .unwrap(); + assert!(image::load_from_memory(&bytes).unwrap().color().has_alpha()); +} + +#[test] +fn animation_markers_are_rejected_before_decode() { + let mut apng = b"\x89PNG\r\n\x1a\n".to_vec(); + apng.extend_from_slice(b"junkacTLjunk"); + assert!(declares_animation(&apng, ImageFormat::Png)); + + let mut webp = b"RIFF\x00\x00\x00\x00WEBPVP8X\x0a\x00\x00\x00".to_vec(); + webp.push(0x02); + assert!(declares_animation(&webp, ImageFormat::WebP)); +} + +#[test] +fn metadata_requires_a_non_empty_title() { + assert_eq!(extract_link_preview_metadata(" "), None); + assert_eq!(extract_link_preview_metadata(""), None); +} From c634b7f4771e471cbcdb1364536ca1b2c5fda8fc Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Tue, 1 Sep 2026 13:35:19 -0700 Subject: [PATCH 04/11] fix(link-preview): align first rate limit with queued retries Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../src-tauri/src/commands/link_preview.rs | 55 ++++++++++++------- .../src/commands/link_preview_rate_limit.rs | 4 +- .../src/commands/link_preview_tests.rs | 45 ++++++++++----- 3 files changed, 68 insertions(+), 36 deletions(-) diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index f091b4572a1..629911624f6 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -1,4 +1,4 @@ -use std::{future::Future, io::Cursor, net::IpAddr, time::Duration}; +use std::{io::Cursor, net::IpAddr, time::Duration}; use base64::{engine::general_purpose::STANDARD as BASE64_STANDARD, Engine as _}; use futures_util::StreamExt; @@ -56,20 +56,7 @@ pub struct LinkPreviewMetadata { pub async fn fetch_link_preview_metadata( href: String, ) -> Result, String> { - fetch_link_preview_metadata_with(href, fetch_link_preview_metadata_for_url).await -} - -/// Run metadata fetching without a native deadline. The renderer owns the -/// finite budget after a composer generation is promoted into a send. -async fn fetch_link_preview_metadata_with( - href: String, - fetch: F, -) -> Result, String> -where - F: FnOnce(String) -> Fut, - Fut: Future, String>>, -{ - fetch(href).await + fetch_link_preview_metadata_for_url(href).await } async fn fetch_link_preview_metadata_for_url( @@ -335,6 +322,32 @@ async fn fetch_sanitized_image_with_retry( .await } +async fn wait_for_image_host_cooldown( + waited_for_cooldown: &mut bool, + retry_after: Duration, +) -> bool { + if *waited_for_cooldown { + return false; + } + *waited_for_cooldown = true; + tokio::time::sleep(retry_after).await; + true +} + +fn retryable_image_cooldown( + url: &Url, + retry_after: Option, + waited_for_cooldown: &mut bool, +) -> Option { + let retry_after = retry_after?; + set_image_host_cooldown(url, retry_after); + if *waited_for_cooldown { + return None; + } + *waited_for_cooldown = true; + Some(retry_after) +} + async fn fetch_sanitized_image( mut url: Url, preserve_transparency: bool, @@ -346,14 +359,12 @@ async fn fetch_sanitized_image( let mut waited_for_cooldown = false; while redirect_count <= MAX_REDIRECTS { if let Some(retry_after) = image_host_cooldown_remaining(&url) { - if waited_for_cooldown { + if !wait_for_image_host_cooldown(&mut waited_for_cooldown, retry_after).await { return Err(ImageFetchError::Transient { retry_after: Some(retry_after), retry_inline: false, }); } - waited_for_cooldown = true; - tokio::time::sleep(retry_after).await; continue; } @@ -393,8 +404,12 @@ async fn fetch_sanitized_image( || status.is_server_error() { let retry_after = retry_after_duration(&response); - if let Some(retry_after) = retry_after { - set_image_host_cooldown(&url, retry_after); + if let Some(retry_after) = + retryable_image_cooldown(&url, retry_after, &mut waited_for_cooldown) + { + drop(_host_guard); + tokio::time::sleep(retry_after).await; + continue; } return Err(ImageFetchError::Transient { retry_after, 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 8a997b3cd8f..46032fc5f01 100644 --- a/desktop/src-tauri/src/commands/link_preview_rate_limit.rs +++ b/desktop/src-tauri/src/commands/link_preview_rate_limit.rs @@ -2,11 +2,11 @@ use std::{ collections::{hash_map::DefaultHasher, HashMap}, hash::{Hash, Hasher}, sync::{LazyLock, Mutex, OnceLock}, - time::{Duration, Instant}, + time::Duration, }; use reqwest::header::RETRY_AFTER; -use tokio::sync::Mutex as AsyncMutex; +use tokio::{sync::Mutex as AsyncMutex, time::Instant}; use url::Url; pub(super) const MAX_IMAGE_RETRY_AFTER: Duration = Duration::from_secs(60 * 60); diff --git a/desktop/src-tauri/src/commands/link_preview_tests.rs b/desktop/src-tauri/src/commands/link_preview_tests.rs index 3477d659024..f17f3465b35 100644 --- a/desktop/src-tauri/src/commands/link_preview_tests.rs +++ b/desktop/src-tauri/src/commands/link_preview_tests.rs @@ -1,10 +1,10 @@ - use super::rate_limit::MAX_IMAGE_RETRY_AFTER; +use super::retryable_image_cooldown; use super::{ apply_image_result, declares_animation, extract_favicon_url, extract_image_url, - extract_link_preview_metadata, fetch_link_preview_metadata_with, 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, read_bytes_prefix, retry_after_duration, + sanitize_image, wait_for_image_host_cooldown, ImageFetchError, LinkPreviewImageFetchState, + LinkPreviewMetadata, MAX_METADATA_DESCRIPTION_CHARS, }; use axum::{body::Body, http::Response, routing::get, Router}; use base64::Engine as _; @@ -26,17 +26,34 @@ async fn test_response(router: Router, path: &str) -> reqwest::Response { } #[tokio::test(start_paused = true)] -async fn metadata_fetch_has_no_native_deadline() { - let fetch = fetch_link_preview_metadata_with("https://example.com".to_string(), |_| async { - tokio::time::sleep(std::time::Duration::from_secs(60)).await; - Ok(None) - }); - tokio::pin!(fetch); +async fn first_rate_limit_and_queued_host_request_share_one_cooldown_boundary() { + let cooldown = std::time::Duration::from_secs(60); + let url = Url::parse("https://rate-limit-regression.example/image.png").unwrap(); + let first = async { + let mut waited = false; + let retry_after = retryable_image_cooldown(&url, Some(cooldown), &mut waited).unwrap(); + tokio::time::sleep(retry_after).await; + assert_eq!( + retryable_image_cooldown(&url, Some(cooldown), &mut waited), + None + ); + }; + let queued = async { + let mut waited = false; + assert!(wait_for_image_host_cooldown(&mut waited, cooldown).await); + }; + tokio::pin!(first); - tokio::time::advance(std::time::Duration::from_secs(59)).await; - assert!(futures_util::poll!(&mut fetch).is_pending()); - tokio::time::advance(std::time::Duration::from_secs(1)).await; - assert_eq!(fetch.await, Ok(None)); + assert!(futures_util::poll!(&mut first).is_pending()); + assert_eq!(super::image_host_cooldown_remaining(&url), Some(cooldown)); + tokio::pin!(queued); + assert!(futures_util::poll!(&mut queued).is_pending()); + tokio::time::advance(cooldown - std::time::Duration::from_millis(1)).await; + assert!(futures_util::poll!(&mut first).is_pending()); + assert!(futures_util::poll!(&mut queued).is_pending()); + tokio::time::advance(std::time::Duration::from_millis(1)).await; + first.await; + queued.await; } #[test] From 52ba827187b697b60e4ba71482aa823ebc6e6c46 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Tue, 1 Sep 2026 13:47:22 -0700 Subject: [PATCH 05/11] test(link-preview): cover shared cooldown retries Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../src-tauri/src/commands/link_preview.rs | 29 ++++- .../src/commands/link_preview_tests.rs | 110 +++++++++++++----- 2 files changed, 108 insertions(+), 31 deletions(-) diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index 629911624f6..b809296f91a 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -349,10 +349,31 @@ fn retryable_image_cooldown( } async fn fetch_sanitized_image( - mut url: Url, + url: Url, preserve_transparency: bool, ) -> Result<(String, String), ImageFetchError> { - validate_public_https_url(&url) + fetch_sanitized_image_using( + url, + preserve_transparency, + |url| async move { validate_public_https_url(&url).await }, + |url, accept| async move { send_pinned_request(&url, accept).await }, + ) + .await +} + +async fn fetch_sanitized_image_using( + mut url: Url, + preserve_transparency: bool, + mut validate_url: V, + mut send_request: F, +) -> Result<(String, String), ImageFetchError> +where + V: FnMut(Url) -> VFut, + VFut: std::future::Future>, + F: FnMut(Url, &'static str) -> Fut, + Fut: std::future::Future>, +{ + validate_url(url.clone()) .await .map_err(|_| ImageFetchError::Rejected)?; let mut redirect_count = 0; @@ -373,7 +394,7 @@ async fn fetch_sanitized_image( if image_host_cooldown_remaining(&url).is_some() { continue; } - let response = send_pinned_request(&url, "image/jpeg,image/png,image/webp") + let response = send_request(url.clone(), "image/jpeg,image/png,image/webp") .await .map_err(|_| ImageFetchError::Transient { retry_after: None, @@ -389,7 +410,7 @@ async fn fetch_sanitized_image( .and_then(|value| value.to_str().ok()) .ok_or(ImageFetchError::Rejected)?; url = url.join(location).map_err(|_| ImageFetchError::Rejected)?; - validate_public_https_url(&url) + validate_url(url.clone()) .await .map_err(|_| ImageFetchError::Rejected)?; redirect_count += 1; diff --git a/desktop/src-tauri/src/commands/link_preview_tests.rs b/desktop/src-tauri/src/commands/link_preview_tests.rs index f17f3465b35..f8849a4963a 100644 --- a/desktop/src-tauri/src/commands/link_preview_tests.rs +++ b/desktop/src-tauri/src/commands/link_preview_tests.rs @@ -1,25 +1,33 @@ use super::rate_limit::MAX_IMAGE_RETRY_AFTER; -use super::retryable_image_cooldown; 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, wait_for_image_host_cooldown, ImageFetchError, LinkPreviewImageFetchState, - LinkPreviewMetadata, MAX_METADATA_DESCRIPTION_CHARS, + extract_link_preview_metadata, fetch_sanitized_image_using, is_html_response, + 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 _; use bytes::Bytes; use futures_util::stream; use image::{DynamicImage, ImageFormat, Rgb, RgbImage, Rgba, RgbaImage}; -use std::{convert::Infallible, io::Cursor}; +use std::{ + convert::Infallible, + io::Cursor, + sync::{Arc, Mutex}, +}; use url::Url; -async fn test_response(router: Router, path: &str) -> reqwest::Response { +async fn start_test_server(router: Router) -> std::net::SocketAddr { let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let address = listener.local_addr().unwrap(); tokio::spawn(async move { axum::serve(listener, router).await.unwrap(); }); + address +} + +async fn test_response(router: Router, path: &str) -> reqwest::Response { + let address = start_test_server(router).await; reqwest::get(format!("http://{address}{path}")) .await .unwrap() @@ -29,31 +37,79 @@ async fn test_response(router: Router, path: &str) -> reqwest::Response { async fn first_rate_limit_and_queued_host_request_share_one_cooldown_boundary() { let cooldown = std::time::Duration::from_secs(60); let url = Url::parse("https://rate-limit-regression.example/image.png").unwrap(); - let first = async { - let mut waited = false; - let retry_after = retryable_image_cooldown(&url, Some(cooldown), &mut waited).unwrap(); - tokio::time::sleep(retry_after).await; - assert_eq!( - retryable_image_cooldown(&url, Some(cooldown), &mut waited), - None - ); - }; - let queued = async { - let mut waited = false; - assert!(wait_for_image_host_cooldown(&mut waited, cooldown).await); + let attempts = Arc::new(Mutex::new(0)); + let image = DynamicImage::ImageRgb8(RgbImage::from_pixel(2, 2, Rgb([10, 20, 30]))); + let mut png = Cursor::new(Vec::new()); + image.write_to(&mut png, ImageFormat::Png).unwrap(); + let image_bytes = png.into_inner(); + let server_attempts = Arc::clone(&attempts); + let address = start_test_server(Router::new().route( + "/image.png", + get(move || { + let image_bytes = image_bytes.clone(); + let server_attempts = Arc::clone(&server_attempts); + async move { + let attempt = { + let mut attempts = server_attempts.lock().unwrap(); + *attempts += 1; + *attempts + }; + if attempt == 1 { + Response::builder() + .status(429) + .header("retry-after", cooldown.as_secs()) + .body(Body::empty()) + .unwrap() + } else { + Response::builder() + .header("content-type", "image/png") + .body(Body::from(image_bytes)) + .unwrap() + } + } + }), + )) + .await; + let test_client = reqwest::Client::new(); + let request = move |url: Url, _accept: &'static str| { + let test_client = test_client.clone(); + async move { + test_client + .get(format!("http://{address}{}", url.path())) + .send() + .await + .map_err(|error| error.to_string()) + } }; - tokio::pin!(first); - - assert!(futures_util::poll!(&mut first).is_pending()); + let validate = |_url: Url| async { Ok(()) }; + let first = tokio::spawn(fetch_sanitized_image_using( + url.clone(), + false, + validate, + request.clone(), + )); + while super::image_host_cooldown_remaining(&url).is_none() { + tokio::task::yield_now().await; + } + assert!(!first.is_finished()); + assert_eq!(*attempts.lock().unwrap(), 1); assert_eq!(super::image_host_cooldown_remaining(&url), Some(cooldown)); - tokio::pin!(queued); - assert!(futures_util::poll!(&mut queued).is_pending()); + + let queued = tokio::spawn(fetch_sanitized_image_using(url, false, validate, request)); + tokio::task::yield_now().await; + assert!(!queued.is_finished()); + assert_eq!(*attempts.lock().unwrap(), 1); + tokio::time::advance(cooldown - std::time::Duration::from_millis(1)).await; - assert!(futures_util::poll!(&mut first).is_pending()); - assert!(futures_util::poll!(&mut queued).is_pending()); + assert!(!first.is_finished()); + assert!(!queued.is_finished()); + assert_eq!(*attempts.lock().unwrap(), 1); + tokio::time::advance(std::time::Duration::from_millis(1)).await; - first.await; - queued.await; + let (first, queued) = tokio::join!(first, queued); + assert!(first.unwrap().is_ok()); + assert!(queued.unwrap().is_ok()); + assert_eq!(*attempts.lock().unwrap(), 3); } #[test] From f3a30832fb2c4340783126804742a72a607ba01f Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Tue, 1 Sep 2026 14:14:37 -0700 Subject: [PATCH 06/11] fix(link-preview): bound transport and cooldown waits Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../src-tauri/src/commands/link_preview.rs | 38 ++++-- .../src/commands/link_preview_tests.rs | 124 ++++++++++++++---- 2 files changed, 128 insertions(+), 34 deletions(-) diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index b809296f91a..d5330501199 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -26,6 +26,10 @@ 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 TRANSPORT_CONNECT_TIMEOUT: Duration = Duration::from_secs(15); +const TRANSPORT_IDLE_TIMEOUT: Duration = Duration::from_secs(30); +const DNS_RESOLUTION_TIMEOUT: Duration = Duration::from_secs(15); +const MAX_INLINE_IMAGE_COOLDOWN: Duration = Duration::from_secs(30); const MAX_REDIRECTS: usize = 3; const MAX_METADATA_CHARS: usize = 180; const MAX_METADATA_DESCRIPTION_CHARS: usize = 280; @@ -164,11 +168,15 @@ async fn validate_public_https_url(url: &Url) -> Result<(), String> { async fn resolve_public_addresses(host: &str) -> Result, String> { let host = host.to_string(); - let addresses = tokio::net::lookup_host((host.as_str(), 443)) - .await - .map_err(|error| format!("link preview DNS resolution failed: {error}"))? - .map(|address| address.ip()) - .collect::>(); + let addresses = tokio::time::timeout( + DNS_RESOLUTION_TIMEOUT, + tokio::net::lookup_host((host.as_str(), 443)), + ) + .await + .map_err(|_| "link preview DNS resolution timed out".to_string())? + .map_err(|error| format!("link preview DNS resolution failed: {error}"))? + .map(|address| address.ip()) + .collect::>(); if addresses.is_empty() { return Err("link preview DNS resolution returned no addresses".to_string()); @@ -193,6 +201,8 @@ async fn send_pinned_request(url: &Url, accept: &str) -> Result Option { let retry_after = retry_after?; - set_image_host_cooldown(url, retry_after); if *waited_for_cooldown { return None; } + set_image_host_cooldown(url, retry_after); + if retry_after > MAX_INLINE_IMAGE_COOLDOWN { + return None; + } *waited_for_cooldown = true; Some(retry_after) } @@ -380,7 +393,9 @@ where let mut waited_for_cooldown = false; while redirect_count <= MAX_REDIRECTS { if let Some(retry_after) = image_host_cooldown_remaining(&url) { - if !wait_for_image_host_cooldown(&mut waited_for_cooldown, retry_after).await { + if retry_after > MAX_INLINE_IMAGE_COOLDOWN + || !wait_for_image_host_cooldown(&mut waited_for_cooldown, retry_after).await + { return Err(ImageFetchError::Transient { retry_after: Some(retry_after), retry_inline: false, @@ -390,7 +405,7 @@ where } let host_gate = image_host_gate(&url); - let _host_guard = host_gate.lock().await; + let host_guard = host_gate.lock().await; if image_host_cooldown_remaining(&url).is_some() { continue; } @@ -414,7 +429,6 @@ where .await .map_err(|_| ImageFetchError::Rejected)?; redirect_count += 1; - waited_for_cooldown = false; continue; } if !response.status().is_success() { @@ -428,13 +442,15 @@ where if let Some(retry_after) = retryable_image_cooldown(&url, retry_after, &mut waited_for_cooldown) { - drop(_host_guard); + drop(host_guard); tokio::time::sleep(retry_after).await; continue; } return Err(ImageFetchError::Transient { retry_after, - retry_inline: status != reqwest::StatusCode::TOO_MANY_REQUESTS, + retry_inline: retry_after.is_none() + && status != reqwest::StatusCode::TOO_MANY_REQUESTS + && !waited_for_cooldown, }); } return Err(ImageFetchError::Rejected); diff --git a/desktop/src-tauri/src/commands/link_preview_tests.rs b/desktop/src-tauri/src/commands/link_preview_tests.rs index f8849a4963a..cd870f119f8 100644 --- a/desktop/src-tauri/src/commands/link_preview_tests.rs +++ b/desktop/src-tauri/src/commands/link_preview_tests.rs @@ -2,8 +2,9 @@ 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, fetch_sanitized_image_using, is_html_response, - read_bytes_prefix, retry_after_duration, sanitize_image, ImageFetchError, - LinkPreviewImageFetchState, LinkPreviewMetadata, MAX_METADATA_DESCRIPTION_CHARS, + read_bytes_prefix, retry_after_duration, retryable_image_cooldown, sanitize_image, + ImageFetchError, LinkPreviewImageFetchState, LinkPreviewMetadata, MAX_INLINE_IMAGE_COOLDOWN, + MAX_METADATA_DESCRIPTION_CHARS, }; use axum::{body::Body, http::Response, routing::get, Router}; use base64::Engine as _; @@ -15,6 +16,7 @@ use std::{ io::Cursor, sync::{Arc, Mutex}, }; +use tokio::sync::oneshot; use url::Url; async fn start_test_server(router: Router) -> std::net::SocketAddr { @@ -35,39 +37,48 @@ async fn test_response(router: Router, path: &str) -> reqwest::Response { #[tokio::test(start_paused = true)] async fn first_rate_limit_and_queued_host_request_share_one_cooldown_boundary() { - let cooldown = std::time::Duration::from_secs(60); - let url = Url::parse("https://rate-limit-regression.example/image.png").unwrap(); + let cooldown = std::time::Duration::from_secs(20); + let rate_limited_path = "/rate-limited.png"; + let success_path = "/success.png"; + let url = Url::parse(&format!( + "https://rate-limit-regression.example{rate_limited_path}" + )) + .unwrap(); let attempts = Arc::new(Mutex::new(0)); + let collision_attempts = Arc::new(Mutex::new(0)); let image = DynamicImage::ImageRgb8(RgbImage::from_pixel(2, 2, Rgb([10, 20, 30]))); let mut png = Cursor::new(Vec::new()); image.write_to(&mut png, ImageFormat::Png).unwrap(); let image_bytes = png.into_inner(); let server_attempts = Arc::clone(&attempts); let address = start_test_server(Router::new().route( - "/image.png", - get(move || { - let image_bytes = image_bytes.clone(); - let server_attempts = Arc::clone(&server_attempts); - async move { - let attempt = { - let mut attempts = server_attempts.lock().unwrap(); - *attempts += 1; - *attempts - }; - if attempt == 1 { - Response::builder() - .status(429) - .header("retry-after", cooldown.as_secs()) - .body(Body::empty()) - .unwrap() - } else { + "/{image}", + get( + move |axum::extract::Path(image): axum::extract::Path| { + let image_bytes = image_bytes.clone(); + let server_attempts = Arc::clone(&server_attempts); + async move { + if image == "rate-limited.png" { + let attempt = { + let mut attempts = server_attempts.lock().unwrap(); + *attempts += 1; + *attempts + }; + if attempt == 1 { + return Response::builder() + .status(429) + .header("retry-after", cooldown.as_secs()) + .body(Body::empty()) + .unwrap(); + } + } Response::builder() .header("content-type", "image/png") .body(Body::from(image_bytes)) .unwrap() } - } - }), + }, + ), )) .await; let test_client = reqwest::Client::new(); @@ -81,6 +92,22 @@ async fn first_rate_limit_and_queued_host_request_share_one_cooldown_boundary() .map_err(|error| error.to_string()) } }; + let collision_request = { + let collision_attempts = Arc::clone(&collision_attempts); + let test_client = reqwest::Client::new(); + move |url: Url, _accept: &'static str| { + let collision_attempts = Arc::clone(&collision_attempts); + let test_client = test_client.clone(); + async move { + *collision_attempts.lock().unwrap() += 1; + test_client + .get(format!("http://{address}{}", url.path())) + .send() + .await + .map_err(|error| error.to_string()) + } + } + }; let validate = |_url: Url| async { Ok(()) }; let first = tokio::spawn(fetch_sanitized_image_using( url.clone(), @@ -95,6 +122,31 @@ async fn first_rate_limit_and_queued_host_request_share_one_cooldown_boundary() assert_eq!(*attempts.lock().unwrap(), 1); assert_eq!(super::image_host_cooldown_remaining(&url), Some(cooldown)); + let colliding_url = (0..10_000) + .map(|index| { + Url::parse(&format!("https://collision-{index}.example{success_path}")).unwrap() + }) + .find(|candidate| { + std::ptr::eq( + super::image_host_gate(candidate), + super::image_host_gate(&url), + ) + }) + .expect("a different host sharing the bounded gate stripe"); + + let (collision_started_tx, collision_started_rx) = oneshot::channel(); + tokio::spawn(async move { + let collision = + fetch_sanitized_image_using(colliding_url, false, validate, collision_request); + tokio::pin!(collision); + assert!(futures_util::poll!(&mut collision).is_pending()); + collision_started_tx.send(()).ok(); + assert!(collision.await.is_ok()); + }); + collision_started_rx.await.unwrap(); + assert_eq!(*collision_attempts.lock().unwrap(), 1); + assert_eq!(*attempts.lock().unwrap(), 1); + let queued = tokio::spawn(fetch_sanitized_image_using(url, false, validate, request)); tokio::task::yield_now().await; assert!(!queued.is_finished()); @@ -112,6 +164,32 @@ async fn first_rate_limit_and_queued_host_request_share_one_cooldown_boundary() assert_eq!(*attempts.lock().unwrap(), 3); } +#[test] +fn image_cooldown_wait_is_short_and_one_shot() { + let url = Url::parse("https://bounded-cooldown.example/image.png").unwrap(); + let mut waited = false; + assert_eq!( + retryable_image_cooldown(&url, Some(MAX_INLINE_IMAGE_COOLDOWN), &mut waited,), + Some(MAX_INLINE_IMAGE_COOLDOWN) + ); + assert!(waited); + assert_eq!( + retryable_image_cooldown(&url, Some(MAX_INLINE_IMAGE_COOLDOWN), &mut waited,), + None + ); + let excessive_url = Url::parse("https://excessive-cooldown.example/image.png").unwrap(); + let mut excessive_waited = false; + assert_eq!( + retryable_image_cooldown( + &excessive_url, + Some(MAX_INLINE_IMAGE_COOLDOWN + std::time::Duration::from_secs(1)), + &mut excessive_waited, + ), + None + ); + assert!(!excessive_waited); +} + #[test] fn metadata_prefers_open_graph_and_reads_site_name() { let html = r#" From fb26040b8389e5d6c140eff7036897fac732f86e Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Tue, 1 Sep 2026 14:43:21 -0700 Subject: [PATCH 07/11] test(link-preview): protect user-paced metadata fetches Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../src-tauri/src/commands/link_preview.rs | 34 +++++++++++-- .../src/commands/link_preview_tests.rs | 51 +++++++++++++++++-- 2 files changed, 78 insertions(+), 7 deletions(-) diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index d5330501199..8b88f60fc35 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -67,14 +67,14 @@ async fn fetch_link_preview_metadata_for_url( href: String, ) -> Result, String> { let mut url = Url::parse(href.trim()).map_err(|error| format!("invalid URL: {error}"))?; - validate_public_https_url(&url).await?; + validate_metadata_url(&url).await?; if youtube::is_video_url(&url) { return youtube::fetch_oembed_metadata(&url).await; } for redirect_count in 0..=MAX_REDIRECTS { - let response = send_pinned_request(&url, "text/html,application/xhtml+xml;q=0.9").await?; + let response = send_metadata_request(&url, "text/html,application/xhtml+xml;q=0.9").await?; if response.status().is_redirection() { if redirect_count == MAX_REDIRECTS { @@ -89,7 +89,7 @@ async fn fetch_link_preview_metadata_for_url( url = url .join(location) .map_err(|error| format!("invalid link preview redirect: {error}"))?; - validate_public_https_url(&url).await?; + validate_metadata_url(&url).await?; continue; } @@ -152,6 +152,15 @@ fn apply_image_result( } } +async fn validate_metadata_url(url: &Url) -> Result<(), String> { + #[cfg(test)] + if METADATA_TEST_SERVER.try_with(|_| ()).is_ok() { + return Ok(()); + } + + validate_public_https_url(url).await +} + async fn validate_public_https_url(url: &Url) -> Result<(), String> { if url.scheme() != "https" || url.username() != "" || url.password().is_some() { return Err("link previews require an HTTPS URL without credentials".to_string()); @@ -188,6 +197,25 @@ async fn resolve_public_addresses(host: &str) -> Result, String> { Ok(addresses) } +#[cfg(test)] +tokio::task_local! { + static METADATA_TEST_SERVER: std::net::SocketAddr; +} + +async fn send_metadata_request(url: &Url, accept: &str) -> Result { + #[cfg(test)] + if let Ok(address) = METADATA_TEST_SERVER.try_with(|address| *address) { + return reqwest::Client::new() + .get(format!("http://{address}{}", url.path())) + .header(ACCEPT, accept) + .send() + .await + .map_err(|error| format!("link preview test request failed: {error}")); + } + + send_pinned_request(url, accept).await +} + async fn send_pinned_request(url: &Url, accept: &str) -> Result { let host = url .host_str() diff --git a/desktop/src-tauri/src/commands/link_preview_tests.rs b/desktop/src-tauri/src/commands/link_preview_tests.rs index cd870f119f8..510fc2a9424 100644 --- a/desktop/src-tauri/src/commands/link_preview_tests.rs +++ b/desktop/src-tauri/src/commands/link_preview_tests.rs @@ -1,10 +1,10 @@ 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, fetch_sanitized_image_using, is_html_response, - read_bytes_prefix, retry_after_duration, retryable_image_cooldown, sanitize_image, - ImageFetchError, LinkPreviewImageFetchState, LinkPreviewMetadata, MAX_INLINE_IMAGE_COOLDOWN, - MAX_METADATA_DESCRIPTION_CHARS, + extract_link_preview_metadata, fetch_link_preview_metadata, fetch_sanitized_image_using, + is_html_response, read_bytes_prefix, retry_after_duration, retryable_image_cooldown, + sanitize_image, ImageFetchError, LinkPreviewImageFetchState, LinkPreviewMetadata, + MAX_INLINE_IMAGE_COOLDOWN, MAX_METADATA_DESCRIPTION_CHARS, }; use axum::{body::Body, http::Response, routing::get, Router}; use base64::Engine as _; @@ -35,6 +35,49 @@ async fn test_response(router: Router, path: &str) -> reqwest::Response { .unwrap() } +#[tokio::test(start_paused = true)] +async fn metadata_pipeline_remains_pending_beyond_former_aggregate_deadline() { + let (request_started_tx, request_started_rx) = oneshot::channel::<()>(); + let request_started_tx = Arc::new(Mutex::new(Some(request_started_tx))); + let (release_response_tx, release_response_rx) = oneshot::channel::<()>(); + let release_response_rx = Arc::new(Mutex::new(Some(release_response_rx))); + let address = start_test_server(Router::new().route( + "/preview", + get(move || { + let request_started_tx = Arc::clone(&request_started_tx); + let release_response_rx = Arc::clone(&release_response_rx); + async move { + request_started_tx + .lock() + .unwrap() + .take() + .unwrap() + .send(()) + .unwrap(); + let release_response_rx = release_response_rx.lock().unwrap().take().unwrap(); + release_response_rx.await.unwrap(); + Response::builder() + .header("content-type", "text/html") + .body(Body::from("User-paced metadata")) + .unwrap() + } + }), + )) + .await; + let fetch = tokio::spawn(super::METADATA_TEST_SERVER.scope( + address, + fetch_link_preview_metadata("https://user-paced.example/preview".to_string()), + )); + + request_started_rx.await.unwrap(); + tokio::time::advance(std::time::Duration::from_secs(11)).await; + assert!(!fetch.is_finished()); + + release_response_tx.send(()).unwrap(); + let metadata = fetch.await.unwrap().unwrap().unwrap(); + assert_eq!(metadata.title, "User-paced metadata"); +} + #[tokio::test(start_paused = true)] async fn first_rate_limit_and_queued_host_request_share_one_cooldown_boundary() { let cooldown = std::time::Duration::from_secs(20); From 3295e9fdeb7faeb68d4913c8ebb77f75323889e1 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Tue, 1 Sep 2026 22:19:51 -0700 Subject: [PATCH 08/11] fix(link-preview): cancel abandoned metadata fetches Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../src-tauri/src/commands/link_preview.rs | 28 +- .../src/commands/link_preview_cancellation.rs | 89 +++++++ .../src/commands/link_preview_tests.rs | 59 ++++- desktop/src-tauri/src/lib.rs | 2 + .../lib/linkPreviewPreparationStore.test.mjs | 15 ++ .../lib/linkPreviewPreparationStore.ts | 43 +++- .../lib/useResolvedLinkPreviews.test.mjs | 188 ++++++++++++-- .../src/shared/lib/useResolvedLinkPreviews.ts | 243 ++++++++++++++---- 8 files changed, 593 insertions(+), 74 deletions(-) create mode 100644 desktop/src-tauri/src/commands/link_preview_cancellation.rs diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index 8b88f60fc35..5e149aa4b0f 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -10,6 +10,8 @@ use reqwest::{ use serde::Serialize; use url::Url; +#[path = "link_preview_cancellation.rs"] +mod cancellation; #[path = "link_preview_image_retry.rs"] mod image_retry; #[path = "link_preview_rate_limit.rs"] @@ -59,8 +61,32 @@ pub struct LinkPreviewMetadata { #[tauri::command] pub async fn fetch_link_preview_metadata( href: String, + request_id: Option, ) -> Result, String> { - fetch_link_preview_metadata_for_url(href).await + let cancellation = cancellation::begin(request_id.as_deref()); + let result = match cancellation { + Some(cancellation) => { + tokio::select! { + result = fetch_link_preview_metadata_for_url(href) => result, + () = cancellation.cancelled() => Err("link preview request cancelled".to_string()), + } + } + None => fetch_link_preview_metadata_for_url(href).await, + }; + cancellation::finish(request_id.as_deref()); + result +} + +/// Cancel renderer-owned metadata work, including an in-flight response body. +#[tauri::command] +pub fn cancel_link_preview_metadata(request_id: String) { + cancellation::cancel(&request_id); +} + +/// Release a renderer's cancellation record after its invocation settles. +#[tauri::command] +pub fn release_link_preview_metadata(request_id: String) { + cancellation::finish(Some(&request_id)); } async fn fetch_link_preview_metadata_for_url( diff --git a/desktop/src-tauri/src/commands/link_preview_cancellation.rs b/desktop/src-tauri/src/commands/link_preview_cancellation.rs new file mode 100644 index 00000000000..38be2b3a386 --- /dev/null +++ b/desktop/src-tauri/src/commands/link_preview_cancellation.rs @@ -0,0 +1,89 @@ +use std::{ + collections::HashMap, + sync::{LazyLock, Mutex}, +}; + +use tokio_util::sync::CancellationToken; + +#[derive(Default)] +struct LinkPreviewCancellations { + tokens: HashMap, +} + +impl LinkPreviewCancellations { + fn begin(&mut self, request_id: &str) -> CancellationToken { + if let Some(cancellation) = self.tokens.get(request_id).cloned() { + return cancellation; + } + let cancellation = CancellationToken::new(); + self.tokens + .insert(request_id.to_string(), cancellation.clone()); + cancellation + } + + fn cancel(&mut self, request_id: &str) { + self.tokens + .entry(request_id.to_string()) + .or_default() + .cancel(); + } + + fn finish(&mut self, request_id: &str) { + self.tokens.remove(request_id); + } +} + +static LINK_PREVIEW_CANCELLATIONS: LazyLock> = + LazyLock::new(|| Mutex::new(LinkPreviewCancellations::default())); + +pub(super) fn begin(request_id: Option<&str>) -> Option { + let request_id = request_id?; + LINK_PREVIEW_CANCELLATIONS + .lock() + .ok() + .map(|mut fetches| fetches.begin(request_id)) +} + +pub(super) fn cancel(request_id: &str) { + if let Ok(mut fetches) = LINK_PREVIEW_CANCELLATIONS.lock() { + fetches.cancel(request_id); + } +} + +pub(super) fn finish(request_id: Option<&str>) { + let Some(request_id) = request_id else { + return; + }; + if let Ok(mut fetches) = LINK_PREVIEW_CANCELLATIONS.lock() { + fetches.finish(request_id); + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn cancellation_before_begin_is_retained() { + let mut fetches = LinkPreviewCancellations::default(); + fetches.cancel("cancel-before-begin"); + + let cancellation = fetches.begin("cancel-before-begin"); + + assert!(cancellation.is_cancelled()); + fetches.finish("cancel-before-begin"); + assert!(fetches.tokens.is_empty()); + } + + #[test] + fn cancellation_reaches_active_owner() { + let mut fetches = LinkPreviewCancellations::default(); + let cancellation = fetches.begin("active-fetch"); + + fetches.cancel("active-fetch"); + + assert!(cancellation.is_cancelled()); + fetches.finish("active-fetch"); + assert!(fetches.tokens.is_empty()); + } +} diff --git a/desktop/src-tauri/src/commands/link_preview_tests.rs b/desktop/src-tauri/src/commands/link_preview_tests.rs index 510fc2a9424..bce7acda005 100644 --- a/desktop/src-tauri/src/commands/link_preview_tests.rs +++ b/desktop/src-tauri/src/commands/link_preview_tests.rs @@ -1,10 +1,10 @@ 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, fetch_link_preview_metadata, fetch_sanitized_image_using, - is_html_response, read_bytes_prefix, retry_after_duration, retryable_image_cooldown, - sanitize_image, ImageFetchError, LinkPreviewImageFetchState, LinkPreviewMetadata, - MAX_INLINE_IMAGE_COOLDOWN, MAX_METADATA_DESCRIPTION_CHARS, + apply_image_result, cancel_link_preview_metadata, declares_animation, extract_favicon_url, + extract_image_url, extract_link_preview_metadata, fetch_link_preview_metadata, + fetch_sanitized_image_using, is_html_response, read_bytes_prefix, retry_after_duration, + retryable_image_cooldown, sanitize_image, ImageFetchError, LinkPreviewImageFetchState, + LinkPreviewMetadata, MAX_INLINE_IMAGE_COOLDOWN, MAX_METADATA_DESCRIPTION_CHARS, }; use axum::{body::Body, http::Response, routing::get, Router}; use base64::Engine as _; @@ -66,7 +66,7 @@ async fn metadata_pipeline_remains_pending_beyond_former_aggregate_deadline() { .await; let fetch = tokio::spawn(super::METADATA_TEST_SERVER.scope( address, - fetch_link_preview_metadata("https://user-paced.example/preview".to_string()), + fetch_link_preview_metadata("https://user-paced.example/preview".to_string(), None), )); request_started_rx.await.unwrap(); @@ -78,6 +78,53 @@ async fn metadata_pipeline_remains_pending_beyond_former_aggregate_deadline() { assert_eq!(metadata.title, "User-paced metadata"); } +#[tokio::test] +async fn metadata_command_cancellation_drops_an_in_flight_response() { + let (request_started_tx, request_started_rx) = oneshot::channel::<()>(); + let request_started_tx = Arc::new(Mutex::new(Some(request_started_tx))); + let (_release_response_tx, release_response_rx) = oneshot::channel::<()>(); + let release_response_rx = Arc::new(Mutex::new(Some(release_response_rx))); + let address = start_test_server(Router::new().route( + "/preview", + get(move || { + let request_started_tx = Arc::clone(&request_started_tx); + let release_response_rx = Arc::clone(&release_response_rx); + async move { + request_started_tx + .lock() + .unwrap() + .take() + .unwrap() + .send(()) + .unwrap(); + let release_response_rx = release_response_rx.lock().unwrap().take().unwrap(); + let _ = release_response_rx.await; + Response::builder() + .header("content-type", "text/html") + .body(Body::from("Too late")) + .unwrap() + } + }), + )) + .await; + let request_id = "cancel-in-flight".to_string(); + let fetch = tokio::spawn(super::METADATA_TEST_SERVER.scope( + address, + fetch_link_preview_metadata( + "https://cancel.example/preview".to_string(), + Some(request_id.clone()), + ), + )); + + request_started_rx.await.unwrap(); + cancel_link_preview_metadata(request_id); + + assert_eq!( + fetch.await.unwrap(), + Err("link preview request cancelled".to_string()) + ); +} + #[tokio::test(start_paused = true)] async fn first_rate_limit_and_queued_host_request_share_one_cooldown_boundary() { let cooldown = std::time::Duration::from_secs(20); diff --git a/desktop/src-tauri/src/lib.rs b/desktop/src-tauri/src/lib.rs index 12082a2a82e..e3ac45953dc 100644 --- a/desktop/src-tauri/src/lib.rs +++ b/desktop/src-tauri/src/lib.rs @@ -600,6 +600,8 @@ pub fn run() { get_relay_http_url, get_media_proxy_port, fetch_link_preview_metadata, + cancel_link_preview_metadata, + release_link_preview_metadata, discover_acp_auth_methods, discover_acp_providers, discover_git_bash_prerequisite, diff --git a/desktop/src/features/messages/lib/linkPreviewPreparationStore.test.mjs b/desktop/src/features/messages/lib/linkPreviewPreparationStore.test.mjs index 293fb63e154..7f67df1f49c 100644 --- a/desktop/src/features/messages/lib/linkPreviewPreparationStore.test.mjs +++ b/desktop/src/features/messages/lib/linkPreviewPreparationStore.test.mjs @@ -272,3 +272,18 @@ test("reset cancels pending preparations instead of authorizing send", async () assert.deepEqual(await preparation.promise, { status: "cancelled" }); }); + +test("Skip aborts an abandoned in-flight preview job", async () => { + const pending = deferred(); + seed(first, pending.promise); + const job = __linkPreviewPreparationTest.jobs.get(first.href); + + const preparation = prepareBackgroundLinkPreviews([first], 1_000); + assert.ok(preparation); + preparation.skip(); + + assert.deepEqual(await preparation.promise, { status: "ready", tags: [] }); + assert.equal(job.controller.signal.aborted, true); + assert.equal(__linkPreviewPreparationTest.jobs.has(first.href), false); + pending.resolve(null); +}); diff --git a/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts b/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts index d61750fcd86..0c7df704b6c 100644 --- a/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts +++ b/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts @@ -9,6 +9,7 @@ import { } from "@/shared/lib/linkPreviewSnapshot"; import { loadLinkPreviewMetadata, + type LinkPreviewMetadata, resolveLinkPreview, } from "@/shared/lib/useResolvedLinkPreviews"; @@ -27,6 +28,7 @@ type PreviewJob = { type BackgroundPreviewTask = { cancel: () => void; + hrefs: Set; id: number; skip: () => void; }; @@ -113,7 +115,18 @@ async function buildSnapshot( signal: AbortSignal, onMetadataReady: (tag: string[]) => void, ): Promise { - const metadata = await loadLinkPreviewMetadata(candidate.href); + const metadataLoad = loadLinkPreviewMetadata(candidate.href); + const cancelMetadata = () => metadataLoad.cancel(); + signal.addEventListener("abort", cancelMetadata, { once: true }); + let metadata: LinkPreviewMetadata | null; + try { + metadata = await metadataLoad.promise; + } catch { + return null; + } finally { + signal.removeEventListener("abort", cancelMetadata); + metadataLoad.cancel(); + } if (signal.aborted || !metadata) return null; const preview = resolveLinkPreview(candidate, metadata); if (!preview.snapshotReady) return null; @@ -288,24 +301,42 @@ export function prepareBackgroundLinkPreviews( let finish: ((result: BackgroundLinkPreviewResult) => void) | null = null; let terminal = false; let timer: ReturnType | null = null; - const complete = (result: BackgroundLinkPreviewResult) => { + const complete = ( + result: BackgroundLinkPreviewResult, + abortUnobservedJobs = false, + ) => { if (terminal) return; terminal = true; if (timer !== null) clearTimeout(timer); tasks.delete(taskId); + if (abortUnobservedJobs) { + for (const candidate of external) { + const observedByAnotherSend = [...tasks.values()].some((task) => + task.hrefs.has(candidate.href), + ); + if (!observedByAnotherSend) { + invalidateLinkPreviewPreparation(candidate.href); + } + } + } publishSnapshot(); finish?.(result); }; const promise = new Promise((resolve) => { finish = resolve; }); - const cancel = () => complete({ status: "cancelled" }); - const skip = () => complete({ status: "ready", tags: [] }); - tasks.set(taskId, { cancel, id: taskId, skip }); + const cancel = () => complete({ status: "cancelled" }, true); + const skip = () => complete({ status: "ready", tags: [] }, true); + tasks.set(taskId, { + cancel, + hrefs: new Set(external.map((candidate) => candidate.href)), + id: taskId, + skip, + }); publishSnapshot(); timer = setTimeout( - () => complete({ status: "ready", tags: availableTags() }), + () => complete({ status: "ready", tags: availableTags() }, true), timeoutMs, ); void Promise.all(external.map(prepareLinkPreview)).then((tags) => { diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs index e413f9c7c20..c69a45144e6 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs @@ -140,14 +140,14 @@ test("metadata loader retries transient images after the server cooldown", async }); assert.equal( - (await loader.load(preview.href)).metadata?.imageFetchState, + (await loader.load(preview.href).promise).metadata?.imageFetchState, "transient_failure", ); assert.equal(calls, 1); now += 10_000; assert.equal( - (await loader.load(preview.href)).metadata?.imageFetchState, + (await loader.load(preview.href).promise).metadata?.imageFetchState, "image", ); assert.equal(calls, 2); @@ -165,12 +165,15 @@ test("metadata loader retries rejected requests after the negative-cache TTL", a now: () => now, }); - assert.equal((await loader.load(preview.href)).metadata, null); - assert.equal((await loader.load(preview.href)).metadata, null); + assert.equal((await loader.load(preview.href).promise).metadata, null); + assert.equal((await loader.load(preview.href).promise).metadata, null); assert.equal(calls, 1); now += 5 * 60_000; - assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.deepEqual( + (await loader.load(preview.href).promise).metadata, + metadata(), + ); assert.equal(calls, 2); }); @@ -191,10 +194,10 @@ test("metadata loader coalesces fragment variants and bounds concurrency", async }); await Promise.all([ - loader.load("https://example.com/one#first"), - loader.load("https://example.com/one#second"), - loader.load("https://example.com/two"), - loader.load("https://example.com/three"), + loader.load("https://example.com/one#first").promise, + loader.load("https://example.com/one#second").promise, + loader.load("https://example.com/two").promise, + loader.load("https://example.com/three").promise, ]); assert.equal(calls, 3); @@ -504,12 +507,15 @@ test("invalidateNegative drops a cached null miss so the next load refetches", a now: () => now, }); - assert.equal((await loader.load(preview.href)).metadata, null); + assert.equal((await loader.load(preview.href).promise).metadata, null); assert.equal(calls, 1); loader.invalidateNegative(preview.href); nextResult = metadata(); - assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.deepEqual( + (await loader.load(preview.href).promise).metadata, + metadata(), + ); assert.equal(calls, 2); }); @@ -538,7 +544,7 @@ test("invalidateNegative drops a cached transient failure so the next load refet }); assert.equal( - (await loader.load(preview.href)).metadata?.imageFetchState, + (await loader.load(preview.href).promise).metadata?.imageFetchState, "transient_failure", ); assert.equal(calls, 1); @@ -547,7 +553,7 @@ test("invalidateNegative drops a cached transient failure so the next load refet // the cooldown — is what forces the refetch. loader.invalidateNegative(preview.href); assert.equal( - (await loader.load(preview.href)).metadata?.imageFetchState, + (await loader.load(preview.href).promise).metadata?.imageFetchState, "image", ); assert.equal(calls, 2); @@ -566,11 +572,17 @@ test("invalidateNegative leaves a healthy cached hit untouched", async () => { now: () => now, }); - assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.deepEqual( + (await loader.load(preview.href).promise).metadata, + metadata(), + ); assert.equal(calls, 1); loader.invalidateNegative(preview.href); - assert.deepEqual((await loader.load(preview.href)).metadata, metadata()); + assert.deepEqual( + (await loader.load(preview.href).promise).metadata, + metadata(), + ); assert.equal(calls, 1); }); @@ -599,10 +611,74 @@ test("invalidateNegative leaves an in-flight fetch untouched", async () => { assert.equal(calls, 1, "no redundant fetch started by the bust"); releaseFetch(); - assert.deepEqual((await pending).metadata, metadata()); + assert.deepEqual((await pending.promise).metadata, metadata()); assert.equal(calls, 1, "the original in-flight fetch resolved, not a retry"); }); +test("an orphaned metadata load is cancelled after its re-entry grace period", async () => { + let cancelled = false; + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + fetcher: (_href, signal) => + new Promise((_resolve, reject) => { + signal.addEventListener( + "abort", + () => { + cancelled = true; + reject(new Error("cancelled")); + }, + { once: true }, + ); + }), + }); + + const load = loader.load(preview.href); + const rejection = assert.rejects(load.promise, /cancelled/); + load.cancel(); + await new Promise((resolve) => setTimeout(resolve, 1_050)); + assert.equal(cancelled, true); + await rejection; +}); + +test("new metadata demand immediately reclaims slots from orphaned loads", async () => { + const started = []; + const cancelled = []; + const releases = new Map(); + const loader = __linkPreviewMetadataTest.createMetadataLoader({ + concurrency: 2, + fetcher: (href, signal) => + new Promise((resolve, reject) => { + started.push(href); + releases.set(href, () => resolve(metadata())); + signal.addEventListener( + "abort", + () => { + cancelled.push(href); + reject(new Error("cancelled")); + }, + { once: true }, + ); + }), + }); + + const firstHref = "https://example.com/stalled-one"; + const secondHref = "https://example.com/stalled-two"; + const thirdHref = "https://example.com/ordinary"; + const first = loader.load(firstHref); + const second = loader.load(secondHref); + const firstSettled = first.promise.catch(() => undefined); + const secondSettled = second.promise.catch(() => undefined); + first.cancel(); + second.cancel(); + + const third = loader.load(thirdHref); + await new Promise((resolve) => setImmediate(resolve)); + assert.deepEqual(started, [firstHref, secondHref, thirdHref]); + assert.deepEqual(new Set(cancelled), new Set([firstHref, secondHref])); + releases.get(thirdHref)(); + assert.deepEqual((await third.promise).metadata, metadata()); + await Promise.all([firstSettled, secondSettled]); +}); + // ── Hook-level regression: retained resolved-state invalidation on re-entry ─── // // The loader tests above prove `invalidateNegative` drops the SHARED loader @@ -899,3 +975,83 @@ test("hook clears a re-entered negative even when the shared cache holds an in-f ipcHandlers.clear(); } }); + +test("removing two stalled previews cancels native work and admits a third URL", async () => { + const { act, cleanup, renderHook } = await import("@testing-library/react"); + const { useResolvedLinkPreviews } = await import( + "./useResolvedLinkPreviews.ts" + ); + + resetLinkPreviewMetadataCache(); + ipcHandlers.clear(); + + const firstPreview = { + ...hookPreview, + href: "https://example.com/stalled-one", + }; + const secondPreview = { + ...hookPreview, + href: "https://example.com/stalled-two", + }; + const thirdPreview = { ...hookPreview, href: "https://example.com/ordinary" }; + const pending = new Map(); + const started = []; + const cancelled = []; + + ipcHandlers.set("fetch_link_preview_metadata", ({ href, requestId }) => { + started.push(href); + return new Promise((resolve, reject) => { + pending.set(requestId, { href, reject, resolve }); + }); + }); + ipcHandlers.set("cancel_link_preview_metadata", ({ requestId }) => { + const request = pending.get(requestId); + if (!request) return Promise.resolve(); + cancelled.push(request.href); + pending.delete(requestId); + request.reject(new Error("cancelled by test native seam")); + return Promise.resolve(); + }); + ipcHandlers.set("release_link_preview_metadata", () => Promise.resolve()); + + const flushScheduledLoads = async () => { + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + }; + + const hook = renderHook(({ previews }) => useResolvedLinkPreviews(previews), { + initialProps: { previews: [firstPreview, secondPreview] }, + }); + + try { + await flushScheduledLoads(); + assert.deepEqual(started, [firstPreview.href, secondPreview.href]); + + hook.rerender({ previews: [thirdPreview] }); + await flushScheduledLoads(); + assert.deepEqual( + new Set(cancelled), + new Set([firstPreview.href, secondPreview.href]), + ); + assert.deepEqual(started, [ + firstPreview.href, + secondPreview.href, + thirdPreview.href, + ]); + + const third = [...pending.values()].find( + (request) => request.href === thirdPreview.href, + ); + assert.ok(third, "third URL reached the native fetch seam"); + await act(async () => { + third.resolve(metadata()); + await new Promise((resolve) => setTimeout(resolve, 0)); + }); + assert.equal(hook.result.current[0].title, "A story"); + } finally { + hook.unmount(); + cleanup(); + ipcHandlers.clear(); + } +}); diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.ts b/desktop/src/shared/lib/useResolvedLinkPreviews.ts index b02aa26650c..d4fd6b67a44 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.ts +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.ts @@ -51,6 +51,10 @@ type MetadataLoadResult = MetadataCacheEntry & { const DEFAULT_TRANSIENT_RETRY_MS = 30_000; const NULL_METADATA_RETRY_MS = 5 * 60_000; const MAX_CONCURRENT_METADATA_FETCHES = 2; +// Keep a just-removed request cacheable through a brief edit/re-entry gap, but +// never let unobserved native work linger indefinitely. New queued demand +// reclaims these loads immediately rather than waiting for this grace period. +const ORPHANED_METADATA_LOAD_GRACE_MS = 1_000; /** * React may flush an interaction-triggered effect before the browser paints. @@ -107,29 +111,70 @@ function metadataExpiry( return now + retryAfterMs; } +type PendingMetadataLoad = { + controller: AbortController; + consumers: number; + orphanTimer: ReturnType | null; + promise: Promise; +}; + +type MetadataCacheValue = MetadataCacheEntry | PendingMetadataLoad; + +function isPendingMetadataLoad( + value: MetadataCacheValue, +): value is PendingMetadataLoad { + return "promise" in value; +} + +function abortError(): DOMException { + return new DOMException("Link preview fetch cancelled", "AbortError"); +} + function createTaskScheduler(concurrency: number) { - const pending: Array<() => void> = []; + const pending: Array<{ + reject: (reason?: unknown) => void; + run: () => void; + signal: AbortSignal; + }> = []; let active = 0; const drain = () => { while (active < concurrency) { - const run = pending.shift(); - if (!run) return; + const next = pending.shift(); + if (!next) return; + if (next.signal.aborted) { + next.reject(abortError()); + continue; + } active += 1; - run(); + next.run(); } }; - return (task: () => Promise): Promise => + return (task: () => Promise, signal: AbortSignal): Promise => new Promise((resolve, reject) => { - pending.push(() => { - void task() - .then(resolve, reject) - .finally(() => { - active -= 1; - drain(); - }); - }); + const entry = { + reject, + signal, + run: () => { + signal.removeEventListener("abort", cancelPending); + void task() + .then(resolve, reject) + .finally(() => { + active -= 1; + drain(); + }); + }, + }; + const cancelPending = () => { + const index = pending.indexOf(entry); + if (index < 0) return; + pending.splice(index, 1); + reject(abortError()); + drain(); + }; + signal.addEventListener("abort", cancelPending, { once: true }); + pending.push(entry); drain(); }); } @@ -140,20 +185,20 @@ function createMetadataLoader({ now = Date.now, }: { concurrency?: number; - fetcher: (href: string) => Promise; + fetcher: ( + href: string, + signal: AbortSignal, + ) => Promise; now?: () => number; }) { - const cache = new Map< - string, - MetadataCacheEntry | Promise - >(); + const cache = new Map(); const schedule = createTaskScheduler(Math.max(1, concurrency)); let generation = 0; const peek = (href: string): MetadataLoadResult | undefined => { const key = metadataCacheKey(href); const cached = cache.get(key); - if (!cached || cached instanceof Promise) return undefined; + if (!cached || isPendingMetadataLoad(cached)) return undefined; if (cached.expiresAt !== null && cached.expiresAt <= now()) { cache.delete(key); return undefined; @@ -161,32 +206,107 @@ function createMetadataLoader({ return { key, ...cached }; }; - const load = (href: string): Promise => { + const abortPending = (key: string, pending: PendingMetadataLoad) => { + if (pending.orphanTimer !== null) clearTimeout(pending.orphanTimer); + pending.orphanTimer = null; + if (cache.get(key) === pending) cache.delete(key); + pending.controller.abort(); + }; + + const release = (key: string, pending: PendingMetadataLoad) => { + if (pending.consumers <= 0) return; + pending.consumers -= 1; + if ( + pending.consumers > 0 || + pending.orphanTimer !== null || + cache.get(key) !== pending + ) { + return; + } + pending.orphanTimer = setTimeout( + () => abortPending(key, pending), + ORPHANED_METADATA_LOAD_GRACE_MS, + ); + }; + + const abortOrphanedLoads = (exceptKey: string) => { + for (const [key, cached] of cache) { + if ( + key !== exceptKey && + isPendingMetadataLoad(cached) && + cached.consumers === 0 + ) { + abortPending(key, cached); + } + } + }; + + const load = ( + href: string, + ): { cancel: () => void; promise: Promise } => { const key = metadataCacheKey(href); const cached = cache.get(key); - if (cached instanceof Promise) return cached; + if (cached && isPendingMetadataLoad(cached)) { + if (cached.orphanTimer !== null) clearTimeout(cached.orphanTimer); + cached.orphanTimer = null; + cached.consumers += 1; + return { + cancel: () => release(key, cached), + promise: cached.promise, + }; + } if (cached) { if (cached.expiresAt === null || cached.expiresAt > now()) { - return Promise.resolve({ key, ...cached }); + return { + cancel: () => undefined, + promise: Promise.resolve({ key, ...cached }), + }; } cache.delete(key); } + abortOrphanedLoads(key); const requestGeneration = generation; - const promise = schedule(() => fetcher(href)) - .catch(() => null) + const controller = new AbortController(); + const pending: PendingMetadataLoad = { + controller, + consumers: 1, + orphanTimer: null, + promise: Promise.resolve({ + expiresAt: null, + key, + metadata: null, + }), + }; + pending.promise = schedule( + () => fetcher(href, controller.signal), + controller.signal, + ) + .catch((error) => { + if (controller.signal.aborted) throw error; + return null; + }) .then((metadata) => { + if (pending.orphanTimer !== null) clearTimeout(pending.orphanTimer); + pending.orphanTimer = null; const entry = { expiresAt: metadataExpiry(metadata, now()), metadata, }; - if (requestGeneration === generation) { + if ( + requestGeneration === generation && + cache.get(key) === pending && + !controller.signal.aborted + ) { cache.set(key, entry); } return { key, ...entry }; }); - cache.set(key, promise); - return promise; + cache.set(key, pending); + return { + cancel: () => release(key, pending), + promise: pending.promise, + }; }; return { @@ -205,7 +325,7 @@ function createMetadataLoader({ invalidateNegative(href: string): boolean { const key = metadataCacheKey(href); const cached = cache.get(key); - if (!cached || cached instanceof Promise) return false; + if (!cached || isPendingMetadataLoad(cached)) return false; if (!isNegativeMetadata(cached.metadata)) return false; cache.delete(key); return true; @@ -214,6 +334,9 @@ function createMetadataLoader({ peek, reset() { generation += 1; + for (const [key, cached] of cache) { + if (isPendingMetadataLoad(cached)) abortPending(key, cached); + } cache.clear(); }, }; @@ -221,7 +344,9 @@ function createMetadataLoader({ function fetchLinkPreviewMetadata( href: string, + signal: AbortSignal, ): Promise { + const requestId = crypto.randomUUID(); const startedAt = performance.now(); const logResult = ( result: LinkPreviewMetadata | null, @@ -250,12 +375,26 @@ function fetchLinkPreviewMetadata( throw error; }; - return invokeTauri( + const request = invokeTauri( "fetch_link_preview_metadata", { href, + requestId, }, - ).then(logResult, logFailure); + ); + const onAbort = () => { + void invokeTauri("cancel_link_preview_metadata", { requestId }).catch( + () => undefined, + ); + }; + signal.addEventListener("abort", onAbort, { once: true }); + if (signal.aborted) onAbort(); + return request.then(logResult, logFailure).finally(() => { + signal.removeEventListener("abort", onAbort); + void invokeTauri("release_link_preview_metadata", { requestId }).catch( + () => undefined, + ); + }); } const metadataLoader = createMetadataLoader({ @@ -263,10 +402,15 @@ const metadataLoader = createMetadataLoader({ }); /** Share the same deduplicated metadata job between composer rendering and send preparation. */ -export async function loadLinkPreviewMetadata( - href: string, -): Promise { - return (await metadataLoader.load(href)).metadata; +export function loadLinkPreviewMetadata(href: string): { + cancel: () => void; + promise: Promise; +} { + const load = metadataLoader.load(href); + return { + cancel: load.cancel, + promise: load.promise.then((result) => result.metadata), + }; } type EntityEventFetcher = ( filter: Parameters[0], @@ -376,14 +520,19 @@ export async function fetchBuzzEntityMetadata( } const entityMetadataLoader = createMetadataLoader({ - fetcher: fetchBuzzEntityMetadata, + fetcher: (href) => fetchBuzzEntityMetadata(href), }); /** Share deduplicated relay-native entity metadata across cards and inline tooltips. */ export async function loadBuzzEntityMetadata( href: string, ): Promise { - return (await entityMetadataLoader.load(href)).metadata; + const load = entityMetadataLoader.load(href); + try { + return (await load.promise).metadata; + } finally { + load.cancel(); + } } /** Clear ephemeral metadata when the active relay/community changes. */ @@ -650,15 +799,19 @@ export function useResolvedLinkPreviews( cancelScheduledLoads.push( scheduleAfterPaint(() => { - void loader.load(preview.href).then((result) => { - if (cancelled) return; - setResolvedMetadata((current) => - current[result.key] === result.metadata - ? current - : { ...current, [result.key]: result.metadata }, - ); - scheduleRetry(result, loader); - }); + const load = loader.load(preview.href); + cancelScheduledLoads.push(load.cancel); + void load.promise + .then((result) => { + if (cancelled) return; + setResolvedMetadata((current) => + current[result.key] === result.metadata + ? current + : { ...current, [result.key]: result.metadata }, + ); + scheduleRetry(result, loader); + }) + .catch(() => undefined); }), ); } From b6e04382dc22cb6be14d881ab92a0167b82ec30c Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Wed, 2 Sep 2026 08:54:17 -0700 Subject: [PATCH 09/11] fix(link-preview): make metadata leases idempotent Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../lib/linkPreviewPreparationStore.ts | 12 ++- .../lib/useResolvedLinkPreviews.test.mjs | 74 +++++++++++++++++++ .../src/shared/lib/useResolvedLinkPreviews.ts | 13 +++- 3 files changed, 95 insertions(+), 4 deletions(-) diff --git a/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts b/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts index 0c7df704b6c..4ee86f36b80 100644 --- a/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts +++ b/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts @@ -116,11 +116,19 @@ async function buildSnapshot( onMetadataReady: (tag: string[]) => void, ): Promise { const metadataLoad = loadLinkPreviewMetadata(candidate.href); - const cancelMetadata = () => metadataLoad.cancel(); + let settleAbortedMetadata: (() => void) | null = null; + const abortedMetadata = new Promise((resolve) => { + settleAbortedMetadata = () => resolve(null); + }); + const cancelMetadata = () => { + metadataLoad.cancel(); + settleAbortedMetadata?.(); + }; signal.addEventListener("abort", cancelMetadata, { once: true }); + if (signal.aborted) cancelMetadata(); let metadata: LinkPreviewMetadata | null; try { - metadata = await metadataLoad.promise; + metadata = await Promise.race([metadataLoad.promise, abortedMetadata]); } catch { return null; } finally { diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs index c69a45144e6..26811489037 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.test.mjs @@ -6,6 +6,7 @@ import { JSDOM } from "jsdom"; import { __linkPreviewMetadataTest, fetchBuzzEntityMetadata, + loadLinkPreviewMetadata, isBuzzEntityPreview, resetLinkPreviewMetadataCache, resolveLinkPreview, @@ -33,6 +34,16 @@ function metadata(overrides = {}) { }; } +function deferred() { + let reject; + let resolve; + const promise = new Promise((resolvePromise, rejectPromise) => { + reject = rejectPromise; + resolve = resolvePromise; + }); + return { promise, reject, resolve }; +} + test("pending external metadata reserves the image treatment", () => { assert.deepEqual(resolveLinkPreview(preview, undefined), { ...preview, @@ -976,6 +987,69 @@ test("hook clears a re-entered negative even when the shared cache holds an in-f } }); +test("preparation abort releases only its own shared metadata lease", async () => { + const { prepareLinkPreview, resetLinkPreviewPreparations } = await import( + "../../features/messages/lib/linkPreviewPreparationStore.ts" + ); + + resetLinkPreviewMetadataCache(); + resetLinkPreviewPreparations(); + ipcHandlers.clear(); + + const href = "https://example.com/shared-pending"; + let fetchRequest; + const cancelledRequestIds = []; + ipcHandlers.set("fetch_link_preview_metadata", (args) => { + const request = deferred(); + fetchRequest = { ...args, ...request }; + return request.promise; + }); + ipcHandlers.set("cancel_link_preview_metadata", ({ requestId }) => { + cancelledRequestIds.push(requestId); + return Promise.resolve(); + }); + ipcHandlers.set("release_link_preview_metadata", () => Promise.resolve()); + + try { + const composerLoad = loadLinkPreviewMetadata(href); + const preparation = prepareLinkPreview({ ...hookPreview, href }); + await new Promise((resolve) => setImmediate(resolve)); + assert.ok(fetchRequest, "shared metadata reached the native fetch seam"); + + resetLinkPreviewPreparations(); + assert.equal(await preparation, null); + await new Promise((resolve) => setTimeout(resolve, 1_050)); + assert.deepEqual( + cancelledRequestIds, + [], + "preparation listener plus finally must not release the composer lease", + ); + + fetchRequest.resolve(metadata()); + assert.deepEqual(await composerLoad.promise, metadata()); + + const finalHref = `${href}/final`; + const finalLoad = loadLinkPreviewMetadata(finalHref); + await new Promise((resolve) => setImmediate(resolve)); + const finalRequest = fetchRequest; + const finalRejection = assert.rejects(finalLoad.promise); + finalLoad.cancel(); + finalLoad.cancel(); + await new Promise((resolve) => setTimeout(resolve, 1_050)); + assert.deepEqual( + cancelledRequestIds, + [finalRequest.requestId], + "one final consumer release permits native abort", + ); + finalRequest.reject(new Error("cancelled by test native seam")); + await finalRejection; + } finally { + resetLinkPreviewPreparations(); + resetLinkPreviewMetadataCache(); + ipcHandlers.clear(); + } +}); + test("removing two stalled previews cancels native work and admits a third URL", async () => { const { act, cleanup, renderHook } = await import("@testing-library/react"); const { useResolvedLinkPreviews } = await import( diff --git a/desktop/src/shared/lib/useResolvedLinkPreviews.ts b/desktop/src/shared/lib/useResolvedLinkPreviews.ts index d4fd6b67a44..5111929b3e3 100644 --- a/desktop/src/shared/lib/useResolvedLinkPreviews.ts +++ b/desktop/src/shared/lib/useResolvedLinkPreviews.ts @@ -229,6 +229,15 @@ function createMetadataLoader({ ); }; + const createRelease = (key: string, pending: PendingMetadataLoad) => { + let released = false; + return () => { + if (released) return; + released = true; + release(key, pending); + }; + }; + const abortOrphanedLoads = (exceptKey: string) => { for (const [key, cached] of cache) { if ( @@ -251,7 +260,7 @@ function createMetadataLoader({ cached.orphanTimer = null; cached.consumers += 1; return { - cancel: () => release(key, cached), + cancel: createRelease(key, cached), promise: cached.promise, }; } @@ -304,7 +313,7 @@ function createMetadataLoader({ }); cache.set(key, pending); return { - cancel: () => release(key, pending), + cancel: createRelease(key, pending), promise: pending.promise, }; }; From d8c06f6143878d9416db2a53b8345f67d129e0c0 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Tue, 8 Sep 2026 11:00:41 -0700 Subject: [PATCH 10/11] fix(link-preview): bound expired preview promotion Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../lib/linkPreviewPreparationStore.test.mjs | 82 ++++++++++++++++++- .../lib/linkPreviewPreparationStore.ts | 7 +- 2 files changed, 86 insertions(+), 3 deletions(-) diff --git a/desktop/src/features/messages/lib/linkPreviewPreparationStore.test.mjs b/desktop/src/features/messages/lib/linkPreviewPreparationStore.test.mjs index 7f67df1f49c..7153ff38518 100644 --- a/desktop/src/features/messages/lib/linkPreviewPreparationStore.test.mjs +++ b/desktop/src/features/messages/lib/linkPreviewPreparationStore.test.mjs @@ -1,5 +1,7 @@ import assert from "node:assert/strict"; -import test from "node:test"; +import { after, before, test } from "node:test"; + +import { JSDOM } from "jsdom"; import { __linkPreviewPreparationTest, @@ -12,6 +14,29 @@ import { const first = { href: "https://example.com/first" }; const second = { href: "https://example.com/second" }; const firstTag = ["link-preview", "snapshot", first.href]; +const dom = new JSDOM("", { + url: "http://localhost", +}); +const ipcHandlers = new Map(); + +before(() => { + Object.assign(globalThis, { + document: dom.window.document, + HTMLElement: dom.window.HTMLElement, + window: dom.window, + }); + dom.window.__TAURI_INTERNALS__ = { + invoke: (cmd, args) => { + const handler = ipcHandlers.get(cmd); + return handler + ? handler(args) + : Promise.reject(new Error(`unmocked Tauri command: ${cmd}`)); + }, + transformCallback: () => Math.random(), + }; +}); + +after(() => dom.window.close()); function deferred() { let resolve; @@ -41,6 +66,7 @@ function seed( test.afterEach(() => { __linkPreviewPreparationTest.reset(); + ipcHandlers.clear(); }); test("adopts one in-flight job for the same canonical URL", () => { @@ -223,6 +249,60 @@ test("Skip after completion cannot replace finalized tags", async () => { }); }); +test("expired settled work replaced by a pending retry keeps deadline and Skip", async () => { + const expiredTag = ["link-preview", "snapshot", first.href, "expired"]; + const pendingRetry = deferred(); + let fetchCalls = 0; + ipcHandlers.set("fetch_link_preview_metadata", () => { + fetchCalls += 1; + return pendingRetry.promise; + }); + ipcHandlers.set("cancel_link_preview_metadata", () => Promise.resolve()); + ipcHandlers.set("release_link_preview_metadata", () => Promise.resolve()); + + const seedExpiredFallback = () => + seed( + first, + Promise.resolve(expiredTag), + true, + Date.now() - 5 * 60_000, + expiredTag, + expiredTag, + ); + + seedExpiredFallback(); + const timeoutPreparation = prepareBackgroundLinkPreviews([first], 0); + assert.ok(timeoutPreparation); + assert.equal( + __linkPreviewPreparationTest.jobs.get(first.href)?.settled, + false, + ); + assert.equal( + fetchCalls, + 1, + "the expired job was replaced through production I/O", + ); + assert.deepEqual(await timeoutPreparation.promise, { + status: "ready", + tags: [], + }); + + seedExpiredFallback(); + const skipPreparation = prepareBackgroundLinkPreviews([first], 1_000); + assert.ok(skipPreparation); + assert.equal( + __linkPreviewPreparationTest.jobs.get(first.href)?.settled, + false, + ); + skipPreparation.skip(); + assert.deepEqual(await skipPreparation.promise, { + status: "ready", + tags: [], + }); + + pendingRetry.resolve(null); +}); + test("already-settled partial results contain only successful tags", async () => { seed(first, Promise.resolve(firstTag), true); seed(second, Promise.resolve(null), true); diff --git a/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts b/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts index 4ee86f36b80..a4f1273ecf2 100644 --- a/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts +++ b/desktop/src/features/messages/lib/linkPreviewPreparationStore.ts @@ -286,12 +286,15 @@ export function prepareBackgroundLinkPreviews( skip, }); + const preparations = external.map((candidate) => + prepareLinkPreview(candidate), + ); const pending = external.some( (candidate) => !jobs.get(candidate.href)?.settled, ); if (!pending) { return preparedSend( - Promise.all(external.map(prepareLinkPreview)).then((tags) => ({ + Promise.all(preparations).then((tags) => ({ status: "ready" as const, tags: tags.filter((tag): tag is string[] => tag !== null), })), @@ -347,7 +350,7 @@ export function prepareBackgroundLinkPreviews( () => complete({ status: "ready", tags: availableTags() }, true), timeoutMs, ); - void Promise.all(external.map(prepareLinkPreview)).then((tags) => { + void Promise.all(preparations).then((tags) => { complete({ status: "ready", tags: tags.filter((tag): tag is string[] => tag !== null), From 274a2a3ad43a3e9811cafb078ba26f55cdc5c1e7 Mon Sep 17 00:00:00 2001 From: Taylor Ho Date: Tue, 8 Sep 2026 11:00:48 -0700 Subject: [PATCH 11/11] fix(link-preview): keep cooldown waits one-shot Co-authored-by: Carl Signed-off-by: Taylor Ho --- .../src-tauri/src/commands/link_preview.rs | 2 +- .../src/commands/link_preview_tests.rs | 38 +++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/desktop/src-tauri/src/commands/link_preview.rs b/desktop/src-tauri/src/commands/link_preview.rs index 5e149aa4b0f..a746cc80598 100644 --- a/desktop/src-tauri/src/commands/link_preview.rs +++ b/desktop/src-tauri/src/commands/link_preview.rs @@ -467,7 +467,7 @@ where .await .map_err(|_| ImageFetchError::Transient { retry_after: None, - retry_inline: true, + retry_inline: !waited_for_cooldown, })?; if response.status().is_redirection() { if redirect_count == MAX_REDIRECTS { diff --git a/desktop/src-tauri/src/commands/link_preview_tests.rs b/desktop/src-tauri/src/commands/link_preview_tests.rs index bce7acda005..05ab0eec2ce 100644 --- a/desktop/src-tauri/src/commands/link_preview_tests.rs +++ b/desktop/src-tauri/src/commands/link_preview_tests.rs @@ -254,6 +254,44 @@ async fn first_rate_limit_and_queued_host_request_share_one_cooldown_boundary() assert_eq!(*attempts.lock().unwrap(), 3); } +#[tokio::test(start_paused = true)] +async fn transport_failure_after_cooldown_does_not_renew_wait_on_outer_retry() { + let cooldown = std::time::Duration::from_secs(20); + let url = Url::parse("https://transport-after-cooldown.example/image.png").unwrap(); + super::set_image_host_cooldown(&url, cooldown); + let attempts = Arc::new(Mutex::new(0)); + + let result = super::image_retry::retry_transient_image_fetch(|| { + let url = url.clone(); + let attempts = Arc::clone(&attempts); + async move { + fetch_sanitized_image_using( + url, + false, + |_url| async { Ok(()) }, + move |_url, _accept| { + let attempts = Arc::clone(&attempts); + async move { + *attempts.lock().unwrap() += 1; + Err("connection failed".to_string()) + } + }, + ) + .await + } + }) + .await; + + assert_eq!( + result, + Err(ImageFetchError::Transient { + retry_after: None, + retry_inline: false, + }) + ); + assert_eq!(*attempts.lock().unwrap(), 1); +} + #[test] fn image_cooldown_wait_is_short_and_one_shot() { let url = Url::parse("https://bounded-cooldown.example/image.png").unwrap();