diff --git a/src/clients/tangled/web/backfill.rs b/src/clients/tangled/web/backfill.rs index 756d637..b2d1425 100644 --- a/src/clients/tangled/web/backfill.rs +++ b/src/clients/tangled/web/backfill.rs @@ -41,6 +41,13 @@ use crate::lexicon::tangled::PULL_NSID; /// eight covers the widest gap a real stall has produced here (six pulls in /// ~29 hours) with room to spare, without letting a long-stalled busy repo /// turn `pr list` into a crawl. +/// +/// It does not scale with the caller's `--limit`, deliberately. The budget +/// bounds *bytes*, not honesty: a `--limit 100` that scaled would spend a +/// hundred multi-megabyte fetches on a scrape of HTML that is slated for +/// deletion. What was missing was not headroom but the admission — the walk +/// now reports what the budget left unopened (see [`Backfill::unexamined`]) +/// and the caller says so, which is what a listing owes a reader either way. const PROBE_BUDGET: usize = 8; /// Stop after this many consecutive probes that landed on pulls the caller @@ -48,8 +55,75 @@ const PROBE_BUDGET: usize = 8; /// index died at is missing and everything older is present, so consecutive /// known pulls mean the walk has crossed the edge and the rest of the budget /// would be spent confirming it. +/// +/// That argument is about the shape of `known`, not about the streak, so it +/// holds only for [`Gap::Suffix`] — see [`Gap`]. const KNOWN_STREAK_STOP: usize = 3; +/// What the caller's `known` set is, which decides what a run of already-known +/// pulls proves. +/// +/// The distinction is the difference between a stop that has reached the end +/// of the gap and a stop that has merely run into the reader's own work. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub(crate) enum Gap { + /// `known` is a stale index's listing plus the reader's own pulls. The + /// index is complete below the point it died at, so three known pulls in + /// a row put the walk past the gap's edge and everything older is + /// already in hand. + Suffix, + /// `known` is the reader's own pulls and nothing else — `--source web` + /// with no index beside it, where the web listing is the *only* + /// cross-account source. Absence from `known` then says nothing about + /// age: three of the reader's own pulls in a row are three of the + /// reader's own pulls, and every other author's below them is still + /// unseen. The streak stop is off, and the budget is the only bound. + Unbounded, +} + +/// Why the walk stopped, or that it has not. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum Step { + /// Open the next listed pull's page. + Probe, + /// [`PROBE_BUDGET`] is spent. Whatever is left of the listing is + /// unexamined, and the caller is told so rather than handed a short + /// listing that looks whole. + BudgetSpent, + /// The walk has crossed the index's edge, and what is left of the + /// listing is already in the caller's hands. + PastTheEdge, +} + +/// Whether to open the next page, and if not, which stop fired. +/// +/// The budget is checked first because it is a hard bound, and the two stops +/// are kept apart because they mean opposite things about what the walk did +/// not look at: past the edge, the rest of the listing is known to be in +/// hand; out of budget, it is simply unseen. +fn step(opened: usize, streak: usize, gap: Gap) -> Step { + if opened >= PROBE_BUDGET { + Step::BudgetSpent + } else if gap == Gap::Suffix && streak >= KNOWN_STREAK_STOP { + Step::PastTheEdge + } else { + Step::Probe + } +} + +/// How many of the listing's `total` rows a stop at `opened` leaves +/// unexamined — which is how many pulls this walk may be silently short. +/// +/// Zero past the edge, where the remainder is proven to be in hand, and zero +/// when the loop simply ran out of rows. Non-zero only for the budget, which +/// is the one stop that ends a walk without an argument that it was finished. +fn unexamined_at(total: usize, opened: usize, stop: Step) -> usize { + match stop { + Step::BudgetSpent => total.saturating_sub(opened), + Step::Probe | Step::PastTheEdge => 0, + } +} + /// The pulls a repo's web listings show that `known` does not contain, shaped /// like Bobbin listing items: `uri`, `state`, and the record under `value`. /// @@ -65,32 +139,51 @@ pub(crate) struct Backfill { /// settle, at no extra cost. Only what the walk happened to visit is /// here; an unresolved state is still honestly unknown. pub states: std::collections::HashMap, + /// How many pulls the web listing showed that this walk never opened, + /// because [`PROBE_BUDGET`] ran out first. + /// + /// The whole reason it is here: without it a walk that stopped eight + /// pages into a sixteen-pull repo returned a short listing shaped + /// exactly like a complete one, and the only thing that could have said + /// otherwise — the staleness warning — fires on a disagreement with + /// Bobbin that under `--source web` was never even asked for. A walk + /// that stopped short says so itself. + pub unexamined: usize, } -pub(crate) async fn backfill_pulls(web_url: &str, known: &HashSet) -> Backfill { +/// Walk the repo's web listings for the pulls `known` does not hold. +/// +/// `gap` says what `known` is, which is what a run of already-known pulls +/// proves — see [`Gap`]. What the walk did not get to comes back in +/// [`Backfill::unexamined`] rather than being left to look like an empty +/// remainder. +pub(crate) async fn backfill_pulls(web_url: &str, known: &HashSet, gap: Gap) -> Backfill { let probes = newest_first(listed_pulls(web_url).await); crate::logging::debug::log(format!( - "web-index backfill: {} pulls listed on {web_url}, probing up to {PROBE_BUDGET}", + "web-index backfill: {} pulls listed on {web_url}, probing up to {PROBE_BUDGET} ({gap:?})", probes.len() )); let mut found = Vec::new(); let mut states = std::collections::HashMap::new(); let mut streak = 0; + let mut unexamined = 0; for (opened, (number, state)) in probes.iter().enumerate() { - if opened == PROBE_BUDGET { - // No silent caps: say the walk stopped short of the listing. - crate::logging::debug::log(format!( - "web-index backfill: probe budget spent with {} listed pulls unexamined", - probes.len() - opened - )); - break; - } - if streak >= KNOWN_STREAK_STOP { - crate::logging::debug::log(format!( - "web-index backfill: {KNOWN_STREAK_STOP} known pulls in a row, \ - the index's edge is behind #{number}" - )); + let stop = step(opened, streak, gap); + if stop != Step::Probe { + unexamined = unexamined_at(probes.len(), opened, stop); + // No silent caps: say what the walk stopped short of, and which + // stop it was. + crate::logging::debug::log(match stop { + Step::BudgetSpent => format!( + "web-index backfill: probe budget spent with {unexamined} listed pulls \ + unexamined" + ), + _ => format!( + "web-index backfill: {KNOWN_STREAK_STOP} known pulls in a row, \ + the index's edge is behind #{number}" + ), + }); break; } // A page that will not parse is not evidence in either direction, so @@ -114,6 +207,7 @@ pub(crate) async fn backfill_pulls(web_url: &str, known: &HashSet) -> Ba Backfill { items: found, states, + unexamined, } } @@ -251,7 +345,7 @@ async fn record_from_pds(did: &str, rkey: &str) -> Option { #[cfg(test)] mod tests { - use super::{item, newest_first}; + use super::{Gap, Step, item, newest_first, step, unexamined_at}; use serde_json::json; /// The walk spends its budget newest-first — the stalled end of the repo @@ -312,4 +406,57 @@ mod tests { assert_eq!(shaped["value"]["title"].as_str(), Some("t")); assert!(shaped.get("commentCount").is_none()); } + + /// A walk that ran out of budget is short, and must say by how much. + /// + /// This is the "sixteen pulls reported as six" failure inside the + /// mechanism built to prevent it: the probe budget bounds how many pull + /// pages one backfill opens, so a repo with more other-author pulls than + /// that came back capped. Nothing announced it — the staleness warning + /// speaks only for a disagreement with Bobbin, and under `--source web` + /// Bobbin is never asked — so the listing read as complete. The edge stop + /// is the one stop that may stay silent, because there the remainder is + /// proven to be in the caller's hands already. + #[test] + fn a_spent_budget_is_a_truncation_and_a_crossed_edge_is_not() { + assert_eq!(step(0, 0, Gap::Suffix), Step::Probe); + assert_eq!(step(super::PROBE_BUDGET, 0, Gap::Suffix), Step::BudgetSpent); + assert_eq!( + step( + super::KNOWN_STREAK_STOP, + super::KNOWN_STREAK_STOP, + Gap::Suffix + ), + Step::PastTheEdge + ); + + // Sixteen listed, eight opened: eight unexamined, and the caller is + // owed that number. + assert_eq!(unexamined_at(16, super::PROBE_BUDGET, Step::BudgetSpent), 8); + assert_eq!(unexamined_at(16, 4, Step::PastTheEdge), 0); + assert_eq!(unexamined_at(6, 6, Step::Probe), 0); + // A listing no longer than the budget is examined whole. + assert_eq!(unexamined_at(6, 6, Step::BudgetSpent), 0); + } + + /// Without an index beside it the streak stop is not an edge at all. + /// + /// The stop rests on `known` being a stale index's listing, which is + /// complete below the point the index died at — so three known pulls in + /// a row mean the walk has crossed the gap. Under `--source web` alone + /// `known` is the reader's own pulls and nothing else, and three of those + /// in a row are just three of the reader's own pulls with every other + /// author's still unopened below them. Stopping there would be the same + /// short read passed off as a whole listing, one stop over. + #[test] + fn the_edge_stop_needs_an_index_to_be_an_edge() { + let past_the_streak = super::KNOWN_STREAK_STOP + 1; + assert_eq!(step(3, past_the_streak, Gap::Suffix), Step::PastTheEdge); + assert_eq!(step(3, past_the_streak, Gap::Unbounded), Step::Probe); + // The budget still bounds it, and still reports what it left. + assert_eq!( + step(super::PROBE_BUDGET, past_the_streak, Gap::Unbounded), + Step::BudgetSpent + ); + } } diff --git a/src/cmd/pr/read/sources.rs b/src/cmd/pr/read/sources.rs index 2dac98b..0228405 100644 --- a/src/cmd/pr/read/sources.rs +++ b/src/cmd/pr/read/sources.rs @@ -1069,7 +1069,12 @@ pub(super) async fn gather(ask: Ask<'_>) -> Result { (true, None) => (HashMap::new(), false), }; - let mut items = merge(pds_records, bobbin_items, &own_states, ownership.own_records_win()); + let mut items = merge( + pds_records, + bobbin_items, + &own_states, + ownership.own_records_win(), + ); // An unanswered question is not a disagreement. Under `--source pds`, or // after a Bobbin request that failed, every PDS record would otherwise // look "missing from the index" and the warning would fire on evidence @@ -1091,13 +1096,31 @@ pub(super) async fn gather(ask: Ask<'_>) -> Result { { let known: std::collections::HashSet = items.iter().map(|i| i.uri.clone()).collect(); - let extra = crate::clients::tangled::web::backfill::backfill_pulls(&web_url, &known).await; + // What `known` is decides what the walk's early stop proves. Beside + // Bobbin it is a stale index's listing, complete below the point it + // died at; without Bobbin it is this account's own pulls and nothing + // else, and a run of them says nothing about anybody else's. + let gap = match source.uses_bobbin() { + true => crate::clients::tangled::web::backfill::Gap::Suffix, + false => crate::clients::tangled::web::backfill::Gap::Unbounded, + }; + let extra = + crate::clients::tangled::web::backfill::backfill_pulls(&web_url, &known, gap).await; backfilled = extra.items.len(); apply_backfill_states(&mut items, &extra.states); append_backfilled(&mut items, extra.items); + // Said here rather than left to `warn_stale`, which returns without a + // word when `missing` is empty — and `missing` is only computed when + // Bobbin was asked, which under `--source web` alone it never is. + // A scrape that stopped short is the listing's own business. + warn_web_truncated(extra.unexamined); } - settle_own_open(&mut items, did, ownership.settles_absence() && states_complete); + settle_own_open( + &mut items, + did, + ownership.settles_absence() && states_complete, + ); Ok(Gathered { items, @@ -1196,6 +1219,41 @@ fn should_backfill( && (!source.uses_bobbin() || missing > 0 || bobbin_failed_softly) } +/// Say that the web-index walk stopped short of the listing it read. +/// +/// Its own sentence, and on its own evidence. [`warn_stale`] speaks for a +/// disagreement between the PDS and Bobbin, so it says nothing at all when +/// there is no Bobbin answer to disagree with — which is exactly the case +/// where this scrape is the only cross-account source there is. A repo with +/// more than [`crate::clients::tangled::web::backfill`]'s probe budget of +/// other authors' pulls used to print the newest few of them under no +/// warning whatsoever, with the PDS-side evidence reading complete because +/// the PDS walk genuinely was. +fn warn_web_truncated(unexamined: usize) { + let Some(sentence) = web_truncation(unexamined) else { + return; + }; + crate::term::say::warning!(Index, "{sentence}"); +} + +/// The sentence for a truncated web walk, or nothing to say. +/// +/// Split from the printing because the judgement is the whole of it: a walk +/// that examined everything the listing showed has no admission to make, and +/// one that did not must say how much it left rather than let a short +/// listing pass for a whole one. +fn web_truncation(unexamined: usize) -> Option { + if unexamined == 0 { + return None; + } + Some(format!( + "the tangled.org web index listed {unexamined} more pull request(s) than this \n\ + read opened, so other contributors' pulls are missing from this listing.\n\ + The walk is budgeted because each pull page runs to megabytes; narrow the repo \n\ + or read the listing at the web index itself." + )) +} + /// The repo's web root, for the backfill and nothing else. /// /// Derived from the repo DID by the same redirect probe `pr list --all` uses, @@ -1494,10 +1552,13 @@ fn newest<'a>(items: impl Iterator) -> Option<&'a #[cfg(test)] mod tests { use super::super::fixtures::{ATGC_REPO, old_item, pds_pulls, pds_statuses}; + use super::Ownership; use super::latest_states; + use super::needs_status_walk; use super::past_the_floor; use super::require_branch_pull; use super::targets_repo; + use super::web_truncation; use super::{Datetime, FromStr}; use super::{Empty, Listed, Source, State, classify_empty}; use super::Ownership; @@ -1891,6 +1952,27 @@ mod tests { assert_eq!(row.state, State::Known("open".into())); } + /// A capped web walk gets its own warning, on its own evidence. + /// + /// `warn_stale` returns without a word when nothing is missing from + /// Bobbin, and `missing` is only computed when Bobbin was asked — which + /// under `--source web` alone it never is. So the one place that could + /// have mentioned a capped scrape was structurally silent exactly where + /// the scrape was the only cross-account source, and `pr list --source + /// web` on a repo with more than the probe budget of other authors' + /// pulls printed the newest few of them as if that were the listing. + /// This sentence depends on the walk and on nothing else. + #[test] + fn a_capped_web_walk_says_so_without_asking_bobbin() { + assert_eq!(web_truncation(0), None, "a whole walk has nothing to admit"); + let short = web_truncation(8).expect("a capped walk owes the reader a sentence"); + assert!(short.contains("8 more pull request(s)"), "{short}"); + assert!( + short.contains("missing from this listing"), + "the sentence has to say what it costs, not only what it did: {short}" + ); + } + /// An ownership lookup that failed must not send the status walk home. /// /// The walk is skipped when every own pull already carries a state from @@ -1920,7 +2002,11 @@ mod tests { "a lookup that could not answer leaves the merge preferring \ records this walk is the only thing that fetches" ); - assert!(needs_status_walk(&pds, &bobbin, Ownership::from_lookup(Some(true)))); + assert!(needs_status_walk( + &pds, + &bobbin, + Ownership::from_lookup(Some(true)) + )); // On a repo the account provably does not own, the index really is // better informed and the skip is the whole point of it. assert!(!needs_status_walk( @@ -1933,7 +2019,11 @@ mod tests { // join onto, whatever the lookup said. assert!(!needs_status_walk(&[], &bobbin, unsettled)); // And a pull the index did not answer for needs the walk regardless. - assert!(needs_status_walk(&pds, &[], Ownership::from_lookup(Some(false)))); + assert!(needs_status_walk( + &pds, + &[], + Ownership::from_lookup(Some(false)) + )); } /// …unless the account read is both authorities, in which case the