From 42186b8d8a2bfeac66e4c1ede46786253165cb86 Mon Sep 17 00:00:00 2001 From: Evan Huus Date: Thu, 22 Jan 2026 15:56:01 -0500 Subject: [PATCH 1/2] 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/2] 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