Skip to content

[TransferEngine] Delay RDMA redispatch while rails recover - #3555

Closed
alogfans wants to merge 1 commit into
kvcache-ai:mainfrom
alogfans:dev/rdma-delay-redispatch
Closed

alogfans wants to merge 1 commit into
kvcache-ai:mainfrom
alogfans:dev/rdma-delay-redispatch

Conversation

@alogfans

Copy link
Copy Markdown
Collaborator

Description

This PR makes classic RDMA WorkerPool delay and retry redispatch when all currently usable RDMA rails are temporarily unavailable, instead of immediately failing slices.

The change covers transient peer/local rail recovery windows seen during link flap chaos testing. Slices are placed into a delayed redispatch queue with bounded wait time, exponential backoff, jitter, and awareness of existing rail pause deadlines. If rails recover, the slice is redispatched and delivery continues. If recovery never happens within the bounded wait window, the slice fails gracefully.

This also reduces reconnect/handshake pressure during recovery windows by avoiding tight 100ms retry loops.

Module

  • Transfer Engine (mooncake-transfer-engine)

Type of Change

  • Bug fix

How Has This Been Tested?

Test commands:

cmake --build build --target tebench -j 16

python3 mooncake-transfer-engine/tests/chaos/te_chaos.py run \
  --target qjh001 \
  --metadata-mode p2p \
  --suite deterministic \
  --backend classic \
  --fault-kinds link-down \
  --seed 334455 \
  --duration 180 \
  --threads 2 \
  --block-size 65536 \
  --batch-size 16

Test results:

  • Manual testing done
  • tebench build passed
  • TE classic p2p link-down chaos test passed with rc=0
  • Verified no AddressNotRegistered, corrupted size, Failed transfer detected, Data mismatch, or delayed redispatch give-up in the passing run

Checklist

  • I have performed a self-review of my own code
  • I have formatted my code using ./scripts/code_format.sh
  • I have run pre-commit on the files changed in this PR and all hooks pass
  • I have updated the documentation (if applicable)
  • I have added tests to prove my changes are effective
  • For changes >500 LOC: I have filed an RFC issue

AI Assistance Disclosure

  • AI tools were used

Codex helped implement and validate this targeted Transfer Engine RDMA recovery fix. The human submitter should review the changed lines before submission.

@alogfans
alogfans marked this pull request as ready for review August 20, 2026 12:21
@alogfans
alogfans requested a review from doujiang24 as a code owner August 20, 2026 12:21
Copilot AI lite review requested due to automatic review settings August 20, 2026 12:21

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

This PR changes the classic RDMA WorkerPool retry behavior so that when peer/local RDMA rails are temporarily unavailable (paused, endpoint missing, handshake transient failures), slices are queued for delayed redispatch with bounded wait time and backoff instead of being failed immediately. This targets link-flap recovery windows and aims to reduce reconnect/handshake pressure during rail recovery.

Changes:

  • Introduces a delayed-redispatch queue with exponential backoff + jitter and a bounded give-up window.
  • Replaces several immediate-failure paths in performPostSend() / redispatch() with delayed redispatch attempts.
  • Adds worker-loop integration to periodically release delayed slices and optionally sleep until the next delayed slice becomes due.

Reviewed changes

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

File Description
mooncake-transfer-engine/src/transport/rdma_transport/worker_pool.cpp Implements delayed redispatch queueing, release logic, and replaces immediate failures with delayed retries across several RDMA send/redispatch paths.
mooncake-transfer-engine/include/transport/rdma_transport/worker_pool.h Declares delayed redispatch helpers/state and adds rail pause remaining-time accessor.

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

Comment on lines +356 to +360
const uint64_t now = static_cast<uint64_t>(getCurrentTimeInNano());
if (slice->ts <= 0) slice->ts = static_cast<int64_t>(now);
const uint64_t first_wait_ns = static_cast<uint64_t>(slice->ts);
if (now > first_wait_ns &&
now - first_wait_ns > delayedRedispatchMaxWaitNs()) {
Comment on lines +1133 to +1141
const uint64_t next_delayed_due_ns = nextDelayedSliceDueNs();
if (next_delayed_due_ns > 0) {
const uint64_t now = static_cast<uint64_t>(getCurrentTimeInNano());
if (next_delayed_due_ns > now) {
const uint64_t sleep_ns = std::min(next_delayed_due_ns - now,
kDelayedRedispatchWakeCapNs);
std::this_thread::sleep_for(std::chrono::nanoseconds(sleep_ns));
}
}
@catyans

catyans commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Two issues remain: delayed redispatch reuses slice->ts, which can cause pre-post timeouts, and its unconditional sleep can stall normal send/CQ polling by up to 100 ms. Please use separate timing, sleep only when idle, and add a mixed-load regression.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants