From 3a27cd3561bc7d9ec5b617459339543774eb7de3 Mon Sep 17 00:00:00 2001 From: Zaki Date: Sun, 22 Mar 2026 18:32:56 -0700 Subject: [PATCH] fix(routines): classify permanent LLM failures, accumulate tokens, warn on tool retry (#1320) Address three review comments on the bounded-retry implementation: 1. Permanent LLM failures no longer retried: `is_retryable()` now inspects the `LlmFailed` reason string for known permanent patterns (auth, content policy, context length, model not available, moderation). These map to `LlmError` variants already classified as non-retryable by the LLM retry layer. 2. Token usage accumulated across retries: `RoutineError::LlmFailed` now carries an optional `partial_tokens` field populated from tokens consumed before the failure. The retry loop sums partial tokens from failed attempts with the final successful attempt's tokens. 3. Tool loop retry limitation documented: added a code comment explaining that the retry wraps the entire `execute_lightweight()` call, and a warning log when retrying a tools-enabled routine so operators know side effects may be repeated. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 (1M context) --- src/agent/routine_engine.rs | 98 ++++++++++++++++++++++++++++++++++--- src/error.rs | 52 ++++++++++++++++---- 2 files changed, 132 insertions(+), 18 deletions(-) diff --git a/src/agent/routine_engine.rs b/src/agent/routine_engine.rs index 4e13d4ed..646c3c71 100644 --- a/src/agent/routine_engine.rs +++ b/src/agent/routine_engine.rs @@ -1097,6 +1097,15 @@ async fn execute_routine(ctx: EngineContext, routine: Routine, run: RoutineRun) let result = { let mut attempt = 0u32; + let mut accumulated_tokens: i32 = 0; + let uses_tools = matches!( + routine.action, + RoutineAction::Lightweight { + use_tools: true, + .. + } + ) && ctx.config.lightweight_tools_enabled; + loop { let execution_result = match &routine.action { RoutineAction::Lightweight { @@ -1132,9 +1141,39 @@ async fn execute_routine(ctx: EngineContext, routine: Routine, run: RoutineRun) }; match execution_result { - Ok(outcome) => break Ok(outcome), + Ok((status, summary, tokens)) => { + // Accumulate tokens from this attempt with any previous attempts + let total = Some(accumulated_tokens + tokens.unwrap_or(0)); + break Ok((status, summary, total)); + } Err(ref e) if is_lightweight && 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; + } + 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)), ); @@ -1547,6 +1586,7 @@ async fn execute_lightweight_no_tools( .await .map_err(|e| RoutineError::LlmFailed { reason: e.to_string(), + partial_tokens: None, })?; handle_text_response( @@ -1651,13 +1691,13 @@ async fn execute_lightweight_with_tools( .with_max_tokens(effective_max_tokens) .with_temperature(0.3); - let response = - ctx.llm - .complete(request) - .await - .map_err(|e| RoutineError::LlmFailed { - reason: e.to_string(), - })?; + let response = ctx.llm.complete(request).await.map_err(|e| { + let partial = (total_input_tokens + total_output_tokens) as i32; + RoutineError::LlmFailed { + reason: e.to_string(), + partial_tokens: if partial > 0 { Some(partial) } else { None }, + } + })?; total_input_tokens += response.input_tokens; total_output_tokens += response.output_tokens; @@ -1684,8 +1724,10 @@ async fn execute_lightweight_with_tools( .with_temperature(0.3); let response = ctx.llm.complete_with_tools(request).await.map_err(|e| { + let partial = (total_input_tokens + total_output_tokens) as i32; RoutineError::LlmFailed { reason: e.to_string(), + partial_tokens: if partial > 0 { Some(partial) } else { None }, } })?; @@ -2554,6 +2596,11 @@ mod tests { let transient_errors: Vec = vec![ RoutineError::LlmFailed { reason: "rate limit".into(), + partial_tokens: None, + }, + RoutineError::LlmFailed { + reason: "network timeout".into(), + partial_tokens: Some(42), }, RoutineError::EmptyResponse, RoutineError::TruncatedResponse, @@ -2562,6 +2609,41 @@ mod tests { assert!(err.is_retryable(), "{} should be retryable", err); } + // Permanent LLM failures that should NOT be retried + let permanent_llm_errors: Vec = vec![ + RoutineError::LlmFailed { + reason: "Authentication failed for provider openai".into(), + partial_tokens: None, + }, + RoutineError::LlmFailed { + reason: "invalid_api_key: bad key".into(), + partial_tokens: None, + }, + RoutineError::LlmFailed { + reason: "content policy violation".into(), + partial_tokens: None, + }, + RoutineError::LlmFailed { + reason: "content_filter triggered".into(), + partial_tokens: None, + }, + RoutineError::LlmFailed { + reason: "context length exceeded: 150000 tokens used, 128000 allowed".into(), + partial_tokens: Some(100), + }, + RoutineError::LlmFailed { + reason: "model not available on provider anthropic".into(), + partial_tokens: None, + }, + RoutineError::LlmFailed { + reason: "content moderation flagged".into(), + partial_tokens: None, + }, + ]; + for err in &permanent_llm_errors { + assert!(!err.is_retryable(), "{} should NOT be retryable", err); + } + // Hard failures (never retried) let hard_errors: Vec = vec![ RoutineError::Disabled { diff --git a/src/error.rs b/src/error.rs index d03b0332..a07a4325 100644 --- a/src/error.rs +++ b/src/error.rs @@ -395,7 +395,12 @@ pub enum RoutineError { Database { reason: String }, #[error("LLM call failed: {reason}")] - LlmFailed { reason: String }, + LlmFailed { + reason: String, + /// Partial token count consumed before the failure (if any). + /// Used to accumulate usage across retry attempts. + partial_tokens: Option, + }, #[error("Failed to dispatch full job: {reason}")] JobDispatchFailed { reason: String }, @@ -408,17 +413,43 @@ 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, empty responses, truncated responses. - /// Non-retryable: configuration errors, authorization, resource limits, DB errors. + /// Retryable: LLM failures (unless the reason matches a known permanent + /// pattern), empty responses, 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 { - matches!( - self, - RoutineError::LlmFailed { .. } - | RoutineError::EmptyResponse - | RoutineError::TruncatedResponse - ) + match self { + RoutineError::LlmFailed { reason, .. } => { + let lower = reason.to_ascii_lowercase(); + !Self::PERMANENT_LLM_PATTERNS + .iter() + .any(|pat| lower.contains(pat)) + } + RoutineError::EmptyResponse | RoutineError::TruncatedResponse => true, + _ => false, + } } } @@ -534,7 +565,8 @@ mod tests { // Transient errors should be retryable assert!( RoutineError::LlmFailed { - reason: "timeout".into() + reason: "timeout".into(), + partial_tokens: None, } .is_retryable() );