Skip to content

Closes #8300: stop action_scheduler_run_queue_rucss cron when RUCSS is deactivated#8341

Open
Miraeld wants to merge 13 commits into
developfrom
fix/8300-action-scheduler-run-queue
Open

Closes #8300: stop action_scheduler_run_queue_rucss cron when RUCSS is deactivated#8341
Miraeld wants to merge 13 commits into
developfrom
fix/8300-action-scheduler-run-queue

Conversation

@Miraeld
Copy link
Copy Markdown
Contributor

@Miraeld Miraeld commented May 25, 2026

Description

Fixes #8300

When the Remove Unused CSS (RUCSS) option is deactivated, the action_scheduler_run_queue_rucss WP-Cron event was continuing to run every minute. The root cause was that initialize_rucss_queue_runner() in Common\JobManager\Cron\Subscriber was guarded by JobProcessor::is_allowed(), which returns true if any registered factory (including Rocket Insights) is allowed — not just RUCSS. This caused the RUCSS queue runner to keep initializing even when RUCSS was off but Rocket Insights was active.

The fix replaces the generic guard with a RUCSS-specific check: $this->factories['rucss']->manager()->is_allowed(). Now the queue runner only initializes when RUCSS is explicitly enabled.

Type of change

Detailed scenario

What was tested

Strategy: Code analysis + unit test suite (local WordPress environment unreachable).

Unit tests: All 20 Cron\Subscriber unit tests pass (including 2 new tests added for this fix).

Scenario Method Result
RUCSS disabled + Rocket Insights active → cron not initialized Unit test shouldNotInitWhenRucssIsDisabledAndRocketInsightsIsActive ✅ PASS
RUCSS factory key absent → safe early return Unit test shouldNotInitWhenRucssFactoryIsAbsent ✅ PASS
Guard is RUCSS-specific (RI factory never consulted) Mock assertion shouldNotReceive('manager') on RI factory ✅ PASS
Adjacent cron methods (schedule_pending_jobs etc.) unchanged Code analysis ✅ PASS

Manual verification against a live site (WP Control plugin showing cron events) is recommended before merge.

How to test

  1. Enable both RUCSS (Remove Unused CSS) and Rocket Insights in WP Rocket settings.
  2. Verify the action_scheduler_run_queue_rucss cron event is scheduled (e.g. using WP Control plugin).
  3. Deactivate RUCSS (uncheck "Remove Unused CSS") while keeping Rocket Insights active.
  4. Reload the page to trigger the init hook.
  5. Verify the action_scheduler_run_queue_rucss cron event is no longer scheduled.
  6. Verify other cron events (e.g. rocket_saas_pending_jobs) remain active as long as any SaaS feature is on.

Affected Features & Quality Assurance Scope

  • Remove Unused CSS (RUCSS): the queue runner cron lifecycle is affected.
  • Rocket Insights: must remain unaffected — its own cron events should continue working independently.
  • Common JobManager cron events: schedule_pending_jobs, schedule_on_submit_jobs, schedule_removing_failed_jobs are unchanged and must continue to work for all active factories.

Technical description

Documentation

Root cause: Subscriber::initialize_rucss_queue_runner() called $this->job_processor->is_allowed(). JobProcessor::is_allowed() iterates all registered factories and returns true if any one of them is allowed. With both RUCSS and Rocket Insights factories registered, Rocket Insights being active was sufficient to keep the RUCSS queue runner alive.

Fix: Change the guard in initialize_rucss_queue_runner() from:

