From a84dec47f0bdc461828c2903429e669fc0fa5e10 Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Wed, 31 Jan 2018 16:15:02 +0100 Subject: 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. --- dashboard/app/jobs.go | 29 +++++++++++++++++++++-------- dashboard/app/jobs_test.go | 14 +++++++++++--- dashboard/app/reporting_email.go | 14 +++----------- dashboard/app/util_test.go | 17 ++++++++++++++--- 4 files changed, 49 insertions(+), 25 deletions(-) (limited to 'dashboard') 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)) } -- cgit mrf-deployment