ActiveRecord adapter: unexpected error when running in a transaction #866
Comments
|
Hi! Can you paste the full statement invalid error? We rescue What version of rails are you using and what database? |
|
Hi! Here's the full error message:
which makes sense to me, the INSERT statement from flipper fails, which aborts the transaction I'm using rails |
|
@tjefferson08 can you get the failure from flipper? Add a rescue around it and puts the error or something? I want to see what error is failing the transaction, because as I said above, we have a rescue for this so its idempotent, so it must be something different that is being raised that we aren't rescuing. Thanks! |
I think the "something different" is the I would not expect rescuing I tried to get a failing test into the flipper test suite but I couldn't get any of the adapter-specific tests to run (even after following the docker-compose instructions) |
|
Where was the original test case from in the issue above? The problem is we can’t replicate it so that’s why I need you to so that I can see what is inside of the active record statement invalid because I don’t want to rescue all of those as that would lead to problems, I’ll likely have to check the error message in addition to the class. |
|
yeah rescuing the test from the report was something I cooked up within my rails app's test suite as I was troubleshooting this error (it was causing an issue in one of our controllers) I'm still trying to get the flipper test suite to go locally, I made some progress by updating some of the docker configs a minimal new rails app might be another avenue for reproducing |
|
@tjefferson08 could you just run it from your app again? If I can just see what the error is I can setup a test to fix it. Maybe this? ActiveRecord::Base.transaction do
begin
Flipper.enable_actor(:foo, actor)
rescue => e
p e
raise
end
end |
|
Here's the trace I get from minitest: I was also able to get a failure to show up within the flipper test suite: # spec/lib/adapters/active_record_spec.rb, added at line 66
it 'should not poision wrapping transactions' do
flipper = Flipper.new(subject)
actor = Struct.new(:flipper_id).new('flipper-id-123')
flipper.enable_actor(:foo, actor)
ActiveRecord::Base.transaction do
2.times { flipper.enable_actor(:foo, actor) }
end
endI don't know why I have to run it 2 times within the transaction here 🤷 |
|
ah OK I narrowed this down it's not until the "next query" that the transaction explodes it 'should not poision wrapping transactions' do
flipper = Flipper.new(subject)
actor = Struct.new(:flipper_id).new('flipper-id-123')
flipper.enable_actor(:foo, actor)
ActiveRecord::Base.transaction do
flipper.enable_actor(:foo, actor)
# we get here just fine!
puts 'got here!'
expect(true).to be true
# whoa it's not until the next query that the transaction blows up
# with StatementInvalid
expect(Flipper::Adapters::ActiveRecord::Gate.count).to eq 1
puts 'never got here!'
expect(true).to be false
end
end |
|
Are you using transactional fixtures? I wonder if somehow that is in play here. |
|
the original test I provided was part of our rails app's test suite, so yes, that uses transactions to keep tests from polluting one another (rails default behavior) but I ran that last code snippet within flipper's own test suite; hopefully you can reproduce that |
|
@jnunemaker here's a PR with that test failing, just to be super clear sorry, I know PR comments can sometimes be kinda indecipherable |
|
@tjefferson08 Thanks! I'll take a look. Sorry this hit right during a busy stretch and then vacation. I'll see what I can get figured out. |
|
We are seeing flavors of this too. My initial question was whether there was a way to get flipper to use nested transactions or a fully-separate DB connection to prevent this. |
|
Any updates on this? We are starting to remove flipper from some key code paths because we are hitting this so much in production right now. |
|
#881 has this fixed. Should have a new release soon. I think there are a couple small things I want to sneak in before next release. |
|
thanks @jnunemaker 🥇 |
Calling
enable_actoris not idempotent (from the application perspective)1st time: 😄
2nd time: 😡
The observed effect:
Running flipper code in a transaction can unexpectedly "poison" the transaction
As I understand it, this is because the AR adapter is relying solely on DB constraints to enforce uniqueness, whereas most rails apps combine DB constraints for uniqueness with a corresponding validation on the AR model
The text was updated successfully, but these errors were encountered: