issue 974: the builders say the checks they run in a bucket that ages, and a builder drops a waiting request for a head and trunk commit another is running

2026-10-17 14:10:26 +02:00
parent 357e08a4f6
commit bd257afbd5
10 changed files with 380 additions and 9 deletions
@@ -367,7 +367,11 @@ func (m *natsMachine) Take(ctx context.Context, do func(context.Context, Build))
 		// the ask to a second machine nor counts the wait against its deliveries.
 		working := make(chan struct{})
 		go stillWorking(msg, working, heartbeatWhileWorking)
+		// And a check says it is running, to the other builders (ADR 0406 §3): its key put now and at each heartbeat,
+		// deleted when it ends, whatever the outcome.
+		stopRunning := keepRunning(ctx, m.js.Conn(), request, m.on, heartbeatWhileWorking)
 		do(ctx, build)
+		stopRunning()
 		close(working)
 	}
 }
@@ -431,17 +435,34 @@ func (m *natsMachine) next(ctx context.Context) (*nats.Msg, *takeQueue, error) {
 // mesh-delivery's, and a check wrongly dropped is worse than one run twice).
 func (m *natsMachine) judgedAlready(ctx context.Context, r BuildRequest) string {
 	c := r.Check
-	if c == nil || c.Group != "" || c.Try || c.Number <= 0 {
+	if c == nil || c.Try || (c.Group == "" && c.Number <= 0) {
 		return ""
 	}
-	key, found, err := ReadPullKey(ctx, m.js.Conn(), c.Owner, c.Repo, c.Number)
+	if c.Group == "" {
+		key, found, err := ReadPullKey(ctx, m.js.Conn(), c.Owner, c.Repo, c.Number)
+		if err != nil {
+			fmt.Fprintf(os.Stderr, "could not read the key of %s/%s#%d, so its check runs: %v\n", c.Owner, c.Repo, c.Number, err)
+		} else if found && key.Judged(r.Ref, c.Trunk) {
+			return fmt.Sprintf("%s/%s#%d is judged already at %.12s on %.12s (%s)", c.Owner, c.Repo, c.Number, r.Ref, c.Trunk,
+				key.Verdict)
+		}
+	}
+	return m.beingChecked(ctx, r)
+}
+
+// beingChecked says why a request need not run, or nothing: another builder's check of the same head and trunk commit is
+// running, and says so under its pull request's key in the bucket of the checks running (ADR 0406 §3). A key a builder
+// stopped renewing is gone within the bucket's age, and the request is taken then. Never the ask's own id: a redelivered ask
+// is its own, and a key an older builder wrote that names no head or trunk is no match.
+func (m *natsMachine) beingChecked(ctx context.Context, r BuildRequest) string {
+	running, ok, err := ReadRunning(ctx, m.js.Conn(), r)
 	if err != nil {
-		fmt.Fprintf(os.Stderr, "could not read the key of %s/%s#%d, so its check runs: %v\n", c.Owner, c.Repo, c.Number, err)
+		fmt.Fprintf(os.Stderr, "could not read whether %s is being checked, so its check runs: %v\n", r.ID, err)
 		return ""
 	}
-	if found && key.Judged(r.Ref, c.Trunk) {
-		return fmt.Sprintf("%s/%s#%d is judged already at %.12s on %.12s (%s)", c.Owner, c.Repo, c.Number, r.Ref, c.Trunk,
-			key.Verdict)
+	if ok && running.Build != r.ID && running.Same(r) {
+		return fmt.Sprintf("%s is being checked at %.12s on %.12s by %s as %s", r.ID, r.Ref, r.Check.Trunk, running.Builder,
+			running.Build)
 	}
 	return ""
 }
@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"os"
 	"strconv"
 	"strings"
 	"time"
@@ -168,3 +169,122 @@ const (
 	checkTakeWait = 30 * time.Millisecond
 	takeIdleWait  = time.Second
 )
+
+// CheckRunning is what a builder says of the check it is running, in the bucket of the checks running (novox/hq ADR 0406
+// §3): the head and trunk commit it checks, the builder and its build. The key is the pull request's (RunningKeyOf).
+type CheckRunning struct {
+	Head    string `json:"head"`
+	Trunk   string `json:"trunk"`
+	Builder string `json:"builder"`
+	Build   string `json:"build"`
+}
+
+// RunningKeyOf is the key a check is said running under: its pull request's tokens, or `group.<name>` for a group's
+// composed check; none for a try, which no other request replaces.
+func RunningKeyOf(r BuildRequest) (string, bool) {
+	c := r.Check
+	switch {
+	case c == nil, c.Try:
+		return "", false
+	case c.Group != "":
+		return "group." + tokenOf(c.Group), true
+	case c.Number <= 0:
+		return "", false
+	}
+	return PullKeyOf(c.Owner, c.Repo, c.Number), true
+}
+
+// runningBucket opens the bucket of the checks running.
+func runningBucket(ctx context.Context, conn *nats.Conn) (jetstream.KeyValue, error) {
+	api, err := jetstream.New(conn)
+	if err != nil {
+		return nil, err
+	}
+	kv, err := api.KeyValue(ctx, broker.CheckRunningBucket)
+	if err != nil {
+		return nil, fmt.Errorf("the checks running: %w", err)
+	}
+	return kv, nil
+}
+
+// ReadRunning reads the key of the check running for a request's pull request: false when there is none.
+func ReadRunning(ctx context.Context, conn *nats.Conn, r BuildRequest) (CheckRunning, bool, error) {
+	key, ok := RunningKeyOf(r)
+	if !ok {
+		return CheckRunning{}, false, nil
+	}
+	kv, err := runningBucket(ctx, conn)
+	if err != nil {
+		return CheckRunning{}, false, err
+	}
+	e, err := kv.Get(ctx, key)
+	if errors.Is(err, jetstream.ErrKeyNotFound) {
+		return CheckRunning{}, false, nil
+	}
+	if err != nil {
+		return CheckRunning{}, false, err
+	}
+	var running CheckRunning
+	if err := json.Unmarshal(e.Value(), &running); err != nil {
+		return CheckRunning{}, false, nil // a key of a builder from before this means no match
+	}
+	return running, true, nil
+}
+
+// Same says a running check is for this request's head and trunk commit: both said, and the same. A key that names neither
+// (an older builder's) is no match.
+func (c CheckRunning) Same(r BuildRequest) bool {
+	return r.Check != nil && c.Head != "" && c.Trunk != "" && r.Check.Trunk != "" && sameCommit(c.Head, r.Ref) &&
+		sameCommit(c.Trunk, r.Check.Trunk)
+}
+
+// keepRunning says a check is running: its key put now, put again at each heartbeat, and deleted by the stop it returns,
+// whatever the outcome. Said to nobody that cannot be told: a bucket that is not there is no reason not to run the check.
+func keepRunning(ctx context.Context, conn *nats.Conn, r BuildRequest, builder string, every time.Duration) (stop func()) {
+	key, ok := RunningKeyOf(r)
+	if !ok {
+		return func() {}
+	}
+	kv, err := runningBucket(ctx, conn)
+	if err != nil {
+		fmt.Fprintf(os.Stderr, "%s runs without saying so: %v\n", r.ID, err)
+		return func() {}
+	}
+	trunk := ""
+	if r.Check != nil {
+		trunk = r.Check.Trunk
+	}
+	body, _ := json.Marshal(CheckRunning{Head: r.Ref, Trunk: trunk, Builder: builder, Build: r.ID})
+	put := func() {
+		putting, end := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second)
+		defer end()
+		if _, err := kv.Put(putting, key, body); err != nil {
+			fmt.Fprintf(os.Stderr, "%s could not say it is running: %v\n", r.ID, err)
+		}
+	}
+	put()
+	done := make(chan struct{})
+	finished := make(chan struct{})
+	go func() {
+		defer close(finished)
+		tick := time.NewTicker(every)
+		defer tick.Stop()
+		for {
+			select {
+			case <-done:
+				return
+			case <-tick.C:
+				put()
+			}
+		}
+	}()
+	return func() {
+		close(done)
+		<-finished
+		deleting, end := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second)
+		defer end()
+		if err := kv.Delete(deleting, key); err != nil {
+			fmt.Fprintf(os.Stderr, "%s could not take its running key away; it ages out: %v\n", r.ID, err)
+		}
+	}
+}
@@ -307,3 +307,180 @@ func TestTheQueueIsReadAcrossTheThreeStreamsAndAnAskIsDeletedFromItsOwn(t *testi
 		t.Errorf("%d asks left; want 2", n)
 	}
 }
+
+// aRunningBucket makes the bucket the builders say the checks they run in, with an age of its own short enough for a test
+// (the mesh's is two minutes): a key not renewed within it is gone.
+func aRunningBucket(t *testing.T, js *broker.JetStream, age time.Duration) jetstream.KeyValue {
+	t.Helper()
+	api, err := jetstream.New(js.Conn())
+	if err != nil {
+		t.Fatal(err)
+	}
+	kv, err := api.CreateKeyValue(t.Context(), jetstream.KeyValueConfig{Bucket: broker.CheckRunningBucket, History: 1, TTL: age})
+	if err != nil {
+		t.Fatal(err)
+	}
+	return kv
+}
+
+// A builder taking a request for a pull request's head and trunk commit that another builder is running a check of drops it:
+// the bucket of running checks names the head and the trunk, the builder and the build (issue 974).
+func TestARunningCheckOnOneBuilderMakesAnotherDropTheSameRequestWaiting(t *testing.T) {
+	js := aBusWithTheBuildRole(t)
+	aKeyBucket(t, js)
+	aRunningBucket(t, js, 2*time.Minute)
+	ctx, stop := context.WithCancel(t.Context())
+	defer stop()
+	asks := BuildsOn(js, TheBuildMachine)
+	running := make(chan string, 4)
+	release := make(chan struct{})
+	a := MachineOverNATS(js, "builder-a")
+	defer a.Close()
+	go func() {
+		_ = a.Take(ctx, func(ctx context.Context, work Build) {
+			running <- work.Request().ID
+			<-release
+			_ = work.Announce(ctx, BuildResult{ID: work.Request().ID, On: "builder-a", Failed: "no"})
+			_ = work.Done()
+		})
+	}()
+	if err := asks.Ask(ctx, aPullCheck("first", 7, "head-7-000000000", "trunk-1-000000", false)); err != nil {
+		t.Fatal(err)
+	}
+	select {
+	case <-running:
+	case <-time.After(10 * time.Second):
+		t.Fatal("builder a took nothing")
+	}
+	// The same head and trunk commit asked again while it runs, and a pull request's other head asked beside it.
+	if err := asks.Ask(ctx, aPullCheck("again", 7, "head-7-000000000", "trunk-1-000000", false)); err != nil {
+		t.Fatal(err)
+	}
+	b := MachineOverNATS(js, "builder-b")
+	defer b.Close()
+	took := make(chan string, 4)
+	go func() {
+		_ = b.Take(ctx, func(ctx context.Context, work Build) {
+			took <- work.Request().ID
+			_ = work.Announce(ctx, BuildResult{ID: work.Request().ID, On: "builder-b", Failed: "no"})
+			_ = work.Done()
+		})
+	}()
+	select {
+	case id := <-took:
+		t.Fatalf("builder b took %s, which builder a is running a check of", id)
+	case <-time.After(3 * time.Second):
+	}
+	if n := len(queueOf(t, js).Of(AskWaiting)); n != 0 {
+		t.Errorf("%d request(s) still wait; builder b was to drop the one for a head and trunk commit being checked", n)
+	}
+	// Another trunk commit is another check.
+	if err := asks.Ask(ctx, aPullCheck("moved", 7, "head-7-000000000", "trunk-2-000000", false)); err != nil {
+		t.Fatal(err)
+	}
+	select {
+	case id := <-took:
+		if id != "moved" {
+			t.Fatalf("builder b took %s", id)
+		}
+	case <-time.After(10 * time.Second):
+		t.Fatal("a check against another trunk commit was dropped too")
+	}
+	close(release)
+}
+
+// A key that is not renewed ages out, and the request for its head and trunk commit is then taken: a builder that died
+// running a check leaves nothing that holds the next one back for longer than the bucket's age.
+func TestAKeyNotRenewedAgesOutAndTheRequestIsThenTaken(t *testing.T) {
+	js := aBusWithTheBuildRole(t)
+	aKeyBucket(t, js)
+	kv := aRunningBucket(t, js, 2*time.Second)
+	ctx, stop := context.WithCancel(t.Context())
+	defer stop()
+	body, _ := json.Marshal(CheckRunning{Head: "head-7-000000000", Trunk: "trunk-1-000000", Builder: "dead", Build: "gone"})
+	if _, err := kv.Put(ctx, PullKeyOf("novox", "a", 7), body); err != nil {
+		t.Fatal(err)
+	}
+	asks := BuildsOn(js, TheBuildMachine)
+	took := make(chan string, 4)
+	b := MachineOverNATS(js, "builder-b")
+	defer b.Close()
+	go func() {
+		_ = b.Take(ctx, func(ctx context.Context, work Build) {
+			took <- work.Request().ID
+			_ = work.Announce(ctx, BuildResult{ID: work.Request().ID, On: "builder-b", Failed: "no"})
+			_ = work.Done()
+		})
+	}()
+	if err := asks.Ask(ctx, aPullCheck("while-it-lives", 7, "head-7-000000000", "trunk-1-000000", false)); err != nil {
+		t.Fatal(err)
+	}
+	select {
+	case id := <-took:
+		t.Fatalf("%s was taken while the key was fresh", id)
+	case <-time.After(1200 * time.Millisecond):
+	}
+	time.Sleep(2500 * time.Millisecond) // the key was not renewed
+	if err := asks.Ask(ctx, aPullCheck("after-it-aged", 7, "head-7-000000000", "trunk-1-000000", false)); err != nil {
+		t.Fatal(err)
+	}
+	select {
+	case id := <-took:
+		if id != "after-it-aged" {
+			t.Fatalf("took %s", id)
+		}
+	case <-time.After(10 * time.Second):
+		t.Fatal("the request was not taken after the key aged out")
+	}
+}
+
+// A builder running a check renews its key on each heartbeat, so it outlives the bucket's age, and deletes it when the check
+// ends, whatever the outcome.
+func TestARunningCheckRenewsItsKeyAndDeletesItWhenItEnds(t *testing.T) {
+	js := aBusWithTheBuildRole(t)
+	kv := aRunningBucket(t, js, 2*time.Second)
+	was := heartbeatWhileWorking
+	heartbeatWhileWorking = 400 * time.Millisecond
+	t.Cleanup(func() { heartbeatWhileWorking = was })
+	ctx, stop := context.WithCancel(t.Context())
+	defer stop()
+	running := make(chan struct{}, 1)
+	release := make(chan struct{})
+	m := MachineOverNATS(js, "builder-a")
+	defer m.Close()
+	go func() {
+		_ = m.Take(ctx, func(ctx context.Context, work Build) {
+			running <- struct{}{}
+			<-release
+			_ = work.Announce(ctx, BuildResult{ID: work.Request().ID, On: "builder-a", Failed: "no"})
+			_ = work.Done()
+		})
+	}()
+	if err := BuildsOn(js, TheBuildMachine).Ask(ctx, aPullCheck("long", 7, "head-7-000000000", "trunk-1-000000", false)); err != nil {
+		t.Fatal(err)
+	}
+	<-running
+	key := PullKeyOf("novox", "a", 7)
+	e, err := kv.Get(ctx, key)
+	if err != nil {
+		t.Fatalf("no key while the check runs: %v", err)
+	}
+	var got CheckRunning
+	_ = json.Unmarshal(e.Value(), &got)
+	if got.Head != "head-7-000000000" || got.Trunk != "trunk-1-000000" || got.Builder != "builder-a" || got.Build != "long" {
+		t.Errorf("the key says %+v", got)
+	}
+	time.Sleep(3 * time.Second) // longer than the bucket's age
+	if _, err := kv.Get(ctx, key); err != nil {
+		t.Fatalf("the key aged out under a check still running: %v", err)
+	}
+	close(release)
+	for end := time.Now().Add(5 * time.Second); ; time.Sleep(50 * time.Millisecond) {
+		if _, err := kv.Get(ctx, key); err != nil {
+			break
+		}
+		if time.Now().After(end) {
+			t.Fatal("the key stays after the check ended")
+		}
+	}
+}