Skip to content

fix(rdma): make endpoint teardown non-blocking#1001

Merged
Coldwings merged 4 commits into
mainfrom
fix/rdma-endpoint-async-shutdown
Jul 21, 2026
Merged

fix(rdma): make endpoint teardown non-blocking#1001
Coldwings merged 4 commits into
mainfrom
fix/rdma-endpoint-async-shutdown

Conversation

@Coldwings

Copy link
Copy Markdown
Owner

Description

Makes rdma_ibverbs::endpoint teardown non-blocking and explicit. Destruction now requests pump shutdown and fails closed instead of waiting on a worker thread; callers can use the new asynchronous shutdown() path to join the pump and release provider resources safely.

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Documentation (changes to documentation, comments, or examples)
  • Tests (adding or modifying tests)

Related Issues

Closes #975

Changes Made

Core Changes

  • Add an asynchronous, retry-safe endpoint shutdown path with explicit pump-exit publication and terminal-state guards.
  • Use checked QP destruction for endpoint-owned external completion queues and retain the full dependency graph on provider teardown failure.
  • Restrict abandon_outstanding() to eager operations that never installed a suspended coroutine waiter.
  • Update RDMA examples, API documentation, changelog, and focused source-contract tests.

API Changes

Before:

// Endpoint destruction could wait for pump termination.

After:

co_await endpoint.shutdown();
// For eager operations without a coroutine waiter only:
bool abandoned = endpoint.abandon_outstanding();

Existing call sites remain source-compatible. The new explicit shutdown API is recommended before destruction.

Testing

Unit Tests

  • Added focused source-contract tests
  • All tests pass locally

Test Results

Static review only. No local build or test run was performed; GitHub Actions is the executable validation for this PR.

Checklist

Code Quality

  • My code follows the project's code style
  • I have added/updated comments for complex concurrency and ownership logic
  • I have removed debug code and temporary notes

Documentation

  • I have updated documentation
  • I have updated examples
  • I have updated API documentation

Testing

  • I have added tests that exercise the intended contracts
  • New and existing unit tests pass locally with my changes
  • I have tested with ASAN and TSAN

Compatibility

  • My changes are backward compatible
  • I have considered the impact on existing users
  • I have updated CHANGELOG.md

Additional Notes

Two independent read-only reviews examined the exact submitted head from lifecycle/ownership and API-contract/provider-boundary perspectives. Their current-head findings will be posted separately.

Reviewer Guidance

Areas requiring special attention:

  • Exactly-once pump exit publication and asynchronous join behavior.
  • QP/CQ/CM ownership retention when provider teardown fails.
  • The eager-only boundary of abandon_outstanding().

Thank you for contributing to Elio! 🎉

Copilot AI review requested due to automatic review settings July 21, 2026 15:47

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Coldwings

Copy link
Copy Markdown
Owner Author

Independent exact-head review summary for 78f01eee5285cbf3e9e5e21f6492b7a9cd8a8372 (base 17b43506e67e83e9f1b59e0642aec9ece3bc402d):

  • Lifecycle and concurrency: PASS. Pump-exit publication, async join, retry serialization, terminal access guards, destructor fail-closed behavior, and constructor rollback form a consistent ownership state machine. The destructor does not wait or sleep.
  • Provider and API boundary: PASS. Direct checked ibv_destroy_qp() is confined to endpoint-owned QPs backed by externally owned CQs/channels. The implementation validates exact cm_id->qp identity, retains the complete dependency graph on failure, and clears the CM pointer only after successful destruction. General cm_id teardown remains unchanged and continues to use RDMA-CM bookkeeping.
  • Coroutine ownership: PASS. abandon_outstanding() is limited to eager operations that never installed a suspended coroutine waiter; suspended frames remain non-revocable.
  • Compatibility and documentation: PASS. Existing examples scope registered memory and awaiters before shutdown(), API/changelog wording matches the implementation, and unrelated base content is preserved.
  • git diff --check passed.

No actionable findings remain from either review angle. This was static review only; no local build or test execution was performed.

@Coldwings

Copy link
Copy Markdown
Owner Author

Updated independent exact-head review summary for b0ad34d60404e41fb6f5628ddd696f6970a62f13 (base 17b43506e67e83e9f1b59e0642aec9ece3bc402d):

  • CI failure root cause and test ownership: PASS after fix. Both Release jobs had failed because the old test manually retained a task while scheduler-less schedule_handle() resumed and destroyed its completed frame. The revised test transfers ownership through scheduler::go_joinable(), exercises the supported production scheduling path, and removes the double-destruction hazard.
  • Ordering and liveness: PASS. A single worker means the publisher cannot execute until the waiter has registered and suspended. Both join handles are destroyed before scheduler shutdown; captured state and atomics outlive both tasks. No lost wakeup, deadlock, hang, or closure-lifetime issue was found.
  • Cross-platform contract: PASS. The test uses C++20 atomics and scheduler APIs already supported by the arm64/x64 Debug/Release matrix. Pre-published exit and destroyed-waiter unregistration coverage remains intact.
  • Production lifecycle and provider ownership: PASS. The corrective commit changes only the unit test. The async shutdown state machine, checked endpoint-owned QP destruction, retain-on-failure behavior, general RDMA-CM teardown boundary, eager-only abandonment, examples, and documentation remain consistent.
  • git diff --check passed.

No actionable findings remain from either current-head review angle. Review was static only; the new GitHub Actions run is the executable validation.

@Coldwings

Copy link
Copy Markdown
Owner Author

Final independent exact-head review summary for dbd400ca0c577b38055f7b08e7eb53cb89a25037 (base 17b43506e67e83e9f1b59e0642aec9ece3bc402d):

  • Release SIGSEGV: fixed. The pump-exit wake test now transfers frame ownership to scheduler::go_joinable() and waits for both scheduler-owned frames to be destroyed before shutdown, avoiding scheduler-less trampoline/local-task double ownership.
  • ASAN stack-use-after-scope: fixed. The pre-published and abandoned-waiter sections now keep named coroutine factories alive until their task frames are resumed and destroyed; reverse declaration order is safe even during assertion unwinding.
  • Example timeout lifetime: fixed after review. rdma_perf now keeps its captured done flag alive through the 300-second timeout path, graceful shutdown, forced worker teardown, frame draining, and worker joins.
  • Exhaustive capture/lifetime pass: PASS. All changed tests and examples were rescanned; no immediately-invoked coroutine lambda or captured referent that can expire before task/frame teardown remains.
  • Production/provider/API contracts: PASS. Async endpoint shutdown, checked endpoint-owned QP destruction, retain/fail-closed paths, general RDMA-CM teardown, eager-only abandonment, examples, API docs, guide, and changelog remain coherent.
  • Worktree and git diff --check are clean.

No actionable findings remain from either final current-head review angle. Reviews were static-only; GitHub Actions on this head is required before merge.

@Coldwings

Copy link
Copy Markdown
Owner Author

CI diagnosis for final-head run 29848628438: x64-Debug built successfully and the normal unit suite passed. Its ASAN-only rerun failed at the unchanged task_group publishes completion after child state release timing assertion (tests/unit/test_task_group.cpp:1193, join_observed_pending == false).

This branch does not modify task_group or that test. The same timing-sensitive ASAN assertion failed once and passed on rerun in PR #1000. On this head, both Release jobs passed, arm64 normal tests and ASAN passed, and all RDMA endpoint lifetime tests passed. I will rerun the failed x64-Debug job after arm64 TSAN completes, and require the rerun to pass before merge.

@Coldwings
Coldwings force-pushed the fix/rdma-endpoint-async-shutdown branch from dbd400c to a753a82 Compare July 21, 2026 17:11
@Coldwings

Copy link
Copy Markdown
Owner Author

Final rebased exact-head review summary for a753a823347a9297c594c2a834ec339b8847ea40 (base ed886f201ce21af6a38284e0d222db2dc917cbf4):

No actionable findings remain from either independent current-head review angle. Review was static-only; GitHub Actions on the rebased head is required before merge.

@Coldwings
Coldwings merged commit 74b0035 into main Jul 21, 2026
16 of 17 checks 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.

[Bug] rdma: endpoint destructor blocks worker thread with sleep_for up to 1s

2 participants