From 2483335e03265791c4e4f76b4894da1c51bdaf1d Mon Sep 17 00:00:00 2001 From: deveshctl Date: Fri, 7 Aug 2026 20:34:22 +0530 Subject: [PATCH 1/2] fix(efficiency): count add-then-delete bytes as waste A file added in one layer and deleted (whiteout or opaque whiteout) in a later layer was contributing zero to WastedBytes and never appeared in WastedFiles. Deleting a file in a later layer only records a whiteout; it never reclaims the earlier layer's bytes, which remain stored in the image and are transferred on every pull. Treating those bytes as "cleaned up" is correct for a live filesystem but wrong for an immutable layered image, and it let the classic download-build-rm bloat pattern pass the efficiency and wasted-bytes gates undetected. pathRuns now records why each run ended: a run closed by a deletion charges all of its occurrences as waste (nothing survives, everything shipped), while a run still live at the top of the stack keeps its last occurrence as before. The score formula, CI rules, thresholds, exit codes, and JSON shape are unchanged; affected images simply report an honest, higher waste figure. --- CHANGELOG.md | 8 ++++ docs/ci-integration.md | 6 +++ docs/configuration.md | 5 +++ docs/json-export.md | 2 +- image/efficiency.go | 60 ++++++++++++++++++++------- image/efficiency_test.go | 90 +++++++++++++++++++++++++++++++++------- 6 files changed, 140 insertions(+), 31 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5129e0..734957e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed +- Efficiency analysis now counts a file that is added in one layer and deleted + in a later layer as wasted space. Deleting a file in a later layer records a + whiteout but does not reclaim the earlier layer's bytes — they remain stored + in the image and are transferred on every pull. Such files now contribute to + `wastedBytes`, appear in the wasted-files list (JSON export, TUI waste + navigator `w`), and lower the efficiency score. Scores may drop for images + built with the common "download, build, then `rm`" pattern; the score formula, + CI rules, thresholds, and exit codes are unchanged. - The interactive file tree now caches its flatten/filter/sort result between redraws, so holding a navigation key or scrolling a large image's tree stays responsive instead of recomputing the whole tree on every frame. The cache diff --git a/docs/ci-integration.md b/docs/ci-integration.md index cd5663b..8785c21 100644 --- a/docs/ci-integration.md +++ b/docs/ci-integration.md @@ -24,6 +24,12 @@ Rules come from `.layerx.yaml` (in the working directory) and can be overridden inline with CLI flags for the three global thresholds. If `.layerx.yaml` is missing, built-in defaults apply. +The efficiency figure counts a file that is added in one layer and deleted in +a later one as wasted — the delete records a whiteout but never reclaims the +earlier layer's bytes, so they still ship. A score may therefore be lower than +a tool that only counts files duplicated across layers. The rules and +thresholds themselves are unchanged. + Two ways to invoke CI mode: ```bash diff --git a/docs/configuration.md b/docs/configuration.md index 1a9407d..6580768 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -123,6 +123,11 @@ decoder is strict. Range validation rejects `NaN`, `±Inf`, negative byte counts, and floats outside `[0, 1]`. +Wasted bytes include files added in one layer and deleted in a later one: the +delete records a whiteout but never reclaims the earlier layer's bytes, so they +still ship in the image. A score may be lower than a tool that counts only +files duplicated across layers. + ### `path-rules` — flat form (mapping) | Field | Type | Default | Required | Description | diff --git a/docs/json-export.md b/docs/json-export.md index e8805c4..d4c2243 100644 --- a/docs/json-export.md +++ b/docs/json-export.md @@ -122,7 +122,7 @@ Schema version `1.0.1`. Pretty-printed with two-space indent. | Field | Type | Description | |---|---|---| | `score` | float64 | `1.0 − wastedBytes / (liveBytes + wastedBytes)`, in `[0.0, 1.0]`. Higher is better. | -| `wastedBytes` | int64 | Bytes duplicated across layers (file-path appears in more than one layer; only first occurrence counts as "live"). | +| `wastedBytes` | int64 | Bytes that ship in the image but are not live in the final filesystem. Two cases: a file rewritten across layers (only the last copy is live; earlier copies are waste), and a file added in one layer then deleted in a later one (the delete records a whiteout but never reclaims the earlier layer's bytes, so the whole file is waste). | | `wastedFiles` | array | Per-file waste breakdown. Always `[]` when empty (never `null`). Sorted by `totalWasted` descending, then `path` ascending. | #### `efficiency.wastedFiles[]` diff --git a/image/efficiency.go b/image/efficiency.go index 9a79dc6..a7257d5 100644 --- a/image/efficiency.go +++ b/image/efficiency.go @@ -18,10 +18,12 @@ type EfficiencyResult struct { // layers. It stacks layers internally; prefer EfficiencyFromAnalysis when the // caller already has stacked trees. // -// A file at the same path counted across runs separated by deletion (whiteout -// or opaque whiteout) is NOT considered wasted: the deleted copy was properly -// cleaned up before the new copy appeared. Within a single run (no deletion -// in between), all but the last occurrence are wasted. +// Within a run of a path's occurrences (a span with no deletion in between), +// all but the last occurrence are wasted — the last is the copy that survives. +// A run that ends in a deletion (whiteout or opaque whiteout) has ALL of its +// occurrences wasted: the bytes were written into earlier layers, still ship in +// the image, and are never reclaimed by the later deletion, yet none of them +// survive into the final filesystem. func Efficiency(layers []Layer) *EfficiencyResult { if len(layers) == 0 { return &EfficiencyResult{Score: 1.0} @@ -56,6 +58,16 @@ type efficiencyOccurrence struct { size int64 } +// pathRun is one contiguous span of a path's Added/Modified occurrences. +// endedInDeletion records why the span closed: true when a whiteout, opaque +// whiteout, or absence removed the path (nothing from the run survives into the +// final image), false when the run reached the top of the layer stack still +// live. The charge rule differs between the two — see computeEfficiency. +type pathRun struct { + occ []efficiencyOccurrence + endedInDeletion bool +} + func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { // Build a path→FileNode index per stacked snapshot once. pathRuns then does // O(1) lookups instead of recursing through the tree once per (path, @@ -89,7 +101,7 @@ func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { var pathWaste int64 var occurrenceCount int for _, run := range runs { - for _, occ := range run { + for _, occ := range run.occ { // LayerCount documents "how many layers contributed bytes". // Zero-size occurrences (hardlink replacements that extend a // run only to keep the earlier real-file bytes chargeable) @@ -98,10 +110,19 @@ func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { occurrenceCount++ } } - if len(run) < 2 { - continue + // A run that ended in deletion ships every one of its copies with + // nothing surviving into the final image, so all occurrences are + // waste. A run still live at the top of the stack keeps its last + // occurrence (the copy present in the image); only the earlier, + // shadowed copies are waste. + charged := run.occ + if !run.endedInDeletion { + if len(run.occ) < 2 { + continue + } + charged = run.occ[:len(run.occ)-1] } - for _, occ := range run[:len(run)-1] { + for _, occ := range charged { pathWaste += occ.size } } @@ -189,34 +210,41 @@ func indexTree(node *FileNode, idx map[string]*FileNode) { // occurrence is recorded only at snapshots where the path was Added or // Modified — the layer that actually wrote new bytes. Unchanged carryover does // not contribute. -func pathRuns(path string, indices []map[string]*FileNode) [][]efficiencyOccurrence { - var runs [][]efficiencyOccurrence +// +// Each run records whether it ended in a deletion. A run flushed by a Removed +// node, an absence, or a nil index is marked endedInDeletion — none of its +// bytes survive into the final image, yet all of them shipped, so +// computeEfficiency charges every occurrence. A run flushed only by reaching +// the end of the layer stack is still live; its last occurrence is the copy +// present in the final image and is not waste. +func pathRuns(path string, indices []map[string]*FileNode) []pathRun { + var runs []pathRun var cur []efficiencyOccurrence - flush := func() { + flush := func(deleted bool) { if len(cur) > 0 { - runs = append(runs, cur) + runs = append(runs, pathRun{occ: cur, endedInDeletion: deleted}) cur = nil } } for i, idx := range indices { if idx == nil { - flush() + flush(true) continue } node, ok := idx[path] switch { case !ok: - flush() + flush(true) case node.DiffType == Removed: - flush() + flush(true) case node.DiffType == Added || node.DiffType == Modified: cur = append(cur, efficiencyOccurrence{layerIdx: i, size: node.Size}) } // Unchanged carryover: skip (no new bytes; don't flush). } - flush() + flush(false) return runs } diff --git a/image/efficiency_test.go b/image/efficiency_test.go index b427861..6546d1a 100644 --- a/image/efficiency_test.go +++ b/image/efficiency_test.go @@ -145,7 +145,11 @@ func TestEfficiency_StableOrderOnEqualWaste(t *testing.T) { // both copies as a duplicate occurrence of the same path and flagged the // first as waste — the canonical apt-get install + apt-get clean + // apt-get install bug. -func TestEfficiency_InstallCleanReinstall_NoWaste(t *testing.T) { +// install -> clean -> reinstall: the first copy is added, deleted by a +// whiteout, then a fresh copy is added at the same path. The deleted first +// copy still ships in its layer and is never reclaimed, so it is wasted; the +// reinstall is a fresh live run and is not. +func TestEfficiency_InstallCleanReinstall_FirstCopyWasted(t *testing.T) { layers := []Layer{ {Index: 0, Size: 100, Tree: makeTree( makeDir("var", "/var", @@ -164,9 +168,11 @@ func TestEfficiency_InstallCleanReinstall_NoWaste(t *testing.T) { )}, } result := Efficiency(layers) - assert.Equal(t, int64(0), result.WastedBytes, - "a deletion between two writes means the first copy was properly cleaned up — not wasted") - assert.Empty(t, result.WastedFiles) + assert.Equal(t, int64(100), result.WastedBytes, + "the deleted first copy ships in layer 0 and is never reclaimed by the later whiteout") + require.Len(t, result.WastedFiles, 1) + assert.Equal(t, "/var/x", result.WastedFiles[0].Path) + assert.Equal(t, int64(100), result.WastedFiles[0].TotalWasted) } // duplicate_in_same_run: a file is added, then modified in the next layer @@ -192,10 +198,11 @@ func TestEfficiency_DuplicateInSameRun_IsWasted(t *testing.T) { assert.Equal(t, int64(100), result.WastedFiles[0].TotalWasted) } -// install -> modify -> clean -> reinstall: only the install->modify pair -// inside the first run contributes waste; the post-clean reinstall is a -// fresh run with one occurrence (no waste). -func TestEfficiency_InstallModifyCleanReinstall_OnlyFirstRunWasted(t *testing.T) { +// install -> modify -> clean -> reinstall: the first run (install then modify, +// no deletion in between) ends in a whiteout. Both of its copies shipped and +// neither survives, so both are wasted (100 + 80). The post-clean reinstall is +// a fresh live run and is not. +func TestEfficiency_InstallModifyCleanReinstall_FirstRunFullyWasted(t *testing.T) { layers := []Layer{ {Index: 0, Size: 100, Tree: makeTree( makeDir("etc", "/etc", @@ -219,12 +226,14 @@ func TestEfficiency_InstallModifyCleanReinstall_OnlyFirstRunWasted(t *testing.T) )}, } result := Efficiency(layers) - assert.Equal(t, int64(100), result.WastedBytes, - "first run has occurrences (0,100)+(1,80); only the size-100 layer-0 copy is shadowed; the post-clean reinstall starts a new run") + assert.Equal(t, int64(180), result.WastedBytes, + "the first run (100 then 80) ends in a whiteout, so both shipped copies are wasted; the reinstall is a fresh live run") } -// Opaque whiteout breaks a run just like an explicit per-file whiteout. -func TestEfficiency_OpaqueWhiteout_BreaksRun(t *testing.T) { +// Opaque whiteout ends a run just like an explicit per-file whiteout: the copy +// written before the opaque marker still ships and is never reclaimed, so it is +// wasted. The post-opaque copy is a fresh live run. +func TestEfficiency_OpaqueWhiteout_PreOpaqueCopyWasted(t *testing.T) { layers := []Layer{ {Index: 0, Size: 100, Tree: makeTree( makeDir("var", "/var", @@ -249,8 +258,61 @@ func TestEfficiency_OpaqueWhiteout_BreaksRun(t *testing.T) { )}, } result := Efficiency(layers) - assert.Equal(t, int64(0), result.WastedBytes, - "opaque whiteout should reset run; the post-opaque copy is fresh") + assert.Equal(t, int64(100), result.WastedBytes, + "the copy written before the opaque whiteout ships and is never reclaimed") + require.Len(t, result.WastedFiles, 1) + assert.Equal(t, "/var/cache/x", result.WastedFiles[0].Path) +} + +// Pure add-then-delete: a file is added in one layer and whiteouted in the +// next, with no reintroduction. Every byte shipped in the earlier layer and +// none survives — the whole thing is waste, and the path is listed even though +// it is absent from the final image. +func TestEfficiency_AddThenDelete_FullyWasted(t *testing.T) { + layers := []Layer{ + {Index: 0, Size: 1000, Tree: makeTree( + makeFile("big", "/big", 1000), + )}, + {Index: 1, Size: 0, Tree: makeTree( + makeFile(".wh.big", "/.wh.big", 0), + )}, + } + result := Efficiency(layers) + assert.Equal(t, int64(1000), result.WastedBytes, + "an added-then-deleted file ships its bytes and is never reclaimed") + require.Len(t, result.WastedFiles, 1) + assert.Equal(t, "/big", result.WastedFiles[0].Path) + assert.Equal(t, int64(1000), result.WastedFiles[0].TotalWasted) + assert.Equal(t, 1, result.WastedFiles[0].LayerCount) +} + +// add -> modify -> delete, with no reintroduction: both shipped copies are in +// one run that ends in deletion, so both are charged. +func TestEfficiency_AddModifyThenDelete_BothCopiesWasted(t *testing.T) { + layers := []Layer{ + {Index: 0, Size: 100, Tree: makeTree(makeFile("x", "/x", 100))}, + {Index: 1, Size: 70, Tree: makeTree(makeFile("x", "/x", 70))}, + {Index: 2, Size: 0, Tree: makeTree(makeFile(".wh.x", "/.wh.x", 0))}, + } + result := Efficiency(layers) + assert.Equal(t, int64(170), result.WastedBytes, + "both the added and modified copies ship and neither survives the whiteout") + require.Len(t, result.WastedFiles, 1) + assert.Equal(t, int64(170), result.WastedFiles[0].TotalWasted) + assert.Equal(t, 2, result.WastedFiles[0].LayerCount) +} + +// Regression guard: a file added once and never duplicated or deleted stays +// live and contributes zero waste. The deletion-aware charge rule must not +// touch the ordinary single-occurrence live case. +func TestEfficiency_SingleLiveFile_NoWaste(t *testing.T) { + layers := []Layer{ + {Index: 0, Size: 500, Tree: makeTree(makeFile("only", "/only", 500))}, + } + result := Efficiency(layers) + assert.Equal(t, int64(0), result.WastedBytes) + assert.Empty(t, result.WastedFiles) + assert.Equal(t, 1.0, result.Score) } // regular_file_replaced_by_hardlink: a file is added as real bytes in layer 0 From 46bdbd76a8cb58978b6c40269f310c96ceb82055 Mon Sep 17 00:00:00 2001 From: deveshctl Date: Sat, 8 Aug 2026 07:25:33 +0530 Subject: [PATCH 2/2] fix(efficiency): correct LayerCount for deleted-then-reinstalled paths MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit occurrenceCount was accumulated over all runs unconditionally, so a reinstalled file's live single-occurrence run inflated LayerCount even though that copy is the surviving keeper and contributes no waste. Fix: only accumulate occurrenceCount for runs that actually contribute waste — the single-occurrence live run continue now skips the count loop too, so the reinstall copy is not counted. Also removes the stale pre-Round-8 comment paragraph from TestEfficiency_InstallCleanReinstall_FirstCopyWasted that contradicted the test's own assertion, and adds a LayerCount assertion to pin the corrected behaviour. --- image/efficiency.go | 24 +++++++++++++++--------- image/efficiency_test.go | 8 ++------ 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/image/efficiency.go b/image/efficiency.go index a7257d5..4839088 100644 --- a/image/efficiency.go +++ b/image/efficiency.go @@ -101,15 +101,6 @@ func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { var pathWaste int64 var occurrenceCount int for _, run := range runs { - for _, occ := range run.occ { - // LayerCount documents "how many layers contributed bytes". - // Zero-size occurrences (hardlink replacements that extend a - // run only to keep the earlier real-file bytes chargeable) - // are not byte-contributors and must not inflate the count. - if occ.size > 0 { - occurrenceCount++ - } - } // A run that ended in deletion ships every one of its copies with // nothing surviving into the final image, so all occurrences are // waste. A run still live at the top of the stack keeps its last @@ -118,6 +109,9 @@ func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { charged := run.occ if !run.endedInDeletion { if len(run.occ) < 2 { + // Single live occurrence with no prior copy in this run: no + // waste, and the path's reinstall copy must not inflate + // LayerCount for waste entries produced by earlier deleted runs. continue } charged = run.occ[:len(run.occ)-1] @@ -125,6 +119,18 @@ func computeEfficiency(layers []Layer, stacked []*FileTree) *EfficiencyResult { for _, occ := range charged { pathWaste += occ.size } + // LayerCount counts byte-contributing occurrences across charged + // copies only. For deleted runs, every occurrence is charged. For + // live runs, the surviving last copy is excluded from charged but + // is still a real byte-contributor visible to the user, so include + // all non-zero occurrences in the run (not just the charged slice). + // The single-occurrence live run above is skipped entirely, so + // reinstalled copies from a separate run do not inflate the count. + for _, occ := range run.occ { + if occ.size > 0 { + occurrenceCount++ + } + } } if pathWaste == 0 { // Path appears in multiple layers but every duplicate has size 0 diff --git a/image/efficiency_test.go b/image/efficiency_test.go index 6546d1a..b334c65 100644 --- a/image/efficiency_test.go +++ b/image/efficiency_test.go @@ -139,12 +139,6 @@ func TestEfficiency_StableOrderOnEqualWaste(t *testing.T) { } } -// install_clean_reinstall: a file is added, deleted via whiteout, and a -// different file appears at the same path. The two distinct files live in -// separate runs and neither is wasted. Pre-Round-8 the algorithm counted -// both copies as a duplicate occurrence of the same path and flagged the -// first as waste — the canonical apt-get install + apt-get clean + -// apt-get install bug. // install -> clean -> reinstall: the first copy is added, deleted by a // whiteout, then a fresh copy is added at the same path. The deleted first // copy still ships in its layer and is never reclaimed, so it is wasted; the @@ -173,6 +167,8 @@ func TestEfficiency_InstallCleanReinstall_FirstCopyWasted(t *testing.T) { require.Len(t, result.WastedFiles, 1) assert.Equal(t, "/var/x", result.WastedFiles[0].Path) assert.Equal(t, int64(100), result.WastedFiles[0].TotalWasted) + assert.Equal(t, 1, result.WastedFiles[0].LayerCount, + "only the deleted copy in layer 0 is waste; the reinstall in layer 2 is the live keeper") } // duplicate_in_same_run: a file is added, then modified in the next layer