Skip to content

SetActiveProfile racing DeleteProfile leaves the active id pointing at a deleted profile, so a later profile with that id becomes active on its own #120

Closed

Description

@matt-edmondson

What's wrong

ProfileManager.SetActiveProfile (Keybinding/Services/ProfileManager.cs:105-110) checks the id and then writes it, with no lock between the two steps:

if (!_profiles.ContainsKey(normalizedId)) return false;
_activeProfileId = normalizedId;

DeleteProfile (lines 60-66) removes the profile, then clears _activeProfileId only if it already equals the deleted id.

The two methods can interleave like this:

  1. SetActiveProfile("x") passes the ContainsKey check.
  2. DeleteProfile("x") removes the profile. _activeProfileId is not "x" yet, so it is left alone.
  3. SetActiveProfile writes _activeProfileId = "x" and returns true.

The active id now names a profile that no longer exists. GetActiveProfile() returns null for now. Later, when a profile with the id "x" is created again, it becomes active even though nothing activated it.

Why it matters

The class is written to be thread-safe: it uses ConcurrentDictionary, has a _lock, and the CreateProfile race was fixed in #112. This is the one check-then-act path left. When it hits, the app runs with a stale active profile, and a recreated profile becomes active without anyone choosing it.

Repro (MSTest, reproduced)

int bad = 0;
for (int i = 0; i < 20000; i++) {
  ProfileManager pm = new(); pm.CreateProfile("x", "X");
  using Barrier b = new(2);
  Task t1 = Task.Run(() => { b.SignalAndWait(); pm.SetActiveProfile("x"); });
  Task t2 = Task.Run(() => { b.SignalAndWait(); pm.DeleteProfile("x"); });
  Task.WaitAll(t1, t2);
  pm.CreateProfile("x", "Recreated");
  if (pm.GetActiveProfile() is not null) bad++;
}
// Observed: bad == 27 of 20000. Expected: 0.

Suggested fix

  • Take _lock around the check-and-set in SetActiveProfile and around the remove-and-clear in DeleteProfile, so each pair runs as one step.
  • A defensive alternative is for GetActiveProfile/CreateProfile to discard an _activeProfileId that points at a missing profile. The lock is the simpler fix.

Acceptance: the stress test above reports 0, and it is added as a regression test.

Activity

  1. matt-edmondson commented on Sep 26, 2026

    @matt-edmondson
    ContributorAuthor

    Triage


    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