diff --git a/src-tauri/crates/core/src/plugin/host_impl.rs b/src-tauri/crates/core/src/plugin/host_impl.rs index 8fc867bd..248bfbff 100644 --- a/src-tauri/crates/core/src/plugin/host_impl.rs +++ b/src-tauri/crates/core/src/plugin/host_impl.rs @@ -332,6 +332,50 @@ fn sum_state_dir_bytes( // ----- waveflow:host/http ------------------------------------------------- +/// Render an error *with its cause chain* for a guest to surface. +/// +/// `reqwest::Error`'s `Display` stops at the outermost layer — a +/// failed request reads only "error sending request for url (…)", +/// which says the transport failed but never says why. The reason +/// (DNS lookup, connection refused, timeout, TLS handshake) lives in +/// `source()`, and the frontend shows this string verbatim, so +/// without unwinding it a bug report arrives with no way to tell +/// those cases apart. That is exactly what happened in issue #452. +/// +/// The chain is depth-capped: `source()` is author-controlled and +/// nothing forbids a cyclic or absurdly deep implementation. +fn describe_error(err: &dyn std::error::Error) -> String { + const MAX_DEPTH: usize = 8; + let mut out = err.to_string(); + // The last message actually appended, tracked separately from + // `out`: testing against the accumulated string would compare a + // cause to the tail of *everything* written so far, so a distinct + // cause that happens to end the buffer ("failed to connect" then + // "connect") would be dropped as a repeat. + let mut last = out.clone(); + let mut source = err.source(); + let mut depth = 0; + while let Some(cause) = source { + if depth >= MAX_DEPTH { + out.push_str(": …"); + break; + } + // Skip a layer that just restates its parent — `reqwest` and + // `hyper` both do this, and "timed out: timed out" reads + // worse than the single line it repeats. Only an exact repeat + // of the previous layer counts. + let text = cause.to_string(); + if text != last { + out.push_str(": "); + out.push_str(&text); + last = text; + } + source = cause.source(); + depth += 1; + } + out +} + impl wit_host::http::Host for HostCtx { fn send( &mut self, @@ -385,7 +429,7 @@ impl wit_host::http::Host for HostCtx { } let resp = match builder.send() { Ok(r) => r, - Err(e) => return Ok(Err(e.to_string())), + Err(e) => return Ok(Err(describe_error(&e))), }; let status = resp.status().as_u16(); let headers = resp @@ -408,7 +452,7 @@ impl wit_host::http::Host for HostCtx { let mut body = Vec::new(); let read = match resp.take(MAX_BODY_BYTES + 1).read_to_end(&mut body) { Ok(n) => n as u64, - Err(e) => return Ok(Err(e.to_string())), + Err(e) => return Ok(Err(describe_error(&e))), }; if read > MAX_BODY_BYTES { return Ok(Err("response body too large".to_string())); @@ -787,4 +831,104 @@ mod tests { Err(err) => panic!("expected InvalidStateKey, got {err:?}"), } } + + // ----- describe_error ------------------------------------------------- + + /// Minimal error whose `source()` we control, so the cause-chain + /// walk can be tested without conjuring a real `reqwest::Error`. + #[derive(Debug)] + struct Chained { + msg: &'static str, + source: Option>, + } + + impl std::fmt::Display for Chained { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(self.msg) + } + } + + impl std::error::Error for Chained { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.source + .as_deref() + .map(|s| s as &(dyn std::error::Error + 'static)) + } + } + + fn chain(msgs: &[&'static str]) -> Chained { + let mut it = msgs.iter().rev(); + let last = Chained { + msg: it.next().expect("at least one"), + source: None, + }; + it.fold(last, |acc, msg| Chained { + msg, + source: Some(Box::new(acc)), + }) + } + + /// The whole point: the root cause has to survive into the string + /// the user pastes into a bug report (issue #452). + #[test] + fn describe_error_keeps_the_root_cause() { + let err = chain(&[ + "error sending request for url (https://de1.api.radio-browser.info/json/…)", + "client error (Connect)", + "dns error", + "failed to lookup address information", + ]); + let out = super::describe_error(&err); + assert!( + out.contains("failed to lookup address information"), + "root cause lost: {out}" + ); + assert!(out.starts_with("error sending request for url")); + } + + #[test] + fn describe_error_handles_a_lone_error() { + let err = chain(&["timed out"]); + assert_eq!(super::describe_error(&err), "timed out"); + } + + /// `reqwest`/`hyper` often wrap an error in one that Displays + /// identically; echoing it would read "timed out: timed out". + #[test] + fn describe_error_skips_a_layer_that_restates_its_parent() { + let err = chain(&["timed out", "timed out"]); + assert_eq!(super::describe_error(&err), "timed out"); + } + + /// The dedup must fire on an exact repeat of the previous layer + /// and nothing else. Comparing against the accumulated buffer + /// instead would drop a distinct cause that merely happens to end + /// it — here "connect" is a suffix of "failed to connect", but it + /// is its own layer and has to survive. + #[test] + fn describe_error_only_dedups_an_exact_repeat_of_the_previous_layer() { + let err = chain(&["failed to connect", "connect"]); + assert_eq!(super::describe_error(&err), "failed to connect: connect"); + } + + /// `source()` is author-controlled — a pathological chain must not + /// build an unbounded string. + #[test] + fn describe_error_caps_a_very_deep_chain() { + let deep = vec!["layer"; 64]; + // Distinct messages, otherwise the dedup above hides the cap. + let mut err = Chained { + msg: "root", + source: None, + }; + for _ in 0..deep.len() { + err = Chained { + msg: "wrap", + source: Some(Box::new(err)), + }; + } + let out = super::describe_error(&err); + assert!(out.ends_with('\u{2026}'), "chain not capped: {out}"); + assert!(out.len() < 200, "chain not capped: {out}"); + } } diff --git a/src-tauri/crates/core/tests/fixtures/web-radio/manifest.toml b/src-tauri/crates/core/tests/fixtures/web-radio/manifest.toml index a714975e..90dd79be 100644 --- a/src-tauri/crates/core/tests/fixtures/web-radio/manifest.toml +++ b/src-tauri/crates/core/tests/fixtures/web-radio/manifest.toml @@ -9,7 +9,7 @@ schema_version = 1 [plugin] id = "web-radio" name = "Web Radio" -version = "0.1.0" +version = "0.1.1" author = "InstaZDLL" description = "Internet radio backed by radio-browser.info — 30 000+ stations searchable by country, language, tag, or codec." homepage = "https://github.com/InstaZDLL/WaveFlow" diff --git a/src-tauri/crates/core/tests/fixtures/web-radio/plugin.wasm b/src-tauri/crates/core/tests/fixtures/web-radio/plugin.wasm index 70413c37..6b37df1c 100644 Binary files a/src-tauri/crates/core/tests/fixtures/web-radio/plugin.wasm and b/src-tauri/crates/core/tests/fixtures/web-radio/plugin.wasm differ diff --git a/src-tauri/plugins/web-radio/Cargo.lock b/src-tauri/plugins/web-radio/Cargo.lock index ff846ffe..c8bd7ced 100644 --- a/src-tauri/plugins/web-radio/Cargo.lock +++ b/src-tauri/plugins/web-radio/Cargo.lock @@ -100,7 +100,7 @@ checksum = "e6e4313cd5fcd3dad5cafa179702e2b244f760991f45397d14d4ebf38247da75" [[package]] name = "waveflow-plugin-web-radio" -version = "0.1.0" +version = "0.1.1" dependencies = [ "serde", "serde_json", diff --git a/src-tauri/plugins/web-radio/Cargo.toml b/src-tauri/plugins/web-radio/Cargo.toml index cc9fb10d..82a3312c 100644 --- a/src-tauri/plugins/web-radio/Cargo.toml +++ b/src-tauri/plugins/web-radio/Cargo.toml @@ -9,7 +9,7 @@ [package] name = "waveflow-plugin-web-radio" -version = "0.1.0" +version = "0.1.1" edition = "2021" license = "GPL-3.0-only" description = "WaveFlow plugin — internet radio source backed by radio-browser.info" diff --git a/src-tauri/plugins/web-radio/manifest.toml b/src-tauri/plugins/web-radio/manifest.toml index a714975e..90dd79be 100644 --- a/src-tauri/plugins/web-radio/manifest.toml +++ b/src-tauri/plugins/web-radio/manifest.toml @@ -9,7 +9,7 @@ schema_version = 1 [plugin] id = "web-radio" name = "Web Radio" -version = "0.1.0" +version = "0.1.1" author = "InstaZDLL" description = "Internet radio backed by radio-browser.info — 30 000+ stations searchable by country, language, tag, or codec." homepage = "https://github.com/InstaZDLL/WaveFlow" diff --git a/src-tauri/plugins/web-radio/src/lib.rs b/src-tauri/plugins/web-radio/src/lib.rs index ba1d8ba4..ffa9d812 100644 --- a/src-tauri/plugins/web-radio/src/lib.rs +++ b/src-tauri/plugins/web-radio/src/lib.rs @@ -20,12 +20,30 @@ use bindings::waveflow::host::log::{self, Level}; use serde::Deserialize; -/// Default federated mirror. radio-browser.info uses round-robin -/// DNS across regional mirrors (`de1`, `at1`, `us1`, …); the -/// chosen prefix doesn't matter — any of them serves the same -/// catalogue. We pin one rather than resolving via -/// `/json/servers` to avoid an extra round-trip per session. -const MIRROR: &str = "https://de1.api.radio-browser.info"; +/// radio-browser.info hosts, tried in order until one answers. +/// +/// `all.` is the project's official **round-robin DNS** entry: it +/// resolves to whichever nodes are alive right now, so a node that +/// goes down is dropped from rotation without anyone editing this +/// list. That matters more than it sounds — this plugin used to pin +/// `de1.` alone, on the assumption that "any regional prefix serves +/// the same catalogue". Several of the prefixes that assumption named +/// (`at1.`, `nl1.`, `fi1.`) no longer resolve at all today, so a +/// hard-coded node list rots on its own; only `all.` tracks reality. +/// +/// `de1.` stays as an explicit second attempt: it's a node that does +/// answer, and having a second entry means a failed first request is +/// retried at all — which is what the user-visible bug in issue #452 +/// came down to (the first call of a session failed, and simply +/// leaving the page and coming back made it work). +/// +/// Both hosts are already covered by the manifest allowlist +/// (`https://*.api.radio-browser.info/**`), so this needs no +/// permission change. +const HOSTS: &[&str] = &[ + "https://all.api.radio-browser.info", + "https://de1.api.radio-browser.info", +]; /// `User-Agent` radio-browser.info asks for in their API /// guidelines. Bumped automatically with the crate version so @@ -37,6 +55,18 @@ const USER_AGENT: &str = concat!("WaveFlow/Web-Radio/", env!("CARGO_PKG_VERSION" /// to drop stations the federated bot has flagged as unreachable. const PAGE_LIMIT: u32 = 50; +/// Query params shared by every "list stations" endpoint (`bytag`, +/// `bycountrycodeexact`, `search`): most-voted first, and let the +/// federated bot's reachability flag filter out dead streams. +/// +/// One constant rather than three copies — they must stay identical +/// for the three category kinds to rank consistently, and a future +/// tweak (a different `order`, say) would otherwise have to be +/// remembered in three places. `topvote` / `lastchange` are NOT in +/// this set: they carry their ordering in the endpoint name and take +/// the limit as a path segment. +const LIST_PARAMS: &str = "order=votes&reverse=true&hidebroken=true"; + struct WebRadio; impl Guest for WebRadio { @@ -69,8 +99,8 @@ impl Guest for WebRadio { /// [`PAGE_LIMIT`] per call — the UI paginates from there. fn resolve(query: String) -> Result, String> { log::emit(Level::Debug, &format!("web-radio resolve: {query}")); - let url = build_url(&query)?; - let body = fetch_json(&url)?; + let path = build_path(&query)?; + let body = fetch_json(&path)?; let stations: Vec = serde_json::from_slice(&body) .map_err(|e| format!("radio-browser response parse: {e}"))?; Ok(stations.into_iter().filter_map(to_track).collect()) @@ -100,7 +130,9 @@ fn entry(label: &str, query: &str) -> Entry { } } -/// Decode the entry / search token into a radio-browser API URL. +/// Decode the entry / search token into a **host-relative** +/// radio-browser API path — [`fetch_json`] prefixes whichever host in +/// [`HOSTS`] answers, so the path must not carry one. /// Tokens we understand: /// /// - `top` → top-voted stations @@ -110,19 +142,19 @@ fn entry(label: &str, query: &str) -> Entry { /// - anything else non-empty → treated as a free-text search term /// (the host's search box hands its raw input here and expects /// matches against station names) -fn build_url(query: &str) -> Result { +fn build_path(query: &str) -> Result { let trimmed = query.trim(); if trimmed.is_empty() { return Err("empty query".into()); } if trimmed.eq_ignore_ascii_case("top") { return Ok(format!( - "{MIRROR}/json/stations/topvote/{PAGE_LIMIT}?hidebroken=true" + "/json/stations/topvote/{PAGE_LIMIT}?hidebroken=true" )); } if trimmed.eq_ignore_ascii_case("trending") { return Ok(format!( - "{MIRROR}/json/stations/lastchange/{PAGE_LIMIT}?hidebroken=true" + "/json/stations/lastchange/{PAGE_LIMIT}?hidebroken=true" )); } if let Some(tag) = trimmed.strip_prefix("tag:") { @@ -131,7 +163,7 @@ fn build_url(query: &str) -> Result { return Err("empty tag".into()); } return Ok(format!( - "{MIRROR}/json/stations/bytag/{tag}?limit={PAGE_LIMIT}&order=votes&reverse=true&hidebroken=true" + "/json/stations/bytag/{tag}?limit={PAGE_LIMIT}&{LIST_PARAMS}" )); } // `country:` → stations in one country, by ISO 3166-1 @@ -148,7 +180,7 @@ fn build_url(query: &str) -> Result { } let code = code.to_ascii_uppercase(); return Ok(format!( - "{MIRROR}/json/stations/bycountrycodeexact/{code}?limit={PAGE_LIMIT}&order=votes&reverse=true&hidebroken=true" + "/json/stations/bycountrycodeexact/{code}?limit={PAGE_LIMIT}&{LIST_PARAMS}" )); } // Both the explicit `search:` form and the free-text fallback @@ -161,29 +193,65 @@ fn build_url(query: &str) -> Result { return Err("empty search term".into()); } Ok(format!( - "{MIRROR}/json/stations/search?name={term}&limit={PAGE_LIMIT}&order=votes&reverse=true&hidebroken=true" + "/json/stations/search?name={term}&limit={PAGE_LIMIT}&{LIST_PARAMS}" )) } -/// Issue a GET, surface the body bytes. The host enforces the -/// manifest's HTTP allowlist + the 10 MB response cap + the offline -/// short-circuit, so failure modes here are limited to network -/// errors and non-2xx HTTP statuses (which we propagate verbatim). -fn fetch_json(url: &str) -> Result, String> { - let req = Request { - method: "GET".into(), - url: url.into(), - headers: vec![ - ("User-Agent".into(), USER_AGENT.into()), - ("Accept".into(), "application/json".into()), - ], - body: None, - }; - let resp = http::send(&req).map_err(|e| format!("http: {e}"))?; - if !(200..300).contains(&resp.status) { - return Err(format!("http status {}", resp.status)); +/// Issue a GET for `path` against each host in [`HOSTS`] until one +/// answers, and surface the body bytes. +/// +/// The host enforces the manifest's HTTP allowlist + the 10 MB +/// response cap + the offline short-circuit, so failure modes here +/// are limited to network errors and non-2xx HTTP statuses. +/// +/// **What is and isn't retried.** A transport failure or a 5xx means +/// "this node couldn't serve it" — worth asking the next one. A 4xx +/// is the API rejecting the request itself (bad tag, malformed +/// query); every node would answer the same way, so it returns +/// immediately rather than burning a second round-trip to be told +/// the same thing twice. +/// +/// Offline mode surfaces as the host's `503` sentinel. It is left in +/// the retry path deliberately: it costs one extra no-op call (the +/// host short-circuits before touching the network), and special- +/// casing the value here would couple the guest to a host +/// implementation detail it has no contract for. +fn fetch_json(path: &str) -> Result, String> { + let mut last_err = String::from("no radio-browser host reachable"); + for host in HOSTS { + let req = Request { + method: "GET".into(), + url: format!("{host}{path}"), + headers: vec![ + ("User-Agent".into(), USER_AGENT.into()), + ("Accept".into(), "application/json".into()), + ], + body: None, + }; + match http::send(&req) { + Ok(resp) if (200..300).contains(&resp.status) => return Ok(resp.body), + // Client-side rejection: the request itself is wrong (bad + // tag, malformed query), so the next node answers the same + // and asking it again just wastes a round-trip. + // + // 429 is the exception: a rate limit is per-node state, not + // a verdict on the request, and the whole point of having a + // second host is to route around a node that won't serve us + // right now. + Ok(resp) if (400..500).contains(&resp.status) && resp.status != 429 => { + return Err(format!("http status {}", resp.status)); + } + Ok(resp) => { + last_err = format!("{host}: http status {}", resp.status); + log::emit(Level::Warn, &format!("radio-browser {last_err}")); + } + Err(e) => { + last_err = format!("{host}: {e}"); + log::emit(Level::Warn, &format!("radio-browser {last_err}")); + } + } } - Ok(resp.body) + Err(format!("http: {last_err}")) } /// Minimal `application/x-www-form-urlencoded`-style encoder for diff --git a/src-tauri/resources/plugins/web-radio/manifest.toml b/src-tauri/resources/plugins/web-radio/manifest.toml index a714975e..90dd79be 100644 --- a/src-tauri/resources/plugins/web-radio/manifest.toml +++ b/src-tauri/resources/plugins/web-radio/manifest.toml @@ -9,7 +9,7 @@ schema_version = 1 [plugin] id = "web-radio" name = "Web Radio" -version = "0.1.0" +version = "0.1.1" author = "InstaZDLL" description = "Internet radio backed by radio-browser.info — 30 000+ stations searchable by country, language, tag, or codec." homepage = "https://github.com/InstaZDLL/WaveFlow" diff --git a/src-tauri/resources/plugins/web-radio/plugin.wasm b/src-tauri/resources/plugins/web-radio/plugin.wasm index 70413c37..6b37df1c 100644 Binary files a/src-tauri/resources/plugins/web-radio/plugin.wasm and b/src-tauri/resources/plugins/web-radio/plugin.wasm differ