Skip to content

[hub] Fix stale abort listener terminating pooled sha256 workers - #2325

Merged
coyotte508 merged 1 commit into
huggingface:mainfrom
DrVelvetFog:fix/sha256-abort-listener-leak
Jul 28, 2026
Merged

[hub] Fix stale abort listener terminating pooled sha256 workers#2325
coyotte508 merged 1 commit into
huggingface:mainfrom
DrVelvetFog:fix/sha256-abort-listener-leak

Conversation

@DrVelvetFog

@DrVelvetFog DrVelvetFog commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Fixes #2311.

The abort listener in the pooled web-worker path is only removed when it fires, so a successful hash leaves it attached to the caller's signal. When that signal aborts later — one signal usually covers a whole multi-file commit, so this is the ordinary case — the listener runs against an operation that already finished and:

  • terminates a worker that freeWorker already returned to pendingWorkers
  • rejects a generator that already returned, producing an unhandled AbortError

cleanup() now removes the abort listener too, so every exit path — success, worker error, non-hash message, abort — drops it in one place.

The test drives the pooled path with a stand-in Worker (Node has no global one), completes a hash, then aborts. Without the fix it fails on the terminate count and throws an unhandled ABORT_ERR. It's excluded from the browser config for the same reason uploadShards.spec.ts is — vi.mock.

One thing I noticed but left alone: destroyWorker only deletes from runningWorkers, so a worker terminated while sitting in pendingWorkers stays in the pool and getWorker hands it to the next caller. Nothing reaches that state with this fix in place, but happy to harden it separately if you'd like.


Note

Low Risk
Targeted fix to frontend sha256 worker cleanup with a focused unit test; no auth, data, or API surface changes.

Overview
Fixes a lifecycle bug in the pooled web-worker sha256 path where the abort listener stayed on the caller’s AbortSignal after a hash finished successfully.

cleanup() now removes that listener on every exit (success, worker error, abort), so a later abort—typical when one signal covers a multi-file commit—no longer terminates a worker already returned to the pool or rejects an already-settled async generator.

Adds a Node-only regression test with a fake Worker and excludes it from the browser Vitest config (same pattern as uploadShards.spec.ts for vi.mock).

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

@DrVelvetFog
DrVelvetFog requested a review from coyotte508 as a code owner July 28, 2026 13:10
@DrVelvetFog

Copy link
Copy Markdown
Contributor Author

The red CI looks unrelated to this PR: HF_TOKEN is empty on fork runs, and all 11 failures are the API integration tests in src/lib (commit, upload-files, create-branch, delete-*, copy-files) — each one a 60s timeout. The new src/utils/sha256-abort.spec.ts passes in that same run (45ms, 404 tests green), and browser was cancelled about two minutes after node failed rather than failing on its own.

Happy to move the test or reshape it if you'd rather it lived somewhere else.

@coyotte508
coyotte508 merged commit 14fd672 into huggingface:main Jul 28, 2026
5 of 9 checks passed
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.

A late abort after successful hashing terminates a worker already returned to the pool and emits an unhandled AbortError

2 participants