diff --git a/src/Queue/Broker/Redis.php b/src/Queue/Broker/Redis.php index dbf20ef..ae257c3 100644 --- a/src/Queue/Broker/Redis.php +++ b/src/Queue/Broker/Redis.php @@ -450,23 +450,30 @@ public function reap(Queue $queue, int $olderThan = 90000, ?int $limit = null, ? $ownerKey = "{$queue->namespace}.owners.{$queue->name}.{$pid}"; $owner = $this->commands->get($ownerKey); - // Only legacy payloads expire while processing. $job = $this->getJob($queue, $pid); - if ($job === false) { - if (\is_string($owner)) { - throw new \RuntimeException('Queue delivery payload is missing'); - } + + // Legacy claims carry no ownership record, so a payload that is + // gone leaves nothing to reclaim atomically: drop the entry. + if ($job === false && !\is_string($owner)) { $this->commands->listRemove($processing, $pid); continue; } - if ($job->getTimestamp() > $cutoff - || \is_string($this->commands->get("{$queue->namespace}.claims.{$queue->name}.{$pid}"))) { + // An owned claim can lose its payload too -- a settle in another + // worker lands between the reads above, or maxmemory evicts a job + // key, which is held without a TTL. Neither is worth failing the + // sweep over: the claim cannot be requeued without its payload, so + // it is parked like an exhausted one and the sweep moves on. The + // script still refuses claims a live heartbeat says are in hand. + if ($job !== false + && ($job->getTimestamp() > $cutoff + || \is_string($this->commands->get("{$queue->namespace}.claims.{$queue->name}.{$pid}")))) { $retained++; continue; } - $dead = ($maxAttempts !== null && $job->getAttempts() >= $maxAttempts) + $dead = $job === false + || ($maxAttempts !== null && $job->getAttempts() >= $maxAttempts) || ($newerThan !== null && $job->getTimestamp() < $now - $newerThan); $moved = $this->script($this->commands, 'reclaim', [ $ownerKey, "{$queue->namespace}.claims.{$queue->name}.{$pid}", diff --git a/src/Queue/Broker/Redis/reclaim.lua b/src/Queue/Broker/Redis/reclaim.lua index 0fb479a..4fe4751 100644 --- a/src/Queue/Broker/Redis/reclaim.lua +++ b/src/Queue/Broker/Redis/reclaim.lua @@ -8,7 +8,9 @@ end local counter = redis.call('GET', KEYS[5]) if counter and not string.match(counter, '^%-?%d+$') then return redis.error_reply('Invalid queue counter') end if (redis.call('GET', KEYS[1]) or '') ~= ARGV[1] or redis.call('EXISTS', KEYS[2]) == 1 then return 0 end -if redis.call('EXISTS', KEYS[3]) == 0 then return 0 end +-- Requeueing replays a payload read before this call; a payload that has since +-- gone means someone else settled the claim. Parking needs no payload at all. +if ARGV[3] ~= '' and redis.call('EXISTS', KEYS[3]) == 0 then return 0 end if redis.call('LREM', KEYS[4], 1, ARGV[2]) == 0 then return 0 end redis.call('DEL', KEYS[1], KEYS[2]) redis.call('DECR', KEYS[5]) diff --git a/tests/Queue/E2E/Adapter/RedisBrokerRecoveryTest.php b/tests/Queue/E2E/Adapter/RedisBrokerRecoveryTest.php index 373e9fd..92f318c 100644 --- a/tests/Queue/E2E/Adapter/RedisBrokerRecoveryTest.php +++ b/tests/Queue/E2E/Adapter/RedisBrokerRecoveryTest.php @@ -91,6 +91,62 @@ public function testReapDropsLegacyClaimsWhosePayloadExpired(): void $this->assertSame(0, $this->broker->getQueueSize($this->queue)); } + public function testReapClearsAnOwnedClaimWhosePayloadIsGone(): void + { + // A payload can go missing under a live ownership record: a settle + // running in another worker deletes the job key while this sweep sits + // between its owner read and its payload read, and maxmemory eviction + // reaches claimed payloads because they are stored without a TTL. + // Either way the claim is unrecoverable, and a sweep that fails on it + // never reaches the claims behind it. + $this->broker->publish($this->queue, ['n' => 1]); + $claimed = $this->broker->receive($this->queue, 0)[0] ?? null; + $this->assertInstanceOf(\Utopia\Queue\Message::class, $claimed); + $this->expire('.claims.*'); + $this->redis->del($this->namespace . '.jobs.recovery.' . $claimed->getPid()); + + $requeued = $this->broker->reap($this->queue, olderThan: 0); + + $this->assertSame(0, $requeued, 'there is no payload left to requeue'); + $this->assertSame(0, $this->processingSize(), 'the unrecoverable claim is cleared'); + $this->assertSame(1, $this->deadSize(), 'the delivery is parked for a human'); + $this->assertSame(0, $this->broker->getQueueSize($this->queue)); + $this->assertSame(0, $this->redis->exists($this->namespace . '.owners.recovery.' . $claimed->getPid()), 'the ownership record goes with it'); + $this->assertSame('0', (string) $this->redis->get($this->namespace . '.stats.recovery.processing'), 'the processing counter is settled'); + } + + public function testReapKeepsSweepingPastAClaimWhosePayloadIsGone(): void + { + $this->broker->publish($this->queue, ['n' => 1]); + $this->broker->publish($this->queue, ['n' => 2]); + $claimed = $this->broker->receive($this->queue, 0, 2); + $this->assertCount(2, $claimed); + $this->expire('.claims.*'); + $this->redis->del($this->namespace . '.jobs.recovery.' . $claimed[0]->getPid()); + + $requeued = $this->broker->reap($this->queue, olderThan: 0); + + $this->assertSame(1, $requeued, 'the claim behind the broken one is still recovered'); + $this->assertSame(0, $this->processingSize()); + $this->assertSame(1, $this->broker->getQueueSize($this->queue)); + } + + public function testAHeartbeatedClaimWithoutItsPayloadIsLeftAlone(): void + { + // Ownership plus a live heartbeat means a worker is still on it; the + // missing payload is its problem to settle, not the sweep's to take. + $this->broker->publish($this->queue, ['n' => 1]); + $claimed = $this->broker->receive($this->queue, 0)[0] ?? null; + $this->assertInstanceOf(\Utopia\Queue\Message::class, $claimed); + $this->redis->del($this->namespace . '.jobs.recovery.' . $claimed->getPid()); + + $requeued = $this->broker->reap($this->queue, olderThan: 0); + + $this->assertSame(0, $requeued); + $this->assertSame(1, $this->processingSize(), 'the live claim keeps its worker'); + $this->assertSame(0, $this->deadSize()); + } + public function testReapParksExhaustedClaimsOnTheDeadQueue(): void { $this->broker->publish($this->queue, ['n' => 1]);