From d569ef2e40f0180a6d233bf08d8eb85b0d578acb Mon Sep 17 00:00:00 2001 From: ember33 Date: Wed, 18 Mar 2026 12:17:14 +0100 Subject: [PATCH] style(spaces): convert to structured logging, fix import ordering, add lock docs Convert all log statements in space roles code to use structured key-value fields instead of string interpolation, matching the project code style. Fix import ordering (serde_json moved after conduwuit_core), move a misplaced `use futures::StreamExt` from function body to file-level imports, add lock ordering comments to prevent deadlocks, fix populate_space to acquire locks in the same order as handle_space_child_change, add diagnostic debug_warn before PL rejection errors, and document the nested cascade limitation on get_parent_spaces. Co-Authored-By: Claude Opus 4.6 (1M context) --- conduwuit-example.toml | 5 ++ src/admin/space/roles.rs | 3 +- src/service/rooms/roles/mod.rs | 104 +++++++++++----------------- src/service/rooms/timeline/build.rs | 22 +++++- 4 files changed, 66 insertions(+), 68 deletions(-) diff --git a/conduwuit-example.toml b/conduwuit-example.toml index 66082a1a..847b261c 100644 --- a/conduwuit-example.toml +++ b/conduwuit-example.toml @@ -476,6 +476,11 @@ # #space_permission_cascading = false +# Maximum number of spaces to cache role data for. When exceeded the +# cache is cleared and repopulated on demand. +# +#space_roles_cache_capacity = 1000 + # Enabling this setting opens registration to anyone without restrictions. # This makes your server vulnerable to abuse # diff --git a/src/admin/space/roles.rs b/src/admin/space/roles.rs index ad14e5dd..387ac7c4 100644 --- a/src/admin/space/roles.rs +++ b/src/admin/space/roles.rs @@ -11,6 +11,7 @@ use ruma::{OwnedRoomId, OwnedRoomOrAliasId, OwnedUserId, events::StateEventType} use serde_json::value::to_raw_value; use conduwuit::matrix::pdu::PduBuilder; +use futures::StreamExt; use crate::{admin_command, admin_command_dispatch}; @@ -235,8 +236,6 @@ async fn remove(&self, space: OwnedRoomOrAliasId, role_name: String) -> Result { .get_room_shortstatehash(&space_id) .await { - use futures::StreamExt; - let user_entries: Vec<(_, ruma::OwnedEventId)> = self .services .rooms diff --git a/src/service/rooms/roles/mod.rs b/src/service/rooms/roles/mod.rs index 11925a58..e8cd8062 100644 --- a/src/service/rooms/roles/mod.rs +++ b/src/service/rooms/roles/mod.rs @@ -17,7 +17,6 @@ use conduwuit::{ matrix::pdu::PduBuilder, warn, }; -use serde_json::value::to_raw_value; use conduwuit_core::{ matrix::space_roles::{ RoleDefinition, SpaceRoleMemberEventContent, SpaceRoleRoomEventContent, @@ -41,6 +40,7 @@ use ruma::{ space::child::SpaceChildEventContent, }, }; +use serde_json::value::to_raw_value; use tokio::sync::RwLock; use crate::{Dep, globals, rooms}; @@ -134,7 +134,7 @@ impl crate::Service for Service { return Ok(()); } - info!("Rebuilding space roles cache from all known rooms..."); + info!("Rebuilding space roles cache from all known rooms"); let mut space_count: usize = 0; let room_ids: Vec = self @@ -148,7 +148,7 @@ impl crate::Service for Service { for room_id in &room_ids { match self.services.state_accessor.get_room_type(room_id).await { | Ok(RoomType::Space) => { - debug!("Populating space roles cache for {room_id}"); + debug!(room_id = %room_id, "Populating space roles cache"); self.populate_space(room_id).await; space_count = space_count.saturating_add(1); }, @@ -156,7 +156,7 @@ impl crate::Service for Service { } } - info!("Space roles cache rebuilt for {space_count} spaces"); + info!(space_count, "Space roles cache rebuilt"); Ok(()) } @@ -225,7 +225,7 @@ pub async fn ensure_default_roles(&self, space_id: &RoomId) -> Result { .build_and_append_pdu(pdu, sender, Some(space_id), &state_lock) .await?; - debug!("Sent default {SPACE_ROLES_EVENT_TYPE} event for {space_id}"); + debug!(space_id = %space_id, event_type = SPACE_ROLES_EVENT_TYPE, "Sent default space roles event"); Ok(()) } @@ -274,7 +274,7 @@ pub async fn populate_space(&self, space_id: &RoomId) { { | Ok(hash) => hash, | Err(e) => { - debug_warn!("Failed to get shortstatehash for {space_id}, cache may be stale: {e}"); + debug_warn!(space_id = %space_id, error = ?e, "Failed to get shortstatehash, cache may be stale"); return; }, }; @@ -373,13 +373,8 @@ pub async fn populate_space(&self, space_id: &RoomId) { }) .await; - // Update forward index - { - let mut space_to_rooms = self.space_to_rooms.write().await; - space_to_rooms.insert(space_id.to_owned(), child_rooms.iter().cloned().collect()) - }; - - // Single write lock for all children — clear stale entries first + // Lock ordering: room_to_space before space_to_rooms. + // This order must be consistent to avoid deadlocks. { let mut room_to_space = self.room_to_space.write().await; // Remove this space from all existing entries @@ -388,13 +383,19 @@ pub async fn populate_space(&self, space_id: &RoomId) { !parents.is_empty() }); // Insert fresh children - for child_room_id in child_rooms { + for child_room_id in &child_rooms { room_to_space - .entry(child_room_id) + .entry(child_room_id.clone()) .or_default() .insert(space_id.to_owned()); } } + + // Update forward index (after room_to_space to maintain lock ordering) + { + let mut space_to_rooms = self.space_to_rooms.write().await; + space_to_rooms.insert(space_id.to_owned(), child_rooms.into_iter().collect()) + }; } } @@ -467,6 +468,9 @@ pub async fn user_qualifies_for_room( } /// Get the parent Spaces of a child room, if any. +/// +/// Only direct parent spaces are returned. Nested sub-space cascading +/// is not supported (see design doc requirement 6). #[implement(Service)] pub async fn get_parent_spaces(&self, room_id: &RoomId) -> Vec { if !self.is_enabled() { @@ -508,9 +512,7 @@ pub async fn sync_power_levels(&self, space_id: &RoomId, room_id: &RoomId) -> Re .is_joined(server_user, room_id) .await { - debug_warn!( - "Server user is not joined to {room_id}, skipping PL sync" - ); + debug_warn!(room_id = %room_id, "Server user is not joined, skipping PL sync"); return Ok(()); } @@ -641,9 +643,7 @@ pub async fn auto_join_qualifying_rooms( .is_joined(server_user, child_room_id) .await { - debug_warn!( - "Server user is not joined to {child_room_id}, skipping auto-join" - ); + debug_warn!(room_id = %child_room_id, "Server user is not joined, skipping auto-join"); continue; } @@ -664,9 +664,7 @@ pub async fn auto_join_qualifying_rooms( ) .await { - debug_warn!( - "Failed to invite {user_id} to {child_room_id} during auto-join: {e}" - ); + debug_warn!(user_id = %user_id, room_id = %child_room_id, error = ?e, "Failed to invite user during auto-join"); continue; } @@ -685,7 +683,7 @@ pub async fn auto_join_qualifying_rooms( ) .await { - warn!("Failed to auto-join {user_id} to {child_room_id}: {e}"); + warn!(user_id = %user_id, room_id = %child_room_id, error = ?e, "Failed to auto-join user"); } } @@ -734,7 +732,7 @@ impl Service { if let Err(e) = this.sync_power_levels(&space_id, child_room_id).await { - debug_warn!("Failed to sync PLs in {child_room_id}: {e}"); + debug_warn!(room_id = %child_room_id, error = ?e, "Failed to sync power levels"); } } // Revalidate all space members against all child rooms @@ -749,9 +747,7 @@ impl Service { if let Err(e) = Box::pin(this.kick_unqualified_from_rooms(&space_id, member)).await { - debug_warn!( - "Role definition revalidation kick failed for {member}: {e}" - ); + debug_warn!(user_id = %member, error = ?e, "Role definition revalidation kick failed"); } } }, @@ -761,16 +757,12 @@ impl Service { if let Err(e) = this.auto_join_qualifying_rooms(&space_id, user_id).await { - debug_warn!( - "Space role auto-join failed for {user_id}: {e}" - ); + debug_warn!(user_id = %user_id, error = ?e, "Space role auto-join failed"); } if let Err(e) = Box::pin(this.kick_unqualified_from_rooms(&space_id, user_id)).await { - debug_warn!( - "Space role auto-kick failed for {user_id}: {e}" - ); + debug_warn!(user_id = %user_id, error = ?e, "Space role auto-kick failed"); } // Sync power levels in all child rooms let child_rooms = this.get_child_rooms(&space_id).await; @@ -778,9 +770,7 @@ impl Service { if let Err(e) = this.sync_power_levels(&space_id, child_room_id).await { - debug_warn!( - "Failed to sync PLs in {child_room_id}: {e}" - ); + debug_warn!(room_id = %child_room_id, error = ?e, "Failed to sync power levels"); } } } @@ -808,9 +798,7 @@ impl Service { .kick_unqualified_from_rooms(&space_id, member)) .await { - debug_warn!( - "Space role requirement kick failed for {member}: {e}" - ); + debug_warn!(user_id = %member, error = ?e, "Space role requirement kick failed"); } } } @@ -860,7 +848,8 @@ impl Service { }; if is_removal { - // Remove child from room_to_space reverse index + // Lock ordering: room_to_space before space_to_rooms. + // This order must be consistent to avoid deadlocks. let mut room_to_space = this.room_to_space.write().await; if let Some(parents) = room_to_space.get_mut(&child_room_id) { parents.remove(&space_id); @@ -900,10 +889,7 @@ impl Service { .is_joined(server_user, &child_room_id) .await { - debug_warn!( - "Server user is not joined to {child_room_id}, \ - skipping auto-join enforcement for new child" - ); + debug_warn!(room_id = %child_room_id, "Server user is not joined, skipping auto-join enforcement for new child"); return; } @@ -947,9 +933,7 @@ impl Service { ) .await { - debug_warn!( - "Failed to invite {member} to {child_room_id}: {e}" - ); + debug_warn!(user_id = %member, room_id = %child_room_id, error = ?e, "Failed to invite user"); continue; } @@ -970,9 +954,7 @@ impl Service { ) .await { - warn!( - "Failed to auto-join {member} to {child_room_id}: {e}" - ); + warn!(user_id = %member, room_id = %child_room_id, error = ?e, "Failed to auto-join user"); } } } @@ -1005,7 +987,7 @@ impl Service { let _permit = this.enforcement_semaphore.acquire().await; if let Err(e) = this.auto_join_qualifying_rooms(&space_id, &user_id).await { - debug_warn!("Auto-join on Space join failed for {user_id}: {e}"); + debug_warn!(user_id = %user_id, error = ?e, "Auto-join on Space join failed"); } // Also sync their power levels let child_rooms = this.get_child_rooms(&space_id).await; @@ -1013,9 +995,7 @@ impl Service { if let Err(e) = this.sync_power_levels(&space_id, child_room_id).await { - debug_warn!( - "Failed to sync PLs on join for {child_room_id}: {e}" - ); + debug_warn!(room_id = %child_room_id, error = ?e, "Failed to sync power levels on join"); } } }); @@ -1059,9 +1039,7 @@ pub async fn kick_unqualified_from_rooms( .is_joined(server_user, child_room_id) .await { - debug_warn!( - "Server user is not joined to {child_room_id}, skipping kick enforcement" - ); + debug_warn!(room_id = %child_room_id, "Server user is not joined, skipping kick enforcement"); continue; } // Skip if not joined @@ -1089,9 +1067,7 @@ pub async fn kick_unqualified_from_rooms( .get_member(child_room_id, user_id) .await else { - debug_warn!( - "Could not get member event for {user_id} in {child_room_id}, skipping kick" - ); + debug_warn!(user_id = %user_id, room_id = %child_room_id, "Could not get member event, skipping kick"); continue; }; @@ -1119,9 +1095,7 @@ pub async fn kick_unqualified_from_rooms( ) .await { - warn!( - "Failed to kick {user_id} from {child_room_id} for missing roles: {e}" - ); + warn!(user_id = %user_id, room_id = %child_room_id, error = ?e, "Failed to kick user for missing roles"); } } diff --git a/src/service/rooms/timeline/build.rs b/src/service/rooms/timeline/build.rs index 5f0eebdb..c9133032 100644 --- a/src/service/rooms/timeline/build.rs +++ b/src/service/rooms/timeline/build.rs @@ -5,7 +5,7 @@ use std::{ use conduwuit_core::matrix::space_roles::RoleDefinition; -use conduwuit::trace; +use conduwuit::{debug_warn, trace}; use conduwuit_core::{ Err, Result, implement, matrix::{event::Event, pdu::PduBuilder}, @@ -122,6 +122,13 @@ pub async fn build_and_append_pdu( self.services.roles.get_user_power_level(parent_space, user_id).await { if i64::from(*proposed_pl) != space_pl { + debug_warn!( + user_id = %user_id, + room_id = %room_id, + proposed_pl = i64::from(*proposed_pl), + space_pl, + "Rejecting PL change conflicting with space role" + ); return Err!(Request(Forbidden( "Cannot change power level that is set by Space roles" ))); @@ -160,11 +167,24 @@ pub async fn build_and_append_pdu( if let Some(space_pl) = space_pl { match proposed.users.get(user_id) { | None => { + debug_warn!( + user_id = %user_id, + room_id = %room_id, + space_pl, + "Rejecting PL change: space-managed user omitted" + ); return Err!(Request(Forbidden( "Cannot omit a user whose power level is managed by Space roles" ))); }, | Some(pl) if i64::from(*pl) != space_pl => { + debug_warn!( + user_id = %user_id, + room_id = %room_id, + proposed_pl = i64::from(*pl), + space_pl, + "Rejecting PL change conflicting with space role" + ); return Err!(Request(Forbidden( "Cannot change power level that is set by Space roles" )));