From 85b2a1855b83c9397e316ba717becde672c65b8f Mon Sep 17 00:00:00 2001 From: jochen Date: Thu, 8 Oct 2026 10:20:12 +0200 Subject: [PATCH] Raise a check's store without durability and remove what earlier holders left (hq issue 306) On the control node the store-bound packages of the controller's suite ran five to seven times slower than on any other holder, and every controller check that landed there ran past the suite's thirty minutes: a throwaway store flushing to a disk the mesh's own store, bus and forge keep busy. A store that lives for one check needs no crash safety. A holder recreated mid-check left the check's store and bus running, and the redelivery went to another machine, so nothing removed them: eleven pairs across four machines. A starting holder has taken nothing, so every container labelled with an ask of the seat is an earlier holder's. --- cmd/mesh-builder/holder.go | 16 +++++++++++ cmd/mesh-builder/holder_test.go | 24 ++++++++++++++++ cmd/mesh-builder/main.go | 3 ++ internal/builder/check.go | 12 +++++++- internal/builder/check_test.go | 37 ++++++++++++++++++++++++ internal/builder/kill.go | 50 +++++++++++++++++++++++++++++++++ internal/builder/kill_test.go | 37 ++++++++++++++++++++++++ 7 files changed, 178 insertions(+), 1 deletion(-) diff --git a/cmd/mesh-builder/holder.go b/cmd/mesh-builder/holder.go index 1c3edf36..bf127ccf 100644 --- a/cmd/mesh-builder/holder.go +++ b/cmd/mesh-builder/holder.go @@ -274,6 +274,22 @@ func (h *holder) removeContainers(id string) (int, error) { return builder.RemoveContainersOf(cleanup, h.remove, id) } +// removeLeftBehind removes, once, what earlier holders on this machine left — called before anything is +// taken, so none of it is this holder's — and says what it did. A runtime that cannot be asked is said +// and the holder takes work all the same: what is left is waste, not a reason to stop building. +func (h *holder) removeLeftBehind() int { + cleanup, stop := context.WithTimeout(context.Background(), killRemoves) + defer stop() + n, err := builder.RemoveLeftBehind(cleanup, h.remove) + switch { + case err != nil: + fmt.Fprintf(os.Stderr, "could not look for containers earlier builds left on %s: %v\n", h.on, err) + case n > 0: + fmt.Fprintf(os.Stderr, "removed %d container(s) earlier builds left on %s\n", n, h.on) + } + return n +} + // 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 { diff --git a/cmd/mesh-builder/holder_test.go b/cmd/mesh-builder/holder_test.go index c27e1d74..d5ac76d1 100644 --- a/cmd/mesh-builder/holder_test.go +++ b/cmd/mesh-builder/holder_test.go @@ -3,6 +3,7 @@ package main import ( "context" "encoding/json" + "errors" "strings" "testing" @@ -146,3 +147,26 @@ func TestAKillArrivingAfterTheBuildEndedIsRefusedAndAnUnannouncedKillSaysSo(t *t t.Fatalf("kill said %q (%v)", said, err) } } + +// **Issue 306**: a holder starting removes the containers earlier builds left on its machine, through +// the runner a kill uses, and a runtime that cannot be asked does not stop it. +func TestAHolderStartingRemovesWhatEarlierBuildsLeftHere(t *testing.T) { + h := newHolder("novox", link.TheBuildMachine, t.TempDir(), nil) + var removed []string + h.remove = func(_ context.Context, _ string, name string, args ...string) (string, error) { + removed = append(removed, name+" "+strings.Join(args, " ")) + if args[0] == "ps" { + return "c1 build-1\nc2 build-1\nc3 check-here-2\n", nil + } + return "", nil + } + if n := h.removeLeftBehind(); n != 2 || len(removed) != 2 || removed[1] != "docker rm -f c1 c2" { + t.Fatalf("removed %d: %v", n, removed) + } + h.remove = func(context.Context, string, string, ...string) (string, error) { + return "", errors.New("no runtime") + } + if n := h.removeLeftBehind(); n != 0 { + t.Errorf("a runtime that cannot be asked removed %d", n) + } +} diff --git a/cmd/mesh-builder/main.go b/cmd/mesh-builder/main.go index bbb446ac..5a5d34cc 100644 --- a/cmd/mesh-builder/main.go +++ b/cmd/mesh-builder/main.go @@ -129,6 +129,9 @@ func run() error { if h.Paused() { fmt.Fprintf(os.Stderr, "paused (kept in %s): taking no build until resumed\n", pausedFile(workspace)) } + // **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}) defer machine.Close() diff --git a/internal/builder/check.go b/internal/builder/check.go index 367a1411..2223c099 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -346,7 +346,7 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry }() labelled := Labelled(run, spec.ID) store, err := throwaway(ctx, labelled, run, spec.ID+"-store", StoreImage(f.Versions.Store), 5432, - []string{"-e", "POSTGRES_PASSWORD=check"}, nil) + []string{"-e", "POSTGRES_PASSWORD=check"}, StoreSettings) if err != nil { return CheckVerdict{}, err } @@ -848,6 +848,16 @@ func StoreImage(version string) string { return "postgres:" + major + "-alpine" } +// StoreSettings are the throwaway store's server settings: **no durability** (novox/hq issue 306). The +// store lives for one check and is removed after it, so nothing it writes has to survive a crash — +// and a test suite that creates a database per test asks the disk to flush thousands of times. On a +// machine whose disk the mesh's own store, bus and forge already keep busy — the control node — each +// flush waited its turn: the store-bound packages of the controller's suite ran five to seven times +// slower there than on any other holder, while those that touch no store ran alike, and its check ran +// past the suite's thirty minutes every time it landed there. The release stays the mesh's; only what +// a crash would need is switched off. +var StoreSettings = []string{"-c", "fsync=off", "-c", "synchronous_commit=off", "-c", "full_page_writes=off"} + // BusImage is the bus a check stands on: the release the mesh's bus server runs. func BusImage(version string) string { v := strings.TrimPrefix(strings.TrimSpace(version), "v") diff --git a/internal/builder/check_test.go b/internal/builder/check_test.go index 5f0db893..92583f5a 100644 --- a/internal/builder/check_test.go +++ b/internal/builder/check_test.go @@ -33,6 +33,43 @@ func TestTheStoreAndBusAChecksStandsOnAreTheOnesTheMeshRuns(t *testing.T) { } } +// **Issue 306**: the throwaway store is raised without durability — it lives for one check — so a suite +// that makes a database per test does not wait on the disk of a busy machine for every flush. +func TestTheThrowawayStoreFlushesNothing(t *testing.T) { + said := strings.Join(StoreSettings, " ") + for _, want := range []string{"-c fsync=off", "-c synchronous_commit=off", "-c full_page_writes=off"} { + if !strings.Contains(said, want) { + t.Errorf("the store is raised with %q, not %q", said, want) + } + } + if os.Getenv("MESH_TEST_DOCKER") != "1" { + t.Skip("MESH_TEST_DOCKER=1: the settings are proven on a raised store") + } + id := fmt.Sprintf("check-store-%d", time.Now().UnixNano()) + labelledRun := Labelled(Command, id) + t.Cleanup(func() { _, _ = RemoveContainersOf(context.Background(), Command, id) }) + if _, err := throwaway(t.Context(), labelledRun, Command, id+"-store", StoreImage("17.11"), 5432, + []string{"-e", "POSTGRES_PASSWORD=check"}, StoreSettings); err != nil { + t.Fatal(err) + } + if err := waitFor(t.Context(), Command, id+"-store", []string{"pg_isready", "-U", "postgres"}); err != nil { + t.Fatal(err) + } + for setting, want := range map[string]string{"fsync": "off", "synchronous_commit": "off", "full_page_writes": "off"} { + var got string + for try := 0; try < 10; try++ { + out, err := exec.Command("docker", "exec", id+"-store", "psql", "-U", "postgres", "-tAc", "SHOW "+setting).CombinedOutput() + if got = strings.TrimSpace(string(out)); err == nil { + break + } + time.Sleep(500 * time.Millisecond) + } + if got != want { + t.Errorf("%s is %q on the raised store", setting, got) + } + } +} + // aRepository is a git repository holding these files, committed, and its head. func aCheckedRepository(t *testing.T, files map[string]string) (string, string) { t.Helper() diff --git a/internal/builder/kill.go b/internal/builder/kill.go index a46dec20..9200da5b 100644 --- a/internal/builder/kill.go +++ b/internal/builder/kill.go @@ -76,3 +76,53 @@ func RemoveContainersOf(ctx context.Context, run Runner, id string) (int, error) } return len(ids), nil } + +// A holder that stops mid-build leaves its containers running (novox/hq issue 306). +// +// A build's containers are removed by the build itself, by a kill, or by the next delivery of the same +// ask — but only on the machine that delivery reaches. A holder recreated by a rollout while a check +// ran left the check's throwaway store and bus running, the ask went to another machine, and nothing +// on the first one ever looked at them again: eleven pairs across four machines in two days, each a +// postgres that had written its share of the disk. A holder starting has taken nothing yet, so every +// container labelled with an ask of the seat is one an earlier holder here left, and is removed then. + +// RemoveLeftBehind removes every container on this machine labelled with an ask of the build seat — +// an id of the seat's shape, `build-` — and says how many. Called by a holder before it takes +// anything, so none of them can be a build it runs. A container labelled with any other id — a check +// a person runs by hand (`check-here-…`), a test's — is not the seat's, and is left. +func RemoveLeftBehind(ctx context.Context, run Runner) (int, error) { + out, err := run(ctx, "", "docker", "ps", "-a", "--filter", "label="+BuildLabel, + "--format", `{{.ID}} {{.Label "`+BuildLabel+`"}}`) + if err != nil { + return 0, err + } + var ids []string + for _, line := range strings.Split(out, "\n") { + fields := strings.Fields(line) + if len(fields) == 2 && anAskOfTheSeat(fields[1]) { + ids = append(ids, fields[0]) + } + } + if len(ids) == 0 { + return 0, nil + } + if _, err := run(ctx, "", "docker", append([]string{"rm", "-f"}, ids...)...); err != nil { + return 0, err + } + return len(ids), nil +} + +// anAskOfTheSeat is whether an id is of the shape the controller gives the seat's asks: `build-` and the +// moment it was asked, in nanoseconds. +func anAskOfTheSeat(id string) bool { + n, ok := strings.CutPrefix(id, "build-") + if !ok || n == "" { + return false + } + for _, c := range n { + if c < '0' || c > '9' { + return false + } + } + return true +} diff --git a/internal/builder/kill_test.go b/internal/builder/kill_test.go index 03a7c90f..30b63c9b 100644 --- a/internal/builder/kill_test.go +++ b/internal/builder/kill_test.go @@ -101,3 +101,40 @@ func TestAKillRemovesTheContainersLabelledWithTheBuild(t *testing.T) { t.Errorf("ran %v", ran) } } + +// **Issue 306**: a holder starting removes what earlier holders on its machine left — every container +// labelled with an ask of the seat — and nothing labelled otherwise. +func TestAStartingHolderRemovesWhatEarlierHoldersLeftAndNothingElse(t *testing.T) { + var ran [][]string + run := func(_ context.Context, _ string, name string, args ...string) (string, error) { + ran = append(ran, append([]string{name}, args...)) + if args[0] == "ps" { + return "c1 build-1791418000320743455\nc2 build-1791418000320743455\n" + + "c3 check-here-1791418000320743455\nc4 check-test-17\nc5 build-x\nc6 build-\n\nc7 build-1791319895953076871\n", nil + } + return "", nil + } + n, err := RemoveLeftBehind(context.Background(), run) + if err != nil { + t.Fatal(err) + } + if n != 3 { + t.Errorf("removed %d", n) + } + if len(ran) != 2 || !reflect.DeepEqual(ran[1], []string{"docker", "rm", "-f", "c1", "c2", "c7"}) { + t.Fatalf("ran %v", ran) + } + if !strings.Contains(strings.Join(ran[0], " "), "--filter label=mesh.build") { + t.Errorf("listed with %v", ran[0]) + } + + // Nothing left behind: nothing removed, and no rm asked. + ran = nil + quiet := func(_ context.Context, _ string, name string, args ...string) (string, error) { + ran = append(ran, append([]string{name}, args...)) + return "c3 check-here-1\n", nil + } + if n, err := RemoveLeftBehind(context.Background(), quiet); err != nil || n != 0 || len(ran) != 1 { + t.Errorf("with nothing of the seat's: %d %v %v", n, err, ran) + } +}