kvcache-ai / kvcache-ai/Mooncake
[P2P-Store] Same-region concurrent Add/Remove races in RegisteredMemory
- Dominant language
- C++
- Stars
- 6.6k
- Forks
- 1.2k
- Avg merge
- 3d 5h
- Merged PRs (30d)
- 312
Description
## Context
Follow-up from #2375 review discussion with @Icedcoco. Two same-region concurrency windows exist in `RegisteredMemory` (`mooncake-p2p-store/src/p2pstore/registered_memory.go`) that predate #2375:
## Race 1: concurrent Add() during registration
`Add()` inserts the `bufferList` entry (refCount=1) and releases the lock **before** the underlying `registerLocalMemory` chunks complete. A concurrent `Add()` for the same region hits the entry, increments refCount and returns success — but the memory may not be registered yet, and if the original registration fails, the rollback (added in #2375) removes the entry while the second caller still believes it holds a reference.
## Race 2: Add() overlapping a final Remove()
The last `Remove()` deletes the entry and releases the lock **before** the underlying `unregisterLocalMemory` calls finish. A new `Add()` for the same region can start registering while unregistration of the same addresses is still in flight.
## Open design question
Are same-region concurrent Add/Remove operations intended to be supported?
- If **yes**: entries need a per-entry state (e.g. registering/ready/removing) with waiters, or per-region synchronization held across the underlying register/unregister — a change to the synchronization semantics for all callers.
- If **no**: a doc comment on `RegisteredMemory` stating the contract (external serialization required for same-region calls) would make the current behavior well-defined.
#2375 narrows the damage (no permanent stale entry on failed Add, no premature unregister on refCount>0) but intentionally does not change the visibility windows.
Happy to take this after #2375 lands, once maintainers weigh in on the intended contract.
Contributor guide
Assessment
This issue has not been assessed yet.