From 576a6b9e3564620e470b083a58a609ce5903c9b6 Mon Sep 17 00:00:00 2001 From: Evan Jarrett Date: Mon, 17 Aug 2026 21:52:51 -0500 Subject: [PATCH] scanner: test grype DB freshness, backoff and reload fallback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit scanner/internal/scan had no test file at all, which is why fa1dfb0 could be written and reviewed without anyone being able to state its defect as an assertion. Six tests now do. The load path is only reachable from a test through an indirection, so grype.LoadVulnerabilityDB is now behind a loadVulnDB package var. Everything worth testing here is what happens when that call returns a stale database or fails outright, and neither is reachable from a test that has to perform a real download. Covered: freshness is taken from the DB's own build timestamp; a fresh DB is reused without a download; a stale DB inside the retry backoff keeps serving without one; the backoff expires and the replacement is adopted with its predecessor closed; a failed reload with a usable provider in hand keeps scanning and still advances the attempt timestamp; and a cold-start failure is an error rather than a scan that silently finds nothing. Verified by mutation. Restoring `vulnDBBuilt = time.Now()` — the original defect — fails the freshness test with the stale build time in the message. Removing the serve-the-old-DB fallback fails the outage test. One honest limit, recorded in the test file. The backoff is tested twice in the production code, on the read-lock fast path and again under the write lock, and mutation shows they are redundant for correctness: deleting either alone leaves the throttle test passing, and only deleting both fails it. The fast-path copy exists so a stale DB does not push every scan through the exclusive lock, which is a contention property, not a behavioural one, and no unit test can assert it without being flaky. Co-Authored-By: Claude Opus 5 (1M context) --- scanner/internal/scan/grype.go | 9 +- scanner/internal/scan/grype_test.go | 243 ++++++++++++++++++++++++++++ 2 files changed, 251 insertions(+), 1 deletion(-) create mode 100644 scanner/internal/scan/grype_test.go diff --git a/scanner/internal/scan/grype.go b/scanner/internal/scan/grype.go index 2eb4c26..8df8d14 100644 --- a/scanner/internal/scan/grype.go +++ b/scanner/internal/scan/grype.go @@ -159,6 +159,13 @@ func grypeDBConfig(vulnDBPath string) (distribution.Config, installation.Config) } } +// loadVulnDB is the Grype loader, indirected so tests can drive the freshness +// and fallback logic without a network fetch or a real database on disk. The +// whole point of this file is what happens when that call returns a stale DB or +// fails outright, and neither is reachable from a test that has to perform a +// real download. +var loadVulnDB = grype.LoadVulnerabilityDB + // loadVulnDatabase loads the Grype vulnerability database with caching and // automatic refresh. The cached DB is returned if loaded less than // vulnDBRefreshAge ago. On a stale or missing DB, Grype downloads a fresh copy @@ -219,7 +226,7 @@ func loadVulnDatabase(ctx context.Context, vulnDBPath string) (vulnerability.Pro // the upstream is unreachable but the on-disk DB is still valid, it falls // back to serving the existing DB. vulnDBAttempt = time.Now() - store, status, err := grype.LoadVulnerabilityDB(distConfig, installConfig, true) + store, status, err := loadVulnDB(distConfig, installConfig, true) if err != nil { // Reload failed (e.g. upstream down and on-disk DB past the 14-day max // age). If we still hold a usable provider, keep serving it until the diff --git a/scanner/internal/scan/grype_test.go b/scanner/internal/scan/grype_test.go new file mode 100644 index 0000000..cf4bccf --- /dev/null +++ b/scanner/internal/scan/grype_test.go @@ -0,0 +1,243 @@ +package scan + +import ( + "context" + "errors" + "testing" + "time" + + v6dist "github.com/anchore/grype/grype/db/v6/distribution" + v6inst "github.com/anchore/grype/grype/db/v6/installation" + "github.com/anchore/grype/grype/vulnerability" +) + +// fakeProvider stands in for a loaded vulnerability database. Nothing here +// queries it; the tests only care which provider comes back and whether the +// previous one was closed. +type fakeProvider struct { + vulnerability.Provider + name string + closed bool +} + +func (f *fakeProvider) Close() error { + f.closed = true + return nil +} + +// resetVulnDBState clears the package-level database state. These globals are +// shared by every worker in the real scanner, so tests must not run in parallel +// and must not inherit each other's state. +func resetVulnDBState(t *testing.T) { + t.Helper() + original := loadVulnDB + t.Cleanup(func() { + vulnDBLock.Lock() + vulnDB, vulnDBBuilt, vulnDBAttempt = nil, time.Time{}, time.Time{} + vulnDBLock.Unlock() + vulnDBScans.Store(0) + loadVulnDB = original + }) + + vulnDBLock.Lock() + vulnDB, vulnDBBuilt, vulnDBAttempt = nil, time.Time{}, time.Time{} + vulnDBLock.Unlock() + vulnDBScans.Store(0) +} + +// stubLoader installs a fake loader and counts how many times it was called. +// Reaching it at all is significant: every call is a real download attempt in +// production, so the call count is the thing several of these tests assert on. +func stubLoader(t *testing.T, built time.Time, err error) (*fakeProvider, *int) { + t.Helper() + loaded := &fakeProvider{name: "loaded"} + calls := 0 + loadVulnDB = func(v6dist.Config, v6inst.Config, bool) (vulnerability.Provider, *vulnerability.ProviderStatus, error) { + calls++ + if err != nil { + return nil, nil, err + } + return loaded, &vulnerability.ProviderStatus{Built: built}, nil + } + return loaded, &calls +} + +// TestLoadVulnDatabase_FreshnessComesFromBuildTime is the defect fa1dfb0 fixed, +// stated as a test. Freshness used to be measured from load time, so a load that +// fell back to a stale-but-valid on-disk DB started a fresh 7-day lease and +// could ride past Grype's MaxAllowedBuiltAge. Past that cliff the scanner +// reports no vulnerabilities rather than an error, which is the worst possible +// way for a security tool to fail. +// +// Loading a DB built 10 days ago must therefore leave it stale, not fresh. +func TestLoadVulnDatabase_FreshnessComesFromBuildTime(t *testing.T) { + resetVulnDBState(t) + + staleBuild := time.Now().Add(-10 * 24 * time.Hour) + _, calls := stubLoader(t, staleBuild, nil) + + if _, err := loadVulnDatabase(context.Background(), t.TempDir()); err != nil { + t.Fatalf("loadVulnDatabase: %v", err) + } + if *calls != 1 { + t.Fatalf("loader called %d times, want 1", *calls) + } + + if !vulnDBBuilt.Equal(staleBuild) { + t.Errorf("vulnDBBuilt = %v, want the DB's own build time %v: recording load time here "+ + "is what let an expired database look fresh for another 7 days", vulnDBBuilt, staleBuild) + } + if time.Since(vulnDBBuilt) < vulnDBRefreshAge { + t.Error("a database built 10 days ago is being treated as fresh") + } +} + +// TestLoadVulnDatabase_FreshDBIsServedWithoutReloading pins the ordinary case: +// a recently built database is reused, and no download is attempted. +func TestLoadVulnDatabase_FreshDBIsServedWithoutReloading(t *testing.T) { + resetVulnDBState(t) + + cached := &fakeProvider{name: "cached"} + vulnDB = cached + vulnDBBuilt = time.Now().Add(-1 * time.Hour) + + _, calls := stubLoader(t, time.Now(), nil) + + got, err := loadVulnDatabase(context.Background(), t.TempDir()) + if err != nil { + t.Fatalf("loadVulnDatabase: %v", err) + } + if got != cached { + t.Error("a freshly built database was replaced instead of reused") + } + if *calls != 0 { + t.Errorf("loader called %d times for a fresh DB, want 0", *calls) + } + if cached.closed { + t.Error("the in-use provider was closed") + } +} + +// TestLoadVulnDatabase_StaleDBIsThrottled covers the backoff. Once the DB is +// stale the freshness test can never pass again, so without a throttle every +// worker on every scan would serialize through its own upstream download +// timeout. Inside the backoff window the stale-but-loaded provider keeps +// serving, untouched. +// +// What this pins, precisely: the behaviour, not either check that implements +// it. fa1dfb0 tests the backoff twice, once on the read-lock fast path and +// again under the write lock, and mutation confirms they are redundant for +// correctness — deleting either one alone leaves this test passing, and only +// deleting both fails it. That redundancy is deliberate. The fast-path copy +// exists so a stale DB does not push every scan through the exclusive lock, a +// contention property no unit test can assert without being flaky. +func TestLoadVulnDatabase_StaleDBIsThrottled(t *testing.T) { + resetVulnDBState(t) + + cached := &fakeProvider{name: "cached"} + vulnDB = cached + vulnDBBuilt = time.Now().Add(-10 * 24 * time.Hour) // stale + vulnDBAttempt = time.Now().Add(-5 * time.Minute) // probed recently + + _, calls := stubLoader(t, time.Now(), nil) + + got, err := loadVulnDatabase(context.Background(), t.TempDir()) + if err != nil { + t.Fatalf("loadVulnDatabase: %v", err) + } + if got != cached { + t.Error("the stale provider was replaced during the backoff window") + } + if *calls != 0 { + t.Errorf("loader called %d times inside the retry backoff, want 0", *calls) + } +} + +// TestLoadVulnDatabase_RetriesOnceTheBackoffElapses is the other half of the +// throttle: it has to expire. A backoff that never lets go would pin the +// scanner to a stale database indefinitely, which is the same silent failure +// the build-time fix exists to prevent. +func TestLoadVulnDatabase_RetriesOnceTheBackoffElapses(t *testing.T) { + resetVulnDBState(t) + + old := &fakeProvider{name: "old"} + vulnDB = old + vulnDBBuilt = time.Now().Add(-10 * 24 * time.Hour) + vulnDBAttempt = time.Now().Add(-vulnDBRetryBackoff - time.Minute) + + freshBuild := time.Now().Add(-1 * time.Hour) + loaded, calls := stubLoader(t, freshBuild, nil) + + got, err := loadVulnDatabase(context.Background(), t.TempDir()) + if err != nil { + t.Fatalf("loadVulnDatabase: %v", err) + } + if *calls != 1 { + t.Fatalf("loader called %d times after the backoff elapsed, want 1", *calls) + } + if got != loaded { + t.Error("the newly loaded provider was not adopted") + } + if !old.closed { + t.Error("the replaced provider was not closed: every reload leaks its predecessor") + } + if !vulnDBBuilt.Equal(freshBuild) { + t.Errorf("vulnDBBuilt = %v, want %v", vulnDBBuilt, freshBuild) + } +} + +// TestLoadVulnDatabase_FailedReloadKeepsServingTheOldDB covers an upstream +// outage. A loaded provider does not re-validate build age on queries, so it +// keeps working; refusing to scan because the update failed would be a worse +// outcome than scanning against a database a few days old. +// +// The attempt timestamp must still advance, or the backoff never engages and +// every subsequent scan pays the failed download again. +func TestLoadVulnDatabase_FailedReloadKeepsServingTheOldDB(t *testing.T) { + resetVulnDBState(t) + + old := &fakeProvider{name: "old"} + vulnDB = old + vulnDBBuilt = time.Now().Add(-10 * 24 * time.Hour) + vulnDBAttempt = time.Now().Add(-vulnDBRetryBackoff - time.Minute) + + _, calls := stubLoader(t, time.Time{}, errors.New("upstream unreachable")) + + got, err := loadVulnDatabase(context.Background(), t.TempDir()) + if err != nil { + t.Fatalf("a failed reload with a usable DB in hand must not fail the scan: %v", err) + } + if got != old { + t.Error("the previously loaded provider was not served after the reload failed") + } + if old.closed { + t.Error("the provider still in use was closed") + } + if *calls != 1 { + t.Errorf("loader called %d times, want 1", *calls) + } + if time.Since(vulnDBAttempt) > time.Minute { + t.Error("vulnDBAttempt was not advanced by the failed attempt, so the backoff " + + "never engages and every scan repeats the download") + } +} + +// TestLoadVulnDatabase_ColdStartFailureIsAnError is the one case that must fail +// loudly. With no provider in hand there is nothing to scan against, and +// returning success would report every image as clean. +func TestLoadVulnDatabase_ColdStartFailureIsAnError(t *testing.T) { + resetVulnDBState(t) + + _, calls := stubLoader(t, time.Time{}, errors.New("upstream unreachable")) + + got, err := loadVulnDatabase(context.Background(), t.TempDir()) + if err == nil { + t.Fatal("a cold start with no database returned success: every scan would find nothing") + } + if got != nil { + t.Error("a provider was returned alongside the error") + } + if *calls != 1 { + t.Errorf("loader called %d times, want 1", *calls) + } +}