Skip to content

fix: delay queues are never redeclared - #39

Merged
iamfarhad merged 1 commit into
iamfarhad:masterfrom
beschoenen:master
Aug 18, 2026
Merged

iamfarhad merged 1 commit into
iamfarhad:masterfrom
beschoenen:master

Conversation

@beschoenen

Copy link
Copy Markdown
Contributor

Summary

Delayed jobs are held in a <queue>.delay.<ttl> queue that RabbitMQ deletes once it falls idle (x-expires), but $declaredTopology memoizes the declaration for the life of the process with no invalidation.

A publisher whose last publish to that TTL bucket was longer ago than the queue's lifetime therefore skips the redeclare and publishes into a queue that no longer exists — the default exchange discards the message silently, with no exception and no failed job.

Any workload that dispatches delayed jobs at intervals hits this routinely: with several worker processes each keeping its own memo, one process's gap between publishes to the same bucket easily exceeds x-expires, and the job is lost.

Type of change

  • Bug fix
  • New feature
  • Documentation
  • Refactor / maintenance
  • Breaking change

Checklist

  • I have read CONTRIBUTING.md.
  • I added or updated tests where relevant.
  • I updated documentation where relevant.
  • composer format-test passes.
  • composer analyse passes.
  • composer test passes.

Runtime matrix checked

  • PHP version(s): 8.5.9
  • Laravel version(s): 13.12
  • RabbitMQ version(s): 4.0.9

Notes for reviewers

@iamfarhad

Copy link
Copy Markdown
Owner

Thanks for the report and the patch — the diagnosis is correct, and it is a regression I introduced in #38. Before that, declareDelayQueue() went through declareQueue() on every laterRaw(), and since redeclaring is one of the three things RabbitMQ counts as using a queue (consumers, redeclare, basic.get — publishing is not), that per-publish declare was quietly keeping the x-expires timer alive. Memoising it removed both the redeclare and the renewal, so the queue is deleted under a live producer and the publish to the default exchange is discarded with no error, since AMQP_MANDATORY is only set when publisher confirms are on.

The approach is right, and keying on x-expires rather than special-casing the delay path is better than what I had in mind. Verified locally: composer format-test and composer analyse clean, the suite matches master's baseline, and testDelayQueueIsRedeclaredOnceItsBrokerSideLifetimeHasPassed does fail without the src change.

One thing needs tightening before this can go in. The window is derived from x-expires alone, but what makes a memo safe is window <= x-expires - x-message-ttl: a message published at the very end of the window still needs its full TTL to dead-letter out. For a delay queue x-expires is 2 * ttl, so ceil($ttlMs / 2000) is the ttl — the window lands on the bound rather than inside it:

ttl x-expires window max safe window
1000 2000 1000 1000 on the bound, margin → 0
1200 2400 2000 1200 over by 800 ms
1500 3000 2000 1500 over by 500 ms
10000 20000 10000 10000 on the bound, margin → 0

Two consequences:

1. ceil() rounds the window up, so any bucketed TTL that is not a whole number of seconds goes over the bound. That is reachable through RABBITMQ_DELAY_QUEUE_GRANULARITY — 300, 750, 1500, 2500 and so on. With granularity 300 a 1 s delay becomes a 1200 ms TTL, so the declare may only be trusted for 1200 ms, but this fails on the branch:

Carbon::setTestNow('2026-08-18 09:40:57.000');
$this->bindConfig(['delay_queue_granularity' => 300]);
// ...
$queue->laterRaw(1, '{"id":"a"}', 'orders');    // ttl 1200 ms, x-expires 2400 ms

Carbon::setTestNow('2026-08-18 09:40:58.500');  // past x-expires - ttl
$queue->laterRaw(1, '{"id":"b"}', 'orders');

$this->assertSame(2, $this->countDeclarations($declaredNames, 'orders.delay.1200'));
// Failed asserting that 1 is identical to 2

That is the original bug again, in a smaller window.

2. In the default config the margin is resolution-limited rather than merely small. currentTime() truncates to whole seconds, so when a declare lands just after a second boundary a publish can follow ~ttl later, and the message's dead-letter then coincides with the broker's x-expires deadline — which the broker measures from when the declare arrived, not when it was sent. A queue removed by x-expires discards its contents without dead-lettering them, so that race loses the message.

Both go away with max(1, intdiv($expires - $messageTtl, 2)) — both arguments are already in $arguments — compared in milliseconds instead of whole seconds. (int) Carbon::now()->format('Uv') keeps one notion of "now" with the secondsUntil() arithmetic inherited from InteractsWithTime and stays controllable through setTestNow().

Two smaller things:

  • queueLifetime()'s comment says auto-delete "never happens to a queue this class declares for publishing", but RABBITMQ_QUEUE_AUTO_DELETE is a documented per-queue override, the same class declares those names on the consume path, and once a consumer has come and gone the broker deletes the queue while a separate producer still holds its memo — the same silent drop. Since there is no timed bound to work with there, the simplest answer is not to memoise an auto-delete declare at all.
  • Please add a ## [Unreleased] entry to CHANGELOG.md; releases are gated on it.

With the window tightened I will merge this and cut a release.

@iamfarhad
iamfarhad merged commit ef5252a into iamfarhad:master Aug 18, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants