dashboard/app: properly handle job request dups

Dups always have the same Message-ID,
and that's more reliable than looking at From/Sender.
So use Message-ID for deduplication.

4 files changed, 49 insertions, 25 deletions

diff --git a/dashboard/app/jobs.go b/dashboard/app/jobs.go
index 4574a333e..01c77c005 100644
--- a/dashboard/app/jobs.go
+++ b/dashboard/app/jobs.go
@@ -20,7 +20,7 @@ import (
 
 // handleTestRequest added new job to datastore.
 // Returns empty string if job added successfully, or reason why it wasn't added.
-func handleTestRequest(c context.Context, bugID, user, extID, patch, repo, branch string) string {
+func handleTestRequest(c context.Context, bugID, user, extID, link, patch, repo, branch string) string {
 	log.Infof(c, "test request: bug=%q user=%q extID=%q patch=%v, repo=%q branch=%q",
 		bugID, user, extID, len(patch), repo, branch)
 	for _, blacklisted := range config.EmailBlacklist {
@@ -29,7 +29,7 @@ func handleTestRequest(c context.Context, bugID, user, extID, patch, repo, branc
 			return ""
 		}
 	}
-	reply, err := addTestJob(c, bugID, user, extID, patch, repo, branch)
+	reply, err := addTestJob(c, bugID, user, extID, link, patch, repo, branch)
 	if err != nil {
 		log.Errorf(c, "test request failed: %v", err)
 		if reply == "" {
@@ -39,7 +39,15 @@ func handleTestRequest(c context.Context, bugID, user, extID, patch, repo, branc
 	return reply
 }
 
-func addTestJob(c context.Context, bugID, user, extID, patch, repo, branch string) (string, error) {
+func addTestJob(c context.Context, bugID, user, extID, link, patch, repo, branch string) (string, error) {
+	// We can get 2 emails for the same request: one direct and one from a mailing list.
+	// Filter out such duplicates (for dup we only need link update).
+	if exists, err := updateExistingJob(c, extID, link); err != nil {
+		return internalError, err
+	} else if exists {
+		return "", nil
+	}
+
 	bug, bugKey, err := findBugByReportingID(c, bugID)
 	if err != nil {
 		return "can't find associated bug", err
@@ -89,6 +97,7 @@ func addTestJob(c context.Context, bugID, user, extID, patch, repo, branch strin
 		User:         user,
 		Reporting:    bugReporting.Name,
 		ExtID:        extID,
+		Link:         link,
 		Namespace:    bug.Namespace,
 		Manager:      manager,
 		BugTitle:     bug.displayTitle(),
@@ -128,17 +137,20 @@ func addTestJob(c context.Context, bugID, user, extID, patch, repo, branch strin
 	return "", nil
 }
 
-func updateTestJob(c context.Context, extID, link string) error {
+func updateExistingJob(c context.Context, extID, link string) (exists bool, err error) {
 	var jobs []*Job
 	keys, err := datastore.NewQuery("Job").
 		Filter("ExtID=", extID).
 		GetAll(c, &jobs)
-	if len(jobs) != 1 || err != nil {
-		return fmt.Errorf("failed to query jobs: jobs=%v err=%v", len(jobs), err)
+	if len(jobs) > 1 || err != nil {
+		return false, fmt.Errorf("failed to query jobs: jobs=%v err=%v", len(jobs), err)
+	}
+	if len(jobs) == 0 {
+		return false, nil
 	}
 	job, jobKey := jobs[0], keys[0]
 	if job.Link != "" {
-		return nil
+		return true, nil
 	}
 	tx := func(c context.Context) error {
 		job := new(Job)
@@ -151,7 +163,8 @@ func updateTestJob(c context.Context, extID, link string) error {
 		}
 		return nil
 	}
-	return datastore.RunInTransaction(c, tx, nil)
+	err = datastore.RunInTransaction(c, tx, nil)
+	return true, err
 }
 
 // pollPendingJobs returns the next job to execute for the provided list of managers.
diff --git a/dashboard/app/jobs_test.go b/dashboard/app/jobs_test.go
index f4a5a16be..9a0e98102 100644
--- a/dashboard/app/jobs_test.go
+++ b/dashboard/app/jobs_test.go
@@ -77,7 +77,12 @@ func TestJob(t *testing.T) {
 	c.expectOK(c.API(client2, key2, "job_poll", &dashapi.JobPollReq{[]string{build.Manager}}, pollResp))
 	c.expectEQ(pollResp.ID, "")
 
-	c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch)
+	c.incomingEmailID(1, sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch)
+	c.expectOK(c.GET("/email_poll"))
+	c.expectEQ(len(c.emailSink), 0)
+
+	// A dup of the same request with the same Message-ID.
+	c.incomingEmailID(1, sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch)
 	c.expectOK(c.GET("/email_poll"))
 	c.expectEQ(len(c.emailSink), 0)
 
@@ -143,7 +148,7 @@ Raw console output is attached.
 			t.Fatalf("got email body:\n%s\n\nwant:\n%s", msg.Body, body)
 		}
 	}
-	c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch)
+	c.incomingEmailID(2, sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch)
 	c.expectOK(c.API(client2, key2, "job_poll", &dashapi.JobPollReq{[]string{build.Manager}}, pollResp))
 	jobDoneReq = &dashapi.JobDoneReq{
 		ID:    pollResp.ID,
@@ -182,7 +187,7 @@ Kernel config is attached.
 		}
 	}
 
-	c.incomingEmail(sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch)
+	c.incomingEmailID(3, sender, "#syz test: git://git.git/git.git kernel-branch\n"+patch)
 	c.expectOK(c.API(client2, key2, "job_poll", &dashapi.JobPollReq{[]string{build.Manager}}, pollResp))
 	jobDoneReq = &dashapi.JobDoneReq{
 		ID:    pollResp.ID,
@@ -228,4 +233,7 @@ correction.
 			t.Fatalf("got email body:\n%s\n\nwant:\n%s", msg.Body, body)
 		}
 	}
+
+	c.expectOK(c.API(client2, key2, "job_poll", &dashapi.JobPollReq{[]string{build.Manager}}, pollResp))
+	c.expectEQ(pollResp.ID, "")
 }
diff --git a/dashboard/app/reporting_email.go b/dashboard/app/reporting_email.go
index 0a0f1ed10..38327b072 100644
--- a/dashboard/app/reporting_email.go
+++ b/dashboard/app/reporting_email.go
@@ -275,28 +275,20 @@ func incomingMail(c context.Context, r *http.Request) error {
 		return nil // error was already logged
 	}
 	emailConfig := reporting.Config.(*EmailConfig)
+	// A mailing list can send us a duplicate email, to not process/reply
+	// to such duplicate emails, we ignore emails coming from our mailing lists.
 	mailingList := email.CanonicalEmail(emailConfig.Email)
 	fromMailingList := email.CanonicalEmail(msg.From) == mailingList
 	mailingListInCC := checkMailingListInCC(c, msg, mailingList)
 	log.Infof(c, "from/cc mailing list: %v/%v", fromMailingList, mailingListInCC)
-	// A mailing list can send us a duplicate email, to not process/reply
-	// to such duplicate emails, we ignore emails coming from our mailing lists.
 	if msg.Command == "test:" {
-		if fromMailingList {
-			if msg.Link != "" {
-				if err := updateTestJob(c, msg.MessageID, msg.Link); err != nil {
-					log.Errorf(c, "failed to update job: %v", err)
-				}
-			}
-			return nil
-		}
 		args := strings.Split(msg.CommandArgs, " ")
 		if len(args) != 2 {
 			return replyTo(c, msg, fmt.Sprintf("want 2 args (repo, branch), got %v",
 				len(args)), nil)
 		}
 		reply := handleTestRequest(c, msg.BugID, email.CanonicalEmail(msg.From),
-			msg.MessageID, msg.Patch, args[0], args[1])
+			msg.MessageID, msg.Link, msg.Patch, args[0], args[1])
 		if reply != "" {
 			return replyTo(c, msg, reply, nil)
 		}
diff --git a/dashboard/app/util_test.go b/dashboard/app/util_test.go
index 0df38a002..d534fd10e 100644
--- a/dashboard/app/util_test.go
+++ b/dashboard/app/util_test.go
@@ -179,13 +179,24 @@ func (c *Ctx) httpRequest(method, url, body string) error {
 }
 
 func (c *Ctx) incomingEmail(to, body string) {
-	c.incomingEmailFrom("default@sender.com", to, body)
+	c.incomingEmailImpl(0, "", to, body)
 }
 
 func (c *Ctx) incomingEmailFrom(from, to, body string) {
+	c.incomingEmailImpl(0, from, to, body)
+}
+
+func (c *Ctx) incomingEmailID(id int, to, body string) {
+	c.incomingEmailImpl(id, "", to, body)
+}
+
+func (c *Ctx) incomingEmailImpl(id int, from, to, body string) {
+	if from == "" {
+		from = "default@sender.com"
+	}
 	email := fmt.Sprintf(`Sender: %v
 Date: Tue, 15 Aug 2017 14:59:00 -0700
-Message-ID: <1234>
+Message-ID: <%v>
 Subject: crash1
 From: %v
 Cc: test@syzkaller.com, bugs@syzkaller.com
@@ -193,7 +204,7 @@ To: %v
 Content-Type: text/plain
 
 %v
-`, from, from, to, body)
+`, from, id, from, to, body)
 	c.expectOK(c.POST("/_ah/mail/", email))
 }