Repository: etcd-io/etcd (language: go)

Issue:
[release-3.4] Races in TestLessorRenewExtendPileup TestLessorDetach
### Which Github Action / Prow Jobs are flaking?

Running `make test-unit` on release-3.4 branch detects races for me

### Which tests are flaking?

TestLessorRenewExtendPileup and TestLessorDetach

### Github Action / Prow Job link

_No response_

### Reason for failure (if possible)

_No response_

### Anything else we need to know?

_No response_


Discussion / hints:
Minimal repro ` go test ./lease/ --count 10 -v --race`


```
=== RUN   TestLessorRenewExtendPileup
==================
WARNING: DATA RACE
Write at 0x00000143c590 by goroutine 385:
  go.etcd.io/etcd/lease.TestLessorRenewExtendPileup()
      /usr/local/google/home/siarkowicz/src/go.etcd.io/etcd/lease/lessor_test.go:294 +0x289
  testing.tRunner()
      /usr/local/google/home/siarkowicz/.gvm/gos/go1.22/src/testing/testing.go:1689 +0x21e
  testing.(*T).Run.gowrap1()
      /usr/local/google/home/siarkowicz/.gvm/gos/go1.22/src/testing/testing.go:1742 +0x44

Previous read at 0x00000143c590 by goroutine 188:
  go.etcd.io/etcd/lease.(*lessor).revokeExpiredLeases()
      /usr/local/google/home/siarkowicz/src/go.etcd.io/etcd/lease/lessor.go:629 +0x3a
  go.etcd.io/etcd/lease.(*lessor).runLoop()
      /usr/local/google/home/siarkowicz/src/go.etcd.io/etcd/lease/lessor.go:612 +0xe4
  go.etcd.io/etcd/lease.newLessor.gowrap1()
      /usr/local/google/home/siarkowicz/src/go.etcd.io/etcd/lease/lessor.go:238 +0x33

Goroutine 385 (running) created at:
  testing.(*T).Run()
      /usr/local/google/home/siarkowicz/.gvm/gos/go1.22/src/testing/testing.go:1742 +0x825
  testing.runTests.func1()
      /usr/local/google/home/siarkowicz/.gvm/gos/go1.22/src/testing/testing.go:2161 +0x85
  testing.tRunner()
      /usr/local/google/home/siarkowicz/.gvm/gos/go1.22/src/testing/testing.go:1689 +0x21e
  testing.runTests()
      /usr/local/google/home/siarkowicz/.gvm/gos/go1.22/src/testing/testing.go:2159 +0x8be
  testing.(*M).Run()
      /usr/local/google/home/siarkowicz/.gvm/gos/go1.22/src/testing/testing.go:2027 +0xf17
  main.main()
      _testmain.go:93 +0x2bd

Goroutine 188 (running) created at:
  go.etcd.io/etcd/lease.newLessor()
      /usr/local/google/home/siarkowicz/src/go.etcd.io/etcd/lease/lessor.go:238 +0x590
  go.etcd.io/etcd/lease.TestLessorCheckpointPersistenceAfterRestart.func1()
      /usr/local/google/home/siarkowicz/src/go.etcd.io/etcd/lease/lessor_test.go:722 +0x7c4
  testing.tRunner()
      /usr/local/google/home/siarkowicz/.gvm/gos/go1.22/src/testing/testing.go:1689 +0x21e
  testing.(*T).Run.gowrap1()
      /usr/local/google/home/siarkowicz/.gvm/gos/go1.22/src/testing/testing.go:1742 +0x44
==================
    testing.go:1398: race detected during execution of test
--- FAIL: TestLessorRenewExtendPileup (0.06s)

```
I was also able to reproduce with `go test ./lease/ --count 10 -v --race`.

```bash
=== RUN   TestLessorRenewExtendPileup                                                                                         
==================                                                                                                            
WARNING: DATA RACE                                                                                                            
Write at 0x0000014595b0 by goroutine 377:                                                                                     
  go.etcd.io/etcd/lease.TestLessorRenewExtendPileup()                                                                         
      /home/james/Documents/etcd/lease/lessor_test.go:295 +0x289                                                              
  testing.tRunner()                                                                                                           
      /home/linuxbrew/.linuxbrew/Cellar/go@1.22/1.22.7/libexec/src/testing/testing.go:1689 +0x21e                             
  testing.(*T).Run.gowrap1()                                                                                                  
      /home/linuxbrew/.linuxbrew/Cellar/go@1.22/1.22.7/libexec/src/testing/testing.go:1742 +0x44                              
                                                                                                                              
Previous read at 0x0000014595b0 by goroutine 187:                                                                             
  go.etcd.io/etcd/lease.(*lessor).revokeExpiredLeases()                                                                       
      /home/james/Documents/etcd/lease/lessor.go:631 +0x3a                                                                    
  go.etcd.io/etcd/lease.(*lessor).runLoop()                                                                                   
      /home/james/Documents/etcd/lease/lessor.go:614 +0xe4                                                                    
  go.etcd.io/etcd/lease.newLessor.gowrap1()                                                                                   
      /home/james/Documents/etcd/lease/lessor.go:238 +0x33                                                                    
                                                                                                                              
Goroutine 377 (running) created at:                                                                                           
  testing.(*T).Run()                                                                                                          
      /home/linuxbrew/.linuxbrew/Cellar/go@1.22/1.22.7/libexec/src/testing/testing.go:1742 +0x825                             
  testing.runTests.func1()                                                                                                    
      /home/linuxbrew/.linuxbrew/Cellar/go@1.22/1.22.7/libexec/src/testing/testing.go:2161 +0x85                              
  testing.tRunner()                                                                                                           
      /home/linuxbrew/.linuxbrew/Cellar/go@1.22/1.22.7/libexec/src/testing/testing.go:1689 +0x21e                             
  testing.runTests()                                                                                                          
      /home/linuxbrew/.linuxbrew/Cellar/go@1.22/1.22.7/libexec/src/testing/testing.go:2159 +0x8be                             
  testing.(*M).Run()                                                                                                          
      /home/linuxbrew/.linuxbrew/Cellar/go@1.22/1.22.7/libexec/src/testing/testing.go:2027 +0xf17                             
  main.main()                                                                                                                 
      _testmain.go:93 +0x2bd                                                                                                  
                                                                                                                              
Goroutine 187 (running) created at:                                                                                           
  go.etcd.io/etcd/lease.newLessor()                                                                                           
      /home/james/Documents/etcd/lease/lessor.go:238 +0x590                                                                   
  go.etcd.io/etcd/lease.TestLessorCheckpointPersistenceAfterRestart.func1()                                                   
      /home/james/Documents/etcd/lease/lessor_test.go:724 +0x7c4                                                              
  testing.tRunner()                                                                                                           
      /home/linuxbrew/.linuxbrew/Cellar/go@1.22/1.22.7/libexec/src/testing/testing.go:1689 +0x21e                             
  testing.(*T).Run.gowrap1()                                                                                                  
      /home/linuxbrew/.linuxbrew/Cellar/go@1.22/1.22.7/libexec/src/testing/testing.go:1742 +0x44                              
==================                                                                                                            
    testing.go:1398: race detected during execution of test                                                                   
```
Hi @serathius @jmhbnz,

I was able to reproduce on the main branch as well with:

```shell
cd server
go test ./lease/ --count 10 -v --race --failfast
```

I fixed the race condition and verified by running the unit tests locally.

I submitted a PR to fix: #18555

Looking forward to your feedback, thanks!

Fix patch:
diff --git a/server/lease/lessor.go b/server/lease/lessor.go
index fc29b9b6663..cf2028933d5 100644
--- a/server/lease/lessor.go
+++ b/server/lease/lessor.go
@@ -42,8 +42,8 @@ const MaxLeaseTTL = 9000000000
 var (
 	forever = time.Time{}
 
-	// maximum number of leases to revoke per second; configurable for tests
-	leaseRevokeRate = 1000
+	// default number of leases to revoke per second; configurable for tests
+	defaultLeaseRevokeRate = 1000
 
 	// maximum number of lease checkpoints recorded to the consensus log per second; configurable for tests
 	leaseCheckpointRate = 1000
@@ -170,6 +170,9 @@ type lessor struct {
 	// requests for shorter TTLs are extended to the minimum TTL.
 	minLeaseTTL int64
 
+	// maximum number of leases to revoke per second
+	leaseRevokeRate int
+
 	expiredC chan []*Lease
 	// stopC is a channel whose closure indicates that the lessor should be stopped.
 	stopC chan struct{}
@@ -198,6 +201,8 @@ type LessorConfig struct {
 	CheckpointInterval         time.Duration
 	ExpiredLeasesRetryInterval time.Duration
 	CheckpointPersist          bool
+
+	leaseRevokeRate int
 }
 
 func NewLessor(lg *zap.Logger, b backend.Backend, cluster cluster, cfg LessorConfig) Lessor {
@@ -207,12 +212,16 @@ func NewLessor(lg *zap.Logger, b backend.Backend, cluster cluster, cfg LessorCon
 func newLessor(lg *zap.Logger, b backend.Backend, cluster cluster, cfg LessorConfig) *lessor {
 	checkpointInterval := cfg.CheckpointInterval
 	expiredLeaseRetryInterval := cfg.ExpiredLeasesRetryInterval
+	leaseRevokeRate := cfg.leaseRevokeRate
 	if checkpointInterval == 0 {
 		checkpointInterval = defaultLeaseCheckpointInterval
 	}
 	if expiredLeaseRetryInterval == 0 {
 		expiredLeaseRetryInterval = defaultExpiredleaseRetryInterval
 	}
+	if leaseRevokeRate == 0 {
+		leaseRevokeRate = defaultLeaseRevokeRate
+	}
 	l := &lessor{
 		leaseMap:                  make(map[LeaseID]*Lease),
 		itemMap:                   make(map[LeaseItem]LeaseID),
@@ -220,6 +229,7 @@ func newLessor(lg *zap.Logger, b backend.Backend, cluster cluster, cfg LessorCon
 		leaseCheckpointHeap:       make(LeaseQueue, 0),
 		b:                         b,
 		minLeaseTTL:               cfg.MinLeaseTTL,
+		leaseRevokeRate:           leaseRevokeRate,
 		checkpointInterval:        checkpointInterval,
 		expiredLeaseRetryInterval: expiredLeaseRetryInterval,
 		checkpointPersist:         cfg.CheckpointPersist,
@@ -473,7 +483,7 @@ func (le *lessor) Promote(extend time.Duration) {
 		le.scheduleCheckpointIfNeeded(l)
 	}
 
-	if len(le.leaseMap) < leaseRevokeRate {
+	if len(le.leaseMap) < le.leaseRevokeRate {
 		// no possibility of lease pile-up
 		return
 	}
@@ -487,7 +497,7 @@ func (le *lessor) Promote(extend time.Duration) {
 	expires := 0
 	// have fewer expires than the total revoke rate so piled up leases
 	// don't consume the entire revoke limit
-	targetExpiresPerSecond := (3 * leaseRevokeRate) / 4
+	targetExpiresPerSecond := (3 * le.leaseRevokeRate) / 4
 	for _, l := range leases {
 		remaining := l.Remaining()
 		if remaining > nextWindow {
@@ -623,7 +633,7 @@ func (le *lessor) revokeExpiredLeases() {
 	var ls []*Lease
 
 	// rate limit
-	revokeLimit := leaseRevokeRate / 2
+	revokeLimit := le.leaseRevokeRate / 2
 
 	le.mu.RLock()
 	if le.isPrimary() {


Test patch:
diff --git a/server/lease/lessor_test.go b/server/lease/lessor_test.go
index 736909169da..0d32300abaf 100644
--- a/server/lease/lessor_test.go
+++ b/server/lease/lessor_test.go
@@ -307,17 +307,15 @@ func TestLessorRenewWithCheckpointer(t *testing.T) {
 // TestLessorRenewExtendPileup ensures Lessor extends leases on promotion if too many
 // expire at the same time.
 func TestLessorRenewExtendPileup(t *testing.T) {
-	oldRevokeRate := leaseRevokeRate
-	defer func() { leaseRevokeRate = oldRevokeRate }()
+	leaseRevokeRate := 10
 	lg := zap.NewNop()
-	leaseRevokeRate = 10
 
 	dir, be := NewTestBackend(t)
 	defer os.RemoveAll(dir)
 
-	le := newLessor(lg, be, clusterLatest(), LessorConfig{MinLeaseTTL: minLeaseTTL})
+	le := newLessor(lg, be, clusterLatest(), LessorConfig{MinLeaseTTL: minLeaseTTL, leaseRevokeRate: leaseRevokeRate})
 	ttl := int64(10)
-	for i := 1; i <= leaseRevokeRate*10; i++ {
+	for i := 1; i <= le.leaseRevokeRate*10; i++ {
 		if _, err := le.Grant(LeaseID(2*i), ttl); err != nil {
 			t.Fatal(err)
 		}
@@ -334,7 +332,7 @@ func TestLessorRenewExtendPileup(t *testing.T) {
 	bcfg.Path = filepath.Join(dir, "be")
 	be = backend.New(bcfg)
 	defer be.Close()
-	le = newLessor(lg, be, clusterLatest(), LessorConfig{MinLeaseTTL: minLeaseTTL})
+	le = newLessor(lg, be, clusterLatest(), LessorConfig{MinLeaseTTL: minLeaseTTL, leaseRevokeRate: leaseRevokeRate})
 	defer le.Stop()
 
 	// extend after recovery should extend expiration on lease pile-up
@@ -349,11 +347,11 @@ func TestLessorRenewExtendPileup(t *testing.T) {
 
 	for i := ttl; i < ttl+20; i++ {
 		c := windowCounts[i]
-		if c > leaseRevokeRate {
-			t.Errorf("expected at most %d expiring at %ds, got %d", leaseRevokeRate, i, c)
+		if c > le.leaseRevokeRate {
+			t.Errorf("expected at most %d expiring at %ds, got %d", le.leaseRevokeRate, i, c)
 		}
-		if c < leaseRevokeRate/2 {
-			t.Errorf("expected at least %d expiring at %ds, got %d", leaseRevokeRate/2, i, c)
+		if c < le.leaseRevokeRate/2 {
+			t.Errorf("expected at least %d expiring at %ds, got %d", le.leaseRevokeRate/2, i, c)
 		}
 	}
 }