Repository: isomorphic-git/isomorphic-git
Pull request #1882 (2024-03-14 19:44:34)

Fix race condition when reading/writing refs
## Description

Adds locking around filesystem operations when reading/writing refs to avoid race conditions.

Fixes https://github.com/isomorphic-git/isomorphic-git/issues/1881.

## I'm fixing a bug or typo

- [X] if this is your first time contributing, run `npm run add-contributor` and follow the prompts to add yourself to the README
- [ ] squash merge the PR with commit message "fix: [Description of fix]"


jcubic: It would be nice to have a unit test for this. I know you said it's hard to reproduce the issue, but maybe do something with threads and try to save and read at the same time.

If you will not be able to tests the locks, we will at least to make sure they don't interfere with normal workflow.

And about the Unit tests, they are failing because of broken credentials in Azure DevOps. We are in the process of migrating the CI/CD to GitHub actions, but maybe we will disable the failing tests for now, so we will be able to test.
jcubic: Looks good.

So now we need to fix the tests. Probably disable failing ones temporarily. I will be not available during a weekend, will get back to this on Monday. The tests are important because they block contribution.
alexvy86: I noticed other places were already using the `async-lock` package so just used that too for this fix, and managed to write a unit test _that is not deterministic_, but as far as I could tell, reproduced the problem pretty reliably before applying the fix.

I see there's already https://github.com/isomorphic-git/isomorphic-git/pull/1884 working on disabling the failing test. Not to be pushy, but isn't there someone with admin permissions on the repo that could merge contributions even with failed checks? As long as that is the only failing test, that seems safe to me.
alexvy86: /azp run isomorphic-git-PR
azure-pipelines[bot]: <samp>
Commenter does not have sufficient privileges for PR 1882 in repo isomorphic-git/isomorphic-git<br>

</samp>
jcubic: I have admin rights and can merge even when test fail, but I don't want to. Also, I think that the release will not happen when the tests are failing. CI/CD will stop execution if there is an error. But even it this would work, it's not good practice to ignore the tests and merge anyway. Tests are created for a reason. To make sure the code works, not something you can just ignore for your convenience.
jcubic: The failing test got merged, please rebase.
alexvy86: > To make sure the code works, not something you can just ignore for your convenience.

Absolutely, and if the failure had been due to a code problem, that's definitely a reason not to merge. Since in this case it was clear that the failing test was not because of a problem in code but an issue with an external system, my take is that it would have been ok to do. But again, not trying to push it, just giving my opinion :).

I'll rebase now!

Patch:
diff --git a/.all-contributorsrc b/.all-contributorsrc
index b4ab87b6..3e45a142 100644
--- a/.all-contributorsrc
+++ b/.all-contributorsrc
@@ -669,6 +669,15 @@
       "contributions": [
         "doc"
       ]
+    },
+    {
+      "login": "alexvy86",
+      "name": "Alex Villarreal",
+      "avatar_url": "https://avatars.githubusercontent.com/u/716334?v=4",
+      "profile": "https://alex-v.blog/",
+      "contributions": [
+        "code"
+      ]
     }
   ],
   "commitConvention": "angular"
