fix(telemetry): do not cancel the finalize that reports an interrupted transfer - #940
Open
sirahd wants to merge 4 commits into
Open
fix(telemetry): do not cancel the finalize that reports an interrupted transfer#940sirahd wants to merge 4 commits into
sirahd wants to merge 4 commits into
Conversation
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>
…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>
There was a problem hiding this comment.
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).
❌ 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.
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).
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.

The bug
A download interrupted with Ctrl-C reported no telemetry at all. Verified against production:
hf downloadinterrupted mid-transfer produced zero documents; it now reportsoutcome=cancelled.The terminal report is produced at the tail of the future handed to a
_finalizingbridge, after the cancelled work returns: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_shutdownthen destroyed the runtime without running the pre-shutdown drain, so anything already in flight was cancelled rather than delivered.The fix
bridge_sync_finalizing/bridge_async_finalizingrun their future to completion. The inner work still returns promptly on cancel — it observes the token through the per-task mapped handles.upload_stream_handleis the one caller whose inner future watches no token, so it keeps the old behaviour via explicit_cancellablevariants.perform_sigint_shutdownruns the pre-shutdown drain before dropping the runtime, asDrop for XetRuntimealready did.sigint_abortcancels first and tears the runtime down last, waiting in between for finalizing work to finish.XetRuntimecounts that work (enter_finalizing/finalizing_in_flight) so the wait ends as soon as it is done.final_flush_timeout2s → 500ms, now both the drain budget and thesigint_abortceiling.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 callsfinalize_download_session, andperform_sigint_shutdowntore 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 streamfinishkeeps the old cancel race via explicit_cancellablevariants because inner work does not watch the token.XetRuntimeaddsenter_finalizing/finalizing_in_flightso shutdown can wait for that work;sigint_abortcancels first, polls until finalizing work finishes (bounded byfinal_flush_timeout), then runs the pre-shutdown drain and drops the runtime.FileDownloadSession::finalize_abandonedis idempotent and called fromDropand from download groupabort()before runtime destruction. Downloadfinishmoves session finalization inside the bridged future. Defaultfinal_flush_timeoutis 500ms (was 2s). Integration tests coverabort+sigint_abortand interrupt mid-finish_blocking.Reviewed by Cursor Bugbot for commit f620608. Bugbot is set up for automated code reviews on this repo. Configure here.