From be9bb8f9616ee4ba15a572de8664732cc652f91b Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 7 Jun 2026 18:04:06 -0400 Subject: [PATCH] NEO-127: restore perk state on TryRecord race rollback Capture pre-skill-apply XP and perk snapshots; on TryRecord loss reverse each applied skill row via negative delta and restore the perk snapshot so level-up unlocks from the loser call are reverted without clobbering a concurrent winner's XP. --- docs/plans/NEO-127-implementation-plan.md | 7 +- docs/reviews/2026-06-07-NEO-127.md | 2 +- .../Rewards/RewardRouterOperationsTests.cs | 54 ++++++++++++++ .../Game/Mastery/IPlayerPerkStateStore.cs | 3 + .../Mastery/InMemoryPlayerPerkStateStore.cs | 29 ++++++++ .../Mastery/PostgresPlayerPerkStateStore.cs | 73 +++++++++++++++++++ .../Game/Rewards/RewardRouterOperations.cs | 29 +++----- .../Skills/SkillProgressionGrantOperations.cs | 30 ++++++++ .../SkillProgressionGrantRollbackPoint.cs | 8 ++ server/README.md | 2 +- 10 files changed, 215 insertions(+), 22 deletions(-) create mode 100644 server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantRollbackPoint.cs diff --git a/docs/plans/NEO-127-implementation-plan.md b/docs/plans/NEO-127-implementation-plan.md index 1d30f34..f9acf25 100644 --- a/docs/plans/NEO-127-implementation-plan.md +++ b/docs/plans/NEO-127-implementation-plan.md @@ -52,9 +52,9 @@ ## Implementation reconciliation (shipped) -- **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`. +- **Operations:** `RewardRouterOperations.TryDeliverQuestCompletion` — item pre-flight + apply, skill XP via `TryApplyGrant` (`mission_reward`), compensating item rollback + **`SkillProgressionGrantRollbackPoint`** restore (XP + perks) on **`TryRecord`** race loss, `IRewardDeliveryStore.TryRecord`. - **Types:** `RewardDeliveryResult`, `RewardDeliveryReasonCodes`. -- **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). +- **Tests:** 13 AAA tests in `RewardRouterOperationsTests` (…, TryRecord race rollback, concurrent race no double-grant, perk state restore on race loss after level-up). - **Docs:** `server/README.md` reward router section; E7.M2 module anchor + alignment register updated. ## Technical approach @@ -97,6 +97,7 @@ public static RewardDeliveryResult TryDeliverQuestCompletion( IPlayerSkillProgressionStore skillProgressionStore, ISkillLevelCurve levelCurve, PerkUnlockEngine perkUnlockEngine, + IPlayerPerkStateStore perkStore, IRewardDeliveryStore deliveryStore, TimeProvider timeProvider) ``` @@ -117,6 +118,8 @@ public static RewardDeliveryResult TryDeliverQuestCompletion( **Compensating item rollback** — private helper mirroring **`EncounterCompletionOperations.CompensatingRemoveGrants`**: reverse-order **`TryRemoveStack`** for applied item rows only (no skill XP undo). +**Compensating skill progression rollback** — **`SkillProgressionGrantOperations.CaptureRollbackPoint`** before skill apply; on **`TryRecord`** race loss, **`TryRestoreRollbackPoint`** reverses each applied skill XP row via negative delta and restores the captured perk snapshot (undoes level-up perk unlocks from the loser call without clobbering a concurrent winner's XP). + ### 3. Skill XP source kind enforcement Always pass **`MissionRewardSkillXpConstants.MissionRewardSourceKind`** at apply time — content rows do not carry **`sourceKind`** (NEO-124/NEO-43). Startup catalog gates already require skills to allow **`mission_reward`**; router still maps **`source_kind_not_allowed`** if a deny occurs. diff --git a/docs/reviews/2026-06-07-NEO-127.md b/docs/reviews/2026-06-07-NEO-127.md index 1370d0e..c16be64 100644 --- a/docs/reviews/2026-06-07-NEO-127.md +++ b/docs/reviews/2026-06-07-NEO-127.md @@ -54,7 +54,7 @@ None. dotnet test server/NeonSprawl.Server.Tests --filter RewardRouterOperationsTests ``` -All 10 tests pass (verified 2026-06-07; was 7 before review follow-up). +All 13 tests pass (verified 2026-06-07; includes race rollback + perk snapshot restore on **`TryRecord`** loss). Optional before merge: diff --git a/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs b/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs index a605a9b..350f7ab 100644 --- a/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs @@ -15,6 +15,7 @@ public sealed class RewardRouterOperationsTests private const string PlayerId = "dev-local-1"; private const string GatherQuestId = "prototype_quest_gather_intro"; private const string OperatorChainQuestId = "prototype_quest_operator_chain"; + private const string SalvageScrapEfficiencyPerk = "salvage_scrap_efficiency_1"; private static readonly DateTimeOffset DeliveredAt = new(2026, 6, 7, 14, 0, 0, TimeSpan.Zero); [Fact] @@ -247,6 +248,7 @@ public sealed class RewardRouterOperationsTests deps.SkillProgressionStore, deps.LevelCurve, deps.PerkUnlockEngine, + deps.PerkStore, deps.DeliveryStore, TimeProvider.System); @@ -275,6 +277,7 @@ public sealed class RewardRouterOperationsTests deps.SkillProgressionStore, deps.LevelCurve, deps.PerkUnlockEngine, + deps.PerkStore, deps.DeliveryStore, TimeProvider.System); @@ -313,6 +316,7 @@ public sealed class RewardRouterOperationsTests deps.SkillProgressionStore, deps.LevelCurve, deps.PerkUnlockEngine, + deps.PerkStore, store, TimeProvider.System); @@ -325,6 +329,53 @@ public sealed class RewardRouterOperationsTests Assert.Equal(salvageBefore, salvageAfter); } + [Fact] + public async Task TryDeliverQuestCompletion_ShouldRestorePerkState_WhenTryRecordLosesRaceAfterLevelUp() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + Assert.True(deps.SkillProgressionStore.TrySetSkillXpTotal(PlayerId, "salvage", 425)); + var pick = deps.PerkUnlockEngine.TrySelectBranch(PlayerId, "salvage", 1, "scrap_efficiency"); + Assert.True(pick.Success); + var perkBefore = deps.PerkStore.GetSnapshot(PlayerId); + Assert.DoesNotContain(SalvageScrapEfficiencyPerk, perkBefore.UnlockedPerkIds); + 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)]); + + // Act + var result = RewardRouterOperations.TryDeliverQuestCompletion( + PlayerId, + GatherQuestId, + bundle, + deps.ItemRegistry, + deps.InventoryStore, + deps.SkillRegistry, + deps.SkillProgressionStore, + deps.LevelCurve, + deps.PerkUnlockEngine, + deps.PerkStore, + store, + TimeProvider.System); + + // Assert + Assert.True(result.Success); + Assert.Equal(RewardDeliveryReasonCodes.AlreadyDelivered, result.ReasonCode); + var xpAfter = deps.SkillProgressionStore.GetXpTotals(PlayerId); + Assert.True(xpAfter.TryGetValue("salvage", out var salvageAfter)); + Assert.Equal(425, salvageAfter); + var perkAfter = deps.PerkStore.GetSnapshot(PlayerId); + Assert.DoesNotContain(SalvageScrapEfficiencyPerk, perkAfter.UnlockedPerkIds); + Assert.Equal(perkBefore.UnlockedPerkIds, perkAfter.UnlockedPerkIds); + } + [Fact] public async Task TryDeliverQuestCompletion_ShouldNotDoubleGrant_WhenConcurrentRecordRaces() { @@ -381,6 +432,7 @@ public sealed class RewardRouterOperationsTests deps.SkillProgressionStore, deps.LevelCurve, deps.PerkUnlockEngine, + deps.PerkStore, deps.DeliveryStore, timeProvider); @@ -408,6 +460,7 @@ public sealed class RewardRouterOperationsTests services.GetRequiredService(), services.GetRequiredService(), services.GetRequiredService(), + services.GetRequiredService(), services.GetRequiredService()); } @@ -446,6 +499,7 @@ public sealed class RewardRouterOperationsTests IPlayerSkillProgressionStore SkillProgressionStore, ISkillLevelCurve LevelCurve, PerkUnlockEngine PerkUnlockEngine, + IPlayerPerkStateStore PerkStore, IRewardDeliveryStore DeliveryStore); /// TryGet misses until after a failed — simulates concurrent race loser. diff --git a/server/NeonSprawl.Server/Game/Mastery/IPlayerPerkStateStore.cs b/server/NeonSprawl.Server/Game/Mastery/IPlayerPerkStateStore.cs index 5ddc70e..29b5051 100644 --- a/server/NeonSprawl.Server/Game/Mastery/IPlayerPerkStateStore.cs +++ b/server/NeonSprawl.Server/Game/Mastery/IPlayerPerkStateStore.cs @@ -19,4 +19,7 @@ public interface IPlayerPerkStateStore /// Clears all branch picks and unlocked perks for the player (NEO-48 dev fixture API). bool TryResetPerkState(string playerId); + + /// Replaces branch picks and unlocked perks with (grant rollback). + bool TryRestoreSnapshot(string playerId, PerkStateSnapshot snapshot); } diff --git a/server/NeonSprawl.Server/Game/Mastery/InMemoryPlayerPerkStateStore.cs b/server/NeonSprawl.Server/Game/Mastery/InMemoryPlayerPerkStateStore.cs index 8e904fa..2736bfa 100644 --- a/server/NeonSprawl.Server/Game/Mastery/InMemoryPlayerPerkStateStore.cs +++ b/server/NeonSprawl.Server/Game/Mastery/InMemoryPlayerPerkStateStore.cs @@ -123,6 +123,35 @@ public sealed class InMemoryPlayerPerkStateStore(IOptions o } } + /// + public bool TryRestoreSnapshot(string playerId, PerkStateSnapshot snapshot) + { + var key = NormalizePlayerId(playerId); + if (key.Length == 0 || !byPlayer.TryGetValue(key, out var inner)) + { + return false; + } + + lock (playerLocks.GetOrAdd(key, _ => new object())) + { + inner.Reset(); + foreach (var (skillId, tiers) in snapshot.BranchPicksBySkillId) + { + foreach (var (tierIndex, branchId) in tiers) + { + inner.SetBranchPick(skillId, tierIndex, branchId); + } + } + + foreach (var perkId in snapshot.UnlockedPerkIds) + { + inner.AddUnlockedPerk(perkId); + } + + return true; + } + } + private static string NormalizePlayerId(string? playerId) { var t = playerId?.Trim(); diff --git a/server/NeonSprawl.Server/Game/Mastery/PostgresPlayerPerkStateStore.cs b/server/NeonSprawl.Server/Game/Mastery/PostgresPlayerPerkStateStore.cs index d455cf8..8617fed 100644 --- a/server/NeonSprawl.Server/Game/Mastery/PostgresPlayerPerkStateStore.cs +++ b/server/NeonSprawl.Server/Game/Mastery/PostgresPlayerPerkStateStore.cs @@ -257,6 +257,79 @@ public sealed class PostgresPlayerPerkStateStore(Npgsql.NpgsqlDataSource dataSou return true; } + /// + public bool TryRestoreSnapshot(string playerId, PerkStateSnapshot snapshot) + { + var norm = NormalizePlayerId(playerId); + if (norm.Length == 0) + { + return false; + } + + PostgresPerkStateBootstrap.EnsureSchema(dataSource); + using var conn = dataSource.OpenConnection(); + using var tx = conn.BeginTransaction(); + if (!PlayerExists(conn, norm, tx)) + { + tx.Rollback(); + return false; + } + + using (var picks = new Npgsql.NpgsqlCommand( + "DELETE FROM player_mastery_branch_pick WHERE player_id = @pid;", + conn, + tx)) + { + picks.Parameters.AddWithValue("pid", norm); + picks.ExecuteNonQuery(); + } + + using (var perks = new Npgsql.NpgsqlCommand( + "DELETE FROM player_unlocked_perk WHERE player_id = @pid;", + conn, + tx)) + { + perks.Parameters.AddWithValue("pid", norm); + perks.ExecuteNonQuery(); + } + + foreach (var (skillId, tiers) in snapshot.BranchPicksBySkillId) + { + foreach (var (tierIndex, branchId) in tiers) + { + using var insert = new Npgsql.NpgsqlCommand( + """ + INSERT INTO player_mastery_branch_pick (player_id, skill_id, tier_index, branch_id, updated_at) + VALUES (@pid, @sid, @tier, @bid, now()); + """, + conn, + tx); + insert.Parameters.AddWithValue("pid", norm); + insert.Parameters.AddWithValue("sid", skillId); + insert.Parameters.AddWithValue("tier", tierIndex); + insert.Parameters.AddWithValue("bid", branchId); + insert.ExecuteNonQuery(); + } + } + + foreach (var perkId in snapshot.UnlockedPerkIds) + { + using var upsert = new Npgsql.NpgsqlCommand( + """ + INSERT INTO player_unlocked_perk (player_id, perk_id, unlocked_at) + VALUES (@pid, @perk, now()); + """, + conn, + tx); + upsert.Parameters.AddWithValue("pid", norm); + upsert.Parameters.AddWithValue("perk", perkId); + upsert.ExecuteNonQuery(); + } + + tx.Commit(); + return true; + } + private static bool PlayerExists(Npgsql.NpgsqlConnection conn, string playerIdNormalized, Npgsql.NpgsqlTransaction tx) { using var cmd = new Npgsql.NpgsqlCommand( diff --git a/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs b/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs index b908cdb..cf4b53c 100644 --- a/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs +++ b/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs @@ -29,6 +29,7 @@ public static class RewardRouterOperations IPlayerSkillProgressionStore skillProgressionStore, ISkillLevelCurve levelCurve, PerkUnlockEngine perkUnlockEngine, + IPlayerPerkStateStore perkStore, IRewardDeliveryStore deliveryStore, TimeProvider timeProvider) { @@ -98,6 +99,10 @@ public static class RewardRouterOperations appliedItems.Add(new RewardItemGrantApplied(grant.ItemId, grant.Quantity)); } + var progressionRollbackPoint = SkillProgressionGrantOperations.CaptureRollbackPoint( + playerId, + skillProgressionStore, + perkStore); var appliedSkillXp = new List(skillGrants.Count); foreach (var grant in skillGrants) { @@ -144,7 +149,12 @@ public static class RewardRouterOperations // 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); + _ = SkillProgressionGrantOperations.TryRestoreRollbackPoint( + playerId, + progressionRollbackPoint, + appliedSkillXp, + skillProgressionStore, + perkStore); if (deliveryStore.TryGet(normalizedPlayerId, normalizedQuestId, out var racedEvent)) { @@ -182,23 +192,6 @@ 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/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs b/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs index 2f7651c..401167a 100644 --- a/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs +++ b/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs @@ -1,4 +1,5 @@ using NeonSprawl.Server.Game.Mastery; +using NeonSprawl.Server.Game.Rewards; namespace NeonSprawl.Server.Game.Skills; @@ -101,4 +102,33 @@ public static class SkillProgressionGrantOperations LevelUps = levelUps, }); } + + /// Captures skill XP totals and perk snapshot before a grant batch for compensating rollback. + public static SkillProgressionGrantRollbackPoint CaptureRollbackPoint( + string playerId, + IPlayerSkillProgressionStore xpStore, + IPlayerPerkStateStore perkStore) => + new( + new Dictionary(xpStore.GetXpTotals(playerId), StringComparer.Ordinal), + perkStore.GetSnapshot(playerId)); + + /// Restores skill XP and perk state captured before a failed grant batch commit. + public static bool TryRestoreRollbackPoint( + string playerId, + SkillProgressionGrantRollbackPoint rollbackPoint, + IReadOnlyList appliedGrants, + IPlayerSkillProgressionStore xpStore, + IPlayerPerkStateStore perkStore) + { + for (var i = appliedGrants.Count - 1; i >= 0; i--) + { + var grant = appliedGrants[i]; + if (!xpStore.TryApplyXpDelta(playerId, grant.SkillId, -grant.Amount, out _, out _)) + { + return false; + } + } + + return perkStore.TryRestoreSnapshot(playerId, rollbackPoint.PerkState); + } } diff --git a/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantRollbackPoint.cs b/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantRollbackPoint.cs new file mode 100644 index 0000000..4978e27 --- /dev/null +++ b/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantRollbackPoint.cs @@ -0,0 +1,8 @@ +using NeonSprawl.Server.Game.Mastery; + +namespace NeonSprawl.Server.Game.Skills; + +/// Captured skill XP + perk state before a grant apply batch (NEO-127 compensating rollback). +public readonly record struct SkillProgressionGrantRollbackPoint( + IReadOnlyDictionary SkillXpTotals, + PerkStateSnapshot PerkState); diff --git a/server/README.md b/server/README.md index 9bf0864..3f4e2ae 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` (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). +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 grants and calls **`SkillProgressionGrantOperations.TryRestoreRollbackPoint`** — negative XP delta per applied skill row plus restore of the captured pre-apply perk snapshot (reverts level-up perk unlocks from the loser without clobbering a concurrent winner's XP) — then 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)