huskies: merge 1160 bug Chat bot crash-loops on poisoned Claude Code session resume

server/src/llm/providers/claude_code/mod.rs

@@ -1,6 +1,7 @@
 //! Claude Code provider — runs Claude Code CLI in a PTY and parses structured output.
 #![allow(unused_imports, dead_code)]
 use crate::slog;
+use crate::slog_warn;
 use portable_pty::{CommandBuilder, PtySize, native_pty_system};
 use std::io::{BufRead, BufReader};
 use std::sync::Arc;
@@ -86,6 +87,14 @@ impl ClaudeCodeProvider {
             let cancelled_inner = cancelled.clone();
             let auth_failed = Arc::new(AtomicBool::new(false));
             let auth_failed_clone = auth_failed.clone();
+            // Set inside run_pty_session whenever the Claude Code process exits
+            // non-zero, even if a "result" event was already received (which
+            // otherwise makes run_pty_session return Ok). A poisoned session
+            // resume can produce a result event and then still crash, or exit
+            // non-zero without ever producing output — either way we must not
+            // hand the session_id back to the caller for reuse.
+            let exit_failed = Arc::new(AtomicBool::new(false));
+            let exit_failed_clone = exit_failed.clone();
 
             let (token_tx, mut token_rx) = tokio::sync::mpsc::unbounded_channel::<String>();
             let (thinking_tx, mut thinking_rx) = tokio::sync::mpsc::unbounded_channel::<String>();
@@ -102,6 +111,7 @@ impl ClaudeCodeProvider {
                     model_override.as_deref(),
                     cancelled_inner,
                     auth_failed_clone,
+                    exit_failed_clone,
                     token_tx,
                     thinking_tx,
                     activity_tx,
@@ -161,9 +171,14 @@ impl ClaudeCodeProvider {
             slog!("[pty-debug] RECEIVED session_id: {:?}", captured_session_id);
             let structured_messages: Vec<Message> = msg_rx.try_iter().collect();
 
+            let exited_non_zero = exit_failed.load(Ordering::Relaxed);
+            let produced_nothing = structured_messages.is_empty();
+            let session_id =
+                resolve_session_id(captured_session_id, exited_non_zero, produced_nothing);
+
             return Ok(ClaudeCodeResult {
                 messages: structured_messages,
-                session_id: captured_session_id,
+                session_id,
             });
         }
 
@@ -172,6 +187,32 @@ impl ClaudeCodeProvider {
     }
 }
 
+/// Decide whether a session_id may be handed back to the caller for reuse.
+///
+/// A poisoned session resume can still leave `run_pty_session` returning
+/// `Ok`: the process may exit non-zero after a `result` event was already
+/// seen, or it may exit cleanly while producing no assistant/tool output at
+/// all. Preserving the session_id in either case would make the next turn
+/// resume the same broken session via `--resume`, crash-looping forever, so
+/// it is cleared instead. Logs a WARN (visible in gateway logs) only when
+/// there was actually a session_id to clear.
+fn resolve_session_id(
+    captured_session_id: Option<String>,
+    exited_non_zero: bool,
+    produced_nothing: bool,
+) -> Option<String> {
+    if exited_non_zero || produced_nothing {
+        if let Some(ref sid) = captured_session_id {
+            slog_warn!(
+                "[claude-code] poisoned session detected (exit_non_zero={exited_non_zero}, no_output={produced_nothing}) — clearing session_id {sid} to prevent crash loop"
+            );
+        }
+        None
+    } else {
+        captured_session_id
+    }
+}
+
 /// Run `claude -p` with stream-json output inside a PTY.
 ///
 /// The PTY makes isatty() return true. The `-p` flag gives us
@@ -194,6 +235,7 @@ fn run_pty_session(
     model: Option<&str>,
     cancelled: Arc<AtomicBool>,
     auth_failed: Arc<AtomicBool>,
+    exit_failed: Arc<AtomicBool>,
     token_tx: tokio::sync::mpsc::UnboundedSender<String>,
     thinking_tx: tokio::sync::mpsc::UnboundedSender<String>,
     activity_tx: tokio::sync::mpsc::UnboundedSender<String>,
@@ -392,6 +434,16 @@ fn run_pty_session(
     // Wait for the reader thread to release the cloned PTY master fd.
     let _ = reader_handle.join();
 
+    // Record a non-zero exit regardless of whether a result event was seen.
+    // A poisoned session resume can crash *after* emitting a result event,
+    // which would otherwise slip past the check below and return Ok — the
+    // caller uses this flag to avoid persisting the session_id in that case.
+    if let Some(ref status) = exit_status
+        && !status.success()
+    {
+        exit_failed.store(true, Ordering::Relaxed);
+    }
+
     // Non-zero exit without a result event means Claude Code crashed
     // (e.g. MCP server not yet connected). Propagate as an error so
     // the caller can clear the session_id instead of persisting it.
@@ -413,9 +465,75 @@ fn run_pty_session(
 #[cfg(test)]
 mod tests {
     use super::*;
+    use crate::log_buffer::LogLevel;
 
     #[test]
     fn claude_code_provider_new() {
         let _provider = ClaudeCodeProvider::new();
     }
+
+    // -----------------------------------------------------------------
+    // resolve_session_id (story 1160: poisoned session crash-loop fix)
+    // -----------------------------------------------------------------
+
+    #[test]
+    fn resolve_session_id_clears_on_non_zero_exit() {
+        let sid = resolve_session_id(Some("poisoned-sid".to_string()), true, false);
+        assert_eq!(sid, None, "non-zero exit must clear the session_id");
+        let warnings =
+            crate::log_buffer::global().get_recent_entries(50, Some("poisoned-sid"), None);
+        assert!(
+            warnings
+                .iter()
+                .any(|e| e.level == LogLevel::Warn && e.message.contains("poisoned-sid")),
+            "expected a WARN log line naming the cleared session_id"
+        );
+    }
+
+    #[test]
+    fn resolve_session_id_clears_on_empty_output() {
+        let sid = resolve_session_id(Some("another-poisoned-sid".to_string()), false, true);
+        assert_eq!(
+            sid, None,
+            "no output and no tool results must clear the session_id"
+        );
+    }
+
+    #[test]
+    fn resolve_session_id_preserves_on_clean_success() {
+        let sid = resolve_session_id(Some("healthy-sid".to_string()), false, false);
+        assert_eq!(
+            sid,
+            Some("healthy-sid".to_string()),
+            "a clean turn with output must preserve the session_id for resumption"
+        );
+    }
+
+    #[test]
+    fn resolve_session_id_handles_none_without_panicking() {
+        // No session_id was captured at all — clearing logic must be a no-op,
+        // not a panic, and must not emit a spurious WARN (nothing to clear).
+        let sid = resolve_session_id(None, true, true);
+        assert_eq!(sid, None);
+    }
+
+    /// Simulates the crash-loop scenario from steps_to_reproduce: a resumed
+    /// turn crashes (non-zero exit), the session_id is cleared, and the very
+    /// next turn (now with no session_id, i.e. no `--resume`) is treated as a
+    /// fresh session rather than being poisoned again.
+    #[test]
+    fn crash_loop_recovers_after_a_single_failed_turn() {
+        // Turn 1: resuming an old session hits the poisoned state and crashes.
+        let turn_1_result = resolve_session_id(Some("old-poisoned-sid".to_string()), true, false);
+        assert_eq!(turn_1_result, None, "turn 1 must clear the session_id");
+
+        // Turn 2: the caller now passes `None` as the resume id (no --resume),
+        // Claude Code starts a fresh session and succeeds, producing a new id.
+        let turn_2_result = resolve_session_id(Some("fresh-sid".to_string()), false, false);
+        assert_eq!(
+            turn_2_result,
+            Some("fresh-sid".to_string()),
+            "turn 2 must succeed and preserve the new session_id — at most one failed turn"
+        );
+    }
 }