From 84a67fcd0d4c6ad7c07971309fc99717628d5810 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 13 Aug 2026 21:24:51 -0700 Subject: [PATCH] Stop an empty base-image label from silencing the migration notice MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A project can be out of date and say nothing about it, in two ways that compound: the lineage lookup treats "unknown" as an answer, and the fallback that exists for unknown lineage disappears when its probe fails. `create_container` always writes triple-c.base-image-id, even when the value is unknown — deliberately, so an inherited image label cannot ride a snapshot forever. That makes Some("") the ordinary reading from a container whose lineage was never established. The lookup filtered for emptiness only on the final result, so that empty string satisfied the container branch and skipped the snapshot entirely: a snapshot that had recorded a real lineage was never consulted, and the project reported "unknown" with the answer one lookup away. Each source is now filtered before it can answer, in pick_recorded_lineage, which is a plain function so the case has a test that fails against the old logic. A genuinely pre-label project stays unknown, and should: its ancestor is not knowable, and inventing one would make it look permanently current. The probe is the intended signal for those — but if the probe failed, get_container_staleness returned early with nothing populated, the banner found no gaps and rendered null, and the probe_error it already knew how to display sat behind a gate that returned before reaching it. Silence there is indistinguishable from "up to date", and it is likeliest for the oldest and largest projects, whose manifests are the ones apt to exceed the inspection limit — one real project measured 6.93 MB against an 8 MB cap. An unknown-lineage container whose probe failed now says the check could not be completed, with the reason, under the tone that means unresolved rather than the one that means something is wrong. Co-Authored-By: Claude Opus 5 (1M context) --- .../src/commands/migration_commands.rs | 71 ++++++++++++++++--- .../home/ContainerMigrationBanner.test.tsx | 40 +++++++++++ .../home/ContainerMigrationBanner.tsx | 29 ++++++-- 3 files changed, 124 insertions(+), 16 deletions(-) diff --git a/app/src-tauri/src/commands/migration_commands.rs b/app/src-tauri/src/commands/migration_commands.rs index 7b86388..f43e348 100644 --- a/app/src-tauri/src/commands/migration_commands.rs +++ b/app/src-tauri/src/commands/migration_commands.rs @@ -73,6 +73,25 @@ use crate::AppState; /// Report how far behind the current base image a project's container is, and /// what migrating it would actually carry across. /// +/// Choose the recorded lineage from the two places it can be written, most +/// authoritative first: the live container's label, then the snapshot image's. +/// +/// **An empty label is absence, not an answer.** `create_container` always +/// writes `triple-c.base-image-id`, even when the value is unknown — that is +/// deliberate, because Docker merges an image's labels into a container's and +/// an inherited value would otherwise ride a snapshot forever. The consequence +/// is that `Some("")` is the *common* reading from a container whose lineage +/// was never established, so treating it as an answer silently skips the +/// snapshot, which may well have recorded a real one. +fn pick_recorded_lineage( + from_container: Option, + from_snapshot: Option, +) -> Option { + from_container + .filter(|v| !v.is_empty()) + .or_else(|| from_snapshot.filter(|v| !v.is_empty())) +} + /// Read-only. Runs two filesystem probes (~3 s each) and is therefore meant to /// be called on demand, not polled. #[tauri::command] @@ -98,20 +117,24 @@ pub async fn get_container_staleness( // Lineage, most authoritative source first: the live container's label, // then the snapshot image's. Both are written by `create_container` and // propagated onto the snapshot by `docker commit`. + // Each source is filtered for emptiness *before* it is allowed to satisfy + // the lookup. `create_container` always writes this label, even when the + // value is unknown — deliberately, so an inherited image label cannot ride + // a snapshot forever — which means the container's copy is very often + // `Some("")`. Filtering only the final result let that empty string count + // as an answer and skip the snapshot entirely, so a snapshot that *did* + // record a lineage was never consulted and the project reported "unknown" + // with the information sitting one lookup away. let container_id = docker::find_existing_container(&project).await.unwrap_or(None); - let recorded = match &container_id { + let from_container = match &container_id { Some(id) => container_label(id, mig::LABEL_BASE_IMAGE_ID).await, None => None, - } - .or_else(|| None); - let recorded = match recorded { - Some(v) => Some(v), - None => mig::image_labels(&snapshot_image) - .await - .get(mig::LABEL_BASE_IMAGE_ID) - .cloned(), - } - .filter(|v| !v.is_empty()); + }; + let from_snapshot = mig::image_labels(&snapshot_image) + .await + .get(mig::LABEL_BASE_IMAGE_ID) + .cloned(); + let recorded = pick_recorded_lineage(from_container, from_snapshot); out.base_image_id = recorded.clone(); out.known = recorded.is_some(); @@ -1671,6 +1694,32 @@ fn summarize( mod tests { use super::*; + #[test] + fn an_empty_lineage_label_is_absence_and_falls_through_to_the_snapshot() { + let some = |s: &str| Some(s.to_string()); + + // The regression: the container always carries the label, so an + // unknown lineage reads as `Some("")`. Letting that satisfy the lookup + // skipped a snapshot that had recorded the real thing. + assert_eq!( + pick_recorded_lineage(some(""), some("sha256:base")), + some("sha256:base") + ); + + // Ordinary precedence still holds: the container wins when it has one. + assert_eq!( + pick_recorded_lineage(some("sha256:container"), some("sha256:snapshot")), + some("sha256:container") + ); + assert_eq!(pick_recorded_lineage(None, some("sha256:snap")), some("sha256:snap")); + + // Genuinely unknown stays unknown — "probe instead", never a lineage + // invented to make the comparison succeed. + assert_eq!(pick_recorded_lineage(None, None), None); + assert_eq!(pick_recorded_lineage(some(""), some("")), None); + assert_eq!(pick_recorded_lineage(some(""), None), None); + } + #[test] fn byte_sizes_read_the_way_a_disk_warning_should() { assert_eq!(human_bytes(512), "512 B"); diff --git a/app/src/components/projects/home/ContainerMigrationBanner.test.tsx b/app/src/components/projects/home/ContainerMigrationBanner.test.tsx index 9b328a5..d140053 100644 --- a/app/src/components/projects/home/ContainerMigrationBanner.test.tsx +++ b/app/src/components/projects/home/ContainerMigrationBanner.test.tsx @@ -127,6 +127,46 @@ describe("ContainerMigrationBanner", () => { expect(container).toBeEmptyDOMElement(); }); + it("speaks up when an unlabelled container could not be probed at all", () => { + // The probe is the only signal a container with no lineage label has. If + // it fails and the banner stays silent, that is indistinguishable from + // "up to date" — the exact reading that let an out-of-date project go + // unnoticed indefinitely. + renderBanner( + migration({ + staleness: { + ...FRESH, + known: false, + stale: false, + probe_error: "output exceeded the inspection limit", + }, + probeSettled: false, + }), + ); + expect( + screen.getByText(/Container base could not be checked/i), + ).toBeInTheDocument(); + expect( + screen.getByText(/output exceeded the inspection limit/i), + ).toBeInTheDocument(); + // And it must not pose as a finding about the container itself. + expect( + screen.queryByText(/Container is missing things/i), + ).not.toBeInTheDocument(); + }); + + it("stays quiet when a labelled container's probe fails but its lineage is current", () => { + // `known` means the version comparison already answered the question, so + // a failed probe is not grounds to raise anything. + const { container } = renderBanner( + migration({ + staleness: { ...FRESH, probe_error: "could not exec in the container" }, + probeSettled: false, + }), + ); + expect(container).toBeEmptyDOMElement(); + }); + it("disables the action and explains why while the container is running", () => { renderBanner(migration({ staleness: STALE }), false); expect( diff --git a/app/src/components/projects/home/ContainerMigrationBanner.tsx b/app/src/components/projects/home/ContainerMigrationBanner.tsx index e8dbae5..c359d35 100644 --- a/app/src/components/projects/home/ContainerMigrationBanner.tsx +++ b/app/src/components/projects/home/ContainerMigrationBanner.tsx @@ -131,7 +131,15 @@ export default function ContainerMigrationBanner({ const probeFoundGaps = !staleness.known && (staleness.missing_features.length > 0 || staleness.missing_paths.length > 0); - if (!staleness.stale && !probeFoundGaps) return null; + + // The probe is the *only* signal a container with no lineage label has, so + // when it fails there is nothing left to be quiet about. Staying silent here + // is indistinguishable from "everything is fine" — and it is the likeliest + // outcome for the oldest, largest projects, whose manifests are the ones apt + // to exceed the inspection limit. Say that the check did not run instead. + const probeUnavailable = !staleness.known && !!staleness.probe_error; + + if (!staleness.stale && !probeFoundGaps && !probeUnavailable) return null; const snapshot = formatSnapshotDate(staleness.snapshot_created_at); const features = joinFeatures(staleness.missing_features); @@ -139,16 +147,25 @@ export default function ContainerMigrationBanner({ return (
@@ -158,7 +175,9 @@ export default function ContainerMigrationBanner({ ? snapshot ? `Running on a saved image from ${snapshot}.` : "Running on a saved image older than the current base." - : "This container predates base-image tracking, so it was probed directly."} + : probeUnavailable + ? "This container predates base-image tracking, so probing it is the only way to tell whether it is behind — and that did not complete." + : "This container predates base-image tracking, so it was probed directly."}

{staleness.missing_features.length > 0 && (