fix(server): Reconcile bucket set#7260
Conversation
|
augment review |
🤖 Augment PR SummarySummary: Updates 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| if (!change_cb_.empty()) { | ||
| auto bucket_set = db.prime.CVCUponInsert(key); | ||
| CallChangeCallbacks(cntx.db_index, bucket_set); | ||
| for (bool consistent = false; !consistent;) { |
There was a problem hiding this comment.
src/server/db_slice.cc:688 — This retry loop has no bound, so if CVCUponInsert(key) keeps changing due to concurrent activity while callbacks block/yield, AddOrFindInternal could potentially livelock and never proceed to the actual insert. Consider adding some guard/guarantee (or documenting the convergence invariant) to ensure termination.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Eventually all possible buckets will be visited, which implies no suspension will occur and the operation will finish
There was a problem hiding this comment.
I'll add that as a comment
There was a problem hiding this comment.
Pull request overview
This PR addresses a fiber-preemption race in DbSlice::AddOrFindInternal where change callbacks (used by snapshotting/migration flows) can yield, allowing concurrent inserts to grow/split the underlying DashTable and invalidate the originally computed insertion bucket set.
Changes:
- Replaces a post-callback
DCHECKonCVCUponInsert()stability with a retry loop that re-runs change callbacks until the computed bucket set remains consistent across the callback. - Adds clarifying comments around why callbacks are invoked on the computed insertion bucket set.
| for (bool consistent = false; !consistent;) { | ||
| auto bucket_set = db.prime.CVCUponInsert(key); | ||
| CallChangeCallbacks(cntx.db_index, bucket_set); | ||
|
|
||
| // Set of possible insertion buckets must be the same after possibly blocking call | ||
| DCHECK(bucket_set == db.prime.CVCUponInsert(key)); | ||
| // Repeat the change callbacks if the target set changed | ||
| consistent = (bucket_set == db.prime.CVCUponInsert(key)); | ||
| } |
Fixes #7245
Repeat on change callbacks if the insertion bucket set changed while calling them