feat: [SDK-4978] add iOS KMP crash capture and upload - #1713
Conversation
There was a problem hiding this comment.
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/logon 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
cancelnever releasesactiveOwner/ process-wide wedge (A, C, D) —cancelonly stripspendingUploads. 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 aftertelemetry.shutdown().isOneSignalAtFaultis too coarse (A, B, C, D) — whole-stack substring"OneSignal"vialocalizedCaseInsensitiveContains: false positives for host exceptions inside swizzled/callback stacks; false negatives under static linking / stripped symbols; locale-sensitive matching for a programmatic identifier.shutdownflipslifecycleinactive beforetelemetry.shutdown()(B, D) — final export/shutdown sends are refused as retryable transport failure (statusCode: -1). In-flightsendcan also race past theisEnabledcheck.- Fatal-path logging via
OneSignalLog(C) —capturecatch (and reporter ->IOSLogger) can walk listeners and present alert UI when_alertLogLevelallows — unsafe on the uncaught-exception path. saveCrashsuccess is discarded (C, D) —_ = try ...;FileLogStore.saveconverts I/O failures tofalseand does not throw, so persist failures can be silent.
Consider
- 5s
minFileAgeForReadMillis+ once-per-start uploader (B) — fast relaunch can skip finalized.otlpuntil another init. - Public
OSRemoteLoggerProtocol.start()(A, B, D) — contradicts no public API changes; two-phase init silently no-ops HTTP untilstart(). initialize()silent no-op whenSelf.active != nil, no re-arm (A, D).- Unregister mid-flight drops persistence (C).
start()holdslifecycleOperationLockacross uploader kickoff (C).- Crash dir not partitioned by app id (B).
exception.reason ?? exception.descriptionmay embeduserInfo(D).
Noted
currentThreadNameduplicated vsOSLoggerPlatformProvider(A, D)- Hand-authored
pbxprojIDs (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.
Sent by Cursor Automation: Untitled
abdulraqeeb33
left a comment
There was a problem hiding this comment.
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 samestart(), so a fast relaunch is not skipped until the next init. - Persist-failure
catchdoes run. KMPLogTelemetryCrashImplthrows ifFileLogStore.savereturns false, so the Swiftcatchis not dead. The remaining issue is the fatal-pathOneSignalLog/ alert UI the bot already flagged. - Stacked on #1703. Crash uploads go through the same private
URLSessionas remote logs. No need to relitigate consent here, but this is more/sdk/logtraffic 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
left a comment
There was a problem hiding this comment.
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:
start()holdslifecycleOperationLockacross the uploader kickoff (same deadlock class as the diagnostic listener test).cancel(active)starts the nextLogCrashUploaderwhile the cancelled job is still in its 5s min-age delay, so two file stores can POST the same crash directory.OneSignalKMPis 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.
443ac01 to
edd3ceb
Compare
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>
edd3ceb to
d525893
Compare
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>


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/logon the next enabled SDK initialization.Scope
Testing
Unit testing
OSLoggerAdaptersTestsandOSRemoteLoggingControllerTests: 28 tests passed.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/logupload and crash-file deletion.Affected code checklist
Checklist
Overview
Testing
Final pass
Made with Cursor