Concurrent NavigateTo throws ArgumentException and corrupts history, despite README's "Thread-safe operations"

[matt-edmondson]

What's wrong

README.md:37 lists "✅ Thread-safe operations". However, Navigation/Services/NavigationStack.cs mutates _items (a List<T>) and _currentIndex with no synchronization:

• NavigateTo (≈43-70)
• GoBack/GoForward (≈73-100)
• Clear (≈103-110)
• RestoreState (≈171-180)

SimpleUndoRedoProvider (≈234-311) has the same problem with _undoStack/_redoStack.

Reproduction

var nav = new Navigation<NavigationItem>(new SimpleUndoRedoProvider());
Parallel.For(0, 2000, i => nav.NavigateTo(new NavigationItem("p" + i, "P" + i)));

This failed in 50 of 50 runs with AggregateException → ArgumentException: Destination array was not long enough.... The exception comes from the [.. _items] snapshot (NavigationStack.cs ≈51/65) racing a concurrent List<T> resize. Even in runs where nothing throws, interleaved RemoveRange/Add/_currentIndex updates can leave the index pointing outside the history, or leave history entries lost.

Why it matters

Consumers who rely on the documented guarantee can get crashes or silently wrong back/forward state. This is most likely when navigation is driven from multiple UI and background threads.

Suggested fix

Choose one:

• Make it true: add a private lock around every read and write of _items/_currentIndex, including snapshots, GetHistory and LoadStateAsync. Raise NavigationChanged outside the lock. Add the same protection to SimpleUndoRedoProvider.
• Drop the claim: document Navigation<T> as single-threaded (callers must synchronize) and remove the README bullet.

Acceptance criteria

The parallel repro above completes without throwing, and ends with History.Count == 2000 and a valid CurrentIndex. Alternatively, the README no longer claims thread safety.

[matt-edmondson]

Triage

• Category: Bug
• Priority: Medium. The README promises something the code doesn't deliver: concurrent NavigateTo crashed in 50 of 50 runs and can corrupt history silently. Most consumers drive navigation from a single UI thread, which keeps this from being High.
• Area / suggested assignee: NavigationStack and SimpleUndoRedoProvider state synchronization, and the README feature list. Maintainer: @matt-edmondson.
• Duplicates: None in this repo. Keybinding#109 has the same pattern: a "thread-safe" claim that concurrent calls contradict.
• In progress: No open PR. Note Delegate undo/redo to ktsu.UndoRedo #46 ("Delegate undo/redo to ktsu.UndoRedo"). If that lands, SimpleUndoRedoProvider may go away, so limit the locking work to NavigationStack until Delegate undo/redo to ktsu.UndoRedo #46 is decided.
• Related: LoadStateAsync with JsonFilePersistenceProvider always throws InvalidOperationException, so saved navigation can never be restored #53, Saving then loading restores the wrong position when the current page also appears earlier in history #54 and Undo after GoBack, Clear or LoadStateAsync drops the newest page and jumps to an unrelated state #55 all touch RestoreState/LoadStateAsync, which a lock would also need to cover.

Next steps: Decide between making the claim true and dropping it. If locking, use one private lock object in NavigationStack that covers every read and write of _items/_currentIndex, including the snapshots. Raise NavigationChanged outside the lock, and add the Parallel.For repro as a test.

---

Generated by Claude Code

[matt-edmondson]

Decision (maintainer, 2026-09-28)

• "Make it true": add locking. The README's "Thread-safe operations" claim stays, and the code has to honour it.
• Scope it to NavigationStack first. Use one private lock over every read and write of _items/_currentIndex, including snapshots, GetHistory, RestoreState and LoadStateAsync, and raise NavigationChanged outside the lock.
• Coordinate with Delegate undo/redo to ktsu.UndoRedo #46, which was decided the same day: adopt ktsu.UndoRedo behind an adapter. SimpleUndoRedoProvider is being replaced, so don't invest in locking it. Make sure the Delegate undo/redo to ktsu.UndoRedo #46 adapter preserves the thread-safety guarantee, by locking there or relying on UndoRedo's own guarantees, and fix any gaps upstream.

Next reader: add the lock to NavigationStack and turn the Parallel.For repro into a test (no throw, History.Count == 2000, valid CurrentIndex). Coordinate with #53/#54/#55 on RestoreState/LoadStateAsync.

---

Generated by Claude Code