Repository navigation
Conversation
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Delayed delivery in
RabbitMQHandler(push()with a delay andlater()) had three problems.queue_{queue}_delaywas declared once withx-dead-letter-routing-keyfixed to the default priority routing key ({queue}.default). A delayed job with a non-default priority was dead-lettered into the default bucket after its TTL, so priority ordering was lost. In addition, a second process declaring the same argument set differently receivedPRECONDITION_FAILED(406) and lost its channel.later()rejected the original message before the replacement was published.basic_nack()was called beforepublishDelayedMessage(). If the publish failed, the original message was already gone — a silently lost job.publishDelayedMessage()relied on the exchange already existing and re-bound the delay queue on every publish.later()can be called by a worker that never went throughpush(), so the exchange (and its bindings) might not exist at that point. In addition,push()only reset$this->priority/$this->delayon the success path, so a failed push could leak state into the next operation.Changes
src/Handlers/RabbitMQHandler.phpqueue_{queue}_{priority}_delay(getDelayQueueName($queue, $priority)). Thex-dead-letter-routing-keytherefore always targets the priority bucket the job belongs to.queue_{queue}_delayname is kept only for cleanup (getLegacyDelayQueueName()).later()publishes the delayed copy first and only then callsbasic_nack()on the original message. If publishing throws, the original message stays unacknowledged and is not lost.publishDelayedMessage()now ensures the exchange exists (declareExchange()), declares and binds the per-priority delay queue only once (guarded by$this->declaredQueues).declareExchange()binds the priority queues together with the exchange declaration instead of re-binding them on every publish.clearQueue()purges the delay queue of every priority (before the priority queues) and also purges the legacy delay queue, so jobs scheduled before this change cannot leak back after a clear.push()resetspriorityanddelayin afinallyblock so the state can never leak into the next push.Testing
Added
tests/RabbitMQHandlerUnitTest.php, which exercises the handler logic without a live broker (channel and config are mocked):testGetDelayQueueNameIncludesPriority()testPublishDelayedMessageDeclaresPerPriorityQueueAndDeadLetterRouting()— asserts thatx-dead-letter-routing-keymatches the priority bucket.testPublishDelayedMessageBindsDelayQueueOnlyOnceAcrossPublications()testLaterPublishesDelayedMessageBeforeRejectingOriginalMessage()— asserts the publish/basic_nackordering.testLaterDoesNotRejectOriginalMessageWhenPublishingDelayedFails()testClearQueuePurgesDelayQueuesForEachPriority()— including the legacy delay queue.testPushResetsPriorityAndDelayInFinallyBlockOnFailure()The existing
tests/RabbitMQHandlerTest.phpandtests/RabbitMQDelayTest.phpsuites still pass against the integration setup.Verified locally on PHP 8.5 with PHPUnit 11.5: