Fix broken notification incremental sync

[texuf]

I was not seeing unread notifications in sync, even if they were written to the db

Notifications are in their own stream, but the code was trying to tack them onto the join room stream. If the offsets “happened” to line up, you might get a count here or there, but they would be totally wrong (jump from 1 to 0 to 2, etc)

To fix, put them in their own top level object, handle them on the client.

Signed-off-by: Austin Ellis <[email protected]>

Pull Request Checklist

• [x] I have added added tests for PR or I have justified why this PR doesn't need tests.
• [x] Pull request includes a sign off

[texuf]

Tests are broken, but I see the same errors on other PRs

[texuf]

Thank you for the quick response. If you don't mind, can you answer two questions for me?

1. Where does UnreadNotificationCount get set? I see s.db.UpsertRoomUnreadNotificationCounts(ctx, userID, data.RoomID, data.UnreadNotificationCount, data.UnreadHighlightCount) in userapi.go, but I don't understand how that data is created. Searching matrix org for UnreadNotificationCount or unread_notification_count isn't bringing up any relevant results for me. Is this something built into jetstream? Can you point me to the right place?

2. Why would it matter if a notification was sent for a room you're no longer in? Wouldn't the client know you're not in that room and just ignore the notification? Is there something else that goes wrong?

Thank you for your time! Austin

[0x1a8510f2]

I don't know the answer to the first but I'll try to answer the second question:

Why would it matter if a notification was sent for a room you're no longer in? Wouldn't the client know you're not in that room and just ignore the notification? Is there something else that goes wrong?

The client may or may not ignore the notification, but it's not a good idea to assume this about a client and rely on that behavior because there are no guarantees and it could confuse users. In addition, it causes unnecessary network traffic which can mean additional mobile data usage, more battery usage and slower notifications. It's not the end of the world but it's not a good design.

[texuf]

I don't know the answer to the first but I'll answer the second question:

Why would it matter if a notification was sent for a room you're no longer in? Wouldn't the client know you're not in that room and just ignore the notification? Is there something else that goes wrong?

The client may or may not ignore the notification, but it's not a good idea to assume this about a client and rely on that behavior because there are no guarantees and it could confuse users. In addition, it causes unnecessary network traffic which can mean additional mobile data usage, more battery usage and slower notifications. It's not the end of the world but it's not a good design.

Usually you want to deliver notifications either at least once or exactly once. As a user I'm pretty used to seeing a text message get delivered again if I'm in a spotty network (though I haven't seen it in a while). Currently dendrite is delivering notifications sometimes. Just something to think about.

[0x1a8510f2]

Usually you want to deliver notifications either at least once or exactly once. As a user I'm pretty used to seeing a text message get delivered again if I'm in a spotty network (though I haven't seen it in a while).

I believe the issue with this change is not that the notification gets re-delivered but rather that it gets delivered potentially after a room has been left by the user, right? In which case delivering the notification is bad for the aforementioned reasons and because the user is unable to view the message anyway (as they have left the room) so it is confusing.

Currently dendrite is delivering notifications sometimes. Just something to think about.

I agree that this should be fixed but I think the point being made is that this isn't a suitable solution.

[texuf]

You are right, that was a bad example, with this change the notifications won't get redelivered. Let me rephrase.

I'm a user. I just left a space (which I hardly ever do) and I receive a notification saying I have a new message in that space. Seeing that I just left the space, I'm probably a little confused, but I remember that matrix is a federated distributed system run by a non profit and it's not perfect, and I go about my day.

OR, I'm a user, and I never get unread notifications. I miss new messages in conversations that I'm trying to participate in all the time. I'm probably going to stop using this product.

I'm sorry if I'm coming across as harsh, I'm not meaning to be. I'm saying this with smiles that you would be able to see if we were in person. I just want to be as direct as possible because it feels like I'm missing something.

[texuf]

Okay! I went through some more tests on my end and you were right, this wasn't the right fix, the join message is the wrong place for this data. I updated my code. It leaves everything else in place and adds a new object to the response that matches the sync pattern you have going on here. It requires client changes but they're pretty simple in theory (in practice I added my own memory store that allows me to get access to the raw sync data and pulled the numbers from there.)

I'd love feedback, but also fine if you just close this request.

[texuf]

In case anyone is still listening to this thread, if I'm reading the code correctly, it looks like we cleanup the notification table after a day

func (s *notificationsStatements) Clean(ctx context.Context, txn *sql.Tx) error {
	_, err := sqlutil.TxStmt(txn, s.cleanNotificationsStmt).ExecContext(
		ctx,
		time.Now().AddDate(0, 0, -1).UnixNano()/int64(time.Millisecond), // keep non-highlights for a day
		time.Now().AddDate(0, -1, 0).UnixNano()/int64(time.Millisecond), // keep highlights for a month
	)
	return err
}

But we don't clean up the notifications in the syncAPI table.

When we send a read receipt we first do a updated _, err := s.db.SetNotificationsRead(ctx, localpart, roomID, int64(read.Read), true) and only forward the message on if the table was updated. If a user waits more than a day to send a read receipt, they can't clear their notifications.

[S7evinK]

Closing this, since #2688 is now merged and solves the same problem.