Fix appservices being backlogged and not receiving new events due to a bug in notify_interested_services

[xyzz]

The bug is:

1. get_new_events_for_appservice returns max(event_id) for all retrieved rows
2. get_new_events_for_appservice accepts upper_bound and all retrieved rows have event_id <= upper_bound
3. since notify_interested_services sets upper_bound to whatever get_new_events_for_appservice returned, what happens is the while True loop at line 75 is executed only once when your server has more than 100 events: first, it gets 100 events, the next iteration it will always get 0
4. so if your server has a lot of unprocessed events, it will only process 100 at a time (e.g. when a message is sent) and it looks as if appservices are not getting events submitted to them

I saw this happen on my homeserver.

Changing it to current_max ensures that it always gets some events, until the backlog is clear.

The second change is removing the len(events) < limit check - I'm not sure why, but sometimes it returned 99 events for me, even while there were a lot of events backlogged. Since there's another check: if not events, this should not make things infloop.

Signed-off-by: Ilya Zhuravlev whatever@xyz.is

[matrixbot]

Can one of the admins verify this patch?

[matrixbot]

Can one of the admins verify this patch?

[xyzz]

the test infloops because it's made to always return a single event

edit: I fixed it in the commit below

[ara4n]

@xyzz - thanks for this; it looks like a pretty plausible bug & fix. @erikjohnston can you review it (when back from hol) please?

[erikjohnston]

@matrixbot ok to test

[erikjohnston]

Woo, thanks.

The second change is removing the len(events) < limit check - I'm not sure why, but sometimes it returned 99 events for me, even while there were a lot of events backlogged. Since there's another check: if not events, this should not make things infloop.

I imagine this happens when notify_interested_services is called again, i.e. there are new events, while the original notify_interested_services is processing the original set of events. There are two possible solutions:

1. Do what you did and remove the len(events) < limjt check, or
2. Check if current_max has changed during the loop, and always recheck if it has

@xyzz any thoughts?

[erikjohnston]

I think it would help avoid confusion if upper_bound = self.current_max above this line was removed

[xyzz]

I like current solution better because there's less code to maintain. Is there any benefit to checking len of returned events?

I removed the assignment as you suggested.

[erikjohnston]

Cool, thanks! :)