Add staggered/batched purge release (spec 003)#12
Conversation
70dfd91 to
01c1cda
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements “spec 003” by adding an optional staggered/batched purge-release path (with locking and runtime budgeting), plus lag detection/notifications and a new CLI status command. It also extends the queue storage contract so drivers can expose queue/claim state for observability and resumable draining.
Changes:
- Extend
QueueStorageInterfacewithpendingCount(),activeBatch(), andoldestPendingAgeSeconds(), and implement these in DB/Redis drivers (plus Pool delegation). - Add
StaggeredFlushto release purges in paced chunks under a lock and runtime budget, with resumption behavior for interrupted runs. - Add
LagDetector(flag-backed counter + admin notifications) andsamjuk:cache-debounce:status, plus config defaults + Admin UI fields + updated docs/tests.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Test/Unit/Model/Storage/RedisTest.php | Adds unit coverage for new Redis storage introspection methods. |
| Test/Unit/Model/Storage/PoolTest.php | Verifies Pool delegates new introspection methods to the resolved driver. |
| Test/Unit/Model/Storage/DatabaseTest.php | Adds unit coverage for DB storage introspection methods (pending/active/oldest age). |
| Test/Unit/Model/StaggeredFlushTest.php | Unit tests for lock behavior, chunking, runtime cutoff, failure handling, and lag sampling. |
| Test/Unit/Model/LagDetectorTest.php | Unit tests for lag ratio evaluation, counter persistence, and notification cadence. |
| Test/Unit/Model/ConfigTest.php | Adds tests for stagger config flag + numeric getters. |
| Test/Integration/Model/Storage/RedisTest.php | Integration coverage for pending/active/oldest age behavior in Redis driver. |
| Test/Integration/Model/Storage/DatabaseTest.php | Integration coverage for pending/active/oldest age behavior in DB driver. |
| Test/Integration/Model/StaggeredFlushTest.php | Integration tests for crash-resumption and lock contention behavior. |
| README.md | Documents staggered release settings/behavior and the new status command. |
| Model/Storage/Redis.php | Implements new storage introspection for Redis driver. |
| Model/Storage/QueueStorageInterface.php | Extends storage contract with introspection methods needed by stagger/status. |
| Model/Storage/Pool.php | Delegates new introspection methods to the configured driver. |
| Model/Storage/Database.php | Implements new storage introspection for DB driver (including oldest pending age). |
| Model/StaggeredFlush.php | New staggered/batched release implementation with lock + runtime budget + lag sampling. |
| Model/LagDetector.php | New persisted lag detector with notification cadence logic. |
| Model/Config.php | Adds stagger config paths and typed getters. |
| etc/di.xml | Registers the new samjuk:cache-debounce:status console command. |
| etc/config.xml | Adds default config values for the stagger settings. |
| etc/adminhtml/system.xml | Exposes stagger settings in Admin configuration UI. |
| Cron/Flush.php | Routes cron flush through StaggeredFlush instead of direct Entries::flush(). |
| Console/Command/Status.php | New CLI command to display pending/active/last-flush/lag state. |
| Console/Command/Flush.php | Routes CLI flush through StaggeredFlush instead of direct Entries::flush(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $keys = $this->connectionResolver->getClient()->keys(self::KEY_BATCH_PREFIX . '*'); | ||
|
|
||
| if (!$keys) { | ||
| return ''; | ||
| } |
There was a problem hiding this comment.
Claude: Confirmed, real. KEYS is an O(N) full-keyspace scan and blocks Redis while running; it's called from both StaggeredFlush and status on every invocation. In practice this app's own keyspace stays tiny (at most one active batch key), but on a shared Redis instance with lots of unrelated keys this still scans everything. Fix: track the active batch id in a dedicated string key (SET/GET) instead of scanning.
| $connection = $this->resourceConnection->getConnection(); | ||
| $query = $connection->select() | ||
| ->from($this->tableName, ['oldest' => 'MIN(created_at)']) | ||
| ->where('batch_id = ?', self::UNCLAIMED_BATCH_ID); | ||
|
|
||
| $oldest = $connection->fetchOne($query); | ||
|
|
||
| return $oldest ? max(0, time() - strtotime($oldest)) : null; |
There was a problem hiding this comment.
Claude: Confirmed, real but display-only — this feeds the status CLI's oldest-pending-age output, not LagDetector's ratio logic, so the blast radius is a wrong number on screen, not a bad alerting decision. Still worth fixing properly: TIMESTAMPDIFF(SECOND, created_at, NOW()) in SQL avoids the tz mismatch and the extra PHP-side parsing.
| $this->flagManager = $this->createMock(FlagManager::class); | ||
| $this->flagManager->method('getFlagData')->willReturnCallback(function () { | ||
| return $this->flagState; | ||
| }); |
There was a problem hiding this comment.
Claude: Not a real issue. PHP closures silently ignore extra call arguments — no warning, no error, on any PHP version here. CI is green on php74–php84, which is direct evidence this doesn't fire. Fluff — skip (though adding the unused $code param costs nothing if you want to silence the suggestion).
| $this->flagManager->method('deleteFlag')->willReturnCallback(function () { | ||
| $this->flagState = null; | ||
| return true; | ||
| }); |
There was a problem hiding this comment.
Claude: Same as the sibling comment on getFlagData() — not a real issue, PHP doesn't error on extra args to a closure, CI proves it. Fluff — skip.
| try { | ||
| foreach ($chunks as $chunk) { | ||
| if ((microtime(true) - $start) >= $maxRuntimeSeconds) { | ||
| $this->logger->debug( | ||
| "[CacheDebounce] Staggered release hit its runtime budget; " | ||
| . "batch $batchId stays claimed for the next run." | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| if (!$this->purgeCache->sendPurgeRequest($chunk)) { | ||
| $this->logger->error( | ||
| "[CacheDebounce] Staggered purge chunk failed — leaving batch $batchId queued for retry." | ||
| ); | ||
| return; | ||
| } | ||
|
|
||
| usleep($intervalMicroseconds); | ||
| } | ||
| } finally { | ||
| $this->config->setShouldDebouncePurgeRequest(true); | ||
| } |
There was a problem hiding this comment.
Claude: Confirmed, real but trivial. Sleeping after the final chunk burns part of the runtime budget for no benefit since there's no next chunk to pace against. One-line fix: skip the sleep when the current chunk is the last one in $chunks.
|
Claude: Overall — good feature, mostly cosmetic findings from Copilot here. One real perf concern worth fixing: Redis activeBatch() uses KEYS which is a blocking full-keyspace scan, called on every StaggeredFlush and status invocation — swap for a dedicated marker key. The oldest-age timezone drift and trailing usleep are real but low-impact (display-only / wastes a few hundred ms), fine to fix opportunistically. The two LagDetectorTest callback-arg comments are fluff — PHP doesn't error on that, CI is green across all PHP versions. Separately, unrelated to Copilot: CI is currently red on phpcs (line-length warning in Console/Command/Status.php:62) — needs fixing regardless before this can merge. |
6ed114f to
30b71be
Compare
01c1cda to
7973aef
Compare
30b71be to
ef94f80
Compare
743adc8 to
dd6e213
Compare
01b2871 to
35709d9
Compare
dd6e213 to
8536789
Compare
35709d9 to
d871903
Compare
8536789 to
81c8fd4
Compare
…e race Model/Entries previously did get() -> sendPurgeRequest() -> delete(whole table), so any tag added while a purge request was in flight got wiped by the trailing delete without ever being purged. Queue storage is now behind QueueStorageInterface (add/claim/tags/clear/release), with claim() atomically carving off only the currently-pending tags into a batch, so anything added afterward survives into the next batch instead of being lost. A failed or throwing purge now releases the claimed batch back to pending instead of leaving it stranded under a batch id no future claim() can ever match. Schema changes (batch_id, created_at columns, db_schema_whitelist.json) are additive only this release — no primary key yet. Existing installs may have NULL/duplicate tag rows from the old unconstrained table, and setup:upgrade applies declarative schema before data patches run, so a constraint added in this same release would fail against that dirty data before the patch ever got a chance to clean it up. A data patch clears the table as a one-time cleanup; the batch_id+tag primary key itself is deferred to a follow-up release. In the meantime, add() dedupes against already-pending tags and filters null/empty ones at the application level, since the constraint that would normally enforce that isn't there yet. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
d871903 to
9255ab1
Compare
81c8fd4 to
ba2efdb
Compare
Redis implementation of QueueStorageInterface (SADD/RENAME/SMEMBERS/DEL, SUNIONSTORE for release), resolved via a Pool that picks db vs redis from samjuk_cache_debounce_advanced/general/storage_driver. That field is CLI-only: hidden from Admin UI (showInDefault/Website/Store all "0") and gated behind its own ACL resource, since switching storage drivers is a deploy-time infra decision, not a store setting. ConnectionResolver reads a dedicated app/etc/env.php block first, falling back to whatever Redis backend the page_cache frontend already uses so it works zero-config once switched on. Missing keys in either config fall back to sane defaults instead of hitting an undefined-index notice. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
9255ab1 to
8ee2c3a
Compare
…ommand Optional paced release of a claimed batch: chunked into batch_size-sized purge requests, spaced by interval_ms, within a max_runtime_seconds budget per cron/CLI invocation. Both the cron job and the flush command route through StaggeredFlush, falling back to a single-shot flush when stagger is disabled. Only one release runs at a time, guarded by LockManagerInterface. An interrupted run leaves its batch claimed; the next invocation resumes it by re-purging the full tag list rather than a persisted chunk offset — a Varnish BAN is idempotent, so the cost is at worst a few redundant purges. The runtime-budget check only applies from the second chunk onward, so a misconfigured (e.g. zero) max_runtime_seconds can't block every chunk and wedge the batch permanently. Lag sampling is skipped on a resumed batch, since its post-drain pending count can include tags that arrived before the run started. Entries::flush() (the non-stagger path) now resumes an already-claimed batch via activeBatch() before claiming new work, and writes its own last-flush flags, so status doesn't lie when stagger is off. LagDetector persists a consecutive-lagging-runs counter and raises an admin notification every lag_alert_after_runs runs for as long as the lag continues. samjuk:cache-debounce:status reports pending count, active batch, last flush, and lag state. QueueStorageInterface gained pendingCount()/activeBatch()/ oldestPendingAgeSeconds() for status/staggering to introspect queue state; Redis's activeBatch() uses a dedicated marker key rather than a KEYS scan, and Database's oldestPendingAgeSeconds() computes the age in SQL via TIMESTAMPDIFF rather than PHP time()/strtotime(), avoiding a timezone mismatch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ba2efdb to
2692260
Compare
|
60d514d to
368869f
Compare
Summary
QueueStorageInterfacewithpendingCount(),activeBatch(),oldestPendingAgeSeconds()(Redis returns null for the last one — Sets have no per-member insertion time).StaggeredFlush: releases a claimed batch inbatch_size-chunked purge requests paced byinterval_ms, within amax_runtime_secondsbudget, guarded byLockManagerInterfaceso only one release runs at a time. Disabled by default (stagger/enabled = 0) — falls straight through to the existing single-shotflush(), no behavior change for current installs. Both the cron job andsamjuk:cache-debounce:flushroute through it now.LagDetector: compares tags arriving during a drain against tags just drained, tracks a consecutive-lagging-runs counter viaFlagManager, and raises an admin bell-icon notification everylag_alert_after_runsruns for as long as the lag persists.bin/magento samjuk:cache-debounce:statusfor pending count, active batch, last flush, and lagging-run visibility.Implements docs/specs/003-staggered-purge-release.md.
Test plan
vendor/bin/phpunit Test/Unit— chunking, runtime-budget cutoff, lock contention, batch resumption, lag-counter/notification cadencebin/magento setup:di:compilestagger, queue >batch_sizetags, confirm Varnish purges arrive in paced chunks andsamjuk:cache-debounce:statusreflects state throughout