Cache correctness fixes + CacheStore extraction; drop dead code#322
Merged
Conversation
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).
- 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).
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.
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.
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.
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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Found during a file-by-file review. Each bug fix is red-first tested; the refactor is behavior-preserving (full suite green, warning-free).
Dead code
AnyCodable.encode(to:)—AnyCodableisDecodable-only, so it satisfied no protocol and had no callers (and was incomplete: threw on the nested collectionsinit(from:)produces).Bug fixes (red-first, see
CacheWriteAndDataBodyRegressionTests)requestDatacached the payload under the path-derived key, then the download handlers cached it again under the realcacheName— leaving a stray file no read path uses (and a redundant double-write when nocacheName). Caching is now the handlers' job only.Dataon verbs. A verb withT == Datareturned an emptyDataon success instead of the response body. Returns the body now (theVoidoverloads discard it, so they're unaffected)..memoryread destroyed the disk tier.objectFromCache's.memorybranch deleted the on-disk file on every read (since the 2018 caching feature). With warm/cold tiering, that file is the durable copy of an earlier.memoryAndFilewrite — so onceNSCacheevicted the warm copy under memory pressure (which iOS does routinely), the entry was lost entirely..noneread purged both tiers. Same class of loss on the download path.objectFromCacheis now a pure read at every level — purging is the write path's andclearCache's job.Refactor
CacheStore. The two-tier cache, sharded on-disk layout, TTL/expiry, sweep, and clear were scattered across static + instance members on theNetworkingactor. Now a dedicated non-actor@unchecked Sendablekeyed by a resolved resource string.Networkingkeeps the public surface unchanged (destinationURL/objectFromCache/cacheOrPurge*are thin shims; the injectedNSCacheis shared by reference so it stays inspectable).Tests
180 tests green against go-httpbin; build warning-free. New regression tests cover the orphan write, the
Databody, and the.memory/.nonepure-read contracts.