fix recursive locking and add test to detect deadlocks

.github/workflows/test.yaml

@@ -4,25 +4,27 @@ on:
   push:
   pull_request:
 
-
 permissions:
   contents: read
 
 jobs:
   pkg-test:
     strategy:
       matrix:
-        go-version: [1.21.x, 1.22.x]
+        go-version: [1.21.x, 1.22.x, 1.23.x]
         os: [ubuntu-latest]
     runs-on: ${{ matrix.os }}
     steps:
-    - name: Checkout code
-      uses: actions/checkout@v4
+      - name: Checkout code
+        uses: actions/checkout@v4
+
+      - name: Install Go
+        uses: actions/setup-go@v5
+        with:
+          go-version: ${{ matrix.go-version }}
+
+      - name: Test
+        run: go test -v -cover ./...
 
-    - name: Install Go
-      uses: actions/setup-go@v5
-      with:
-        go-version: ${{ matrix.go-version }}
-
-    - name: Test
-      run: go test -v -cover ./...
+      - name: Test with race
+        run: go test -v -cover -race -count 1 -timeout 20s --tags deadlock_test -run Test_mirror_detect_race ./pkg/mirror/...

go.mod

@@ -1,20 +1,23 @@
 module github.com/utilitywarehouse/git-mirror
 
-go 1.22.0
+go 1.23.0
 
 require (
 	github.com/google/go-cmp v0.6.0
-	github.com/prometheus/client_golang v1.19.1
+	github.com/prometheus/client_golang v1.20.1
+	github.com/sasha-s/go-deadlock v0.3.5
 	gopkg.in/yaml.v3 v3.0.1
 )
 
 require (
 	github.com/beorn7/perks v1.0.1 // indirect
-	github.com/cespare/xxhash/v2 v2.2.0 // indirect
+	github.com/cespare/xxhash/v2 v2.3.0 // indirect
 	github.com/kr/text v0.2.0 // indirect
-	github.com/prometheus/client_model v0.5.0 // indirect
-	github.com/prometheus/common v0.48.0 // indirect
-	github.com/prometheus/procfs v0.12.0 // indirect
-	golang.org/x/sys v0.17.0 // indirect
-	google.golang.org/protobuf v1.33.0 // indirect
+	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
+	github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 // indirect
+	github.com/prometheus/client_model v0.6.1 // indirect
+	github.com/prometheus/common v0.55.0 // indirect
+	github.com/prometheus/procfs v0.15.1 // indirect
+	golang.org/x/sys v0.24.0 // indirect
+	google.golang.org/protobuf v1.34.2 // indirect
 )

go.sum

@@ -1,28 +1,36 @@
 github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
 github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
-github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44=
-github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
+github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=
+github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
 github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
 github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
+github.com/klauspost/compress v1.17.9 h1:6KIumPrER1LHsvBVuDa0r5xaG0Es51mhhB9BQB2qeMA=
+github.com/klauspost/compress v1.17.9/go.mod h1:Di0epgTjJY877eYKx5yC51cX2A2Vl2ibi7bDH9ttBbw=
 github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
 github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
 github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
-github.com/prometheus/client_golang v1.19.1 h1:wZWJDwK+NameRJuPGDhlnFgx8e8HN3XHQeLaYJFJBOE=
-github.com/prometheus/client_golang v1.19.1/go.mod h1:mP78NwGzrVks5S2H6ab8+ZZGJLZUq1hoULYBAYBw1Ho=
-github.com/prometheus/client_model v0.5.0 h1:VQw1hfvPvk3Uv6Qf29VrPF32JB6rtbgI6cYPYQjL0Qw=
-github.com/prometheus/client_model v0.5.0/go.mod h1:dTiFglRmd66nLR9Pv9f0mZi7B7fk5Pm3gvsjB5tr+kI=
-github.com/prometheus/common v0.48.0 h1:QO8U2CdOzSn1BBsmXJXduaaW+dY/5QLjfB8svtSzKKE=
-github.com/prometheus/common v0.48.0/go.mod h1:0/KsvlIEfPQCQ5I2iNSAWKPZziNCvRs5EC6ILDTlAPc=
-github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k6Bo=
-github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo=
+github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA=
+github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ=
+github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 h1:Dx7Ovyv/SFnMFw3fD4oEoeorXc6saIiQ23LrGLth0Gw=
+github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4=
+github.com/prometheus/client_golang v1.20.1 h1:IMJXHOD6eARkQpxo8KkhgEVFlBNm+nkrFUyGlIu7Na8=
+github.com/prometheus/client_golang v1.20.1/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE=
+github.com/prometheus/client_model v0.6.1 h1:ZKSh/rekM+n3CeS952MLRAdFwIKqeY8b62p8ais2e9E=
+github.com/prometheus/client_model v0.6.1/go.mod h1:OrxVMOVHjw3lKMa8+x6HeMGkHMQyHDk9E3jmP2AmGiY=
+github.com/prometheus/common v0.55.0 h1:KEi6DK7lXW/m7Ig5i47x0vRzuBsHuvJdi5ee6Y3G1dc=
+github.com/prometheus/common v0.55.0/go.mod h1:2SECS4xJG1kd8XF9IcM1gMX6510RAEL65zxzNImwdc8=
+github.com/prometheus/procfs v0.15.1 h1:YagwOFzUgYfKKHX6Dr+sHT7km/hxC76UB0learggepc=
+github.com/prometheus/procfs v0.15.1/go.mod h1:fB45yRUv8NstnjriLhBQLuOUt+WW4BsoGhij/e3PBqk=
 github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ=
 github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog=
-golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y=
-golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
-google.golang.org/protobuf v1.33.0 h1:uNO2rsAINq/JlFpSdYEKIZ0uKD/R9cpdv0T+yoGwGmI=
-google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=
+github.com/sasha-s/go-deadlock v0.3.5 h1:tNCOEEDG6tBqrNDOX35j/7hL5FcFViG6awUGROb2NsU=
+github.com/sasha-s/go-deadlock v0.3.5/go.mod h1:bugP6EGbdGYObIlx7pUZtWqlvo8k9H6vCBBsiChJQ5U=
+golang.org/x/sys v0.24.0 h1:Twjiwq9dn6R1fQcyiK+wQyHWfaz/BJB+YIpzU/Cv3Xg=
+golang.org/x/sys v0.24.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
+google.golang.org/protobuf v1.34.2 h1:6xV6lTsCfpGD21XK49h7MhtcApnLqkfYgPcdHftf6hg=
+google.golang.org/protobuf v1.34.2/go.mod h1:qYOHts0dSfpeUzUFpOMr/WGzszTmLH+DiWniOlNbLDw=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=

pkg/lock/lock_deadlock.go

@@ -0,0 +1,10 @@
+//go:build deadlock_test
+
+package lock
+
+import "github.com/sasha-s/go-deadlock"
+
+// this type is used only in test for deadlock detection
+type RWMutex struct {
+	deadlock.RWMutex
+}

pkg/lock/lock_sync.go

@@ -0,0 +1,9 @@
+//go:build !deadlock_test
+
+package lock
+
+import "sync"
+
+type RWMutex struct {
+	sync.RWMutex
+}

pkg/mirror/repository.go

