bug #49063 [Messenger] Respect isRetryable decision of the retry st…

…rategy for re-delivery (FlyingDR)

This PR was merged into the 5.4 branch.

Discussion
----------

[Messenger] Respect `isRetryable` decision of the retry strategy for re-delivery

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

[Documentation](https://symfony.com/doc/5.4/messenger.html#retries-failures) for retry strategy for the Messenger component declares that message will not be retried to be re-delivered more than the value of `max_retries` configuration for the retry strategy.

However, after merging #36557 [actual implementation](https://github.com/symfony/symfony/blob/39cd93a9f7ea072b26853e87ceb1142d6dd019b0/src/Symfony/Component/Messenger/EventListener/SendFailedMessageForRetryListener.php#L126-L128) gives priority to the existence of the `RecoverableExceptionInterface`, effectively opening a way for a message to be re-delivered indefinitely.

Additionally, in the case of using `multiplier` with a value of more than 1 this unlimited re-delivery causes unlimited growth of the `delay` value for the `DelayStamp`. Its type is defined as `int`, so on 32-bit platforms after some time `delay` value may exceed `PHP_INT_MAX` which will lead to the `TypeError`.

The proposed change enforces respect for the `max_retries` setting value for the decision of re-delivery.

Commits
-------

8fc3dcc45d [Messenger] Respect `isRetryable` decision of the retry strategy when deciding if failed message should be re-delivered

EventListener/SendFailedMessageForRetryListener.php

@@ -123,7 +123,8 @@ public static function getSubscribedEvents()
 
     private function shouldRetry(\Throwable $e, Envelope $envelope, RetryStrategyInterface $retryStrategy): bool
     {
-        if ($e instanceof RecoverableExceptionInterface) {
+        $isRetryable = $retryStrategy->isRetryable($envelope, $e);
+        if ($isRetryable && $e instanceof RecoverableExceptionInterface) {
             return true;
         }
 
@@ -132,7 +133,7 @@ private function shouldRetry(\Throwable $e, Envelope $envelope, RetryStrategyInt
         if ($e instanceof HandlerFailedException) {
             $shouldNotRetry = true;
             foreach ($e->getNestedExceptions() as $nestedException) {
-                if ($nestedException instanceof RecoverableExceptionInterface) {
+                if ($isRetryable && $nestedException instanceof RecoverableExceptionInterface) {
                     return true;
                 }
 
@@ -150,7 +151,7 @@ private function shouldRetry(\Throwable $e, Envelope $envelope, RetryStrategyInt
             return false;
         }
 
-        return $retryStrategy->isRetryable($envelope, $e);
+        return $isRetryable;
     }
 
     private function getRetryStrategyForTransport(string $alias): ?RetryStrategyInterface

Tests/EventListener/SendFailedMessageForRetryListenerTest.php

@@ -63,7 +63,7 @@ public function testRecoverableStrategyCausesRetry()
         $senderLocator->expects($this->once())->method('has')->willReturn(true);
         $senderLocator->expects($this->once())->method('get')->willReturn($sender);
         $retryStategy = $this->createMock(RetryStrategyInterface::class);
-        $retryStategy->expects($this->never())->method('isRetryable');
+        $retryStategy->expects($this->once())->method('isRetryable')->willReturn(true);
         $retryStategy->expects($this->once())->method('getWaitingTime')->willReturn(1000);
         $retryStrategyLocator = $this->createMock(ContainerInterface::class);
         $retryStrategyLocator->expects($this->once())->method('has')->willReturn(true);
@@ -78,6 +78,27 @@ public function testRecoverableStrategyCausesRetry()
         $listener->onMessageFailed($event);
     }
 
+    public function testRetryIsOnlyAllowedWhenPermittedByRetryStrategy()
+    {
+        $senderLocator = $this->createMock(ContainerInterface::class);
+        $senderLocator->expects($this->never())->method('has');
+        $senderLocator->expects($this->never())->method('get');
+        $retryStrategy = $this->createMock(RetryStrategyInterface::class);
+        $retryStrategy->expects($this->once())->method('isRetryable')->willReturn(false);
+        $retryStrategy->expects($this->never())->method('getWaitingTime');
+        $retryStrategyLocator = $this->createMock(ContainerInterface::class);
+        $retryStrategyLocator->expects($this->once())->method('has')->willReturn(true);
+        $retryStrategyLocator->expects($this->once())->method('get')->willReturn($retryStrategy);
+
+        $listener = new SendFailedMessageForRetryListener($senderLocator, $retryStrategyLocator);
+
+        $exception = new RecoverableMessageHandlingException('retry');
+        $envelope = new Envelope(new \stdClass());
+        $event = new WorkerMessageFailedEvent($envelope, 'my_receiver', $exception);
+
+        $listener->onMessageFailed($event);
+    }
+
     public function testEnvelopeIsSentToTransportOnRetry()
     {
         $exception = new \Exception('no!');