mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-25 14:53:34 +00:00
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) <[email protected]>
This commit is contained in:
+104
-29
@@ -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<OutgoingResponse>,
|
||||
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<RoutineError> = 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<RoutineError> = 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;
|
||||
|
||||
@@ -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<T> = std::result::Result<T, Error>;
|
||||
|
||||
@@ -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());
|
||||
|
||||
Reference in New Issue
Block a user