fix: serialize UserStore.UpdateAsync under the exclusive stream lock (#70)#74
Merged
Merged
Conversation
…70) UpdateAsync read current state from an unlocked query and then appended a full-state UserUpdated in a separate session. That leaves a window in which a concurrent lockout increment can land between the read and the append and be silently overwritten by the stale counter — the #22 accumulation race re-entering through the generic update path. Take the exclusive stream lock (AppendExclusive) before reading current state and do the whole read-modify-write in one session, matching the pattern already used by PersistLockoutStateAsync. The lockout counter and root-deletable invariant are now preserved from state read under the lock, and concurrent updates serialize instead of racing. Adds an integration test interleaving profile updates with failed-login increments and asserting no increment is lost. Note: this hardens the lockout race on the generic path and serializes writers; it does not add field-level optimistic concurrency (rejecting a stale write that reverts an unrelated concurrent change, e.g. a 2FA toggle). That is a larger, separate change and remains tracked as follow-up.
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
1 participant
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.
Addresses #70 (safe-hardening scope; see note below).
Problem
UserStore.UpdateAsyncread the current user from an unlocked query and then appended a full-stateUserUpdatedin a separate session. Between that read and the append, a concurrent lockout increment (from a failed login on another request) could commit — and then get silently overwritten by the staleAccessFailedCountthis method carries forward. That is the #22 failed-login accumulation race re-entering through the generic update path.Fix
Take the exclusive stream lock before reading current state and perform the whole read-modify-write in one session — exactly the pattern already used by
PersistLockoutStateAsync:session.Events.AppendExclusive(streamId)first,LoadAsync<User>),LockoutEndand the root-Deletableinvariant from that in-lock read,Concurrent
UpdateAsynccalls now serialize on the same stream lock as the atomic lockout methods instead of racing.Tests
UpdateAsync_ConcurrentWithLockoutIncrements_LosesNoIncrementsinterleaves 15 failed-login increments with 15 concurrent profile updates and asserts every increment accumulated.UpdateAsync_DoesNotRegressConcurrentlyChangedLockoutState,UpdateAsync_RootUser_StaysNonDeletable,UpdateAsync_PersistsChangedSecurityStamp, etc.Scope note (agreed)
This hardens the lockout race on the generic path and serializes writers. It intentionally does not add field-level optimistic concurrency — i.e. rejecting a stale full-state write that reverts an unrelated concurrent change (such as a 2FA toggle overwritten by a stale profile save). Doing that correctly requires base-version/3-way tracking that conflicts with the deliberate "merge lockout forward" behavior and with ASP.NET Identity flows that call
UpdateAsynctwice on one object; it's a larger change left as follow-up.