fix(evaluator): recover cache when cve gets re-imported

[pindo696]

Evaluators CVE cache is now able to recover for re-imported CVE. RHINENG-29405

Secure Coding Practices Checklist GitHub Link

• https://github.com/RedHatInsights/secure-coding-checklist

Secure Coding Checklist

• Input Validation
• Output Encoding
• Authentication and Password Management
• Session Management
• Access Control
• Cryptographic Practices
• Error Handling and Logging
• Data Protection
• Communication Security
• System Configuration
• Database Security
• File Management
• Memory Management
• General Coding Practices

Summary by Sourcery

Refresh the evaluator CVE cache after completed VMAAS CVE imports while avoiding unnecessary database reloads through configurable cache expiration.

New Features:

• Add configurable TTL-based refresh behavior for the evaluator CVE cache.

Bug Fixes:

• Recover evaluator CVE cache contents after CVE metadata is re-imported by detecting completed VMAAS CVE synchronization.

Enhancements:

• Centralize evaluator cache expiration metadata for rule and CVE caches.

Deployment:

• Expose the CVE cache TTL setting through evaluator deployment configuration.

Tests:

• Add coverage for CVE cache expiration, synchronization detection, reload behavior, and TTL reset handling.

[sourcery-ai]

Reviewer's Guide

Adds a recovery path in evaluator logic for stale CVE cache entries that cause foreign key violations on insert, and introduces focused tests to validate the cache eviction and retry behavior as well as correct error propagation.

Sequence diagram for evaluator CVE cache recovery on foreign key violation

sequenceDiagram
    participant Evaluator
    participant Conn as AsyncConnection
    participant DB as Database

    Evaluator->>Evaluator: _evaluate_vmaas_res(to_insert, to_delete, conn)
    alt has_rows_to_insert
        Evaluator->>Evaluator: _insert_vulnerable_package_cve_with_recovery(to_insert, conn)
        rect rgb(240,240,240)
            Evaluator->>Conn: transaction()
            Evaluator->>DB: _insert_vulnerable_package_cve(to_insert, conn)
            alt ForeignKeyViolation on cve_id
                DB-->>Evaluator: psycopg_errors.ForeignKeyViolation
                Evaluator->>Evaluator: refresh_cve_cache_from_id_to_name
                loop for each (vpid, cve_id) in to_insert
                    Evaluator->>Evaluator: cve_cache.pop(name)
                    Evaluator->>DB: _get_or_upsert_cve(name)
                    DB-->>Evaluator: cve_with_fresh_id
                end
                Evaluator->>DB: _insert_vulnerable_package_cve(refreshed, conn)
                DB-->>Evaluator: insert_success
            else no ForeignKeyViolation
                DB-->>Evaluator: insert_success
            end
        end
    end
    alt has_rows_to_delete
        Evaluator->>DB: _delete_vulnerable_package_cve(to_delete, conn)
    end

Loading

File-Level Changes

Change	Details	Files
Add a cache-recovery insertion path that retries vulnerable_package_cve inserts when a stale CVE cache causes a foreign key violation on cve_id.	Import psycopg errors module to catch specific database exceptions Introduce _insert_vulnerable_package_cve_with_recovery helper that wraps the existing insert in a transaction and handles ForeignKeyViolation on the cve_id constraint On FK violation, rebuild a mapping from cached CVE IDs to names, evict stale cache entries, re-resolve CVEs via _get_or_upsert_cve, and retry the insert once Leave non-cve_id foreign key violations and second-attempt failures to propagate	evaluator/logic.py
Wire the new recovery-aware insertion method into the evaluation flow and add tests to validate recovery and error handling.	Update _evaluate_vmaas_res to call the new _insert_vulnerable_package_cve_with_recovery instead of the original insert helper Add a new test module that builds fake async connections/cursors/transactions to simulate FK violations and verify cache refresh and retry behavior Test successful recovery from stale CVE cache including updated cache ID and inserted rows Test that foreign key violations on other constraints are re-raised and that repeated cve_id violations after retry still propagate	evaluator/logic.py tests/common_tests/test_evaluator_cve_cache.py

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• Accessing e.diag.constraint_name assumes diag is always present; consider defensively checking hasattr(e, "diag") and that e.diag.constraint_name is not None before comparing to avoid attribute errors in unexpected psycopg error shapes.
• The first insert attempt is wrapped in conn.transaction() but the recovery insert is not, which changes transactional behavior compared to the original _insert_vulnerable_package_cve; consider making the second attempt use the same transaction semantics for consistency and atomicity.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- Accessing `e.diag.constraint_name` assumes `diag` is always present; consider defensively checking `hasattr(e, "diag")` and that `e.diag.constraint_name` is not `None` before comparing to avoid attribute errors in unexpected psycopg error shapes.
- The first insert attempt is wrapped in `conn.transaction()` but the recovery insert is not, which changes transactional behavior compared to the original `_insert_vulnerable_package_cve`; consider making the second attempt use the same transaction semantics for consistency and atomicity.

## Individual Comments

### Comment 1
<location path="evaluator/logic.py" line_range="434-439" />
<code_context>
                 to_insert,
             )

+    async def _insert_vulnerable_package_cve_with_recovery(self, to_insert: List[Tuple[int, int]], conn: AsyncConnection):
+        """Insert vulnerable_package_cve rows, recovering from stale CVE cache"""
+        try:
+            async with conn.transaction():
+                await self._insert_vulnerable_package_cve(to_insert, conn)
+        except psycopg_errors.ForeignKeyViolation as e:
+            if e.diag.constraint_name != "cve_id":
+                raise
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against missing/None `diag.constraint_name` when inspecting the ForeignKeyViolation.

This code assumes `e.diag` and `diag.constraint_name` are always set, which isn’t guaranteed. If `e.diag` is `None` or lacks `constraint_name`, the recovery logic will raise `AttributeError` instead of correctly handling the foreign key violation. Consider guarding access, e.g. `if getattr(e.diag, "constraint_name", None) != "cve_id": raise`.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[jdobes]

I don't like this solution very much because it's hijacking the foreign key violation handling on specific table inserts. The issue can generally happen every table with cve_id foreign key. I'd prefer more systematic solution similar to the rule_id cache refresh mechanism:

• Once every few minutes rebuild the whole CVE cache
• Since CVE cache contains about 1000x more items than rule_id cache, we can track when the last vmaas_sync ran and re-build the cache after that (since the bug can happen only after vmaas-sync removes a CVE)

[pindo696]

Okay cool, I got this.
I have switched to a hybrid approach where we set a TTL for warming the cve cache and track the last vmaas_sync using timestamp_kv table.
The TTL must expire first before a cache refresh can happen, and the refresh only triggers if vmaas_sync has run since the last refresh. We're using the TTL as a guard, meaning, "the earliest time we can sync after the last sync" rather than a polling interval "check every N seconds". However, if preferred, switching this to a polling interval is basically a one-line change.

This approach incurs a slightly higher DB load (though the timestamp_kv table is so small the overhead should be truly negligible), but it makes cache retrieval much more responsive to sync changes. The alternative approach would do the opposite, save db rtt at the expense of making the cache less resilient to sync changes.
I initially configured the cache ttl to six minutes, dont know how often the sync happens or how time critical it is.

Edit: Switched from guard to polling strategy and increased the ttl time.