deduplicate rooms in hierarchy traversal

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

@@ -0,0 +1 @@
+Fixed the client space hierarchy endpoint returning a 500 "Space hierarchy is unreasonably large" error for cyclic space graphs (e.g. a space containing itself). Rooms are now deduplicated during traversal as required by the spec, and the traversal depth is bounded even when the client does not specify `max_depth`.

src/api/client/space.rs

@@ -19,9 +19,14 @@ pub(crate) async fn get_hierarchy_route(
 	// there's no reasonable way to handle a space hierarchy changing during
 	// pagination.
 
-	let max_depth = body
-		.max_depth
-		.map(|max_depth| max_depth.min(UInt::from(MAX_MAX_DEPTH)));
+	// Default to MAX_MAX_DEPTH when the client doesn't specify one, so an
+	// unbounded traversal can never happen.
+	let max_depth = Some(
+		body.max_depth
+			.map_or(UInt::from(MAX_MAX_DEPTH), |max_depth| {
+				max_depth.min(UInt::from(MAX_MAX_DEPTH))
+			}),
+	);
 
 	let hierarchy = services
 		.rooms

src/service/rooms/summary/mod.rs

@@ -210,6 +210,10 @@ pub async fn get_room_hierarchy_for_user(
 		let mut summaries = vec![root_summary.summary];
 		let mut inaccessible_children: HashSet<_> =
 			root_summary.inaccessible_children.into_iter().collect();
+		// Rooms already included in the response. The spec requires each room to
+		// appear at most once, and this also terminates traversal of cyclic space
+		// graphs (e.g. a space containing itself).
+		let mut visited: HashSet<OwnedRoomId> = HashSet::from([room_id]);
 
 		// TODO refactor this with Vec::peek_mut once it's stabilized
 		while let Some(layer) = queue.last_mut() {
@@ -224,6 +228,11 @@ pub async fn get_room_hierarchy_for_user(
 				continue;
 			}
 
+			// Skip rooms already present in the response
+			if !visited.insert(room_id.clone()) {
+				continue;
+			}
+
 			let summary = match self
 				.get_room_summary_and_children_for_user(
 					Some(querying_user),
@@ -269,7 +278,7 @@ pub async fn get_room_hierarchy_for_user(
 			// suggest you reconsider some of the choices you made which led you to this
 			// point.
 			if queue.len() > 50 {
-				return Err!("Space hierarchy is unreasonably large");
+				return Err!(Request(TooLarge("Space hierarchy is unreasonably large")));
 			}
 
 			// Add accessible children as a new layer