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

Catching EnsureOlmSessionsForDevicesAction errors #4229

Merged
merged 4 commits into from
Oct 19, 2021

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Oct 12, 2021

Fixes #3608

When failing to decrypt a MXCRYPTO_ALGORITHM_OLM event due to BAD_ENCRYPTED_MESSAGE the app can crash if the ensureOlmSessionsForDevicesAction logic fails, which can happen if the claimOneTimeKeysForUsersDevices http request fails due to being offline.

  • Also updates the sendToDeviceTask & oneTimeKeysForUsersDeviceTask executions to retry 3 times

The error handling logic is asynchronous executed meaning the main decryption flow isn't able to react to any further errors.

I've naively fixed this crash by simply catching the source of the error, similar to how the surrounding logic behaves.

…cryption flow

- we currently can't do much but log here as we've asynchronously start the fallback flow, catching the error at least stops a hard crash
@@ -146,29 +146,36 @@ internal class EventDecryptor @Inject constructor(

// offload this from crypto thread (?)
cryptoCoroutineScope.launch(coroutineDispatchers.computation) {
val ensured = ensureOlmSessionsForDevicesAction.handle(mapOf(senderId to listOf(deviceInfo)), force = true)
runCatching { ensureOlmSessionsForDevicesAction.handle(mapOf(senderId to listOf(deviceInfo)), force = true) }.fold(
onSuccess = { sendDummyToDevice(ensured = it, deviceInfo, senderId) },
Copy link
Contributor Author

Choose a reason for hiding this comment

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

extracted the logic out to its own function to avoid too many levels of indentation

Copy link
Member

Choose a reason for hiding this comment

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

OK.
Could you also update sendToDeviceTask.execute(sendToDeviceParams) and use executeRetry (3 retries?)
ALso in the EnsureOlmSessionsForDevicesAction, we should probably executeWith retry the claim oneTimeKeysForUsersDeviceTask.execute(claimParams)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, maybe @BillCarsonFr can have a look too.
Can you add a small file for the changelog please?

@bmarty bmarty requested a review from BillCarsonFr October 12, 2021 16:23
@github-actions
Copy link

github-actions bot commented Oct 12, 2021

Unit Test Results

  48 files  +  4    48 suites  +4   55s ⏱️ +6s
  91 tests +  4    91 ✔️ +  4  0 💤 ±0  0 ±0 
244 runs  +16  244 ✔️ +16  0 💤 ±0  0 ±0 

Results for commit c8a8d2e. ± Comparison against base commit 7338982.

♻️ This comment has been updated with latest results.

withContext(coroutineDispatchers.io) {
val sendToDeviceParams = SendToDeviceTask.Params(EventType.ENCRYPTED, sendToDeviceMap)
try {
sendToDeviceTask.executeRetry(sendToDeviceParams, remainingRetry = SEND_TO_DEVICE_RETRY_COUNT)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sendToDeviceTask is now executed with the ability to retry 3 times

@@ -72,7 +74,7 @@ internal class EnsureOlmSessionsForDevicesAction @Inject constructor(
Timber.i("## CRYPTO | claimOneTimeKeysForUsersDevices() : $usersDevicesToClaim")

val claimParams = ClaimOneTimeKeysForUsersDeviceTask.Params(usersDevicesToClaim)
val oneTimeKeys = oneTimeKeysForUsersDeviceTask.execute(claimParams)
val oneTimeKeys = oneTimeKeysForUsersDeviceTask.executeRetry(claimParams, remainingRetry = ONE_TIME_KEYS_RETRY_COUNT)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oneTimeKeysForUsersDeviceTask is now executed with the ability to retry 3 times

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

Still OK to me, @BillCarsonFr ?

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

Approved, thx.

@bmarty bmarty merged commit ab0e707 into develop Oct 19, 2021
@bmarty bmarty deleted the feature/adm/decrypt-event-dummy-keys-fallback-crash branch October 19, 2021 14:13
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.

Crash
3 participants