Skip to content

[Bugfix][TENT] Don't throw when aggregating transfer status during teardown - #3768

Merged
alogfans merged 1 commit into
kvcache-ai:mainfrom
staryxchen:fix/tent-peer-death-abort
Sep 2, 2026
Merged

alogfans merged 1 commit into
kvcache-ai:mainfrom
staryxchen:fix/tent-peer-death-abort

Conversation

@staryxchen

Copy link
Copy Markdown
Collaborator

Description

When an RDMA peer disappears (process SIGKILL, host crash), posted writes
often complete with no error CQE. Slices wait for the software timeout,
failover exhausts the only enabled transport, and getTransferStatus
reports FAILED. Clients that poll from several threads then tear down
the engine (or exit the process) overlap getBatchStatus with destruction
of a function-local unordered_map. .at() throws std::out_of_range,
which during terminate/exit becomes terminate called recursively.

That is an engine teardown bug, not a benchmark-only failure. tebench
with 8 initiator threads and a killed target is how we reproduced it
end-to-end; any multi-threaded Transfer Engine client that stops the
engine on FAILED can hit the same throw.

This change:

  • ranks transfer statuses with transferStatusSeverity() (a switch,
    unknown values treated as FAILED) so getBatchStatus never throws
    from unordered_map::at during teardown
  • stops tebench workers with a flag and joins them from main instead of
    calling exit() on each FAILED (also treats TIMEOUT / CANCELED /
    INVALID as terminal so the poll loop cannot spin)

No GitHub issue filed. Distinct from #3636 (worker self-deadlock on a
full MPSC queue); this path reached FAILED in ~12s.

Module

  • Transfer Engine (mooncake-transfer-engine)
  • Mooncake Store (mooncake-store)
  • Reshard (mooncake-reshard)
  • Mooncake EP (mooncake-ep)
  • Mooncake PG (mooncake-pg)
  • Integration (mooncake-integration)
  • P2P Store (mooncake-p2p-store)
  • Python Wheel (mooncake-wheel)
  • Common (mooncake-common)
  • Mooncake RL (mooncake-rl)
  • CI/CD
  • Docs
  • Other

Type of Change

  • Bug fix
  • New feature
  • Refactor
  • Breaking change
  • Documentation update
  • Performance improvement
  • Other

How Has This Been Tested?

Test commands:

cmake --build build -j32 --target tent_failover_test tebench
./build/mooncake-transfer-engine/tent/tests/tent_failover_test
# Two-node DRAM / TENT / RDMA: target on one host, initiator
# WRITE 1MB × 8 threads --duration=60; after measurement starts,
# SIGKILL the target process (not SIGTERM).

Test results:

  • Unit tests pass

  • Integration tests pass (if applicable)

  • Manual testing done (describe below)

  • tent_failover_test: 16/16 passed (including
    TransferStatusSeverityTest known ranks and unknown enum)

  • Two-node DRAM / TENT / RDMA, 8 concurrent transfers × 1MB WRITE:
    SIGKILL peer → FAILED surfaced, process exit 1; no
    terminate called / unordered_map::at. Previously abort, exit 134.

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

  • No AI tools were used
  • AI tools were used (specify below)

Cursor (Grok 4.6) helped diagnose the status-aggregation throw during
peer-death teardown, implement the ranking helper and stop-without-exit
path, and retest with a killed peer. The submitter reviewed every changed
line and reproduced the failure before and after the fix.

Made with Cursor

@catyans

catyans commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator

The new completion wait can deadlock when one benchmark worker fails before every other worker has dequeued its assigned task. runInitiatorTasks() sets pending_ to the full thread count and now waits only for pending_ == 0. The failing worker calls noteTransferFailed(), sets g_tent_running = false, and wakes the other workers. A worker that has not yet swapped its current_task_ wakes in runner(), observes !g_tent_running, and exits before executing the task and before decrementing pending_. That scheduled slot is therefore never retired, so runInitiatorTasks() can wait forever instead of reaching the intended orderly teardown.

Please account for every scheduled slot exactly once even when a worker exits before dequeue—for example, cancel and retire still-assigned tasks under mtx_, or use a per-generation completion counter whose cancellation path contributes to completion. A deterministic regression test can pause one worker before it dequeues, make another worker report transfer failure, and assert that runInitiatorTasks() returns and stopInitiator() joins all workers.

Related non-blocking cleanup: volatile bool is not C++ thread synchronization; this path would be safer with an atomic stop flag while keeping task/pending bookkeeping under the mutex.

@staryxchen

Copy link
Copy Markdown
Collaborator Author

@catyans Thanks — that hang is real. runInitiatorTasks() waiting only on pending_ == 0 left a scheduled slot unretired if noteTransferFailed() woke a worker before it dequeued.

The assigned slot is now retired under the gate mutex on the stop path, so every scheduled slot still contributes to completion. g_tent_running is an std::atomic<bool>. InitiatorTaskGateTest.FailureBeforeDequeueDoesNotHang covers the pause-before-dequeue case.

tebench called exit() from worker threads when a transfer FAILED, so a
SIGKILL'd RDMA target made concurrent workers abort with unordered_map::at.
Return failure to main so threads can join, rank unknown statuses without
throwing, and wait on stop or pending==0 under the mutex like classic TE.

Signed-off-by: staryxchen <staryxchen@tencent.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@staryxchen
staryxchen force-pushed the fix/tent-peer-death-abort branch from 953d820 to 13803d7 Compare September 1, 2026 03:58

@catyans catyans 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. Re-reviewed the current head; transfer failures now stop the atomic run state, release the batch, wake the task waiters, and let runInitiatorTasks() return so stopInitiator() can join every worker. All CI checks pass.

suggestion (non-blocking): the FailureBeforeDequeueDoesNotHang regression mentioned above is not present in the current squashed head. Please restore that deterministic coverage in this PR or a follow-up.

@alogfans alogfans 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

@alogfans
alogfans merged commit 128ba6c into kvcache-ai:main Sep 2, 2026
25 checks passed
@staryxchen
staryxchen deleted the fix/tent-peer-death-abort branch September 2, 2026 06:49
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