Skip to content

Configurable TTL for the targeting cache - #62

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

Configurable TTL for the targeting cache#62
OlenaPostindustria wants to merge 4 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.

guard isTargetingFresh() else {
clearTargeting()
return nil
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Have this concern from Claude that I believe is relevant. Race condition in the set / clear path.

  Failure scenario, expired entry present:

  1. Main thread targetingFromCache() reads targetingData (old), reads the old storedAt, decides expired.
  2. Background thread completes targeting() and runs setTargeting() — writes the new timestamp and all three data keys.
  3. Main thread resumes into clearTargeting() and removes all four keys.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oh this is already discussed above I did not read the comments. I will approve but this should be addressed in a followup

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

good catch! guarded set / clear path with NSLock

@mosherBT mosherBT left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM except one comment

@OlenaPostindustria
OlenaPostindustria force-pushed the 60/configurable-ttl-for-targeting-cache branch from 93bf653 to 47b3f6a Compare September 4, 2026 09:14
@OlenaPostindustria
OlenaPostindustria requested review from a team as code owners September 4, 2026 09:14
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)

3 participants