From 42a8c9ae71efdd932488f5d12bcb4fb9ef67f2c1 Mon Sep 17 00:00:00 2001 From: Martin Zurowietz Date: Wed, 27 Nov 2024 09:56:41 +0100 Subject: [PATCH] Make ApplyLargoSession job more robust against errors If a job failed for some other reason than an exception during saving, the largo_job_id was not cleaned up and no error was shown. This is handled more robustly now. Also we need a mechanism to prevent re-running of the failed job. Otherwise unexpected changes could be applied. The exprected behavior is now that the user is shown the error message and they try to save the session again. The old job should be discarded then. --- src/Jobs/ApplyLargoSession.php | 51 +++++++++++++++-------- tests/Jobs/ApplyLargoSessionTest.php | 61 +++++++++++++++++----------- 2 files changed, 73 insertions(+), 39 deletions(-) diff --git a/src/Jobs/ApplyLargoSession.php b/src/Jobs/ApplyLargoSession.php index 1a3c21f..1de04ce 100644 --- a/src/Jobs/ApplyLargoSession.php +++ b/src/Jobs/ApplyLargoSession.php @@ -20,6 +20,7 @@ use DB; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Queue\InteractsWithQueue; +use Throwable; class ApplyLargoSession extends Job implements ShouldQueue { @@ -120,23 +121,41 @@ public function __construct($id, User $user, $dismissedImageAnnotations, $change */ public function handle() { - try { - DB::transaction(function () { - $this->handleImageAnnotations(); - $this->handleVideoAnnotations(); - }); - - LargoSessionSaved::dispatch($this->id, $this->user); - } catch (\Exception $e) { - LargoSessionFailed::dispatch($this->id, $this->user); - } finally { - Volume::where('attrs->largo_job_id', $this->id)->each(function ($volume) { - $attrs = $volume->attrs; - unset($attrs['largo_job_id']); - $volume->attrs = $attrs; - $volume->save(); - }); + if (!Volume::where('attrs->largo_job_id', $this->id)->exists()) { + // The job previously exited or failed. Don't run it twice. + // This can happen if the admin reruns failed jobs. + return; } + + DB::transaction(function () { + $this->handleImageAnnotations(); + $this->handleVideoAnnotations(); + }); + + $this->cleanupJobId(); + LargoSessionSaved::dispatch($this->id, $this->user); + } + + /** + * Handle a job failure. + */ + public function failed(?Throwable $exception): void + { + $this->cleanupJobId(); + LargoSessionFailed::dispatch($this->id, $this->user); + } + + /** + * Remove the properties that indicate that a save Largo session is in progress. + */ + protected function cleanupJobId(): void + { + Volume::where('attrs->largo_job_id', $this->id)->each(function ($volume) { + $attrs = $volume->attrs; + unset($attrs['largo_job_id']); + $volume->attrs = $attrs; + $volume->save(); + }); } /** diff --git a/tests/Jobs/ApplyLargoSessionTest.php b/tests/Jobs/ApplyLargoSessionTest.php index 9711813..2a8bb98 100644 --- a/tests/Jobs/ApplyLargoSessionTest.php +++ b/tests/Jobs/ApplyLargoSessionTest.php @@ -17,6 +17,7 @@ use Biigle\Tests\VideoAnnotationLabelTest; use Biigle\Tests\VideoAnnotationTest; use Biigle\Tests\VideoTest; +use Biigle\VolumeFile; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Queue; use TestCase; @@ -27,6 +28,7 @@ public function testChangedAlreadyExistsImageAnnotations() { $user = UserTest::create(); $image = ImageTest::create(); + $this->setJobId($image, 'job_id'); $a1 = ImageAnnotationTest::create(['image_id' => $image->id]); $l1 = LabelTest::create(); @@ -57,6 +59,7 @@ public function testChangedAlreadyExistsVideoAnnotations() { $user = UserTest::create(); $video = VideoTest::create(); + $this->setJobId($video, 'job_id'); $a1 = VideoAnnotationTest::create(['video_id' => $video->id]); $l1 = LabelTest::create(); @@ -87,6 +90,7 @@ public function testChangedDuplicateImageAnnotations() { $user = UserTest::create(); $image = ImageTest::create(); + $this->setJobId($image, 'job_id'); $a1 = ImageAnnotationTest::create(['image_id' => $image->id]); $l1 = LabelTest::create(); $al1 = ImageAnnotationLabelTest::create([ @@ -112,6 +116,7 @@ public function testChangedDuplicateVideoAnnotations() { $user = UserTest::create(); $video = VideoTest::create(); + $this->setJobId($video, 'job_id'); $a1 = VideoAnnotationTest::create(['video_id' => $video->id]); $l1 = LabelTest::create(); $al1 = VideoAnnotationLabelTest::create([ @@ -137,6 +142,7 @@ public function testAnnotationMeanwhileDeletedImageAnnotations() { $user = UserTest::create(); $image = ImageTest::create(); + $this->setJobId($image, 'job_id'); $a1 = ImageAnnotationTest::create(['image_id' => $image->id]); $a2 = ImageAnnotationTest::create(['image_id' => $image->id]); @@ -169,6 +175,7 @@ public function testAnnotationMeanwhileDeletedVideoAnnotations() { $user = UserTest::create(); $video = VideoTest::create(); + $this->setJobId($video, 'job_id'); $a1 = VideoAnnotationTest::create(['video_id' => $video->id]); $a2 = VideoAnnotationTest::create(['video_id' => $video->id]); @@ -201,6 +208,7 @@ public function testLabelMeanwhileDeletedImageAnnotations() { $user = UserTest::create(); $image = ImageTest::create(); + $this->setJobId($image, 'job_id'); $a1 = ImageAnnotationTest::create(['image_id' => $image->id]); $a2 = ImageAnnotationTest::create(['image_id' => $image->id]); @@ -235,6 +243,7 @@ public function testLabelMeanwhileDeletedVideoAnnotations() { $user = UserTest::create(); $video = VideoTest::create(); + $this->setJobId($video, 'job_id'); $a1 = VideoAnnotationTest::create(['video_id' => $video->id]); $a2 = VideoAnnotationTest::create(['video_id' => $video->id]); @@ -269,6 +278,7 @@ public function testDismissImageAnnotations() { $user = UserTest::create(); $al1 = ImageAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->image, 'job_id'); $dismissed = [$al1->label_id => [$al1->annotation_id]]; $job = new ApplyLargoSession('job_id', $user, $dismissed, [], [], [], false); @@ -284,6 +294,7 @@ public function testDismissVideoAnnotations() { $user = UserTest::create(); $al1 = VideoAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->video, 'job_id'); $dismissed = [$al1->label_id => [$al1->annotation_id]]; $job = new ApplyLargoSession('job_id', $user, [], [], $dismissed, [], false); @@ -299,6 +310,7 @@ public function testDismissForceImageAnnotations() { $user = UserTest::create(); $al1 = ImageAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->image, 'job_id'); $user2 = UserTest::create(); $dismissed = [$al1->label_id => [$al1->annotation_id]]; @@ -314,6 +326,7 @@ public function testDismissForceVideoAnnotations() { $user = UserTest::create(); $al1 = VideoAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->video, 'job_id'); $user2 = UserTest::create(); $dismissed = [$al1->label_id => [$al1->annotation_id]]; @@ -329,6 +342,7 @@ public function testChangeOwnImageAnnotations() { $user = UserTest::create(); $al1 = ImageAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->image, 'job_id'); $annotation = $al1->annotation; $l1 = LabelTest::create(); @@ -347,6 +361,7 @@ public function testChangeOwnVideoAnnotations() { $user = UserTest::create(); $al1 = VideoAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->video, 'job_id'); $annotation = $al1->annotation; $l1 = LabelTest::create(); @@ -365,6 +380,7 @@ public function testChangeOtherImageAnnotations() { $user = UserTest::create(); $al1 = ImageAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->image, 'job_id'); $annotation = $al1->annotation; $l1 = LabelTest::create(); @@ -386,6 +402,7 @@ public function testChangeOtherVideoAnnotations() { $user = UserTest::create(); $al1 = VideoAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->video, 'job_id'); $annotation = $al1->annotation; $l1 = LabelTest::create(); @@ -407,6 +424,7 @@ public function testChangeOtherForceImageAnnotations() { $user = UserTest::create(); $al1 = ImageAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->image, 'job_id'); $annotation = $al1->annotation; $l1 = LabelTest::create(); @@ -426,6 +444,7 @@ public function testChangeOtherForceVideoAnnotations() { $user = UserTest::create(); $al1 = VideoAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->video, 'job_id'); $annotation = $al1->annotation; $l1 = LabelTest::create(); @@ -445,6 +464,7 @@ public function testChangeMultipleImageAnnotations() { $user = UserTest::create(); $al1 = ImageAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->image, 'job_id'); $annotation = $al1->annotation; $al2 = ImageAnnotationLabelTest::create([ 'user_id' => $user->id, @@ -478,6 +498,7 @@ public function testChangeMultipleVideoAnnotations() { $user = UserTest::create(); $al1 = VideoAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->video, 'job_id'); $annotation = $al1->annotation; $al2 = VideoAnnotationLabelTest::create([ 'user_id' => $user->id, @@ -555,12 +576,8 @@ public function testRemoveJobIdOnErrorImageAnnotations() $dismissed = [$al1->label_id => [$al1->annotation_id]]; $changed = [$l1->id => [$al1->annotation_id]]; - $job = new ApplyLargoSessionStub('job_id', $user, $dismissed, $changed, [], [], false); - try { - $job->handle(); - } catch (\Exception $e) { - // ignore - } + $job = new ApplyLargoSession('job_id', $user, $dismissed, $changed, [], [], false); + $job->failed(null); $this->assertEmpty($volume->fresh()->attrs); } @@ -577,12 +594,8 @@ public function testRemoveJobIdOnErrorVideoAnnotations() $dismissed = [$al1->label_id => [$al1->annotation_id]]; $changed = [$l1->id => [$al1->annotation_id]]; - $job = new ApplyLargoSessionStub('job_id', $user, [], [], $dismissed, $changed, false); - try { - $job->handle(); - } catch (\Exception $e) { - // ignore - } + $job = new ApplyLargoSession('job_id', $user, [], [], $dismissed, $changed, false); + $job->failed(null); $this->assertEmpty($volume->fresh()->attrs); } @@ -592,6 +605,7 @@ public function testDispatchEventOnSuccess() Event::fake(); $user = UserTest::create(); $al1 = ImageAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->image, 'job_id'); $l1 = LabelTest::create(); $dismissed = [$al1->label_id => [$al1->annotation_id]]; @@ -611,16 +625,13 @@ public function testDispatchEventOnError() Event::fake(); $user = UserTest::create(); $al1 = ImageAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->image, 'job_id'); $l1 = LabelTest::create(); $dismissed = [$al1->label_id => [$al1->annotation_id]]; $changed = [$l1->id => [$al1->annotation_id]]; - $job = new ApplyLargoSessionStub('job_id', $user, $dismissed, $changed, [], [], false); - try { - $job->handle(); - } catch (\Exception $e) { - // ignore - } + $job = new ApplyLargoSession('job_id', $user, $dismissed, $changed, [], [], false); + $job->failed(null); Event::assertDispatched(function (LargoSessionFailed $event) use ($user) { $this->assertSame($user->id, $event->user->id); @@ -633,6 +644,7 @@ public function testChangeImageAnnotationCopyFeatureVector() { $user = UserTest::create(); $al1 = ImageAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->image, 'job_id'); $vector1 = ImageAnnotationLabelFeatureVector::factory()->create([ 'id' => $al1->id, 'annotation_id' => $al1->annotation_id, @@ -655,6 +667,7 @@ public function testChangeVideoAnnotationCopyFeatureVector() { $user = UserTest::create(); $al1 = VideoAnnotationLabelTest::create(['user_id' => $user->id]); + $this->setJobId($al1->annotation->video, 'job_id'); $vector1 = VideoAnnotationLabelFeatureVector::factory()->create([ 'id' => $al1->id, 'annotation_id' => $al1->annotation_id, @@ -677,6 +690,7 @@ public function testDismissButChangeBackToSameLabelImage() { $user = UserTest::create(); $image = ImageTest::create(); + $this->setJobId($image, 'job_id'); $a1 = ImageAnnotationTest::create(['image_id' => $image->id]); $al1 = ImageAnnotationLabelTest::create([ 'annotation_id' => $a1->id, @@ -703,6 +717,7 @@ public function testDismissButChangeBackToSameLabelVideo() { $user = UserTest::create(); $video = VideoTest::create(); + $this->setJobId($video, 'job_id'); $a1 = VideoAnnotationTest::create(['video_id' => $video->id]); $al1 = VideoAnnotationLabelTest::create([ 'annotation_id' => $a1->id, @@ -724,12 +739,12 @@ public function testDismissButChangeBackToSameLabelVideo() $this->assertNotNull($a1->fresh()); $this->assertNotNull($a2->fresh()); } -} -class ApplyLargoSessionStub extends ApplyLargoSession -{ - protected function ignoreDeletedLabels($dismissed, $changed) + protected function setJobId(VolumeFile $file, string $id): void { - throw new \Exception; + $attrs = $file->volume->attrs ?? []; + $attrs['largo_job_id'] = $id; + $file->volume->attrs = $attrs; + $file->volume->save(); } }