perf(covgate,covratchet): cap child GOMAXPROCS to avoid CI CPU oversubscription#28
Merged
Conversation
Adds plans/backlog/20260508-cap-child-gomaxprocs.md describing the covgate/covratchet CI CPU-oversubscription fix as four commit-per- milestone steps. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Names the four acceptance tests in Validation, corrects the fmt-import note (both files already import fmt), and uses the module-qualified "testmod/mypkg" path in the MeasureWithEnv test example to match the existing TestMeasure convention. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…iption Default outer parallelism switches from runtime.NumCPU() to runtime.GOMAXPROCS(0) so it respects cgroup CPU limits and the GOMAXPROCS env in CI containers (Go 1.25 makes that primitive cgroup-aware). Each spawned go test subprocess now receives GOMAXPROCS=N where N = max(1, runtime.GOMAXPROCS(0) / parallelism), so outer * inner parallelism stays close to the available CPU budget instead of ballooning to NumCPU * NumCPU. Implemented via a new gocover.MeasureWithEnv that takes an extra-env slice; existing gocover.Measure stays as a thin wrapper for backward compatibility. Bump covratchet's .covgate threshold to 99.5 in anticipation of new tests that lift its measured coverage to 100%; covgate's tightness check would otherwise flag the gap. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add intent comments noting effectiveParallelism/childGOMAXPROCS are
intentionally duplicated across covgate and covratchet so future
readers don't DRY-refactor the per-service split or let copies drift.
- Thread the parallelism value through a new runner.parallelism field
so Run() and r.run() share a single computed value (and r.run still
falls back to effectiveParallelism(opts) when r.parallelism is zero,
preserving the "construct a bare runner{} in tests" path).
- Document MeasureWithEnv's last-wins env merge semantics so callers
can rely on extraEnv overriding earlier definitions of the same key.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…XPROCS Add tests so the new code paths are covered against the per-package .covgate thresholds: - TestMeasureWithEnv_AppliesExtraEnv exercises the extraEnv branch of the new gocover.MeasureWithEnv against a tiny temp Go module. - TestEffectiveParallelism and TestChildGOMAXPROCS pin both branches of each helper in covgate and covratchet. - TestRun_PublicWrapper_PassesThrough drives Run end-to-end against a real Go module so the closure body in Run (which calls MeasureWithEnv) is reached. Required because covgate's .covgate is 100.0; without this the closure body would be uncovered. - Rename TestRun_Parallelism_DefaultsToNumCPU to *_DefaultsToGOMAXPROCS in both covgate and covratchet, including the inline error message, so the test name reflects the new GOMAXPROCS-based default. Also adjust line wrapping of the new code so it satisfies both the custom collapse linter and the 88-col line-length rule: - Wrap the duplication-intent comment over two lines. - Split the measure closure body over two lines (signature stays on one line, gofumpt-canonical) so the line stays under 88 cols. - Wrap MeasureWithEnv's signature so the function declaration fits. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Tighten TestEffectiveParallelism's default-branch assertion to compare against runtime.GOMAXPROCS(0), so a regression returning any positive constant would be caught. - Tighten TestChildGOMAXPROCS's no-clamp assertion to verify childGOMAXPROCS(1) == runtime.GOMAXPROCS(0) (the function multiplies-out to that ratio at parallelism=1). - Rename TestRun_PublicWrapper_PassesThrough to *_HappyPath to match what the test actually pins (a successful wrapper run for coverage), since it does not probe the injected GOMAXPROCS. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
5 tasks
ben-miru
added a commit
that referenced
this pull request
May 18, 2026
## Summary - Reverts the CI wall-clock regression introduced by #28: restores the outer default to `runtime.NumCPU()` and drops the per-child `GOMAXPROCS=max(1, NumCPU/parallelism)` env injection used to start each `go test` invocation. - The premise of #28 was avoiding `NumCPU x NumCPU` oversubscription, but this repo has zero `t.Parallel` callsites — so the inner cap never throttled test execution, it only throttled compile/link, which is exactly the phase that dominates CI wall-clock time. - Adds mutex-guarded `[idx/total] STATUS pkg [elapsed]` progress output to `covgate` and `covratchet` when `--parallelism=0` (auto mode), with a leading announcement line above the per-package table so operators can see liveness on slow runners. - Plan with milestones, rationale, and validation: `plans/completed/20260517-restore-parallelism-and-progress.md`. ## Test plan - [ ] `./preflight.sh` passes locally (includes covgate/covratchet self-lint and unit tests) - [ ] `go test ./internal/services/covgate/... ./internal/services/covratchet/...` passes - [ ] Manual: run `covratchet --parallelism=0` against a multi-package target and confirm the announcement line plus interleaved `[idx/total] STATUS pkg [elapsed]` rows appear without interleaved-character corruption - [ ] Manual: run `covgate --parallelism=0` and confirm the same progress output behavior - [ ] CI on this PR completes faster than the post-#28 baseline on a comparable run --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
go testsubprocess per package behind a parallelism semaphore. Outer parallelism defaulted toruntime.NumCPU()and each child go test inherited the parent's GOMAXPROCS, so the effective worker count was roughly NumCPU × NumCPU — ~16 threads on a 4-core CI runner. The repo has zerot.Parallelcallsites, so the inner GOMAXPROCS only helped compile/build, never tests.runtime.NumCPU()toruntime.GOMAXPROCS(0)so it respects cgroup CPU limits and theGOMAXPROCSenv in CI containers (cgroup-aware in Go 1.25).GOMAXPROCS=Ninto each child go test subprocess whereN = max(1, runtime.GOMAXPROCS(0) / parallelism). Outer × inner ≈ available CPUs; explicit--parallelismoverrides still scale inner accordingly.gocover.MeasureWithEnv(existingMeasurebecomes a thin wrapper); per-serviceeffectiveParallelism/childGOMAXPROCShelpers (deliberately duplicated with a comment); arunner.parallelismfield soRunandr.runshare one computed value. Also bumpinternal/services/covratchet/.covgate97.4 → 99.5 to track the new measured coverage.Test plan
🤖 Generated with Claude Code