diff --git a/README.md b/README.md
index dd736fa9..d625474f 100644
--- a/README.md
+++ b/README.md
@@ -368,6 +368,7 @@ Thanks goes to these wonderful people ([emoji key](https://github.com/kentcdodds
     <td align="center"><a href="https://api.github.com/users/hisco"><img src="https://avatars.githubusercontent.com/u/39222286?v=4?s=60" width="60px;" alt=""/><br /><sub><b>Eyal Hisco</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/issues?q=author%3Ahisco" title="Bug reports">🐛</a></td>
     <td align="center"><a href="https://github.com/scolladon"><img src="https://avatars.githubusercontent.com/u/522422?v=4?s=60" width="60px;" alt=""/><br /><sub><b>Sebastien</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=scolladon" title="Code">💻</a></td>
     <td align="center"><a href="https://github.com/yarikoptic"><img src="https://avatars.githubusercontent.com/u/39889?v=4?s=60" width="60px;" alt=""/><br /><sub><b>Yaroslav Halchenko</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=yarikoptic" title="Documentation">📖</a></td>
+    <td align="center"><a href="https://alex-v.blog/"><img src="https://avatars.githubusercontent.com/u/716334?v=4?s=60" width="60px;" alt=""/><br /><sub><b>Alex Villarreal</b></sub></a><br /><a href="https://github.com/isomorphic-git/isomorphic-git/commits?author=alexvy86" title="Code">💻</a></td>
   </tr>
 </table>
 
diff --git a/src/managers/GitRefManager.js b/src/managers/GitRefManager.js
index b0828074..64e7ac22 100644
--- a/src/managers/GitRefManager.js
+++ b/src/managers/GitRefManager.js
@@ -1,4 +1,6 @@
 // This is a convenience wrapper for reading and writing files in the 'refs' directory.
+import AsyncLock from 'async-lock'
+
 import { InvalidOidError } from '../errors/InvalidOidError.js'
 import { NoRefspecError } from '../errors/NoRefspecError.js'
 import { NotFoundError } from '../errors/NotFoundError.js'
@@ -22,6 +24,13 @@ const refpaths = ref => [
 // @see https://git-scm.com/docs/gitrepository-layout
 const GIT_FILES = ['config', 'description', 'index', 'shallow', 'commondir']
 
+let lock
+
+async function acquireLock(ref, callback) {
+  if (lock === undefined) lock = new AsyncLock()
+  return lock.acquire(ref, callback)
+}
+
 export class GitRefManager {
   static async updateRemoteRefs({
     fs,
@@ -128,7 +137,9 @@ export class GitRefManager {
     // are .git/refs/remotes/origin/refs/remotes/remote_mirror_3059
     // and .git/refs/remotes/origin/refs/merge-requests
     for (const [key, value] of actualRefsToWrite) {
-      await fs.write(join(gitdir, key), `${value.trim()}\n`, 'utf8')
+      await acquireLock(key, async () =>
+        fs.write(join(gitdir, key), `${value.trim()}\n`, 'utf8')
+      )
     }
     return { pruned }
   }
@@ -139,11 +150,15 @@ export class GitRefManager {
     if (!value.match(/[0-9a-f]{40}/)) {
       throw new InvalidOidError(value)
     }
-    await fs.write(join(gitdir, ref), `${value.trim()}\n`, 'utf8')
+    await acquireLock(ref, async () =>
+      fs.write(join(gitdir, ref), `${value.trim()}\n`, 'utf8')
+    )
   }
 
   static async writeSymbolicRef({ fs, gitdir, ref, value }) {
-    await fs.write(join(gitdir, ref), 'ref: ' + `${value.trim()}\n`, 'utf8')
+    await acquireLock(ref, async () =>
+      fs.write(join(gitdir, ref), 'ref: ' + `${value.trim()}\n`, 'utf8')
+    )
   }
 
   static async deleteRef({ fs, gitdir, ref }) {
@@ -154,7 +169,9 @@ export class GitRefManager {
     // Delete regular ref
     await Promise.all(refs.map(ref => fs.rm(join(gitdir, ref))))
     // Delete any packed ref
-    let text = await fs.read(`${gitdir}/packed-refs`, { encoding: 'utf8' })
+    let text = await acquireLock('packed-refs', async () =>
+      fs.read(`${gitdir}/packed-refs`, { encoding: 'utf8' })
+    )
     const packed = GitPackedRefs.from(text)
     const beforeSize = packed.refs.size
     for (const ref of refs) {
@@ -164,7 +181,9 @@ export class GitRefManager {
     }
     if (packed.refs.size < beforeSize) {
       text = packed.toString()
-      await fs.write(`${gitdir}/packed-refs`, text, { encoding: 'utf8' })
+      await acquireLock('packed-refs', async () =>
+        fs.write(`${gitdir}/packed-refs`, text, { encoding: 'utf8' })
+      )
     }
   }
 
@@ -183,7 +202,7 @@ export class GitRefManager {
         return ref
       }
     }
-    let sha
+
     // Is it a ref pointer?
     if (ref.startsWith('ref: ')) {
       ref = ref.slice('ref: '.length)
@@ -199,9 +218,12 @@ export class GitRefManager {
     const allpaths = refpaths(ref).filter(p => !GIT_FILES.includes(p)) // exclude git system files (#709)
 
     for (const ref of allpaths) {
-      sha =
-        (await fs.read(`${gitdir}/${ref}`, { encoding: 'utf8' })) ||
-        packedMap.get(ref)
+      const sha = await acquireLock(
+        ref,
+        async () =>
+          (await fs.read(`${gitdir}/${ref}`, { encoding: 'utf8' })) ||
+          packedMap.get(ref)
+      )
       if (sha) {
         return GitRefManager.resolve({ fs, gitdir, ref: sha.trim(), depth })
       }
@@ -229,7 +251,10 @@ export class GitRefManager {
     // Look in all the proper paths, in this order
     const allpaths = refpaths(ref)
     for (const ref of allpaths) {
-      if (await fs.exists(`${gitdir}/${ref}`)) return ref
+      const refExists = await acquireLock(ref, async () =>
+        fs.exists(`${gitdir}/${ref}`)
+      )
+      if (refExists) return ref
       if (packedMap.has(ref)) return ref
     }
     // Do we give up?
@@ -280,7 +305,9 @@ export class GitRefManager {
   }
 
   static async packedRefs({ fs, gitdir }) {
-    const text = await fs.read(`${gitdir}/packed-refs`, { encoding: 'utf8' })
+    const text = await acquireLock('packed-refs', async () =>
+      fs.read(`${gitdir}/packed-refs`, { encoding: 'utf8' })
+    )
     const packed = GitPackedRefs.from(text)
     return packed.refs
   }


Test patch:
diff --git a/__tests__/test-GitRefManager.js b/__tests__/test-GitRefManager.js
index 652d6f73..43576c34 100644
--- a/__tests__/test-GitRefManager.js
+++ b/__tests__/test-GitRefManager.js
@@ -245,4 +245,41 @@ describe('GitRefManager', () => {
       ]
     `)
   })
+  it('concurrently reading/writing a ref should not cause a NotFoundError resolving it', async () => {
+    // There are some expect() calls below, but as of 2023-03-15, if this test fails it will do so by logging instances
+    // of 'NotFoundError: Could not find myRef', which should not happen.
+    const { fs, gitdir } = await makeFixture('test-GitRefManager')
+    const ref = 'myRef'
+    const value = '1234567890123456789012345678901234567890'
+    await GitRefManager.writeRef({ fs, gitdir, ref, value }) // Guarantee that the file for the ref exists on disk
+
+    const writePromises = []
+    const resolvePromises = []
+    // Some arbitrary number of iterations that seems to guarantee that the error (pre-fix) is hit.
+    // With 100 the test *mostly* failed but still passed every now and then.
+    const iterations = 500
+
+    for (let i = 0; i < iterations; i++) {
+      // I was only able to cause the error to reproduce consistently by mixing awaited and non-awaited versions of the
+      // calls to writeRef() and resolve(). I tried several variations of the combination but none of them caused the
+      // error to happen as consistently.
+      if (Math.random() < 0.5) {
+        await GitRefManager.writeRef({ fs, gitdir, ref, value })
+      } else {
+        writePromises.push(GitRefManager.writeRef({ fs, gitdir, ref, value }))
+      }
+      if (Math.random() < 0.5) {
+        const resolvedRef = await GitRefManager.resolve({ fs, gitdir, ref })
+        expect(resolvedRef).toMatch(value)
+      } else {
+        resolvePromises.push(GitRefManager.resolve({ fs, gitdir, ref }))
+      }
+    }
+
+    const resolvedRefs = await Promise.all(resolvePromises)
+    for (const resolvedRef of resolvedRefs) {
+      expect(resolvedRef).toMatch(value)
+    }
+    await Promise.all(writePromises)
+  })
 })