This repository has been archived by the owner on Apr 26, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fix serialization errors when rotating notifications #13118
Fix serialization errors when rotating notifications #13118
Changes from 13 commits
52b0ad0
a7b010b
22bd551
3fb73c3
e8c71de
0081a41
f8df221
456e9fd
7dea161
6235916
12efcd7
681c070
840c253
3627197
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I still don't get why this is an ok thing to do. Haven't we deleted rows from
event_push_actions
, so relying on it (rather than includingevent_push_summary
) will give numbers that are too low?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We delete rows at the same time as we update the
event_push_summary
. After a user sends a receipt in the room there are two cases:_handle_new_receipts_for_notifs_txn
background task, and so we have deleted the push actions but updated theevent_push_summary
.event_push_summary
is stale but we haven't yet deleted the push actions so we can correctly calculate the count fromevent_push_actions
.Maybe:
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, but suppose:
event_push_summary
to say there are 3 remaining unread messages, and delete all the entries fromevent_push_actions
.event_push_summary
and there is nothing left inevent_push_actions
. So we return 0 unread messages instead of 1?Or am I fundamentally misunderstanding how this works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We delete all entries from
event_push_actions
from before E2, so there will still be the three push actions for the three unread messages in there. So in the final step we'd still have push actions E3, E4 and E5 in theevent_push_actions
tableThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it'd be quite nice to do an assertion that we have the right isolation level, but that looks a bit fiddly and probably a thing for another time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a change in behavior then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is just making sure we're inject a read receipt at an actual event (which we have to), rather than at some arbitrary place.
(Side note: we need to rewrite this test to not poke things directly into the DB, and instead create real rooms, events and receipts)