Fix Playwright setup destroying its own install, and the migration notice that stayed silent #25

Merged
jknapp merged 2 commits from fix/playwright-container-setup into main 2026-08-14 04:34:20 +00:00
3 changed files with 124 additions and 16 deletions
Showing only changes of commit 84a67fcd0d - Show all commits
@@ -73,6 +73,25 @@ use crate::AppState;
/// Report how far behind the current base image a project's container is, and /// Report how far behind the current base image a project's container is, and
/// what migrating it would actually carry across. /// 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<String>,
from_snapshot: Option<String>,
) -> Option<String> {
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 /// Read-only. Runs two filesystem probes (~3 s each) and is therefore meant to
/// be called on demand, not polled. /// be called on demand, not polled.
#[tauri::command] #[tauri::command]
@@ -98,20 +117,24 @@ pub async fn get_container_staleness(
// Lineage, most authoritative source first: the live container's label, // Lineage, most authoritative source first: the live container's label,
// then the snapshot image's. Both are written by `create_container` and // then the snapshot image's. Both are written by `create_container` and
// propagated onto the snapshot by `docker commit`. // 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 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, Some(id) => container_label(id, mig::LABEL_BASE_IMAGE_ID).await,
None => None, None => None,
} };
.or_else(|| None); let from_snapshot = mig::image_labels(&snapshot_image)
let recorded = match recorded {
Some(v) => Some(v),
None => mig::image_labels(&snapshot_image)
.await .await
.get(mig::LABEL_BASE_IMAGE_ID) .get(mig::LABEL_BASE_IMAGE_ID)
.cloned(), .cloned();
} let recorded = pick_recorded_lineage(from_container, from_snapshot);
.filter(|v| !v.is_empty());
out.base_image_id = recorded.clone(); out.base_image_id = recorded.clone();
out.known = recorded.is_some(); out.known = recorded.is_some();
@@ -1671,6 +1694,32 @@ fn summarize(
mod tests { mod tests {
use super::*; 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] #[test]
fn byte_sizes_read_the_way_a_disk_warning_should() { fn byte_sizes_read_the_way_a_disk_warning_should() {
assert_eq!(human_bytes(512), "512 B"); assert_eq!(human_bytes(512), "512 B");
@@ -127,6 +127,46 @@ describe("ContainerMigrationBanner", () => {
expect(container).toBeEmptyDOMElement(); 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", () => { it("disables the action and explains why while the container is running", () => {
renderBanner(migration({ staleness: STALE }), false); renderBanner(migration({ staleness: STALE }), false);
expect( expect(
@@ -131,7 +131,15 @@ export default function ContainerMigrationBanner({
const probeFoundGaps = const probeFoundGaps =
!staleness.known && !staleness.known &&
(staleness.missing_features.length > 0 || staleness.missing_paths.length > 0); (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 snapshot = formatSnapshotDate(staleness.snapshot_created_at);
const features = joinFeatures(staleness.missing_features); const features = joinFeatures(staleness.missing_features);
@@ -139,15 +147,24 @@ export default function ContainerMigrationBanner({
return ( return (
<section <section
className={`${SHELL} border-[var(--warning)]/40 bg-[var(--warning-muted)]`} className={`${SHELL} border-[var(--warning)]/40 bg-[var(--warning-muted)]`}
aria-label="Container base is out of date" aria-label={
probeUnavailable
? "Container base could not be checked"
: "Container base is out of date"
}
> >
<div className="flex items-start justify-between gap-3"> <div className="flex items-start justify-between gap-3">
<div className="min-w-0 space-y-1"> <div className="min-w-0 space-y-1">
<StatusIndicator <StatusIndicator
tone="error" // A check that could not run is not a finding: it gets the
// "unresolved" tone rather than the one that says something is
// wrong with the container.
tone={probeUnavailable ? "unknown" : "error"}
label={ label={
staleness.known staleness.known
? "Container base is out of date" ? "Container base is out of date"
: probeUnavailable
? "Container base could not be checked"
: "Container is missing things the current base ships" : "Container is missing things the current base ships"
} }
className="text-[13px] font-semibold" className="text-[13px] font-semibold"
@@ -158,6 +175,8 @@ export default function ContainerMigrationBanner({
? snapshot ? snapshot
? `Running on a saved image from ${snapshot}.` ? `Running on a saved image from ${snapshot}.`
: "Running on a saved image older than the current base." : "Running on a saved image older than the current base."
: 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."} : "This container predates base-image tracking, so it was probed directly."}
</p> </p>