feat(hub): resume interrupted model downloads via HTTP Range (Node)

[renezander030]

Follow-up to the discussion in #1220 (thanks @emojiiii for handing it over, and @sroussey for the Xet/CDC pointer).

This is step 1: HTTP Range-resume for the Node file-cache path, which is where large model weights are streamed to disk. It ships resume for how downloads work today, with no new transport dependency. Xet-native chunk caching (via the huggingface.js download path) is the natural follow-up and is intentionally out of scope here.

What changes

• FileCache streams large downloads to a deterministic <key>.incomplete file plus a small sidecar ({ etag, total }), and keeps the partial on failure instead of deleting it.
  • getResumeInfo(request) reports { size, etag, total } when a consistent partial exists.
  • put() branches on the response status: 206 appends to the partial (offset/total parsed from Content-Range); 200 restarts cleanly (truncates any stale partial). A truncated body (loaded < Content-Length) is rejected and the partial retained for the next attempt.
  • A per-key lock preserves the previous concurrent-writer guarantee: if the partial is already held by another writer, put() falls back to the original unique-temp path (correct, non-resumable, never corrupts the shared partial). Stale locks are stolen after a TTL.
• hub.js only sends Range/If-Range on the streaming path (IS_NODE_ENV && return_path), since a 206 on the buffered path would yield only the trailing bytes. It now accepts 206 alongside 200 and caches both. If-Range: <etag> makes the server fall back to a full 200 if the file changed upstream.
• CacheInterface gains an optional getResumeInfo so custom caches can opt in.

Scope / notes

• Node-only by design. The browser Cache API cannot store partials; OPFS/Xet is the follow-up (step 2).
• Incremental sha256 integrity verification is deliberately left for a follow-up so this PR stays focused on the resume mechanics.

Tests

New tests/utils/file_cache.test.js covers: no-partial, full 200 (no leftovers), truncated body (partial + sidecar retained, getResumeInfo correct), 206 append to completion, and 200 restart over a stale partial. Existing cache/custom_cache suites still pass; typecheck, prettier, and build are green.

Closes #1220

Opening as a draft to get maintainer direction on the Xet question in #1220 before polishing. Happy to adjust.

[nico-martin]

Hi @renezander030, thank you so much for looking into this! Resumable downloads would be really useful here. Before this is safe to merge, we need to fix the 206 lock-contention path, avoid stealing locks from slow active downloads, and fully validate ranges and validators. Could you also add concurrency and malformed-range tests? I'm holding off on merging because the current edge cases can corrupt a cached model.

[renezander030]

Thanks @nico-martin — all four are fair, and the 206 one is a genuine corruption bug rather than a sharp edge. You're right that a ranged response can reach the unique-temp path and get renamed over the final file.

To answer the concurrency question directly: yes, concurrent use is supported. The random-suffix temp path predates this PR and exists for exactly that. My fallback preserved it for 200 responses and quietly broke it for 206.

Rather than patch the fallback, I moved the coordination ahead of the request, which I think removes the class of bug instead of the instance:

• Reserve before requesting. getResumeInfo is now reserveResume — it takes the per-key lock and only then reports the partial. A Range is therefore only ever sent by the writer that owns the bytes it continues. A writer that loses the race gets undefined and does a plain full download, i.e. today's behaviour. put() refuses an unreserved 206 outright, and the fallback path refuses any non-200, so neither can publish a fragment as a whole file. releaseResume covers callers that abandon a download — including the case where another writer finished the file first, where put() is skipped entirely.

• Lock ownership and liveness, not a blind TTL. A held lock records its owner and is refreshed every 30s while the download runs, so an untouched lock means an absent writer rather than a slow one — that was the missing piece in the original TTL. A lock is only taken over when it is stale and its owner is provably gone (process.kill(pid, 0) → ESRCH). Since a pid on a shared cache directory can belong to an unrelated process on another host, an unverifiable owner is left alone until a one-hour ceiling, which prevents a permanent deadlock without ever interrupting a live slow download.

• Full Content-Range validation. Start must equal the partial size (no gap, no overlap), the span must agree with Content-Length, and the total must match the sidecar. bytes */N and suffix bytes -N/M are rejected outright — neither can be appended safely. On a mismatch the partial is discarded so the next attempt restarts clean rather than retrying onto data from a different revision forever.

• No resume without a validator. If there is no usable ETag to pin via If-Range, no Range is sent at all and the download restarts from byte 0.

Tests added as requested: concurrent writers (both finish, file complete exactly once, no temp/lock/sidecar left behind), a failing writer not stranding the key, unreserved and contended 206s, ten malformed Content-Range forms, lock stealing vs. not-stealing across four ownership scenarios, and the no-validator case. 29 tests in file_cache.test.js; the existing cache/custom_cache/hub suites, typecheck, prettier, and build are green.

One open item from the original description that I don't think got picked up: is the step-1 (Range) / step-2 (Xet-native) split the scope you want here, or would you rather this land closer to the huggingface.js download path? Happy to go either way — I'll take it out of draft now since the correctness gate is addressed.

[renezander030]

@nico-martin one small thing when you get a chance — the workflow runs on this branch are sitting in action_required, so CI hasn't actually executed against the new commit. Could you approve them?

The full gate is green locally (jest including the new concurrency and malformed-range tests, tsc --build, prettier, and the esbuild bundles), but I'd rather have CI confirm that on your matrix than take my machine's word for it.

[renezander030]

Thanks for approving the runs. Results on 76e1bc8c: Unit tests passed on Node 18, 20 and 22; Build PR Documentation failed, and that one was on me — now fixed in 677362ac.

The failure was in the workflow's pre_command, at docs-api. jsdoc cannot parse an object literal nested inside a generic, so the CacheInterface entry

@property {(request: string) => Promise<{size: number, etag: string|null, total: number}|undefined>}

aborted the run with Invalid type expression. Extracted into a named ResumeInfo typedef, referenced from both the interface and FileCache.reserveResume.

Worth flagging that this was not introduced by the review fixes — it has been present since the first commit on this branch under the old getResumeInfo name. I confirmed it by running docs-api against main (passes), against the original commit 5101df2c (fails with the same error), and against the fix (passes). It went unnoticed because the workflow runs on this branch were awaiting approval and had never actually executed. So the Range-resume work itself was never doc-clean; it just looked that way.

Local gate on the new commit: docs-api, the four cache/hub jest suites, tsc --build, prettier and the esbuild bundles are all green. Could you approve the runs once more so CI can confirm?

[HuggingFaceDocBuilderDev]

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

[nico-martin]

@renezander030 Thanks, the redesign addresses the four blocking issues from my first review, and rest looks good too.

I found two cleanup issues remaining:

• FileCache.delete() should remove the associated .incomplete, sidecar, and lock artifacts, not only the completed file.
• Please ensure the output stream is destroyed/closed before releasing the reservation on reader, writer, or callback errors. While touching this path, completion should require loaded === total, not only reject loaded < total, so an oversized ranged body cannot be promoted.

Once those are covered with failure/deletion tests, I’m happy to approve.

[renezander030]

@nico-martin Both covered in 8b3f681d.

delete() artifacts. It now removes the .incomplete, its sidecar and the lock alongside the completed file. If this instance holds a reservation for the key it is released first, so the lock is dropped by its owner and its heartbeat stops, rather than being unlinked out from under a live timer.

One deliberate choice on the return value: it still reports true only for a real entry — the completed file or a partial. A leftover sidecar or lock is swept up either way but does not count. clear_cache chains on this (delete(proposedCacheKey) falling back to delete(localPath)), so letting a stray lock report true would silently skip the fallback key.

Stream teardown. Both write paths now destroy the stream and wait for the descriptor to be released before the reservation is dropped — reader errors, write errors and progress-callback errors alike. Previously the finally released the lock while the stream was still open on the partial, so the next writer could take the key with a descriptor still on the file. The fallback path had the same shape, where an open handle also makes the temp-file unlink fail outright on Windows.

