mirror of
https://github.com/outbackdingo/optimclaw.git
synced 2026-08-27 08:00:17 +00:00
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) <[email protected]>
This commit is contained in:
+42
-10
@@ -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<i32>,
|
||||
},
|
||||
|
||||
#[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()
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user