Merge branch 'wc/tracker-error-race' into 'master'

tracker: Fix racey error insertion in test

Closes #4796

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5391

Merged-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: Quang-Minh Nguyen <qmnguyen@gitlab.com>
Co-authored-by: Will Chandler <wchandler@gitlab.com>

2 files changed, 18 insertions, 7 deletions

diff --git a/internal/praefect/nodes/tracker/errors.go b/internal/praefect/nodes/tracker/errors.go
index 8a87d30cc..c7f8236be 100644
--- a/internal/praefect/nodes/tracker/errors.go
+++ b/internal/praefect/nodes/tracker/errors.go
@@ -75,6 +75,10 @@ func NewErrors(ctx context.Context, isInErrorWindow ErrorWindowFunction, readThr
 
 // IncrReadErr increases the read errors for a node by 1
 func (e *errorTracker) IncrReadErr(node string) {
+	e.incrReadErrTime(node, time.Now())
+}
+
+func (e *errorTracker) incrReadErrTime(node string, t time.Time) {
 	select {
 	case <-e.ctx.Done():
 		return
@@ -82,7 +86,7 @@ func (e *errorTracker) IncrReadErr(node string) {
 		e.m.Lock()
 		defer e.m.Unlock()
 
-		e.readErrors[node] = append(e.readErrors[node], time.Now())
+		e.readErrors[node] = append(e.readErrors[node], t)
 
 		if len(e.readErrors[node]) > e.readThreshold {
 			e.readErrors[node] = e.readErrors[node][1:]
@@ -92,6 +96,10 @@ func (e *errorTracker) IncrReadErr(node string) {
 
 // IncrWriteErr increases the read errors for a node by 1
 func (e *errorTracker) IncrWriteErr(node string) {
+	e.incrWriteErrTime(node, time.Now())
+}
+
+func (e *errorTracker) incrWriteErrTime(node string, t time.Time) {
 	select {
 	case <-e.ctx.Done():
 		return
@@ -99,7 +107,7 @@ func (e *errorTracker) IncrWriteErr(node string) {
 		e.m.Lock()
 		defer e.m.Unlock()
 
-		e.writeErrors[node] = append(e.writeErrors[node], time.Now())
+		e.writeErrors[node] = append(e.writeErrors[node], t)
 
 		if len(e.writeErrors[node]) > e.writeThreshold {
 			e.writeErrors[node] = e.writeErrors[node][1:]
diff --git a/internal/praefect/nodes/tracker/errors_test.go b/internal/praefect/nodes/tracker/errors_test.go
index 81e4a8e3b..61576ac09 100644
--- a/internal/praefect/nodes/tracker/errors_test.go
+++ b/internal/praefect/nodes/tracker/errors_test.go
@@ -90,18 +90,21 @@ func TestErrorTracker_ClearErrors(t *testing.T) {
 
 	node := "backend-node-1"
 
-	errors.IncrWriteErr(node)
-	errors.IncrReadErr(node)
-
 	now := time.Now()
+	beforeNow := now.Add(-10 * time.Millisecond)
+	afterNow := now.Add(10 * time.Millisecond)
+
+	errors.incrWriteErrTime(node, beforeNow)
+	errors.incrReadErrTime(node, beforeNow)
+
 	errors.isInErrorWindow = func(_ time.Time, errTime time.Time) bool {
 		// Consider all errors which have been added until now to not be part of the error
 		// window anymore. All new events will be considered part of it though.
 		return errTime.After(now)
 	}
 
-	errors.IncrWriteErr(node)
-	errors.IncrReadErr(node)
+	errors.incrWriteErrTime(node, afterNow)
+	errors.incrReadErrTime(node, afterNow)
 
 	errors.clear()
 	assert.Len(t, errors.readErrors[node], 1, "clear should only have cleared the read error older than the time specified")