Eliminate Event_Summaries / bucket-table deadlocks (zmstats, zmaudit, Event::delete, zmDbDo retry)

[connortechnology]

Summary

Eliminates the recurring InnoDB deadlock cycle observed under load between zma's event_update_trigger path, zmstats.pl's Event_Summaries maintenance, zmaudit.pl's aggregations, and zmfilter.pl (Event::delete).

The root cause was a multi-table UPDATE Event_Summaries es JOIN Events_Hour ... pattern: MariaDB takes S-locks on the joined bucket rows and holds them to TX commit regardless of isolation level, which inverts the canonical lock order

Events[Id] -> buckets[EventId] -> Event_Summaries[MonitorId]

against the trigger writers that hold X-locks on those same bucket rows. The InnoDB deadlock log captured the exact zma vs zmstats cycle.

What changes

• zmDbDo auto-retries on errno 1213 (5 attempts, exponential backoff with jitter) when called outside an explicit transaction. Most call sites are autocommit-style and already idempotent on retry.

• Event::delete sets READ COMMITTED for the per-event delete TX, runs Stats/Event_Data/Frames/Events deletes, and retries the whole TX on 1213. Also fixes a latent commit() -> rollback() bug on the error path.

• zmstats.pl Event_Summaries maintenance is rewritten to:

  1. Prune aged rows from Events_Hour/Day/Week/Month (RC, no gap locks).
  2. Snapshot per-monitor counts/disk-space via plain SELECT (consistent read at RC, no locks).
  3. Apply one UPDATE Event_Summaries per MonitorId using the snapshotted values.

  This keeps the lock prefix (buckets -> Event_Summaries) consistent with the trigger path and never holds bucket S-locks while writing ES. Wrapped in 5-retry deadlock loop.

• zmaudit.pl receives the same snapshot-then-per-row treatment for its Event_Summaries and Storage DiskSpace recomputations, replacing five correlated/JOINed UPDATEs that had the same lock-ordering hazard.

• db/triggers.sql picks up a comment block above event_update_trigger documenting the canonical lock acquisition order so future writers don't reintroduce the cycle.

Why isolation level alone wasn't enough

SET TRANSACTION ISOLATION LEVEL READ COMMITTED drops the next-key/gap locks on plain DELETE/UPDATE range scans, but a multi-table UPDATE always takes S-locks on the JOINed rows for as long as the transaction lives. The structural fix had to be removing the JOIN, not lowering isolation.

Notes for reviewers

• No schema changes. Pure script + Perl module + retry-loop changes plus one comment block in triggers.sql.
• The zmDbDo retry is conditional on AutoCommit — it will not silently re-run statements that are part of an explicit caller transaction (those callers are responsible for their own retry, e.g. the Event::delete loop).
• Event::delete's isolation set + retry path is bypassed when the caller is already inside a transaction ($in_transaction short-circuit), so callers that wrap multiple deletes in their own TX keep their semantics.

Test plan

• Build clean (cmake --build build) — Perl scripts go through cmake configure_file.
• perl -c on the touched modules with a real install (placeholders substituted).
• Run zmstats.pl, zmaudit.pl against a busy install and confirm no Deadlock found errors in /var/log/zm/.
• Force contention by deleting a hot event via filter while zma is updating the same monitor; confirm Event::delete retries succeed and the row is removed.
• Verify Event_Summaries totals match recomputed SUM(DiskSpace)/COUNT(*) from the bucket tables after a full zmstats cycle.
• Confirm zmaudit.pl --interactive Storage DiskSpace UPDATE still produces correct totals.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Reduces recurring MariaDB/InnoDB deadlocks between event-trigger writers (zma), maintenance scripts (zmstats.pl, zmaudit.pl), and event deletion by removing join-based multi-table UPDATE patterns and adding targeted deadlock retries.

Changes:

• Add deadlock (errno 1213) retry with exponential backoff to ZoneMinder::Database::zmDbDo when running in autocommit mode.
• Rework Event::delete to run under READ COMMITTED (when it owns the transaction) and retry the whole delete transaction on deadlock.
• Rewrite zmstats.pl and zmaudit.pl Event_Summaries (and Storage DiskSpace in zmaudit.pl) maintenance to “snapshot via SELECT, then per-row UPDATE” to avoid lock-order inversions; add deadlock retry loop in zmstats.pl.
• Add documentation in db/triggers.sql describing intended lock acquisition order.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
scripts/ZoneMinder/lib/ZoneMinder/Event.pm	Adds RC isolation + deadlock retry loop around per-event delete transaction and updates trigger/locking rationale comments.
scripts/ZoneMinder/lib/ZoneMinder/Database.pm	Adds autocommit-only deadlock retry/backoff behavior to zmDbDo.
scripts/zmstats.pl.in	Rewrites bucket prune + Event_Summaries resync to snapshot aggregates then update per MonitorId inside an RC transaction with retries.
scripts/zmaudit.pl.in	Rewrites Event_Summaries and Storage DiskSpace recomputation to snapshot aggregates then update per row.
db/triggers.sql	Adds lock-order comment block near event_update_trigger definition.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[connortechnology]

Pushed two follow-up commits addressing the review feedback:

