feat: SDK-4363: Turbine remote SDK feature flags and foreground refresh#2612
feat: SDK-4363: Turbine remote SDK feature flags and foreground refresh#2612abdulraqeeb33 wants to merge 5 commits intomainfrom
Conversation
- Compare base.sha...head.sha on pull_request so the target branch matches the PR base. - Enforce >= threshold on aggregate touched executable lines; keep per-file detail informational. - Fail when main-source files in the diff cannot be mapped to JaCoCo. - Rename workflow step for clarity. Made-with: Cursor
- GET /apps/:app_id/sdk/features/android/:sdk_version with validated version label - FeatureFlagsJsonParser (kotlinx.serialization), metadata as sibling JsonObjects - FeatureFlagsRefreshService with ~10 min foreground polling; ConfigModelChangeTags.REMOTE_FEATURE_FLAGS - FeatureManager merges remote flags; IFeatureManager.remoteFeatureFlagMetadata() - SDK_BACKGROUND_THREADING / sdk_background_threading; CoreModule wiring and tests Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Implements Turbine-aligned remote SDK feature flags on Android by adding a dedicated backend client + JSON parser, persisting results into ConfigModel, and wiring foreground refresh + FeatureManager evaluation (including metadata access).
Changes:
- Add Turbine SDK feature flags backend service and strict kotlinx.serialization-based parser (with optional per-flag sibling metadata objects).
- Add
ConfigModelfields for remote SDK flags + metadata, and a foreground polling refresh service to keep them updated. - Update feature flag key conventions and threading flag usage across core, plus Gradle/CI coverage tooling updates.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt | Updates mocks for new IFeatureManager API + new flag enum value. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt | Updates mocks for new IFeatureManager API + new flag enum value. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt | Expands tests for remote sdk flags + metadata parsing. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureFlagTests.kt | Updates key-format expectations for FeatureFlag.key. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt | Adds unit tests for new Turbine feature-flags JSON parser. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTest.kt | Adds unit tests for Turbine endpoint path construction + version label validation. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt | Switches to the new SDK_BACKGROUND_THREADING flag. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt | Switches to the new SDK_BACKGROUND_THREADING flag. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt | Switches to the new SDK_BACKGROUND_THREADING flag. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt | Evaluates new remote SDK flags, observes new change tags/properties, and exposes metadata map API. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt | Updates canonical feature key to lowercase Turbine/ConfigCat-style id. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt | Adds foreground polling service to fetch/apply remote SDK flags + metadata into ConfigModel. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt | Introduces a dedicated replace-tag for remote SDK feature flag updates. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt | Adds persisted fields for remote SDK feature flags + metadata and JSON-array parsing support. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt | Adds strict JSON parsing + metadata encoding/decoding helpers. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt | Adds Turbine endpoint client with SDK version label validation and parsing integration. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IFeatureFlagsBackendService.kt | Adds backend interface and result model for remote SDK feature flags. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt | Registers new backend service + refresh service into the module wiring. |
| OneSignalSDK/onesignal/core/build.gradle | Enables Kotlin serialization plugin + adds kotlinx-serialization-json dependency. |
| OneSignalSDK/coverage/checkCoverage.sh | Updates diff-coverage gating to use exact PR base/head SHAs and an aggregate “touched lines” gate. |
| OneSignalSDK/build.gradle | Adds kotlinx serialization plugin classpath and version pin. |
| .github/workflows/ci.yml | Passes PR base/head SHAs into diff coverage step for accurate patch gating. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| val newMetaString = FeatureFlagsJsonParser.encodeMetadata(result.metadata) | ||
| if (result.enabledKeys == current.sdkRemoteFeatureFlags && newMetaString == current.sdkRemoteFeatureFlagMetadata) { | ||
| return | ||
| } | ||
|
|
||
| val updated = ConfigModel() | ||
| updated.initializeFromModel(null, current) | ||
| updated.sdkRemoteFeatureFlags = result.enabledKeys |
| if (applicationService.isInForeground) { | ||
| onFocus(firedOnSubscribe = true) | ||
| } |
| private fun restartForegroundPolling() { | ||
| pollJob?.cancel() | ||
| pollJob = | ||
| launchOnIO { | ||
| while (coroutineContext.isActive) { | ||
| if (!applicationService.isInForeground) { | ||
| break | ||
| } | ||
| val appId = configModelStore.model.appId | ||
| if (appId.isNotEmpty()) { | ||
| try { | ||
| fetchAndApply(appId) | ||
| } catch (t: Throwable) { | ||
| Logging.warn("FeatureFlagsRefreshService: fetch failed", t) | ||
| } | ||
| } | ||
| delay(REFRESH_INTERVAL_MS) | ||
| } | ||
| } |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Annotate pollJob with @volatile for cross-thread visibility - Remove duplicate onFocus from start(); ApplicationService already fires on subscribe - Apply remote flags via in-place model property updates (REMOTE_FEATURE_FLAGS tag) to avoid clobbering concurrent params hydration with a stale full-model replace - Use OneSignalDispatchers.launchOnIO so CancellationException is not logged as an error - Compare enabled key sets (order-insensitive) before writing Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Implements Turbine-aligned remote SDK feature flags for the Android SDK, including a dedicated backend fetch, JSON parsing with optional per-flag metadata, foreground refresh polling, and FeatureManager integration.
Changes:
- Added Turbine SDK feature flags endpoint client + path/version label validation.
- Added kotlinx.serialization-based parser for
features+ optional sibling per-flag metadata, storing results intoConfigModel. - Added foreground polling service to refresh remote SDK flags periodically, and updated FeatureManager/FeatureFlag to use lowercase remote keys.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapperTest.kt | Updates mocks to use new SDK_BACKGROUND_THREADING flag + new remoteFeatureFlagMetadata() API. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/startup/StartupServiceTests.kt | Updates mocks for renamed flag + new FeatureManager API. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureManagerTests.kt | Expands FeatureManager tests to include sdk-remote flags + stored metadata parsing behavior. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/features/FeatureFlagTests.kt | Updates feature-flag key convention test to lowercase snake_case and validates new canonical key. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPathTest.kt | New tests for RFC3986-ish path encoding + Turbine relative path construction. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParserTests.kt | New tests for JSON parsing, normalization, metadata extraction, and storage round-trip. |
| OneSignalSDK/onesignal/core/src/test/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendServiceTest.kt | New tests for endpoint path building and sdk version label validation. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/OneSignalImp.kt | Switches background threading gate to SDK_BACKGROUND_THREADING. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt | Switches background threading gate to SDK_BACKGROUND_THREADING. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/startup/StartupService.kt | Switches background threading gate to SDK_BACKGROUND_THREADING. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureManager.kt | Unions legacy config features with sdk-remote flags; adds remoteFeatureFlagMetadata(); listens to new config fields/tags. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/features/FeatureFlag.kt | Renames/updates BACKGROUND_THREADING feature key to lowercase remote id. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/impl/FeatureFlagsRefreshService.kt | New foreground polling service that fetches and stores remote SDK feature flags + metadata. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModelChangeTags.kt | New config replace tag for remote feature-flag refresh updates. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/config/ConfigModel.kt | Adds persisted fields for sdk-remote feature keys and per-flag metadata string; supports list creation for new field. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/TurbineSdkFeatureFlagsPath.kt | New pure-Kotlin path builder + version label validator + UTF-8 percent-encoding helper. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsJsonParser.kt | New kotlinx.serialization parser for endpoint response and metadata storage/restore utilities. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/impl/FeatureFlagsBackendService.kt | New HTTP client implementation for fetching remote SDK feature flags from Turbine endpoint. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/internal/backend/IFeatureFlagsBackendService.kt | New backend interface + result model for enabled keys and optional metadata payloads. |
| OneSignalSDK/onesignal/core/src/main/java/com/onesignal/core/CoreModule.kt | Registers new backend service and refresh service into the core module DI graph. |
| OneSignalSDK/onesignal/core/build.gradle | Enables Kotlin serialization plugin and adds kotlinx-serialization-json dependency. |
| OneSignalSDK/build.gradle | Adds kotlinx serialization JSON version and Kotlin serialization Gradle plugin dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| fetchAndApply(appId) | ||
| } catch (t: Throwable) { | ||
| Logging.warn("FeatureFlagsRefreshService: fetch failed", t) | ||
| } |
There was a problem hiding this comment.
Catching Throwable here will also catch CancellationException when the polling job is cancelled (e.g., onUnfocused / restartForegroundPolling), which can lead to noisy warning logs and can interfere with prompt coroutine cancellation. Prefer not catching cancellation (e.g., catch Exception and rethrow CancellationException) so the Job cancels cleanly.
| val enabledFeatureKeys = | ||
| ( | ||
| model.features + | ||
| model.sdkRemoteFeatureFlags + | ||
| localFeatureOverrides | ||
| ).toSet() |
There was a problem hiding this comment.
FeatureFlag.key is now documented/validated as lowercase, but enabledFeatureKeys is built from model.features + model.sdkRemoteFeatureFlags without normalizing casing. If cached/remote values ever differ in case (including upgrades from previously stored keys), APP_STARTUP flags can latch the wrong value for the whole run. Consider canonicalizing the unioned keys (e.g., lowercase) or providing an alias mapping for legacy keys before evaluating FeatureFlag.isEnabledIn().
| /** | ||
| * Remote feature-flags HTTP endpoint updated [ConfigModel] SDK flag fields only | ||
| * ([sdkRemoteFeatureFlags], [sdkRemoteFeatureFlagMetadata]). | ||
| */ |
There was a problem hiding this comment.
KDoc links in this comment likely won't resolve as written because sdkRemoteFeatureFlags / sdkRemoteFeatureFlagMetadata aren't in scope here. Qualify them as ConfigModel.sdkRemoteFeatureFlags / ConfigModel.sdkRemoteFeatureFlagMetadata (or use plain code formatting) so the documentation stays accurate and navigable.
| // Threading mode is selected once per app startup to avoid mixed-mode behavior mid-session. | ||
| // | ||
| // Naming convention: | ||
| // SDK_<MMmmpp>_<FEATURE_DESCRIPTION> | ||
| // Remote key (lowercase) must match backend / ConfigCat flag id. | ||
| // |
There was a problem hiding this comment.
This comment mentions ConfigCat, but there are no other references to ConfigCat in the codebase and the rest of this PR describes Turbine/remote config. Consider removing or updating the reference so readers don't infer an unrelated dependency or backend source of truth.
abdulraqeeb33
changed the title
| val result = featureFlagsBackend.fetchRemoteFeatureFlags(appId) | ||
| val current = configModelStore.model | ||
| val newMetaString = FeatureFlagsJsonParser.encodeMetadata(result.metadata) | ||
| if (result.enabledKeys.toSet() == current.sdkRemoteFeatureFlags.toSet() && | ||
| newMetaString == current.sdkRemoteFeatureFlagMetadata | ||
| ) { | ||
| return | ||
| } | ||
|
|
||
| val tag = ConfigModelChangeTags.REMOTE_FEATURE_FLAGS | ||
| current.setListProperty(ConfigModel::sdkRemoteFeatureFlags.name, result.enabledKeys, tag) | ||
| current.setOptStringProperty(ConfigModel::sdkRemoteFeatureFlagMetadata.name, newMetaString, tag) | ||
| } | ||
|
|
||
| companion object { | ||
| private const val REFRESH_INTERVAL_MS = 600_000L |
There was a problem hiding this comment.
🔴 In fetchAndApply(), RemoteFeatureFlagsResult.EMPTY (with enabledKeys=emptyList()) is returned for all failure modes—HTTP non-success, empty/null body, invalid SDK version, and JSON parse errors—making it indistinguishable from a legitimate server response of "no flags enabled". When previously-cached flags exist (e.g. ["sdk_background_threading"]), the equality guard emptySet() \!= setOf("sdk_background_threading") fails and setListProperty is called with emptyList(), wiping all stored feature flags from SharedPreferences. A single transient server 500 or network error silently disables all remote feature flags at the next app startup; fix by distinguishing failure results from a legitimately-empty server response and skipping the write on failure.
Extended reasoning...
What the bug is and how it manifests
FeatureFlagsBackendService.fetchRemoteFeatureFlags() returns the sentinel value RemoteFeatureFlagsResult.EMPTY (which has enabledKeys = emptyList() and metadata = null) for at least four distinct failure conditions: (1) the SDK version label fails validation via isValidFeaturesSdkVersionLabel, (2) the HTTP response is non-success or has a null/blank body, (3) the JSON body cannot be parsed by FeatureFlagsJsonParser.parse() (the catch (_: Throwable) block returns EMPTY), and (4) the top-level parsed element is not a JsonObject. In all of these cases, the returned value is identical to what the server would return for a legitimate response containing zero enabled flags. There is no field or separate return path to distinguish "request failed" from "server says no flags".
The specific code path that triggers it
In fetchAndApply() (FeatureFlagsRefreshService.kt lines 105–120), the equality guard compares result.enabledKeys.toSet() against current.sdkRemoteFeatureFlags.toSet(). If the device previously had cached flags — for example ["sdk_background_threading"] — and a transient HTTP 500 or network timeout causes fetchRemoteFeatureFlags to return EMPTY, the comparison becomes emptySet() == setOf("sdk_background_threading"), which is false. The early-return is not taken. The code proceeds to call current.setListProperty(ConfigModel::sdkRemoteFeatureFlags.name, result.enabledKeys, tag) where result.enabledKeys is emptyList(), clearing all persisted feature flags.
Why existing code does not prevent it
The equality guard was intended to prevent unnecessary writes when nothing has changed, but it cannot distinguish between two semantically different situations that produce the same value: (a) the server responded with an empty features array, and (b) the request failed entirely. RemoteFeatureFlagsResult.EMPTY is a single shared sentinel used as a universal failure fallback across all error branches. There is no isFailure field, no separate error return type, and no null check in fetchAndApply to skip the write on failure.
What the impact would be
For the current session, the impact is limited: FeatureManager has already latched APP_STARTUP features (like SDK_BACKGROUND_THREADING) at startup with applyNextRunOnlyFeatures=true, and subsequent onModelUpdated calls during the session are guarded by the APP_STARTUP check which prevents changing the latched value mid-run. However, sdkRemoteFeatureFlags in ConfigModel/SharedPreferences IS cleared. On the next app startup, FeatureManager.init calls refreshEnabledFeatures(model, applyNextRunOnlyFeatures=true) and reads the now-empty sdkRemoteFeatureFlags, so sdk_background_threading is absent from enabledFeatureKeys and SDK_BACKGROUND_THREADING is initialized to false. The SDK runs in legacy threading mode for that entire session — even though the feature was previously enabled and the server would have returned it enabled if the transient error had not occurred.
How to fix it
The cleanest fix is to return null (or a dedicated Failure subtype) from fetchRemoteFeatureFlags on error, and only write to the model when the result is non-null. For example:
- Change the return type to
RemoteFeatureFlagsResult?and returnnullin all error branches inFeatureFlagsBackendService. - In
fetchAndApply, addif (result == null) returnbefore the equality check. - Reserve
RemoteFeatureFlagsResult.EMPTY(or a newRemoteFeatureFlagsResult(emptyList(), null)instance) exclusively for the legitimate case where the server responds with an emptyfeaturesarray.
Alternatively, add a val isFailure: Boolean field to RemoteFeatureFlagsResult and set it to true in all error code paths, then skip the write in fetchAndApply when result.isFailure is true.
Step-by-step proof
- App starts.
FeatureManager.initreadssdkRemoteFeatureFlags = ["sdk_background_threading"]from SharedPreferences.SDK_BACKGROUND_THREADINGis latched totrue. The SDK starts in background-threading mode. - App is foregrounded.
FeatureFlagsRefreshService.restartForegroundPolling()launches the polling coroutine. After 10 minutes, it callsfetchAndApply(appId). - The server returns HTTP 500.
FeatureFlagsBackendService.fetchRemoteFeatureFlags()hits the\!response.isSuccessbranch and returnsRemoteFeatureFlagsResult.EMPTY(enabledKeys=[], metadata=null). - Back in
fetchAndApply:result.enabledKeys.toSet()=emptySet();current.sdkRemoteFeatureFlags.toSet()=setOf("sdk_background_threading"). Equality check isfalse— early return is not taken. current.setListProperty("sdkRemoteFeatureFlags", emptyList(), REMOTE_FEATURE_FLAGS)is called. SharedPreferences now stores an empty list forsdkRemoteFeatureFlags.FeatureManager.onModelUpdatedfires but theAPP_STARTUPguard keeps the current-session state unchanged.- App is killed and relaunched.
FeatureManager.initreadssdkRemoteFeatureFlags = []from SharedPreferences.SDK_BACKGROUND_THREADINGis initialized tofalse. The SDK starts in legacy threading mode, despite the feature being actively enabled on the server.
| override fun onModelReplaced( | ||
| model: ConfigModel, | ||
| tag: String, | ||
| ) { | ||
| Logging.debug("OneSignal: FeatureManager.onModelReplaced(tag=$tag)") | ||
| if (tag == ModelChangeTags.HYDRATE || tag == ModelChangeTags.NORMAL) { | ||
| if (tag == ModelChangeTags.HYDRATE || | ||
| tag == ModelChangeTags.NORMAL || | ||
| tag == ConfigModelChangeTags.REMOTE_FEATURE_FLAGS | ||
| ) { | ||
| try { | ||
| refreshEnabledFeatures(model, applyNextRunOnlyFeatures = false) | ||
| } catch (t: Throwable) { |
There was a problem hiding this comment.
🟡 The tag == ConfigModelChangeTags.REMOTE_FEATURE_FLAGS branch inside FeatureManager.onModelReplaced() is dead code that can never be reached. fetchAndApply() updates the model via current.setListProperty() and current.setOptStringProperty() directly on the live ConfigModel, which fire onModelUpdated callbacks — not onModelReplaced. Consider removing the REMOTE_FEATURE_FLAGS check from onModelReplaced() in both FeatureManager and FeatureFlagsRefreshService to avoid misleading future readers.
Extended reasoning...
What the dead code is and how it manifests
FeatureManager.onModelReplaced() was updated to process tag == ConfigModelChangeTags.REMOTE_FEATURE_FLAGS alongside HYDRATE and NORMAL. Similarly, FeatureFlagsRefreshService.onModelReplaced() has an early-return guard if (tag == ConfigModelChangeTags.REMOTE_FEATURE_FLAGS) return. Neither branch can ever be reached in practice.
The specific code path that shows why
FeatureFlagsRefreshService.fetchAndApply() updates the model at lines 113–116 via:
current.setListProperty(ConfigModel::sdkRemoteFeatureFlags.name, result.enabledKeys, tag)
current.setOptStringProperty(ConfigModel::sdkRemoteFeatureFlagMetadata.name, newMetaString, tag)Both calls operate on the live singleton ConfigModel reference. In Model.kt, setListProperty → setOptAnyProperty → notifyChanged() → fires ISingletonModelStoreChangeHandler.onModelUpdated() on all subscribers. The onModelReplaced() callback is only triggered by SingletonModelStore.replace(), which calls changeSubscription.fire { it.onModelReplaced(existingModel, tag) }. Since fetchAndApply() never calls configModelStore.replace(), the REMOTE_FEATURE_FLAGS tag can never arrive via onModelReplaced().
Why existing code doesn't prevent the confusion
The docstring on ConfigModelChangeTags.REMOTE_FEATURE_FLAGS correctly states it is for "ConfigModel / ConfigModelStore replace notifications," but the tag is also passed as an argument to the property setters (which fire onModelUpdated). A developer reading FeatureManager.onModelReplaced() would reasonably infer that there is a code path where the feature-flags refresh triggers model replacement — there is not. Features ARE correctly updated through the onModelUpdated path (lines 73–83 in FeatureManager) which checks for sdkRemoteFeatureFlags and sdkRemoteFeatureFlagMetadata property changes.
Addressing the refutation
The refutation correctly notes this is "harmless defensive code" with no functional regression. All three confirmers agree — the flags ARE correctly updated via onModelUpdated, and no behavior is broken. The concern is purely about code clarity: the dead REMOTE_FEATURE_FLAGS branch implies a model-replacement update path that does not exist, and a future maintainer might test only that path and miss the real one.
Impact
No functional regression. Feature flag states are always correctly reflected in FeatureManager.featureStates. The risk is purely maintenance: a developer adding a new FeatureActivationMode or changing the update flow might trace through onModelReplaced as the canonical handler and miss the property-setter path.
How to fix it
Remove the tag == ConfigModelChangeTags.REMOTE_FEATURE_FLAGS branch from FeatureManager.onModelReplaced() (since it is unreachable) and remove the early-return guard in FeatureFlagsRefreshService.onModelReplaced() for the same reason. If the intent is to document that REMOTE_FEATURE_FLAGS updates go through onModelUpdated, a code comment at the onModelUpdated call site would be clearer.
Step-by-step proof
FeatureFlagsRefreshService.fetchAndApply()retrievesval current = configModelStore.model— the live singleton.- It calls
current.setListProperty(..., ConfigModelChangeTags.REMOTE_FEATURE_FLAGS). - Inside
Model.setListProperty→setOptAnyProperty→notifyChanged(tag=REMOTE_FEATURE_FLAGS). notifyChangedfireschangeNotifierlisteners →SingletonModelStore.onModelUpdated(model, args, tag)→ISingletonModelStoreChangeHandler.onModelUpdated(args, tag).FeatureManager.onModelUpdatedis called withargs.property == "sdkRemoteFeatureFlags"— this is the real update path.SingletonModelStore.replace()is never called, soonModelReplaced()withREMOTE_FEATURE_FLAGSis never triggered.
| try { | ||
| fetchAndApply(appId) | ||
| } catch (t: Throwable) { | ||
| Logging.warn("FeatureFlagsRefreshService: fetch failed", t) | ||
| } | ||
| } | ||
| delay(REFRESH_INTERVAL_MS) | ||
| } |
There was a problem hiding this comment.
🟡 The inner catch (t: Throwable) in restartForegroundPolling() catches CancellationException (which extends Throwable on the JVM), so every normal pollJob.cancel() call on app unfocus that interrupts an in-flight HTTP request inside fetchAndApply() emits a spurious Logging.warn warning. Fix by changing catch (t: Throwable) to catch (t: Exception), or by adding if (t is CancellationException) throw t as the first line of the catch body.
Extended reasoning...
What the bug is and how it manifests
In FeatureFlagsRefreshService.restartForegroundPolling() (lines 93-100 of the new file), the polling loop wraps fetchAndApply(appId) in a try/catch (t: Throwable) block that logs Logging.warn("FeatureFlagsRefreshService: fetch failed", t). On the JVM, CancellationException extends IllegalStateException -> RuntimeException -> Exception -> Throwable, so it is caught by catch (t: Throwable). This means any coroutine cancellation that occurs while the coroutine is suspended inside fetchAndApply() will be caught and logged as a warning rather than propagated.
The specific code path that triggers it
When the user moves the app to the background, onUnfocused() calls pollJob?.cancel(). If the coroutine is suspended at the http.get() call inside featureFlagsBackend.fetchRemoteFeatureFlags(appId) (a suspend fun), the runtime injects a CancellationException at that suspension point. The exception propagates up through fetchAndApply() and is caught by the catch (t: Throwable) handler in restartForegroundPolling(), triggering Logging.warn("FeatureFlagsRefreshService: fetch failed", t). After the catch block completes, the delay(REFRESH_INTERVAL_MS) call (which sits outside the try/catch) also throws CancellationException, this time exiting the while loop normally.
Why existing code does not prevent it
The coroutine is correctly launched via OneSignalDispatchers.launchOnIO (which has no outer catch), so the outer launcher does not swallow cancellation. However, the inner catch (t: Throwable) is too broad: it was intended to catch transient network or parsing errors from the HTTP fetch, but Throwable also encompasses CancellationException. There is no if (t is CancellationException) throw t guard, nor is the catch narrowed to Exception.
What the impact would be
Every app unfocus event during which an HTTP call to the feature-flags endpoint is in-flight generates a spurious WARNING-level log entry. In production log-aggregation or monitoring systems, repeated false-alarm warnings inflate error rates and can drown out genuine issues. The coroutine terminates correctly (via the subsequent delay() outside the try/catch), so there is no functional regression — only polluted warning logs on each focus/unfocus cycle.
How to fix it
Change the catch clause from Throwable to Exception:
Alternatively, add an explicit rethrow guard as the first line of the catch body:
Both approaches ensure CancellationException is rethrown and the coroutine exits cleanly without a spurious log entry.
Step-by-step proof
- App is in the foreground; restartForegroundPolling() launches a coroutine on the IO dispatcher. The coroutine calls fetchAndApply(appId), which calls featureFlagsBackend.fetchRemoteFeatureFlags(appId), which suspends inside http.get() waiting for an HTTP response.
- User presses the home button; onUnfocused() is invoked. It calls pollJob?.cancel().
- The coroutine runtime delivers a CancellationException to the active suspension point inside http.get(). The exception propagates up through fetchRemoteFeatureFlags and fetchAndApply (no inner catch there).
- The exception is caught by catch (t: Throwable) in restartForegroundPolling(). Logging.warn("FeatureFlagsRefreshService: fetch failed", t) is emitted — the spurious warning.
- Execution continues past the catch block and reaches delay(REFRESH_INTERVAL_MS). Since the job is still cancelled, delay() immediately throws another CancellationException. This one is not caught (it is outside the try/catch), so it propagates out of the while loop and the coroutine terminates — but only after the false-alarm warning was already emitted in step 4.
🔬 also observed by 3081204109
| kotlinVersion = '1.9.25' | ||
| dokkaVersion = '1.9.10' | ||
| coroutinesVersion = '1.7.3' | ||
| kotlinxSerializationJsonVersion = '1.6.3' |
There was a problem hiding this comment.
do we need this change?
There was a problem hiding this comment.
yes, so i shifted away from using org.json.JsonObject to using kotlinxSerializationJson. This will help us in KMP as it needs to be pure kotlin.
Summary
Implements Turbine-aligned remote SDK feature flags for Android: GET
/apps/:app_id/sdk/features/android/:sdk_versionwith validated SDK version label, kotlinx.serialization parsing (including optional sibling metadata per flag), foreground refresh (~10 min), andFeatureManagerintegration withremoteFeatureFlagMetadata().Linear
SDK-4363
Test plan