source tree service: Fix ensuring Git cache root

This is an amendment to the changes in commit 8234079, as the
underlying issue was only partially solved there. While the call to
GitRepo::InitAndOpen is in itself properly guarded, it does not
share a lock with the call to create the path to the Git cache if
it is missing.

Fix this by moving the call to the method ensuring the Git cache
initialization to after acquiring the Git cache root garbage
collector shared lock.

1 files changed, 50 insertions, 58 deletions

diff --git a/src/buildtool/serve_api/serve_service/source_tree.cpp b/src/buildtool/serve_api/serve_service/source_tree.cpp
index 3e0d3bbe..f9725412 100644
--- a/src/buildtool/serve_api/serve_service/source_tree.cpp
+++ b/src/buildtool/serve_api/serve_service/source_tree.cpp
@@ -231,12 +231,6 @@ auto SourceTreeService::ServeCommitTree(
     ::grpc::ServerContext* /* context */,
     const ::justbuild::just_serve::ServeCommitTreeRequest* request,
     ServeCommitTreeResponse* response) -> ::grpc::Status {
-    // ensure Git cache exists
-    if (auto done = EnsureGitCacheRoot(); not done) {
-        logger_->Emit(LogLevel::Error, std::move(done).error());
-        response->set_status(ServeCommitTreeResponse::INTERNAL_ERROR);
-        return ::grpc::Status::OK;
-    }
     // get lock for Git cache
     auto repo_lock = RepositoryGarbageCollector::SharedLock(
         *native_context_->storage_config);
@@ -245,6 +239,12 @@ auto SourceTreeService::ServeCommitTree(
         response->set_status(ServeCommitTreeResponse::INTERNAL_ERROR);
         return ::grpc::Status::OK;
     }
+    // ensure Git cache exists
+    if (auto done = EnsureGitCacheRoot(); not done) {
+        logger_->Emit(LogLevel::Error, std::move(done).error());
+        response->set_status(ServeCommitTreeResponse::INTERNAL_ERROR);
+        return ::grpc::Status::OK;
+    }
 
     auto const& commit{request->commit()};
     auto const& subdir{request->subdir()};
@@ -837,12 +837,6 @@ auto SourceTreeService::ServeArchiveTree(
     ::grpc::ServerContext* /* context */,
     const ::justbuild::just_serve::ServeArchiveTreeRequest* request,
     ServeArchiveTreeResponse* response) -> ::grpc::Status {
-    // ensure Git cache exists
-    if (auto done = EnsureGitCacheRoot(); not done) {
-        logger_->Emit(LogLevel::Error, std::move(done).error());
-        response->set_status(ServeArchiveTreeResponse::INTERNAL_ERROR);
-        return ::grpc::Status::OK;
-    }
     // get gc lock for Git cache
     auto repo_lock = RepositoryGarbageCollector::SharedLock(
         *native_context_->storage_config);
@@ -851,6 +845,12 @@ auto SourceTreeService::ServeArchiveTree(
         response->set_status(ServeArchiveTreeResponse::INTERNAL_ERROR);
         return ::grpc::Status::OK;
     }
+    // ensure Git cache exists
+    if (auto done = EnsureGitCacheRoot(); not done) {
+        logger_->Emit(LogLevel::Error, std::move(done).error());
+        response->set_status(ServeArchiveTreeResponse::INTERNAL_ERROR);
+        return ::grpc::Status::OK;
+    }
 
     auto const& content{request->content()};
     auto archive_type = ArchiveTypeToString(request->archive_type());
@@ -1128,12 +1128,6 @@ auto SourceTreeService::ServeDistdirTree(
     ::grpc::ServerContext* /* context */,
     const ::justbuild::just_serve::ServeDistdirTreeRequest* request,
     ServeDistdirTreeResponse* response) -> ::grpc::Status {
-    // ensure Git cache exists
-    if (auto done = EnsureGitCacheRoot(); not done) {
-        logger_->Emit(LogLevel::Error, std::move(done).error());
-        response->set_status(ServeDistdirTreeResponse::INTERNAL_ERROR);
-        return ::grpc::Status::OK;
-    }
     // get gc lock for Git cache
     auto repo_lock = RepositoryGarbageCollector::SharedLock(
         *native_context_->storage_config);
@@ -1142,6 +1136,12 @@ auto SourceTreeService::ServeDistdirTree(
         response->set_status(ServeDistdirTreeResponse::INTERNAL_ERROR);
         return ::grpc::Status::OK;
     }
+    // ensure Git cache exists
+    if (auto done = EnsureGitCacheRoot(); not done) {
+        logger_->Emit(LogLevel::Error, std::move(done).error());
+        response->set_status(ServeDistdirTreeResponse::INTERNAL_ERROR);
+        return ::grpc::Status::OK;
+    }
     // acquire lock for native CAS
     auto lock = GarbageCollector::SharedLock(*native_context_->storage_config);
     if (not lock) {
@@ -1388,15 +1388,7 @@ auto SourceTreeService::ServeContent(
     const ::justbuild::just_serve::ServeContentRequest* request,
     ServeContentResponse* response) -> ::grpc::Status {
     auto const& content{request->content()};
-
-    // ensure Git cache exists
-    if (auto done = EnsureGitCacheRoot(); not done) {
-        logger_->Emit(LogLevel::Error, std::move(done).error());
-        response->set_status(ServeContentResponse::INTERNAL_ERROR);
-        return ::grpc::Status::OK;
-    }
-
-    // acquire locks
+    // get gc lock for Git cache
     auto repo_lock = RepositoryGarbageCollector::SharedLock(
         *native_context_->storage_config);
     if (not repo_lock) {
@@ -1404,7 +1396,13 @@ auto SourceTreeService::ServeContent(
         response->set_status(ServeContentResponse::INTERNAL_ERROR);
         return ::grpc::Status::OK;
     }
-
+    // ensure Git cache exists
+    if (auto done = EnsureGitCacheRoot(); not done) {
+        logger_->Emit(LogLevel::Error, std::move(done).error());
+        response->set_status(ServeContentResponse::INTERNAL_ERROR);
+        return ::grpc::Status::OK;
+    }
+    // get gc lock for native storage
     auto lock = GarbageCollector::SharedLock(*native_context_->storage_config);
     if (not lock) {
         logger_->Emit(LogLevel::Error, "Could not acquire gc SharedLock");
@@ -1497,15 +1495,7 @@ auto SourceTreeService::ServeTree(
     const ::justbuild::just_serve::ServeTreeRequest* request,
     ServeTreeResponse* response) -> ::grpc::Status {
     auto const& tree_id{request->tree()};
-
-    // ensure Git cache exists
-    if (auto done = EnsureGitCacheRoot(); not done) {
-        logger_->Emit(LogLevel::Error, std::move(done).error());
-        response->set_status(ServeTreeResponse::INTERNAL_ERROR);
-        return ::grpc::Status::OK;
-    }
-
-    // acquire locks
+    // get gc lock for Git cache
     auto repo_lock = RepositoryGarbageCollector::SharedLock(
         *native_context_->storage_config);
     if (not repo_lock) {
@@ -1513,7 +1503,13 @@ auto SourceTreeService::ServeTree(
         response->set_status(ServeTreeResponse::INTERNAL_ERROR);
         return ::grpc::Status::OK;
     }
-
+    // ensure Git cache exists
+    if (auto done = EnsureGitCacheRoot(); not done) {
+        logger_->Emit(LogLevel::Error, std::move(done).error());
+        response->set_status(ServeTreeResponse::INTERNAL_ERROR);
+        return ::grpc::Status::OK;
+    }
+    // get gc lock for native storage
     auto lock = GarbageCollector::SharedLock(*native_context_->storage_config);
     if (not lock) {
         logger_->Emit(LogLevel::Error, "Could not acquire gc SharedLock");
@@ -1606,15 +1602,7 @@ auto SourceTreeService::CheckRootTree(
     const ::justbuild::just_serve::CheckRootTreeRequest* request,
     CheckRootTreeResponse* response) -> ::grpc::Status {
     auto const& tree_id{request->tree()};
-
-    // ensure Git cache exists
-    if (auto done = EnsureGitCacheRoot(); not done) {
-        logger_->Emit(LogLevel::Error, std::move(done).error());
-        response->set_status(CheckRootTreeResponse::INTERNAL_ERROR);
-        return ::grpc::Status::OK;
-    }
-
-    // acquire locks
+    // get gc lock for Git cache
     auto repo_lock = RepositoryGarbageCollector::SharedLock(
         *native_context_->storage_config);
     if (not repo_lock) {
@@ -1622,7 +1610,13 @@ auto SourceTreeService::CheckRootTree(
         response->set_status(CheckRootTreeResponse::INTERNAL_ERROR);
         return ::grpc::Status::OK;
     }
-
+    // ensure Git cache exists
+    if (auto done = EnsureGitCacheRoot(); not done) {
+        logger_->Emit(LogLevel::Error, std::move(done).error());
+        response->set_status(CheckRootTreeResponse::INTERNAL_ERROR);
+        return ::grpc::Status::OK;
+    }
+    // get gc lock for native storage
     auto lock = GarbageCollector::SharedLock(*native_context_->storage_config);
     if (not lock) {
         logger_->Emit(LogLevel::Error, "Could not acquire gc SharedLock");
@@ -1734,15 +1728,7 @@ auto SourceTreeService::GetRemoteTree(
     ::grpc::ServerContext* /* context */,
     const ::justbuild::just_serve::GetRemoteTreeRequest* request,
     GetRemoteTreeResponse* response) -> ::grpc::Status {
-
-    // ensure Git cache exists
-    if (auto done = EnsureGitCacheRoot(); not done) {
-        logger_->Emit(LogLevel::Error, std::move(done).error());
-        response->set_status(GetRemoteTreeResponse::INTERNAL_ERROR);
-        return ::grpc::Status::OK;
-    }
-
-    // acquire locks
+    // get gc lock for Git cache
     auto repo_lock = RepositoryGarbageCollector::SharedLock(
         *native_context_->storage_config);
     if (not repo_lock) {
@@ -1750,7 +1736,13 @@ auto SourceTreeService::GetRemoteTree(
         response->set_status(GetRemoteTreeResponse::INTERNAL_ERROR);
         return ::grpc::Status::OK;
     }
-
+    // ensure Git cache exists
+    if (auto done = EnsureGitCacheRoot(); not done) {
+        logger_->Emit(LogLevel::Error, std::move(done).error());
+        response->set_status(GetRemoteTreeResponse::INTERNAL_ERROR);
+        return ::grpc::Status::OK;
+    }
+    // get gc lock for native storage
     auto lock = GarbageCollector::SharedLock(*native_context_->storage_config);
     if (not lock) {
         logger_->Emit(LogLevel::Error, "Could not acquire gc SharedLock");