Fix push for m.read events

[SpiritCroc]

badge_count_last_call was never set to non-zero values when the response for push
notifications included a "rejected" key which mapped to an empty list.

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)

Signed-off-by: Tobias Büttner dev@spiritcroc.de

[f35f0ef9d0e827dae86552d3899f78fc]

I manually patched this commit on top of v1.58.1 and built a Docker image. Initial testing appears that it has fixed the problem. Thank you!

[BloodyIron]

What rough ETA can we anticipate for this to roll out to prod public matrix.org?

[erikjohnston]

Hi, thanks for this. I agree that your change is sensible, but I'm failing to understand how this is causing badge_count_last_call to never be set to zero? My reading of the code is that dispatch_push will never set badge_count_last_call, and so the only place we're setting and using it is at:

synapse/synapse/push/httppusher.py

Lines 140 to 142 in 1fe202a

	if self.badge_count_last_call is None or self.badge_count_last_call != badge:
	self.badge_count_last_call = badge
	await self._send_badge(badge)

which I think should correctly send out badges when they hit zero?

[SpiritCroc]

Hi, thanks for this. I agree that your change is sensible, but I'm failing to understand how this is causing badge_count_last_call to never be set to zero? My reading of the code is that dispatch_push will never set badge_count_last_call, and so the only place we're setting and using it is at:

synapse/synapse/push/httppusher.py

Lines 140 to 142 in 1fe202a

	if self.badge_count_last_call is None or self.badge_count_last_call != badge:
	self.badge_count_last_call = badge
	await self._send_badge(badge)

which I think should correctly send out badges when they hit zero?

The issue before was that the badge_count_last_call was always none or zero, and never set to anything that is NOT zero. So once it was zero, there weren't any further pushes for read events, since the remembered value hasn't changed. The code setting it to zero indeed works, hence I wrote "never set to non-zero values", which may be a confusing double negation :)

[erikjohnston]

Oh yeah, I misread that setting to zero didn't work. Still, _update_badge gets called every time a receipt is sent, and we recalculate badge each time, so changes would cause self.badge_count_last_call != badge to be true at that point, no?

[SpiritCroc]

Oh yeah, I misread that setting to zero didn't work. Still, _update_badge gets called every time a receipt is sent, and we recalculate badge each time, so changes would cause self.badge_count_last_call != badge to be true at that point, no?

Yes, the count updated on every read receipt. Usually, users just read to the bottom, so it was set to zero. But we also need to update the count on new messages, which was broken. E.g. following scenario:

• User read all, badge_count_last_call successfully set to zero
• New message -> push notification (but badge update is broken as before this PR, so badge_count_last_call is still zero
• User read all -> no update is sent, because badge_count_last_call == badge == 0

ok, now I think about it, my point that it is never set to something not-zero is wrong, if users don't read to the bottom - but it's still broken in the described scenario :)

[erikjohnston]

Oh yeah, I misread that setting to zero didn't work. Still, _update_badge gets called every time a receipt is sent, and we recalculate badge each time, so changes would cause self.badge_count_last_call != badge to be true at that point, no?

Yes, the count updated on every read receipt. Usually, users just read to the bottom, so it was set to zero. But we also need to update the receipt on new messages, which was broken. E.g. following scenario:

• User read all, badge_count_last_call successfully set to zero
• New message -> push notification (but badge update is broken as before this PR, so badge_count_last_call is still zero
• User read all -> no update is sent, because badge_count_last_call == badge == 0

OH, right yes. So basically: we have sent a badge count value to the push gateway, but since we didn't record that we incorrectly thing we don't need to send out a new badge count value when we see a receipt.

[erikjohnston]

Thanks for talking me through it!