From 58f47933084ed494227b8a9199fd1743ca54645f Mon Sep 17 00:00:00 2001 From: ppom Date: Fri, 8 Aug 2025 12:00:00 +0200 Subject: [PATCH] Fix triggers being forgotten on after actions with on_exit: true decrement_trigger do not delete triggers anymore when exiting test still failing because filters start before start commands --- TODO | 2 + src/daemon/filter/mod.rs | 4 +- src/daemon/filter/state.rs | 78 ++++++++++++++++++++++++++++---------- tests/start_stop.jsonnet | 5 ++- tests/start_stop.rs | 46 ++++++++++++---------- 5 files changed, 90 insertions(+), 45 deletions(-) diff --git a/TODO b/TODO index 586b4e6..dfa721f 100644 --- a/TODO +++ b/TODO @@ -4,3 +4,5 @@ stream: test regex ending with $ should an ipv6-mapped ipv4 match a pattern of type ipv6? should it be normalized as ipv4 then? + +fix filter commands executing before start commands diff --git a/src/daemon/filter/mod.rs b/src/daemon/filter/mod.rs index 790c637..9d2c322 100644 --- a/src/daemon/filter/mod.rs +++ b/src/daemon/filter/mod.rs @@ -268,7 +268,7 @@ impl FilterManager { let m = m.clone(); if exec_time <= now { - if state.decrement_trigger(&m, t) { + if state.decrement_trigger(&m, t, false) { exec_now(&self.exec_limit, self.shutdown.clone(), action, m); } } else { @@ -289,7 +289,7 @@ impl FilterManager { if !exiting || action.on_exit { #[allow(clippy::unwrap_used)] // propagating panics is ok let mut state = this.state.lock().unwrap(); - if state.decrement_trigger(&m, t) { + if state.decrement_trigger(&m, t, exiting) { exec_now(&this.exec_limit, this.shutdown, action, m); } } diff --git a/src/daemon/filter/state.rs b/src/daemon/filter/state.rs index d18bb0a..ec7e9ab 100644 --- a/src/daemon/filter/state.rs +++ b/src/daemon/filter/state.rs @@ -140,7 +140,7 @@ impl State { } /// Returns whether we should still execute an action for this (Match, Time) trigger - pub fn decrement_trigger(&mut self, m: &Match, t: Time) -> bool { + pub fn decrement_trigger(&mut self, m: &Match, t: Time, exiting: bool) -> bool { // We record triggered filters only when there is an action with an `after` directive if self.has_after { let mut exec_needed = false; @@ -153,16 +153,24 @@ impl State { if let Some(count) = count { exec_needed = true; if count <= 1 { - self.triggers.fetch_update(mt.m, |map| { - map.and_then(|mut map| { - map.remove(&mt.t); - if map.is_empty() { - None - } else { - Some(map) - } - }) - }); + if !exiting { + self.triggers.fetch_update(mt.m, |map| { + map.and_then(|mut map| { + map.remove(&mt.t); + if map.is_empty() { + None + } else { + Some(map) + } + }) + }); + } + // else don't do anything + // Because that will remove the entry in the DB, and make + // it forget this trigger. + // Maybe we should have 2 maps for triggers: + // - The current for action counting, not persisted + // - Another like ordered_times, Tree, persisted } else { self.triggers.fetch_update(mt.m, |map| { map.map(|mut map| { @@ -429,7 +437,7 @@ mod tests { assert!(state.triggers.tree().is_empty()); // Will be called immediately after, it returns true - assert!(state.decrement_trigger(&one, now)); + assert!(state.decrement_trigger(&one, now, false)); } #[tokio::test] @@ -484,22 +492,52 @@ mod tests { &BTreeMap::from([(one.clone(), [(now, 3)].into())]) ); // Decrement → true - assert!(state.decrement_trigger(&one, now)); + assert!(state.decrement_trigger(&one, now, false)); assert_eq!( state.triggers.tree(), &BTreeMap::from([(one.clone(), [(now, 2)].into())]) ); // Decrement → true - assert!(state.decrement_trigger(&one, now)); + assert!(state.decrement_trigger(&one, now, false)); assert_eq!( state.triggers.tree(), &BTreeMap::from([(one.clone(), [(now, 1)].into())]) ); // Decrement → true - assert!(state.decrement_trigger(&one, now)); + assert!(state.decrement_trigger(&one, now, false)); assert!(state.triggers.tree().is_empty()); // Decrement → false - assert!(!state.decrement_trigger(&one, now)); + assert!(!state.decrement_trigger(&one, now, false)); + + // Add unique trigger (but decrement exiting-like) + state.add_trigger(one.clone(), now, None); + assert_eq!( + state.triggers.tree(), + &BTreeMap::from([(one.clone(), [(now, 3)].into())]) + ); + // Decrement → true + assert!(state.decrement_trigger(&one, now, true)); + assert_eq!( + state.triggers.tree(), + &BTreeMap::from([(one.clone(), [(now, 2)].into())]) + ); + // Decrement → true + assert!(state.decrement_trigger(&one, now, true)); + assert_eq!( + state.triggers.tree(), + &BTreeMap::from([(one.clone(), [(now, 1)].into())]) + ); + // Decrement but exiting → true, does nothing + assert!(state.decrement_trigger(&one, now, true)); + assert_eq!( + state.triggers.tree(), + &BTreeMap::from([(one.clone(), [(now, 1)].into())]) + ); + // Decrement → true + assert!(state.decrement_trigger(&one, now, false)); + assert!(state.triggers.tree().is_empty()); + // Decrement → false + assert!(!state.decrement_trigger(&one, now, false)); // Add trigger with neighbour state.add_trigger(one.clone(), now, None); @@ -509,25 +547,25 @@ mod tests { &BTreeMap::from([(one.clone(), [(now_plus_1s, 3), (now, 3)].into())]) ); // Decrement → true - assert!(state.decrement_trigger(&one, now)); + assert!(state.decrement_trigger(&one, now, false)); assert_eq!( state.triggers.tree(), &BTreeMap::from([(one.clone(), [(now_plus_1s, 3), (now, 2)].into())]) ); // Decrement → true - assert!(state.decrement_trigger(&one, now)); + assert!(state.decrement_trigger(&one, now, false)); assert_eq!( state.triggers.tree(), &BTreeMap::from([(one.clone(), [(now_plus_1s, 3), (now, 1)].into())]) ); // Decrement → true - assert!(state.decrement_trigger(&one, now)); + assert!(state.decrement_trigger(&one, now, false)); assert_eq!( state.triggers.tree(), &BTreeMap::from([(one.clone(), [(now_plus_1s, 3)].into())]) ); // Decrement → false - assert!(!state.decrement_trigger(&one, now)); + assert!(!state.decrement_trigger(&one, now, false)); // Remove neighbour state.remove_trigger(&one); assert!(state.triggers.tree().is_empty()); diff --git a/tests/start_stop.jsonnet b/tests/start_stop.jsonnet index a85636c..1e483da 100644 --- a/tests/start_stop.jsonnet +++ b/tests/start_stop.jsonnet @@ -25,6 +25,7 @@ local echo(message, before='true') = [ cmd: ['sh', '-c', 'seq 2 | while read i; do echo runtime $i; sleep 0.1; done'], filters: { f1: { + duplicate: 'rerun', regex: [ '^runtime $', ], @@ -33,8 +34,8 @@ local echo(message, before='true') = [ cmd: echo('runtime '), }, two: { - cmd: echo('after', before='sleep 1'), - after: '5s', + cmd: echo('after', before='sleep 0.2'), + after: '5m', onexit: true, }, }, diff --git a/tests/start_stop.rs b/tests/start_stop.rs index 15d1ee3..64112f5 100644 --- a/tests/start_stop.rs +++ b/tests/start_stop.rs @@ -5,7 +5,6 @@ use assert_fs::{prelude::*, TempDir}; use predicates::prelude::predicate; #[test] -// #[ignore = "currently failing"] // FIXME fn start_stop() { let tmp_dir = assert_fs::TempDir::new().unwrap(); @@ -27,27 +26,32 @@ fn start_stop() { tmp_dir.child("log").assert(&output.join("\n")); tmp_dir.child("log").write_str("").unwrap(); - // // Second run - // run_reaction(&tmp_dir); + println!( + "DATABASE:\n{}", + std::fs::read_to_string(tmp_dir.child("reaction.db")).unwrap() + ); - // // Expected output - // let output = [ - // "start 1", - // "start 2", - // "runtime 1", - // "runtime 2", - // "runtime 1", - // "runtime 2", - // // no order required because they'll be awaken all together on exit - // "after", - // "after", - // "after", - // "after", - // "stop 1", - // "stop 2", - // "", - // ]; - // tmp_dir.child("log").assert(&output.join("\n")); + // Second run + run_reaction(&tmp_dir); + + // Expected output + let output = [ + "start 1", + "start 2", + "runtime 1", + "runtime 2", + "runtime 1", + "runtime 2", + // no order required because they'll be awaken all together on exit + "after", + "after", + "after", + "after", + "stop 1", + "stop 2", + "", + ]; + tmp_dir.child("log").assert(&output.join("\n")); } fn run_reaction(tmp_dir: &TempDir) {