feat(threads): store the "bump_stamps" in the thread subscriptions

[bnjbvr]

See also MSC4308 for details about the bumpstamps. They allow to order two subscriptions, and avoid bad race conditions where one would get a thread subscription by both paginating and sync'ing them.

It's a bit of a shame that the code comparing bumpstamps looks a bit duplicated, but note that, among the 3 implementations, only 2 are duplicated, for real: the last one uses the PersistedThreadSubscription in lieu of StoreThreadSubscription type.

Part of #5038.

[codecov]

Codecov Report

❌ Patch coverage is 81.88406% with 25 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.65%. Comparing base (371ed49) to head (32ef255).
⚠️ Report is 4 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...tes/matrix-sdk-base/src/store/integration_tests.rs	77.46%	0 Missing and 16 partials ⚠️
crates/matrix-sdk-sqlite/src/state_store.rs	71.42%	3 Missing and 5 partials ⚠️
crates/matrix-sdk-base/src/store/mod.rs	92.30%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5589      +/-   ##
==========================================
- Coverage   88.65%   88.65%   -0.01%     
==========================================
  Files         345      345              
  Lines       96132    96225      +93     
  Branches    96132    96225      +93     
==========================================
+ Hits        85230    85312      +82     
+ Misses       6676     6674       -2     
- Partials     4226     4239      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[codspeed-hq]

CodSpeed Performance Report

Merging #5589 will not alter performance

_{Comparing bnjbvr/thread-sub-bumpstamps (32ef255) with main (371ed49)}

Summary

✅ 49 untouched benchmarks

[poljar]

Not really requesting many changes but I'm not sure if I misunderstood something of if this is incorrect in some edge cases.

[poljar]

Any reason why we're using as_str() for database storage instead of our usual Serialize serde stuff?

Additionally if we use any of the more usual traits for this, then we can use insta to add more protection to this. Display, Debug, or Serialize would work.

[bnjbvr]

It makes for a denser format to use as_str() (it flattens the subscription into a string, instead of a JSON map), and it allows future use cases like "find the list of all subscribed threads in this room" (which we couldn't do if we serialized the ThreadSubscription as a blob inside the DB, b/o encrypted DBs).

[poljar]

Well I'm not saying that you shouldn't flatten out the serialized format, just that a different trait is usually used for this. A custom Serialize implementation can produce the same serialized format as you already have.

[bnjbvr]

No, it'd be wrong to use a custom Serialize impl in the sqlite impl, which has status and bump_stamps columns.

[poljar]

Hmm, but this will avoid a transition from unsubscribed to subscribed if the bump stamps are equal, no?

Is such a transition impossible or undesirable?

Furthermore, as you said, this logic is duplicated a couple of times. Can't we make the bump stamp a separate type BumpStamp(u64) and the implement either a is_better() method or even add a PartialEq implementation that will contain this logic?

[bnjbvr]

I think it's not possible, because the bump stamps will increase if the thread subscription goes from subscribed to unsubscribed. My understanding is that same bump stamp = same subscription information; it forms an "index" when combined with the (room id, thread root event id) pair. Will ask to clarify this in the MSC, and maybe I can add a small comment here.

But yeah, the duplication isn't great, will find a way to avoid it.