From a1caf6b0dd8269911a61adff9bb31e1ad1a01461 Mon Sep 17 00:00:00 2001 From: Anso Date: Fri, 22 May 2026 02:40:06 -0400 Subject: [PATCH] fix(resources): use daemon-reported reclaimable image bytes (#1154) * fix(resources): use daemon-reported reclaimable image bytes The Reclaim banner summed per-image `VirtualSize` (or `Size`) across every image with no running container. That counts shared base layers once per image, so an unused base layer of 1 GB shared across ten builds showed up as 10 GB of "prunable" space. The actual prune frees the layer once and reports a much smaller `SpaceReclaimed`, leaving the banner and the post-prune toast badly out of step. Prefer Docker's own `ImageUsage.Reclaimable` (API v1.44+), which is the exact value `docker system df` displays. Older daemons fall back to the Docker CLI's internal formula: `LayersSize - sum(Size - SharedSize)` for in-use images, clamped to 0, skipping any image Docker flags with the -1 unknown-size sentinel. Verified against a live daemon: the banner now matches `docker system df`'s IMAGES RECLAIMABLE byte-for-byte. * fix(resources): treat SharedSize=-1 as 0 in fallback, don't drop in-use bytes The fallback formula is `LayersSize - used`. The previous version skipped any active image whose VirtualSize or SharedSize was -1 (Docker's "unknown" sentinel). Skipping leaves the image's bytes out of `used`, which reads back as reclaimable -- the exact inflation the PR set out to fix, just on older daemons. Treat SharedSize=-1 (or absent) as 0 so the image's full size counts as in-use, and only skip when no usable size is available at all (VirtualSize and Size both unknown). Under-reporting reclaimable is the safe direction; over-reporting was the original bug. Add a test for the SharedSize=-1 case with Size known, and rename the existing test so it reflects what is actually being asserted now. Addresses Codex audit blocker on PR #1154. --- .../src/__tests__/docker-controller.test.ts | 100 +++++++++++++++++- backend/src/services/DockerController.ts | 46 ++++++-- 2 files changed, 135 insertions(+), 11 deletions(-) diff --git a/backend/src/__tests__/docker-controller.test.ts b/backend/src/__tests__/docker-controller.test.ts index c13774dc..3ba691b6 100644 --- a/backend/src/__tests__/docker-controller.test.ts +++ b/backend/src/__tests__/docker-controller.test.ts @@ -167,9 +167,10 @@ describe('DockerController - removeContainers', () => { describe('DockerController - getDiskUsage', () => { it('calculates reclaimable space correctly', async () => { mockDocker.df.mockResolvedValue({ + LayersSize: 800, // total image-layer bytes on disk Images: [ - { Id: 'img1', Containers: 0, Size: 500 }, // reclaimable (unused) - { Id: 'img2', Containers: 1, Size: 300 }, // not reclaimable (in use) + { Id: 'img1', Containers: 0, Size: 500, SharedSize: 0 }, // reclaimable (unused) + { Id: 'img2', Containers: 1, Size: 300, SharedSize: 0 }, // not reclaimable (in use); used = 300 ], Containers: [ { State: 'running', SizeRw: 100 }, // not reclaimable (running) @@ -188,13 +189,108 @@ describe('DockerController - getDiskUsage', () => { const dc = DockerController.getInstance(1); const usage = await dc.getDiskUsage(); + // 800 LayersSize - 300 used-by-in-use-image = 500 reclaimable expect(usage.reclaimableImages).toBe(500); + expect(usage.reclaimableImageCount).toBe(1); expect(usage.reclaimableContainers).toBe(200); expect(usage.reclaimableVolumes).toBe(400); expect(usage.reclaimableBuildCache).toBe(600); expect(usage.reclaimableBuildCacheCount).toBe(1); }); + it('uses daemon-reported ImageUsage.Reclaimable when present (API v1.44+)', async () => { + // Modern Docker daemons compute Reclaimable server-side and ship the exact + // value `docker system df` displays. Trust it over any client-side fallback. + mockDocker.df.mockResolvedValue({ + LayersSize: 10_000_000_000, + ImageUsage: { Reclaimable: 7_837_305_085, TotalSize: 10_000_000_000, ActiveCount: 11, TotalCount: 33 }, + Images: [ + { Id: 'a', Containers: 0, Size: 5_000_000_000, SharedSize: 0 }, // a fallback formula would say 5G + { Id: 'b', Containers: 1, Size: 3_000_000_000, SharedSize: 0 }, + ], + Containers: [], + Volumes: [], + }); + + const dc = DockerController.getInstance(1); + const usage = await dc.getDiskUsage(); + + expect(usage.reclaimableImages).toBe(7_837_305_085); + expect(usage.reclaimableImageCount).toBe(1); + }); + + it('does not double-count shared layers across unused images', async () => { + // Three images sharing a 400MB base layer: two unused (800MB virtual each), + // one in-use (600MB virtual). Total on-disk: 1GB. The in-use image holds + // its unique 200MB; pruning the two unused frees the remaining 800MB. + // The previous formula summed VirtualSize and would have returned 1.6GB + // (impossibly larger than LayersSize). + mockDocker.df.mockResolvedValue({ + LayersSize: 1_000_000_000, + Images: [ + { Id: 'a', Containers: 0, VirtualSize: 800_000_000, SharedSize: 400_000_000 }, + { Id: 'b', Containers: 0, VirtualSize: 800_000_000, SharedSize: 400_000_000 }, + { Id: 'c', Containers: 1, VirtualSize: 600_000_000, SharedSize: 400_000_000 }, + ], + Containers: [], + Volumes: [], + }); + + const dc = DockerController.getInstance(1); + const usage = await dc.getDiskUsage(); + + expect(usage.reclaimableImages).toBe(800_000_000); + expect(usage.reclaimableImageCount).toBe(2); + }); + + it('skips active images only when no usable size is available', async () => { + // Truly unaccountable image: both VirtualSize and Size are -1 / missing. + // Skipping leaks at most one image's worth of bytes into the reclaim + // total, but modern daemons never return this shape; the prior "always + // skip on -1" path moved any image with SharedSize=-1 into the leak set. + mockDocker.df.mockResolvedValue({ + LayersSize: 1000, + Images: [ + { Id: 'known', Containers: 1, VirtualSize: 400, SharedSize: 100 }, + { Id: 'truly-lost', Containers: 1, VirtualSize: -1, Size: -1 }, + ], + Containers: [], + Volumes: [], + }); + + const dc = DockerController.getInstance(1); + const usage = await dc.getDiskUsage(); + + // truly-lost is unaccountable and skipped; used = 400 - 100 = 300 + expect(usage.reclaimableImages).toBe(700); + expect(usage.reclaimableImageCount).toBe(0); + }); + + it('treats SharedSize=-1 conservatively (full Size counts as used)', async () => { + // Older daemons may report SharedSize as -1 (unknown) while Size is + // accurate. Treating SharedSize as 0 in that case under-reports + // reclaimable, which is the safe direction; the prior skip-on-(-1) + // path made the image's full Size look reclaimable. + mockDocker.df.mockResolvedValue({ + LayersSize: 1000, + Images: [ + { Id: 'modern', Containers: 1, Size: 400, SharedSize: 100 }, + { Id: 'no-shared-info', Containers: 1, Size: 300, SharedSize: -1 }, + ], + Containers: [], + Volumes: [], + }); + + const dc = DockerController.getInstance(1); + const usage = await dc.getDiskUsage(); + + // modern: used += 400 - 100 = 300 + // no-shared-info: shared treated as 0, used += 300 - 0 = 300; total = 600 + // (the old buggy formula skipped no-shared-info, leaving used=300 and + // reclaimable=700 — i.e. the in-use 300 bytes looked reclaimable) + expect(usage.reclaimableImages).toBe(400); + }); + it('handles empty arrays gracefully', async () => { mockDocker.df.mockResolvedValue({ Images: [], diff --git a/backend/src/services/DockerController.ts b/backend/src/services/DockerController.ts index 8c1bd398..c61c7844 100644 --- a/backend/src/services/DockerController.ts +++ b/backend/src/services/DockerController.ts @@ -145,16 +145,41 @@ class DockerController { return { bytes, count: reclaimable.length }; }; - const reclaimableImages = (items: any[]) => { + // Prefer the daemon's own ImageUsage.Reclaimable (Docker API v1.44+); it is + // the exact number `docker system df` displays and accounts for shared + // layers correctly. On older daemons, fall back to LayersSize minus the + // unique-to-in-use bytes, the same arithmetic the CLI uses internally. + // Summing per-image Size/VirtualSize would over-report by the multiplicity + // of every shared layer (the old bug that inflated the banner). + const reclaimableImages = (items: any[], layersSize: number, serverReclaimable: number | undefined) => { if (!items || !Array.isArray(items)) return { bytes: 0, count: 0 }; - const reclaimable = items.filter(i => i.Containers === 0); - const bytes = reclaimable.reduce((acc, item) => { - let size = item.VirtualSize || item.Size || item.SharedSize || 0; - if (item.UsageData && typeof item.UsageData.Size === 'number') { - size = item.UsageData.Size; + const reclaimable = items.filter(i => (i?.Containers ?? 0) === 0); + if (serverReclaimable !== undefined && serverReclaimable >= 0) { + return { bytes: serverReclaimable, count: reclaimable.length }; + } + let used = 0; + for (const item of items) { + if (!item || (item.Containers ?? 0) <= 0) continue; + // Choose the best non-negative size: prefer VirtualSize, fall back to + // Size. If neither is known the image is truly unaccountable; skipping + // it leaks at most one image's worth of bytes into the reclaim total. + let virt = -1; + if (typeof item.VirtualSize === 'number' && item.VirtualSize >= 0) { + virt = item.VirtualSize; + } else if (typeof item.Size === 'number' && item.Size >= 0) { + virt = item.Size; } - return acc + size; - }, 0); + if (virt < 0) continue; + // SharedSize === -1 (or absent) means "unknown" on older daemons. Treat + // it as 0 so the image's full size counts as in-use. Under-reporting + // reclaimable is the safer direction; the previous skip-on-(-1) path + // moved those bytes into the reclaimable total and re-inflated it. + const shared = typeof item.SharedSize === 'number' && item.SharedSize >= 0 + ? item.SharedSize + : 0; + used += Math.max(0, virt - shared); + } + const bytes = Math.max(0, (layersSize || 0) - used); return { bytes, count: reclaimable.length }; }; @@ -175,7 +200,10 @@ class DockerController { return { bytes, count: reclaimable.length }; }; - const images = df.Images ? reclaimableImages(df.Images) : { bytes: 0, count: 0 }; + const imageUsage = (df as { ImageUsage?: { Reclaimable?: number } }).ImageUsage; + const images = df.Images + ? reclaimableImages(df.Images, df.LayersSize ?? 0, imageUsage?.Reclaimable) + : { bytes: 0, count: 0 }; const containers = df.Containers ? reclaimableContainers(df.Containers) : { bytes: 0, count: 0 }; const volumes = df.Volumes ? reclaimableVolumes(df.Volumes) : { bytes: 0, count: 0 }; const buildCache = df.BuildCache ? reclaimableBuildCache(df.BuildCache) : { bytes: 0, count: 0 };