Repository navigation
Conversation
Reviewer's GuideTemplate IDs are now resolved through Candlepin before inventory persistence acquires its row lock, reducing Kafka/upload lag while a second deletion check prevents stale systems from being written after the external lookup. 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="listener/upload.go" line_range="813" />
<code_context>
+ // Resolve template via Candlepin before opening the DB transaction so HTTP
+ // retries do not hold SELECT ... FOR UPDATE on system_inventory or stall
+ // this Kafka consumer while a row lock is held.
+ templateID := hostTemplate(database.DB, accountID, host)
+
tx := database.DB.WithContext(base.Context).Begin()
</code_context>
<issue_to_address>
**issue (performance):** Recently deleted systems still execute Candlepin resolution, including its retry delay, before `processUpload` checks `DeletedSystem` and discards the upload. A slow or unavailable Candlepin therefore continues to block the single-threaded Kafka listener for uploads that should be rejected without external work.
**Triggers:** When an upload arrives for a system deleted within the last hour and Candlepin is slow or unavailable.
**Suggested fix:** Perform a preliminary deleted-system check before calling `hostTemplate`, then retain the transactional check before writing to avoid the deletion race.
```suggestion
var preliminaryDeleted models.DeletedSystem
if err := database.DB.Find(&preliminaryDeleted, "inventory_id = ?", host.ID).Error; err != nil &&
!errors.Is(err, gorm.ErrRecordNotFound) {
return nil, base.WrapFatalDBError(err, "checking deleted systems")
}
if preliminaryDeleted.InventoryID != uuid.Nil && preliminaryDeleted.WhenDeleted.After(time.Now().Add(-deletionThreshold)) {
utils.LogInfo("inventoryID", host.ID, "Received recently deleted system")
return nil, nil
}
// Resolve template via Candlepin before opening the DB transaction so HTTP
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 1 finding to address first, and if template resolution is wrong or becomes stale before the transaction commits, the resulting TemplateID is persisted on the system record and may drive later behavior. Reverting stops the new timing, but does not remove assignments already written; those are bounded and can be corrected by a subsequent upload or recomputation.
Blocking findings: listener/upload.go:813
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2352 +/- ##
==========================================
- Coverage 58.95% 58.95% -0.01%
==========================================
Files 150 150
Lines 9612 9626 +14
==========================================
+ Hits 5667 5675 +8
- Misses 3349 3352 +3
- Partials 596 599 +3
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:
|
MichaelMraka
requested changes
Oct 5, 2026
| // Resolve template via Candlepin before opening the DB transaction so HTTP | ||
| // retries do not hold SELECT ... FOR UPDATE on system_inventory or stall | ||
| // this Kafka consumer while a row lock is held. | ||
| templateID := hostTemplate(database.DB, accountID, host) |
Collaborator
There was a problem hiding this comment.
this should go after
tx := database.DB.WithContext(base.Context).Begin()
defer tx.Rollback()
and reuse opened tx so it's properly closed with the potential context close.
Contributor
Author
There was a problem hiding this comment.
You mean:
tx := database.DB.WithContext(base.Context).Begin() defer tx.Rollback() templateID := hostTemplate(tx, accountID, host)
That conflicts with the PR goal: keep Candlepin HTTP (with retries) outside an open transaction. Putting it after Begin() still avoids SELECT … FOR UPDATE (that happens later in updateSystemPlatform), but holds a DB connection for the whole Candlepin wait.
Is this compromise that keeps the lock fix and addresses context: call before Begin(), but with context acceptable?:
templateID := hostTemplate(database.DB.WithContext(base.Context), accountID, host)
tx := database.DB.WithContext(base.Context).Begin()
Let me know, option 1 or 2, and thank you for the review.
Collaborator
There was a problem hiding this comment.
open db transaction is not a problem. it's SELECT … FOR UPDATE which creates a lock.
so we just need run hostTemplate() before the lock is created.
Creating new session with database.DB.WithContext(base.Context) "costs" more than keeping open transaction (without lock) a bit longer.
I prefer 1.
swadeley
force-pushed
the
swadeley/fix_inventory_template_assignment_lag
branch
from
October 6, 2026 10:14
f50cc77 to
89e1cb6
Compare
Resolve Candlepin template assignment in the upload transaction before SELECT ... FOR UPDATE so HTTP retries do not hold a system_inventory row lock. Skip recently deleted systems before Candlepin and re-check afterward so dropped uploads do not wait on HTTP retries. Co-authored-by: Cursor <cursoragent@cursor.com>
swadeley
force-pushed
the
swadeley/fix_inventory_template_assignment_lag
branch
from
October 6, 2026 10:16
89e1cb6 to
f7a786a
Compare
Contributor
Author
|
Rebased |
Contributor
Author
|
@sourcery-ai review |
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/upload.go" line_range="823-826" />
<code_context>
+ // updateSystemPlatform so HTTP retries do not hold a row lock.
+ templateID := hostTemplate(tx, accountID, host)
+
+ if skip, err := skipRecentlyDeleted(tx, host.ID); err != nil || skip {
+ return nil, err
}
- sys, err := updateSystemPlatform(tx, accountID, host, yumUpdates, &updatesReq)
+ sys, err := updateSystemPlatform(tx, accountID, host, yumUpdates, &updatesReq, templateID)
if err != nil {
return nil, errors.Wrap(err, "saving system into the database")
</code_context>
<issue_to_address>
**Deleted systems are revived**
When a delete commits after the tombstone check but before the upload upsert, `updateSystemPlatform` upserts the upload's `culled_timestamp` over the delete's culled state, leaving the deleted system active and potentially clearing its tombstone.
Make the upload upsert preserve a delete committed after the tombstone check instead of overwriting its culled state.
</issue_to_address>
### Comment 2
<location path="listener/upload.go" line_range="821" />
<code_context>
- return nil, nil
+ // Resolve template via Candlepin before SELECT ... FOR UPDATE in
+ // updateSystemPlatform so HTTP retries do not hold a row lock.
+ templateID := hostTemplate(tx, accountID, host)
+
+ if skip, err := skipRecentlyDeleted(tx, host.ID); err != nil || skip {
</code_context>
<issue_to_address>
**Slow Candlepin ties up DB connections**
When candlepin is slow, retrying or stalled during template resolution, `processUpload` has already begun `tx` when `hostTemplate` waits on Candlepin, keeping the transaction and pooled connection open until the call returns; slow requests consume connections and delay uploads and other database work.
Resolve the template before `tx.Begin()` so the Candlepin call does not hold an open transaction or pooled connection.
</issue_to_address>
### Comment 3
<location path="listener/upload.go" line_range="821" />
<code_context>
- return nil, nil
+ // Resolve template via Candlepin before SELECT ... FOR UPDATE in
+ // updateSystemPlatform so HTTP retries do not hold a row lock.
+ templateID := hostTemplate(tx, accountID, host)
+
+ if skip, err := skipRecentlyDeleted(tx, host.ID); err != nil || skip {
</code_context>
<issue_to_address>
**Invalid uploads block event processing**
When an upload with a template repository has missing or invalid workspace data and Candlepin is slow or retrying, `processUpload` calls `hostTemplate` before `updateSystemPlatform` validates `host.Groups`, so Candlepin retries delay the workspace error and hold up later events on that listener.
Validate the workspace group before calling `hostTemplate`, so invalid uploads are rejected without waiting on Candlepin.
</issue_to_address>Sourcery assessment
Needs a human reviewer. 3 findings to address first, and if template resolution is wrong or stale, the upload can persist an incorrect template association on the inventory record until a later upload or repair recomputes it. Reverting stops future assignments but does not undo associations already written.
Blocking findings: listener/upload.go:826, listener/upload.go:821, listener/upload.go:821
Reject uploads with missing or invalid host groups before calling Candlepin so the Kafka listener does not wait on HTTP retries for events that will fail workspace validation anyway. Co-authored-by: Cursor <cursoragent@cursor.com>
MichaelMraka
approved these changes
Oct 6, 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
4 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.
Moved Candlepin template resolution before the upload DB transaction in patchman-engine.
Previously Candlepin GET /consumers/{owner_id} ran while the upload transaction held a row lock. Slow or failing Candlepin meant:
longer system_inventory locks
the single-threaded Kafka listener blocked with an open tx more lag on platform.inventory.events → systems late or missing in Patch more chance of landing in Patch with empty template_uuid under load
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Resolve Candlepin templates before updating locked inventory records to reduce upload lag and preserve deletion safety.
Bug Fixes:
Enhancements: