fix: schedule cron for isolated networks - #60
Conversation
📝 WalkthroughWalkthroughThe change adds isolated-network routing metadata to job payloads, validates routing during scanning and execution, and extends worker rescans to isolated-network registries. It also adds socket-based WP-Cron health status handling, regression coverage, and explicit pnpm setup in CI. ChangesIsolated network routing
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker_Process
participant scan_cron as bin/scan-cron.php
participant Job_Payload
participant execute_job as bin/execute-job.php
Worker_Process->>scan_cron: request isolated-network scan
scan_cron->>Job_Payload: create routed payload
Job_Payload-->>scan_cron: return validated routing metadata
scan_cron->>execute_job: provide job payload
execute_job->>Job_Payload: validate routing metadata
execute_job-->>Worker_Process: execute or reject job
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 PHPStan (2.2.7)PHPStan was skipped because the config uses disallowed Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/class-worker-process.php (1)
1066-1098: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a timeout to the registry scan subprocess.
rescan_registry_jobsreads the child stdout with a blockingstream_get_contentsand then callsproc_close. No timeout bounds that read. If a tenant scan hangs, for example on a slow tenant database, the coordinator worker event loop blocks indefinitely. The batch flush timer and the subprocess poll timer stop firing while the loop is blocked.This path now runs once per sovereign site and once per isolated network, so the total blocking window grows with the number of registered isolated networks.
Set the pipes to non-blocking, then read with a deadline and terminate the child when the deadline passes.
🛡️ Sketch of a bounded read
- $stdout = stream_get_contents($pipes[1]); - $stderr = stream_get_contents($pipes[2]); - fclose($pipes[1]); - fclose($pipes[2]); - $exit_code = proc_close($process); + stream_set_blocking($pipes[1], false); + stream_set_blocking($pipes[2], false); + $stdout = ''; + $stderr = ''; + $deadline = time() + $this->batch_timeout; + while (true) { + $stdout .= (string) stream_get_contents($pipes[1]); + $stderr .= (string) stream_get_contents($pipes[2]); + $status = proc_get_status($process); + if (!$status['running']) { + break; + } + if (time() >= $deadline) { + proc_terminate($process, 9); + Worker::log(sprintf( + '[RESCAN][%s][FAIL] %s %d scan timed out', + $registry_kind, + ucfirst($subject), + $registry_id + )); + break; + } + usleep(50000); + } + fclose($pipes[1]); + fclose($pipes[2]); + $exit_code = proc_close($process);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/class-worker-process.php` around lines 1066 - 1098, Update rescan_registry_jobs to bound subprocess I/O: configure the stdin/stdout/stderr pipes as non-blocking, poll their streams until completion or a fixed deadline, and terminate the child when the deadline expires before closing the process. Preserve the existing scan payload and logging behavior while ensuring a hung tenant scan cannot block the worker event loop indefinitely.
🧹 Nitpick comments (5)
src/class-job-payload.php (1)
7-7: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the constant to use the
QW_prefix.The coding guidelines require the
QW_prefix for plugin constants.Worker_Processalready follows this for class constants (QW_CRON_SITE_LOCK_TTL_SECONDS,QW_CRON_SITE_LOCK_REFRESH_SECONDS).♻️ Proposed rename
- private const ISOLATED_SITE_FACTOR = 4294967296; + private const QW_ISOLATED_SITE_FACTOR = 4294967296;Update the usage at Line 180 accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/class-job-payload.php` at line 7, Rename the class constant ISOLATED_SITE_FACTOR to use the required QW_ prefix, and update its reference in Worker_Process at the indicated usage so the code continues using the renamed constant.Source: Coding guidelines
bin/scan-cron.php (1)
192-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the unused loop value.
$actionis never read inside the loop body. PHPMD reports it as an unused local variable. Iterate over the keys instead.♻️ Proposed change
- foreach ($actions as $action_id => $action) { + foreach (array_keys($actions) as $action_id) { $action_payload = Job_Payload::from_as_action($action_id);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@bin/scan-cron.php` around lines 192 - 197, Update the foreach loop around Job_Payload::from_as_action to iterate only over the action IDs, removing the unused $action value while preserving the existing payload generation behavior.Source: Linters/SAST tools
tests/regression.php (1)
746-750: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the network registry fixture after the isolated tests.
The fixture file stays on disk for the rest of the run. The later full rescan at Line 1146 still discovers isolated network 12. That worker has no scan script, so it logs an error and schedules nothing, and the assertion at Line 1148 still passes. The pass depends on that indirect behavior, not on isolated state.
♻️ Proposed cleanup after Line 813
assert_true(unlink($isolated_scan_fixture), 'Isolated scan fixture must be removed'); + assert_true(unlink(WP_CONTENT_DIR . '/network-registry.data.json'), 'Network registry fixture must be removed'); \Workerman\Timer::$delays = [];Keep the fixture in place if a later test needs it, and assert its effect explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/regression.php` around lines 746 - 750, Remove the network-registry fixture created by the final isolated test after the test block ending near the cleanup point after line 813, before later full-rescan tests run. Ensure the cleanup targets WP_CONTENT_DIR . '/network-registry.data.json' and preserves the existing assertions for the isolated tests.src/class-worker-process.php (2)
1209-1227: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNormalize and validate the registry domain.
The method concatenates the registry value into
https://<domain>/without validation. If a registry entry contains a scheme, a path, a port with a stray character, or trailing whitespace inside the string, the resulting URL is malformed and the tenant scan fails with an opaque bootstrap error.
Job_Payload::current_mapped_site_urlinsrc/class-job-payload.phpalready validates a domain with/^[a-z0-9.-]+(?::[0-9]+)?$/. Apply the same rule here.♻️ Proposed validation
foreach ($domains as $domain) { - $domain = trim((string) $domain); - if ($domain !== '') { - return 'https://' . $domain . '/'; - } + $domain = strtolower(rtrim(trim((string) $domain), '.')); + if ($domain === '' || preg_match('/^[a-z0-9.-]+(?::[0-9]+)?$/', $domain) !== 1) { + continue; + } + + return 'https://' . $domain . '/'; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/class-worker-process.php` around lines 1209 - 1227, Update site_url_from_registry_entry to normalize each candidate domain by trimming whitespace, lowercasing it, and removing any trailing dot before validation. Apply the same /^[a-z0-9.-]+(?::[0-9]+)?$/ rule used by Job_Payload::current_mapped_site_url; skip invalid values, and only construct the HTTPS URL for a validated domain.
1147-1194: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the diagnostic helper and replace the string branch.
The validation flow is correct and fails closed. Two naming points remain after the generalization:
sovereign_scan_diagnosticnow serves both registry kinds. Rename it toregistry_scan_diagnostic.- Line 1182 branches on the
'ISOLATED'string literal. Pass an explicit flag or an enum instead, so a typo in a future caller cannot silently skip the routing check.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/class-worker-process.php` around lines 1147 - 1194, Rename the diagnostic helper sovereign_scan_diagnostic to registry_scan_diagnostic and update all references in the registry scan validation flow. Replace the literal ISOLATED comparison in the routing check with an explicit boolean flag or enum representing whether isolated-network routing validation is required, ensuring future callers cannot bypass the check through a typo.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/assets.yml:
- Line 18: Update the action references in the assets workflow, including
pnpm/action-setup and actions/setup-node, to releases that declare using:
node24. Replace both current `@v4` references with their Node 24-compatible
versions while preserving the existing workflow steps and configuration.
---
Outside diff comments:
In `@src/class-worker-process.php`:
- Around line 1066-1098: Update rescan_registry_jobs to bound subprocess I/O:
configure the stdin/stdout/stderr pipes as non-blocking, poll their streams
until completion or a fixed deadline, and terminate the child when the deadline
expires before closing the process. Preserve the existing scan payload and
logging behavior while ensuring a hung tenant scan cannot block the worker event
loop indefinitely.
---
Nitpick comments:
In `@bin/scan-cron.php`:
- Around line 192-197: Update the foreach loop around
Job_Payload::from_as_action to iterate only over the action IDs, removing the
unused $action value while preserving the existing payload generation behavior.
In `@src/class-job-payload.php`:
- Line 7: Rename the class constant ISOLATED_SITE_FACTOR to use the required QW_
prefix, and update its reference in Worker_Process at the indicated usage so the
code continues using the renamed constant.
In `@src/class-worker-process.php`:
- Around line 1209-1227: Update site_url_from_registry_entry to normalize each
candidate domain by trimming whitespace, lowercasing it, and removing any
trailing dot before validation. Apply the same /^[a-z0-9.-]+(?::[0-9]+)?$/ rule
used by Job_Payload::current_mapped_site_url; skip invalid values, and only
construct the HTTPS URL for a validated domain.
- Around line 1147-1194: Rename the diagnostic helper sovereign_scan_diagnostic
to registry_scan_diagnostic and update all references in the registry scan
validation flow. Replace the literal ISOLATED comparison in the routing check
with an explicit boolean flag or enum representing whether isolated-network
routing validation is required, ensuring future callers cannot bypass the check
through a typo.
In `@tests/regression.php`:
- Around line 746-750: Remove the network-registry fixture created by the final
isolated test after the test block ending near the cleanup point after line 813,
before later full-rescan tests run. Ensure the cleanup targets WP_CONTENT_DIR .
'/network-registry.data.json' and preserves the existing assertions for the
isolated tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dee5b892-44db-4850-8fbf-39e93364ac0c
📒 Files selected for processing (8)
.github/workflows/assets.ymlbin/execute-job.phpbin/scan-cron.phpsrc/class-job-payload.phpsrc/class-socket-client.phpsrc/class-worker-process.phptests/regression.phpthe-perfect-wp-cron.php
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - uses: pnpm/action-setup@v4 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for action in pnpm/action-setup/v4 actions/setup-node/v4; do
echo "== $action =="
curl -fsSL "https://raw.githubusercontent.com/$action/action.yml" | rg -n 'using:'
doneRepository: Ultimate-Multisite/the-perfect-wp-cron
Length of output: 271
🌐 Web query:
GitHub Actions hosted runners Node 24 default June 16 2026 action-runner node24 official
💡 Result:
GitHub Actions transitioned to using Node.js 24 as the default runtime for hosted runners on June 16, 2026 [1][2][3]. Key details regarding this migration include: Default Version Change: Starting June 16, 2026, GitHub Actions runners began defaulting to Node 24 for JavaScript actions [1][3]. Opt-Out Mechanism: Users who were not ready to migrate could temporarily opt out of this change by setting the environment variable ACTIONS_ALLOW_USE_UNSECURE_NODE_VERSION=true in their workflow or on the runner machine [1][3]. This opt-out mechanism was intended as a temporary measure until Node 20 support was fully removed from the runners [1]. Future Deprecation: Following the transition to Node 24, Node 20 was scheduled to be completely removed from GitHub-hosted runners in the fall of 2026 (specifically identified as September 16, 2026, in related documentation and issue tracking) [1][3]. Testing/Forced Usage: Prior to the June 16 default date, users were encouraged to test their actions on Node 24 by setting the environment variable FORCE_JAVASCRIPT_ACTIONS_TO_NODE24=true [1][3]. The scheduling of this date was managed via the actions/runner repository, with the final fallback date of June 16, 2026, being codified in the runner's constants [2][4].
Citations:
- 1: https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/
- 2: Update Node 24 default date to June 16th, 2026 actions/runner#4462
- 3: Node.js 20 actions are deprecated podman-desktop/podman-desktop#17802
- 4: https://github.com/actions/runner/releases
Update these action refs to Node 24-compatible versions.
pnpm/action-setup@v4 and actions/setup-node@v4 both target node20, but GitHub-hosted runners default to Node 24 from June 16, 2026. Point these refs to versions that ship using: node24, or the workflow may fail on current runners.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/assets.yml at line 18, Update the action references in the
assets workflow, including pnpm/action-setup and actions/setup-node, to releases
that declare using: node24. Replace both current `@v4` references with their Node
24-compatible versions while preserving the existing workflow steps and
configuration.
|
DISPATCH_CLAIM nonce=053afe0cfed4023f3d9d9a0254834f77 runner=superdav42 ts=2026-08-07T18:19:35Z max_age_s=120 version=3.32.232 opencode_version=1.18.9 lease_token=053afe0cfed4023f3d9d9a0254834f77 device=device-1783824528-2609248-26808 session=issue-60 phase=prelaunch expires_at=1786126897 |
|
REVIEW_FOLLOWUP_CREATED source_pr=60 issue=63 fingerprint=source-pr-60 runner=superdav42 ts=2026-08-07T18:19:58Z |
superdav42
added
the
review-feedback-scanned
1 participant
Summary
Problem
The queue worker scanned control-plane sites and sovereign site-registry entries, but not legacy isolated-network databases. Cron events in those tenant databases could therefore remain overdue indefinitely. The existing asset workflow also requested pnpm caching before pnpm was installed, causing every PR build to stop during setup-node.
Verification
php -lon all changed PHP filesphp tests/regression.phpgit diff --checkThe regression suite covers isolated-network discovery, proxy skipping, composite identity round trips, tenant-local site scans, route validation, and external scheduler status.
Summary by CodeRabbit
New Features
Bug Fixes
Tests