Lock a profile's chords so concurrent binds cannot corrupt them [minor]

Keybinding.Test/ProfileChordAccessTests.cs

@@ -0,0 +1,87 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.Keybinding.Test;
+
+using ktsu.Keybinding.Core.Models;
+using ktsu.Keybinding.Core.Services;
+
+[TestClass]
+public class ProfileChordAccessTests
+{
+	private static readonly Chord CtrlA = Chord.Parse("Ctrl+A");
+	private static readonly Chord CtrlB = Chord.Parse("Ctrl+B");
+	private static readonly string[] BothCommands = ["a", "b"];
+
+	[TestMethod]
+	public void ChordAccessors_ReflectSetRemoveAndClear()
+	{
+		Profile profile = new("p", "Profile");
+
+		profile.SetChord(" a ", CtrlA);
+		profile.SetChord("b", CtrlB);
+
+		Assert.AreEqual(2, profile.ChordCount);
+		Assert.AreEqual(CtrlA, profile.GetChord("a"));
+		Assert.IsNull(profile.GetChord("missing"));
+		Assert.IsTrue(profile.HasChord(" a "));
+		Assert.IsFalse(profile.HasChord("missing"));
+		CollectionAssert.AreEquivalent(BothCommands, profile.BoundCommands.ToArray());
+
+		Assert.IsTrue(profile.RemoveChord("a"));
+		Assert.IsFalse(profile.RemoveChord("a"));
+		Assert.AreEqual(1, profile.ChordCount);
+
+		profile.ClearChords();
+		Assert.AreEqual(0, profile.ChordCount);
+		Assert.IsEmpty(profile.GetAllChords());
+	}
+
+	[TestMethod]
+	public void ChordAccessors_RejectBlankCommandIds()
+	{
+		Profile profile = new("p", "Profile");
+
+		Assert.ThrowsExactly<ArgumentException>(() => profile.SetChord(" ", CtrlA));
+		Assert.ThrowsExactly<ArgumentException>(() => profile.GetChord(" "));
+		Assert.ThrowsExactly<ArgumentException>(() => profile.HasChord(" "));
+		Assert.ThrowsExactly<ArgumentException>(() => profile.RemoveChord(" "));
+	}
+
+	[TestMethod]
+	public void ObsoleteChords_StillExposesTheLiveBindings()
+	{
+		Profile profile = new("p", "Profile");
+		profile.SetChord("a", CtrlA);
+
+#pragma warning disable CS0618 // Type or member is obsolete
+		Dictionary<string, Chord> chords = profile.Chords;
+#pragma warning restore CS0618 // Type or member is obsolete
+
+		Assert.HasCount(1, chords);
+		profile.SetChord("b", CtrlB);
+		Assert.HasCount(2, chords, "Chords keeps returning the live dictionary for existing callers");
+	}
+
+	[TestMethod]
+	public void FindAndExecuteChord_UseTheLockedSnapshot()
+	{
+		CommandRegistry registry = new();
+		ProfileManager profiles = new();
+		profiles.CreateProfile("p", "Profile");
+		profiles.SetActiveProfile("p");
+		KeybindingService service = new(registry, profiles);
+		registry.RegisterCommand(new Command("a", "A"));
+		registry.RegisterCommand(new Command("b", "B"));
+
+		Assert.IsTrue(service.BindChord("a", CtrlA));
+		Assert.IsTrue(service.BindChord("b", CtrlB));
+
+		Assert.AreEqual("a", service.FindCommandByChord(CtrlA));
+		Assert.IsNull(service.FindCommandByChord(Chord.Parse("Ctrl+C")));
+		Assert.AreEqual("b", service.ExecuteChord(CtrlB));
+
+		registry.UnregisterCommand("b");
+		Assert.IsNull(service.ExecuteChord(CtrlB), "A binding whose command is unregistered is skipped");
+		Assert.AreEqual("b", service.FindCommandByChord(CtrlB), "FindCommandByChord does not filter by registration");
+	}
+}

Keybinding.Test/ProfileChordConcurrencyTests.cs

