From 46e9d8e060515275078b76da523f1bec1e1a08cc Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Tue, 7 Oct 2014 15:37:31 -0400 Subject: [PATCH] dashboard/app: ignore, try not to create partial Commits It looks like the partial Commits are coming from the build breakages mails. If you have commit A newer than commit B, then there are two code paths depending on which reports its build result first. For slow development, B finishes before A is committed, so when A notices a failure it checks to see if B was okay. That code path seems to be okay. For submit of back-to-back changes, typically A finishes before B, so when B notices an okay it checks to see if A failed. That code path seems to zero the Commit for A while trying to set its FailNotificationSent to true. It does (did) succeed in setting FailNotificationSent to true, it just zeroed everything else. This CL adds code to refuse to do the datastore.Put to update FailNotificationSent if the Commit info is incomplete. It also tries to ignore Num=0 records, but that may not be as important anymore. LGTM=cmang R=cmang CC=golang-codereviews https://golang.org/cl/154080043 --- dashboard/app/build/handler.go | 8 +++++++- dashboard/app/build/notify.go | 8 ++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/dashboard/app/build/handler.go b/dashboard/app/build/handler.go index 94504766..7834afa4 100644 --- a/dashboard/app/build/handler.go +++ b/dashboard/app/build/handler.go @@ -46,7 +46,13 @@ func commitHandler(r *http.Request) (interface{}, error) { if r.Method == "GET" { com.PackagePath = r.FormValue("packagePath") com.Hash = r.FormValue("hash") - if err := datastore.Get(c, com.Key(c), com); err != nil { + err := datastore.Get(c, com.Key(c), com) + if com.Num == 0 && com.Desc == "" { + // Perf builder might have written an incomplete Commit. + // Pretend it doesn't exist, so that we can get complete details. + err = datastore.ErrNoSuchEntity + } + if err != nil { if err == datastore.ErrNoSuchEntity { // This error string is special. // The commit watcher expects it. diff --git a/dashboard/app/build/notify.go b/dashboard/app/build/notify.go index bb12b21b..bbd710e7 100644 --- a/dashboard/app/build/notify.go +++ b/dashboard/app/build/notify.go @@ -15,6 +15,7 @@ import ( "net/http" "net/url" "regexp" + "runtime" "sort" "text/template" @@ -216,6 +217,13 @@ func sendPerfFailMail(c appengine.Context, builder string, res *PerfResult) erro } func commonNotify(c appengine.Context, com *Commit, builder, logHash string) error { + if com.Num == 0 || com.Desc == "" { + stk := make([]byte, 10000) + n := runtime.Stack(stk, false) + stk = stk[:n] + c.Errorf("refusing to notify with com=%+v\n%s", *com, string(stk)) + return fmt.Errorf("misuse of commonNotify") + } if com.FailNotificationSent { return nil }