fix: make network rescans non-blocking - #65
Conversation
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe change adds scheduling-horizon and scan-timeout configuration, horizon-aware job filtering, registry-driven full-network scans, asynchronous subprocess handling, shutdown cleanup, and regression coverage. ChangesFull-network scanning
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Worker_Process
participant scan_cron.php
participant RegistrySites
participant IsolatedScanner
Worker_Process->>scan_cron.php: start full-network scan
scan_cron.php->>RegistrySites: load active registry entries
scan_cron.php->>IsolatedScanner: send URL, horizon, and timeout
IsolatedScanner-->>scan_cron.php: return validated JSON jobs
scan_cron.php-->>Worker_Process: provide jobs for scheduling
Worker_Process->>Worker_Process: poll, schedule, or terminate scan
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (6)
tests/regression.php (1)
814-818: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd scan-timeout expiry coverage.
This fixture only verifies shutdown cleanup. It does not set
scan_timeoutbelow the subprocess duration or poll after its deadline. Add a case that setsscan_timeoutto one second, polls the five-second fixture after expiry, and asserts that the process is terminated and reaped.🤖 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 814 - 818, Extend the regression coverage around Worker_Process::run_full_rescan and scan polling by configuring scan_timeout to one second for the five-second async_scan_fixture, waiting or polling past that deadline, and then asserting active_scan_process is null to confirm the scanner subprocess was terminated and reaped. Keep the existing shutdown cleanup assertion intact.src/class-worker-process.php (1)
1134-1146: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider skipping invalid payloads instead of discarding the whole batch.
One invalid entry causes
schedule_scan_outputto return 0. The worker then discards every valid job from every site in the network scan. Those jobs wait until the next rescan. Inbin/scan-cron.php,qw_scan_registry_jobsapplies the same all-or-nothing rule, but its blast radius is one registry entry. Here it is the whole network.Skipping the bad entry and logging it keeps the remaining jobs schedulable.
♻️ Proposed change to skip invalid entries
$validated_payloads = []; + $invalid = 0; foreach ($payloads as $payload_data) { if (!is_array($payload_data)) { - Worker::log(sprintf('[W%d][RESCAN][FAIL] Full-network scanner returned an invalid payload shape.', $worker_id)); - return 0; + $invalid++; + continue; } try { $validated_payloads[] = new Job_Payload($payload_data); } catch (\Throwable $e) { - Worker::log(sprintf('[W%d][RESCAN][FAIL] Full-network scanner returned invalid routing metadata (%s).', $worker_id, $e->getMessage())); - return 0; + $invalid++; + Worker::log(sprintf('[W%d][RESCAN][WARN] Skipping payload with invalid routing metadata (%s).', $worker_id, $e->getMessage())); } } + + if ($invalid > 0) { + Worker::log(sprintf('[W%d][RESCAN][WARN] Skipped %d invalid payloads.', $worker_id, $invalid)); + }🤖 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 1134 - 1146, Update the payload-validation loop in schedule_scan_output to log and skip invalid non-array payloads and Job_Payload construction failures instead of returning 0. Continue appending valid payloads to validated_payloads so they remain schedulable, while preserving the existing failure messages for each rejected entry.bin/scan-cron.php (4)
174-194: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAccumulate payloads with a nested array instead of
array_mergein the loop.
array_mergecopies the whole accumulated array on every iteration. On a network with many sites this is quadratic in the total payload count. Collecting per-site results and flattening once removes the repeated copying.♻️ Proposed change
+ $payload_groups = []; foreach ($site_ids as $site_id) { @@ $switched = $site_id !== get_current_blog_id(); if ($switched) { switch_to_blog($site_id); } - $payloads = array_merge($payloads, qw_scan_current_site_jobs($scheduling_horizon)); + $payload_groups[] = qw_scan_current_site_jobs($scheduling_horizon); if ($switched) { restore_current_blog(); } } + $payloads = array_merge($payloads, ...$payload_groups);🤖 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 174 - 194, Update the site iteration around qw_scan_current_site_jobs to collect each site’s payload array as a nested element instead of repeatedly calling array_merge on $payloads. After the loop, flatten the collected per-site payloads once while preserving the existing skipped-site, blog-switching, and payload ordering behavior.
396-416: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider
stream_selectinstead of a 10 ms polling loop.The loop wakes 100 times per second for the whole child scan.
stream_selecton the two pipes with a timeout blocks until output arrives and removes the spin. The current form is correct; this is an efficiency point only.Note on the static analysis hint for lines 369-373:
proc_openreceives an array, so no shell is involved and the arguments arePHP_BINARYand__FILE__. The command-injection finding is a false positive.🤖 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 396 - 416, Replace the fixed 10 ms usleep polling in the scan process loop with stream_select on the stdout and stderr pipes, using the remaining scan timeout as its wait limit. Continue reading available output, checking proc_get_status, and terminating on timeout while preserving the existing stdout/stderr handling and process behavior.Source: Linters/SAST tools
281-345: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThese registry readers duplicate the
Worker_Processversions and already diverge.
qw_scan_sovereign_site_entriesandqw_scan_isolated_network_entriesrepeatsovereign_site_entries()andisolated_network_entries()insrc/class-worker-process.php. The predicates are not identical. The worker version requires a non-emptydomainsarray. This version accepts an entry that only setsdomain, because it tests the resolved URL instead. The two components can therefore select different sovereign sites from the same registry file.Move the registry parsing into one shared class under
src/and call it from both places.As per coding guidelines: "Use the
QueueWorker\namespace prefix for classes insrc/."🤖 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 281 - 345, Create a shared namespaced registry-reader class under src/ using the QueueWorker\ namespace, and move the common sovereign-site and isolated-network parsing logic into it. Replace qw_scan_sovereign_site_entries() and qw_scan_isolated_network_entries() in bin/scan-cron.php and the corresponding sovereign_site_entries() and isolated_network_entries() methods in Worker_Process with calls to that class, preserving one consistent domains-array validation and registry filtering behavior.Source: Coding guidelines
239-242: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueMove the deadline check out of the events loop.
$timestampdoes not change insideforeach ($events as $event). The check repeats for every event of the same timestamp. Testing it at theforeach ($crons as $timestamp => $hooks)level skips the whole timestamp bucket in one step.🤖 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 239 - 242, Move the `$timestamp > $deadline` guard from inside the `foreach ($events as $event)` loop to the enclosing `foreach ($crons as $timestamp => $hooks)` level, before events are processed, so an expired timestamp skips its entire bucket once.
🤖 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 `@bin/scan-cron.php`:
- Around line 200-216: Update the registry-entry loops around
qw_scan_registry_jobs to share the overall scan deadline: track the remaining
total budget and derive a per-entry timeout from it before each call, rather
than passing the full $scan_timeout for every entry. Apply the same deadline
logic to both $sovereign_sites and $isolated_networks, while preserving the
existing payload aggregation and ensuring the timeout never becomes invalid.
In `@tests/regression.php`:
- Around line 601-623: Make the beyond_horizon_hook fixture in the
schedule_timer test deterministic by placing its timestamp safely beyond the
60-second scheduling_horizon, rather than only one second past it. Keep the
within_horizon_hook timestamp and pending_timers assertion unchanged.
---
Nitpick comments:
In `@bin/scan-cron.php`:
- Around line 174-194: Update the site iteration around
qw_scan_current_site_jobs to collect each site’s payload array as a nested
element instead of repeatedly calling array_merge on $payloads. After the loop,
flatten the collected per-site payloads once while preserving the existing
skipped-site, blog-switching, and payload ordering behavior.
- Around line 396-416: Replace the fixed 10 ms usleep polling in the scan
process loop with stream_select on the stdout and stderr pipes, using the
remaining scan timeout as its wait limit. Continue reading available output,
checking proc_get_status, and terminating on timeout while preserving the
existing stdout/stderr handling and process behavior.
- Around line 281-345: Create a shared namespaced registry-reader class under
src/ using the QueueWorker\ namespace, and move the common sovereign-site and
isolated-network parsing logic into it. Replace qw_scan_sovereign_site_entries()
and qw_scan_isolated_network_entries() in bin/scan-cron.php and the
corresponding sovereign_site_entries() and isolated_network_entries() methods in
Worker_Process with calls to that class, preserving one consistent domains-array
validation and registry filtering behavior.
- Around line 239-242: Move the `$timestamp > $deadline` guard from inside the
`foreach ($events as $event)` loop to the enclosing `foreach ($crons as
$timestamp => $hooks)` level, before events are processed, so an expired
timestamp skips its entire bucket once.
In `@src/class-worker-process.php`:
- Around line 1134-1146: Update the payload-validation loop in
schedule_scan_output to log and skip invalid non-array payloads and Job_Payload
construction failures instead of returning 0. Continue appending valid payloads
to validated_payloads so they remain schedulable, while preserving the existing
failure messages for each rejected entry.
In `@tests/regression.php`:
- Around line 814-818: Extend the regression coverage around
Worker_Process::run_full_rescan and scan polling by configuring scan_timeout to
one second for the five-second async_scan_fixture, waiting or polling past that
deadline, and then asserting active_scan_process is null to confirm the scanner
subprocess was terminated and reaped. Keep the existing shutdown cleanup
assertion intact.
🪄 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: e6051e1a-dc2f-440f-ad70-32683e5b2e8b
📒 Files selected for processing (6)
README.mdbin/scan-cron.phpbin/worker.phpsrc/class-config.phpsrc/class-worker-process.phptests/regression.php
superdav42
added
the
review-feedback-scanned
1 participant
Summary
Configuration
QUEUE_WORKER_SCAN_TIMEOUTdefaults to300secondsQUEUE_WORKER_SCHEDULING_HORIZONdefaults to3600seconds and is clamped to at least the rescan intervalVerification
composer test:regressionphp -l src/class-worker-process.phpphp -l src/class-config.phpphp -l bin/scan-cron.phpphp -l tests/regression.phpcomposer validate --strictRegression coverage includes scheduling-horizon filtering, asynchronous scan startup, overlap prevention, successful polling, malformed output, timeouts, and process cleanup.
aidevops.sh v3.32.245 plugin for OpenCode v1.18.9 with gpt-5.5 spent 3h 35m and 1,547,684 tokens on this with the user in an interactive session.
Summary by CodeRabbit
New Features
Documentation