Page MenuHomePhabricator

[lib] Stop advancing message checkpoint when messages send successfully
ClosedPublic

Authored by ashoat on Jul 26 2022, 5:53 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Jul 3, 8:59 AM
Unknown Object (File)
Mon, Jul 1, 9:22 AM
Unknown Object (File)
Mon, Jul 1, 9:22 AM
Unknown Object (File)
Mon, Jul 1, 9:12 AM
Unknown Object (File)
Thu, Jun 27, 6:30 PM
Unknown Object (File)
Wed, Jun 26, 3:39 PM
Unknown Object (File)
Wed, Jun 26, 3:38 AM
Unknown Object (File)
Tue, Jun 25, 11:22 PM

Details

Summary

This addresses ENG-1399.

We have a long history of issues with message checkpoints being advanced and messages being missed. D511 addressed most of the issues, but unfortunately I recently saw this issue reoccur.

While I'm not sure exactly what happened, my suspicion is that the message checkpoint was advanced due to a SEND_TEXT_MESSAGE_SUCCESS from the socket, but the socket was closed before delivering another message with a lower timestamp. We don't have a hard guarantee that a closing socket will deliver any messages with a timestamp lower than a just-confirmed SEND_TEXT_MESSAGE_SUCCESS.

One approach would be for the server to have some way to reject the advance of a message checkpoint based on knowledge that an earlier message has been generated. But that would be rather difficult to implement.

Instead, this diff just stops incrementing the message checkpoint on successful message sends. This will result in some locally-created messages being redelivered, specifically in the case where the user sent the most recent message right before a socket is closed. However it will prevent the very negative user experience of missing a message, and not being able to get it without logging out and back in.

Test Plan

I tested this scenario:

  1. I loaded two web clients (A and B) in my local environment
  2. Client A sends a message and then tabbed away
  3. Client B responds
  4. Client A reconnects to the socket

I made sure that the INCREMENTAL_STATE_SYNC on Client A:

  1. Included both messages from the scenario
  2. Diff not result in a diff on the first message

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable