Download the go.mod directories side by side and verify a go.mod and go.sum pair once, so a fetch spends no minutes in serial go mod steps (issue 890, ADR 0536 s2)

2026-10-23 14:08:20 +02:00
parent 24cbc8961e
commit 3ff6a8b502
4 changed files with 184 additions and 3 deletions
@@ -6,12 +6,15 @@ package main

 import (
 	"context"
+	"crypto/sha256"
+	"encoding/hex"
 	"fmt"
 	"os"
 	"path/filepath"
 	"regexp"
 	"sort"
 	"strings"
+	"sync"
 	"time"

 	"git.novox.be/novox/mesh-sdk/go/buildstep"
@@ -143,11 +146,27 @@ func (m *Module) fetchInto(ctx context.Context, req buildstep.FetchRequest, id s
 	if base != "" && res.Merge == "" {
 		return res, nil // a conflict: the tree is at the trunk, nothing of the head is built or tested
 	}
-	for _, d := range dirs {
+	// The directories download side by side, up to the bound (ADR 0536 §2); the first failure in directory order is the
+	// step's. Resolving a pin places a repository beside the workspace, which two directories may share: one at a time.
+	var pins sync.Mutex
+	errs := make([]error, len(dirs))
+	slots := make(chan struct{}, m.parallel())
+	var wg sync.WaitGroup
+	for i, d := range dirs {
 		if _, err := os.Stat(filepath.Join(tree, d, "vendor", "modules.txt")); err == nil {
 			continue // a module that vendors resolves nothing
 		}
-		if err := m.downloadModules(ctx, root, filepath.Join(tree, d), d); err != nil {
+		wg.Add(1)
+		slots <- struct{}{}
+		go func() {
+			defer wg.Done()
+			defer func() { <-slots }()
+			errs[i] = m.downloadModules(ctx, root, filepath.Join(tree, d), d, &pins)
+		}()
+	}
+	wg.Wait()
+	for _, err := range errs {
+		if err != nil {
 			return nil, err
 		}
 	}
@@ -157,13 +176,15 @@ func (m *Module) fetchInto(ctx context.Context, req buildstep.FetchRequest, id s
 // downloadModules downloads and verifies the Go modules of the directory dir (d in the tree). A pin of a forge module
 // that Go cannot fetch is looked for among the forge's pull request heads and handed to Go by a modfile (pins.go); a
 // failure that names no such pin is the first failure.
-func (m *Module) downloadModules(ctx context.Context, root, dir, d string) error {
+func (m *Module) downloadModules(ctx context.Context, root, dir, d string, pins *sync.Mutex) error {
 	for {
 		failure := m.downloadAndVerify(ctx, dir, d)
 		if failure == nil {
 			return nil
 		}
+		pins.Lock()
 		progress, err := m.resolvePins(ctx, root, dir, failure.Error())
+		pins.Unlock()
 		if err != nil {
 			return err
 		}
@@ -173,8 +194,41 @@ func (m *Module) downloadModules(ctx context.Context, root, dir, d string) error
 	}
 }

+// verifyLocks hold one mutex per digest of a go.mod and go.sum, so two directories with the same pair verify once.
+var verifyLocks sync.Map
+
+// verifiedMark is the file naming a pass of go mod verify for a go.mod and go.sum pair, "" when a pass is not kept:
+// the module cache is only read by steps that are shut off with it read-only (ADR 0448 §3), so the pass holds only then,
+// and a directory with a pin modfile verifies another file.
+func (m *Module) verifiedMark(dir string) string {
+	if m.Cache == "" || m.Bubblewrap == nil {
+		return ""
+	}
+	if _, ok := m.pinModfile(dir); ok {
+		return ""
+	}
+	h := sha256.New()
+	for _, name := range []string{"go.mod", "go.sum"} {
+		raw, _ := os.ReadFile(filepath.Join(dir, name))
+		fmt.Fprintf(h, "%s %d\n", name, len(raw))
+		h.Write(raw)
+	}
+	return filepath.Join(m.Cache, "verified", hex.EncodeToString(h.Sum(nil)))
+}
+
 func (m *Module) downloadAndVerify(ctx context.Context, dir, d string) error {
 	for _, verb := range []string{"download", "verify"} {
+		mark := ""
+		if verb == "verify" {
+			if mark = m.verifiedMark(dir); mark != "" {
+				mu, _ := verifyLocks.LoadOrStore(mark, &sync.Mutex{})
+				mu.(*sync.Mutex).Lock()
+				defer mu.(*sync.Mutex).Unlock()
+				if _, err := os.Stat(mark); err == nil {
+					continue // this go.mod and go.sum passed before
+				}
+			}
+		}
 		cmd, env := []string{"-C", dir, "mod", verb}, m.goEnvFor(dir)
 		if modfile, ok := m.pinModfile(dir); ok && verb == "verify" {
 			// go mod verify takes no -modfile: it is run on the modfile itself, which names the placed repositories.
@@ -184,6 +238,12 @@ func (m *Module) downloadAndVerify(ctx context.Context, dir, d string) error {
 		if r.Status != 0 || r.Err != "" {
 			return fmt.Errorf("go mod %s in %s: %s", verb, d, causeLine(r.Err+"\n"+r.Stdout+"\n"+r.Stderr))
 		}
+		if mark != "" {
+			// Said after the pass; a mark that cannot be written only means the next fetch verifies again.
+			if os.MkdirAll(filepath.Dir(mark), 0o700) == nil {
+				os.WriteFile(mark, nil, 0o600)
+			}
+		}
 	}
 	return nil
 }
@@ -30,6 +30,7 @@ type fakeStep struct {
 	gomods  string
 	gate    chan struct{} // when set, the first git command waits for it
 	goRule  func(line string) forgekey.Ran
+	files   map[string]string                      // files (relative to the tree) written at checkout: go.mod and go.sum of the fixture
 	vendors []string                               // directories (relative to the tree) that get a vendor/modules.txt at checkout
 	rule    func(line string) (forgekey.Ran, bool) // answers a command before the defaults do
 }
@@ -55,6 +56,10 @@ func (f *fakeStep) run(ctx context.Context, env []string, name string, args ...s
 	case name == "git" && strings.Contains(line, " checkout "):
 		for i, a := range args {
 			if a == "-C" {
+				for name, text := range f.files {
+					os.MkdirAll(filepath.Dir(filepath.Join(args[i+1], name)), 0o755)
+					os.WriteFile(filepath.Join(args[i+1], name), []byte(text), 0o644)
+				}
 				for _, v := range f.vendors {
 					os.MkdirAll(filepath.Join(args[i+1], v, "vendor"), 0o755)
 					os.WriteFile(filepath.Join(args[i+1], v, "vendor", "modules.txt"), nil, 0o644)
@@ -6,6 +6,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 	"testing"

 	"git.novox.be/novox/mesh-sdk/go/delivery"
@@ -28,6 +29,7 @@ type pinHarness struct {
 	pulls   string // what git ls-remote answers
 	onTrunk bool   // the pin is fetchable as it is: go mod download never says unknown revision
 	cmds    []cmdRecord
+	mu      sync.Mutex
 }

 func newPinHarness(t *testing.T, gomod string, vendor bool, pulls string) (*pinHarness, *keptStates) {
@@ -36,7 +38,9 @@ func newPinHarness(t *testing.T, gomod string, vendor bool, pulls string) (*pinH
 	h := &pinHarness{t: t, m: m, f: f, pulls: pulls}
 	m.Run = func(ctx context.Context, env []string, name string, args ...string) forgekey.Ran {
 		line := name + " " + strings.Join(args, " ")
+		h.mu.Lock()
 		h.cmds = append(h.cmds, cmdRecord{env: env, line: line})
+		h.mu.Unlock()
 		switch {
 		case name == "git" && strings.Contains(line, " checkout ") && !strings.Contains(line, "pinned"):
 			for i, a := range args {
@@ -0,0 +1,112 @@
+package main
+
+import (
+	"context"
+	"strings"
+	"sync"
+	"testing"
+	"time"
+
+	"git.novox.be/novox/mesh-sdk/go/forgekey"
+)
+
+func shutOffModule(t *testing.T, f *fakeStep) (*Module, *keptStates) {
+	t.Helper()
+	m, states := fetchModule(t, f)
+	// A module with bubblewrap runs its steps shut off, with the module cache read-only: a pass of go mod verify holds.
+	m.Bubblewrap = func(context.Context) (string, error) { return "bubblewrap 0.13.0", nil }
+	return m, states
+}
+
+func countVerifies(f *fakeStep) int {
+	n := 0
+	for _, l := range f.lines() {
+		if strings.HasPrefix(l, "go -C ") && strings.HasSuffix(l, " mod verify") {
+			n++
+		}
+	}
+	return n
+}
+
+// Issue 890/AC5 (ADR 0536 §2): two directories with the same go.mod and go.sum verify once, and a later fetch with the
+// same pair verifies no time; a different go.mod verifies again.
+func TestGoModVerifyRunsOncePerGoModAndGoSumDigest(t *testing.T) {
+	same := map[string]string{"a/go.mod": "module a\n", "a/go.sum": "x\n", "b/go.mod": "module a\n", "b/go.sum": "x\n"}
+	f := &fakeStep{gomods: "a/go.mod\nb/go.mod\n", files: same}
+	m, states := shutOffModule(t, f)
+	fetchOnce(t, m, states, fetchArgs)
+	if n := countVerifies(f); n != 1 {
+		t.Errorf("two directories with one go.mod and go.sum verified %d times, want 1", n)
+	}
+	fetchOnce(t, m, states, fetchArgs)
+	if n := countVerifies(f); n != 1 {
+		t.Errorf("a later fetch with the same pair verified again (%d in all)", n)
+	}
+
+	g := &fakeStep{gomods: "a/go.mod\nb/go.mod\n", files: map[string]string{"a/go.mod": "module a\n", "a/go.sum": "x\n", "b/go.mod": "module b\n", "b/go.sum": "x\n"}}
+	m, states = shutOffModule(t, g)
+	fetchOnce(t, m, states, fetchArgs)
+	if n := countVerifies(g); n != 2 {
+		t.Errorf("two different go.mod verified %d times, want 2", n)
+	}
+}
+
+// Without bubblewrap the module cache is not read-only to a step: a pass is never reused.
+func TestAPassOfVerifyIsNotReusedWhenStepsAreNotShutOff(t *testing.T) {
+	f := &fakeStep{gomods: "a/go.mod\nb/go.mod\n", files: map[string]string{"a/go.mod": "m\n", "b/go.mod": "m\n"}}
+	m, states := fetchModule(t, f) // no Bubblewrap
+	fetchOnce(t, m, states, fetchArgs)
+	fetchOnce(t, m, states, fetchArgs)
+	if n := countVerifies(f); n != 4 {
+		t.Errorf("verified %d times, want every directory every fetch (4)", n)
+	}
+}
+
+// A verify that fails leaves no mark: the next fetch verifies again.
+func TestAFailedVerifyLeavesNoMark(t *testing.T) {
+	fail := true
+	f := &fakeStep{gomods: "go.mod\n", files: map[string]string{"go.mod": "m\n"}}
+	f.goRule = func(line string) forgekey.Ran {
+		if strings.HasSuffix(line, " mod verify") && fail {
+			return forgekey.Ran{Status: 1, Stdout: "x v1: dir has been modified\n"}
+		}
+		return forgekey.Ran{}
+	}
+	m, states := shutOffModule(t, f)
+	fetchTool(t, m)(fetchArgs)
+	states.waitEnd(t)
+	fail = false
+	fetchOnce(t, m, states, fetchArgs)
+	if n := countVerifies(f); n != 2 {
+		t.Errorf("verified %d times, want 2 (the failed one and the retry)", n)
+	}
+}
+
+// Issue 890 (ADR 0536 §2): the downloads of the directories run side by side, never more at once than the bound.
+func TestDownloadsRunSideBySideUpToTheBound(t *testing.T) {
+	var mu sync.Mutex
+	running, most := 0, 0
+	f := &fakeStep{gomods: "a/go.mod\nb/go.mod\nc/go.mod\nd/go.mod\n", files: map[string]string{
+		"a/go.mod": "a\n", "b/go.mod": "b\n", "c/go.mod": "c\n", "d/go.mod": "d\n"}}
+	f.goRule = func(line string) forgekey.Ran {
+		if strings.HasSuffix(line, " mod download") {
+			mu.Lock()
+			running++
+			if running > most {
+				most = running
+			}
+			mu.Unlock()
+			time.Sleep(40 * time.Millisecond)
+			mu.Lock()
+			running--
+			mu.Unlock()
+		}
+		return forgekey.Ran{}
+	}
+	m, states := fetchModule(t, f)
+	m.Parallel = 2
+	fetchOnce(t, m, states, fetchArgs)
+	if most != 2 {
+		t.Errorf("at most %d downloads ran at once, want 2", most)
+	}
+}