diff --git a/dashboard/app/access_test.go b/dashboard/app/access_test.go index 45b67375..c02ea1d6 100644 --- a/dashboard/app/access_test.go +++ b/dashboard/app/access_test.go @@ -80,7 +80,7 @@ func TestAccess(t *testing.T) { if err != nil { t.Fatal(err) } - bugID := bugKeyHash(bug.Namespace, bug.Title, bug.Seq) + bugID := bug.keyHash() entities = append(entities, []entity{ { level: level, diff --git a/dashboard/app/api.go b/dashboard/app/api.go index 80f1919e..5c84ad75 100644 --- a/dashboard/app/api.go +++ b/dashboard/app/api.go @@ -399,7 +399,7 @@ func addCommitsToBug(c context.Context, bug *Bug, manager string, managers []str return nil } now := timeNow(c) - bugKey := datastore.NewKey(c, "Bug", bugKeyHash(bug.Namespace, bug.Title, bug.Seq), 0, nil) + bugKey := bug.key(c) tx := func(c context.Context) error { bug := new(Bug) if err := datastore.Get(c, bugKey, bug); err != nil { @@ -508,7 +508,7 @@ func apiReportBuildError(c context.Context, ns string, r *http.Request, payload return nil, err } if err := updateManager(c, ns, req.Build.Manager, func(mgr *Manager, stats *ManagerStats) { - mgr.FailedBuildBug = bugKeyHash(bug.Namespace, bug.Title, bug.Seq) + mgr.FailedBuildBug = bug.keyHash() }); err != nil { return nil, err } @@ -659,23 +659,19 @@ func saveCrash(c context.Context, ns string, req *dashapi.Crash, bugKey *datasto } func purgeOldCrashes(c context.Context, bug *Bug, bugKey *datastore.Key) { - if bug.NumCrashes <= maxCrashes || (bug.NumCrashes-1)%10 != 0 { + const purgeEvery = 10 + if bug.NumCrashes <= 2*maxCrashes || (bug.NumCrashes-1)%purgeEvery != 0 { return } var crashes []*Crash keys, err := datastore.NewQuery("Crash"). Ancestor(bugKey). - Filter("ReproC=", 0). - Filter("ReproSyz=", 0). Filter("Reported=", time.Time{}). GetAll(c, &crashes) if err != nil { log.Errorf(c, "failed to fetch purge crashes: %v", err) return } - if len(keys) <= maxCrashes { - return - } keyMap := make(map[*Crash]*datastore.Key) for i, crash := range crashes { keyMap[crash] = keys[i] @@ -684,27 +680,26 @@ func purgeOldCrashes(c context.Context, bug *Bug, bugKey *datastore.Key) { sort.Slice(crashes, func(i, j int) bool { return crashes[i].Time.After(crashes[j].Time) }) - // Find latest crash on each manager. - latestOnManager := make(map[string]*Crash) - for _, crash := range crashes { - if latestOnManager[crash.Manager] == nil { - latestOnManager[crash.Manager] = crash - } - } - // Oldest first but move latest crash on each manager to the end (preserve them). - sort.Slice(crashes, func(i, j int) bool { - latesti := latestOnManager[crashes[i].Manager] == crashes[i] - latestj := latestOnManager[crashes[j].Manager] == crashes[j] - if latesti != latestj { - return latestj - } - return crashes[i].Time.Before(crashes[j].Time) - }) - crashes = crashes[:len(crashes)-maxCrashes] var toDelete []*datastore.Key + latestOnManager := make(map[string]bool) + deleted, reproCount, noreproCount := 0, 0, 0 for _, crash := range crashes { - if crash.ReproSyz != 0 || crash.ReproC != 0 || !crash.Reported.IsZero() { - log.Errorf(c, "purging reproducer?") + if !crash.Reported.IsZero() { + log.Errorf(c, "purging reported crash?") + continue + } + // Preserve latest crash on each manager. + if !latestOnManager[crash.Manager] { + latestOnManager[crash.Manager] = true + continue + } + // Preserve maxCrashes latest crashes with repro and without repro. + count := &noreproCount + if crash.ReproSyz != 0 || crash.ReproC != 0 { + count = &reproCount + } + if *count < maxCrashes { + *count++ continue } toDelete = append(toDelete, keyMap[crash]) @@ -714,12 +709,25 @@ func purgeOldCrashes(c context.Context, bug *Bug, bugKey *datastore.Key) { if crash.Report != 0 { toDelete = append(toDelete, datastore.NewKey(c, textCrashReport, "", crash.Report, nil)) } + if crash.ReproSyz != 0 { + toDelete = append(toDelete, datastore.NewKey(c, textReproSyz, "", crash.ReproSyz, nil)) + } + if crash.ReproC != 0 { + toDelete = append(toDelete, datastore.NewKey(c, textReproC, "", crash.ReproC, nil)) + } + deleted++ + if deleted == 2*purgeEvery { + break + } + } + if len(toDelete) == 0 { + return } if err := datastore.DeleteMulti(c, toDelete); err != nil { log.Errorf(c, "failed to delete old crashes: %v", err) return } - log.Infof(c, "deleted %v crashes for bug %q", len(crashes), bug.Title) + log.Infof(c, "deleted %v crashes for bug %q", deleted, bug.Title) } func apiReportFailedRepro(c context.Context, ns string, r *http.Request, payload []byte) (interface{}, error) { diff --git a/dashboard/app/app_test.go b/dashboard/app/app_test.go index af4bacf4..57202e7c 100644 --- a/dashboard/app/app_test.go +++ b/dashboard/app/app_test.go @@ -7,6 +7,7 @@ package dash import ( "fmt" + "strconv" "strings" "testing" "time" @@ -285,3 +286,87 @@ func TestApp(t *testing.T) { ReproLevel: dashapi.ReproLevelC, }) } + +// Test purging of old crashes for bugs with lots of crashes. +func TestPurgeOldCrashes(t *testing.T) { + if testing.Short() { + t.Skip() + } + c := NewCtx(t) + defer c.Close() + + build := testBuild(1) + c.client.UploadBuild(build) + + // First, send 3 crashes that are reported. These need to be preserved regardless. + crash := testCrash(build, 1) + crash.ReproOpts = []byte("no repro") + c.client.ReportCrash(crash) + rep := c.client.pollBug() + + crash.ReproSyz = []byte("getpid()") + crash.ReproOpts = []byte("syz repro") + c.client.ReportCrash(crash) + c.client.pollBug() + + crash.ReproC = []byte("int main() {}") + crash.ReproOpts = []byte("C repro") + c.client.ReportCrash(crash) + c.client.pollBug() + + // Now report lots of bugs with/without repros. Some of the older ones should be purged. + const totalReported = 3 * maxCrashes + for i := 0; i < totalReported; i++ { + c.advanceTime(2 * time.Hour) // This ensures that crashes are saved. + crash.ReproSyz = nil + crash.ReproC = nil + crash.ReproOpts = []byte(fmt.Sprintf("%v", i)) + c.client.ReportCrash(crash) + + crash.ReproSyz = []byte("syz repro") + crash.ReproC = []byte("C repro") + crash.ReproOpts = []byte(fmt.Sprintf("%v", i)) + c.client.ReportCrash(crash) + } + bug, _, _ := c.loadBug(rep.ID) + crashes, _, err := queryCrashesForBug(c.ctx, bug.key(c.ctx), 10*totalReported) + if err != nil { + c.t.Fatal(err) + } + // First, count how many crashes of different types we have. + // We should get all 3 reported crashes + some with repros and some without repros. + reported, norepro, repro := 0, 0, 0 + for _, crash := range crashes { + if !crash.Reported.IsZero() { + reported++ + } else if crash.ReproSyz == 0 { + norepro++ + } else { + repro++ + } + } + c.t.Logf("got reported=%v, norepro=%v, repro=%v, maxCrashes=%v", + reported, norepro, repro, maxCrashes) + if reported != 3 || + norepro < maxCrashes || norepro > maxCrashes+10 || + repro < maxCrashes || repro > maxCrashes+10 { + c.t.Fatalf("bad purged crashes") + } + // Then, check that latest crashes were preserved. + for _, crash := range crashes { + if !crash.Reported.IsZero() { + continue + } + idx, err := strconv.Atoi(string(crash.ReproOpts)) + if err != nil { + c.t.Fatal(err) + } + count := norepro + if crash.ReproSyz != 0 { + count = repro + } + if idx < totalReported-count { + c.t.Errorf("preserved bad crash repro=%v: %v", crash.ReproC != 0, idx) + } + } +} diff --git a/dashboard/app/entities.go b/dashboard/app/entities.go index 9154bc68..152a1250 100644 --- a/dashboard/app/entities.go +++ b/dashboard/app/entities.go @@ -336,12 +336,20 @@ func canonicalBug(c context.Context, bug *Bug) (*Bug, error) { bugKey := datastore.NewKey(c, "Bug", bug.DupOf, 0, nil) if err := datastore.Get(c, bugKey, canon); err != nil { return nil, fmt.Errorf("failed to get dup bug %q for %q: %v", - bug.DupOf, bugKeyHash(bug.Namespace, bug.Title, bug.Seq), err) + bug.DupOf, bug.keyHash(), err) } bug = canon } } +func (bug *Bug) key(c context.Context) *datastore.Key { + return datastore.NewKey(c, "Bug", bug.keyHash(), 0, nil) +} + +func (bug *Bug) keyHash() string { + return bugKeyHash(bug.Namespace, bug.Title, bug.Seq) +} + func bugKeyHash(ns, title string, seq int64) string { return hash.String([]byte(fmt.Sprintf("%v-%v-%v-%v", config.Namespaces[ns].Key, ns, title, seq))) } diff --git a/dashboard/app/main.go b/dashboard/app/main.go index aee82ed8..044b4f76 100644 --- a/dashboard/app/main.go +++ b/dashboard/app/main.go @@ -418,7 +418,7 @@ func fetchNamespaceBugs(c context.Context, accessLevel AccessLevel, ns string, continue } uiBug := createUIBug(c, bug, state, managers) - bugMap[bugKeyHash(bug.Namespace, bug.Title, bug.Seq)] = uiBug + bugMap[bug.keyHash()] = uiBug id := uiBug.ReportingIndex if bug.Status == BugStatusFixed { id = -1 @@ -491,7 +491,7 @@ func fetchNamespaceBugs(c context.Context, accessLevel AccessLevel, ns string, func loadDupsForBug(c context.Context, r *http.Request, bug *Bug, state *ReportingState, managers []string) ( *uiBugGroup, error) { - bugHash := bugKeyHash(bug.Namespace, bug.Title, bug.Seq) + bugHash := bug.keyHash() var dups []*Bug _, err := datastore.NewQuery("Bug"). Filter("Status=", BugStatusDup). @@ -603,7 +603,7 @@ func createUIBug(c context.Context, bug *Bug, state *ReportingState, managers [] if err != nil { log.Errorf(c, "failed to generate credit email: %v", err) } - id := bugKeyHash(bug.Namespace, bug.Title, bug.Seq) + id := bug.keyHash() uiBug := &uiBug{ Namespace: bug.Namespace, Title: bug.displayTitle(), @@ -662,10 +662,9 @@ func updateBugBadness(c context.Context, bug *uiBug) { } func loadCrashesForBug(c context.Context, bug *Bug) ([]*uiCrash, []byte, error) { - bugHash := bugKeyHash(bug.Namespace, bug.Title, bug.Seq) - bugKey := datastore.NewKey(c, "Bug", bugHash, 0, nil) + bugKey := bug.key(c) // We can have more than maxCrashes crashes, if we have lots of reproducers. - crashes, _, err := queryCrashesForBug(c, bugKey, maxCrashes+200) + crashes, _, err := queryCrashesForBug(c, bugKey, 2*maxCrashes+200) if err != nil || len(crashes) == 0 { return nil, nil, err } diff --git a/dashboard/app/reporting.go b/dashboard/app/reporting.go index 4982c8ad..4ae3ae29 100644 --- a/dashboard/app/reporting.go +++ b/dashboard/app/reporting.go @@ -447,7 +447,7 @@ func findDupBug(c context.Context, cmd *dashapi.BugUpdate, bug *Bug, bugKey *dat if !dupReporting.Closed.IsZero() && dupCanon.Status == BugStatusOpen { return "", false, "Dup bug is already upstreamed.", nil } - dupHash := bugKeyHash(dup.Namespace, dup.Title, dup.Seq) + dupHash := dup.keyHash() return dupHash, true, "", nil } @@ -723,7 +723,7 @@ func queryCrashesForBug(c context.Context, bugKey *datastore.Key, limit int) ( } func findCrashForBug(c context.Context, bug *Bug) (*Crash, *datastore.Key, error) { - bugKey := datastore.NewKey(c, "Bug", bugKeyHash(bug.Namespace, bug.Title, bug.Seq), 0, nil) + bugKey := bug.key(c) crashes, keys, err := queryCrashesForBug(c, bugKey, 1) if err != nil { return nil, nil, err