Make a rollback pin outliving its project visible and deletable
`survey_rollback_pins` walks images, not projects, and deliberately tolerates an absent project by falling back to the raw id as the display name. Two things then dropped it on the floor: `destroy` called `find_project` before the confirmation check, so it refused such a pin every time, and the per-project table joins destructive items to rows by project_id, where rows come only from projects in the store. The result was a multi-GB `pre-migration-*` image that the scan measured, the panel never rendered, and nothing could remove — in the one screen built to find exactly that. `destroy` takes the same early return `OrphanVolume` already takes, and still validates the tag: `latest` names the project's live snapshot, so the ownerless path must not be a way around that check. The UI grows a bucket for destructive items matching no row, rather than filtering them away. The typed gate already compared against the id via `project_name`; the dialog now says "project id" instead of asking for a project name that no longer exists. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc
This commit is contained in:
@@ -3035,6 +3035,52 @@ async fn reclaim_containers(
|
|||||||
/// the typed confirmation is permission to act, not a promise that the world
|
/// the typed confirmation is permission to act, not a promise that the world
|
||||||
/// stood still. Both of those re-checks predate the move to the destructive
|
/// stood still. Both of those re-checks predate the move to the destructive
|
||||||
/// path and are deliberately unchanged.
|
/// path and are deliberately unchanged.
|
||||||
|
/// Drop a rollback pin whose project is no longer in `projects.json`.
|
||||||
|
///
|
||||||
|
/// The owned case ([`destroy`]'s `RollbackPin` arm) takes the project's claim
|
||||||
|
/// and clears the ownerless marker. Neither applies here: there is no project
|
||||||
|
/// to claim, and nothing else can be mid-operation on an id the store does not
|
||||||
|
/// know. What *does* still apply is the tag validation — this is the one
|
||||||
|
/// destructive variant carrying a free-form string over IPC, and `latest` would
|
||||||
|
/// name a live snapshot rather than a pin.
|
||||||
|
async fn destroy_ownerless_rollback_pin(
|
||||||
|
project_id: &str,
|
||||||
|
tag: &str,
|
||||||
|
) -> Result<ReclaimResult, String> {
|
||||||
|
if migration::parse_rollback_tag(tag).is_none() {
|
||||||
|
return Err(format!(
|
||||||
|
"{:?} is not a rollback pin tag. Nothing was removed.",
|
||||||
|
tag
|
||||||
|
));
|
||||||
|
}
|
||||||
|
let reference = format!("triple-c-snapshot-{}:{}", project_id, tag);
|
||||||
|
migration::untag_image(&reference).await?;
|
||||||
|
// The grace clock is meaningless once the tag is gone, and the marker file
|
||||||
|
// would otherwise outlive everything that could ever read it.
|
||||||
|
migration_store::clear_ownerless(project_id, tag);
|
||||||
|
// Untagging only makes the image dangling; the sweep applies its own
|
||||||
|
// refusal rules to whatever that turns out to be.
|
||||||
|
let sweep = container::sweep_orphaned_snapshots().await;
|
||||||
|
log::info!(
|
||||||
|
"Dropped ownerless rollback pin {} on explicit confirmation",
|
||||||
|
reference
|
||||||
|
);
|
||||||
|
Ok(ReclaimResult {
|
||||||
|
target: None,
|
||||||
|
destroyed: Some(DestructiveTarget::RollbackPin {
|
||||||
|
project_id: project_id.to_string(),
|
||||||
|
tag: tag.to_string(),
|
||||||
|
}),
|
||||||
|
ok: true,
|
||||||
|
freed_bytes: sweep.reclaimed_bytes,
|
||||||
|
projected_bytes: None,
|
||||||
|
message: format!(
|
||||||
|
"Dropped rollback pin {} for a project that is no longer in Triple-C.",
|
||||||
|
tag
|
||||||
|
),
|
||||||
|
})
|
||||||
|
}
|
||||||
|
|
||||||
async fn destroy_orphan_volume(name: &str, projects: &[Project]) -> Result<ReclaimResult, String> {
|
async fn destroy_orphan_volume(name: &str, projects: &[Project]) -> Result<ReclaimResult, String> {
|
||||||
let docker = get_docker()?;
|
let docker = get_docker()?;
|
||||||
|
|
||||||
@@ -3772,6 +3818,26 @@ pub async fn destroy(
|
|||||||
return destroy_orphan_volume(name, projects).await;
|
return destroy_orphan_volume(name, projects).await;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// **A rollback pin can outlive the project it belongs to.**
|
||||||
|
// `survey_rollback_pins` walks *images*, not projects, and deliberately
|
||||||
|
// tolerates an absent project by falling back to the raw id as the display
|
||||||
|
// name. So a pin left by a project the user has since deleted is measured
|
||||||
|
// and listed — and `find_project` below would refuse it on every attempt,
|
||||||
|
// making a multi-GB image permanently undeletable through the panel that
|
||||||
|
// exists to find exactly that. Take the same early return `OrphanVolume`
|
||||||
|
// takes, confirming against the subject the UI actually showed: the id.
|
||||||
|
if let DestructiveTarget::RollbackPin { project_id, tag } = target {
|
||||||
|
if find_project(projects, target.project_id()).is_err() {
|
||||||
|
if !confirmation_matches(project_id, confirmation) {
|
||||||
|
return Err(format!(
|
||||||
|
"This pin's project is no longer in Triple-C, so there is no name to type. Type the project id ({}) exactly to confirm. Nothing was removed.",
|
||||||
|
project_id
|
||||||
|
));
|
||||||
|
}
|
||||||
|
return destroy_ownerless_rollback_pin(project_id, tag).await;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
let project = find_project(projects, target.project_id())?;
|
let project = find_project(projects, target.project_id())?;
|
||||||
if !confirmation_matches(&project.name, confirmation) {
|
if !confirmation_matches(&project.name, confirmation) {
|
||||||
return Err(format!(
|
return Err(format!(
|
||||||
|
|||||||
@@ -1547,3 +1547,48 @@ async fn compaction_end_to_end_against_a_real_image() {
|
|||||||
probe.stdout
|
probe.stdout
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn an_ownerless_rollback_pin_is_confirmed_against_its_id_not_a_project_name() {
|
||||||
|
// `survey_rollback_pins` walks images, not projects, so a pin outlives the
|
||||||
|
// project that made it. Before the early return, `destroy` called
|
||||||
|
// `find_project` first and refused such a pin every single time — a
|
||||||
|
// multi-GB image measured by the panel and deletable by nothing in it.
|
||||||
|
let projects: Vec<Project> = Vec::new();
|
||||||
|
let target = DestructiveTarget::RollbackPin {
|
||||||
|
project_id: "dead0000-0000-0000-0000-000000000000".to_string(),
|
||||||
|
tag: "pre-migration-20260101-101500".to_string(),
|
||||||
|
};
|
||||||
|
|
||||||
|
// The wrong subject is refused, and the message says what to type instead
|
||||||
|
// of the "project not found" the old path produced.
|
||||||
|
let err = destroy(&target, "some-project-name", &projects)
|
||||||
|
.await
|
||||||
|
.expect_err("a mismatched confirmation must refuse");
|
||||||
|
assert!(
|
||||||
|
err.contains("dead0000-0000-0000-0000-000000000000"),
|
||||||
|
"the refusal should name the id to type, got: {}",
|
||||||
|
err
|
||||||
|
);
|
||||||
|
assert!(err.contains("Nothing was removed"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[tokio::test]
|
||||||
|
async fn an_ownerless_rollback_pin_still_refuses_a_tag_that_is_not_a_pin() {
|
||||||
|
// The tag is the one free-form string a destructive target carries, and
|
||||||
|
// `latest` names the project's live snapshot. The owned arm validates it;
|
||||||
|
// the ownerless arm must not be the way around that check.
|
||||||
|
let projects: Vec<Project> = Vec::new();
|
||||||
|
let target = DestructiveTarget::RollbackPin {
|
||||||
|
project_id: "dead0000-0000-0000-0000-000000000000".to_string(),
|
||||||
|
tag: "latest".to_string(),
|
||||||
|
};
|
||||||
|
let err = destroy(&target, "dead0000-0000-0000-0000-000000000000", &projects)
|
||||||
|
.await
|
||||||
|
.expect_err("`latest` is not a rollback pin tag");
|
||||||
|
assert!(
|
||||||
|
err.contains("not a rollback pin tag"),
|
||||||
|
"got: {}",
|
||||||
|
err
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|||||||
@@ -492,6 +492,68 @@ describe("DiskSettings", () => {
|
|||||||
expect(await screen.findByTestId("disk-orphan-bucket")).toBeInTheDocument();
|
expect(await screen.findByTestId("disk-orphan-bucket")).toBeInTheDocument();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("shows a rollback pin whose project is gone, instead of dropping it", async () => {
|
||||||
|
// `survey_rollback_pins` walks images, not projects, and falls back to the
|
||||||
|
// raw id when the project is absent. The per-project table joins
|
||||||
|
// destructive items to rows by `project_id`, and rows come only from
|
||||||
|
// projects in the store — so before the unmatched bucket, such a pin was
|
||||||
|
// measured by the scan and rendered nowhere at all. A multi-GB image the
|
||||||
|
// panel knew about and offered no way to remove.
|
||||||
|
const ownerlessPin: DestructiveItem = {
|
||||||
|
target: {
|
||||||
|
kind: "rollback_pin",
|
||||||
|
project_id: "dead0000-0000-0000-0000-000000000000",
|
||||||
|
tag: "pre-migration-20260101-101500",
|
||||||
|
},
|
||||||
|
project_id: "dead0000-0000-0000-0000-000000000000",
|
||||||
|
// The Rust falls back to the id, and it is what `destroy` compares
|
||||||
|
// against — so this is the string the user has to type.
|
||||||
|
project_name: "dead0000-0000-0000-0000-000000000000",
|
||||||
|
label: "Rollback pin pre-migration-20260101-101500",
|
||||||
|
loses: "The only copy of that migration's rollback target.",
|
||||||
|
bytes: 5_400_000_000,
|
||||||
|
blocked: null,
|
||||||
|
};
|
||||||
|
listReclaimable.mockResolvedValue(plan({ destructive: [ownerlessPin] }));
|
||||||
|
await renderAndScan();
|
||||||
|
|
||||||
|
const bucket = await screen.findByTestId("disk-unmatched-bucket");
|
||||||
|
expect(within(bucket).getByText(/Rollback pin pre-migration-20260101-101500/)).toBeInTheDocument();
|
||||||
|
// And it is not silently folded into the project table.
|
||||||
|
const table = screen.queryByTestId("disk-project-table");
|
||||||
|
if (table) {
|
||||||
|
expect(table.textContent).not.toMatch(/pre-migration-20260101-101500/);
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
it("asks for the project id, not a project name, when there is no project", async () => {
|
||||||
|
// The gate compares against `project_name`, which is the raw id here. That
|
||||||
|
// works — but a dialog captioned "type the project name" for a project that
|
||||||
|
// no longer exists asks for something the user cannot supply.
|
||||||
|
const ownerlessPin: DestructiveItem = {
|
||||||
|
target: {
|
||||||
|
kind: "rollback_pin",
|
||||||
|
project_id: "dead0000-0000-0000-0000-000000000000",
|
||||||
|
tag: "pre-migration-20260101-101500",
|
||||||
|
},
|
||||||
|
project_id: "dead0000-0000-0000-0000-000000000000",
|
||||||
|
project_name: "dead0000-0000-0000-0000-000000000000",
|
||||||
|
label: "Rollback pin pre-migration-20260101-101500",
|
||||||
|
loses: "The only copy of that migration's rollback target.",
|
||||||
|
bytes: 5_400_000_000,
|
||||||
|
blocked: null,
|
||||||
|
};
|
||||||
|
listReclaimable.mockResolvedValue(plan({ destructive: [ownerlessPin] }));
|
||||||
|
await renderAndScan();
|
||||||
|
|
||||||
|
const bucket = await screen.findByTestId("disk-unmatched-bucket");
|
||||||
|
fireEvent.click(within(bucket).getByRole("button", { name: /Delete/ }));
|
||||||
|
|
||||||
|
const dialog = await screen.findByRole("dialog");
|
||||||
|
expect(dialog.textContent).toMatch(/project id/i);
|
||||||
|
expect(dialog.textContent).not.toMatch(/type the project name/i);
|
||||||
|
});
|
||||||
|
|
||||||
it("keeps orphaned volumes out of the per-project table", async () => {
|
it("keeps orphaned volumes out of the per-project table", async () => {
|
||||||
// The table keys off `project_id`, and an orphan's id matches no row by
|
// The table keys off `project_id`, and an orphan's id matches no row by
|
||||||
// definition. Passing them in anyway is how one would leak into the wrong
|
// definition. Passing them in anyway is how one would leak into the wrong
|
||||||
|
|||||||
@@ -105,7 +105,18 @@ export default function DiskSettings() {
|
|||||||
// moved from the tick list to the destructive list can vanish from the UI
|
// moved from the tick list to the destructive list can vanish from the UI
|
||||||
// entirely rather than reappear behind a confirmation.
|
// entirely rather than reappear behind a confirmation.
|
||||||
const orphanVolumes = plan?.destructive.filter(isOrphanVolume) ?? [];
|
const orphanVolumes = plan?.destructive.filter(isOrphanVolume) ?? [];
|
||||||
const projectDestructive = plan?.destructive.filter((d) => !isOrphanVolume(d)) ?? [];
|
// A destructive item is rendered inside its project's row, so one whose
|
||||||
|
// project id matches no row would be measured and shown nowhere. That is not
|
||||||
|
// hypothetical: `survey_rollback_pins` walks *images*, not projects, and
|
||||||
|
// deliberately tolerates an absent project by falling back to the raw id as
|
||||||
|
// the display name — so a pin left behind by a deleted project is exactly
|
||||||
|
// this case, and it is the multi-GB kind. Anything unmatched gets its own
|
||||||
|
// section rather than being silently dropped.
|
||||||
|
const rowIds = new Set((report?.projects ?? []).map((r) => r.project_id));
|
||||||
|
const projectDestructive =
|
||||||
|
plan?.destructive.filter((d) => !isOrphanVolume(d) && rowIds.has(d.project_id)) ?? [];
|
||||||
|
const unmatchedDestructive =
|
||||||
|
plan?.destructive.filter((d) => !isOrphanVolume(d) && !rowIds.has(d.project_id)) ?? [];
|
||||||
|
|
||||||
const safeItems = plan?.items.filter((i) => i.safety === "safe") ?? [];
|
const safeItems = plan?.items.filter((i) => i.safety === "safe") ?? [];
|
||||||
const semiItems = plan?.items.filter((i) => i.safety === "semi_safe") ?? [];
|
const semiItems = plan?.items.filter((i) => i.safety === "semi_safe") ?? [];
|
||||||
@@ -495,6 +506,58 @@ export default function DiskSettings() {
|
|||||||
</section>
|
</section>
|
||||||
)}
|
)}
|
||||||
|
|
||||||
|
{/* --- Destructive leftovers with no project row ------------------- */}
|
||||||
|
{unmatchedDestructive.length > 0 && (
|
||||||
|
<section className="space-y-2" data-testid="disk-unmatched-bucket">
|
||||||
|
<h3 className="text-[13px] font-medium text-[var(--text-primary)]">
|
||||||
|
Leftovers from projects no longer in Triple-C
|
||||||
|
</h3>
|
||||||
|
<p className="text-xs text-[var(--text-secondary)] leading-relaxed">
|
||||||
|
These belong to a project id that is not in your project list, so there
|
||||||
|
is no row above to show them under. The same caveat as the volumes below
|
||||||
|
applies: “not in your project list” is the only thing this
|
||||||
|
means, and an idle live project is indistinguishable from a deleted one
|
||||||
|
from Docker’s side. Because there is no project name to type, each
|
||||||
|
one is confirmed against its project <em>id</em>.
|
||||||
|
</p>
|
||||||
|
<ul className="space-y-1.5">
|
||||||
|
{unmatchedDestructive.map((item) => (
|
||||||
|
<li
|
||||||
|
key={destructiveKey(item)}
|
||||||
|
className="flex items-start justify-between gap-3"
|
||||||
|
data-testid={`disk-unmatched-${destructiveKey(item)}`}
|
||||||
|
>
|
||||||
|
<span className="flex-1 min-w-0">
|
||||||
|
<span className="block text-[var(--text-primary)] font-mono break-all">
|
||||||
|
{item.label}
|
||||||
|
</span>
|
||||||
|
<span className="block text-xs text-[var(--text-secondary)] leading-snug">
|
||||||
|
{item.loses}
|
||||||
|
</span>
|
||||||
|
{item.blocked && (
|
||||||
|
<span className="block text-xs text-[var(--text-secondary)]">
|
||||||
|
{item.blocked}
|
||||||
|
</span>
|
||||||
|
)}
|
||||||
|
</span>
|
||||||
|
<span className="flex items-center gap-2 whitespace-nowrap">
|
||||||
|
<span className="text-xs text-[var(--text-secondary)] tabular-nums">
|
||||||
|
{formatBytes(item.bytes)}
|
||||||
|
</span>
|
||||||
|
<Button
|
||||||
|
size="sm"
|
||||||
|
disabled={item.blocked !== null || working}
|
||||||
|
onClick={() => openDestroying(item)}
|
||||||
|
>
|
||||||
|
Delete…
|
||||||
|
</Button>
|
||||||
|
</span>
|
||||||
|
</li>
|
||||||
|
))}
|
||||||
|
</ul>
|
||||||
|
</section>
|
||||||
|
)}
|
||||||
|
|
||||||
{/* --- Orphaned volumes: destructive, one at a time ---------------- */}
|
{/* --- Orphaned volumes: destructive, one at a time ---------------- */}
|
||||||
{orphanVolumes.length > 0 && (
|
{orphanVolumes.length > 0 && (
|
||||||
<section className="space-y-2" data-testid="disk-orphan-bucket">
|
<section className="space-y-2" data-testid="disk-orphan-bucket">
|
||||||
@@ -704,6 +767,12 @@ export default function DiskSettings() {
|
|||||||
// case-sensitive and a mangled name in the heading is a name the user
|
// case-sensitive and a mangled name in the heading is a name the user
|
||||||
// cannot type.
|
// cannot type.
|
||||||
const orphan = isOrphanVolume(destroying);
|
const orphan = isOrphanVolume(destroying);
|
||||||
|
// A leftover whose project is gone has no name either. `project_name`
|
||||||
|
// is the raw id in that case — which is deliberate on the Rust side and
|
||||||
|
// is exactly what `destroy` compares against — so the gate works, but
|
||||||
|
// the label has to say "id" or it asks for something that does not
|
||||||
|
// exist.
|
||||||
|
const ownerless = !orphan && !rowIds.has(destroying.project_id);
|
||||||
return (
|
return (
|
||||||
<TypedConfirmModal
|
<TypedConfirmModal
|
||||||
title={
|
title={
|
||||||
@@ -712,7 +781,7 @@ export default function DiskSettings() {
|
|||||||
: `Delete ${destroying.label.toLowerCase()}`
|
: `Delete ${destroying.label.toLowerCase()}`
|
||||||
}
|
}
|
||||||
expected={destroying.project_name}
|
expected={destroying.project_name}
|
||||||
subject={orphan ? "volume name" : "project name"}
|
subject={orphan ? "volume name" : ownerless ? "project id" : "project name"}
|
||||||
confirmLabel={orphan ? "Delete volume" : `Delete ${destroying.label.toLowerCase()}`}
|
confirmLabel={orphan ? "Delete volume" : `Delete ${destroying.label.toLowerCase()}`}
|
||||||
busy={working}
|
busy={working}
|
||||||
// A failure here has to land inside the dialog. The panel's own
|
// A failure here has to land inside the dialog. The panel's own
|
||||||
|
|||||||
Reference in New Issue
Block a user