Take the review: cache the stopped probe, and never let it cost an answer

Six findings, all real. The one that mattered: `getContainerStaleness` is
called from a `useEffect` that fires whenever the container settles, so
merely opening a stopped project's Overview now committed its whole writable
layer — 44 s on a real project, against ~3 s for the snapshot probe it
replaced. Shipping that would have traded one bad banner for a bad page.

A stopped container's writable layer cannot change, so the probe is exactly
cacheable: `STOPPED_MANIFEST_CACHE` keys on the container's `FinishedAt`,
which moves on every stop. Cold 2967 ms, warm 1 ms, measured. A live test
asserts the restart case as well as the hit, because a cache that failed to
invalidate would plan a migration against a filesystem the project no longer
has — verified by breaking the token and watching that assertion fail.

Skipping the probe for projects that are not stale looked like the cheaper
fix and is unsafe: the deltas would be empty while `probeSettled` stayed
true, and the migrate action in the project menu is not gated on the banner,
so the pre-flight would report nothing to copy while the backend was told to
copy nothing. That is the hazard `canMigrate`'s comment already warns about.
Not done, and written down so it is not tried again.

Also from the review:

- A failed commit no longer costs an answer the snapshot could have given.
  Before this feature a stopped project read its snapshot directly, so
  surfacing this error would have made the banner worse than it was — and
  the failure modes are where the fallback earns its keep: a full disk (the
  commit allocates the whole layer, the snapshot probe allocates nothing)
  and a 409 from a concurrent claim.
- The probe no longer commits while the project is claimed. The collision is
  not symmetric: the probe losing is a retryable `probe_error`, but
  `start_project_container` removes the old container with a hard `?`, so a
  remove that raced a commit would fail the user's Start with an opaque
  error. `stopped_probe_policy` reads `project_lock::held` and probes the
  snapshot instead, or defers with a message that says so.
- The cleanup-failure warning claimed the next probe of the same container
  would reclaim the leftover. Unique names made that false the moment they
  landed; it is `reap_probe_images` that collects it.
- The TS binding still called the command read-only, which is how the
  auto-refresh got added in the first place.
- CLAUDE.md still documented the stable `triple-c-probe-{cid}:latest` name
  this PR removed as unsafe.

548 unit tests, 752 frontend tests, 4 live-Docker tests. Clippy unchanged at
44 warnings.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019RSaoDLovVV2wmH4H8VVxz

CLAUDE.md

@@ -441,7 +441,7 @@ security update. Migration is the non-destructive way out; Reset is the destruct
   or inside a migration. **Never on stop.** So a project in daily use for a year can legitimately
   have no `triple-c-snapshot-{id}:latest` at all, and one that has is stale by everything installed
   since. `pick_probe_source` therefore reads a *stopped* container directly — commit its writable
-  layer to `triple-c-probe-{cid}:latest`, probe that, drop it — and ranks it **above** the snapshot,
+  layer to a unique `triple-c-probe-*` image, probe that, drop it — and ranks it **above** the snapshot,
   for the same reason a running container already outranked it. Assuming a snapshot existed is what
   made a stopped, never-recreated project report "no container or snapshot image yet" with its
   container sitting right there, and left Update disabled on the projects furthest behind.
@@ -466,6 +466,30 @@ security update. Migration is the non-destructive way out; Reset is the destruct
   other was still reading, reporting a bogus `probe_error` on a healthy project. `get_container_staleness`
   takes no `project_lock` claim (the migration banner needs it to answer *during* a migration), so
   uniqueness is what makes overlapping probes safe.
