From a6ba1b010320d20677fd5ccc14c2a4f7cd6b0ca4 Mon Sep 17 00:00:00 2001
From: Fintan Halpenny <fintan.halpenny@gmail.com>
Date: Fri, 15 Dec 2023 13:41:51 +0000
Subject: [PATCH] node: inspect outdated sigrefs test

This test is flakey and requires further investigation. Some extra
assertions are added to ensure that the expected behaviour is
happening upon fetching. It is unknown if the fetch queue is behaving
properly, and it's suspected that some existing fetch results might be
used for queued fetches.

Signed-off-by: Fintan Halpenny <fintan.halpenny@gmail.com>
X-Clacks-Overhead: GNU Terry Pratchett
---
 radicle-node/src/service.rs   |  6 ++---
 radicle-node/src/tests/e2e.rs | 43 +++++++++++++++++++++++++++++++----
 radicle/src/node.rs           |  9 ++++++++
 radicle/src/storage.rs        |  9 ++++++++
 4 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/radicle-node/src/service.rs b/radicle-node/src/service.rs
index fd5b693f..87dacb17 100644
--- a/radicle-node/src/service.rs
+++ b/radicle-node/src/service.rs
@@ -901,12 +901,12 @@ where
         }
 
         for sub in &fetching.subscribers {
-            debug!(target: "service", "Found existing fetch request, sending result..");
+            debug!(target: "service", "Found existing fetch request from {remote}, sending result..");
 
             if sub.send(result.clone()).is_err() {
-                error!(target: "service", "Error sending fetch result for {rid}..");
+                error!(target: "service", "Error sending fetch result for {rid} from {remote}..");
             } else {
-                debug!(target: "service", "Sent fetch result for {rid}..");
+                debug!(target: "service", "Sent fetch result for {rid} from {remote}..");
             }
         }
 
diff --git a/radicle-node/src/tests/e2e.rs b/radicle-node/src/tests/e2e.rs
index a9c20dbc..fe5ed3db 100644
--- a/radicle-node/src/tests/e2e.rs
+++ b/radicle-node/src/tests/e2e.rs
@@ -842,7 +842,24 @@ fn test_non_fastforward_sigrefs() {
     // Eve fetches the inital project from Bob.
     eve.handle.fetch(rid, bob.id, DEFAULT_TIMEOUT).unwrap();
     // Alice fetches it too.
-    alice.handle.fetch(rid, bob.id, DEFAULT_TIMEOUT).unwrap();
+    let old_bob = alice.handle.fetch(rid, bob.id, DEFAULT_TIMEOUT).unwrap();
+    let bob_sigrefs = bob
+        .storage
+        .repository(rid)
+        .unwrap()
+        .reference_oid(&bob.id, &radicle::storage::refs::SIGREFS_BRANCH)
+        .unwrap();
+    let up = old_bob
+        .find_updated(
+            &(*radicle::storage::refs::Special::SignedRefs.namespaced(&bob.id)).to_ref_string(),
+        )
+        .unwrap();
+    let old_bob = match up {
+        RefUpdate::Created { oid, .. } => oid,
+        RefUpdate::Skipped { oid, .. } => oid,
+        _ => panic!("rad/sigrefs should have been created or skipped: {:?}", up),
+    };
+    assert_eq!(bob_sigrefs, old_bob);
 
     // Log the before Oid value of bob's 'rad/sigrefs', for debugging purposes.
     {
@@ -867,7 +884,25 @@ fn test_non_fastforward_sigrefs() {
         "Updated sigrefs are harshing my vibes",
     );
     // Alice fetches from Bob.
-    alice.handle.fetch(rid, bob.id, DEFAULT_TIMEOUT).unwrap();
+    let new_bob = alice.handle.fetch(rid, bob.id, DEFAULT_TIMEOUT).unwrap();
+    let bob_sigrefs = bob
+        .storage
+        .repository(rid)
+        .unwrap()
+        .reference_oid(&bob.id, &radicle::storage::refs::SIGREFS_BRANCH)
+        .unwrap();
+    let up = new_bob
+        .find_updated(
+            &(*radicle::storage::refs::Special::SignedRefs.namespaced(&bob.id)).to_ref_string(),
+        )
+        .unwrap();
+    let new_bob = match up {
+        RefUpdate::Updated { new, .. } => new,
+        // FIXME: Really it shouldn't be skipped but let's see what happens
+        RefUpdate::Skipped { oid, .. } => oid,
+        _ => panic!("rad/sigrefs should have been updated {:?}", up),
+    };
+    assert_eq!(bob_sigrefs, new_bob);
 
     // Log the after Oid value of bob's 'rad/sigrefs', for debugging purposes.
     {
@@ -877,11 +912,9 @@ fn test_non_fastforward_sigrefs() {
             .unwrap()
             .reference_oid(&bob.id, &radicle::storage::refs::SIGREFS_BRANCH)
             .unwrap();
-        log::debug!(target: "test", "bob's old 'rad/sigrefs': {}", after);
+        log::debug!(target: "test", "bob's new 'rad/sigrefs': {}", after);
     }
 
-    // Now Alice has the latest, and when she tries to fetch from Eve, it breaks because
-    // Eve has old refs.
     assert_matches!(
         alice.handle.fetch(rid, eve.id, DEFAULT_TIMEOUT).unwrap(),
         FetchResult::Success { updated, .. } if updated.is_empty()
diff --git a/radicle/src/node.rs b/radicle/src/node.rs
index d53a25f5..ea7b5b72 100644
--- a/radicle/src/node.rs
+++ b/radicle/src/node.rs
@@ -24,6 +24,7 @@ use serde::{Deserialize, Serialize};
 use serde_json as json;
 
 use crate::crypto::PublicKey;
+use crate::git;
 use crate::identity::Id;
 use crate::profile;
 use crate::storage::refs::RefsAt;
@@ -655,6 +656,14 @@ impl FetchResult {
             _ => None,
         }
     }
+
+    pub fn find_updated(&self, name: &git::RefString) -> Option<RefUpdate> {
+        let updated = match self {
+            Self::Success { updated, .. } => Some(updated),
+            _ => None,
+        }?;
+        updated.iter().find(|up| up.name() == name).cloned()
+    }
 }
 
 impl<S: ToString> From<Result<(Vec<RefUpdate>, HashSet<NodeId>), S>> for FetchResult {
diff --git a/radicle/src/storage.rs b/radicle/src/storage.rs
index a1a571ff..3e77b241 100644
--- a/radicle/src/storage.rs
+++ b/radicle/src/storage.rs
@@ -170,6 +170,15 @@ impl RefUpdate {
             Self::Skipped { name, oid: old }
         }
     }
+
+    pub fn name(&self) -> &RefString {
+        match &self {
+            RefUpdate::Updated { name, .. } => name,
+            RefUpdate::Created { name, .. } => name,
+            RefUpdate::Deleted { name, .. } => name,
+            RefUpdate::Skipped { name, .. } => name,
+        }
+    }
 }
 
 impl fmt::Display for RefUpdate {