Presence last_update_id API

[mateuszmandera]

Implements a prototype for the API discussed in https://chat.zulip.org/#narrow/stream/378-api-design/topic/presence.20rewrite/near/1586819

Commit 1 Adds the necessary last_update_id column and PresenceSequence model, commit message elaborates on the migration plan
Commit 2 The backend+frontend parts of the system using the above data structure.

This is WIP, so no tests and only tested in the webapp.
What this has:

• Adds the last_update_id param and plumbing in the API through the backend and frontend
• Implements what is intended to be an efficient locking mechanism inside do_update_user_presence, updating PresenceSequence and the related UserPresence update in a single SQL query, at the very end of the transaction, ensuring the hot lock only gets held as briefly as possible.
• Verified the core mechanism seems to work; the webapp only fetches the recently updated presence data and updates its buddy list with the incremental info

What this does NOT have:

• Doesn't handle the discussed case of getting presence info about a user the webapp doesn't know yet (https://chat.zulip.org/#narrow/stream/378-api-design/topic/presence.20rewrite/near/1586828) EDIT: Fixed
• Doesn't remove the user_presence query worker, which we probably want to do, moving presence updates in-process
• PresenceSequence for the realm doesn't get created in the import codepath. EDIT: Fixed

[timabbott]

How do we expect this to be used in GET /events? I guess a potential use case would be for mobile/terminal clients to just cache what they had the last presence state for the user, so it could be good, but maybe worth somet discussion.

[mateuszmandera]

Hmm yeah the idea was in case the mobile app wanted to use this; and also in case we were changing presence events format based on whether the client is last_update_id-based. But we're not changing events right now, so no point perhaps. Removed this for now.

[timabbott]

I think a feature to have mobile be able to just used cached data on this detail is probably a good idea, so worth bringing it up in API design.

[mateuszmandera]

https://chat.zulip.org/#narrow/stream/378-api-design/topic/presence.20rewrite.20.3E.20add.20last_update_to.20.2Fregister.3F

[mateuszmandera]

Responded to comments and pushed a commit with a proposal for handling the main race:

the discussed case of getting presence info about a user the webapp doesn't know yet (https://chat.zulip.org/#narrow/stream/378-api-design/topic/presence.20rewrite/near/1586828)

Will start working on test coverage next.

[timabbott]

I opened #30283 to integrate the migrations part of this, with the following changes.

diff --git a/zerver/migrations/0526_user_presence_backfill_last_update_id_to_0.py b/zerver/migrations/0526_user_presence_backfill_last_update_id_to_0.py
index 7083776df6..babf436730 100644
--- a/zerver/migrations/0526_user_presence_backfill_last_update_id_to_0.py
+++ b/zerver/migrations/0526_user_presence_backfill_last_update_id_to_0.py
@@ -17,12 +17,14 @@ def backfill_user_presence_last_update_id(
     BATCH_SIZE = 10000
     lower_bound = 0
 
-    # Make sure we run past the end in case of new rows created while we run.
+    # Add a slop factor to make it likely we run past the end in case
+    # of new rows created while we run. The next step will fail to
+    # remove the null possibility if we race, so this is safe.
     max_id += BATCH_SIZE / 2
 
     while lower_bound < max_id:
         UserPresence.objects.filter(
-            id__gt=lower_bound, id__lte=lower_bound + BATCH_SIZE, last_update=None
+            id__gt=lower_bound, id__lte=lower_bound + BATCH_SIZE, last_update_id=None
         ).update(last_update_id=0)
         lower_bound += BATCH_SIZE
 
diff --git a/zerver/migrations/0527_presencesequence.py b/zerver/migrations/0527_presencesequence.py
index 222fd3b9c0..76a172c06c 100644
--- a/zerver/migrations/0527_presencesequence.py
+++ b/zerver/migrations/0527_presencesequence.py
@@ -20,7 +20,10 @@ def create_presence_sequence_for_old_realms(
     BATCH_SIZE = 2000
     lower_bound = 0
 
-    # Make sure we run past the end in case of new rows created while we run.
+    # Add a slop factor to make it likely we run past the end in case
+    # of new rows created while we run. Races with realm creation are
+    # pretty unlikely, and should throw an exception, so we should
+    # catch them.
     max_id += BATCH_SIZE / 2
 
     while lower_bound < max_id:

[alexmv]

The first commit says This builds on top of the previous commit which is no longer the case, since the commit in question was merged in #30283, and there have been other things merged since then.

[mateuszmandera]

Squashed and split thing into two deployable commits - first just backend API support, and then the frontend actually using it.

The first commit says This builds on top of the previous commit which is no longer the case, since the commit in question was merged in #30283, and there have been other things merged since then.

Thanks, fixed that message to refer that commit specifically by its hash.

[mateuszmandera]

This was a small bug in this doc btw, the param is called last_update_id for this endpoint. (the prefix would be redundant here, since obviously it's in the presence domain so to speak; there isn't other non-presence data included here that would require such disambiguation via prefixes)

[mateuszmandera]

Pushed a commit tweaking the details for last_update_id+slim_presence interactions a bit, to fit https://chat.zulip.org/#narrow/stream/378-api-design/topic/presence.20rewrite/near/1817372 It should be squashed into the backed implementation commit, just leaving it separate to easily show what's changed.

[timabbott]

Squashed the backend/API commits to merge as #30343.

[timabbott]

Probably we can actually merge the rest of this too, I'll come back to that tomorrow.

[mateuszmandera]

There was a small bug where for a zephyr mirror realm, we'd return last_update_id=None. Pushed a fix to make it -1

[timabbott]

Merged, thanks @mateuszmandera!