Conversation
storeArtifact returned a CacheResult holding an open file handle. A handle has one read position, so it can only ever serve a single caller, which is what blocks sharing one fetch between concurrent requests. Return the artifact and its storage path instead, and let each caller open its own reader through openStoredArtifact. Threading that type through fetchAndCache, fetchAndCacheFromURL and their error paths is mechanical; behaviour is unchanged.
A cache miss went from checkCache straight to an upstream fetch with nothing tracking in-flight work, so N concurrent requests for one uncached artifact produced N upstream fetches and N stores to the same key. That is the CI shape: parallel jobs installing overlapping dependencies against a cold cache. The duplicate stores also fail requests, racing fileblob's per-key ".attrs" sidecar into a partial read served as a 502. Over 12 runs of 8 simultaneous requests for one uncached tarball, against bb2205a: before, 8 fetches per run and 12 of 96 responses were 502; after, 1 fetch per run and none failed. Route both miss paths through a shared in-flight map keyed on the artifact, including the download URL and upstream-declared hash so callers expecting different bytes never share a fetch. singleflight does not fit: Do gives waiters no way to leave, while DoChan lets the caller running the fetch abandon it, breaking storeArtifact's scan-on-disconnect contract. Deciding roles under a mutex gives both behaviours. The fetch runs on the first caller's context and is seen through; waiters leave when their own clients do. This removes the sidecar trigger on this path. The race is in fileblob and three writers bypass this path entirely, so it is fixed separately. Fewer failures now reach the circuit breaker, so it trips later. Sixteen concurrent callers against real file:// storage fail 10 of 10 runs on main and pass 10 of 10 here. Other tests pin key discrimination, failure propagation, resolver-path coalescing, per-caller readers, waiter cancellation, key release and panic safety. allocs/op is unchanged. mockStorage gains a mutex so concurrent tests can use it.
There was a problem hiding this comment.
🟡 Changes recommended
Critical concurrency, cache invalidation, request-identity, and test race issues remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds in-process coalescing for concurrent artifact cache misses, with storage refactoring and expanded concurrency tests.
Changes:
- Coordinates concurrent resolver and URL-based fetches.
- Returns reusable artifact metadata for independent readers.
- Adds concurrency, failure, cancellation, and file-storage tests.
File summaries
| File | Summary |
|---|---|
internal/handler/handler.go |
Implements fetch coalescing and storage refactoring. |
internal/handler/handler_test.go |
Makes mock storage concurrency-safe. |
internal/handler/coalesce_test.go |
Tests concurrent cache-miss behavior. |
internal/handler/coalesce_semantics_test.go |
Tests coalescing semantics and cancellation. |
Review details
Suppressed comments (4)
internal/handler/coalesce_semantics_test.go:421
- This sleep does not guarantee that the first goroutine has registered the in-flight entry. Under scheduler or I/O delay, the second call can become the leader; its
panickingFetcherpanic is not recovered, so the test can crash instead of testing waiter cleanup. Synchronize on an explicit fetch-start signal before launching the waiter.
time.Sleep(fetchHoldTime / 2) // join while the doomed fetch is still running
internal/handler/handler.go:236
- The cache lookup above happens before this key is registered. A request can observe the miss, be descheduled while the first caller commits and removes
inFlight, then arrive here and become a new leader without rechecking the cache, causing a second upstream fetch for the same concurrent miss. Recheck the cache after acquiring the leader role, or coordinate the cache lookup and registration atomically.
key := artifactCoalesceKey(versionPURL, filename, "", "")
return p.coalesceFetch(ctx, key, func(fetchCtx context.Context) (artifacts.Artifact, string, error) {
return p.fetchAndCache(fetchCtx, ecosystem, name, version, filename, pkgPURL, versionPURL)
})
internal/handler/handler.go:1218
- As in the resolver path, this call is reached only after a cache miss was checked. A caller that completed that check before the shared fetch committed can arrive after the entry is deleted and start a duplicate fetch instead of observing the newly committed row. Recheck the cache after acquiring the leader role, or make cache miss registration atomic with the lookup.
key := artifactCoalesceKey(versionPURL, filename, downloadURL, upstreamHash)
return p.coalesceFetch(ctx, key, func(fetchCtx context.Context) (artifacts.Artifact, string, error) {
return p.fetchAndCacheFromURL(fetchCtx, ecosystem, name, version, filename, pkgPURL, versionPURL, downloadURL, headers, upstreamHash)
})
internal/handler/handler.go:1215
artifactHashMatchestreats SHA-256 hex case-insensitively, but this key usesupstreamHashverbatim. Callers that provide the same digest with different casing therefore bypass coalescing and can duplicate the upstream fetch and storage write this change is intended to eliminate. Normalize the hash before constructing the key.
key := artifactCoalesceKey(versionPURL, filename, downloadURL, upstreamHash)
- Files reviewed: 4/4 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // it, and the key is released so a later request retries. | ||
| func TestCoalesce_FailedFetchReachesEveryCallerAndIsRetriable(t *testing.T) { | ||
| const callers = 8 | ||
| proxy, _, _, fetcher := setupTestProxy(t) |
There was a problem hiding this comment.
Fixed in 8634c00: the mock locks the fields it records.
| func artifactCoalesceKey(versionPURL, filename, downloadURL, upstreamHash string) string { | ||
| return strings.Join([]string{versionPURL, filename, downloadURL, upstreamHash}, "\x00") |
There was a problem hiding this comment.
This predates the PR. storeArtifact has always written every fetch to storage.ArtifactPath(ecosystem, name, version, filename), whatever URL or hash the request carried, so two such requests could already overwrite and open the same object concurrently. Coalescing removes that for identical requests and leaves the rest as it was. Widening the key to the storage path would make callers expecting different bytes share one result, which the key must not do. The proper fix is a storage path that carries the digest, and I'd rather do that as a follow-up than grow this PR. Happy to open it.
| key := artifactCoalesceKey(versionPURL, filename, downloadURL, upstreamHash) | ||
| return p.coalesceFetch(ctx, key, func(fetchCtx context.Context) (artifacts.Artifact, string, error) { | ||
| return p.fetchAndCacheFromURL(fetchCtx, ecosystem, name, version, filename, pkgPURL, versionPURL, downloadURL, headers, upstreamHash) |
There was a problem hiding this comment.
Agreed the window exists, and it predates coalescing: two requests that both saw a stale row already both discarded, and a slow discard could already delete the other's fresh commit. What coalescing changes is that the second discard can now hit bytes several waiters are about to open. Moving the invalidation under the shared fetch closes it: the leader would compare the cached digest with upstream's after taking the key, then discard, fetch and commit while holding it. That is a real change to the digest-aware entry point, so I'd rather do it as a follow-up PR than here, but can fold it in if you prefer. Which do you want?
| key := artifactCoalesceKey(versionPURL, filename, downloadURL, upstreamHash) | ||
| return p.coalesceFetch(ctx, key, func(fetchCtx context.Context) (artifacts.Artifact, string, error) { | ||
| return p.fetchAndCacheFromURL(fetchCtx, ecosystem, name, version, filename, pkgPURL, versionPURL, downloadURL, headers, upstreamHash) |
There was a problem hiding this comment.
The cache has the same identity: rows and storage paths are keyed by ecosystem, name, version and filename, never by headers. Two callers with different headers already get whichever bytes were cached first, on every request after the first. Keying the fetch by headers would only split the cold-cache fetch and then serve the second caller from the first caller's bytes anyway. The coalescing key matches what the cache will serve. Caching per-header variants separately would be a change to the cache identity, which this PR leaves alone.
| leaderDone <- err | ||
| }() | ||
|
|
||
| time.Sleep(200 * time.Millisecond) // let the leader take the key |
artifactHashMatches compares digests with strings.EqualFold, but the coalescing key used the hash verbatim. The same digest in two casings produced two keys, so two callers for one artifact each ran their own upstream fetch and store, which is what the coalescing is meant to prevent.
The test timed the second caller's arrival with a sleep, so which caller became the leader was left to the scheduler. When it lost that race the second caller ran the fetch itself, and its panic was not recovered, so the test binary died instead of the test failing. Whether a caller has reached the wait is not observable from outside: it runs a cache lookup against the database first, so releasing the leader on a timer races that query. Drive coalesceFetch directly and hold the shared entry instead, which removes the timing entirely. The panicking fetcher is no longer needed.
A caller checks the cache before it reaches coalesceFetch, so a fetch that commits in that gap is invisible to it. Arriving after the sharing entry is gone, it became a new leader and fetched, stored and scanned an artifact the cache already held. The leader now rechecks the committed record first. It serves that record only if its bytes still open, because a record can outlive them, and refetching is the recovery the cache lookup already makes for that case. Waiters are unaffected: the record fills the same shared value a fetch would, and each caller opens its own reader from it. The recheck is the leader's alone. A waiter has a fetch in flight to wait on, and rechecking would race it for no gain.
Coalescing tests call the handler from many goroutines. The key keeps the fetch itself serialized, but the mock should not lean on that: it now locks the fields it records, so any concurrency the handler applies is safe under the race detector.
The canceled-waiter test slept 200ms and assumed the leader had taken the key by then. On a slow scheduler the canceled call could become the leader and the test would no longer cover waiter cancellation. The fetcher now signals when its first fetch begins, which happens only once the key is held.
The problem
A cache miss goes from
checkCachestraight to an upstream fetch with nothing tracking in-flight work, so N concurrent requests for one uncached artifact produce N upstream fetches and N stores to the same storage key. That is the CI shape: parallel jobs installing overlapping dependencies against a cold cache.The duplicate stores also fail requests, racing fileblob's per-key
.attrssidecar into a partial read served as a 502.Over 12 runs of 8 simultaneous requests for one uncached tarball, against
bb2205a:The fix
Both miss paths route through a shared in-flight map keyed on the artifact. The key includes the download URL and upstream-declared hash, so callers expecting different bytes never share a fetch.
x/sync/singleflightwould be the obvious tool and is already used for ECR tokens, but neither mode fits:Dogives waiters no way to leave, whileDoChanlets the caller running the fetch abandon it on its own cancellation, which breaks the scan-on-disconnect contract instoreArtifact. Deciding the roles under a mutex makes both behaviours available.Two commits. The first returns the stored artifact from
storeArtifactinstead of an open reader, a mechanical refactor with no behaviour change. The second adds the coalescing. The reproduction tests fail at bothmainand the refactor commit, so a bisect lands on the right one.Scope
cacheMetadataBlob,storeContainerMetadataand the Gradle build cachePUTreach storage without coming through here.fileblob's and is addressed separately in Stop writing fileblob's .attrs sidecar #328.Green on ubuntu, macOS and Windows.