Skip to content

feat(logger): purge unrecognized/legacy crash files in shared uploader - #8

Merged
abdulraqeeb33 merged 4 commits into
mainfrom
ar/sdk-4939-purge
Jul 24, 2026
Merged

feat(logger): purge unrecognized/legacy crash files in shared uploader#8
abdulraqeeb33 merged 4 commits into
mainfrom
ar/sdk-4939-purge

Conversation

@abdulraqeeb33

Copy link
Copy Markdown
Collaborator

Summary

Related: OneSignal-Android-SDK SDK-4939

Moves the legacy crash-file cleanup into the shared KMP module so every host (Android now, iOS later) reclaims foreign/legacy files without host-specific glue. This is the shared-module home for the behavior that the Android SDK currently does host-side (OneSignal-Android-SDK#2691).

Changes

  • ILogFileStore.deleteUnrecognizedEntries() — new contract method with a default no-op (safe for test doubles / platforms with no shared-directory legacy). Implementations remove on-disk entries the store does not own, while preserving owned records (including failed / too-young uploads).
  • LogCrashUploader — after the owned-record upload pass (or immediately when remote logging is off), purge unrecognized entries so a later module flip isn't poisoned by stale legacy files. Adds inventory/pass logging.
  • Tests + FakeFileStore — cover: purge after successful uploads, purge when remote logging disabled, and owned records retained on upload failure while legacy files are still purged.

Consumer follow-up

The Android host PR that bumps the submodule to this commit and implements deleteUnrecognizedEntries() in FileLogStore is stacked on #2691. Once this merges, the Android submodule pin will be updated to the merged SHA.

Test plan

  • ./gradlew :kmp:testDebugUnitTestLogCrashTest 10/10 pass (incl. 3 new purge tests)
  • ./gradlew :kmp:spotlessCheck

Made with Cursor

Move the legacy crash-file cleanup into the shared KMP module so every host
(Android today, iOS later) reclaims foreign files without host-specific glue.

- ILogFileStore.deleteUnrecognizedEntries(): new contract method (default no-op
  for test doubles / platforms with no shared-directory legacy) that removes
  on-disk entries the store does not own, while preserving owned records
  (incl. failed / too-young uploads).
- LogCrashUploader: after the owned-record upload pass (or immediately when
  remote logging is off), purge unrecognized entries so a later module flip is
  not poisoned by stale legacy files. Adds inventory/pass logging.
- Tests + FakeFileStore support covering purge after success, purge when remote
  logging disabled, and owned records retained on upload failure while legacy
  files are still purged.

Co-authored-by: Cursor <cursoragent@cursor.com>
Keep the legacy-file sweep exception-safe when listReadable/export fails,
and document that host stores should age-gate unrecognized deletions during
module-flag transitions.

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

Copy link
Copy Markdown
Collaborator Author

Addressed review feedback in e636c2b:

  • purgeUnrecognizedEntries() now runs in a finally around internalStart() so a throwing listReadable/export path still reclaims legacy files
  • Contract note on deleteUnrecognizedEntries recommending a minimal age margin for unrecognized files during module-flag transitions
  • New test: uploaderPurgesUnrecognizedFilesEvenWhenListReadableThrows

Android #2691 submodule re-pinned to this SHA.

@fadi-george

Copy link
Copy Markdown

A couple of issues to address before merging:

  1. purgeUnrecognizedEntries() catches all Exceptions, including coroutine cancellation. Please rethrow CancellationException, particularly for the remote-logging-disabled path where cancellation could otherwise be swallowed.
  2. The contract says implementations must preserve too-young foreign files, but the method receives no age threshold and the tests delete every unrecognized file. Please make the age policy explicit in the API or implementation and add coverage for young versus stale foreign files.

Also, please confirm this will not ship before the Android production implementation lands. In this repo the default remains a no-op and only FakeFileStore overrides it. The new crash path and report inventory logging should ideally remain at DEBUG to avoid unnecessary exposure and log volume.

Rethrow CancellationException from purge/export catch blocks, pass
minFileAgeForReadMillis into deleteUnrecognizedEntries so foreign files
respect the same age policy as owned reads, cover young-vs-stale and
cancellation cases, and keep path/inventory upload logs at DEBUG.

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

Copy link
Copy Markdown
Collaborator Author

Addressed in e28e174:

  1. CancellationpurgeUnrecognizedEntries / export catch blocks now rethrow CancellationException before handling other Exceptions (covered for the remote-logging-disabled path).
  2. Age policydeleteUnrecognizedEntries(minAgeMillis) is now an explicit API parameter. LogCrashUploader passes platformProvider.minFileAgeForReadMillis. Fake + tests cover young vs stale foreign files.
  3. Logging — path / report inventory / per-report upload lines are back at DEBUG. Purge-with-deletions stays at INFO so rollout cleanup is visible; empty purge is DEBUG.

Shipping with Android: yes — this will not ship alone. OneSignal-Android-SDK#2691 pins this submodule and provides the production FileLogStore override (including the age gate). Default no-op remains until a host lands an implementation.

Co-authored-by: Cursor <cursoragent@cursor.com>
@abdulraqeeb33
abdulraqeeb33 merged commit 4864ae1 into main Jul 24, 2026
1 check passed
@abdulraqeeb33
abdulraqeeb33 deleted the ar/sdk-4939-purge branch July 24, 2026 17:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants