Skip to content

Handle new execute_intent non-raising behaviour - #1048

Merged
andrewn617 merged 4 commits into
mainfrom
execute-intent-without-raising
Sep 4, 2026
Merged

Handle new execute_intent non-raising behaviour#1048
andrewn617 merged 4 commits into
mainfrom
execute-intent-without-raising

Conversation

@andrewn617

Copy link
Copy Markdown
Member

rails/rails#58049 refactored execute_intent to no longer raise errors. Instead execution errors are stored on the query intent and can be checked later by the caller. This is to pave the way for better async execution.

However, semian relies on the raising behaviour to trip the circuit breaker. With this change to Rails, any query that fails mid execution will be considered successful by Semian. And indeed ActiveRecordTrilogyAdapterTest#test_read_timeout_error_opens_the_circuit fails when we bump Rails to this commit.

To solve it, in our execute_intent patch we can check the intent for an error and raise it within the acquire_semian_resource block to trip the circuit breaker. Then we can rescue it and return the result to match the new Rails behaviour.

Comment thread lib/semian/activerecord_adapter.rb Outdated
Comment thread lib/semian/activerecord_adapter.rb Outdated
@andrewn617
andrewn617 force-pushed the execute-intent-without-raising branch 2 times, most recently from a426727 to 60406e6 Compare September 3, 2026 19:36
Comment thread lib/semian/activerecord_adapter.rb Outdated
Comment thread lib/semian/activerecord_adapter.rb

@Edouard-chin Edouard-chin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to also rm -rf the Gemfile.lock before the bundle install steps kicks in on CI. This way we are sure bundler picks the latest Rails edge

…red on the query intent and raise it to trip the circuit breaker, then rescue and allow Rails to continue execution.
@andrewn617
andrewn617 force-pushed the execute-intent-without-raising branch 3 times, most recently from 0832de9 to e2f1cae Compare September 4, 2026 13:32
@andrewn617
andrewn617 force-pushed the execute-intent-without-raising branch 3 times, most recently from 764c346 to cea4e92 Compare September 4, 2026 14:45
@andrewn617

Copy link
Copy Markdown
Member Author

Alright, I took a crack at improving the testing situation for this gem. Now the main gemfile and all the testing gemfiles lock to the latest released version, and I added _edge versions for each. That way we are running all tests against Rails 8.1 and edge.

I had to keep minitest locked to 5.6. If we bump it to 6.0 we get some test failures, so that can be done in a separate PR.

Also, this uncovered we weren't handling PG::ConnectionBad errors that can be raised by 8.1, since edge fixed this behaviour and we were only testing on edge. So I fixed that.

Comment thread .gitignore Outdated
Comment thread test/adapters/activerecord_adapter_shared_tests.rb Outdated

@Edouard-chin Edouard-chin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@adrianna-chang-shopify Usually committing the main Gemfile.lock is enough.

The lockfile in gemfiles/ are mostly CI only, and it's rare that developers need to run BUNDLE_GEMFILE=gemfiles/Rails_8.2 bundle exec rake test. In the event that they have a failing CI for a specific gemfile, then it's better to not have a lockfile committed so that tests run with the same dependencies as on CI (the latest one).

Committing the Gemfile.lock remove all possible friction to contribute to a project

@adrianna-chang-shopify

Copy link
Copy Markdown
Contributor

The lockfile in gemfiles/ are mostly CI only

@Edouard-chin , yep fair, in this case though, should we remove all lockfiles under gemfiles/? As long as the main lockfile is committed to help contributors, it would be advantageous for the CI-only ones to resolve their latest permitted dependencies right?

We can probably also drop the rm from CI then if we're not tracking the edge lockfiles with git.

@Edouard-chin

Copy link
Copy Markdown
Member

in this case though, should we remove all lockfiles under gemfiles/

@adrianna-chang-shopify Ah yes, great point. I think we should

@andrewn617
andrewn617 force-pushed the execute-intent-without-raising branch from cea4e92 to 2942b8e Compare September 4, 2026 16:58
Consistently run all tests against both the latest released Rails version as well
as Rails edge. Also drop tests for Ruby 3.2 as it is EOL.
@andrewn617
andrewn617 force-pushed the execute-intent-without-raising branch 2 times, most recently from cc06ea3 to 47cba91 Compare September 4, 2026 17:05
…s/rails@e61b5e24 rescues these and converts them to ActiveRecord::ConnectionNotEstablished, but that is only on edge. Now that we are also testing on 8.1 it exposed that we are not handling the 8.1 behaviour which is to raise a PG::ConnectionBad.
@andrewn617
andrewn617 force-pushed the execute-intent-without-raising branch from 47cba91 to e4c7e51 Compare September 4, 2026 17:11
@andrewn617
andrewn617 merged commit f2de776 into main Sep 4, 2026
34 checks passed
@andrewn617
andrewn617 deleted the execute-intent-without-raising branch September 4, 2026 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants