* Return the stored artifact from storeArtifact, not a reader
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.
* Coalesce concurrent cache misses
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.
* Normalize digest case in the coalescing 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.
* Make the panic coalescing test deterministic
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.
* Recheck the cache before running a shared fetch
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.
* Lock the mock fetcher's bookkeeping
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.
* Wait for the leader's fetch instead of sleeping
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.
montehurd