NEO-146: address Bugbot findings on contract outcome stores
Ensure instance schema before outcome DDL, require matching player/instance on Postgres append, catch unique violations on concurrent duplicates, and add parity plus concurrency integration tests.pull/187/head
parent
b087c03a47
commit
7740d2145f
|
|
@ -68,8 +68,8 @@ Durable per-player **`ContractInstance`** rows (active/completed) and append-onl
|
||||||
- **Types:** `ContractInstanceStatus`, `ContractInstanceState`, `ContractInstanceIds`, `ContractInstanceReasonCodes`, `ContractOutcomeRow`, `ContractOutcomeIds`.
|
- **Types:** `ContractInstanceStatus`, `ContractInstanceState`, `ContractInstanceIds`, `ContractInstanceReasonCodes`, `ContractOutcomeRow`, `ContractOutcomeIds`.
|
||||||
- **Stores:** `IContractInstanceStore` / `IContractOutcomeStore`; in-memory + Postgres (`V011`, `V012`) when connection string set; partial unique index + app-level one-active enforcement.
|
- **Stores:** `IContractInstanceStore` / `IContractOutcomeStore`; in-memory + Postgres (`V011`, `V012`) when connection string set; partial unique index + app-level one-active enforcement.
|
||||||
- **DI:** `AddContractInstanceStores` in `Program.cs`; in-memory override in `InMemoryWebApplicationFactory`.
|
- **DI:** `AddContractInstanceStores` in `Program.cs`; in-memory override in `InMemoryWebApplicationFactory`.
|
||||||
- **Tests:** `InMemoryContractInstanceStoreTests`, `InMemoryContractOutcomeStoreTests`, Postgres persistence integration tests; host DI smoke (`867` tests green).
|
- **Tests:** `InMemoryContractInstanceStoreTests`, `InMemoryContractOutcomeStoreTests`, Postgres persistence integration tests; host DI smoke (`871` tests green).
|
||||||
- **In-memory outcome guard:** `InMemoryContractOutcomeStore.TryAppend` denies when **`IContractInstanceStore.TryGet`** is false (Postgres FK parity).
|
- **In-memory outcome guard:** `InMemoryContractOutcomeStore.TryAppend` denies when **`IContractInstanceStore.TryGet`** is false (Postgres FK + player/instance SQL guard parity).
|
||||||
- **Docs:** `server/README.md` contract instance + outcome store sections.
|
- **Docs:** `server/README.md` contract instance + outcome store sections.
|
||||||
- **Bruno:** `bruno/neon-sprawl-server/contract-stores/` health smoke (startup-only; no contract HTTP in E7M4-03).
|
- **Bruno:** `bruno/neon-sprawl-server/contract-stores/` health smoke (startup-only; no contract HTTP in E7M4-03).
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -129,6 +129,104 @@ public sealed class ContractOutcomePersistenceIntegrationTests(PostgresIntegrati
|
||||||
Assert.False(appended);
|
Assert.False(appended);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[RequirePostgresFact]
|
||||||
|
public async Task TryAppend_ShouldReturnFalse_WhenPlayerIdDoesNotMatchInstanceOwner()
|
||||||
|
{
|
||||||
|
// Arrange
|
||||||
|
await ResetContractTablesAsync();
|
||||||
|
const string otherPlayerId = "other-player-pg-001";
|
||||||
|
var idempotencyKey = ContractOutcomeIds.MakeIdempotencyKey(otherPlayerId, InstanceId);
|
||||||
|
var outcomeRow = new ContractOutcomeRow(
|
||||||
|
OutcomeRowId,
|
||||||
|
InstanceId,
|
||||||
|
otherPlayerId,
|
||||||
|
idempotencyKey,
|
||||||
|
[new RewardItemGrantApplied("scrap_metal_bulk", 5)],
|
||||||
|
[],
|
||||||
|
[],
|
||||||
|
RecordedAt);
|
||||||
|
using (var scope = Factory.Services.CreateScope())
|
||||||
|
{
|
||||||
|
var instanceStore = scope.ServiceProvider.GetRequiredService<IContractInstanceStore>();
|
||||||
|
var outcomeStore = scope.ServiceProvider.GetRequiredService<IContractOutcomeStore>();
|
||||||
|
Assert.True(instanceStore.TryCreateActive(
|
||||||
|
PlayerId,
|
||||||
|
InstanceId,
|
||||||
|
TemplateId,
|
||||||
|
SeedBucket,
|
||||||
|
IssuedAt,
|
||||||
|
out _));
|
||||||
|
await SeedPostgresPlayerAsync(otherPlayerId);
|
||||||
|
// Act
|
||||||
|
var appended = outcomeStore.TryAppend(outcomeRow);
|
||||||
|
// Assert
|
||||||
|
Assert.False(appended);
|
||||||
|
Assert.Empty(outcomeStore.GetOutcomesForInstance(InstanceId));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
[RequirePostgresFact]
|
||||||
|
public async Task TryAppend_ShouldAllowOnlyOneSuccess_WhenConcurrentDuplicateRowsRace()
|
||||||
|
{
|
||||||
|
// Arrange
|
||||||
|
await ResetContractTablesAsync();
|
||||||
|
var idempotencyKey = ContractOutcomeIds.MakeIdempotencyKey(PlayerId, InstanceId);
|
||||||
|
var outcomeRow = new ContractOutcomeRow(
|
||||||
|
OutcomeRowId,
|
||||||
|
InstanceId,
|
||||||
|
PlayerId,
|
||||||
|
idempotencyKey,
|
||||||
|
[new RewardItemGrantApplied("scrap_metal_bulk", 5)],
|
||||||
|
[],
|
||||||
|
[],
|
||||||
|
RecordedAt);
|
||||||
|
using (var scope = Factory.Services.CreateScope())
|
||||||
|
{
|
||||||
|
var instanceStore = scope.ServiceProvider.GetRequiredService<IContractInstanceStore>();
|
||||||
|
Assert.True(instanceStore.TryCreateActive(
|
||||||
|
PlayerId,
|
||||||
|
InstanceId,
|
||||||
|
TemplateId,
|
||||||
|
SeedBucket,
|
||||||
|
IssuedAt,
|
||||||
|
out _));
|
||||||
|
}
|
||||||
|
|
||||||
|
// Act
|
||||||
|
var results = await Task.WhenAll(
|
||||||
|
Enumerable.Range(0, 8).Select(_ => Task.Run(() =>
|
||||||
|
{
|
||||||
|
using var scope = Factory.Services.CreateScope();
|
||||||
|
var outcomeStore = scope.ServiceProvider.GetRequiredService<IContractOutcomeStore>();
|
||||||
|
return outcomeStore.TryAppend(outcomeRow);
|
||||||
|
})));
|
||||||
|
|
||||||
|
// Assert
|
||||||
|
Assert.Equal(1, results.Count(static r => r));
|
||||||
|
Assert.Equal(7, results.Count(static r => !r));
|
||||||
|
}
|
||||||
|
|
||||||
|
private static async Task SeedPostgresPlayerAsync(string playerId)
|
||||||
|
{
|
||||||
|
var cs = Environment.GetEnvironmentVariable("ConnectionStrings__NeonSprawl");
|
||||||
|
if (string.IsNullOrWhiteSpace(cs))
|
||||||
|
{
|
||||||
|
throw new InvalidOperationException("ConnectionStrings__NeonSprawl is not set.");
|
||||||
|
}
|
||||||
|
|
||||||
|
await using var conn = new NpgsqlConnection(cs);
|
||||||
|
await conn.OpenAsync();
|
||||||
|
await using var cmd = new NpgsqlCommand(
|
||||||
|
"""
|
||||||
|
INSERT INTO player_position (player_id, pos_x, pos_y, pos_z, sequence)
|
||||||
|
VALUES (@pid, 0, 0, 0, 0)
|
||||||
|
ON CONFLICT (player_id) DO NOTHING;
|
||||||
|
""",
|
||||||
|
conn);
|
||||||
|
cmd.Parameters.AddWithValue("pid", playerId);
|
||||||
|
await cmd.ExecuteNonQueryAsync();
|
||||||
|
}
|
||||||
|
|
||||||
private async Task ResetContractTablesAsync()
|
private async Task ResetContractTablesAsync()
|
||||||
{
|
{
|
||||||
var cs = Environment.GetEnvironmentVariable("ConnectionStrings__NeonSprawl");
|
var cs = Environment.GetEnvironmentVariable("ConnectionStrings__NeonSprawl");
|
||||||
|
|
|
||||||
|
|
@ -93,6 +93,20 @@ public sealed class InMemoryContractOutcomeStoreTests
|
||||||
Assert.Empty(store.GetOutcomesForInstance(InstanceId));
|
Assert.Empty(store.GetOutcomesForInstance(InstanceId));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
[Fact]
|
||||||
|
public void TryAppend_ShouldReturnFalse_WhenPlayerIdDoesNotMatchInstanceOwner()
|
||||||
|
{
|
||||||
|
// Arrange
|
||||||
|
var (instanceStore, store) = CreateStores();
|
||||||
|
SeedActiveInstance(instanceStore);
|
||||||
|
var row = CreatePrototypeOutcome() with { PlayerId = "other-player-xyz" };
|
||||||
|
// Act
|
||||||
|
var appended = store.TryAppend(row);
|
||||||
|
// Assert
|
||||||
|
Assert.False(appended);
|
||||||
|
Assert.Empty(store.GetOutcomesForInstance(InstanceId));
|
||||||
|
}
|
||||||
|
|
||||||
private static (InMemoryContractInstanceStore InstanceStore, InMemoryContractOutcomeStore OutcomeStore) CreateStores()
|
private static (InMemoryContractInstanceStore InstanceStore, InMemoryContractOutcomeStore OutcomeStore) CreateStores()
|
||||||
{
|
{
|
||||||
var options = Options.Create(new GamePositionOptions { DevPlayerId = PlayerId });
|
var options = Options.Create(new GamePositionOptions { DevPlayerId = PlayerId });
|
||||||
|
|
|
||||||
|
|
@ -13,6 +13,8 @@ public static class PostgresContractOutcomeBootstrap
|
||||||
|
|
||||||
public static void EnsureSchema(Npgsql.NpgsqlDataSource dataSource)
|
public static void EnsureSchema(Npgsql.NpgsqlDataSource dataSource)
|
||||||
{
|
{
|
||||||
|
PostgresContractInstanceBootstrap.EnsureSchema(dataSource);
|
||||||
|
|
||||||
if (Volatile.Read(ref _schemaReady) != 0)
|
if (Volatile.Read(ref _schemaReady) != 0)
|
||||||
{
|
{
|
||||||
return;
|
return;
|
||||||
|
|
|
||||||
|
|
@ -29,7 +29,10 @@ public sealed class PostgresContractOutcomeStore(Npgsql.NpgsqlDataSource dataSou
|
||||||
SELECT @id, @instance_id, @pid, @key, @items::jsonb, @skill_xp::jsonb, @rep::jsonb, @recorded
|
SELECT @id, @instance_id, @pid, @key, @items::jsonb, @skill_xp::jsonb, @rep::jsonb, @recorded
|
||||||
WHERE NOT EXISTS (
|
WHERE NOT EXISTS (
|
||||||
SELECT 1 FROM contract_outcome
|
SELECT 1 FROM contract_outcome
|
||||||
WHERE id = @id OR idempotency_key = @key);
|
WHERE id = @id OR idempotency_key = @key)
|
||||||
|
AND EXISTS (
|
||||||
|
SELECT 1 FROM contract_instance
|
||||||
|
WHERE contract_instance_id = @instance_id AND player_id = @pid);
|
||||||
""",
|
""",
|
||||||
conn);
|
conn);
|
||||||
cmd.Parameters.AddWithValue("id", normalized.Id);
|
cmd.Parameters.AddWithValue("id", normalized.Id);
|
||||||
|
|
@ -44,7 +47,9 @@ public sealed class PostgresContractOutcomeStore(Npgsql.NpgsqlDataSource dataSou
|
||||||
{
|
{
|
||||||
return cmd.ExecuteNonQuery() == 1;
|
return cmd.ExecuteNonQuery() == 1;
|
||||||
}
|
}
|
||||||
catch (Npgsql.PostgresException ex) when (ex.SqlState == Npgsql.PostgresErrorCodes.ForeignKeyViolation)
|
catch (Npgsql.PostgresException ex) when (
|
||||||
|
ex.SqlState == Npgsql.PostgresErrorCodes.ForeignKeyViolation ||
|
||||||
|
ex.SqlState == Npgsql.PostgresErrorCodes.UniqueViolation)
|
||||||
{
|
{
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -292,7 +292,7 @@ Append-only contract completion audit rows live in **`IContractOutcomeStore`**.
|
||||||
|
|
||||||
**Store interface methods:**
|
**Store interface methods:**
|
||||||
|
|
||||||
- **`TryAppend`** — first row id or idempotency key returns `true`; duplicates return `false`. Missing **`contract_instance`** row returns `false` (Postgres FK; in-memory checks **`IContractInstanceStore.TryGet`**) — orchestrators (NEO-149) must create the instance before appending an outcome.
|
- **`TryAppend`** — first row id or idempotency key returns `true`; duplicates return `false`. Missing **`contract_instance`** row, or a **`player_id`** that does not match the instance owner, returns `false` (Postgres SQL guard + FK/unique catch; in-memory checks **`IContractInstanceStore.TryGet`**) — orchestrators (NEO-149) must create the instance before appending an outcome.
|
||||||
- **`TryGetByIdempotencyKey`** — read one outcome for delivery replay / audit tests.
|
- **`TryGetByIdempotencyKey`** — read one outcome for delivery replay / audit tests.
|
||||||
- **`GetOutcomesForInstance`** — list outcomes for one contract instance id (ordered by recorded time).
|
- **`GetOutcomesForInstance`** — list outcomes for one contract instance id (ordered by recorded time).
|
||||||
- **`TryClearForInstance`** — dev fixture only.
|
- **`TryClearForInstance`** — dev fixture only.
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue