fetch: Improve `refs/rad/id` resolution

The previous resolution would favour one `Oid` over another, due to
the call to `Option::or`.

This was found to result in scenarios where an identity document being
advertised was older than the identity document that the fetching node
already has, and would not fetch namespaces by newly added delegates.

The resolution logic is updated to handle all cases for these `Oid`s,
and when they are both present, uses a commit graph check to see which
one is the latest.

An e2e test is added to check ensure behaviour works correctly.

crates/radicle-fetch/src/state.rs

@@ -87,6 +87,8 @@ pub mod error {
         Resolve(#[from] git::repository::error::Resolve),
         #[error(transparent)]
         Verified(#[from] radicle::identity::DocError),
+        #[error("failed to verify `refs/rad/id`: {0}")]
+        Graph(#[source] radicle::git::raw::Error),
     }
 }
 
@@ -637,13 +639,48 @@ where
         self.handle.verified(head)
     }
 
+    /// Resolve the verified [`Doc`], by choosing a `refs/rad/id` head to
+    /// resolve from.
+    ///
+    /// There are two candidate namespaces:
+    ///
+    ///   1. Of the fetching node.
+    ///   2. Of the node being fetched from.
+    ///
+    /// Both might be unset, in this case [`None`] is returned.
+    ///
+    /// If exactly one of the two is set, it is used.
+    ///
+    /// Otherwise, the ahead/behind relationship between the two candidates
+    /// is checked, and (2.) is used if it is ahead of (1.).
     pub fn canonical(&self) -> Result<Option<Doc>, error::Canonical> {
         let tip = self.refname_to_id(refs::REFS_RAD_ID.clone())?;
         let cached_tip = self.canonical_rad_id();
 
-        tip.or(cached_tip)
-            .map(|tip| self.verified(tip).map_err(error::Canonical::from))
-            .transpose()
+        let oid = match (tip, cached_tip) {
+            (None, None) => {
+                return Ok(None);
+            }
+            (Some(oid), None) | (None, Some(oid)) => oid,
+            (Some(repository), Some(cached)) => {
+                let repo = self.handle.repository();
+                match repo
+                    .backend
+                    .graph_ahead_behind(repository.into(), cached.into())
+                {
+                    Ok((ahead, behind)) => match (ahead, behind) {
+                        (0, _) => cached,
+                        _ => repository,
+                    },
+                    Err(err) if err.code() == radicle::git::raw::ErrorCode::NotFound => repository,
+                    Err(err) => {
+                        return Err(error::Canonical::Graph(err));
+                    }
+                }
+            }
+        };
+
+        self.verified(oid).map(Some).map_err(error::Canonical::from)
     }
 
     pub fn load(&self, remote: &PublicKey) -> Result<Option<SignedRefsAt>, sigrefs::error::Load> {

crates/radicle-node/src/tests/e2e.rs

@@ -1,6 +1,8 @@
 use std::{collections::HashSet, thread, time};
 
+use radicle::cob;
 use radicle::cob::Title;
+use radicle_crypto::test::signer::MockSigner;
 use test_log::test;
 
 use radicle::git::raw::ErrorExt as _;
@@ -20,7 +22,7 @@ use crate::node::config::Limits;
 use crate::node::{Config, ConnectOptions};
 use crate::service;
 use crate::storage::git::transport;
-use crate::test::node::{converge, Node};
+use crate::test::node::{converge, Node, NodeHandle};
 
 mod config {
     use super::*;
@@ -1559,3 +1561,97 @@ fn test_fetch_emits_canonical_ref_update() {
         )
         .unwrap();
 }
+
+#[test]
+fn test_non_fastword_identity_doc() {
+    use radicle::identity::Identity;
+
+    let tmp = tempfile::tempdir().unwrap();
+
+    let mut alice = Node::init(tmp.path(), Config::test(Alias::new("alice")));
+    let bob = Node::init(tmp.path(), Config::test(Alias::new("bob")));
+    let eve = Node::init(tmp.path(), Config::test(Alias::new("eve")));
+    let alice_laptop = Node::init(tmp.path(), Config::test(Alias::new("alice-laptop")));
+
+    let rid = alice.project("acme", "");
+
+    let mut alice = alice.spawn();
+    let mut alice_laptop = alice_laptop.spawn();
+    let mut bob = bob.spawn();
+    let mut eve = eve.spawn();
+
+    let has_issue = |node: &NodeHandle<MockSigner>, issue: &cob::ObjectId| -> bool {
+        let repo = node.storage.repository(rid).unwrap();
+        repo.contains(**issue).unwrap()
+    };
+
+    alice.connect(&alice_laptop);
+    alice.connect(&bob);
+    alice.connect(&eve);
+    eve.connect(&bob);
+    eve.connect(&alice_laptop);
+
+    // Bob and Eve have the same state for the repository
+    bob.handle.seed(rid, Scope::Followed).unwrap();
+    bob.handle.fetch(rid, alice.id, DEFAULT_TIMEOUT).unwrap();
+
+    alice_laptop.handle.seed(rid, Scope::All).unwrap();
+    alice_laptop
+        .handle
+        .fetch(rid, alice.id, DEFAULT_TIMEOUT)
+        .unwrap();
+    // Alice pushes new references to her laptop
+    let issue = alice_laptop.issue(
+        rid,
+        "Feature #1".parse().unwrap(),
+        "Implementing new feature",
+    );
+
+    // Eve will fetch these references since her scope is "all"
+    eve.handle.seed(rid, Scope::All).unwrap();
+    eve.handle
+        .fetch(rid, alice_laptop.id, DEFAULT_TIMEOUT)
+        .unwrap();
+    assert!(has_issue(&eve, &issue));
+
+    // Alice updates the identity of the document to include her laptop
+    let (prev, next) = {
+        let repo = alice.storage.repository(rid).unwrap();
+        let mut identity = Identity::load_mut(&repo).unwrap();
+        let prev = identity.current;
+        let doc = repo
+            .identity_doc()
+            .unwrap()
+            .doc
+            .with_edits(|raw| raw.delegate(alice_laptop.id.into()))
+            .unwrap();
+        let rev = identity
+            .update(Title::new("Add Laptop").unwrap(), "", &doc, &alice.signer)
+            .unwrap();
+        repo.set_identity_head_to(rev).unwrap();
+        (prev, rev)
+    };
+
+    // Bob fetches from Alice and we see the identity document was updated.
+    //
+    // Bob does not have the issue because Alice does not have the updates from
+    // Alice's Laptop.
+    let result = bob.handle.fetch(rid, alice.id, DEFAULT_TIMEOUT).unwrap();
+    assert!(matches!(result, FetchResult::Success { .. }));
+    assert!(!has_issue(&bob, &issue));
+    let repo = bob.storage.repository(rid).unwrap();
+    let identity = Identity::load_mut(&repo).unwrap();
+    assert_eq!(identity.current, next);
+    assert_eq!(identity.parent, Some(prev));
+
+    // Bob fetches from Eve, the identity document should remain the same, but
+    // since Bob now knows that Alice's Laptop is a delegate, the issue should
+    // be fetched.
+    bob.handle.fetch(rid, eve.id, DEFAULT_TIMEOUT).unwrap();
+    assert!(matches!(result, FetchResult::Success { .. }));
+    assert!(has_issue(&bob, &issue));
+    let repo = bob.storage.repository(rid).unwrap();
+    let identity = Identity::load_mut(&repo).unwrap();
+    assert_eq!(identity.current, next);
+    assert_eq!(identity.parent, Some(prev));
+}