Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[messenger] \Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware do not run retry logic as intended #52903

Closed
itcodeOstrowski opened this issue Dec 5, 2023 · 3 comments

Comments

@itcodeOstrowski
Copy link

itcodeOstrowski commented Dec 5, 2023

Symfony version(s) affected

6.4

Description

Hello,

If messages retrieved from the queue with $amqpReceivedStamp->getAmqpEnvelope()->isRedelivery() are true, the RejectRedeliveredMessageException will be thrown in RejectRedeliveredMessageMiddleware. This should cause the default retry logic to run, but because RejectRedeliveredMessageException implements UnrecoverableExceptionInterface, and because in the SendFailedMessageForRetryListener::shouldRetry() logic we check for UnrecoverableExceptionInterface, the retry logic is not triggered.

How to reproduce

  1. Consume message that has Redelivered stamp.
  2. Message should be queued again with default retry logic, but instead it is gone.

Possible Solution

Just revert that commit: symfony/messenger@2479dc6 (i don't understand intention of this commit, maybe just use \Symfony\Component\Messenger\Exception\RecoverableExceptionInterface instead) or change description of \Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware because it is misleading otherwise.

Additional Context

interesting classes:
\Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware
\Symfony\Component\Messenger\Exception\RejectRedeliveredMessageException
\Symfony\Component\Messenger\EventListener\SendFailedMessageForRetryListener::shouldRetry

This bug makes usage of rabbitMQ much more difficult because now it is not possible to check messages in web interface, if the check is done, it would be not possible to use that message.

@itcodeOstrowski itcodeOstrowski changed the title \Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware do not run retry logic as intended. \Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware do not run retry logic as intended Dec 5, 2023
@itcodeOstrowski itcodeOstrowski changed the title \Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware do not run retry logic as intended [messenger] \Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware do not run retry logic as intended Dec 5, 2023
@mythxxx
Copy link

mythxxx commented Feb 9, 2024

This is definitely a bug, redelivered messages should not be simply dropped, but retried as before. At this moment we are losing messages in production, we had to revert to v.6.3.12

@lermontex
Copy link

/cc @nikophil (PR #51756)

@nikophil
Copy link
Contributor

Hi, sorry for the late reply and for that bug 😬

I'm going to revert this very soon

nicolas-grekas added a commit that referenced this issue Mar 12, 2024
…ry (nikophil)

This PR was merged into the 6.4 branch.

Discussion
----------

[Messenger] trigger retry logic when message is a redelivery

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #52903 #53311
| License       | MIT

This PR rollbacks #51756 which was an error

ping `@itcodeOstrowski` `@beermeat`

Commits
-------

e397200 [Messenger] trigger retry logic when message is a redelivery
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants