Fix non-local returns inside transaction blocks (silent rollback on Rails 7.0/7.1) - #32
Open
TalGruperFolloze wants to merge 1 commit into
Open
Conversation
β¦ails 7.0/7.1) Fixes rameerez#31 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
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.
Fixes #31.
Rails' behavior for
returnout of atransaction/with_lockblock changed across the versions this gem supports (rails >= 6.1): on 7.0, and on 7.1 withoutload_defaults 7.1, a non-local return silently rolls back the transaction while the method still returns normally.Audited every
transaction/with_lockblock inlib/for lexicalreturn/break/throwand found three:wallet.rbβWallet#add_credits:return transactionβtransaction(the issue's report; every credit grant routes through this, so affected apps lose every grant with no error)concerns/pay_subscription_extension.rbβ plan-change handler:returnafterclear_pending_plan_change(which does anupdate!) βnext, so the metadata clear isn't rolled backservices/fulfillment_service.rbβFulfillmentService#process: earlyreturn unless due_for_fulfillment?βnext(nothing written before it, but same deprecated pattern)The
breakinWallet#deduct_creditsis inside an innereachloop, not the transaction block, so it's fine. All otherreturns near transactions are in methods called from blocks, which are ordinary method returns.Per the issue's definition of done, added a test that grants credits and asserts the transaction row and balance actually persist after the call (reloading the wallet), since the failure mode returns a truthy
Transactionand only a persistence assertion catches it.I skipped enabling
Rails/TransactionExitStatement: the repo's RuboCop setup doesn't loadrubocop-railsand RuboCop isn't wired into CI or the Rakefile, so the config would be inert β happy to add it if you'd like the dependency.Tests:
wallet_test.rb(63 runs),fulfillment_service_test.rb(32),pay_subscription_extension_test.rb(79) β all green on sqlite3.π€ Generated with Claude Code