From 90870bd4a5092d0445c516e23fff0581f33d317d Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Tue, 8 Sep 2026 15:59:27 -0600 Subject: [PATCH] fix: DiskCache served a cache miss for entries that were still fresh read(_:forKey:notOlderThan:) compared the entry's expiry against now and returned nil when the expiry was still in the future, so it discarded every entry inside the requested window and served only entries that had already aged past it. Also reads the creation date off the URL instead of a percent-encoded path string, matching the fileExists(at:) check two lines above, and takes the cache root as an init parameter so the tests don't touch the real caches directory. --- Modules/Sources/WordPressCore/DiskCache.swift | 16 ++- .../WordPressCoreTests/DiskCacheTests.swift | 101 ++++++++++++++++++ 2 files changed, 112 insertions(+), 5 deletions(-) create mode 100644 Modules/Tests/WordPressCoreTests/DiskCacheTests.swift diff --git a/Modules/Sources/WordPressCore/DiskCache.swift b/Modules/Sources/WordPressCore/DiskCache.swift index 37b7268fc02d..82acfa0b2357 100644 --- a/Modules/Sources/WordPressCore/DiskCache.swift +++ b/Modules/Sources/WordPressCore/DiskCache.swift @@ -7,9 +7,11 @@ public actor DiskCache: DiskCacheProtocol { public static let shared = DiskCache() - private let cacheRoot = URL.cachesDirectory + private let cacheRoot: URL - public init() {} + public init(cacheRoot: URL = .cachesDirectory) { + self.cacheRoot = cacheRoot + } public func read( _ type: T.Type, @@ -23,15 +25,19 @@ public actor DiskCache: DiskCacheProtocol { } if let interval { - let attributes = try FileManager.default.attributesOfItem(atPath: path.path()) + // Read the date off the URL rather than a path string: `path()` percent-encodes, so a + // key needing encoding would name a file that doesn't exist. + let creationDate = try path.resourceValues(forKeys: [.creationDateKey]).creationDate // If we can't find the creation date, assume the cache object is invalid because we can't guarantee // the developer's intent will be respected. - guard let creationDate = attributes[.creationDate] as? Date else { + guard let creationDate else { return nil } - if creationDate.addingTimeInterval(interval) > Date.now { + // The entry expires once `interval` has elapsed since it was written, so treat it as a + // miss only once that moment has passed. + if creationDate.addingTimeInterval(interval) < Date.now { return nil } } diff --git a/Modules/Tests/WordPressCoreTests/DiskCacheTests.swift b/Modules/Tests/WordPressCoreTests/DiskCacheTests.swift new file mode 100644 index 000000000000..6ed4d4e646fe --- /dev/null +++ b/Modules/Tests/WordPressCoreTests/DiskCacheTests.swift @@ -0,0 +1,101 @@ +import Foundation +import Testing + +@testable import WordPressCore + +struct DiskCacheTests { + + /// Each test gets its own root so a run never reads, writes, or deletes anything in the real + /// caches directory. + private func makeCache() throws -> (DiskCache, URL) { + let root = URL.temporaryDirectory.appending(path: "DiskCacheTests-\(UUID().uuidString)") + try FileManager.default.createDirectory(at: root, withIntermediateDirectories: true) + return (DiskCache(cacheRoot: root), root) + } + + @Test func storesAndReadsBackAValue() async throws { + let (cache, root) = try makeCache() + defer { try? FileManager.default.removeItem(at: root) } + + try await cache.store(["a", "b"], forKey: "key") + + #expect(try await cache.read([String].self, forKey: "key") == ["a", "b"]) + } + + /// A freshly-written entry is inside any sane window, so it has to come back. Before the fix + /// the comparison was inverted and this returned `nil`. + @Test func readsBackAnEntryThatIsStillFresh() async throws { + let (cache, root) = try makeCache() + defer { try? FileManager.default.removeItem(at: root) } + + try await cache.store("value", forKey: "key") + + #expect(try await cache.read(String.self, forKey: "key", notOlderThan: 3600) == "value") + } + + /// The counterpart: a zero-length window expires the entry immediately. + @Test func treatsAnExpiredEntryAsAMiss() async throws { + let (cache, root) = try makeCache() + defer { try? FileManager.default.removeItem(at: root) } + + try await cache.store("value", forKey: "key") + // The entry was written in the past, however narrowly, so any elapsed window excludes it. + try await Task.sleep(for: .milliseconds(50)) + + #expect(try await cache.read(String.self, forKey: "key", notOlderThan: 0) == nil) + } + + /// Passing no interval skips the age check entirely. + @Test func readsBackAnEntryWhenNoIntervalIsGiven() async throws { + let (cache, root) = try makeCache() + defer { try? FileManager.default.removeItem(at: root) } + + try await cache.store("value", forKey: "key") + + #expect(try await cache.read(String.self, forKey: "key") == "value") + } + + @Test func returnsNilForAKeyThatWasNeverStored() async throws { + let (cache, root) = try makeCache() + defer { try? FileManager.default.removeItem(at: root) } + + #expect(try await cache.read(String.self, forKey: "missing") == nil) + #expect(try await cache.read(String.self, forKey: "missing", notOlderThan: 3600) == nil) + } + + /// The age check used to resolve the file through a percent-encoded path string, which named a + /// file that doesn't exist whenever the key needed encoding. + @Test func handlesAKeyThatNeedsPercentEncoding() async throws { + let (cache, root) = try makeCache() + defer { try? FileManager.default.removeItem(at: root) } + + try await cache.store("value", forKey: "conversation 1 of 100%") + + #expect(try await cache.read(String.self, forKey: "conversation 1 of 100%", notOlderThan: 3600) == "value") + } + + @Test func removesAStoredEntry() async throws { + let (cache, root) = try makeCache() + defer { try? FileManager.default.removeItem(at: root) } + + try await cache.store("value", forKey: "key") + try await cache.remove(key: "key") + + #expect(try await cache.read(String.self, forKey: "key") == nil) + } + + /// `removeAll` only touches the injected root, which is what keeps these tests from clearing a + /// developer's real cache. + @Test func removeAllClearsOnlyTheInjectedRoot() async throws { + let (cache, root) = try makeCache() + defer { try? FileManager.default.removeItem(at: root) } + + try await cache.store("a", forKey: "one") + try await cache.store("b", forKey: "two") + #expect(try await cache.count() == 2) + + try await cache.removeAll() + + #expect(try await cache.count() == 0) + } +}