From 66a445317f2ba7aed4437b19494373cb069841a8 Mon Sep 17 00:00:00 2001 From: fabbrik <22822543+fabbrik@users.noreply.github.com> Date: Fri, 18 Sep 2026 12:16:34 -0300 Subject: [PATCH] feat: add journaled schema migration runner ExperienceSchemaMigrator.MigrateAsync applies the package's embedded scripts through DbUp: journaled to agent_experience.schema_versions, one transaction per script, serialized across processes by a session advisory lock held on its own connection. Hosts call it explicitly; the store never migrates. Replaces the manual apply loop in the README and the test fixture, and closes the Story 2.1 epic criterion for documented migrations. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 9 +- .../AgentExperience.Storage.Postgres.csproj | 8 +- .../ExperienceSchemaMigrator.cs | 263 ++++++++++++++++ .../0001_create_experience_records.sql | 5 +- .../PostgresExperienceRecordSchema.cs | 15 +- .../PostgresExperienceRecordStore.cs | 4 +- .../README.md | 65 +++- .../packages.lock.json | 19 ++ .../DependencyBoundaryTests.cs | 3 +- .../DependencyBoundaryTests.cs | 5 +- ...ntExperience.Storage.Postgres.Tests.csproj | 11 + .../DependencyBoundaryTests.cs | 16 +- .../ExperienceSchemaMigratorTests.cs | 298 ++++++++++++++++++ .../ExtraMigrations/0001_marker.sql | 4 + .../ExtraMigrations/0002_dollar_quoted.sql | 7 + .../FailingMigrations/0001_marker.sql | 4 + .../FailingMigrations/0002_broken.sql | 2 + .../OfflineStoreTests.cs | 32 ++ .../PostgresFixture.cs | 37 ++- .../packages.lock.json | 21 +- 20 files changed, 788 insertions(+), 40 deletions(-) create mode 100644 src/AgentExperience.Storage.Postgres/ExperienceSchemaMigrator.cs create mode 100644 tests/AgentExperience.Storage.Postgres.Tests/ExperienceSchemaMigratorTests.cs create mode 100644 tests/AgentExperience.Storage.Postgres.Tests/ExtraMigrations/0001_marker.sql create mode 100644 tests/AgentExperience.Storage.Postgres.Tests/ExtraMigrations/0002_dollar_quoted.sql create mode 100644 tests/AgentExperience.Storage.Postgres.Tests/FailingMigrations/0001_marker.sql create mode 100644 tests/AgentExperience.Storage.Postgres.Tests/FailingMigrations/0002_broken.sql diff --git a/README.md b/README.md index 2869850..2b85a7f 100644 --- a/README.md +++ b/README.md @@ -31,6 +31,7 @@ AgentExperience.NET records observable evidence (tool calls, results, errors, ve | Auditable, template-based reflections traceable to evidence IDs | `AgentExperience.Core` | | MAF adapter: captures ordinary, streaming, failed, and cancelled runs plus tool calls, without altering results | `AgentExperience.MicrosoftAgentFramework` | | PostgreSQL Experience Record store: create, get, and scoped query; host authorization checked before database access; exact scope matching in SQL | `AgentExperience.Storage.Postgres` | +| Journaled schema migrations: embedded scripts applied once, one transaction per script, serialized across processes by an advisory lock | `AgentExperience.Storage.Postgres` | ## Quick look @@ -50,7 +51,7 @@ await agent.RunAsync("Triage ticket #4812", session); // The run, its tool calls, and its sanitized outcome are now available from captureService. ``` -See the [adapter README](src/AgentExperience.MicrosoftAgentFramework/README.md) for options, supported agent types, and caveats. See the [PostgreSQL store README](src/AgentExperience.Storage.Postgres/README.md) for the trust boundary, schema script, and data semantics. +See the [adapter README](src/AgentExperience.MicrosoftAgentFramework/README.md) for options, supported agent types, and caveats. See the [PostgreSQL store README](src/AgentExperience.Storage.Postgres/README.md) for the trust boundary, the `ExperienceSchemaMigrator.MigrateAsync` startup call, and data semantics. ## Design principles @@ -67,7 +68,7 @@ src/ AgentExperience.Abstractions/ domain contracts and ports (BCL only) AgentExperience.Core/ sanitization, capture, verification, reflection AgentExperience.MicrosoftAgentFramework/ MAF adapter (pinned Microsoft.Agents.AI 1.20.0) - AgentExperience.Storage.Postgres/ PostgreSQL Experience Record store (pinned Npgsql 10.0.3) + AgentExperience.Storage.Postgres/ PostgreSQL Experience Record store and schema migrator (pinned Npgsql 10.0.3, dbup-postgresql 7.0.1, dbup-core 6.1.1) tests/ AgentExperience.Abstractions.Tests/ contract and dependency-boundary tests AgentExperience.Core.Tests/ sanitizer, capture, verification, reflection tests @@ -88,10 +89,10 @@ dotnet build dotnet test ``` -Unit and MAF adapter tests run in memory, with no network, database, or model credentials. `AgentExperience.CompatibilityProof` and the `PostgresExperienceRecordStoreTests` in `AgentExperience.Storage.Postgres.Tests` start a PostgreSQL/pgvector container through Testcontainers, so they need Docker. If Testcontainers' Ryuk container fails to start under your local Docker setup, set `TESTCONTAINERS_RYUK_DISABLED=true`. To skip the container-backed tests: +Unit and MAF adapter tests run in memory, with no network, database, or model credentials. `AgentExperience.CompatibilityProof` and the `PostgresExperienceRecordStoreTests` and `ExperienceSchemaMigratorTests` in `AgentExperience.Storage.Postgres.Tests` start a PostgreSQL/pgvector container through Testcontainers, so they need Docker. If Testcontainers' Ryuk container fails to start under your local Docker setup, set `TESTCONTAINERS_RYUK_DISABLED=true`. To skip the container-backed tests: ```bash -dotnet test --filter "FullyQualifiedName!~CompatibilityProof&FullyQualifiedName!~PostgresExperienceRecordStoreTests" +dotnet test --filter "FullyQualifiedName!~CompatibilityProof&FullyQualifiedName!~PostgresExperienceRecordStoreTests&FullyQualifiedName!~ExperienceSchemaMigratorTests" ``` ## Roadmap diff --git a/src/AgentExperience.Storage.Postgres/AgentExperience.Storage.Postgres.csproj b/src/AgentExperience.Storage.Postgres/AgentExperience.Storage.Postgres.csproj index 41c2c29..5e2c110 100644 --- a/src/AgentExperience.Storage.Postgres/AgentExperience.Storage.Postgres.csproj +++ b/src/AgentExperience.Storage.Postgres/AgentExperience.Storage.Postgres.csproj @@ -1,7 +1,7 @@ - PostgreSQL adapter for AgentExperience.NET: persists canonical, scoped Experience Records through the IExperienceRecordStore port with plain Npgsql, host-authorization checks before any database access, exact scope predicates applied in SQL, and an embedded, versioned schema script. Pinned to Npgsql 10.0.3. + PostgreSQL adapter for AgentExperience.NET: persists canonical, scoped Experience Records through the IExperienceRecordStore port with plain Npgsql, host-authorization checks before any database access, exact scope predicates applied in SQL, and a journaled schema migration runner over embedded, versioned scripts. Pinned to Npgsql 10.0.3, dbup-postgresql 7.0.1, and dbup-core 6.1.1. true README.md @@ -9,6 +9,12 @@ + + + + diff --git a/src/AgentExperience.Storage.Postgres/ExperienceSchemaMigrator.cs b/src/AgentExperience.Storage.Postgres/ExperienceSchemaMigrator.cs new file mode 100644 index 0000000..6a38f3a --- /dev/null +++ b/src/AgentExperience.Storage.Postgres/ExperienceSchemaMigrator.cs @@ -0,0 +1,263 @@ +using System.Net.Sockets; +using System.Reflection; +using AgentExperience.Abstractions; +using DbUp.Engine; +using DbUp.Postgresql; +using Npgsql; + +namespace AgentExperience.Storage.Postgres; + +/// +/// Applies this package's embedded schema scripts to a PostgreSQL database, journaled, so a host can +/// bring a database up to the schema needs. The host calls +/// explicitly, once, before using the +/// store; the store never migrates on its own. +/// +/// +/// +/// Scripts run in name order, one transaction per script, and each applied script is recorded in +/// agent_experience.schema_versions, so a rerun applies nothing. The whole run is serialized +/// across processes by a PostgreSQL session advisory lock, held on its own connection, so concurrent +/// hosts cannot apply the same script twice. +/// +/// +/// The caller's data source must allow at least two concurrent connections: one for the advisory lock +/// and one for the scripts, and must not be multiplexing, because a multiplexed command does not stay +/// on one physical connection and so cannot hold a session advisory lock. The migrating role needs +/// CREATE on the database (to create the agent_experience schema) and on that schema (to +/// create its tables). The store itself only needs INSERT and SELECT on +/// agent_experience.experience_records. +/// +/// +/// The wait for the advisory lock is deliberately unbounded and ends only with the caller's token. Each +/// script, by contrast, runs under the data source's ordinary command timeout (30 seconds by default), +/// so a single long script fails with a timeout unless the connection string raises it. +/// +/// +public static class ExperienceSchemaMigrator +{ + /// The embedded-resource prefix that selects this assembly's migration scripts. + private const string ResourcePrefix = "AgentExperience.Storage.Postgres.Migrations."; + + /// The journal table, inside . + private const string JournalTable = "schema_versions"; + + /// + /// The advisory-lock key every AgentExperience.NET migration run takes, so runs in different + /// processes serialize. It is the constant 0x4147455850455201 (ASCII AGEXPER plus + /// 0x01), scoped to the database the data source points at. Hosts that coordinate their own + /// schema work on the same database must not reuse this key for anything else. + /// + internal const long AdvisoryLockKey = 0x4147455850455201L; + + /// + /// Applies every embedded schema script that this database has not recorded yet. + /// + /// + /// The host-owned data source for the database to migrate. Never disposed here. It must allow at + /// least two concurrent connections. + /// + /// + /// Cancels opening the lock connection and waiting for the advisory lock. Once scripts start + /// running, cancellation is ignored -- DbUp's upgrade has no cancellation point -- so the run + /// finishes and returns normally. + /// + /// The scripts applied by this call, in the order they ran. Empty when nothing was pending. + /// is . + /// + /// A script failed, or the database was unreachable. The failing script is named; no SQL text or + /// row data is included, and the original failure is the when + /// DbUp reported one. Scripts that ran before the failure stay applied and journaled; the failing + /// script's own transaction is rolled back. + /// + /// + /// was cancelled. Thrown unwrapped, and the advisory lock is + /// not left held. + /// + public static Task MigrateAsync( + NpgsqlDataSource dataSource, + CancellationToken cancellationToken) + { + ArgumentNullException.ThrowIfNull(dataSource); + return MigrateAsync(dataSource, typeof(ExperienceSchemaMigrator).Assembly, ResourcePrefix, cancellationToken); + } + + /// + /// The script-selection seam: same run, but over an arbitrary assembly and resource prefix. Test + /// only, so a failing script never has to ship in the package. + /// + internal static async Task MigrateAsync( + NpgsqlDataSource dataSource, + Assembly scriptAssembly, + string resourcePrefix, + CancellationToken cancellationToken) + { + cancellationToken.ThrowIfCancellationRequested(); + + NpgsqlConnection lockConnection; + try + { + lockConnection = await dataSource.OpenConnectionAsync(cancellationToken).ConfigureAwait(false); + } + catch (Exception ex) when (IsInfrastructureFailure(ex, cancellationToken)) + { + throw Translate(ex, cancellationToken); + } + + try + { + await AcquireLockAsync(lockConnection, cancellationToken).ConfigureAwait(false); + + // A cancellation request can lose the race with the server granting the lock. The upgrade + // cannot be interrupted once it starts, so this is the last point at which a cancelled + // caller can still be told nothing ran. + cancellationToken.ThrowIfCancellationRequested(); + + return await RunUpgradeAsync(dataSource, scriptAssembly, resourcePrefix, cancellationToken).ConfigureAwait(false); + } + finally + { + // Also unlocks when the acquire itself threw: a cancellation request can lose the race with + // the server granting the lock, and the connection goes back to a pool that keeps the session + // (and therefore its advisory locks) alive. Unlocking a lock this session does not hold is a + // no-op that returns false. + await ReleaseLockAsync(lockConnection).ConfigureAwait(false); + await lockConnection.DisposeAsync().ConfigureAwait(false); + } + } + + private static async Task RunUpgradeAsync( + NpgsqlDataSource dataSource, + Assembly scriptAssembly, + string resourcePrefix, + CancellationToken cancellationToken) + { + DatabaseUpgradeResult result; + try + { + // A fresh engine per call: DbUp's builder is stateful and its connection manager owns a + // connection. Building inside the guarded region keeps resource-loading and journal failures + // inside the documented exception contract. + var engine = PostgresqlExtensions + .PostgresqlDatabase(new PostgresqlConnectionManager(dataSource), PostgresExperienceRecordSchema.SchemaName) + .WithScriptsEmbeddedInAssembly(scriptAssembly, name => IsMigrationScript(name, resourcePrefix)) + .JournalToPostgresqlTable(PostgresExperienceRecordSchema.SchemaName, JournalTable) + .WithTransactionPerScript() + .WithVariablesDisabled() + .LogToNowhere() + .Build(); + + // PerformUpgrade is synchronous and blocking, so keep it off the caller's thread. It takes no + // token: cancelling mid-run would strand the advisory lock and a half-applied script. + result = await Task.Run(engine.PerformUpgrade, CancellationToken.None).ConfigureAwait(false); + } + catch (Exception ex) when (ex is not ExperienceStoreException + && (ex is not OperationCanceledException || !cancellationToken.IsCancellationRequested)) + { + // Anything else -- a driver failure, a missing or unreadable embedded resource, a journal the + // role may not create -- is a storage infrastructure failure, so only the two documented + // exception types ever leave this method. + throw Translate(ex, cancellationToken); + } + + if (!result.Successful) + { + throw Failed(result, resourcePrefix); + } + + return new ExperienceSchemaMigrationResult( + [.. result.Scripts.Select(script => ToScriptName(script.Name, resourcePrefix))]); + } + + private static Exception Failed(DatabaseUpgradeResult result, string resourcePrefix) + { + // DbUp catches script failures and reports them on the result instead of throwing. + var failedScript = result.ErrorScript is null ? null : ToScriptName(result.ErrorScript.Name, resourcePrefix); + var message = failedScript is null + ? "Experience Record schema migration failed." + : $"Experience Record schema migration failed while applying script '{failedScript}'."; + + // DbUp can report an unsuccessful upgrade with no Error, so never promise an inner exception that + // does not exist. + return result.Error is null + ? new ExperienceStoreException(message) + : new ExperienceStoreException(message, result.Error); + } + + private static async Task AcquireLockAsync(NpgsqlConnection connection, CancellationToken cancellationToken) + { + try + { + await using var command = new NpgsqlCommand("SELECT pg_advisory_lock(@key)", connection) + { + // Waiting for another host's run is normal and can outlast any fixed timeout. The + // caller's token is the only bound on the wait. + CommandTimeout = 0, + }; + command.Parameters.Add(new NpgsqlParameter("key", AdvisoryLockKey)); + await command.ExecuteNonQueryAsync(cancellationToken).ConfigureAwait(false); + } + catch (Exception ex) when (IsInfrastructureFailure(ex, cancellationToken)) + { + throw Translate(ex, cancellationToken); + } + } + + private static async Task ReleaseLockAsync(NpgsqlConnection connection) + { + try + { + await using var command = new NpgsqlCommand("SELECT pg_advisory_unlock(@key)", connection); + command.Parameters.Add(new NpgsqlParameter("key", AdvisoryLockKey)); + await command.ExecuteNonQueryAsync(CancellationToken.None).ConfigureAwait(false); + } + catch (Exception ex) when (ex is NpgsqlException or SocketException or TimeoutException + or OperationCanceledException or InvalidOperationException or ObjectDisposedException) + { + // The unlock only fails when the lock connection is already broken, closed, or disposed + // (Npgsql reports the last two as InvalidOperationException/ObjectDisposedException, which this + // filter must cover because it throws from a finally). PostgreSQL releases a + // session's advisory locks with the session. Npgsql discards broken connections rather than + // returning them to the pool, so the lock cannot outlive this call. Swallowing here keeps the + // original migration failure, which is the useful one, from being masked. + } + } + + private static bool IsMigrationScript(string resourceName, string resourcePrefix) => + resourceName.StartsWith(resourcePrefix, StringComparison.Ordinal) + && resourceName.EndsWith(".sql", StringComparison.Ordinal); + + private static string ToScriptName(string resourceName, string resourcePrefix) => + resourceName.StartsWith(resourcePrefix, StringComparison.Ordinal) + ? resourceName[resourcePrefix.Length..] + : resourceName; + + /// + /// Driver, socket, and timeout failures are translated. An + /// caused by the caller's own token is not matched, so it propagates unwrapped with its stack. + /// + private static bool IsInfrastructureFailure(Exception ex, CancellationToken cancellationToken) => ex switch + { + OperationCanceledException => !cancellationToken.IsCancellationRequested, + NpgsqlException or SocketException or TimeoutException => true, + _ => false, + }; + + private static Exception Translate(Exception ex, CancellationToken cancellationToken) + { + if (cancellationToken.IsCancellationRequested) + { + // The caller cancelled while the driver reported a failure: surface cancellation, unwrapped. + return new OperationCanceledException("The Experience Record schema migration was cancelled.", ex, cancellationToken); + } + + return new ExperienceStoreException("Experience Record schema migration failed due to a storage infrastructure error.", ex); + } +} + +/// What one call changed. +/// +/// The scripts this call applied and journaled, in the order they ran, named as in +/// . Empty when the database was already current. +/// +public sealed record ExperienceSchemaMigrationResult(IReadOnlyList AppliedScripts); diff --git a/src/AgentExperience.Storage.Postgres/Migrations/0001_create_experience_records.sql b/src/AgentExperience.Storage.Postgres/Migrations/0001_create_experience_records.sql index d119ec0..1fc4cbd 100644 --- a/src/AgentExperience.Storage.Postgres/Migrations/0001_create_experience_records.sql +++ b/src/AgentExperience.Storage.Postgres/Migrations/0001_create_experience_records.sql @@ -1,5 +1,8 @@ -- AgentExperience.NET: initial Experience Record schema (payload_version 1). --- Plain SQL with no journal table, so a DbUp-style migrator can run it unchanged. +-- Applied by ExperienceSchemaMigrator and recorded in agent_experience.schema_versions. +-- Every statement is IF NOT EXISTS on purpose: that idempotence is what lets the migrator journal a +-- database whose schema was applied by hand before the runner existed. Do not edit this script; it is +-- already journaled elsewhere. Add the next-numbered script instead. CREATE SCHEMA IF NOT EXISTS agent_experience; diff --git a/src/AgentExperience.Storage.Postgres/PostgresExperienceRecordSchema.cs b/src/AgentExperience.Storage.Postgres/PostgresExperienceRecordSchema.cs index e9d7bde..893cf14 100644 --- a/src/AgentExperience.Storage.Postgres/PostgresExperienceRecordSchema.cs +++ b/src/AgentExperience.Storage.Postgres/PostgresExperienceRecordSchema.cs @@ -1,14 +1,19 @@ +using Npgsql; + namespace AgentExperience.Storage.Postgres; /// -/// Access to the schema scripts embedded in this package. Until a migration runner ships, the host -/// applies these scripts itself, in order, before using -/// . Scripts are plain SQL with no journal table, so a -/// DbUp-style migrator can run them unchanged later. +/// Access to the schema scripts embedded in this package, for reading a script's SQL before it runs. +/// To apply them, call , +/// which runs them in order and journals what it applied; hosts do not need +/// their own apply loop. /// public static class PostgresExperienceRecordSchema { - /// The PostgreSQL schema that holds every AgentExperience.NET table. + /// + /// The PostgreSQL schema that holds every AgentExperience.NET table, including the migration + /// journal schema_versions. + /// public const string SchemaName = "agent_experience"; /// The initial script that creates the experience_records table. diff --git a/src/AgentExperience.Storage.Postgres/PostgresExperienceRecordStore.cs b/src/AgentExperience.Storage.Postgres/PostgresExperienceRecordStore.cs index 8103bad..981d149 100644 --- a/src/AgentExperience.Storage.Postgres/PostgresExperienceRecordStore.cs +++ b/src/AgentExperience.Storage.Postgres/PostgresExperienceRecordStore.cs @@ -10,7 +10,9 @@ namespace AgentExperience.Storage.Postgres; /// over PostgreSQL with plain Npgsql. Each operation validates /// the request, checks it against the host-established , and only /// then opens a connection and runs parameterized SQL whose predicates apply the exact scope. The -/// schema must already exist; see . +/// schema must already exist: the host applies it once by calling +/// . The store +/// never migrates, on construction or otherwise. /// /// /// PostgreSQL timestamptz stores microseconds, so and diff --git a/src/AgentExperience.Storage.Postgres/README.md b/src/AgentExperience.Storage.Postgres/README.md index adfc60b..9a777be 100644 --- a/src/AgentExperience.Storage.Postgres/README.md +++ b/src/AgentExperience.Storage.Postgres/README.md @@ -3,8 +3,9 @@ Stores AgentExperience.NET Experience Records in PostgreSQL through the `IExperienceRecordStore` port, using plain Npgsql. -Pinned to `Npgsql` **10.0.3** (exact). Integration tests run against PostgreSQL 16 (`pgvector/pgvector:pg16`) through -`Testcontainers.PostgreSql` 4.15.0. This package does not use EF Core, Dapper, Pgvector, or the pgvector extension. +Pinned to `Npgsql` **10.0.3**, `dbup-postgresql` **7.0.1**, and `dbup-core` **6.1.1** (all exact). Integration tests +run against PostgreSQL 16 (`pgvector/pgvector:pg16`) through `Testcontainers.PostgreSql` 4.15.0. This package does not +use EF Core, Dapper, Pgvector, or the pgvector extension. ## Usage @@ -14,6 +15,10 @@ using AgentExperience.Storage.Postgres; using Npgsql; await using var dataSource = NpgsqlDataSource.Create(connectionString); + +// Once at startup, before the store is used. See "Schema" below. +await ExperienceSchemaMigrator.MigrateAsync(dataSource, cancellationToken); + IExperienceRecordStore store = new PostgresExperienceRecordStore(dataSource); // Established by the host from its own authentication and authorization. Never built from request input. @@ -77,19 +82,57 @@ The schema lives in the embedded script `Migrations/0001_create_experience_recor - A `payload_version` column. This adapter owns versioning, so the domain types carry no version field. - An index on `(tenant_id, application_id, project_id)`. -No migration runner ships yet. Until one does, apply the scripts yourself, in order, before using the store: +### Applying it + +Call `ExperienceSchemaMigrator.MigrateAsync` explicitly at startup, before using the store. The store never migrates +on its own, and nothing migrates on construction. ```csharp -foreach (var name in PostgresExperienceRecordSchema.ScriptNames) -{ - await using var command = dataSource.CreateCommand(PostgresExperienceRecordSchema.GetScript(name)); - await command.ExecuteNonQueryAsync(cancellationToken); -} +await using var dataSource = NpgsqlDataSource.Create(connectionString); + +var migration = await ExperienceSchemaMigrator.MigrateAsync(dataSource, cancellationToken); +// migration.AppliedScripts lists what this call applied; it is empty when the database was already current. ``` -The script is plain SQL with no journal table and uses `IF NOT EXISTS`, so a DbUp-style migrator can later run it -unchanged. Applying it needs permission to create schemas and tables. The store itself only needs `INSERT` and -`SELECT` on `agent_experience.experience_records`. +- **Journaled.** Applied scripts are recorded in `agent_experience.schema_versions` (created by the runner), so a + rerun applies nothing. A database whose `0001` was applied by hand is journaled on the next run without losing + rows, because `0001` is idempotent. +- **One transaction per script.** A failing script rolls back its own transaction; scripts applied before it stay + applied and journaled. +- **Serialized across processes.** The whole run holds a PostgreSQL session advisory lock on its own connection, so + two hosts starting at once cannot apply the same script twice. The lock is always released. +- **Permissions.** The migrating role needs `CREATE` on the database (for the `agent_experience` schema) and on that + schema (for its tables). The store itself only needs `INSERT` and `SELECT` on + `agent_experience.experience_records`. +- **Connections.** The data source must allow at least two concurrent connections: one for the advisory lock and one + for the scripts. A multiplexing data source (`NpgsqlDataSourceBuilder.EnableMultiplexing`) cannot hold a session + advisory lock, because its commands do not stay on one physical connection, so it is not supported for migration. + Build a non-multiplexing data source for the `MigrateAsync` call. +- **Timeouts.** The wait for the advisory lock is deliberately unbounded, so a run can queue behind another host for + as long as the caller allows; the caller's `CancellationToken` is the only bound on it. Each *script*, by contrast, + runs under Npgsql's ordinary command timeout (30 seconds by default), so a single script that takes longer fails + with a timeout. Raise `Command Timeout` on the connection string if a future script needs longer. + +| Situation | Result | +| --- | --- | +| Nothing pending | returns an empty `AppliedScripts` | +| A script fails | throws `ExperienceStoreException` naming the failed script (no SQL text or row data), with the original failure as `InnerException` | +| Database unreachable | throws `ExperienceStoreException` | +| Caller cancellation (before the call, or while waiting for the lock) | throws `OperationCanceledException`, unwrapped; no lock left held | +| Caller cancellation once scripts are running | ignored: DbUp's upgrade has no cancellation point, so the run finishes and returns normally | + +Scripts are selected only from this package's embedded `Migrations/*.sql` resources, in name order. DbUp variable +substitution is off, so `$body$` and `$1` in a script are left alone, and the runner does not log. + +`PostgresExperienceRecordSchema.ScriptNames` and `GetScript` remain available for reading a script's SQL (for review +or for applying it through your own change-management tooling), but the migrator is the supported way to apply it. + +### Script naming and ordering + +Scripts are **append-only**. Each is named with a zero-padded numeric prefix (`0001_`, `0002_`, ...) and a short +description, and they run in ordinal name order. The journal records a script by name, so **a script that has been +journaled anywhere must never be edited or renamed**: databases that already applied it would silently keep the old +definition, and a rename would reapply it. Change the schema by adding the next-numbered script instead. ## Data semantics diff --git a/src/AgentExperience.Storage.Postgres/packages.lock.json b/src/AgentExperience.Storage.Postgres/packages.lock.json index 42ab5e8..b0e78c4 100644 --- a/src/AgentExperience.Storage.Postgres/packages.lock.json +++ b/src/AgentExperience.Storage.Postgres/packages.lock.json @@ -2,6 +2,25 @@ "version": 1, "dependencies": { "net10.0": { + "dbup-core": { + "type": "Direct", + "requested": "[6.1.1, 6.1.1]", + "resolved": "6.1.1", + "contentHash": "kgpuyJVEFJHoIj/slnc994Go88aoeZqNDfGHDBr4sh7CsEWwJhOTCt/FJqO4ziUImL5L0NEY0kxxOiNgPKI2Fw==", + "dependencies": { + "Microsoft.Extensions.Logging.Abstractions": "8.0.0" + } + }, + "dbup-postgresql": { + "type": "Direct", + "requested": "[7.0.1, 7.0.1]", + "resolved": "7.0.1", + "contentHash": "mRnmENWWPuuMZ538gOd1mZnzucx6FQk0anmw3EABjGfcbp24FDb9QdGepYrDiaM8K9s5/gd49+5cmBOlniH/lg==", + "dependencies": { + "Npgsql": "10.0.1", + "dbup-core": "6.1.1" + } + }, "Npgsql": { "type": "Direct", "requested": "[10.0.3, 10.0.3]", diff --git a/tests/AgentExperience.Abstractions.Tests/DependencyBoundaryTests.cs b/tests/AgentExperience.Abstractions.Tests/DependencyBoundaryTests.cs index c0bea92..7d805ca 100644 --- a/tests/AgentExperience.Abstractions.Tests/DependencyBoundaryTests.cs +++ b/tests/AgentExperience.Abstractions.Tests/DependencyBoundaryTests.cs @@ -5,7 +5,7 @@ namespace AgentExperience.Abstractions.Tests; /// -/// Proves AgentExperience.Abstractions has no dependency on MAF, EF Core, Npgsql, +/// Proves AgentExperience.Abstractions has no dependency on MAF, EF Core, Npgsql, DbUp, /// OpenTelemetry, or a model-provider package (AC5). This runs in CI on every push/PR so the /// boundary cannot silently regress as later stories/adapters are added to the solution. /// @@ -20,6 +20,7 @@ public class DependencyBoundaryTests "Microsoft.Agents", // Microsoft Agent Framework (MAF) "Microsoft.EntityFrameworkCore", // EF Core "Npgsql", // PostgreSQL driver + "dbup", // DbUp schema migrations (adapter-only, see AgentExperience.Storage.Postgres) "OpenTelemetry", "Microsoft.Extensions.AI", // model-provider / AI abstractions "Microsoft.SemanticKernel", diff --git a/tests/AgentExperience.Core.Tests/DependencyBoundaryTests.cs b/tests/AgentExperience.Core.Tests/DependencyBoundaryTests.cs index dcff2b7..acb7904 100644 --- a/tests/AgentExperience.Core.Tests/DependencyBoundaryTests.cs +++ b/tests/AgentExperience.Core.Tests/DependencyBoundaryTests.cs @@ -5,8 +5,8 @@ namespace AgentExperience.Core.Tests; /// -/// Proves AgentExperience.Core has no dependency on MAF, EF Core, Npgsql, OpenTelemetry, or -/// a model-provider package (AD-1) -- its only allowed dependencies are +/// Proves AgentExperience.Core has no dependency on MAF, EF Core, Npgsql, DbUp, OpenTelemetry, +/// or a model-provider package (AD-1) -- its only allowed dependencies are /// AgentExperience.Abstractions and Microsoft.Extensions.Compliance.Redaction (plus /// that package's own transitive Microsoft.Extensions.* configuration/DI/options graph). /// Mirrors AgentExperience.Abstractions.Tests/DependencyBoundaryTests.cs. This runs in CI on @@ -24,6 +24,7 @@ public class DependencyBoundaryTests "Microsoft.Agents", // Microsoft Agent Framework (MAF) "Microsoft.EntityFrameworkCore", // EF Core "Npgsql", // PostgreSQL driver + "dbup", // DbUp schema migrations (adapter-only, see AgentExperience.Storage.Postgres) "OpenTelemetry", "Microsoft.Extensions.AI", // model-provider / AI abstractions "Microsoft.SemanticKernel", diff --git a/tests/AgentExperience.Storage.Postgres.Tests/AgentExperience.Storage.Postgres.Tests.csproj b/tests/AgentExperience.Storage.Postgres.Tests/AgentExperience.Storage.Postgres.Tests.csproj index a65e49d..465eb1a 100644 --- a/tests/AgentExperience.Storage.Postgres.Tests/AgentExperience.Storage.Postgres.Tests.csproj +++ b/tests/AgentExperience.Storage.Postgres.Tests/AgentExperience.Storage.Postgres.Tests.csproj @@ -20,6 +20,17 @@ + + + + + + + + + diff --git a/tests/AgentExperience.Storage.Postgres.Tests/DependencyBoundaryTests.cs b/tests/AgentExperience.Storage.Postgres.Tests/DependencyBoundaryTests.cs index 43a321a..8623590 100644 --- a/tests/AgentExperience.Storage.Postgres.Tests/DependencyBoundaryTests.cs +++ b/tests/AgentExperience.Storage.Postgres.Tests/DependencyBoundaryTests.cs @@ -4,8 +4,9 @@ namespace AgentExperience.Storage.Postgres.Tests; /// -/// Proves AgentExperience.Storage.Postgres uses plain Npgsql only: no MAF, EF Core, Dapper, -/// Pgvector, or model-provider dependency, in either its compiled references or its csproj. +/// Proves AgentExperience.Storage.Postgres uses plain Npgsql plus DbUp for schema migrations and +/// nothing else: no MAF, EF Core, Dapper, Pgvector, or model-provider dependency, in either its compiled +/// references or its csproj. /// public class DependencyBoundaryTests { @@ -41,19 +42,20 @@ public void Storage_Postgres_does_not_reference_a_forbidden_assembly() } [Fact] - public void Storage_Postgres_csproj_declares_only_an_exact_Npgsql_pin() + public void Storage_Postgres_csproj_declares_only_the_exact_Npgsql_and_DbUp_pins() { var csprojPath = GetCsprojPath(); Assert.True(File.Exists(csprojPath), $"Could not locate AgentExperience.Storage.Postgres.csproj at '{csprojPath}'."); var packages = XDocument.Load(csprojPath) .Descendants("PackageReference") - .Select(e => (Include: e.Attribute("Include")?.Value ?? string.Empty, Version: e.Attribute("Version")?.Value)) + .Select(e => $"{e.Attribute("Include")?.Value} {e.Attribute("Version")?.Value}") + .Order(StringComparer.Ordinal) .ToList(); - var npgsql = Assert.Single(packages); - Assert.Equal("Npgsql", npgsql.Include); - Assert.Equal("[10.0.3]", npgsql.Version); + Assert.Equal( + ["Npgsql [10.0.3]", "dbup-core [6.1.1]", "dbup-postgresql [7.0.1]"], + packages); } private static string GetCsprojPath([CallerFilePath] string testSourceFilePath = "") => diff --git a/tests/AgentExperience.Storage.Postgres.Tests/ExperienceSchemaMigratorTests.cs b/tests/AgentExperience.Storage.Postgres.Tests/ExperienceSchemaMigratorTests.cs new file mode 100644 index 0000000..23527e8 --- /dev/null +++ b/tests/AgentExperience.Storage.Postgres.Tests/ExperienceSchemaMigratorTests.cs @@ -0,0 +1,298 @@ +using AgentExperience.Abstractions; +using Npgsql; +using static AgentExperience.Storage.Postgres.Tests.TestRecords; + +namespace AgentExperience.Storage.Postgres.Tests; + +/// +/// One test per row of the migrator's edge-case matrix, each on its own freshly created database in the +/// shared PostgreSQL 16 container, so a migration in one test can never be seen by another. +/// +[Collection(PostgresCollection.Name)] +public sealed class ExperienceSchemaMigratorTests +{ + /// Test-only scripts embedded in this assembly: 0001_marker.sql then a failing 0002_broken.sql. + private const string FailingPrefix = "AgentExperience.Storage.Postgres.Tests.FailingMigrations."; + + /// Test-only scripts embedded in this assembly: 0001_marker.sql then a $body$-quoted 0002_dollar_quoted.sql. + private const string ExtraPrefix = "AgentExperience.Storage.Postgres.Tests.ExtraMigrations."; + + private const string ShippedPrefix = "AgentExperience.Storage.Postgres.Migrations."; + + private readonly PostgresFixture _fixture; + + public ExperienceSchemaMigratorTests(PostgresFixture fixture) => _fixture = fixture; + + [Fact] + public async Task Fresh_database_applies_and_journals_the_initial_script_and_the_store_round_trips() + { + await using var dataSource = await _fixture.CreateDatabaseAsync("fresh"); + + var result = await ExperienceSchemaMigrator.MigrateAsync(dataSource, CancellationToken.None); + + Assert.Equal(PostgresExperienceRecordSchema.ScriptNames, result.AppliedScripts); + Assert.Equal(PostgresExperienceRecordSchema.ScriptNames, await JournaledAsync(dataSource, ShippedPrefix)); + + var store = new PostgresExperienceRecordStore(dataSource); + var tenant = NewTenant(); + var record = Full(Scope(tenant)); + + var created = await store.CreateAsync(Authorize(tenant), record, CancellationToken.None); + var read = await store.GetAsync(Authorize(tenant), record.Scope, record.ExperienceId, CancellationToken.None); + + Assert.Equal(ExperienceStoreOutcome.Created, created.Outcome); + Assert.Equal(ExperienceStoreOutcome.Found, read.Outcome); + Assert.Equal(Canonical(record), Canonical(read.Record!)); + } + + [Fact] + public async Task Rerun_on_a_migrated_database_applies_nothing_and_leaves_rows_intact() + { + await using var dataSource = await _fixture.CreateDatabaseAsync("rerun"); + await ExperienceSchemaMigrator.MigrateAsync(dataSource, CancellationToken.None); + + var store = new PostgresExperienceRecordStore(dataSource); + var tenant = NewTenant(); + var record = Full(Scope(tenant)); + await store.CreateAsync(Authorize(tenant), record, CancellationToken.None); + var appliedAt = await JournalAppliedAtAsync(dataSource); + + var rerun = await ExperienceSchemaMigrator.MigrateAsync(dataSource, CancellationToken.None); + + Assert.Empty(rerun.AppliedScripts); + Assert.Equal(PostgresExperienceRecordSchema.ScriptNames, await JournaledAsync(dataSource, ShippedPrefix)); + Assert.Equal(appliedAt, await JournalAppliedAtAsync(dataSource)); + + var read = await store.GetAsync(Authorize(tenant), record.Scope, record.ExperienceId, CancellationToken.None); + Assert.Equal(ExperienceStoreOutcome.Found, read.Outcome); + Assert.Equal(Canonical(record), Canonical(read.Record!)); + } + + [Fact] + public async Task Database_whose_initial_script_was_applied_by_hand_is_journaled_without_losing_rows() + { + await using var dataSource = await _fixture.CreateDatabaseAsync("manual"); + + // The pre-migrator way a host applied the schema: run the script text, no journal table. + await using (var command = dataSource.CreateCommand( + PostgresExperienceRecordSchema.GetScript(PostgresExperienceRecordSchema.InitialScriptName))) + { + await command.ExecuteNonQueryAsync(); + } + + var store = new PostgresExperienceRecordStore(dataSource); + var tenant = NewTenant(); + var record = Full(Scope(tenant)); + await store.CreateAsync(Authorize(tenant), record, CancellationToken.None); + + // 0001 is idempotent, so re-running it over the hand-applied schema is safe. + var result = await ExperienceSchemaMigrator.MigrateAsync(dataSource, CancellationToken.None); + + Assert.Equal(PostgresExperienceRecordSchema.ScriptNames, result.AppliedScripts); + Assert.Equal(PostgresExperienceRecordSchema.ScriptNames, await JournaledAsync(dataSource, ShippedPrefix)); + + var read = await store.GetAsync(Authorize(tenant), record.Scope, record.ExperienceId, CancellationToken.None); + Assert.Equal(ExperienceStoreOutcome.Found, read.Outcome); + Assert.Equal(Canonical(record), Canonical(read.Record!)); + } + + [Fact] + public async Task Concurrent_runs_both_succeed_and_journal_each_script_exactly_once() + { + await using var dataSource = await _fixture.CreateDatabaseAsync("concurrent"); + + // Hold the migrator's lock from outside so the first run is provably blocked, then start the second + // run and wait until it is blocked too. Both are in flight, contending, before either can proceed. + await using var holder = await dataSource.OpenConnectionAsync(); + await AdvisoryLockAsync(holder, "pg_advisory_lock"); + + var first = ExperienceSchemaMigrator.MigrateAsync(dataSource, CancellationToken.None); + await WaitForAdvisoryLockWaiterAsync(dataSource, expected: 1); + var second = ExperienceSchemaMigrator.MigrateAsync(dataSource, CancellationToken.None); + await WaitForAdvisoryLockWaiterAsync(dataSource, expected: 2); + + await AdvisoryLockAsync(holder, "pg_advisory_unlock"); + var results = await Task.WhenAll(first, second); + + // Serialized by the advisory lock: one run applies every script, the other finds the journal current. + var applied = results.Select(r => r.AppliedScripts).OrderBy(names => names.Count).ToList(); + Assert.Empty(applied[0]); + Assert.Equal(PostgresExperienceRecordSchema.ScriptNames, applied[1]); + Assert.Equal(PostgresExperienceRecordSchema.ScriptNames, await JournaledAsync(dataSource, ShippedPrefix)); + Assert.Equal(0, await AdvisoryLockCountAsync(dataSource)); + } + + [Fact] + public async Task Second_script_applies_on_top_of_an_already_journaled_one_and_keeps_its_dollar_quoting() + { + await using var dataSource = await _fixture.CreateDatabaseAsync("secondscript"); + + // A prefix that reaches only 0001 leaves 0002 pending while 0001 is journaled under its full + // resource name, which is what the next run matches on. + var firstRun = await ExperienceSchemaMigrator.MigrateAsync( + dataSource, typeof(ExperienceSchemaMigratorTests).Assembly, ExtraPrefix + "0001", CancellationToken.None); + + Assert.Equal(["_marker.sql"], firstRun.AppliedScripts); + Assert.Equal(["0001_marker.sql"], await JournaledAsync(dataSource, ExtraPrefix)); + + var secondRun = await ExperienceSchemaMigrator.MigrateAsync( + dataSource, typeof(ExperienceSchemaMigratorTests).Assembly, ExtraPrefix, CancellationToken.None); + + // Only the pending script runs, and its $body$ block reached PostgreSQL unsubstituted. + Assert.Equal(["0002_dollar_quoted.sql"], secondRun.AppliedScripts); + Assert.Equal(["0001_marker.sql", "0002_dollar_quoted.sql"], await JournaledAsync(dataSource, ExtraPrefix)); + Assert.True(await TableExistsAsync(dataSource, "extra_dollar_quoted")); + } + + [Fact] + public async Task Failing_script_throws_naming_the_script_keeps_earlier_scripts_and_releases_the_lock() + { + await using var dataSource = await _fixture.CreateDatabaseAsync("failing"); + + var ex = await Assert.ThrowsAsync(() => ExperienceSchemaMigrator.MigrateAsync( + dataSource, typeof(ExperienceSchemaMigratorTests).Assembly, FailingPrefix, CancellationToken.None)); + + Assert.Contains("0002_broken.sql", ex.Message, StringComparison.Ordinal); + Assert.DoesNotContain("table_that_does_not_exist", ex.Message, StringComparison.Ordinal); + Assert.DoesNotContain("SELECT", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.NotNull(ex.InnerException); + + // The script before the failure stays applied and journaled; the failing one rolled back. + Assert.Equal(["0001_marker.sql"], await JournaledAsync(dataSource, FailingPrefix)); + Assert.True(await TableExistsAsync(dataSource, "migration_marker")); + Assert.Equal(0, await AdvisoryLockCountAsync(dataSource)); + + // Lock released, so the next run gets in and does its work. + var recovered = await ExperienceSchemaMigrator.MigrateAsync(dataSource, CancellationToken.None); + Assert.Equal(PostgresExperienceRecordSchema.ScriptNames, recovered.AppliedScripts); + } + + [Fact] + public async Task Token_cancelled_before_the_call_throws_OperationCanceledException_unwrapped() + { + await using var dataSource = await _fixture.CreateDatabaseAsync("precancelled"); + using var cts = new CancellationTokenSource(); + await cts.CancelAsync(); + + var ex = await Assert.ThrowsAnyAsync( + () => ExperienceSchemaMigrator.MigrateAsync(dataSource, cts.Token)); + + Assert.IsNotType(ex); + Assert.False(await SchemaExistsAsync(dataSource)); + } + + [Fact] + public async Task Token_cancelled_while_waiting_for_the_lock_throws_OperationCanceledException_and_leaves_no_lock() + { + await using var dataSource = await _fixture.CreateDatabaseAsync("cancelwait"); + + // Hold the migrator's advisory lock from outside, so the call blocks on the wait. + await using var holder = await dataSource.OpenConnectionAsync(); + await AdvisoryLockAsync(holder, "pg_advisory_lock"); + + using var cts = new CancellationTokenSource(); + var migrating = ExperienceSchemaMigrator.MigrateAsync(dataSource, cts.Token); + await WaitForAdvisoryLockWaiterAsync(dataSource, expected: 1); + await cts.CancelAsync(); + + var ex = await Assert.ThrowsAnyAsync(() => migrating); + Assert.IsNotType(ex); + Assert.False(await SchemaExistsAsync(dataSource)); + + await AdvisoryLockAsync(holder, "pg_advisory_unlock"); + + // The cancelled call left nothing held, so a later run takes the lock and completes. + Assert.Equal(0, await AdvisoryLockCountAsync(dataSource)); + var result = await ExperienceSchemaMigrator.MigrateAsync(dataSource, CancellationToken.None); + Assert.Equal(PostgresExperienceRecordSchema.ScriptNames, result.AppliedScripts); + } + + [Fact] + public async Task Null_data_source_throws_ArgumentNullException() + { + await Assert.ThrowsAsync( + () => ExperienceSchemaMigrator.MigrateAsync(null!, CancellationToken.None)); + } + + private static async Task> JournaledAsync(NpgsqlDataSource dataSource, string resourcePrefix) + { + await using var command = dataSource.CreateCommand( + "SELECT scriptname FROM agent_experience.schema_versions ORDER BY scriptname"); + await using var reader = await command.ExecuteReaderAsync(); + + var names = new List(); + while (await reader.ReadAsync()) + { + var name = reader.GetString(0); + Assert.StartsWith(resourcePrefix, name, StringComparison.Ordinal); + names.Add(name[resourcePrefix.Length..]); + } + + return names; + } + + private static async Task> JournalAppliedAtAsync(NpgsqlDataSource dataSource) + { + await using var command = dataSource.CreateCommand( + "SELECT applied FROM agent_experience.schema_versions ORDER BY scriptname"); + await using var reader = await command.ExecuteReaderAsync(); + + var applied = new List(); + while (await reader.ReadAsync()) + { + applied.Add(reader.GetDateTime(0)); + } + + return applied; + } + + private static async Task AdvisoryLockCountAsync(NpgsqlDataSource dataSource) + { + await using var command = dataSource.CreateCommand( + "SELECT count(*) FROM pg_locks WHERE locktype = 'advisory' " + + "AND database = (SELECT oid FROM pg_database WHERE datname = current_database())"); + return (int)(long)(await command.ExecuteScalarAsync())!; + } + + private static async Task SchemaExistsAsync(NpgsqlDataSource dataSource) + { + await using var command = dataSource.CreateCommand( + "SELECT to_regnamespace('agent_experience') IS NOT NULL"); + return (bool)(await command.ExecuteScalarAsync())!; + } + + private static async Task TableExistsAsync(NpgsqlDataSource dataSource, string tableName) + { + await using var command = dataSource.CreateCommand( + "SELECT to_regclass('agent_experience.' || @table) IS NOT NULL"); + command.Parameters.Add(new NpgsqlParameter("table", tableName)); + return (bool)(await command.ExecuteScalarAsync())!; + } + + /// Takes or releases the migrator's advisory lock from outside, on a caller-owned connection. + private static async Task AdvisoryLockAsync(NpgsqlConnection connection, string function) + { + await using var command = new NpgsqlCommand($"SELECT {function}(@key)", connection); + command.Parameters.Add(new NpgsqlParameter("key", ExperienceSchemaMigrator.AdvisoryLockKey)); + await command.ExecuteNonQueryAsync(); + } + + /// Waits until connections are actually blocked on the advisory lock. + private static async Task WaitForAdvisoryLockWaiterAsync(NpgsqlDataSource dataSource, int expected) + { + for (var attempt = 0; attempt < 200; attempt++) + { + await using var command = dataSource.CreateCommand( + "SELECT count(*) FROM pg_locks WHERE locktype = 'advisory' AND NOT granted " + + "AND database = (SELECT oid FROM pg_database WHERE datname = current_database())"); + if ((long)(await command.ExecuteScalarAsync())! >= expected) + { + return; + } + + await Task.Delay(50); + } + + Assert.Fail($"Fewer than {expected} migration runs blocked on the advisory lock."); + } +} diff --git a/tests/AgentExperience.Storage.Postgres.Tests/ExtraMigrations/0001_marker.sql b/tests/AgentExperience.Storage.Postgres.Tests/ExtraMigrations/0001_marker.sql new file mode 100644 index 0000000..52247d4 --- /dev/null +++ b/tests/AgentExperience.Storage.Postgres.Tests/ExtraMigrations/0001_marker.sql @@ -0,0 +1,4 @@ +-- Test-only script: the already-journaled script a later run must not reapply. +CREATE TABLE IF NOT EXISTS agent_experience.extra_marker ( + id integer NOT NULL +); diff --git a/tests/AgentExperience.Storage.Postgres.Tests/ExtraMigrations/0002_dollar_quoted.sql b/tests/AgentExperience.Storage.Postgres.Tests/ExtraMigrations/0002_dollar_quoted.sql new file mode 100644 index 0000000..a4cfbfa --- /dev/null +++ b/tests/AgentExperience.Storage.Postgres.Tests/ExtraMigrations/0002_dollar_quoted.sql @@ -0,0 +1,7 @@ +-- Test-only script: a $body$-quoted DO block. It only runs unchanged while DbUp variable substitution +-- is disabled; with variables enabled, $body$ is read as a variable and the script fails. +DO $body$ +BEGIN + EXECUTE 'CREATE TABLE IF NOT EXISTS agent_experience.extra_dollar_quoted (id integer NOT NULL)'; +END +$body$; diff --git a/tests/AgentExperience.Storage.Postgres.Tests/FailingMigrations/0001_marker.sql b/tests/AgentExperience.Storage.Postgres.Tests/FailingMigrations/0001_marker.sql new file mode 100644 index 0000000..0ad3282 --- /dev/null +++ b/tests/AgentExperience.Storage.Postgres.Tests/FailingMigrations/0001_marker.sql @@ -0,0 +1,4 @@ +-- Test-only script: proves scripts applied before a failure stay applied and journaled. +CREATE TABLE IF NOT EXISTS agent_experience.migration_marker ( + id integer NOT NULL +); diff --git a/tests/AgentExperience.Storage.Postgres.Tests/FailingMigrations/0002_broken.sql b/tests/AgentExperience.Storage.Postgres.Tests/FailingMigrations/0002_broken.sql new file mode 100644 index 0000000..3c4ac26 --- /dev/null +++ b/tests/AgentExperience.Storage.Postgres.Tests/FailingMigrations/0002_broken.sql @@ -0,0 +1,2 @@ +-- Test-only script: fails at execution time (undefined_table, 42P01). Never shipped in the package. +SELECT * FROM agent_experience.table_that_does_not_exist; diff --git a/tests/AgentExperience.Storage.Postgres.Tests/OfflineStoreTests.cs b/tests/AgentExperience.Storage.Postgres.Tests/OfflineStoreTests.cs index 67153e1..2661ada 100644 --- a/tests/AgentExperience.Storage.Postgres.Tests/OfflineStoreTests.cs +++ b/tests/AgentExperience.Storage.Postgres.Tests/OfflineStoreTests.cs @@ -276,6 +276,38 @@ public async Task Null_arguments_throw_ArgumentNullException() Assert.Throws(() => new PostgresExperienceRecordStore(null!)); } + [Fact] + public async Task Unavailable_database_makes_the_migrator_throw_ExperienceStoreException_with_the_original_failure() + { + var ex = await Assert.ThrowsAsync( + () => ExperienceSchemaMigrator.MigrateAsync(_dataSource, CancellationToken.None)); + + Assert.NotNull(ex.InnerException); + } + + [Fact] + public void Advisory_lock_key_is_pinned() + { + // Changing this silently stops serializing against hosts still running the previous package + // version, so two of them could apply the same script at once. Treat a change as breaking. + Assert.Equal(0x4147455850455201L, ExperienceSchemaMigrator.AdvisoryLockKey); + } + + [Fact] + public void Embedded_migration_resources_match_the_declared_script_names() + { + // The migrator scans by resource prefix while callers read ScriptNames, so the two must agree: + // an embedded script missing from ScriptNames (or the reverse) would go unnoticed otherwise. + const string ResourcePrefix = "AgentExperience.Storage.Postgres.Migrations."; + + var embedded = typeof(PostgresExperienceRecordSchema).Assembly.GetManifestResourceNames() + .Where(name => name.StartsWith(ResourcePrefix, StringComparison.Ordinal) && name.EndsWith(".sql", StringComparison.Ordinal)) + .Select(name => name[ResourcePrefix.Length..]) + .Order(StringComparer.Ordinal); + + Assert.Equal(PostgresExperienceRecordSchema.ScriptNames.Order(StringComparer.Ordinal), embedded); + } + [Fact] public void Embedded_schema_script_is_available_and_creates_the_versioned_table() { diff --git a/tests/AgentExperience.Storage.Postgres.Tests/PostgresFixture.cs b/tests/AgentExperience.Storage.Postgres.Tests/PostgresFixture.cs index 106d4d6..0e42f9b 100644 --- a/tests/AgentExperience.Storage.Postgres.Tests/PostgresFixture.cs +++ b/tests/AgentExperience.Storage.Postgres.Tests/PostgresFixture.cs @@ -1,13 +1,14 @@ -using AgentExperience.Storage.Postgres; using Npgsql; using Testcontainers.PostgreSql; namespace AgentExperience.Storage.Postgres.Tests; /// -/// Starts one ephemeral pgvector/pgvector:pg16 container for the whole collection, applies the -/// package's embedded schema scripts in order, and tears the container down afterwards. Set -/// TESTCONTAINERS_RYUK_DISABLED=true if Ryuk fails under a local Docker setup. +/// Starts one ephemeral pgvector/pgvector:pg16 container for the whole collection, migrates its +/// default database with , and tears the container down +/// afterwards. Migrator tests create their own databases in the same container through +/// . Set TESTCONTAINERS_RYUK_DISABLED=true if Ryuk fails under +/// a local Docker setup. /// public sealed class PostgresFixture : IAsyncLifetime { @@ -22,11 +23,35 @@ public async Task InitializeAsync() await _container.StartAsync(); _dataSource = NpgsqlDataSource.Create(_container.GetConnectionString()); - foreach (var scriptName in PostgresExperienceRecordSchema.ScriptNames) + await ExperienceSchemaMigrator.MigrateAsync(_dataSource, CancellationToken.None); + } + + /// + /// Creates an empty database in the shared container and returns a data source for it. The caller + /// owns the data source and disposes it; the database goes away with the container. + /// + /// + /// A short name fragment identifying the test: 1 to 20 lower-case ASCII letters, digits, or + /// underscores. The bound keeps the generated identifier inside PostgreSQL's 63-byte limit, so two + /// tests can never be truncated onto the same database. + /// + public async Task CreateDatabaseAsync(string purpose) + { + Assert.InRange(purpose.Length, 1, 20); + Assert.All(purpose, c => Assert.True(c is (>= 'a' and <= 'z') or (>= '0' and <= '9') or '_', $"Invalid purpose character '{c}'.")); + + // "aen_" + <=20 + "_" + 32 hex = at most 57 bytes, inside PostgreSQL's 63-byte identifier limit. + var name = $"aen_{purpose}_{Guid.NewGuid():N}"; + + // CREATE DATABASE takes no parameters and cannot run inside a transaction, so the identifier is + // interpolated. Every character of it has just been checked against the allowlist above. + await using (var command = DataSource.CreateCommand($"CREATE DATABASE \"{name}\"")) { - await using var command = _dataSource.CreateCommand(PostgresExperienceRecordSchema.GetScript(scriptName)); await command.ExecuteNonQueryAsync(); } + + var builder = new NpgsqlConnectionStringBuilder(_container!.GetConnectionString()) { Database = name }; + return NpgsqlDataSource.Create(builder.ConnectionString); } public async Task DisposeAsync() diff --git a/tests/AgentExperience.Storage.Postgres.Tests/packages.lock.json b/tests/AgentExperience.Storage.Postgres.Tests/packages.lock.json index f828891..414661c 100644 --- a/tests/AgentExperience.Storage.Postgres.Tests/packages.lock.json +++ b/tests/AgentExperience.Storage.Postgres.Tests/packages.lock.json @@ -43,6 +43,23 @@ "resolved": "2.7.0", "contentHash": "U+12df8UEWHgBi04YVf/Lgi2dy3SItlIYvHjjEVa/BngCQIzDCDRBk50DDByCfDvSbe5pRNFr3b7UrVK2kMcLw==" }, + "dbup-core": { + "type": "Transitive", + "resolved": "6.1.1", + "contentHash": "kgpuyJVEFJHoIj/slnc994Go88aoeZqNDfGHDBr4sh7CsEWwJhOTCt/FJqO4ziUImL5L0NEY0kxxOiNgPKI2Fw==", + "dependencies": { + "Microsoft.Extensions.Logging.Abstractions": "8.0.0" + } + }, + "dbup-postgresql": { + "type": "Transitive", + "resolved": "7.0.1", + "contentHash": "mRnmENWWPuuMZ538gOd1mZnzucx6FQk0anmw3EABjGfcbp24FDb9QdGepYrDiaM8K9s5/gd49+5cmBOlniH/lg==", + "dependencies": { + "Npgsql": "10.0.1", + "dbup-core": "6.1.1" + } + }, "Docker.DotNet.Enhanced": { "type": "Transitive", "resolved": "4.3.3", @@ -222,7 +239,9 @@ "type": "Project", "dependencies": { "AgentExperience.Abstractions": "[1.0.0, )", - "Npgsql": "[10.0.3, 10.0.3]" + "Npgsql": "[10.0.3, 10.0.3]", + "dbup-core": "[6.1.1, 6.1.1]", + "dbup-postgresql": "[7.0.1, 7.0.1]" } } }