From 841cf5fe76a7f8b65a6ec177ff900aa8583fa3b1 Mon Sep 17 00:00:00 2001 From: Zaki Date: Fri, 20 Mar 2026 10:37:34 -0700 Subject: [PATCH] fix(routines): add bounded retry for transient lightweight execution failures (#1320) Lightweight routine executions that fail with transient errors (LLM failures, empty responses, truncated responses) are now retried up to 3 times with exponential backoff (1s, 2s, 4s) before reporting failure. Full-job routines are not retried since the scheduler/watcher handles their lifecycle. Hard failures (disabled, not found, auth, DB errors) fail immediately without retry. Adds RoutineError::is_retryable() to classify transient vs hard errors, with comprehensive regression tests covering all error variants. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/agent/routine_engine.rs | 133 ++++++++++++++++++++++++++++-------- src/error.rs | 75 ++++++++++++++++++++ 2 files changed, 179 insertions(+), 29 deletions(-) diff --git a/src/agent/routine_engine.rs b/src/agent/routine_engine.rs index 64c3b94c..4e13d4ed 100644 --- a/src/agent/routine_engine.rs +++ b/src/agent/routine_engine.rs @@ -1089,36 +1089,66 @@ async fn execute_routine(ctx: EngineContext, routine: Routine, run: RoutineRun) // Increment running count (atomic: survives panics in the execution below) ctx.running_count.fetch_add(1, Ordering::Relaxed); - let result = match &routine.action { - RoutineAction::Lightweight { - prompt, - context_paths, - max_tokens, - use_tools, - max_tool_rounds, - } => { - execute_lightweight( - &ctx, - &routine, - prompt, - context_paths, - *max_tokens, - *use_tools, - *max_tool_rounds, - ) - .await - } - RoutineAction::FullJob { - title, - description, - max_iterations, - } => { - let execution = FullJobExecutionConfig { - title, - description, - max_iterations: *max_iterations, + // Retry constants for transient lightweight execution failures. + const MAX_RETRIES: u32 = 3; + const BASE_DELAY_MS: u64 = 1000; + + let is_lightweight = matches!(routine.action, RoutineAction::Lightweight { .. }); + + let result = { + let mut attempt = 0u32; + loop { + let execution_result = match &routine.action { + RoutineAction::Lightweight { + prompt, + context_paths, + max_tokens, + use_tools, + max_tool_rounds, + } => { + execute_lightweight( + &ctx, + &routine, + prompt, + context_paths, + *max_tokens, + *use_tools, + *max_tool_rounds, + ) + .await + } + RoutineAction::FullJob { + title, + description, + max_iterations, + } => { + let execution = FullJobExecutionConfig { + title, + description, + max_iterations: *max_iterations, + }; + execute_full_job(&ctx, &routine, &run, &execution).await + } }; - execute_full_job(&ctx, &routine, &run, &execution).await + + match execution_result { + Ok(outcome) => break Ok(outcome), + Err(ref e) if is_lightweight && e.is_retryable() && attempt < MAX_RETRIES => { + attempt += 1; + let delay = Duration::from_millis( + BASE_DELAY_MS.saturating_mul(2u64.saturating_pow(attempt - 1)), + ); + tracing::warn!( + routine = %routine.name, + attempt = attempt, + max_retries = MAX_RETRIES, + delay_ms = delay.as_millis() as u64, + "Transient routine error, retrying: {}", e + ); + tokio::time::sleep(delay).await; + } + Err(e) => break Err(e), + } } }; @@ -1828,6 +1858,7 @@ async fn execute_routine_tool( } /// Send a notification based on the routine's notify config and run status. +#[allow(clippy::too_many_arguments)] async fn send_notification( tx: &mpsc::Sender, notify: &NotifyConfig, @@ -2513,6 +2544,50 @@ mod tests { } } + /// Regression test for #1320: transient errors are retried for lightweight + /// routines but not for full-job routines or hard failures. + #[test] + fn test_retry_classification_for_routine_errors() { + use crate::error::RoutineError; + + // Transient errors (retryable for lightweight routines) + let transient_errors: Vec = vec![ + RoutineError::LlmFailed { + reason: "rate limit".into(), + }, + RoutineError::EmptyResponse, + RoutineError::TruncatedResponse, + ]; + for err in &transient_errors { + assert!(err.is_retryable(), "{} should be retryable", err); + } + + // Hard failures (never retried) + let hard_errors: Vec = vec![ + RoutineError::Disabled { + name: "test".into(), + }, + RoutineError::NotFound { + id: uuid::Uuid::new_v4(), + }, + RoutineError::NotAuthorized { + id: uuid::Uuid::new_v4(), + }, + RoutineError::MaxConcurrent { + name: "test".into(), + }, + RoutineError::JobDispatchFailed { + reason: "no docker".into(), + }, + RoutineError::Database { + reason: "connection refused".into(), + }, + ]; + for err in &hard_errors { + assert!(!err.is_retryable(), "{} should NOT be retryable", err); + } + } + #[test] fn test_sanitize_summary_strips_control_chars() { use super::sanitize_summary; diff --git a/src/error.rs b/src/error.rs index e4f1b957..b1c45fdf 100644 --- a/src/error.rs +++ b/src/error.rs @@ -407,6 +407,21 @@ pub enum RoutineError { TruncatedResponse, } +impl RoutineError { + /// Whether this error is transient and worth retrying with backoff. + /// + /// Retryable: LLM failures, empty responses, truncated responses. + /// Non-retryable: configuration errors, authorization, resource limits, DB errors. + pub fn is_retryable(&self) -> bool { + matches!( + self, + RoutineError::LlmFailed { .. } + | RoutineError::EmptyResponse + | RoutineError::TruncatedResponse + ) + } +} + /// Result type alias for the agent. pub type Result = std::result::Result; @@ -514,6 +529,66 @@ mod tests { assert!(msg.contains("bad format"), "Should mention reason: {msg}"); } + #[test] + fn routine_error_retryable_classification() { + // Transient errors should be retryable + assert!( + RoutineError::LlmFailed { + reason: "timeout".into() + } + .is_retryable() + ); + assert!(RoutineError::EmptyResponse.is_retryable()); + assert!(RoutineError::TruncatedResponse.is_retryable()); + + // Hard failures should NOT be retryable + assert!(!RoutineError::Disabled { + name: "test".into() + } + .is_retryable()); + assert!(!RoutineError::JobDispatchFailed { + reason: "no docker".into() + } + .is_retryable()); + assert!(!RoutineError::Database { + reason: "conn refused".into() + } + .is_retryable()); + assert!(!RoutineError::NotFound { + id: Uuid::new_v4() + } + .is_retryable()); + assert!(!RoutineError::NotAuthorized { + id: Uuid::new_v4() + } + .is_retryable()); + assert!(!RoutineError::MaxConcurrent { + name: "test".into() + } + .is_retryable()); + assert!(!RoutineError::UnknownTriggerType { + trigger_type: "x".into() + } + .is_retryable()); + assert!(!RoutineError::UnknownActionType { + action_type: "x".into() + } + .is_retryable()); + assert!(!RoutineError::MissingField { + context: "c".into(), + field: "f".into() + } + .is_retryable()); + assert!(!RoutineError::InvalidCron { + reason: "bad".into() + } + .is_retryable()); + assert!(!RoutineError::UnknownRunStatus { + status: "bad".into() + } + .is_retryable()); + } + #[test] fn top_level_error_from_conversions() { let config_err = ConfigError::MissingEnvVar("TEST".to_string());