From 6b081389290a5a6a6d143ee7fa4d12ea1296a8d0 Mon Sep 17 00:00:00 2001
From: eldritch horrors <pennae@lix.systems>
Date: Sat, 4 May 2024 01:39:06 +0200
Subject: [PATCH] filetransfer: abort transfer on receiver exception

not doing this will cause transfers that had their readers disappear to
linger. with lingering transfers the curl thread can't shut down, which
will cause nix itself to not shut down until the transfer finishes some
other way (most likely network timeouts). also add a new test for this.

Change-Id: Id2401b3ac85731c824db05918d4079125be25b57
---
 src/libstore/filetransfer.cc        | 16 ++++++++-------
 tests/unit/libstore/filetransfer.cc | 32 +++++++++++++++++++++++++++++
 tests/unit/meson.build              |  1 +
 3 files changed, 42 insertions(+), 7 deletions(-)
 create mode 100644 tests/unit/libstore/filetransfer.cc

diff --git a/src/libstore/filetransfer.cc b/src/libstore/filetransfer.cc
index c9f5d6260..aa8f4be1d 100644
--- a/src/libstore/filetransfer.cc
+++ b/src/libstore/filetransfer.cc
@@ -708,7 +708,7 @@ struct curlFileTransfer : public FileTransfer
            download thread and the calling thread. */
 
         struct State {
-            bool quit = false;
+            bool done = false, failed = false;
             std::exception_ptr exc;
             std::string data;
             std::condition_variable avail, request;
@@ -717,18 +717,17 @@ struct curlFileTransfer : public FileTransfer
 
         auto _state = std::make_shared<Sync<State>>();
 
-        /* In case of an exception, wake up the download thread. FIXME:
-           abort the download request. */
+        /* In case of an exception, wake up the download thread. */
         Finally finally([&]() {
             auto state(_state->lock());
-            state->quit = true;
+            state->failed |= std::uncaught_exceptions() != 0;
             state->request.notify_one();
         });
 
         enqueueFileTransfer(request,
             {[_state](std::future<FileTransferResult> fut) {
                 auto state(_state->lock());
-                state->quit = true;
+                state->done = true;
                 try {
                     fut.get();
                 } catch (...) {
@@ -740,7 +739,10 @@ struct curlFileTransfer : public FileTransfer
             [_state, &sink](TransferItem & transfer, std::string_view data) {
                 auto state(_state->lock());
 
-                if (state->quit) return;
+                if (state->failed) {
+                    // actual exception doesn't matter, the other end is already dead
+                    throw std::exception{};
+                }
 
                 if (!state->decompressor) {
                     state->decompressor = makeDecompressionSink(transfer.encoding, sink);
@@ -775,7 +777,7 @@ struct curlFileTransfer : public FileTransfer
 
                 if (state->data.empty()) {
 
-                    if (state->quit) {
+                    if (state->done) {
                         if (state->exc) std::rethrow_exception(state->exc);
                         if (state->decompressor) {
                             state->decompressor->finish();
diff --git a/tests/unit/libstore/filetransfer.cc b/tests/unit/libstore/filetransfer.cc
new file mode 100644
index 000000000..192bf81ef
--- /dev/null
+++ b/tests/unit/libstore/filetransfer.cc
@@ -0,0 +1,32 @@
+#include "filetransfer.hh"
+
+#include <future>
+#include <gtest/gtest.h>
+
+using namespace std::chrono_literals;
+
+namespace nix {
+
+TEST(FileTransfer, exceptionAbortsDownload)
+{
+    struct Done
+    {};
+
+    auto ft = makeFileTransfer();
+
+    LambdaSink broken([](auto block) { throw Done(); });
+
+    ASSERT_THROW(ft->download(FileTransferRequest("file:///dev/zero"), broken), Done);
+
+    // makeFileTransfer returns a ref<>, which cannot be cleared. since we also
+    // can't default-construct it we'll have to overwrite it instead, but we'll
+    // take the raw pointer out first so we can destroy it in a detached thread
+    // (otherwise a failure will stall the process and have it killed by meson)
+    auto reset = std::async(std::launch::async, [&]() { ft = makeFileTransfer(); });
+    EXPECT_EQ(reset.wait_for(10s), std::future_status::ready);
+    // if this did time out we have to leak `reset`.
+    if (reset.wait_for(0s) == std::future_status::timeout) {
+        (void) new auto(std::move(reset));
+    }
+}
+}
diff --git a/tests/unit/meson.build b/tests/unit/meson.build
index 339ac9a4a..f5355cce8 100644
--- a/tests/unit/meson.build
+++ b/tests/unit/meson.build
@@ -113,6 +113,7 @@ libstore_tests_sources = files(
   'libstore/derivation.cc',
   'libstore/derived-path.cc',
   'libstore/downstream-placeholder.cc',
+  'libstore/filetransfer.cc',
   'libstore/machines.cc',
   'libstore/nar-info-disk-cache.cc',
   'libstore/outputs-spec.cc',