What's wrong
JsonKeybindingRepository.SaveProfileAsync and DeleteProfileAsync (Keybinding/Services/JsonKeybindingRepository.cs L43-L149) each follow the same steps:
- Read the whole of
profiles.json.
- Modify the list.
- Write the whole file back with
File.WriteAllTextAsync.
Nothing synchronises these steps, so two concurrent calls overwrite each other's changes. There is a worse case: LoadAllProfilesInternalAsync turns a JsonException into an empty list (L116-L120). If a save reads a half-written file from another save, it treats the store as empty and writes back a file that contains only its own profile, which wipes every other profile.
CLAUDE.md promises "thread-safe operations in all services". #142 recently fixed the same class of problem for Profile chords, but it never covered the persistence layer.
Reproduction
These were run on Linux against the current main; no exception was thrown in any run.
- Eight parallel
repo.SaveProfileAsync(new Profile("p" + i, …)) calls against a fresh directory, followed by LoadAllProfilesAsync(), returned fewer than 8 profiles in 21 of 30 trials.
- A single
KeybindingManager with 5 profiles ran Task.WhenAll(m.SaveAsync(), m.SaveAsync()), and a fresh manager then loaded it. The fresh manager saw fewer than 5 profiles in 13 of 30 trials.
On Windows, the overlapping WriteAllTextAsync calls can also throw IOException (sharing violation).
A realistic trigger is a DI-singleton KeybindingManager (as in the WebAPI example) handling two requests that both call SaveAsync.
Suggested fix
- Serialise every read-modify-write of each file with a
SemaphoreSlim(1, 1) held by the repository.
- Write to a temporary file in the same directory, then
File.Move(temp, path, overwrite: true), so a reader never sees a partial file.
- On the save and delete paths, don't treat a
JsonException as an empty store. Rethrow it, or refuse to write, so one bad read cannot erase the file.
- Optionally, add a batch
SaveProfilesAsync(IEnumerable<Profile>) so KeybindingManager.SaveAsync writes once instead of once per profile.
- Add a regression test that runs parallel
SaveProfileAsync calls with distinct IDs and asserts that all profiles are present afterwards.
#93 (delegating persistence to ktsu.AppDataStorage) would replace this code. Until then, this bug is live.
What's wrong
JsonKeybindingRepository.SaveProfileAsyncandDeleteProfileAsync(Keybinding/Services/JsonKeybindingRepository.csL43-L149) each follow the same steps:profiles.json.File.WriteAllTextAsync.Nothing synchronises these steps, so two concurrent calls overwrite each other's changes. There is a worse case:
LoadAllProfilesInternalAsyncturns aJsonExceptioninto an empty list (L116-L120). If a save reads a half-written file from another save, it treats the store as empty and writes back a file that contains only its own profile, which wipes every other profile.CLAUDE.md promises "thread-safe operations in all services". #142 recently fixed the same class of problem for
Profilechords, but it never covered the persistence layer.Reproduction
These were run on Linux against the current
main; no exception was thrown in any run.repo.SaveProfileAsync(new Profile("p" + i, …))calls against a fresh directory, followed byLoadAllProfilesAsync(), returned fewer than 8 profiles in 21 of 30 trials.KeybindingManagerwith 5 profiles ranTask.WhenAll(m.SaveAsync(), m.SaveAsync()), and a fresh manager then loaded it. The fresh manager saw fewer than 5 profiles in 13 of 30 trials.On Windows, the overlapping
WriteAllTextAsynccalls can also throwIOException(sharing violation).A realistic trigger is a DI-singleton
KeybindingManager(as in the WebAPI example) handling two requests that both callSaveAsync.Suggested fix
SemaphoreSlim(1, 1)held by the repository.File.Move(temp, path, overwrite: true), so a reader never sees a partial file.JsonExceptionas an empty store. Rethrow it, or refuse to write, so one bad read cannot erase the file.SaveProfilesAsync(IEnumerable<Profile>)soKeybindingManager.SaveAsyncwrites once instead of once per profile.SaveProfileAsynccalls with distinct IDs and asserts that all profiles are present afterwards.#93 (delegating persistence to ktsu.AppDataStorage) would replace this code. Until then, this bug is live.