mirror of
https://github.com/microsoft/PowerToys.git
synced 2026-04-06 03:07:04 +02:00
CmdPal: Make settings and app state immutable (#46451)
## Summary This PR refactors CmdPal settings/state to be immutable end-to-end. ### Core changes - Convert model types to immutable records / init-only properties: - `SettingsModel` - `AppStateModel` - `ProviderSettings` - `DockSettings` - `RecentCommandsManager` - supporting settings types (fallback/hotkey/alias/top-level hotkey/history items, etc.) - Replace mutable collections with immutable equivalents where appropriate: - `ImmutableDictionary<,>` - `ImmutableList<>` - Move mutation flow to atomic service updates: - `ISettingsService.UpdateSettings(Func<SettingsModel, SettingsModel>)` - `IAppStateService.UpdateState(Func<AppStateModel, AppStateModel>)` - Update ViewModels/managers/services to use copy-on-write (`with`) patterns instead of in-place mutation. - Update serialization context + tests for immutable model graph compatibility. ## Why Issue #46437 is caused by mutable shared state being updated from different execution paths/threads, leading to race-prone behavior during persistence/serialization. By making settings/app state immutable and using atomic swap/update patterns, we remove in-place mutation and eliminate this class of concurrency bug. ## Validation - Built successfully: - `Microsoft.CmdPal.UI.ViewModels` - `Microsoft.CmdPal.UI` - `Microsoft.CmdPal.UI.ViewModels.UnitTests` - Updated unit tests for immutable update patterns. Fixes #46437 --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -4,6 +4,7 @@
|
||||
|
||||
using System;
|
||||
using System.Collections.Generic;
|
||||
using System.Collections.Immutable;
|
||||
using System.IO;
|
||||
using Microsoft.CmdPal.Common.Services;
|
||||
using Microsoft.CmdPal.UI.ViewModels.Services;
|
||||
@@ -41,7 +42,7 @@ public class AppStateServiceTests
|
||||
// Arrange
|
||||
var expectedState = new AppStateModel
|
||||
{
|
||||
RunHistory = new List<string> { "command1", "command2" },
|
||||
RunHistory = ImmutableList.Create("command1", "command2"),
|
||||
};
|
||||
_mockPersistence
|
||||
.Setup(p => p.Load(
|
||||
@@ -86,7 +87,8 @@ public class AppStateServiceTests
|
||||
{
|
||||
// Arrange
|
||||
var service = new AppStateService(_mockPersistence.Object, _mockAppInfo.Object);
|
||||
service.State.RunHistory.Add("test-command");
|
||||
service.UpdateState(s => s with { RunHistory = s.RunHistory.Add("test-command") });
|
||||
_mockPersistence.Invocations.Clear(); // Reset after Arrange — UpdateState also persists
|
||||
|
||||
// Act
|
||||
service.Save();
|
||||
@@ -160,4 +162,44 @@ public class AppStateServiceTests
|
||||
// Assert
|
||||
Assert.AreEqual(2, eventCount);
|
||||
}
|
||||
|
||||
[TestMethod]
|
||||
public void UpdateState_ConcurrentUpdates_NoLostUpdates()
|
||||
{
|
||||
// Arrange — two threads each add items to RunHistory concurrently.
|
||||
// With the CAS loop, every add must land (no lost updates).
|
||||
var service = new AppStateService(_mockPersistence.Object, _mockAppInfo.Object);
|
||||
const int iterations = 50;
|
||||
var barrier = new System.Threading.Barrier(2);
|
||||
|
||||
// Act
|
||||
var t1 = System.Threading.Tasks.Task.Run(() =>
|
||||
{
|
||||
barrier.SignalAndWait();
|
||||
for (var i = 0; i < iterations; i++)
|
||||
{
|
||||
service.UpdateState(s => s with { RunHistory = s.RunHistory.Add($"t1-{i}") });
|
||||
}
|
||||
});
|
||||
|
||||
var t2 = System.Threading.Tasks.Task.Run(() =>
|
||||
{
|
||||
barrier.SignalAndWait();
|
||||
for (var i = 0; i < iterations; i++)
|
||||
{
|
||||
service.UpdateState(s => s with { RunHistory = s.RunHistory.Add($"t2-{i}") });
|
||||
}
|
||||
});
|
||||
|
||||
System.Threading.Tasks.Task.WaitAll(t1, t2);
|
||||
|
||||
// Assert — all 100 items must be present (no lost updates)
|
||||
Assert.AreEqual(iterations * 2, service.State.RunHistory.Count, "All concurrent updates should be preserved");
|
||||
|
||||
for (var i = 0; i < iterations; i++)
|
||||
{
|
||||
Assert.IsTrue(service.State.RunHistory.Contains($"t1-{i}"), $"Missing t1-{i}");
|
||||
Assert.IsTrue(service.State.RunHistory.Contains($"t2-{i}"), $"Missing t2-{i}");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -26,7 +26,7 @@ public partial class RecentCommandsTests : CommandPaletteUnitTestBase
|
||||
{
|
||||
foreach (var item in commandIds)
|
||||
{
|
||||
history.AddHistoryItem(item);
|
||||
history = history.WithHistoryItem(item);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -54,7 +54,7 @@ public partial class RecentCommandsTests : CommandPaletteUnitTestBase
|
||||
var history = CreateHistory();
|
||||
|
||||
// Act
|
||||
history.AddHistoryItem("com.microsoft.cmdpal.shell");
|
||||
history = history.WithHistoryItem("com.microsoft.cmdpal.shell");
|
||||
|
||||
// Assert
|
||||
Assert.IsTrue(history.GetCommandHistoryWeight("com.microsoft.cmdpal.shell") > 0);
|
||||
@@ -121,7 +121,7 @@ public partial class RecentCommandsTests : CommandPaletteUnitTestBase
|
||||
var history = new RecentCommandsManager();
|
||||
foreach (var item in items)
|
||||
{
|
||||
history.AddHistoryItem(item.Id);
|
||||
history = history.WithHistoryItem(item.Id);
|
||||
}
|
||||
|
||||
return history;
|
||||
@@ -417,7 +417,7 @@ public partial class RecentCommandsTests : CommandPaletteUnitTestBase
|
||||
// Add extra uses of VS Code to try and push it above Terminal
|
||||
for (var i = 0; i < 10; i++)
|
||||
{
|
||||
history.AddHistoryItem(items[1].Id);
|
||||
history = history.WithHistoryItem(items[1].Id);
|
||||
}
|
||||
|
||||
var weightedScores = items.Select(item => MainListPage.ScoreTopLevelItem(q, item, history, fuzzyMatcher)).ToList();
|
||||
@@ -446,7 +446,7 @@ public partial class RecentCommandsTests : CommandPaletteUnitTestBase
|
||||
var vsCodeId = items[1].Id;
|
||||
for (var i = 0; i < 10; i++)
|
||||
{
|
||||
history.AddHistoryItem(vsCodeId);
|
||||
history = history.WithHistoryItem(vsCodeId);
|
||||
|
||||
var weightedScores = items.Select(item => MainListPage.ScoreTopLevelItem(q, item, history, fuzzyMatcher)).ToList();
|
||||
var weightedMatches = GetMatches(items, weightedScores).ToList();
|
||||
|
||||
@@ -75,7 +75,14 @@ public class SettingsServiceTests
|
||||
public void Settings_ReturnsLoadedModel()
|
||||
{
|
||||
// Arrange
|
||||
_testSettings.ShowAppDetails = true;
|
||||
_testSettings = _testSettings with { ShowAppDetails = true };
|
||||
|
||||
// Reset mock to return updated settings
|
||||
_mockPersistence
|
||||
.Setup(p => p.Load(
|
||||
It.IsAny<string>(),
|
||||
It.IsAny<System.Text.Json.Serialization.Metadata.JsonTypeInfo<SettingsModel>>()))
|
||||
.Returns(_testSettings);
|
||||
|
||||
// Act
|
||||
var service = new SettingsService(_mockPersistence.Object, _mockAppInfo.Object);
|
||||
@@ -89,7 +96,10 @@ public class SettingsServiceTests
|
||||
{
|
||||
// Arrange
|
||||
var service = new SettingsService(_mockPersistence.Object, _mockAppInfo.Object);
|
||||
service.Settings.SingleClickActivates = true;
|
||||
service.UpdateSettings(
|
||||
s => s with { SingleClickActivates = true },
|
||||
hotReload: false);
|
||||
_mockPersistence.Invocations.Clear(); // Reset after Arrange — UpdateSettings also persists
|
||||
|
||||
// Act
|
||||
service.Save(hotReload: false);
|
||||
@@ -178,4 +188,40 @@ public class SettingsServiceTests
|
||||
Assert.AreSame(service, receivedSender);
|
||||
Assert.AreSame(service.Settings, receivedSettings);
|
||||
}
|
||||
|
||||
[TestMethod]
|
||||
public void UpdateSettings_ConcurrentUpdates_NoLostUpdates()
|
||||
{
|
||||
// Arrange — two threads each set a different property to true, 100 times.
|
||||
// Without a CAS loop, one thread's Exchange can overwrite the other's
|
||||
// property back to false from a stale snapshot. With CAS, both survive.
|
||||
var service = new SettingsService(_mockPersistence.Object, _mockAppInfo.Object);
|
||||
const int iterations = 100;
|
||||
var barrier = new System.Threading.Barrier(2);
|
||||
|
||||
// Act — t1 sets ShowAppDetails=true, t2 sets SingleClickActivates=true
|
||||
var t1 = System.Threading.Tasks.Task.Run(() =>
|
||||
{
|
||||
barrier.SignalAndWait();
|
||||
for (var i = 0; i < iterations; i++)
|
||||
{
|
||||
service.UpdateSettings(s => s with { ShowAppDetails = true }, hotReload: false);
|
||||
}
|
||||
});
|
||||
|
||||
var t2 = System.Threading.Tasks.Task.Run(() =>
|
||||
{
|
||||
barrier.SignalAndWait();
|
||||
for (var i = 0; i < iterations; i++)
|
||||
{
|
||||
service.UpdateSettings(s => s with { SingleClickActivates = true }, hotReload: false);
|
||||
}
|
||||
});
|
||||
|
||||
System.Threading.Tasks.Task.WaitAll(t1, t2);
|
||||
|
||||
// Assert — both properties must be true; neither should have been overwritten
|
||||
Assert.IsTrue(service.Settings.ShowAppDetails, "ShowAppDetails lost — a stale snapshot overwrote it");
|
||||
Assert.IsTrue(service.Settings.SingleClickActivates, "SingleClickActivates lost — a stale snapshot overwrote it");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user