From 2061ab4ab2efc787959a8015af48a3f42ee211ac Mon Sep 17 00:00:00 2001
From: Jorge Manrubia <jorge@hey.com>
Date: Thu, 27 Nov 2025 11:55:47 +0100
Subject: [PATCH] Use dedicated sequence record instead of deal with issues
 with ids and uuids when using the previous approach

---
 app/models/account.rb                         |  2 +-
 app/models/account/external_id_sequence.rb    | 26 ++++++++++++
 app/models/external_account.rb                |  5 ---
 ...01_create_account_external_id_sequences.rb |  9 ++++
 ...20251127000001_create_external_accounts.rb | 15 -------
 db/schema_sqlite.rb                           |  7 +++-
 .../account/external_id_sequence_test.rb      | 42 +++++++++++++++++++
 test/models/account_test.rb                   | 13 +++---
 8 files changed, 91 insertions(+), 28 deletions(-)
 create mode 100644 app/models/account/external_id_sequence.rb
 delete mode 100644 app/models/external_account.rb
 create mode 100644 db/migrate/20251127000001_create_account_external_id_sequences.rb
 delete mode 100644 db/migrate/20251127000001_create_external_accounts.rb
 create mode 100644 test/models/account/external_id_sequence_test.rb

diff --git a/app/models/account.rb b/app/models/account.rb
index 7e7977967..887ce708d 100644
--- a/app/models/account.rb
+++ b/app/models/account.rb
@@ -39,6 +39,6 @@ class Account < ApplicationRecord
 
   private
     def assign_external_account_id
-      self.external_account_id ||= ExternalAccount.create!.id
+      self.external_account_id ||= ExternalIdSequence.next
     end
 end
diff --git a/app/models/account/external_id_sequence.rb b/app/models/account/external_id_sequence.rb
new file mode 100644
index 000000000..8d9cecf6d
--- /dev/null
+++ b/app/models/account/external_id_sequence.rb
@@ -0,0 +1,26 @@
+# Provides sequential IDs for +external_account_id+ when creating accounts without one.
+class Account::ExternalIdSequence < ApplicationRecord
+  class << self
+    def next
+      with_lock do |sequence|
+        sequence.increment!(:value).value
+      end
+    end
+
+    def value
+      first&.value
+    end
+
+    private
+      def with_lock
+        transaction do
+          sequence = lock.first_or_create!(value: initial_value)
+          yield sequence
+        end
+      end
+
+      def initial_value
+        Account.maximum(:external_account_id) || 0
+      end
+  end
+end
diff --git a/app/models/external_account.rb b/app/models/external_account.rb
deleted file mode 100644
index 11b5c4d5e..000000000
--- a/app/models/external_account.rb
+++ /dev/null
@@ -1,5 +0,0 @@
-# This is meant to provide a sequential ID for +external_account_id+ when creating
-# accounts without one. The SaaS version of the app uses that column, but most apps
-# can just go with the default handling here.
-class ExternalAccount < ApplicationRecord
-end
diff --git a/db/migrate/20251127000001_create_account_external_id_sequences.rb b/db/migrate/20251127000001_create_account_external_id_sequences.rb
new file mode 100644
index 000000000..834b80c5b
--- /dev/null
+++ b/db/migrate/20251127000001_create_account_external_id_sequences.rb
@@ -0,0 +1,9 @@
+class CreateAccountExternalIdSequences < ActiveRecord::Migration[8.0]
+  def change
+    create_table :account_external_id_sequences do |t|
+      t.bigint :value, null: false, default: 0
+
+      t.index :value, unique: true
+    end
+  end
+end
diff --git a/db/migrate/20251127000001_create_external_accounts.rb b/db/migrate/20251127000001_create_external_accounts.rb
deleted file mode 100644
index 8dee2feaf..000000000
--- a/db/migrate/20251127000001_create_external_accounts.rb
+++ /dev/null
@@ -1,15 +0,0 @@
-class CreateExternalAccounts < ActiveRecord::Migration[8.0]
-  def up
-    create_table :external_accounts do |t|
-    end
-
-    max_id = Account.maximum(:external_account_id) || 0
-    if max_id > 0
-      execute "INSERT INTO external_accounts (id) VALUES (#{max_id})"
-    end
-  end
-
-  def down
-    drop_table :external_accounts
-  end
-end
diff --git a/db/schema_sqlite.rb b/db/schema_sqlite.rb
index fe2aad115..e4475e7ea 100644
--- a/db/schema_sqlite.rb
+++ b/db/schema_sqlite.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_25_130010) do
+ActiveRecord::Schema[8.2].define(version: 2025_11_27_000001) do
   create_table "accesses", id: :uuid, force: :cascade do |t|
     t.datetime "accessed_at"
     t.uuid "account_id", null: false
