fix(spaces): Correct hierarchy traversal

changelog.d/+space-hierarchy.bugfix.md

@@ -0,0 +1 @@
+Fixed space hierarchy depth limits, child ordering, and non-space child traversal. Contributed by @eleboucher.

conduwuit-example.toml

@@ -293,16 +293,6 @@
 #
 #stateinfo_cache_capacity = varies by system
 
-# Determines the cache size for spacehierarchy data.
-# Accepts any 32-bit integer.
-#
-# Each roomid contains info on its place in the space hierarchy.
-# This caps the capacity of cached spacehierarchy data.
-#
-# This defaults to 1,000 * CPU core count.
-#
-#roomid_spacehierarchy_cache_capacity = varies by system
-
 # Maximum entries stored in DNS memory-cache. The size of an entry may
 # vary so please take care if raising this value excessively. Only
 # decrease this when using an external DNS cache. Please note that

src/core/config/mod.rs

@@ -379,18 +379,6 @@ pub struct Config {
 	#[serde(default = "default_stateinfo_cache_capacity")]
 	pub stateinfo_cache_capacity: u32,
 
-	/// Determines the cache size for spacehierarchy data.
-	/// Accepts any 32-bit integer.
-	///
-	/// Each roomid contains info on its place in the space hierarchy.
-	/// This caps the capacity of cached spacehierarchy data.
-	///
-	/// This defaults to 1,000 * CPU core count.
-	///
-	/// default: varies by system
-	#[serde(default = "default_roomid_spacehierarchy_cache_capacity")]
-	pub roomid_spacehierarchy_cache_capacity: u32,
-
 	/// Maximum entries stored in DNS memory-cache. The size of an entry may
 	/// vary so please take care if raising this value excessively. Only
 	/// decrease this when using an external DNS cache. Please note that
@@ -2805,6 +2793,7 @@ const DEPRECATED_KEYS: &[&str] = &[
 	"cache_capacity",
 	"conduit_cache_capacity_modifier",
 	"max_concurrent_requests",
+	"roomid_spacehierarchy_cache_capacity",
 	"well_known_client",
 	"well_known_server",
 	"well_known_support_page",
@@ -2934,8 +2923,6 @@ fn default_servernameevent_data_cache_capacity() -> u32 {
 
 fn default_stateinfo_cache_capacity() -> u32 { parallelism_scaled_u32(100) }
 
-fn default_roomid_spacehierarchy_cache_capacity() -> u32 { parallelism_scaled_u32(1000) }
-
 fn default_dns_cache_entries() -> u32 { 32768 }
 
 fn default_dns_min_ttl() -> u64 { 60 * 180 }

src/service/rooms/summary/mod.rs

@@ -17,7 +17,7 @@ use ruma::{
 		StateEventType,
 		space::child::{HierarchySpaceChildEvent, SpaceChildEventContent},
 	},
-	room::{JoinRuleSummary, RestrictedSummary, RoomSummary},
+	room::{JoinRuleSummary, RestrictedSummary, RoomSummary, RoomType},
 	serde::Raw,
 };
 
@@ -124,7 +124,7 @@ impl Service {
 		via: Option<&[OwnedServerName]>,
 		suggested_only: bool,
 	) -> Result<Accessibility<SpaceSummaryAndChildren>> {
-		let (summary, inaccessible_children) = {
+		let (mut summary, inaccessible_children) = {
 			if let Some(summary) = self.build_local_room_summary(room_id).await {
 				// We have this room locally.
 				let children_state = self.get_space_child_events(room_id).await;
@@ -159,15 +159,27 @@ impl Service {
 			return Ok(Accessibility::Inaccessible);
 		}
 
-		let children = summary
+		if !matches!(summary.summary.room_type.as_ref(), Some(RoomType::Space)) {
+			summary.children_state.clear();
+		}
+
+		let mut children: Vec<_> = summary
             .children_state
             .iter()
             // Ignore deserialization failures
             .flat_map(Raw::deserialize)
             // Filter out non-suggested children if suggested_only is set
             .filter(|child| !suggested_only || child.content.suggested)
-            .map(|child| SpaceChild { room_id: child.state_key, via: child.content.via })
-            .collect();
+			.collect();
+		children.sort();
+		let children = children
+			.into_iter()
+			.rev()
+			.map(|child| SpaceChild {
+				room_id: child.state_key,
+				via: child.content.via,
+			})
+			.collect();
 
 		Ok(Accessibility::Accessible(SpaceSummaryAndChildren {
 			summary,
@@ -223,6 +235,13 @@ impl Service {
 				continue;
 			};
 
+			// The root room is at depth zero, so the current layer is the depth of
+			// the child about to be processed.
+			let depth = u64::try_from(queue.len()).expect("hierarchy depth should fit into u64");
+			if max_depth.is_some_and(|max_depth| depth > max_depth.into()) {
+				continue;
+			}
+
 			// Do not request rooms which have been determined to be inaccessible
 			if inaccessible_children.contains(&room_id) {
 				continue;
@@ -261,17 +280,6 @@ impl Service {
 			summaries.push(summary.summary);
 			inaccessible_children.extend(summary.inaccessible_children);
 
-			// Don't traverse the tree deeper than max_depth
-			#[allow(
-				clippy::as_conversions,
-				clippy::arithmetic_side_effects,
-				reason = "queue.len() should never be large enough to cause strange behavior \
-				          here"
-			)]
-			if max_depth.is_some_and(|max_depth| (queue.len() as u64 + 1) > max_depth.into()) {
-				continue;
-			}
-
 			// Bail out if queue length exceeds 50 to keep cycles in the space graph from
 			// blowing up the traversal. If you actually have over fifty nested spaces,
 			// and you're looking at this function to figure out what the issue is, I
@@ -306,7 +314,11 @@ impl Service {
 			return Accessibility::Inaccessible;
 		}
 
-		let children_state = self.get_space_child_events(room_id).await;
+		let children_state = if matches!(summary.room_type.as_ref(), Some(RoomType::Space)) {
+			self.get_space_child_events(room_id).await
+		} else {
+			vec![]
+		};
 
 		let (accessible_children, inaccessible_children) = children_state
             .iter()