Speed up mirror checkouts: 100ms lock polling and snapshot-based commit verification - #4210
Draft
lox wants to merge 3 commits into
Draft
Speed up mirror checkouts: 100ms lock polling and snapshot-based commit verification#4210lox wants to merge 3 commits into
lox wants to merge 3 commits into
Conversation
LockFile retried a held lock every second. The mirror clone/update locks (LockFile's only callers) are typically held for a few milliseconds, so the poll interval — not the lock — serialized same-host parallel jobs: with N jobs racing for one mirror, the last waited ~N seconds. Retry every 100ms. Waiting also logged two lines per poll. Log once when waiting starts, every 30s while still blocked, and once with the elapsed time on acquisition, and reuse a single timer instead of deferring a stop per retry. Benchmark (300ms-latency origin, 72MiB repo, warm mirror): 8 parallel checkout jobs' makespan drops from 8.4s to 1.4s. Amp-Thread-ID: https://ampcode.com/threads/T-019fc68c-277d-7365-8a6c-7c3b9fcbe8fb Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
…p is fresh Strict commit verification always fetched the canonical branch tip, even when this job's mirror update had just fetched that same tip into the mirror moments earlier. Track that fact — mirrorReference.branchTipFresh, set when the mirror was freshly cloned from canonical or the warm-path update fetched the build branch from canonical — and answer the ancestry check from the immutable per-job snapshot instead of the network. Only a definitive local positive short-circuits: the snapshot must have refs/heads/<branch> resolving to a commit, and the build commit must be an ancestor of it. Anything else (missing ref, negative ancestry, command failure) falls through to the canonical fetch-based check unchanged. Paths that cannot vouch for the tip never claim freshness: fetch skipped because the commit was already present, refs populated from a remote mirror, --git-mirrors-skip-update, submodule mirrors, and tag builds. For the freshness claim to be sound, the warm-path update now fetches the branch by explicit forced refspec (+refs/heads/X:refs/heads/X) instead of its bare name. A bare name resolves refs/tags/ first, so a tag sharing the branch's name was fetched instead, leaving the mirror's branch ref stale — previously a silent staleness bug, now also a verification-soundness requirement. The refspec is passed unsplit (quotes are legal in ref names), via a new RawRefSpecs field on gitFetchArgs. Tag builds and pre-qualified branch names keep the historical bare-name fetch. Benchmark (300ms-latency origin, 72MiB repo): steady-state checkout (new commit per build) 1.9s -> 0.7s and 4 -> 1 origin connections; re-running a commit already in the mirror keeps the tiny canonical verification fetch by design. Amp-Thread-ID: https://ampcode.com/threads/T-019fc68c-277d-7365-8a6c-7c3b9fcbe8fb Co-authored-by: Lachlan Donald <lachlan@buildkite.com>
t.TempDir() embeds this file's long test names in GitMirrorsPath, so the deepest snapshot paths (mirrors path + snapshots + dirForRepository(repo URL) + a pack file name) exceeded 260 characters on the Windows agents, failing the mirror snapshot clone with a bare exit status 128. Use a short MkdirTemp dir with best-effort cleanup instead, following tracingTempDir's precedent. Amp-Thread-ID: https://ampcode.com/threads/T-019fc68c-277d-7365-8a6c-7c3b9fcbe8fb Co-authored-by: Lachlan Donald <lachlan@buildkite.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
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.
Stacked on #4201 (which stacks on #4198). Two independent checkout-latency changes for mirror-enabled agents, benchmarked with real
bootstrapcheckout jobs against a 72 MiB repo behind a 300 ms-connect-latency proxy.1. Poll mirror locks every 100 ms
Shell.LockFileretried a held lock every second, but the mirror clone/update locks (its only callers) are typically held for single-digit milliseconds. The poll interval — not the lock — serialized same-host parallel jobs into ~N seconds for N jobs. Now 100 ms, with quieter logging: one line when waiting starts, one every 30 s while blocked, one with elapsed time on acquisition. The lock protocol itself (flock,path+"f") is unchanged, so mixed-fleet/shared-filesystem behavior is preserved.2. Answer strict commit verification from the per-job snapshot when it can
Strict verification always fetched the canonical branch tip — even when this job's mirror update had just fetched exactly that tip.
mirrorReferencenow recordsbranchTipFresh: set only when this job syncedrefs/heads/<branch>from the canonical repository (fresh canonical mirror clone, or the warm-path branch fetch). When set, verification runs the ancestry check against the immutable per-job snapshot with no network. Only a definitive local positive short-circuits; any other outcome (missing ref, negative, command failure) falls through to the unchanged canonical fetch-based check.Paths that cannot vouch for the tip never claim freshness and keep full canonical verification:
--git-mirrors-skip-update, submodule mirrors, tag buildsSo the security posture of
strictis unchanged; steady-state CI (a new commit triggers the build) just stops paying a canonical round-trip for information it already fetched. Re-running an old commit still makes the small canonical verification fetch, by design.Soundness fix the freshness claim required
The warm-path mirror update fetched the branch by bare name. Git resolves a bare name against
refs/tags/beforerefs/heads/, so a tag sharing the branch's name was fetched instead and the mirror's branch ref silently stayed stale (reproducible on main today). The update now fetches+refs/heads/X:refs/heads/Xexplicitly, passed unsplit via a newRawRefSpecsfield ongitFetchArgsbecause quotes are legal in ref names andshellwords.Splitwould mangle them. Tag builds (whereBUILDKITE_BRANCHmay name the tag) and pre-qualifiedrefs/...branch values keep the historical bare-name fetch and never claim freshness.Review notes
TestUpdateGitMirrorBranchTipFreshDespiteSameNamedTag, which fails against the bare-name fetch.verifyCommitAgainstSnapshot.Validation:
go test ./...(37 packages ok),golangci-lint runclean,gofumpt -extraclean. New tests:TestVerifyCommitAgainstSnapshot,TestVerifyCommitSnapshotShortCircuitAndFallback,TestUpdateGitMirrorBranchTipFreshDespiteSameNamedTag,TestUpdateGitMirrorNoFreshnessForTagBuilds.