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

get_failed_rows date query should match the value of the rocket_remove_rucss_failed_jobs_cron_interval filter #6066

Closed
4 tasks
alfonso100 opened this issue Jul 25, 2023 · 5 comments · Fixed by #6087
Assignees
Labels
effort: [S] 1-2 days of estimated development time module: remove unused css needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. 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

@alfonso100
Copy link
Contributor

Before submitting an issue please check that you’ve completed the following steps:
Yes - Made sure you’re on the latest version
Yes - Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
We can filter the rocket_remove_rucss_failed_jobs_cron_interval and set the recurrence of WP Rocket clear Remove Unused CSS failed jobs to 1 hour for example.

This should allow to re-process the failed jobs faster.

But in the plugin code, the get_failed_rows function has the modified date query hardcoded to 3 days ago
https://github.com/wp-media/wp-rocket/blob/develop/inc/Engine/Optimization/RUCSS/Database/Queries/UsedCSS.php#L423

The result is no failed row "younger" than 3 days wil be set to pending.

To Reproduce
Steps to reproduce the behavior:

  1. On a site with failed RUCSS jobs
  2. Install this helper plugin to set the rocket_remove_rucss_failed_jobs_cron_interval to 1 hour
  3. Deactivate/reactivate RUCSS so the Event schedule is updated.
  4. Wait for the job to run, or manually execute it using WP Crontrol.
  5. See no failed job older than 3 days reset to pending

Expected behavior
The get_failed_rows function should be aware of the value of the rocket_remove_rucss_failed_jobs_cron_interval filter so the query is dynamic and not static to "3 days ago".

Additional context
Slack threads:
https://wp-media.slack.com/archives/C029G1B5HD2/p1690205678240629
https://wp-media.slack.com/archives/C08N8J6VC/p1690270849029839

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
DahmaniAdame added a commit to wp-media/wp-rocket-helpers that referenced this issue Jul 26, 2023
Added temporary manual failed jobs clearing + cache clearing until wp-media/wp-rocket#6066 is sorted out. 

Once done, using `rocket_remove_rucss_failed_jobs` will be enough.
@piotrbak
Copy link
Contributor

piotrbak commented Jul 27, 2023

Acceptance Criteria:

  1. We need to be able to filter the default value of 3 days here https://github.com/wp-media/wp-rocket/blob/develop/inc/Engine/Optimization/RUCSS/Database/Queries/UsedCSS.php#L423
  2. The filter should allow us to define the schedule in hours, minutes or seconds
  3. The code should be guarded against the wrong or weird values inside the filter like null, strings, etc. and don't cause fatal errors
  4. WP Rocket will be reverting failed RUCSS jobs older than specified time with the rocket_remove_rucss_failed_jobs cron without any regressions
  5. The Cron will be able to create Unused CSS for the reverted jobs (currently there's a problem related to jobs ID not being erased + something's still not working as expected)

@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement priority: medium Issues which are important, but no one will go out of business. module: remove unused css needs: grooming labels Jul 27, 2023
@CrochetFeve0251
Copy link
Contributor

CrochetFeve0251 commented Jul 31, 2023

Reproduce the problem

The problem was easy to reproduce using instructions.

Identify the root cause

The root cause is an hard-coded constant inside https://github.com/wp-media/wp-rocket/blob/develop/inc/Engine/Optimization/RUCSS/Database/Queries/UsedCSS.php#L423 which prevent us from changing the behavior.

Scope a solution

For that solution we could inspire from what was done to solve a similar issue on the preload:
https://github.com/wp-media/wp-rocket/blob/219a0989dc6ec41db29b47ac38f0869da57a3e86/inc/Engine/Preload/Cron/Subscriber.php#L238C37-L238C77

This is particularly good example as the implementation from this one created a discussion on how we should organize ourselves concerning magic constants inside query classes.

First inside the clear_failed_urls method from the UsedCSS controller we will have to call a new filter rocket_delay_remove_rucss_failed_jobs which takes as parameter 3 days.

Then the result need to be validated (cast to string, not empty and both parts existing) and then passed to the method get_failed_rows from the query:

public function get_failed_rows( float $delay = 3, string $unit = 'days') {

if ( ! self::$table_exists && ! $this->table_exists() ) {
			return false;
		}

		$query = $this->query(
			[
				'status'     => 'failed',
				'date_query' => [
					[
						'column'    => 'modified',
						'before'    => $delay $unit ago",
						'inclusive' => true,
					],
				],
			],
			false
		);

Estimate the effort

Effort S

@CrochetFeve0251 CrochetFeve0251 added the effort: [S] 1-2 days of estimated development time label Jul 31, 2023
@Tabrisrp
Copy link
Contributor

looks good

@mostafa-hisham mostafa-hisham self-assigned this Aug 2, 2023
@piotrbak piotrbak added needs: grooming needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. and removed effort: [S] 1-2 days of estimated development time labels Aug 13, 2023
@mostafa-hisham
Copy link
Contributor

Reproduce the problem

The problem was easy to reproduce using instructions.

Identify the root cause

We found out that changing failed jobs status to pending will not give them a new Job ID which means it is going to try to fetch the same failed data in this case 400 bad json 3 more times

Scope a solution

we should send the URL again to saas to get a new jobId and change the status to pending so we can fetch the result for the saas new job

in clear_failed_urls functions we should do the following

			$add_to_queue_response = $this->add_url_to_the_queue( $row->url, (bool) $row->is_mobile );
			if ( false !== $add_to_queue_response ) {
				$new_job_id = $add_to_queue_response['contents']['jobId'];
				$this->used_css_query->update_job_id( $id, $new_job_id );
			}
			$this->used_css_query->revert_to_pending( $id );

we should use this function make_status_pending instead of doing 2 queries to set the new job id and change status to pending but we need to edit the make_status_pending to reset the error and retries number

				'error_code'    => '',
				'error_message' => '',
				'retries'       => 0,	

Estimate the effort

Effort S

@mostafa-hisham mostafa-hisham added effort: [S] 1-2 days of estimated development time and removed needs: grooming labels Aug 14, 2023
@Tabrisrp
Copy link
Contributor

Should we create a new method like reset_job() instead of doing 2 calls? The method would be a mix of updating and make_status_pending

@piotrbak piotrbak added this to the 3.15.1 milestone Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort: [S] 1-2 days of estimated development time module: remove unused css needs: r&d Needs research and development (R&D) before a solution can be proposed and scoped. 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
5 participants