Skip to content

Concurrent BindChord calls corrupt a profile's bindings, despite the "thread-safe services" guarantee #109

Closed

Description

@matt-edmondson

What's wrong

CLAUDE.md promises "Thread-safe operations in all services", and ProfileManager / CommandRegistry do use ConcurrentDictionary. Each Profile, however, stores its bindings in a plain dictionary (Models/Profile.cs):

public Dictionary<string, Chord> Chords { get; }

SetChord, RemoveChord and ClearChords modify it without synchronization. They are reached from KeybindingService.BindChord / UnbindChord through profile.SetChord(commandId, chord). At the same time, FindCommandByChord and SaveAsync enumerate the same dictionary.

Reproduction (from the review, against current main)

A Parallel.For of 20,000 BindChord calls on the active profile threw an AggregateException wrapping InvalidOperationException (concurrent modification). It left 435 bindings instead of 20,000. Concurrent reads during a bind can also throw "Collection was modified" from FindCommandByChord or SaveAsync.

A related race is in ProfileManager.RenameProfile. It copies the chords into a new Profile and then runs a non-atomic TryRemove followed by TryAdd. A bind that lands on the old instance in between is lost, and GetProfile(id) briefly returns null.

Why it matters

Bindings can be changed from input handlers, background loads, or UI threads. A torn Dictionary can lose bindings, throw, or in the worst case corrupt its internal state and spin forever.

Suggested fix

  • Back Profile.Chords with a ConcurrentDictionary<string, Chord>, or lock inside Profile, and expose it only as IReadOnlyDictionary. Today it is a public mutable Dictionary, which lets callers bypass any locking.
  • Make the rename swap atomic: assign _profiles[id] = updated under the existing lock rather than doing TryRemove + TryAdd.

Acceptance criteria: a stress test that runs parallel BindChord, UnbindChord and FindCommandByChord completes with no exception and the expected final binding count.

Activity

  1. matt-edmondson commented on Sep 26, 2026

    @matt-edmondson
    ContributorAuthor

    Triage

    • Category: Bug
    • Priority: High. This breaks the documented "thread-safe services" guarantee. In the reproduction, 20,000 parallel binds left 435 bindings and threw. Unsynchronized Dictionary mutation can also corrupt the dictionary's internal state (possible infinite loop). The RenameProfile swap race is a second, smaller hole.
    • Area: Models/Profile.cs (Chords), ProfileManager.RenameProfile, KeybindingService.BindChord/UnbindChord
    • Suggested assignment: Keybinding maintainer, services/models area
    • Duplicates: None found.
    • In progress: No open PR covers this. PR Persist profile deletion in KeybindingManager.SaveAsync #111 (profile deletion persistence) touches SaveAsync, which enumerates the same dictionary, so expect a possible merge overlap.

    Notes: Changing Chords from a public mutable Dictionary to IReadOnlyDictionary is a public API break and needs a major-version bump or an [Obsolete] transition. If that is unwanted, keeping the type and locking inside Profile still leaves callers able to bypass the lock. Decide this before implementing.


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

readyFully specified; implement as written

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions