Page MenuHomePhabricator

[lib] Check the timestamps when adding thread members
ClosedPublic

Authored by tomek on Sep 2 2024, 1:36 AM.
Tags
None
Referenced Files
F3341683: D13216.diff
Thu, Nov 21, 11:26 PM
Unknown Object (File)
Sun, Nov 10, 1:56 AM
Unknown Object (File)
Sat, Nov 9, 7:58 PM
Unknown Object (File)
Thu, Nov 7, 5:50 PM
Unknown Object (File)
Fri, Nov 1, 5:54 AM
Unknown Object (File)
Fri, Nov 1, 5:36 AM
Unknown Object (File)
Mon, Oct 28, 9:27 PM
Unknown Object (File)
Thu, Oct 24, 5:38 AM
Subscribers

Details

Summary

Check the timestamp and don't update the membership when the timestamp is more recent than the operation.

https://linear.app/comm/issue/ENG-9113/update-addmembersspec

Depends on D13215

Test Plan

Tested mostly in combination with the rest of the stack.
Check if the timestamp gets properly updated: create an operation at T+1 time which adds u1, u2, and u3, and an operation at T which adds u3, and u4. The timestamps should be T+1, T+1, T+1, T respectively. Also, the membership array should contain all the members without any repetition.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Sep 2 2024, 1:57 AM
lib/shared/dm-ops/add-members-spec.js
86 ↗(On Diff #43823)

We're assuming the oldest possible update time.

inka added inline comments.
lib/shared/dm-ops/add-members-spec.js
90–92 ↗(On Diff #43833)
This revision is now accepted and ready to land.Sep 3 2024, 3:33 AM
lib/shared/dm-ops/add-members-spec.js
90–92 ↗(On Diff #43833)

My proposition will break in case the equality comes from the if above being run - we won't add the user to members. But we should still handle the case where this timestamp collision is a result of two separate requests

lib/shared/dm-ops/add-members-spec.js
90–92 ↗(On Diff #43833)
lib/shared/dm-ops/add-members-spec.js
94–97 ↗(On Diff #43932)

Why do we bump the timestamp even if we continue on line 100 below?

114–121 ↗(On Diff #43932)

If no changes end up being applied, is there a way we can skip this update, instead of propagating an empty update?

lib/shared/dm-ops/add-members-spec.js
94–97 ↗(On Diff #43932)

It is to handle the following scenario:

  1. We receive an operation about a user being added at time T
  2. We don't receive an operation about the user leaving the thread at T+1
  3. We receive an operation about the user being added at T+2

We want to keep the user in the membership table, but we should update the timestamp so that when we receive the operation from T+1 we don't remove the user.

114–121 ↗(On Diff #43932)

Obviously, there is, but the case where no updates were made is a relly unlikely edge case, and don't think is worth introducing. If you feel strongly, I can add that (and review all the specs to check whether we can skip updates also there). Created https://linear.app/comm/issue/ENG-9232/consider-skipping-creating-updates-when-there-are-no-changes-to-a to track.

lib/shared/dm-ops/add-members-spec.js
94–97 ↗(On Diff #43932)

Thanks for explaining!

114–121 ↗(On Diff #43932)

Appreciate the explanation. I don't feel strongly, and didn't have the context that it's unlikely. I don't feel strongly, so I'll go ahead and cancel the task you created – sorry for the churn!