Fix lost-update race in channel subscriptions - #691
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSubscription creation and removal now use atomic read-modify-write operations. A concurrent test uses an in-memory compare-and-set KV implementation to detect lost subscriptions. ChangesSubscription update flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant AddSubscription
participant modifySubscriptions
participant KVStore
Test->>AddSubscription: issue 200 concurrent additions
AddSubscription->>modifySubscriptions: mutate subscription state
modifySubscriptions->>KVStore: read and compare-and-set state
KVStore-->>modifySubscriptions: current state or retry
modifySubscriptions-->>AddSubscription: report result
Test->>KVStore: retrieve persisted subscriptions
KVStore-->>Test: return stored subscriptions
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
db632e4 to
b1b6f9f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@server/subscriptions.go`:
- Around line 195-242: Update Unsubscribe’s result handling after
modifySubscriptions to check err before removed, returning the store error
immediately; treat errStopModify as the expected no-subscription case, while
preserving the existing removed/current return behavior for successful
modifications.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a00066a3-10a8-4694-8c45-b2a6aa6e7cb4
📒 Files selected for processing (2)
server/subscription_race_test.goserver/subscriptions.go
StoreSubscriptions wrote the whole subscriptions blob with a plain p.client.KV.Set, with no compare-and-set, and swallowed any write error (it logged a warning and returned nil, so a failed write 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 an atomic SetAtomicWithRetries callback via a new modifySubscriptions helper. Each retry re-reads the fresh blob and re-applies the mutation, so no update is lost, and the real store error is now returned to the caller instead of being swallowed. AddSubscription and Unsubscribe now express their change as a mutate closure. Add TestSubscriptionRace, which fires 200 concurrent AddSubscription calls at an in-memory KV store with real compare-and-set semantics and asserts persisted == reported-success. It fails on the old code and passes with this fix.
b1b6f9f to
4550822
Compare
|
|
||
| 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.
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")
}
There was a problem hiding this comment.
Thanks, addressed. Added the same guard after the silentlyLost check: the test now fails with "no AddSubscription calls succeeded" if reportedSuccess == 0, so it can't pass without actually exercising the race. Pushed in 8ab8a0e.
2 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. Both callers were told the operation succeeded, but only
one of the two changes actually persisted. The other was silently lost.
Root cause
StoreSubscriptionsinserver/subscriptions.gowrote the whole blob with aplain
Setand no compare-and-set, and it also swallowed any write error:AddSubscription(subscriptions.go:65) andUnsubscribe(
subscriptions.go:159) both didGetSubscriptions()to read the whole blob,mutated the in-memory copy, then handed that copy to
StoreSubscriptions.Because the write used a plain
Set, a concurrent writer that had read the samestarting blob would overwrite the first writer's change with its own copy. The
last write won and every earlier change was dropped. Since the error was
swallowed, a genuinely failed write also reported success.
The fix
Switch to the atomic
SetAtomicWithRetriespattern exposed by the pluginapi KVservice, and move the read-modify-write inside its 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. The real store error is now returned to the caller instead
of being swallowed.
AddSubscriptionandUnsubscribenow express their change as a mutate closureand keep their existing return signatures (
Unsubscribestill reports whether asubscription was removed and returns the resulting subscriptions).
GetSubscriptionsandGetSubscriptionsByChannelare unchanged.Regression test
TestSubscriptionRace(server/subscription_race_test.go) fires 200 concurrentAddSubscriptioncalls against an in-memory KV store whoseKVGetandKVSetWithOptionsimplement real compare-and-set semantics, matching the serverKV store. It then 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
All 200 calls reported success because the old code swallowed write errors, yet
only 10 subscriptions persisted.
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: 🟠 Medium
Reasoning: The change modifies subscription persistence and concurrency handling. The scope is isolated, but it affects data integrity and removes the public
StoreSubscriptionsmethod.Regression Risk: Medium. The atomic write flow changes
AddSubscriptionandUnsubscribe, including error handling and removal behavior. A concurrency regression test covers concurrent additions, but other mutation paths remain less covered.** QA Recommendation:** Perform focused manual QA for concurrent updates, replacements, unsubscriptions, store errors, and consumers of
StoreSubscriptions. Skipping manual QA carries moderate risk because the change affects persisted subscription data.Generated by CodeRabbitAI