fix(core): harden scan durability and idempotency (#303)

[SHAURYAKSHARMA24]

Summary

Implements the #303 hardening contract: transaction recovery, fenced scan leases, idempotent result persistence, durable scan admission, durable CVE enrichment, and bounded operational signals.

Scope note (changed after review): this PR does not implement the #263 rule-evaluation coverage contract. That contract — the rule_evaluations table, PASS/FAIL/UNKNOWN/ERROR/NOT_APPLICABLE semantics, engine emission, and the get_compliance_score() fix — is owned by #321. An earlier revision of this branch carried a second, near-identical rule_evaluations table that nothing populated; it has been removed. See Relationship to #321 and #310.

Problems fixed

• Aborted PostgreSQL transactions no longer poison worker progress; broken connections are discarded and reacquired.
• Expired or stale scan workers cannot write authoritative completion, failure, or findings.
• Repeated result delivery no longer creates duplicate findings.
• Concurrent/replayed API triggers cannot create uncontrolled duplicate active scans.
• CVE enrichment is no longer owned by a Gunicorn daemon thread and no longer stops after a single NVD page.
• A CVE enrichment job that exhausts its retries can be recovered by an operator instead of being permanently dead.
• Operators now have worker, queue, lease, retry, and last-success visibility.

Architecture

• Scan leases and fencing: PostgreSQL claims have owner, expiry, monotonic fencing token, renewal, stale recovery, and fenced final writes.
• Idempotent persistence: findings are uniquely identified by scan, rule, canonical resource scope, and an optional rule-specific discriminator. Mutable fields are updated with ON CONFLICT.
• Admission: a transaction-scoped PostgreSQL advisory lock plus partial unique indexes enforce one pending/running scan per subscription and a unique subscription/idempotency-key pair. Same semantics replay the logical scan; changed semantics conflict. OPENSHIELD_MAX_SCANS_PER_SUBSCRIPTION_PER_HOUR provides an explicit optional time-window policy; one active scan remains the enforced concurrency quota.
• Durable enrichment: a completed fenced scan atomically creates one PostgreSQL enrichment job. The worker claims, renews, checkpoints, retries with bounded exponential backoff, recovers expired leases, and fences stale writers. NVD retrieval follows totalResults through every page.
• Worker scheduling: each loop iteration takes at most one enrichment job and at most one scan, so neither durable queue can starve the other.
• Metrics: /metrics reads bounded PostgreSQL aggregates; labels are only queue (scan/enrichment) and worker_type.

Enrichment job lifecycle

POST /api/scans/<scan_id>/enrich never creates more than one job per scan and reports what it did via an outcome field:

outcome	Status	Meaning
created	202	No job existed; one was queued.
requeued	202	A terminally failed job was reset to pending with a fresh retry budget.
active	202	A pending/running job already exists; a live lease is never disturbed.
completed	200	Enrichment already finished; nothing restarted.

A requeue keeps the same job row, its last error_message (audit) and its checkpoint (so the retry resumes rather than re-enriching findings that already succeeded). Concurrent re-POSTs converge: exactly one reports requeued, the rest report active.

Database migrations

1. e4f7a9b2c6d8 — renewable scan leases and fencing tokens.
2. f2b6d8e1a4c9 — stable finding identities. Existing findings receive distinct legacy:<id> keys; no legacy rows are silently collapsed.
3. a7c5e9d2f1b4 — durable scan admission/idempotency indexes.
4. c9e1a5b7d3f2 — durable fenced enrichment jobs.
5. d4a8c1e6b2f9 — worker heartbeat storage and the metrics index for completed scans.

Rebased onto current dev (f69db7a). There is exactly one Alembic head, d4a8c1e6b2f9, chained from d8e4f6a1b2c3 — which is still dev's Alembic head, since no migration has landed on dev since. (d8e4f6a1b2c3 is an Alembic revision id, not a git SHA.)

Migration prerequisite: one active scan per subscription

a7c5e9d2f1b4 enforces one pending/running scan per subscription. On a deployment that already violates that rule, CREATE UNIQUE INDEX CONCURRENTLY would fail and leave an INVALID index behind. The migration now preflights instead: it stops before creating any index, changes nothing, and names the offending subscriptions:

Cannot enforce one active scan per subscription: 1 subscription(s) already have
more than one pending/running scan: <subscription-id> (2 active). Resolve them
first (let the scans finish, or mark the superseded rows 'failed'), then re-run
this migration.

Deciding which production scan is authoritative is deliberately left to the operator — no scan history is deleted or rewritten automatically. Retrying is safe: the migration drops the INVALID index left by an interrupted concurrent build before rebuilding. Cleanup order is documented in docs/async-scan-architecture.md.

Concurrency guarantees

All authoritative scan-result writes re-check lease owner, fencing token, running state, and unexpired lease under FOR UPDATE in the same transaction as persistence. Once worker A loses its lease and worker B reclaims with a newer token, A cannot update scan state, findings, or enrichment progress. The same holds for enrichment checkpoints, retry state, completion, and CVE outputs. PostgreSQL unique constraints and upserts make duplicate API/result/job delivery converge on one logical record.

Deployment

1. Stop or drain old scan workers; mixed old/new workers are unsafe because old workers cannot satisfy the fencing contract.
2. Resolve any duplicate active scans (see above), then apply Alembic migrations through d4a8c1e6b2f9.
3. Deploy this API and scanner/worker.py. The worker now processes both scan and enrichment jobs.
4. Monitor /metrics for worker liveness, queue age, lease age, retries, and last successful scan.

Configuration

SCAN_LEASE_SECONDS, SCAN_HEARTBEAT_SECONDS, OPENSHIELD_MAX_SCANS_PER_SUBSCRIPTION_PER_HOUR and WORKER_HEARTBEAT_RETENTION_SECONDS are documented in .env.example, including the heartbeat-must-be-shorter-than-lease constraint (a heartbeat >= lease logs a warning and falls back to one third of the lease).

Relationship to #321 and #310

• feat(engine): add rule evaluation coverage contract (#263) #321 (feat: persist PASS/FAIL/ERROR/NOT_APPLICABLE per rule per resource, fix compliance score #263 evaluation contract) owns rule_evaluations — the table, the statuses, engine emission, and the compliance-score fix. This PR previously created a competing near-identical table that no production code populated; that is removed, so the two PRs no longer collide on CREATE TABLE rule_evaluations. When feat(engine): add rule evaluation coverage contract (#263) #321 lands, its evaluation writes belong inside this PR's fenced completion transaction and inherit the ownership check for free (a comment in save_scan marks the exact spot).
• Migration ordering: fix(compliance): make framework reports evidence-based and non-certifying #310 (3a76ff935bf6), feat(engine): add rule evaluation coverage contract (#263) #321 (3f59f83a5253) and this PR (e4f7a9b2c6d8) all still chain from d8e4f6a1b2c3, the current dev Alembic head. Neither fix(compliance): make framework reports evidence-based and non-certifying #310 nor feat(engine): add rule evaluation coverage contract (#263) #321 is merged (fix(compliance): make framework reports evidence-based and non-certifying #310 is currently conflicting against dev; feat(engine): add rule evaluation coverage contract (#263) #321 is still a draft), so this PR depends on neither and produces exactly one head on its own. Whichever merges second repoints its down_revision — a one-line change now, since no two of them create the same object. Merge order remains an open maintainer decision; happy to rebase onto whichever lands first.

Tests

Re-run after the rebase onto current dev:

• Full backend suite: 977 passed, 3 skipped, 0 failed. The 3 skips are dev's own AI/RAG tests that require a locally built BM25 index; they are unrelated to this PR.
• #303-focused suites against postgres:16-alpine: scan leases (12), admission (3), admission-migration (3), enrichment jobs (10), operational metrics (2), worker (11 + 4 subtests), enrich route (7), NVD (16), observability (12), database reliability (7), async persistence (9).
• Alembic validated on real PostgreSQL 16: empty→head, current-dev-Alembic-head→head, full 5-step downgrade→upgrade round trip, and alembic heads returning exactly d4a8c1e6b2f9 (head).
• Coverage added across review rounds: terminal-failure requeue (6 PostgreSQL tests incl. concurrent requeue and post-requeue fencing), duplicate-active-scan migration preflight and INVALID-index retry (3 tests), stale worker-heartbeat pruning, worker queue fairness with both queues backlogged, and the lease/heartbeat interval clamp.
• ruff check . and ruff format --check . pass.

Acceptance criteria (#303)

• All transactions rollback on failure and discard/reacquire broken connections.
• Claims use renewable leases with owner, expiry and fencing token.
• Heartbeat and completion updates require the current fencing token.
• Finding persistence is idempotent using stable unique keys/upserts.
• Scan admission has per-subscription quotas, one-active-scan deduplication and idempotency keys.
• Enrichment is a durable claimed job with retries, stale recovery and complete pagination.
• PostgreSQL-backed fault-injection tests cover abort, restart, duplicate delivery, lease expiry and two-worker races.
• Metrics include worker heartbeat, oldest queue age, lease age, retry count and last successful complete scan.

Evaluation persistence is idempotent by construction once #321 lands (its writes sit inside this fenced transaction), but this PR does not itself persist evaluations and does not claim that criterion.

Known limitations

• /metrics recomputes its aggregates on every scrape. The queue/lease/heartbeat/last-success lookups are index-served; the two retry_attempts sums scan their whole table and grow with scan history. Acceptable at current volumes; a short-TTL in-process cache is the first thing to add if scrape latency becomes visible.
• The scan and enrichment queues alternate one item per iteration. That bounds starvation but does not prioritise; a deep enrichment backlog still adds one job of latency per scan.

Related

Closes #303

[m-khan-97]

@SHAURYAKSHARMA24, this is the canonical track for #303’s durability layer: transaction recovery, leases/fencing, idempotent admission and writes, durable enrichment, and worker telemetry. One integration boundary must be resolved before lead review: migration f2b6d8e1a4c9 creates rule evaluations and associated persistence semantics that overlap #321, which is the already-agreed #263 evaluation-contract implementation. Please coordinate with Dipesh and either stack/rebase #325 on the accepted #321 contract or remove the duplicate evaluation-schema ownership from this PR. We must not merge two competing rule_evaluations definitions or aggregation contracts. Keep the fencing/idempotency guarantees around whichever canonical evaluation model is selected.

[ritiksah141]

Reviewed all 28 files end to end: the five migrations, the lease and fencing layer in api/models/finding.py, the worker loop, the new enrichment worker, the scan routes, observability, and the NVD client. Also ran the suite locally against a scratch Postgres with the migrations applied from base to head.

The lease and fencing design is correct and applied consistently. The fault-injection tests are thorough and map to every acceptance criterion in #303. Two functional gaps should be resolved before merge, plus a few smaller items.

Must fix

1. Terminally failed enrichment jobs are unrecoverable. After 3 attempts fail_enrichment_job marks the job failed. From there POST /api/scans//enrich returns the dead job via ON CONFLICT DO NOTHING, claim_next_enrichment_job only picks pending, and recover_stale_enrichment_jobs only handles expired running leases. So a scan whose enrichment exhausts its retries is stuck unless someone edits the DB by hand. This is a regression from the previous thread-based path, where a re-POST simply worked. Please reset a terminal failed job back to pending on enqueue, or return an explicit 409 telling the operator.

2. rule_evaluations is dead in production. The migration, unique constraint, the save_scan upsert, and the tests all exist, but nothing populates it: scanner/engine.py run_scan returns no evaluations key and no production code emits one. Either wire the engine to emit evaluations, or scope this explicitly as storage-only for now. As written, the #303 evaluations criterion looks met but is not observable in production.

Should fix

3. uq_scans_one_active_per_subscription can leave an INVALID index. If a deployment already has two or more active (pending/running) scans for one subscription, CREATE UNIQUE INDEX CONCURRENTLY fails and leaves an invalid index behind silently. Add a dedupe/cleanup note to the deployment-order doc, or a cleanup step in the migration before the index is created.

4. The enrichment fixture assumes the pending queue is empty. This is a general test-isolation issue, not an environment quirk. claim_next_pending_scan claims the oldest pending scan, but the fixture assumes it claims the scan it just created. Any developer who sets both DATABASE_URL and AZURE_SUBSCRIPTION_ID (common when developing against a real local Postgres plus Azure) and runs the full suite will hit this: the pre-existing role tests in test_auth.py admit a real pending scan, and the enrichment fixture then claims that older row instead of its own scan, so save_scan correctly raises LostLease. Make the fixture robust by truncating scans/enrichment_jobs in setup, or by asserting on the claimed scan_id.

Nits

5. docs/api-reference.md is not updated for Idempotency-Key, the 409/429 responses, the 200 replay response, and the enrich job_id response.
6. The new env vars OPENSHIELD_MAX_SCANS_PER_SUBSCRIPTION_PER_HOUR, SCAN_LEASE_SECONDS, and SCAN_HEARTBEAT_SECONDS are missing from .env.example.
7. worker_heartbeats grows unbounded: one row per worker restart, never cleaned.
8. /metrics now runs full DB aggregates on every scrape with no caching. Fine at current scale, worth a note.
9. Enrichment jobs run serially ahead of scans in the same worker loop. Worth documenting as a throughput characteristic.

What looks good

Fencing is correct and applied uniformly on every authoritative write. The fault-injection suite is the most thorough in the repo and covers every #303 acceptance criterion. The deployment-order doc is honest about the migrate-then-run-workers constraint. Error sanitization is preserved. NVD pagination (resultsPerPage=2000, following totalResults, bounded retries with 429 backoff) is done properly. Metrics are bounded-cardinality.

[ritiksah141]

Following up on @m-khan-97's integration note, here is the concrete side-by-side between this PR's rule_evaluations handling and the one in #321, so we converge on a single canonical model. Short version: this PR should drop the evaluation schema and keep the fencing, and the two save_scan implementations need to be reconciled, not just the table.

Schema: rule_evaluations

Aspect	#321 (3f59f83a5253)	#325 (f2b6d8e1a4c9)
Core columns (id, scan_id, rule_id, resource_id, resource_type, status, reason_code, reason, evidence, finding_id, evaluated_at)	yes	identical
PK, FK scan_id, FK finding_id ON DELETE SET NULL	yes	identical
Unique (scan_id, rule_id, resource_id), same constraint name	yes	same name
Status CHECK (5 values), same constraint name	yes	same name
resource_id <> '' CHECK	yes	yes
Index on scan_id	yes	yes
Index on rule_id	yes	no
Index on status	yes	no
CHECK: reason_code required for UNKNOWN/ERROR/NOT_APPLICABLE	yes	no

The core columns, keys, and constraints overlap and share names, so whichever migration runs second fails with "relation already exists." #321 is a strict superset: it adds the rule_id and status indexes and the reason_code-required CHECK.

Producer (who emits evaluations)

	#321	#325
scanner/evaluation.py (EvaluationStatus, RuleEvaluation, subscription_scope_id, aggregate_status)	added	none
engine.run_scan calls evaluate() per rule and collects evaluations	added	none, engine untouched
FAIL evaluation contributes a finding (deduped vs scan() by rule_id+resource_id)	added	none
Returns evaluations in the scan result	added	reads it but never populated

Decisive difference: this PR has no producer, so its evaluations are always empty in production. Only #321 makes evaluations observable.

save_scan persistence semantics

Both PRs are full rewrites of the same method with incompatible idempotency models, so the conflict is larger than the table.

	#321	#325
Signature	save_scan(scan_result)	save_scan(scan_result, lease_owner, fencing_token)
Fencing / lease check	none	SELECT FOR UPDATE owner+token+unexpired, else LostLease
Findings idempotency	DELETE all, re-insert (full replace)	UPSERT ON CONFLICT (scan_id, finding_key) + delete-absent
Evaluations idempotency	DELETE all, plain re-insert	UPSERT ON CONFLICT (scan_id, rule_id, resource_id) + delete-absent
FAIL eval linked to finding via (rule_id, resource_id) in same txn	yes	yes
Requires rule_id + resource_id on every evaluation	no	yes, raises ValueError

Compliance score (the actual #263 bug)

	#321	#325
Rewrites get_compliance_score to read statuses from rule_evaluations	yes	no
aggregate_status FAIL > ERROR > UNKNOWN > PASS > NOT_APPLICABLE	yes	no
No evaluation row reports UNKNOWN instead of PASS	yes	no
Score excludes NOT_APPLICABLE from denominator, never counts UNKNOWN/ERROR as pass	yes	no

Only #321 fixes the score-inflation bug.

Recommended resolution

1. feat(engine): add rule evaluation coverage contract (#263) #321 owns the evaluation contract: schema, scanner/evaluation.py, engine producer, aggregation, and the compliance-score fix. That is the agreed feat: persist PASS/FAIL/ERROR/NOT_APPLICABLE per rule per resource, fix compliance score #263 scope and the only version that is observable.
2. This PR drops its rule_evaluations table creation and its evaluation upsert, and keeps everything else: leases, fencing, admission, enrichment, metrics.
3. The real merge point is save_scan, not just the table. The final save_scan should keep this PR's fenced, upsert skeleton (ownership check, lease clear, findings upsert by finding_key) and fold feat(engine): add rule evaluation coverage contract (#263) #321's evaluation field set and FAIL-to-finding_id linkage into that same fenced transaction. Given the replay-safety goal of core: harden scan transactions, leases, idempotency, and durable background work #303, evaluations should use the upsert-plus-delete-absent model.
4. Ordering: this only composes cleanly if feat(engine): add rule evaluation coverage contract (#263) #321 merges first (or both merge as a deliberate pair), then this PR rebases its remaining migrations on top of 3f59f83a5253. Both are currently OPEN and both branch off d8e4f6a1b2c3, so merging in the wrong order produces two Alembic heads and breaks the single-head CI gate.

One integration detail for whoever reconciles: #321's engine adds evaluate()-derived FAIL findings to the findings list, and this PR derives finding_key from rule_id plus resource scope plus discriminator. Those compose, but make sure evaluate()-derived findings get stable finding_keys so the upsert stays idempotent.

[parthrohit22]

This is careful work — the connection-lifecycle rework (discard-and-reacquire on an aborted/unknown-status transaction instead of trusting a poisoned connection), the stable_finding_key() identity hash excluding presentation fields so retries update rather than duplicate, and the legacy:<id> backfill for existing rows before adding the unique index (with CREATE INDEX CONCURRENTLY in an autocommit block, so it doesn't lock writes) are all the right calls. CI is green.

One real blocker before this can merge, not about the code itself: this PR's first migration (e4f7a9b2c6d8) forks off d8e4f6a1b2c3, same as #310's 3a76ff935bf6 — both currently share that parent, so if both land as-is alembic heads ends up with two heads. Whichever of #310/#325 merges second needs to rebase and repoint its down_revision, same as the #308/#310 fork we resolved earlier. Given this PR also touches api/models/finding.py/scanner/worker.py/api/routes/scans.py — the same files #310 rewrites — that rebase is going to be a real one, not just a migration-pointer fix. Worth coordinating merge order with #310 explicitly before either goes in.

Requesting changes only for the migration fork — nothing else jumped out as wrong in what I read.

[ritiksah141]

Ordering proposal for this PR and #321.

My suggestion: merge #321 first as the canonical evaluation contract, then rebase this PR on top of it. Concretely that means dropping the duplicate rule_evaluations migration and the evaluation upsert from this PR, re-chaining the remaining migrations on top of 3f59f83a5253, and keeping the leases, fencing, admission, enrichment, and metrics work here. This matches the resolution agreed above.

Two reasons I want to move this now rather than wait. @SHAURYAKSHARMA24 does not seem active at the moment, and my own draft PR #293 also depends on this resolution landing, since it needs the canonical contract in place before it can move forward.

@m-khan-97 can you approve the ordering, #321 first? With your sign-off we can lock in the rebase plan and unblock #293. Happy to help drive it however is useful.