feat(git): queue clone-shaped upload-pack under load

[ntopousis]

Why

EvalHub launched about 49 full-history git fetches against production Cachew on 2026-09-16 (25× squareup/java, 24× squareup/go-square) within a few minutes. Each Job POSTs /ensure-refs then runs git fetch --no-tags --no-recurse-submodules origin <sha> into a fresh bare repo. java is ~90 GiB; go-square is also large.

Clients then saw mid-stream failures after 4–8 minutes (error: RPC failed; HTTP 503 / fatal: expected 'packfile', and curl 18 transfer closed). Successful fetches took 209–499 s (go-square) and ~760 s (java). Cachew has no application-level cap on concurrent git-upload-pack today: each request spawns git http-backend with no semaphore. The per-repo fetch lock serializes upstream git fetch only. Scheduler concurrency applies to background clone/repack/snapshot jobs, not live pack generation. Inbound Istio is bypassed on port 8080.

What

This adds an optional bounded queue around clone-shaped git-upload-pack (POST bodies with no have lines). Incremental fetches stay unlimited. When at capacity the handler waits up to a configured timeout, then returns 503 with Retry-After before any packfile bytes.

How

New strategy git settings, all optional:

• upload-pack-clone-concurrency (default 0, unlimited globally)
• upload-pack-clone-per-repo-concurrency (default 0, unlimited per repo)
• upload-pack-clone-queue-timeout (default 0, fail immediately when at capacity)
• upload-pack-clone-retry-after (default 30s)

Zero defaults keep current behavior for normal load. Production Cachew (7 replicas, 30 CPU / 96Gi, no HPA) should set limits in squareup/cash-helm-charts charts/cachew/config/cachew.hcl. A starting point:

strategy git {
  upload-pack-clone-concurrency         = 8
  upload-pack-clone-per-repo-concurrency = 2
  upload-pack-clone-queue-timeout       = "60s"
  upload-pack-clone-retry-after         = "30s"
}

Tune from cachew.git.upload_pack_clone_gate_total (result=admitted|rejected|canceled) and cachew.git.upload_pack_clone_queue_wait_seconds. Rejected clones log Rejecting clone-shaped upload-pack due to concurrency limit.

POST .../ensure-refs is unchanged. If the requested commits are missing it still does a synchronous upstream git fetch, serialized per repo by the existing fetch semaphore. Different repos can fetch in parallel on the request path.

Risk

Default limits are unlimited, so existing deployments do not change until operators set the new fields. Once enabled, clone bursts get 503 instead of a mid-stream drop. Git retries 503; clients should honor Retry-After.

Testing

No manual testing.

Bigger picture

A follow-up in squareup/cash-helm-charts is required before production Cachew enforces this. EvalHub should also cap concurrent preparations per repository: production max_running_preparations is 100 in evalhub-production.yaml.

Generated with Cursor

Made with Cursor

[ntopousis]

🤖 Adversarial review (AI agent, comment-only). I built a model of the existing concurrency controls first, then hunted for concrete bugs; repros below were run as temporary tests and removed. go vet clean; go test -race on internal/strategy/git, jobscheduler, gitclone passes with GIT_CONFIG_GLOBAL=/dev/null (the 3 TestSnapshot* failures in a default env are the local insteadOf rewrite, as noted). Note the Go build/test workflow did not run on this fork PR — only DCO/Semgrep/zizmor/title did.

Verdict: merging with limits unset is low-risk (see m1 below for the one behavioural delta). Enabling the recommended prod HCL (8 / 2 / 60s) is not safe yet — B1, M1, M4 need fixing and M2/M3 need the description and root cause revisited.

BLOCKER (for enabling in prod)

B1 — Lazy partial-clone blob fetches are classified as clones. RequestIsClone (repocounts.go:73-115, reused at git.go:398-409) treats any upload-pack POST without have as a clone. Git's promisor lazy fetch runs git -c fetch.negotiationAlgorithm=noop fetch … --filter=blob:none --stdin (upstream promisor-remote.c:47-50): wants + filter blob:none + done, no haves. Prod snapshot-filters puts blob:none on squareup/java, ios-register, cash-ios, so every workstation git log -p / blame / checkout <old> on those repos issues such POSTs. Under per-repo = 2 they compete with 200–760s full clones for 2 slots and will mostly 503 → user-facing git commands fail. Repro: v2 body command=fetch … want <oid> filter blob:none done → RequestIsClone == true. Options: exempt bodies with a filter line; or gate the expensive stage (uploadpack.packObjectsHook wrapper holding the semaphore) instead of request shape; or cat-file --batch-check the wants and exempt non-commit wants.

MAJOR

