feat: Add incremental git pull-through - #401
Open
nsextonTT wants to merge 12 commits into
Open
Conversation
nsextonTT
force-pushed
the
399-incremental-pullthrough
branch
2 times, most recently
from
August 2, 2026 06:28
53ecf4a to
89d53df
Compare
Callers that need to know whether a mirror already holds a set of objects have no way to ask without shelling out per object, which is prohibitive for a fetch request naming dozens of wants. Add MissingObjects, which feeds every OID to a single `cat-file --batch-check` process and returns those the mirror lacks, plus ValidateOID so untrusted input never reaches git's stdin. Co-Authored-By: BrAIn <brain@tenstorrent.com> Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
Deciding whether a fetch can be served from the mirror means knowing which objects the client asked for, and the existing clone heuristic only reports whether a body looks like a clone at all. Parse the pkt-line stream properly: collect wants from both protocol v0 and v2 bodies, and flag the shapes that rule out local service, namely haves, ls-refs and want-ref. Bodies above a fixed limit are rejected rather than parsed, so a hostile client cannot make the proxy buffer without bound. Co-Authored-By: BrAIn <brain@tenstorrent.com> Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
A caller that coalesces onto a concurrent fetch cannot tell whether its objects actually arrived: the semaphore holder may have started before the caller's refs existed upstream. Today that is indistinguishable from a successful fetch, so the caller has to either trust it or always fetch again. Return a fetched flag from the coalescing path so callers can retry only when they piggy-backed, and add FetchVerifiedIfAbsent, which takes the semaphore and then skips the network call entirely if the objects it needs are already present. Co-Authored-By: BrAIn <brain@tenstorrent.com> Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
The ref-staleness check runs on every request, and its caching is unsound in three ways. It marks the verdict valid before ls-remote returns, so concurrent callers can read "refs up to date" from a check that is still in flight and skip a fetch they needed. It caches nothing on a stale or failed outcome, so a mirror that is behind upstream, or an upstream that is down, re-runs ls-remote for every single request. And nothing coalesces concurrent checks, so a burst of requests each opens its own connection to upstream. Run one check at a time and have later callers wait on it and reuse its recorded verdict, caching stale and failed outcomes as well as successful ones. Failures use a shorter window than RefCheckInterval so recovery is still noticed promptly. The leader detaches from its caller's context, otherwise a client disconnecting mid-check would fail every waiter. A successful fetch drops the cached verdict. Also record when a fetch is attempted, not only when it succeeds, so NeedsFetch's cooldown applies during an upstream outage instead of letting every request pay the full fetch timeout. checkRefsStale now swallows and debug-logs its error instead of returning it: no caller could do anything but ignore it, and warning per request during an outage is noise. Co-Authored-By: BrAIn <brain@tenstorrent.com> Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
Operators need to see how often a fetch is served from the mirror versus forwarded upstream, and how long the mirror spends catching up when it has to. Neither is derivable from the existing counters, and fetches issued on the request path are indistinguishable from background ones. Add counters for incremental serve outcomes and for requests eligible for incremental service, plus a histogram of catch-up fetch duration, and label every git operation with the trigger that caused it. Co-Authored-By: BrAIn <brain@tenstorrent.com> Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
A client fetching a branch that is a few commits ahead of the mirror gets the entire response proxied from upstream, even though the mirror already holds all but the last handful of objects. Every such fetch costs a full upstream pack. Instead, check which of the requested objects the mirror is missing and fetch just those into the mirror, then let the local upload-pack serve the request. Fetching runs in two stages under one deadline: a coalescing attempt that can piggy-back on a concurrent fetch, and, if that did not run, a verified attempt that re-checks for the objects before hitting upstream. Anything that goes wrong (fetch failure, objects still absent, client disconnect) falls back to forwarding upstream, so the feature can only add a fast path, never break one. want-ref bodies are always forwarded: the mirror does not advertise ref-in-want, and resolving ref names locally could serve a stale tip that regresses the client's FETCH_HEAD. A failed catch-up fetch queues a forced background fetch, bypassing the cooldown that the failed attempt itself just satisfied, so later requests do not each pay the synchronous timeout. Controlled by incremental-pullthrough, on by default. Co-Authored-By: BrAIn <brain@tenstorrent.com> Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
Serving a fetch from the mirror requires buffering the request body in order to parse it, which lets a client force the proxy to hold an arbitrary amount of memory per request. Cap upload-pack bodies at twice the parse limit and answer 413 above that. The cap sits behind the same switch as the feature: with incremental pull-through disabled the body is streamed straight through, so a legitimately large one must not start failing. Co-Authored-By: BrAIn <brain@tenstorrent.com> Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
The Git section lists the techniques cachew uses to reduce upstream load, and the config example shows the knobs worth setting. Both need the new technique and its kill switch. Co-Authored-By: BrAIn <brain@tenstorrent.com> Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
TestIntegrationGitCloneViaProxy and TestIntegrationGitFetchViaProxy clone a real repo from github.com. CI runners do not have reliable network access to the public internet, so these tests should only run when invoked locally with -tags=integration. Co-Authored-By: BrAIn <brain@tenstorrent.com> Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
Judging whether incremental pull-through is paying off means reading several of the new metrics together: hit rate, fallback reasons, fetch latency, upstream requests avoided. Add a script that queries Prometheus and prints them as one report, so operators do not have to reconstruct the PromQL each time. Co-Authored-By: BrAIn <brain@tenstorrent.com> Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
nsextonTT
force-pushed
the
399-incremental-pullthrough
branch
from
August 2, 2026 06:29
89d53df to
8ff8d79
Compare
nsextonTT
changed the title
nsextonTT
marked this pull request as ready for review
August 3, 2026 22:47
nsextonTT
requested review from
jrobotham-square
and removed request for
a team
August 3, 2026 22:47
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ff8d79a99
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
checkRefsStale swallowed EnsureRefsUpToDate errors and logged them
internally, so callers had no control over how ref-check failures
were reported or handled. Return the error instead and move the
fail-open ("treat as not stale") decision and its debug log to the
call site in serveReadyRepo, per AGENTS.md's guidance that functions
should return errors rather than log them internally.
Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a675584ad8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
executeFetch stamps lastFetchAttempt on entry, not on completion. So NeedsFetch cannot tell an in-progress fetch from one that just finished, and ensureWantsAvailable's cooldown check short-circuited to fallback_missing (forward upstream) even while the mirror's own background fetch was still running and about to satisfy the want. Add Repository.fetchActive, an atomic flag set for the duration of each executeFetch call, and FetchInFlight to read it. Skip the cooldown short-circuit when a fetch is in flight; fall through to the existing coalescing wait instead, which already blocks on fetchSem and re-checks MissingObjects once the fetch completes. fetchActive is separate from fetchSem. fetchSem is also held by non-fetch operations (e.g. WithFetchExclusion's snapshot tar), so "holds fetchSem" is not the same signal as "is fetching." An earlier version of this fix peeked at fetchSem directly and hit that exact problem. Adds CooldownSkippedWhenFetchInFlight: starts a real fetch, stalls it mid-flight, and checks that a concurrent EnsureWantsAvailable call waits for it and returns "fetched" instead of "fallback_missing". Confirmed it fails against the pre-fix guard. Known tradeoff, not fixed here: a request landing while a long catch-up fetch (FetchLenient) is in flight now waits out part of IncrementalFetchTimeout/2 instead of getting an instant fallback_missing. Same budget every other incremental request already uses, just reachable in one more case. Addresses PR review comment: block#401 (comment) Signed-off-by: nsextonTT <216619413+nsextonTT@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ecfff7789
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+871
to
+872
| if !repo.NeedsFetch(s.cloneManager.Config().RefCheckInterval) { | ||
| return |
There was a problem hiding this comment.
With the new cached stale-ref verdict, a burst of info/refs requests after one stale check can all reach submitFetch without another ls-remote. Each request passes this pre-enqueue cooldown check until the first queued job actually starts and stamps lastFetchAttempt; RootScheduler.Submit appends every job and only serializes by queue, so the burst can queue many redundant git fetch jobs that will run one after another even after the first catch-up succeeds. Please track a pending normal fetch, or mark the attempt when enqueueing, before relying on this cooldown.
Useful? React with 👍 / 👎.
| // trips NeedsFetch's cooldown and serial requests do not each pay a full | ||
| // IncrementalFetchTimeout / FetchTimeout during an upstream outage. | ||
| r.mu.Lock() | ||
| r.lastFetchAttempt = time.Now() |
There was a problem hiding this comment.
When an upstream outage or slow server makes git fetch run longer than RefCheckInterval (defaults are 5m/2m fetch budgets versus a 10s ref-check interval), stamping lastFetchAttempt only at the start means the cooldown has already expired by the time the fetch returns an error. The next request with missing wants will immediately start another synchronous fetch instead of backing off, so the outage protection only works for fast failures. Update the timestamp on failure/return as well so the cooldown is measured after slow failed attempts.
Useful? React with 👍 / 👎.
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.
Closes #399. Requesting Comments.
Notes for reviewers
Summary
Adds incremental pull-through for git mirrors: when a client fetch can be satisfied by objects already in the local mirror, serve it directly instead of falling back to a full upstream fetch. When it cannot, fetch the missing delta, coalescing with any concurrent fetch and verifying the objects arrived.
internal/gitclone/objects.go)Each commit builds and passes its package's tests independently.
New metrics
Three new metrics from incremental pull-through, plus a new label on an existing one.
cachew_git_incremental_serves_totalcachew_git_incremental_eligible_totalcachew_git_incremental_fetch_duration_secondscachew_git_operation_duration_secondsandcachew_git_operations_totalalso gained a newtriggerlabel (backgroundorincremental), so fetches this feature triggers can be told apart from the existing scheduled background fetches.Benchmark: tenstorrent/tt-metal through cachew, cold vs. hot mirror
Six clones on one sandbox host, using this branch's cachewd with incremental pull-through on. The client ran
git clone [--depth N] http://127.0.0.1:<port>/git/github.com/tenstorrent/tt-metal <dir>and measured wall time.Cold: the repo has never been mirrored, so the request forwards to upstream while a mirror builds in the background.
Hot: the mirror is already built and the pack comes from it. Each run was checked against the log to confirm which path it actually took.
--depth 1--depth 1--depth 500--depth 500For comparison, cloning tt-metal directly from GitHub on the same host took 56.0 s and 60.5 s (average 58.3 s). So a hot full clone through cachew is a bit faster than cloning directly, and a cold full clone is about 28% slower.
Resulting client repos, for comparison:
.git)--depth 500--depth 1The mirror itself is 2.5 G with 37,103 refs, more than a client clone gets since the mirror also keeps the full
refs/pull/*namespace.Caveats