feat: optimistic concurrency for UserStore.UpdateAsync (#70)#77
Merged
Conversation
The generic update path appended a full-state UserUpdated from the caller's snapshot. If a concurrent change committed after the caller loaded, that write silently reverted it (e.g. a 2FA enable overwritten by a stale profile save) — last-writer-wins. The earlier fix serialized the read-modify-write under the stream lock but still couldn't detect a stale snapshot. Add a content-version optimistic-concurrency token. A new User.ContentVersion is advanced by the projection on every content-changing event, but NOT by the auto-managed lockout increments (UserUpdated gains a LockoutOnly flag the lockout path sets). UpdateAsync now, under the exclusive stream lock, rejects a write whose content version no longer matches the committed one (IdentityResult "ConcurrencyFailure") instead of clobbering; lockout counters are still merged forward so concurrent failed-login counting never causes a spurious conflict. After a successful write the in-memory version is refreshed so ASP.NET Identity flows that call UpdateAsync twice on one instance (e.g. ResetAuthenticator) keep working. Existing docs (pre-ContentVersion) deserialize to 0 and self-heal on the next update; the exclusive lock still serializes concurrent writers across that transition. Adds tests: stale snapshot rejected, 2FA not clobbered, sequential same-instance updates succeed, concurrent lockout increment is not a conflict.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #70 (the deferred field-level concurrency follow-up).
Problem
UpdateAsyncappends a full-stateUserUpdatedfrom the caller's snapshot. If a change committed after the caller loaded, this write silently reverts it — e.g. a user enables 2FA while a profile edit (loaded a moment earlier) is in flight; the profile save writesTwoFactorEnabled = falseand 2FA is quietly turned back off. The earlier PR serialized the read-modify-write under the exclusive stream lock, but that alone can't tell a stale snapshot from an intended change.Why not just use the stream version
A raw Marten stream version advances on every event, including the auto-managed lockout increments. Using it as the concurrency token would reject a profile update that merely raced a failed-login count — regressing the deliberate "merge lockout forward" behavior (and its test
UpdateAsync_DoesNotRegressConcurrentlyChangedLockoutState).Approach — a content-version token
User.ContentVersion(new) is advanced by the projection on every content-changing event (UserUpdatednot marked lockout-only, andUserRestored).UserUpdatedgains aLockoutOnlyflag thatPersistLockoutStateAsyncsets, so lockout increments do not bump the content version.UpdateAsync, under the exclusive lock, compares the caller'sContentVersionto the committed one; if it advanced, the snapshot is stale → returnIdentityResult.Failedwith codeConcurrencyFailureinstead of clobbering. Lockout counters are still merged forward, so concurrent failed-login counting never causes a spurious conflict.ContentVersionis refreshed, so ASP.NET Identity flows that callUpdateAsynctwice on the same instance (e.g.ResetAuthenticator:SetTwoFactorEnabledAsyncthenResetAuthenticatorKeyAsync) keep working.Compatibility
Existing projected docs (pre-
ContentVersion) deserialize to0and self-heal on the next update; because both writers still go throughAppendExclusive, concurrent updates are serialized and protected even across that0 → 1transition. A projection rebuild recomputes the token consistently.Tests
New cases in
UserStoreTests:ConcurrencyFailure, first writer's change survives;Existing coverage (lockout merge, security-stamp rotation, root invariants, the #70 no-lost-increments test) stays green. Full suite: 187 unit + 68 Blazor + 75 integration; build 0 warnings; CSharpier clean.
THREAT-MODEL.mdupdated.