From 7c29c06f0a39b259411c47bd70200fedd0283187 Mon Sep 17 00:00:00 2001 From: Benjamin Smidt Date: Wed, 24 Jun 2026 15:52:31 -0700 Subject: [PATCH 1/2] docs(covgate): plan pre-warm build cache before parallel coverage runs Co-Authored-By: Claude Opus 4.8 (1M context) --- .../20260624-covgate-prewarm-build-cache.md | 382 ++++++++++++++++++ 1 file changed, 382 insertions(+) create mode 100644 plans/active/20260624-covgate-prewarm-build-cache.md diff --git a/plans/active/20260624-covgate-prewarm-build-cache.md b/plans/active/20260624-covgate-prewarm-build-cache.md new file mode 100644 index 0000000..5e34ff0 --- /dev/null +++ b/plans/active/20260624-covgate-prewarm-build-cache.md @@ -0,0 +1,382 @@ +# Pre-warm the Go build cache before covgate's parallel per-package coverage runs + +This ExecPlan is a living document. The sections Progress, Surprises & Discoveries, Decision Log, and Outcomes & Retrospective must be kept up to date as work proceeds. + +## Scope + +| Repository | Access | Description | +|-----------|--------|-------------| +| `gotools/` | read-write | Add a build-cache pre-warm pass to the covgate service and tests | + +This plan lives in `gotools/plans/backlog/` because all code changes happen inside the `gotools` repo, in `internal/services/covgate/` (and a small read-only reuse of `internal/services/gocover/`). No CLI, DB, or external-service changes are required. + +## Purpose / Big Picture + +`miru covgate` enforces per-package Go coverage thresholds. It resolves the package set, then fans out `N` parallel per-package coverage runs (`N` = effective parallelism, default `runtime.NumCPU()`). Each run executes, via `gocover.Measure`: + + go test -coverprofile= -coverpkg= + +The `-coverpkg=` flag instruments only that package's own (small, unique) target. But every one of those `go test` invocations must first **compile the shared, non-instrumented dependency packages** that the test binary imports. Go does not lock build actions across separate `go test` processes, so when `N` runs start simultaneously they each independently begin compiling the same heavy shared dependencies before any process has populated `~/.cache/go-build`. This is a compile stampede that roughly doubles per-package wall time on heavy packages. + +The fix proven downstream: run **one** coherent compile pass over the same package + test-path set first — `go test -run='^$' ` with no `-coverpkg` instrumentation — so the shared dependencies land in the build cache exactly once. After that single pass, the `N` parallel coverage runs find their shared deps already cached and only pay for their own small instrumented target. In CI this cut covgate's wall time from ~188s to ~44s. + +Key insight that shapes the design: **the stampede is on the SHARED NON-INSTRUMENTED dependency compiles, not on the per-package instrumented target.** Each coverage run instruments only its own unique package via `-coverpkg`. Therefore a single **plain** (non-instrumented) compile pass over the union of package + test paths is sufficient. We must **not** replicate per-package `-coverpkg` instrumentation in the warm pass — doing so would defeat Go's single build planner and re-introduce per-package work. + +After this change: + +- When covgate runs with parallelism > 1 **and** more than one package, it performs one warm-up compile pass before fanning out, then runs the parallel coverage checks exactly as today. +- When parallelism <= 1 **or** there is only a single package (no concurrency, hence no stampede), the warm pass is skipped and behavior is byte-for-byte identical to today. +- Coverage results, thresholds, `--exclude` handling, printed rows, and progress output are all unchanged. The warm pass is purely cache-warming. +- If the warm compile fails with a genuine build error, covgate propagates the error instead of silently proceeding. + +## Progress + +- [ ] Milestone 1 — Add an injectable `prewarm` seam to the `runner` struct and wire the real implementation in `Run` +- [ ] Milestone 2 — Implement the warm-pass helper in `gocover` (one `go test -run='^$'` over the union of test paths) +- [ ] Milestone 3 — Call the warm pass in `(*runner).run` before `runPackages`, gated on `parallelism > 1 && len(pkgs) > 1`, propagating errors +- [ ] Milestone 4 — Add/extend tests in `covgate_test.go` for the three seam assertions (invoked-once / skipped / error-propagated) +- [ ] Milestone 5 — Run preflight (`./scripts/preflight.sh`) and ensure it reports `clean`; fix anything flagged + +Use timestamps when you complete steps. Split partially completed work into "done" and "remaining" as needed. + +## Surprises & Discoveries + +(Add entries as work proceeds.) + +## Decision Log + +- Decision: The warm pass is a **single** `go test -run='^$' ` invocation over the union of all package + test paths, NOT a per-package loop. + Rationale: Go's single build planner deduplicates and parallelizes the shared dependency compiles within one invocation. A per-package loop would re-stampede or serialize redundantly. One invocation is the whole point. + Date/Author: 2026-06-24, planning subagent. + +- Decision: The warm pass uses **plain** compilation — no `-coverpkg`, no `-coverprofile`. + Rationale: The stampede is on shared non-instrumented dependency compiles. Each real run instruments only its own unique `-coverpkg` target, which is small and not shared, so warming it buys nothing. Adding `-coverpkg` to the warm pass would force a coverage-instrumented build of the union and defeat the single-planner benefit. `-run='^$'` matches no tests, so the pass compiles the test binaries but executes nothing. + Date/Author: 2026-06-24, planning subagent. + +- Decision: The warm pass covers exactly the **same package + test-path set** the real per-package runs will build, computed by reusing the existing `gocover.BuildTestPaths(pkg, relPkg, srcPrefix, testDir)` logic that `checkPackage` already uses — collected into a de-duplicated union. Do not duplicate path-construction logic. + Rationale: Warming a different set than the real runs build would leave some shared deps cold (under-warm) or compile unused packages (waste). Reuse guarantees the warm set tracks the real set across future path-logic changes. + Date/Author: 2026-06-24, planning subagent. + +- Decision: Gate the warm pass automatically on `parallelism > 1 && len(pkgs) > 1`. No new `Opts` flag in the first cut. + Rationale: With parallelism <= 1 there is no concurrency and thus no stampede; with a single package there is nothing to stampede against. In both cases the warm pass is pure overhead (it would double-compile that one package). The automatic gate matches the prompt's accepted default and keeps the `Opts` surface unchanged. A `--no-prewarm` opt-out can be added later if a need arises; it is intentionally out of scope here to avoid touching the CLI surface and `lint-surface` checks. + Date/Author: 2026-06-24, planning subagent. + +- Decision: The reported **"Total time" / wall-clock stays measured exactly as today** — around `runPackages` only (the existing `start := time.Now()` / `wallTime := time.Since(start)` bracket in `(*runner).run`). The warm pass runs **before** that bracket and is treated as setup, excluded from the reported total. + Rationale: "Total time" today communicates the parallel measurement phase. Keeping its definition stable preserves byte-comparable output for existing callers and tests (`TestPrintResults_UsesWallTime`, `TestRun_OutputContainsTiming`), and avoids conflating one-time cache warming with the steady-state measurement cost. The prompt explicitly prefers keeping the reported time measured the same way; this is that choice, stated. + Date/Author: 2026-06-24, planning subagent. + +- Decision: Warm-pass build failure propagates as an error from `(*runner).run` (returned before `runPackages` is reached). + Rationale: A genuine build error means the per-package runs would fail too; surfacing it early is correct and avoids a confusing later failure. The warm pass must never silently swallow a compile error. + Date/Author: 2026-06-24, planning subagent. + +## Outcomes & Retrospective + +(Summarize at completion or major milestones.) + +## Context and Orientation + +Covgate is a Go test-coverage gate exposed as the `covgate` subcommand of the `miru` CLI. All paths below are relative to the repo root `gotools/`. + +### Service layer — `internal/services/covgate/covgate.go` + +- `Opts` struct: `Packages`, `Exclude`, `SrcPrefix`, `TestDir`, `DefaultThreshold`, `Parallelism`, `TightnessEnabled`, `TightnessTolerance`, `Out`. (No new field is added by this plan.) +- `runner` struct currently injects three seams plus two scalar fields: + + type runner struct { + goModule func() (string, error) + goListPackages func(string) ([]string, error) + measure func(pkg string, testPaths []string) (float64, []byte, error) + parallelism int + emitProgress bool + } + + This plan adds a fourth seam, `prewarm`, alongside them (see Milestone 1). +- `Run(opts Opts) error` constructs the `runner`, wiring the real `gocover.GoModule`, `gocover.GoListPackages`, `gocover.Measure`, `effectiveParallelism(opts)`, and `emitProgress`. It then calls `r.run(opts)`. This is where the real `prewarm` implementation is wired. +- `(*runner).run(opts Opts)` is the orchestration entry point. Current flow: + 1. resolve `module` via `r.goModule()` + 2. resolve `pkgs` via `r.goListPackages(opts.Packages)` + 3. `r.applyExclude(pkgs, opts.Exclude, w)` → final `pkgs`, `excluded` + 4. `r.writeRunHeader(w, len(pkgs), parallelism)` + 5. build `checkPackageCtx` + 6. `start := time.Now()` → `results := r.runPackages(pkgs, ctx, parallelism, w)` → `wallTime := time.Since(start)` + 7. `r.printResults(w, results, excluded, module, wallTime)` + + The warm pass is inserted **between step 5 and step 6** — after the final `pkgs` is known and `ctx` is built, but **before** the `start := time.Now()` timing bracket so it is excluded from "Total time". +- `checkPackageCtx` holds `module`, `srcPrefix`, `testDir`, `threshold`, `tightnessEnabled`, `tightnessTolerance`. The warm pass needs `module`, `srcPrefix`, `testDir` to build the same test paths. +- `(*runner).checkPackage(pkg, ctx)` is the per-package worker. It computes: + + relPkg := gocover.RelPkg(pkg, ctx.module) + ... + testPaths := gocover.BuildTestPaths(pkg, relPkg, ctx.srcPrefix, ctx.testDir) + + and then calls `r.measure(pkg, testPaths)`. **The warm pass must reuse this exact pair (`RelPkg` then `BuildTestPaths`) per package, unioned across all packages.** + +### Dependency layer — `internal/services/gocover/gocover.go` + +- `RelPkg(pkg, module string) string` — module-relative path. +- `BuildTestPaths(pkg, relPkg, srcPrefix, testDir string) []string` — returns `[]string{pkg}` plus, when an external test dir applies, `"./" + extPath`. This is the authoritative test-path construction; both `covgate.checkPackage` and `covratchet` already reuse it (`internal/services/covratchet/covratchet.go:213`). +- `Measure(pkg, testPaths)` / `MeasureWithEnv(pkg, testPaths, extraEnv)` — the real per-package coverage runner; uses `cmdutil.GoCommand("test", "-coverprofile=...", "-coverpkg="+pkg, testPaths...)`. The warm pass is the non-instrumented sibling of this: same `cmdutil.GoCommand` mechanism, but `go test -run='^$' ` with no coverage flags. +- `cmdutil.GoCommand(args ...string) *exec.Cmd` (`internal/services/cmdutil/cmd.go`) — builds an `exec.Cmd` for `go ...` with `GOWORK=off`. The warm pass MUST use this so it inherits the same toolchain/env as the real runs (otherwise it could warm a different build graph than the measured runs). + +### Tests — `internal/services/covgate/covgate_test.go` + +- Established seam-injection pattern: construct a `runner{...}` literal with stubbed `goModule`, `goListPackages`, `measure`, marked `//nolint:exhaustruct // test uses partial initialization`. `fakeMeasure(cov)` returns a passing measure stub. +- `TestRun_AllPass`, `TestRun_WithFailure`, `TestRun_Parallelism`, `TestRun_Exclude`, `TestRun_GoListError` show the run-level wiring. New tests inject the new `prewarm` seam the same way. +- `TestPrintResults_UsesWallTime` and `TestRun_OutputContainsTiming` lock the "Total time" semantics that this change must not disturb. + +### Validation scripts (`scripts/`) + +- `scripts/preflight.sh` — runs `lint.sh`, `covgate.sh`, and `lint-surface.sh`; prints `=== All checks passed ===` and exits 0 when clean. **This is the canonical preflight gate; a clean preflight is mandatory before publishing.** + +### Glossary + +- **Warm pass / pre-warm**: a single `go test -run='^$' ` invocation that compiles the test binaries (populating the shared build cache) but runs no tests. +- **Stampede**: multiple concurrent `go test` processes independently compiling the same uncached shared dependency at once, because Go does not lock build actions across processes. +- **Union test-path set**: the de-duplicated concatenation of `BuildTestPaths(...)` across every package covgate will measure. +- **Seam**: an injectable function field on `runner`, substituted in tests. +- **Preflight clean**: `./scripts/preflight.sh` exits 0 and prints `=== All checks passed ===`. + +## Plan of Work + +### Milestone 1 — Add the `prewarm` seam to `runner` and wire it in `Run` + +Edit `internal/services/covgate/covgate.go`. + +1. Add a fourth function field to the `runner` struct, mirroring the existing seam style and signature shape of `measure`: + + type runner struct { + goModule func() (string, error) + goListPackages func(string) ([]string, error) + measure func(pkg string, testPaths []string) (float64, []byte, error) + prewarm func(testPaths []string) error + parallelism int + emitProgress bool + } + + The seam takes the already-computed union of test paths and returns only an error (it produces no coverage). Keeping path computation in covgate (not in the seam) lets unit tests assert on the exact path set passed in. + +2. In `Run`, wire the real implementation (added in Milestone 2) alongside the existing seams: + + r := runner{ + goModule: gocover.GoModule, + goListPackages: gocover.GoListPackages, + measure: gocover.Measure, + prewarm: gocover.PrewarmBuild, + parallelism: effectiveParallelism(opts), + emitProgress: opts.Parallelism == 0, + } + +### Milestone 2 — Implement the warm-pass helper in `gocover` + +Edit `internal/services/gocover/gocover.go`. Add an exported `PrewarmBuild`: + + // PrewarmBuild compiles the test binaries for the given test + // paths without running any tests, populating the shared Go + // build cache so that subsequent parallel `go test` runs do + // not stampede on the same shared dependency compiles. It runs + // a single `go test -run='^$' ` (no coverage + // instrumentation) so Go's build planner deduplicates the + // shared compiles in one coherent pass. A genuine build error + // is returned (with combined output) so callers can surface it. + func PrewarmBuild(testPaths []string) error { + if len(testPaths) == 0 { + return nil + } + args := make([]string, 0, 2+len(testPaths)) + args = append(args, "test", "-run=^$") + args = append(args, testPaths...) + cmd := cmdutil.GoCommand(args...) + if out, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("prewarm build: %w\n%s", err, out) + } + return nil + } + + Notes: + - Reuse `cmdutil.GoCommand` so the warm pass inherits `GOWORK=off` and the same toolchain as `Measure`/`MeasureWithEnv`. + - Do **not** add `-coverpkg` or `-coverprofile` — plain compilation only (see Decision Log). + - `-run=^$` matches no test names, so binaries compile but nothing executes. + +### Milestone 3 — Call the warm pass in `(*runner).run`, gated and error-propagating + +Edit `internal/services/covgate/covgate.go` inside `(*runner).run`, **after** the `checkPackageCtx` is built and **before** `start := time.Now()`: + +1. Compute the gate and, when it holds, the union of test paths, then call the seam: + + if parallelism > 1 && len(pkgs) > 1 { + warmPaths := collectWarmPaths(pkgs, ctx) + if err := r.prewarm(warmPaths); err != nil { + return err + } + } + +2. Add a small helper that reuses the exact path logic `checkPackage` uses, unioned and de-duplicated while preserving first-seen order: + + // collectWarmPaths returns the de-duplicated union of the test + // paths covgate will build for every package, using the same + // RelPkg + BuildTestPaths logic as checkPackage so the warm + // pass compiles exactly the set the real runs need. + func collectWarmPaths(pkgs []string, ctx checkPackageCtx) []string { + seen := make(map[string]struct{}) + var paths []string + for _, pkg := range pkgs { + relPkg := gocover.RelPkg(pkg, ctx.module) + for _, p := range gocover.BuildTestPaths( + pkg, relPkg, ctx.srcPrefix, ctx.testDir, + ) { + if _, ok := seen[p]; ok { + continue + } + seen[p] = struct{}{} + paths = append(paths, p) + } + } + return paths + } + +3. Leave the existing timing bracket, `runPackages`, and `printResults` untouched. "Total time" continues to measure only the parallel phase. + +The relevant region of `(*runner).run` becomes: + + ctx := checkPackageCtx{ ... } // unchanged + + if parallelism > 1 && len(pkgs) > 1 { + if err := r.prewarm(collectWarmPaths(pkgs, ctx)); err != nil { + return err + } + } + + start := time.Now() + results := r.runPackages(pkgs, ctx, parallelism, w) + wallTime := time.Since(start) + return r.printResults(w, results, excluded, module, wallTime) + +### Milestone 4 — Tests + +See **Test Steps** below for the enumerated cases. All new tests inject the `prewarm` seam using the existing `//nolint:exhaustruct` runner-literal style. + +### Milestone 5 — Validation pass + +Run `./scripts/preflight.sh` from the repo root and fix any findings until it reports clean (see Validation and Acceptance). + +## Concrete Steps + +All commands run from `/home/ben/miru/workbench6/repos/gotools/`. The current branch is `perf/covgate-prewarm-build-cache`; do not switch branches. + +### Step 0 — Sanity baseline + + git status + go build ./... + go test ./internal/services/covgate/... ./internal/services/gocover/... + +Expected: clean tree (or only this plan file added), build exit 0, both packages `ok`. + +### Step 1 — Implement Milestones 1–3 (source) + +Edit `internal/services/gocover/gocover.go` (add `PrewarmBuild`) and `internal/services/covgate/covgate.go` (add `prewarm` seam, wire `Run`, add `collectWarmPaths`, insert the gated call). Then: + + go build ./... + +Expected: exit 0. + +### Step 2 — Implement Milestone 4 (tests) + +Edit `internal/services/covgate/covgate_test.go` per Test Steps below. Then: + + go test ./internal/services/covgate/... ./internal/services/gocover/... + +Expected: `ok` for both. + +### Step 3 — Milestone 5 preflight + + ./scripts/preflight.sh + +Expected final line `=== All checks passed ===`, exit 0. If not clean, read the offending output (lint / covgate / surface lint — note `PrewarmBuild` is a new exported symbol that `lint-surface` will see; that is expected and acceptable as a deliberate API addition), fix in place, and rerun until clean. + +## Test Steps + +Add the following to `internal/services/covgate/covgate_test.go`, using the established seam-injection pattern (a `runner{...}` literal with stubbed `goModule`, `goListPackages`, `measure`, plus the new `prewarm`, each marked `//nolint:exhaustruct`). Use `fakeMeasure(90.0)` for the measure stub. These cover the three required assertions. + +The shared stub shape for the `prewarm` seam records its invocations: + + var ( + warmCalls int + warmPaths []string + ) + prewarm := func(paths []string) error { + warmCalls++ + warmPaths = paths + return nil + } + +### Test 1 — `TestRun_Prewarm_InvokedOnce_WhenParallelAndMultiPkg` + +Assertion (1): warm pass invoked exactly once with the full package/test-path set when parallelism > 1 and there are multiple packages. + +- Use a single temp dir with subdirs `pkg/a`, `pkg/b`, `pkg/c` (mirror `TestRun_Parallelism`'s `t.TempDir()` + `t.Chdir` setup so `BuildTestPaths`/`RelPkg` resolve). +- `goModule` returns `modName`; `goListPackages` returns the three `modName + "/pkg/{a,b,c}"` paths. +- Inject `prewarm` recording stub; run `Opts{Out: &buf, DefaultThreshold: 80.0, Parallelism: 3}`. +- Assert: `warmCalls == 1`. +- Assert the union of test paths was passed: each of the three packages' import paths appears in `warmPaths` (with `SrcPrefix`/`TestDir` empty, `BuildTestPaths` returns just `[]string{pkg}`, so `warmPaths` should equal the three package paths). Confirm length 3 and membership. +- Sanity: `err == nil`, output still contains all three rows and `Total time:`. + +### Test 2 — `TestRun_Prewarm_Skipped_WhenSinglePackageOrSerial` + +Assertion (2): warm pass skipped when parallelism <= 1 OR a single package. Table-driven with subtests: + +- **SingleParallelism**: three packages, `Parallelism: 1`. Assert `warmCalls == 0`. +- **SinglePackage**: one package `pkg/a`, `Parallelism: 4`. Assert `warmCalls == 0`. +- (Optional) **SinglePackageSerial**: one package, `Parallelism: 1`. Assert `warmCalls == 0`. + +In every subtest assert `err == nil` and that the normal per-package rows still print (behavior unchanged when warm pass is skipped). + +### Test 3 — `TestRun_Prewarm_ErrorPropagates` + +Assertion (3): a warm-pass build error is propagated and `runPackages` is not reached. + +- Three packages, `Parallelism: 3` (gate satisfied). +- Inject `prewarm` returning `fmt.Errorf("prewarm build: boom")`. +- Inject a `measure` stub that flips a bool / increments a counter if called, so the test can assert the real runs did **not** start. +- Run and assert: `err != nil`; `err.Error()` contains `"prewarm build"` (or `"boom"`); the measure stub was never invoked; the output does **not** contain `Total time:` (run returned before printing results). + +### Notes on existing tests + +- `TestPrintResults_UsesWallTime` and `TestRun_OutputContainsTiming` must remain green unchanged — they validate the "Total time" semantics this change deliberately preserves. +- Existing `run`-level tests that build a `runner` literal without a `prewarm` field still satisfy their cases because they use `Parallelism: 0/1/2` with package counts where the gate is false (single package) or are unaffected; where an existing test has parallelism > 1 with multiple packages (e.g. `TestRun_Parallelism` uses `Parallelism: 2` with three packages), it will now invoke `r.prewarm`, which is a **nil** function in that literal and would panic. **Therefore: audit every existing `runner{...}` literal whose `run`/`Run` path can satisfy `parallelism > 1 && len(pkgs) > 1` and give it a no-op `prewarm: func([]string) error { return nil }`.** Concretely this includes `TestRun_Parallelism` and `TestRun_SuppressesProgress_WhenExplicitParallelism` (both `Parallelism: 2`, three / two packages). Add the no-op seam to those literals as part of Milestone 4. Tests that go through `Run` (the public wrapper) get the real `prewarm` automatically and need no change. + +## Validation and Acceptance + +Acceptance is observable behavior plus a clean preflight, in this order. **Changes MUST NOT be published (no push, no PR) until preflight reports `clean`.** + +1. **gocover tests pass.** + + go test ./internal/services/gocover/... + + Expected: `ok`. `PrewarmBuild` compiles and behaves (covered indirectly; the covgate seam tests assert the wiring). + +2. **covgate tests pass**, including the three new tests `TestRun_Prewarm_InvokedOnce_WhenParallelAndMultiPkg`, `TestRun_Prewarm_Skipped_WhenSinglePackageOrSerial`, `TestRun_Prewarm_ErrorPropagates`, and the unchanged timing tests: + + go test ./internal/services/covgate/... + + Expected: `ok`. + +3. **Build is clean.** + + go build ./... + + Expected: exit 0, no output. + +4. **Output is unchanged for callers.** No new rows, no change to `Total time` definition, no change to `--exclude`/threshold/tightness output. Confirmed by the surviving `TestRun_*` output assertions and `TestPrintResults_UsesWallTime`. + +5. **Preflight is clean — mandatory gate.** + + ./scripts/preflight.sh + + Expected final line: `=== All checks passed ===` with exit 0. **No changes are published, no PR is opened, and no branch is pushed until this command reports `clean`.** If it fails on lint, covgate, or surface lint, fix the underlying issue and rerun. Do not bypass preflight. + +## Idempotence and Recovery + +- Source edits (Milestones 1–3) are additive: one new exported function (`gocover.PrewarmBuild`), one new struct field (`runner.prewarm`), one new unexported helper (`collectWarmPaths`), and one gated call site. Re-applying is safe; if partially applied, re-read the file and reconcile against Plan of Work. +- Test edits (Milestone 4): if Go reports duplicate function names after a retry, remove the half-applied block and re-paste. Remember the no-op `prewarm` seam must be added to the pre-existing parallel `runner` literals (`TestRun_Parallelism`, `TestRun_SuppressesProgress_WhenExplicitParallelism`) or those tests will nil-panic. +- Preflight (Milestone 5) is idempotent — rerun freely. +- Rollback: the change is purely cache-warming with no semantic effect on results; revert the milestone commits in reverse order with `git revert ` if it must be removed after merge. Use `git restore ` (not `git reset --hard`) to discard uncommitted work on specific files during a retry. From 0f5ef8fa9ba95064df4ec8c98c45f1f920530af5 Mon Sep 17 00:00:00 2001 From: Benjamin Smidt Date: Wed, 24 Jun 2026 15:55:40 -0700 Subject: [PATCH 2/2] perf(covgate): pre-warm build cache before parallel coverage runs covgate fans out N parallel `go test -coverpkg=` processes; without a warm cache they each begin compiling the same shared dependency packages simultaneously (Go does not lock build actions across processes), a stampede that roughly doubles per-package wall time on heavy deps. Run one coherent `go test -run=^$ ` compile pass before fanning out, so shared deps land in the build cache once and the per-package runs only re-link. Gated on parallelism > 1 && len(pkgs) > 1 (no concurrency, no stampede). The warm pass runs no tests, needs no DB, and is excluded from the reported "Total time" so output stays byte-identical. Co-Authored-By: Claude Opus 4.8 (1M context) --- internal/services/covgate/covgate.go | 32 ++- internal/services/covgate/covgate_test.go | 203 +++++++++++++++++- internal/services/gocover/.covgate | 2 +- internal/services/gocover/gocover.go | 22 ++ internal/services/gocover/gocover_test.go | 42 ++++ .../20260624-covgate-prewarm-build-cache.md | 0 6 files changed, 297 insertions(+), 4 deletions(-) rename plans/{active => completed}/20260624-covgate-prewarm-build-cache.md (100%) diff --git a/internal/services/covgate/covgate.go b/internal/services/covgate/covgate.go index 455f14b..fcfadbc 100644 --- a/internal/services/covgate/covgate.go +++ b/internal/services/covgate/covgate.go @@ -33,6 +33,7 @@ type runner struct { goModule func() (string, error) goListPackages func(string) ([]string, error) measure func(pkg string, testPaths []string) (float64, []byte, error) + prewarm func(testPaths []string) error parallelism int emitProgress bool } @@ -52,6 +53,7 @@ func Run(opts Opts) error { goModule: gocover.GoModule, goListPackages: gocover.GoListPackages, measure: gocover.Measure, + prewarm: gocover.PrewarmBuild, parallelism: effectiveParallelism(opts), emitProgress: opts.Parallelism == 0, } @@ -101,12 +103,40 @@ func (r *runner) run(opts Opts) error { tightnessTolerance: opts.TightnessTolerance, } + if parallelism > 1 && len(pkgs) > 1 { + if err := r.prewarm(collectWarmPaths(pkgs, ctx)); err != nil { + return err + } + } + start := time.Now() results := r.runPackages(pkgs, ctx, parallelism, w) wallTime := time.Since(start) return r.printResults(w, results, excluded, module, wallTime) } +// collectWarmPaths returns the de-duplicated union of the test +// paths covgate will build for every package, using the same +// RelPkg + BuildTestPaths logic as checkPackage so the warm +// pass compiles exactly the set the real runs need. +func collectWarmPaths(pkgs []string, ctx checkPackageCtx) []string { + seen := make(map[string]struct{}) + var paths []string + for _, pkg := range pkgs { + relPkg := gocover.RelPkg(pkg, ctx.module) + for _, p := range gocover.BuildTestPaths( + pkg, relPkg, ctx.srcPrefix, ctx.testDir, + ) { + if _, ok := seen[p]; ok { + continue + } + seen[p] = struct{}{} + paths = append(paths, p) + } + } + return paths +} + // applyExclude removes packages matched by the comma-separated // exclude patterns from pkgs. It preserves the original order of // pkgs in both returned slices and prints a one-line notice to w @@ -121,7 +151,7 @@ func (r *runner) applyExclude( } excludedSet := make(map[string]struct{}) - for _, raw := range strings.Split(exclude, ",") { + for raw := range strings.SplitSeq(exclude, ",") { entry := strings.TrimSpace(raw) if entry == "" { continue diff --git a/internal/services/covgate/covgate_test.go b/internal/services/covgate/covgate_test.go index 80409eb..4d4384e 100644 --- a/internal/services/covgate/covgate_test.go +++ b/internal/services/covgate/covgate_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "runtime" + "slices" "strings" "testing" "time" @@ -281,7 +282,7 @@ func TestRun_Exclude(t *testing.T) { t.Errorf("expected exactly 1 SKIPPED, got %d:\n%s", got, out) } var skippedLine string - for _, line := range strings.Split(out, "\n") { + for line := range strings.SplitSeq(out, "\n") { if strings.Contains(line, "pkg/b") { skippedLine = line break @@ -318,6 +319,7 @@ func TestRun_Exclude(t *testing.T) { return out, nil }, measure: fakeMeasure(90.0), + prewarm: func([]string) error { return nil }, } //nolint:exhaustruct // test uses partial initialization @@ -408,6 +410,7 @@ func TestRun_Parallelism(t *testing.T) { }, nil }, measure: fakeMeasure(90.0), + prewarm: func([]string) error { return nil }, } //nolint:exhaustruct // test uses partial initialization @@ -489,6 +492,7 @@ func TestRun_EmitsProgress_WhenAutoParallelism(t *testing.T) { }, measure: fakeMeasure(90.0), emitProgress: true, + prewarm: func([]string) error { return nil }, } //nolint:exhaustruct // test uses partial initialization @@ -549,7 +553,7 @@ func TestRun_EmitsProgress_WhenAutoParallelism(t *testing.T) { // (used to verify progress lines carry full row data, not just the // counter). func hasProgressLineWith(out, marker, contains string) bool { - for _, line := range strings.Split(out, "\n") { + for line := range strings.SplitSeq(out, "\n") { if strings.Contains(line, marker) && strings.Contains(line, contains) { return true } @@ -576,6 +580,7 @@ func TestRun_SuppressesProgress_WhenExplicitParallelism(t *testing.T) { }, measure: fakeMeasure(90.0), emitProgress: false, + prewarm: func([]string) error { return nil }, } //nolint:exhaustruct // test uses partial initialization @@ -601,6 +606,200 @@ func TestRun_SuppressesProgress_WhenExplicitParallelism(t *testing.T) { } } +func TestRun_Prewarm_InvokedOnce_WhenParallelAndMultiPkg(t *testing.T) { + tmp := t.TempDir() + t.Chdir(tmp) + for _, rel := range []string{"pkg/a", "pkg/b", "pkg/c"} { + //nolint:gosec // G301: test directory + if err := os.MkdirAll(filepath.Join(tmp, rel), 0o755); err != nil { + t.Fatal(err) + } + } + + pkgs := []string{modName + "/pkg/a", modName + "/pkg/b", modName + "/pkg/c"} + + var ( + warmCalls int + warmPaths []string + ) + prewarm := func(paths []string) error { + warmCalls++ + warmPaths = paths + return nil + } + + var buf bytes.Buffer + //nolint:exhaustruct // test uses partial initialization + r := runner{ + goModule: func() (string, error) { return modName, nil }, + goListPackages: func(string) ([]string, error) { return pkgs, nil }, + measure: fakeMeasure(90.0), + prewarm: prewarm, + } + + //nolint:exhaustruct // test uses partial initialization + err := r.run(Opts{Out: &buf, DefaultThreshold: 80.0, Parallelism: 3}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if warmCalls != 1 { + t.Fatalf("expected prewarm invoked exactly once, got %d", warmCalls) + } + // With empty SrcPrefix/TestDir, BuildTestPaths returns just + // the package path, so the union is the three package paths. + if len(warmPaths) != 3 { + t.Fatalf("expected 3 warm paths, got %d: %v", len(warmPaths), warmPaths) + } + for _, pkg := range pkgs { + if !slices.Contains(warmPaths, pkg) { + t.Errorf("warm paths missing %q: %v", pkg, warmPaths) + } + } + out := buf.String() + for _, want := range []string{"pkg/a", "pkg/b", "pkg/c", "Total time:"} { + if !strings.Contains(out, want) { + t.Errorf("output missing %q:\n%s", want, out) + } + } +} + +func TestCollectWarmPaths_DeduplicatesSharedPaths(t *testing.T) { + // With empty SrcPrefix/TestDir, BuildTestPaths returns just the + // package path, so a duplicated package in the input must collapse + // to a single warm path (exercising the seen-set skip branch). + pkgA := modName + "/pkg/a" + pkgB := modName + "/pkg/b" + pkgs := []string{pkgA, pkgB, pkgA} + + //nolint:exhaustruct // only module is needed for path collection + got := collectWarmPaths(pkgs, checkPackageCtx{module: modName}) + + want := []string{pkgA, pkgB} + if len(got) != len(want) { + t.Fatalf("expected %d deduplicated paths, got %d: %v", len(want), len(got), got) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("path %d: expected %q, got %q (full: %v)", i, want[i], got[i], got) + } + } +} + +func TestRun_Prewarm_Skipped_WhenSinglePackageOrSerial(t *testing.T) { + cases := []struct { + name string + pkgs []string + parallelism int + }{ + { + name: "SingleParallelism", + pkgs: []string{ + modName + "/pkg/a", + modName + "/pkg/b", + modName + "/pkg/c", + }, + parallelism: 1, + }, + { + name: "SinglePackage", + pkgs: []string{modName + "/pkg/a"}, + parallelism: 4, + }, + { + name: "SinglePackageSerial", + pkgs: []string{modName + "/pkg/a"}, + parallelism: 1, + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + tmp := t.TempDir() + t.Chdir(tmp) + for _, pkg := range tc.pkgs { + rel := gocover.RelPkg(pkg, modName) + //nolint:gosec // G301: test directory + if err := os.MkdirAll(filepath.Join(tmp, rel), 0o755); err != nil { + t.Fatal(err) + } + } + + warmCalls := 0 + var buf bytes.Buffer + //nolint:exhaustruct // test uses partial initialization + r := runner{ + goModule: func() (string, error) { return modName, nil }, + goListPackages: func(string) ([]string, error) { return tc.pkgs, nil }, + measure: fakeMeasure(90.0), + prewarm: func([]string) error { warmCalls++; return nil }, + } + + //nolint:exhaustruct // test uses partial initialization + err := r.run(Opts{ + Out: &buf, + DefaultThreshold: 80.0, + Parallelism: tc.parallelism, + }) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if warmCalls != 0 { + t.Errorf("expected prewarm skipped, got %d calls", warmCalls) + } + out := buf.String() + rel := gocover.RelPkg(tc.pkgs[0], modName) + if !strings.Contains(out, rel) { + t.Errorf("output missing per-package row %q:\n%s", rel, out) + } + }) + } +} + +func TestRun_Prewarm_ErrorPropagates(t *testing.T) { + tmp := t.TempDir() + t.Chdir(tmp) + for _, rel := range []string{"pkg/a", "pkg/b", "pkg/c"} { + //nolint:gosec // G301: test directory + if err := os.MkdirAll(filepath.Join(tmp, rel), 0o755); err != nil { + t.Fatal(err) + } + } + + measureCalls := 0 + var buf bytes.Buffer + //nolint:exhaustruct // test uses partial initialization + r := runner{ + goModule: func() (string, error) { return modName, nil }, + goListPackages: func(string) ([]string, error) { + return []string{ + modName + "/pkg/a", + modName + "/pkg/b", + modName + "/pkg/c", + }, nil + }, + measure: func(string, []string) (float64, []byte, error) { + measureCalls++ + return 90.0, nil, nil + }, + prewarm: func([]string) error { return fmt.Errorf("prewarm build: boom") }, + } + + //nolint:exhaustruct // test uses partial initialization + err := r.run(Opts{Out: &buf, DefaultThreshold: 80.0, Parallelism: 3}) + if err == nil { + t.Fatal("expected error from prewarm build") + } + if !strings.Contains(err.Error(), "prewarm build") { + t.Errorf("error missing 'prewarm build': %v", err) + } + if measureCalls != 0 { + t.Errorf("expected measure never invoked, got %d calls", measureCalls) + } + if strings.Contains(buf.String(), "Total time:") { + t.Errorf("expected run to return before printing results:\n%s", buf.String()) + } +} + func TestMeasure_Pass(t *testing.T) { tmp := t.TempDir() t.Chdir(tmp) diff --git a/internal/services/gocover/.covgate b/internal/services/gocover/.covgate index cf199f8..bd5e61c 100644 --- a/internal/services/gocover/.covgate +++ b/internal/services/gocover/.covgate @@ -1 +1 @@ -92.1 +93.3 diff --git a/internal/services/gocover/gocover.go b/internal/services/gocover/gocover.go index 9e5c51e..95939d0 100644 --- a/internal/services/gocover/gocover.go +++ b/internal/services/gocover/gocover.go @@ -171,6 +171,28 @@ func MeasureWithEnv( return coverage, output, nil } +// PrewarmBuild compiles the test binaries for the given test +// paths without running any tests, populating the shared Go +// build cache so that subsequent parallel `go test` runs do +// not stampede on the same shared dependency compiles. It runs +// a single `go test -run='^$' ` (no coverage +// instrumentation) so Go's build planner deduplicates the +// shared compiles in one coherent pass. A genuine build error +// is returned (with combined output) so callers can surface it. +func PrewarmBuild(testPaths []string) error { + if len(testPaths) == 0 { + return nil + } + args := make([]string, 0, 2+len(testPaths)) + args = append(args, "test", "-run=^$") + args = append(args, testPaths...) + cmd := cmdutil.GoCommand(args...) + if out, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("prewarm build: %w\n%s", err, out) + } + return nil +} + // ExecOutput runs a command with GOWORK=off and returns // its stdout. func ExecOutput(errW io.Writer, name string, args ...string) (string, error) { diff --git a/internal/services/gocover/gocover_test.go b/internal/services/gocover/gocover_test.go index a353f7a..d7bb5ee 100644 --- a/internal/services/gocover/gocover_test.go +++ b/internal/services/gocover/gocover_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "strings" "testing" ) @@ -332,6 +333,47 @@ func TestMeasure_TestFailure(t *testing.T) { } } +func TestPrewarmBuild_Empty(t *testing.T) { + if err := PrewarmBuild(nil); err != nil { + t.Fatalf("expected nil error for empty paths, got: %v", err) + } +} + +func TestPrewarmBuild_Success(t *testing.T) { + makeGoProject(t) + + if err := PrewarmBuild([]string{"testmod/mypkg"}); err != nil { + t.Fatalf("PrewarmBuild: %v", err) + } +} + +func TestPrewarmBuild_BuildError(t *testing.T) { + tmp := t.TempDir() + t.Chdir(tmp) + + goMod := "module testmod\n\ngo 1.23\n" + //nolint:gosec // G306: test file + _ = os.WriteFile(filepath.Join(tmp, "go.mod"), []byte(goMod), 0o644) + pkg := filepath.Join(tmp, "mypkg") + //nolint:gosec // G301: test directory + _ = os.MkdirAll(pkg, 0o755) + // Source that does not compile so the build planner fails. + //nolint:gosec // G306: test file + _ = os.WriteFile( + filepath.Join(pkg, "lib.go"), + []byte("package mypkg\n\nfunc Add() int { return }\n"), + 0o644, + ) + + err := PrewarmBuild([]string{"testmod/mypkg"}) + if err == nil { + t.Fatal("expected error from failing build") + } + if !strings.Contains(err.Error(), "prewarm build") { + t.Errorf("expected 'prewarm build' in error, got: %v", err) + } +} + func TestNonEmptyLines(t *testing.T) { tests := []struct { name string diff --git a/plans/active/20260624-covgate-prewarm-build-cache.md b/plans/completed/20260624-covgate-prewarm-build-cache.md similarity index 100% rename from plans/active/20260624-covgate-prewarm-build-cache.md rename to plans/completed/20260624-covgate-prewarm-build-cache.md