From 1f5858098b9fea518229758ed9aab67185ffe72b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 19:36:01 +0000 Subject: [PATCH 1/4] Keep bot page totals on SQLite-maintained rooms.messages_count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Total-Count on GET /rooms/:id/:bot_key/messages was COUNT(*) of the room on every page. Serve it from rooms.messages_count updated by SQLite triggers so Rails, bulk SQL, and foreign writers stay in step — without ActiveRecord counter_cache callbacks those paths skip. Fixes #309. Co-authored-by: Thomas Klemm --- .../messages/by_bots_controller.rb | 2 +- app/models/room/messages_count.rb | 64 +++++++ config/initializers/room_messages_count.rb | 10 + ...61007190000_add_messages_count_to_rooms.rb | 17 ++ db/schema.rb | 5 +- lib/tasks/room_messages_count.rake | 38 ++++ .../messages/by_bots_controller_test.rb | 13 +- test/models/room/messages_count_test.rb | 174 ++++++++++++++++++ test/test_helper.rb | 19 ++ 9 files changed, 338 insertions(+), 4 deletions(-) create mode 100644 app/models/room/messages_count.rb create mode 100644 config/initializers/room_messages_count.rb create mode 100644 db/migrate/20261007190000_add_messages_count_to_rooms.rb create mode 100644 lib/tasks/room_messages_count.rake create mode 100644 test/models/room/messages_count_test.rb diff --git a/app/controllers/messages/by_bots_controller.rb b/app/controllers/messages/by_bots_controller.rb index 36a5069..d652afd 100644 --- a/app/controllers/messages/by_bots_controller.rb +++ b/app/controllers/messages/by_bots_controller.rb @@ -37,7 +37,7 @@ class Messages::ByBotsController < MessagesController end def set_pagination_headers - headers["X-Total-Count"] = @room.messages.count.to_s + headers["X-Total-Count"] = @room.messages_count.to_s if next_page = next_page_params headers["Link"] = %(<#{room_bot_messages_url(@room, params[:bot_key], **next_page)}>; rel="next") diff --git a/app/models/room/messages_count.rb b/app/models/room/messages_count.rb new file mode 100644 index 0000000..5790108 --- /dev/null +++ b/app/models/room/messages_count.rb @@ -0,0 +1,64 @@ +# Keeps rooms.messages_count correct for every SQLite writer — Rails, bulk SQL, +# and foreign connections — without ActiveRecord counter_cache callbacks that +# those paths skip (and that would double-count if combined with triggers). +class Room::MessagesCount + INSERT_TRIGGER = "messages_ai_rooms_messages_count" + DELETE_TRIGGER = "messages_ad_rooms_messages_count" + UPDATE_TRIGGER = "messages_au_rooms_messages_count" + + class << self + def install!(connection = ActiveRecord::Base.connection) + uninstall!(connection) + connection.execute <<~SQL + CREATE TRIGGER #{INSERT_TRIGGER} AFTER INSERT ON messages + BEGIN + UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id; + END + SQL + connection.execute <<~SQL + CREATE TRIGGER #{DELETE_TRIGGER} AFTER DELETE ON messages + BEGIN + UPDATE rooms SET messages_count = messages_count - 1 WHERE id = OLD.room_id; + END + SQL + connection.execute <<~SQL + CREATE TRIGGER #{UPDATE_TRIGGER} AFTER UPDATE OF room_id ON messages + WHEN OLD.room_id IS NOT NEW.room_id + BEGIN + UPDATE rooms SET messages_count = messages_count - 1 WHERE id = OLD.room_id; + UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id; + END + SQL + end + + def uninstall!(connection = ActiveRecord::Base.connection) + [ INSERT_TRIGGER, DELETE_TRIGGER, UPDATE_TRIGGER ].each do |name| + connection.execute("DROP TRIGGER IF EXISTS #{name}") + end + end + + def backfill!(connection = ActiveRecord::Base.connection) + connection.execute <<~SQL + UPDATE rooms SET messages_count = ( + SELECT COUNT(*) FROM messages WHERE messages.room_id = rooms.id + ) + SQL + end + + # schema.rb does not dump SQLite triggers; reinstall after schema:load. + def ensure!(connection = ActiveRecord::Base.connection) + return unless connection.data_source_exists?(:rooms) + return unless connection.column_exists?(:rooms, :messages_count) + return if trigger_installed?(connection, INSERT_TRIGGER) + + install!(connection) + end + + def trigger_installed?(connection, name) + connection.select_value( + "SELECT 1 FROM sqlite_master WHERE type = 'trigger' AND name = #{connection.quote(name)}" + ).present? + end + end +end + diff --git a/config/initializers/room_messages_count.rb b/config/initializers/room_messages_count.rb new file mode 100644 index 0000000..44af41b --- /dev/null +++ b/config/initializers/room_messages_count.rb @@ -0,0 +1,10 @@ +# schema.rb cannot dump SQLite triggers. Reinstall after boot when an existing DB +# is missing them (db:schema:load / test schema load also call ensure! via rake). +Rails.application.config.after_initialize do + ActiveRecord::Base.connection_pool.with_connection do |connection| + next unless connection.adapter_name.match?(/sqlite/i) + + Room::MessagesCount.ensure!(connection) + end +rescue ActiveRecord::NoDatabaseError, ActiveRecord::ConnectionNotEstablished +end diff --git a/db/migrate/20261007190000_add_messages_count_to_rooms.rb b/db/migrate/20261007190000_add_messages_count_to_rooms.rb new file mode 100644 index 0000000..afc58c9 --- /dev/null +++ b/db/migrate/20261007190000_add_messages_count_to_rooms.rb @@ -0,0 +1,17 @@ +class AddMessagesCountToRooms < ActiveRecord::Migration[8.2] + def up + unless column_exists?(:rooms, :messages_count) + add_column :rooms, :messages_count, :integer, null: false, default: 0 + end + + # Backfill before installing triggers so the COUNT rewrite does not race with + # concurrent inserts, and so we never rely on Rails callbacks for the tally. + Room::MessagesCount.backfill!(connection) + Room::MessagesCount.install!(connection) + end + + def down + Room::MessagesCount.uninstall!(connection) + remove_column :rooms, :messages_count if column_exists?(:rooms, :messages_count) + end +end diff --git a/db/schema.rb b/db/schema.rb index 460193a..d5b3a37 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.2].define(version: 2026_10_05_022000) do +ActiveRecord::Schema[8.2].define(version: 2026_10_07_190000) do create_table "accounts", force: :cascade do |t| t.datetime "created_at", null: false t.text "custom_styles" @@ -124,6 +124,7 @@ ActiveRecord::Schema[8.2].define(version: 2026_10_05_022000) do t.string "name" t.string "type", null: false t.datetime "updated_at", null: false + t.integer "messages_count", default: 0, null: false end create_table "searches", force: :cascade do |t| @@ -182,4 +183,6 @@ ActiveRecord::Schema[8.2].define(version: 2026_10_05_022000) do # Virtual tables defined in this database. # Note that virtual tables may not work with other database engines. Be careful if changing database. create_virtual_table "message_search_index", "fts5", ["body", "tokenize=porter"] + # SQLite triggers are not dumped by schema.rb; keep rooms.messages_count honest after schema:load. + Room::MessagesCount.install! end diff --git a/lib/tasks/room_messages_count.rake b/lib/tasks/room_messages_count.rake new file mode 100644 index 0000000..dcb2d08 --- /dev/null +++ b/lib/tasks/room_messages_count.rake @@ -0,0 +1,38 @@ +# schema.rb cannot dump SQLite triggers. Reinstall after loads, and re-append +# Room::MessagesCount.install! after dumps so the call is not lost. +namespace :room_messages_count do + task ensure: :environment do + ActiveRecord::Base.connection_pool.with_connection do |connection| + next unless connection.adapter_name.match?(/sqlite/i) + + Room::MessagesCount.ensure!(connection) + end + end + + task append_schema_install: :environment do + schema = Rails.root.join("db/schema.rb") + contents = schema.read + marker = "Room::MessagesCount.install!" + next if contents.include?(marker) + + contents.sub!(/\nend\n?\z/, <<~RUBY) + + # SQLite triggers are not dumped by schema.rb; keep rooms.messages_count honest after schema:load. + #{marker} + end + RUBY + schema.write(contents) + end +end + +{ + "db:schema:load" => "room_messages_count:ensure", + "db:test:load_schema" => "room_messages_count:ensure", + "db:schema:dump" => "room_messages_count:append_schema_install" +}.each do |task_name, enhancement| + next unless Rake::Task.task_defined?(task_name) + + Rake::Task[task_name].enhance do + Rake::Task[enhancement].invoke + end +end diff --git a/test/controllers/messages/by_bots_controller_test.rb b/test/controllers/messages/by_bots_controller_test.rb index da6f5cc..a7fc835 100644 --- a/test/controllers/messages/by_bots_controller_test.rb +++ b/test/controllers/messages/by_bots_controller_test.rb @@ -1,6 +1,9 @@ require "test_helper" +require "active_record/testing/query_assertions" class Messages::ByBotsControllerTest < ActionDispatch::IntegrationTest + include ActiveRecord::Assertions::QueryAssertions + setup do @room = rooms(:watercooler) end @@ -93,11 +96,14 @@ class Messages::ByBotsControllerTest < ActionDispatch::IntegrationTest @room.messages.create!(body: "Filler #{i}", creator: users(:jason), client_message_id: "filler-#{i}") end - get room_bot_messages_url(@room, users(:bender).bot_key) + assert_no_queries_match(/SELECT COUNT\(\*\) FROM "messages"/i) do + get room_bot_messages_url(@room, users(:bender).bot_key) + end assert_response :success json = JSON.parse(response.body) assert_equal Message::PAGE_SIZE, json.size + assert_equal @room.reload.messages_count.to_s, response.headers["X-Total-Count"] assert_equal "41", response.headers["X-Total-Count"] assert_not_includes json.map { it["id"] }, messages(:fourth).id @@ -119,7 +125,10 @@ class Messages::ByBotsControllerTest < ActionDispatch::IntegrationTest end test "index in a room with no messages" do - get room_bot_messages_url(rooms(:bender_and_kevin), users(:bender).bot_key) + room = rooms(:bender_and_kevin) + assert_equal 0, room.messages_count + + get room_bot_messages_url(room, users(:bender).bot_key) assert_response :success assert_equal [], JSON.parse(response.body) diff --git a/test/models/room/messages_count_test.rb b/test/models/room/messages_count_test.rb new file mode 100644 index 0000000..53deac1 --- /dev/null +++ b/test/models/room/messages_count_test.rb @@ -0,0 +1,174 @@ +require "test_helper" +require "sqlite3" + +class Room::MessagesCountTest < ActiveSupport::TestCase + setup do + @room = rooms(:designers) + @other_room = rooms(:pets) + synchronize!(@room, @other_room) + end + + test "ActiveRecord create and destroy adjust the counter once" do + assert_difference -> { @room.reload.messages_count }, +1 do + assert_difference -> { @room.messages.count }, +1 do + @room.messages.create!(creator: users(:jason), body: "Hello", client_message_id: "count-ar-create") + end + end + + assert_equal @room.messages.count, @room.reload.messages_count + + assert_difference -> { @room.reload.messages_count }, -1 do + assert_difference -> { @room.messages.count }, -1 do + @room.messages.order(:id).last.destroy + end + end + + assert_equal @room.messages.count, @room.reload.messages_count + end + + test "bulk insert_all and delete_all keep the counter in step" do + rows = Array.new(3) do |i| + { + room_id: @room.id, + creator_id: users(:david).id, + client_message_id: "count-bulk-#{i}", + created_at: Time.current, + updated_at: Time.current + } + end + + assert_difference -> { @room.reload.messages_count }, +3 do + Message.insert_all!(rows) + end + + assert_equal @room.messages.count, @room.reload.messages_count + + assert_difference -> { @room.reload.messages_count }, -3 do + @room.messages.where(client_message_id: rows.map { it[:client_message_id] }).delete_all + end + + assert_equal @room.messages.count, @room.reload.messages_count + end + + test "rolled back writes leave the counter unchanged" do + before = @room.reload.messages_count + + Message.transaction do + @room.messages.create!(creator: users(:jason), body: "Nope", client_message_id: "count-rollback") + raise ActiveRecord::Rollback + end + + assert_equal before, @room.reload.messages_count + assert_nil Message.find_by(client_message_id: "count-rollback") + end + + test "moving a message between rooms moves the counter" do + message = @room.messages.create!(creator: users(:jason), body: "Move me", client_message_id: "count-move") + synchronize!(@room, @other_room) + + assert_difference -> { @room.reload.messages_count }, -1 do + assert_difference -> { @other_room.reload.messages_count }, +1 do + message.update!(room: @other_room) + end + end + + assert_equal @room.messages.count, @room.reload.messages_count + assert_equal @other_room.messages.count, @other_room.reload.messages_count + end + + test "ensure! installs missing triggers without changing existing counts" do + before = @room.reload.messages_count + Room::MessagesCount.uninstall! + + assert_not Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) + + Room::MessagesCount.ensure! + + assert Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) + assert_equal before, @room.reload.messages_count + + assert_difference -> { @room.reload.messages_count }, +1 do + @room.messages.create!(creator: users(:jason), body: "After ensure", client_message_id: "count-ensure") + end + ensure + Room::MessagesCount.ensure! + end + + test "backfill plus triggers do not double-count ActiveRecord writes" do + Room::MessagesCount.uninstall! + ActiveRecord::Base.connection.execute <<~SQL + UPDATE rooms SET messages_count = ( + SELECT COUNT(*) FROM messages WHERE messages.room_id = rooms.id + ) + SQL + Room::MessagesCount.install! + + assert_equal @room.messages.count, @room.reload.messages_count + + assert_difference -> { @room.reload.messages_count }, +1 do + assert_difference -> { @room.messages.count }, +1 do + @room.messages.create!(creator: users(:jason), body: "Once", client_message_id: "count-no-double") + end + end + + assert_equal @room.messages.count, @room.reload.messages_count + ensure + Room::MessagesCount.ensure! + end + + private + def synchronize!(*rooms) + Room::MessagesCount.ensure! + Room::MessagesCount.backfill! + rooms.each(&:reload) + end +end + +# Foreign connections cannot join the transactional fixture lock; run outside it. +class Room::MessagesCountForeignConnectionTest < ActiveSupport::TestCase + self.use_transactional_tests = false + + setup do + Room::MessagesCount.ensure! + @room = rooms(:designers) + Room::MessagesCount.backfill! + @room.reload + end + + teardown do + Message.where(client_message_id: "count-foreign-insert").delete_all + Room::MessagesCount.backfill! + end + + test "foreign SQLite connections keep the counter in step" do + path = File.expand_path(ActiveRecord::Base.connection_db_config.database) + now = Time.current.utc.strftime("%Y-%m-%d %H:%M:%S.%6N") + before = @room.reload.messages_count + + # Release AR's checkout so the native connection can write. + ActiveRecord::Base.connection_pool.release_connection + + SQLite3::Database.new(path) do |db| + db.busy_timeout = 5_000 + db.execute( + "INSERT INTO messages (room_id, creator_id, client_message_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?)", + [ @room.id, users(:david).id, "count-foreign-insert", now, now ] + ) + end + + assert_equal before + 1, @room.reload.messages_count + assert_equal @room.messages.count, @room.messages_count + + foreign_id = Message.find_by!(client_message_id: "count-foreign-insert").id + + ActiveRecord::Base.connection_pool.release_connection + + SQLite3::Database.new(path) do |db| + db.busy_timeout = 5_000 + db.execute("DELETE FROM messages WHERE id = ?", [ foreign_id ]) + end + + assert_equal before, @room.reload.messages_count + assert_equal @room.messages.count, @room.messages_count + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 5a998e9..cf47a46 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -6,10 +6,25 @@ require "mocha/minitest" require "webmock/minitest" require "turbo/broadcastable/test_helper" +# maintain_test_schema! may reload schema.rb after after_initialize; triggers are +# not dumped, so ensure they exist before fixtures insert messages. +Room::MessagesCount.ensure! + WebMock.enable! +module RoomMessagesCountFixtures + # Fixture YAML loads alphabetically, so messages are inserted before rooms. + # INSERT triggers then update zero room rows; reconcile once after load. + def load_fixtures(config) + fixtures = super + Room::MessagesCount.backfill! + fixtures + end +end + class ActiveSupport::TestCase include ActiveJob::TestHelper + prepend RoomMessagesCountFixtures parallelize(workers: :number_of_processors) @@ -19,6 +34,9 @@ class ActiveSupport::TestCase include SessionTestHelper, MentionTestHelper, TurboTestHelper, DnsTestHelper setup do + # DROP TRIGGER commits outside transactional fixtures; put triggers back each test. + Room::MessagesCount.ensure! + ActionCable.server.pubsub.clear Rails.configuration.tap do |config| @@ -34,3 +52,4 @@ class ActiveSupport::TestCase WebMock.reset! end end + From 9fd1563c9b2961916d5b17cac739ba0bc428d475 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 19:46:57 +0000 Subject: [PATCH 2/4] Simplify messages_count trigger install and harden ensure! Require all three SQLite triggers before treating the counter as installed, drop the schema.rb / dump-rewrite install paths in favor of rake ensure after schema load, isolate destructive trigger tests, and cover foreign room moves plus fixture baseline counts. Co-authored-by: Thomas Klemm --- app/models/room/messages_count.rb | 7 +- config/initializers/room_messages_count.rb | 9 +- ...61007190000_add_messages_count_to_rooms.rb | 6 +- db/schema.rb | 3 +- lib/tasks/room_messages_count.rake | 28 +---- .../messages/by_bots_controller_test.rb | 1 - test/models/room/messages_count_test.rb | 100 ++++++++++-------- test/test_helper.rb | 13 +-- 8 files changed, 73 insertions(+), 94 deletions(-) diff --git a/app/models/room/messages_count.rb b/app/models/room/messages_count.rb index 5790108..53254ac 100644 --- a/app/models/room/messages_count.rb +++ b/app/models/room/messages_count.rb @@ -5,6 +5,7 @@ class Room::MessagesCount INSERT_TRIGGER = "messages_ai_rooms_messages_count" DELETE_TRIGGER = "messages_ad_rooms_messages_count" UPDATE_TRIGGER = "messages_au_rooms_messages_count" + TRIGGERS = [ INSERT_TRIGGER, DELETE_TRIGGER, UPDATE_TRIGGER ].freeze class << self def install!(connection = ActiveRecord::Base.connection) @@ -32,7 +33,7 @@ class Room::MessagesCount end def uninstall!(connection = ActiveRecord::Base.connection) - [ INSERT_TRIGGER, DELETE_TRIGGER, UPDATE_TRIGGER ].each do |name| + TRIGGERS.each do |name| connection.execute("DROP TRIGGER IF EXISTS #{name}") end end @@ -47,9 +48,10 @@ class Room::MessagesCount # schema.rb does not dump SQLite triggers; reinstall after schema:load. def ensure!(connection = ActiveRecord::Base.connection) + return unless connection.adapter_name.match?(/sqlite/i) return unless connection.data_source_exists?(:rooms) return unless connection.column_exists?(:rooms, :messages_count) - return if trigger_installed?(connection, INSERT_TRIGGER) + return if TRIGGERS.all? { |name| trigger_installed?(connection, name) } install!(connection) end @@ -61,4 +63,3 @@ class Room::MessagesCount end end end - diff --git a/config/initializers/room_messages_count.rb b/config/initializers/room_messages_count.rb index 44af41b..6c1c057 100644 --- a/config/initializers/room_messages_count.rb +++ b/config/initializers/room_messages_count.rb @@ -1,10 +1,5 @@ -# schema.rb cannot dump SQLite triggers. Reinstall after boot when an existing DB -# is missing them (db:schema:load / test schema load also call ensure! via rake). +# schema.rb cannot dump SQLite triggers. Cover existing DBs that lost them. Rails.application.config.after_initialize do - ActiveRecord::Base.connection_pool.with_connection do |connection| - next unless connection.adapter_name.match?(/sqlite/i) - - Room::MessagesCount.ensure!(connection) - end + Room::MessagesCount.ensure! rescue ActiveRecord::NoDatabaseError, ActiveRecord::ConnectionNotEstablished end diff --git a/db/migrate/20261007190000_add_messages_count_to_rooms.rb b/db/migrate/20261007190000_add_messages_count_to_rooms.rb index afc58c9..55286fa 100644 --- a/db/migrate/20261007190000_add_messages_count_to_rooms.rb +++ b/db/migrate/20261007190000_add_messages_count_to_rooms.rb @@ -1,8 +1,6 @@ class AddMessagesCountToRooms < ActiveRecord::Migration[8.2] def up - unless column_exists?(:rooms, :messages_count) - add_column :rooms, :messages_count, :integer, null: false, default: 0 - end + add_column :rooms, :messages_count, :integer, null: false, default: 0 # Backfill before installing triggers so the COUNT rewrite does not race with # concurrent inserts, and so we never rely on Rails callbacks for the tally. @@ -12,6 +10,6 @@ class AddMessagesCountToRooms < ActiveRecord::Migration[8.2] def down Room::MessagesCount.uninstall!(connection) - remove_column :rooms, :messages_count if column_exists?(:rooms, :messages_count) + remove_column :rooms, :messages_count end end diff --git a/db/schema.rb b/db/schema.rb index d5b3a37..ccb79c9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -183,6 +183,5 @@ ActiveRecord::Schema[8.2].define(version: 2026_10_07_190000) do # Virtual tables defined in this database. # Note that virtual tables may not work with other database engines. Be careful if changing database. create_virtual_table "message_search_index", "fts5", ["body", "tokenize=porter"] - # SQLite triggers are not dumped by schema.rb; keep rooms.messages_count honest after schema:load. - Room::MessagesCount.install! end + diff --git a/lib/tasks/room_messages_count.rake b/lib/tasks/room_messages_count.rake index dcb2d08..e042a14 100644 --- a/lib/tasks/room_messages_count.rake +++ b/lib/tasks/room_messages_count.rake @@ -1,38 +1,16 @@ -# schema.rb cannot dump SQLite triggers. Reinstall after loads, and re-append -# Room::MessagesCount.install! after dumps so the call is not lost. +# schema.rb cannot dump SQLite triggers — reinstall after schema loads. namespace :room_messages_count do task ensure: :environment do ActiveRecord::Base.connection_pool.with_connection do |connection| - next unless connection.adapter_name.match?(/sqlite/i) - Room::MessagesCount.ensure!(connection) end end - - task append_schema_install: :environment do - schema = Rails.root.join("db/schema.rb") - contents = schema.read - marker = "Room::MessagesCount.install!" - next if contents.include?(marker) - - contents.sub!(/\nend\n?\z/, <<~RUBY) - - # SQLite triggers are not dumped by schema.rb; keep rooms.messages_count honest after schema:load. - #{marker} - end - RUBY - schema.write(contents) - end end -{ - "db:schema:load" => "room_messages_count:ensure", - "db:test:load_schema" => "room_messages_count:ensure", - "db:schema:dump" => "room_messages_count:append_schema_install" -}.each do |task_name, enhancement| +%w[db:schema:load db:test:load_schema].each do |task_name| next unless Rake::Task.task_defined?(task_name) Rake::Task[task_name].enhance do - Rake::Task[enhancement].invoke + Rake::Task["room_messages_count:ensure"].invoke end end diff --git a/test/controllers/messages/by_bots_controller_test.rb b/test/controllers/messages/by_bots_controller_test.rb index a7fc835..4345dca 100644 --- a/test/controllers/messages/by_bots_controller_test.rb +++ b/test/controllers/messages/by_bots_controller_test.rb @@ -103,7 +103,6 @@ class Messages::ByBotsControllerTest < ActionDispatch::IntegrationTest json = JSON.parse(response.body) assert_equal Message::PAGE_SIZE, json.size - assert_equal @room.reload.messages_count.to_s, response.headers["X-Total-Count"] assert_equal "41", response.headers["X-Total-Count"] assert_not_includes json.map { it["id"] }, messages(:fourth).id diff --git a/test/models/room/messages_count_test.rb b/test/models/room/messages_count_test.rb index 53deac1..911b21e 100644 --- a/test/models/room/messages_count_test.rb +++ b/test/models/room/messages_count_test.rb @@ -5,7 +5,12 @@ class Room::MessagesCountTest < ActiveSupport::TestCase setup do @room = rooms(:designers) @other_room = rooms(:pets) - synchronize!(@room, @other_room) + end + + test "fixture rooms start with an accurate messages_count" do + [ rooms(:watercooler), rooms(:designers), rooms(:bender_and_kevin) ].each do |room| + assert_equal room.messages.count, room.messages_count, "#{room.name || room.id} fixture count" + end end test "ActiveRecord create and destroy adjust the counter once" do @@ -64,7 +69,6 @@ class Room::MessagesCountTest < ActiveSupport::TestCase test "moving a message between rooms moves the counter" do message = @room.messages.create!(creator: users(:jason), body: "Move me", client_message_id: "count-move") - synchronize!(@room, @other_room) assert_difference -> { @room.reload.messages_count }, -1 do assert_difference -> { @other_room.reload.messages_count }, +1 do @@ -76,6 +80,31 @@ class Room::MessagesCountTest < ActiveSupport::TestCase assert_equal @other_room.messages.count, @other_room.reload.messages_count end + test "Message does not declare an ActiveRecord counter_cache" do + reflection = Message.reflect_on_association(:room) + assert_not reflection.counter_cache_column + end +end + +# Destructive trigger DDL and foreign connections need a committed DB. +class Room::MessagesCountLifecycleTest < ActiveSupport::TestCase + self.use_transactional_tests = false + + setup do + Room::MessagesCount.ensure! + @room = rooms(:designers) + @other_room = rooms(:pets) + Room::MessagesCount.backfill! + @room.reload + @other_room.reload + end + + teardown do + Message.where("client_message_id LIKE ?", "count-%").delete_all + Room::MessagesCount.ensure! + Room::MessagesCount.backfill! + end + test "ensure! installs missing triggers without changing existing counts" do before = @room.reload.messages_count Room::MessagesCount.uninstall! @@ -84,68 +113,41 @@ class Room::MessagesCountTest < ActiveSupport::TestCase Room::MessagesCount.ensure! - assert Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) + assert Room::MessagesCount::TRIGGERS.all? { |name| + Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, name) + } assert_equal before, @room.reload.messages_count assert_difference -> { @room.reload.messages_count }, +1 do @room.messages.create!(creator: users(:jason), body: "After ensure", client_message_id: "count-ensure") end - ensure - Room::MessagesCount.ensure! end - test "backfill plus triggers do not double-count ActiveRecord writes" do + test "ensure! repairs a partial trigger install" do Room::MessagesCount.uninstall! ActiveRecord::Base.connection.execute <<~SQL - UPDATE rooms SET messages_count = ( - SELECT COUNT(*) FROM messages WHERE messages.room_id = rooms.id - ) + CREATE TRIGGER #{Room::MessagesCount::INSERT_TRIGGER} AFTER INSERT ON messages + BEGIN + UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id; + END SQL - Room::MessagesCount.install! - assert_equal @room.messages.count, @room.reload.messages_count + assert Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) + assert_not Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::DELETE_TRIGGER) - assert_difference -> { @room.reload.messages_count }, +1 do - assert_difference -> { @room.messages.count }, +1 do - @room.messages.create!(creator: users(:jason), body: "Once", client_message_id: "count-no-double") - end - end - - assert_equal @room.messages.count, @room.reload.messages_count - ensure Room::MessagesCount.ensure! - end - private - def synchronize!(*rooms) - Room::MessagesCount.ensure! - Room::MessagesCount.backfill! - rooms.each(&:reload) - end -end - -# Foreign connections cannot join the transactional fixture lock; run outside it. -class Room::MessagesCountForeignConnectionTest < ActiveSupport::TestCase - self.use_transactional_tests = false - - setup do - Room::MessagesCount.ensure! - @room = rooms(:designers) - Room::MessagesCount.backfill! - @room.reload - end - - teardown do - Message.where(client_message_id: "count-foreign-insert").delete_all - Room::MessagesCount.backfill! + assert Room::MessagesCount::TRIGGERS.all? { |name| + Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, name) + } end test "foreign SQLite connections keep the counter in step" do path = File.expand_path(ActiveRecord::Base.connection_db_config.database) now = Time.current.utc.strftime("%Y-%m-%d %H:%M:%S.%6N") before = @room.reload.messages_count + other_before = @other_room.reload.messages_count - # Release AR's checkout so the native connection can write. ActiveRecord::Base.connection_pool.release_connection SQLite3::Database.new(path) do |db| @@ -163,12 +165,22 @@ class Room::MessagesCountForeignConnectionTest < ActiveSupport::TestCase ActiveRecord::Base.connection_pool.release_connection + SQLite3::Database.new(path) do |db| + db.busy_timeout = 5_000 + db.execute("UPDATE messages SET room_id = ? WHERE id = ?", [ @other_room.id, foreign_id ]) + end + + assert_equal before, @room.reload.messages_count + assert_equal other_before + 1, @other_room.reload.messages_count + + ActiveRecord::Base.connection_pool.release_connection + SQLite3::Database.new(path) do |db| db.busy_timeout = 5_000 db.execute("DELETE FROM messages WHERE id = ?", [ foreign_id ]) end assert_equal before, @room.reload.messages_count - assert_equal @room.messages.count, @room.messages_count + assert_equal other_before, @other_room.reload.messages_count end end diff --git a/test/test_helper.rb b/test/test_helper.rb index cf47a46..6e3fa8b 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -13,8 +13,8 @@ Room::MessagesCount.ensure! WebMock.enable! module RoomMessagesCountFixtures - # Fixture YAML loads alphabetically, so messages are inserted before rooms. - # INSERT triggers then update zero room rows; reconcile once after load. + # Fixture YAML still inserts messages before rooms via some load paths, so + # INSERT triggers can update zero room rows. Reconcile once after load. def load_fixtures(config) fixtures = super Room::MessagesCount.backfill! @@ -28,15 +28,13 @@ class ActiveSupport::TestCase parallelize(workers: :number_of_processors) - # Setup all fixtures in test/fixtures/*.yml for all tests in alphabetical order. - fixtures :all + # Prefer rooms before messages when the loader honors declaration order. + fixtures :accounts, :users, :rooms, :memberships, :messages, "action_text/rich_texts", + :boosts, :searches, :sessions, :webhooks, "push/subscriptions" include SessionTestHelper, MentionTestHelper, TurboTestHelper, DnsTestHelper setup do - # DROP TRIGGER commits outside transactional fixtures; put triggers back each test. - Room::MessagesCount.ensure! - ActionCable.server.pubsub.clear Rails.configuration.tap do |config| @@ -52,4 +50,3 @@ class ActiveSupport::TestCase WebMock.reset! end end - From 32748e7fd64762c6e5284fb23b91358f11db78ec Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 20:14:13 +0000 Subject: [PATCH 3/4] Keep fixtures :all; isolate trigger lifecycle tests Drop the explicit fixture list and prepend module. Override load_fixtures only long enough to ensure! + backfill after alphabetical fixture load. Move destructive trigger DDL into its own test file so parallel CI workers do not strip triggers from the counter examples. Co-authored-by: Thomas Klemm --- .../room/messages_count_lifecycle_test.rb | 102 ++++++++++++++++++ test/models/room/messages_count_test.rb | 100 ----------------- test/test_helper.rb | 29 ++--- 3 files changed, 113 insertions(+), 118 deletions(-) create mode 100644 test/models/room/messages_count_lifecycle_test.rb diff --git a/test/models/room/messages_count_lifecycle_test.rb b/test/models/room/messages_count_lifecycle_test.rb new file mode 100644 index 0000000..295e178 --- /dev/null +++ b/test/models/room/messages_count_lifecycle_test.rb @@ -0,0 +1,102 @@ +require "test_helper" +require "sqlite3" + +# Destructive trigger DDL and foreign connections need their own file so parallel +# workers (and transactional tests in messages_count_test) keep a stable trigger set. +class Room::MessagesCountLifecycleTest < ActiveSupport::TestCase + self.use_transactional_tests = false + + setup do + Room::MessagesCount.ensure! + @room = rooms(:designers) + @other_room = rooms(:pets) + Room::MessagesCount.backfill! + @room.reload + @other_room.reload + end + + teardown do + Message.where("client_message_id LIKE ?", "count-%").delete_all + Room::MessagesCount.ensure! + Room::MessagesCount.backfill! + end + + test "ensure! installs missing triggers without changing existing counts" do + before = @room.reload.messages_count + Room::MessagesCount.uninstall! + + assert_not Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) + + Room::MessagesCount.ensure! + + assert Room::MessagesCount::TRIGGERS.all? { |name| + Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, name) + } + assert_equal before, @room.reload.messages_count + + assert_difference -> { @room.reload.messages_count }, +1 do + @room.messages.create!(creator: users(:jason), body: "After ensure", client_message_id: "count-ensure") + end + end + + test "ensure! repairs a partial trigger install" do + Room::MessagesCount.uninstall! + ActiveRecord::Base.connection.execute <<~SQL + CREATE TRIGGER #{Room::MessagesCount::INSERT_TRIGGER} AFTER INSERT ON messages + BEGIN + UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id; + END + SQL + + assert Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) + assert_not Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::DELETE_TRIGGER) + + Room::MessagesCount.ensure! + + assert Room::MessagesCount::TRIGGERS.all? { |name| + Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, name) + } + end + + test "foreign SQLite connections keep the counter in step" do + path = File.expand_path(ActiveRecord::Base.connection_db_config.database) + now = Time.current.utc.strftime("%Y-%m-%d %H:%M:%S.%6N") + before = @room.reload.messages_count + other_before = @other_room.reload.messages_count + + ActiveRecord::Base.connection_pool.release_connection + + SQLite3::Database.new(path) do |db| + db.busy_timeout = 5_000 + db.execute( + "INSERT INTO messages (room_id, creator_id, client_message_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?)", + [ @room.id, users(:david).id, "count-foreign-insert", now, now ] + ) + end + + assert_equal before + 1, @room.reload.messages_count + assert_equal @room.messages.count, @room.messages_count + + foreign_id = Message.find_by!(client_message_id: "count-foreign-insert").id + + ActiveRecord::Base.connection_pool.release_connection + + SQLite3::Database.new(path) do |db| + db.busy_timeout = 5_000 + db.execute("UPDATE messages SET room_id = ? WHERE id = ?", [ @other_room.id, foreign_id ]) + end + + assert_equal before, @room.reload.messages_count + assert_equal other_before + 1, @other_room.reload.messages_count + + ActiveRecord::Base.connection_pool.release_connection + + SQLite3::Database.new(path) do |db| + db.busy_timeout = 5_000 + db.execute("DELETE FROM messages WHERE id = ?", [ foreign_id ]) + end + + assert_equal before, @room.reload.messages_count + assert_equal other_before, @other_room.reload.messages_count + end +end diff --git a/test/models/room/messages_count_test.rb b/test/models/room/messages_count_test.rb index 911b21e..3b0e447 100644 --- a/test/models/room/messages_count_test.rb +++ b/test/models/room/messages_count_test.rb @@ -1,5 +1,4 @@ require "test_helper" -require "sqlite3" class Room::MessagesCountTest < ActiveSupport::TestCase setup do @@ -85,102 +84,3 @@ class Room::MessagesCountTest < ActiveSupport::TestCase assert_not reflection.counter_cache_column end end - -# Destructive trigger DDL and foreign connections need a committed DB. -class Room::MessagesCountLifecycleTest < ActiveSupport::TestCase - self.use_transactional_tests = false - - setup do - Room::MessagesCount.ensure! - @room = rooms(:designers) - @other_room = rooms(:pets) - Room::MessagesCount.backfill! - @room.reload - @other_room.reload - end - - teardown do - Message.where("client_message_id LIKE ?", "count-%").delete_all - Room::MessagesCount.ensure! - Room::MessagesCount.backfill! - end - - test "ensure! installs missing triggers without changing existing counts" do - before = @room.reload.messages_count - Room::MessagesCount.uninstall! - - assert_not Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) - - Room::MessagesCount.ensure! - - assert Room::MessagesCount::TRIGGERS.all? { |name| - Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, name) - } - assert_equal before, @room.reload.messages_count - - assert_difference -> { @room.reload.messages_count }, +1 do - @room.messages.create!(creator: users(:jason), body: "After ensure", client_message_id: "count-ensure") - end - end - - test "ensure! repairs a partial trigger install" do - Room::MessagesCount.uninstall! - ActiveRecord::Base.connection.execute <<~SQL - CREATE TRIGGER #{Room::MessagesCount::INSERT_TRIGGER} AFTER INSERT ON messages - BEGIN - UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id; - END - SQL - - assert Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) - assert_not Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::DELETE_TRIGGER) - - Room::MessagesCount.ensure! - - assert Room::MessagesCount::TRIGGERS.all? { |name| - Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, name) - } - end - - test "foreign SQLite connections keep the counter in step" do - path = File.expand_path(ActiveRecord::Base.connection_db_config.database) - now = Time.current.utc.strftime("%Y-%m-%d %H:%M:%S.%6N") - before = @room.reload.messages_count - other_before = @other_room.reload.messages_count - - ActiveRecord::Base.connection_pool.release_connection - - SQLite3::Database.new(path) do |db| - db.busy_timeout = 5_000 - db.execute( - "INSERT INTO messages (room_id, creator_id, client_message_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?)", - [ @room.id, users(:david).id, "count-foreign-insert", now, now ] - ) - end - - assert_equal before + 1, @room.reload.messages_count - assert_equal @room.messages.count, @room.messages_count - - foreign_id = Message.find_by!(client_message_id: "count-foreign-insert").id - - ActiveRecord::Base.connection_pool.release_connection - - SQLite3::Database.new(path) do |db| - db.busy_timeout = 5_000 - db.execute("UPDATE messages SET room_id = ? WHERE id = ?", [ @other_room.id, foreign_id ]) - end - - assert_equal before, @room.reload.messages_count - assert_equal other_before + 1, @other_room.reload.messages_count - - ActiveRecord::Base.connection_pool.release_connection - - SQLite3::Database.new(path) do |db| - db.busy_timeout = 5_000 - db.execute("DELETE FROM messages WHERE id = ?", [ foreign_id ]) - end - - assert_equal before, @room.reload.messages_count - assert_equal other_before, @other_room.reload.messages_count - end -end diff --git a/test/test_helper.rb b/test/test_helper.rb index 6e3fa8b..30b87be 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -6,31 +6,15 @@ require "mocha/minitest" require "webmock/minitest" require "turbo/broadcastable/test_helper" -# maintain_test_schema! may reload schema.rb after after_initialize; triggers are -# not dumped, so ensure they exist before fixtures insert messages. -Room::MessagesCount.ensure! - WebMock.enable! -module RoomMessagesCountFixtures - # Fixture YAML still inserts messages before rooms via some load paths, so - # INSERT triggers can update zero room rows. Reconcile once after load. - def load_fixtures(config) - fixtures = super - Room::MessagesCount.backfill! - fixtures - end -end - class ActiveSupport::TestCase include ActiveJob::TestHelper - prepend RoomMessagesCountFixtures parallelize(workers: :number_of_processors) - # Prefer rooms before messages when the loader honors declaration order. - fixtures :accounts, :users, :rooms, :memberships, :messages, "action_text/rich_texts", - :boosts, :searches, :sessions, :webhooks, "push/subscriptions" + # Setup all fixtures in test/fixtures/*.yml for all tests in alphabetical order. + fixtures :all include SessionTestHelper, MentionTestHelper, TurboTestHelper, DnsTestHelper @@ -49,4 +33,13 @@ class ActiveSupport::TestCase teardown do WebMock.reset! end + + # fixtures :all inserts messages before rooms, so INSERT triggers cannot count + # yet. Backfill once after load; ensure! covers schema.rb (triggers not dumped). + def load_fixtures(config) + fixtures = super + Room::MessagesCount.ensure! + Room::MessagesCount.backfill! + fixtures + end end From b4ab2df20f59c91c055e8005e05fb12dce82e12d Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 20:58:32 +0000 Subject: [PATCH 4/4] Repair messages_count triggers in one SQLite write transaction ensure! now rechecks trigger presence under BEGIN IMMEDIATE, backfills drifted rooms.messages_count, and reinstalls all three triggers before commit so concurrent writers and concurrent boot repairs cannot observe a partial install or keep a wrong total. install! uses the same immediate write lock for drop/recreate. Lifecycle regressions cover missing/partial trigger drift, concurrent ensure!, and writes racing repair. Co-authored-by: Thomas Klemm --- app/models/room/messages_count.rb | 77 ++++++--- .../room/messages_count_lifecycle_test.rb | 153 ++++++++++++++++-- 2 files changed, 198 insertions(+), 32 deletions(-) diff --git a/app/models/room/messages_count.rb b/app/models/room/messages_count.rb index 53254ac..948724e 100644 --- a/app/models/room/messages_count.rb +++ b/app/models/room/messages_count.rb @@ -9,27 +9,9 @@ class Room::MessagesCount class << self def install!(connection = ActiveRecord::Base.connection) - uninstall!(connection) - connection.execute <<~SQL - CREATE TRIGGER #{INSERT_TRIGGER} AFTER INSERT ON messages - BEGIN - UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id; - END - SQL - connection.execute <<~SQL - CREATE TRIGGER #{DELETE_TRIGGER} AFTER DELETE ON messages - BEGIN - UPDATE rooms SET messages_count = messages_count - 1 WHERE id = OLD.room_id; - END - SQL - connection.execute <<~SQL - CREATE TRIGGER #{UPDATE_TRIGGER} AFTER UPDATE OF room_id ON messages - WHEN OLD.room_id IS NOT NEW.room_id - BEGIN - UPDATE rooms SET messages_count = messages_count - 1 WHERE id = OLD.room_id; - UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id; - END - SQL + with_immediate_write(connection) do + replace_triggers!(connection) + end end def uninstall!(connection = ActiveRecord::Base.connection) @@ -47,13 +29,21 @@ class Room::MessagesCount end # schema.rb does not dump SQLite triggers; reinstall after schema:load. + # Repair rechecks, rebuilds counts, and installs triggers in one write txn + # so concurrent writers and concurrent boot repairs cannot observe a gap. def ensure!(connection = ActiveRecord::Base.connection) return unless connection.adapter_name.match?(/sqlite/i) return unless connection.data_source_exists?(:rooms) return unless connection.column_exists?(:rooms, :messages_count) - return if TRIGGERS.all? { |name| trigger_installed?(connection, name) } + return if triggers_installed?(connection) - install!(connection) + with_immediate_write(connection) do + next if triggers_installed?(connection) + + uninstall!(connection) + backfill!(connection) + create_triggers!(connection) + end end def trigger_installed?(connection, name) @@ -61,5 +51,46 @@ class Room::MessagesCount "SELECT 1 FROM sqlite_master WHERE type = 'trigger' AND name = #{connection.quote(name)}" ).present? end + + def triggers_installed?(connection = ActiveRecord::Base.connection) + TRIGGERS.all? { |name| trigger_installed?(connection, name) } + end + + private + def with_immediate_write(connection) + if connection.transaction_open? + yield + else + connection.raw_connection.transaction(:immediate) { yield } + end + end + + def replace_triggers!(connection) + uninstall!(connection) + create_triggers!(connection) + end + + def create_triggers!(connection) + connection.execute <<~SQL + CREATE TRIGGER #{INSERT_TRIGGER} AFTER INSERT ON messages + BEGIN + UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id; + END + SQL + connection.execute <<~SQL + CREATE TRIGGER #{DELETE_TRIGGER} AFTER DELETE ON messages + BEGIN + UPDATE rooms SET messages_count = messages_count - 1 WHERE id = OLD.room_id; + END + SQL + connection.execute <<~SQL + CREATE TRIGGER #{UPDATE_TRIGGER} AFTER UPDATE OF room_id ON messages + WHEN OLD.room_id IS NOT NEW.room_id + BEGIN + UPDATE rooms SET messages_count = messages_count - 1 WHERE id = OLD.room_id; + UPDATE rooms SET messages_count = messages_count + 1 WHERE id = NEW.room_id; + END + SQL + end end end diff --git a/test/models/room/messages_count_lifecycle_test.rb b/test/models/room/messages_count_lifecycle_test.rb index 295e178..fff8a3f 100644 --- a/test/models/room/messages_count_lifecycle_test.rb +++ b/test/models/room/messages_count_lifecycle_test.rb @@ -21,25 +21,24 @@ class Room::MessagesCountLifecycleTest < ActiveSupport::TestCase Room::MessagesCount.backfill! end - test "ensure! installs missing triggers without changing existing counts" do + test "ensure! installs missing triggers and keeps accurate counts" do before = @room.reload.messages_count Room::MessagesCount.uninstall! - assert_not Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) + assert_not Room::MessagesCount.triggers_installed? Room::MessagesCount.ensure! - assert Room::MessagesCount::TRIGGERS.all? { |name| - Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, name) - } + assert Room::MessagesCount.triggers_installed? assert_equal before, @room.reload.messages_count + assert_equal @room.messages.count, @room.messages_count assert_difference -> { @room.reload.messages_count }, +1 do @room.messages.create!(creator: users(:jason), body: "After ensure", client_message_id: "count-ensure") end end - test "ensure! repairs a partial trigger install" do + test "ensure! repairs a partial trigger install and backfills drifted counts" do Room::MessagesCount.uninstall! ActiveRecord::Base.connection.execute <<~SQL CREATE TRIGGER #{Room::MessagesCount::INSERT_TRIGGER} AFTER INSERT ON messages @@ -51,11 +50,147 @@ class Room::MessagesCountLifecycleTest < ActiveSupport::TestCase assert Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::INSERT_TRIGGER) assert_not Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, Room::MessagesCount::DELETE_TRIGGER) + # Delete fires with no delete trigger → counter drifts high. + drifted = @room.messages.create!(creator: users(:jason), body: "Drift", client_message_id: "count-partial-drift") + ActiveRecord::Base.connection.execute("DELETE FROM messages WHERE id = #{drifted.id}") + assert_operator @room.reload.messages_count, :>, @room.messages.count + Room::MessagesCount.ensure! - assert Room::MessagesCount::TRIGGERS.all? { |name| - Room::MessagesCount.trigger_installed?(ActiveRecord::Base.connection, name) - } + assert Room::MessagesCount.triggers_installed? + assert_equal @room.messages.count, @room.reload.messages_count + end + + test "ensure! backfills writes that landed while the insert trigger was missing" do + Room::MessagesCount.uninstall! + before_count = @room.messages.count + before_cached = @room.reload.messages_count + assert_equal before_count, before_cached + + path = File.expand_path(ActiveRecord::Base.connection_db_config.database) + now = Time.current.utc.strftime("%Y-%m-%d %H:%M:%S.%6N") + ActiveRecord::Base.connection_pool.release_connection + + SQLite3::Database.new(path) do |db| + db.busy_timeout = 5_000 + db.execute( + "INSERT INTO messages (room_id, creator_id, client_message_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?)", + [ @room.id, users(:david).id, "count-missing-insert", now, now ] + ) + end + + assert_equal before_count + 1, @room.messages.count + assert_equal before_cached, @room.reload.messages_count + + Room::MessagesCount.ensure! + + assert Room::MessagesCount.triggers_installed? + assert_equal @room.messages.count, @room.reload.messages_count + assert_equal before_cached + 1, @room.messages_count + end + + test "concurrent ensure! repairs leave triggers installed and counts accurate" do + Room::MessagesCount.uninstall! + path = File.expand_path(ActiveRecord::Base.connection_db_config.database) + now = Time.current.utc.strftime("%Y-%m-%d %H:%M:%S.%6N") + ActiveRecord::Base.connection_pool.release_connection + + SQLite3::Database.new(path) do |db| + db.busy_timeout = 5_000 + db.execute( + "INSERT INTO messages (room_id, creator_id, client_message_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?)", + [ @room.id, users(:david).id, "count-concurrent-drift", now, now ] + ) + end + + assert_not_equal @room.messages.count, @room.reload.messages_count + + errors = [] + errors_mutex = Mutex.new + + workers = 2.times.map do + Thread.new do + ActiveRecord::Base.connection_pool.with_connection do |connection| + Room::MessagesCount.ensure!(connection) + end + rescue => error + errors_mutex.synchronize { errors << error } + end + end + + workers.each(&:join) + + assert_empty errors, -> { errors.map(&:full_message).join("\n") } + assert Room::MessagesCount.triggers_installed? + assert_equal @room.messages.count, @room.reload.messages_count + end + + test "writes concurrent with ensure! repair leave accurate counts" do + Room::MessagesCount.uninstall! + path = File.expand_path(ActiveRecord::Base.connection_db_config.database) + now = Time.current.utc.strftime("%Y-%m-%d %H:%M:%S.%6N") + ActiveRecord::Base.connection_pool.release_connection + + errors = [] + errors_mutex = Mutex.new + + ensure_thread = Thread.new do + ActiveRecord::Base.connection_pool.with_connection do |connection| + Room::MessagesCount.ensure!(connection) + end + rescue => error + errors_mutex.synchronize { errors << error } + end + + insert_thread = Thread.new do + SQLite3::Database.new(path) do |db| + db.busy_timeout = 10_000 + db.execute( + "INSERT INTO messages (room_id, creator_id, client_message_id, created_at, updated_at) VALUES (?, ?, ?, ?, ?)", + [ @room.id, users(:david).id, "count-during-repair", now, now ] + ) + end + rescue => error + errors_mutex.synchronize { errors << error } + end + + [ ensure_thread, insert_thread ].each(&:join) + + assert_empty errors, -> { errors.map(&:full_message).join("\n") } + assert Room::MessagesCount.triggers_installed? + assert_equal @room.messages.count, @room.reload.messages_count + assert Message.exists?(client_message_id: "count-during-repair") + end + + test "install! never exposes a partial trigger set to other connections" do + Room::MessagesCount.uninstall! + path = File.expand_path(ActiveRecord::Base.connection_db_config.database) + ActiveRecord::Base.connection_pool.release_connection + + stop = false + partial_snapshots = [] + snapshots_mutex = Mutex.new + + watcher = Thread.new do + SQLite3::Database.new(path) do |db| + db.busy_timeout = 5_000 + until stop + names = db.execute( + "SELECT name FROM sqlite_master WHERE type = 'trigger' AND name IN (#{Room::MessagesCount::TRIGGERS.map { "'#{it}'" }.join(", ")})" + ).flatten + if names.any? && names.sort != Room::MessagesCount::TRIGGERS.sort + snapshots_mutex.synchronize { partial_snapshots << names.sort } + end + end + end + end + + 25.times { Room::MessagesCount.install! } + stop = true + watcher.join + + assert_empty partial_snapshots + assert Room::MessagesCount.triggers_installed? end test "foreign SQLite connections keep the counter in step" do