While there I added an error listener to both streams. An async write failure surfaces through the write/close callbacks, but with no listener attached the same failure also reached the process as an unhandled 'error' event.

Completion gate. Now loaded === total rather than loaded < total. A short body still keeps its partial so the next attempt can resume; an oversized one is discarded, since it already runs past total and can never be resumed onto. Neither is published.

Tests — 13 added, 42 total in file_cache.test.js:

• deletion: completed file, partial + sidecar + stale lock, both together, a held reservation, nothing cached, and a bare leftover lock (which must return false)
• stream teardown: reader error mid-body, progress callback throwing, writer error, and the fallback path's temp-file cleanup
• oversized bodies: a 200 overshooting Content-Length and a 206 overshooting a valid Content-Range

Nine of the thirteen fail against the previous commit; the other four pin down behaviour that was already correct and is easy to break from here. The teardown tests assert against /proc/self/fd where it exists — POSIX unlinks an open file happily, so a removability check alone would have passed either way and proved nothing.

Local gate green: file_cache/cache/custom_cache/hub suites, tsc --build, prettier, docs-api, and the esbuild bundles.

I resolved the four threads from your first review, so only what you flag from here stays open. CI on 8b3f681d has gone to action_required again — could you approve the runs so the matrix confirms the above?

[nico-martin]

Thanks, the cleanup and exact-size checks look good. I found one remaining concurrency issue: delete() currently removes the lock even if another writer is actively using it. That could let a second writer acquire the same key and write concurrently. Could you preserve live foreign locks and only remove locks we own or have confirmed are stale?
Also, stream.destroyed doesn’t necessarily mean the file descriptor is closed yet. destroyStream() should still wait for close unless stream.closed is already true.

[renezander030]

@nico-martin Both fixed in 51ffe494. You were right on each, and the lock one was a real hole — I had reasoned about delete() as "remove the entry" and not as something that races a live writer, which is exactly the case the reservation scheme exists to handle.

Live foreign locks. delete() now runs the same abandonment check reserveResume uses, rather than unlinking the lock outright. Absent, stale with a provably dead owner, or stamped by this process → ours to clear. Anything else is a writer downloading right now.

One decision I'd like you to sanity-check, since it goes slightly past what you asked. When the lock is live I also leave the .incomplete and its sidecar. They are that writer's working set: pulling the partial out from under it would leave the lock pointing at nothing and make the writer's final rename fail with ENOENT, so preserving only the lock felt half-done. The completed file is still removed in that case — it is the entry the caller asked to delete, and it is not what the other writer is holding. If you'd rather delete() were strictly lock-scoped and cleared the partial regardless, say so and I'll narrow it.

destroyed vs closed. Correct, and my test was weaker than I thought. destroyStream now short-circuits on stream.closed and waits for the close event otherwise, re-destroying only a stream that has not already begun tearing down.

Worth being precise about the impact, because it changes how the test had to be written: against a real fs.WriteStream the gap is invisible. The descriptor is released within the next event-loop turn, and the unlink in releaseResume yields long enough for that to land — which is why my /proc/self/fd assertions passed against the previous commit either way and proved nothing here. The guarantee was missing rather than the behaviour, so the new test drives put through a stream whose teardown is slow enough to observe and asserts put does not return while the descriptor is still open. It fails against 8b3f681d.

Tests — 4 added, 46 total:

• a live foreign lock surviving delete while the completed file goes, and the key staying unavailable to anyone else afterwards
• a stale lock cleared along with its partial and sidecar once its owner is provably gone
• a lock this instance stamped but no longer tracks — the "we own it" branch, where the pid is alive because it is us
• put not returning while a mid-teardown stream still holds its descriptor

Two of the four fail against 8b3f681d; the other two pin down the branches of the new abandonment check that happen to coincide with the old unconditional behaviour.

Local gate green: file_cache/cache/custom_cache/hub/model_registry (98 passed, 1 skipped), tsc --build, prettier, docs-api, esbuild bundles. Could you approve the runs on 51ffe494?