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(proteus): prevent missing messages by using transactions [WPB-10873] #2992

Merged

Conversation

vitorhugods
Copy link
Member

@vitorhugods vitorhugods commented Sep 4, 2024

BugWPB-10873 Version 4.8 does not deliver all attachments in a group chat


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

Some clients reported missing messages.
We found that it is not that hard to kill the app by swiping it away between decryption and DB insertion.

Solutions

  • CoreCrypto doesn't support transactions yet: every decrypt persist state and if the app dies after decrypting, the message is forever gone.
  • CryptoBox does to some degree. It doesn't support rolling back a transaction gracefully. It requires recreating a CoreCrypto instance using the same files. So we can at least simulate app killing by recreating this.

By not saving the session before inserting the messages into the DB, we can try to process this event again and recover this message.

Changed the API so instead of returning ByteArray, the Proteus clients will accept a lambda to process the output.
After allowing processing the output, the Proteus clients will persist the session.
CoreCrypto (that doesn't support saving session later) will decrypt and call the lambda normally.

Testing

Test Coverage

  • I have added automated test to this contribution

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

Some clients reported missing messages.
We found that it is not that hard to kill the app by swiping it away between decryption and DB insertion.
CoreCrypto doesn't support transactions yet. So we're only tackling CryptoBox at the moment, but the API changes are adapting CoreCrypto for the future as well.

By not saving the session before inserting the messages into the DB, we can try to process this event again and recover this message.
@vitorhugods vitorhugods requested review from a team, typfel, alexandreferris, borichellow, ohassine and saleniuk and removed request for a team September 4, 2024 11:24
@echoes-hq echoes-hq bot added the echoes: unplanned Any work item that isn’t part of the product or technical roadmap. label Sep 4, 2024
JS Cryptobox doesn't have it.
iOS uses CoreCrypto
Copy link
Contributor

github-actions bot commented Sep 4, 2024

Test Results

3 195 tests  +2   3 088 ✔️ ±0   3m 30s ⏱️ +7s
   548 suites ±0      107 💤 +2 
   548 files   ±0          0 ±0 

Results for commit e6a5adb. ± Comparison against base commit b12a05b.

♻️ This comment has been updated with latest results.

@datadog-wireapp
Copy link

datadog-wireapp bot commented Sep 4, 2024

Datadog Report

Branch report: fix/proteus/use-transactions-for-cryptobox
Commit report: 04d1984
Test service: kalium-jvm

✅ 0 Failed, 3088 Passed, 107 Skipped, 12.08s Total Time

It seems that CryptoBox has static data across instances on JVM, so it can't be tested there either
Copy link

sonarqubecloud bot commented Sep 4, 2024

@MohamadJaara MohamadJaara marked this pull request as draft September 4, 2024 16:19
@vitorhugods vitorhugods marked this pull request as ready for review September 12, 2024 15:37
@vitorhugods vitorhugods added this pull request to the merge queue Sep 12, 2024
Merged via the queue into release/candidate with commit 987b782 Sep 12, 2024
22 checks passed
@vitorhugods vitorhugods deleted the fix/proteus/use-transactions-for-cryptobox branch September 12, 2024 16:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
echoes: unplanned Any work item that isn’t part of the product or technical roadmap. 👕 size: L type: bug / fix 🐞
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants