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

Set higher timeouts for import/export wp-cli commands #904

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

Conversation

ashfame
Copy link
Member

@ashfame ashfame commented Feb 6, 2025

Related issues

Proposed Changes

  • Default timeout of 5mins for waiting on wp-cli command to finish is too small for big imports. In this PR, I have added a higher timeout for import/export commands, which gets used by passing an arg longRunning: true to executeWpCliCommand()
  • Show a custom error message when it does timeout (currently set to 24hrs)
Screenshot 2025-02-06 at 22 13 40

Testing Instructions

  • I tested it by lowering the default timeout to small like 5 secs and process a normal backup file, it fails as per the linked issue
  • Then on this branch, keep the default timeout lowered and see it uses the higher timeout set for import/export and finishes up.

Pre-merge Checklist

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

@ashfame ashfame self-assigned this Feb 6, 2025
@ashfame ashfame marked this pull request as ready for review February 7, 2025 08:46
@ashfame ashfame requested a review from a team February 7, 2025 08:46
) {
await getIpcApi().showErrorMessageBox( {
title: __( 'Failed importing site' ),
message: __( 'Import process timed out. Very large import?' ),
Copy link
Member Author

Choose a reason for hiding this comment

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

What should this message say?

Copy link
Member

Choose a reason for hiding this comment

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

Here is my suggestion: The import process timed out after %d hours, which can occur when processing very large imports. If the issue persists, please contact support.')

@@ -11,7 +11,8 @@ export type MessageName = 'execute';
export type WpCliResult = ReturnType< typeof executeWPCli >;
export type MessageCanceled = { error: Error; canceled: boolean };

const DEFAULT_RESPONSE_TIMEOUT = 300 * 1000;
const DEFAULT_RESPONSE_TIMEOUT = 5 * 60 * 1000; // 5min
const IMPORT_EXPORT_RESPONSE_TIMEOUT = 24 * 60 * 60 * 1000; // 24hr
Copy link
Member Author

Choose a reason for hiding this comment

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

What should be the timeout we set here?

Copy link
Member

Choose a reason for hiding this comment

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

What if we start with fewer hours? I think 6 hours should be enough for most imports, if not maybe we could remove the timeout from imports. Usually 0 represents infinite in these context.
I wonder if these constants could live in a separate config file that the user can tune for their specific needs.

Copy link
Member

@sejas sejas left a comment

Choose a reason for hiding this comment

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

The changes look good. Increasing the import process will allow users to import websites with more content.

To establish reasonable limits, we might need to benchmark the amount of content imported on a typical computer and set a limit based on those metrics.

I’ve added a couple of suggestions, please let me know what you think.

Comment on lines +62 to +63
phpVersion?: string;
longRunning?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

What do you think if we pass the exact timeout to wait as a parameter? That way we could be more precise with the different type of executions.
Instead of having a boolean longRunning, we could have an integer with the timeout ms. By default it would be DEFAULT_RESPONSE_TIMEOUT.

);

if ( stderr ) {
console.error( `Warning during import of ${ sqlFile }:`, stderr );
console.error( `Error during import of ${ sqlFile }:`, stderr );
Copy link
Member

Choose a reason for hiding this comment

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

👍

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