NEO-146: address re-review in-memory outcome guard parity
InMemoryContractOutcomeStore checks IContractInstanceStore.TryGet before append; add unit test, README/plan updates, and close r2 review items.pull/187/head
parent
a9bc1acaa8
commit
b087c03a47
|
|
@ -68,7 +68,8 @@ Durable per-player **`ContractInstance`** rows (active/completed) and append-onl
|
|||
- **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.
|
||||
- **DI:** `AddContractInstanceStores` in `Program.cs`; in-memory override in `InMemoryWebApplicationFactory`.
|
||||
- **Tests:** `InMemoryContractInstanceStoreTests`, `InMemoryContractOutcomeStoreTests`, Postgres persistence integration tests; host DI smoke (`866` tests green).
|
||||
- **Tests:** `InMemoryContractInstanceStoreTests`, `InMemoryContractOutcomeStoreTests`, Postgres persistence integration tests; host DI smoke (`867` tests green).
|
||||
- **In-memory outcome guard:** `InMemoryContractOutcomeStore.TryAppend` denies when **`IContractInstanceStore.TryGet`** is false (Postgres FK parity).
|
||||
- **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).
|
||||
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@ Follow-up review after the initial NEO-146 review and a broad C# style/analyzer
|
|||
|
||||
| Document | Alignment |
|
||||
|----------|-----------|
|
||||
| `docs/plans/NEO-146-implementation-plan.md` | **Matches** — AC complete; reconciliation includes Bruno smoke, `MakeInstanceKey` locking, Postgres clear adopted, review follow-up tests. Test count says `866`; suite now **867** (minor drift). |
|
||||
| `docs/plans/NEO-146-implementation-plan.md` | **Matches** — AC complete; reconciliation includes Bruno smoke, `MakeInstanceKey` locking, Postgres clear adopted, review follow-up tests. Test count **867**. |
|
||||
| `docs/plans/E7M4-pre-production-backlog.md` (E7M4-03) | **Matches** |
|
||||
| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — runtime stores paragraph (NEO-146). |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7M4-03 / NEO-146 stores row added. |
|
||||
|
|
@ -43,13 +43,13 @@ None.
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **PR / commit scope for future stories** — Commits `b455ff5`…`01e82f3` touch ~50 files outside `Game/Contracts/` (catalog loaders, integration-test `using` layout, bootstrap `Lock` gates, mutation-outcome docs). Mechanical and green, but it widens review/merge-conflict surface for an infrastructure ticket. Consider a separate `chore:` PR for repo-wide analyzer/style passes when the diff is this large.
|
||||
1. ~~**PR / commit scope for future stories**~~ — **Intentional.** Repo-wide analyzer/style cleanup (`b455ff5`…`01e82f3`) was bundled on this branch by author request; no split `chore:` PR for E7M4-03.
|
||||
|
||||
2. **In-memory outcome instance guard (optional parity)** — Postgres `TryAppend` returns `false` when `contract_instance` is missing (FK); in-memory still accepts a normalized row without checking instance existence. README documents Postgres ordering; orchestrators (NEO-149) will create the instance first. If strict cross-backend parity matters before NEO-149, add an optional `IContractInstanceStore` lookup in `InMemoryContractOutcomeStore.TryAppend` or a shared guard helper — otherwise document the asymmetry explicitly in README (one line under in-memory storage).
|
||||
2. ~~**In-memory outcome instance guard (optional parity)**~~ — **Done.** `InMemoryContractOutcomeStore` injects **`IContractInstanceStore`** and denies **`TryAppend`** when **`TryGet`** is false; unit test + README/plan updated (Postgres FK parity).
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: Plan reconciliation cites **866** tests; re-review run reports **867** — update the count on next doc touch.
|
||||
- ~~Nit: Plan reconciliation cites **866** tests; re-review run reports **867**~~ — **Done.** Plan reconciliation updated to **867**.
|
||||
|
||||
- Nit: `ContractInstanceReasonCodes` remains unused until NEO-147 orchestrators — expected deferral; no action in E7M4-03.
|
||||
|
||||
|
|
@ -70,4 +70,4 @@ Optional Postgres integration (when `ConnectionStrings__NeonSprawl` is set):
|
|||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractInstancePersistence|FullyQualifiedName~ContractOutcomePersistence"
|
||||
```
|
||||
|
||||
Merge-ready: prior review feedback is closed; remaining items are process/parity nits only.
|
||||
Merge-ready: all suggestions addressed or intentionally accepted; remaining nits are expected deferrals only.
|
||||
|
|
|
|||
|
|
@ -1,4 +1,6 @@
|
|||
using Microsoft.Extensions.Options;
|
||||
using NeonSprawl.Server.Game.Contracts;
|
||||
using NeonSprawl.Server.Game.PositionState;
|
||||
using NeonSprawl.Server.Game.Rewards;
|
||||
using Xunit;
|
||||
|
||||
|
|
@ -9,13 +11,17 @@ public sealed class InMemoryContractOutcomeStoreTests
|
|||
private const string PlayerId = "dev-local-1";
|
||||
private const string InstanceId = "prototype_contract_instance_001";
|
||||
private const string OutcomeRowId = "outcome-row-001";
|
||||
private const string TemplateId = "prototype_contract_clear_combat_pocket";
|
||||
private const string SeedBucket = "2026-06-22";
|
||||
private static readonly DateTimeOffset IssuedAt = new(2026, 6, 22, 10, 0, 0, TimeSpan.Zero);
|
||||
private static readonly DateTimeOffset RecordedAt = new(2026, 6, 22, 12, 0, 0, TimeSpan.Zero);
|
||||
|
||||
[Fact]
|
||||
public void TryAppend_ShouldPersistAndQueryByInstanceAndIdempotencyKey()
|
||||
{
|
||||
// Arrange
|
||||
var store = new InMemoryContractOutcomeStore();
|
||||
var (instanceStore, store) = CreateStores();
|
||||
SeedActiveInstance(instanceStore);
|
||||
var row = CreatePrototypeOutcome();
|
||||
// Act
|
||||
var appended = store.TryAppend(row);
|
||||
|
|
@ -36,7 +42,8 @@ public sealed class InMemoryContractOutcomeStoreTests
|
|||
public void TryAppend_ShouldDenyDuplicateRowId()
|
||||
{
|
||||
// Arrange
|
||||
var store = new InMemoryContractOutcomeStore();
|
||||
var (instanceStore, store) = CreateStores();
|
||||
SeedActiveInstance(instanceStore);
|
||||
var row = CreatePrototypeOutcome();
|
||||
Assert.True(store.TryAppend(row));
|
||||
// Act
|
||||
|
|
@ -49,7 +56,8 @@ public sealed class InMemoryContractOutcomeStoreTests
|
|||
public void TryAppend_ShouldDenyDuplicateIdempotencyKey()
|
||||
{
|
||||
// Arrange
|
||||
var store = new InMemoryContractOutcomeStore();
|
||||
var (instanceStore, store) = CreateStores();
|
||||
SeedActiveInstance(instanceStore);
|
||||
var row = CreatePrototypeOutcome();
|
||||
Assert.True(store.TryAppend(row));
|
||||
// Act
|
||||
|
|
@ -62,7 +70,8 @@ public sealed class InMemoryContractOutcomeStoreTests
|
|||
public void TryAppend_ShouldFailClosed_WhenRequiredFieldsEmpty()
|
||||
{
|
||||
// Arrange
|
||||
var store = new InMemoryContractOutcomeStore();
|
||||
var (instanceStore, store) = CreateStores();
|
||||
SeedActiveInstance(instanceStore);
|
||||
var invalidRow = CreatePrototypeOutcome() with { PlayerId = " " };
|
||||
// Act
|
||||
var appended = store.TryAppend(invalidRow);
|
||||
|
|
@ -71,6 +80,37 @@ public sealed class InMemoryContractOutcomeStoreTests
|
|||
Assert.Empty(store.GetOutcomesForInstance(InstanceId));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryAppend_ShouldReturnFalse_WhenContractInstanceMissing()
|
||||
{
|
||||
// Arrange
|
||||
var (_, store) = CreateStores();
|
||||
var row = CreatePrototypeOutcome();
|
||||
// Act
|
||||
var appended = store.TryAppend(row);
|
||||
// Assert
|
||||
Assert.False(appended);
|
||||
Assert.Empty(store.GetOutcomesForInstance(InstanceId));
|
||||
}
|
||||
|
||||
private static (InMemoryContractInstanceStore InstanceStore, InMemoryContractOutcomeStore OutcomeStore) CreateStores()
|
||||
{
|
||||
var options = Options.Create(new GamePositionOptions { DevPlayerId = PlayerId });
|
||||
var instanceStore = new InMemoryContractInstanceStore(options);
|
||||
return (instanceStore, new InMemoryContractOutcomeStore(instanceStore));
|
||||
}
|
||||
|
||||
private static void SeedActiveInstance(InMemoryContractInstanceStore instanceStore)
|
||||
{
|
||||
Assert.True(instanceStore.TryCreateActive(
|
||||
PlayerId,
|
||||
InstanceId,
|
||||
TemplateId,
|
||||
SeedBucket,
|
||||
IssuedAt,
|
||||
out _));
|
||||
}
|
||||
|
||||
private static ContractOutcomeRow CreatePrototypeOutcome()
|
||||
{
|
||||
var idempotencyKey = ContractOutcomeIds.MakeIdempotencyKey(PlayerId, InstanceId);
|
||||
|
|
|
|||
|
|
@ -4,7 +4,7 @@ using NeonSprawl.Server.Game.Rewards;
|
|||
namespace NeonSprawl.Server.Game.Contracts;
|
||||
|
||||
/// <summary>Thread-safe in-memory append-only contract outcome audit log (NEO-146).</summary>
|
||||
public sealed class InMemoryContractOutcomeStore : IContractOutcomeStore
|
||||
public sealed class InMemoryContractOutcomeStore(IContractInstanceStore instanceStore) : IContractOutcomeStore
|
||||
{
|
||||
private readonly ConcurrentDictionary<string, ContractOutcomeRow> rowsById = new(StringComparer.Ordinal);
|
||||
|
||||
|
|
@ -20,6 +20,11 @@ public sealed class InMemoryContractOutcomeStore : IContractOutcomeStore
|
|||
return false;
|
||||
}
|
||||
|
||||
if (!instanceStore.TryGet(normalized.PlayerId, normalized.ContractInstanceId, out _))
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
lock (idLocks.GetOrAdd(normalized.Id, _ => new object()))
|
||||
{
|
||||
if (rowsById.ContainsKey(normalized.Id) ||
|
||||
|
|
|
|||
|
|
@ -292,12 +292,12 @@ Append-only contract completion audit rows live in **`IContractOutcomeStore`**.
|
|||
|
||||
**Store interface methods:**
|
||||
|
||||
- **`TryAppend`** — first row id or idempotency key returns `true`; duplicates return `false`. Missing **`contract_instance`** row (Postgres FK) also returns `false` — 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 returns `false` (Postgres FK; 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.
|
||||
- **`GetOutcomesForInstance`** — list outcomes for one contract instance id (ordered by recorded time).
|
||||
- **`TryClearForInstance`** — dev fixture only.
|
||||
|
||||
**Storage:** in-memory when Postgres is unset; **`PostgresContractOutcomeStore`** appends to **`contract_outcome`** ([`V012__contract_outcome.sql`](../db/migrations/V012__contract_outcome.sql)) with JSONB grant snapshots when configured. Plan: [NEO-146 implementation plan](../../docs/plans/NEO-146-implementation-plan.md).
|
||||
**Storage:** in-memory when Postgres is unset (**`InMemoryContractOutcomeStore`** requires a matching instance row in **`IContractInstanceStore`**); **`PostgresContractOutcomeStore`** appends to **`contract_outcome`** ([`V012__contract_outcome.sql`](../db/migrations/V012__contract_outcome.sql)) with JSONB grant snapshots when configured. Plan: [NEO-146 implementation plan](../../docs/plans/NEO-146-implementation-plan.md).
|
||||
|
||||
## Quest definitions (NEO-115)
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue