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.

(cherry-picked from 7d0a4df3822ca194d93abb6b65d0ebb264cc1974)

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 2272cac9..d013f03a 100644
--- a/src/buildtool/serve_api/serve_service/source_tree.cpp
+++ b/src/buildtool/serve_api/serve_service/source_tree.cpp
@@ -218,12 +218,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);
@@ -232,6 +226,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()};
@@ -825,12 +825,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);
@@ -839,6 +833,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());
@@ -1116,12 +1116,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);
@@ -1130,6 +1124,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) {
@@ -1376,15 +1376,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) {
@@ -1392,7 +1384,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");
@@ -1485,15 +1483,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) {
@@ -1501,7 +1491,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");
@@ -1594,15 +1590,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) {
@@ -1610,7 +1598,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");
@@ -1722,15 +1716,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) {
@@ -1738,7 +1724,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");