fix: reverted onInitialized behaviour to previous and moved logic to …#295
fix: reverted onInitialized behaviour to previous and moved logic to …#295jsalaber wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts cached-config initialization behavior so the OpenFeature provider can return immediately from cache while the DevCycle client continues its network initialization path.
Changes:
- Reworked
DevCycleClientinitialization to always perform a network fetch after loading cached config. - Moved cache-hit readiness handling into
DevCycleProvider. - Added provider tests for cache-hit background refresh success/error behavior and removed older client cache/auth tests.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
android-client-sdk/src/main/java/com/devcycle/sdk/android/api/DevCycleClient.kt |
Changes startup and identify-user cache fallback behavior. |
android-client-sdk/src/main/java/com/devcycle/sdk/android/openfeature/DevCycleProvider.kt |
Handles cached initialization readiness and background refresh events in the provider. |
android-client-sdk/src/main/java/com/devcycle/sdk/android/util/DVCSharedPrefs.kt |
Removes per-user persisted config clearing helper. |
android-client-sdk/src/test/java/com/devcycle/sdk/android/api/DevCycleClientTests.kt |
Removes cache-first initialization and auth-error cache clearing tests. |
android-client-sdk/src/test/java/com/devcycle/sdk/android/openfeature/DevCycleProviderTest.kt |
Adds OpenFeature provider cache-hit/background refresh tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
| } | ||
| } catch (t: Throwable) { | ||
| DevCycleLogger.e(t, "DevCycle SDK Failed to Initialize!") | ||
| throw t |
There was a problem hiding this comment.
notifyConfigError is defined but never called — the init catch block does not call it before rethrowing, so direct onConfigUpdated subscribers will not hear about init failures. Either call it here or remove the method if it is intentionally out of scope for this PR.
There was a problem hiding this comment.
notifyConfigError gets called for any refetch / sse config fetches
|
Should we rename |
jsalaber
force-pushed
the
fix-on-initialized-behaviour
branch
from
e2ee2d1 to
0098d54
Compare
jsalaber
force-pushed
the
fix-on-initialized-behaviour
branch
from
0098d54 to
10a3ffb
Compare
jsalaber
force-pushed
the
fix-on-initialized-behaviour
branch
from
10a3ffb to
2c62e1a
Compare
jsalaber
force-pushed
the
fix-on-initialized-behaviour
branch
from
2c62e1a to
d57b5b2
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Comments suppressed due to low confidence (1)
android-client-sdk/src/test/java/com/devcycle/sdk/android/api/DevCycleClientTests.kt:2063
- With the new client behavior, initialization always starts a network fetch, but this cached-provider test does not enqueue any config response. That can leave the background initialization request blocked/leaking after provider.initialize returns, and it no longer exercises the post-cache refresh path deterministically.
Assertions.assertTrue(
provider.devcycleClient.hasUsableCachedConfig,
"Provider's underlying client should report a usable cached config"
)
val eval = provider.getStringEvaluation("cached-flag", "default", null)
Assertions.assertEquals("cached-value", eval.value)
d57b5b2 to
2efc6ca
Compare
| _providerEvents.tryEmit(OpenFeatureProviderEvents.ProviderError( | ||
| OpenFeatureError.GeneralError(t.message ?: "Config error") | ||
| )) | ||
| } |
There was a problem hiding this comment.
These continuation.resume / resumeWithException calls aren't guarded by continuation.isActive. If the calling coroutine is cancelled before the network fetch completes (e.g. an OpenFeature init timeout), the callbacks are still registered on _devcycleClient and will try to resume an already-inactive continuation when the fetch eventually finishes. I'd guard each call with if (continuation.isActive), or use invokeOnCancellation to deregister.
jsalaber
force-pushed
the
fix-on-initialized-behaviour
branch
from
2efc6ca to
bb57d90
Compare
| override fun onSuccess(result: String) { | ||
| DevCycleLogger.d("DevCycle OpenFeature provider initialized successfully") | ||
| continuation.resume(Unit) | ||
| if (!continuation.isActive) return | ||
| if (consumeResume()) { | ||
| continuation.resume(Unit) | ||
| } else { | ||
| _providerEvents.tryEmit(OpenFeatureProviderEvents.ProviderConfigurationChanged) |
| override fun onError(t: Throwable) { | ||
| DevCycleLogger.e("DevCycle OpenFeature provider initialization failed: ${t.message}") | ||
| continuation.resumeWithException( | ||
| OpenFeatureError.ProviderFatalError("DevCycle client initialization error: ${t.message}") | ||
| ) | ||
| if (!continuation.isActive) return | ||
| if (!isCacheHit) { | ||
| // No cache — fatal. | ||
| if (consumeResume()) { | ||
| continuation.resumeWithException( | ||
| OpenFeatureError.ProviderFatalError("DevCycle client initialization error: ${t.message}") | ||
| ) | ||
| } | ||
| } else { | ||
| // Cache hit: network error is non-fatal; cached config remains usable. | ||
| _providerEvents.tryEmit(OpenFeatureProviderEvents.ProviderError( | ||
| OpenFeatureError.GeneralError("Background refresh failed: ${t.message}") | ||
| )) |
Changes