fix(pd): guard remote_agents dict with a lock to fix NIXL concurrent-add race (GH-1470) - #1505
Open
Ashfaqbs wants to merge 1 commit into
Open
fix(pd): guard remote_agents dict with a lock to fix NIXL concurrent-add race (GH-1470)#1505Ashfaqbs wants to merge 1 commit into
Ashfaqbs wants to merge 1 commit into
Conversation
…add race connect_add_remote_agent() did an unlocked check-then-add on self.remote_agents, and remove_remote_agent() did an unlocked pop. Both are called from several worker threads on the same NixlKVTransporter instance (recv/dispatch/accept_peer/request-page/ ready-page loops all run as separate threads sharing one transporter). When a peer briefly looks missing (e.g. after a transient NIXL_ERR_REMOTE_DISCONNECT removes it), two threads can both see it absent and both call nixl_agent.add_remote_agent() for the same peer concurrently, which can corrupt UCX endpoint state and abort the NIXL progress thread (observed as a ud_ep.c PSN assertion), taking down the router. Guard the whole check-then-add and check-then-remove sequences with a lock so only one thread can add or remove a given peer at a time. Fixes ModelTC#1470
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
NixlKVTransporter.remote_agentsis a plain dict that several worker threads read and mutate concurrently on the same instance (recv_task_loop,dispatch_task_loop,accept_peer_task_loop,request_page_loop,read_page_to_mems_loop,success_loop,fail_loopare all started as separate daemon threads sharing onetransporter).connect_add_remote_agent()did an unlocked check-then-add:and
remove_remote_agent()did an unlocked check-then-pop. Everysend_*method also does an unlockedif peer_name not in self.remote_agents: connect_add_remote_agent(...).When a peer transiently disappears (e.g. after
NIXL_ERR_REMOTE_DISCONNECTremoves it), two threads can both observe it missing and both callnixl_agent.add_remote_agent()for the same peer at the same time. That double-add corrupts UCX endpoint state and aborts the NIXL progress thread with aud_ep.cPSN assertion, which kills the KV-transfer process and takes down the whole PD router (see #1470 for the full failure timeline and stack trace).Fix
Add a
threading.LocktoNixlKVTransporterand guard the full check-then-add sequence inconnect_add_remote_agent()and the check-then-pop inremove_remote_agent()with it, so only one thread can add or remove a given peer at a time. The rest of the call sites' pre-checks (if X not in self.remote_agents) stay as cheap fast-path hints — correctness now comes from the lock insideconnect_add_remote_agent, which re-checks membership once it holds the lock.Fixes #1470
Scope
Kept to the one file / one race described in the issue (
nixl_kv_transporter.py). Did not touch the wider unlocked reads inwrite_blocks_pagedetc. since those aren't the failure mode reported in #1470 and would be a separate, larger change.Verification
python -m py_compileon the changed file.