docs(server): document star count lost-update race in PatchStarCount (REL-04) - #2368
Open
wilfredmulenga wants to merge 2 commits into
Open
docs(server): document star count lost-update race in PatchStarCount (REL-04)#2368wilfredmulenga wants to merge 2 commits into
wilfredmulenga wants to merge 2 commits into
Conversation
Compliance scan issue #96 flagged REL-04: PatchStarCount reads starredBy/starCount, mutates them in memory, then writes the result back with no transaction and no atomic update. Two near simultaneous stars or unstars on the same project could let one write silently overwrite the other. Adding a comment instead of an atomic fix for now, since it is unlikely in practice for two different users to star the exact same project within the same narrow race window at the same time.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR documents a known lost-update race in PatchStarCount where starredBy/starCount are read, mutated in memory, and then written back without an atomic update across concurrent requests.
Changes:
- Added an inline NOTE comment warning about the read-modify-write lost-update risk at the
UpdateProjectMetadataByAnyUsercall site.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
wilfredmulenga
changed the title
wilfredmulenga
changed the title
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.
Summary
A compliance scan (issue #96) flagged REL-04: PatchStarCount in server/internal/adapter/internalapi/server.go reads starredBy and starCount, mutates them in memory, then writes the whole thing back. Nothing covers this span with a transaction, and there is no optimistic lock or atomic update such as $addToSet or $inc. If two users star or unstar the same project at nearly the same instant, the second write can overwrite the first, so one user's star can disappear while starCount stays out of sync.
Decision
Rather than switching to an atomic update right now, this change adds a comment at the write call documenting the race for anyone reading this code later. The reasoning is that it is unlikely in practice for two different users to star the exact same project within the same narrow window at the same time, so the risk is judged low relative to the cost of reworking this path.
This is a comment-only change. No behavior changes.
Test plan