From 8c1ece93cc21b7c2fff4031b4de7a40d919bf4eb Mon Sep 17 00:00:00 2001
From: eldritch horrors <pennae@lix.systems>
Date: Sun, 5 Jan 2025 01:19:12 +0100
Subject: [PATCH] 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        | 13 ++++++++++++
 tests/unit/libstore/filetransfer.cc | 31 +++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/lix/libstore/filetransfer.cc b/lix/libstore/filetransfer.cc
index d960f6878..511244479 100644
--- a/lix/libstore/filetransfer.cc
+++ b/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;
         }
diff --git a/tests/unit/libstore/filetransfer.cc b/tests/unit/libstore/filetransfer.cc
index e998aaaf7..52369ee11 100644
--- a/tests/unit/libstore/filetransfer.cc
+++ b/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);
+}
+
 }