From eba088f30e5cc02bd73e8c3a478a26aceecfd28d Mon Sep 17 00:00:00 2001 From: Zaki Date: Tue, 24 Mar 2026 10:41:48 -0700 Subject: [PATCH] fix(routines): address PR review feedback for bounded retry (#1320) - Skip retry for tools-enabled routines to prevent duplicate side effects - Replace fragile PERMANENT_LLM_PATTERNS substring matching with a retryable bool field on RoutineError::LlmFailed, set at the LlmError conversion site using llm::retry::is_retryable() - Use saturating_add for token accumulation Co-Authored-By: Claude Opus 4.6 (1M context) --- src/agent/routine_engine.rs | 46 +++++++++++++++++++----------------- src/error.rs | 47 +++++++++++++++---------------------- 2 files changed, 43 insertions(+), 50 deletions(-) diff --git a/src/agent/routine_engine.rs b/src/agent/routine_engine.rs index 188ef2e9..41a6c86b 100644 --- a/src/agent/routine_engine.rs +++ b/src/agent/routine_engine.rs @@ -1143,37 +1143,21 @@ async fn execute_routine(ctx: EngineContext, routine: Routine, run: RoutineRun) match execution_result { Ok((status, summary, tokens)) => { // Accumulate tokens from this attempt with any previous attempts - let total = Some(accumulated_tokens + tokens.unwrap_or(0)); + let total = Some(accumulated_tokens.saturating_add(tokens.unwrap_or(0))); break Ok((status, summary, total)); } - Err(ref e) if is_lightweight && e.is_retryable() && attempt < MAX_RETRIES => { + Err(ref e) if is_lightweight && !uses_tools && e.is_retryable() && attempt < MAX_RETRIES => { // Accumulate any partial tokens from the failed attempt if let RoutineError::LlmFailed { partial_tokens: Some(t), .. } = e { - accumulated_tokens += t; + accumulated_tokens = accumulated_tokens.saturating_add(*t); } attempt += 1; - // NOTE: When retrying a tools-enabled routine, the entire - // execute_lightweight() call is re-run, which means - // already-executed tool calls will be repeated. This is a - // known limitation — restructuring the retry to wrap only - // the LLM call would require splitting the tool loop from - // the LLM call, which is too invasive for the current - // architecture. - if uses_tools { - tracing::warn!( - routine = %routine.name, - attempt = attempt, - "Retrying tools-enabled routine; previously executed tool calls \ - may be repeated causing duplicate side effects" - ); - } - let delay = Duration::from_millis( BASE_DELAY_MS.saturating_mul(2u64.saturating_pow(attempt - 1)), ); @@ -1578,9 +1562,13 @@ async fn execute_lightweight_no_tools( .llm .complete(request) .await - .map_err(|e| RoutineError::LlmFailed { - reason: e.to_string(), - partial_tokens: None, + .map_err(|e| { + let retryable = crate::llm::retry::is_retryable(&e); + RoutineError::LlmFailed { + reason: e.to_string(), + partial_tokens: None, + retryable, + } })?; handle_text_response( @@ -1687,9 +1675,11 @@ async fn execute_lightweight_with_tools( let response = ctx.llm.complete(request).await.map_err(|e| { let partial = (total_input_tokens + total_output_tokens) as i32; + let retryable = crate::llm::retry::is_retryable(&e); RoutineError::LlmFailed { reason: e.to_string(), partial_tokens: if partial > 0 { Some(partial) } else { None }, + retryable, } })?; @@ -1719,9 +1709,11 @@ async fn execute_lightweight_with_tools( let response = ctx.llm.complete_with_tools(request).await.map_err(|e| { let partial = (total_input_tokens + total_output_tokens) as i32; + let retryable = crate::llm::retry::is_retryable(&e); RoutineError::LlmFailed { reason: e.to_string(), partial_tokens: if partial > 0 { Some(partial) } else { None }, + retryable, } })?; @@ -2591,10 +2583,12 @@ mod tests { RoutineError::LlmFailed { reason: "rate limit".into(), partial_tokens: None, + retryable: true, }, RoutineError::LlmFailed { reason: "network timeout".into(), partial_tokens: Some(42), + retryable: true, }, RoutineError::EmptyResponse, RoutineError::TruncatedResponse, @@ -2604,34 +2598,42 @@ mod tests { } // Permanent LLM failures that should NOT be retried + // (retryable: false is set at conversion time by llm::retry::is_retryable) let permanent_llm_errors: Vec = vec![ RoutineError::LlmFailed { reason: "Authentication failed for provider openai".into(), partial_tokens: None, + retryable: false, }, RoutineError::LlmFailed { reason: "invalid_api_key: bad key".into(), partial_tokens: None, + retryable: false, }, RoutineError::LlmFailed { reason: "content policy violation".into(), partial_tokens: None, + retryable: false, }, RoutineError::LlmFailed { reason: "content_filter triggered".into(), partial_tokens: None, + retryable: false, }, RoutineError::LlmFailed { reason: "context length exceeded: 150000 tokens used, 128000 allowed".into(), partial_tokens: Some(100), + retryable: false, }, RoutineError::LlmFailed { reason: "model not available on provider anthropic".into(), partial_tokens: None, + retryable: false, }, RoutineError::LlmFailed { reason: "content moderation flagged".into(), partial_tokens: None, + retryable: false, }, ]; for err in &permanent_llm_errors { diff --git a/src/error.rs b/src/error.rs index a07a4325..5da9068f 100644 --- a/src/error.rs +++ b/src/error.rs @@ -400,6 +400,11 @@ pub enum RoutineError { /// Partial token count consumed before the failure (if any). /// Used to accumulate usage across retry attempts. partial_tokens: Option, + /// Whether the underlying LLM error was classified as retryable. + /// Set at the `LlmError` → `RoutineError` conversion site using + /// `crate::llm::retry::is_retryable()`, avoiding fragile substring + /// matching on the stringified reason. + retryable: bool, }, #[error("Failed to dispatch full job: {reason}")] @@ -413,40 +418,16 @@ pub enum RoutineError { } impl RoutineError { - /// Known permanent failure substrings in LLM error reasons. - /// - /// These map to `LlmError` variants that the LLM retry layer already - /// considers non-retryable (`AuthFailed`, `ContextLengthExceeded`, - /// `ModelNotAvailable`) plus content-policy rejections surfaced as - /// provider-level `RequestFailed` with descriptive messages. - const PERMANENT_LLM_PATTERNS: &'static [&'static str] = &[ - "auth", - "authentication", - "invalid_api_key", - "content policy", - "content_policy", - "content_filter", - "context length", - "context_length", - "model not available", - "model_not_available", - "moderation", - ]; - /// Whether this error is transient and worth retrying with backoff. /// - /// Retryable: LLM failures (unless the reason matches a known permanent - /// pattern), empty responses, truncated responses. + /// Retryable: LLM failures where the underlying `LlmError` was classified + /// as retryable by `crate::llm::retry::is_retryable()`, empty responses, + /// and truncated responses. /// Non-retryable: configuration errors, authorization, resource limits, /// DB errors, and LLM failures caused by auth/content-policy/context-length. pub fn is_retryable(&self) -> bool { match self { - RoutineError::LlmFailed { reason, .. } => { - let lower = reason.to_ascii_lowercase(); - !Self::PERMANENT_LLM_PATTERNS - .iter() - .any(|pat| lower.contains(pat)) - } + RoutineError::LlmFailed { retryable, .. } => *retryable, RoutineError::EmptyResponse | RoutineError::TruncatedResponse => true, _ => false, } @@ -567,6 +548,16 @@ mod tests { RoutineError::LlmFailed { reason: "timeout".into(), partial_tokens: None, + retryable: true, + } + .is_retryable() + ); + // Non-retryable LLM error + assert!( + !RoutineError::LlmFailed { + reason: "timeout".into(), + partial_tokens: None, + retryable: false, } .is_retryable() );