diff --git a/src/Model/Filter/QueuedJobsCollection.php b/src/Model/Filter/QueuedJobsCollection.php index a515c87a..dd96904b 100644 --- a/src/Model/Filter/QueuedJobsCollection.php +++ b/src/Model/Filter/QueuedJobsCollection.php @@ -6,6 +6,7 @@ use Cake\Http\Exception\NotImplementedException; use Cake\I18n\DateTime; use Cake\ORM\Query\SelectQuery; +use Queue\Model\Table\QueuedJobsTable; use Search\Model\Filter\FilterCollection; class QueuedJobsCollection extends FilterCollection { @@ -32,9 +33,17 @@ public function initialize(): void { if ($status === 'in_progress') { $query->where([ 'completed IS' => null, + // Exclude terminally-failed (aborted) jobs: they are + // done, not in progress, even without a completed stamp. 'OR' => [ - 'notbefore <=' => new DateTime(), - 'notbefore IS' => null, + 'status IS' => null, + 'status !=' => QueuedJobsTable::STATUS_ABORTED, + ], + 'AND' => [ + 'OR' => [ + 'notbefore <=' => new DateTime(), + 'notbefore IS' => null, + ], ], ]); diff --git a/src/Model/Table/QueuedJobsTable.php b/src/Model/Table/QueuedJobsTable.php index d94e07ae..1a4feb04 100644 --- a/src/Model/Table/QueuedJobsTable.php +++ b/src/Model/Table/QueuedJobsTable.php @@ -76,6 +76,16 @@ class QueuedJobsTable extends Table { */ public const DAY = 86400; + /** + * Terminal `status` value stamped on a job that has exhausted all of its + * retries. Distinguishes a permanently-failed job (which will never run + * again) from one that is pending, in progress, or still being retried — + * all of which otherwise share `completed IS NULL`. + * + * @var string + */ + public const STATUS_ABORTED = 'aborted'; + /** * @var array */ @@ -291,6 +301,15 @@ public function isQueued(string $reference, ?string $jobTask = null): bool { $conditions = [ 'reference' => $reference, 'completed IS' => null, + // A job that exhausted its retries (status = aborted) will never + // run again, so it is not "queued" anymore even though it has no + // completed timestamp. Excluding it stops callers that gate on + // isQueued() — e.g. a non-concurrent scheduler row — from wedging + // permanently behind a dead job. The OR keeps null/other statuses. + 'OR' => [ + 'status IS' => null, + 'status !=' => static::STATUS_ABORTED, + ], ]; if ($jobTask) { $conditions['job_task'] = $jobTask; @@ -769,6 +788,26 @@ public function markJobFailed(QueuedJob $job, ?string $failureMessage = null, ?s return (bool)$this->save($job); } + /** + * Mark a job as terminally failed (aborted) after it has exhausted all + * retries. Records the terminal `status` so the job is no longer reported + * as queued/in-progress: `isQueued()` excludes it, the admin status filter + * skips it, and the status badge renders it as failed rather than running. + * + * Call this only once the worker has determined no further attempts will + * be made (see Processor, getFailedStatus() === self::STATUS_ABORTED). + * + * @param \Queue\Model\Entity\QueuedJob $job Job + * + * @return bool Success + */ + public function markJobAborted(QueuedJob $job): bool { + return (bool)$this->updateAll( + ['status' => static::STATUS_ABORTED], + ['id' => $job->id], + ); + } + /** * Removes all failed jobs. * @@ -1202,7 +1241,7 @@ public function getFailedStatus(QueuedJob $queuedTask, array $taskConfiguration) return $failureMessageRequeued; } - return 'aborted'; + return static::STATUS_ABORTED; } /** diff --git a/src/Queue/Processor.php b/src/Queue/Processor.php index 36b2412d..2b887571 100644 --- a/src/Queue/Processor.php +++ b/src/Queue/Processor.php @@ -349,7 +349,13 @@ protected function runJob(QueuedJob $queuedJob, string $pid): void { EventManager::instance()->dispatch($event); // Dispatch event when job has exhausted all retries - if ($failedStatus === 'aborted') { + if ($failedStatus === $this->QueuedJobs::STATUS_ABORTED) { + // Persist the terminal state so the job stops counting as + // queued/in-progress (it will never run again). Without this + // it keeps `completed IS NULL` forever and wedges callers that + // gate on isQueued(), e.g. a non-concurrent scheduler row. + $this->QueuedJobs->markJobAborted($queuedJob); + $event = new Event('Queue.Job.maxAttemptsExhausted', $this, [ 'job' => $queuedJob, 'failureMessage' => $failureMessage, diff --git a/templates/element/Queue/status_badge.php b/templates/element/Queue/status_badge.php index da1d4ef4..3352b48d 100644 --- a/templates/element/Queue/status_badge.php +++ b/templates/element/Queue/status_badge.php @@ -1,10 +1,12 @@ completed) { $status = 'completed'; $icon = 'check'; -} elseif ($job->failure_message) { +} elseif ($job->status === QueuedJobsTable::STATUS_ABORTED || $job->failure_message) { + // Terminal: retries exhausted (status = aborted) or a recorded failure. + // Checked before `fetched` so a job whose worker died on its last attempt + // shows as Failed, not stuck "Running". $status = 'failed'; $icon = 'times'; } elseif ($job->fetched) { diff --git a/tests/TestCase/Model/Table/QueuedJobsTableTest.php b/tests/TestCase/Model/Table/QueuedJobsTableTest.php index 3dd24c0d..acbd2aa7 100644 --- a/tests/TestCase/Model/Table/QueuedJobsTableTest.php +++ b/tests/TestCase/Model/Table/QueuedJobsTableTest.php @@ -834,6 +834,46 @@ public function testIsQueued() { $this->assertFalse($result); } + /** + * A job that has exhausted its retries (status = aborted) is terminal and + * must no longer count as queued, even though it has no completed stamp. + * Normal null-status rows still count. + * + * @return void + */ + public function testIsQueuedExcludesAborted() { + $queuedJob = $this->QueuedJobs->newEntity([ + 'key' => 'key', + 'job_task' => 'FooBar', + 'reference' => 'foo-bar', + ]); + $this->QueuedJobs->saveOrFail($queuedJob); + $this->assertTrue($this->QueuedJobs->isQueued('foo-bar')); + + $this->QueuedJobs->markJobAborted($queuedJob); + $this->assertFalse($this->QueuedJobs->isQueued('foo-bar')); + } + + /** + * markJobAborted() stamps the terminal status without touching completed. + * + * @return void + */ + public function testMarkJobAborted() { + $queuedJob = $this->QueuedJobs->newEntity([ + 'key' => 'key', + 'job_task' => 'FooBar', + 'reference' => 'foo-bar', + ]); + $this->QueuedJobs->saveOrFail($queuedJob); + + $this->assertTrue($this->QueuedJobs->markJobAborted($queuedJob)); + + $reloaded = $this->QueuedJobs->get($queuedJob->id); + $this->assertSame(QueuedJobsTable::STATUS_ABORTED, $reloaded->status); + $this->assertNull($reloaded->completed); + } + /** * @return void */ diff --git a/tests/TestCase/Queue/ProcessorTest.php b/tests/TestCase/Queue/ProcessorTest.php index 1170e168..d7c024fa 100644 --- a/tests/TestCase/Queue/ProcessorTest.php +++ b/tests/TestCase/Queue/ProcessorTest.php @@ -271,6 +271,11 @@ public function testMaxAttemptsExhaustedEvent() { // Verify event data // The event was fired successfully (assertEventFired passed) // We don't need to check the event data again since assertEventFired confirms it was fired + + // The exhausted job must be stamped terminal so it no longer counts as + // queued/in-progress (otherwise isQueued() wedges callers behind it). + $reloaded = $QueuedJobs->get($job->id); + $this->assertSame(QueuedJobsTable::STATUS_ABORTED, $reloaded->status); } /**