diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 564012f..4768638 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, ProjectResetOutcome, 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,76 @@ 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) { - if let Some(ref container_id) = project.container_id { - state.exec_manager.close_sessions_for_container(container_id).await; + // 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 { + // 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. + // + // 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) => { + 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 { + 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; - 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 + ); + // 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()); + } } // Legacy MCP cleanup (pre-MCP-removal installs): drop any leftover MCP @@ -738,10 +802,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 +812,216 @@ 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(()) => { + report.retry_scheduled = true; + log::warn!( + "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; 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 + ); + } + } + } + + // 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. + // + // 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() { + 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); + } + 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. +/// +/// 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; + } + + let mut cleaned = 0usize; + 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(()) => {} + 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; + // `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. + 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`", + 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!( + "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 + ); +} + +/// 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; + +/// 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] @@ -1226,7 +1498,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 @@ -1253,25 +1525,58 @@ 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 { - state.exec_manager.close_sessions_for_container(container_id).await; + // 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. 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. + // 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 { + 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)?; } - // 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)); } - 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_image.is_some() || !leftover_volumes.is_empty() { + log::warn!( + "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. - 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_image, leftover_volumes }) } /// Reconcile project statuses against actual Docker container state. @@ -1379,6 +1684,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/docker/container.rs b/app/src-tauri/src/docker/container.rs index a48cf72..038e837 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,91 @@ 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 remove_one_volume_with_retry(&docker, vol).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 +} + +/// 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 diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 24b6bae..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; @@ -257,6 +258,15 @@ 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. 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/models/project.rs b/app/src-tauri/src/models/project.rs index dfa8f38..5c22e15 100644 --- a/app/src-tauri/src/models/project.rs +++ b/app/src-tauri/src/models/project.rs @@ -422,6 +422,61 @@ 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. 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 { + /// True when nothing was left behind. + pub fn is_clean(&self) -> bool { + self.container.is_none() && self.image.is_none() && self.volumes.is_empty() + } +} + +/// What `rebuild_project_container` (Reset) produced: the project as it +/// stands after restarting, and anything Reset could not clear. +/// +/// 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, +} + /// 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 +705,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..85e89ee --- /dev/null +++ b/app/src-tauri/src/storage/pending_cleanup.rs @@ -0,0 +1,349 @@ +//! 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. +//! +//! **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::{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, + /// 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, + 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() +} + +/// Write (or overwrite) a project's pending-cleanup record. +pub fn save(record: &PendingCleanup) -> Result<(), String> { + 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> { + clear_in(&dir()?, project_id) +} + +/// 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() }; + 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() + .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) => { + // 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 {}: {}{}", + path.display(), + err, + if moved { + format!(" — moved aside to {}", corrupt.display()) + } else { + " — leaving it in place".to_string() + } + ); + None + } + } + }) + .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_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("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(), + } + } + + #[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()); + } + + /// 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_dir("roundtrip"); + let rec = record("proj-1"); + + 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()]); + + clear_in(&dir, "proj-1").expect("clear"); + assert!(list_in(&dir).is_empty()); + + 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(); + } + + /// 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_dir("corrupt"); + fs::write(dir.join("bad.json"), "{ not json").unwrap(); + save_in(&dir, &record("proj-2")).expect("save"); + + let found = list_in(&dir); + assert_eq!(found.len(), 1); + assert_eq!(found[0].project_id, "proj-2"); + + 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 + /// 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 9245f4d..024446e 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 { projectRemovalIsClean } from "../../../lib/types"; import { useAppState } from "../../../store/appState"; import { useProjectActions } from "../../../hooks/useProjectActions"; import { useProjects } from "../../../hooks/useProjects"; @@ -18,6 +19,7 @@ import ConfigTab from "./ConfigTab"; import FilesTab from "./FilesTab"; import BrowserTab from "./BrowserTab"; import { formatUptime } from "./format"; +import { describeLeftovers, leftoverPronoun, leftoverVerb } from "./removalReport"; const TABS = [ { id: "overview", label: "Overview" }, @@ -282,7 +284,25 @@ export default function ProjectHome({ projectId, active }: Props) { onConfirm={async () => { setConfirmRemove(false); try { - await remove(project.id); + 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 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 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 ${leftoverPronoun(report)} manually (\`docker rm\` / \`docker rmi\` / \`docker volume rm\`).`, + }); + } + } } catch (e) { useAppState.getState().pushToast({ kind: "error", 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..52f9aa4 --- /dev/null +++ b/app/src/components/projects/home/removalReport.ts @@ -0,0 +1,39 @@ +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(", "); +} + +/** 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" { + 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 07ded95..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"; @@ -28,13 +29,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 +56,25 @@ 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_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 + // Reset did. Stopping the project first is what actually frees it. + pushToast({ + kind: "error", + message: `Reset for “${project.name}” did not fully clean up`, + 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 ${resetLeftoverPronoun(outcome)} manually once stopped.`, + }); + } + return outcome; + }), + [run, rebuild, project.id, project.name, pushToast], ); const openClaudeTerminal = useCallback(async () => { 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/hooks/useProjects.ts b/app/src/hooks/useProjects.ts index 66868a0..4b88c5e 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], ); @@ -135,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/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"; +} diff --git a/app/src/lib/tauri-commands.ts b/app/src/lib/tauri-commands.ts index f9bb0fe..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, 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"); @@ -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) => @@ -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 cc394d6..9a6462c 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -77,6 +77,37 @@ export type ProjectStatus = | "stopping" | "error"; +/** What `removeProject` could not delete. The project is removed from the + * 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; + /** 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[]; +} + export type Backend = | "anthropic" | "bedrock"