Big Issue (results in negative numbers in inventory)

[DxM]

2 people are able to buy the last of an item as long as the second
person adds it to their cart before the first person confirms their
order. This resulted in me having a -1 in my inventory.

The code needs to be changed so that the inventory is checked at order
confirmation, not just at the addition into the cart.

Is this possible? Easy? I haven't had time to try it myself, but I
need to get this ironed out before August 1st. I expect a rush on the
limited products I am selling, and having people being able to buy
inventory that's non-existant will be a nightmare.

[rogerdpack]

I believe it is double checked at some point along its way. Like when
they first click checkout or something. However, as you point out, if
a two users get past that point and both haven't checked out then it
would cause a conflict.
I suppose the only 'real' way is to double check their inventory right
before calling run_transaction. Since rails is single threaded, and
you're probably only on one instance of it, that would work. I'd just
copy and paste the checking code [or write your own there], and if
something has gone out of stock, flash a message saying what it was
and send them back to checkout or something [before charging them for
it, obviously :) ].

Man I almost feel sorry for the substruct maintainers, since I think
sometimes a lot of people [not referring to this email, which is
legit--just in general] will start using substruct with little or no
rails and/or programming background. If they knew rails they wouldn't
need substruct as much :) Oh wait--except I know rails and use it
just because I'm lazy. Obviously I contradict myself :)
:P

-R

[Edmundo Valle Neto]

rogerdpack escreveu:
> (...)

>
> Man I almost feel sorry for the substruct maintainers,

That I know of, theres no such plural.

> since I think
> sometimes a lot of people [not referring to this email, which is
> legit--just in general] will start using substruct with little or no
> rails and/or programming background.

I think theres no problem with that.

> If they knew rails they wouldn't
> need substruct as much :) Oh wait--except I know rails and use it
> just because I'm lazy. Obviously I contradict myself :)
> :P
>
> -R

> (...)

I really didn't got your point.

What I have saw mostly is people requiring something with hurry as it
was an obligation of someone to give support instantly.
REQUIRING that (and sometimes nonsense things, just by ignorance, as
demand that the version be upgraded just because they don't know how to
deploy an application in a hosted solution) for volunteer work in my
opinion this is just stupid.

Some projects have differentiated and of course paid support plans, here
at least for now theres no such a thing.

I think that if people don't want/know how to be part of the solution at
least they can have the good sense of not be part of problem.

If that kind of collaborative work don't please some people, they can
always buy a closed product as they don't pretend/want/know how to help
anyway. Then they will not have to worry to claim for anything.

Regards.

Edmundo Valle Neto

[DxM]

Ok I got it.

Here's what I did:

def finish_order
if Preference.find_by_name('store_use_inventory_control').is_true?
removed_items = @cart.check_inventory()
logger.info "REMOVED ITEMS: #{removed_items.inspect}"
# If any items were removed, flash and alert.
if removed_items.size > 0
flash_msg = "The following item(s) have gone out of stock
before you could purchase them:"
for item_name in removed_items do
flash_msg << "\n* #{item_name}"
end
flash_msg << "\n\n...The item(s) have been removed from your
cart."

if @cart.empty?
redirect_to_index(flash_msg) and return
end
end
end

@title = "Thanks for your order!"
cc_processor = Order.get_cc_processor
order_success = @order.run_transaction

if order_success == true
@payment_message = "Card processed successfully"
clear_cart_and_order(false)
elsif cc_processor == Preference::CC_PROCESSORS[1]
case order_success
when 4
@payment_message = %q\
Your order has been processed at PayPal but we
have not heard back from them yet. Your order
will be ready to ship as soon as we receive
confirmation of your payment.
\
clear_cart_and_order(false)
when 5
@payment_message = "Transaction processed successfully"
clear_cart_and_order(false)
else
error_message = "Something went wrong and your transaction
failed.<br/>"
error_message << "Please try again or contact us."
redirect_to_checkout(error_message)
end
else
# Redirect to checkout and allow them to enter info again.
error_message = "Sorry, but your transaction didn't go
through.<br/>"
error_message << "#{order_success}<br/>"
error_message << "Please try again or contact us."
redirect_to_checkout(error_message)
end
end

Yes, I just hacked the additional code out of 'do_checkout' Feel
free to ridicule me if this is stupid, but I also wouldn't mind being
let known if there's a better way to do it.

[William Chow]

This is a rather serious race condition bug, and it cannot simply be addressed in substruct. The ActiveRecord ORM does not handle race conditions very well, and as such simply adding in a validation to ensure that inventory is always above 0 will not work (it may seem to work, but it will break, especially for a high volume site).
The only way to ensure that this race condition doesn't cause -1 inventory is to add in a check constraint, which MySQL doesn't support. You'd have to code in a necessary trigger to substitute for the lack of check constraints.

Sincerely,

William Chow

--- On Fri, 7/25/08, DxM <xmisc...@gmail.com> wrote:

[William Chow]

Your code doesn't guarantee that the same thing won't happen again, it merely moves the problem to another part of the code.
The code may be enough to prevent you from seeing the error, and it may solve your problem for the immediate term, but most likely for a high volume site you'd get the same error in the future.
In other words, the code is non-reentrant.

Will

--- On Sat, 7/26/08, DxM <xmisc...@gmail.com> wrote:

> From: DxM <xmisc...@gmail.com>
> Subject: [Substruct] Re: Big Issue (results in negative numbers in inventory)
> To: "substruct" <subs...@googlegroups.com>

[DxM]

That makes a lot of sense.

Roger's comment about Rails being single threaded had me feeling
falsely secure (thanks to my ignorance) that just making sure the
inventory was there was going to be enough to prohibit customers from
purchasing non-existant items. This certainly sounds more difficult
than I was expecting. I guess I will have to do a little more
research. Thanks for the help.

[William Chow]

Actually, Rails being single-threaded is somewhat tangential to the problem. More like, Rails isn't really a thread-aware framework, specifically, ActiveRecord. Ruby itself is threaded, although the threads are implemented in software (at least so-called "C-Ruby"). The fact is that the Rails framework doesn't take into account concurrent processing, which is fine for the majority of things, but not when it comes to persistent access. Not only is this a problem with ActiveRecord, but it is also a problem when accessing any data, period. Essentially, a web application is an instance of multiple instances, single data.
Due to the complexity of multi-threaded programming, not a lot of programmers understand the problem, much less are able to find a good way to resolve the situation.
ActiveRecord, IMHO, is majorly broken. It has to do with the brain-damaged design philosophy behind it, which tries to abstract the DB layer from you. Well, that doesn't really work in the real world. Specific DB mechanisms, such as constraints, views, and triggers exist for a reason.

Will

--- On Sat, 7/26/08, DxM <xmisc...@gmail.com> wrote:

> From: DxM <xmisc...@gmail.com>
> Subject: [Substruct] Re: Big Issue (results in negative numbers in inventory)
> To: "substruct" <subs...@googlegroups.com>

[Edmundo Valle Neto]

This helps to clarify the situation [1], I would stay with the trigger
solution from Will to not let it drop below 0 and catch it in the app.

1. http://www.spacevatican.org/2008/6/8/dealing-with-concurrency

Regards.

Edmundo Valle Neto

DxM escreveu:

[DxM]

This is certainly far more difficult than I thought it might be.

For now, my solution has been to change the order model to only
authorize the credit card charges and not capture the funds. This way,
I can manually capture and prohibit too many items from being sold.
This may leave a few unhappy customers who will be getting emails
explaining the item sold out befoe they could get it, but that is
better than having to refund a bunch of credit cards.

Thanks for opening my eyes to the depth of this issue. (in turn
forcing me to read a bunch about race conditions, ActiveRecord, ORM,
and to learn the substruct code even better)

[Edmundo Valle Neto]

I was looking at this, the trigger is easy to do, the exception is easy
to catch and handle but I couldn't think in a solution to tie it with
the payment processing.

With one transaction:

Situation 1:

