From 7b16014f018bb3c8ee56c94f9f1a027b4631f1d3 Mon Sep 17 00:00:00 2001 From: Marcello Costagliola Date: Mon, 5 Oct 2026 20:41:55 +0200 Subject: [PATCH 1/2] Delete a room's messages in a job, one transaction each Room#destroy destroyed every message inside the room's own transaction, which holds SQLite's write lock until the last one: on a room with many messages, every other write in the app waited and failed. The request now takes the room away from its members and leaves the rest to Room::DestroyJob, which destroys the messages one at a time, each in its own short transaction, and then the room. An open room is closed in the request, so that someone who joins the account before the job ends isn't given it. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa --- app/controllers/rooms_controller.rb | 2 +- app/jobs/room/destroy_job.rb | 7 +++ app/models/room.rb | 21 +++++++++ .../rooms/directs_controller_test.rb | 6 ++- test/controllers/rooms_controller_test.rb | 16 ++++++- test/models/room_test.rb | 46 +++++++++++++++++++ 6 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 app/jobs/room/destroy_job.rb diff --git a/app/controllers/rooms_controller.rb b/app/controllers/rooms_controller.rb index 276265a..237d35a 100644 --- a/app/controllers/rooms_controller.rb +++ b/app/controllers/rooms_controller.rb @@ -12,7 +12,7 @@ class RoomsController < ApplicationController end def destroy - @room.destroy + @room.destroy_later broadcast_remove_room redirect_to root_url diff --git a/app/jobs/room/destroy_job.rb b/app/jobs/room/destroy_job.rb new file mode 100644 index 0000000..dc647e3 --- /dev/null +++ b/app/jobs/room/destroy_job.rb @@ -0,0 +1,7 @@ +class Room::DestroyJob < ApplicationJob + discard_on ActiveJob::DeserializationError + + def perform(room) + room.destroy_one_message_at_a_time + end +end diff --git a/app/models/room.rb b/app/models/room.rb index 20865fc..9a23847 100644 --- a/app/models/room.rb +++ b/app/models/room.rb @@ -45,6 +45,27 @@ class Room < ApplicationRecord end end + # Takes the room away from its members at once, and leaves its messages to Room::DestroyJob. Destroying them in + # the room's own transaction held the database's write lock, and stopped every other write, until the last one. + # An open room is closed first, so that no one who joins the account before the job runs is let in. + def destroy_later + room = open? ? becomes!(Rooms::Closed) : self + + transaction do + room.save! + room.memberships.delete_all + end + + Room::DestroyJob.perform_later(room) + end + + # Each message is destroyed in its own transaction, so other writes get through in between. Any message posted + # meanwhile goes with the room. + def destroy_one_message_at_a_time + messages.find_each(&:destroy) + destroy + end + def receive(message) unread_memberships(message) push_later(message) diff --git a/test/controllers/rooms/directs_controller_test.rb b/test/controllers/rooms/directs_controller_test.rb index a1d7b06..383788e 100644 --- a/test/controllers/rooms/directs_controller_test.rb +++ b/test/controllers/rooms/directs_controller_test.rb @@ -25,8 +25,10 @@ class Rooms::DirectsControllerTest < ActionDispatch::IntegrationTest sign_in :kevin assert_difference -> { Room.count }, -1 do - delete rooms_direct_url(rooms(:david_and_kevin)) - assert_redirected_to root_url + perform_enqueued_jobs do + delete rooms_direct_url(rooms(:david_and_kevin)) + assert_redirected_to root_url + end end end diff --git a/test/controllers/rooms_controller_test.rb b/test/controllers/rooms_controller_test.rb index dfba3bb..824d884 100644 --- a/test/controllers/rooms_controller_test.rb +++ b/test/controllers/rooms_controller_test.rb @@ -83,11 +83,23 @@ class RoomsControllerTest < ActionDispatch::IntegrationTest test "destroy" do assert_turbo_stream_broadcasts :rooms, count: 1 do assert_difference -> { Room.count }, -1 do - delete room_url(rooms(:designers)) + perform_enqueued_jobs { delete room_url(rooms(:designers)) } end end end + test "destroy takes the room away from its members at once and leaves its messages to a job" do + room = rooms(:designers) + + assert_enqueued_with(job: Room::DestroyJob, args: [ room ]) do + assert_no_difference -> { Message.count } do + delete room_url(room) + assert_redirected_to root_url + end + end + assert_empty room.memberships.reload + end + test "destroy only allowed for creators or those who can administer" do sign_in :jz @@ -99,7 +111,7 @@ class RoomsControllerTest < ActionDispatch::IntegrationTest rooms(:designers).update! creator: users(:jz) assert_difference -> { Room.count }, -1 do - delete room_url(rooms(:designers)) + perform_enqueued_jobs { delete room_url(rooms(:designers)) } end end diff --git a/test/models/room_test.rb b/test/models/room_test.rb index b113726..1c425ac 100644 --- a/test/models/room_test.rb +++ b/test/models/room_test.rb @@ -30,8 +30,54 @@ class RoomTest < ActiveSupport::TestCase assert Rooms::Closed.new.closed? end + test "an open room destroyed later lets no one in who joins the account before the job runs" do + room = rooms(:pets) + + room.destroy_later + newcomer = User.create!(name: "Newcomer", email_address: "newcomer@example.com", password: "secret123456") + + assert_not newcomer.memberships.exists?(room_id: room.id) + perform_enqueued_jobs only: Room::DestroyJob + assert_not Room.exists?(room.id) + end + + test "destroying one message at a time leaves nothing of the room behind" do + room = rooms(:designers) + searchable = room.messages.create!(body: "Kept in the search index", creator: users(:david)) + searchable.attachment.attach io: StringIO.new("hello"), filename: "hello.txt", content_type: "text/plain" + message_ids = room.messages.ids + assert Boost.where(message_id: message_ids).exists? + assert_equal 1, search_index_rows(searchable) + + room.destroy_one_message_at_a_time + + assert_not Room.exists?(room.id) + assert_empty Message.where(id: message_ids) + assert_empty Boost.where(message_id: message_ids) + assert_empty ActionText::RichText.where(record_type: "Message", record_id: message_ids) + assert_empty ActiveStorage::Attachment.where(record_type: "Message", record_id: message_ids) + assert_enqueued_jobs 1, only: ActiveStorage::PurgeJob + assert_equal 0, search_index_rows(searchable) + end + + test "each message is destroyed in its own transaction, so other writes get through in between" do + room = rooms(:designers) + messages = room.messages.count + transactions = 0 + count_transactions = ->(*, payload) { transactions += 1 if payload[:sql].start_with?("RELEASE SAVEPOINT") } + + ActiveSupport::Notifications.subscribed(count_transactions, "sql.active_record") { room.destroy_one_message_at_a_time } + + assert_equal messages + 1, transactions + end + test "default involvement for new users" do room = Rooms::Closed.create_for({ name: "Hello!", creator: users(:david) }, users: [ users(:kevin), users(:david) ]) assert room.memberships.all? { |m| m.involved_in_mentions? } end + + private + def search_index_rows(message) + Message.connection.select_value("select count(*) from message_search_index where rowid = #{message.id}") + end end From 462eff10df8dfdf6b9d132c701059261a44950a9 Mon Sep 17 00:00:00 2001 From: Marcello Costagliola Date: Tue, 6 Oct 2026 14:45:18 +0200 Subject: [PATCH 2/2] Reset former members' connections and grant open rooms in one statement Deleting the memberships with delete_all skips Membership's after_destroy_commit, which resets a member's connections when one membership is revoked. Until the job ran, a member who had the room open kept its streams, and a message that still landed in the room (a request already past the membership check, a bot's reply) reached them. The request now reads the members' ids in the transaction that deletes their memberships, and the job resets their connections before it destroys the messages. It costs a Redis round trip per member, about 1.5 s for 10,000, so it's done in the job rather than in the request. User#grant_membership_to_open_rooms read the open rooms and inserted in a separate statement, so a user created while a room was being closed could read it as open and be granted it after the close. It's now a single insert ... select, which SQLite runs under the write lock, skipping duplicates as insert_all did. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Bj8KnxpTf9sj2Ysa8aLAVa --- app/jobs/room/destroy_job.rb | 3 ++- app/models/room.rb | 13 ++++++++++--- app/models/user.rb | 12 +++++++++++- test/controllers/rooms_controller_test.rb | 3 ++- test/models/room_test.rb | 15 +++++++++++++++ test/models/user_test.rb | 12 ++++++++++++ 6 files changed, 52 insertions(+), 6 deletions(-) diff --git a/app/jobs/room/destroy_job.rb b/app/jobs/room/destroy_job.rb index dc647e3..af5bef8 100644 --- a/app/jobs/room/destroy_job.rb +++ b/app/jobs/room/destroy_job.rb @@ -1,7 +1,8 @@ class Room::DestroyJob < ApplicationJob discard_on ActiveJob::DeserializationError - def perform(room) + def perform(room, former_member_ids) + room.reset_remote_connections_of(former_member_ids) room.destroy_one_message_at_a_time end end diff --git a/app/models/room.rb b/app/models/room.rb index 9a23847..ed7280d 100644 --- a/app/models/room.rb +++ b/app/models/room.rb @@ -51,12 +51,19 @@ class Room < ApplicationRecord def destroy_later room = open? ? becomes!(Rooms::Closed) : self - transaction do + former_member_ids = transaction do room.save! - room.memberships.delete_all + room.memberships.pluck(:user_id).tap { room.memberships.delete_all } end - Room::DestroyJob.perform_later(room) + Room::DestroyJob.perform_later(room, former_member_ids) + end + + # Deleting the memberships skips the reset that revoking one does, so the former members get it here, all at once, + # before the messages go: their connections stop receiving the room's streams. It's done in the job, not the + # request, because it costs a Redis round trip per member. + def reset_remote_connections_of(user_ids) + User.where(id: user_ids).find_each(&:reset_remote_connections) end # Each message is destroyed in its own transaction, so other writes get through in between. Any message posted diff --git a/app/models/user.rb b/app/models/user.rb index 20d05dd..df2fee1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -50,8 +50,18 @@ class User < ApplicationRecord end private + # One statement, which SQLite runs under the write lock, so no room is closed between reading the open ones and + # inserting. A room that Room#destroy_later closes is either closed before, and skipped, or after, and takes this + # membership away with the others. A room that granted itself to every user when it was created is skipped too, + # as insert_all did. def grant_membership_to_open_rooms - Membership.insert_all(Rooms::Open.pluck(:id).collect { |room_id| { room_id: room_id, user_id: id } }) + now = Time.current + + Membership.connection.execute Membership.sanitize_sql([ <<~SQL, id, now, now ]) + insert into memberships (room_id, user_id, created_at, updated_at) + select id, ?, ?, ? from rooms where type = 'Rooms::Open' + on conflict do nothing + SQL end def deactived_email_address diff --git a/test/controllers/rooms_controller_test.rb b/test/controllers/rooms_controller_test.rb index 824d884..e8ed51f 100644 --- a/test/controllers/rooms_controller_test.rb +++ b/test/controllers/rooms_controller_test.rb @@ -90,8 +90,9 @@ class RoomsControllerTest < ActionDispatch::IntegrationTest test "destroy takes the room away from its members at once and leaves its messages to a job" do room = rooms(:designers) + member_ids = room.memberships.pluck(:user_id) - assert_enqueued_with(job: Room::DestroyJob, args: [ room ]) do + assert_enqueued_with(job: Room::DestroyJob, args: [ room, member_ids ]) do assert_no_difference -> { Message.count } do delete room_url(room) assert_redirected_to root_url diff --git a/test/models/room_test.rb b/test/models/room_test.rb index 1c425ac..09771f3 100644 --- a/test/models/room_test.rb +++ b/test/models/room_test.rb @@ -1,6 +1,8 @@ require "test_helper" class RoomTest < ActiveSupport::TestCase + include ActionCable::TestHelper + test "grant membership to user" do rooms(:watercooler).memberships.grant_to(users(:kevin)) assert rooms(:watercooler).users.include?(users(:kevin)) @@ -41,6 +43,19 @@ class RoomTest < ActiveSupport::TestCase assert_not Room.exists?(room.id) end + test "a room destroyed later resets the connections of its former members, as revoking their memberships would" do + room = rooms(:designers) + former_members = room.users.to_a + assert former_members.many? + + room.destroy_later + perform_enqueued_jobs only: Room::DestroyJob + + former_members.each do |member| + assert_broadcast_on "action_cable/#{member.to_gid_param}", { type: "disconnect", reconnect: true } + end + end + test "destroying one message at a time leaves nothing of the room behind" do room = rooms(:designers) searchable = room.messages.create!(body: "Kept in the search index", creator: users(:david)) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index efeda3c..61bb1c3 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -12,6 +12,18 @@ class UserTest < ActiveSupport::TestCase end end + test "memberships granted to the open rooms are like any other" do + freeze_time + user = create_new_user + + assert_equal Rooms::Open.ids.sort, user.memberships.pluck(:room_id).sort + user.memberships.each do |membership| + assert membership.involved_in_mentions? + assert_equal Time.current, membership.created_at + assert_equal Time.current, membership.updated_at + end + end + test "deactivating a user deletes push subscriptions, searches, memberships for non-direct rooms, and changes their email address" do assert_difference -> { Membership.count }, -users(:david).memberships.without_direct_rooms.count do assert_difference -> { Push::Subscription.count }, -users(:david).push_subscriptions.count do