@@ -0,0 +1,49 @@
+// Copyright (c) 2023-2026 ktsu-dev contributors
+
+namespace ktsu.Keybinding.Test;
+
+using System.Globalization;
+using ktsu.Keybinding.Core.Models;
+using ktsu.Keybinding.Core.Services;
+
+[TestClass]
+public class ProfileChordConcurrencyTests
+{
+	private const int CommandCount = 20_000;
+
+	[TestMethod]
+	public void BindUnbindAndFind_CalledInParallel_KeepEveryBindingAndDoNotThrow()
+	{
+		CommandRegistry registry = new();
+		ProfileManager profiles = new();
+		profiles.CreateProfile("p", "Profile");
+		profiles.SetActiveProfile("p");
+		KeybindingService service = new(registry, profiles);
+
+		for (int i = 0; i < CommandCount; i++)
+		{
+			registry.RegisterCommand(new Command($"c{i}", $"C{i}"));
+		}
+
+		Chord chord = Chord.Parse("Ctrl+K");
+
+		// Odd commands stay bound, even ones are unbound again, and reads race the writes.
+		Parallel.For(0, CommandCount, i =>
+		{
+			string commandId = $"c{i}";
+			Assert.IsTrue(service.BindChord(commandId, chord));
+			_ = service.FindCommandByChord(chord);
+			_ = service.GetAllChords();
+
+			if (i % 2 == 0)
+			{
+				Assert.IsTrue(service.UnbindChord(commandId));
+			}
+		});
+
+		Profile profile = profiles.GetProfile("p")!;
+		Assert.AreEqual(CommandCount / 2, profile.ChordCount);
+		Assert.HasCount(CommandCount / 2, service.GetAllChords());
+		Assert.IsTrue(service.GetAllChords().Keys.All(id => int.Parse(id[1..], CultureInfo.InvariantCulture) % 2 == 1));
+	}
+}

Keybinding/KeybindingManager.cs

@@ -199,7 +199,7 @@
 		DisposalHelper.ThrowIfDisposed(_disposed, this);
 		Ensure.NotNull(chords);
 
 		Profile activeProfile = Profiles.GetActiveProfile() ?? throw new InvalidOperationException("No active profile is set");
 
 		return OperationHelper.ExecuteWithCount(chords, Keybindings.BindChord);
 	}
@@ -215,7 +215,7 @@
 		Profile? activeProfile = Profiles.GetActiveProfile();
 		int totalCommands = Commands.GetAllCommands().Count;
 		int totalProfiles = Profiles.GetAllProfiles().Count;
-		int activeKeybindings = activeProfile?.Chords.Count ?? 0;
+		int activeKeybindings = activeProfile?.ChordCount ?? 0;
 
 		return new KeybindingSummary
 		{

Keybinding/Models/Profile.cs

@@ -31,9 +31,14 @@
 		Id = id.Trim();
 		Name = name.Trim();
 		Description = description?.Trim();
-		Chords = [];
 	}
 
+	[System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0032:Use auto property", Justification = "The only property over this field is obsolete, and the field is what the lock guards.")]
+	private readonly Dictionary<string, Chord> _chords = [];
+
+	// Every read and write of _chords takes this lock, so bindings can change from any thread.
+	private readonly Lock _chordsLock = new();
+
 	/// <summary>
 	/// Gets the unique profile identifier
 	/// </summary>
@@ -68,9 +73,29 @@
 	}
 
 	/// <summary>
-	/// Gets the chord bindings for this profile (command ID to chord mapping)
+	/// Gets the live chord bindings for this profile (command ID to chord mapping)
 	/// </summary>
-	public Dictionary<string, Chord> Chords { get; }
+	/// <remarks>
+	/// Reading or changing this dictionary bypasses the profile's synchronization, so it is not
+	/// thread-safe. Use <see cref="GetAllChords"/>, <see cref="SetChord"/>, <see cref="RemoveChord"/>
+	/// and <see cref="ClearChords"/> instead.
+	/// </remarks>
+	[Obsolete("Chords is not thread-safe. Use GetAllChords, SetChord, RemoveChord or ClearChords instead.")]
+	public Dictionary<string, Chord> Chords => _chords;
+
+	/// <summary>
+	/// Gets the number of chord bindings in this profile
+	/// </summary>
+	public int ChordCount
+	{
+		get
+		{
+			lock (_chordsLock)
+			{
+				return _chords.Count;
+			}
+		}
+	}
 
 	/// <summary>
 	/// Sets a chord binding for a command in this profile
@@ -83,12 +108,15 @@
 	{
 		if (string.IsNullOrWhiteSpace(commandId))
 		{
 			throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId));
 		}
 
 		Ensure.NotNull(chord);
 
-		Chords[commandId.Trim()] = chord;
+		lock (_chordsLock)
+		{
+			_chords[commandId.Trim()] = chord;
+		}
 	}
 
 	/// <summary>
@@ -99,18 +127,47 @@
 	/// <exception cref="ArgumentException">Thrown when commandId is null or whitespace</exception>
 	public Chord? GetChord(string commandId)
 	{
-		return string.IsNullOrWhiteSpace(commandId)
-			? throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId))
-			: Chords.TryGetValue(commandId.Trim(), out Chord? chord)
-			? chord
-			: null;
+		if (string.IsNullOrWhiteSpace(commandId))
+		{
+			throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId));
+		}
+
+		lock (_chordsLock)
+		{
+			return _chords.TryGetValue(commandId.Trim(), out Chord? chord) ? chord : null;
+		}
 	}
 
 	/// <summary>
 	/// Gets all chord bindings for this profile
 	/// </summary>
 	/// <returns>Dictionary of command ID to chord mappings</returns>
-	public IReadOnlyDictionary<string, Chord> GetAllChords() => new Dictionary<string, Chord>(Chords).AsReadOnly();
+	public IReadOnlyDictionary<string, Chord> GetAllChords()
+	{
+		lock (_chordsLock)
+		{
+			return new Dictionary<string, Chord>(_chords).AsReadOnly();
+		}
+	}
+
+	/// <summary>
+	/// Finds the first command bound to a chord in this profile that satisfies a condition
+	/// </summary>
+	/// <param name="chord">The chord to look up</param>
+	/// <param name="predicate">An optional condition the command ID must satisfy</param>
+	/// <returns>The command ID if found, null otherwise</returns>
+	internal string? FindCommand(Chord chord, Func<string, bool>? predicate = null)
+	{
+		foreach (KeyValuePair<string, Chord> binding in GetAllChords())
+		{
+			if (binding.Value.Equals(chord) && (predicate is null || predicate(binding.Key)))
+			{
+				return binding.Key;
+			}
+		}
+
+		return null;
+	}
 
 	/// <summary>
 	/// Checks if a command has a chord binding in this profile
@@ -120,9 +177,15 @@
 	/// <exception cref="ArgumentException">Thrown when commandId is null or whitespace</exception>
 	public bool HasChord(string commandId)
 	{
-		return string.IsNullOrWhiteSpace(commandId)
-			? throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId))
-			: Chords.ContainsKey(commandId.Trim());
+		if (string.IsNullOrWhiteSpace(commandId))
+		{
+			throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId));
+		}
+
+		lock (_chordsLock)
+		{
+			return _chords.ContainsKey(commandId.Trim());
+		}
 	}
 
 	/// <summary>
@@ -133,21 +196,42 @@
 	/// <exception cref="ArgumentException">Thrown when commandId is null or whitespace</exception>
 	public bool RemoveChord(string commandId)
 	{
-		return string.IsNullOrWhiteSpace(commandId)
-			? throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId))
-			: Chords.Remove(commandId.Trim());
+		if (string.IsNullOrWhiteSpace(commandId))
+		{
+			throw new ArgumentException("Command ID cannot be null or whitespace", nameof(commandId));
+		}
+
+		lock (_chordsLock)
+		{
+			return _chords.Remove(commandId.Trim());
+		}
 	}
 
 	/// <summary>
 	/// Gets all command IDs that have chord bindings in this profile
 	/// </summary>
 	/// <returns>Collection of command IDs</returns>
