From 1aa19f06b1011555841ae9184e55d4f7bad0ae50 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Sun, 12 Feb 2023 21:51:53 +0100 Subject: [PATCH 1/2] Do not delete task if task status is 20 --- src/CloudTasksJob.php | 20 ++++++++++++++++++++ tests/QueueTest.php | 6 +++--- 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/src/CloudTasksJob.php b/src/CloudTasksJob.php index 7dcea27..9e804cf 100644 --- a/src/CloudTasksJob.php +++ b/src/CloudTasksJob.php @@ -103,11 +103,31 @@ public function timeoutAt(): ?int public function delete(): void { + // Laravel automatically calls delete() after a job is processed successfully. However, this is + // not what we want to happen in Cloud Tasks because Cloud Tasks will also delete the task upon + // a 200 OK status, which means a task is deleted twice, possibly resulting in errors. So if the + // task was processed successfully (no errors or failures) then we will not delete the task + // manually and will let Cloud Tasks do it. + $successful = + // If the task has failed, we should be able to delete it permanently + $this->hasFailed() === false + // If the task has errored, it should be released, which in process deletes the errored task + && $this->hasError() === false; + + if ($successful) { + return; + } + parent::delete(); $this->cloudTasksQueue->delete($this); } + public function hasError(): bool + { + return data_get($this->job, 'internal.errored') === true; + } + public function release($delay = 0) { parent::release(); diff --git a/tests/QueueTest.php b/tests/QueueTest.php index 29a891d..d4ef6aa 100644 --- a/tests/QueueTest.php +++ b/tests/QueueTest.php @@ -242,7 +242,7 @@ public function jobs_can_be_released() // Assert Event::assertNotDispatched($this->getJobReleasedAfterExceptionEvent()); - CloudTasksApi::assertDeletedTaskCount(1); + CloudTasksApi::assertDeletedTaskCount(0); // it returned 200 OK so we dont delete it, but Google does $releasedJob = null; Event::assertDispatched(JobReleased::class, function (JobReleased $event) use (&$releasedJob) { $releasedJob = $event->job; @@ -257,7 +257,7 @@ public function jobs_can_be_released() $this->runFromPayload($releasedJob->getRawBody()); - CloudTasksApi::assertDeletedTaskCount(2); + CloudTasksApi::assertDeletedTaskCount(0); CloudTasksApi::assertTaskCreated(function (Task $task) { $body = $task->getHttpRequest()->getBody(); $decoded = json_decode($body, true); @@ -476,6 +476,6 @@ public function test_ignoring_jobs_with_deleted_models() // Act Log::assertLogged('UserJob:John'); - CloudTasksApi::assertTaskDeleted($job->task->getName()); + CloudTasksApi::assertTaskNotDeleted($job->task->getName()); } } From 64595e97bc695bcfe98d60e34970f2f23ffbc105 Mon Sep 17 00:00:00 2001 From: Marick van Tuil Date: Wed, 15 Feb 2023 18:56:00 +0100 Subject: [PATCH 2/2] Prevent already deleted tasks from being called again --- .gitignore | 1 + src/CloudTasksApi.php | 2 +- src/CloudTasksApiConcrete.php | 4 +--- src/CloudTasksApiContract.php | 2 +- src/CloudTasksApiFake.php | 2 +- src/TaskHandler.php | 34 ++++++++++++++++++++-------------- tests/CloudTasksApiTest.php | 2 +- 7 files changed, 26 insertions(+), 21 deletions(-) diff --git a/.gitignore b/.gitignore index beb6383..c88b8a9 100644 --- a/.gitignore +++ b/.gitignore @@ -2,3 +2,4 @@ composer.lock .idea/ .phpunit.result.cache +.phpunit.cache diff --git a/src/CloudTasksApi.php b/src/CloudTasksApi.php index bc8f4bd..c113bf4 100644 --- a/src/CloudTasksApi.php +++ b/src/CloudTasksApi.php @@ -13,7 +13,7 @@ * @method static Task createTask(string $queueName, Task $task) * @method static void deleteTask(string $taskName) * @method static Task getTask(string $taskName) - * @method static int|null getRetryUntilTimestamp(string $taskName) + * @method static int|null getRetryUntilTimestamp(Task $task) */ class CloudTasksApi extends Facade { diff --git a/src/CloudTasksApiConcrete.php b/src/CloudTasksApiConcrete.php index 1488074..d63b8ed 100644 --- a/src/CloudTasksApiConcrete.php +++ b/src/CloudTasksApiConcrete.php @@ -50,10 +50,8 @@ public function getTask(string $taskName): Task return $this->client->getTask($taskName); } - public function getRetryUntilTimestamp(string $taskName): ?int + public function getRetryUntilTimestamp(Task $task): ?int { - $task = $this->getTask($taskName); - $attempt = $task->getFirstAttempt(); if (!$attempt instanceof Attempt) { diff --git a/src/CloudTasksApiContract.php b/src/CloudTasksApiContract.php index d43e1ec..aa0880b 100644 --- a/src/CloudTasksApiContract.php +++ b/src/CloudTasksApiContract.php @@ -13,5 +13,5 @@ public function getRetryConfig(string $queueName): RetryConfig; public function createTask(string $queueName, Task $task): Task; public function deleteTask(string $taskName): void; public function getTask(string $taskName): Task; - public function getRetryUntilTimestamp(string $taskName): ?int; + public function getRetryUntilTimestamp(Task $task): ?int; } diff --git a/src/CloudTasksApiFake.php b/src/CloudTasksApiFake.php index 59a046a..c193e71 100644 --- a/src/CloudTasksApiFake.php +++ b/src/CloudTasksApiFake.php @@ -49,7 +49,7 @@ public function getTask(string $taskName): Task } - public function getRetryUntilTimestamp(string $taskName): ?int + public function getRetryUntilTimestamp(Task $task): ?int { return null; } diff --git a/src/TaskHandler.php b/src/TaskHandler.php index a535f30..8be84c3 100644 --- a/src/TaskHandler.php +++ b/src/TaskHandler.php @@ -2,6 +2,7 @@ namespace Stackkit\LaravelGoogleCloudTasksQueue; +use Google\ApiCore\ApiException; use Google\Cloud\Tasks\V2\CloudTasksClient; use Google\Cloud\Tasks\V2\RetryConfig; use Illuminate\Bus\Queueable; @@ -122,6 +123,24 @@ private function handleTask(array $task): void $this->loadQueueRetryConfig($job); + $taskName = request()->header('X-Cloudtasks-Taskname'); + $fullTaskName = $this->client->taskName( + $this->config['project'], + $this->config['location'], + $job->getQueue() ?: $this->config['queue'], + $taskName, + ); + + try { + $apiTask = CloudTasksApi::getTask($fullTaskName); + } catch (ApiException $e) { + if (in_array($e->getStatus(), ['NOT_FOUND', 'PRECONDITION_FAILED'])) { + abort(404); + } + + throw $e; + } + // If the task has a [X-CloudTasks-TaskRetryCount] header higher than 0, then // we know the job was created using an earlier version of the package. This // job does not have the attempts tracked internally yet. @@ -138,20 +157,7 @@ private function handleTask(array $task): void // max retry duration has been set. If that duration // has passed, it should stop trying altogether. if ($job->attempts() > 0) { - $taskName = request()->header('X-Cloudtasks-Taskname'); - - if (!is_string($taskName)) { - throw new UnexpectedValueException('Expected task name to be a string.'); - } - - $fullTaskName = $this->client->taskName( - $this->config['project'], - $this->config['location'], - $job->getQueue() ?: $this->config['queue'], - $taskName, - ); - - $job->setRetryUntil(CloudTasksApi::getRetryUntilTimestamp($fullTaskName)); + $job->setRetryUntil(CloudTasksApi::getRetryUntilTimestamp($apiTask)); } $job->setAttempts($job->attempts() + 1); diff --git a/tests/CloudTasksApiTest.php b/tests/CloudTasksApiTest.php index 19413f4..5b5a1c2 100644 --- a/tests/CloudTasksApiTest.php +++ b/tests/CloudTasksApiTest.php @@ -184,7 +184,7 @@ public function test_get_retry_until_timestamp() // The queue max retry duration is 5 seconds. The max retry until timestamp is calculated from the // first attempt, so we expect it to be [timestamp first attempt] + 5 seconds. $expected = $createdTask->getFirstAttempt()->getDispatchTime()->getSeconds() + 5; - $actual = CloudTasksApi::getRetryUntilTimestamp($createdTask->getName()); + $actual = CloudTasksApi::getRetryUntilTimestamp($createdTask); $this->assertSame($expected, $actual); } }