diff --git a/config/services.yaml b/config/services.yaml index 73cf333c..ae701873 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -110,6 +110,12 @@ services: App\Infrastructure\Encryption\Encryptor: public: true + # Filled in by DatabaseCachePoolsPass. Autowiring runs before the profiler + # wraps the cache pools, too early to tell an adapter from its wrapper. + App\Infrastructure\Setup\DatabaseCacheTables: + arguments: + $pools: !abstract 'every DoctrineDbalAdapter in the container' + # The policy is enforced in production and reported under debug, because the # profiler toolbar and VarDumper inject inline scripts nothing can nonce. App\Security\Csp\ContentSecurityPolicyListener: diff --git a/migrations/Version20260731222453.php b/migrations/Version20260731222453.php index 6dec8c21..ca76b9e1 100644 --- a/migrations/Version20260731222453.php +++ b/migrations/Version20260731222453.php @@ -33,6 +33,10 @@ use Doctrine\Migrations\AbstractMigration; * * cache_items is deliberately absent despite showing up in the diff: the DBAL * cache adapter creates that table itself on first write and owns its DDL. + * First write is not soon enough, though — every consumer reads the + * restart-workers signal long before anything writes it, and each failed read + * is logged — so app:db:migrate has the adapter create it at boot. See + * App\Infrastructure\Setup\DatabaseCacheTables. */ final class Version20260731222453 extends AbstractMigration { diff --git a/src/Command/Setup/MigrateCommand.php b/src/Command/Setup/MigrateCommand.php index b90e7766..f0f81b5a 100644 --- a/src/Command/Setup/MigrateCommand.php +++ b/src/Command/Setup/MigrateCommand.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace App\Command\Setup; +use App\Infrastructure\Setup\DatabaseCacheTables; use App\Repository\Monitoring\PostgresStatusRepository; use Doctrine\DBAL\Connection; use Doctrine\DBAL\Platforms\PostgreSQLPlatform; @@ -88,6 +89,7 @@ final class MigrateCommand extends Command public function __construct( private readonly Connection $connection, private readonly PostgresStatusRepository $statistics, + private readonly DatabaseCacheTables $cacheTables, ) { parent::__construct(); } @@ -114,7 +116,7 @@ final class MigrateCommand extends Command // migrate unprotected, which is exactly what happened before this // command existed. if (!$this->connection->getDatabasePlatform() instanceof PostgreSQLPlatform) { - return $this->runMigrations($output); + return $this->migrate($io, $output); } $timeout = max(0, (int) $input->getOption('lock-timeout')); @@ -132,7 +134,7 @@ final class MigrateCommand extends Command } try { - $result = $this->runMigrations($output); + $result = $this->migrate($io, $output); // Under the same lock, so only the container that migrated tries it // rather than all four racing a CREATE EXTENSION. Best effort by @@ -148,6 +150,38 @@ final class MigrateCommand extends Command } } + /** + * The migrations, then the cache tables they deliberately leave out — see + * DatabaseCacheTables for why those cannot wait for their first write. + * + * Here because this is the step every container finishes before it starts + * anything that reads them, and, on Postgres, inside the lock, so php and + * the worker booting together cannot both find the table missing and race + * to create it. + * + * Best effort, like the statement statistics: the adapter still creates its + * table on its first write, so a failure here costs log noise until then, + * which is no reason to keep a container from starting. + */ + private function migrate(SymfonyStyle $io, OutputInterface $output): int + { + $result = $this->runMigrations($output); + + if (Command::SUCCESS !== $result) { + return $result; + } + + try { + foreach ($this->cacheTables->createMissing() as $table) { + $io->text(sprintf('Created the cache table %s.', $table)); + } + } catch (\Throwable $error) { + $io->warning(sprintf('Could not create the cache tables: %s', $error->getMessage())); + } + + return $result; + } + /** * The real command, with the flags the entrypoint used to pass it. * diff --git a/src/Infrastructure/Setup/DatabaseCachePoolsPass.php b/src/Infrastructure/Setup/DatabaseCachePoolsPass.php new file mode 100644 index 00000000..b6afb349 --- /dev/null +++ b/src/Infrastructure/Setup/DatabaseCachePoolsPass.php @@ -0,0 +1,48 @@ +.recorder_inner`, so `cache.messenger.restart_workers_signal` + * would hand over a wrapper that has no createTable(). By class, the adapters + * are found wherever they ended up — which is how DoctrineBundle finds them + * for the schema listener that puts cache_items into the migrations diff — and + * a pool added to cache.yaml later is covered without anyone remembering this. + * + * Only correct once the profiler has done its wrapping; Kernel::build() is + * what orders it after that. + */ +final class DatabaseCachePoolsPass implements CompilerPassInterface +{ + public function process(ContainerBuilder $container): void + { + $pools = []; + + foreach ($container->getDefinitions() as $id => $definition) { + if ($definition->isAbstract() || $definition->isSynthetic()) { + continue; + } + + if (DoctrineDbalAdapter::class === $definition->getClass()) { + $pools[] = new Reference($id); + } + } + + // Argument 0 is $pools, left abstract in services.yaml so that a + // container this pass never ran on fails loudly instead of quietly + // creating nothing. + $container->getDefinition(DatabaseCacheTables::class)->replaceArgument(0, new IteratorArgument($pools)); + } +} diff --git a/src/Infrastructure/Setup/DatabaseCacheTables.php b/src/Infrastructure/Setup/DatabaseCacheTables.php new file mode 100644 index 00000000..b290a00b --- /dev/null +++ b/src/Infrastructure/Setup/DatabaseCacheTables.php @@ -0,0 +1,70 @@ + $pools every one the container defines; see DatabaseCachePoolsPass + */ + public function __construct( + private iterable $pools, + private Connection $connection, + private PostgresStatusRepository $database, + ) { + } + + /** + * @return list the tables this call created; empty when they all existed already + */ + public function createMissing(): array + { + $created = []; + + foreach ($this->pools as $pool) { + // The adapter is asked for its table rather than the name being + // assumed here, so its options stay the one place that decides it. + // A pool on another connection answers with nothing: its table is + // not in this database, and looking for it here would answer the + // wrong question. + $wanted = $pool->configureSchema(new Schema(), $this->connection, static fn (): bool => false); + + foreach ($wanted->getTables() as $table) { + $name = $table->getObjectName()->toString(); + + if ($this->database->hasTable($name)) { + continue; + } + + $pool->createTable(); + $created[] = $name; + } + } + + return $created; + } +} diff --git a/src/Kernel.php b/src/Kernel.php index 80431c02..425e61cf 100644 --- a/src/Kernel.php +++ b/src/Kernel.php @@ -4,13 +4,27 @@ namespace App; use App\Infrastructure\Doctrine\Type\EncryptedStringType; use App\Infrastructure\Encryption\Encryptor; +use App\Infrastructure\Setup\DatabaseCachePoolsPass; use Symfony\Bundle\FrameworkBundle\Kernel\MicroKernelTrait; +use Symfony\Component\DependencyInjection\Compiler\PassConfig; +use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\HttpKernel\Kernel as BaseKernel; class Kernel extends BaseKernel { use MicroKernelTrait; + /** + * Late on purpose: DatabaseCachePoolsPass has to see the cache pools after + * the profiler has wrapped them (before-removing, priority 0), or it would + * take a wrapper for an adapter. -10 is the slot DoctrineBundle's own pass + * over the same adapters takes, for the same reason. + */ + protected function build(ContainerBuilder $container): void + { + $container->addCompilerPass(new DatabaseCachePoolsPass(), PassConfig::TYPE_BEFORE_REMOVING, -10); + } + /** * Doctrine builds its types through a static registry with no access to * the container, so EncryptedStringType is handed its Encryptor here — diff --git a/src/Repository/Monitoring/PostgresStatusRepository.php b/src/Repository/Monitoring/PostgresStatusRepository.php index 0a1872a0..7fe7a218 100644 --- a/src/Repository/Monitoring/PostgresStatusRepository.php +++ b/src/Repository/Monitoring/PostgresStatusRepository.php @@ -120,6 +120,20 @@ final readonly class PostgresStatusRepository return $this->hasStatStatements(); } + /** + * Whether a table exists, for the tables that are not the migrations' to + * create. False when the catalogue cannot be read: the caller then tries to + * create the table, and that attempt reports the real problem. + */ + public function hasTable(string $name): bool + { + try { + return $this->connection->createSchemaManager()->tableExists($name); + } catch (\Throwable) { + return false; + } + } + /** * Statements whose *average* is slow, worst first — "which queries are * slow". Floored, because a statement that averages a fraction of a diff --git a/tests/Command/MigrateCommandTest.php b/tests/Command/MigrateCommandTest.php index eccc1375..ec11438e 100644 --- a/tests/Command/MigrateCommandTest.php +++ b/tests/Command/MigrateCommandTest.php @@ -29,6 +29,9 @@ use Symfony\Component\Console\Tester\CommandTester; * container that cannot get it refuses to migrate rather than pressing on, and * the lock is handed back afterwards. If any of those three regress, boot goes * back to being a race. + * + * Also the cache table the migrations leave out, which the command creates + * after them; see DatabaseCacheTables. */ final class MigrateCommandTest extends KernelTestCase { @@ -122,6 +125,28 @@ final class MigrateCommandTest extends KernelTestCase $this->otherContainer->executeQuery('SELECT pg_advisory_unlock(?)', [$this->lockKey()]); } + public function testTheCacheTableExistsBeforeAnyConsumerReadsTheRestartSignal(): void + { + $schema = $this->connection->createSchemaManager(); + + // A fresh install: the migrations leave cache_items to the adapter, and + // nothing has written to a pool yet. + if ($schema->tableExists('cache_items')) { + $schema->dropTable('cache_items'); + } + + self::assertSame(Command::SUCCESS, $this->command->execute([])); + self::assertTrue( + $schema->tableExists('cache_items'), + 'Without it every consumer logs a failed read of the restart signal once a second.', + ); + self::assertStringContainsString('cache_items', $this->command->getDisplay()); + + // Every later boot finds it there and leaves it alone. + self::assertSame(Command::SUCCESS, $this->command->execute([])); + self::assertStringNotContainsString('cache_items', $this->command->getDisplay()); + } + /** * A second connection to the same database, which is what makes these * assertions mean anything: the lock is scoped to a session, and the