[chippison]

DEV-20866

Follow-up to matomo-org/matomo#25549, which makes the tracker connection refuse every query and the commit after an error that rolled back the transaction.

If a tracking step caught such an error on the last query of a request set, nothing noticed until the lock or commit queries, which then failed outside the retry path. The worker crashed, and the unlock in the error handler failed too, so the lock stayed held until it expired.

• After each tracked request the handler checks whether the transaction was rolled back, so the set goes down the existing retry path.
• When processing fails, the processor rolls back before it releases the lock.
• The retry check uses core's TransactionErrors when it exists.

Both checks are skipped on Matomo versions without the core change. The new HandlerTest case needs the core PR, and is skipped without it.

Review

• [✔] I have understood, reviewed, and tested all AI outputs before use
• [✔] All AI instructions respect security, IP, and privacy rules

[snake14]

The fix works: a deadlock swallowed on the last request of a batch is now retried, the lock is released, and every request is stored once, with both PDO and MYSQLI. A few things to check:

Medium

The rollback added to Processor::process()'s catch block has no test, so removing or reordering it before unlock() would leave the suite green. A ProcessorTest case that makes a backend query fail on an aborted transaction and asserts the lock is released would cover it.

Low

1. failIfTransactionAborted() always says "MySQL error 1213", even when the swallowed error was another one that ends the transaction, and that message goes to the warning log. A neutral message ("a caught error already rolled back this transaction") with the same code would avoid misleading triage.
2. The docblocks of hasTransactionErrors(), getRequestSetsWithTransactionErrors() and isRetryableTransactionError() still describe the old set of errors.

[chippison]

@snake14
Thanks for the quick review.
Fixed all 3 issues, then I also found another low issue: a request that swallowed a deadlock and was then skipped (invalid request or unknown site) didn't run the new check, so the set counted as processed. This now gets retried like other cases

Should be good to be re-reviewed now

[snake14]

Thanks for the changes @chippison . It looks like we missed the automated release for this week. Can you please merge in the base and bump the plugin.json/changelog version?

[snake14]

AI also mentioned few things to check. One is the version bump which I requested:

Medium

1. 6.0.3 was released today, but the CHANGELOG.md entry is still under 6.0.3 and plugin.json wasn't bumped. It merges cleanly, so the fix would be listed under the shipped version. It needs a 6.0.4 section and version bump.
2. The \Throwable catch in Handler::process() doesn't call failIfTransactionAborted() like the other two catches. When a step swallows a deadlock and then throws anything else, with the MySQL backend sharing the tracker connection, the error still escapes Processor::process() and the rest of that run's queues are skipped; on Redis the same case just carries on. Calling it at the top of that catch, after logging, would send the set down the retry path in both cases.

Low

1. In failIfTransactionAborted(), "the code sends the set down the retry path" reads as "the program". Something like "1213 sends the set down the retry path" would be clearer.

[chippison]

@snake14
Nice catch on the second issue!
Ive added the fix for the 3 issues found.
Ready to be re-reviewed again
Thanks

[snake14]

Works as expected. All review points have been addressed. Thank you. 👍