Skip to content

Configurable TTL for the targeting cache - #62

Open
OlenaPostindustria wants to merge 3 commits into
masterfrom
60/configurable-ttl-for-targeting-cache
Open

Configurable TTL for the targeting cache#62
OlenaPostindustria wants to merge 3 commits into
masterfrom
60/configurable-ttl-for-targeting-cache

Conversation

@OlenaPostindustria

Copy link
Copy Markdown
Collaborator

Closes #60

@eugenedorfman-optable eugenedorfman-optable left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 eugenedorfman-optable left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Configurable TTL for the targeting cache (default 24 hours)

2 participants