Hi there 👋
| 🌥️ Tech |
☀️ Feat |
| 1 high, 2 medium, 1 low |
4 ok |
The MR addresses the MySQL deadlock (#64979) by splitting the annotation-cleanup DELETE ... WHERE id IN (subquery) into a separate SELECT id ... followed by a DELETE ... WHERE id IN (...) with no shared transaction. The core approach is sound and portable (verified across MySQL/SQLite/Postgres), memory is bounded by the batch-size config, and the integration tests meaningfully exercise both the SELECT and DELETE paths. However, the review surfaced several issues that should be addressed before merge: (1) routine cleanup progress is logged at Error severity with full ids/SQL payloads, causing log flooding; (2) the cleanup ticker was changed from 10 minutes to 1 minute, affecting all cleanup jobs and, combined with the existing 9-minute timeout and absence of a server lock, creates a risk of overlapping runs on the same instance; (3) the placeholder-based delete path may exceed PostgreSQL's binding-parameter limit for very large batch sizes; and (4) the accepted under-delete window between SELECT and DELETE is only documented in an inline comment, not at the caller's contract level. The interval change in particular is unrelated to the deadlock fix and needs explicit justification or adjustment.
These might need a close look
- 🔴 pkg/services/cleanup/cleanup.go (L77)
The cleanup ticker was changed from 10 minutes to 1 minute. This increases the frequency of every cleanup job (not just annotations) by 10x, and with the existing 9-minute timeout and the unused ServerLockService field, a slow clean() run can overlap with the next tick, causing concurrent cleanup jobs on the same instance. This change is unrelated to the deadlock fix and must be confirmed as intentional, documented, and guarded against overlap.
Worth checking
- 🟡 pkg/services/annotations/annotationsimpl/xorm_store.go (L531)
The cleanup routines use log.Error for routine, non-error progress, logging the entire ids slice and the raw SQL condition on every batch. This floods the error log (default batch size 100, and the job now also runs more frequently) and logs err=nil as if it were an error. These should be log.Debug/log.Info, with payloads trimmed or removed.
- 🟡 pkg/services/annotations/annotationsimpl/xorm_store.go (L608)
The SQLite-only inline-values path only applies when r.db.GetDBType() == migrator.SQLite && AnnotationCleanupJobBatchSize > 999. For Postgres, the placeholder path builds DELETE ... WHERE id IN (?,?,...) with up to AnnotationCleanupJobBatchSize bound parameters, which can hit Postgres's 32767-parameter limit if a large batch size is configured (e.g., 32767 as exercised in the test). The inline-values or chunked-placeholder approach should not be gated solely on the SQLite branch for robustness.
Small things (take or leave)
- 🔵 pkg/services/annotations/annotationsimpl/xorm_store.go (L526)
The two-step select-then-delete flow can under-delete if a row fetched by fetchIDs no longer satisfies the filter (or no longer exists) by the time deleteByIDs runs, and loop termination relies on RowsAffected() rather than len(ids). This is acknowledged in the inline comment and is safe (rows are cleaned next cycle), but it should be explicitly accepted for the MaxCount path, where the 'keep newest N' guarantee becomes approximate under concurrency.
✅ Feature-level checklist
Looks good
- ✅ Resolve the MySQL deadlock (#64979) by splitting the cleanup subquery into separate SQL statements that do not share a transaction.
The change correctly splits the DELETE subquery into a separate SELECT followed by a DELETE, each in its own auto-committed transaction, allowing locks to flush between statements. The review confirmed this is portable and the loop terminates correctly.
- ✅ It works as expected from a user's perspective.
The implementation is functionally correct for the default configuration, and integration tests cover both the SELECT and DELETE paths end-to-end. Minor caveats (the under-delete window and the Postgres parameter-limit edge case for very large batch sizes) are non-blocking for typical usage.
- ✅ If this is a pre-GA feature, it is behind a feature toggle.
This is a bug fix for an existing feature, not a pre-GA feature, so no feature toggle is required. No evidence contradicts this.
- ✅ The docs are updated (and if it is a notable improvement, added to What's New).
The PR description indicates the docs checkbox is ticked. No review feedback contradicts this, and the change does not introduce a user-facing feature that would mandate documentation updates.
The dataset holds a single golden comment for this PR, and FriendlyReviewer found it — recall 100%. The golden (Low) flags the use of the
Errorlog level for what is routine cleanup progress; the candidate matches directly, adding the concrete consequences: the fullidsslice and raw SQL condition are logged on every batch, flooding the error log at the default batch size of 100, anderr=nilis logged as if it were an error. The only divergence is severity calibration: FriendlyReviewer rated the issue Medium, one notch above the dataset's Low.The strongest extra is the ticker change flagged High: the cleanup interval was moved from 10 minutes to 1 minute, which multiplies by ten the frequency of every cleanup job — not just annotations — and, combined with the existing 9-minute timeout and the unused
ServerLockServicefield, creates a real risk of overlapping cleanup runs on the same instance. The change is unrelated to the deadlock fix, which makes the finding particularly useful. The Postgres binding-parameter limit edge case (32767 placeholders for large batch sizes, with the inline-values fallback gated only on the SQLite branch) and the under-delete window documented only inline complete the set; the latter is safe in practice but should be acknowledged at the caller's contract level.The score here rests on a single match, so it is thin by construction — but the correspondence is exact, and the review's extras are specific, localizable, and legitimately actionable.