~20 messages missing from backscroll while back-paginating

[bwindels]

Looks like the previousToken was removed too soon, and messages from back-paginating weren't even persisted. All the extracted info at https://matrix.to/#/!hmSsLnYZrcUIbuplaG:matrix.org/$1585389963162341Oiitx:matrix.org?via=matrix.org

[bwindels]

This is exactly after having refreshed after #15 (comment) happened, same room and same gap.

[bwindels]

(There was nothing relevant in the console)

[bwindels]

Backfill was indeed obtained with the previousToken we see in #15 (comment)

[bwindels]

It seemed to have stored only the first event in the backfill, and removed the previousToken, closing the gap.

[bwindels]

Ok, I think I know what happened. The first message that was not persisted from the backfill was a message that was already stored in our database at a later point in the timeline (within the same fragment) that came in over sync. So looks like synapse repeated an event, brawl detected it already knew about the event and closed the gap. Apparently we're not checking which fragment that event is in, and we're not changing the fragment id of the previous fragment (if there is already a previous fragment) (probably for the best in this case or previous fragment would have been the same as the fragment's own id === 19). We should file a synapse bug and make our code more robust.

[bwindels]

Filed matrix-org/synapse#7164

[bwindels]

the previous fragment id did get set to 19 actually, idb data must have been stale in the devtools:

[bwindels]

This is probably what happened in #15 actually

Since the previousId === id now, when going back to this room, I can't scroll past fragment 19 and there is no gap fill button. Exactly the same symptoms.

[bwindels]

And also the initial error we saw at #15 (comment)

[bwindels]

so, things to do when filling a gap:

• if the fragment is already linked up in the direction we're filling, look up the first event id we should encounter when filling overlaps with the adjacent fragment.
• when looking for overlapping events, check if the event is the event id expected when overlapping. If not, it means we've hit Synapse returned event in backfill that was already returned during sync matrix-org/synapse#7164 and we ignore the event as it is already present in the timeline elsewhere, and find the next overlapping id in the remaining events. Repeat this until either finding the expected overlapping event id or we've gone through all the events in the backfill.
• don't add fragments to the FragmentIdComparer until the transaction has been comitted. This is another case of modify-memory-state-before-commit.

When the fragment is not linked up already in the fill direction (this would only be the case when we start supporting different islands, e.g. a permalink island and the sync island), we can at least detect if the found event id is at the expected edge (e.g. backfilling should collide with an event at the end of a fragment). And the fragment id should obviously not be the same.

[bwindels]

https://matrix.org/docs/spec/client_server/latest#syncing actually has a warning about duplicate events, which is exactly what we reported at matrix-org/synapse#7164, so specced behaviour, not a server bug.