CyberCoveLLC/Triple-C
1
0
Fork 0
Code Issues 1 Pull Requests Actions Packages Projects Releases 35 Wiki Activity

Fix what review found in the Disk section

Browse Source 
Safety:

- `destroy`'s rollback-pin arm took a tag over IPC and interpolated it
  straight into an image reference it then removed. `tag: "latest"` named
  the project's live snapshot, deleted under a dialog saying "rollback
  pin". It is the one destructive variant carrying a free-form string, so
  it now goes through `parse_rollback_tag`.
- The compaction's scratch container was named `triple-c-scrub-*`, which
  is what the scrub reclaim bucket hunts and force-removes. A reclaim from
  a second window would have destroyed the container a running compaction
  was about to commit. It gets `triple-c-compact-*`, swept at the start of
  the next compaction rather than from a bucket anything else can fire.
- Deleting a home or config volume only refused a *running* container, but
  a stopped one still pins its volumes — the resting state of every
  project ever started — so the user typed the project name and met a raw
  409. The container is now removed first and `loses` says so.

Correctness:

- The compaction Dockerfile emitted no `LABEL`, so the flattened
  intermediate could never match the sweep's `dangling` + `triple-c.managed`
  filter that three cleanup paths rely on. Verified on Docker 29.7.2 that
  the label lands on the final stage, the build still yields one layer, and
  untagging the staging tag after the commit leaves the committed snapshot
  intact and startable.
- `snapshot_commit_layers` silently meant something else when
  `triple-c.base-image-id` was absent — the normal case for a pre-label
  project — counting the base's own layers and letting a never-recreated
  project qualify for compaction. `base_lineage_known` now carries that,
  the column says "unknown", and the plan does not offer the rewrite.
- `destroy` returned a `ReclaimResult` wearing a `ReclaimTarget` that named
  work it had not done (a home-volume deletion came back as
  `OrphanVolume`). Split into `target` / `destroyed`, exactly one set.
- `formatBytes` ran `toFixed` after the divide loop, so 999,999 rendered as
  "1000.0 KB" — in the app's only byte formatter, in a panel full of
  near-boundary sizes.
- `is_base_image_reference` split on the first colon, so a registry port
  ate the repo name.

UI:

- `snapshot_above_base_bytes: null` — deliberately unmeasurable — rendered
  as "0 B", the one guessed number in the table.
- Layer count was flagged by colour alone; it now says "stacked".
- The tick list survived a reclaim, so the same call could be re-fired at
  objects that no longer existed. The plan is dropped after any action and
  the panel says the totals predate it.
- `setReport` landed before the plan call was awaited, so a plan failure
  rendered fresh totals above the previous scan's rows.
- Both confirmation modals unmounted before awaiting, making the entire
  busy path dead code during multi-second work.
- `buildx du` failures silently showed `docker system df`'s under-reported
  build-cache figure with no explanation.
- Tooltip text reached no assistive tech, so two headers announced as
  "Help"; hardcoded input id; error-toned glyph in warning-toned panels;
  `sweepOrphanedSnapshots` and `clearOutcome` had no callers.
- Four docstrings claimed things the code did not do, and two tests were
  named for behaviour they did not assert.

