Payment processing state does not block double submission

[shioyama]

We've been experiencing issues where two requests complete an order in rapid succession resulting in two purchase requests being sent to the gateway. This results in a success followed by a failure (double submission) which overwrites the payment state to failed, causing problems down the line.

At first I thought this was something specific to do with the payment gateway or our spree customizations, but this PR confirms that the "processing" state which should prevent this is not working, because it is set within a transaction. AFAICT this would affect any store using Spree. Although here I'm just testing that ActiveRecord::Base.connection.open_transactions is zero, I've actually tested parallel requests and confirmed that the "processing" state is not being persisted to the db at any time.

Can someone confirm that this is the case? If so I can try to push up a fix. If I'm misunderstanding something, then some clarification would be great (I'm fairly new to Spree).

Thanks!

[radar]

@shioyama I think this test should be within core/spec/models. It's not doing any request-y stuff, so why would it need to be in spec/requests?

[shioyama]

See below, the fact that it is happening as a result on an order state transition is exactly why it doesn't work as expected.

[shioyama]

But yes, I can move this to core/spec/models, you're right it's not doing anything request-y. First I'd just like to confirm that this is indeed the problem.

[shioyama]

Ok, moved this to core/spec/models/order/checkout_spec.rb. Not really sure that's the right place, but there didn't seem to be anywhere really appropriate. The existing payment specs are not testing this scenario.

[shioyama]

@radar I added a commit which fixes this problem by turning off transactions around Spree::Order state transitions. All tests pass, but I'm not sure this is safe.

In any case we need a solution and this seems important, so any advice would be much appreciated.

[huoxito]

👍 This makes a lot of sense to me. We should apply to the other branches too. Thanks a lot @shioyama

[shioyama]

@huoxito Great, good to hear that. I'd be happy to make PRs for other branches, if these changes look okay.

Just one thing, I see that the default ActiveRecord integration in state machine runs transitions around validations. This PR would turn that off unless we explicitly add it back (like I did to get it to run transition callbacks around the action itself.)

No tests are failing and this doesn't look like an issue, but I just wanted to check.

[huoxito]

yes @shioyama please submit another PR to master including a changelog entry :)

Just one thing, I see that the default ActiveRecord integration in state machine runs transitions around validations.

Sorry I'm not sure I understand that bit. Are you saying that your patch turn off validations when doing transitions? I think I misunderstood that, validations stills seem to happen with your patch applied.

thanks for looking into state_machine internals to figure this @shioyama, appreciate it

[shioyama]

@huoxito great, will submit that asap.

Sorry I'm not sure I understand that bit. Are you saying that your patch turn off validations when doing transitions? I think I misunderstood that, validations stills seem to happen with your patch applied.

No, the other way around: the patch turns off transitions around validations. By default ActiveRecord models with state machines insert into model actions (save by default) and validations a call to StateMachine::Transition#perform, which executes any outstanding transitions as callbacks on the model:

# Runs state events around the object's validation process
def around_validation(object)
  object.class.state_machines.transitions(object, action, :after => false).perform { yield }
end

So if you save the model, it will automatically run any transitions as an around_save callback, and likewise if you call @model.valid? it will also run any transitions as an around_validation callback and then check if the model is valid. The former is important (one test failed without putting it back) but the latter seems to me unnecessary.

Incidentally, although I added back the action callback, it now fires around the action (save) rather than as an ActiveRecord callback on the model, which would run within the action (and therefore be wrapped in a transaction, which we don't want). As a result, if a transition succeeds but the action then fails, the transition will remain persisted as-is, whereas previously it would have been rolled back (except in case of a payment processing failure, see #1767).

Took me a while to get my head around this... but I believe this is basically how it works.

[shioyama]

The last part of my comment above is not quite right. The action callbacks are fired around the action by default, and only as model callbacks (within the action) for save. Using save_state as the action and aliasing that to save (see these inline notes) prevents the callbacks from being run as model callbacks, so they wrap around the action (so not wrapped in a transaction).

So, by default a save action looks like this:

# save (transaction)
  # before transition callback
  # save code
  # after transition callback
# end

By renaming the action and turning transactions off, this becomes:

# before transition callback
# save (transaction)
  # save code
# end
# after transition callback

But now if we save the model, transitions are not fired, only the other way around (i.e. making a transition between states saves the model). This test was failing because it seems to (implicitly?) rely on this behavior, so I added it back. The result is this:

# before transition callback
# save (transaction)
  # before transition callback
  # save code
  # after transition callback
# end
# after transition callback

The transitions get called outside of the transaction and inside the transaction. In the case of the failing test, something between the first (outer) and second (inner) before transition callback changes so that in the latter case, it actually transitions to complete, and the test passes. In other cases these added callbacks do not change anything.

... that's probably more than you wanted to know, but I wanted to get this down for the record, in case anybody comes back to this.

[huoxito]

Thanks for the thorough explanation, great to have it documented here.

Took a while into this now but I still don't understand why that spec is failing. Maybe the set up is just wrong. Everything else seems to work perfectly for me without the around_save :perform_transitions. Definitely missing something here. Also are you saying that a before_transition transition callback, for example, would fire twice now? Doesn't seem to be happening for me.

I think ideally we should only turn off transactions when processing payments since I don't remember anywhere else on the order state_machine that relies on persisted data. But I'm not sure how to do that, if it's possible right now.

Anyway we still go with your patch just wanted to understand it better before merging. Maybe this also brings other benefits we can't see right now. Again appreciate all your investigation here @shioyama

[shioyama]

@huoxito My conclusion after going deep into a stack trace is that that spec is just screwed up. Something is happening with email validation -- it shouldn't even pass because the email is not set, so the model can't be saved, but somehow it previously was (being saved) regardless of validation failing. With the change this no longer happens, but that's a good thing I think.

Can I just mark it as pending with a note that it is not actually testing what it claims to be testing? The same thing happens on master.

[shioyama]

And: I will take out that around_save callback, which was only needed to make that test pass.

[shioyama]

Don't pull this yet, going to clean it up first.

[shioyama]

Ok took out the callback and marked the failing test as pending, as I did in #4542. This should be good to go.

[shioyama]

@huoxito Thanks for merging #4542. Can you merge this one too (or let me know if there are any issues)? This is actually the one we need 😄

[huoxito]

Merged. Thank you @shioyama :)

[shioyama]

Thanks!

[ghost]

Just as a follow up, we turned transactions back on in our Spree site and it improved the stability A LOT.

Per the documentation here https://github.com/pluginaweek/state_machine/blob/master/lib/state_machine/integrations/active_record.rb#L286

here is the callback list

Callbacks occur in the following order. Callbacks specific to state_machine

# are bolded.  The remaining callbacks are part of ActiveRecord.
# 
# \* (-) save
# \* (-) begin transaction (if enabled)
# \* (1) _before_transition_
# \* (-) valid
# \* (2) before_validation
# \* (-) validate
# \* (3) after_validation
# \* (4) before_save
# \* (5) before_create
# \* (-) create
# \* (6) after_create
# \* (7) after_save
# \* (8) _after_transition_
# \* (-) end transaction (if enabled)
# \* (9) after_commit

[JDutil]

@tesserakt could you elaborate on the stability improvement? Should we turn them back on by default? I'm pretty sure we've even since removed the regression spec added to this.

[ghost]

@JDutil We only allow 1 shipment per location in our Spree Store. Without the lock, when the server got slow some customers would double submit on the address page. This created a race condition that sometimes allowed 2 identical shipments to be created for the same location, i.e. duplicates. Adding the transaction back in prevents this.

[ghost]

@JDutil there were some other weird edge cases we used to see but no longer do after transactions

For example: some orders coming in without completed_at fields.

Some orders that have no shipments
Some orders where the shipments don't have a location attached.

Still looking into some of these, but they have disappeared since adding transactions.

Granted we are running a slightly modified version of 2.3, but making the change helped.

[huoxito]

@tesserakt I think we wanted to disable the transaction only for the payment processing which seems pretty useful to prevent double processing but I dont think it's possible not sure. We'd probably need to decouple the payment processing from state machine callbacks (this alone sounds like a nice change already)

[JDutil]

I've seen tons of errors related to the duplicate shipments, and it drove me nuts on 2.2+ stores. I thought we addressed it by adding the order lock version, which I believe would also have helped resolve double payment processing as well. I don't recall if we made that lock version change to 2.3 or if it wasn't until 2.4+

I think removing payments from state machine callback would be a good idea too, but not sure how best to accomplish that.

[ghost]

@huoxito you could write a record outside of the transaction (using a separate thread) that says "Hey I'm processing payments" and then in the transaction check for that.

[JDutil]

Well we added it to 2-2-stable+ db52e1e

[ghost]

@JDutil we still get double shipments even with that order lock code in 2.3

[ghost]

@JDutil @huoxito re-enabling transactions on the order requires a little finagling in the finalize method. If you get past processing the payments but then hit a snag, i.e. a random exception gets thrown somewhere in the stack; it is nice to have the finalize code catch the exception, auto void the payment, send the admin an email, then roll back the transaction and notify the user that something went wrong.