Merge pull request 'Mark that the build agent takes work, so its health check can see it ready (hq issue 544)' (#277) from fix/544-the-build-agent-says-it-is-ready into main

cmd/mesh-builder/main.go

@@ -66,6 +66,11 @@ broker to take work from or a registry to publish into:
 
 Without --registry the artifacts stay in this machine's container runtime, named by the
 digest of their own configuration. The result is printed as JSON.
+
+  mesh-builder ready
+
+exits 0 once the agent running in this container is bound to its build work and pulls it, and
+1 before: its container's health check.
 `
 
 func run() error {
@@ -76,12 +81,18 @@ func run() error {
 			return nil
 		case "build":
 			return buildOnce(context.Background(), os.Args[2:])
+		case "ready":
+			// The container's health check (novox/hq issue 544): exits non-zero until the agent takes work.
+			return ready()
 		default:
 			fmt.Print(usage)
 			return nil
 		}
 	}
 
+	if err := notReady(); err != nil {
+		return err
+	}
 	credential, err := brokerFrom()
 	if err != nil {
 		return err
@@ -134,7 +145,8 @@ func run() error {
 	// **What an earlier holder here left goes before anything is taken** (novox/hq issue 306): a holder
 	// recreated mid-check left the check's store and bus running, and the ask went to another machine.
 	h.removeLeftBehind()
-	machine := link.MachineOverNATSWith(js, on, seat, link.MachineOptions{Paused: h.Paused})
+	machine := link.MachineOverNATSWith(js, on, seat, link.MachineOptions{Paused: h.Paused,
+		Taking: func() { taking(seat) }})
 	defer machine.Close()
 
 	// The seat's verbs, on this machine's subjects. Only the seat that declares them: the retired

cmd/mesh-builder/ready.go

@@ -0,0 +1,44 @@
+package main
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"path/filepath"
+)
+
+// **The build agent says it is ready** (novox/hq issue 544). It listens on nothing, so no http or tcp
+// check can see it work; it writes a mark once it is bound to its worker and pulls build work, and its
+// container's health check runs `mesh-builder ready`, which passes only while the mark is there. Without
+// it the node-engine could judge the agent only alive, and every walk that moved it waited out the whole
+// grace on each machine.
+//
+// The mark lives in the container's own temporary directory, never in the workspace the machine keeps:
+// a new container starts without it, and a restarted one removes it before it binds again.
+func readyMark() string { return filepath.Join(os.TempDir(), "mesh-builder.ready") }
+
+// notReady removes the mark: a holder starting is not taking work until it says so again.
+func notReady() error {
+	if err := os.Remove(readyMark()); err != nil && !errors.Is(err, os.ErrNotExist) {
+		return fmt.Errorf("cannot remove the mark that this agent takes build work (%s): %w", readyMark(), err)
+	}
+	return nil
+}
+
+// taking writes the mark. A mark that cannot be written is said, and the agent goes on building: its
+// check then says it is not ready, which is the truth the gate should hear.
+func taking(seat string) {
+	if err := os.WriteFile(readyMark(), []byte(seat+"\n"), 0o644); err != nil {
+		fmt.Fprintf(os.Stderr, "cannot mark that this agent takes build work (%s): %v\n", readyMark(), err)
+		return
+	}
+	fmt.Fprintf(os.Stderr, "taking build work from %s\n", seat)
+}
+
+// ready is the health check: nil when this container's agent is bound and pulling build work.
+func ready() error {
+	if _, err := os.Stat(readyMark()); err != nil {
+		return fmt.Errorf("not taking build work yet: %s is not there", readyMark())
+	}
+	return nil
+}

cmd/mesh-builder/ready_test.go

@@ -0,0 +1,32 @@
+package main
+
+import (
+	"os"
+	"testing"
+)
+
+// The health check passes only between the agent saying it takes work and its next start (novox/hq
+// issue 544).
+func TestTheAgentIsReadyOnlyOnceItTakesWork(t *testing.T) {
+	t.Setenv("TMPDIR", t.TempDir())
+	if err := ready(); err == nil {
+		t.Fatal("an agent that never took work is ready")
+	}
+	taking("mesh-build-machine")
+	if err := ready(); err != nil {
+		t.Fatalf("an agent taking work is not ready: %v", err)
+	}
+	// Started again, it is not ready until it binds again.
+	if err := notReady(); err != nil {
+		t.Fatal(err)
+	}
+	if err := ready(); err == nil {
+		t.Fatal("an agent starting again is ready before it takes work")
+	}
+	if err := notReady(); err != nil {
+		t.Fatalf("removing a mark that is not there failed: %v", err)
+	}
+	if _, err := os.Stat(readyMark()); err == nil {
+		t.Fatal("the mark is still there")
+	}
+}

internal/link/builds_nats.go

@@ -167,6 +167,10 @@ type MachineOptions struct {
 	// Paused is asked before every fetch: while it says so, nothing new is taken, and a build
 	// already running finishes. Nil is never paused.
 	Paused func() bool
+	// Taking is called once, when this machine is bound to its worker and pulls work: from then on it
+	// can take a build, which is what its container's health check reads (novox/hq issue 544). Nil is
+	// told nothing.
+	Taking func()
 }
 
 // MachineOverNATS takes build work from the current build role.
@@ -232,6 +236,9 @@ func (m *natsMachine) Take(ctx context.Context, do func(context.Context, Build))
 				"the mesh's to answer, not this machine's", m.seat, err)
 	}
 	m.sub = sub
+	if m.opts.Taking != nil {
+		m.opts.Taking()
+	}
 
 	for {
 		if ctx.Err() != nil {

internal/link/builds_nats_test.go

@@ -368,3 +368,36 @@ func TestNatsAMachineOnTheRetiredBuildRoleTakesThatRolesAsks(t *testing.T) {
 		t.Errorf("the retired role's holder did not answer: %+v", result)
 	}
 }
+
+// A holder says once that it takes work, and only once it is bound to its worker (novox/hq issue 544):
+// its container's health check reads that, so a holder that could not bind is never said ready.
+func TestNatsAHolderSaysItTakesWorkOnceItIsBound(t *testing.T) {
+	js := aBusWithTheBuildRole(t)
+	ctx, stop := context.WithCancel(context.Background())
+	defer stop()
+	said := make(chan struct{}, 4)
+	machine := MachineOverNATSWith(js, "anchor", TheBuildMachine, MachineOptions{Taking: func() { said <- struct{}{} }})
+	defer machine.Close()
+	go func() { _ = machine.Take(ctx, func(context.Context, Build) {}) }()
+	select {
+	case <-said:
+	case <-time.After(5 * time.Second):
+		t.Fatal("a holder bound to its worker never said it takes work")
+	}
+	time.Sleep(300 * time.Millisecond)
+	if len(said) != 0 {
+		t.Fatalf("a holder said it takes work %d more time(s)", len(said))
+	}
+
+	// A role the mesh raised no worker for cannot be bound, and its holder says nothing.
+	bounded, end := context.WithTimeout(ctx, 10*time.Second)
+	defer end()
+	other := MachineOverNATSWith(js, "anchor", "mesh-no-such-seat", MachineOptions{Taking: func() { said <- struct{}{} }})
+	defer other.Close()
+	if err := other.Take(bounded, func(context.Context, Build) {}); err == nil {
+		t.Fatal("a machine with no worker took work")
+	}
+	if len(said) != 0 {
+		t.Fatal("a machine that could not bind said it takes work")
+	}
+}