Abstract queue storage behind an interface, fix add-during-purge race#10
Conversation
9d4411a to
43348f3
Compare
There was a problem hiding this comment.
Pull request overview
This PR refactors the cache-debounce queue to be storage-backed behind a QueueStorageInterface, introducing an atomic “claim + drain” batch model to prevent tags being dropped when they’re added during an in-flight purge.
Changes:
- Introduces
QueueStorageInterfaceand aDatabaseimplementation that batches pending tags viabatch_idand supportsadd(),claim(),tags(),clear(). - Updates
Entriesto use the new storage abstraction (and removes the oldEntries::get()surface) so a purge drains a claimed batch rather than read-then-wipe the whole table. - Updates declarative schema to add
batch_id,created_at, composite PK(batch_id, tag), and addsdb_schema_whitelist.json; adds/updates unit + integration tests.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Test/Unit/Model/Storage/DatabaseTest.php | Adds unit coverage for Database storage add/claim/tags/clear behavior. |
| Test/Unit/Model/EntriesTest.php | Updates unit tests for Entries to use storage abstraction and batch flow. |
| Test/Integration/Model/Storage/DatabaseTest.php | Adds integration coverage for claim batching and add-during-claim regression. |
| Test/Integration/Model/EntriesTest.php | Updates integration tests to validate behavior via storage batches (no more Entries::get()). |
| Model/Storage/QueueStorageInterface.php | Defines the queue storage contract for batch-based claiming and clearing. |
| Model/Storage/Database.php | Implements queue storage in Magento DB using batch_id updates and batch-scoped reads/deletes. |
| Model/Entries.php | Switches queue operations to storage + batch claim, and removes live-table get(). |
| etc/di.xml | Wires QueueStorageInterface to the Database implementation via preference. |
| etc/db_schema.xml | Adds batch_id, created_at, and composite primary key to support idempotency and batching. |
| etc/db_schema_whitelist.json | Adds the whitelist file required for declarative schema. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!$success) { | ||
| $this->logger->error( | ||
| "[CacheDebounce] Purge request failed — leaving tags queued for retry: " . json_encode($tags) | ||
| "[CacheDebounce] Purge request failed — leaving batch $batchId queued for retry: " | ||
| . json_encode($tags) | ||
| ); |
There was a problem hiding this comment.
Claude: Confirmed, real bug. claim() only matches batch_id = '', so on sendPurgeRequest() failure this leaves the batch stuck under its random batch_id forever — the "queued for retry" log message is wrong. Needs a release-back-to-unclaimed path (either re-add tags with batch_id='' or a dedicated release(batchId) storage method) on both the failure branch and any exception.
| <constraint xsi:type="primary" referenceId="PRIMARY"> | ||
| <column name="batch_id"/> | ||
| <column name="tag"/> | ||
| </constraint> |
There was a problem hiding this comment.
Claude: Confirmed, real risk but low urgency. Previous schema had tag nullable=true with no uniqueness constraint, so any existing install with dupes/nulls will fail setup:upgrade against the new composite PK. Cheap fix since this table is disposable/derived data: add a data patch that truncates it before the schema change lands, rather than trying to dedupe in place.
|
Claude: Overall — solid abstraction, one real bug blocks merge. |
7c1f22a to
b932ccf
Compare
|
b932ccf to
ad5a3df
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>
ad5a3df to
8d9da1f
Compare
Summary
ResourceConnectionusage inEntrieswith aQueueStorageInterface+Databasedriver, so a batch of pending tags can be atomically claimed and drained instead of read-then-wiped.delete().batch_id+ composite(batch_id, tag)primary key,tagis nowNOT NULL, and regenerates the previously-missingdb_schema_whitelist.json.Entries::get()is removed from the public surface — it read the whole live table, which isn't meaningful once "pending" isn't a flat readable set.Test plan
vendor/bin/phpunit Test/Unitpasses (verified against a real Magento install's classes)vendor/bin/phpstan analyseclean at the repo's configured levelsetup:upgrade+setup:di:compile+dev/tests/integrationin CI