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

Improve the cleaning process for the failed preload actions #6059

Closed
4 tasks
piotrbak opened this issue Jul 19, 2023 · 6 comments · Fixed by #6082
Closed
4 tasks

Improve the cleaning process for the failed preload actions #6059

piotrbak opened this issue Jul 19, 2023 · 6 comments · Fixed by #6082
Assignees
Labels
effort: [XS] < 1 day of estimated development time module: preload priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@piotrbak
Copy link
Contributor

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version
  • Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
It looks like our implementation of actions clearing is not taking into the consideration failed actions in the database.

To Reproduce
Steps to reproduce the behavior:

  1. Make sure there are some failed preload actions in the AS
  2. Wait a couple of minutes or trigger manually rocket_preload_process_pending event.
  3. Check the failed actions in the AS, they're not reduced.

Expected behavior
We should also clean the failed actions, as they can stuck in the logs for a very long time. In my case, it looks like AS is not clearing them at all as this one is in the logs for 7 months.

Additional context
Not sure if we should use the same cleaner or different one, it might be useful to have ability to disable this cleaner for debugging purposes.

It's a part of this issue:
#6052

And database health in general can be improved by implementing also those issues:
#6057
#6058

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement module: preload priority: medium Issues which are important, but no one will go out of business. labels Jul 19, 2023
@Mai-Saad
Copy link

Note: the same is happening with RUCSS when executing action_scheduler_run_queue_rucss while having failed RUCSS entries in the actions table => only completed jobs are removed from the table

@piotrbak
Copy link
Contributor Author

piotrbak commented Jul 25, 2023

Acceptance Criteria:

  1. When rocket_preload_process_pending cron is triggered, we should also clean the failed AS jobs related to WP Rocket Preload and RUCSS
  2. Cleaner batch size (default 100) should apply for both, failed + completed
  3. When rocket_preload_process_pending cron is triggered, we should not clean the failed AS jobs related to 3rd parties
  4. With rocket_action_scheduler_retention_period filter, we should be apply to change the lifetime of items in the AS log

@piotrbak
Copy link
Contributor Author

@Mai-Saad For RUCSS we have

public function cron_remove_failed_jobs() {

@CrochetFeve0251
Copy link
Contributor

Currently we are using the Cleaner class to clean our AS actions.
However inside that class we didn't listed failed tasks:

$statuses_to_purge = [

If we don't want to keep them then we would just have a add them inside that logic.
If we want to have a different lifespan we would need to reimplement the logic from that for loop with a different cutoff:

foreach ( $statuses_to_purge as $status ) {

@CrochetFeve0251
Copy link
Contributor

CrochetFeve0251 commented Jul 31, 2023

Reproduce the problem

As we already discussed and reproduced the issue during a meeting last week I consider this as done.

Identify the root cause

The root cause is that the cleaner is not clearing errors cf:
https://github.com/wp-media/wp-rocket/blame/219a0989dc6ec41db29b47ac38f0869da57a3e86/inc/Engine/Common/Queue/Cleaner.php#L62

Scope a solution

The solution from @piotrbak is related to the wrong table (wpr table instead of AS table).
Instead for the similar fashion solution in RUCSS we need to look at the RUCSSQueueRunner on this line:

$cleaner = new Cleaner( $store, $batch_size, $this->group );

As per discussion with @piotrbak we need to support RUCSS failed action if possible and we do not need a specific filter for failed action retention.

Due tot that the solution will be to add the failed actions to the statuses to clean:
https://github.com/wp-media/wp-rocket/blame/219a0989dc6ec41db29b47ac38f0869da57a3e86/inc/Engine/Common/Queue/Cleaner.php#L62

For that we just have to add \ActionScheduler_Store::STATUS_FAILED, to that array.

Estimate the effort

Effort XS

@CrochetFeve0251 CrochetFeve0251 added the effort: [XS] < 1 day of estimated development time label Jul 31, 2023
@Tabrisrp
Copy link
Contributor

Looks good to me 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [XS] < 1 day of estimated development time module: preload priority: medium Issues which are important, but no one will go out of business. type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants