From 101d8ae9ebc91fba194f45ae713a0842bb7868cc Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 26 Aug 2026 22:18:45 -0400 Subject: [PATCH] fix(pr): report the web backfill's own truncation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scrape's probe budget caps how many cross-account pulls it returns, and the only thing that could have mentioned it fires on a disagreement with Bobbin that `--source web` never asks for — so a repo with more pulls than the budget printed the newest few under no warning at all. The walk now hands back what it left unopened and the listing says so, and without an index beside it the early stop no longer reads a run of your own pulls as the end of the gap. Change-Id: Ia9d168f0606522d742eb13359c0da13977bd9f18 --- src/clients/tangled/web/backfill.rs | 179 +++++++++++++++++++++++++--- src/cmd/pr/read/sources.rs | 100 +++++++++++++++- 2 files changed, 258 insertions(+), 21 deletions(-) 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 -- 2.51.2