diff --git a/README.md b/README.md index a5ec9b7a..9c896c5d 100644 --- a/README.md +++ b/README.md @@ -353,7 +353,7 @@ milestone. | **Every Ubuntu LTS validated** โ€” 22.04, 24.04 and 26.04 all at 79/79, each with a replay twin that reproduces it | โœ… | | Telegram approval interface | ๐Ÿ“‹ roadmap | -**1,991 Rust tests and 72 frontend tests** form the current deterministic +**1,993 Rust tests and 72 frontend tests** form the current deterministic release baseline. ## Configure your LLM diff --git a/crates/sysknife-brain/src/planner.rs b/crates/sysknife-brain/src/planner.rs index 42532fb8..23d23623 100644 --- a/crates/sysknife-brain/src/planner.rs +++ b/crates/sysknife-brain/src/planner.rs @@ -1742,6 +1742,75 @@ mod tests { ); } + /// A 408 Request Timeout is the second 4xx worth retrying, and for a + /// different reason than the tool-use 400: the status says the *server* did + /// not see the whole request in time, so nothing about the bytes we built is + /// known to be wrong. The identical request is re-sent, unchanged, and must + /// reach the provider a second time. + /// + /// The stub times out on the first attempt and answers on the second, so the + /// request count is exactly two โ€” "a 408 goes out twice", not "a 408 goes out + /// until the retry budget is spent". + #[tokio::test] + async fn a_408_request_timeout_goes_out_twice() { + struct TimesOutOnceThenAnswers { + calls: Arc, + second: Arc>>, + } + + #[async_trait::async_trait] + impl LlmProvider for TimesOutOnceThenAnswers { + async fn complete( + &self, + _system: &str, + messages: &[Message], + _tools: &[ToolDefinition], + _max_tokens: u32, + ) -> Result { + if self.calls.fetch_add(1, Ordering::SeqCst) == 0 { + return Err(rig_status( + http::StatusCode::REQUEST_TIMEOUT, + "request timed out", + )); + } + *self.second.lock().await = messages.to_vec(); + Ok(Completion { + content: vec![ContentBlock::Text { + text: "recovered".into(), + }], + stop_reason: StopReason::EndTurn, + }) + } + } + + let calls = Arc::new(AtomicUsize::new(0)); + let second = Arc::new(tokio::sync::Mutex::new(Vec::new())); + let planner = LlmPlanner::new( + Box::new(TimesOutOnceThenAnswers { + calls: Arc::clone(&calls), + second: Arc::clone(&second), + }), + Box::new(UnusedState), + 1, + ); + planner + .complete_with_retry("SYS", &[Message::user_text("hi")], &[]) + .await + .expect("the second attempt succeeds"); + + assert_eq!( + calls.load(Ordering::SeqCst), + 2, + "a 408 must be re-sent exactly once, so it goes out twice" + ); + let sent = second.lock().await; + assert_eq!( + sent.len(), + 1, + "a 408 retry re-sends the request unchanged โ€” no correction is appended" + ); + } + /// A 400 that *is* worth retrying: the retry appends a correction, so the /// bytes change. It must reach the provider a second time. #[tokio::test] diff --git a/crates/sysknife-brain/src/provider.rs b/crates/sysknife-brain/src/provider.rs index f233f096..347ee365 100644 --- a/crates/sysknife-brain/src/provider.rs +++ b/crates/sysknife-brain/src/provider.rs @@ -165,8 +165,8 @@ pub enum ProviderError { /// `providers::classify_status`; the `Auth`/`RateLimit` classes become their /// own variants, and `StatusClass::Other` โ€” every 4xx and 5xx that is not /// 401, 403, or 429 โ€” becomes this variant, keeping the status intact. - /// [`is_retryable`](Self::is_retryable) then applies the real policy: 5xx and - /// 429 deserve a backoff, a permanent 4xx does not. + /// [`is_retryable`](Self::is_retryable) then applies the real policy: 5xx, + /// 429, and 408 deserve a backoff; a permanent 4xx does not. /// /// This is the only *status-bearing* variant, and it is deliberately the one /// the policy was written for. [`Request`](Self::Request) is now reserved for @@ -226,11 +226,19 @@ impl ProviderError { // 5xx is the provider's own fault and usually momentary; 429 means // "later", which is exactly what a backoff provides. Every other 4xx // is a defect in the request just built, and building it again - // produces the same bytes โ€” except a tool-use rejection, where the - // retry appends a correction and so does send different bytes. That - // is the one 4xx worth re-asking about. + // produces the same bytes โ€” except two cases. + // + // A tool-use rejection is one: the retry appends a correction, so it + // does send different bytes, and without this arm the correction + // would never reach the model. + // + // 408 Request Timeout is the other, and it is not a statement about + // the request at all โ€” the server gave up *reading* it before the + // body arrived. The bytes were fine; the read did not win its race + // with the clock. Re-sending the identical request is the whole fix, + // so it retries even though it is a 4xx. ProviderError::Http { status, .. } => { - *status >= 500 || *status == 429 || self.is_invalid_tool_call() + *status >= 500 || *status == 429 || *status == 408 || self.is_invalid_tool_call() } ProviderError::RateLimit(_) => true, ProviderError::Auth(_) => false, @@ -331,6 +339,23 @@ mod retryability_tests { ); } + /// The other 4xx worth retrying, and for a different reason than a tool-use + /// rejection. A 408 Request Timeout says the *server* did not receive the + /// whole request in time โ€” nothing about the bytes we sent is known to be + /// wrong, so re-sending the identical request is the right move rather than a + /// request defect. + #[test] + fn a_request_timeout_is_retryable_despite_being_a_4xx() { + assert!( + ProviderError::Http { + status: 408, + body: "request timed out".into() + } + .is_retryable(), + "408 means the server timed out reading the request, not that our request was wrong" + ); + } + #[test] fn auth_failure_is_not_retryable() { assert!(!ProviderError::Auth("invalid api key".into()).is_retryable()); diff --git a/docs/distro-support.md b/docs/distro-support.md index 566fb327..0733bc6c 100644 --- a/docs/distro-support.md +++ b/docs/distro-support.md @@ -94,7 +94,7 @@ family and the atomic story family are implemented and covered by the workspace suite. What is missing is a way to put the helpers somewhere the daemon's own grants already point. -The deterministic workspace baseline is 1,991 Rust tests plus 72 frontend +The deterministic workspace baseline is 1,993 Rust tests plus 72 frontend tests. Those tests verify action construction, policy, approval, storage, and UI behavior, but they do not replace a real distribution VM run. diff --git a/docs/introduction.md b/docs/introduction.md index a1d77e96..99d15ebf 100644 --- a/docs/introduction.md +++ b/docs/introduction.md @@ -146,7 +146,7 @@ flow. ## Status -192 typed actions ยท 1,991 Rust tests + 72 frontend tests ยท MIT +192 typed actions ยท 1,993 Rust tests + 72 frontend tests ยท MIT SysKnife is the reference implementation of the [LACS specification](https://github.com/lacs-project/specification) โ€” a diff --git a/tests/evidence/workspace-tests.json b/tests/evidence/workspace-tests.json index 46fd23d8..b3e4e3eb 100644 --- a/tests/evidence/workspace-tests.json +++ b/tests/evidence/workspace-tests.json @@ -4,6 +4,6 @@ "tests": "cargo nextest run --workspace --locked" }, "frontend_tests": 72, - "tests": 1991, + "tests": 1993, "version": 2 }