NEO-146: address code review findings for contract stores
Add Postgres race/deny tests, duplicate instance id coverage, FK-safe outcome append, MakeInstanceKey locking, decomposition alignment updates, and strike-through review doc.pull/187/head
parent
be53d80705
commit
1922900477
File diff suppressed because one or more lines are too long
|
|
@ -118,7 +118,7 @@ Gameplay **content and curves** default to **boot load** with optional **dev-onl
|
|||
**E7.M3 note:** Epic 7 **Slice 3** backlog in Linear ([Epic 7 — Questing, Narrative, and Faction Reputation](https://linear.app/neon-sprawl/project/epic-7-questing-narrative-and-faction-reputation-a9416783ceee)): [NEO-133](https://linear.app/neon-sprawl/issue/NEO-133) → [NEO-143](https://linear.app/neon-sprawl/issue/NEO-143) **landed**; label **`E7.M3`**. See [E7M3-pre-production-backlog.md](../../plans/E7M3-pre-production-backlog.md), [E7_M3_FactionReputationLedger.md](E7_M3_FactionReputationLedger.md). Upstream: E7.M1 **Ready**, E7.M2 **Ready**. Prototype spine: two frozen factions; operator chain grants **+15 Grid Operators**; **`prototype_quest_grid_contract`** gated at **minStanding 15**; rep grants via **`completionRewardBundle.reputationGrants`**; accept deny **`faction_gate_blocked`**. **E7M3-11 / NEO-143** client capstone landed — [`NEO-143` manual QA](../../manual-qa/NEO-143.md), [client README — End-to-end faction reputation loop (NEO-143)](../../../client/README.md#end-to-end-faction-reputation-loop-neo-143), plan [NEO-143](../../plans/NEO-143-implementation-plan.md). Epic 7 Slice 3 client capstone complete.
|
||||
| E7.M4 | ContractMissionGenerator | E4.M1, E5.M3, E7.M3 | ContractTemplate, ContractSeed, ContractOutcome | Pre-production | In Progress |
|
||||
|
||||
**E7.M4 note:** Epic 7 **Slice 4** backlog in Linear ([Epic 7 — Questing, Narrative, and Faction Reputation](https://linear.app/neon-sprawl/project/epic-7-questing-narrative-and-faction-reputation-a9416783ceee)): [NEO-144](https://linear.app/neon-sprawl/issue/NEO-144) → [NEO-154](https://linear.app/neon-sprawl/issue/NEO-154); label **`E7.M4`**. See [E7M4-pre-production-backlog.md](../../plans/E7M4-pre-production-backlog.md), [E7_M4_ContractMissionGenerator.md](E7_M4_ContractMissionGenerator.md). **E7M4-01 / NEO-144** catalog + CI landed; **E7M4-02 / NEO-145** fail-fast server catalog load + **`IContractTemplateRegistry`** landed ([NEO-145 plan](../../plans/NEO-145-implementation-plan.md)). Upstream: E5.M3 **Ready**, E7.M3 **Ready**; E4.M1 live zone read deferred (content-only **`zoneDifficultyBand`** in Slice 4 v1). Prototype spine: one template **`prototype_contract_clear_combat_pocket`** → **`prototype_combat_pocket`** encounter clear → repeat **`scrap_metal_bulk` ×5** + salvage XP; economy band caps at issue. Client capstone **E7M4-11** [NEO-154](https://linear.app/neon-sprawl/issue/NEO-154).
|
||||
**E7.M4 note:** Epic 7 **Slice 4** backlog in Linear ([Epic 7 — Questing, Narrative, and Faction Reputation](https://linear.app/neon-sprawl/project/epic-7-questing-narrative-and-faction-reputation-a9416783ceee)): [NEO-144](https://linear.app/neon-sprawl/issue/NEO-144) → [NEO-154](https://linear.app/neon-sprawl/issue/NEO-154); label **`E7.M4`**. See [E7M4-pre-production-backlog.md](../../plans/E7M4-pre-production-backlog.md), [E7_M4_ContractMissionGenerator.md](E7_M4_ContractMissionGenerator.md). **E7M4-01 / NEO-144** catalog + CI landed; **E7M4-02 / NEO-145** fail-fast server catalog load + **`IContractTemplateRegistry`** landed ([NEO-145 plan](../../plans/NEO-145-implementation-plan.md)); **E7M4-03 / NEO-146** contract instance + outcome stores landed ([NEO-146 plan](../../plans/NEO-146-implementation-plan.md)). Upstream: E5.M3 **Ready**, E7.M3 **Ready**; E4.M1 live zone read deferred (content-only **`zoneDifficultyBand`** in Slice 4 v1). Prototype spine: one template **`prototype_contract_clear_combat_pocket`** → **`prototype_combat_pocket`** encounter clear → repeat **`scrap_metal_bulk` ×5** + salvage XP; economy band caps at issue. Client capstone **E7M4-11** [NEO-154](https://linear.app/neon-sprawl/issue/NEO-154).
|
||||
|
||||
### Epic 8 — Social / Guild
|
||||
|
||||
|
|
|
|||
|
|
@ -68,8 +68,9 @@ 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 (`862` tests green).
|
||||
- **Tests:** `InMemoryContractInstanceStoreTests`, `InMemoryContractOutcomeStoreTests`, Postgres persistence integration tests; host DI smoke (`866` tests green).
|
||||
- **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).
|
||||
|
||||
## Technical approach
|
||||
|
||||
|
|
@ -79,7 +80,7 @@ Durable per-player **`ContractInstance`** rows (active/completed) and append-onl
|
|||
|------|------|
|
||||
| `ContractInstanceStatus` | `Active`, `Completed`, `Expired` (no expiry transition in E7M4-03) |
|
||||
| `ContractInstanceState` | Immutable snapshot: `ContractInstanceId`, `TemplateId`, `PlayerId`, `Status`, `SeedBucket`, `IssuedAt`, `CompletedAt?` |
|
||||
| `ContractInstanceIds` | `NormalizePlayerId`, `NormalizeContractInstanceId`, `MakeInstanceKey`, `MakeActivePlayerKey` |
|
||||
| `ContractInstanceIds` | `NormalizePlayerId`, `NormalizeContractInstanceId`, `MakeInstanceKey` (per-row lock key) |
|
||||
| `ContractInstanceReasonCodes` | `player_not_writable`, `active_contract_exists`, `instance_not_found`, `instance_not_active`, `invalid_ids` |
|
||||
| `ContractOutcomeRow` | `Id`, `ContractInstanceId`, `PlayerId`, `IdempotencyKey`, grant snapshots (items/skill XP/rep), `RecordedAt` |
|
||||
| `ContractOutcomeIds` | `MakeIdempotencyKey(playerId, contractInstanceId)` → `{p}:contract_complete:{i}` |
|
||||
|
|
@ -229,7 +230,7 @@ Manual Godot QA: **none** (server infrastructure; capstone is NEO-154).
|
|||
| Postgres partial unique index vs app-only one-active check | **Both** — enforce in store logic (fast deny reason) + partial unique index for race safety | `adopted` |
|
||||
| Grant snapshot on outcome before E7M4-05 router | Store accepts caller-supplied snapshot on append; E7M4-06 orchestrator fills from router result | `adopted` |
|
||||
| `Expired` status unused in Slice 4 | Keep enum for forward compatibility; no API until a future expiry story | `adopted` |
|
||||
| Dev `TryClear*` on Postgres | Implement on in-memory only first; add Postgres clear if Bruno fixture story needs it (mirror quest `TryClearProgress` on both when needed) | `deferred` |
|
||||
| Dev `TryClear*` on Postgres | Implement on in-memory only first; add Postgres clear if Bruno fixture story needs it (mirror quest `TryClearProgress` on both when needed) | `adopted` — both backends implement clear helpers |
|
||||
|
||||
## Client counterpart
|
||||
|
||||
|
|
|
|||
|
|
@ -19,12 +19,12 @@ NEO-146 adds durable per-player contract instance state (`IContractInstanceStore
|
|||
| `docs/plans/NEO-146-implementation-plan.md` | **Matches** — AC checklist complete; reconciliation reflects shipped types, stores, migrations, DI, tests, README. Minor drift: plan lists `MakeActivePlayerKey` (not implemented) and defers Postgres `TryClear*` (implemented anyway — acceptable improvement). |
|
||||
| `docs/plans/E7M4-pre-production-backlog.md` (E7M4-03) | **Matches** — dual stores, one-active policy, idempotency key shape, grant snapshots, NEO-116-style Postgres split; generator/payout/HTTP correctly out of scope. |
|
||||
| `docs/decomposition/modules/E7_M4_ContractMissionGenerator.md` | **Matches** — runtime stores paragraph + README links added (NEO-146). |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Partially matches** — E7.M4 row cites E7M4-01/NEO-144 and E7M4-02/NEO-145 only; should add E7M4-03/NEO-146 stores after merge (same pattern as NEO-145 follow-up). |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E7.M4 In Progress spine unchanged; optional one-line NEO-146 note after merge. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | ~~**Partially matches** — E7.M4 row cites E7M4-01/NEO-144 and E7M4-02/NEO-145 only; should add E7M4-03/NEO-146 stores after merge (same pattern as NEO-145 follow-up).~~ **Done.** — E7M4-03 / NEO-146 stores added to E7.M4 row. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | ~~**Partially matches** — E7.M4 In Progress spine unchanged; optional one-line NEO-146 note after merge.~~ **Done.** — E7M4-03 / NEO-146 note added. |
|
||||
| `docs/decomposition/modules/client_server_authority.md` | **N/A** — server-side persistence only; no authority boundary change. |
|
||||
| Full-stack epic decomposition | **N/A** — infrastructure-only; client counterpart correctly none (NEO-154 capstone). |
|
||||
|
||||
Register/tracking tables should be updated for E7M4-03 after merge — **should fix** (non-blocking), mirroring NEO-145 alignment follow-up.
|
||||
~~Register/tracking tables should be updated for E7M4-03 after merge — **should fix** (non-blocking), mirroring NEO-145 alignment follow-up.~~ **Done.**
|
||||
|
||||
## Blocking issues
|
||||
|
||||
|
|
@ -32,25 +32,25 @@ None.
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **Update decomposition tracking for E7M4-03** — After merge, extend `documentation_and_implementation_alignment.md` E7.M4 row (and optionally `module_dependency_register.md`) with E7M4-03: `IContractInstanceStore` + `IContractOutcomeStore`, `V011`/`V012`, README sections — same pattern as NEO-145.
|
||||
1. ~~**Update decomposition tracking for E7M4-03** — After merge, extend `documentation_and_implementation_alignment.md` E7.M4 row (and optionally `module_dependency_register.md`) with E7M4-03: `IContractInstanceStore` + `IContractOutcomeStore`, `V011`/`V012`, README sections — same pattern as NEO-145.~~ **Done.**
|
||||
|
||||
2. **Postgres one-active concurrent race** — The existing `TryCreateActive_ShouldReturnOneTrueAndRestFalse_WhenCalledConcurrently` test races on the **same** instance id (PK dedup), matching the quest-store precedent. For contracts, the more important race is **different** instance ids for the same player competing for the one-active partial unique index. Consider a second `RequirePostgresFact` that launches concurrent `TryCreateActive` with distinct instance ids and asserts exactly one `active` row remains.
|
||||
2. ~~**Postgres one-active concurrent race** — The existing `TryCreateActive_ShouldReturnOneTrueAndRestFalse_WhenCalledConcurrently` test races on the **same** instance id (PK dedup), matching the quest-store precedent. For contracts, the more important race is **different** instance ids for the same player competing for the one-active partial unique index. Consider a second `RequirePostgresFact` that launches concurrent `TryCreateActive` with distinct instance ids and asserts exactly one `active` row remains.~~ **Done.** — `TryCreateActive_ShouldAllowOnlyOneActive_WhenDistinctInstanceIdsRace`.
|
||||
|
||||
3. **Postgres outcome deny paths** — In-memory tests cover duplicate row id / idempotency key; Postgres integration only covers happy-path append. Add `RequirePostgresFact` duplicate-append deny (and optionally idempotent complete replay on Postgres) to lock SQL `INSERT … WHERE NOT EXISTS` / complete `FOR UPDATE` behavior.
|
||||
3. ~~**Postgres outcome deny paths** — In-memory tests cover duplicate row id / idempotency key; Postgres integration only covers happy-path append. Add `RequirePostgresFact` duplicate-append deny (and optionally idempotent complete replay on Postgres) to lock SQL `INSERT … WHERE NOT EXISTS` / complete `FOR UPDATE` behavior.~~ **Done.** — duplicate-append deny, missing-instance FK deny, and Postgres idempotent complete tests added.
|
||||
|
||||
4. **In-memory duplicate instance id** — Plan §7 lists duplicate instance id deny; no explicit unit test calls `TryCreateActive` twice with the same instance id (second call should return `false` with unchanged snapshot). Cheap coverage gap.
|
||||
4. ~~**In-memory duplicate instance id** — Plan §7 lists duplicate instance id deny; no explicit unit test calls `TryCreateActive` twice with the same instance id (second call should return `false` with unchanged snapshot). Cheap coverage gap.~~ **Done.** — `TryCreateActive_ShouldDenyDuplicateInstanceId`.
|
||||
|
||||
5. **Postgres outcome FK parity** — `PostgresContractOutcomeStore.TryAppend` will throw on FK violation (missing `contract_instance` row) while in-memory accepts any normalized row. Orchestrators (NEO-149) will create the instance first, so this is low risk today; document in README or catch `PostgresException` and return `false` if you want strict bool-only store semantics across backends.
|
||||
5. ~~**Postgres outcome FK parity** — `PostgresContractOutcomeStore.TryAppend` will throw on FK violation (missing `contract_instance` row) while in-memory accepts any normalized row. Orchestrators (NEO-149) will create the instance first, so this is low risk today; document in README or catch `PostgresException` and return `false` if you want strict bool-only store semantics across backends.~~ **Done.** — FK violation caught; README documents orchestrator ordering.
|
||||
|
||||
## Nits
|
||||
|
||||
- Nit: `ContractInstanceIds.MakeInstanceKey` is defined but unused; plan also lists `MakeActivePlayerKey` which was not added. Either wire keys into store indexing or trim plan/reconciliation to match shipped API.
|
||||
- ~~Nit: `ContractInstanceIds.MakeInstanceKey` is defined but unused; plan also lists `MakeActivePlayerKey` which was not added. Either wire keys into store indexing or trim plan/reconciliation to match shipped API.~~ **Done.** — `MakeInstanceKey` wired for per-row locks; plan trimmed; player-level lock for one-active create path.
|
||||
|
||||
- Nit: `ContractInstanceReasonCodes` is defined for E7M4-04+ orchestrators but unused in this slice — correct deferral; no action until NEO-147.
|
||||
|
||||
- Nit: Bruno folder `bruno/neon-sprawl-server/contract-stores/` (health smoke) is useful but not listed in plan §Implementation reconciliation — optional one-line note for doc parity (same as NEO-145 Bruno nit).
|
||||
- ~~Nit: Bruno folder `bruno/neon-sprawl-server/contract-stores/` (health smoke) is useful but not listed in plan §Implementation reconciliation — optional one-line note for doc parity (same as NEO-145 Bruno nit).~~ **Done.** — plan reconciliation updated.
|
||||
|
||||
- Nit: Plan open questions marked Postgres `TryClear*` as deferred; both Postgres stores implement clear helpers anyway — update plan risk row to `adopted` on next doc pass.
|
||||
- ~~Nit: Plan open questions marked Postgres `TryClear*` as deferred; both Postgres stores implement clear helpers anyway — update plan risk row to `adopted` on next doc pass.~~ **Done.**
|
||||
|
||||
## Verification
|
||||
|
||||
|
|
@ -67,4 +67,4 @@ Author should run Postgres integration locally when `ConnectionStrings__NeonSpra
|
|||
dotnet test NeonSprawl.sln --filter "FullyQualifiedName~ContractInstancePersistence|FullyQualifiedName~ContractOutcomePersistence"
|
||||
```
|
||||
|
||||
Before merge: skim decomposition alignment suggestion — functional code is merge-ready without it.
|
||||
~~Before merge: skim decomposition alignment suggestion — functional code is merge-ready without it.~~ **Done** — review follow-up committed.
|
||||
|
|
|
|||
|
|
@ -53,6 +53,70 @@ public sealed class ContractInstancePersistenceIntegrationTests(PostgresIntegrat
|
|||
Assert.Equal(TemplateId, snapshot.TemplateId);
|
||||
}
|
||||
|
||||
[RequirePostgresFact]
|
||||
public async Task TryCreateActive_ShouldAllowOnlyOneActive_WhenDistinctInstanceIdsRace()
|
||||
{
|
||||
// Arrange
|
||||
await ResetContractInstanceTableAsync();
|
||||
const int concurrentCalls = 8;
|
||||
|
||||
// Act
|
||||
var results = await Task.WhenAll(
|
||||
Enumerable.Range(0, concurrentCalls).Select(i => Task.Run(() =>
|
||||
{
|
||||
using var scope = Factory.Services.CreateScope();
|
||||
var store = scope.ServiceProvider.GetRequiredService<IContractInstanceStore>();
|
||||
var instanceId = $"prototype_contract_instance_pg_race_{i:D3}";
|
||||
return (
|
||||
Success: store.TryCreateActive(
|
||||
PlayerId,
|
||||
instanceId,
|
||||
TemplateId,
|
||||
SeedBucket,
|
||||
IssuedAt,
|
||||
out _),
|
||||
InstanceId: instanceId);
|
||||
})));
|
||||
|
||||
// Assert
|
||||
Assert.Equal(1, results.Count(static r => r.Success));
|
||||
Assert.Equal(concurrentCalls - 1, results.Count(static r => !r.Success));
|
||||
Assert.True(
|
||||
Factory.Services.CreateScope().ServiceProvider
|
||||
.GetRequiredService<IContractInstanceStore>()
|
||||
.TryGetActiveForPlayer(PlayerId, out var active));
|
||||
Assert.Equal(ContractInstanceStatus.Active, active.Status);
|
||||
Assert.Contains(results.Where(static r => r.Success).Select(static r => r.InstanceId), id => id == active.ContractInstanceId);
|
||||
}
|
||||
|
||||
[RequirePostgresFact]
|
||||
public async Task TryMarkComplete_ShouldBeIdempotent_OnPostgres()
|
||||
{
|
||||
// Arrange
|
||||
await ResetContractInstanceTableAsync();
|
||||
using var scope = Factory.Services.CreateScope();
|
||||
var store = scope.ServiceProvider.GetRequiredService<IContractInstanceStore>();
|
||||
Assert.True(store.TryCreateActive(
|
||||
PlayerId,
|
||||
InstanceId,
|
||||
TemplateId,
|
||||
SeedBucket,
|
||||
IssuedAt,
|
||||
out _));
|
||||
// Act
|
||||
var first = store.TryMarkComplete(PlayerId, InstanceId, CompletedAt, out var firstSnapshot);
|
||||
var second = store.TryMarkComplete(
|
||||
PlayerId,
|
||||
InstanceId,
|
||||
CompletedAt.AddHours(1),
|
||||
out var secondSnapshot);
|
||||
// Assert
|
||||
Assert.True(first);
|
||||
Assert.False(second);
|
||||
Assert.Equal(CompletedAt, secondSnapshot.CompletedAt);
|
||||
Assert.Equal(firstSnapshot.ContractInstanceId, secondSnapshot.ContractInstanceId);
|
||||
}
|
||||
|
||||
[RequirePostgresFact]
|
||||
public async Task CreateComplete_ShouldPersistAcrossNewFactory()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -71,6 +71,69 @@ public sealed class ContractOutcomePersistenceIntegrationTests(PostgresIntegrati
|
|||
Assert.Equal(15, readBack.GrantedSkillXp[0].Amount);
|
||||
}
|
||||
|
||||
[RequirePostgresFact]
|
||||
public async Task TryAppend_ShouldDenyDuplicateRowIdAndIdempotencyKey()
|
||||
{
|
||||
// Arrange
|
||||
await ResetContractTablesAsync();
|
||||
var idempotencyKey = ContractOutcomeIds.MakeIdempotencyKey(PlayerId, InstanceId);
|
||||
var outcomeRow = new ContractOutcomeRow(
|
||||
OutcomeRowId,
|
||||
InstanceId,
|
||||
PlayerId,
|
||||
idempotencyKey,
|
||||
[new RewardItemGrantApplied("scrap_metal_bulk", 5)],
|
||||
[new RewardSkillXpGrantApplied("salvage", 15)],
|
||||
[],
|
||||
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 _));
|
||||
Assert.True(outcomeStore.TryAppend(outcomeRow));
|
||||
// Act
|
||||
var duplicateId = outcomeStore.TryAppend(outcomeRow with { IdempotencyKey = "other-key" });
|
||||
var duplicateKey = outcomeStore.TryAppend(outcomeRow with { Id = "outcome-row-pg-002" });
|
||||
// Assert
|
||||
Assert.False(duplicateId);
|
||||
Assert.False(duplicateKey);
|
||||
}
|
||||
}
|
||||
|
||||
[RequirePostgresFact]
|
||||
public async Task TryAppend_ShouldReturnFalse_WhenContractInstanceMissing()
|
||||
{
|
||||
// Arrange
|
||||
await ResetContractTablesAsync();
|
||||
var idempotencyKey = ContractOutcomeIds.MakeIdempotencyKey(PlayerId, InstanceId);
|
||||
var outcomeRow = new ContractOutcomeRow(
|
||||
OutcomeRowId,
|
||||
InstanceId,
|
||||
PlayerId,
|
||||
idempotencyKey,
|
||||
[new RewardItemGrantApplied("scrap_metal_bulk", 5)],
|
||||
[],
|
||||
[],
|
||||
RecordedAt);
|
||||
// Act
|
||||
bool appended;
|
||||
using (var scope = Factory.Services.CreateScope())
|
||||
{
|
||||
var outcomeStore = scope.ServiceProvider.GetRequiredService<IContractOutcomeStore>();
|
||||
appended = outcomeStore.TryAppend(outcomeRow);
|
||||
}
|
||||
// Assert
|
||||
Assert.False(appended);
|
||||
}
|
||||
|
||||
private async Task ResetContractTablesAsync()
|
||||
{
|
||||
var cs = Environment.GetEnvironmentVariable("ConnectionStrings__NeonSprawl");
|
||||
|
|
|
|||
|
|
@ -53,6 +53,33 @@ public sealed class InMemoryContractInstanceStoreTests
|
|||
Assert.Null(snapshot.CompletedAt);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryCreateActive_ShouldDenyDuplicateInstanceId()
|
||||
{
|
||||
// Arrange
|
||||
var store = CreateStore();
|
||||
Assert.True(store.TryCreateActive(
|
||||
PlayerId,
|
||||
InstanceId,
|
||||
TemplateId,
|
||||
SeedBucket,
|
||||
IssuedAt,
|
||||
out var firstSnapshot));
|
||||
// Act
|
||||
var duplicate = store.TryCreateActive(
|
||||
PlayerId,
|
||||
InstanceId,
|
||||
TemplateId,
|
||||
SeedBucket,
|
||||
IssuedAt.AddHours(1),
|
||||
out var secondSnapshot);
|
||||
// Assert
|
||||
Assert.False(duplicate);
|
||||
Assert.Equal(firstSnapshot.ContractInstanceId, secondSnapshot.ContractInstanceId);
|
||||
Assert.Equal(firstSnapshot.IssuedAt, secondSnapshot.IssuedAt);
|
||||
Assert.Equal(ContractInstanceStatus.Active, secondSnapshot.Status);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void TryCreateActive_ShouldDenySecondActiveForSamePlayer()
|
||||
{
|
||||
|
|
|
|||
|
|
@ -48,6 +48,8 @@ public sealed class InMemoryContractInstanceStore(IOptions<GamePositionOptions>
|
|||
|
||||
private readonly ConcurrentDictionary<string, object> instanceLocks = new(StringComparer.Ordinal);
|
||||
|
||||
private static string PlayerLockKey(string player) => $"{player}\0active";
|
||||
|
||||
private static HashSet<string> CreateKnownPlayers(GamePositionOptions o)
|
||||
{
|
||||
var id = ContractInstanceIds.NormalizePlayerId(o.DevPlayerId);
|
||||
|
|
@ -77,7 +79,13 @@ public sealed class InMemoryContractInstanceStore(IOptions<GamePositionOptions>
|
|||
return false;
|
||||
}
|
||||
|
||||
lock (instanceLocks.GetOrAdd(instanceId, _ => new object()))
|
||||
var lockKey = ContractInstanceIds.MakeInstanceKey(player, instanceId);
|
||||
if (lockKey.Length == 0)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
lock (instanceLocks.GetOrAdd(lockKey, _ => new object()))
|
||||
{
|
||||
if (!byInstanceId.TryGetValue(instanceId, out var row) ||
|
||||
!string.Equals(row.PlayerId, player, StringComparison.Ordinal))
|
||||
|
|
@ -105,7 +113,7 @@ public sealed class InMemoryContractInstanceStore(IOptions<GamePositionOptions>
|
|||
return false;
|
||||
}
|
||||
|
||||
lock (instanceLocks.GetOrAdd(instanceId, _ => new object()))
|
||||
lock (instanceLocks.GetOrAdd(PlayerLockKey(player), _ => new object()))
|
||||
{
|
||||
if (!byInstanceId.TryGetValue(instanceId, out var row) ||
|
||||
row.Status != ContractInstanceStatus.Active ||
|
||||
|
|
@ -143,7 +151,7 @@ public sealed class InMemoryContractInstanceStore(IOptions<GamePositionOptions>
|
|||
return false;
|
||||
}
|
||||
|
||||
lock (instanceLocks.GetOrAdd(instanceId, _ => new object()))
|
||||
lock (instanceLocks.GetOrAdd(PlayerLockKey(player), _ => new object()))
|
||||
{
|
||||
if (activeInstanceByPlayer.TryGetValue(player, out var existingActiveId))
|
||||
{
|
||||
|
|
@ -193,7 +201,13 @@ public sealed class InMemoryContractInstanceStore(IOptions<GamePositionOptions>
|
|||
return false;
|
||||
}
|
||||
|
||||
lock (instanceLocks.GetOrAdd(instanceId, _ => new object()))
|
||||
var lockKey = ContractInstanceIds.MakeInstanceKey(player, instanceId);
|
||||
if (lockKey.Length == 0)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
lock (instanceLocks.GetOrAdd(lockKey, _ => new object()))
|
||||
{
|
||||
if (!byInstanceId.TryGetValue(instanceId, out var row) ||
|
||||
!string.Equals(row.PlayerId, player, StringComparison.Ordinal))
|
||||
|
|
@ -230,7 +244,13 @@ public sealed class InMemoryContractInstanceStore(IOptions<GamePositionOptions>
|
|||
return false;
|
||||
}
|
||||
|
||||
lock (instanceLocks.GetOrAdd(instanceId, _ => new object()))
|
||||
var lockKey = ContractInstanceIds.MakeInstanceKey(player, instanceId);
|
||||
if (lockKey.Length == 0)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
|
||||
lock (instanceLocks.GetOrAdd(lockKey, _ => new object()))
|
||||
{
|
||||
if (!byInstanceId.TryGetValue(instanceId, out var row) ||
|
||||
!string.Equals(row.PlayerId, player, StringComparison.Ordinal))
|
||||
|
|
|
|||
|
|
@ -40,8 +40,15 @@ public sealed class PostgresContractOutcomeStore(Npgsql.NpgsqlDataSource dataSou
|
|||
cmd.Parameters.AddWithValue("skill_xp", SerializeGrants(normalized.GrantedSkillXp));
|
||||
cmd.Parameters.AddWithValue("rep", SerializeGrants(normalized.GrantedReputation));
|
||||
cmd.Parameters.AddWithValue("recorded", normalized.RecordedAt);
|
||||
try
|
||||
{
|
||||
return cmd.ExecuteNonQuery() == 1;
|
||||
}
|
||||
catch (Npgsql.PostgresException ex) when (ex.SqlState == Npgsql.PostgresErrorCodes.ForeignKeyViolation)
|
||||
{
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
public bool TryGetByIdempotencyKey(string idempotencyKey, out ContractOutcomeRow row)
|
||||
|
|
|
|||
|
|
@ -292,7 +292,7 @@ Append-only contract completion audit rows live in **`IContractOutcomeStore`**.
|
|||
|
||||
**Store interface methods:**
|
||||
|
||||
- **`TryAppend`** — first row id or idempotency key returns `true`; duplicates return `false`.
|
||||
- **`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.
|
||||
- **`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.
|
||||
|
|
|
|||
Loading…
Reference in New Issue