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.

Rebased onto current dev (b7e9a40). #321 has since merged, so this branch now integrates the #263 evaluation contract instead of standing apart from it — see Relationship to #321.

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, findings, or evaluation coverage.
• 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. 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.

Changes in this round (review follow-ups)

1. recover_stale_scans is one atomic, non-blocking statement. It previously issued two sequential UPDATEs whose attempt-count predicates disagreed — COALESCE(attempt_count, 1) on the fail branch versus COALESCE(attempt_count, 0) on the retry branch — so a row predating the column counted as one attempt already spent and was retired a run early. Neither statement took SKIP LOCKED: measured against PostgreSQL, one row held by another transaction blocked recovery for the full 3s the lock was held while four other stale scans made no progress at all. Recovery runs at the top of every worker iteration, so that stalls claiming and enrichment too. Both branches are now a single CTE selecting FOR UPDATE SKIP LOCKED; the same measured case returns in 0.01s and recovers every scan except the held one.

attempt_count is documented as counting claims already started (claim_next_pending_scan increments it with the lease), so a scan gets exactly max_attempts executions and NULL reads as zero.

For the record: the two-worker double-claim this was reported as could not be reproduced. Under READ COMMITTED PostgreSQL re-checks the UPDATE predicate after taking the row lock, and claim_next_pending_scan already used SKIP LOCKED, so exactly one worker won both the recovery and the claim before this change. The convoy and the attempt-count asymmetry were real; a barrier-synchronised regression test now pins the single-owner invariant regardless.

2. oldest_lease_age.scan measures lease freshness. It reported CURRENT_TIMESTAMP - MIN(claimed_at), which grows for the whole life of a scan even while its worker heartbeats on schedule — a healthy long scan was indistinguishable from a stalled one, which is the situation the metric exists to detect. It now reports MIN(COALESCE(last_heartbeat_at, claimed_at)), matching what the enrichment counterpart already did. Labels are unchanged.

3. request_fingerprint and the 409 path are removed. The hash covered {"subscription_id": ...} only, while admission already looked the key up as (subscription_id, idempotency_key) — so every request that could reach the comparison carried an identical fingerprint and the documented 409 was unreachable. The existing test only reached it by passing hand-written fingerprints straight to admit_scan.

Making it real needs a second semantic request input and there is none: trigger_scan rejects every body field except subscription_id, and docs/api-reference.md deliberately scopes a key to one subscription. Widening keys to be global would make the 409 reachable but would contradict that documented contract, so the dead model is removed rather than propped up. The column is dropped from a7c5e9d2f1b4 (unreleased on this branch), and the API reference now states why no changed-payload conflict exists.

4. /enrich has one response contract. The COMPLETED early return emitted {message, scan_id} with 200 while every other outcome — and the API reference — used {scan_id, job_id, status, outcome, message}. All four outcomes now come from enqueue_enrichment_job. Two consequences are handled rather than absorbed: a scan enriched before durable jobs existed has no job row, so enqueue_enrichment_job records that work as already finished and returns completed instead of silently re-enriching it; and because a clean scan can finish enrichment with nothing to enrich, the findings-404 guard is skipped once a scan is enriched (otherwise an existing 200 would have become a 404).

5. Every migration here is safe to re-run. alembic/env.py does not set transaction_per_migration, so the whole upgrade shares one transaction — but each autocommit_block() commits it. Any DDL issued before a CONCURRENTLY index build is already durable when that build fails, while alembic_version still names the previous revision, so the retry died on its own committed work before reaching the index recovery. This was reported against a7c5e9d2f1b4 but applies to all five revisions, so all five are fixed: columns use ADD COLUMN IF NOT EXISTS, the two CREATE TABLEs are skipped when the table is present, and every concurrent build drops its index name first (CREATE INDEX ... IF NOT EXISTS would keep an INVALID index, which owns the name but can never serve a query). No data is touched — the duplicate-active-scan preflight still refuses to choose which scan history to discard.

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. Every response carries scan_id, job_id, status, outcome and message.

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

Chained from 3f59f83a5253 (#321's rule_evaluations, the current dev Alembic head):

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.

alembic heads returns exactly d4a8c1e6b2f9 (head). (These are Alembic revision ids, not git SHAs.)

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 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, and now so is retrying any of the five revisions after a partial failure. 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, evaluation coverage, 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

#321 merged while this branch was outstanding, and both branches added their first migration on top of d8e4f6a1b2c3. This branch's first migration is repointed at 3f59f83a5253, so the chain is linear and there is one head — no merge revision.

save_scan now performs #321's evaluation upsert and stale-coverage cleanup inside this PR's fenced transaction rather than alongside it. The evaluation semantics are #321's, unchanged; what this PR adds is that a worker which lost its lease can no longer rewrite another owner's coverage rows. The finding upsert keeps RETURNING id, so a replayed delivery reuses the existing finding row and FAIL evaluations stay linked to it instead of pointing at a row that was deleted and recreated. #321's save_scan tests are adapted to the fenced signature with their assertions intact.

#310 is unmerged and still chains from d8e4f6a1b2c3; whichever of it and this PR merges second repoints its down_revision.

Tests

Re-run after the rebase onto b7e9a40, against postgres:16-alpine:

• Full backend suite: 1045 passed, 3 skipped, 0 failed. The 3 skips are dev's own AI/RAG tests that need a locally built BM25 index (tests/test_ai_hallucination_guard.py); they are unrelated to this PR.
• 43 PostgreSQL-gated tests across scan leases, admission, admission migrations, enrichment jobs and operational metrics — all run, none skipped.
• Alembic validated on real PostgreSQL 16: empty→head; dev Alembic head (3f59f83a5253)→head; downgrade to 3f59f83a5253→upgrade→head; and alembic heads returning exactly d4a8c1e6b2f9 (head).
• ruff check . and ruff format --check . pass.

New regression coverage this round, each verified to fail against the previous implementation:

• Two barrier-synchronised workers transition one stale scan exactly once and the winner's fencing token strictly advances; a row held by another transaction is stepped over rather than waited on; the attempt budget is spent fully before a scan becomes terminal.
• A scan claimed an hour ago reports >= 3600s lease age while silent, then drops as soon as its worker heartbeats even though claimed_at is still an hour old (the previous query still reported 3600.02s).
• Admission replays a repeated key, and the same key under a different subscription admits its own scan; the route returns 200 with the original scan_id on replay and passes no fingerprint to admission.
• Every /enrich outcome returns the same keys with the documented status code; a PostgreSQL test reproduces the legacy no-job-row scan and asserts it resolves to completed without re-queueing.
• Migration partial states replayed against throwaway databases: columns committed with the version stamp behind, a table committed the same way, and an index marked INVALID the way an interrupted build leaves it — all three previously failed with DuplicateColumn/DuplicateTable, and now reach head with the index rebuilt valid.
• An owner writes findings and coverage rows together; a worker whose lease was reclaimed raises LostLease and leaves no coverage behind; a replayed delivery converges with the FAIL row still pointing at the same finding id.

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 #321's contract; this PR does not define it, but its writes are now idempotent and fenced because they happen inside this transaction.

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.
• Idempotency keys carry no changed-payload conflict detection, because a trigger has no request input beyond subscription_id. If the trigger body ever gains a semantic field, a real request fingerprint and the 409 should come back with it.

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.

[ritiksah141]

Re-reviewed the updated branch (rebase onto current dev, plus 8f27e8f and 9da3d3e). All three items from my earlier request for changes are resolved.

1. The duplicate rule_evaluations table is gone. The idempotent_findings_and_evaluations migration was replaced by idempotent_finding_identities, and there are now zero rule_evaluations references in this PR. save_scan carries an explicit comment noting the #263 evaluation contract belongs to #321 and that its writes belong inside this fenced transaction once #321 lands. That is the agreed split.

2. Terminal enrichment jobs are now recoverable. enqueue_enrichment_job explicitly requeues a terminally failed job: it resets attempt_count for a fresh retry budget, keeps the checkpoint so it resumes rather than redoing, and never steals a running claim. It is exposed via POST /api/scans/<scan_id>/enrich with a clear requeued outcome.

3. The INVALID-index risk on the one-active-scan index is handled. The admission migration now pre-checks and raises an actionable error listing exactly which subscriptions have multiple pending/running scans, and it drops both index names first so retrying after an interrupted CONCURRENTLY build is deterministic.

Verification: all eight migrations apply cleanly on a fresh Postgres to a single head d4a8c1e6b2f9, including the CONCURRENTLY index build. Full suite passes 978 tests, all 30 postgres-gated tests run and pass, and the previously flaky duplicate_enqueue_and_claim_race now passes. Ruff check and format are clean, CI is green including DCO. Also confirmed: trigger_scan masks internal errors and validates idempotency keys and subscription authorization; the NVD client uses a hardcoded HTTPS base URL with a timeout; and the heartbeat clamp (heartbeat must be shorter than the lease) is now tested.

What remains is coordination, not code. This needs to merge after #321 and be rebased onto 3f59f83a5253. Both branches currently sit off d8e4f6a1b2c3 and #321 is not yet merged, so merging this as-is would fork the migration chain into two heads. Two things to do at rebase time: fold #321's evaluation upsert block into this fenced save_scan (otherwise merging this PR would silently drop the #263 evaluation persistence that #321 adds), and confirm evaluate()-derived findings get stable finding_keys under the upsert model (they should, since the key derives from rule_id plus resource scope).

[TFT444]

Fencing design is correct and the fault-injection suite is the most thorough in the repo. Five items need fixing before this can merge.

1. recover_stale_scans double-claim race. Two sequential UPDATEs without FOR UPDATE SKIP LOCKED let concurrent workers claim the same scan twice. Collapse into one CTE with SKIP LOCKED. Also fix the asymmetric COALESCE(attempt_count, 1) vs COALESCE(attempt_count, 0) between the fail and retry branches -- scans exhaust retries one attempt early.

2. oldest_lease_age.scan measures the wrong thing. MIN(claimed_at) grows forever during healthy operation. The enrichment counterpart uses last_heartbeat_at correctly -- align scans to match.

3. request_fingerprint is a no-op. The hash only covers subscription_id, so every request for the same subscription produces the same fingerprint. The 409 Conflict path is unreachable and the column does nothing. Include the request body in the hash or remove the field.

4. Enrich route response contract mismatch. The status == "COMPLETED" early return emits {message, scan_id} but the API reference documents {outcome, job_id}. Drop the early return -- enqueue_enrichment_job already handles the completed case and returns the right shape.

5. Migration a7c5e9d2f1b4 re-run fails on partial failure. op.add_column runs before the INVALID-index cleanup, so a re-run after a failed attempt crashes on "column already exists" before reaching the recovery logic. Use ADD COLUMN IF NOT EXISTS.

On the Alembic fork: #310 is currently conflicting with open blockers, so #325 merging first is the right order. No action needed beyond maintainer confirmation.

[parthrohit22]

I reviewed the lease and fencing implementation, scan admission, enrichment queue, migrations, worker lifecycle, observability, and regression tests.

The overall design is strong, but several blocking issues remain around stale-scan double claiming, incorrect lease-age metrics, incomplete request fingerprints, inconsistent enrichment responses, and migration rerun safety. The PR is also currently conflicted.

Please resolve these issues, rebase onto the current dev branch, and rerun the PostgreSQL and migration test suites before requesting another review.

[SHAURYAKSHARMA24]

Addressed the latest blocking review feedback and rebased #325 onto current dev.
Current head: 2575640
Resolved

• Reworked stale-scan recovery to use a single FOR UPDATE SKIP LOCKED recovery path and made attempt_count semantics consistent. The originally reported double-claim was not reproducible under PostgreSQL locking, but the investigation exposed a real recovery convoy and NULL-attempt off-by-one issue; both are fixed with PostgreSQL regression coverage.
• Fixed scan oldest_lease_age to use heartbeat freshness rather than initial claimed_at.
• Removed the ineffective request_fingerprint / unreachable changed-payload 409 path. The scan request contract only contains subscription_id, and idempotency keys are deliberately scoped per subscription.
• Unified /api/scans/<id>/enrich responses through the durable enrichment lifecycle while preserving the completed/no-findings cases.
• Hardened migration rerun behavior after partial failures across all five fix(core): harden scan durability and idempotency (#303) #325 migrations, not only a7c5e9d2f1b4.

dev integration
#321 has now landed, so this rebase also reconciles its canonical rule_evaluations implementation with #325.
The #321 evaluation persistence now executes inside #325's fenced save_scan transaction, preserving the canonical evaluation semantics while adding lease/fencing protection.
#321's four save_scan tests were adapted to the fenced signature with their assertions preserved, so the integration carries the upstream regression coverage rather than only the production code.
The Alembic fork is resolved:
e4f7a9b2c6d8 now chains from 3f59f83a5253.
alembic heads reports exactly:
d4a8c1e6b2f9 (head)
Verification

• Full suite: 1045 passed, 3 skipped, 0 failed
• 43 PostgreSQL-gated tests run and pass
• Fresh PostgreSQL → head: pass
• current-dev-head → PR head: pass
• downgrade → upgrade round trip: pass
• partial-migration rerun scenarios: pass
• ruff check .: pass
• ruff format --check .: pass
• CodeQL: pass
• Semgrep: pass
• Bandit: pass
• DCO: pass
• Zero open code-scanning alerts

The PR is now GitHub-mergeable; the remaining BLOCKED state is from the existing CHANGES_REQUESTED review decisions.
Please re-review the current head when convenient.

[parthrohit22]

Hey @SHAURYAKSHARMA24 ,
Thank you for the thorough work on scan durability and idempotency. The lease/fencing design is correct and the fault-injection suite is comprehensive. However, I have the same concerns as TFT444 regarding terminally failed enrichment jobs being unrecoverable and the recover_stale_scans double-claim race. Additionally, migration re-run safety concerns need addressing. Please resolve these blocking issues, rebase onto current dev, and rerun the PostgreSQL/migration test suites before requesting another review.

[TFT444]

Re-reviewed. The double-claim race is fixed: recover_stale_scans now uses a single FOR UPDATE SKIP LOCKED CTE, and COALESCE(attempt_count, 0) is consistent between retry and fail branches. Approved.