From 8cf7b72368903bf5225e088dd330cc47e34d8bb5 Mon Sep 17 00:00:00 2001
From: Aleksandr Nogikh <nogikh@google.com>
Date: Wed, 4 Sep 2024 16:01:38 +0200
Subject: pkg/manager: give preference to never reproduced crashes

First consider how many times we've tried to reproduce the crash, and
only then look at other criteria.

This will make the reproduction process more just since in the presence
of multiple crashes in the queue we will give at least one chance to a
wider range of different crashes.
---
 pkg/manager/repro.go      | 22 ++++++++++++++++------
 pkg/manager/repro_test.go | 21 +++++++++++++--------
 2 files changed, 29 insertions(+), 14 deletions(-)

(limited to 'pkg')

diff --git a/pkg/manager/repro.go b/pkg/manager/repro.go
index 7a50f06ab..038980624 100644
--- a/pkg/manager/repro.go
+++ b/pkg/manager/repro.go
@@ -64,7 +64,8 @@ type ReproLoop struct {
 	mu          sync.Mutex
 	queue       []*Crash
 	reproducing map[string]bool
-	attempted   map[string]bool
+	enqueued    map[string]bool
+	attempts    map[string]int
 }
 
 func NewReproLoop(mgr ReproManagerView, reproVMs int, onlyOnce bool) *ReproLoop {
@@ -75,7 +76,8 @@ func NewReproLoop(mgr ReproManagerView, reproVMs int, onlyOnce bool) *ReproLoop
 		reproVMs:    reproVMs,
 		reproducing: map[string]bool{},
 		pingQueue:   make(chan struct{}, 1),
-		attempted:   map[string]bool{},
+		enqueued:    map[string]bool{},
+		attempts:    map[string]int{},
 	}
 	ret.statNumReproducing = stat.New("reproducing", "Number of crashes being reproduced",
 		stat.Console, stat.NoGraph, func() int {
@@ -133,7 +135,7 @@ func (r *ReproLoop) Enqueue(crash *Crash) {
 	defer r.mu.Unlock()
 
 	title := crash.FullTitle()
-	if r.onlyOnce && r.attempted[title] {
+	if r.onlyOnce && r.enqueued[title] {
 		// Try to reproduce each bug at most 1 time in this mode.
 		// Since we don't upload bugs/repros to dashboard, it likely won't have
 		// the reproducer even if we succeeded last time, and will repeatedly
@@ -141,7 +143,7 @@ func (r *ReproLoop) Enqueue(crash *Crash) {
 		return
 	}
 	log.Logf(1, "scheduled a reproduction of '%v'", title)
-	r.attempted[title] = true
+	r.enqueued[title] = true
 	r.queue = append(r.queue, crash)
 
 	// Ping the loop.
@@ -156,6 +158,12 @@ func (r *ReproLoop) popCrash() *Crash {
 	defer r.mu.Unlock()
 
 	newBetter := func(base, new *Crash) bool {
+		// The more times we failed, the less likely we are to actually
+		// find a reproducer. Give preference to not yet attempted repro runs.
+		baseTitle, newTitle := base.FullTitle(), new.FullTitle()
+		if r.attempts[baseTitle] != r.attempts[newTitle] {
+			return r.attempts[newTitle] < r.attempts[baseTitle]
+		}
 		// First, serve manual requests.
 		if new.Manual != base.Manual {
 			return new.Manual
@@ -213,8 +221,10 @@ func (r *ReproLoop) Loop(ctx context.Context) {
 			return
 		}
 
+		title := crash.FullTitle()
 		r.mu.Lock()
-		r.reproducing[crash.FullTitle()] = true
+		r.attempts[title]++
+		r.reproducing[title] = true
 		r.adjustPoolSizeLocked()
 		r.mu.Unlock()
 
@@ -225,7 +235,7 @@ func (r *ReproLoop) Loop(ctx context.Context) {
 			r.handle(crash)
 
 			r.mu.Lock()
-			delete(r.reproducing, crash.FullTitle())
+			delete(r.reproducing, title)
 			r.adjustPoolSizeLocked()
 			r.mu.Unlock()
 
diff --git a/pkg/manager/repro_test.go b/pkg/manager/repro_test.go
index 0149aafdb..3433341d0 100644
--- a/pkg/manager/repro_test.go
+++ b/pkg/manager/repro_test.go
@@ -63,7 +63,12 @@ func TestReproOrder(t *testing.T) {
 	mock := &reproMgrMock{
 		run: make(chan runCallback),
 	}
-	obj := NewReproLoop(mock, 3, false)
+	obj := NewReproLoop(mock, 1, false)
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	go func() {
+		obj.Loop(ctx)
+	}()
 
 	// The right order is A B C.
 	crashes := []*Crash{
@@ -85,16 +90,16 @@ func TestReproOrder(t *testing.T) {
 	obj.Enqueue(crashes[2])
 	obj.Enqueue(crashes[1])
 	obj.Enqueue(crashes[0])
-	assert.Equal(t, crashes[0], obj.popCrash())
-	assert.Equal(t, crashes[1], obj.popCrash())
-	assert.Equal(t, crashes[2], obj.popCrash())
-
 	obj.Enqueue(crashes[1])
 	obj.Enqueue(crashes[0])
 	obj.Enqueue(crashes[2])
-	assert.Equal(t, crashes[0], obj.popCrash())
-	assert.Equal(t, crashes[1], obj.popCrash())
-	assert.Equal(t, crashes[2], obj.popCrash())
+
+	obj.StartReproduction()
+	for i := 0; i < len(crashes)*2; i++ {
+		called := <-mock.run
+		assert.Equal(t, crashes[i%len(crashes)], called.crash)
+		called.ret <- &ReproResult{}
+	}
 }
 
 func TestReproRWRace(t *testing.T) {
-- 
cgit mrf-deployment