diff --git a/README.md b/README.md index 709ea39..0f36fcd 100644 --- a/README.md +++ b/README.md @@ -109,7 +109,7 @@ nothing to clean up after — but with no daemon those suites fail rather than s ## Running it -Four commands, in order. The first two are once per machine. +Three commands, in order. The first is once per machine. **1. The development dependencies** — PostgreSQL and Keycloak, with the `dodossh` realm imported: @@ -117,27 +117,29 @@ Four commands, in order. The first two are once per machine. docker compose -f deploy/docker-compose.dev.yml up -d ``` -**2. The schema.** The API never migrates anything: it fails readiness while a migration is pending, and -says which one. `dotnet-ef` is pinned in `.config/dotnet-tools.json`, so run `dotnet tool restore` first if -you have not: - -```bash -dotnet ef database update --project src/DodoSSH.Infrastructure -``` - -With nothing else configured this targets the compose stack above. Set `DODOSSH_DESIGN_CONNECTION` to point -it at another database. - -**3. The server:** +**2. The server:** ```bash dotnet run --project src/DodoSSH.Api ``` -It listens on `http://localhost:5233`, serving `/healthz/live`, `/healthz/ready` and — in +It applies any pending migrations before it opens its port, so there is no separate schema step and no +window in which a request meets a half-applied schema. Set `Database:AutoMigrate` to `false` where +something else owns the schema — a migrator job, or a database user denied DDL — and the old behaviour +comes back: readiness fails while a migration is pending, and the log says which one. To apply them by +hand, `dotnet-ef` is pinned in `.config/dotnet-tools.json` (`dotnet tool restore` first if you have not): + +```bash +dotnet ef database update --project src/DodoSSH.Infrastructure +``` + +With nothing else configured that targets the compose stack above. Set `DODOSSH_DESIGN_CONNECTION` to +point it at another database. + +The server listens on `http://localhost:5233`, serving `/healthz/live`, `/healthz/ready` and — in Development — `/openapi/v1.json`. -**4. The desktop client:** +**3. The desktop client:** ```bash dotnet run --project src/DodoSSH.Client.App diff --git a/src/DodoSSH.Api/Program.cs b/src/DodoSSH.Api/Program.cs index 68bdf46..df29030 100644 --- a/src/DodoSSH.Api/Program.cs +++ b/src/DodoSSH.Api/Program.cs @@ -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); diff --git a/src/DodoSSH.Api/Setup/Configuration.cs b/src/DodoSSH.Api/Setup/Configuration.cs index 3efec49..6ed888f 100644 --- a/src/DodoSSH.Api/Setup/Configuration.cs +++ b/src/DodoSSH.Api/Setup/Configuration.cs @@ -17,6 +17,11 @@ internal static class Configuration .ValidateDataAnnotations() .ValidateOnStart(); + services.AddOptions() + .BindConfiguration(DatabaseOptions.SectionName) + .ValidateDataAnnotations() + .ValidateOnStart(); + services.AddOptions() .BindConfiguration(OidcOptions.SectionName) .ValidateDataAnnotations() diff --git a/src/DodoSSH.Api/Setup/DodoOptions.cs b/src/DodoSSH.Api/Setup/DodoOptions.cs index 97edac1..857a696 100644 --- a/src/DodoSSH.Api/Setup/DodoOptions.cs +++ b/src/DodoSSH.Api/Setup/DodoOptions.cs @@ -65,6 +65,32 @@ public sealed class OidcOptions public string NameClaim { get; set; } = "name"; } +/// Schema management. +public sealed class DatabaseOptions +{ + /// Configuration section name. + public const string SectionName = "Database"; + + /// + /// Whether the API applies pending migrations as it starts. + /// + /// + /// + /// 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. + /// + /// + /// 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. + /// + /// + public bool AutoMigrate { get; set; } = true; +} + /// Relay settings. See ADR 0004. public sealed class RelayOptions { diff --git a/src/DodoSSH.Api/Setup/Persistence.cs b/src/DodoSSH.Api/Setup/Persistence.cs index b05880b..f50d21d 100644 --- a/src/DodoSSH.Api/Setup/Persistence.cs +++ b/src/DodoSSH.Api/Setup/Persistence.cs @@ -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"; + /// + /// Advisory lock every instance takes before migrating. + /// + /// + /// 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 pg_locks row + /// recognisable to whoever is looking at a stuck deployment at the time. + /// + 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; } + + /// + /// Brings the schema up to date, before the first request rather than during it. + /// + /// + /// + /// Ordered ahead of RunAsync 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. + /// + /// + /// See for the deployments that should switch this off. + /// + /// + internal static async Task MigrateDodoDatabaseAsync(this WebApplication app) + { + ArgumentNullException.ThrowIfNull(app); + + var logger = app.Services.GetRequiredService().CreateLogger("DodoSSH.Startup"); + + if (!app.Services.GetRequiredService>().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().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; + } + + /// + /// Holds the advisory lock that makes concurrent starts safe. + /// + /// + /// Without it, two replicas rolled out together run MigrateAsync 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. + /// + private sealed class MigrationGuard(NpgsqlConnection? connection) : IAsyncDisposable + { + internal static async Task 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); + } + + /// + 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); + } + } + } } /// /// Reports the database reachable and the schema current. /// /// -/// 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 — so reaching this check with migrations +/// still pending means either that 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. /// internal sealed class DatabaseHealthCheck(DodoDbContext context, ILogger logger) : IHealthCheck diff --git a/src/DodoSSH.Api/Setup/StartupLog.cs b/src/DodoSSH.Api/Setup/StartupLog.cs index ea3d5ff..c201965 100644 --- a/src/DodoSSH.Api/Setup/StartupLog.cs +++ b/src/DodoSSH.Api/Setup/StartupLog.cs @@ -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); } diff --git a/src/DodoSSH.Api/appsettings.json b/src/DodoSSH.Api/appsettings.json index 9dbf573..6df91bf 100644 --- a/src/DodoSSH.Api/appsettings.json +++ b/src/DodoSSH.Api/appsettings.json @@ -10,6 +10,9 @@ "Server": { "PublicBaseUrl": "http://localhost:5233" }, + "Database": { + "AutoMigrate": true + }, "Oidc": { "Audience": "dodossh-api", "ClientId": "dodossh-desktop",