From 68ffe8744951ecfa9a0d1e0ddcf0158a2c9854cf Mon Sep 17 00:00:00 2001
From: Benjamin Bouvier <benjamin@bouvier.cc>
Date: Mon, 25 Nov 2024 16:18:39 +0100
Subject: [PATCH] task(linked chunk): make error reporting deterministic in
 `LinkedChunkBuilder::build`

By using a deterministic iteration ordering.
---
 .../src/linked_chunk/builder.rs               | 37 +++++--------------
 .../matrix-sdk-common/src/linked_chunk/mod.rs |  2 +-
 2 files changed, 10 insertions(+), 29 deletions(-)

diff --git a/crates/matrix-sdk-common/src/linked_chunk/builder.rs b/crates/matrix-sdk-common/src/linked_chunk/builder.rs
index adaf3cb4442..221e176633d 100644
--- a/crates/matrix-sdk-common/src/linked_chunk/builder.rs
+++ b/crates/matrix-sdk-common/src/linked_chunk/builder.rs
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-use std::{collections::HashMap, marker::PhantomData, ptr::NonNull};
+use std::{collections::BTreeMap, marker::PhantomData, ptr::NonNull};
 
 use super::{
     Chunk, ChunkContent, ChunkIdentifier, ChunkIdentifierGenerator, Ends, LinkedChunk,
@@ -37,7 +37,7 @@ struct TemporaryChunk<Item, Gap> {
 #[allow(missing_debug_implementations)]
 pub struct LinkedChunkBuilder<const CAP: usize, Item, Gap> {
     /// Work-in-progress chunks.
-    chunks: HashMap<ChunkIdentifier, TemporaryChunk<Item, Gap>>,
+    chunks: BTreeMap<ChunkIdentifier, TemporaryChunk<Item, Gap>>,
 
     /// Is the final `LinkedChunk` expected to be observable, as if it were
     /// created with [`LinkedChunk::new_with_update_history`]?
@@ -214,8 +214,8 @@ impl<const CAP: usize, Item, Gap> LinkedChunkBuilder<CAP, Item, Gap> {
             return Err(LinkedChunkBuilderError::MultipleConnectedComponents { chunks: remaining });
         }
 
-        // SAFETY: if this unwrap fails, it means we had no chunks in the stash, so we
-        // should have returned early.
+        // SAFETY: if one of these unwrap fails, it means we had no chunks in the stash,
+        // so we should have returned early.
         let first = first_chunk_ptr.unwrap();
         let last = prev_chunk_ptr.unwrap();
         let links = Ends { first, last: Some(last) };
@@ -389,11 +389,7 @@ mod tests {
 
         let err = lcb.build().unwrap_err();
         assert_matches!(err, LinkedChunkBuilderError::Cycle { path } => {
-            assert_eq!(path.len(), 3);
-            // Order is not guaranteed.
-            assert!(path.contains(&cid0));
-            assert!(path.contains(&cid1));
-            assert!(path.contains(&cid2));
+            assert_eq!(path, vec![cid0, cid2, cid1]);
         });
     }
 
@@ -413,15 +409,7 @@ mod tests {
 
         let err = lcb.build().unwrap_err();
         assert_matches!(err, LinkedChunkBuilderError::MultipleConnectedComponents { chunks } => {
-            // Order of iteration is not guaranteed, so the error either tells that cid0 and cid1
-            // are an isolated component, or that cid2 is.
-            if chunks.len() == 1 {
-                assert_eq!(chunks, [cid2]);
-            } else {
-                assert_eq!(chunks.len(), 2);
-                assert!(chunks.contains(&cid0));
-                assert!(chunks.contains(&cid1));
-            }
+            assert_eq!(chunks, [cid2]);
         });
     }
 
@@ -443,16 +431,9 @@ mod tests {
         assert_matches!(err, LinkedChunkBuilderError::InconsistentLinks {
             chunk, declared_prev, actual_prev
         } => {
-            // Since the order of traversal is undefined, the target chunk may be cid0 or cid1.
-            // Let's account for both.
-            if chunk == cid0 {
-                assert!(declared_prev.is_none());
-                assert_eq!(actual_prev, Some(cid1));
-            } else {
-                assert_eq!(chunk, cid1);
-                assert!(declared_prev.is_none());
-                assert_eq!(actual_prev, Some(cid0));
-            }
+            assert_eq!(chunk, cid1);
+            assert!(declared_prev.is_none());
+            assert_eq!(actual_prev, Some(cid0));
         });
     }
 }
diff --git a/crates/matrix-sdk-common/src/linked_chunk/mod.rs b/crates/matrix-sdk-common/src/linked_chunk/mod.rs
index a292942fa8f..bbe08325b2e 100644
--- a/crates/matrix-sdk-common/src/linked_chunk/mod.rs
+++ b/crates/matrix-sdk-common/src/linked_chunk/mod.rs
@@ -933,7 +933,7 @@ impl ChunkIdentifierGenerator {
 /// It is not the position of the chunk, just its unique identifier.
 ///
 /// Learn more with [`ChunkIdentifierGenerator`].
-#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash)]
+#[derive(Copy, Clone, Debug, PartialEq, Eq, Hash, PartialOrd, Ord)]
 #[repr(transparent)]
 pub struct ChunkIdentifier(u64);