mirror of
https://github.com/karatektus/pl_mail.git
synced 2026-10-25 10:04:18 +00:00
Stop four containers racing each other to run the same migration
A deploy carrying a migration brought up one service out of four. php,
imap-supervisor, messenger-worker and scheduler are the same image running the
same entrypoint, they start within milliseconds of each other against one
database, and every one of them migrates on boot. All four read the ledger
before any of them writes to it, so all four decide the same migration is
pending; one wins and the rest die on
SQLSTATE[42701]: Duplicate column: 7
ERROR: column "timezone" of relation "user" already exists
which under `set -e` is a container that never comes up. The timezone column in
v0.0.6 is what exposed it, not what caused it -- any migration would have done,
and `--all-or-nothing` makes it likelier rather than safer, because the losers
are held for as long as the whole batch takes instead of one statement.
Reproduced before it was fixed: four concurrent migrates against one database,
three exited 7 with that message verbatim. The same four under the lock all exit
0, one migrates and three find the ledger current.
The losers still run the migration rather than skipping it. Skipping would mean
a container coming up against a schema nobody in that process verified, which is
the failure this is supposed to prevent, one step later.
The lock cannot be a line of shell, which is the whole reason there is a command
here. `pg_advisory_lock()` is scoped to the session, so taking it with
`dbal:run-sql` hands it straight back when that process exits and leaves the
migration it was meant to protect running unguarded. It has to be held by the
connection that migrates, which means one PHP process doing both.
Waiting is bounded. A container OOM-killed mid-migration would otherwise hang
every other service in the stack forever with nothing on stdout to say why, so a
timeout reports what it was waiting for -- and refuses to migrate anyway, which
is the only answer that is not a guess about what the holder was doing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
5a6316ef0a
commit
53ba299501
4 files changed
+396
-1
No files matched your search
@@ -10,6 +10,21 @@ The published image tags: `latest` follows the most recent release below,
|
||||
|
||||
Nothing yet.
|
||||
|
||||
## v0.0.7 — 2026-08-02
|
||||
|
||||
### Fixed
|
||||
|
||||
- **Three of the four services failed to start on any deploy that carried a
|
||||
migration.** php, imap-supervisor, messenger-worker and scheduler run the same
|
||||
entrypoint and all migrate on boot, within milliseconds of each other against
|
||||
one database. All four read the migration ledger before any of them writes to
|
||||
it, so all four try to apply the same migration; one succeeds and the rest die
|
||||
on `column "timezone" of relation "user" already exists`, which under `set -e`
|
||||
is a container that never comes up. Boot migrations now run under a Postgres
|
||||
advisory lock held for the whole run (`app:db:migrate`), so the containers
|
||||
that lose the race wait, find the ledger already current, and start normally.
|
||||
Nothing to do on upgrade.
|
||||
|
||||
## v0.0.6 — 2026-08-02
|
||||
|
||||
### Added
|
||||
|
||||
@@ -167,8 +167,24 @@ if [ "$1" = 'frankenphp' ] || [ "$1" = 'php' ] || [ "$1" = 'bin/console' ]; then
|
||||
echo 'The database is now ready and reachable'
|
||||
fi
|
||||
|
||||
# app:db:migrate, not doctrine:migrations:migrate — it runs exactly
|
||||
# that, with exactly these flags, but holds a Postgres advisory lock
|
||||
# across the whole run.
|
||||
#
|
||||
# php, imap-supervisor, messenger-worker and scheduler all reach this
|
||||
# line within milliseconds of each other against one database, and all
|
||||
# four read the migration ledger before any of them writes to it. One
|
||||
# applies the change and commits; the others block on its table lock and
|
||||
# then fail on a schema that has already moved — "column ... already
|
||||
# exists". With `set -e` above, that is three services that never start.
|
||||
#
|
||||
# The lock cannot live in this script: pg_advisory_lock is scoped to the
|
||||
# database session, so a `dbal:run-sql` that takes it hands it straight
|
||||
# back on exit and leaves the migrate that follows unprotected. It has
|
||||
# to be the same connection that migrates, which means one PHP process
|
||||
# doing both. See src/Command/Setup/MigrateCommand.php.
|
||||
if [ "$( find ./migrations -iname '*.php' -print -quit )" ]; then
|
||||
php bin/console doctrine:migrations:migrate --no-interaction --all-or-nothing
|
||||
php bin/console app:db:migrate --no-interaction
|
||||
fi
|
||||
|
||||
# The secrets that need PHP: a VAPID keypair and the JWT keys. Runs
|
||||
|
||||
@@ -0,0 +1,221 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Command\Setup;
|
||||
|
||||
use Doctrine\DBAL\Connection;
|
||||
use Doctrine\DBAL\Platforms\PostgreSQLPlatform;
|
||||
use Symfony\Component\Console\Attribute\AsCommand;
|
||||
use Symfony\Component\Console\Command\Command;
|
||||
use Symfony\Component\Console\Input\ArrayInput;
|
||||
use Symfony\Component\Console\Input\InputInterface;
|
||||
use Symfony\Component\Console\Input\InputOption;
|
||||
use Symfony\Component\Console\Output\OutputInterface;
|
||||
use Symfony\Component\Console\Style\SymfonyStyle;
|
||||
|
||||
/**
|
||||
* `doctrine:migrations:migrate`, but only one container at a time.
|
||||
*
|
||||
* A plMail stack starts four containers from the same image — php,
|
||||
* imap-supervisor, messenger-worker and scheduler — and every one of them runs
|
||||
* `frankenphp/docker-entrypoint.sh`, which migrates on boot. They come up
|
||||
* within milliseconds of each other against one database, all four read the
|
||||
* ledger before any of them has written to it, and all four decide the same
|
||||
* migration is pending. One wins; the rest block on the table lock it holds,
|
||||
* are released when it commits, and then fail on a schema that has already
|
||||
* moved:
|
||||
*
|
||||
* SQLSTATE[42701]: Duplicate column: 7
|
||||
* ERROR: column "timezone" of relation "user" already exists
|
||||
*
|
||||
* `--all-or-nothing` widens that window rather than closing it: every pending
|
||||
* migration goes into one transaction, so the losers are held for as long as
|
||||
* the whole batch takes instead of one statement. Under `set -e` in the
|
||||
* entrypoint the failure aborts the boot, and three of the four services never
|
||||
* start. Reproduced with four concurrent migrates against one database: three
|
||||
* exited 7 with the message above.
|
||||
*
|
||||
* The fix is a Postgres advisory lock held across the whole run, so the losers
|
||||
* arrive after the winner has committed AND recorded the version, read a ledger
|
||||
* that is already current, and exit 0 having found nothing to do. They still
|
||||
* run the migration — skipping it would mean a container coming up against a
|
||||
* schema nobody verified.
|
||||
*
|
||||
* Why this cannot be a line of shell. `pg_advisory_lock()` is scoped to the
|
||||
* database session, so `bin/console dbal:run-sql "SELECT pg_advisory_lock(…)"`
|
||||
* takes the lock and gives it straight back when the process exits — the next
|
||||
* statement is unprotected. The lock has to be held by the same connection that
|
||||
* runs the migration, which means one PHP process doing both. That is this one.
|
||||
*/
|
||||
#[AsCommand(
|
||||
name: 'app:db:migrate',
|
||||
description: 'Run pending migrations under a lock, so several containers booting together cannot collide.',
|
||||
)]
|
||||
final class MigrateCommand extends Command
|
||||
{
|
||||
/**
|
||||
* crc32('plmail:doctrine-migrations').
|
||||
*
|
||||
* Any stable number would do — what matters is that every container of a
|
||||
* given install computes the same one, which rules out anything derived at
|
||||
* runtime. Hashing a string the project owns means it is reproducible from
|
||||
* the source rather than being a magic number nobody may change.
|
||||
*
|
||||
* The scope is already right without any effort: Postgres advisory locks
|
||||
* live in one database, so two plMail installs sharing a server never see
|
||||
* each other's, and no key needs to carry the database name.
|
||||
*/
|
||||
private const int LOCK_KEY = 1227082528;
|
||||
|
||||
/**
|
||||
* How long a container waits for whoever is migrating before it gives up,
|
||||
* in seconds.
|
||||
*
|
||||
* Blocking forever is not an option: one container that dies holding the
|
||||
* lock — OOM-killed mid-migration, say — would otherwise hang every other
|
||||
* service in the stack indefinitely, with no output to say why. Five
|
||||
* minutes is long enough for any migration this project has ever shipped
|
||||
* and short enough that a stuck stack reports itself.
|
||||
*/
|
||||
private const int DEFAULT_LOCK_TIMEOUT = 300;
|
||||
|
||||
/** Between attempts. Contention lasts seconds, so polling is cheap. */
|
||||
private const int POLL_INTERVAL_MICROSECONDS = 250_000;
|
||||
|
||||
public function __construct(private readonly Connection $connection)
|
||||
{
|
||||
parent::__construct();
|
||||
}
|
||||
|
||||
protected function configure(): void
|
||||
{
|
||||
$this->addOption(
|
||||
'lock-timeout',
|
||||
null,
|
||||
InputOption::VALUE_REQUIRED,
|
||||
'Seconds to wait for another container to finish migrating before failing',
|
||||
(string) self::DEFAULT_LOCK_TIMEOUT,
|
||||
);
|
||||
}
|
||||
|
||||
protected function execute(InputInterface $input, OutputInterface $output): int
|
||||
{
|
||||
$io = new SymfonyStyle($input, $output);
|
||||
|
||||
// Everything plMail ships runs on Postgres, but the connection is an
|
||||
// operator's to point wherever they like. On anything else the advisory
|
||||
// lock functions do not exist and asking for them would turn "your
|
||||
// database is unusual" into "your container will not start" — so
|
||||
// migrate unprotected, which is exactly what happened before this
|
||||
// command existed.
|
||||
if (!$this->connection->getDatabasePlatform() instanceof PostgreSQLPlatform) {
|
||||
return $this->runMigrations($output);
|
||||
}
|
||||
|
||||
$timeout = max(0, (int) $input->getOption('lock-timeout'));
|
||||
|
||||
if (!$this->acquireLock($io, $timeout)) {
|
||||
$io->error(sprintf(
|
||||
'Timed out after %ds waiting for another container to finish running migrations. '
|
||||
.'Refusing to migrate: whatever holds the lock may still be mid-migration, and a '
|
||||
.'second migrate on top of it is what this lock exists to prevent. Check whether '
|
||||
.'another container is stuck, then start this one again.',
|
||||
$timeout,
|
||||
));
|
||||
|
||||
return Command::FAILURE;
|
||||
}
|
||||
|
||||
try {
|
||||
return $this->runMigrations($output);
|
||||
} finally {
|
||||
$this->releaseLock($io);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* The real command, with the flags the entrypoint used to pass it.
|
||||
*
|
||||
* Run through the console application rather than reimplemented, so the
|
||||
* output, the exit codes and the behaviour of every edge case stay
|
||||
* Doctrine's and not a second copy of them that drifts.
|
||||
*/
|
||||
private function runMigrations(OutputInterface $output): int
|
||||
{
|
||||
$application = $this->getApplication();
|
||||
|
||||
if (null === $application) {
|
||||
throw new \RuntimeException('app:db:migrate has to be run through the console application.');
|
||||
}
|
||||
|
||||
$migrate = new ArrayInput([
|
||||
'command' => 'doctrine:migrations:migrate',
|
||||
// Deliberately null and not true: --all-or-nothing takes an
|
||||
// optional value, and passing one is deprecated. null is how the
|
||||
// console represents "the flag was given, bare", which is what the
|
||||
// entrypoint's command line produced.
|
||||
'--all-or-nothing' => null,
|
||||
]);
|
||||
|
||||
// The entrypoint's --no-interaction. Without it a container with a TTY
|
||||
// would sit on Doctrine's "are you sure?" prompt forever.
|
||||
$migrate->setInteractive(false);
|
||||
|
||||
return $application->find('doctrine:migrations:migrate')->run($migrate, $output);
|
||||
}
|
||||
|
||||
/**
|
||||
* Polls rather than blocking on pg_advisory_lock() so that giving up is
|
||||
* possible at all — a blocking acquire cannot be interrupted from PHP, and
|
||||
* a stuck winner would take the whole stack down with it.
|
||||
*/
|
||||
private function acquireLock(SymfonyStyle $io, int $timeout): bool
|
||||
{
|
||||
$deadline = microtime(true) + $timeout;
|
||||
$announced = false;
|
||||
|
||||
while (true) {
|
||||
if ($this->tryLock()) {
|
||||
return true;
|
||||
}
|
||||
|
||||
if (microtime(true) >= $deadline) {
|
||||
return false;
|
||||
}
|
||||
|
||||
if (!$announced) {
|
||||
$io->text('Another container is running migrations; waiting for it to finish...');
|
||||
$announced = true;
|
||||
}
|
||||
|
||||
usleep(self::POLL_INTERVAL_MICROSECONDS);
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Cast to int in SQL: pg_try_advisory_lock returns a boolean, and what a
|
||||
* driver hands back for one is its own business — true, 't' and '1' are all
|
||||
* plausible. An integer is not.
|
||||
*/
|
||||
private function tryLock(): bool
|
||||
{
|
||||
return 1 === (int) $this->connection->fetchOne(
|
||||
'SELECT pg_try_advisory_lock(?)::int',
|
||||
[self::LOCK_KEY],
|
||||
);
|
||||
}
|
||||
|
||||
private function releaseLock(SymfonyStyle $io): void
|
||||
{
|
||||
try {
|
||||
$this->connection->executeQuery('SELECT pg_advisory_unlock(?)', [self::LOCK_KEY]);
|
||||
} catch (\Throwable $error) {
|
||||
// Worth saying, not worth failing over. The lock belongs to this
|
||||
// session, so it is gone the moment the process is — the only cost
|
||||
// of an unlock that did not happen is the next container waiting
|
||||
// out the few milliseconds until PHP exits.
|
||||
$io->warning(sprintf('Could not release the migration lock: %s', $error->getMessage()));
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,143 @@
|
||||
<?php
|
||||
|
||||
declare(strict_types=1);
|
||||
|
||||
namespace App\Tests\Command;
|
||||
|
||||
use App\Command\Setup\MigrateCommand;
|
||||
use Doctrine\DBAL\Connection;
|
||||
use Doctrine\DBAL\DriverManager;
|
||||
use Symfony\Bundle\FrameworkBundle\Console\Application;
|
||||
use Symfony\Bundle\FrameworkBundle\Test\KernelTestCase;
|
||||
use Symfony\Component\Console\Command\Command;
|
||||
use Symfony\Component\Console\Tester\CommandTester;
|
||||
|
||||
/**
|
||||
* The advisory lock that keeps four containers from migrating at once.
|
||||
*
|
||||
* What is NOT tested here, deliberately: four real migrates racing. PHPUnit is
|
||||
* one process against one database that is already at the latest version, so
|
||||
* "they collide" cannot be staged inside it without forking processes and
|
||||
* rolling the schema backwards underneath the suite — a test that breaks the
|
||||
* database it runs on when it fails. That case was reproduced and re-verified
|
||||
* by hand instead (four concurrent boots: three exited 7 with "column
|
||||
* 'timezone' of relation 'user' already exists" before the fix, all four exited
|
||||
* 0 after).
|
||||
*
|
||||
* What is tested is the part that makes that outcome possible and that could
|
||||
* silently stop working: the lock is genuinely exclusive across sessions, a
|
||||
* 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.
|
||||
*/
|
||||
final class MigrateCommandTest extends KernelTestCase
|
||||
{
|
||||
private Connection $connection;
|
||||
private CommandTester $command;
|
||||
|
||||
/** The session standing in for another container mid-migration. */
|
||||
private ?Connection $otherContainer = null;
|
||||
|
||||
protected function setUp(): void
|
||||
{
|
||||
self::bootKernel();
|
||||
|
||||
$this->connection = self::getContainer()->get(Connection::class);
|
||||
$this->command = new CommandTester(
|
||||
new Application(self::$kernel)->find('app:db:migrate'),
|
||||
);
|
||||
}
|
||||
|
||||
protected function tearDown(): void
|
||||
{
|
||||
// Session-scoped, so closing is enough — but an assertion that failed
|
||||
// mid-test would otherwise leave the lock held for as long as the
|
||||
// connection lived, and every later test in the process would wait it
|
||||
// out.
|
||||
$this->otherContainer?->close();
|
||||
$this->otherContainer = null;
|
||||
|
||||
parent::tearDown();
|
||||
}
|
||||
|
||||
public function testWaitingContainerRefusesToMigrateWhenTheLockNeverFrees(): void
|
||||
{
|
||||
$this->otherContainer = $this->openSecondSession();
|
||||
$this->otherContainer->executeQuery('SELECT pg_advisory_lock(?)', [$this->lockKey()]);
|
||||
|
||||
// Short, because the point is the timeout branch and not the wait.
|
||||
$exit = $this->command->execute(['--lock-timeout' => '1']);
|
||||
|
||||
self::assertSame(Command::FAILURE, $exit);
|
||||
|
||||
$display = $this->command->getDisplay();
|
||||
|
||||
// The message has to name what it was waiting for: a container that
|
||||
// dies on boot with a bare non-zero exit tells an operator nothing.
|
||||
self::assertStringContainsString('Timed out', $display);
|
||||
self::assertStringContainsString('migrations', $display);
|
||||
|
||||
// The whole point of bounding the wait is that it stays a refusal.
|
||||
// Giving up and migrating anyway would be the original race with extra
|
||||
// steps.
|
||||
self::assertStringNotContainsString('Migrating', $display);
|
||||
}
|
||||
|
||||
public function testTheLockIsExclusiveBetweenSessions(): void
|
||||
{
|
||||
$this->otherContainer = $this->openSecondSession();
|
||||
|
||||
self::assertSame(
|
||||
1,
|
||||
(int) $this->otherContainer->fetchOne('SELECT pg_try_advisory_lock(?)::int', [$this->lockKey()]),
|
||||
'A free lock has to be takeable, or this test proves nothing below.',
|
||||
);
|
||||
|
||||
// Same key, different session: this is the mutual exclusion the boot
|
||||
// sequence rests on. Postgres advisory locks are re-entrant within a
|
||||
// session, so testing it from one connection would always pass.
|
||||
self::assertSame(
|
||||
0,
|
||||
(int) $this->connection->fetchOne('SELECT pg_try_advisory_lock(?)::int', [$this->lockKey()]),
|
||||
);
|
||||
|
||||
$this->connection->executeQuery('SELECT pg_advisory_unlock(?)', [$this->lockKey()]);
|
||||
}
|
||||
|
||||
public function testTheLockIsHandedBackWhenTheCommandFinishes(): void
|
||||
{
|
||||
// Nothing is pending — the suite runs on a migrated database — so this
|
||||
// is the normal case: the container that has nothing to do still takes
|
||||
// the lock, and must not sit on it.
|
||||
self::assertSame(Command::SUCCESS, $this->command->execute([]));
|
||||
|
||||
$this->otherContainer = $this->openSecondSession();
|
||||
|
||||
self::assertSame(
|
||||
1,
|
||||
(int) $this->otherContainer->fetchOne('SELECT pg_try_advisory_lock(?)::int', [$this->lockKey()]),
|
||||
'The lock was still held after the command exited; the next container would wait for nothing.',
|
||||
);
|
||||
|
||||
$this->otherContainer->executeQuery('SELECT pg_advisory_unlock(?)', [$this->lockKey()]);
|
||||
}
|
||||
|
||||
/**
|
||||
* A second connection to the same database, which is what makes these
|
||||
* assertions mean anything: the lock is scoped to a session, and the
|
||||
* container this is standing in for has its own.
|
||||
*/
|
||||
private function openSecondSession(): Connection
|
||||
{
|
||||
return DriverManager::getConnection($this->connection->getParams());
|
||||
}
|
||||
|
||||
/**
|
||||
* Read off the command rather than copied, so a change to the key cannot
|
||||
* leave a green test asserting against the old one.
|
||||
*/
|
||||
private function lockKey(): int
|
||||
{
|
||||
return (int) new \ReflectionClassConstant(MigrateCommand::class, 'LOCK_KEY')->getValue();
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user