libstore: eliminate curl_easy_pause() to fix TCP window collapse on large downloads#10
Open
jld-adriano wants to merge 2 commits intomasterfrom
Open
libstore: eliminate curl_easy_pause() to fix TCP window collapse on large downloads#10jld-adriano wants to merge 2 commits intomasterfrom
jld-adriano wants to merge 2 commits intomasterfrom
Conversation
The streaming download path uses curl_easy_pause() when the sink (store import) can't keep up with the download speed. Each pause/unpause cycle collapses the TCP receive window, throttling effective throughput to ~5 MB/s even though curl can burst at 300+ MB/s — regardless of HTTP/1.1 or HTTP/2. Fix by downloading the entire file into memory before writing to the sink. This lets curl run at full wire speed without any pause cycles. The trade-off is higher peak memory (one full NAR per concurrent download), which matches S3BinaryCacheStore's existing behavior. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Never pause curl during streaming downloads. The previous behavior paused when the internal buffer exceeded downloadBufferSize, calling curl_easy_pause(CURLPAUSE_RECV). This collapses the TCP receive window (rcv_wnd drops from ~80 KB to ~7 KB) and, with HTTP/2, adds WINDOW_UPDATE round-trip overhead on every pause/unpause cycle. For large downloads where the sink is slow (e.g. NAR store imports at ~5 MB/s while curl bursts at 300+ MB/s), the pause/unpause cycles throttle effective throughput by 60x. Instead, let the buffer grow unboundedly so curl always runs at wire speed. The consumer thread drains concurrently. Peak memory equals the full download size, matching S3BinaryCacheStore behavior. This is the general fix — it benefits all callers of the streaming download(request, sink) path, not just HTTP binary caches. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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.
Motivation
curl_easy_pause(CURLPAUSE_RECV)collapses the TCP receive window (rcv_wnddrops from ~80 KB to ~7 KB) and, with HTTP/2, addsWINDOW_UPDATEround-trip overhead on every pause/unpause cycle. For large NAR downloads where the sink is slow (e.g. store imports at ~5 MB/s while curl bursts at 300+ MB/s), this throttles effective throughput by ~60x.This was the root cause of multi-minute CI stalls when downloading from binary caches — observed in production on exa-labs/monorepo CI runners. The stall affects all substituter protocols on nix ≥2.33: both
https://ands3://, sinceS3BinaryCacheStorenow inherits fromHttpBinaryCacheStore(NixOS/nix PRs NixOS#14198 + NixOS#14350, merged Oct 2025).Context
Since nix 2.33,
S3BinaryCacheStoreinherits fromHttpBinaryCacheStore, meaning boths3://andhttps://substituters download NARs through the sameFileTransfer::download(request, sink)streaming path. This path callscurl_easy_pause()when the buffer exceedsdownloadBufferSize, triggering TCP window collapse on every pause/unpause cycle. With the default 1 MB buffer (or even 64 MB), a 2.9 GB NAR liketensorrt-llmcauses hundreds of pause cycles, each throttling the connection.Related monorepo PRs for context: exa-labs/monorepo#29228 (disabled HTTP/2), exa-labs/monorepo#29235 (reverted to
s3://), exa-labs/monorepo#29254 (download-buffer-size→ 4 GB as a config-level workaround). This PR fixes the underlying nix issue so all substituters work at full speed regardless ofdownload-buffer-size.Changes
Two commits, read in order:
1.
http-binary-cache-store.cc—HttpBinaryCacheStore::getFile(path, Sink&)now downloads the entire file via the non-streamingfileTransfer->download(request)before writing to the sink. This is the targeted fix for the binary cache path (covers bothhttps://ands3://on nix ≥2.33).2.
filetransfer.cc— The general-purpose streamingdownload(request, sink)path no longer callscurl_easy_pause(). The buffer grows unboundedly and the consumer drains concurrently. This benefits all callers of the streaming path, not just binary caches.Note: commit 1 makes commit 2 partially redundant for
HttpBinaryCacheStore(since it no longer uses the streaming path at all), but commit 2 fixes the behavior for any other code that callsdownload(request, sink).Review checklist
max-substitution-jobs = 128and a 2.9 GB NAR, worst-case peak is significant — verify runners have headroom (our runners have 123+ GB RAM).ItemHandle: Commit 2 discards the return value ofenqueueFileTransfersinceunpauseTransferis no longer called. VerifyItemHandledestruction has no side effects (it's a trivial wrapper aroundstd::reference_wrapper<Item>).downloadBufferSizesetting: Now effectively unused by the streaming path — the buffer is never checked against it. Should it be deprecated or removed in a follow-up?Add 👍 to pull requests you find important.
Link to Devin session: https://app.devin.ai/sessions/beb12ee3c3c94474ad193926cde267a9
Requested by: @jld-adriano