Fix base-only rescue handlers never firing (#2794)

* Fix base-only rescue handlers never firing

`rescue_from Klass, rescue_subclasses: false` registered its handler in
`namespace_reverse_stackable[:base_only_rescue_handlers]` (dsl/request_response.rb),
but the error middleware options read `:base_only_rescue_handlers` from
`namespace_stackable` via `namespace_stackable_with_hash`. Those are two
different stores, so the read was always empty and base-only handlers never
ran — the exception propagated uncaught.

Read base-only handlers from the same reverse-stackable store the write uses,
sharing the child-precedence merge with `rescue_handlers` via
`merged_reverse_stackable`. This also gives base-only handlers the same
nested-scope precedence that subclass handlers already had.

The gap slipped through because the only behavioral test asserted a *child*
error is not rescued (which passes even when base-only is fully broken);
nothing asserted the exact class is rescued. Add that missing test.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* Encapsulate namespace_reverse_stackable behind rescue-handler methods

namespace_reverse_stackable existed only to store rescue-handler maps, yet it
was a public attr_reader that callers reached into with raw keys from two
different files (the rescue_from write and the endpoint read). That's how the
base-only handlers write/read drifted onto mismatched stores.

Make the store internal (protected, still used by inherit_from/copy_state_from/
to_hash) and expose intent-revealing methods on InheritableSetting instead:
rescue_handlers / base_only_rescue_handlers (merged, child-scope-first) and
add_rescue_handlers(mapping, subclasses:). The DSL write and endpoint reads go
through those, so there's one place that knows the key, store and merge.

The reverse-ordering behaviour stays covered by ReverseStackableValues' own
spec; the InheritableSetting specs that poked the raw store are replaced with
tests for the new methods.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Daniel (dB.) Doubrovkine <dblock@dblock.org>

Author: 3 people

CHANGELOG.md

@@ -12,6 +12,7 @@
 * [#2782](https://github.com/ruby-grape/grape/pull/2782): Remove the dead `format` keyword from `Grape::Router::Pattern` - [@ericproulx](https://github.com/ericproulx).
 * [#2783](https://github.com/ruby-grape/grape/pull/2783): Read a route's `version`, `anchor` and `requirements` from its pattern instead of storing them again on the route - [@ericproulx](https://github.com/ericproulx).
 * [#2784](https://github.com/ruby-grape/grape/pull/2784): Move HEAD route creation into `Grape::Router::Route#to_head` - [@ericproulx](https://github.com/ericproulx).
+* [#2795](https://github.com/ruby-grape/grape/pull/2795): Make `Grape::Util::InheritableSetting#namespace_reverse_stackable` internal, exposing `rescue_handlers` / `base_only_rescue_handlers` / `add_rescue_handlers` instead - [@ericproulx](https://github.com/ericproulx).
 * [#2793](https://github.com/ruby-grape/grape/pull/2793): Add a `Grape::Mountable` marker to identify a Grape app instead of duck-typing on `respond_to?(:inheritable_setting)` - [@ericproulx](https://github.com/ericproulx).
 * [#2792](https://github.com/ruby-grape/grape/pull/2792): Move `Grape::Path` to `Grape::Router::Pattern::Path` and add a `Grape::Router::Pattern.build` factory (`Grape::Path` kept as a deprecated constant) - [@ericproulx](https://github.com/ericproulx).
 * [#2785](https://github.com/ruby-grape/grape/pull/2785): Make route `params` a first-class endpoint input and split `Grape::Router::Route#params` into `#params` (declared definitions) and `#params_for(input)` (extracted values) - [@ericproulx](https://github.com/ericproulx).
@@ -21,6 +22,7 @@
 #### Fixes
 
 * [#2767](https://github.com/ruby-grape/grape/pull/2767): Update rubocop to 1.88.0 and rubocop-rspec to 3.10.2 - [@ericproulx](https://github.com/ericproulx).
+* [#2794](https://github.com/ruby-grape/grape/pull/2794): Fix `rescue_from Klass, rescue_subclasses: false` never rescuing — base-only handlers were written to and read from different settings stores - [@ericproulx](https://github.com/ericproulx).
 * [#2789](https://github.com/ruby-grape/grape/pull/2789): Escape error messages served with a parameterized `text/html` content-type (e.g. `text/html; charset=utf-8`) and warn when a bare Rack app is mounted under authentication - [@ericproulx](https://github.com/ericproulx).
 * [#2790](https://github.com/ruby-grape/grape/pull/2790): Stop mutating `Grape::Router`'s internal maps at request time for HTTP methods with no routes (data race on JRuby/TruffleRuby and unbounded growth) - [@ericproulx](https://github.com/ericproulx).
 * [#2791](https://github.com/ruby-grape/grape/pull/2791): Build HEAD/OPTIONS/405 helper routes from a copy of the inheritable settings instead of temporarily mutating the shared class-level settings during compilation - [@ericproulx](https://github.com/ericproulx).

lib/grape/dsl/request_response.rb

@@ -120,8 +120,7 @@ def rescue_from(*args, with: nil, rescue_subclasses: true, backtrace: false, ori
         when :internal_grape_exceptions
           namespace_inheritable[:internal_grape_exceptions_rescue_handler] = handler
         else
-          handler_type = rescue_subclasses ? :rescue_handlers : :base_only_rescue_handlers
-          inheritable_setting.namespace_reverse_stackable[handler_type] = args.to_h { |klass| [klass, handler] }
+          inheritable_setting.add_rescue_handlers(args.to_h { |klass| [klass, handler] }, subclasses: rescue_subclasses)
         end
 
         inheritable_setting.namespace_stackable[:rescue_options] = RescueOptions.new(backtrace:, original_exception:)

lib/grape/endpoint.rb

@@ -384,8 +384,8 @@ def error_middleware_options(format, content_types)
         default_error_formatter: ns_inh[:default_error_formatter],
         error_formatters: ns_stack.namespace_stackable_with_hash(:error_formatters),
         rescue_options: ns_stack.namespace_stackable[:rescue_options]&.last,
-        rescue_handlers:,
-        base_only_rescue_handlers: ns_stack.namespace_stackable_with_hash(:base_only_rescue_handlers),
+        rescue_handlers: ns_stack.rescue_handlers,
+        base_only_rescue_handlers: ns_stack.base_only_rescue_handlers,
         all_rescue_handler: ns_inh[:all_rescue_handler],
         grape_exceptions_rescue_handler: ns_inh[:grape_exceptions_rescue_handler],
         internal_grape_exceptions_rescue_handler: ns_inh[:internal_grape_exceptions_rescue_handler]
@@ -424,14 +424,5 @@ def build_response_cookies
     def lint?
       inheritable_setting.namespace_inheritable[:lint] || Grape.config.lint
     end
-
-    def rescue_handlers
-      rescue_handlers = inheritable_setting.namespace_reverse_stackable[:rescue_handlers]
-      return if rescue_handlers.blank?
-
-      rescue_handlers.each_with_object({}) do |rescue_handler, result|
-        result.merge!(rescue_handler) { |_k, s1, _s2| s1 }
-      end
-    end
   end
 end

lib/grape/util/inheritable_setting.rb

@@ -5,7 +5,7 @@ module Util
     # A branchable, inheritable settings object which can store both stackable
     # and inheritable values (see InheritableValues and StackableValues).
     class InheritableSetting
-      attr_reader :route, :namespace, :namespace_inheritable, :namespace_stackable, :namespace_reverse_stackable, :parent
+      attr_reader :route, :namespace, :namespace_inheritable, :namespace_stackable, :parent
 
       # Lazy-allocated; +api_class+ and +point_in_time_copies+ are rarely
       # written on most settings layers, so don't pay for a Hash/Array each.
@@ -107,8 +107,28 @@ def namespace_stackable_with_hash(key)
         data.each_with_object({}) { |value, result| result.deep_merge!(value) }
       end
 
+      # Rescue-handler maps registered by +rescue_from+, keyed by exception
+      # class and merged so a nested scope's handler wins. Record them with
+      # #add_rescue_handlers; the backing store is an internal detail.
+      def rescue_handlers
+        merged_rescue_handlers(:rescue_handlers)
+      end
+
+      def base_only_rescue_handlers
+        merged_rescue_handlers(:base_only_rescue_handlers)
+      end
+
+      def add_rescue_handlers(mapping, subclasses:)
+        namespace_reverse_stackable[subclasses ? :rescue_handlers : :base_only_rescue_handlers] = mapping
+      end
+
       protected
 
+      # Reverse-stackable so a child scope's rescue handlers precede inherited
+      # ones. Reached only through #rescue_handlers / #base_only_rescue_handlers
+      # / #add_rescue_handlers, and #inherit_from / #copy_state_from.
+      attr_reader :namespace_reverse_stackable
+
       # Used by +point_in_time_copy+ to populate a freshly-built instance
       # with cloned state from another instance of the same class.
       def copy_state_from(source)
@@ -119,6 +139,15 @@ def copy_state_from(source)
         @route = source.route.clone
         @api_class = source.api_class
       end
+
+      private
+
+      def merged_rescue_handlers(key)
+        handlers = namespace_reverse_stackable[key]
+        return if handlers.blank?
+
+        handlers.each_with_object({}) { |handler, result| result.merge!(handler) { |_k, s1, _s2| s1 } }
+      end
     end
   end
 end

spec/grape/api_spec.rb

@@ -2438,6 +2438,19 @@ def rescue_all_errors
       expect(last_response).to be_server_error
     end
 
+    it 'rescues the exact error class if rescue_subclasses is false' do
+      subject.rescue_from ApiSpec::APIErrors::ParentError, rescue_subclasses: false do |e|
+        error!("rescued from #{e.class.name}", 500)
+      end
+      subject.get '/caught_parent' do
+        raise ApiSpec::APIErrors::ParentError
+      end
+
+      get '/caught_parent'
+      expect(last_response).to be_server_error
+      expect(last_response.body).to eq('rescued from ApiSpec::APIErrors::ParentError')
+    end
+
     it 'does not rescue child errors if rescue_subclasses is false' do
       subject.rescue_from ApiSpec::APIErrors::ParentError, rescue_subclasses: false do |e|
         error!("rescued from #{e.class.name}", 500)

spec/grape/dsl/request_response_spec.rb

@@ -220,34 +220,34 @@
 
       it 'sets hash of exceptions as rescue handlers' do
         subject.rescue_from StandardError
-        expect(subject.inheritable_setting.namespace_reverse_stackable[:rescue_handlers]).to eq([{ StandardError => nil }])
+        expect(subject.inheritable_setting.rescue_handlers).to eq(StandardError => nil)
         expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq(default_rescue_options)
       end
 
       it 'rescues only base handlers if rescue_subclasses: false option is passed' do
         subject.rescue_from StandardError, rescue_subclasses: false
-        expect(subject.inheritable_setting.namespace_reverse_stackable[:base_only_rescue_handlers]).to eq([{ StandardError => nil }])
+        expect(subject.inheritable_setting.base_only_rescue_handlers).to eq(StandardError => nil)
         expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq(default_rescue_options)
       end
 
       it 'sets given proc as rescue handler for each key in hash' do
         rescue_handler_proc = proc {}
         subject.rescue_from StandardError, rescue_handler_proc
-        expect(subject.inheritable_setting.namespace_reverse_stackable[:rescue_handlers]).to eq([{ StandardError => rescue_handler_proc }])
+        expect(subject.inheritable_setting.rescue_handlers).to eq(StandardError => rescue_handler_proc)
         expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq(default_rescue_options)
       end
 
       it 'sets given block as rescue handler for each key in hash' do
         rescue_handler_proc = proc {}
         subject.rescue_from StandardError, &rescue_handler_proc
-        expect(subject.inheritable_setting.namespace_reverse_stackable[:rescue_handlers]).to eq([{ StandardError => rescue_handler_proc }])
+        expect(subject.inheritable_setting.rescue_handlers).to eq(StandardError => rescue_handler_proc)
         expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq(default_rescue_options)
       end
 
       it 'sets a rescue handler declared through :with option for each key in hash' do
         with_block = -> { 'hello' }
         subject.rescue_from StandardError, with: with_block
-        expect(subject.inheritable_setting.namespace_reverse_stackable[:rescue_handlers]).to eq([{ StandardError => with_block }])
+        expect(subject.inheritable_setting.rescue_handlers).to eq(StandardError => with_block)
         expect(subject.inheritable_setting.namespace_stackable[:rescue_options]).to eq(default_rescue_options)
       end
     end

spec/grape/util/inheritable_setting_spec.rb

@@ -12,7 +12,6 @@
       settings.namespace[:namespace_thing] = :namespace_foo_bar
       settings.namespace_inheritable[:namespace_inheritable_thing] = :namespace_inheritable_foo_bar
       settings.namespace_stackable[:namespace_stackable_thing] = :namespace_stackable_foo_bar
-      settings.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = :namespace_reverse_stackable_foo_bar
       settings.route[:route_thing] = :route_foo_bar
     end
   end
@@ -22,7 +21,6 @@
       settings.namespace[:namespace_thing] = :namespace_foo_bar_other
       settings.namespace_inheritable[:namespace_inheritable_thing] = :namespace_inheritable_foo_bar_other
       settings.namespace_stackable[:namespace_stackable_thing] = :namespace_stackable_foo_bar_other
-      settings.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = :namespace_reverse_stackable_foo_bar_other
       settings.route[:route_thing] = :route_foo_bar_other
     end
   end
@@ -116,13 +114,24 @@
     end
   end
 
-  describe '#namespace_reverse_stackable' do
-    it 'works with reverse stackable values' do
-      expect(subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar]
+  describe '#rescue_handlers / #add_rescue_handlers' do
+    it 'records subclass-matching handlers under rescue_handlers' do
+      subject.add_rescue_handlers({ StandardError => :handler }, subclasses: true)
+      expect(subject.rescue_handlers).to eq(StandardError => :handler)
+      expect(subject.base_only_rescue_handlers).to be_nil
+    end
 
-      subject.inherit_from other_parent
+    it 'records exact-match handlers under base_only_rescue_handlers' do
+      subject.add_rescue_handlers({ StandardError => :handler }, subclasses: false)
+      expect(subject.base_only_rescue_handlers).to eq(StandardError => :handler)
+      expect(subject.rescue_handlers).to be_nil
+    end
 
-      expect(subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar_other]
+    it 'lets a nested scope override an inherited handler for the same class' do
+      parent = described_class.new.tap { |s| s.add_rescue_handlers({ StandardError => :parent }, subclasses: true) }
+      subject.inherit_from parent
+      subject.add_rescue_handlers({ StandardError => :child }, subclasses: true)
+      expect(subject.rescue_handlers).to eq(StandardError => :child)
     end
   end
 
@@ -192,14 +201,6 @@
       expect(cloned_obj.namespace_stackable[:namespace_stackable_thing]).to eq [:namespace_stackable_foo_bar]
     end
 
-    it 'decouples namespace reverse stackable values' do
-      expect(cloned_obj.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar]
-
-      subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = :other_thing
-      expect(subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq %i[other_thing namespace_reverse_stackable_foo_bar]
-      expect(cloned_obj.namespace_reverse_stackable[:namespace_reverse_stackable_thing]).to eq [:namespace_reverse_stackable_foo_bar]
-    end
-
     it 'decouples route values' do
       expect(cloned_obj.route[:route_thing]).to eq :route_foo_bar
 
@@ -218,7 +219,7 @@
       subject.namespace[:namespace_thing] = :namespace_foo_bar
       subject.namespace_inheritable[:namespace_inheritable_thing] = :namespace_inheritable_foo_bar
       subject.namespace_stackable[:namespace_stackable_thing] = [:namespace_stackable_foo_bar]
-      subject.namespace_reverse_stackable[:namespace_reverse_stackable_thing] = [:namespace_reverse_stackable_foo_bar]
+      subject.add_rescue_handlers({ StandardError => :handler }, subclasses: true)
       subject.route[:route_thing] = :route_foo_bar
       expect(subject.to_hash).to match(
         global: { global_thing: :global_foo_bar },
@@ -227,8 +228,7 @@
           namespace_inheritable_thing: :namespace_inheritable_foo_bar
         },
         namespace_stackable: { namespace_stackable_thing: [:namespace_stackable_foo_bar, [:namespace_stackable_foo_bar]] },
-        namespace_reverse_stackable:
-          { namespace_reverse_stackable_thing: [[:namespace_reverse_stackable_foo_bar], :namespace_reverse_stackable_foo_bar] },
+        namespace_reverse_stackable: { rescue_handlers: [{ StandardError => :handler }] },
         route: { route_thing: :route_foo_bar }
       )
     end