mirror of
https://github.com/git-pkgs/proxy.git
synced 2026-09-16 07:42:05 -04:00
* 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.
185 lines
5.2 KiB
Go
185 lines
5.2 KiB
Go
package handler
|
|
|
|
import (
|
|
"bytes"
|
|
"context"
|
|
"io"
|
|
"log/slog"
|
|
"net/http"
|
|
"path/filepath"
|
|
"strings"
|
|
"sync"
|
|
"sync/atomic"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/git-pkgs/proxy/internal/database"
|
|
"github.com/git-pkgs/proxy/internal/storage"
|
|
"github.com/git-pkgs/registries/fetch"
|
|
)
|
|
|
|
// fetchHoldTime holds each stub fetch open long enough that concurrent callers
|
|
// reliably overlap inside it. The exact value is not significant.
|
|
const fetchHoldTime = 50 * time.Millisecond
|
|
|
|
// countingFetcher counts upstream fetches and holds each one open.
|
|
type countingFetcher struct {
|
|
calls atomic.Int64
|
|
content string
|
|
delay time.Duration
|
|
|
|
// entered, if set, is closed when the first fetch begins. A test can wait
|
|
// on it to know the leader holds the key, rather than guessing with a
|
|
// sleep.
|
|
entered chan struct{}
|
|
enterOnce sync.Once
|
|
}
|
|
|
|
func (f *countingFetcher) Fetch(ctx context.Context, url string) (*fetch.Artifact, error) {
|
|
return f.FetchWithHeaders(ctx, url, nil)
|
|
}
|
|
|
|
func (f *countingFetcher) FetchWithHeaders(_ context.Context, _ string, _ http.Header) (*fetch.Artifact, error) {
|
|
f.calls.Add(1)
|
|
if f.entered != nil {
|
|
f.enterOnce.Do(func() { close(f.entered) })
|
|
}
|
|
time.Sleep(f.delay)
|
|
return &fetch.Artifact{
|
|
Body: io.NopCloser(strings.NewReader(f.content)),
|
|
ContentType: "application/gzip",
|
|
}, nil
|
|
}
|
|
|
|
func (f *countingFetcher) Head(context.Context, string) (int64, string, error) {
|
|
return 0, "", nil
|
|
}
|
|
|
|
// TestGetOrFetchArtifactFromURL_ConcurrentMissesCoalesce asserts that N
|
|
// simultaneous misses for one artifact produce a single upstream fetch. That is
|
|
// the CI shape: parallel jobs installing overlapping dependencies cold.
|
|
func TestGetOrFetchArtifactFromURL_ConcurrentMissesCoalesce(t *testing.T) {
|
|
const goroutines = 8
|
|
const content = "left-pad tarball bytes"
|
|
|
|
proxy, _, _, _ := setupTestProxy(t)
|
|
fetcher := &countingFetcher{content: content, delay: fetchHoldTime}
|
|
proxy.Fetcher = fetcher
|
|
|
|
start := make(chan struct{})
|
|
var wg sync.WaitGroup
|
|
errs := make([]error, goroutines)
|
|
bodies := make([]string, goroutines)
|
|
|
|
for i := 0; i < goroutines; i++ {
|
|
wg.Add(1)
|
|
go func(i int) {
|
|
defer wg.Done()
|
|
<-start
|
|
res, err := proxy.GetOrFetchArtifactFromURL(context.Background(),
|
|
"npm", "left-pad", "1.3.0", "left-pad-1.3.0.tgz",
|
|
"https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz")
|
|
if err != nil {
|
|
errs[i] = err
|
|
return
|
|
}
|
|
defer func() { _ = res.Reader.Close() }()
|
|
b, err := io.ReadAll(res.Reader)
|
|
errs[i] = err
|
|
bodies[i] = string(b)
|
|
}(i)
|
|
}
|
|
|
|
close(start)
|
|
wg.Wait()
|
|
|
|
for i, err := range errs {
|
|
if err != nil {
|
|
t.Errorf("goroutine %d: unexpected error: %v", i, err)
|
|
}
|
|
}
|
|
// Every caller must get its own intact copy of the bytes.
|
|
for i, b := range bodies {
|
|
if b != content {
|
|
t.Errorf("goroutine %d: body = %q, want %q", i, b, content)
|
|
}
|
|
}
|
|
if got := fetcher.calls.Load(); got != 1 {
|
|
t.Errorf("upstream fetches = %d, want 1 (%d concurrent callers stampeded the upstream)", got, goroutines)
|
|
}
|
|
}
|
|
|
|
// TestGetOrFetchArtifactFromURL_ConcurrentMissesFileStorage runs the same
|
|
// scenario against the real file:// backend, the default in production.
|
|
//
|
|
// Uncoalesced this fails outright, not merely wastefully. Every caller stores
|
|
// to one key, and fileblob rewrites a ".attrs" sidecar per key with os.Create,
|
|
// truncating in place outside the rename that protects the blob. Decoding that
|
|
// sidecar mid-truncate gives "opening reader: EOF", served as a 502.
|
|
//
|
|
// Only the fetcher is stubbed, because the real one refuses loopback so an
|
|
// httptest upstream is unreachable. The storage, where this fails, is real.
|
|
func TestGetOrFetchArtifactFromURL_ConcurrentMissesFileStorage(t *testing.T) {
|
|
const goroutines = 16
|
|
content := bytes.Repeat([]byte("tarball-bytes-"), 512)
|
|
|
|
ctx := context.Background()
|
|
dir := t.TempDir()
|
|
|
|
db, err := database.Create(filepath.Join(dir, "test.db"))
|
|
if err != nil {
|
|
t.Fatalf("create database: %v", err)
|
|
}
|
|
t.Cleanup(func() { _ = db.Close() })
|
|
|
|
store, err := storage.OpenBucket(ctx, "file://"+filepath.Join(dir, "cache"))
|
|
if err != nil {
|
|
t.Fatalf("open storage: %v", err)
|
|
}
|
|
t.Cleanup(func() { _ = store.Close() })
|
|
|
|
fetcher := &countingFetcher{content: string(content), delay: fetchHoldTime}
|
|
proxy := NewProxy(db, store, fetcher, fetch.NewResolver(),
|
|
slog.New(slog.NewTextHandler(io.Discard, nil)))
|
|
|
|
start := make(chan struct{})
|
|
var wg sync.WaitGroup
|
|
errs := make([]error, goroutines)
|
|
bodies := make([][]byte, goroutines)
|
|
|
|
for i := 0; i < goroutines; i++ {
|
|
wg.Add(1)
|
|
go func(i int) {
|
|
defer wg.Done()
|
|
<-start
|
|
res, err := proxy.GetOrFetchArtifactFromURL(ctx,
|
|
"npm", "left-pad", "1.3.0", "left-pad-1.3.0.tgz",
|
|
"https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz")
|
|
if err != nil {
|
|
errs[i] = err
|
|
return
|
|
}
|
|
defer func() { _ = res.Reader.Close() }()
|
|
body, readErr := io.ReadAll(res.Reader)
|
|
errs[i] = readErr
|
|
bodies[i] = body
|
|
}(i)
|
|
}
|
|
|
|
close(start)
|
|
wg.Wait()
|
|
|
|
for i, err := range errs {
|
|
if err != nil {
|
|
t.Errorf("caller %d failed: %v", i, err)
|
|
}
|
|
}
|
|
for i, body := range bodies {
|
|
if !bytes.Equal(body, content) {
|
|
t.Errorf("caller %d got %d bytes, want %d", i, len(body), len(content))
|
|
}
|
|
}
|
|
if got := fetcher.calls.Load(); got != 1 {
|
|
t.Errorf("upstream fetches = %d, want 1", got)
|
|
}
|
|
}
|