feat(logger): [SDK-4939] wire Android FileLogStore for shared KMP legacy purge

.github/workflows/ci.yml

@@ -48,20 +48,17 @@ jobs:
       - name: "[Diff Coverage] Check for bypass"
         id: coverage_bypass
         if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch'
+        env:
+          # Evaluate in Actions expressions (not shell) so label names with spaces cannot break quoting.
+          HAS_SKIP_COVERAGE_LABEL: ${{ github.event_name == 'pull_request' && (contains(github.event.pull_request.labels.*.name, 'Skip Coverage Check') || contains(github.event.pull_request.labels.*.name, 'skip-coverage-check')) }}
         run: |
-          # Check if PR has Skip Coverage Check label
-          if [ "${{ github.event_name }}" = "pull_request" ]; then
-            LABELS="${{ toJson(github.event.pull_request.labels.*.name) }}"
-            if echo "$LABELS" | grep -qiE "Skip Coverage Check|skip-coverage-check"; then
-              echo "bypass=true" >> $GITHUB_OUTPUT
-              echo "reason=PR has 'Skip Coverage Check' label" >> $GITHUB_OUTPUT
-              echo "⚠️ Coverage check will not fail build (PR has 'Skip Coverage Check' label)"
-              echo "   Coverage will still be checked and reported"
-            else
-              echo "bypass=false" >> $GITHUB_OUTPUT
-            fi
+          if [ "$HAS_SKIP_COVERAGE_LABEL" = "true" ]; then
+            echo "bypass=true" >> "$GITHUB_OUTPUT"
+            echo "reason=PR has Skip Coverage Check label" >> "$GITHUB_OUTPUT"
+            echo "Coverage check will not fail build (PR has Skip Coverage Check label)"
+            echo "Coverage will still be checked and reported"
           else
-            echo "bypass=false" >> $GITHUB_OUTPUT
+            echo "bypass=false" >> "$GITHUB_OUTPUT"
           fi
       - name: "[PR] Coverage on changed lines (min ${{ env.DIFF_COVERAGE_THRESHOLD }}%)"
         if: github.event_name == 'pull_request' || github.event_name == 'workflow_dispatch'

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

@@ -4,16 +4,21 @@ import com.onesignal.common.threading.OneSignalDispatchers
 import com.onesignal.core.internal.application.IApplicationService
 import com.onesignal.core.internal.features.IFeatureManager
 import com.onesignal.core.internal.startup.IStartableService
+import com.onesignal.debug.internal.logging.Logging
 import com.onesignal.debug.internal.logging.logger.LoggerModuleSwitch
 import com.onesignal.debug.internal.logging.logger.android.AndroidLogger
+import com.onesignal.debug.internal.logging.logger.android.CrashDirEntry
 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.otel.android.AndroidOtelLogger
 import com.onesignal.debug.internal.logging.otel.android.createAndroidOtelPlatformProvider
 import com.onesignal.logger.LoggerFactory
 import com.onesignal.otel.OtelFactory
 import com.onesignal.otel.crash.OtelCrashUploader
+import java.io.File
+import kotlin.coroutines.cancellation.CancellationException
 
 /**
  * Android-specific wrapper for OtelCrashUploader that implements IStartableService.
@@ -66,17 +71,68 @@ internal class OneSignalCrashUploaderWrapper(
         if (!OtelSdkSupport.isSupported) return
         OneSignalDispatchers.launchOnIO {
             try {
-                if (LoggerModuleSwitch.useLoggerModule(applicationService.appContext)) {
+                val useLogger = LoggerModuleSwitch.useLoggerModule(applicationService.appContext)
+                val module = if (useLogger) "logger" else "otel"
+                Logging.info("OneSignal: Crash uploader selecting module=$module (SDK_CUSTOM_LOGGING=$useLogger)")
+                logCrashDirInventory("before-upload")
+                if (useLogger) {
+                    // Shared LogCrashUploader.start() is suspend and finishes the owned-record
+                    // upload pass plus the finally-purge before returning, so the after-cleanup
+                    // inventory below is not racing a background purge.
                     loggerUploader.start()
+                    logCrashDirInventory("after-cleanup")
                 } else {
                     otelUploader.start()
                 }
+            } catch (e: CancellationException) {
+                throw e
             } catch (t: Throwable) {
-                com.onesignal.debug.internal.logging.Logging.warn(
+                Logging.warn(
                     "OneSignal: Crash uploader failed to start: ${t.message}",
                     t,
                 )
             }
         }
     }
+
+    /** Resolves the shared crash directory both modules write to. */
+    private fun crashStoragePath(): String =
+        createAndroidOtelPlatformProvider(applicationService.appContext) { featureManager }
+            .crashStoragePath
+
+    /**
+     * Logs a snapshot of the shared crash dir (counts of owned `.otlp` vs foreign/legacy
+     * entries, plus a bounded per-file sample) so leftover formats are visible and
+     * cleanup is verifiable from logs alone.
+     */
+    @Suppress("TooGenericExceptionCaught")
+    private fun logCrashDirInventory(label: String) {
+        try {
+            val path = crashStoragePath()
+            val now = System.currentTimeMillis()
+            val entries =
+                File(path).listFiles()?.filter { it.isFile }?.map { file ->
+                    CrashDirEntry(
+                        name = file.name,
+                        lastModifiedMs = file.lastModified(),
+                        lengthBytes = file.length(),
+                    )
+                }.orEmpty()
+            Logging.info(
+                formatCrashDirInventory(
+                    label = label,
+                    path = path,
+                    entries = entries,
+                    nowMs = now,
+                    maxSample = MAX_INVENTORY_SAMPLE,
+                ),
+            )
+        } catch (t: Throwable) {
+            Logging.warn("OneSignal: Crash storage inventory failed: ${t.message}", t)
+        }
+    }
+
+    private companion object {
+        const val MAX_INVENTORY_SAMPLE = 20
+    }
 }

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

@@ -0,0 +1,67 @@
+package com.onesignal.debug.internal.logging.logger.android
+
+/**
+ * Pure, Android-free helpers for the shared logger/otel crash directory.
+ *
+ * Ownership is suffix-based: logger-owned records end in [CRASH_OWNED_SUFFIX]; everything else
+ * (legacy otel bare-millis names, stray `.tmp`s) is foreign. Keeping this logic free of
+ * `File` / `Logging` / Robolectric means it is counted by Jacoco on the plain JVM.
+ */
+internal const val CRASH_OWNED_SUFFIX = ".otlp"
+
+internal data class CrashDirEntry(
+    val name: String,
+    val lastModifiedMs: Long,
+    val lengthBytes: Long = 0L,
+)
+
+/** True when [name] is a logger-owned crash record. */
+internal fun isOwnedCrashFile(name: String, ownedSuffix: String = CRASH_OWNED_SUFFIX): Boolean =
+    name.endsWith(ownedSuffix)
+
+/**
+ * Returns foreign/legacy entries old enough to reclaim. Owned `*.otlp` names are never selected,
+ * regardless of age.
+ */
+internal fun selectUnrecognizedEntries(
+    entries: List<CrashDirEntry>,
+    nowMs: Long,
+    minAgeMillis: Long,
+    ownedSuffix: String = CRASH_OWNED_SUFFIX,
+): List<CrashDirEntry> =
+    entries.filter { entry ->
+        !isOwnedCrashFile(entry.name, ownedSuffix) &&
+            nowMs - entry.lastModifiedMs >= minAgeMillis
+    }
+
+/**
+ * Builds the human-readable crash-dir inventory line used for rollout verification.
+ * Per-file detail is capped at [maxSample] so Logcat is not flooded.
+ */
+internal fun formatCrashDirInventory(
+    label: String,
+    path: String,
+    entries: List<CrashDirEntry>,
+    nowMs: Long,
+    maxSample: Int,
+    ownedSuffix: String = CRASH_OWNED_SUFFIX,
+): String {
+    if (entries.isEmpty()) {
+        return "OneSignal: Crash storage inventory [$label] ($path): empty"
+    }
+    val otlp = entries.count { isOwnedCrashFile(it.name, ownedSuffix) }
+    val legacy = entries.size - otlp
+    val sample = entries.take(maxSample)
+    val summary =
+        sample.joinToString(separator = "; ") { entry ->
+            "name=${entry.name} bytes=${entry.lengthBytes} ageMs=${nowMs - entry.lastModifiedMs}"
+        }
+    val truncated =
+        if (entries.size > maxSample) {
+            " …(+${entries.size - maxSample} more)"
+        } else {
+            ""
+        }
+    return "OneSignal: Crash storage inventory [$label] ($path): " +
+        "total=${entries.size} otlp=$otlp legacy=$legacy [$summary]$truncated"
+}

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

@@ -1,11 +1,14 @@
 package com.onesignal.debug.internal.logging.logger.android
 
+import android.util.Log
+import com.onesignal.debug.internal.logging.Logging
 import com.onesignal.logger.ILogFileStore
 import com.onesignal.logger.StoredLogFile
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.withContext
 import java.io.File
 import java.util.UUID
+import kotlin.coroutines.cancellation.CancellationException
 
 /**
  * Android [ILogFileStore] backed by the local filesystem. Replaces OpenTelemetry's
@@ -15,14 +18,19 @@ import java.util.UUID
  * modified time is used as the record age for [listReadable], mirroring the old
  * `minFileAgeForReadMillis` behavior (never read a file the crashing process may
  * still have been writing).
+ *
+ * The logger and the legacy otel module share this one crash directory, so ownership
+ * is distinguished purely by [CRASH_OWNED_SUFFIX]: everything the logger writes ends in
+ * `.otlp`; anything else (legacy otel bare-millis files, stray `.tmp`s) is foreign and
+ * reclaimable via [deleteUnrecognizedEntries] once the logger is the active module.
  */
 internal class FileLogStore(
     private val rootPath: String,
 ) : ILogFileStore {
     private val rootDir: File get() = File(rootPath)
 
     private companion object {
-        const val FILE_SUFFIX = ".otlp"
+        const val TAG = "OneSignal"
     }
 
     @Suppress("TooGenericExceptionCaught", "SwallowedException")
@@ -31,17 +39,21 @@ internal class FileLogStore(
             val dir = rootDir
             if (!dir.exists()) dir.mkdirs()
             // Write to a temp file then rename so a half-written file is never readable.
-            val target = File(dir, "${System.currentTimeMillis()}-${UUID.randomUUID()}$FILE_SUFFIX")
+            val target = File(dir, "${System.currentTimeMillis()}-${UUID.randomUUID()}$CRASH_OWNED_SUFFIX")
             val temp = File(dir, target.name + ".tmp")
             temp.writeBytes(bytes)
             if (!temp.renameTo(target)) {
                 // Fallback: write directly if rename is unsupported on this fs.
                 target.writeBytes(bytes)
                 temp.delete()
             }
+            // Crash path: raw Logcat only — Logging.info can invoke app listeners
+            // synchronously, and a listener exception would flip a successful write to false.
+            Log.i(TAG, "FileLogStore: saved name=${target.name} bytes=${bytes.size} dir=${dir.path}")
             true
         } catch (t: Throwable) {
             // Crash-path safety: never throw from persistence; signal failure to caller.
+            Log.w(TAG, "FileLogStore: save failed: ${t.message}")
             false
         }
     }
