From 13ae3b9cb22e95c0984708fa2348778259c1cb2f Mon Sep 17 00:00:00 2001 From: Simon Chrzanowski Date: Fri, 13 Nov 2026 16:26:20 +0100 Subject: [PATCH] Keep cleared artifacts out of the index eviction reads Eviction and retention clear an artifact's storage_path and keep the row. In idx_artifacts_last_used those rows sort first, so every eviction batch walked past every artifact ever cleared before it found one still in storage. The index is now partial, on storage_path IS NOT NULL, which the eviction query already filters on. --- .../database/cache_eviction_index_test.go | 129 ++++++++++++++++++ internal/database/schema.go | 31 ++++- 2 files changed, 158 insertions(+), 2 deletions(-) create mode 100644 internal/database/cache_eviction_index_test.go diff --git a/internal/database/cache_eviction_index_test.go b/internal/database/cache_eviction_index_test.go new file mode 100644 index 00000000..37559a1f --- /dev/null +++ b/internal/database/cache_eviction_index_test.go @@ -0,0 +1,129 @@ +package database + +import ( + "database/sql" + "strings" + "testing" + "time" +) + +// lastUsedIndexDefinition returns the SQL idx_artifacts_last_used was +// created with, or "" if there is no such index. +func lastUsedIndexDefinition(t *testing.T, db *DB) string { + t.Helper() + query := `SELECT sql FROM sqlite_master WHERE type = 'index' AND name = 'idx_artifacts_last_used'` + if db.dialect == DialectPostgres { + query = `SELECT indexdef FROM pg_indexes WHERE schemaname = current_schema() AND indexname = 'idx_artifacts_last_used'` + } + var def sql.NullString + if err := db.Get(&def, query); err != nil && err != sql.ErrNoRows { + t.Fatalf("reading the index: %v", err) + } + return def.String +} + +// isPartialOnStoredArtifacts reports whether an index definition leaves +// out artifacts that are not in storage. +func isPartialOnStoredArtifacts(def string) bool { + _, where, ok := strings.Cut(strings.ToLower(def), " where ") + return ok && strings.Contains(where, "storage_path is not null") +} + +// TestArtifactsLastUsedIndexLeavesOutClearedArtifacts checks that the index +// eviction reads holds only artifacts in storage. Eviction and retention +// clear a row's storage_path and keep the row, and those rows sort first, +// so in a full index every batch walks past all of them. +func TestArtifactsLastUsedIndexLeavesOutClearedArtifacts(t *testing.T) { + runWithBothDatabases(t, func(t *testing.T, db *DB) { + def := lastUsedIndexDefinition(t, db) + if !isPartialOnStoredArtifacts(def) { + t.Errorf("fresh schema: idx_artifacts_last_used = %q, want it limited to storage_path IS NOT NULL", def) + } + }) +} + +// TestMigrationMakesArtifactsLastUsedIndexPartial upgrades a database that +// has the index over every artifact, as migration 014 created it. +func TestMigrationMakesArtifactsLastUsedIndexPartial(t *testing.T) { + runWithBothDatabases(t, func(t *testing.T, db *DB) { + full := `CREATE INDEX idx_artifacts_last_used ON artifacts(COALESCE(last_accessed_at, fetched_at))` + if db.dialect == DialectPostgres { + full = `CREATE INDEX idx_artifacts_last_used ON artifacts((COALESCE(last_accessed_at, fetched_at)) NULLS FIRST)` + } + if _, err := db.Exec(`DROP INDEX IF EXISTS idx_artifacts_last_used`); err != nil { + t.Fatalf("dropping the index: %v", err) + } + if _, err := db.Exec(full); err != nil { + t.Fatalf("creating the old index: %v", err) + } + if isPartialOnStoredArtifacts(lastUsedIndexDefinition(t, db)) { + t.Fatal("the old index is already partial") + } + + // Twice: the migration must be safe to run again. + for range 2 { + if err := migratePartialArtifactsLastUsedIndex(db); err != nil { + t.Fatalf("migration: %v", err) + } + if def := lastUsedIndexDefinition(t, db); !isPartialOnStoredArtifacts(def) { + t.Fatalf("after the migration: idx_artifacts_last_used = %q, want it limited to storage_path IS NOT NULL", def) + } + } + + // A database that never had the index gets it too. + if _, err := db.Exec(`DROP INDEX IF EXISTS idx_artifacts_last_used`); err != nil { + t.Fatalf("dropping the index: %v", err) + } + if err := migratePartialArtifactsLastUsedIndex(db); err != nil { + t.Fatalf("migration without an index: %v", err) + } + if def := lastUsedIndexDefinition(t, db); !isPartialOnStoredArtifacts(def) { + t.Errorf("created from nothing: idx_artifacts_last_used = %q", def) + } + }) +} + +// TestLeastRecentlyUsedSkipsClearedArtifacts checks eviction still gets the +// same answer: artifacts in storage, least recently used first, with the +// cleared ones, which sort before them, left out. +func TestLeastRecentlyUsedSkipsClearedArtifacts(t *testing.T) { + runWithBothDatabases(t, func(t *testing.T, db *DB) { + now := time.Now() + add := func(name string, used time.Duration) { + t.Helper() + path := "npm/" + name + "/1.0.0/" + name + ".tgz" + if err := db.UpsertArtifact(&Artifact{ + VersionPURL: "pkg:npm/" + name + "@1.0.0", + Filename: name + ".tgz", + UpstreamURL: "https://example.test/" + name + ".tgz", + StoragePath: sql.NullString{String: path, Valid: true}, + Size: sql.NullInt64{Int64: 1, Valid: true}, + FetchedAt: sql.NullTime{Time: now.Add(-used), Valid: true}, + }); err != nil { + t.Fatalf("UpsertArtifact: %v", err) + } + } + add("cleared-a", 5*time.Hour) + add("cleared-b", 4*time.Hour) + add("oldest", 3*time.Hour) + add("newer", 2*time.Hour) + for _, name := range []string{"cleared-a", "cleared-b"} { + cleared, err := db.ClearArtifactCache("pkg:npm/"+name+"@1.0.0", name+".tgz", "npm/"+name+"/1.0.0/"+name+".tgz") + if err != nil || !cleared { + t.Fatalf("ClearArtifactCache(%s) = %v, %v", name, cleared, err) + } + } + + entries, err := db.GetLeastRecentlyUsedCacheEntries(10) + if err != nil { + t.Fatalf("GetLeastRecentlyUsedCacheEntries: %v", err) + } + var got []string + for _, e := range entries { + got = append(got, e.Name) + } + if want := []string{"oldest.tgz", "newer.tgz"}; strings.Join(got, ",") != strings.Join(want, ",") { + t.Errorf("entries = %v, want %v", got, want) + } + }) +} diff --git a/internal/database/schema.go b/internal/database/schema.go index bca37bd4..210310e1 100644 --- a/internal/database/schema.go +++ b/internal/database/schema.go @@ -140,7 +140,7 @@ CREATE TABLE IF NOT EXISTS metadata_cache ( CREATE UNIQUE INDEX IF NOT EXISTS idx_metadata_eco_name ON metadata_cache(ecosystem, name); CREATE INDEX IF NOT EXISTS idx_metadata_storage_path ON metadata_cache(storage_path); CREATE INDEX IF NOT EXISTS idx_metadata_last_used ON metadata_cache(COALESCE(last_accessed_at, updated_at)); -CREATE INDEX IF NOT EXISTS idx_artifacts_last_used ON artifacts(COALESCE(last_accessed_at, fetched_at)); +CREATE INDEX IF NOT EXISTS idx_artifacts_last_used ON artifacts(COALESCE(last_accessed_at, fetched_at)) WHERE storage_path IS NOT NULL; CREATE TABLE IF NOT EXISTS metadata_dates ( ecosystem TEXT NOT NULL, @@ -319,7 +319,7 @@ CREATE TABLE IF NOT EXISTS metadata_cache ( CREATE UNIQUE INDEX IF NOT EXISTS idx_metadata_eco_name ON metadata_cache(ecosystem, name); CREATE INDEX IF NOT EXISTS idx_metadata_storage_path ON metadata_cache(storage_path); CREATE INDEX IF NOT EXISTS idx_metadata_last_used ON metadata_cache((COALESCE(last_accessed_at, updated_at)) NULLS FIRST); -CREATE INDEX IF NOT EXISTS idx_artifacts_last_used ON artifacts((COALESCE(last_accessed_at, fetched_at)) NULLS FIRST); +CREATE INDEX IF NOT EXISTS idx_artifacts_last_used ON artifacts((COALESCE(last_accessed_at, fetched_at)) NULLS FIRST) WHERE storage_path IS NOT NULL; CREATE TABLE IF NOT EXISTS metadata_dates ( ecosystem TEXT NOT NULL, @@ -546,6 +546,7 @@ var migrations = []migration{ {"021_add_projects", migrateAddProjects}, {"022_add_artifact_mirrored_at", migrateAddArtifactMirroredAt}, {"023_add_metadata_upstream_url", migrateAddMetadataUpstreamURL}, + {"024_partial_artifacts_last_used_index", migratePartialArtifactsLastUsedIndex}, } // isTableNotFound returns true if the error indicates a missing table. @@ -1173,3 +1174,29 @@ func migrateAddMetadataLastAccessed(db *DB) error { } return nil } + +// migratePartialArtifactsLastUsedIndex rebuilds the index eviction reads so +// it holds only artifacts in storage. Eviction and retention clear a row's +// storage_path and keep the row, and cleared rows sort first, so over a +// full index every eviction batch walked past every artifact ever cleared. +// The index is replaced in one transaction, so there is always one to read. +func migratePartialArtifactsLastUsedIndex(db *DB) error { + create := `CREATE INDEX idx_artifacts_last_used ON artifacts(COALESCE(last_accessed_at, fetched_at)) WHERE storage_path IS NOT NULL` + if db.dialect == DialectPostgres { + create = `CREATE INDEX idx_artifacts_last_used ON artifacts((COALESCE(last_accessed_at, fetched_at)) NULLS FIRST) WHERE storage_path IS NOT NULL` + } + tx, err := db.Beginx() + if err != nil { + return fmt.Errorf("replacing artifacts last-used index: %w", err) + } + defer func() { _ = tx.Rollback() }() + for _, q := range []string{`DROP INDEX IF EXISTS idx_artifacts_last_used`, create} { + if _, err := tx.Exec(q); err != nil { + return fmt.Errorf("replacing artifacts last-used index: %w", err) + } + } + if err := tx.Commit(); err != nil { + return fmt.Errorf("replacing artifacts last-used index: %w", err) + } + return nil +}