Merge pull request #852 from qonversion/kamo/dev-1232-android-sdk-zalipanie-issendingscheduled-posle-force-flasha

fix: unstick property sending flag + invalidate named-key RC cache on attach/detach (DEV-1232)

Author: SpertsyanKM

sdk/src/main/java/com/qonversion/android/sdk/internal/QRemoteConfigManager.kt

@@ -44,6 +44,12 @@ internal class QRemoteConfigManager @Inject constructor(
     lateinit var userPropertiesManager: QUserPropertiesManager
     private val mainHandler = Handler(Looper.getMainLooper())
 
+    // Bumped on every cache invalidation (attach/detach, user change). Loads
+    // capture it when they start and skip the cache write if it moved — an
+    // in-flight response evaluated before the invalidating event must not be
+    // re-cached as fresh. Main-thread confined like the rest of the state.
+    private var invalidationGeneration = 0
+
     fun handlePendingRequests() = postToMainThread {
         loadingStates.filter { it.value.callbacks.isNotEmpty() }
             .keys.forEach { contextKey -> loadRemoteConfig(contextKey, null) }
@@ -74,6 +80,7 @@ internal class QRemoteConfigManager @Inject constructor(
     }
 
     fun onUserUpdate() = postToMainThread {
+        invalidationGeneration++
         loadingStates = mutableMapOf()
     }
 
@@ -99,12 +106,15 @@ internal class QRemoteConfigManager @Inject constructor(
 
         loadingState.isInProgress = true
         loadingState.loadedConfig = null
+        val generationAtStart = invalidationGeneration
 
         userPropertiesManager.forceSendProperties(object : QonversionEmptyCallback {
             override fun onComplete() {
                 remoteConfigService.loadRemoteConfig(contextKey, object : QonversionRemoteConfigCallback {
                     override fun onSuccess(remoteConfig: QRemoteConfig) {
-                        loadingState.loadedConfig = remoteConfig
+                        if (invalidationGeneration == generationAtStart) {
+                            loadingState.loadedConfig = remoteConfig
+                        }
                         fireToCallbacks(contextKey) { onSuccess(remoteConfig) }
                     }
 
@@ -176,31 +186,40 @@ internal class QRemoteConfigManager @Inject constructor(
     }
 
     fun attachUserToExperiment(experimentId: String, groupId: String, callback: QonversionExperimentAttachCallback) = postToMainThread {
-        loadingStates[EmptyContextKey]?.loadedConfig = null
+        invalidateLoadedConfigs()
         remoteConfigService.attachUserToExperiment(experimentId, groupId, callback)
     }
 
     fun detachUserFromExperiment(experimentId: String, callback: QonversionExperimentAttachCallback) = postToMainThread {
-        loadingStates[EmptyContextKey]?.loadedConfig = null
+        invalidateLoadedConfigs()
         remoteConfigService.detachUserFromExperiment(experimentId, callback)
     }
 
     fun attachUserToRemoteConfiguration(
         remoteConfigurationId: String,
         callback: QonversionRemoteConfigurationAttachCallback
     ) = postToMainThread {
-        loadingStates[EmptyContextKey]?.loadedConfig = null
+        invalidateLoadedConfigs()
         remoteConfigService.attachUserToRemoteConfiguration(remoteConfigurationId, callback)
     }
 
     fun detachUserFromRemoteConfiguration(
         remoteConfigurationId: String,
         callback: QonversionRemoteConfigurationAttachCallback
     ) = postToMainThread {
-        loadingStates[EmptyContextKey]?.loadedConfig = null
+        invalidateLoadedConfigs()
         remoteConfigService.detachUserFromRemoteConfiguration(remoteConfigurationId, callback)
     }
 
+    // An attach/detach is addressed by experiment/configuration id, and the SDK does not
+    // know which context key that entity serves — drop every cached config, not just the
+    // empty-key one, or configs under named context keys stay stale until process restart.
+    // The generation bump also stops in-flight loads from re-caching a pre-attach response.
+    private fun invalidateLoadedConfigs() {
+        invalidationGeneration++
+        loadingStates.values.forEach { it.loadedConfig = null }
+    }
+
     private fun getRemoteConfigListCallbackWrapper(
         contextKeys: List<String>?,
         includeEmptyContextKey: Boolean,
@@ -209,13 +228,16 @@ internal class QRemoteConfigManager @Inject constructor(
         // Remembering loading states for the case of user change -
         // if it happens, we won't store remote configs for different user.
         val localLoadingStates = loadingStates
+        val generationAtStart = invalidationGeneration
         return object : QonversionRemoteConfigListCallback {
             override fun onSuccess(remoteConfigList: QRemoteConfigList) {
-                remoteConfigList.remoteConfigs.forEach { remoteConfig ->
-                    val contextKey = remoteConfig.source.contextKey
-                    val loadingState = localLoadingStates[contextKey] ?: LoadingState()
-                    loadingState.loadedConfig = remoteConfig
-                    localLoadingStates[contextKey] = loadingState
+                if (invalidationGeneration == generationAtStart) {
+                    remoteConfigList.remoteConfigs.forEach { remoteConfig ->
+                        val contextKey = remoteConfig.source.contextKey
+                        val loadingState = localLoadingStates[contextKey] ?: LoadingState()
+                        loadingState.loadedConfig = remoteConfig
+                        localLoadingStates[contextKey] = loadingState
+                    }
                 }
 
                 callback.onSuccess(remoteConfigList)

sdk/src/main/java/com/qonversion/android/sdk/internal/QUserPropertiesManager.kt

@@ -30,6 +30,11 @@ internal class QUserPropertiesManager @Inject internal constructor(
     internal var productCenterManager: QProductCenterManager? = null
     private var handler: Handler? = null
     private var isRequestInProgress = false
+
+    // Semantics: "a send is known to be scheduled or will be re-scheduled".
+    // false while a delayed job is still pending is a tolerated state (it only
+    // causes an extra no-op wakeup, never a missed send) — do not "fix" it by
+    // resetting the flag exclusively from the scheduled job.
     private var isSendingScheduled = false
     private var retryDelay = PROPERTY_UPLOAD_MIN_DELAY
     private var retriesCounter = 0
@@ -72,6 +77,11 @@ internal class QUserPropertiesManager @Inject internal constructor(
 
     public fun forceSendProperties(callback: QonversionEmptyCallback? = null) {
         if (isRequestInProgress) {
+            // The scheduled job that lands here is consumed without sending.
+            // Drop the flag so subsequent setters can schedule a new send —
+            // otherwise properties set during an in-flight request hang until
+            // the next background/RC trigger.
+            isSendingScheduled = false
             if (callback != null) {
                 completions.add(callback)
             }
@@ -101,6 +111,15 @@ internal class QUserPropertiesManager @Inject internal constructor(
 
                     // Cleaning all the properties (not only succeeded) as we don't want to resend invalid ones again
                     propertiesStorage.clear(properties)
+
+                    // Properties stored or overwritten while the request was in
+                    // flight are not part of the sent snapshot — schedule a
+                    // follow-up send for them (the clear above is
+                    // value-conditional, so overwrites survive it). In background
+                    // the schedule is skipped and onAppForeground picks them up.
+                    if (propertiesStorage.getProperties().isNotEmpty() && !isSendingScheduled) {
+                        sendPropertiesWithDelay(retryDelay)
+                    }
                 },
                 onError = {
                     fireCallbacks()
@@ -124,6 +143,8 @@ internal class QUserPropertiesManager @Inject internal constructor(
                     }
                 })
         } else {
+            // Nothing to send — the pending schedule (if any) is satisfied.
+            isSendingScheduled = false
             callback?.onComplete()
         }
     }

sdk/src/main/java/com/qonversion/android/sdk/internal/storage/UserPropertiesStorage.kt

@@ -3,16 +3,20 @@ package com.qonversion.android.sdk.internal.storage
 import java.util.concurrent.ConcurrentHashMap
 
 internal class UserPropertiesStorage : PropertiesStorage {
-    private val userProperties: MutableMap<String, String> =
+    private val userProperties: ConcurrentHashMap<String, String> =
         ConcurrentHashMap()
 
     override fun save(key: String, value: String) {
         userProperties[key] = value
     }
 
     override fun clear(properties: Map<String, String>) {
-        properties.keys.map {
-            userProperties.remove(it)
+        // Value-conditional removal: a key overwritten while its previous value
+        // was in flight must survive the post-send cleanup, or the new value
+        // would be silently lost. Unchanged values are still removed so invalid
+        // ones are not resent.
+        properties.forEach { (key, value) ->
+            userProperties.remove(key, value)
         }
     }
 

sdk/src/test/java/com/qonversion/android/sdk/internal/QRemoteConfigManagerTest.kt

@@ -12,9 +12,14 @@ import com.qonversion.android.sdk.internal.services.QFallbacksService
 import com.qonversion.android.sdk.internal.services.QRemoteConfigService
 import com.qonversion.android.sdk.listeners.QonversionRemoteConfigCallback
 import com.qonversion.android.sdk.listeners.QonversionRemoteConfigListCallback
+import com.qonversion.android.sdk.listeners.QonversionEmptyCallback
 import io.mockk.Called
 import io.mockk.clearAllMocks
+import io.mockk.every
+import io.mockk.just
 import io.mockk.mockk
+import io.mockk.runs
+import io.mockk.slot
 import io.mockk.verify
 import org.junit.Assert.assertEquals
 import org.junit.Assert.assertTrue
@@ -256,6 +261,95 @@ internal class QRemoteConfigManagerTest {
         verify { mockRemoteConfigService wasNot Called }
     }
 
+    @Test
+    fun `every attach and detach entry point invalidates named-key cached configs`() {
+        // given - all four entry points are addressed by entity id only, so the
+        // SDK cannot know which context key is served and must drop every cache
+        userStateProvider.stable = true
+        val entryPoints = listOf<Pair<String, (QRemoteConfigManager) -> Unit>>(
+            "attachUserToRemoteConfiguration" to { it.attachUserToRemoteConfiguration("config_id", mockk(relaxed = true)) },
+            "detachUserFromRemoteConfiguration" to { it.detachUserFromRemoteConfiguration("config_id", mockk(relaxed = true)) },
+            "attachUserToExperiment" to { it.attachUserToExperiment("experiment_id", "group_id", mockk(relaxed = true)) },
+            "detachUserFromExperiment" to { it.detachUserFromExperiment("experiment_id", mockk(relaxed = true)) },
+        )
+
+        entryPoints.forEach { (name, entryPoint) ->
+            // given - cached configs under the empty AND a named context key,
+            // plus a pending callback that must survive the invalidation
+            val pendingCallback = mockk<QonversionRemoteConfigCallback>(relaxed = true)
+            loadingStates()[null] = QRemoteConfigManager.LoadingState(loadedConfig = mockk<QRemoteConfig>(relaxed = true))
+            loadingStates()["ctx"] = QRemoteConfigManager.LoadingState(
+                loadedConfig = mockk<QRemoteConfig>(relaxed = true),
+                callbacks = mutableListOf(pendingCallback),
+            )
+
+            // when
+            entryPoint(manager)
+            shadowOf(Looper.getMainLooper()).idle()
+
+            // then - every cached config is dropped, but loading states and
+            // their pending callbacks are preserved (non-destructive invalidation)
+            assertEquals("$name must drop the empty-key config", null, loadingStates()[null]?.loadedConfig)
+            assertEquals("$name must drop the named-key config", null, loadingStates()["ctx"]?.loadedConfig)
+            assertEquals("$name must preserve pending callbacks", 1, loadingStates()["ctx"]?.callbacks?.size)
+        }
+    }
+
+    @Test
+    fun `attach invalidation prevents an in-flight load from re-caching a stale config`() {
+        // given - a load is in flight when the attach lands
+        userStateProvider.stable = true
+        val loadCallback = mockk<QonversionRemoteConfigCallback>(relaxed = true)
+        val serviceCallback = slot<QonversionRemoteConfigCallback>()
+        every { mockRemoteConfigService.loadRemoteConfig("ctx", capture(serviceCallback)) } just runs
+        every { mockUserPropertiesManager.forceSendProperties(any()) } answers {
+            firstArg<QonversionEmptyCallback?>()?.onComplete()
+        }
+
+        manager.loadRemoteConfig("ctx", loadCallback)
+        shadowOf(Looper.getMainLooper()).idle()
+
+        // when - the attach invalidates mid-flight, then the pre-attach response lands
+        manager.attachUserToRemoteConfiguration("config_id", mockk(relaxed = true))
+        shadowOf(Looper.getMainLooper()).idle()
+        val staleConfig = mockk<QRemoteConfig>(relaxed = true)
+        serviceCallback.captured.onSuccess(staleConfig)
+
+        // then - the stale (pre-attach) evaluation must not be re-cached, but
+        // the response is still DELIVERED to the waiting caller and the state
+        // is left refetchable (the guard skips only the cache write)
+        assertEquals(null, loadingStates()["ctx"]?.loadedConfig)
+        verify(exactly = 1) { loadCallback.onSuccess(staleConfig) }
+        assertEquals(false, loadingStates()["ctx"]?.isInProgress)
+    }
+
+    @Test
+    fun `attach invalidation prevents an in-flight list load from re-caching stale configs`() {
+        // given - a list load is in flight when the attach lands (the map is
+        // NOT replaced on attach, so the generation guard is the only barrier)
+        userStateProvider.stable = true
+        val listServiceCallback = slot<QonversionRemoteConfigListCallback>()
+        every {
+            mockRemoteConfigService.loadRemoteConfigs(listOf("ctx"), false, capture(listServiceCallback))
+        } just runs
+        every { mockUserPropertiesManager.forceSendProperties(any()) } answers {
+            firstArg<QonversionEmptyCallback?>()?.onComplete()
+        }
+
+        manager.loadRemoteConfigList(listOf("ctx"), false, mockk(relaxed = true))
+        shadowOf(Looper.getMainLooper()).idle()
+
+        // when - the attach invalidates mid-flight, then the pre-attach list lands
+        manager.attachUserToRemoteConfiguration("config_id", mockk(relaxed = true))
+        shadowOf(Looper.getMainLooper()).idle()
+        val staleConfig = mockk<QRemoteConfig>(relaxed = true)
+        every { staleConfig.source.contextKey } returns "ctx"
+        listServiceCallback.captured.onSuccess(QRemoteConfigList(listOf(staleConfig)))
+
+        // then - nothing from the stale list is cached
+        assertEquals(null, loadingStates()["ctx"]?.loadedConfig)
+    }
+
     private fun listRequests() =
         manager.getPrivateField<List<*>>("listRequests")
 

sdk/src/test/java/com/qonversion/android/sdk/internal/QUserPropertiesManagerTest.kt

@@ -267,8 +267,10 @@ internal class QUserPropertiesManagerTest {
 
     @Test
     fun `should force send properties and get response in onSuccess callback`() {
-        // given
-        mockPropertiesStorage(properties)
+        // given - the storage is drained by the send, so the post-clear check sees no leftovers
+        every {
+            mockPropertiesStorage.getProperties()
+        } returnsMany listOf(properties, emptyMap())
 
         val propertiesResult = SendPropertiesResult(
             emptyList(),
@@ -289,6 +291,7 @@ internal class QUserPropertiesManagerTest {
             mockPropertiesStorage.getProperties()
             mockRepository.sendProperties(properties, any(), any())
             mockPropertiesStorage.clear(properties)
+            mockPropertiesStorage.getProperties()
         }
 
         val isRequestInProgress =
@@ -306,6 +309,81 @@ internal class QUserPropertiesManagerTest {
         )
     }
 
+    @Test
+    fun `should reset isSendingScheduled when request is in progress`() {
+        // given - a scheduled job fires while a request is still in flight
+        propertiesManager.mockPrivateField(fieldIsRequestInProgress, true)
+        propertiesManager.mockPrivateField(fieldIsSendingScheduled, true)
+
+        // when
+        propertiesManager.forceSendProperties()
+
+        // then - the flag is dropped so later setters can schedule a new send
+        val isSendingScheduled = propertiesManager.getPrivateField<Boolean>(fieldIsSendingScheduled)
+        assertEquals(false, isSendingScheduled)
+    }
+
+    @Test
+    fun `should reset isSendingScheduled when properties storage is empty`() {
+        // given - the scheduled send finds nothing to deliver
+        propertiesManager.mockPrivateField(fieldIsSendingScheduled, true)
+        mockPropertiesStorage(mapOf())
+
+        // when
+        propertiesManager.forceSendProperties()
+
+        // then
+        val isSendingScheduled = propertiesManager.getPrivateField<Boolean>(fieldIsSendingScheduled)
+        assertEquals(false, isSendingScheduled)
+    }
+
+    @Test
+    fun `should not schedule follow-up send when sending is already scheduled`() {
+        // given - a setter scheduled a send while the request was in flight
+        every {
+            mockPropertiesStorage.getProperties()
+        } returnsMany listOf(properties, mapOf("newKey" to "newValue"))
+
+        val successLambda = slot<(SendPropertiesResult) -> Unit>()
+        every {
+            mockRepository.sendProperties(properties, capture(successLambda), any())
+        } just runs
+        every { propertiesManager.sendPropertiesWithDelay(any()) } just runs
+
+        // when - the flag is set mid-flight (as a setter would), then the request completes
+        propertiesManager.forceSendProperties()
+        propertiesManager.mockPrivateField(fieldIsSendingScheduled, true)
+        successLambda.captured.invoke(SendPropertiesResult(emptyList(), emptyList()))
+
+        // then - no double scheduling on top of the setter's job
+        verify(exactly = 0) {
+            propertiesManager.sendPropertiesWithDelay(any())
+        }
+    }
+
+    @Test
+    fun `should schedule follow-up send for properties set during the request`() {
+        // given - the storage still holds properties after the sent snapshot is cleared
+        every {
+            mockPropertiesStorage.getProperties()
+        } returnsMany listOf(properties, mapOf("newKey" to "newValue"))
+
+        every {
+            mockRepository.sendProperties(properties, captureLambda(), any())
+        } answers {
+            lambda<(SendPropertiesResult) -> Unit>().captured.invoke(SendPropertiesResult(emptyList(), emptyList()))
+        }
+        every { propertiesManager.sendPropertiesWithDelay(any()) } just runs
+
+        // when
+        propertiesManager.forceSendProperties()
+
+        // then
+        verify(exactly = 1) {
+            propertiesManager.sendPropertiesWithDelay(minDelay)
+        }
+    }
+
     @Test
     fun setProperty() {
         // given