diff --git a/Cargo.lock b/Cargo.lock index bda9c2d3..70987333 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1723,6 +1723,7 @@ dependencies = [ "schemars", "serde", "serde_json", + "tempfile", "tokio", "tracing", "tracing-subscriber", diff --git a/hive-bash-mcp/Cargo.toml b/hive-bash-mcp/Cargo.toml index 0c4f36eb..696ee5ee 100644 --- a/hive-bash-mcp/Cargo.toml +++ b/hive-bash-mcp/Cargo.toml @@ -26,6 +26,9 @@ tokio.workspace = true tracing.workspace = true tracing-subscriber.workspace = true +[dev-dependencies] +tempfile = "3" + # `hive-bash-daemon` — long-running per-agent bash task runner. Spawns # `sh -c` subprocesses, monitors completion, writes task state files # under harness/bash-tasks/, and serves the MCP tools (`run`/`status`/ diff --git a/hive-bash-mcp/src/runner.rs b/hive-bash-mcp/src/runner.rs index 8e7d609f..a13d141d 100644 --- a/hive-bash-mcp/src/runner.rs +++ b/hive-bash-mcp/src/runner.rs @@ -18,6 +18,12 @@ //! (the process died with the previous daemon). The todo is still updated to //! its interrupted-done summary so the agent is not silently blocked. //! +//! At daemon startup, before that interrupted-marking pass, any file under +//! `tasks_dir()` older than 30 days is deleted outright — see +//! [`sweep_old_tasks`]. Nothing swept this directory before, and it had +//! grown without bound as a result, repeatedly holding stale credentials +//! in captured output. +//! //! The runner kills the child process on timeout — `tokio::process::Child::drop()` //! does not kill children, so we explicitly call `child.kill().await`. @@ -436,6 +442,10 @@ async fn run_loop(socket: PathBuf) { if let Err(e) = std::fs::create_dir_all(paths::tasks_dir()) { tracing::warn!(error = ?e, "bash_runner: create tasks dir failed"); } + let removed = sweep_old_tasks(&paths::tasks_dir(), SystemTime::now() - RETENTION); + if removed > 0 { + tracing::info!(removed, "bash_runner: swept old task files at startup"); + } mark_interrupted(&socket).await; let claimed: Arc>> = Arc::new(Mutex::new(HashSet::new())); @@ -446,6 +456,58 @@ async fn run_loop(socket: PathBuf) { } } +/// How long a task's files are kept before [`sweep_old_tasks`] removes +/// them. `harness/bash-tasks/` grew without bound (and had repeatedly +/// held stale credentials in captured output) because nothing ever swept +/// it. Mara: "build automatic cleanup at daemon start to clear older +/// than 30d" — a flat age cutoff, no count-based retention knob. +const RETENTION: Duration = Duration::from_hours(720); + +/// Delete every regular file directly under `dir` whose mtime is older +/// than `cutoff`. Runs once, at daemon startup (`run_loop`), before +/// [`mark_interrupted`] — an ancient still-`running` task is swept away +/// outright rather than given a fresh "interrupted" todo nobody's going +/// to act on 30 days later. +/// +/// Whole-directory, extension-agnostic rather than `.json`-id-driven like +/// [`mark_interrupted`]/[`poll_once`]: a task's `.json`/`.out`/`.err` +/// triple is written together at completion (the same instant, well +/// inside the 30-day granularity this cares about), so sweeping +/// file-by-file on its own mtime can't split a still-referenced group — +/// and this shape also reaps orphaned `.out`/`.err` (a crash between +/// writing output and the final `.json` write) and stray `.json.tmp` +/// scratch files (`write_task`'s tmp+rename can leave one behind on a +/// crash mid-rename) that an id-driven walk would miss entirely. +/// Directories are skipped (`tasks_dir()` holds none today, but a future +/// entry shouldn't be treated as sweepable file content). Best-effort: a +/// per-entry `remove_file` failure is logged and the pass continues +/// rather than aborting on the first one. Returns the count actually +/// removed. +fn sweep_old_tasks(dir: &Path, cutoff: SystemTime) -> usize { + let Ok(rd) = std::fs::read_dir(dir) else { + return 0; + }; + let mut removed = 0; + for entry in rd.flatten() { + let Ok(meta) = entry.metadata() else { continue }; + if !meta.is_file() { + continue; + } + let Ok(mtime) = meta.modified() else { continue }; + if mtime >= cutoff { + continue; + } + let path = entry.path(); + match std::fs::remove_file(&path) { + Ok(()) => removed += 1, + Err(e) => { + tracing::warn!(path = %path.display(), error = ?e, "bash_runner: sweep remove failed"); + } + } + } + removed +} + /// On boot, flip any `running` tasks to `interrupted` and fire a wake. async fn mark_interrupted(socket: &Path) { let Ok(rd) = std::fs::read_dir(paths::tasks_dir()) else { @@ -967,4 +1029,119 @@ mod tests { assert!(!body.contains(".out"), "no stdout ⇒ no stdout pointer"); }); } + + // sweep_old_tasks: a flat, whole-directory age sweep. + // `cutoff` is passed in explicitly (mirroring `hive-agent`'s + // `has_fresh_credentials(dir, since, ..)` pattern) so these don't need + // to fake a real 30-day-old file — `set_modified` moves a fresh + // tempfile's mtime directly, no sleeping required. + + #[test] + fn sweep_removes_only_files_older_than_cutoff() { + use super::sweep_old_tasks; + use std::time::{Duration, SystemTime}; + let dir = tempfile::tempdir().unwrap(); + let old = dir.path().join("old.json"); + let fresh = dir.path().join("fresh.json"); + std::fs::write(&old, b"{}").unwrap(); + std::fs::write(&fresh, b"{}").unwrap(); + let now = SystemTime::now(); + std::fs::File::open(&old) + .unwrap() + .set_modified(now - Duration::from_hours(744)) // 31 days + .unwrap(); + std::fs::File::open(&fresh) + .unwrap() + .set_modified(now) + .unwrap(); + + let removed = sweep_old_tasks(dir.path(), now - Duration::from_hours(720)); // 30 days + + assert_eq!(removed, 1); + assert!(!old.exists(), "the 31-day-old file must be gone"); + assert!(fresh.exists(), "today's file must survive"); + } + + #[test] + fn sweep_keeps_a_file_exactly_at_the_cutoff() { + // Boundary: `mtime >= cutoff` is kept, matching `sweep_old_tasks`'s + // doc — a task's mtime is never used to justify deleting it on the + // very instant it crosses the line, only once it's strictly older. + use super::sweep_old_tasks; + use std::time::SystemTime; + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("boundary.json"); + std::fs::write(&path, b"{}").unwrap(); + let cutoff = SystemTime::now(); + std::fs::File::open(&path) + .unwrap() + .set_modified(cutoff) + .unwrap(); + + let removed = sweep_old_tasks(dir.path(), cutoff); + + assert_eq!(removed, 0); + assert!(path.exists()); + } + + #[test] + fn sweep_is_extension_agnostic_and_reaps_orphans_and_tmp_files() { + // Whole-directory by design (see the doc comment): an orphaned + // `.out` with no `.json` (crash mid-task) and a stray `.json.tmp` + // (crash mid-rename in `write_task`) are exactly the garbage an + // id-driven walk like `mark_interrupted`'s would miss. + use super::sweep_old_tasks; + use std::time::{Duration, SystemTime}; + let dir = tempfile::tempdir().unwrap(); + let orphan_out = dir.path().join("orphan.out"); + let stray_tmp = dir.path().join("abc123.json.tmp"); + std::fs::write(&orphan_out, b"stdout").unwrap(); + std::fs::write(&stray_tmp, b"{}").unwrap(); + let past = SystemTime::now() - Duration::from_hours(1440); // 60 days + std::fs::File::open(&orphan_out) + .unwrap() + .set_modified(past) + .unwrap(); + std::fs::File::open(&stray_tmp) + .unwrap() + .set_modified(past) + .unwrap(); + + let removed = sweep_old_tasks(dir.path(), SystemTime::now()); + + assert_eq!(removed, 2); + assert!(!orphan_out.exists()); + assert!(!stray_tmp.exists()); + } + + #[test] + fn sweep_skips_subdirectories() { + // Defensive: `tasks_dir()` holds no directories today, but a + // future entry must not be treated as sweepable file content + // (`remove_file` on a directory just errors and is swallowed — + // this pins the *intent*, not merely today's accidental safety). + use super::sweep_old_tasks; + use std::time::{Duration, SystemTime}; + let dir = tempfile::tempdir().unwrap(); + let sub = dir.path().join("a-subdir"); + std::fs::create_dir(&sub).unwrap(); + + let removed = sweep_old_tasks(dir.path(), SystemTime::now() + Duration::from_secs(1)); + + assert_eq!(removed, 0); + assert!(sub.exists()); + } + + #[test] + fn sweep_missing_dir_is_a_noop() { + // Defensive, same shape as `has_fresh_credentials_missing_dir_is_false` + // in `hive-agent`: a not-yet-created tasks dir must not panic. + use super::sweep_old_tasks; + use std::time::SystemTime; + let missing = tempfile::tempdir() + .unwrap() + .path() + .join("never-created-subdir"); + assert_eq!(sweep_old_tasks(&missing, SystemTime::now()), 0); + } }