From cd1cde2fd44d8bf67c397aa3ade6b4d99f523147 Mon Sep 17 00:00:00 2001 From: Anso Date: Fri, 22 May 2026 02:40:22 -0400 Subject: [PATCH] fix(resources): subtract shared layers when accounting managed prune bytes (#1155) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(resources): subtract shared layers when accounting managed prune bytes Both `pruneManagedOnly` and `estimateManagedReclaim` walked the Sencho-managed prunable image set and summed `img.Size` per image. That counts shared base layers once per image, so a 1 GB base layer shared across N managed images was reported as N GB freed — the same shape of inflation we just fixed for the system-scope banner. Introduce `getImageSharedSizeMap()` which reads `df.Images[].SharedSize` once and lets both code paths subtract `SharedSize` per image when totalling: `+= max(0, Size - shared)`. If `df` fails, the helper returns an empty map and the accounting degrades to the prior sum-of-Size behavior rather than failing the prune. Verified against a live daemon: `/api/system/prune/estimate` with `scope: managed, target: images` now returns the layer-aware number; the older per-image-Size sum was roughly 1.8× larger on the same set. * fix(resources): use df-delta for destructive managed prune; label estimate as lower bound The first attempt at this PR subtracted SharedSize per prunable image on both paths. That formula undercounts when prunable images share a layer exclusively with each other: Docker frees the layer once, but the per-image subtraction removes it from every referrer. The reported total is then strictly less than the truth. Split the two paths: - pruneManagedOnly (destructive) now snapshots `docker df` before and after the parallel removes and reports `max(0, before.LayersSize - after.LayersSize)`. That is the honest measurement of bytes freed. Concurrent pulls during the prune can grow the after value; the clamp treats that as 0 reclaimed for the affected delta rather than attributing the new bytes to us. - estimateManagedReclaim keeps the per-image Σ(Size - SharedSize) formula but the JSDoc now calls it a "conservative lower bound" and documents the under-report mechanism. There is no cheap way to exactly price an arbitrary prune subset without per-layer enumeration. Fallback chain when df fails on the destructive path: - before-snapshot succeeded, after failed → per-image lower bound from before-snapshot (safe; SharedSize was known at start). - before-snapshot failed → report 0 with a warn log (after-only would build a SharedSize map missing the just-pruned images, which would over-report by treating them as having no sharing). Replaces the prior `getImageSharedSizeMap()` helper with two pieces: `safeDfSnapshot()` (I/O) and a private static `mapSharedSizesFromDf()` (pure parse), reused by both code paths. New invariant test asserts `prune.reclaimedBytes >= estimate.reclaimableBytes` on the same inputs so future changes to either formula cannot flip the direction. Addresses Codex audit blocker on PR #1155. --- .../src/__tests__/docker-controller.test.ts | 204 ++++++++++++++++++ backend/src/services/DockerController.ts | 73 ++++++- 2 files changed, 275 insertions(+), 2 deletions(-) diff --git a/backend/src/__tests__/docker-controller.test.ts b/backend/src/__tests__/docker-controller.test.ts index 3ba691b6..147e4d41 100644 --- a/backend/src/__tests__/docker-controller.test.ts +++ b/backend/src/__tests__/docker-controller.test.ts @@ -431,6 +431,210 @@ describe('DockerController - getClassifiedResources', () => { }); }); +// ── pruneManagedOnly / estimateManagedReclaim (images) ───────────────── + +describe('DockerController - managed image prune accounting', () => { + // Two unused images each report a 1000-byte rolled-up Size, but 400 of those + // bytes live in a layer shared with a third (in-use) image, so removing + // the unused pair only frees the 600 unique bytes from each. The buggy + // sum-of-Size accounting would have reported 2000; the fix should report + // 1200 (600 × 2). + const sharedLayerDfMock = { + Images: [ + { Id: 'img-a', SharedSize: 400 }, + { Id: 'img-b', SharedSize: 400 }, + { Id: 'img-c', SharedSize: 400 }, + ], + }; + const unusedManagedListMock = [ + { Id: 'img-a', Containers: 0, Size: 1000 }, + { Id: 'img-b', Containers: 0, Size: 1000 }, + { Id: 'img-c', Containers: 1, Size: 800 }, // in-use; filtered by Containers===0 + ]; + + it('estimateManagedReclaim subtracts SharedSize per prunable image', async () => { + mockDocker.df.mockResolvedValue(sharedLayerDfMock); + mockDocker.listImages.mockResolvedValue(unusedManagedListMock); + mockDocker.listContainers.mockResolvedValue([]); + + const dc = DockerController.getInstance(1); + const result = await dc.estimateManagedReclaim('images', ['any-stack']); + + expect(result.reclaimableBytes).toBe(1200); + }); + + it('pruneManagedOnly reports the LayersSize delta as the reclaimed total', async () => { + // df-before reports 5000 bytes on disk; df-after reports 3400. The + // honest reclaim is 1600, independent of per-image Size/SharedSize. + // Two prunable images that share a layer exclusively would be + // undercounted by the per-image formula (1200) but Docker actually + // frees the shared layer too. + mockDocker.df + .mockResolvedValueOnce({ + LayersSize: 5000, + Images: [ + { Id: 'img-a', SharedSize: 400 }, + { Id: 'img-b', SharedSize: 400 }, + ], + }) + .mockResolvedValueOnce({ LayersSize: 3400, Images: [] }); + mockDocker.listImages.mockResolvedValue([ + { Id: 'img-a', Containers: 0, Size: 1000 }, + { Id: 'img-b', Containers: 0, Size: 1000 }, + ]); + mockDocker.listContainers.mockResolvedValue([]); + const removeFn = vi.fn().mockResolvedValue(undefined); + mockDocker.getImage.mockReturnValue({ remove: removeFn }); + + const dc = DockerController.getInstance(1); + const result = await dc.pruneManagedOnly('images', ['any-stack']); + + expect(result).toEqual({ success: true, reclaimedBytes: 1600 }); + expect(removeFn).toHaveBeenCalledTimes(2); + }); + + it('pruneManagedOnly falls back to per-image lower bound using before-snapshot when after-df fails', async () => { + // Before-snapshot succeeded (SharedSize known); after-snapshot failed. + // Fall back to Σ(Size - SharedSize) over the prunable set: a + // conservative lower bound, but defensible because each pruned image's + // SharedSize was known at the start. + mockDocker.df + .mockResolvedValueOnce({ + Images: [ + { Id: 'img-a', SharedSize: 400 }, + { Id: 'img-b', SharedSize: 400 }, + ], + }) + .mockRejectedValueOnce(new Error('df unavailable after prune')); + mockDocker.listImages.mockResolvedValue([ + { Id: 'img-a', Containers: 0, Size: 1000 }, + { Id: 'img-b', Containers: 0, Size: 1000 }, + ]); + mockDocker.listContainers.mockResolvedValue([]); + mockDocker.getImage.mockReturnValue({ remove: vi.fn().mockResolvedValue(undefined) }); + + const dc = DockerController.getInstance(1); + const result = await dc.pruneManagedOnly('images', ['any-stack']); + + // (1000 - 400) + (1000 - 400) = 1200 + expect(result).toEqual({ success: true, reclaimedBytes: 1200 }); + }); + + it('pruneManagedOnly reports 0 reclaimed when both df snapshots fail', async () => { + // Without the before-snapshot we cannot build a meaningful per-image + // bound (after-only has stale image IDs missing from its view), so the + // destructive path completes the removes and reports 0 rather than a + // misleading approximation. + mockDocker.df + .mockRejectedValueOnce(new Error('df unavailable before')) + .mockRejectedValueOnce(new Error('df unavailable after')); + mockDocker.listImages.mockResolvedValue([ + { Id: 'img-a', Containers: 0, Size: 1000 }, + ]); + mockDocker.listContainers.mockResolvedValue([]); + const removeFn = vi.fn().mockResolvedValue(undefined); + mockDocker.getImage.mockReturnValue({ remove: removeFn }); + + const dc = DockerController.getInstance(1); + const result = await dc.pruneManagedOnly('images', ['any-stack']); + + expect(result).toEqual({ success: true, reclaimedBytes: 0 }); + expect(removeFn).toHaveBeenCalledTimes(1); + }); + + it('estimateManagedReclaim under-reports when layers are shared exclusively between prunable images', async () => { + // Two prunable images that share a 400-byte layer with no retained + // image. Docker would free that layer once on prune (1600 bytes total: + // each image's unique 600 plus the shared 400). The estimate formula + // subtracts 400 from each image and reports 1200, a conservative + // lower bound documented in the JSDoc. + mockDocker.df.mockResolvedValue({ + Images: [ + { Id: 'img-a', SharedSize: 400 }, + { Id: 'img-b', SharedSize: 400 }, + ], + }); + mockDocker.listImages.mockResolvedValue([ + { Id: 'img-a', Containers: 0, Size: 1000 }, + { Id: 'img-b', Containers: 0, Size: 1000 }, + ]); + mockDocker.listContainers.mockResolvedValue([]); + + const dc = DockerController.getInstance(1); + const result = await dc.estimateManagedReclaim('images', ['any-stack']); + + expect(result.reclaimableBytes).toBe(1200); + }); + + it('invariant: pruneManagedOnly reports >= estimateManagedReclaim on the same inputs', async () => { + // The estimate is a conservative lower bound; the destructive path + // reports the truth (df-delta). On any input, destructive >= estimate. + // This locks the contract so future changes to either formula cannot + // accidentally flip the direction. + const dfBefore = { + LayersSize: 5000, + Images: [ + { Id: 'img-a', SharedSize: 400 }, + { Id: 'img-b', SharedSize: 400 }, + ], + }; + const dfAfter = { LayersSize: 3400, Images: [] }; + const listImages = [ + { Id: 'img-a', Containers: 0, Size: 1000 }, + { Id: 'img-b', Containers: 0, Size: 1000 }, + ]; + + // Estimate path: df called once. + mockDocker.df.mockResolvedValueOnce(dfBefore); + mockDocker.listImages.mockResolvedValue(listImages); + mockDocker.listContainers.mockResolvedValue([]); + const dc = DockerController.getInstance(1); + const estimate = await dc.estimateManagedReclaim('images', ['any-stack']); + + // Reset the df mock for the destructive path: before then after. + mockDocker.df.mockReset(); + mockDocker.df.mockResolvedValueOnce(dfBefore).mockResolvedValueOnce(dfAfter); + mockDocker.getImage.mockReturnValue({ remove: vi.fn().mockResolvedValue(undefined) }); + const prune = await dc.pruneManagedOnly('images', ['any-stack']); + + expect(prune.reclaimedBytes).toBeGreaterThanOrEqual(estimate.reclaimableBytes); + }); + + it('treats images missing from the df map as having no shared layers', async () => { + // df reports only img-a; img-b is omitted (stale / race / older daemon). + // The accounting should still process img-b, defaulting its SharedSize to 0. + mockDocker.df.mockResolvedValue({ Images: [{ Id: 'img-a', SharedSize: 400 }] }); + mockDocker.listImages.mockResolvedValue([ + { Id: 'img-a', Containers: 0, Size: 1000 }, + { Id: 'img-b', Containers: 0, Size: 1000 }, + ]); + mockDocker.listContainers.mockResolvedValue([]); + + const dc = DockerController.getInstance(1); + const result = await dc.estimateManagedReclaim('images', ['any-stack']); + + // img-a: 1000-400=600; img-b: 1000-0=1000; total 1600 + expect(result.reclaimableBytes).toBe(1600); + }); + + it('degrades to no-sharing assumption when df fails', async () => { + // If df throws, the helper returns an empty map and the accounting falls + // back to full per-image Size. This preserves prior behavior (over-report) + // rather than failing the prune. + mockDocker.df.mockRejectedValue(new Error('daemon df failed')); + mockDocker.listImages.mockResolvedValue([ + { Id: 'img-a', Containers: 0, Size: 1000 }, + { Id: 'img-b', Containers: 0, Size: 1000 }, + ]); + mockDocker.listContainers.mockResolvedValue([]); + + const dc = DockerController.getInstance(1); + const result = await dc.estimateManagedReclaim('images', ['any-stack']); + + expect(result.reclaimableBytes).toBe(2000); + }); +}); + // ── getOrphanContainers ──────────────────────────────────────────────── describe('DockerController - getOrphanContainers', () => { diff --git a/backend/src/services/DockerController.ts b/backend/src/services/DockerController.ts index c61c7844..c7233be1 100644 --- a/backend/src/services/DockerController.ts +++ b/backend/src/services/DockerController.ts @@ -363,6 +363,42 @@ class DockerController { return { images, volumes, networks }; } + /** + * Returns the `docker df` snapshot, or null if the call fails. The + * destructive prune path uses this for a `LayersSize` before/after delta; + * the estimate path uses it for a SharedSize lookup. Null on failure lets + * each caller decide how to degrade rather than throwing mid-prune. + */ + private async safeDfSnapshot(): Promise<{ + LayersSize?: number; + Images?: Array<{ Id?: string; SharedSize?: number }>; + } | null> { + try { + return await this.docker.df(); + } catch { + return null; + } + } + + /** + * Extracts `Id -> SharedSize` from a df snapshot. Treats missing or + * negative (Docker's "unknown" sentinel) SharedSize as 0 so the caller + * counts the image's full Size as unique, an under-report rather than an + * over-report. + */ + private static mapSharedSizesFromDf( + df: { Images?: Array<{ Id?: string; SharedSize?: number }> } | null, + ): Map { + const m = new Map(); + if (!df?.Images) return m; + for (const img of df.Images) { + if (!img?.Id) continue; + const s = typeof img.SharedSize === 'number' && img.SharedSize >= 0 ? img.SharedSize : 0; + m.set(img.Id, s); + } + return m; + } + public async pruneManagedOnly( target: 'images' | 'volumes' | 'networks', knownStackNames: string[] @@ -422,14 +458,37 @@ class DockerController { && !unmanagedImageIds.has(img.Id) && !selfIdentity.isOwnImage(img.Id) ); + // df-before / df-after delta is the only honest measurement of bytes + // actually freed. Per-image (Size - SharedSize) undercounts layers + // shared exclusively between prunable images (Docker frees the layer + // once, but the per-image formula subtracts it from every referrer). + const beforeDf = await this.safeDfSnapshot(); await Promise.all(prunable.map(async (img) => { try { await this.docker.getImage(img.Id).remove({ force: true }); - reclaimedBytes += img.Size ?? 0; } catch (e) { console.error(`[pruneManagedOnly] Failed to remove image ${img.Id}:`, e); } })); + const afterDf = await this.safeDfSnapshot(); + if (typeof beforeDf?.LayersSize === 'number' && typeof afterDf?.LayersSize === 'number') { + // Clamp to 0: a concurrent pull during the prune can grow on-disk + // bytes, and attributing that growth to "reclaimed" would mislead. + reclaimedBytes = Math.max(0, beforeDf.LayersSize - afterDf.LayersSize); + } else if (beforeDf) { + // After-snapshot failed but we have the before-snapshot, so fall + // back to a per-image lower bound. (We deliberately do not fall back + // on after-only: after the prune those image IDs are gone from the + // daemon's view, so a SharedSize map built from afterDf would treat + // every pruned image as having no sharing and over-report.) + console.warn('[pruneManagedOnly] docker df after-snapshot unavailable; reporting per-image lower bound'); + const shared = DockerController.mapSharedSizesFromDf(beforeDf); + for (const img of prunable) { + reclaimedBytes += Math.max(0, (img.Size ?? 0) - (shared.get(img.Id) ?? 0)); + } + } else { + console.warn('[pruneManagedOnly] docker df unavailable on both ends; reporting 0 reclaimed'); + } } return { success: true, reclaimedBytes }; @@ -440,6 +499,13 @@ class DockerController { * the `/api/system/prune/estimate` route. Walks the same filter rules but * does not call `.remove()`. Kept structurally parallel to the destructive * method so the two stay in lockstep when the enumeration logic changes. + * + * For images, the returned figure is a **conservative lower bound** on + * bytes that will actually be freed. The formula subtracts each prunable + * image's `SharedSize`, which double-counts layers shared between two + * prunable images (the layer would only be freed once on prune). The + * exact freed bytes can only be measured by the df-delta that + * `pruneManagedOnly` reports after the destructive action. */ public async estimateManagedReclaim( target: 'images' | 'volumes' | 'networks', @@ -479,7 +545,10 @@ class DockerController { && !unmanagedImageIds.has(img.Id) && !selfIdentity.isOwnImage(img.Id), ); - for (const img of prunable) reclaimableBytes += img.Size ?? 0; + const sharedSizes = DockerController.mapSharedSizesFromDf(await this.safeDfSnapshot()); + for (const img of prunable) { + reclaimableBytes += Math.max(0, (img.Size ?? 0) - (sharedSizes.get(img.Id) ?? 0)); + } } return { reclaimableBytes };