diff --git a/docs/plans/NEO-127-implementation-plan.md b/docs/plans/NEO-127-implementation-plan.md index 32fe435..1d30f34 100644 --- a/docs/plans/NEO-127-implementation-plan.md +++ b/docs/plans/NEO-127-implementation-plan.md @@ -23,7 +23,7 @@ - **Skill XP path** — call **`SkillProgressionGrantOperations.TryApplyGrant`** with **`MissionRewardSkillXpConstants.MissionRewardSourceKind`** (not the fire-and-forget **`MissionRewardSkillXpGrant`** wrapper) so deny envelopes map to router reason codes. - **Idempotent replay** — **`IRewardDeliveryStore.TryGet`** before apply; return success with existing **`RewardDeliveryEvent`** without re-granting (backlog “idempotent replay via store”). - **Empty bundle** — null or both-empty grant lists → success with empty snapshots and **`TryRecord`** (NEO-126 allows empty arrays; CI guarantees all four prototype quests have bundles). -- **Store race** — if grants succeed but **`TryRecord`** returns `false`, retain grants and treat as idempotent success (mirror [NEO-107 encounter event note](../../../server/README.md#encounter-complete-event-record-neo-107)). +- **Store race** — if grants succeed but **`TryRecord`** returns `false`, **rollback this call's item + skill XP grants** and return the winner's store event (mirror **`EncounterCompletionOperations`** when **`TryMarkCompleted`** loses); do not retain duplicate grants on the loser. ## Goal, scope, and out-of-scope @@ -52,9 +52,9 @@ ## Implementation reconciliation (shipped) -- **Operations:** `RewardRouterOperations.TryDeliverQuestCompletion` — item pre-flight + apply, skill XP via `TryApplyGrant` (`mission_reward`), compensating item rollback, `IRewardDeliveryStore.TryRecord`. +- **Operations:** `RewardRouterOperations.TryDeliverQuestCompletion` — item pre-flight + apply, skill XP via `TryApplyGrant` (`mission_reward`), compensating item + skill XP rollback on apply/skill deny and on **`TryRecord`** race loss, `IRewardDeliveryStore.TryRecord`. - **Types:** `RewardDeliveryResult`, `RewardDeliveryReasonCodes`. -- **Tests:** 10 AAA tests in `RewardRouterOperationsTests` (skill-only + item+skill happy paths, inventory deny, skill deny rollback, store replay, mission_reward apply, unknown skill deny, null bundle empty record, invalid id deny). +- **Tests:** 12 AAA tests in `RewardRouterOperationsTests` (skill-only + item+skill happy paths, inventory deny, skill deny rollback, store replay, mission_reward apply, unknown skill deny, null bundle empty record, invalid id deny, TryRecord race rollback, concurrent race no double-grant). - **Docs:** `server/README.md` reward router section; E7.M2 module anchor + alignment register updated. ## Technical approach @@ -113,7 +113,7 @@ public static RewardDeliveryResult TryDeliverQuestCompletion( - On **`ProgressionStoreMissing`**, compensating item rollback + deny. - On **`Granted`**, append **`RewardSkillXpGrantApplied`**. 7. **Record** — build **`RewardDeliveryEvent`** with **`DeliveredAt = timeProvider.GetUtcNow()`**, **`IdempotencyKey = RewardDeliveryIds.MakeIdempotencyKey(...)`**, grant snapshots. -8. **`deliveryStore.TryRecord`** — if `true`, return fresh success; if `false` (race), return success with **`TryGet`** event (grants already applied — idempotent success). +8. **`deliveryStore.TryRecord`** — if `true`, return fresh success; if `false` (race), **compensating rollback** of this call's item + skill XP grants, then return **`TryGet`** winner event with **`already_delivered`** (or deny if store miss). **Compensating item rollback** — private helper mirroring **`EncounterCompletionOperations.CompensatingRemoveGrants`**: reverse-order **`TryRemoveStack`** for applied item rows only (no skill XP undo). diff --git a/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs index 9315eae..a605a9b 100644 --- a/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs @@ -284,6 +284,88 @@ public sealed class RewardRouterOperationsTests Assert.False(deps.DeliveryStore.TryGet(PlayerId, " ", out _)); } + [Fact] + public async Task TryDeliverQuestCompletion_ShouldRollbackGrants_WhenTryRecordLosesRace() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + var winnerEvent = new RewardDeliveryEvent( + PlayerId, + GatherQuestId, + DeliveredAt, + [], + [new RewardSkillXpGrantApplied("salvage", 25)], + RewardDeliveryIds.MakeIdempotencyKey(PlayerId, GatherQuestId)); + var store = new DeliveryStoreSimulatingRecordRaceLoss(winnerEvent); + var bundle = new QuestRewardBundleRow([], [new QuestSkillXpGrantRow("salvage", 25)]); + var xpBefore = deps.SkillProgressionStore.GetXpTotals(PlayerId); + var salvageBefore = xpBefore.TryGetValue("salvage", out var beforeSalvage) ? beforeSalvage : 0; + + // Act + var result = RewardRouterOperations.TryDeliverQuestCompletion( + PlayerId, + GatherQuestId, + bundle, + deps.ItemRegistry, + deps.InventoryStore, + deps.SkillRegistry, + deps.SkillProgressionStore, + deps.LevelCurve, + deps.PerkUnlockEngine, + store, + TimeProvider.System); + + // Assert + Assert.True(result.Success); + Assert.Equal(RewardDeliveryReasonCodes.AlreadyDelivered, result.ReasonCode); + Assert.Equal(winnerEvent.IdempotencyKey, result.DeliveryEvent!.Value.IdempotencyKey); + var xpAfter = deps.SkillProgressionStore.GetXpTotals(PlayerId); + var salvageAfter = xpAfter.TryGetValue("salvage", out var afterSalvage) ? afterSalvage : 0; + Assert.Equal(salvageBefore, salvageAfter); + } + + [Fact] + public async Task TryDeliverQuestCompletion_ShouldNotDoubleGrant_WhenConcurrentRecordRaces() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + var bundle = new QuestRewardBundleRow( + [new RewardGrantRow("survey_drone_kit", 1)], + [new QuestSkillXpGrantRow("salvage", 25)]); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventoryBefore); + var xpBefore = deps.SkillProgressionStore.GetXpTotals(PlayerId); + var salvageBefore = xpBefore.TryGetValue("salvage", out var beforeSalvage) ? beforeSalvage : 0; + var kitBefore = CountBagItem(inventoryBefore!, "survey_drone_kit"); + var barrier = new Barrier(2); + RewardDeliveryResult result1 = default; + RewardDeliveryResult result2 = default; + + // Act + await Task.WhenAll( + Task.Run(() => + { + barrier.SignalAndWait(); + result1 = Deliver(deps, OperatorChainQuestId, bundle, TimeProvider.System); + }), + Task.Run(() => + { + barrier.SignalAndWait(); + result2 = Deliver(deps, OperatorChainQuestId, bundle, TimeProvider.System); + })); + + // Assert + Assert.True(result1.Success); + Assert.True(result2.Success); + deps.InventoryStore.TryGetSnapshot(PlayerId, out var inventoryAfter); + var xpAfter = deps.SkillProgressionStore.GetXpTotals(PlayerId); + Assert.True(xpAfter.TryGetValue("salvage", out var salvageAfter)); + Assert.Equal(salvageBefore + 25, salvageAfter); + Assert.Equal(kitBefore + 1, CountBagItem(inventoryAfter!, "survey_drone_kit")); + Assert.True(deps.DeliveryStore.TryGet(PlayerId, OperatorChainQuestId, out _)); + } + private static RewardDeliveryResult Deliver( RewardRouterTestDependencies deps, string questId, @@ -365,4 +447,28 @@ public sealed class RewardRouterOperationsTests ISkillLevelCurve LevelCurve, PerkUnlockEngine PerkUnlockEngine, IRewardDeliveryStore DeliveryStore); + + /// TryGet misses until after a failed — simulates concurrent race loser. + private sealed class DeliveryStoreSimulatingRecordRaceLoss(RewardDeliveryEvent winnerEvent) : IRewardDeliveryStore + { + private bool recordAttempted; + + public bool TryRecord(RewardDeliveryEvent deliveryEvent) + { + recordAttempted = true; + return false; + } + + public bool TryGet(string playerId, string questId, out RewardDeliveryEvent deliveryEvent) + { + if (!recordAttempted) + { + deliveryEvent = default; + return false; + } + + deliveryEvent = winnerEvent; + return true; + } + } } diff --git a/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs b/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs index 9826a23..b908cdb 100644 --- a/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs +++ b/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs @@ -141,12 +141,17 @@ public static class RewardRouterOperations return SuccessFromEvent(deliveryEvent, reasonCode: null); } + // Lost TryRecord race after apply — rollback this call's grants (mirror + // EncounterCompletionOperations when TryMarkCompleted returns false). + CompensatingRemoveItems(playerId, appliedItems, itemRegistry, inventoryStore); + CompensatingRemoveSkillXp(playerId, appliedSkillXp, skillProgressionStore); + if (deliveryStore.TryGet(normalizedPlayerId, normalizedQuestId, out var racedEvent)) { return SuccessFromEvent(racedEvent, RewardDeliveryReasonCodes.AlreadyDelivered); } - return SuccessFromEvent(deliveryEvent, RewardDeliveryReasonCodes.AlreadyDelivered); + return Deny(RewardDeliveryReasonCodes.AlreadyDelivered); } private static string MapInventoryReason(string? reasonCode) => @@ -177,6 +182,23 @@ public static class RewardRouterOperations } } + private static void CompensatingRemoveSkillXp( + string playerId, + IReadOnlyList grants, + IPlayerSkillProgressionStore skillProgressionStore) + { + for (var i = grants.Count - 1; i >= 0; i--) + { + var grant = grants[i]; + _ = skillProgressionStore.TryApplyXpDelta( + playerId, + grant.SkillId, + -grant.Amount, + out _, + out _); + } + } + private static RewardDeliveryResult Deny(string reasonCode) => new( Success: false, diff --git a/server/README.md b/server/README.md index efa5c87..9bf0864 100644 --- a/server/README.md +++ b/server/README.md @@ -297,7 +297,7 @@ Encounter loot remains on **`IEncounterCompleteEventStore`** (E5.M3); quest comp | **`unknown_skill`**, **`invalid_amount`**, **`source_kind_not_allowed`** | Skill XP grant denied. | | **`progression_store_missing`** | Skill progression store could not apply XP delta. | -Apply order: **items first**, then **skill XP**. Inventory deny or skill deny fails closed without store record (compensating item rollback on partial apply or skill failure after items). Idempotent replay checks **`TryGet`** before apply. If grants succeed but **`TryRecord`** returns `false` (race), grants are retained and the existing store event is returned. Quest-state wiring lands in NEO-128. Plan: [NEO-127 implementation plan](../../docs/plans/NEO-127-implementation-plan.md). +Apply order: **items first**, then **skill XP**. Inventory deny or skill deny fails closed without store record (compensating item rollback on partial apply or skill failure after items). Idempotent replay checks **`TryGet`** before apply. If grants succeed but **`TryRecord`** returns `false` (concurrent race), this call **rolls back** its item + skill XP grants and returns the existing store event (mirror **`EncounterCompletionOperations`** compensating rollback when **`TryMarkCompleted`** loses). Quest-state wiring lands in NEO-128. Plan: [NEO-127 implementation plan](../../docs/plans/NEO-127-implementation-plan.md). ## Encounter definitions (NEO-103)