[6.2] Cherry Pick - Fix inverted logic on registry cache expiration #9144 (#9210)

Cherry pick of #9144

In `RegistryClient.swift` there is an in-memory package metadata cache
expiration check
```swift
        let cacheKey = MetadataCacheKey(registry: registry, package: package)
        if let cached = self.metadataCache[cacheKey], cached.expires < .now() {
            return cached.metadata
        }
```

The logic for `cached.expires < .now()` appears to be inverted, e.g. if
a cache is found for the cacheKey, the initial values look like this
`2389992062152` < `2303592072235` which equates to false, because
[`cached.expires`](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/RegistryClient.swift#L443)
is set with a TTL of [60*60
seconds](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/RegistryClient.swift#L38)
in the future. This leads to the cache only being used _after_ it has
expired and may be causing downstream fingerprint TOFU issues when new
package releases are published and the metadata cache is in-memory.

### Motivation:

This issue was identified via the use of a private SPM registry
conforming to the [registry server
specification](https://docs.swift.org/swiftpm/documentation/packagemanagerdocs/registryserverspecification/).
Disclaimer: This issue was also occurring in previous versions of Xcode
but now that they are [sharing code with
SPM](https://developer.apple.com/documentation/xcode-release-notes/xcode-26-release-notes#Swift-Package-Manager),
I hope this example is still relevant. The scenario it is occurring in
is as follows:
1. Have Xcode open for 1 hour or more after resolving dependencies at
least once to populate the cache.
2. Release a new package version to the registry
3. Attempt to resolve the new version in Xcode
4. At this stage, my belief is that the previous release's metadata
(including the checksum) from the "expired" cache is stored to disk as
the fingerprint associated with the new release for TOFU purposes.
   - Code path: 
- -> [Get expected checksum for new
version](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/ChecksumTOFU.swift#L49)
- -> [Get metadata for
version](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/ChecksumTOFU.swift#L111-L122)
- -> [Check if we should hit the server or use
cache](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/RegistryClient.swift#L290-L310)
- -> [Use cached metadata if it's been in memory for 1
hour](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/RegistryClient.swift#L400-L403)
- -> [Write cached (old) checksum to fingerprint storage for new
version](https://github.com/swiftlang/swift-package-manager/blob/18bc4801d1e0aa02ca69633c3c12d5d1135b65bc/Sources/PackageRegistry/ChecksumTOFU.swift#L131-L138)
6. Attempting even `swift package resolve` via CLI at this point results
the following error:
- `invalid registry source archive checksum 'newchecksum', expected
'previouschecksum'`

Because the checksum fingerprint is stored on disk in
`~/Library/org.swift.swiftpm/security/fingerprints/`, it takes manual
intervention to recover from this by purging various caches.
Example of a common workaround:
https://stackoverflow.com/questions/79417322/invalid-registry-source-archive-checksum-a-expected-b
Related github issue:
#8981

In addition to that, there are probably excessive requests being sent to
various registries due to the cache not being used during the initial
TTL.

### Modifications:

Two main changes in this PR that should help with this:
1. Adding `version` property to the `MetadataCacheKey`, so that even if
the previous metadata is in cache and hasn't expired, it won't be
applied to the new version.
2. Reversing the logic for checking expiration, so that it will
appropriately fetch new data upon expiration, rather than only fetching
new data until expiration.

In addition to that, this PR also:
- Fixes a similar cache expiration issue for the `availabilityCache`
- Adds tests for both caches
- Updates `withAvailabilityCheck` to internal (from private) for
testability.

### Result:

The hopeful result of this is that package registry consumers will no
longer require manual workarounds to fix their local state when they hit
this edge case, as well as avoid hitting github and other package
registries [too
often](https://x.com/krzyzanowskim/status/1967333807318777900) (my guess
for the original intent of the cache).

That said, it will be a bit tricky to validate in its entirety because
this cache is only relevant for long-running tasks via libSwiftPM I
presume.

One big thing to note is that this may be a noticeable change to
existing behavior since the cache is not currently being used as far as
I can tell, and once fixed, it will significantly reduce the freshness
of the metadata compared to before.

Co-authored-by: Zach Nagengast <[email protected]>

Author: plemarquand

Sources/PackageRegistry/RegistryClient.swift

@@ -397,8 +397,8 @@ public final class RegistryClient: AsyncCancellable {
         timeout: DispatchTimeInterval?,
         observabilityScope: ObservabilityScope
     ) async throws -> Serialization.VersionMetadata {
-        let cacheKey = MetadataCacheKey(registry: registry, package: package)
-        if let cached = self.metadataCache[cacheKey], cached.expires < .now() {
+        let cacheKey = MetadataCacheKey(registry: registry, package: package, version: version)
+        if let cached = self.metadataCache[cacheKey], cached.expires > .now() {
             return cached.metadata
         }
 
@@ -1403,21 +1403,9 @@ public final class RegistryClient: AsyncCancellable {
         }
     }
 
-    private func unwrapRegistry(from package: PackageIdentity) throws -> (PackageIdentity.RegistryIdentity, Registry) {
-        guard let registryIdentity = package.registry else {
-            throw RegistryError.invalidPackageIdentity(package)
-        }
-
-        guard let registry = self.configuration.registry(for: registryIdentity.scope) else {
-            throw RegistryError.registryNotConfigured(scope: registryIdentity.scope)
-        }
-
-        return (registryIdentity, registry)
-    }
-
     // If the registry is available, the function returns, otherwise an error
     // explaining why the registry is unavailable is thrown.
-    private func withAvailabilityCheck(
+    func withAvailabilityCheck(
         registry: Registry,
         observabilityScope: ObservabilityScope
     ) async throws {
@@ -1438,7 +1426,7 @@ public final class RegistryClient: AsyncCancellable {
             }
         }
 
-        if let cached = self.availabilityCache[registry.url], cached.expires < .now() {
+        if let cached = self.availabilityCache[registry.url], cached.expires > .now() {
             return try availabilityHandler(cached.status)
         }
 
@@ -1453,6 +1441,18 @@ public final class RegistryClient: AsyncCancellable {
         return try availabilityHandler(result)
     }
 
+    private func unwrapRegistry(from package: PackageIdentity) throws -> (PackageIdentity.RegistryIdentity, Registry) {
+        guard let registryIdentity = package.registry else {
+            throw RegistryError.invalidPackageIdentity(package)
+        }
+
+        guard let registry = self.configuration.registry(for: registryIdentity.scope) else {
+            throw RegistryError.registryNotConfigured(scope: registryIdentity.scope)
+        }
+
+        return (registryIdentity, registry)
+    }
+
     private func unexpectedStatusError(
         _ response: HTTPClientResponse,
         expectedStatus: [Int]
@@ -1495,6 +1495,7 @@ public final class RegistryClient: AsyncCancellable {
     private struct MetadataCacheKey: Hashable {
         let registry: Registry
         let package: PackageIdentity.RegistryIdentity
+        let version: Version
     }
 }
 

Tests/PackageRegistryTests/RegistryClientTests.swift

@@ -10,7 +10,7 @@
 //
 //===----------------------------------------------------------------------===//
 
-import Basics
+@testable import Basics
 import _Concurrency
 import Foundation
 import PackageFingerprint
@@ -440,6 +440,126 @@ fileprivate var availabilityURL = URL("\(registryURL)/availability")
         assert(metadataSync)
     }
 
+    @Test func getPackageVersionMetadataInCache() async throws {
+        let checksumAlgorithm: HashAlgorithm = MockHashAlgorithm()
+        let expectedChecksums: [Version: String] = [
+            Version("1.1.1"): "a2ac54cf25fbc1ad0028f03f0aa4b96833b83bb05a14e510892bb27dea4dc812",
+            Version("1.1.0"): checksumAlgorithm.hash(emptyZipFile).hexadecimalRepresentation
+        ]
+
+        let counter = SendableBox(0)
+        let handler: HTTPClient.Implementation = { request, _ in
+            await counter.increment()
+            switch (request.method, request.url) {
+            case (.get, releasesURL.appending(component: "1.1.1")):
+                let expectedChecksum = expectedChecksums[Version("1.1.1")]!
+                #expect(request.headers.get("Accept").first == "application/vnd.swift.registry.v1+json")
+
+                let data = """
+                {
+                    "id": "mona.LinkedList",
+                    "version": "1.1.1",
+                    "resources": [
+                        {
+                            "name": "source-archive",
+                            "type": "application/zip",
+                            "checksum": "\(expectedChecksum)"
+                        }
+                    ],
+                    "metadata": {
+                        "author": {
+                            "name": "J. Appleseed"
+                        },
+                        "licenseURL": "https://github.com/mona/LinkedList/license",
+                        "readmeURL": "https://github.com/mona/LinkedList/readme",
+                        "repositoryURLs": [
+                            "https://github.com/mona/LinkedList",
+                            "ssh://[email protected]:mona/LinkedList.git",
+                            "[email protected]:mona/LinkedList.git"
+                        ]
+                    }
+                }
+                """.data(using: .utf8)!
+
+                return .init(
+                    statusCode: 200,
+                    headers: .init([
+                        .init(name: "Content-Length", value: "\(data.count)"),
+                        .init(name: "Content-Type", value: "application/json"),
+                        .init(name: "Content-Version", value: "1"),
+                    ]),
+                    body: data
+                )
+            case (.get, releasesURL.appending(component: "1.1.0")):
+                let expectedChecksum = expectedChecksums[Version("1.1.0")]!
+                #expect(request.headers.get("Accept").first == "application/vnd.swift.registry.v1+json")
+
+                let data = """
+                {
+                    "id": "mona.LinkedList",
+                    "version": "1.1.0",
+                    "resources": [
+                        {
+                            "name": "source-archive",
+                            "type": "application/zip",
+                            "checksum": "\(expectedChecksum)",
+                        }
+                    ],
+                    "metadata": {
+                        "author": {
+                            "name": "J. Appleseed"
+                        },
+                        "licenseURL": "https://github.com/mona/LinkedList/license",
+                        "readmeURL": "https://github.com/mona/LinkedList/readme",
+                        "repositoryURLs": [
+                            "https://github.com/mona/LinkedList",
+                            "ssh://[email protected]:mona/LinkedList.git",
+                            "[email protected]:mona/LinkedList.git"
+                        ]
+                    }
+                }
+                """.data(using: .utf8)!
+
+                return .init(
+                    statusCode: 200,
+                    headers: .init([
+                        .init(name: "Content-Length", value: "\(data.count)"),
+                        .init(name: "Content-Type", value: "application/json"),
+                        .init(name: "Content-Version", value: "1"),
+                    ]),
+                    body: data
+                )
+            default:
+                throw StringError("method and url should match")
+            }
+        }
+
+        let httpClient = HTTPClient(implementation: handler)
+        var configuration = RegistryConfiguration()
+        configuration.defaultRegistry = Registry(url: registryURL, supportsAvailability: false)
+
+        let registryClient = makeRegistryClient(configuration: configuration, httpClient: httpClient)
+
+        var expectedRequestCount = 0
+        try await check(version: Version("1.1.1"), expectCached: false)
+        try await check(version: Version("1.1.0"), expectCached: false)
+        try await check(version: Version("1.1.1"), expectCached: true)
+        try await check(version: Version("1.1.0"), expectCached: true)
+
+        func check(version: Version, expectCached: Bool) async throws {
+            let metadata = try await registryClient.getPackageVersionMetadata(package: identity, version: version)
+
+            if !expectCached {
+                expectedRequestCount += 1
+            }
+
+            let count = await counter.value
+            #expect(count == expectedRequestCount)
+            #expect(metadata.author?.name == "J. Appleseed")
+            #expect(metadata.resources[0].checksum == expectedChecksums[version]!)
+        }
+    }
+
     func getPackageVersionMetadata_404() async throws {
         let serverErrorHandler = ServerErrorHandler(
             method: .get,
@@ -3701,6 +3821,87 @@ fileprivate var availabilityURL = URL("\(registryURL)/availability")
             #expect(error as? StringError == StringError("registry \(registry.url) does not support availability checks."))
         }
     }
+
+    @Test func withAvailabilityCheck() async throws {
+        let handler: HTTPClient.Implementation = { request, _ in
+            switch (request.method, request.url) {
+            case (.get, availabilityURL):
+                return .okay()
+            default:
+                throw StringError("method and url should match")
+            }
+        }
+
+        let httpClient = HTTPClient(implementation: handler)
+        let registry = Registry(url: registryURL, supportsAvailability: true)
+
+        let registryClient = makeRegistryClient(
+            configuration: .init(),
+            httpClient: httpClient
+        )
+
+        try await registryClient.withAvailabilityCheck(
+            registry: registry,
+            observabilityScope: ObservabilitySystem.NOOP
+        )
+    }
+
+    @Test func withAvailabilityCheckServerError() async throws {
+        let handler: HTTPClient.Implementation = { request, _ in
+            switch (request.method, request.url) {
+            case (.get, availabilityURL):
+                return .serverError(reason: "boom")
+            default:
+                throw StringError("method and url should match")
+            }
+        }
+
+        let httpClient = HTTPClient(implementation: handler)
+        let registry = Registry(url: registryURL, supportsAvailability: true)
+
+        let registryClient = makeRegistryClient(
+            configuration: .init(),
+            httpClient: httpClient
+        )
+
+        await #expect(throws: StringError("unknown server error (500)")) {
+            try await registryClient.withAvailabilityCheck(
+                registry: registry,
+                observabilityScope: ObservabilitySystem.NOOP
+            )
+        }
+    }
+
+    @Test func withAvailabilityCheckInCache() async throws {
+        let counter = SendableBox(0)
+        let handler: HTTPClient.Implementation = { request, _ in
+            await counter.increment()
+            switch (request.method, request.url) {
+            case (.get, availabilityURL):
+                return .okay()
+            default:
+                throw StringError("method and url should match")
+            }
+        }
+
+        let httpClient = HTTPClient(implementation: handler)
+        let registry = Registry(url: registryURL, supportsAvailability: true)
+
+        let registryClient = makeRegistryClient(
+            configuration: .init(),
+            httpClient: httpClient
+        )
+
+        // Request count should not increase after first check
+        for _ in 0..<5 {
+            try await registryClient.withAvailabilityCheck(
+                registry: registry,
+                observabilityScope: ObservabilitySystem.NOOP
+            )
+            let count = await counter.value
+            #expect(count == 1)
+        }
+    }
 }
 
 // MARK: - Sugar