diff --git a/server/src/session/claude.rs b/server/src/session/claude.rs index c7ece4a..b8afb6c 100644 --- a/server/src/session/claude.rs +++ b/server/src/session/claude.rs @@ -802,11 +802,13 @@ async fn follow( process::Liveness::Dead if complete > 0 => {} process::Liveness::Dead => { queue.lock().unwrap().close(&sink, "the session ended"); - let detail = stderr_tail(&stderr_path); - if !detail.is_empty() { - let _ = sink.send(Event::Error { - message: format!("{label} exited:\n{detail}"), - }); + if !process::stopping(&session_dir) { + let detail = stderr_tail(&stderr_path); + if !detail.is_empty() { + let _ = sink.send(Event::Error { + message: format!("{label} exited:\n{detail}"), + }); + } } let _ = sink.send(Event::Status { state: SessionStatus::Exited, diff --git a/server/src/session/codex.rs b/server/src/session/codex.rs index a07c49e..5d26279 100644 --- a/server/src/session/codex.rs +++ b/server/src/session/codex.rs @@ -623,6 +623,7 @@ async fn follow(inner: Arc, mut record: process::Record, mut offset: u64) process::Liveness::Alive | process::Liveness::Unknown => {} process::Liveness::Dead if complete > 0 => {} process::Liveness::Dead => { + let stopped = process::stopping(&inner.session_dir); process::clear(&inner.session_dir); let dropped = { let mut state = inner.state.lock().unwrap(); @@ -639,11 +640,13 @@ async fn follow(inner: Arc, mut record: process::Record, mut offset: u64) for id in dropped { let _ = inner.sink.send(Event::MessageDropped { id }); } - let detail = stderr_tail(&stderr); - if !detail.is_empty() { - let _ = inner.sink.send(Event::Error { - message: format!("Codex exited:\n{detail}"), - }); + if !stopped { + let detail = stderr_tail(&stderr); + if !detail.is_empty() { + let _ = inner.sink.send(Event::Error { + message: format!("Codex exited:\n{detail}"), + }); + } } let _ = inner.sink.send(Event::Status { state: SessionStatus::Exited, diff --git a/server/src/session/llama.rs b/server/src/session/llama.rs index f3feb91..8f7f297 100644 --- a/server/src/session/llama.rs +++ b/server/src/session/llama.rs @@ -312,9 +312,11 @@ fn watch(session_dir: PathBuf, sink: EventSink) { // deliberately, and whoever did that has already said so. None => return, Some((_, process::Liveness::Dead)) => { - let _ = sink.send(Event::Error { - message: "llama-server exited".to_string(), - }); + if !process::stopping(&session_dir) { + let _ = sink.send(Event::Error { + message: "llama-server exited".to_string(), + }); + } let _ = sink.send(Event::Status { state: SessionStatus::Exited, }); diff --git a/server/src/session/mod.rs b/server/src/session/mod.rs index c40eb09..91af508 100644 --- a/server/src/session/mod.rs +++ b/server/src/session/mod.rs @@ -1652,6 +1652,7 @@ impl SessionManager { cwd.display(), record.pid ); + process::mark_stopping(&dir)?; process::stop(&record, process::STOP_GRACE); } Ok(()) @@ -1714,6 +1715,7 @@ impl SessionManager { effort.unwrap_or("default"), record.pid ); + process::mark_stopping(&dir)?; process::stop(&record, process::STOP_GRACE); } Ok(()) @@ -1761,6 +1763,7 @@ impl SessionManager { } }; tracing::info!("stopping session {id} (pid {})", record.pid); + process::mark_stopping(&self.data_dir.join(id))?; process::stop(&record, process::STOP_GRACE); Ok(()) } @@ -4329,6 +4332,52 @@ mod tests { )); } + /// Stderr is a log of the process's whole lifetime, not necessarily why it + /// ended. A requested Stop used to paste its oldest surviving diagnostics + /// into the transcript as a fresh failure -- complete with terminal escape + /// codes -- even though the process ended only because the person asked. + #[tokio::test] + async fn stopping_a_session_does_not_report_old_stderr_as_a_failure() { + let dir = tempfile::tempdir().expect("tempdir"); + let config_path = dir.path().join("config.ron"); + let data_dir = dir.path().join("sessions"); + let provider = seed_stand_in_cli(&config_path, dir.path()); + let manager = SessionManager::new(config_path, data_dir.clone(), data_dir.join("models")) + .expect("manager"); + let info = manager + .spawn_session(stand_in_spec(&provider)) + .expect("spawn"); + let mut rx = manager.session(&info.id).expect("live").subscribe(); + let session_dir = data_dir.join(&info.id); + + std::fs::write( + session_dir.join("stderr.log"), + "\u{1b}[31mERROR\u{1b}[0m an old tool diagnostic\n", + ) + .expect("seed stderr"); + manager.stop_session(&info.id).expect("stop"); + + let seen = collect_until(&mut rx, |event| { + matches!( + event, + Event::Status { + state: SessionStatus::Exited + } + ) + }) + .await; + assert!( + !seen + .iter() + .any(|entry| matches!(entry.event, Event::Error { .. })), + "a requested stop was presented as a failure: {seen:?}" + ); + assert!( + !process::stopping(&session_dir), + "the completed stop left its marker behind" + ); + } + /// A message and a command both mean "now", so neither answers that the /// session's process has gone -- they start one and go to it. /// diff --git a/server/src/session/process.rs b/server/src/session/process.rs index e3b0539..928d4c6 100644 --- a/server/src/session/process.rs +++ b/server/src/session/process.rs @@ -30,6 +30,7 @@ use anyhow::{Context, Result}; use serde::{Deserialize, Serialize}; const RECORD_FILE: &str = "process.json"; +const STOP_REQUEST_FILE: &str = "stop-requested"; /// A process this server started and expects to outlive it. #[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] @@ -101,6 +102,28 @@ fn path(session_dir: &Path) -> PathBuf { session_dir.join(RECORD_FILE) } +fn stop_request_path(session_dir: &Path) -> PathBuf { + session_dir.join(STOP_REQUEST_FILE) +} + +/// Marks the process as one the server deliberately asked to end, so its watcher reports an exit +/// without presenting ordinary stderr from the process's lifetime as the cause. +pub fn mark_stopping(session_dir: &Path) -> Result<()> { + std::fs::OpenOptions::new() + .create(true) + .write(true) + .truncate(true) + .mode(0o600) + .open(stop_request_path(session_dir)) + .with_context(|| format!("marking {} as stopping", session_dir.display()))?; + Ok(()) +} + +/// Whether this process's exit was deliberately requested. +pub fn stopping(session_dir: &Path) -> bool { + stop_request_path(session_dir).is_file() +} + /// The recorded process and whether it is still there, or `None` when nothing /// usable is recorded. A record that does not parse reads as no record: the /// only way to get one is a crash partway through writing it, and the safe @@ -179,11 +202,12 @@ pub fn size_of(path: &Path) -> u64 { /// Forgets the recorded process -- for one confirmed dead, or a session /// being deleted. The path out for [`write`]. pub fn clear(session_dir: &Path) { - let path = path(session_dir); - if let Err(err) = std::fs::remove_file(&path) - && err.kind() != std::io::ErrorKind::NotFound - { - tracing::warn!("couldn't remove {}: {err}", path.display()); + for path in [path(session_dir), stop_request_path(session_dir)] { + if let Err(err) = std::fs::remove_file(&path) + && err.kind() != std::io::ErrorKind::NotFound + { + tracing::warn!("couldn't remove {}: {err}", path.display()); + } } } @@ -405,8 +429,11 @@ mod tests { write(dir.path(), &record); assert_eq!(live(dir.path()), Some(record)); + mark_stopping(dir.path()).expect("mark stopping"); + assert!(stopping(dir.path())); clear(dir.path()); assert_eq!(live(dir.path()), None); + assert!(!stopping(dir.path())); } #[test]