find_cached_scan does not apply the not_expired scope when reusing SBOM scan results

Summary

SbomScanResultCachingService#find_cached_scan picks a scan to reuse without applying SbomScan.not_expired, so the EXPIRED_AGE bound on cache reuse is enforced only by a daily reaper cron rather than at read time.

Problem

find_cached_scan (ee/app/services/security/vulnerability_scanning/sbom_scan_result_caching_service.rb:39) is:

def find_cached_scan(sbom_digest)
  Security::VulnerabilityScanning::SbomScan
    .for_project_and_sbom_digest(project, sbom_digest)
    .most_recently_finished
    .first
end

The model defines both halves of the age bound, but only one is used:

  • EXPIRED_AGE = 2.days (ee/app/models/security/vulnerability_scanning/sbom_scan.rb:8)
  • scope :expired — used by DestroyExpiredSbomScansWorker
  • scope :not_expired — defined and never used anywhere

So nothing in the read path bounds a reused scan's age. The only freshness check is advisories_synced_since?, which is sound for advisory updates but says nothing about how old the scan itself is.

How wide the window actually is

DestroyExpiredSbomScansWorker runs on cron: "0 2 * * *" — once daily at 02:00 UTC (ee/config/schedule.yml:358). A scan becomes expired at created_at + 2.days but survives until the next run, so in the normal case a scan stays reusable for up to roughly a day past EXPIRED_AGE.

The window is wider when the reaper is behind, and it has two exit conditions that can leave a backlog for a full day:

  • It self-reschedules every DELAY (30s) in batches of BATCH_SIZE (200), but stops rescheduling when a batch destroys 0 records.
  • DestroySbomScansService errors are re-raised (sidekiq_options retry: MAX_RETRIES = 3), so an object-storage deletion failure truncates the chain rather than skipping past it.

On the local observation

While verifying #612092 (closed) I saw a 5.9-day-old result reused (SbomScan id=37 reusing result_id=15). That GDK had background jobs deliberately stopped, so the reaper was never running — this is not evidence of production behaviour and the production window is bounded by the daily cron as described above. It does demonstrate the read path imposes no bound of its own.

Why it is worth fixing

Reuse correctness currently depends on a cron running on time. not_expired exists to express exactly the "still reusable" predicate and simply is not applied where reuse is decided. Applying it makes the read path self-enforcing and removes the dependency on reaper health.

Note the class comment lists three reuse criteria and does not mention age, so the first thing to settle is whether age is intended to be a criterion at all.

Proposal

If age is a reuse criterion, add the scope:

def find_cached_scan(sbom_digest)
  Security::VulnerabilityScanning::SbomScan
    .for_project_and_sbom_digest(project, sbom_digest)
    .not_expired
    .most_recently_finished
    .first
end

index_sbom_vulnerability_scans_on_project_id_and_sbom_digest should still drive the lookup, with created_at and status as filters on a small number of rows per project and digest. Worth confirming with a query plan, but an index change looks unlikely.

If unbounded reuse is intentional as long as no advisory has synced, close this and make the class comment say so explicitly, so the unused not_expired scope stops reading like an oversight.

Acceptance criteria

  • Decision recorded on whether scan age is a reuse criterion.
  • If yes, find_cached_scan applies not_expired, with a spec covering a scan that is expired but not yet reaped returning :not_found.
  • Query plan checked for the added created_at predicate.
  • The class comment reflects the outcome either way.

Found while verifying #612092 (closed) / !250273 (merged).

Edited by 🤖 GitLab Bot 🤖