address comments

Author: farjaad-opslevel

README.md

@@ -322,7 +322,7 @@ When you include ```has_closure_tree``` in your model, you can provide a hash to
 * ```:order``` used to set up [deterministic ordering](#deterministic-ordering)
 * ```:scope``` restricts root nodes and sibling ordering to specific columns. Can be a single symbol or an array of symbols. Example: ```scope: :user_id``` or ```scope: [:user_id, :group_id]```. This ensures that root nodes and siblings are scoped correctly when reordering. See [Ordering Roots](#ordering-roots) for more details.
 * ```:touch``` delegates to the `belongs_to` annotation for the parent, so `touch`ing cascades to all children (the performance of this for deep trees isn't currently optimal).
-* ```:advisory_lock_timeout_seconds``` Raises an error when the advisory lock cannot be acquired within the timeout period
+* ```:advisory_lock_timeout_seconds``` When set, the advisory lock will raise `WithAdvisoryLock::FailedToAcquireLock` if the lock cannot be acquired within the timeout period. This helps callers handle timeout scenarios (e.g. retry or fail fast).
 
 ## Accessing Data
 

lib/closure_tree/support.rb

@@ -158,9 +158,8 @@ def build_scope_where_clause(scope_conditions)
     end
 
     def with_advisory_lock(&block)
-      if options[:with_advisory_lock] && connection.supports_advisory_locks? && model_class.respond_to?(:with_advisory_lock!)
-        lock_method = options[:advisory_lock_timeout_seconds].present? ? :with_advisory_lock! : :with_advisory_lock
-
+      lock_method = options[:advisory_lock_timeout_seconds].present? ? :with_advisory_lock! : :with_advisory_lock
+      if options[:with_advisory_lock] && connection.supports_advisory_locks? && model_class.respond_to?(lock_method)
         model_class.public_send(lock_method, advisory_lock_name, advisory_lock_options) do
           transaction(&block)
         end

test/closure_tree/support_test.rb

@@ -27,21 +27,34 @@
 
   it 'calls :with_advisory_lock! when with_advisory_lock is true and timeout is 10' do
     options = sut.options.merge(with_advisory_lock: true, advisory_lock_timeout_seconds: 10)
+    received_lock_name = nil
+    received_options = nil
     called = false
     sut.stub(:options, options) do
-      model.stub(:with_advisory_lock!, ->(_lock_name, _options, &block) { block.call }) do
+      model.stub(:with_advisory_lock!, ->(lock_name, opts, &block) {
+        received_lock_name = lock_name
+        received_options = opts
+        block.call
+      }) do
         sut.with_advisory_lock { called = true }
       end
     end
     assert called, 'block should have been called'
+    assert_equal sut.advisory_lock_name, received_lock_name, 'lock name should be passed to with_advisory_lock!'
+    assert_equal({ timeout_seconds: 10 }, received_options, 'options should include timeout_seconds when timeout is configured')
   end
 
   it 'calls :with_advisory_lock when with_advisory_lock is true and timeout is nil' do
+    received_options = nil
     called = false
-    model.stub(:with_advisory_lock, ->(_lock_name, _options, &block) { block.call }) do
+    model.stub(:with_advisory_lock, ->(_lock_name, opts, &block) {
+      received_options = opts
+      block.call
+    }) do
       sut.with_advisory_lock { called = true }
     end
     assert called, 'block should have been called'
+    assert_equal({}, received_options, 'options should be empty when timeout is not configured')
   end
 
   it 'does not call advisory lock methods when with_advisory_lock is false' do
@@ -52,4 +65,37 @@
     end
     assert called, 'block should have been called'
   end
+
+  it 'raises ArgumentError when advisory_lock_timeout_seconds is set but with_advisory_lock is false' do
+    error = assert_raises(ArgumentError) do
+      ClosureTree::Support.new(model, with_advisory_lock: false, advisory_lock_timeout_seconds: 10)
+    end
+    assert_match(/advisory_lock_timeout_seconds can't be specified when advisory_lock is disabled/, error.message)
+  end
+
+  it 'raises WithAdvisoryLock::FailedToAcquireLock when lock cannot be acquired within timeout' do
+    lock_held = false
+    holder_thread = Thread.new do
+      model.connection_pool.with_connection do
+        model.with_advisory_lock(sut.advisory_lock_name) do
+          lock_held = true
+          sleep 2
+        end
+      end
+    end
+
+    # Wait for holder to acquire the lock
+    sleep 0.2 until lock_held
+
+    support_with_timeout = ClosureTree::Support.new(
+      model,
+      sut.options.merge(with_advisory_lock: true, advisory_lock_timeout_seconds: 1)
+    )
+
+    assert_raises(WithAdvisoryLock::FailedToAcquireLock) do
+      support_with_timeout.with_advisory_lock { nil }
+    end
+
+    holder_thread.join
+  end
 end