Fix: ANR on POST to Outcomes endpoint

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsController.kt

@@ -1,6 +1,7 @@
 package com.onesignal.session.internal.outcomes.impl
 
 import android.os.Process
+import com.onesignal.common.NetworkUtils
 import com.onesignal.common.exceptions.BackendException
 import com.onesignal.common.threading.suspendifyOnThread
 import com.onesignal.core.internal.config.ConfigModelStore
@@ -75,10 +76,15 @@ internal class OutcomeEventsController(
 
             _outcomeEventsCache.deleteOldOutcomeEvent(event)
         } catch (ex: BackendException) {
-            Logging.warn(
-                """OutcomeEventsController.sendSavedOutcomeEvent: Sending outcome with name: ${event.outcomeId} failed with status code: ${ex.statusCode} and response: ${ex.response}
-Outcome event was cached and will be reattempted on app cold start""",
-            )
+            val responseType = NetworkUtils.getResponseStatusType(ex.statusCode)
+            val err = "OutcomeEventsController.sendSavedOutcomeEvent: Sending outcome with name: ${event.outcomeId} failed with status code: ${ex.statusCode} and response: ${ex.response}"
+
+            if (responseType == NetworkUtils.ResponseStatusType.RETRYABLE) {
+                Logging.warn("$err Outcome event was cached and will be reattempted on app cold start")
+            } else {
+                Logging.error("$err Outcome event will be omitted!")
+                _outcomeEventsCache.deleteOldOutcomeEvent(event)
+            }
         }
     }
 
@@ -220,14 +226,19 @@ Outcome event was cached and will be reattempted on app cold start""",
             // The only case where an actual success has occurred and the OutcomeEvent should be sent back
             return OutcomeEvent.fromOutcomeEventParamstoOutcomeEvent(eventParams)
         } catch (ex: BackendException) {
-            Logging.warn(
-                """OutcomeEventsController.sendAndCreateOutcomeEvent: Sending outcome with name: $name failed with status code: ${ex.statusCode} and response: ${ex.response}
-Outcome event was cached and will be reattempted on app cold start""",
-            )
-
-            // Only if we need to save and retry the outcome, then we will save the timestamp for future sending
-            eventParams.timestamp = timestampSeconds
-            _outcomeEventsCache.saveOutcomeEvent(eventParams)
+            val responseType = NetworkUtils.getResponseStatusType(ex.statusCode)
+            val err = "OutcomeEventsController.sendAndCreateOutcomeEvent: Sending outcome with name: $name failed with status code: ${ex.statusCode} and response: ${ex.response}"
+
+            if (responseType == NetworkUtils.ResponseStatusType.RETRYABLE) {
+                Logging.warn("$err Outcome event was cached and will be reattempted on app cold start")
+
+                // Only if we need to save and retry the outcome, then we will save the timestamp for future sending
+                eventParams.timestamp = timestampSeconds
+                _outcomeEventsCache.saveOutcomeEvent(eventParams)
+            } else {
+                Logging.error("$err Outcome event will be omitted!")
+                _outcomeEventsCache.deleteOldOutcomeEvent(eventParams)
+            }
 
             // Return null to determine not a failure, but not a success in terms of the request made
             return null

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/impl/OutcomeEventsRepository.kt

@@ -8,7 +8,7 @@ import com.onesignal.session.internal.influence.Influence
 import com.onesignal.session.internal.influence.InfluenceChannel
 import com.onesignal.session.internal.influence.InfluenceType
 import com.onesignal.session.internal.influence.InfluenceType.Companion.fromString
-import com.onesignal.session.internal.outcomes.migrations.RemoveZeroSessionTimeRecords
+import com.onesignal.session.internal.outcomes.migrations.RemoveInvalidSessionTimeRecords
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
 import org.json.JSONArray
@@ -102,7 +102,7 @@ internal class OutcomeEventsRepository(
     override suspend fun getAllEventsToSend(): List<OutcomeEventParams> {
         val events: MutableList<OutcomeEventParams> = ArrayList()
         withContext(Dispatchers.IO) {
-            RemoveZeroSessionTimeRecords.run(_databaseProvider)
+            RemoveInvalidSessionTimeRecords.run(_databaseProvider)
             _databaseProvider.os.query(OutcomeEventsTable.TABLE_NAME) { cursor ->
                 if (cursor.moveToFirst()) {
                     do {

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveZeroSessionTimeRecords.kt → OneSignalSDK/onesignal/core/src/main/java/com/onesignal/session/internal/outcomes/migrations/RemoveInvalidSessionTimeRecords.kt

@@ -4,20 +4,22 @@ import com.onesignal.core.internal.database.IDatabaseProvider
 import com.onesignal.session.internal.outcomes.impl.OutcomeEventsTable
 
 /**
- * Purpose: Clean up invalid cached os__session_duration outcome records
- * with zero session_time produced in SDK versions 5.1.15 to 5.1.20 so we stop
- * sending these requests to the backend.
+ * Purpose: Clean up invalid cached os__session_duration outcome records with
+ *   1. zero session_time produced in SDK versions 5.1.15 to 5.1.20
+ *   2. missing session_time produced in SDK
+ * so we stop sending these requests to the backend.
  *
  * Issue: SessionService.backgroundRun() didn't account for it being run more
  * than one time in the background, when this happened it would create a
- * outcome record with zero time which is invalid.
+ * outcome record with zero time or null which is invalid.
  */
-object RemoveZeroSessionTimeRecords {
+object RemoveInvalidSessionTimeRecords {
     fun run(databaseProvider: IDatabaseProvider) {
         databaseProvider.os.delete(
             OutcomeEventsTable.TABLE_NAME,
             OutcomeEventsTable.COLUMN_NAME_NAME + " = \"os__session_duration\"" +
-                " AND " + OutcomeEventsTable.COLUMN_NAME_SESSION_TIME + " = 0",
+                " AND (" + OutcomeEventsTable.COLUMN_NAME_SESSION_TIME + " = 0" +
+                " OR " + OutcomeEventsTable.COLUMN_NAME_SESSION_TIME + " IS NULL)",
             null,
         )
     }

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/session/internal/outcomes/OutcomeEventsControllerTests.kt

@@ -723,4 +723,105 @@ class OutcomeEventsControllerTests : FunSpec({
         }
         coVerify(exactly = 0) { mockOutcomeEventsRepository.deleteOldOutcomeEvent(any()) }
     }
+
+    test("send saved outcome event with 400 error deletes event and does not retry") {
+        // Given
+        val now = 111111L
+        val mockSessionService = mockk<ISessionService>()
+        every { mockSessionService.subscribe(any()) } just Runs
+
+        val subscriptionModel = createTestSubscriptionModel()
+
+        val mockSubscriptionManager = mockk<ISubscriptionManager>()
+        every { mockSubscriptionManager.subscriptions.push } returns PushSubscription(subscriptionModel)
+
+        val mockInfluenceManager = mockk<IInfluenceManager>()
+        val mockOutcomeEventsRepository = mockk<IOutcomeEventsRepository>()
+        coEvery { mockOutcomeEventsRepository.cleanCachedUniqueOutcomeEventNotifications() } just runs
+        coEvery { mockOutcomeEventsRepository.deleteOldOutcomeEvent(any()) } just runs
+        coEvery { mockOutcomeEventsRepository.getAllEventsToSend() } returns
+            listOf(
+                OutcomeEventParams("outcomeId1", OutcomeSource(OutcomeSourceBody(JSONArray().put("notificationId1")), null), .4f, 0, 1111),
+            )
+        val mockOutcomeEventsPreferences = spyk<IOutcomeEventsPreferences>()
+        val mockOutcomeEventsBackend = mockk<IOutcomeEventsBackendService>()
+        coEvery { mockOutcomeEventsBackend.sendOutcomeEvent(any(), any(), any(), any(), any(), any()) } throws BackendException(400, "Bad Request")
+
+        val outcomeEventsController =
+            OutcomeEventsController(
+                mockSessionService,
+                mockInfluenceManager,
+                mockOutcomeEventsRepository,
+                mockOutcomeEventsPreferences,
+                mockOutcomeEventsBackend,
+                MockHelper.configModelStore(),
+                MockHelper.identityModelStore { it.onesignalId = "onesignalId" },
+                mockSubscriptionManager,
+                MockHelper.deviceService(),
+                MockHelper.time(now),
+            )
+
+        // When
+        outcomeEventsController.start()
+        delay(1000)
+
+        // Then
+        coVerify(exactly = 1) {
+            mockOutcomeEventsBackend.sendOutcomeEvent(any(), any(), any(), any(), any(), any())
+        }
+        coVerify(exactly = 1) {
+            mockOutcomeEventsRepository.deleteOldOutcomeEvent(any())
+        }
+    }
+
+    test("send outcome event with 400 error deletes event and returns null") {
+        // Given
+        val now = 111L
+        val mockSessionService = mockk<ISessionService>()
+        every { mockSessionService.subscribe(any()) } just Runs
+
+        val mockInfluenceManager = mockk<IInfluenceManager>()
+        every { mockInfluenceManager.influences } returns listOf(Influence(InfluenceChannel.NOTIFICATION, InfluenceType.UNATTRIBUTED, null))
+
+        val subscriptionModel = createTestSubscriptionModel()
+
+        val mockSubscriptionManager = mockk<ISubscriptionManager>()
+        every { mockSubscriptionManager.subscriptions.push } returns PushSubscription(subscriptionModel)
+
+        val mockOutcomeEventsRepository = spyk<IOutcomeEventsRepository>()
+        val mockOutcomeEventsPreferences = spyk<IOutcomeEventsPreferences>()
+        val mockOutcomeEventsBackend = mockk<IOutcomeEventsBackendService>()
+        coEvery { mockOutcomeEventsBackend.sendOutcomeEvent(any(), any(), any(), any(), any(), any()) } throws BackendException(400, "Bad Request")
+
+        val outcomeEventsController =
+            OutcomeEventsController(
+                mockSessionService,
+                mockInfluenceManager,
+                mockOutcomeEventsRepository,
+                mockOutcomeEventsPreferences,
+                mockOutcomeEventsBackend,
+                MockHelper.configModelStore(),
+                MockHelper.identityModelStore(),
+                mockSubscriptionManager,
+                MockHelper.deviceService(),
+                MockHelper.time(now),
+            )
+
+        // When
+        val evnt = outcomeEventsController.sendOutcomeEvent("OUTCOME_1")
+
+        // Then
+        evnt shouldBe null
+
+        coVerify(exactly = 1) {
+            mockOutcomeEventsBackend.sendOutcomeEvent(any(), any(), any(), any(), any(), any())
+        }
+        coVerify(exactly = 1) {
+            mockOutcomeEventsRepository.deleteOldOutcomeEvent(any())
+        }
+        // Verify event is NOT saved for retry (unlike other error codes)
+        coVerify(exactly = 0) {
+            mockOutcomeEventsRepository.saveOutcomeEvent(any())
+        }
+    }
 })