From 68d68daeafc0982d47f4cfcd7da4cb78a807a934 Mon Sep 17 00:00:00 2001 From: roboshyim Date: Thu, 23 Jul 2026 19:28:26 +0000 Subject: [PATCH 1/6] feat: configurable queue monitoring for QueueChecker (#269) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Failed messenger transports (async_failed, …) were treated like live queues and caused false positives in System Status / frosh:monitor. - Exclude queue names containing "failed" by default (toggleable) - Optional allowlist of queues to monitor - Optional per-queue grace times (queue:minutes) - Fix age calculation (was effectively ~2× the configured grace) - Surface oldest queue name in the status current value - Unit tests (9) covering filters, grace overrides, and age math --- .../Checker/HealthChecker/QueueChecker.php | 136 +++++++++++-- .../frosh-tools-tab-index/recommendations.js | 6 +- src/Resources/config/config.xml | 35 +++- .../HealthChecker/QueueCheckerTest.php | 189 +++++++++++++++--- 4 files changed, 319 insertions(+), 47 deletions(-) diff --git a/src/Components/Health/Checker/HealthChecker/QueueChecker.php b/src/Components/Health/Checker/HealthChecker/QueueChecker.php index 11d3af27..5d69e89a 100644 --- a/src/Components/Health/Checker/HealthChecker/QueueChecker.php +++ b/src/Components/Health/Checker/HealthChecker/QueueChecker.php @@ -12,6 +12,12 @@ class QueueChecker implements HealthCheckerInterface, CheckerInterface { + private const CONFIG_GRACE = 'FroshTools.config.monitorQueueGraceTime'; + private const CONFIG_EXCLUDE_FAILED = 'FroshTools.config.monitorExcludeFailedQueues'; + private const CONFIG_QUEUES = 'FroshTools.config.monitorQueues'; + private const CONFIG_GRACE_TIMES = 'FroshTools.config.monitorQueueGraceTimes'; + private const DEFAULT_GRACE_MINUTES = 15; + public function __construct( private readonly Connection $connection, private readonly SystemConfigService $configService, @@ -20,29 +26,127 @@ public function __construct( public function collect(HealthCollection $collection): void { - $maxDiff = $this->configService->getInt('FroshTools.config.monitorQueueGraceTime') ?: 15; - $oldMessageLimit = (new \DateTimeImmutable())->modify(\sprintf('-%d minutes', $maxDiff)); + $defaultGrace = $this->configService->getInt(self::CONFIG_GRACE) ?: self::DEFAULT_GRACE_MINUTES; + $excludeFailed = $this->shouldExcludeFailedQueues(); + $queues = $this->parseCsvList($this->configService->getString(self::CONFIG_QUEUES)); + $graceByQueue = $this->parseGraceMap($this->configService->getString(self::CONFIG_GRACE_TIMES)); $snippet = 'Open Queues'; - $recommended = \sprintf('max %d mins', $maxDiff); - - /** @var string|false $oldestMessageAt */ - $oldestMessageAt = $this->connection->fetchOne('SELECT available_at FROM messenger_messages WHERE available_at < UTC_TIMESTAMP() ORDER BY available_at ASC LIMIT 1'); + $row = $this->fetchOldestPendingMessage($excludeFailed, $queues); - if (\is_string($oldestMessageAt)) { - $diff = round(abs( - ((new \DateTime($oldestMessageAt . ' UTC'))->getTimestamp() - $oldMessageLimit->getTimestamp()) / 60, + if ($row === null) { + $collection->add(SettingsResult::info( + 'queue', + $snippet, + '0 mins', + \sprintf('max %d mins', $defaultGrace), )); - if ($diff > $maxDiff) { - $result = SettingsResult::warning('queue', $snippet, $diff . ' mins', $recommended); - } else { - $result = SettingsResult::ok('queue', $snippet, $diff . ' mins', $recommended); + return; + } + + $queueName = (string) $row['queue_name']; + $grace = $graceByQueue[$queueName] ?? $defaultGrace; + $recommended = \sprintf('max %d mins', $grace); + $ageMinutes = $this->ageInMinutes((string) $row['available_at']); + $current = \sprintf('%d mins (%s)', $ageMinutes, $queueName); + + if ($ageMinutes > $grace) { + $collection->add(SettingsResult::warning('queue', $snippet, $current, $recommended)); + + return; + } + + $collection->add(SettingsResult::ok('queue', $snippet, $current, $recommended)); + } + + /** + * @param list $queues + * + * @return array{available_at: string, queue_name: string}|null + */ + private function fetchOldestPendingMessage(bool $excludeFailed, array $queues): ?array + { + $sql = 'SELECT available_at, queue_name FROM messenger_messages WHERE available_at <= UTC_TIMESTAMP()'; + $params = []; + + if ($excludeFailed) { + // Symfony failure transport names typically contain "failed" (e.g. async_failed). + $sql .= ' AND queue_name NOT LIKE ?'; + $params[] = '%failed%'; + } + + if ($queues !== []) { + $placeholders = \implode(', ', \array_fill(0, \count($queues), '?')); + $sql .= \sprintf(' AND queue_name IN (%s)', $placeholders); + foreach ($queues as $queue) { + $params[] = $queue; + } + } + + $sql .= ' ORDER BY available_at ASC LIMIT 1'; + + /** @var array{available_at: string, queue_name: string}|false $row */ + $row = $this->connection->fetchAssociative($sql, $params); + + return \is_array($row) ? $row : null; + } + + private function ageInMinutes(string $availableAt): int + { + $available = new \DateTimeImmutable($availableAt, new \DateTimeZone('UTC')); + $now = new \DateTimeImmutable('now', new \DateTimeZone('UTC')); + $seconds = \max(0, $now->getTimestamp() - $available->getTimestamp()); + + return (int) \floor($seconds / 60); + } + + private function shouldExcludeFailedQueues(): bool + { + $value = $this->configService->get(self::CONFIG_EXCLUDE_FAILED); + + // Default true when the setting has never been stored. + return $value === null ? true : (bool) $value; + } + + /** + * @return list + */ + private function parseCsvList(string $value): array + { + if (\trim($value) === '') { + return []; + } + + $parts = \array_map(\trim(...), \explode(',', $value)); + + return \array_values(\array_filter($parts, static fn (string $part): bool => $part !== '')); + } + + /** + * Parses "async:15, low_priority:60" into queue => grace minutes. + * + * @return array + */ + private function parseGraceMap(string $value): array + { + $map = []; + foreach ($this->parseCsvList($value) as $entry) { + if (!\str_contains($entry, ':')) { + continue; + } + + [$name, $minutes] = \array_map(\trim(...), \explode(':', $entry, 2)); + if ($name === '' || !\is_numeric($minutes)) { + continue; + } + + $grace = (int) $minutes; + if ($grace > 0) { + $map[$name] = $grace; } - } else { - $result = SettingsResult::info('queue', $snippet, 'unknown', $recommended); } - $collection->add($result); + return $map; } } diff --git a/src/Resources/app/administration/src/module/frosh-tools/component/frosh-tools-tab-index/recommendations.js b/src/Resources/app/administration/src/module/frosh-tools/component/frosh-tools-tab-index/recommendations.js index 9edd6fa8..af1d4749 100644 --- a/src/Resources/app/administration/src/module/frosh-tools/component/frosh-tools-tab-index/recommendations.js +++ b/src/Resources/app/administration/src/module/frosh-tools/component/frosh-tools-tab-index/recommendations.js @@ -21,10 +21,10 @@ export default { }, queue: { description: - 'Messages have been waiting in the message queue longer than the configured grace time. That almost always means no worker is consuming the queue, so asynchronous tasks (mails, indexing, …) pile up.', + 'Messages have been waiting in a monitored messenger queue longer than the configured grace time. That almost always means no worker is consuming that queue, so asynchronous tasks (mails, indexing, …) pile up. Failed queues (names containing "failed") are ignored by default because they are usually drained manually.', solution: - 'Make sure a CLI worker is running and supervised (e.g. via systemd or Supervisor) so the queue is drained continuously.', - code: 'bin/console messenger:consume async low_priority failed', + 'Make sure a CLI worker is running and supervised (e.g. via systemd or Supervisor) so the queue is drained continuously. Adjust monitored queues and per-queue grace times under Settings → Extensions → Frosh Tools if needed.', + code: 'bin/console messenger:consume async low_priority\n# Extension config examples:\n# monitorQueues = async, low_priority\n# monitorQueueGraceTimes = async:15, low_priority:60\n# monitorExcludeFailedQueues = true', }, 'mysql-timezone': { description: diff --git a/src/Resources/config/config.xml b/src/Resources/config/config.xml index 2fcffb39..771376e5 100644 --- a/src/Resources/config/config.xml +++ b/src/Resources/config/config.xml @@ -16,13 +16,40 @@ monitorQueueGraceTime - - - After X minutes a queue is considered stuck / faulty - Nach X Minuten gilt eine Queue als festgefahren / fehlerhaft + + + After this many minutes a pending message is considered stuck. Used when no per-queue override matches. + Nach so vielen Minuten gilt eine ausstehende Nachricht als festgefahren. Gilt, wenn kein Queue-Override greift. 15 + + monitorExcludeFailedQueues + + + Ignore messenger queues whose name contains "failed" (e.g. async_failed). Those are usually drained manually and would otherwise cause false positives. + Ignoriert Messenger-Queues, deren Name "failed" enthält (z. B. async_failed). Diese werden meist manuell abgearbeitet und würden sonst Fehlalarme auslösen. + true + + + + monitorQueues + + + Optional comma-separated allowlist (e.g. async, low_priority). Leave empty to monitor all queues (except failed ones when exclusion is enabled). + Optionale kommaseparierte Allowlist (z. B. async, low_priority). Leer lassen, um alle Queues zu überwachen (außer Failed-Queues, wenn der Ausschluss aktiv ist). + async, low_priority + + + + monitorQueueGraceTimes + + + Optional overrides as queue:minutes pairs (e.g. async:15, low_priority:60). Queues not listed use the default grace time. + Optionale Overrides als Queue:Minuten-Paare (z. B. async:15, low_priority:60). Nicht gelistete Queues nutzen die Standard-Karenzzeit. + async:15, low_priority:60 + + monitorTaskGraceTime diff --git a/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php b/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php index be80ce9c..5a61ffad 100644 --- a/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php +++ b/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php @@ -8,61 +8,202 @@ use Frosh\Tools\Components\Health\Checker\HealthChecker\QueueChecker; use Frosh\Tools\Components\Health\HealthCollection; use Frosh\Tools\Components\Health\SettingsResult; -use Frosh\Tools\Tests\IntegrationTestCase; use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\TestCase; +use Shopware\Core\System\SystemConfig\SystemConfigService; #[CoversClass(QueueChecker::class)] -class QueueCheckerTest extends IntegrationTestCase +class QueueCheckerTest extends TestCase { - private QueueChecker $checker; + public function testEmptyQueueResultsInInfoState(): void + { + $result = $this->collect( + connectionRows: false, + config: [], + ); - private Connection $connection; + static::assertSame(SettingsResult::INFO, $result->state); + static::assertSame('0 mins', $result->current); + } - protected function setUp(): void + public function testOldMessageResultsInWarningState(): void { - $this->checker = static::getContainer()->get(QueueChecker::class); - $this->connection = static::getContainer()->get(Connection::class); + $result = $this->collect( + connectionRows: [ + 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], + config: ['FroshTools.config.monitorQueueGraceTime' => 15], + ); - $this->connection->executeStatement('DELETE FROM messenger_messages'); + static::assertSame(SettingsResult::WARNING, $result->state); + static::assertStringContainsString('async', $result->current); } - public function testEmptyQueueResultsInInfoState(): void + public function testRecentMessageWithinGracePeriodResultsInOkState(): void { - $result = $this->collectQueueResult(); + $result = $this->collect( + connectionRows: [ + 'available_at' => (new \DateTimeImmutable('-1 minute', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], + config: ['FroshTools.config.monitorQueueGraceTime' => 15], + ); - static::assertSame(SettingsResult::INFO, $result->state); + static::assertSame(SettingsResult::GREEN, $result->state); } - public function testOldMessageResultsInWarningState(): void + public function testMessageJustOverGraceIsWarningNotDoubleGrace(): void { - $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 2 HOUR'); + // Regression: previous formula effectively required age > 2 * grace. + $result = $this->collect( + connectionRows: [ + 'available_at' => (new \DateTimeImmutable('-20 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], + config: ['FroshTools.config.monitorQueueGraceTime' => 15], + ); + + static::assertSame(SettingsResult::WARNING, $result->state); + } - $result = $this->collectQueueResult(); + public function testFailedQueuesAreExcludedByDefault(): void + { + $connection = $this->createMock(Connection::class); + $connection->expects(static::once()) + ->method('fetchAssociative') + ->with( + static::callback(static function (string $sql): bool { + return \str_contains($sql, 'NOT LIKE') && \str_contains($sql, 'queue_name'); + }), + static::callback(static function (array $params): bool { + return $params === ['%failed%']; + }), + ) + ->willReturn(false); + + $result = $this->collectWith(connection: $connection, config: []); + + static::assertSame(SettingsResult::INFO, $result->state); + } + + public function testFailedQueuesCanBeIncluded(): void + { + $connection = $this->createMock(Connection::class); + $connection->expects(static::once()) + ->method('fetchAssociative') + ->with( + static::callback(static function (string $sql): bool { + return !\str_contains($sql, 'NOT LIKE'); + }), + static::equalTo([]), + ) + ->willReturn([ + 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async_failed', + ]); + + $result = $this->collectWith( + connection: $connection, + config: ['FroshTools.config.monitorExcludeFailedQueues' => false], + ); static::assertSame(SettingsResult::WARNING, $result->state); + static::assertStringContainsString('async_failed', $result->current); } - public function testRecentMessageWithinGracePeriodResultsInOkState(): void + public function testAllowlistRestrictsMonitoredQueues(): void { - $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 1 MINUTE'); + $connection = $this->createMock(Connection::class); + $connection->expects(static::once()) + ->method('fetchAssociative') + ->with( + static::callback(static function (string $sql): bool { + return \str_contains($sql, 'IN ('); + }), + static::equalTo(['%failed%', 'async', 'low_priority']), + ) + ->willReturn([ + 'available_at' => (new \DateTimeImmutable('-5 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ]); + + $result = $this->collectWith( + connection: $connection, + config: [ + 'FroshTools.config.monitorQueues' => 'async, low_priority', + 'FroshTools.config.monitorQueueGraceTime' => 15, + ], + ); - $result = $this->collectQueueResult(); + static::assertSame(SettingsResult::GREEN, $result->state); + } + public function testPerQueueGraceTimeOverridesDefault(): void + { + $result = $this->collect( + connectionRows: [ + 'available_at' => (new \DateTimeImmutable('-30 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'low_priority', + ], + config: [ + 'FroshTools.config.monitorQueueGraceTime' => 15, + 'FroshTools.config.monitorQueueGraceTimes' => 'low_priority:60, async:10', + ], + ); + + // 30 mins old with 60 min grace for low_priority → OK static::assertSame(SettingsResult::GREEN, $result->state); + static::assertSame('max 60 mins', $result->recommended); } - private function insertMessage(string $availableAt): void + public function testPerQueueGraceTimeCanTightenDefault(): void { - $this->connection->executeStatement(\sprintf( - "INSERT INTO messenger_messages (body, headers, queue_name, created_at, available_at) VALUES ('a:0:{}', '[]', 'default', UTC_TIMESTAMP(), %s)", - $availableAt, - )); + $result = $this->collect( + connectionRows: [ + 'available_at' => (new \DateTimeImmutable('-12 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], + config: [ + 'FroshTools.config.monitorQueueGraceTime' => 15, + 'FroshTools.config.monitorQueueGraceTimes' => 'async:10', + ], + ); + + static::assertSame(SettingsResult::WARNING, $result->state); + static::assertSame('max 10 mins', $result->recommended); } - private function collectQueueResult(): SettingsResult + /** + * @param array{available_at: string, queue_name: string}|false $connectionRows + * @param array $config + */ + private function collect(array|false $connectionRows, array $config): SettingsResult { + $connection = $this->createMock(Connection::class); + $connection->method('fetchAssociative')->willReturn($connectionRows); + + return $this->collectWith($connection, $config); + } + + /** + * @param array $config + */ + private function collectWith(Connection $connection, array $config): SettingsResult + { + $configService = $this->createMock(SystemConfigService::class); + $configService->method('getInt')->willReturnCallback( + static fn (string $key): int => (int) ($config[$key] ?? 0), + ); + $configService->method('getString')->willReturnCallback( + static fn (string $key): string => (string) ($config[$key] ?? ''), + ); + $configService->method('get')->willReturnCallback( + static fn (string $key): mixed => $config[$key] ?? null, + ); + $collection = new HealthCollection(); - $this->checker->collect($collection); + (new QueueChecker($connection, $configService))->collect($collection); foreach ($collection->getElements() as $element) { if ($element->id === 'queue') { From 2ff30ab3fbd311e3f34bafdacf59c6f31a3c0ce4 Mon Sep 17 00:00:00 2001 From: roboshyim Date: Thu, 23 Jul 2026 19:28:40 +0000 Subject: [PATCH 2/6] docs: document queue monitoring config options --- README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 3f209666..7faf6153 100644 --- a/README.md +++ b/README.md @@ -206,7 +206,10 @@ frosh_tools: Plugin system config (Admin → Extensions → Frosh Tools): - **Monitor mail address** — recipient for `frosh:monitor` -- **Queue grace time** (minutes) — when a queue is considered stuck +- **Default queue grace time** (minutes) — when a pending message is considered stuck +- **Exclude failed queues** — ignore transports whose name contains `failed` (default on) +- **Queues to monitor** — optional allowlist, e.g. `async, low_priority` +- **Per-queue grace times** — optional overrides, e.g. `async:15, low_priority:60` - **Task grace time** (minutes) — when a scheduled task is considered stuck JSON Schema for IDE validation: [`frosh-tools-schema.json`](frosh-tools-schema.json). From f1e514a9d2850aaf50520c9e60206739c36ade2c Mon Sep 17 00:00:00 2001 From: roboshyim Date: Thu, 23 Jul 2026 19:31:39 +0000 Subject: [PATCH 3/6] refactor: use Doctrine QueryBuilder in QueueChecker Replace hand-built SQL with Connection::createQueryBuilder(), matching TaskChecker and keeping named parameters / ArrayParameterType for the queue allowlist. --- .../Checker/HealthChecker/QueueChecker.php | 26 ++-- .../HealthChecker/QueueCheckerTest.php | 127 +++++++++++------- 2 files changed, 95 insertions(+), 58 deletions(-) diff --git a/src/Components/Health/Checker/HealthChecker/QueueChecker.php b/src/Components/Health/Checker/HealthChecker/QueueChecker.php index 5d69e89a..667f4d16 100644 --- a/src/Components/Health/Checker/HealthChecker/QueueChecker.php +++ b/src/Components/Health/Checker/HealthChecker/QueueChecker.php @@ -4,6 +4,7 @@ namespace Frosh\Tools\Components\Health\Checker\HealthChecker; +use Doctrine\DBAL\ArrayParameterType; use Doctrine\DBAL\Connection; use Frosh\Tools\Components\Health\Checker\CheckerInterface; use Frosh\Tools\Components\Health\HealthCollection; @@ -67,27 +68,28 @@ public function collect(HealthCollection $collection): void */ private function fetchOldestPendingMessage(bool $excludeFailed, array $queues): ?array { - $sql = 'SELECT available_at, queue_name FROM messenger_messages WHERE available_at <= UTC_TIMESTAMP()'; - $params = []; + $query = $this->connection->createQueryBuilder() + ->select('available_at', 'queue_name') + ->from('messenger_messages') + ->where('available_at <= UTC_TIMESTAMP()') + ->orderBy('available_at', 'ASC') + ->setMaxResults(1); if ($excludeFailed) { // Symfony failure transport names typically contain "failed" (e.g. async_failed). - $sql .= ' AND queue_name NOT LIKE ?'; - $params[] = '%failed%'; + $query + ->andWhere('queue_name NOT LIKE :failedPattern') + ->setParameter('failedPattern', '%failed%'); } if ($queues !== []) { - $placeholders = \implode(', ', \array_fill(0, \count($queues), '?')); - $sql .= \sprintf(' AND queue_name IN (%s)', $placeholders); - foreach ($queues as $queue) { - $params[] = $queue; - } + $query + ->andWhere('queue_name IN (:queues)') + ->setParameter('queues', $queues, ArrayParameterType::STRING); } - $sql .= ' ORDER BY available_at ASC LIMIT 1'; - /** @var array{available_at: string, queue_name: string}|false $row */ - $row = $this->connection->fetchAssociative($sql, $params); + $row = $query->fetchAssociative(); return \is_array($row) ? $row : null; } diff --git a/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php b/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php index 5a61ffad..f5fb1b92 100644 --- a/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php +++ b/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php @@ -4,11 +4,14 @@ namespace Frosh\Tools\Tests\Components\Health\Checker\HealthChecker; +use Doctrine\DBAL\ArrayParameterType; use Doctrine\DBAL\Connection; +use Doctrine\DBAL\Query\QueryBuilder; use Frosh\Tools\Components\Health\Checker\HealthChecker\QueueChecker; use Frosh\Tools\Components\Health\HealthCollection; use Frosh\Tools\Components\Health\SettingsResult; use PHPUnit\Framework\Attributes\CoversClass; +use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shopware\Core\System\SystemConfig\SystemConfigService; @@ -69,42 +72,34 @@ public function testMessageJustOverGraceIsWarningNotDoubleGrace(): void public function testFailedQueuesAreExcludedByDefault(): void { - $connection = $this->createMock(Connection::class); - $connection->expects(static::once()) - ->method('fetchAssociative') - ->with( - static::callback(static function (string $sql): bool { - return \str_contains($sql, 'NOT LIKE') && \str_contains($sql, 'queue_name'); - }), - static::callback(static function (array $params): bool { - return $params === ['%failed%']; - }), - ) - ->willReturn(false); - - $result = $this->collectWith(connection: $connection, config: []); + $query = $this->createQueryBuilderMock(false); + $query->expects(static::once()) + ->method('andWhere') + ->with('queue_name NOT LIKE :failedPattern') + ->willReturnSelf(); + $query->expects(static::once()) + ->method('setParameter') + ->with('failedPattern', '%failed%') + ->willReturnSelf(); + + $result = $this->collectWith( + connection: $this->connectionReturning($query), + config: [], + ); static::assertSame(SettingsResult::INFO, $result->state); } public function testFailedQueuesCanBeIncluded(): void { - $connection = $this->createMock(Connection::class); - $connection->expects(static::once()) - ->method('fetchAssociative') - ->with( - static::callback(static function (string $sql): bool { - return !\str_contains($sql, 'NOT LIKE'); - }), - static::equalTo([]), - ) - ->willReturn([ - 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async_failed', - ]); + $query = $this->createQueryBuilderMock([ + 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async_failed', + ]); + $query->expects(static::never())->method('andWhere'); $result = $this->collectWith( - connection: $connection, + connection: $this->connectionReturning($query), config: ['FroshTools.config.monitorExcludeFailedQueues' => false], ); @@ -114,28 +109,42 @@ public function testFailedQueuesCanBeIncluded(): void public function testAllowlistRestrictsMonitoredQueues(): void { - $connection = $this->createMock(Connection::class); - $connection->expects(static::once()) - ->method('fetchAssociative') - ->with( - static::callback(static function (string $sql): bool { - return \str_contains($sql, 'IN ('); - }), - static::equalTo(['%failed%', 'async', 'low_priority']), - ) - ->willReturn([ - 'available_at' => (new \DateTimeImmutable('-5 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', - ]); + $query = $this->createQueryBuilderMock([ + 'available_at' => (new \DateTimeImmutable('-5 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ]); + + $andWhere = []; + $query->expects(static::exactly(2)) + ->method('andWhere') + ->willReturnCallback(static function (string $predicate) use ($query, &$andWhere): QueryBuilder { + $andWhere[] = $predicate; + + return $query; + }); + + $parameters = []; + $query->expects(static::exactly(2)) + ->method('setParameter') + ->willReturnCallback(static function (string $name, mixed $value, mixed $type = null) use ($query, &$parameters): QueryBuilder { + $parameters[$name] = ['value' => $value, 'type' => $type]; + + return $query; + }); $result = $this->collectWith( - connection: $connection, + connection: $this->connectionReturning($query), config: [ 'FroshTools.config.monitorQueues' => 'async, low_priority', 'FroshTools.config.monitorQueueGraceTime' => 15, ], ); + static::assertContains('queue_name NOT LIKE :failedPattern', $andWhere); + static::assertContains('queue_name IN (:queues)', $andWhere); + static::assertSame('%failed%', $parameters['failedPattern']['value']); + static::assertSame(['async', 'low_priority'], $parameters['queues']['value']); + static::assertSame(ArrayParameterType::STRING, $parameters['queues']['type']); static::assertSame(SettingsResult::GREEN, $result->state); } @@ -180,10 +189,10 @@ public function testPerQueueGraceTimeCanTightenDefault(): void */ private function collect(array|false $connectionRows, array $config): SettingsResult { - $connection = $this->createMock(Connection::class); - $connection->method('fetchAssociative')->willReturn($connectionRows); - - return $this->collectWith($connection, $config); + return $this->collectWith( + connection: $this->connectionReturning($this->createQueryBuilderMock($connectionRows)), + config: $config, + ); } /** @@ -213,4 +222,30 @@ private function collectWith(Connection $connection, array $config): SettingsRes static::fail('HealthCollection does not contain a result with id "queue"'); } + + /** + * @param array{available_at: string, queue_name: string}|false $row + */ + private function createQueryBuilderMock(array|false $row): QueryBuilder&MockObject + { + $query = $this->createMock(QueryBuilder::class); + $query->method('select')->willReturnSelf(); + $query->method('from')->willReturnSelf(); + $query->method('where')->willReturnSelf(); + $query->method('andWhere')->willReturnSelf(); + $query->method('orderBy')->willReturnSelf(); + $query->method('setMaxResults')->willReturnSelf(); + $query->method('setParameter')->willReturnSelf(); + $query->method('fetchAssociative')->willReturn($row); + + return $query; + } + + private function connectionReturning(QueryBuilder $query): Connection&MockObject + { + $connection = $this->createMock(Connection::class); + $connection->method('createQueryBuilder')->willReturn($query); + + return $connection; + } } From 00f8f9f0bb2dc9c0c94546baa1df94ef7da922c5 Mon Sep 17 00:00:00 2001 From: roboshyim Date: Thu, 23 Jul 2026 19:38:26 +0000 Subject: [PATCH 4/6] test: restore QueueChecker integration suite alongside unit tests Keep mock-based QueueCheckerUnitTest for config/age edge cases, and put back IntegrationTestCase coverage (real messenger_messages + QueryBuilder) including failed-queue exclusion, allowlist, and per-queue grace. --- .../HealthChecker/QueueCheckerTest.php | 245 +++++------------ .../HealthChecker/QueueCheckerUnitTest.php | 255 ++++++++++++++++++ 2 files changed, 327 insertions(+), 173 deletions(-) create mode 100644 tests/Components/Health/Checker/HealthChecker/QueueCheckerUnitTest.php diff --git a/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php b/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php index f5fb1b92..9789f9c8 100644 --- a/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php +++ b/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php @@ -4,26 +4,40 @@ namespace Frosh\Tools\Tests\Components\Health\Checker\HealthChecker; -use Doctrine\DBAL\ArrayParameterType; use Doctrine\DBAL\Connection; -use Doctrine\DBAL\Query\QueryBuilder; use Frosh\Tools\Components\Health\Checker\HealthChecker\QueueChecker; use Frosh\Tools\Components\Health\HealthCollection; use Frosh\Tools\Components\Health\SettingsResult; +use Frosh\Tools\Tests\IntegrationTestCase; use PHPUnit\Framework\Attributes\CoversClass; -use PHPUnit\Framework\MockObject\MockObject; -use PHPUnit\Framework\TestCase; use Shopware\Core\System\SystemConfig\SystemConfigService; +/** + * Kernel + DB coverage for QueueChecker (QueryBuilder against real messenger_messages). + * Fast mock-based edge cases live in QueueCheckerUnitTest. + */ #[CoversClass(QueueChecker::class)] -class QueueCheckerTest extends TestCase +class QueueCheckerTest extends IntegrationTestCase { + private QueueChecker $checker; + + private Connection $connection; + + private SystemConfigService $configService; + + protected function setUp(): void + { + $this->checker = static::getContainer()->get(QueueChecker::class); + $this->connection = static::getContainer()->get(Connection::class); + $this->configService = static::getContainer()->get(SystemConfigService::class); + + $this->connection->executeStatement('DELETE FROM messenger_messages'); + $this->resetMonitorConfig(); + } + public function testEmptyQueueResultsInInfoState(): void { - $result = $this->collect( - connectionRows: false, - config: [], - ); + $result = $this->collectQueueResult(); static::assertSame(SettingsResult::INFO, $result->state); static::assertSame('0 mins', $result->current); @@ -31,13 +45,9 @@ public function testEmptyQueueResultsInInfoState(): void public function testOldMessageResultsInWarningState(): void { - $result = $this->collect( - connectionRows: [ - 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', - ], - config: ['FroshTools.config.monitorQueueGraceTime' => 15], - ); + $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 2 HOUR', 'async'); + + $result = $this->collectQueueResult(); static::assertSame(SettingsResult::WARNING, $result->state); static::assertStringContainsString('async', $result->current); @@ -45,174 +55,81 @@ public function testOldMessageResultsInWarningState(): void public function testRecentMessageWithinGracePeriodResultsInOkState(): void { - $result = $this->collect( - connectionRows: [ - 'available_at' => (new \DateTimeImmutable('-1 minute', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', - ], - config: ['FroshTools.config.monitorQueueGraceTime' => 15], - ); + $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 1 MINUTE', 'async'); + + $result = $this->collectQueueResult(); static::assertSame(SettingsResult::GREEN, $result->state); } - public function testMessageJustOverGraceIsWarningNotDoubleGrace(): void + public function testFailedQueueMessagesAreIgnoredByDefault(): void { - // Regression: previous formula effectively required age > 2 * grace. - $result = $this->collect( - connectionRows: [ - 'available_at' => (new \DateTimeImmutable('-20 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', - ], - config: ['FroshTools.config.monitorQueueGraceTime' => 15], - ); - - static::assertSame(SettingsResult::WARNING, $result->state); - } + $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 2 HOUR', 'async_failed'); - public function testFailedQueuesAreExcludedByDefault(): void - { - $query = $this->createQueryBuilderMock(false); - $query->expects(static::once()) - ->method('andWhere') - ->with('queue_name NOT LIKE :failedPattern') - ->willReturnSelf(); - $query->expects(static::once()) - ->method('setParameter') - ->with('failedPattern', '%failed%') - ->willReturnSelf(); - - $result = $this->collectWith( - connection: $this->connectionReturning($query), - config: [], - ); + $result = $this->collectQueueResult(); static::assertSame(SettingsResult::INFO, $result->state); } - public function testFailedQueuesCanBeIncluded(): void + public function testFailedQueueMessagesAreCountedWhenExclusionDisabled(): void { - $query = $this->createQueryBuilderMock([ - 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async_failed', - ]); - $query->expects(static::never())->method('andWhere'); - - $result = $this->collectWith( - connection: $this->connectionReturning($query), - config: ['FroshTools.config.monitorExcludeFailedQueues' => false], - ); + $this->configService->set('FroshTools.config.monitorExcludeFailedQueues', false); + $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 2 HOUR', 'async_failed'); + + $result = $this->collectQueueResult(); static::assertSame(SettingsResult::WARNING, $result->state); static::assertStringContainsString('async_failed', $result->current); } - public function testAllowlistRestrictsMonitoredQueues(): void + public function testAllowlistIgnoresQueuesOutsideTheList(): void { - $query = $this->createQueryBuilderMock([ - 'available_at' => (new \DateTimeImmutable('-5 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', - ]); - - $andWhere = []; - $query->expects(static::exactly(2)) - ->method('andWhere') - ->willReturnCallback(static function (string $predicate) use ($query, &$andWhere): QueryBuilder { - $andWhere[] = $predicate; - - return $query; - }); - - $parameters = []; - $query->expects(static::exactly(2)) - ->method('setParameter') - ->willReturnCallback(static function (string $name, mixed $value, mixed $type = null) use ($query, &$parameters): QueryBuilder { - $parameters[$name] = ['value' => $value, 'type' => $type]; - - return $query; - }); - - $result = $this->collectWith( - connection: $this->connectionReturning($query), - config: [ - 'FroshTools.config.monitorQueues' => 'async, low_priority', - 'FroshTools.config.monitorQueueGraceTime' => 15, - ], - ); + $this->configService->set('FroshTools.config.monitorQueues', 'async'); + $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 2 HOUR', 'low_priority'); - static::assertContains('queue_name NOT LIKE :failedPattern', $andWhere); - static::assertContains('queue_name IN (:queues)', $andWhere); - static::assertSame('%failed%', $parameters['failedPattern']['value']); - static::assertSame(['async', 'low_priority'], $parameters['queues']['value']); - static::assertSame(ArrayParameterType::STRING, $parameters['queues']['type']); - static::assertSame(SettingsResult::GREEN, $result->state); + $result = $this->collectQueueResult(); + + static::assertSame(SettingsResult::INFO, $result->state); } - public function testPerQueueGraceTimeOverridesDefault(): void + public function testAllowlistMonitorsListedQueue(): void { - $result = $this->collect( - connectionRows: [ - 'available_at' => (new \DateTimeImmutable('-30 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'low_priority', - ], - config: [ - 'FroshTools.config.monitorQueueGraceTime' => 15, - 'FroshTools.config.monitorQueueGraceTimes' => 'low_priority:60, async:10', - ], - ); + $this->configService->set('FroshTools.config.monitorQueues', 'async, low_priority'); + $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 2 HOUR', 'async'); - // 30 mins old with 60 min grace for low_priority → OK - static::assertSame(SettingsResult::GREEN, $result->state); - static::assertSame('max 60 mins', $result->recommended); + $result = $this->collectQueueResult(); + + static::assertSame(SettingsResult::WARNING, $result->state); + static::assertStringContainsString('async', $result->current); } - public function testPerQueueGraceTimeCanTightenDefault(): void + public function testPerQueueGraceTimeIsApplied(): void { - $result = $this->collect( - connectionRows: [ - 'available_at' => (new \DateTimeImmutable('-12 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', - ], - config: [ - 'FroshTools.config.monitorQueueGraceTime' => 15, - 'FroshTools.config.monitorQueueGraceTimes' => 'async:10', - ], - ); + $this->configService->set('FroshTools.config.monitorQueueGraceTime', 15); + $this->configService->set('FroshTools.config.monitorQueueGraceTimes', 'low_priority:120'); + $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 30 MINUTE', 'low_priority'); - static::assertSame(SettingsResult::WARNING, $result->state); - static::assertSame('max 10 mins', $result->recommended); + $result = $this->collectQueueResult(); + + static::assertSame(SettingsResult::GREEN, $result->state); + static::assertSame('max 120 mins', $result->recommended); } - /** - * @param array{available_at: string, queue_name: string}|false $connectionRows - * @param array $config - */ - private function collect(array|false $connectionRows, array $config): SettingsResult + private function insertMessage(string $availableAt, string $queueName = 'default'): void { - return $this->collectWith( - connection: $this->connectionReturning($this->createQueryBuilderMock($connectionRows)), - config: $config, + $this->connection->executeStatement( + \sprintf( + "INSERT INTO messenger_messages (body, headers, queue_name, created_at, available_at) VALUES ('a:0:{}', '[]', %s, UTC_TIMESTAMP(), %s)", + $this->connection->quote($queueName), + $availableAt, + ), ); } - /** - * @param array $config - */ - private function collectWith(Connection $connection, array $config): SettingsResult + private function collectQueueResult(): SettingsResult { - $configService = $this->createMock(SystemConfigService::class); - $configService->method('getInt')->willReturnCallback( - static fn (string $key): int => (int) ($config[$key] ?? 0), - ); - $configService->method('getString')->willReturnCallback( - static fn (string $key): string => (string) ($config[$key] ?? ''), - ); - $configService->method('get')->willReturnCallback( - static fn (string $key): mixed => $config[$key] ?? null, - ); - $collection = new HealthCollection(); - (new QueueChecker($connection, $configService))->collect($collection); + $this->checker->collect($collection); foreach ($collection->getElements() as $element) { if ($element->id === 'queue') { @@ -223,29 +140,11 @@ private function collectWith(Connection $connection, array $config): SettingsRes static::fail('HealthCollection does not contain a result with id "queue"'); } - /** - * @param array{available_at: string, queue_name: string}|false $row - */ - private function createQueryBuilderMock(array|false $row): QueryBuilder&MockObject + private function resetMonitorConfig(): void { - $query = $this->createMock(QueryBuilder::class); - $query->method('select')->willReturnSelf(); - $query->method('from')->willReturnSelf(); - $query->method('where')->willReturnSelf(); - $query->method('andWhere')->willReturnSelf(); - $query->method('orderBy')->willReturnSelf(); - $query->method('setMaxResults')->willReturnSelf(); - $query->method('setParameter')->willReturnSelf(); - $query->method('fetchAssociative')->willReturn($row); - - return $query; - } - - private function connectionReturning(QueryBuilder $query): Connection&MockObject - { - $connection = $this->createMock(Connection::class); - $connection->method('createQueryBuilder')->willReturn($query); - - return $connection; + $this->configService->delete('FroshTools.config.monitorExcludeFailedQueues'); + $this->configService->delete('FroshTools.config.monitorQueues'); + $this->configService->delete('FroshTools.config.monitorQueueGraceTimes'); + $this->configService->set('FroshTools.config.monitorQueueGraceTime', 15); } } diff --git a/tests/Components/Health/Checker/HealthChecker/QueueCheckerUnitTest.php b/tests/Components/Health/Checker/HealthChecker/QueueCheckerUnitTest.php new file mode 100644 index 00000000..2ef959df --- /dev/null +++ b/tests/Components/Health/Checker/HealthChecker/QueueCheckerUnitTest.php @@ -0,0 +1,255 @@ +collect( + connectionRows: false, + config: [], + ); + + static::assertSame(SettingsResult::INFO, $result->state); + static::assertSame('0 mins', $result->current); + } + + public function testOldMessageResultsInWarningState(): void + { + $result = $this->collect( + connectionRows: [ + 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], + config: ['FroshTools.config.monitorQueueGraceTime' => 15], + ); + + static::assertSame(SettingsResult::WARNING, $result->state); + static::assertStringContainsString('async', $result->current); + } + + public function testRecentMessageWithinGracePeriodResultsInOkState(): void + { + $result = $this->collect( + connectionRows: [ + 'available_at' => (new \DateTimeImmutable('-1 minute', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], + config: ['FroshTools.config.monitorQueueGraceTime' => 15], + ); + + static::assertSame(SettingsResult::GREEN, $result->state); + } + + public function testMessageJustOverGraceIsWarningNotDoubleGrace(): void + { + // Regression: previous formula effectively required age > 2 * grace. + $result = $this->collect( + connectionRows: [ + 'available_at' => (new \DateTimeImmutable('-20 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], + config: ['FroshTools.config.monitorQueueGraceTime' => 15], + ); + + static::assertSame(SettingsResult::WARNING, $result->state); + } + + public function testFailedQueuesAreExcludedByDefault(): void + { + $query = $this->createQueryBuilderMock(false); + $query->expects(static::once()) + ->method('andWhere') + ->with('queue_name NOT LIKE :failedPattern') + ->willReturnSelf(); + $query->expects(static::once()) + ->method('setParameter') + ->with('failedPattern', '%failed%') + ->willReturnSelf(); + + $result = $this->collectWith( + connection: $this->connectionReturning($query), + config: [], + ); + + static::assertSame(SettingsResult::INFO, $result->state); + } + + public function testFailedQueuesCanBeIncluded(): void + { + $query = $this->createQueryBuilderMock([ + 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async_failed', + ]); + $query->expects(static::never())->method('andWhere'); + + $result = $this->collectWith( + connection: $this->connectionReturning($query), + config: ['FroshTools.config.monitorExcludeFailedQueues' => false], + ); + + static::assertSame(SettingsResult::WARNING, $result->state); + static::assertStringContainsString('async_failed', $result->current); + } + + public function testAllowlistRestrictsMonitoredQueues(): void + { + $query = $this->createQueryBuilderMock([ + 'available_at' => (new \DateTimeImmutable('-5 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ]); + + $andWhere = []; + $query->expects(static::exactly(2)) + ->method('andWhere') + ->willReturnCallback(static function (string $predicate) use ($query, &$andWhere): QueryBuilder { + $andWhere[] = $predicate; + + return $query; + }); + + $parameters = []; + $query->expects(static::exactly(2)) + ->method('setParameter') + ->willReturnCallback(static function (string $name, mixed $value, mixed $type = null) use ($query, &$parameters): QueryBuilder { + $parameters[$name] = ['value' => $value, 'type' => $type]; + + return $query; + }); + + $result = $this->collectWith( + connection: $this->connectionReturning($query), + config: [ + 'FroshTools.config.monitorQueues' => 'async, low_priority', + 'FroshTools.config.monitorQueueGraceTime' => 15, + ], + ); + + static::assertContains('queue_name NOT LIKE :failedPattern', $andWhere); + static::assertContains('queue_name IN (:queues)', $andWhere); + static::assertSame('%failed%', $parameters['failedPattern']['value']); + static::assertSame(['async', 'low_priority'], $parameters['queues']['value']); + static::assertSame(ArrayParameterType::STRING, $parameters['queues']['type']); + static::assertSame(SettingsResult::GREEN, $result->state); + } + + public function testPerQueueGraceTimeOverridesDefault(): void + { + $result = $this->collect( + connectionRows: [ + 'available_at' => (new \DateTimeImmutable('-30 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'low_priority', + ], + config: [ + 'FroshTools.config.monitorQueueGraceTime' => 15, + 'FroshTools.config.monitorQueueGraceTimes' => 'low_priority:60, async:10', + ], + ); + + // 30 mins old with 60 min grace for low_priority → OK + static::assertSame(SettingsResult::GREEN, $result->state); + static::assertSame('max 60 mins', $result->recommended); + } + + public function testPerQueueGraceTimeCanTightenDefault(): void + { + $result = $this->collect( + connectionRows: [ + 'available_at' => (new \DateTimeImmutable('-12 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], + config: [ + 'FroshTools.config.monitorQueueGraceTime' => 15, + 'FroshTools.config.monitorQueueGraceTimes' => 'async:10', + ], + ); + + static::assertSame(SettingsResult::WARNING, $result->state); + static::assertSame('max 10 mins', $result->recommended); + } + + /** + * @param array{available_at: string, queue_name: string}|false $connectionRows + * @param array $config + */ + private function collect(array|false $connectionRows, array $config): SettingsResult + { + return $this->collectWith( + connection: $this->connectionReturning($this->createQueryBuilderMock($connectionRows)), + config: $config, + ); + } + + /** + * @param array $config + */ + private function collectWith(Connection $connection, array $config): SettingsResult + { + $configService = $this->createMock(SystemConfigService::class); + $configService->method('getInt')->willReturnCallback( + static fn (string $key): int => (int) ($config[$key] ?? 0), + ); + $configService->method('getString')->willReturnCallback( + static fn (string $key): string => (string) ($config[$key] ?? ''), + ); + $configService->method('get')->willReturnCallback( + static fn (string $key): mixed => $config[$key] ?? null, + ); + + $collection = new HealthCollection(); + (new QueueChecker($connection, $configService))->collect($collection); + + foreach ($collection->getElements() as $element) { + if ($element->id === 'queue') { + return $element; + } + } + + static::fail('HealthCollection does not contain a result with id "queue"'); + } + + /** + * @param array{available_at: string, queue_name: string}|false $row + */ + private function createQueryBuilderMock(array|false $row): QueryBuilder&MockObject + { + $query = $this->createMock(QueryBuilder::class); + $query->method('select')->willReturnSelf(); + $query->method('from')->willReturnSelf(); + $query->method('where')->willReturnSelf(); + $query->method('andWhere')->willReturnSelf(); + $query->method('orderBy')->willReturnSelf(); + $query->method('setMaxResults')->willReturnSelf(); + $query->method('setParameter')->willReturnSelf(); + $query->method('fetchAssociative')->willReturn($row); + + return $query; + } + + private function connectionReturning(QueryBuilder $query): Connection&MockObject + { + $connection = $this->createMock(Connection::class); + $connection->method('createQueryBuilder')->willReturn($query); + + return $connection; + } +} From 3f749b9b1daeec8a80dc0415ac426c3078bd048f Mon Sep 17 00:00:00 2001 From: roboshyim Date: Fri, 24 Jul 2026 05:06:13 +0000 Subject: [PATCH 5/6] fix: evaluate each queue against its own grace period Addresses Greptile P1 on #448: a global oldest-message LIMIT 1 could hide a newer message that already exceeded a shorter per-queue grace. Fetch MIN(available_at) per queue_name and pick the worst offender (furthest past its own grace, else oldest healthy age). --- .../Checker/HealthChecker/QueueChecker.php | 98 ++++++++++++++++--- .../HealthChecker/QueueCheckerTest.php | 15 +++ .../HealthChecker/QueueCheckerUnitTest.php | 83 ++++++++++++---- 3 files changed, 159 insertions(+), 37 deletions(-) diff --git a/src/Components/Health/Checker/HealthChecker/QueueChecker.php b/src/Components/Health/Checker/HealthChecker/QueueChecker.php index 667f4d16..c87731b4 100644 --- a/src/Components/Health/Checker/HealthChecker/QueueChecker.php +++ b/src/Components/Health/Checker/HealthChecker/QueueChecker.php @@ -33,9 +33,9 @@ public function collect(HealthCollection $collection): void $graceByQueue = $this->parseGraceMap($this->configService->getString(self::CONFIG_GRACE_TIMES)); $snippet = 'Open Queues'; - $row = $this->fetchOldestPendingMessage($excludeFailed, $queues); + $pendingByQueue = $this->fetchOldestPendingMessagePerQueue($excludeFailed, $queues); - if ($row === null) { + if ($pendingByQueue === []) { $collection->add(SettingsResult::info( 'queue', $snippet, @@ -46,13 +46,11 @@ public function collect(HealthCollection $collection): void return; } - $queueName = (string) $row['queue_name']; - $grace = $graceByQueue[$queueName] ?? $defaultGrace; - $recommended = \sprintf('max %d mins', $grace); - $ageMinutes = $this->ageInMinutes((string) $row['available_at']); - $current = \sprintf('%d mins (%s)', $ageMinutes, $queueName); + $worst = $this->selectWorstQueue($pendingByQueue, $graceByQueue, $defaultGrace); + $recommended = \sprintf('max %d mins', $worst['grace']); + $current = \sprintf('%d mins (%s)', $worst['ageMinutes'], $worst['queueName']); - if ($ageMinutes > $grace) { + if ($worst['overdue']) { $collection->add(SettingsResult::warning('queue', $snippet, $current, $recommended)); return; @@ -64,16 +62,18 @@ public function collect(HealthCollection $collection): void /** * @param list $queues * - * @return array{available_at: string, queue_name: string}|null + * @return list */ - private function fetchOldestPendingMessage(bool $excludeFailed, array $queues): ?array + private function fetchOldestPendingMessagePerQueue(bool $excludeFailed, array $queues): array { + // One row per queue (oldest pending message). Evaluating each queue against its + // own grace avoids masking a tighter queue behind an older message on a looser one. $query = $this->connection->createQueryBuilder() - ->select('available_at', 'queue_name') + ->select('queue_name', 'MIN(available_at) AS available_at') ->from('messenger_messages') ->where('available_at <= UTC_TIMESTAMP()') - ->orderBy('available_at', 'ASC') - ->setMaxResults(1); + ->groupBy('queue_name') + ->orderBy('available_at', 'ASC'); if ($excludeFailed) { // Symfony failure transport names typically contain "failed" (e.g. async_failed). @@ -88,10 +88,76 @@ private function fetchOldestPendingMessage(bool $excludeFailed, array $queues): ->setParameter('queues', $queues, ArrayParameterType::STRING); } - /** @var array{available_at: string, queue_name: string}|false $row */ - $row = $query->fetchAssociative(); + /** @var list $rows */ + $rows = $query->fetchAllAssociative(); - return \is_array($row) ? $row : null; + return $rows; + } + + /** + * Prefer any overdue queue (highest minutes-over-grace); otherwise the oldest pending age. + * + * @param list $pendingByQueue + * @param array $graceByQueue + * + * @return array{queueName: string, ageMinutes: int, grace: int, overdue: bool} + */ + private function selectWorstQueue(array $pendingByQueue, array $graceByQueue, int $defaultGrace): array + { + $worst = null; + + foreach ($pendingByQueue as $row) { + $queueName = (string) $row['queue_name']; + $grace = $graceByQueue[$queueName] ?? $defaultGrace; + $ageMinutes = $this->ageInMinutes((string) $row['available_at']); + $overdue = $ageMinutes > $grace; + $overBy = $overdue ? $ageMinutes - $grace : 0; + + $candidate = [ + 'queueName' => $queueName, + 'ageMinutes' => $ageMinutes, + 'grace' => $grace, + 'overdue' => $overdue, + 'overBy' => $overBy, + ]; + + if ($worst === null) { + $worst = $candidate; + continue; + } + + // Overdue always beats healthy. + if ($candidate['overdue'] && !$worst['overdue']) { + $worst = $candidate; + continue; + } + if (!$candidate['overdue'] && $worst['overdue']) { + continue; + } + + // Both overdue: the one furthest past its own grace. + if ($candidate['overdue'] && $worst['overdue']) { + if ($candidate['overBy'] > $worst['overBy'] + || ($candidate['overBy'] === $worst['overBy'] && $candidate['ageMinutes'] > $worst['ageMinutes'])) { + $worst = $candidate; + } + continue; + } + + // Both healthy: report the oldest pending age for visibility. + if ($candidate['ageMinutes'] > $worst['ageMinutes']) { + $worst = $candidate; + } + } + + \assert($worst !== null); + + return [ + 'queueName' => $worst['queueName'], + 'ageMinutes' => $worst['ageMinutes'], + 'grace' => $worst['grace'], + 'overdue' => $worst['overdue'], + ]; } private function ageInMinutes(string $availableAt): int diff --git a/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php b/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php index 9789f9c8..e2b8e1c2 100644 --- a/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php +++ b/tests/Components/Health/Checker/HealthChecker/QueueCheckerTest.php @@ -115,6 +115,21 @@ public function testPerQueueGraceTimeIsApplied(): void static::assertSame('max 120 mins', $result->recommended); } + public function testShorterGraceQueueIsNotMaskedByOlderLooserQueue(): void + { + $this->configService->set('FroshTools.config.monitorQueueGraceTime', 15); + $this->configService->set('FroshTools.config.monitorQueueGraceTimes', 'async:10, low_priority:120'); + // Older message on a loose queue must not hide a newer overdue tight queue. + $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 90 MINUTE', 'low_priority'); + $this->insertMessage('UTC_TIMESTAMP() - INTERVAL 20 MINUTE', 'async'); + + $result = $this->collectQueueResult(); + + static::assertSame(SettingsResult::WARNING, $result->state); + static::assertStringContainsString('async', $result->current); + static::assertSame('max 10 mins', $result->recommended); + } + private function insertMessage(string $availableAt, string $queueName = 'default'): void { $this->connection->executeStatement( diff --git a/tests/Components/Health/Checker/HealthChecker/QueueCheckerUnitTest.php b/tests/Components/Health/Checker/HealthChecker/QueueCheckerUnitTest.php index 2ef959df..5753a18b 100644 --- a/tests/Components/Health/Checker/HealthChecker/QueueCheckerUnitTest.php +++ b/tests/Components/Health/Checker/HealthChecker/QueueCheckerUnitTest.php @@ -25,7 +25,7 @@ class QueueCheckerUnitTest extends TestCase public function testEmptyQueueResultsInInfoState(): void { $result = $this->collect( - connectionRows: false, + connectionRows: [], config: [], ); @@ -37,8 +37,10 @@ public function testOldMessageResultsInWarningState(): void { $result = $this->collect( connectionRows: [ - 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', + [ + 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], ], config: ['FroshTools.config.monitorQueueGraceTime' => 15], ); @@ -51,8 +53,10 @@ public function testRecentMessageWithinGracePeriodResultsInOkState(): void { $result = $this->collect( connectionRows: [ - 'available_at' => (new \DateTimeImmutable('-1 minute', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', + [ + 'available_at' => (new \DateTimeImmutable('-1 minute', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], ], config: ['FroshTools.config.monitorQueueGraceTime' => 15], ); @@ -65,8 +69,10 @@ public function testMessageJustOverGraceIsWarningNotDoubleGrace(): void // Regression: previous formula effectively required age > 2 * grace. $result = $this->collect( connectionRows: [ - 'available_at' => (new \DateTimeImmutable('-20 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', + [ + 'available_at' => (new \DateTimeImmutable('-20 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], ], config: ['FroshTools.config.monitorQueueGraceTime' => 15], ); @@ -76,7 +82,7 @@ public function testMessageJustOverGraceIsWarningNotDoubleGrace(): void public function testFailedQueuesAreExcludedByDefault(): void { - $query = $this->createQueryBuilderMock(false); + $query = $this->createQueryBuilderMock([]); $query->expects(static::once()) ->method('andWhere') ->with('queue_name NOT LIKE :failedPattern') @@ -97,8 +103,10 @@ public function testFailedQueuesAreExcludedByDefault(): void public function testFailedQueuesCanBeIncluded(): void { $query = $this->createQueryBuilderMock([ - 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async_failed', + [ + 'available_at' => (new \DateTimeImmutable('-2 hours', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async_failed', + ], ]); $query->expects(static::never())->method('andWhere'); @@ -114,8 +122,10 @@ public function testFailedQueuesCanBeIncluded(): void public function testAllowlistRestrictsMonitoredQueues(): void { $query = $this->createQueryBuilderMock([ - 'available_at' => (new \DateTimeImmutable('-5 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', + [ + 'available_at' => (new \DateTimeImmutable('-5 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], ]); $andWhere = []; @@ -156,8 +166,10 @@ public function testPerQueueGraceTimeOverridesDefault(): void { $result = $this->collect( connectionRows: [ - 'available_at' => (new \DateTimeImmutable('-30 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'low_priority', + [ + 'available_at' => (new \DateTimeImmutable('-30 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'low_priority', + ], ], config: [ 'FroshTools.config.monitorQueueGraceTime' => 15, @@ -174,8 +186,10 @@ public function testPerQueueGraceTimeCanTightenDefault(): void { $result = $this->collect( connectionRows: [ - 'available_at' => (new \DateTimeImmutable('-12 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), - 'queue_name' => 'async', + [ + 'available_at' => (new \DateTimeImmutable('-12 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], ], config: [ 'FroshTools.config.monitorQueueGraceTime' => 15, @@ -187,11 +201,37 @@ public function testPerQueueGraceTimeCanTightenDefault(): void static::assertSame('max 10 mins', $result->recommended); } + public function testShorterGraceQueueIsNotMaskedByOlderLooserQueue(): void + { + // Greptile P1: oldest global message was on low_priority (grace 120), which masked + // a newer async message that already exceeded async's shorter grace (10). + $result = $this->collect( + connectionRows: [ + [ + 'available_at' => (new \DateTimeImmutable('-90 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'low_priority', + ], + [ + 'available_at' => (new \DateTimeImmutable('-20 minutes', new \DateTimeZone('UTC')))->format('Y-m-d H:i:s'), + 'queue_name' => 'async', + ], + ], + config: [ + 'FroshTools.config.monitorQueueGraceTime' => 15, + 'FroshTools.config.monitorQueueGraceTimes' => 'async:10, low_priority:120', + ], + ); + + static::assertSame(SettingsResult::WARNING, $result->state); + static::assertStringContainsString('async', $result->current); + static::assertSame('max 10 mins', $result->recommended); + } + /** - * @param array{available_at: string, queue_name: string}|false $connectionRows + * @param list $connectionRows * @param array $config */ - private function collect(array|false $connectionRows, array $config): SettingsResult + private function collect(array $connectionRows, array $config): SettingsResult { return $this->collectWith( connection: $this->connectionReturning($this->createQueryBuilderMock($connectionRows)), @@ -228,19 +268,20 @@ private function collectWith(Connection $connection, array $config): SettingsRes } /** - * @param array{available_at: string, queue_name: string}|false $row + * @param list $rows */ - private function createQueryBuilderMock(array|false $row): QueryBuilder&MockObject + private function createQueryBuilderMock(array $rows): QueryBuilder&MockObject { $query = $this->createMock(QueryBuilder::class); $query->method('select')->willReturnSelf(); $query->method('from')->willReturnSelf(); $query->method('where')->willReturnSelf(); $query->method('andWhere')->willReturnSelf(); + $query->method('groupBy')->willReturnSelf(); $query->method('orderBy')->willReturnSelf(); $query->method('setMaxResults')->willReturnSelf(); $query->method('setParameter')->willReturnSelf(); - $query->method('fetchAssociative')->willReturn($row); + $query->method('fetchAllAssociative')->willReturn($rows); return $query; } From f7ff327037e922e4c87a619355d9b7c030bdf241 Mon Sep 17 00:00:00 2001 From: roboshyim Date: Fri, 24 Jul 2026 05:13:34 +0000 Subject: [PATCH 6/6] fix: satisfy phpstan in QueueChecker worst-queue selection booleanAnd.rightAlwaysTrue after the overdue/healthy branch; simplify comparison so shopware-cli extension validate --full is clean. --- .../Health/Checker/HealthChecker/QueueChecker.php | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Components/Health/Checker/HealthChecker/QueueChecker.php b/src/Components/Health/Checker/HealthChecker/QueueChecker.php index c87731b4..8a81d7ba 100644 --- a/src/Components/Health/Checker/HealthChecker/QueueChecker.php +++ b/src/Components/Health/Checker/HealthChecker/QueueChecker.php @@ -127,16 +127,15 @@ private function selectWorstQueue(array $pendingByQueue, array $graceByQueue, in } // Overdue always beats healthy. - if ($candidate['overdue'] && !$worst['overdue']) { - $worst = $candidate; - continue; - } - if (!$candidate['overdue'] && $worst['overdue']) { + if ($candidate['overdue'] !== $worst['overdue']) { + if ($candidate['overdue']) { + $worst = $candidate; + } continue; } - // Both overdue: the one furthest past its own grace. - if ($candidate['overdue'] && $worst['overdue']) { + // Same state: both overdue → furthest past grace; both healthy → oldest age. + if ($candidate['overdue']) { if ($candidate['overBy'] > $worst['overBy'] || ($candidate['overBy'] === $worst['overBy'] && $candidate['ageMinutes'] > $worst['ageMinutes'])) { $worst = $candidate; @@ -144,7 +143,6 @@ private function selectWorstQueue(array $pendingByQueue, array $graceByQueue, in continue; } - // Both healthy: report the oldest pending age for visibility. if ($candidate['ageMinutes'] > $worst['ageMinutes']) { $worst = $candidate; }