diff --git a/internal/provider/rpm/github.go b/internal/provider/rpm/github.go index 7d6fbc6..3788072 100644 --- a/internal/provider/rpm/github.go +++ b/internal/provider/rpm/github.go @@ -33,6 +33,12 @@ const ( defaultHeaderRangeInitial = 1 << 20 // 1 MiB — covers the header of almost every RPM defaultHeaderRangeMax = 16 << 20 // 16 MiB — give up past this and skip the asset defaultReleasePageCap = 10 // 100 releases/page * 10 pages + + // defaultScanTimeout bounds a detached background scan (which may do one + // ranged fetch per asset across every release) so it can never run forever. + defaultScanTimeout = 10 * time.Minute + // defaultServeTimeout bounds a repodata DB read served on a detached context. + defaultServeTimeout = 30 * time.Second ) // GitHubProvider is a metadata-only remote: it scans a GitHub repo's releases @@ -46,9 +52,11 @@ type GitHubProvider struct { headerInitial int64 headerMax int64 pageCap int + scanTimeout time.Duration + serveTimeout time.Duration mu sync.Mutex - scanLock map[string]*sync.Mutex + scanning map[string]bool lastScan map[string]time.Time } @@ -58,7 +66,9 @@ func newGitHubProvider() *GitHubProvider { headerInitial: defaultHeaderRangeInitial, headerMax: defaultHeaderRangeMax, pageCap: defaultReleasePageCap, - scanLock: map[string]*sync.Mutex{}, + scanTimeout: defaultScanTimeout, + serveTimeout: defaultServeTimeout, + scanning: map[string]bool{}, lastScan: map[string]time.Time{}, } } @@ -104,20 +114,27 @@ func (p *GitHubProvider) AuthHeaders(_ context.Context, remote models.Remote) (h // 302-redirects .rpm downloads to the backend releases_remote. Returns false // only for paths it does not own, letting the normal proxy path take over. func (p *GitHubProvider) ServeRemote(w http.ResponseWriter, r *http.Request, remote models.Remote, path, proxyBaseURL string, store provider.RemoteMetadataStore) bool { - p.ensureFresh(r.Context(), remote, store) + p.refresh(remote, store) if strings.HasPrefix(path, "repodata/") { + // Serve repodata on a context detached from the inbound request: a + // client disconnect (e.g. dnf makecache timing out) must never cancel + // the metadata DB read and surface as a 500. + sctx, cancel := context.WithTimeout(context.WithoutCancel(r.Context()), p.serveTimeout) + defer cancel() + sr := r.WithContext(sctx) + tail := strings.TrimPrefix(path, "repodata/") lp := &Provider{} switch { case tail == "repomd.xml": - lp.serveRepomd(w, r, store, remote.Name) + lp.serveRepomd(w, sr, store, remote.Name) case strings.HasSuffix(tail, "-primary.xml.gz"): - lp.servePrimary(w, r, store, remote.Name) + lp.servePrimary(w, sr, store, remote.Name) case strings.HasSuffix(tail, "-filelists.xml.gz"): - lp.serveFilelists(w, r, store, remote.Name) + lp.serveFilelists(w, sr, store, remote.Name) case strings.HasSuffix(tail, "-other.xml.gz"): - lp.serveOther(w, r, store, remote.Name) + lp.serveOther(w, sr, store, remote.Name) default: http.Error(w, "not found", http.StatusNotFound) } @@ -137,22 +154,52 @@ func (p *GitHubProvider) ServeRemote(w http.ResponseWriter, r *http.Request, rem return false } -func (p *GitHubProvider) ensureFresh(ctx context.Context, remote models.Remote, store provider.RemoteMetadataStore) { +// refresh brings the derived metadata up to date without coupling the scan to +// the inbound request. When the cache is stale it single-flights a scan: if the +// cache already holds rows the scan runs in the background and the caller serves +// the current cache immediately; only a completely empty cache blocks on a +// bounded first scan (so the first client sees packages rather than an empty or +// 500 repodata). +func (p *GitHubProvider) refresh(remote models.Remote, store provider.RemoteMetadataStore) { ttl := time.Duration(remote.MutableTTL) * time.Second if ttl <= 0 { ttl = 5 * time.Minute } - lock := p.lockFor(remote.Name) - lock.Lock() - defer lock.Unlock() - p.mu.Lock() last, ok := p.lastScan[remote.Name] - p.mu.Unlock() - if ok && time.Since(last) < ttl { + fresh := ok && time.Since(last) < ttl + if fresh || p.scanning[remote.Name] { + p.mu.Unlock() return } + p.scanning[remote.Name] = true + p.mu.Unlock() + + empty := true + if rows, err := store.ListRPMMetadataEntries(context.Background(), remote.Name); err == nil { + empty = len(rows) == 0 + } + + if empty { + p.runScan(remote, store) + return + } + go p.runScan(remote, store) +} + +// runScan derives metadata on a detached, bounded context so a client cancel +// can neither abort the shared derive nor poison the metadata read. The caller +// must have already claimed the single-flight slot (scanning[name] = true). +func (p *GitHubProvider) runScan(remote models.Remote, store provider.RemoteMetadataStore) { + defer func() { + p.mu.Lock() + delete(p.scanning, remote.Name) + p.mu.Unlock() + }() + + ctx, cancel := context.WithTimeout(context.Background(), p.scanTimeout) + defer cancel() if err := p.scan(ctx, remote, store); err != nil { // Keep serving whatever metadata is already cached rather than 500ing. @@ -165,17 +212,6 @@ func (p *GitHubProvider) ensureFresh(ctx context.Context, remote models.Remote, p.mu.Unlock() } -func (p *GitHubProvider) lockFor(name string) *sync.Mutex { - p.mu.Lock() - defer p.mu.Unlock() - l, ok := p.scanLock[name] - if !ok { - l = &sync.Mutex{} - p.scanLock[name] = l - } - return l -} - func (p *GitHubProvider) scan(ctx context.Context, remote models.Remote, store provider.RemoteMetadataStore) error { releases, err := p.fetchReleases(ctx, remote) if err != nil { diff --git a/internal/provider/rpm/github_test.go b/internal/provider/rpm/github_test.go index 2f6b854..3a30278 100644 --- a/internal/provider/rpm/github_test.go +++ b/internal/provider/rpm/github_test.go @@ -14,6 +14,7 @@ import ( "strings" "sync" "testing" + "time" "git.unkin.net/unkin/artifactapi/internal/provider" "git.unkin.net/unkin/artifactapi/internal/testsupport" @@ -46,7 +47,12 @@ func (f *fakeStore) DeleteRPMMetadata(_ context.Context, _, filePath string) err return nil } -func (f *fakeStore) ListRPMMetadataEntries(_ context.Context, _ string) ([]provider.RPMMetadata, error) { +func (f *fakeStore) ListRPMMetadataEntries(ctx context.Context, _ string) ([]provider.RPMMetadata, error) { + // Mirror pgx: a canceled/expired context fails the read. This is what + // poisons the repodata response if the read runs on the inbound request. + if err := ctx.Err(); err != nil { + return nil, err + } f.mu.Lock() defer f.mu.Unlock() out := make([]provider.RPMMetadata, 0, len(f.rows)) @@ -266,6 +272,43 @@ func TestGitHubServeRemoteRepodataAndRedirect(t *testing.T) { } } +// TestGitHubServeRemoteCanceledRequestServesCache reproduces the cold-makecache +// 500: when the inbound request context is already canceled (dnf timed out and +// disconnected), the repodata read must not be run on that context and turned +// into a 500. With the cache already warm, the handler serves it as 200. +// Before the fix the read used r.Context() and returned 500; after the fix it +// runs on a detached context and serves the cached repomd. +func TestGitHubServeRemoteCanceledRequestServesCache(t *testing.T) { + fx := newGitHubFixture(t, true) + p := newTestProvider() + store := newFakeStore() + remote := fx.remote() + + // Warm the cache and mark the scan fresh so ServeRemote does not re-derive. + if err := p.scan(context.Background(), remote, store); err != nil { + t.Fatalf("warm scan: %v", err) + } + p.mu.Lock() + p.lastScan[remote.Name] = time.Now() + p.mu.Unlock() + + // Inbound request whose context is already canceled (client went away). + ctx, cancel := context.WithCancel(context.Background()) + cancel() + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/v1/remote/acme-rpm/repodata/repomd.xml", nil).WithContext(ctx) + + if !p.ServeRemote(rec, req, remote, "repodata/repomd.xml", "https://x", store) { + t.Fatal("ServeRemote did not handle repomd.xml") + } + if rec.Code != http.StatusOK { + t.Fatalf("canceled request must serve cache, not error; got code=%d body=%s", rec.Code, rec.Body.String()) + } + if !strings.Contains(rec.Body.String(), "