Fix case where get_partial_current_state_deltas could return >100 rows

[anoadragon453]

get_partial_current_state_deltas is supposed to return max 100 rows. We relied on this assumption in #18926, forgoing batching of state deltas. It turns out that get_partial_current_state_deltas can potentially return millions of rows, and we saw this in production on element.io.

This PR fixes the case where every state delta in current_state_delta_stream has a count of 1, meaning total will be 100. Before this change, that would result in clipped_stream_id = max_stream_id, meaning we'd potentially pull out millions of rows.

I haven't written a regression test for this and I'm not sure how to easily seed current_state_delta_stream with the correct data. I suppose we could create a room and change the topic >100 times? Alternatively we could write directly to the table, but such a test would need to be updated if the schema changes. I was able to write a unit test without writing SQL directly.

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.
• Code style is correct (run the linters)

[reivilibre]

nothing materially wrong with this, just some thoughts, what do you think?

[reivilibre]

wondering if instead of doing this with a for loop, we should just ask the DB for the result we want directly?

I think this would do it, as long as you still handle the case where there are no result rows (less than 100 left, so no need to clamp)

SELECT stream_id
FROM current_state_delta_stream
WHERE stream_id > ? AND stream_id <= ?
ORDER BY stream_id ASC
LIMIT 1 OFFSET 99

[anoadragon453]

I actually just ended up using LIMIT in the second query in 0e40bf4.

Am I missing something - is it really this simple? All the unit tests appear to pass 🤷

[anoadragon453]

Complement CI is failing due to a flaky test.

Edit: And sytest appears to just be stalling...

[reivilibre]

I mean.... it's weird if the tests don't catch this, probably a smell that we're missing a test

[reivilibre]

afraid this doesn't work AFAICT, because stream_id is not unique, but we always want to process the entire state with the same stream_id (never process some of the rows but not others)

[anoadragon453]

So would this mean that if a group of state deltas with the same stream_id that was >100 items would cause this function to deadlock?

I suppose we should just return the whole group, even if it's larger than limit, in that case.

Edit: especially since the docstring states that if the list of returned state deltas is empty, then we are up to date. Potentially skipping over a lot of state deltas!