libstore: eagerly mark daemon connections as bad on local errors

do not rely on Source/Sink `good()` or delayed guessing about whether
an exception was thrown by the daemon or not. mark connections as bad
for all local errors happening while communication is ongoing instead,
and leave it valid only when an exception was provided by the remote.

we may drop connections a bit too eagerly now, but all cases in which
that happens were vulnerable to protocol desynchronization. there are
still a few windows for this to happen left, but those are unfixable.

Change-Id: Iefaa66c552092c436b9de77aa3f8e09f847a966e

lix/libstore/remote-store-connection.hh

@@ -120,7 +120,6 @@ struct RemoteStore::ConnectionHandle
 {
     Pool<RemoteStore::Connection>::Handle handle;
     Sync<ThreadPool> & handlerThreads;
-    bool daemonException = false;
 
     ConnectionHandle(
         Pool<RemoteStore::Connection>::Handle && handle, Sync<ThreadPool> & handlerThreads
@@ -133,13 +132,9 @@ struct RemoteStore::ConnectionHandle
     ConnectionHandle(ConnectionHandle && h)
         : handle(std::move(h.handle))
         , handlerThreads(h.handlerThreads)
-        , daemonException(h.daemonException)
     {
-        h.daemonException = false;
     }
 
-    ~ConnectionHandle();
-
     RemoteStore::Connection & operator * () { return *handle; }
     RemoteStore::Connection * operator -> () { return &*handle; }
 
@@ -161,23 +156,33 @@ struct RemoteStore::ConnectionHandle
         // and serialize all *preceding* arguments normally before handing over to
         // the subframing layer (which is then responsible for any error handling)
         if constexpr (requires { *handle->to << std::declval<LastArgT>(); }) {
-            StringSink msg;
-            ((msg << std::forward<Args>(args)), ...);
-            StringSource{msg.s}.drainInto(*handle->to);
-            handle->to->flush();
+            try {
+                StringSink msg;
+                ((msg << std::forward<Args>(args)), ...);
+                StringSource{msg.s}.drainInto(*handle->to);
+                handle->to->flush();
+            } catch (...) {
+                handle.markBad();
+                throw;
+            }
             LIX_TRY_AWAIT(processStderr());
         } else {
             using ImmediateArgsIdxs = std::make_index_sequence<sizeof...(Args) - 1>;
             AllArgsT allArgs(std::forward<Args>(args)...);
 
             [&]<size_t... Ids>(std::integer_sequence<size_t, Ids...>) {
-                StringSink msg;
-                ((msg << std::forward<std::tuple_element_t<Ids, AllArgsT>>(
-                      std::get<Ids>(allArgs)
-                  )),
-                 ...);
-                StringSource{msg.s}.drainInto(*handle->to);
-                handle->to->flush();
+                try {
+                    StringSink msg;
+                    ((msg << std::forward<std::tuple_element_t<Ids, AllArgsT>>(
+                        std::get<Ids>(allArgs)
+                    )),
+                    ...);
+                    StringSource{msg.s}.drainInto(*handle->to);
+                    handle->to->flush();
+                } catch (...) {
+                    handle.markBad();
+                    throw;
+                }
             }(ImmediateArgsIdxs{});
 
             LIX_TRY_AWAIT(withFramedSinkAsync(std::get<LastArgIdx>(allArgs)));
@@ -186,7 +191,12 @@ struct RemoteStore::ConnectionHandle
         if constexpr (std::is_void_v<R>) {
             co_return result::success();
         } else {
-            co_return WorkerProto::Serialise<R>::read(*handle);
+            try {
+                co_return WorkerProto::Serialise<R>::read(*handle);
+            } catch (...) {
+                handle.markBad();
+                throw;
+            }
         }
     } catch (...) {
         co_return result::current_exception();

lix/libstore/remote-store.cc

@@ -38,8 +38,7 @@ RemoteStore::RemoteStore(const RemoteStoreConfig & config)
           std::max(1, (int) config.maxConnections),
           [this]() { return openAndInitConnection(); },
           [this](const ref<Connection> & r) {
-              return r->to->good() && r->from->good()
-                  && std::chrono::duration_cast<std::chrono::seconds>(
+              return std::chrono::duration_cast<std::chrono::seconds>(
                          std::chrono::steady_clock::now() - r->startTime
                      )
                          .count()
@@ -169,23 +168,15 @@ try {
     co_return result::current_exception();
 }
 
-RemoteStore::ConnectionHandle::~ConnectionHandle()
-{
-    if (!daemonException && std::uncaught_exceptions()) {
-        handle.markBad();
-        debug("closing daemon connection because of an exception");
-    }
-}
-
 kj::Promise<Result<void>> RemoteStore::ConnectionHandle::processStderr()
 try {
     auto ex = TRY_AWAIT(handle->processStderr());
     if (ex.e) {
-        daemonException = true;
-        std::rethrow_exception(ex.e);
+        co_return result::failure(ex.e);
     }
     co_return result::success();
 } catch (...) {
+    handle.markBad();
     co_return result::current_exception();
 }
 
@@ -727,7 +718,12 @@ try {
     auto conn(TRY_AWAIT(getConnection()));
     TRY_AWAIT(conn.sendCommand(WorkerProto::Op::NarFromPath, printStorePath(path)));
     co_return make_box_ptr<AsyncGeneratorInputStream>([](auto conn) -> WireFormatGenerator {
-        co_yield copyNAR(*conn->from);
+        try {
+            co_yield copyNAR(*conn->from);
+        } catch (...) {
+            conn.handle.markBad();
+            throw;
+        }
     }(std::move(conn)));
 } catch (...) {
     co_return result::current_exception();
@@ -828,7 +824,7 @@ RemoteStore::ConnectionHandle::FramedSinkHandler::~FramedSinkHandler() noexcept(
     // handle multiple exceptions anyway the safest path is to simply drop
     // the remote (possibly Interrupted) exception when called for unwind.
     if (ex && std::uncaught_exceptions() == 0) {
-        std::rethrow_exception(ex);
+        throw FramedSink::RemoteError(ex);
     }
 }
 
@@ -843,8 +839,10 @@ try {
         sink.flush();
     }
     co_return result::success();
+} catch (FramedSink::RemoteError & e) {
+    co_return result::failure(e.e);
 } catch (...) {
+    handle.markBad();
     co_return result::current_exception();
 }
-
 }

lix/libutil/serialise.hh

@@ -1,8 +1,10 @@
 #pragma once
 ///@file
 
+#include <exception>
 #include <memory>
 
+#include "error.hh"
 #include "lix/libutil/charptr-cast.hh"
 #include "lix/libutil/generator.hh"
 #include "lix/libutil/io-buffer.hh"
@@ -559,6 +561,16 @@ struct FramedSink : nix::BufferedSink
     BufferedSink & to;
     std::exception_ptr & ex;
 
+    struct RemoteError : BaseException
+    {
+        std::exception_ptr e;
+
+        RemoteError(std::exception_ptr e)
+            : e(e) // NOLINT(bugprone-throw-keyword-missing): intentional copy
+        {
+        }
+    };
+
     FramedSink(BufferedSink & to, std::exception_ptr & ex) : to(to), ex(ex)
     { }
 
@@ -579,7 +591,7 @@ struct FramedSink : nix::BufferedSink
         if (ex) {
             auto ex2 = ex;
             ex = nullptr;
-            std::rethrow_exception(ex2);
+            throw RemoteError{ex2};
         }
         to << data.size();
         to(data);