@@ -51,29 +63,99 @@ internal class FileLogStore(
         withContext(Dispatchers.IO) {
             try {
                 val now = System.currentTimeMillis()
-                rootDir.listFiles { file -> file.isFile && file.name.endsWith(FILE_SUFFIX) }
-                    ?.filter { now - it.lastModified() >= minAgeMillis }
-                    ?.mapNotNull { file ->
-                        try {
-                            StoredLogFile(id = file.name, bytes = file.readBytes())
-                        } catch (t: Throwable) {
-                            null
-                        }
-                    }
-                    ?: emptyList()
+                val allFiles = rootDir.listFiles()?.filter { it.isFile }.orEmpty()
+                val suffixMatches = allFiles.filter { isOwnedCrashFile(it.name) }
+                val readable =
+                    suffixMatches
+                        .filter { now - it.lastModified() >= minAgeMillis }
+                        .mapNotNull { file -> readRecord(file) }
+                Logging.debug(
+                    "FileLogStore: listReadable minAgeMs=$minAgeMillis total=${allFiles.size} " +
+                        "suffix=${suffixMatches.size} readable=${readable.size} " +
+                        "legacy=${allFiles.size - suffixMatches.size}",
+                )
+                readable
+            } catch (e: CancellationException) {
+                throw e
             } catch (t: Throwable) {
+                Logging.warn("FileLogStore: listReadable failed: ${t.message}")
                 emptyList()
             }
         }
 
+    @Suppress("TooGenericExceptionCaught", "SwallowedException")
+    private fun readRecord(file: File): StoredLogFile? =
+        try {
+            StoredLogFile(id = file.name, bytes = file.readBytes())
+        } catch (t: Throwable) {
+            Logging.warn("FileLogStore: failed to read ${file.name}: ${t.message}")
+            null
+        }
+
     @Suppress("TooGenericExceptionCaught", "SwallowedException")
     override suspend fun delete(id: String) {
         withContext(Dispatchers.IO) {
             try {
-                File(rootDir, id).delete()
+                val deleted = File(rootDir, id).delete()
+                Logging.debug("FileLogStore: delete id=$id deleted=$deleted")
+            } catch (e: CancellationException) {
+                throw e
             } catch (t: Throwable) {
-                // best-effort
+                Logging.warn("FileLogStore: delete failed id=$id: ${t.message}")
             }
         }
     }
+
+    /**
+     * Removes on-disk entries this store does not own — legacy OTEL disk-buffering
+     * files (bare-millis names) and stray `.tmp`s that share this directory — whose
+     * age is at least [minAgeMillis]. All `*.otlp` owned records are left untouched
+     * so failed / too-young uploads can still retry on the next launch.
+     *
+     * Implements the shared [ILogFileStore] contract: the KMP `LogCrashUploader`
+     * invokes this after its owned-record upload pass. Idempotent and safe to call
+     * repeatedly.
+     *
+     * @return number of unrecognized entries deleted
+     */
+    @Suppress("TooGenericExceptionCaught", "SwallowedException")
+    override suspend fun deleteUnrecognizedEntries(minAgeMillis: Long): Int =
+        withContext(Dispatchers.IO) {
+            try {
+                val now = System.currentTimeMillis()
+                val listed =
+                    rootDir.listFiles()?.filter { it.isFile }?.map { file ->
+                        CrashDirEntry(
+                            name = file.name,
+                            lastModifiedMs = file.lastModified(),
+                            lengthBytes = file.length(),
+                        )
+                    }.orEmpty()
+                val foreign = selectUnrecognizedEntries(listed, now, minAgeMillis)
+                if (foreign.isEmpty()) {
+                    Logging.debug("FileLogStore: no unrecognized files to purge in ${rootDir.path}")
+                    return@withContext 0
+                }
+                var deleted = 0
+                val names = mutableListOf<String>()
+                for (entry in foreign) {
+                    val file = File(rootDir, entry.name)
+                    if (file.delete()) {
+                        deleted++
+                        names.add(entry.name)
+                    } else {
+                        Logging.warn("FileLogStore: failed to purge unrecognized file ${entry.name}")
+                    }
+                }
+                Logging.info(
+                    "FileLogStore: purged $deleted unrecognized file(s) in ${rootDir.path}: $names",
+                )
+                deleted
+            } catch (e: CancellationException) {
+                throw e
+            } catch (t: Throwable) {
+                Logging.warn("FileLogStore: deleteUnrecognizedEntries failed: ${t.message}")
+                0
+            }
+        }
 }