Merge pull request #96 from stackkit/bugfix/95-job-deletion

#95 job deletion

Author: marickvantuil

.gitignore

@@ -2,3 +2,4 @@
 composer.lock
 .idea/
 .phpunit.result.cache
+.phpunit.cache

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
 {

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) {

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;
 }

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;
     }

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();

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);

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);
     }
 }

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());
     }
 }