User1 start transaction.
Decrease quantity for each item (an exception is raised by the trigger).
Rollback.

Thats OK.

Situation 2:

User1 start transaction.
Decrease quantity for each item (no exception is raised).
Commit.
Payment fails, the order is abandoned.

Thats not OK. The quantity was decreased because of an order that will
not be finished.

That I know of, InnoDB tables doesn't support nested transactions so
wrapping the two actions makes things worse.

Situation 3:

User1 start order transaction.
User2 start order transaction.
User 1 decrease quantity for each item (no exception is raised).
User 2 decrease quantity for each item (no exception is raised).
User1 payment approved.
User2 payment approved.
User1 commit order transaction.
User2 try to commit order transaction, it fail in the items, and
everything is rolled back. But then he was already charged.

Someone? Any toughs about that? Should situation 2 be used and
unfinished orders be cleaned to give their items back to stock?

Regards.

Edmundo Valle Neto

DxM escreveu:

[Edmundo Valle Neto]

Maybe save the quantities in another object, start a new transaction and
if the payment failed add those quantities back?

Edmundo Valle Neto escreveu:

[Edmundo Valle Neto]

For anyone that is interested in or want to point out defects in the fix
:P, it was patched in issue 112.

Regards.

Edmundo Valle Neto

[patrickberkeley]

One of my co-workers suggested a better way to take care of this would
be to add a ledger_quantity column to the items table. Every time
add_to_cart(_ajax) is run, the quantity would be subtracted from the
actual product quantity then that would be stored in the ledger
column. Shorten the session life and expire both the session and the
ledger_quantity column at the same time (maybe 30 minutes). When no
one has a session open, the default for the ledger_quantity is to
equal the product's actual quantity.

[Edmundo Valle Neto]

Are you proposing to wait 30 minutes to restore not finished cart items,
for other people be able to buy them?
I think I didn't understood it.

Regards.

Edmundo

patrickberkeley escreveu:

> One of my co-workers suggested a better way to take care of this would
> be to add a ledger_quantity column to the items table. Every time
> add_to_cart(_ajax) is run, the quantity would be subtracted from the
> actual product quantity then that would be stored in the ledger
> column. Shorten the session life and expire both the session and the
> ledger_quantity column at the same time (maybe 30 minutes). When no
> one has a session open, the default for the ledger_quantity is to
> equal the product's actual quantity.
>
>

> (...)

[patrickberkeley]

Yes. Restore the quantity of a product in a user's session after a
period of time. It doesn't need to be as long as 30 minutes, but the
quantity of a certain product in multiple users' session should be
kept track of in the database. Then check the quantity a user is
adding to their cart against the ledger quantity. That way a product
will never be over-sold, and it prevents the user from getting all the
way through the checkout process only to be told 'This item went out-
of-stock while you were checking out'.

[Edmundo Valle Neto]

patrickberkeley escreveu:

> Yes. Restore the quantity of a product in a user's session after a
> period of time. It doesn't need to be as long as 30 minutes, but the
> quantity of a certain product in multiple users' session should be
> kept track of in the database. Then check the quantity a user is
> adding to their cart against the ledger quantity. That way a product
> will never be over-sold, and it prevents the user from getting all the
> way through the checkout process only to be told 'This item went out-
> of-stock while you were checking out'.
>
>

(...)

I think the fact of see the products already in a cart as unavailable
products as a totally different approach but not better or worst.
That means if I add all items to my cart nobody can buy them, it can be
used even as a "denial of service" attack? I can automate the cart
manipulation from 30 to 30 minutes and freeze your store?

Regards.

Edmundo

[patrickberkeley]

That's an interesting question about the "denial of service" attack.
You could check the ip address and limit its activity. Seems like it's
starting to get messy.

[Edmundo Valle Neto]

patrickberkeley escreveu:

> That's an interesting question about the "denial of service" attack.
> You could check the ip address and limit its activity. Seems like it's
> starting to get messy.
>

> (...)

Then you can make it pass trough a ton of anonymous proxies :)