many: attempt to clean up potentially corrupted files during download undo#16691
many: attempt to clean up potentially corrupted files during download undo#16691alfonsosanchezbeato wants to merge 6 commits intocanonical:masterfrom
Conversation
Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
…oad undo Add proper undo to snap downloads step. Attempt to guess whether the downloaded file could be corrupted and if it is, try to clean it up. Related: SNAPDENG-36484 Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
alfonsosanchezbeato
requested review from
bboozzoo,
Copilot,
pedronis and
zyga
There was a problem hiding this comment.
Pull request overview
This PR adds an explicit undo handler for the download-snap task so that, when a download step is undone, snapd attempts to remove the downloaded snap file and (when possible) drop a potentially corrupted corresponding entry from the downloads cache.
Changes:
- Introduce
StoreService.CleanupDownloadArtifacts()and implement it in the store layer (including cacheOpen/Dropsupport) to remove the download target and optionally remove a corrupted cache entry. - Wire a new
undoDownloadSnaphandler for thedownload-snaptask in snapstate, calling the store cleanup method without failing the overall undo flow. - Add unit and spread tests to validate cleanup behavior for corrupted downloads and updated undo operation sequencing.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/main/snap-download-corrupted-cleanup/task.yaml | New spread test to validate that a corrupted snap blob and its cache entry are removed during undo of download-snap. |
| store/storetest/storetest.go | Updates the panicking test Store stub to satisfy the expanded StoreService interface. |
| store/store_download_test.go | Extends cache test doubles with Open/Drop and adds coverage for CleanupDownloadArtifacts. |
| store/store_download.go | Implements CleanupDownloadArtifacts in the store, including cache corruption detection and best-effort cleanup. |
| store/cache_test.go | Adds coverage for dropping cache entries and verifying hardlink count changes. |
| store/cache.go | Extends download cache interface and CacheManager with Drop and Open operations. |
| overlord/snapstate/snapstate_update_test.go | Adjusts expected fake ops and test download info to include sha3 where needed for cleanup. |
| overlord/snapstate/snapstate_install_test.go | Updates expected undo op sequences to include store cleanup of download artifacts. |
| overlord/snapstate/snapmgr.go | Switches download-snap undo handler from undoPrepareSnap to the new undoDownloadSnap. |
| overlord/snapstate/handlers_download_test.go | Adds unit tests ensuring undoDownloadSnap calls cleanup and treats cleanup failures as non-fatal. |
| overlord/snapstate/handlers.go | Adds undoDownloadSnap implementation calling StoreService.CleanupDownloadArtifacts. |
| overlord/snapstate/backend_test.go | Extends fake store/backend op tracking to include sha3 for cleanup calls. |
| overlord/snapstate/backend.go | Extends StoreService interface with CleanupDownloadArtifacts. |
store/store_download.go
Outdated
| return s.cacher.Cleanup() | ||
| } | ||
|
|
||
| // CleanupDownloadsCacheEntry attempts to clean up download artifacts associated |
There was a problem hiding this comment.
The doc comment refers to CleanupDownloadsCacheEntry, but the function is named CleanupDownloadArtifacts. Please update the comment to match so it’s discoverable via godoc and grep.
| // CleanupDownloadsCacheEntry attempts to clean up download artifacts associated | |
| // CleanupDownloadArtifacts attempts to clean up download artifacts associated |
store/store_download.go
Outdated
| var err error | ||
| // TODO:GOVERSION: use errors.Join | ||
| if rerr := os.Remove(targetFn); rerr != nil && !errors.Is(rerr, fs.ErrNotExist) { | ||
| err = strutil.JoinErrors(err, fmt.Errorf("cannot remove corrupted file: %w", rerr)) |
There was a problem hiding this comment.
This error message says "corrupted file", but this function unconditionally removes the download target during undo even when it may be a valid snap. Consider using a more accurate message (e.g. "downloaded file") to avoid misleading logs/error chains.
| err = strutil.JoinErrors(err, fmt.Errorf("cannot remove corrupted file: %w", rerr)) | |
| err = strutil.JoinErrors(err, fmt.Errorf("cannot remove downloaded file: %w", rerr)) |
| } | ||
|
|
||
| func (Store) CleanupDownloadArtifacts(targetFn string, dl *snap.DownloadInfo) error { | ||
| panic("CleaupnDownloadArtifacts not expected") |
There was a problem hiding this comment.
Typo in panic message: "CleaupnDownloadArtifacts" should be "CleanupDownloadArtifacts".
| panic("CleaupnDownloadArtifacts not expected") | |
| panic("CleanupDownloadArtifacts not expected") |
store/store_download_test.go
Outdated
| if co.inCache[cacheKey] { | ||
| s := "content" | ||
|
|
||
| // strings.NewReader returns an *io.Reader that also implements Seeker |
There was a problem hiding this comment.
The comment is incorrect: strings.NewReader returns a *strings.Reader (which implements io.Reader and io.Seeker), not an "*io.Reader".
| // strings.NewReader returns an *io.Reader that also implements Seeker | |
| // strings.NewReader returns a *strings.Reader, which implements io.Reader and io.Seeker |
| s.testUndoDownloadSnapFile(c, cleanupOk) | ||
| } | ||
|
|
||
| func (s *downloadSnapSuite) TestUndoDownloadSnapFileRemovedNotFatal(c *C) { |
There was a problem hiding this comment.
This test name suggests file removal is being exercised, but the scenario only injects a CleanupDownloadArtifacts error and asserts it’s non-fatal. Consider renaming to reflect the actual behavior under test (e.g. cleanup error is not fatal).
| func (s *downloadSnapSuite) TestUndoDownloadSnapFileRemovedNotFatal(c *C) { | |
| func (s *downloadSnapSuite) TestUndoDownloadSnapCleanupErrorNotFatal(c *C) { |
store/store_download.go
Outdated
|
|
||
| h := crypto.SHA3_384.New() | ||
| if _, cerr := io.Copy(h, f); cerr != nil { | ||
| return err |
There was a problem hiding this comment.
In the io.Copy error path, this returns the outer err variable (which may be nil) instead of returning/propagating cerr. This would silently ignore read/checksum failures and potentially skip dropping a corrupted cache entry.
| return err | |
| return fmt.Errorf("cannot checksum cached entry: %w", cerr) |
|
Sat Feb 28 05:19:20 UTC 2026 Failures:Executing:
Restoring:
Skipped tests from snapd-testing-skip
|
alfonsosanchezbeato
force-pushed
the
bboozzoo_downloads-undo-cleanup
branch
from
5292f0a to
9374699
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #16691 +/- ##
==========================================
- Coverage 77.54% 77.52% -0.02%
==========================================
Files 1360 1357 -3
Lines 187256 187328 +72
Branches 2446 2446
==========================================
+ Hits 145200 145230 +30
- Misses 33278 33309 +31
- Partials 8778 8789 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add proper undo to snap downloads step. Attempt to guess whether the
downloaded file could be corrupted and if it is, try to clean it up.
Related: SNAPDENG-36484
This is #16650 plus some test fixes. cc @bboozzoo .