Configurable TTL for the targeting cache - #62
Conversation
eugenedorfman-optable
left a comment
There was a problem hiding this comment.
One race worth fixing before merge, plus a boundary difference against the Android SDK.
The cache write is not atomic against the new destructive read. setTargeting writes four UserDefaults keys separately with the timestamp last (Source/Core/LocalStorage.swift:74-77), and getTargeting now calls clearTargeting() when the timestamp is missing or stale. setTargeting runs in URLSession completion handlers (Source/OptableSDK.swift:344 and :400) on a background queue, while targetingFromCache() is typically called from the main thread. A read landing between the data writes and the timestamp write therefore sees fresh data with no timestamp, and wipes the entry that was just fetched. Before this change the read path never mutated storage, so this failure mode is new. Writing targetingStoredAtKey first would narrow the window; storing the timestamp alongside the data in a single value, the way Android does it in one prefs.edit block, closes it.
Expiry boundary is inclusive. Source/Core/LocalStorage.swift:96 uses age <= config.cacheTTL, while Android expires an entry at exactly the TTL (age !in 0..<ttlMs). It also makes the documented behaviour of cacheTTL = 0 ("disables caching entirely", Source/Public/OptableConfig.swift:56, repeated at docs/usage-swift.md:171 and docs/usage-objc.md:160) not strictly true, since an age of exactly 0 counts as fresh. Using age < config.cacheTTL makes the zero case exact and matches Android.
No test for cacheTTL = 0 in Tests/Unit/LocalStorageTests.swift, which is the behaviour the docs promise. The exact boundary itself is not worth a test: with a wall clock the measured age always lands just past the TTL, so such a test would pass under either comparison.
Everything else checked out. The timestamp is written on every set, a missing timestamp (the upgrade path from a cache stored by an earlier SDK version) and a future timestamp are both treated as expired, clearTargeting removes the timestamp too, and the tests backdate the stored value instead of sleeping, so they are not wall-clock flaky.
eugenedorfman-optable
left a comment
There was a problem hiding this comment.
The three points from the earlier review are addressed: the expiry boundary is now exclusive and matches the Android SDK, the timestamp is written before the data so a concurrent read can no longer clear an entry that was just fetched, and there is a test for cacheTTL = 0. CI is green.
The remaining non-atomicity of the four separate UserDefaults writes predates this change and is not worth holding the PR for.
Closes #60