fix: announce only what concurrent override writes stored

CLAUDE.md

@@ -97,7 +97,12 @@ method name is fine, a rename supersedes nothing), this file does.
   review, an override closing the pending suggestions of its key, a retry closing an errored
   one — re-reads it in the database, rows locked (`lockStillPending()`,
   `lockPendingByKeys()`): the identity map does not refresh an entity loaded earlier
-  ([0013](docs/adr/0013-stale-entities-never-decide.md)).
+  ([0013](docs/adr/0013-stale-entities-never-decide.md)). The same goes for removing an
+  override (`OverrideWriter::remove()`, through `lockOneByKey()`): of two removals of one key,
+  the second finds nothing and announces nothing. `save()` re-reads an existing row the same
+  way, never an absent key: an InnoDB gap lock would make first writes of different keys wait
+  for each other. The batch writes (`writeMany()`) read without a lock, a limit
+  `docs/concepts/manual-override.md` states.
 - **A batch is decided against the state it leaves** (`OverrideReader::planChanges()`): an
   import or a batch approval that changes a parent (the global value, the parent language's)
   compares the children with the new value, not the stored one — and reads only the rows of

docs/concepts/manual-override.md

@@ -99,6 +99,22 @@ is corrupted (the first value is stored, the second write is refused), and repla
 updates the row. It takes a write race on a key that has no override yet, so it is left as a
 known limit rather than paid for with a platform-specific upsert on every save.
 
+A save never locks a key that has no row: on MySQL and MariaDB, a locked read of an absent key
+locks the gap where it would go, and two first writes of different keys would wait for each
+other. Only the row of an existing override is read locked, so that a save meeting a removal
+of the same key waits for it and stores its value in a new row.
+
+### Batch writes during a removal
+
+`saveMany()`, `saveAndRemoveMany()` and `removeMany()` (the import, a batch approval) read
+their rows without a lock, and `removeMany()` deletes the entities its caller loaded. Meeting
+a removal of one of their keys from another request, a batch can announce a value that is not
+stored (its `UPDATE` matched no row) or a removal another request already made: Doctrine does
+not check how many rows a write touched. The stored state stays right; only the events and
+cache invalidations of those keys are off. Locking every row of a batch would make concurrent
+imports wait for each other, or deadlock, so it is a known limit: replay the import, or save
+the entry again, if a webhook missed it.
+
 ## Living with overrides
 
 - the interactive console's "Browse and edit translations" entry inventories what is

docs/concepts/overrides.md

@@ -130,4 +130,9 @@ names it; an orphan catalogue is not judged.
   promotion between environments).
 - **Events**: every save/remove dispatches `OverrideSavedEvent` / `OverrideRemovedEvent`,
   the hook point for side effects the bundle cannot know about (HTTP cache purge, audit log,
-  webhook…).
+  webhook…). A removal reads the row locked, in its own transaction: two removals of the same
+  key at once (two admins resetting the same entry) delete one row and dispatch one event,
+  the second one finding nothing to remove. A save of an existing override reads its row
+  locked too: one meeting a removal waits for it, then stores its value in a new row, and
+  its event announces what is stored. Batch writes are not covered: see
+  [Batch writes during a removal](manual-override.md#batch-writes-during-a-removal).

src/Override/OverrideWriter.php

@@ -88,6 +88,14 @@ public function save(string $key, string $catalogue, string $locale, string $val
         return $this->withEffectsAfter(fn (): TranslationOverride => $this->suggestionRepository->transactional(function () use ($key, $catalogue, $locale, $value, $originalValue, $scope): TranslationOverride {
             $override = $this->repository->findOneByKey($key, $catalogue, $locale, $scope);
 
+            // Only a row that exists is read again locked: a removal holding it is waited for,
+            // and the value lands in a new row instead of an UPDATE matching nothing. A locked
+            // read of an absent key would take an InnoDB gap lock, and two first writes of
+            // different keys would block each other.
+            if (null !== $override) {
+                $override = $this->repository->lockOneByKey($key, $catalogue, $locale, $scope);
+            }
+
             if (null === $override) {
                 $override = new TranslationOverride($key, $catalogue, $locale, $scope);
                 $override->setOriginalValue($originalValue);
@@ -284,17 +292,27 @@ private function writeMany(array $entries, array $removals): array
         return [\count($pending), \count($removals)];
     }
 
+    /**
+     * Two removals of the same key at once (two admins resetting the same entry) delete
+     * one row: the row is read locked, so the second one waits for the first, finds
+     * nothing, and neither invalidates nor announces anything. Read without the lock,
+     * both found the row, and the host purged and notified twice for one removal.
+     */
     public function remove(string $key, string $catalogue, string $locale, string $scope = ''): void
     {
-        $override = $this->repository->findOneByKey($key, $catalogue, $locale, $scope);
+        $this->withEffectsAfter(fn () => $this->suggestionRepository->transactional(function () use ($key, $catalogue, $locale, $scope): void {
+            $override = $this->repository->lockOneByKey($key, $catalogue, $locale, $scope);
+
+            if (null === $override) {
+                return;
+            }
 
-        if (null !== $override) {
             $this->repository->remove($override);
             $this->applyEffect(function () use ($key, $catalogue, $locale, $scope): void {
                 $this->cacheManager->invalidate($locale);
                 $this->eventDispatcher->dispatch(new OverrideRemovedEvent($key, $catalogue, $locale, $scope));
             });
-        }
+        }));
     }
 
     /**

src/Repository/TranslationOverrideRepository.php

@@ -8,6 +8,7 @@
 use CylleneDigital\AiTranslationBundle\Locale\LocaleFallback;
 use CylleneDigital\AiTranslationBundle\Transfer\OverrideExportFilter;
 use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
+use Doctrine\DBAL\LockMode;
 use Doctrine\ORM\QueryBuilder;
 use Doctrine\Persistence\ManagerRegistry;
 
@@ -59,6 +60,32 @@ public function findOneByKey(string $key, string $catalogue, string $locale, str
         return $this->findOneBy(['key' => $key, 'catalogue' => $catalogue, 'locale' => $locale, 'scope' => $scope]);
     }
 
+    /**
+     * The override of this key IN DATABASE, its row locked until the end of the caller's
+     * transaction: a concurrent removal of the same key waits for this one, then finds
+     * nothing, instead of deleting the row a second time and announcing it twice. On a key
+     * with no row, InnoDB locks the gap where it would go, which blocks inserts of nearby
+     * keys until the commit: save() only calls it for a row it found.
+     */
+    public function lockOneByKey(string $key, string $catalogue, string $locale, string $scope = ''): ?TranslationOverride
+    {
+        /** @var TranslationOverride|null $override */
+        $override = $this->createQueryBuilder('t')
+            ->andWhere('t.key = :key')
+            ->andWhere('t.catalogue = :catalogue')
+            ->andWhere('t.locale = :locale')
+            ->andWhere('t.scope = :scope')
+            ->setParameter('key', $key)
+            ->setParameter('catalogue', $catalogue)
+            ->setParameter('locale', $locale)
+            ->setParameter('scope', $scope)
+            ->getQuery()
+            ->setLockMode(LockMode::PESSIMISTIC_WRITE)
+            ->getOneOrNullResult();
+
+        return $override;
+    }
+
     /**
      * The overrides of these catalogues in one (locale, scope): one query for a whole view,
      * filtered in SQL — a single catalogue costs what it always cost.

tests/Functional/ConcurrentOverrideWritesTest.php

@@ -0,0 +1,153 @@
+<?php
+
+declare(strict_types=1);
+
+namespace CylleneDigital\AiTranslationBundle\Tests\Functional;
+
+use CylleneDigital\AiTranslationBundle\Event\OverrideRemovedEvent;
+use CylleneDigital\AiTranslationBundle\Event\OverrideSavedEvent;
+use Doctrine\DBAL\Platforms\SQLitePlatform;
+use Symfony\Component\EventDispatcher\EventDispatcherInterface;
+
+/**
+ * A write of the bundle meeting another connection's write of the same table. The other
+ * connection is a second PHP process, which writes in a transaction left open for
+ * HOLD_MICROSECONDS while this one writes. Row locks only: SQLite locks the whole database.
+ */
+final class ConcurrentOverrideWritesTest extends DatabaseTestCase
+{
+    /** How long the other process keeps its statement uncommitted. */
+    private const int HOLD_MICROSECONDS = 1_000_000;
+
+    /** Below it, a write did not wait for the other process: the test would prove nothing. */
+    private const float WAITED_SECONDS = 0.2;
+
+    /** For the other process: app.two saved by the bundle's OverrideWriter::save(), not by SQL. */
+    private const string SAVE_TWO = 'save app.two';
+
+    private const string DELETE_RESET = "DELETE FROM cyllene_translation_override WHERE translation_key = 'app.reset'";
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        if ($this->entityManager()->getConnection()->getDatabasePlatform() instanceof SQLitePlatform) {
+            self::markTestSkipped('SQLite has no row lock: run it on MariaDB, MySQL or PostgreSQL (DATABASE_URL).');
+        }
+    }
+
+    /** Two admins reset the same entry at once: one row is deleted, so one removal is announced. */
+    public function testARemovalThatWaitedForAConcurrentOneAnnouncesNothing(): void
+    {
+        $this->writer()->save('app.reset', 'messages', 'fr', 'Réinitialiser');
+        $removed = 0;
+        $this->dispatcher()->addListener(OverrideRemovedEvent::class, static function () use (&$removed): void {
+            ++$removed;
+        });
+
+        $waited = $this->whileTheOtherProcessHolds(self::DELETE_RESET, fn () => $this->writer()->remove('app.reset', 'messages', 'fr'));
+
+        self::assertGreaterThan(self::WAITED_SECONDS, $waited, 'The removal did not wait for the other process.');
+        self::assertSame(0, $removed, 'The other process removed the row: this removal announces nothing.');
+        self::assertSame(0, $this->fetchInt('SELECT COUNT(*) FROM cyllene_translation_override'));
+    }
+
+    /** An admin edits an entry another one is resetting: the value announced is the one stored. */
+    public function testASaveThatWaitedForAConcurrentRemovalStoresItsValue(): void
+    {
+        $this->writer()->save('app.reset', 'messages', 'fr', 'Réinitialiser');
+        $saved = [];
+        $this->dispatcher()->addListener(OverrideSavedEvent::class, static function (OverrideSavedEvent $event) use (&$saved): void {
+            $saved[] = $event->override->getValue();
+        });
+
+        $waited = $this->whileTheOtherProcessHolds(self::DELETE_RESET, fn () => $this->writer()->save('app.reset', 'messages', 'fr', 'Remettre à zéro'));
+
+        self::assertGreaterThan(self::WAITED_SECONDS, $waited, 'The save did not wait for the other process.');
+        self::assertSame(['Remettre à zéro'], $saved);
+        self::assertSame(
+            'Remettre à zéro',
+            $this->entityManager()->getConnection()->fetchOne("SELECT value FROM cyllene_translation_override WHERE translation_key = 'app.reset'"),
+            'The UPDATE of the deleted row matched nothing: the value has to land in a new row.',
+        );
+    }
+
+    /**
+     * Two admins create overrides of different keys, both through save(): neither waits. A
+     * locked read of an absent key would take an InnoDB gap lock, and the second insert
+     * would wait for the first commit, or deadlock.
+     */
+    public function testTwoFirstWritesOfDifferentKeysDoNotWaitForEachOther(): void
+    {
+        $waited = $this->whileTheOtherProcessHolds(self::SAVE_TWO, fn () => $this->writer()->save('app.one', 'messages', 'fr', 'Un'));
+
+        self::assertLessThan(self::WAITED_SECONDS, $waited, 'The first write of app.one waited for the insert of app.two.');
+        self::assertSame(2, $this->fetchInt('SELECT COUNT(*) FROM cyllene_translation_override'));
+    }
+
+    /**
+     * @param string $statement SQL, or SAVE_TWO for the bundle's own save() of app.two
+     *
+     * @return float how long $write took, in seconds
+     */
+    private function whileTheOtherProcessHolds(string $statement, callable $write): float
+    {
+        $process = proc_open(
+            [\PHP_BINARY, '-r', self::OTHER_PROCESS],
+            [1 => ['pipe', 'w'], 2 => ['pipe', 'w']],
+            $pipes,
+            \dirname(__DIR__, 2),
+            // The whole environment: the kernel the other process boots reads DATABASE_URL.
+            [
+                ...getenv(),
+                'CONNECTION_PARAMS' => base64_encode(serialize($this->entityManager()->getConnection()->getParams())),
+                'STATEMENT' => $statement,
+                'HOLD' => (string) self::HOLD_MICROSECONDS,
+            ],
+        );
+        self::assertIsResource($process);
+
+        // The other process holds its rows from here until its commit. Its error output is
+        // only read on failure: reading it waits for the process to end.
+        if ("done\n" !== fgets($pipes[1])) {
+            self::fail((string) stream_get_contents($pipes[2]));
+        }
+
+        $start = hrtime(true);
+        $write();
+        $waited = (hrtime(true) - $start) / 1e9;
+
+        fclose($pipes[1]);
+        fclose($pipes[2]);
+        self::assertSame(0, proc_close($process));
+
+        return $waited;
+    }
+
+    private function dispatcher(): EventDispatcherInterface
+    {
+        $dispatcher = self::getContainer()->get('event_dispatcher');
+        self::assertInstanceOf(EventDispatcherInterface::class, $dispatcher);
+
+        return $dispatcher;
+    }
+
+    private const string OTHER_PROCESS = <<<'PHP'
+        require 'vendor/autoload.php';
+        if ('save app.two' === getenv('STATEMENT')) {
+            $kernel = new CylleneDigital\AiTranslationBundle\Tests\App\TestKernel('test', true);
+            $kernel->boot();
+            $container = $kernel->getContainer()->get('test.service_container');
+            $connection = $container->get('doctrine.dbal.default_connection');
+            $connection->beginTransaction();
+            $container->get(CylleneDigital\AiTranslationBundle\Override\OverrideWriter::class)->save('app.two', 'messages', 'fr', 'Deux');
+        } else {
+            $connection = Doctrine\DBAL\DriverManager::getConnection(unserialize(base64_decode((string) getenv('CONNECTION_PARAMS'))));
+            $connection->beginTransaction();
+            $connection->executeStatement((string) getenv('STATEMENT'));
+        }
+        fwrite(STDOUT, "done\n");
+        usleep((int) getenv('HOLD'));
+        $connection->commit();
+        PHP;
+}