Skip to content

Fix Ethernet receiver teardown race - #876

Open
Changho Hwang (chhwang) wants to merge 10 commits into
mainfrom
fix/ethernet-receiver-close-race
Open

Fix Ethernet receiver teardown race#876
Changho Hwang (chhwang) wants to merge 10 commits into
mainfrom
fix/ethernet-receiver-close-race

Conversation

@chhwang

@chhwang Changho Hwang (chhwang) commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Fix the close-vs-blocked-recv race where closing the receive socket before its worker exited could expose recv to EBADF or a reused file descriptor.
  • Publish the stop state, shut down both sockets, join the receiver, and only then close both descriptors.
  • Preserve exact-read framing, distinguish clean EOF from truncated frames and local shutdown, and publish genuine receiver failures to connection operations.
  • Add focused host-only coverage for blocked receiver wakeup, bounded shutdown, repeated teardown, exact reads, EOF handling, and descriptor reservation/reuse.

Validation

  • A100: full unit suite 40 passed / 6 skipped; 2,500 socket teardown cycles; 50x 8-rank Ethernet pingpong.
  • MI300X: full unit suite 29 passed / 9 skipped; 2,500 socket teardown cycles; 3x 8-rank non-IB aggregate runs; 10x Ethernet sequences.
  • No EBADF failures or hangs observed.

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

@chhwang

Copy link
Copy Markdown
Contributor Author

/azp run mscclpp-ut

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

Copilot AI 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.

Pull request overview

Fixes Ethernet receiver teardown races by separating socket shutdown from descriptor closure.

Changes:

  • Adds exact-read outcomes and non-closing socket shutdown.
  • Publishes receiver failures and safely joins before closing descriptors.
  • Adds teardown/EOF tests and migrates socket logging.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/core/bootstrap/socket.cc Implements shutdown and exact-read handling.
src/core/connection.cc Reorders teardown and propagates receiver errors.
src/core/include/connection.hpp Adds receiver lifecycle/error state.
src/core/include/socket.h Declares receive results and shutdown API.
test/unit/socket_tests.cc Adds socket framing and teardown tests.
Suppressed comments (1)

test/unit/socket_tests.cc:153

  • descriptorReused is effectively guaranteed whenever /dev/null opens successfully: dup2 succeeds even when receiverFd is still open because it silently closes the target first. As a result, this assertion cannot verify that close() released the receiver descriptor; check for EBADF directly instead.
    int reuseFd = ::open("/dev/null", O_RDONLY);
    const bool descriptorReused = reuseFd == receiverFd || (reuseFd >= 0 && ::dup2(reuseFd, receiverFd) == receiverFd);
    if (reuseFd >= 0 && reuseFd != receiverFd) ::close(reuseFd);
    if (descriptorReused) ::close(receiverFd);

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread test/unit/socket_tests.cc Outdated
Comment thread src/core/bootstrap/socket.cc Outdated
@chhwang
Changho Hwang (chhwang) requested a review from a team August 20, 2026 06:26
Comment thread src/core/connection.cc Outdated
@chhwang

Copy link
Copy Markdown
Contributor Author

/azp run mscclpp-ut

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).

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.

3 participants