From 44b5c5dbdd82ab902eb65b1033da39b4b4794454 Mon Sep 17 00:00:00 2001 From: Julien Dehais Date: Thu, 15 Oct 2026 13:19:41 +0200 Subject: [PATCH 1/2] fix: announce the removal of an override once when two requests remove it at once OverrideWriter::remove() read the override, then deleted it, without a lock. Two removals of the same key at once (two admins resetting the same entry) both found the row: the second DELETE matched nothing, but both invalidated the cache and dispatched OverrideRemovedEvent, so the host purged and notified twice for one removal. The row is now read locked (TranslationOverrideRepository::lockOneByKey()) in a transaction of the writer, and the effects follow its commit: the second removal waits for the first, finds nothing, and announces nothing. The signature of removeOverride() does not change. The test runs a second PHP process that deletes the row in a transaction left open while this one removes it; it fails on the previous code with MariaDB and PostgreSQL, and is skipped on SQLite, which has no row lock. --- CLAUDE.md | 4 +- docs/concepts/overrides.md | 4 +- src/Override/OverrideWriter.php | 16 ++++- .../TranslationOverrideRepository.php | 25 +++++++ .../ConcurrentOverrideRemovalTest.php | 69 +++++++++++++++++++ 5 files changed, 113 insertions(+), 5 deletions(-) create mode 100644 tests/Functional/ConcurrentOverrideRemovalTest.php diff --git a/CLAUDE.md b/CLAUDE.md index 9ce780e..4020b7c 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -97,7 +97,9 @@ 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. - **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 diff --git a/docs/concepts/overrides.md b/docs/concepts/overrides.md index fd6c0b6..9b99be2 100644 --- a/docs/concepts/overrides.md +++ b/docs/concepts/overrides.md @@ -130,4 +130,6 @@ 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. diff --git a/src/Override/OverrideWriter.php b/src/Override/OverrideWriter.php index 7a700b7..b3212d3 100644 --- a/src/Override/OverrideWriter.php +++ b/src/Override/OverrideWriter.php @@ -284,17 +284,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)); }); - } + })); } /** diff --git a/src/Repository/TranslationOverrideRepository.php b/src/Repository/TranslationOverrideRepository.php index 53ac653..8075ed9 100644 --- a/src/Repository/TranslationOverrideRepository.php +++ b/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,30 @@ 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. + */ + 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. diff --git a/tests/Functional/ConcurrentOverrideRemovalTest.php b/tests/Functional/ConcurrentOverrideRemovalTest.php new file mode 100644 index 0000000..b40efce --- /dev/null +++ b/tests/Functional/ConcurrentOverrideRemovalTest.php @@ -0,0 +1,69 @@ +entityManager()->getConnection(); + + if ($connection->getDatabasePlatform() instanceof SQLitePlatform) { + self::markTestSkipped('SQLite has no row lock: run it on MariaDB or PostgreSQL (DATABASE_URL).'); + } + + $this->writer()->save('app.reset', 'messages', 'fr', 'Réinitialiser'); + + $removed = 0; + $dispatcher = self::getContainer()->get('event_dispatcher'); + self::assertInstanceOf(EventDispatcherInterface::class, $dispatcher); + $dispatcher->addListener(OverrideRemovedEvent::class, static function () use (&$removed): void { + ++$removed; + }); + + $process = proc_open( + [\PHP_BINARY, '-r', self::OTHER_PROCESS], + [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], + $pipes, + \dirname(__DIR__, 2), + ['CONNECTION_PARAMS' => base64_encode(serialize($connection->getParams())), 'HOLD' => (string) self::HOLD_MICROSECONDS], + ); + self::assertIsResource($process); + + // The other process holds the row from here until its commit. Its error output is + // only read on failure: reading it waits for the process to end. + if ("deleted\n" !== fgets($pipes[1])) { + self::fail((string) stream_get_contents($pipes[2])); + } + + $this->writer()->remove('app.reset', 'messages', 'fr'); + + self::assertSame(0, proc_close($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')); + } + + private const string OTHER_PROCESS = <<<'PHP' + require 'vendor/autoload.php'; + $connection = Doctrine\DBAL\DriverManager::getConnection(unserialize(base64_decode((string) getenv('CONNECTION_PARAMS')))); + $connection->beginTransaction(); + $connection->executeStatement('DELETE FROM cyllene_translation_override WHERE translation_key = ?', ['app.reset']); + fwrite(STDOUT, "deleted\n"); + usleep((int) getenv('HOLD')); + $connection->commit(); + PHP; +} From 741bf0497a1df41f7ebadae52219229cb4c72a20 Mon Sep 17 00:00:00 2001 From: Julien Dehais Date: Thu, 15 Oct 2026 15:40:40 +0200 Subject: [PATCH 2/2] fix: announce the value a save stored when it meets a removal of the same key OverrideWriter::save() read the override without a lock. A save meeting a removal of the same key found the row, its UPDATE waited for the removal and matched nothing, and OverrideSavedEvent announced a value that was not stored: Doctrine does not check how many rows a write touched. A row the save finds is now read again locked (lockOneByKey()): the save waits for the removal, finds nothing, and stores its value in a new row. An absent key is never read locked: on MySQL and MariaDB that takes a gap lock, and two first writes of different keys would wait for each other, or deadlock. The race of record 0016 is unchanged. The batch writes (saveMany(), saveAndRemoveMany(), removeMany()) still read without a lock: locking every row of an import would make concurrent imports wait for each other. The limit is documented in manual-override.md. The concurrency test now also covers a save during a removal (fails on the previous code) and two first writes of different keys through save() in two processes (fails on MariaDB when the absent key is read locked). Each test checks how long the write took, so a slow runner fails it instead of passing it without proving anything, and the pipes are closed before proc_close(). --- CLAUDE.md | 5 +- docs/concepts/manual-override.md | 16 ++ docs/concepts/overrides.md | 5 +- src/Override/OverrideWriter.php | 8 + .../TranslationOverrideRepository.php | 4 +- .../ConcurrentOverrideRemovalTest.php | 69 -------- .../ConcurrentOverrideWritesTest.php | 153 ++++++++++++++++++ 7 files changed, 188 insertions(+), 72 deletions(-) delete mode 100644 tests/Functional/ConcurrentOverrideRemovalTest.php create mode 100644 tests/Functional/ConcurrentOverrideWritesTest.php diff --git a/CLAUDE.md b/CLAUDE.md index 4020b7c..0350de1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -99,7 +99,10 @@ method name is fine, a rename supersedes nothing), this file does. `lockPendingByKeys()`): the identity map does not refresh an entity loaded earlier ([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. + 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 diff --git a/docs/concepts/manual-override.md b/docs/concepts/manual-override.md index 89f43d3..68f8f9d 100644 --- a/docs/concepts/manual-override.md +++ b/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 diff --git a/docs/concepts/overrides.md b/docs/concepts/overrides.md index 9b99be2..edee66f 100644 --- a/docs/concepts/overrides.md +++ b/docs/concepts/overrides.md @@ -132,4 +132,7 @@ names it; an orphan catalogue is not judged. the hook point for side effects the bundle cannot know about (HTTP cache purge, audit log, 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. + 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). diff --git a/src/Override/OverrideWriter.php b/src/Override/OverrideWriter.php index b3212d3..3e69ef1 100644 --- a/src/Override/OverrideWriter.php +++ b/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); diff --git a/src/Repository/TranslationOverrideRepository.php b/src/Repository/TranslationOverrideRepository.php index 8075ed9..6b5d22d 100644 --- a/src/Repository/TranslationOverrideRepository.php +++ b/src/Repository/TranslationOverrideRepository.php @@ -63,7 +63,9 @@ public function findOneByKey(string $key, string $catalogue, string $locale, str /** * 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. + * 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 { diff --git a/tests/Functional/ConcurrentOverrideRemovalTest.php b/tests/Functional/ConcurrentOverrideRemovalTest.php deleted file mode 100644 index b40efce..0000000 --- a/tests/Functional/ConcurrentOverrideRemovalTest.php +++ /dev/null @@ -1,69 +0,0 @@ -entityManager()->getConnection(); - - if ($connection->getDatabasePlatform() instanceof SQLitePlatform) { - self::markTestSkipped('SQLite has no row lock: run it on MariaDB or PostgreSQL (DATABASE_URL).'); - } - - $this->writer()->save('app.reset', 'messages', 'fr', 'Réinitialiser'); - - $removed = 0; - $dispatcher = self::getContainer()->get('event_dispatcher'); - self::assertInstanceOf(EventDispatcherInterface::class, $dispatcher); - $dispatcher->addListener(OverrideRemovedEvent::class, static function () use (&$removed): void { - ++$removed; - }); - - $process = proc_open( - [\PHP_BINARY, '-r', self::OTHER_PROCESS], - [1 => ['pipe', 'w'], 2 => ['pipe', 'w']], - $pipes, - \dirname(__DIR__, 2), - ['CONNECTION_PARAMS' => base64_encode(serialize($connection->getParams())), 'HOLD' => (string) self::HOLD_MICROSECONDS], - ); - self::assertIsResource($process); - - // The other process holds the row from here until its commit. Its error output is - // only read on failure: reading it waits for the process to end. - if ("deleted\n" !== fgets($pipes[1])) { - self::fail((string) stream_get_contents($pipes[2])); - } - - $this->writer()->remove('app.reset', 'messages', 'fr'); - - self::assertSame(0, proc_close($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')); - } - - private const string OTHER_PROCESS = <<<'PHP' - require 'vendor/autoload.php'; - $connection = Doctrine\DBAL\DriverManager::getConnection(unserialize(base64_decode((string) getenv('CONNECTION_PARAMS')))); - $connection->beginTransaction(); - $connection->executeStatement('DELETE FROM cyllene_translation_override WHERE translation_key = ?', ['app.reset']); - fwrite(STDOUT, "deleted\n"); - usleep((int) getenv('HOLD')); - $connection->commit(); - PHP; -} diff --git a/tests/Functional/ConcurrentOverrideWritesTest.php b/tests/Functional/ConcurrentOverrideWritesTest.php new file mode 100644 index 0000000..2c449e3 --- /dev/null +++ b/tests/Functional/ConcurrentOverrideWritesTest.php @@ -0,0 +1,153 @@ +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; +}