-
Notifications
You must be signed in to change notification settings - Fork 1
[OutOfMemoryError] Fix OOM at LogEncrypt.encrypt(...)
#28
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an OutOfMemoryError occurring in the LogEncrypt.encrypt(...) method by temporarily disabling the actual log upload functionality while preserving the encryption logging for debugging purposes.
- Commented out the actual REST client upload call to prevent OOM issues
- Added debug logging to track successful encryption operations
- Modified success message to reflect that uploads are being handled rather than actually uploaded
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
encryptedlogging/src/main/kotlin/com/automattic/encryptedlogging/store/EncryptedLogStore.kt
Outdated
Show resolved
Hide resolved
encryptedlogging/src/main/kotlin/com/automattic/encryptedlogging/store/EncryptedLogStore.kt
Outdated
Show resolved
Hide resolved
FYI: This change is for debugging purposes so that to avoid uploading log files that are too big, and until we figure out how big a log file should be to cause this OOM problem. Then, to try to fix the problem and see if 'logEncrypter.encrypt(...)' could now handle such big log files.
8c6bb2f to
5e85f84
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
encryptedlogging/src/main/kotlin/com/automattic/encryptedlogging/store/EncryptedLogStore.kt
Show resolved
Hide resolved
...gging/src/main/kotlin/com/automattic/encryptedlogging/model/encryptedlogging/LogEncrypter.kt
Outdated
Show resolved
Hide resolved
encryptedlogging/src/main/kotlin/com/automattic/encryptedlogging/store/EncryptedLogStore.kt
Show resolved
Hide resolved
FYI: This 5MB max logs size limit is both arbitrary and empirical because it seems that for (at least) some standard devices, large logs files of 100MB+ can easily cause OOM. Also, this 5MB max logs size limit is chosen because the API accepts logs up to 10MB, but we leave enough headroom for encoding overhead. PS: It is worth to note that we could apply a different kind of a solution (better one), like streaming encryption or chunked processing. But, I don't think it is worth it, nor I think it is worth sending such large log files to the API.
…art" This reverts commit a16584d.
FYI: Even after fixing the logs size problem during encryption, if the file is too big (200MB+), just because of the 'file.readText()', which tries to read the entire file into memory as string, another OOM problem occurs, this time before the encryption logic even runs. By file size checking and truncating the file to the last 10MB (if too big), we are avoiding this OOM issue. The current approach: - Creates a temp file with only the last 10MB of data. - Reads the temp file content and returns it. - Cleans up the temp file in the finally block. - Leaves the original file completely untouched.
5e85f84 to
7ae6f7d
Compare
This reverts commit 6990d7c
16c06ca to
4a7c828
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
encryptedlogging/src/main/kotlin/com/automattic/encryptedlogging/store/EncryptedLogStore.kt
Outdated
Show resolved
Hide resolved
encryptedlogging/src/main/kotlin/com/automattic/encryptedlogging/store/EncryptedLogStore.kt
Outdated
Show resolved
Hide resolved
Now that the initial fix (6990d7c) got reverted (b007944) there is no point referring to a 'MAX_IN_MEMORY_SIZE' constant, but rather a 'MAX_LOG_FILE_SIZE' constant which also adheres to what the API accepts, which is logs up to 10MB. FYI: Although the API accepts logs up to 10MB, we use 5MB to leave enough room for the encryption overhead (and in order to avoid 400 failure with upload turning into invalid requests). PS: Actually, while testing this solution I also tried working with 9MB, then 8MB, then 7MB, all resulting in 400 ('InvalidRequest'). Only when I tried using 6MB the API started accepting the truncated encrypted log. As such, an in order to give enough buffer, I set the final value to 5MB, just in case 6MB isn't enough for every case.
4a7c828 to
a999320
Compare
9c1878e to
f19c0d9
Compare
| val encryptedText = logEncrypter.encrypt(text = encryptedLog.file.readText(), uuid = encryptedLog.uuid) | ||
| val encryptedText = if (encryptedLog.file.length() <= MAX_LOG_FILE_SIZE) { | ||
| logEncrypter.encrypt( | ||
| text = encryptedLog.file.readText(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cap the file at 5 MB but still load it into memory with readText(). This inflates size (UTF-16 + base64) and may still cause OOM on low-RAM devices. Could LogEncrypter.encrypt support ByteArray or InputStream for streaming/chunked encryption to avoid building a full String?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We cap the file at 5 MB but still load it into memory with readText(). This inflates size (UTF-16 + base64) and may still cause OOM on low-RAM devices.
That's true @harrytmthy and thanks for the note here! 💯
Could LogEncrypter.encrypt support ByteArray or InputStream for streaming/chunked encryption to avoid building a full String?
We could, we'll try this first, since the API anyway doesn't accept encrypted log files larger than 10MB and take it from there. 👍
|
👋 @wzieba just a friendly reminder about this! 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, leaving one minor nitpick
encryptedlogging/src/main/kotlin/com/automattic/encryptedlogging/utils/Utils.kt
Show resolved
Hide resolved
PR Comment: #28 #discussion_r2355022277 Co-authored-by: Wojciech Zięba <wojtek.zieba@automattic.com>
|
Thanks for reviewing/testing and leaving your suggestions on this @wzieba , much appreciated! 🙇 ❤️ 🚀 |
Fixes: #16
Closes: AINFRA-1217
(Internal) Discussions: p1754667446732699-slack-C030U03RC8Y + p1757585328963649-slack-C030U03RC8Y
Description
This PR resolves a long standing
OutOfMemory(OOM) error when using this library alongside logs that are of large size.FYI: Actually, this change resolves a couple more (potential) issues like:
ANRs when the library tries to process large logs (no matter if that results in an OOM or not). See for example: AINFRA-1182InvalidRequests when the library is successful in processing large logs (10MB+), but the API respond with400just because of the MAX_FILE_SIZE_IN_BYTES restriction on WPCOM_JSON_API_Encrypted_Logs_Endpoint.Overall, this change:
MAX_LOG_FILE_SIZEof5MB, just because the resulting encrypted text is about1.4larger perMBof a log file, resulting in a final max of about7MBthat is sent to the API. We could have used6MBas the max log file size, which would result in8.4MBof encrypted text sent, but it is better to keep enough buffer just in case, and this way, to safeguard ourselves from anyInvalidRequestresponses from the API.MAX_LOG_FILE_SIZEof5MB, if a log's file size exceeds this max, it is truncated to that5MBof size, skipping the beginning of the log and only keep the more recent logs.Testing Steps
Using the DOAndroid project, update
automattic-encryptedloggingto28-f19c0d92473042e5976c92cd7b88cf641456691a(version which is based on this PR's latest commit) and make sure that encrypted logging could work with logs of large size.Note
I would first advise you to replicate an OOM using your physical/virtual device of choice. To do so, update
automattic-encryptedloggingto28-a16584d1c883c758c29e8e40f8bba0c1320ff567instead. This commit skips the uploading of the encrypted log to the API, and just handles the encryption part.Tip
FYI: You could use the below DOAndroid patch, which add the
Fill The Logdevelopment option into theDeveloperscreen of the app and on click adds10000log lines (or else4MBworth of logs). Adding another0to thatnumberOfLogsval will give you40MBinstead, use what you need:patch
PS: Don't forget to use the
Device Explorerto check on the actual size of the log right before you cause a crash (using anotherCrash The Appdevelopment option from theDeveloperscreen of the app. File log path:/data/data/com.dayoneapp.dayone.debug/files/dayonelogs/com.dayoneapp.dayone.debug YYYY-MM-DD.logFollow the below generic steps with a log of
2MB,10MB,50MBand200MBfile size:App Inspectionfrom AS find theencrypted-logging.dband its soleEncryptedLogModeltable. Notice that the table is empty, there are no log to be sent.Settings->Developerand then clickCrash The App.App Inspectionfrom AS on thatDETACHEDapp (or not, it depends). Now, switch to theATTACHEDapp and notice the db empty. This is because this db entry/log has been already sent to Sentry already, on app start and after the app crashed. To really notice the entry, trying crashing the app again, this time put the device on offline mode first. Doing that you'll be able to see the entry on the db, even when switching to the newly createdATTACHEDapp.Issues. Find the project you're testing for (for exampleday-one-androidor/andpocket-casts-android) and check the latestdebugexceptions. Using DOAndroid for example, you could search forDeveloper caused a crash!and check the latest crash. Now navigate at the bottom and check theuuid. Verify it is the sameUUIDthat you saw on that db entry before.UUIDintoLog UUID, clickViewand make sure you can read the newly uploaded and now decrypted log.For a log of specific size (see above) expect to get the following results, all without any OOM, or ANR, or an
InvalidRequestresponse from the API, and the app should launch instantly, that is, after the crash, without any delay:2MB:10MB:50MB:200MB: