Skip to content

[Bugfix][TE] Drain timed-out RDMA tasks before publishing failure - #5034

Draft
alogfans wants to merge 1 commit into
kvcache-ai:mainfrom
alogfans:fix/classic-rdma-timeout-drain
Draft

alogfans wants to merge 1 commit into
kvcache-ai:mainfrom
alogfans:fix/classic-rdma-timeout-drain

Conversation

@alogfans

@alogfans alogfans commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Description

With MC_SLICE_TIMEOUT enabled, classic RDMA currently reports TIMEOUT from a status query while the timed-out WR can still access the caller's buffers. Reusing those buffers after that result can allow a delayed READ or WRITE to overwrite new data.

Move timeout detection to the posting/CQ owner. Mark the task canceled, reject queued and retried siblings, retire its affected endpoints through the existing QP-to-ERR path, and consume actual completion/flush CQEs before publishing task failure. The task stays WAITING while draining and becomes FAILED after every slice is terminal; a late success cannot undo timeout cancellation. Retain endpoint references and accounting until CQ processing finishes. The registry is enabled only with MC_SLICE_TIMEOUT, independently of diagnostic tracking.

This is an independent change based on fe4f297a. If QP-to-ERR fails or the device never flushes, retain pending work and its buffers. This does not add a hard completion deadline, a public cancellation API, or a fix for Python/Store wait deadlines. Batch-level FAILED can still precede completion of other tasks. The API documentation states these limits.

Related work checked:

Suggested Merge Priority

  • P0 — Urgent
  • P1 — High
  • P2 — Normal: Focused correctness fix for an opt-in timeout
  • P3 — Low

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Docs

Type of Change

  • Bug fix
  • Documentation update

How Has This Been Tested?

Linux CPU classic build with USE_TENT=OFF, USE_CUDA=OFF. The new suite simulates only the verbs boundary and exercises production posting, endpoint retirement, CQ accounting and public task status queries.

Test commands:

cmake --build build --parallel 8 --target   worker_pool_timeout_test rdma_endpoint_state_test rdma_posted_fifo_test   worker_pool_rail_state_test endpoint_store_test batch_counter_wait_test   transport_uint_test rdma_endpoint_reestablish_test
ctest --test-dir build --output-on-failure   -R '^(worker_pool_timeout_test|rdma_endpoint_state_test|rdma_posted_fifo_test|worker_pool_rail_state_test|endpoint_store_test|batch_counter_wait_test|transport_uint_test)$'

# Separate ASan build, with USE_EVENT_DRIVEN_COMPLETION=ON:
ASAN_OPTIONS=detect_leaks=1:halt_on_error=1 ctest --test-dir build-asan   --output-on-failure -R '^(worker_pool_timeout_test|batch_counter_wait_test)$'
  • Seven focused suites passed, including all 10 new timeout cases. The original four regressions failed against unmodified main and passed after the fix.
  • Coverage includes withholding failure until flush, queued and cross-context siblings, multiple QPs, another task sharing a QP, late success, failed QP-to-ERR, selective signaling, partial post failure and disabled timeout.
  • ASan with leak detection and event-driven completion: both selected suites passed.
  • Real-device endpoint restart/reconnect: passed with MC_SLICE_TIMEOUT=1, MC_WORKERS_PER_CTX=2, mlx5_1 and mlx5_2.
  • Scoped pre-commit passed all applicable hooks. Two pre-existing, unrelated CMake formatting rewrites were excluded from the patch.
  • Documentation: make html passed without warnings; inspected the generated Transfer Engine API page.

Manual real-RDMA A/B fault probe (local harness):

Both binaries used the same probe object and dependencies; baseline substituted the original status/worker/endpoint implementations. Post a real 1 MiB READ or WRITE while the peer QP remains RESET, with a one-second software timeout shorter than the hardware retry budget. Verify no completion and an untouched destination before the timeout. Reuse the destination after the observed terminal result, then enable the peer QP. Fixed runs observe the buffer for 600 ms after recovery, and every case also transfers data over a fresh connection.

On each of mlx5_4 and mlx5_1, run 10 READ and 10 WRITE cases (40 cases per version):

Observation Baseline Patched
Reused buffer overwritten by the old operation 40/40 0/40
Subsequent transfer on a fresh connection succeeded 40/40 40/40

The probe uses real same-host RNICs and controlled QP unavailability. It does not qualify physical link outages, GPU/SGLang workloads, or already-delivered cross-host packets.

Test results:

  • Unit tests pass
  • Integration tests pass (real-device case above)
  • Manual testing done (A/B probe above)

Checklist

  • I have performed a self-review of my own code — human line-by-line review pending
  • I have formatted my code using ./scripts/code_format.sh
  • I have run pre-commit on the files changed in this PR and all applicable hooks pass (unrelated baseline CMake formatting excluded)
  • I have updated the documentation
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue — not applicable; production changes are below 500 LOC, most additions are tests

AI Assistance Disclosure

  • No AI tools were used
  • AI tools were used

OpenAI Codex assisted with implementation, code review, regression tests, real-RDMA A/B testing, documentation and this description. This draft awaits human review of every changed line; no completed human review is asserted.

@github-actions github-actions Bot added the size/M 100-1000 changed lines (additions + deletions) label Oct 28, 2026
@github-actions github-actions Bot added documentation Improvements or additions to documentation run-ci Transfer Engine labels Oct 28, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation run-ci size/M 100-1000 changed lines (additions + deletions) Transfer Engine

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant