NEO-127: avoid clobbering winner perks on TryRecord race rollback
Track perks unlocked per grant and remove only those on race loss instead of restoring a full pre-apply snapshot; re-run ReevaluateAfterLevelUp at post-rollback levels so a concurrent winner's path-auto unlocks survive.pull/166/head
parent
be9bb8f961
commit
039672cdac
|
|
@ -52,9 +52,9 @@
|
||||||
|
|
||||||
## Implementation reconciliation (shipped)
|
## 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`.
|
- **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.
|
- **Docs:** `server/README.md` reward router section; E7.M2 module anchor + alignment register updated.
|
||||||
|
|
||||||
## Technical approach
|
## 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 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
|
### 3. Skill XP source kind enforcement
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -54,7 +54,7 @@ None.
|
||||||
dotnet test server/NeonSprawl.Server.Tests --filter RewardRouterOperationsTests
|
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:
|
Optional before merge:
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -417,6 +417,43 @@ public sealed class RewardRouterOperationsTests
|
||||||
Assert.True(deps.DeliveryStore.TryGet(PlayerId, OperatorChainQuestId, out _));
|
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(
|
private static RewardDeliveryResult Deliver(
|
||||||
RewardRouterTestDependencies deps,
|
RewardRouterTestDependencies deps,
|
||||||
string questId,
|
string questId,
|
||||||
|
|
|
||||||
|
|
@ -14,6 +14,9 @@ public interface IPlayerPerkStateStore
|
||||||
/// <summary>Adds perks not already unlocked; fails when player cannot be written.</summary>
|
/// <summary>Adds perks not already unlocked; fails when player cannot be written.</summary>
|
||||||
bool TryAddUnlockedPerks(string playerId, IEnumerable<string> perkIds);
|
bool TryAddUnlockedPerks(string playerId, IEnumerable<string> perkIds);
|
||||||
|
|
||||||
|
/// <summary>Removes unlocked perks present in the store; fails when player cannot be written.</summary>
|
||||||
|
bool TryRemoveUnlockedPerks(string playerId, IEnumerable<string> perkIds);
|
||||||
|
|
||||||
/// <summary>Removes a branch pick (engine rollback when perk unlock fails after pick).</summary>
|
/// <summary>Removes a branch pick (engine rollback when perk unlock fails after pick).</summary>
|
||||||
bool TryRemoveBranchPick(string playerId, string skillId, int tierIndex);
|
bool TryRemoveBranchPick(string playerId, string skillId, int tierIndex);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -91,6 +91,32 @@ public sealed class InMemoryPlayerPerkStateStore(IOptions<GamePositionOptions> o
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <inheritdoc />
|
||||||
|
public bool TryRemoveUnlockedPerks(string playerId, IEnumerable<string> 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;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// <inheritdoc />
|
/// <inheritdoc />
|
||||||
public bool TryRemoveBranchPick(string playerId, string skillId, int tierIndex)
|
public bool TryRemoveBranchPick(string playerId, string skillId, int tierIndex)
|
||||||
{
|
{
|
||||||
|
|
@ -185,6 +211,8 @@ public sealed class InMemoryPlayerPerkStateStore(IOptions<GamePositionOptions> o
|
||||||
|
|
||||||
public void AddUnlockedPerk(string perkId) => unlockedPerks.Add(perkId);
|
public void AddUnlockedPerk(string perkId) => unlockedPerks.Add(perkId);
|
||||||
|
|
||||||
|
public void RemoveUnlockedPerk(string perkId) => unlockedPerks.Remove(perkId);
|
||||||
|
|
||||||
public bool RemoveBranchPick(string skillId, int tierIndex) =>
|
public bool RemoveBranchPick(string skillId, int tierIndex) =>
|
||||||
branchPicks.TryGetValue(skillId, out var tiers) && tiers.Remove(tierIndex);
|
branchPicks.TryGetValue(skillId, out var tiers) && tiers.Remove(tierIndex);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -193,6 +193,45 @@ public sealed class PostgresPlayerPerkStateStore(Npgsql.NpgsqlDataSource dataSou
|
||||||
return true;
|
return true;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// <inheritdoc />
|
||||||
|
public bool TryRemoveUnlockedPerks(string playerId, IEnumerable<string> 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;
|
||||||
|
}
|
||||||
|
|
||||||
/// <inheritdoc />
|
/// <inheritdoc />
|
||||||
public bool TryRemoveBranchPick(string playerId, string skillId, int tierIndex)
|
public bool TryRemoveBranchPick(string playerId, string skillId, int tierIndex)
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -99,13 +99,11 @@ public static class RewardRouterOperations
|
||||||
appliedItems.Add(new RewardItemGrantApplied(grant.ItemId, grant.Quantity));
|
appliedItems.Add(new RewardItemGrantApplied(grant.ItemId, grant.Quantity));
|
||||||
}
|
}
|
||||||
|
|
||||||
var progressionRollbackPoint = SkillProgressionGrantOperations.CaptureRollbackPoint(
|
var perksUnlockedByThisCall = new HashSet<string>(StringComparer.Ordinal);
|
||||||
playerId,
|
|
||||||
skillProgressionStore,
|
|
||||||
perkStore);
|
|
||||||
var appliedSkillXp = new List<RewardSkillXpGrantApplied>(skillGrants.Count);
|
var appliedSkillXp = new List<RewardSkillXpGrantApplied>(skillGrants.Count);
|
||||||
foreach (var grant in skillGrants)
|
foreach (var grant in skillGrants)
|
||||||
{
|
{
|
||||||
|
var perkBeforeGrant = perkStore.GetSnapshot(playerId);
|
||||||
var skillOutcome = SkillProgressionGrantOperations.TryApplyGrant(
|
var skillOutcome = SkillProgressionGrantOperations.TryApplyGrant(
|
||||||
playerId,
|
playerId,
|
||||||
grant.SkillId,
|
grant.SkillId,
|
||||||
|
|
@ -130,6 +128,10 @@ public static class RewardRouterOperations
|
||||||
}
|
}
|
||||||
|
|
||||||
appliedSkillXp.Add(new RewardSkillXpGrantApplied(grant.SkillId, grant.Amount));
|
appliedSkillXp.Add(new RewardSkillXpGrantApplied(grant.SkillId, grant.Amount));
|
||||||
|
SkillProgressionGrantOperations.CollectPerksUnlockedSince(
|
||||||
|
perkBeforeGrant,
|
||||||
|
perkStore.GetSnapshot(playerId),
|
||||||
|
perksUnlockedByThisCall);
|
||||||
}
|
}
|
||||||
|
|
||||||
var deliveredAt = timeProvider.GetUtcNow();
|
var deliveredAt = timeProvider.GetUtcNow();
|
||||||
|
|
@ -149,12 +151,18 @@ public static class RewardRouterOperations
|
||||||
// Lost TryRecord race after apply — rollback this call's grants (mirror
|
// Lost TryRecord race after apply — rollback this call's grants (mirror
|
||||||
// EncounterCompletionOperations when TryMarkCompleted returns false).
|
// EncounterCompletionOperations when TryMarkCompleted returns false).
|
||||||
CompensatingRemoveItems(playerId, appliedItems, itemRegistry, inventoryStore);
|
CompensatingRemoveItems(playerId, appliedItems, itemRegistry, inventoryStore);
|
||||||
_ = SkillProgressionGrantOperations.TryRestoreRollbackPoint(
|
_ = SkillProgressionGrantOperations.TryRevertAppliedSkillGrants(
|
||||||
playerId,
|
playerId,
|
||||||
progressionRollbackPoint,
|
|
||||||
appliedSkillXp,
|
appliedSkillXp,
|
||||||
|
perksUnlockedByThisCall,
|
||||||
skillProgressionStore,
|
skillProgressionStore,
|
||||||
perkStore);
|
perkStore);
|
||||||
|
ResyncPathAutoPerksAfterRaceRollback(
|
||||||
|
playerId,
|
||||||
|
appliedSkillXp,
|
||||||
|
skillProgressionStore,
|
||||||
|
levelCurve,
|
||||||
|
perkUnlockEngine);
|
||||||
|
|
||||||
if (deliveryStore.TryGet(normalizedPlayerId, normalizedQuestId, out var racedEvent))
|
if (deliveryStore.TryGet(normalizedPlayerId, normalizedQuestId, out var racedEvent))
|
||||||
{
|
{
|
||||||
|
|
@ -164,6 +172,27 @@ public static class RewardRouterOperations
|
||||||
return Deny(RewardDeliveryReasonCodes.AlreadyDelivered);
|
return Deny(RewardDeliveryReasonCodes.AlreadyDelivered);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
private static void ResyncPathAutoPerksAfterRaceRollback(
|
||||||
|
string playerId,
|
||||||
|
IReadOnlyList<RewardSkillXpGrantApplied> 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) =>
|
private static string MapInventoryReason(string? reasonCode) =>
|
||||||
reasonCode switch
|
reasonCode switch
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -103,20 +103,26 @@ public static class SkillProgressionGrantOperations
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
/// <summary>Captures skill XP totals and perk snapshot before a grant batch for compensating rollback.</summary>
|
/// <summary>Collects perk ids unlocked between two snapshots (this batch only).</summary>
|
||||||
public static SkillProgressionGrantRollbackPoint CaptureRollbackPoint(
|
public static void CollectPerksUnlockedSince(
|
||||||
string playerId,
|
PerkStateSnapshot before,
|
||||||
IPlayerSkillProgressionStore xpStore,
|
PerkStateSnapshot after,
|
||||||
IPlayerPerkStateStore perkStore) =>
|
ISet<string> sink)
|
||||||
new(
|
{
|
||||||
new Dictionary<string, int>(xpStore.GetXpTotals(playerId), StringComparer.Ordinal),
|
foreach (var perkId in after.UnlockedPerkIds)
|
||||||
perkStore.GetSnapshot(playerId));
|
{
|
||||||
|
if (!before.UnlockedPerkIds.Contains(perkId))
|
||||||
|
{
|
||||||
|
sink.Add(perkId);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/// <summary>Restores skill XP and perk state captured before a failed grant batch commit.</summary>
|
/// <summary>Reverts skill XP grants and perks unlocked during this failed batch commit.</summary>
|
||||||
public static bool TryRestoreRollbackPoint(
|
public static bool TryRevertAppliedSkillGrants(
|
||||||
string playerId,
|
string playerId,
|
||||||
SkillProgressionGrantRollbackPoint rollbackPoint,
|
|
||||||
IReadOnlyList<RewardSkillXpGrantApplied> appliedGrants,
|
IReadOnlyList<RewardSkillXpGrantApplied> appliedGrants,
|
||||||
|
IReadOnlyCollection<string> perksUnlockedByBatch,
|
||||||
IPlayerSkillProgressionStore xpStore,
|
IPlayerSkillProgressionStore xpStore,
|
||||||
IPlayerPerkStateStore perkStore)
|
IPlayerPerkStateStore perkStore)
|
||||||
{
|
{
|
||||||
|
|
@ -129,6 +135,7 @@ public static class SkillProgressionGrantOperations
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
return perkStore.TryRestoreSnapshot(playerId, rollbackPoint.PerkState);
|
return perksUnlockedByBatch.Count == 0
|
||||||
|
|| perkStore.TryRemoveUnlockedPerks(playerId, perksUnlockedByBatch);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -1,8 +0,0 @@
|
||||||
using NeonSprawl.Server.Game.Mastery;
|
|
||||||
|
|
||||||
namespace NeonSprawl.Server.Game.Skills;
|
|
||||||
|
|
||||||
/// <summary>Captured skill XP + perk state before a grant apply batch (NEO-127 compensating rollback).</summary>
|
|
||||||
public readonly record struct SkillProgressionGrantRollbackPoint(
|
|
||||||
IReadOnlyDictionary<string, int> SkillXpTotals,
|
|
||||||
PerkStateSnapshot PerkState);
|
|
||||||
|
|
@ -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. |
|
| **`unknown_skill`**, **`invalid_amount`**, **`source_kind_not_allowed`** | Skill XP grant denied. |
|
||||||
| **`progression_store_missing`** | Skill progression store could not apply XP delta. |
|
| **`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)
|
## Encounter definitions (NEO-103)
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue