Skip to content

Commit

Permalink
fix: local commit detection not thread safe
Browse files Browse the repository at this point in the history
synchronizeWithRemoteRepository did not handle the scenario where other
threads may modify the local repository between the merge operation and
when hasLocalCommits is set to false. This can cause the local view of
the repo to become out of sync with the remote. At best it is resolved
when another OSB operation, e.g. a provision request, creates a new
commit with the side effect that it signals there are local changes. At
worst the commits are lost if the repo storage is ephemeral and
is destroyed before the commits are pushed.
  • Loading branch information
tracemeyers authored and JohannesRudolph committed Jun 21, 2024
1 parent 25369b6 commit e0df96d
Showing 1 changed file with 19 additions and 21 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import org.eclipse.jgit.transport.URIish
import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider
import org.springframework.stereotype.Service
import java.io.File
import java.util.concurrent.atomic.AtomicBoolean

private val log = KotlinLogging.logger {}

Expand All @@ -24,18 +23,6 @@ class GitHandlerService(
private val gitConfig: GitConfig
) : GitHandler {

/**
* Signal if we have local commits that we want to sync or not.
*
* At startup we just assume there are new commits in git.
* This prevents commits "laying around" at start time until a new
* commit would set this flag.
*/
private val _hasLocalCommits = AtomicBoolean(true)
fun hasLocalCommits(): Boolean {
return _hasLocalCommits.get()
}

private val git = initGit(gitConfig)

override fun instancesDirectory(): File {
Expand Down Expand Up @@ -105,8 +92,6 @@ class GitHandlerService(
override fun commitAllChanges(commitMessage: String) {
addAllChanges()
commitAsOsbApi(commitMessage)

_hasLocalCommits.set(true)
}

override fun synchronizeWithRemoteRepository() {
Expand All @@ -115,7 +100,7 @@ class GitHandlerService(
return
}

if (!_hasLocalCommits.get()) {
if (!hasLocalCommits()) {
// 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
Expand All @@ -126,10 +111,7 @@ class GitHandlerService(
log.info { "Starting synchronizeWithRemoteRepository" }

try {
val pushedSuccessfully = fetchMergePush()
if (pushedSuccessfully) {
_hasLocalCommits.set(false)
}
fetchMergePush()

log.info { "Completed synchronizeWithRemoteRepository" }
} catch (ex: Exception) {
Expand Down Expand Up @@ -207,6 +189,22 @@ class GitHandlerService(
return false
}

fun hasLocalCommits(): Boolean {
val origin = git.repository.resolve("origin/${gitConfig.remoteBranch}")
val head = git.getRepository().resolve("HEAD")

var count = 0
for (entry in git.log().addRange(origin, head).call()) {
++count
}

if (count > 0) {
log.info { "Your branch is ahead of 'origin/${gitConfig.remoteBranch}' by $count commit(s)." }
}

return count > 0
}

private fun resolveMergeConflictsUsingOurs(files: List<String>) {
log.warn { "Encountered conflicts in files $files. Will attempt to auto-resolve conflicts, preferring local changes." }

Expand Down Expand Up @@ -372,4 +370,4 @@ class GitHandlerService(
}
}
}
}
}

0 comments on commit e0df96d

Please sign in to comment.