From d1b702d8e4dec1d92cf1e205adb79b3e7aff2594 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Sat, 8 Aug 2026 19:26:12 +0100 Subject: [PATCH 01/13] Split queue workers into fast and slow pools A single worker consumed every queue, so a long-running handler blocked everything behind it: a newsletter batch or a ten-minute feed build would hold up an order confirmation email for its whole runtime. Cron now keeps one detached worker per configured pool. Queues are routed to a tier under global/queue/routing, one node per queue so a module can route its own and local.xml can retarget a single one. Pools themselves carry only resourcing (count, memory, time limit, idle timeout, redelivery window) under global/queue/pools. Core ships two tiers: "fast" stays resident for transactional email, while "slow" is the catch-all and only runs while it has work, exiting after an idle grace period. An unrouted queue falls to the catch-all on purpose, so forgetting to declare a tier costs a message waiting behind a feed rather than a checkout email stuck behind one. Redelivery is per pool, which means stale claims are only requeued for the queues a worker owns; otherwise the fast worker's shorter window would requeue a feed the slow worker is still running. The watchdog probe counts only work that is actually due, plus claims abandoned by a dead worker, so a campaign scheduled for next week does not respawn a worker every minute and a crashed handler is not stranded. Queue filtering is unavailable on the Redis transport, which is not a QueueReceiverInterface, so pools collapse to a single worker there. --- AGENTS.md | 8 +- app/code/core/Maho/Queue/Model/Cron.php | 81 ++++-- app/code/core/Maho/Queue/etc/config.xml | 25 ++ app/code/core/Maho/Queue/etc/system.xml | 4 +- app/locale/en_US/Maho_Queue.csv | 4 +- lib/Maho/Queue/Pool.php | 62 +++++ lib/Maho/Queue/PoolRegistry.php | 191 +++++++++++++++ lib/Maho/Queue/QueueManager.php | 26 ++ lib/Maho/Queue/StopWorkerWhenIdleListener.php | 23 +- lib/Maho/Queue/Transport/DbTransport.php | 77 +++++- lib/Maho/Queue/WorkerFactory.php | 8 +- lib/MahoCLI/Commands/EmailQueueProcess.php | 2 +- lib/MahoCLI/Commands/QueueList.php | 11 +- lib/MahoCLI/Commands/QueueWork.php | 72 ++++-- .../Integration/Queue/CronConsumerTest.php | 63 ++++- tests/Backend/Integration/Queue/PoolTest.php | 231 ++++++++++++++++++ 16 files changed, 813 insertions(+), 75 deletions(-) create mode 100644 lib/Maho/Queue/Pool.php create mode 100644 lib/Maho/Queue/PoolRegistry.php create mode 100644 tests/Backend/Integration/Queue/PoolTest.php diff --git a/AGENTS.md b/AGENTS.md index c645daaf87..0f1a0acea4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -171,8 +171,12 @@ class My_Module_Checkout_CartController extends Mage_Checkout_CartController { / - **Events**: `Mage::dispatchEvent('event_name', ['data' => $data])` - **Async queue**: `\Maho\Queue\QueueManager::dispatch($messageDto)` queues a flat DTO for a `#[Maho\Config\MessageHandler]` method (message class inferred from the first parameter type); - cron automatically keeps a detached `queue:work` worker alive, with retries/backoff and an - admin grid under System > Message Queue + cron keeps a detached `queue:work` worker alive per pool, with retries/backoff and an admin + grid under System > Message Queue. Worker pools split latency classes: `fast` is resident, + `slow` is the on-demand catch-all. Pass `queue:` to `dispatch()`, then route that queue with + `fast`; anything + unrouted lands in the catch-all, so a long-running handler never blocks short ones. Pool + resourcing (count, limits, idle timeout, redelivery) lives under `global/queue/pools` - **Layout**: XML-based block hierarchy and template assignment - **Sessions**: `Mage::getSingleton('customer/session')`, `'admin/session'`, `'checkout/session'` - **Translations**: `$this->__('Text')`, CSVs in `app/locale/[locale]/` diff --git a/app/code/core/Maho/Queue/Model/Cron.php b/app/code/core/Maho/Queue/Model/Cron.php index 40170f94ec..3aaa3c4fb9 100644 --- a/app/code/core/Maho/Queue/Model/Cron.php +++ b/app/code/core/Maho/Queue/Model/Cron.php @@ -8,36 +8,26 @@ declare(strict_types=1); +use Maho\Queue\Pool; +use Maho\Queue\PoolRegistry; use Maho\Queue\QueueManager; use Maho\Queue\Transport\DbTransport; class Maho_Queue_Model_Cron { - /** - * Held by the exclusive worker for its whole life; deliberately a - * machine-local kernel flock even when the db lock backend is configured, - * so every frontend server runs one worker of its own (parallel - * consumption is safe, the transport claim is atomic) and the flock - * disappears the moment the process dies, doubling as the liveness probe. - */ - public const WORKER_LOCK = 'queue.worker'; - - /** Worker recycling cadence: picks up deployed code and frees memory. */ - public const WORKER_TIME_LIMIT = 3600; - public const WORKER_MEMORY_LIMIT = '256M'; - private const SPAWN_WAIT_ATTEMPTS = 10; private const SPAWN_WAIT_MICROSECONDS = 500_000; /** - * Watchdog: when no worker holds the lock, start a detached - * `queue:work --exclusive` (respawned within a minute of any death, - * recycled hourly via its time limit). + * Watchdog: start a detached `queue:work --exclusive` for every configured + * pool with no live worker, so each latency tier gets a process of its own + * and a slow handler can never sit in front of a fast one. */ #[Maho\Config\CronJob('queue_process', schedule: '* * * * *')] public function process(): void { - if (Mage::getSingleton('core/lock')->isHeld(self::WORKER_LOCK, machineLocal: true)) { + $pending = $this->workersToSpawn(); + if ($pending === []) { return; } @@ -46,7 +36,46 @@ public function process(): void return; } - $this->spawnWorker(); + foreach ($pending as [$pool, $index]) { + $this->spawnWorker($pool, $index); + } + } + + /** + * Split out from process() so the decision is testable without spawning. + * + * @return list + */ + public function workersToSpawn(): array + { + $lock = Mage::getSingleton('core/lock'); + $spawn = []; + + foreach (PoolRegistry::all() as $pool) { + $hasWork = null; + for ($index = 0; $index < $pool->count; $index++) { + if ($lock->isHeld($pool->lockName($index), machineLocal: true)) { + continue; + } + if ($pool->isOnDemand()) { + $hasWork ??= $this->hasDueWork($pool); + if (!$hasWork) { + break; + } + } + $spawn[] = [$pool, $index]; + } + } + + return $spawn; + } + + private function hasDueWork(Pool $pool): bool + { + $transport = QueueManager::workerTransport($pool); + + // Redis cannot be probed per queue; spawn and let the worker idle out. + return !$transport instanceof DbTransport || $transport->countDue($pool->queues) > 0; } #[Maho\Config\CronJob('queue_clean_up', schedule: '0 2 * * *')] @@ -72,25 +101,29 @@ public function cleanup(): void } } - private function spawnWorker(): void + private function spawnWorker(Pool $pool, int $index): void { exec(sprintf( - 'nohup %s %s queue:work --exclusive --time-limit=%d --memory-limit=%s >> %s 2>&1 &', + 'nohup %s %s queue:work --exclusive --pool=%s --index=%d >> %s 2>&1 &', escapeshellarg(PHP_BINARY), escapeshellarg(Mage::getBaseDir() . '/maho'), - self::WORKER_TIME_LIMIT, - escapeshellarg(self::WORKER_MEMORY_LIMIT), + escapeshellarg($pool->name), + $index, escapeshellarg(Mage::getBaseDir('var') . '/log/queue-worker.log'), )); $lock = Mage::getSingleton('core/lock'); for ($attempt = 0; $attempt < self::SPAWN_WAIT_ATTEMPTS; $attempt++) { usleep(self::SPAWN_WAIT_MICROSECONDS); - if ($lock->isHeld(self::WORKER_LOCK, machineLocal: true)) { + if ($lock->isHeld($pool->lockName($index), machineLocal: true)) { return; } } - Mage::log('Queue worker did not start after spawning; check var/log/queue-worker.log', Mage::LOG_ERROR); + // An on-demand worker may have drained its queue and exited inside the wait window. + Mage::log( + sprintf('Queue worker for pool "%s" did not start after spawning; check var/log/queue-worker.log', $pool->name), + $pool->isOnDemand() ? Mage::LOG_NOTICE : Mage::LOG_ERROR, + ); } } diff --git a/app/code/core/Maho/Queue/etc/config.xml b/app/code/core/Maho/Queue/etc/config.xml index 212112b788..ade1951ddc 100644 --- a/app/code/core/Maho/Queue/etc/config.xml +++ b/app/code/core/Maho/Queue/etc/config.xml @@ -39,6 +39,31 @@ Maho_Queue_Block + + + + + fast + + + + 900 + 10 + + + 1 + 60 + 512M + 10800 + 20 + + + diff --git a/app/code/core/Maho/Queue/etc/system.xml b/app/code/core/Maho/Queue/etc/system.xml index 7687fd709d..8d09fdf0c7 100644 --- a/app/code/core/Maho/Queue/etc/system.xml +++ b/app/code/core/Maho/Queue/etc/system.xml @@ -13,7 +13,7 @@ SPDX-License-Identifier: AFL-3.0 1 0 0 - Background message processing. Maho cron keeps a detached "queue:work" worker running automatically: respawned within a minute if it dies, recycled hourly. + Background message processing. Maho cron keeps one detached "queue:work" worker running per pool: a resident "fast" worker so short jobs never queue behind long ones, plus a "slow" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools. @@ -57,7 +57,7 @@ SPDX-License-Identifier: AFL-3.0 0 0 required-entry validate-digits validate-greater-than-zero - Messages claimed by a worker that died are re-queued after this long. Keep above the runtime of your slowest handler. + Messages claimed by a worker that died are re-queued after this long. Keep above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume. diff --git a/app/locale/en_US/Maho_Queue.csv b/app/locale/en_US/Maho_Queue.csv index 1dd00e20de..a54d325d50 100644 --- a/app/locale/en_US/Maho_Queue.csv +++ b/app/locale/en_US/Maho_Queue.csv @@ -6,7 +6,7 @@ "Available","Available" "Available (UTC)","Available (UTC)" "Back","Back" -"Background message processing. Maho cron keeps a detached ""queue:work"" worker running automatically: respawned within a minute if it dies, recycled hourly.","Background message processing. Maho cron keeps a detached ""queue:work"" worker running automatically: respawned within a minute if it dies, recycled hourly." +"Background message processing. Maho cron keeps one detached ""queue:work"" worker running per pool: a resident ""fast"" worker so short jobs never queue behind long ones, plus a ""slow"" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools.","Background message processing. Maho cron keeps one detached ""queue:work"" worker running per pool: a resident ""fast"" worker so short jobs never queue behind long ones, plus a ""slow"" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools." "Body (serialized)","Body (serialized)" "Claimed (UTC)","Claimed (UTC)" "Completed","Completed" @@ -30,7 +30,7 @@ "Message not found.","Message not found." "Message Queue","Message Queue" "Message re-queued.","Message re-queued." -"Messages claimed by a worker that died are re-queued after this long. Keep above the runtime of your slowest handler.","Messages claimed by a worker that died are re-queued after this long. Keep above the runtime of your slowest handler." +"Messages claimed by a worker that died are re-queued after this long. Keep above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume.","Messages claimed by a worker that died are re-queued after this long. Keep above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume." "Only failed messages can be retried.","Only failed messages can be retried." "Pending","Pending" "Permanently delete the selected messages?","Permanently delete the selected messages?" diff --git a/lib/Maho/Queue/Pool.php b/lib/Maho/Queue/Pool.php new file mode 100644 index 0000000000..e4b6c10bc1 --- /dev/null +++ b/lib/Maho/Queue/Pool.php @@ -0,0 +1,62 @@ + + * SPDX-License-Identifier: OSL-3.0 + */ + +declare(strict_types=1); + +namespace Maho\Queue; + +/** + * A worker pool: one or more `queue:work` processes consuming a subset of the + * logical queues, with their own limits and redelivery window. Pools keep + * latency classes apart, so a ten-minute feed build cannot sit in front of an + * order confirmation email. + */ +final readonly class Pool +{ + public const LOCK_PREFIX = 'queue.worker'; + + /** + * @param list $queues Consume only these queues; empty consumes every queue not excluded + * @param list $excludedQueues Never consume these; the catch-all pool excludes every other pool's queues + * @param ?int $idleTimeout Seconds of continuous idleness before exiting; null keeps the worker resident + * @param ?int $redeliverAfter Per-pool override of system/queue/redeliver_after + */ + public function __construct( + public string $name, + public array $queues = [], + public array $excludedQueues = [], + public int $count = 1, + public ?int $idleTimeout = null, + public string $memoryLimit = '256M', + public int $timeLimit = 3600, + public ?int $redeliverAfter = null, + ) {} + + /** + * On-demand pools exit once idle, so the watchdog only starts them when + * their queues have work due. + */ + public function isOnDemand(): bool + { + return $this->idleTimeout !== null; + } + + /** Taken machine-local: the flock dies with the process and doubles as the liveness probe. */ + public function lockName(int $index = 0): string + { + return self::LOCK_PREFIX . '.' . $this->name . '.' . $index; + } + + public function consumes(string $queue): bool + { + if (in_array($queue, $this->excludedQueues, true)) { + return false; + } + + return $this->queues === [] || in_array($queue, $this->queues, true); + } +} diff --git a/lib/Maho/Queue/PoolRegistry.php b/lib/Maho/Queue/PoolRegistry.php new file mode 100644 index 0000000000..cf12581060 --- /dev/null +++ b/lib/Maho/Queue/PoolRegistry.php @@ -0,0 +1,191 @@ + + * SPDX-License-Identifier: OSL-3.0 + */ + +declare(strict_types=1); + +namespace Maho\Queue; + +/** + * Reads the worker pools declared under ``, so a module + * can attach its queue to a tier without touching core. + * + * Exactly one pool should be marked `1`: it consumes + * every queue no other pool claims. Core makes the *slow* pool the catch-all + * on purpose, since an unclassified handler is likelier to be a slow newcomer + * than a latency-critical one, and forgetting to declare a tier should cost a + * message waiting behind a feed rather than a checkout email stuck behind one. + */ +final class PoolRegistry +{ + public const FALLBACK_POOL = 'default'; + + /** @var array|null */ + private static ?array $pools = null; + + /** + * @return array + */ + public static function all(): array + { + return self::$pools ??= self::build(); + } + + public static function get(string $name): ?Pool + { + return self::all()[$name] ?? null; + } + + /** Null means the queue is orphaned and no worker will ever pick it up. */ + public static function poolFor(string $queue): ?Pool + { + foreach (self::all() as $pool) { + if ($pool->consumes($queue)) { + return $pool; + } + } + + return null; + } + + public static function reset(): void + { + self::$pools = null; + } + + /** + * @return array + */ + private static function build(): array + { + $node = \Mage::getConfig()->getNode('global/queue'); + $poolsNode = $node !== false && isset($node->pools) ? $node->pools : false; + + // Symfony's Redis transport is not a QueueReceiverInterface, so Worker::run() + // rejects any queue filter: on Redis a single catch-all worker is all we can run. + if (QueueManager::transportName() === QueueManager::TRANSPORT_REDIS) { + if ($poolsNode !== false && $poolsNode->children()->count() > 1) { + \Mage::log( + 'Queue pools are ignored on the Redis transport (it cannot filter by queue name); running a single worker over all queues', + \Mage::LOG_NOTICE, + ); + } + + return [self::FALLBACK_POOL => new Pool(self::FALLBACK_POOL)]; + } + + $definitions = []; + if ($poolsNode !== false) { + foreach ($poolsNode->children() as $name => $child) { + if (!self::flag($child, 'active', true)) { + continue; + } + $definitions[(string) $name] = $child; + } + } + + if ($definitions === []) { + return [self::FALLBACK_POOL => new Pool(self::FALLBACK_POOL)]; + } + + uasort($definitions, fn($a, $b) => (int) $a->sort_order <=> (int) $b->sort_order); + + $catchAll = null; + foreach ($definitions as $name => $child) { + if (!self::flag($child, 'catch_all', false)) { + continue; + } + if ($catchAll === null) { + $catchAll = $name; + continue; + } + \Mage::log( + sprintf('Queue pool "%s" is marked catch_all but "%s" already is; dropping it', $name, $catchAll), + \Mage::LOG_ERROR, + ); + unset($definitions[$name]); + } + + if ($catchAll === null) { + \Mage::log( + 'No queue pool is marked catch_all: messages dispatched to an unrouted queue will never be consumed', + \Mage::LOG_ERROR, + ); + } + + $queuesByPool = self::routing($node, $definitions, $catchAll); + + // A pool with nothing routed to it would consume everything and become a + // second catch-all, so it is dropped rather than left to compete. + foreach ($queuesByPool as $name => $queues) { + if ($queues === [] && $name !== $catchAll) { + \Mage::log(sprintf('Queue pool "%s" has no queues routed to it; skipping', $name), \Mage::LOG_NOTICE); + unset($definitions[$name], $queuesByPool[$name]); + } + } + + $pools = []; + foreach ($definitions as $name => $child) { + $excluded = []; + if ($name === $catchAll) { + $excluded = array_values(array_unique(array_merge(...array_values( + array_diff_key($queuesByPool, [$name => true]), + )))); + } + + $pools[$name] = new Pool( + name: (string) $name, + queues: $queuesByPool[$name], + excludedQueues: $excluded, + count: max(1, (int) ($child->count ?? 1)), + idleTimeout: isset($child->idle_timeout) ? max(0, (int) $child->idle_timeout) : null, + memoryLimit: trim((string) ($child->memory_limit ?? '')) ?: '256M', + timeLimit: max(0, (int) ($child->time_limit ?? 3600)), + redeliverAfter: isset($child->redeliver_after) ? max(0, (int) $child->redeliver_after) : null, + ); + } + + return $pools; + } + + /** + * The queue to pool map, one node per queue so a module can route its own + * queue and local.xml can retarget a single one without clobbering the rest. + * An empty value unroutes a queue, handing it back to the catch-all. + * + * @param array $definitions + * @return array> + */ + private static function routing(\Mage_Core_Model_Config_Element|false $node, array $definitions, ?string $catchAll): array + { + $queuesByPool = array_fill_keys(array_keys($definitions), []); + if ($node === false || !isset($node->routing)) { + return $queuesByPool; + } + + foreach ($node->routing->children() as $queue => $target) { + $pool = trim((string) $target); + if ($pool === '' || $pool === $catchAll) { + continue; + } + if (!isset($definitions[$pool])) { + \Mage::log( + sprintf('Queue "%s" is routed to unknown pool "%s"; it falls to the catch-all', $queue, $pool), + \Mage::LOG_ERROR, + ); + continue; + } + $queuesByPool[$pool][] = (string) $queue; + } + + return $queuesByPool; + } + + private static function flag(\Mage_Core_Model_Config_Element $node, string $child, bool $default): bool + { + return isset($node->{$child}) ? (bool) (int) $node->{$child} : $default; + } +} diff --git a/lib/Maho/Queue/QueueManager.php b/lib/Maho/Queue/QueueManager.php index 662e2ad91e..a60591ac7f 100644 --- a/lib/Maho/Queue/QueueManager.php +++ b/lib/Maho/Queue/QueueManager.php @@ -139,6 +139,31 @@ public static function dbTransport(): DbTransport ); } + /** + * The transport a pool's worker consumes from: the shared one unless the + * pool narrows what it sees or overrides the redelivery window, in which + * case it gets its own instance. Redis cannot be narrowed at all, so pools + * fall back to consuming everything there. + */ + public static function workerTransport(?Pool $pool = null): TransportInterface + { + if ($pool === null + || self::transportName() === self::TRANSPORT_REDIS + || ($pool->excludedQueues === [] && $pool->redeliverAfter === null) + ) { + return self::transport(); + } + + return new DbTransport( + self::writeAdapter(), + self::tableName(), + self::serializer(), + $pool->redeliverAfter ?? (int) \Mage::getStoreConfig(self::XML_PATH_REDELIVER_AFTER), + (int) \Mage::getStoreConfig(self::XML_PATH_COMPLETED_RETENTION), + $pool->excludedQueues, + ); + } + public static function serializer(): Serializer { return self::$serializer ??= new Serializer(); @@ -208,6 +233,7 @@ public static function reset(): void self::$serializer = null; self::$transportName = null; HandlerRegistry::reset(); + PoolRegistry::reset(); } private static function redisDsn(): ?string diff --git a/lib/Maho/Queue/StopWorkerWhenIdleListener.php b/lib/Maho/Queue/StopWorkerWhenIdleListener.php index e877f83255..e4541b372f 100644 --- a/lib/Maho/Queue/StopWorkerWhenIdleListener.php +++ b/lib/Maho/Queue/StopWorkerWhenIdleListener.php @@ -13,15 +13,30 @@ use Symfony\Component\Messenger\Event\WorkerRunningEvent; /** - * Stops the worker as soon as a poll finds no message, for bounded runs - * (cron consumer, `queue:work --stop-when-empty`). Messenger has no built-in - * stop-on-idle listener. + * Stops the worker once polls stop finding messages, for bounded runs and for + * on-demand pools that free their process between bursts. Messenger has no + * built-in stop-on-idle listener. + * + * On-demand pools want a grace period: exiting on the first empty poll makes a + * job queued seconds later wait a whole cron tick for a replacement. */ final class StopWorkerWhenIdleListener implements EventSubscriberInterface { + private ?int $idleSince = null; + + public function __construct( + private readonly int $idleTimeoutSeconds = 0, + ) {} + public function onWorkerRunning(WorkerRunningEvent $event): void { - if ($event->isWorkerIdle()) { + if (!$event->isWorkerIdle()) { + $this->idleSince = null; + return; + } + + $this->idleSince ??= time(); + if (time() - $this->idleSince >= $this->idleTimeoutSeconds) { $event->getWorker()->stop(); } } diff --git a/lib/Maho/Queue/Transport/DbTransport.php b/lib/Maho/Queue/Transport/DbTransport.php index 8f606cdab7..a324b93f1f 100644 --- a/lib/Maho/Queue/Transport/DbTransport.php +++ b/lib/Maho/Queue/Transport/DbTransport.php @@ -47,12 +47,16 @@ final class DbTransport implements TransportInterface, QueueReceiverInterface, L public const DEFAULT_QUEUE = 'default'; + /** + * @param list $excludedQueues Queues this instance never consumes, so a pool worker can be "everything but" + */ public function __construct( private readonly AdapterInterface $adapter, private readonly string $table, private readonly Serializer $serializer, private readonly int $redeliverAfterSeconds, private readonly int $completedRetentionDays, + private readonly array $excludedQueues = [], ) {} #[\Override] @@ -189,13 +193,52 @@ public function getMessageCount(): int ); } + /** + * Work this instance would pick up right now: rows past their availability + * plus rows a dead worker abandoned. Messages scheduled for the future are + * deliberately excluded, or the watchdog would respawn an on-demand worker + * every cron tick until a delayed campaign's send date. + * + * @param list|null $queues + */ + public function countDue(?array $queues = null): int + { + $clauses = ['(' . $this->adapter->quoteInto('status = ?', self::STATUS_PENDING) + . ' AND ' . $this->adapter->quoteInto('available_at <= ?', \Mage_Core_Model_Locale::nowUtc()) . ')']; + + if ($this->redeliverAfterSeconds > 0) { + $clauses[] = '(' . $this->adapter->quoteInto('status = ?', self::STATUS_PROCESSING) + . ' AND ' . $this->adapter->quoteInto('claimed_at < ?', $this->staleClaimCutoff()) . ')'; + } + + $select = $this->adapter->select() + ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) + ->where(implode(' OR ', $clauses)); + $this->applyQueueFilter($select, $queues); + + return (int) $this->adapter->fetchOne($select); + } + + /** + * @param list|null $queues + */ + private function applyQueueFilter(\Maho\Db\Select $select, ?array $queues): void + { + if ($queues !== null && $queues !== []) { + $select->where('queue IN (?)', $queues); + } + if ($this->excludedQueues !== []) { + $select->where('queue NOT IN (?)', $this->excludedQueues); + } + } + /** * @param list|null $queues * @return list */ private function claimNext(?array $queues): array { - $this->requeueStaleClaims(); + $this->requeueStaleClaims($queues); $now = \Mage_Core_Model_Locale::nowUtc(); for ($attempt = 0; $attempt < 5; $attempt++) { @@ -205,9 +248,7 @@ private function claimNext(?array $queues): array ->where('available_at <= ?', $now) ->order(['available_at ASC', 'message_id ASC']) ->limit(1); - if ($queues !== null && $queues !== []) { - $select->where('queue IN (?)', $queues); - } + $this->applyQueueFilter($select, $queues); $row = $this->adapter->fetchRow($select); if ($row === false) { @@ -239,21 +280,39 @@ private function claimNext(?array $queues): array /** * Crash recovery: rows claimed longer ago than redeliver_after belong to a * worker that died without ack/reject; put them back up for grabs. + * + * Scoped to the queues this instance consumes, because pools carry their own + * window: a fast worker must not requeue a feed a slow worker is still running. + * + * @param list|null $queues */ - private function requeueStaleClaims(): void + private function requeueStaleClaims(?array $queues): void { if ($this->redeliverAfterSeconds <= 0) { return; } + $where = [ + 'status = ?' => self::STATUS_PROCESSING, + 'claimed_at < ?' => $this->staleClaimCutoff(), + ]; + if ($queues !== null && $queues !== []) { + $where['queue IN (?)'] = $queues; + } + if ($this->excludedQueues !== []) { + $where['queue NOT IN (?)'] = $this->excludedQueues; + } + $this->adapter->update($this->table, [ 'status' => self::STATUS_PENDING, 'claimed_at' => null, 'updated_at' => \Mage_Core_Model_Locale::nowUtc(), - ], [ - 'status = ?' => self::STATUS_PROCESSING, - 'claimed_at < ?' => gmdate(\Mage_Core_Model_Locale::DATETIME_FORMAT, time() - $this->redeliverAfterSeconds), - ]); + ], $where); + } + + private function staleClaimCutoff(): string + { + return gmdate(\Mage_Core_Model_Locale::DATETIME_FORMAT, time() - $this->redeliverAfterSeconds); } private function inFlightRowExists(string $dedupeKey): bool diff --git a/lib/Maho/Queue/WorkerFactory.php b/lib/Maho/Queue/WorkerFactory.php index 5f9807dac1..46a65a5ccc 100644 --- a/lib/Maho/Queue/WorkerFactory.php +++ b/lib/Maho/Queue/WorkerFactory.php @@ -28,12 +28,12 @@ final class WorkerFactory { /** - * @param array{limit?: ?int, memoryLimit?: ?int, stopWhenIdle?: bool} $options + * @param array{limit?: ?int, memoryLimit?: ?int, idleTimeout?: ?int, pool?: ?Pool} $options */ public static function create(array $options = []): Worker { $transportName = QueueManager::transportName(); - $transport = QueueManager::transport(); + $transport = QueueManager::workerTransport($options['pool'] ?? null); $dispatcher = new EventDispatcher(); $dispatcher->addSubscriber(new AddErrorDetailsStampListener()); @@ -70,8 +70,8 @@ public static function create(array $options = []): Worker if (isset($options['memoryLimit']) && $options['memoryLimit'] > 0) { $dispatcher->addSubscriber(new StopWorkerOnMemoryLimitListener($options['memoryLimit'])); } - if ($options['stopWhenIdle'] ?? false) { - $dispatcher->addSubscriber(new StopWorkerWhenIdleListener()); + if (isset($options['idleTimeout'])) { + $dispatcher->addSubscriber(new StopWorkerWhenIdleListener($options['idleTimeout'])); } return new Worker([$transportName => $transport], QueueManager::bus(), $dispatcher); diff --git a/lib/MahoCLI/Commands/EmailQueueProcess.php b/lib/MahoCLI/Commands/EmailQueueProcess.php index a8e6884084..d865f28062 100644 --- a/lib/MahoCLI/Commands/EmailQueueProcess.php +++ b/lib/MahoCLI/Commands/EmailQueueProcess.php @@ -47,7 +47,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int } try { - $worker = WorkerFactory::create(['stopWhenIdle' => true]); + $worker = WorkerFactory::create(['idleTimeout' => 0]); $options = []; if ($isDbTransport) { $options['queues'] = [\Mage_Core_Model_Email_Queue::QUEUE_NAME]; diff --git a/lib/MahoCLI/Commands/QueueList.php b/lib/MahoCLI/Commands/QueueList.php index 34671f2f07..3539bbfb36 100644 --- a/lib/MahoCLI/Commands/QueueList.php +++ b/lib/MahoCLI/Commands/QueueList.php @@ -11,6 +11,7 @@ use Mage; use Maho\Db\Expr; +use Maho\Queue\PoolRegistry; use Maho\Queue\QueueManager; use Maho\Queue\Transport\DbTransport; use Symfony\Component\Console\Attribute\AsCommand; @@ -68,10 +69,14 @@ protected function execute(InputInterface $input, OutputInterface $output): int } $table = new Table($output); - $table->setHeaders(['Queue', 'Pending', 'Processing', 'Failed', 'Completed', 'Oldest pending (UTC)']); + $table->setHeaders(['Queue', 'Pool', 'Pending', 'Processing', 'Failed', 'Completed', 'Oldest pending (UTC)']); + $orphaned = false; foreach ($queues as $queue => $counts) { + $pool = PoolRegistry::poolFor((string) $queue); + $orphaned = $orphaned || $pool === null; $table->addRow([ $queue, + $pool === null ? 'none' : $pool->name, $counts[DbTransport::STATUS_PENDING], $counts[DbTransport::STATUS_PROCESSING], $counts[DbTransport::STATUS_FAILED], @@ -81,6 +86,10 @@ protected function execute(InputInterface $input, OutputInterface $output): int } $table->render(); + if ($orphaned) { + $output->writeln('Queues with no pool are never consumed; mark a pool catch_all or list them under global/queue/pools.'); + } + return Command::SUCCESS; } } diff --git a/lib/MahoCLI/Commands/QueueWork.php b/lib/MahoCLI/Commands/QueueWork.php index 695cf3b109..93bb310961 100644 --- a/lib/MahoCLI/Commands/QueueWork.php +++ b/lib/MahoCLI/Commands/QueueWork.php @@ -9,6 +9,8 @@ namespace MahoCLI\Commands; +use Maho\Queue\Pool; +use Maho\Queue\PoolRegistry; use Maho\Queue\QueueManager; use Maho\Queue\WorkerFactory; use Symfony\Component\Console\Attribute\AsCommand; @@ -32,13 +34,17 @@ class QueueWork extends BaseMahoCommand implements SignalableCommandInterface protected function configure(): void { $this + ->addOption('pool', null, InputOption::VALUE_REQUIRED, 'Consume as this configured worker pool, taking its queues and limits as defaults') + ->addOption('index', null, InputOption::VALUE_REQUIRED, 'Which worker of the pool this process is, when the pool runs more than one', '0') ->addOption('queue', null, InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY, 'Only consume these queues (repeatable); default all') + ->addOption('exclude-queue', null, InputOption::VALUE_REQUIRED | InputOption::VALUE_IS_ARRAY, 'Never consume these queues (repeatable); how a catch-all worker leaves another pool its own') ->addOption('limit', null, InputOption::VALUE_REQUIRED, 'Stop after handling this many messages') ->addOption('time-limit', null, InputOption::VALUE_REQUIRED, 'Stop after this many seconds') ->addOption('memory-limit', null, InputOption::VALUE_REQUIRED, 'Stop once memory usage exceeds this limit (e.g. 256M)') ->addOption('sleep', null, InputOption::VALUE_REQUIRED, 'Seconds to sleep when the queue is empty', '1') - ->addOption('stop-when-empty', null, InputOption::VALUE_NONE, 'Stop as soon as the queue is empty') - ->addOption('exclusive', null, InputOption::VALUE_NONE, 'Hold the queue.worker lock and refuse to run when another exclusive worker is active (used by the cron watchdog)'); + ->addOption('idle-timeout', null, InputOption::VALUE_REQUIRED, 'Stop after this many seconds with nothing to do; 0 stops on the first empty poll') + ->addOption('stop-when-empty', null, InputOption::VALUE_NONE, 'Stop as soon as the queue is empty (same as --idle-timeout=0)') + ->addOption('exclusive', null, InputOption::VALUE_NONE, 'Hold the pool worker lock and refuse to run when another exclusive worker holds it (used by the cron watchdog)'); } #[\Override] @@ -46,19 +52,45 @@ protected function execute(InputInterface $input, OutputInterface $output): int { $this->initMaho(); - if ($input->getOption('exclusive') - && !\Mage::getSingleton('core/lock')->acquire(\Maho_Queue_Model_Cron::WORKER_LOCK, machineLocal: true) - ) { - $output->writeln('Another exclusive queue worker is already running'); - return Command::INVALID; + $pool = null; + $poolName = $input->getOption('pool'); + if ($poolName !== null) { + $pool = PoolRegistry::get((string) $poolName); + if ($pool === null) { + $output->writeln("Unknown queue pool: {$poolName}"); + return Command::INVALID; + } } + if ($input->getOption('exclusive')) { + $lockName = $pool?->lockName((int) $input->getOption('index')) ?? Pool::LOCK_PREFIX; + if (!\Mage::getSingleton('core/lock')->acquire($lockName, machineLocal: true)) { + $output->writeln("Another exclusive queue worker already holds {$lockName}"); + return Command::INVALID; + } + } + + // Unbounded unless asked: a hand-run worker keeps the limits it had before pools existed. + $base = $pool ?? new Pool(name: 'ad-hoc', memoryLimit: '', timeLimit: 0); + $effective = new Pool( + name: $base->name, + queues: $input->getOption('queue') ?: $base->queues, + excludedQueues: $input->getOption('exclude-queue') ?: $base->excludedQueues, + idleTimeout: match (true) { + $input->getOption('idle-timeout') !== null => max(0, (int) $input->getOption('idle-timeout')), + (bool) $input->getOption('stop-when-empty') => 0, + default => $base->idleTimeout, + }, + memoryLimit: (string) ($input->getOption('memory-limit') ?? $base->memoryLimit), + timeLimit: (int) ($input->getOption('time-limit') ?? $base->timeLimit), + redeliverAfter: $base->redeliverAfter, + ); + $memoryLimit = null; - $memoryLimitOption = $input->getOption('memory-limit'); - if ($memoryLimitOption !== null) { - $memoryLimit = $this->parseMemoryLimit((string) $memoryLimitOption); + if ($effective->memoryLimit !== '') { + $memoryLimit = $this->parseMemoryLimit($effective->memoryLimit); if ($memoryLimit === null) { - $output->writeln("Invalid memory limit: {$memoryLimitOption}"); + $output->writeln("Invalid memory limit: {$effective->memoryLimit}"); return Command::INVALID; } } @@ -66,22 +98,24 @@ protected function execute(InputInterface $input, OutputInterface $output): int $this->worker = WorkerFactory::create([ 'limit' => $input->getOption('limit') !== null ? (int) $input->getOption('limit') : null, 'memoryLimit' => $memoryLimit, - 'stopWhenIdle' => (bool) $input->getOption('stop-when-empty'), + 'idleTimeout' => $effective->idleTimeout, + 'pool' => $effective, ]); - $queues = $input->getOption('queue'); $output->writeln(sprintf( - 'Consuming messages from the %s transport%s (press Ctrl-C to stop gracefully)', + 'Consuming messages from the %s transport%s%s%s (press Ctrl-C to stop gracefully)', QueueManager::transportName(), - $queues !== [] ? ', queues: ' . implode(', ', $queues) : '', + $pool !== null ? ', pool: ' . $pool->name : '', + $effective->queues !== [] ? ', queues: ' . implode(', ', $effective->queues) : '', + $effective->excludedQueues !== [] ? ', excluding: ' . implode(', ', $effective->excludedQueues) : '', )); $options = ['sleep' => (int) $input->getOption('sleep') * 1_000_000]; - if ($input->getOption('time-limit') !== null) { - $options['time_limit'] = (int) $input->getOption('time-limit'); + if ($effective->timeLimit > 0) { + $options['time_limit'] = $effective->timeLimit; } - if ($queues !== []) { - $options['queues'] = $queues; + if ($effective->queues !== []) { + $options['queues'] = $effective->queues; } $this->worker->run($options); diff --git a/tests/Backend/Integration/Queue/CronConsumerTest.php b/tests/Backend/Integration/Queue/CronConsumerTest.php index 61f44cf76b..5ff7019c0b 100644 --- a/tests/Backend/Integration/Queue/CronConsumerTest.php +++ b/tests/Backend/Integration/Queue/CronConsumerTest.php @@ -7,11 +7,46 @@ declare(strict_types=1); +use Maho\Queue\PoolRegistry; use Maho\Queue\QueueManager; use Maho\Queue\Transport\DbTransport; uses(Tests\MahoBackendTestCase::class); +/** + * @return list "pool:index" for each worker the watchdog would start + */ +function pendingWorkers(): array +{ + return array_map( + fn(array $worker) => $worker[0]->name . ':' . $worker[1], + Mage::getModel('queue/cron')->workersToSpawn(), + ); +} + +/** + * @param callable():void $body + */ +function withAllPoolLocks(callable $body): void +{ + $lock = Mage::getSingleton('core/lock'); + $held = []; + foreach (PoolRegistry::all() as $pool) { + for ($index = 0; $index < $pool->count; $index++) { + expect($lock->acquire($pool->lockName($index), machineLocal: true))->toBeTrue(); + $held[] = $pool->lockName($index); + } + } + + try { + $body(); + } finally { + foreach ($held as $name) { + $lock->release($name); + } + } +} + beforeEach(function () { QueueManager::reset(); clearQueueTable(); @@ -22,19 +57,33 @@ QueueManager::reset(); }); -it('does not spawn a second worker while the lock is held', function () { +it('does not spawn a second worker while the pool lock is held', function () { QueueManager::dispatch(makeEmailMessage()); - $lock = Mage::getSingleton('core/lock'); - expect($lock->acquire(Maho_Queue_Model_Cron::WORKER_LOCK, machineLocal: true))->toBeTrue(); - try { + withAllPoolLocks(function () { + expect(pendingWorkers())->toBe([]); + Mage::getModel('queue/cron')->process(); $rows = fetchQueueRows(); expect($rows)->toHaveCount(1); expect($rows[0]['status'])->toBe(DbTransport::STATUS_PENDING); - } finally { - $lock->release(Maho_Queue_Model_Cron::WORKER_LOCK); - } + }); +}); + +it('keeps the resident tier running even with nothing queued', function () { + expect(pendingWorkers())->toBe(['fast:0']); +}); + +it('starts the on-demand tier only once its queues have work due', function () { + QueueManager::dispatch( + makeEmailMessage(), + delaySeconds: 7 * 86400, + queue: Mage_Newsletter_Model_Queue::QUEUE_NAME, + ); + expect(pendingWorkers())->toBe(['fast:0']); + + QueueManager::dispatch(makeEmailMessage('due now'), queue: Mage_Newsletter_Model_Queue::QUEUE_NAME); + expect(pendingWorkers())->toBe(['fast:0', 'slow:0']); }); it('removes old failed messages during cleanup', function () { diff --git a/tests/Backend/Integration/Queue/PoolTest.php b/tests/Backend/Integration/Queue/PoolTest.php new file mode 100644 index 0000000000..ce6cf3b9ca --- /dev/null +++ b/tests/Backend/Integration/Queue/PoolTest.php @@ -0,0 +1,231 @@ + + * SPDX-License-Identifier: OSL-3.0 + */ + +declare(strict_types=1); + +use Maho\Queue\Pool; +use Maho\Queue\PoolRegistry; +use Maho\Queue\QueueManager; +use Maho\Queue\StopWorkerWhenIdleListener; +use Maho\Queue\Transport\DbTransport; +use Symfony\Component\Messenger\Event\WorkerRunningEvent; +use Symfony\Component\Messenger\Worker; + +uses(Tests\MahoBackendTestCase::class); + +function queuePool(string $name): Pool +{ + return PoolRegistry::get($name) ?? throw new RuntimeException("pool {$name} is missing"); +} + +function poolTransport(string $name): DbTransport +{ + $transport = QueueManager::workerTransport(queuePool($name)); + assert($transport instanceof DbTransport); + + return $transport; +} + +/** + * Merge extra queue config the way another module's config.xml would, run the + * assertions, then take it back out again. + * + * @param callable():void $body + */ +function withQueueConfig(string $xml, callable $body): void +{ + $node = Mage::getConfig()->getNode('global/queue'); + $node->extend(new Maho\Simplexml\Element($xml), true); + QueueManager::reset(); + + try { + $body(); + } finally { + foreach (new Maho\Simplexml\Element($xml) as $section => $children) { + foreach (array_keys((array) $children->children()) as $name) { + unset($node->{$section}->{$name}); + } + } + QueueManager::reset(); + } +} + +function insertQueueRow(string $queue, string $status, ?string $claimedAt = null): void +{ + $now = Mage_Core_Model_Locale::nowUtc(); + queueAdapter()->insert(QueueManager::tableName(), [ + 'queue' => $queue, + 'status' => $status, + 'message_class' => Mage_Core_Model_Email_SendMessage::class, + 'body' => serialize(makeEmailMessage()), + 'available_at' => $now, + 'claimed_at' => $claimedAt, + 'created_at' => $now, + 'updated_at' => $now, + ]); +} + +/** + * A Worker that only records stop(), so the idle listener can be driven + * directly without a transport behind it. + */ +class RecordingWorker extends Worker +{ + public int $stopped = 0; + + public function __construct() {} + + #[\Override] + public function stop(): void + { + $this->stopped++; + } +} + +beforeEach(function () { + QueueManager::reset(); + clearQueueTable(); +}); + +afterEach(function () { + clearQueueTable(); + QueueManager::reset(); +}); + +it('ships a resident fast tier and an on-demand slow catch-all', function () { + expect(array_keys(PoolRegistry::all()))->toBe(['fast', 'slow']); + expect(queuePool('fast')->queues)->toBe([Mage_Core_Model_Email_Queue::QUEUE_NAME]); + expect(queuePool('fast')->isOnDemand())->toBeFalse(); + expect(queuePool('slow')->queues)->toBe([]); + expect(queuePool('slow')->isOnDemand())->toBeTrue(); +}); + +it('keeps the catch-all off the queues another pool claims', function () { + expect(queuePool('slow')->excludedQueues)->toBe([Mage_Core_Model_Email_Queue::QUEUE_NAME]); + expect(queuePool('slow')->consumes(Mage_Core_Model_Email_Queue::QUEUE_NAME))->toBeFalse(); + expect(queuePool('slow')->consumes(Mage_Newsletter_Model_Queue::QUEUE_NAME))->toBeTrue(); +}); + +it('routes an unclassified queue to the slow tier rather than leaving it unconsumed', function () { + expect(PoolRegistry::poolFor('some_third_party_queue')?->name)->toBe('slow'); + expect(PoolRegistry::poolFor(DbTransport::DEFAULT_QUEUE)?->name)->toBe('slow'); +}); + +it('lets another module route its own queue without clobbering the existing map', function () { + withQueueConfig('fast', function () { + expect(queuePool('fast')->queues)->toBe([Mage_Core_Model_Email_Queue::QUEUE_NAME, 'zz_scratch']); + expect(queuePool('slow')->excludedQueues)->toContain('zz_scratch'); + }); +}); + +it('hands a queue routed to an unknown pool back to the catch-all', function () { + withQueueConfig('nowhere', function () { + expect(PoolRegistry::poolFor('zz_scratch')?->name)->toBe('slow'); + }); +}); + +it('skips a pool with nothing routed to it instead of letting it rival the catch-all', function () { + withQueueConfig('5', function () { + expect(array_keys(PoolRegistry::all()))->toBe(['fast', 'slow']); + }); +}); + +it('does not hand the catch-all worker a message belonging to another pool', function () { + QueueManager::dispatch(makeEmailMessage(), queue: Mage_Core_Model_Email_Queue::QUEUE_NAME); + QueueManager::dispatch(makeEmailMessage('newsletter batch'), queue: Mage_Newsletter_Model_Queue::QUEUE_NAME); + + expect(iterator_to_array(poolTransport('slow')->get()))->toHaveCount(1); + + $processing = array_values(array_filter( + fetchQueueRows(), + fn($row) => $row['status'] === DbTransport::STATUS_PROCESSING, + )); + expect($processing)->toHaveCount(1); + expect($processing[0]['queue'])->toBe(Mage_Newsletter_Model_Queue::QUEUE_NAME); +}); + +it('counts only work that is due, not a campaign scheduled for later', function () { + // The regression this guards: scheduleSending() queues a campaign as a + // long-delayed message, so a watchdog probing raw pending counts would + // respawn the on-demand worker every cron tick until its send date. + QueueManager::dispatch( + makeEmailMessage(), + delaySeconds: 7 * 86400, + queue: Mage_Newsletter_Model_Queue::QUEUE_NAME, + ); + + expect(poolTransport('slow')->getMessageCount())->toBe(1); + expect(poolTransport('slow')->countDue())->toBe(0); +}); + +it('counts a message abandoned by a dead worker as due', function () { + insertQueueRow( + Mage_Newsletter_Model_Queue::QUEUE_NAME, + DbTransport::STATUS_PROCESSING, + gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 4 * 3600), + ); + + // Without this the row is invisible to the probe, nothing respawns, and the + // message is stranded for good instead of for one redelivery window. + expect(poolTransport('slow')->countDue())->toBe(1); +}); + +it('leaves a claim still inside the pool redelivery window alone', function () { + insertQueueRow( + Mage_Newsletter_Model_Queue::QUEUE_NAME, + DbTransport::STATUS_PROCESSING, + gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 1800), + ); + + // Slow allows 3h, so a 30-minute-old claim is a running feed, not a corpse. + expect(poolTransport('slow')->countDue())->toBe(0); +}); + +it('does not let a fast worker requeue a slow job running under a longer window', function () { + insertQueueRow( + Mage_Newsletter_Model_Queue::QUEUE_NAME, + DbTransport::STATUS_PROCESSING, + gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 1800), + ); + + // Fast redelivers after 15 minutes, but the claim is on a queue it does not + // own; requeueing it would run the handler a second time alongside the first. + iterator_to_array(poolTransport('fast')->getFromQueues(queuePool('fast')->queues)); + + $rows = fetchQueueRows(); + expect($rows)->toHaveCount(1); + expect($rows[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); +}); + +it('stops an idle worker immediately when no grace period is set', function () { + $worker = new RecordingWorker(); + (new StopWorkerWhenIdleListener(0))->onWorkerRunning(new WorkerRunningEvent($worker, true)); + + expect($worker->stopped)->toBe(1); +}); + +it('holds an idle worker open for the grace period', function () { + $worker = new RecordingWorker(); + $listener = new StopWorkerWhenIdleListener(3600); + + $listener->onWorkerRunning(new WorkerRunningEvent($worker, true)); + $listener->onWorkerRunning(new WorkerRunningEvent($worker, true)); + + expect($worker->stopped)->toBe(0); +}); + +it('restarts the grace period when work arrives', function () { + $worker = new RecordingWorker(); + $listener = new StopWorkerWhenIdleListener(1); + + $listener->onWorkerRunning(new WorkerRunningEvent($worker, true)); + sleep(2); + $listener->onWorkerRunning(new WorkerRunningEvent($worker, false)); + $listener->onWorkerRunning(new WorkerRunningEvent($worker, true)); + + expect($worker->stopped)->toBe(0); +}); From 6e7b33fe4351cb8b3f13cacea97f9a092bf41a85 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Sat, 8 Aug 2026 19:37:50 +0100 Subject: [PATCH 02/13] Fixed queue pool fallback, ad-hoc redelivery window and on-demand worker count - PoolRegistry::build() falls back to the default pool when every declared pool is dropped, instead of leaving the queue with no workers - queue:work without --pool uses the widest redelivery window in play, so it cannot requeue another pool's in-flight claim early - On-demand pools spawn at most one worker per due message - Corrected the queue:list orphan hint and the undefined newsletter queue constant in the pool tests --- app/code/core/Maho/Queue/Model/Cron.php | 14 ++++++++----- lib/Maho/Queue/PoolRegistry.php | 21 ++++++++++++++++++- lib/MahoCLI/Commands/QueueList.php | 2 +- lib/MahoCLI/Commands/QueueWork.php | 20 +++++++++++++++++- .../Integration/Queue/CronConsumerTest.php | 4 ++-- tests/Backend/Integration/Queue/PoolTest.php | 20 +++++++++--------- 6 files changed, 61 insertions(+), 20 deletions(-) diff --git a/app/code/core/Maho/Queue/Model/Cron.php b/app/code/core/Maho/Queue/Model/Cron.php index 3aaa3c4fb9..48702c72f0 100644 --- a/app/code/core/Maho/Queue/Model/Cron.php +++ b/app/code/core/Maho/Queue/Model/Cron.php @@ -52,30 +52,34 @@ public function workersToSpawn(): array $spawn = []; foreach (PoolRegistry::all() as $pool) { - $hasWork = null; + $due = null; + $started = 0; for ($index = 0; $index < $pool->count; $index++) { if ($lock->isHeld($pool->lockName($index), machineLocal: true)) { continue; } + // One process per due message: an on-demand pool holding its + // whole roster open for a single message would idle them all out. if ($pool->isOnDemand()) { - $hasWork ??= $this->hasDueWork($pool); - if (!$hasWork) { + $due ??= $this->dueWorkCount($pool); + if ($started >= $due) { break; } } $spawn[] = [$pool, $index]; + $started++; } } return $spawn; } - private function hasDueWork(Pool $pool): bool + private function dueWorkCount(Pool $pool): int { $transport = QueueManager::workerTransport($pool); // Redis cannot be probed per queue; spawn and let the worker idle out. - return !$transport instanceof DbTransport || $transport->countDue($pool->queues) > 0; + return $transport instanceof DbTransport ? $transport->countDue($pool->queues) : PHP_INT_MAX; } #[Maho\Config\CronJob('queue_clean_up', schedule: '0 2 * * *')] diff --git a/lib/Maho/Queue/PoolRegistry.php b/lib/Maho/Queue/PoolRegistry.php index cf12581060..25db66b121 100644 --- a/lib/Maho/Queue/PoolRegistry.php +++ b/lib/Maho/Queue/PoolRegistry.php @@ -51,6 +51,22 @@ public static function poolFor(string $queue): ?Pool return null; } + /** + * The longest redelivery window any pool declares, or null when none + * overrides the store default. A worker that consumes every queue must not + * requeue a claim sooner than the pool owning it would, or the handler runs + * a second time alongside the first. + */ + public static function widestRedeliveryWindow(): ?int + { + $windows = array_filter( + array_map(static fn(Pool $pool): ?int => $pool->redeliverAfter, self::all()), + static fn(?int $window): bool => $window !== null, + ); + + return $windows === [] ? null : max($windows); + } + public static function reset(): void { self::$pools = null; @@ -148,7 +164,10 @@ private static function build(): array ); } - return $pools; + // Every declared pool was dropped (none marked catch_all, none routed a + // queue): without a fallback the watchdog would start nothing at all and + // the queue would stall silently. + return $pools === [] ? [self::FALLBACK_POOL => new Pool(self::FALLBACK_POOL)] : $pools; } /** diff --git a/lib/MahoCLI/Commands/QueueList.php b/lib/MahoCLI/Commands/QueueList.php index 3539bbfb36..7e9e464584 100644 --- a/lib/MahoCLI/Commands/QueueList.php +++ b/lib/MahoCLI/Commands/QueueList.php @@ -87,7 +87,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $table->render(); if ($orphaned) { - $output->writeln('Queues with no pool are never consumed; mark a pool catch_all or list them under global/queue/pools.'); + $output->writeln('Queues with no pool are never consumed; mark a pool catch_all under global/queue/pools or route them under global/queue/routing.'); } return Command::SUCCESS; diff --git a/lib/MahoCLI/Commands/QueueWork.php b/lib/MahoCLI/Commands/QueueWork.php index 93bb310961..0bce349a13 100644 --- a/lib/MahoCLI/Commands/QueueWork.php +++ b/lib/MahoCLI/Commands/QueueWork.php @@ -71,7 +71,12 @@ protected function execute(InputInterface $input, OutputInterface $output): int } // Unbounded unless asked: a hand-run worker keeps the limits it had before pools existed. - $base = $pool ?? new Pool(name: 'ad-hoc', memoryLimit: '', timeLimit: 0); + $base = $pool ?? new Pool( + name: 'ad-hoc', + memoryLimit: '', + timeLimit: 0, + redeliverAfter: $this->adHocRedeliveryWindow(), + ); $effective = new Pool( name: $base->name, queues: $input->getOption('queue') ?: $base->queues, @@ -141,6 +146,19 @@ public function handleSignal(int $signal, int|false $previousExitCode = 0): int| return false; } + /** + * A worker with no pool consumes every queue, so it must not requeue a claim + * before the pool owning that queue would: take the widest window in play. + */ + private function adHocRedeliveryWindow(): ?int + { + $widest = PoolRegistry::widestRedeliveryWindow(); + + return $widest === null + ? null + : max($widest, (int) \Mage::getStoreConfig(QueueManager::XML_PATH_REDELIVER_AFTER)); + } + private function parseMemoryLimit(string $limit): ?int { if (!preg_match('/^(\d+)([KMG]?)$/i', trim($limit), $matches)) { diff --git a/tests/Backend/Integration/Queue/CronConsumerTest.php b/tests/Backend/Integration/Queue/CronConsumerTest.php index 5ff7019c0b..3f23ae164c 100644 --- a/tests/Backend/Integration/Queue/CronConsumerTest.php +++ b/tests/Backend/Integration/Queue/CronConsumerTest.php @@ -78,11 +78,11 @@ function withAllPoolLocks(callable $body): void QueueManager::dispatch( makeEmailMessage(), delaySeconds: 7 * 86400, - queue: Mage_Newsletter_Model_Queue::QUEUE_NAME, + queue: 'newsletter', ); expect(pendingWorkers())->toBe(['fast:0']); - QueueManager::dispatch(makeEmailMessage('due now'), queue: Mage_Newsletter_Model_Queue::QUEUE_NAME); + QueueManager::dispatch(makeEmailMessage('due now'), queue: 'newsletter'); expect(pendingWorkers())->toBe(['fast:0', 'slow:0']); }); diff --git a/tests/Backend/Integration/Queue/PoolTest.php b/tests/Backend/Integration/Queue/PoolTest.php index ce6cf3b9ca..fc3601f65b 100644 --- a/tests/Backend/Integration/Queue/PoolTest.php +++ b/tests/Backend/Integration/Queue/PoolTest.php @@ -107,7 +107,7 @@ public function stop(): void it('keeps the catch-all off the queues another pool claims', function () { expect(queuePool('slow')->excludedQueues)->toBe([Mage_Core_Model_Email_Queue::QUEUE_NAME]); expect(queuePool('slow')->consumes(Mage_Core_Model_Email_Queue::QUEUE_NAME))->toBeFalse(); - expect(queuePool('slow')->consumes(Mage_Newsletter_Model_Queue::QUEUE_NAME))->toBeTrue(); + expect(queuePool('slow')->consumes('newsletter'))->toBeTrue(); }); it('routes an unclassified queue to the slow tier rather than leaving it unconsumed', function () { @@ -136,7 +136,7 @@ public function stop(): void it('does not hand the catch-all worker a message belonging to another pool', function () { QueueManager::dispatch(makeEmailMessage(), queue: Mage_Core_Model_Email_Queue::QUEUE_NAME); - QueueManager::dispatch(makeEmailMessage('newsletter batch'), queue: Mage_Newsletter_Model_Queue::QUEUE_NAME); + QueueManager::dispatch(makeEmailMessage('newsletter batch'), queue: 'newsletter'); expect(iterator_to_array(poolTransport('slow')->get()))->toHaveCount(1); @@ -145,17 +145,17 @@ public function stop(): void fn($row) => $row['status'] === DbTransport::STATUS_PROCESSING, )); expect($processing)->toHaveCount(1); - expect($processing[0]['queue'])->toBe(Mage_Newsletter_Model_Queue::QUEUE_NAME); + expect($processing[0]['queue'])->toBe('newsletter'); }); it('counts only work that is due, not a campaign scheduled for later', function () { - // The regression this guards: scheduleSending() queues a campaign as a - // long-delayed message, so a watchdog probing raw pending counts would - // respawn the on-demand worker every cron tick until its send date. + // The regression this guards: a campaign queued as a long-delayed message + // would make a watchdog probing raw pending counts respawn the on-demand + // worker every cron tick until its send date. QueueManager::dispatch( makeEmailMessage(), delaySeconds: 7 * 86400, - queue: Mage_Newsletter_Model_Queue::QUEUE_NAME, + queue: 'newsletter', ); expect(poolTransport('slow')->getMessageCount())->toBe(1); @@ -164,7 +164,7 @@ public function stop(): void it('counts a message abandoned by a dead worker as due', function () { insertQueueRow( - Mage_Newsletter_Model_Queue::QUEUE_NAME, + 'newsletter', DbTransport::STATUS_PROCESSING, gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 4 * 3600), ); @@ -176,7 +176,7 @@ public function stop(): void it('leaves a claim still inside the pool redelivery window alone', function () { insertQueueRow( - Mage_Newsletter_Model_Queue::QUEUE_NAME, + 'newsletter', DbTransport::STATUS_PROCESSING, gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 1800), ); @@ -187,7 +187,7 @@ public function stop(): void it('does not let a fast worker requeue a slow job running under a longer window', function () { insertQueueRow( - Mage_Newsletter_Model_Queue::QUEUE_NAME, + 'newsletter', DbTransport::STATUS_PROCESSING, gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 1800), ); From ab3acaaa1716c00104c96e8cfad0f2b901b63a39 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Sun, 9 Aug 2026 09:57:25 +0100 Subject: [PATCH 03/13] Detected a dead queue worker from its lock instead of a timer A worker stamps claimed_by (machine:lock-name) as it claims a message, and only while it holds its machine-local worker lock. The kernel frees that flock however the process ends, so a claim from this machine is settled by reading the lock rather than the clock: free means the worker died and the row is requeued at once, held means the handler is still running and the row is never touched, however long it takes. redeliver_after stays as the fallback for a claim from another server, whose lock is not visible here. That removes both failure modes of the timer on a single-server install. Recovery no longer waits out the window, and a handler slower than the window is no longer requeued underneath itself and run a second time. Dropped the fast pool's 900s override. It only existed to reach a crashed worker sooner, which the lock now does immediately, and it silently ignored system/queue/redeliver_after for transactional email. Removed the Redis transport. It had no test coverage, was an optional suggest rather than a dependency, and each of its seven branch points disabled a queue feature: worker pools, the admin grid listing, queue:list, the on-demand spawn probe, redeliver_after and completed_retention. It still needed the database table for failures, so it ran alongside the database rather than in place of it. Also fixed, from review of the earlier commits: the watchdog counted only newly spawned workers against an on-demand pool's due budget, it ignored the poolless lock a hand-run exclusive worker takes, getMessageCount() skipped the queue filter, an out-of-range --index took a lock nobody probes, and the Redis pool notice logged on every install because core ships two pools. --- app/code/core/Maho/Queue/Model/Cron.php | 31 +++-- app/code/core/Maho/Queue/Model/Message.php | 1 + .../controllers/Adminhtml/QueueController.php | 6 - app/code/core/Maho/Queue/etc/config.xml | 1 - app/code/core/Maho/Queue/etc/system.xml | 2 +- app/code/core/Maho/Queue/sql/schema.php | 2 + .../template/maho/queue/message/view.phtml | 4 + app/etc/local.xml.template | 10 -- app/locale/en_US/Maho_Queue.csv | 4 +- composer.json | 1 - lib/Maho/Queue/PoolRegistry.php | 13 -- lib/Maho/Queue/QueueManager.php | 124 +++++------------- lib/Maho/Queue/Transport/DbTransport.php | 68 ++++++++-- lib/Maho/Queue/WorkerFactory.php | 14 +- lib/Maho/Queue/WorkerIdentity.php | 86 ++++++++++++ lib/MahoCLI/Commands/EmailConfigShow.php | 14 +- lib/MahoCLI/Commands/EmailQueueProcess.php | 30 ++--- lib/MahoCLI/Commands/QueueList.php | 6 - lib/MahoCLI/Commands/QueueWork.php | 19 ++- .../Integration/Queue/CronConsumerTest.php | 37 ++++++ .../Integration/Queue/DbTransportTest.php | 2 +- tests/Backend/Integration/Queue/PoolTest.php | 103 ++++++++++++--- 22 files changed, 362 insertions(+), 216 deletions(-) create mode 100644 lib/Maho/Queue/WorkerIdentity.php diff --git a/app/code/core/Maho/Queue/Model/Cron.php b/app/code/core/Maho/Queue/Model/Cron.php index 48702c72f0..296a3297cf 100644 --- a/app/code/core/Maho/Queue/Model/Cron.php +++ b/app/code/core/Maho/Queue/Model/Cron.php @@ -49,25 +49,39 @@ public function process(): void public function workersToSpawn(): array { $lock = Mage::getSingleton('core/lock'); + + // A hand-run `queue:work --exclusive` consumes every queue, so it stands + // in for the whole roster and the watchdog keeps out of its way. + if ($lock->isHeld(Pool::LOCK_PREFIX, machineLocal: true)) { + return []; + } + $spawn = []; foreach (PoolRegistry::all() as $pool) { $due = null; - $started = 0; + $live = 0; + $free = []; for ($index = 0; $index < $pool->count; $index++) { if ($lock->isHeld($pool->lockName($index), machineLocal: true)) { - continue; + $live++; + } else { + $free[] = $index; } - // One process per due message: an on-demand pool holding its - // whole roster open for a single message would idle them all out. + } + + foreach ($free as $index) { + // One process per due message, workers already alive included: an + // on-demand pool holding its whole roster open for a single + // message would idle them all out. if ($pool->isOnDemand()) { $due ??= $this->dueWorkCount($pool); - if ($started >= $due) { + if ($live >= $due) { break; } } $spawn[] = [$pool, $index]; - $started++; + $live++; } } @@ -76,10 +90,7 @@ public function workersToSpawn(): array private function dueWorkCount(Pool $pool): int { - $transport = QueueManager::workerTransport($pool); - - // Redis cannot be probed per queue; spawn and let the worker idle out. - return $transport instanceof DbTransport ? $transport->countDue($pool->queues) : PHP_INT_MAX; + return QueueManager::workerTransport($pool)->countDue($pool->queues); } #[Maho\Config\CronJob('queue_clean_up', schedule: '0 2 * * *')] diff --git a/app/code/core/Maho/Queue/Model/Message.php b/app/code/core/Maho/Queue/Model/Message.php index 41a5ada4a8..d3097f3495 100644 --- a/app/code/core/Maho/Queue/Model/Message.php +++ b/app/code/core/Maho/Queue/Model/Message.php @@ -22,6 +22,7 @@ * @method int getRetries() * @method string getAvailableAt() * @method ?string getClaimedAt() + * @method ?string getClaimedBy() * @method ?string getProcessedAt() * @method string getCreatedAt() * @method string getUpdatedAt() diff --git a/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php b/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php index 41760f98b8..ff60399ca6 100644 --- a/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php +++ b/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php @@ -54,12 +54,6 @@ protected function _initAction(): static #[Maho\Config\Route('/admin/queue')] public function indexAction(): void { - if (QueueManager::transportName() === QueueManager::TRANSPORT_REDIS) { - Mage::getSingleton('adminhtml/session')->addNotice( - Mage::helper('queue')->__('The Redis transport is active: pending messages live in Redis and are not listed here, only failures are.'), - ); - } - $this->_title(Mage::helper('queue')->__('Message Queue')); $this->_initAction(); $this->renderLayout(); diff --git a/app/code/core/Maho/Queue/etc/config.xml b/app/code/core/Maho/Queue/etc/config.xml index ade1951ddc..9e12648185 100644 --- a/app/code/core/Maho/Queue/etc/config.xml +++ b/app/code/core/Maho/Queue/etc/config.xml @@ -52,7 +52,6 @@ - 900 10 diff --git a/app/code/core/Maho/Queue/etc/system.xml b/app/code/core/Maho/Queue/etc/system.xml index 8d09fdf0c7..c36050a190 100644 --- a/app/code/core/Maho/Queue/etc/system.xml +++ b/app/code/core/Maho/Queue/etc/system.xml @@ -57,7 +57,7 @@ SPDX-License-Identifier: AFL-3.0 0 0 required-entry validate-digits validate-greater-than-zero - Messages claimed by a worker that died are re-queued after this long. Keep above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume. + A worker that dies on this server is detected from its lock and its messages are re-queued at once, so this timer only covers a worker on another server, whose lock is not visible here. Keep it above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume. diff --git a/app/code/core/Maho/Queue/sql/schema.php b/app/code/core/Maho/Queue/sql/schema.php index 5a519e0274..b19d4593be 100644 --- a/app/code/core/Maho/Queue/sql/schema.php +++ b/app/code/core/Maho/Queue/sql/schema.php @@ -26,6 +26,8 @@ $message->addColumn('dedupe_key', Types::STRING, ['length' => 64, 'notnull' => false]); $message->addColumn('available_at', Types::DATETIME_MUTABLE, []); $message->addColumn('claimed_at', Types::DATETIME_MUTABLE, ['notnull' => false]); + // Worker holding the claim, as machine:lock-name; crash recovery reads its lock. + $message->addColumn('claimed_by', Types::STRING, ['length' => 128, 'notnull' => false]); $message->addColumn('processed_at', Types::DATETIME_MUTABLE, ['notnull' => false]); $message->addColumn('created_at', Types::DATETIME_MUTABLE, ['default' => new CurrentTimestamp()]); // Transport keeps updated_at current on every write; the on-update diff --git a/app/design/adminhtml/default/default/template/maho/queue/message/view.phtml b/app/design/adminhtml/default/default/template/maho/queue/message/view.phtml index 60cd5a2c53..3e885701e2 100644 --- a/app/design/adminhtml/default/default/template/maho/queue/message/view.phtml +++ b/app/design/adminhtml/default/default/template/maho/queue/message/view.phtml @@ -51,6 +51,10 @@ $statusOptions = Maho_Queue_Model_Message::getStatusOptions(); __('Processed (UTC)') ?> escapeHtml($message->getProcessedAt() ?: '-') ?> + + __('Claimed By') ?> + escapeHtml($message->getClaimedBy() ?: '-') ?> + getErrorMessage()): ?> diff --git a/app/etc/local.xml.template b/app/etc/local.xml.template index 6880375f73..99beecc97a 100644 --- a/app/etc/local.xml.template +++ b/app/etc/local.xml.template @@ -46,16 +46,6 @@ SPDX-License-Identifier: AFL-3.0 db --> - - {{admin_frontname}} diff --git a/app/locale/en_US/Maho_Queue.csv b/app/locale/en_US/Maho_Queue.csv index a54d325d50..db25fb8e8a 100644 --- a/app/locale/en_US/Maho_Queue.csv +++ b/app/locale/en_US/Maho_Queue.csv @@ -2,6 +2,7 @@ "%s message(s) re-queued.","%s message(s) re-queued." "0 keeps failed messages forever.","0 keeps failed messages forever." "0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days.","0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days." +"A worker that dies on this server is detected from its lock and its messages are re-queued at once, so this timer only covers a worker on another server, whose lock is not visible here. Keep it above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume.","A worker that dies on this server is detected from its lock and its messages are re-queued at once, so this timer only covers a worker on another server, whose lock is not visible here. Keep it above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume." "Action","Action" "Available","Available" "Available (UTC)","Available (UTC)" @@ -9,6 +10,7 @@ "Background message processing. Maho cron keeps one detached ""queue:work"" worker running per pool: a resident ""fast"" worker so short jobs never queue behind long ones, plus a ""slow"" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools.","Background message processing. Maho cron keeps one detached ""queue:work"" worker running per pool: a resident ""fast"" worker so short jobs never queue behind long ones, plus a ""slow"" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools." "Body (serialized)","Body (serialized)" "Claimed (UTC)","Claimed (UTC)" +"Claimed By","Claimed By" "Completed","Completed" "Discard","Discard" "Discard Messages","Discard Messages" @@ -30,7 +32,6 @@ "Message not found.","Message not found." "Message Queue","Message Queue" "Message re-queued.","Message re-queued." -"Messages claimed by a worker that died are re-queued after this long. Keep above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume.","Messages claimed by a worker that died are re-queued after this long. Keep above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume." "Only failed messages can be retried.","Only failed messages can be retried." "Pending","Pending" "Permanently delete the selected messages?","Permanently delete the selected messages?" @@ -48,7 +49,6 @@ "Retry Delay Multiplier","Retry Delay Multiplier" "Retry Messages","Retry Messages" "Status","Status" -"The Redis transport is active: pending messages live in Redis and are not listed here, only failures are.","The Redis transport is active: pending messages live in Redis and are not listed here, only failures are." "Upper bound for the backoff. 0 means no bound.","Upper bound for the backoff. 0 means no bound." "View","View" "View Messages","View Messages" diff --git a/composer.json b/composer.json index 7ed7e39160..ea14fa5644 100644 --- a/composer.json +++ b/composer.json @@ -117,7 +117,6 @@ "mahocommerce/module-braintree": "Braintree payment gateway integration", "nyholm/psr7": "Required (together with symfony/mcp-bundle) for the MCP protocol at /api/mcp: supplies the PSR-17 factories its HTTP transport discovers. Only a require-dev of mcp-bundle, so nothing pulls it in for you. Any psr/http-factory-implementation works.", "picqer/php-barcode-generator": "Required for gift card barcode generation", - "symfony/redis-messenger": "Enables the Redis transport for the Maho message queue when global/queue/dsn is set in app/etc/local.xml", "symfony/twig-bundle": "Required (together with symfony/asset) to render the API Platform Swagger UI, ReDoc, and GraphiQL explorer pages. Without it, the JSON API and /api/docs.json still work but the human-browsable docs at /api/docs do not.", "symfony/asset": "Required (together with symfony/twig-bundle) for the API Platform Swagger UI: provides the asset() function its template uses.", "symfony/mcp-bundle": "Required (together with nyholm/psr7) for the MCP protocol at /api/mcp: exposes the REST v2 surface to AI agents. Brings mcp/sdk with it. Without both the toggle in System > Config > Services > API stays inert." diff --git a/lib/Maho/Queue/PoolRegistry.php b/lib/Maho/Queue/PoolRegistry.php index 25db66b121..3e610d0296 100644 --- a/lib/Maho/Queue/PoolRegistry.php +++ b/lib/Maho/Queue/PoolRegistry.php @@ -80,19 +80,6 @@ private static function build(): array $node = \Mage::getConfig()->getNode('global/queue'); $poolsNode = $node !== false && isset($node->pools) ? $node->pools : false; - // Symfony's Redis transport is not a QueueReceiverInterface, so Worker::run() - // rejects any queue filter: on Redis a single catch-all worker is all we can run. - if (QueueManager::transportName() === QueueManager::TRANSPORT_REDIS) { - if ($poolsNode !== false && $poolsNode->children()->count() > 1) { - \Mage::log( - 'Queue pools are ignored on the Redis transport (it cannot filter by queue name); running a single worker over all queues', - \Mage::LOG_NOTICE, - ); - } - - return [self::FALLBACK_POOL => new Pool(self::FALLBACK_POOL)]; - } - $definitions = []; if ($poolsNode !== false) { foreach ($poolsNode->children() as $name => $child) { diff --git a/lib/Maho/Queue/QueueManager.php b/lib/Maho/Queue/QueueManager.php index a60591ac7f..191387a94e 100644 --- a/lib/Maho/Queue/QueueManager.php +++ b/lib/Maho/Queue/QueueManager.php @@ -22,7 +22,6 @@ use Symfony\Component\Messenger\Stamp\DelayStamp; use Symfony\Component\Messenger\Stamp\StampInterface; use Symfony\Component\Messenger\Transport\Sender\SendersLocator; -use Symfony\Component\Messenger\Transport\TransportInterface; /** * Entry point of the Maho message queue: dispatch a message object and a @@ -32,16 +31,14 @@ * \Maho\Queue\QueueManager::dispatch(new My_Module_Model_SomeMessage(...)); * ``` * - * The default transport stores messages in the maho_queue_message table; a - * `redis://...` node under `` in - * app/etc/local.xml switches to Redis (requires symfony/redis-messenger). - * With the DB transport, dispatching inside a database transaction - * participates in it: the message becomes visible only on commit. + * Messages are stored in the maho_queue_message table, so dispatching inside a + * database transaction participates in it: the message becomes visible only on + * commit. */ final class QueueManager { + /** The name this transport is registered under with Messenger. */ public const TRANSPORT_DB = 'db'; - public const TRANSPORT_REDIS = 'redis'; public const XML_PATH_MAX_RETRIES = 'system/queue/max_retries'; public const XML_PATH_RETRY_DELAY = 'system/queue/retry_delay'; @@ -52,10 +49,8 @@ final class QueueManager public const XML_PATH_FAILED_RETENTION = 'system/queue/failed_retention'; private static ?MessageBus $bus = null; - private static ?TransportInterface $transport = null; private static ?DbTransport $dbTransport = null; private static ?Serializer $serializer = null; - private static ?string $transportName = null; /** * Dispatch a message for asynchronous handling. @@ -93,41 +88,13 @@ public static function bus(): MessageBusInterface return self::$bus ??= new MessageBus([ new AddBusNameStampMiddleware('maho'), new SendMessageMiddleware(new SendersLocator( - ['*' => [self::transportName()]], - new ServiceLocator([self::transportName() => self::transport()]), + ['*' => [self::TRANSPORT_DB]], + new ServiceLocator([self::TRANSPORT_DB => self::dbTransport()]), )), new HandleMessageMiddleware(HandlerRegistry::handlersLocator()), ]); } - public static function transport(): TransportInterface - { - if (self::$transport !== null) { - return self::$transport; - } - - if (self::transportName() === self::TRANSPORT_REDIS) { - $factory = new \Symfony\Component\Messenger\Bridge\Redis\Transport\RedisTransportFactory(); // @phpstan-ignore class.notFound - return self::$transport = $factory->createTransport((string) self::redisDsn(), [], self::serializer()); // @phpstan-ignore class.notFound - } - - return self::$transport = self::dbTransport(); - } - - public static function transportName(): string - { - if (self::$transportName !== null) { - return self::$transportName; - } - - $dsn = self::redisDsn(); - if ($dsn !== null && !class_exists('Symfony\Component\Messenger\Bridge\Redis\Transport\RedisTransportFactory')) { - throw new \RuntimeException('global/queue/dsn is set in app/etc/local.xml but the symfony/redis-messenger package is not installed; run "composer require symfony/redis-messenger" or remove the node'); - } - - return self::$transportName = $dsn !== null ? self::TRANSPORT_REDIS : self::TRANSPORT_DB; - } - public static function dbTransport(): DbTransport { return self::$dbTransport ??= new DbTransport( @@ -141,17 +108,19 @@ public static function dbTransport(): DbTransport /** * The transport a pool's worker consumes from: the shared one unless the - * pool narrows what it sees or overrides the redelivery window, in which - * case it gets its own instance. Redis cannot be narrowed at all, so pools - * fall back to consuming everything there. + * pool narrows what it sees, overrides the redelivery window, or the caller + * claims under a worker id, in which case it gets its own instance. + * + * @param ?string $workerId Only a pool worker holding its lock passes one; see WorkerIdentity */ - public static function workerTransport(?Pool $pool = null): TransportInterface + public static function workerTransport(?Pool $pool = null, ?string $workerId = null): DbTransport { - if ($pool === null - || self::transportName() === self::TRANSPORT_REDIS - || ($pool->excludedQueues === [] && $pool->redeliverAfter === null) - ) { - return self::transport(); + if ($pool === null) { + return self::dbTransport(); + } + + if ($workerId === null && $pool->excludedQueues === [] && $pool->redeliverAfter === null) { + return self::dbTransport(); } return new DbTransport( @@ -161,6 +130,7 @@ public static function workerTransport(?Pool $pool = null): TransportInterface $pool->redeliverAfter ?? (int) \Mage::getStoreConfig(self::XML_PATH_REDELIVER_AFTER), (int) \Mage::getStoreConfig(self::XML_PATH_COMPLETED_RETENTION), $pool->excludedQueues, + $workerId, ); } @@ -170,11 +140,9 @@ public static function serializer(): Serializer } /** - * Re-queue a stored failed message from the admin grid or CLI. DB mode - * flips the row back to pending with a fresh retry budget; Redis failure - * rows are re-dispatched through the bus and the stored row removed. - * Only failed rows are retryable: flipping a claimed row would race the - * worker's ack. + * Re-queue a stored failed message from the admin grid or CLI, flipping the + * row back to pending with a fresh retry budget. Only failed rows are + * retryable: flipping a claimed row would race the worker's ack. */ public static function retryStoredMessage(int $messageId): bool { @@ -187,29 +155,20 @@ public static function retryStoredMessage(int $messageId): bool return false; } - if (self::transportName() === self::TRANSPORT_DB) { - $now = \Mage_Core_Model_Locale::nowUtc(); - return $adapter->update($table, [ - 'status' => DbTransport::STATUS_PENDING, - 'retries' => 0, - 'available_at' => $now, - 'claimed_at' => null, - 'processed_at' => null, - 'updated_at' => $now, - ], [ - 'message_id = ?' => $messageId, - 'status = ?' => DbTransport::STATUS_FAILED, - ]) === 1; - } - - $envelope = self::serializer()->decode([ - 'body' => (string) $row['body'], - 'headers' => ['type' => (string) $row['message_class']], - ]); - self::bus()->dispatch($envelope->getMessage(), [new QueueNameStamp((string) $row['queue'])]); - $adapter->delete($table, ['message_id = ?' => $messageId]); - - return true; + $now = \Mage_Core_Model_Locale::nowUtc(); + + return $adapter->update($table, [ + 'status' => DbTransport::STATUS_PENDING, + 'retries' => 0, + 'available_at' => $now, + 'claimed_at' => null, + 'claimed_by' => null, + 'processed_at' => null, + 'updated_at' => $now, + ], [ + 'message_id = ?' => $messageId, + 'status = ?' => DbTransport::STATUS_FAILED, + ]) === 1; } public static function discardStoredMessage(int $messageId): bool @@ -228,25 +187,12 @@ public static function tableName(): string public static function reset(): void { self::$bus = null; - self::$transport = null; self::$dbTransport = null; self::$serializer = null; - self::$transportName = null; HandlerRegistry::reset(); PoolRegistry::reset(); } - private static function redisDsn(): ?string - { - $node = \Mage::getConfig()->getNode('global/queue/dsn'); - if ($node === false) { - return null; - } - $dsn = trim((string) $node); - - return $dsn === '' ? null : $dsn; - } - private static function writeAdapter(): \Maho\Db\Adapter\AdapterInterface { return \Mage::getSingleton('core/resource')->getConnection('core_write'); diff --git a/lib/Maho/Queue/Transport/DbTransport.php b/lib/Maho/Queue/Transport/DbTransport.php index a324b93f1f..9b854ece6d 100644 --- a/lib/Maho/Queue/Transport/DbTransport.php +++ b/lib/Maho/Queue/Transport/DbTransport.php @@ -12,6 +12,7 @@ use Maho\Db\Adapter\AdapterInterface; use Maho\Queue\Stamp\DedupeKeyStamp; use Maho\Queue\Stamp\QueueNameStamp; +use Maho\Queue\WorkerIdentity; use Symfony\Component\Messenger\Envelope; use Symfony\Component\Messenger\Exception\MessageDecodingFailedException; use Symfony\Component\Messenger\Exception\TransportException; @@ -49,6 +50,7 @@ final class DbTransport implements TransportInterface, QueueReceiverInterface, L /** * @param list $excludedQueues Queues this instance never consumes, so a pool worker can be "everything but" + * @param ?string $workerId Stamped on every claim; set only by a worker holding its lock, so a free lock proves the claim is orphaned */ public function __construct( private readonly AdapterInterface $adapter, @@ -57,6 +59,7 @@ public function __construct( private readonly int $redeliverAfterSeconds, private readonly int $completedRetentionDays, private readonly array $excludedQueues = [], + private readonly ?string $workerId = null, ) {} #[\Override] @@ -70,7 +73,7 @@ public function send(Envelope $envelope): Envelope $messageIdStamp = $envelope->last(TransportMessageIdStamp::class); $redeliveryStamp = $envelope->last(RedeliveryStamp::class); - // A failure-transport send carries both stamps too, but its id is a Redis stream id: insert, not update. + // A failure-transport send carries both stamps too, but its id belongs to the origin transport: insert, not update. if ($messageIdStamp !== null && $redeliveryStamp !== null && $envelope->last(SentToFailureTransportStamp::class) === null) { $this->adapter->update($this->table, [ @@ -79,6 +82,7 @@ public function send(Envelope $envelope): Envelope 'available_at' => $availableAt, 'error_message' => $envelope->last(ErrorDetailsStamp::class)?->getExceptionMessage(), 'claimed_at' => null, + 'claimed_by' => null, 'updated_at' => $now, ], ['message_id = ?' => (int) $messageIdStamp->getId()]); @@ -103,6 +107,7 @@ public function send(Envelope $envelope): Envelope 'dedupe_key' => $dedupeKey, 'available_at' => $availableAt, 'claimed_at' => null, + 'claimed_by' => null, 'processed_at' => $isFailure ? $now : null, 'created_at' => $now, 'updated_at' => $now, @@ -186,11 +191,12 @@ public function find(mixed $id): ?Envelope #[\Override] public function getMessageCount(): int { - return (int) $this->adapter->fetchOne( - $this->adapter->select() - ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) - ->where('status = ?', self::STATUS_PENDING), - ); + $select = $this->adapter->select() + ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) + ->where('status = ?', self::STATUS_PENDING); + $this->applyQueueFilter($select, null); + + return (int) $this->adapter->fetchOne($select); } /** @@ -206,9 +212,10 @@ public function countDue(?array $queues = null): int $clauses = ['(' . $this->adapter->quoteInto('status = ?', self::STATUS_PENDING) . ' AND ' . $this->adapter->quoteInto('available_at <= ?', \Mage_Core_Model_Locale::nowUtc()) . ')']; - if ($this->redeliverAfterSeconds > 0) { + $abandoned = $this->abandonedClaimClause(); + if ($abandoned !== null) { $clauses[] = '(' . $this->adapter->quoteInto('status = ?', self::STATUS_PROCESSING) - . ' AND ' . $this->adapter->quoteInto('claimed_at < ?', $this->staleClaimCutoff()) . ')'; + . ' AND ' . $abandoned . ')'; } $select = $this->adapter->select() @@ -258,6 +265,7 @@ private function claimNext(?array $queues): array $claimed = $this->adapter->update($this->table, [ 'status' => self::STATUS_PROCESSING, 'claimed_at' => $now, + 'claimed_by' => $this->workerId, 'updated_at' => $now, ], [ 'message_id = ?' => (int) $row['message_id'], @@ -278,8 +286,8 @@ private function claimNext(?array $queues): array } /** - * Crash recovery: rows claimed longer ago than redeliver_after belong to a - * worker that died without ack/reject; put them back up for grabs. + * Crash recovery: a row still processing whose worker died without ack or + * reject goes back up for grabs. * * Scoped to the queues this instance consumes, because pools carry their own * window: a fast worker must not requeue a feed a slow worker is still running. @@ -288,13 +296,14 @@ private function claimNext(?array $queues): array */ private function requeueStaleClaims(?array $queues): void { - if ($this->redeliverAfterSeconds <= 0) { + $abandoned = $this->abandonedClaimClause(); + if ($abandoned === null) { return; } $where = [ 'status = ?' => self::STATUS_PROCESSING, - 'claimed_at < ?' => $this->staleClaimCutoff(), + $abandoned, ]; if ($queues !== null && $queues !== []) { $where['queue IN (?)'] = $queues; @@ -306,10 +315,45 @@ private function requeueStaleClaims(?array $queues): void $this->adapter->update($this->table, [ 'status' => self::STATUS_PENDING, 'claimed_at' => null, + 'claimed_by' => null, 'updated_at' => \Mage_Core_Model_Locale::nowUtc(), ], $where); } + /** + * Rows whose claiming worker is gone, to AND with `status = processing`. + * + * A claim from this machine is settled by its worker lock, not by the clock: + * a free lock proves the process died, a held one proves the handler is + * still running however long it takes. Claims from another machine have no + * lock to read here, so they keep the redeliver_after timer, as do rows with + * no id (mid-upgrade, or a worker running without --exclusive). + */ + private function abandonedClaimClause(): ?string + { + if ($this->redeliverAfterSeconds <= 0) { + return null; + } + + $clauses = []; + + $dead = WorkerIdentity::deadLocalIds(); + if ($dead !== []) { + $clauses[] = $this->adapter->quoteInto('claimed_by IN (?)', $dead); + } + + $timer = $this->adapter->quoteInto('claimed_at < ?', $this->staleClaimCutoff()); + $local = WorkerIdentity::localIds(); + if ($local !== []) { + $timer .= ' AND (claimed_by IS NULL OR ' + . $this->adapter->quoteInto('claimed_by NOT IN (?)', $local) . ')'; + } + $clauses[] = $timer; + + // Wrapped whole: the caller ANDs this with a status and a queue filter. + return '((' . implode(') OR (', $clauses) . '))'; + } + private function staleClaimCutoff(): string { return gmdate(\Mage_Core_Model_Locale::DATETIME_FORMAT, time() - $this->redeliverAfterSeconds); diff --git a/lib/Maho/Queue/WorkerFactory.php b/lib/Maho/Queue/WorkerFactory.php index 46a65a5ccc..c3d73b31fe 100644 --- a/lib/Maho/Queue/WorkerFactory.php +++ b/lib/Maho/Queue/WorkerFactory.php @@ -14,7 +14,6 @@ use Symfony\Component\Messenger\EventListener\AddErrorDetailsStampListener; use Symfony\Component\Messenger\EventListener\DispatchPcntlSignalListener; use Symfony\Component\Messenger\EventListener\SendFailedMessageForRetryListener; -use Symfony\Component\Messenger\EventListener\SendFailedMessageToFailureTransportListener; use Symfony\Component\Messenger\EventListener\StopWorkerOnMemoryLimitListener; use Symfony\Component\Messenger\EventListener\StopWorkerOnMessageLimitListener; use Symfony\Component\Messenger\Retry\MultiplierRetryStrategy; @@ -28,12 +27,12 @@ final class WorkerFactory { /** - * @param array{limit?: ?int, memoryLimit?: ?int, idleTimeout?: ?int, pool?: ?Pool} $options + * @param array{limit?: ?int, memoryLimit?: ?int, idleTimeout?: ?int, pool?: ?Pool, workerId?: ?string} $options */ public static function create(array $options = []): Worker { - $transportName = QueueManager::transportName(); - $transport = QueueManager::workerTransport($options['pool'] ?? null); + $transportName = QueueManager::TRANSPORT_DB; + $transport = QueueManager::workerTransport($options['pool'] ?? null, $options['workerId'] ?? null); $dispatcher = new EventDispatcher(); $dispatcher->addSubscriber(new AddErrorDetailsStampListener()); @@ -49,13 +48,6 @@ public static function create(array $options = []): Worker )]), )); - if ($transportName === QueueManager::TRANSPORT_REDIS) { - // Final failures on Redis land in the DB table so the admin grid sees them. - $dispatcher->addSubscriber(new SendFailedMessageToFailureTransportListener( - new ServiceLocator([$transportName => QueueManager::dbTransport()]), - )); - } - $dispatcher->addListener(WorkerMessageFailedEvent::class, static function (WorkerMessageFailedEvent $event): void { if (!$event->willRetry()) { \Mage::logException($event->getThrowable()); diff --git a/lib/Maho/Queue/WorkerIdentity.php b/lib/Maho/Queue/WorkerIdentity.php new file mode 100644 index 0000000000..de2dabb814 --- /dev/null +++ b/lib/Maho/Queue/WorkerIdentity.php @@ -0,0 +1,86 @@ + + * SPDX-License-Identifier: OSL-3.0 + */ + +declare(strict_types=1); + +namespace Maho\Queue; + +/** + * Identifies the worker holding a claim, as machine:lock-name, so crash + * recovery can read the worker's lock instead of guessing from elapsed time. + * A worker stamps this on `claimed_by` only while it holds that lock, and the + * kernel frees the flock however the process ends. + * + * The machine part is the hashed hostname: two servers on one database must + * never share an id, or each would probe its own locks for the other's claims. + */ +final class WorkerIdentity +{ + private static ?string $machine = null; + + /** Null when the hostname is unknown, which falls back to the timer. */ + public static function forLock(string $lockName): ?string + { + $machine = self::machine(); + + return $machine === null ? null : $machine . ':' . $lockName; + } + + /** + * Every id a worker on this machine can hold, alive or not. + * + * @return list + */ + public static function localIds(): array + { + return array_values(array_filter(array_map(self::forLock(...), self::lockNames()))); + } + + /** + * The ids in localIds() whose lock nobody holds, so whose process is gone. + * + * @return list + */ + public static function deadLocalIds(): array + { + $lock = \Mage::getSingleton('core/lock'); + $free = array_filter( + self::lockNames(), + static fn(string $name): bool => !$lock->isHeld($name, machineLocal: true), + ); + + return array_values(array_filter(array_map(self::forLock(...), $free))); + } + + /** + * One per pool worker, plus the poolless lock a hand-run + * `queue:work --exclusive` takes. + * + * @return list + */ + private static function lockNames(): array + { + $names = [Pool::LOCK_PREFIX]; + foreach (PoolRegistry::all() as $pool) { + for ($index = 0; $index < $pool->count; $index++) { + $names[] = $pool->lockName($index); + } + } + + return $names; + } + + private static function machine(): ?string + { + if (self::$machine === null) { + $hostname = gethostname(); + self::$machine = $hostname === false ? '' : substr(md5($hostname), 0, 12); + } + + return self::$machine === '' ? null : self::$machine; + } +} diff --git a/lib/MahoCLI/Commands/EmailConfigShow.php b/lib/MahoCLI/Commands/EmailConfigShow.php index fd8b6df715..4c9de81b4f 100644 --- a/lib/MahoCLI/Commands/EmailConfigShow.php +++ b/lib/MahoCLI/Commands/EmailConfigShow.php @@ -63,15 +63,11 @@ protected function execute(InputInterface $input, OutputInterface $output): int $table->addRow(['Return Path', $returnPathSetting]); // Queue System Information - if (\Maho\Queue\QueueManager::transportName() === \Maho\Queue\QueueManager::TRANSPORT_REDIS) { - $pendingDisplay = 'Unknown (pending messages live in Redis)'; - } else { - $pendingCount = Mage::getModel('queue/message')->getCollection() - ->addFieldToFilter('queue', \Mage_Core_Model_Email_Queue::QUEUE_NAME) - ->addFieldToFilter('status', \Maho_Queue_Model_Message::STATUS_PENDING) - ->getSize(); - $pendingDisplay = $pendingCount . ' emails'; - } + $pendingCount = Mage::getModel('queue/message')->getCollection() + ->addFieldToFilter('queue', \Mage_Core_Model_Email_Queue::QUEUE_NAME) + ->addFieldToFilter('status', \Maho_Queue_Model_Message::STATUS_PENDING) + ->getSize(); + $pendingDisplay = $pendingCount . ' emails'; // Get cron schedules dynamically $cronHelper = Mage::helper('cron'); diff --git a/lib/MahoCLI/Commands/EmailQueueProcess.php b/lib/MahoCLI/Commands/EmailQueueProcess.php index d865f28062..2e2b77537c 100644 --- a/lib/MahoCLI/Commands/EmailQueueProcess.php +++ b/lib/MahoCLI/Commands/EmailQueueProcess.php @@ -10,7 +10,6 @@ namespace MahoCLI\Commands; use Mage; -use Maho\Queue\QueueManager; use Maho\Queue\WorkerFactory; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Command\Command; @@ -34,32 +33,21 @@ protected function execute(InputInterface $input, OutputInterface $output): int return Command::FAILURE; } - $isDbTransport = QueueManager::transportName() === QueueManager::TRANSPORT_DB; - if ($isDbTransport) { - $pendingCount = $this->getPendingCount(); - if ($pendingCount === 0) { - $output->writeln('No emails in queue to process.'); - return Command::SUCCESS; - } - $output->writeln("Processing email queue ({$pendingCount} emails pending)..."); - } else { - $output->writeln('Redis transport is active: consuming all queues until empty.'); + $pendingCount = $this->getPendingCount(); + if ($pendingCount === 0) { + $output->writeln('No emails in queue to process.'); + return Command::SUCCESS; } + $output->writeln("Processing email queue ({$pendingCount} emails pending)..."); try { $worker = WorkerFactory::create(['idleTimeout' => 0]); - $options = []; - if ($isDbTransport) { - $options['queues'] = [\Mage_Core_Model_Email_Queue::QUEUE_NAME]; - } - $worker->run($options); + $worker->run(['queues' => [\Mage_Core_Model_Email_Queue::QUEUE_NAME]]); $output->writeln('Queue processing completed.'); - if ($isDbTransport) { - $failedCount = $this->getFailedCount(); - if ($failedCount > 0) { - $output->writeln("{$failedCount} email(s) are in failed state; inspect them in System > Tools > Message Queue or with ./maho queue:list."); - } + $failedCount = $this->getFailedCount(); + if ($failedCount > 0) { + $output->writeln("{$failedCount} email(s) are in failed state; inspect them in System > Tools > Message Queue or with ./maho queue:list."); } return Command::SUCCESS; diff --git a/lib/MahoCLI/Commands/QueueList.php b/lib/MahoCLI/Commands/QueueList.php index 7e9e464584..f1b50db2d4 100644 --- a/lib/MahoCLI/Commands/QueueList.php +++ b/lib/MahoCLI/Commands/QueueList.php @@ -57,12 +57,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int } ksort($queues); - $transportName = QueueManager::transportName(); - $output->writeln("Active transport: {$transportName}"); - if ($transportName === QueueManager::TRANSPORT_REDIS) { - $output->writeln('Pending messages live in Redis; the counts below only cover messages stored in the database (failures).'); - } - if ($queues === []) { $output->writeln('The queue is empty.'); return Command::SUCCESS; diff --git a/lib/MahoCLI/Commands/QueueWork.php b/lib/MahoCLI/Commands/QueueWork.php index 0bce349a13..d69a69e9f8 100644 --- a/lib/MahoCLI/Commands/QueueWork.php +++ b/lib/MahoCLI/Commands/QueueWork.php @@ -13,6 +13,7 @@ use Maho\Queue\PoolRegistry; use Maho\Queue\QueueManager; use Maho\Queue\WorkerFactory; +use Maho\Queue\WorkerIdentity; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Command\SignalableCommandInterface; @@ -62,12 +63,24 @@ protected function execute(InputInterface $input, OutputInterface $output): int } } + $index = (int) $input->getOption('index'); + if ($pool !== null && ($index < 0 || $index >= $pool->count)) { + // Out of range takes a lock the watchdog never probes, so it would + // spawn a duplicate worker for the index this one is impersonating. + $output->writeln("Pool {$pool->name} runs {$pool->count} worker(s); --index must be 0.." . ($pool->count - 1) . ''); + return Command::INVALID; + } + + // Only a worker holding its lock may stamp claims: without one, a free + // lock would make every claim look orphaned the moment it is taken. + $workerId = null; if ($input->getOption('exclusive')) { - $lockName = $pool?->lockName((int) $input->getOption('index')) ?? Pool::LOCK_PREFIX; + $lockName = $pool?->lockName($index) ?? Pool::LOCK_PREFIX; if (!\Mage::getSingleton('core/lock')->acquire($lockName, machineLocal: true)) { $output->writeln("Another exclusive queue worker already holds {$lockName}"); return Command::INVALID; } + $workerId = WorkerIdentity::forLock($lockName); } // Unbounded unless asked: a hand-run worker keeps the limits it had before pools existed. @@ -105,11 +118,11 @@ protected function execute(InputInterface $input, OutputInterface $output): int 'memoryLimit' => $memoryLimit, 'idleTimeout' => $effective->idleTimeout, 'pool' => $effective, + 'workerId' => $workerId, ]); $output->writeln(sprintf( - 'Consuming messages from the %s transport%s%s%s (press Ctrl-C to stop gracefully)', - QueueManager::transportName(), + 'Consuming messages%s%s%s (press Ctrl-C to stop gracefully)', $pool !== null ? ', pool: ' . $pool->name : '', $effective->queues !== [] ? ', queues: ' . implode(', ', $effective->queues) : '', $effective->excludedQueues !== [] ? ', excluding: ' . implode(', ', $effective->excludedQueues) : '', diff --git a/tests/Backend/Integration/Queue/CronConsumerTest.php b/tests/Backend/Integration/Queue/CronConsumerTest.php index 3f23ae164c..9e4a41b22f 100644 --- a/tests/Backend/Integration/Queue/CronConsumerTest.php +++ b/tests/Backend/Integration/Queue/CronConsumerTest.php @@ -7,6 +7,7 @@ declare(strict_types=1); +use Maho\Queue\Pool; use Maho\Queue\PoolRegistry; use Maho\Queue\QueueManager; use Maho\Queue\Transport\DbTransport; @@ -86,6 +87,42 @@ function withAllPoolLocks(callable $body): void expect(pendingWorkers())->toBe(['fast:0', 'slow:0']); }); +it('stands aside for a hand-run exclusive worker', function () { + QueueManager::dispatch(makeEmailMessage()); + + $lock = Mage::getSingleton('core/lock'); + expect($lock->acquire(Pool::LOCK_PREFIX, machineLocal: true))->toBeTrue(); + try { + expect(pendingWorkers())->toBe([]); + } finally { + $lock->release(Pool::LOCK_PREFIX); + } +}); + +it('counts the on-demand workers already alive against the due budget', function () { + $node = Mage::getConfig()->getNode('global/queue'); + $node->extend(new Maho\Simplexml\Element('3'), true); + QueueManager::reset(); + + $lock = Mage::getSingleton('core/lock'); + $slow = PoolRegistry::get('slow'); + expect($slow?->count)->toBe(3); + $held = $slow->lockName(0); + expect($lock->acquire($held, machineLocal: true))->toBeTrue(); + + try { + QueueManager::dispatch(makeEmailMessage('due now'), queue: 'newsletter'); + + // One message, one worker already consuming it: a second process would + // boot only to idle straight back out. + expect(pendingWorkers())->toBe(['fast:0']); + } finally { + $lock->release($held); + unset($node->pools->slow->count); + QueueManager::reset(); + } +}); + it('removes old failed messages during cleanup', function () { $now = Mage_Core_Model_Locale::nowUtc(); queueAdapter()->insert(QueueManager::tableName(), [ diff --git a/tests/Backend/Integration/Queue/DbTransportTest.php b/tests/Backend/Integration/Queue/DbTransportTest.php index 45a9f596b2..1755586d60 100644 --- a/tests/Backend/Integration/Queue/DbTransportTest.php +++ b/tests/Backend/Integration/Queue/DbTransportTest.php @@ -101,7 +101,7 @@ $envelope = (new Envelope(makeEmailMessage()))->with( new TransportMessageIdStamp('1712345678901-0'), new RedeliveryStamp(0), - new SentToFailureTransportStamp('redis'), + new SentToFailureTransportStamp('origin'), ErrorDetailsStamp::create(new RuntimeException('handler blew up')), ); diff --git a/tests/Backend/Integration/Queue/PoolTest.php b/tests/Backend/Integration/Queue/PoolTest.php index fc3601f65b..ee010af2f7 100644 --- a/tests/Backend/Integration/Queue/PoolTest.php +++ b/tests/Backend/Integration/Queue/PoolTest.php @@ -12,6 +12,7 @@ use Maho\Queue\QueueManager; use Maho\Queue\StopWorkerWhenIdleListener; use Maho\Queue\Transport\DbTransport; +use Maho\Queue\WorkerIdentity; use Symfony\Component\Messenger\Event\WorkerRunningEvent; use Symfony\Component\Messenger\Worker; @@ -22,14 +23,41 @@ function queuePool(string $name): Pool return PoolRegistry::get($name) ?? throw new RuntimeException("pool {$name} is missing"); } -function poolTransport(string $name): DbTransport +function poolTransport(string $name, ?string $workerId = null): DbTransport { - $transport = QueueManager::workerTransport(queuePool($name)); + $transport = QueueManager::workerTransport(queuePool($name), $workerId); assert($transport instanceof DbTransport); return $transport; } +function poolWorkerId(string $name, int $index = 0): string +{ + return WorkerIdentity::forLock(queuePool($name)->lockName($index)) + ?? throw new RuntimeException('this host has no worker identity'); +} + +/** + * Hold a pool's worker lock for the duration of the assertions, the way a live + * `queue:work --exclusive` holds it. + * + * @param callable():void $body + */ +function withWorkerLock(string $name, callable $body): void +{ + $lock = Mage::getSingleton('core/lock'); + $lockName = queuePool($name)->lockName(); + if (!$lock->acquire($lockName, machineLocal: true)) { + throw new RuntimeException("could not take {$lockName}"); + } + + try { + $body(); + } finally { + $lock->release($lockName); + } +} + /** * Merge extra queue config the way another module's config.xml would, run the * assertions, then take it back out again. @@ -54,7 +82,7 @@ function withQueueConfig(string $xml, callable $body): void } } -function insertQueueRow(string $queue, string $status, ?string $claimedAt = null): void +function insertQueueRow(string $queue, string $status, ?string $claimedAt = null, ?string $claimedBy = null): void { $now = Mage_Core_Model_Locale::nowUtc(); queueAdapter()->insert(QueueManager::tableName(), [ @@ -64,11 +92,17 @@ function insertQueueRow(string $queue, string $status, ?string $claimedAt = null 'body' => serialize(makeEmailMessage()), 'available_at' => $now, 'claimed_at' => $claimedAt, + 'claimed_by' => $claimedBy, 'created_at' => $now, 'updated_at' => $now, ]); } +function agoUtc(int $seconds): string +{ + return gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - $seconds); +} + /** * A Worker that only records stop(), so the idle listener can be driven * directly without a transport behind it. @@ -163,36 +197,24 @@ public function stop(): void }); it('counts a message abandoned by a dead worker as due', function () { - insertQueueRow( - 'newsletter', - DbTransport::STATUS_PROCESSING, - gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 4 * 3600), - ); + insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(4 * 3600)); // Without this the row is invisible to the probe, nothing respawns, and the // message is stranded for good instead of for one redelivery window. expect(poolTransport('slow')->countDue())->toBe(1); }); -it('leaves a claim still inside the pool redelivery window alone', function () { - insertQueueRow( - 'newsletter', - DbTransport::STATUS_PROCESSING, - gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 1800), - ); +it('leaves an unattributed claim still inside the pool redelivery window alone', function () { + insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(1800)); // Slow allows 3h, so a 30-minute-old claim is a running feed, not a corpse. expect(poolTransport('slow')->countDue())->toBe(0); }); it('does not let a fast worker requeue a slow job running under a longer window', function () { - insertQueueRow( - 'newsletter', - DbTransport::STATUS_PROCESSING, - gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 1800), - ); + insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(7200)); - // Fast redelivers after 15 minutes, but the claim is on a queue it does not + // Fast redelivers after an hour, but the claim is on a queue it does not // own; requeueing it would run the handler a second time alongside the first. iterator_to_array(poolTransport('fast')->getFromQueues(queuePool('fast')->queues)); @@ -201,6 +223,47 @@ public function stop(): void expect($rows[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); }); +it('stamps the claiming worker on the row', function () { + QueueManager::dispatch(makeEmailMessage('feed'), queue: 'newsletter'); + $workerId = poolWorkerId('slow'); + + expect(iterator_to_array(poolTransport('slow', $workerId)->get()))->toHaveCount(1); + expect(fetchQueueRows()[0]['claimed_by'])->toBe($workerId); +}); + +it('reclaims a local worker crash at once, without waiting out the window', function () { + // Fresh claim, nowhere near the 3h window, but nobody holds slow's lock, so + // the process that took it is provably gone. + insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(5), poolWorkerId('slow')); + + expect(poolTransport('slow')->countDue())->toBe(1); + expect(iterator_to_array(poolTransport('slow')->get()))->toHaveCount(1); +}); + +it('never requeues a claim whose local worker still holds its lock', function () { + // The duplicate this guards: a handler slower than the window used to be + // requeued underneath itself and run a second time alongside the first. + withWorkerLock('slow', function () { + insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(4 * 3600), poolWorkerId('slow')); + + expect(poolTransport('slow')->countDue())->toBe(0); + expect(iterator_to_array(poolTransport('slow')->get()))->toHaveCount(0); + expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); + }); +}); + +it('falls back to the window for a claim held by another server', function () { + // No lock of ours to read, so the timer is all this machine has to go on. + $elsewhere = 'ffffffffffff:' . queuePool('slow')->lockName(); + + insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(1800), $elsewhere); + expect(poolTransport('slow')->countDue())->toBe(0); + + clearQueueTable(); + insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(4 * 3600), $elsewhere); + expect(poolTransport('slow')->countDue())->toBe(1); +}); + it('stops an idle worker immediately when no grace period is set', function () { $worker = new RecordingWorker(); (new StopWorkerWhenIdleListener(0))->onWorkerRunning(new WorkerRunningEvent($worker, true)); From f684aa1042cbf0d6492dfcb96c3f81f79dc48e5f Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Sun, 9 Aug 2026 10:00:06 +0100 Subject: [PATCH 04/13] lint --- app/locale/en_US/Maho_Queue.csv | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/locale/en_US/Maho_Queue.csv b/app/locale/en_US/Maho_Queue.csv index db25fb8e8a..06a6fa4b63 100644 --- a/app/locale/en_US/Maho_Queue.csv +++ b/app/locale/en_US/Maho_Queue.csv @@ -2,10 +2,10 @@ "%s message(s) re-queued.","%s message(s) re-queued." "0 keeps failed messages forever.","0 keeps failed messages forever." "0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days.","0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days." -"A worker that dies on this server is detected from its lock and its messages are re-queued at once, so this timer only covers a worker on another server, whose lock is not visible here. Keep it above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume.","A worker that dies on this server is detected from its lock and its messages are re-queued at once, so this timer only covers a worker on another server, whose lock is not visible here. Keep it above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume." "Action","Action" "Available","Available" "Available (UTC)","Available (UTC)" +"A worker that dies on this server is detected from its lock and its messages are re-queued at once, so this timer only covers a worker on another server, whose lock is not visible here. Keep it above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume.","A worker that dies on this server is detected from its lock and its messages are re-queued at once, so this timer only covers a worker on another server, whose lock is not visible here. Keep it above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume." "Back","Back" "Background message processing. Maho cron keeps one detached ""queue:work"" worker running per pool: a resident ""fast"" worker so short jobs never queue behind long ones, plus a ""slow"" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools.","Background message processing. Maho cron keeps one detached ""queue:work"" worker running per pool: a resident ""fast"" worker so short jobs never queue behind long ones, plus a ""slow"" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools." "Body (serialized)","Body (serialized)" From 64c482ac26968137e0290d6f1712d9216981963a Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Sun, 9 Aug 2026 10:26:18 +0100 Subject: [PATCH 05/13] Stopped re-delivering a queue message a dead worker left claimed A claim is now parked, not redelivered. Nothing infers a worker's death from elapsed time, so no handler is ever started a second time by a clock. Inferring death from a timer is wrong in both directions: it waits out the window before recovering, and it re-queues a handler that is merely slower than the window, running it alongside the first. The lock-based version of this traded the second fault for a third, since a hung worker holds its lock forever and would strand the message with no way back. The message is not lost. The row keeps its body, queue and claimed_at, and the admin grid lists it, so recovery is an operator decision: Retry now accepts a claimed row as well as a failed one, and Discard already took any row. That is the whole of the recovery path. Removed with it: claimed_by and WorkerIdentity, requeueStaleClaims(), redeliver_after and its admin field, the per-pool override, PoolRegistry::widestRedeliveryWindow() and QueueWork::adHocRedeliveryWindow(). countDue() is back to pending-and-due, so the watchdog no longer respawns an on-demand worker for a claim no worker will pick up. Note the queue is at-most-once for a crash only. The retry-on-exception path is untouched, so a handler that throws is still re-run up to max_retries. --- .../Queue/Block/Adminhtml/Message/Grid.php | 2 +- .../Queue/Block/Adminhtml/Message/View.php | 9 +- app/code/core/Maho/Queue/Model/Message.php | 1 - .../controllers/Adminhtml/QueueController.php | 2 +- app/code/core/Maho/Queue/etc/config.xml | 2 - app/code/core/Maho/Queue/etc/system.xml | 9 -- app/code/core/Maho/Queue/sql/schema.php | 2 - .../template/maho/queue/message/view.phtml | 4 - app/locale/en_US/Maho_Queue.csv | 23 ++-- lib/Maho/Queue/Pool.php | 4 +- lib/Maho/Queue/PoolRegistry.php | 17 --- lib/Maho/Queue/QueueManager.php | 31 ++---- lib/Maho/Queue/Transport/DbTransport.php | 101 +---------------- lib/Maho/Queue/WorkerFactory.php | 4 +- lib/Maho/Queue/WorkerIdentity.php | 86 --------------- lib/MahoCLI/Commands/QueueWork.php | 28 +---- .../Integration/Queue/DbTransportTest.php | 19 +++- tests/Backend/Integration/Queue/PoolTest.php | 103 ++---------------- 18 files changed, 61 insertions(+), 386 deletions(-) delete mode 100644 lib/Maho/Queue/WorkerIdentity.php diff --git a/app/code/core/Maho/Queue/Block/Adminhtml/Message/Grid.php b/app/code/core/Maho/Queue/Block/Adminhtml/Message/Grid.php index e91a70f264..e75f67ebf1 100644 --- a/app/code/core/Maho/Queue/Block/Adminhtml/Message/Grid.php +++ b/app/code/core/Maho/Queue/Block/Adminhtml/Message/Grid.php @@ -114,7 +114,7 @@ protected function _prepareMassaction(): static $this->getMassactionBlock()->addItem('retry', [ 'label' => $helper->__('Retry'), 'url' => $this->getUrl('*/*/massRetry'), - 'confirm' => $helper->__('Re-queue the selected failed messages?'), + 'confirm' => $helper->__('Re-queue the selected messages?'), ]); $this->getMassactionBlock()->addItem('discard', [ diff --git a/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php b/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php index 930334b9f6..468be4cc15 100644 --- a/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php +++ b/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php @@ -30,8 +30,15 @@ public function getDiscardUrl(): string return $this->getUrlSecure('*/*/discard', ['id' => $this->getMessage()?->getId()]); } + /** + * Failed, plus a claim a dead worker left behind: nothing re-queues those + * automatically, so the grid is the only way back. + */ public function isRetryable(): bool { - return $this->getMessage()?->getStatus() === Maho_Queue_Model_Message::STATUS_FAILED; + return in_array($this->getMessage()?->getStatus(), [ + Maho_Queue_Model_Message::STATUS_FAILED, + Maho_Queue_Model_Message::STATUS_PROCESSING, + ], true); } } diff --git a/app/code/core/Maho/Queue/Model/Message.php b/app/code/core/Maho/Queue/Model/Message.php index d3097f3495..41a5ada4a8 100644 --- a/app/code/core/Maho/Queue/Model/Message.php +++ b/app/code/core/Maho/Queue/Model/Message.php @@ -22,7 +22,6 @@ * @method int getRetries() * @method string getAvailableAt() * @method ?string getClaimedAt() - * @method ?string getClaimedBy() * @method ?string getProcessedAt() * @method string getCreatedAt() * @method string getUpdatedAt() diff --git a/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php b/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php index ff60399ca6..0e5936c7a5 100644 --- a/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php +++ b/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php @@ -98,7 +98,7 @@ public function retryAction(): void if (QueueManager::retryStoredMessage($id)) { Mage::getSingleton('adminhtml/session')->addSuccess(Mage::helper('queue')->__('Message re-queued.')); } else { - Mage::getSingleton('adminhtml/session')->addError(Mage::helper('queue')->__('Only failed messages can be retried.')); + Mage::getSingleton('adminhtml/session')->addError(Mage::helper('queue')->__('Only failed or stuck messages can be retried.')); } $this->_redirect('*/*/'); } diff --git a/app/code/core/Maho/Queue/etc/config.xml b/app/code/core/Maho/Queue/etc/config.xml index 9e12648185..167e8885be 100644 --- a/app/code/core/Maho/Queue/etc/config.xml +++ b/app/code/core/Maho/Queue/etc/config.xml @@ -58,7 +58,6 @@ 1 60 512M - 10800 20 @@ -91,7 +90,6 @@ 60 4 21600 - 3600 0 30 diff --git a/app/code/core/Maho/Queue/etc/system.xml b/app/code/core/Maho/Queue/etc/system.xml index c36050a190..d4cf0a4a07 100644 --- a/app/code/core/Maho/Queue/etc/system.xml +++ b/app/code/core/Maho/Queue/etc/system.xml @@ -50,15 +50,6 @@ SPDX-License-Identifier: AFL-3.0 required-entry validate-digits validate-zero-or-greater Upper bound for the backoff. 0 means no bound. - - - 80 - 1 - 0 - 0 - required-entry validate-digits validate-greater-than-zero - A worker that dies on this server is detected from its lock and its messages are re-queued at once, so this timer only covers a worker on another server, whose lock is not visible here. Keep it above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume. - 90 diff --git a/app/code/core/Maho/Queue/sql/schema.php b/app/code/core/Maho/Queue/sql/schema.php index b19d4593be..5a519e0274 100644 --- a/app/code/core/Maho/Queue/sql/schema.php +++ b/app/code/core/Maho/Queue/sql/schema.php @@ -26,8 +26,6 @@ $message->addColumn('dedupe_key', Types::STRING, ['length' => 64, 'notnull' => false]); $message->addColumn('available_at', Types::DATETIME_MUTABLE, []); $message->addColumn('claimed_at', Types::DATETIME_MUTABLE, ['notnull' => false]); - // Worker holding the claim, as machine:lock-name; crash recovery reads its lock. - $message->addColumn('claimed_by', Types::STRING, ['length' => 128, 'notnull' => false]); $message->addColumn('processed_at', Types::DATETIME_MUTABLE, ['notnull' => false]); $message->addColumn('created_at', Types::DATETIME_MUTABLE, ['default' => new CurrentTimestamp()]); // Transport keeps updated_at current on every write; the on-update diff --git a/app/design/adminhtml/default/default/template/maho/queue/message/view.phtml b/app/design/adminhtml/default/default/template/maho/queue/message/view.phtml index 3e885701e2..60cd5a2c53 100644 --- a/app/design/adminhtml/default/default/template/maho/queue/message/view.phtml +++ b/app/design/adminhtml/default/default/template/maho/queue/message/view.phtml @@ -51,10 +51,6 @@ $statusOptions = Maho_Queue_Model_Message::getStatusOptions(); __('Processed (UTC)') ?> escapeHtml($message->getProcessedAt() ?: '-') ?> - - __('Claimed By') ?> - escapeHtml($message->getClaimedBy() ?: '-') ?> - getErrorMessage()): ?> diff --git a/app/locale/en_US/Maho_Queue.csv b/app/locale/en_US/Maho_Queue.csv index 06a6fa4b63..709ef8bbb7 100644 --- a/app/locale/en_US/Maho_Queue.csv +++ b/app/locale/en_US/Maho_Queue.csv @@ -3,52 +3,49 @@ "0 keeps failed messages forever.","0 keeps failed messages forever." "0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days.","0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days." "Action","Action" -"Available","Available" "Available (UTC)","Available (UTC)" -"A worker that dies on this server is detected from its lock and its messages are re-queued at once, so this timer only covers a worker on another server, whose lock is not visible here. Keep it above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume.","A worker that dies on this server is detected from its lock and its messages are re-queued at once, so this timer only covers a worker on another server, whose lock is not visible here. Keep it above the runtime of your slowest handler. Pools that declare their own redeliver_after override this for the queues they consume." +"Available","Available" "Back","Back" "Background message processing. Maho cron keeps one detached ""queue:work"" worker running per pool: a resident ""fast"" worker so short jobs never queue behind long ones, plus a ""slow"" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools.","Background message processing. Maho cron keeps one detached ""queue:work"" worker running per pool: a resident ""fast"" worker so short jobs never queue behind long ones, plus a ""slow"" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools." "Body (serialized)","Body (serialized)" "Claimed (UTC)","Claimed (UTC)" -"Claimed By","Claimed By" "Completed","Completed" -"Discard","Discard" "Discard Messages","Discard Messages" +"Discard","Discard" "Each retry waits this many times longer than the previous one.","Each retry waits this many times longer than the previous one." "Error","Error" -"Failed","Failed" "Failed messages are retried this many times with exponential backoff, then marked failed.","Failed messages are retried this many times with exponential backoff, then marked failed." +"Failed","Failed" "ID","ID" "Initial Retry Delay (seconds)","Initial Retry Delay (seconds)" "Keep Completed Messages (days)","Keep Completed Messages (days)" "Keep Failed Messages (days)","Keep Failed Messages (days)" "Max Retries","Max Retries" "Max Retry Delay (seconds)","Max Retry Delay (seconds)" -"Message","Message" "Message #%s","Message #%s" "Message Class","Message Class" "Message Details","Message Details" +"Message Queue","Message Queue" "Message discarded.","Message discarded." "Message not found.","Message not found." -"Message Queue","Message Queue" "Message re-queued.","Message re-queued." -"Only failed messages can be retried.","Only failed messages can be retried." +"Message","Message" +"Only failed or stuck messages can be retried.","Only failed or stuck messages can be retried." "Pending","Pending" "Permanently delete the selected messages?","Permanently delete the selected messages?" "Permanently delete this message?","Permanently delete this message?" "Processed (UTC)","Processed (UTC)" "Processing","Processing" "Queue","Queue" -"Queued","Queued" "Queued (UTC)","Queued (UTC)" -"Re-queue the selected failed messages?","Re-queue the selected failed messages?" +"Queued","Queued" +"Re-queue the selected messages?","Re-queue the selected messages?" "Re-queue this message?","Re-queue this message?" -"Redeliver Stuck Messages After (seconds)","Redeliver Stuck Messages After (seconds)" "Retries","Retries" -"Retry","Retry" "Retry Delay Multiplier","Retry Delay Multiplier" "Retry Messages","Retry Messages" +"Retry","Retry" "Status","Status" "Upper bound for the backoff. 0 means no bound.","Upper bound for the backoff. 0 means no bound." -"View","View" "View Messages","View Messages" +"View","View" diff --git a/lib/Maho/Queue/Pool.php b/lib/Maho/Queue/Pool.php index e4b6c10bc1..91a3c1adea 100644 --- a/lib/Maho/Queue/Pool.php +++ b/lib/Maho/Queue/Pool.php @@ -11,7 +11,7 @@ /** * A worker pool: one or more `queue:work` processes consuming a subset of the - * logical queues, with their own limits and redelivery window. Pools keep + * logical queues, with their own limits. Pools keep * latency classes apart, so a ten-minute feed build cannot sit in front of an * order confirmation email. */ @@ -23,7 +23,6 @@ * @param list $queues Consume only these queues; empty consumes every queue not excluded * @param list $excludedQueues Never consume these; the catch-all pool excludes every other pool's queues * @param ?int $idleTimeout Seconds of continuous idleness before exiting; null keeps the worker resident - * @param ?int $redeliverAfter Per-pool override of system/queue/redeliver_after */ public function __construct( public string $name, @@ -33,7 +32,6 @@ public function __construct( public ?int $idleTimeout = null, public string $memoryLimit = '256M', public int $timeLimit = 3600, - public ?int $redeliverAfter = null, ) {} /** diff --git a/lib/Maho/Queue/PoolRegistry.php b/lib/Maho/Queue/PoolRegistry.php index 3e610d0296..27d7178a26 100644 --- a/lib/Maho/Queue/PoolRegistry.php +++ b/lib/Maho/Queue/PoolRegistry.php @@ -51,22 +51,6 @@ public static function poolFor(string $queue): ?Pool return null; } - /** - * The longest redelivery window any pool declares, or null when none - * overrides the store default. A worker that consumes every queue must not - * requeue a claim sooner than the pool owning it would, or the handler runs - * a second time alongside the first. - */ - public static function widestRedeliveryWindow(): ?int - { - $windows = array_filter( - array_map(static fn(Pool $pool): ?int => $pool->redeliverAfter, self::all()), - static fn(?int $window): bool => $window !== null, - ); - - return $windows === [] ? null : max($windows); - } - public static function reset(): void { self::$pools = null; @@ -147,7 +131,6 @@ private static function build(): array idleTimeout: isset($child->idle_timeout) ? max(0, (int) $child->idle_timeout) : null, memoryLimit: trim((string) ($child->memory_limit ?? '')) ?: '256M', timeLimit: max(0, (int) ($child->time_limit ?? 3600)), - redeliverAfter: isset($child->redeliver_after) ? max(0, (int) $child->redeliver_after) : null, ); } diff --git a/lib/Maho/Queue/QueueManager.php b/lib/Maho/Queue/QueueManager.php index 191387a94e..9f286b4e0b 100644 --- a/lib/Maho/Queue/QueueManager.php +++ b/lib/Maho/Queue/QueueManager.php @@ -44,7 +44,6 @@ final class QueueManager public const XML_PATH_RETRY_DELAY = 'system/queue/retry_delay'; public const XML_PATH_RETRY_MULTIPLIER = 'system/queue/retry_multiplier'; public const XML_PATH_RETRY_MAX_DELAY = 'system/queue/retry_max_delay'; - public const XML_PATH_REDELIVER_AFTER = 'system/queue/redeliver_after'; public const XML_PATH_COMPLETED_RETENTION = 'system/queue/completed_retention'; public const XML_PATH_FAILED_RETENTION = 'system/queue/failed_retention'; @@ -101,25 +100,17 @@ public static function dbTransport(): DbTransport self::writeAdapter(), self::tableName(), self::serializer(), - (int) \Mage::getStoreConfig(self::XML_PATH_REDELIVER_AFTER), (int) \Mage::getStoreConfig(self::XML_PATH_COMPLETED_RETENTION), ); } /** * The transport a pool's worker consumes from: the shared one unless the - * pool narrows what it sees, overrides the redelivery window, or the caller - * claims under a worker id, in which case it gets its own instance. - * - * @param ?string $workerId Only a pool worker holding its lock passes one; see WorkerIdentity + * pool narrows what it sees, in which case it gets its own instance. */ - public static function workerTransport(?Pool $pool = null, ?string $workerId = null): DbTransport + public static function workerTransport(?Pool $pool = null): DbTransport { - if ($pool === null) { - return self::dbTransport(); - } - - if ($workerId === null && $pool->excludedQueues === [] && $pool->redeliverAfter === null) { + if ($pool === null || $pool->excludedQueues === []) { return self::dbTransport(); } @@ -127,10 +118,8 @@ public static function workerTransport(?Pool $pool = null, ?string $workerId = n self::writeAdapter(), self::tableName(), self::serializer(), - $pool->redeliverAfter ?? (int) \Mage::getStoreConfig(self::XML_PATH_REDELIVER_AFTER), (int) \Mage::getStoreConfig(self::XML_PATH_COMPLETED_RETENTION), $pool->excludedQueues, - $workerId, ); } @@ -140,18 +129,21 @@ public static function serializer(): Serializer } /** - * Re-queue a stored failed message from the admin grid or CLI, flipping the - * row back to pending with a fresh retry budget. Only failed rows are - * retryable: flipping a claimed row would race the worker's ack. + * Re-queue a stored message from the admin grid or CLI, flipping the row + * back to pending with a fresh retry budget. Failed rows and rows a dead + * worker left claimed are both retryable; nothing else, since a pending row + * needs no help and a completed one is done. Nothing re-queues an abandoned + * claim automatically, so this is the only way one comes back. */ public static function retryStoredMessage(int $messageId): bool { + $retryable = [DbTransport::STATUS_FAILED, DbTransport::STATUS_PROCESSING]; $adapter = self::writeAdapter(); $table = self::tableName(); $row = $adapter->fetchRow( $adapter->select()->from($table)->where('message_id = ?', $messageId), ); - if ($row === false || $row['status'] !== DbTransport::STATUS_FAILED) { + if ($row === false || !in_array($row['status'], $retryable, true)) { return false; } @@ -162,12 +154,11 @@ public static function retryStoredMessage(int $messageId): bool 'retries' => 0, 'available_at' => $now, 'claimed_at' => null, - 'claimed_by' => null, 'processed_at' => null, 'updated_at' => $now, ], [ 'message_id = ?' => $messageId, - 'status = ?' => DbTransport::STATUS_FAILED, + 'status IN (?)' => $retryable, ]) === 1; } diff --git a/lib/Maho/Queue/Transport/DbTransport.php b/lib/Maho/Queue/Transport/DbTransport.php index 9b854ece6d..f05b8c173b 100644 --- a/lib/Maho/Queue/Transport/DbTransport.php +++ b/lib/Maho/Queue/Transport/DbTransport.php @@ -12,7 +12,6 @@ use Maho\Db\Adapter\AdapterInterface; use Maho\Queue\Stamp\DedupeKeyStamp; use Maho\Queue\Stamp\QueueNameStamp; -use Maho\Queue\WorkerIdentity; use Symfony\Component\Messenger\Envelope; use Symfony\Component\Messenger\Exception\MessageDecodingFailedException; use Symfony\Component\Messenger\Exception\TransportException; @@ -50,16 +49,13 @@ final class DbTransport implements TransportInterface, QueueReceiverInterface, L /** * @param list $excludedQueues Queues this instance never consumes, so a pool worker can be "everything but" - * @param ?string $workerId Stamped on every claim; set only by a worker holding its lock, so a free lock proves the claim is orphaned */ public function __construct( private readonly AdapterInterface $adapter, private readonly string $table, private readonly Serializer $serializer, - private readonly int $redeliverAfterSeconds, private readonly int $completedRetentionDays, private readonly array $excludedQueues = [], - private readonly ?string $workerId = null, ) {} #[\Override] @@ -82,7 +78,6 @@ public function send(Envelope $envelope): Envelope 'available_at' => $availableAt, 'error_message' => $envelope->last(ErrorDetailsStamp::class)?->getExceptionMessage(), 'claimed_at' => null, - 'claimed_by' => null, 'updated_at' => $now, ], ['message_id = ?' => (int) $messageIdStamp->getId()]); @@ -107,7 +102,6 @@ public function send(Envelope $envelope): Envelope 'dedupe_key' => $dedupeKey, 'available_at' => $availableAt, 'claimed_at' => null, - 'claimed_by' => null, 'processed_at' => $isFailure ? $now : null, 'created_at' => $now, 'updated_at' => $now, @@ -200,27 +194,18 @@ public function getMessageCount(): int } /** - * Work this instance would pick up right now: rows past their availability - * plus rows a dead worker abandoned. Messages scheduled for the future are - * deliberately excluded, or the watchdog would respawn an on-demand worker - * every cron tick until a delayed campaign's send date. + * Work this instance would pick up right now. Messages scheduled for the + * future are deliberately excluded, or the watchdog would respawn an + * on-demand worker every cron tick until a delayed campaign's send date. * * @param list|null $queues */ public function countDue(?array $queues = null): int { - $clauses = ['(' . $this->adapter->quoteInto('status = ?', self::STATUS_PENDING) - . ' AND ' . $this->adapter->quoteInto('available_at <= ?', \Mage_Core_Model_Locale::nowUtc()) . ')']; - - $abandoned = $this->abandonedClaimClause(); - if ($abandoned !== null) { - $clauses[] = '(' . $this->adapter->quoteInto('status = ?', self::STATUS_PROCESSING) - . ' AND ' . $abandoned . ')'; - } - $select = $this->adapter->select() ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) - ->where(implode(' OR ', $clauses)); + ->where('status = ?', self::STATUS_PENDING) + ->where('available_at <= ?', \Mage_Core_Model_Locale::nowUtc()); $this->applyQueueFilter($select, $queues); return (int) $this->adapter->fetchOne($select); @@ -245,7 +230,6 @@ private function applyQueueFilter(\Maho\Db\Select $select, ?array $queues): void */ private function claimNext(?array $queues): array { - $this->requeueStaleClaims($queues); $now = \Mage_Core_Model_Locale::nowUtc(); for ($attempt = 0; $attempt < 5; $attempt++) { @@ -265,7 +249,6 @@ private function claimNext(?array $queues): array $claimed = $this->adapter->update($this->table, [ 'status' => self::STATUS_PROCESSING, 'claimed_at' => $now, - 'claimed_by' => $this->workerId, 'updated_at' => $now, ], [ 'message_id = ?' => (int) $row['message_id'], @@ -285,80 +268,6 @@ private function claimNext(?array $queues): array return []; } - /** - * Crash recovery: a row still processing whose worker died without ack or - * reject goes back up for grabs. - * - * Scoped to the queues this instance consumes, because pools carry their own - * window: a fast worker must not requeue a feed a slow worker is still running. - * - * @param list|null $queues - */ - private function requeueStaleClaims(?array $queues): void - { - $abandoned = $this->abandonedClaimClause(); - if ($abandoned === null) { - return; - } - - $where = [ - 'status = ?' => self::STATUS_PROCESSING, - $abandoned, - ]; - if ($queues !== null && $queues !== []) { - $where['queue IN (?)'] = $queues; - } - if ($this->excludedQueues !== []) { - $where['queue NOT IN (?)'] = $this->excludedQueues; - } - - $this->adapter->update($this->table, [ - 'status' => self::STATUS_PENDING, - 'claimed_at' => null, - 'claimed_by' => null, - 'updated_at' => \Mage_Core_Model_Locale::nowUtc(), - ], $where); - } - - /** - * Rows whose claiming worker is gone, to AND with `status = processing`. - * - * A claim from this machine is settled by its worker lock, not by the clock: - * a free lock proves the process died, a held one proves the handler is - * still running however long it takes. Claims from another machine have no - * lock to read here, so they keep the redeliver_after timer, as do rows with - * no id (mid-upgrade, or a worker running without --exclusive). - */ - private function abandonedClaimClause(): ?string - { - if ($this->redeliverAfterSeconds <= 0) { - return null; - } - - $clauses = []; - - $dead = WorkerIdentity::deadLocalIds(); - if ($dead !== []) { - $clauses[] = $this->adapter->quoteInto('claimed_by IN (?)', $dead); - } - - $timer = $this->adapter->quoteInto('claimed_at < ?', $this->staleClaimCutoff()); - $local = WorkerIdentity::localIds(); - if ($local !== []) { - $timer .= ' AND (claimed_by IS NULL OR ' - . $this->adapter->quoteInto('claimed_by NOT IN (?)', $local) . ')'; - } - $clauses[] = $timer; - - // Wrapped whole: the caller ANDs this with a status and a queue filter. - return '((' . implode(') OR (', $clauses) . '))'; - } - - private function staleClaimCutoff(): string - { - return gmdate(\Mage_Core_Model_Locale::DATETIME_FORMAT, time() - $this->redeliverAfterSeconds); - } - private function inFlightRowExists(string $dedupeKey): bool { $existing = $this->adapter->fetchOne( diff --git a/lib/Maho/Queue/WorkerFactory.php b/lib/Maho/Queue/WorkerFactory.php index c3d73b31fe..983979f174 100644 --- a/lib/Maho/Queue/WorkerFactory.php +++ b/lib/Maho/Queue/WorkerFactory.php @@ -27,12 +27,12 @@ final class WorkerFactory { /** - * @param array{limit?: ?int, memoryLimit?: ?int, idleTimeout?: ?int, pool?: ?Pool, workerId?: ?string} $options + * @param array{limit?: ?int, memoryLimit?: ?int, idleTimeout?: ?int, pool?: ?Pool} $options */ public static function create(array $options = []): Worker { $transportName = QueueManager::TRANSPORT_DB; - $transport = QueueManager::workerTransport($options['pool'] ?? null, $options['workerId'] ?? null); + $transport = QueueManager::workerTransport($options['pool'] ?? null); $dispatcher = new EventDispatcher(); $dispatcher->addSubscriber(new AddErrorDetailsStampListener()); diff --git a/lib/Maho/Queue/WorkerIdentity.php b/lib/Maho/Queue/WorkerIdentity.php deleted file mode 100644 index de2dabb814..0000000000 --- a/lib/Maho/Queue/WorkerIdentity.php +++ /dev/null @@ -1,86 +0,0 @@ - - * SPDX-License-Identifier: OSL-3.0 - */ - -declare(strict_types=1); - -namespace Maho\Queue; - -/** - * Identifies the worker holding a claim, as machine:lock-name, so crash - * recovery can read the worker's lock instead of guessing from elapsed time. - * A worker stamps this on `claimed_by` only while it holds that lock, and the - * kernel frees the flock however the process ends. - * - * The machine part is the hashed hostname: two servers on one database must - * never share an id, or each would probe its own locks for the other's claims. - */ -final class WorkerIdentity -{ - private static ?string $machine = null; - - /** Null when the hostname is unknown, which falls back to the timer. */ - public static function forLock(string $lockName): ?string - { - $machine = self::machine(); - - return $machine === null ? null : $machine . ':' . $lockName; - } - - /** - * Every id a worker on this machine can hold, alive or not. - * - * @return list - */ - public static function localIds(): array - { - return array_values(array_filter(array_map(self::forLock(...), self::lockNames()))); - } - - /** - * The ids in localIds() whose lock nobody holds, so whose process is gone. - * - * @return list - */ - public static function deadLocalIds(): array - { - $lock = \Mage::getSingleton('core/lock'); - $free = array_filter( - self::lockNames(), - static fn(string $name): bool => !$lock->isHeld($name, machineLocal: true), - ); - - return array_values(array_filter(array_map(self::forLock(...), $free))); - } - - /** - * One per pool worker, plus the poolless lock a hand-run - * `queue:work --exclusive` takes. - * - * @return list - */ - private static function lockNames(): array - { - $names = [Pool::LOCK_PREFIX]; - foreach (PoolRegistry::all() as $pool) { - for ($index = 0; $index < $pool->count; $index++) { - $names[] = $pool->lockName($index); - } - } - - return $names; - } - - private static function machine(): ?string - { - if (self::$machine === null) { - $hostname = gethostname(); - self::$machine = $hostname === false ? '' : substr(md5($hostname), 0, 12); - } - - return self::$machine === '' ? null : self::$machine; - } -} diff --git a/lib/MahoCLI/Commands/QueueWork.php b/lib/MahoCLI/Commands/QueueWork.php index d69a69e9f8..d45b2d8f2c 100644 --- a/lib/MahoCLI/Commands/QueueWork.php +++ b/lib/MahoCLI/Commands/QueueWork.php @@ -11,9 +11,7 @@ use Maho\Queue\Pool; use Maho\Queue\PoolRegistry; -use Maho\Queue\QueueManager; use Maho\Queue\WorkerFactory; -use Maho\Queue\WorkerIdentity; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Command\SignalableCommandInterface; @@ -71,25 +69,16 @@ protected function execute(InputInterface $input, OutputInterface $output): int return Command::INVALID; } - // Only a worker holding its lock may stamp claims: without one, a free - // lock would make every claim look orphaned the moment it is taken. - $workerId = null; if ($input->getOption('exclusive')) { $lockName = $pool?->lockName($index) ?? Pool::LOCK_PREFIX; if (!\Mage::getSingleton('core/lock')->acquire($lockName, machineLocal: true)) { $output->writeln("Another exclusive queue worker already holds {$lockName}"); return Command::INVALID; } - $workerId = WorkerIdentity::forLock($lockName); } // Unbounded unless asked: a hand-run worker keeps the limits it had before pools existed. - $base = $pool ?? new Pool( - name: 'ad-hoc', - memoryLimit: '', - timeLimit: 0, - redeliverAfter: $this->adHocRedeliveryWindow(), - ); + $base = $pool ?? new Pool(name: 'ad-hoc', memoryLimit: '', timeLimit: 0); $effective = new Pool( name: $base->name, queues: $input->getOption('queue') ?: $base->queues, @@ -101,7 +90,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int }, memoryLimit: (string) ($input->getOption('memory-limit') ?? $base->memoryLimit), timeLimit: (int) ($input->getOption('time-limit') ?? $base->timeLimit), - redeliverAfter: $base->redeliverAfter, ); $memoryLimit = null; @@ -118,7 +106,6 @@ protected function execute(InputInterface $input, OutputInterface $output): int 'memoryLimit' => $memoryLimit, 'idleTimeout' => $effective->idleTimeout, 'pool' => $effective, - 'workerId' => $workerId, ]); $output->writeln(sprintf( @@ -159,19 +146,6 @@ public function handleSignal(int $signal, int|false $previousExitCode = 0): int| return false; } - /** - * A worker with no pool consumes every queue, so it must not requeue a claim - * before the pool owning that queue would: take the widest window in play. - */ - private function adHocRedeliveryWindow(): ?int - { - $widest = PoolRegistry::widestRedeliveryWindow(); - - return $widest === null - ? null - : max($widest, (int) \Mage::getStoreConfig(QueueManager::XML_PATH_REDELIVER_AFTER)); - } - private function parseMemoryLimit(string $limit): ?int { if (!preg_match('/^(\d+)([KMG]?)$/i', trim($limit), $matches)) { diff --git a/tests/Backend/Integration/Queue/DbTransportTest.php b/tests/Backend/Integration/Queue/DbTransportTest.php index 1755586d60..b1808fd7d4 100644 --- a/tests/Backend/Integration/Queue/DbTransportTest.php +++ b/tests/Backend/Integration/Queue/DbTransportTest.php @@ -113,23 +113,28 @@ expect($rows[0]['error_message'])->toContain('handler blew up'); }); -it('retries only failed stored messages, refusing rows a worker holds', function () { +it('retries a failed row and a claim a dead worker left behind, but not a pending one', function () { QueueManager::dispatch(makeEmailMessage()); $transport = QueueManager::dbTransport(); - $envelopes = [...$transport->get()]; $id = (int) fetchQueueRows()[0]['message_id']; + // Pending needs no help. expect(QueueManager::retryStoredMessage($id))->toBeFalse(); + + // Claimed: nothing requeues this automatically, so the grid must be able to. + $envelopes = [...$transport->get()]; expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); + expect(QueueManager::retryStoredMessage($id))->toBeTrue(); + expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PENDING); + [...$transport->get()]; $transport->reject($envelopes[0]); expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_FAILED); - expect(QueueManager::retryStoredMessage($id))->toBeTrue(); expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PENDING); }); -it('re-queues stale processing claims after redeliver_after', function () { +it('never hands out a claim again on its own, however old it is', function () { QueueManager::dispatch(makeEmailMessage()); $transport = QueueManager::dbTransport(); expect([...$transport->get()])->toHaveCount(1); @@ -138,8 +143,10 @@ 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200), ]); - $envelopes = [...$transport->get()]; - expect($envelopes)->toHaveCount(1); + // A claim is parked for an operator, never redelivered on a timer: running + // a handler a second time is not something a clock gets to decide. + expect([...$transport->get()])->toHaveCount(0); + expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); }); it('fails a claimed row whose message class has no registered handler', function () { diff --git a/tests/Backend/Integration/Queue/PoolTest.php b/tests/Backend/Integration/Queue/PoolTest.php index ee010af2f7..9e1b767efb 100644 --- a/tests/Backend/Integration/Queue/PoolTest.php +++ b/tests/Backend/Integration/Queue/PoolTest.php @@ -12,7 +12,6 @@ use Maho\Queue\QueueManager; use Maho\Queue\StopWorkerWhenIdleListener; use Maho\Queue\Transport\DbTransport; -use Maho\Queue\WorkerIdentity; use Symfony\Component\Messenger\Event\WorkerRunningEvent; use Symfony\Component\Messenger\Worker; @@ -23,41 +22,14 @@ function queuePool(string $name): Pool return PoolRegistry::get($name) ?? throw new RuntimeException("pool {$name} is missing"); } -function poolTransport(string $name, ?string $workerId = null): DbTransport +function poolTransport(string $name): DbTransport { - $transport = QueueManager::workerTransport(queuePool($name), $workerId); + $transport = QueueManager::workerTransport(queuePool($name)); assert($transport instanceof DbTransport); return $transport; } -function poolWorkerId(string $name, int $index = 0): string -{ - return WorkerIdentity::forLock(queuePool($name)->lockName($index)) - ?? throw new RuntimeException('this host has no worker identity'); -} - -/** - * Hold a pool's worker lock for the duration of the assertions, the way a live - * `queue:work --exclusive` holds it. - * - * @param callable():void $body - */ -function withWorkerLock(string $name, callable $body): void -{ - $lock = Mage::getSingleton('core/lock'); - $lockName = queuePool($name)->lockName(); - if (!$lock->acquire($lockName, machineLocal: true)) { - throw new RuntimeException("could not take {$lockName}"); - } - - try { - $body(); - } finally { - $lock->release($lockName); - } -} - /** * Merge extra queue config the way another module's config.xml would, run the * assertions, then take it back out again. @@ -82,7 +54,7 @@ function withQueueConfig(string $xml, callable $body): void } } -function insertQueueRow(string $queue, string $status, ?string $claimedAt = null, ?string $claimedBy = null): void +function insertQueueRow(string $queue, string $status, ?string $claimedAt = null): void { $now = Mage_Core_Model_Locale::nowUtc(); queueAdapter()->insert(QueueManager::tableName(), [ @@ -92,7 +64,6 @@ function insertQueueRow(string $queue, string $status, ?string $claimedAt = null 'body' => serialize(makeEmailMessage()), 'available_at' => $now, 'claimed_at' => $claimedAt, - 'claimed_by' => $claimedBy, 'created_at' => $now, 'updated_at' => $now, ]); @@ -196,72 +167,14 @@ public function stop(): void expect(poolTransport('slow')->countDue())->toBe(0); }); -it('counts a message abandoned by a dead worker as due', function () { +it('never counts a claim as due, however old, so no worker is respawned for it', function () { insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(4 * 3600)); - // Without this the row is invisible to the probe, nothing respawns, and the - // message is stranded for good instead of for one redelivery window. - expect(poolTransport('slow')->countDue())->toBe(1); -}); - -it('leaves an unattributed claim still inside the pool redelivery window alone', function () { - insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(1800)); - - // Slow allows 3h, so a 30-minute-old claim is a running feed, not a corpse. - expect(poolTransport('slow')->countDue())->toBe(0); -}); - -it('does not let a fast worker requeue a slow job running under a longer window', function () { - insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(7200)); - - // Fast redelivers after an hour, but the claim is on a queue it does not - // own; requeueing it would run the handler a second time alongside the first. - iterator_to_array(poolTransport('fast')->getFromQueues(queuePool('fast')->queues)); - - $rows = fetchQueueRows(); - expect($rows)->toHaveCount(1); - expect($rows[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); -}); - -it('stamps the claiming worker on the row', function () { - QueueManager::dispatch(makeEmailMessage('feed'), queue: 'newsletter'); - $workerId = poolWorkerId('slow'); - - expect(iterator_to_array(poolTransport('slow', $workerId)->get()))->toHaveCount(1); - expect(fetchQueueRows()[0]['claimed_by'])->toBe($workerId); -}); - -it('reclaims a local worker crash at once, without waiting out the window', function () { - // Fresh claim, nowhere near the 3h window, but nobody holds slow's lock, so - // the process that took it is provably gone. - insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(5), poolWorkerId('slow')); - - expect(poolTransport('slow')->countDue())->toBe(1); - expect(iterator_to_array(poolTransport('slow')->get()))->toHaveCount(1); -}); - -it('never requeues a claim whose local worker still holds its lock', function () { - // The duplicate this guards: a handler slower than the window used to be - // requeued underneath itself and run a second time alongside the first. - withWorkerLock('slow', function () { - insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(4 * 3600), poolWorkerId('slow')); - - expect(poolTransport('slow')->countDue())->toBe(0); - expect(iterator_to_array(poolTransport('slow')->get()))->toHaveCount(0); - expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); - }); -}); - -it('falls back to the window for a claim held by another server', function () { - // No lock of ours to read, so the timer is all this machine has to go on. - $elsewhere = 'ffffffffffff:' . queuePool('slow')->lockName(); - - insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(1800), $elsewhere); + // An abandoned claim waits for an operator, so the watchdog must not treat + // it as work: respawning for it would start a worker with nothing to do. expect(poolTransport('slow')->countDue())->toBe(0); - - clearQueueTable(); - insertQueueRow('newsletter', DbTransport::STATUS_PROCESSING, agoUtc(4 * 3600), $elsewhere); - expect(poolTransport('slow')->countDue())->toBe(1); + expect(iterator_to_array(poolTransport('slow')->get()))->toHaveCount(0); + expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); }); it('stops an idle worker immediately when no grace period is set', function () { From 0640349723434bd44713b99541555c59ab51df99 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Sun, 9 Aug 2026 10:34:54 +0100 Subject: [PATCH 06/13] Warned in the admin about a queue message no worker finished Nothing re-queues an abandoned claim now, so the grid is the only way one comes back, and a message parked there is invisible until somebody opens the page. Every admin page carries a notice while such a row exists, linking to the grid to retry or discard it. The one-hour threshold drives display only, so a handler that honestly overruns costs a notice rather than a second run. The notice is skipped for users without the queue ACL, and added through addUniqueMessages() so browsing does not stack it up. --- app/code/core/Maho/Queue/Model/Observer.php | 57 +++++++++++++++++++ app/locale/en_US/Maho_Queue.csv | 1 + lib/Maho/Queue/Transport/DbTransport.php | 19 +++++++ .../Integration/Queue/DbTransportTest.php | 4 ++ 4 files changed, 81 insertions(+) create mode 100644 app/code/core/Maho/Queue/Model/Observer.php diff --git a/app/code/core/Maho/Queue/Model/Observer.php b/app/code/core/Maho/Queue/Model/Observer.php new file mode 100644 index 0000000000..665d437ea6 --- /dev/null +++ b/app/code/core/Maho/Queue/Model/Observer.php @@ -0,0 +1,57 @@ + + * SPDX-License-Identifier: OSL-3.0 + * @package Maho_Queue + */ + +declare(strict_types=1); + +use Maho\Queue\QueueManager; + +class Maho_Queue_Model_Observer +{ + /** + * A claim older than this is reported as abandoned. Display only: nothing + * acts on it, so an honest handler that overruns costs a notice, not a + * second run. + */ + public const STUCK_AFTER_SECONDS = 3600; + + /** + * Nothing re-queues a claim a dead worker left behind, so the grid is the + * only way one comes back and somebody has to be told to look at it. + */ + #[Maho\Config\Observer('controller_action_layout_generate_blocks_before', area: 'adminhtml')] + public function warnAboutAbandonedMessages(): void + { + if (!Mage::getSingleton('admin/session')->isAllowed('system/tools/maho_queue/view')) { + return; + } + + $stuck = $this->abandonedMessageCount(); + if ($stuck === 0) { + return; + } + + $helper = Mage::helper('queue'); + Mage::getSingleton('adminhtml/session')->addUniqueMessages([ + Mage::getSingleton('core/message')->notice($helper->__( + '%s queue message(s) were claimed by a worker that never finished. They are not re-queued automatically: retry or discard them.', + $stuck, + Mage::helper('adminhtml')->getUrl('adminhtml/queue'), + )), + ]); + } + + private function abandonedMessageCount(): int + { + $adapter = Mage::getSingleton('core/resource')->getConnection('core_read'); + if (!$adapter->isTableExists(QueueManager::tableName())) { + return 0; + } + + return QueueManager::dbTransport()->countAbandoned(self::STUCK_AFTER_SECONDS); + } +} diff --git a/app/locale/en_US/Maho_Queue.csv b/app/locale/en_US/Maho_Queue.csv index 709ef8bbb7..a35fd6c40d 100644 --- a/app/locale/en_US/Maho_Queue.csv +++ b/app/locale/en_US/Maho_Queue.csv @@ -1,5 +1,6 @@ "%s message(s) discarded.","%s message(s) discarded." "%s message(s) re-queued.","%s message(s) re-queued." +"%s queue message(s) were claimed by a worker that never finished. They are not re-queued automatically: retry or discard them.","%s queue message(s) were claimed by a worker that never finished. They are not re-queued automatically: retry or discard them." "0 keeps failed messages forever.","0 keeps failed messages forever." "0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days.","0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days." "Action","Action" diff --git a/lib/Maho/Queue/Transport/DbTransport.php b/lib/Maho/Queue/Transport/DbTransport.php index f05b8c173b..25a93047d8 100644 --- a/lib/Maho/Queue/Transport/DbTransport.php +++ b/lib/Maho/Queue/Transport/DbTransport.php @@ -211,6 +211,25 @@ public function countDue(?array $queues = null): int return (int) $this->adapter->fetchOne($select); } + /** + * Claims old enough to be read as abandoned by a worker that died. Nothing + * acts on this: it drives the admin notice, so an honest handler that + * overruns costs a notice rather than a second run. + */ + public function countAbandoned(int $olderThanSeconds): int + { + $select = $this->adapter->select() + ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) + ->where('status = ?', self::STATUS_PROCESSING) + ->where('claimed_at < ?', gmdate( + \Mage_Core_Model_Locale::DATETIME_FORMAT, + time() - $olderThanSeconds, + )); + $this->applyQueueFilter($select, null); + + return (int) $this->adapter->fetchOne($select); + } + /** * @param list|null $queues */ diff --git a/tests/Backend/Integration/Queue/DbTransportTest.php b/tests/Backend/Integration/Queue/DbTransportTest.php index b1808fd7d4..e2bef8fc6c 100644 --- a/tests/Backend/Integration/Queue/DbTransportTest.php +++ b/tests/Backend/Integration/Queue/DbTransportTest.php @@ -147,6 +147,10 @@ // a handler a second time is not something a clock gets to decide. expect([...$transport->get()])->toHaveCount(0); expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); + + // It is still reported, so the admin notice can point an operator at it. + expect($transport->countAbandoned(3600))->toBe(1); + expect($transport->countAbandoned(4 * 3600))->toBe(0); }); it('fails a claimed row whose message class has no registered handler', function () { From 3fe68742c8456c2103d7372b0603c9abe19529f8 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Sun, 9 Aug 2026 10:37:24 +0100 Subject: [PATCH 07/13] lint --- app/locale/en_US/Maho_Queue.csv | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/locale/en_US/Maho_Queue.csv b/app/locale/en_US/Maho_Queue.csv index a35fd6c40d..06f3e069f3 100644 --- a/app/locale/en_US/Maho_Queue.csv +++ b/app/locale/en_US/Maho_Queue.csv @@ -4,33 +4,33 @@ "0 keeps failed messages forever.","0 keeps failed messages forever." "0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days.","0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days." "Action","Action" -"Available (UTC)","Available (UTC)" "Available","Available" +"Available (UTC)","Available (UTC)" "Back","Back" "Background message processing. Maho cron keeps one detached ""queue:work"" worker running per pool: a resident ""fast"" worker so short jobs never queue behind long ones, plus a ""slow"" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools.","Background message processing. Maho cron keeps one detached ""queue:work"" worker running per pool: a resident ""fast"" worker so short jobs never queue behind long ones, plus a ""slow"" catch-all started only while it has work. Both are respawned within a minute if they die. Pools are declared in config.xml under global/queue/pools." "Body (serialized)","Body (serialized)" "Claimed (UTC)","Claimed (UTC)" "Completed","Completed" -"Discard Messages","Discard Messages" "Discard","Discard" +"Discard Messages","Discard Messages" "Each retry waits this many times longer than the previous one.","Each retry waits this many times longer than the previous one." "Error","Error" -"Failed messages are retried this many times with exponential backoff, then marked failed.","Failed messages are retried this many times with exponential backoff, then marked failed." "Failed","Failed" +"Failed messages are retried this many times with exponential backoff, then marked failed.","Failed messages are retried this many times with exponential backoff, then marked failed." "ID","ID" "Initial Retry Delay (seconds)","Initial Retry Delay (seconds)" "Keep Completed Messages (days)","Keep Completed Messages (days)" "Keep Failed Messages (days)","Keep Failed Messages (days)" "Max Retries","Max Retries" "Max Retry Delay (seconds)","Max Retry Delay (seconds)" +"Message","Message" "Message #%s","Message #%s" "Message Class","Message Class" "Message Details","Message Details" -"Message Queue","Message Queue" "Message discarded.","Message discarded." "Message not found.","Message not found." +"Message Queue","Message Queue" "Message re-queued.","Message re-queued." -"Message","Message" "Only failed or stuck messages can be retried.","Only failed or stuck messages can be retried." "Pending","Pending" "Permanently delete the selected messages?","Permanently delete the selected messages?" @@ -38,15 +38,15 @@ "Processed (UTC)","Processed (UTC)" "Processing","Processing" "Queue","Queue" -"Queued (UTC)","Queued (UTC)" "Queued","Queued" +"Queued (UTC)","Queued (UTC)" "Re-queue the selected messages?","Re-queue the selected messages?" "Re-queue this message?","Re-queue this message?" "Retries","Retries" +"Retry","Retry" "Retry Delay Multiplier","Retry Delay Multiplier" "Retry Messages","Retry Messages" -"Retry","Retry" "Status","Status" "Upper bound for the backoff. 0 means no bound.","Upper bound for the backoff. 0 means no bound." -"View Messages","View Messages" "View","View" +"View Messages","View Messages" From 488969597204a410e0cd503b1f6fd07c2dc40ea8 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Sun, 9 Aug 2026 16:46:54 +0100 Subject: [PATCH 08/13] Gated queue retry and dedupe on an abandoned claim Retrying a message that a worker was still handling ran the handler a second time. Retry now accepts a claimed row only when the claim is older than the abandoned cut-off, applied in the update where-clause so a fresh claim wins the race, and the grid hides the button in the same case. A parked claim also kept its dedupe key in flight forever, turning every later dispatch of that key into a silent no-op. The in-flight check now ignores claims past the same cut-off. --- AGENTS.md | 4 ++- .../Queue/Block/Adminhtml/Message/View.php | 18 +++++++---- app/code/core/Maho/Queue/Model/Observer.php | 8 ++--- lib/Maho/Queue/QueueManager.php | 32 +++++++++++++------ lib/Maho/Queue/Transport/DbTransport.php | 24 +++++++++++--- lib/MahoCLI/Commands/QueueList.php | 2 +- .../Integration/Queue/DbTransportTest.php | 27 ++++++++++++++-- 7 files changed, 87 insertions(+), 28 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0f1a0acea4..42324570df 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -176,7 +176,9 @@ class My_Module_Checkout_CartController extends Mage_Checkout_CartController { / `slow` is the on-demand catch-all. Pass `queue:` to `dispatch()`, then route that queue with `fast`; anything unrouted lands in the catch-all, so a long-running handler never blocks short ones. Pool - resourcing (count, limits, idle timeout, redelivery) lives under `global/queue/pools` + resourcing (count, memory/time limits, idle timeout) lives under `global/queue/pools`. A crash + parks a claimed message for an operator instead of redelivering it: retry or discard it in the + grid - **Layout**: XML-based block hierarchy and template assignment - **Sessions**: `Mage::getSingleton('customer/session')`, `'admin/session'`, `'checkout/session'` - **Translations**: `$this->__('Text')`, CSVs in `app/locale/[locale]/` diff --git a/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php b/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php index 468be4cc15..73ea51abf3 100644 --- a/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php +++ b/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php @@ -31,14 +31,20 @@ public function getDiscardUrl(): string } /** - * Failed, plus a claim a dead worker left behind: nothing re-queues those - * automatically, so the grid is the only way back. + * Failed, plus a claim old enough to belong to a dead worker: nothing + * re-queues those automatically, so the grid is the only way back. A fresh + * claim is not offered, since re-queueing it would run the handler a second + * time alongside the worker still holding it. */ public function isRetryable(): bool { - return in_array($this->getMessage()?->getStatus(), [ - Maho_Queue_Model_Message::STATUS_FAILED, - Maho_Queue_Model_Message::STATUS_PROCESSING, - ], true); + $message = $this->getMessage(); + + return match ($message?->getStatus()) { + Maho_Queue_Model_Message::STATUS_FAILED => true, + Maho_Queue_Model_Message::STATUS_PROCESSING => $message->getClaimedAt() !== null + && $message->getClaimedAt() < \Maho\Queue\Transport\DbTransport::abandonedBefore(), + default => false, + }; } } diff --git a/app/code/core/Maho/Queue/Model/Observer.php b/app/code/core/Maho/Queue/Model/Observer.php index 665d437ea6..95131e09b7 100644 --- a/app/code/core/Maho/Queue/Model/Observer.php +++ b/app/code/core/Maho/Queue/Model/Observer.php @@ -9,15 +9,15 @@ declare(strict_types=1); use Maho\Queue\QueueManager; +use Maho\Queue\Transport\DbTransport; class Maho_Queue_Model_Observer { /** - * A claim older than this is reported as abandoned. Display only: nothing - * acts on it, so an honest handler that overruns costs a notice, not a - * second run. + * A claim older than this is reported as abandoned. Nothing redelivers on + * it, so an honest handler that overruns costs a notice, not a second run. */ - public const STUCK_AFTER_SECONDS = 3600; + public const STUCK_AFTER_SECONDS = DbTransport::ABANDONED_AFTER_SECONDS; /** * Nothing re-queues a claim a dead worker left behind, so the grid is the diff --git a/lib/Maho/Queue/QueueManager.php b/lib/Maho/Queue/QueueManager.php index 9f286b4e0b..4fe5fa3e65 100644 --- a/lib/Maho/Queue/QueueManager.php +++ b/lib/Maho/Queue/QueueManager.php @@ -130,20 +130,35 @@ public static function serializer(): Serializer /** * Re-queue a stored message from the admin grid or CLI, flipping the row - * back to pending with a fresh retry budget. Failed rows and rows a dead - * worker left claimed are both retryable; nothing else, since a pending row - * needs no help and a completed one is done. Nothing re-queues an abandoned - * claim automatically, so this is the only way one comes back. + * back to pending with a fresh retry budget. Failed rows and claims old + * enough to belong to a dead worker are both retryable; nothing else, since + * a pending row needs no help and a completed one is done. Nothing re-queues + * an abandoned claim automatically, so this is the only way one comes back. + * + * A fresh claim is refused on purpose: re-queueing a row a live worker is + * still inside runs the handler a second time, the very thing dropping + * timer-based redelivery was meant to make impossible. The age cut-off is + * applied in the UPDATE, so a worker that claims the row between the read + * and the write keeps it. */ public static function retryStoredMessage(int $messageId): bool { - $retryable = [DbTransport::STATUS_FAILED, DbTransport::STATUS_PROCESSING]; $adapter = self::writeAdapter(); $table = self::tableName(); $row = $adapter->fetchRow( $adapter->select()->from($table)->where('message_id = ?', $messageId), ); - if ($row === false || !in_array($row['status'], $retryable, true)) { + if ($row === false) { + return false; + } + + $where = ['message_id = ?' => $messageId]; + if ($row['status'] === DbTransport::STATUS_PROCESSING) { + $where['status = ?'] = DbTransport::STATUS_PROCESSING; + $where['claimed_at < ?'] = DbTransport::abandonedBefore(); + } elseif ($row['status'] === DbTransport::STATUS_FAILED) { + $where['status = ?'] = DbTransport::STATUS_FAILED; + } else { return false; } @@ -156,10 +171,7 @@ public static function retryStoredMessage(int $messageId): bool 'claimed_at' => null, 'processed_at' => null, 'updated_at' => $now, - ], [ - 'message_id = ?' => $messageId, - 'status IN (?)' => $retryable, - ]) === 1; + ], $where) === 1; } public static function discardStoredMessage(int $messageId): bool diff --git a/lib/Maho/Queue/Transport/DbTransport.php b/lib/Maho/Queue/Transport/DbTransport.php index 25a93047d8..1ca514bf12 100644 --- a/lib/Maho/Queue/Transport/DbTransport.php +++ b/lib/Maho/Queue/Transport/DbTransport.php @@ -47,6 +47,14 @@ final class DbTransport implements TransportInterface, QueueReceiverInterface, L public const DEFAULT_QUEUE = 'default'; + /** + * A claim held longer than this is read as abandoned by a worker that died. + * Nothing redelivers on it: it gates the admin notice, the admin retry, and + * the dedupe check, so an honest handler that overruns costs a notice rather + * than a second run. + */ + public const ABANDONED_AFTER_SECONDS = 3600; + /** * @param list $excludedQueues Queues this instance never consumes, so a pool worker can be "everything but" */ @@ -221,10 +229,7 @@ public function countAbandoned(int $olderThanSeconds): int $select = $this->adapter->select() ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) ->where('status = ?', self::STATUS_PROCESSING) - ->where('claimed_at < ?', gmdate( - \Mage_Core_Model_Locale::DATETIME_FORMAT, - time() - $olderThanSeconds, - )); + ->where('claimed_at < ?', self::abandonedBefore($olderThanSeconds)); $this->applyQueueFilter($select, null); return (int) $this->adapter->fetchOne($select); @@ -289,17 +294,28 @@ private function claimNext(?array $queues): array private function inFlightRowExists(string $dedupeKey): bool { + // A claim a dead worker left behind waits for an operator forever, so it + // must not keep suppressing new dispatches of the same key: that would + // silently drop every later send instead of parking one message. + // Pending rows always have a null claimed_at, so they are never excluded. $existing = $this->adapter->fetchOne( $this->adapter->select() ->from($this->table, 'message_id') ->where('dedupe_key = ?', $dedupeKey) ->where('status IN (?)', [self::STATUS_PENDING, self::STATUS_PROCESSING]) + ->where('claimed_at IS NULL OR claimed_at >= ?', self::abandonedBefore()) ->limit(1), ); return $existing !== false && $existing !== null; } + /** UTC cut-off before which a claim counts as abandoned. */ + public static function abandonedBefore(int $olderThanSeconds = self::ABANDONED_AFTER_SECONDS): string + { + return gmdate(\Mage_Core_Model_Locale::DATETIME_FORMAT, time() - $olderThanSeconds); + } + /** * @param array $row */ diff --git a/lib/MahoCLI/Commands/QueueList.php b/lib/MahoCLI/Commands/QueueList.php index f1b50db2d4..30df84a59a 100644 --- a/lib/MahoCLI/Commands/QueueList.php +++ b/lib/MahoCLI/Commands/QueueList.php @@ -22,7 +22,7 @@ #[AsCommand( name: 'queue:list', - description: 'Show per-queue message counts and the active transport', + description: 'Show per-queue message counts and the worker pool each queue is consumed by', )] class QueueList extends BaseMahoCommand { diff --git a/tests/Backend/Integration/Queue/DbTransportTest.php b/tests/Backend/Integration/Queue/DbTransportTest.php index e2bef8fc6c..7fffa7aee6 100644 --- a/tests/Backend/Integration/Queue/DbTransportTest.php +++ b/tests/Backend/Integration/Queue/DbTransportTest.php @@ -97,6 +97,23 @@ expect(fetchQueueRows())->toHaveCount(2); }); +it('lets a dedupe key be dispatched again once its claim is abandoned', function () { + QueueManager::dispatch(makeEmailMessage(), dedupeKey: 'abc'); + $transport = QueueManager::dbTransport(); + expect([...$transport->getFromQueues(['default'])])->toHaveCount(1); + + QueueManager::dispatch(makeEmailMessage(), dedupeKey: 'abc'); + expect(fetchQueueRows())->toHaveCount(1); + + // A claim nobody will finish must stop suppressing later sends, or every + // future dispatch of this key is silently dropped instead of one being parked. + queueAdapter()->update(QueueManager::tableName(), [ + 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200), + ]); + QueueManager::dispatch(makeEmailMessage(), dedupeKey: 'abc'); + expect(fetchQueueRows())->toHaveCount(2); +}); + it('inserts a failure-transport send as a failed row instead of updating by the foreign message id', function () { $envelope = (new Envelope(makeEmailMessage()))->with( new TransportMessageIdStamp('1712345678901-0'), @@ -113,7 +130,7 @@ expect($rows[0]['error_message'])->toContain('handler blew up'); }); -it('retries a failed row and a claim a dead worker left behind, but not a pending one', function () { +it('retries a failed row and an abandoned claim, but not a pending or freshly claimed one', function () { QueueManager::dispatch(makeEmailMessage()); $transport = QueueManager::dbTransport(); $id = (int) fetchQueueRows()[0]['message_id']; @@ -121,9 +138,15 @@ // Pending needs no help. expect(QueueManager::retryStoredMessage($id))->toBeFalse(); - // Claimed: nothing requeues this automatically, so the grid must be able to. + // A live worker is still inside this one: re-queueing it runs the handler twice. $envelopes = [...$transport->get()]; expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); + expect(QueueManager::retryStoredMessage($id))->toBeFalse(); + + // Old enough to belong to a dead worker: nothing else requeues it, so the grid must. + queueAdapter()->update(QueueManager::tableName(), [ + 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200), + ]); expect(QueueManager::retryStoredMessage($id))->toBeTrue(); expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PENDING); From d6cafa22cdcc6f1d606275815f595eccece7b234 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Sun, 9 Aug 2026 17:18:32 +0100 Subject: [PATCH 09/13] Raised a queue worker that never started to an error A pool that fails to take its lock is now logged at ERROR unless it is an on-demand pool with nothing left due, so a worker that fatals on boot no longer hides behind a notice while its queues stop being consumed. The spawn confirmation also waits once for the whole batch instead of five seconds per pool in turn. `queue:work --queue` now drops the pool's inherited exclusion list, so `--pool=slow --queue=email` consumes email instead of idling on `queue IN ('email') AND queue NOT IN ('email')`. --- app/code/core/Maho/Queue/Model/Cron.php | 36 +++++++++++++++------ app/code/core/Maho/Queue/Model/Observer.php | 9 +----- lib/Maho/Queue/Transport/DbTransport.php | 2 +- lib/MahoCLI/Commands/QueueWork.php | 8 +++-- 4 files changed, 35 insertions(+), 20 deletions(-) diff --git a/app/code/core/Maho/Queue/Model/Cron.php b/app/code/core/Maho/Queue/Model/Cron.php index 296a3297cf..55b5a680f9 100644 --- a/app/code/core/Maho/Queue/Model/Cron.php +++ b/app/code/core/Maho/Queue/Model/Cron.php @@ -39,6 +39,8 @@ public function process(): void foreach ($pending as [$pool, $index]) { $this->spawnWorker($pool, $index); } + + $this->reportWorkersThatDidNotStart($pending); } /** @@ -126,19 +128,35 @@ private function spawnWorker(Pool $pool, int $index): void $index, escapeshellarg(Mage::getBaseDir('var') . '/log/queue-worker.log'), )); + } + /** + * One shared wait for the whole batch: waiting out each pool in turn would + * cost a cron tick five seconds per worker slow to take its lock. + * + * @param list $spawned + */ + private function reportWorkersThatDidNotStart(array $spawned): void + { $lock = Mage::getSingleton('core/lock'); - for ($attempt = 0; $attempt < self::SPAWN_WAIT_ATTEMPTS; $attempt++) { + for ($attempt = 0; $attempt < self::SPAWN_WAIT_ATTEMPTS && $spawned !== []; $attempt++) { usleep(self::SPAWN_WAIT_MICROSECONDS); - if ($lock->isHeld($pool->lockName($index), machineLocal: true)) { - return; - } + $spawned = array_values(array_filter( + $spawned, + fn(array $worker) => !$lock->isHeld($worker[0]->lockName($worker[1]), machineLocal: true), + )); } - // An on-demand worker may have drained its queue and exited inside the wait window. - Mage::log( - sprintf('Queue worker for pool "%s" did not start after spawning; check var/log/queue-worker.log', $pool->name), - $pool->isOnDemand() ? Mage::LOG_NOTICE : Mage::LOG_ERROR, - ); + foreach ($spawned as [$pool]) { + // An on-demand worker that drained its queue and exited inside the + // wait window did its job; only a pool still owed work failed. + if ($pool->isOnDemand() && $this->dueWorkCount($pool) === 0) { + continue; + } + Mage::log( + sprintf('Queue worker for pool "%s" did not start after spawning; check var/log/queue-worker.log', $pool->name), + Mage::LOG_ERROR, + ); + } } } diff --git a/app/code/core/Maho/Queue/Model/Observer.php b/app/code/core/Maho/Queue/Model/Observer.php index 95131e09b7..a48a8262e3 100644 --- a/app/code/core/Maho/Queue/Model/Observer.php +++ b/app/code/core/Maho/Queue/Model/Observer.php @@ -9,16 +9,9 @@ declare(strict_types=1); use Maho\Queue\QueueManager; -use Maho\Queue\Transport\DbTransport; class Maho_Queue_Model_Observer { - /** - * A claim older than this is reported as abandoned. Nothing redelivers on - * it, so an honest handler that overruns costs a notice, not a second run. - */ - public const STUCK_AFTER_SECONDS = DbTransport::ABANDONED_AFTER_SECONDS; - /** * Nothing re-queues a claim a dead worker left behind, so the grid is the * only way one comes back and somebody has to be told to look at it. @@ -52,6 +45,6 @@ private function abandonedMessageCount(): int return 0; } - return QueueManager::dbTransport()->countAbandoned(self::STUCK_AFTER_SECONDS); + return QueueManager::dbTransport()->countAbandoned(); } } diff --git a/lib/Maho/Queue/Transport/DbTransport.php b/lib/Maho/Queue/Transport/DbTransport.php index 1ca514bf12..68ea7d4e0b 100644 --- a/lib/Maho/Queue/Transport/DbTransport.php +++ b/lib/Maho/Queue/Transport/DbTransport.php @@ -224,7 +224,7 @@ public function countDue(?array $queues = null): int * acts on this: it drives the admin notice, so an honest handler that * overruns costs a notice rather than a second run. */ - public function countAbandoned(int $olderThanSeconds): int + public function countAbandoned(int $olderThanSeconds = self::ABANDONED_AFTER_SECONDS): int { $select = $this->adapter->select() ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) diff --git a/lib/MahoCLI/Commands/QueueWork.php b/lib/MahoCLI/Commands/QueueWork.php index d45b2d8f2c..e92534a6be 100644 --- a/lib/MahoCLI/Commands/QueueWork.php +++ b/lib/MahoCLI/Commands/QueueWork.php @@ -79,10 +79,14 @@ protected function execute(InputInterface $input, OutputInterface $output): int // Unbounded unless asked: a hand-run worker keeps the limits it had before pools existed. $base = $pool ?? new Pool(name: 'ad-hoc', memoryLimit: '', timeLimit: 0); + $queues = $input->getOption('queue'); $effective = new Pool( name: $base->name, - queues: $input->getOption('queue') ?: $base->queues, - excludedQueues: $input->getOption('exclude-queue') ?: $base->excludedQueues, + queues: $queues ?: $base->queues, + // An explicit allow-list already narrows the worker, and keeping the + // pool's "everything but" list on top of it would leave + // `--pool=slow --queue=email` consuming nothing at all. + excludedQueues: $input->getOption('exclude-queue') ?: ($queues ? [] : $base->excludedQueues), idleTimeout: match (true) { $input->getOption('idle-timeout') !== null => max(0, (int) $input->getOption('idle-timeout')), (bool) $input->getOption('stop-when-empty') => 0, From 5eb2422d28e16ef3fbb05a24249fab008a3389bb Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Mon, 10 Aug 2026 10:49:46 +0100 Subject: [PATCH 10/13] Kept a queue worker's claim alive while its handler runs DbTransport implements Symfony's KeepaliveReceiverInterface and QueueWork wires SIGALRM the way ConsumeMessagesCommand does, so claimed_at means "a worker is alive here" rather than "a worker started here a while ago". The admin notice, the retry gate and the dedupe check now only fire on a worker that died, instead of on any handler that outran the window. The abandoned window drops from an hour to 300 seconds, next to the 5 second refresh it depends on. email:queue:process delegates to queue:work through the Application instead of building a second Worker, so the signal wiring is not duplicated and WorkerFactory::create() has a single caller. On-demand pools subtract their live claims from the roster, so a worker inside a long handler no longer counts as available and block a spawn for a due message. The admin notice skips AJAX requests and no longer probes the schema with isTableExists() on every admin request. Dropped the post-spawn worker start check: startup failures already land in var/log/queue-worker.log, and its wait blocked every cron tick that spawned. --- AGENTS.md | 5 +- app/code/core/Maho/Queue/Model/Cron.php | 50 ++++------------- app/code/core/Maho/Queue/Model/Observer.php | 13 +++-- lib/Maho/Queue/Transport/DbTransport.php | 54 +++++++++++++++---- lib/MahoCLI/Commands/EmailQueueProcess.php | 14 +++-- lib/MahoCLI/Commands/QueueWork.php | 19 ++++++- .../Integration/Queue/CronConsumerTest.php | 26 +++++++++ .../Integration/Queue/DbTransportTest.php | 35 ++++++++++++ 8 files changed, 157 insertions(+), 59 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 42324570df..e1e553ba52 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -178,7 +178,10 @@ class My_Module_Checkout_CartController extends Mage_Checkout_CartController { / unrouted lands in the catch-all, so a long-running handler never blocks short ones. Pool resourcing (count, memory/time limits, idle timeout) lives under `global/queue/pools`. A crash parks a claimed message for an operator instead of redelivering it: retry or discard it in the - grid + grid. A handler may run as long as it needs: the worker refreshes its claim on Symfony's + keepalive alarm, so only a worker that actually died is reported as abandoned. Worker startup + failures land in `var/log/queue-worker.log`; production installs should prefer supervisord or + systemd over the cron watchdog - **Layout**: XML-based block hierarchy and template assignment - **Sessions**: `Mage::getSingleton('customer/session')`, `'admin/session'`, `'checkout/session'` - **Translations**: `$this->__('Text')`, CSVs in `app/locale/[locale]/` diff --git a/app/code/core/Maho/Queue/Model/Cron.php b/app/code/core/Maho/Queue/Model/Cron.php index 55b5a680f9..98e39eb087 100644 --- a/app/code/core/Maho/Queue/Model/Cron.php +++ b/app/code/core/Maho/Queue/Model/Cron.php @@ -15,9 +15,6 @@ class Maho_Queue_Model_Cron { - private const SPAWN_WAIT_ATTEMPTS = 10; - private const SPAWN_WAIT_MICROSECONDS = 500_000; - /** * Watchdog: start a detached `queue:work --exclusive` for every configured * pool with no live worker, so each latency tier gets a process of its own @@ -39,8 +36,6 @@ public function process(): void foreach ($pending as [$pool, $index]) { $this->spawnWorker($pool, $index); } - - $this->reportWorkersThatDidNotStart($pending); } /** @@ -62,6 +57,7 @@ public function workersToSpawn(): array foreach (PoolRegistry::all() as $pool) { $due = null; + $idle = null; $live = 0; $free = []; for ($index = 0; $index < $pool->count; $index++) { @@ -73,17 +69,17 @@ public function workersToSpawn(): array } foreach ($free as $index) { - // One process per due message, workers already alive included: an - // on-demand pool holding its whole roster open for a single - // message would idle them all out. + // One process per due message. A busy worker cannot take one, and holds + // exactly one claim, so live claims come off the roster. if ($pool->isOnDemand()) { $due ??= $this->dueWorkCount($pool); - if ($live >= $due) { + $idle ??= $live === 0 ? 0 : max(0, $live - $this->busyWorkerCount($pool)); + if ($idle >= $due) { break; } + $idle++; } $spawn[] = [$pool, $index]; - $live++; } } @@ -95,6 +91,11 @@ private function dueWorkCount(Pool $pool): int return QueueManager::workerTransport($pool)->countDue($pool->queues); } + private function busyWorkerCount(Pool $pool): int + { + return QueueManager::workerTransport($pool)->countClaimed($pool->queues); + } + #[Maho\Config\CronJob('queue_clean_up', schedule: '0 2 * * *')] public function cleanup(): void { @@ -130,33 +131,4 @@ private function spawnWorker(Pool $pool, int $index): void )); } - /** - * One shared wait for the whole batch: waiting out each pool in turn would - * cost a cron tick five seconds per worker slow to take its lock. - * - * @param list $spawned - */ - private function reportWorkersThatDidNotStart(array $spawned): void - { - $lock = Mage::getSingleton('core/lock'); - for ($attempt = 0; $attempt < self::SPAWN_WAIT_ATTEMPTS && $spawned !== []; $attempt++) { - usleep(self::SPAWN_WAIT_MICROSECONDS); - $spawned = array_values(array_filter( - $spawned, - fn(array $worker) => !$lock->isHeld($worker[0]->lockName($worker[1]), machineLocal: true), - )); - } - - foreach ($spawned as [$pool]) { - // An on-demand worker that drained its queue and exited inside the - // wait window did its job; only a pool still owed work failed. - if ($pool->isOnDemand() && $this->dueWorkCount($pool) === 0) { - continue; - } - Mage::log( - sprintf('Queue worker for pool "%s" did not start after spawning; check var/log/queue-worker.log', $pool->name), - Mage::LOG_ERROR, - ); - } - } } diff --git a/app/code/core/Maho/Queue/Model/Observer.php b/app/code/core/Maho/Queue/Model/Observer.php index a48a8262e3..28295c9a16 100644 --- a/app/code/core/Maho/Queue/Model/Observer.php +++ b/app/code/core/Maho/Queue/Model/Observer.php @@ -19,6 +19,11 @@ class Maho_Queue_Model_Observer #[Maho\Config\Observer('controller_action_layout_generate_blocks_before', area: 'adminhtml')] public function warnAboutAbandonedMessages(): void { + // An AJAX response renders no message block; the next full page load adds it back. + if (Mage::app()->getRequest()->isXmlHttpRequest()) { + return; + } + if (!Mage::getSingleton('admin/session')->isAllowed('system/tools/maho_queue/view')) { return; } @@ -38,13 +43,13 @@ public function warnAboutAbandonedMessages(): void ]); } + /** Caught rather than probed: isTableExists() would introspect the schema on every request. */ private function abandonedMessageCount(): int { - $adapter = Mage::getSingleton('core/resource')->getConnection('core_read'); - if (!$adapter->isTableExists(QueueManager::tableName())) { + try { + return QueueManager::dbTransport()->countAbandoned(); + } catch (Exception) { return 0; } - - return QueueManager::dbTransport()->countAbandoned(); } } diff --git a/lib/Maho/Queue/Transport/DbTransport.php b/lib/Maho/Queue/Transport/DbTransport.php index 68ea7d4e0b..c88f7444d4 100644 --- a/lib/Maho/Queue/Transport/DbTransport.php +++ b/lib/Maho/Queue/Transport/DbTransport.php @@ -20,6 +20,7 @@ use Symfony\Component\Messenger\Stamp\RedeliveryStamp; use Symfony\Component\Messenger\Stamp\SentToFailureTransportStamp; use Symfony\Component\Messenger\Stamp\TransportMessageIdStamp; +use Symfony\Component\Messenger\Transport\Receiver\KeepaliveReceiverInterface; use Symfony\Component\Messenger\Transport\Receiver\ListableReceiverInterface; use Symfony\Component\Messenger\Transport\Receiver\MessageCountAwareInterface; use Symfony\Component\Messenger\Transport\Receiver\QueueReceiverInterface; @@ -38,7 +39,7 @@ * The table is declared in Maho_Queue's sql/schema.php and is never * auto-created here. */ -final class DbTransport implements TransportInterface, QueueReceiverInterface, ListableReceiverInterface, MessageCountAwareInterface +final class DbTransport implements TransportInterface, QueueReceiverInterface, ListableReceiverInterface, MessageCountAwareInterface, KeepaliveReceiverInterface { public const STATUS_PENDING = 'pending'; public const STATUS_PROCESSING = 'processing'; @@ -47,13 +48,15 @@ final class DbTransport implements TransportInterface, QueueReceiverInterface, L public const DEFAULT_QUEUE = 'default'; + /** How often a worker refreshes the claim of the message it is handling. */ + public const KEEPALIVE_INTERVAL_SECONDS = 5; + /** - * A claim held longer than this is read as abandoned by a worker that died. - * Nothing redelivers on it: it gates the admin notice, the admin retry, and - * the dedupe check, so an honest handler that overruns costs a notice rather - * than a second run. + * Sixty missed refreshes: a claim this stale belongs to a worker that died. + * Gates the admin notice, the admin retry and the dedupe check; nothing + * redelivers on it. */ - public const ABANDONED_AFTER_SECONDS = 3600; + public const ABANDONED_AFTER_SECONDS = 300; /** * @param list $excludedQueues Queues this instance never consumes, so a pool worker can be "everything but" @@ -162,6 +165,23 @@ public function reject(Envelope $envelope): void ]); } + /** + * Makes claimed_at mean "a worker is alive here", not "a worker started here + * a while ago". The status guard leaves a row an operator already retried alone. + */ + #[\Override] + public function keepalive(Envelope $envelope, ?int $seconds = null): void + { + $now = \Mage_Core_Model_Locale::nowUtc(); + $this->adapter->update($this->table, [ + 'claimed_at' => $now, + 'updated_at' => $now, + ], [ + 'message_id = ?' => $this->messageId($envelope), + 'status = ?' => self::STATUS_PROCESSING, + ]); + } + #[\Override] public function all(?int $limit = null): iterable { @@ -219,11 +239,7 @@ public function countDue(?array $queues = null): int return (int) $this->adapter->fetchOne($select); } - /** - * Claims old enough to be read as abandoned by a worker that died. Nothing - * acts on this: it drives the admin notice, so an honest handler that - * overruns costs a notice rather than a second run. - */ + /** Drives the admin notice. A working worker refreshes its claim, so this counts dead ones. */ public function countAbandoned(int $olderThanSeconds = self::ABANDONED_AFTER_SECONDS): int { $select = $this->adapter->select() @@ -235,6 +251,22 @@ public function countAbandoned(int $olderThanSeconds = self::ABANDONED_AFTER_SEC return (int) $this->adapter->fetchOne($select); } + /** + * Live claims, so the watchdog can tell a pool's busy workers from its idle ones. + * + * @param list|null $queues + */ + public function countClaimed(?array $queues = null): int + { + $select = $this->adapter->select() + ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) + ->where('status = ?', self::STATUS_PROCESSING) + ->where('claimed_at >= ?', self::abandonedBefore()); + $this->applyQueueFilter($select, $queues); + + return (int) $this->adapter->fetchOne($select); + } + /** * @param list|null $queues */ diff --git a/lib/MahoCLI/Commands/EmailQueueProcess.php b/lib/MahoCLI/Commands/EmailQueueProcess.php index 2e2b77537c..65a168ed7c 100644 --- a/lib/MahoCLI/Commands/EmailQueueProcess.php +++ b/lib/MahoCLI/Commands/EmailQueueProcess.php @@ -10,9 +10,9 @@ namespace MahoCLI\Commands; use Mage; -use Maho\Queue\WorkerFactory; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Input\ArrayInput; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; @@ -41,8 +41,16 @@ protected function execute(InputInterface $input, OutputInterface $output): int $output->writeln("Processing email queue ({$pendingCount} emails pending)..."); try { - $worker = WorkerFactory::create(['idleTimeout' => 0]); - $worker->run(['queues' => [\Mage_Core_Model_Email_Queue::QUEUE_NAME]]); + // Not the command's own run(): only the full lifecycle registers its signal handlers. + $status = $this->getApplication()?->doRun(new ArrayInput([ + 'command' => 'queue:work', + '--queue' => [\Mage_Core_Model_Email_Queue::QUEUE_NAME], + '--idle-timeout' => '0', + ]), $output) ?? Command::FAILURE; + + if ($status !== Command::SUCCESS) { + return $status; + } $output->writeln('Queue processing completed.'); $failedCount = $this->getFailedCount(); diff --git a/lib/MahoCLI/Commands/QueueWork.php b/lib/MahoCLI/Commands/QueueWork.php index e92534a6be..d1cdf0129d 100644 --- a/lib/MahoCLI/Commands/QueueWork.php +++ b/lib/MahoCLI/Commands/QueueWork.php @@ -11,6 +11,7 @@ use Maho\Queue\Pool; use Maho\Queue\PoolRegistry; +use Maho\Queue\Transport\DbTransport; use Maho\Queue\WorkerFactory; use Symfony\Component\Console\Attribute\AsCommand; use Symfony\Component\Console\Command\Command; @@ -127,6 +128,11 @@ protected function execute(InputInterface $input, OutputInterface $output): int $options['queues'] = $effective->queues; } + // Drives keepalive(): without it a slow handler is indistinguishable from a dead worker. + if (SignalRegistry::isSupported()) { + $this->getApplication()?->setAlarmInterval(DbTransport::KEEPALIVE_INTERVAL_SECONDS); + } + $this->worker->run($options); return Command::SUCCESS; @@ -138,12 +144,23 @@ protected function execute(InputInterface $input, OutputInterface $output): int #[\Override] public function getSubscribedSignals(): array { - return SignalRegistry::isSupported() ? [\SIGTERM, \SIGINT] : []; + return SignalRegistry::isSupported() ? [\SIGTERM, \SIGINT, \SIGALRM] : []; } #[\Override] public function handleSignal(int $signal, int|false $previousExitCode = 0): int|false { + if ($signal === \SIGALRM) { + try { + $this->worker?->keepalive($this->getApplication()?->getAlarmInterval()); + } catch (\Exception $e) { + // The alarm lands mid-handler, so an escaping error would unwind it half-done. + \Mage::log('Queue worker could not refresh its claim: ' . $e->getMessage(), \Mage::LOG_WARNING); + } + + return false; + } + // Finish the in-flight message, then exit cleanly. $this->worker?->stop(); diff --git a/tests/Backend/Integration/Queue/CronConsumerTest.php b/tests/Backend/Integration/Queue/CronConsumerTest.php index 9e4a41b22f..05b0aebf45 100644 --- a/tests/Backend/Integration/Queue/CronConsumerTest.php +++ b/tests/Backend/Integration/Queue/CronConsumerTest.php @@ -123,6 +123,32 @@ function withAllPoolLocks(callable $body): void } }); +it('does not count a worker inside a long handler as free to take the next message', function () { + $node = Mage::getConfig()->getNode('global/queue'); + $node->extend(new Maho\Simplexml\Element('3'), true); + QueueManager::reset(); + + $lock = Mage::getSingleton('core/lock'); + $slow = PoolRegistry::get('slow'); + expect($slow?->count)->toBe(3); + $held = $slow->lockName(0); + expect($lock->acquire($held, machineLocal: true))->toBeTrue(); + + try { + QueueManager::dispatch(makeEmailMessage('long feed build'), queue: 'feed'); + QueueManager::dispatch(makeEmailMessage('due now'), queue: 'newsletter'); + + // The live worker is busy on the feed build, so it cannot take the + // newsletter: a free slot must be started rather than left idle behind it. + expect([...QueueManager::workerTransport($slow)->get()])->toHaveCount(1); + expect(pendingWorkers())->toBe(['fast:0', 'slow:1']); + } finally { + $lock->release($held); + unset($node->pools->slow->count); + QueueManager::reset(); + } +}); + it('removes old failed messages during cleanup', function () { $now = Mage_Core_Model_Locale::nowUtc(); queueAdapter()->insert(QueueManager::tableName(), [ diff --git a/tests/Backend/Integration/Queue/DbTransportTest.php b/tests/Backend/Integration/Queue/DbTransportTest.php index 7fffa7aee6..a375a5f8aa 100644 --- a/tests/Backend/Integration/Queue/DbTransportTest.php +++ b/tests/Backend/Integration/Queue/DbTransportTest.php @@ -195,6 +195,41 @@ expect($rows[0]['error_message'])->toContain('Nonexistent_Evil_Class'); }); +it('keeps a live claim out of the abandoned set', function () { + QueueManager::dispatch(makeEmailMessage()); + + $transport = QueueManager::dbTransport(); + $envelopes = [...$transport->get()]; + expect($envelopes)->toHaveCount(1); + + queueAdapter()->update(QueueManager::tableName(), [ + 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - DbTransport::ABANDONED_AFTER_SECONDS - 60), + ], ['message_id = ?' => (int) fetchQueueRows()[0]['message_id']]); + expect($transport->countAbandoned())->toBe(1); + + $transport->keepalive($envelopes[0]); + + expect($transport->countAbandoned())->toBe(0); + expect($transport->countClaimed())->toBe(1); +}); + +it('does not revive a claim that is no longer processing', function () { + QueueManager::dispatch(makeEmailMessage()); + + $transport = QueueManager::dbTransport(); + $envelopes = [...$transport->get()]; + queueAdapter()->update(QueueManager::tableName(), [ + 'status' => DbTransport::STATUS_PENDING, + 'claimed_at' => null, + ], ['message_id = ?' => (int) fetchQueueRows()[0]['message_id']]); + + $transport->keepalive($envelopes[0]); + + $row = fetchQueueRows()[0]; + expect($row['status'])->toBe(DbTransport::STATUS_PENDING); + expect($row['claimed_at'])->toBeNull(); +}); + it('counts pending messages and finds stored ones', function () { QueueManager::dispatch(makeEmailMessage()); QueueManager::dispatch(makeEmailMessage('second')); From 7597bb5665c187f7991bf8ccaa2fa0ad2331f26e Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Mon, 10 Aug 2026 11:16:40 +0100 Subject: [PATCH 11/13] Addressed review findings on queue worker pools - Disarmed the keepalive alarm after the worker run so a pending SIGALRM cannot kill the surrounding CLI process - Guarded ack() on processing status so a late ack cannot swallow a message an operator already retried - Made --exclusive refuse --queue and --exclude-queue, since a filtered worker cannot claim whole-roster coverage of the lock - Merged --exclude-queue with the pool's configured exclusions instead of replacing them - Parsed pool config booleans via Mage_Core_Model_Config_Element::is() so true works - Added a (status, queue, available_at) index for queue-filtered polls - Reported skipped non-retryable rows in the admin mass retry - Fetched only the status column in the retry pre-check - Warned at startup when pcntl is unavailable and keepalive is off --- app/code/core/Maho/Queue/Model/Cron.php | 1 - .../controllers/Adminhtml/QueueController.php | 13 +++++-- app/code/core/Maho/Queue/sql/schema.php | 4 +++ app/locale/en_US/Maho_Queue.csv | 1 + lib/Maho/Queue/PoolRegistry.php | 3 +- lib/Maho/Queue/QueueManager.php | 11 +++--- lib/Maho/Queue/Transport/DbTransport.php | 14 ++++++-- lib/MahoCLI/Commands/QueueWork.php | 34 +++++++++++++++++-- .../Integration/Queue/DbTransportTest.php | 22 ++++++++++++ tests/Backend/Integration/Queue/PoolTest.php | 13 +++++++ 10 files changed, 98 insertions(+), 18 deletions(-) diff --git a/app/code/core/Maho/Queue/Model/Cron.php b/app/code/core/Maho/Queue/Model/Cron.php index 98e39eb087..f568fccff7 100644 --- a/app/code/core/Maho/Queue/Model/Cron.php +++ b/app/code/core/Maho/Queue/Model/Cron.php @@ -130,5 +130,4 @@ private function spawnWorker(Pool $pool, int $index): void escapeshellarg(Mage::getBaseDir('var') . '/log/queue-worker.log'), )); } - } diff --git a/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php b/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php index 0e5936c7a5..9e0d8c108f 100644 --- a/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php +++ b/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php @@ -119,14 +119,21 @@ public function discardAction(): void public function massRetryAction(): void { $retried = 0; + $skipped = 0; foreach ($this->getMessageIds() as $id) { if (QueueManager::retryStoredMessage($id)) { $retried++; + } else { + $skipped++; } } - Mage::getSingleton('adminhtml/session')->addSuccess( - Mage::helper('queue')->__('%s message(s) re-queued.', $retried), - ); + $session = Mage::getSingleton('adminhtml/session'); + if ($retried > 0) { + $session->addSuccess(Mage::helper('queue')->__('%s message(s) re-queued.', $retried)); + } + if ($skipped > 0) { + $session->addNotice(Mage::helper('queue')->__('%s message(s) were skipped: only failed or stuck messages can be retried.', $skipped)); + } $this->_redirect('*/*/'); } diff --git a/app/code/core/Maho/Queue/sql/schema.php b/app/code/core/Maho/Queue/sql/schema.php index 5a519e0274..1771d8dfb0 100644 --- a/app/code/core/Maho/Queue/sql/schema.php +++ b/app/code/core/Maho/Queue/sql/schema.php @@ -34,7 +34,11 @@ $message->addPrimaryKeyConstraint( PrimaryKeyConstraint::editor()->setUnquotedColumnNames('message_id')->create(), ); + // (status, available_at, queue) serves unfiltered polls and countDue's date + // bound; (status, queue, available_at) lets a pool worker's queue IN (...) + // poll seek instead of walking every due row of the other pools' backlogs. $message->addIndex(['status', 'available_at', 'queue']); + $message->addIndex(['status', 'queue', 'available_at']); $message->addIndex(['dedupe_key']); $message->addIndex(['created_at']); $message->addIndex(['status', 'processed_at']); diff --git a/app/locale/en_US/Maho_Queue.csv b/app/locale/en_US/Maho_Queue.csv index 06f3e069f3..b00873bd90 100644 --- a/app/locale/en_US/Maho_Queue.csv +++ b/app/locale/en_US/Maho_Queue.csv @@ -1,5 +1,6 @@ "%s message(s) discarded.","%s message(s) discarded." "%s message(s) re-queued.","%s message(s) re-queued." +"%s message(s) were skipped: only failed or stuck messages can be retried.","%s message(s) were skipped: only failed or stuck messages can be retried." "%s queue message(s) were claimed by a worker that never finished. They are not re-queued automatically: retry or discard them.","%s queue message(s) were claimed by a worker that never finished. They are not re-queued automatically: retry or discard them." "0 keeps failed messages forever.","0 keeps failed messages forever." "0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days.","0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days." diff --git a/lib/Maho/Queue/PoolRegistry.php b/lib/Maho/Queue/PoolRegistry.php index 27d7178a26..1f6525cd3a 100644 --- a/lib/Maho/Queue/PoolRegistry.php +++ b/lib/Maho/Queue/PoolRegistry.php @@ -173,8 +173,9 @@ private static function routing(\Mage_Core_Model_Config_Element|false $node, arr return $queuesByPool; } + /** Defers to is() so 'true' means the same as everywhere else in config. */ private static function flag(\Mage_Core_Model_Config_Element $node, string $child, bool $default): bool { - return isset($node->{$child}) ? (bool) (int) $node->{$child} : $default; + return isset($node->{$child}) ? $node->is($child) : $default; } } diff --git a/lib/Maho/Queue/QueueManager.php b/lib/Maho/Queue/QueueManager.php index 4fe5fa3e65..a02639d8ae 100644 --- a/lib/Maho/Queue/QueueManager.php +++ b/lib/Maho/Queue/QueueManager.php @@ -145,18 +145,15 @@ public static function retryStoredMessage(int $messageId): bool { $adapter = self::writeAdapter(); $table = self::tableName(); - $row = $adapter->fetchRow( - $adapter->select()->from($table)->where('message_id = ?', $messageId), + $status = $adapter->fetchOne( + $adapter->select()->from($table, 'status')->where('message_id = ?', $messageId), ); - if ($row === false) { - return false; - } $where = ['message_id = ?' => $messageId]; - if ($row['status'] === DbTransport::STATUS_PROCESSING) { + if ($status === DbTransport::STATUS_PROCESSING) { $where['status = ?'] = DbTransport::STATUS_PROCESSING; $where['claimed_at < ?'] = DbTransport::abandonedBefore(); - } elseif ($row['status'] === DbTransport::STATUS_FAILED) { + } elseif ($status === DbTransport::STATUS_FAILED) { $where['status = ?'] = DbTransport::STATUS_FAILED; } else { return false; diff --git a/lib/Maho/Queue/Transport/DbTransport.php b/lib/Maho/Queue/Transport/DbTransport.php index c88f7444d4..4dece47cc1 100644 --- a/lib/Maho/Queue/Transport/DbTransport.php +++ b/lib/Maho/Queue/Transport/DbTransport.php @@ -133,19 +133,27 @@ public function getFromQueues(array $queueNames): iterable return $this->claimNext($queueNames); } + /** + * The status guard mirrors reject() and keepalive(): a row an operator + * already flipped back to pending must survive the late ack of the worker + * that used to hold it, or the retry is silently swallowed. + */ #[\Override] public function ack(Envelope $envelope): void { - $messageId = $this->messageId($envelope); + $where = [ + 'message_id = ?' => $this->messageId($envelope), + 'status = ?' => self::STATUS_PROCESSING, + ]; if ($this->completedRetentionDays > 0) { $now = \Mage_Core_Model_Locale::nowUtc(); $this->adapter->update($this->table, [ 'status' => self::STATUS_COMPLETED, 'processed_at' => $now, 'updated_at' => $now, - ], ['message_id = ?' => $messageId]); + ], $where); } else { - $this->adapter->delete($this->table, ['message_id = ?' => $messageId]); + $this->adapter->delete($this->table, $where); } } diff --git a/lib/MahoCLI/Commands/QueueWork.php b/lib/MahoCLI/Commands/QueueWork.php index d1cdf0129d..321175ffd4 100644 --- a/lib/MahoCLI/Commands/QueueWork.php +++ b/lib/MahoCLI/Commands/QueueWork.php @@ -71,6 +71,13 @@ protected function execute(InputInterface $input, OutputInterface $output): int } if ($input->getOption('exclusive')) { + // The exclusive lock tells the watchdog this process covers the whole + // roster (or a whole pool); a queue filter would make that a lie and + // silently starve every queue the filter leaves out. + if ($input->getOption('queue') !== [] || $input->getOption('exclude-queue') !== []) { + $output->writeln('--exclusive cannot be combined with --queue or --exclude-queue: the lock claims coverage the filter takes away'); + return Command::INVALID; + } $lockName = $pool?->lockName($index) ?? Pool::LOCK_PREFIX; if (!\Mage::getSingleton('core/lock')->acquire($lockName, machineLocal: true)) { $output->writeln("Another exclusive queue worker already holds {$lockName}"); @@ -86,8 +93,12 @@ protected function execute(InputInterface $input, OutputInterface $output): int queues: $queues ?: $base->queues, // An explicit allow-list already narrows the worker, and keeping the // pool's "everything but" list on top of it would leave - // `--pool=slow --queue=email` consuming nothing at all. - excludedQueues: $input->getOption('exclude-queue') ?: ($queues ? [] : $base->excludedQueues), + // `--pool=slow --queue=email` consuming nothing at all. Without an + // allow-list the pool's own exclusions stay: they are the catch-all's + // isolation boundary, and an extra --exclude-queue must not erase it. + excludedQueues: $queues + ? $input->getOption('exclude-queue') + : array_values(array_unique(array_merge($base->excludedQueues, $input->getOption('exclude-queue')))), idleTimeout: match (true) { $input->getOption('idle-timeout') !== null => max(0, (int) $input->getOption('idle-timeout')), (bool) $input->getOption('stop-when-empty') => 0, @@ -131,9 +142,26 @@ protected function execute(InputInterface $input, OutputInterface $output): int // Drives keepalive(): without it a slow handler is indistinguishable from a dead worker. if (SignalRegistry::isSupported()) { $this->getApplication()?->setAlarmInterval(DbTransport::KEEPALIVE_INTERVAL_SECONDS); + } else { + $output->writeln(sprintf( + 'pcntl is unavailable, so claims are not refreshed while a handler runs: a handler longer than %d seconds is reported as abandoned even though its worker is alive', + DbTransport::ABANDONED_AFTER_SECONDS, + )); } - $this->worker->run($options); + try { + $this->worker->run($options); + } finally { + // The recurring alarm outlives run(): the command's return pops the + // SIGALRM handler back to SIG_DFL while an alarm is still pending, + // and SIG_DFL terminates the process. Harmless for a detached worker + // about to exit, fatal for a parent command (email:queue:process) + // that invoked queue:work in-process and still has work to do. + if (SignalRegistry::isSupported()) { + $this->getApplication()?->setAlarmInterval(null); + pcntl_alarm(0); + } + } return Command::SUCCESS; } diff --git a/tests/Backend/Integration/Queue/DbTransportTest.php b/tests/Backend/Integration/Queue/DbTransportTest.php index a375a5f8aa..786923e6cb 100644 --- a/tests/Backend/Integration/Queue/DbTransportTest.php +++ b/tests/Backend/Integration/Queue/DbTransportTest.php @@ -230,6 +230,28 @@ expect($row['claimed_at'])->toBeNull(); }); +it('does not let a late ack swallow a row an operator already retried', function () { + QueueManager::dispatch(makeEmailMessage()); + + $transport = QueueManager::dbTransport(); + $envelopes = [...$transport->get()]; + $id = (int) fetchQueueRows()[0]['message_id']; + + // The claim goes stale (the worker looks dead), so the operator re-queues it. + queueAdapter()->update(QueueManager::tableName(), [ + 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200), + ], ['message_id = ?' => $id]); + expect(QueueManager::retryStoredMessage($id))->toBeTrue(); + + // The worker was alive after all and finishes: its ack must not delete or + // complete the pending row, or the operator's retry silently vanishes. + $transport->ack($envelopes[0]); + + $rows = fetchQueueRows(); + expect($rows)->toHaveCount(1); + expect($rows[0]['status'])->toBe(DbTransport::STATUS_PENDING); +}); + it('counts pending messages and finds stored ones', function () { QueueManager::dispatch(makeEmailMessage()); QueueManager::dispatch(makeEmailMessage('second')); diff --git a/tests/Backend/Integration/Queue/PoolTest.php b/tests/Backend/Integration/Queue/PoolTest.php index 9e1b767efb..40d2fc0c02 100644 --- a/tests/Backend/Integration/Queue/PoolTest.php +++ b/tests/Backend/Integration/Queue/PoolTest.php @@ -139,6 +139,19 @@ public function stop(): void }); }); +it('reads pool flags with core config semantics, so "true" means on', function () { + withQueueConfig('zz_flagtrue5', function () { + expect(PoolRegistry::get('zz_flag'))->not->toBeNull(); + }); +}); + +it('drops a pool declared inactive with "false"', function () { + withQueueConfig('zz_flagfalse5', function () { + expect(PoolRegistry::get('zz_flag'))->toBeNull(); + expect(PoolRegistry::poolFor('zz_q')?->name)->toBe('slow'); + }); +}); + it('does not hand the catch-all worker a message belonging to another pool', function () { QueueManager::dispatch(makeEmailMessage(), queue: Mage_Core_Model_Email_Queue::QUEUE_NAME); QueueManager::dispatch(makeEmailMessage('newsletter batch'), queue: 'newsletter'); From 1b82e7f2872f512d97a03b87f47caff25aef0b98 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Mon, 10 Aug 2026 12:36:04 +0100 Subject: [PATCH 12/13] Addressed review findings on queue retry, keepalive, and pool config - Guarded the retry re-send UPDATE with status=processing so a stale worker's late re-send cannot overwrite a row an operator already retried, completed, or deleted - Skipped the keepalive refresh while the shared connection is inside a handler's transaction, so the refresh never locks the row against the admin retry or vanishes on rollback - Let memory_limit 0 mean unbounded instead of falling back to 256M, matching time_limit and the CLI path - Extracted the abandoned-claim predicate into DbTransport::isAbandonedClaim() and used it in the admin view - Documented that spawn counts are cluster-global while worker locks are machine-local --- .../Queue/Block/Adminhtml/Message/View.php | 3 +- app/code/core/Maho/Queue/Model/Cron.php | 5 +- lib/Maho/Queue/PoolRegistry.php | 2 +- lib/Maho/Queue/Transport/DbTransport.php | 20 +++++++- .../Integration/Queue/DbTransportTest.php | 48 +++++++++++++++++++ tests/Backend/Integration/Queue/PoolTest.php | 6 +++ 6 files changed, 79 insertions(+), 5 deletions(-) diff --git a/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php b/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php index 73ea51abf3..b2f7ddd852 100644 --- a/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php +++ b/app/code/core/Maho/Queue/Block/Adminhtml/Message/View.php @@ -42,8 +42,7 @@ public function isRetryable(): bool return match ($message?->getStatus()) { Maho_Queue_Model_Message::STATUS_FAILED => true, - Maho_Queue_Model_Message::STATUS_PROCESSING => $message->getClaimedAt() !== null - && $message->getClaimedAt() < \Maho\Queue\Transport\DbTransport::abandonedBefore(), + Maho_Queue_Model_Message::STATUS_PROCESSING => \Maho\Queue\Transport\DbTransport::isAbandonedClaim($message->getClaimedAt()), default => false, }; } diff --git a/app/code/core/Maho/Queue/Model/Cron.php b/app/code/core/Maho/Queue/Model/Cron.php index f568fccff7..186b62f4c2 100644 --- a/app/code/core/Maho/Queue/Model/Cron.php +++ b/app/code/core/Maho/Queue/Model/Cron.php @@ -70,7 +70,10 @@ public function workersToSpawn(): array foreach ($free as $index) { // One process per due message. A busy worker cannot take one, and holds - // exactly one claim, so live claims come off the roster. + // exactly one claim, so live claims come off the roster. Due and busy + // counts are cluster-global while locks are machine-local, so on + // multi-server installs each server may spawn for the same backlog; + // the excess is bounded by pool->count and drains via idle timeout. if ($pool->isOnDemand()) { $due ??= $this->dueWorkCount($pool); $idle ??= $live === 0 ? 0 : max(0, $live - $this->busyWorkerCount($pool)); diff --git a/lib/Maho/Queue/PoolRegistry.php b/lib/Maho/Queue/PoolRegistry.php index 1f6525cd3a..78682345bb 100644 --- a/lib/Maho/Queue/PoolRegistry.php +++ b/lib/Maho/Queue/PoolRegistry.php @@ -129,7 +129,7 @@ private static function build(): array excludedQueues: $excluded, count: max(1, (int) ($child->count ?? 1)), idleTimeout: isset($child->idle_timeout) ? max(0, (int) $child->idle_timeout) : null, - memoryLimit: trim((string) ($child->memory_limit ?? '')) ?: '256M', + memoryLimit: isset($child->memory_limit) ? trim((string) $child->memory_limit) : '256M', timeLimit: max(0, (int) ($child->time_limit ?? 3600)), ); } diff --git a/lib/Maho/Queue/Transport/DbTransport.php b/lib/Maho/Queue/Transport/DbTransport.php index 4dece47cc1..3c59d84d38 100644 --- a/lib/Maho/Queue/Transport/DbTransport.php +++ b/lib/Maho/Queue/Transport/DbTransport.php @@ -83,6 +83,8 @@ public function send(Envelope $envelope): Envelope // A failure-transport send carries both stamps too, but its id belongs to the origin transport: insert, not update. if ($messageIdStamp !== null && $redeliveryStamp !== null && $envelope->last(SentToFailureTransportStamp::class) === null) { + // The status guard mirrors ack(): a row an operator already re-queued, + // completed or deleted must survive a stale worker's late re-send. $this->adapter->update($this->table, [ 'status' => self::STATUS_PENDING, 'retries' => $redeliveryStamp->getRetryCount(), @@ -90,7 +92,10 @@ public function send(Envelope $envelope): Envelope 'error_message' => $envelope->last(ErrorDetailsStamp::class)?->getExceptionMessage(), 'claimed_at' => null, 'updated_at' => $now, - ], ['message_id = ?' => (int) $messageIdStamp->getId()]); + ], [ + 'message_id = ?' => (int) $messageIdStamp->getId(), + 'status = ?' => self::STATUS_PROCESSING, + ]); return $envelope; } @@ -180,6 +185,13 @@ public function reject(Envelope $envelope): void #[\Override] public function keepalive(Envelope $envelope, ?int $seconds = null): void { + // The alarm can fire while the handler holds an open transaction on this + // shared connection; joining it would lock the row against the admin's + // retry and lose the refresh on rollback. Skip and let the next one land. + if ($this->adapter->getTransactionLevel() > 0) { + return; + } + $now = \Mage_Core_Model_Locale::nowUtc(); $this->adapter->update($this->table, [ 'claimed_at' => $now, @@ -356,6 +368,12 @@ public static function abandonedBefore(int $olderThanSeconds = self::ABANDONED_A return gmdate(\Mage_Core_Model_Locale::DATETIME_FORMAT, time() - $olderThanSeconds); } + /** True when a processing row's claim is old enough to belong to a dead worker. */ + public static function isAbandonedClaim(?string $claimedAt): bool + { + return $claimedAt !== null && $claimedAt < self::abandonedBefore(); + } + /** * @param array $row */ diff --git a/tests/Backend/Integration/Queue/DbTransportTest.php b/tests/Backend/Integration/Queue/DbTransportTest.php index 786923e6cb..b35e2b8296 100644 --- a/tests/Backend/Integration/Queue/DbTransportTest.php +++ b/tests/Backend/Integration/Queue/DbTransportTest.php @@ -252,6 +252,54 @@ expect($rows[0]['status'])->toBe(DbTransport::STATUS_PENDING); }); +it('does not let a late retry re-send overwrite a row an operator already retried', function () { + QueueManager::dispatch(makeEmailMessage()); + + $transport = QueueManager::dbTransport(); + $envelopes = [...$transport->get()]; + $id = (int) fetchQueueRows()[0]['message_id']; + + // The claim goes stale (the worker looks dead), so the operator re-queues it. + queueAdapter()->update(QueueManager::tableName(), [ + 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200), + ], ['message_id = ?' => $id]); + expect(QueueManager::retryStoredMessage($id))->toBeTrue(); + + // The worker was alive after all and its handler fails: the retry listener's + // in-place re-send must leave the operator's fresh pending row alone. + $transport->send($envelopes[0]->with( + new RedeliveryStamp(1), + ErrorDetailsStamp::create(new RuntimeException('handler blew up')), + )); + + $rows = fetchQueueRows(); + expect($rows)->toHaveCount(1); + expect($rows[0]['status'])->toBe(DbTransport::STATUS_PENDING); + expect((int) $rows[0]['retries'])->toBe(0); +}); + +it('skips the keepalive refresh while the shared connection is inside a transaction', function () { + QueueManager::dispatch(makeEmailMessage()); + + $transport = QueueManager::dbTransport(); + $envelopes = [...$transport->get()]; + $id = (int) fetchQueueRows()[0]['message_id']; + + $stale = gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200); + queueAdapter()->update(QueueManager::tableName(), ['claimed_at' => $stale], ['message_id = ?' => $id]); + + queueAdapter()->beginTransaction(); + try { + $transport->keepalive($envelopes[0]); + expect(fetchQueueRows()[0]['claimed_at'])->toBe($stale); + } finally { + queueAdapter()->rollBack(); + } + + $transport->keepalive($envelopes[0]); + expect(fetchQueueRows()[0]['claimed_at'])->not->toBe($stale); +}); + it('counts pending messages and finds stored ones', function () { QueueManager::dispatch(makeEmailMessage()); QueueManager::dispatch(makeEmailMessage('second')); diff --git a/tests/Backend/Integration/Queue/PoolTest.php b/tests/Backend/Integration/Queue/PoolTest.php index 40d2fc0c02..f62c92a267 100644 --- a/tests/Backend/Integration/Queue/PoolTest.php +++ b/tests/Backend/Integration/Queue/PoolTest.php @@ -152,6 +152,12 @@ public function stop(): void }); }); +it('keeps memory_limit 0 as unbounded instead of falling back to the default', function () { + withQueueConfig('zz_pool50', function () { + expect(queuePool('zz_pool')->memoryLimit)->toBe('0'); + }); +}); + it('does not hand the catch-all worker a message belonging to another pool', function () { QueueManager::dispatch(makeEmailMessage(), queue: Mage_Core_Model_Email_Queue::QUEUE_NAME); QueueManager::dispatch(makeEmailMessage('newsletter batch'), queue: 'newsletter'); From 6b971ff3679a6f03e21611193df9ba0ac431e692 Mon Sep 17 00:00:00 2001 From: Fabrizio Balliano Date: Mon, 10 Aug 2026 13:29:32 +0100 Subject: [PATCH 13/13] Addressed review findings on queue claim ownership, dedupe retry, and pool validation - Added a claim_token stamp so ack/reject/keepalive/re-send only touch a row the worker still owns; an operator retry hands the row to a new token - Refused retry of a parked message while a newer in-flight copy of its dedupe key exists - Validated pool memory_limit at build time with a logged 256M fallback instead of a silent worker respawn loop - Cached the abandoned-message admin notice count for 60s; retry/discard drop the cache - Warned when a bare queue:work --exclusive starts alongside live pool workers - Consolidated transport count queries, hoisted shared test helpers into Pest.php, removed the transport-name vestige --- AGENTS.md | 4 +- app/code/core/Maho/Queue/Model/Observer.php | 19 ++- .../controllers/Adminhtml/QueueController.php | 14 +- app/code/core/Maho/Queue/sql/schema.php | 1 + app/locale/en_US/Maho_Queue.csv | 4 +- lib/Maho/Queue/Pool.php | 15 ++ lib/Maho/Queue/PoolRegistry.php | 21 ++- lib/Maho/Queue/QueueManager.php | 23 ++- lib/Maho/Queue/Stamp/ClaimTokenStamp.php | 24 ++++ lib/Maho/Queue/Transport/DbTransport.php | 135 +++++++++++------- lib/Maho/Queue/WorkerFactory.php | 7 +- lib/MahoCLI/Commands/QueueWork.php | 32 +++-- .../Integration/Queue/CronConsumerTest.php | 82 +++++------ .../Integration/Queue/DbTransportTest.php | 74 ++++++++-- tests/Backend/Integration/Queue/PoolTest.php | 37 +---- tests/Pest.php | 30 ++++ 16 files changed, 357 insertions(+), 165 deletions(-) create mode 100644 lib/Maho/Queue/Stamp/ClaimTokenStamp.php diff --git a/AGENTS.md b/AGENTS.md index e1e553ba52..df88a7091f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -179,7 +179,9 @@ class My_Module_Checkout_CartController extends Mage_Checkout_CartController { / resourcing (count, memory/time limits, idle timeout) lives under `global/queue/pools`. A crash parks a claimed message for an operator instead of redelivering it: retry or discard it in the grid. A handler may run as long as it needs: the worker refreshes its claim on Symfony's - keepalive alarm, so only a worker that actually died is reported as abandoned. Worker startup + keepalive alarm, so only a worker that actually died is reported as abandoned (the refresh + needs pcntl and cannot land while the handler holds an open transaction on the shared + connection, so such a worker can be misreported after 5 minutes). Worker startup failures land in `var/log/queue-worker.log`; production installs should prefer supervisord or systemd over the cron watchdog - **Layout**: XML-based block hierarchy and template assignment diff --git a/app/code/core/Maho/Queue/Model/Observer.php b/app/code/core/Maho/Queue/Model/Observer.php index 28295c9a16..008944cf00 100644 --- a/app/code/core/Maho/Queue/Model/Observer.php +++ b/app/code/core/Maho/Queue/Model/Observer.php @@ -12,6 +12,14 @@ class Maho_Queue_Model_Observer { + /** + * Cached count so every admin page load does not pay a queue-table query; + * well under the abandonment threshold, so the notice is never meaningfully + * late. The retry/discard actions drop the key for instant feedback. + */ + public const ABANDONED_COUNT_CACHE_KEY = 'maho_queue_abandoned_count'; + private const ABANDONED_COUNT_CACHE_SECONDS = 60; + /** * Nothing re-queues a claim a dead worker left behind, so the grid is the * only way one comes back and somebody has to be told to look at it. @@ -46,10 +54,19 @@ public function warnAboutAbandonedMessages(): void /** Caught rather than probed: isTableExists() would introspect the schema on every request. */ private function abandonedMessageCount(): int { + $cached = Mage::app()->loadCache(self::ABANDONED_COUNT_CACHE_KEY); + if (is_string($cached) && $cached !== '') { + return (int) $cached; + } + try { - return QueueManager::dbTransport()->countAbandoned(); + $count = QueueManager::dbTransport()->countAbandoned(); } catch (Exception) { return 0; } + + Mage::app()->saveCache((string) $count, self::ABANDONED_COUNT_CACHE_KEY, [], self::ABANDONED_COUNT_CACHE_SECONDS); + + return $count; } } diff --git a/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php b/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php index 9e0d8c108f..70d2f4c03f 100644 --- a/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php +++ b/app/code/core/Maho/Queue/controllers/Adminhtml/QueueController.php @@ -98,8 +98,9 @@ public function retryAction(): void if (QueueManager::retryStoredMessage($id)) { Mage::getSingleton('adminhtml/session')->addSuccess(Mage::helper('queue')->__('Message re-queued.')); } else { - Mage::getSingleton('adminhtml/session')->addError(Mage::helper('queue')->__('Only failed or stuck messages can be retried.')); + Mage::getSingleton('adminhtml/session')->addError(Mage::helper('queue')->__('The message was not retried: only failed or stuck messages without a newer copy of their dedupe key can be retried.')); } + $this->refreshAbandonedNotice(); $this->_redirect('*/*/'); } @@ -112,6 +113,7 @@ public function discardAction(): void } else { Mage::getSingleton('adminhtml/session')->addError(Mage::helper('queue')->__('Message not found.')); } + $this->refreshAbandonedNotice(); $this->_redirect('*/*/'); } @@ -132,8 +134,9 @@ public function massRetryAction(): void $session->addSuccess(Mage::helper('queue')->__('%s message(s) re-queued.', $retried)); } if ($skipped > 0) { - $session->addNotice(Mage::helper('queue')->__('%s message(s) were skipped: only failed or stuck messages can be retried.', $skipped)); + $session->addNotice(Mage::helper('queue')->__('%s message(s) were skipped: only failed or stuck messages without a newer copy of their dedupe key can be retried.', $skipped)); } + $this->refreshAbandonedNotice(); $this->_redirect('*/*/'); } @@ -149,9 +152,16 @@ public function massDiscardAction(): void Mage::getSingleton('adminhtml/session')->addSuccess( Mage::helper('queue')->__('%s message(s) discarded.', $discarded), ); + $this->refreshAbandonedNotice(); $this->_redirect('*/*/'); } + /** The abandoned-count notice is cached; an operator action must not leave it stale. */ + private function refreshAbandonedNotice(): void + { + Mage::app()->removeCache(Maho_Queue_Model_Observer::ABANDONED_COUNT_CACHE_KEY); + } + /** * @return list */ diff --git a/app/code/core/Maho/Queue/sql/schema.php b/app/code/core/Maho/Queue/sql/schema.php index 1771d8dfb0..4553dfaa6f 100644 --- a/app/code/core/Maho/Queue/sql/schema.php +++ b/app/code/core/Maho/Queue/sql/schema.php @@ -26,6 +26,7 @@ $message->addColumn('dedupe_key', Types::STRING, ['length' => 64, 'notnull' => false]); $message->addColumn('available_at', Types::DATETIME_MUTABLE, []); $message->addColumn('claimed_at', Types::DATETIME_MUTABLE, ['notnull' => false]); + $message->addColumn('claim_token', Types::STRING, ['length' => 32, 'notnull' => false]); $message->addColumn('processed_at', Types::DATETIME_MUTABLE, ['notnull' => false]); $message->addColumn('created_at', Types::DATETIME_MUTABLE, ['default' => new CurrentTimestamp()]); // Transport keeps updated_at current on every write; the on-update diff --git a/app/locale/en_US/Maho_Queue.csv b/app/locale/en_US/Maho_Queue.csv index b00873bd90..298fdc38e9 100644 --- a/app/locale/en_US/Maho_Queue.csv +++ b/app/locale/en_US/Maho_Queue.csv @@ -1,6 +1,6 @@ "%s message(s) discarded.","%s message(s) discarded." "%s message(s) re-queued.","%s message(s) re-queued." -"%s message(s) were skipped: only failed or stuck messages can be retried.","%s message(s) were skipped: only failed or stuck messages can be retried." +"%s message(s) were skipped: only failed or stuck messages without a newer copy of their dedupe key can be retried.","%s message(s) were skipped: only failed or stuck messages without a newer copy of their dedupe key can be retried." "%s queue message(s) were claimed by a worker that never finished. They are not re-queued automatically: retry or discard them.","%s queue message(s) were claimed by a worker that never finished. They are not re-queued automatically: retry or discard them." "0 keeps failed messages forever.","0 keeps failed messages forever." "0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days.","0 removes messages immediately on success; a positive value keeps them visible in the grid for this many days." @@ -32,7 +32,6 @@ "Message not found.","Message not found." "Message Queue","Message Queue" "Message re-queued.","Message re-queued." -"Only failed or stuck messages can be retried.","Only failed or stuck messages can be retried." "Pending","Pending" "Permanently delete the selected messages?","Permanently delete the selected messages?" "Permanently delete this message?","Permanently delete this message?" @@ -48,6 +47,7 @@ "Retry Delay Multiplier","Retry Delay Multiplier" "Retry Messages","Retry Messages" "Status","Status" +"The message was not retried: only failed or stuck messages without a newer copy of their dedupe key can be retried.","The message was not retried: only failed or stuck messages without a newer copy of their dedupe key can be retried." "Upper bound for the backoff. 0 means no bound.","Upper bound for the backoff. 0 means no bound." "View","View" "View Messages","View Messages" diff --git a/lib/Maho/Queue/Pool.php b/lib/Maho/Queue/Pool.php index 91a3c1adea..c2adb25fb4 100644 --- a/lib/Maho/Queue/Pool.php +++ b/lib/Maho/Queue/Pool.php @@ -57,4 +57,19 @@ public function consumes(string $queue): bool return $this->queues === [] || in_array($queue, $this->queues, true); } + + /** Bytes for a shorthand like "256M", or null when the string is not a memory limit at all. */ + public static function parseMemoryLimit(string $limit): ?int + { + if (!preg_match('/^(\d+)([KMG]?)$/i', trim($limit), $matches)) { + return null; + } + + return (int) $matches[1] * match (strtoupper($matches[2])) { + 'K' => 1024, + 'M' => 1024 ** 2, + 'G' => 1024 ** 3, + default => 1, + }; + } } diff --git a/lib/Maho/Queue/PoolRegistry.php b/lib/Maho/Queue/PoolRegistry.php index 78682345bb..0650ac1f05 100644 --- a/lib/Maho/Queue/PoolRegistry.php +++ b/lib/Maho/Queue/PoolRegistry.php @@ -129,7 +129,7 @@ private static function build(): array excludedQueues: $excluded, count: max(1, (int) ($child->count ?? 1)), idleTimeout: isset($child->idle_timeout) ? max(0, (int) $child->idle_timeout) : null, - memoryLimit: isset($child->memory_limit) ? trim((string) $child->memory_limit) : '256M', + memoryLimit: self::memoryLimit($child, (string) $name), timeLimit: max(0, (int) ($child->time_limit ?? 3600)), ); } @@ -178,4 +178,23 @@ private static function flag(\Mage_Core_Model_Config_Element $node, string $chil { return isset($node->{$child}) ? $node->is($child) : $default; } + + /** + * Rejected here rather than in the spawned worker: an invalid limit there + * makes queue:work exit before taking its lock, so the watchdog would + * respawn it every minute while the pool's queues silently stop draining. + */ + private static function memoryLimit(\Mage_Core_Model_Config_Element $child, string $name): string + { + $limit = isset($child->memory_limit) ? trim((string) $child->memory_limit) : '256M'; + if ($limit !== '' && Pool::parseMemoryLimit($limit) === null) { + \Mage::log( + sprintf('Queue pool "%s" has an invalid memory_limit "%s"; using 256M', $name, $limit), + \Mage::LOG_ERROR, + ); + return '256M'; + } + + return $limit; + } } diff --git a/lib/Maho/Queue/QueueManager.php b/lib/Maho/Queue/QueueManager.php index a02639d8ae..73dee41982 100644 --- a/lib/Maho/Queue/QueueManager.php +++ b/lib/Maho/Queue/QueueManager.php @@ -140,25 +140,39 @@ public static function serializer(): Serializer * timer-based redelivery was meant to make impossible. The age cut-off is * applied in the UPDATE, so a worker that claims the row between the read * and the write keeps it. + * + * A deduped row is refused while a newer copy of its key is in flight: + * once an abandoned claim stops suppressing dispatches, the fresh copy + * supersedes it and retrying the old one would run the job twice. Discard + * is the way out for that row. */ public static function retryStoredMessage(int $messageId): bool { $adapter = self::writeAdapter(); $table = self::tableName(); - $status = $adapter->fetchOne( - $adapter->select()->from($table, 'status')->where('message_id = ?', $messageId), + $row = $adapter->fetchRow( + $adapter->select()->from($table, ['status', 'dedupe_key'])->where('message_id = ?', $messageId), ); + if ($row === false) { + return false; + } $where = ['message_id = ?' => $messageId]; - if ($status === DbTransport::STATUS_PROCESSING) { + if ($row['status'] === DbTransport::STATUS_PROCESSING) { $where['status = ?'] = DbTransport::STATUS_PROCESSING; $where['claimed_at < ?'] = DbTransport::abandonedBefore(); - } elseif ($status === DbTransport::STATUS_FAILED) { + } elseif ($row['status'] === DbTransport::STATUS_FAILED) { $where['status = ?'] = DbTransport::STATUS_FAILED; } else { return false; } + if ($row['dedupe_key'] !== null + && self::dbTransport()->inFlightRowExists((string) $row['dedupe_key'], $messageId) + ) { + return false; + } + $now = \Mage_Core_Model_Locale::nowUtc(); return $adapter->update($table, [ @@ -166,6 +180,7 @@ public static function retryStoredMessage(int $messageId): bool 'retries' => 0, 'available_at' => $now, 'claimed_at' => null, + 'claim_token' => null, 'processed_at' => null, 'updated_at' => $now, ], $where) === 1; diff --git a/lib/Maho/Queue/Stamp/ClaimTokenStamp.php b/lib/Maho/Queue/Stamp/ClaimTokenStamp.php new file mode 100644 index 0000000000..f5516322be --- /dev/null +++ b/lib/Maho/Queue/Stamp/ClaimTokenStamp.php @@ -0,0 +1,24 @@ + + * SPDX-License-Identifier: OSL-3.0 + */ + +declare(strict_types=1); + +namespace Maho\Queue\Stamp; + +use Symfony\Component\Messenger\Stamp\StampInterface; + +/** + * Proof of claim ownership: written to the row when a worker claims it and + * carried on the envelope, so a stale worker's late ack, reject, re-send or + * keepalive can never touch a row another worker has since claimed. + */ +final readonly class ClaimTokenStamp implements StampInterface +{ + public function __construct( + public string $token, + ) {} +} diff --git a/lib/Maho/Queue/Transport/DbTransport.php b/lib/Maho/Queue/Transport/DbTransport.php index 3c59d84d38..7921adf5fc 100644 --- a/lib/Maho/Queue/Transport/DbTransport.php +++ b/lib/Maho/Queue/Transport/DbTransport.php @@ -10,6 +10,7 @@ namespace Maho\Queue\Transport; use Maho\Db\Adapter\AdapterInterface; +use Maho\Queue\Stamp\ClaimTokenStamp; use Maho\Queue\Stamp\DedupeKeyStamp; use Maho\Queue\Stamp\QueueNameStamp; use Symfony\Component\Messenger\Envelope; @@ -83,19 +84,18 @@ public function send(Envelope $envelope): Envelope // A failure-transport send carries both stamps too, but its id belongs to the origin transport: insert, not update. if ($messageIdStamp !== null && $redeliveryStamp !== null && $envelope->last(SentToFailureTransportStamp::class) === null) { - // The status guard mirrors ack(): a row an operator already re-queued, - // completed or deleted must survive a stale worker's late re-send. + // The ownership guard mirrors ack(): a row an operator already re-queued, + // completed, deleted or that another worker has since claimed must + // survive a stale worker's late re-send. $this->adapter->update($this->table, [ 'status' => self::STATUS_PENDING, 'retries' => $redeliveryStamp->getRetryCount(), 'available_at' => $availableAt, 'error_message' => $envelope->last(ErrorDetailsStamp::class)?->getExceptionMessage(), 'claimed_at' => null, + 'claim_token' => null, 'updated_at' => $now, - ], [ - 'message_id = ?' => (int) $messageIdStamp->getId(), - 'status = ?' => self::STATUS_PROCESSING, - ]); + ], $this->ownershipWhere($envelope)); return $envelope; } @@ -139,21 +139,20 @@ public function getFromQueues(array $queueNames): iterable } /** - * The status guard mirrors reject() and keepalive(): a row an operator - * already flipped back to pending must survive the late ack of the worker - * that used to hold it, or the retry is silently swallowed. + * The ownership guard mirrors reject() and keepalive(): a row an operator + * already flipped back to pending, or that another worker has since + * claimed, must survive the late ack of the worker that used to hold it, + * or the retry (or the other worker's run) is silently swallowed. */ #[\Override] public function ack(Envelope $envelope): void { - $where = [ - 'message_id = ?' => $this->messageId($envelope), - 'status = ?' => self::STATUS_PROCESSING, - ]; + $where = $this->ownershipWhere($envelope); if ($this->completedRetentionDays > 0) { $now = \Mage_Core_Model_Locale::nowUtc(); $this->adapter->update($this->table, [ 'status' => self::STATUS_COMPLETED, + 'claim_token' => null, 'processed_at' => $now, 'updated_at' => $now, ], $where); @@ -169,18 +168,17 @@ public function reject(Envelope $envelope): void // A row the retry listener already re-queued in place stays pending. $this->adapter->update($this->table, [ 'status' => self::STATUS_FAILED, + 'claim_token' => null, 'error_message' => $envelope->last(ErrorDetailsStamp::class)?->getExceptionMessage(), 'processed_at' => $now, 'updated_at' => $now, - ], [ - 'message_id = ?' => $this->messageId($envelope), - 'status = ?' => self::STATUS_PROCESSING, - ]); + ], $this->ownershipWhere($envelope)); } /** * Makes claimed_at mean "a worker is alive here", not "a worker started here - * a while ago". The status guard leaves a row an operator already retried alone. + * a while ago". The ownership guard leaves a row an operator already retried + * (or another worker has since claimed) alone. */ #[\Override] public function keepalive(Envelope $envelope, ?int $seconds = null): void @@ -196,10 +194,30 @@ public function keepalive(Envelope $envelope, ?int $seconds = null): void $this->adapter->update($this->table, [ 'claimed_at' => $now, 'updated_at' => $now, - ], [ + ], $this->ownershipWhere($envelope)); + } + + /** + * WHERE clause proving the caller still owns the row: the claim token + * written when it claimed, when the envelope carries one. Status alone + * cannot tell "my claim" from a newer claim another worker took after an + * operator retried this one. + * + * @return array + */ + private function ownershipWhere(Envelope $envelope): array + { + $where = [ 'message_id = ?' => $this->messageId($envelope), 'status = ?' => self::STATUS_PROCESSING, - ]); + ]; + + $token = $envelope->last(ClaimTokenStamp::class)?->token; + if ($token !== null) { + $where['claim_token = ?'] = $token; + } + + return $where; } #[\Override] @@ -233,12 +251,7 @@ public function find(mixed $id): ?Envelope #[\Override] public function getMessageCount(): int { - $select = $this->adapter->select() - ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) - ->where('status = ?', self::STATUS_PENDING); - $this->applyQueueFilter($select, null); - - return (int) $this->adapter->fetchOne($select); + return $this->countRows(['status = ?' => self::STATUS_PENDING]); } /** @@ -250,25 +263,19 @@ public function getMessageCount(): int */ public function countDue(?array $queues = null): int { - $select = $this->adapter->select() - ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) - ->where('status = ?', self::STATUS_PENDING) - ->where('available_at <= ?', \Mage_Core_Model_Locale::nowUtc()); - $this->applyQueueFilter($select, $queues); - - return (int) $this->adapter->fetchOne($select); + return $this->countRows([ + 'status = ?' => self::STATUS_PENDING, + 'available_at <= ?' => \Mage_Core_Model_Locale::nowUtc(), + ], $queues); } /** Drives the admin notice. A working worker refreshes its claim, so this counts dead ones. */ public function countAbandoned(int $olderThanSeconds = self::ABANDONED_AFTER_SECONDS): int { - $select = $this->adapter->select() - ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) - ->where('status = ?', self::STATUS_PROCESSING) - ->where('claimed_at < ?', self::abandonedBefore($olderThanSeconds)); - $this->applyQueueFilter($select, null); - - return (int) $this->adapter->fetchOne($select); + return $this->countRows([ + 'status = ?' => self::STATUS_PROCESSING, + 'claimed_at < ?' => self::abandonedBefore($olderThanSeconds), + ]); } /** @@ -278,10 +285,22 @@ public function countAbandoned(int $olderThanSeconds = self::ABANDONED_AFTER_SEC */ public function countClaimed(?array $queues = null): int { - $select = $this->adapter->select() - ->from($this->table, new \Maho\Db\Expr('COUNT(*)')) - ->where('status = ?', self::STATUS_PROCESSING) - ->where('claimed_at >= ?', self::abandonedBefore()); + return $this->countRows([ + 'status = ?' => self::STATUS_PROCESSING, + 'claimed_at >= ?' => self::abandonedBefore(), + ], $queues); + } + + /** + * @param array $conditions + * @param list|null $queues + */ + private function countRows(array $conditions, ?array $queues = null): int + { + $select = $this->adapter->select()->from($this->table, new \Maho\Db\Expr('COUNT(*)')); + foreach ($conditions as $condition => $value) { + $select->where($condition, $value); + } $this->applyQueueFilter($select, $queues); return (int) $this->adapter->fetchOne($select); @@ -322,9 +341,11 @@ private function claimNext(?array $queues): array return []; } + $token = bin2hex(random_bytes(16)); $claimed = $this->adapter->update($this->table, [ 'status' => self::STATUS_PROCESSING, 'claimed_at' => $now, + 'claim_token' => $token, 'updated_at' => $now, ], [ 'message_id = ?' => (int) $row['message_id'], @@ -337,27 +358,33 @@ private function claimNext(?array $queues): array $envelope = $this->hydrateOrFail($row); if ($envelope !== null) { - return [$envelope]; + return [$envelope->with(new ClaimTokenStamp($token))]; } } return []; } - private function inFlightRowExists(string $dedupeKey): bool + /** + * A pending or freshly claimed row already carries this dedupe key: gates + * both new dispatches and the admin retry of an older copy. + */ + public function inFlightRowExists(string $dedupeKey, ?int $excludeMessageId = null): bool { // A claim a dead worker left behind waits for an operator forever, so it // must not keep suppressing new dispatches of the same key: that would // silently drop every later send instead of parking one message. // Pending rows always have a null claimed_at, so they are never excluded. - $existing = $this->adapter->fetchOne( - $this->adapter->select() - ->from($this->table, 'message_id') - ->where('dedupe_key = ?', $dedupeKey) - ->where('status IN (?)', [self::STATUS_PENDING, self::STATUS_PROCESSING]) - ->where('claimed_at IS NULL OR claimed_at >= ?', self::abandonedBefore()) - ->limit(1), - ); + $select = $this->adapter->select() + ->from($this->table, 'message_id') + ->where('dedupe_key = ?', $dedupeKey) + ->where('status IN (?)', [self::STATUS_PENDING, self::STATUS_PROCESSING]) + ->where('claimed_at IS NULL OR claimed_at >= ?', self::abandonedBefore()) + ->limit(1); + if ($excludeMessageId !== null) { + $select->where('message_id != ?', $excludeMessageId); + } + $existing = $this->adapter->fetchOne($select); return $existing !== false && $existing !== null; } diff --git a/lib/Maho/Queue/WorkerFactory.php b/lib/Maho/Queue/WorkerFactory.php index 983979f174..426f382953 100644 --- a/lib/Maho/Queue/WorkerFactory.php +++ b/lib/Maho/Queue/WorkerFactory.php @@ -31,7 +31,6 @@ final class WorkerFactory */ public static function create(array $options = []): Worker { - $transportName = QueueManager::TRANSPORT_DB; $transport = QueueManager::workerTransport($options['pool'] ?? null); $dispatcher = new EventDispatcher(); @@ -39,8 +38,8 @@ public static function create(array $options = []): Worker $dispatcher->addSubscriber(new DispatchPcntlSignalListener()); $dispatcher->addSubscriber(new SendFailedMessageForRetryListener( - new ServiceLocator([$transportName => $transport]), - new ServiceLocator([$transportName => new MultiplierRetryStrategy( + new ServiceLocator([QueueManager::TRANSPORT_DB => $transport]), + new ServiceLocator([QueueManager::TRANSPORT_DB => new MultiplierRetryStrategy( (int) \Mage::getStoreConfig(QueueManager::XML_PATH_MAX_RETRIES), (int) \Mage::getStoreConfig(QueueManager::XML_PATH_RETRY_DELAY) * 1000, (float) \Mage::getStoreConfig(QueueManager::XML_PATH_RETRY_MULTIPLIER), @@ -66,6 +65,6 @@ public static function create(array $options = []): Worker $dispatcher->addSubscriber(new StopWorkerWhenIdleListener($options['idleTimeout'])); } - return new Worker([$transportName => $transport], QueueManager::bus(), $dispatcher); + return new Worker([QueueManager::TRANSPORT_DB => $transport], QueueManager::bus(), $dispatcher); } } diff --git a/lib/MahoCLI/Commands/QueueWork.php b/lib/MahoCLI/Commands/QueueWork.php index 321175ffd4..bbda38ac81 100644 --- a/lib/MahoCLI/Commands/QueueWork.php +++ b/lib/MahoCLI/Commands/QueueWork.php @@ -83,6 +83,14 @@ protected function execute(InputInterface $input, OutputInterface $output): int $output->writeln("Another exclusive queue worker already holds {$lockName}"); return Command::INVALID; } + if ($pool === null && ($running = $this->livePoolWorkers()) !== []) { + // The bare lock only stops the watchdog from spawning more; it + // does not evict workers already running. + $output->writeln(sprintf( + 'Pool worker(s) %s are still running and keep consuming until their own limits stop them', + implode(', ', $running), + )); + } } // Unbounded unless asked: a hand-run worker keeps the limits it had before pools existed. @@ -110,7 +118,7 @@ protected function execute(InputInterface $input, OutputInterface $output): int $memoryLimit = null; if ($effective->memoryLimit !== '') { - $memoryLimit = $this->parseMemoryLimit($effective->memoryLimit); + $memoryLimit = Pool::parseMemoryLimit($effective->memoryLimit); if ($memoryLimit === null) { $output->writeln("Invalid memory limit: {$effective->memoryLimit}"); return Command::INVALID; @@ -195,17 +203,21 @@ public function handleSignal(int $signal, int|false $previousExitCode = 0): int| return false; } - private function parseMemoryLimit(string $limit): ?int + /** + * @return list + */ + private function livePoolWorkers(): array { - if (!preg_match('/^(\d+)([KMG]?)$/i', trim($limit), $matches)) { - return null; + $lock = \Mage::getSingleton('core/lock'); + $running = []; + foreach (PoolRegistry::all() as $pool) { + for ($index = 0; $index < $pool->count; $index++) { + if ($lock->isHeld($pool->lockName($index), machineLocal: true)) { + $running[] = "{$pool->name}.{$index}"; + } + } } - return (int) $matches[1] * match (strtoupper($matches[2])) { - 'K' => 1024, - 'M' => 1024 ** 2, - 'G' => 1024 ** 3, - default => 1, - }; + return $running; } } diff --git a/tests/Backend/Integration/Queue/CronConsumerTest.php b/tests/Backend/Integration/Queue/CronConsumerTest.php index 05b0aebf45..1ef9a653c0 100644 --- a/tests/Backend/Integration/Queue/CronConsumerTest.php +++ b/tests/Backend/Integration/Queue/CronConsumerTest.php @@ -100,53 +100,45 @@ function withAllPoolLocks(callable $body): void }); it('counts the on-demand workers already alive against the due budget', function () { - $node = Mage::getConfig()->getNode('global/queue'); - $node->extend(new Maho\Simplexml\Element('3'), true); - QueueManager::reset(); - - $lock = Mage::getSingleton('core/lock'); - $slow = PoolRegistry::get('slow'); - expect($slow?->count)->toBe(3); - $held = $slow->lockName(0); - expect($lock->acquire($held, machineLocal: true))->toBeTrue(); - - try { - QueueManager::dispatch(makeEmailMessage('due now'), queue: 'newsletter'); - - // One message, one worker already consuming it: a second process would - // boot only to idle straight back out. - expect(pendingWorkers())->toBe(['fast:0']); - } finally { - $lock->release($held); - unset($node->pools->slow->count); - QueueManager::reset(); - } + withQueueConfig('3', function () { + $lock = Mage::getSingleton('core/lock'); + $slow = PoolRegistry::get('slow'); + expect($slow?->count)->toBe(3); + $held = $slow->lockName(0); + expect($lock->acquire($held, machineLocal: true))->toBeTrue(); + + try { + QueueManager::dispatch(makeEmailMessage('due now'), queue: 'newsletter'); + + // One message, one worker already consuming it: a second process would + // boot only to idle straight back out. + expect(pendingWorkers())->toBe(['fast:0']); + } finally { + $lock->release($held); + } + }); }); it('does not count a worker inside a long handler as free to take the next message', function () { - $node = Mage::getConfig()->getNode('global/queue'); - $node->extend(new Maho\Simplexml\Element('3'), true); - QueueManager::reset(); - - $lock = Mage::getSingleton('core/lock'); - $slow = PoolRegistry::get('slow'); - expect($slow?->count)->toBe(3); - $held = $slow->lockName(0); - expect($lock->acquire($held, machineLocal: true))->toBeTrue(); - - try { - QueueManager::dispatch(makeEmailMessage('long feed build'), queue: 'feed'); - QueueManager::dispatch(makeEmailMessage('due now'), queue: 'newsletter'); - - // The live worker is busy on the feed build, so it cannot take the - // newsletter: a free slot must be started rather than left idle behind it. - expect([...QueueManager::workerTransport($slow)->get()])->toHaveCount(1); - expect(pendingWorkers())->toBe(['fast:0', 'slow:1']); - } finally { - $lock->release($held); - unset($node->pools->slow->count); - QueueManager::reset(); - } + withQueueConfig('3', function () { + $lock = Mage::getSingleton('core/lock'); + $slow = PoolRegistry::get('slow'); + expect($slow?->count)->toBe(3); + $held = $slow->lockName(0); + expect($lock->acquire($held, machineLocal: true))->toBeTrue(); + + try { + QueueManager::dispatch(makeEmailMessage('long feed build'), queue: 'feed'); + QueueManager::dispatch(makeEmailMessage('due now'), queue: 'newsletter'); + + // The live worker is busy on the feed build, so it cannot take the + // newsletter: a free slot must be started rather than left idle behind it. + expect([...QueueManager::workerTransport($slow)->get()])->toHaveCount(1); + expect(pendingWorkers())->toBe(['fast:0', 'slow:1']); + } finally { + $lock->release($held); + } + }); }); it('removes old failed messages during cleanup', function () { @@ -157,7 +149,7 @@ function withAllPoolLocks(callable $body): void 'message_class' => Mage_Core_Model_Email_SendMessage::class, 'body' => serialize(makeEmailMessage()), 'available_at' => $now, - 'processed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 40 * 86400), + 'processed_at' => agoUtc(40 * 86400), 'created_at' => $now, 'updated_at' => $now, ]); diff --git a/tests/Backend/Integration/Queue/DbTransportTest.php b/tests/Backend/Integration/Queue/DbTransportTest.php index b35e2b8296..857bdfd966 100644 --- a/tests/Backend/Integration/Queue/DbTransportTest.php +++ b/tests/Backend/Integration/Queue/DbTransportTest.php @@ -108,7 +108,7 @@ // A claim nobody will finish must stop suppressing later sends, or every // future dispatch of this key is silently dropped instead of one being parked. queueAdapter()->update(QueueManager::tableName(), [ - 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200), + 'claimed_at' => agoUtc(7200), ]); QueueManager::dispatch(makeEmailMessage(), dedupeKey: 'abc'); expect(fetchQueueRows())->toHaveCount(2); @@ -139,19 +139,20 @@ expect(QueueManager::retryStoredMessage($id))->toBeFalse(); // A live worker is still inside this one: re-queueing it runs the handler twice. - $envelopes = [...$transport->get()]; + expect([...$transport->get()])->toHaveCount(1); expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); expect(QueueManager::retryStoredMessage($id))->toBeFalse(); // Old enough to belong to a dead worker: nothing else requeues it, so the grid must. queueAdapter()->update(QueueManager::tableName(), [ - 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200), + 'claimed_at' => agoUtc(7200), ]); expect(QueueManager::retryStoredMessage($id))->toBeTrue(); expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PENDING); - [...$transport->get()]; - $transport->reject($envelopes[0]); + // The row's owner (its re-claimer, not the stale envelope) can still fail it. + $reclaimed = [...$transport->get()]; + $transport->reject($reclaimed[0]); expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_FAILED); expect(QueueManager::retryStoredMessage($id))->toBeTrue(); expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PENDING); @@ -163,7 +164,7 @@ expect([...$transport->get()])->toHaveCount(1); queueAdapter()->update(QueueManager::tableName(), [ - 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200), + 'claimed_at' => agoUtc(7200), ]); // A claim is parked for an operator, never redelivered on a timer: running @@ -203,7 +204,7 @@ expect($envelopes)->toHaveCount(1); queueAdapter()->update(QueueManager::tableName(), [ - 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - DbTransport::ABANDONED_AFTER_SECONDS - 60), + 'claimed_at' => agoUtc(DbTransport::ABANDONED_AFTER_SECONDS + 60), ], ['message_id = ?' => (int) fetchQueueRows()[0]['message_id']]); expect($transport->countAbandoned())->toBe(1); @@ -239,7 +240,7 @@ // The claim goes stale (the worker looks dead), so the operator re-queues it. queueAdapter()->update(QueueManager::tableName(), [ - 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200), + 'claimed_at' => agoUtc(7200), ], ['message_id = ?' => $id]); expect(QueueManager::retryStoredMessage($id))->toBeTrue(); @@ -252,6 +253,59 @@ expect($rows[0]['status'])->toBe(DbTransport::STATUS_PENDING); }); +it('does not let a stale worker touch a row another worker has since claimed', function () { + QueueManager::dispatch(makeEmailMessage()); + + $transport = QueueManager::dbTransport(); + $staleEnvelopes = [...$transport->get()]; + $id = (int) fetchQueueRows()[0]['message_id']; + + // The claim goes stale (worker A looks dead), the operator re-queues it, + // and worker B claims the row again. + queueAdapter()->update(QueueManager::tableName(), ['claimed_at' => agoUtc(7200)], ['message_id = ?' => $id]); + expect(QueueManager::retryStoredMessage($id))->toBeTrue(); + $freshEnvelopes = [...$transport->get()]; + expect($freshEnvelopes)->toHaveCount(1); + + // Worker A was alive after all: its claim token no longer matches, so its + // late ack, reject and re-send must all leave B's claim alone. + $transport->ack($staleEnvelopes[0]); + expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); + $transport->reject($staleEnvelopes[0]); + expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); + $transport->send($staleEnvelopes[0]->with( + new RedeliveryStamp(1), + ErrorDetailsStamp::create(new RuntimeException('handler blew up')), + )); + expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PROCESSING); + + // B still owns the row and its ack lands. + $transport->ack($freshEnvelopes[0]); + expect(fetchQueueRows())->toHaveCount(0); +}); + +it('refuses to retry a parked claim while a newer copy of its dedupe key is in flight', function () { + QueueManager::dispatch(makeEmailMessage(), dedupeKey: 'abc'); + $transport = QueueManager::dbTransport(); + expect([...$transport->get()])->toHaveCount(1); + $id = (int) fetchQueueRows()[0]['message_id']; + + // The claim is abandoned, so a later dispatch of the same key inserts a + // fresh copy that supersedes the parked one. + queueAdapter()->update(QueueManager::tableName(), ['claimed_at' => agoUtc(7200)], ['message_id = ?' => $id]); + QueueManager::dispatch(makeEmailMessage(), dedupeKey: 'abc'); + expect(fetchQueueRows())->toHaveCount(2); + + // Retrying the parked claim would run the deduped job twice; discard is the way out. + expect(QueueManager::retryStoredMessage($id))->toBeFalse(); + + // Once the fresh copy is gone, the parked claim is retryable again. + $freshId = (int) fetchQueueRows()[1]['message_id']; + expect(QueueManager::discardStoredMessage($freshId))->toBeTrue(); + expect(QueueManager::retryStoredMessage($id))->toBeTrue(); + expect(fetchQueueRows()[0]['status'])->toBe(DbTransport::STATUS_PENDING); +}); + it('does not let a late retry re-send overwrite a row an operator already retried', function () { QueueManager::dispatch(makeEmailMessage()); @@ -261,7 +315,7 @@ // The claim goes stale (the worker looks dead), so the operator re-queues it. queueAdapter()->update(QueueManager::tableName(), [ - 'claimed_at' => gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200), + 'claimed_at' => agoUtc(7200), ], ['message_id = ?' => $id]); expect(QueueManager::retryStoredMessage($id))->toBeTrue(); @@ -285,7 +339,7 @@ $envelopes = [...$transport->get()]; $id = (int) fetchQueueRows()[0]['message_id']; - $stale = gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - 7200); + $stale = agoUtc(7200); queueAdapter()->update(QueueManager::tableName(), ['claimed_at' => $stale], ['message_id = ?' => $id]); queueAdapter()->beginTransaction(); diff --git a/tests/Backend/Integration/Queue/PoolTest.php b/tests/Backend/Integration/Queue/PoolTest.php index f62c92a267..43ca30e185 100644 --- a/tests/Backend/Integration/Queue/PoolTest.php +++ b/tests/Backend/Integration/Queue/PoolTest.php @@ -30,30 +30,6 @@ function poolTransport(string $name): DbTransport return $transport; } -/** - * Merge extra queue config the way another module's config.xml would, run the - * assertions, then take it back out again. - * - * @param callable():void $body - */ -function withQueueConfig(string $xml, callable $body): void -{ - $node = Mage::getConfig()->getNode('global/queue'); - $node->extend(new Maho\Simplexml\Element($xml), true); - QueueManager::reset(); - - try { - $body(); - } finally { - foreach (new Maho\Simplexml\Element($xml) as $section => $children) { - foreach (array_keys((array) $children->children()) as $name) { - unset($node->{$section}->{$name}); - } - } - QueueManager::reset(); - } -} - function insertQueueRow(string $queue, string $status, ?string $claimedAt = null): void { $now = Mage_Core_Model_Locale::nowUtc(); @@ -69,20 +45,19 @@ function insertQueueRow(string $queue, string $status, ?string $claimedAt = null ]); } -function agoUtc(int $seconds): string -{ - return gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - $seconds); -} - /** * A Worker that only records stop(), so the idle listener can be driven - * directly without a transport behind it. + * directly without a transport behind it. The parent constructor runs with + * empty receivers so inherited typed properties stay initialized. */ class RecordingWorker extends Worker { public int $stopped = 0; - public function __construct() {} + public function __construct() + { + parent::__construct([], QueueManager::bus(), new Symfony\Component\EventDispatcher\EventDispatcher()); + } #[\Override] public function stop(): void diff --git a/tests/Pest.php b/tests/Pest.php index a995b42933..1af950f439 100644 --- a/tests/Pest.php +++ b/tests/Pest.php @@ -471,3 +471,33 @@ function fetchQueueRows(): array queueAdapter()->select()->from(\Maho\Queue\QueueManager::tableName())->order('message_id ASC'), ); } + +function agoUtc(int $seconds): string +{ + return gmdate(Mage_Core_Model_Locale::DATETIME_FORMAT, time() - $seconds); +} + +/** + * Merge extra queue config the way another module's config.xml would, run the + * assertions, then restore the original config exactly (a snapshot, so merging + * into an existing node does not delete it on the way out). + * + * @param callable():void $body + */ +function withQueueConfig(string $xml, callable $body): void +{ + $node = Mage::getConfig()->getNode('global/queue'); + $snapshot = new Maho\Simplexml\Element($node->asXML()); + $node->extend(new Maho\Simplexml\Element($xml), true); + \Maho\Queue\QueueManager::reset(); + + try { + $body(); + } finally { + foreach (array_keys((array) $node->children()) as $child) { + unset($node->{$child}); + } + $node->extend($snapshot, true); + \Maho\Queue\QueueManager::reset(); + } +}