diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 64ecd809..c8d0bc79 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,6 +31,40 @@ jobs: - name: Test run: go test -v -race ./... + postgres: + name: test (postgres) + runs-on: ubuntu-latest + services: + postgres: + image: postgres:17@sha256:b8faeff7712663c2773a190181e1246e37e364b887611c04658c28b44e2408fe + env: + POSTGRES_PASSWORD: test + POSTGRES_DB: proxy + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U postgres -d proxy" + --health-interval 2s + --health-timeout 5s + --health-retries 30 + + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + + - name: Set up Go + uses: actions/setup-go@b7ad1dad31e06c5925ef5d2fc7ad053ef454303e # v7.0.0 + with: + go-version-file: go.mod + + # The packages with tests that need PROXY_DATABASE_URL. -p 1 because + # their tests drop and recreate tables in the one shared database. + - name: Test + env: + PROXY_DATABASE_URL: postgres://postgres:test@localhost:5432/proxy?sslmode=disable + run: go test -v -race -p 1 ./internal/database/... ./internal/handler/... ./internal/server/... + lint: runs-on: ubuntu-latest steps: diff --git a/README.md b/README.md index 0553e4a1..e5a5105c 100644 --- a/README.md +++ b/README.md @@ -1474,6 +1474,7 @@ The proxy exposes Prometheus metrics at `GET /metrics`. All metric names are pre | `proxy_metadata_cache_entries` | gauge | `ecosystem` | Number of cached metadata documents per ecosystem. | | `proxy_artifacts_evicted_total` | counter | `reason`, `ecosystem` | Artifacts evicted by the size limit (`lru`) or by age (`retention`) | | `proxy_metadata_evicted_total` | counter | `ecosystem` | Cached metadata documents evicted by the size limit | +| `proxy_fetch_lease_waits_total` | counter | `kind` | Cache misses (`artifact` or `metadata`) that waited for another instance sharing the database to fetch them | | `proxy_upstream_fetch_duration_seconds` | histogram | `ecosystem` | Time spent fetching from upstream | | `proxy_upstream_errors_total` | counter | `ecosystem`, `error_type` | Upstream fetch failures | | `proxy_storage_operation_duration_seconds` | histogram | `operation` | Storage read/write latency | diff --git a/deploy/charts/proxy/values.yaml b/deploy/charts/proxy/values.yaml index 9a74e475..cb7a0bfe 100644 --- a/deploy/charts/proxy/values.yaml +++ b/deploy/charts/proxy/values.yaml @@ -1,7 +1,8 @@ -# More than one replica requires config.data.database on Postgres, -# config.data.storage on object storage, deploymentStrategy: RollingUpdate, -# and a PVC access mode other than ReadWriteOnce (or persistence disabled). -# With upstream.git, each replica also needs its own git.cache_dir. +# More than one replica requires config.data.database on Postgres and +# deploymentStrategy: RollingUpdate. Storage can be object storage (with +# persistence disabled) or a file:// URL on a ReadWriteMany volume +# (persistence.accessModes: [ReadWriteMany]). With upstream.git, each +# replica also needs its own git.cache_dir. replicaCount: 1 nameOverride: "" diff --git a/docs/configuration.md b/docs/configuration.md index 4da6f634..50327ce3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -197,6 +197,20 @@ database: |--------|-------------|------|-------------| | `database.url` | `PROXY_DATABASE_URL` | `-database-url` | PostgreSQL connection URL | +### Several instances + +Several instances can serve one cache when they share a PostgreSQL database and storage. The storage can be S3 or a `file://` directory on a volume every instance mounts, such as NFS, CephFS or a Kubernetes `ReadWriteMany` volume. Each fetch writes its own file and renames it into place, and records are updated with compare-and-swap, so concurrent fetches and a crashed instance leave no partial files or records that don't match a file. SQLite only supports a single instance. + +Instances coordinate through PostgreSQL advisory locks: + +- Instances started together create the schema and run migrations one after the other. +- Eviction, reclaim, retention and the Gradle build cache sweep run on one instance at a time. An instance that finds a job running elsewhere skips that pass. +- A [scheduled mirror](#scheduled-refresh) runs on whichever instance reaches it first in each interval. + +Cache misses are fetched from upstream once across instances. The instance that fetches holds a lease on the miss in the `fetch_leases` table and renews it while it downloads. Another instance missing the same file waits until it is stored and serves the stored copy; `proxy_fetch_lease_waits_total` counts those waits. If the fetch fails, the instances waiting on it stop waiting together: each serves its stale metadata copy if it has one, and otherwise fetches itself, so a failing upstream costs every instance one attempt at the same time rather than one after another. A lease of an instance that stopped expires after 30 seconds. Metadata is shared this way only with [`cache_metadata`](#metadata-caching) and a `metadata_ttl` above zero, since without a stored copy there is nothing to wait for. A cache hit costs no extra query; a miss costs one. + +Each instance needs its own [`git.cache_dir`](#git-repositories). + ### Hit counts Every cache hit updates the artifact's hit count and last-access time, which the stats pages and LRU eviction use. Rather than writing each hit as its own transaction, the proxy counts hits in memory and writes them together every `hit_flush_interval`. SQLite allows one writer at a time, and the proxy uses a single SQLite connection, so with a write per hit, concurrent downloads queue behind each other. @@ -1496,7 +1510,7 @@ mirror: Each run checks the tag with a HEAD request. If the tag still points to the cached digest, nothing else is fetched. If it moved, the new manifests and blobs are fetched first and the tag is switched only when all of them are cached, so a failed run leaves the previous image pullable. Images of one schedule are mirrored one after the other. -- The first run starts within ten minutes of startup, later runs every `interval` ±10 %, so restarts and several instances don't hit the registry at once. Each instance runs the schedules on its own. +- The first run starts within ten minutes of startup, later runs every `interval` ±10 %, so restarts and several instances don't hit the registry at once. Instances sharing a PostgreSQL database run each schedule once per interval: the first to reach it records the run, and the others skip it. - Digest-pinned PURLs never move and are rejected. A `pkg:oci` digest with `tag=` is accepted; the tag is refreshed and a warning is logged when it no longer matches the digest. - The previous digest stays cached, so pulls by digest keep working, until eviction or retention removes it. - Docker Hub does not count HEAD requests against its pull limit, but every changed manifest is a pull. Set `platforms` and `upstream.auth` for `registry-1.docker.io` to stay under the anonymous limit. diff --git a/internal/database/database.go b/internal/database/database.go index f438f676..5a7e1275 100644 --- a/internal/database/database.go +++ b/internal/database/database.go @@ -1,6 +1,7 @@ package database import ( + "context" "fmt" "os" "path/filepath" @@ -125,18 +126,24 @@ func OpenPostgresOrCreate(url string) (*DB, error) { return nil, err } - var exists bool - err = db.Get(&exists, "SELECT EXISTS (SELECT FROM information_schema.tables WHERE table_name = 'schema_info')") - if err != nil { - _ = db.Close() - return nil, fmt.Errorf("checking schema: %w", err) - } - - if !exists { + // Instances started together on an empty database would otherwise + // create the schema at the same time and fail on each other's tables. + err = db.WithLock(context.Background(), schemaLock, func() error { + var exists bool + if err := db.Get(&exists, "SELECT EXISTS (SELECT FROM information_schema.tables WHERE table_name = 'schema_info')"); err != nil { + return fmt.Errorf("checking schema: %w", err) + } + if exists { + return nil + } if err := db.CreateSchema(); err != nil { - _ = db.Close() - return nil, fmt.Errorf("creating schema: %w", err) + return fmt.Errorf("creating schema: %w", err) } + return nil + }) + if err != nil { + _ = db.Close() + return nil, err } return db, nil diff --git a/internal/database/database_test.go b/internal/database/database_test.go index f09a86a0..8b29cede 100644 --- a/internal/database/database_test.go +++ b/internal/database/database_test.go @@ -613,7 +613,7 @@ func createTestPostgresDB(t *testing.T) *DB { // Drop and recreate every table CreateSchema creates for clean test state; // leftover migration records make the next CreateSchema fail on the // migrations primary key. - tables := []string{"artifacts", "pending_deletes", "versions", "packages", "vulnerabilities", "metadata_cache", "metadata_dates", "migrations", "schema_info"} + tables := []string{"artifacts", "pending_deletes", "versions", "packages", "vulnerabilities", "metadata_cache", "metadata_dates", "job_runs", "fetch_leases", "migrations", "schema_info"} for _, table := range tables { _, _ = db.Exec("DROP TABLE IF EXISTS " + table + " CASCADE") } diff --git a/internal/database/fetch_leases.go b/internal/database/fetch_leases.go new file mode 100644 index 00000000..56390e35 --- /dev/null +++ b/internal/database/fetch_leases.go @@ -0,0 +1,94 @@ +package database + +import ( + "database/sql" + "errors" + "fmt" + "time" +) + +// Fetch leases let instances sharing a Postgres database fetch each cache +// miss from upstream once. The instance that fetches holds the lease for the +// miss's key and renews it until the result is stored; the others wait for +// the stored result. Expiry uses the database clock, so instances whose +// clocks disagree still agree on when a lease has run out. SQLite is +// single-instance, so these are only used on Postgres. + +// AcquireFetchLease takes the lease on key for owner unless another owner +// holds one that has neither expired nor failed, and reports whether it did. +func (db *DB) AcquireFetchLease(key, owner string, ttl time.Duration) (bool, error) { + res, err := db.Exec(` + INSERT INTO fetch_leases (key, owner, expires_at) + VALUES ($1, $2, now() + $3 * interval '1 millisecond') + ON CONFLICT (key) DO UPDATE SET owner = excluded.owner, expires_at = excluded.expires_at, failed = FALSE + WHERE fetch_leases.expires_at < now() OR fetch_leases.failed`, key, owner, ttl.Milliseconds()) + if err != nil { + return false, fmt.Errorf("acquiring fetch lease: %w", err) + } + n, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("acquiring fetch lease: %w", err) + } + return n > 0, nil +} + +// RenewFetchLease extends owner's lease on key by ttl from now, and reports +// whether owner still held it. +func (db *DB) RenewFetchLease(key, owner string, ttl time.Duration) (bool, error) { + res, err := db.Exec(` + UPDATE fetch_leases SET expires_at = now() + $3 * interval '1 millisecond' + WHERE key = $1 AND owner = $2`, key, owner, ttl.Milliseconds()) + if err != nil { + return false, fmt.Errorf("renewing fetch lease: %w", err) + } + n, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("renewing fetch lease: %w", err) + } + return n > 0, nil +} + +// FetchLeaseHolder returns the owner of the lease on key, and whether its +// fetch failed. ok is false when no lease is held or it has expired. +func (db *DB) FetchLeaseHolder(key string) (owner string, failed, ok bool, err error) { + err = db.QueryRow(` + SELECT owner, failed FROM fetch_leases + WHERE key = $1 AND expires_at >= now()`, key).Scan(&owner, &failed) + if errors.Is(err, sql.ErrNoRows) { + return "", false, false, nil + } + if err != nil { + return "", false, false, fmt.Errorf("reading fetch lease: %w", err) + } + return owner, failed, true, nil +} + +// FailFetchLease marks owner's lease on key as failed. Instances waiting on +// it stop waiting, and the next AcquireFetchLease takes it at once. +func (db *DB) FailFetchLease(key, owner string) error { + if _, err := db.Exec(`UPDATE fetch_leases SET failed = TRUE WHERE key = $1 AND owner = $2`, key, owner); err != nil { + return fmt.Errorf("failing fetch lease: %w", err) + } + return nil +} + +// ReleaseFetchLease drops owner's lease on key. +func (db *DB) ReleaseFetchLease(key, owner string) error { + if _, err := db.Exec(`DELETE FROM fetch_leases WHERE key = $1 AND owner = $2`, key, owner); err != nil { + return fmt.Errorf("releasing fetch lease: %w", err) + } + return nil +} + +// DeleteExpiredFetchLeases removes leases that expired more than grace ago, +// left by instances that stopped while fetching. +func (db *DB) DeleteExpiredFetchLeases(grace time.Duration) (int64, error) { + if db.dialect != DialectPostgres { + return 0, nil + } + res, err := db.Exec(`DELETE FROM fetch_leases WHERE expires_at < now() - $1 * interval '1 millisecond'`, grace.Milliseconds()) + if err != nil { + return 0, fmt.Errorf("deleting expired fetch leases: %w", err) + } + return res.RowsAffected() +} diff --git a/internal/database/lock.go b/internal/database/lock.go new file mode 100644 index 00000000..b245c629 --- /dev/null +++ b/internal/database/lock.go @@ -0,0 +1,98 @@ +package database + +import ( + "context" + "database/sql" + "database/sql/driver" + "fmt" + "hash/fnv" + "time" +) + +// Several instances can share one Postgres database. Advisory locks keep +// them from creating the schema, migrating it or running a background job at +// the same time. SQLite is single-instance, so there every lock is granted +// at once. + +// lockKey maps a lock name to the 64-bit key Postgres advisory locks take. +func lockKey(name string) int64 { + h := fnv.New64a() + _, _ = h.Write([]byte("git-pkgs/proxy:" + name)) + return int64(h.Sum64()) //nolint:gosec // a key, wrapping is fine +} + +// WithLock runs fn while holding the named lock, waiting for another +// instance that holds it. +func (db *DB) WithLock(ctx context.Context, name string, fn func() error) error { + if db.dialect != DialectPostgres { + return fn() + } + conn, err := db.Conn(ctx) + if err != nil { + return fmt.Errorf("taking connection for lock %s: %w", name, err) + } + defer func() { _ = conn.Close() }() + if _, err := conn.ExecContext(ctx, "SELECT pg_advisory_lock($1)", lockKey(name)); err != nil { + return fmt.Errorf("taking lock %s: %w", name, err) + } + defer unlock(conn, name) + return fn() +} + +// TryLock takes the named lock if no other instance holds it. When ok is +// true the caller must call release once done. The lock lives in a +// connection held for that time, and Postgres frees it if the process dies. +func (db *DB) TryLock(ctx context.Context, name string) (release func(), ok bool, err error) { + if db.dialect != DialectPostgres { + return func() {}, true, nil + } + conn, err := db.Conn(ctx) + if err != nil { + return nil, false, fmt.Errorf("taking connection for lock %s: %w", name, err) + } + if err := conn.QueryRowContext(ctx, "SELECT pg_try_advisory_lock($1)", lockKey(name)).Scan(&ok); err != nil { + _ = conn.Close() + return nil, false, fmt.Errorf("taking lock %s: %w", name, err) + } + if !ok { + _ = conn.Close() + return nil, false, nil + } + return func() { + unlock(conn, name) + _ = conn.Close() + }, true, nil +} + +// unlockTimeout bounds the unlock. If it fails, closing the connection still +// ends the session that holds the lock. +const unlockTimeout = 10 * time.Second + +func unlock(conn *sql.Conn, name string) { + ctx, cancel := context.WithTimeout(context.Background(), unlockTimeout) + defer cancel() + if _, err := conn.ExecContext(ctx, "SELECT pg_advisory_unlock($1)", lockKey(name)); err != nil { + // Drop the connection so the session, and with it the lock, ends. + _ = conn.Raw(func(any) error { return driver.ErrBadConn }) + } +} + +// ClaimJobRun records that the named job starts now unless it started less +// than minGap ago, and reports whether the caller should run it. Instances +// sharing the database each keep their own timer for a scheduled job, and +// this lets only the first of them run it per interval. +func (db *DB) ClaimJobRun(name string, minGap time.Duration) (bool, error) { + now := time.Now().UTC() + res, err := db.Exec(db.Rebind(` + INSERT INTO job_runs (name, started_at) VALUES (?, ?) + ON CONFLICT(name) DO UPDATE SET started_at = excluded.started_at + WHERE job_runs.started_at <= ?`), name, now, now.Add(-minGap)) + if err != nil { + return false, fmt.Errorf("claiming job run %s: %w", name, err) + } + n, err := res.RowsAffected() + if err != nil { + return false, fmt.Errorf("claiming job run %s: %w", name, err) + } + return n > 0, nil +} diff --git a/internal/database/lock_test.go b/internal/database/lock_test.go new file mode 100644 index 00000000..11dd6198 --- /dev/null +++ b/internal/database/lock_test.go @@ -0,0 +1,335 @@ +package database + +import ( + "context" + "net/url" + "os" + "strings" + "sync" + "sync/atomic" + "testing" + "time" +) + +func TestTryLockSQLiteAlwaysGranted(t *testing.T) { + db := createTestDB(t) + defer func() { _ = db.Close() }() + release, ok, err := db.TryLock(context.Background(), "job:test") + if err != nil || !ok { + t.Fatalf("TryLock = %v, %v; want granted", ok, err) + } + // SQLite is single-instance, so a second holder is granted too. + release2, ok, err := db.TryLock(context.Background(), "job:test") + if err != nil || !ok { + t.Fatalf("second TryLock = %v, %v; want granted", ok, err) + } + release2() + release() +} + +func TestClaimJobRun(t *testing.T) { + forEachDialect(t, func(t *testing.T, db *DB) { + claimed, err := db.ClaimJobRun("mirror-schedule:a", time.Hour) + if err != nil || !claimed { + t.Fatalf("first claim = %v, %v; want claimed", claimed, err) + } + claimed, err = db.ClaimJobRun("mirror-schedule:a", time.Hour) + if err != nil || claimed { + t.Fatalf("claim within the gap = %v, %v; want not claimed", claimed, err) + } + claimed, err = db.ClaimJobRun("mirror-schedule:b", time.Hour) + if err != nil || !claimed { + t.Fatalf("claim of another job = %v, %v; want claimed", claimed, err) + } + // A run started longer ago than the gap can be claimed again. + claimed, err = db.ClaimJobRun("mirror-schedule:a", 0) + if err != nil || !claimed { + t.Fatalf("claim after the gap = %v, %v; want claimed", claimed, err) + } + }) +} + +// forEachDialect runs fn against SQLite and, when PROXY_DATABASE_URL is set, +// against Postgres. +func forEachDialect(t *testing.T, fn func(t *testing.T, db *DB)) { + t.Run("sqlite", func(t *testing.T) { + db := createTestDB(t) + defer func() { _ = db.Close() }() + fn(t, db) + }) + t.Run("postgres", func(t *testing.T) { + db := createTestPostgresDB(t) + if db == nil { + t.Skip("PROXY_DATABASE_URL not set") + } + defer func() { _ = db.Close() }() + fn(t, db) + }) +} + +func openTestPostgres(t *testing.T) *DB { + t.Helper() + dbURL := os.Getenv("PROXY_DATABASE_URL") + if dbURL == "" { + t.Skip("PROXY_DATABASE_URL not set") + } + db, err := OpenPostgres(dbURL) + if err != nil { + t.Fatalf("OpenPostgres: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + return db +} + +func TestTryLockPostgresExcludesOtherInstance(t *testing.T) { + a := openTestPostgres(t) + b := openTestPostgres(t) + ctx := context.Background() + + release, ok, err := a.TryLock(ctx, "job:test-exclusive") + if err != nil || !ok { + t.Fatalf("TryLock on a = %v, %v; want granted", ok, err) + } + if _, ok, err := b.TryLock(ctx, "job:test-exclusive"); err != nil || ok { + t.Fatalf("TryLock on b while a holds it = %v, %v; want refused", ok, err) + } + // Another lock name is independent. + releaseOther, ok, err := b.TryLock(ctx, "job:test-other") + if err != nil || !ok { + t.Fatalf("TryLock on another name = %v, %v; want granted", ok, err) + } + releaseOther() + + release() + release, ok, err = b.TryLock(ctx, "job:test-exclusive") + if err != nil || !ok { + t.Fatalf("TryLock on b after a released = %v, %v; want granted", ok, err) + } + release() +} + +func TestTryLockPostgresFreedWhenSessionEnds(t *testing.T) { + a := openTestPostgres(t) + b := openTestPostgres(t) + ctx := context.Background() + + if _, ok, err := a.TryLock(ctx, "job:test-crash"); err != nil || !ok { + t.Fatalf("TryLock on a = %v, %v; want granted", ok, err) + } + // End a's session holding the lock, as a crash would. + if _, err := b.Exec(`SELECT pg_terminate_backend(pid) FROM pg_locks + WHERE locktype = 'advisory' AND pid <> pg_backend_pid()`); err != nil { + t.Fatalf("terminating a's session: %v", err) + } + waitUntil(t, func() bool { + release, ok, err := b.TryLock(ctx, "job:test-crash") + if ok { + release() + } + return err == nil && ok + }) +} + +func TestWithLockPostgresSerialises(t *testing.T) { + const instances = 4 + dbs := make([]*DB, instances) + for i := range dbs { + dbs[i] = openTestPostgres(t) + } + var inside, overlapped atomic.Int32 + var wg sync.WaitGroup + for _, db := range dbs { + wg.Go(func() { + err := db.WithLock(context.Background(), "test-serialise", func() error { + if inside.Add(1) > 1 { + overlapped.Store(1) + } + time.Sleep(20 * time.Millisecond) + inside.Add(-1) + return nil + }) + if err != nil { + t.Errorf("WithLock: %v", err) + } + }) + } + wg.Wait() + if overlapped.Load() != 0 { + t.Error("two instances held the lock at once") + } +} + +// TestOpenPostgresOrCreateConcurrent starts several instances at once on an +// empty database. Without the schema lock all but one failed on each +// other's tables. +func TestOpenPostgresOrCreateConcurrent(t *testing.T) { + admin := openTestPostgres(t) + name := "proxy_lock_test_" + strings.ReplaceAll(time.Now().UTC().Format("150405.000000"), ".", "_") + if _, err := admin.Exec("CREATE DATABASE " + name); err != nil { + t.Skipf("cannot create a scratch database: %v", err) + } + t.Cleanup(func() { _, _ = admin.Exec("DROP DATABASE IF EXISTS " + name + " WITH (FORCE)") }) + + u, err := url.Parse(os.Getenv("PROXY_DATABASE_URL")) + if err != nil { + t.Fatalf("parsing PROXY_DATABASE_URL: %v", err) + } + u.Path = "/" + name + scratch := u.String() + + const instances = 5 + errs := make([]error, instances) + var wg sync.WaitGroup + start := make(chan struct{}) + for i := range instances { + wg.Go(func() { + <-start + db, err := OpenPostgresOrCreate(scratch) + if err == nil { + err = db.MigrateSchema() + _ = db.Close() + } + errs[i] = err + }) + } + close(start) + wg.Wait() + for i, err := range errs { + if err != nil { + t.Errorf("instance %d: %v", i, err) + } + } + + db, err := OpenPostgres(scratch) + if err != nil { + t.Fatalf("OpenPostgres: %v", err) + } + defer func() { _ = db.Close() }() + var n int + if err := db.Get(&n, "SELECT COUNT(*) FROM schema_info"); err != nil || n != 1 { + t.Errorf("schema_info rows = %d, %v; want 1", n, err) + } + if err := db.Get(&n, "SELECT COUNT(*) FROM migrations"); err != nil || n != len(migrations) { + t.Errorf("migrations rows = %d, %v; want %d", n, err, len(migrations)) + } +} + +func waitUntil(t *testing.T, cond func() bool) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for !cond() { + if time.Now().After(deadline) { + t.Fatal("timed out") + } + time.Sleep(10 * time.Millisecond) + } +} + +func TestMigrateAddsJobRuns(t *testing.T) { + forEachDialect(t, func(t *testing.T, db *DB) { + // A database from before the job_runs migration. + if _, err := db.Exec("DROP TABLE job_runs"); err != nil { + t.Fatalf("dropping job_runs: %v", err) + } + if _, err := db.Exec(db.Rebind("DELETE FROM migrations WHERE name = ?"), "015_add_job_runs"); err != nil { + t.Fatalf("forgetting the migration: %v", err) + } + if err := db.MigrateSchema(); err != nil { + t.Fatalf("MigrateSchema: %v", err) + } + if claimed, err := db.ClaimJobRun("mirror-schedule:a", time.Hour); err != nil || !claimed { + t.Fatalf("ClaimJobRun after migration = %v, %v; want claimed", claimed, err) + } + }) +} + +func TestFetchLeases(t *testing.T) { + db := createTestPostgresDB(t) + if db == nil { + t.Skip("PROXY_DATABASE_URL not set") + } + defer func() { _ = db.Close() }() + + acquire := func(owner string, ttl time.Duration) bool { + t.Helper() + ok, err := db.AcquireFetchLease("k", owner, ttl) + if err != nil { + t.Fatalf("AcquireFetchLease(%s): %v", owner, err) + } + return ok + } + if !acquire("a", time.Minute) { + t.Fatal("a could not take a free lease") + } + if acquire("b", time.Minute) { + t.Fatal("b took a lease a holds") + } + if held, err := db.RenewFetchLease("k", "b", time.Minute); err != nil || held { + t.Fatalf("b renewed a's lease: %v, %v", held, err) + } + if held, err := db.RenewFetchLease("k", "a", time.Minute); err != nil || !held { + t.Fatalf("a could not renew its lease: %v, %v", held, err) + } + // Only the owner can release it. + if err := db.ReleaseFetchLease("k", "b"); err != nil { + t.Fatal(err) + } + if acquire("b", time.Minute) { + t.Fatal("b's release dropped a's lease") + } + if err := db.ReleaseFetchLease("k", "a"); err != nil { + t.Fatal(err) + } + if !acquire("b", time.Millisecond) { + t.Fatal("b could not take the lease a released") + } + time.Sleep(20 * time.Millisecond) + if !acquire("c", time.Millisecond) { + t.Fatal("c could not take b's expired lease") + } + time.Sleep(20 * time.Millisecond) + if n, err := db.DeleteExpiredFetchLeases(0); err != nil || n != 1 { + t.Fatalf("DeleteExpiredFetchLeases = %d, %v; want 1", n, err) + } +} + +func TestFetchLeaseFailure(t *testing.T) { + db := createTestPostgresDB(t) + if db == nil { + t.Skip("PROXY_DATABASE_URL not set") + } + defer func() { _ = db.Close() }() + + if ok, err := db.AcquireFetchLease("k", "a", time.Minute); err != nil || !ok { + t.Fatalf("a could not take a free lease: %v, %v", ok, err) + } + if owner, failed, ok, err := db.FetchLeaseHolder("k"); err != nil || !ok || owner != "a" || failed { + t.Fatalf("FetchLeaseHolder = %q, failed %v, ok %v, %v; want a's live lease", owner, failed, ok, err) + } + // A failed lease stays readable for those waiting on it, and is free to + // take. + if err := db.FailFetchLease("k", "b"); err != nil { + t.Fatal(err) + } + if _, failed, _, _ := db.FetchLeaseHolder("k"); failed { + t.Fatal("b failed a's lease") + } + if err := db.FailFetchLease("k", "a"); err != nil { + t.Fatal(err) + } + if owner, failed, ok, err := db.FetchLeaseHolder("k"); err != nil || !ok || owner != "a" || !failed { + t.Fatalf("FetchLeaseHolder = %q, failed %v, ok %v, %v; want a's failed lease", owner, failed, ok, err) + } + if ok, err := db.AcquireFetchLease("k", "b", time.Minute); err != nil || !ok { + t.Fatalf("b could not take a's failed lease: %v, %v", ok, err) + } + if owner, failed, _, _ := db.FetchLeaseHolder("k"); owner != "b" || failed { + t.Fatalf("after takeover: owner %q, failed %v", owner, failed) + } + if err := db.ReleaseFetchLease("k", "b"); err != nil { + t.Fatal(err) + } + if _, _, ok, err := db.FetchLeaseHolder("k"); err != nil || ok { + t.Fatalf("FetchLeaseHolder after release: ok %v, %v", ok, err) + } +} diff --git a/internal/database/schema.go b/internal/database/schema.go index dc9fc80a..74cc00fb 100644 --- a/internal/database/schema.go +++ b/internal/database/schema.go @@ -1,6 +1,7 @@ package database import ( + "context" "fmt" "strings" "time" @@ -86,6 +87,18 @@ CREATE TABLE IF NOT EXISTS pending_deletes ( ); CREATE INDEX IF NOT EXISTS idx_pending_deletes_queued_at ON pending_deletes(queued_at); +CREATE TABLE IF NOT EXISTS job_runs ( + name TEXT NOT NULL PRIMARY KEY, + started_at DATETIME NOT NULL +); + +CREATE TABLE IF NOT EXISTS fetch_leases ( + key TEXT NOT NULL PRIMARY KEY, + owner TEXT NOT NULL, + expires_at DATETIME NOT NULL, + failed BOOLEAN NOT NULL DEFAULT FALSE +); + CREATE TABLE IF NOT EXISTS vulnerabilities ( id INTEGER PRIMARY KEY, vuln_id TEXT NOT NULL, @@ -210,6 +223,18 @@ CREATE TABLE IF NOT EXISTS pending_deletes ( ); CREATE INDEX IF NOT EXISTS idx_pending_deletes_queued_at ON pending_deletes(queued_at); +CREATE TABLE IF NOT EXISTS job_runs ( + name TEXT NOT NULL PRIMARY KEY, + started_at TIMESTAMP NOT NULL +); + +CREATE TABLE IF NOT EXISTS fetch_leases ( + key TEXT NOT NULL PRIMARY KEY, + owner TEXT NOT NULL, + expires_at TIMESTAMP NOT NULL, + failed BOOLEAN NOT NULL DEFAULT FALSE +); + CREATE TABLE IF NOT EXISTS vulnerabilities ( id SERIAL PRIMARY KEY, vuln_id TEXT NOT NULL, @@ -420,6 +445,8 @@ var migrations = []migration{ {"012_add_artifact_source", migrateAddArtifactSource}, {"013_add_artifact_manifest", migrateAddArtifactManifest}, {"014_add_metadata_last_accessed", migrateAddMetadataLastAccessed}, + {"015_add_job_runs", migrateAddJobRuns}, + {"016_add_fetch_leases", migrateAddFetchLeases}, } // isTableNotFound returns true if the error indicates a missing table. @@ -489,9 +516,19 @@ func (db *DB) recordAllMigrations() error { return nil } +// schemaLock serialises schema creation and migrations across instances +// sharing a Postgres database. +const schemaLock = "schema" + // MigrateSchema applies any unapplied migrations in order. // For a fully migrated database this executes a single SELECT query. +// Instances sharing a Postgres database run it one after the other, so each +// sees the migrations the one before it recorded. func (db *DB) MigrateSchema() error { + return db.WithLock(context.Background(), schemaLock, db.migrateSchema) +} + +func (db *DB) migrateSchema() error { applied, err := db.appliedMigrations() if err != nil { return err @@ -709,6 +746,42 @@ func migrateAddPendingDeletes(db *DB) error { return nil } +// migrateAddJobRuns creates the table that records when a scheduled job +// last started, so instances sharing the database run it once per interval. +func migrateAddJobRuns(db *DB) error { + ts := sqliteDatetime + if db.dialect == DialectPostgres { + ts = postgresTimestamp + } + query := fmt.Sprintf(`CREATE TABLE IF NOT EXISTS job_runs ( + name TEXT NOT NULL PRIMARY KEY, + started_at %s NOT NULL + )`, ts) + if _, err := db.Exec(query); err != nil { + return fmt.Errorf("creating job_runs table: %w", err) + } + return nil +} + +// migrateAddFetchLeases creates the table instances sharing a Postgres +// database use to fetch each cache miss from upstream only once. +func migrateAddFetchLeases(db *DB) error { + ts := sqliteDatetime + if db.dialect == DialectPostgres { + ts = postgresTimestamp + } + query := fmt.Sprintf(`CREATE TABLE IF NOT EXISTS fetch_leases ( + key TEXT NOT NULL PRIMARY KEY, + owner TEXT NOT NULL, + expires_at %s NOT NULL, + failed BOOLEAN NOT NULL DEFAULT FALSE + )`, ts) + if _, err := db.Exec(query); err != nil { + return fmt.Errorf("creating fetch_leases table: %w", err) + } + return nil +} + // migrateAddPendingDeletesQueuedAtIndex indexes the column reclaim orders // the queue by, which grows large after a retention sweep. func migrateAddPendingDeletesQueuedAtIndex(db *DB) error { diff --git a/internal/handler/fetch_lease.go b/internal/handler/fetch_lease.go new file mode 100644 index 00000000..e49b7efc --- /dev/null +++ b/internal/handler/fetch_lease.go @@ -0,0 +1,183 @@ +package handler + +import ( + "context" + "crypto/rand" + "crypto/sha256" + "encoding/hex" + "time" + + "github.com/git-pkgs/proxy/internal/database" + "github.com/git-pkgs/proxy/internal/metrics" +) + +// Instances sharing a Postgres database coalesce cache misses through fetch +// leases, as coalesceFetch and coalesceMetadata do within one instance. The +// instance that runs the fetch holds the lease for the miss and renews it +// while it downloads. Another instance missing the same key waits for the +// stored result and serves that, or takes the lease over if the holder +// releases it without storing anything or stops renewing it. +var ( + fetchLeaseTTL = 30 * time.Second + fetchLeaseRenew = 10 * time.Second + // The wait for another instance's fetch polls the cache, starting + // quickly for small files and backing off for large ones. + fetchLeasePollMin = 50 * time.Millisecond + fetchLeasePollMax = 2 * time.Second +) + +// fetchLeaseKey turns a coalescing key, which can hold long URLs and NUL +// separators, into a fixed-size table key. +func fetchLeaseKey(kind, key string) string { + sum := sha256.Sum256([]byte(kind + "\x00" + key)) + return hex.EncodeToString(sum[:]) +} + +func newLeaseOwner() string { + var b [16]byte + _, _ = rand.Read(b[:]) + return hex.EncodeToString(b[:]) +} + +// fetchLeasesEnabled reports whether misses are coalesced across instances. +func (p *Proxy) fetchLeasesEnabled() bool { + return p.DB != nil && p.DB.Dialect() == database.DialectPostgres +} + +// leaseOutcome says how a caller goes on after awaitFetchLease. +type leaseOutcome int + +const ( + // leaseFetch: this instance fetches, and calls release afterwards. + leaseFetch leaseOutcome = iota + // leaseStored: ready found the result another instance stored. + leaseStored + // leaseFailed: the fetch this caller waited on failed. It fetches + // without the lease, as every instance that waited does at the same + // time, or serves a stale copy, rather than queueing behind the others. + leaseFailed +) + +// awaitFetchLease takes the lease on key before this instance fetches it from +// upstream. While another instance holds the lease, it polls ready, which +// reports whether that instance's result is now stored and, if so, keeps it +// for the caller. release is to be called with whether the fetch failed once +// it is done; err is set when ctx ended while waiting. +// +// A failing lease query is logged and the fetch goes ahead without a lease, +// so a database hiccup costs a duplicate fetch rather than the request. +func (p *Proxy) awaitFetchLease(ctx context.Context, kind, key string, ready func() bool) (release func(failed bool), outcome leaseOutcome, err error) { + noop := func(bool) {} + if !p.fetchLeasesEnabled() { + return noop, leaseFetch, nil + } + leaseKey := fetchLeaseKey(kind, key) + owner := newLeaseOwner() + delay := fetchLeasePollMin + waited := false + for { + acquired, err := p.DB.AcquireFetchLease(leaseKey, owner, fetchLeaseTTL) + if err != nil { + p.Logger.Warn("fetch lease unavailable, fetching without it", "kind", kind, "error", err) + return noop, leaseFetch, nil + } + if acquired { + // The holder may have stored the result and released the lease + // between this caller's last poll and taking it. + if waited && ready() { + p.releaseFetchLease(leaseKey, owner) + return noop, leaseStored, nil + } + return p.holdFetchLease(leaseKey, owner), leaseFetch, nil + } + if !waited { + metrics.RecordFetchLeaseWait(kind) + waited = true + } + outcome, done, err := p.waitForLeaseHolder(ctx, kind, leaseKey, ready, &delay) + if done || err != nil { + return noop, outcome, err + } + } +} + +// waitForLeaseHolder polls ready while another instance holds the lease on +// leaseKey. It is done when ready found the result, when the fetch it waited +// on failed, or when the lease can't be read; otherwise it returns once the +// lease is free to take. +func (p *Proxy) waitForLeaseHolder(ctx context.Context, kind, leaseKey string, ready func() bool, delay *time.Duration) (outcome leaseOutcome, done bool, err error) { + holder := "" + for { + current, failed, held, err := p.DB.FetchLeaseHolder(leaseKey) + if err != nil { + p.Logger.Warn("fetch lease unavailable, fetching without it", "kind", kind, "error", err) + return leaseFetch, true, nil + } + if failed && current == holder { + return leaseFailed, true, nil + } + if !held || failed { + // Released, expired, or failed before this caller waited on + // it: free to take. + return leaseFetch, false, nil + } + holder = current + timer := time.NewTimer(*delay) + select { + case <-ctx.Done(): + timer.Stop() + return leaseFetch, true, ctx.Err() + case <-timer.C: + } + if ready() { + return leaseStored, true, nil + } + *delay = min(*delay+*delay, fetchLeasePollMax) + } +} + +// holdFetchLease renews owner's lease on leaseKey until the returned release +// is called. release drops the lease, or marks it failed so instances +// waiting on it stop waiting. +func (p *Proxy) holdFetchLease(leaseKey, owner string) func(failed bool) { + stop := make(chan struct{}) + done := make(chan struct{}) + go func() { + defer close(done) + ticker := time.NewTicker(fetchLeaseRenew) + defer ticker.Stop() + for { + select { + case <-stop: + return + case <-ticker.C: + // A lease that lapsed, say after a long pause, lets another + // instance fetch too. That costs a duplicate fetch, nothing + // else, so the fetch here carries on either way. + if held, err := p.DB.RenewFetchLease(leaseKey, owner, fetchLeaseTTL); err != nil || !held { + p.Logger.Debug("fetch lease lapsed", "error", err) + } + } + } + }() + return func(failed bool) { + close(stop) + <-done + if !failed { + p.releaseFetchLease(leaseKey, owner) + return + } + if err := p.DB.FailFetchLease(leaseKey, owner); err != nil { + // Waiting instances take the lease once it expires. + p.Logger.Warn("failed to mark fetch lease failed", "error", err) + } + } +} + +func (p *Proxy) releaseFetchLease(leaseKey, owner string) { + if err := p.DB.ReleaseFetchLease(leaseKey, owner); err != nil { + // The lease expires on its own; until then other instances wait + // on it and then fetch. + p.Logger.Warn("failed to release fetch lease", "error", err) + } +} diff --git a/internal/handler/fetch_lease_test.go b/internal/handler/fetch_lease_test.go new file mode 100644 index 00000000..0be8d42d --- /dev/null +++ b/internal/handler/fetch_lease_test.go @@ -0,0 +1,428 @@ +package handler + +import ( + "context" + "errors" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/git-pkgs/proxy/internal/database" + "github.com/git-pkgs/proxy/internal/metrics" + "github.com/git-pkgs/registries/fetch" + "github.com/prometheus/client_golang/prometheus/testutil" +) + +// gateFetcher counts upstream fetches, signals entered when the first one +// starts and holds every fetch until release is closed. +type gateFetcher struct { + calls atomic.Int64 + content string + err error + entered chan struct{} + release chan struct{} + once sync.Once + opened sync.Once +} + +// newGateFetcher returns a gateFetcher that is released when the test ends +// at the latest, so a failing test does not leave fetches hanging. +func newGateFetcher(t *testing.T, content string, err error) *gateFetcher { + f := &gateFetcher{content: content, err: err, entered: make(chan struct{}), release: make(chan struct{})} + t.Cleanup(f.open) + return f +} + +func (f *gateFetcher) open() { f.opened.Do(func() { close(f.release) }) } + +func (f *gateFetcher) Fetch(ctx context.Context, url string) (*fetch.Artifact, error) { + return f.FetchWithHeaders(ctx, url, nil) +} + +func (f *gateFetcher) FetchWithHeaders(ctx context.Context, _ string, _ http.Header) (*fetch.Artifact, error) { + f.calls.Add(1) + f.once.Do(func() { close(f.entered) }) + select { + case <-f.release: + case <-ctx.Done(): + return nil, ctx.Err() + } + if f.err != nil { + return nil, f.err + } + return &fetch.Artifact{Body: io.NopCloser(strings.NewReader(f.content)), ContentType: "application/gzip"}, nil +} + +func (f *gateFetcher) Head(context.Context, string) (int64, string, error) { return 0, "", nil } + +// sharedInstances returns two proxies sharing one Postgres database and one +// storage, as two replicas would. It skips without PROXY_DATABASE_URL. +func sharedInstances(t *testing.T) (*Proxy, *Proxy, *database.DB) { + t.Helper() + db := openTestPostgres(t) + if db == nil { + t.Skip("PROXY_DATABASE_URL not set") + } + store := newMockStorage() + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + a := NewProxy(db, store, &mockFetcher{}, fetch.NewResolver(), logger) + b := NewProxy(db, store, &mockFetcher{}, fetch.NewResolver(), logger) + return a, b, db +} + +const leaseTestURL = "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz" + +func fetchLeftPad(p *Proxy) (string, error) { + return fetchLeftPadCtx(context.Background(), p) +} + +func fetchLeftPadCtx(ctx context.Context, p *Proxy) (string, error) { + res, err := p.GetOrFetchArtifactFromURL(ctx, "npm", "left-pad", "1.3.0", "left-pad-1.3.0.tgz", leaseTestURL) + if err != nil { + return "", err + } + defer func() { _ = res.Reader.Close() }() + b, err := io.ReadAll(res.Reader) + return string(b), err +} + +type fetchResult struct { + body string + err error +} + +func goFetchLeftPad(ctx context.Context, p *Proxy) chan fetchResult { + ch := make(chan fetchResult, 1) + go func() { + body, err := fetchLeftPadCtx(ctx, p) + ch <- fetchResult{body, err} + }() + return ch +} + +// waitForLeaseWaits returns once the lease wait counter for kind has passed +// from. +func waitForLeaseWaits(t *testing.T, kind string, from float64) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for testutil.ToFloat64(metrics.FetchLeaseWaits.WithLabelValues(kind)) <= from { + if time.Now().After(deadline) { + t.Fatal("timed out waiting for the second instance to wait on the lease") + } + time.Sleep(time.Millisecond) + } +} + +// waitForArtifactWaiters returns once n callers have joined p's fetch of +// left-pad. +func waitForArtifactWaiters(t *testing.T, p *Proxy, n int) { + t.Helper() + deadline := time.Now().Add(5 * time.Second) + for { + p.fetchMu.Lock() + joined := 0 + for _, f := range p.inFlight { + joined += f.waiters + } + p.fetchMu.Unlock() + if joined >= n { + return + } + if time.Now().After(deadline) { + t.Fatal("timed out waiting for a caller to join the fetch") + } + time.Sleep(time.Millisecond) + } +} + +func TestFetchLeaseCoalescesArtifactAcrossInstances(t *testing.T) { + a, b, _ := sharedInstances(t) + const content = "left-pad tarball" + fa := newGateFetcher(t, content, nil) + fb := newGateFetcher(t, content, nil) + fb.open() + a.Fetcher, b.Fetcher = fa, fb + + waits := testutil.ToFloat64(metrics.FetchLeaseWaits.WithLabelValues(metrics.CacheKindArtifact)) + resA := goFetchLeftPad(context.Background(), a) + <-fa.entered + resB := goFetchLeftPad(context.Background(), b) + waitForLeaseWaits(t, metrics.CacheKindArtifact, waits) + fa.open() + + for name, ch := range map[string]chan fetchResult{"a": resA, "b": resB} { + r := <-ch + if r.err != nil || r.body != content { + t.Errorf("instance %s: body %q, err %v", name, r.body, r.err) + } + } + if got := fa.calls.Load() + fb.calls.Load(); got != 1 { + t.Errorf("upstream fetches = %d (a %d, b %d), want 1", got, fa.calls.Load(), fb.calls.Load()) + } +} + +// TestFetchLeaseFailedFetchLetsOtherInstanceFetch checks that an instance +// waiting on a fetch that fails takes the lease over and fetches itself, +// rather than waiting until the lease expires. +func TestFetchLeaseFailedFetchLetsOtherInstanceFetch(t *testing.T) { + a, b, _ := sharedInstances(t) + const content = "left-pad tarball" + fa := newGateFetcher(t, "", errors.New("upstream down")) + fb := newGateFetcher(t, content, nil) + fb.open() + a.Fetcher, b.Fetcher = fa, fb + + waits := testutil.ToFloat64(metrics.FetchLeaseWaits.WithLabelValues(metrics.CacheKindArtifact)) + resA := goFetchLeftPad(context.Background(), a) + <-fa.entered + resB := goFetchLeftPad(context.Background(), b) + waitForLeaseWaits(t, metrics.CacheKindArtifact, waits) + started := time.Now() + fa.open() + + if r := <-resA; r.err == nil { + t.Error("instance a: want the upstream error") + } + r := <-resB + if r.err != nil || r.body != content { + t.Fatalf("instance b: body %q, err %v", r.body, r.err) + } + if fb.calls.Load() != 1 { + t.Errorf("b's upstream fetches = %d, want 1", fb.calls.Load()) + } + if waited := time.Since(started); waited >= fetchLeaseTTL { + t.Errorf("b waited %s, as long as the lease lasts", waited) + } +} + +// TestFetchLeaseExpiredIsTakenOver checks that a lease left by an instance +// that stopped while fetching holds the others up only until it expires. +func TestFetchLeaseExpiredIsTakenOver(t *testing.T) { + _, b, db := sharedInstances(t) + const content = "left-pad tarball" + fb := newGateFetcher(t, content, nil) + fb.open() + b.Fetcher = fb + + key := artifactCoalesceKey("pkg:npm/left-pad@1.3.0", "left-pad-1.3.0.tgz", leaseTestURL, "", "") + if ok, err := db.AcquireFetchLease(fetchLeaseKey(metrics.CacheKindArtifact, key), "stopped-instance", 300*time.Millisecond); err != nil || !ok { + t.Fatalf("taking the lease for the stopped instance: %v, %v", ok, err) + } + started := time.Now() + body, err := fetchLeftPad(b) + if err != nil || body != content { + t.Fatalf("body %q, err %v", body, err) + } + if waited := time.Since(started); waited < 250*time.Millisecond { + t.Errorf("fetched after %s, before the lease expired; is the test key right?", waited) + } +} + +func TestFetchLeaseCoalescesMetadataAcrossInstances(t *testing.T) { + a, b, _ := sharedInstances(t) + u := newMetadataUpstream(t, http.StatusOK) + var released sync.Once + release := func() { released.Do(func() { close(u.release) }) } + t.Cleanup(release) + for _, p := range []*Proxy{a, b} { + p.HTTPClient = u.Client() + p.CacheMetadata = true + p.MetadataTTL = time.Minute + } + + waits := testutil.ToFloat64(metrics.FetchLeaseWaits.WithLabelValues(metrics.CacheKindMetadata)) + resA := startMetadataCalls(a, context.Background(), 1, u.URL+"/left-pad", "application/json")[0] + <-u.entered + resB := startMetadataCalls(b, context.Background(), 1, u.URL+"/left-pad", "application/json")[0] + waitForLeaseWaits(t, metrics.CacheKindMetadata, waits) + release() + + for name, ch := range map[string]chan metadataCall{"a": resA, "b": resB} { + r := <-ch + if r.err != nil || !strings.Contains(string(r.body), "application/json") { + t.Errorf("instance %s: body %q, err %v", name, r.body, r.err) + } + } + if got := u.calls.Load(); got != 1 { + t.Errorf("upstream metadata requests = %d, want 1", got) + } +} + +// TestFetchLeaseNotUsedWithoutMetadataCache checks that metadata misses are +// not held up when there is no stored copy to wait for. +func TestFetchLeaseNotUsedWithoutMetadataCache(t *testing.T) { + a, b, _ := sharedInstances(t) + u := newMetadataUpstream(t, http.StatusOK) + a.HTTPClient, b.HTTPClient = u.Client(), u.Client() + + resA := startMetadataCalls(a, context.Background(), 1, u.URL+"/left-pad", "application/json")[0] + <-u.entered + resB := startMetadataCalls(b, context.Background(), 1, u.URL+"/left-pad", "application/json")[0] + deadline := time.Now().Add(5 * time.Second) + for u.calls.Load() < 2 { + if time.Now().After(deadline) { + t.Fatal("b did not fetch while a was fetching") + } + time.Sleep(time.Millisecond) + } + close(u.release) + for _, ch := range []chan metadataCall{resA, resB} { + if r := <-ch; r.err != nil { + t.Errorf("unexpected error: %v", r.err) + } + } +} + +// TestFetchLeaseWaitOutlivesDetachedCaller checks that a caller waiting on +// another instance's fetch keeps waiting after its client goes away when its +// fetch is detached, so callers that joined it still get the artifact. +func TestFetchLeaseWaitOutlivesDetachedCaller(t *testing.T) { + a, b, _ := sharedInstances(t) + const content = "left-pad tarball" + fa := newGateFetcher(t, content, nil) + fb := newGateFetcher(t, content, nil) + fb.open() + a.Fetcher, b.Fetcher = fa, fb + + waits := testutil.ToFloat64(metrics.FetchLeaseWaits.WithLabelValues(metrics.CacheKindArtifact)) + resA := goFetchLeftPad(context.Background(), a) + <-fa.entered + leadCtx, cancelLead := context.WithCancel(DetachUpstreamFetch(context.Background())) + defer cancelLead() + resLead := goFetchLeftPad(leadCtx, b) + waitForLeaseWaits(t, metrics.CacheKindArtifact, waits) + resJoined := goFetchLeftPad(context.Background(), b) + waitForArtifactWaiters(t, b, 1) + cancelLead() + fa.open() + <-resLead + + for name, ch := range map[string]chan fetchResult{"a": resA, "b's joined caller": resJoined} { + r := <-ch + if r.err != nil || r.body != content { + t.Errorf("%s: body %q, err %v", name, r.body, r.err) + } + } + if got := fb.calls.Load(); got != 0 { + t.Errorf("b fetched %d times, want 0", got) + } +} + +// TestFetchLeaseFailedFetchWaitersFetchInParallel checks that once a fetch +// fails, the instances waiting on it fetch at the same time rather than one +// after another, so a hanging upstream costs each one timeout. +func TestFetchLeaseFailedFetchWaitersFetchInParallel(t *testing.T) { + a, b, db := sharedInstances(t) + c := NewProxy(db, b.Storage, &mockFetcher{}, fetch.NewResolver(), b.Logger) + const content = "left-pad tarball" + fa := newGateFetcher(t, "", errors.New("upstream down")) + fb := newGateFetcher(t, content, nil) + fc := newGateFetcher(t, content, nil) + a.Fetcher, b.Fetcher, c.Fetcher = fa, fb, fc + + waits := testutil.ToFloat64(metrics.FetchLeaseWaits.WithLabelValues(metrics.CacheKindArtifact)) + resA := goFetchLeftPad(context.Background(), a) + <-fa.entered + resB := goFetchLeftPad(context.Background(), b) + waitForLeaseWaits(t, metrics.CacheKindArtifact, waits) + resC := goFetchLeftPad(context.Background(), c) + waitForLeaseWaits(t, metrics.CacheKindArtifact, waits+1) + fa.open() + if r := <-resA; r.err == nil { + t.Fatal("instance a: want the upstream error") + } + + for name, entered := range map[string]chan struct{}{"b": fb.entered, "c": fc.entered} { + select { + case <-entered: + case <-time.After(5 * time.Second): + t.Fatalf("%s did not fetch while the other waiting instance was fetching", name) + } + } + fb.open() + fc.open() + for name, ch := range map[string]chan fetchResult{"b": resB, "c": resC} { + if r := <-ch; r.err != nil || r.body != content { + t.Errorf("instance %s: body %q, err %v", name, r.body, r.err) + } + } +} + +// TestFetchLeaseFailedMetadataServesStale checks that an instance waiting on +// a metadata fetch that fails serves its stale copy at once, as the holder +// does, instead of asking the failing upstream again. +func TestFetchLeaseFailedMetadataServesStale(t *testing.T) { + a, b, _ := sharedInstances(t) + var status atomic.Int32 + status.Store(http.StatusOK) + var calls atomic.Int32 + entered := make(chan struct{}, 1) + release := make(chan struct{}) + var released sync.Once + open := func() { released.Do(func() { close(release) }) } + t.Cleanup(open) + upstream := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls.Add(1) + if code := int(status.Load()); code != http.StatusOK { + entered <- struct{}{} + <-release + w.WriteHeader(code) + return + } + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"name":"left-pad"}`)) + })) + t.Cleanup(upstream.Close) + for _, p := range []*Proxy{a, b} { + p.HTTPClient = upstream.Client() + p.CacheMetadata = true + p.MetadataTTL = 50 * time.Millisecond + } + fetchMeta := func(p *Proxy) chan metadataCall { + ch := make(chan metadataCall, 1) + go func() { + body, _, err := p.FetchOrCacheMetadata(context.Background(), "npm", "left-pad", upstream.URL+"/left-pad", "application/json") + ch <- metadataCall{body, err} + }() + return ch + } + if r := <-fetchMeta(a); r.err != nil { + t.Fatalf("seeding the cache: %v", r.err) + } + time.Sleep(100 * time.Millisecond) + + status.Store(http.StatusServiceUnavailable) + waits := testutil.ToFloat64(metrics.FetchLeaseWaits.WithLabelValues(metrics.CacheKindMetadata)) + resA := fetchMeta(a) + <-entered + resB := fetchMeta(b) + waitForLeaseWaits(t, metrics.CacheKindMetadata, waits) + open() + + for name, ch := range map[string]chan metadataCall{"a": resA, "b": resB} { + if r := <-ch; r.err != nil || string(r.body) != `{"name":"left-pad"}` { + t.Errorf("instance %s: body %q, err %v; want the stale copy", name, r.body, r.err) + } + } + if got := calls.Load(); got != 2 { + t.Errorf("upstream requests = %d, want 2: the seed and a's failed refresh", got) + } +} + +func TestFetchLeaseNotUsedOnSQLite(t *testing.T) { + proxy, _, _, _ := setupTestProxy(t) + release, outcome, err := proxy.awaitFetchLease(context.Background(), metrics.CacheKindArtifact, "k", func() bool { + t.Error("ready polled on SQLite") + return false + }) + if err != nil || outcome != leaseFetch { + t.Fatalf("awaitFetchLease = %v, %v; want to fetch at once", outcome, err) + } + release(false) +} diff --git a/internal/handler/handler.go b/internal/handler/handler.go index 022f3d8b..542dd47a 100644 --- a/internal/handler/handler.go +++ b/internal/handler/handler.go @@ -744,6 +744,10 @@ type inflightFetch struct { done chan struct{} val fetchedArtifact err error + + // waiters counts callers that joined the fetch, guarded by fetchMu. + // Tests read it to know a caller has joined. + waiters int } // coalesceFetch runs commit at most once for concurrent callers sharing key, @@ -776,7 +780,9 @@ func (p *Proxy) coalesceFetch(ctx context.Context, key string, recheck func() (a p.inFlight = make(map[string]*inflightFetch) } f, joined := p.inFlight[key] - if !joined { + if joined { + f.waiters++ + } else { f = &inflightFetch{done: make(chan struct{})} p.inFlight[key] = f } @@ -815,12 +821,49 @@ func (p *Proxy) runSharedFetch(ctx context.Context, key string, f *inflightFetch // in between would otherwise be repeated. Serve that record only if its // bytes are still present: a record can outlive them, and refetching is // the same recovery the cache lookup makes. - if stored, path, ok := recheck(); ok { - if res, err := p.openStoredArtifact(ctx, stored, path); err == nil { - f.val, f.err = fetchedArtifact{artifact: stored, storagePath: path}, nil - return res, nil + served := func(ctx context.Context) (*CacheResult, bool) { + stored, path, ok := recheck() + if !ok { + return nil, false } + res, err := p.openStoredArtifact(ctx, stored, path) + if err != nil { + return nil, false + } + f.val, f.err = fetchedArtifact{artifact: stored, storagePath: path}, nil + return res, true + } + if res, ok := served(ctx); ok { + return res, nil + } + + // Another instance sharing the database may be fetching the same + // artifact. Wait for it to store the artifact rather than fetch it too. + // The wait makes no upstream request, so a detached fetch waits past + // its client too, bounded by the holder's fetch and the lease expiry; + // otherwise callers that joined this one would get its cancellation. + waitCtx := ctx + if upstreamFetchDetached(ctx) { + waitCtx = context.WithoutCancel(ctx) + } + var waitedFor *CacheResult + release, outcome, err := p.awaitFetchLease(waitCtx, metrics.CacheKindArtifact, key, func() bool { + res, ok := served(waitCtx) + waitedFor = res + return ok + }) + if err != nil { + f.err = err + return nil, err + } + if outcome == leaseStored { + return waitedFor, nil } + // With leaseFailed, release is a no-op and this instance fetches on its + // own, so a failing upstream costs each instance one attempt in + // parallel. + failed := true + defer func() { release(failed) }() commitCtx := ctx if upstreamFetchDetached(ctx) && p.acquireDetachSlot() { @@ -828,6 +871,7 @@ func (p *Proxy) runSharedFetch(ctx context.Context, key string, f *inflightFetch commitCtx = context.WithoutCancel(ctx) } stored, path, err := commit(commitCtx) + failed = err != nil f.val, f.err = fetchedArtifact{artifact: stored, storagePath: path}, err if err != nil { return nil, err @@ -1369,7 +1413,38 @@ func (p *Proxy) coalescedMetadataMiss(ctx context.Context, ecosystem, cacheKey, if hit != nil { return *hit } - return p.fetchMetadataFromUpstream(fetchCtx, ecosystem, cacheKey, upstreamURL, accept, acceptEncoding, validate, entry) + // Another instance sharing the database may be fetching the same + // document. Only a stored copy within TTL can be served after + // waiting, so without that there is nothing to wait for. + release := func(bool) {} + if p.CacheMetadata && p.MetadataTTL > 0 { + var outcome leaseOutcome + var err error + release, outcome, err = p.awaitFetchLease(fetchCtx, metrics.CacheKindMetadata, key, func() bool { + entry, hit = p.cachedMetadataState(fetchCtx, ecosystem, cacheKey, validate) + return hit != nil + }) + if err != nil { + return metadataResult{err: err} + } + switch outcome { + case leaseStored: + return *hit + case leaseFailed: + // Serve the stale copy at once, as the holder did, rather + // than trying the failing upstream again. Without one, fetch. + if entry != nil { + if res := p.staleMetadata(fetchCtx, ecosystem, cacheKey, validate, entry, errFetchFailedElsewhere); res.err == nil { + return res + } + } + case leaseFetch: + } + } + res := metadataResult{err: errSharedFetchAbandoned} + defer func() { release(res.err != nil || res.stale) }() + res = p.fetchMetadataFromUpstream(fetchCtx, ecosystem, cacheKey, upstreamURL, accept, acceptEncoding, validate, entry) + return res }) // The fetch ran the first caller's validate. A caller that joined it runs // its own on the shared bytes, since validate may also decode them for it. @@ -1428,7 +1503,16 @@ func (p *Proxy) fetchMetadataFromUpstream(ctx context.Context, ecosystem, cacheK return metadataResult{body: meta.body, contentType: meta.contentType, contentEncoding: meta.contentEncoding, digest: digest} } - // Upstream failed -- fall back to cache if available + return p.staleMetadata(ctx, ecosystem, cacheKey, validate, entry, err) +} + +// errFetchFailedElsewhere is the upstream error behind a stale copy served +// because another instance's fetch for it failed. +var errFetchFailedElsewhere = errors.New("upstream fetch failed on another instance") + +// staleMetadata falls back to the cached copy in entry after the upstream +// fetch failed with err. +func (p *Proxy) staleMetadata(ctx context.Context, ecosystem, cacheKey string, validate func([]byte) error, entry *database.MetadataCacheEntry, err error) metadataResult { if !p.CacheMetadata || entry == nil { return metadataResult{err: fmt.Errorf("upstream failed and no cached metadata: %w", err)} } diff --git a/internal/metrics/metrics.go b/internal/metrics/metrics.go index 8fb857cb..708b5b30 100644 --- a/internal/metrics/metrics.go +++ b/internal/metrics/metrics.go @@ -86,6 +86,14 @@ var ( []string{"ecosystem"}, ) + FetchLeaseWaits = prometheus.NewCounterVec( + prometheus.CounterOpts{ + Name: "proxy_fetch_lease_waits_total", + Help: "Cache misses that waited for another instance's upstream fetch, by kind (artifact or metadata)", + }, + []string{"kind"}, + ) + // Upstream metrics UpstreamFetchDuration = prometheus.NewHistogramVec( prometheus.HistogramOpts{ @@ -303,6 +311,7 @@ func init() { CachedArtifacts, ArtifactsEvicted, MetadataEvicted, + FetchLeaseWaits, UpstreamFetchDuration, UpstreamErrors, CircuitBreakerState, @@ -400,6 +409,12 @@ func RecordMetadataEvicted(ecosystem string) { MetadataEvicted.WithLabelValues(purl.NormalizeEcosystem(ecosystem)).Inc() } +// RecordFetchLeaseWait counts a cache miss that found another instance +// sharing the database already fetching it. +func RecordFetchLeaseWait(kind string) { + FetchLeaseWaits.WithLabelValues(kind).Inc() +} + // RecordArtifactCacheMiss counts an artifact that had to be fetched upstream. func RecordArtifactCacheMiss(ecosystem string) { CacheMisses.WithLabelValues(purl.NormalizeEcosystem(ecosystem), CacheKindArtifact).Inc() diff --git a/internal/mirror/schedule.go b/internal/mirror/schedule.go index 4ab0d2b2..55d6ab4a 100644 --- a/internal/mirror/schedule.go +++ b/internal/mirror/schedule.go @@ -2,6 +2,9 @@ package mirror import ( "context" + "crypto/sha256" + "encoding/hex" + "fmt" "log/slog" "math/rand/v2" "sync" @@ -27,6 +30,11 @@ type Scheduler struct { schedules []Schedule logger *slog.Logger wg sync.WaitGroup + // Claim, when set, reports whether this instance should run a schedule + // that no instance sharing the database started within minGap. Each + // instance keeps its own timer, so without it every instance would run + // every schedule. + Claim func(key string, minGap time.Duration) (bool, error) // randN returns a value in [0, n); tests replace it. randN func(n int64) int64 } @@ -75,6 +83,20 @@ func (s *Scheduler) loop(ctx context.Context, index int, schedule Schedule) { } func (s *Scheduler) run(ctx context.Context, index int, schedule Schedule) { + if s.Claim != nil { + // Later runs come at least interval - 2*spread apart, so a run + // started more recently than that was another instance's. + minGap := schedule.Interval - 2*(schedule.Interval/scheduleJitter) //nolint:mnd // two spreads of margin + claimed, err := s.Claim(scheduleKey(schedule), minGap) + if err != nil { + s.logger.Warn("mirror schedule: failed to claim run, skipping it", "schedule", index, "error", err) + return + } + if !claimed { + s.logger.Debug("mirror schedule: another instance ran it, skipping", "schedule", index) + return + } + } started := time.Now() progress, err := s.mirror.Run(ctx, &PURLSource{PURLs: schedule.PURLs, Platforms: schedule.Platforms}) if ctx.Err() != nil { @@ -103,3 +125,18 @@ func scheduleDelay(interval time.Duration, first bool, randN func(int64) int64) spread := interval / scheduleJitter return interval - spread + time.Duration(randN(int64(2*spread)+1)) //nolint:mnd // spread on both sides } + +// scheduleKey names a schedule by what it mirrors and how often, so +// instances with the same configuration agree on it and a changed schedule +// does not inherit the old one's last run. +func scheduleKey(schedule Schedule) string { + h := sha256.New() + for _, purl := range schedule.PURLs { + _, _ = fmt.Fprintf(h, "purl %s\n", purl) + } + for _, platform := range schedule.Platforms { + _, _ = fmt.Fprintf(h, "platform %s\n", platform) + } + _, _ = fmt.Fprintf(h, "interval %s\n", schedule.Interval) + return "mirror-schedule:" + hex.EncodeToString(h.Sum(nil))[:32] +} diff --git a/internal/mirror/schedule_test.go b/internal/mirror/schedule_test.go index c869c8a1..53bdea2e 100644 --- a/internal/mirror/schedule_test.go +++ b/internal/mirror/schedule_test.go @@ -175,3 +175,59 @@ func TestSchedulerSkipsScheduleWithoutInterval(t *testing.T) { t.Errorf("calls = %d, want 0", got) } } + +func TestSchedulerSkipsRunClaimedElsewhere(t *testing.T) { + images := &scheduleImages{} + schedule := Schedule{PURLs: []string{"pkg:docker/alpine@3"}, Interval: time.Hour} + s := newTestScheduler(images, schedule) + var mu sync.Mutex + var keys []string + var gaps []time.Duration + claim := true + s.Claim = func(key string, minGap time.Duration) (bool, error) { + mu.Lock() + defer mu.Unlock() + keys = append(keys, key) + gaps = append(gaps, minGap) + return claim, nil + } + + s.run(context.Background(), 0, schedule) + if got := images.calls.Load(); got != 1 { + t.Fatalf("calls after a claimed run = %d, want 1", got) + } + claim = false + s.run(context.Background(), 0, schedule) + if got := images.calls.Load(); got != 1 { + t.Fatalf("calls after a run claimed elsewhere = %d, want 1", got) + } + s.Claim = func(string, time.Duration) (bool, error) { return false, errors.New("database down") } + s.run(context.Background(), 0, schedule) + if got := images.calls.Load(); got != 1 { + t.Fatalf("calls after a failed claim = %d, want 1", got) + } + + if keys[0] != keys[1] { + t.Errorf("claim keys differ between runs: %q, %q", keys[0], keys[1]) + } + // Runs come at least 0.9 intervals apart; the gap stays below that. + if want := 48 * time.Minute; gaps[0] != want { + t.Errorf("minGap = %s, want %s", gaps[0], want) + } +} + +func TestScheduleKey(t *testing.T) { + base := Schedule{PURLs: []string{"pkg:docker/alpine@3"}, Platforms: []string{"linux/amd64"}, Interval: time.Hour} + if scheduleKey(base) != scheduleKey(Schedule{PURLs: []string{"pkg:docker/alpine@3"}, Platforms: []string{"linux/amd64"}, Interval: time.Hour}) { + t.Error("same schedule gave different keys") + } + for name, other := range map[string]Schedule{ + "purls": {PURLs: []string{"pkg:docker/alpine@4"}, Platforms: base.Platforms, Interval: base.Interval}, + "platforms": {PURLs: base.PURLs, Platforms: []string{"linux/arm64"}, Interval: base.Interval}, + "interval": {PURLs: base.PURLs, Platforms: base.Platforms, Interval: 2 * time.Hour}, + } { + if scheduleKey(other) == scheduleKey(base) { + t.Errorf("changing %s kept the key", name) + } + } +} diff --git a/internal/server/eviction.go b/internal/server/eviction.go index 5bbae41e..66887d18 100644 --- a/internal/server/eviction.go +++ b/internal/server/eviction.go @@ -39,7 +39,9 @@ func (s *Server) startEvictionLoop(ctx context.Context) { } func (s *Server) runEviction(ctx context.Context, maxSize int64) { - evictLRU(ctx, s.db, s.storage, s.logger, maxSize) + s.runExclusive(ctx, "eviction", func() { + evictLRU(ctx, s.db, s.storage, s.logger, maxSize) + }) } // evictLRU removes the least recently used cached artifacts and metadata diff --git a/internal/server/exclusive.go b/internal/server/exclusive.go new file mode 100644 index 00000000..5067afdd --- /dev/null +++ b/internal/server/exclusive.go @@ -0,0 +1,21 @@ +package server + +import "context" + +// runExclusive runs one pass of a background job unless another instance +// sharing the database is running it. That instance does the work, so this +// pass is skipped rather than waited for. Jobs that act on shared state, such +// as eviction against max_size, would otherwise each do all of it. +func (s *Server) runExclusive(ctx context.Context, job string, run func()) { + release, ok, err := s.db.TryLock(ctx, "job:"+job) + if err != nil { + s.logger.Warn("background job: failed to take lock, skipping this pass", "job", job, "error", err) + return + } + if !ok { + s.logger.Debug("background job: running on another instance, skipping this pass", "job", job) + return + } + defer release() + run() +} diff --git a/internal/server/exclusive_test.go b/internal/server/exclusive_test.go new file mode 100644 index 00000000..abd506b2 --- /dev/null +++ b/internal/server/exclusive_test.go @@ -0,0 +1,60 @@ +package server + +import ( + "context" + "io" + "log/slog" + "os" + "path/filepath" + "testing" + + "github.com/git-pkgs/proxy/internal/database" +) + +func TestRunExclusiveSQLiteRuns(t *testing.T) { + db, err := database.Create(filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatalf("creating database: %v", err) + } + defer func() { _ = db.Close() }() + s := &Server{db: db, logger: slog.New(slog.NewTextHandler(io.Discard, nil))} + + ran := false + s.runExclusive(context.Background(), "eviction", func() { ran = true }) + if !ran { + t.Error("job did not run on SQLite") + } +} + +// TestRunExclusiveSkipsJobRunningElsewhere checks that a pass is skipped +// while another instance sharing the Postgres database runs the job, and +// runs once that instance is done. +func TestRunExclusiveSkipsJobRunningElsewhere(t *testing.T) { + url := os.Getenv("PROXY_DATABASE_URL") + if url == "" { + t.Skip("PROXY_DATABASE_URL not set") + } + open := func() *Server { + db, err := database.OpenPostgres(url) + if err != nil { + t.Fatalf("OpenPostgres: %v", err) + } + t.Cleanup(func() { _ = db.Close() }) + return &Server{db: db, logger: slog.New(slog.NewTextHandler(io.Discard, nil))} + } + a, b := open(), open() + + ranOnB := 0 + a.runExclusive(context.Background(), "eviction", func() { + b.runExclusive(context.Background(), "eviction", func() { ranOnB++ }) + // A different job is not held up. + b.runExclusive(context.Background(), "retention", func() { ranOnB += 10 }) + }) + if ranOnB != 10 { + t.Fatalf("while a ran eviction, b ran %d (want only retention, 10)", ranOnB) + } + b.runExclusive(context.Background(), "eviction", func() { ranOnB++ }) + if ranOnB != 11 { + t.Errorf("after a finished, b's eviction did not run") + } +} diff --git a/internal/server/gradle_cache_eviction.go b/internal/server/gradle_cache_eviction.go index 7f546d16..10980bb9 100644 --- a/internal/server/gradle_cache_eviction.go +++ b/internal/server/gradle_cache_eviction.go @@ -34,7 +34,7 @@ func (s *Server) startGradleBuildCacheEviction(ctx context.Context) { "max_size_bytes", maxSize, "interval", interval) - sweep := func() { + sweepOnce := func() { deletedCount, freedBytes, err := sweepGradleBuildCache(ctx, s.storage, lister, maxAge, maxSize, time.Now()) if err != nil { s.logger.Warn("gradle cache eviction sweep failed", "error", err) @@ -47,6 +47,7 @@ func (s *Server) startGradleBuildCacheEviction(ctx context.Context) { } } + sweep := func() { s.runExclusive(ctx, "gradle-cache-eviction", sweepOnce) } sweep() go func() { diff --git a/internal/server/mirror_schedule.go b/internal/server/mirror_schedule.go index 3f69e7fa..fb32f249 100644 --- a/internal/server/mirror_schedule.go +++ b/internal/server/mirror_schedule.go @@ -24,6 +24,7 @@ func (s *Server) startMirrorSchedules(ctx context.Context, proxy *handler.Proxy) schedules[i] = mirror.Schedule{PURLs: schedule.Images, Platforms: schedule.Platforms, Interval: schedule.IntervalDuration()} } s.scheduler = mirror.NewScheduler(m, schedules, s.logger) + s.scheduler.Claim = s.db.ClaimJobRun s.scheduler.Start(ctx) s.logger.Info("mirror schedules enabled", "schedules", len(schedules)) } diff --git a/internal/server/reclaim.go b/internal/server/reclaim.go index aa986a1c..58aefeda 100644 --- a/internal/server/reclaim.go +++ b/internal/server/reclaim.go @@ -34,7 +34,13 @@ func (s *Server) startReclaimLoop(ctx context.Context) { case <-ctx.Done(): return case <-ticker.C: - reclaimDue(ctx, s.db, s.storage, s.logger, time.Now().Add(-grace), reclaimTickBudget) + s.runExclusive(ctx, "reclaim", func() { + reclaimDue(ctx, s.db, s.storage, s.logger, time.Now().Add(-grace), reclaimTickBudget) + // Leases left by instances that stopped while fetching. + if _, err := s.db.DeleteExpiredFetchLeases(reclaimMinGrace); err != nil { + s.logger.Warn("reclaim: failed to delete expired fetch leases", "error", err) + } + }) } } } diff --git a/internal/server/retention.go b/internal/server/retention.go index 9a2e9fbc..520b4cc2 100644 --- a/internal/server/retention.go +++ b/internal/server/retention.go @@ -47,18 +47,24 @@ func (s *Server) startRetentionLoop(ctx context.Context) { ticker := time.NewTicker(s.cfg.ParseRetentionSweepInterval()) defer ticker.Stop() - sweepRetention(ctx, s.db, s.logger, reg, rules, time.Now()) + s.sweepRetention(ctx, reg, rules) for { select { case <-ctx.Done(): return case <-ticker.C: - sweepRetention(ctx, s.db, s.logger, reg, rules, time.Now()) + s.sweepRetention(ctx, reg, rules) } } } +func (s *Server) sweepRetention(ctx context.Context, reg *retention.Registry, rules retention.Rules) { + s.runExclusive(ctx, "retention", func() { + sweepRetention(ctx, s.db, s.logger, reg, rules, time.Now()) + }) +} + // notCovered returns the known ecosystem keys the global default cannot reach. func notCovered(covered []string) []string { var missing []string