diff --git a/crates/core/src/protocol.rs b/crates/core/src/protocol.rs --- a/crates/core/src/protocol.rs +++ b/crates/core/src/protocol.rs @@ -155,7 +155,8 @@ pub enum Source { Feed, /// The GitHub REST API. GitHubApi, - /// github.com release pages (no API quota; used when the API is rate-limited). + /// github.com release pages (no API quota; used whenever the API gave no answer, whether it + /// was out of quota or couldn't be reached). GitHubPages, /// Saved answers from an earlier check. Cache, diff --git a/crates/native/src/http.rs b/crates/native/src/http.rs --- a/crates/native/src/http.rs +++ b/crates/native/src/http.rs @@ -141,7 +141,7 @@ impl Http { { req = req.header("Authorization", &format!("Bearer {token}")); } - let mut resp = req.call().map_err(|e| format!("couldn't reach GitHub ({e})"))?; + let mut resp = req.call().map_err(|e| format!("{UNREACHABLE} GitHub ({e})"))?; let header = |name: &str| resp.headers().get(name).and_then(|v| v.to_str().ok()).map(str::to_string); let status = resp.status().as_u16(); match status { @@ -170,7 +170,7 @@ impl Http { let failed = |why: String| FeedError { why, throttled: false, retry_after: None }; let agent = if is_loopback(url) { &self.direct } else { &self.feed }; let mut resp = - agent.get(url).header("Accept", "application/json").call().map_err(|e| failed(format!("couldn't reach {} ({e})", host(url))))?; + agent.get(url).header("Accept", "application/json").call().map_err(|e| failed(format!("{UNREACHABLE} {} ({e})", host(url))))?; // Seconds only; an HTTP date falls back to our own backoff. let retry_after = resp.headers().get("retry-after").and_then(|v| v.to_str().ok()).and_then(|r| r.trim().parse::().ok()); match resp.status().as_u16() { @@ -182,7 +182,7 @@ impl Http { /// GET a small text file (following redirects). pub fn get_text(&self, url: &str) -> Result, String> { let agent = if is_loopback(url) { &self.direct } else { &self.small }; - let mut resp = agent.get(url).call().map_err(|e| format!("couldn't reach {} ({e})", host(url)))?; + let mut resp = agent.get(url).call().map_err(|e| format!("{UNREACHABLE} {} ({e})", host(url)))?; match resp.status().as_u16() { 200 => read_text_capped(&mut resp, MAX_SMALL).map(Some).map_err(|e| format!("unreadable answer ({e})")), 404 => Ok(None), @@ -195,7 +195,7 @@ impl Http { /// on the body when the length wasn't announced or was wrong. pub fn get_bytes(&self, url: &str, max: usize) -> Result>, String> { let agent = if is_loopback(url) { &self.direct } else { &self.small }; - let mut resp = agent.get(url).call().map_err(|e| format!("couldn't reach {} ({e})", host(url)))?; + let mut resp = agent.get(url).call().map_err(|e| format!("{UNREACHABLE} {} ({e})", host(url)))?; match resp.status().as_u16() { 200 => { let announced = resp.headers().get("content-length").and_then(|v| v.to_str().ok()).and_then(|v| v.parse::().ok()); @@ -214,7 +214,7 @@ impl Http { /// Where `url` redirects to, without following it. pub fn redirect_target(&self, url: &str) -> Result, String> { - let resp = self.no_redirect.head(url).call().map_err(|e| format!("couldn't reach {} ({e})", host(url)))?; + let resp = self.no_redirect.head(url).call().map_err(|e| format!("{UNREACHABLE} {} ({e})", host(url)))?; match resp.status().as_u16() { 300..=399 => Ok(resp.headers().get("location").and_then(|v| v.to_str().ok()).map(str::to_string)), 404 => Ok(None), @@ -569,6 +569,20 @@ fn host(url: &str) -> &str { url.split_once("://").and_then(|(_, rest)| rest.split('/').next()).unwrap_or(url) } +/// How every error in this module starts when the request didn't complete at all: no name +/// resolution, no route, a refused connection, a dropped handshake, nothing within the timeout. +/// Kept next to the errors it describes so the two can't drift apart. (`Http::attempt`'s +/// "couldn't connect" is deliberately outside it: that one never leaves the download loop.) +const UNREACHABLE: &str = "couldn't reach"; + +/// Whether one of this module's GETs ended without the host completing an answer, as opposed to +/// answering with something unwelcome (an HTTP status, a body that couldn't be read). A caller +/// with a second host to try uses this to tell "ask the other one" from "this network is going +/// nowhere". Erring towards `true` only costs that caller one skipped retry. +pub fn is_unreachable(error: &str) -> bool { + error.starts_with(UNREACHABLE) +} + /// Reads a text body, rejecting one whose *decoded* size exceeds `max`. /// /// ureq's own `limit` guards the *compressed* stream: it sits below the transparent gzip decoder, @@ -611,6 +625,22 @@ pub(crate) mod tests { assert_eq!(host("not a url"), "not a url"); } + #[test] + fn a_host_that_never_answered_is_told_from_one_that_said_no() { + // A caller with a second host to try leans on this: only the first kind is worth retrying + // somewhere else. + let gone = std::net::TcpListener::bind("127.0.0.1:0").unwrap().local_addr().unwrap(); + let http = Http::new().unwrap(); + let refused = http.get_text(&format!("http://{gone}/file.txt")).unwrap_err(); + assert!(is_unreachable(&refused), "{refused}"); + assert!(is_unreachable(&http.get_feed(&format!("http://{gone}/feed.json")).unwrap_err().why)); + assert!(is_unreachable(&http.redirect_target(&format!("http://{gone}/latest")).unwrap_err())); + // A refusal is an answer, and so is a body that can't be read. + assert!(!is_unreachable("GitHub refused the request (HTTP 403)")); + assert!(!is_unreachable(&format!("{} answered HTTP 503", host("https://github.com/storytold/photocraft/releases/latest")))); + assert!(!is_unreachable("unreadable answer from GitHub (the answer is larger than 4194304 bytes)")); + } + #[test] fn cleaning_keeps_current_partial_files_and_removes_leftovers() { let dir = tempfile::tempdir().unwrap(); diff --git a/crates/native/src/source.rs b/crates/native/src/source.rs --- a/crates/native/src/source.rs +++ b/crates/native/src/source.rs @@ -1,10 +1,17 @@ //! Where release information comes from. //! //! [`GitHubSource`] asks the GitHub REST API (one request per app, revalidated with ETags) and, -//! when the API is out of quota, falls back to github.com's release pages: the +//! whenever that produces no answer, falls back to github.com's release pages: the //! `releases/latest` redirect names the tag and `SHA256SUMS.txt` lists every file with its //! checksum, which costs no API quota at all. A later [`ReleaseSource`] will ask the ArtCraft //! catalog API for every app in one request. +//! +//! The fallback is not only for the API's quota. `api.github.com` is a different host from +//! `github.com`, and on a managed machine it is often the one that doesn't answer: a firewall, a +//! filtering DNS resolver or an HTTPS-inspecting proxy blocks the API (no connection at all, or a +//! 403/407 with none of GitHub's rate-limit headers) while the release pages load normally. Taking +//! that route only when GitHub itself said "out of quota" left every app the user hadn't installed +//! showing "Offline" on such a machine, which is what `resolve` now avoids. use std::collections::BTreeMap; use std::sync::{Mutex, PoisonError}; @@ -54,17 +61,26 @@ struct CacheFile { entries: BTreeMap, } +/// Once github.com itself hasn't answered, the release-pages route is left alone for this long. +/// Without it a machine with no network at all would take that route for every app in the catalog +/// in turn, each waiting on the same unreachable host, before reporting what the first try already +/// settled. (A user-pressed check still probes github.com once per app through +/// [`ReleaseSource::newest_tag`], which answers a different question and has its own quick timeout.) +const PAGES_RETRY_AFTER: i64 = 60; + pub struct GitHubSource { http: Http, cache: Mutex, cache_path: std::path::PathBuf, limited_until: Mutex>, + /// github.com couldn't be reached; don't ask it again until this time (Unix seconds). + pages_quiet_until: Mutex>, } impl GitHubSource { pub fn new(http: Http, cache_path: std::path::PathBuf) -> Self { let cache = std::fs::read(&cache_path).ok().and_then(|b| serde_json::from_slice::(&b).ok()).unwrap_or_default(); - Self { http, cache: Mutex::new(cache), cache_path, limited_until: Mutex::new(None) } + Self { http, cache: Mutex::new(cache), cache_path, limited_until: Mutex::new(None), pages_quiet_until: Mutex::new(None) } } fn entry(&self, repo: &str, prereleases: bool) -> Option { @@ -118,12 +134,39 @@ impl GitHubSource { } } + /// Whether github.com has just shown it can't be reached, so there is no point asking it again + /// for the next app in the same sweep. + fn pages_quiet(&self) -> bool { + let until = *self.pages_quiet_until.lock().unwrap_or_else(PoisonError::into_inner); + until.is_some_and(|u| u > crate::now()) + } + + /// Passes a release-pages error through, noting it when it means github.com never answered. + fn note_pages_error(&self, why: String) -> String { + if crate::http::is_unreachable(&why) { + *self.pages_quiet_until.lock().unwrap_or_else(PoisonError::into_inner) = Some(crate::now().saturating_add(PAGES_RETRY_AFTER)); + } + why + } + /// The quota-free route through github.com (stable releases only). fn ask_pages(&self, repo: &str) -> Result { - let location = self.http.redirect_target(&format!("https://github.com/{repo}/releases/latest"))?; - let Some(tag) = location.as_deref().and_then(|l| release::tag_from_redirect(l, repo)) else { - // No published release: github.com sends us to the release list instead. - return Ok(Answer { latest: LatestRelease::None, source: Source::GitHubPages }); + if self.pages_quiet() { + return Err("github.com couldn't be reached a moment ago".into()); + } + let location = self.http.redirect_target(&format!("https://github.com/{repo}/releases/latest")).map_err(|e| self.note_pages_error(e))?; + let no_release = || Ok(Answer { latest: LatestRelease::None, source: Source::GitHubPages }); + // A repo that isn't there answers 404, which `redirect_target` reports as no target at all. + let Some(target) = location.as_deref() else { return no_release() }; + // Nothing published yet: github.com sends us to the repo's own release list instead. + if is_release_list(target, repo) { + return no_release(); + } + let Some(tag) = release::tag_from_redirect(target, repo) else { + // Somewhere that isn't github.com's: a captive portal's sign-in page, a filtering + // proxy's notice. Reading that as "no release yet" would turn a blocked network into a + // confident "Coming soon" on every card, so let the saved answer stand in instead. + return Err("the latest-release redirect went somewhere that isn't github.com's".into()); }; if let Some(e) = self.entry(repo, false) && let Some(r) = e.release.as_ref().filter(|r| r.tag == tag) @@ -131,6 +174,8 @@ impl GitHubSource { return Ok(Answer { latest: LatestRelease::Found(r.clone()), source: Source::GitHubPages }); } let sums_url = format!("{}{tag}/SHA256SUMS.txt", release::download_prefix(repo)); + // Not `note_pages_error`: this one redirects to GitHub's asset CDN, so its failure says + // nothing about whether github.com itself can be reached for the next app. let sums = self.http.get_text(&sums_url)?.map(|t| release::parse_sha256sums(&t)).unwrap_or_default(); let r = release::release_from_sums(repo, &tag, &sums); self.store(repo, Entry { etag: None, release: Some(r.clone()), prereleases: false, fetched_at: crate::now() }); @@ -138,29 +183,60 @@ impl GitHubSource { } } -impl ReleaseSource for GitHubSource { - fn latest(&self, repo: &str, prereleases: bool) -> Answer { - let api_error = if self.limited() { - None - } else { - match self.ask_api(repo, prereleases) { - Ok(Some(answer)) => return answer, - Ok(None) => None, - Err(e) => Some(e), - } - }; - if !prereleases && api_error.is_none() { - match self.ask_pages(repo) { - Ok(answer) => return answer, - Err(e) => log::warn!("{repo}: release pages: {e}"), - } +/// Whether `location` is github.com's own release list for `repo`, which is where it sends a +/// request for the latest release of a repo that has published none. +fn is_release_list(location: &str, repo: &str) -> bool { + let list = format!("https://github.com/{repo}/releases"); + location == list || location.strip_suffix('/') == Some(list.as_str()) +} + +/// Which route one lookup takes, with the three ways of asking handed in so the policy can be +/// tested without a network. +/// +/// The REST API goes first: it is the only route that knows pre-releases, and an ETag makes a +/// repeat check nearly free. When it produces no answer — out of quota, unreachable, refused, or +/// an answer that couldn't be read — github.com's release pages are asked instead, because they +/// cost no quota and sit on a host that may well be reachable when the API's isn't (see the module +/// docs). Pre-releases aren't published there, so with pre-releases turned on that route is +/// skipped. Only when neither answered does the lookup settle for the last saved answer, and +/// report the failure when there isn't one. +fn resolve( + repo: &str, + prereleases: bool, + limited: bool, + ask_api: impl FnOnce() -> Result, String>, + ask_pages: impl FnOnce() -> Result, + cached: impl FnOnce() -> Option, +) -> Answer { + let api_error = if limited { + None + } else { + match ask_api() { + Ok(Some(answer)) => return answer, + Ok(None) => None, + Err(e) => Some(e), + } + }; + if !prereleases { + if let Some(e) = &api_error { + log::info!("{repo}: the GitHub API didn't answer ({e}); trying the release pages"); } - let why = api_error.unwrap_or_else(|| "GitHub's request limit was reached".into()); - match self.cached(repo, prereleases) { - Some(latest) => Answer { latest, source: Source::Cache }, - None => Answer { latest: LatestRelease::Failed(why), source: Source::Cache }, + match ask_pages() { + Ok(answer) => return answer, + Err(e) => log::warn!("{repo}: release pages: {e}"), } } + let why = api_error.unwrap_or_else(|| "GitHub's request limit was reached".into()); + match cached() { + Some(latest) => Answer { latest, source: Source::Cache }, + None => Answer { latest: LatestRelease::Failed(why), source: Source::Cache }, + } +} + +impl ReleaseSource for GitHubSource { + fn latest(&self, repo: &str, prereleases: bool) -> Answer { + resolve(repo, prereleases, self.limited(), || self.ask_api(repo, prereleases), || self.ask_pages(repo), || self.cached(repo, prereleases)) + } fn cached(&self, repo: &str, prereleases: bool) -> Option { self.entry(repo, prereleases).map(|e| e.release.map_or(LatestRelease::None, LatestRelease::Found)) @@ -196,3 +272,140 @@ impl ReleaseSource for GitHubSource { } } } + +#[cfg(test)] +mod tests { + use super::*; + use std::cell::Cell; + + const REPO: &str = "storytold/photocraft"; + /// What a blocked `api.github.com` leaves behind, and what github.com then says in turn. + const NO_API: &str = "couldn't reach GitHub (io: connection refused)"; + const NO_PAGES: &str = "couldn't reach github.com (io: connection refused)"; + + fn answer(tag: &str, source: Source) -> Answer { + Answer { latest: LatestRelease::Found(Release { tag: tag.to_string(), ..Release::default() }), source } + } + + fn tag_of(a: &Answer) -> Option<&str> { + match &a.latest { + LatestRelease::Found(r) => Some(r.tag.as_str()), + _ => None, + } + } + + /// Records whether the release-pages route was taken, and answers `reply` when it was. + fn pages(asked: &Cell, reply: Result) -> impl FnOnce() -> Result { + move || { + asked.set(true); + reply + } + } + + #[test] + fn an_api_answer_ends_the_lookup() { + let asked = Cell::new(false); + let a = resolve(REPO, false, false, || Ok(Some(answer("v0.6.0", Source::GitHubApi))), pages(&asked, Err("unused".into())), || None); + assert_eq!((tag_of(&a), a.source), (Some("v0.6.0"), Source::GitHubApi)); + assert!(!asked.get(), "github.com is left alone when the API answered"); + } + + #[test] + fn an_api_that_cannot_be_reached_still_reaches_the_release_pages() { + // The bug this guards (#875): a machine where api.github.com is blocked but github.com is + // not showed every app the user hadn't installed as "Offline", because the quota-free + // route was taken only when GitHub itself said "out of quota". + for api_error in [ + NO_API, + "GitHub refused the request (HTTP 403)", + "GitHub answered HTTP 407", + "GitHub answered HTTP 503", + "GitHub said nothing changed, but there's no saved answer", + ] { + let asked = Cell::new(false); + let a = resolve(REPO, false, false, || Err(api_error.into()), pages(&asked, Ok(answer("v0.6.0", Source::GitHubPages))), || None); + assert!(asked.get(), "{api_error}"); + assert_eq!((tag_of(&a), a.source), (Some("v0.6.0"), Source::GitHubPages), "{api_error}"); + } + } + + #[test] + fn an_exhausted_quota_takes_the_same_route() { + // Already known to be out of quota, so the API isn't asked at all. + let (asked, api_asked) = (Cell::new(false), Cell::new(false)); + let api = || { + api_asked.set(true); + Ok(None) + }; + let a = resolve(REPO, false, true, api, pages(&asked, Ok(answer("v0.6.0", Source::GitHubPages))), || None); + assert_eq!(a.source, Source::GitHubPages); + assert!(asked.get() && !api_asked.get(), "an API known to be out of quota is left alone"); + // Told so by this very request. + let asked = Cell::new(false); + let a = resolve(REPO, false, false, || Ok(None), pages(&asked, Ok(answer("v0.6.0", Source::GitHubPages))), || None); + assert_eq!(a.source, Source::GitHubPages); + assert!(asked.get()); + } + + #[test] + fn with_prereleases_on_the_release_pages_cannot_answer() { + // They only ever name the latest stable release, so there is nothing to ask them for. + let asked = Cell::new(false); + let saved = || Some(LatestRelease::Found(Release { tag: "v0.6.0-rc.1".into(), ..Release::default() })); + let a = resolve(REPO, true, false, || Err(NO_API.into()), pages(&asked, Err("unused".into())), saved); + assert!(!asked.get()); + assert_eq!((tag_of(&a), a.source), (Some("v0.6.0-rc.1"), Source::Cache)); + } + + #[test] + fn when_neither_route_answers_the_saved_answer_stands_in() { + let asked = Cell::new(false); + let saved = || Some(LatestRelease::Found(Release { tag: "v0.5.0".into(), ..Release::default() })); + let a = resolve(REPO, false, false, || Err(NO_API.into()), pages(&asked, Err(NO_PAGES.into())), saved); + assert!(asked.get()); + assert_eq!((tag_of(&a), a.source), (Some("v0.5.0"), Source::Cache)); + } + + #[test] + fn with_nothing_saved_the_api_reason_is_what_the_user_is_told() { + let asked = Cell::new(false); + let a = resolve(REPO, false, false, || Err(NO_API.into()), pages(&asked, Err(NO_PAGES.into())), || None); + assert!(asked.get()); + assert_eq!(a.latest, LatestRelease::Failed(NO_API.into())); + // Out of quota, with github.com no help either: say that instead. + let a = resolve(REPO, false, true, || Ok(None), || Err(NO_PAGES.into()), || None); + assert_eq!(a.latest, LatestRelease::Failed("GitHub's request limit was reached".into())); + } + + #[test] + fn only_github_coms_own_answer_is_read_as_no_release_yet() { + // github.com sends a repo without releases to its release list. A sign-in page from a + // captive portal or a filtering proxy must not be read the same way: the saved answer is + // better than a confident "Coming soon" on every card. + assert!(is_release_list("https://github.com/storytold/photocraft/releases", REPO)); + assert!(is_release_list("https://github.com/storytold/photocraft/releases/", REPO)); + for elsewhere in [ + "https://github.com/storytold/photocraft/releases/tag/v0.6.0", + "https://github.com/storytold/photocraft/releases/latest", + "https://github.com/storytold/photocraft", + "https://github.com/someone-else/photocraft/releases", + "http://portal.example/login?continue=github.com", + ] { + assert!(!is_release_list(elsewhere, REPO), "{elsewhere}"); + } + } + + #[test] + fn github_com_is_not_probed_again_for_every_app_once_it_is_silent() { + let dir = tempfile::tempdir().unwrap(); + let source = GitHubSource::new(Http::new().unwrap(), dir.path().join("releases.json")); + assert!(!source.pages_quiet()); + // An HTTP status is an answer: github.com is still worth asking for the next app. + assert_eq!(source.note_pages_error("github.com answered HTTP 500".into()), "github.com answered HTTP 500"); + assert!(!source.pages_quiet()); + assert_eq!(source.note_pages_error(NO_PAGES.into()), NO_PAGES); + assert!(source.pages_quiet()); + let err = source.ask_pages(REPO).unwrap_err(); + assert!(err.contains("couldn't be reached a moment ago"), "{err}"); + } +}