Sliding Sync: Handle timeline limit changes (take 2)

[erikjohnston]

This supersedes #17503, given the per-connection state is being heavily rewritten it felt easier to recreate the PR on top of that work.

This correctly handles the case of timeline limits going up and down.

This does not handle changes in required_state, but that can be done as a separate PR.

Based on #17575.

[MadLittleMods]

Overall, I just don't get it. Related to the comment above

I'm confused why we're even updating new_connection_state here given we set it above though

[erikjohnston]

Sorry, looks like I moved the code and then just didn't remove it from the old place.

I think the example explains the motivating case we're trying to handle? Another way of putting it is that if the response isn't limited, then we know we have sent down at least the previous timeline_limit worth of events, and so we don't need to record the reduced timeline limit until we actually see a limited response

[MadLittleMods]

After talking with @erikjohnston and some more thinking, I think I understand the optimization here.

The goal of timeline trickling/unstable_expanded_timeline is that the client can get at-least a chunk of current messages to fill up the screen. So if we were already able to give the client a nice contiguous chunk of current timeline, when they increase the timeline_limit again, we can tell that they already have enough messages and don't need to trigger unstable_expanded_timeline again.

If the response is limited, the client has a gap in the timeline. So if they trigger unstable_expanded_timeline on a subsequent request, we can give them a full chunk of history again. This won't necessarily fill the gap but they will get enough events to fill up the screen and they can paginate from there.

I'm not sure this explanation would have made it more clear from the beginning.

[MadLittleMods]

This continues to feel horrible especially given new edge cases like this comment. Highly recommend we just update the client to use an initial sync request with timeline_limit: 20 and required_state: [] (which allows us to avoid the extra bytes) to accomplish the exact same thing without introducing any of this bizarre behavior.

Previous conversation for context

[MadLittleMods]

Talked with @erikjohnston a bit more and was trying to figure out why the initial sync request doesn't solve this completely. I basically asked the opposite question for how/why timeline trickling/unstable_expanded_timeline makes this easier. The following question unlocked a better understanding for one complication when trying to use the initial sync route.

How is the timeline stitching done for the timeline trickling? How is the problem easier when using that?

[...] its all coming down one connection, so you know you've got a consistent "current" timeline chunk that you'll get updates for (which you can then optionally stitch together with whatever timeline chunks the client currently has)

This is a valid point!

I'm still leaning towards the side of initial sync being possible to use (and better) and just requires some basic timeline stitching logic. ElementX might already have some stitching and event de-duplication logic to handle what the proxy was doing before that would also cover this case.

Since ElementX doesn't have offline support and throws away events, I think we could just do this:

For the timeline stitching logic, the client can store the latest event in timeline before our initial sync, then find that event in the initial sync timeline events and spread backwards from that point. That way, the ongoing sync loop can still append to the end of the timeline and continue seamlessly.

So if we have a timeline [103] already on the client, we store latest_event_id = 103, do our initial sync which returns [100, 101, 102, 103, 104] and we splice/spread in only [100, 101, 102, 103] accordingly (drop any new events after the latest_event_id from the initial sync response). This makes it so that even if the ongoing sync loop sends 104 before or after our initial sync does, it still appends like normal and everything is in seamless order.

If there are so many new messages sent in the time between us storing the latest_event_id and the initial sync responding that we now have a gap, we can just throw away our initial sync events because we have enough events to fill up the timeline just from our normal ongoing sync loop.

---

To be clear, the client doesn't need to be fancy about stitching:

If the client had more timeline like [98, 99, 100, 101, 102, 103], we store latest_event_id = 103, we start the initial sync, our ongoing sync loop races us and returns 104 which makes our timeline look like [98, 99, 100, 101, 102, 103, 104]. Then our initial sync responds with [100, 101, 102, 103, 104], we find the 103 spot in the response to slice at and place it at the 103 spot in the client timeline leaving us with [100, 101, 102, 103, 104]

Pseudo code (maybe off-by-one errors):

latest_event_id = 103

# do initial sync request

initial_sync_timeline = [100, 101, 102, 103, 104]
event_index_in_response = initial_sync_timeline.index(latest_event_id)
# Skip if we can't find the `latest_event_id` in the response.
# This means there have been so many messages sent between the time we initially
# made the initial sync and the response that this is no longer relevant.
# We already have enough events to fill up the timeline from the normal
# ongoing sync loop
if event_index_in_response is None:
	return

event_index_in_client_timeline = client_timeline.index(latest_event_id)
# Update the timeline
client_timeline = initial_sync_timeline[0:event_index_in_response] + client_timeline[event_index_in_client_timeline:-1]

[MadLittleMods]

We will need this same room sync config tracking for required_state (and probably filter/extension) changes so overall the concept isn't lost.

[MadLittleMods]

I think it would be better if clients provided the unstable_expanded_timeline option to opt-in to the odd behavior.

But providing unstable_expanded_timeline on the response is better than nothing; clients can at-least detect when they've triggered this behavior. And we also at-least have a chance of migrating away from this behavior.

[MadLittleMods]

Possible edge case: ElementX triggers unstable_expanded_timeline by increasing the timeline_limit and gets a chunk of timeline and we record that higher timeline_limit. I go to bed and then I wake up (or just some period of time that new messages were sent in but I didn't have the app open). How does ElementX get timeline for all of the events in the gap? If it tries to trigger unstable_expanded_timeline again, it won't work because the last recorded timeline_limit is already just as high.

[erikjohnston]

I think this is fine. There are two cases depending on if the timeline limit in the morning is small or high:

For the small case:

• When the client comes online it'll get a small bit of history with limited=true.
• The client can then either backpaginate in the room, or do a room sub with a larger timeline

For the high case:

• When the client comes online it'll get a large chunk of history with limited=true. This is (hopefully) enough to show a screens worth of data, which is what we want.
• If the client wants more it can just backpaginate.

What EX at least wants is to quickly be able to get enough chunks of history in rooms (in the background) to be able to show a screens worth of data. That way the UX is open the app, see a fast sync, (in the background it preloads the top N rooms with more timeline), the user clicks on one of the rooms and sees a page of timeline, and then the app can paginate in more timeline as usual (via /messages).

[MadLittleMods]

After talking with @erikjohnston and some more thinking, I think I understand the optimization here.

The goal of timeline trickling/unstable_expanded_timeline is that the client can get at-least a chunk of current messages to fill up the screen. So if we were already able to give the client a nice contiguous chunk of current timeline, when they increase the timeline_limit again, we can tell that they already have enough messages and don't need to trigger unstable_expanded_timeline again.

If the response is limited, the client has a gap in the timeline. So if they trigger unstable_expanded_timeline on a subsequent request, we can give them a full chunk of history again. This won't necessarily fill the gap but they will get enough events to fill up the screen and they can paginate from there.

I'm not sure this explanation would have made it more clear from the beginning.

[MadLittleMods]

Talked with @erikjohnston a bit more and was trying to figure out why the initial sync request doesn't solve this completely. I basically asked the opposite question for how/why timeline trickling/unstable_expanded_timeline makes this easier. The following question unlocked a better understanding for one complication when trying to use the initial sync route.

How is the timeline stitching done for the timeline trickling? How is the problem easier when using that?

[...] its all coming down one connection, so you know you've got a consistent "current" timeline chunk that you'll get updates for (which you can then optionally stitch together with whatever timeline chunks the client currently has)

This is a valid point!

I'm still leaning towards the side of initial sync being possible to use (and better) and just requires some basic timeline stitching logic. ElementX might already have some stitching and event de-duplication logic to handle what the proxy was doing before that would also cover this case.

Since ElementX doesn't have offline support and throws away events, I think we could just do this:

For the timeline stitching logic, the client can store the latest event in timeline before our initial sync, then find that event in the initial sync timeline events and spread backwards from that point. That way, the ongoing sync loop can still append to the end of the timeline and continue seamlessly.

So if we have a timeline [103] already on the client, we store latest_event_id = 103, do our initial sync which returns [100, 101, 102, 103, 104] and we splice/spread in only [100, 101, 102, 103] accordingly (drop any new events after the latest_event_id from the initial sync response). This makes it so that even if the ongoing sync loop sends 104 before or after our initial sync does, it still appends like normal and everything is in seamless order.

If there are so many new messages sent in the time between us storing the latest_event_id and the initial sync responding that we now have a gap, we can just throw away our initial sync events because we have enough events to fill up the timeline just from our normal ongoing sync loop.

---

To be clear, the client doesn't need to be fancy about stitching:

If the client had more timeline like [98, 99, 100, 101, 102, 103], we store latest_event_id = 103, we start the initial sync, our ongoing sync loop races us and returns 104 which makes our timeline look like [98, 99, 100, 101, 102, 103, 104]. Then our initial sync responds with [100, 101, 102, 103, 104], we find the 103 spot in the response to slice at and place it at the 103 spot in the client timeline leaving us with [100, 101, 102, 103, 104]

Pseudo code (maybe off-by-one errors):

latest_event_id = 103

# do initial sync request

initial_sync_timeline = [100, 101, 102, 103, 104]
event_index_in_response = initial_sync_timeline.index(latest_event_id)
# Skip if we can't find the `latest_event_id` in the response.
# This means there have been so many messages sent between the time we initially
# made the initial sync and the response that this is no longer relevant.
# We already have enough events to fill up the timeline from the normal
# ongoing sync loop
if event_index_in_response is None:
	return

event_index_in_client_timeline = client_timeline.index(latest_event_id)
# Update the timeline
client_timeline = initial_sync_timeline[0:event_index_in_response] + client_timeline[event_index_in_client_timeline:-1]

[MadLittleMods]

Assuming the edge case from the last round of review isn't a show stopper, I guess this has gotten to the point that it is good enough to ship.

Overall, this feels like a step backwards (better than originally proposed though 🙇) for good clients taking everything into account. I fear that we won't touch this again at-least until after the ElementX launch at which point I'll be gone and might live on. Hoping we actually get to that better/brighter future that doesn't include this behavior 🤞