From 4827170715dfa8916349a39b6493877c402fae87 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 27 Aug 2026 08:18:41 -0700 Subject: [PATCH 1/4] Report and retry Docker resources remove_project could not delete MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit remove_project_volumes always returned Ok(()) regardless of what actually happened, making the `if let Err(e)` guarding it at every call site dead code. remove_project then dropped the project record unconditionally, so a volume, image or container that failed to delete became permanently unreachable — confirmed against a real orphaned volume pair found in the wild (fixes #31). remove_project_volumes/remove_snapshot_image/remove_container now report what they could not remove (treating "already gone" as success rather than a leftover), remove_project surfaces this to the user via a toast, and before dropping the project record it writes a pending-cleanup record that startup housekeeping retries automatically on the next launch. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FGjXq6fqtAFHdbhk4f3PfZ --- .../src/commands/project_commands.rs | 148 +++++++++++- app/src-tauri/src/docker/container.rs | 102 ++++++-- app/src-tauri/src/lib.rs | 4 + app/src-tauri/src/models/project.rs | 44 ++++ app/src-tauri/src/storage/mod.rs | 1 + app/src-tauri/src/storage/pending_cleanup.rs | 217 ++++++++++++++++++ .../components/projects/home/ProjectHome.tsx | 20 +- app/src/hooks/useProjects.ts | 3 +- app/src/lib/tauri-commands.ts | 4 +- app/src/lib/types.ts | 9 + 10 files changed, 520 insertions(+), 32 deletions(-) create mode 100644 app/src-tauri/src/storage/pending_cleanup.rs diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 564012f..0f0ce5d 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -2,7 +2,7 @@ use tauri::{Emitter, State}; use crate::commands::aws_commands; use crate::docker; -use crate::models::{container_config, AppSettings, Backend, BedrockAuthMethod, Project, ProjectPath, ProjectStatus}; +use crate::models::{container_config, AppSettings, Backend, BedrockAuthMethod, Project, ProjectPath, ProjectRemovalReport, ProjectStatus}; use crate::storage::secure; use crate::AppState; @@ -696,7 +696,7 @@ pub async fn add_project( pub async fn remove_project( project_id: String, state: State<'_, AppState>, -) -> Result<(), String> { +) -> Result { // **H-2: the only writer of these three categories that held nothing.** // This purges migration artifacts, removes `triple-c-snapshot-{id}` and // both named volumes — and a compaction resolves that same tag when its @@ -722,12 +722,24 @@ pub async fn remove_project( // holding an entire snapshot image that nothing will ever reference again. crate::commands::migration_commands::purge_migration_artifacts(&project_id).await; - // Stop and remove container if it exists - if let Some(ref project) = state.projects_store.get(&project_id) { + // Stop and remove container if it exists. Everything named in `report` + // below is what will be unreachable the moment this function drops the + // project record — see [`ProjectRemovalReport`] and + // `storage::pending_cleanup`, which is what makes it reachable anyway. + let mut report = ProjectRemovalReport::default(); + let existing_project = state.projects_store.get(&project_id); + + if let Some(ref project) = existing_project { if let Some(ref container_id) = project.container_id { state.exec_manager.close_sessions_for_container(container_id).await; let _ = docker::stop_container(container_id).await; - let _ = docker::remove_container(container_id).await; + if let Err(e) = docker::remove_container(container_id).await { + log::warn!( + "Failed to remove container {} for project {}: {}", + container_id, project_id, e + ); + report.container = Some(container_id.clone()); + } } // Legacy MCP cleanup (pre-MCP-removal installs): drop any leftover MCP @@ -738,10 +750,9 @@ pub async fn remove_project( // Clean up the snapshot image + volumes if let Err(e) = docker::remove_snapshot_image(project).await { log::warn!("Failed to remove snapshot image for project {}: {}", project_id, e); + report.image = Some(docker::get_snapshot_image_name(project)); } - if let Err(e) = docker::remove_project_volumes(project).await { - log::warn!("Failed to remove project volumes for project {}: {}", project_id, e); - } + report.volumes = docker::remove_project_volumes(project).await; } // Clean up keychain secrets for this project @@ -749,7 +760,111 @@ pub async fn remove_project( log::warn!("Failed to delete keychain secrets for project {}: {}", project_id, e); } - state.projects_store.remove(&project_id) + if !report.is_clean() { + let record = crate::storage::pending_cleanup::PendingCleanup { + project_id: project_id.clone(), + project_name: existing_project.map(|p| p.name).unwrap_or_default(), + container_id: report.container.clone(), + image: report.image.clone(), + volumes: report.volumes.clone(), + recorded_at: chrono::Utc::now().to_rfc3339(), + }; + match crate::storage::pending_cleanup::save(&record) { + Ok(()) => log::warn!( + "Project {} removed with Docker resources still present: {:?} — recorded for \ + automatic retry on next launch", + project_id, report + ), + Err(e) => log::error!( + "Project {} removed with Docker resources still present ({:?}), and the \ + pending-cleanup record could not be written ({}) — nothing will retry removing \ + them", + project_id, report, e + ), + } + } + + state.projects_store.remove(&project_id)?; + Ok(report) +} + +/// Retry every pending-cleanup record left behind by a [`remove_project`] +/// that could not finish. Run once at startup alongside the other reapers +/// (see `lib.rs`'s "Startup disk housekeeping" block) — never on a timer and +/// never blocking anything, since a locked volume or an in-use image can sit +/// unresolved for an arbitrary amount of time and the daemon may not even be +/// up yet. +/// +/// Not a `#[tauri::command]`: nothing in the UI surfaces this list yet +/// (deliberately — see `SnapshotSweepReport`'s doc comment for the same +/// reasoning), so there is no IPC contract to keep. A record that still has +/// leftovers after this is written back so the next run does not lose track +/// of what changed; one that is now empty is deleted. +pub async fn retry_pending_cleanup_logged() { + let records = crate::storage::pending_cleanup::list(); + if records.is_empty() { + return; + } + + let mut cleaned = 0usize; + let mut still_pending = 0usize; + + for mut record in records { + if let Some(container_id) = record.container_id.take() { + match docker::remove_container(&container_id).await { + Ok(()) => {} + Err(e) => { + log::warn!( + "Pending cleanup: still could not remove container {} for project {} \ + ({}): {}", + container_id, record.project_id, record.project_name, e + ); + record.container_id = Some(container_id); + } + } + } + + if let Some(image) = record.image.take() { + match docker::remove_image_by_name(&image).await { + Ok(()) => {} + Err(e) => { + log::warn!( + "Pending cleanup: still could not remove image {} for project {} ({}): {}", + image, record.project_id, record.project_name, e + ); + record.image = Some(image); + } + } + } + + if !record.volumes.is_empty() { + record.volumes = docker::remove_volumes_by_name(&record.volumes).await; + } + + if record.is_empty() { + if let Err(e) = crate::storage::pending_cleanup::clear(&record.project_id) { + log::warn!( + "Pending cleanup for project {} ({}) finished but the record could not be \ + deleted: {}", + record.project_id, record.project_name, e + ); + } + cleaned += 1; + } else { + still_pending += 1; + if let Err(e) = crate::storage::pending_cleanup::save(&record) { + log::warn!( + "Could not update pending cleanup record for project {} ({}): {}", + record.project_id, record.project_name, e + ); + } + } + } + + log::info!( + "Pending cleanup retry: {} project(s) fully cleaned up, {} still have leftovers", + cleaned, still_pending + ); } #[tauri::command] @@ -1265,8 +1380,19 @@ pub async fn rebuild_project_container( if let Err(e) = docker::remove_snapshot_image(&project).await { log::warn!("Failed to remove snapshot image for project {}: {}", project_id, e); } - if let Err(e) = docker::remove_project_volumes(&project).await { - log::warn!("Failed to remove project volumes for project {}: {}", project_id, e); + let leftover_volumes = docker::remove_project_volumes(&project).await; + if !leftover_volumes.is_empty() { + // Unlike `remove_project`, Reset keeps the project record — but a + // volume that survives this is reused as-is by the container + // `start_project_container_locked` creates below, which is exactly + // what Reset promises not to do. No pending-cleanup record: the + // project id is still live, so a later Reset attempt can retry this + // itself rather than needing startup housekeeping to do it. + log::warn!( + "Reset could not remove volume(s) {:?} for project {} — the new container may reuse \ + their old contents instead of starting clean", + leftover_volumes, project_id + ); } // Start fresh. The locked variant, because `_guard` above is this project's diff --git a/app/src-tauri/src/docker/container.rs b/app/src-tauri/src/docker/container.rs index a48cf72..39dd9ff 100644 --- a/app/src-tauri/src/docker/container.rs +++ b/app/src-tauri/src/docker/container.rs @@ -1935,7 +1935,7 @@ pub async fn remove_container(container_id: &str) -> Result<(), String> { "Removing container {} (v=false: named volumes such as claude config are preserved)", container_id ); - docker + match docker .remove_container( container_id, Some(RemoveContainerOptions { @@ -1945,7 +1945,17 @@ pub async fn remove_container(container_id: &str) -> Result<(), String> { }), ) .await - .map_err(|e| format!("Failed to remove container: {}", e)) + { + Ok(()) => Ok(()), + // Already gone is the outcome this call wants, not a failure — a + // caller retrying a leftover from a previous, partially-failed removal + // (see `remove_project`) must not be told it failed forever just + // because a *different* attempt already succeeded. + Err(bollard::errors::Error::DockerResponseServerError { + status_code: 404, .. + }) => Ok(()), + Err(e) => Err(format!("Failed to remove container: {}", e)), + } } /// Return the snapshot image name for a project. @@ -3496,13 +3506,27 @@ async fn rewrite_image_without_secrets( } /// Remove the snapshot image for a project (used on Reset / project removal). +/// +/// A project that never started never built a snapshot, so "no such image" is +/// the ordinary case, not a failure — it is treated the same as success and +/// logged at most at `info`. A real failure (the image is in use, a +/// permission error, the daemon dropped the connection) is the one thing this +/// returns `Err` for, and callers must not throw that away: see the +/// `remove_project` doc comment on `ProjectRemovalReport` for why an +/// unreported failure here used to make the resource unreachable forever. pub async fn remove_snapshot_image(project: &Project) -> Result<(), String> { - let docker = get_docker()?; - let image_name = get_snapshot_image_name(project); + remove_image_by_name(&get_snapshot_image_name(project)).await +} - docker +/// Remove a Docker image by name/tag, treating "does not exist" as success. +/// Shared by [`remove_snapshot_image`] and the pending-cleanup retry, which +/// only has the image name (the project record is already gone by then). +pub async fn remove_image_by_name(image_name: &str) -> Result<(), String> { + let docker = get_docker()?; + + match docker .remove_image( - &image_name, + image_name, Some(RemoveImageOptions { force: true, noprune: false, @@ -3510,25 +3534,69 @@ pub async fn remove_snapshot_image(project: &Project) -> Result<(), String> { None, ) .await - .map_err(|e| format!("Failed to remove snapshot image {}: {}", image_name, e))?; - - log::info!("Removed snapshot image {}", image_name); - Ok(()) + { + Ok(_) => { + log::info!("Removed snapshot image {}", image_name); + Ok(()) + } + Err(bollard::errors::Error::DockerResponseServerError { + status_code: 404, .. + }) => Ok(()), + Err(e) => Err(format!("Failed to remove snapshot image {}: {}", image_name, e)), + } } /// Remove both named volumes for a project (used on Reset / project removal). -pub async fn remove_project_volumes(project: &Project) -> Result<(), String> { - let docker = get_docker()?; - for vol in [ +/// +/// Returns the names of volumes that still exist afterwards — empty means +/// both are gone (removed here, or never created). This used to always +/// return `Ok(())` regardless of what actually happened, which made the +/// `if let Err(e)` at every call site unreachable by construction; see +/// triple-c#31. A volume Docker reports as simply not existing is not a +/// leftover and is not included. +pub async fn remove_project_volumes(project: &Project) -> Vec { + remove_volumes_by_name(&[ home_volume_name(&project.id), config_volume_name(&project.id), - ] { - match docker.remove_volume(&vol, None).await { + ]) + .await +} + +/// Remove a set of named volumes, treating "does not exist" as success. +/// Returns the names that still exist afterwards — empty means every one is +/// gone (removed here, or never created). +/// +/// Shared by [`remove_project_volumes`] and the pending-cleanup retry, the +/// latter calling this with whatever the former could not remove the first +/// time. Used to always report success regardless of what actually happened, +/// which made every `if let Err(e)` at its call sites unreachable by +/// construction; see triple-c#31. +pub async fn remove_volumes_by_name(names: &[String]) -> Vec { + let docker = match get_docker() { + Ok(d) => d, + Err(e) => { + // Can't reach the daemon to even try, so nothing here can be + // confirmed removed. Reporting all as leftover is the safe + // direction: worst case a later retry finds them already gone. + log::warn!("Could not remove volumes {:?}: {}", names, e); + return names.to_vec(); + } + }; + + let mut leftover = Vec::new(); + for vol in names { + match docker.remove_volume(vol, None).await { Ok(_) => log::info!("Removed volume {}", vol), - Err(e) => log::warn!("Failed to remove volume {} (may not exist): {}", vol, e), + Err(bollard::errors::Error::DockerResponseServerError { + status_code: 404, .. + }) => {} + Err(e) => { + log::warn!("Failed to remove volume {}: {}", vol, e); + leftover.push(vol.clone()); + } } } - Ok(()) + leftover } /// Check whether the existing container's configuration still matches the diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 24b6bae..b9a92e6 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -257,6 +257,10 @@ pub fn run() { log::info!("Startup housekeeping dropped {} stale rollback pin(s)", reaped); } crate::docker::sweep_orphaned_snapshots_logged("startup").await; + // A container/image/volume `remove_project` could not delete + // is recorded rather than lost — see triple-c#31 — and this is + // the only place anything ever retries it. + crate::commands::project_commands::retry_pending_cleanup_logged().await; }); // Auto-start web terminal server if enabled in settings diff --git a/app/src-tauri/src/models/project.rs b/app/src-tauri/src/models/project.rs index dfa8f38..f17747b 100644 --- a/app/src-tauri/src/models/project.rs +++ b/app/src-tauri/src/models/project.rs @@ -422,6 +422,31 @@ pub enum ProjectStatus { Error, } +/// What `remove_project` could not delete, named so the UI can say so instead +/// of reporting a clean removal that was not one. +/// +/// The project record is dropped from `projects.json` regardless — see the +/// long comment on `remove_project` for why refusing is not the answer — but +/// anything named here is also written to a pending-cleanup record that +/// startup housekeeping retries, so it stays reachable after the project it +/// belonged to no longer exists. +#[derive(Debug, Default, Clone, Serialize, Deserialize)] +pub struct ProjectRemovalReport { + /// The project's container, if it could not be removed. + pub container: Option, + /// The `triple-c-snapshot-{id}` image, if it could not be removed. + pub image: Option, + /// Named volumes (home, claude config) that could not be removed. + pub volumes: Vec, +} + +impl ProjectRemovalReport { + /// True when nothing was left behind. + pub fn is_clean(&self) -> bool { + self.container.is_none() && self.image.is_none() && self.volumes.is_empty() + } +} + /// Which AI model backend/provider the project uses. /// - `Anthropic`: Direct Anthropic API (user runs `claude login` inside the container) /// - `Bedrock`: AWS Bedrock with per-project AWS credentials @@ -650,6 +675,25 @@ impl Project { mod tests { use super::*; + // ── ProjectRemovalReport ──────────────────────────────────────────────── + + #[test] + fn a_report_is_clean_only_with_nothing_left_behind() { + assert!(ProjectRemovalReport::default().is_clean()); + + let mut r = ProjectRemovalReport::default(); + r.container = Some("abc123".to_string()); + assert!(!r.is_clean(), "a leftover container must not read as clean"); + + let mut r = ProjectRemovalReport::default(); + r.image = Some("triple-c-snapshot-x:latest".to_string()); + assert!(!r.is_clean(), "a leftover image must not read as clean"); + + let mut r = ProjectRemovalReport::default(); + r.volumes.push("triple-c-home-x".to_string()); + assert!(!r.is_clean(), "a leftover volume must not read as clean"); + } + // ── Custom environment variable names ───────────────────────────────── #[test] diff --git a/app/src-tauri/src/storage/mod.rs b/app/src-tauri/src/storage/mod.rs index 1559e3e..151759b 100644 --- a/app/src-tauri/src/storage/mod.rs +++ b/app/src-tauri/src/storage/mod.rs @@ -1,4 +1,5 @@ pub mod migration_store; +pub mod pending_cleanup; pub mod projects_store; pub mod secure; pub mod settings_store; diff --git a/app/src-tauri/src/storage/pending_cleanup.rs b/app/src-tauri/src/storage/pending_cleanup.rs new file mode 100644 index 0000000..0045ca6 --- /dev/null +++ b/app/src-tauri/src/storage/pending_cleanup.rs @@ -0,0 +1,217 @@ +//! Host-side record of Docker resources `remove_project` could not delete. +//! +//! `remove_project` drops a project's id from `projects.json` unconditionally +//! — see the comment on `ProjectRemovalReport` — so once that happens nothing +//! in the app can name the leftover container, image or volume again by any +//! path a user can reach. This is what keeps it reachable anyway: one JSON +//! file per affected project under `/triple-c/pending-cleanup/`, +//! written *before* the project record is dropped. Startup housekeeping +//! retries every record on the next launch (see +//! `commands::project_commands::retry_pending_cleanup_logged`) and deletes +//! the ones that fully succeed. +//! +//! Same write-temp-then-rename shape as `projects.json` and the migration +//! store, and the same per-project-file layout as +//! `storage::migration_store` — a stuck cleanup record for one project must +//! never block the retry of another's. + +use std::fs; +use std::path::PathBuf; + +use serde::{Deserialize, Serialize}; + +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct PendingCleanup { + pub project_id: String, + /// Kept only so a log line or a future UI can name the project without a + /// second lookup — the project record itself is already gone by the time + /// this is read back. + pub project_name: String, + pub container_id: Option, + pub image: Option, + pub volumes: Vec, + pub recorded_at: String, +} + +impl PendingCleanup { + /// True once nothing named here still needs to be removed. + pub fn is_empty(&self) -> bool { + self.container_id.is_none() && self.image.is_none() && self.volumes.is_empty() + } +} + +/// `/triple-c/pending-cleanup`, created on demand. +fn dir() -> Result { + let dir = dirs::data_dir() + .ok_or_else(|| { + "Could not determine data directory. Set XDG_DATA_HOME on Linux.".to_string() + })? + .join("triple-c") + .join("pending-cleanup"); + fs::create_dir_all(&dir) + .map_err(|e| format!("Failed to create pending-cleanup directory: {}", e))?; + Ok(dir) +} + +/// Project ids are UUIDs, but they arrive over IPC, so refuse to let one steer +/// the write anywhere but the pending-cleanup directory. Mirrors +/// `storage::migration_store::sanitize`. +fn sanitize(project_id: &str) -> String { + project_id + .chars() + .map(|c| if c.is_ascii_alphanumeric() || c == '-' || c == '_' { c } else { '_' }) + .collect() +} + +fn path_for(project_id: &str) -> Result { + Ok(dir()?.join(format!("{}.json", sanitize(project_id)))) +} + +/// Write (or overwrite) a project's pending-cleanup record. +pub fn save(record: &PendingCleanup) -> Result<(), String> { + let path = path_for(&record.project_id)?; + let data = serde_json::to_string_pretty(record) + .map_err(|e| format!("Failed to serialize pending cleanup record: {}", e))?; + let tmp = path.with_extension("json.tmp"); + fs::write(&tmp, data).map_err(|e| format!("Failed to write pending cleanup record: {}", e))?; + fs::rename(&tmp, &path).map_err(|e| format!("Failed to commit pending cleanup record: {}", e)) +} + +/// Remove a project's pending-cleanup record. Missing is success — this is +/// how a fully-succeeded retry (or a record that never existed) is expressed. +pub fn clear(project_id: &str) -> Result<(), String> { + let path = path_for(project_id)?; + match fs::remove_file(&path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(format!("Failed to remove pending cleanup record: {}", e)), + } +} + +/// Every pending-cleanup record on disk. An unparseable file is logged and +/// skipped rather than blocking every other project's retry — the same +/// "one bad record can't wedge the rest" reasoning as the migration store. +pub fn list() -> Vec { + let Ok(dir) = dir() else { return Vec::new() }; + let Ok(entries) = fs::read_dir(&dir) else { return Vec::new() }; + + entries + .flatten() + .filter(|e| e.path().extension().is_some_and(|ext| ext == "json")) + .filter_map(|e| { + let path = e.path(); + let data = fs::read_to_string(&path).ok()?; + match serde_json::from_str::(&data) { + Ok(record) => Some(record), + Err(err) => { + log::warn!( + "Could not parse pending cleanup record {}: {} — skipping it this run", + path.display(), + err + ); + None + } + } + }) + .collect() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn temp_data_dir(name: &str) -> PathBuf { + std::env::temp_dir().join(format!("triple-c-pending-cleanup-{}-{}", name, uuid::Uuid::new_v4().simple())) + } + + fn record(project_id: &str) -> PendingCleanup { + PendingCleanup { + project_id: project_id.to_string(), + project_name: "Some Project".to_string(), + container_id: Some("abc123".to_string()), + image: Some("triple-c-snapshot-abc:latest".to_string()), + volumes: vec!["triple-c-home-abc".to_string()], + recorded_at: "2026-08-25T00:00:00Z".to_string(), + } + } + + /// `list`/`save`/`clear` go through `dirs::data_dir()`, so these exercise + /// the pure parts directly against a temp directory rather than the real + /// one — same approach `migration_store`'s tests take for `sanitize`. + #[test] + fn project_ids_cannot_escape_the_pending_cleanup_directory() { + assert_eq!(sanitize("../../etc/passwd"), "______etc_passwd"); + assert_eq!(sanitize("a/b"), "a_b"); + assert_eq!( + sanitize("ab62cd24-51aa-4645-8f5c-17a124062050"), + "ab62cd24-51aa-4645-8f5c-17a124062050" + ); + } + + #[test] + fn is_empty_reflects_whatever_still_needs_removing() { + let mut r = record("p1"); + assert!(!r.is_empty()); + + r.container_id = None; + r.image = None; + assert!(!r.is_empty(), "a leftover volume alone still counts"); + + r.volumes.clear(); + assert!(r.is_empty()); + } + + /// Save-then-list-then-clear against a real (temp) directory, bypassing + /// `dir()`'s hardcoded `dirs::data_dir()` join by writing/reading the + /// files directly the way `save`/`list` do internally. + #[test] + fn a_saved_record_round_trips_and_clearing_removes_it() { + let dir = temp_data_dir("roundtrip"); + fs::create_dir_all(&dir).unwrap(); + let rec = record("proj-1"); + let path = dir.join(format!("{}.json", sanitize(&rec.project_id))); + + let data = serde_json::to_string_pretty(&rec).unwrap(); + fs::write(&path, data).unwrap(); + + let loaded: PendingCleanup = + serde_json::from_str(&fs::read_to_string(&path).unwrap()).unwrap(); + assert_eq!(loaded.project_id, "proj-1"); + assert_eq!(loaded.volumes, vec!["triple-c-home-abc".to_string()]); + + fs::remove_file(&path).unwrap(); + assert!(!path.exists()); + + fs::remove_dir_all(&dir).ok(); + } + + /// A record that fails to parse must not poison the rest of the listing. + #[test] + fn an_unparseable_record_is_skipped_not_fatal() { + let dir = temp_data_dir("corrupt"); + fs::create_dir_all(&dir).unwrap(); + fs::write(dir.join("bad.json"), "{ not json").unwrap(); + let good = record("proj-2"); + fs::write( + dir.join("proj-2.json"), + serde_json::to_string_pretty(&good).unwrap(), + ) + .unwrap(); + + let mut found = Vec::new(); + for entry in fs::read_dir(&dir).unwrap().flatten() { + let path = entry.path(); + if path.extension().is_some_and(|ext| ext == "json") { + if let Ok(data) = fs::read_to_string(&path) { + if let Ok(r) = serde_json::from_str::(&data) { + found.push(r); + } + } + } + } + assert_eq!(found.len(), 1); + assert_eq!(found[0].project_id, "proj-2"); + + fs::remove_dir_all(&dir).ok(); + } +} diff --git a/app/src/components/projects/home/ProjectHome.tsx b/app/src/components/projects/home/ProjectHome.tsx index 9245f4d..a83b04a 100644 --- a/app/src/components/projects/home/ProjectHome.tsx +++ b/app/src/components/projects/home/ProjectHome.tsx @@ -1,5 +1,6 @@ import { useEffect, useMemo, useState } from "react"; import { useShallow } from "zustand/react/shallow"; +import type { ProjectRemovalReport } from "../../../lib/types"; import { useAppState } from "../../../store/appState"; import { useProjectActions } from "../../../hooks/useProjectActions"; import { useProjects } from "../../../hooks/useProjects"; @@ -30,6 +31,16 @@ const TABS = [ export type ProjectHomeTabId = (typeof TABS)[number]["id"]; +/** Names what a `ProjectRemovalReport` says survived, for the leftover toast. */ +function describeLeftovers(report: ProjectRemovalReport): string { + const parts: string[] = []; + if (report.container) parts.push("its container"); + if (report.image) parts.push("its saved image"); + if (report.volumes.length === 1) parts.push("a volume"); + else if (report.volumes.length > 1) parts.push(`${report.volumes.length} volumes`); + return parts.join(", "); +} + interface Props { projectId: string; active: boolean; @@ -282,7 +293,14 @@ export default function ProjectHome({ projectId, active }: Props) { onConfirm={async () => { setConfirmRemove(false); try { - await remove(project.id); + const report = await remove(project.id); + if (report.container || report.image || report.volumes.length > 0) { + useAppState.getState().pushToast({ + kind: "info", + message: `“${project.name}” was removed, but some Docker resources are still on disk`, + detail: `Triple-C will retry removing ${describeLeftovers(report)} the next time it starts.`, + }); + } } catch (e) { useAppState.getState().pushToast({ kind: "error", diff --git a/app/src/hooks/useProjects.ts b/app/src/hooks/useProjects.ts index 66868a0..2ebcb5b 100644 --- a/app/src/hooks/useProjects.ts +++ b/app/src/hooks/useProjects.ts @@ -44,8 +44,9 @@ export function useProjects() { const remove = useCallback( async (id: string) => { - await commands.removeProject(id); + const report = await commands.removeProject(id); removeProjectFromList(id); + return report; }, [removeProjectFromList], ); diff --git a/app/src/lib/tauri-commands.ts b/app/src/lib/tauri-commands.ts index f9bb0fe..3264818 100644 --- a/app/src/lib/tauri-commands.ts +++ b/app/src/lib/tauri-commands.ts @@ -1,5 +1,5 @@ import { invoke } from "@tauri-apps/api/core"; -import type { Project, ProjectPath, ContainerInfo, AppSettings, UpdateInfo, ImageUpdateInfo, FileEntry, FileContents, WebTerminalInfo, SttStatus, GatewayStatus, InstallOptions, ClaudeSession, ContainerCapabilities, ScheduledTask, ScheduledTaskInput, SchedulerNotification, AuthBridgeStatus, BrowserViewStatus, BrowserViewPopoutState, BrowserPageState, PlaywrightDetection, BrowserSetupOutcome, BrowserInstallTarget, ContainerStaleness, MigrationOptions, MigrationReport, MigrationState, ClearTokenOutcome, CaCertInfo, UploadOutcome } from "./types"; +import type { Project, ProjectPath, ProjectRemovalReport, ContainerInfo, AppSettings, UpdateInfo, ImageUpdateInfo, FileEntry, FileContents, WebTerminalInfo, SttStatus, GatewayStatus, InstallOptions, ClaudeSession, ContainerCapabilities, ScheduledTask, ScheduledTaskInput, SchedulerNotification, AuthBridgeStatus, BrowserViewStatus, BrowserViewPopoutState, BrowserPageState, PlaywrightDetection, BrowserSetupOutcome, BrowserInstallTarget, ContainerStaleness, MigrationOptions, MigrationReport, MigrationState, ClearTokenOutcome, CaCertInfo, UploadOutcome } from "./types"; // Docker export const checkDocker = () => invoke("check_docker"); @@ -13,7 +13,7 @@ export const listProjects = () => invoke("list_projects"); export const addProject = (name: string, paths: ProjectPath[]) => invoke("add_project", { name, paths }); export const removeProject = (projectId: string) => - invoke("remove_project", { projectId }); + invoke("remove_project", { projectId }); export const updateProject = (project: Project) => invoke("update_project", { project }); export const startProjectContainer = (projectId: string) => diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index cc394d6..a84e85a 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -77,6 +77,15 @@ export type ProjectStatus = | "stopping" | "error"; +/** What `removeProject` could not delete. The project is removed from the + * sidebar either way; anything named here is recorded on the host and + * retried automatically the next time the app starts. */ +export interface ProjectRemovalReport { + container: string | null; + image: string | null; + volumes: string[]; +} + export type Backend = | "anthropic" | "bedrock" -- 2.52.0 From d8bb5ab2628d66f9d3ea4dad15d31670d3d25412 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 27 Aug 2026 08:36:27 -0700 Subject: [PATCH 2/4] Address review findings: durability, stale container ids, honest toasts MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An Opus review of the previous commit found several real gaps: - pending_cleanup::save used plain write-temp-then-rename, unlike migration_store's fsync'd write it claimed to mirror — a crash in that window left a truncated record that list() would skip forever, silently reproducing the exact bug this module exists to fix. Now matches migration_store's File::create/write_all/sync_all/rename/sync_dir shape, and the tests exercise the real save/list/clear functions against a temp dir instead of re-implementing their bodies inline. - remove_project and rebuild_project_container only ever looked at project.container_id, unlike every other container-destroying path in the codebase, which falls back to find_existing_container for exactly this race (a crash between creating a container and persisting its id). A miss here left a container that then blocked every subsequent volume removal with a 409, forever. Both now resolve the same way the rest of the codebase does, and record the container by its deterministic name rather than its id so a retry still has something that resolves. - remove_project's toast promised an automatic retry unconditionally, even when writing the pending-cleanup record itself failed (the one case where nothing will actually retry). ProjectRemovalReport now carries retry_scheduled, and the UI is honest about which case it's in. - remove_volumes_by_name now retries once after a short delay on a 409, since Docker releasing a volume's mount reference right after its container is removed is not always instantaneous, and this is exactly the sequence remove_project runs. - rebuild_project_container (Reset) returns ProjectResetOutcome so the UI can warn when Reset could not fully clear a project's volumes, instead of only logging it — the new container silently reuses old data otherwise, which is what Reset promises not to do. - retry_pending_cleanup_logged escalates a record's log level after it has failed for a week, since recorded_at was otherwise write-only. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FGjXq6fqtAFHdbhk4f3PfZ --- .../src/commands/project_commands.rs | 94 ++++++-- app/src-tauri/src/docker/container.rs | 24 +- app/src-tauri/src/models/project.rs | 27 ++- app/src-tauri/src/storage/pending_cleanup.rs | 217 +++++++++++++----- .../components/projects/home/ProjectHome.tsx | 34 ++- app/src/hooks/useProjectActions.ts | 23 +- app/src/hooks/useProjects.ts | 6 +- app/src/lib/tauri-commands.ts | 4 +- app/src/lib/types.ts | 22 +- 9 files changed, 351 insertions(+), 100 deletions(-) diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 0f0ce5d..27a1e5f 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -2,7 +2,7 @@ use tauri::{Emitter, State}; use crate::commands::aws_commands; use crate::docker; -use crate::models::{container_config, AppSettings, Backend, BedrockAuthMethod, Project, ProjectPath, ProjectRemovalReport, ProjectStatus}; +use crate::models::{container_config, AppSettings, Backend, BedrockAuthMethod, Project, ProjectPath, ProjectRemovalReport, ProjectResetOutcome, ProjectStatus}; use crate::storage::secure; use crate::AppState; @@ -730,7 +730,21 @@ pub async fn remove_project( let existing_project = state.projects_store.get(&project_id); if let Some(ref project) = existing_project { - if let Some(ref container_id) = project.container_id { + // `project.container_id` can be `None` or stale — a crash between + // creating a container and persisting its id is the same race every + // other destroyer of a project's container already guards against + // with `find_existing_container` (`start_project_container`, + // migration's recreate paths). Removal is the one place that + // mattered least before this fix, because a container `remove_project` + // missed just sat there; now a miss here poisons the volume removal + // right after it (Docker refuses to delete a volume a container still + // references) and mints a pending-cleanup record for volumes with no + // way to name the container actually blocking them. + let container_ref = match &project.container_id { + Some(id) => Some(id.clone()), + None => docker::find_existing_container(project).await.ok().flatten(), + }; + if let Some(ref container_id) = container_ref { state.exec_manager.close_sessions_for_container(container_id).await; let _ = docker::stop_container(container_id).await; if let Err(e) = docker::remove_container(container_id).await { @@ -738,7 +752,11 @@ pub async fn remove_project( "Failed to remove container {} for project {}: {}", container_id, project_id, e ); - report.container = Some(container_id.clone()); + // Recorded by name, not id: the name is the stable handle a + // later retry can still resolve (Docker's remove-container + // call accepts either), and it is what `container_ref` above + // falls back to finding in the first place. + report.container = Some(project.container_name()); } } @@ -770,17 +788,23 @@ pub async fn remove_project( recorded_at: chrono::Utc::now().to_rfc3339(), }; match crate::storage::pending_cleanup::save(&record) { - Ok(()) => log::warn!( - "Project {} removed with Docker resources still present: {:?} — recorded for \ - automatic retry on next launch", - project_id, report - ), - Err(e) => log::error!( - "Project {} removed with Docker resources still present ({:?}), and the \ - pending-cleanup record could not be written ({}) — nothing will retry removing \ - them", - project_id, report, e - ), + Ok(()) => { + report.retry_scheduled = true; + log::warn!( + "Project {} removed with Docker resources still present: {:?} — recorded for \ + automatic retry on next launch", + project_id, report + ); + } + Err(e) => { + report.retry_scheduled = false; + log::error!( + "Project {} removed with Docker resources still present ({:?}), and the \ + pending-cleanup record could not be written ({}) — nothing will retry \ + removing them", + project_id, report, e + ); + } } } @@ -852,6 +876,26 @@ pub async fn retry_pending_cleanup_logged() { cleaned += 1; } else { still_pending += 1; + // `recorded_at` is otherwise write-only — nothing read it back, + // which is exactly the shape `storage::migration_store` calls out + // as a bug in its own history ("nothing ever removed them"). A + // record that has failed every retry for a week is no longer + // routine: escalate the log level so it is not indistinguishable + // from one seen for the first time. + let age = chrono::DateTime::parse_from_rfc3339(&record.recorded_at) + .ok() + .map(|t| chrono::Utc::now().signed_duration_since(t.with_timezone(&chrono::Utc))); + match age { + Some(age) if age > chrono::Duration::days(PENDING_CLEANUP_STALE_AFTER_DAYS) => { + log::error!( + "Pending cleanup for project {} ({}) has not succeeded in over {} days: \ + {:?} — this may need a manual `docker volume rm` / `docker rmi` / \ + `docker rm`", + record.project_id, record.project_name, PENDING_CLEANUP_STALE_AFTER_DAYS, record + ); + } + _ => {} + } if let Err(e) = crate::storage::pending_cleanup::save(&record) { log::warn!( "Could not update pending cleanup record for project {} ({}): {}", @@ -867,6 +911,11 @@ pub async fn retry_pending_cleanup_logged() { ); } +/// After this many days of a pending-cleanup record failing every retry, +/// `retry_pending_cleanup_logged` escalates its log line from `warn` to +/// `error` — see the comment at its call site. +const PENDING_CLEANUP_STALE_AFTER_DAYS: i64 = 7; + #[tauri::command] pub async fn update_project( project: serde_json::Value, @@ -1341,7 +1390,7 @@ pub async fn rebuild_project_container( project_id: String, app_handle: tauri::AppHandle, state: State<'_, AppState>, -) -> Result { +) -> Result { // Reset deletes both volumes and the snapshot image. Doing that while a // migration is mid-flight pulls the ground out from under it and leaves an // orphan migration record pointing at images that no longer exist — and @@ -1368,8 +1417,16 @@ pub async fn rebuild_project_container( // `start_project_container` below re-arms it against the new one. state.auth_bridge.stop(&project_id).await; - // Remove existing container - if let Some(ref container_id) = project.container_id { + // Remove existing container. Resolved the same way `remove_project` now + // is — `project.container_id` can be `None` or stale — because a + // container this misses blocks the volume removal immediately below with + // a 409, and Reset silently keeping the old volumes is exactly the bug + // this whole change is closing. + let container_ref = match &project.container_id { + Some(id) => Some(id.clone()), + None => docker::find_existing_container(&project).await.ok().flatten(), + }; + if let Some(ref container_id) = container_ref { state.exec_manager.close_sessions_for_container(container_id).await; let _ = docker::stop_container(container_id).await; docker::remove_container(container_id).await?; @@ -1397,7 +1454,8 @@ pub async fn rebuild_project_container( // Start fresh. The locked variant, because `_guard` above is this project's // claim and the public command would be refused by it. - start_project_container_locked(project_id, app_handle, state).await + let project = start_project_container_locked(project_id, app_handle, state).await?; + Ok(ProjectResetOutcome { project, leftover_volumes }) } /// Reconcile project statuses against actual Docker container state. diff --git a/app/src-tauri/src/docker/container.rs b/app/src-tauri/src/docker/container.rs index 39dd9ff..038e837 100644 --- a/app/src-tauri/src/docker/container.rs +++ b/app/src-tauri/src/docker/container.rs @@ -3585,7 +3585,7 @@ pub async fn remove_volumes_by_name(names: &[String]) -> Vec { let mut leftover = Vec::new(); for vol in names { - match docker.remove_volume(vol, None).await { + match remove_one_volume_with_retry(&docker, vol).await { Ok(_) => log::info!("Removed volume {}", vol), Err(bollard::errors::Error::DockerResponseServerError { status_code: 404, .. @@ -3599,6 +3599,28 @@ pub async fn remove_volumes_by_name(names: &[String]) -> Vec { leftover } +/// Remove one volume, retrying once after a short delay on a 409 ("volume is +/// in use"). Docker releasing a volume's mount reference after the container +/// using it is removed is not always instantaneous, so the very first call +/// site of this — `remove_project`, whose container removal lands +/// immediately before its volume removal — could otherwise turn an ordinary +/// race into a permanent pending-cleanup record and an alarming toast for +/// something that would have cleared itself half a second later. +async fn remove_one_volume_with_retry( + docker: &bollard::Docker, + name: &str, +) -> Result<(), bollard::errors::Error> { + match docker.remove_volume(name, None).await { + Err(bollard::errors::Error::DockerResponseServerError { + status_code: 409, .. + }) => { + tokio::time::sleep(std::time::Duration::from_millis(500)).await; + docker.remove_volume(name, None).await + } + other => other, + } +} + /// Check whether the existing container's configuration still matches the /// current project settings. Returns `true` when the container must be /// recreated (mounts or env vars differ). diff --git a/app/src-tauri/src/models/project.rs b/app/src-tauri/src/models/project.rs index f17747b..c6b09c1 100644 --- a/app/src-tauri/src/models/project.rs +++ b/app/src-tauri/src/models/project.rs @@ -432,12 +432,21 @@ pub enum ProjectStatus { /// belonged to no longer exists. #[derive(Debug, Default, Clone, Serialize, Deserialize)] pub struct ProjectRemovalReport { - /// The project's container, if it could not be removed. + /// The project's container, if it could not be removed. Named by its + /// deterministic `triple-c-{id}` name (see `Project::container_name`), + /// not the container id, since the id can be stale or absent and the + /// name is what a later retry can still resolve. pub container: Option, /// The `triple-c-snapshot-{id}` image, if it could not be removed. pub image: Option, /// Named volumes (home, claude config) that could not be removed. pub volumes: Vec, + /// True once the leftovers above were durably recorded for automatic + /// retry on the next launch. False means the pending-cleanup record + /// itself could not be written — nothing will retry these, and the UI + /// must say so rather than promising a retry that will not happen. + /// Meaningless (and left at its default) when `is_clean()` is true. + pub retry_scheduled: bool, } impl ProjectRemovalReport { @@ -447,6 +456,22 @@ impl ProjectRemovalReport { } } +/// What `rebuild_project_container` (Reset) produced: the project as it +/// stands after restarting, and any volume Reset could not clear. +/// +/// Reset's contract is "back to a clean base image", so a leftover volume +/// here is reused as-is by the container this creates — the opposite of what +/// was asked for — and unlike [`ProjectRemovalReport`] there is no +/// pending-cleanup record for it: the project id survives Reset, so a later +/// Reset attempt can retry the same volume itself. +#[derive(Debug, Clone, Serialize)] +pub struct ProjectResetOutcome { + pub project: Project, + /// Volumes that survived Reset and were mounted into the new container + /// unchanged. + pub leftover_volumes: Vec, +} + /// Which AI model backend/provider the project uses. /// - `Anthropic`: Direct Anthropic API (user runs `claude login` inside the container) /// - `Bedrock`: AWS Bedrock with per-project AWS credentials diff --git a/app/src-tauri/src/storage/pending_cleanup.rs b/app/src-tauri/src/storage/pending_cleanup.rs index 0045ca6..f83bc75 100644 --- a/app/src-tauri/src/storage/pending_cleanup.rs +++ b/app/src-tauri/src/storage/pending_cleanup.rs @@ -10,13 +10,21 @@ //! `commands::project_commands::retry_pending_cleanup_logged`) and deletes //! the ones that fully succeed. //! -//! Same write-temp-then-rename shape as `projects.json` and the migration -//! store, and the same per-project-file layout as -//! `storage::migration_store` — a stuck cleanup record for one project must -//! never block the retry of another's. +//! **This record is written in the same instant its record in `projects.json` +//! is destroyed, and it is the only remaining handle on the leftover +//! resource** — which is a stronger claim on durability than an ordinary +//! write-temp-then-rename gives. `storage::migration_store::save` carries the +//! same reasoning for the migration state file: `fs::write` returns once the +//! bytes are in the page cache, and a rename over them is atomic with respect +//! to other readers, not to power loss. A crash in that window leaves the +//! rename applied and the data half-written, which [`list`] then treats as +//! unparseable and skips — reproducing the exact bug this module exists to +//! close, silently, with only a startup log line as evidence. So `save` here +//! takes the same `File::create` → `write_all` → `sync_all` → `rename` → +//! directory-sync shape `migration_store` does. use std::fs; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use serde::{Deserialize, Serialize}; @@ -27,6 +35,11 @@ pub struct PendingCleanup { /// second lookup — the project record itself is already gone by the time /// this is read back. pub project_name: String, + /// The project's container, if it could not be removed. Named by its + /// deterministic `triple-c-{id}` name rather than the (possibly stale) + /// container id Docker handed out — Docker's remove-container API + /// accepts either, and the name is the one identifier guaranteed to still + /// resolve to the same container by the time a retry runs. pub container_id: Option, pub image: Option, pub volumes: Vec, @@ -63,29 +76,15 @@ fn sanitize(project_id: &str) -> String { .collect() } -fn path_for(project_id: &str) -> Result { - Ok(dir()?.join(format!("{}.json", sanitize(project_id)))) -} - /// Write (or overwrite) a project's pending-cleanup record. pub fn save(record: &PendingCleanup) -> Result<(), String> { - let path = path_for(&record.project_id)?; - let data = serde_json::to_string_pretty(record) - .map_err(|e| format!("Failed to serialize pending cleanup record: {}", e))?; - let tmp = path.with_extension("json.tmp"); - fs::write(&tmp, data).map_err(|e| format!("Failed to write pending cleanup record: {}", e))?; - fs::rename(&tmp, &path).map_err(|e| format!("Failed to commit pending cleanup record: {}", e)) + save_in(&dir()?, record) } /// Remove a project's pending-cleanup record. Missing is success — this is /// how a fully-succeeded retry (or a record that never existed) is expressed. pub fn clear(project_id: &str) -> Result<(), String> { - let path = path_for(project_id)?; - match fs::remove_file(&path) { - Ok(()) => Ok(()), - Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), - Err(e) => Err(format!("Failed to remove pending cleanup record: {}", e)), - } + clear_in(&dir()?, project_id) } /// Every pending-cleanup record on disk. An unparseable file is logged and @@ -93,7 +92,50 @@ pub fn clear(project_id: &str) -> Result<(), String> { /// "one bad record can't wedge the rest" reasoning as the migration store. pub fn list() -> Vec { let Ok(dir) = dir() else { return Vec::new() }; - let Ok(entries) = fs::read_dir(&dir) else { return Vec::new() }; + list_in(&dir) +} + +fn path_in(dir: &Path, project_id: &str) -> PathBuf { + dir.join(format!("{}.json", sanitize(project_id))) +} + +/// Durable write: fsync the file before the rename, and fsync the directory +/// after it — see the module doc comment for why a plain +/// write-temp-then-rename is not enough here. Mirrors +/// `storage::migration_store::save`/`sync_dir`. +fn save_in(dir: &Path, record: &PendingCleanup) -> Result<(), String> { + let path = path_in(dir, &record.project_id); + let data = serde_json::to_string_pretty(record) + .map_err(|e| format!("Failed to serialize pending cleanup record: {}", e))?; + let tmp = path.with_extension("json.tmp"); + + { + use std::io::Write; + let mut file = fs::File::create(&tmp) + .map_err(|e| format!("Failed to write pending cleanup record: {}", e))?; + file.write_all(data.as_bytes()) + .map_err(|e| format!("Failed to write pending cleanup record: {}", e))?; + file.sync_all() + .map_err(|e| format!("Failed to flush pending cleanup record to disk: {}", e))?; + } + + fs::rename(&tmp, &path) + .map_err(|e| format!("Failed to commit pending cleanup record: {}", e))?; + sync_dir(&path); + Ok(()) +} + +fn clear_in(dir: &Path, project_id: &str) -> Result<(), String> { + let path = path_in(dir, project_id); + match fs::remove_file(&path) { + Ok(()) => Ok(()), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(format!("Failed to remove pending cleanup record: {}", e)), + } +} + +fn list_in(dir: &Path) -> Vec { + let Ok(entries) = fs::read_dir(dir) else { return Vec::new() }; entries .flatten() @@ -116,28 +158,50 @@ pub fn list() -> Vec { .collect() } +/// fsync the directory holding `path`, so a rename into it survives power +/// loss. Best effort only on the platforms where it is meaningless: Windows +/// has no directory handle to sync and errors on the attempt, so failure is +/// logged rather than propagated — the file's own `sync_all` above is what +/// carries the data. Mirrors `storage::migration_store::sync_dir`, which is +/// private to that module, so this is a small deliberate duplicate rather +/// than a shared dependency between two otherwise-independent stores. +fn sync_dir(path: &Path) { + let Some(dir) = path.parent() else { return }; + match fs::File::open(dir).and_then(|d| d.sync_all()) { + Ok(()) => {} + Err(e) => log::debug!( + "Could not fsync the pending-cleanup directory {}: {} — the record itself was flushed", + dir.display(), + e + ), + } +} + #[cfg(test)] mod tests { use super::*; - fn temp_data_dir(name: &str) -> PathBuf { - std::env::temp_dir().join(format!("triple-c-pending-cleanup-{}-{}", name, uuid::Uuid::new_v4().simple())) + fn temp_dir(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "triple-c-pending-cleanup-{}-{}", + name, + uuid::Uuid::new_v4().simple() + )); + fs::create_dir_all(&dir).unwrap(); + dir } fn record(project_id: &str) -> PendingCleanup { PendingCleanup { project_id: project_id.to_string(), project_name: "Some Project".to_string(), - container_id: Some("abc123".to_string()), + container_id: Some("triple-c-abc".to_string()), image: Some("triple-c-snapshot-abc:latest".to_string()), volumes: vec!["triple-c-home-abc".to_string()], recorded_at: "2026-08-25T00:00:00Z".to_string(), } } - /// `list`/`save`/`clear` go through `dirs::data_dir()`, so these exercise - /// the pure parts directly against a temp directory rather than the real - /// one — same approach `migration_store`'s tests take for `sanitize`. #[test] fn project_ids_cannot_escape_the_pending_cleanup_directory() { assert_eq!(sanitize("../../etc/passwd"), "______etc_passwd"); @@ -161,26 +225,43 @@ mod tests { assert!(r.is_empty()); } - /// Save-then-list-then-clear against a real (temp) directory, bypassing - /// `dir()`'s hardcoded `dirs::data_dir()` join by writing/reading the - /// files directly the way `save`/`list` do internally. + /// Exercises the real `save_in`/`list_in`/`clear_in` — not a + /// re-implementation of their bodies — against a temp directory standing + /// in for `dir()`. #[test] fn a_saved_record_round_trips_and_clearing_removes_it() { - let dir = temp_data_dir("roundtrip"); - fs::create_dir_all(&dir).unwrap(); + let dir = temp_dir("roundtrip"); let rec = record("proj-1"); - let path = dir.join(format!("{}.json", sanitize(&rec.project_id))); - let data = serde_json::to_string_pretty(&rec).unwrap(); - fs::write(&path, data).unwrap(); + save_in(&dir, &rec).expect("save"); + let found = list_in(&dir); + assert_eq!(found.len(), 1); + assert_eq!(found[0].project_id, "proj-1"); + assert_eq!(found[0].volumes, vec!["triple-c-home-abc".to_string()]); - let loaded: PendingCleanup = - serde_json::from_str(&fs::read_to_string(&path).unwrap()).unwrap(); - assert_eq!(loaded.project_id, "proj-1"); - assert_eq!(loaded.volumes, vec!["triple-c-home-abc".to_string()]); + clear_in(&dir, "proj-1").expect("clear"); + assert!(list_in(&dir).is_empty()); - fs::remove_file(&path).unwrap(); - assert!(!path.exists()); + fs::remove_dir_all(&dir).ok(); + } + + /// A second `save` for the same project overwrites rather than appending + /// — a retry that narrows the leftovers must not leave the old, wider + /// record behind it. + #[test] + fn saving_the_same_project_twice_overwrites_not_appends() { + let dir = temp_dir("overwrite"); + let mut rec = record("proj-1"); + save_in(&dir, &rec).expect("save"); + + rec.container_id = None; + rec.image = None; + save_in(&dir, &rec).expect("save again"); + + let found = list_in(&dir); + assert_eq!(found.len(), 1, "one file per project, not one per save"); + assert!(found[0].container_id.is_none()); + assert_eq!(found[0].volumes, vec!["triple-c-home-abc".to_string()]); fs::remove_dir_all(&dir).ok(); } @@ -188,30 +269,44 @@ mod tests { /// A record that fails to parse must not poison the rest of the listing. #[test] fn an_unparseable_record_is_skipped_not_fatal() { - let dir = temp_data_dir("corrupt"); - fs::create_dir_all(&dir).unwrap(); + let dir = temp_dir("corrupt"); fs::write(dir.join("bad.json"), "{ not json").unwrap(); - let good = record("proj-2"); - fs::write( - dir.join("proj-2.json"), - serde_json::to_string_pretty(&good).unwrap(), - ) - .unwrap(); + save_in(&dir, &record("proj-2")).expect("save"); - let mut found = Vec::new(); - for entry in fs::read_dir(&dir).unwrap().flatten() { - let path = entry.path(); - if path.extension().is_some_and(|ext| ext == "json") { - if let Ok(data) = fs::read_to_string(&path) { - if let Ok(r) = serde_json::from_str::(&data) { - found.push(r); - } - } - } - } + let found = list_in(&dir); assert_eq!(found.len(), 1); assert_eq!(found[0].project_id, "proj-2"); fs::remove_dir_all(&dir).ok(); } + + /// `list_in` must not pick up the `.json.tmp` staging file `save_in` + /// leaves behind if a crash lands between the write and the rename — the + /// whole point of the temp-then-rename dance is that only the renamed + /// file is ever a complete record. + #[test] + fn a_leftover_tmp_file_is_not_listed() { + let dir = temp_dir("tmp-leftover"); + fs::write(dir.join("proj-3.json.tmp"), "not a complete record").unwrap(); + assert!(list_in(&dir).is_empty()); + + fs::remove_dir_all(&dir).ok(); + } + + /// Clearing by project id must remove exactly the file that id maps to + /// under `sanitize`, and nothing else. + #[test] + fn clearing_one_project_does_not_touch_another() { + let dir = temp_dir("clear-scoped"); + save_in(&dir, &record("proj-a")).unwrap(); + save_in(&dir, &record("proj-b")).unwrap(); + + clear_in(&dir, "proj-a").unwrap(); + + let found = list_in(&dir); + assert_eq!(found.len(), 1); + assert_eq!(found[0].project_id, "proj-b"); + + fs::remove_dir_all(&dir).ok(); + } } diff --git a/app/src/components/projects/home/ProjectHome.tsx b/app/src/components/projects/home/ProjectHome.tsx index a83b04a..cde6832 100644 --- a/app/src/components/projects/home/ProjectHome.tsx +++ b/app/src/components/projects/home/ProjectHome.tsx @@ -1,6 +1,6 @@ import { useEffect, useMemo, useState } from "react"; import { useShallow } from "zustand/react/shallow"; -import type { ProjectRemovalReport } from "../../../lib/types"; +import { projectRemovalIsClean, type ProjectRemovalReport } from "../../../lib/types"; import { useAppState } from "../../../store/appState"; import { useProjectActions } from "../../../hooks/useProjectActions"; import { useProjects } from "../../../hooks/useProjects"; @@ -31,7 +31,15 @@ const TABS = [ export type ProjectHomeTabId = (typeof TABS)[number]["id"]; -/** Names what a `ProjectRemovalReport` says survived, for the leftover toast. */ +/** + * Names what a `ProjectRemovalReport` says survived, for the leftover toast. + * + * Worded as "could not confirm" rather than "is still on disk": the same + * report shape covers a genuine leftover (a locked volume) and a daemon that + * was simply unreachable at the time, in which case nothing was ever created + * and there is nothing to find — asserting certainty either way would be + * wrong in one of those cases. + */ function describeLeftovers(report: ProjectRemovalReport): string { const parts: string[] = []; if (report.container) parts.push("its container"); @@ -294,12 +302,22 @@ export default function ProjectHome({ projectId, active }: Props) { setConfirmRemove(false); try { const report = await remove(project.id); - if (report.container || report.image || report.volumes.length > 0) { - useAppState.getState().pushToast({ - kind: "info", - message: `“${project.name}” was removed, but some Docker resources are still on disk`, - detail: `Triple-C will retry removing ${describeLeftovers(report)} the next time it starts.`, - }); + if (!projectRemovalIsClean(report)) { + if (report.retry_scheduled) { + useAppState.getState().pushToast({ + kind: "info", + message: `“${project.name}” was removed, but Triple-C could not confirm all its Docker resources were cleaned up`, + detail: `Triple-C could not confirm ${describeLeftovers(report)} were removed. It will check again the next time it starts.`, + }); + } else { + // The pending-cleanup record itself failed to save — no + // retry will happen, so this must not promise one. + useAppState.getState().pushToast({ + kind: "error", + message: `“${project.name}” was removed, but its Docker resources could not be cleaned up`, + detail: `Triple-C could not confirm ${describeLeftovers(report)} were removed, and could not record this for a retry. You may need to remove them manually (\`docker rm\` / \`docker rmi\` / \`docker volume rm\`).`, + }); + } } } catch (e) { useAppState.getState().pushToast({ diff --git a/app/src/hooks/useProjectActions.ts b/app/src/hooks/useProjectActions.ts index 07ded95..ed66e2d 100644 --- a/app/src/hooks/useProjectActions.ts +++ b/app/src/hooks/useProjectActions.ts @@ -28,13 +28,14 @@ export function useProjectActions(project: Project) { ); const run = useCallback( - async (label: string, fn: () => Promise) => { + async (label: string, fn: () => Promise): Promise => { setBusy(true); setContainerProgress(project.id, null); try { - await fn(); + return await fn(); } catch (e) { fail(`${label} failed for “${project.name}”`, e); + return undefined; } finally { setContainerProgress(project.id, null); setBusy(false); @@ -54,8 +55,22 @@ export function useProjectActions(project: Project) { ); const handleReset = useCallback( - () => run("Reset", () => rebuild(project.id)), - [run, rebuild, project.id], + () => + run("Reset", async () => { + const outcome = await rebuild(project.id); + if (outcome.leftover_volumes.length > 0) { + const n = outcome.leftover_volumes.length; + pushToast({ + kind: "error", + message: `Reset for “${project.name}” could not fully clean up`, + detail: `${n === 1 ? "A volume" : `${n} volumes`} could not be removed, so the new \ +container may still contain data from before the reset. You may need to remove ${n === 1 ? "it" : "them"} \ +manually with \`docker volume rm\`.`, + }); + } + return outcome; + }), + [run, rebuild, project.id, project.name, pushToast], ); const openClaudeTerminal = useCallback(async () => { diff --git a/app/src/hooks/useProjects.ts b/app/src/hooks/useProjects.ts index 2ebcb5b..4b88c5e 100644 --- a/app/src/hooks/useProjects.ts +++ b/app/src/hooks/useProjects.ts @@ -136,9 +136,9 @@ export function useProjects() { const rebuild = useCallback( (id: string) => withOptimisticStatus(id, "starting", async () => { - const updated = await commands.rebuildProjectContainer(id); - updateProjectInList(updated); - return updated; + const outcome = await commands.rebuildProjectContainer(id); + updateProjectInList(outcome.project); + return outcome; }), [updateProjectInList, withOptimisticStatus], ); diff --git a/app/src/lib/tauri-commands.ts b/app/src/lib/tauri-commands.ts index 3264818..6389a4c 100644 --- a/app/src/lib/tauri-commands.ts +++ b/app/src/lib/tauri-commands.ts @@ -1,5 +1,5 @@ import { invoke } from "@tauri-apps/api/core"; -import type { Project, ProjectPath, ProjectRemovalReport, ContainerInfo, AppSettings, UpdateInfo, ImageUpdateInfo, FileEntry, FileContents, WebTerminalInfo, SttStatus, GatewayStatus, InstallOptions, ClaudeSession, ContainerCapabilities, ScheduledTask, ScheduledTaskInput, SchedulerNotification, AuthBridgeStatus, BrowserViewStatus, BrowserViewPopoutState, BrowserPageState, PlaywrightDetection, BrowserSetupOutcome, BrowserInstallTarget, ContainerStaleness, MigrationOptions, MigrationReport, MigrationState, ClearTokenOutcome, CaCertInfo, UploadOutcome } from "./types"; +import type { Project, ProjectPath, ProjectRemovalReport, ProjectResetOutcome, ContainerInfo, AppSettings, UpdateInfo, ImageUpdateInfo, FileEntry, FileContents, WebTerminalInfo, SttStatus, GatewayStatus, InstallOptions, ClaudeSession, ContainerCapabilities, ScheduledTask, ScheduledTaskInput, SchedulerNotification, AuthBridgeStatus, BrowserViewStatus, BrowserViewPopoutState, BrowserPageState, PlaywrightDetection, BrowserSetupOutcome, BrowserInstallTarget, ContainerStaleness, MigrationOptions, MigrationReport, MigrationState, ClearTokenOutcome, CaCertInfo, UploadOutcome } from "./types"; // Docker export const checkDocker = () => invoke("check_docker"); @@ -21,7 +21,7 @@ export const startProjectContainer = (projectId: string) => export const stopProjectContainer = (projectId: string) => invoke("stop_project_container", { projectId }); export const rebuildProjectContainer = (projectId: string) => - invoke("rebuild_project_container", { projectId }); + invoke("rebuild_project_container", { projectId }); export const reconcileProjectStatuses = () => invoke("reconcile_project_statuses"); diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index a84e85a..b6c2be8 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -78,12 +78,30 @@ export type ProjectStatus = | "error"; /** What `removeProject` could not delete. The project is removed from the - * sidebar either way; anything named here is recorded on the host and - * retried automatically the next time the app starts. */ + * sidebar either way. When `retry_scheduled` is true, anything named here + * was recorded on the host and will be retried automatically the next time + * the app starts; when false, the record itself could not be saved and + * nothing will retry it. `retry_scheduled` is meaningless when nothing was + * left behind. */ export interface ProjectRemovalReport { container: string | null; image: string | null; volumes: string[]; + retry_scheduled: boolean; +} + +/** True when a `ProjectRemovalReport` left nothing behind. Mirrors the + * Rust-side `ProjectRemovalReport::is_clean`. */ +export function projectRemovalIsClean(report: ProjectRemovalReport): boolean { + return !report.container && !report.image && report.volumes.length === 0; +} + +/** What Reset (`rebuildProjectContainer`) produced: the project as it stands + * after restarting, and any volume Reset could not clear — which is reused + * as-is by the new container instead of starting clean. */ +export interface ProjectResetOutcome { + project: Project; + leftover_volumes: string[]; } export type Backend = -- 2.52.0 From 439ef16f07f8d44e3599ca670a3c7bda2d6c4d04 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 27 Aug 2026 09:09:46 -0700 Subject: [PATCH 3/4] Fix two new bugs a second review found: stale container id, orphaned record MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second Opus review of commit 2 found it had introduced real problems of its own rather than just polish gaps: - remove_project's "None or stale" container-id fallback only handled None. A stale id (the documented start-failure race in start_project_container_locked, where the old container is removed and the new one's id isn't persisted until after start_container succeeds) still 404'd on removal — now treated as success by commit 1's own fix — while the real container survived to block every volume removal with a 409 forever, with nothing in the pending-cleanup record ever naming it. Both remove_project and rebuild_project_container now resolve the container via find_existing_container() unconditionally, matching every other container-destroying path in the codebase, and remove_project fails closed (records a leftover rather than silently skipping) if Docker itself can't be reached to check. - remove_project could leave a pending-cleanup record for a project still live in projects.json: if the store's own save failed after the record was written, startup housekeeping would delete that project's container and volumes out from under it on the next launch. The record is now rolled back when the store write fails. - rebuild_project_container (Reset) only surfaced a leftover volume, not a leftover snapshot image — the more serious failure, since the next container is built from that image whenever it exists, silently reviving the exact system layer Reset was asked to discard. ProjectResetOutcome now carries leftover_image too, and the toast's "run docker volume rm" advice is corrected: the new container has already remounted the volume by the time the toast renders, so that command would just hit the same conflict Reset did. Also from the same pass: reworded a couple of log/toast lines that still asserted resources were "still present" when the daemon-unreachable case covered by the same code path can't actually confirm that; fixed a singular/verb mismatch in the leftover toast text; moved an unparseable pending-cleanup record aside instead of re-warning about it forever; and added a debug log when a record's recorded_at can't be parsed, so aging never silently no-ops. Pulled describeLeftovers/leftoverVerb out of ProjectHome.tsx into their own module with unit tests, and added tests for the recorded_at staleness check — the previous commit's equivalent logic had none. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FGjXq6fqtAFHdbhk4f3PfZ --- .../src/commands/project_commands.rs | 193 +++++++++++++----- app/src-tauri/src/models/project.rs | 17 +- app/src-tauri/src/storage/pending_cleanup.rs | 19 +- .../components/projects/home/ProjectHome.tsx | 30 +-- .../projects/home/removalReport.test.ts | 51 +++++ .../components/projects/home/removalReport.ts | 26 +++ app/src/hooks/useProjectActions.ts | 20 +- app/src/lib/types.ts | 4 + 8 files changed, 276 insertions(+), 84 deletions(-) create mode 100644 app/src/components/projects/home/removalReport.test.ts create mode 100644 app/src/components/projects/home/removalReport.ts diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 27a1e5f..1ae7275 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -730,19 +730,37 @@ pub async fn remove_project( let existing_project = state.projects_store.get(&project_id); if let Some(ref project) = existing_project { - // `project.container_id` can be `None` or stale — a crash between - // creating a container and persisting its id is the same race every - // other destroyer of a project's container already guards against - // with `find_existing_container` (`start_project_container`, - // migration's recreate paths). Removal is the one place that - // mattered least before this fix, because a container `remove_project` - // missed just sat there; now a miss here poisons the volume removal - // right after it (Docker refuses to delete a volume a container still - // references) and mints a pending-cleanup record for volumes with no - // way to name the container actually blocking them. - let container_ref = match &project.container_id { - Some(id) => Some(id.clone()), - None => docker::find_existing_container(project).await.ok().flatten(), + // Resolved via `find_existing_container` unconditionally rather than + // trusting `project.container_id` — that field can be *stale*, not + // just absent: `start_project_container_locked`'s recreate path + // removes the old container, creates a new one, and does not persist + // the new id until after `start_container` succeeds, so a start + // failure in between (a missing `/dev/net/tun`, an image that exits + // immediately) leaves the stored id pointing at a container that no + // longer exists while a live one sits under the same deterministic + // name. Removing by a stale id then 404s — success as far as Docker + // is concerned — while the real container survives to block every + // subsequent volume removal with a 409, with nothing in the report + // ever naming it. `find_existing_container` is what every other + // destroyer of a project's container already resolves through + // (`start_project_container`, migration's recreate paths) for this + // exact reason. + // + // A `Docker unreachable` error here is treated as "assume a + // container is still there" rather than "assume none is", matching + // `remove_volumes_by_name`'s fail-closed handling of the same + // situation — the alternative silently drops the one resource most + // likely to block everything else if it does exist. + let container_ref = match docker::find_existing_container(project).await { + Ok(found) => found, + Err(e) => { + log::warn!( + "Could not check for an existing container for project {}: {}", + project_id, e + ); + report.container = Some(project.container_name()); + None + } }; if let Some(ref container_id) = container_ref { state.exec_manager.close_sessions_for_container(container_id).await; @@ -791,24 +809,35 @@ pub async fn remove_project( Ok(()) => { report.retry_scheduled = true; log::warn!( - "Project {} removed with Docker resources still present: {:?} — recorded for \ - automatic retry on next launch", + "Project {} removed; could not confirm these Docker resources were removed: \ + {:?} — recorded for automatic retry on next launch", project_id, report ); } Err(e) => { report.retry_scheduled = false; log::error!( - "Project {} removed with Docker resources still present ({:?}), and the \ - pending-cleanup record could not be written ({}) — nothing will retry \ - removing them", + "Project {} removed; could not confirm these Docker resources were removed \ + ({:?}), and the pending-cleanup record could not be written ({}) — nothing \ + will retry removing them", project_id, report, e ); } } } - state.projects_store.remove(&project_id)?; + // The pending-cleanup record above must not outlive the project record it + // describes: if the store's own write fails (full disk, permissions) the + // project is still on disk and will reload on the next launch, but the + // record would tell startup housekeeping to delete its container and + // volumes out from under it. Roll the record back rather than leaving + // that mismatch for the retry to discover the hard way. + if let Err(e) = state.projects_store.remove(&project_id) { + if !report.is_clean() { + let _ = crate::storage::pending_cleanup::clear(&project_id); + } + return Err(e); + } Ok(report) } @@ -882,19 +911,23 @@ pub async fn retry_pending_cleanup_logged() { // record that has failed every retry for a week is no longer // routine: escalate the log level so it is not indistinguishable // from one seen for the first time. - let age = chrono::DateTime::parse_from_rfc3339(&record.recorded_at) - .ok() - .map(|t| chrono::Utc::now().signed_duration_since(t.with_timezone(&chrono::Utc))); - match age { - Some(age) if age > chrono::Duration::days(PENDING_CLEANUP_STALE_AFTER_DAYS) => { + match pending_cleanup_is_stale(&record.recorded_at, chrono::Utc::now()) { + Some(true) => { log::error!( - "Pending cleanup for project {} ({}) has not succeeded in over {} days: \ - {:?} — this may need a manual `docker volume rm` / `docker rmi` / \ - `docker rm`", + "Pending cleanup for project {} ({}) has not succeeded in over {} \ + days: {:?} — this may need a manual `docker volume rm` / \ + `docker rmi` / `docker rm`", record.project_id, record.project_name, PENDING_CLEANUP_STALE_AFTER_DAYS, record ); } - _ => {} + Some(false) => {} + // Silent otherwise would mean a record with a corrupted + // timestamp never escalates and nothing says why. + None => log::debug!( + "Pending cleanup record for project {} ({}) has an unreadable recorded_at \ + ({:?}) — its age cannot be tracked", + record.project_id, record.project_name, record.recorded_at + ), } if let Err(e) = crate::storage::pending_cleanup::save(&record) { log::warn!( @@ -916,6 +949,19 @@ pub async fn retry_pending_cleanup_logged() { /// `error` — see the comment at its call site. const PENDING_CLEANUP_STALE_AFTER_DAYS: i64 = 7; +/// Whether a pending-cleanup record's `recorded_at` is older than +/// [`PENDING_CLEANUP_STALE_AFTER_DAYS`], measured against `now`. `None` means +/// the timestamp could not be parsed at all — a corrupted or (hypothetically) +/// hand-edited record — which callers must not silently treat as "not stale" +/// without saying why. `now` is a parameter rather than read internally so +/// this is testable without a live clock. +fn pending_cleanup_is_stale(recorded_at: &str, now: chrono::DateTime) -> Option { + let recorded = chrono::DateTime::parse_from_rfc3339(recorded_at) + .ok()? + .with_timezone(&chrono::Utc); + Some(now.signed_duration_since(recorded) > chrono::Duration::days(PENDING_CLEANUP_STALE_AFTER_DAYS)) +} + #[tauri::command] pub async fn update_project( project: serde_json::Value, @@ -1417,15 +1463,16 @@ pub async fn rebuild_project_container( // `start_project_container` below re-arms it against the new one. state.auth_bridge.stop(&project_id).await; - // Remove existing container. Resolved the same way `remove_project` now - // is — `project.container_id` can be `None` or stale — because a + // Remove existing container. Resolved via `find_existing_container` + // unconditionally, not `project.container_id` — see the long comment in + // `remove_project` for why that field can be stale, not just absent. A // container this misses blocks the volume removal immediately below with // a 409, and Reset silently keeping the old volumes is exactly the bug - // this whole change is closing. - let container_ref = match &project.container_id { - Some(id) => Some(id.clone()), - None => docker::find_existing_container(&project).await.ok().flatten(), - }; + // this whole change is closing. Unlike `remove_project`'s best-effort + // handling of the same lookup failing, `?` here aborts Reset outright: + // every step after this one needs Docker too, so there is no useful + // partial progress to make without it. + let container_ref = docker::find_existing_container(&project).await?; if let Some(ref container_id) = container_ref { state.exec_manager.close_sessions_for_container(container_id).await; let _ = docker::stop_container(container_id).await; @@ -1433,29 +1480,33 @@ pub async fn rebuild_project_container( state.projects_store.set_container_id(&project_id, None)?; } - // Remove snapshot image + volumes so Reset creates from the clean base image + // Remove snapshot image + volumes so Reset creates from the clean base + // image. Both leftovers are surfaced, not just logged — an image that + // survives is the more serious of the two, since + // `start_project_container_locked` below builds from + // `triple-c-snapshot-{id}:latest` whenever it exists, so a leftover image + // means Reset silently rebuilds the exact system layer it promised to + // discard. No pending-cleanup record for either: unlike `remove_project`, + // Reset keeps the project record, so a later Reset attempt can retry + // these itself rather than needing startup housekeeping to do it. + let mut leftover_image = None; if let Err(e) = docker::remove_snapshot_image(&project).await { log::warn!("Failed to remove snapshot image for project {}: {}", project_id, e); + leftover_image = Some(docker::get_snapshot_image_name(&project)); } let leftover_volumes = docker::remove_project_volumes(&project).await; - if !leftover_volumes.is_empty() { - // Unlike `remove_project`, Reset keeps the project record — but a - // volume that survives this is reused as-is by the container - // `start_project_container_locked` creates below, which is exactly - // what Reset promises not to do. No pending-cleanup record: the - // project id is still live, so a later Reset attempt can retry this - // itself rather than needing startup housekeeping to do it. + if leftover_image.is_some() || !leftover_volumes.is_empty() { log::warn!( - "Reset could not remove volume(s) {:?} for project {} — the new container may reuse \ - their old contents instead of starting clean", - leftover_volumes, project_id + "Reset for project {} could not fully clean up — image: {:?}, volumes: {:?} — the \ + new container may be built from, or reuse, old contents instead of starting clean", + project_id, leftover_image, leftover_volumes ); } // Start fresh. The locked variant, because `_guard` above is this project's // claim and the public command would be refused by it. let project = start_project_container_locked(project_id, app_handle, state).await?; - Ok(ProjectResetOutcome { project, leftover_volumes }) + Ok(ProjectResetOutcome { project, leftover_image, leftover_volumes }) } /// Reconcile project statuses against actual Docker container state. @@ -1563,6 +1614,54 @@ fn default_docker_socket() -> String { mod tests { use super::*; + // ── Pending-cleanup aging ──────────────────────────────────────────── + + #[test] + fn a_record_younger_than_the_threshold_is_not_stale() { + let now = "2026-08-25T00:00:00Z".parse().unwrap(); + let recorded_at = "2026-08-19T00:00:00Z"; // 6 days before `now` + assert_eq!(pending_cleanup_is_stale(recorded_at, now), Some(false)); + } + + #[test] + fn a_record_exactly_at_the_threshold_is_not_yet_stale() { + let now = "2026-08-25T00:00:00Z".parse().unwrap(); + let recorded_at = "2026-08-18T00:00:00Z"; // exactly 7 days before `now` + assert_eq!( + pending_cleanup_is_stale(recorded_at, now), + Some(false), + "the boundary itself must not already read as stale" + ); + } + + #[test] + fn a_record_older_than_the_threshold_is_stale() { + let now = "2026-08-25T00:00:00Z".parse().unwrap(); + let recorded_at = "2026-08-17T00:00:00Z"; // 8 days before `now` + assert_eq!(pending_cleanup_is_stale(recorded_at, now), Some(true)); + } + + /// A clock that ran fast when the record was written leaves a timestamp + /// in the future. This must read as "not stale" rather than underflow or + /// panic — `signed_duration_since` returns a negative `Duration` here, + /// which compares less than any positive threshold correctly. + #[test] + fn a_timestamp_in_the_future_is_not_stale() { + let now = "2026-08-25T00:00:00Z".parse().unwrap(); + let recorded_at = "2026-08-26T00:00:00Z"; // one day after `now` + assert_eq!(pending_cleanup_is_stale(recorded_at, now), Some(false)); + } + + /// A corrupted or hand-edited `recorded_at` must not silently read as + /// "not stale" through some default — callers need to be able to tell + /// "definitely not stale" apart from "cannot tell". + #[test] + fn an_unparseable_recorded_at_reports_unknown_rather_than_not_stale() { + let now = "2026-08-25T00:00:00Z".parse().unwrap(); + assert_eq!(pending_cleanup_is_stale("not a timestamp", now), None); + assert_eq!(pending_cleanup_is_stale("", now), None); + } + fn path(host: &str, mount: &str) -> ProjectPath { ProjectPath { host_path: host.to_string(), diff --git a/app/src-tauri/src/models/project.rs b/app/src-tauri/src/models/project.rs index c6b09c1..5c22e15 100644 --- a/app/src-tauri/src/models/project.rs +++ b/app/src-tauri/src/models/project.rs @@ -457,16 +457,21 @@ impl ProjectRemovalReport { } /// What `rebuild_project_container` (Reset) produced: the project as it -/// stands after restarting, and any volume Reset could not clear. +/// stands after restarting, and anything Reset could not clear. /// -/// Reset's contract is "back to a clean base image", so a leftover volume -/// here is reused as-is by the container this creates — the opposite of what -/// was asked for — and unlike [`ProjectRemovalReport`] there is no -/// pending-cleanup record for it: the project id survives Reset, so a later -/// Reset attempt can retry the same volume itself. +/// Reset's contract is "back to a clean base image", so a leftover volume or +/// image here is reused/rebuilt-from as-is by the container this creates — +/// the opposite of what was asked for — and unlike [`ProjectRemovalReport`] +/// there is no pending-cleanup record for either: the project id survives +/// Reset, so a later Reset attempt can retry them itself. #[derive(Debug, Clone, Serialize)] pub struct ProjectResetOutcome { pub project: Project, + /// The `triple-c-snapshot-{id}` image, if Reset could not remove it. The + /// more serious of the two leftovers here: the new container is created + /// from this image whenever it exists, so a surviving image means Reset + /// silently rebuilt the exact system layer it was asked to discard. + pub leftover_image: Option, /// Volumes that survived Reset and were mounted into the new container /// unchanged. pub leftover_volumes: Vec, diff --git a/app/src-tauri/src/storage/pending_cleanup.rs b/app/src-tauri/src/storage/pending_cleanup.rs index f83bc75..c544884 100644 --- a/app/src-tauri/src/storage/pending_cleanup.rs +++ b/app/src-tauri/src/storage/pending_cleanup.rs @@ -146,10 +146,25 @@ fn list_in(dir: &Path) -> Vec { match serde_json::from_str::(&data) { Ok(record) => Some(record), Err(err) => { + // Moved aside rather than left in place: a record nothing + // ever repairs would otherwise warn on every single + // startup forever, same as an ordinary `.json` file it + // would keep looking like one to `list_in` on the next + // call too. One aside-copy is enough here — this only + // ever holds names to retry removing, not the class of + // once-in-a-lifetime crash evidence `migration_store` + // keeps multiple timestamped backups of. + let corrupt = path.with_extension("json.corrupt"); + let moved = !corrupt.exists() && fs::rename(&path, &corrupt).is_ok(); log::warn!( - "Could not parse pending cleanup record {}: {} — skipping it this run", + "Could not parse pending cleanup record {}: {}{}", path.display(), - err + err, + if moved { + format!(" — moved aside to {}", corrupt.display()) + } else { + " — leaving it in place".to_string() + } ); None } diff --git a/app/src/components/projects/home/ProjectHome.tsx b/app/src/components/projects/home/ProjectHome.tsx index cde6832..907b8a9 100644 --- a/app/src/components/projects/home/ProjectHome.tsx +++ b/app/src/components/projects/home/ProjectHome.tsx @@ -1,6 +1,6 @@ import { useEffect, useMemo, useState } from "react"; import { useShallow } from "zustand/react/shallow"; -import { projectRemovalIsClean, type ProjectRemovalReport } from "../../../lib/types"; +import { projectRemovalIsClean } from "../../../lib/types"; import { useAppState } from "../../../store/appState"; import { useProjectActions } from "../../../hooks/useProjectActions"; import { useProjects } from "../../../hooks/useProjects"; @@ -19,6 +19,7 @@ import ConfigTab from "./ConfigTab"; import FilesTab from "./FilesTab"; import BrowserTab from "./BrowserTab"; import { formatUptime } from "./format"; +import { describeLeftovers, leftoverVerb } from "./removalReport"; const TABS = [ { id: "overview", label: "Overview" }, @@ -31,24 +32,6 @@ const TABS = [ export type ProjectHomeTabId = (typeof TABS)[number]["id"]; -/** - * Names what a `ProjectRemovalReport` says survived, for the leftover toast. - * - * Worded as "could not confirm" rather than "is still on disk": the same - * report shape covers a genuine leftover (a locked volume) and a daemon that - * was simply unreachable at the time, in which case nothing was ever created - * and there is nothing to find — asserting certainty either way would be - * wrong in one of those cases. - */ -function describeLeftovers(report: ProjectRemovalReport): string { - const parts: string[] = []; - if (report.container) parts.push("its container"); - if (report.image) parts.push("its saved image"); - if (report.volumes.length === 1) parts.push("a volume"); - else if (report.volumes.length > 1) parts.push(`${report.volumes.length} volumes`); - return parts.join(", "); -} - interface Props { projectId: string; active: boolean; @@ -303,19 +286,20 @@ export default function ProjectHome({ projectId, active }: Props) { try { const report = await remove(project.id); if (!projectRemovalIsClean(report)) { + const verb = leftoverVerb(report); if (report.retry_scheduled) { useAppState.getState().pushToast({ kind: "info", - message: `“${project.name}” was removed, but Triple-C could not confirm all its Docker resources were cleaned up`, - detail: `Triple-C could not confirm ${describeLeftovers(report)} were removed. It will check again the next time it starts.`, + message: `“${project.name}” was removed, but Triple-C could not confirm all its Docker resources were removed`, + detail: `Triple-C could not confirm ${describeLeftovers(report)} ${verb} removed. It will check again the next time it starts.`, }); } else { // The pending-cleanup record itself failed to save — no // retry will happen, so this must not promise one. useAppState.getState().pushToast({ kind: "error", - message: `“${project.name}” was removed, but its Docker resources could not be cleaned up`, - detail: `Triple-C could not confirm ${describeLeftovers(report)} were removed, and could not record this for a retry. You may need to remove them manually (\`docker rm\` / \`docker rmi\` / \`docker volume rm\`).`, + message: `“${project.name}” was removed, but Triple-C could not confirm its Docker resources were removed`, + detail: `Triple-C could not confirm ${describeLeftovers(report)} ${verb} removed, and could not record this for a retry. You may need to remove them manually (\`docker rm\` / \`docker rmi\` / \`docker volume rm\`).`, }); } } diff --git a/app/src/components/projects/home/removalReport.test.ts b/app/src/components/projects/home/removalReport.test.ts new file mode 100644 index 0000000..8d9c752 --- /dev/null +++ b/app/src/components/projects/home/removalReport.test.ts @@ -0,0 +1,51 @@ +import { describe, it, expect } from "vitest"; +import { describeLeftovers, leftoverVerb } from "./removalReport"; +import { projectRemovalIsClean } from "../../../lib/types"; +import type { ProjectRemovalReport } from "../../../lib/types"; + +function report(overrides: Partial = {}): ProjectRemovalReport { + return { + container: null, + image: null, + volumes: [], + retry_scheduled: false, + ...overrides, + }; +} + +describe("projectRemovalIsClean", () => { + it("is true only when nothing survived", () => { + expect(projectRemovalIsClean(report())).toBe(true); + expect(projectRemovalIsClean(report({ container: "triple-c-abc" }))).toBe(false); + expect(projectRemovalIsClean(report({ image: "triple-c-snapshot-abc:latest" }))).toBe(false); + expect(projectRemovalIsClean(report({ volumes: ["triple-c-home-abc"] }))).toBe(false); + }); +}); + +describe("describeLeftovers", () => { + it("names each kind of leftover", () => { + expect(describeLeftovers(report({ container: "triple-c-abc" }))).toBe("its container"); + expect(describeLeftovers(report({ image: "x" }))).toBe("its saved image"); + expect(describeLeftovers(report({ volumes: ["v1"] }))).toBe("a volume"); + expect(describeLeftovers(report({ volumes: ["v1", "v2"] }))).toBe("2 volumes"); + }); + + it("joins multiple kinds together", () => { + expect( + describeLeftovers(report({ container: "triple-c-abc", image: "x", volumes: ["v1", "v2"] })), + ).toBe("its container, its saved image, 2 volumes"); + }); +}); + +describe("leftoverVerb", () => { + it("is singular for exactly one leftover of any kind", () => { + expect(leftoverVerb(report({ container: "triple-c-abc" }))).toBe("was"); + expect(leftoverVerb(report({ image: "x" }))).toBe("was"); + expect(leftoverVerb(report({ volumes: ["v1"] }))).toBe("was"); + }); + + it("is plural once more than one thing survived, including multiple volumes alone", () => { + expect(leftoverVerb(report({ container: "triple-c-abc", image: "x" }))).toBe("were"); + expect(leftoverVerb(report({ volumes: ["v1", "v2"] }))).toBe("were"); + }); +}); diff --git a/app/src/components/projects/home/removalReport.ts b/app/src/components/projects/home/removalReport.ts new file mode 100644 index 0000000..dc0367f --- /dev/null +++ b/app/src/components/projects/home/removalReport.ts @@ -0,0 +1,26 @@ +import type { ProjectRemovalReport } from "../../../lib/types"; + +/** + * Names what a `ProjectRemovalReport` says survived, for the leftover toast. + * + * Worded as "could not confirm" rather than "is still on disk": the same + * report shape covers a genuine leftover (a locked volume) and a daemon that + * was simply unreachable at the time, in which case nothing was ever created + * and there is nothing to find — asserting certainty either way would be + * wrong in one of those cases. + */ +export function describeLeftovers(report: ProjectRemovalReport): string { + const parts: string[] = []; + if (report.container) parts.push("its container"); + if (report.image) parts.push("its saved image"); + if (report.volumes.length === 1) parts.push("a volume"); + else if (report.volumes.length > 1) parts.push(`${report.volumes.length} volumes`); + return parts.join(", "); +} + +/** Verb agreement for `describeLeftovers`'s output — "its container" needs + * "was", "its container, a volume" needs "were". */ +export function leftoverVerb(report: ProjectRemovalReport): "was" | "were" { + const count = (report.container ? 1 : 0) + (report.image ? 1 : 0) + report.volumes.length; + return count === 1 ? "was" : "were"; +} diff --git a/app/src/hooks/useProjectActions.ts b/app/src/hooks/useProjectActions.ts index ed66e2d..9f81e8f 100644 --- a/app/src/hooks/useProjectActions.ts +++ b/app/src/hooks/useProjectActions.ts @@ -58,14 +58,22 @@ export function useProjectActions(project: Project) { () => run("Reset", async () => { const outcome = await rebuild(project.id); - if (outcome.leftover_volumes.length > 0) { - const n = outcome.leftover_volumes.length; + const { leftover_image, leftover_volumes } = outcome; + if (leftover_image || leftover_volumes.length > 0) { + const parts: string[] = []; + if (leftover_image) parts.push("its previous container image"); + if (leftover_volumes.length === 1) parts.push("a volume"); + else if (leftover_volumes.length > 1) parts.push(`${leftover_volumes.length} volumes`); + // Not "run `docker volume rm`" — by the time this renders, the new + // container this same call just started already has the leftover + // volume mounted, so that command would just hit the same 409 + // Reset did. Stopping the project first is what actually frees it. pushToast({ kind: "error", - message: `Reset for “${project.name}” could not fully clean up`, - detail: `${n === 1 ? "A volume" : `${n} volumes`} could not be removed, so the new \ -container may still contain data from before the reset. You may need to remove ${n === 1 ? "it" : "them"} \ -manually with \`docker volume rm\`.`, + message: `Reset for “${project.name}” did not fully clean up`, + detail: `Triple-C could not remove ${parts.join(" and ")} from before the reset, so \ +the new container may still be built from, or contain, old data. Stop the project, then try \ +Reset again, or remove ${parts.length > 1 ? "them" : "it"} manually once stopped.`, }); } return outcome; diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index b6c2be8..9a6462c 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -101,6 +101,10 @@ export function projectRemovalIsClean(report: ProjectRemovalReport): boolean { * as-is by the new container instead of starting clean. */ export interface ProjectResetOutcome { project: Project; + /** The saved container image, if Reset could not remove it — the new + * container is built from it whenever it exists, so this means Reset + * silently rebuilt the system layer it was asked to discard. */ + leftover_image: string | null; leftover_volumes: string[]; } -- 2.52.0 From 61bdbc4a5bef4ebce9806aac8443c9e98ca1c766 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 27 Aug 2026 09:47:45 -0700 Subject: [PATCH 4/4] Close the crash-window gap and exec-session leak a third review found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A third Opus review pass confirmed round 2's fixes hold up, then found: - The pending-cleanup record `remove_project` writes is fully durable (fsync'd); the projects_store.remove() that follows it is a plain fs::write with no fsync. A crash or power loss in that window — or that store write failing outright, beyond what the previous round's in-process rollback catches — leaves a record on disk naming a project projects.json still lists as present. The very next startup retry would then delete that project's container, snapshot image, and both volumes (including the one holding the OAuth credential and every session transcript) out from under a project the user still sees in the sidebar. retry_pending_cleanup_logged now takes the ProjectsStore and refuses to touch — clearing instead — any record whose project id still exists. Also stopped swallowing the round-2 rollback's own failure. - Resolving the container through find_existing_container instead of project.container_id (round 2's stale-id fix) changed what drove close_sessions_for_container in remove_project and rebuild_project_ container: sessions are now leaked when Docker is unreachable (nothing resolves, so nothing closes, and the project record is gone a moment later) and in the stale-id race itself (sessions were opened against the container that actually exists, not the id find_existing_container bypasses). Both functions now close sessions for the stored id unconditionally, and again for the resolved id if it differs. - A pronoun-agreement bug in the no-retry removal toast ("remove them manually" for a single leftover) that was fixed one line above for verb agreement but not for the pronoun. Also closed the test gaps the review named: the pending-cleanup corrupt-record aside-move had no test, the Reset toast's leftover copy was inline and untested (extracted to lib/resetOutcome.ts, mirroring components/projects/home/removalReport.ts, with unit tests), and nothing asserted rebuild()'s success path maps outcome.project into the list rather than the whole outcome. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01FGjXq6fqtAFHdbhk4f3PfZ --- .../src/commands/project_commands.rs | 78 ++++++++++++++++++- app/src-tauri/src/lib.rs | 10 ++- app/src-tauri/src/storage/pending_cleanup.rs | 22 ++++++ .../components/projects/home/ProjectHome.tsx | 4 +- .../components/projects/home/removalReport.ts | 17 +++- app/src/hooks/useProjectActions.ts | 12 +-- app/src/hooks/useProjects.test.ts | 24 ++++++ app/src/lib/resetOutcome.test.ts | 39 ++++++++++ app/src/lib/resetOutcome.ts | 32 ++++++++ 9 files changed, 220 insertions(+), 18 deletions(-) create mode 100644 app/src/lib/resetOutcome.test.ts create mode 100644 app/src/lib/resetOutcome.ts diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 1ae7275..4768638 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -751,6 +751,20 @@ pub async fn remove_project( // `remove_volumes_by_name`'s fail-closed handling of the same // situation — the alternative silently drops the one resource most // likely to block everything else if it does exist. + // + // Exec sessions are closed for `project.container_id` unconditionally, + // before the lookup above and regardless of whether it succeeds — + // that is host-side state with no Docker dependency, so it must not + // wait on a daemon that might not answer. Resolving through + // `find_existing_container` instead of using it directly would leave + // these open in exactly the two cases this whole change exists to + // handle: Docker unreachable (no id resolved, no way to ever close + // them again once the project record is gone) and the stale-id race + // (sessions were opened against the container that actually exists, + // which is what gets resolved below, not the stored id). + if let Some(ref stored_id) = project.container_id { + state.exec_manager.close_sessions_for_container(stored_id).await; + } let container_ref = match docker::find_existing_container(project).await { Ok(found) => found, Err(e) => { @@ -763,7 +777,9 @@ pub async fn remove_project( } }; if let Some(ref container_id) = container_ref { - state.exec_manager.close_sessions_for_container(container_id).await; + if project.container_id.as_deref() != Some(container_id.as_str()) { + state.exec_manager.close_sessions_for_container(container_id).await; + } let _ = docker::stop_container(container_id).await; if let Err(e) = docker::remove_container(container_id).await { log::warn!( @@ -832,9 +848,24 @@ pub async fn remove_project( // record would tell startup housekeeping to delete its container and // volumes out from under it. Roll the record back rather than leaving // that mismatch for the retry to discover the hard way. + // + // This is a second, narrower line of defence, not the only one — a crash + // between the `save` above and the `remove` below leaves exactly the same + // mismatch with no error for either side to catch, which is why + // `retry_pending_cleanup_logged` also refuses to act on a record whose + // project is still listed in `projects.json`. Belt and suspenders: a + // caught failure here is handled immediately rather than waiting for the + // next launch to notice. if let Err(e) = state.projects_store.remove(&project_id) { if !report.is_clean() { - let _ = crate::storage::pending_cleanup::clear(&project_id); + if let Err(clear_err) = crate::storage::pending_cleanup::clear(&project_id) { + log::error!( + "Project {} was not removed ({}), and its pending-cleanup record could not \ + be rolled back either ({}) — it will name this still-live project until \ + startup housekeeping's own guard clears it", + project_id, e, clear_err + ); + } } return Err(e); } @@ -853,7 +884,21 @@ pub async fn remove_project( /// reasoning), so there is no IPC contract to keep. A record that still has /// leftovers after this is written back so the next run does not lose track /// of what changed; one that is now empty is deleted. -pub async fn retry_pending_cleanup_logged() { +/// +/// Takes the `ProjectsStore` so it can refuse to touch a project that is +/// still live: `remove_project` writes a pending-cleanup record durably +/// (fsync'd) *before* it asks the store to drop the project, and that +/// store write is a plain `fs::write` with no fsync of its own. A crash or +/// power loss in the gap between the two — or the store write failing +/// outright, on top of the round-2 fix that only rolls the record back when +/// that failure is caught in-process — can leave a record on disk pointing +/// at a project `projects.json` still lists. Without this check, the very +/// first retry after such a crash deletes that project's container, +/// snapshot image and *both volumes, including the one holding the OAuth +/// credential and every session transcript*, out from under a project the +/// user still sees in the sidebar. A record whose project still exists is +/// therefore always stale — cleared without touching Docker, not retried. +pub async fn retry_pending_cleanup_logged(projects_store: &crate::storage::projects_store::ProjectsStore) { let records = crate::storage::pending_cleanup::list(); if records.is_empty() { return; @@ -863,6 +908,23 @@ pub async fn retry_pending_cleanup_logged() { let mut still_pending = 0usize; for mut record in records { + if projects_store.get(&record.project_id).is_some() { + log::warn!( + "Pending cleanup record for project {} ({}) names a project that still exists — \ + clearing the record without touching Docker rather than risk deleting a live \ + project's resources", + record.project_id, record.project_name + ); + if let Err(e) = crate::storage::pending_cleanup::clear(&record.project_id) { + log::error!( + "Could not clear the stale pending-cleanup record for still-live project {} \ + ({}): {}", + record.project_id, record.project_name, e + ); + } + continue; + } + if let Some(container_id) = record.container_id.take() { match docker::remove_container(&container_id).await { Ok(()) => {} @@ -1472,9 +1534,17 @@ pub async fn rebuild_project_container( // handling of the same lookup failing, `?` here aborts Reset outright: // every step after this one needs Docker too, so there is no useful // partial progress to make without it. + // Closed for the stored id unconditionally, then again for the resolved + // one if it differs — see the matching comment in `remove_project` for + // why the stale-id race can leave sessions open under either identity. + if let Some(ref stored_id) = project.container_id { + state.exec_manager.close_sessions_for_container(stored_id).await; + } let container_ref = docker::find_existing_container(&project).await?; if let Some(ref container_id) = container_ref { - state.exec_manager.close_sessions_for_container(container_id).await; + if project.container_id.as_deref() != Some(container_id.as_str()) { + state.exec_manager.close_sessions_for_container(container_id).await; + } let _ = docker::stop_container(container_id).await; docker::remove_container(container_id).await?; state.projects_store.set_container_id(&project_id, None)?; diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index b9a92e6..2bbe8f1 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -250,6 +250,7 @@ pub fn run() { // an image open and the sweep will not force; pins are untagged // second so the images they were holding are dangling by the time // the sweep lists them; the sweep runs last and collects both. + let projects_store_for_cleanup = projects_store_setup.clone(); tauri::async_runtime::spawn(async move { crate::docker::reap_probe_containers().await; let reaped = crate::docker::reap_stale_migration_pins().await; @@ -259,8 +260,13 @@ pub fn run() { crate::docker::sweep_orphaned_snapshots_logged("startup").await; // A container/image/volume `remove_project` could not delete // is recorded rather than lost — see triple-c#31 — and this is - // the only place anything ever retries it. - crate::commands::project_commands::retry_pending_cleanup_logged().await; + // the only place anything ever retries it. Takes the store so + // it can refuse to touch a project that turns out to still be + // live — see the long comment on the function itself. + crate::commands::project_commands::retry_pending_cleanup_logged( + &projects_store_for_cleanup, + ) + .await; }); // Auto-start web terminal server if enabled in settings diff --git a/app/src-tauri/src/storage/pending_cleanup.rs b/app/src-tauri/src/storage/pending_cleanup.rs index c544884..85e89ee 100644 --- a/app/src-tauri/src/storage/pending_cleanup.rs +++ b/app/src-tauri/src/storage/pending_cleanup.rs @@ -295,6 +295,28 @@ mod tests { fs::remove_dir_all(&dir).ok(); } + /// A record that fails to parse is moved aside once, rather than left in + /// place to be re-warned about — and re-warned about — on every future + /// launch forever. + #[test] + fn an_unparseable_record_is_moved_aside_exactly_once() { + let dir = temp_dir("corrupt-aside"); + let bad = dir.join("bad.json"); + fs::write(&bad, "{ not json").unwrap(); + + list_in(&dir); + assert!(!bad.exists(), "the bad file should have been moved aside"); + let corrupt = dir.join("bad.json.corrupt"); + assert!(corrupt.exists(), "and the moved copy should be at .json.corrupt"); + + // A second pass must not warn about `bad.json` again — it is gone — + // and must not choke on `.json.corrupt` already being there. + assert!(list_in(&dir).is_empty()); + assert!(corrupt.exists(), "the aside copy is not itself deleted"); + + fs::remove_dir_all(&dir).ok(); + } + /// `list_in` must not pick up the `.json.tmp` staging file `save_in` /// leaves behind if a crash lands between the write and the rename — the /// whole point of the temp-then-rename dance is that only the renamed diff --git a/app/src/components/projects/home/ProjectHome.tsx b/app/src/components/projects/home/ProjectHome.tsx index 907b8a9..024446e 100644 --- a/app/src/components/projects/home/ProjectHome.tsx +++ b/app/src/components/projects/home/ProjectHome.tsx @@ -19,7 +19,7 @@ import ConfigTab from "./ConfigTab"; import FilesTab from "./FilesTab"; import BrowserTab from "./BrowserTab"; import { formatUptime } from "./format"; -import { describeLeftovers, leftoverVerb } from "./removalReport"; +import { describeLeftovers, leftoverPronoun, leftoverVerb } from "./removalReport"; const TABS = [ { id: "overview", label: "Overview" }, @@ -299,7 +299,7 @@ export default function ProjectHome({ projectId, active }: Props) { useAppState.getState().pushToast({ kind: "error", message: `“${project.name}” was removed, but Triple-C could not confirm its Docker resources were removed`, - detail: `Triple-C could not confirm ${describeLeftovers(report)} ${verb} removed, and could not record this for a retry. You may need to remove them manually (\`docker rm\` / \`docker rmi\` / \`docker volume rm\`).`, + detail: `Triple-C could not confirm ${describeLeftovers(report)} ${verb} removed, and could not record this for a retry. You may need to remove ${leftoverPronoun(report)} manually (\`docker rm\` / \`docker rmi\` / \`docker volume rm\`).`, }); } } diff --git a/app/src/components/projects/home/removalReport.ts b/app/src/components/projects/home/removalReport.ts index dc0367f..52f9aa4 100644 --- a/app/src/components/projects/home/removalReport.ts +++ b/app/src/components/projects/home/removalReport.ts @@ -18,9 +18,22 @@ export function describeLeftovers(report: ProjectRemovalReport): string { return parts.join(", "); } +/** How many distinct things `describeLeftovers` is describing — a container + * and an image each count as one, however many volumes are named. Shared by + * `leftoverVerb` and `leftoverPronoun` so the two can never disagree about + * singular vs. plural. */ +function leftoverCount(report: ProjectRemovalReport): number { + return (report.container ? 1 : 0) + (report.image ? 1 : 0) + report.volumes.length; +} + /** Verb agreement for `describeLeftovers`'s output — "its container" needs * "was", "its container, a volume" needs "were". */ export function leftoverVerb(report: ProjectRemovalReport): "was" | "were" { - const count = (report.container ? 1 : 0) + (report.image ? 1 : 0) + report.volumes.length; - return count === 1 ? "was" : "were"; + return leftoverCount(report) === 1 ? "was" : "were"; +} + +/** Pronoun agreement for referring back to `describeLeftovers`'s output — + * "remove it manually" for one thing, "remove them manually" for more. */ +export function leftoverPronoun(report: ProjectRemovalReport): "it" | "them" { + return leftoverCount(report) === 1 ? "it" : "them"; } diff --git a/app/src/hooks/useProjectActions.ts b/app/src/hooks/useProjectActions.ts index 9f81e8f..a8102d1 100644 --- a/app/src/hooks/useProjectActions.ts +++ b/app/src/hooks/useProjectActions.ts @@ -3,6 +3,7 @@ import { save } from "@tauri-apps/plugin-dialog"; import type { Project } from "../lib/types"; import * as commands from "../lib/tauri-commands"; import { formatBytes } from "../lib/formatBytes"; +import { describeResetLeftovers, resetLeftoverPronoun } from "../lib/resetOutcome"; import { useAppState } from "../store/appState"; import { useProjects } from "./useProjects"; import { useTerminal } from "./useTerminal"; @@ -58,12 +59,7 @@ export function useProjectActions(project: Project) { () => run("Reset", async () => { const outcome = await rebuild(project.id); - const { leftover_image, leftover_volumes } = outcome; - if (leftover_image || leftover_volumes.length > 0) { - const parts: string[] = []; - if (leftover_image) parts.push("its previous container image"); - if (leftover_volumes.length === 1) parts.push("a volume"); - else if (leftover_volumes.length > 1) parts.push(`${leftover_volumes.length} volumes`); + if (outcome.leftover_image || outcome.leftover_volumes.length > 0) { // Not "run `docker volume rm`" — by the time this renders, the new // container this same call just started already has the leftover // volume mounted, so that command would just hit the same 409 @@ -71,9 +67,9 @@ export function useProjectActions(project: Project) { pushToast({ kind: "error", message: `Reset for “${project.name}” did not fully clean up`, - detail: `Triple-C could not remove ${parts.join(" and ")} from before the reset, so \ + detail: `Triple-C could not remove ${describeResetLeftovers(outcome)} from before the reset, so \ the new container may still be built from, or contain, old data. Stop the project, then try \ -Reset again, or remove ${parts.length > 1 ? "them" : "it"} manually once stopped.`, +Reset again, or remove ${resetLeftoverPronoun(outcome)} manually once stopped.`, }); } return outcome; diff --git a/app/src/hooks/useProjects.test.ts b/app/src/hooks/useProjects.test.ts index 1130479..3a2cded 100644 --- a/app/src/hooks/useProjects.test.ts +++ b/app/src/hooks/useProjects.test.ts @@ -140,3 +140,27 @@ describe("useProjects puts the status back when a refused command never ran", () expect(statusOf()).toBe("stopped"); }); }); + +describe("useProjects.rebuild on success", () => { + it("puts the outcome's project, not the whole outcome, into the list", async () => { + const rebuilt = project("running"); + rebuildProjectContainer.mockResolvedValue({ + project: rebuilt, + leftover_image: null, + leftover_volumes: [], + }); + + const { result } = renderHook(() => useProjects()); + let outcome!: Awaited>; + await act(async () => { + outcome = await result.current.rebuild("p1"); + }); + + // A regression here would put the `{ project, leftover_image, + // leftover_volumes }` wrapper into the projects list instead of the + // `Project` it wraps — a shape mismatch `tsc` would not catch inside a + // callback typed to take `unknown` per Tauri's `invoke`. + expect(useAppState.getState().projects.find((p) => p.id === "p1")).toEqual(rebuilt); + expect(outcome.leftover_volumes).toEqual([]); + }); +}); diff --git a/app/src/lib/resetOutcome.test.ts b/app/src/lib/resetOutcome.test.ts new file mode 100644 index 0000000..1307f9b --- /dev/null +++ b/app/src/lib/resetOutcome.test.ts @@ -0,0 +1,39 @@ +import { describe, it, expect } from "vitest"; +import { describeResetLeftovers, resetLeftoverPronoun } from "./resetOutcome"; +import type { ProjectResetOutcome } from "./types"; + +function outcome(overrides: Partial = {}): ProjectResetOutcome { + return { + project: {} as ProjectResetOutcome["project"], + leftover_image: null, + leftover_volumes: [], + ...overrides, + }; +} + +describe("describeResetLeftovers", () => { + it("names the image first, then the volumes", () => { + expect(describeResetLeftovers(outcome({ leftover_image: "x" }))).toBe( + "its previous container image", + ); + expect(describeResetLeftovers(outcome({ leftover_volumes: ["v1"] }))).toBe("a volume"); + expect(describeResetLeftovers(outcome({ leftover_volumes: ["v1", "v2"] }))).toBe("2 volumes"); + expect( + describeResetLeftovers(outcome({ leftover_image: "x", leftover_volumes: ["v1", "v2"] })), + ).toBe("its previous container image and 2 volumes"); + }); +}); + +describe("resetLeftoverPronoun", () => { + it("is singular for exactly one leftover", () => { + expect(resetLeftoverPronoun(outcome({ leftover_image: "x" }))).toBe("it"); + expect(resetLeftoverPronoun(outcome({ leftover_volumes: ["v1"] }))).toBe("it"); + }); + + it("is plural once more than one thing survived", () => { + expect(resetLeftoverPronoun(outcome({ leftover_image: "x", leftover_volumes: ["v1"] }))).toBe( + "them", + ); + expect(resetLeftoverPronoun(outcome({ leftover_volumes: ["v1", "v2"] }))).toBe("them"); + }); +}); diff --git a/app/src/lib/resetOutcome.ts b/app/src/lib/resetOutcome.ts new file mode 100644 index 0000000..f2a2bf7 --- /dev/null +++ b/app/src/lib/resetOutcome.ts @@ -0,0 +1,32 @@ +import type { ProjectResetOutcome } from "./types"; + +/** + * Names what a `ProjectResetOutcome` says Reset could not clear, for + * `useProjectActions`'s Reset toast. + * + * The image is named first and phrased as "its previous container image" + * rather than folded in with the volumes — it is the more serious of the + * two: the new container is built from it whenever it exists, so a + * surviving image means Reset silently rebuilt the exact system layer it + * was asked to discard, while a surviving volume only means old data rides + * along. + */ +export function describeResetLeftovers(outcome: ProjectResetOutcome): string { + const parts: string[] = []; + if (outcome.leftover_image) parts.push("its previous container image"); + if (outcome.leftover_volumes.length === 1) parts.push("a volume"); + else if (outcome.leftover_volumes.length > 1) parts.push(`${outcome.leftover_volumes.length} volumes`); + return parts.join(" and "); +} + +/** How many distinct things `describeResetLeftovers` is describing — the + * image counts as one, however many volumes are named alongside it. */ +function resetLeftoverCount(outcome: ProjectResetOutcome): number { + return (outcome.leftover_image ? 1 : 0) + outcome.leftover_volumes.length; +} + +/** Pronoun agreement for referring back to `describeResetLeftovers`'s + * output — "remove it manually" for one thing, "remove them" for more. */ +export function resetLeftoverPronoun(outcome: ProjectResetOutcome): "it" | "them" { + return resetLeftoverCount(outcome) === 1 ? "it" : "them"; +} -- 2.52.0