+- **The stopped-container probe is cached per stop, and that is not an optimisation you may drop.**
+  `getContainerStaleness` is called from a `useEffect` that fires whenever the container settles, so
+  merely opening a stopped project's Overview probes it. Uncached that is a `docker commit` of the
+  whole writable layer per visit — measured at 44 s on a real project, against ~3 s for the snapshot
+  probe it replaced. `STOPPED_MANIFEST_CACHE` is keyed on the container's `FinishedAt`, which is
+  exact rather than merely plausible: nothing can write to a stopped container's writable layer, and
+  `FinishedAt` moves on every stop. A live test asserts the restart case, because a cache that
+  failed to invalidate would plan a migration against a filesystem the project no longer has.
+- **Do not "skip the probe when the project is not stale" to save that cost.** It was tried. The
+  deltas would be empty while `probeSettled` (`!probing && staleness && !probe_error`) stayed *true*,
+  which leaves the migrate action in the project menu enabled — that action is not gated on the
+  banner — so the pre-flight would report nothing to copy while the backend was told to copy
+  nothing. That is the exact hazard `ProjectHome.tsx`'s `canMigrate` comment already warns about.
+- **A failed stopped-container probe falls back to the snapshot whenever one exists.** Before this
+  feature a stopped project read its snapshot directly, so surfacing a commit failure where the
+  snapshot could have answered would make the banner *worse* than it was — and the failure modes are
+  exactly the ones where the fallback earns its keep: a full disk (the commit allocates the whole
+  writable layer; the snapshot probe allocates nothing) and a 409 from a concurrent claim.
+- **`get_container_staleness` never commits while the project is claimed.** It takes no
+  `project_lock` claim itself, deliberately — the banner has to answer *during* a migration — so it
+  reads `project_lock::held` instead and probes the snapshot rather than the container. The
+  collision is not symmetric: the probe losing is a retryable `probe_error`, but
+  `start_project_container` removes the old container with a hard `?`, so a remove that raced a
+  commit would fail the user's Start with an opaque error.
 - **An image's `Created` is the image's own, not its tag's.** Tagging an existing image gives you
   that image's age; BuildKit stamps `docker build` output with a fixed epoch. Only `docker commit`
   stamps *now* — which is what real probe images do, and what any fixture for them must do.

app/src-tauri/src/commands/migration_commands.rs

@@ -142,6 +142,45 @@ fn pick_probe_source(container_running: Option<bool>, snapshot_exists: bool) ->
     }
 }
 
+/// Reported as `probe_error` when another operation owns the project and there
+/// is no snapshot image to read instead. Deliberately not a claim about the
+/// container: nothing is wrong with it, the answer is simply not safe to take
+/// right now. See [`stopped_probe_policy`].
+const PROJECT_BUSY: &str = "Another operation is running on this project, so its contents could not be inspected. Try again once it finishes.";
+
+/// What to do about a stopped container, whose probe is the expensive one: it
+/// commits the writable layer before it can read anything.
+#[derive(Debug, PartialEq, Eq)]
+enum StoppedProbe {
+    /// Commit and probe. The current answer, and the default.
+    Commit,
+    /// Probe the snapshot image instead. Less current — it lags the container by
+    /// everything installed since the last commit — but it allocates nothing and
+    /// touches nothing, which is what makes it the right answer while another
+    /// operation owns the container.
+    SnapshotInstead,
+    /// Report rather than guess.
+    Defer,
+}
+
+/// Pick what to do about a stopped container.
+///
+/// **Never commits while the project is claimed.** `get_container_staleness`
+/// takes no [`crate::project_lock`] claim of its own, by design, so a commit
+/// here can overlap a Recreate or Reset — and the collision is not symmetric.
+/// The probe losing is harmless: a surfaced `probe_error` the user retries. The
+/// *recreate* losing is not, because `start_project_container` removes the old
+/// container with a hard `?`, so a non-404 from a remove that raced this commit
+/// fails the whole Start with an opaque "Failed to remove container". Reading
+/// the claim costs nothing and takes that failure off the table.
+fn stopped_probe_policy(project_is_busy: bool, snapshot_exists: bool) -> StoppedProbe {
+    match (project_is_busy, snapshot_exists) {
+        (false, _) => StoppedProbe::Commit,
+        (true, true) => StoppedProbe::SnapshotInstead,
+        (true, false) => StoppedProbe::Defer,
+    }
+}
+
 /// Runs two filesystem probes (~3 s each) and is therefore meant to be called
 /// on demand, not polled.
 ///
