From 20161eb2a6b1f9bd2b116cba0d6aa45a3685653e Mon Sep 17 00:00:00 2001 From: "@permadeath.com" Date: Wed, 26 Aug 2026 22:18:45 -0400 Subject: [PATCH] fix(pr): read a failed ownership lookup as unsettled, not as a denial MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `owns_repo` returns `Err` when the PDS will not resolve or a listRecords page runs out of retries, and `unwrap_or(false)` turned that into "this is not your repo" — handing a stale index precedence over the account's own status records and skipping the status walk that would have read them. Ownership is now three-valued, and "could not tell" keeps the own-records-win rule while leaving the absent-record-means-open inference off. Change-Id: I03e62a04725fd6cc7137c361f85133d8d4f048d5 --- src/clients/tangled/ownership.rs | 105 +++++++++++++++++++++++++- src/cmd/pr/read/sources.rs | 125 ++++++++++++++++++++++++------- 2 files changed, 201 insertions(+), 29 deletions(-) diff --git a/src/clients/tangled/ownership.rs b/src/clients/tangled/ownership.rs index 019dd23..cb4fd6a 100644 --- a/src/clients/tangled/ownership.rs +++ b/src/clients/tangled/ownership.rs @@ -24,7 +24,10 @@ use anyhow::Result; /// A walk that runs out of budget answers `false`, which is the safe /// direction in both callers and worth naming rather than leaving implied: a /// listing then keeps `?` instead of asserting `open`, and a close is refused -/// instead of being written where it would be dropped. The budget is +/// instead of being written where it would be dropped. That argument is about +/// a *complete* walk that found nothing, and it does not extend to the `Err` +/// below — a PDS that would not resolve has not found anything either way. +/// Readers that cannot act on an error want [`ownership`] instead. The budget is /// [`crate::clients::atproto::pds::MAX_PAGES_COMPLETE`] rather than a /// screenful, because the conclusion drawn here is drawn from *absence* and /// an early stop looks exactly like an account that owns nothing. @@ -63,9 +66,81 @@ fn names_repo(record: &serde_json::Value, repo_did: &str) -> bool { record["value"]["repoDid"].as_str() == Some(repo_did) } +/// What one ownership lookup came back with — including "could not tell". +/// +/// Two consumers want opposite defaults from the same absence, which is why +/// one boolean could not serve both. `cmd::pr::read`'s merge asks so it can +/// decide whether the account's own status records outrank the index; a +/// listing asks so it can decide whether an *absent* status record means +/// open. A lookup that failed — an unresolvable PDS, a `listRecords` page +/// that ran out of retries — must keep the first and withhold the second, +/// and `unwrap_or(false)` handed it the wrong answer to both. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +pub enum Ownership { + /// The actor has a `sh.tangled.repo` record naming this repo DID. + Owns, + /// A complete walk of the actor's repo records found no such record. + Foreign, + /// The walk could not be completed, so this is not an answer at all. + Unsettled, +} + +impl Ownership { + /// Read a lookup's answer, where `None` is a lookup that failed. + /// + /// Split from the request so the mapping — the whole of the judgement — + /// is decided over data rather than over a socket. + pub fn from_lookup(answer: Option) -> Self { + match answer { + Some(true) => Self::Owns, + Some(false) => Self::Foreign, + None => Self::Unsettled, + } + } + + /// Whether the account's own status records outrank an index that + /// disagrees with them. + /// + /// True when the account is known to be the repo's owner — it is then + /// both accounts Tangled honours a status record from, so there is no + /// third party left to be better informed by — and true again when + /// ownership could not be settled, because the rule this protects was + /// added after `pr list` printed `open` over two pulls this account had + /// closed, and a failed lookup is no reason to reopen that hole. + pub fn own_records_win(self) -> bool { + matches!(self, Self::Owns | Self::Unsettled) + } + + /// Whether a *missing* status record can be read as "nobody has acted on + /// this pull, so it is open". + /// + /// Only on a settled `Owns`. That inference needs to have seen every + /// status record that could exist, which is a claim only the repo's owner + /// reading their own PDS can make — and a lookup that could not answer + /// has not made it. The pull keeps `?`. + pub fn settles_absence(self) -> bool { + matches!(self, Self::Owns) + } +} + +/// Ownership as the listings want it: three-valued, and never fatal. +/// +/// [`owns_repo`]'s `Err` is a failure to look, not a finding, and every +/// caller here has a listing to print either way. The error is logged rather +/// than warned about — it costs precision in one column, not the answer. +pub async fn ownership(actor_did: &str, repo_did: &str) -> Ownership { + let answer = owns_repo(actor_did, repo_did).await; + if let Err(e) = &answer { + crate::logging::debug::log(format!( + "could not settle whether {actor_did} owns {repo_did}: {e:#}" + )); + } + Ownership::from_lookup(answer.ok()) +} + #[cfg(test)] mod tests { - use super::names_repo; + use super::{Ownership, names_repo}; use serde_json::json; const REPO: &str = "did:plc:gspkabpde4kx47fj3bhiwrms"; @@ -97,4 +172,30 @@ mod tests { assert!(!names_repo(&json!({ "value": value }), REPO)); } } + + /// A lookup that could not be completed is not a denial. `owns_repo` + /// returns `Err` when the PDS will not resolve or a `listRecords` page + /// runs out of retries, and the merge in `cmd::pr::read` used to take + /// `unwrap_or(false)` from it — which handed the index precedence over + /// the account's own status records and re-created the bug that rule was + /// added to fix, two pulls this account had closed reading back `open`. + /// "Could not tell" keeps the own-records-win rule and withholds the + /// absence inference; only a settled answer moves either. + #[test] + fn a_lookup_that_could_not_answer_is_not_a_denial() { + assert_eq!(Ownership::from_lookup(None), Ownership::Unsettled); + assert!(Ownership::from_lookup(None).own_records_win()); + assert!(!Ownership::from_lookup(None).settles_absence()); + + assert_eq!(Ownership::from_lookup(Some(true)), Ownership::Owns); + assert!(Ownership::from_lookup(Some(true)).own_records_win()); + assert!(Ownership::from_lookup(Some(true)).settles_absence()); + + // The one answer that hands the index the state field: a repo this + // account provably does not own, where a maintainer's close is a + // record in a PDS this read never saw. + assert_eq!(Ownership::from_lookup(Some(false)), Ownership::Foreign); + assert!(!Ownership::from_lookup(Some(false)).own_records_win()); + assert!(!Ownership::from_lookup(Some(false)).settles_absence()); + } } diff --git a/src/cmd/pr/read/sources.rs b/src/cmd/pr/read/sources.rs index 781b411..2dac98b 100644 --- a/src/cmd/pr/read/sources.rs +++ b/src/cmd/pr/read/sources.rs @@ -16,6 +16,7 @@ use super::labels::{appview_url, repo_label_from_appview}; use crate::clients::atproto::pds::{Evidence, Listing, Reach}; use crate::clients::git::run as git; +use crate::clients::tangled::ownership::Ownership; use crate::clients::tangled::resolve; use crate::lexicon::tangled::PullState; use anyhow::Result; @@ -551,13 +552,17 @@ fn past_the_floor(so_far: &[serde_json::Value], floor: Option<&Datetime>) -> boo /// the PDS when it is there, because that copy came from the account that /// owns it rather than from an index of it. /// -/// `own_repo` is the one case where "written by people whose PDSes are not -/// being read here" is false, and it turns the state rule around. Bobbin +/// `own_records_win` is the one case where "written by people whose PDSes are +/// not being read here" is false, and it turns the state rule around. Bobbin /// accepts a status record from exactly two accounts — the pull's author and /// the target repo's owner — so when the account whose PDS was read is both, /// there is no third party left to be better informed by. Its own records are /// the whole of what Bobbin will eventually say, and a disagreement means -/// Bobbin has not caught up yet. +/// Bobbin has not caught up yet. It is +/// [`Ownership::own_records_win`](crate::clients::tangled::ownership::Ownership::own_records_win) +/// and so it is also true when ownership could not be settled at all: the +/// argument for handing the index this field rests on knowing the account is +/// *not* the owner, and a lookup that failed knows nothing of the kind. /// /// That disagreement is not hypothetical: two pulls this account closed and /// re-created read back `open` for hours, because Bobbin's index still had @@ -569,7 +574,7 @@ fn merge( pds: Vec, bobbin: Vec, own_states: &HashMap, - own_repo: bool, + own_records_win: bool, ) -> Vec { let mut merged: Vec = Vec::with_capacity(pds.len() + bobbin.len()); let mut seen: HashMap = HashMap::new(); @@ -606,7 +611,7 @@ fn merge( let slot = &mut merged[at]; slot.indexed = true; slot.comments = comments; - let settled_here = own_repo && matches!(slot.state, State::Known(_)); + let settled_here = own_records_win && matches!(slot.state, State::Known(_)); if let State::Known(_) = state && !settled_here { @@ -1030,28 +1035,19 @@ pub(super) async fn gather(ask: Ask<'_>) -> Result { // asking per repo would cost a listing each. See [`merge`] for what the // answer buys, and `clients/tangled/ownership.rs` for why it is not an // index question. - let own_repo = match (did, repo_filter) { - (Some(did), Some(repo)) => crate::clients::tangled::ownership::owns_repo(did, repo) - .await - .unwrap_or(false), - _ => false, + // + // Three-valued on purpose. The lookup fails whenever the actor's PDS will + // not resolve or a `listRecords` page runs out of retries, and reading + // that absence as "not your repo" handed a stale index precedence over + // the account's own status records — the exact bug [`merge`]'s rule + // exists to prevent, re-created by a failed request. + let ownership = match (did, repo_filter) { + (Some(did), Some(repo)) => crate::clients::tangled::ownership::ownership(did, repo).await, + _ => Ownership::Foreign, }; - // Only worth a request when something needs a state Bobbin did not - // supply. In the healthy case — index caught up, every pull matched — - // this is skipped and the whole command costs what it always did. - // - // Except on the account's own repo, where the walk is no longer filling - // gaps in Bobbin's answer but checking it: those records outrank the - // index, so skipping the read because Bobbin had *something* to say is - // how a stale `open` got printed over a close the account itself wrote. - let needs_states = !pds_records.is_empty() - && (own_repo - || pds_records.iter().any(|record| { - record["uri"] - .as_str() - .is_some_and(|uri| !bobbin_items.iter().any(|i| i["uri"].as_str() == Some(uri))) - })); + // See [`needs_status_walk`] for when this is worth a request. + let needs_states = needs_status_walk(&pds_records, &bobbin_items, ownership); // The oldest pull in hand is what bounds the status walk — see // [`pds_states`]. Taken from the PDS records alone because those are the // only ones an own status record is joined onto; a Bobbin-only row @@ -1073,7 +1069,7 @@ pub(super) async fn gather(ask: Ask<'_>) -> Result { (true, None) => (HashMap::new(), false), }; - let mut items = merge(pds_records, bobbin_items, &own_states, own_repo); + 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 @@ -1101,7 +1097,7 @@ pub(super) async fn gather(ask: Ask<'_>) -> Result { append_backfilled(&mut items, extra.items); } - settle_own_open(&mut items, did, own_repo && states_complete); + settle_own_open(&mut items, did, ownership.settles_absence() && states_complete); Ok(Gathered { items, @@ -1112,6 +1108,34 @@ pub(super) async fn gather(ask: Ask<'_>) -> Result { }) } +/// Whether the account's own `sh.tangled.repo.pull.status` records are worth +/// a request. +/// +/// Normally only when something needs a state Bobbin did not supply: in the +/// healthy case — index caught up, every pull matched — the walk is skipped +/// and the command costs what it always did. +/// +/// The exception is every case where the account's own records outrank the +/// index. There the walk is not filling gaps in Bobbin's answer but checking +/// it, so skipping it because Bobbin had *something* to say is how a stale +/// `open` got printed over a close the account itself wrote. That includes +/// [`Ownership::Unsettled`]: a lookup that could not answer left [`merge`] +/// ready to prefer records this would decline to fetch, which is the same +/// stale `open` by a longer route. +fn needs_status_walk( + pds_records: &[serde_json::Value], + bobbin_items: &[serde_json::Value], + ownership: Ownership, +) -> bool { + !pds_records.is_empty() + && (ownership.own_records_win() + || pds_records.iter().any(|record| { + record["uri"] + .as_str() + .is_some_and(|uri| !bobbin_items.iter().any(|i| i["uri"].as_str() == Some(uri))) + })) +} + /// On your own repo, your own pull with no status record anywhere is open. /// /// Not a guess — the same argument [`merge`] makes for letting your records @@ -1476,6 +1500,8 @@ mod tests { use super::targets_repo; use super::{Datetime, FromStr}; use super::{Empty, Listed, Source, State, classify_empty}; + use super::Ownership; + use super::needs_status_walk; use super::{append_backfilled, merge, missing_from_bobbin, newest, newest_missing}; use super::{apply_state_filter, for_branch, settle_own_open, should_backfill, unknown_states}; use serde_json::{Value, json}; @@ -1865,6 +1891,51 @@ mod tests { assert_eq!(row.state, State::Known("open".into())); } + /// 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 + /// the index — which is safe only where the index outranks the account's + /// own records. It does not, wherever those records win, and they win on + /// an unsettled lookup as well as on a settled `Owns`. Reading a failed + /// lookup as `Foreign` skipped the walk for every pull Bobbin returned, + /// so a close written into this account's own PDS was never read at all + /// and `pr list` printed Bobbin's `open` over it. + #[test] + fn a_failed_ownership_lookup_still_reads_your_own_status_records() { + let pds: Vec = pds_pulls() + .into_iter() + .filter(|r| targets_repo(r, ATGC_REPO)) + .collect(); + // The healthy shape the skip was written for: the index answered for + // every one of the account's own pulls, so nothing below is waiting + // on a record. + let bobbin: Vec = pds + .iter() + .map(|r| json!({ "uri": r["uri"], "state": "open", "value": r["value"] })) + .collect(); + + let unsettled = Ownership::from_lookup(None); + assert!( + needs_status_walk(&pds, &bobbin, unsettled), + "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)))); + // 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( + &pds, + &bobbin, + Ownership::from_lookup(Some(false)) + )); + + // What the skip is still allowed to be: nothing read, nothing to + // 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)))); + } + /// …unless the account read is both authorities, in which case the /// absence *is* the answer and `?` becomes open. /// -- 2.51.2