Skip to content

fix(download): let an aborted download group actually stop its transfers - #949

Open
dhruvdavest07 wants to merge 1 commit into
huggingface:mainfrom
dhruvdavest07:fix/download-group-abort-cancels-transfers
Open

fix(download): let an aborted download group actually stop its transfers#949
dhruvdavest07 wants to merge 1 commit into
huggingface:mainfrom
dhruvdavest07:fix/download-group-abort-cancels-transfers

Conversation

@dhruvdavest07

@dhruvdavest07 dhruvdavest07 commented Aug 24, 2026

Copy link
Copy Markdown

Fixes #942.

What was happening

XetFileDownloadGroup::abort() unblocked the caller straight away, but the transfer it was
meant to stop carried on. The report has a 1 GB file aborted after five seconds still pulling
data for another three minutes, roughly 1.2 GB moved and then discarded.

Why

Two separate things, and each one on its own would have been enough.

FileDownloadSession::setup_reconstructor never called
FileReconstructor::with_cancellation_token. Every reconstruction therefore ran with the
token FileReconstructor::new builds for itself, which no caller holds a handle to. The
reconstruction loop does check that token, at the outer term loop, at each term, and while
waiting on the download buffer semaphore, but nothing could ever cancel it. Searching for
with_cancellation_token in the tree turns up three call sites, all inside
mod cancellation_tests. The mechanism was live in tests and dead in production.

Separately, abort() calls download_join_handle.abort(). That drops the wrapper task, which
is why the Python caller returns immediately and wait_to_finish() raises. But the work the
reconstruction had already spawned is not a child of that task, so aborting the handle does
nothing to it. Tokio does not cancel the tasks a cancelled task spawned.

So the caller saw a cancelled download and the network saw a download running to completion.

The change

download_file_background takes an Option<CancellationToken> and passes it down through
download_file_with_id to setup_reconstructor, which hands it to the reconstructor. The
existing checks in the term loop then do what they were written to do.

XetFileDownloadGroup::start_download_file_to_path creates its per-file child task runtime
before starting the transfer rather than after, so it has a token to pass. That reorder is the
whole change on the xet_pkg side.

One behaviour change worth calling out. download_file_with_id validates the byte count
against file_info.file_size() and returns DataError::SizeMismatch on a mismatch. A
cancelled download stops short by design, so it tripped that check and a user abort surfaced
as a corrupt download. The size check is now skipped when the token is cancelled.

download_file_background is public, so api_changes/update_260824_download_cancellation_token.md
is included per the convention in api_changes/README.md. The one other caller,
legacy/data_client.rs, passes None and is unchanged in behaviour.

What this does and does not do

It stops new ranges being scheduled. Ranges already in flight when the token fires still run to
completion, so abort frees bandwidth progressively rather than instantly. That matches the
issue's stated expectation ("cancel outstanding chunk transfers or prevent scheduling new
ranges") on the second count.

If you want in-flight range requests torn down as well, that is a larger change into the
transfer scheduler and I would rather do it as a follow-up than bundle it here. Happy to take
it on if you point me at where you would want the seam.

Testing

Three tests in xet_data/src/processing/file_download_session.rs, using the existing local-CAS
harness, so no network:

  • an already-cancelled token stops the transfer: 0 bytes, no file contents written
  • None still downloads normally, so the token is genuinely optional
  • a live, uncancelled token downloads normally, so passing one does not interfere
cargo test -p xet-data --lib
cargo test -p hf-xet --lib

280 passing in xet-data and 142 in hf-xet.

I checked the tests against the unfixed code by leaving the plumbing in place and removing only
the with_cancellation_token hand-off. The cancellation test fails, the other two pass. That is
the isolation I wanted: the two that pass either way are regression cover, and the one that
flips is the one demonstrating the bug.

cargo fmt --check is clean. cargo clippy --all-targets reports the same two warnings in
file_download_session.rs before and after this change, both well away from the lines touched,
so I left them alone.


Note

Medium Risk
Touches core download cancellation and error classification for aborted transfers; behavior change is intentional but affects bandwidth and how partial downloads surface to callers.

Overview
Fixes aborted download groups continuing to pull data (#942) by threading a caller-owned CancellationToken through background file downloads into FileReconstructor, so the reconstruction term loop can stop scheduling new ranges when the group is cancelled.

FileDownloadSession::download_file_background now takes Option<CancellationToken>; setup_reconstructor applies with_cancellation_token when a token is supplied (streaming paths pass None). XetFileDownloadGroup creates the per-file child TaskRuntime before starting the transfer and passes task_runtime.cancellation_token() into the session. The legacy download_async path passes None, preserving prior behavior.

When a download is stopped via cancellation, download_file_with_id returns bytes written so far and skips the expected-size SizeMismatch check so user abort is not reported as corruption. An api_changes note documents the public signature change.

Reviewed by Cursor Bugbot for commit 19b079b. Bugbot is set up for automated code reviews on this repo. Configure here.

XetFileDownloadGroup::abort() unblocked the caller but the transfer kept
going. A 1 GB file aborted after five seconds carried on for another three
minutes at full bandwidth before the data was thrown away.

Two things combined to cause it. FileDownloadSession::setup_reconstructor
never called FileReconstructor::with_cancellation_token, so every
reconstruction ran against a token the reconstructor had made for itself
and nobody could reach; the loop checks it at every term and every buffer
acquisition, but it could never be cancelled. In production
with_cancellation_token was only ever called from tests.

Meanwhile abort() aborted the wrapper join handle. That drops the wrapper
task, which is why the Python side returned promptly, but the work the
reconstruction had already spawned is not a child of that task and carried
on to completion.

download_file_background now takes an optional cancellation token and
passes it down to the reconstructor, so cancelling stops the term loop from
scheduling further ranges. The download group creates its per-file child
task runtime before starting the transfer so it has a token to hand over.

A cancelled download also no longer reports SizeMismatch. Stopping short is
the point, so the size check is skipped once the token is cancelled rather
than reporting a user abort as a corrupt file.

Ranges already in flight when the token fires still finish; this stops new
ones being scheduled rather than tearing down open connections.

Fixes huggingface#942

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 19b079b. Configure here.

.download_session
.download_file_background(file_info.clone(), absolute_path.clone(), Some(token.clone()))
.await?;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Errors reported as user cancellation

High Severity

The per-file TaskRuntime token is handed straight to FileReconstructor, whose RunState::set_error cancels that same token on a fetch or write failure. The mapped handle is already waiting on token.cancelled() and treats it as UserCancelled, so a real download failure is reported as a user abort and the underlying error is lost.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 19b079b. Configure here.

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.

XetFileDownloadGroup.abort() returns immediately but the aborted transfer keeps downloading to completion

1 participant