diff --git a/docs/plans/NEO-127-implementation-plan.md b/docs/plans/NEO-127-implementation-plan.md index f9acf25..cb84565 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 rollback + **`SkillProgressionGrantRollbackPoint`** restore (XP + perks) on **`TryRecord`** race loss, `IRewardDeliveryStore.TryRecord`. +- **Operations:** `RewardRouterOperations.TryDeliverQuestCompletion` — item pre-flight + apply, skill XP via `TryApplyGrant` (`mission_reward`), compensating item rollback + per-grant perk tracking + **`TryRevertAppliedSkillGrants`** on **`TryRecord`** race loss, `IRewardDeliveryStore.TryRecord`. - **Types:** `RewardDeliveryResult`, `RewardDeliveryReasonCodes`. -- **Tests:** 13 AAA tests in `RewardRouterOperationsTests` (…, TryRecord race rollback, concurrent race no double-grant, perk state restore on race loss after level-up). +- **Tests:** 14 AAA tests in `RewardRouterOperationsTests` (…, TryRecord race rollback, concurrent race no double-grant, perk revert on simulated race loss, concurrent level-up winner perk preserved). - **Docs:** `server/README.md` reward router section; E7.M2 module anchor + alignment register updated. ## Technical approach @@ -118,7 +118,7 @@ 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). +**Compensating skill progression rollback** — track perks unlocked per skill grant (`CollectPerksUnlockedSince`); on **`TryRecord`** race loss, **`TryRevertAppliedSkillGrants`** reverses each applied skill XP row via negative delta and removes only perks this call unlocked, then **`ReevaluateAfterLevelUp`** at post-rollback levels re-applies path-auto perks the winner's XP still qualifies for (avoids full snapshot restore clobbering a concurrent winner). ### 3. Skill XP source kind enforcement diff --git a/docs/reviews/2026-06-07-NEO-127.md b/docs/reviews/2026-06-07-NEO-127.md index c16be64..56f2460 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 13 tests pass (verified 2026-06-07; includes race rollback + perk snapshot restore on **`TryRecord`** loss). +All 14 tests pass (verified 2026-06-07; includes concurrent level-up winner perk preservation on **`TryRecord`** race 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 350f7ab..3c45a7b 100644 --- a/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Rewards/RewardRouterOperationsTests.cs @@ -417,6 +417,43 @@ public sealed class RewardRouterOperationsTests Assert.True(deps.DeliveryStore.TryGet(PlayerId, OperatorChainQuestId, out _)); } + [Fact] + public async Task TryDeliverQuestCompletion_ShouldPreserveWinnerPerks_WhenConcurrentRecordRacesAfterLevelUp() + { + // Arrange + await using var factory = new InMemoryWebApplicationFactory(); + var deps = ResolveDependencies(factory); + Assert.True(deps.SkillProgressionStore.TrySetSkillXpTotal(PlayerId, "salvage", 425)); + Assert.True(deps.PerkUnlockEngine.TrySelectBranch(PlayerId, "salvage", 1, "scrap_efficiency").Success); + var bundle = new QuestRewardBundleRow([], [new QuestSkillXpGrantRow("salvage", 25)]); + var barrier = new Barrier(2); + RewardDeliveryResult result1 = default; + RewardDeliveryResult result2 = default; + + // Act + await Task.WhenAll( + Task.Run(() => + { + barrier.SignalAndWait(); + result1 = Deliver(deps, GatherQuestId, bundle, TimeProvider.System); + }), + Task.Run(() => + { + barrier.SignalAndWait(); + result2 = Deliver(deps, GatherQuestId, bundle, TimeProvider.System); + })); + + // Assert + Assert.True(result1.Success); + Assert.True(result2.Success); + var xpAfter = deps.SkillProgressionStore.GetXpTotals(PlayerId); + Assert.True(xpAfter.TryGetValue("salvage", out var salvageAfter)); + Assert.Equal(450, salvageAfter); + var perkAfter = deps.PerkStore.GetSnapshot(PlayerId); + Assert.Contains(SalvageScrapEfficiencyPerk, perkAfter.UnlockedPerkIds); + Assert.True(deps.DeliveryStore.TryGet(PlayerId, GatherQuestId, out _)); + } + private static RewardDeliveryResult Deliver( RewardRouterTestDependencies deps, string questId, diff --git a/server/NeonSprawl.Server/Game/Mastery/IPlayerPerkStateStore.cs b/server/NeonSprawl.Server/Game/Mastery/IPlayerPerkStateStore.cs index 29b5051..131157a 100644 --- a/server/NeonSprawl.Server/Game/Mastery/IPlayerPerkStateStore.cs +++ b/server/NeonSprawl.Server/Game/Mastery/IPlayerPerkStateStore.cs @@ -14,6 +14,9 @@ public interface IPlayerPerkStateStore /// Adds perks not already unlocked; fails when player cannot be written. bool TryAddUnlockedPerks(string playerId, IEnumerable perkIds); + /// Removes unlocked perks present in the store; fails when player cannot be written. + bool TryRemoveUnlockedPerks(string playerId, IEnumerable perkIds); + /// Removes a branch pick (engine rollback when perk unlock fails after pick). bool TryRemoveBranchPick(string playerId, string skillId, int tierIndex); diff --git a/server/NeonSprawl.Server/Game/Mastery/InMemoryPlayerPerkStateStore.cs b/server/NeonSprawl.Server/Game/Mastery/InMemoryPlayerPerkStateStore.cs index 2736bfa..5883dea 100644 --- a/server/NeonSprawl.Server/Game/Mastery/InMemoryPlayerPerkStateStore.cs +++ b/server/NeonSprawl.Server/Game/Mastery/InMemoryPlayerPerkStateStore.cs @@ -91,6 +91,32 @@ public sealed class InMemoryPlayerPerkStateStore(IOptions o } } + /// + public bool TryRemoveUnlockedPerks(string playerId, IEnumerable perkIds) + { + var key = NormalizePlayerId(playerId); + if (key.Length == 0 || !byPlayer.TryGetValue(key, out var inner)) + { + return false; + } + + lock (playerLocks.GetOrAdd(key, _ => new object())) + { + foreach (var raw in perkIds) + { + var pid = raw.Trim(); + if (pid.Length == 0) + { + continue; + } + + inner.RemoveUnlockedPerk(pid); + } + + return true; + } + } + /// public bool TryRemoveBranchPick(string playerId, string skillId, int tierIndex) { @@ -185,6 +211,8 @@ public sealed class InMemoryPlayerPerkStateStore(IOptions o public void AddUnlockedPerk(string perkId) => unlockedPerks.Add(perkId); + public void RemoveUnlockedPerk(string perkId) => unlockedPerks.Remove(perkId); + public bool RemoveBranchPick(string skillId, int tierIndex) => branchPicks.TryGetValue(skillId, out var tiers) && tiers.Remove(tierIndex); diff --git a/server/NeonSprawl.Server/Game/Mastery/PostgresPlayerPerkStateStore.cs b/server/NeonSprawl.Server/Game/Mastery/PostgresPlayerPerkStateStore.cs index 8617fed..d108260 100644 --- a/server/NeonSprawl.Server/Game/Mastery/PostgresPlayerPerkStateStore.cs +++ b/server/NeonSprawl.Server/Game/Mastery/PostgresPlayerPerkStateStore.cs @@ -193,6 +193,45 @@ public sealed class PostgresPlayerPerkStateStore(Npgsql.NpgsqlDataSource dataSou return true; } + /// + public bool TryRemoveUnlockedPerks(string playerId, IEnumerable perkIds) + { + 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; + } + + foreach (var raw in perkIds) + { + var perkId = raw.Trim(); + if (perkId.Length == 0) + { + continue; + } + + using var delete = new Npgsql.NpgsqlCommand( + "DELETE FROM player_unlocked_perk WHERE player_id = @pid AND perk_id = @perk;", + conn, + tx); + delete.Parameters.AddWithValue("pid", norm); + delete.Parameters.AddWithValue("perk", perkId); + delete.ExecuteNonQuery(); + } + + tx.Commit(); + return true; + } + /// public bool TryRemoveBranchPick(string playerId, string skillId, int tierIndex) { diff --git a/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs b/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs index cf4b53c..32cb573 100644 --- a/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs +++ b/server/NeonSprawl.Server/Game/Rewards/RewardRouterOperations.cs @@ -99,13 +99,11 @@ public static class RewardRouterOperations appliedItems.Add(new RewardItemGrantApplied(grant.ItemId, grant.Quantity)); } - var progressionRollbackPoint = SkillProgressionGrantOperations.CaptureRollbackPoint( - playerId, - skillProgressionStore, - perkStore); + var perksUnlockedByThisCall = new HashSet(StringComparer.Ordinal); var appliedSkillXp = new List(skillGrants.Count); foreach (var grant in skillGrants) { + var perkBeforeGrant = perkStore.GetSnapshot(playerId); var skillOutcome = SkillProgressionGrantOperations.TryApplyGrant( playerId, grant.SkillId, @@ -130,6 +128,10 @@ public static class RewardRouterOperations } appliedSkillXp.Add(new RewardSkillXpGrantApplied(grant.SkillId, grant.Amount)); + SkillProgressionGrantOperations.CollectPerksUnlockedSince( + perkBeforeGrant, + perkStore.GetSnapshot(playerId), + perksUnlockedByThisCall); } var deliveredAt = timeProvider.GetUtcNow(); @@ -149,12 +151,18 @@ 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); - _ = SkillProgressionGrantOperations.TryRestoreRollbackPoint( + _ = SkillProgressionGrantOperations.TryRevertAppliedSkillGrants( playerId, - progressionRollbackPoint, appliedSkillXp, + perksUnlockedByThisCall, skillProgressionStore, perkStore); + ResyncPathAutoPerksAfterRaceRollback( + playerId, + appliedSkillXp, + skillProgressionStore, + levelCurve, + perkUnlockEngine); if (deliveryStore.TryGet(normalizedPlayerId, normalizedQuestId, out var racedEvent)) { @@ -164,6 +172,27 @@ public static class RewardRouterOperations return Deny(RewardDeliveryReasonCodes.AlreadyDelivered); } + private static void ResyncPathAutoPerksAfterRaceRollback( + string playerId, + IReadOnlyList appliedSkillXp, + IPlayerSkillProgressionStore skillProgressionStore, + ISkillLevelCurve levelCurve, + PerkUnlockEngine perkUnlockEngine) + { + if (appliedSkillXp.Count == 0) + { + return; + } + + var xpBySkill = skillProgressionStore.GetXpTotals(playerId); + foreach (var grant in appliedSkillXp) + { + xpBySkill.TryGetValue(grant.SkillId, out var totalXp); + var level = levelCurve.LevelFromTotalXp(totalXp); + _ = perkUnlockEngine.ReevaluateAfterLevelUp(playerId, grant.SkillId, level); + } + } + private static string MapInventoryReason(string? reasonCode) => reasonCode switch { diff --git a/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs b/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs index 401167a..0d27599 100644 --- a/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs +++ b/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantOperations.cs @@ -103,20 +103,26 @@ public static class SkillProgressionGrantOperations }); } - /// 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)); + /// Collects perk ids unlocked between two snapshots (this batch only). + public static void CollectPerksUnlockedSince( + PerkStateSnapshot before, + PerkStateSnapshot after, + ISet sink) + { + foreach (var perkId in after.UnlockedPerkIds) + { + if (!before.UnlockedPerkIds.Contains(perkId)) + { + sink.Add(perkId); + } + } + } - /// Restores skill XP and perk state captured before a failed grant batch commit. - public static bool TryRestoreRollbackPoint( + /// Reverts skill XP grants and perks unlocked during this failed batch commit. + public static bool TryRevertAppliedSkillGrants( string playerId, - SkillProgressionGrantRollbackPoint rollbackPoint, IReadOnlyList appliedGrants, + IReadOnlyCollection perksUnlockedByBatch, IPlayerSkillProgressionStore xpStore, IPlayerPerkStateStore perkStore) { @@ -129,6 +135,7 @@ public static class SkillProgressionGrantOperations } } - return perkStore.TryRestoreSnapshot(playerId, rollbackPoint.PerkState); + return perksUnlockedByBatch.Count == 0 + || perkStore.TryRemoveUnlockedPerks(playerId, perksUnlockedByBatch); } } diff --git a/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantRollbackPoint.cs b/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantRollbackPoint.cs deleted file mode 100644 index 4978e27..0000000 --- a/server/NeonSprawl.Server/Game/Skills/SkillProgressionGrantRollbackPoint.cs +++ /dev/null @@ -1,8 +0,0 @@ -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 3f4e2ae..8b46293 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 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). +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.TryRevertAppliedSkillGrants`** — negative XP delta per applied skill row plus removal of perks unlocked during this call only — then **`ReevaluateAfterLevelUp`** at post-rollback levels so a concurrent winner's path-auto unlocks are preserved — 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)