Failing test to demo race condition in synchronizeWithRemoteRepository

src/main/kotlin/io/meshcloud/dockerosb/persistence/GitHandlerService.kt

@@ -31,7 +31,10 @@ class GitHandlerService(
    * This prevents commits "laying around" at start time until a new
    * commit would set this flag.
    */
-  private val hasLocalCommits = AtomicBoolean(true)
+  private val _hasLocalCommits = AtomicBoolean(true)
+  fun hasLocalCommits(): Boolean {
+    return _hasLocalCommits.get()
+  }
 
   private val git = initGit(gitConfig)
 
@@ -103,7 +106,7 @@ class GitHandlerService(
     addAllChanges()
     commitAsOsbApi(commitMessage)
 
-    hasLocalCommits.set(true)
+    _hasLocalCommits.set(true)
   }
 
   override fun synchronizeWithRemoteRepository() {
@@ -112,7 +115,7 @@ class GitHandlerService(
       return
     }
 
-    if (!hasLocalCommits.get()) {
+    if (!_hasLocalCommits.get()) {
       // note: we do also not execute a fetch in this case because if the unipipe-osb is just idling
       // there's no sense in us fetching from the remote all the time. Consumers can explicitly call
       // pullFastForwardOnly if they want an up to date copy
@@ -125,7 +128,7 @@ class GitHandlerService(
     try {
       val pushedSuccessfully = fetchMergePush()
       if (pushedSuccessfully) {
-        hasLocalCommits.set(false)
+        _hasLocalCommits.set(false)
       }
 
       log.info { "Completed synchronizeWithRemoteRepository" }
@@ -245,7 +248,7 @@ class GitHandlerService(
         .call()
   }
 
-  private fun push() {
+  protected fun push() {
     val pushCommand = git.push()
 
     gitConfig.username?.let {

src/test/kotlin/io/meshcloud/dockerosb/service/ConcurrentGitInteractionsTest.kt

@@ -1,6 +1,8 @@
 package io.meshcloud.dockerosb.service
 
 import io.meshcloud.dockerosb.ServiceBrokerFixture
+import io.meshcloud.dockerosb.config.GitConfig
+import io.meshcloud.dockerosb.persistence.GitHandlerService
 import io.meshcloud.dockerosb.persistence.ScheduledPushHandler
 import org.hamcrest.MatcherAssert
 import org.hamcrest.Matchers.greaterThan
@@ -231,7 +233,66 @@ class ConcurrentGitInteractionsTest {
     assertTrue(fixture.gitHandler.fileInRepo(statusFilePath).exists())
   }
 
+  @Test
+  fun `can synchronize without race conditions for tracking local changes`() {
+    val sut = makeSut()
+
+    val createRequest = createServiceInstanceRequest("00000000-7e05-4d5a-97b8-f8c5d1c710ab")
+
+    val statusFilePath = "instances/${createRequest.serviceInstanceId}/status.yml"
+
+    var firstCommitDone = false
+    var secondCommitDone = false
+
+    val postPushHook = { gitHandler: GitHandlerService ->
+        if (firstCommitDone) {
+            // Simulate some OSB request that occurs just after the first
+            // commit was pushed to the remote and before
+            // synchronizeWithRemoteRepository has completed. This tests for
+            // race conditions if any shared state is manipulated by
+            // commitAllChanges but undone by the rest of the
+            // synchronizeWithRemoteRepository call.
+            gitHandler.fileInRepo(statusFilePath).delete()
+            gitHandler.commitAllChanges("second commit")
+
+            secondCommitDone = true
+        }
+    }
+
+    val gitHandler = HookedGitHandlerService(fixture.gitConfig, postPushHook)
+
+    // this will create a local service instance request
+    sut.createServiceInstance(createRequest).block()
+    gitHandler.synchronizeWithRemoteRepository()
+
+    // Create a commit so that synchronizeWithRemoteRepository needs to push
+    // something to the remote and ends up calling `postPushHook`
+    fixture.remote.writeFile(statusFilePath, "status: in progress\ndescription: deploying")
+    gitHandler.commitAllChanges("first commit")
+    firstCommitDone = true
+
+    // This should push the first commit then immediately `postPushHook` will
+    // create a second commit. If there's any shared book keeping between
+    // `commitAllChanges` and `synchronizeWithRemoteRepository` that's
+    // vulnerable to a race condition then this should expose it.
+    assertEquals(false, secondCommitDone)
+    gitHandler.synchronizeWithRemoteRepository()
+    assertEquals(true, secondCommitDone)
+
+    assertEquals(true, gitHandler.hasLocalCommits())
+  }
+
   private fun createServiceInstanceRequest(instanceId: String): CreateServiceInstanceRequest {
     return fixture.builder.createServiceInstanceRequest(instanceId)
   }
-}
+}
+
+class HookedGitHandlerService(
+    private val gitConfig: GitConfig,
+    private val postPushHook: ((GitHandlerService) -> Unit) = {}
+): GitHandlerService(gitConfig) {
+  override fun push() {
+    super.push()
+    postPushHook(this)
+  }
+}