From 42186b8d8a2bfeac66e4c1ede46786253165cb86 Mon Sep 17 00:00:00 2001 From: Evan Huus Date: Thu, 22 Jan 2026 15:56:01 -0500 Subject: [PATCH 1/3] Add advisory lock timeout configuration with error on failure - Add advisory_lock_timeout_seconds option (default: 15 seconds) - Raise error when advisory lock cannot be acquired within timeout - This provides explicit failure behavior for lock contention issues Co-Authored-By: Claude Opus 4.5 --- README.md | 27 +++++++++++++++++++++++++- lib/closure_tree/support.rb | 5 +++-- lib/closure_tree/support_attributes.rb | 4 ++++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 45dd33f3..1200c1c7 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,29 @@ -# Closure Tree +# Closure Tree (OpsLevel Fork) + +This is the OpsLevel fork of [closure_tree](https://github.com/ClosureTree/closure_tree), based on upstream v9.5.0. + +## Fork-Specific Changes + +This fork adds one enhancement to the advisory lock behavior: + +### Advisory Lock Timeout with Error on Failure + +- Adds `advisory_lock_timeout_seconds` option (default: 15 seconds) +- Raises an error when the advisory lock cannot be acquired within the timeout period + +The upstream version has no timeout and hangs forever if lock acquisition fails. This fork uses `with_advisory_lock!` (with bang) which raises `WithAdvisoryLock::FailedToAcquireLock` when the lock cannot be obtained, providing explicit failure behavior for lock contention issues. + +```ruby +class Tag < ApplicationRecord + # Use default 15 second timeout + has_closure_tree + + # Or customize the timeout + has_closure_tree advisory_lock_timeout_seconds: 30 +end +``` + +--- ### Closure_tree lets your ActiveRecord models act as nodes in a [tree data structure](http://en.wikipedia.org/wiki/Tree_%28data_structure%29) diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index b27b7c6f..bb19aade 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -19,6 +19,7 @@ def initialize(model_class, options) dependent: :nullify, # or :destroy, :delete_all, or :adopt -- see the README name_column: 'name', with_advisory_lock: true, # This will be overridden by adapter support + advisory_lock_timeout_seconds: 15, numeric_order: false }.merge(options) raise ArgumentError, "name_column can't be 'path'" if options[:name_column] == 'path' @@ -153,8 +154,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) - model_class.with_advisory_lock(advisory_lock_name) do + if options[:with_advisory_lock] && connection.supports_advisory_locks? && model_class.respond_to?(:with_advisory_lock!) + model_class.with_advisory_lock!(advisory_lock_name, advisory_lock_options) do transaction(&block) end else diff --git a/lib/closure_tree/support_attributes.rb b/lib/closure_tree/support_attributes.rb index fd304937..a40ed573 100644 --- a/lib/closure_tree/support_attributes.rb +++ b/lib/closure_tree/support_attributes.rb @@ -34,6 +34,10 @@ def advisory_lock_name end end + def advisory_lock_options + { timeout_seconds: options[:advisory_lock_timeout_seconds] }.compact + end + def quoted_table_name connection.quote_table_name(table_name) end From 547b422628d9ee498babdc26d92c863bccab86ab Mon Sep 17 00:00:00 2001 From: Farjaad Rawasia Date: Wed, 28 Jan 2026 13:39:04 -0500 Subject: [PATCH 2/3] feat: raise error when advisory lock cannot be acquired within configured timeout --- README.md | 28 ++------------------ lib/closure_tree/has_closure_tree.rb | 1 + lib/closure_tree/support.rb | 10 +++++-- test/closure_tree/parallel_test.rb | 2 +- test/closure_tree/support_test.rb | 39 +++++++++++++++++++++++++++- 5 files changed, 50 insertions(+), 30 deletions(-) diff --git a/README.md b/README.md index 1200c1c7..ec04f658 100644 --- a/README.md +++ b/README.md @@ -1,29 +1,4 @@ -# Closure Tree (OpsLevel Fork) - -This is the OpsLevel fork of [closure_tree](https://github.com/ClosureTree/closure_tree), based on upstream v9.5.0. - -## Fork-Specific Changes - -This fork adds one enhancement to the advisory lock behavior: - -### Advisory Lock Timeout with Error on Failure - -- Adds `advisory_lock_timeout_seconds` option (default: 15 seconds) -- Raises an error when the advisory lock cannot be acquired within the timeout period - -The upstream version has no timeout and hangs forever if lock acquisition fails. This fork uses `with_advisory_lock!` (with bang) which raises `WithAdvisoryLock::FailedToAcquireLock` when the lock cannot be obtained, providing explicit failure behavior for lock contention issues. - -```ruby -class Tag < ApplicationRecord - # Use default 15 second timeout - has_closure_tree - - # Or customize the timeout - has_closure_tree advisory_lock_timeout_seconds: 30 -end -``` - ---- +# Closure Tree ### Closure_tree lets your ActiveRecord models act as nodes in a [tree data structure](http://en.wikipedia.org/wiki/Tree_%28data_structure%29) @@ -347,6 +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 ## Accessing Data diff --git a/lib/closure_tree/has_closure_tree.rb b/lib/closure_tree/has_closure_tree.rb index 24dc1381..8c2ed61d 100644 --- a/lib/closure_tree/has_closure_tree.rb +++ b/lib/closure_tree/has_closure_tree.rb @@ -15,6 +15,7 @@ def has_closure_tree(options = {}) :touch, :with_advisory_lock, :advisory_lock_name, + :advisory_lock_timeout_seconds, :scope ) diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index bb19aade..6e9f7968 100644 --- a/lib/closure_tree/support.rb +++ b/lib/closure_tree/support.rb @@ -19,7 +19,7 @@ def initialize(model_class, options) dependent: :nullify, # or :destroy, :delete_all, or :adopt -- see the README name_column: 'name', with_advisory_lock: true, # This will be overridden by adapter support - advisory_lock_timeout_seconds: 15, + advisory_lock_timeout_seconds: nil, numeric_order: false }.merge(options) raise ArgumentError, "name_column can't be 'path'" if options[:name_column] == 'path' @@ -31,6 +31,10 @@ def initialize(model_class, options) end end + if !options[:with_advisory_lock] && options[:advisory_lock_timeout_seconds].present? + raise ArgumentError, "advisory_lock_timeout_seconds can't be specified when advisory_lock is disabled" + end + return unless order_is_numeric? extend NumericOrderSupport.adapter_for_connection(connection) @@ -155,7 +159,9 @@ def build_scope_where_clause(scope_conditions) def with_advisory_lock(&block) if options[:with_advisory_lock] && connection.supports_advisory_locks? && model_class.respond_to?(:with_advisory_lock!) - model_class.with_advisory_lock!(advisory_lock_name, advisory_lock_options) do + lock_method = options[:advisory_lock_timeout_seconds].present? ? :with_advisory_lock! : :with_advisory_lock + + model_class.public_send(lock_method, advisory_lock_name, advisory_lock_options) do transaction(&block) end else diff --git a/test/closure_tree/parallel_test.rb b/test/closure_tree/parallel_test.rb index 6dc5cf99..dc528f46 100644 --- a/test/closure_tree/parallel_test.rb +++ b/test/closure_tree/parallel_test.rb @@ -137,7 +137,7 @@ def run_workers(worker_class = FindOrCreateWorker) skip('unsupported') unless run_parallel_tests? # disable with_advisory_lock: - Tag.stub(:with_advisory_lock, ->(_lock_name, &block) { block.call }) do + Tag.stub(:with_advisory_lock, ->(_lock_name, _lock_options, &block) { block.call }) do run_workers # duplication from at least one iteration: assert Tag.where(name: @names).size > @iterations diff --git a/test/closure_tree/support_test.rb b/test/closure_tree/support_test.rb index 3770d954..d4ffdb2e 100644 --- a/test/closure_tree/support_test.rb +++ b/test/closure_tree/support_test.rb @@ -3,7 +3,8 @@ require 'test_helper' describe ClosureTree::Support do - let(:sut) { Tag._ct } + let(:model) { Tag } + let(:sut) { model._ct } it 'passes through table names without prefix and suffix' do expected = 'some_random_table_name' @@ -15,4 +16,40 @@ tn = ActiveRecord::Base.table_name_prefix + expected + ActiveRecord::Base.table_name_suffix assert_equal expected, sut.remove_prefix_and_suffix(tn) end + + it 'initializes without error when with_advisory_lock is false' do + assert ClosureTree::Support.new(model, { with_advisory_lock: false }) + end + + it 'initializes without error when with_advisory_lock is true and advisory_lock_timeout_seconds is set' do + assert ClosureTree::Support.new(model, { with_advisory_lock: true, advisory_lock_timeout_seconds: 10 }) + 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) + called = false + sut.stub(:options, options) do + model.stub(:with_advisory_lock!, ->(_lock_name, _options, &block) { block.call }) do + sut.with_advisory_lock { called = true } + end + end + assert called, 'block should have been called' + end + + it 'calls :with_advisory_lock when with_advisory_lock is true and timeout is nil' do + called = false + model.stub(:with_advisory_lock, ->(_lock_name, _options, &block) { block.call }) do + sut.with_advisory_lock { called = true } + end + assert called, 'block should have been called' + end + + it 'does not call advisory lock methods when with_advisory_lock is false' do + options = sut.options.merge(with_advisory_lock: false, advisory_lock_timeout_seconds: nil) + called = false + sut.stub(:options, options) do + sut.with_advisory_lock { called = true } + end + assert called, 'block should have been called' + end end From 91c90afbf03c8088b56f74bcca2b409968a428d2 Mon Sep 17 00:00:00 2001 From: Farjaad Rawasia Date: Sat, 31 Jan 2026 12:15:46 -0500 Subject: [PATCH 3/3] address comments --- README.md | 2 +- lib/closure_tree/support.rb | 5 ++-- test/closure_tree/support_test.rb | 50 +++++++++++++++++++++++++++++-- 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index ec04f658..e4017b5b 100644 --- a/README.md +++ b/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 diff --git a/lib/closure_tree/support.rb b/lib/closure_tree/support.rb index 6e9f7968..04f73ac6 100644 --- a/lib/closure_tree/support.rb +++ b/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 diff --git a/test/closure_tree/support_test.rb b/test/closure_tree/support_test.rb index d4ffdb2e..9e353e63 100644 --- a/test/closure_tree/support_test.rb +++ b/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