From 3e7796e44a4b7361ab3ebc6b5843756c585f8594 Mon Sep 17 00:00:00 2001 From: suryatho Date: Sun, 16 Nov 2025 22:17:07 -0500 Subject: [PATCH 01/13] Add BiMultiMap to `rule_structs.rs` and use it in RuleManager Move RwLocks into RuleManager for more granular control of blocking Implement borrow for Topic struct so we strings can lend their ownership to topic when all we really need is &Topic --- .../src/controllers/rule_controller.rs | 11 +- scylla-server/src/main.rs | 4 +- scylla-server/src/rule_structs.rs | 329 +++++++++++------- scylla-server/src/socket_handler.rs | 10 +- 4 files changed, 213 insertions(+), 141 deletions(-) diff --git a/scylla-server/src/controllers/rule_controller.rs b/scylla-server/src/controllers/rule_controller.rs index f8778a52..8d475046 100644 --- a/scylla-server/src/controllers/rule_controller.rs +++ b/scylla-server/src/controllers/rule_controller.rs @@ -5,7 +5,6 @@ use axum_extra::{ headers::{authorization::Basic, Authorization}, TypedHeader, }; -use tokio::sync::RwLock; use tracing::debug; use crate::{ @@ -16,7 +15,7 @@ use crate::{ #[debug_handler] pub async fn add_rule( TypedHeader(auth): TypedHeader>, - Extension(rules_manager): Extension>>, + Extension(rules_manager): Extension>, Json(rule): Json, ) -> Result, ScyllaError> { debug!( @@ -25,9 +24,8 @@ pub async fn add_rule( auth.username().to_string() ); match rules_manager - .write() - .await .add_rule(ClientId(auth.username().to_string()), rule) + .await { Ok(_) => Ok(Json::from("Rule added!".to_owned())), Err(err) => Err(ScyllaError::RuleError(err)), @@ -37,7 +35,7 @@ pub async fn add_rule( #[debug_handler] pub async fn delete_rule( TypedHeader(auth): TypedHeader>, - Extension(rules_manager): Extension>>, + Extension(rules_manager): Extension>, Path(rule_id): Path, ) -> Result<(), ScyllaError> { debug!( @@ -46,9 +44,8 @@ pub async fn delete_rule( auth.username().to_string() ); match rules_manager - .write() - .await .delete_rule(ClientId(auth.username().to_string()), RuleId(rule_id)) + .await { Ok(_) => Ok(()), Err(err) => Err(ScyllaError::RuleError(err)), diff --git a/scylla-server/src/main.rs b/scylla-server/src/main.rs index d51d3fc1..1a2e3fc1 100755 --- a/scylla-server/src/main.rs +++ b/scylla-server/src/main.rs @@ -43,7 +43,7 @@ use scylla_server::{ use socketioxide::{extract::SocketRef, SocketIo}; use tokio::{ signal, - sync::{broadcast, mpsc, RwLock}, + sync::{broadcast, mpsc}, }; use tokio_util::{sync::CancellationToken, task::TaskTracker}; use tower::ServiceBuilder; @@ -249,7 +249,7 @@ async fn main() -> Result<(), Box> { let (db_send, db_receive) = mpsc::channel::>(1000); // the rules manager - let rules_manager = Arc::new(RwLock::new(RuleManager::new())); + let rules_manager = Arc::new(RuleManager::new()); // the below two threads need to cancel cleanly to ensure all queued messages are sent. therefore they are part of the a task tracker group. // create a task tracker and cancellation token diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index 8d5ca58c..98b72143 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -10,7 +10,10 @@ use rustc_hash::FxHashSet; use serde::{Deserialize, Serialize}; use serde_with::serde_as; use serde_with::DurationSeconds; +use std::borrow::Borrow; +use std::hash::Hash; use std::time::Duration; +use tokio::sync::RwLock; use tracing::trace; use tracing::warn; @@ -21,6 +24,76 @@ static ASCII_LOWER: [char; 26] = [ 't', 'u', 'v', 'w', 'x', 'y', 'z', ]; +struct BiMultiMap { + left_to_right: FxHashMap>, + right_to_left: FxHashMap>, +} + +#[allow(dead_code)] +impl BiMultiMap { + pub fn new() -> Self { + Self { + left_to_right: FxHashMap::default(), + right_to_left: FxHashMap::default(), + } + } + + pub fn get_right(&self, left: &L) -> Option<&FxHashSet> { + self.left_to_right.get(left) + } + + pub fn get_left(&self, right: &R) -> Option<&FxHashSet> { + self.right_to_left.get(right) + } + + pub fn get_right_mut(&mut self, left: &L) -> Option<&mut FxHashSet> { + self.left_to_right.get_mut(left) + } + + pub fn get_left_mut(&mut self, right: &R) -> Option<&mut FxHashSet> { + self.right_to_left.get_mut(right) + } + + pub fn insert(&mut self, left: &L, right: &R) { + self.left_to_right + .entry(left.clone()) + .or_insert_with(FxHashSet::default) + .insert(right.clone()); + self.right_to_left + .entry(right.clone()) + .or_insert_with(FxHashSet::default) + .insert(left.clone()); + } + + /// remove all mappings for a given left key, if none left keys remain for a right key, remove that right key as well + pub fn remove_left(&mut self, left: &L) { + if let Some(rights) = self.left_to_right.remove(left) { + for right in rights { + if let Some(lefts) = self.right_to_left.get_mut(&right) { + lefts.remove(left); + if lefts.is_empty() { + self.right_to_left.remove(&right); + } + } + } + } + } + + /// remove all mappings for a given right key, if none right keys remain for a left key, remove that left key as well + pub fn remove_right(&mut self, right: &R) { + if let Some(lefts) = self.right_to_left.remove(right) { + for left in lefts { + if let Some(rights) = self.left_to_right.get_mut(&left) { + rights.remove(right); + if rights.is_empty() { + self.left_to_right.remove(&left); + } + } + } + } + } +} + /// cooldown time const COOLDOWN_TIME: std::time::Duration = std::time::Duration::from_secs(60); @@ -41,6 +114,18 @@ pub struct RuleId(pub String); #[derive(PartialEq, Eq, Hash, Display, Clone, Serialize, Deserialize)] pub struct Topic(String); +impl Borrow for Topic { + fn borrow(&self) -> &str { + &self.0 + } +} + +impl Borrow for Topic { + fn borrow(&self) -> &String { + &self.0 + } +} + #[derive(Serialize)] pub struct RuleNotification { pub id: RuleId, @@ -155,10 +240,12 @@ pub enum RuleManagerError { /// the rule manager pub struct RuleManager { - /// > - clients_map: FxHashMap>>, - /// > - rules_lookup: FxHashMap>, + /// + rules: RwLock>, + /// > + topic_index: RwLock>>, + /// bimap + subscriptions: RwLock>, } impl Default for RuleManager { fn default() -> Self { @@ -169,164 +256,152 @@ impl Default for RuleManager { impl RuleManager { pub fn new() -> Self { Self { - clients_map: FxHashMap::default(), - rules_lookup: FxHashMap::default(), + rules: RwLock::new(FxHashMap::default()), + topic_index: RwLock::new(FxHashMap::default()), + subscriptions: RwLock::new(BiMultiMap::new()), } } /// Handles a new socket message, returning a RuleNotification for one to many clients if action should be taken - pub fn handle_msg( - &mut self, + pub async fn handle_msg( + &self, data: &ClientData, ) -> Result>, RuleManagerError> { - // TODO uneccessary clone - let topic = Topic(data.name.clone()); - - let Some(clients) = self.rules_lookup.get(&topic) else { - trace!("(normal) Could not find rule in rule cache: {}", data.name); - return Err(RuleManagerError::NoMatchingRule); + // Read from topic to rule index and drop lock immediately + let rule_ids = { + let topic_index_read = self.topic_index.read().await; + match topic_index_read.get(&data.name) { + Some(rule_ids) => rule_ids.clone(), + None => { + trace!("Could not find rule in topic -> rule index: {}", data.name); + return Err(RuleManagerError::NoMatchingRule); + } + } }; - let mut notifications: Vec<(ClientId, RuleNotification)> = Vec::new(); - // warning if the clients is empty we havent been cleaning right - if clients.is_empty() { - warn!("Empty rule cache entry for {}!", data.name); - } + let mut notifications: Vec<(ClientId, RuleNotification)> = Vec::new(); - for client_want in clients { - let Some(rules) = self.clients_map.get_mut(client_want) else { - warn!("Client cached but not found!"); - return Err(RuleManagerError::Failure); + for rule_id in rule_ids { + let rule_triggered = { + let mut rules_write = self.rules.write().await; + let Some(rule) = rules_write.get_mut(&rule_id) else { + trace!("Could not find rule in rules map: {}", rule_id); + return Err(RuleManagerError::NoMatchingRule); + }; + let Some(triggered) = rule.tick(&data.values) else { + return Err(RuleManagerError::RuleFailure); + }; + triggered }; - if let Some(rule_set) = rules.get_mut(&topic) { - for rule in rule_set { - // return rule failure if underlying tick fails - let Some(is_triggered) = rule.tick(&data.values) else { - return Err(RuleManagerError::RuleFailure); - }; - if is_triggered { - notifications.push(( - client_want.clone(), - RuleNotification { - id: rule.id.clone(), - topic: topic.clone(), - values: data.values.clone(), - time: data.timestamp, - }, - )); + let clients = { + let subscriptions_read = self.subscriptions.read().await; + // get all clients subscribed to this rule + match subscriptions_read.get_left(&rule_id) { + Some(clients) => clients.clone(), + None => { + trace!( + "(indexed) Could not find clients for rule in subscriptions bimap: {}", + data.name + ); + return Err(RuleManagerError::NoMatchingRule); } } - } - } - // pass back the results - if notifications.is_empty() { - Ok(None) - } else { - Ok(Some(notifications)) - } - } + }; - /// Adds a rule, creating or activating the client if needed - pub fn add_rule(&mut self, client: ClientId, rule: Rule) -> Result<(), RuleManagerError> { - // go through the topics and add to rules lookup table - match self.rules_lookup.get_mut(&rule.topic) { - Some(rules) => { - rules.insert(client.clone()); - } - None => { - let mut new_set = FxHashSet::default(); - new_set.insert(client.clone()); - self.rules_lookup.insert(rule.topic.clone(), new_set); + // Clients should never be empty + if clients.is_empty() { + warn!("Empty subscriptions entry for rule {}!", rule_id); } - } - // push rule, make client active and push rule, or create client and push rule - match self.clients_map.get_mut(&client) { - Some(client) => match client.get_mut(&rule.topic) { - Some(set) => set.push(rule), - None => { - client.insert(rule.topic.clone(), vec![rule]); + // Push notifications for all clients who are subscribed to this rule + if rule_triggered { + for client in clients { + notifications.push(( + client.clone(), + RuleNotification { + id: rule_id.clone(), + topic: Topic(data.name.clone()), + values: data.values.clone(), + time: data.timestamp, + }, + )); } - }, - - None => { - let mut map = FxHashMap::default(); - map.insert(rule.topic.clone(), vec![rule]); - self.clients_map.insert(client, map); } - }; + } + Ok(Some(notifications)) + } + /// Adds a rule, creating or activating the client if needed + pub async fn add_rule(&self, client: ClientId, rule: Rule) -> Result<(), RuleManagerError> { + // Add to subscriptions bimap + self.subscriptions + .write() + .await + .insert(&client, &rule.id.clone()); + + // Add to topic index + self.topic_index + .write() + .await + .entry(rule.topic.clone()) + .or_insert(FxHashSet::default()) + .insert(rule.id.clone()); + + // Add to rules lookup + self.rules.write().await.insert(rule.id.clone(), rule); Ok(()) } - /// Deletes a rule, leaving the client existing and active no matter what - pub fn delete_rule( - &mut self, + /// Deletes a rule from client, if no more clients exist for the rule, delete it entirely + pub async fn delete_rule( + &self, client_id: ClientId, rule_id: RuleId, ) -> Result<(), RuleManagerError> { - // first, find the rules from the clients map - let Some(rules) = self.clients_map.get_mut(&client_id) else { - warn!("Could not find client {}", client_id); - return Err(RuleManagerError::NoSuchClient); - }; - - let mut removed: Option = None; - for rule_vals in rules.values_mut() { - let Some(pos) = rule_vals.iter().position(|a| a.id == rule_id) else { - break; + // Remove rule from client + let no_more_clients = { + let mut subscriptions_write = self.subscriptions.write().await; + let Some(rules) = subscriptions_write.get_right_mut(&client_id) else { + warn!( + "Could not find client in subscriptions bimap to delete rule: {}", + client_id + ); + return Err(RuleManagerError::NoSuchClient); // Keep this early return }; - removed = Some(rule_vals.remove(pos)); - } - - let Some(removed) = removed else { - warn!("Could not find rule: {}", rule_id); - return Err(RuleManagerError::NoMatchingRule); + rules.remove(&rule_id); + if rules.is_empty() { + subscriptions_write.remove_left(&client_id); + true + } else { + false + } }; - // now, yeet the rule from the lookup cache, ONLY IF the client doesnt have any rules with the given topic left - let lookup_preserve = rules.contains_key(&removed.topic); + // If no more clients exist for this rule, delete it from rules and topic index + // Gaurd + if !no_more_clients { + return Ok(()); + } - if !lookup_preserve { - let Some(clients) = self.rules_lookup.get_mut(&removed.topic) else { - warn!("Could not find rule in cache!"); - return Err(RuleManagerError::Failure); - }; - // remove the client from the cache for that topic, deleting rule from cache if necessary - clients.retain(|client| *client != client_id); - // delete client from cache is normal, the client could still exist without rules in client_map - if clients.is_empty() { - self.rules_lookup.remove(&removed.topic); + // Remove from topic index and rules map + let topic = match self.rules.read().await.get(&rule_id) { + Some(rule) => rule.topic.clone(), + None => { + warn!("No matching rule found for rule_id {}", rule_id); + return Err(RuleManagerError::NoMatchingRule); } - } // else we dont touch the lookup cache + }; + self.topic_index.write().await.remove(&topic); + self.rules.write().await.remove(&rule_id); Ok(()) } /// deletes a client, and all of its rules - pub fn delete_client(&mut self, client_id: ClientId) -> Result<(), RuleManagerError> { - // first, yeet from clients map - let Some(rules) = self.clients_map.remove(&client_id) else { - warn!("Could not find client to delete: {}", client_id); - return Err(RuleManagerError::NoSuchClient); - }; - - // now, for each unique topic found amongst the rules, yeet it from the lookup - // this uses a hashset to de-dup the rules to avoid annoying warnings - for rule in rules.keys() { - warn!("DELETING {}", rule); - let Some(client_list) = self.rules_lookup.get_mut(rule) else { - warn!("Could not find topic in rule lookup table!"); - return Err(RuleManagerError::Failure); - }; - client_list.retain(|client| *client != client_id); - // remove the whole entry if no clients exist for the topic - if client_list.is_empty() { - self.rules_lookup.remove(rule); - } - } - + pub async fn delete_client(&self, client_id: ClientId) -> Result<(), RuleManagerError> { + // Just remove using bi multi map + self.subscriptions.write().await.remove_left(&client_id); Ok(()) } } diff --git a/scylla-server/src/socket_handler.rs b/scylla-server/src/socket_handler.rs index 372fa05f..88042255 100644 --- a/scylla-server/src/socket_handler.rs +++ b/scylla-server/src/socket_handler.rs @@ -46,7 +46,7 @@ struct AuthData { pub async fn socket_handler_with_metadata( cancel_token: CancellationToken, mut data_channel: broadcast::Receiver, - rules_manager: Arc>, + rules_manager: Arc, io: SocketIo, ) { let mut upload_counter = 0u8; @@ -195,18 +195,18 @@ pub async fn socket_handler_with_metadata( /// Handles triggering rules based on a recieved datapoint async fn handle_rule_processing( data: &ClientData, - rule_manager: &Arc>, + rule_manager: &Arc, client_socket_map: &Arc>>, io: &SocketIo, ) { - let Ok(Some(notifs)) = rule_manager.write().await.handle_msg(data) else { + let Ok(Some(notifs)) = rule_manager.handle_msg(data).await else { return; }; for notification in notifs { let read_clients = client_socket_map.read().await; let Some(sid) = read_clients.get(¬ification.0 .0) else { warn!("Could not find client to deliver notification, deleting client"); - let _ = rule_manager.write().await.delete_client(notification.0); + let _ = rule_manager.delete_client(notification.0).await; return; }; debug!( @@ -215,7 +215,7 @@ async fn handle_rule_processing( ); let Some(socket) = io.get_socket(*sid) else { warn!("Could not find client socket, deleting client"); - let _ = rule_manager.write().await.delete_client(notification.0); + let _ = rule_manager.delete_client(notification.0).await; return; }; if let Err(err) = socket.emit(RULE_SOCKET_KEY, ¬ification.1) { From 1b292bbbbc41975e17835a5e6bece2f45f9df69e Mon Sep 17 00:00:00 2001 From: suryatho Date: Mon, 17 Nov 2025 14:27:59 -0500 Subject: [PATCH 02/13] Update BiMultiMap to return optional of newly empty values when removing key. Used to remove useless rules from topic and rules maps in RuleManager. --- scylla-server/src/rule_structs.rs | 123 ++++++++++++++++++++---------- 1 file changed, 81 insertions(+), 42 deletions(-) diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index 98b72143..da77c7aa 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -24,7 +24,7 @@ static ASCII_LOWER: [char; 26] = [ 't', 'u', 'v', 'w', 'x', 'y', 'z', ]; -struct BiMultiMap { +pub struct BiMultiMap { left_to_right: FxHashMap>, right_to_left: FxHashMap>, } @@ -65,32 +65,52 @@ impl BiMultiMap { .insert(left.clone()); } - /// remove all mappings for a given left key, if none left keys remain for a right key, remove that right key as well - pub fn remove_left(&mut self, left: &L) { + /// Remove all mappings for a given left key, if none left keys remain for a right key, remove that right key as well. + /// returns: optional set of empty rights that no longer map to any lefts + pub fn remove_left(&mut self, left: &L) -> Option> { + let mut empty_rights = FxHashSet::default(); + if let Some(rights) = self.left_to_right.remove(left) { for right in rights { if let Some(lefts) = self.right_to_left.get_mut(&right) { lefts.remove(left); if lefts.is_empty() { self.right_to_left.remove(&right); + empty_rights.insert(right); } } } } + + if empty_rights.is_empty() { + None + } else { + Some(empty_rights) + } } - /// remove all mappings for a given right key, if none right keys remain for a left key, remove that left key as well - pub fn remove_right(&mut self, right: &R) { + /// Remove all mappings for a given right key, if none right keys remain for a left key, remove that left key as well. + /// returns: optional set of empty lefts that no longer map to any rights + pub fn remove_right(&mut self, right: &R) -> Option> { + let mut empty_lefts = FxHashSet::default(); + if let Some(lefts) = self.right_to_left.remove(right) { for left in lefts { if let Some(rights) = self.left_to_right.get_mut(&left) { rights.remove(right); if rights.is_empty() { self.left_to_right.remove(&left); + empty_lefts.insert(left); } } } } + + if empty_lefts.is_empty() { + None + } else { + Some(empty_lefts) + } } } @@ -268,19 +288,15 @@ impl RuleManager { data: &ClientData, ) -> Result>, RuleManagerError> { // Read from topic to rule index and drop lock immediately - let rule_ids = { - let topic_index_read = self.topic_index.read().await; - match topic_index_read.get(&data.name) { - Some(rule_ids) => rule_ids.clone(), - None => { - trace!("Could not find rule in topic -> rule index: {}", data.name); - return Err(RuleManagerError::NoMatchingRule); - } + let rule_ids = match self.topic_index.read().await.get(&data.name) { + Some(rule_ids) => rule_ids.clone(), + None => { + trace!("Could not find rule in topic -> rule index: {}", data.name); + return Err(RuleManagerError::NoMatchingRule); } }; let mut notifications: Vec<(ClientId, RuleNotification)> = Vec::new(); - for rule_id in rule_ids { let rule_triggered = { let mut rules_write = self.rules.write().await; @@ -294,18 +310,19 @@ impl RuleManager { triggered }; - let clients = { - let subscriptions_read = self.subscriptions.read().await; - // get all clients subscribed to this rule - match subscriptions_read.get_left(&rule_id) { - Some(clients) => clients.clone(), - None => { - trace!( - "(indexed) Could not find clients for rule in subscriptions bimap: {}", - data.name - ); - return Err(RuleManagerError::NoMatchingRule); - } + if !rule_triggered { + continue; + } + + // get all clients subscribed to this rule + let clients = match self.subscriptions.read().await.get_left(&rule_id) { + Some(clients) => clients.clone(), + None => { + trace!( + "(indexed) Could not find clients for rule in subscriptions bimap: {}", + data.name + ); + return Err(RuleManagerError::NoMatchingRule); } }; @@ -315,21 +332,24 @@ impl RuleManager { } // Push notifications for all clients who are subscribed to this rule - if rule_triggered { - for client in clients { - notifications.push(( - client.clone(), - RuleNotification { - id: rule_id.clone(), - topic: Topic(data.name.clone()), - values: data.values.clone(), - time: data.timestamp, - }, - )); - } + for client in clients { + notifications.push(( + client.clone(), + RuleNotification { + id: rule_id.clone(), + topic: Topic(data.name.clone()), + values: data.values.clone(), + time: data.timestamp, + }, + )); } } - Ok(Some(notifications)) + + if notifications.is_empty() { + Ok(None) + } else { + Ok(Some(notifications)) + } } /// Adds a rule, creating or activating the client if needed @@ -371,6 +391,8 @@ impl RuleManager { }; rules.remove(&rule_id); if rules.is_empty() { + // Remove client. Since client is no longer subscribed to any rule + // no rules are removed as well as a side effect subscriptions_write.remove_left(&client_id); true } else { @@ -398,10 +420,27 @@ impl RuleManager { Ok(()) } - /// deletes a client, and all of its rules + /// Deletes a client, and all of its rules. + /// Removes rules that are no longer subscribed to if needed. pub async fn delete_client(&self, client_id: ClientId) -> Result<(), RuleManagerError> { - // Just remove using bi multi map - self.subscriptions.write().await.remove_left(&client_id); + // Removing from left returns rules that no longer have clients + let empty_rules_op = self.subscriptions.write().await.remove_left(&client_id); + + if empty_rules_op.is_none() { + return Ok(()); + } + + // If there are empty rules (i.e. no clients are subscribed to rule) remove that rule from topic index and rules map + let empty_rules = empty_rules_op.unwrap(); + for rule_id in empty_rules { + let mut rules_write = self.rules.write().await; + let Some(rule) = rules_write.get(&rule_id) else { + warn!("No matching rule found for rule_id {}", rule_id); + return Err(RuleManagerError::NoMatchingRule); + }; + self.topic_index.write().await.remove(&rule.topic); + rules_write.remove(&rule_id); + } Ok(()) } } From b2582c000276fc7facc34567153c362079142b51 Mon Sep 17 00:00:00 2001 From: suryatho Date: Tue, 18 Nov 2025 01:16:22 -0500 Subject: [PATCH 03/13] Improve BiMultiMap api to remove get_mut methods and also have clearer method signatures on remove methods. Fix logical error in delete_rule and delete_client, where entire topic was removed from index when rule is no longer subscribed to. --- scylla-server/src/rule_structs.rs | 275 +++++++++++++++++++++--------- 1 file changed, 193 insertions(+), 82 deletions(-) diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index da77c7aa..f7b552e9 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -17,6 +17,7 @@ use tokio::sync::RwLock; use tracing::trace; use tracing::warn; +use crate::rule_structs::BiMapRemoveResult::*; use crate::ClientData; static ASCII_LOWER: [char; 26] = [ @@ -24,7 +25,26 @@ static ASCII_LOWER: [char; 26] = [ 't', 'u', 'v', 'w', 'x', 'y', 'z', ]; -pub struct BiMultiMap { +#[allow(dead_code)] +#[derive(Debug, Clone)] +struct BiMapCleanupData { + /// left keys that were thrown out + lefts: Option>, + /// right keys that were thrown out + rights: Option>, +} + +#[derive(Debug, Clone)] +enum BiMapRemoveResult { + /// Removed succesfully, and also removed any empty mappings \ + /// Contains the data that was thrown out from the map because they were unused. + RemovedWithCleanUp(BiMapCleanupData), + /// Removed succesfully, no empty mappings to clean up + RemovedOnly, + NothingToRemove, +} + +struct BiMultiMap { left_to_right: FxHashMap>, right_to_left: FxHashMap>, } @@ -46,14 +66,6 @@ impl BiMultiMap { self.right_to_left.get(right) } - pub fn get_right_mut(&mut self, left: &L) -> Option<&mut FxHashSet> { - self.left_to_right.get_mut(left) - } - - pub fn get_left_mut(&mut self, right: &R) -> Option<&mut FxHashSet> { - self.right_to_left.get_mut(right) - } - pub fn insert(&mut self, left: &L, right: &R) { self.left_to_right .entry(left.clone()) @@ -65,51 +77,129 @@ impl BiMultiMap { .insert(left.clone()); } - /// Remove all mappings for a given left key, if none left keys remain for a right key, remove that right key as well. - /// returns: optional set of empty rights that no longer map to any lefts - pub fn remove_left(&mut self, left: &L) -> Option> { + /// Remove all mappings for a given left key, if none left keys remain for a right key, remove that right key as well. \ + /// Returns: BiMapRemoveResult with optional set of empty rights that were cleaned from map. + pub fn remove_left(&mut self, left: &L) -> BiMapRemoveResult { + let Some(rights) = self.left_to_right.remove(left) else { + return NothingToRemove; + }; + let mut empty_rights = FxHashSet::default(); - if let Some(rights) = self.left_to_right.remove(left) { - for right in rights { - if let Some(lefts) = self.right_to_left.get_mut(&right) { - lefts.remove(left); - if lefts.is_empty() { - self.right_to_left.remove(&right); - empty_rights.insert(right); - } + for right in rights { + if let Some(lefts) = self.right_to_left.get_mut(&right) { + lefts.remove(left); + if lefts.is_empty() { + self.right_to_left.remove(&right); + empty_rights.insert(right); } } } if empty_rights.is_empty() { - None + RemovedOnly } else { - Some(empty_rights) + RemovedWithCleanUp(BiMapCleanupData { + lefts: None, + rights: Some(empty_rights), + }) } } - /// Remove all mappings for a given right key, if none right keys remain for a left key, remove that left key as well. - /// returns: optional set of empty lefts that no longer map to any rights - pub fn remove_right(&mut self, right: &R) -> Option> { + /// Remove all mappings for a given right key, if none right keys remain for a left key, remove that left key as well. \ + /// Returns: BiMapRemoveResult with optional set of empty lefts that were cleaned from map. + pub fn remove_right(&mut self, right: &R) -> BiMapRemoveResult { + let Some(lefts) = self.right_to_left.remove(right) else { + return NothingToRemove; + }; + let mut empty_lefts = FxHashSet::default(); - if let Some(lefts) = self.right_to_left.remove(right) { - for left in lefts { - if let Some(rights) = self.left_to_right.get_mut(&left) { - rights.remove(right); - if rights.is_empty() { - self.left_to_right.remove(&left); - empty_lefts.insert(left); - } + for left in lefts { + if let Some(rights) = self.left_to_right.get_mut(&left) { + rights.remove(right); + if rights.is_empty() { + self.left_to_right.remove(&left); + empty_lefts.insert(left); } } } if empty_lefts.is_empty() { - None + RemovedOnly + } else { + RemovedWithCleanUp(BiMapCleanupData { + lefts: Some(empty_lefts), + rights: None, + }) + } + } + + /// Remove a specific mapping from left to right, cleaning up empty entries as needed.\ + /// Returns: BiMapRemoveresult with optional right that was cleaned from map. + pub fn remove_right_from_left(&mut self, left: &L, right: &R) -> BiMapRemoveResult { + let Some(rights) = self.left_to_right.get_mut(left) else { + return NothingToRemove; + }; + + if !rights.remove(right) { + return NothingToRemove; + } + + if rights.is_empty() { + // Kick out this left because it doesn't have any rights (?) + self.left_to_right.remove(left); + } + + if let Some(lefts) = self.right_to_left.get_mut(right) { + lefts.remove(left); + if lefts.is_empty() { + self.right_to_left.remove(right); + // TODO: make inline ? + let mut set = FxHashSet::default(); + set.insert(right.clone()); + RemovedWithCleanUp(BiMapCleanupData { + lefts: None, + rights: Some(set), + }) + } else { + RemovedOnly + } } else { - Some(empty_lefts) + NothingToRemove + } + } + + /// Remove a specific mapping from right to left, cleaning up empty entries as needed. \ + /// Returns: BiMapRemoveresult with optional left that was cleaned from map. + pub fn remove_left_from_right(&mut self, right: &R, left: &L) -> BiMapRemoveResult { + let Some(lefts) = self.right_to_left.get_mut(right) else { + return NothingToRemove; + }; + + if !lefts.remove(left) { + return NothingToRemove; + } + + if lefts.is_empty() { + self.right_to_left.remove(right); + } + + if let Some(rights) = self.left_to_right.get_mut(left) { + rights.remove(right); + if rights.is_empty() { + self.left_to_right.remove(left); + let mut set = FxHashSet::default(); + set.insert(left.clone()); + RemovedWithCleanUp(BiMapCleanupData { + lefts: Some(set), + rights: None, + }) + } else { + RemovedOnly + } + } else { + NothingToRemove } } } @@ -289,7 +379,7 @@ impl RuleManager { ) -> Result>, RuleManagerError> { // Read from topic to rule index and drop lock immediately let rule_ids = match self.topic_index.read().await.get(&data.name) { - Some(rule_ids) => rule_ids.clone(), + Some(rule_ids) => rule_ids.clone(), // Clone so we can drop resource None => { trace!("Could not find rule in topic -> rule index: {}", data.name); return Err(RuleManagerError::NoMatchingRule); @@ -380,67 +470,88 @@ impl RuleManager { rule_id: RuleId, ) -> Result<(), RuleManagerError> { // Remove rule from client - let no_more_clients = { - let mut subscriptions_write = self.subscriptions.write().await; - let Some(rules) = subscriptions_write.get_right_mut(&client_id) else { + match self + .subscriptions + .write() + .await + .remove_right_from_left(&client_id, &rule_id) + { + RemovedWithCleanUp(clean_up_data) => { + let BiMapCleanupData { + rights: Some(_), .. + } = clean_up_data + else { + unreachable!(); // remove_right_from_left always returns Some in rights if cleanup happened + // TODO: make this better + }; + // Delete rule from topic_index and rules maps + self.delete_rule_from_topic_and_rule_cache(&rule_id).await + } + RemovedOnly => Ok(()), + NothingToRemove => { warn!( "Could not find client in subscriptions bimap to delete rule: {}", client_id ); - return Err(RuleManagerError::NoSuchClient); // Keep this early return - }; - rules.remove(&rule_id); - if rules.is_empty() { - // Remove client. Since client is no longer subscribed to any rule - // no rules are removed as well as a side effect - subscriptions_write.remove_left(&client_id); - true - } else { - false + Err(RuleManagerError::NoSuchClient) } - }; - - // If no more clients exist for this rule, delete it from rules and topic index - // Gaurd - if !no_more_clients { - return Ok(()); } - - // Remove from topic index and rules map - let topic = match self.rules.read().await.get(&rule_id) { - Some(rule) => rule.topic.clone(), - None => { - warn!("No matching rule found for rule_id {}", rule_id); - return Err(RuleManagerError::NoMatchingRule); - } - }; - self.topic_index.write().await.remove(&topic); - self.rules.write().await.remove(&rule_id); - - Ok(()) } /// Deletes a client, and all of its rules. /// Removes rules that are no longer subscribed to if needed. pub async fn delete_client(&self, client_id: ClientId) -> Result<(), RuleManagerError> { // Removing from left returns rules that no longer have clients - let empty_rules_op = self.subscriptions.write().await.remove_left(&client_id); - - if empty_rules_op.is_none() { - return Ok(()); + match self.subscriptions.write().await.remove_left(&client_id) { + RemovedWithCleanUp(clean_up_data) => { + let BiMapCleanupData { + rights: Some(rule_ids), + .. + } = clean_up_data + else { + unreachable!(); // remove_left always returns Some in rights if cleanup happened + }; + for rule_id in rule_ids { + self.delete_rule_from_topic_and_rule_cache(&rule_id).await?; + } + Ok(()) + } + RemovedOnly => Ok(()), + NothingToRemove => { + warn!( + "Could not find client in subscriptions bimap to delete client: {}", + client_id + ); + Err(RuleManagerError::NoSuchClient) + } } + } - // If there are empty rules (i.e. no clients are subscribed to rule) remove that rule from topic index and rules map - let empty_rules = empty_rules_op.unwrap(); - for rule_id in empty_rules { - let mut rules_write = self.rules.write().await; - let Some(rule) = rules_write.get(&rule_id) else { - warn!("No matching rule found for rule_id {}", rule_id); - return Err(RuleManagerError::NoMatchingRule); - }; - self.topic_index.write().await.remove(&rule.topic); - rules_write.remove(&rule_id); + async fn delete_rule_from_topic_and_rule_cache( + &self, + rule_id: &RuleId, + ) -> Result<(), RuleManagerError> { + let mut rules_map_write = self.rules.write().await; + let Some(rule) = rules_map_write.get(rule_id) else { + warn!("No matching topic found for rule_id {}", rule_id); + return Err(RuleManagerError::NoMatchingRule); + }; + + let mut topic_index_write = self.topic_index.write().await; + let Some(rule_ids_for_topic) = topic_index_write.get_mut(&rule.topic) else { + warn!("No matching topic found for rule_id {}", rule_id); + return Err(RuleManagerError::NoMatchingRule); + }; + + // Remove rule from topic and if none left remove topic + rule_ids_for_topic.remove(rule_id); + if rule_ids_for_topic.is_empty() { + topic_index_write.remove(&rule.topic); } + + // Remove rule from rules map + rules_map_write.remove(rule_id); + Ok(()) } } From ea1d7c75499be260d5dbe68439d3772738bd5b82 Mon Sep 17 00:00:00 2001 From: suryatho Date: Tue, 18 Nov 2025 02:18:32 -0500 Subject: [PATCH 04/13] Use tokio::join! wherever possible for better concurrency --- scylla-server/src/rule_structs.rs | 144 ++++++++++++++++++------------ 1 file changed, 87 insertions(+), 57 deletions(-) diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index f7b552e9..d63edecc 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -388,34 +388,41 @@ impl RuleManager { let mut notifications: Vec<(ClientId, RuleNotification)> = Vec::new(); for rule_id in rule_ids { - let rule_triggered = { - let mut rules_write = self.rules.write().await; - let Some(rule) = rules_write.get_mut(&rule_id) else { - trace!("Could not find rule in rules map: {}", rule_id); - return Err(RuleManagerError::NoMatchingRule); - }; - let Some(triggered) = rule.tick(&data.values) else { - return Err(RuleManagerError::RuleFailure); - }; - triggered - }; + let (rule_triggered, clients) = tokio::join!( + async { + let mut rules_write = self.rules.write().await; + let Some(rule) = rules_write.get_mut(&rule_id) else { + trace!("Could not find rule in rules map: {}", rule_id); + return Err(RuleManagerError::NoMatchingRule); + }; + if let Some(triggered) = rule.tick(&data.values) { + Ok(triggered) + } else { + Err(RuleManagerError::RuleFailure) + } + }, + async { + match self.subscriptions.read().await.get_left(&rule_id) { + Some(clients) => Ok(clients.clone()), + None => { + trace!( + "(indexed) Could not find clients for rule in subscriptions bimap: {}", + data.name + ); + return Err(RuleManagerError::NoMatchingRule); + } + } + } + ); + // TODO: should we use concurrency here? We sometimes do not need clients if the rule isn't triggered + + let triggered = rule_triggered?; + let clients = clients?; - if !rule_triggered { + if !triggered { continue; } - // get all clients subscribed to this rule - let clients = match self.subscriptions.read().await.get_left(&rule_id) { - Some(clients) => clients.clone(), - None => { - trace!( - "(indexed) Could not find clients for rule in subscriptions bimap: {}", - data.name - ); - return Err(RuleManagerError::NoMatchingRule); - } - }; - // Clients should never be empty if clients.is_empty() { warn!("Empty subscriptions entry for rule {}!", rule_id); @@ -444,22 +451,30 @@ impl RuleManager { /// Adds a rule, creating or activating the client if needed pub async fn add_rule(&self, client: ClientId, rule: Rule) -> Result<(), RuleManagerError> { - // Add to subscriptions bimap - self.subscriptions - .write() - .await - .insert(&client, &rule.id.clone()); - - // Add to topic index - self.topic_index - .write() - .await - .entry(rule.topic.clone()) - .or_insert(FxHashSet::default()) - .insert(rule.id.clone()); + let rule_id = rule.id.clone(); + let topic = rule.topic.clone(); + + // Run all three writes concurrently + let ((), (), ()) = tokio::join!( + async { + // Add to subscriptions bimap + self.subscriptions.write().await.insert(&client, &rule_id); + }, + async { + // Add to topic index + self.topic_index + .write() + .await + .entry(topic) + .or_insert(FxHashSet::default()) + .insert(rule_id.clone()); + }, + async { + // Add to rules lookup + self.rules.write().await.insert(rule_id.clone(), rule); + } + ); - // Add to rules lookup - self.rules.write().await.insert(rule.id.clone(), rule); Ok(()) } @@ -531,27 +546,42 @@ impl RuleManager { &self, rule_id: &RuleId, ) -> Result<(), RuleManagerError> { - let mut rules_map_write = self.rules.write().await; - let Some(rule) = rules_map_write.get(rule_id) else { - warn!("No matching topic found for rule_id {}", rule_id); - return Err(RuleManagerError::NoMatchingRule); - }; - - let mut topic_index_write = self.topic_index.write().await; - let Some(rule_ids_for_topic) = topic_index_write.get_mut(&rule.topic) else { - warn!("No matching topic found for rule_id {}", rule_id); - return Err(RuleManagerError::NoMatchingRule); + let topic = { + let rules_read = self.rules.read().await; + let Some(rule) = rules_read.get(rule_id) else { + warn!("No matching topic found for rule_id {}", rule_id); + return Err(RuleManagerError::NoMatchingRule); + }; + rule.topic.clone() }; - // Remove rule from topic and if none left remove topic - rule_ids_for_topic.remove(rule_id); - if rule_ids_for_topic.is_empty() { - topic_index_write.remove(&rule.topic); - } - - // Remove rule from rules map - rules_map_write.remove(rule_id); + let (topic_result, rules_result) = tokio::join!( + async { + // Remove rule from topic index + let mut topic_index_write = self.topic_index.write().await; + let Some(rule_ids_for_topic) = topic_index_write.get_mut(&topic) else { + warn!("No matching topic found for rule_id {}", rule_id); + return Err(RuleManagerError::NoMatchingRule); + }; + // Remove rule from topic and if none left remove topic + rule_ids_for_topic.remove(rule_id); + if rule_ids_for_topic.is_empty() { + topic_index_write.remove(&topic); + } + Ok(()) + }, + async { + if self.rules.write().await.remove(rule_id).is_none() { + warn!("No matching rule found for rule_id {}", rule_id); + Err(RuleManagerError::NoMatchingRule) + } else { + Ok(()) + } + } + ); + topic_result?; + rules_result?; Ok(()) } } From 6baacb308b1da69190d7b81c5bf3feefd2a5e263 Mon Sep 17 00:00:00 2001 From: suryatho Date: Tue, 18 Nov 2025 02:57:13 -0500 Subject: [PATCH 05/13] Use generic internal static methods for BiMultiMap --- scylla-server/src/rule_structs.rs | 189 +++++++++++------------------- 1 file changed, 70 insertions(+), 119 deletions(-) diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index d63edecc..c21f93c8 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -25,20 +25,11 @@ static ASCII_LOWER: [char; 26] = [ 't', 'u', 'v', 'w', 'x', 'y', 'z', ]; -#[allow(dead_code)] -#[derive(Debug, Clone)] -struct BiMapCleanupData { - /// left keys that were thrown out - lefts: Option>, - /// right keys that were thrown out - rights: Option>, -} - #[derive(Debug, Clone)] -enum BiMapRemoveResult { +enum BiMapRemoveResult { /// Removed succesfully, and also removed any empty mappings \ /// Contains the data that was thrown out from the map because they were unused. - RemovedWithCleanUp(BiMapCleanupData), + RemovedWithCleanUp(T), /// Removed succesfully, no empty mappings to clean up RemovedOnly, NothingToRemove, @@ -79,122 +70,96 @@ impl BiMultiMap { /// Remove all mappings for a given left key, if none left keys remain for a right key, remove that right key as well. \ /// Returns: BiMapRemoveResult with optional set of empty rights that were cleaned from map. - pub fn remove_left(&mut self, left: &L) -> BiMapRemoveResult { - let Some(rights) = self.left_to_right.remove(left) else { - return NothingToRemove; - }; - - let mut empty_rights = FxHashSet::default(); - - for right in rights { - if let Some(lefts) = self.right_to_left.get_mut(&right) { - lefts.remove(left); - if lefts.is_empty() { - self.right_to_left.remove(&right); - empty_rights.insert(right); - } - } - } - - if empty_rights.is_empty() { - RemovedOnly - } else { - RemovedWithCleanUp(BiMapCleanupData { - lefts: None, - rights: Some(empty_rights), - }) - } + pub fn remove_left(&mut self, left: &L) -> BiMapRemoveResult> { + Self::remove_key(&mut self.left_to_right, &mut self.right_to_left, left) } /// Remove all mappings for a given right key, if none right keys remain for a left key, remove that left key as well. \ /// Returns: BiMapRemoveResult with optional set of empty lefts that were cleaned from map. - pub fn remove_right(&mut self, right: &R) -> BiMapRemoveResult { - let Some(lefts) = self.right_to_left.remove(right) else { - return NothingToRemove; - }; - - let mut empty_lefts = FxHashSet::default(); - - for left in lefts { - if let Some(rights) = self.left_to_right.get_mut(&left) { - rights.remove(right); - if rights.is_empty() { - self.left_to_right.remove(&left); - empty_lefts.insert(left); - } - } - } - - if empty_lefts.is_empty() { - RemovedOnly - } else { - RemovedWithCleanUp(BiMapCleanupData { - lefts: Some(empty_lefts), - rights: None, - }) - } + pub fn remove_right(&mut self, right: &R) -> BiMapRemoveResult> { + Self::remove_key(&mut self.right_to_left, &mut self.left_to_right, right) } /// Remove a specific mapping from left to right, cleaning up empty entries as needed.\ /// Returns: BiMapRemoveresult with optional right that was cleaned from map. - pub fn remove_right_from_left(&mut self, left: &L, right: &R) -> BiMapRemoveResult { - let Some(rights) = self.left_to_right.get_mut(left) else { - return NothingToRemove; - }; + pub fn remove_right_from_left(&mut self, left: &L, right: &R) -> BiMapRemoveResult { + Self::remove_mapping( + &mut self.left_to_right, + &mut self.right_to_left, + left, + right, + ) + } - if !rights.remove(right) { + /// Remove a specific mapping from right to left, cleaning up empty entries as needed. \ + /// Returns: BiMapRemoveresult with optional left that was cleaned from map. + pub fn remove_left_from_right(&mut self, right: &R, left: &L) -> BiMapRemoveResult { + Self::remove_mapping( + &mut self.right_to_left, + &mut self.left_to_right, + right, + left, + ) + } + + fn remove_key( + k_to_v: &mut FxHashMap>, + v_to_k: &mut FxHashMap>, + key: &K, + ) -> BiMapRemoveResult> + where + K: Hash + Eq + Clone, + V: Hash + Eq + Clone, + { + let Some(values) = k_to_v.remove(key) else { return NothingToRemove; - } + }; - if rights.is_empty() { - // Kick out this left because it doesn't have any rights (?) - self.left_to_right.remove(left); + let mut empty_values = FxHashSet::default(); + for value in values { + if let Some(keys) = v_to_k.get_mut(&value) { + keys.remove(key); + if keys.is_empty() { + v_to_k.remove(&value); + empty_values.insert(value); + } + } } - if let Some(lefts) = self.right_to_left.get_mut(right) { - lefts.remove(left); - if lefts.is_empty() { - self.right_to_left.remove(right); - // TODO: make inline ? - let mut set = FxHashSet::default(); - set.insert(right.clone()); - RemovedWithCleanUp(BiMapCleanupData { - lefts: None, - rights: Some(set), - }) - } else { - RemovedOnly - } + if empty_values.is_empty() { + RemovedOnly } else { - NothingToRemove + RemovedWithCleanUp(empty_values) } } - /// Remove a specific mapping from right to left, cleaning up empty entries as needed. \ - /// Returns: BiMapRemoveresult with optional left that was cleaned from map. - pub fn remove_left_from_right(&mut self, right: &R, left: &L) -> BiMapRemoveResult { - let Some(lefts) = self.right_to_left.get_mut(right) else { + fn remove_mapping( + k_to_v: &mut FxHashMap>, + v_to_k: &mut FxHashMap>, + key: &K, + value: &V, + ) -> BiMapRemoveResult + where + K: Hash + Eq + Clone, + V: Hash + Eq + Clone, + { + let Some(values) = k_to_v.get_mut(key) else { return NothingToRemove; }; - if !lefts.remove(left) { + if !values.remove(value) { return NothingToRemove; } - if lefts.is_empty() { - self.right_to_left.remove(right); + if values.is_empty() { + k_to_v.remove(key); } - if let Some(rights) = self.left_to_right.get_mut(left) { - rights.remove(right); - if rights.is_empty() { - self.left_to_right.remove(left); - let mut set = FxHashSet::default(); - set.insert(left.clone()); - RemovedWithCleanUp(BiMapCleanupData { - lefts: Some(set), - rights: None, - }) + if let Some(keys) = v_to_k.get_mut(value) { + keys.remove(key); + if keys.is_empty() { + v_to_k.remove(value); + RemovedWithCleanUp(value.clone()) } else { RemovedOnly } @@ -491,15 +456,8 @@ impl RuleManager { .await .remove_right_from_left(&client_id, &rule_id) { - RemovedWithCleanUp(clean_up_data) => { - let BiMapCleanupData { - rights: Some(_), .. - } = clean_up_data - else { - unreachable!(); // remove_right_from_left always returns Some in rights if cleanup happened - // TODO: make this better - }; - // Delete rule from topic_index and rules maps + RemovedWithCleanUp(rule_id) => { + // No more clients for this rule, delete it from topic and rule cache self.delete_rule_from_topic_and_rule_cache(&rule_id).await } RemovedOnly => Ok(()), @@ -518,14 +476,7 @@ impl RuleManager { pub async fn delete_client(&self, client_id: ClientId) -> Result<(), RuleManagerError> { // Removing from left returns rules that no longer have clients match self.subscriptions.write().await.remove_left(&client_id) { - RemovedWithCleanUp(clean_up_data) => { - let BiMapCleanupData { - rights: Some(rule_ids), - .. - } = clean_up_data - else { - unreachable!(); // remove_left always returns Some in rights if cleanup happened - }; + RemovedWithCleanUp(rule_ids) => { for rule_id in rule_ids { self.delete_rule_from_topic_and_rule_cache(&rule_id).await?; } From 151dd1e8fa3dcb35422db4d305d5bc0a66f5445a Mon Sep 17 00:00:00 2001 From: suryatho Date: Tue, 18 Nov 2025 12:08:43 -0500 Subject: [PATCH 06/13] Use branching concurrency inside handle_msg --- scylla-server/src/rule_structs.rs | 38 +++++++++++++++++++++++-------- 1 file changed, 28 insertions(+), 10 deletions(-) diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index c21f93c8..e9ad00ce 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -11,6 +11,7 @@ use serde::{Deserialize, Serialize}; use serde_with::serde_as; use serde_with::DurationSeconds; use std::borrow::Borrow; +use std::collections::HashSet; use std::hash::Hash; use std::time::Duration; use tokio::sync::RwLock; @@ -353,8 +354,8 @@ impl RuleManager { let mut notifications: Vec<(ClientId, RuleNotification)> = Vec::new(); for rule_id in rule_ids { - let (rule_triggered, clients) = tokio::join!( - async { + let (rule_triggered, clients) = { + let triggered_future = async { let mut rules_write = self.rules.write().await; let Some(rule) = rules_write.get_mut(&rule_id) else { trace!("Could not find rule in rules map: {}", rule_id); @@ -365,29 +366,45 @@ impl RuleManager { } else { Err(RuleManagerError::RuleFailure) } - }, - async { + }; + + let clients_future = async { match self.subscriptions.read().await.get_left(&rule_id) { Some(clients) => Ok(clients.clone()), None => { trace!( - "(indexed) Could not find clients for rule in subscriptions bimap: {}", + "Could not find clients for rule in subscriptions bimap: {}", data.name ); return Err(RuleManagerError::NoMatchingRule); } } + }; + + tokio::pin!(triggered_future); + tokio::pin!(clients_future); + + // Check which operation finished first + tokio::select! { + triggered_result = &mut triggered_future => { + match triggered_result? { + true => (Ok(true), clients_future.await), + false => (Ok(false), Ok(FxHashSet::default())), + } + }, + clients_result = &mut clients_future => { + let triggered_result = triggered_future.await; + (triggered_result, clients_result) + } } - ); - // TODO: should we use concurrency here? We sometimes do not need clients if the rule isn't triggered + }; let triggered = rule_triggered?; - let clients = clients?; - if !triggered { continue; } + let clients = clients?; // Clients should never be empty if clients.is_empty() { warn!("Empty subscriptions entry for rule {}!", rule_id); @@ -443,7 +460,8 @@ impl RuleManager { Ok(()) } - /// Deletes a rule from client, if no more clients exist for the rule, delete it entirely + /// Deletes a rule from client, if no more clients exist for the rule, delete it entirely. \ + /// If no more rules exist for that client, the client is also removed. pub async fn delete_rule( &self, client_id: ClientId, From afd5d0f73c49c6df0692295eaab515b6d5729e8f Mon Sep 17 00:00:00 2001 From: suryatho Date: Wed, 19 Nov 2025 15:51:30 -0500 Subject: [PATCH 07/13] Make sure rule manager can't delete rule by client unsubscribing to rule manager #475 --- scylla-server/src/rule_structs.rs | 96 +++++-------------------------- 1 file changed, 14 insertions(+), 82 deletions(-) diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index e9ad00ce..d3507d14 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -11,7 +11,6 @@ use serde::{Deserialize, Serialize}; use serde_with::serde_as; use serde_with::DurationSeconds; use std::borrow::Borrow; -use std::collections::HashSet; use std::hash::Hash; use std::time::Duration; use tokio::sync::RwLock; @@ -354,7 +353,7 @@ impl RuleManager { let mut notifications: Vec<(ClientId, RuleNotification)> = Vec::new(); for rule_id in rule_ids { - let (rule_triggered, clients) = { + let (triggered_result, clients_result) = { let triggered_future = async { let mut rules_write = self.rules.write().await; let Some(rule) = rules_write.get_mut(&rule_id) else { @@ -368,18 +367,8 @@ impl RuleManager { } }; - let clients_future = async { - match self.subscriptions.read().await.get_left(&rule_id) { - Some(clients) => Ok(clients.clone()), - None => { - trace!( - "Could not find clients for rule in subscriptions bimap: {}", - data.name - ); - return Err(RuleManagerError::NoMatchingRule); - } - } - }; + let clients_future = + async { self.subscriptions.read().await.get_left(&rule_id).cloned() }; tokio::pin!(triggered_future); tokio::pin!(clients_future); @@ -389,29 +378,25 @@ impl RuleManager { triggered_result = &mut triggered_future => { match triggered_result? { true => (Ok(true), clients_future.await), - false => (Ok(false), Ok(FxHashSet::default())), + false => (Ok(false), None), } }, clients_result = &mut clients_future => { - let triggered_result = triggered_future.await; - (triggered_result, clients_result) + match clients_result { + Some(_) => (triggered_future.await, clients_result), + None => (Ok(false), None) + } } } }; - let triggered = rule_triggered?; - if !triggered { + let triggered = triggered_result?; + if !triggered || clients_result.is_none() { continue; } - let clients = clients?; - // Clients should never be empty - if clients.is_empty() { - warn!("Empty subscriptions entry for rule {}!", rule_id); - } - // Push notifications for all clients who are subscribed to this rule - for client in clients { + for client in clients_result.unwrap() { notifications.push(( client.clone(), RuleNotification { @@ -437,7 +422,7 @@ impl RuleManager { let topic = rule.topic.clone(); // Run all three writes concurrently - let ((), (), ()) = tokio::join!( + tokio::join!( async { // Add to subscriptions bimap self.subscriptions.write().await.insert(&client, &rule_id); @@ -474,11 +459,7 @@ impl RuleManager { .await .remove_right_from_left(&client_id, &rule_id) { - RemovedWithCleanUp(rule_id) => { - // No more clients for this rule, delete it from topic and rule cache - self.delete_rule_from_topic_and_rule_cache(&rule_id).await - } - RemovedOnly => Ok(()), + RemovedWithCleanUp(_) | RemovedOnly => Ok(()), NothingToRemove => { warn!( "Could not find client in subscriptions bimap to delete rule: {}", @@ -494,13 +475,7 @@ impl RuleManager { pub async fn delete_client(&self, client_id: ClientId) -> Result<(), RuleManagerError> { // Removing from left returns rules that no longer have clients match self.subscriptions.write().await.remove_left(&client_id) { - RemovedWithCleanUp(rule_ids) => { - for rule_id in rule_ids { - self.delete_rule_from_topic_and_rule_cache(&rule_id).await?; - } - Ok(()) - } - RemovedOnly => Ok(()), + RemovedWithCleanUp(_) | RemovedOnly => Ok(()), NothingToRemove => { warn!( "Could not find client in subscriptions bimap to delete client: {}", @@ -510,47 +485,4 @@ impl RuleManager { } } } - - async fn delete_rule_from_topic_and_rule_cache( - &self, - rule_id: &RuleId, - ) -> Result<(), RuleManagerError> { - let topic = { - let rules_read = self.rules.read().await; - let Some(rule) = rules_read.get(rule_id) else { - warn!("No matching topic found for rule_id {}", rule_id); - return Err(RuleManagerError::NoMatchingRule); - }; - rule.topic.clone() - }; - - let (topic_result, rules_result) = tokio::join!( - async { - // Remove rule from topic index - let mut topic_index_write = self.topic_index.write().await; - let Some(rule_ids_for_topic) = topic_index_write.get_mut(&topic) else { - warn!("No matching topic found for rule_id {}", rule_id); - return Err(RuleManagerError::NoMatchingRule); - }; - // Remove rule from topic and if none left remove topic - rule_ids_for_topic.remove(rule_id); - if rule_ids_for_topic.is_empty() { - topic_index_write.remove(&topic); - } - Ok(()) - }, - async { - if self.rules.write().await.remove(rule_id).is_none() { - warn!("No matching rule found for rule_id {}", rule_id); - Err(RuleManagerError::NoMatchingRule) - } else { - Ok(()) - } - } - ); - - topic_result?; - rules_result?; - Ok(()) - } } From 340a0801d8acc169912d1c3e74a01a66fc704eb9 Mon Sep 17 00:00:00 2001 From: suryatho Date: Sat, 22 Nov 2025 22:57:56 -0500 Subject: [PATCH 08/13] Add test file for testing functionality of bimultimap and rule manager in rule_structs.rs #475 --- scylla-server/src/rule_structs.rs | 39 +- scylla-server/tests/rule_structs_test.rs | 789 +++++++++++++++++++++++ 2 files changed, 815 insertions(+), 13 deletions(-) create mode 100644 scylla-server/tests/rule_structs_test.rs diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index d3507d14..52c9fa75 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -26,7 +26,7 @@ static ASCII_LOWER: [char; 26] = [ ]; #[derive(Debug, Clone)] -enum BiMapRemoveResult { +pub enum BiMapRemoveResult { /// Removed succesfully, and also removed any empty mappings \ /// Contains the data that was thrown out from the map because they were unused. RemovedWithCleanUp(T), @@ -35,7 +35,7 @@ enum BiMapRemoveResult { NothingToRemove, } -struct BiMultiMap { +pub struct BiMultiMap { left_to_right: FxHashMap>, right_to_left: FxHashMap>, } @@ -187,7 +187,7 @@ pub struct RuleId(pub String); /// a MQTT topic to trigger on, add to derives to get more string features #[derive(PartialEq, Eq, Hash, Display, Clone, Serialize, Deserialize)] -pub struct Topic(String); +pub struct Topic(pub String); impl Borrow for Topic { fn borrow(&self) -> &str { @@ -210,7 +210,7 @@ pub struct RuleNotification { } #[serde_as] -#[derive(Deserialize)] +#[derive(Deserialize, Clone)] /// A single modular rule, can be serial/deserialized pub struct Rule { id: RuleId, @@ -306,6 +306,7 @@ impl Rule { } /// errors seen in the rule manager +#[derive(Debug)] pub enum RuleManagerError { NoMatchingRule, NoSuchClient, @@ -354,19 +355,21 @@ impl RuleManager { let mut notifications: Vec<(ClientId, RuleNotification)> = Vec::new(); for rule_id in rule_ids { let (triggered_result, clients_result) = { + // Future for if rule was triggered let triggered_future = async { - let mut rules_write = self.rules.write().await; - let Some(rule) = rules_write.get_mut(&rule_id) else { - trace!("Could not find rule in rules map: {}", rule_id); - return Err(RuleManagerError::NoMatchingRule); - }; - if let Some(triggered) = rule.tick(&data.values) { - Ok(triggered) + if let Some(rule) = self.rules.write().await.get_mut(&rule_id) { + if let Some(triggered) = rule.tick(&data.values) { + Ok(triggered) + } else { + Err(RuleManagerError::RuleFailure) + } } else { - Err(RuleManagerError::RuleFailure) + trace!("Could not find rule in rules map: {}", rule_id); + Err(RuleManagerError::NoMatchingRule) } }; + // Future for getting subscribed clients let clients_future = async { self.subscriptions.read().await.get_left(&rule_id).cloned() }; @@ -445,7 +448,7 @@ impl RuleManager { Ok(()) } - /// Deletes a rule from client, if no more clients exist for the rule, delete it entirely. \ + /// Deletes a rule from client. \ /// If no more rules exist for that client, the client is also removed. pub async fn delete_rule( &self, @@ -485,4 +488,14 @@ impl RuleManager { } } } + + pub async fn get_all_rules(&self) -> Vec { + self.rules + .read() + .await + .values() + .into_iter() + .map(|rule| rule.clone()) + .collect() + } } diff --git a/scylla-server/tests/rule_structs_test.rs b/scylla-server/tests/rule_structs_test.rs new file mode 100644 index 00000000..1578aba9 --- /dev/null +++ b/scylla-server/tests/rule_structs_test.rs @@ -0,0 +1,789 @@ +use chrono::Utc; +use scylla_server::rule_structs::*; +use scylla_server::ClientData; + +// Tests for BiMultiMap +#[test] +fn test_bi_multi_map_new() { + let bimap: BiMultiMap = BiMultiMap::new(); + assert!(bimap.get_left(&1).is_none()); + assert!(bimap.get_right(&"test".to_string()).is_none()); +} + +#[test] +fn test_bi_multi_map_insert_single() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right = 42; + + bimap.insert(&left, &right); + + assert_eq!(bimap.get_right(&left).unwrap().len(), 1); + assert!(bimap.get_right(&left).unwrap().contains(&right)); + + assert_eq!(bimap.get_left(&right).unwrap().len(), 1); + assert!(bimap.get_left(&right).unwrap().contains(&left)); +} + +#[test] +fn test_bi_multi_map_insert_multiple_rights() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right1 = 42; + let right2 = 43; + let right3 = 44; + + bimap.insert(&left, &right1); + bimap.insert(&left, &right2); + bimap.insert(&left, &right3); + + let rights = bimap.get_right(&left).unwrap(); + assert_eq!(rights.len(), 3); + assert!(rights.contains(&right1)); + assert!(rights.contains(&right2)); + assert!(rights.contains(&right3)); + + // Each right should map back to the left + assert!(bimap.get_left(&right1).unwrap().contains(&left)); + assert!(bimap.get_left(&right2).unwrap().contains(&left)); + assert!(bimap.get_left(&right3).unwrap().contains(&left)); +} + +#[test] +fn test_bi_multi_map_insert_multiple_lefts() { + let mut bimap = BiMultiMap::new(); + let left1 = "client1".to_string(); + let left2 = "client2".to_string(); + let left3 = "client3".to_string(); + let right = 42; + + bimap.insert(&left1, &right); + bimap.insert(&left2, &right); + bimap.insert(&left3, &right); + + let lefts = bimap.get_left(&right).unwrap(); + assert_eq!(lefts.len(), 3); + assert!(lefts.contains(&left1)); + assert!(lefts.contains(&left2)); + assert!(lefts.contains(&left3)); + + // Each left should map to the right + assert!(bimap.get_right(&left1).unwrap().contains(&right)); + assert!(bimap.get_right(&left2).unwrap().contains(&right)); + assert!(bimap.get_right(&left3).unwrap().contains(&right)); +} + +#[test] +fn test_bi_multi_map_insert_many_to_many() { + let mut bimap = BiMultiMap::new(); + let left1 = "client1".to_string(); + let left2 = "client2".to_string(); + let right1 = 42; + let right2 = 43; + + // Create many-to-many relationships + bimap.insert(&left1, &right1); + bimap.insert(&left1, &right2); + bimap.insert(&left2, &right1); + bimap.insert(&left2, &right2); + + // Verify left1 maps to both rights + let rights_for_left1 = bimap.get_right(&left1).unwrap(); + assert_eq!(rights_for_left1.len(), 2); + assert!(rights_for_left1.contains(&right1)); + assert!(rights_for_left1.contains(&right2)); + + // Verify left2 maps to both rights + let rights_for_left2 = bimap.get_right(&left2).unwrap(); + assert_eq!(rights_for_left2.len(), 2); + assert!(rights_for_left2.contains(&right1)); + assert!(rights_for_left2.contains(&right2)); + + // Verify right1 maps to both lefts + let lefts_for_right1 = bimap.get_left(&right1).unwrap(); + assert_eq!(lefts_for_right1.len(), 2); + assert!(lefts_for_right1.contains(&left1)); + assert!(lefts_for_right1.contains(&left2)); + + // Verify right2 maps to both lefts + let lefts_for_right2 = bimap.get_left(&right2).unwrap(); + assert_eq!(lefts_for_right2.len(), 2); + assert!(lefts_for_right2.contains(&left1)); + assert!(lefts_for_right2.contains(&left2)); +} + +#[test] +fn test_bi_multi_map_insert_duplicate() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right = 42; + + bimap.insert(&left, &right); + bimap.insert(&left, &right); // Duplicate insertion + + // Should still only have one mapping + assert_eq!(bimap.get_right(&left).unwrap().len(), 1); + assert_eq!(bimap.get_left(&right).unwrap().len(), 1); +} + +#[test] +fn test_bi_multi_map_remove_left_single() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right = 42; + + bimap.insert(&left, &right); + + let result = bimap.remove_left(&left); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_rights) = result { + assert_eq!(removed_rights.len(), 1); + assert!(removed_rights.contains(&right)); + } + + // Verify both directions are cleaned up + assert!(bimap.get_right(&left).is_none()); + assert!(bimap.get_left(&right).is_none()); +} + +#[test] +fn test_bi_multi_map_remove_left_shared_right() { + let mut bimap = BiMultiMap::new(); + let left1 = "client1".to_string(); + let left2 = "client2".to_string(); + let right = 42; + + bimap.insert(&left1, &right); + bimap.insert(&left2, &right); + + let result = bimap.remove_left(&left1); + assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); + + // left1 should be gone + assert!(bimap.get_right(&left1).is_none()); + + // right should still exist and map to left2 + let remaining_lefts = bimap.get_left(&right).unwrap(); + assert_eq!(remaining_lefts.len(), 1); + assert!(remaining_lefts.contains(&left2)); + + // left2 should still map to right + assert!(bimap.get_right(&left2).unwrap().contains(&right)); +} + +#[test] +fn test_bi_multi_map_remove_left_nonexistent() { + let mut bimap: BiMultiMap = BiMultiMap::new(); + let left = "nonexistent".to_string(); + + let result = bimap.remove_left(&left); + assert!(matches!(result, BiMapRemoveResult::NothingToRemove)); +} + +#[test] +fn test_bi_multi_map_remove_right_single() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right = 42; + + bimap.insert(&left, &right); + + let result = bimap.remove_right(&right); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_lefts) = result { + assert_eq!(removed_lefts.len(), 1); + assert!(removed_lefts.contains(&left)); + } + + // Verify both directions are cleaned up + assert!(bimap.get_right(&left).is_none()); + assert!(bimap.get_left(&right).is_none()); +} + +#[test] +fn test_bi_multi_map_remove_right_shared_left() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right1 = 42; + let right2 = 43; + + bimap.insert(&left, &right1); + bimap.insert(&left, &right2); + + let result = bimap.remove_right(&right1); + assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); + + // right1 should be gone + assert!(bimap.get_left(&right1).is_none()); + + // left should still exist and map to right2 + let remaining_rights = bimap.get_right(&left).unwrap(); + assert_eq!(remaining_rights.len(), 1); + assert!(remaining_rights.contains(&right2)); + + // right2 should still map to left + assert!(bimap.get_left(&right2).unwrap().contains(&left)); +} + +#[test] +fn test_bi_multi_map_remove_right_from_left() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right1 = 42; + let right2 = 43; + + bimap.insert(&left, &right1); + bimap.insert(&left, &right2); + + let result = bimap.remove_right_from_left(&left, &right1); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_right) = result { + assert_eq!(removed_right, right1); + } + + // left should still exist but only map to right2 + let remaining_rights = bimap.get_right(&left).unwrap(); + assert_eq!(remaining_rights.len(), 1); + assert!(remaining_rights.contains(&right2)); + + // right1 should be completely removed + assert!(bimap.get_left(&right1).is_none()); + + // right2 should still map to left + assert!(bimap.get_left(&right2).unwrap().contains(&left)); +} + +#[test] +fn test_bi_multi_map_remove_right_from_left_shared_right() { + let mut bimap = BiMultiMap::new(); + let left1 = "client1".to_string(); + let left2 = "client2".to_string(); + let right = 42; + + bimap.insert(&left1, &right); + bimap.insert(&left2, &right); + + let result = bimap.remove_right_from_left(&left1, &right); + assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); + + // left1 should be gone + assert!(bimap.get_right(&left1).is_none()); + + // right should still exist and map to left2 + let remaining_lefts = bimap.get_left(&right).unwrap(); + assert_eq!(remaining_lefts.len(), 1); + assert!(remaining_lefts.contains(&left2)); +} + +#[test] +fn test_bi_multi_map_remove_right_from_left_nonexistent() { + let mut bimap: BiMultiMap = BiMultiMap::new(); + let left = "client1".to_string(); + let right = 42; + + let result = bimap.remove_right_from_left(&left, &right); + assert!(matches!(result, BiMapRemoveResult::NothingToRemove)); +} + +#[test] +fn test_bi_multi_map_remove_left_from_right() { + let mut bimap = BiMultiMap::new(); + let left1 = "client1".to_string(); + let left2 = "client2".to_string(); + let right = 42; + + bimap.insert(&left1, &right); + bimap.insert(&left2, &right); + + let result = bimap.remove_left_from_right(&right, &left1); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_left) = result { + assert_eq!(removed_left, left1); + } + + // right should still exist but only map to left2 + let remaining_lefts = bimap.get_left(&right).unwrap(); + assert_eq!(remaining_lefts.len(), 1); + assert!(remaining_lefts.contains(&left2)); + + // left1 should be completely removed + assert!(bimap.get_right(&left1).is_none()); + + // left2 should still map to right + assert!(bimap.get_right(&left2).unwrap().contains(&right)); +} + +#[test] +fn test_bi_multi_map_complex_operations() { + let mut bimap = BiMultiMap::new(); + + // Set up a complex mapping + bimap.insert(&"client1", &"rule1"); + bimap.insert(&"client1", &"rule2"); + bimap.insert(&"client2", &"rule1"); + bimap.insert(&"client2", &"rule3"); + bimap.insert(&"client3", &"rule3"); + + // Remove a shared rule from one client + let result = bimap.remove_right_from_left(&"client1", &"rule1"); + assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); + + // Verify rule1 still exists for client2 + assert!(bimap.get_right(&"client2").unwrap().contains(&"rule1")); + + // Verify client1 still has rule2 + assert!(bimap.get_right(&"client1").unwrap().contains(&"rule2")); + assert!(!bimap.get_right(&"client1").unwrap().contains(&"rule1")); + + // Remove client2 entirely + let result = bimap.remove_left(&"client2"); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_rights) = result { + assert_eq!(removed_rights.len(), 1); + assert!(removed_rights.contains(&"rule1")); // rule1 should be cleaned up + } + + // rule3 should still exist for client3 + assert!(bimap.get_left(&"rule3").unwrap().contains(&"client3")); + + // rule1 should be completely gone + assert!(bimap.get_left(&"rule1").is_none()); +} + +#[test] +fn test_bi_multi_map_with_rule_manager_types() { + let mut bimap: BiMultiMap = BiMultiMap::new(); + + let client1 = ClientId("client1".to_string()); + let client2 = ClientId("client2".to_string()); + let rule1 = RuleId("rule1".to_string()); + let rule2 = RuleId("rule2".to_string()); + + bimap.insert(&client1, &rule1); + bimap.insert(&client1, &rule2); + bimap.insert(&client2, &rule1); + + // Test with actual types used in RuleManager + assert_eq!(bimap.get_right(&client1).unwrap().len(), 2); + assert_eq!(bimap.get_left(&rule1).unwrap().len(), 2); + + let result = bimap.remove_left(&client1); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_rules) = result { + assert_eq!(removed_rules.len(), 1); + assert!(removed_rules.contains(&rule2)); // rule2 should be cleaned up + } + + // rule1 should still exist for client2 + assert!(bimap.get_left(&rule1).unwrap().contains(&client2)); +} + +// Tests for rule manager +#[tokio::test] +async fn test_add_one_rule() -> Result<(), RuleManagerError> { + let rule_manager = RuleManager::new(); + + let client = ClientId("test_client".to_string()); + let rule = Rule::new( + RuleId("rule_1".to_string()), + Topic("test/topic".to_string()), + core::time::Duration::from_secs(60), + "value > 10".to_owned(), + ); + + rule_manager.add_rule(client, rule.clone()).await?; + + assert_eq!(rule_manager.get_all_rules().await.len(), 1); + assert_eq!( + &rule_manager.get_all_rules().await[0].topic.0, + &rule.topic.0 + ); + + Ok(()) +} + +#[tokio::test] +async fn test_add_multiple_rules_same_client() -> Result<(), RuleManagerError> { + let rule_manager = RuleManager::new(); + let client = ClientId("test_client".to_string()); + + let rule1 = Rule::new( + RuleId("rule_1".to_string()), + Topic("test/topic1".to_string()), + core::time::Duration::from_secs(60), + "a > 10".to_owned(), + ); + + let rule2 = Rule::new( + RuleId("rule_2".to_string()), + Topic("test/topic2".to_string()), + core::time::Duration::from_secs(30), + "b < 5".to_owned(), + ); + + rule_manager.add_rule(client.clone(), rule1).await?; + rule_manager.add_rule(client, rule2).await?; + + assert_eq!(rule_manager.get_all_rules().await.len(), 2); + Ok(()) +} + +#[tokio::test] +async fn test_add_multiple_clients_same_rule_topic() -> Result<(), RuleManagerError> { + let rule_manager = RuleManager::new(); + + let client1 = ClientId("client1".to_string()); + let client2 = ClientId("client2".to_string()); + + let rule1 = Rule::new( + RuleId("rule_1".to_string()), + Topic("shared/topic".to_string()), + core::time::Duration::from_secs(60), + "a > 10".to_owned(), + ); + + let rule2 = Rule::new( + RuleId("rule_2".to_string()), + Topic("shared/topic".to_string()), + core::time::Duration::from_secs(30), + "a < 5".to_owned(), + ); + + rule_manager.add_rule(client1, rule1).await?; + rule_manager.add_rule(client2, rule2).await?; + + assert_eq!(rule_manager.get_all_rules().await.len(), 2); + Ok(()) +} + +#[tokio::test] +async fn test_delete_rule_success() -> Result<(), RuleManagerError> { + let rule_manager = RuleManager::new(); + let client = ClientId("test_client".to_string()); + let rule_id = RuleId("rule_1".to_string()); + + let rule = Rule::new( + rule_id.clone(), + Topic("test/topic".to_string()), + core::time::Duration::from_secs(60), + "a > 10".to_owned(), + ); + + rule_manager.add_rule(client.clone(), rule).await?; + assert_eq!(rule_manager.get_all_rules().await.len(), 1); + + rule_manager.delete_rule(client, rule_id).await?; + assert_eq!(rule_manager.get_all_rules().await.len(), 1); // Rule still exists but client is unsubscribed + + Ok(()) +} + +#[tokio::test] +async fn test_delete_rule_nonexistent_client() { + let rule_manager = RuleManager::new(); + let client = ClientId("nonexistent_client".to_string()); + let rule_id = RuleId("nonexistent_rule".to_string()); + + let result = rule_manager.delete_rule(client, rule_id).await; + assert!(matches!(result, Err(RuleManagerError::NoSuchClient))); +} + +#[tokio::test] +async fn test_delete_client_success() -> Result<(), RuleManagerError> { + let rule_manager = RuleManager::new(); + let client = ClientId("test_client".to_string()); + + let rule1 = Rule::new( + RuleId("rule_1".to_string()), + Topic("test/topic1".to_string()), + core::time::Duration::from_secs(60), + "a > 10".to_owned(), + ); + + let rule2 = Rule::new( + RuleId("rule_2".to_string()), + Topic("test/topic2".to_string()), + core::time::Duration::from_secs(30), + "b < 5".to_owned(), + ); + + rule_manager.add_rule(client.clone(), rule1).await?; + rule_manager.add_rule(client.clone(), rule2).await?; + assert_eq!(rule_manager.get_all_rules().await.len(), 2); + + rule_manager.delete_client(client).await?; + // Rules still exist in the system but client is removed from subscriptions + assert_eq!(rule_manager.get_all_rules().await.len(), 2); + + Ok(()) +} + +#[tokio::test] +async fn test_delete_client_nonexistent() { + let rule_manager = RuleManager::new(); + let client = ClientId("nonexistent_client".to_string()); + + let result = rule_manager.delete_client(client).await; + assert!(matches!(result, Err(RuleManagerError::NoSuchClient))); +} + +#[tokio::test] +async fn test_handle_msg_no_matching_rule() { + let rule_manager = RuleManager::new(); + + let client_data = ClientData { + run_id: 1, + name: "nonexistent/topic".to_string(), + unit: "test_unit".to_string(), + values: vec![15.0, 20.0], + timestamp: Utc::now(), + }; + + let result = rule_manager.handle_msg(&client_data).await; + assert!(matches!(result, Err(RuleManagerError::NoMatchingRule))); +} + +#[tokio::test] +async fn test_handle_msg_rule_triggered() -> Result<(), RuleManagerError> { + let rule_manager = RuleManager::new(); + let client = ClientId("test_client".to_string()); + + let rule = Rule::new( + RuleId("rule_1".to_string()), + Topic("test/topic".to_string()), + core::time::Duration::from_millis(100), // Short debounce for testing + "a > 10".to_owned(), // First value (a) should be > 10 + ); + + rule_manager.add_rule(client.clone(), rule).await?; + + let client_data = ClientData { + run_id: 1, + name: "test/topic".to_string(), + unit: "test_unit".to_string(), + values: vec![15.0], // a = 15.0 > 10, should trigger + timestamp: Utc::now(), + }; + + // First trigger might not fire due to debounce logic + let _ = rule_manager.handle_msg(&client_data).await; + + // Wait for debounce time + tokio::time::sleep(tokio::time::Duration::from_millis(150)).await; + + let result = rule_manager.handle_msg(&client_data).await?; + + let notifications = result.unwrap(); + assert!(!notifications.is_empty()); + assert_eq!(notifications[0].0 .0, client.0); + assert_eq!(notifications[0].1.topic.0, "test/topic"); + + Ok(()) +} + +#[tokio::test] +async fn test_handle_msg_rule_not_triggered() -> Result<(), RuleManagerError> { + let rule_manager = RuleManager::new(); + let client = ClientId("test_client".to_string()); + + let rule = Rule::new( + RuleId("rule_1".to_string()), + Topic("test/topic".to_string()), + core::time::Duration::from_secs(1), + "a > 10".to_owned(), + ); + + rule_manager.add_rule(client, rule).await?; + + let client_data = ClientData { + run_id: 1, + name: "test/topic".to_string(), + unit: "test_unit".to_string(), + values: vec![5.0], // a = 5.0 < 10, should not trigger + timestamp: Utc::now(), + }; + + let result = rule_manager.handle_msg(&client_data).await?; + assert!(result.is_none()); + + Ok(()) +} + +#[tokio::test] +async fn test_handle_msg_multiple_clients_same_rule() -> Result<(), RuleManagerError> { + let rule_manager = RuleManager::new(); + let client1 = ClientId("client1".to_string()); + let client2 = ClientId("client2".to_string()); + + let rule1 = Rule::new( + RuleId("rule_1".to_string()), + Topic("shared/topic".to_string()), + core::time::Duration::from_millis(100), + "a > 10".to_owned(), + ); + + let rule2 = Rule::new( + RuleId("rule_2".to_string()), + Topic("shared/topic".to_string()), + core::time::Duration::from_millis(100), + "a > 5".to_owned(), // Different condition but same topic + ); + + rule_manager.add_rule(client1.clone(), rule1).await?; + rule_manager.add_rule(client2.clone(), rule2).await?; + + let client_data = ClientData { + run_id: 1, + name: "shared/topic".to_string(), + unit: "test_unit".to_string(), + values: vec![15.0], + timestamp: Utc::now(), + }; + + // First trigger to start debounce timers + let _ = rule_manager.handle_msg(&client_data).await; + + // Wait for debounce + tokio::time::sleep(tokio::time::Duration::from_millis(150)).await; + + let result = rule_manager.handle_msg(&client_data).await?; + + if let Some(notifications) = result { + // Both rules should trigger since 15.0 > 10 and 15.0 > 5 + assert!(notifications.len() >= 1); + + let client_ids: Vec<_> = notifications.iter().map(|(id, _)| id.clone()).collect(); + assert!(client_ids.contains(&client1) || client_ids.contains(&client2)); + } + + Ok(()) +} + +#[tokio::test] +async fn test_get_all_rules_empty() { + let rule_manager = RuleManager::new(); + let rules = rule_manager.get_all_rules().await; + assert!(rules.is_empty()); +} + +#[tokio::test] +async fn test_get_all_rules_multiple() -> Result<(), RuleManagerError> { + let rule_manager = RuleManager::new(); + let client = ClientId("test_client".to_string()); + + let rule1 = Rule::new( + RuleId("rule_1".to_string()), + Topic("topic1".to_string()), + core::time::Duration::from_secs(60), + "a > 10".to_owned(), + ); + + let rule2 = Rule::new( + RuleId("rule_2".to_string()), + Topic("topic2".to_string()), + core::time::Duration::from_secs(30), + "b < 5".to_owned(), + ); + + let rule3 = Rule::new( + RuleId("rule_3".to_string()), + Topic("topic3".to_string()), + core::time::Duration::from_secs(45), + "c == 0".to_owned(), + ); + + rule_manager.add_rule(client.clone(), rule1).await?; + rule_manager.add_rule(client.clone(), rule2).await?; + rule_manager.add_rule(client, rule3).await?; + + let rules = rule_manager.get_all_rules().await; + assert_eq!(rules.len(), 3); + + // Verify all topics are present + let topics: Vec<_> = rules.iter().map(|rule| &rule.topic.0).collect(); + assert!(topics.contains(&&"topic1".to_string())); + assert!(topics.contains(&&"topic2".to_string())); + assert!(topics.contains(&&"topic3".to_string())); + + Ok(()) +} + +#[tokio::test] +async fn test_rule_manager_concurrent_operations() -> Result<(), RuleManagerError> { + let rule_manager = std::sync::Arc::new(RuleManager::new()); + + let mut handles = vec![]; + + // Spawn multiple tasks that add rules concurrently + for i in 0..10 { + let rm = rule_manager.clone(); + let handle = tokio::spawn(async move { + let client = ClientId(format!("client_{}", i)); + let rule = Rule::new( + RuleId(format!("rule_{}", i)), + Topic(format!("topic/{}", i)), + core::time::Duration::from_secs(60), + "a > 5".to_owned(), + ); + + rm.add_rule(client, rule).await + }); + handles.push(handle); + } + + // Wait for all tasks to complete + for handle in handles { + handle.await.unwrap()?; + } + + assert_eq!(rule_manager.get_all_rules().await.len(), 10); + + Ok(()) +} + +#[tokio::test] +async fn test_rule_manager_default() { + let rule_manager = RuleManager::default(); + assert!(rule_manager.get_all_rules().await.is_empty()); +} + +#[tokio::test] +async fn test_complex_rule_expression() -> Result<(), RuleManagerError> { + let rule_manager = RuleManager::new(); + let client = ClientId("test_client".to_string()); + + let rule = Rule::new( + RuleId("complex_rule".to_string()), + Topic("sensor/data".to_string()), + core::time::Duration::from_millis(100), + "a > 10 && b < 5 && c >= 0".to_owned(), // Complex expression with multiple variables + ); + + rule_manager.add_rule(client.clone(), rule).await?; + + let client_data = ClientData { + run_id: 1, + name: "sensor/data".to_string(), + unit: "mixed".to_string(), + values: vec![15.0, 3.0, 1.0], // a=15 > 10, b=3 < 5, c=1 >= 0 - should trigger + timestamp: Utc::now(), + }; + + // Prime the rule + let _ = rule_manager.handle_msg(&client_data).await; + tokio::time::sleep(tokio::time::Duration::from_millis(150)).await; + + let result = rule_manager.handle_msg(&client_data).await?; + if let Some(notifications) = result { + assert!(!notifications.is_empty()); + assert_eq!(notifications[0].0 .0, client.0); + } + + Ok(()) +} From cacdc7a4961f18f0389a159039b64b3024a1ea07 Mon Sep 17 00:00:00 2001 From: suryatho Date: Mon, 1 Dec 2025 19:16:40 -0500 Subject: [PATCH 09/13] Added more concurrent tests and more asserts #475 --- scylla-server/src/rule_structs.rs | 12 + scylla-server/tests/bimultimap_test.rs | 383 ++++++++ scylla-server/tests/rule_structs_test.rs | 1035 +++++++++++++--------- 3 files changed, 1016 insertions(+), 414 deletions(-) create mode 100644 scylla-server/tests/bimultimap_test.rs diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index 52c9fa75..9df51d35 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -49,6 +49,14 @@ impl BiMultiMap { } } + pub fn lefts(&self) -> Vec { + self.left_to_right.keys().cloned().collect() + } + + pub fn rights(&self) -> Vec { + self.right_to_left.keys().cloned().collect() + } + pub fn get_right(&self, left: &L) -> Option<&FxHashSet> { self.left_to_right.get(left) } @@ -498,4 +506,8 @@ impl RuleManager { .map(|rule| rule.clone()) .collect() } + + pub async fn get_all_clients(&self) -> Vec { + self.subscriptions.read().await.lefts() + } } diff --git a/scylla-server/tests/bimultimap_test.rs b/scylla-server/tests/bimultimap_test.rs new file mode 100644 index 00000000..2a406df9 --- /dev/null +++ b/scylla-server/tests/bimultimap_test.rs @@ -0,0 +1,383 @@ +use scylla_server::rule_structs::{BiMapRemoveResult, BiMultiMap, ClientId, RuleId}; + +// Tests for BiMultiMap +#[test] +fn test_bi_multi_map_new() { + let bimap: BiMultiMap = BiMultiMap::new(); + assert!(bimap.get_left(&1).is_none()); + assert!(bimap.get_right(&"test".to_string()).is_none()); +} + +#[test] +fn test_bi_multi_map_insert_single() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right = 42; + + bimap.insert(&left, &right); + + assert_eq!(bimap.get_right(&left).unwrap().len(), 1); + assert!(bimap.get_right(&left).unwrap().contains(&right)); + + assert_eq!(bimap.get_left(&right).unwrap().len(), 1); + assert!(bimap.get_left(&right).unwrap().contains(&left)); +} + +#[test] +fn test_bi_multi_map_insert_multiple_rights() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right1 = 42; + let right2 = 43; + let right3 = 44; + + bimap.insert(&left, &right1); + bimap.insert(&left, &right2); + bimap.insert(&left, &right3); + + let rights = bimap.get_right(&left).unwrap(); + assert_eq!(rights.len(), 3); + assert!(rights.contains(&right1)); + assert!(rights.contains(&right2)); + assert!(rights.contains(&right3)); + + // Each right should map back to the left + assert!(bimap.get_left(&right1).unwrap().contains(&left)); + assert!(bimap.get_left(&right2).unwrap().contains(&left)); + assert!(bimap.get_left(&right3).unwrap().contains(&left)); +} + +#[test] +fn test_bi_multi_map_insert_multiple_lefts() { + let mut bimap = BiMultiMap::new(); + let left1 = "client1".to_string(); + let left2 = "client2".to_string(); + let left3 = "client3".to_string(); + let right = 42; + + bimap.insert(&left1, &right); + bimap.insert(&left2, &right); + bimap.insert(&left3, &right); + + let lefts = bimap.get_left(&right).unwrap(); + assert_eq!(lefts.len(), 3); + assert!(lefts.contains(&left1)); + assert!(lefts.contains(&left2)); + assert!(lefts.contains(&left3)); + + // Each left should map to the right + assert!(bimap.get_right(&left1).unwrap().contains(&right)); + assert!(bimap.get_right(&left2).unwrap().contains(&right)); + assert!(bimap.get_right(&left3).unwrap().contains(&right)); +} + +#[test] +fn test_bi_multi_map_insert_many_to_many() { + let mut bimap = BiMultiMap::new(); + let left1 = "client1".to_string(); + let left2 = "client2".to_string(); + let right1 = 42; + let right2 = 43; + + // Create many-to-many relationships + bimap.insert(&left1, &right1); + bimap.insert(&left1, &right2); + bimap.insert(&left2, &right1); + bimap.insert(&left2, &right2); + + // Verify left1 maps to both rights + let rights_for_left1 = bimap.get_right(&left1).unwrap(); + assert_eq!(rights_for_left1.len(), 2); + assert!(rights_for_left1.contains(&right1)); + assert!(rights_for_left1.contains(&right2)); + + // Verify left2 maps to both rights + let rights_for_left2 = bimap.get_right(&left2).unwrap(); + assert_eq!(rights_for_left2.len(), 2); + assert!(rights_for_left2.contains(&right1)); + assert!(rights_for_left2.contains(&right2)); + + // Verify right1 maps to both lefts + let lefts_for_right1 = bimap.get_left(&right1).unwrap(); + assert_eq!(lefts_for_right1.len(), 2); + assert!(lefts_for_right1.contains(&left1)); + assert!(lefts_for_right1.contains(&left2)); + + // Verify right2 maps to both lefts + let lefts_for_right2 = bimap.get_left(&right2).unwrap(); + assert_eq!(lefts_for_right2.len(), 2); + assert!(lefts_for_right2.contains(&left1)); + assert!(lefts_for_right2.contains(&left2)); +} + +#[test] +fn test_bi_multi_map_insert_duplicate() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right = 42; + + bimap.insert(&left, &right); + bimap.insert(&left, &right); // Duplicate insertion + + // Should still only have one mapping + assert_eq!(bimap.get_right(&left).unwrap().len(), 1); + assert_eq!(bimap.get_left(&right).unwrap().len(), 1); +} + +#[test] +fn test_bi_multi_map_remove_left_single() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right = 42; + + bimap.insert(&left, &right); + + let result = bimap.remove_left(&left); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_rights) = result { + assert_eq!(removed_rights.len(), 1); + assert!(removed_rights.contains(&right)); + } + + // Verify both directions are cleaned up + assert!(bimap.get_right(&left).is_none()); + assert!(bimap.get_left(&right).is_none()); +} + +#[test] +fn test_bi_multi_map_remove_left_shared_right() { + let mut bimap = BiMultiMap::new(); + let left1 = "client1".to_string(); + let left2 = "client2".to_string(); + let right = 42; + + bimap.insert(&left1, &right); + bimap.insert(&left2, &right); + + let result = bimap.remove_left(&left1); + assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); + + // left1 should be gone + assert!(bimap.get_right(&left1).is_none()); + + // right should still exist and map to left2 + let remaining_lefts = bimap.get_left(&right).unwrap(); + assert_eq!(remaining_lefts.len(), 1); + assert!(remaining_lefts.contains(&left2)); + + // left2 should still map to right + assert!(bimap.get_right(&left2).unwrap().contains(&right)); +} + +#[test] +fn test_bi_multi_map_remove_left_nonexistent() { + let mut bimap: BiMultiMap = BiMultiMap::new(); + let left = "nonexistent".to_string(); + + let result = bimap.remove_left(&left); + assert!(matches!(result, BiMapRemoveResult::NothingToRemove)); +} + +#[test] +fn test_bi_multi_map_remove_right_single() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right = 42; + + bimap.insert(&left, &right); + + let result = bimap.remove_right(&right); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_lefts) = result { + assert_eq!(removed_lefts.len(), 1); + assert!(removed_lefts.contains(&left)); + } + + // Verify both directions are cleaned up + assert!(bimap.get_right(&left).is_none()); + assert!(bimap.get_left(&right).is_none()); +} + +#[test] +fn test_bi_multi_map_remove_right_shared_left() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right1 = 42; + let right2 = 43; + + bimap.insert(&left, &right1); + bimap.insert(&left, &right2); + + let result = bimap.remove_right(&right1); + assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); + + // right1 should be gone + assert!(bimap.get_left(&right1).is_none()); + + // left should still exist and map to right2 + let remaining_rights = bimap.get_right(&left).unwrap(); + assert_eq!(remaining_rights.len(), 1); + assert!(remaining_rights.contains(&right2)); + + // right2 should still map to left + assert!(bimap.get_left(&right2).unwrap().contains(&left)); +} + +#[test] +fn test_bi_multi_map_remove_right_from_left() { + let mut bimap = BiMultiMap::new(); + let left = "client1".to_string(); + let right1 = 42; + let right2 = 43; + + bimap.insert(&left, &right1); + bimap.insert(&left, &right2); + + let result = bimap.remove_right_from_left(&left, &right1); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_right) = result { + assert_eq!(removed_right, right1); + } + + // left should still exist but only map to right2 + let remaining_rights = bimap.get_right(&left).unwrap(); + assert_eq!(remaining_rights.len(), 1); + assert!(remaining_rights.contains(&right2)); + + // right1 should be completely removed + assert!(bimap.get_left(&right1).is_none()); + + // right2 should still map to left + assert!(bimap.get_left(&right2).unwrap().contains(&left)); +} + +#[test] +fn test_bi_multi_map_remove_right_from_left_shared_right() { + let mut bimap = BiMultiMap::new(); + let left1 = "client1".to_string(); + let left2 = "client2".to_string(); + let right = 42; + + bimap.insert(&left1, &right); + bimap.insert(&left2, &right); + + let result = bimap.remove_right_from_left(&left1, &right); + assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); + + // left1 should be gone + assert!(bimap.get_right(&left1).is_none()); + + // right should still exist and map to left2 + let remaining_lefts = bimap.get_left(&right).unwrap(); + assert_eq!(remaining_lefts.len(), 1); + assert!(remaining_lefts.contains(&left2)); +} + +#[test] +fn test_bi_multi_map_remove_right_from_left_nonexistent() { + let mut bimap: BiMultiMap = BiMultiMap::new(); + let left = "client1".to_string(); + let right = 42; + + let result = bimap.remove_right_from_left(&left, &right); + assert!(matches!(result, BiMapRemoveResult::NothingToRemove)); +} + +#[test] +fn test_bi_multi_map_remove_left_from_right() { + let mut bimap = BiMultiMap::new(); + let left1 = "client1".to_string(); + let left2 = "client2".to_string(); + let right = 42; + + bimap.insert(&left1, &right); + bimap.insert(&left2, &right); + + let result = bimap.remove_left_from_right(&right, &left1); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_left) = result { + assert_eq!(removed_left, left1); + } + + // right should still exist but only map to left2 + let remaining_lefts = bimap.get_left(&right).unwrap(); + assert_eq!(remaining_lefts.len(), 1); + assert!(remaining_lefts.contains(&left2)); + + // left1 should be completely removed + assert!(bimap.get_right(&left1).is_none()); + + // left2 should still map to right + assert!(bimap.get_right(&left2).unwrap().contains(&right)); +} + +#[test] +fn test_bi_multi_map_complex_operations() { + let mut bimap = BiMultiMap::new(); + + // Set up a complex mapping + bimap.insert(&"client1", &"rule1"); + bimap.insert(&"client1", &"rule2"); + bimap.insert(&"client2", &"rule1"); + bimap.insert(&"client2", &"rule3"); + bimap.insert(&"client3", &"rule3"); + + // Remove a shared rule from one client + let result = bimap.remove_right_from_left(&"client1", &"rule1"); + assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); + + // Verify rule1 still exists for client2 + assert!(bimap.get_right(&"client2").unwrap().contains(&"rule1")); + + // Verify client1 still has rule2 + assert!(bimap.get_right(&"client1").unwrap().contains(&"rule2")); + assert!(!bimap.get_right(&"client1").unwrap().contains(&"rule1")); + + // Remove client2 entirely + let result = bimap.remove_left(&"client2"); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_rights) = result { + assert_eq!(removed_rights.len(), 1); + assert!(removed_rights.contains(&"rule1")); // rule1 should be cleaned up + } + + // rule3 should still exist for client3 + assert!(bimap.get_left(&"rule3").unwrap().contains(&"client3")); + + // rule1 should be completely gone + assert!(bimap.get_left(&"rule1").is_none()); +} + +#[test] +fn test_bi_multi_map_with_rule_manager_types() { + let mut bimap: BiMultiMap = BiMultiMap::new(); + + let client1 = ClientId("client1".to_string()); + let client2 = ClientId("client2".to_string()); + let rule1 = RuleId("rule1".to_string()); + let rule2 = RuleId("rule2".to_string()); + + bimap.insert(&client1, &rule1); + bimap.insert(&client1, &rule2); + bimap.insert(&client2, &rule1); + + // Test with actual types used in RuleManager + assert_eq!(bimap.get_right(&client1).unwrap().len(), 2); + assert_eq!(bimap.get_left(&rule1).unwrap().len(), 2); + + let result = bimap.remove_left(&client1); + assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); + + if let BiMapRemoveResult::RemovedWithCleanUp(removed_rules) = result { + assert_eq!(removed_rules.len(), 1); + assert!(removed_rules.contains(&rule2)); // rule2 should be cleaned up + } + + // rule1 should still exist for client2 + assert!(bimap.get_left(&rule1).unwrap().contains(&client2)); +} diff --git a/scylla-server/tests/rule_structs_test.rs b/scylla-server/tests/rule_structs_test.rs index 1578aba9..fe07db9c 100644 --- a/scylla-server/tests/rule_structs_test.rs +++ b/scylla-server/tests/rule_structs_test.rs @@ -1,390 +1,10 @@ use chrono::Utc; use scylla_server::rule_structs::*; use scylla_server::ClientData; +use std::sync::Arc; +use std::time::Duration; +use tokio::task::JoinSet; -// Tests for BiMultiMap -#[test] -fn test_bi_multi_map_new() { - let bimap: BiMultiMap = BiMultiMap::new(); - assert!(bimap.get_left(&1).is_none()); - assert!(bimap.get_right(&"test".to_string()).is_none()); -} - -#[test] -fn test_bi_multi_map_insert_single() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right = 42; - - bimap.insert(&left, &right); - - assert_eq!(bimap.get_right(&left).unwrap().len(), 1); - assert!(bimap.get_right(&left).unwrap().contains(&right)); - - assert_eq!(bimap.get_left(&right).unwrap().len(), 1); - assert!(bimap.get_left(&right).unwrap().contains(&left)); -} - -#[test] -fn test_bi_multi_map_insert_multiple_rights() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right1 = 42; - let right2 = 43; - let right3 = 44; - - bimap.insert(&left, &right1); - bimap.insert(&left, &right2); - bimap.insert(&left, &right3); - - let rights = bimap.get_right(&left).unwrap(); - assert_eq!(rights.len(), 3); - assert!(rights.contains(&right1)); - assert!(rights.contains(&right2)); - assert!(rights.contains(&right3)); - - // Each right should map back to the left - assert!(bimap.get_left(&right1).unwrap().contains(&left)); - assert!(bimap.get_left(&right2).unwrap().contains(&left)); - assert!(bimap.get_left(&right3).unwrap().contains(&left)); -} - -#[test] -fn test_bi_multi_map_insert_multiple_lefts() { - let mut bimap = BiMultiMap::new(); - let left1 = "client1".to_string(); - let left2 = "client2".to_string(); - let left3 = "client3".to_string(); - let right = 42; - - bimap.insert(&left1, &right); - bimap.insert(&left2, &right); - bimap.insert(&left3, &right); - - let lefts = bimap.get_left(&right).unwrap(); - assert_eq!(lefts.len(), 3); - assert!(lefts.contains(&left1)); - assert!(lefts.contains(&left2)); - assert!(lefts.contains(&left3)); - - // Each left should map to the right - assert!(bimap.get_right(&left1).unwrap().contains(&right)); - assert!(bimap.get_right(&left2).unwrap().contains(&right)); - assert!(bimap.get_right(&left3).unwrap().contains(&right)); -} - -#[test] -fn test_bi_multi_map_insert_many_to_many() { - let mut bimap = BiMultiMap::new(); - let left1 = "client1".to_string(); - let left2 = "client2".to_string(); - let right1 = 42; - let right2 = 43; - - // Create many-to-many relationships - bimap.insert(&left1, &right1); - bimap.insert(&left1, &right2); - bimap.insert(&left2, &right1); - bimap.insert(&left2, &right2); - - // Verify left1 maps to both rights - let rights_for_left1 = bimap.get_right(&left1).unwrap(); - assert_eq!(rights_for_left1.len(), 2); - assert!(rights_for_left1.contains(&right1)); - assert!(rights_for_left1.contains(&right2)); - - // Verify left2 maps to both rights - let rights_for_left2 = bimap.get_right(&left2).unwrap(); - assert_eq!(rights_for_left2.len(), 2); - assert!(rights_for_left2.contains(&right1)); - assert!(rights_for_left2.contains(&right2)); - - // Verify right1 maps to both lefts - let lefts_for_right1 = bimap.get_left(&right1).unwrap(); - assert_eq!(lefts_for_right1.len(), 2); - assert!(lefts_for_right1.contains(&left1)); - assert!(lefts_for_right1.contains(&left2)); - - // Verify right2 maps to both lefts - let lefts_for_right2 = bimap.get_left(&right2).unwrap(); - assert_eq!(lefts_for_right2.len(), 2); - assert!(lefts_for_right2.contains(&left1)); - assert!(lefts_for_right2.contains(&left2)); -} - -#[test] -fn test_bi_multi_map_insert_duplicate() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right = 42; - - bimap.insert(&left, &right); - bimap.insert(&left, &right); // Duplicate insertion - - // Should still only have one mapping - assert_eq!(bimap.get_right(&left).unwrap().len(), 1); - assert_eq!(bimap.get_left(&right).unwrap().len(), 1); -} - -#[test] -fn test_bi_multi_map_remove_left_single() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right = 42; - - bimap.insert(&left, &right); - - let result = bimap.remove_left(&left); - assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); - - if let BiMapRemoveResult::RemovedWithCleanUp(removed_rights) = result { - assert_eq!(removed_rights.len(), 1); - assert!(removed_rights.contains(&right)); - } - - // Verify both directions are cleaned up - assert!(bimap.get_right(&left).is_none()); - assert!(bimap.get_left(&right).is_none()); -} - -#[test] -fn test_bi_multi_map_remove_left_shared_right() { - let mut bimap = BiMultiMap::new(); - let left1 = "client1".to_string(); - let left2 = "client2".to_string(); - let right = 42; - - bimap.insert(&left1, &right); - bimap.insert(&left2, &right); - - let result = bimap.remove_left(&left1); - assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); - - // left1 should be gone - assert!(bimap.get_right(&left1).is_none()); - - // right should still exist and map to left2 - let remaining_lefts = bimap.get_left(&right).unwrap(); - assert_eq!(remaining_lefts.len(), 1); - assert!(remaining_lefts.contains(&left2)); - - // left2 should still map to right - assert!(bimap.get_right(&left2).unwrap().contains(&right)); -} - -#[test] -fn test_bi_multi_map_remove_left_nonexistent() { - let mut bimap: BiMultiMap = BiMultiMap::new(); - let left = "nonexistent".to_string(); - - let result = bimap.remove_left(&left); - assert!(matches!(result, BiMapRemoveResult::NothingToRemove)); -} - -#[test] -fn test_bi_multi_map_remove_right_single() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right = 42; - - bimap.insert(&left, &right); - - let result = bimap.remove_right(&right); - assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); - - if let BiMapRemoveResult::RemovedWithCleanUp(removed_lefts) = result { - assert_eq!(removed_lefts.len(), 1); - assert!(removed_lefts.contains(&left)); - } - - // Verify both directions are cleaned up - assert!(bimap.get_right(&left).is_none()); - assert!(bimap.get_left(&right).is_none()); -} - -#[test] -fn test_bi_multi_map_remove_right_shared_left() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right1 = 42; - let right2 = 43; - - bimap.insert(&left, &right1); - bimap.insert(&left, &right2); - - let result = bimap.remove_right(&right1); - assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); - - // right1 should be gone - assert!(bimap.get_left(&right1).is_none()); - - // left should still exist and map to right2 - let remaining_rights = bimap.get_right(&left).unwrap(); - assert_eq!(remaining_rights.len(), 1); - assert!(remaining_rights.contains(&right2)); - - // right2 should still map to left - assert!(bimap.get_left(&right2).unwrap().contains(&left)); -} - -#[test] -fn test_bi_multi_map_remove_right_from_left() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right1 = 42; - let right2 = 43; - - bimap.insert(&left, &right1); - bimap.insert(&left, &right2); - - let result = bimap.remove_right_from_left(&left, &right1); - assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); - - if let BiMapRemoveResult::RemovedWithCleanUp(removed_right) = result { - assert_eq!(removed_right, right1); - } - - // left should still exist but only map to right2 - let remaining_rights = bimap.get_right(&left).unwrap(); - assert_eq!(remaining_rights.len(), 1); - assert!(remaining_rights.contains(&right2)); - - // right1 should be completely removed - assert!(bimap.get_left(&right1).is_none()); - - // right2 should still map to left - assert!(bimap.get_left(&right2).unwrap().contains(&left)); -} - -#[test] -fn test_bi_multi_map_remove_right_from_left_shared_right() { - let mut bimap = BiMultiMap::new(); - let left1 = "client1".to_string(); - let left2 = "client2".to_string(); - let right = 42; - - bimap.insert(&left1, &right); - bimap.insert(&left2, &right); - - let result = bimap.remove_right_from_left(&left1, &right); - assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); - - // left1 should be gone - assert!(bimap.get_right(&left1).is_none()); - - // right should still exist and map to left2 - let remaining_lefts = bimap.get_left(&right).unwrap(); - assert_eq!(remaining_lefts.len(), 1); - assert!(remaining_lefts.contains(&left2)); -} - -#[test] -fn test_bi_multi_map_remove_right_from_left_nonexistent() { - let mut bimap: BiMultiMap = BiMultiMap::new(); - let left = "client1".to_string(); - let right = 42; - - let result = bimap.remove_right_from_left(&left, &right); - assert!(matches!(result, BiMapRemoveResult::NothingToRemove)); -} - -#[test] -fn test_bi_multi_map_remove_left_from_right() { - let mut bimap = BiMultiMap::new(); - let left1 = "client1".to_string(); - let left2 = "client2".to_string(); - let right = 42; - - bimap.insert(&left1, &right); - bimap.insert(&left2, &right); - - let result = bimap.remove_left_from_right(&right, &left1); - assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); - - if let BiMapRemoveResult::RemovedWithCleanUp(removed_left) = result { - assert_eq!(removed_left, left1); - } - - // right should still exist but only map to left2 - let remaining_lefts = bimap.get_left(&right).unwrap(); - assert_eq!(remaining_lefts.len(), 1); - assert!(remaining_lefts.contains(&left2)); - - // left1 should be completely removed - assert!(bimap.get_right(&left1).is_none()); - - // left2 should still map to right - assert!(bimap.get_right(&left2).unwrap().contains(&right)); -} - -#[test] -fn test_bi_multi_map_complex_operations() { - let mut bimap = BiMultiMap::new(); - - // Set up a complex mapping - bimap.insert(&"client1", &"rule1"); - bimap.insert(&"client1", &"rule2"); - bimap.insert(&"client2", &"rule1"); - bimap.insert(&"client2", &"rule3"); - bimap.insert(&"client3", &"rule3"); - - // Remove a shared rule from one client - let result = bimap.remove_right_from_left(&"client1", &"rule1"); - assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); - - // Verify rule1 still exists for client2 - assert!(bimap.get_right(&"client2").unwrap().contains(&"rule1")); - - // Verify client1 still has rule2 - assert!(bimap.get_right(&"client1").unwrap().contains(&"rule2")); - assert!(!bimap.get_right(&"client1").unwrap().contains(&"rule1")); - - // Remove client2 entirely - let result = bimap.remove_left(&"client2"); - assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); - - if let BiMapRemoveResult::RemovedWithCleanUp(removed_rights) = result { - assert_eq!(removed_rights.len(), 1); - assert!(removed_rights.contains(&"rule1")); // rule1 should be cleaned up - } - - // rule3 should still exist for client3 - assert!(bimap.get_left(&"rule3").unwrap().contains(&"client3")); - - // rule1 should be completely gone - assert!(bimap.get_left(&"rule1").is_none()); -} - -#[test] -fn test_bi_multi_map_with_rule_manager_types() { - let mut bimap: BiMultiMap = BiMultiMap::new(); - - let client1 = ClientId("client1".to_string()); - let client2 = ClientId("client2".to_string()); - let rule1 = RuleId("rule1".to_string()); - let rule2 = RuleId("rule2".to_string()); - - bimap.insert(&client1, &rule1); - bimap.insert(&client1, &rule2); - bimap.insert(&client2, &rule1); - - // Test with actual types used in RuleManager - assert_eq!(bimap.get_right(&client1).unwrap().len(), 2); - assert_eq!(bimap.get_left(&rule1).unwrap().len(), 2); - - let result = bimap.remove_left(&client1); - assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); - - if let BiMapRemoveResult::RemovedWithCleanUp(removed_rules) = result { - assert_eq!(removed_rules.len(), 1); - assert!(removed_rules.contains(&rule2)); // rule2 should be cleaned up - } - - // rule1 should still exist for client2 - assert!(bimap.get_left(&rule1).unwrap().contains(&client2)); -} - -// Tests for rule manager #[tokio::test] async fn test_add_one_rule() -> Result<(), RuleManagerError> { let rule_manager = RuleManager::new(); @@ -516,9 +136,10 @@ async fn test_delete_client_success() -> Result<(), RuleManagerError> { rule_manager.add_rule(client.clone(), rule1).await?; rule_manager.add_rule(client.clone(), rule2).await?; assert_eq!(rule_manager.get_all_rules().await.len(), 2); + assert_eq!(rule_manager.get_all_clients().await.len(), 1); rule_manager.delete_client(client).await?; - // Rules still exist in the system but client is removed from subscriptions + assert!(rule_manager.get_all_clients().await.is_empty()); assert_eq!(rule_manager.get_all_rules().await.len(), 2); Ok(()) @@ -557,8 +178,8 @@ async fn test_handle_msg_rule_triggered() -> Result<(), RuleManagerError> { let rule = Rule::new( RuleId("rule_1".to_string()), Topic("test/topic".to_string()), - core::time::Duration::from_millis(100), // Short debounce for testing - "a > 10".to_owned(), // First value (a) should be > 10 + core::time::Duration::from_secs(1), + "a > 10".to_owned(), // First value (a) should be > 10 ); rule_manager.add_rule(client.clone(), rule).await?; @@ -572,10 +193,11 @@ async fn test_handle_msg_rule_triggered() -> Result<(), RuleManagerError> { }; // First trigger might not fire due to debounce logic - let _ = rule_manager.handle_msg(&client_data).await; + let empty_notifications = rule_manager.handle_msg(&client_data).await; + assert!(empty_notifications.is_ok_and(|op| op.is_none())); // Wait for debounce time - tokio::time::sleep(tokio::time::Duration::from_millis(150)).await; + tokio::time::sleep(tokio::time::Duration::from_secs(2)).await; let result = rule_manager.handle_msg(&client_data).await?; @@ -647,7 +269,8 @@ async fn test_handle_msg_multiple_clients_same_rule() -> Result<(), RuleManagerE }; // First trigger to start debounce timers - let _ = rule_manager.handle_msg(&client_data).await; + let empty = rule_manager.handle_msg(&client_data).await; + assert!(empty.is_ok_and(|op| op.is_none())); // Wait for debounce tokio::time::sleep(tokio::time::Duration::from_millis(150)).await; @@ -659,7 +282,7 @@ async fn test_handle_msg_multiple_clients_same_rule() -> Result<(), RuleManagerE assert!(notifications.len() >= 1); let client_ids: Vec<_> = notifications.iter().map(|(id, _)| id.clone()).collect(); - assert!(client_ids.contains(&client1) || client_ids.contains(&client2)); + assert!(client_ids.contains(&client1) && client_ids.contains(&client2)); } Ok(()) @@ -714,43 +337,627 @@ async fn test_get_all_rules_multiple() -> Result<(), RuleManagerError> { Ok(()) } +fn check_rules_present(rules: Vec, prefix: &str, k: usize) { + assert_eq!(rules.len(), k); + let topics = rules.into_iter().map(|r| r.topic.0).collect::>(); + assert!((0..k).all(|i| topics.contains(&format!("{}{}", prefix, i)))); +} + +fn check_clients_present(clients: Vec, prefix: &str, k: usize) { + assert_eq!(clients.len(), k); + let client_strings = clients.into_iter().map(|c| c.0).collect::>(); + assert!((0..k).all(|i| client_strings.contains(&format!("{}{}", prefix, i)))); +} + #[tokio::test] -async fn test_rule_manager_concurrent_operations() -> Result<(), RuleManagerError> { +async fn test_rule_manager_concurrent_add_rule() -> Result<(), RuleManagerError> { + let num_rules = 10; let rule_manager = std::sync::Arc::new(RuleManager::new()); - let mut handles = vec![]; - - // Spawn multiple tasks that add rules concurrently - for i in 0..10 { - let rm = rule_manager.clone(); - let handle = tokio::spawn(async move { - let client = ClientId(format!("client_{}", i)); - let rule = Rule::new( - RuleId(format!("rule_{}", i)), - Topic(format!("topic/{}", i)), - core::time::Duration::from_secs(60), - "a > 5".to_owned(), - ); + (0..num_rules) + .fold(JoinSet::new(), |mut set, i| { + let rm = rule_manager.clone(); + set.spawn(async move { + let client = ClientId(format!("client_{}", i)); + let rule = Rule::new( + RuleId(format!("rule_{}", i)), + Topic(format!("topic/{}", i)), + core::time::Duration::from_secs(60), + "a > 5".to_owned(), + ); + + rm.add_rule(client, rule).await.unwrap(); + }); + set + }) + .join_all() + .await; + + let clients = rule_manager.get_all_clients().await; + check_clients_present(clients, "client_", num_rules); + + let rules = rule_manager.get_all_rules().await; + check_rules_present(rules, "topic/", num_rules); + + Ok(()) +} + +#[tokio::test] +async fn test_rule_manager_concurrent_delete_rule() -> Result<(), RuleManagerError> { + let num_rules = 10; + let rule_manager = std::sync::Arc::new(RuleManager::new()); - rm.add_rule(client, rule).await - }); - handles.push(handle); + (0..num_rules) + .fold(JoinSet::new(), |mut set, i| { + let rm = rule_manager.clone(); + set.spawn(async move { + let client = ClientId(format!("client_{}", i)); + let rule = Rule::new( + RuleId(format!("rule_{}", i)), + Topic(format!("topic/{}", i)), + core::time::Duration::from_secs(60), + "a > 5".to_owned(), + ); + + rm.add_rule(client, rule).await.unwrap(); + }); + set + }) + .join_all() + .await; + + check_clients_present(rule_manager.get_all_clients().await, "client_", num_rules); + check_rules_present(rule_manager.get_all_rules().await, "topic/", num_rules); + + let f = async || { + (0..10) + .fold(JoinSet::new(), |mut set, i| { + let rm = rule_manager.clone(); + set.spawn(async move { + let client = ClientId(format!("client_{}", i)); + let rule_id = RuleId(format!("rule_{}", i)); + rm.delete_rule(client, rule_id).await + }); + set + }) + .join_all() + .await + }; + + // Deleting rules from calling client side code doesn't actually remove rules + let res = f().await; + assert!(res.into_iter().all(|e| e.is_ok())); + check_rules_present(rule_manager.get_all_rules().await, "topic/", num_rules); + assert!(rule_manager.get_all_clients().await.is_empty()); + + // Deleting again will result in NoSuchClient errors + let res = f().await; + assert!(res.into_iter().all(|e| e.is_err())); + check_rules_present(rule_manager.get_all_rules().await, "topic/", num_rules); + assert!(rule_manager.get_all_clients().await.is_empty()); + + Ok(()) +} + +#[tokio::test] +async fn test_concurrent_handle_msg_stress() -> Result<(), RuleManagerError> { + let num_clients = 10; + let num_messages = 50; + let rule_manager = std::sync::Arc::new(RuleManager::new()); + + // Set up multiple clients with rules on the same topic + for i in 0..num_clients { + let client = ClientId(format!("stress_client_{}", i)); + let rule = Rule::new( + RuleId(format!("stress_rule_{}", i)), + Topic("stress/topic".to_string()), + core::time::Duration::from_secs(1), + format!("a > {}", i * 2), // Different thresholds: 0, 2, 4, 6, 8, 10, 12, 14, 16, 18 + ); + rule_manager.add_rule(client, rule).await?; + } + + // Verify setup + assert_eq!(rule_manager.get_all_rules().await.len(), num_clients); + assert_eq!(rule_manager.get_all_clients().await.len(), num_clients); + + let client_datas = Arc::new( + (0..num_messages) + .map(|i| { + ClientData { + run_id: 1, + name: "stress/topic".to_string(), + unit: "test".to_string(), + values: vec![(i % 20) as f32], // Values from 0-19 + timestamp: Utc::now(), + } + }) + .collect::>(), + ); + + // Spawn many concurrent message handlers + (0..num_messages) + .fold(JoinSet::new(), |mut set, i| { + let rm = rule_manager.clone(); + let client_datas = client_datas.clone(); + set.spawn(async move { + let empty = rm.handle_msg(&client_datas[i]).await; + assert!(empty.is_ok_and(|op| op.is_none())); + }); + set + }) + .join_all() + .await; + + tokio::time::sleep(Duration::from_secs(2)).await; + + // Now send the messages again to trigger rules after debounce + let results: Vec<_> = (0..num_messages) + .fold(JoinSet::new(), |mut set, i| { + let rm = rule_manager.clone(); + let client_datas = client_datas.clone(); + set.spawn(async move { + let result = rm.handle_msg(&client_datas[i]).await; + + assert!(result.is_ok()); + (i, result) + }); + set + }) + .join_all() + .await; + + // Verify all messages were processed + assert_eq!(results.len(), num_messages); + + // Count notifications generated + let total_notifications: usize = results + .iter() + .map(|(_, result)| { + result + .as_ref() + .unwrap() + .as_ref() + .map(|notifications| notifications.len()) + .unwrap_or(0) + }) + .sum(); + + println!("Total notifications generated: {}", total_notifications); + // Should have some notifications, but exact count depends on timing + assert!(total_notifications > 0); + + results.iter().for_each(|(i, result)| { + assert!(result.is_ok()); + + if let Some(notifications) = result.as_ref().unwrap() { + for (client_id, notification) in notifications { + assert!(client_id.0.starts_with("stress_client_")); + assert_eq!(notification.topic.0, "stress/topic"); + assert!(notification.id.0.starts_with("stress_rule_")); + assert_eq!(notification.values, vec![(i % 20) as f32]); + } + } + }); + + Ok(()) +} + +#[tokio::test] +async fn test_concurrent_topic_index_stress() -> Result<(), RuleManagerError> { + let num_topics = 20; + let num_rules_per_topic = 5; + let rule_manager = std::sync::Arc::new(RuleManager::new()); + + // Create multiple rules for the same topics concurrently + let results: Vec<_> = (0..num_topics) + .flat_map(|topic_idx| (0..num_rules_per_topic).map(move |rule_idx| (topic_idx, rule_idx))) + .fold(JoinSet::new(), |mut set, (topic_idx, rule_idx)| { + let rm = rule_manager.clone(); + set.spawn(async move { + let client = ClientId(format!("topic_client_{}_{}", topic_idx, rule_idx)); + let rule = Rule::new( + RuleId(format!("topic_rule_{}_{}", topic_idx, rule_idx)), + Topic(format!("topic/{}", topic_idx)), + core::time::Duration::from_millis(50), + format!("a > {}", rule_idx), + ); + rm.add_rule(client.clone(), rule) + .await + .map(|_| (topic_idx, rule_idx, client)) + }); + set + }) + .join_all() + .await; + + // Verify all operations succeeded + let successful_adds: Vec<_> = results.into_iter().filter_map(|r| r.ok()).collect(); + let total_expected = num_topics * num_rules_per_topic; + assert_eq!(successful_adds.len(), total_expected); + + // Verify final counts + assert_eq!(rule_manager.get_all_rules().await.len(), total_expected); + assert_eq!(rule_manager.get_all_clients().await.len(), total_expected); + + // Verify topic distribution + let all_rules = rule_manager.get_all_rules().await; + let mut topic_counts = std::collections::HashMap::new(); + for rule in all_rules { + *topic_counts.entry(rule.topic.0).or_insert(0) += 1; } - // Wait for all tasks to complete - for handle in handles { - handle.await.unwrap()?; + assert_eq!(topic_counts.len(), num_topics); + for i in 0..num_topics { + let topic_name = format!("topic/{}", i); + assert_eq!(topic_counts[&topic_name], num_rules_per_topic); } - assert_eq!(rule_manager.get_all_rules().await.len(), 10); + // Test that all topics can handle messages concurrently + let message_results: Vec<_> = (0..num_topics) + .fold(JoinSet::new(), |mut set, topic_idx| { + let rm = rule_manager.clone(); + set.spawn(async move { + let client_data = ClientData { + run_id: 1, + name: format!("topic/{}", topic_idx), + unit: "test".to_string(), + values: vec![10.0], // Should trigger rules with threshold < 10 + timestamp: Utc::now(), + }; + rm.handle_msg(&client_data) + .await + .map(|result| (topic_idx, result)) + }); + set + }) + .join_all() + .await; + + // Verify all messages were processed + let successful_messages: Vec<_> = message_results.into_iter().filter_map(|r| r.ok()).collect(); + assert_eq!(successful_messages.len(), num_topics); + + // Wait for debounce and try again + tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; + + let second_round_results: Vec<_> = (0..num_topics) + .fold(JoinSet::new(), |mut set, topic_idx| { + let rm = rule_manager.clone(); + set.spawn(async move { + let client_data = ClientData { + run_id: 1, + name: format!("topic/{}", topic_idx), + unit: "test".to_string(), + values: vec![10.0], + timestamp: Utc::now(), + }; + rm.handle_msg(&client_data).await + }); + set + }) + .join_all() + .await; + + // Count total notifications from second round (should have some due to debounce completion) + let total_notifications: usize = second_round_results + .iter() + .filter_map(|r| r.as_ref().ok()) + .map(|result| result.as_ref().map(|n| n.len()).unwrap_or(0)) + .sum(); + + // Should have triggered some rules (those with threshold < 10) + // Each topic has rules with thresholds 0,1,2,3,4 so value 10.0 should trigger all of them + assert!(total_notifications > 0); + println!( + "Total notifications in second round: {}", + total_notifications + ); Ok(()) } #[tokio::test] -async fn test_rule_manager_default() { - let rule_manager = RuleManager::default(); - assert!(rule_manager.get_all_rules().await.is_empty()); +async fn test_concurrent_high_frequency_messages() -> Result<(), RuleManagerError> { + let rule_manager = std::sync::Arc::new(RuleManager::new()); + + // Set up multiple rules that will receive high-frequency messages + let num_rules = 5; + for i in 0..num_rules { + let client = ClientId(format!("high_freq_client_{}", i)); + let rule = Rule::new( + RuleId(format!("high_freq_rule_{}", i)), + Topic("high_freq/topic".to_string()), + core::time::Duration::from_millis(50), + format!("a > {}", i * 10), // Thresholds: 0, 10, 20, 30, 40 + ); + rule_manager.add_rule(client, rule).await?; + } + + // Verify setup + assert_eq!(rule_manager.get_all_rules().await.len(), num_rules); + assert_eq!(rule_manager.get_all_clients().await.len(), num_rules); + + let messages_per_task = 20; + let num_tasks = 10; + let total_messages = messages_per_task * num_tasks; + + // Send high-frequency messages from multiple tasks + let results: Vec<_> = (0..num_tasks) + .fold(JoinSet::new(), |mut set, task_id| { + let rm = rule_manager.clone(); + set.spawn(async move { + let mut task_results = Vec::new(); + for msg_id in 0..messages_per_task { + let value = (task_id * messages_per_task + msg_id) as f32 % 100.0; + let client_data = ClientData { + run_id: task_id as i32, + name: "high_freq/topic".to_string(), + unit: "test".to_string(), + values: vec![value], + timestamp: Utc::now(), + }; + + let result = rm.handle_msg(&client_data).await; + task_results.push((msg_id, value, result)); + + // Small delay to simulate realistic message timing + if msg_id % 5 == 0 { + tokio::task::yield_now().await; + } + } + (task_id, task_results) + }); + set + }) + .join_all() + .await; + + // Verify all tasks completed + assert_eq!(results.len(), num_tasks); + + // Flatten and verify all message results + let all_message_results: Vec<_> = results + .into_iter() + .flat_map(|(task_id, task_results)| { + task_results + .into_iter() + .map(move |(msg_id, value, result)| (task_id, msg_id, value, result)) + }) + .collect(); + + assert_eq!(all_message_results.len(), total_messages); + + // Verify all messages were processed successfully + let successful_messages: Vec<_> = all_message_results + .iter() + .filter(|(_, _, _, result)| result.is_ok()) + .collect(); + assert_eq!(successful_messages.len(), total_messages); + + println!( + "Successfully processed {} high-frequency messages", + total_messages + ); + + // Wait for any pending debounce timers + tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; + + // Send final test messages with known values that should trigger specific rules + let test_values = vec![5.0, 15.0, 25.0, 35.0, 45.0]; // Should trigger different numbers of rules + let final_results: Vec<_> = test_values + .into_iter() + .fold(JoinSet::new(), |mut set, value| { + let rm = rule_manager.clone(); + set.spawn(async move { + let client_data = ClientData { + run_id: 999, + name: "high_freq/topic".to_string(), + unit: "test".to_string(), + values: vec![value], + timestamp: Utc::now(), + }; + + // Wait for debounce + tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; + let result = rm.handle_msg(&client_data).await; + (value, result) + }); + set + }) + .join_all() + .await; + + // Verify final test results + assert_eq!(final_results.len(), 5); + + for (value, result) in final_results { + assert!( + result.is_ok(), + "Failed to process message with value {}", + value + ); + + if let Ok(Some(notifications)) = result { + // Count how many rules should trigger for this value + let expected_triggers = num_rules - (value as usize / 10).min(num_rules); + if expected_triggers > 0 { + assert!( + !notifications.is_empty(), + "Value {} should have triggered some rules", + value + ); + assert!( + notifications.len() <= expected_triggers, + "Value {} triggered {} rules, expected at most {}", + value, + notifications.len(), + expected_triggers + ); + + // Verify notification structure + for (client_id, notification) in notifications { + assert!(client_id.0.starts_with("high_freq_client_")); + assert_eq!(notification.topic.0, "high_freq/topic"); + assert_eq!(notification.values, vec![value]); + } + } + } + } + + // Verify system state is unchanged + assert_eq!(rule_manager.get_all_rules().await.len(), num_rules); + assert_eq!(rule_manager.get_all_clients().await.len(), num_rules); + + Ok(()) +} + +#[tokio::test] +async fn test_concurrent_rule_expression_evaluation() -> Result<(), RuleManagerError> { + let num_expressions = 20; + let rule_manager = std::sync::Arc::new(RuleManager::new()); + + // Create rules with different complex expressions + let expressions = vec![ + ("simple_gt", "a > 10"), + ("simple_and", "a > 10 && b < 5"), + ("addition", "a + b > 20"), + ("multiplication", "a * b < 100"), + ("comparison", "a > b + c"), + ("or_condition", "(a > 10) || (b > 20)"), + ("all_positive", "a >= 0 && b >= 0 && c >= 0"), + ("complex_and_or", "a > 5 && (b > 3 || c > 7)"), + ("modulo", "a % 2 == 0"), + ("range_check", "a > 0 && a < 50"), + ]; + + let results: Vec<_> = (0..num_expressions) + .fold(JoinSet::new(), |mut set, i| { + let rm = rule_manager.clone(); + let (expr_name, expr) = expressions[i % expressions.len()].clone(); + set.spawn(async move { + let client = ClientId(format!("expr_client_{}_{}", i, expr_name)); + let rule = Rule::new( + RuleId(format!("expr_rule_{}_{}", i, expr_name)), + Topic("expr/topic".to_string()), + core::time::Duration::from_millis(25), + expr.to_string(), + ); + rm.add_rule(client.clone(), rule) + .await + .map(|_| (i, expr_name, client)) + }); + set + }) + .join_all() + .await; + + // Verify all rules were added successfully + let successful_additions: Vec<_> = results.into_iter().filter_map(|r| r.ok()).collect(); + assert_eq!(successful_additions.len(), num_expressions); + assert_eq!(rule_manager.get_all_rules().await.len(), num_expressions); + + // Send messages with different value combinations concurrently + let num_message_sets = 50; + let message_results: Vec<_> = (0..num_message_sets) + .fold(JoinSet::new(), |mut set, i| { + let rm = rule_manager.clone(); + set.spawn(async move { + let a_val = (i % 30) as f32; // a: 0-29 + let b_val = ((i * 2) % 25) as f32; // b: 0-24 + let c_val = ((i * 3) % 15) as f32; // c: 0-14 + + let client_data = ClientData { + run_id: 1, + name: "expr/topic".to_string(), + unit: "test".to_string(), + values: vec![a_val, b_val, c_val], + timestamp: Utc::now(), + }; + + let result = rm.handle_msg(&client_data).await; + (i, a_val, b_val, c_val, result) + }); + set + }) + .join_all() + .await; + + // Verify all messages were processed + assert_eq!(message_results.len(), num_message_sets); + + let successful_messages: Vec<_> = message_results + .iter() + .filter(|(_, _, _, _, result)| result.is_ok()) + .collect(); + assert_eq!(successful_messages.len(), num_message_sets); + + // Count total notifications (some may be None due to debounce) + let total_notifications: usize = message_results + .iter() + .filter_map(|(_, _, _, _, result)| result.as_ref().ok()?.as_ref().map(|n| n.len())) + .sum(); + + println!( + "Expression evaluation test - Total notifications: {}", + total_notifications + ); + + // Wait for debounce and send specific test messages + tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; + + // Test specific value combinations that should trigger known expressions + let test_cases = vec![ + (15.0, 3.0, 2.0), // Should trigger: a > 10, a > 10 && b < 5, etc. + (25.0, 30.0, 8.0), // Should trigger: (a > 10) || (b > 20), etc. + (2.0, 1.0, 1.0), // Should trigger: a % 2 == 0, all_positive, etc. + (0.0, 0.0, 0.0), // Should trigger: all_positive, a % 2 == 0 + ]; + + for (a, b, c) in test_cases { + tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; + + let client_data = ClientData { + run_id: 999, + name: "expr/topic".to_string(), + unit: "test".to_string(), + values: vec![a, b, c], + timestamp: Utc::now(), + }; + + let result = rule_manager.handle_msg(&client_data).await; + assert!( + result.is_ok(), + "Failed to process test case ({}, {}, {})", + a, + b, + c + ); + + if let Ok(Some(notifications)) = result { + assert!( + !notifications.is_empty(), + "Test case ({}, {}, {}) should trigger some rules", + a, + b, + c + ); + assert!(notifications.len() <= num_expressions); + + // Verify notification structure + for (client_id, notification) in notifications { + assert!(client_id.0.starts_with("expr_client_")); + assert_eq!(notification.topic.0, "expr/topic"); + assert_eq!(notification.values, vec![a, b, c]); + } + } + } + + // Verify final state + assert_eq!(rule_manager.get_all_rules().await.len(), num_expressions); + assert_eq!(rule_manager.get_all_clients().await.len(), num_expressions); + + Ok(()) } #[tokio::test] From 36ae11c5cfc53178d4fd603698ed864a2bdd1675 Mon Sep 17 00:00:00 2001 From: suryatho Date: Thu, 4 Dec 2025 20:37:54 -0500 Subject: [PATCH 10/13] Remove lots of redundant testst #475 --- scylla-server/tests/rule_structs_test.rs | 436 ----------------------- 1 file changed, 436 deletions(-) diff --git a/scylla-server/tests/rule_structs_test.rs b/scylla-server/tests/rule_structs_test.rs index fe07db9c..6c30ab48 100644 --- a/scylla-server/tests/rule_structs_test.rs +++ b/scylla-server/tests/rule_structs_test.rs @@ -1,33 +1,8 @@ use chrono::Utc; use scylla_server::rule_structs::*; use scylla_server::ClientData; -use std::sync::Arc; -use std::time::Duration; use tokio::task::JoinSet; -#[tokio::test] -async fn test_add_one_rule() -> Result<(), RuleManagerError> { - let rule_manager = RuleManager::new(); - - let client = ClientId("test_client".to_string()); - let rule = Rule::new( - RuleId("rule_1".to_string()), - Topic("test/topic".to_string()), - core::time::Duration::from_secs(60), - "value > 10".to_owned(), - ); - - rule_manager.add_rule(client, rule.clone()).await?; - - assert_eq!(rule_manager.get_all_rules().await.len(), 1); - assert_eq!( - &rule_manager.get_all_rules().await[0].topic.0, - &rule.topic.0 - ); - - Ok(()) -} - #[tokio::test] async fn test_add_multiple_rules_same_client() -> Result<(), RuleManagerError> { let rule_manager = RuleManager::new(); @@ -54,34 +29,6 @@ async fn test_add_multiple_rules_same_client() -> Result<(), RuleManagerError> { Ok(()) } -#[tokio::test] -async fn test_add_multiple_clients_same_rule_topic() -> Result<(), RuleManagerError> { - let rule_manager = RuleManager::new(); - - let client1 = ClientId("client1".to_string()); - let client2 = ClientId("client2".to_string()); - - let rule1 = Rule::new( - RuleId("rule_1".to_string()), - Topic("shared/topic".to_string()), - core::time::Duration::from_secs(60), - "a > 10".to_owned(), - ); - - let rule2 = Rule::new( - RuleId("rule_2".to_string()), - Topic("shared/topic".to_string()), - core::time::Duration::from_secs(30), - "a < 5".to_owned(), - ); - - rule_manager.add_rule(client1, rule1).await?; - rule_manager.add_rule(client2, rule2).await?; - - assert_eq!(rule_manager.get_all_rules().await.len(), 2); - Ok(()) -} - #[tokio::test] async fn test_delete_rule_success() -> Result<(), RuleManagerError> { let rule_manager = RuleManager::new(); @@ -104,16 +51,6 @@ async fn test_delete_rule_success() -> Result<(), RuleManagerError> { Ok(()) } -#[tokio::test] -async fn test_delete_rule_nonexistent_client() { - let rule_manager = RuleManager::new(); - let client = ClientId("nonexistent_client".to_string()); - let rule_id = RuleId("nonexistent_rule".to_string()); - - let result = rule_manager.delete_rule(client, rule_id).await; - assert!(matches!(result, Err(RuleManagerError::NoSuchClient))); -} - #[tokio::test] async fn test_delete_client_success() -> Result<(), RuleManagerError> { let rule_manager = RuleManager::new(); @@ -145,15 +82,6 @@ async fn test_delete_client_success() -> Result<(), RuleManagerError> { Ok(()) } -#[tokio::test] -async fn test_delete_client_nonexistent() { - let rule_manager = RuleManager::new(); - let client = ClientId("nonexistent_client".to_string()); - - let result = rule_manager.delete_client(client).await; - assert!(matches!(result, Err(RuleManagerError::NoSuchClient))); -} - #[tokio::test] async fn test_handle_msg_no_matching_rule() { let rule_manager = RuleManager::new(); @@ -209,34 +137,6 @@ async fn test_handle_msg_rule_triggered() -> Result<(), RuleManagerError> { Ok(()) } -#[tokio::test] -async fn test_handle_msg_rule_not_triggered() -> Result<(), RuleManagerError> { - let rule_manager = RuleManager::new(); - let client = ClientId("test_client".to_string()); - - let rule = Rule::new( - RuleId("rule_1".to_string()), - Topic("test/topic".to_string()), - core::time::Duration::from_secs(1), - "a > 10".to_owned(), - ); - - rule_manager.add_rule(client, rule).await?; - - let client_data = ClientData { - run_id: 1, - name: "test/topic".to_string(), - unit: "test_unit".to_string(), - values: vec![5.0], // a = 5.0 < 10, should not trigger - timestamp: Utc::now(), - }; - - let result = rule_manager.handle_msg(&client_data).await?; - assert!(result.is_none()); - - Ok(()) -} - #[tokio::test] async fn test_handle_msg_multiple_clients_same_rule() -> Result<(), RuleManagerError> { let rule_manager = RuleManager::new(); @@ -288,55 +188,6 @@ async fn test_handle_msg_multiple_clients_same_rule() -> Result<(), RuleManagerE Ok(()) } -#[tokio::test] -async fn test_get_all_rules_empty() { - let rule_manager = RuleManager::new(); - let rules = rule_manager.get_all_rules().await; - assert!(rules.is_empty()); -} - -#[tokio::test] -async fn test_get_all_rules_multiple() -> Result<(), RuleManagerError> { - let rule_manager = RuleManager::new(); - let client = ClientId("test_client".to_string()); - - let rule1 = Rule::new( - RuleId("rule_1".to_string()), - Topic("topic1".to_string()), - core::time::Duration::from_secs(60), - "a > 10".to_owned(), - ); - - let rule2 = Rule::new( - RuleId("rule_2".to_string()), - Topic("topic2".to_string()), - core::time::Duration::from_secs(30), - "b < 5".to_owned(), - ); - - let rule3 = Rule::new( - RuleId("rule_3".to_string()), - Topic("topic3".to_string()), - core::time::Duration::from_secs(45), - "c == 0".to_owned(), - ); - - rule_manager.add_rule(client.clone(), rule1).await?; - rule_manager.add_rule(client.clone(), rule2).await?; - rule_manager.add_rule(client, rule3).await?; - - let rules = rule_manager.get_all_rules().await; - assert_eq!(rules.len(), 3); - - // Verify all topics are present - let topics: Vec<_> = rules.iter().map(|rule| &rule.topic.0).collect(); - assert!(topics.contains(&&"topic1".to_string())); - assert!(topics.contains(&&"topic2".to_string())); - assert!(topics.contains(&&"topic3".to_string())); - - Ok(()) -} - fn check_rules_present(rules: Vec, prefix: &str, k: usize) { assert_eq!(rules.len(), k); let topics = rules.into_iter().map(|r| r.topic.0).collect::>(); @@ -439,110 +290,6 @@ async fn test_rule_manager_concurrent_delete_rule() -> Result<(), RuleManagerErr Ok(()) } -#[tokio::test] -async fn test_concurrent_handle_msg_stress() -> Result<(), RuleManagerError> { - let num_clients = 10; - let num_messages = 50; - let rule_manager = std::sync::Arc::new(RuleManager::new()); - - // Set up multiple clients with rules on the same topic - for i in 0..num_clients { - let client = ClientId(format!("stress_client_{}", i)); - let rule = Rule::new( - RuleId(format!("stress_rule_{}", i)), - Topic("stress/topic".to_string()), - core::time::Duration::from_secs(1), - format!("a > {}", i * 2), // Different thresholds: 0, 2, 4, 6, 8, 10, 12, 14, 16, 18 - ); - rule_manager.add_rule(client, rule).await?; - } - - // Verify setup - assert_eq!(rule_manager.get_all_rules().await.len(), num_clients); - assert_eq!(rule_manager.get_all_clients().await.len(), num_clients); - - let client_datas = Arc::new( - (0..num_messages) - .map(|i| { - ClientData { - run_id: 1, - name: "stress/topic".to_string(), - unit: "test".to_string(), - values: vec![(i % 20) as f32], // Values from 0-19 - timestamp: Utc::now(), - } - }) - .collect::>(), - ); - - // Spawn many concurrent message handlers - (0..num_messages) - .fold(JoinSet::new(), |mut set, i| { - let rm = rule_manager.clone(); - let client_datas = client_datas.clone(); - set.spawn(async move { - let empty = rm.handle_msg(&client_datas[i]).await; - assert!(empty.is_ok_and(|op| op.is_none())); - }); - set - }) - .join_all() - .await; - - tokio::time::sleep(Duration::from_secs(2)).await; - - // Now send the messages again to trigger rules after debounce - let results: Vec<_> = (0..num_messages) - .fold(JoinSet::new(), |mut set, i| { - let rm = rule_manager.clone(); - let client_datas = client_datas.clone(); - set.spawn(async move { - let result = rm.handle_msg(&client_datas[i]).await; - - assert!(result.is_ok()); - (i, result) - }); - set - }) - .join_all() - .await; - - // Verify all messages were processed - assert_eq!(results.len(), num_messages); - - // Count notifications generated - let total_notifications: usize = results - .iter() - .map(|(_, result)| { - result - .as_ref() - .unwrap() - .as_ref() - .map(|notifications| notifications.len()) - .unwrap_or(0) - }) - .sum(); - - println!("Total notifications generated: {}", total_notifications); - // Should have some notifications, but exact count depends on timing - assert!(total_notifications > 0); - - results.iter().for_each(|(i, result)| { - assert!(result.is_ok()); - - if let Some(notifications) = result.as_ref().unwrap() { - for (client_id, notification) in notifications { - assert!(client_id.0.starts_with("stress_client_")); - assert_eq!(notification.topic.0, "stress/topic"); - assert!(notification.id.0.starts_with("stress_rule_")); - assert_eq!(notification.values, vec![(i % 20) as f32]); - } - } - }); - - Ok(()) -} - #[tokio::test] async fn test_concurrent_topic_index_stress() -> Result<(), RuleManagerError> { let num_topics = 20; @@ -811,186 +558,3 @@ async fn test_concurrent_high_frequency_messages() -> Result<(), RuleManagerErro Ok(()) } - -#[tokio::test] -async fn test_concurrent_rule_expression_evaluation() -> Result<(), RuleManagerError> { - let num_expressions = 20; - let rule_manager = std::sync::Arc::new(RuleManager::new()); - - // Create rules with different complex expressions - let expressions = vec![ - ("simple_gt", "a > 10"), - ("simple_and", "a > 10 && b < 5"), - ("addition", "a + b > 20"), - ("multiplication", "a * b < 100"), - ("comparison", "a > b + c"), - ("or_condition", "(a > 10) || (b > 20)"), - ("all_positive", "a >= 0 && b >= 0 && c >= 0"), - ("complex_and_or", "a > 5 && (b > 3 || c > 7)"), - ("modulo", "a % 2 == 0"), - ("range_check", "a > 0 && a < 50"), - ]; - - let results: Vec<_> = (0..num_expressions) - .fold(JoinSet::new(), |mut set, i| { - let rm = rule_manager.clone(); - let (expr_name, expr) = expressions[i % expressions.len()].clone(); - set.spawn(async move { - let client = ClientId(format!("expr_client_{}_{}", i, expr_name)); - let rule = Rule::new( - RuleId(format!("expr_rule_{}_{}", i, expr_name)), - Topic("expr/topic".to_string()), - core::time::Duration::from_millis(25), - expr.to_string(), - ); - rm.add_rule(client.clone(), rule) - .await - .map(|_| (i, expr_name, client)) - }); - set - }) - .join_all() - .await; - - // Verify all rules were added successfully - let successful_additions: Vec<_> = results.into_iter().filter_map(|r| r.ok()).collect(); - assert_eq!(successful_additions.len(), num_expressions); - assert_eq!(rule_manager.get_all_rules().await.len(), num_expressions); - - // Send messages with different value combinations concurrently - let num_message_sets = 50; - let message_results: Vec<_> = (0..num_message_sets) - .fold(JoinSet::new(), |mut set, i| { - let rm = rule_manager.clone(); - set.spawn(async move { - let a_val = (i % 30) as f32; // a: 0-29 - let b_val = ((i * 2) % 25) as f32; // b: 0-24 - let c_val = ((i * 3) % 15) as f32; // c: 0-14 - - let client_data = ClientData { - run_id: 1, - name: "expr/topic".to_string(), - unit: "test".to_string(), - values: vec![a_val, b_val, c_val], - timestamp: Utc::now(), - }; - - let result = rm.handle_msg(&client_data).await; - (i, a_val, b_val, c_val, result) - }); - set - }) - .join_all() - .await; - - // Verify all messages were processed - assert_eq!(message_results.len(), num_message_sets); - - let successful_messages: Vec<_> = message_results - .iter() - .filter(|(_, _, _, _, result)| result.is_ok()) - .collect(); - assert_eq!(successful_messages.len(), num_message_sets); - - // Count total notifications (some may be None due to debounce) - let total_notifications: usize = message_results - .iter() - .filter_map(|(_, _, _, _, result)| result.as_ref().ok()?.as_ref().map(|n| n.len())) - .sum(); - - println!( - "Expression evaluation test - Total notifications: {}", - total_notifications - ); - - // Wait for debounce and send specific test messages - tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; - - // Test specific value combinations that should trigger known expressions - let test_cases = vec![ - (15.0, 3.0, 2.0), // Should trigger: a > 10, a > 10 && b < 5, etc. - (25.0, 30.0, 8.0), // Should trigger: (a > 10) || (b > 20), etc. - (2.0, 1.0, 1.0), // Should trigger: a % 2 == 0, all_positive, etc. - (0.0, 0.0, 0.0), // Should trigger: all_positive, a % 2 == 0 - ]; - - for (a, b, c) in test_cases { - tokio::time::sleep(tokio::time::Duration::from_millis(50)).await; - - let client_data = ClientData { - run_id: 999, - name: "expr/topic".to_string(), - unit: "test".to_string(), - values: vec![a, b, c], - timestamp: Utc::now(), - }; - - let result = rule_manager.handle_msg(&client_data).await; - assert!( - result.is_ok(), - "Failed to process test case ({}, {}, {})", - a, - b, - c - ); - - if let Ok(Some(notifications)) = result { - assert!( - !notifications.is_empty(), - "Test case ({}, {}, {}) should trigger some rules", - a, - b, - c - ); - assert!(notifications.len() <= num_expressions); - - // Verify notification structure - for (client_id, notification) in notifications { - assert!(client_id.0.starts_with("expr_client_")); - assert_eq!(notification.topic.0, "expr/topic"); - assert_eq!(notification.values, vec![a, b, c]); - } - } - } - - // Verify final state - assert_eq!(rule_manager.get_all_rules().await.len(), num_expressions); - assert_eq!(rule_manager.get_all_clients().await.len(), num_expressions); - - Ok(()) -} - -#[tokio::test] -async fn test_complex_rule_expression() -> Result<(), RuleManagerError> { - let rule_manager = RuleManager::new(); - let client = ClientId("test_client".to_string()); - - let rule = Rule::new( - RuleId("complex_rule".to_string()), - Topic("sensor/data".to_string()), - core::time::Duration::from_millis(100), - "a > 10 && b < 5 && c >= 0".to_owned(), // Complex expression with multiple variables - ); - - rule_manager.add_rule(client.clone(), rule).await?; - - let client_data = ClientData { - run_id: 1, - name: "sensor/data".to_string(), - unit: "mixed".to_string(), - values: vec![15.0, 3.0, 1.0], // a=15 > 10, b=3 < 5, c=1 >= 0 - should trigger - timestamp: Utc::now(), - }; - - // Prime the rule - let _ = rule_manager.handle_msg(&client_data).await; - tokio::time::sleep(tokio::time::Duration::from_millis(150)).await; - - let result = rule_manager.handle_msg(&client_data).await?; - if let Some(notifications) = result { - assert!(!notifications.is_empty()); - assert_eq!(notifications[0].0 .0, client.0); - } - - Ok(()) -} From d348c1b08a4e91d1723fe4fc2dea35864d7de839 Mon Sep 17 00:00:00 2001 From: suryatho Date: Thu, 4 Dec 2025 20:44:11 -0500 Subject: [PATCH 11/13] Remove redundant tests from bimultimap tests #475 --- scylla-server/tests/bimultimap_test.rs | 143 ----------------------- scylla-server/tests/rule_structs_test.rs | 16 --- 2 files changed, 159 deletions(-) diff --git a/scylla-server/tests/bimultimap_test.rs b/scylla-server/tests/bimultimap_test.rs index 2a406df9..24b76528 100644 --- a/scylla-server/tests/bimultimap_test.rs +++ b/scylla-server/tests/bimultimap_test.rs @@ -1,13 +1,5 @@ use scylla_server::rule_structs::{BiMapRemoveResult, BiMultiMap, ClientId, RuleId}; -// Tests for BiMultiMap -#[test] -fn test_bi_multi_map_new() { - let bimap: BiMultiMap = BiMultiMap::new(); - assert!(bimap.get_left(&1).is_none()); - assert!(bimap.get_right(&"test".to_string()).is_none()); -} - #[test] fn test_bi_multi_map_insert_single() { let mut bimap = BiMultiMap::new(); @@ -47,30 +39,6 @@ fn test_bi_multi_map_insert_multiple_rights() { assert!(bimap.get_left(&right3).unwrap().contains(&left)); } -#[test] -fn test_bi_multi_map_insert_multiple_lefts() { - let mut bimap = BiMultiMap::new(); - let left1 = "client1".to_string(); - let left2 = "client2".to_string(); - let left3 = "client3".to_string(); - let right = 42; - - bimap.insert(&left1, &right); - bimap.insert(&left2, &right); - bimap.insert(&left3, &right); - - let lefts = bimap.get_left(&right).unwrap(); - assert_eq!(lefts.len(), 3); - assert!(lefts.contains(&left1)); - assert!(lefts.contains(&left2)); - assert!(lefts.contains(&left3)); - - // Each left should map to the right - assert!(bimap.get_right(&left1).unwrap().contains(&right)); - assert!(bimap.get_right(&left2).unwrap().contains(&right)); - assert!(bimap.get_right(&left3).unwrap().contains(&right)); -} - #[test] fn test_bi_multi_map_insert_many_to_many() { let mut bimap = BiMultiMap::new(); @@ -110,20 +78,6 @@ fn test_bi_multi_map_insert_many_to_many() { assert!(lefts_for_right2.contains(&left2)); } -#[test] -fn test_bi_multi_map_insert_duplicate() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right = 42; - - bimap.insert(&left, &right); - bimap.insert(&left, &right); // Duplicate insertion - - // Should still only have one mapping - assert_eq!(bimap.get_right(&left).unwrap().len(), 1); - assert_eq!(bimap.get_left(&right).unwrap().len(), 1); -} - #[test] fn test_bi_multi_map_remove_left_single() { let mut bimap = BiMultiMap::new(); @@ -179,103 +133,6 @@ fn test_bi_multi_map_remove_left_nonexistent() { assert!(matches!(result, BiMapRemoveResult::NothingToRemove)); } -#[test] -fn test_bi_multi_map_remove_right_single() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right = 42; - - bimap.insert(&left, &right); - - let result = bimap.remove_right(&right); - assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); - - if let BiMapRemoveResult::RemovedWithCleanUp(removed_lefts) = result { - assert_eq!(removed_lefts.len(), 1); - assert!(removed_lefts.contains(&left)); - } - - // Verify both directions are cleaned up - assert!(bimap.get_right(&left).is_none()); - assert!(bimap.get_left(&right).is_none()); -} - -#[test] -fn test_bi_multi_map_remove_right_shared_left() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right1 = 42; - let right2 = 43; - - bimap.insert(&left, &right1); - bimap.insert(&left, &right2); - - let result = bimap.remove_right(&right1); - assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); - - // right1 should be gone - assert!(bimap.get_left(&right1).is_none()); - - // left should still exist and map to right2 - let remaining_rights = bimap.get_right(&left).unwrap(); - assert_eq!(remaining_rights.len(), 1); - assert!(remaining_rights.contains(&right2)); - - // right2 should still map to left - assert!(bimap.get_left(&right2).unwrap().contains(&left)); -} - -#[test] -fn test_bi_multi_map_remove_right_from_left() { - let mut bimap = BiMultiMap::new(); - let left = "client1".to_string(); - let right1 = 42; - let right2 = 43; - - bimap.insert(&left, &right1); - bimap.insert(&left, &right2); - - let result = bimap.remove_right_from_left(&left, &right1); - assert!(matches!(result, BiMapRemoveResult::RemovedWithCleanUp(_))); - - if let BiMapRemoveResult::RemovedWithCleanUp(removed_right) = result { - assert_eq!(removed_right, right1); - } - - // left should still exist but only map to right2 - let remaining_rights = bimap.get_right(&left).unwrap(); - assert_eq!(remaining_rights.len(), 1); - assert!(remaining_rights.contains(&right2)); - - // right1 should be completely removed - assert!(bimap.get_left(&right1).is_none()); - - // right2 should still map to left - assert!(bimap.get_left(&right2).unwrap().contains(&left)); -} - -#[test] -fn test_bi_multi_map_remove_right_from_left_shared_right() { - let mut bimap = BiMultiMap::new(); - let left1 = "client1".to_string(); - let left2 = "client2".to_string(); - let right = 42; - - bimap.insert(&left1, &right); - bimap.insert(&left2, &right); - - let result = bimap.remove_right_from_left(&left1, &right); - assert!(matches!(result, BiMapRemoveResult::RemovedOnly)); - - // left1 should be gone - assert!(bimap.get_right(&left1).is_none()); - - // right should still exist and map to left2 - let remaining_lefts = bimap.get_left(&right).unwrap(); - assert_eq!(remaining_lefts.len(), 1); - assert!(remaining_lefts.contains(&left2)); -} - #[test] fn test_bi_multi_map_remove_right_from_left_nonexistent() { let mut bimap: BiMultiMap = BiMultiMap::new(); diff --git a/scylla-server/tests/rule_structs_test.rs b/scylla-server/tests/rule_structs_test.rs index 6c30ab48..97343a01 100644 --- a/scylla-server/tests/rule_structs_test.rs +++ b/scylla-server/tests/rule_structs_test.rs @@ -82,22 +82,6 @@ async fn test_delete_client_success() -> Result<(), RuleManagerError> { Ok(()) } -#[tokio::test] -async fn test_handle_msg_no_matching_rule() { - let rule_manager = RuleManager::new(); - - let client_data = ClientData { - run_id: 1, - name: "nonexistent/topic".to_string(), - unit: "test_unit".to_string(), - values: vec![15.0, 20.0], - timestamp: Utc::now(), - }; - - let result = rule_manager.handle_msg(&client_data).await; - assert!(matches!(result, Err(RuleManagerError::NoMatchingRule))); -} - #[tokio::test] async fn test_handle_msg_rule_triggered() -> Result<(), RuleManagerError> { let rule_manager = RuleManager::new(); From ac5a78f867eb47a6928b9482693a58a227214f29 Mon Sep 17 00:00:00 2001 From: suryatho Date: Fri, 5 Dec 2025 01:13:27 -0500 Subject: [PATCH 12/13] Make handle_msg more performant ? #475 --- scylla-server/src/rule_structs.rs | 135 ++++++++++++++--------- scylla-server/tests/rule_structs_test.rs | 5 +- 2 files changed, 84 insertions(+), 56 deletions(-) diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index 9df51d35..d84b0152 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -12,8 +12,10 @@ use serde_with::serde_as; use serde_with::DurationSeconds; use std::borrow::Borrow; use std::hash::Hash; +use std::sync::Arc; use std::time::Duration; use tokio::sync::RwLock; +use tokio::task::JoinSet; use tracing::trace; use tracing::warn; @@ -348,7 +350,7 @@ impl RuleManager { /// Handles a new socket message, returning a RuleNotification for one to many clients if action should be taken pub async fn handle_msg( - &self, + self: &Arc, data: &ClientData, ) -> Result>, RuleManagerError> { // Read from topic to rule index and drop lock immediately @@ -360,65 +362,90 @@ impl RuleManager { } }; - let mut notifications: Vec<(ClientId, RuleNotification)> = Vec::new(); - for rule_id in rule_ids { - let (triggered_result, clients_result) = { - // Future for if rule was triggered - let triggered_future = async { - if let Some(rule) = self.rules.write().await.get_mut(&rule_id) { - if let Some(triggered) = rule.tick(&data.values) { - Ok(triggered) + let data = Arc::new(data.clone()); + let notifications = rule_ids + .into_iter() + .fold(JoinSet::new(), |mut set, rule_id| { + let data = data.clone(); + let self_ref = self.clone(); + set.spawn(async move { + let triggered_fut = async { + if let Some(rule) = self_ref.rules.write().await.get_mut(&rule_id) { + if let Some(triggered) = rule.tick(&data.values) { + Ok(triggered) + } else { + Err(RuleManagerError::RuleFailure) + } } else { - Err(RuleManagerError::RuleFailure) + trace!("Could not find rule in rules map: {}", rule_id); + Err(RuleManagerError::NoMatchingRule) + } + }; + + // Future for getting subscribed clients + let clients_fut = async { + self_ref + .subscriptions + .read() + .await + .get_left(&rule_id) + .cloned() + }; + + tokio::pin!(triggered_fut); + tokio::pin!(clients_fut); + + // Check which operation finished first + let rule_id = rule_id.clone(); + tokio::select! { + triggered_result = &mut triggered_fut => { + match triggered_result { + Ok(true) => (Ok(true), clients_fut.await, rule_id), + Ok(false) => (Ok(false), None, rule_id), + _ => (triggered_result, None, rule_id), + } + }, + clients_result = &mut clients_fut => { + match clients_result { + Some(_) => (triggered_fut.await, clients_result, rule_id), + None => (Ok(false), None, rule_id) + } } - } else { - trace!("Could not find rule in rules map: {}", rule_id); - Err(RuleManagerError::NoMatchingRule) } + }); + set + }) + .join_all() + .await + .into_iter() + .filter_map(|(triggered_res, clients_op, rule_id)| { + let Ok(triggered) = triggered_res else { + return Some(Err(triggered_res.unwrap_err())); }; - // Future for getting subscribed clients - let clients_future = - async { self.subscriptions.read().await.get_left(&rule_id).cloned() }; - - tokio::pin!(triggered_future); - tokio::pin!(clients_future); - - // Check which operation finished first - tokio::select! { - triggered_result = &mut triggered_future => { - match triggered_result? { - true => (Ok(true), clients_future.await), - false => (Ok(false), None), - } - }, - clients_result = &mut clients_future => { - match clients_result { - Some(_) => (triggered_future.await, clients_result), - None => (Ok(false), None) - } - } + if !triggered || clients_op.is_none() { + None + } else { + Some(Ok(clients_op + .unwrap() + .into_iter() + .map(|client| { + ( + client, + RuleNotification { + id: rule_id.clone(), + topic: Topic(data.name.clone()), + values: data.values.clone(), + time: data.timestamp, + }, + ) + }) + .collect::>())) } - }; - - let triggered = triggered_result?; - if !triggered || clients_result.is_none() { - continue; - } - - // Push notifications for all clients who are subscribed to this rule - for client in clients_result.unwrap() { - notifications.push(( - client.clone(), - RuleNotification { - id: rule_id.clone(), - topic: Topic(data.name.clone()), - values: data.values.clone(), - time: data.timestamp, - }, - )); - } - } + }) + .flatten() + .flatten() + .collect::>(); if notifications.is_empty() { Ok(None) diff --git a/scylla-server/tests/rule_structs_test.rs b/scylla-server/tests/rule_structs_test.rs index 97343a01..64d075cb 100644 --- a/scylla-server/tests/rule_structs_test.rs +++ b/scylla-server/tests/rule_structs_test.rs @@ -1,6 +1,7 @@ use chrono::Utc; use scylla_server::rule_structs::*; use scylla_server::ClientData; +use std::sync::Arc; use tokio::task::JoinSet; #[tokio::test] @@ -84,7 +85,7 @@ async fn test_delete_client_success() -> Result<(), RuleManagerError> { #[tokio::test] async fn test_handle_msg_rule_triggered() -> Result<(), RuleManagerError> { - let rule_manager = RuleManager::new(); + let rule_manager = Arc::new(RuleManager::new()); let client = ClientId("test_client".to_string()); let rule = Rule::new( @@ -123,7 +124,7 @@ async fn test_handle_msg_rule_triggered() -> Result<(), RuleManagerError> { #[tokio::test] async fn test_handle_msg_multiple_clients_same_rule() -> Result<(), RuleManagerError> { - let rule_manager = RuleManager::new(); + let rule_manager = Arc::new(RuleManager::new()); let client1 = ClientId("client1".to_string()); let client2 = ClientId("client2".to_string()); From 7fa63c99b1a296d53e8f4c091f94510591071cf4 Mon Sep 17 00:00:00 2001 From: suryatho Date: Mon, 15 Dec 2025 13:10:07 -0500 Subject: [PATCH 13/13] Undo changes to handle_msg #475 --- scylla-server/src/rule_structs.rs | 135 +++++++++-------------- scylla-server/tests/rule_structs_test.rs | 5 +- 2 files changed, 56 insertions(+), 84 deletions(-) diff --git a/scylla-server/src/rule_structs.rs b/scylla-server/src/rule_structs.rs index d84b0152..9df51d35 100644 --- a/scylla-server/src/rule_structs.rs +++ b/scylla-server/src/rule_structs.rs @@ -12,10 +12,8 @@ use serde_with::serde_as; use serde_with::DurationSeconds; use std::borrow::Borrow; use std::hash::Hash; -use std::sync::Arc; use std::time::Duration; use tokio::sync::RwLock; -use tokio::task::JoinSet; use tracing::trace; use tracing::warn; @@ -350,7 +348,7 @@ impl RuleManager { /// Handles a new socket message, returning a RuleNotification for one to many clients if action should be taken pub async fn handle_msg( - self: &Arc, + &self, data: &ClientData, ) -> Result>, RuleManagerError> { // Read from topic to rule index and drop lock immediately @@ -362,90 +360,65 @@ impl RuleManager { } }; - let data = Arc::new(data.clone()); - let notifications = rule_ids - .into_iter() - .fold(JoinSet::new(), |mut set, rule_id| { - let data = data.clone(); - let self_ref = self.clone(); - set.spawn(async move { - let triggered_fut = async { - if let Some(rule) = self_ref.rules.write().await.get_mut(&rule_id) { - if let Some(triggered) = rule.tick(&data.values) { - Ok(triggered) - } else { - Err(RuleManagerError::RuleFailure) - } + let mut notifications: Vec<(ClientId, RuleNotification)> = Vec::new(); + for rule_id in rule_ids { + let (triggered_result, clients_result) = { + // Future for if rule was triggered + let triggered_future = async { + if let Some(rule) = self.rules.write().await.get_mut(&rule_id) { + if let Some(triggered) = rule.tick(&data.values) { + Ok(triggered) } else { - trace!("Could not find rule in rules map: {}", rule_id); - Err(RuleManagerError::NoMatchingRule) - } - }; - - // Future for getting subscribed clients - let clients_fut = async { - self_ref - .subscriptions - .read() - .await - .get_left(&rule_id) - .cloned() - }; - - tokio::pin!(triggered_fut); - tokio::pin!(clients_fut); - - // Check which operation finished first - let rule_id = rule_id.clone(); - tokio::select! { - triggered_result = &mut triggered_fut => { - match triggered_result { - Ok(true) => (Ok(true), clients_fut.await, rule_id), - Ok(false) => (Ok(false), None, rule_id), - _ => (triggered_result, None, rule_id), - } - }, - clients_result = &mut clients_fut => { - match clients_result { - Some(_) => (triggered_fut.await, clients_result, rule_id), - None => (Ok(false), None, rule_id) - } + Err(RuleManagerError::RuleFailure) } + } else { + trace!("Could not find rule in rules map: {}", rule_id); + Err(RuleManagerError::NoMatchingRule) } - }); - set - }) - .join_all() - .await - .into_iter() - .filter_map(|(triggered_res, clients_op, rule_id)| { - let Ok(triggered) = triggered_res else { - return Some(Err(triggered_res.unwrap_err())); }; - if !triggered || clients_op.is_none() { - None - } else { - Some(Ok(clients_op - .unwrap() - .into_iter() - .map(|client| { - ( - client, - RuleNotification { - id: rule_id.clone(), - topic: Topic(data.name.clone()), - values: data.values.clone(), - time: data.timestamp, - }, - ) - }) - .collect::>())) + // Future for getting subscribed clients + let clients_future = + async { self.subscriptions.read().await.get_left(&rule_id).cloned() }; + + tokio::pin!(triggered_future); + tokio::pin!(clients_future); + + // Check which operation finished first + tokio::select! { + triggered_result = &mut triggered_future => { + match triggered_result? { + true => (Ok(true), clients_future.await), + false => (Ok(false), None), + } + }, + clients_result = &mut clients_future => { + match clients_result { + Some(_) => (triggered_future.await, clients_result), + None => (Ok(false), None) + } + } } - }) - .flatten() - .flatten() - .collect::>(); + }; + + let triggered = triggered_result?; + if !triggered || clients_result.is_none() { + continue; + } + + // Push notifications for all clients who are subscribed to this rule + for client in clients_result.unwrap() { + notifications.push(( + client.clone(), + RuleNotification { + id: rule_id.clone(), + topic: Topic(data.name.clone()), + values: data.values.clone(), + time: data.timestamp, + }, + )); + } + } if notifications.is_empty() { Ok(None) diff --git a/scylla-server/tests/rule_structs_test.rs b/scylla-server/tests/rule_structs_test.rs index 64d075cb..97343a01 100644 --- a/scylla-server/tests/rule_structs_test.rs +++ b/scylla-server/tests/rule_structs_test.rs @@ -1,7 +1,6 @@ use chrono::Utc; use scylla_server::rule_structs::*; use scylla_server::ClientData; -use std::sync::Arc; use tokio::task::JoinSet; #[tokio::test] @@ -85,7 +84,7 @@ async fn test_delete_client_success() -> Result<(), RuleManagerError> { #[tokio::test] async fn test_handle_msg_rule_triggered() -> Result<(), RuleManagerError> { - let rule_manager = Arc::new(RuleManager::new()); + let rule_manager = RuleManager::new(); let client = ClientId("test_client".to_string()); let rule = Rule::new( @@ -124,7 +123,7 @@ async fn test_handle_msg_rule_triggered() -> Result<(), RuleManagerError> { #[tokio::test] async fn test_handle_msg_multiple_clients_same_rule() -> Result<(), RuleManagerError> { - let rule_manager = Arc::new(RuleManager::new()); + let rule_manager = RuleManager::new(); let client1 = ClientId("client1".to_string()); let client2 = ClientId("client2".to_string());