diff --git a/src/GameLogic/Player.cs b/src/GameLogic/Player.cs
index 5b9928b..a7f316e 100644
--- a/src/GameLogic/Player.cs
+++ b/src/GameLogic/Player.cs
@@ -54,6 +54,21 @@ public class Player : AsyncDisposable, IBucketMapObserver, IAttackable, IAttacke
private readonly AsyncLock _moveLock = new();
private readonly AsyncLock _experienceLock = new();
+ ///
+ /// Serializes context mutations done by this player's action handlers against the periodic and
+ /// disconnect progress saves, which run on an independent timer flow. See
+ /// .
+ ///
+ private readonly AsyncLock _persistenceLock = new();
+
+ ///
+ /// Tracks, per asynchronous flow, whether is already held, so the
+ /// lock can be re-entered (Nito's is not reentrant). It is an instance
+ /// field on purpose: reentrancy must be tracked per player, so a flow holding player A's lock
+ /// still acquires player B's lock (e.g. during a trade) instead of wrongly skipping it.
+ ///
+ private readonly AsyncLocal _persistenceLockHeld = new();
+
private readonly Walker _walker;
private readonly AppearanceDataAdapter _appearanceData;
@@ -1879,12 +1894,78 @@ public class Player : AsyncDisposable, IBucketMapObserver, IAttackable, IAttacke
/// Success of the save operation.
public async ValueTask SaveProgressAsync(CancellationToken cancellationToken = default)
{
- if (!this.IsTemplatePlayer)
+ if (this.IsTemplatePlayer)
{
- return await this.PersistenceContext.SaveChangesAsync(cancellationToken).ConfigureAwait(false);
+ return true;
}
- return true;
+ return await this.RunPersistenceExclusiveAsync(
+ () => this.PersistenceContext.SaveChangesAsync(cancellationToken),
+ cancellationToken).ConfigureAwait(false);
+ }
+
+ ///
+ /// Runs the given operation while holding this player's persistence lock, so that context
+ /// mutations and progress saves for the player never run concurrently.
+ ///
+ ///
+ /// The periodic progress save () runs on an
+ /// independent timer flow. Action handlers mutate tracked entities with plain field/collection
+ /// writes (e.g. crafting toggling item.ItemOptions) which bypass the persistence context's
+ /// own lock; if such a mutation runs while enumerates the
+ /// change tracker, the save throws (collection-modified / DbUpdateConcurrency) and every following
+ /// save fails too, so the whole session is lost on relog. Serializing the packet handler funnel
+ /// and the save against each other closes that window. The lock is re-entrant per asynchronous
+ /// flow, so an inline save inside an already-serialized handler does not deadlock.
+ ///
+ /// The result type of the operation.
+ /// The operation to run exclusively.
+ /// The cancellation token.
+ /// The result of the operation.
+ public async ValueTask RunPersistenceExclusiveAsync(Func> operation, CancellationToken cancellationToken = default)
+ {
+ if (this._persistenceLockHeld.Value)
+ {
+ return await operation().ConfigureAwait(false);
+ }
+
+ using var l = await this._persistenceLock.LockAsync(cancellationToken).ConfigureAwait(false);
+ this._persistenceLockHeld.Value = true;
+ try
+ {
+ return await operation().ConfigureAwait(false);
+ }
+ finally
+ {
+ this._persistenceLockHeld.Value = false;
+ }
+ }
+
+ ///
+ /// Runs the given operation while holding this player's persistence lock.
+ /// See for the rationale.
+ ///
+ /// The operation to run exclusively.
+ /// The cancellation token.
+ /// A value task which completes when the operation completed.
+ public async ValueTask RunPersistenceExclusiveAsync(Func operation, CancellationToken cancellationToken = default)
+ {
+ if (this._persistenceLockHeld.Value)
+ {
+ await operation().ConfigureAwait(false);
+ return;
+ }
+
+ using var l = await this._persistenceLock.LockAsync(cancellationToken).ConfigureAwait(false);
+ this._persistenceLockHeld.Value = true;
+ try
+ {
+ await operation().ConfigureAwait(false);
+ }
+ finally
+ {
+ this._persistenceLockHeld.Value = false;
+ }
}
///
diff --git a/src/GameServer/RemoteView/RemotePlayer.cs b/src/GameServer/RemoteView/RemotePlayer.cs
index 2c0b584..d7687f7 100644
--- a/src/GameServer/RemoteView/RemotePlayer.cs
+++ b/src/GameServer/RemoteView/RemotePlayer.cs
@@ -151,7 +151,7 @@ public class RemotePlayer : Player, IClientVersionProvider, IHasIpAddress
this.Logger.LogDebug("[C->S] {0}", buffer.ToArray().AsString());
}
- await this.MainPacketHandler.HandlePacketAsync(this, buffer).ConfigureAwait(false);
+ await this.RunPersistenceExclusiveAsync(() => this.MainPacketHandler.HandlePacketAsync(this, buffer)).ConfigureAwait(false);
}
finally
{
diff --git a/tests/MUnique.OpenMU.Tests/PersistenceLockTest.cs b/tests/MUnique.OpenMU.Tests/PersistenceLockTest.cs
new file mode 100644
index 0000000..1ca00dc
--- /dev/null
+++ b/tests/MUnique.OpenMU.Tests/PersistenceLockTest.cs
@@ -0,0 +1,114 @@
+//
+// Licensed under the MIT License. See LICENSE file in the project root for full license information.
+//
+
+namespace MUnique.OpenMU.Tests;
+
+using System.Threading;
+
+///
+/// Tests for the per-player persistence lock (),
+/// which serializes a player's context mutations against its periodic/disconnect progress saves so they
+/// can never run concurrently. Without it, a mutation running during SaveChangesAsync corrupts the
+/// change tracker and rolls the whole session back.
+///
+[TestFixture]
+public class PersistenceLockTest
+{
+ ///
+ /// Verifies that concurrent exclusive operations for the same player never overlap.
+ ///
+ [Test]
+ public async Task ConcurrentAccessIsSerializedAsync()
+ {
+ var player = await PlayerTestHelper.CreatePlayerAsync().ConfigureAwait(false);
+ var concurrent = 0;
+ var overlapDetected = false;
+
+ async ValueTask BodyAsync()
+ {
+ if (Interlocked.Increment(ref concurrent) > 1)
+ {
+ overlapDetected = true;
+ }
+
+ await Task.Delay(1).ConfigureAwait(false);
+ Interlocked.Decrement(ref concurrent);
+ }
+
+ var tasks = Enumerable.Range(0, 50)
+ .Select(_ => player.RunPersistenceExclusiveAsync(BodyAsync).AsTask())
+ .ToArray();
+ await Task.WhenAll(tasks).ConfigureAwait(false);
+
+ Assert.That(overlapDetected, Is.False, "Two exclusive operations for the same player ran at the same time.");
+ }
+
+ ///
+ /// Verifies that re-entering the lock from within an already-held exclusive scope does not deadlock
+ /// (an inline save inside a packet handler is exactly this case).
+ ///
+ [Test]
+ public async Task ReentrantAccessDoesNotDeadlockAsync()
+ {
+ var player = await PlayerTestHelper.CreatePlayerAsync().ConfigureAwait(false);
+ var executed = 0;
+
+ var run = player.RunPersistenceExclusiveAsync(async () =>
+ {
+ Interlocked.Increment(ref executed);
+ await player.RunPersistenceExclusiveAsync(async () =>
+ {
+ Interlocked.Increment(ref executed);
+ await Task.Yield();
+ }).ConfigureAwait(false);
+ }).AsTask();
+
+ // If reentrancy deadlocked, this would hang; fail fast instead of blocking the suite.
+ await run.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false);
+
+ Assert.That(executed, Is.EqualTo(2));
+ }
+
+ ///
+ /// Verifies that a re-entrant exclusive operation still runs while another flow holds the lock:
+ /// the outer flow keeps the lock, an independent flow must wait, and the re-entrant call inside the
+ /// outer flow proceeds without waiting for itself.
+ ///
+ [Test]
+ public async Task IndependentFlowWaitsWhileLockIsHeldAsync()
+ {
+ var player = await PlayerTestHelper.CreatePlayerAsync().ConfigureAwait(false);
+ var otherEntered = false;
+ var holderHasLock = new TaskCompletionSource();
+ var mayRelease = new TaskCompletionSource();
+
+ // Holder runs on its own flow and keeps the lock until signalled.
+ var holder = Task.Run(() => player.RunPersistenceExclusiveAsync(async () =>
+ {
+ holderHasLock.SetResult();
+
+ // A re-entrant call from the holding flow must NOT block on the lock we already hold.
+ await player.RunPersistenceExclusiveAsync(() => ValueTask.CompletedTask).ConfigureAwait(false);
+
+ await mayRelease.Task.ConfigureAwait(false);
+ }).AsTask());
+
+ await holderHasLock.Task.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false);
+
+ // Competing flow started from an INDEPENDENT context (does not inherit the reentrancy flag).
+ var other = Task.Run(() => player.RunPersistenceExclusiveAsync(() =>
+ {
+ otherEntered = true;
+ return ValueTask.CompletedTask;
+ }).AsTask());
+
+ await Task.Delay(50).ConfigureAwait(false);
+ Assert.That(otherEntered, Is.False, "An independent flow entered while the lock was held.");
+
+ mayRelease.SetResult();
+ await holder.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false);
+ await other.WaitAsync(TimeSpan.FromSeconds(5)).ConfigureAwait(false);
+ Assert.That(otherEntered, Is.True, "The competing flow never ran after the lock was released.");
+ }
+}