From 8ae2eaaad391b1a59307594b49a93821147eb794 Mon Sep 17 00:00:00 2001 From: Huskies Agent Date: Thu, 16 Jul 2026 09:45:47 +0000 Subject: [PATCH] huskies: merge 1160 bug Chat bot crash-loops on poisoned Claude Code session resume --- server/src/llm/providers/claude_code/mod.rs | 120 +++++++++++++++++++- 1 file changed, 119 insertions(+), 1 deletion(-) diff --git a/server/src/llm/providers/claude_code/mod.rs b/server/src/llm/providers/claude_code/mod.rs index 047923b9..9f4573ef 100644 --- a/server/src/llm/providers/claude_code/mod.rs +++ b/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::(); let (thinking_tx, mut thinking_rx) = tokio::sync::mpsc::unbounded_channel::(); @@ -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 = 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, + exited_non_zero: bool, + produced_nothing: bool, +) -> Option { + 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, auth_failed: Arc, + exit_failed: Arc, token_tx: tokio::sync::mpsc::UnboundedSender, thinking_tx: tokio::sync::mpsc::UnboundedSender, activity_tx: tokio::sync::mpsc::UnboundedSender, @@ -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" + ); + } }