Suppress errors for requested session stops
This commit is contained in:
1 parent
76895bc644
commit
559e6c9226
5 files changed
+101
-18
No files matched your search
@@ -802,11 +802,13 @@ async fn follow(
|
|||||||
process::Liveness::Dead if complete > 0 => {}
|
process::Liveness::Dead if complete > 0 => {}
|
||||||
process::Liveness::Dead => {
|
process::Liveness::Dead => {
|
||||||
queue.lock().unwrap().close(&sink, "the session ended");
|
queue.lock().unwrap().close(&sink, "the session ended");
|
||||||
let detail = stderr_tail(&stderr_path);
|
if !process::stopping(&session_dir) {
|
||||||
if !detail.is_empty() {
|
let detail = stderr_tail(&stderr_path);
|
||||||
let _ = sink.send(Event::Error {
|
if !detail.is_empty() {
|
||||||
message: format!("{label} exited:\n{detail}"),
|
let _ = sink.send(Event::Error {
|
||||||
});
|
message: format!("{label} exited:\n{detail}"),
|
||||||
|
});
|
||||||
|
}
|
||||||
}
|
}
|
||||||
let _ = sink.send(Event::Status {
|
let _ = sink.send(Event::Status {
|
||||||
state: SessionStatus::Exited,
|
state: SessionStatus::Exited,
|
||||||
|
|||||||
@@ -623,6 +623,7 @@ async fn follow(inner: Arc<Inner>, mut record: process::Record, mut offset: u64)
|
|||||||
process::Liveness::Alive | process::Liveness::Unknown => {}
|
process::Liveness::Alive | process::Liveness::Unknown => {}
|
||||||
process::Liveness::Dead if complete > 0 => {}
|
process::Liveness::Dead if complete > 0 => {}
|
||||||
process::Liveness::Dead => {
|
process::Liveness::Dead => {
|
||||||
|
let stopped = process::stopping(&inner.session_dir);
|
||||||
process::clear(&inner.session_dir);
|
process::clear(&inner.session_dir);
|
||||||
let dropped = {
|
let dropped = {
|
||||||
let mut state = inner.state.lock().unwrap();
|
let mut state = inner.state.lock().unwrap();
|
||||||
@@ -639,11 +640,13 @@ async fn follow(inner: Arc<Inner>, mut record: process::Record, mut offset: u64)
|
|||||||
for id in dropped {
|
for id in dropped {
|
||||||
let _ = inner.sink.send(Event::MessageDropped { id });
|
let _ = inner.sink.send(Event::MessageDropped { id });
|
||||||
}
|
}
|
||||||
let detail = stderr_tail(&stderr);
|
if !stopped {
|
||||||
if !detail.is_empty() {
|
let detail = stderr_tail(&stderr);
|
||||||
let _ = inner.sink.send(Event::Error {
|
if !detail.is_empty() {
|
||||||
message: format!("Codex exited:\n{detail}"),
|
let _ = inner.sink.send(Event::Error {
|
||||||
});
|
message: format!("Codex exited:\n{detail}"),
|
||||||
|
});
|
||||||
|
}
|
||||||
}
|
}
|
||||||
let _ = inner.sink.send(Event::Status {
|
let _ = inner.sink.send(Event::Status {
|
||||||
state: SessionStatus::Exited,
|
state: SessionStatus::Exited,
|
||||||
|
|||||||
@@ -312,9 +312,11 @@ fn watch(session_dir: PathBuf, sink: EventSink) {
|
|||||||
// deliberately, and whoever did that has already said so.
|
// deliberately, and whoever did that has already said so.
|
||||||
None => return,
|
None => return,
|
||||||
Some((_, process::Liveness::Dead)) => {
|
Some((_, process::Liveness::Dead)) => {
|
||||||
let _ = sink.send(Event::Error {
|
if !process::stopping(&session_dir) {
|
||||||
message: "llama-server exited".to_string(),
|
let _ = sink.send(Event::Error {
|
||||||
});
|
message: "llama-server exited".to_string(),
|
||||||
|
});
|
||||||
|
}
|
||||||
let _ = sink.send(Event::Status {
|
let _ = sink.send(Event::Status {
|
||||||
state: SessionStatus::Exited,
|
state: SessionStatus::Exited,
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -1652,6 +1652,7 @@ impl SessionManager {
|
|||||||
cwd.display(),
|
cwd.display(),
|
||||||
record.pid
|
record.pid
|
||||||
);
|
);
|
||||||
|
process::mark_stopping(&dir)?;
|
||||||
process::stop(&record, process::STOP_GRACE);
|
process::stop(&record, process::STOP_GRACE);
|
||||||
}
|
}
|
||||||
Ok(())
|
Ok(())
|
||||||
@@ -1714,6 +1715,7 @@ impl SessionManager {
|
|||||||
effort.unwrap_or("default"),
|
effort.unwrap_or("default"),
|
||||||
record.pid
|
record.pid
|
||||||
);
|
);
|
||||||
|
process::mark_stopping(&dir)?;
|
||||||
process::stop(&record, process::STOP_GRACE);
|
process::stop(&record, process::STOP_GRACE);
|
||||||
}
|
}
|
||||||
Ok(())
|
Ok(())
|
||||||
@@ -1761,6 +1763,7 @@ impl SessionManager {
|
|||||||
}
|
}
|
||||||
};
|
};
|
||||||
tracing::info!("stopping session {id} (pid {})", record.pid);
|
tracing::info!("stopping session {id} (pid {})", record.pid);
|
||||||
|
process::mark_stopping(&self.data_dir.join(id))?;
|
||||||
process::stop(&record, process::STOP_GRACE);
|
process::stop(&record, process::STOP_GRACE);
|
||||||
Ok(())
|
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
|
/// 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.
|
/// session's process has gone -- they start one and go to it.
|
||||||
///
|
///
|
||||||
|
|||||||
@@ -30,6 +30,7 @@ use anyhow::{Context, Result};
|
|||||||
use serde::{Deserialize, Serialize};
|
use serde::{Deserialize, Serialize};
|
||||||
|
|
||||||
const RECORD_FILE: &str = "process.json";
|
const RECORD_FILE: &str = "process.json";
|
||||||
|
const STOP_REQUEST_FILE: &str = "stop-requested";
|
||||||
|
|
||||||
/// A process this server started and expects to outlive it.
|
/// A process this server started and expects to outlive it.
|
||||||
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)]
|
||||||
@@ -101,6 +102,28 @@ fn path(session_dir: &Path) -> PathBuf {
|
|||||||
session_dir.join(RECORD_FILE)
|
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
|
/// 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
|
/// 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
|
/// 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
|
/// Forgets the recorded process -- for one confirmed dead, or a session
|
||||||
/// being deleted. The path out for [`write`].
|
/// being deleted. The path out for [`write`].
|
||||||
pub fn clear(session_dir: &Path) {
|
pub fn clear(session_dir: &Path) {
|
||||||
let path = path(session_dir);
|
for path in [path(session_dir), stop_request_path(session_dir)] {
|
||||||
if let Err(err) = std::fs::remove_file(&path)
|
if let Err(err) = std::fs::remove_file(&path)
|
||||||
&& err.kind() != std::io::ErrorKind::NotFound
|
&& err.kind() != std::io::ErrorKind::NotFound
|
||||||
{
|
{
|
||||||
tracing::warn!("couldn't remove {}: {err}", path.display());
|
tracing::warn!("couldn't remove {}: {err}", path.display());
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -405,8 +429,11 @@ mod tests {
|
|||||||
write(dir.path(), &record);
|
write(dir.path(), &record);
|
||||||
assert_eq!(live(dir.path()), Some(record));
|
assert_eq!(live(dir.path()), Some(record));
|
||||||
|
|
||||||
|
mark_stopping(dir.path()).expect("mark stopping");
|
||||||
|
assert!(stopping(dir.path()));
|
||||||
clear(dir.path());
|
clear(dir.path());
|
||||||
assert_eq!(live(dir.path()), None);
|
assert_eq!(live(dir.path()), None);
|
||||||
|
assert!(!stopping(dir.path()));
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
|
|||||||
Reference in new issue
Block a user