[BUG][AscendP2P][InProcess] Fix Host-Staging H2H Copy non-draining when cancelled - #272
Open
matthewygf wants to merge 2 commits into
Open
matthewygf wants to merge 2 commits into
matthewygf wants to merge 2 commits into
Conversation
* [Bug] Drain host-staging H2H copy pool before arena release Cancelling or failing mid-copy left ThreadPoolExecutor workers writing into staging pages that callers freelist via release_staged(), causing arena UAF on reuse (e.g. sync-get timeout during post-DMA H2H copy). Co-authored-by: Matthew Yeung <yyygggfff@hotmail.com> * Harden H2H drain against repeated cancellation A single shielded await still resumes the caller on the next cancel, so the previous drain leaked live writers when a second cancel arrived (e.g. loop teardown after a sync-get timeout), and the no-pool branch was not drained at all. Re-await until the work settles via _await_settled, and mirror the pool futures onto the loop with wrap_future instead of blocking a default executor thread per in-flight copy. Co-authored-by: Matthew Yeung <yyygggfff@hotmail.com> * Assert live worker count in H2H drain tests The tests inferred that copy threads were still running from the fact that the release gate was shut. Count threads inside the fake copy instead and assert on it, so a failure reports how many were still writing, and widen the observation window to 500ms per test. Co-authored-by: Matthew Yeung <yyygggfff@hotmail.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Matthew Yeung <yyygggfff@hotmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
The AscendP2P host-staging path can cancel the H2H copy coroutine, but the underlying
thread pool keeps executing. Cancelling an asyncio future does not stop a
ThreadPoolExecutorworker that has already started —concurrent.futures.Future.cancel()returns
Falseonce the work is running.The awaiting coroutine therefore resumed immediately while copy threads were still
touching the staging pages, and the caller went on to
release_staged()them. This is nowfixed: we drain the copy first, then re-raise the cancellation.
Introduced with H2H host staging (#259).
Impact
release_staged()returns pages to the staging arena free list, so a page could be handedto another in-flight transfer while a copy thread was still using it. Both peers are
affected, because both funnel through the same helper:
copy_receiver_staging_to): the page is freed while threads still read it,so a concurrent transfer can allocate it and DMA new data in — the final KV buffer ends
up holding another request's bytes.
stage): the page is freed while threads still write into it, so the nextstage()can take the page and have it overwritten underneath.stage()also caught onlyException, so aCancelledErrorskipped the release entirely and leaked the page untilchannel teardown.
Trigger:
use_host_stagingwithp2p_delay_pull: Falseandp2p_use_npu: False, when async-get timeout (
p2p_sync_get_timeout_s) cancels the pull during the post-DMA host copy.What changed
All in
_async_h2h_copy/stageinhccl_channel.py:executor.submitand keep theconcurrent.futureshandles locally, sotask cancellation cannot detach the handle from work that is still running.
run_in_executormarks its future cancelled even when the thread cannot be stopped._await_settledhelper: re-await underasyncio.shielduntil the work actuallysettles, then re-raise the cancellation. A single shielded await is not enough —
shieldkeeps the inner operation alive but still resumes the caller immediately, so repeated
cancels (e.g. loop teardown after a timeout) would still free pages under live writers.
gather(..., return_exceptions=True)so a failing slice cannot resume the caller whilesibling threads are still writing.
asyncio.wrap_futureto observe thread completion on the event loop instead of blocking anextra executor thread per in-flight copy.
stage()releases arena pages onCancelledErroras well asException, which is safenow that the drain is guaranteed.
Implications
We wait slightly longer on the abort path for the in-flight copy to finish. That is one
chunk-sized host memcpy — sub-millisecond to a couple of milliseconds in practice. It is
also off the request critical path: the blocking sync-get has already returned
TimeoutErrorto the worker by then, so the only thing deferred is reuse of the stagingpage.
The copies themselves still run off the event loop; only the completion notification touches
it. At teardown
close()already does_staging_copy_pool.shutdown(wait=True)beforeclosing the arena, so threads are joined before the arena memory goes away.
Tests
tests/v1/transfer_channel/test_staging_h2h_copy.pyreplacestorch._foreach_copy_with afake that parks inside the copy body and counts the threads currently in it, so each test
asserts both "N threads are still writing" and "the caller has not resumed" at the same
instant, across a 500 ms window.
test_cancel_waits_for_executor_workerstest_repeated_cancel_still_drainstest_exception_in_one_slice_drains_siblingstest_stage_cancelled_error_releases_stagedstage()leaking its arena page on cancelAll four fail against the pre-fix code (
caller resumed while 2 copy thread(s) were still writing) and pass with the fix. They need no NPU device and no peers, but are skipped whenthe HCCL extension is not built.
Scope note
This addresses the host-side copy pool only. Draining the HCCL transport stream before
releasing buffers on an aborted transfer is a separate concern and is not part of this PR.