Repository navigation
Fix publisher confirms, Laravel 13 worker options, and terminal-failure ownership - #29
Merged
iamfarhad merged 1 commit intoAug 4, 2026
Conversation
Fixes four issues reported against 1.3.4. Publisher confirms could not complete a publish (#25). enable() called confirmSelect() without installing ext-amqp's confirm callbacks, so the extension refused to process the broker's acknowledgement and aborted the publish with "Unhandled basic.ack method from server received.". The ACK and NACK callbacks are now registered before confirm mode is switched on, exactly once per instance so repeated enable() calls cannot stack handlers. The callbacks release waitForConfirm() as soon as nothing is outstanding instead of blocking for the whole timeout, and a confirm-wait timeout (AMQPQueueException) is now reported as a failed confirmation instead of escaping unhandled. A NACK could fail a later, acknowledged publish (#26). The stored NACK was never cleared, so on a long-lived publisher or worker every subsequent waitForConfirms() re-threw the first broker NACK. NACK state is now single-use: the wait takes it, clears it, and only then reports the failure. disable() and clearPending() clear it too, and a wait that fails for another reason no longer leaves stale state behind. rabbitmq:consume failed to start on Laravel 13 (#27). The command overrides queue:work's signature but omitted --stop-when-empty-for and --json. WorkCommand::gatherWorkerOptions() reads stop-when-empty-for unconditionally, so Symfony threw for the undefined option and the worker exited before consuming anything -- even when the option was never passed -- putting deployments into a restart loop. Both options are now declared with their queue:work meanings, and a test compares the command against every option the installed WorkCommand reads so future framework drift is caught. Terminal failure had two sinks (#28). A permanently failed job was rejected and also copied to a hard-coded failed_messages exchange, so a queue with broker dead-letter routing recorded the same failure twice, with divergent retention, alerting, and replay policies. Ownership is now explicit via failed.ownership: 'broker' (new default) rejects without requeue and lets the queue's dead-letter exchange own the failure, while 'exchange' keeps the previous copy behaviour against a configurable destination. The copy is published through the default exchange so it lands in the failure queue declared for it, and the re-entry guard also checks the consumed queue -- not just the envelope's exchange name -- so consuming the failure queue can no longer republish failures in a loop. Settings are read from the failing job's own connection, falling back to the default rabbitmq connection.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
iamfarhad
deleted the
fix/publisher-confirms-worker-options-failure-ownership
branch
August 4, 2026 09:23
Merged
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.
Fixes four issues reported against
1.3.4. Each was validated against the source onmasterbefore fixing, and every regression test was re-run against the unfixed code to prove it fails without the change.Fixes #25
Fixes #26
Fixes #27
Fixes #28
Publisher confirms aborted the publish (#25)
PublisherConfirms::enable()calledconfirmSelect()without installing ext-amqp's confirm callbacks, so the extension refused to process the broker's acknowledgement and turned a successful publish into a fatalUnhandled basic.ack method from server received.The callback fix from
64ad6b3only ever existed on the unmergedtest/rmq4-laravel13-coveragebranch, somasterand the1.3.4release were both affected. Implemented fresh rather than ported, since #26 reviews that approach as incorrect.enable()calls cannot stack handlers.waitForConfirm()as soon as nothing is outstanding, instead of returningtrueunconditionally and blocking for the whole timeout.AMQPQueueException) is now reported as a failed confirmation instead of escaping unhandled.AMQPConnectionExceptionstill propagates untouched, soRabbitQueue's dead-channel retry keeps working.A NACK could fail a later, acknowledged publish (#26)
The stored NACK was never cleared, so on a long-lived publisher or worker every subsequent
waitForConfirms()re-threw the first broker NACK.NACK state is now single-use: the wait captures it, clears it, and only then reports the failure.
disable()andclearPending()clear it as well, and a wait that fails for any other reason no longer leaves stale state behind for the next publish. Documented behaviour when several callbacks arrive before a wait returns: the first NACK observed is reported by the next wait, then dropped.rabbitmq:consumefailed to start on Laravel 13 (#27)The command overrides
queue:work's signature but omitted--stop-when-empty-forand--json.Reproduced before fixing:
WorkCommand::gatherWorkerOptions()readsstop-when-empty-forunconditionally, so Symfony threw for the undefined option and the worker exited before consuming anything — even when the option was never passed — putting deployments into a restart loop.One correction to the report:
--jsondid not crash, becauseoutputUsingJson()guards withhasOption(). It was still rejected by the parser, so JSON worker output was unreachable. Both options are now declared with theirqueue:workmeanings.A test asserts the command defines every option the installed
WorkCommandreads (parsed from the framework source), so this drift is caught on whichever Laravel version CI runs.Terminal failure had two sinks (#28)
A permanently failed job was rejected and copied to a hard-coded
failed_messagesexchange, so a queue with broker dead-letter routing recorded the same failure twice, with divergent retention, alerting, and replay policies.Ownership is now explicit via
failed.ownership(RABBITMQ_FAILED_OWNERSHIP):broker(new default)x-dead-letter-exchangeowns the failure.exchangefailed.exchange(previous behaviour, now with a configurable destination).Two adjacent defects found in the same path and fixed:
failed_messagescould republish failures in a loop. It now checks the consumed queue too.Settings are read from the failing job's own connection, falling back to the default
rabbitmqconnection.failureOwner()isprotected, so a custom job class (options.queue.job) can decide ownership per job.Defaulting to
brokermeans anyone relying on thefailed_messagescopy without dead-letter routing stops receiving it until they setRABBITMQ_FAILED_OWNERSHIP=exchange. #28 asked to prefer broker ownership, so that is the default; the restore path is documented inCHANGELOG.md, a newUPGRADE.mdsection, anddocs/production.md.Testing
PublisherConfirmsTest,ConsumeCommandTest, andFailureOwnershipTest; 187 total pass.Consumer.php,Support/RpcClient.php, and the admin commands.FailureOwnershipTestintentionally has noskipIfAmqpExtensionLoaded()guard: it uses plain Mockery doubles (neveroverload:instance mocks), exactly like the existing unguardedAMQPChannel/AMQPConnectionmocks, so the Terminal job failure can reject to RabbitMQ and also copy the message to hard-codedfailed_messages#28 coverage runs in CI where ext-amqp is loaded rather than being silently skipped.