diff --git a/CLAUDE.md b/CLAUDE.md index 545f969..b4addc2 100644 --- a/CLAUDE.md +++ b/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. diff --git a/app/src-tauri/src/commands/migration_commands.rs b/app/src-tauri/src/commands/migration_commands.rs index e97fc31..5d4c0de 100644 --- a/app/src-tauri/src/commands/migration_commands.rs +++ b/app/src-tauri/src/commands/migration_commands.rs @@ -142,6 +142,45 @@ fn pick_probe_source(container_running: Option, 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"); diff --git a/app/src-tauri/src/docker/migration.rs b/app/src-tauri/src/docker/migration.rs index a5332c0..67c242d 100644 --- a/app/src-tauri/src/docker/migration.rs +++ b/app/src-tauri/src/docker/migration.rs @@ -1087,6 +1087,89 @@ pub async fn manifest_from_container(container_id: &str) -> Result>, +> = 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 { + 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 { + 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 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 diff --git a/app/src/lib/tauri-commands.ts b/app/src/lib/tauri-commands.ts index 49349e8..d6934f8 100644 --- a/app/src/lib/tauri-commands.ts +++ b/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("get_container_staleness", { projectId });