perf(ci): stabilize file mtimes for Go test cache#19395
Draft
perf(ci): stabilize file mtimes for Go test cache#19395
Conversation
Replace the //XDef: annotation + status.sh + -X ldflags mechanism with a generated pkg/version/internal/zversion.go file. This stabilizes Go build cache ActionIDs across commits because: - Tests use only the base version tag (e.g. "4.11.x"), stable across commits - Builds use the full git-describe format (e.g. "4.11.x-193-gabcdef") - -buildvcs=false prevents Go from re-stamping VCS info The XDef mechanism injected per-commit values via -X ldflags on every build, which Go treated as different linker inputs, invalidating the entire GOCACHE. Changes: - scripts/go-tool.sh: generate_version_file() replaces XDef parsing - pkg/version/internal/version_data.go: removed XDef annotations - VERSION: new file with base version (4.11.x) - .gitignore: added pkg/version/internal/zversion.go - make/env.mk: TAG falls back to VERSION file when git describe fails - operator/Dockerfile: USER 0 for writable zversion.go + copy VERSION files - tests/roxctl/bats-tests/helpers.bash: VERSION file fallback Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Go's test cache validates inputs by (path, size, mtime) — not content. git checkout sets all file mtimes to the current time, which differs between CI runs, causing test results to never be cached. Fix: set all tracked file mtimes to a fixed date (2001-01-01) after checkout and cache restore. This makes the test cache hit across runs when file content hasn't changed. Measured impact (from PR #19201): - go unit tests: 35m → 8m (4.5x) - go-postgres: 31m → 36s (52x) - sensor-integration: 26m → 2.5m (11x) - Test cache hit rate: 97.6% (681/698 packages) Risk: if a non-.go testdata file changes content but not size between commits, the test cache could return a stale result. This is the same limitation Go's test cache has by design (it explicitly avoids hashing file content for performance). Partially generated by AI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Skipping CI for Draft Pull Request. |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider restricting the
git ls-filesfilter (e.g., to*.goand known testdata paths) to avoid unnecessarily touching large/binary/irrelevant tracked files and potentially interfering with tools that rely on mtimes for non-code assets. - It may be safer to make the fixed timestamp explicitly UTC-aware (e.g.,
TZ=UTC touch -t 200101010000ortouch -d '2001-01-01T00:00:00Z') to avoid any subtle differences across runners with different default timezones.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider restricting the `git ls-files` filter (e.g., to `*.go` and known testdata paths) to avoid unnecessarily touching large/binary/irrelevant tracked files and potentially interfering with tools that rely on mtimes for non-code assets.
- It may be safer to make the fixed timestamp explicitly UTC-aware (e.g., `TZ=UTC touch -t 200101010000` or `touch -d '2001-01-01T00:00:00Z'`) to avoid any subtle differences across runners with different default timezones.
## Individual Comments
### Comment 1
<location path=".github/actions/cache-go-dependencies/action.yaml" line_range="68" />
<code_context>
+ # Risk: if a non-.go testdata file changes content but not size between
+ # commits, the test cache could return a stale result. This is extremely
+ # rare and Go's own test cache has the same inherent limitation.
+ git ls-files -z | xargs -0 touch -t 200101010000
+ shell: bash
</code_context>
<issue_to_address>
**issue (bug_risk):** The fixed timestamp may still differ between runners due to local timezone interpretation.
`touch -t 200101010000` uses the runner’s local timezone. If different runners use different timezones (e.g., self‑hosted vs GitHub‑hosted), the resulting mtimes/epoch values will differ and the Go test cache may still miss across runners. To keep mtimes stable, force UTC (e.g., `TZ=UTC git ls-files -z | xargs -0 touch -t 200101010000`) or use a UTC `-d '2001-01-01T00:00:00Z'` style argument where available.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Images are ready for the commit at f13f6a2. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19395 +/- ##
=======================================
Coverage 49.68% 49.69%
=======================================
Files 2700 2700
Lines 203278 203278
=======================================
+ Hits 100999 101009 +10
+ Misses 94753 94745 -8
+ Partials 7526 7524 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
05f5ad5 to
21752e5
Compare
21752e5 to
f13f6a2
Compare
This was referenced Mar 12, 2026
Contributor
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
3 participants
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.
Description
Go's test result cache has a two-phase lookup:
Phase 1 — Test binary identity: Go computes a test ID from the test
binary's build ActionID, which includes source file content hashes AND
-Xldflags (exec.go:1404: linkflags %q). If the test binary IDchanged since the last run, the cache misses and all tests re-run.
Phase 2 — Test input validation: If the test binary ID matches, Go
checks the files the test accessed at runtime (testdata, config files,
etc.) by
(path, size, mtime)— NOT content. If any mtime changed,the cache misses for that test.
Both phases must hit for test caching to work.
Currently on master, Phase 1 always misses because
-X MainVersionand-X GitShortShachange on every commit, creating a new test binary ID.Go never reaches Phase 2 — tests re-run from scratch every time regardless
of file mtimes.
What each PR fixes
ldflags so the test binary ID stays constant across commits. This also
independently improves build cache reuse (compilation avoidance).
input validation succeeds when files haven't changed.
Test caching requires both PRs together. PR #19394 alone improves build
times but tests still miss at Phase 2 (mtimes differ between CI runs).
This PR alone never reaches Phase 2 (ldflags still change at Phase 1).
When both land, tests pass both phases and achieve 4-52x speedup.
The fix
Set all tracked file mtimes to a fixed date (2001-01-01 UTC) after checkout
and cache restore:
git ls-files -z | xargs -0 touch -t 200101010000Measured results
This PR alone (no test improvement — Phase 1 still misses)
Times are within run-to-run noise — the mtime fix alone has no effect on
tests because
-Xldflags still invalidate the test binary ID at Phase 1.Both PRs combined (PR #19201 data)
Test cache hit rate: 97.6% (681/698 packages cached).
Risk
If a non-
.gotestdata file changes content but not size between commits,the test cache could return a stale result. This is Go's own inherent
limitation — it explicitly avoids hashing file content for test input
validation (
test.go:hashOpen()).Why not combine the PRs?
They are logically independent changes (ldflags vs mtimes) and are easier
to review separately. Either can merge first — whichever lands second
completes the pair and enables the full test cache speedup.
Partially generated by AI.