diff --git a/internal/builder/check.go b/internal/builder/check.go index fc368569..0e10a85f 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -335,16 +335,9 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry if err != nil { return CheckVerdict{}, err } - if spec.JudgeCache != "" { + if replay.base != "" { defer os.RemoveAll(replay.base) } - defer func() { - if replay.volumeMade { - removing, done := context.WithTimeout(context.Background(), time.Minute) - defer done() - _, _ = run(removing, root, "docker", "volume", "rm", "--force", replay.volume) - } - }() // cloneAt clones a repository and checks out ref — or, when the repository has no branch ref and orElse is said, // orElse (novox/hq ADR 0358 §3); readAt keeps which, by directory. readAt := map[string]string{} @@ -466,7 +459,7 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry if dir == "mesh-lab" { // The copy the replays run from, made before anything of the change runs, outside every check container // (novox/hq issue 709) — and whether it holds replays, read now, so nothing written later decides it. - if err := replay.copyLab(ctx, run, spec, root, workspace, filepath.Join(root, dir)); err != nil { + if err := replay.copyLab(ctx, run, filepath.Join(root, dir)); err != nil { return CheckVerdict{}, fmt.Errorf("mesh-lab could not be copied for its replays: %w", err) } } @@ -644,13 +637,8 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry args := in(spec.Toolchain, filepath.Join(replay.lab, "replays"), append(ReplayEnv(replay.home, replay.goCache, replay.modCache), env...), command...) // The socket goes to the replays alone, never to the change's own code. - mounts := []string{"--volume", "/var/run/docker.sock:/var/run/docker.sock"} - if replay.volume != "" { - mounts = append(mounts, "--volume", replay.volume+":"+replay.base) - } else { - mounts = append(mounts, "--volume", replay.base+":"+replay.base, "--volume", replay.goCache+":"+replay.goCache, - "--volume", replay.modCache+":"+replay.modCache) - } + mounts := []string{"--volume", "/var/run/docker.sock:/var/run/docker.sock", "--volume", replay.base + ":" + + replay.base, "--volume", replay.goCache + ":" + replay.goCache, "--volume", replay.modCache + ":" + replay.modCache} return append(append([]string{args[0]}, mounts...), args[1:]...) } var out tail @@ -706,7 +694,7 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry say("check", "the gate: %d module(s) of the graph touched — %s", len(modules), strings.Join(modules, ", ")) fmt.Fprintf(&out, "--- the gate: %s\n", strings.Join(modules, ", ")) v.Gate.Verdict, v.Gate.Summary = gateLayer(ctx, spec, tree, root, gate, verdictFile, env, inToolchain, - running, &out, bus, name, replay.hasReplays, inReplays, say) + running, &out, bus, name, replay.hasReplays, replay.refused(), inReplays, say) } if timedOut() { v.Gate.Verdict, v.Gate.Summary = "error", fmt.Sprintf("the check ran past %s and was ended", CheckTimeout) @@ -984,7 +972,7 @@ const SomeManifestsEnv = "MESH_MODULE_CHECK_SOME=1" // change, and the replays of what the mesh runs. It answers the gate's verdict and summary. func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFile string, env []string, inToolchain func(string, []string, ...string) []string, running func([]string) error, out *tail, bus, - name string, hasReplays bool, inReplays func([]string, ...string) []string, + name string, hasReplays bool, replaysRefused string, inReplays func([]string, ...string) []string, say func(step, format string, args ...any)) (string, string) { // 1. The manifests the change touches, as the judge reads them: a manifest it cannot read, or one with a // problem the change brings, fails here. **What was already so on the base branch is said and fails @@ -1077,6 +1065,12 @@ func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFil // 3. **The replays of what the mesh runs** (to-be 45 §9, M9): mesh-lab's, from its main — reviewed code, // so given the container runtime the resolver replay raises containers with — against the bus of the // release the mesh runs and the change's own catalogue when the change is to the catalogue. + if ctx.Err() == nil && hasReplays && replaysRefused != "" { + // **Never from the workspace** (novox/hq issue 709): without the judge cache there is no place outside it. + say("check", "%s", replaysRefused) + fmt.Fprintf(out, "--- the replays: %s\n", replaysRefused) + return "error", replaysRefused + } if ctx.Err() == nil { if hasReplays { catalogue := filepath.Join(root, "mesh-catalog") @@ -1093,7 +1087,15 @@ func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFil if ctx.Err() != nil { return "error", "the check was ended during the replays" } - return "fail", "a replay of a core incident fails with this change: " + lastLine(out.String()) + if replaysNotStarted(err) { + // The container runtime could not start them (a mount, an image): the check's fault, never the change's. + said := "the replays could not be started, so the change is not judged by them: " + lastLine(out.String()) + say("check", "%s", said) + return "error", said + } + said := "a replay of a core incident fails with this change: " + lastLine(out.String()) + say("check", "%s", said) + return "fail", said } } } @@ -1247,7 +1249,7 @@ func judgeBuildDirs(cache, workspace, root, id string) (string, string, error) { "no judge is built where a pull request could reach it", cache, workspace) } if !safeName.MatchString(id) { - id = "check" + return "", "", fmt.Errorf("the check's id %q cannot name its place in the judge cache", id) } home, goCache := filepath.Join(cache, "homes", id), filepath.Join(cache, "go-cache") if err := os.RemoveAll(home); err != nil { @@ -1262,38 +1264,42 @@ func judgeBuildDirs(cache, workspace, root, id string) (string, string, error) { } // replayPlace is where mesh-lab's replays run from and with (novox/hq issue 709), never anything a pull request can -// write: base, the place made for this check; lab, the copy of mesh-lab they run; home, their HOME and configuration; -// and their Go build and module caches. With a judge cache, base is a directory under its homes/ (the paths the same -// on the host and in the build agent) and the caches are the judge cache's. Without one, base is where a container -// volume of this check's own is mounted (volume), holding the copy, the home and throwaway caches, removed after the -// check: the build agent declares no other host path outside the workspace. hasReplays says whether mesh-lab held -// replays when it was copied, before anything of the change ran. +// write: base, the directory made for this check under the judge cache's homes/ (the same path on the host and in the +// build agent), removed after it; lab, the copy of mesh-lab they run; home, their HOME and configuration; and the judge +// cache's Go build and module caches. Empty without a judge cache: the replays are then refused. hasReplays says whether +// mesh-lab held replays when it was cloned, before anything of the change ran. type replayPlace struct { base, lab, home, goCache, modCache string - volume string - volumeMade, hasReplays bool + hasReplays bool } -// ReplayMount is where a check's own replay volume is mounted in the replays' container when there is no judge cache. -var ReplayMount = "/mesh-replays" +// replaysNeedTheJudgeCache is the gate's error when mesh-lab holds replays and the build agent has no judge cache. +const replaysNeedTheJudgeCache = "the replays need the build agent's judge cache (mesh-catalog#297); refused rather " + + "than run from the workspace" + +// refused is why the replays cannot run here, empty when they can. +func (p replayPlace) refused() string { + if p.base == "" { + return replaysNeedTheJudgeCache + } + return "" +} // replayDirs makes the replays' place (issue 709, ADR 0358 §6). The replays' build is as trusted as a judge's: it // compiles mesh-lab's trunk, reviewed code, and nothing of the pull request, which hands them only its catalogue, as -// data. Never the workspace: the gate runs before the replays, a pull request's own code among its containers. +// data. Never the workspace: the gate runs before the replays, a pull request's own code among its containers. Without +// a judge cache there is no place: the build agent declares no other host path, and the replays are refused. func replayDirs(cache, workspace, root, id string) (replayPlace, error) { - if !safeName.MatchString(id) { - id = "check" - } if cache == "" { - base := ReplayMount - return replayPlace{base: base, lab: filepath.Join(base, "mesh-lab"), home: filepath.Join(base, "home"), - goCache: filepath.Join(base, "go-cache"), modCache: filepath.Join(base, "go-modules"), - volume: "mesh-check-" + strings.ToLower(id) + "-replays"}, nil + return replayPlace{}, nil } if withinDir(workspace, cache) { return replayPlace{}, fmt.Errorf("the judge cache %s is inside the workspace %s, which a check's containers "+ "mount: no replay is run where a pull request could reach it", cache, workspace) } + if !safeName.MatchString(id) { + return replayPlace{}, fmt.Errorf("the check's id %q cannot name its place in the judge cache", id) + } base := filepath.Join(cache, "homes", id+"-replays") p := replayPlace{base: base, lab: filepath.Join(base, "mesh-lab"), home: filepath.Join(base, "home"), goCache: filepath.Join(cache, "go-cache"), modCache: filepath.Join(cache, "go-modules")} @@ -1308,34 +1314,26 @@ func replayDirs(cache, workspace, root, id string) (replayPlace, error) { return p, nil } -// copyLab copies mesh-lab's checkout, freshly cloned and before anything of the change runs, to where the replays -// run it. With a judge cache, over git's own transport into its directory. Without one, into a volume of this check's -// own, made now: a container reads the checkout from the workspace, mounted read-only, and copies it with a fresh home -// and empty caches, owned by the user the check's containers run as. -func (p *replayPlace) copyLab(ctx context.Context, run Runner, spec CheckSpec, root, workspace, from string) error { +// copyLab reads whether mesh-lab's checkout, freshly cloned and before anything of the change runs, holds replays, and +// copies it over git's own transport to where the replays run it — when there is such a place. +func (p *replayPlace) copyLab(ctx context.Context, run Runner, from string) error { _, err := os.Stat(filepath.Join(from, "replays")) p.hasReplays = err == nil - if p.volume == "" { - return copyCheckout(ctx, run, from, p.lab) + if p.lab == "" { + return nil } - if spec.Toolchain == "" { - return errors.New("the mesh holds no Go toolchain to copy mesh-lab with") + return copyCheckout(ctx, run, from, p.lab) +} + +// replaysNotStarted says the container runtime could not start the replays' container — its own exit status 125, or +// 126 and 127 for a command it could not run — rather than that a replay failed. +func replaysNotStarted(err error) bool { + var exit *exec.ExitError + if !errors.As(err, &exit) { + return true } - user := spec.User - if user == "" { - user = fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()) - } - _, _ = run(ctx, root, "docker", "volume", "rm", "--force", p.volume) // one an earlier delivery left - if _, err := run(ctx, root, "docker", "volume", "create", "--label", BuildLabel+"="+spec.ID, p.volume); err != nil { - return err - } - p.volumeMade = true - script := fmt.Sprintf("mkdir -p %s %s %s %s && cp -a %s/. %s/ && chown -R %s %s", - p.lab, p.home, p.goCache, p.modCache, from, p.lab, user, p.base) - _, err = run(ctx, root, "docker", LabelledArgs("docker", []string{"run", "--rm", "--network", "none", - "--volume", p.volume + ":" + p.base, "--volume", workspace + ":" + workspace + ":ro", "--workdir", "/", - spec.Toolchain, "sh", "-c", script}, spec.ID)...) - return err + code := exit.ExitCode() + return code == 125 || code == 126 || code == 127 } // ReplayEnv is the environment mesh-lab's replays run in (novox/hq issue 709): home as HOME and configuration, the Go diff --git a/internal/builder/release_branch_test.go b/internal/builder/release_branch_test.go index 1e560d36..6547ac0e 100644 --- a/internal/builder/release_branch_test.go +++ b/internal/builder/release_branch_test.go @@ -131,7 +131,6 @@ case "$cmd" in ps|rm|exec) exit 0 ;; volume) if [ -n "$VOLUMES" ]; then echo "$*" >> "$VOLUMES"; fi - if [ "$1" = rm ] && [ -n "$REPLAY_MOUNT" ]; then rm -rf "$REPLAY_MOUNT"; fi exit 0 ;; port) echo "` + bus.Addr().String() + `"; exit 0 ;; run) @@ -151,6 +150,9 @@ run) shift if [ "$1" = go ]; then echo "GOCACHE=$GOCACHE GOENV=$GOENV $*" >> "` + built + `"; fi if [ "$1" = go ] && [ "$2" = build ] && [ -n "$JUDGE_HOMES" ]; then echo "$HOME" >> "$JUDGE_HOMES"; fi + if [ "$1" = go ] && [ "$2" = test ] && [ -n "$REPLAYS_CANNOT_START" ]; then + echo "docker: Error response from daemon: invalid mount config for type bind" >&2; exit 125 + fi if [ "$1" = go ] && [ "$2" = test ] && [ -n "$REPLAYS" ]; then echo "HOME=$HOME GOCACHE=$GOCACHE GOMODCACHE=$GOMODCACHE GOENV=$GOENV GOFLAGS=$GOFLAGS DIR=$workdir" >> "$REPLAYS" fi @@ -744,13 +746,13 @@ func aSelfJudgingControllerThatWritesIntoTheReplays(t *testing.T, marker string) return aPullRequestInto(t, files, "main", map[string]string{"cmd/x.txt": "x"}) } -// **Without a judge cache, the replays still never read what a pull request wrote** (issue 709, review 12303): the -// copy of mesh-lab, its home and throwaway caches are in a container volume of the check's own, made before anything -// of the change runs and removed after — never the workspace. With a judge cache, under it. In both: a self-judging -// controller pull request writing a test into the replays beside the check, a planted GOFLAGS hook and a poisoned -// cache entry in the workspace neither run nor are read. -func TestTheReplaysNeverReadWhatAPullRequestWroteWithOrWithoutAJudgeCache(t *testing.T) { - for _, withCache := range []bool{false, true} { +// **The replays never read what a pull request wrote** (issue 709, reviews 12303 and 12308): with a judge cache they +// run from a copy under it, and a self-judging controller pull request writing a test into the replays beside the +// check, a planted GOFLAGS hook and a poisoned cache entry in the workspace neither run nor are read. Without one the +// replays are refused — the gate an error that says why, never a pass, never the workspace — they never start, and +// nothing is made on the host. +func TestTheReplaysNeverReadWhatAPullRequestWroteAndNeedAJudgeCache(t *testing.T) { + for _, withCache := range []bool{true, false} { t.Run(fmt.Sprintf("judge cache %v", withCache), func(t *testing.T) { aRuntimeThatBuildsTheJudge(t) replays, volumes := filepath.Join(t.TempDir(), "replays"), filepath.Join(t.TempDir(), "volumes") @@ -758,11 +760,6 @@ func TestTheReplaysNeverReadWhatAPullRequestWroteWithOrWithoutAJudgeCache(t *tes t.Setenv("VOLUMES", volumes) tmp := t.TempDir() t.Setenv("TMPDIR", tmp) - mount := filepath.Join(t.TempDir(), "mesh-replays") - t.Setenv("REPLAY_MOUNT", mount) - was := ReplayMount - ReplayMount = mount - t.Cleanup(func() { ReplayMount = was }) registry := aFactsRegistry(t) workspace, ran := aPlantedWorkspace(t) if err := os.MkdirAll(filepath.Join(workspace, "go-cache"), 0o755); err != nil { @@ -786,9 +783,6 @@ func TestTheReplaysNeverReadWhatAPullRequestWroteWithOrWithoutAJudgeCache(t *tes if err != nil { t.Fatal(err) } - if v.Gate == nil || v.Gate.Verdict != "pass" || !strings.Contains(v.Report, "--- the replays") { - t.Fatalf("the replays did not run and pass: %+v\n%s", v.Gate, v.Report) - } if _, err := os.Stat(evilRan + ".wrote"); err != nil { t.Fatalf("the self-judging pull request did not write into the replays beside it, so this proves nothing: %v", err) } @@ -800,27 +794,70 @@ func TestTheReplaysNeverReadWhatAPullRequestWroteWithOrWithoutAJudgeCache(t *tes } body, _ := os.ReadFile(replays) said := strings.TrimSpace(string(body)) - if said == "" || strings.Contains(said, workspace) || !strings.Contains(said, "GOENV=off ") { - t.Fatalf("the replays ran with something of the workspace, or not at all: %q", said) - } - base := mount - if withCache { - base = filepath.Join(cache, "homes", "check-709-self-replays") - } - if !strings.Contains(said, "DIR="+filepath.Join(base, "mesh-lab", "replays")) { - t.Fatalf("the replays did not run from the copy under %s: %s", base, said) - } - vols, _ := os.ReadFile(volumes) - if !withCache && (!strings.Contains(string(vols), "create --label "+BuildLabel+"=check-709-self") || - !strings.Contains(string(vols), "rm --force mesh-check-check-709-self-replays")) { - t.Fatalf("the check's own replay volume was not made and removed: %q", vols) - } - if _, err := os.Stat(base); err == nil { - t.Fatalf("the replays' place %s was left after the check", base) + if vols, _ := os.ReadFile(volumes); len(vols) != 0 { + t.Fatalf("the check made a container volume: %q", vols) } if left, _ := os.ReadDir(tmp); len(left) != 0 { t.Fatalf("a check left %d entr(ies) in the temporary directory", len(left)) } + if !withCache { + if v.Gate == nil || v.Gate.Verdict != "error" || !strings.Contains(v.Gate.Summary, replaysNeedTheJudgeCache) || + v.Verdict != "error" { + t.Fatalf("without a judge cache the gate said %+v, not an error saying the replays need it", v.Gate) + } + if said != "" { + t.Fatalf("without a judge cache the replays started: %s", said) + } + return + } + if v.Gate == nil || v.Gate.Verdict != "pass" || !strings.Contains(v.Report, "--- the replays") { + t.Fatalf("the replays did not run and pass: %+v\n%s", v.Gate, v.Report) + } + base := filepath.Join(cache, "homes", "check-709-self-replays") + if said == "" || strings.Contains(said, workspace) || !strings.Contains(said, "GOENV=off ") || + !strings.Contains(said, "DIR="+filepath.Join(base, "mesh-lab", "replays")) { + t.Fatalf("the replays did not run from the copy under %s, apart from the workspace: %q", base, said) + } + if _, err := os.Stat(base); err == nil { + t.Fatalf("the replays' place %s was left after the check", base) + } }) } } + +// **Replays the container runtime could not start are the check's error, never the change's failure**, said in the +// build's log (review 12308); and a check whose id cannot name a place in the judge cache is refused, never given a +// shared one. +func TestReplaysThatCannotStartAreAnErrorAndAnUnsafeIDIsRefused(t *testing.T) { + aRuntimeThatBuildsTheJudge(t) + t.Setenv("REPLAYS_CANNOT_START", "1") + registry := aFactsRegistry(t) + controller, controllerMain, _ := aRepositoryWithARelease(t, aJudgeOf("the controller the mesh runs"), nil) + lab, labMain := aLabWithReplays(t) + catalogue, head := aPullRequestInto(t, map[string]string{"modules/gitea/module.json": `{"module":"gitea"}`, + CheckScript: "true\n"}, "main", map[string]string{"modules/gitea/index.ts": "x"}) + spec := CheckSpec{ID: "check-709-start", Repository: catalogue, Ref: head, Owner: "novox", Repo: "mesh-catalog", + Number: 1, Base: "main", JudgeCache: t.TempDir(), Paths: []string{"modules/gitea/index.ts"}, + Modules: []string{"gitea"}, Toolchain: "golang", Beside: map[string]Beside{ + "mesh-controller": {Repository: controller, Ref: controllerMain}, "mesh-lab": {Repository: lab, Ref: labMain}}} + var logged []string + v, err := Check(t.Context(), Command, spec, t.TempDir(), registry, GitCredential{}, func(step, message string) { + logged = append(logged, message) + }) + if err != nil { + t.Fatal(err) + } + if v.Gate == nil || v.Gate.Verdict != "error" || !strings.Contains(v.Gate.Summary, "could not be started") || + !strings.Contains(v.Gate.Summary, "invalid mount config") { + t.Fatalf("replays that could not start were said %+v, not an error naming why", v.Gate) + } + if !strings.Contains(strings.Join(logged, "\n"), "the replays could not be started") { + t.Fatalf("the build's log does not say what failed:\n%s", strings.Join(logged, "\n")) + } + + spec.ID = "Check 1/../../x" + if _, err := Check(t.Context(), Command, spec, t.TempDir(), registry, GitCredential{}, nil); err == nil || + !strings.Contains(err.Error(), "cannot name its place in the judge cache") { + t.Fatalf("a check with an unsafe id was taken: %v", err) + } +}