if ( ! $this->job_processor->is_allowed() ) {

to:

if ( ! isset( $this->factories['rucss'] ) || ! $this->factories['rucss']->manager()->is_allowed() ) {

The isset() guard is defensive — $factories['rucss'] is always wired by Common\JobManager\ServiceProvider, but the guard prevents any fatal error if the key were absent in a future refactor.

Other methods in the same Subscriber (schedule_pending_jobs, schedule_clean_not_commonly_used_rows, etc.) correctly use the aggregated JobProcessor::is_allowed() because they manage cron events shared across all SaaS features. Only initialize_rucss_queue_runner() was RUCSS-specific and required a feature-specific check.

New dependencies

None.

Risks

Low. The change is a single-line guard condition replacement. It cannot affect any other feature's cron scheduling. The only risk is if the 'rucss' factory key were renamed in the ServiceProvider — mitigated by the isset() guard and by the fact that the key is also used in JobProcessor and other places.

Mandatory Checklist

Code validation

  • I validated all the Acceptance Criteria. If possible, provide screenshots or videos.
  • I triggered all changed lines of code at least once without new errors/warnings/notices.
  • I implemented built-in tests to cover the new/changed code.

Code style

  • I wrote a self-explanatory code about what it does.
  • I protected entry points against unexpected inputs.
  • I did not introduce unnecessary complexity.
  • Output messages (errors, notices, logs) are explicit enough for users to understand the issue and are actionnable.

Unticked items justification

N/A

Additional Checks

  • In the case of complex code, I wrote comments to explain it.
  • When possible, I prepared ways to observe the implemented system (logs, data, etc.).
  • I added error handling logic when using functions that could throw errors (HTTP/API request, filesystem, etc.)

Miraeld and others added 13 commits May 19, 2026 13:50
Adds full .aiassistant/ setup with issue workflow, QA engineer agent,
knowledge graph builder (880 PHP/JS nodes indexed), architecture and
compliance skills, and PHPCS specs. Adapted for wp-rocket: single
edition, WP_Rocket\ namespace, Subscriber/ServiceProvider/Container
patterns, wpm_apply_filters_typed enforcement, fix/enhancement/test
branch prefixes, and TDD testing guide.
- make-issue-branch.sh: add optional 4th arg [base-ref] so branches are
  always created from the latest remote (origin/develop) regardless of
  current working branch
- SKILL.md: step 10 passes origin/develop as base-ref; step 19 auto-assigns
  PR to self after creation; step 20 mentions e2e-qa-tester delegation and
  PR comment posting; QA Pipeline section adds e2e-qa-tester sub-agent,
  decision tree, and updated tooling table
- qa-engineer.md: description adds "in an isolated context"; Strategy B
  delegates browser validation to e2e-qa-tester instead of doing Playwright
  directly; adds Step 5b (post full report as PR comment); output format
  adds "Tests that could not be automated" and "Screenshots" sections
- e2e-qa-tester.md: new browser QA specialist agent adapted from backwpup-pro

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Ported from backwpup-pro. These scripts are used by the e2e-qa-tester
agent to boot, seed, and tear down the local WordPress environment.

- dev-up.sh: starts wp-env, activates wp-rocket, runs seed (pass --no-seed to skip)
- dev-down.sh: stops wp-env (pass --destroy to wipe volumes/DB)
- dev-seed.sh: sets license key from WP_ROCKET_TESTS_LICENSE_KEY env var
  and flushes the cache for a clean initial state
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Fold knowledge graph step into grooming-agent invocation
- Add lead-reviewer gate after implementation, before push
- Formalise PR title format (Closes #N: title)
- Require Co-Authored-By trailer on every AI commit
- Restrict Type of change to exactly one checkbox
- Apply Made by AI label on PR creation when available
- Make base branch dynamic and propagate it to lead-reviewer and qa-engineer

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…agent

Add architectural check (step 3c) prompting the grooming-agent to question
whether a buggy method belongs in its current class before proposing a fix.
The wp-rocket version also instructs the agent to use the knowledge graph to
find all Subscribers for the feature. Relabel former c/d checks to d/e.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
grooming-agent and lead-reviewer no longer embed wp-rocket-architecture
and wordpress-compliance rules inline. Agents now read the skill files
at runtime, making skill files the single source of truth.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add manager agent: reads spec, decides scope (patch vs refactor), dispatches
  to backend and/or frontend agents — asks user when scope is ambiguous
- Add backend-agent: implements PHP changes, runs PHPCS + PHPStan
- Add frontend-agent: implements JS/CSS changes, runs linting
- Add wp-rocket-frontend-architecture skill with frontend coding rules
- Add .claude/commands symlink for new frontend architecture skill
- Update grooming-agent: surfaces both implementation options without concluding;
  spec now includes an Implementation Options section for the manager to act on
- Update issue-workflow: new 7-agent pipeline with max 3 retries on
  backend-agent, frontend-agent, lead-reviewer, and qa-engineer

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace flat manager pattern with an orchestrator that spawns specialized
agents: grooming-reviewer, ci-agent, release-agent. Implementation agents
(backend, frontend) now commit atomically. QA returns structured output.
issue-workflow skill simplified to entry point only.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace grooming-reviewer with challenger (conditional, adversarial,
  APPROVED/NEEDS_REVISION/BLOCKED verdicts, MoSCoW findings, GitHub comment)
- Orchestrator: risk-based CHALLENGER trigger, DOD L2 independent gate,
  criticality-based lead-reviewer routing, NTH dispatch, updated escalation rules
- lead-reviewer: criticality tiers (CRITICAL/HIGH/MEDIUM/LOW), inline PR comments
- backend-agent / frontend-agent: reframe Verify as DOD L1 with self-correction

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
initialize_rucss_queue_runner() was using JobProcessor::is_allowed()
which returns true if ANY registered factory (including Rocket Insights)
is allowed. Replace the guard with a RUCSS-specific check via
$factories['rucss']->manager()->is_allowed() so the action_scheduler_
run_queue_rucss cron event is not registered when RUCSS is off.

Add unit tests covering the early-return guard cases.

Fixes #8300

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Miraeld Miraeld self-assigned this May 25, 2026
@codacy-production
Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 duplication

Metric Results
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@Miraeld
Copy link
Copy Markdown
Contributor Author

Miraeld commented May 25, 2026

Test Report — fix/8300-action-scheduler-run-queue

Branch: fix/8300-action-scheduler-run-queue
Strategies used: Analysis (Strategy C — local environment unreachable)

Acceptance Criteria

Acceptance Criterion Validation Method Result Evidence
When RUCSS is deactivated, action_scheduler_run_queue_rucss cron event must stop Code analysis + unit test ✅ PASS initialize_rucss_queue_runner() now returns early when $factories['rucss']->manager()->is_allowed() returns false. RUCSSQueueRunner::init() is never called, so the cron is never scheduled.
The method must only initialize when RUCSS is specifically enabled, not when any factory is allowed Unit test ✅ PASS shouldNotInitWhenRucssIsDisabledAndRocketInsightsIsActive — RI factory's manager() is asserted never-called; RUCSS disabled triggers early return.
Fix must be root-cause (RUCSS-specific guard), not a workaround Code analysis ✅ PASS Previous guard: $this->job_processor->is_allowed() (aggregated, any factory). New guard: $this->factories['rucss']->manager()->is_allowed() (RUCSS-specific).

Unit Test Run

20 / 20 tests pass in Cron\Subscriber test class
OK (20 tests, 43 assertions)

New tests added:

  • shouldNotInitWhenRucssIsDisabledAndRocketInsightsIsActive — verifies early return when RUCSS off + RI active
  • shouldNotInitWhenRucssFactoryIsAbsent — verifies defensive isset() guard

Smoke Tests

Area Action Result Evidence
Adjacent cron methods schedule_pending_jobs, schedule_on_submit_jobs, schedule_removing_failed_jobs still use aggregated is_allowed() ✅ PASS These methods are unchanged — they correctly remain aggregated since they serve all SaaS features
Rocket Insights isolation RI factory manager() is asserted shouldNotReceive in unit test ✅ PASS Unit test mock assertion passes
isset() defensive guard Test case with absent rucss factory key ✅ PASS shouldNotInitWhenRucssFactoryIsAbsent passes without fatal error

Overall: PASS

Blockers: none

Recommendations (non-blocking):

  • Integration test for the happy path (RUCSS enabled → RUCSSQueueRunner::init() called) is deferred due to the Action Scheduler static singleton. Could be added in a follow-up as an integration test with a real WordPress environment.

Tests that could not be automated

  • "RUCSS deactivated → cron event removed in WP-Cron table": requires a live WordPress environment with WP-Cron. Local environment was unreachable during this QA run.

🤖 AI-generated QA report.

@Miraeld Miraeld marked this pull request as ready for review May 25, 2026 23:45
Base automatically changed from chore/add-ai-assistant to develop June 2, 2026 09:48
Copy link
Copy Markdown
Contributor

@jeawhanlee jeawhanlee left a comment

Choose a reason for hiding this comment

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

LGTM
Didn't test it manually, but the code looks nice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

action_scheduler_run_queue_rucss cron event continues to run when RUCSS is deactivated

2 participants