dashboard/app: tell managers when dashboard needs a repro

Managers can't decide when a repro is required.
First, there can be bugs with seq>0.
Second, a repro can be already obtained on another manager.

dashboard/app/api.go

@@ -33,6 +33,7 @@ var apiHandlers = map[string]APIHandler{
 	"builder_poll":        apiBuilderPoll,
 	"report_crash":        apiReportCrash,
 	"report_failed_repro": apiReportFailedRepro,
+	"need_repro":          apiNeedRepro,
 	"reporting_poll":      apiReportingPoll,
 	"reporting_update":    apiReportingUpdate,
 }
@@ -420,7 +421,10 @@ func apiReportCrash(c context.Context, ns string, r *http.Request) (interface{},
 		return nil, err
 	}
 	purgeOldCrashes(c, bug, bugKey)
-	return nil, nil
+	resp := &dashapi.ReportCrashResp{
+		NeedRepro: needRepro(bug),
+	}
+	return resp, nil
 }
 
 func purgeOldCrashes(c context.Context, bug *Bug, bugKey *datastore.Key) {
@@ -473,7 +477,7 @@ func purgeOldCrashes(c context.Context, bug *Bug, bugKey *datastore.Key) {
 }
 
 func apiReportFailedRepro(c context.Context, ns string, r *http.Request) (interface{}, error) {
-	req := new(dashapi.FailedRepro)
+	req := new(dashapi.CrashID)
 	if err := json.NewDecoder(r.Body).Decode(req); err != nil {
 		return nil, fmt.Errorf("failed to unmarshal request: %v", err)
 	}
@@ -505,6 +509,37 @@ func apiReportFailedRepro(c context.Context, ns string, r *http.Request) (interf
 	return nil, nil
 }
 
+func apiNeedRepro(c context.Context, ns string, r *http.Request) (interface{}, error) {
+	req := new(dashapi.CrashID)
+	if err := json.NewDecoder(r.Body).Decode(req); err != nil {
+		return nil, fmt.Errorf("failed to unmarshal request: %v", err)
+	}
+	req.Title = limitLength(req.Title, maxTextLen)
+
+	bug := new(Bug)
+	for seq := int64(0); ; seq++ {
+		bugHash := bugKeyHash(ns, req.Title, seq)
+		bugKey := datastore.NewKey(c, "Bug", bugHash, 0, nil)
+		if err := datastore.Get(c, bugKey, bug); err != nil {
+			return nil, fmt.Errorf("failed to get bug: %v", err)
+		}
+		if bug.Status == BugStatusOpen || bug.Status == BugStatusDup {
+			break
+		}
+	}
+
+	resp := &dashapi.NeedReproResp{
+		NeedRepro: needRepro(bug),
+	}
+	return resp, nil
+}
+
+func needRepro(bug *Bug) bool {
+	return bug.ReproLevel < ReproLevelC &&
+		bug.NumRepro < 5 &&
+		len(bug.Commits) == 0
+}
+
 func putText(c context.Context, ns, tag string, data []byte, dedup bool) (int64, error) {
 	if ns == "" {
 		return 0, fmt.Errorf("putting text outside of namespace")

dashboard/app/app_test.go

@@ -98,6 +98,13 @@ func testCrash(build *dashapi.Build, id int) *dashapi.Crash {
 	}
 }
 
+func testCrashID(crash *dashapi.Crash) *dashapi.CrashID {
+	return &dashapi.CrashID{
+		BuildID: crash.BuildID,
+		Title:   crash.Title,
+	}
+}
+
 func TestApp(t *testing.T) {
 	c := NewCtx(t)
 	defer c.Close()
@@ -158,12 +165,11 @@ func TestApp(t *testing.T) {
 		c.expectOK(c.API(client1, key1, "report_crash", crash, nil))
 	}
 
-	repro := &dashapi.FailedRepro{
-		Manager: "manager1",
+	cid := &dashapi.CrashID{
 		BuildID: "build1",
 		Title:   "title1",
 	}
-	c.expectOK(c.API(client1, key1, "report_failed_repro", repro, nil))
+	c.expectOK(c.API(client1, key1, "report_failed_repro", cid, nil))
 
 	pr := &dashapi.PollRequest{
 		Type: "test",
@@ -179,3 +185,245 @@ func TestApp(t *testing.T) {
 	}
 	c.expectOK(c.API(client1, key1, "reporting_update", cmd, nil))
 }
+
+// Normal workflow:
+//  - upload crash -> need repro
+//  - upload syz repro -> still need repro
+//  - upload C repro -> don't need repro
+func testNeedRepro1(t *testing.T, crashCtor func(c *Ctx) *dashapi.Crash) {
+	c := NewCtx(t)
+	defer c.Close()
+
+	crash1 := crashCtor(c)
+	resp := new(dashapi.ReportCrashResp)
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, resp))
+	c.expectEQ(resp.NeedRepro, true)
+
+	cid := testCrashID(crash1)
+	needReproResp := new(dashapi.NeedReproResp)
+	c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+	c.expectEQ(needReproResp.NeedRepro, true)
+
+	// Still need repro for this crash.
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, resp))
+	c.expectEQ(resp.NeedRepro, true)
+	c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+	c.expectEQ(needReproResp.NeedRepro, true)
+
+	crash2 := new(dashapi.Crash)
+	*crash2 = *crash1
+	crash2.ReproOpts = []byte("opts")
+	crash2.ReproSyz = []byte("repro syz")
+	c.expectOK(c.API(client1, key1, "report_crash", crash2, resp))
+	c.expectEQ(resp.NeedRepro, true)
+	c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+	c.expectEQ(needReproResp.NeedRepro, true)
+
+	crash2.ReproC = []byte("repro C")
+	c.expectOK(c.API(client1, key1, "report_crash", crash2, resp))
+	c.expectEQ(resp.NeedRepro, false)
+	c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+	c.expectEQ(needReproResp.NeedRepro, false)
+
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, resp))
+	c.expectEQ(resp.NeedRepro, false)
+}
+
+func TestNeedRepro1_normal(t *testing.T)      { testNeedRepro1(t, normalCrash) }
+func TestNeedRepro1_dup(t *testing.T)         { testNeedRepro1(t, dupCrash) }
+func TestNeedRepro1_closed(t *testing.T)      { testNeedRepro1(t, closedCrash) }
+func TestNeedRepro1_closedRepro(t *testing.T) { testNeedRepro1(t, closedWithReproCrash) }
+
+// Upload C repro with first crash -> don't need repro.
+func testNeedRepro2(t *testing.T, crashCtor func(c *Ctx) *dashapi.Crash) {
+	c := NewCtx(t)
+	defer c.Close()
+
+	crash1 := crashCtor(c)
+	crash1.ReproOpts = []byte("opts")
+	crash1.ReproSyz = []byte("repro syz")
+	crash1.ReproC = []byte("repro C")
+	resp := new(dashapi.ReportCrashResp)
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, resp))
+	c.expectEQ(resp.NeedRepro, false)
+
+	cid := testCrashID(crash1)
+	needReproResp := new(dashapi.NeedReproResp)
+	c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+	c.expectEQ(needReproResp.NeedRepro, false)
+}
+
+func TestNeedRepro2_normal(t *testing.T)      { testNeedRepro2(t, normalCrash) }
+func TestNeedRepro2_dup(t *testing.T)         { testNeedRepro2(t, dupCrash) }
+func TestNeedRepro2_closed(t *testing.T)      { testNeedRepro2(t, closedCrash) }
+func TestNeedRepro2_closedRepro(t *testing.T) { testNeedRepro2(t, closedWithReproCrash) }
+
+// Test that after uploading 5 failed repros, app stops requesting repros.
+func testNeedRepro3(t *testing.T, crashCtor func(c *Ctx) *dashapi.Crash) {
+	c := NewCtx(t)
+	defer c.Close()
+
+	crash1 := crashCtor(c)
+	resp := new(dashapi.ReportCrashResp)
+	cid := testCrashID(crash1)
+	needReproResp := new(dashapi.NeedReproResp)
+
+	for i := 0; i < 5; i++ {
+		c.expectOK(c.API(client1, key1, "report_crash", crash1, resp))
+		c.expectEQ(resp.NeedRepro, true)
+
+		c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+		c.expectEQ(needReproResp.NeedRepro, true)
+
+		c.expectOK(c.API(client1, key1, "report_failed_repro", cid, nil))
+	}
+
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, resp))
+	c.expectEQ(resp.NeedRepro, false)
+
+	c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+	c.expectEQ(needReproResp.NeedRepro, false)
+}
+
+func TestNeedRepro3_normal(t *testing.T)      { testNeedRepro3(t, normalCrash) }
+func TestNeedRepro3_dup(t *testing.T)         { testNeedRepro3(t, dupCrash) }
+func TestNeedRepro3_closed(t *testing.T)      { testNeedRepro3(t, closedCrash) }
+func TestNeedRepro3_closedRepro(t *testing.T) { testNeedRepro3(t, closedWithReproCrash) }
+
+// Test that after uploading 5 syz repros, app stops requesting repros.
+func testNeedRepro4(t *testing.T, crashCtor func(c *Ctx) *dashapi.Crash) {
+	c := NewCtx(t)
+	defer c.Close()
+
+	crash1 := crashCtor(c)
+	crash1.ReproOpts = []byte("opts")
+	crash1.ReproSyz = []byte("repro syz")
+	resp := new(dashapi.ReportCrashResp)
+	cid := testCrashID(crash1)
+	needReproResp := new(dashapi.NeedReproResp)
+
+	for i := 0; i < 4; i++ {
+		c.expectOK(c.API(client1, key1, "report_crash", crash1, resp))
+		c.expectEQ(resp.NeedRepro, true)
+
+		c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+		c.expectEQ(needReproResp.NeedRepro, true)
+	}
+
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, resp))
+	c.expectEQ(resp.NeedRepro, false)
+
+	c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+	c.expectEQ(needReproResp.NeedRepro, false)
+}
+
+func TestNeedRepro4_normal(t *testing.T)      { testNeedRepro4(t, normalCrash) }
+func TestNeedRepro4_dup(t *testing.T)         { testNeedRepro4(t, dupCrash) }
+func TestNeedRepro4_closed(t *testing.T)      { testNeedRepro4(t, closedCrash) }
+func TestNeedRepro4_closedRepro(t *testing.T) { testNeedRepro4(t, closedWithReproCrash) }
+
+func testNeedRepro5(t *testing.T, crashCtor func(c *Ctx) *dashapi.Crash) {
+	c := NewCtx(t)
+	defer c.Close()
+
+	crash1 := crashCtor(c)
+	crash1.ReproOpts = []byte("opts")
+	crash1.ReproSyz = []byte("repro syz")
+	resp := new(dashapi.ReportCrashResp)
+	cid := testCrashID(crash1)
+	needReproResp := new(dashapi.NeedReproResp)
+
+	for i := 0; i < 4; i++ {
+		c.expectOK(c.API(client1, key1, "report_crash", crash1, resp))
+		c.expectEQ(resp.NeedRepro, true)
+
+		c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+		c.expectEQ(needReproResp.NeedRepro, true)
+	}
+
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, resp))
+	c.expectEQ(resp.NeedRepro, false)
+
+	c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+	c.expectEQ(needReproResp.NeedRepro, false)
+}
+
+func TestNeedRepro5_normal(t *testing.T)      { testNeedRepro5(t, normalCrash) }
+func TestNeedRepro5_dup(t *testing.T)         { testNeedRepro5(t, dupCrash) }
+func TestNeedRepro5_closed(t *testing.T)      { testNeedRepro5(t, closedCrash) }
+func TestNeedRepro5_closedRepro(t *testing.T) { testNeedRepro5(t, closedWithReproCrash) }
+
+func normalCrash(c *Ctx) *dashapi.Crash {
+	build := testBuild(1)
+	c.expectOK(c.API(client1, key1, "upload_build", build, nil))
+	return testCrash(build, 1)
+}
+
+func dupCrash(c *Ctx) *dashapi.Crash {
+	build := testBuild(1)
+	c.expectOK(c.API(client1, key1, "upload_build", build, nil))
+
+	crash1 := testCrash(build, 1)
+	c.expectOK(c.API(client1, key1, "report_crash", crash1, nil))
+
+	crash2 := testCrash(build, 2)
+	c.expectOK(c.API(client1, key1, "report_crash", crash2, nil))
+
+	pr := &dashapi.PollRequest{
+		Type: "test",
+	}
+	resp := new(dashapi.PollResponse)
+	c.expectOK(c.API(client1, key1, "reporting_poll", pr, resp))
+	c.expectEQ(len(resp.Reports), 2)
+	rep1 := resp.Reports[0]
+	rep2 := resp.Reports[1]
+	cmd := &dashapi.BugUpdate{
+		ID:     rep2.ID,
+		Status: dashapi.BugStatusDup,
+		DupOf:  rep1.ID,
+	}
+	reply := new(dashapi.BugUpdateReply)
+	c.expectOK(c.API(client1, key1, "reporting_update", cmd, reply))
+	c.expectEQ(reply.OK, true)
+
+	return crash2
+}
+
+func closedCrash(c *Ctx) *dashapi.Crash {
+	return closedCrashImpl(c, false)
+}
+
+func closedWithReproCrash(c *Ctx) *dashapi.Crash {
+	return closedCrashImpl(c, true)
+}
+
+func closedCrashImpl(c *Ctx, withRepro bool) *dashapi.Crash {
+	build := testBuild(1)
+	c.expectOK(c.API(client1, key1, "upload_build", build, nil))
+
+	crash := testCrash(build, 1)
+	if withRepro {
+		crash.ReproC = []byte("repro C")
+	}
+	resp := new(dashapi.ReportCrashResp)
+	c.expectOK(c.API(client1, key1, "report_crash", crash, resp))
+	c.expectEQ(resp.NeedRepro, !withRepro)
+
+	pr := &dashapi.PollRequest{
+		Type: "test",
+	}
+	pollResp := new(dashapi.PollResponse)
+	c.expectOK(c.API(client1, key1, "reporting_poll", pr, pollResp))
+	c.expectEQ(len(pollResp.Reports), 1)
+	rep := pollResp.Reports[0]
+	cmd := &dashapi.BugUpdate{
+		ID:     rep.ID,
+		Status: dashapi.BugStatusInvalid,
+	}
+	reply := new(dashapi.BugUpdateReply)
+	c.expectOK(c.API(client1, key1, "reporting_update", cmd, reply))
+	c.expectEQ(reply.OK, true)
+
+	crash.ReproC = nil
+	return crash
+}

dashboard/app/config_stub.go

@@ -2,6 +2,7 @@
 // Use of this source code is governed by Apache 2 LICENSE that can be found in the LICENSE file.
 
 // +build !aetest
+// +build
 
 package dash
 

dashboard/app/fix_test.go

@@ -49,6 +49,11 @@ func TestFixBasic(t *testing.T) {
 	c.expectOK(c.API(client1, key1, "builder_poll", builderPollReq, builderPollResp))
 	c.expectEQ(len(builderPollResp.PendingCommits), 0)
 
+	cid := testCrashID(crash1)
+	needReproResp := new(dashapi.NeedReproResp)
+	c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+	c.expectEQ(needReproResp.NeedRepro, true)
+
 	reports := reportAllBugs(c, 1)
 	rep := reports[0]
 
@@ -62,6 +67,10 @@ func TestFixBasic(t *testing.T) {
 	c.expectOK(c.API(client1, key1, "reporting_update", cmd, reply))
 	c.expectEQ(reply.OK, true)
 
+	// Don't need repro once there are fixing commits.
+	c.expectOK(c.API(client1, key1, "need_repro", cid, needReproResp))
+	c.expectEQ(needReproResp.NeedRepro, false)
+
 	// Check that the commit is now passed to builders.
 	c.expectOK(c.API(client1, key1, "builder_poll", builderPollReq, builderPollResp))
 	c.expectEQ(len(builderPollResp.PendingCommits), 1)

dashboard/app/reporting_test.go

@@ -192,12 +192,11 @@ func TestInvalidBug(t *testing.T) {
 	}
 	c.expectEQ(rep, want)
 
-	repro := &dashapi.FailedRepro{
-		Manager: build.Manager,
+	cid := &dashapi.CrashID{
 		BuildID: build.ID,
 		Title:   crash1.Title,
 	}
-	c.expectOK(c.API(client1, key1, "report_failed_repro", repro, nil))
+	c.expectOK(c.API(client1, key1, "report_failed_repro", cid, nil))
 }
 
 func TestReportingQuota(t *testing.T) {

dashboard/dashapi/dashapi.go

@@ -85,19 +85,36 @@ type Crash struct {
 	ReproC    []byte
 }
 
-func (dash *Dashboard) ReportCrash(crash *Crash) error {
-	return dash.query("report_crash", crash, nil)
+type ReportCrashResp struct {
+	NeedRepro bool
 }
 
-// FailedRepro describes a failed repro attempt.
-type FailedRepro struct {
-	Manager string
+func (dash *Dashboard) ReportCrash(crash *Crash) (*ReportCrashResp, error) {
+	resp := new(ReportCrashResp)
+	err := dash.query("report_crash", crash, resp)
+	return resp, err
+}
+
+// CrashID is a short summary of a crash for repro queires.
+type CrashID struct {
 	BuildID string
 	Title   string
 }
 
-func (dash *Dashboard) ReportFailedRepro(repro *FailedRepro) error {
-	return dash.query("report_failed_repro", repro, nil)
+type NeedReproResp struct {
+	NeedRepro bool
+}
+
+// NeedRepro checks if dashboard needs a repro for this crash or not.
+func (dash *Dashboard) NeedRepro(crash *CrashID) (bool, error) {
+	resp := new(NeedReproResp)
+	err := dash.query("need_repro", crash, resp)
+	return resp.NeedRepro, err
+}
+
+// ReportFailedRepro notifies dashboard about a failed repro attempt for the crash.
+func (dash *Dashboard) ReportFailedRepro(crash *CrashID) error {
+	return dash.query("report_failed_repro", crash, nil)
 }
 
 type LogEntry struct {

syz-manager/manager.go

@@ -347,8 +347,24 @@ func (mgr *Manager) vmLoop() {
 				continue
 			}
 			delete(pendingRepro, crash)
-			if !crash.hub && !mgr.needRepro(crash.desc) {
-				continue
+			if !crash.hub {
+				if mgr.dash == nil {
+					if !mgr.needRepro(crash.desc) {
+						continue
+					}
+				} else {
+					cid := &dashapi.CrashID{
+						BuildID: mgr.cfg.Tag,
+						Title:   crash.desc,
+					}
+					needRepro, err := mgr.dash.NeedRepro(cid)
+					if err != nil {
+						Logf(0, "dashboard.NeedRepro failed: %v", err)
+					}
+					if !needRepro {
+						continue
+					}
+				}
 			}
 			Logf(1, "loop: add to repro queue '%v'", crash.desc)
 			reproducing[crash.desc] = true
@@ -414,8 +430,8 @@ func (mgr *Manager) vmLoop() {
 			// On shutdown qemu crashes with "qemu: terminating on signal 2",
 			// which we detect as "lost connection". Don't save that as crash.
 			if shutdown != nil && res.crash != nil && !mgr.isSuppressed(res.crash) {
-				mgr.saveCrash(res.crash)
-				if mgr.needRepro(res.crash.desc) {
+				needRepro := mgr.saveCrash(res.crash)
+				if needRepro {
 					Logf(1, "loop: add pending repro for '%v'", res.crash.desc)
 					pendingRepro[res.crash] = true
 				}
@@ -529,7 +545,7 @@ func (mgr *Manager) isSuppressed(crash *Crash) bool {
 	return false
 }
 
-func (mgr *Manager) saveCrash(crash *Crash) {
+func (mgr *Manager) saveCrash(crash *Crash) bool {
 	Logf(0, "vm-%v: crash: %v", crash.vmIndex, crash.desc)
 	mgr.mu.Lock()
 	mgr.stats["crashes"]++
@@ -557,12 +573,13 @@ func (mgr *Manager) saveCrash(crash *Crash) {
 			Log:         crash.log,
 			Report:      crash.report,
 		}
-		if err := mgr.dash.ReportCrash(dc); err != nil {
+		resp, err := mgr.dash.ReportCrash(dc)
+		if err != nil {
 			Logf(0, "failed to report crash to dashboard: %v", err)
 		} else {
 			// Don't store the crash locally, if we've successfully
 			// uploaded it to the dashboard. These will just eat disk space.
-			return
+			return resp.NeedRepro
 		}
 	}
 
@@ -596,6 +613,8 @@ func (mgr *Manager) saveCrash(crash *Crash) {
 	if len(crash.report) > 0 {
 		osutil.WriteFile(filepath.Join(dir, fmt.Sprintf("report%v", oldestI)), crash.report)
 	}
+
+	return mgr.needRepro(crash.desc)
 }
 
 const maxReproAttempts = 3
@@ -619,12 +638,11 @@ func (mgr *Manager) needRepro(desc string) bool {
 
 func (mgr *Manager) saveFailedRepro(desc string) {
 	if mgr.dash != nil {
-		fr := &dashapi.FailedRepro{
-			Manager: mgr.cfg.Name,
+		cid := &dashapi.CrashID{
 			BuildID: mgr.cfg.Tag,
 			Title:   desc,
 		}
-		if err := mgr.dash.ReportFailedRepro(fr); err != nil {
+		if err := mgr.dash.ReportFailedRepro(cid); err != nil {
 			Logf(0, "failed to report failed repro to dashboard: %v", err)
 		}
 	}
@@ -707,7 +725,7 @@ func (mgr *Manager) saveRepro(res *repro.Result, hub bool) {
 			ReproSyz:    []byte(res.Prog.Serialize()),
 			ReproC:      cprogText,
 		}
-		if err := mgr.dash.ReportCrash(dc); err != nil {
+		if _, err := mgr.dash.ReportCrash(dc); err != nil {
 			Logf(0, "failed to report repro to dashboard: %v", err)
 		}
 	}