• 19c5bcbbd docs: correct Event::delete lock-order comment — fixes both review comments about the inconsistent comment vs. trigger definition. The block in Event.pm previously claimed event_update_trigger was changed to BEFORE UPDATE; that change was considered then reverted (lock order doesn't depend on trigger timing — InnoDB X-locks the matched row during WHERE evaluation before either trigger fires), but the comment never got rewritten. Now matches the chain documented in triggers.sql: Events[Id] -> buckets[EventId] -> Event_Summaries[MonitorId].
• 943114da9 fix: don't zero Event_Summaries counters on aggregate SELECT failure in zmaudit — addresses the third review comment. Now tracks per-aggregate success and only updates the column groups whose SELECT succeeded; if every aggregate fails the resync is skipped entirely with an error logged, and the next zmaudit pass corrects it.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

[connortechnology]

Pushed 0dcd500b7 addressing the new review comment: the bucket-prune SELECT now checks $dbh->err() and bails the same way the DELETE error path does. (Same hazard wasn't present in the snapshot/aggregate sections — those already check explicitly.)

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (5)

scripts/zmaudit.pl.in:1025

• The loop ignores whether a per-monitor UPDATE ultimately failed after all zmDbDo retries. If one row fails, the script continues updating later rows and prints the success message, leaving Event_Summaries only partially resynced with no clear failure at the resync level.

        ZoneMinder::Database::zmDbDo(
          $sql,
          (map { $a->{$_} // 0 } @bind_template),
          $mid
        );

scripts/zmaudit.pl.in:1050

• This Storage overwrite uses a previously snapshotted sum, so a concurrent event writer can update Storage.DiskSpace after the SELECT and then be overwritten here with the stale value. That can lose the live storage increment/decrement until a later audit pass.

          ZoneMinder::Database::zmDbDo(
            'UPDATE Storage SET DiskSpace=? WHERE Id=?',
            $disk{$sid} // 0, $sid

scripts/zmaudit.pl.in:1055

• This success message is emitted even when the aggregate SELECT or Storage enumeration failed, because it is outside the success branch. In those cases no Storage rows were updated, so the message can mislead interactive users and logs into thinking the resync completed.

    aud_print('Finished updating Storage DiskSpace');

scripts/zmaudit.pl.in:1050

• The return value from the per-storage UPDATE is ignored. If one Storage row still fails after zmDbDo's retries, the script silently continues with later rows and then reports completion, leaving DiskSpace only partially resynced.

          ZoneMinder::Database::zmDbDo(
            'UPDATE Storage SET DiskSpace=? WHERE Id=?',
            $disk{$sid} // 0, $sid

scripts/zmaudit.pl.in:1027

• This reports a full Event_Summaries resync whenever at least one aggregate group succeeded, even if other aggregate SELECTs failed and their column groups were intentionally skipped. The message should distinguish a partial resync so operators do not assume all counters were refreshed.

      aud_print('Finished resyncing Event_Summaries from Events + bucket tables');

[connortechnology]

Addressed the four new review comments:

• 628bcb3e8 fix: don't log Error on deadlock inside a caller-managed transaction — zmDbDo no longer emits the misleading Error('Failed ... Deadlock found') when called from Event::delete / zmstats prune+resync TXs; those have their own outer retry loops. Now a Debug line in that path; Error still fires for non-1213 failures and for autocommit calls that exhaust retries.
• 7b4617b1a docs: document atomicity tradeoff in zmstats/zmaudit ES resync — agreed the snapshot-then-update split is non-atomic; that's intentional. The trigger writers maintain ES incrementally between resyncs, drift is bounded by the resync interval and corrected on the next pass, and locking ES before reading aggregates would invert the canonical order and re-introduce the deadlock we just removed. Comment block on both scripts now spells this out.
• e254ed41d docs: expand triggers.sql lock-order block with full writer list — added src/zm_event.cpp's create path (event_insert_trigger is commented out below, so zmc inserts Events then bucket rows then Event_Summaries directly), the bucket triggers cascading into ES, and zmaudit.pl alongside zmstats.pl. The doc now enumerates every writer obligated to follow the canonical order, not just the Events-level triggers.

PR is now at 11 commits.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

scripts/zmaudit.pl.in:1064

• This success message is emitted even when the aggregate SELECT or Storage enumeration fails and no Storage rows were updated. Move it into the successful update path (or make it conditional) so operators do not see “Finished updating Storage DiskSpace” immediately after an error.

    aud_print('Finished updating Storage DiskSpace');

[connortechnology]

Five new comments addressed in four follow-up commits:

• `d01300832` fix: log Error in Event::delete when deadlock retries are exhausted — addresses the "silent return after 5 failed attempts" hazard. zmDbDo intentionally suppresses Error on 1213 inside a caller-managed TX, so Event::delete now logs its own Error (with errstr captured before rollback) on the exhausted-retries path.
• `1323e9f02` docs: reference the actual Event::Event constructor in lock-order block — fixed the bogus `Event::createNotification` symbol; the bucket+ES insert sequence lives in `Event::Event` at `src/zm_event.cpp:46`.
• `52d5f80e0` docs: clarify per-monitor UPDATE lock state in zmstats resync — agreed the previous comment conflated statement-level locks with TX-level locks. Rewrote it to distinguish the TX's already-held bucket and ES X-locks (acquired in canonical order by the earlier DELETEs and their cascading triggers) from the new statement-level UPDATE.
• `ccc0bcf82` fix: track per-row UPDATE failures in zmaudit ES and Storage resync — both `aud_print('Finished ...')` messages now reflect actual completion: skipped aggregate column groups and per-row `zmDbDo` failures are counted and reported instead of being masked. An Error is logged if Storage resync didn't complete.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

[connortechnology]

Five new comments addressed in three follow-up commits (the zmstats commit folds K + L together because the touched-monitors restriction fixes both at once):

• `411b6916b` fix: avoid DB-backed logging inside zmDbDo when caller manages the transaction — the most serious of the batch. `ZoneMinder::Logger->logPrint` does `INSERT INTO Logs` on the same `$dbh`, so calling `Debug()`/`Error()` from zmDbDo's failure path inside a caller-managed TX would execute another statement, clearing the err/errstr the caller needs for rollback/retry. A caller could then read err=0 after a deadlock-victim TX and `commit()` what looks like success but is actually a rolled-back no-op. Now zmDbDo bails silently when AutoCommit is off; the caller's own retry loop handles logging.
• `8fb4ad79e` perf: restrict zmstats ES resync to monitors with pruned buckets — addresses both the "full Events_Month scan every 60s" workload concern and the "X-lock every ES row every pass" contention concern. Capture MonitorIds during the prune SELECT, skip the resync entirely if nothing was pruned, and restrict the aggregate SELECTs (`WHERE MonitorId IN (...)`) and per-monitor UPDATEs to the touched set. zmaudit remains the deep-resync safety net for drift in untouched monitors. Also captures errstr before rollback so the gave-up Error reports the actual reason on drivers that clear errstr on rollback.
• `d098a055b` fix: track Event_Summaries enumerate failure as partial resync in zmaudit — without it, the enumerate failure left `@skipped` empty and the audit log still claimed a full resync while monitors with no current bucket rows never got zeroed. Now tracked alongside the per-aggregate flags and surfaced in the partial-resync message.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.