M1 — Global token held while waiting for a per-repo slot (head-of-line blocking). acquire takes global then repoSem with a shared deadline (uploadpack_limit.go:91-106). With 8/2/60s and 25 java clones: 2 run, 6 more hold global tokens for 60s each waiting on java slots that turn over every ~6 min, so go-square is rejected with 2 free slots while only 2 clones run; 30s client retries keep those tokens pinned. Repro: global=2, per-repo=1, timeout=2s, A running + A queued → B waited 1.9s and was admitted only when A's waiter timed out. Fix: acquire per-repo first, then global.

M2 — "Git retries 503; clients should honor Retry-After" is false; 503 bursts also trip Istio outlier ejection. post_rpc → run_slot fails on anything but OK/REAUTH (remote-curl.c:859-872, 1066-1076) → fatal: expected 'packfile'. Retry-After is honoured only for 429, only on the GET path (http.c:2405-2440, git ≥ 2.50). The chart's DR has outlierDetection consecutive5xxErrors: 5 / 30s; Envoy counts only 5xx for HTTP, so ≥5 consecutive gate rejections from one pod within 30s eject it from the caller's Envoy and push the burst onto the other pods. 429 would not count. Decide deliberately and fix the Risk section.

M3 — Production telemetry does not support the stated root cause, and the gate misses the path that failed. Datadog 2026-09-16 15:00–19:05Z, block.cachew.http_server_request_duration_seconds.count{cachew_http_path_prefix:git}: 503 = 20 (all GET), POST 502 = 66, POST 503 = 0. Logs: Upstream request failed (context canceled) ×66 (= the POST 502s from the proxy ErrorHandler), (unexpected EOF) ×37, Falling back to upstream due to 'not our ref' java ×5 / go-square ×6, Refs stale, forwarding to upstream for java info/refs ×~1300. I.e. the POST failures Cachew recorded were fetches proxied from GitHub (not our ref fallback on a replica missing the SHA — allowAnySHA1InWant=true is set, so likely ensure-refs hit one of 7 pods and the fetch another). GitHub truncating those = curl 18; a POST 503 cannot have come from Cachew. Local pack overload may exist too, but it is asserted from code, not observed. Also the gate sits before the state switch (git.go:405), so it gates/503s the fallback and spool paths, which are GitHub-bound and cheap for Cachew.

M4 — Inspection window fails closed. RequestIsClone scans ≤64 KiB decoded and returns clone if no have appears (repocounts.go:55, 80-85, 111-114). Wants precede haves in v0/v2, so an incremental fetch with >~1300 wants is gated. Repro: 1500 wants + 1 have (75 KB) → true. Fail open when the window is exhausted without done/flush.

MINOR

• m1 Disabled path isn't byte-for-byte: recordUploadPackCloneGate emits cachew.git.upload_pack_clone_gate_total{result=admitted,repository=<url>} on every clone-shaped POST even when enabled() is false (uploadpack_limit.go:152-153), on an unbounded URL key space. Skip when disabled; reconsider the repository label.
• m2 perRepo sync.Map (:71-78) is never pruned.
• m3 IncrementClone runs before the gate (git.go:401-405): rejections and retries inflate the histogram.
• m4 No validation: negative concurrency ⇒ silently unlimited; negative queue-timeout ⇒ fail-fast; queue-timeout without concurrency ⇒ silent no-op.
• m5 HCL numbers are per-pod (7× ⇒ 56 / 14 cluster-wide; locality LB can concentrate one AZ on one pod). 60s queue vs 200–760s clones almost never admits — it just adds 60s (and a pinned global token, M1) before the same 503. Use 0 or minutes.
• m6 Cold-mirror regression: identical concurrent clones the spool was built to coalesce are now capped, and a rejected StateEmpty request never submits the background clone (git.go:424).
• m7 Tests: no HTTP-level proof the slot is held for the whole stream/released on return; no HTTP-level timeout/cancel test; TestIncrementalUploadPackNotGated starts a real clone job against 127.0.0.1:443 with cache=nil → recovered nil-pointer panic in snapshot.Restore in the test log.
• m8 The gate is cooperative: a bogus have <sha> bypasses it. Fine in-mesh, but document it.

NIT

• rejectCloneUploadPack re-derives the 30s default already normalised in newUploadPackLimiter; default: arm in gateCloneUploadPack is unreachable; gate key is case-preserving (squareup/Java gets its own budget — consistent with mirror identity); queue wait is charged against the 30m WriteTimeout.

Checked and correct

Release on every path (defer release() after admission, no token taken on cancel, global returned when per-repo fails, ctx.Err() checked before waits); 503 is written before git http-backend is spawned and before any bytes; body replay after inspection keeps ContentLength correct; channel semaphores wake FIFO; unlimited default adds no body read or log lines.

