[web][native] Fix retrying sending message when having device without keys
ClosedPublic
Actions

Authored by angelika on Feb 4 2025, 8:57 AM.

Details

Reviewers

tomek
kamil

Commits

rCOMM883e1ca1276d: [web][native] Fix retrying sending message when having device without keys

Summary

https://linear.app/comm/issue/ENG-10141/retrying-message-on-prod-cause-it-sending-multiple-times

To reproduce have a device in a "broken" state (? instead of web/mobile icon). Send a message. It will
fail with log "Keys missing for device ..." but the message will be delivered. Showing that the delivery failed
is ok (we couldn't deliver to the broken device) but after retrying the message will be delivered again and
the other user will see duplicated messages.

The exception is thrown in function returned from useInputStateContainerSendTextMessage. failedOutboundP2PMessageIDs is
set correctly here. But then the exception is caught in sendTextMessageAction and re-thrown without setting failedOutboundP2PMessageIDs.
failedOutboundP2PMessageIDs is then used do determine how we should resend the message.
Therefore setting failedOutboundP2PMessageIDs in sendTextMessageAction fixes the bug.

Test Plan

Have a "broken" device (by logging out with v1 flow or by failing to connect the device)
Send a message and retry.
Verify the other user has no duplicated messages.

Diff Detail

Repository

rCOMM Comm

Branch

graszka22/ENG-10141

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

angelika created this revision.Feb 4 2025, 8:57 AM

Herald added a subscriber: ashoat. · View Herald TranscriptFeb 4 2025, 8:57 AM

Harbormaster completed remote builds in B33382: Diff 46901.Feb 4 2025, 10:02 AM

angelika requested review of this revision.Feb 4 2025, 10:02 AM

Great find!

I would also test retrying multimedia messages and test retires in thin threads (Genesis).

To make testing easier and do it in a more real-life use case without a "broken" device you can just disable internet connection from dev tools, send message (will fail after TB timeout) and then try to retry.

native/input/input-state-container.react.js
580–584 ↗	(On Diff #46901)	Looking at it, I guess we should do the same for multimedia messages where we have exactly the same issue
615–625 ↗	(On Diff #46901)	can we do something like this to avoid creating new `SendMessageError` and overriding `failedOutboundP2PMessageIDs`?
622 ↗	(On Diff #46901)	This isn't safe here - we shouldn't set `failedOutboundP2PMessageIDs` for thin threads (keyserver hosted) messages