fix(downloader): bound the wait for response headers so a wedged origin cannot hang an install forever#11053
Merged
Merged
Conversation
…in cannot hang an install forever
A gallery model install hung for 94 minutes with zero bytes transferred, no
error, no retry and no abort, leaving a partial tree frozen at 18G. The last
log line was the download starting, then silence:
14:06:19 INFO Downloading url=".../LongCat-Video-Avatar-1.5/resolve/<rev>/base_model/diffusion_pytorch_model-000..."
The retry machinery from #10985 was working (two retries fired at 14:05:01 and
14:06:14); the third attempt simply never returned. The install never
completed, the model config was never written, and nothing surfaced the
failure.
The stall watchdog added earlier wraps the response *body*, so it only starts
guarding once downloadClient.Do() has returned. The transport had no
ResponseHeaderTimeout, so a peer that completes the dial and TLS handshake,
reads the request, and then never sends a status line parks Do() for the
process lifetime. IdleConnTimeout governs pooled idle connections, not an
in-flight request. Both the body request and the HEAD that probes for Range
support were unguarded.
Bound the header wait at the transport, not the client: a client-level Timeout
would also bound the body and truncate multi-tens-of-GB downloads. The knob is
opt-in (WithResponseHeaderTimeout) rather than a default in HardenedTransport,
because a streaming endpoint may legitimately withhold headers until it has
something to say, and capping that would break the streaming clients that share
this constructor.
Also fix a classification trap this exposed: net/http reports a
ResponseHeaderTimeout as an error satisfying errors.Is(err,
context.DeadlineExceeded), which IsRetryable read as "the caller gave up" and
refused to retry. An explicit transient marking now outranks the cancellation
sentinels; a caller who genuinely gave up is still caught by the ctx.Err()
check. The resume probe's error is likewise marked transient, so a momentarily
wedged origin no longer turns a resumable download into a hard install failure.
Third defect found in this download path, after #10985 (read vs write errors
conflated) and #11026 (hash verification emitted no progress and an expired
deadline returned success).
Signed-off-by: Ettore Di Giacinto <mudler@localai.io>
Assisted-by: Claude Code:claude-opus-4-8[1m] [Read] [Edit] [Bash]
pull Bot
pushed a commit
to ACay047/TacticalAI
that referenced
this pull request
Jul 23, 2026
… from scratch (mudler#11071) materializeLocked built a download task for every file in the resolved snapshot unconditionally. A completed file is promoted from .downloads/<hash> into snapshot/<path> and its blob deleted, so on any re-entry (a controller pod roll, a resubmit, a crash) the new pass built a task whose .downloads blob no longer existed, re-downloaded the whole file from Hugging Face, and its AfterDownload even removed the already-complete snapshot copy first. The only resume that worked was the downloader's per-file .partial resume for a file caught mid-transfer; completed files were never skipped. Production consequence: installing longcat-video-avatar-1.5 (~35 GB after allow_patterns) on a cluster whose controller rolls hourly (Flux image automation) never converged across ~14 hours. Each roll restarted from the first shard; the completed bytes on disk were repeatedly deleted and re-fetched, and the artifact never promoted. curl of the same files from inside the pod ran fine, proving the loss was the materializer re-fetching, not the network. Before building a task, check whether the file is already materialized and verified in this staging tree's snapshot/ and, if so, keep it and count it complete instead of downloading. "Materialized" means a regular file of the expected size that passes the same verifyDownloadedFile check the download path uses, so the kept manifest entry is byte-for-byte identical to a fresh one and integrity is re-checked. The manifest requires a SHA-256 for every file and non-LFS files carry none to borrow, so a hash is unavoidable for the manifest anyway; a full re-hash of local disk is still orders of magnitude cheaper than re-downloading, and the downloader re-verifies any file it does fetch. Manifest entries are now written at their snapshot index rather than appended in completion order, so a mix of skipped and downloaded files keeps the resolved order that committedResult and staging read. The unconditional root.Remove(destination) now runs only on the fresh-download path; a kept file survives. Skips are logged at INFO with count and bytes so an operator can see resume working. This is the resume-side counterpart to the sibling defects on this path: read/write error conflation and transient retry (mudler#10985), hash-verify progress accounting and silent success on an expired deadline (mudler#11026), and the response-header hang (mudler#11053). The download machinery resumed a single in-flight file; the materializer above it still threw away every completed file on restart. It also makes orphan-partial adoption worth its cost: an adopted tree's completed files were re-downloaded anyway until now. Assisted-by: Claude Code:claude-opus-4-8[1m] [Read] [Edit] [Bash] Signed-off-by: Ettore Di Giacinto <mudler@localai.io> Co-authored-by: Ettore Di Giacinto <mudler@localai.io>
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.
What happened
A gallery model install (materializing a HuggingFace repo) hung for 94 minutes with zero bytes transferred, no error, no retry and no abort. The partial tree stayed frozen at 18G. The last log line was the download starting, then complete silence:
The retry machinery from #10985 was working - two retries fired just before, at 14:05:01 and 14:06:14 ("download failed, retrying"). The third attempt started at 14:06:19 and wedged. The install never completed, the model config was never written, and the operator sees only an install that silently never finishes.
Root cause
The stall watchdog added earlier wraps the response body:
That only starts guarding once
downloadClient.Do(req)has returned. The transport built byhttpclient.Newbounds dial (30s), TLS handshake (10s), idle pooled connections (90s) and expect-continue (1s) - but had noResponseHeaderTimeout. So a peer that completes the dial and TLS handshake, reads the request, and then never sends a status line parksDo()for the process lifetime.IdleConnTimeoutgoverns pooled idle connections, not an in-flight request.Two call sites were unguarded, not one: the body request, and the
HEADthat probes forRangesupport before a resume.The fix
http.Transport.ResponseHeaderTimeout, set at the transport rather than as a client-levelTimeout- a clientTimeoutalso bounds the body and would truncate multi-tens-of-GB downloads, reintroducing a worse bug. The deliberate "no body deadline" property is preserved and covered by a test.120 seconds. It has to tolerate an origin legitimately slow to start answering (a CDN under load, cold object storage, a redirect chain) while still bounding a wedge. Two minutes is far above any plausible legitimate time-to-first-byte and twice the existing 60s stall window. Worst case becomes roughly 6 minutes across the 3-attempt budget instead of unbounded. Overridable via
DownloadResponseHeaderTimeout, mirroringDownloadStallTimeout.The knob is opt-in (
httpclient.WithResponseHeaderTimeout) rather than a new default inHardenedTransport.httpclient.Newis a shared constructor with 28 call sites, several of which carry streaming traffic - the cloud-proxy backend proxying chat completions, and the MCP SSE clients. A streaming endpoint may legitimately withhold headers until it has something to say (a queued or slow-to-first-token completion), so capping that by default would risk breaking exactly the traffic the constructor's doc comment says it protects. Only the downloader opts in; no other caller's behaviour changes.A classification trap this exposed
net/http reports a
ResponseHeaderTimeoutas an error satisfyingerrors.Is(err, context.DeadlineExceeded)(verified empirically, not assumed).IsRetryableexcludedcontext.DeadlineExceededoutright, so it read our own transport guard firing on a wedged peer as "the caller gave up" and refused to retry - the guard would have fired and then aborted the install permanently.An explicit
ErrTransientDownloadmarking now outranks the cancellation sentinels. A caller who genuinely gave up is still caught by thectx.Err()check above it, so nothing that should stop is let through. The resume probe's error is likewise marked transient (it only ever fails on transport trouble - the status is not consulted), so a momentarily wedged origin no longer turns a resumable download into a hard install failure.Tests
TDD, Ginkgo/Gomega. The new downloader specs use a server that accepts the connection, reads the request, and never writes a response. Before the fix all three hung the full
Eventuallywindow; the fourth spec (body not truncated) passed throughout, confirming the suite was not trivially red.After the fix the wedge is detected in ~0.3s. Coverage added:
.partialHEADprobe is guarded toohttpclient: opt-in bounds headers, does not bound the body, and the default transport stays unbounded so streaming is unaffectedIsRetryable: transient outranks a deadline-reporting error, still refuses a done context and a deliberate user abortVerification
go build ./core/... ./pkg/...go test ./pkg/downloader/ ./pkg/httpclient/make lintRelated
Third defect found in this download path, and they form a set:
🤖 Generated with Claude Code
https://claude.ai/code/session_0156CbKpDh8qbAYzJZJDZ2yk