From a1a19408c21477cc6e2f71d0fa7f6749412b9316 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Fri, 19 Aug 2016 13:12:27 -0700
Subject: [PATCH] Detect when callers catch DB errors and fail to rollback

This makes it harder to accidently circumvent the MWExceptionHandler
rollback logic.

Change-Id: Ia1f89fa0f88ff3aacf5d9b93300dbf909fa74fdd
---
 includes/db/DBConnRef.php                 | 4 ++++
 includes/db/Database.php                  | 5 +----
 includes/db/IDatabase.php                 | 6 ++++++
 includes/db/loadbalancer/LoadBalancer.php | 9 +++++++++
 4 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/includes/db/DBConnRef.php b/includes/db/DBConnRef.php
index 4a7836355a..e5b6d050d8 100644
--- a/includes/db/DBConnRef.php
+++ b/includes/db/DBConnRef.php
@@ -55,6 +55,10 @@ class DBConnRef implements IDatabase {
 		return $this->__call( __FUNCTION__, func_get_args() );
 	}
 
+	public function explicitTrxActive() {
+		return $this->__call( __FUNCTION__, func_get_args() );
+	}
+
 	public function tablePrefix( $prefix = null ) {
 		return $this->__call( __FUNCTION__, func_get_args() );
 	}
diff --git a/includes/db/Database.php b/includes/db/Database.php
index 933bea60a9..528a2985fe 100644
--- a/includes/db/Database.php
+++ b/includes/db/Database.php
@@ -2826,10 +2826,7 @@ abstract class DatabaseBase implements IDatabase {
 		}
 	}
 
-	/**
-	 * @return bool
-	 */
-	protected function explicitTrxActive() {
+	public function explicitTrxActive() {
 		return $this->mTrxLevel && ( $this->mTrxAtomicLevels || !$this->mTrxAutomatic );
 	}
 
diff --git a/includes/db/IDatabase.php b/includes/db/IDatabase.php
index bdab09e7b5..25100db0dc 100644
--- a/includes/db/IDatabase.php
+++ b/includes/db/IDatabase.php
@@ -103,6 +103,12 @@ interface IDatabase {
 	 */
 	public function trxTimestamp();
 
+	/**
+	 * @return bool Whether an explicit transaction or atomic sections are still open
+	 * @since 1.28
+	 */
+	public function explicitTrxActive();
+
 	/**
 	 * Get/set the table prefix.
 	 * @param string $prefix The table prefix to set, or omitted to leave it unchanged.
diff --git a/includes/db/loadbalancer/LoadBalancer.php b/includes/db/loadbalancer/LoadBalancer.php
index d08172a5e8..93ec0373e0 100644
--- a/includes/db/loadbalancer/LoadBalancer.php
+++ b/includes/db/loadbalancer/LoadBalancer.php
@@ -1090,6 +1090,15 @@ class LoadBalancer {
 	public function approveMasterChanges( array $options ) {
 		$limit = isset( $options['maxWriteDuration'] ) ? $options['maxWriteDuration'] : 0;
 		$this->forEachOpenMasterConnection( function ( DatabaseBase $conn ) use ( $limit ) {
+			// If atomic section or explicit transactions are still open, some caller must have
+			// caught an exception but failed to properly rollback any changes. Detect that and
+			// throw and error (causing rollback).
+			if ( $conn->explicitTrxActive() ) {
+				throw new DBTransactionError(
+					$conn,
+					"Explicit transaction still active. A caller may have caught an error."
+				);
+			}
 			// Assert that the time to replicate the transaction will be sane.
 			// If this fails, then all DB transactions will be rollback back together.
 			$time = $conn->pendingWriteQueryDuration();
-- 
2.20.1