From ebb6f1c2d2cf4f13e424225a368fec12df3f6ce3 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Sat, 21 Dec 2024 00:49:21 +0100
Subject: [PATCH] FIX: better handle race condition when a channel is deleted
 (#30403)

NOTE: I wasn't able to reproduce locally, so that's my best guess as to what happens based on the production error logs.
It's also the reason why I haven't changed/added any tests...

Earlier today, we started seeing a growing number of errors in the `register_presence_channel_prefix("chat-reply")` handler of the chat plugin.
It was all coming from a Discourse where they make a heavy use of chat channels. They create and **delete** category channels regularly.

If a user has a thread in one of the channels that just got deleted, the client application might not be aware (just yet), asks the server to be connected to the "presence" bus of that channel, and BOOOM.

The following [line](https://github.com/discourse/discourse/blob/fa0ad0306c06250f8af5f842d86b86f609fe02ea/plugins/chat/plugin.rb#L325) explodes because `chat_channel` is `nil`

```ruby
config.allowed_group_ids = chat_channel.allowed_group_ids
```

And why is `chat_channel` `nil`? Because when we [do](https://github.com/discourse/discourse/blob/fa0ad0306c06250f8af5f842d86b86f609fe02ea/plugins/chat/plugin.rb#L319)

```ruby
chat_channel = Chat::Thread.find_by!(id: thread_id, channel_id: channel_id).channel
```

The thread is still in the database, but the associated channel has been deleted.

A proper fix would most likely be to delete all the `Chat::Thread` associated to a deleted `Chat::Channel` but this might have more technical & business implications.
---
 plugins/chat/plugin.rb | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/plugins/chat/plugin.rb b/plugins/chat/plugin.rb
index fda894c9325..915d7f3e11e 100644
--- a/plugins/chat/plugin.rb
+++ b/plugins/chat/plugin.rb
@@ -37,6 +37,7 @@ module ::Chat
     chat_channel_retention_days: :dismissed_channel_retention_reminder,
     chat_dm_retention_days: :dismissed_dm_retention_reminder,
   }
+  PRESENCE_REGEXP = %r{^/chat-reply/(\d+)(?:/thread/(\d+))?$}
 end
 
 require_relative "lib/chat/engine"
@@ -303,32 +304,29 @@ after_initialize do
   end
 
   register_presence_channel_prefix("chat") do |channel_name|
-    next nil unless channel_name == "/chat/online"
-    config = PresenceChannel::Config.new
-    config.allowed_group_ids = Chat.allowed_group_ids
-    config
+    next if channel_name != "/chat/online"
+    PresenceChannel::Config.new.tap { |config| config.allowed_group_ids = Chat.allowed_group_ids }
   end
 
   register_presence_channel_prefix("chat-reply") do |channel_name|
-    if (
-         channel_id, thread_id =
-           channel_name.match(%r{^/chat-reply/(\d+)(?:/thread/(\d+))?$})&.captures
-       )
-      chat_channel = nil
-      if thread_id
-        chat_channel = Chat::Thread.find_by!(id: thread_id, channel_id: channel_id).channel
+    channel_id, thread_id = Chat::PRESENCE_REGEXP.match(channel_name)&.captures
+
+    next if channel_id.blank?
+
+    chat_channel =
+      if thread_id.present?
+        Chat::Thread.find_by(id: thread_id, channel_id:)&.channel
       else
-        chat_channel = Chat::Channel.find(channel_id)
+        Chat::Channel.find_by(id: channel_id)
       end
 
-      PresenceChannel::Config.new.tap do |config|
-        config.allowed_group_ids = chat_channel.allowed_group_ids
-        config.allowed_user_ids = chat_channel.allowed_user_ids
-        config.public = !chat_channel.read_restricted?
-      end
+    next if chat_channel.nil?
+
+    PresenceChannel::Config.new.tap do |config|
+      config.allowed_group_ids = chat_channel.allowed_group_ids
+      config.allowed_user_ids = chat_channel.allowed_user_ids
+      config.public = !chat_channel.read_restricted?
     end
-  rescue ActiveRecord::RecordNotFound
-    nil
   end
 
   register_push_notification_filter do |user, payload|