Fix/mutex lock (#911)

* fix: await routeHandler

* fix: return after next

* feat: send LockedError instead of ConflictError

* fix: import lockedError

* Fix/git file lock (#905)

* feat: add hasGitFileLock

* feat: make check for git file lock in route handlers

* fix: always return true if something exists on the path

* fix: return if handleGitFileLock fails

* fix: use lockedError instead of conflictError

* chore: update error message

src/errors/LockedError.ts

@@ -0,0 +1,11 @@
+import { BaseIsomerError } from "./BaseError"
+
+export default class LockedError extends BaseIsomerError {
+  constructor(message: string) {
+    super({
+      status: 423,
+      code: "LockedError",
+      message,
+    })
+  }
+}

src/middleware/routeHandler.js

@@ -8,16 +8,44 @@ const { default: GithubSessionData } = require("@classes/GithubSessionData")
 const { lock, unlock } = require("@utils/mutex-utils")
 const { getCommitAndTreeSha, revertCommit } = require("@utils/utils.js")
 
-const GitFileSystemError = require("@root/errors/GitFileSystemError")
+const GitFileSystemError = require("@root/errors/GitFileSystemError").default
+const LockedError = require("@root/errors/LockedError").default
 const {
   default: GitFileSystemService,
 } = require("@services/db/GitFileSystemService")
 
+const logger = require("@logger/logger").default
+
 const WHITELISTED_GIT_SERVICE_REPOS = config.get(
   "featureFlags.ggsWhitelistedRepos"
 )
 const BRANCH_REF = config.get("github.branchRef")
 
+const gitFileSystemService = new GitFileSystemService(new SimpleGit())
+
+const isRepoWhitelisted = (siteName) =>
+  WHITELISTED_GIT_SERVICE_REPOS.split(",").includes(siteName)
+
+const handleGitFileLock = async (repoName, next) => {
+  if (!isRepoWhitelisted(repoName)) return true
+  const result = await gitFileSystemService.hasGitFileLock(repoName)
+  if (result.isErr()) {
+    next(result.err)
+    return false
+  }
+  const isGitLocked = result.value
+  if (isGitLocked) {
+    logger.error(`Failed to lock repo ${repoName}: git file system in use.`)
+    next(
+      new LockedError(
+        `Someone else is currently modifying repo ${repoName}. Please try again later.`
+      )
+    )
+    return false
+  }
+  return true
+}
+
 // Used when there are no write API calls to the repo on GitHub
 const attachReadRouteHandlerWrapper = (routeHandler) => async (
   req,
@@ -36,13 +64,17 @@ const attachWriteRouteHandlerWrapper = (routeHandler) => async (
   next
 ) => {
   const { siteName } = req.params
+
+  const isGitAvailable = await handleGitFileLock(siteName, next)
+  if (!isGitAvailable) return
   try {
     await lock(siteName)
   } catch (err) {
     next(err)
+    return
   }
 
-  routeHandler(req, res, next).catch(async (err) => {
+  await routeHandler(req, res, next).catch(async (err) => {
     await unlock(siteName)
     next(err)
   })
@@ -53,8 +85,6 @@ const attachWriteRouteHandlerWrapper = (routeHandler) => async (
   }
 }
 
-const gitFileSystemService = new GitFileSystemService(new SimpleGit())
-
 const attachRollbackRouteHandlerWrapper = (routeHandler) => async (
   req,
   res,
@@ -64,37 +94,39 @@ const attachRollbackRouteHandlerWrapper = (routeHandler) => async (
   const { siteName } = req.params
 
   const { accessToken } = userSessionData
-  const isRepoWhitelisted = WHITELISTED_GIT_SERVICE_REPOS.split(",").includes(
-    siteName
-  )
+  const shouldUseGitFileSystem = isRepoWhitelisted(siteName)
 
+  const isGitAvailable = await handleGitFileLock(siteName, next)
+  if (!isGitAvailable) return
   try {
     await lock(siteName)
   } catch (err) {
     next(err)
+    return
   }
 
   let originalCommitSha
-  if (isRepoWhitelisted) {
+  if (shouldUseGitFileSystem) {
     const result = await gitFileSystemService.getLatestCommitOfBranch(
       siteName,
       BRANCH_REF
     )
     if (result.isErr()) {
       await unlock(siteName)
       next(result.err)
-    } else {
-      originalCommitSha = result.value.sha
-      if (!originalCommitSha) {
-        await unlock(siteName)
-        next(result.err)
-      }
-      // Unused for git file system, but to maintain existing structure
-      res.locals.githubSessionData = new GithubSessionData({
-        currentCommitSha: "",
-        treeSha: "",
-      })
+      return
+    }
+    originalCommitSha = result.value.sha
+    if (!originalCommitSha) {
+      await unlock(siteName)
+      next(result.err)
+      return
     }
+    // Unused for git file system, but to maintain existing structure
+    res.locals.githubSessionData = new GithubSessionData({
+      currentCommitSha: "",
+      treeSha: "",
+    })
   } else {
     try {
       const { currentCommitSha, treeSha } = await getCommitAndTreeSha(
@@ -112,11 +144,12 @@ const attachRollbackRouteHandlerWrapper = (routeHandler) => async (
     } catch (err) {
       await unlock(siteName)
       next(err)
+      return
     }
   }
-  routeHandler(req, res, next).catch(async (err) => {
+  await routeHandler(req, res, next).catch(async (err) => {
     try {
-      if (isRepoWhitelisted) {
+      if (shouldUseGitFileSystem) {
         await backOff(() => {
           const rollbackRes = gitFileSystemService
             .rollback(siteName, originalCommitSha)
@@ -137,6 +170,7 @@ const attachRollbackRouteHandlerWrapper = (routeHandler) => async (
     } catch (retryErr) {
       await unlock(siteName)
       next(retryErr)
+      return
     }
     await unlock(siteName)
     next(err)

src/services/db/GitFileSystemService.ts

@@ -50,6 +50,21 @@ export default class GitFileSystemService {
     this.git = git
   }
 
+  hasGitFileLock(repoName: string): ResultAsync<boolean, GitFileSystemError> {
+    const gitFileLockPath = ".git/index.lock"
+    return this.getFilePathStats(repoName, gitFileLockPath)
+      .andThen(() => ok(true))
+      .orElse((error) => {
+        if (error instanceof NotFoundError) {
+          return ok(false)
+        }
+        logger.error(
+          `Error when checking for git file lock for ${repoName}: ${error}`
+        )
+        return err(error)
+      })
+  }
+
   isDefaultLogFields(logFields: unknown): logFields is DefaultLogFields {
     const c = logFields as DefaultLogFields
     return (

src/utils/mutex-utils.js

@@ -5,7 +5,7 @@ const { config } = require("@config/config")
 
 const logger = require("@logger/logger").default
 
-const { ConflictError } = require("@errors/ConflictError")
+const LockedError = require("@root/errors/LockedError").default
 
 // Env vars
 const NODE_ENV = config.get("env")
@@ -52,7 +52,7 @@ const lock = async (siteName) => {
     logger.error(
       `Failed to lock repo ${siteName}: ${JSON.stringify(serializeError(err))}`
     )
-    throw new ConflictError(
+    throw new LockedError(
       `Someone else is currently modifying repo ${siteName}. Please try again later.`
     )
   }