Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Studio: Increase timeout in wp-cli-process and terminate database import on timeout #741

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

katinthehatsite
Copy link
Contributor

@katinthehatsite katinthehatsite commented Dec 13, 2024

Related issues

Closes https://github.com/Automattic/dotcom-forge/issues/10032

Proposed Changes

This PR increases the timeout for the wp-cli-process so that the database import does not fail for larger sites.

Some related details:

  • While testing the backup from https://github.com/Automattic/dotcom-forge/issues/10032 , I noticed that the error is specifically related to executing wp-cli commands: Warning during import of /var/folders/bj/1qgxs1qd6_z20tw9llkw842h0000gn/T/studio_backupsJr1QS/sql/wp_aiowps_events.sql: error when executing wp-cli command.
  • After inspecting the error more and comparing other .sql files from the wp_aiowps_ that have the same configuration, I noticed that wp_aiowps_events.sql is substantially larger.
  • I suggest increasing the timeout in wp-cli-process so that the larger .sql files get processed without failing.

Testing Instructions

To test the fix for importing larger .sql files

  • Download the backup for the site reported in CfT: https://mc.a8c.com/rewind/debugger.php?site=220627588# (you can produce the backup by clicking on Create backup link and download it after)
  • Pull the changes from this branch
  • Run npm install && npm start
  • Open the Studio app
  • Navigate to the Import / Export tab
  • Drop the backup into the Import field
  • Wait for the import to finish
  • Confirm the import finishes successfully
  • You can also repeat the same steps on trunk and confirm that the database import fails

To test the fix for terminating database import process when it fails:

  • Download the backup for the site reported in CfT: https://mc.a8c.com/rewind/debugger.php?site=220627588# (you can produce the backup by clicking on Create backup link and download it after)
  • Pull the changes from this branch
  • Set this timeout to a shorter period of time e.g. 1 minute so that you do not have to wait for a long time for the import failure
  • Run npm install && npm start
  • Open the Studio app
  • Navigate to the Import / Export tab
  • Open Activity Monitor app on macOS
  • Observe low CPU usage by Studio under the CPU tab (see screenshot for reference of what to look for):

Screenshot 2024-12-16 at 11 29 17 AM

  • Drop the backup into the Import field to start the import
  • Observe that the CPU usage rises drastically
  • Wait for the import to fail and click on OK once you see the error
  • Check the Activity Monitor and confirm that CPU usage is dropped back down to initial state before import (approximately)
  • This will confirm that the import process is stopped and not running any further

Pre-merge Checklist

  • Have you checked for TypeScript, React or other console errors?

@katinthehatsite katinthehatsite self-assigned this Dec 13, 2024
@katinthehatsite
Copy link
Contributor Author

I tried both 180000 (3 minutes > increased by 1 minute from the current value) and 240000(4 mins > increased by 2 minutes from the current value) and both worked as expected for me so I stuck with the shorter timeout. Happy to change it to a different value though if there are other suggestions.

@katinthehatsite katinthehatsite requested a review from a team December 13, 2024 19:17
@wojtekn
Copy link
Contributor

wojtekn commented Dec 16, 2024

@katinthehatsite altough it makes sense to increase the timeout, should we also ensure that when timeout is reached, the action that experiences it fails?

It seems to be related or similar to #544 where the import process fails in Studio, but the database import continues running in the background.

src/lib/wp-cli-process.ts Outdated Show resolved Hide resolved
@katinthehatsite katinthehatsite changed the title Studio: Increase timeout in wp-cli-process Studio: Increase timeout in wp-cli-process and terminate database import on timeout Dec 16, 2024
@katinthehatsite
Copy link
Contributor Author

altough it makes sense to increase the timeout, should we also ensure that when timeout is reached, the action that experiences it fails?
It seems to be related or similar to #544 where the import process fails in Studio, but the database import continues running in the background.

Thanks for the review @wojtekn ! I added some handling for that and updated testing steps. Let me know what you think

@katinthehatsite katinthehatsite requested review from wojtekn and a team December 16, 2024 16:48
Copy link
Contributor

@wojtekn wojtekn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code change looks correct. It kills the process and informs user about failed import when timeout is reached.

However, three minutes is still low as database dumps can take minutes to import. In my case, it couldn't import linked site's database within three minutes.

Let's go with 5 minutes for now, and later, we should benchmark it to see what should be the reasonable limit that allows importing bigger sites.

src/lib/wp-cli-process.ts Outdated Show resolved Hide resolved
@katinthehatsite
Copy link
Contributor Author

However, three minutes is still low as database dumps can take minutes to import. In my case, it couldn't import linked site's database within three minutes.
Let's go with 5 minutes for now, and later, we should benchmark it to see what should be the reasonable limit that allows importing bigger sites.

Cool, sounds good. The timeframe of 3 minutes worked for me but I was not sure what would be the optimal amount for different environments where users might run Studio. I will adjust to five minutes 👍

@katinthehatsite
Copy link
Contributor Author

@wojtekn I made suggested changes. Let me know what you think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants