Commit 970614872d
Verified · cmc
Layout: unified · split
internal/control/runnernext_test.go +48
| @@ -336,3 +336,51 @@ func TestRunnerDoneToleratesMissingOrBadStep(t *testing.T) { | |||
| 336 | } | 336 | } |
| 337 | } | 337 | } |
| 338 | } | 338 | } |
| 339 | |||
| 340 | // A runner still holding a build whose repository was deleted reports | ||
| 341 | // on an id no later build takes: its runner done finds nothing rather | ||
| 342 | // than finishing another repository's running build (#306). | ||
| 343 | func TestRunnerDoneAfterRepositoryDeleted(t *testing.T) { | ||
| 344 | st, keep, uid := newQueueTestRepo(t) | ||
| 345 | goneID, err := st.CreateRepo("user", uid, "gone", "public") | ||
| 346 | if err != nil { | ||
| 347 | t.Fatal(err) | ||
| 348 | } | ||
| 349 | if _, err := st.CreateBuild(keep.ID, "unit", "aaa", "main", "[]", "", "", true); err != nil { | ||
| 350 | t.Fatal(err) | ||
| 351 | } | ||
| 352 | if _, err := st.CreateBuild(goneID, "unit", "bbb", "main", "[]", "", "", true); err != nil { | ||
| 353 | t.Fatal(err) | ||
| 354 | } | ||
| 355 | var stale store.Build | ||
| 356 | for range 2 { | ||
| 357 | b, ok, err := st.ClaimBuild(nil, false) | ||
| 358 | if err != nil || !ok { | ||
| 359 | t.Fatalf("claim: %v ok=%v", err, ok) | ||
| 360 | } | ||
| 361 | if b.RepoID == goneID { | ||
| 362 | stale = b | ||
| 363 | } | ||
| 364 | } | ||
| 365 | if err := st.DeleteRepo(goneID); err != nil { | ||
| 366 | t.Fatal(err) | ||
| 367 | } | ||
| 368 | if _, err := st.CreateBuild(keep.ID, "unit", "ccc", "main", "[]", "", "", true); err != nil { | ||
| 369 | t.Fatal(err) | ||
| 370 | } | ||
| 371 | next, ok, err := st.ClaimBuild(nil, false) | ||
| 372 | if err != nil || !ok { | ||
| 373 | t.Fatalf("claim: %v ok=%v", err, ok) | ||
| 374 | } | ||
| 375 | if next.ID == stale.ID { | ||
| 376 | t.Fatalf("the next build took the deleted repository's build id %d", stale.ID) | ||
| 377 | } | ||
| 378 | |||
| 379 | c, out := runnerCtx(st, uid, t.TempDir()) | ||
| 380 | if code := runRunnerDone(c, []string{fmt.Sprint(stale.ID), "success"}); code != protocol.ExitNotFound { | ||
| 381 | t.Fatalf("runner done on the deleted build: exit %d, want %d: %s", code, protocol.ExitNotFound, out) | ||
| 382 | } | ||
| 383 | if b, err := st.BuildByID(next.ID); err != nil || b.Status != "running" { | ||
| 384 | t.Fatalf("the other build after the stale report: %+v, %v", b, err) | ||
| 385 | } | ||
| 386 | } | ||
internal/store/builds.go +18 −2
| @@ -61,9 +61,25 @@ func (s *Store) CreateBuild(repoID int64, job, sha, ref, stepsJSON, image, tree | |||
| 61 | if err := tx.QueryRow("SELECT build_counter FROM repos WHERE id = ?", repoID).Scan(&n); err != nil { | 61 | if err := tx.QueryRow("SELECT build_counter FROM repos WHERE id = ?", repoID).Scan(&n); err != nil { |
| 62 | return 0, err | 62 | return 0, err |
| 63 | } | 63 | } |
| 64 | // A runner names a build by id in runner log and runner done, so an id | ||
| 65 | // is never handed out twice, including after a repository's deletion | ||
| 66 | // takes the newest builds with it (#306). builds is not AUTOINCREMENT | ||
| 67 | // because rebuilding it would copy every stored log; the high-water | ||
| 68 | // mark lives in settings instead. | ||
| 69 | var id int64 | ||
| 70 | if err := tx.QueryRow(`SELECT MAX( | ||
| 71 | COALESCE((SELECT MAX(id) FROM builds), 0), | ||
| 72 | COALESCE((SELECT CAST(value AS INTEGER) FROM settings WHERE key = 'build_id_seq'), 0)) + 1`). | ||
| 73 | Scan(&id); err != nil { | ||
| 74 | return 0, err | ||
| 75 | } | ||
| 64 | if _, err := tx.Exec( | 76 | if _, err := tx.Exec( |
| 65 | "INSERT INTO builds (repo_id, number, job, sha, ref, steps, image, tree, trusted) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)", | 77 | "INSERT INTO builds (id, repo_id, number, job, sha, ref, steps, image, tree, trusted) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", |
| 66 | repoID, n, job, sha, ref, stepsJSON, image, tree, trusted); err != nil { | 78 | id, repoID, n, job, sha, ref, stepsJSON, image, tree, trusted); err != nil { |
| 79 | return 0, err | ||
| 80 | } | ||
| 81 | if _, err := tx.Exec(`INSERT INTO settings (key, value) VALUES ('build_id_seq', ?) | ||
| 82 | ON CONFLICT (key) DO UPDATE SET value = excluded.value`, strconv.FormatInt(id, 10)); err != nil { | ||
| 67 | return 0, err | 83 | return 0, err |
| 68 | } | 84 | } |
| 69 | return n, tx.Commit() | 85 | return n, tx.Commit() |
internal/store/builds_test.go +48
| @@ -601,3 +601,51 @@ func TestSetBuildFailure(t *testing.T) { | |||
| 601 | t.Fatalf("rewrote a finished build: %v", err) | 601 | t.Fatalf("rewrote a finished build: %v", err) |
| 602 | } | 602 | } |
| 603 | } | 603 | } |
| 604 | |||
| 605 | // A runner names its build by id in runner log and runner done. Deleting | ||
| 606 | // the repository that holds the newest build must not hand that id to | ||
| 607 | // the next build in another repository (#306). | ||
| 608 | func TestBuildIDsNotReusedAfterRepoDelete(t *testing.T) { | ||
| 609 | s := open(t) | ||
| 610 | if err := s.MigrateUp(); err != nil { | ||
| 611 | t.Fatal(err) | ||
| 612 | } | ||
| 613 | uid, err := s.CreateUser("cmc", true) | ||
| 614 | if err != nil { | ||
| 615 | t.Fatal(err) | ||
| 616 | } | ||
| 617 | keep, err := s.CreateRepo("user", uid, "keep", "public") | ||
| 618 | if err != nil { | ||
| 619 | t.Fatal(err) | ||
| 620 | } | ||
| 621 | gone, err := s.CreateRepo("user", uid, "gone", "public") | ||
| 622 | if err != nil { | ||
| 623 | t.Fatal(err) | ||
| 624 | } | ||
| 625 | newest := func() int64 { | ||
| 626 | var id int64 | ||
| 627 | if err := s.DB.QueryRow("SELECT COALESCE(MAX(id), 0) FROM builds").Scan(&id); err != nil { | ||
| 628 | t.Fatal(err) | ||
| 629 | } | ||
| 630 | return id | ||
| 631 | } | ||
| 632 | if _, err := s.CreateBuild(keep, "test", "abc", "main", `["true"]`, "", "", true); err != nil { | ||
| 633 | t.Fatal(err) | ||
| 634 | } | ||
| 635 | if _, err := s.CreateBuild(gone, "test", "abc", "main", `["true"]`, "", "", true); err != nil { | ||
| 636 | t.Fatal(err) | ||
| 637 | } | ||
| 638 | claimed := newest() | ||
| 639 | if err := s.DeleteRepo(gone); err != nil { | ||
| 640 | t.Fatal(err) | ||
| 641 | } | ||
| 642 | if newest() >= claimed { | ||
| 643 | t.Fatal("the deleted repository's build survived") | ||
| 644 | } | ||
| 645 | if _, err := s.CreateBuild(keep, "test", "def", "main", `["true"]`, "", "", true); err != nil { | ||
| 646 | t.Fatal(err) | ||
| 647 | } | ||
| 648 | if got := newest(); got <= claimed { | ||
| 649 | t.Fatalf("next build took id %d; a runner may still hold %d", got, claimed) | ||
| 650 | } | ||
| 651 | } | ||