Repository navigation
build: remove the partial layer file when ImageLayoutToLayer fails - #2586
RaphaelFakhri wants to merge 2 commits into
Conversation
ImageLayoutToLayer left its output file behind on every error path, and TarballPath kept pointing at it. The CLI wraps builds in a temp dir it removes, but library callers that supply TempDirPath, or let TempDir() create one, keep a partial layer from every failed attempt. Remove the file and clear TarballPath on failure when apko named the file itself. A path set through WithTarball belongs to the caller and is left in place. This matches the cleanup splitLayers and splitErofsLayers already do. Fixes chainguard-dev#2483
7283324 to
9fb3b6b
Compare
| if bc.o.TarballPath != "" { | ||
| // A path set through WithTarball belongs to the caller; only a file apko | ||
| // named itself is removed on failure. | ||
| owned := bc.o.TarballPath == "" |
There was a problem hiding this comment.
Non-blocking: owned is inferred from TarballPath being empty on entry, but this function sets bc.o.TarballPath itself on success. So if a Context is reused after a successful call, the second call treats apko's own temp file as caller-owned and won't clean it up on failure.
Nothing in-tree reuses a Context, and resetting the path to "" on failure means a retry after a failed call still behaves, so this is fine as is. Just noting that the ownership signal depends on that side effect. Tracking it explicitly, e.g. a field set only by WithTarball, would remove the dependency if it ever matters.
There was a problem hiding this comment.
Both notes are addressed in 37ea80d:
ImageLayoutToLayernow records the file it creates itself in a newownedTarballfield onContext, and treats that path as owned on later calls. AContextreused after a successful call now removes its own file when a later call fails.TestImageLayoutToLayerReuseFailureCleanupcovers this for tar and EROFS, and fails without the change.- The
WithTarballdoc comment now says the file belongs to the caller and may hold a partial layer after a failed build.TestImageLayoutToLayerFailureKeepsCallerTarballalso checks thatTarballPath()still returns the caller's path.
|
|
||
| // A file the caller named through WithTarball is the caller's, so a failed | ||
| // build leaves it in place. | ||
| func TestImageLayoutToLayerFailureKeepsCallerTarball(t *testing.T) { |
There was a problem hiding this comment.
Non-blocking: keeping a WithTarball file after a failure seems like the right call (don't delete what you didn't create), but what's left is a truncated layer. Might be worth a line in the WithTarball doc comment saying the file may be partial on error. This test could also assert what TarballPath() returns in this case, since that is part of the contract too.
Ownership of the output file was inferred from TarballPath being empty on entry, but a successful call sets TarballPath to apko's own temp file. A Context reused after a success then treated that file as caller-owned and left it behind when a later call failed. Record the file ImageLayoutToLayer creates itself and treat it as owned on later calls. Document that a WithTarball file may hold a partial layer after a failed build, and assert TarballPath in the caller-tarball test.
Fixes #2483
ImageLayoutToLayerleaves its output file behind on every error path, andTarballPath()keeps returning that path. The CLI hides this because it removes its temp dir, but a library caller that setsTempDirPath, or letsOptions.TempDir()create one, keeps a partial layer from every failed build. WithWithCompressedLayerFile(true)the leftover is a well-formed gzip of a truncated tar.This change removes the file and clears
TarballPathon failure, for both the tar and EROFS paths, but only when apko named the file. A path set throughWithTarballbelongs to the caller and stays in place.splitLayersandsplitErofsLayersalready clean up this way.Tests
TestImageLayoutToLayerFailureCleanupinjects an open failure for one file (reusingfailOpenFS) and checks, fortaranderofs, that the temp dir is empty andTarballPath()is cleared. Both subtests fail onmain(partial layer files left behind: [apko.tar.gz],[apko.erofs]) and pass with the change.TestImageLayoutToLayerFailureKeepsCallerTarballchecks that aWithTarballpath is kept.go test ./pkg/build/... ./internal/cli/...andgolangci-lint run ./pkg/build/...(v2.14) pass.