perf(notificator): drop unknown CVEs queue#2419
Merged
Merged
Conversation
this is a trade-off to keep memory usage reasonable after the number of notifications heavily increased system-level notifications for unknown CVEs now fire immediately only for the -all event type account-level notifications will be sent when CVEs receives metadata without CVE metadata we can't categorize is CVE is fresh, what's the CVSS score or severity RHINENG-28638
Reviewer's GuideThis PR removes the separate "unknown CVEs" queue and treats CVEs without public_date as immediately notifiable only for system-cve-all, while tightening CVE existence/freshness logic and updating tests and docs accordingly. Sequence diagram for CVE notification flow after removing unknown CVEs queuesequenceDiagram
actor Evaluator
participant Notificator
participant NotificatorConditions
participant QueueProcessor
participant NotificationsPlatform
Evaluator->>Notificator: _process_new_sys_vulns(new_sys_vulns, rh_account_id, org_id, inventory_id, display_name)
loop for each sys_vuln_id
Notificator->>NotificatorConditions: cve_exists(cve_id)
alt cve_id not in cve_map or public_date is None
NotificatorConditions->>NotificatorConditions: _fetch_cve(cve_id)
end
NotificatorConditions-->>Notificator: cve_exists result
alt cve_exists is True
Notificator->>NotificatorConditions: make_events_for_cve(cve_id)
NotificatorConditions-->>Notificator: events (e.g. system-cve-all when public_date is None)
alt events is not empty
Notificator->>QueueProcessor: notif_cves_queue[sys_vuln_id] = QueueItem(..., events)
end
end
end
loop every period
QueueProcessor->>QueueProcessor: _process_normal_queue()
loop for each sys_vuln_id in notif_cves_queue
QueueProcessor->>QueueProcessor: _create_notif_events(cve_id)
QueueProcessor->>NotificationsPlatform: notifications_topic.send(events)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- Using
datetime.fromtimestamp(0, tz=timezone.utc)as a syntheticpublish_datefor CVEs withoutpublic_datemay be misleading downstream; consider omitting the field or explicitly indicating an unknown/null value in the payload instead of an epoch placeholder. - The updated
cve_existswill repeatedly refetch CVEs withpublic_date is Noneon every check; consider caching this "known but not yet published" state to avoid unnecessary DB round-trips until the public date is actually populated. - The new
(Re)fetched CVElog is atinfolevel and may become noisy in production for frequently re-checked CVEs; consider lowering this todebugor rate-limiting it.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `datetime.fromtimestamp(0, tz=timezone.utc)` as a synthetic `publish_date` for CVEs without `public_date` may be misleading downstream; consider omitting the field or explicitly indicating an unknown/null value in the payload instead of an epoch placeholder.
- The updated `cve_exists` will repeatedly refetch CVEs with `public_date is None` on every check; consider caching this "known but not yet published" state to avoid unnecessary DB round-trips until the public date is actually populated.
- The new `(Re)fetched CVE` log is at `info` level and may become noisy in production for frequently re-checked CVEs; consider lowering this to `debug` or rate-limiting it.
## Individual Comments
### Comment 1
<location path="notificator/notificator_queue.py" line_range="114-115" />
<code_context>
"known_exploit": cve["exploits"],
"has_rule": cve["is_rule"],
- "publish_date": cve["public_date"].isoformat(),
+ "publish_date": (
+ cve["public_date"].isoformat() if cve["public_date"] else datetime.fromtimestamp(0, tz=timezone.utc).isoformat()
+ ),
},
</code_context>
<issue_to_address>
**suggestion:** Using Unix epoch as a fallback publish_date may be misleading downstream.
This fallback makes it impossible to distinguish a truly early CVE from one with missing `public_date`, which can affect sorting, freshness checks, and analytics. Instead, either omit `publish_date` when it's `None`, propagate `None`, or add a separate sentinel/flag so downstream code can explicitly handle unknown dates.
Suggested implementation:
```python
"impact_id": cve["impact_id"],
"known_exploit": cve["exploits"],
"has_rule": cve["is_rule"],
"publish_date": (
cve["public_date"].isoformat() if cve["public_date"] is not None else None
),
```
Downstream code that consumes `publish_date` should be updated (if not already robust) to:
1. Treat `None`/`null` as "unknown publish date".
2. Adjust any sort/analytics logic that previously assumed `publish_date` is always a string ISO timestamp so it explicitly handles the `None` case instead of silently treating it as an early date.
</issue_to_address>
### Comment 2
<location path="notificator/notificator.py" line_range="93-95" />
<code_context>
cve["is_rule"] = cve_row[6]
cve["public_date"] = cve_row[7]
self.cve_map[cve_row[0]] = cve
+ LOGGER.info("(Re)fetched CVE: %s", cve["cve"])
return True
return False
</code_context>
<issue_to_address>
**suggestion (performance):** Logging every (re)fetched CVE at info level may generate excessive log volume.
If `_fetch_cve` runs frequently or over many CVEs, this info-level log per item could flood logs and hurt usability/storage. Consider lowering to debug or aggregating/rate-limiting these messages while retaining necessary visibility.
```suggestion
self.cve_map[cve_row[0]] = cve
LOGGER.debug("(Re)fetched CVE: %s", cve["cve"])
return True
```
</issue_to_address>
### Comment 3
<location path="tests/notificator_tests/test_notificator_queue.py" line_range="92-101" />
<code_context>
+ def _send_kafka_notif_mock(org_id, inventory_id, display_name, event_type, events, *_, **__):
</code_context>
<issue_to_address>
**suggestion (testing):** Strengthen the unknown-CVE test by asserting the number of events sent in the batched message
Since `msgs.append((inventory_id, event_type, len(events)))` already records the batch size, you can also assert `msgs[0][2] == 1` to verify that the batched event count is exactly one for unknown CVEs, not just that a single `SYSTEM_ALL_NOTIFICATION` message was sent.
Suggested implementation:
```python
msgs = []
def _send_kafka_notif_mock(org_id, inventory_id, display_name, event_type, events, *_, **__):
msgs.append((inventory_id, event_type, len(events)))
```
```python
assert len(msgs) == 1
# Verify that the batched event count is exactly one for unknown CVEs
assert msgs[0][2] == 1
```
I assumed the unknown-CVE test already asserts `len(msgs) == 1` after invoking the notificator with the `_send_kafka_notif_mock`. If the assertion currently has a different form (for example, checking the full tuple contents or indexing differently into `msgs`), you should:
1. Locate the test case that exercises unknown CVEs and uses `msgs` populated via `_send_kafka_notif_mock`.
2. Add `assert msgs[0][2] == 1` (or the equivalent index, if the tuple structure differs) right next to the existing assertions to ensure the batch size for the unknown CVE notification is exactly one.
3. If `msgs` is structured differently (e.g., more fields in the tuple), adjust the index `2` to point to the stored `len(events)` value.
</issue_to_address>
### Comment 4
<location path="notificator/README.md" line_range="31" />
<code_context>
```
-There are two queues, the unknown cves queue and known cves queue.
-Queue resolver process runs every 1 minute and checks both queues, for normal cve queue:
+Queue resolver process runs every 1 minute and checks the queue:
* Checks if customer was already notified about this CVE.
</code_context>
<issue_to_address>
**nitpick (typo):** Consider rephrasing "every 1 minute" to the more natural "every minute".
"Every 1 minute" is understandable but a bit awkward; "every minute" is more natural and keeps the same meaning.
```suggestion
Queue resolver process runs every minute and checks the queue:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jdobes
enabled auto-merge (squash)
July 15, 2026 16:01
pindo696
approved these changes
Jul 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
this is a trade-off to keep memory usage reasonable after the number of notifications heavily increased
system-level notifications for unknown CVEs now fire immediately only for the -all event type
account-level notifications will be sent when CVEs receives metadata
without CVE metadata we can't categorize is CVE is fresh, what's the CVSS score or severity
RHINENG-28638
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Simplify CVE notification handling by removing the separate unknown-CVE queue and adjusting logic for CVEs without public_date.
New Features:
Enhancements:
Documentation:
Tests: