Remove an image's pushed names with its build tag, and take no work under the free-space floor (issue 1834)

The registry push tags the image a second time, so removing only the build tag freed nothing.
The held image a comparison pulls goes after it is read, and a node under the floor leaves the ask
to another build node instead of taking it and failing.

cmd/mesh-builder/busy.go

@@ -9,6 +9,8 @@ import (
 	"sync"
 	"sync/atomic"
 	"time"
+
+	"github.com/novox/mesh-controller/internal/builder"
 )
 
 // **The build agent says whether it is busy, and being asked drains it** (novox/hq ADR 0335 §1, §5). Its
@@ -211,3 +213,23 @@ func busy() (string, error) {
 	}
 	return fmt.Sprintf("busy: %s, in hand since %s; taking no new build", what, since), nil
 }
+
+var (
+	roomMu   sync.Mutex
+	roomSaid bool
+)
+
+// noRoom says this node is under the floor of free space (novox/hq issue 1834): it takes no work, so another build node
+// takes the ask instead of this one taking it and ending it in an error. Said once when it starts and again when it ends.
+func noRoom(workspace string) bool {
+	err := builder.RoomToWork("/", workspace)
+	roomMu.Lock()
+	defer roomMu.Unlock()
+	if err != nil && !roomSaid {
+		fmt.Fprintf(os.Stderr, "taking no builds: %v\n", err)
+	} else if err == nil && roomSaid {
+		fmt.Fprintf(os.Stderr, "there is room again: taking builds\n")
+	}
+	roomSaid = err != nil
+	return err != nil
+}

cmd/mesh-builder/busy_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"github.com/novox/mesh-controller/internal/builder"
 	"os"
 	"strings"
 	"sync"
@@ -221,3 +222,18 @@ func TestAnAgentThatCannotMarkABuildTakesNoWorkUntilItCan(t *testing.T) {
 	}
 	released()
 }
+
+// 1834/AC4: given a node under the floor of free space, when the taking loop asks, then it takes nothing, and takes again
+// once there is room.
+func TestANodeUnderTheFloorOfFreeSpaceTakesNoWork(t *testing.T) {
+	was := builder.FreeBytes
+	t.Cleanup(func() { builder.FreeBytes = was })
+	builder.FreeBytes = func(string) (uint64, error) { return 2 << 30, nil }
+	if !noRoom(t.TempDir()) {
+		t.Error("a node with 2 GiB free took work")
+	}
+	builder.FreeBytes = func(string) (uint64, error) { return 200 << 30, nil }
+	if noRoom(t.TempDir()) {
+		t.Error("a node with 200 GiB free refused work")
+	}
+}

cmd/mesh-builder/main.go

@@ -190,7 +190,7 @@ func run() error {
 	// Paused or drained, it takes nothing new (novox/hq ADR 0219, ADR 0335): a drained agent finishes the build
 	// in hand and then says idle, and its container is replaced between builds.
 	machine := link.MachineOverNATSWith(js, on, seat, link.MachineOptions{
-		Paused: func() bool { return h.Paused() || drained() || markFaulty(time.Now()) },
+		Paused: func() bool { return h.Paused() || drained() || markFaulty(time.Now()) || noRoom(workspace) },
 		Taking: func() { taking(seat) },
 		Claim:  claim,
 	})

internal/builder/builder.go

@@ -752,18 +752,25 @@ func keepsImageLocally(publish Publisher) bool {
 	return false
 }
 
-// releaseImage removes the local image tag a build made, with a context of its own: the build's may be cancelled. A
-// tag that was never made (the build failed before it) is no fault; one that cannot be removed is said.
-func releaseImage(ctx context.Context, run Runner, tag string, say func(step, format string, args ...any)) {
+// PushedNames is implemented by a publisher that tags the image under other names to push it: the names it leaves in the
+// machine's runtime for a repository, which go with the build's own tag (novox/hq issue 1834).
+type PushedNames interface {
+	PushedNames(repository string) []string
+}
+
+// releaseImage removes the names an image was given on the node, in one command: its layers go only with the last. It runs
+// with a context of its own, the build's may be cancelled. A name never made (the build failed before it) is no fault;
+// one that cannot be removed is said, and removal is said only when the image is gone.
+func releaseImage(ctx context.Context, run Runner, names []string, say func(step, format string, args ...any)) {
 	removing, done := context.WithTimeout(context.WithoutCancel(ctx), time.Minute)
 	defer done()
-	if _, err := run(removing, "", "docker", "rmi", tag); err != nil {
+	if _, err := run(removing, "", "docker", append([]string{"rmi"}, names...)...); err != nil {
 		if !strings.Contains(err.Error(), "No such image") {
-			say("image", "the local image %s could not be removed, and goes with the week's sweep: %v", tag, err)
+			say("image", "the local image %s could not be removed, and goes with the week's sweep: %v", strings.Join(names, ", "), err)
 		}
 		return
 	}
-	say("image", "the local image %s is removed: the registry holds it", tag)
+	say("image", "the local image %s is removed: the registry holds it", strings.Join(names, ", "))
 }
 
 // contextFrom clones an image artifact's own build context, when it names one apart from this
@@ -1246,7 +1253,11 @@ func one(ctx context.Context, run Runner, publish Publisher,
 		// a tag per module and commit left on the node was the 96 GB of a full disk. Only a node publishing nowhere (a
 		// mesh being raised) keeps it, since the local image is then the artifact.
 		if !keepsImageLocally(publish) {
-			defer releaseImage(ctx, run, local, say)
+			names := []string{local}
+			if p, ok := publish.(PushedNames); ok {
+				names = append(names, p.PushedNames(module+"/"+a.Name)...)
+			}
+			defer releaseImage(ctx, run, names, say)
 		}
 		if _, err := run(ctx, buildDir, "docker", invocation...); err != nil {
 			return catalogue.Built{}, fmt.Errorf("%s: building %s failed: %w", module, a.Name, err)

internal/builder/disk_1834_test.go

@@ -133,18 +133,26 @@ func TestTheThrowawayStoreIsNotStartedWithRm(t *testing.T) {
 // 1834/AC1: given an image built for a check or a walk and published to the registry, when the build ends, then the
 // local image tag, the narrowed source and the context clone it left are gone; an image published nowhere (a mesh
 // being raised) is kept, since it is the artifact.
+// pushedNamed is a publisher that tags the image as <registry>/<repository> to push it, as the registry one does.
+type pushedNamed struct{ *recorded }
+
+func (pushedNamed) PushedNames(repository string) []string {
+	return []string{"registry.invalid/" + repository}
+}
+
 func TestABuildRemovesWhatItLeftOnTheNode(t *testing.T) {
 	r := &recorded{
 		contents:  map[string]string{"modules/m/" + ManifestName: anImageWithAContext, "modules/m/Dockerfile": "FROM scratch\n"},
 		secondary: map[string]map[string]string{"https://forge.invalid/ctl.git": {"go.mod": "module x\n"}},
 	}
 	workspace := t.TempDir()
-	if _, err := Build(context.Background(), r.run, r, "https://forge.invalid/c.git", "modules/m", "", workspace, nil, Npmrc{},
+	if _, err := Build(context.Background(), r.run, pushedNamed{r}, "https://forge.invalid/c.git", "modules/m", "", workspace, nil, Npmrc{},
 		GitCredential{}, nil); err != nil {
 		t.Fatal(err)
 	}
-	if !strings.Contains(strings.Join(r.ran, "\n"), "docker rmi m-server:c0ffeec0") {
-		t.Errorf("the image built for it was not removed: %v", r.ran)
+	// Both names go in one rmi: the layers stay while either is left (the push tagged the image with the registry's).
+	if !strings.Contains(strings.Join(r.ran, "\n"), "docker rmi m-server:c0ffeec0 registry.invalid/m/server") {
+		t.Errorf("the image built for it and its pushed name were not removed: %v", r.ran)
 	}
 	left, _ := filepath.Glob(filepath.Join(workspace, "*"))
 	if len(left) != 0 {
@@ -172,3 +180,37 @@ func TestAnImageKeptLocallyIsNotRemoved(t *testing.T) {
 		t.Errorf("the artifact of a mesh being raised was removed: %v", r.ran)
 	}
 }
+
+// The held image the comparison pulls to read is let go of once read.
+func TestTheHeldImageAComparisonPullsIsRemovedAfterReading(t *testing.T) {
+	var ran []string
+	run := func(_ context.Context, _ string, name string, args ...string) (string, error) {
+		ran = append(ran, name+" "+strings.Join(args, " "))
+		if args[0] == "create" {
+			return "abcdef0123456789\n", nil
+		}
+		if args[0] == "image" {
+			return "", errors.New("stop here")
+		}
+		return "", nil
+	}
+	_, _ = imageContent(context.Background(), run, t.TempDir(), "reg.invalid/m/server@sha256:"+strings.Repeat("a", 64))
+	if got := ran[len(ran)-1]; !strings.HasPrefix(got, "docker rmi reg.invalid/m/server@sha256:") {
+		t.Errorf("the last command was %q, not the removal of the pulled image: %v", got, ran)
+	}
+}
+
+func TestAnImageThatCannotBeRemovedIsNotSaidRemoved(t *testing.T) {
+	var said []string
+	run := func(context.Context, string, string, ...string) (string, error) {
+		return "", errors.New("image is being used")
+	}
+	releaseImage(context.Background(), run, []string{"a:1"}, func(step, format string, args ...any) {
+		said = append(said, format)
+	})
+	for _, s := range said {
+		if strings.Contains(s, "is removed") {
+			t.Errorf("said removed: %q", s)
+		}
+	}
+}

internal/builder/registry.go

@@ -43,6 +43,11 @@ type Registry struct {
 	HTTP *http.Client
 }
 
+// PushedNames are the names PublishImage leaves the image under on the node.
+func (r Registry) PushedNames(repository string) []string {
+	return []string{r.Address + "/" + repository}
+}
+
 // PublishImage pushes a locally built image and returns a reference pinned by digest.
 //
 // The digest is read back from the registry's own answer rather than computed here. What matters

internal/builder/reuse.go

@@ -253,6 +253,13 @@ func (s scratchPublisher) PublishImage(ctx context.Context, localTag, repository
 	return s.to.PublishImage(ctx, localTag, ScratchPrefix+repository)
 }
 
+func (s scratchPublisher) PushedNames(repository string) []string {
+	if p, ok := s.to.(PushedNames); ok {
+		return p.PushedNames(ScratchPrefix + repository)
+	}
+	return nil
+}
+
 func (s scratchPublisher) PublishArchive(ctx context.Context, repository string, body []byte, digest string) (string, error) {
 	return s.to.PublishArchive(ctx, ScratchPrefix+repository, body, digest)
 }

internal/builder/same_program.go

@@ -88,6 +88,9 @@ func imageContent(ctx context.Context, run Runner, dir, ref string) (string, err
 	if _, err := run(ctx, dir, "docker", "pull", "--quiet", ref); err != nil {
 		return "", fmt.Errorf("pulling %s: %w", ref, err)
 	}
+	// The image is on the node only for the reading (novox/hq issue 1834); one a container still runs is refused by
+	// docker and stays.
+	defer func() { _, _ = run(context.WithoutCancel(ctx), dir, "docker", "rmi", ref) }()
 	config, err := run(ctx, dir, "docker", "image", "inspect", "--format",
 		"{{.Os}}/{{.Architecture}}/{{.Variant}} {{json .Config}}", ref)
 	if err != nil {