Skip to content

Commit 7866c21

Browse files
authored
Cache correctness fixes + CacheStore extraction; drop dead code (#322)
* Drop dead AnyCodable.encode(to:) AnyCodable conforms only to Decodable, so encode(to:) satisfied no protocol and had no callers. It was also incomplete (threw on the nested AnyCodable collections that init(from:) produces). * Fix download orphan cache write and empty Data verb body - requestData wrote the payload under the path-derived key, then the download handlers wrote it again under the real cacheName. With a cacheName that left a stray file no read path uses; with none it was a redundant double write. Caching is now the handlers' job only; drop requestData's now-dead cachingLevel parameter. - A verb with T == Data returned an empty Data on success instead of the response body. Return the actual body (the Void overloads discard it, so they're unaffected). * Fix .memory cache read destroying the disk tier objectFromCache's .memory branch deleted the on-disk file on every read (present since the 2018 caching feature, #227). With the warm/cold tiering, that file is the durable cold copy of an earlier .memoryAndFile write — so a .memory read evicted it, and once the warm NSCache tier was dropped under pressure the entry was lost entirely, forcing a re-fetch. Make .memory a pure read of the warm tier; purging stays the write path's and clearCache's job. * Make .none cache read pure too — no purge on read objectFromCache's .none branch purged both tiers on read. The verb path already guards against calling it for .none, but the download path reads through it unconditionally, so a .none download wiped a disk entry an earlier .memoryAndFile write had created. Report a miss without touching either tier; clearCache is the way to evict. objectFromCache is now a pure read at every level. * Extract the disk-cache subsystem into CacheStore The two-tier cache (warm NSCache over the sharded on-disk layout), its TTL/expiry, sweep, and clear were scattered across static and instance members on the Networking actor. Move them into a dedicated CacheStore — a non-actor @unchecked Sendable keyed by a resolved resource string, so the layout/sharding/sweep logic is cohesive and independently testable. Networking keeps the public surface unchanged: destinationURL, objectFromCache, cacheOrPurgeData/Image are thin nonisolated shims that resolve the cache key (baseURL + path stays the networking layer's job, via cacheResource) and delegate. The injected NSCache is shared by reference so it stays inspectable. Behavior-preserving — full suite green. * Audit comments added in this PR Cut DI-narration on folderName, a duplicated baseURL-agnostic note on cacheResource, the regression-test file header (duplicated the per-test comments), and two test comments that restated the test name/assertion. Comment-only; full suite green. * Test CacheStore directly; decouple cache tests from the actor The cache-layout, sliding-TTL expiry, pure-read, and clear behaviors are the store's, but were tested through Networking's internal shims (objectFromCache/cacheOrPurgeData) — coupling the tests to actor internals. Move them to CacheStoreTests against CacheStore directly. Bonus: the .memory/.none pure-read contracts now run as deterministic unit tests with real NSCache eviction (cache.removeAllObjects), which the download-based versions couldn't do — and they need no go-httpbin. destinationURL/CacheExpiry tests stay on Networking (public API / own type). The internal shims keep their production callers (5/3/2 sites each + the path→resource resolution), so they remain — but no test depends on them now.
1 parent 2049253 commit 7866c21

9 files changed

Lines changed: 405 additions & 273 deletions

AGENTS.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@ Async HTTP client for Apple platforms. **iOS 18+ / Swift 6** (`swift-tools 6.2`,
1515
- **Verbs:** `get`/`post`/`put`/`patch`/`delete``Result<T, NetworkingError>`. Pick `T`: any `Decodable` model, `Data`, `Void`, or `JSONResponse` (`{ statusCode, headers, body }`) when you want metadata.
1616
- **Typed bodies — no `Any`.** The encoding is chosen by the method, not an untyped `parameters:`/`parameterType:` pair (`Networking+HTTPRequests.swift`): `post`/`put`/`patch` take `body:` (any `Encodable` → JSON, ISO-8601 dates), `form:` (any flat `Encodable` → url-encoded), `parts:`+`fields:` (multipart), or `data:contentType:` (raw); `get`/`delete` take `query:` (a `[URLQueryItem]` for ordering/dupes, or any flat `Encodable`). `form:`/`query:` flatten an `Encodable` to `[String: String]` via `formFields` in `Networking+FormEncoding.swift` (JSON bridge + scalar re-decode so `Bool`→"true", not NSNumber "1"). Bodies flow through the typed `RequestBody` enum in `Networking+New.swift`.
1717
- **Downloads:** `downloadImage`/`downloadData``Result<T, NetworkingError>` where `T` is the payload (`Image`/`Data`) or an envelope (`ImageResponse`/`DataResponse`).
18-
- **Cache lifecycle (`CacheExpiry.swift`, `Networking+Private.swift`):** a key→blob store — in-memory `NSCache` (the **warm** tier, pressure-evicted, no exposed limits) over on-disk files. Cleanup is instance-based and symmetric: `clearCache()` empties **both** tiers (the old disk-only static `deleteCachedFiles()` is gone — it left memory serving deleted data); `reset()` = `clearCache()` + wipe credentials/headers/fakes. On-disk entries carry a **sliding TTL** (`cacheTTL`, default 7 days) whose clock is the **file's modification date** — persisted, no in-memory map, no manifest. `objectFromCache` drops a disk entry whose mtime is older than the TTL (`CacheExpiry.isExpired(fileDate:)`); a disk hit (memory miss) re-warms by bumping the mtime — and since `NSCache` absorbs repeat reads, that touch is ~once per entry per launch, so no explicit debounce is needed. (Memory hits deliberately don't re-stamp the disk file, so an entry kept warm *only* in memory past `cacheTTL` can read as cold once evicted — accepted; normal use cycles through disk hits that re-warm it.) **Sharded from the start:** `destinationURL` lays files out under `domain/<shard>/<file>` where the shard is one hex nibble of the key hash (`shardName`, `shardCount` = 16); a detached `sweepExpiredCacheFiles` runs once per init and sweeps **one shard per launch** (rotated via a `.sweep-shard` cursor file), so per-launch work is O(N / shardCount) and a full pass takes `shardCount` launches (it also clears pre-sharding strays at the domain root). Sweep and `deleteCacheFolder` serialize on `cacheMutationLock` so the background sweep can't race a clear. `destinationURL` also hashes the filename component past the filesystem's 255-byte limit (readable prefix + SHA-256 suffix, `filesystemSafeComponent`).
18+
- **Cache lifecycle (`CacheStore.swift`, `CacheExpiry.swift`):** the whole two-tier cache subsystem lives in **`CacheStore`** — a non-actor `@unchecked Sendable` keyed by an already-resolved resource string (composing a key from baseURL + path stays the networking layer's job; `Networking.cacheResource(for:cacheName:)` does it and `destinationURL`/`objectFromCache`/`cacheOrPurgeData`/`cacheOrPurgeImage` are now thin nonisolated shims that delegate). The store is an in-memory `NSCache` (the **warm** tier, pressure-evicted by iOS, no exposed limits — assume it can be wiped at any time) over on-disk files; `Networking` holds the same `NSCache` by reference so it stays injectable/inspectable. **Reads are pure** (`CacheStore.object`): `.memory` serves the warm tier only and `.none` reports a miss — neither touches disk, so a read can't destroy a durable `.memoryAndFile` copy (purging is the write path's and `clearCache`'s job). Cleanup is symmetric: `clearCache()` → `cacheStore.clear()` empties **both** tiers (the old disk-only static `deleteCachedFiles()` is gone — it left memory serving deleted data); `reset()` = `clearCache()` + wipe credentials/headers/fakes. On-disk entries carry a **sliding TTL** (`cacheTTL`, default 7 days) whose clock is the **file's modification date** — persisted, no in-memory map, no manifest. The store drops a disk entry whose mtime is older than the TTL (`CacheExpiry.isExpired(fileDate:)`); a disk hit (memory miss) re-warms by bumping the mtime — and since `NSCache` absorbs repeat reads, that touch is ~once per entry per launch, so no explicit debounce is needed. (Memory hits deliberately don't re-stamp the disk file, so an entry kept warm *only* in memory past `cacheTTL` can read as cold once evicted — accepted; normal use cycles through disk hits that re-warm it.) **Sharded from the start:** `CacheStore.destinationURL(forResource:)` lays files out under `domain/<shard>/<file>` where the shard is one hex nibble of the key hash (`shardName`, `shardCount` = 16); a detached `CacheStore.sweepExpired` runs once per init and sweeps **one shard per launch** (rotated via a `.sweep-shard` cursor file), so per-launch work is O(N / shardCount) and a full pass takes `shardCount` launches (it also clears pre-sharding strays at the domain root). Sweep and `clear` serialize on `CacheStore.mutationLock` (a static, shared across instances) so the background sweep can't race a clear. `destinationURL` also hashes the filename component past the filesystem's 255-byte limit (readable prefix + SHA-256 suffix, `filesystemSafeComponent`).
1919
- **Fakes (testing):** `fakeGET`/`fakePOST`/… take `response:` over any `Encodable` (encoded to JSON), a no-body overload for status-only fakes, or `fileName:` for bundled raw `Data`; `FakeRequest` holds a typed `Payload` enum.
2020
- **Errors (`NetworkingError.swift`):** categorized by where the failure happened — `invalidRequest(InvalidRequestReason)`, `transport(URLError)`, `http(HTTPError)`, `decoding(DecodingError, ResponseMetadata)`, `validation(reason:ResponseMetadata)` (a 2xx rejected by a `ResponseValidatorInterceptor`), `invalidResponse`, `cancelled`. `HTTPError` carries `statusCode`/`isClientError`/`isServerError`/`metadata`; cross-cutting `statusCode`, `responseMetadata`, and conservative `isRetryable` (transient transport + 408/429/5xx). **The core makes no assumption about the error body's shape**`ResponseMetadata` retains the **full** `body: Data` (with `bodySnippet` a derived, truncated log excerpt and `decode(_:)` a convenience over `body`), so a caller decodes its own error envelope; there's no `serverMessage`/in-core parse. `ResponseMetadata(response:body:)` is the single source for metadata extraction. Construction lives in `Networking+New.swift` (`handleResponse`/`handleSuccessfulResponse`/`mapThrownError`).
2121
- **Observability (`NetworkingEvent.swift`):** no `print`, no closure observer. One consumer hook — `events()`, an `AsyncStream<NetworkingEvent>` (multi-consumer, `for await` accumulation without `Box`/`@unchecked`; continuations registry + `onTermination` cleanup on the actor). Every request emits `.started(RequestContext)` then `.completed(RequestContext, outcome:duration:metrics:)` — verbs via `handle`/`emitPreflightFailure`, downloads via `handleDataRequest`/`handleImageRequest` (all share `complete`/`makeContext` in `Networking+New.swift`; downloads inline the emit/complete rather than a closure, to satisfy Swift 6.2 region isolation). `RequestContext`: per-request id + the real request headers (raw — redaction is a log-path concern); `TransactionMetrics` distills `URLSessionTaskMetrics` (via `MetricsCollector`, a lock-synchronized per-task delegate).
Lines changed: 223 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,223 @@
1+
import Foundation
2+
import CryptoKit
3+
4+
// Owns both cache tiers: the in-memory `NSCache` (warm, pressure-evicted by iOS) over the on-disk shard
5+
// layout (cold, durable). A non-actor, synchronous type so the nonisolated cache reads
6+
// (`imageFromCache`/`dataFromCache`) stay synchronous; thread-safety comes from `NSCache` (thread-safe),
7+
// `CacheExpiry` (lock-synchronized), and a static lock serializing whole-folder mutations against the
8+
// background sweep. Keyed by an already-resolved resource string — composing a key from baseURL + path is
9+
// the networking layer's job, not the store's.
10+
final class CacheStore: @unchecked Sendable {
11+
let memory: NSCache<AnyObject, AnyObject>
12+
let expiry: CacheExpiry
13+
let folderName: String
14+
15+
init(memory: NSCache<AnyObject, AnyObject>, ttl: Duration, folderName: String) {
16+
self.memory = memory
17+
self.expiry = CacheExpiry(ttl: ttl)
18+
self.folderName = folderName
19+
}
20+
21+
var ttl: Duration { expiry.ttl }
22+
func setTTL(_ ttl: Duration) { expiry.setTTL(ttl) }
23+
24+
// MARK: - Layout
25+
26+
/// The on-disk URL for a resolved resource key, laid out under `folderName/<shard>/<file>`. Creates the
27+
/// shard directory if needed.
28+
func destinationURL(forResource resource: String) throws -> URL {
29+
let component = Self.filesystemSafeComponent(resource.replacingOccurrences(of: "/", with: "-"))
30+
// Shard the cache directory by a hash of the key so the per-launch sweep is O(N / shardCount), not
31+
// O(N). Always sharded — one uniform layout, no migration or size threshold (see `sweepExpired`).
32+
let folderPath = "\(folderName)/\(Self.shardName(for: component))"
33+
let finalPath = "\(folderPath)/\(component)"
34+
35+
guard let url = URL(string: finalPath),
36+
let cachesURL = FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask).first else {
37+
throw NSError(domain: folderName, code: 9999, userInfo: [NSLocalizedDescriptionKey: "Couldn't build a cache URL for: \(finalPath)"])
38+
}
39+
40+
let folderURL = cachesURL.appendingPathComponent(URL(string: folderPath)!.absoluteString)
41+
if FileManager.default.exists(at: folderURL) == false {
42+
try FileManager.default.createDirectory(at: folderURL, withIntermediateDirectories: true, attributes: nil)
43+
}
44+
return cachesURL.appendingPathComponent(url.absoluteString)
45+
}
46+
47+
static let shardCount = 16
48+
49+
// One hex-nibble shard derived from the key's hash; stable for a given key so reads and writes agree.
50+
static func shardName(for component: String) -> String {
51+
let byte = Array(SHA256.hash(data: Data(component.utf8))).first ?? 0
52+
return String(Int(byte) % shardCount, radix: 16)
53+
}
54+
55+
// A filesystem path component caps at 255 bytes, which a long URL would overflow. Keep a readable,
56+
// byte-bounded prefix and append a hash of the full name so distinct URLs still get distinct files.
57+
static func filesystemSafeComponent(_ name: String) -> String {
58+
let maxBytes = 255
59+
guard name.utf8.count > maxBytes else { return name }
60+
let hash = SHA256.hash(data: Data(name.utf8)).map { String(format: "%02x", $0) }.joined()
61+
let prefixBudget = maxBytes - hash.count - 1
62+
var prefix = ""
63+
var byteCount = 0
64+
for character in name {
65+
let width = String(character).utf8.count
66+
if byteCount + width > prefixBudget { break }
67+
prefix.append(character)
68+
byteCount += width
69+
}
70+
return "\(prefix)-\(hash)"
71+
}
72+
73+
// MARK: - Read
74+
75+
/// A pure read: serve from the warm tier, falling back to a non-expired disk entry (which re-warms both
76+
/// tiers). Never mutates a tier except to drop an entry it finds expired. `.memory`/`.none` never touch
77+
/// disk, so a read can't destroy a durable copy written at `.memoryAndFile`.
78+
func object(forResource resource: String, level: Networking.CachingLevel, asImage: Bool) throws -> Any? {
79+
let destinationURL = try destinationURL(forResource: resource)
80+
let key = destinationURL.absoluteString
81+
switch level {
82+
case .memory:
83+
return memory.object(forKey: key as AnyObject)
84+
case .memoryAndFile:
85+
// Memory is the warm tier — a memory hit is served without touching the disk (the NSCache
86+
// absorbs repeat reads, so the file's mtime is only re-warmed on a memory miss, below).
87+
if let object = memory.object(forKey: key as AnyObject) {
88+
return object
89+
} else if FileManager.default.exists(at: destinationURL) {
90+
let fileDate = try? destinationURL.resourceValues(forKeys: [.contentModificationDateKey]).contentModificationDate
91+
if expiry.isExpired(fileDate: fileDate) {
92+
try FileManager.default.remove(at: destinationURL)
93+
return nil
94+
}
95+
96+
// The file can vanish between the exists() check above and this read — the background
97+
// sweep runs concurrently and doesn't share a lock with reads — so a read failure is a
98+
// cache miss, not a crash.
99+
guard let data = FileManager.default.contents(atPath: destinationURL.path) else { return nil }
100+
let returnedObject: Any? = asImage ? Image(data: data) : data
101+
if let returnedObject {
102+
memory.setObject(returnedObject as AnyObject, forKey: key as AnyObject)
103+
// Re-warm: bump the file's mtime so an entry in active use never expires. Only happens
104+
// on a memory miss, so it's ~once per entry per launch — no explicit debounce needed.
105+
try? FileManager.default.setAttributes([.modificationDate: Date()], ofItemAtPath: destinationURL.path)
106+
}
107+
108+
return returnedObject
109+
} else {
110+
return nil
111+
}
112+
case .none:
113+
return nil
114+
}
115+
}
116+
117+
// MARK: - Write
118+
119+
func storeData(_ data: Data?, forResource resource: String, level: Networking.CachingLevel) throws {
120+
let destinationURL = try destinationURL(forResource: resource)
121+
let key = destinationURL.absoluteString
122+
123+
if let returnedData = data, returnedData.count > 0 {
124+
switch level {
125+
case .memory:
126+
memory.setObject(returnedData as AnyObject, forKey: key as AnyObject)
127+
case .memoryAndFile:
128+
_ = try returnedData.write(to: destinationURL, options: [.atomic])
129+
memory.setObject(returnedData as AnyObject, forKey: key as AnyObject)
130+
case .none:
131+
break
132+
}
133+
// The disk write itself sets a fresh mtime — that *is* the entry's last-use timestamp.
134+
} else {
135+
memory.removeObject(forKey: key as AnyObject)
136+
}
137+
}
138+
139+
@discardableResult
140+
func storeImage(data: Data?, forResource resource: String, level: Networking.CachingLevel) throws -> Image? {
141+
let destinationURL = try destinationURL(forResource: resource)
142+
let key = destinationURL.absoluteString
143+
144+
var image: Image?
145+
if let data = data, let nonOptionalImage = Image(data: data), data.count > 0 {
146+
switch level {
147+
case .memory:
148+
memory.setObject(nonOptionalImage, forKey: key as AnyObject)
149+
case .memoryAndFile:
150+
_ = try data.write(to: destinationURL, options: [.atomic])
151+
memory.setObject(nonOptionalImage, forKey: key as AnyObject)
152+
case .none:
153+
break
154+
}
155+
image = nonOptionalImage
156+
} else {
157+
memory.removeObject(forKey: key as AnyObject)
158+
}
159+
160+
return image
161+
}
162+
163+
// MARK: - Clear & sweep
164+
165+
// Serializes whole-folder mutations of the shared cache directory so the background sweep (which
166+
// creates the folder + writes its cursor) can't race a clear.
167+
static let mutationLock = NSLock()
168+
static let sweepCursorFileName = ".sweep-shard"
169+
170+
/// Empties **both** tiers (clearing only one would leave the other serving deleted data). Scoped to the
171+
/// networking folder; unrelated files in Caches are untouched.
172+
func clear() throws {
173+
memory.removeAllObjects()
174+
Self.mutationLock.lock()
175+
defer { Self.mutationLock.unlock() }
176+
guard let folderURL = Self.folderURL(named: folderName) else { return }
177+
if FileManager.default.exists(at: folderURL) {
178+
_ = try FileManager.default.remove(at: folderURL)
179+
}
180+
}
181+
182+
private static func folderURL(named folderName: String) -> URL? {
183+
guard let cachesURL = FileManager.default.urls(for: .cachesDirectory, in: .userDomainMask).first else { return nil }
184+
return cachesURL.appendingPathComponent(URL(string: folderName)!.absoluteString)
185+
}
186+
187+
// Deletes expired files from **one** shard per call (rotated via a tiny cursor file), so each launch's
188+
// sweep is O(N / shardCount) and everything gets visited over `shardCount` launches. Age is judged by
189+
// the file's modification date. Best-effort and off the request path.
190+
func sweepExpired() {
191+
Self.mutationLock.lock()
192+
defer { Self.mutationLock.unlock() }
193+
guard let domainURL = Self.folderURL(named: folderName) else { return }
194+
let now = Date()
195+
let maxAge = CacheExpiry.seconds(expiry.ttl)
196+
197+
let cursorURL = domainURL.appendingPathComponent(Self.sweepCursorFileName)
198+
let cursor = (try? String(contentsOf: cursorURL, encoding: .utf8)).flatMap { Int($0.trimmingCharacters(in: .whitespacesAndNewlines)) } ?? 0
199+
let shardURL = domainURL.appendingPathComponent(String(cursor % Self.shardCount, radix: 16))
200+
201+
if let files = try? FileManager.default.contentsOfDirectory(at: shardURL, includingPropertiesForKeys: [.contentModificationDateKey], options: []) {
202+
for file in files {
203+
guard let modified = try? file.resourceValues(forKeys: [.contentModificationDateKey]).contentModificationDate else { continue }
204+
if now.timeIntervalSince(modified) > maxAge {
205+
try? FileManager.default.removeItem(at: file)
206+
}
207+
}
208+
}
209+
210+
// Pre-sharding versions wrote files directly under the domain root; clear those strays (the cursor
211+
// file and the shard subdirectories stay).
212+
if let rootEntries = try? FileManager.default.contentsOfDirectory(at: domainURL, includingPropertiesForKeys: [.isRegularFileKey], options: []) {
213+
for entry in rootEntries where entry.lastPathComponent != Self.sweepCursorFileName {
214+
if (try? entry.resourceValues(forKeys: [.isRegularFileKey]).isRegularFile) == true {
215+
try? FileManager.default.removeItem(at: entry)
216+
}
217+
}
218+
}
219+
220+
try? FileManager.default.createDirectory(at: domainURL, withIntermediateDirectories: true)
221+
try? String(cursor &+ 1).write(to: cursorURL, atomically: true, encoding: .utf8)
222+
}
223+
}

Sources/Networking/JSONResponse.swift

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -37,23 +37,6 @@ public struct AnyCodable: Decodable, @unchecked Sendable {
3737
}
3838
}
3939

40-
func encode(to encoder: Encoder) throws {
41-
var container = encoder.singleValueContainer()
42-
43-
if self.value is NSNull {
44-
try container.encodeNil()
45-
} else if let value = self.value as? Bool {
46-
try container.encode(value)
47-
} else if let value = self.value as? Int {
48-
try container.encode(value)
49-
} else if let value = self.value as? Double {
50-
try container.encode(value)
51-
} else if let value = self.value as? String {
52-
try container.encode(value)
53-
} else {
54-
throw EncodingError.invalidValue(value, EncodingError.Context(codingPath: container.codingPath, debugDescription: "The value cannot be encoded"))
55-
}
56-
}
5740
}
5841

5942
extension AnyCodable: Hashable {

0 commit comments

Comments
 (0)