Tests: 513 frontend (was 502), 370 Rust (was 365).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc
This commit is contained in:
shadow-test
2026-08-23 09:51:09 -07:00
co-authored by Claude Opus 5
parent 77ef2291d7
commit 611f67cca7
11 changed files with 799 additions and 100 deletions
Download Patch File Download Diff File Expand all files Collapse all files
app/src/hooks/useDiskUsage.test.tsx
+86 -3
View File
@@ -14,9 +14,11 @@ vi.mock("../lib/tauri-commands", () => ({
reclaim: (targets: unknown) => reclaim(targets),
destroyProjectDiskObject: (target: unknown, confirmation: string) =>
destroyProjectDiskObject(target, confirmation),
sweepOrphanedSnapshots: vi.fn(),
sweepOrphanedSnapshots: () => sweepOrphanedSnapshots(),
}));
const sweepOrphanedSnapshots = vi.fn();
const report = (scanned_at: string): DiskUsageReport =>
({ scanned_at, projects: [] }) as unknown as DiskUsageReport;
@@ -149,13 +151,94 @@ describe("useDiskUsage", () => {
expect(result.current.outcome?.total_freed_bytes).toBe(100);
});
it("surfaces a failure rather than leaving a stale report on screen", async () => {
getDockerDiskUsage.mockRejectedValue("daemon unreachable");
it("reports a scan failure and keeps the last good measurement", async () => {
// The old report is still an accurate measurement of an earlier moment,
// and the error says the refresh failed. Blanking it would leave the panel
// with nothing while telling the user nothing more.
getDockerDiskUsage.mockResolvedValueOnce(report("first"));
const { result } = renderHook(() => useDiskUsage());
await act(async () => {
await result.current.scan();
});
getDockerDiskUsage.mockRejectedValueOnce("daemon unreachable");
await act(async () => {
await result.current.scan();
});
await waitFor(() => expect(result.current.error).toMatch(/daemon unreachable/));
expect(result.current.report?.scanned_at).toBe("first");
expect(result.current.scanning).toBe(false);
});
it("never shows fresh totals beside a stale tick list", async () => {
// `setReport` used to land before the plan call was awaited, so a plan
// failure rendered this scan's numbers above the previous scan's rows.
getDockerDiskUsage.mockResolvedValueOnce(report("first"));
const { result } = renderHook(() => useDiskUsage());
await act(async () => {
await result.current.scan();
});
getDockerDiskUsage.mockResolvedValueOnce(report("second"));
listReclaimable.mockRejectedValueOnce("planner exploded");
await act(async () => {
await result.current.scan();
});
expect(result.current.error).toMatch(/planner exploded/);
expect(result.current.report?.scanned_at).toBe("first");
});
it("drops the plan after a reclaim so ticks cannot be re-fired at nothing", async () => {
getDockerDiskUsage.mockResolvedValue(report("first"));
const { result } = renderHook(() => useDiskUsage());
await act(async () => {
await result.current.scan();
});
expect(result.current.plan).toEqual(plan);
await act(async () => {
await result.current.runReclaim([{ kind: "dangling_snapshots" }]);
});
expect(result.current.plan).toBeNull();
// The totals stay — they were measured before the reclaim and the outcome
// says what changed.
expect(result.current.report?.scanned_at).toBe("first");
});
it("runs the sweep through its own command and reports what it refused", async () => {
// The sweep's `in_use` count — orphans Docker refused to delete because a
// stopped project still needs them — is invisible everywhere else in the
// app, because every other caller throws the report away.
sweepOrphanedSnapshots.mockResolvedValue({
removed: ["sha256:a", "sha256:b"],
reclaimed_bytes: 11_900_000_000,
in_use: 3,
failed: [],
unavailable: null,
});
const { result } = renderHook(() => useDiskUsage());
await act(async () => {
await result.current.runSweep();
});
expect(sweepOrphanedSnapshots).toHaveBeenCalled();
expect(result.current.outcome?.total_freed_bytes).toBe(11_900_000_000);
expect(result.current.outcome?.results[0].message).toMatch(/Swept 2 superseded image/);
expect(result.current.outcome?.results[0].message).toMatch(/3 were left alone/);
});
it("treats an unreachable daemon in the sweep report as an error", async () => {
sweepOrphanedSnapshots.mockResolvedValue({
removed: [],
reclaimed_bytes: 0,
in_use: 0,
failed: [],
unavailable: "Could not reach the Docker engine",
});
const { result } = renderHook(() => useDiskUsage());
await act(async () => {
await result.current.runSweep();
});
expect(result.current.error).toMatch(/Could not reach the Docker engine/);
expect(result.current.outcome).toBeNull();
});
});
app/src/hooks/useDiskUsage.ts
+92 -8
View File
@@ -17,16 +17,24 @@ import type {
* the daemon and computes shared-layer sizes. On a 100 GB store that is
* seconds. `AccordionSection` unmounts its body when collapsed, so a
* `useEffect` scan would re-run every single time the user opened the section.
* The scan is therefore a `scan()` the Scan button calls and nothing else, and
* the result lives in this hook rather than in the component so that reopening
* the section shows the last result instead of paying again.
* The scan is therefore only ever what the Scan button calls.
*
* Note what that does *not* buy: this hook lives inside `DiskSettings`, which
* the accordion unmounts on collapse, so its state goes with it and reopening
* the section shows an unscanned panel again. That is the honest behaviour —
* a stale total is worse than an absent one — but it means collapsing and
* reopening discards a scan the user paid for. Lifting the report into
* `appState` would fix that and is deliberately not done here: it would put a
* multi-megabyte, rapidly-stale blob into the app-wide store for one panel.
*
* ## The generation guard
*
* A user who hits Scan twice can have two `df()` calls in flight, and they can
* land out of order — the second one is not necessarily slower. Every async
* write checks it is still the newest before it lands, the same pattern
* `useContainerMigration` uses.
* write in `scan` checks it is still the newest before it lands, the same
* pattern `useContainerMigration` uses. `runReclaim` and `destroy` do not need
* it: the UI disables their buttons while `working` is set, so there is never
* a second one to race.
*/
export interface DiskUsageState {
report: DiskUsageReport | null;
@@ -41,6 +49,8 @@ export interface DiskUsageState {
scan: () => Promise<void>;
runReclaim: (targets: ReclaimTarget[]) => Promise<void>;
destroy: (target: DestructiveTarget, confirmation: string) => Promise<void>;
/** Run the orphaned-snapshot sweep and report what it found *and refused*. */
runSweep: () => Promise<void>;
clearOutcome: () => void;
}
@@ -63,16 +73,24 @@ export function useDiskUsage(): DiskUsageState {
try {
const next = await commands.getDockerDiskUsage();
if (generation.current !== mine) return;
setReport(next);
// Planning is cheap and always wanted: the classification is what makes
// the numbers actionable, and it reuses the report rather than scanning
// again.
const nextPlan = await commands.listReclaimable(next);
if (generation.current !== mine) return;
// Both land together, or neither does. Setting the report before
// awaiting the plan would render this scan's totals above the *previous*
// scan's still-clickable tick list if the plan call failed.
setReport(next);
setPlan(nextPlan);
} catch (e) {
if (generation.current !== mine) return;
setError(String(e));
// The old report is left on screen deliberately — it is still an
// accurate measurement of an earlier moment, and the error says the
// refresh failed. What must not survive is a plan describing a scan the
// user can no longer see the totals for, but that cannot happen: the two
// only ever move together.
} finally {
if (generation.current === mine) setScanning(false);
}
@@ -85,9 +103,16 @@ export function useDiskUsage(): DiskUsageState {
try {
const result = await commands.reclaim(targets);
setOutcome(result);
// Deliberately no automatic re-scan. It costs another `df()`, and the
// **The plan is now stale and must not stay clickable.** Its rows
// describe objects this call just removed, so leaving them ticked lets
// the user fire the same reclaim again against nothing. Dropping the plan
// (not the report) leaves the totals on screen, marked as measured before
// the reclaim, with the tick list gone.
//
// Deliberately no automatic re-scan: it costs another `df()`, and the
// outcome already reports measured bytes for every target — a user who
// wants the new totals asks for them.
setPlan(null);
} catch (e) {
setError(String(e));
} finally {
@@ -101,6 +126,53 @@ export function useDiskUsage(): DiskUsageState {
try {
const result = await commands.destroyProjectDiskObject(target, confirmation);
setOutcome({ results: [result], total_freed_bytes: result.freed_bytes });
// Same reasoning as `runReclaim`: the destructive list named an object
// that is now gone.
setPlan(null);
} catch (e) {
setError(String(e));
} finally {
setWorking(false);
}
}, []);
/**
* The startup sweep, on demand.
*
* Not the same as ticking "superseded snapshot layers", even though both end
* up removing the same images: this reports `in_use` — the orphans Docker
* *refused* to delete because a stopped project's container still needs
* them. That refusal is the sweep's third safety net and it is invisible
* everywhere else in the app, because every existing caller throws the
* report away.
*/
const runSweep = useCallback(async () => {
setWorking(true);
setError(null);
try {
const sweep = await commands.sweepOrphanedSnapshots();
if (sweep.unavailable) {
setError(sweep.unavailable);
return;
}
const refused =
sweep.in_use > 0
? ` ${sweep.in_use} were left alone because a container is still built from them — start and stop, or recreate, that project and a later sweep gets them.`
: "";
setOutcome({
results: [
{
target: { kind: "dangling_snapshots" },
destroyed: null,
ok: sweep.failed.length === 0,
freed_bytes: sweep.reclaimed_bytes,
projected_bytes: null,
message: `Swept ${sweep.removed.length} superseded image(s).${refused}`,
},
],
total_freed_bytes: sweep.reclaimed_bytes,
});
setPlan(null);
} catch (e) {
setError(String(e));
} finally {
@@ -110,5 +182,17 @@ export function useDiskUsage(): DiskUsageState {
const clearOutcome = useCallback(() => setOutcome(null), []);
return { report, plan, scanning, working, error, outcome, scan, runReclaim, destroy, clearOutcome };
return {
report,
plan,
scanning,
working,
error,
outcome,
scan,
runReclaim,
destroy,
runSweep,
clearOutcome,
};
}