-	public IReadOnlyCollection<string> BoundCommands => [.. Chords.Keys];
+	public IReadOnlyCollection<string> BoundCommands
+	{
+		get
+		{
+			lock (_chordsLock)
+			{
+				return [.. _chords.Keys];
+			}
+		}
+	}
 
 	/// <summary>
 	/// Clears all chord bindings from this profile
 	/// </summary>
-	public void ClearChords() => Chords.Clear();
+	public void ClearChords()
+	{
+		lock (_chordsLock)
+		{
+			_chords.Clear();
+		}
+	}
 
 	/// <summary>
 	/// Returns a string representation of the profile

Keybinding/Services/JsonKeybindingRepository.cs

@@ -59,7 +59,7 @@ public async Task SaveProfileAsync(Profile profile)
 			Id = p.Id,
 			Name = p.Name,
 			Description = p.Description,
-			Chords = p.Chords.ToDictionary(
+			Chords = p.GetAllChords().ToDictionary(
 				kvp => kvp.Key,
 				kvp => new ChordDto
 				{
@@ -137,7 +137,7 @@ public async Task DeleteProfileAsync(string profileId)
 			Id = p.Id,
 			Name = p.Name,
 			Description = p.Description,
-			Chords = p.Chords.ToDictionary(
+			Chords = p.GetAllChords().ToDictionary(
 				kvp => kvp.Key,
 				kvp => new ChordDto
 				{

Keybinding/Services/KeybindingService.cs

@@ -36,7 +36,7 @@
 			throw new ArgumentException("Profile ID cannot be null or whitespace", nameof(id));
 		}
 
 		ArgumentException.ThrowIfNullOrWhiteSpace(name, nameof(name));
 
 		return _profileManager.CreateProfile(id, name, description);
 	}
@@ -247,9 +247,7 @@
 		// bindings in place, so skip bindings whose command is no longer registered rather than
 		// giving up on the first match.
 		Profile? profile = _profileManager.GetProfile(profileId);
-		string? commandId = profile?.Chords
-			.FirstOrDefault(kvp => kvp.Value.Equals(chord) && _commandRegistry.IsCommandRegistered(kvp.Key))
-			.Key;
+		string? commandId = profile?.FindCommand(chord, _commandRegistry.IsCommandRegistered);
 
 		// In a real implementation, this would trigger command execution
 		// For now, we just return the command ID that would be executed
@@ -298,8 +296,6 @@
 		}
 
 		Profile? profile = _profileManager.GetProfile(profileId);
-		return profile?.Chords
-			.FirstOrDefault(kvp => kvp.Value.Equals(chord))
-			.Key;
+		return profile?.FindCommand(chord);
 	}
 }

Keybinding/Services/ProfileManager.cs

@@ -32,7 +32,7 @@
 	{
 		if (string.IsNullOrWhiteSpace(id))
 		{
 			throw new ArgumentException("Profile ID cannot be null or whitespace", nameof(id));
 		}
 
 		if (string.IsNullOrWhiteSpace(name))
@@ -73,7 +73,7 @@
 	{
 		return string.IsNullOrWhiteSpace(profileId)
 			? throw new ArgumentException("Profile ID cannot be null or whitespace", nameof(profileId))
 			: _profiles.TryGetValue(profileId.Trim(), out Profile? profile) ? profile : null;
 	}
 
 	/// <inheritdoc/>
@@ -155,7 +155,7 @@
 		Profile newProfile = new(normalizedNewId, newProfileName.Trim(), newDescription);
 
 		// Copy all chords from source profile
-		foreach (KeyValuePair<string, Chord> kvp in sourceProfile.Chords)
+		foreach (KeyValuePair<string, Chord> kvp in sourceProfile.GetAllChords())
 		{
 			newProfile.SetChord(kvp.Key, kvp.Value);
 		}