fix: [SDK-5065] retry observability components that failed to start

applyAction committed currentConfig even when a component never came up. Since
a stable remote payload produces an identical config on the next refresh, the
evaluator returned NoChange and the dead crash handler, ANR detector or sink
stayed down for the rest of the process. enableFeatures now reports whether
everything started, the config is only committed once it did, and startLogging
is null-guarded like its siblings so a retry cannot tear down a healthy sink.

startLogging also only had half the teardown invariant: it cleared its own
field but left Logging's global pointing at the old sink while shutting it
down. Every log emitted between shutdown and the replacement being installed --
including the warn in that window -- went to a telemetry whose consumer was
already cancelled, where it queued and was never drained. On a throwing factory
the global stayed on the dead instance for the session.

Reverts the ExpiryOutcome split from the previous commit. It was added on the
theory that an expired record whose delete failed could hold the directory over
cap while invisible to the byte accounting. Writing the test disproved it:
expired records are by definition the oldest, so the selector always picks them
for eviction rather than retention, and only retained records claim budget.
Including them in the candidate set changes no outcome, so the two-set
bookkeeping was inert complexity. Kept a test that the record stays unreadable
when its delete fails, which is the part that does matter.

Also drops a tautological assertion that passed regardless of keepName now that
size is not grounds for eviction, replaces a counter mutated from six
concurrent coroutines with an AtomicInteger, stops building a throwaway
platform provider just to read a path the pure helper computes, and corrects
two KDocs that still claimed the byte cap bounds disk rather than claim.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: AR Abdul Azeez

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt

@@ -11,6 +11,7 @@ import com.onesignal.debug.internal.logging.logger.android.FileLogStore
 import com.onesignal.debug.internal.logging.logger.android.OneSignalLogHttpSender
 import com.onesignal.debug.internal.logging.logger.android.createAndroidLoggerPlatformProvider
 import com.onesignal.debug.internal.logging.logger.android.formatCrashDirInventory
+import com.onesignal.debug.internal.logging.logger.android.getCrashStoragePath
 import com.onesignal.logger.LoggerFactory
 import java.io.File
 import kotlin.coroutines.cancellation.CancellationException
@@ -63,10 +64,13 @@ internal class OneSignalCrashUploaderWrapper(
         }
     }
 
-    /** Resolves the crash directory the logger module reads and writes. */
-    private fun crashStoragePath(): String =
-        createAndroidLoggerPlatformProvider(applicationService.appContext) { featureManager }
-            .crashStoragePath
+    /**
+     * Resolves the crash directory the logger module reads and writes. Uses the pure path
+     * helper rather than a provider: building one costs a `PackageManager` round-trip and an
+     * ID resolver, and re-emits the provider's "Crash logs stored at" line, all to read a
+     * value derived from the context alone.
+     */
+    private fun crashStoragePath(): String = getCrashStoragePath(applicationService.appContext)
 
     /**
      * Logs a snapshot of the crash dir (counts of owned `.otlp` vs foreign/legacy

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/CrashDirCleanup.kt

@@ -24,6 +24,13 @@ internal const val CRASH_MAX_READ_AGE_MILLIS = 72L * 60 * 60 * 1000
  * than a healthy install will ever hold. The byte bound is the backstop for pathological
  * payloads (deep stacktraces, huge exception messages) where count alone would not keep the
  * directory small.
+ *
+ * [CRASH_MAX_TOTAL_BYTES] bounds *claim*, not bytes on disk. Since writes are size-limited,
+ * the two coincide for anything this build wrote. They diverge only for records inherited
+ * from a build without that limit: each claims at most [CRASH_MAX_RECORD_BYTES], so a handful
+ * of oversized leftovers can occupy more than this while still counting as within cap. That
+ * is deliberate — they are real crashes and deserve an upload attempt — and it is bounded by
+ * the count cap and by [CRASH_MAX_READ_AGE_MILLIS] aging them out.
  */
 internal const val CRASH_MAX_RECORD_COUNT = 50
 

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/FileLogStore.kt

@@ -26,7 +26,9 @@ import kotlin.coroutines.cancellation.CancellationException
  *
  * Owned records are bounded on both axes, replacing the caps disk-buffering used to apply:
  * [CRASH_MAX_READ_AGE_MILLIS] ages records out, and [CRASH_MAX_RECORD_COUNT] /
- * [CRASH_MAX_TOTAL_BYTES] cap accumulation. Both bounds are enforced on every path that
+ * [CRASH_MAX_TOTAL_BYTES] cap accumulation — the latter by budget claim rather than raw disk
+ * bytes, which differ only for oversized records inherited from a build that predates the
+ * write-time limit in [save]. Both bounds are enforced on every path that
  * touches the directory — [save], [listReadable] and [deleteUnrecognizedEntries] — so a
  * backlog inherited from a build without caps is reclaimed on the next uploader pass rather
  * than waiting for a crash. Over-limit records are deleted, not merely hidden from
@@ -154,38 +156,32 @@ internal class FileLogStore(
             )
         }.orEmpty()
 
-    /**
-     * Outcome of an expiry pass. The two sets differ when a delete fails: the record is still
-     * unreadable, but it also still occupies the directory, so it must stay visible to the
-     * accumulation caps instead of being quietly exempted from them.
-     */
-    private data class ExpiryOutcome(val expired: Set<String>, val removed: Set<String>) {
-        companion object {
-            val NONE = ExpiryOutcome(emptySet(), emptySet())
-        }
-    }
-
     /**
      * Deletes owned records past [CRASH_MAX_READ_AGE_MILLIS]. Called from both read paths so
      * over-age records are reclaimed even when remote logging is off and the uploader never
      * gets as far as [listReadable].
+     *
+     * @return every expired name, whether or not its delete succeeded. One that could not be
+     *   removed must still not be read, and it cannot distort the accumulation caps either:
+     *   expired records are by definition the oldest, so the selector always picks them for
+     *   eviction rather than retention, and only retained records claim budget.
      */
-    private fun reclaimExpiredOwnedRecords(entries: List<CrashDirEntry>, nowMs: Long): ExpiryOutcome {
+    private fun reclaimExpiredOwnedRecords(entries: List<CrashDirEntry>, nowMs: Long): Set<String> {
         val expired = selectExpiredOwnedEntries(entries, nowMs)
-        if (expired.isEmpty()) return ExpiryOutcome.NONE
-        val removed = HashSet<String>()
+        if (expired.isEmpty()) return emptySet()
+        var deleted = 0
         for (entry in expired) {
             if (File(rootDir, entry.name).delete()) {
-                removed.add(entry.name)
+                deleted++
             } else {
                 Logging.warn("FileLogStore: failed to reclaim expired record ${entry.name}")
             }
         }
         Logging.info(
-            "FileLogStore: reclaimed ${removed.size}/${expired.size} expired record(s) in ${rootDir.path}: " +
+            "FileLogStore: reclaimed $deleted/${expired.size} expired record(s) in ${rootDir.path}: " +
                 expired.take(MAX_NAMES_LOGGED).joinToString(", ") { it.name },
         )
-        return ExpiryOutcome(expired = expired.mapTo(HashSet()) { it.name }, removed = removed)
+        return expired.mapTo(HashSet()) { it.name }
     }
 
     @Suppress("TooGenericExceptionCaught", "SwallowedException")
@@ -196,9 +192,9 @@ internal class FileLogStore(
                 val entries = listEntries(rootDir)
                 // Reclaim before reading: payloads are only materialized for records that
                 // survive both bounds, so an over-cap backlog is never fully loaded.
-                val expiry = reclaimExpiredOwnedRecords(entries, now)
-                val evicted = reclaimOverLimitRecords(entries.filterNot { expiry.removed.contains(it.name) })
-                val dropped = expiry.expired + evicted
+                val expired = reclaimExpiredOwnedRecords(entries, now)
+                val evicted = reclaimOverLimitRecords(entries.filterNot { expired.contains(it.name) })
+                val dropped = expired + evicted
                 val suffixMatches =
                     entries.filter { isOwnedCrashFile(it.name) && !dropped.contains(it.name) }
                 val readable =
@@ -208,7 +204,7 @@ internal class FileLogStore(
                 Logging.debug(
                     "FileLogStore: listReadable minAgeMs=$minAgeMillis total=${entries.size} " +
                         "suffix=${suffixMatches.size} readable=${readable.size} " +
-                        "expired=${expiry.expired.size} overCap=${evicted.size} " +
+                        "expired=${expired.size} overCap=${evicted.size} " +
                         "legacy=${entries.count { !isOwnedCrashFile(it.name) }}",
                 )
                 readable
@@ -264,8 +260,8 @@ internal class FileLogStore(
             try {
                 val now = System.currentTimeMillis()
                 val listed = listEntries(rootDir)
-                val expiry = reclaimExpiredOwnedRecords(listed, now)
-                reclaimOverLimitRecords(listed.filterNot { expiry.removed.contains(it.name) })
+                val expired = reclaimExpiredOwnedRecords(listed, now)
+                reclaimOverLimitRecords(listed.filterNot { expired.contains(it.name) })
                 val foreign = selectUnrecognizedEntries(listed, now, minAgeMillis)
                 if (foreign.isEmpty()) {
                     Logging.debug("FileLogStore: no unrecognized files to purge in ${rootDir.path}")

OneSignalSDK/onesignal/core/src/main/java/com/onesignal/internal/LoggerLifecycleManager.kt

@@ -138,35 +138,64 @@ internal class LoggerLifecycleManager(
         return ObservabilityConfig(isEnabled = enabled, logLevel = level)
     }
 
-    /** Must be called while holding [lock]. */
+    /**
+     * Must be called while holding [lock].
+     *
+     * [currentConfig] is only advanced once the requested state is actually in place. If a
+     * component failed to start, the config is left behind so the next HYDRATE — which for a
+     * stable remote payload is an identical one — still evaluates to `Enable` and retries the
+     * missing piece, instead of collapsing to `NoChange` and leaving it dead for the session.
+     */
     private fun applyAction(action: ObservabilityConfigAction, newConfig: ObservabilityConfig) {
-        when (action) {
-            is ObservabilityConfigAction.Enable -> enableFeatures(newConfig.logLevel ?: LogLevel.ERROR)
-            is ObservabilityConfigAction.Disable -> disableFeatures()
-            is ObservabilityConfigAction.UpdateLogLevel -> updateLogLevel(action.newLevel)
-            is ObservabilityConfigAction.NoChange -> Logging.debug("OneSignal: logger config unchanged")
-        }
-        currentConfig = newConfig
+        val applied =
+            when (action) {
+                is ObservabilityConfigAction.Enable -> enableFeatures(newConfig.logLevel ?: LogLevel.ERROR)
+                is ObservabilityConfigAction.UpdateLogLevel -> updateLogLevel(action.newLevel)
+                is ObservabilityConfigAction.Disable -> {
+                    disableFeatures()
+                    true
+                }
+                is ObservabilityConfigAction.NoChange -> {
+                    Logging.debug("OneSignal: logger config unchanged")
+                    true
+                }
+            }
+        if (applied) currentConfig = newConfig
     }
 
+    /**
+     * Starts whatever is not already running. Each component is independent: one failing must
+     * not stop the others.
+     *
+     * @return true when every feature is up, so the caller knows whether to commit the config
+     */
     @Suppress("TooGenericExceptionCaught")
-    private fun enableFeatures(logLevel: LogLevel) {
+    private fun enableFeatures(logLevel: LogLevel): Boolean {
         Logging.info("OneSignal: Enabling logger module features at level $logLevel")
+        var allStarted = true
         try {
             startCrashHandler()
         } catch (t: Throwable) {
+            allStarted = false
             Logging.warn("OneSignal: Failed to start logger crash handler: ${t.message}", t)
         }
         try {
             startAnrDetector()
         } catch (t: Throwable) {
+            allStarted = false
             Logging.warn("OneSignal: Failed to start logger ANR detector: ${t.message}", t)
         }
         try {
-            startLogging(logLevel)
+            // Guarded like the other two so a retry does not tear down a healthy sink.
+            if (remoteTelemetry == null) startLogging(logLevel)
         } catch (t: Throwable) {
+            allStarted = false
             Logging.warn("OneSignal: Failed to start logger logging: ${t.message}", t)
         }
+        if (!allStarted) {
+            Logging.warn("OneSignal: Some logger features did not start; will retry on the next config refresh")
+        }
+        return allStarted
     }
 
     @Suppress("TooGenericExceptionCaught")
@@ -200,13 +229,16 @@ internal class LoggerLifecycleManager(
         }
     }
 
+    /** @return true when the new level is live, so the caller knows whether to commit the config */
     @Suppress("TooGenericExceptionCaught")
-    private fun updateLogLevel(newLevel: LogLevel) {
+    private fun updateLogLevel(newLevel: LogLevel): Boolean {
         Logging.info("OneSignal: Updating logger module log level to $newLevel")
-        try {
+        return try {
             startLogging(newLevel)
+            true
         } catch (t: Throwable) {
             Logging.warn("OneSignal: Failed to update logger log level: ${t.message}", t)
+            false
         }
     }
 
@@ -228,11 +260,14 @@ internal class LoggerLifecycleManager(
 
     @Suppress("TooGenericExceptionCaught")
     private fun startLogging(logLevel: LogLevel) {
-        // Same invariant as disableFeatures: drop the reference before tearing the old sink
-        // down. A throwing shutdown() must not leave the field pointing at a dead instance,
-        // because an identical later config evaluates to NoChange and would never replace it.
+        // Same invariant as disableFeatures: detach both the field and Logging's global before
+        // tearing the old sink down. Shutting down first would leave every log emitted until
+        // the replacement is installed — including the warn below — going to a telemetry whose
+        // consumer is already cancelled, where it is queued and never drained. If the factory
+        // then throws, the global would keep pointing at that dead instance for the session.
         val previous = remoteTelemetry
         remoteTelemetry = null
+        Logging.setLoggerTelemetry(null) { false }
         try {
             previous?.shutdown()
         } catch (t: Throwable) {

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/LoggingRemoteTest.kt

@@ -16,6 +16,7 @@ import kotlinx.coroutines.delay
 import kotlinx.coroutines.runBlocking
 import kotlinx.coroutines.withTimeout
 import org.robolectric.annotation.Config
+import java.util.concurrent.atomic.AtomicInteger
 
 /**
  * Covers the single remaining remote-logging sink. Emission is asynchronous, so each
@@ -110,8 +111,9 @@ class LoggingRemoteTest : FunSpec({
     test("every severity is forwarded") {
         val telemetry = mockk<ILogTelemetryRemote>(relaxed = true)
         val sixth = CompletableDeferred<Unit>()
-        var seen = 0
-        coEvery { telemetry.emit(any()) } answers { if (++seen == 6) sixth.complete(Unit); Unit }
+        // Emission fans out across Dispatchers.Default, so the counter is touched concurrently.
+        val seen = AtomicInteger(0)
+        coEvery { telemetry.emit(any()) } answers { if (seen.incrementAndGet() == 6) sixth.complete(Unit); Unit }
         Logging.setLoggerTelemetry(telemetry) { true }
 
         Logging.verbose("v")

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/logger/android/CrashDirCleanupTest.kt

@@ -147,12 +147,6 @@ class CrashDirCleanupTest : FunSpec({
         evicted.map { it.name } shouldBe listOf("${CRASH_MAX_RECORD_COUNT}-a.otlp")
     }
 
-    test("keepName retains an oversized just-written record") {
-        val entries = listOf(owned("fresh-a.otlp", ageMs = 1_000, bytes = CRASH_MAX_RECORD_BYTES + 1))
-
-        selectOverflowOwnedEntries(entries, keepName = "fresh-a.otlp") shouldBe emptyList()
-    }
-
     test("an oversized keepName does not evict the pending backlog") {
         // The regression this guards: charging keepName its full length started the budget
         // over cap, so every sibling failed the remaining-budget check and the entire backlog

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/logger/android/FileLogStoreTest.kt

@@ -189,6 +189,21 @@ class FileLogStoreTest : FunSpec({
         dir.listFiles()!!.count { it.name.endsWith(CRASH_OWNED_SUFFIX) } shouldBe CRASH_MAX_RECORD_COUNT
     }
 
+    // A delete can fail (read-only dir, filesystem error). The record must stay unreadable
+    // regardless, and must not resurface on a later pass just because it survived.
+    test("an expired record that cannot be deleted is still withheld from readers") {
+        write("expired-stuck.otlp", ageMsAgo = CRASH_MAX_READ_AGE_MILLIS + 60_000)
+        write("fresh.otlp", ageMsAgo = 60_000)
+        // Read-only dir makes unlink fail on POSIX without making the entries unreadable.
+        dir.setWritable(false)
+
+        val readable = runBlocking { FileLogStore(dir.path).listReadable(minAgeMillis = 0) }
+
+        dir.setWritable(true)
+        readable.map { it.id } shouldBe listOf("fresh.otlp")
+        File(dir, "expired-stuck.otlp").exists() shouldBe true
+    }
+
     test("deleteUnrecognizedEntries evicts an inherited over-cap backlog") {
         repeat(CRASH_MAX_RECORD_COUNT + 10) { i -> write("seed-$i.otlp", ageMsAgo = 1_000L * (i + 1)) }
         write("1784621689841")

OneSignalSDK/onesignal/core/src/test/java/com/onesignal/internal/LoggerLifecycleManagerFaultTest.kt

@@ -261,6 +261,67 @@ class LoggerLifecycleManagerFaultTest : FunSpec({
         detectorCount shouldBe 1
     }
 
+    // A component that failed to start must be retried on the next config refresh. Committing
+    // currentConfig after a partial failure collapsed the next identical HYDRATE to NoChange,
+    // which left the dead component down for the rest of the process.
+
+    test("a crash handler that failed to start is retried on the next identical config") {
+        val failing = mockk<ILogCrashHandler>(relaxed = true)
+        every { failing.initialize() } throws RuntimeException("initialize boom")
+        val replacement = mockk<ILogCrashHandler>(relaxed = true)
+        var calls = 0
+        val manager = managerWith(crashHandler = { if (calls++ == 0) failing else replacement })
+
+        manager.onModelReplaced(enabledConfig(), ModelChangeTags.HYDRATE)
+        manager.onModelReplaced(enabledConfig(), ModelChangeTags.HYDRATE)
+
+        verify { replacement.initialize() }
+    }
+
+    test("an ANR detector that failed to start is retried on the next identical config") {
+        val failing = mockk<ILogAnrDetector>(relaxed = true)
+        every { failing.start() } throws RuntimeException("start boom")
+        val replacement = mockk<ILogAnrDetector>(relaxed = true)
+        var calls = 0
+        val manager = managerWith(anrDetector = { if (calls++ == 0) failing else replacement })
+
+        manager.onModelReplaced(enabledConfig(), ModelChangeTags.HYDRATE)
+        manager.onModelReplaced(enabledConfig(), ModelChangeTags.HYDRATE)
+
+        verify { replacement.start() }
+    }
+
+    test("a retry does not tear down the components that did start") {
+        val handler = mockk<ILogCrashHandler>(relaxed = true)
+        val failingDetector = mockk<ILogAnrDetector>(relaxed = true)
+        every { failingDetector.start() } throws RuntimeException("start boom")
+        var detectorCalls = 0
+        var handlerCalls = 0
+        val manager =
+            managerWith(
+                crashHandler = { handlerCalls++; handler },
+                anrDetector = { detectorCalls++; if (detectorCalls == 1) failingDetector else mockk(relaxed = true) },
+            )
+
+        manager.onModelReplaced(enabledConfig(), ModelChangeTags.HYDRATE)
+        manager.onModelReplaced(enabledConfig(), ModelChangeTags.HYDRATE)
+
+        // Only the ANR detector is rebuilt; the healthy crash handler is left alone.
+        handlerCalls shouldBe 1
+        detectorCalls shouldBe 2
+    }
+
+    test("once every component is up an identical config stops retrying") {
+        var handlerCalls = 0
+        val manager = managerWith(crashHandler = { handlerCalls++; mockk(relaxed = true) })
+
+        manager.onModelReplaced(enabledConfig(), ModelChangeTags.HYDRATE)
+        manager.onModelReplaced(enabledConfig(), ModelChangeTags.HYDRATE)
+        manager.onModelReplaced(enabledConfig(), ModelChangeTags.HYDRATE)
+
+        handlerCalls shouldBe 1
+    }
+
     test("disable then re-enable builds fresh collaborators") {
         var handlerCount = 0
         var detectorCount = 0