perf(cache-proxy): bound, hedge, and adapt peer fetches - #1047
Conversation
…imeout Phase-timing trails from production cold scans showed 100% of block-request serving time inside FetchFromPeers (median 714ms cold / 237ms steady-state per request, vs 8ms for local hits), with 16% of cold requests burning the full 1s has-probe timeout, and 2-block requests costing exactly twice the single-block peer round. Three changes, each aimed at one measured cost: - peerHasTimeout 1s -> 150ms: healthy peers answer probes in single-digit ms while the origin path costs ~200ms, so waiting longer for the fleet's slowest "no" only delays a faster fallback. - Peer fills for a request's missing blocks now run concurrently (bounded at 8) instead of one FetchFromPeers round per block in sequence; phase 1 consumes results in order, so miss-run coalescing and single-flight keys are unchanged. - A wait budget (400ms, shared absolute deadline across the request's fills) hedges stalled fills to the coalesced origin fetch instead of pinning the request on the 30s body-transfer timeout; the fill keeps running in the background and still populates the cache. New counter cache_proxy_peer_fill_hedged_total and Served. field blocks_hedged track how often that happens. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Test Impact PlanDeterministic summary of how this PR changes tests, CI runners, and coverage-risk signals. Summary
Signals
Coverage risk: neutral or increased No coverage-reduction warnings detected. |
bill-ph
left a comment
bill-ph
left a comment
There was a problem hiding this comment.
Reviewed the peer-path changes, including the cache-proxy suite and targeted race tests. The tests pass, but I found one high-priority background-work/resource issue and two hedge correctness/observability issues that are not covered by the new tests.
| } | ||
| f := &peerFill{done: make(chan struct{})} | ||
| fills[idx] = f | ||
| go func() { |
There was a problem hiding this comment.
[P1] This launches one goroutine per missing block; the semaphore only parks excess goroutines and is scoped per request. Because blockCount is bounded only by cache capacity (not maxSpanBlocks), a wide accepted range can retain a very large number of goroutines. Queued jobs also acquire slots after the 400 ms deadline and still call FetchFromPeers for up to 30 seconds even after the origin hedge has populated those keys. Please use a bounded/global worker queue or acquire before spawning, cancel expired jobs, and recheck store.Has(key) after acquiring. A >8-block test should assert no peer GETs start after the deadline.
| _, _, ok := p.peers.FetchFromPeers(key, func(rd io.Reader) (int64, error) { | ||
| return p.store.PutStream(key, rd) | ||
| }) | ||
| filled, hedged := waitFill(f, fillDeadline) |
There was a problem hiding this comment.
[P2] After this times out, the peer fill keeps running but its completion is no longer considered by the request. If the peer lands the block at 450 ms and the origin hedge fails at 500 ms, flushRun returns the origin error even though the requested block is valid locally (pre-PR, the peer would have served it). Before propagating an origin error, recheck the run's block keys/fill handles so either successful side of the hedge can satisfy the request.
| case <-timer.C: | ||
| } | ||
| } | ||
| peerFillHedgedTotal.Inc() |
There was a problem hiding this comment.
[P2] This increments cache_proxy_peer_fill_hedged_total as soon as the deadline wins, but the caller subsequently checks store.Has and may avoid origin entirely if the peer or another request landed the block. That makes this counter and blocks_hedged overstate the documented “blocks fetched from origin” signal. Increment when the block is actually added to an origin miss run, or rename/document the metric as peer-budget expiration.
EDsCODE
changed the title
2 participants
Problem
Peer lookup is useful only while it beats origin. The previous implementation had three failure modes under contention: peer fills were unbounded across requests, a fixed wait delayed origin even when the fleet was degraded, and losing transfers could continue for up to 30 seconds after the response path had already chosen the other source.
Changes
CACHE_PEER_FETCH_MAX_CONCURRENCYdefaults to 32;CACHE_PEER_FETCH_MAX_BYTESdefaults to concurrency × block size. Permit queueing consumes the hedge head start, so overloaded work sheds to the coalesced origin path instead of starting late.Served.lines now include the client address without its ephemeral port. Existing phase-timing fields andblocks_hedgedremain intact.hasprobes retain their 150 ms timeout, block fills remain parallel, and all peer/body operations now honor caller cancellation.Runtime defaults and a peer-path recovery runbook are documented in
cmd/cache-proxy/README.md; the new metric families are documented indocs/metrics.md.Default-cap sweep
An isolated 16/32/64 contention sweep selected 32. A cap of 64 removed synthetic origin fallback but did not show a stable latency improvement and doubled the maximum byte reservation. A cap of 32 materially reduced fallback versus 16 while preserving the safer resource ceiling.
Testing
just test-cache-proxygo test -race, 3 repetitionsjust lintCoverage includes global count/byte admission, queued-deadline shedding, adaptive head starts, shared-flight and per-block cancellation, partial duplicate-byte accounting, mixed peer/origin spans, abrupt breaker opening, bounded recovery before and after the first origin block, pure-peer reset behavior, and client attribution.