Skip to content

Profiles deleted and commands unregistered through IKeybindingManagerFactory.CreateManager() come back after SaveAsync + restart #202

Open

Description

@matt-edmondson

What's wrong

Since #188, KeybindingManagerFactory.CreateManager() returns a new KeybindingManager built over the shared DI ICommandRegistry, IProfileManager and IKeybindingRepository (Keybinding/Services/KeybindingManagerFactory.cs:33-44). The IKeybindingManagerFactory docs say it shares their state.

The persistence bookkeeping that #106, #124 and #150 introduced is kept per manager instance, though. _persistedProfileIds, _persistedCommandIds and _activeProfileKnown live in Keybinding/KeybindingManager.cs:17-28, and only that instance's own InitializeAsync/SaveAsync fills them. SaveAsync uses them like this:

  • Commands (KeybindingManager.cs:~146): it keeps every stored command that is not in _persistedCommandIds. A fresh factory manager's set is empty, so a command unregistered through it is written back to commands.json.
  • Profiles (KeybindingManager.cs:~166): it deletes only stored profiles that are in _persistedProfileIds. For a fresh factory manager that set is empty, so nothing is deleted from storage.

Failure scenario

  1. services.AddKeybinding(dir). Resolve the registered KeybindingManager and call InitializeAsync(). This loads profiles default and vim, and command old.cmd.
  2. using var m = factory.CreateManager();
  3. m.Profiles.DeleteProfile("vim"); m.Commands.UnregisterCommand("old.cmd"); await m.SaveAsync();
  4. Restart (create a fresh manager over dir and call InitializeAsync()). vim and old.cmd are both back.

I reproduced this with an MSTest case against HEAD 6d9183d, and both asserts fail.

Calling m.InitializeAsync() before editing is not a safe workaround:

  • It reloads from disk into the shared registry and profile manager. That resurrects profiles another holder deleted in memory but has not saved yet (KeybindingManager.cs:103-107).
  • It resets the shared active profile to the stored one, which overwrites an unsaved choice (:115-119).

Suggested fix

Keep the persisted-id state with the shared services rather than per facade instance. For example, hold it in a small singleton tracker registered alongside IKeybindingRepository, or inside the repository itself. Managers built over the same registry, profile manager and repository then agree on what is already on disk.

Acceptance criteria

Related, but not the same: #195, a race between two managers saving at once. This bug needs no concurrency.

Activity

  1. matt-edmondson commented on Oct 29, 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

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions