From d2a1e8de6893ed53fcc68155e13c8dfdd7e5dde5 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Thu, 12 Feb 2026 08:38:54 -0800 Subject: [PATCH 1/4] filetransfer: mark enqueueFileTransfer() as noexcept --- src/libstore/filetransfer.cc | 5 +++-- src/libstore/include/nix/store/filetransfer.hh | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 42cc2bc0e78d..a75a8ff2b7e8 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -965,7 +965,8 @@ struct curlFileTransfer : public FileTransfer return ItemHandle(static_cast(*item)); } - ItemHandle enqueueFileTransfer(const FileTransferRequest & request, Callback callback) override + ItemHandle + enqueueFileTransfer(const FileTransferRequest & request, Callback callback) noexcept override { /* Handle s3:// URIs by converting to HTTPS and optionally adding auth */ if (request.uri.scheme() == "s3") { @@ -1047,7 +1048,7 @@ void FileTransferRequest::setupForS3() #endif } -std::future FileTransfer::enqueueFileTransfer(const FileTransferRequest & request) +std::future FileTransfer::enqueueFileTransfer(const FileTransferRequest & request) noexcept { auto promise = std::make_shared>(); enqueueFileTransfer(request, {[promise](std::future fut) { diff --git a/src/libstore/include/nix/store/filetransfer.hh b/src/libstore/include/nix/store/filetransfer.hh index fa8a649e2b36..ca5d2a9f1cd5 100644 --- a/src/libstore/include/nix/store/filetransfer.hh +++ b/src/libstore/include/nix/store/filetransfer.hh @@ -279,14 +279,14 @@ public: * exception. */ virtual ItemHandle - enqueueFileTransfer(const FileTransferRequest & request, Callback callback) = 0; + enqueueFileTransfer(const FileTransferRequest & request, Callback callback) noexcept = 0; /** * Unpause a transfer that has been previously paused by a dataCallback. */ virtual void unpauseTransfer(ItemHandle handle) = 0; - std::future enqueueFileTransfer(const FileTransferRequest & request); + std::future enqueueFileTransfer(const FileTransferRequest & request) noexcept; /** * Synchronously download a file. From b8d886936ff69c635cd756a0533a5db1688eb732 Mon Sep 17 00:00:00 2001 From: Sergei Zimmerman Date: Mon, 30 Mar 2026 23:46:35 +0300 Subject: [PATCH 2/4] libstore: Use std::weak_ptr in ItemHandle I noticed cole-h came across this issue in detnix. Silly mistake on my part, the TransferItem can die by the time we might want to unpause it. Haven't seen this fail in in the wild, but the weak_ptr approach is the correct one. The enqueueing thread mustn't take shared ownership. Same for enqueueing for wakeup. Only the worker thread must have the ownership of the TransferItem. --- src/libstore/filetransfer.cc | 21 +++++++++++++------ .../include/nix/store/filetransfer.hh | 4 ++-- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index a75a8ff2b7e8..2e040d639d02 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -714,7 +714,7 @@ struct curlFileTransfer : public FileTransfer }; std::priority_queue, std::vector>, EmbargoComparator> incoming; - std::vector> unpause; + std::vector> unpause; private: bool quitting = false; public: @@ -919,8 +919,15 @@ struct curlFileTransfer : public FileTransfer return res; }(); - for (auto & item : unpause) - item->unpause(); + for (auto & item : unpause) { + /* The transfer might have completed (failed) between it getting + enqueued for unpause and by the time the worker thread picked + it up. */ + auto ptr = item.lock(); + if (!ptr) + continue; + static_cast(*ptr).unpause(); + } } debug("download thread shutting down"); @@ -962,7 +969,7 @@ struct curlFileTransfer : public FileTransfer writeFull(wakeupPipe.writeSide.get(), " "); #endif - return ItemHandle(static_cast(*item)); + return ItemHandle(item.get_ptr()); } ItemHandle @@ -978,7 +985,7 @@ struct curlFileTransfer : public FileTransfer return enqueueItem(make_ref(*this, request, std::move(callback))); } - void unpauseTransfer(ref item) + void unpauseTransfer(std::weak_ptr item) { auto state(state_.lock()); state->unpause.push_back(std::move(item)); @@ -989,7 +996,9 @@ struct curlFileTransfer : public FileTransfer void unpauseTransfer(ItemHandle handle) override { - unpauseTransfer(ref{static_cast(handle.item.get()).shared_from_this()}); + /* The transfer might have completed (more likely failed) when we want + to wake it up. That's why we must use a weak_ptr throughout. */ + unpauseTransfer(handle.item); } }; diff --git a/src/libstore/include/nix/store/filetransfer.hh b/src/libstore/include/nix/store/filetransfer.hh index ca5d2a9f1cd5..58f83ab5d8ec 100644 --- a/src/libstore/include/nix/store/filetransfer.hh +++ b/src/libstore/include/nix/store/filetransfer.hh @@ -262,10 +262,10 @@ public: */ struct ItemHandle { - std::reference_wrapper item; + std::weak_ptr item; friend struct FileTransfer; - ItemHandle(Item & item) + explicit ItemHandle(std::weak_ptr item) : item(item) { } From 44e2e32d6bd82363a0700bfcc1683829dcf58729 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Thu, 12 Feb 2026 08:53:04 -0800 Subject: [PATCH 3/4] filetransfer: handle exceptions thrown from enqueueItem --- src/libstore/filetransfer.cc | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 2e040d639d02..6b9ea2381cfe 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -979,10 +979,22 @@ struct curlFileTransfer : public FileTransfer if (request.uri.scheme() == "s3") { auto modifiedRequest = request; modifiedRequest.setupForS3(); - return enqueueItem(make_ref(*this, std::move(modifiedRequest), std::move(callback))); + auto item = make_ref(*this, std::move(modifiedRequest), std::move(callback)); + try { + return enqueueItem(item); + } catch (const nix::Error & e) { + item->fail(e); + return ItemHandle(item.get_ptr()); + } } - return enqueueItem(make_ref(*this, request, std::move(callback))); + auto item = make_ref(*this, request, std::move(callback)); + try { + return enqueueItem(item); + } catch (const nix::Error & e) { + item->fail(e); + return ItemHandle(item.get_ptr()); + } } void unpauseTransfer(std::weak_ptr item) From b69b84e292ce60542dd03959cfa0f3addaa21a31 Mon Sep 17 00:00:00 2001 From: Cole Helbling Date: Mon, 30 Mar 2026 13:01:54 -0700 Subject: [PATCH 4/4] fixup: catch BaseError instead of Error That way it catches both nix::Error as well as nix::Interrupted. --- src/libstore/filetransfer.cc | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc index 6b9ea2381cfe..51a26819ab3a 100644 --- a/src/libstore/filetransfer.cc +++ b/src/libstore/filetransfer.cc @@ -982,7 +982,10 @@ struct curlFileTransfer : public FileTransfer auto item = make_ref(*this, std::move(modifiedRequest), std::move(callback)); try { return enqueueItem(item); - } catch (const nix::Error & e) { + } catch (const nix::BaseError & e) { + // NOTE(cole-h): catches both nix::Error and nix::Interrupted -- enqueueItem calls + // writeFull which may throw nix::Interrupted, and the rest of enqueueItem may throw + // nix::Error item->fail(e); return ItemHandle(item.get_ptr()); } @@ -991,7 +994,10 @@ struct curlFileTransfer : public FileTransfer auto item = make_ref(*this, request, std::move(callback)); try { return enqueueItem(item); - } catch (const nix::Error & e) { + } catch (const nix::BaseError & e) { + // NOTE(cole-h): catches both nix::Error and nix::Interrupted -- enqueueItem calls + // writeFull which may throw nix::Interrupted, and the rest of enqueueItem may throw + // nix::Error item->fail(e); return ItemHandle(item.get_ptr()); }