Repository: zulip/zulip
Pull request: delete_in_topic: Split up the deletion into batches. 
Fixes https://github.com/zulip/zulip/issues/22821.

As explained in the comment in the code:

Topics can be large enough that this request will inevitably time out.
In such a case, it's good for some progress to be accomplished, so that
full deletion can be achieved by repeating the request. For that purpose,
we delete messages in atomic batches, committing after each batch.

The additional perk is that the ordering of messages should prevent some
hypothetical deadlocks - ref https://github.com/zulip/zulip/issues/19054

---

Tested manually with print statements to verify messages are getting deleted in expected batches. `TopicDeleteTest` is a simple automated test that already exists for verifying this endpoint works.

Problem statement:
Unable to delete a topic
Hi! I'm attemptimg to delete a large (over 50k messages) topic that contains alerts and logs quotations sent by elastalert. When deleting it from the web gui, the delete request gets stuck for about a minute and then i get 502 with no any entry in zulip logs. Messages from other topics and entire topics still can be deleted, only this one remains untouched.

Is there any way to delete such large topics? Can i delete messages from the database directly?

Hints from issue discussion:
Hello @zulip/server-message-view members, this issue was labeled with the "area: message-editing" label, so you may want to check it out!

<!-- areaLabelAddition -->

Yikes, 50k messages is a topic is a lot of log spam. You can likely delete it using `do_delete_messages` via a management command shell (https://zulip.readthedocs.io/en/latest/production/management-commands.html), which would avoid the time limit for requests that we have configured. 

The other option is to modify `/etc/zulip/uwsgi.conf` to dramatically increase the timeout (the setting is called `harikiri` in that file; and then you'll need use `scripts/restart-server` to restart the server) and see if that's good enough; might be easier.
@timabbott Thanks for you response! I'm a bit confused, manage.py doesn't show such command:

```
zulip@f866a8398bf6:~/deployments/current$ ./manage.py help do_delete_messages
Unknown command: 'do_delete_messages'. Did you mean restore_messages?
```

And yes, i'm running Zulip in docker, if it's relevant.
@timabbott increased hahakiri to about 3 hours as you suggested but it still fails after the time is up, looks like it takes much more... Maybe you could point how can that `do_delete_messages` be used? Or if it is possible to delete messages from the database directly?
To make it easier to address follow-up questions, I [started a thread in the development community](https://chat.zulip.org/#narrow/stream/31-production-help/topic/Unable.20to.20delete.20a.20topic.20.2322821/near/1432762).
I said "management command shell" - I meant `manage.py shell`, which is documented on that page. `do_delete_messages` is a function you need to call there; it's a Python shell. If you need help scripting that, I recommend getting a support contract.

In terms of a bug fix for Zulip, we'll likely just want to paginate the queries to do blocks of 1k messages in the topic as individual transactions. @mateuszmandera perhaps you can look at whether this is easy to do. I think it'd be a significant improvement even if it still times out, if it just manages to make clear progress before doing so, since that at least would offer a UI level workaround of doing it repeatedly. Eventually we'll need to move to the `deferred_work` queue processor.

Commit message: delete_in_topic: Split up the deletion into batches.

Fixes #22821.

As explained in the comment in the code:

Topics can be large enough that this request will inevitably time out.
In such a case, it's good for some progress to be accomplished, so that
full deletion can be achieved by repeating the request. For that purpose,
we delete messages in atomic batches, committing after each batch.

The additional perk is that the ordering of messages should prevent some
hypothetical deadlocks - ref #19054

Patch to review:
diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md
index 6e26a135e626d..ed50d848a848f 100644
--- a/templates/zerver/api/changelog.md
+++ b/templates/zerver/api/changelog.md
@@ -20,6 +20,13 @@ format used by the Zulip server that they are interacting with.
 
 ## Changes in Zulip 6.0
 
+**Feature level 147**
+
+* [`POST /streams/{stream_id}/delete_topic`](/api/delete-topic):
+  Messages now are deleted in batches, starting from the newest, so
+  that progress will be made even if the request times out because of
+  an extremely large topic.
+
 **Feature level 146**
 
 * [`POST /realm/profile_fields`](/api/create-custom-profile-field),
diff --git a/version.py b/version.py
index 97524b849f587..c6e4ee55e7fb3 100644
--- a/version.py
+++ b/version.py
@@ -33,7 +33,7 @@
 # Changes should be accompanied by documentation explaining what the
 # new level means in templates/zerver/api/changelog.md, as well as
 # "**Changes**" entries in the endpoint's documentation in `zulip.yaml`.
-API_FEATURE_LEVEL = 146
+API_FEATURE_LEVEL = 147
 
 # Bump the minor PROVISION_VERSION to indicate that folks should provision
 # only when going from an old version of the code to a newer version. Bump
diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml
index a2a651e1c659a..873733f56e110 100644
--- a/zerver/openapi/zulip.yaml
+++ b/zerver/openapi/zulip.yaml
@@ -14232,6 +14232,12 @@ paths:
         Topics are a field on messages (not an independent
         data structure), so deleting all the messages in the topic
         deletes the topic from Zulip.
+
+        **Changes**: Before Zulip 6.0 (feature level 147), this
+        request did a single atomic operation, which could time out
+        for very large topics. It now deletes messages in batches,
+        starting with the newest messages, so that progress will be
+        made even if the request times out.
       parameters:
         - $ref: "#/components/parameters/StreamIdInPath"
         - name: topic_name
diff --git a/zerver/views/streams.py b/zerver/views/streams.py
index cb928e334358b..818c36001fd68 100644
--- a/zerver/views/streams.py
+++ b/zerver/views/streams.py
@@ -55,6 +55,7 @@
 from zerver.lib.mention import MentionBackend, silent_mention_syntax_for_user
 from zerver.lib.request import REQ, has_request_variables
 from zerver.lib.response import json_success
+from zerver.lib.retention import STREAM_MESSAGE_BATCH_SIZE as RETENTION_STREAM_MESSAGE_BATCH_SIZE
 from zerver.lib.retention import parse_message_retention_days
 from zerver.lib.streams import (
     StreamDict,
@@ -848,7 +849,6 @@ def get_topics_backend(
     return json_success(request, data=dict(topics=result))
 
 
-@transaction.atomic
 @require_realm_admin
 @has_request_variables
 def delete_in_topic(
@@ -857,7 +857,7 @@ def delete_in_topic(
     stream_id: int = REQ(converter=to_non_negative_int, path_only=True),
     topic_name: str = REQ("topic_name"),
 ) -> HttpResponse:
-    (stream, sub) = access_stream_by_id(user_profile, stream_id)
+    stream, ignored_sub = access_stream_by_id(user_profile, stream_id)
 
     messages = messages_for_topic(assert_is_not_none(stream.recipient_id), topic_name)
     if not stream.is_history_public_to_subscribers():
@@ -867,9 +867,20 @@ def delete_in_topic(
         ).values_list("message_id", flat=True)
         messages = messages.filter(id__in=deletable_message_ids)
 
-    messages = messages.select_for_update(of=("self",))
-
-    do_delete_messages(user_profile.realm, messages)
+    # Topics can be large enough that this request will inevitably time out.
+    # In such a case, it's good for some progress to be accomplished, so that
+    # full deletion can be achieved by repeating the request. For that purpose,
+    # we delete messages in atomic batches, committing after each batch.
+    # TODO: Ideally this should be moved to the deferred_work queue.
+    batch_size = RETENTION_STREAM_MESSAGE_BATCH_SIZE
+    while True:
+        with transaction.atomic(durable=True):
+            messages_to_delete = messages.order_by("-id")[0:batch_size].select_for_update(
+                of=("self",)
+            )
+            if not messages_to_delete:
+                break
+            do_delete_messages(user_profile.realm, messages_to_delete)
 
     return json_success(request)
 


Code review comments:
diff_hunk: @@ -1139,6 +1139,20 @@ export function delete_topic(stream_id, topic_name) {
         data: {
             topic_name,
         },
+        success() {},
+        error(xhr) {
+            if (xhr.status === 502) {
+                /* When trying to delete a very large topic, it's
+                   possible for the request to the server to
+                   time out after making some progress. Retry the
+                   request, so that the user can just do nothing and
+                   watch the topic slowly be deleted.
+
+                   TODO: Show a nice loading indicator experience.
+                */
+                delete_topic(stream_id, topic_name);
+            }
+        },
path: static/js/message_edit.js
text: @user1:
An unlimited retry load on 502s is the kind of code we don't want to have exist because you can DoS yourself. I think given the very infrequent nature of calls to this code path, it's probably fine; but in an ideal world we'd limit this to 10 retries or something.

Is it easy to add that?

@author:
@user1  Yeah, it crossed my mind but the retries just run into rate limits, but not sure if that's sufficient for us? I added some code to limit the retries in the frontend now though, it's awkward but we do something similar in `static/js/server_events.js` with `get_events_failures`.

Test manually that it works in dev env:
1. Failure case, by haaving the server return 502 infinitely without doing anything. The frontend stops at 10 tries indeed.
2. Success, where the server returns 502 after processing a tiny batch. The app keeps retrying until the topic is deleted (as long as that's under 10 tries).

@user1:
I think you don't need a global variable, which is problematic because multiple of these requests can be in flight at the same time. Just pass `delete_topic_failures + 1` to `delete_topic` in the recursive call, and have the initial call assume 0 if the parameter isn't provided.