RHINENG-29746: backport notified - #2337
Conversation
Co-authored-by: Claude <noreply@anthropic.com>
Reviewer's GuideBackports advisory notification state synchronization by copying notified timestamps from advisory_account_data into account_advisory during backfills and notification processing, and by inheriting existing notification state for newly inserted workspace records. The database migration adds the trigger and updated backfill logic, with corresponding rollback support. 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 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="database_admin/migrations/168_backport_advisories_notified.up.sql" line_range="22-28" />
<code_context>
+ RETURNS TRIGGER AS
+$sync_notified_insert$
+BEGIN
+ IF NEW.notified IS NULL THEN
+ SELECT notified INTO NEW.notified
+ FROM account_advisory
+ WHERE rh_account_id = NEW.rh_account_id
+ AND advisory_id = NEW.advisory_id
+ AND notified IS NOT NULL
+ LIMIT 1;
+ END IF;
+ RETURN NEW;
</code_context>
<issue_to_address>
**issue (bug_risk):** A concurrent workspace insert can permanently retain a NULL `notified` value: the trigger reads existing `account_advisory` rows before the notification transaction updates them, while the notification UPDATE can finish before the new insert commits. The new row then remains unnotified even though `advisory_account_data.notified` and the existing workspace rows are set.
**Triggers:** When a new workspace for an already-notified advisory is inserted concurrently with `markAdvisoriesNotified`.
**Suggested fix:** Synchronize the insert and notification update with a locking or atomic database operation, or derive the inserted value from `advisory_account_data.notified` as well as existing `account_advisory` rows.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if the backfill or synchronization logic is wrong, it can persist incorrect notification timestamps in account_advisory and cause notifications to be suppressed or retriggered. Reverting removes the trigger and code but does not undo those stored values, though the affected records are bounded and can be repaired or recomputed.
Blocking findings: database_admin/migrations/168_backport_advisories_notified.up.sql:28
| IF NEW.notified IS NULL THEN | ||
| SELECT notified INTO NEW.notified | ||
| FROM account_advisory | ||
| WHERE rh_account_id = NEW.rh_account_id | ||
| AND advisory_id = NEW.advisory_id | ||
| AND notified IS NOT NULL | ||
| LIMIT 1; |
There was a problem hiding this comment.
issue (bug_risk): A concurrent workspace insert can permanently retain a NULL notified value: the trigger reads existing account_advisory rows before the notification transaction updates them, while the notification UPDATE can finish before the new insert commits. The new row then remains unnotified even though advisory_account_data.notified and the existing workspace rows are set.
Triggers: When a new workspace for an already-notified advisory is inserted concurrently with markAdvisoriesNotified.
Suggested fix: Synchronize the insert and notification update with a locking or atomic database operation, or derive the inserted value from advisory_account_data.notified as well as existing account_advisory rows.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2337 +/- ##
==========================================
+ Coverage 58.92% 58.94% +0.02%
==========================================
Files 150 150
Lines 9596 9601 +5
==========================================
+ Hits 5654 5659 +5
+ Misses 3349 3348 -1
- Partials 593 594 +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:
|
3 participants
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Synchronize advisory notification status across account and workspace advisory data.
Bug Fixes:
Enhancements:
Chores: