[GH-ISSUE #3143] [P2P-Store] Same-region concurrent Add/Remove races in RegisteredMemory #809

New issue

Open

opened 2026-10-29 21:19:46 +00:00 by bsakaguchi · 3 comments

bsakaguchi commented

2026-10-29 21:19:46 +00:00

Owner

Originally created by @jfeng18 on GitHub (Jul 27, 2026).
Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/3143

Originally assigned to: @Icedcoco on GitHub.

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.

Originally created by @jfeng18 on GitHub (Jul 27, 2026). Original GitHub issue: https://github.com/kvcache-ai/Mooncake/issues/3143 Originally assigned to: @Icedcoco on GitHub. ## 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.

bsakaguchi added the

stale

label

2026-10-29 21:19:46 +00:00

bsakaguchi commented

2026-10-29 21:19:47 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Jul 27, 2026):

Thanks for opening this issue, @jfeng18!

Field	Value
Issue	#3143
GitHub user ID	`288662032`
Reporter	@jfeng18

A maintainer will triage this when possible. To help us respond faster, please include:

Mooncake version or commit SHA
Environment (OS, CUDA/driver, RDMA stack if relevant)
Steps to reproduce and expected vs. actual behavior

Useful links: Documentation · Contributing guide

This message was posted automatically by the issue bot.

@github-actions[bot] commented on GitHub (Jul 27, 2026): Thanks for opening this issue, @jfeng18! | Field | Value | |-------|-------| | **Issue** | #3143 | | **GitHub user ID** | `288662032` | | **Reporter** | @jfeng18 | A maintainer will triage this when possible. To help us respond faster, please include: - Mooncake version or commit SHA - Environment (OS, CUDA/driver, RDMA stack if relevant) - Steps to reproduce and expected vs. actual behavior Useful links: [Documentation](https://kvcache-ai.github.io/Mooncake/) · [Contributing guide](https://github.com/kvcache-ai/Mooncake/blob/main/CONTRIBUTING.md) > This message was posted automatically by the issue bot.

bsakaguchi commented

2026-10-29 21:19:47 +00:00

Author

Owner

@jfeng18 commented on GitHub (Jul 28, 2026):

Now that #2375 has landed — @ykwd could you weigh in on the design question here? If same-region concurrent Add/Remove should be supported, I'll implement per-entry state tracking; if it's outside the supported usage, a doc comment stating the contract would suffice. Happy to send the PR either way.

@jfeng18 commented on GitHub (Jul 28, 2026): Now that #2375 has landed — @ykwd could you weigh in on the design question here? If same-region concurrent Add/Remove should be supported, I'll implement per-entry state tracking; if it's outside the supported usage, a doc comment stating the contract would suffice. Happy to send the PR either way.

bsakaguchi commented

2026-10-29 21:19:47 +00:00

Author

Owner

@github-actions[bot] commented on GitHub (Oct 27, 2026):

This issue has had no activity for 90 days and will be closed in 7 days if there is no further activity. Please comment or react if it should stay open.

@github-actions[bot] commented on GitHub (Oct 27, 2026): This issue has had no activity for 90 days and will be closed in 7 days if there is no further activity. Please comment or react if it should stay open.