Skip to content

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

Merged
fadi-george merged 4 commits into
mainfrom
ar/sdk-4939
Jul 28, 2026
Merged

feat(logger): [SDK-4939] wire Android FileLogStore for shared KMP legacy purge#2691
fadi-george merged 4 commits into
mainfrom
ar/sdk-4939

Conversation

@abdulraqeeb33

@abdulraqeeb33 abdulraqeeb33 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Linear: SDK-4939

The KMP logger module and the legacy otel module share one on-disk crash directory ({cacheDir}/onesignal/otel/crashes). Ownership is distinguished by suffix — logger writes *.otlp; legacy otel writes bare-millis filenames plus stray .tmps.

Purge orchestration lives in shared KMP (OneSignal-KMP-SDK#8LogCrashUploaderILogFileStore.deleteUnrecognizedEntries(minAgeMillis)). This PR provides the Android store implementation and rollout inventory logs.

Changes

  • Submodule — pins OneSignal-KMP-SDK to KMP DatabaseNotClosedException raised when notification arrives #8 (post-upload / finally purge, age-gated foreign deletes, cancellation-safe).
  • FileLogStore.deleteUnrecognizedEntries(minAgeMillis) — Android implementation; deletes non-.otlp entries at least minAgeMillis old; preserves owned .otlp and too-young foreign files.
  • OneSignalCrashUploaderWrapper — before/after crash-dir inventory when the logger module is active (no host-side purge call).
  • FileLogStoreTest — stale purge / young preserved / keep-owned / missing-dir / idempotency.

Merge order

  1. Land OneSignal-KMP-SDK#8 first.
  2. Merge this PR (re-pin submodule to the merged KMP SHA if the tip moves).

Test plan

  • :OneSignal:core:compileDebugKotlin (after submodule pin)
  • KMP :kmp:testDebugUnitTest --tests "*LogCrashTest*"
  • :OneSignal:core:testDebugUnitTest --tests "*FileLogStoreTest*"
  • Manual: with SDK_CUSTOM_LOGGING on, seed a stale legacy file in the crash dir, confirm KMP purge removes it and after-cleanup inventory logs legacy=0

Made with Cursor

@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

📊 Diff Coverage Report

Diff Coverage Report (Changed Lines Only)

Gate: aggregate coverage on changed executable lines must be ≥ 80% (JaCoCo line data for lines touched in the diff).

Changed Files Coverage

  • OneSignalCrashUploaderWrapper.kt: 0/31 touched executable lines (0.0%) (58 touched lines in diff)
    • 31 uncovered touched lines in this file
  • CrashDirCleanup.kt: 28/29 touched executable lines (96.6%) (67 touched lines in diff)
  • FileLogStore.kt: 0/59 touched executable lines (0.0%) (96 touched lines in diff)
    • 59 uncovered touched lines in this file

Overall (aggregate gate)

28/119 touched executable lines covered (23.5% — requires ≥ 80%)

Per-file detail (informational; gate is aggregate above):

  • OneSignalCrashUploaderWrapper.kt: 0.0% (31 uncovered touched lines)

  • FileLogStore.kt: 0.0% (59 uncovered touched lines)

❌ Coverage Check Failed

Aggregate coverage on touched lines is 23.5% (minimum 80%).

📥 View workflow run

@abdulraqeeb33 abdulraqeeb33 changed the title feat(logger): [SDK-4939] harden legacy crash-file cleanup when logger is active feat(logger): [SDK-4939] harden legacy crash-file cleanup (host + shared KMP purge) Jul 24, 2026
@abdulraqeeb33

Copy link
Copy Markdown
Contributor Author

Submodule re-pinned to KMP e636c2b (exception-safe purge finally from KMP #8 review follow-up).

@abdulraqeeb33 abdulraqeeb33 changed the title feat(logger): [SDK-4939] harden legacy crash-file cleanup (host + shared KMP purge) feat(logger): [SDK-4939] wire Android FileLogStore for shared KMP legacy purge Jul 24, 2026
@abdulraqeeb33
abdulraqeeb33 force-pushed the ar/sdk-4939 branch 3 times, most recently from 06297f7 to 8138ef3 Compare July 24, 2026 17:14
@abdulraqeeb33
abdulraqeeb33 requested a review from a team July 24, 2026 17:22
@fadi-george

Copy link
Copy Markdown
Contributor

A few items to address before merging:

  1. CI currently fails Spotless, so the new tests have not run. Please apply the import-order fix and confirm FileLogStoreTest passes.
  2. The submodule is pinned to PR-head SHA 3ececce7, but KMP DatabaseNotClosedException raised when notification arrives #8 was squash-merged as 4864ae1b4265baa433ce60a3588aa153689341ff. Please re-pin to the merged SHA.
  3. Avoid Logging.info() inside crash-path FileLogStore.save(). Logging synchronously invokes app listeners, and a listener exception can make a successfully persisted record return false. Raw Logcat would be safer there.
  4. Please rethrow CancellationException from the Android wrapper and the touched suspend store paths instead of swallowing it through Throwable.
  5. Bound the crash-directory inventory sample. It currently builds an unlimited filename summary before starting the uploader, and Logcat will truncate large output anyway.

The core purge classification and age-gating otherwise look good.

@abdulraqeeb33

Copy link
Copy Markdown
Contributor Author

Addressed in the latest push:

  1. Spotless — import order fixed; :OneSignal:core:spotlessKotlinCheck + FileLogStoreTest pass locally.
  2. Submodule pin — re-pinned to merged KMP v0.1.1 / 4864ae1b4265baa433ce60a3588aa153689341ff.
  3. Crash-path save() — uses raw android.util.Log instead of Logging.info/warn so app listeners cannot flip a successful write to false.
  4. CancellationCancellationException is rethrown from the uploader wrapper and from listReadable / delete / deleteUnrecognizedEntries.
  5. Inventory bound — per-file sample capped at 20 entries with an …(+N more) suffix.

@fadi-george fadi-george left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix failing build/coverage checks too?

…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 <cursoragent@cursor.com>
@fadi-george fadi-george added the Skip Coverage Check To skip coverage check in the run label Jul 27, 2026
…h-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 <cursoragent@cursor.com>
@fadi-george

Copy link
Copy Markdown
Contributor

Rebased onto latest main and addressed the newest review notes:

  1. Suspend / race — replied on the thread and added a call-site comment: LogCrashUploader.start() is suspend and finishes upload + finally purge before after-cleanup inventory runs.
  2. Coverage — Robolectric Kotest shells are not attributed by Jacoco here (same constraint documented for AnrCheckEvaluator). Extracted ownership/inventory helpers to pure-JVM CrashDirCleanup + CrashDirCleanupTest, and applied the Skip Coverage Check label for the remaining Android shell lines.

fadi-george and others added 2 commits July 27, 2026 18:06
…n 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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
@fadi-george
fadi-george merged commit c27974d into main Jul 28, 2026
5 checks passed
@fadi-george
fadi-george deleted the ar/sdk-4939 branch July 28, 2026 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip Coverage Check To skip coverage check in the run

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants