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). If the option is not specified, the lock is waited for indefinitely until it is acquired. See [Lock wait timeouts](https://github.com/ClosureTree/with_advisory_lock?tab=readme-ov-file#lock-wait-timeouts) in the with_advisory_lock gem for details.
 
 ## 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

@@ -25,23 +25,43 @@
     assert ClosureTree::Support.new(model, { with_advisory_lock: true, advisory_lock_timeout_seconds: 10 })
   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 '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 +72,30 @@
     end
     assert called, 'block should have been called'
   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