Draft: Make security scan purge cursor tail-resilient (#606136)

Gregory Havengarequested to merge
ghavenga/606136-tail-resilient-cursor into master

What does this MR do and why?

This is 1 of 2 MRs for #606136. This MR makes the Security::PurgeScansService cursor tail-resilient so it can no longer permanently strand the newest stale security_scans. A separate MR (by another author) handles the worker DB-state reactivity / scheduling (cron cadence in ee/config/schedule.yml); that scope is intentionally not touched here.

Related to #606136 Addresses the purge-path defect discussed in #606136 (comment 3658881104)

Root cause: tail starvation

Security::PurgeScansService.purge_stale_records walks Security::Scan.stale.ordered_by_created_at_and_id (ascending created_at, id) with a forward-only Gitlab::Pagination::Keyset::Iterator, persisting the last-purged (created_at, id) tuple in a Redis CursorStore (24h TTL), capped at MAX_STALE_SCANS_SIZE = 200_000 per run.

On a high-volume instance where more than the cap of stale scans exist:

  1. The cursor only ever marches forward from the oldest (head) of the stale set.
  2. Each run exhausts its 200K budget on the head and never reaches the newest (tail) stale scans.
  3. When the 24h TTL expires, the cursor resets back to the head and re-consumes the budget on the oldest scans again.

Those tail scans are exactly the ones Security::Finding#detach_partition? inspects — it uses last_finding_in_partition (the max-id, i.e. newest, finding of an aged partition). If the tail scan is never :purged, findings_can_be_purged? stays false and the security_findings partition can never detach.

Chosen approach: alternating bidirectional sweep (candidate (a))

I chose the alternating sweep over a within-run split-budget (candidate (b)) or TTL hardening (candidate (c)) because it most directly and simply guarantees the partition-tail scans are reached within a bounded number of runs while keeping each run's work budget and per-batch commit semantics unchanged:

  • Forward runs purge from the oldest end (ordered_by_created_at_and_id).
  • Reverse runs purge from the newest end (new ordered_by_created_at_and_id_desc scope), so the partition-tail scans get :purged even when the head backlog exceeds the cap.
  • Direction alternates each run; a dedicated cursor is tracked per direction in the CursorStore payload so both ends keep making independent progress and converge inward. Because the stale scope excludes already-purged rows, the two cursors cannot reprocess each other's work.

Candidate (b) would also work but complicates a single run (two passes, budget split); candidate (a) is a smaller, more obviously-correct change. The tail is reached in at most one extra run.

The behaviour is gated behind an ops feature flag tail_resilient_scan_purge_cursor (default disabled) — idiomatic for a change to a purge algorithm. When disabled, the original forward-only path runs unchanged.

Cursor payload & back-compat

New payload shape:

{ "last_direction": "forward",
  "forward": { "created_at": "...", "id": 1 },
  "reverse": { "created_at": "...", "id": 9 } }

Back-compat is handled in PurgeCursorState:

  • An old single-cursor payload ({"created_at": ..., "id": ...}, as written by the pre-flag forward-only algorithm) is read as the forward cursor.
  • Since its direction is unknown, it is treated as a completed forward run, so the next run sweeps in reverse and reaches the tail immediately post-deploy.
  • An absent/empty payload (first run) defaults to a forward sweep.

Files changed

  • ee/app/services/security/purge_scans_service.rb — alternating bidirectional sweep behind the FF; new PurgeCursorState helper for direction selection + per-direction cursor + back-compat; per-direction commit into the CursorStore payload.
  • ee/app/models/security/scan.rb — new ordered_by_created_at_and_id_desc scope for the reverse sweep.
  • ee/config/feature_flags/ops/tail_resilient_scan_purge_cursor.yml — ops FF, default_enabled: false.
  • ee/spec/services/security/purge_scans_service_spec.rb — new specs.

What is unchanged (by design)

  • The stale/retention semantics (created_at < retention AND status != :purged).
  • The Security::Finding#detach_partition? last-finding check (correct by design; reporter's "detach on any purged" is unsafe).
  • Set-based update_all(status: :purged) and per-batch cursor commit.
  • The worker wiring / cron cadence (owned by the other MR).

Testing

bundle exec rspec ee/spec/services/security/purge_scans_service_spec.rb — 9 examples, 0 failures. New coverage:

  • FF path purges stale scans, leaves fresh scans untouched.
  • Direction is recorded and alternates across consecutive runs.
  • With a stubbed small cap and >cap stale scans, the newest (tail) scans are purged on the run after the head-only run (bounded).
  • Back-compat: an old single-cursor payload triggers a reverse sweep next.

Database review

This MR adds a new scope and the reverse sweep issues a SELECT ... ORDER BY created_at DESC, id DESC (plus keyset (created_at, id) < cursor conditions mid-sweep). No schema change and no new index — the reverse sweep is served by the existing partial index via a backward scan.

Existing index:

CREATE INDEX index_security_scans_for_non_purged_records
  ON public.security_scans USING btree (created_at, id) WHERE (status <> 6)

Forward (head) sweep query:

SELECT "security_scans".* FROM "security_scans"
WHERE (created_at < $1) AND "security_scans"."status" != 6
ORDER BY "security_scans"."created_at" ASC, "security_scans"."id" ASC
LIMIT 100
Limit  (cost=0.14..8.29 rows=100 width=129)
  ->  Index Scan using index_security_scans_for_non_purged_records on security_scans
        Index Cond: (created_at < '...'::timestamptz)

Reverse (tail) sweep query:

SELECT "security_scans".* FROM "security_scans"
WHERE (created_at < $1) AND "security_scans"."status" != 6
ORDER BY "security_scans"."created_at" DESC, "security_scans"."id" DESC
LIMIT 100
Limit  (cost=0.14..8.29 rows=100 width=129)
  ->  Index Scan Backward using index_security_scans_for_non_purged_records on security_scans
        Index Cond: (created_at < '...'::timestamptz)

The reverse sweep uses Index Scan Backward on the same partial index with identical cost to the forward scan. The mid-sweep keyset conditions add (created_at, id) < cursor, which the same (created_at, id) btree serves. The write path (update_all(status: :purged)) is unchanged.

Merge request reports