Fix flaky tests caused by data races in MockDeviceCache#6888
Conversation
|
@RCGitBot please test |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0a8e91f. Configure here.
| var cachedCustomerInfo: [String: Data] { | ||
| get { self._cachedCustomerInfo.value } | ||
| set { self._cachedCustomerInfo.value = newValue } | ||
| } |
There was a problem hiding this comment.
Non-atomic read-modify-write on cachedCustomerInfo via computed property
Low Severity
Converting cachedCustomerInfo from a stored property to a computed property backed by Atomic changes the semantics of mutating calls like cachedCustomerInfo.removeValue(forKey:) (line 257 in clearCustomerInfoCache). This now performs a non-atomic get-copy-mutate-set through the computed property's getter and setter, while cache(customerInfo:appUserID:) correctly uses _cachedCustomerInfo.modify { ... } for an atomic update. If both methods run concurrently, the removeValue can silently lose a concurrent write from cache.
Reviewed by Cursor Bugbot for commit 0a8e91f. Configure here.
There was a problem hiding this comment.
We should be using the _ cachedCustomerInfo.modify {} in the relevant tests that were causing issues due to multi-thread access, so we should be covered.
We could choose to update all callsites, but I tried to keep the diff minimal.
|
@RCGitBot please test |
22a155f to
1fdd4e0
Compare
|
@RCGitBot please test |
1 similar comment
|
@RCGitBot please test |
b05effa to
c91390d
Compare
|
@RCGitBot please test |
ajpallares
left a comment
There was a problem hiding this comment.
Thank you for this improvement!
MockDeviceCachepossibly being accessed from multiple threads at the same time in tests. This was fixed by backing the fields withAtomic<T>and updating the relevant callsites to use the.modify {}method.testPurchaseOutsideTheAppUpdatesCustomerInfoDelegatein an attempt to reduce flakiness there as well.Note
Low Risk
Test-only mock changes with no production SDK behavior; aligns with existing Atomic usage in other mocks.
Overview
Makes customer info tracking in
MockDeviceCachethread-safe so unit tests match concurrent SDK access (e.g. configure + foreground sync).Counters (
cacheCustomerInfoCount,cachedCustomerInfoCount, etc.) and thecachedCustomerInfodictionary are backed by privateAtomicstorage with the same public property names. Overrides usemodifyfor increments and dictionary updates instead of non-atomic+=and subscript writes.Reviewed by Cursor Bugbot for commit 0a8e91f. Bugbot is set up for automated code reviews on this repo. Configure here.