@@ -11,10 +11,10 @@ import (
 	"regexp"
 	"slices"
 	"strings"
-	"sync"
 	"time"
 
 	"github.com/utilitywarehouse/git-mirror/pkg/giturl"
+	"github.com/utilitywarehouse/git-mirror/pkg/lock"
 )
 
 const (
@@ -49,6 +49,7 @@ const (
 // The implementation borrows heavily from https://github.com/kubernetes/git-sync.
 // A Repository is safe for concurrent use by multiple goroutines.
 type Repository struct {
+	lock          lock.RWMutex             // repository will be locked during mirror
 	gitURL        *giturl.URL              // parsed remote git URL
 	remote        string                   // remote repo to mirror
 	root          string                   // absolute path to the root where repo directory createdabsolute path to the root where repo directory created
@@ -60,7 +61,6 @@ type Repository struct {
 	envs          []string                 // envs which will be passed to git commands
 	running       bool                     // indicates if repository is running the mirror loop
 	workTreeLinks map[string]*WorkTreeLink // list of worktrees which will be maintained
-	lock          sync.RWMutex             // repository will be locked during mirror
 	stop, stopped chan bool                // chans to stop mirror loops
 	log           *slog.Logger
 }
@@ -188,13 +188,13 @@ func (r *Repository) LogMsg(ctx context.Context, ref, path string) (string, erro
 
 // Subject returns commit subject of given commit hash
 func (r *Repository) Subject(ctx context.Context, hash string) (string, error) {
-	r.lock.RLock()
-	defer r.lock.RUnlock()
-
 	if err := r.ObjectExists(ctx, hash); err != nil {
 		return "", err
 	}
 
+	r.lock.RLock()
+	defer r.lock.RUnlock()
+
 	args := []string{"show", `--no-patch`, `--format='%s'`, hash}
 	msg, err := runGitCommand(ctx, r.log, r.envs, r.dir, args...)
 	if err != nil {
@@ -205,13 +205,13 @@ func (r *Repository) Subject(ctx context.Context, hash string) (string, error) {
 
 // ChangedFiles returns path of the changed files for given commit hash
 func (r *Repository) ChangedFiles(ctx context.Context, hash string) ([]string, error) {
-	r.lock.RLock()
-	defer r.lock.RUnlock()
-
 	if err := r.ObjectExists(ctx, hash); err != nil {
 		return nil, err
 	}
 
+	r.lock.RLock()
+	defer r.lock.RUnlock()
+
 	args := []string{"show", `--name-only`, `--pretty=format:`, hash}
 	msg, err := runGitCommand(ctx, r.log, r.envs, r.dir, args...)
 	if err != nil {

pkg/mirror/z_e2e_race_test.go

@@ -0,0 +1,107 @@
+//go:build deadlock_test
+
+package mirror
+
+import (
+	"context"
+	"log"
+	"os"
+	"path/filepath"
+	"sync"
+	"testing"
+)
+
+func Test_mirror_detect_race(t *testing.T) {
+	testTmpDir := mustTmpDir(t)
+	defer os.RemoveAll(testTmpDir)
+
+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+
+	upstream := filepath.Join(testTmpDir, testUpstreamRepo)
+	root := filepath.Join(testTmpDir, testRoot)
+	link1 := "link1" // on testBranchMain branch
+	link2 := "link2" // on remote HEAD
+	ref1 := testMainBranch
+	ref2 := "HEAD"
+	testName := t.Name()
+
+	t.Log("TEST-1: init upstream")
+	fileSHA1 := mustInitRepo(t, upstream, "file", testName+"-1")
+
+	repo := mustCreateRepoAndMirror(t, upstream, root, link1, ref1)
+	// add worktree for HEAD
+	if err := repo.AddWorktreeLink(link2, ref2, ""); err != nil {
+		t.Fatalf("unable to add worktree error: %v", err)
+	}
+	// mirror again for 2nd worktree
+	if err := repo.Mirror(ctx); err != nil {
+		t.Fatalf("unable to mirror error: %v", err)
+	}
+
+	// verify checkout files
+	assertCommitLog(t, repo, "HEAD", "", fileSHA1, testName+"-1", []string{"file"})
+	assertLinkedFile(t, root, link1, "file", testName+"-1")
+	assertLinkedFile(t, root, link2, "file", testName+"-1")
+
+	// start mirror loop
+	go repo.StartLoop(ctx)
+	close(repo.stop)
+
+	t.Log("TEST-2: forward HEAD")
+	fileSHA2 := mustCommit(t, upstream, "file", testName+"-2")
+
+	t.Run("test-1", func(t *testing.T) {
+		wg := &sync.WaitGroup{}
+		// all following assertions will always be true
+		// this test is about testing deadlocks and detecting race conditions
+		for i := 0; i < 100; i++ {
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				if err := repo.Mirror(ctx); err != nil {
+					log.Fatalf("unable to mirror error: %v", err)
+				}
+
+				assertLinkedFile(t, root, link1, "file", testName+"-2")
+				assertLinkedFile(t, root, link2, "file", testName+"-2")
+			}()
+
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+
+				assertCommitLog(t, repo, "HEAD", "", fileSHA2, testName+"-2", []string{"file"})
+			}()
+		}
+
+		// clone tests
+		for i := 0; i < 10; i++ {
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				if err := repo.Mirror(ctx); err != nil {
+					log.Fatalf("unable to mirror error: %v", err)
+				}
+			}()
+
+			wg.Add(1)
+			go func() {
+				defer wg.Done()
+				tempClone := mustTmpDir(t)
+				defer os.RemoveAll(tempClone)
+
+				if cloneSHA, err := repo.Clone(ctx, tempClone, testMainBranch, "", i%2 == 0); err != nil {
+					t.Fatalf("unexpected error %s", err)
+				} else {
+					if cloneSHA != fileSHA2 {
+						t.Errorf("clone sha mismatch got:%s want:%s", cloneSHA, fileSHA2)
+					}
+					assertFile(t, filepath.Join(tempClone, "file"), testName+"-2")
+				}
+			}()
+		}
+		wg.Wait()
+	})
+
+}