Public Access
Apply pending migrations at startup instead of asking for a second command
The API deliberately never migrated: it failed readiness while a migration was pending and named it, and a separate step applied them. That is the right split for a deployment with a release pipeline and the wrong one for a self-hosted server, where it means an image that boots, refuses traffic, and waits for somebody to know that dotnet ef exists. The schema and the code that expects it ship in the same image, so the image is where the two are reconciled now. Before RunAsync rather than in the background. A migration racing the first requests would let them through against a half-applied schema, and the first authenticated request is the one that provisions accounts. Failing to migrate therefore fails to start, which is the loudest signal available and the one an orchestrator already acts on. Concurrent starts take a Postgres advisory lock first. Without it two replicas rolled out together read the same empty history table, both apply the same migration, and the second dies on an object that already exists — a crash loop on the day of a schema change, which is the worst day to have one. The lock is held on a connection of its own because EF opens and closes one per command, and a session lock belongs to the connection that took it. The exception is a database that does not exist yet: there is nothing to hold a lock in, so that path migrates without one and says so. Two instances creating it at once still converges — one wins, the other restarts into the ordinary locked path — and refusing to start would leave a fresh deployment stuck on the step this removes. Database:AutoMigrate turns it off for the deployments that own their schema: a migrator job, a rollout where new code must run against the old schema first, or a database user denied DDL. With it off the behaviour is exactly what it was, and the health check now explains which of the two situations a pending migration means. Verified against a throwaway PostgreSQL container: an empty database gets all seven migrations applied before the port opens, the tables land in the dodo schema, and a second start logs the schema up to date and serves. The API suite passes, which exercises the startup path once per assembly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -71,4 +71,7 @@ if (app.Environment.IsDevelopment())
|
||||
|
||||
app.WarnOnRiskyConfiguration();
|
||||
|
||||
// Before the port opens, not after. See Persistence.MigrateDodoDatabaseAsync.
|
||||
await app.MigrateDodoDatabaseAsync().ConfigureAwait(false);
|
||||
|
||||
await app.RunAsync().ConfigureAwait(false);
|
||||
|
||||
@@ -17,6 +17,11 @@ internal static class Configuration
|
||||
.ValidateDataAnnotations()
|
||||
.ValidateOnStart();
|
||||
|
||||
services.AddOptions<DatabaseOptions>()
|
||||
.BindConfiguration(DatabaseOptions.SectionName)
|
||||
.ValidateDataAnnotations()
|
||||
.ValidateOnStart();
|
||||
|
||||
services.AddOptions<OidcOptions>()
|
||||
.BindConfiguration(OidcOptions.SectionName)
|
||||
.ValidateDataAnnotations()
|
||||
|
||||
@@ -65,6 +65,32 @@ public sealed class OidcOptions
|
||||
public string NameClaim { get; set; } = "name";
|
||||
}
|
||||
|
||||
/// <summary>Schema management.</summary>
|
||||
public sealed class DatabaseOptions
|
||||
{
|
||||
/// <summary>Configuration section name.</summary>
|
||||
public const string SectionName = "Database";
|
||||
|
||||
/// <summary>
|
||||
/// Whether the API applies pending migrations as it starts.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// On by default, because the alternative asks every self-hosted operator to run a second thing in
|
||||
/// the right order and gives them an instance that boots and then fails readiness when they do not.
|
||||
/// The schema and the code that expects it ship in the same image, so the image is the natural place
|
||||
/// for the two to be reconciled.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Turn it off where the deployment already owns schema changes: a migrator job in the release
|
||||
/// pipeline, a rollout where the new code must run against the old schema first, or a database user
|
||||
/// that is deliberately denied DDL. With it off the behaviour is exactly what it was before this
|
||||
/// setting existed — pending migrations fail readiness and say which, and nothing repairs itself.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
public bool AutoMigrate { get; set; } = true;
|
||||
}
|
||||
|
||||
/// <summary>Relay settings. See ADR 0004.</summary>
|
||||
public sealed class RelayOptions
|
||||
{
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
using DodoSSH.Infrastructure;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Microsoft.Extensions.Diagnostics.HealthChecks;
|
||||
using Microsoft.Extensions.Options;
|
||||
using Npgsql;
|
||||
|
||||
namespace DodoSSH.Api.Setup;
|
||||
|
||||
@@ -9,6 +11,16 @@ internal static class Persistence
|
||||
{
|
||||
private const string ConnectionStringName = "Postgres";
|
||||
|
||||
/// <summary>
|
||||
/// Advisory lock every instance takes before migrating.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// The value is arbitrary and only has to be the same in every replica and different from whatever
|
||||
/// else might lock in this database. Spelling "MIGR" in the low bytes makes the <c>pg_locks</c> row
|
||||
/// recognisable to whoever is looking at a stuck deployment at the time.
|
||||
/// </remarks>
|
||||
private const long MigrationLockKey = 0x0D0D_0555_4D49_4752;
|
||||
|
||||
internal static IServiceCollection AddDodoPersistence(
|
||||
this IServiceCollection services,
|
||||
IConfiguration configuration)
|
||||
@@ -33,15 +45,169 @@ internal static class Persistence
|
||||
|
||||
return services;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Brings the schema up to date, before the first request rather than during it.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// Ordered ahead of <c>RunAsync</c> deliberately: a migration that ran in the background would let
|
||||
/// the first requests through against a half-applied schema, and those are precisely the requests
|
||||
/// that provision accounts. Failing to migrate therefore fails to start, which is the loudest signal
|
||||
/// available and the one an orchestrator already knows how to act on.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// See <see cref="DatabaseOptions.AutoMigrate"/> for the deployments that should switch this off.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
internal static async Task<WebApplication> MigrateDodoDatabaseAsync(this WebApplication app)
|
||||
{
|
||||
ArgumentNullException.ThrowIfNull(app);
|
||||
|
||||
var logger = app.Services.GetRequiredService<ILoggerFactory>().CreateLogger("DodoSSH.Startup");
|
||||
|
||||
if (!app.Services.GetRequiredService<IOptions<DatabaseOptions>>().Value.AutoMigrate)
|
||||
{
|
||||
StartupLog.AutoMigrateDisabled(logger);
|
||||
|
||||
return app;
|
||||
}
|
||||
|
||||
var cancellationToken = app.Lifetime.ApplicationStopping;
|
||||
|
||||
var scope = app.Services.CreateAsyncScope();
|
||||
await using var scopeHandle = scope.ConfigureAwait(false);
|
||||
|
||||
var database = scope.ServiceProvider.GetRequiredService<DodoDbContext>().Database;
|
||||
|
||||
// A connection of its own, held open across the whole migration. A session-level advisory lock
|
||||
// belongs to the connection that took it and EF opens and closes one per command, so borrowing
|
||||
// its connection would drop the lock somewhere in the middle.
|
||||
var guard = await MigrationGuard
|
||||
.AcquireAsync(database.GetConnectionString(), logger, cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
|
||||
await using var guardHandle = guard.ConfigureAwait(false);
|
||||
|
||||
var pending = await database.GetPendingMigrationsAsync(cancellationToken).ConfigureAwait(false);
|
||||
var pendingList = pending.ToList();
|
||||
|
||||
// Reported even when there is nothing to do. Two replicas starting together both say something,
|
||||
// and "up to date" from the second is what shows the lock worked rather than that it was skipped.
|
||||
if (pendingList.Count == 0)
|
||||
{
|
||||
StartupLog.SchemaUpToDate(logger);
|
||||
|
||||
return app;
|
||||
}
|
||||
|
||||
// Named rather than joined inside the call, matching the health check below: the analyzer cannot
|
||||
// see through a static wrapper to a guard, and a join over a handful of names once per process
|
||||
// start is not worth the shape needed to convince it otherwise.
|
||||
var pendingDescription = string.Join(", ", pendingList);
|
||||
|
||||
StartupLog.ApplyingMigrations(logger, pendingList.Count, pendingDescription);
|
||||
|
||||
await database.MigrateAsync(cancellationToken).ConfigureAwait(false);
|
||||
|
||||
StartupLog.MigrationsApplied(logger, pendingList.Count);
|
||||
|
||||
return app;
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Holds the advisory lock that makes concurrent starts safe.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Without it, two replicas rolled out together run <c>MigrateAsync</c> at the same moment: each
|
||||
/// reads the same empty history table, both apply the same migration, and the second fails on an
|
||||
/// object that already exists — a crash loop on the day of a schema change, which is the worst day
|
||||
/// for one. PostgreSQL grants the lock to one of them and blocks the other until it is done, at
|
||||
/// which point the second finds nothing pending and starts serving.
|
||||
/// </remarks>
|
||||
private sealed class MigrationGuard(NpgsqlConnection? connection) : IAsyncDisposable
|
||||
{
|
||||
internal static async Task<MigrationGuard> AcquireAsync(
|
||||
string? connectionString,
|
||||
ILogger logger,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
var connection = new NpgsqlConnection(connectionString);
|
||||
|
||||
try
|
||||
{
|
||||
await connection.OpenAsync(cancellationToken).ConfigureAwait(false);
|
||||
}
|
||||
catch (PostgresException exception)
|
||||
when (string.Equals(
|
||||
exception.SqlState,
|
||||
PostgresErrorCodes.InvalidCatalogName,
|
||||
StringComparison.Ordinal))
|
||||
{
|
||||
// Nothing to lock against yet: the database itself is missing, and EF's own Migrate
|
||||
// creates it. Racing here would have one replica win the CREATE DATABASE and the other
|
||||
// restart into the ordinary locked path, which is a worse first boot than it looks and
|
||||
// still converges — whereas refusing to start would leave a fresh deployment stuck on a
|
||||
// step this setting exists to remove.
|
||||
await connection.DisposeAsync().ConfigureAwait(false);
|
||||
|
||||
StartupLog.MigratingUnlockedDatabase(logger);
|
||||
|
||||
return new MigrationGuard(null);
|
||||
}
|
||||
catch
|
||||
{
|
||||
await connection.DisposeAsync().ConfigureAwait(false);
|
||||
throw;
|
||||
}
|
||||
|
||||
var command = new NpgsqlCommand("SELECT pg_advisory_lock($1)", connection);
|
||||
await using var commandHandle = command.ConfigureAwait(false);
|
||||
|
||||
command.Parameters.Add(new NpgsqlParameter { Value = MigrationLockKey });
|
||||
|
||||
await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false);
|
||||
|
||||
return new MigrationGuard(connection);
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
public async ValueTask DisposeAsync()
|
||||
{
|
||||
if (connection is null)
|
||||
{
|
||||
return;
|
||||
}
|
||||
|
||||
// Released explicitly rather than left to the connection closing. Both free the lock, but
|
||||
// only this one does it at a point in the code where a failure is visible — and an
|
||||
// unreleased migration lock is a deployment that hangs on its next start with no clue why.
|
||||
try
|
||||
{
|
||||
var command = new NpgsqlCommand("SELECT pg_advisory_unlock($1)", connection);
|
||||
await using var commandHandle = command.ConfigureAwait(false);
|
||||
|
||||
command.Parameters.Add(new NpgsqlParameter { Value = MigrationLockKey });
|
||||
|
||||
await command.ExecuteNonQueryAsync().ConfigureAwait(false);
|
||||
}
|
||||
finally
|
||||
{
|
||||
await connection.DisposeAsync().ConfigureAwait(false);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// Reports the database reachable and the schema current.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// Pending migrations make the instance unready rather than crashing it. The API never migrates in
|
||||
/// production — a separate migrator job does — so the correct response to a schema mismatch is to
|
||||
/// stop taking traffic and say so loudly, not to attempt a repair.
|
||||
/// Pending migrations make the instance unready rather than crashing it. Startup normally applies them
|
||||
/// — see <see cref="Persistence.MigrateDodoDatabaseAsync"/> — so reaching this check with migrations
|
||||
/// still pending means either that <see cref="DatabaseOptions.AutoMigrate"/> is off and whatever owns
|
||||
/// the schema has not run, or that the schema moved under a live instance. The right answer to both is
|
||||
/// to stop taking traffic and say so loudly rather than to attempt a repair mid-flight.
|
||||
/// </remarks>
|
||||
internal sealed class DatabaseHealthCheck(DodoDbContext context, ILogger<DatabaseHealthCheck> logger)
|
||||
: IHealthCheck
|
||||
|
||||
@@ -30,9 +30,45 @@ internal static partial class StartupLog
|
||||
EventId = 1010,
|
||||
Level = LogLevel.Critical,
|
||||
Message = "Database schema is out of date: {PendingCount} migration(s) pending "
|
||||
+ "({PendingMigrations}). Readiness will fail until the migrator has run.")]
|
||||
+ "({PendingMigrations}). Readiness will fail until they are applied.")]
|
||||
internal static partial void PendingMigrations(
|
||||
ILogger logger,
|
||||
int pendingCount,
|
||||
string pendingMigrations);
|
||||
|
||||
[LoggerMessage(
|
||||
EventId = 1011,
|
||||
Level = LogLevel.Information,
|
||||
Message = "Database schema is up to date.")]
|
||||
internal static partial void SchemaUpToDate(ILogger logger);
|
||||
|
||||
[LoggerMessage(
|
||||
EventId = 1012,
|
||||
Level = LogLevel.Information,
|
||||
Message = "Applying {PendingCount} pending migration(s): {PendingMigrations}.")]
|
||||
internal static partial void ApplyingMigrations(
|
||||
ILogger logger,
|
||||
int pendingCount,
|
||||
string pendingMigrations);
|
||||
|
||||
[LoggerMessage(
|
||||
EventId = 1013,
|
||||
Level = LogLevel.Information,
|
||||
Message = "Applied {AppliedCount} migration(s). The schema is now up to date.")]
|
||||
internal static partial void MigrationsApplied(ILogger logger, int appliedCount);
|
||||
|
||||
[LoggerMessage(
|
||||
EventId = 1014,
|
||||
Level = LogLevel.Information,
|
||||
Message = "Database:AutoMigrate is false. This instance will not touch the schema, and will "
|
||||
+ "fail readiness for as long as a migration is pending.")]
|
||||
internal static partial void AutoMigrateDisabled(ILogger logger);
|
||||
|
||||
[LoggerMessage(
|
||||
EventId = 1015,
|
||||
Level = LogLevel.Warning,
|
||||
Message = "The database does not exist yet, so migrations run without the advisory lock that "
|
||||
+ "normally serialises replicas. Two instances creating it at once will have one of them "
|
||||
+ "fail and restart into the ordinary path.")]
|
||||
internal static partial void MigratingUnlockedDatabase(ILogger logger);
|
||||
}
|
||||
|
||||
@@ -10,6 +10,9 @@
|
||||
"Server": {
|
||||
"PublicBaseUrl": "http://localhost:5233"
|
||||
},
|
||||
"Database": {
|
||||
"AutoMigrate": true
|
||||
},
|
||||
"Oidc": {
|
||||
"Audience": "dodossh-api",
|
||||
"ClientId": "dodossh-desktop",
|
||||
|
||||
Reference in New Issue
Block a user