Create judge tasks for a judging only once.

unblockJudgeTasks() checks for judgings without judge tasks and creates
them, but two concurrent callers both pass the check. The second set fails
on judging_run's unique key with the "1062 Duplicate entry" from #2848 and
leaves judge tasks behind that no judging run points at. Lock the judging
and re-check inside.

Author: meisterT

webapp/src/Service/DOMJudgeService.php

@@ -62,6 +62,7 @@
 use Symfony\Component\Security\Core\Authentication\Token\UsernamePasswordToken;
 use Symfony\Component\Security\Core\User\UserInterface;
 use Symfony\Contracts\Cache\ItemInterface;
+use Throwable;
 use Twig\Attribute\AsTwigFilter;
 use Twig\Attribute\AsTwigFunction;
 use Twig\Environment;
@@ -1798,15 +1799,42 @@ private function actuallyCreateJudgetasks(int $priority, Judging $judging, int $
             $judgetaskInsertParamsWithoutColon[$key] = $param;
         }
 
-        $this->em->getConnection()->executeQuery($judgetaskInsertQuery, $judgetaskInsertParamsWithoutColon);
+        // Create the judge tasks exactly once: two concurrent unblockJudgeTasks() calls both
+        // see a judging without them, and a second set fails on judging_run's unique key. Lock
+        // the judging and re-check. The raw transaction API, because wrapInTransaction()
+        // flushes, which createRejudging() avoids for speed.
+        $connection = $this->em->getConnection();
+        $connection->beginTransaction();
+        try {
+            $connection->executeQuery(
+                'SELECT judgingid FROM judging WHERE judgingid = :judgingid FOR UPDATE',
+                ['judgingid' => $judging->getJudgingid()]
+            );
 
-        // Step 3: Insert the corresponding judging runs.
-        $this->em->getConnection()->executeQuery(
-            'INSERT INTO judging_run (judgingid, judgetaskid, testcaseid)
-                    SELECT :judgingid, judgetaskid, testcase_id FROM judgetask
-                    WHERE jobid = :judgingid ORDER BY judgetaskid',
-            ['judgingid' => $judging->getJudgingid()]
-        );
+            $alreadyCreated = (int)$connection->fetchOne(
+                'SELECT COUNT(*) FROM judgetask WHERE jobid = :judgingid',
+                ['judgingid' => $judging->getJudgingid()]
+            );
+            if ($alreadyCreated > 0) {
+                $connection->commit();
+                return;
+            }
+
+            $connection->executeQuery($judgetaskInsertQuery, $judgetaskInsertParamsWithoutColon);
+
+            // Step 3: Insert the corresponding judging runs.
+            $connection->executeQuery(
+                'INSERT INTO judging_run (judgingid, judgetaskid, testcaseid)
+                        SELECT :judgingid, judgetaskid, testcase_id FROM judgetask
+                        WHERE jobid = :judgingid ORDER BY judgetaskid',
+                ['judgingid' => $judging->getJudgingid()]
+            );
+
+            $connection->commit();
+        } catch (Throwable $e) {
+            $connection->rollBack();
+            throw $e;
+        }
     }
 
     public function shadowMode(): bool

webapp/tests/Unit/Service/DOMJudgeServiceTest.php

@@ -2,11 +2,22 @@
 
 namespace App\Tests\Unit\Service;
 
+use App\Entity\Contest;
+use App\Entity\JudgeTask;
+use App\Entity\Judging;
+use App\Entity\JudgingRun;
+use App\Entity\Problem;
+use App\Entity\Submission;
+use App\Entity\SubmissionSource;
+use App\Entity\Team;
 use App\Service\DOMJudgeService;
-use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
+use App\Service\SubmissionService;
+use App\Tests\Unit\BaseTestCase;
+use Doctrine\ORM\EntityManagerInterface;
 use Symfony\Component\Filesystem\Filesystem;
+use Symfony\Component\HttpFoundation\File\UploadedFile;
 
-class DOMJudgeServiceTest extends KernelTestCase
+class DOMJudgeServiceTest extends BaseTestCase
 {
     private const ASSET_PATH = 'test-assets';
 
@@ -15,7 +26,7 @@ class DOMJudgeServiceTest extends KernelTestCase
 
     protected function setUp(): void
     {
-        self::bootKernel();
+        parent::setUp();
         $this->dj       = static::getContainer()->get(DOMJudgeService::class);
         $this->assetDir = static::getContainer()->getParameter('kernel.project_dir') . '/public/' . self::ASSET_PATH;
         (new Filesystem())->mkdir($this->assetDir);
@@ -36,4 +47,65 @@ public function testGetAssetFilesMatchesOnTheExtensionOnly(): void
         self::assertSame(['also-ok.css', 'ok.css'], $this->dj->getAssetFiles(self::ASSET_PATH, ['css']));
         self::assertSame(['image.png', 'script.js'], $this->dj->getAssetFiles(self::ASSET_PATH, ['png', 'js']));
     }
+
+    private function em(): EntityManagerInterface
+    {
+        return static::getContainer()->get(EntityManagerInterface::class);
+    }
+
+    private function addSubmission(): Submission
+    {
+        $em = $this->em();
+
+        $message = null;
+        return static::getContainer()->get(SubmissionService::class)->submitSolution(
+            $em->getRepository(Team::class)->findOneBy(['name' => 'DOMjudge']),
+            null,
+            $em->getRepository(Problem::class)->findOneBy(['externalid' => 'hello']),
+            $em->getRepository(Contest::class)->findOneBy(['shortname' => 'demo']),
+            'c',
+            [new UploadedFile(__FILE__, 'foo.c', null, null, true)],
+            SubmissionSource::UNKNOWN, null, null, null, null, null, $message
+        );
+    }
+
+    /**
+     * Judge tasks are created for a judging exactly once.
+     *
+     * unblockJudgeTasks() looks for judgings that have no judge tasks yet, so two callers
+     * running at the same time - enabling a language and a problem, say - would both pass that
+     * check. Creating a second set would violate the unique key on judging_run and leave judge
+     * tasks behind that no judging run points at, which judgehosts then choke on.
+     */
+    public function testJudgeTasksAreCreatedOnlyOncePerJudging(): void
+    {
+        $this->logIn();
+
+        $submission = $this->addSubmission();
+        $judgingId = $submission->getJudgings()->first()->getJudgingid();
+
+        $em = $this->em();
+        $judgeTasksAfterFirst = (int)$em->getConnection()->fetchOne(
+            'SELECT COUNT(*) FROM judgetask WHERE jobid = ?',
+            [$judgingId]
+        );
+        self::assertGreaterThan(0, $judgeTasksAfterFirst, 'submitting must create judge tasks');
+
+        // A second attempt for the same judging must be a no-op rather than an error.
+        $judging = $em->getRepository(Judging::class)->find($judgingId);
+        $this->dj->maybeCreateJudgeTasks($judging);
+
+        self::assertSame(
+            $judgeTasksAfterFirst,
+            (int)$em->getConnection()->fetchOne('SELECT COUNT(*) FROM judgetask WHERE jobid = ?', [$judgingId]),
+            'no extra judge tasks may be created'
+        );
+
+        // Every judge task still has exactly one judging run, which is what judgehosts rely on.
+        $tasks = $em->getRepository(JudgeTask::class)->findBy(['jobid' => $judgingId]);
+        foreach ($tasks as $task) {
+            $runs = $em->getRepository(JudgingRun::class)->findBy(['judgetaskid' => $task->getJudgetaskid()]);
+            self::assertCount(1, $runs, 'each judge task needs exactly one judging run');
+        }
+    }
 }