Keep a replay from moving the mesh backwards, and tighten the queue's edges (review of hq ADR 0219)

A registered replay asked now outranked newer asks of its module, and a plan took any later outcome as its answer. A replay is now refused while the module is asked anywhere, a plan module asked under an id is answered by that id alone, and a rebuild of a commit asks what the module follows. An ask handed back after a restart no longer reads as dead; a cancel that meets a start is withdrawn; pause holds for an ask fetched as it lands; kill removes containers before and after the build ends and says whether its outcome went out; a holder may say only its own machine is paused.
2026-10-05 19:45:45 +02:00
parent 541603c15c
commit 9873b3bf13
13 changed files with 600 additions and 106 deletions
@@ -54,7 +54,12 @@ type running struct {
 	started time.Time
 	cancel  context.CancelFunc
 	killed  bool
-	done    chan struct{}
+	// returned is the build's own work having ended, before a kill or not; outcome is what was then
+	// announced, and announced whether it went out.
+	returned  bool
+	outcome   string
+	announced bool
+	done      chan struct{}
 }

 // pausedFile is where the flag lives in the workspace.
@@ -152,13 +157,6 @@ func (h *holder) stepped(r *running, step string) {
 	h.mu.Unlock()
 }

-// wasKilled is whether a person killed this build.
-func (h *holder) wasKilled(r *running) bool {
-	h.mu.Lock()
-	defer h.mu.Unlock()
-	return r.killed
-}
-
 // currentBuild is what `current` answers.
 type currentBuild struct {
 	On      string `json:"on"`
@@ -204,10 +202,19 @@ func (h *holder) current() currentBuild {
 	return out
 }

-// kill ends the build with this id, if it is the one running here: its context cancelled — which
-// kills each command's process group — and the containers it started removed by their label. The
-// build's own goroutine then announces it failed, killed by hand, and settles the ask, so it is not
-// redelivered; this waits a while for that, to say it happened.
+// The bounds a kill keeps: each pass removing containers, and the wait for the build to end between
+// them. The worst case — 15s, 20s, 15s — is inside what the controller waits for the answer
+// (killAnswer in the controller's queue.go, 75s).
+var (
+	killRemoves = 15 * time.Second
+	killWaits   = 20 * time.Second
+)
+
+// kill ends the build with this id, if it is the one running here and still working: its context
+// cancelled — which kills each command's process group — and the containers it started removed by
+// their label, once at once and again after the build has ended, so one created while it was being
+// killed is not left. The build's own goroutine announces it failed, killed by hand, and settles the
+// ask so it is not redelivered; the answer says whether that happened.
 func (h *holder) kill(id string) (string, error) {
 	h.mu.Lock()
 	r := h.running
@@ -219,27 +226,68 @@ func (h *holder) kill(id string) (string, error) {
 		h.mu.Unlock()
 		return "", fmt.Errorf("%s is not building %s; it is building %s. `queue` says where an ask is", h.on, id, doing)
 	}
+	if r.returned {
+		h.mu.Unlock()
+		return "", fmt.Errorf("%s on %s has already ended on its own and is saying how; `builds` shows it", id, h.on)
+	}
 	r.killed = true
 	cancel, done := r.cancel, r.done
 	h.mu.Unlock()

 	cancel()
-	cleanup, stop := context.WithTimeout(context.Background(), 30*time.Second)
-	defer stop()
-	removed, err := builder.RemoveContainersOf(cleanup, h.remove, id)
-	containers := fmt.Sprintf("%d container(s) it started removed", removed)
-	if err != nil {
-		containers = "its containers could not be listed or removed: " + err.Error()
-	}
+	removed, removeErr := h.removeContainers(id)
+	ended := false
 	select {
 	case <-done:
-		return fmt.Sprintf("killed %s (%s) on %s: its commands ended, %s, and its outcome announced as "+
-			"failed, %s — settled, so it is not handed to another machine", id, r.request.Repository, h.on,
-			containers, link.KilledByHand), nil
-	case <-time.After(30 * time.Second):
+		ended = true
+	case <-time.After(killWaits):
+	}
+	again, againErr := h.removeContainers(id)
+	removed += again
+	if removeErr == nil {
+		removeErr = againErr
+	}
+	containers := fmt.Sprintf("%d container(s) it started removed", removed)
+	if removeErr != nil {
+		containers = "its containers could not all be listed or removed: " + removeErr.Error()
+	}
+	if !ended {
 		return fmt.Sprintf("killed %s on %s: %s; the build has not finished ending yet — `builds` says when "+
 			"its outcome is in", id, h.on, containers), nil
 	}
+	h.mu.Lock()
+	outcome, announced := r.outcome, r.announced
+	h.mu.Unlock()
+	if !announced {
+		return fmt.Sprintf("killed %s (%s) on %s: its commands ended and %s, and its outcome could not be "+
+			"announced — the ask is not settled and will be handed out again", id, r.request.Repository, h.on,
+			containers), nil
+	}
+	return fmt.Sprintf("killed %s (%s) on %s: its commands ended, %s, and its outcome announced as failed, %s — "+
+		"settled, so it is not handed to another machine", id, r.request.Repository, h.on, containers, outcome), nil
+}
+
+// removeContainers is one pass of removing what the build left, bounded.
+func (h *holder) removeContainers(id string) (int, error) {
+	cleanup, stop := context.WithTimeout(context.Background(), killRemoves)
+	defer stop()
+	return builder.RemoveContainersOf(cleanup, h.remove, id)
+}
+
+// returned records that the build's work ended, and says whether a kill came first — only then is
+// the build killed; an error it ended with on its own is its own outcome.
+func (h *holder) returned(r *running) bool {
+	h.mu.Lock()
+	defer h.mu.Unlock()
+	r.returned = true
+	return r.killed
+}
+
+// said records the outcome announced, and whether it went out, for a kill to answer with.
+func (h *holder) said(r *running, outcome string, announced bool) {
+	h.mu.Lock()
+	r.outcome, r.announced = outcome, announced
+	h.mu.Unlock()
 }

 // handlers are the seat's verbs, as this machine answers them.
@@ -313,6 +361,8 @@ func announcement(seat, module, node string) micro.Info {
 		endpoints = append(endpoints, micro.EndpointInfo{
 			Name:    seat + "__" + v.Name,
 			Subject: link.NodeSeatToolSubject(seat, v.Name, node),
+			// The queue group the verbs are served in, as every runtime announces its own.
+			QueueGroup: "seat." + seat,
 			Metadata: map[string]string{
 				"kind": "seat", "module": module, "tool": v.Name, "seat": seat, "scope": "node",
 				"node": node, "interchangeable": "false", "description": v.Description, "schema": string(schema),
@@ -66,9 +66,13 @@ func TestKillEndsTheBuildRunningHereAndNoOther(t *testing.T) {
 		!strings.Contains(err.Error(), "build-1") {
 		t.Fatalf("killed another id: %v", err)
 	}
-	// The build's own goroutine: it ends when its context does, as a build's commands do.
+	// The build's own goroutine: it ends when its context does, as a build's commands do, and
+	// announces what came of it.
 	go func() {
 		<-building.Done()
+		if h.returned(r) {
+			h.said(r, link.KilledByHand, true)
+		}
 		h.end(r)
 	}()
 	answer, err := kill(context.Background(), json.RawMessage(`{"id":"build-1"}`))
@@ -78,14 +82,15 @@ func TestKillEndsTheBuildRunningHereAndNoOther(t *testing.T) {
 	if building.Err() == nil {
 		t.Fatal("the build's context was not cancelled")
 	}
-	if !h.wasKilled(r) {
+	if !r.killed {
 		t.Error("the build is not marked killed, so it would be announced as an ordinary failure")
 	}
 	said := answer.(map[string]any)["said"].(string)
-	if !strings.Contains(said, "1 container(s)") || !strings.Contains(said, link.KilledByHand) {
+	if !strings.Contains(said, "2 container(s)") || !strings.Contains(said, "announced as failed") || !strings.Contains(said, link.KilledByHand) {
 		t.Errorf("kill said %q", said)
 	}
-	if len(removed) != 2 || !strings.Contains(removed[0], "label=mesh.build=build-1") {
+	// Removed at the kill and again once the build had ended: a container made in between is caught.
+	if len(removed) != 4 || !strings.Contains(removed[0], "label=mesh.build=build-1") || !strings.Contains(removed[2], "label=mesh.build=build-1") {
 		t.Errorf("removed %v", removed)
 	}
 	if c := h.current(); c.Running != nil {
@@ -101,7 +106,7 @@ func TestAHolderAnnouncesItsMachinesVerbsForTheConsole(t *testing.T) {
 	}
 	kill := info.Endpoints[1]
 	md := kill.Metadata
-	if kill.Subject != "mesh.seat.node-build-agent.tool.kill.ace" || md["kind"] != "seat" || md["seat"] != "node-build-agent" ||
+	if kill.Subject != "mesh.seat.node-build-agent.tool.kill.ace" || kill.QueueGroup != "seat.node-build-agent" || md["kind"] != "seat" || md["seat"] != "node-build-agent" ||
 		md["scope"] != "node" || md["node"] != "ace" || md["tool"] != "kill" || md["module"] != "build-agent" ||
 		!strings.Contains(md["description"], "killed by hand") || !strings.Contains(md["schema"], `"id"`) {
 		t.Fatalf("kill is announced as %+v", kill)
@@ -111,3 +116,33 @@ func TestAHolderAnnouncesItsMachinesVerbsForTheConsole(t *testing.T) {
 		t.Fatalf("the answer is %d bytes (%v)", len(body), err)
 	}
 }
+
+// A build that ended on its own as the kill arrived says what it did: the kill is refused, and its
+// own error is its outcome. One whose outcome could not be announced is not said to be settled.
+func TestAKillArrivingAfterTheBuildEndedIsRefusedAndAnUnannouncedKillSaysSo(t *testing.T) {
+	h := newHolder("ace", link.TheBuildMachine, t.TempDir(), nil)
+	h.remove = func(context.Context, string, string, ...string) (string, error) { return "", nil }
+	_, cancel := context.WithCancel(context.Background())
+	r := h.begin(link.BuildRequest{ID: "build-1"}, cancel)
+	if killed := h.returned(r); killed {
+		t.Fatal("a build nobody killed reads as killed")
+	}
+	if _, err := h.kill("build-1"); err == nil || !strings.Contains(err.Error(), "ended on its own") {
+		t.Fatalf("killed a build that had ended: %v", err)
+	}
+	h.end(r)
+
+	building, cancel := context.WithCancel(context.Background())
+	r = h.begin(link.BuildRequest{ID: "build-2"}, cancel)
+	go func() {
+		<-building.Done()
+		if h.returned(r) {
+			h.said(r, link.KilledByHand, false)
+		}
+		h.end(r)
+	}()
+	said, err := h.kill("build-2")
+	if err != nil || strings.Contains(said, "announced as failed") || !strings.Contains(said, "could not be announced") {
+		t.Fatalf("kill said %q (%v)", said, err)
+	}
+}
@@ -247,8 +247,12 @@ func answer(ctx context.Context, publisher builder.Publisher, on, workspace stri
 			request.Repository, request.Path, request.Ref, workspace, request.Held, npmrc,
 			forgeFrom(), say, request.Seats)
 	}
-	// Only a build the kill ended: one that finished in the moment the kill arrived built, and says so.
-	killed := err != nil && mine != nil && h.wasKilled(mine)
+	// Only a build the kill ended: the kill came before its work did. One that finished — built, or
+	// failed on its own — in the moment the kill arrived says what it did, and the kill is refused.
+	killed := false
+	if mine != nil {
+		killed = h.returned(mine) && err != nil
+	}
 	if killed {
 		// **Killed by hand is the outcome, whatever the build was doing** (novox/hq ADR 0219): the
 		// error it ended with is the kill's consequence, not a fault of the source.
@@ -289,7 +293,11 @@ func answer(ctx context.Context, publisher builder.Publisher, on, workspace stri
 		defer stop()
 		announcing = fresh
 	}
-	if err := work.Announce(announcing, result); err != nil {
+	announceErr := work.Announce(announcing, result)
+	if mine != nil {
+		h.said(mine, result.Failed, announceErr == nil)
+	}
+	if err := announceErr; err != nil {
 		// Said, not fatal: the build happened. A build reported as failed because announcing it
 		// failed is a lie about work that was done — and the request stays unsettled below only if
 		// nothing was said at all, so another machine can try.