fix(retry): retry a 408 Request Timeout, which is not a request defect

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

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<AtomicUsize>,
+            second: Arc<tokio::sync::Mutex<Vec<Message>>>,
+        }
+
+        #[async_trait::async_trait]
+        impl LlmProvider for TimesOutOnceThenAnswers {
+            async fn complete(
+                &self,
+                _system: &str,
+                messages: &[Message],
+                _tools: &[ToolDefinition],
+                _max_tokens: u32,
+            ) -> Result<Completion, ProviderError> {
+                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]

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());

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.
 

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

tests/evidence/workspace-tests.json

@@ -4,6 +4,6 @@
     "tests": "cargo nextest run --workspace --locked"
   },
   "frontend_tests": 72,
-  "tests": 1991,
+  "tests": 1993,
   "version": 2
 }