Repository navigation
RHINENG-32308: sync template advisories from the message - #2373
Merged
Merged
Conversation
Reviewer's GuideTemplate events now carry optional advisory IDs, and the listener can synchronize template advisories directly from those message snapshots when 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 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="listener/templates.go" line_range="156" />
<code_context>
- ctx := identity.WithIdentity(base.Context,
+ switch {
+ case templatesWithAdvisories:
+ err = applyTemplateAdvisories(ctx, accountID, row.ID, template.UUID, template.OrgID, template.AdvisoryIDs)
+ if err != nil {
+ return errors.Wrap(err, "syncing template advisories from message")
</code_context>
<issue_to_address>
**Failed advisory sync stays stale**
When an advisory lookup or database write fails during a message-based sync, `TemplateUpdate` returns the error, but `TemplatesMessageHandler` only logs it and returns nil, so `MakeRetryingHandler` acknowledges the message and the stored advisory state remains stale or partial.
Propagate the per-template error from `TemplatesMessageHandler` so the retry handler can retry the message.
Also at `listener/templates.go:157-158`.
</issue_to_address>
### Comment 2
<location path="listener/template_advisories.go" line_range="32-39" />
<code_context>
+// applyTemplateAdvisories diffs advisory names against template_advisory and writes the change.
+// advisoryIDs is the full snapshot. Nil and empty both mean no advisories.
+func applyTemplateAdvisories(ctx context.Context, accountID int, templateID int64, templateUUID, orgID string,
+ advisoryIDs []string) error {
stored, err := lookUpTemplateAdvisories(database.DB(ctx), accountID, templateID)
if err != nil {
return errors.Wrap(err, "looking up stored template advisories")
}
- toAdd, toRemove := diffTemplateAdvisories(current.AdvisoryIDs, stored)
+ toAdd, toRemove := diffTemplateAdvisories(advisoryIDs, stored)
if len(toAdd) == 0 && len(toRemove) == 0 {
utils.LogInfo("template_uuid", templateUUID, "template advisories unchanged")
</code_context>
<issue_to_address>
**Concurrent snapshots mix advisory rows**
When two handlers process different snapshots for the same template concurrently, `applyTemplateAdvisories` reads and diffs the stored rows before opening its transaction, so both handlers can apply additions based on the same old set and leave rows from both snapshots; callers see an advisory set matching neither snapshot.
Serialize updates per template and read and diff the stored rows within that serialized transaction.
</issue_to_address>
### Comment 3
<location path="listener/templates.go" line_range="156" />
<code_context>
- ctx := identity.WithIdentity(base.Context,
+ switch {
+ case templatesWithAdvisories:
+ err = applyTemplateAdvisories(ctx, accountID, row.ID, template.UUID, template.OrgID, template.AdvisoryIDs)
+ if err != nil {
+ return errors.Wrap(err, "syncing template advisories from message")
</code_context>
<issue_to_address>
**Older snapshots overwrite newer advisories**
When an older template event is processed after a newer event, `TemplateUpdate` applies each message's `AdvisoryIDs` as the full snapshot without comparing event times or revisions, so `applyTemplateAdvisories` replaces the newer advisory set with stale rows.
Compare the event time or revision with the latest processed value and ignore stale snapshots.
Also at `listener/templates.go:157`.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and when enabled, the message snapshot is authoritative: missing or empty advisory_ids can delete stored template-advisory rows and trigger reevaluation. The default is off, and the rows are bounded and can be restored by resyncing the correct snapshot, but reverting the code alone does not restore deleted rows.
Blocking findings: listener/templates.go:156, listener/template_advisories.go:39, listener/templates.go:156
| ctx := identity.WithIdentity(base.Context, | ||
| switch { | ||
| case templatesWithAdvisories: | ||
| err = applyTemplateAdvisories(ctx, accountID, row.ID, template.UUID, template.OrgID, template.AdvisoryIDs) |
There was a problem hiding this comment.
🟠 High · Failed advisory sync stays stale
When an advisory lookup or database write fails during a message-based sync, TemplateUpdate returns the error, but TemplatesMessageHandler only logs it and returns nil, so MakeRetryingHandler acknowledges the message and the stored advisory state remains stale or partial.
Propagate the per-template error from TemplatesMessageHandler so the retry handler can retry the message.
Also at listener/templates.go:157-158.
Prompt for AI agents
In `listener/templates.go` at line 156:
**Failed advisory sync stays stale**
When an advisory lookup or database write fails during a message-based sync, `TemplateUpdate` returns the error, but `TemplatesMessageHandler` only logs it and returns nil, so `MakeRetryingHandler` acknowledges the message and the stored advisory state remains stale or partial.
Propagate the per-template error from `TemplatesMessageHandler` so the retry handler can retry the message.
Also at `listener/templates.go:157-158`.
Comment on lines
+32
to
+39
| func applyTemplateAdvisories(ctx context.Context, accountID int, templateID int64, templateUUID, orgID string, | ||
| advisoryIDs []string) error { | ||
| stored, err := lookUpTemplateAdvisories(database.DB(ctx), accountID, templateID) | ||
| if err != nil { | ||
| return errors.Wrap(err, "looking up stored template advisories") | ||
| } | ||
|
|
||
| toAdd, toRemove := diffTemplateAdvisories(current.AdvisoryIDs, stored) | ||
| toAdd, toRemove := diffTemplateAdvisories(advisoryIDs, stored) |
There was a problem hiding this comment.
🟠 High · Concurrent snapshots mix advisory rows
When two handlers process different snapshots for the same template concurrently, applyTemplateAdvisories reads and diffs the stored rows before opening its transaction, so both handlers can apply additions based on the same old set and leave rows from both snapshots; callers see an advisory set matching neither snapshot.
Serialize updates per template and read and diff the stored rows within that serialized transaction.
Prompt for AI agents
In `listener/template_advisories.go` at lines 32-39:
**Concurrent snapshots mix advisory rows**
When two handlers process different snapshots for the same template concurrently, `applyTemplateAdvisories` reads and diffs the stored rows before opening its transaction, so both handlers can apply additions based on the same old set and leave rows from both snapshots; callers see an advisory set matching neither snapshot.
Serialize updates per template and read and diff the stored rows within that serialized transaction.| ctx := identity.WithIdentity(base.Context, | ||
| switch { | ||
| case templatesWithAdvisories: | ||
| err = applyTemplateAdvisories(ctx, accountID, row.ID, template.UUID, template.OrgID, template.AdvisoryIDs) |
There was a problem hiding this comment.
🟠 High · Older snapshots overwrite newer advisories
When an older template event is processed after a newer event, TemplateUpdate applies each message's AdvisoryIDs as the full snapshot without comparing event times or revisions, so applyTemplateAdvisories replaces the newer advisory set with stale rows.
Compare the event time or revision with the latest processed value and ignore stale snapshots.
Also at listener/templates.go:157.
Prompt for AI agents
In `listener/templates.go` at line 156:
**Older snapshots overwrite newer advisories**
When an older template event is processed after a newer event, `TemplateUpdate` applies each message's `AdvisoryIDs` as the full snapshot without comparing event times or revisions, so `applyTemplateAdvisories` replaces the newer advisory set with stale rows.
Compare the event time or revision with the latest processed value and ignore stale snapshots.
Also at `listener/templates.go:157`.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2373 +/- ##
==========================================
+ Coverage 59.89% 59.90% +0.01%
==========================================
Files 157 157
Lines 10119 10127 +8
==========================================
+ Hits 6061 6067 +6
- Misses 3431 3432 +1
- Partials 627 628 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
katarinazaprazna
approved these changes
Oct 28, 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
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.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Enable configurable template advisory synchronization from template event messages while retaining the existing Content Sources API fallback.
New Features:
Enhancements:
Documentation:
Tests: