Skip to content

feat: SDK-4363: Turbine remote SDK feature flags and foreground refresh#2612

Open
abdulraqeeb33 wants to merge 14 commits intomainfrom
sdk-4363-android-sdk-turbine-feature-flags
Open

feat: SDK-4363: Turbine remote SDK feature flags and foreground refresh#2612
abdulraqeeb33 wants to merge 14 commits intomainfrom
sdk-4363-android-sdk-turbine-feature-flags

Conversation

@abdulraqeeb33
Copy link
Copy Markdown
Contributor

@abdulraqeeb33 abdulraqeeb33 commented Apr 14, 2026

Summary

Implements Turbine-aligned remote SDK feature flags for Android. The SDK now fetches a per-app feature-flag document from Turbine while in the foreground and applies the result through FeatureManager, with per-flag activation modes so threading-sensitive flags only flip at cold start.

Linear

SDK-4363

What's in the PR

Backend client

  • IFeatureFlagsBackendService + sealed RemoteFeatureFlagsFetchOutcome (Success / Unavailable) so callers don't have to interpret HTTP status codes.
  • FeatureFlagsBackendServiceGET /apps/:app_id/sdk/features/android/:sdk_version:
    • 4xx → Unavailable, logged at WARN with a 200-char single-line body snippet (Turbine errors like 403/404 are obvious in logcat without flipping log level).
    • 5xx / network (statusCode=0) → Unavailable, logged at DEBUG to keep transient failures quiet.
    • 200 with non-contract JSON → Unavailable, logged at WARN (surfaces real contract regressions).
  • TurbineSdkFeatureFlagsPath — validates and encodes the SDK version path segment (allow-lists [A-Za-z0-9._-], rejects empties / path-traversal).
  • HttpResponse.isClientError — narrows !isSuccess to 4xx so the service can log 4xx at WARN and 5xx/network at DEBUG without reimplementing status-code ranges at each call site.

JSON parsing

  • FeatureFlagsJsonParser (kotlinx.serialization):
    • Parses { "features": ["flag", { "id": "flag", ...metadata }] } — both string and object forms per flag.
    • Trims + deduplicates keys, preserves first-seen order.
    • Encodes per-flag metadata objects into a compact JSON string stored on ConfigModel.sdkRemoteFeatureFlagMetadata; exposes a decode helper that returns Map<String, JsonObject> for consumers.
    • Tolerates missing features, wrong root type, and malformed entries (drops them, keeps valid ones).

Config + foreground refresh

  • ConfigModel gains sdkRemoteFeatureFlags: List<String> and sdkRemoteFeatureFlagMetadata: String?.
  • ConfigModelChangeTags.REMOTE_FEATURE_FLAGS scopes in-place writes so a stale full-model hydrate can't overwrite a fresh Turbine result.
  • FeatureFlagsRefreshService (IStartableService, IApplicationLifecycleHandler, ISingletonModelStoreChangeHandler<ConfigModel>):
    • Kicks off a fetch immediately on onFocus (fires on subscribe), then every 8 minutes while foregrounded.
    • Cancels the poll loop on onUnfocused and on app-id changes; re-arms when ConfigModel is rehydrated / replaced.
    • Coroutine-cancellation safe (re-throws CancellationException, swallows everything else with a WARN).

FeatureManager integration

  • FeatureFlag enum entries now carry a FeatureActivationMode:
    • IMMEDIATE — in-memory state + side effects applied as soon as remote config changes.
    • APP_STARTUP — value is persisted to ConfigModel the moment it arrives, but the in-memory featureStates[feature] and side effects are not flipped mid-run; the new value is latched on the next cold start. An INFO breadcrumb records the pending change.
  • SDK_BACKGROUND_THREADING is wired as APP_STARTUP so ThreadingMode can't swap mid-session.
  • IFeatureManager.remoteFeatureFlagMetadata() exposes the decoded Map<String, JsonObject> for downstream consumers (e.g. nested Turbine config blocks).
  • FeatureManager.init reads the cached ConfigModel with applyNextRunOnlyFeatures = true, so any value written during the previous run is activated before the rest of the SDK starts consuming feature state.

DI / wiring

  • CoreModule registers FeatureFlagsBackendService and FeatureFlagsRefreshService.
  • build.gradle picks up kotlinx-serialization-json for the parser.

Test results

Run locally with ./gradlew :OneSignal:core:testOriginalUnitTest — all green:

Suite Tests
FeatureFlagsBackendServiceTest 4 / 4
FeatureFlagsBackendServiceTests (logging + outcome behavior) 8 / 8
FeatureFlagsJsonParserTests 13 / 13
TurbineSdkFeatureFlagsPathTest 3 / 3
FeatureManagerTests (4 new covering remote flags + metadata, 4 preexisting) 8 / 8
FeatureFlagTests 2 / 2

Notable coverage:

  • 4xx / 5xx / statusCode=0 / 200-with-bad-JSON all route to Unavailable with the right log level and body snippet; snippet capped at 200 chars; newlines/CRs flattened so logcat stays single-line.
  • JSON parser handles mixed string + object entries, deduplicates, trims whitespace, drops malformed entries, round-trips metadata through the encode/decode pair.
  • FeatureManager honors APP_STARTUP (latches current run, persists new value for next run) and IMMEDIATE (applies in place); remoteFeatureFlagMetadata() returns the decoded map or null when unset.
  • Turbine path segment rejects empty / traversal / illegal characters; encoded identically for valid labels.

Test plan

  • CI passes (currently failing on spotlessKotlinCheck — needs ./gradlew :OneSignal:core:spotlessApply for import ordering in FeatureFlagsRefreshService.kt / FeatureManagerTests.kt and brace indent in FeatureManager.kt).
  • Unit tests for backend service, JSON parser, refresh path, and FeatureManager.
  • Manual verification: cold start with a Turbine-enabled app id returns flags and FeatureManager.isEnabled(...) reflects them on the next run; mid-session flips for APP_STARTUP flags are persisted but not applied until the next process start.

AR Abdul Azeez added 2 commits April 9, 2026 13:37
- 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
Copilot AI review requested due to automatic review settings April 14, 2026 15:59
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 ConfigModel fields 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.

@nan-li
Copy link
Copy Markdown
Contributor

nan-li commented Apr 14, 2026

@claude review

- 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
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 into ConfigModel.
  • 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.

@abdulraqeeb33 abdulraqeeb33 changed the title SDK-4363: Turbine remote SDK feature flags and foreground refresh feat: SDK-4363: Turbine remote SDK feature flags and foreground refresh Apr 14, 2026
Comment thread OneSignalSDK/build.gradle
AR Abdul Azeez added 3 commits April 20, 2026 14:29
…y casing

- RemoteFeatureFlagsFetchOutcome Success/Unavailable; parseSuccessful distinguishes valid
  Turbine JSON (including empty features) from parse/contract failure; no cache wipe on Unavailable
- Polling loop: rethrow CancellationException; catch Exception only for fetch errors
- FeatureManager: drop dead REMOTE_FEATURE_FLAGS onModelReplaced branch; canonicalize flag keys
- ConfigModelChangeTags + FeatureFlag KDoc/comment fixes
- Tests: parseSuccessful, case-insensitive remote key

Made-with: Cursor
Keep the "next fetch in …ms" and before/after diff debug logs in
FeatureFlagsRefreshService, and keep mavenLocal() in the demo's
allprojects repositories; just remove the TODO(revert-before-merge)
markers now that these are staying.

Made-with: Cursor
Comment thread examples/demo/build.gradle.kts Outdated

/**
* Feature keys from the dedicated SDK feature-flags HTTP endpoint (see [com.onesignal.core.internal.backend.IFeatureFlagsBackendService]).
* Unioned with [features] for [com.onesignal.core.internal.features.FeatureManager].
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.

what are we doing with features, do we need to maintain both properties

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes we do, features has logging level and we cant get rid of it as there SDK versions that are already out there. We could consider moving to this latest way but once we have this settled.

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.

Does features include logging? I don't see it on ConfigModel in 5.7.7, I don't think we shipped features property on ConfigModel yet: c779475

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nan-li you are right. we dont need features. Logging was RemoteLoggingParams

Comment thread OneSignalSDK/onesignal/core/build.gradle
@nan-li nan-li force-pushed the sdk-4363-android-sdk-turbine-feature-flags branch from 0698aea to 441c409 Compare April 24, 2026 18:30
@nan-li nan-li force-pushed the sdk-4363-android-sdk-turbine-feature-flags branch from 441c409 to 68f2df3 Compare April 24, 2026 18:37
Comment on lines 69 to 77
args: ModelChangedArgs,
tag: String,
) {
if (args.property == ConfigModel::features.name) {
if (args.property == ConfigModel::sdkRemoteFeatureFlags.name ||
args.property == ConfigModel::sdkRemoteFeatureFlagMetadata.name
) {
Logging.debug("OneSignal: FeatureManager.onModelUpdated(property=${args.property}, tag=$tag)")
try {
refreshEnabledFeatures(configModelStore.model, applyNextRunOnlyFeatures = false)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 FeatureFlagsRefreshService.fetchAndApply writes both sdkRemoteFeatureFlags and sdkRemoteFeatureFlagMetadata as separate property setters (lines 129–130), and FeatureManager.onModelUpdated matches on either property name (lines 72–74). When both values change together — the common case on a successful poll — refreshEnabledFeatures runs twice back-to-back for a single fetch result, and ModelStore.persist / JSON serialization also happens twice. Second run is idempotent so there's no correctness issue, just 2× per-poll overhead; the simplest fix is to drop the sdkRemoteFeatureFlagMetadata check from onModelUpdated since metadata is pulled on demand via remoteFeatureFlagMetadata() and never affects enabled-state, or coalesce both writes under a single notifyChanged.

Extended reasoning...

What this is

FeatureFlagsRefreshService.fetchAndApply (FeatureFlagsRefreshService.kt:129-130) performs two sequential in-place writes on the live ConfigModel:

current.setListProperty(ConfigModel::sdkRemoteFeatureFlags.name, result.enabledKeys, tag)
current.setOptStringProperty(ConfigModel::sdkRemoteFeatureFlagMetadata.name, newMetaString, tag)

Both setters route through Model.setOptAnyPropertynotifyChangedSingletonModelStore.onModelUpdatedISingletonModelStoreChangeHandler.onModelUpdated. FeatureManager.onModelUpdated (FeatureManager.kt:72-74) matches both property names with an OR:

if (args.property == ConfigModel::sdkRemoteFeatureFlags.name ||
    args.property == ConfigModel::sdkRemoteFeatureFlagMetadata.name
) { ... refreshEnabledFeatures(...) }

So when both values actually change (first successful poll, or any poll where both keys and metadata shift), refreshEnabledFeatures is invoked twice in rapid succession for the same fetch result.

Why the second run is pure waste

refreshEnabledFeatures (FeatureManager.kt:85-126) only reads model.sdkRemoteFeatureFlags and localFeatureOverrides when computing enabledFeatureKeys — metadata is never consulted in enabled-state computation. The APP_STARTUP branch additionally guards against changing featureStates after hasBeenInitialized, so the second invocation produces the identical set and performs a no-op copy-back. ThreadingMode.updateUseBackgroundThreading with the same value is also idempotent.

Additionally, each property setter triggers its own ModelStore.onChangedpersist() path, which JSON-serializes the full model list twice and enqueues two saveString calls to PreferencesService (the actual SharedPreferences flush is coalesced by the async worker, but the serialization and queue churn happen twice).

Impact

Not a correctness bug — all verifiers agreed this is nit severity. The user-visible effect is duplicated work per successful foreground poll: 2× listener fan-out, 2× JSON serialization, 2× feature-state recomputation including enum iteration over FeatureFlag.entries. On the default 8-minute poll cadence this is negligible per session but wholly avoidable.

Step-by-step proof

  1. App is foregrounded; FeatureFlagsRefreshService.restartForegroundPolling launches the polling coroutine and it calls fetchAndApply(appId).
  2. featureFlagsBackend.fetchRemoteFeatureFlags(appId) returns Success(RemoteFeatureFlagsResult(enabledKeys=["sdk_background_threading"], metadata={...})).
  3. The equality guard at line 124 sees both afterKeys != beforeKeys and newMetaString != current.sdkRemoteFeatureFlagMetadata, so execution proceeds past the early return.
  4. Line 129: current.setListProperty("sdkRemoteFeatureFlags", [...], REMOTE_FEATURE_FLAGS). Inside Model.setOptAnyProperty, oldValue != value, so notifyChanged fires. SingletonModelStore forwards to FeatureManager.onModelUpdated(args.property="sdkRemoteFeatureFlags", ...). The OR matches → refreshEnabledFeatures runs (call gradle? #1). Separately, ModelStore.onChanged fires persist() → JSON-serialize + saveString (gradle? #1).
  5. Line 130: current.setOptStringProperty("sdkRemoteFeatureFlagMetadata", "{...}", REMOTE_FEATURE_FLAGS). notifyChanged fires again. FeatureManager.onModelUpdated(args.property="sdkRemoteFeatureFlagMetadata", ...). The OR matches → refreshEnabledFeatures runs (call Added possibility to send status 'opened' for a message back to onesignal backend from client source code #2) — recomputes the identical union, iterates all FeatureFlag.entries, invokes applySideEffects again. persist() fires again → JSON-serialize + saveString (Added possibility to send status 'opened' for a message back to onesignal backend from client source code #2).

Fix

Either is sufficient:

  • Drop the sdkRemoteFeatureFlagMetadata branch from onModelUpdated — metadata never affects enabled-state, and remoteFeatureFlagMetadata() reads the model on demand, so no listener is needed.
  • Or coalesce both writes under a single notifyChanged fire in fetchAndApply.

Comment on lines +121 to +126
val newMetaString = FeatureFlagsJsonParser.encodeMetadata(result.metadata)
val beforeKeys = current.sdkRemoteFeatureFlags.toSet()
val afterKeys = result.enabledKeys.toSet()
if (afterKeys == beforeKeys && newMetaString == current.sdkRemoteFeatureFlagMetadata) {
return
}
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 metadata short-circuit in fetchAndApply uses raw-string equality (newMetaString == current.sdkRemoteFeatureFlagMetadata), but the encoded metadata is built in backend-array order via buildJsonObject and kotlinx.serialization preserves JsonObject insertion order — so the same flags+metadata in a different order across polls produce byte-different strings, the guard fails, and setListProperty/setOptStringProperty fire every poll. The keys comparison was already fixed via toSet(); apply the same treatment to metadata by parsing both sides as JsonObject (Map equality is order-insensitive) or by canonicalizing the encoded string. Nit only: no functional regression because FeatureManager re-derives state from a Set, just wasted SharedPreferences writes and onModelUpdated notifications per poll cycle.

Extended reasoning...

What the bug is

In FeatureFlagsRefreshService.fetchAndApply (lines 121–126), the early-exit guard is:

val beforeKeys = current.sdkRemoteFeatureFlags.toSet()
val afterKeys = result.enabledKeys.toSet()
if (afterKeys == beforeKeys && newMetaString == current.sdkRemoteFeatureFlagMetadata) {
    return
}

Keys are compared as Set<String> (order-insensitive — this was the fix from the earlier review), but newMetaString is compared via plain String.equals, which is byte-for-byte and therefore order-sensitive. The metadata field is the encoded form of a JsonObject whose key order is not normalized.

The specific code path that triggers it

FeatureFlagsJsonParser.parseRootStrict (lines 82–88) builds the metadata JsonObject via buildJsonObject iterating flagEntries in the order produced by featuresArray.mapNotNull { … }.distinctBy { it.second } — i.e. backend array order. findSiblingJsonObject returns the inner JsonObject straight from the parsed root, preserving the server's key order within each flag. encodeMetadata serializes via format.encodeToString(JsonElement.serializer(), it); kotlinx.serialization's JsonObject is backed by a LinkedHashMap and emits keys in insertion order, so the encoded string is deterministic per insertion order but not normalized.

Why existing code does not prevent it

There is no canonicalization on either side of the comparison: the parser does not sort outer or inner keys, and the storage layer just round-trips the same string. The earlier fix added .toSet() for keys but did not extend it to metadata.

Impact

When the backend returns the same flags+metadata in a different order across polls, two things happen on every 8-minute poll cycle: (1) setListProperty is called because the encoded metadata differs (see step 5 below — List equality is order-sensitive too, but that's separate); (2) setOptStringProperty writes the new metadata string. Both fire notifyChangedFeatureManager.onModelUpdated and the ModelStore.persist SharedPreferences write. There is no functional regression because FeatureManager.refreshEnabledFeatures canonicalizes via a Set of lowercase keys, so the effective feature state is unchanged — this is purely wasted I/O and listener work. Hence nit severity.

How to fix it

Either parse both sides as JsonObject and compare (Map equality is order-insensitive in Kotlin), or normalize the encoded form (e.g. recursively sort keys before encoding). The simplest spot-fix mirrors the pattern already applied to keys:

val newMeta = result.metadata
val currentMeta = FeatureFlagsJsonParser.parseStoredMetadataMap(current.sdkRemoteFeatureFlagMetadata)
        .let { if (it.isEmpty()) null else JsonObject(it) }
if (afterKeys == beforeKeys && newMeta == currentMeta) return

Step-by-step proof

  1. Poll N: backend returns {"features":["a","b"],"a":{"weight":1},"b":{"enabled":true}}. Parser produces metadata = {"a":{"weight":1},"b":{"enabled":true}} and encodeMetadata yields {"a":{"weight":1},"b":{"enabled":true}}. Stored in current.sdkRemoteFeatureFlagMetadata.
  2. Poll N+1: backend returns the same content but reordered: {"features":["b","a"],"b":{"enabled":true},"a":{"weight":1}}.
  3. Parser: flagEntries is [("b","b"),("a","a")] (server order). buildJsonObject inserts b then a. Encoded metadata is now {"b":{"enabled":true},"a":{"weight":1}}.
  4. afterKeys.toSet() == beforeKeys.toSet()true (keys guard passes). But newMetaString == current.sdkRemoteFeatureFlagMetadatafalse (different byte sequence).
  5. Guard does not return. setListProperty is invoked with a reordered List (List.equals is order-sensitive, so Model.setOptAnyProperty treats it as a change and calls notifyChanged). setOptStringProperty is invoked with the new metadata string and also calls notifyChanged.
  6. FeatureManager.onModelUpdated fires twice; ModelStore.persist writes to SharedPreferences. Effective feature state in FeatureManager.featureStates is unchanged because refreshEnabledFeatures canonicalizes to a lowercase Set — but the I/O and listener work is wasted on every poll cycle where the backend reorders.

The same scenario also applies to inner-object key reordering ({"weight":1,"enabled":true} vs {"enabled":true,"weight":1}), since findSiblingJsonObject returns the inner JsonObject unchanged from the parsed root.

🔬 also observed by copilot-pull-request-reviewer

AR Abdul Azeez added 2 commits April 27, 2026 14:07
…ract-violating responses

ConfigModelStoreListener.fetchParams now re-sources sdkRemoteFeatureFlags{,Metadata}
from the live model immediately before replace(), so concurrent in-place writes
from FeatureFlagsRefreshService survive Model.initializeFromModel's data.clear()+putAll().

FeatureFlagsJsonParser.parseRootStrict now distinguishes an authoritative empty
features array from one that filtered down to empty due to element-type violations;
the latter returns null -> Unavailable so callers preserve the cached list rather
than overwriting it with [].

Adds regression tests for both paths. Addresses PR #2612 review comments.

Made-with: Cursor
`ConfigModelStoreListener.fetchParams` replaces the live `ConfigModel`
with the HYDRATE tag during init, which fired `onModelReplaced` on
`FeatureFlagsRefreshService` and triggered a second Turbine GET right
after the focus-driven first one, even though the appId hadn't changed.

Track the appId currently being polled and treat a re-trigger for the
same appId as a no-op; genuine appId changes still cancel and restart.
`onUnfocused` clears the cached appId so the next focus event re-arms
polling.

Pull the foreground cadence off the constructor and onto an
`internal var refreshIntervalMs` (default 8 min, test-only override
before `start`). The IoC reflection in
`ServiceRegistrationReflection.resolve` cannot resolve a non-service
`Long` parameter and was throwing
"Could not instantiate service: ServiceRegistrationReflection@..."
out of `StartupService.scheduleStart` once the service was registered.

Made-with: Cursor
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.

5 participants