Harden joined-group refresh test convergence logic

Author: tripledoublev

src/lib.rs

@@ -1039,72 +1039,130 @@ mod tests {
             backend.join_from_url(join_url.as_str()).await?;
         }
 
-        // Wait for replication; then retry refresh until P2P converges (same pattern as save-dweb-backend).
-        // Propagation can be slow in CI/P2P environments.
-        tokio::time::sleep(Duration::from_secs(10)).await;
-
-        // Retry refresh until P2P replication converges: the refresh must succeed AND
-        // the response must contain the creator's repo (by name) with the uploaded file.
-        // Repo names and file lists propagate via DHT and may lag behind the initial join.
-        let mut refresh_retries = 12;
+        // Wait for replication and then run convergence in two phases:
+        // A) metadata convergence (group + repo visible on joiner)
+        // B) content convergence (file downloadable on joiner)
+        // This reduces flakiness caused by asking refresh to do all work while relays are unstable.
+        tokio::time::sleep(Duration::from_secs(6)).await;
+
+        {
+            let mut retries = 20;
+            loop {
+                let groups_req = test::TestRequest::get().uri("/api/groups").to_request();
+                let groups_resp: GroupsResponse =
+                    test::call_and_read_body_json(&app, groups_req).await;
+                let repos_req = test::TestRequest::get()
+                    .uri(&format!("/api/groups/{}/repos", group.id()))
+                    .to_request();
+                let repos_resp: ReposResponse =
+                    test::call_and_read_body_json(&app, repos_req).await;
+
+                let has_group = groups_resp
+                    .groups
+                    .iter()
+                    .any(|g| g.key == group.id().to_string());
+                let has_any_repo = !repos_resp.repos.is_empty();
+                let ok = has_group && has_any_repo;
+
+                if ok {
+                    break;
+                }
+
+                retries -= 1;
+                if retries == 0 {
+                    panic!(
+                        "Refresh-joined metadata did not converge after retries. groups: {}, repos: {}",
+                        groups_resp.groups.len(),
+                        repos_resp.repos.len(),
+                    );
+                }
+                tokio::time::sleep(Duration::from_secs(5)).await;
+            }
+        };
+
+        // Retry refresh until it returns the expected repo + file view.
+        let mut refresh_retries = 20;
         let refresh_data: serde_json::Value = loop {
             let refresh_req = test::TestRequest::post()
                 .uri(&format!("/api/groups/{}/refresh", group.id()))
                 .to_request();
             let resp = test::call_service(&app, refresh_req).await;
+            let resp_status = resp.status();
 
-            if resp.status().is_success() {
+            if resp_status.is_success() {
                 let data: serde_json::Value = test::read_body_json(resp).await;
-                // Check if the expected repo with the correct name and file is present
-                let has_expected_repo = data["repos"].as_array()
-                    .and_then(|repos| repos.iter().find(|r| r["name"] == TEST_GROUP_NAME))
-                    .and_then(|repo| repo["all_files"].as_array())
-                    .map(|files| files.iter().any(|f| f.as_str() == Some(file_name)))
+                let has_repo_with_file = data["repos"]
+                    .as_array()
+                    .map(|repos| {
+                        repos.iter().any(|repo| {
+                            repo["all_files"]
+                                .as_array()
+                                .map(|files| files.iter().any(|f| f.as_str() == Some(file_name)))
+                                .unwrap_or(false)
+                        })
+                    })
                     .unwrap_or(false);
 
-                if has_expected_repo {
+                if has_repo_with_file {
                     break data;
                 }
-                log::warn!("Refresh succeeded but repo data not yet propagated (attempt {})",
-                    12 - refresh_retries + 1);
+                log::warn!(
+                    "Refresh succeeded but expected repo/file not yet visible (attempt {})",
+                    21 - refresh_retries
+                );
             } else {
-                log::warn!("Refresh failed (attempt {}): status={}, body={:?}",
-                    12 - refresh_retries + 1,
-                    resp.status(),
-                    test::read_body(resp).await
+                let body = test::read_body(resp).await;
+                let body_text = String::from_utf8_lossy(&body).to_string();
+                let is_retryable = body_text.contains("couldn't look up relay")
+                    || body_text.contains("Unable to open group DHT record")
+                    || body_text.contains("Group not found:");
+                log::warn!(
+                    "Refresh failed (attempt {}): status={}, body={}",
+                    21 - refresh_retries,
+                    resp_status,
+                    body_text
                 );
+                if !is_retryable {
+                    panic!(
+                        "Refresh failed with non-retryable error. status={resp_status}, body={body_text}"
+                    );
+                }
             }
 
             refresh_retries -= 1;
             if refresh_retries == 0 {
-                panic!("Refresh did not converge after 12 attempts.");
+                panic!("Refresh did not converge after 20 attempts.");
             }
-            tokio::time::sleep(Duration::from_secs(4)).await;
+            tokio::time::sleep(Duration::from_secs(5)).await;
         };
 
         assert_eq!(refresh_data["status"], "success", "First refresh status should be success");
 
         let repos = refresh_data["repos"].as_array().expect("repos should be an array");
         assert!(!repos.is_empty(), "Should have at least one repo after joining");
 
-        let repo_data = repos.iter()
-            .find(|r| r["name"] == TEST_GROUP_NAME)
-            .expect("Should find the creator's repo by name");
+        let repo_data = repos
+            .iter()
+            .find(|r| {
+                r["all_files"]
+                    .as_array()
+                    .map(|files| files.iter().any(|f| f.as_str() == Some(file_name)))
+                    .unwrap_or(false)
+            })
+            .expect("Should find a repo containing the uploaded file");
+        let refreshed_repo_id = repo_data["repo_id"]
+            .as_str()
+            .expect("repo_id should be a string")
+            .to_string();
 
         let refreshed_files = repo_data["refreshed_files"].as_array()
             .expect("refreshed_files should be an array");
         assert!(
-            refreshed_files.len() <= 1,
-            "First refresh should refresh at most one file, got {}",
-            refreshed_files.len()
+            refreshed_files.is_empty()
+                || (refreshed_files.len() == 1
+                    && refreshed_files[0].as_str() == Some(file_name)),
+            "First refresh should report either no-op or one refreshed expected file, got {refreshed_files:?}"
         );
-        if refreshed_files.len() == 1 {
-            assert_eq!(
-                refreshed_files[0].as_str().unwrap(),
-                file_name,
-                "Should have refreshed the correct file"
-            );
-        }
 
         let all_files = repo_data["all_files"].as_array().expect("all_files should be an array");
         assert_eq!(all_files.len(), 1, "Should have one file in all_files");
@@ -1115,7 +1173,7 @@ mod tests {
         let get_file_req = test::TestRequest::get()
             .uri(&format!(
                 "/api/groups/{}/repos/{}/media/{}",
-                group.id(), repo.id(), file_name
+                group.id(), refreshed_repo_id, file_name
             ))
             .to_request();
         let get_file_resp = test::call_service(&app, get_file_req).await;
@@ -1137,9 +1195,15 @@ mod tests {
         let repos2 = refresh_data2["repos"].as_array().expect("repos should be an array");
         assert!(!repos2.is_empty(), "Should still have repos");
 
-        let repo_data2 = repos2.iter()
-            .find(|r| r["name"] == TEST_GROUP_NAME)
-            .expect("Should still find the creator's repo by name on second refresh");
+        let repo_data2 = repos2
+            .iter()
+            .find(|r| {
+                r["all_files"]
+                    .as_array()
+                    .map(|files| files.iter().any(|f| f.as_str() == Some(file_name)))
+                    .unwrap_or(false)
+            })
+            .expect("Should still find the repo containing the uploaded file on second refresh");
         let refreshed_files2 = repo_data2["refreshed_files"].as_array()
             .expect("refreshed_files should be an array");
         assert!(refreshed_files2.is_empty(),