Fix base-only rescue handlers never firing#2794
Merged
Merged
Conversation
`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>
ericproulx
force-pushed
the
fix/base-only-rescue-handlers
branch
from
7dd1b39 to
60dcb27
Compare
Danger ReportNo issues found. |
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>
dblock
approved these changes
Jul 12, 2026
…-stackable Encapsulate namespace_reverse_stackable behind rescue-handler methods
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Bug
rescue_from SomeError, rescue_subclasses: falsenever rescues anything — the exception propagates uncaught:Cause
The write and read use different settings stores:
dsl/request_response.rb):namespace_reverse_stackable[:base_only_rescue_handlers] = …endpoint.rb#error_middleware_options):namespace_stackable_with_hash(:base_only_rescue_handlers)— readsnamespace_stackablenamespace_stackable[:base_only_rescue_handlers]is never written, so the middleware'sbase_only_rescue_handlersis always empty and theklass == errbranch inregistered_rescue_handlerfinds nothing.:rescue_handlers(therescue_subclasses: truedefault) is unaffected — it's both written and read vianamespace_reverse_stackable.Likely introduced by
60c1a99e, which moved the base-only read tonamespace_stackable_with_hashwhile the write stayed onnamespace_reverse_stackable.Fix
Read base-only handlers from the same reverse-stackable store the write uses, sharing the child-precedence merge with
rescue_handlers(merged_reverse_stackable). Base-only handlers now also get the nested-scope precedence subclass handlers already had.Why it slipped through
The only behavioral test asserted a child error is not rescued — which passes even when base-only is 100% broken (a broken handler catches nothing). Nothing asserted the exact class is rescued. Added that test; verified it fails on the old code and passes on the fix.
Full suite: 2352 examples, 0 failures. RuboCop clean.
🤖 Generated with Claude Code