From 8f09c1dc30d24d5f2c1873fb0681ca77ae168675 Mon Sep 17 00:00:00 2001 From: montehurd Date: Mon, 21 Sep 2026 13:51:48 -0700 Subject: [PATCH 1/2] Fix the handler test build The denylist tests call NewDebianHandler with three arguments, but the additional APT archives change gave it a fourth. Each passed CI on its own branch, and together they leave internal/handler without a test binary that compiles, so go vet and go test fail on main. --- internal/handler/denylist_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/handler/denylist_test.go b/internal/handler/denylist_test.go index 3bcb7e73..5034a403 100644 --- a/internal/handler/denylist_test.go +++ b/internal/handler/denylist_test.go @@ -172,7 +172,7 @@ func TestDenylistDownloadRoutes(t *testing.T) { {"npm", "/demo/-/demo-1.0.0.tgz", "demo-1.0.0.tgz", func(p *Proxy) http.Handler { return NewNPMHandler(p, "http://proxy.test", "").Routes() }}, {"pypi", "/packages/a/b/demo-1.0.0.tar.gz", "demo-1.0.0.tar.gz", func(p *Proxy) http.Handler { return NewPyPIHandler(p, "http://proxy.test").Routes() }}, {"cargo", "/crates/demo/1.0.0/download", "demo-1.0.0.crate", func(p *Proxy) http.Handler { return NewCargoHandler(p, "http://proxy.test", "", "").Routes() }}, - {"deb", "/pool/main/d/demo/demo_1.0.0_amd64.deb", "demo_1.0.0_amd64.deb", func(p *Proxy) http.Handler { return NewDebianHandler(p, "http://proxy.test", "").Routes() }}, + {"deb", "/pool/main/d/demo/demo_1.0.0_amd64.deb", "demo_1.0.0_amd64.deb", func(p *Proxy) http.Handler { return NewDebianHandler(p, "http://proxy.test", "", nil).Routes() }}, } { t.Run(tc.ecosystem, func(t *testing.T) { p, db, store, _ := setupTestProxy(t) From c9aca77526189eb62fe31ac773bed6b72441053c Mon Sep 17 00:00:00 2001 From: montehurd Date: Mon, 21 Sep 2026 13:33:31 -0700 Subject: [PATCH 2/2] End an eviction pass that cannot evict anything A failed delete leaves the record eligible, so the same batch came back on the next iteration while the recorded size never dropped. Against a backend refusing deletes the pass retried forever, burning CPU and flooding the log. A canceled context made it faster: every delete fails at once, so the loop spun as quickly as the database could answer. The pass now ends when a batch clears no record, and returns as soon as the context is canceled. Progress counts records rather than bytes, because a cached record can have no size recorded. A pass can therefore end with the cache still above its limit, leaving the records that failed for the next pass to retry. --- internal/server/eviction.go | 89 +++++++++++++++++-------- internal/server/eviction_test.go | 110 +++++++++++++++++++++++++++++++ 2 files changed, 171 insertions(+), 28 deletions(-) diff --git a/internal/server/eviction.go b/internal/server/eviction.go index 4173bd53..ea29e525 100644 --- a/internal/server/eviction.go +++ b/internal/server/eviction.go @@ -59,42 +59,34 @@ func evictLRU(ctx context.Context, db *database.DB, store storage.Storage, logge freedBytes := int64(0) for totalSize-freedBytes > maxSize { + if ctx.Err() != nil { + break + } + artifacts, err := db.GetLeastRecentlyUsedArtifacts(evictionBatch) if err != nil { logger.Warn("eviction: failed to get LRU artifacts", "error", err) - return + break } if len(artifacts) == 0 { break } - for _, art := range artifacts { - if totalSize-freedBytes <= maxSize { - break - } - - if !art.StoragePath.Valid { - continue - } - - if err := store.Delete(ctx, art.StoragePath.String); err != nil { - logger.Warn("eviction: failed to delete from storage", - "path", art.StoragePath.String, "error", err) - continue - } - - if err := db.ClearArtifactCache(art.VersionPURL, art.Filename); err != nil { - logger.Warn("eviction: failed to clear artifact record", - "version_purl", art.VersionPURL, "filename", art.Filename, "error", err) - continue - } - - size := int64(0) - if art.Size.Valid { - size = art.Size.Int64 - } - freedBytes += size - evicted++ + cleared, freed := evictBatch(ctx, db, store, logger, artifacts, maxSize, totalSize-freedBytes) + evicted += cleared + freedBytes += freed + + if ctx.Err() != nil { + break + } + + // A batch that clears nothing returns the same records next time, so + // stop rather than retry them without end. The next pass retries after + // the interval. + if cleared == 0 { + logger.Warn("eviction: no artifact in this batch could be evicted, ending the pass", + "batch", len(artifacts)) + break } } @@ -103,3 +95,44 @@ func evictLRU(ctx context.Context, db *database.DB, store storage.Storage, logge "evicted", evicted, "freed_bytes", freedBytes) } } + +// evictBatch evicts from artifacts until the size still in use falls to +// maxSize, reporting how many records it cleared and how many bytes that +// freed. Progress is reported in records as well as bytes because a cached +// record can have no size recorded, and clearing one is still progress. +func evictBatch(ctx context.Context, db *database.DB, store storage.Storage, logger *slog.Logger, + artifacts []database.Artifact, maxSize, inUse int64) (int, int64) { + cleared := 0 + freed := int64(0) + + for _, art := range artifacts { + if inUse-freed <= maxSize { + break + } + if ctx.Err() != nil { + break + } + if !art.StoragePath.Valid { + continue + } + + if err := store.Delete(ctx, art.StoragePath.String); err != nil { + logger.Warn("eviction: failed to delete from storage", + "path", art.StoragePath.String, "error", err) + continue + } + + if err := db.ClearArtifactCache(art.VersionPURL, art.Filename); err != nil { + logger.Warn("eviction: failed to clear artifact record", + "version_purl", art.VersionPURL, "filename", art.Filename, "error", err) + continue + } + + if art.Size.Valid { + freed += art.Size.Int64 + } + cleared++ + } + + return cleared, freed +} diff --git a/internal/server/eviction_test.go b/internal/server/eviction_test.go index bac33252..efb7a2e9 100644 --- a/internal/server/eviction_test.go +++ b/internal/server/eviction_test.go @@ -3,10 +3,12 @@ package server import ( "context" "database/sql" + "errors" "io" "log/slog" "path/filepath" "strings" + "sync/atomic" "testing" "time" @@ -293,3 +295,111 @@ func defaultTestConfig(storagePath, dbPath string) *config.Config { Log: config.LogConfig{Level: "info", Format: "text"}, } } + +// undeletableStorage is a backend whose Delete always fails, standing in for a +// backend that refuses deletion: a permissions problem, or Windows while a +// reader holds the file open. +type undeletableStorage struct { + storage.Storage + deletes atomic.Int64 +} + +func (u *undeletableStorage) Delete(_ context.Context, _ string) error { + u.deletes.Add(1) + return errors.New("storage refused the delete") +} + +// runEvictionWithDeadline fails the test if a pass does not return, rather than +// hanging until the package timeout. +func runEvictionWithDeadline(t *testing.T, ctx context.Context, db *database.DB, store storage.Storage, maxSize int64) { + t.Helper() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + done := make(chan struct{}) + go func() { + defer close(done) + evictLRU(ctx, db, store, logger, maxSize) + }() + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("evictLRU never returned: it is retrying records it cannot evict") + } +} + +// TestEvictLRU_EndsPassWhenNothingCanBeEvicted is the loop that would otherwise +// never end. Records that fail to delete stay eligible, so the same batch comes +// back forever while the recorded size never drops. +func TestEvictLRU_EndsPassWhenNothingCanBeEvicted(t *testing.T) { + db, store := setupEvictionTest(t) + ctx := context.Background() + + now := time.Now() + seedArtifact(t, ctx, db, store, "old-pkg", 500, now.Add(-2*time.Hour)) + seedArtifact(t, ctx, db, store, "new-pkg", 500, now) + + undeletable := &undeletableStorage{Storage: store} + runEvictionWithDeadline(t, ctx, db, undeletable, 100) + + // One attempt per record, then the pass ends. Both records survive for the + // next pass to retry. + if got := undeletable.deletes.Load(); got != 2 { + t.Errorf("delete attempts = %d, want 2: one per record in the single batch", got) + } + count, err := db.GetCachedArtifactCount() + if err != nil { + t.Fatalf("failed to get count: %v", err) + } + if count != 2 { + t.Errorf("cached artifacts = %d, want 2: a failed delete must not clear the record", count) + } +} + +// TestEvictLRU_StopsWhenContextCanceled covers shutdown: a canceled context can +// make every delete fail instantly, which is the fastest way to spin. +func TestEvictLRU_StopsWhenContextCanceled(t *testing.T) { + db, store := setupEvictionTest(t) + seedArtifact(t, context.Background(), db, store, "old-pkg", 500, time.Now()) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + undeletable := &undeletableStorage{Storage: store} + runEvictionWithDeadline(t, ctx, db, undeletable, 100) + + if got := undeletable.deletes.Load(); got != 0 { + t.Errorf("delete attempts = %d, want 0 once the context is canceled", got) + } +} + +// TestEvictLRU_EvictsArtifactWithoutRecordedSize covers progress counted in +// records rather than bytes: a record with no size frees nothing, but evicting +// it is still progress and the pass must go on to the next record. +func TestEvictLRU_EvictsArtifactWithoutRecordedSize(t *testing.T) { + db, store := setupEvictionTest(t) + ctx := context.Background() + + now := time.Now() + seedArtifact(t, ctx, db, store, "sizeless-pkg", 500, now.Add(-2*time.Hour)) + seedArtifact(t, ctx, db, store, "sized-pkg", 500, now) + clearRecordedSize(t, db, "pkg:npm/sizeless-pkg@1.0.0", "sizeless-pkg-1.0.0.tgz") + + // The sizeless record is the oldest, so it is evicted first and frees + // nothing. Only evicting the second record gets under the limit. + runEvictionWithDeadline(t, ctx, db, store, 100) + + count, err := db.GetCachedArtifactCount() + if err != nil { + t.Fatalf("failed to get count: %v", err) + } + if count != 0 { + t.Errorf("cached artifacts = %d, want 0: the pass must continue past a record that frees nothing", count) + } +} + +func clearRecordedSize(t *testing.T, db *database.DB, versionPURL, filename string) { + t.Helper() + query := db.Rebind(`UPDATE artifacts SET size = NULL WHERE version_purl = ? AND filename = ?`) + if _, err := db.Exec(query, versionPURL, filename); err != nil { + t.Fatalf("clearing recorded size: %v", err) + } +}