From aa6d9e95394e511eaafd218a6c214c512cb4e6ea Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Fri, 24 Jul 2026 23:22:29 +0530 Subject: [PATCH 1/4] feat(logger): [SDK-4939] wire Android FileLogStore for shared legacy purge Pins OneSignal-KMP-SDK to v0.1.1 and implements age-gated ILogFileStore.deleteUnrecognizedEntries on Android. Adds bounded before/after crash-dir inventory logs; purge orchestration stays in KMP. Crash-path save() uses raw Logcat so listener failures cannot flip a successful write to false. Co-authored-by: Cursor --- .../crash/OneSignalCrashUploaderWrapper.kt | 63 ++++++++++- .../logging/logger/android/FileLogStore.kt | 103 ++++++++++++++++-- .../logger/android/FileLogStoreTest.kt | 91 ++++++++++++++++ 3 files changed, 243 insertions(+), 14 deletions(-) create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/logger/android/FileLogStoreTest.kt diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt index fb3ed3a0b3..fb9babb35f 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt @@ -4,6 +4,7 @@ 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.FileLogStore @@ -14,6 +15,8 @@ import com.onesignal.debug.internal.logging.otel.android.createAndroidOtelPlatfo 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 +69,73 @@ 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 owns the legacy-file purge via + // ILogFileStore.deleteUnrecognizedEntries() (FileLogStore). 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 dir = File(path) + val files = dir.listFiles()?.filter { it.isFile }.orEmpty() + if (files.isEmpty()) { + Logging.info("OneSignal: Crash storage inventory [$label] ($path): empty") + return + } + val otlp = files.count { it.name.endsWith(".otlp") } + val legacy = files.size - otlp + val now = System.currentTimeMillis() + val sample = files.take(MAX_INVENTORY_SAMPLE) + val summary = + sample.joinToString(separator = "; ") { file -> + "name=${file.name} bytes=${file.length()} ageMs=${now - file.lastModified()}" + } + val truncated = + if (files.size > MAX_INVENTORY_SAMPLE) { + " …(+${files.size - MAX_INVENTORY_SAMPLE} more)" + } else { + "" + } + Logging.info( + "OneSignal: Crash storage inventory [$label] ($path): " + + "total=${files.size} otlp=$otlp legacy=$legacy [$summary]$truncated", + ) + } catch (t: Throwable) { + Logging.warn("OneSignal: Crash storage inventory failed: ${t.message}", t) + } + } + + private companion object { + const val MAX_INVENTORY_SAMPLE = 20 + } } diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/FileLogStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/FileLogStore.kt index a65d87be92..e2fd66f96b 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/FileLogStore.kt +++ b/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,6 +18,11 @@ 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 the [FILE_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, @@ -23,6 +31,7 @@ internal class FileLogStore( private companion object { const val FILE_SUFFIX = ".otlp" + const val TAG = "OneSignal" } @Suppress("TooGenericExceptionCaught", "SwallowedException") @@ -39,9 +48,13 @@ internal class FileLogStore( 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 +64,95 @@ 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 { it.name.endsWith(FILE_SUFFIX) } + 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 foreign = + rootDir.listFiles()?.filter { file -> + file.isFile && + !file.name.endsWith(FILE_SUFFIX) && + now - file.lastModified() >= minAgeMillis + }.orEmpty() + if (foreign.isEmpty()) { + Logging.debug("FileLogStore: no unrecognized files to purge in ${rootDir.path}") + return@withContext 0 + } + var deleted = 0 + val names = mutableListOf() + for (file in foreign) { + if (file.delete()) { + deleted++ + names.add(file.name) + } else { + Logging.warn("FileLogStore: failed to purge unrecognized file ${file.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 + } + } } diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/logger/android/FileLogStoreTest.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/logger/android/FileLogStoreTest.kt new file mode 100644 index 0000000000..d7083a58d3 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/logger/android/FileLogStoreTest.kt @@ -0,0 +1,91 @@ +package com.onesignal.debug.internal.logging.logger.android + +import android.os.Build +import br.com.colman.kotest.android.extensions.robolectric.RobolectricTest +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe +import kotlinx.coroutines.runBlocking +import org.robolectric.annotation.Config +import java.io.File +import java.nio.file.Files + +@RobolectricTest +@Config(sdk = [Build.VERSION_CODES.O]) +class FileLogStoreTest : FunSpec({ + + lateinit var dir: File + + beforeEach { + dir = Files.createTempDirectory("crashes").toFile() + } + + afterEach { + dir.deleteRecursively() + } + + fun write(name: String, ageMsAgo: Long = 60_000L): File = + File(dir, name).apply { + writeBytes("x".toByteArray()) + setLastModified(System.currentTimeMillis() - ageMsAgo) + } + + test("deleteUnrecognizedEntries removes stale legacy files and keeps owned .otlp records") { + write("1784621689841") // legacy otel bare-millis file + write("stale.tmp") // stray temp + write("123-abc.otlp") // owned logger record + write("456-def.otlp") // owned logger record + + val purged = runBlocking { FileLogStore(dir.path).deleteUnrecognizedEntries(minAgeMillis = 0) } + + purged shouldBe 2 + File(dir, "1784621689841").exists() shouldBe false + File(dir, "stale.tmp").exists() shouldBe false + File(dir, "123-abc.otlp").exists() shouldBe true + File(dir, "456-def.otlp").exists() shouldBe true + } + + test("deleteUnrecognizedEntries preserves too-young foreign files") { + write("too-young-legacy", ageMsAgo = 100) + write("stale-legacy", ageMsAgo = 10_000) + + val purged = runBlocking { + FileLogStore(dir.path).deleteUnrecognizedEntries(minAgeMillis = 5_000) + } + + purged shouldBe 1 + File(dir, "too-young-legacy").exists() shouldBe true + File(dir, "stale-legacy").exists() shouldBe false + } + + test("deleteUnrecognizedEntries is a no-op when only owned records exist") { + write("123-abc.otlp") + + val purged = runBlocking { FileLogStore(dir.path).deleteUnrecognizedEntries(minAgeMillis = 0) } + + purged shouldBe 0 + File(dir, "123-abc.otlp").exists() shouldBe true + } + + test("deleteUnrecognizedEntries returns 0 for a missing directory") { + val missing = File(dir, "does-not-exist") + + val purged = runBlocking { + FileLogStore(missing.path).deleteUnrecognizedEntries(minAgeMillis = 0) + } + + purged shouldBe 0 + } + + test("deleteUnrecognizedEntries is idempotent across repeated calls") { + write("legacy-file") + write("789-ghi.otlp") + + val store = FileLogStore(dir.path) + val first = runBlocking { store.deleteUnrecognizedEntries(minAgeMillis = 0) } + val second = runBlocking { store.deleteUnrecognizedEntries(minAgeMillis = 0) } + + first shouldBe 1 + second shouldBe 0 + File(dir, "789-ghi.otlp").exists() shouldBe true + } +}) From c1066c98771b92f60fb9639af6d7ed1203e719a1 Mon Sep 17 00:00:00 2001 From: Fadi George Date: Mon, 27 Jul 2026 13:19:37 -0700 Subject: [PATCH 2/4] fix(logger): [SDK-4939] clarify suspend purge ordering and cover crash-dir helpers Extract ownership/inventory helpers to pure JVM code so Jacoco attributes them, and document that LogCrashUploader.start() completes upload+purge before the after-cleanup inventory snapshot. Co-authored-by: Cursor --- .../crash/OneSignalCrashUploaderWrapper.kt | 43 +++++----- .../logging/logger/android/CrashDirCleanup.kt | 67 +++++++++++++++ .../logging/logger/android/FileLogStore.kt | 27 +++--- .../logger/android/CrashDirCleanupTest.kt | 83 +++++++++++++++++++ 4 files changed, 185 insertions(+), 35 deletions(-) create mode 100644 OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/CrashDirCleanup.kt create mode 100644 OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/logger/android/CrashDirCleanupTest.kt diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt index fb9babb35f..28b1820189 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/crash/OneSignalCrashUploaderWrapper.kt @@ -7,9 +7,11 @@ 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 @@ -74,8 +76,9 @@ internal class OneSignalCrashUploaderWrapper( Logging.info("OneSignal: Crash uploader selecting module=$module (SDK_CUSTOM_LOGGING=$useLogger)") logCrashDirInventory("before-upload") if (useLogger) { - // Shared LogCrashUploader owns the legacy-file purge via - // ILogFileStore.deleteUnrecognizedEntries() (FileLogStore). + // 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 { @@ -106,29 +109,23 @@ internal class OneSignalCrashUploaderWrapper( private fun logCrashDirInventory(label: String) { try { val path = crashStoragePath() - val dir = File(path) - val files = dir.listFiles()?.filter { it.isFile }.orEmpty() - if (files.isEmpty()) { - Logging.info("OneSignal: Crash storage inventory [$label] ($path): empty") - return - } - val otlp = files.count { it.name.endsWith(".otlp") } - val legacy = files.size - otlp val now = System.currentTimeMillis() - val sample = files.take(MAX_INVENTORY_SAMPLE) - val summary = - sample.joinToString(separator = "; ") { file -> - "name=${file.name} bytes=${file.length()} ageMs=${now - file.lastModified()}" - } - val truncated = - if (files.size > MAX_INVENTORY_SAMPLE) { - " …(+${files.size - MAX_INVENTORY_SAMPLE} more)" - } else { - "" - } + val entries = + File(path).listFiles()?.filter { it.isFile }?.map { file -> + CrashDirEntry( + name = file.name, + lastModifiedMs = file.lastModified(), + lengthBytes = file.length(), + ) + }.orEmpty() Logging.info( - "OneSignal: Crash storage inventory [$label] ($path): " + - "total=${files.size} otlp=$otlp legacy=$legacy [$summary]$truncated", + 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) diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/CrashDirCleanup.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/CrashDirCleanup.kt new file mode 100644 index 0000000000..377aeab71f --- /dev/null +++ b/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, + nowMs: Long, + minAgeMillis: Long, + ownedSuffix: String = CRASH_OWNED_SUFFIX, +): List = + 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, + 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" +} diff --git a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/FileLogStore.kt b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/FileLogStore.kt index e2fd66f96b..b36893434e 100644 --- a/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/FileLogStore.kt +++ b/OneSignalSDK/onesignal/core/src/main/java/com/onesignal/debug/internal/logging/logger/android/FileLogStore.kt @@ -20,7 +20,7 @@ import kotlin.coroutines.cancellation.CancellationException * still have been writing). * * The logger and the legacy otel module share this one crash directory, so ownership - * is distinguished purely by the [FILE_SUFFIX]: everything the logger writes ends in + * 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. */ @@ -30,7 +30,6 @@ internal class FileLogStore( private val rootDir: File get() = File(rootPath) private companion object { - const val FILE_SUFFIX = ".otlp" const val TAG = "OneSignal" } @@ -40,7 +39,7 @@ 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)) { @@ -65,7 +64,7 @@ internal class FileLogStore( try { val now = System.currentTimeMillis() val allFiles = rootDir.listFiles()?.filter { it.isFile }.orEmpty() - val suffixMatches = allFiles.filter { it.name.endsWith(FILE_SUFFIX) } + val suffixMatches = allFiles.filter { isOwnedCrashFile(it.name) } val readable = suffixMatches .filter { now - it.lastModified() >= minAgeMillis } @@ -124,24 +123,28 @@ internal class FileLogStore( withContext(Dispatchers.IO) { try { val now = System.currentTimeMillis() - val foreign = - rootDir.listFiles()?.filter { file -> - file.isFile && - !file.name.endsWith(FILE_SUFFIX) && - now - file.lastModified() >= minAgeMillis + 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() - for (file in foreign) { + for (entry in foreign) { + val file = File(rootDir, entry.name) if (file.delete()) { deleted++ - names.add(file.name) + names.add(entry.name) } else { - Logging.warn("FileLogStore: failed to purge unrecognized file ${file.name}") + Logging.warn("FileLogStore: failed to purge unrecognized file ${entry.name}") } } Logging.info( diff --git a/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/logger/android/CrashDirCleanupTest.kt b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/logger/android/CrashDirCleanupTest.kt new file mode 100644 index 0000000000..21f90abe04 --- /dev/null +++ b/OneSignalSDK/onesignal/core/src/test/java/com/onesignal/debug/internal/logging/logger/android/CrashDirCleanupTest.kt @@ -0,0 +1,83 @@ +package com.onesignal.debug.internal.logging.logger.android + +import io.kotest.core.spec.style.FunSpec +import io.kotest.matchers.shouldBe +import io.kotest.matchers.string.shouldContain +import io.kotest.matchers.string.shouldNotContain + +/** + * Pure-JVM coverage for crash-dir ownership / inventory helpers. Runs without Robolectric so Jacoco + * counts these lines (Roboelectric Android shells are not attributed in this project's reports). + */ +class CrashDirCleanupTest : FunSpec({ + + val now = 100_000L + + test("isOwnedCrashFile recognizes only the logger suffix") { + isOwnedCrashFile("123-abc.otlp") shouldBe true + isOwnedCrashFile("1784621689841") shouldBe false + isOwnedCrashFile("stale.tmp") shouldBe false + } + + test("selectUnrecognizedEntries keeps owned and too-young foreign files") { + val entries = + listOf( + CrashDirEntry("123-abc.otlp", lastModifiedMs = now - 60_000), + CrashDirEntry("too-young-legacy", lastModifiedMs = now - 100), + CrashDirEntry("stale-legacy", lastModifiedMs = now - 10_000), + CrashDirEntry("stale.tmp", lastModifiedMs = now - 60_000), + ) + + val selected = + selectUnrecognizedEntries( + entries = entries, + nowMs = now, + minAgeMillis = 5_000, + ) + + selected.map { it.name } shouldBe listOf("stale-legacy", "stale.tmp") + } + + test("selectUnrecognizedEntries is empty when only owned records exist") { + val selected = + selectUnrecognizedEntries( + entries = listOf(CrashDirEntry("123-abc.otlp", lastModifiedMs = now - 60_000)), + nowMs = now, + minAgeMillis = 0, + ) + + selected shouldBe emptyList() + } + + test("formatCrashDirInventory reports empty directories") { + formatCrashDirInventory( + label = "before-upload", + path = "/cache/crashes", + entries = emptyList(), + nowMs = now, + maxSample = 20, + ) shouldBe "OneSignal: Crash storage inventory [before-upload] (/cache/crashes): empty" + } + + test("formatCrashDirInventory counts otlp vs legacy and bounds the sample") { + val entries = + (1..25).map { index -> + val name = if (index <= 3) "$index.otlp" else "legacy-$index" + CrashDirEntry(name, lastModifiedMs = now - index * 1_000L, lengthBytes = index.toLong()) + } + + val line = + formatCrashDirInventory( + label = "after-cleanup", + path = "/cache/crashes", + entries = entries, + nowMs = now, + maxSample = 5, + ) + + line shouldContain "total=25 otlp=3 legacy=22" + line shouldContain "name=1.otlp" + line shouldContain "…(+20 more)" + line shouldNotContain "name=legacy-25" + } +}) From 23fa36e574a73e852545f45dc7467dd450fc9b99 Mon Sep 17 00:00:00 2001 From: Fadi George Date: Mon, 27 Jul 2026 18:06:47 -0700 Subject: [PATCH 3/4] test: [SDK-4939] raise callback timeout on immediate-drop notification flakes The suite default EXTERNAL_CALLBACKS_TIMEOUT (10ms) can elapse before Dispatchers.IO runs preventDefault(true), so display is attempted against an unstubbed mock on slow CI runners. Co-authored-by: Cursor --- .../generation/NotificationGenerationProcessorTests.kt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/generation/NotificationGenerationProcessorTests.kt b/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/generation/NotificationGenerationProcessorTests.kt index 64dd10eafb..b992a70565 100644 --- a/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/generation/NotificationGenerationProcessorTests.kt +++ b/OneSignalSDK/onesignal/notifications/src/test/java/com/onesignal/notifications/internal/generation/NotificationGenerationProcessorTests.kt @@ -272,6 +272,10 @@ class NotificationGenerationProcessorTests : FunSpec({ test("processNotificationData should immediately drop the notification when will display callback indicates to") { // Given val mocks = Mocks() + // Same CI flake as the preventDefault-twice cases below: the suite default timeout (10ms) + // can elapse before Dispatchers.IO runs the callback, so discard is never set and display + // is attempted against an unstubbed mock. + every { mocks.notificationGenerationProcessor getProperty "EXTERNAL_CALLBACKS_TIMEOUT" } answers { 1_000L } coEvery { mocks.notificationLifecycleService.externalRemoteNotificationReceived(any()) } just runs coEvery { mocks.notificationLifecycleService.externalNotificationWillShowInForeground(any()) } answers { val willDisplayEvent = firstArg() @@ -288,6 +292,7 @@ class NotificationGenerationProcessorTests : FunSpec({ test("processNotificationData should immediately drop the notification when received event callback indicates to") { // Given val mocks = Mocks() + every { mocks.notificationGenerationProcessor getProperty "EXTERNAL_CALLBACKS_TIMEOUT" } answers { 1_000L } coEvery { mocks.notificationLifecycleService.externalRemoteNotificationReceived(any()) } answers { val receivedEvent = firstArg() receivedEvent.preventDefault(true) From 8034317c6b9b6888c4bdd991258bfbbea22e7bef Mon Sep 17 00:00:00 2001 From: Fadi George Date: Mon, 27 Jul 2026 18:20:00 -0700 Subject: [PATCH 4/4] fix(ci): [SDK-4939] evaluate skip-coverage label outside shell quoting Embedding toJson(labels) in a double-quoted shell assignment breaks when a label contains spaces (e.g. Skip Coverage Check), so the bypass step aborts before coverage runs. Co-authored-by: Cursor --- .github/workflows/ci.yml | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 6e857b1947..d7d1f6f677 100644 --- a/.github/workflows/ci.yml +++ b/.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'