From 7b16014f018bb3c8ee56c94f9f1a027b4631f1d3 Mon Sep 17 00:00:00 2001 From: Marcello Costagliola Date: Mon, 5 Oct 2026 20:41:55 +0200 Subject: [PATCH] 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