fix: decouple github_rpm scan and repodata read from the client request
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/test Pipeline was successful
ci/woodpecker/pr/pre-commit Pipeline was successful

A cold `dnf makecache` against a github_rpm remote with many release
assets 500s on the first request: ServeRemote derives metadata for every
asset synchronously on the inbound request context, so once dnf hits its
makecache timeout and disconnects the canceled request context both
aborts the in-flight derive and poisons the subsequent repodata DB read,
which surfaces as HTTP 500. It only "works" on a lucky client retry that
finds the partially-populated cache fresh.

- detach the release scan to a background, timeout-bounded context so a
  client cancel can neither abort the shared derive nor cancel the read
- single-flight the scan per remote so concurrent requests never launch
  duplicate derives
- serve the current cache immediately when it is non-empty and derive in
  the background; only a completely empty cache blocks on a bounded first
  scan
- serve repodata on a context detached from the request, and treat a
  canceled/deadline-exceeded metadata read as a retryable 503 instead of
  a hard 500
This commit is contained in:
2026-08-10 11:09:22 +10:00
parent b727b990a2
commit 9ba96ace41
3 changed files with 131 additions and 36 deletions
+44 -1
View File
@@ -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(), "<repomd") {
t.Fatalf("expected repomd served from cache, got %s", rec.Body.String())
}
}
func TestGitHubServeRemoteRedirectRequiresReleasesRemote(t *testing.T) {
fx := newGitHubFixture(t, true)
p := newTestProvider()