From 0fb67468fb6946fd0de1e41b541b43360c0c2b0a Mon Sep 17 00:00:00 2001 From: "adriano@exa.ai" Date: Fri, 14 Aug 2026 22:02:12 +0000 Subject: [PATCH 1/2] libstore: fix lost file transfer wakeups and pause-state race MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Port upstream NixOS/nix commit 0d0c3335045 ("libstore: Fix libcurl thread wakeup with curl >= 8.21"): track pending work in State::work under the state lock and skip the curl_multi_poll() sleep when work was queued, so a curl_multi_wakeup() consumed inside curl_multi_perform() can no longer leave the worker sleeping (up to 10s, or indefinitely if no further wakeups arrive) with an enqueued transfer or unpause request. The worker now processes incoming/unpause requests before polling instead of after. Also fix TransferItem::unpause() to clear `paused` before calling curl_easy_pause(CURLPAUSE_CONT): the CONT call synchronously flushes buffered data through the write callback, which can re-pause the transfer and set `paused = true`; the old order then overwrote it with `false`, leaving a transfer paused in curl while nix believes it is unpaused — a later unpause request becomes a no-op and the transfer never completes, matching the silent substitution hangs seen on the CI runners. Assisted-by: Devin GPT-5.2 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- src/libstore/filetransfer.cc | 67 +++++++++++++++++++++--------------- 1 file changed, 39 insertions(+), 28 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 22762333cb99..aceebfe992a6 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -29,6 +29,7 @@ #include #include #include +#include namespace nix { @@ -485,8 +486,8 @@ struct curlFileTransfer : public FileTransfer { /* Unpausing an already unpaused transfer is a no-op. */ if (paused) { - curl_easy_pause(req, CURLPAUSE_CONT); paused = false; + curl_easy_pause(req, CURLPAUSE_CONT); } } @@ -864,6 +865,8 @@ struct curlFileTransfer : public FileTransfer private: bool quitting = false; public: + bool work = false; + void quit() { quitting = true; @@ -921,12 +924,14 @@ struct curlFileTransfer : public FileTransfer void stopWorkerThread() { /* Signal the worker thread to exit. */ - state_.lock()->quit(); - wakeupMulti(); + auto state(state_.lock()); + state->quit(); + wakeupMulti(*state); } - void wakeupMulti() + void wakeupMulti(State & state) { + state.work = true; if (auto ec = ::curl_multi_wakeup(curlm.get())) throw curlMultiError(ec); } @@ -981,25 +986,12 @@ struct curlFileTransfer : public FileTransfer } } - /* Wait for activity, including wakeup events. */ - long maxSleepTimeMs = items.empty() ? 10000 : 100; - auto sleepTimeMs = nextWakeup != std::chrono::steady_clock::time_point() - ? std::max( - 0, - (int) std::chrono::duration_cast( - nextWakeup - std::chrono::steady_clock::now()) - .count()) - : maxSleepTimeMs; - - int numfds = 0; - mc = curl_multi_poll(curlm.get(), nullptr, 0, sleepTimeMs, &numfds); - if (mc != CURLM_OK) - throw curlMultiError(mc); - nextWakeup = std::chrono::steady_clock::time_point(); std::vector> incoming; + std::vector> unpause; auto now = std::chrono::steady_clock::now(); + bool haveWork; { auto state(state_.lock()); @@ -1021,7 +1013,9 @@ struct curlFileTransfer : public FileTransfer break; } } + unpause = std::exchange(state->unpause, {}); quit = state->isQuitting(); + haveWork = std::exchange(state->work, false); } for (auto & item : incoming) { @@ -1032,14 +1026,10 @@ struct curlFileTransfer : public FileTransfer items[item->req] = item; } - /* NOTE: Unpausing may invoke callbacks to flush all buffers. */ - auto unpause = [&]() { - auto state(state_.lock()); - auto res = state->unpause; - state->unpause.clear(); - return res; - }(); + if (quit) + break; + /* NOTE: Unpausing may invoke callbacks to flush all buffers. */ for (auto & item : unpause) { /* The transfer might have completed (failed) between it getting enqueued for unpause and by the time the worker thread picked @@ -1049,6 +1039,27 @@ struct curlFileTransfer : public FileTransfer continue; static_cast(*ptr).unpause(); } + + /* Wait for activity, including wakeup events. */ + long maxSleepTimeMs = items.empty() ? 10000 : 100; + auto sleepTimeMs = nextWakeup != std::chrono::steady_clock::time_point() + ? std::max( + 0, + (int) std::chrono::duration_cast( + nextWakeup - std::chrono::steady_clock::now()) + .count()) + : maxSleepTimeMs; + + /* Since https://github.com/curl/curl/commit/2a2104f3cff44bb28bb570a093be52bbeeed8f23 (8.21), + curl_multi_perform seems to swallow queued up events, so a wakeup sent while we were in + curl_multi_perform() would otherwise be lost and we'd sleep despite having work queued. */ + if (haveWork) + sleepTimeMs = 0; + + int numfds = 0; + mc = curl_multi_poll(curlm.get(), nullptr, 0, sleepTimeMs, &numfds); + if (mc != CURLM_OK) + throw curlMultiError(mc); } debug("download thread shutting down"); @@ -1085,9 +1096,9 @@ struct curlFileTransfer : public FileTransfer throw nix::Error("cannot enqueue download request because the download thread is shutting down"); state->incoming.push(item); item->enqueued = true; /* Now any exceptions should be reported via the callback. */ + wakeupMulti(*state); } - wakeupMulti(); return ItemHandle(item.get_ptr()); } @@ -1124,7 +1135,7 @@ struct curlFileTransfer : public FileTransfer { auto state(state_.lock()); state->unpause.push_back(std::move(item)); - wakeupMulti(); + wakeupMulti(*state); } void unpauseTransfer(ItemHandle handle) override From e5a675f7f2b68d4256f433962e5408b8011fb0c8 Mon Sep 17 00:00:00 2001 From: "adriano@exa.ai" Date: Sat, 15 Aug 2026 16:13:16 +0000 Subject: [PATCH 2/2] libstore: self-heal lost transfer unpauses A transfer paused in curl never triggers the stalled-download timeout, so a single lost unpause (whether from pause-state bookkeeping races or a missed worker wakeup) hangs the download, and any substitution waiting on it, forever. Make recovery not depend on any single unpause surviving: - download(): while the transfer may be paused, wait with a 1s bound and re-request the unpause until data flows again (dataCallback clears the flag when it delivers below-buffer-size data). Unpausing an unpaused transfer is a no-op, so retries are safe. - TransferItem::unpause(): issue CURLPAUSE_CONT unconditionally for active transfers instead of gating on our own pause flag, so a flag mismatch cannot suppress the unpause. Verified with a fault-injection harness (LD_PRELOAD shim dropping curl_easy_pause(CONT) calls): without this, one dropped unpause hangs the download indefinitely with the worker idle in curl_multi_poll and the consumer blocked on avail (matching the production CI hang); with it, downloads complete despite dropped unpauses. Assisted-by: Devin:claude-opus-4-6 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- src/libstore/filetransfer.cc | 27 +++++++++++++++++++++------ 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index aceebfe992a6..cfc0ada36768 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -484,8 +484,12 @@ struct curlFileTransfer : public FileTransfer void unpause() { - /* Unpausing an already unpaused transfer is a no-op. */ - if (paused) { + /* Unconditionally tell curl to continue: unpausing an already + unpaused transfer is a no-op, and gating this on our own pause + bookkeeping risks leaving the transfer paused in curl forever + if the two ever disagree (a paused transfer never triggers the + stalled-download timeout). */ + if (active) { paused = false; curl_easy_pause(req, CURLPAUSE_CONT); } @@ -1276,8 +1280,12 @@ void FileTransfer::download( state->data.append(data); state->avail.notify_one(); - if (state->data.size() <= fileTransferSettings.downloadBufferSize) + if (state->data.size() <= fileTransferSettings.downloadBufferSize) { + /* Data is flowing again, so any previously requested pause is + no longer in effect. */ + state->paused = false; return PauseTransfer::No; + } /* dataCallback gets called multiple times by an intermediate sink. Only issue the debug message the first time around. */ @@ -1330,10 +1338,17 @@ void FileTransfer::download( } if (state->paused) { + /* Keep requesting an unpause until data flows again + (dataCallback clears the flag): an unpause can be lost + to races in the pause bookkeeping, and a transfer that + stays paused never triggers the stalled-download + timeout, hanging the download forever. Repeating the + request is safe since unpausing an unpaused transfer + is a no-op. */ unpauseTransfer(handle); - state->paused = false; - } - state.wait(state->avail); + state.wait_for(state->avail, std::chrono::seconds(1)); + } else + state.wait(state->avail); if (state->data.empty()) continue;