Fix: cache not cleaned up if download fails #7663
Conversation
If the download of a binaryTarget fails a cache file still remains in the artifact cache. The file only contains the server's response which is usually the status code and the error message. All following resolve calls therefore fail because the cached file is used but is an invalid archive
pwallrich
requested review from
bnbarham,
MaxDesiatov,
jakepetroules,
francescomikulis and
xedin
as code owners
pwallrich
changed the title
| @@ -7471,6 +7471,7 @@ final class WorkspaceTests: XCTestCase { | |||
| // make sure artifact downloaded is deleted | |||
| XCTAssertTrue(fs.isDirectory(AbsolutePath("/tmp/ws/.build/artifacts/root"))) | |||
| XCTAssertFalse(fs.exists(AbsolutePath("/tmp/ws/.build/artifacts/root/a.zip"))) | |||
| XCTAssertFalse(fs.exists(AbsolutePath("/home/user/caches/org.swift.swiftpm/artifacts/https___a_com_a_zip"))) | |||
There was a problem hiding this comment.
Is this the actual path where caches were previously stored? Can we compute it somehow from previously defined paths instead of hardcoding here?
There was a problem hiding this comment.
My concern here is that if cache storage somehow changes, this test won't detect that and will continue passing.
There was a problem hiding this comment.
I've updated the test to get the url from the location of the workspace which is also used when initialising the BinaryArtifactsManager in the production code.
The cacheKey is now created the same way as in the BinaryArtifactsManager.
The test could still lead to false positives, if the underlying logic is changed. But I guess to prevent this the BinaryArtifactsManager would need to be refactored which I don't feel comfortable doing at the moment ^^.
|
@swift-ci test |
|
Definitely worth cherry-picking this one for 6.0 👍 |
|
Cherry-picked as #7671 |
Cherry-pick of #7663. **Explanation**: If a server returns an error when trying to download a binaryTarget an invalid file remains in the artifacts cache. **Scope**: Isolated to binary targets cache. **Risk**: Low, scope is isolated, and the change has a corresponding test. **Testing**: Automated with an existing test case refined. **Issue**: rdar://123897276 **Reviewer**: @MaxDesiatov --------- Co-authored-by: Philipp Wallrich <p.wallrich@gmail.com>
If a server returns an error when trying to download a binaryTarget an invalid file remains in the artifacts cache.
Motivation:
If the download of a binaryTarget fails a cache file still remains in the artifact cache.
The file only contains the server's response which is usually the status code and the error message.
Example:
Worse, all following
resolvecalls therefore fail because the cached file is used but is an invalid archive.Modifications:
If the download fails the cache file in the artifacts cache is deleted.
Result:
As expected, the artifacts folder is empty.
Example: