Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: SDK detects OutOfMemory after device reboot #3352

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Fix: SDK detects OutOfMemory after device reboot #3352

merged 4 commits into from
Oct 24, 2023

Conversation

sepbehroozi
Copy link
Contributor

@sepbehroozi sepbehroozi commented Oct 19, 2023

📜 Description

If the host app has been terminated due to device reboot, SentrySDK assumes it as a WatchdogTermination crash. In such cases, mostly, the boot time will be mentioned as "a few seconds before this event".

Screenshot 2023-10-19 at 09 52 27

The idea behind this fix is that if the system boot time has been changed since the previous app launch, most likely the app has been terminated due to the reboot and the SentrySDK should not consider it as a WatchdogTermination crash.

💡 Motivation and Context

This change helps reducing the amount of false WatchdogTermination events detected by the SDK.

💚 How did you test it?

There's a new test case added in the SentryWatchdogTerminationsTrackerTests.swift file, which sets the previous app-launch time to different value and expects not sending any OOM event.

📝 Checklist

You have to check all boxes before merging:

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@brustolin
Copy link
Contributor

Thanks @sepbehroozi for contributing. I believe this change makes sense.
Would you mind taking a look at the tests that are failing?

@sepbehroozi
Copy link
Contributor Author

Thanks @sepbehroozi for contributing. I believe this change makes sense.
Would you mind taking a look at the tests that are failing?

Hello @brustolin
Thanks for checking the PR.
Of course. I'll check them later today 👍

Fix formatting issue in SentryWatchdogTerminationLogic.m
Use TestSysctl instead of the SentrySysctl in SentryCrashIntegrationTests's Fixture
@sepbehroozi
Copy link
Contributor Author

Thanks @sepbehroozi for contributing. I believe this change makes sense. Would you mind taking a look at the tests that are failing?

Hey @brustolin. I believe the reason for the failed tests was that the Fixture class of the SentryCrashIntegrationTests doesn't introduce a mock SysctlWrapper (In this case, the TestSysctl()) to the SentryDependencyContainer. That's why the buildCurrentAppState function in the SentryAppStateManager class uses the real SentrySysctl class when getting called from the SentryCrashIntegrationTests tests.
What I thought would be a good solution in this case, setting the sysctlWrapper of SentryDependencyContainer.sharedInstance() to TestSysctl() in the Fixture's initializer. I hope I could make sense of it :))

@codecov
Copy link

codecov bot commented Oct 19, 2023

Codecov Report

Merging #3352 (11087a0) into main (209e288) will increase coverage by 0.059%.
The diff coverage is 92.857%.

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #3352       +/-   ##
=============================================
+ Coverage   89.182%   89.241%   +0.059%     
=============================================
  Files          500       500               
  Lines        54488     54508       +20     
  Branches     19561     19568        +7     
=============================================
+ Hits         48594     48644       +50     
+ Misses        5028      4889      -139     
- Partials       866       975      +109     
Files Coverage Δ
Sources/Sentry/SentryWatchdogTerminationLogic.m 63.636% <100.000%> (+2.097%) ⬆️
...ions/SentryCrash/SentryCrashIntegrationTests.swift 83.987% <100.000%> (+0.048%) ⬆️
...tions/SentryWatchdogTerminationsTrackerTests.swift 96.646% <90.000%> (-0.210%) ⬇️

... and 35 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 209e288...11087a0. Read the comment docs.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

Good catch, thanks, @sepbehroozi 🥳. I also think this change makes sense. I leave it up to @brustolin to give you a final review.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

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

LGTM

@brustolin brustolin merged commit ae9c51b into getsentry:main Oct 24, 2023
56 of 61 checks passed
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.

3 participants