From d35b8054b484c83abcbaad4143cc96346a42dba4 Mon Sep 17 00:00:00 2001 From: rays Date: Fri, 23 Oct 2026 14:18:32 +0000 Subject: [PATCH] Bound the scan work one feed's author can ask for (#339 #337) Both run on the daemon's single worker, so every other feed's check and download waited behind them. #339: prefer_https asked each new artwork host whether it served the image on https, up to 20 s each, one after another, and the per-scan cache is keyed by the authority, which ports and user names multiply for one server. It now asks at most eight hosts a read; past that an http image stays http, for /api/art to fetch, as one that fails does. #337: a list (OPML or Patreon) may bring in 2,000 feeds; past that sync_group refuses it whole, so the feed shows an error and nothing changes. davewiner's, the largest seen, had 922. sync_group also read the whole feeds table and rebuilt an id map with a Feed cloned per entry for every new feed; it now reads the table once and checks ids against sets, and unique_slug takes a predicate rather than a map. same_feed skips parsing two URLs unless both are Patreon's, since only those can differ and match. sync_group and sync_opml lose the parent argument, which nothing used once promote_kept stopped taking it (#332). Co-Authored-By: Claude Opus 5.5 --- crates/feed/src/lib.rs | 23 +++++++++++++- crates/ipx/CHANGELOG.md | 2 ++ crates/ipx/src/config.rs | 8 ++--- crates/ipx/src/main.rs | 67 ++++++++++++++++++++++++++-------------- 4 files changed, 71 insertions(+), 29 deletions(-) diff --git a/crates/feed/src/lib.rs b/crates/feed/src/lib.rs index 1b6e80c..f1fc771 100644 --- a/crates/feed/src/lib.rs +++ b/crates/feed/src/lib.rs @@ -452,7 +452,11 @@ pub fn expand_input(input: &str) -> String { /// creator's name, by number, or with no creator at all -- and the token and show are what /// identify it. pub fn same_feed(a: &str, b: &str) -> bool { - a == b || patreon_parts(a).is_some_and(|p| Some(p) == patreon_parts(b)) + // Parsing both is the slow part, and only Patreon's addresses can differ and still match. + a == b + || (a.contains("patreon.com") + && b.contains("patreon.com") + && patreon_parts(a).is_some_and(|p| Some(p) == patreon_parts(b))) } /// A Patreon creator's name and shows, each show as (title, feed URL). @@ -666,6 +670,9 @@ pub async fn site_icon(client: &reqwest::Client, site: &str) -> Option { is_image(client, ico.as_str()).await.then(|| ico.into()) } +/// How many hosts one scan of a feed asks whether its artwork is on https. +const MAX_ART_HOSTS: usize = 8; + /// Artwork's address on https when its host serves it there, else as it was. The page is https /// and must not load http; the host is asked once per `known` (one feed's read), and an http /// address it does not serve on https stays, for /api/art to fetch (#90). Four of the five @@ -681,6 +688,10 @@ pub async fn prefer_https( let secure = format!("https://{rest}"); let ok = match known.get(&host) { Some(ok) => *ok, + // Each new host is a probe of up to 20 s, one after another on the scan, and the feed's + // author decides how many hosts there are: ports and user names make as many as they like + // of one server (#339). Real feeds put their artwork on a few. + None if known.len() >= MAX_ART_HOSTS => false, None => { let ok = is_image(client, &secure).await; known.insert(host, ok); @@ -1838,6 +1849,16 @@ Some **bold** and [a link](https://x.example/). assert_eq!((p1.season, p1.episode), (Some(8), Some(1))); } + #[tokio::test] + async fn artwork_on_more_hosts_than_the_limit_is_not_asked_about() { + let mut known: std::collections::HashMap = + (0..MAX_ART_HOSTS).map(|n| (format!("h{n}:80"), false)).collect(); + // No server answers here: were it asked, this would wait on a connection, not return. + let url = "http://192.0.2.1:9/a.png"; + assert_eq!(prefer_https(&reqwest::Client::new(), url, &mut known).await, url); + assert_eq!(known.len(), MAX_ART_HOSTS); + } + #[test] fn deeply_nested_opml_is_refused_before_it_is_parsed() { let nest = |n: usize| { diff --git a/crates/ipx/CHANGELOG.md b/crates/ipx/CHANGELOG.md index 8e40f20..8368712 100644 --- a/crates/ipx/CHANGELOG.md +++ b/crates/ipx/CHANGELOG.md @@ -31,6 +31,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - A new install's `admin` gets a random password, written to `admin-password` beside config.toml and readable by iPX's user alone, or the one in `IPX_ADMIN_PASSWORD`. Every install began as admin / ipodderx, a password published in the README, and the log printed it (#321). - A show kept from a private OPML or Patreon list stays private, with the list's username and password, once the list is removed or left. It became public and was shared to the directory (#332). - A feed's username and password go to its own host alone: not to its episodes on other hosts, nor to the feeds an OPML lists on other hosts, which took the OPML's password on every check (#327, #328). +- A check of one feed asks at most eight hosts whether its artwork is on https. A feed could name artwork on as many hosts as it liked, each asked in turn for up to 20 seconds, and hold up every other feed's check for hours (#339). +- An OPML or Patreon list may bring in up to 2,000 feeds; one with more shows an error and adds none. A list of tens of thousands held up checking for everyone, and adding its feeds read the whole feeds table once for each (#337). ## [1.4.0] - 2026-10-19 diff --git a/crates/ipx/src/config.rs b/crates/ipx/src/config.rs index c4aa319..04c4fb8 100644 --- a/crates/ipx/src/config.rs +++ b/crates/ipx/src/config.rs @@ -522,12 +522,12 @@ pub fn slug(text: &str) -> String { } /// `slug`, with a numeric suffix if that id is taken. -pub fn unique_slug(text: &str, taken: &BTreeMap) -> String { +pub fn unique_slug(text: &str, taken: impl Fn(&str) -> bool) -> String { let base = slug(text); - if !taken.contains_key(&base) { + if !taken(&base) { return base; } - (2..).map(|n| format!("{base}-{n}")).find(|s| !taken.contains_key(s)).unwrap() + (2..).map(|n| format!("{base}-{n}")).find(|s| !taken(s)).unwrap() } /// `$var`, or `~/fallback` when it is unset or empty, as the XDG base directory spec says. @@ -732,7 +732,7 @@ mod tests { private: false, }, ); - assert_eq!(unique_slug("The Daily", &taken), "the-daily-2"); + assert_eq!(unique_slug("The Daily", |s| taken.contains_key(s)), "the-daily-2"); } #[test] diff --git a/crates/ipx/src/main.rs b/crates/ipx/src/main.rs index a362dc2..f15f962 100644 --- a/crates/ipx/src/main.rs +++ b/crates/ipx/src/main.rs @@ -980,7 +980,7 @@ async fn add_one( taken.entry(id).or_insert_with(|| probe.clone()); } } - let id = config::unique_slug(&title, &taken); + let id = config::unique_slug(&title, |s| taken.contains_key(s)); cfg.feeds.insert(id.clone(), probe); Ok(id) } @@ -1050,7 +1050,7 @@ pub async fn subscribe_opml(ctx: &Ctx, doc: &opml::OPML, user_id: i64) -> Result let id = match existing { Some(id) => id, None => { - let id = config::unique_slug(&title, &cfg.feeds); + let id = config::unique_slug(&title, |s| cfg.feeds.contains_key(s)); cfg.feeds.insert( id.clone(), config::Feed { @@ -1758,7 +1758,7 @@ async fn scan_one( // heap. The items go; its files and read state move to each show as the show // lists them (`Db::adopt`), so no show comes up empty for want of a URL. ctx.db.clear_entries(id).await?; - return sync_group(ctx, id, feed_cfg, &shows).await; + return sync_group(ctx, id, &shows).await; } Ok(_) => {} // One show: the creator's feed is that show. // Already split: keep the shows it has rather than read the creator as one heap. @@ -1836,7 +1836,7 @@ async fn scan_one( // URL; sniffing the body also catches one served from a URL without that extension. if feed::is_opml(&bytes) { ctx.db.touch_feed(id, &feed_cfg.url).await?; - return sync_opml(ctx, id, feed_cfg, &bytes).await; + return sync_opml(ctx, id, &bytes).await; } let mut parsed = feed::parse(&bytes)?; @@ -2071,14 +2071,17 @@ async fn scan_one( /// A feed's body fetched by the scan ahead of its turn, and the site's icon if one was looked up. type Prefetched = (Result<(feed::Fetched, Option)>, Option); -async fn sync_opml(ctx: &Arc, parent_id: &str, parent: &config::Feed, bytes: &[u8]) -> Result { +async fn sync_opml(ctx: &Arc, parent_id: &str, bytes: &[u8]) -> Result { let listed = feed::parse_opml(bytes)?; if let Some(title) = feed::opml_title(bytes) { ctx.db.set_title(parent_id, &title).await?; } - sync_group(ctx, parent_id, parent, &listed).await + sync_group(ctx, parent_id, &listed).await } +/// How many feeds an OPML or Patreon list may bring in. +const MAX_LISTED: usize = 2000; + /// Brings the feed list in step with a list of feeds: a subscribed OPML, or a Patreon /// creator's shows. /// @@ -2086,15 +2089,22 @@ async fn sync_opml(ctx: &Arc, parent_id: &str, parent: &config::Feed, bytes /// the list is unsubscribed *only if nothing was ever downloaded for it* -- otherwise it /// is kept and flagged, because dropping it would orphan files on disk with nothing in /// the UI to explain them. -async fn sync_group( - ctx: &Arc, - parent_id: &str, - parent: &config::Feed, - listed: &[(String, String)], -) -> Result { +async fn sync_group(ctx: &Arc, parent_id: &str, listed: &[(String, String)]) -> Result { + // Each is a feed scanned for everyone subscribed to the list, and the list is its author's to + // write: one of tens of thousands held the scan for hours (#337). davewiner's had 922. + anyhow::ensure!( + listed.len() <= MAX_LISTED, + "it lists {} feeds, more than the {MAX_LISTED} a list may have", + listed.len() + ); let cfg = ctx.cfg(); let existing = ctx.db.managed_feeds().await?; let mut added = vec![]; + // Read once, not once a new feed: each read is the whole feeds table. + let known = ctx.db.feed_urls().await?; + let fixed: std::collections::HashSet<&str> = + cfg.feeds.keys().chain(existing.iter().map(|m| &m.id)).map(String::as_str).collect(); + let known_ids: std::collections::HashSet<&str> = known.iter().map(|(id, _)| id.as_str()).collect(); for (title, url) in listed { // Already known, whether derived or promoted into the config. @@ -2107,16 +2117,11 @@ async fn sync_group( continue; } // A removed feed keeps its rows, so its id is only free again for the same feed. - let known = ctx.db.feed_urls().await?; - let taken: std::collections::BTreeMap = cfg - .feeds - .keys() - .chain(existing.iter().map(|m| &m.id)) - .chain(added.iter()) - .chain(known.iter().filter(|(_, u)| !feed::same_feed(u, url)).map(|(id, _)| id)) - .map(|id| (id.clone(), parent.clone())) - .collect(); - let id = config::unique_slug(title, &taken); + let its_own: Vec<&str> = + known.iter().filter(|(_, u)| feed::same_feed(u, url)).map(|(id, _)| id.as_str()).collect(); + let id = config::unique_slug(title, |s| { + fixed.contains(s) || added.iter().any(|a| a == s) || (known_ids.contains(s) && !its_own.contains(&s)) + }); ctx.db.upsert_managed(&id, url, title, parent_id).await?; added.push(id); } @@ -3081,6 +3086,21 @@ mod tests { assert!(ctx.db.feed_summary("has-file").await.unwrap().orphaned, "and flagged as orphaned"); } + #[tokio::test] + async fn a_list_of_more_feeds_than_the_limit_is_refused() { + let ctx = Arc::new(test_ctx(config::Config::default()).await); + let listed: Vec<(String, String)> = + (0..=MAX_LISTED).map(|n| (format!("Show {n}"), format!("http://x/{n}.xml"))).collect(); + let err = sync_group(&ctx, "opml", &listed).await.err().expect("refused"); + assert!(err.to_string().contains("more than the 2000"), "{err:#}"); + assert!(ctx.db.managed_feeds().await.unwrap().is_empty(), "nothing taken from it"); + // At the limit, all of them, each its own id. + sync_group(&ctx, "opml", &listed[..MAX_LISTED]).await.unwrap(); + let ids: std::collections::HashSet = + ctx.db.managed_feeds().await.unwrap().into_iter().map(|m| m.id).collect(); + assert_eq!(ids.len(), MAX_LISTED); + } + #[tokio::test] async fn a_kept_feed_outlives_its_opml_for_whoever_kept_it() { let mut cfg = config::Config::default(); @@ -3102,8 +3122,7 @@ mod tests { assert!(ctx.db.set_kept(me, "b", true).await.unwrap()); // The OPML drops a: mine to keep, a feed of its own; yours, unkept, goes. - let parent = ctx.cfg().feeds["opml"].clone(); - sync_group(&ctx, "opml", &parent, &[("b".into(), "http://x/b.xml".into())]).await.unwrap(); + sync_group(&ctx, "opml", &[("b".into(), "http://x/b.xml".into())]).await.unwrap(); let a = ctx.cfg().feeds.get("a").cloned().expect("a is in the catalogue"); assert_eq!((a.url.as_str(), a.group, a.private), ("http://x/a.xml", None, true)); assert!(!ctx.db.managed_feeds().await.unwrap().iter().any(|m| m.id == "a"));