fix(cron): only update geoip timestamp on successful download#185
fix(cron): only update geoip timestamp on successful download#185parhumm merged 6 commits intodevelopmentfrom
Conversation
The cron callback updated slimstat_last_geoip_dl unconditionally after updateDatabase(), while the AJAX path correctly checked the return value. A transient download failure would suppress retries until the next monthly window. Now checks $ok before updating, matching the AJAX handler behavior.
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe GeoIP update now records the last-download timestamp only when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Test 4 stubs HTTP with WP_Error so updateDatabase() returns false, then asserts slimstat_last_geoip_dl remains null — proving failed downloads don't suppress retry attempts.
![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
🧹 Nitpick comments (1)
wp-slimstat.php (1)
1267-1269: Use a strict success check before persisting the timestamp.Line 1269 currently treats any truthy return as success. Tighten this to strict boolean success (
true ===) so future provider contract drift cannot incorrectly advanceslimstat_last_geoip_dl.🔧 Small hardening diff
- $ok = $geographicProvider->updateDatabase(); + $ok = ( true === $geographicProvider->updateDatabase() ); - if ($ok) { + if ( true === $ok ) { update_option('slimstat_last_geoip_dl', time()); }As per coding guidelines, "Use Yoda Conditions: if ( 'value' === $var )".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wp-slimstat.php` around lines 1267 - 1269, The code currently treats any truthy return from $geographicProvider->updateDatabase() as success before persisting slimstat_last_geoip_dl; change the conditional to a strict boolean comparison using Yoda style (true === $geographicProvider->updateDatabase()) so only an explicit true advances slimstat_last_geoip_dl, ensuring provider contract drift or non-boolean truthy values won't update the timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@wp-slimstat.php`:
- Around line 1267-1271: The GeoIP update path
($geographicProvider->updateDatabase()) currently only updates
slimstat_last_geoip_dl on success, leaving the timestamp stale on failure and
causing repeated blocking attempts in the cron-disabled fallback; change the
failure handling so a retry/backoff is recorded: when updateDatabase() fails,
set a retry timestamp (either update_option('slimstat_last_geoip_dl', time() +
BACKOFF_SECONDS) or a dedicated option like 'slimstat_geoip_retry_after') and
implement exponential/backoff logic (e.g., increase BACKOFF_SECONDS on
consecutive failures using a failure count option) so admin/index.php’s fallback
will skip attempts until the retry timestamp has passed instead of retrying on
every page load.
---
Nitpick comments:
In `@wp-slimstat.php`:
- Around line 1267-1269: The code currently treats any truthy return from
$geographicProvider->updateDatabase() as success before persisting
slimstat_last_geoip_dl; change the conditional to a strict boolean comparison
using Yoda style (true === $geographicProvider->updateDatabase()) so only an
explicit true advances slimstat_last_geoip_dl, ensuring provider contract drift
or non-boolean truthy values won't update the timestamp.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3a7995a1-e1ca-4400-8337-1e2d72b3b9dd
📒 Files selected for processing (1)
wp-slimstat.php
| $ok = $geographicProvider->updateDatabase(); | ||
|
|
||
| // Set the last update time | ||
| update_option('slimstat_last_geoip_dl', time()); | ||
| if ($ok) { | ||
| update_option('slimstat_last_geoip_dl', time()); | ||
| } |
There was a problem hiding this comment.
Add retry backoff to avoid repeated blocking update attempts when cron is disabled.
With this change, failed updates leave slimstat_last_geoip_dl stale. In the cron-disabled fallback path (admin/index.php Line 300–315), that keeps update-needed true and can retrigger GeoIP downloads on every eligible admin page load. Given provider timeout can be long, this can repeatedly block admin requests.
💡 Proposed mitigation (rate-limit failed attempts)
- $last_update = get_option('slimstat_last_geoip_dl', 0);
- if ($last_update < $this_update) {
+ $last_update = (int) get_option('slimstat_last_geoip_dl', 0);
+ $last_attempt = (int) get_option('slimstat_last_geoip_attempt', 0);
+ $retry_after = 12 * HOUR_IN_SECONDS;
+ if ( $last_update < $this_update && ( $current_time - $last_attempt ) >= $retry_after ) {
+ update_option('slimstat_last_geoip_attempt', $current_time);
// Determine which geolocation provider to use
$provider = self::resolve_geolocation_provider();
if (false === $provider) {
return;
}
try {
$geographicProvider = new \SlimStat\Services\Geolocation\GeolocationService($provider, []);
- $ok = $geographicProvider->updateDatabase();
+ $ok = ( true === $geographicProvider->updateDatabase() );
- if ($ok) {
- update_option('slimstat_last_geoip_dl', time());
+ if ( true === $ok ) {
+ update_option('slimstat_last_geoip_dl', $current_time);
}As per coding guidelines, "Implement graceful fallbacks when external services fail; do not break the site if a remote request fails" and "Use WP-Cron or batching for heavy jobs like IP lookups and aggregations; avoid blocking requests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@wp-slimstat.php` around lines 1267 - 1271, The GeoIP update path
($geographicProvider->updateDatabase()) currently only updates
slimstat_last_geoip_dl on success, leaving the timestamp stale on failure and
causing repeated blocking attempts in the cron-disabled fallback; change the
failure handling so a retry/backoff is recorded: when updateDatabase() fails,
set a retry timestamp (either update_option('slimstat_last_geoip_dl', time() +
BACKOFF_SECONDS) or a dedicated option like 'slimstat_geoip_retry_after') and
implement exponential/backoff logic (e.g., increase BACKOFF_SECONDS on
consecutive failures using a failure count option) so admin/index.php’s fallback
will skip attempts until the retry timestamp has passed instead of retrying on
every page load.
E2E Test Results4/4 passed (7.0s)
Test 4 is new — stubs HTTP with WP_Error so updateDatabase() returns false, then asserts slimstat_last_geoip_dl remains null. Confirms that failed downloads no longer suppress retry attempts. |
PR Review: fix(cron): only update geoip timestamp on successful downloadVerdict: APPROVE Production Change (
|
| Category | Status |
|---|---|
| Security | Pass — no new attack surface |
| Performance | Pass — negligible overhead |
| Standards | Pass — follows existing patterns |
| Backward Compat | Pass — no public API changes |
| Test Coverage | Pass — dedicated regression test added |
No findings above confidence threshold. Clean PR.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/e2e/helpers/cron-frontend-shim-mu-plugin.php (1)
81-86: Align this new branch with PHP style guidelines (Yoda + tabs).Use Yoda condition format and tab indentation in this changed block.
As per coding guidelines, `**/*.php`: "Use tabs (not spaces) for indentation" and "Use Yoda Conditions: `if ( 'value' === $var )`".Suggested change
- } elseif ($mode === 'callback_download_fail') { + } elseif ( 'callback_download_fail' === $mode ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/helpers/cron-frontend-shim-mu-plugin.php` around lines 81 - 86, Change the branch to use a Yoda condition and tabs for indentation: replace "elseif ($mode === 'callback_download_fail')" with "elseif ( 'callback_download_fail' === $mode )" and convert the indentation in that block (including the add_filter('pre_http_request', function () { ... }, 1); closure and its inner lines) from spaces to tabs so the block uses tab indentation consistently.tests/e2e/issue-180-dbip-cron-tempnam.spec.ts (1)
83-88: Assert shim success before timestamp assertions.Add an explicit success check so failures in the shim path are surfaced immediately.
Suggested change
const body = await res.json(); + expect(body.success, 'Shim callback should complete successfully').toBe(true); // The download failed (HTTP stubbed with WP_Error → updateDatabase returns false). // The timestamp must NOT be updated — otherwise retries are suppressed until next month. expect(body.ts_changed, 'Timestamp should not change on failed download').toBe(false);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/issue-180-dbip-cron-tempnam.spec.ts` around lines 83 - 88, Before asserting the timestamp, add an explicit assertion that the shim succeeded by checking the response body success flag (e.g. assert expect(body.success, 'Shim should succeed').toBe(true) or the appropriate success field on body) immediately after const body = await res.json(); so shim failures are surfaced before the ts_changed assertion; keep the new expect placed before expect(body.ts_changed...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/helpers/cron-frontend-shim-mu-plugin.php`:
- Around line 91-104: The current try/catch around
\wp_slimstat::wp_slimstat_update_geoip_database() swallows any \Throwable and
still returns success: true, hiding failures; change the handler so that if a
\Throwable is caught you either rethrow or set the response to indicate failure
(e.g. success: false) and include/log the exception details (message/trace)
before returning the JSON; update the catch block for
wp_slimstat_update_geoip_database to capture $e, record/process the error (via
error_log or returning it in the JSON) and ensure the echo json_encode response
uses the captured error to set 'success' => false and include an 'error' field
instead of always reporting success.
---
Nitpick comments:
In `@tests/e2e/helpers/cron-frontend-shim-mu-plugin.php`:
- Around line 81-86: Change the branch to use a Yoda condition and tabs for
indentation: replace "elseif ($mode === 'callback_download_fail')" with "elseif
( 'callback_download_fail' === $mode )" and convert the indentation in that
block (including the add_filter('pre_http_request', function () { ... }, 1);
closure and its inner lines) from spaces to tabs so the block uses tab
indentation consistently.
In `@tests/e2e/issue-180-dbip-cron-tempnam.spec.ts`:
- Around line 83-88: Before asserting the timestamp, add an explicit assertion
that the shim succeeded by checking the response body success flag (e.g. assert
expect(body.success, 'Shim should succeed').toBe(true) or the appropriate
success field on body) immediately after const body = await res.json(); so shim
failures are surfaced before the ts_changed assertion; keep the new expect
placed before expect(body.ts_changed...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 82d5f0f4-4561-4d80-9158-eaf9a824d074
📒 Files selected for processing (2)
tests/e2e/helpers/cron-frontend-shim-mu-plugin.phptests/e2e/issue-180-dbip-cron-tempnam.spec.ts
- Shim callback_download_fail catch block now returns success:false with error details when \Throwable is caught instead of silently succeeding - Use Yoda conditions throughout cron-frontend-shim-mu-plugin.php - Add expect(body.success).toBe(true) guard in Test 4 before timestamp assertions to surface shim failures early
Add E2E tests for Pro issue #182 (Advanced Whois AJAX migration) with supporting nonce-helper MU-plugin and setup.ts helper functions.
Replace json_encode() with wp_json_encode() (6 instances) and sanitize direct $_GET comparison on line 24 for WPCS consistency.
PR Review: APPROVEReviewer: Automated WP PR Review Production Code (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/helpers/nonce-helper-mu-plugin.php`:
- Around line 1-21: Wrap the registration of the WP AJAX endpoint so it only
runs when the E2E harness constant is set: check for the SLIMSTAT_E2E_TESTING
constant before calling add_action('wp_ajax_test_create_nonce', ...). Modify the
top-level registration in this MU-plugin (the anonymous callback that uses
current_user_can('manage_options'), sanitize_text_field($_POST['nonce_action']),
wp_create_nonce and wp_send_json_success/wp_send_json_error) to be a no-op
unless SLIMSTAT_E2E_TESTING is defined and truthy, mirroring the guard used in
cron-frontend-shim-mu-plugin.php.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 18d78b85-5c28-4ab8-b512-e2807c8d6ec2
📒 Files selected for processing (5)
tests/e2e/helpers/cron-frontend-shim-mu-plugin.phptests/e2e/helpers/nonce-helper-mu-plugin.phptests/e2e/helpers/setup.tstests/e2e/issue-180-dbip-cron-tempnam.spec.tstests/e2e/pro-maxmind-details-addon.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/issue-180-dbip-cron-tempnam.spec.ts
| test('admin pages load without PHP fatal errors', async ({ page }) => { | ||
| await setSlimstatOption(page, 'addon_maxmind_enable', 'on'); | ||
|
|
||
| const adminPages = [ | ||
| '/wp-admin/', | ||
| '/wp-admin/admin.php?page=slimview1', | ||
| '/wp-admin/admin.php?page=slimconfig', | ||
| ]; | ||
|
|
||
| for (const adminPage of adminPages) { | ||
| const response = await page.goto(adminPage); | ||
| expect(response?.status(), `${adminPage} should not 500`).toBeLessThan(500); | ||
|
|
||
| const body = await page.content(); | ||
| expect(body).not.toContain('Fatal error'); | ||
| expect(body).not.toContain("Class 'SlimStat\\Services\\GeoIP' not found"); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Assert authenticated admin context before treating these pages as covered.
Tests 1, 5, and 6 will also pass on wp-login.php: the login page typically returns a non-500 response, contains neither fatal string, and does not inject the whois URL. That means these checks can go green without ever rendering the addon UI. Please add an explicit admin-only assertion before the page checks so the suite fails fast when auth/setup is broken.
Suggested change
test('admin pages load without PHP fatal errors', async ({ page }) => {
await setSlimstatOption(page, 'addon_maxmind_enable', 'on');
+ await page.goto('/wp-admin/');
+ await expect(page).toHaveTitle(/Dashboard/);
const adminPages = [
'/wp-admin/',
'/wp-admin/admin.php?page=slimview1',
'/wp-admin/admin.php?page=slimconfig',
@@
test('admin pages stable when switching providers with addon enabled', async ({ page }) => {
await setSlimstatOption(page, 'addon_maxmind_enable', 'on');
+ await page.goto('/wp-admin/');
+ await expect(page).toHaveTitle(/Dashboard/);
for (const provider of ['dbip', 'maxmind', 'cloudflare', 'disable']) {
await setSlimstatOption(page, 'geolocation_provider', provider);
@@
test('addon disabled does not inject whois URL', async ({ page }) => {
await setSlimstatOption(page, 'addon_maxmind_enable', 'off');
await setSlimstatOption(page, 'geolocation_provider', 'dbip');
+ await page.goto('/wp-admin/');
+ await expect(page).toHaveTitle(/Dashboard/);
await page.goto('/wp-admin/admin.php?page=slimview1');
await page.waitForLoadState('domcontentloaded');Also applies to: 155-170, 174-183
Mirror the safety guard from cron-frontend-shim-mu-plugin.php so the test AJAX endpoint is a no-op on non-test environments.
1 participant
Summary
updateDatabase()return value before updatingslimstat_last_geoip_dladmin/index.php:2088Addresses review feedback from PR #183: #183 (comment)
Test plan
updateDatabase()returnsfalse,slimstat_last_geoip_dlis NOT updatedupdateDatabase()returnstrue, timestamp is updated normallySummary by CodeRabbit
Bug Fixes
Tests