diff --git a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift index 3969158..506e223 100644 --- a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift +++ b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift @@ -192,8 +192,34 @@ private struct BrowserStackCLIDownloader { return cacheRoot } + /// Best-effort removal of stale staging artifacts (`.tmp.*` files and directories) left + /// behind when a previous extraction was interrupted. The extract helpers call + /// forwardExit()/exit() on failure and SIGKILL can hit at any point, both of which + /// bypass the `defer` cleanup in prepareArtifact. Only entries older than one hour are + /// removed, so a concurrent build's in-flight staging directory is never deleted + /// mid-extraction. + private func sweepStaleStaging(in cacheRoot: URL) { + let staleStagingAge: TimeInterval = 3600 + let now = Date() + guard let entries = try? fileManager.contentsOfDirectory( + at: cacheRoot, + includingPropertiesForKeys: [.contentModificationDateKey], + options: [] + ) else { + return + } + for entry in entries where entry.lastPathComponent.hasPrefix(".tmp.") { + let modified = (try? entry.resourceValues(forKeys: [.contentModificationDateKey]))?.contentModificationDate + guard let modified, now.timeIntervalSince(modified) > staleStagingAge else { + continue + } + try? fileManager.removeItem(at: entry) + } + } + private func prepareArtifact(using info: ArtifactInfo) async throws -> BrowserStackCLIArtifact { let cacheRoot = try ensureCacheRootExists() + sweepStaleStaging(in: cacheRoot) let versionDirectory = cacheRoot.appendingPathComponent(info.version, isDirectory: true) let executableName = info.executableName let expectedExecutableURL = versionDirectory.appendingPathComponent(executableName, isDirectory: false) @@ -202,37 +228,104 @@ private struct BrowserStackCLIDownloader { return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL) } - if fileManager.fileExists(atPath: versionDirectory.path) { - try fileManager.removeItem(at: versionDirectory) - } - try fileManager.createDirectory(at: versionDirectory, withIntermediateDirectories: true) + // Extract into a unique staging directory and atomically publish it to the final + // version directory (DEVA11Y-482). The previous check-delete-recreate sequence was + // a TOCTOU: two concurrent builds sharing ~/.cache could both fall through the + // isExecutableFile check, then one instance's removeItem/createDirectory would wipe + // the other's in-progress extraction, corrupting the binary or leaving a partial + // file that locateExecutable's fallback would happily run. Staging + rename means a + // version directory only ever becomes visible fully-formed, and a loser of the + // publish race reuses the winner's binary instead of clobbering it. + let stagingDirectory = cacheRoot.appendingPathComponent( + ".tmp.\(info.version).\(UUID().uuidString)", + isDirectory: true + ) + defer { try? fileManager.removeItem(at: stagingDirectory) } + try fileManager.createDirectory(at: stagingDirectory, withIntermediateDirectories: true) Diagnostics.remark("BrowserStackAccessibilityLint: Downloading CLI \(info.version)...") #if os(Windows) - let archiveURL = versionDirectory.appendingPathComponent("browserstack-cli.zip") + // Download the archive to a sibling temp file *outside* the staging directory so a + // failed cleanup (e.g. an AV scanner or indexer holding a handle on Windows) can + // never bake the .zip into the published version directory. A leftover is a `.tmp.*` + // sibling that sweepStaleStaging reclaims later. + let archiveURL = cacheRoot.appendingPathComponent(".tmp.\(info.version).\(UUID().uuidString).zip") + defer { try? fileManager.removeItem(at: archiveURL) } try await download(from: info.resolvedURL, to: archiveURL) Diagnostics.remark("BrowserStackAccessibilityLint: Extracting CLI \(info.version)...") - try unzip(archive: archiveURL, into: versionDirectory) - try? fileManager.removeItem(at: archiveURL) + try unzip(archive: archiveURL, into: stagingDirectory) #else - try extractWithBsdtar(from: info.resolvedURL, into: versionDirectory) + try extractWithBsdtar(from: info.resolvedURL, into: stagingDirectory) #endif - let locatedBinary = try locateExecutable(in: versionDirectory, preferredName: executableName) - let finalBinaryURL: URL - if locatedBinary.lastPathComponent == executableName { - finalBinaryURL = locatedBinary - } else { - finalBinaryURL = expectedExecutableURL - if fileManager.fileExists(atPath: finalBinaryURL.path) { - try fileManager.removeItem(at: finalBinaryURL) + // Normalise the binary to the expected name *inside* the staging directory so the + // published version directory is always structurally complete before it is renamed. + // Compare full paths, not just the last component: locateExecutable recurses, so a + // binary that already has the right name can still sit in a nested subdirectory + // (e.g. a versioned tarball folder). Relocating it to the top-level staged path + // unless it is already exactly there guarantees stagedExecutableURL exists before + // we set permissions and publish. + let locatedBinary = try locateExecutable(in: stagingDirectory, preferredName: executableName) + let stagedExecutableURL = stagingDirectory.appendingPathComponent(executableName, isDirectory: false) + if locatedBinary.standardizedFileURL != stagedExecutableURL.standardizedFileURL { + if fileManager.fileExists(atPath: stagedExecutableURL.path) { + try fileManager.removeItem(at: stagedExecutableURL) } - try fileManager.moveItem(at: locatedBinary, to: finalBinaryURL) + try fileManager.moveItem(at: locatedBinary, to: stagedExecutableURL) + } + try ensureExecutablePermissions(at: stagedExecutableURL) + + try publishVersionDirectory(from: stagingDirectory, to: versionDirectory, expectedExecutableURL: expectedExecutableURL) + return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL) + } + + /// Publishes a fully-prepared staging directory to its final version directory. + /// + /// Correctness does not depend on `moveItem`'s throw-on-existing-destination behaviour, + /// which differs across Foundation platforms (Darwin throws `fileWriteFileExists`; a + /// bare POSIX `rename(2)` silently replaces an empty destination). We check for the + /// destination explicitly: when it is absent the publish is a single atomic rename on + /// the shared cache filesystem (staging and version dir are both children of cacheRoot), + /// so concurrent builds never observe a half-formed version directory; when it is + /// present — another build won the race, or `forceDownload` is refreshing a stale copy — + /// a valid published binary is reused, otherwise the stale directory is replaced and the + /// rename retried once. The replace path is deliberate last-writer-wins and is *not* + /// atomic; it tolerates a peer removing or republishing the directory concurrently + /// rather than failing the build. + private func publishVersionDirectory(from stagingDirectory: URL, to versionDirectory: URL, expectedExecutableURL: URL) throws { + // Fast path: destination absent -> single atomic rename. A create race that briefly + // loses (destination appears between the check and the move) falls through to the + // shared "destination present" handling below rather than failing. + if !fileManager.fileExists(atPath: versionDirectory.path) { + do { + try fileManager.moveItem(at: stagingDirectory, to: versionDirectory) + return + } catch { + // Fall through. + } + } + + // Destination present: reuse a valid binary unless a forced refresh was requested. + if !forceDownload, fileManager.isExecutableFile(atPath: expectedExecutableURL.path) { + return } - try ensureExecutablePermissions(at: finalBinaryURL) - return BrowserStackCLIArtifact(version: info.version, executableURL: finalBinaryURL) + // Stale/incomplete destination, or a forced refresh: replace and retry once. + // removeItem is best-effort so a peer deleting the directory first cannot turn into + // an ENOENT crash mid-race. + try? fileManager.removeItem(at: versionDirectory) + do { + try fileManager.moveItem(at: stagingDirectory, to: versionDirectory) + } catch { + // A peer republished the version directory between our remove and move. If it + // now holds a valid binary, treat that as success rather than failing a build + // that already has the artifact it needs. + if fileManager.isExecutableFile(atPath: expectedExecutableURL.path) { + return + } + throw error + } } #if !os(Windows)