@@ -220,7 +259,44 @@ pub async fn get_container_staleness(
     ) {
         (ProbeSource::RunningContainer, Some(id)) => mig::manifest_from_container(id).await,
         (ProbeSource::StoppedContainer, Some(id)) => {
-            mig::manifest_from_stopped_container(id).await
+            let busy = crate::project_lock::held(&project_id).is_some();
+            match stopped_probe_policy(busy, snapshot_exists) {
+                StoppedProbe::Commit => {
+                    match mig::manifest_from_stopped_container_cached(id).await {
+                        Ok(m) => Ok(m),
+                        // **Never let a failed commit cost an answer the
+                        // snapshot could have given.** Before stopped
+                        // containers were readable at all, a stopped project
+                        // fell straight through to its snapshot, so surfacing
+                        // this error where the snapshot exists would make the
+                        // banner *worse* than it was — and the ways this fails
+                        // are the ones where the fallback matters most: a full
+                        // disk (the commit has to allocate the whole writable
+                        // layer; the snapshot probe allocates nothing) and a
+                        // 409 from an operation that claimed the project after
+                        // the check above.
+                        Err(e) if snapshot_exists => {
+                            log::warn!(
+                                "Probing the stopped container for project {} failed ({}) — \
+                                 falling back to its snapshot image, which may lag it",
+                                project_id,
+                                e
+                            );
+                            mig::manifest_from_image(&snapshot_image).await
+                        }
+                        Err(e) => Err(e),
+                    }
+                }
+                StoppedProbe::SnapshotInstead => {
+                    log::info!(
+                        "Project {} is claimed by another operation — probing its snapshot image \
+                         rather than committing the container",
+                        project_id
+                    );
+                    mig::manifest_from_image(&snapshot_image).await
+                }
+                StoppedProbe::Defer => Err(PROJECT_BUSY.to_string()),
+            }
         }
         (ProbeSource::Snapshot, _) => mig::manifest_from_image(&snapshot_image).await,
         // `container_running` is `Some` exactly when `container_id` is, so the
@@ -2073,6 +2149,23 @@ mod tests {
         assert_eq!(pick_probe_source(None, false), ProbeSource::Nothing);
     }
 
+    #[test]
+    fn a_stopped_container_is_committed_only_when_nothing_else_owns_the_project() {
+        assert_eq!(stopped_probe_policy(false, false), StoppedProbe::Commit);
+        assert_eq!(stopped_probe_policy(false, true), StoppedProbe::Commit);
+    }
+
+    #[test]
+    fn a_busy_project_falls_back_rather_than_racing_a_recreate() {
+        // The snapshot lags, but a stale answer beats failing someone's Start.
+        assert_eq!(
+            stopped_probe_policy(true, true),
+            StoppedProbe::SnapshotInstead
+        );
+        // Nothing to fall back to: say so instead of committing anyway.
+        assert_eq!(stopped_probe_policy(true, false), StoppedProbe::Defer);
+    }
+
     #[test]
     fn byte_sizes_read_the_way_a_disk_warning_should() {
         assert_eq!(human_bytes(512), "512 B");

app/src-tauri/src/docker/migration.rs

@@ -1087,6 +1087,89 @@ pub async fn manifest_from_container(container_id: &str) -> Result<Manifest, Str
     Ok(parse_manifest(&out))
 }
 
+/// Cached stopped-container manifests, keyed by container id, each paired with
+/// the container's `FinishedAt` at the time it was captured.
+///
+/// **Sound because a stopped container's writable layer cannot change.** Nothing
+/// can write to it while it is not running, so a manifest captured after it
+/// stopped stays true until it is started again — and `FinishedAt` moves on
+/// every stop, which is what makes the key exact rather than merely plausible.
+///
+/// This exists because `get_container_staleness` is called from a `useEffect`
+/// that fires whenever the container settles, so simply opening a stopped
+/// project's Overview probes it. Uncached that meant a `docker commit` of the
+/// whole writable layer per visit — measured at 44 s on a real project — where
+/// before this feature the same visit cost one throwaway container or nothing at
+/// all. A regression like that is not worth the answer it buys.
+///
+/// Capped, because a `Manifest` of a real container is a few MB: this only has
+/// to serve "the project whose page is open", so a handful of entries is the
+/// whole working set and the oldest is dropped past that.
+static STOPPED_MANIFEST_CACHE: std::sync::Mutex<
+    Option<Vec<(String, String, Manifest)>>,
+> = std::sync::Mutex::new(None);
+
+/// How many stopped-container manifests [`STOPPED_MANIFEST_CACHE`] keeps.
+const STOPPED_MANIFEST_CACHE_MAX: usize = 4;
+
+/// `FinishedAt` for a container, the cache's validity token. `None` when it
+/// cannot be read, which is never treated as a hit.
+async fn container_finished_at(container_id: &str) -> Option<String> {
+    let docker = get_docker().ok()?;
+    docker
+        .inspect_container(container_id, None)
+        .await
+        .ok()?
+        .state?
+        .finished_at
+        .filter(|s| !s.is_empty())
+}
+
+/// Capture a [`Manifest`] from a **stopped** container, reusing a cached one
+/// when the container has not been started since it was taken.
+///
+/// See [`STOPPED_MANIFEST_CACHE`] for why this is exact and why it is needed.
+pub async fn manifest_from_stopped_container_cached(
+    container_id: &str,
+) -> Result<Manifest, String> {
+    let finished_at = container_finished_at(container_id).await;
+
+    if let Some(token) = &finished_at {
+        let guard = STOPPED_MANIFEST_CACHE.lock();
+        if let Ok(cache) = guard {
+            if let Some(entries) = cache.as_ref() {
+                if let Some((_, _, manifest)) = entries
+                    .iter()
+                    .find(|(id, tok, _)| id == container_id && tok == token)
+                {
+                    log::debug!(
+                        "Reusing the cached manifest for stopped container {}",
+                        container_id
+                    );
+                    return Ok(manifest.clone());
+                }
+            }
+        }
+    }
+
+    let manifest = manifest_from_stopped_container(container_id).await?;
+
+    // Only cacheable if the container's state could be read at all; an unknown
+    // `FinishedAt` means there is no token that could later be compared.
+    if let Some(token) = finished_at {
+        if let Ok(mut cache) = STOPPED_MANIFEST_CACHE.lock() {
+            let entries = cache.get_or_insert_with(Vec::new);
+            entries.retain(|(id, _, _)| id != container_id);
+            entries.push((container_id.to_string(), token, manifest.clone()));
+            while entries.len() > STOPPED_MANIFEST_CACHE_MAX {
+                entries.remove(0);
+            }
+        }
+    }
+
+    Ok(manifest)
+}
+
 /// Capture a [`Manifest`] from a **stopped** container.
 ///
 /// Commits the container's writable layer to a throwaway image, probes that,
@@ -1107,8 +1190,8 @@ pub async fn manifest_from_stopped_container(container_id: &str) -> Result<Manif
 
     if let Err(e) = super::container::remove_image_by_name(&image).await {
         log::warn!(
-            "Could not remove the staleness probe's throwaway image {}: {} — the next probe of \
-             this container reuses the name, which leaves this one dangling for the orphan sweep",
+            "Could not remove the staleness probe's throwaway image {}: {} — `reap_probe_images` \
+             collects it at the next app start; the orphan sweep never will, because it is tagged",
             image,
             e
         );
@@ -2273,6 +2356,95 @@ mod tests {
 
     // ── Live Docker ─────────────────────────────────────────────────────────
 
+    /// The cache serves a second read of an unchanged stopped container, and —
+    /// the half that matters — stops serving it the moment the container is
+    /// started and stopped again. If invalidation were wrong this would report a
+    /// filesystem the project no longer has, and a migration would be planned
+    /// against it.
+    ///
+    /// ```text
+    /// cargo test -- --ignored --nocapture stopped_manifest_cache
+    /// ```
+    #[cfg(unix)]
+    #[tokio::test]
+    #[ignore = "needs a Docker daemon; creates, commits and removes a throwaway container"]
+    async fn the_stopped_manifest_cache_survives_a_reread_but_not_a_restart() {
+        fn docker_cli(args: &[&str]) -> String {
+            let out = std::process::Command::new("docker")
+                .args(args)
+                .output()
+                .expect("docker CLI");
+            assert!(
+                out.status.success(),
+                "docker {:?} failed: {}",
+                args,
+                String::from_utf8_lossy(&out.stderr)
+            );
+            String::from_utf8_lossy(&out.stdout).trim().to_string()
+        }
+
+        let image = std::env::var("TRIPLE_C_TEST_IMAGE")
+            .unwrap_or_else(|_| "ghcr.io/shadowdao/triple-c-sandbox:latest".to_string());
+        let first = format!("/opt/cache-marker-a-{}", std::process::id());
+        let second = format!("/opt/cache-marker-b-{}", std::process::id());
+
+        let id = docker_cli(&[
+            "run", "-d", "--label", "triple-c.managed=true",
+            "--entrypoint", "/bin/sh",
+            &image, "-c", "sleep 600",
+        ]);
+        let cleanup = || {
+            let _ = std::process::Command::new("docker")
+                .args(["rm", "-f", &id])
+                .output();
+        };
+
+        docker_cli(&["exec", &id, "mkdir", "-p", &first]);
+        docker_cli(&["stop", "-t", "1", &id]);
+
+        let t0 = std::time::Instant::now();
+        let cold = manifest_from_stopped_container_cached(&id).await;
+        let cold_ms = t0.elapsed().as_millis();
+
+        let t1 = std::time::Instant::now();
+        let warm = manifest_from_stopped_container_cached(&id).await;
+        let warm_ms = t1.elapsed().as_millis();
+
+        // Restart, change the filesystem, stop again — `FinishedAt` moves.
+        docker_cli(&["start", &id]);
+        docker_cli(&["exec", &id, "mkdir", "-p", &second]);
+        docker_cli(&["stop", "-t", "1", &id]);
+        let after_restart = manifest_from_stopped_container_cached(&id).await;
+
+        cleanup();
+
+        let has = |m: &Manifest, p: &str| m.paths.iter().any(|e| e.path == p && e.is_dir());
+
+        let cold = cold.expect("cold read");
+        let warm = warm.expect("warm read");
+        let after_restart = after_restart.expect("read after restart");
+
+        assert!(has(&cold, &first), "cold read missed {}", first);
+        assert!(has(&warm, &first), "warm read missed {}", first);
+        println!("cold {} ms, warm {} ms", cold_ms, warm_ms);
+        assert!(
+            warm_ms * 5 < cold_ms.max(5),
+            "the second read cost {} ms against a cold {} ms — it re-committed \
+             instead of using the cache",
+            warm_ms,
+            cold_ms
+        );
+
+        // The restart must have invalidated it: the new directory has to show up.
+        assert!(
+            has(&after_restart, &second),
+            "a restart did not invalidate the cache — {} is missing, so this is \
+             a stale manifest of a filesystem the container no longer has",
+            second
+        );
+        assert!(has(&after_restart, &first), "the restart lost {}", first);
+    }
+
     /// The reaper finds a leftover probe image by prefix and — crucially —
     /// refuses to remove a young one, because that image may be another
     /// Triple-C instance's live probe. Only a real daemon can say whether the

app/src/lib/tauri-commands.ts

@@ -350,8 +350,8 @@ export const sweepClaudeTokenSnapshots = () =>
 // without deleting its volumes. Reset is the destructive alternative: it wipes
 // ~/.claude, the OAuth credential, installed skills and every transcript.
 //
-// Flow: getContainerStaleness (read-only, ~6s — two filesystem probes, so call
-// it on demand rather than polling) → migrateProjectToBase → the project sits
+// Flow: getContainerStaleness (~6s — two filesystem probes, so call it on demand
+// rather than polling) → migrateProjectToBase → the project sits
 // in "awaiting-confirmation" while the user tries it → confirmMigration or
 // rollbackMigration.
 //
@@ -361,7 +361,19 @@ export const sweepClaudeTokenSnapshots = () =>
 //
 // Progress arrives on the existing `container-progress` event.
 
-/** Read-only. Runs two container/image filesystem probes; not for polling. */
+/**
+ * Runs two container/image filesystem probes; not for polling.
+ *
+ * **Not read-only, despite only reporting.** When the container is *stopped*
+ * the backend has to commit its writable layer to a throwaway image before it
+ * can read anything — `docker exec` needs a running container — so this writes
+ * (and then removes) an image. The result is cached per stop, so repeat calls
+ * while the container stays stopped are cheap, but the first one after each stop
+ * pays for a commit of the whole layer: seconds on a small project, tens of
+ * seconds on a large one. Do not add a caller that fires more often than "the
+ * container settled into a new state" without re-reading
+ * `get_container_staleness`'s doc comment first.
+ */
 export const getContainerStaleness = (projectId: string) =>
   invoke<ContainerStaleness>("get_container_staleness", { projectId });