gracefuly handle pre-existing binary artifact from failed downloads (#4021)

motivation: when binary artifact download fails it may leave behind a partial or otherwise corrupt file

changes:
* remove pre-existing archive file before attempting to download the same file again
* add test

Author: tomerd

Sources/Workspace/Workspace.swift

@@ -2281,6 +2281,11 @@ extension Workspace {
             }
 
             let archivePath = parentDirectory.appending(component: artifact.url.lastPathComponent)
+            if self.fileSystem.exists(archivePath) {
+                guard observabilityScope.trap ({ try self.fileSystem.removeFileTree(archivePath) }) else {
+                    continue
+                }
+            }
 
             semaphore.wait()
             group.enter()

Tests/WorkspaceTests/WorkspaceTests.swift

@@ -6502,6 +6502,131 @@ final class WorkspaceTests: XCTestCase {
         }
     }
 
+    func testArtifactDownloadArchiveExists() throws {
+        let sandbox = AbsolutePath("/tmp/ws/")
+        let fs = InMemoryFileSystem()
+        // this relies on internal knowledge of the destination path construction
+        let expectedDownloadDestination = sandbox.appending(components: ".build", "artifacts", "library", "binary.zip")
+
+        // returns a dummy zipfile for the requested artifact
+        let httpClient = HTTPClient(handler: { request, _, completion in
+            do {
+                guard case .download(let fileSystem, let destination) = request.kind else {
+                    throw StringError("invalid request \(request.kind)")
+                }
+
+                // this is to test the test's integrity, as it relied on internal knowledge of the destination path construction
+                guard expectedDownloadDestination == destination else {
+                    throw StringError("expected destination of \(expectedDownloadDestination)")
+                }
+
+                let contents: [UInt8]
+                switch request.url.lastPathComponent {
+                    case "binary.zip":
+                        contents = [0x01]
+                    default:
+                        throw StringError("unexpected url \(request.url)")
+                }
+
+                // in-memory fs does not check for this!
+                if fileSystem.exists(destination) {
+                    throw StringError("\(destination) already exists")
+                }
+
+                try fileSystem.writeFileContents(
+                    destination,
+                    bytes: ByteString(contents),
+                    atomically: true
+                )
+
+                completion(.success(.okay()))
+            } catch {
+                completion(.failure(error))
+            }
+        })
+
+        // create a dummy xcframework directory from the request archive
+        let archiver = MockArchiver(handler: { archiver, archivePath, destinationPath, completion in
+            do {
+                let name: String
+                switch archivePath.basename {
+                case "binary.zip":
+                    name = "binary.xcframework"
+                default:
+                    throw StringError("unexpected archivePath \(archivePath)")
+                }
+                try fs.createDirectory(destinationPath.appending(component: name), recursive: false)
+                archiver.extractions.append(MockArchiver.Extraction(archivePath: archivePath, destinationPath: destinationPath))
+                completion(.success(()))
+            } catch {
+                completion(.failure(error))
+            }
+        })
+
+        let workspace = try MockWorkspace(
+            sandbox: sandbox,
+            fileSystem: fs,
+            roots: [
+                MockPackage(
+                    name: "App",
+                    targets: [
+                        MockTarget(name: "App", dependencies: [
+                            .product(name: "binary", package: "library"),
+                        ]),
+                    ],
+                    products: [],
+                    dependencies: [
+                        .sourceControl(path: "./library", requirement: .exact("1.0.0")),
+                    ]
+                ),
+            ],
+            packages: [
+                MockPackage(
+                    name: "library",
+                    targets: [
+                        MockTarget(
+                            name: "binary",
+                            type: .binary,
+                            url: "https://a.com/binary.zip",
+                            checksum: "01"
+                        )
+                    ],
+                    products: [
+                        MockProduct(name: "binary", targets: ["binary"]),
+                    ],
+                    versions: ["1.0.0"]
+                )
+            ],
+            customHttpClient: httpClient,
+            customBinaryArchiver: archiver
+        )
+
+        // write the file to test it gets deleted
+
+        try fs.createDirectory(expectedDownloadDestination.parentDirectory, recursive: true)
+        try fs.writeFileContents(
+            expectedDownloadDestination,
+            bytes: [],
+            atomically: true
+        )
+
+        try workspace.checkPackageGraph(roots: ["App"]) { graph, diagnostics in
+            XCTAssertNoDiagnostics(diagnostics)
+        }
+
+        workspace.checkManagedArtifacts { result in
+            result.check(
+                packageIdentity: .plain("library"),
+                targetName: "binary",
+                source: .remote(
+                    url: "https://a.com/binary.zip",
+                    checksum: "01"
+                ),
+                path: workspace.artifactsDir.appending(components: "library", "binary.xcframework")
+            )
+        }
+    }
+
     func testDownloadArchiveIndexFilesHappyPath() throws {
         let sandbox = AbsolutePath("/tmp/ws/")
         let fs = InMemoryFileSystem()