gitops/optimclaw
1
0
Fork 0
mirror of https://github.com/outbackdingo/optimclaw.git synced 2026-08-31 08:39:24 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: staging CI triage — consolidate retry parsing, fix flaky tests, add docs (#1427)

Browse Source 
* fix: consolidate retry-after parsing and fix flaky OAuth env tests (#1288, #1280)

- Extract shared `parse_retry_after()` into `src/llm/retry.rs` supporting
  both delay-seconds and RFC2822 formats, replacing duplicated inline parsing
  in anthropic_oauth.rs, nearai_chat.rs, and embeddings.rs
- Fix flaky `bind_rejects_wildcard_*` tests in oauth_helpers.rs by adding
  `tokio::sync::Mutex` to serialize env var access (matching the ENV_MUTEX
  pattern in oauth_defaults.rs)
- Add regression tests for parse_retry_after edge cases

Closes #1288, #1280

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* docs: add comments explaining CLI_ENABLED=false in service templates (#990)

Clarify that CLI_ENABLED=false is needed in daemon mode (launchd/systemd)
to prevent blocking on stdin when running as a background service.

Closes #990

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: address review comments on retry-after consolidation

- Change parse_retry_after() return type from Option<Duration> to Duration
  (it never returns None due to the 60s fallback)
- Fix doc comment: reference RFC 7231 §7.1.1 for HTTP-date, not RFC 2822
- Add parse_retry_after_http_date test for the RFC 2822 date parsing branch
- Remove stale per-file test helpers (parse_retry_after_*_for_test) that
  duplicated old inline logic instead of testing the shared function
- Remove unnecessary comments above #[cfg(test)] imports
- Use crate-wide ENV_MUTEX instead of local tokio::sync::Mutex in
  oauth_helpers tests to prevent cross-module env-var races

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

* fix: reword await_holding_lock safety comment

Drop runtime-flavor assumption; justify by short-lived awaited operation.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]>
This commit is contained in:
Henry Park
2026-03-19 18:33:15 -07:00
committed by GitHub
co-authored by Claude Opus 4.6
parent 6b0f84bbe0
commit 8920322589
6 changed files with 201 additions and 302 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/llm/anthropic_oauth.rs
+3 -91
View File
@@ -22,8 +22,6 @@ use crate::llm::provider::{
ToolCompletionRequest, ToolCompletionResponse, strip_unsupported_completion_params,
strip_unsupported_tool_params,
};
use crate::llm::retry::cap_retry_after;
const ANTHROPIC_API_URL: &str = "https://api.anthropic.com/v1/messages";
/// OAuth beta requires 2023-06-01; the 2024-10-22 version is not valid with the beta flag.
const ANTHROPIC_API_VERSION: &str = "2023-06-01";
@@ -144,15 +142,9 @@ impl AnthropicOAuthProvider {
if !status.is_success() {
// Parse Retry-After header before consuming the body.
// Falls back to 60s if header is missing or unparseable (prevents "retry after None" errors).
let retry_after = response
.headers()
.get("retry-after")
.and_then(|v| v.to_str().ok())
.and_then(|v| v.parse::<u64>().ok())
.map(std::time::Duration::from_secs)
.map(cap_retry_after)
.or(Some(std::time::Duration::from_secs(60)));
let retry_after = Some(crate::llm::retry::parse_retry_after(
response.headers().get("retry-after"),
));
let response_text = response
.text()
@@ -709,84 +701,4 @@ mod tests {
// Subsequent reads see the updated token
assert_eq!(token.read().unwrap().expose_secret(), "new_token");
}
// -- Retry-After header parsing tests (regression for rate limit "None" bug) --
#[test]
fn test_retry_after_parsing_delay_seconds() {
// Verify delay-seconds format is parsed correctly
let header_value = "45";
let duration = parse_retry_after_anthropic_for_test(header_value);
assert_eq!(
duration,
Some(std::time::Duration::from_secs(45)),
"Should parse delay-seconds format"
);
}
#[test]
fn test_retry_after_fallback_missing_header() {
// Regression test: When Retry-After header is missing,
// should fall back to 60s instead of None
let duration = parse_retry_after_anthropic_for_test("");
assert_eq!(
duration,
Some(std::time::Duration::from_secs(60)),
"Missing header should fallback to 60s"
);
}
#[test]
fn test_retry_after_fallback_invalid_format() {
// Regression test: When Retry-After header is in unexpected format,
// should fall back to 60s instead of None
let invalid_formats = vec![
"invalid",
"not-a-number",
"30.5", // float instead of int
"abc123",
"Mon, 02 Mar 2026 18:00:00 GMT", // RFC2822 not supported in anthropic version
];
for format in invalid_formats {
let duration = parse_retry_after_anthropic_for_test(format);
assert_eq!(
duration,
Some(std::time::Duration::from_secs(60)),
"Invalid format '{}' should fallback to 60s",
format
);
}
}
#[test]
fn test_retry_after_zero_seconds_accepted() {
// Verify zero seconds is a valid retry delay
let duration = parse_retry_after_anthropic_for_test("0");
assert_eq!(duration, Some(std::time::Duration::ZERO));
}
#[test]
fn test_retry_after_large_number() {
// Verify large numbers are capped to the safe maximum
let duration = parse_retry_after_anthropic_for_test("7200"); // 2 hours
assert_eq!(
duration,
Some(std::time::Duration::from_secs(
crate::llm::retry::MAX_RETRY_AFTER_SECS
))
);
}
/// Helper function to test Retry-After header parsing logic for Anthropic
/// (simulates the parsing done in send_request without actual HTTP, including fallback)
fn parse_retry_after_anthropic_for_test(header_value: &str) -> Option<std::time::Duration> {
header_value
.trim()
.parse::<u64>()
.ok()
.map(std::time::Duration::from_secs)
.map(cap_retry_after)
.or(Some(std::time::Duration::from_secs(60)))
}
}
src/llm/nearai_chat.rs
+4 -144
View File
@@ -22,7 +22,7 @@ use crate::llm::provider::{
ChatMessage, CompletionRequest, CompletionResponse, FinishReason, LlmProvider, Role, ToolCall,
ToolCompletionRequest, ToolCompletionResponse,
};
use crate::llm::{costs, retry::cap_retry_after, session::SessionManager};
use crate::llm::{costs, session::SessionManager};
/// Information about an available model from NEAR AI API.
#[derive(Debug, Clone, Serialize, Deserialize)]
@@ -243,30 +243,9 @@ impl NearAiChatProvider {
let status = response.status();
// Extract Retry-After header before consuming the response body.
// Supports both delay-seconds (RFC 7231 §7.1.3) and HTTP-date formats.
// Falls back to 60s if header is missing or unparseable (prevents "retry after None" errors).
let retry_after_header = response
.headers()
.get("retry-after")
.and_then(|v| v.to_str().ok())
.and_then(|v| {
// Try delay-seconds first (most common from API providers)
if let Ok(secs) = v.trim().parse::<u64>() {
return Some(cap_retry_after(std::time::Duration::from_secs(secs)));
}
// Try HTTP-date (e.g. "Mon, 02 Mar 2026 18:00:00 GMT")
if let Ok(dt) = chrono::DateTime::parse_from_rfc2822(v.trim()) {
let now = chrono::Utc::now();
let delta = dt.signed_duration_since(now);
// Use max(0) so past/present dates yield Duration::ZERO
// rather than None (which would cause an immediate retry).
return Some(cap_retry_after(std::time::Duration::from_secs(
delta.num_seconds().max(0) as u64,
)));
}
None
})
.or(Some(std::time::Duration::from_secs(60)));
let retry_after_header = Some(crate::llm::retry::parse_retry_after(
response.headers().get("retry-after"),
));
let response_text = response.text().await.map_err(|e| LlmError::RequestFailed {
provider: "nearai_chat".to_string(),
reason: format!("Failed to read response body: {}", e),
@@ -2218,123 +2197,4 @@ mod tests {
"http://example.com/api/proxy/v1/chat/completions"
);
}
// -- Retry-After header parsing tests (regression for rate limit "None" bug) --
#[test]
fn test_retry_after_parsing_delay_seconds() {
// Verify delay-seconds format (most common) is parsed correctly
let header_value = "30";
let duration = parse_retry_after_for_test(header_value);
assert_eq!(duration, Some(std::time::Duration::from_secs(30)));
}
#[test]
fn test_retry_after_parsing_rfc2822_date() {
// Verify HTTP-date (RFC 2822) format is parsed correctly
// Use a date 60 seconds in the future
let now = chrono::Utc::now();
let future = now + chrono::Duration::seconds(60);
let date_str = future.to_rfc2822();
let duration = parse_retry_after_for_test(&date_str);
assert!(duration.is_some());
let d = duration.unwrap();
// Allow ±5 seconds of drift due to processing time
assert!(
d.as_secs() >= 55 && d.as_secs() <= 65,
"Expected ~60s, got {}s",
d.as_secs()
);
}
#[test]
fn test_retry_after_fallback_missing_header() {
// Regression test: When Retry-After header is missing,
// should fall back to 60s instead of None
let duration = parse_retry_after_for_test("");
assert_eq!(
duration,
Some(std::time::Duration::from_secs(60)),
"Missing header should fallback to 60s"
);
}
#[test]
fn test_retry_after_fallback_invalid_format() {
// Regression test: When Retry-After header is in unexpected format,
// should fall back to 60s instead of None
let invalid_formats = vec![
"invalid",
"not-a-number",
"30.5", // float instead of int
"abc123",
];
for format in invalid_formats {
let duration = parse_retry_after_for_test(format);
assert_eq!(
duration,
Some(std::time::Duration::from_secs(60)),
"Invalid format '{}' should fallback to 60s",
format
);
}
}
#[test]
fn test_retry_after_past_date_returns_zero() {
// When HTTP-date is in the past, should return Duration::ZERO
// (not None, which would trigger immediate retry)
let past = chrono::Utc::now() - chrono::Duration::seconds(60);
let past_date_str = past.to_rfc2822();
let duration = parse_retry_after_for_test(&past_date_str);
assert_eq!(
duration,
Some(std::time::Duration::ZERO),
"Past date should return Duration::ZERO, not None"
);
}
#[test]
fn test_retry_after_zero_seconds_accepted() {
// Verify zero seconds is a valid retry delay
let duration = parse_retry_after_for_test("0");
assert_eq!(duration, Some(std::time::Duration::ZERO));
}
#[test]
fn test_retry_after_large_number() {
// Verify large numbers are capped to the safe maximum
let duration = parse_retry_after_for_test("3600"); // 1 hour
assert_eq!(duration, Some(std::time::Duration::from_secs(3600)));
let huge = parse_retry_after_for_test("18446744073709551615");
assert_eq!(
huge,
Some(std::time::Duration::from_secs(
crate::llm::retry::MAX_RETRY_AFTER_SECS
))
);
}
/// Helper function to test Retry-After header parsing logic
/// (simulates the parsing done in send_request without actual HTTP, including fallback)
fn parse_retry_after_for_test(header_value: &str) -> Option<std::time::Duration> {
let trimmed = header_value.trim();
let parsed = if let Ok(secs) = trimmed.parse::<u64>() {
Some(cap_retry_after(std::time::Duration::from_secs(secs)))
} else if let Ok(dt) = chrono::DateTime::parse_from_rfc2822(trimmed) {
let now = chrono::Utc::now();
let delta = dt.signed_duration_since(now);
Some(cap_retry_after(std::time::Duration::from_secs(
delta.num_seconds().max(0) as u64,
)))
} else {
None
};
// Apply fallback to 60s if parsing failed (matches actual code behavior)
parsed.or(Some(std::time::Duration::from_secs(60)))
}
}
src/llm/oauth_helpers.rs
+25 -4
View File
@@ -361,6 +361,7 @@ pub fn landing_html(provider_name: &str, success: bool) -> String {
#[cfg(test)]
mod tests {
use super::*;
use crate::config::helpers::ENV_MUTEX;
#[test]
fn loopback_detection() {
@@ -385,12 +386,22 @@ mod tests {
assert!(!is_wildcard_host("localhost"));
}
// Lock held across await to serialize env-var mutation; the awaited op is a quick local TCP bind.
#[allow(clippy::await_holding_lock)]
#[tokio::test]
async fn bind_rejects_wildcard_ipv4() {
// SAFETY: test is single-threaded; env var is restored immediately after.
let _guard = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner());
let original = std::env::var("OAUTH_CALLBACK_HOST").ok();
// SAFETY: Under ENV_MUTEX, no concurrent env access.
unsafe { std::env::set_var("OAUTH_CALLBACK_HOST", "0.0.0.0") };
let result = bind_callback_listener().await;
unsafe { std::env::remove_var("OAUTH_CALLBACK_HOST") };
// SAFETY: Under ENV_MUTEX, no concurrent env access.
unsafe {
match &original {
Some(v) => std::env::set_var("OAUTH_CALLBACK_HOST", v),
None => std::env::remove_var("OAUTH_CALLBACK_HOST"),
}
}
assert!(result.is_err());
let err = result.unwrap_err().to_string();
assert!(
@@ -399,12 +410,22 @@ mod tests {
);
}
// Lock held across await to serialize env-var mutation; the awaited op is a quick local TCP bind.
#[allow(clippy::await_holding_lock)]
#[tokio::test]
async fn bind_rejects_wildcard_ipv6() {
// SAFETY: test is single-threaded; env var is restored immediately after.
let _guard = ENV_MUTEX.lock().unwrap_or_else(|e| e.into_inner());
let original = std::env::var("OAUTH_CALLBACK_HOST").ok();
// SAFETY: Under ENV_MUTEX, no concurrent env access.
unsafe { std::env::set_var("OAUTH_CALLBACK_HOST", "::") };
let result = bind_callback_listener().await;
unsafe { std::env::remove_var("OAUTH_CALLBACK_HOST") };
// SAFETY: Under ENV_MUTEX, no concurrent env access.
unsafe {
match &original {
Some(v) => std::env::set_var("OAUTH_CALLBACK_HOST", v),
None => std::env::remove_var("OAUTH_CALLBACK_HOST"),
}
}
assert!(result.is_err());
let err = result.unwrap_err().to_string();
assert!(
src/llm/retry.rs
+76
View File
@@ -78,6 +78,33 @@ pub(crate) fn cap_retry_after(duration: Duration) -> Duration {
duration.min(Duration::from_secs(MAX_RETRY_AFTER_SECS))
}
/// Parse a `Retry-After` header value into a capped `Duration`.
///
/// Supports both delay-seconds (RFC 7231 §7.1.3) and HTTP-date formats (RFC 7231
/// §7.1.1 / IMF-fixdate). The implementation uses `chrono::DateTime::parse_from_rfc2822`,
/// which also accepts RFC 2822-style dates.
/// Returns `DEFAULT_RETRY_AFTER` (60 s) if the header is missing or unparseable.
pub(crate) fn parse_retry_after(header: Option<&reqwest::header::HeaderValue>) -> Duration {
header
.and_then(|v| v.to_str().ok())
.and_then(|v| {
if let Ok(secs) = v.trim().parse::<u64>() {
return Some(cap_retry_after(Duration::from_secs(secs)));
}
if let Ok(dt) = chrono::DateTime::parse_from_rfc2822(v.trim()) {
let now = chrono::Utc::now();
let delta = dt.signed_duration_since(now);
return Some(cap_retry_after(Duration::from_secs(
delta.num_seconds().max(0) as u64,
)));
}
None
})
.unwrap_or(Duration::from_secs(DEFAULT_RETRY_AFTER_SECS))
}
const DEFAULT_RETRY_AFTER_SECS: u64 = 60;
/// Configuration for the retry decorator.
#[derive(Debug, Clone)]
pub struct RetryConfig {
@@ -444,4 +471,53 @@ mod tests {
Duration::from_secs(0)
);
}
#[test]
fn parse_retry_after_delay_seconds() {
let val = reqwest::header::HeaderValue::from_static("30");
assert_eq!(parse_retry_after(Some(&val)), Duration::from_secs(30));
}
#[test]
fn parse_retry_after_missing_header() {
assert_eq!(
parse_retry_after(None),
Duration::from_secs(DEFAULT_RETRY_AFTER_SECS)
);
}
#[test]
fn parse_retry_after_unparseable() {
let val = reqwest::header::HeaderValue::from_static("not-a-number");
assert_eq!(
parse_retry_after(Some(&val)),
Duration::from_secs(DEFAULT_RETRY_AFTER_SECS)
);
}
#[test]
fn parse_retry_after_clamps_large_value() {
let val = reqwest::header::HeaderValue::from_static("999999");
assert_eq!(
parse_retry_after(Some(&val)),
Duration::from_secs(MAX_RETRY_AFTER_SECS)
);
}
#[test]
fn parse_retry_after_http_date() {
let future = chrono::Utc::now() + chrono::Duration::seconds(30);
let date_str = future.to_rfc2822();
let val = reqwest::header::HeaderValue::from_str(&date_str).unwrap();
let parsed = parse_retry_after(Some(&val));
let diff = if parsed > Duration::from_secs(30) {
parsed - Duration::from_secs(30)
} else {
Duration::from_secs(30) - parsed
};
assert!(
diff <= Duration::from_secs(2),
"expected ~30s, got {parsed:?} (diff {diff:?}) from header {date_str:?}"
);
}
}
src/workspace/embeddings.rs
+6 -63
View File
@@ -6,8 +6,6 @@
use async_trait::async_trait;
use serde::{Deserialize, Serialize};
use crate::llm::retry::cap_retry_after;
/// Error type for embedding operations.
#[derive(Debug, thiserror::Error)]
pub enum EmbeddingError {
@@ -228,14 +226,9 @@ impl EmbeddingProvider for OpenAiEmbeddings {
}
if status == reqwest::StatusCode::TOO_MANY_REQUESTS {
let retry_after = response
.headers()
.get("retry-after")
.and_then(|v| v.to_str().ok())
.and_then(|s| s.parse::<u64>().ok())
.map(std::time::Duration::from_secs)
.map(cap_retry_after)
.or(Some(std::time::Duration::from_secs(60)));
let retry_after = Some(crate::llm::retry::parse_retry_after(
response.headers().get("retry-after"),
));
return Err(EmbeddingError::RateLimited { retry_after });
}
@@ -371,14 +364,9 @@ impl EmbeddingProvider for NearAiEmbeddings {
}
if status == reqwest::StatusCode::TOO_MANY_REQUESTS {
let retry_after = response
.headers()
.get("retry-after")
.and_then(|v| v.to_str().ok())
.and_then(|s| s.parse::<u64>().ok())
.map(std::time::Duration::from_secs)
.map(cap_retry_after)
.or(Some(std::time::Duration::from_secs(60)));
let retry_after = Some(crate::llm::retry::parse_retry_after(
response.headers().get("retry-after"),
));
return Err(EmbeddingError::RateLimited { retry_after });
}
@@ -652,49 +640,4 @@ mod tests {
let provider = OpenAiEmbeddings::new("test-key").with_base_url("custom.example.com/v1");
assert_eq!(provider.base_url, "https://custom.example.com/v1");
}
// -- Retry-After header parsing tests (regression for rate limit "None" bug) --
#[test]
fn test_retry_after_parsing_delay_seconds() {
// Verify delay-seconds format is parsed correctly
let header_value = "120";
let duration = parse_retry_after_embeddings_for_test(header_value);
assert_eq!(
duration,
Some(std::time::Duration::from_secs(120)),
"Should parse delay-seconds format"
);
}
#[test]
fn test_retry_after_fallback_missing_header() {
// Regression test: When Retry-After header is missing,
// should fall back to 60s instead of None
let duration = parse_retry_after_embeddings_for_test("");
assert_eq!(
duration,
Some(std::time::Duration::from_secs(60)),
"Missing header should fallback to 60s"
);
}
#[test]
fn test_retry_after_zero_seconds_accepted() {
// Verify zero seconds is a valid retry delay
let duration = parse_retry_after_embeddings_for_test("0");
assert_eq!(duration, Some(std::time::Duration::ZERO));
}
/// Helper function to test Retry-After header parsing logic for embeddings
/// (simulates the parsing done in embed without actual HTTP, including fallback)
fn parse_retry_after_embeddings_for_test(header_value: &str) -> Option<std::time::Duration> {
header_value
.trim()
.parse::<u64>()
.ok()
.map(std::time::Duration::from_secs)
.map(cap_retry_after)
.or(Some(std::time::Duration::from_secs(60)))
}
}