diff --git a/CLAUDE.md b/CLAUDE.md index 9ce780e..0350de1 100644 --- a/CLAUDE.md +++ b/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 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 fd6c0b6..edee66f 100644 --- a/docs/concepts/overrides.md +++ b/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). diff --git a/src/Override/OverrideWriter.php b/src/Override/OverrideWriter.php index 7a700b7..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); @@ -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)); }); - } + })); } /** diff --git a/src/Repository/TranslationOverrideRepository.php b/src/Repository/TranslationOverrideRepository.php index 53ac653..6b5d22d 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,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. 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; +}