FFL-1878: Fix stale state context mismatch#3224
Conversation
e864065 to
942f9ed
Compare
This comment has been minimized.
This comment has been minimized.
| * | ||
| * This state occurs: | ||
| * - Before the first [setEvaluationContext()] call (initialization) | ||
| * - After client shutdown |
There was a problem hiding this comment.
I don't see a shutdown method, so if we want to keep this comment here, it would help to point instead to a specific function or code path. The closest thing I could find is Datadog.stopInstance()
| * - After client shutdown | ||
| * - When the provider is not initialized or unavailable | ||
| * | ||
| * No flags are available for evaluation in this state. |
There was a problem hiding this comment.
This is not quite true. There may be cached flags available
Provider readiness checks if there's an evaluation context (which may be cached) and doesn't check the client's state
iOS does something similar in checking the cached context, but there is a difference in behaviors
| Scenario | iOS | Android |
|---|---|---|
Fresh install, NotReady state |
context = nil → PROVIDER_NOT_READY |
Same |
App restart with cached data, NotReady state |
context = cached → flags work | Same |
After reset() |
context = nil → PROVIDER_NOT_READY |
N/A (no reset method) |
iOS has a reset() which clears data and sets state to notReady. Android doesn't have this method.
| // Only use cached flags if they match the requested context to avoid | ||
| // serving flags from a different user/context. | ||
| val cachedContextMatches = flagsRepository.getEvaluationContext() == context | ||
| if (hadFlags && cachedContextMatches) { |
There was a problem hiding this comment.
This fixes the state transition logic if there's a network error when requesting flags for a context (User B). It's only safe to serve "stale" flags if the requested context matches the cached context
| Cached Flags | Cached Context | Requested Context | State |
|---|---|---|---|
| None | - | User B | Error |
| Exist | User A | User A | Stale |
| Exist | User A | User B | Error ✓ (was incorrectly Stale) |
Assuming User B should never see User A's flags
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3224 +/- ##
===========================================
- Coverage 71.42% 71.35% -0.07%
===========================================
Files 940 940
Lines 34764 34765 +1
Branches 5894 5894
===========================================
- Hits 24828 24806 -22
- Misses 8288 8317 +29
+ Partials 1648 1642 -6
🚀 New features to boost your workflow:
|
942f9ed to
08a17b8
Compare
08a17b8 to
41107ad
Compare
3 participants
What does this PR do?
Fixes a bug where the Flags SDK would transition to
Stalestate when a network request fails, without checking if the cached context matches the requested context. This could result in serving flags from a different user/context.Changes:
Stalestate. If contexts don't match, transitions toErrorinstead.NotReadystate documentation to remove incorrect claim about flag availability.Motivation
When
setEvaluationContext()fails due to a network error, the SDK was transitioning toStalestate if any cached flags existed, without verifying the cached context matched the requested context. This could cause:This fix aligns the Android SDK behavior with the iOS SDK implementation.
Additional Notes
FlagsRepository.swift)Fixes: FFL-1878
Review checklist (to be filled by reviewers)