[ntopousis]

🤖 M1 (major): the global token acquired at L92 is held while this waits for the per-repo slot, sharing the same deadline. A hot repo's waiters therefore pin global tokens: with 8/2/60s and 25 java clones, 6 java waiters occupy 6/8 global slots for 60s at a time while only 2 clones run, and go-square (2 free slots) is rejected. Repro (global=2, per-repo=1, timeout=2s; A running + A queued): B waited 1.9s and was admitted only when A's waiter timed out. Acquire per-repo first, then global — a per-repo waiter then blocks only its own repo, which needs a global slot anyway. TestUploadPackLimiterReleasesGlobalIfPerRepoRejects covers release-after-reject, not hold-while-waiting.

[ntopousis]

🤖 m1 (minor): this records cachew.git.upload_pack_clone_gate_total{result=admitted, repository=<upstream URL>} on every clone-shaped POST even when the limiter is disabled (enabled() false), so the "defaults are byte-for-byte unchanged" claim isn't quite true: every deployment gets a new per-repository series on an unbounded key space (any POST /git/<host>/<x>/git-upload-pack with no body creates one; the Datadog openmetrics check scrapes .*). Return early when !l.enabled() before touching metrics, and consider dropping/bucketing the repository attribute. Also IncrementClone at git.go:401 runs before the gate, so rejections and client retries inflate the clone histogram.

[ntopousis]

🤖 M2 (major): the PR description says "Git retries 503; clients should honor Retry-After" — upstream git does not. post_rpc → run_slot treats anything but HTTP_OK/HTTP_REAUTH as fatal (remote-curl.c:859-872, 1066-1076 → error: RPC failed; HTTP 503 / fatal: expected 'packfile'). Retry-After is only honoured for 429, and only in http_request_recoverable (GET path, http.c:2405-2440, git ≥ 2.50). So the client (EvalHub) must implement retry itself; the description should say so. Separately, the chart's DestinationRule has outlierDetection { consecutive5xxErrors: 5, interval: 30s } and Envoy counts only 5xx for HTTP upstreams: ≥5 consecutive rejections from one pod inside 30s eject that pod from the caller's Envoy for 30s and push the burst onto the remaining pods. A 429 would not count toward ejection. Worth choosing deliberately and documenting.

[ntopousis]

🤖 B1 (blocker for enabling in prod) + M3/M4: RequestIsClone treats any upload-pack POST without have as a clone. Git's promisor lazy fetch runs git -c fetch.negotiationAlgorithm=noop fetch … --filter=blob:none --stdin (upstream promisor-remote.c:47-50), i.e. wants + filter blob:none + done and no haves. Prod snapshot-filters applies blob:none to squareup/java, ios-register, cash-ios, so every workstation git log -p / blame / checkout <old> on those repos issues such POSTs; under per-repo = 2 they compete with 200–760s full clones and will mostly 503, failing the user's git command. Repro: v2 body command=fetch … want <oid> filter blob:none done → true. Also (M4) the detector fails closed when the 64 KiB inspect window is exhausted without seeing have — 1500 wants + 1 have (75 KB) → true. Options: exempt bodies containing a filter line, or gate the expensive stage via uploadpack.packObjectsHook (sees the resolved want set), or cat-file --batch-check the wants and exempt non-commit wants; and fail open at the inspect limit.

Placement (M3/m6): this runs before the switch state, so it also gates — and 503s — the not our ref fallback and the StateEmpty/StateCloning spool/forward paths, which are GitHub-bound and cheap for Cachew, and a rejected StateEmpty request never reaches the scheduler.Submit("clone") at L424. Production telemetry for the 2026-09-16 window shows Cachew emitted 0 POST 503s on /git (503s: 20, all GET; POST 502s: 66 = Upstream request failed (context canceled) ×66, plus unexpected EOF ×37 and Falling back to upstream due to 'not our ref' for java/go-square ×11) — i.e. the failures Cachew recorded were upstream-proxied fetches, not local pack generation. Worth verifying the root cause with server-side status metrics before tuning the HCL.

[ntopousis]

🤖 m7 (minor): this test uses host 127.0.0.1 with cache = nil, so the repo is StateEmpty, handleGitRequest submits a real background clone job, and startClone → tryRestoreSnapshot → snapshot.Restore(nil cache) nil-pointer panics (recovered by the scheduler; visible in the -v log), then the spool path dials 127.0.0.1:443. It still asserts "not 503", but the coverage is incidental. Consider a ready local mirror (as integration_test.go does). There is also no HTTP-level test that the slot is held for the whole stream and released when the handler returns, nor a queue-timeout / client-cancel test at the HTTP layer.