libstore: fix download deadlock on interruption

in a daemon all calls to the logger can throw an Interrupted exception,
which so far has silently stopped the curl thread without notifying its
transfers and leaving them stuck as a result. ensuring that the loggers
can never throw Interrupted will have very unpleasant side-effects, and
throwing depending on context requires large amount of bookkeeping. for
now it is easiest to abort all transfers on Interrupted during cleanup.

the test for this is extremely sketchy because we want to hit a single,
very specifically chosen, loger call in TransferItem::finish(). the bug
was triggered by the `act.progress` further down from what we're aiming
for, but that one is much harder to select for than the debug log here.

fixes #613

Change-Id: Id72efa64dd30cbbf256d2ab2a328457a0b095c6a

lix/libstore/filetransfer.cc

@@ -509,6 +509,14 @@ struct curlFileTransfer : public FileTransfer
 
         std::map<CURL *, std::shared_ptr<TransferItem>> items;
 
+        // clear all current transfers in case of an early exit, as can happen
+        // via Interrupted if the interruption occured right before a log call
+        KJ_DEFER({
+            for (auto & [_, item] : items) {
+                item->finish(CURLE_ABORTED_BY_CALLBACK);
+            }
+        });
+
         bool quit = false;
 
         // NOTE: we will need to use CURLMOPT_TIMERFUNCTION to integrate this
@@ -593,10 +601,15 @@ struct curlFileTransfer : public FileTransfer
         } catch (nix::Interrupted & e) {
         } catch (std::exception & e) {
             printError("unexpected error in download thread: %s", e.what());
+        } catch (...) {
+            printError("unexpected error in download thread");
         }
 
         {
             auto state(state_.lock());
+            for (auto & item : state->incoming) {
+                item->finish(CURLE_ABORTED_BY_CALLBACK);
+            }
             state->incoming.clear();
             state->quit = true;
         }

tests/unit/libstore/filetransfer.cc

@@ -1,11 +1,13 @@
 #include "lix/libstore/filetransfer.hh"
 #include "lix/libutil/compression.hh"
+#include "lix/libutil/signals.hh"
 #include "lix/libutil/thread-name.hh"
 
 #include <cstdint>
 #include <exception>
 #include <future>
 #include <gtest/gtest.h>
+#include <kj/common.h>
 #include <netinet/in.h>
 #include <string>
 #include <string_view>
@@ -395,4 +397,33 @@ TEST(FileTransfer, doesntRetryUploads)
     }
 }
 
+// this test does not work unless run alone. we can't fork because that breaks
+// the file transfer thread, restoring state is insufficient and very fragile.
+TEST(FileTransfer, DISABLED_interrupt)
+{
+    struct InterruptingLogger : Logger
+    {
+        void log(Verbosity lvl, std::string_view s) override
+        {
+            if (s.starts_with("finished") && s.ends_with("body = 10 bytes")) {
+                triggerInterrupt();
+                checkInterrupt();
+            }
+        }
+        void logEI(const ErrorInfo & ei) override
+        {
+        }
+    };
+
+    verbosity = lvlDebug;
+    logger = new InterruptingLogger;
+
+    auto ft = makeFileTransfer(0);
+    auto [port, srv] = serveHTTP({
+        {"200 ok", "content-length: 10\r\n", [] { return "0123456789"; }},
+    });
+
+    ASSERT_THROW(ft->download(fmt("http://[::1]:%d/index", port)).second->drain(), FileTransferError);
+}
+
 }