@@ -25,6 +25,11 @@ ActiveRecord::Schema[8.2].define(version: 2025_11_25_130010) do
     t.index ["user_id"], name: "index_accesses_on_user_id"
   end
 
+  create_table "account_external_id_sequences", force: :cascade do |t|
+    t.bigint "value", default: 0, null: false
+    t.index ["value"], name: "index_account_external_id_sequences_on_value", unique: true
+  end
+
   create_table "account_join_codes", id: :uuid, force: :cascade do |t|
     t.uuid "account_id", null: false
     t.string "code", limit: 255, null: false
diff --git a/test/models/account/external_id_sequence_test.rb b/test/models/account/external_id_sequence_test.rb
new file mode 100644
index 000000000..a7f8ffd18
--- /dev/null
+++ b/test/models/account/external_id_sequence_test.rb
@@ -0,0 +1,42 @@
+require "test_helper"
+
+class Account::ExternalIdSequenceTest < ActiveSupport::TestCase
+  setup do
+    Account::ExternalIdSequence.delete_all
+  end
+
+  test ".next returns sequential values" do
+    first_value = Account::ExternalIdSequence.next
+    second_value = Account::ExternalIdSequence.next
+    third_value = Account::ExternalIdSequence.next
+
+    assert_equal first_value + 1, second_value
+    assert_equal second_value + 1, third_value
+  end
+
+  test ".next initializes from maximum external_account_id" do
+    max_id = Account.maximum(:external_account_id) || 0
+
+    first_value = Account::ExternalIdSequence.next
+
+    assert_equal max_id + 1, first_value
+  end
+
+  test ".next creates single sequence record" do
+    3.times { Account::ExternalIdSequence.next }
+
+    assert_equal 1, Account::ExternalIdSequence.count
+  end
+
+  test ".next is concurrency-safe" do
+    values = 20.times.map do
+      Thread.new do
+        Account::ExternalIdSequence.next
+      end
+    end.map(&:value)
+
+    assert_equal 20, values.uniq.size, "All values should be unique"
+    assert_equal values.min..values.max, values.sort.first..values.sort.last
+    assert_equal 20, values.max - values.min + 1, "Values should be sequential with no gaps"
+  end
+end
diff --git a/test/models/account_test.rb b/test/models/account_test.rb
index 57f2406be..541ceb654 100644
--- a/test/models/account_test.rb
+++ b/test/models/account_test.rb
@@ -69,12 +69,13 @@ class AccountTest < ActiveSupport::TestCase
     assert_equal account1.external_account_id + 1, account2.external_account_id
   end
 
-  test "external_account_id can be overridden without creating sequence entry" do
-    custom_id = Account.last.id + 99999
+  test "external_account_id can be overridden" do
+    custom_id = 999999
+    sequence_value_before = Account::ExternalIdSequence.first_or_create!(value: 0).value
 
-    assert_no_difference -> { ExternalAccount.count } do
-      account = Account.create!(name: "Custom ID Account", external_account_id: custom_id)
-      assert_equal custom_id, account.external_account_id
-    end
+    account = Account.create!(name: "Custom ID Account", external_account_id: custom_id)
+
+    assert_equal custom_id, account.external_account_id
+    assert_equal sequence_value_before, Account::ExternalIdSequence.value
   end
 end