feat(errortracking): capture native ndk crashes from tombstones - #659
feat(errortracking): capture native ndk crashes from tombstones#659cat-ph wants to merge 16 commits into
Conversation
posthog-android Compliance ReportDate: 2026-08-07 21:01:38 UTC ✅ All Tests Passed!46/46 tests passed Capture Tests✅ 29/29 tests passed View Details
Feature_Flags Tests✅ 17/17 tests passed View Details
|
…ture # Conflicts: # posthog-android/src/main/java/com/posthog/android/PostHogAndroid.kt # posthog/api/posthog.api # posthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.kt
| } | ||
|
|
||
| properties?.let { | ||
| postHog.capture( |
There was a problem hiding this comment.
we should attach the cached properties as we do for error tracking hard crashes (iOS)
not a blocker for this PR, but an improvement we'd need to make at some point to avoid issues with error investigation
eg sdk version 1.0.0, app version 2.0.0, but the app got upgraded after sending this error, customers would be investigating the wrong version eg 2.0.1 instead
this should be written down in the docs/config since its an important caveat that can waste lots of hours of investigation
There was a problem hiding this comment.
I have a draft docs PR and will mention it there: PostHog/posthog.com#19035
I definitely agree and we can follow-up separately IMO, it probably needs its own design 🤔 (I can open an issue and take a look after)
| private var postHog: PostHogInterface? = null | ||
|
|
||
| private companion object { | ||
| private const val LAST_CAPTURED_TIMESTAMP_KEY = "nativeCrashLastCapturedTimestamp" |
There was a problem hiding this comment.
blocking: This watermark needs to be treated as SDK-internal, project-level state. Because the key is not included in PostHogPreferences.ALL_INTERNAL_KEYS, PostHogSharedPreferences.getAll() returns it as a registered property and buildProperties() attaches nativeCrashLastCapturedTimestamp to subsequent customer events. It is also absent from PostHog.reset()'s preserved keys, so reset/logout clears the deduplication cursor and the same retained tombstones are captured again after restart.
There is also a durability issue: setValue() ultimately uses SharedPreferences.Editor.apply(), which updates memory synchronously but writes to disk asynchronously without reporting failures. An abrupt process death before that write completes can lose the cursor and recapture an already queued crash on the next launch. Could we add the key to ALL_INTERNAL_KEYS, preserve it across reset(), and persist this marker synchronously in project-scoped storage before treating the record as acknowledged?
There was a problem hiding this comment.
combined the reply in this one #659 (comment) 😆 but updated with a NativeCrashWatermarkStore SharedPreferences file, I think that should work
|
|
||
| // Advance per record — unparsable ones too, retrying can't succeed — | ||
| // so dying mid-scan never re-captures already-reported crashes. | ||
| preferences.setValue(LAST_CAPTURED_TIMESTAMP_KEY, exitInfo.timestamp) |
There was a problem hiding this comment.
blocking: This advances the watermark without knowing whether capture() durably queued the event. capture() returns Unit, and queue submission/storage failures are swallowed, so a rejected executor, serialization/disk failure, or close() disabling the client while this scanner is still running can drop the event while permanently marking the crash as handled. Could we advance the watermark only after acknowledged durable queue persistence, and ensure uninstall/close cancels or waits for the scanner so it cannot acknowledge a dropped capture?
There was a problem hiding this comment.
yeah, great catch; I added the separate own posthog-native-crash SharedPreferences file (NativeCrashWatermarkStore), written with commit()
however I didn't do the durable capture() because I think right now it's fire-and-forget and we don't really ack that 🤔 I would've rathered duplicated than losing silently, I think we need to do some more work to have a ack way
|
A few additional follow-ups from the review:
|
|
Two additional behavioral points to address before a stable release:
|
| override fun install(postHog: PostHogInterface) { | ||
| this.postHog = postHog | ||
|
|
||
| if (integrationInstalled || Build.VERSION.SDK_INT < Build.VERSION_CODES.S) { |
There was a problem hiding this comment.
blocking: @Volatile only guarantees visibility; the check followed by assignment is a compound operation and is not atomic, so concurrent installs can both observe false and start duplicate scanners. uninstall() also clears the process-wide flag from any integration instance, even one that did not acquire it, which can allow another scanner to start while the original is still active. Could we use atomic acquisition (for example AtomicBoolean.compareAndSet), track ownership per integration instance, and release the guard only from the owner after its scanner has terminated?
There was a problem hiding this comment.
I think this was copied from PostHogErrorTrackingAutoCaptureIntegration and might happen there too, but fixed it here and we can follow-up
|
left a few comments @cat-ph its in the right direction |
|
you can also check the https://github.com/abovevacant/epitaph TombstoneDecoder impl for parsing the exit metadata |
Atomic per-process ownership of the scanner guard, injectable single-thread executor, uninstall cancels the scan, all retained exit records scanned before reason filtering, and the watermark advances per record through the synchronous store.
Prompt To Fix All With AI### Issue 1
posthog-android/src/main/java/com/posthog/android/errortracking/PostHogNativeCrashIntegration.kt:143
**Timestamp watermark drops tied records**
If two retained native-crash records have the same exit timestamp and scanning stops after the first is acknowledged, the strict `timestamp > watermark` filter permanently excludes the unprocessed record on the next launch, causing its exception event to be lost.
### Issue 2
posthog-android/src/main/java/com/posthog/android/errortracking/PostHogNativeCrashIntegration.kt:41
**Internal integration exposed publicly**
This SDK-managed integration is included in the generated public API without `@PostHogInternal`, presenting its constructor and lifecycle methods as supported consumer API and creating unnecessary compatibility obligations.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(errortracking): harden the native cr..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🦔 ReviewHog reviewed this pull requestFound 0 must fix, 1 should fix, 3 consider. Published 4 findings (view the review). |
|
ReviewHog Alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏 |
![posthog[bot]](https://avatars.githubusercontent.com/in/1203049?s=60&v=4){kind=small}
There was a problem hiding this comment.
ReviewHog Report
Feature
Issues: 4 issues
Files (9)
posthog-android/src/main/java/com/posthog/android/errortracking/PostHogNativeCrashIntegration.ktposthog-android/src/main/java/com/posthog/android/internal/errortracking/NativeCrashWatermarkStore.ktposthog-android/src/main/java/com/posthog/android/internal/PostHogAndroidUtils.ktposthog-android/src/main/java/com/posthog/android/PostHogAndroid.ktposthog-android/api/posthog-android.apiposthog/src/main/java/com/posthog/errortracking/PostHogErrorTrackingConfig.ktposthog/src/main/java/com/posthog/internal/PostHogRemoteConfig.ktposthog/api/posthog.api.changeset/native-crash-capture.md
What were the main changes
- New PostHogNativeCrashIntegration scans ApplicationExitInfo history on startup for REASON_CRASH_NATIVE records (API 31+), parses tombstones, and captures one
$exceptionper crash via a single-thread executor with atomic install/uninstall guarding against concurrent scanners - NativeCrashWatermarkStore persists a synchronous (commit()), device-scoped dedup watermark in its own SharedPreferences file, isolated from the shared preferences/registered-properties store and immune to reset()
- Watermark advanced per-record only after capture() returns, one record at a time, and interruptible via executor shutdown on uninstall
- Requests all retained exit records (maxNum=0) then filters by reason/timestamp, avoiding starvation by newer non-crash exits under a positive cap
- Wired into PostHogAndroid.kt behind new
errorTrackingConfig.captureNativeCrashesopt-in flag, added as a class-body property (not a constructor param) to avoid breaking the synthetic default-arguments constructor for existing Kotlin callers - New PostHogRemoteConfig.isNativeCrashCaptureEnabled() combining the remote exception-autocapture toggle with the local captureNativeCrashes flag
- New getActivityManager() helper in PostHogAndroidUtils.kt; generated API surface and changeset updates
Sibling processes can die in the same millisecond; advancing the watermark on the first record of a tie would orphan the rest if the scan dies mid-group.
Robolectric integration tests for reason filtering, watermark acknowledgement, tied-timestamp retry, the process-wide scanner guard, and the remote toggle, plus PostHogAndroid registration tests for the captureNativeCrashes flag.
| exitInfo.traceInputStream?.use { stream -> | ||
| coercer.toPostHogProperties(parser.parse(stream)) | ||
| } | ||
| } catch (e: Throwable) { |
There was a problem hiding this comment.
blocking: This catch turns any tombstone read/parser failure into null, but the loop still advances the timestamp watermark below. A transient IOException or unsupported tombstone is therefore permanently lost; for tied timestamps, a later successful record acknowledges the failed sibling too. Please distinguish a documented missing trace from a parse failure and stop or propagate on the latter so it is not acknowledged.
There was a problem hiding this comment.
soooo I tried to do that in c93f163, just that when a tombstone is fully read and doesn't parse, I ack'd it so it doesn't poison-pill ourselves (since future reads still won't parse)
|
@cat-ph Could you take another look at the four follow-ups in this review comment? They still appear to be present on the current head:
Could you either address these or reply with the rationale and intended follow-up for anything being deferred? |
|
did another pass after commits, a few small things to be fixed but almost there |
… in_app by app paths The same ELF can be mapped at multiple bases, and frames from a base without its own image entry would not symbolicate. in_app now matches the app's native library dir, APKs, and data dir instead of everything under /data.
…e failures separate
A transient tombstone read failure aborts the scan without acknowledging so the next launch retries it, while a tombstone that read fully but does not parse is acknowledged and skipped, because retrying it forever would block every newer crash behind it. Scanning is restricted to the main process, since the exit history spans the whole package while the guard and watermark are process-local. A live remote disable now aborts an in-flight scan, and the executor is created per acquisition so a later re-enable schedules a fresh scanner.
… process android:process on the application element renames the default process away from the package name, so the package-name comparison disabled scanning in every process of such apps. Covered by renamed-default-process and secondary-process tests.
I .. totally missed that reply sorry @marandaneto 🤦 addressed all 4! plus the others and small test fixes |
2 participants
💡 Motivation and Context
Native (NDK) crashes kill the process before any JVM handler runs, so today they are invisible to error tracking. This adds capture for them without shipping any native code in the SDK: on startup, the SDK reads the crash records the OS kept via
ApplicationExitInfo(REASON_CRASH_NATIVE, Android 12+), parses the attached tombstone protobuf, and captures one$exceptionevent per crash using the native stack frame contract PostHog already resolves for the Rust and Go SDKs.Per crash, the event carries:
platform: "native",instruction_addr,image_addr, optional client-resolvedfunction/symbol_addr) in canonical bottom-up order$debug_imagesentries derived from the tombstone's per-frame GNU build ids, so the server matches frames to.sosymbols uploaded withposthog-cli symbol-sets uploadSIGSEGV/SEGV_MAPERR at 0x..., abort message when present),$exception_level: fatal, and the original crash timestampDesign notes:
instruction_addris biased by +1: tombstone pcs are already the correct lookup address (the leaf is the faulting instruction and libunwindstack rewinds caller pcs to the call instruction), so the bias cancels the server's uniform -1 return-address adjustment. This is pinned by a cymbal fixture test on the server side.errorTrackingConfig.captureNativeCrashes, additionally gated on the project's exception autocapture remote toggle. A dedicated, synchronous timestamp watermark prevents duplicate capture across launches and survives identity resets, advancing per record so dying mid-scan cannot re-capture.android:processstill need handling before merge.Known limitations (draft): events are associated with the identity at next launch, not at crash time;
$exception_stepsrecorded in the new process may attach to the previous run's crash; API 31+ only (tombstone protos attach from Android 12).💚 How did you test it?
in_appclassification. The tied-timestamp regression test fails when transplanted onto the previous head and passes on this head. Pre-existingPostHogAndroidTestconscrypt failures on that box reproduce onmain(arm64 environment issue, no conscrypt aarch64 linux artifact) and are unrelated.c93f163with feat(gradle-plugin): upload native debug symbols via symbol-sets upload #660ecc097a): installed a debug build on an API 35 ARM64 emulator, triggered a real SIGSEGV (ApplicationExitInforeason 5/status 11), then cold-relaunched. The SDK queued exactly one$exception; another relaunch and an identity reset produced no duplicate.latest-v5source context. Only an actual Play Console upload/distribution remains untested.📝 Checklist
If releasing new changes
pnpm changesetto generate a changeset file