github-mirror/discourse
2
0
Fork 0
mirror of https://github.com/discourse/discourse.git synced 2025-01-04 11:40:14 +08:00
Code Issues Actions 1 Packages Projects Releases Wiki Activity

FIX: better handle race condition when a channel is deleted (#30403)

Browse Source 
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](fa0ad0306c/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](fa0ad0306c/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.
This commit is contained in:
Régis Hanol 2024-12-21 00:49:21 +01:00 committed by GitHub
parent fa0ad0306c
commit ebb6f1c2d2
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
1 changed files with 17 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

36
plugins/chat/plugin.rb
View File

@ -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|