fix(image): layer analysis correctness — missing blobs, dir metadata, hardlink waste - #107
Merged
Merged
Conversation
…a, fix hardlink waste Three correctness fixes to the layer analysis pipeline: 1. parseLayers now returns an error when a manifest-referenced layer blob is absent from the outer tar. Previously the missing layer was silently treated as an empty tree, which could cause CI efficiency rules to pass on genuinely incomplete input. 2. Implicit parent directory nodes (created by insertNode when a tar entry lacks an explicit header for its ancestors) are now marked Inferred. mergeLayerWith skips metadata propagation from inferred nodes, so explicit mode/UID/GID set by an earlier layer is no longer overwritten by a structural placeholder. 3. Hardlink-aliased content is no longer counted as wasted bytes when the original filename is deleted but a hardlink pointing at the same payload survives in the final image. computeEfficiency now builds a set of surviving hardlink targets from the final stacked snapshot and skips waste charging for any deleted path whose content remains reachable through an alias.
… comments Follow-up to the layer analysis correctness fixes: - Apply cleanTarPath to hdr.Linkname in ParseLayerTar so hardlink target paths receive the same ./- and backslash-normalization as hdr.Name. Without this, images from Windows-native or some older build tools emit ./data/a as the linkname, which failed to match the stored /data/a key in walkSurvivingHardlinks and caused the deleted original to be incorrectly charged as waste. - Propagate Inferred in all four FileNode clone functions (cloneAsUnchanged, cloneAsRemoved, cloneAsAdded, cloneWithDiffType, cloneStructure). The field is only read from raw layer-side nodes today, but omitting it from clones would silently break any future reader inspecting the cumulative tree. - Restore the four explanatory comments removed during the hardlink-waste refactor in computeEfficiency: the O(1) index rationale and the LayerCount accounting rule. The LayerCount comment prevents a future reader from treating the run.occ loop as a copy-paste error. - Fix TestParseLayers_MissingLayerBlob_ReturnsError: replace the 1 KiB of raw zeros for layer0 with a valid empty tar so the test exercises the missing-blob guard specifically, not the corrupt-tar parse error.
…red size The previous guard used l.Size > 0 (the declared tar header size) to distinguish a legitimately empty layer from a missing one. A layer absent from the archive entirely gets Size == 0 from a missing-key map lookup, so the check never fired and parseLayers returned nil error. Use the headers map from Pass 1 instead: every entry that appeared in the outer tar has a key there. A manifest-referenced path with no key in headers was never in the archive, regardless of its declared size.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three independent correctness bugs in the layer analysis pipeline, plus follow-up fixes from code review:
Missing layer blob detection
parseLayerspreviously treated a manifest-referenced layer whose blob was absent from the outer tar as an empty tree. Any CI efficiency rule would pass on the incomplete input. The check now returns an error with the layer index and ID.Implicit parent directory metadata preservation
When a layer adds a file without including an explicit directory header for its parent,
insertNodecreates a placeholder directory node with structural defaults (mode 0755, uid/gid 0). That placeholder was reachingmergeLayerWithand overwriting the real mode/UID/GID recorded by an earlier layer. Placeholder nodes are now markedInferred; the merge skips metadata propagation from them.Hardlink alias waste accounting
If a file's original name was deleted by a whiteout but a hardlink alias pointing at the same content survived in the final image, the original's bytes were charged as waste. The payload remained reachable through the alias.
computeEfficiencynow checks the final stacked snapshot for surviving hardlink targets and skips waste charging when the content is still live.Follow-up (code review)
hdr.Linknamenow receives the samecleanTarPathnormalization ashdr.NameinParseLayerTar, closing a latent gap for images from Windows-native build tools that emit./-prefixed hardlink targets.Inferredis propagated in all fiveFileNodeclone functions so the field's semantics hold on cloned/stacked nodes.computeEfficiency(O(1) index rationale; charge rules for deleted vs live runs; LayerCount accounting invariant) that were accidentally removed during the hardlink refactor.TestParseLayers_MissingLayerBlob_ReturnsErrornow uses a valid empty tar for the present layer so the test exercises the missing-blob guard specifically.Test plan
go build ./...andgo vet ./...cleanTestParseLayers_MissingLayerBlob_ReturnsError,TestStack_ImplicitParentDoesNotOverwriteMetadata,TestEfficiency_HardlinkAlias_DeletedOriginal_NotWastedgo test ./...suite runs on CI