From c2787198edc5928d54e1c6d704d0c8e0e0369497 Mon Sep 17 00:00:00 2001 From: "coderabbitai[bot]" <136622811+coderabbitai[bot]@users.noreply.github.com> Date: Wed, 8 Apr 2026 12:04:36 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=93=9D=20CodeRabbit=20Chat:=20Implement?= =?UTF-8?q?=20requested=20code=20changes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/agent/routine_engine.rs | 9 +-- tests/e2e_traces.rs | 6 +- tests/e2e_traces/routine_cooldown.rs | 6 +- tests/e2e_traces/routine_cron.rs | 8 +-- tests/e2e_traces/routine_event.rs | 6 +- tests/e2e_traces/routine_system_event.rs | 5 +- tests/support/routine_sync.rs | 90 ++++++++++++++++++++++++ tests/support/routines.rs | 68 ------------------ 8 files changed, 112 insertions(+), 86 deletions(-) create mode 100644 tests/support/routine_sync.rs diff --git a/src/agent/routine_engine.rs b/src/agent/routine_engine.rs index d0652eec3..00bd6f96e 100644 --- a/src/agent/routine_engine.rs +++ b/src/agent/routine_engine.rs @@ -1110,12 +1110,13 @@ async fn send_notification( } } +#[cfg(feature = "libsql")] impl RoutineEngine { - /// Returns the current running count as a read-only snapshot. + /// Returns a clone of the running count atomic counter. /// /// Intended for test synchronisation only — do not use in production paths. - pub fn running_count(&self) -> usize { - self.running_count.load(Ordering::SeqCst) + pub fn running_count(&self) -> Arc { + self.running_count.clone() } } @@ -1303,4 +1304,4 @@ mod tests { assert_eq!(finish_reason_length, crate::llm::FinishReason::Length); assert_eq!(finish_reason_stop, crate::llm::FinishReason::Stop); } -} +} \ No newline at end of file diff --git a/tests/e2e_traces.rs b/tests/e2e_traces.rs index 8868af3e3..7921ae9d7 100644 --- a/tests/e2e_traces.rs +++ b/tests/e2e_traces.rs @@ -3,6 +3,10 @@ mod support; +#[cfg(feature = "libsql")] +#[path = "support/routine_sync.rs"] +mod routine_sync; + #[path = "e2e_traces/advanced_traces.rs"] mod advanced_traces; #[path = "e2e_traces/attachments.rs"] @@ -42,4 +46,4 @@ mod trace_memory; #[path = "e2e_traces/worker_coverage.rs"] mod worker_coverage; #[path = "e2e_traces/workspace_coverage.rs"] -mod workspace_coverage; +mod workspace_coverage; \ No newline at end of file diff --git a/tests/e2e_traces/routine_cooldown.rs b/tests/e2e_traces/routine_cooldown.rs index d6a478b44..d7f85e5a6 100644 --- a/tests/e2e_traces/routine_cooldown.rs +++ b/tests/e2e_traces/routine_cooldown.rs @@ -10,9 +10,9 @@ use chrono::Utc; use ironclaw::agent::routine::Trigger; use ironclaw::db::RoutineRuntimeUpdate; +use crate::routine_sync::{wait_for_idle, wait_for_persisted_run}; use crate::support::routines::{ - create_test_db, create_workspace, make_minimal_engine, make_routine, - make_test_incoming_message, wait_for_idle, wait_for_persisted_run, + create_test_db, create_workspace, make_minimal_engine, make_routine, make_test_incoming_message, }; use crate::support::trace_llm::{LlmTrace, TraceResponse, TraceStep}; @@ -81,4 +81,4 @@ async fn routine_cooldown() { // Second fire should be blocked by cooldown. let fired2 = engine.check_event_triggers(&msg).await; assert_eq!(fired2, 0, "Second fire should be blocked by cooldown"); -} +} \ No newline at end of file diff --git a/tests/e2e_traces/routine_cron.rs b/tests/e2e_traces/routine_cron.rs index 07056209b..5f620a435 100644 --- a/tests/e2e_traces/routine_cron.rs +++ b/tests/e2e_traces/routine_cron.rs @@ -9,10 +9,8 @@ use chrono::Utc; use ironclaw::agent::routine::Trigger; -use crate::support::routines::{ - create_test_db, create_workspace, make_minimal_engine, make_routine, wait_for_idle, - wait_for_persisted_run, -}; +use crate::routine_sync::{wait_for_idle, wait_for_persisted_run}; +use crate::support::routines::{create_test_db, create_workspace, make_minimal_engine, make_routine}; use crate::support::trace_llm::{LlmTrace, TraceResponse, TraceStep}; #[tokio::test] @@ -62,4 +60,4 @@ async fn cron_routine_fires() { // Notification may or may not be sent depending on config; // just verify no panic occurred. Drain the channel. let _ = notify_rx.try_recv(); -} +} \ No newline at end of file diff --git a/tests/e2e_traces/routine_event.rs b/tests/e2e_traces/routine_event.rs index 1d1016f76..9eff02ea5 100644 --- a/tests/e2e_traces/routine_event.rs +++ b/tests/e2e_traces/routine_event.rs @@ -7,9 +7,9 @@ use ironclaw::agent::routine::Trigger; use std::time::Duration; +use crate::routine_sync::{wait_for_idle, wait_for_persisted_run}; use crate::support::routines::{ - create_test_db, create_workspace, make_minimal_engine, make_routine, - make_test_incoming_message, wait_for_idle, wait_for_persisted_run, + create_test_db, create_workspace, make_minimal_engine, make_routine, make_test_incoming_message, }; use crate::support::trace_llm::{LlmTrace, TraceResponse, TraceStep}; @@ -67,4 +67,4 @@ async fn event_trigger_matches() { let non_matching_msg = make_test_incoming_message("check the staging environment"); let fired_neg = engine.check_event_triggers(&non_matching_msg).await; assert_eq!(fired_neg, 0, "Expected 0 routines fired on non-match"); -} +} \ No newline at end of file diff --git a/tests/e2e_traces/routine_system_event.rs b/tests/e2e_traces/routine_system_event.rs index 3d5e56c6a..fb93a2df6 100644 --- a/tests/e2e_traces/routine_system_event.rs +++ b/tests/e2e_traces/routine_system_event.rs @@ -5,9 +5,10 @@ use std::time::Duration; +use crate::routine_sync::{wait_for_idle, wait_for_persisted_run}; use crate::support::routines::{ SystemEventSpec, assert_system_event_count, create_test_db, create_workspace, - make_minimal_engine, register_github_issue_routine, wait_for_idle, wait_for_persisted_run, + make_minimal_engine, register_github_issue_routine, }; use crate::support::trace_llm::{LlmTrace, TraceResponse, TraceStep}; @@ -67,4 +68,4 @@ async fn system_event_trigger_matches_and_filters() { for (spec, expected, msg) in scenarios { assert_system_event_count(&engine, spec, expected, msg).await; } -} +} \ No newline at end of file diff --git a/tests/support/routine_sync.rs b/tests/support/routine_sync.rs new file mode 100644 index 000000000..6c67c2e1c --- /dev/null +++ b/tests/support/routine_sync.rs @@ -0,0 +1,90 @@ +//! Deterministic synchronisation helpers for routine engine E2E tests. +//! +//! Provides `wait_for_idle` and `wait_for_persisted_run` for coordinating +//! asynchronous routine execution and database persistence in tests that +//! trigger `RoutineEngine`'s task-spawning paths. +//! +//! This module is intentionally **not** declared in `support/mod.rs`. It is +//! included directly from `tests/e2e_traces.rs` so that it is only compiled +//! into the test binary that actually calls these helpers, avoiding spurious +//! `dead_code` warnings in unrelated test binaries without requiring any lint +//! suppression. + +#![cfg(feature = "libsql")] + +use std::sync::Arc; +use std::sync::atomic::Ordering; +use std::time::Duration; + +use uuid::Uuid; + +use ironclaw::agent::routine_engine::RoutineEngine; +use ironclaw::db::Database; + +/// Polls until the engine's running count reaches zero or the timeout expires. +/// +/// This provides deterministic synchronisation for tests that need to wait +/// for asynchronous routine execution to complete, eliminating timing-dependent +/// flakiness without slowing down the test suite on fast machines. +/// +/// **Note:** Combine with [`wait_for_persisted_run`] to ensure both execution +/// completion and database persistence, as the running count may reach zero +/// before the database record is fully committed. +pub async fn wait_for_idle(engine: &RoutineEngine, timeout: Duration) { + let start = std::time::Instant::now(); + let poll_interval = Duration::from_millis(10); + + loop { + let count = engine.running_count().load(Ordering::SeqCst); + if count == 0 { + return; + } + + if start.elapsed() >= timeout { + panic!( + "Timeout waiting for engine to become idle (running count: {})", + count + ); + } + + tokio::time::sleep(poll_interval).await; + } +} + +/// Polls until a routine run is persisted in the database or the timeout expires. +/// +/// This helper provides deterministic synchronisation for database persistence, +/// complementing [`wait_for_idle`] which only waits for in-memory execution +/// completion. Call this after `wait_for_idle` to ensure the routine run is +/// durably recorded before asserting on persisted state. +/// +/// # Arguments +/// * `db` - The database to query for persisted runs. +/// * `routine_id` - The ID of the routine to check for runs. +/// * `timeout` - Maximum duration to wait for persistence. +pub async fn wait_for_persisted_run(db: &Arc, routine_id: Uuid, timeout: Duration) { + let start = std::time::Instant::now(); + let poll_interval = Duration::from_millis(10); + let max_attempts: u32 = 500; // At 10ms intervals, this is ~5 seconds. + + let mut attempts: u32 = 0; + loop { + let runs = db + .list_routine_runs(routine_id, 10) + .await + .expect("list_routine_runs should not fail"); + + if !runs.is_empty() { + return; + } + + attempts += 1; + if attempts >= max_attempts || start.elapsed() >= timeout { + panic!( + "Timeout waiting for routine run to be persisted (routine_id: {routine_id}, attempts: {attempts})" + ); + } + + tokio::time::sleep(poll_interval).await; + } +} \ No newline at end of file diff --git a/tests/support/routines.rs b/tests/support/routines.rs index e65a3730f..989b1ab40 100644 --- a/tests/support/routines.rs +++ b/tests/support/routines.rs @@ -174,71 +174,3 @@ pub async fn assert_system_event_count( .await; assert_eq!(fired, expected, "{msg}"); } - -/// Polls until the engine's running count reaches zero or the timeout expires. -/// -/// This provides deterministic synchronisation for tests that need to wait -/// for asynchronous routine execution to complete, eliminating timing-dependent -/// flakiness without slowing down the test suite on fast machines. -/// -/// **Note:** This helper should be combined with `wait_for_persisted_run` to ensure -/// both execution completion and database persistence, as the running count may -/// reach zero before the database record is fully committed. -#[allow(dead_code)] -pub async fn wait_for_idle(engine: &RoutineEngine, timeout: Duration) { - let start = std::time::Instant::now(); - let poll_interval = Duration::from_millis(10); - - loop { - let count = engine.running_count(); - if count == 0 { - return; - } - - if start.elapsed() >= timeout { - panic!( - "Timeout waiting for engine to become idle (running count: {})", - count - ); - } - - tokio::time::sleep(poll_interval).await; - } -} - -/// Polls until a routine run is persisted in the database or the timeout expires. -/// -/// This helper provides deterministic synchronisation for database persistence, -/// complementing `wait_for_idle` which only waits for in-memory execution completion. -/// Use this helper after `wait_for_idle` to ensure the routine run is durably recorded. -/// -/// # Arguments -/// * `db` - The database to query for persisted runs -/// * `routine_id` - The ID of the routine to check for runs -/// * `timeout` - Maximum duration to wait for persistence -#[allow(dead_code)] -pub async fn wait_for_persisted_run(db: &Arc, routine_id: Uuid, timeout: Duration) { - let start = std::time::Instant::now(); - let poll_interval = Duration::from_millis(10); - - loop { - let runs = db - .list_routine_runs(routine_id, 10) - .await - .expect("list_routine_runs should not fail"); - - if !runs.is_empty() { - return; - } - - if start.elapsed() >= timeout { - panic!( - "Timeout waiting for routine run to be persisted (routine_id: {}, elapsed: {:?})", - routine_id, - start.elapsed() - ); - } - - tokio::time::sleep(poll_interval).await; - } -}