Fix PositionSnapshotHolder returning live reference instead of copy

mikasoukhov · mikasoukhov · commit 88cbb3d85b5b · 2026-03-18T05:20:14.000+03:00
TryGetSnapshot and Process returned the internal dictionary reference,
allowing external code to mutate internal snapshot state. Now returns
TypedClone() like OrderSnapshotHolder does.
diff --git a/Messages/PositionSnapshotHolder.cs b/Messages/PositionSnapshotHolder.cs
@@ -4,7 +4,7 @@ namespace StockSharp.Messages;
 /// <see cref="PositionChangeMessage"/> snapshots holder.
 /// </summary>
 /// <remarks>
-/// Returns the same reference for the same position key (PortfolioName + SecurityId + StrategyId + Side + ClientCode + DepoName + LimitType).
+/// Returns a copy of the snapshot for the same position key (PortfolioName + SecurityId + StrategyId + Side + ClientCode + DepoName + LimitType).
 /// </remarks>
 public class PositionSnapshotHolder : BaseLogReceiver
 {
@@ -32,7 +32,15 @@ public PositionSnapshotHolder()
 	public bool TryGetSnapshot(string portfolioName, SecurityId securityId, string strategyId, Sides? side, string clientCode, string depoName, TPlusLimits? limitType, out PositionChangeMessage snapshot)
 	{
 		var key = CreateKey(portfolioName, securityId, strategyId, side, clientCode, depoName, limitType);
-		return _snapshots.TryGetValue(key, out snapshot);
+
+		if (_snapshots.TryGetValue(key, out var stored))
+		{
+			snapshot = stored.TypedClone();
+			return true;
+		}
+
+		snapshot = null;
+		return false;
 	}
 
 	/// <summary>
@@ -46,15 +54,22 @@ public bool TryGetSnapshot(PositionChangeMessage posMsg, out PositionChangeMessa
 		if (posMsg is null)
 			throw new ArgumentNullException(nameof(posMsg));
 
-		return _snapshots.TryGetValue(CreateKey(posMsg), out snapshot);
+		if (_snapshots.TryGetValue(CreateKey(posMsg), out var stored))
+		{
+			snapshot = stored.TypedClone();
+			return true;
+		}
+
+		snapshot = null;
+		return false;
 	}
 
 	/// <summary>
 	/// Process <see cref="PositionChangeMessage"/> change.
 	/// </summary>
 	/// <param name="posMsg"><see cref="PositionChangeMessage"/> change.</param>
 	/// <returns>
-	/// Position snapshot. Returns the same reference for the same position key.
+	/// Position snapshot copy.
 	/// </returns>
 	public PositionChangeMessage Process(PositionChangeMessage posMsg)
 	{
@@ -68,13 +83,13 @@ public PositionChangeMessage Process(PositionChangeMessage posMsg)
 			if (_snapshots.TryGetValue(key, out var snapshot))
 			{
 				ApplyChanges(snapshot, posMsg);
-				return snapshot;
+				return snapshot.TypedClone();
 			}
 			else
 			{
-				snapshot = (PositionChangeMessage)posMsg.Clone();
+				snapshot = posMsg.TypedClone();
 				_snapshots.Add(key, snapshot);
-				return snapshot;
+				return snapshot.TypedClone();
 			}
 		}
 	}
diff --git a/Tests/SnapshotHolderTests.cs b/Tests/SnapshotHolderTests.cs
@@ -2196,7 +2196,7 @@ public void Position_TryGetSnapshot_NoSnapshot_ReturnsFalse()
 	}
 
 	[TestMethod]
-	public void Position_Process_ReturnsSameReference()
+	public void Position_Process_ReturnsCopy()
 	{
 		var holder = new PositionSnapshotHolder();
 		var msg1 = new PositionChangeMessage
@@ -2219,8 +2219,13 @@ public void Position_Process_ReturnsSameReference()
 
 		var result2 = holder.Process(msg2);
 
-		// Must return same reference
-		result1.AssertSame(result2);
+		// Must return different references (copies)
+		result1.AssertNotSame(result2);
+
+		// result1 has state from first Process call
+		result1.Changes[PositionChangeTypes.CurrentValue].AssertEqual(100m);
+		// result2 has updated state
+		result2.Changes[PositionChangeTypes.CurrentValue].AssertEqual(150m);
 	}
 
 	[TestMethod]
@@ -2237,9 +2242,7 @@ public void Position_Process_UpdatesExistingSnapshot()
 		.TryAdd(PositionChangeTypes.CurrentValue, 100m)
 		.TryAdd(PositionChangeTypes.AveragePrice, 50m);
 
-		var snapshot = holder.Process(msg1);
-		snapshot.Changes[PositionChangeTypes.CurrentValue].AssertEqual(100m);
-		snapshot.Changes[PositionChangeTypes.AveragePrice].AssertEqual(50m);
+		holder.Process(msg1);
 
 		var msg2 = new PositionChangeMessage
 		{
@@ -2250,9 +2253,9 @@ public void Position_Process_UpdatesExistingSnapshot()
 		.TryAdd(PositionChangeTypes.CurrentValue, 150m)
 		.TryAdd(PositionChangeTypes.UnrealizedPnL, 25m);
 
-		holder.Process(msg2);
+		var snapshot = holder.Process(msg2);
 
-		// Same snapshot should be updated
+		// Returned copy should have merged state
 		snapshot.Changes[PositionChangeTypes.CurrentValue].AssertEqual(150m);
 		snapshot.Changes[PositionChangeTypes.AveragePrice].AssertEqual(50m); // unchanged
 		snapshot.Changes[PositionChangeTypes.UnrealizedPnL].AssertEqual(25m); // new field
@@ -2472,7 +2475,7 @@ public void Position_KeyIsCaseInsensitive()
 		}
 		.TryAdd(PositionChangeTypes.CurrentValue, 100m);
 
-		var snap1 = holder.Process(msg1);
+		holder.Process(msg1);
 
 		var msg2 = new PositionChangeMessage
 		{
@@ -2486,9 +2489,12 @@ public void Position_KeyIsCaseInsensitive()
 
 		var snap2 = holder.Process(msg2);
 
-		// Should be same reference due to case-insensitive key
-		snap1.AssertSame(snap2);
-		snap1.Changes[PositionChangeTypes.CurrentValue].AssertEqual(200m);
+		// Case-insensitive key &mdash; same internal snapshot updated, returned as copy
+		snap2.Changes[PositionChangeTypes.CurrentValue].AssertEqual(200m);
+
+		// Verify via TryGetSnapshot &mdash; should also return 200
+		holder.TryGetSnapshot(msg1, out var retrieved);
+		retrieved.Changes[PositionChangeTypes.CurrentValue].AssertEqual(200m);
 	}
 
 	#endregion
@@ -2774,10 +2780,10 @@ public void Order_TryGetSnapshot_ReturnsCopy()
 	}
 
 	/// <summary>
-	/// BUG #3: Position holder also returns live reference.
+	/// Position TryGetSnapshot must return a copy, not the internal reference.
 	/// </summary>
 	[TestMethod]
-	public void Position_LiveReference_CanBeMutatedExternally_Bug()
+	public void Position_TryGetSnapshot_ReturnsCopy()
 	{
 		var holder = new PositionSnapshotHolder();
 
@@ -2789,23 +2795,44 @@ public void Position_LiveReference_CanBeMutatedExternally_Bug()
 		}
 		.TryAdd(PositionChangeTypes.CurrentValue, 100m);
 
-		var snapshot = holder.Process(msg1);
+		holder.Process(msg1);
 
-		// Get snapshot via TryGetSnapshot
-		holder.TryGetSnapshot(msg1, out var retrieved);
+		// Get snapshot twice
+		holder.TryGetSnapshot(msg1, out var snap1);
+		holder.TryGetSnapshot(msg1, out var snap2);
+
+		// Should be different references (copies)
+		snap1.AssertNotSame(snap2);
 
-		// BUG: Same reference returned
-		snapshot.AssertSame(retrieved);
+		// But with same data
+		snap1.Changes[PositionChangeTypes.CurrentValue].AssertEqual(snap2.Changes[PositionChangeTypes.CurrentValue]);
+	}
 
-		// External code can mutate the internal state!
-		retrieved.Changes[PositionChangeTypes.CurrentValue] = 999m;
+	/// <summary>
+	/// Position Process must return a copy, not the internal reference.
+	/// External mutation must not affect internal state.
+	/// </summary>
+	[TestMethod]
+	public void Position_Process_ReturnsCopy_IsolatedFromInternal()
+	{
+		var holder = new PositionSnapshotHolder();
 
-		// This affects the internal snapshot
-		holder.TryGetSnapshot(msg1, out var afterMutation);
-		afterMutation.Changes[PositionChangeTypes.CurrentValue].AssertEqual(999m);
+		var msg1 = new PositionChangeMessage
+		{
+			PortfolioName = "Portfolio1",
+			SecurityId = _secId1,
+			ServerTime = _now,
+		}
+		.TryAdd(PositionChangeTypes.CurrentValue, 100m);
 
-		// Original 'snapshot' variable is also affected (same reference)
-		snapshot.Changes[PositionChangeTypes.CurrentValue].AssertEqual(999m);
+		var snapshot = holder.Process(msg1);
+
+		// Mutate the returned copy
+		snapshot.Changes[PositionChangeTypes.CurrentValue] = 999m;
+
+		// Internal state must NOT be affected
+		holder.TryGetSnapshot(msg1, out var afterMutation);
+		afterMutation.Changes[PositionChangeTypes.CurrentValue].AssertEqual(100m);
 	}
 
 	/// <summary>
