Generate historic pagination token for /messages when no ?from token provided

[MadLittleMods]

Generate historic pagination (tXXX-XXX_...) token for /messages when no ?from token provided. Previously, it generated a live pagination token (sXXX_...) but always sorted by topological_ordering anyway when querying for messages.

For more documentation around historic vs live pagination tokens, see #12317

Part of #12281

Context: #12319 (comment)

To be clear, this PR in itself doesn't fix any bug. But when combined with the changes from #12319, it does fix a bug with /messages was previously masked. See the "Context" section below.

The first thing to decide is whether we like the fix from #12319. If we like the fix there, then we also need to consider the fix here to fix the bug that is uncovered.

But this PR does update /messages to do the right thing imo so it would be a good change regardless. We should be using historic tokens as the default for /messages.

---

Notice that start is now a historic token.

/messages?dir=b

Before	After
{ "chunk": [...], "start": "s962_0_0_0_0_0_0_0_0", "end": "t181-536_0_0_0_0_0_0_0_0" }	{ "chunk": [...], "start": "t190-962_0_0_0_0_0_0_0_0", "end": "t181-536_0_0_0_0_0_0_0_0" }

/messages?dir=f

Before	After
{ "chunk": [], "start": "s962_0_0_0_0_0_0_0_0", "end": "s962_0_0_0_0_0_0_0_0" }	{ "chunk": [], "start": "t190-962_0_0_0_0_0_0_0_0", "end": "t190-962_0_0_0_0_0_0_0_0" }

Context

Context: #12319 (comment)

In #12319, get_state_at(since_token) -> get_recent_events_for_room -> _paginate_room_events_txn is being updated to sort according to the type of token that it's given. So if a historic token is provided, sort by topological_ordering first, or if it's a live token, sort by stream_ordering first. #12319 aims to fix problems with our incremental /sync not including the state it should (a known FIXME comment).

But that PR also unearthed a bug that's been hiding in /messages. The problem was previously masked because even though we generated a live token (stream_ordering) before, it still always sorted by topological_ordering. We expect to get messages by topological_ordering when calling /messages by default. Although it should work if it's provided with a live token in the ?from or ?to query parameters.

• /messages [...] return them in the order determined by the event graph (topological_ordering, stream_ordering).

-- https://github.com/matrix-org/synapse/blob/f608e6c8cf48d8d5327c5ac04746f4f7f3992e31/docs/development/room-dag-concepts.md

When you use the changes from #12319 with /messages without a ?from token, it can give messages in the wrong order in some situations because it now sorts by stream_ordering because the token generated when you don't provide one is a live token (stream_ordering). Say the room has less than 100 messages which all fit within the /messages?dir=b&limit=100 response; all of the federation backfilled messages with negative stream_ordering will be at the end of the response even though they should be intertwined according to their topological_ordering(depth). The same problem occurs with MSC2716 batches because they also have negative stream_ordering.

So this PR, aims to fix the /messages sorting problem by generating a historic pagination token to have as the starting bounds (known as from_token in the code).

Dev notes

Add "tabulate>=0.8.9", to synapse/python_dependencies.py#L91

Maybe can format tabular data without a dependency: https://stackoverflow.com/a/9536084/796832

Update: New iteration without a dependency available in the description of matrix-org/matrix-react-sdk#8354

logger.info(
    "asdf_get_debug_events_in_room_ordered_by_depth %s",
    await self.store.asdf_get_debug_events_in_room_ordered_by_depth(
        room_id
    ),
)
logger.info(
    "asdf_get_debug_events_in_room_ordered_by_stream_ordering %s",
    await self.store.asdf_get_debug_events_in_room_ordered_by_stream_ordering(
        room_id
    ),
)


from tabulate import tabulate
            
async def asdf_get_debug_events_in_room_ordered_by_depth(self, room_id: str) -> Any:
    """Gets the topological token in a room after or at the given stream
    ordering.
    Args:
        room_id
    """
    sql = (
        "SELECT depth, stream_ordering, type, state_key, event_id FROM events"
        " WHERE events.room_id = ?"
        " ORDER BY depth DESC, stream_ordering DESC;"
    )
    rows = await self.db_pool.execute(
        "asdf_get_debug_events_in_room_ordered_by_depth", None, sql, room_id
    )

    headers = ["depth", "stream_ordering", "type", "state_key", "event_id"]
    return tabulate(rows, headers=headers)

async def asdf_get_debug_events_in_room_ordered_by_stream_ordering(self, room_id: str) -> Any:
    """Gets the topological token in a room after or at the given stream
    ordering.
    Args:
        room_id
    """
    sql = (
        "SELECT depth, stream_ordering, type, state_key, event_id FROM events"
        " WHERE events.room_id = ?"
        " ORDER BY stream_ordering DESC, depth DESC;"
    )
    rows = await self.db_pool.execute(
        "asdf_get_debug_events_in_room_ordered_by_depth", None, sql, room_id
    )

    headers = ["depth", "stream_ordering", "type", "state_key", "event_id"]
    return tabulate(rows, headers=headers)

Pull Request Checklist

• Pull request is based on the develop branch
• Pull request includes a changelog file. The entry should:
  • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
  • Use markdown where necessary, mostly for code blocks.
  • End with either a period (.) or an exclamation mark (!).
  • Start with a capital letter.
  • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
• Pull request includes a sign off
• Code style is correct
  (run the linters)

[MadLittleMods]

We can probably use the same changelog as #12319 as this PR doesn't actually fix anything without additionally merging that PR.

In any case, this PR does update /messages to do the right thing imo so it would be a good change regardless. But with more of changelog like:

Update `/messages` to use historic pagination tokens by default.

[MadLittleMods]

Previously, get_current_key_for_room was unused along with get_room_events_max_id.

It seemed almost exactly what I was looking for so I repurposed both functions with proper RoomStreamToken types instead of returning the string version that was interpolated together.

[erikjohnston]

I think this PR makes sense independently of #12319.

[MadLittleMods]

I think this PR makes sense independently of #12319.

Agreed 👍 But I think we need to update the changelog #12370 (comment)

Will send another PR for that ⏩

[MadLittleMods]

Thanks for the review and merge @erikjohnston 🐱

Created #12401 to update the changelog ⏩

[babolivier]

If someone (like me) is wondering why we're allowing /messages requests without a from even though the spec says it's required, it's because we're following MSC3567 which just hasn't made it into a release of the spec yet.