Cache Go module downloads in a persistent named cache (GOMODCACHE) - #23424
Cache Go module downloads in a persistent named cache (GOMODCACHE)#23424rdeknijf wants to merge 3 commits into
Conversation
|
Confirming the description's framing with a real-repo measurement. On a 24-module monorepo A/B (warm daemon and store, remote cache off), cold wall time is flat, exactly as the description says: on a fast connection, downloading was never the slow part. This PR only touches the two download processes, so it does not affect compile, link, or the warm incremental build at all. The win remains cold CI and slow or rate-limited proxies, where a cold build otherwise pays the full download set every time. Methodology and the per-lever breakdown are on #20274. |
|
Are the benchmarking script available somewhere? |
|
cc @jgranstrom |
|
This looks generally good to me. I benchmarked it as well on a repo with ~500 modules. Correctness looks good. Cache behavior also checks out, only affected modules need to redownload to the shared cache, then are captured via action cache. And with the slimmer capture it reduces the store outside the named cache. 👍 The one regression, or rather trade-off, I found is on a completely cold download of an entire A follow-up to address that regression could be to extract directly into the sandbox (as before) and keep only the downloaded archives in the named cache, though I'm not sure there's a trivial path to that. I can rebase my #23499 on this once it's landed. |
There was a problem hiding this comment.
I added a few comments about not hard-coding binary paths for CLI tools.
Relatedly, this change increases the complexity of the shell script injected for the Process generated from a GoSdkProcess. What are your thoughts on adding assertions that the exact expected artifacts were in fact copied into the new cache?
| module_version="$__PANTS_GO_FETCH_MODULE" | ||
| "{goroot.path}/bin/go" mod download -json "$module_version" > __module_metadata.json | ||
| download_status=$? | ||
| /bin/cat __module_metadata.json |
There was a problem hiding this comment.
This code should not assume that the environment has cat at /bin (nor any other utility for that matter). The rule code should use BinaryPathRequest (or one of its tool-specific subclasses) to request the engine to find the actual path of the tool to be used.
In this case, the rule should request CatBinary from pants.core.util_rules.system_binaries.
There was a problem hiding this comment.
Done, it now requests CatBinary and interpolates the discovered path. Same treatment for the other utilities in this script: CpBinary, MkdirBinary, and PwdBinary for the pwd call further up, which was the remaining hard-coded /bin path. The pwd one passes -P explicitly, since go needs the resolved physical path and a bare pwd would return a logical one containing symlinks. That was implicit in the old hard-coded call, so it seemed better to state it than to depend on which pwd gets found.
All the paths are shlex.quoted before going into the script.
| if [ -n "$__PANTS_GO_MODCACHE" ]; then | ||
| export GOMODCACHE="${{sandbox_root}}/__gomodcache" | ||
| export GOFLAGS="${{GOFLAGS:+${{GOFLAGS}} }}-modcacherw" | ||
| /bin/mkdir -p "$GOMODCACHE" |
There was a problem hiding this comment.
Similar comment to the one for cat: Use the path from MkdirBinary.
There was a problem hiding this comment.
Done, this uses MkdirBinary now. I converted the pre-existing /bin/mkdir on the GOPATH/GOCACHE setup line in the same pass, since it had the same problem.
| exit "$download_status" | ||
| fi | ||
| # Parse the Dir/GoMod paths from the metadata with shell string operations only: | ||
| # the sandbox PATH cannot be assumed to provide grep/sed. |
There was a problem hiding this comment.
You can ask the engine to find a grep binary if you want. Trade off is requiring grep to be present versus complicated bash logic.
There was a problem hiding this comment.
I went with keeping the bash logic. The trade-off you describe is real, and I do not think it is clear-cut, but requiring grep in every Go sandbox to parse two fields felt like the larger cost.
I did tighten the parse so it is not silently wrong: it now rejects missing or unrecognized fields rather than slicing whatever it got. It is still string matching, so I will not claim it cannot misparse a sufficiently strange path. Happy to switch to GrepBinary if you would rather take the dependency, it is a small change.
| /bin/mkdir -p "$dest_dir" "${{dest_gomod%/*}}" || exit 1 | ||
| /bin/cp -Rp "$dir/." "$dest_dir/" || exit 1 | ||
| /bin/cp -p "$gomod" "$dest_gomod" || exit 1 |
There was a problem hiding this comment.
Use the paths from the applicable BinaryPathRequest result types in pants.core.util_rules.system_binaries.
There was a problem hiding this comment.
Done. go_sdk_invoke_setup now takes CatBinary, CpBinary, MkdirBinary and PwdBinary as rule parameters and interpolates the resolved paths, so there are no hard-coded /bin paths left in this script.
|
Thanks for the contribution. We've just branched for 2.33.x, so merging this pull request now will come out in 2.34.x, please move the release notes updates to docs/notes/2.34.x.md if that's appropriate. |
Previously every sandbox received a fresh GOMODCACHE, causing all
third-party modules to be re-downloaded from the network on every cold
build. This makes third-party module downloads use the `go_mod_cache`
named cache as an accelerator: the download process writes modules into
the shared cache and copies them into a sandbox-local `gopath/pkg/mod`
tree before capture. Captured digests remain the source of truth, so
results are byte-identical whether the cache is warm or cold.
Key design choices:
- Download and copy happen in a single process (the `__PANTS_GO_FETCH_MODULE`
mode in `__run_go.sh`) to satisfy the self-healing invariant: if the
named cache is wiped between steps, the process that captures `gopath/`
must itself be able to re-populate it. A two-process shape would violate
this invariant.
- GOMODCACHE is set to `__gomodcache` (sibling of `gopath/`, outside the
captured tree) when `use_module_cache=True`. `output_directories=("gopath",)`
can therefore never accidentally walk into the shared cache.
- `-modcacherw` is injected via `GOFLAGS` so Pants can prune or clear the
named cache without `rm: cannot remove ... Permission denied` errors.
- Only the two `allow_downloads=True` processes in `third_party_pkg.py`
get the named cache. Compile/link/vet processes run `GOPROXY=off` against
captured digests and must not see a shared cache that could silently satisfy
a missing input.
- The fetch mode uses only shell builtins and /bin tools (no grep/sed/PATH assumptions), emits the module metadata on stdout and propagates the
exit code of `go mod download`, so failures surface through the normal
fallible-process path and the engine never materializes the module tree
just to read the metadata.
- Remote execution without `--remote-execution-append-only-caches-base-path`
degrades gracefully: the cache mount is absent, `mkdir -p $GOMODCACHE`
creates a plain sandbox-local directory, and behavior is identical to today.
- `ModuleDescriptors.go_mods_digest` is removed (zero consumers, verified).
One-time invalidation: the `__run_go.sh` script change invalidates all
previously cached Go process results.
See pantsbuild#13390.
84caacf to
27aa5db
Compare
The sandbox root is resolved with a hard-coded /bin/pwd, which assumes a path that is not guaranteed to exist. Use the PwdBinary system binary like the other utilities in this script, and pass -P explicitly so the physical path resolution the go tool relies on is stated rather than implied by which pwd happens to be invoked.
|
@tdyas I answered your four review comments inline on the threads themselves, so this is only the part that does not belong on a specific line. On the assertions you asked about: the script now checks after the copy that the destination On your question about the benchmarking scripts: https://gist.github.com/rdeknijf/ade1f4f652e234c01721670097c72f0d. @jgranstrom thanks for benchmarking this on a 500-module repo, that is the validation it needed most. Agreed on the trade-off, with one clarification on where the cost sits: the copy runs on every fetch execution, so an action cache hit is what skips it. What is specific to the cold case is the extra pass, since with a cold module cache you pay the extraction into it and then the copy out of it, where before there was only the extraction. Once the module cache is warm the copy replaces work the old code did anyway. Your follow-up direction looks right to me. Extraction looks deterministic enough for it, since Go writes 0444 regular files and does not carry archive timestamps through, and Pants digests only record content, path and the executable bit. The checksumming does less work for us than it first appears though: Go verifies an archive when it first downloads it, but once the On #23499, that ended up landing ahead of this one, so there is nothing to rebase on your side. I rebased this PR onto current main on the 24th, so your change is already in here. @cburroughs release note moved to |
jgranstrom
left a comment
There was a problem hiding this comment.
The test failures look to be because we re-use a test fixture go-sample-for-test@v0.0.1 across tests with different contents, so the hash verification fails for whoever lost the race to the named cache. The fix could be either naming them uniquely per test/variation, or use an isolated named cache per test.
Worth noting that this also fixes another issue that I noticed but not sure if it's been mentioned. Before, since we downloaded without -modcacherw, every sandbox for a go module download was silently partially leaked. Sandbox cleanup couldn't delete the extracted module cache inside the sandbox (gopath/pkg/mod) because of the read-only directory permissions, so extracted go mods leaked on every download. Since this PR adds -modcacherw, those sandboxes can now be fully torn down at cleanup.
Go stores a downloaded module under `<module>@<version>`, a path that says nothing about the bytes it holds. A single shared GOMODCACHE therefore serves whatever the first build happened to store there to every later build, and when those bytes disagree with the consumer's `go.sum`, Go refuses to build with a checksum-mismatch SECURITY ERROR that persists until someone wipes the cache by hand. Re-tagging a version, which private modules and internal proxies do, is enough to trigger it. The named cache is now partitioned. The module fetch uses a partition derived from `<module>@<version>` plus the `go.sum` entries the caller expects for it, and the module-graph analysis uses one derived from the `go.mod`/`go.sum` pair in its input digest. Builds that disagree about a module's contents land in different partitions, so a stale entry can no longer poison a later build and changing the recorded checksums re-downloads instead of failing. A module with no recorded checksums has nothing to partition by, and there the integrity of a download rests on the environment instead: which proxy served it and whether the checksum database was consulted. The partition therefore also covers `GOPROXY`, `GOPRIVATE`, `GOSUMDB` and the rest of that set, so builds configuring them differently never share downloads. This also fixes the Go backend's own test suite, where every fixture publishes `pantsbuild.org/go-sample-for-test@v0.0.1` with per-test file contents through a local module proxy. With one shared partition the first test to run decided what every later test in the same shard received.
|
@tdyas @jgranstrom Pushed a fix for the CI failures on this branch, and moved the release note to What went wrong: the named cache was a single shared GOMODCACHE. Go stores an extracted module at The same hazard exists outside the test suite: re-tagging a version, which private modules and internal proxies do, leaves a stale extraction behind that fails every later build until someone wipes the cache by hand. The fix partitions the named cache. The module fetch uses a partition derived from Two tests cover it: one publishes the same |
|
OP and reviewers - I'm just checking in on the status of this. What needs to happen to merge it? Thanks. |
|
I would have liked to see more benchmarks on it. I ran a slightly more comprehensive benchmark on it against main on ~700 modules and I'm considering if this needs the additional pass fix before landing. On a not-slow network I could not find a configuration where this is a win. Not cold cache, not warm cache. Even with the named cache fully populated it is significantly slower than having no cache at all, and it costs 80% more disk across the named cache and local store. I wanted to attribute the regression properly, so I did a quick experiment using GOPROXY so that there is no additional materialization while still adding the named cache. I.e. Go downloads the archive, we publish it to the named cache, and later builds reach for it via GOPROXY=file:// instead of copying an extracted tree back into the sandbox. That came in at ~2% wall time and ~2% disk against the same baseline. To be clear though, that prototype is not a speedup either. Its warm-cache cell is +2.9% against the baseline and about 8s slower than its own cold run. Reading an archive back off local disk and copying it into the sandbox costs about what downloading it costs here, so the named cache does not buy wall time under either transport on my benchmarks. The difference is only in what having the cache costs you: ~2% instead of ~22%. Another secondary cost introduced is the sandbox cleanup. That was silently skipped on main because Go leaves module directories read-only, so remove_dir_all fails and the error is swallowed. Main leaks ~700 sandboxes / 2.1GB per cold run, indefinitely. -modcacherw correctly fixes that and adds the cost back, which is significant and comes with contention problems. That is a separate issue in itself, but for the benchmarks to be fair I measured against main with that flag enabled so cleanup runs on both sides. Isolating with One thing worth reconsidering is the motivating case. #13390 is about a go.mod change forcing a full re-download. That is already handled on main: each download is keyed on And note that named caches are excluded from the action cache key, so with a warm local store these processes never execute and the cache is never read at all. Numbers below. Base 23be51b, free-threaded CPython 3.14.6, --no-pantsd, no remote cache, fresh store and named-cache dir per run. Variants were run alternating rather than in blocks so machine drift affects them equally, each run verified it was actually running the variant it claimed, and runs producing the wrong target count were discarded. Process counts identical across every arm (1,549). Measured under both third_party_target_granularity settings; every ratio agreed within ~2 points, so the numbers are granularity-independent. Fetch and analysis (
Work vs cleanup, isolated with
Disk (local store + named cache)
In-process time and other scopes
The prototype only swaps transport on the fetch path. Metadata steps ( My recommendation is to look into the near-zero-cost wiring via GOPROXY before landing. The mechanism here is sound and the leak fix is valuable on its own, but even with a corrected baseline, extract-then-copy costs more than the network it saves in every cell I could measure and nearly doubles Go disk usage. Sharing only the archive would not make builds faster here either, but it makes the cost of having the cache available ~2% rather than ~22%, which matters on the slow networks where it might pay off. |
|
@benjyw @jgranstrom @tdyas Closing this one, and splitting out the part of it that is worth keeping. Thanks in particular to @jgranstrom for the ~700-module benchmark. It changed my mind on the cache, and I would rather act on the data than defend the PR. The short version of why this should not land as-is:
The one piece of this PR that stands on its own is the sandbox leak fix. Without If the slow-network case ever gets a concrete workload behind it, the right shape is the |
Disclaimer: Like my previous Go PR, I'm still not primarily a golang developer. However, the fact that Golang doesn't work properly in Pants has been the bane of my existence for like 2 years now. The moment that Mythos/Fable dropped I immediately had it dig deeply into whether the whole road to proper-golang-in-pants could be opened up. So this is all by Claude Code with Fable 5 (xhigh), with consults to GPT 5.5 (xhigh) and Gemini 3.1 Pro. I've had it check and recheck, I ran many different roles over it, and I had it explain and re-explain it to me, and then I checked myself. The cache-partitioning commit that fixes the CI failure came later and was written by Claude Opus 5, then reviewed by three separate Fable passes (semantics, security, tests) before I pushed it; same drill of checking and re-checking.
So, as much as I dislike AI slop and am worried about AI PR overload, I've done my very best to avoid exactly that while still using AI. I hope we can get this road unblocked.
The rest is Fable talking:
Every Go sandbox currently gets a fresh GOMODCACHE, so a cold build re-downloads every third-party module from the network, and so does any change that invalidates the cached download processes. This is #13390; I posted the design there in #13390 (comment) and benjyw signed off on it, deferring the Go specifics to tdyas.
This PR gives the two download processes in
third_party_pkg.py(the only Go processes that run withallow_downloads=True) ago_mod_cachenamed cache, used purely as a download accelerator. Captured digests remain the source of truth: modules are copied out of the shared cache into the sandbox'sgopath/pkg/modbefore capture, so process results are byte-identical whether the cache is warm or cold, and compile, link, and vet sandboxes never see the shared cache at all. They keep running withGOPROXY=offagainst captured digests, exactly as today.Mechanics:
Download and copy happen in one process: a fetch mode in
__run_go.shrunsgo mod download -json, emits the metadata on stdout, and copies the extracted module and its go.mod out of the cache intogopath/pkg/mod. Keeping it one process preserves self-healing: wipe the named cache and the next run of the same process repopulates it.GOMODCACHE points inside
__gomodcache, a sibling of the capturedgopath/tree, sooutput_directories=("gopath",)can never capture the shared cache by accident.The cache is partitioned by the checksums the caller expects. Go stores a module at
<module>@<version>, a path that says nothing about the bytes, so one shared cache would serve whatever the first build stored there to every later build. A disagreement then means a checksum-mismatch SECURITY ERROR that survives until someone wipes the cache by hand, and re-tagging a version (which private modules and internal proxies do) is enough to trigger it. The fetch process therefore uses a partition derived from<module>@<version>plus thego.sumentries the caller expects for it, and module-graph analysis uses one derived from thego.mod/go.sumpair in its input digest. Stable checksums mean a stable partition, so builds and repos that agree still share their downloads. Partitions accumulate, as named caches are never garbage collected today, but they stay small: an analysis partition holds only.modtext files, and a fetch partition holds the same extracted modules a single shared cache would have held, duplicated only when the recorded checksums for a module actually change. Each partition is a self-contained directory, so pruning by mtime later needs no bookkeeping.The partition also covers the environment that decides where a download comes from and whether it gets checked (
GOPROXY,GOPRIVATE,GOSUMDBand the rest), which matters for modules with nogo.sumentries: those have no checksums to partition by, so builds configuring a different proxy or checksum database must not share downloads.-modcacherwis added via GOFLAGS so the cache can be pruned with plainrm -rf.Checksum verification is unchanged: downloads are still verified against the synthetic per-module go.sum introduced in backend/go: deduplicate third-party module downloads across go.mods #23261, and nothing touches GONOSUMCHECK or GOFLAGS beyond
-modcacherw. The trust model of the shared cache is the same as the default~/go/pkg/modon a developer machine.Remote execution degrades gracefully: without
--remote-execution-append-only-caches-base-paththe cache mount is absent, the fetch falls back to a sandbox-local directory, and behavior is identical to today.ModuleDescriptors.go_mods_digestis removed; it had no consumers.On a 3-go.mod / 206-module reproducer with a cold engine store, a warm module cache eliminates all 103 network downloads (the slowest module went from 8.6 s of network fetch to 3.1 s of local copy) and module-graph analysis (
go list -m) stops hitting the network entirely. Cold wall clock on that reproducer is unchanged within noise, because compile time dominates there and proxy.golang.org is fast from my machine. The practical win is cold CI builds and slow or rate-limited proxies, where today every cold build pays the full download set again.Tests: warm-vs-cold digest equality (which also catches absolute paths leaking into captured digests), named cache populated and writable after a download, module analysis served from the cache, and two builds publishing the same
module@versionwith different contents through a local proxy, which is the collision the partitioning prevents. Release note indocs/notes/2.34.x.md, including the one-time invalidation from the__run_go.shchange.Note: this touches the same
__run_go.shheredoc as #23420 (GOTOOLCHAIN pin); whichever lands second needs a trivial rebase.