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..948724e --- /dev/null +++ b/app/models/room/messages_count.rb @@ -0,0 +1,96 @@ +# 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" + TRIGGERS = [ INSERT_TRIGGER, DELETE_TRIGGER, UPDATE_TRIGGER ].freeze + + class << self + def install!(connection = ActiveRecord::Base.connection) + with_immediate_write(connection) do + replace_triggers!(connection) + end + end + + def uninstall!(connection = ActiveRecord::Base.connection) + TRIGGERS.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. + # 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_installed?(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) + connection.select_value( + "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/config/initializers/room_messages_count.rb b/config/initializers/room_messages_count.rb new file mode 100644 index 0000000..6c1c057 --- /dev/null +++ b/config/initializers/room_messages_count.rb @@ -0,0 +1,5 @@ +# schema.rb cannot dump SQLite triggers. Cover existing DBs that lost them. +Rails.application.config.after_initialize do + 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 new file mode 100644 index 0000000..55286fa --- /dev/null +++ b/db/migrate/20261007190000_add_messages_count_to_rooms.rb @@ -0,0 +1,15 @@ +class AddMessagesCountToRooms < ActiveRecord::Migration[8.2] + def up + 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. + Room::MessagesCount.backfill!(connection) + Room::MessagesCount.install!(connection) + end + + def down + Room::MessagesCount.uninstall!(connection) + remove_column :rooms, :messages_count + end +end diff --git a/db/schema.rb b/db/schema.rb index 460193a..ccb79c9 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| @@ -183,3 +184,4 @@ ActiveRecord::Schema[8.2].define(version: 2026_10_05_022000) do # 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"] end + diff --git a/lib/tasks/room_messages_count.rake b/lib/tasks/room_messages_count.rake new file mode 100644 index 0000000..e042a14 --- /dev/null +++ b/lib/tasks/room_messages_count.rake @@ -0,0 +1,16 @@ +# 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| + Room::MessagesCount.ensure!(connection) + end + end +end + +%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["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 da6f5cc..4345dca 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,7 +96,9 @@ 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) @@ -119,7 +124,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_lifecycle_test.rb b/test/models/room/messages_count_lifecycle_test.rb new file mode 100644 index 0000000..fff8a3f --- /dev/null +++ b/test/models/room/messages_count_lifecycle_test.rb @@ -0,0 +1,237 @@ +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 and keeps accurate counts" do + before = @room.reload.messages_count + Room::MessagesCount.uninstall! + + assert_not Room::MessagesCount.triggers_installed? + + Room::MessagesCount.ensure! + + 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 and backfills drifted counts" 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) + + # 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_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 + 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 new file mode 100644 index 0000000..3b0e447 --- /dev/null +++ b/test/models/room/messages_count_test.rb @@ -0,0 +1,86 @@ +require "test_helper" + +class Room::MessagesCountTest < ActiveSupport::TestCase + setup do + @room = rooms(:designers) + @other_room = rooms(:pets) + 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 + 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") + + 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 "Message does not declare an ActiveRecord counter_cache" do + reflection = Message.reflect_on_association(:room) + assert_not reflection.counter_cache_column + end +end diff --git a/test/test_helper.rb b/test/test_helper.rb index 5a998e9..30b87be 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -33,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