From 1f5858098b9fea518229758ed9aab67185ffe72b Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 7 Oct 2026 19:36:01 +0000 Subject: [PATCH] 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 +