From 66a9fbb7ffa781f46e246de08700739fa775650c Mon Sep 17 00:00:00 2001
From: Jade Lovelace <lix@jade.fyi>
Date: Sat, 15 Jun 2024 13:10:03 -0700
Subject: [PATCH] libstore: fix queryValidPaths concurrency

The lock usage was obviously wrong so it was entirely serialized. This
has the predicted speedups, the only question is whether it is sound
because it's exposing a bunch of new code to actual concurrency.

I did audit all the stores' queryPathInfoUncached implementations and
they all look *intended* to be thread safe, but whether that is actually
sound or not: lol lmao. I am highly confident in the s3 one because it
is calling s3 sdk methods that are thread safe and has no actual state.

Others are using Pool and look to be *supposed* to be thread safe, but
unsure if they actually are.

Change-Id: I0369152a510e878b5ac56c9ac956a98d48cd5fef
---
 src/libstore/store-api.cc | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/src/libstore/store-api.cc b/src/libstore/store-api.cc
index 55083e834..4e1fb4bd5 100644
--- a/src/libstore/store-api.cc
+++ b/src/libstore/store-api.cc
@@ -803,17 +803,31 @@ StorePathSet Store::queryValidPaths(const StorePathSet & paths, SubstituteFlag m
 
     auto doQuery = [&](const StorePath & path) {
         checkInterrupt();
-        auto state(state_.lock());
+
+        bool exists = false;
+        std::exception_ptr newExc{};
+
         try {
-            auto info = queryPathInfo(path);
-            state->valid.insert(path);
+            queryPathInfo(path);
+            exists = true;
         } catch (InvalidPath &) {
         } catch (...) {
-            state->exc = std::current_exception();
+            newExc = std::current_exception();
+        }
+
+        {
+            auto state(state_.lock());
+
+            if (exists) {
+                state->valid.insert(path);
+            }
+            if (newExc != nullptr) {
+                state->exc = newExc;
+            }
+            assert(state->left);
+            if (!--state->left)
+                wakeup.notify_one();
         }
-        assert(state->left);
-        if (!--state->left)
-            wakeup.notify_one();
     };
 
     for (auto & path : paths)