From 632b86c938a94237bde5ec0b45df7258bf95ba8f Mon Sep 17 00:00:00 2001 From: Dmitry Vyukov Date: Mon, 23 Oct 2017 09:58:09 +0200 Subject: [PATCH] dashboard/app: better errors from bug update command Provide better errors from bug update command. In particular, distinguish between bad updates and internal errors. Also better messages. Allow duping onto closed bugs. Don't allow unduping from closed bugs. --- dashboard/app/fix_test.go | 1 + dashboard/app/reporting.go | 120 ++++++++++++++++------------ dashboard/app/reporting_email.go | 6 +- dashboard/app/reporting_external.go | 7 +- dashboard/app/reporting_test.go | 82 ++++++++++++------- dashboard/dashapi/dashapi.go | 8 +- 6 files changed, 139 insertions(+), 85 deletions(-) diff --git a/dashboard/app/fix_test.go b/dashboard/app/fix_test.go index 99d41a88..a1267476 100644 --- a/dashboard/app/fix_test.go +++ b/dashboard/app/fix_test.go @@ -34,6 +34,7 @@ func reportAllBugs(c *Ctx, expect int) []*dashapi.BugReport { } reply := new(dashapi.BugUpdateReply) c.expectOK(c.API(client1, key1, "reporting_update", cmd, reply)) + c.expectEQ(reply.Error, false) c.expectEQ(reply.OK, true) } return resp.Reports diff --git a/dashboard/app/reporting.go b/dashboard/app/reporting.go index 407478b2..d4e4d67c 100644 --- a/dashboard/app/reporting.go +++ b/dashboard/app/reporting.go @@ -5,7 +5,6 @@ package dash import ( "encoding/json" - "errors" "fmt" "sort" "strings" @@ -241,20 +240,25 @@ func createBugReport(c context.Context, bug *Bug, crash *Crash, bugReporting *Bu } // incomingCommand is entry point to bug status updates. -func incomingCommand(c context.Context, cmd *dashapi.BugUpdate) (string, bool) { +func incomingCommand(c context.Context, cmd *dashapi.BugUpdate) (bool, string, error) { log.Infof(c, "got command: %+q", cmd) - reply, err := incomingCommandImpl(c, cmd) + ok, reason, err := incomingCommandImpl(c, cmd) + if !ok { + log.Errorf(c, "%v", reason) + } if err != nil { log.Errorf(c, "%v", err) - return reply, false } - return reply, true + if !ok { + log.Warningf(c, "invalid update: %v", reason) + } + return ok, reason, err } -func incomingCommandImpl(c context.Context, cmd *dashapi.BugUpdate) (string, error) { +func incomingCommandImpl(c context.Context, cmd *dashapi.BugUpdate) (bool, string, error) { bug, bugKey, err := findBugByReportingID(c, cmd.ID) if err != nil { - return "can't find the corresponding bug", err + return false, internalError, err } now := timeNow(c) dupHash := "" @@ -265,7 +269,7 @@ func incomingCommandImpl(c context.Context, cmd *dashapi.BugUpdate) (string, err // Email reporting passes bug title in cmd.DupOf, try to find bug by title. dup, dupKey, err = findDupByTitle(c, bug.Namespace, cmd.DupOf) if err != nil { - return "can't find the dup bug", err + return false, "can't find the dup bug", err } cmd.DupOf = "" for i := range dup.Reporting { @@ -275,70 +279,86 @@ func incomingCommandImpl(c context.Context, cmd *dashapi.BugUpdate) (string, err } } if cmd.DupOf == "" { - return "can't find the dup bug", + return false, "can't find the dup bug", fmt.Errorf("dup does not have reporting %q", bugReporting.Name) } } - if bugKey.StringID() == dupKey.StringID() { - return "can't dup bug to itself", fmt.Errorf("can't dup bug to itself") - } - if bug.Namespace != dup.Namespace { - return "can't find the dup bug", - fmt.Errorf("inter-namespace dup: %v->%v", bug.Namespace, dup.Namespace) - } dupReporting, _ := bugReportingByID(dup, cmd.DupOf, now) if bugReporting == nil || dupReporting == nil { - return internalError, fmt.Errorf("can't find bug reporting") + return false, internalError, fmt.Errorf("can't find bug reporting") } - if !dupReporting.Closed.IsZero() { - return "dup bug is already closed", fmt.Errorf("dup bug is already closed") + if bugKey.StringID() == dupKey.StringID() { + if bugReporting.Name == dupReporting.Name { + return false, "Can't dup bug to itself.", nil + } + return false, fmt.Sprintf("Can't dup bug to itself in different reporting (%v->%v).\n"+ + "Please dup syzbot bugs only onto syzbot bugs for the same kernel/reporting.", + bugReporting.Name, dupReporting.Name), nil + } + if bug.Namespace != dup.Namespace { + return false, fmt.Sprintf("Duplicate bug corresponds to a different kernel (%v->%v).\n"+ + "Please dup syzbot bugs only onto syzbot bugs for the same kernel.", + bug.Namespace, dup.Namespace), nil } if bugReporting.Name != dupReporting.Name { - return "can't find the dup bug", - fmt.Errorf("inter-reporting dup: %v -> %v", - bugReporting.Name, dupReporting.Name) + return false, fmt.Sprintf("Can't dup bug to a bug in different reporting (%v->%v)."+ + "Please dup syzbot bugs only onto syzbot bugs for the same kernel/reporting.", + bugReporting.Name, dupReporting.Name), nil + } + dupCanon, err := canonicalBug(c, dup) + if err != nil { + return false, internalError, fmt.Errorf("failed to get canonical bug for dup: %v", err) + } + if !dupReporting.Closed.IsZero() && dupCanon.Status == BugStatusOpen { + return false, "Dup bug is already upstreamed.", nil } dupHash = bugKeyHash(dup.Namespace, dup.Title, dup.Seq) } - reply := "" + ok, reply := false, "" tx := func(c context.Context) error { var err error - reply, err = incomingCommandTx(c, now, cmd, bugKey, dupHash) + ok, reply, err = incomingCommandTx(c, now, cmd, bugKey, dupHash) return err } err = datastore.RunInTransaction(c, tx, &datastore.TransactionOptions{XG: true}) - if err != nil && reply == "" { - reply = internalError + if err != nil { + return false, internalError, err } - return reply, err + return ok, reply, nil } -func incomingCommandTx(c context.Context, now time.Time, cmd *dashapi.BugUpdate, bugKey *datastore.Key, dupHash string) (string, error) { +func incomingCommandTx(c context.Context, now time.Time, cmd *dashapi.BugUpdate, bugKey *datastore.Key, dupHash string) (bool, string, error) { bug := new(Bug) if err := datastore.Get(c, bugKey, bug); err != nil { - return "can't find the corresponding bug", err + return false, internalError, fmt.Errorf("can't find the corresponding bug: %v", err) } switch bug.Status { - case BugStatusOpen, BugStatusDup: + case BugStatusOpen: + case BugStatusDup: + canon, err := canonicalBug(c, bug) + if err != nil { + return false, internalError, err + } + if canon.Status != BugStatusOpen { + return false, "This bug is already closed (dup was closed).\n" + + "Don't change closed syzbot bugs.", nil + } case BugStatusFixed, BugStatusInvalid: - return "this bug is already closed", - fmt.Errorf("got a command for a closed bug") + return false, "This bug is already closed.", nil default: - return internalError, - fmt.Errorf("unknown bug status %v", bug.Status) + return false, internalError, fmt.Errorf("unknown bug status %v", bug.Status) } bugReporting, final := bugReportingByID(bug, cmd.ID, now) if bugReporting == nil { - return internalError, fmt.Errorf("can't find bug reporting") + return false, internalError, fmt.Errorf("can't find bug reporting") } if !bugReporting.Closed.IsZero() { - return "this bug is already closed", fmt.Errorf("got a command for a closed reporting") + return false, "This bug is already closed.", nil } - state, err := loadReportingState(c) if err != nil { - return internalError, err + return false, internalError, err } stateEnt := state.getEntry(now, bug.Namespace, bugReporting.Name) switch cmd.Status { @@ -350,22 +370,21 @@ func incomingCommandTx(c context.Context, now time.Time, cmd *dashapi.BugUpdate, stateEnt.Sent++ // sending repro does not count against the quota } if bug.ReproLevel < cmd.ReproLevel { - return internalError, fmt.Errorf("bug update with invalid repro level: %v/%v", - bug.ReproLevel, cmd.ReproLevel) + return false, internalError, + fmt.Errorf("bug update with invalid repro level: %v/%v", + bug.ReproLevel, cmd.ReproLevel) } case dashapi.BugStatusUpstream: if final { - reply := "can't close, this is final destination" - return reply, errors.New(reply) + return false, "Can't upstream, this is final destination.", nil } if len(bug.Commits) != 0 { // We could handle this case, but how/when it will occur // in real life is unclear now. - reply := "can't upstream, the bug has fixing commits" - return reply, errors.New(reply) + return false, "Can't upstream this bug, the bug has fixing commits.", nil } bug.Status = BugStatusOpen - bug.Closed = now + bug.Closed = time.Time{} bugReporting.Closed = now case dashapi.BugStatusInvalid: bugReporting.Closed = now @@ -378,7 +397,7 @@ func incomingCommandTx(c context.Context, now time.Time, cmd *dashapi.BugUpdate, case dashapi.BugStatusUpdate: // Just update Link, Commits, etc below. default: - return "unknown bug status", fmt.Errorf("unknown bug status %v", cmd.Status) + return false, internalError, fmt.Errorf("unknown bug status %v", cmd.Status) } if len(cmd.FixCommits) != 0 && (bug.Status == BugStatusOpen || bug.Status == BugStatusDup) { m := make(map[string]bool) @@ -399,8 +418,7 @@ func incomingCommandTx(c context.Context, now time.Time, cmd *dashapi.BugUpdate, commits := make([]string, 0, len(m)) for com := range m { if len(com) < 3 { - err := fmt.Errorf("bad commit title: %q", com) - return err.Error(), err + return false, fmt.Sprintf("bad commit title: %q", com), nil } commits = append(commits, com) } @@ -426,12 +444,12 @@ func incomingCommandTx(c context.Context, now time.Time, cmd *dashapi.BugUpdate, bug.DupOf = "" } if _, err := datastore.Put(c, bugKey, bug); err != nil { - return internalError, fmt.Errorf("failed to put bug: %v", err) + return false, internalError, fmt.Errorf("failed to put bug: %v", err) } if err := saveReportingState(c, state); err != nil { - return internalError, err + return false, internalError, err } - return "", nil + return true, "", nil } func findBugByReportingID(c context.Context, id string) (*Bug, *datastore.Key, error) { diff --git a/dashboard/app/reporting_email.go b/dashboard/app/reporting_email.go index c9caa712..0c6cb9f5 100644 --- a/dashboard/app/reporting_email.go +++ b/dashboard/app/reporting_email.go @@ -153,7 +153,8 @@ func emailReport(c context.Context, rep *dashapi.BugReport) error { Status: dashapi.BugStatusOpen, ReproLevel: repro, } - incomingCommand(c, cmd) + ok, reason, err := incomingCommand(c, cmd) + _, _, _ = ok, reason, err return nil } @@ -204,7 +205,8 @@ func incomingMail(c context.Context, r *http.Request) error { default: return replyTo(c, msg, fmt.Sprintf("unknown command %q", msg.Command), nil) } - reply, _ := incomingCommand(c, cmd) + ok, reply, err := incomingCommand(c, cmd) + _, _ = ok, err if !sendReply { return nil } diff --git a/dashboard/app/reporting_external.go b/dashboard/app/reporting_external.go index e8f9ea62..56f8c3b4 100644 --- a/dashboard/app/reporting_external.go +++ b/dashboard/app/reporting_external.go @@ -41,9 +41,10 @@ func apiReportingUpdate(c context.Context, ns string, r *http.Request) (interfac if err := json.NewDecoder(r.Body).Decode(req); err != nil { return nil, fmt.Errorf("failed to unmarshal request: %v", err) } - reply, ok := incomingCommand(c, req) + ok, reason, err := incomingCommand(c, req) return &dashapi.BugUpdateReply{ - OK: ok, - Text: reply, + OK: ok, + Error: err != nil, + Text: reason, }, nil } diff --git a/dashboard/app/reporting_test.go b/dashboard/app/reporting_test.go index 9d6368b9..d09f874a 100644 --- a/dashboard/app/reporting_test.go +++ b/dashboard/app/reporting_test.go @@ -326,6 +326,53 @@ func TestReportingDup(t *testing.T) { c.expectOK(c.API(client1, key1, "reporting_poll", pr, resp)) c.expectEQ(len(resp.Reports), 1) c.expectEQ(resp.Reports[0].Title, crash2.Title+" (2)") + + // Unduping after the canonical bugs was closed must not work + // (we already created new bug for this report). + cmd = &dashapi.BugUpdate{ + ID: rep2.ID, + Status: dashapi.BugStatusOpen, + } + c.expectOK(c.API(client1, key1, "reporting_update", cmd, reply)) + c.expectEQ(reply.OK, false) +} + +// Dup bug onto a closed bug. +// A new crash report must create a new bug. +func TestReportingDupToClosed(t *testing.T) { + c := NewCtx(t) + defer c.Close() + + 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)) + + reports := reportAllBugs(c, 2) + + cmd := &dashapi.BugUpdate{ + ID: reports[0].ID, + Status: dashapi.BugStatusInvalid, + } + reply := new(dashapi.BugUpdateReply) + c.expectOK(c.API(client1, key1, "reporting_update", cmd, reply)) + c.expectEQ(reply.OK, true) + + cmd = &dashapi.BugUpdate{ + ID: reports[1].ID, + Status: dashapi.BugStatusDup, + DupOf: reports[0].ID, + } + c.expectOK(c.API(client1, key1, "reporting_update", cmd, reply)) + c.expectEQ(reply.OK, true) + + c.expectOK(c.API(client1, key1, "report_crash", crash2, nil)) + reports2 := reportAllBugs(c, 1) + c.expectEQ(reports2[0].Title, crash2.Title+" (2)") } // Test that marking dups across reporting levels is not permitted. @@ -342,40 +389,21 @@ func TestReportingDupCrossReporting(t *testing.T) { 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) + reports := reportAllBugs(c, 2) + rep1 := reports[0] + rep2 := reports[1] - rep1 := resp.Reports[0] + // Upstream second bug. cmd := &dashapi.BugUpdate{ - ID: rep1.ID, - Status: dashapi.BugStatusOpen, + ID: rep2.ID, + Status: dashapi.BugStatusUpstream, } reply := new(dashapi.BugUpdateReply) c.expectOK(c.API(client1, key1, "reporting_update", cmd, reply)) c.expectEQ(reply.OK, true) - rep2 := resp.Reports[1] - cmd = &dashapi.BugUpdate{ - ID: rep2.ID, - Status: dashapi.BugStatusOpen, - } - c.expectOK(c.API(client1, key1, "reporting_update", cmd, reply)) - c.expectEQ(reply.OK, true) - - // Upstream second bug. - cmd = &dashapi.BugUpdate{ - ID: rep2.ID, - Status: dashapi.BugStatusUpstream, - } - c.expectOK(c.API(client1, key1, "reporting_update", cmd, reply)) - c.expectEQ(reply.OK, true) - c.expectOK(c.API(client1, key1, "reporting_poll", pr, resp)) - c.expectEQ(len(resp.Reports), 1) - rep3 := resp.Reports[0] + reports = reportAllBugs(c, 1) + rep3 := reports[0] // Duping must fail all ways. cmds := []*dashapi.BugUpdate{ diff --git a/dashboard/dashapi/dashapi.go b/dashboard/dashapi/dashapi.go index d59b2a83..ef51f4f5 100644 --- a/dashboard/dashapi/dashapi.go +++ b/dashboard/dashapi/dashapi.go @@ -170,8 +170,12 @@ type BugUpdate struct { } type BugUpdateReply struct { - OK bool - Text string + // Bug update can fail for 2 reason: + // - update does not pass logical validataion, in this case OK=false + // - internal/datastore error, in this case Error=true + OK bool + Error bool + Text string } type PollRequest struct {