Skip to content

fix(telemetry): do not cancel the finalize that reports an interrupted transfer - #940

Open
sirahd wants to merge 4 commits into
mainfrom
sira/telemetry-report-on-abort
Open

fix(telemetry): do not cancel the finalize that reports an interrupted transfer#940
sirahd wants to merge 4 commits into
mainfrom
sira/telemetry-report-on-abort

Conversation

@sirahd

@sirahd sirahd commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

The bug

A download interrupted with Ctrl-C reported no telemetry at all. Verified against production: hf download interrupted mid-transfer produced zero documents; it now reports outcome=cancelled.

The terminal report is produced at the tail of the future handed to a _finalizing bridge, after the cancelled work returns:

let result = inner.handle_finish().await;
finalize_download_session(&ds, &result).await;   // <- the report

Both bridges wrapped that whole future in a select! against the cancellation token, so a cancel dropped the future and took the report with it. perform_sigint_shutdown then destroyed the runtime without running the pre-shutdown drain, so anything already in flight was cancelled rather than delivered.

The fix

  • Non-racing finalizing bridges. bridge_sync_finalizing / bridge_async_finalizing run their future to completion. The inner work still returns promptly on cancel — it observes the token through the per-task mapped handles. upload_stream_handle is the one caller whose inner future watches no token, so it keeps the old behaviour via explicit _cancellable variants.
  • perform_sigint_shutdown runs the pre-shutdown drain before dropping the runtime, as Drop for XetRuntime already did.
  • sigint_abort cancels first and tears the runtime down last, waiting in between for finalizing work to finish. XetRuntime counts that work (enter_finalizing / finalizing_in_flight) so the wait ends as soon as it is done.
  • final_flush_timeout 2s → 500ms, now both the drain budget and the sigint_abort ceiling.

Note

Medium Risk
Touches SIGINT shutdown, task-runtime cancellation semantics, and telemetry flush timing; behavior change is intentional but affects all interrupt and finalize paths on native targets.

Overview
Ctrl-C and abort paths no longer drop download telemetry. Interrupted transfers (e.g. hf download) used to emit no terminal document because finalizing bridges raced their futures against the cancellation token and dropped the tail that calls finalize_download_session, and perform_sigint_shutdown tore down the runtime before draining in-flight telemetry.

Finalizing bridges now run to completion by default (run_inner_async_to_completion / non-racing sync path) so cooperative cancel still returns quickly but the terminal report always runs. Upload stream finish keeps the old cancel race via explicit _cancellable variants because inner work does not watch the token. XetRuntime adds enter_finalizing / finalizing_in_flight so shutdown can wait for that work; sigint_abort cancels first, polls until finalizing work finishes (bounded by final_flush_timeout), then runs the pre-shutdown drain and drops the runtime. FileDownloadSession::finalize_abandoned is idempotent and called from Drop and from download group abort() before runtime destruction. Download finish moves session finalization inside the bridged future. Default final_flush_timeout is 500ms (was 2s). Integration tests cover abort + sigint_abort and interrupt mid-finish_blocking.

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

An interrupted download reported nothing. huggingface_hub's KeyboardInterrupt
handler runs the group's __exit__ (which calls abort()) and then
abort_xet_session() -> XetSession::sigint_abort(). Two things then conspired:

- abort() deliberately leaves the session unfinalized so Drop can infer the
  outcome from progress, but sigint_abort() destroys the tokio runtime while the
  group is still alive, so that Drop had nothing left to send on.
- perform_sigint_shutdown() drops the runtime directly rather than through
  Drop for XetRuntime, so the registered pre-shutdown drain never ran and any
  in-flight document was cancelled rather than delivered.

Fixes both halves. FileDownloadSession::finalize_abandoned reports and marks the
session finalized, so abort() can report while sending still works and the later
Drop stays a no-op. perform_sigint_shutdown now runs the drain before dropping
the runtime, exactly as Drop for XetRuntime does.

A user aborting a large download is the abandonment case most worth capturing,
and it was the one case that reported nothing.

Not covered here: XetUploadCommit::abort has the same shape, and
_upload_pipeline.py aborts on any BaseException, so upload telemetry is lost the
same way. Left for a follow-up with its own test rather than shipped untested.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment thread xet_pkg/tests/test_download_telemetry.rs
…d transfer

The previous commit fixed a path the CLI never takes. An interrupted download
still reported nothing, because the finalize is not skipped - it is cancelled.

Every group's terminal report is produced at the tail of the future passed to a
_finalizing bridge, after the cancelled work returns:

    let result = inner.handle_finish().await;
    finalize_download_session(&ds, &result).await;   // <- the report

but both bridges wrapped that whole future in a select! against the cancellation
token, so a cancel dropped the future and took the report with it. That is the
real Ctrl-C path: start_download_file only starts the transfer, the wait happens
in the binding's __exit__ via finish_blocking, and SIGINT lands while that is
running.

The finalizing bridges now run their future to completion. The inner work still
returns promptly on cancel - it observes the token through the per-task mapped
handles - so a cancel is no less responsive.

One caller cannot: upload_stream_handle's cleaner.finish() watches no token, so
the bridge's race is the only thing making a cancel prompt there. It keeps the
old behaviour through the explicit _cancellable variants, audited call site by
call site; the other four were confirmed token-aware before switching.

sigint_abort now cancels first and tears the runtime down last, with a bounded
wait in between: the report is produced on whichever thread called finish - the
bindings run it on a spawned thread so they can poll for signals - so it has to
be waited for rather than raced. XetRuntime counts finalizing work so the wait
ends as soon as it is done rather than always burning the budget; every
TaskRuntime already holds that Arc, so nothing is threaded through the tree.

final_flush_timeout drops to 500ms. It is now both the drain budget and the
sigint ceiling, and with the counter the ceiling is rarely reached.

Verified against production: an interrupted 'hf download' reports
outcome=cancelled where it previously reported nothing.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sirahd sirahd changed the title fix(telemetry): report the transfer a Ctrl-C interrupted fix(telemetry): do not cancel the finalize that reports an interrupted transfer Aug 11, 2026

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

There are 2 total unresolved issues (including 1 from previous review).

Fix All in Cursor

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

Reviewed by Cursor Bugbot for commit c87c52d. Configure here.

Comment thread xet_pkg/src/xet_session/task_runtime.rs Outdated
Async XetFileDownloadGroup::finish() called finalize_download_session
after the bridge awaited, outside the enter_finalizing guard, so a
concurrent sigint_abort could tear the runtime down before the report
sent. Move the finalize call inside the bridged future, mirroring
finish_blocking. Also fixes a flaky sigint abandonment test that could
race a fast in-process completion, and trims several overly long
comments added in the prior two commits.
Convert bridge_async_finalizing_inner to an async fn (clippy's
manual_async_fn) and drop redundant closures wrapping block_on calls
in the upload_commit blocking-round-trip tests (clippy::redundant_closure).
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.

2 participants