Automattic / woocommerce-services

WooCommerce Services is a feature plugin that integrates hosted services into WooCommerce (3.0+), and currently includes automated tax rates and the ability to purchase and print USPS shipping labels.
GNU General Public License v2.0
105 stars 20 forks source link

large orders causing 504 in orders dash #2731

Closed Nic-Sevic closed 1 month ago

Nic-Sevic commented 2 months ago

User with a large order (13k qty of a single item) is getting a 504/ OOM when they try to open the order screen to review the order. Doesn't happen for any other order (but no other orders on site same qty).

slack: p1713454127420779-slack-C7U3Y3VMY

I think the issue is that the code is trying to load all the order items for the meta box, and then timed out before it could finish: https://github.com/Automattic/woocommerce-services/blob/trunk/classes/class-wc-connect-shipping-label.php#L556-L576

issue doesn't happen when product is switched to virtual

p2 where being addressed: p9F6qB-eRR-p2 site: riotrfid.com SA: https://wordpress.com/wp-admin/network/admin.php?page=store-admin&action=search&username=riotrfid

samnajian commented 2 months ago

Blocked, as I'm waiting for access to be provided pLwCt-e5Z-p2 so that I can check the created staging site for this issue. The plan is to check if the issue arises from the queries themselves or how we make the server to spit out all of these data to the browser.

harriswong commented 2 months ago

👋 I was able to reproduce this locally when I was in janitorial. You can also spin up the local environment with the following step

  1. Add any product to the cart to checkout
  2. Purchase a quantity of 132136 of this product
  3. Go through the checkout as 1 item but with 132136 as the amount
  4. Go into wp-admin and click Orders http://localhost/wp-admin/edit.php?post_type=shop_order, click into that order.
  5. Observe the page hangs a bit.

If you look at the source, then you can see that the metabox is trying to load all the item data into the webpage. It's probably coming from this part of the code https://github.com/Automattic/woocommerce-services/blob/trunk/classes/class-wc-connect-shipping-label.php#L556-L576

samnajian commented 2 months ago

The is produced by these line where we want to enable shipment splitting, we can try to improve the performance of the PHP side, but when it gets to the client side, because of how we enable shipment splitting, a very similar issue will happen on the front-end as well, please the following on how we've implemented the split shipment for an item with quantity = 6, the bigger quantities breaks the page.

https://github.com/Automattic/woocommerce-services/assets/789421/c2d890d4-b97c-4ef3-a022-220916cb8089

@MatthiasReinholz We've added a functionality very similar to this for WCS, so we'd need to add a new implementation for that as well that is more performant.

The following solutions come to mind: 1) Add a setting to disable split shipment for such orders, which is only a transient solution to avoid breaking the page for users describe in this issue. 2) Changing the split functionality which shows only 1 single items on the label purchase modal for an order with more than 1 quantity and the merchant can split the item just by inputting a number which is a fraction of the main quantity, this was proposed by @harriswong when we added the Split Shipment functionality to WCS.

What do you think @MatthiasReinholz && @kloon?

upwardmomentum84 commented 2 months ago

The user also mentioned:

I am also having a related issue with large quantities. 2 of our accounts/profiles have quantities over 100,000 in their cart and now anytime we try to access or modify the cart, I get a 504 Gateway Time-out. So we cannot even delete the quantities since we cannot access the cart at all. This is relatively important as we cannot create mock orders or place any orders through those accounts.

It sounds like this is related to the issue described above?

samnajian commented 2 months ago

The user also mentioned:

I am also having a related issue with large quantities. 2 of our accounts/profiles have quantities over 100,000 in their cart and now anytime we try to access or modify the cart, I get a 504 Gateway Time-out. So we cannot even delete the quantities since we cannot access the cart at all. This is relatively important as we cannot create mock orders or place any orders through those accounts.

It sounds like this is related to the issue described above?

@upwardmomentum84 This sound similar, we're looking into a proper solution for this. I may be provide a workaround for this if the mentioned user is fine shipping the whole order in one shipment instead of splitting into multiple ones.

ariel-maidana commented 2 months ago

@samnajian I just asked the user if that would be OK with that.

sbathompson-he commented 2 months ago

@samnajian, the user confirmed that would be fine. Could you provide that workaround either here or in the ticket itself? 8047692-zen

samnajian commented 2 months ago

@samnajian, the user confirmed that would be fine. Could you provide that workaround either here or in the ticket itself? 8047692-zd-a8c

@sbathompson-he I'll be working on a workaround and will keep you posted.

Nic-Sevic commented 1 month ago

@samnajian Hey 👋 Just bringing up here since it might be tied to the same cause. The user recently shared the following:

I am also having a related issue with large quantities. 2 of our accounts/profiles have quantities over 100,000 in their cart and now anytime we try to access or modify the cart, I get a 504 Gateway Time-out. So we cannot even delete the quantities since we cannot access the cart at all. This is relatively important as we cannot create mock orders or place any orders through those accounts.

I'm guessing the same shipping data is getting pulled for the cart as the order screen

samnajian commented 1 month ago

I'm guessing the same shipping data is getting pulled for the cart as the order screen

@Nic-Sevic, I can't say with confidence it's the same issue, will take a look into it and report.

Nic-Sevic commented 1 month ago

Just noticed that I missed that Brandon had added this additional information already and it's already being considered

@upwardmomentum84 This sound similar, we're looking into a proper solution for this. I may be provide a workaround for this if the mentioned user is fine shipping the whole order in one shipment instead of splitting into multiple ones.