Skip to content

[Bug] Fix possible race condition in HCCL handshake#266

Merged
matthewygf merged 5 commits into
LMCache:mainfrom
marcobarlo:fix_263_issue
Jul 24, 2026
Merged

[Bug] Fix possible race condition in HCCL handshake#266
matthewygf merged 5 commits into
LMCache:mainfrom
marcobarlo:fix_263_issue

Conversation

@marcobarlo

@marcobarlo marcobarlo commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR fixes issue #263.

  • Fix a race in Ascend HCCL PD peer handshake: HcclMemRegRequest can run
    before the background accept() thread publishes conn_handles_dict[peer_id].
  • Decoder then returns HcclErrorResponse; prefiller crashes with
    AttributeError: 'HcclErrorResponse' object has no attribute 'server_mem_handle_bytes'. PD transfer fails; proxy hangs on KV-ready.
  • Scope: HcclChannel._handle_init_msg MemReg path only
    (lmcache_ascend/v1/transfer_channel/hccl_channel.py).

Why

  • On HcclInitRequest, the server starts complete_handshake() (background
    accept()), waits only for accept_started_event, then replies Init.
  • Client may finish connect() and send MemReg before T3 writes the handle.
  • HcclReadyRequest already waits on _peer_ready_events; MemReg did not.
  • Fix: wait on the same ready_event (timeout 120s) before looking up
    conn_handles_dict, and raise clearly if the handle is still missing.

Test plan

  • Apply only this change.
  • Launch Ascend PD disagg (HCCL, prefiller + decoder).
  • Send a request through the disagg proxy.
  • Inject delay right after accept().
  • Decoder no longer logs MemReg KeyError / init-loop failure before
    “Connection established”.
  • Prefiller no longer raises on server_mem_handle_bytes; KV transfer
    completes and proxy returns successfully.

Notes

  • Threads: T1 lazy_init_peer_connection, T2 _init_loop
    _handle_init_msg, T3 nested complete_handshake.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request addresses a race condition in the HCCL handshake timeout (Github issue #263) by waiting for the handshake ready event before retrieving the connection handle. The feedback suggests handling the case where the handshake event is None explicitly and raising a more idiomatic ConnectionError instead of a KeyError when the connection handle is missing.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread lmcache_ascend/v1/transfer_channel/hccl_channel.py
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Signed-off-by: Marco Barletta <65128997+marcobarlo@users.noreply.github.com>

@matthewygf matthewygf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe 120s is a bit too long tho, wdyt @chloroethylene

@matthewygf matthewygf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a cmt.

Comment thread lmcache_ascend/v1/transfer_channel/hccl_channel.py Outdated
Comment thread tests/v1/transfer_channel/test_hccl_channel_write.py Outdated

@matthewygf matthewygf left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, and tested

@matthewygf
matthewygf merged commit 1b6e3a4 into LMCache:main Jul 24, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants