Fix race condition in slug generation with retry mechanism

CHANGELOG.md

@@ -1,3 +1,17 @@
+## [Unreleased]
+
+- Added retry handling for `ActiveRecord::RecordNotUnique` slug collisions during post-create slug persistence
+- Added `around_create` retry handling for insert-time slug collisions in pre-insert (`null: false`) slug strategies
+- Wrapped insert retries in savepoint transactions (`requires_new: true`) to keep PostgreSQL transactions retry-safe
+- Added regression coverage for insert-time and update-time slug race windows
+- Added optional PostgreSQL integration test for transaction-abort-safe insert retries
+
+### Breaking Changes (Behavioral)
+
+- **For NOT NULL slug columns:** On insert-time slug collision, the entire `around_create` chain re-executes, which means `before_create` callbacks **will** fire multiple times per collision. Move non-idempotent side effects (emails, jobs) to `after_create` to avoid duplication.
+- **Slug save now uses `save!` instead of `save`:** If a validation fails during slug-save (both `after_create` and `after_find` nil-slug repair), the new code raises `ActiveRecord::RecordInvalid` instead of silently skipping. This affects any record with nil slugs that triggers `update_slug_if_nil` on load.
+- **Removed `id_changed?` check from `set_slug`:** The check was redundant for the `after_create` path (where ID always just changed from nil). This should not affect normal usage.
+
 ## [0.2.0] - 2026-01-16
 
 - Added a full Minitest test suite

Gemfile

@@ -11,6 +11,8 @@ group :development, :test do
   gem "appraisal"
   gem "minitest", "~> 6.0"
   gem "minitest-mock"
+  # Optional: install manually for PostgreSQL integration tests (requires libpq)
+  # gem "pg"
   gem "rack-test"
   gem "simplecov", require: false
   gem "sqlite3", ">= 2.1"

README.md

@@ -81,8 +81,15 @@ Product.first.slug
 If your model has a `slug` attribute in the database, `slugifiable` will automatically generate a slug for that model upon instance creation, and save it to the DB.
 
 > [!IMPORTANT]
-> Your `slug` attribute **SHOULD NOT** have `null: false` in the migration / database. If it does, `slugifiable` will not be able to save the slug to the database, and will raise an error like `ERROR:  null value in column "slug" of relation "posts" violates not-null constraint (PG::NotNullViolation)`
-> This is because records are created without a slug, and the slug is generated later.
+> By default, `slugifiable` persists slugs after `create`, so a nullable `slug` column is the simplest setup.
+> If you need `null: false`, generate the slug before `INSERT` (for example with a `before_validation` callback that sets `self.slug = compute_slug` when blank).
+> `slugifiable` handles slug unique-collision retries for both post-create and pre-insert slug strategies.
+
+> [!CAUTION]
+> **For NOT NULL slug columns with INSERT-time collision retry:** When a slug collision occurs and retry is needed, the entire `around_create` callback chain (including your `before_create` callbacks) re-executes. If you have non-idempotent callbacks like sending emails or enqueuing jobs, they may fire multiple times. Either make such callbacks idempotent, or move side-effects to `after_create` (which only fires once, after the record is persisted).
+
+> [!NOTE]
+> **Exhaustion behavior differs by slug strategy:** For nullable slug columns (post-create slug), if all retries are exhausted, `slugifiable` falls back to a timestamp-based slug. For NOT NULL slug columns (pre-insert slug), exhaustion raises an error instead, as sustained collisions in this case indicate a systemic issue that warrants attention.
 
 If you're generating slugs based off the model `id`, you can also set a desired length:
 ```ruby
@@ -214,6 +221,15 @@ bundle exec rake test
 
 The test suite uses SQLite3 in-memory database and requires no additional setup.
 
+Optional PostgreSQL integration check:
+
+```bash
+SLUGIFIABLE_TEST_POSTGRES_URL=postgres://user:pass@localhost:5432/slugifiable_test \
+bundle exec ruby -Itest test/slugifiable/postgresql_insert_race_retry_test.rb
+```
+
+This test validates PostgreSQL transaction behavior for retrying INSERT-time slug collisions.
+
 ## Development
 
 After checking out the repo, run `bin/setup` to install dependencies. Then, run `rake spec` to run the tests. You can also run `bin/console` for an interactive prompt that will allow you to experiment.

lib/slugifiable/model.rb

@@ -33,11 +33,13 @@ module Model
     # 10^18 fits safely in a 64-bit integer
     MAX_NUMBER_LENGTH = 18
 
-    # Maximum number of attempts to generate a unique slug
-    # before falling back to timestamp-based suffix
+    # Maximum number of attempts to generate a unique slug before exhaustion.
+    # Total saves in worst case for nullable-slug path: 10 attempts + 1 exhaustion fallback = 11.
+    # For NOT NULL path: 10 attempts then raises (no fallback).
     MAX_SLUG_GENERATION_ATTEMPTS = 10
 
     included do
+      around_create :retry_create_on_slug_unique_violation
       after_create :set_slug
       after_find :update_slug_if_nil
       validates :slug, uniqueness: true
@@ -106,7 +108,57 @@ def compute_slug_based_on_attribute(attribute_name)
       # 4. Ensure uniqueness
       # 5. Fallback to random number if anything fails
 
-      # First check if we can get a value from the database
+      base_slug = parameterize_attribute_value(attribute_name)
+
+      # Different fallback paths based on why parameterization failed
+      return compute_slug_as_string if base_slug == :attribute_missing
+      return generate_random_number_based_on_id_hex if base_slug == :value_nil_or_blank
+
+      # Handle duplicate slugs by adding a random suffix if needed
+      # e.g. "my-title" -> "my-title-123456"
+      unique_slug = generate_unique_slug(base_slug)
+      unique_slug.presence || generate_random_number_based_on_id_hex
+    end
+
+    private
+
+    # Returns the raw parameterized slug without uniqueness handling.
+    # Used by the exhaustion fallback to avoid double-suffixing.
+    #
+    # Always uses generate_random_number_based_on_id_hex for ALL fallback cases
+    # (missing attribute, nil value, blank value). This ensures the exhaustion
+    # fallback produces a simple numeric suffix regardless of failure reason.
+    def compute_base_slug
+      strategy, options = determine_slug_generation_method
+
+      case strategy
+      when :compute_slug_based_on_attribute
+        base_slug = parameterize_attribute_value(options)
+
+        # All fallback cases use numeric ID-based slug for exhaustion path
+        return generate_random_number_based_on_id_hex if base_slug.is_a?(Symbol)
+
+        base_slug
+      else
+        # For ID-based strategies, return the deterministic hash.
+        # NOTE: This path should never execute in practice since ID-based slug
+        # collisions are impossible (two records can't have the same ID). If it
+        # ever did, the exhaustion fallback would produce a double-suffixed slug
+        # like "d4735e3a265-1234567890-abcd1234".
+        compute_slug
+      end
+    end
+
+    # Shared helper: extracts and parameterizes an attribute value.
+    # Returns one of:
+    # - String: the parameterized value (e.g. "my-title")
+    # - :attribute_missing: attribute/method doesn't exist
+    # - :value_nil_or_blank: attribute exists but value is nil or parameterizes to blank
+    #
+    # Used by both compute_slug_based_on_attribute and compute_base_slug
+    # to ensure consistent behavior. Callers handle fallback logic.
+    def parameterize_attribute_value(attribute_name)
+      # Check if we can get a value from the database
       has_attribute = self.attributes.include?(attribute_name.to_s)
 
       # Only check for methods if no DB attribute exists
@@ -117,27 +169,18 @@ def compute_slug_based_on_attribute(attribute_name)
         self.class.protected_method_defined?(attribute_name)
       )
 
-      # If we can't get a value from either source, fallback to using the record's ID
-      return compute_slug_as_string unless has_attribute || responds_to_method
+      # If we can't get a value from either source, signal missing attribute
+      return :attribute_missing unless has_attribute || responds_to_method
 
       # Get and clean the raw value (e.g. "  My Title  " -> "My Title")
-      # Works for both DB attributes and methods thanks to Ruby's send
       raw_value = self.send(attribute_name)
-      return generate_random_number_based_on_id_hex if raw_value.nil?
+      return :value_nil_or_blank if raw_value.nil?
 
-      # Convert to URL-friendly format
-      # e.g. "My Title" -> "my-title"
+      # Convert to URL-friendly format (e.g. "My Title" -> "my-title")
       base_slug = raw_value.to_s.strip.parameterize
-      return generate_random_number_based_on_id_hex if base_slug.blank?
-
-      # Handle duplicate slugs by adding a random suffix if needed
-      # e.g. "my-title" -> "my-title-123456"
-      unique_slug = generate_unique_slug(base_slug)
-      unique_slug.presence || generate_random_number_based_on_id_hex
+      base_slug.presence || :value_nil_or_blank
     end
 
-    private
-
     def normalize_length(length, default, max)
       length = length.to_i
       return default if length <= 0
@@ -170,8 +213,8 @@ def generate_unique_slug(base_slug)
 
       # If we couldn't find a unique slug after MAX_SLUG_GENERATION_ATTEMPTS,
       # append timestamp + random to ensure uniqueness
-      if attempts == MAX_SLUG_GENERATION_ATTEMPTS
-        slug_candidate = "#{base_slug}-#{Time.current.to_i}-#{SecureRandom.random_number(1000)}"
+      if attempts >= MAX_SLUG_GENERATION_ATTEMPTS
+        slug_candidate = "#{base_slug}-#{Time.current.to_i}-#{SecureRandom.hex(4)}"
       end
 
       slug_candidate
@@ -221,8 +264,164 @@ def has_slug_method?
     def set_slug
       return unless slug_persisted?
 
-      self.slug = compute_slug if id_changed? || slug.blank?
-      self.save
+      set_slug_with_retry
+    end
+
+    def set_slug_with_retry
+      # NOTE: The old code checked `id_changed? || slug.blank?`. The `id_changed?`
+      # guard is unnecessary in `after_create` — the ID always just changed from nil.
+      # It would only matter if a trigger reassigned the ID post-INSERT, which is rare.
+      return unless slug.blank?
+
+      # For attribute-based slugs, compute_slug -> generate_unique_slug already
+      # handles uniqueness with random suffixes on each call.
+      # For ID-based slugs, collisions are impossible since two records can't
+      # have the same ID, so retries would never be triggered in practice.
+      #
+      # Each attempt runs in a savepoint so a unique-constraint violation does
+      # not abort the outer transaction in PostgreSQL.
+      #
+      # Fallback when all retries exhausted: use timestamp suffix for guaranteed uniqueness.
+      # Uses compute_base_slug to avoid double-suffixing (compute_slug includes random suffixes).
+      # NOTE: The exhaustion fallback uses save! and does not retry on failure. If this
+      # raises (unlikely given timestamp+random uniqueness), the exception propagates.
+      # This is intentional — exhaustion indicates a systemic issue warranting attention.
+      on_exhaustion = -> {
+        base_slug = compute_base_slug
+        self.slug = "#{base_slug}-#{Time.current.to_i}-#{SecureRandom.hex(4)}"
+        self.class.transaction(requires_new: true) { self.save! }
+      }
+
+      with_slug_retry(-> { self.slug = nil }, on_exhaustion: on_exhaustion) do
+        self.slug = compute_slug
+        # Use save! for consistency with exhaustion fallback — both paths
+        # now raise on any failure, not just RecordNotUnique.
+        self.class.transaction(requires_new: true) { self.save! }
+      end
+    end
+
+    # Detects if a RecordNotUnique error is related to the slug column.
+    #
+    # Uses patterns that handle common error message formats without false positives:
+    # - SQLite: "UNIQUE constraint failed: table.slug" -> matches \bslug\b (period is word boundary)
+    # - PostgreSQL: "Key (slug)=(value)" -> matches \bslug\b (parens are word boundaries)
+    # - MySQL/PG index: "index_posts_on_slug" -> matches _on_slug\b
+    #
+    # IMPORTANT: We use _on_slug\b specifically to avoid false positives on columns
+    # like canonical_slug, parent_slug, original_slug, etc. The pattern _slug alone
+    # would incorrectly match those.
+    #
+    # LIMITATION: Custom index names like `uq_slug_col`, `slug_idx`, or `posts_slug_unique`
+    # won't match. This results in a false negative (exception bubbles up), which is the
+    # safe outcome — no data corruption, just no automatic retry for that schema.
+    def slug_unique_violation?(error)
+      message = error.message.to_s.downcase
+      cause_message = error.cause&.message.to_s.downcase
+      # \bslug\b matches "slug" as standalone word (SQLite ".slug", PostgreSQL "(slug)")
+      # _on_slug\b matches Rails index naming convention "index_*_on_slug"
+      pattern = /\bslug\b|_on_slug\b/
+      [message, cause_message].any? { |m| m.match?(pattern) }
+    end
+
+    # Handle INSERT-time slug races for models that persist slugs at create-time
+    # (e.g., NOT NULL slug columns with before_validation slug generation).
+    #
+    # NOTE: This calls `yield` multiple times (once per attempt) via `retry`.
+    # This relies on Rails `around_create` yielding a re-invocable Proc, which
+    # is undocumented but has worked consistently in Rails 6-8. The behavior
+    # stems from Rails building callback chains as re-callable Procs/lambdas.
+    # See: activerecord/lib/active_record/callbacks.rb and
+    # activesupport/lib/active_support/callbacks.rb in Rails source.
+    # The test suite includes `around_create_state_machine_test.rb` which
+    # validates this behavior continues to work on Rails upgrades.
+    #
+    # Unlike set_slug_with_retry (which falls back to timestamp suffix on exhaustion),
+    # this method raises after MAX_SLUG_GENERATION_ATTEMPTS because INSERT-time
+    # collisions requiring 10+ retries indicate a systemic issue that warrants attention.
+    def retry_create_on_slug_unique_violation
+      return yield unless slug_persisted?
+      # Skip savepoint overhead for nullable slug columns — INSERT-time slug
+      # collisions are impossible when slug is NULL at INSERT time.
+      return yield unless slug_column_not_null?
+
+      # Each attempt runs in a savepoint so a unique-constraint violation does
+      # not abort the outer transaction in PostgreSQL.
+      # retry_if guard: Only retry if the record wasn't actually persisted.
+      # This handles the edge case where an after_create callback raises a
+      # slug-named RecordNotUnique on an already-persisted record — we don't
+      # want to retry the INSERT in that case.
+      #
+      # pre_retry_action: Recompute slug via compute_slug_for_retry before each
+      # retry. Although validation callbacks DO re-run on retry (before_validation
+      # fires again when yield is called), models using ensure_slug_for_insert
+      # typically guard with `slug.blank?`, so the slug isn't recomputed there.
+      # The pre_retry_action ensures a fresh slug is generated for each attempt.
+      with_slug_retry(
+        -> { self.slug = compute_slug_for_retry },
+        retry_if: ->(_error) { !persisted? }
+      ) do
+        self.class.transaction(requires_new: true) { yield }
+      end
+    end
+
+    # Check if the slug column is NOT NULL. This uses ActiveRecord's schema cache
+    # (columns_hash), which is already memoized. We don't add our own memoization
+    # to avoid staleness issues in development when schema changes.
+    def slug_column_not_null?
+      self.class.columns_hash["slug"]&.null == false
+    end
+
+    protected
+
+    # Generates a slug for retry attempts during INSERT-time race conditions.
+    #
+    # SUBCLASS OVERRIDE POINT: Override this method in subclasses to customize
+    # retry slug generation. For example, to use a different suffix strategy or
+    # incorporate additional uniqueness factors.
+    #
+    # Default behavior: delegates to compute_slug, which for attribute-based
+    # strategies calls generate_unique_slug (adds random suffixes on each call),
+    # ensuring retry attempts try different values.
+    #
+    # NOTE: ID-based slug collisions are impossible (two records can't share the same ID),
+    # so this method is only meaningful for attribute-based strategies where concurrent
+    # inserts can race to claim the same slug.
+    def compute_slug_for_retry
+      compute_slug
+    end
+
+    private
+
+    # Shared retry logic for slug unique constraint violations.
+    # Makes up to MAX_SLUG_GENERATION_ATTEMPTS total attempts (1 initial + N-1 retries),
+    # calling pre_retry_action before each retry to regenerate the slug.
+    #
+    # When retries are exhausted, calls the optional on_exhaustion proc if provided,
+    # otherwise re-raises the exception. The on_exhaustion proc can apply a timestamp
+    # fallback to maintain parity with generate_unique_slug's lenient behavior.
+    #
+    # KNOWN LIMITATION: This catches RecordNotUnique (DB constraint violation) but not
+    # RecordInvalid (AR validation failure). If another process steals the slug between
+    # the AR uniqueness validation SELECT and the actual UPDATE, save! raises RecordInvalid
+    # instead of RecordNotUnique. This is a narrow window and the DB constraint is the
+    # ultimate safeguard, but be aware that RecordInvalid from a race is not retried.
+    def with_slug_retry(pre_retry_action, retry_if: ->(_error) { true }, on_exhaustion: nil)
+      attempts = 0
+
+      begin
+        yield
+      rescue ActiveRecord::RecordNotUnique => e
+        raise unless slug_unique_violation?(e)
+        raise unless retry_if.call(e)
+
+        attempts += 1
+        if attempts >= MAX_SLUG_GENERATION_ATTEMPTS
+          on_exhaustion ? on_exhaustion.call : raise
+        else
+          pre_retry_action.call
+          retry
+        end
+      end
     end
 
     def update_slug_if_nil