From 1bbb6d0c037d026af1fa4a8b91883d4fcb547af1 Mon Sep 17 00:00:00 2001
From: Mike Dalessio <mike@37signals.com>
Date: Thu, 20 Nov 2025 11:28:58 -0500
Subject: [PATCH] Fix race condition in Card::ActivitySpike::DetectionJob

The check-then-act pattern in `register_activity_spike` has been
replaced with `find_or_create_by!` to eliminate the race condition
that could lead to creating multiple activity spikes for a card. To
support this change, the `card_id` index on `cards_activity_spikes`
has been made unique.

ref: https://app.fizzy.do/5986089/cards/3063
---
 app/models/card/activity_spike/detector.rb      |  6 +-----
 ..._index_to_card_activity_spikes_on_card_id.rb | 17 +++++++++++++++++
 db/schema.rb                                    |  4 ++--
 .../models/card/activity_spike/detector_test.rb | 15 +++++++++++++++
 4 files changed, 35 insertions(+), 7 deletions(-)
 create mode 100644 db/migrate/20251120203100_add_unique_index_to_card_activity_spikes_on_card_id.rb

diff --git a/app/models/card/activity_spike/detector.rb b/app/models/card/activity_spike/detector.rb
index b589e0c26..b533318e3 100644
--- a/app/models/card/activity_spike/detector.rb
+++ b/app/models/card/activity_spike/detector.rb
@@ -21,11 +21,7 @@ class Card::ActivitySpike::Detector
 
     def register_activity_spike
       Card.suppressing_turbo_broadcasts do
-        if card.activity_spike
-          card.activity_spike.touch
-        else
-          card.create_activity_spike!
-        end
+        Card::ActivitySpike.find_or_create_by!(card: card).touch
       end
     end
 
diff --git a/db/migrate/20251120203100_add_unique_index_to_card_activity_spikes_on_card_id.rb b/db/migrate/20251120203100_add_unique_index_to_card_activity_spikes_on_card_id.rb
new file mode 100644
index 000000000..850c8e604
--- /dev/null
+++ b/db/migrate/20251120203100_add_unique_index_to_card_activity_spikes_on_card_id.rb
@@ -0,0 +1,17 @@
+class AddUniqueIndexToCardActivitySpikesOnCardId < ActiveRecord::Migration[8.2]
+  def change
+    reversible do |dir|
+      dir.up do
+        execute <<-SQL
+          DELETE s1 FROM card_activity_spikes s1
+          INNER JOIN card_activity_spikes s2
+          WHERE s1.card_id = s2.card_id
+          AND s1.updated_at < s2.updated_at
+        SQL
+      end
+    end
+
+    remove_index :card_activity_spikes, :card_id
+    add_index :card_activity_spikes, :card_id, unique: true
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index bf2395905..e6a403fee 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: 2025_11_20_194700) do
+ActiveRecord::Schema[8.2].define(version: 2025_11_20_203100) do
   create_table "accesses", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
     t.datetime "accessed_at"
     t.uuid "account_id", null: false
@@ -150,7 +150,7 @@ ActiveRecord::Schema[8.2].define(version: 2025_11_20_194700) do
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.index ["account_id"], name: "index_card_activity_spikes_on_account_id"
-    t.index ["card_id"], name: "index_card_activity_spikes_on_card_id"
+    t.index ["card_id"], name: "index_card_activity_spikes_on_card_id", unique: true
   end
 
   create_table "card_engagements", id: :uuid, charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t|
diff --git a/test/models/card/activity_spike/detector_test.rb b/test/models/card/activity_spike/detector_test.rb
index 0eebe479e..6fe6183b7 100644
--- a/test/models/card/activity_spike/detector_test.rb
+++ b/test/models/card/activity_spike/detector_test.rb
@@ -39,6 +39,21 @@ class Card::ActivitySpike::DetectorTest < ActiveSupport::TestCase
     assert @card.reload.activity_spike.updated_at > original_last_spike_at
   end
 
+  test "concurrent spike creation should not create multiple spikes for a card" do
+    multiple_people_comment_on(@card)
+    @card.activity_spike&.destroy
+
+    5.times.map do
+      Thread.new do
+        ActiveRecord::Base.connection_pool.with_connection do
+          Card.find(@card.id).detect_activity_spikes
+        end
+      end
+    end.each(&:join)
+
+    assert_equal 1, Card::ActivitySpike.where(card: @card).count
+  end
+
   private
     def assert_activity_spike_detected(card: @card)
       assert card.activity_spike.blank?