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.pull/166/head
parent
71fbf3d628
commit
be9bb8f961
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
||||
|
|
|
|||
|
|
@ -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<IPlayerSkillProgressionStore>(),
|
||||
services.GetRequiredService<ISkillLevelCurve>(),
|
||||
services.GetRequiredService<PerkUnlockEngine>(),
|
||||
services.GetRequiredService<IPlayerPerkStateStore>(),
|
||||
services.GetRequiredService<IRewardDeliveryStore>());
|
||||
}
|
||||
|
||||
|
|
@ -446,6 +499,7 @@ public sealed class RewardRouterOperationsTests
|
|||
IPlayerSkillProgressionStore SkillProgressionStore,
|
||||
ISkillLevelCurve LevelCurve,
|
||||
PerkUnlockEngine PerkUnlockEngine,
|
||||
IPlayerPerkStateStore PerkStore,
|
||||
IRewardDeliveryStore DeliveryStore);
|
||||
|
||||
/// <summary>TryGet misses until after a failed <see cref="IRewardDeliveryStore.TryRecord"/> — simulates concurrent race loser.</summary>
|
||||
|
|
|
|||
|
|
@ -19,4 +19,7 @@ public interface IPlayerPerkStateStore
|
|||
|
||||
/// <summary>Clears all branch picks and unlocked perks for the player (NEO-48 dev fixture API).</summary>
|
||||
bool TryResetPerkState(string playerId);
|
||||
|
||||
/// <summary>Replaces branch picks and unlocked perks with <paramref name="snapshot"/> (grant rollback).</summary>
|
||||
bool TryRestoreSnapshot(string playerId, PerkStateSnapshot snapshot);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -123,6 +123,35 @@ public sealed class InMemoryPlayerPerkStateStore(IOptions<GamePositionOptions> o
|
|||
}
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
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();
|
||||
|
|
|
|||
|
|
@ -257,6 +257,79 @@ public sealed class PostgresPlayerPerkStateStore(Npgsql.NpgsqlDataSource dataSou
|
|||
return true;
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
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(
|
||||
|
|
|
|||
|
|
@ -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<RewardSkillXpGrantApplied>(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<RewardSkillXpGrantApplied> 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,
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
});
|
||||
}
|
||||
|
||||
/// <summary>Captures skill XP totals and perk snapshot before a grant batch for compensating rollback.</summary>
|
||||
public static SkillProgressionGrantRollbackPoint CaptureRollbackPoint(
|
||||
string playerId,
|
||||
IPlayerSkillProgressionStore xpStore,
|
||||
IPlayerPerkStateStore perkStore) =>
|
||||
new(
|
||||
new Dictionary<string, int>(xpStore.GetXpTotals(playerId), StringComparer.Ordinal),
|
||||
perkStore.GetSnapshot(playerId));
|
||||
|
||||
/// <summary>Restores skill XP and perk state captured before a failed grant batch commit.</summary>
|
||||
public static bool TryRestoreRollbackPoint(
|
||||
string playerId,
|
||||
SkillProgressionGrantRollbackPoint rollbackPoint,
|
||||
IReadOnlyList<RewardSkillXpGrantApplied> 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);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -0,0 +1,8 @@
|
|||
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. |
|
||||
| **`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)
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue