diff --git a/app/src-tauri/src/commands/migration_commands.rs b/app/src-tauri/src/commands/migration_commands.rs index 1327f15..a9ee7a2 100644 --- a/app/src-tauri/src/commands/migration_commands.rs +++ b/app/src-tauri/src/commands/migration_commands.rs @@ -227,58 +227,40 @@ async fn container_label(container_id: &str, label: &str) -> Option { // Migrate // ───────────────────────────────────────────────────────────────────────────── -/// Project ids with a migration running **in this process right now**. -/// -/// Two things need it. `reconcile_project_statuses` is callable from the -/// frontend at any time, not only at startup, and a live migration looks -/// exactly like a crashed one from the outside (state file says `in-progress`, -/// container carries the label) — without this guard a reconcile mid-run would -/// rewrite the phase to `interrupted` underneath a migration that is fine. -/// It also makes a second concurrent `migrate_project_to_base` for the same -/// project impossible. -static ACTIVE_MIGRATIONS: std::sync::OnceLock< - std::sync::Mutex>, -> = std::sync::OnceLock::new(); - -fn active_migrations() -> &'static std::sync::Mutex> { - ACTIVE_MIGRATIONS.get_or_init(|| std::sync::Mutex::new(std::collections::HashSet::new())) -} - /// Whether a migration for this project is running **in this process right /// now**. Every command that stops, removes or recreates the project's /// container has to consult it: the window between `remove_container` and the /// create that follows looks exactly like "no container", and an ordinary /// Start landing in it creates a second container under the same name. +/// +/// **This is now a view onto [`crate::project_lock`], not a set of its own.** +/// It used to be the app's only mutual-exclusion primitive, and it was one-way: +/// a migration claimed a project, everything else merely polled this once at +/// entry and never claimed anything. Two non-migration writers of +/// `triple-c-snapshot-{id}:latest` — a compaction and a recreate — could not +/// see each other at all. Folding the set into the shared registry means there +/// is exactly one answer to "is something happening to this project", and this +/// function is the specialisation of it that reconcile still needs: a *live* +/// migration is indistinguishable from a crashed one from the outside, and only +/// this process knows which it is looking at. pub(crate) fn is_migrating(project_id: &str) -> bool { - active_migrations() - .lock() - .unwrap_or_else(|e| e.into_inner()) - .contains(project_id) + crate::project_lock::is_held_by(project_id, crate::project_lock::ProjectOp::Migration) } -/// RAII marker: removes the project from [`ACTIVE_MIGRATIONS`] however the -/// migration ends, including an early `?`. -struct ActiveGuard(String); +/// RAII marker: releases the project's [`crate::project_lock`] claim however +/// the migration ends, including an early `?`. +/// +/// Kept as a named type rather than using [`crate::project_lock::ProjectGuard`] +/// directly so the migration path keeps reading as "take the migration guard", +/// and so the one place that decides what a migration's claim *is* stays here. +struct ActiveGuard(#[allow(dead_code)] crate::project_lock::ProjectGuard); impl ActiveGuard { - /// `None` when a migration is already running for this project. + /// `None` when a migration — or anything else — already holds this project. fn acquire(project_id: &str) -> Option { - let mut set = active_migrations() - .lock() - .unwrap_or_else(|e| e.into_inner()); - if !set.insert(project_id.to_string()) { - return None; - } - Some(Self(project_id.to_string())) - } -} - -impl Drop for ActiveGuard { - fn drop(&mut self) { - active_migrations() - .lock() - .unwrap_or_else(|e| e.into_inner()) - .remove(&self.0); + crate::project_lock::try_acquire(project_id, crate::project_lock::ProjectOp::Migration) + .ok() + .map(Self) } } @@ -1163,7 +1145,23 @@ pub(crate) async fn purge_migration_artifacts(project_id: &str) { } } } - Ok(None) => return, + Ok(None) => { + // **`Ok(None)` is not the same as "no file".** `migration_store::load` + // now reports an *unparseable* record as absent while deliberately + // leaving it on disk, so that `has_record` goes on protecting the + // rollback pin it describes. Returning here on that would leave the + // file — and therefore a permanently "claimed" pin — behind a Reset + // that has just deleted the snapshot and both volumes the record + // could possibly refer to. + if !migration_store::has_record(project_id).unwrap_or(false) { + return; + } + log::warn!( + "Project {} has a migration record that could not be read; removing it anyway \ + because a Reset supersedes it", + project_id + ); + } Err(e) => log::warn!( "Could not read the migration record for {} while cleaning up: {}", project_id, @@ -1172,6 +1170,10 @@ pub(crate) async fn purge_migration_artifacts(project_id: &str) { } let _ = migration_store::clear_staging(project_id); let _ = migration_store::clear(project_id); + // The pins this project had are gone with the snapshot; their grace clocks + // are meaningless and would otherwise sit in the migrations directory + // forever. + migration_store::clear_ownerless_for_project(project_id); } fn default_docker_socket() -> String { diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 09030ef..0473b64 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -221,20 +221,35 @@ pub async fn start_project_container( app_handle: tauri::AppHandle, state: State<'_, AppState>, ) -> Result { - // A migration removes the container and creates its replacement moments - // later. Starting in that window finds no container, creates a second one - // under the same name, and the migration's own create then fails on the - // name conflict — which sends it into an auto-rollback that also cannot - // create. The UI already refuses (`canMigrate` gates on the container being - // stopped and no run being in flight); this is the same gate on the side - // that actually owns the invariant. - if crate::commands::migration_commands::is_migrating(&project_id) { - return Err( - "A container base update is running for this project. Wait for it to finish, then start the project." - .to_string(), - ); - } + // **Acquired, not polled.** A migration removes the container and creates + // its replacement moments later. Starting in that window finds no + // container, creates a second one under the same name, and the migration's + // own create then fails on the name conflict — which sends it into an + // auto-rollback that also cannot create. This used to be a one-shot + // `is_migrating` read, which covered that case and no other: a start also + // commits `triple-c-snapshot-{id}:latest`, so it races a *compaction* + // committing the same tag with nothing between them. The claim is held for + // the whole start rather than checked at its door. + // + // The UI already refuses (`canMigrate` gates on the container being stopped + // and no run being in flight); this is the same gate on the side that + // actually owns the invariant. + let _guard = + crate::project_lock::try_acquire(&project_id, crate::project_lock::ProjectOp::Recreate)?; + start_project_container_locked(project_id, app_handle, state).await +} +/// The body of [`start_project_container`], with **no claim of its own**. +/// +/// Split out for exactly one caller: [`rebuild_project_container`] already +/// holds the project under [`crate::project_lock::ProjectOp::Reset`] for its +/// whole run, and a Reset that then went through the public command would be +/// refused by its own guard. Every other path must go through the command. +async fn start_project_container_locked( + project_id: String, + app_handle: tauri::AppHandle, + state: State<'_, AppState>, +) -> Result { let mut project = state .projects_store .get(&project_id) @@ -539,6 +554,14 @@ pub async fn stop_project_container( app_handle: tauri::AppHandle, state: State<'_, AppState>, ) -> Result<(), String> { + // Stop had **no** exclusion at all, which is the one gap CLAUDE.md's "every + // command that stops, removes or recreates the container consults + // `is_migrating`" rule already named and this function did not honour. A + // stop lands on the container a migration is mid-swap on, and it closes the + // exec sessions a compaction's config replay is not expecting to lose. + let _guard = + crate::project_lock::try_acquire(&project_id, crate::project_lock::ProjectOp::Recreate)?; + let project = state .projects_store .get(&project_id) @@ -571,13 +594,13 @@ pub async fn rebuild_project_container( ) -> 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. - if crate::commands::migration_commands::is_migrating(&project_id) { - return Err( - "A container base update is running for this project. Wait for it to finish before resetting." - .to_string(), - ); - } + // orphan migration record pointing at images that no longer exist — and + // doing it while a *compaction* is mid-flight is worse, because the + // compaction then commits `flat(old)` back over the `:latest` this just + // destroyed and resurrects the system layer the user asked to be rid of. + // Held for the whole Reset, including the start at the end of it. + let _guard = + crate::project_lock::try_acquire(&project_id, crate::project_lock::ProjectOp::Reset)?; let project = state .projects_store @@ -611,8 +634,9 @@ pub async fn rebuild_project_container( log::warn!("Failed to remove project volumes for project {}: {}", project_id, e); } - // Start fresh - start_project_container(project_id, app_handle, state).await + // 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 } /// Reconcile project statuses against actual Docker container state. @@ -656,9 +680,14 @@ pub async fn reconcile_project_statuses( ) { continue; } - // ...but never for a project this process is actively migrating: the - // container is legitimately absent for part of that run. - if crate::commands::migration_commands::is_migrating(&project.id) { + // ...but never for a project this process is actively working on. A + // migration's container is legitimately absent between the + // `remove_container` and the create that follows, and so is a Reset's, + // and so is a start's before its create returns. Reconciling into any + // of those windows writes `Stopped` over a project that is mid-run. + // Broadened from `is_migrating` to the whole lock for exactly that + // reason — the migration was never the only operation with a gap. + if crate::project_lock::held(&project.id).is_some() { continue; } diff --git a/app/src-tauri/src/docker/disk.rs b/app/src-tauri/src/docker/disk.rs index 43d7b65..f1007bc 100644 --- a/app/src-tauri/src/docker/disk.rs +++ b/app/src-tauri/src/docker/disk.rs @@ -75,6 +75,23 @@ use crate::storage::migration_store; /// its warm cache and short enough that abandoned trees are collected. pub const BUILD_CACHE_DEFAULT_UNTIL_HOURS: i64 = 168; +/// How long [`docker_cli`] waits for the `docker` command line tool. +/// +/// Long enough for `buildx du` over a large build tree on a cold daemon, short +/// enough that a wedged daemon does not hold the Scan button down forever. A +/// prune uses the same bound; a prune that outruns it has still done its work +/// on the daemon, and the next scan reports the result. +const DOCKER_CLI_TIMEOUT_SECS: u64 = 45; + +/// The bound for a `builder prune`, which is a different kind of wait. +/// +/// `buildx du` is a query and 45 seconds is generous for it. A prune of a +/// 60 GB cache genuinely takes minutes, and cancelling it does not undo the +/// daemon-side work — it only loses the `Total reclaimed space:` line, so the +/// run would be reported as a failure that in fact freed the space. Ten minutes +/// is a bound against a wedged daemon rather than against a slow one. +const DOCKER_PRUNE_TIMEOUT_SECS: u64 = 600; + // --------------------------------------------------------------------------- // Scan result // --------------------------------------------------------------------------- @@ -123,6 +140,27 @@ pub struct ProjectDiskRow { pub home_volume_present: bool, pub config_volume_bytes: i64, pub config_volume_present: bool, + /// **The one snapshot figure the row adds up from.** + /// + /// The Snapshot column shows this and [`Self::total_bytes`] is computed + /// from it, so the Total column reconciles with its parts. It did not + /// before: `total_bytes` used `size - shared_size` unconditionally while + /// the column fell back to [`Self::snapshot_above_base_bytes`] or to `—`, + /// and in that fallback branch `size - shared_size` is the *whole base + /// image*. A row could show `—` for its snapshot and still carry 4.7 GB of + /// base in its total, which `triple_c_total_bytes` then added again as a + /// base-image row. + /// + /// The rule, in order: + /// + /// 1. `df()` computed a shared size → `size - shared`, the daemon's own + /// measurement of what is unique to this image. + /// 2. No shared size but the base lineage is known → the layer arithmetic + /// in [`layer_stats`]. + /// 3. Neither → the full size. Not a fallback to zero: an image nothing + /// shares with and whose lineage is unknown really does cost its whole + /// size, and a flattened snapshot is exactly that shape. + pub snapshot_attributed_bytes: i64, pub total_bytes: i64, /// A migration is in flight; every action on this row is blocked. pub migrating: bool, @@ -293,8 +331,6 @@ pub enum ReclaimTarget { ProbeContainers, /// `triple-c-scrub-*` containers left by an interrupted secret rewrite. ScrubContainers, - /// One orphaned volume, ticked individually by name. - OrphanVolume { name: String }, /// Rewrite a project's stacked commit layers into a single layer. The /// highest-yield action in this module. CompactSnapshot { project_id: String }, @@ -325,6 +361,27 @@ pub enum DestructiveTarget { /// A rollback pin whose migration is still awaiting confirmation — the only /// copy of that migration's rollback target. RollbackPin { project_id: String, tag: String }, + /// A `triple-c-home-*` / `triple-c-claude-config-*` volume whose project id + /// is in no `projects.json` this app can find. + /// + /// **It was a `ReclaimTarget` at `Safety::Safe`** — a tick and the group + /// Reclaim button, no confirmation. The object behind that tick is a + /// `triple-c-claude-config-*` volume holding a Claude OAuth credential, + /// every installed plugin and skill, and every conversation transcript the + /// project ever had, and the *same volume* for a project still in the store + /// requires typing the project's name. The only difference between the two + /// is a lookup against a file this app has been wrong about before: a + /// second app instance's project is absent from an in-memory list, a + /// corrupt `projects.json` empties it, and a data directory restored + /// without it empties it too. So it is confirmed like everything else that + /// has no other copy — see [`destroy`], where the typed string is the + /// **volume name**, there being no project name to type. + OrphanVolume { + name: String, + /// The id parsed out of the volume name. Display only — it names no + /// project in the store, which is the whole reason this variant exists. + project_id: String, + }, } impl ReclaimTarget { @@ -339,7 +396,6 @@ impl ReclaimTarget { | ReclaimTarget::MigrationStaging | ReclaimTarget::ProbeContainers | ReclaimTarget::ScrubContainers - | ReclaimTarget::OrphanVolume { .. } | ReclaimTarget::BuildCache { .. } => Safety::Safe, // A rewrite and a cache flush: nothing is lost, but time is. ReclaimTarget::CompactSnapshot { .. } | ReclaimTarget::ClearCaches { .. } => { @@ -373,9 +429,11 @@ impl DestructiveTarget { DestructiveTarget::HomeVolume { project_id } | DestructiveTarget::ConfigVolume { project_id } | DestructiveTarget::SnapshotImage { project_id } - | DestructiveTarget::RollbackPin { project_id, .. } => project_id, + | DestructiveTarget::RollbackPin { project_id, .. } + | DestructiveTarget::OrphanVolume { project_id, .. } => project_id, } } + } /// One offered action, with its measured cost. @@ -577,6 +635,33 @@ pub fn parse_project_volume_name(name: &str) -> Option<(&str, &'static str)> { None } +/// This project's share of its snapshot image, in bytes. +/// +/// The single rule behind [`ProjectDiskRow::snapshot_attributed_bytes`], pulled +/// out of [`scan`] so it can be tested without a daemon — the bug it fixes was +/// two call sites disagreeing, and a rule that lives in one function cannot +/// disagree with itself. +/// +/// 1. `df()` computed a shared size → `size - shared`. The daemon's own +/// measurement of what is unique to this image, and the only exact answer +/// available. +/// 2. No shared size, but the base lineage is known → the layer arithmetic from +/// [`layer_stats`]. +/// 3. Neither → `size - shared`, which with no shared size is the full image. +/// Deliberately not zero: an image that shares nothing measurable really +/// does cost its whole size, and a flattened snapshot is exactly that shape. +pub fn snapshot_attribution( + snapshot_bytes: i64, + snapshot_shared_bytes: i64, + above_base_bytes: Option, +) -> i64 { + let unique = (snapshot_bytes - snapshot_shared_bytes.max(0)).max(0); + if snapshot_shared_bytes > 0 { + return unique; + } + above_base_bytes.map(|b| b.max(0)).unwrap_or(unique) +} + /// What a snapshot's layer stack looks like relative to its base. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] pub struct LayerStats { @@ -795,34 +880,85 @@ pub fn parse_reclaimed_space(output: &str) -> i64 { /// never match, and the flattened intermediate is left to whatever `untag_image` /// happens to delete on its own. pub fn compaction_dockerfile(snapshot_ref: &str, scrub_script: &str) -> String { - // The scrub script is multi-line shell. `RUN` takes it verbatim only if the - // newlines are escaped, so it is folded onto one line with `;` separators — - // the script is already a sequence of statements and a `for` loop, both of - // which survive that. - let folded = fold_shell_script(scrub_script); format!( "FROM {snapshot_ref} AS src\n\ - RUN {folded}\n\ + RUN {run}\n\ FROM scratch\n\ COPY --from=src / /\n\ - LABEL {LABEL_MANAGED}=true\n" + LABEL {LABEL_MANAGED}=true\n", + run = run_exec_form(scrub_script) ) } -/// Collapse a multi-line `/bin/sh` program into a single `RUN` line. +/// Render a multi-line `/bin/sh` program as a `RUN` the daemon will actually +/// execute. /// -/// Blank lines go; every other line is joined with a space. The script's own -/// syntax already terminates its statements (`;` inside the `for`, newlines -/// after each simple command are not load-bearing because each line here is a -/// complete word sequence), so this is a join and not a rewrite — but it is -/// pinned by a test against the real script for exactly that reason. -fn fold_shell_script(script: &str) -> String { - script - .lines() - .map(str::trim) - .filter(|line| !line.is_empty()) - .collect::>() - .join(" ") +/// ## What was here before, and why it never worked once +/// +/// The scrub script is multi-line shell, and a Dockerfile instruction does not +/// continue over a bare newline — so the script was folded onto one line by +/// joining its lines **with a space**. That is not a rewrite the shell +/// tolerates. `for p in …` on one line and `do` on the next are separated by a +/// newline that *is* load-bearing; joining them produces +/// `… for p in …; do [ -e "$p" ] || continue sz=$(…) …`, and `sh` stops at: +/// +/// ```text +/// /bin/sh: line 0: syntax error: unexpected "do" +/// ERROR: process "/bin/sh -c total=0 for p in …" did not complete successfully: exit code: 2 +/// ``` +/// +/// Verified with a real `docker build`, not reasoned about. The build failed on +/// its first stage every single time, so `compact_snapshot` has always returned +/// a failure — the headline action of the whole panel, broken since it landed. +/// Nothing was lost, because the failure is before anything is removed, but +/// nothing was ever reclaimed either. The test that was supposed to catch this +/// asserted only that the `RUN` was *one line*, which the broken fold satisfied +/// perfectly. +/// +/// ## Why the JSON exec form rather than a better fold +/// +/// Any fold is a rewrite of somebody else's shell, and the script is not this +/// module's to own — `container::snapshot_scrub_script` is free to grow a +/// `case`, an `if`, a heredoc or a function, and each of those breaks a +/// different set of join rules. Inserting `;` between lines is wrong for +/// exactly the same reason a space was: `; do` is fine, but `if x; then; y` is +/// not. +/// +/// So the script is not transformed at all. `RUN ["/bin/sh", "-c", "