Skip to content

feat: [SDK-4978] add iOS KMP crash capture and upload - #1713

Open
fadi-george wants to merge 5 commits into
mainfrom
fadi/sdk-4978
Open

feat: [SDK-4978] add iOS KMP crash capture and upload#1713
fadi-george wants to merge 5 commits into
mainfrom
fadi/sdk-4978

Conversation

@fadi-george

Copy link
Copy Markdown
Collaborator

Description

One Line Summary

Adds iOS crash persistence and next-launch upload through the shared KMP logger pipeline.

Details

Motivation

Capture OneSignal-related uncaught Objective-C exceptions synchronously before process termination, then upload retained crash records through /sdk/log on the next enabled SDK initialization.

Scope

  • Composes the KMP crash reporter and uploader in the iOS remote-logging lifecycle.
  • Preserves existing host exception handlers and avoids POSIX signal interception because the Swift/Kotlin persistence path is not async-signal-safe.
  • Serializes uploader runs and prevents requests after remote logging shuts down.
  • Keeps all changes internal with no public API changes.

Testing

Unit testing

  • Added coverage for synchronous persistence, OneSignal attribution, host-handler preservation, exception-handler chaining and reentrancy, uploader serialization, disabled transport, and winning logger activation.
  • Ran OSLoggerAdaptersTests and OSRemoteLoggingControllerTests: 28 tests passed.
  • Verified release iOS Simulator and Mac Catalyst builds.

Manual testing

Not run on a physical device. End-to-end validation requires enabling the remote logging feature flag, forcing an uncaught Objective-C exception, relaunching, and confirming the /sdk/log upload and crash-file deletion.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

Made with Cursor

@fadi-george
fadi-george requested a review from nan-li August 12, 2026 20:05

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Multi-model adversarial review (interrogate)

PR: feat: [SDK-4978] add iOS KMP crash capture and upload
Models: claude-fable-5-thinking-xhigh · gpt-5.6-sol-xhigh · cursor-grok-4.5-high-fast · claude-opus-5-thinking-high
Base...Head: 110a56a6...30e95514

Intent

Compose the KMP crash reporter/uploader into the iOS remote-logging lifecycle so OneSignal-related uncaught Objective-C exceptions are persisted synchronously before termination, then uploaded via /sdk/log on the next enabled init. Preserve the host exception handler, avoid POSIX signals, serialize uploader runs, and refuse HTTP after shutdown — with no public API changes.

Reviewers

  • A claude-fable-5-thinking-xhigh — 7 findings
  • B gpt-5.6-sol-xhigh — 5 findings
  • C cursor-grok-4.5-high-fast — 6 findings
  • D claude-opus-5-thinking-high — 9 findings

Act On

  1. cancel never releases activeOwner / process-wide wedge (A, C, D)cancel only strips pendingUploads. If the active uploader completion never fires (including after shutdown disables transport), crash upload is stuck for the rest of the process. Also: finish-then-cancel race can start work after telemetry.shutdown().
  2. isOneSignalAtFault is too coarse (A, B, C, D) — whole-stack substring "OneSignal" via localizedCaseInsensitiveContains: false positives for host exceptions inside swizzled/callback stacks; false negatives under static linking / stripped symbols; locale-sensitive matching for a programmatic identifier.
  3. shutdown flips lifecycle inactive before telemetry.shutdown() (B, D) — final export/shutdown sends are refused as retryable transport failure (statusCode: -1). In-flight send can also race past the isEnabled check.
  4. Fatal-path logging via OneSignalLog (C)capture catch (and reporter -> IOSLogger) can walk listeners and present alert UI when _alertLogLevel allows — unsafe on the uncaught-exception path.
  5. saveCrash success is discarded (C, D)_ = try ...; FileLogStore.save converts I/O failures to false and does not throw, so persist failures can be silent.

Consider

  • 5s minFileAgeForReadMillis + once-per-start uploader (B) — fast relaunch can skip finalized .otlp until another init.
  • Public OSRemoteLoggerProtocol.start() (A, B, D) — contradicts no public API changes; two-phase init silently no-ops HTTP until start().
  • initialize() silent no-op when Self.active != nil, no re-arm (A, D).
  • Unregister mid-flight drops persistence (C).
  • start() holds lifecycleOperationLock across uploader kickoff (C).
  • Crash dir not partitioned by app id (B).
  • exception.reason ?? exception.description may embed userInfo (D).

Noted

  • currentThreadName duplicated vs OSLoggerPlatformProvider (A, D)
  • Hand-authored pbxproj IDs (D)
  • Coordinator may be heavier than a process-level drain latch (D)
  • Composition path largely untested end-to-end (A)

Dismissed

  • Pure structure preference without a concrete failure mode beyond the coordinator latch note.
  • Mac Catalyst gaps already gated by #if !targetEnvironment(macCatalyst).

Agreement map

Strongest consensus: fault attribution (4/4) and uploader lifecycle/cancel vs active slot (3/4). Transport/shutdown ordering and silent persist failure are next. Unit tests cover pieces well; they miss shared-coordinator + shutdown races and real release-build stacks.

Inline comments mark Act On anchors. No code changes from this automation.

Open in Web View Automation 

Sent by Cursor Automation: Untitled

@abdulraqeeb33 abdulraqeeb33 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.

Thanks Fadi — composition looks right (sync persist on the crashing thread, KMP encode/upload, host handler chaining, no POSIX signals). I am not restating the Cursor bot Act Ons; those still stand, especially shutdown-vs-transport ordering, cancel/activeOwner, and fault attribution.

Requested change

CI is red on this PR: SwiftLint type_body_length on OSLoggerAdaptersTests (386 / 350). The job fails at lint, before tests run. Please split the new crash-handler tests into OSLogCrashHandlerTests.swift (that also clears the 549-line file_length warning).

Notes the bot did not cover

  • Dismiss the 5s min-age consider. LogCrashUploader.internalStart() already does send → delay(minFileAgeForReadMillis) → send again in the same start(), so a fast relaunch is not skipped until the next init.
  • Persist-failure catch does run. KMP LogTelemetryCrashImpl throws if FileLogStore.save returns false, so the Swift catch is not dead. The remaining issue is the fatal-path OneSignalLog / alert UI the bot already flagged.
  • Stacked on #1703. Crash uploads go through the same private URLSession as remote logs. No need to relitigate consent here, but this is more /sdk/log traffic on that ungated path.

Ticket vs PR body

SDK-4978 done-when mentions non-fatal capture and signal handlers. Worth one line in the description that those are explicitly out of scope (ANR follow-up; signals skipped because the persist path is not async-signal-safe) so it does not read as incomplete ticket work.

@abdulraqeeb33 abdulraqeeb33 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.

Thanks Fadi — the harden commits landed the first-round Act Ons. Two-phase shutdown, the distinct −2 disabled signal, OSCrashLogger on the fatal path, first-throwing-module attribution, and the test split are the right shape.

Still a few items before merge:

  1. start() holds lifecycleOperationLock across the uploader kickoff (same deadlock class as the diagnostic listener test).
  2. cancel(active) starts the next LogCrashUploader while the cancelled job is still in its 5s min-age delay, so two file stores can POST the same crash directory.
  3. OneSignalKMP is missing from the attribution module set.

Inline comments have the suggested fixes. CI red is the same unrelated IAM/User suite as #1703 — OSCore’s 52 tests passed.

Nit: one sentence in the PR body that non-fatal capture and POSIX signals are out of scope (ANR follow-up; signals skipped because persist is not async-signal-safe) so this does not read as incomplete ticket work.

Comment thread iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/OSRemoteLogger.swift Outdated
Base automatically changed from fadi/sdk-4977 to main August 14, 2026 01:11
fadi-george and others added 4 commits August 13, 2026 18:12
Harden shutdown and uploader cancellation races, constrain crash attribution, and keep fatal-path logging isolated from application listeners.

Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid treating host exceptions as OneSignal faults merely because a deeper callback frame belongs to the SDK.

Co-authored-by: Cursor <cursoragent@cursor.com>
Avoid startup reentrancy deadlocks, keep active uploads serialized through completion, and recognize crashes originating in the KMP image.

Co-authored-by: Cursor <cursoragent@cursor.com>
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