From dd802c81a7d5597cdc2d2f18bf8b07d9d035ba96 Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 15:22:00 +0200 Subject: [PATCH 1/2] Pass a check's changed paths in a file so a change of thousands of files can start (issue 1247) One environment string over 128 KiB made the container fail to start with no words; the gate reads --changed-file, and a command that cannot start now says its error. --- cmd/mesh-controller/merge_gate.go | 26 ++++- .../merge_gate_changed_test.go | 31 +++++ internal/builder/check.go | 61 +++++++++- internal/builder/check_changed_test.go | 109 ++++++++++++++++++ internal/builder/check_integration_test.go | 30 +++++ 5 files changed, 251 insertions(+), 6 deletions(-) create mode 100644 cmd/mesh-controller/merge_gate_changed_test.go create mode 100644 internal/builder/check_changed_test.go diff --git a/cmd/mesh-controller/merge_gate.go b/cmd/mesh-controller/merge_gate.go index 854f39ef..5557b96e 100644 --- a/cmd/mesh-controller/merge_gate.go +++ b/cmd/mesh-controller/merge_gate.go @@ -154,6 +154,24 @@ type mergeWidthOf struct { // wideRebuild is how many modules a rebuild may take before the gate says so as a warning. var wideRebuild = 12 +// changedPaths are the paths --changed (comma-separated) and --changed-file (one per line) name together. +func changedPaths(list, file string) ([]string, error) { + out := splitList(list) + if file == "" { + return out, nil + } + raw, err := os.ReadFile(file) + if err != nil { + return nil, fmt.Errorf("--changed-file: %w", err) + } + for _, line := range strings.Split(string(raw), "\n") { + if line = strings.TrimSpace(line); line != "" { + out = append(out, line) + } + } + return out, nil +} + // mergeGateCommand is `gate`: // // merge-gate --facts --store [--repository owner/repo --tree ] [--changed a,b] [--json] @@ -167,6 +185,8 @@ func mergeGateCommand(ctx context.Context, args []string) error { repository := set.String("repository", "", "owner/repository of the change") tree := set.String("tree", "", "the change's checkout: every module.json in it replaces the mesh's") changed := set.String("changed", "", "the paths the change touches, comma-separated, for its rebuild's width") + changedFile := set.String("changed-file", "", "a file of the paths the change touches, one per line; the same as "+ + "--changed, for a list too long for an argument (novox/hq issue 1247)") group := set.String("group", "", "a delivery group's other heads, as JSON [{repository, tree, changed}], composed "+ "with this one as one future state (novox/hq ADR 0239)") asJSON := set.Bool("json", false, "print the verdict as JSON") @@ -198,7 +218,11 @@ func mergeGateCommand(ctx context.Context, args []string) error { // verdict at all. What is said goes beside the report. os.Stdout = os.Stderr } - v, err := judgeChange(ctx, mergeCheckInput{facts: f, repository: *repository, tree: *tree, changed: splitList(*changed), + changedList, err := changedPaths(*changed, *changedFile) + if err != nil { + return err + } + v, err := judgeChange(ctx, mergeCheckInput{facts: f, repository: *repository, tree: *tree, changed: changedList, group: others, admin: *admin, say: out, beside: os.Getenv("MESH_CHECK_BESIDE")}) os.Stdout = stdout if err != nil { diff --git a/cmd/mesh-controller/merge_gate_changed_test.go b/cmd/mesh-controller/merge_gate_changed_test.go new file mode 100644 index 00000000..4772c0d8 --- /dev/null +++ b/cmd/mesh-controller/merge_gate_changed_test.go @@ -0,0 +1,31 @@ +package main + +import ( + "os" + "path/filepath" + "reflect" + "strings" + "testing" +) + +func TestTheGateJudgesAFileOfPathsAsItDoesTheSameListOfThem(t *testing.T) { // 1247/AC3 + paths := []string{"modules/a/x.go", "modules/a/y.go", "modules/b/z.go", "README.md"} + file := filepath.Join(t.TempDir(), "changed.txt") + if err := os.WriteFile(file, []byte(strings.Join(paths, "\n")+"\n"), 0o644); err != nil { + t.Fatal(err) + } + fromFile, err := changedPaths("", file) + if err != nil { + t.Fatal(err) + } + fromList, err := changedPaths(strings.Join(paths, ","), "") + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(fromFile, fromList) || len(fromFile) != len(paths) { + t.Errorf("the file gives %v, the list %v", fromFile, fromList) + } + if _, err := changedPaths("", filepath.Join(t.TempDir(), "none")); err == nil { + t.Error("a file that is not there is read as no paths") + } +} diff --git a/internal/builder/check.go b/internal/builder/check.go index d25d2687..9d9547ae 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -260,9 +260,12 @@ const ( EnvTestBus = "MESH_TEST_NATS" EnvRepository = "MESH_CHECK_REPOSITORY" EnvChanged = "MESH_CHECK_CHANGED" - EnvVerdict = "MESH_CHECK_VERDICT" - EnvBeside = "MESH_CHECK_BESIDE" - EnvModules = "MESH_CHECK_MODULES" + // EnvChangedFile names a file holding every changed path, one per line, in the order given: the whole + // list however long, which one environment string cannot carry (novox/hq issue 1247). + EnvChangedFile = "MESH_CHECK_CHANGED_FILE" + EnvVerdict = "MESH_CHECK_VERDICT" + EnvBeside = "MESH_CHECK_BESIDE" + EnvModules = "MESH_CHECK_MODULES" // EnvGroup is a delivery group's other heads, as JSON, for the gate (novox/hq ADR 0239); empty for a // pull request checked alone. EnvGroup = "MESH_CHECK_GROUP" @@ -719,12 +722,17 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry judged.JudgeFrom = siblingJudge.From judged.Validator = siblingJudge.Validator verdictFile := filepath.Join(root, "verdict.json") + changedEnv, err := changedEnvironment(root, paths, func(line string) { fmt.Fprintln(&out, line) }) + if err != nil { + return CheckVerdict{}, err + } env := []string{EnvFacts + "=" + factsFile, EnvGate + "=" + gate, EnvGateStore + "=" + storeURL, EnvTestStore + "=" + storeURL, EnvTestBus + "=nats://" + bus, EnvRepository + "=" + spec.Owner + "/" + spec.Repo, - EnvChanged + "=" + strings.Join(paths, ","), EnvBeside + "=" + root, + EnvBeside + "=" + root, EnvModules + "=" + strings.Join(append(append([]string{}, spec.Modules...), spec.New...), ","), EnvGroup + "=" + groupFile, "GOCACHE=" + filepath.Join(workspace, "go-cache"), "GOMODCACHE=" + filepath.Join(workspace, "go-modules")} + env = append(env, changedEnv...) env = append(env, selection(spec)...) v := CheckVerdict{} @@ -919,6 +927,10 @@ func ownCheck(ctx context.Context, spec CheckSpec, parts []ScriptPart, tree stri if said == "" { said = whatFailed(own.String()) } + if said == "" { + // A command that could not start says nothing itself (novox/hq issue 1247): its error does. + said = err.Error() + } return &Layer{Verdict: "fail", Summary: passedSoFar(ran) + "its " + part.Script + " failed: " + said, Failed: picked.String()} } @@ -1084,7 +1096,7 @@ func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFil // 2. Every machine composed with the change. if err := running(inToolchain(tree, append(env, EnvVerdict+"="+verdictFile), "sh", "-c", `"$MESH_GATE" merge-gate --facts "$MESH_FACTS" --store "$MESH_GATE_POSTGRES" `+ - `--repository "$MESH_CHECK_REPOSITORY" --tree . --changed "$MESH_CHECK_CHANGED" `+ + `--repository "$MESH_CHECK_REPOSITORY" --tree . --changed-file "$MESH_CHECK_CHANGED_FILE" `+ `${MESH_CHECK_GROUP:+--group "$MESH_CHECK_GROUP"} --json > "$MESH_CHECK_VERDICT"`)); err != nil { if ctx.Err() != nil { return "error", "the check was ended during the merge gate" @@ -1895,3 +1907,42 @@ func treeAt(ctx context.Context, run Runner, dir string) string { } return strings.TrimSpace(out) } + +// changedEnvLimit is how long MESH_CHECK_CHANGED may be: well under the kernel's 128 KiB for one environment +// string, over which the container cannot start (novox/hq issue 1247). +const changedEnvLimit = 64 << 10 + +// changedEnvironment is the changed paths as a check reads them: every path, one per line, in a file beside the +// check's checkouts that MESH_CHECK_CHANGED_FILE names; and MESH_CHECK_CHANGED, the same comma-joined while +// that is under changedEnvLimit, otherwise each path's first two directories (a top-level file as itself) once +// each, said in the check's output. +func changedEnvironment(root string, paths []string, say func(string)) ([]string, error) { + file := filepath.Join(root, "changed.txt") + body := "" + if len(paths) > 0 { + body = strings.Join(paths, "\n") + "\n" + } + if err := os.WriteFile(file, []byte(body), 0o644); err != nil { + return nil, err + } + joined := strings.Join(paths, ",") + if len(joined) >= changedEnvLimit { + var short []string + seen := map[string]bool{} + for _, p := range paths { + parts := strings.SplitN(p, "/", 3) + d := p + if len(parts) == 3 { + d = parts[0] + "/" + parts[1] + "/" + } + if !seen[d] { + seen[d] = true + short = append(short, d) + } + } + joined = strings.Join(short, ",") + say(fmt.Sprintf("--- %d changed paths are too long for one environment string: %s holds all of them, one per "+ + "line, and %s only each path's first two directories", len(paths), EnvChangedFile, EnvChanged)) + } + return []string{EnvChanged + "=" + joined, EnvChangedFile + "=" + file}, nil +} diff --git a/internal/builder/check_changed_test.go b/internal/builder/check_changed_test.go new file mode 100644 index 00000000..e85f7e62 --- /dev/null +++ b/internal/builder/check_changed_test.go @@ -0,0 +1,109 @@ +package builder + +import ( + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// manyPaths is a list shaped like the real one of mesh-delivery feat/delivery-parts 9e9c2a7 (novox/hq issue +// 1247): 2494 vendored files of nats-server across four new modules, about 208 KB comma-joined. +func manyPaths() []string { + modules := []string{"mesh-delivery-forge-bridge", "mesh-delivery-indexer", "mesh-delivery-teller", "mesh-delivery-view"} + var paths []string + for i := 0; i < 2494; i++ { + paths = append(paths, fmt.Sprintf("modules/%s/vendor/github.com/nats-io/nats-server/v2/server/file_%04d.go", + modules[i%len(modules)], i)) + } + return paths +} + +func envValue(env []string, name string) string { + for _, e := range env { + if v, ok := strings.CutPrefix(e, name+"="); ok { + return v + } + } + return "\x00unset" +} + +func TestALongListOfChangedPathsGoesInAFileAndTheStringHoldsTheModules(t *testing.T) { // 1247/AC1 + paths := manyPaths() + if n := len(strings.Join(paths, ",")); n < 200<<10 { + t.Fatalf("the fixture is only %d bytes: not the real list's shape", n) + } + root := t.TempDir() + var said []string + env, err := changedEnvironment(root, paths, func(l string) { said = append(said, l) }) + if err != nil { + t.Fatal(err) + } + for _, e := range env { + if len(e) > 64<<10 { + t.Errorf("an environment string of %d bytes is handed to docker: %.60s", len(e), e) + } + } + raw, err := os.ReadFile(envValue(env, EnvChangedFile)) + if err != nil { + t.Fatal(err) + } + if got := strings.Split(strings.TrimSuffix(string(raw), "\n"), "\n"); strings.Join(got, "\n") != strings.Join(paths, "\n") { + t.Errorf("the file holds %d lines, not the %d paths in the order given", len(got), len(paths)) + } + want := "modules/mesh-delivery-forge-bridge/,modules/mesh-delivery-indexer/,modules/mesh-delivery-teller/,modules/mesh-delivery-view/" + if got := envValue(env, EnvChanged); got != want { + t.Errorf("%s is %q, want each module's directory once: %q", EnvChanged, got, want) + } + if len(said) != 1 || !strings.Contains(said[0], EnvChangedFile) { + t.Errorf("the check's output says %q, want one line naming %s", said, EnvChangedFile) + } +} + +func TestAShortListOfChangedPathsStaysAsItWasAndIsInTheFileToo(t *testing.T) { // 1247/AC2 + var said []string + env, err := changedEnvironment(t.TempDir(), []string{"a.go", "b.go"}, func(l string) { said = append(said, l) }) + if err != nil { + t.Fatal(err) + } + if got := envValue(env, EnvChanged); got != "a.go,b.go" { + t.Errorf("%s is %q", EnvChanged, got) + } + raw, err := os.ReadFile(envValue(env, EnvChangedFile)) + if err != nil || string(raw) != "a.go\nb.go\n" { + t.Errorf("the file holds %q, %v", raw, err) + } + if len(said) != 0 { + t.Errorf("a short list said %q", said) + } +} + +func TestTheGateReadsTheChangedPathsFromTheFile(t *testing.T) { // 1247/AC3 + var args []string + gateLayer(t.Context(), CheckSpec{}, t.TempDir(), t.TempDir(), "gate", filepath.Join(t.TempDir(), "verdict.json"), nil, + func(dir string, env []string, command ...string) []string { return command }, + func(a []string) error { args = a; return errors.New("stop here") }, &tail{}, "bus", "name", false, "", nil, + func(string, string, ...any) {}) + command := strings.Join(args, " ") + if !strings.Contains(command, `--changed-file "$MESH_CHECK_CHANGED_FILE"`) || strings.Contains(command, `--changed "`) { + t.Errorf("the gate's command does not read the paths from the file: %s", command) + } +} + +func TestACommandThatCannotStartFailsWithItsError(t *testing.T) { // 1247/AC4 + var out tail + layer := ownCheck(t.Context(), CheckSpec{Toolchain: "go-image"}, []ScriptPart{{Toolchain: "go", Script: CheckScript}}, t.TempDir(), + &out, func() bool { return false }, func(image, script string) *exec.Cmd { + return exec.CommandContext(t.Context(), "/nonexistent/docker", script) + }, func(string, string, ...any) {}) + if layer == nil || layer.Verdict != "fail" { + t.Fatalf("a command that cannot start answered %+v", layer) + } + if !strings.Contains(layer.Summary, CheckScript) || !strings.Contains(layer.Summary, "no such file or directory") || + strings.HasSuffix(strings.TrimSpace(layer.Summary), "failed:") { + t.Errorf("the summary does not say why: %q", layer.Summary) + } +} diff --git a/internal/builder/check_integration_test.go b/internal/builder/check_integration_test.go index 6e9f9992..3cbb6b2c 100644 --- a/internal/builder/check_integration_test.go +++ b/internal/builder/check_integration_test.go @@ -114,6 +114,36 @@ echo checked } } +func TestACheckOfAChangeTouchingMoreFilesThanOneArgumentHoldsRunsWhole(t *testing.T) { // 1247/AC5 + registry := checkEnvironment(t) + // 3000 paths of about 70 bytes: over 128 KiB comma-joined, which no environment string can carry. + var paths []string + for i := 0; i < 3000; i++ { + paths = append(paths, fmt.Sprintf("modules/m%d/vendor/github.com/nats-io/nats-server/v2/server/file_%05d.go", i%4, i)) + } + script := `set -e +test -s "$MESH_CHECK_CHANGED_FILE" +test "$(wc -l < "$MESH_CHECK_CHANGED_FILE")" = 3000 +head -n 1 "$MESH_CHECK_CHANGED_FILE" | grep -q '^modules/m0/vendor/.*file_00000.go$' +test "${#MESH_CHECK_CHANGED}" -lt 65536 +case "$MESH_CHECK_CHANGED" in modules/m0/,*) ;; *) echo "changed: $MESH_CHECK_CHANGED"; exit 1;; esac +echo checked +` + repo, head := aCheckedRepository(t, map[string]string{CheckScript: script}) + v, err := Check(t.Context(), Command, CheckSpec{ID: fmt.Sprintf("check-many-%d", time.Now().UnixNano()), Repository: repo, + Ref: head, Owner: "novox", Repo: "hq", Number: 8, Paths: paths, Toolchain: goToolchain}, t.TempDir(), registry, + GitCredential{}, nil) + if err != nil { + t.Fatal(err) + } + if v.Repo == nil || v.Repo.Verdict != "pass" || !strings.Contains(v.Report, "checked") { + t.Fatalf("the repository's check answered %+v\n%s", v.Repo, v.Report) + } + if !strings.Contains(v.Report, EnvChangedFile) { + t.Errorf("the report does not say the list went into a file:\n%s", v.Report) + } +} + func TestAFailingCheckFailsAndOneThatCannotRunIsNeverAPass(t *testing.T) { registry := checkEnvironment(t) repo, head := aCheckedRepository(t, map[string]string{CheckScript: "echo 'resource \"x.service\": refused'; exit 3\n"}) -- 2.54.0 From da5a5c9b67b2e0cef27dbe8c06f715075d74971b Mon Sep 17 00:00:00 2001 From: jochen Date: Mon, 19 Oct 2026 15:40:20 +0200 Subject: [PATCH 2/2] Give the gate a short changed list as an argument, as before, so an older judge still runs (issue 1247, review round 1) --- internal/builder/check.go | 31 +++++++++++++++++++++----- internal/builder/check_changed_test.go | 29 ++++++++++++++++++++---- 2 files changed, 51 insertions(+), 9 deletions(-) diff --git a/internal/builder/check.go b/internal/builder/check.go index 9d9547ae..f49cc160 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -263,9 +263,12 @@ const ( // EnvChangedFile names a file holding every changed path, one per line, in the order given: the whole // list however long, which one environment string cannot carry (novox/hq issue 1247). EnvChangedFile = "MESH_CHECK_CHANGED_FILE" - EnvVerdict = "MESH_CHECK_VERDICT" - EnvBeside = "MESH_CHECK_BESIDE" - EnvModules = "MESH_CHECK_MODULES" + // EnvChangedCut is 1 when MESH_CHECK_CHANGED was cut to directories, so the gate reads the file; unset + // otherwise, and the gate is given the list as before, which a judge older than --changed-file understands. + EnvChangedCut = "MESH_CHECK_CHANGED_CUT" + EnvVerdict = "MESH_CHECK_VERDICT" + EnvBeside = "MESH_CHECK_BESIDE" + EnvModules = "MESH_CHECK_MODULES" // EnvGroup is a delivery group's other heads, as JSON, for the gate (novox/hq ADR 0239); empty for a // pull request checked alone. EnvGroup = "MESH_CHECK_GROUP" @@ -1096,7 +1099,7 @@ func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFil // 2. Every machine composed with the change. if err := running(inToolchain(tree, append(env, EnvVerdict+"="+verdictFile), "sh", "-c", `"$MESH_GATE" merge-gate --facts "$MESH_FACTS" --store "$MESH_GATE_POSTGRES" `+ - `--repository "$MESH_CHECK_REPOSITORY" --tree . --changed-file "$MESH_CHECK_CHANGED_FILE" `+ + `--repository "$MESH_CHECK_REPOSITORY" --tree . `+gateChangedFlag(env)+` `+ `${MESH_CHECK_GROUP:+--group "$MESH_CHECK_GROUP"} --json > "$MESH_CHECK_VERDICT"`)); err != nil { if ctx.Err() != nil { return "error", "the check was ended during the merge gate" @@ -1926,6 +1929,7 @@ func changedEnvironment(root string, paths []string, say func(string)) ([]string return nil, err } joined := strings.Join(paths, ",") + cut := false if len(joined) >= changedEnvLimit { var short []string seen := map[string]bool{} @@ -1941,8 +1945,25 @@ func changedEnvironment(root string, paths []string, say func(string)) ([]string } } joined = strings.Join(short, ",") + cut = true say(fmt.Sprintf("--- %d changed paths are too long for one environment string: %s holds all of them, one per "+ "line, and %s only each path's first two directories", len(paths), EnvChangedFile, EnvChanged)) } - return []string{EnvChanged + "=" + joined, EnvChangedFile + "=" + file}, nil + env := []string{EnvChanged + "=" + joined, EnvChangedFile + "=" + file} + if cut { + env = append(env, EnvChangedCut+"=1") + } + return env, nil +} + +// gateChangedFlag is how the gate is given the changed paths: as the argument it always was while the list was not +// cut, so a judge from before --changed-file still runs; from the file only when it was cut, which that judge +// could never have started anyway. +func gateChangedFlag(env []string) string { + for _, e := range env { + if e == EnvChangedCut+"=1" { + return `--changed-file "$MESH_CHECK_CHANGED_FILE"` + } + } + return `--changed "$MESH_CHECK_CHANGED"` } diff --git a/internal/builder/check_changed_test.go b/internal/builder/check_changed_test.go index e85f7e62..048d0851 100644 --- a/internal/builder/check_changed_test.go +++ b/internal/builder/check_changed_test.go @@ -81,15 +81,36 @@ func TestAShortListOfChangedPathsStaysAsItWasAndIsInTheFileToo(t *testing.T) { / } } -func TestTheGateReadsTheChangedPathsFromTheFile(t *testing.T) { // 1247/AC3 +// theGatesCommand is the command gateLayer hands the toolchain for the merge gate, given the environment of the check. +func theGatesCommand(t *testing.T, env []string) string { + t.Helper() var args []string - gateLayer(t.Context(), CheckSpec{}, t.TempDir(), t.TempDir(), "gate", filepath.Join(t.TempDir(), "verdict.json"), nil, + gateLayer(t.Context(), CheckSpec{}, t.TempDir(), t.TempDir(), "gate", filepath.Join(t.TempDir(), "verdict.json"), env, func(dir string, env []string, command ...string) []string { return command }, func(a []string) error { args = a; return errors.New("stop here") }, &tail{}, "bus", "name", false, "", nil, func(string, string, ...any) {}) - command := strings.Join(args, " ") + return strings.Join(args, " ") +} + +func TestTheGateIsGivenAShortListAsBeforeSoAnOlderJudgeStillRuns(t *testing.T) { // 1247/AC3 + env, err := changedEnvironment(t.TempDir(), []string{"a.go", "b.go"}, func(string) {}) + if err != nil { + t.Fatal(err) + } + command := theGatesCommand(t, env) + if !strings.Contains(command, `--changed "$MESH_CHECK_CHANGED"`) || strings.Contains(command, "--changed-file") { + t.Errorf("a short list is not given to the gate as --changed alone: %s", command) + } +} + +func TestTheGateReadsACutListFromTheFile(t *testing.T) { // 1247/AC3 + env, err := changedEnvironment(t.TempDir(), manyPaths(), func(string) {}) + if err != nil { + t.Fatal(err) + } + command := theGatesCommand(t, env) if !strings.Contains(command, `--changed-file "$MESH_CHECK_CHANGED_FILE"`) || strings.Contains(command, `--changed "`) { - t.Errorf("the gate's command does not read the paths from the file: %s", command) + t.Errorf("a cut list is not given to the gate through the file alone: %s", command) } } -- 2.54.0