From 6edc6c324c298e12f4c5464c15a4d2d428aa5353 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Paul=20L=C3=BCtzner?= Date: Thu, 24 Sep 2026 16:45:47 +0200 Subject: [PATCH] fix(deploy): the cache table exists before any worker reads it A fresh install logged a warning about once a second from the worker, until an administrator happened to press "Restart workers": Failed to fetch key "workers.restart_requested_timestamp": ... relation "cache_items" does not exist The restart signal and the notice throttle live in cache pools on Symfony's DBAL adapter, which creates cache_items on its first write. Every Messenger consumer reads the signal on every loop, long before anything writes it, and each failed read is logged. The migrations leave the table out on purpose, because the adapter owns its DDL. app:db:migrate now has each DBAL adapter create its table when it is missing, after the migrations and inside the advisory lock, so php and the worker cannot race each other to it. The adapter still owns the DDL: it names its table through configureSchema() and creates it with createTable(). Best effort, like the statement statistics: a failure is a warning, not a container that will not start. Existing installs get the table on their next boot. The adapters are collected by class in a compiler pass rather than injected by pool id. In dev and test the profiler wraps every pool in a TraceableAdapter, which has no createTable(). It is the lookup DoctrineBundle uses for its schema listener, and it covers any DBAL pool added later. Checked on two fresh compose.yaml + compose.demo.yaml stacks: 95 warnings in 75 seconds without this, none with it, and Restart workers still reaches every consumer through the table created at boot. Co-Authored-By: Claude Opus 5.5 --- config/services.yaml | 6 ++ migrations/Version20260731222453.php | 4 ++ src/Command/Setup/MigrateCommand.php | 38 +++++++++- .../Setup/DatabaseCachePoolsPass.php | 48 +++++++++++++ .../Setup/DatabaseCacheTables.php | 70 +++++++++++++++++++ src/Kernel.php | 14 ++++ .../Monitoring/PostgresStatusRepository.php | 14 ++++ tests/Command/MigrateCommandTest.php | 25 +++++++ 8 files changed, 217 insertions(+), 2 deletions(-) create mode 100644 src/Infrastructure/Setup/DatabaseCachePoolsPass.php create mode 100644 src/Infrastructure/Setup/DatabaseCacheTables.php 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