Have you verified that all the places with older versions were cleaned?

Why was this change made?

Why did we increase the number here?

I don't remember suggesting to increase this number, but if that happened then it was my mistake. If we're dropping support for older clients, we should remove the code that handles versions < 32. In this case only the early return should stay.

This change does not change functionality since anything greater or equal to 32 is simultaneously greater than 30

This isn't correct. If one branch is used for code < 30 and other for >= 31 it might be the case that a client with code = 31 would receive something invalid for him. If we maintained backward compatibility we could do that, but we're not maintaining it. By backward compatibility I understand the approach where a client with version N is able to consume any data which clients < N could consume.

I am not sure whether it is possible that delivery attribute of notifications table row can have both androidID and deviceType === 'android' set? Or are those mutually exclusive?

I don't recall unfortunately. If you want to get an answer you can do some "code archaeology" and look at the Git history to see what the code did in the past.

this is done by child differential for this one

I can't find what diff you're referring to here. In general you should always link things to make it easier for your reviewers...

By dropping support for code version below 31 we decided to drop support for android clients requiring notification with custom_notification property

1. Is it the case that delivery.androidID will only appear if the notif that is being rescinded was sent to a codeVersion below 31? If so, can you please link the code (potentially old code that has been deleted, so you may have to check Git history) that would set delivery.androidID?
2. I don't see any codeVersion checks in fcmPush. I'm confused why you're claiming that a null codeVersion will result in a custom_notification property... can you link the code that shows that?

It would also be good to check where this function in lib is used, and whether it's only used in keyserver. I assume yes, since keyserver is the only place it makes sense to do codeVersion checks... but I want to make sure before we delete the condition

I don't think we should remove this code. First of all, we still have many unrescinded notifs with androidID and iosID set on my production keyserver:

MariaDB [comm]> SELECT COUNT(*) FROM notifications WHERE JSON_EXTRACT(delivery, '$.androidID') IS NOT NULL AND rescinded = 0;
+----------+
| COUNT(*) |
+----------+
|      380 |
+----------+
1 row in set (0.020 sec)

MariaDB [comm]> SELECT COUNT(*) FROM notifications WHERE JSON_EXTRACT(delivery, '$.iosID') IS NOT NULL AND rescinded = 0;
+----------+
| COUNT(*) |
+----------+
|      544 |
+----------+
1 row in set (0.015 sec)

Second of all, it's important to understand the significance of codeVersion here. The codeVersion we have in the delivery column is the codeVersion of the device at the time the notif was sent. That's not necessarily the codeVersion at the time the rescind is sent.

In fact, the code should probably be different... it would be better if we try to match up the cookie of the device to which the original notif was sent. If we can't find one, then there's probably no point rescinding the notif since the device is probably not online anymore. If we can find a cookie row then we could check the codeVersion there, and then we would know how to process the rescind.

Regardless this potential improvement is outside the scope of this work. My larger point is that I don't see a reason to delete this code. If for whatever reason we end up rescinding one of these notifs, if we assume the device is still online then I think it's fair to also assume its codeVersion is higher than 30.

Looking at my production keyserver, it appears that the oldest codeVersion currently active is 128:

MariaDB [comm]> SELECT COUNT(*), JSON_EXTRACT(versions, '$.codeVersion') AS code_version FROM cookies WHERE platform != 'web' GROUP BY code_version;
+----------+--------------+
| COUNT(*) | code_version |
+----------+--------------+
|        1 | 128          |
|       89 | 131          |
|        1 | 136          |
|       19 | 137          |
|        1 | 138          |
|        3 | 139          |
|       10 | 140          |
|       16 | 141          |
+----------+--------------+
8 rows in set (0.004 sec)

Why 32 instead of 31? Did we decide on 32 specifically?