RHINENG-25740: remove long time locking from culling job - #2159
Merged
Conversation
to aviod locking system_inventory for long time
there's already delete_system() for removal of single system which we will use to avoid locking
to aviod locking system_inventory for long time
for deleteCulledSystems() and markSystemsStale()
Reviewer's GuideRefactors system culling and stale-marking to avoid long-held locks by deleting and updating systems one-by-one in separate transactions, simplifies the delete_system DB function interface, and removes now-unneeded bulk PL/pgSQL helper functions while updating callers and tests accordingly. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
SC Environment Impact AssessmentOverall Impact: ⚪ NONE No SC Environment-specific impacts detected in this PR. What was checkedThis PR was automatically scanned for:
|
![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 2 issues, and left some high level feedback:
- The new
deleteCulledSystemsandSystemDeleteHandlercode usesExec("select delete_system(?::uuid)", ...); issuing aSELECTthroughExecis driver‑dependent and may fail or always reportRowsAffected = 0, so consider switching toRaw("select delete_system(?::uuid)", ...).Scan(&unused)(or changing the function toRETURNS voidand usingPERFORMin SQL) to make the behavior explicit and reliable. - Both
deleteCulledSystemsandmarkSystemsStalenow select candidate rows in one transaction and then operate on each row in independent transactions without any locking on the initial query, which can lead to races or double processing under concurrency; consider adding row‑level locking (e.g.,FOR UPDATE SKIP LOCKEDvia GORM) or another coordination mechanism to preserve the previous mutual‑exclusion semantics while still avoiding long‑held locks on large batches. - Inside
deleteCulledSystems/markSystemsStale, each per‑row operation usestasks.CancelableDB().Transaction(...)instead of the passed‑intx, which may ignore the caller’s transaction/context; if that’s not intentional, it would be safer to derive the per‑row operations from the providedtxor its context.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `deleteCulledSystems` and `SystemDeleteHandler` code uses `Exec("select delete_system(?::uuid)", ...)`; issuing a `SELECT` through `Exec` is driver‑dependent and may fail or always report `RowsAffected = 0`, so consider switching to `Raw("select delete_system(?::uuid)", ...).Scan(&unused)` (or changing the function to `RETURNS void` and using `PERFORM` in SQL) to make the behavior explicit and reliable.
- Both `deleteCulledSystems` and `markSystemsStale` now select candidate rows in one transaction and then operate on each row in independent transactions without any locking on the initial query, which can lead to races or double processing under concurrency; consider adding row‑level locking (e.g., `FOR UPDATE SKIP LOCKED` via GORM) or another coordination mechanism to preserve the previous mutual‑exclusion semantics while still avoiding long‑held locks on large batches.
- Inside `deleteCulledSystems`/`markSystemsStale`, each per‑row operation uses `tasks.CancelableDB().Transaction(...)` instead of the passed‑in `tx`, which may ignore the caller’s transaction/context; if that’s not intentional, it would be safer to derive the per‑row operations from the provided `tx` or its context.
## Individual Comments
### Comment 1
<location path="tasks/system_culling/system_culling.go" line_range="62-71" />
<code_context>
+ }
+
+ for _, id := range inventoryIDs {
+ var rowsAffected int64
+ delErr := tasks.CancelableDB().Transaction(func(tx2 *gorm.DB) error {
+ res := tx2.Exec("select delete_system(?::uuid)", id)
+ if res.Error != nil {
+ return res.Error
+ }
+ rowsAffected = res.RowsAffected
+ return nil
+ })
+ if delErr != nil {
+ utils.LogWarn("inventoryID", id, "err", delErr.Error(), "Delete culled system")
+ continue
+ }
+ nDeleted += rowsAffected
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Using Exec + RowsAffected on a SELECTed function likely miscounts deletions
In PostgreSQL, `SELECT delete_system(?::uuid)` always returns one row per call, so `RowsAffected` will be `1` even when nothing was actually deleted. As a result, `nDeleted` will usually equal `len(inventoryIDs)` rather than the true number of deletions.
To keep an accurate count (and match the old `delete_culled_systems()` behavior), consider either:
- Having `delete_system` return a boolean/int and using `Row().Scan(&deleted)`; or
- Returning NULL vs non-NULL and checking the returned value in Go instead of relying on `RowsAffected`.
</issue_to_address>
### Comment 2
<location path="tasks/system_culling/system_culling.go" line_range="49" />
<code_context>
- Find(&nDeletedArr).Error
- if len(nDeletedArr) > 0 {
- nDeleted = nDeletedArr[0]
+// systems are deleted in independent transactions to avoid locking multiple rows for long time
+func deleteCulledSystems(tx *gorm.DB, limitDeleted int) (nDeleted int64, err error) {
+ var inventoryIDs []string
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting a shared per-row transaction helper and a named candidate type to remove duplicated boilerplate and make the culling and staleness logic easier to follow.
You can keep the per-row transactional behavior but reduce the incidental complexity with a small helper and simpler data shapes.
### 1. Remove closure-mutated `rowsAffected` and per-function boilerplate
Extract a tiny helper that runs a per-row operation in its own transaction and returns the affected rows. That removes the “outer variable mutated in closure” and most boilerplate:
```go
func withPerRowTx(do func(tx *gorm.DB) (int64, error)) (int64, error) {
var rowsAffected int64
err := tasks.CancelableDB().Transaction(func(tx2 *gorm.DB) error {
var opErr error
rowsAffected, opErr = do(tx2)
return opErr
})
if err != nil {
return 0, err
}
return rowsAffected, nil
}
```
Then `deleteCulledSystems` becomes:
```go
func deleteCulledSystems(tx *gorm.DB, limitDeleted int) (int64, error) {
var inventoryIDs []string
if err := tx.Model(&models.SystemInventory{}).
Where("culled_timestamp < ?", time.Now()).
Order("id").
Limit(limitDeleted).
Pluck("inventory_id", &inventoryIDs).Error; err != nil {
return 0, err
}
var nDeleted int64
for _, id := range inventoryIDs {
rows, err := withPerRowTx(func(tx2 *gorm.DB) (int64, error) {
res := tx2.Exec("select delete_system(?::uuid)", id)
return res.RowsAffected, res.Error
})
if err != nil {
utils.LogWarn("inventoryID", id, "err", err.Error(), "Delete culled system")
continue
}
nDeleted += rows
}
return nDeleted, nil
}
```
And `markSystemsStale` uses the same helper:
```go
for _, c := range candidates {
rows, err := withPerRowTx(func(tx2 *gorm.DB) (int64, error) {
res := tx2.Model(&models.SystemInventory{}).
Where("rh_account_id = ? AND id = ?", c.RhAccountID, c.ID).
Update("stale", c.Expired)
return res.RowsAffected, res.Error
})
if err != nil {
utils.LogWarn("rhAccountID", c.RhAccountID, "systemID", c.ID, "err", err.Error(), "Mark stale system")
continue
}
nMarked += rows
}
```
This keeps semantics (per-row transactions, same logging) but removes duplicated plumbing and cross-closure state.
### 2. Use a named type and compute `Expired` in Go
You can simplify the query and get rid of the anonymous struct + computed SQL column by using a named candidate type and computing `Expired` in Go:
```go
type staleCandidate struct {
RhAccountID int `gorm:"column:rh_account_id"`
ID int64 `gorm:"column:id"`
Stale bool `gorm:"column:stale"`
StaleWarningTimestamp time.Time `gorm:"column:stale_warning_timestamp"`
}
func markSystemsStale(tx *gorm.DB, markedLimit int) (int64, error) {
now := time.Now()
var candidates []staleCandidate
if err := tx.Model(&models.SystemInventory{}).
Select("rh_account_id, id, stale, stale_warning_timestamp").
Where("stale != (stale_warning_timestamp < ?)", now).
Order("rh_account_id").Order("id").
Limit(markedLimit).
Find(&candidates).Error; err != nil {
return 0, err
}
var nMarked int64
for _, c := range candidates {
expired := c.StaleWarningTimestamp.Before(now)
rows, err := withPerRowTx(func(tx2 *gorm.DB) (int64, error) {
res := tx2.Model(&models.SystemInventory{}).
Where("rh_account_id = ? AND id = ?", c.RhAccountID, c.ID).
Update("stale", expired)
return res.RowsAffected, res.Error
})
if err != nil {
utils.LogWarn("rhAccountID", c.RhAccountID, "systemID", c.ID, "err", err.Error(), "Mark stale system")
continue
}
nMarked += rows
}
return nMarked, nil
}
```
This:
- Keeps the per-row transaction and existing `WHERE stale != (...)` semantics.
- Moves the `expired` computation into Go, making the query projection simpler.
- Replaces the anonymous struct with a named, reusable type.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2159 +/- ##
==========================================
+ Coverage 59.29% 59.30% +0.01%
==========================================
Files 135 135
Lines 8748 8785 +37
==========================================
+ Hits 5187 5210 +23
- Misses 3022 3030 +8
- Partials 539 545 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
3 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.
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Refactor system culling and staleness handling to avoid long-lived database locks and simplify deletion routines.
Bug Fixes:
Enhancements:
Tests:
Chores: