Fix lost-update race in channel subscriptions - #1045
Conversation
|
Hello @aegonmyy, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project. Please help complete the Mattermost contribution license agreement? This is a standard procedure for many open source projects. Please let us know if you have any questions. We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughSubscription creation and removal now use a shared atomic read-modify-write helper. The change removes ChangesSubscription update consistency
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
b67cf15 to
80f94ca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/plugin/subscription_race_test.go (1)
14-30: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueRemove or tighten the "byte-identical" claim.
pluginapi.MemoryStoreexists for the pinnedgithub.com/mattermost/mattermost/server/public v0.3.0, but the comment should not say it is byte-identical unless the stored source is inspected. Use the generic CAS retry wording already present after the test logic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/plugin/subscription_race_test.go` around lines 14 - 30, Update the comments above TestSubscriptionRace to remove the unverified “byte-identical” claim about pluginapi.MemoryStore and describe only the generic CAS retry behavior relevant to the test. Keep the explanation that the test exercises real AddSubscription logic without mocks and verifies persisted subscriptions match reported successes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/plugin/subscription_race_test.go`:
- Around line 14-30: Update the comments above TestSubscriptionRace to remove
the unverified “byte-identical” claim about pluginapi.MemoryStore and describe
only the generic CAS retry behavior relevant to the test. Keep the explanation
that the test exercises real AddSubscription logic without mocks and verifies
persisted subscriptions match reported successes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dc610e8e-4bdb-4b30-a25d-886ce0e01d38
📒 Files selected for processing (3)
server/plugin/command_test.goserver/plugin/subscription_race_test.goserver/plugin/subscriptions.go
StoreSubscriptions passed a snapshot captured before the retry loop into SetAtomicWithRetries and ignored the callback's oldValue argument, so a conflicting concurrent write was overwritten with stale data while the compare-and-set still reported success. AddSubscription and Unsubscribe both read the whole blob, mutated it in memory, then called StoreSubscriptions, so concurrent subscribe/unsubscribe across channels silently clobbered each other. Move the read-modify-write inside the atomic callback via a new modifySubscriptions helper. Each retry re-reads the fresh blob and re-applies the mutation, so no update is lost. AddSubscription and Unsubscribe now express their change as a mutate closure. Add TestSubscriptionRace, which fires 200 concurrent AddSubscription calls at the real pluginapi.MemoryStore and asserts persisted == reported-success. It fails on the old code and passes with this fix.
80f94ca to
c3b8ae8
Compare
|
/check-cla |
|
|
||
| if silentlyLost > 0 { | ||
| t.Fatalf("lost-update bug: %d subscriptions were reported as saved but silently dropped", silentlyLost) | ||
| } |
There was a problem hiding this comment.
(Similar to mattermost/mattermost-plugin-gitlab#691.)
If all AddSubscription calls fail, subs will be empty and persisted will be 0 and so silentlyLost will be 0 and no failures will be reported. Suggest adding the following after silentlyLost check:
if reportedSuccess == 0 {
t.Fatalf("No AddSubscription calls succeeded")
}
3 participants
Fix lost-update race in channel subscriptions
Summary
All channel subscriptions are stored in a single KV key (
subscriptions).Every mutation reads the whole blob, changes it in memory, and writes the whole
blob back. That read-modify-write was not atomic, so two subscribe or
unsubscribe operations running at the same time in different channels could
overwrite each other. The plugin reported success to both callers, but only one
of the two changes actually persisted. The other was silently lost.
Root cause
StoreSubscriptionsinserver/plugin/subscriptions.gocalledSetAtomicWithRetries, but its callback ignored theoldValueargument andreturned a snapshot of the subscriptions that was captured before the retry
loop began:
AddSubscription(subscriptions.go:306) andUnsubscribe(
subscriptions.go:422) both didGetSubscriptions()to read the whole blob,mutated the in-memory copy, then handed that stale copy to
StoreSubscriptions.SetAtomicWithRetriesis supposed to re-run its callback on each retry so thechange is re-applied on top of the freshly read value. Because the callback
ignored
oldValueand always marshaled the same pre-read snapshot, a retryafter a conflicting concurrent write just rewrote stale data. The compare-and-set
still succeeded, so the concurrent writer's change was dropped without any error.
The fix
Move the read-modify-write inside the atomic callback. A new
modifySubscriptionshelper decodesoldValueon every attempt, applies amutate closure to that fresh state, and returns the re-encoded result. On a
conflicting write the retry re-reads the latest blob and re-applies the change,
so nothing is lost.
AddSubscriptionandUnsubscribenow express their change as a mutate closurepassed to the helper.
GetSubscriptionsandGetSubscriptionsByChannelareunchanged. Error types and return values are preserved.
Regression test
TestSubscriptionRace(server/plugin/subscription_race_test.go) fires 200concurrent
AddSubscriptioncalls against the vendoredpluginapi.MemoryStore,whose
SetAtomicWithRetriesis byte-identical to the production KV service. Itthen compares how many calls reported success against how many subscriptions
actually persisted. It fails on the old code and passes with this fix.
Before the fix
After the fix
After the fix, every call that reports success is durable. Under heavy
contention some calls exhaust the five internal retries and return an error
instead. Those are reported as failures and are not counted as success, which is
the correct behavior.
Change Impact: 🔴 High
Reasoning: The change modifies subscription persistence and data integrity logic. It also removes the public
StoreSubscriptionsmethod.Regression Risk: Concurrent subscription updates and unsubscribe behavior changed. The main race path has regression coverage, but other mutation and storage-error paths remain possible regression areas.
** QA Recommendation:** Perform focused manual QA for concurrent add, unsubscribe, duplicate, not-found, and storage-error cases. Skipping manual QA carries moderate risk because the change affects shared persistence behavior.
Generated by CodeRabbitAI