From 453d88605b1612c07fe838d0c21d54e1a871c702 Mon Sep 17 00:00:00 2001
From: Aaron Schulz <aschulz@wikimedia.org>
Date: Thu, 1 Oct 2015 17:57:07 -0700
Subject: [PATCH] Enforce stricter slave lag limits for bot API requests

* All users with the bot right sending requests to
  write APIs will get a read-only error when slave
  lag is high. The lag tolerance is configured via
  $wgAPIMaxLagThreshold.
* Making this lower than 'max lag' in the LBFactory
  config means bots get read-only errors before other
  users, which is sensible.
* The default LoadBalancer 'max lag' is now 10 instead
  of 30, which now matches the timeout used for waiting
  on MASTER_POS_WAIT() for ChronologyProtector. This is
  better for assuring that users see their own changes.

Bug: T95501
Change-Id: I56a7f3538278378c67fa3ef5ff0ba4aa6a9a8c64
---
 RELEASE-NOTES-1.27                        |  2 +
 includes/DefaultSettings.php              |  7 ++++
 includes/api/ApiMain.php                  | 45 ++++++++++++++++++++---
 includes/db/loadbalancer/LoadBalancer.php |  6 ++-
 4 files changed, 53 insertions(+), 7 deletions(-)

diff --git a/RELEASE-NOTES-1.27 b/RELEASE-NOTES-1.27
index 13b9ba26de..2701593ecf 100644
--- a/RELEASE-NOTES-1.27
+++ b/RELEASE-NOTES-1.27
@@ -67,6 +67,8 @@ production.
 * Extensions can now return detailed error information via the API when
   preventing user actions using 'getUserPermissionsErrors' and similar hooks
   by using ApiMessage instances instead of strings for the $result value.
+* $wgAPIMaxLagThreshold was added to limit bot changes when databases lag
+  becomes too high.
 
 ==== External libraries ====
 
diff --git a/includes/DefaultSettings.php b/includes/DefaultSettings.php
index 6032ba3bbc..bf6e245d72 100644
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -7325,6 +7325,13 @@ $wgAPIMaxResultSize = 8388608;
  */
 $wgAPIMaxUncachedDiffs = 1;
 
+/**
+ * Maximum amount of DB lag on a majority of DB slaves to tolerate
+ * before forcing bots to retry any write requests via API errors.
+ * This should be lower than the 'max lag' value in $wgLBFactoryConf.
+ */
+$wgAPIMaxLagThreshold = 7;
+
 /**
  * Log file or URL (TCP or UDP) to log API requests to, or false to disable
  * API request logging
diff --git a/includes/api/ApiMain.php b/includes/api/ApiMain.php
index 86a8a87a6b..e4ab7a5b38 100644
--- a/includes/api/ApiMain.php
+++ b/includes/api/ApiMain.php
@@ -1002,7 +1002,6 @@ class ApiMain extends ApiBase {
 	 */
 	protected function checkMaxLag( $module, $params ) {
 		if ( $module->shouldCheckMaxlag() && isset( $params['maxlag'] ) ) {
-			// Check for maxlag
 			$maxLag = $params['maxlag'];
 			list( $host, $lag ) = wfGetLB()->getMaxLag();
 			if ( $lag > $maxLag ) {
@@ -1148,20 +1147,20 @@ class ApiMain extends ApiBase {
 		) {
 			$this->dieUsageMsg( 'readrequired' );
 		}
+
 		if ( $module->isWriteMode() ) {
 			if ( !$this->mEnableWrite ) {
 				$this->dieUsageMsg( 'writedisabled' );
 			} elseif ( !$user->isAllowed( 'writeapi' ) ) {
 				$this->dieUsageMsg( 'writerequired' );
-			} elseif ( wfReadOnly() ) {
-				$this->dieReadOnly();
-			}
-			if ( $this->getRequest()->getHeader( 'Promise-Non-Write-API-Action' ) ) {
+			} elseif ( $this->getRequest()->getHeader( 'Promise-Non-Write-API-Action' ) ) {
 				$this->dieUsage(
 					"Promise-Non-Write-API-Action HTTP header cannot be sent to write API modules",
 					'promised-nonwrite-api'
 				);
 			}
+
+			$this->checkReadOnly( $module );
 		}
 
 		// Allow extensions to stop execution for arbitrary reasons.
@@ -1171,6 +1170,42 @@ class ApiMain extends ApiBase {
 		}
 	}
 
+	/**
+	 * Check if the DB is read-only for this user
+	 * @param ApiBase $module An Api module
+	 */
+	protected function checkReadOnly( $module ) {
+		if ( wfReadOnly() ) {
+			$this->dieReadOnly();
+		}
+
+		if ( $module->isWriteMode()
+			&& in_array( 'bot', $this->getUser()->getGroups() )
+			&& wfGetLB()->getServerCount() > 1
+		) {
+			// Figure out how many servers have passed the lag threshold
+			$numLagged = 0;
+			$lagLimit = $this->getConfig()->get( 'APIMaxLagThreshold' );
+			foreach ( wfGetLB()->getLagTimes() as $lag ) {
+				if ( $lag > $lagLimit ) {
+					++$numLagged;
+				}
+			}
+			// If a majority of slaves are too lagged then disallow writes
+			$slaveCount = wfGetLB()->getServerCount() - 1;
+			if ( $numLagged >= ceil( $slaveCount / 2 ) ) {
+				$parsed = $this->parseMsg( array( 'readonlytext' ) );
+				$this->dieUsage(
+					$parsed['info'],
+					$parsed['code'],
+					/* http error */
+					0,
+					array( 'readonlyreason' => "Waiting for $numLagged lagged database(s)" )
+				);
+			}
+		}
+	}
+
 	/**
 	 * Check asserts of the user's rights
 	 * @param array $params
diff --git a/includes/db/loadbalancer/LoadBalancer.php b/includes/db/loadbalancer/LoadBalancer.php
index bc9465ba60..a5a5c37d2b 100644
--- a/includes/db/loadbalancer/LoadBalancer.php
+++ b/includes/db/loadbalancer/LoadBalancer.php
@@ -68,7 +68,9 @@ class LoadBalancer {
 	/** @var integer Warn when this many connection are held */
 	const CONN_HELD_WARN_THRESHOLD = 10;
 	/** @var integer Default 'max lag' when unspecified */
-	const MAX_LAG = 30;
+	const MAX_LAG = 10;
+	/** @var integer Max time to wait for a slave to catch up (e.g. ChronologyProtector) */
+	const POS_WAIT_TIMEOUT = 10;
 
 	/**
 	 * @param array $params Array with keys:
@@ -82,7 +84,7 @@ class LoadBalancer {
 			throw new MWException( __CLASS__ . ': missing servers parameter' );
 		}
 		$this->mServers = $params['servers'];
-		$this->mWaitTimeout = 10;
+		$this->mWaitTimeout = self::POS_WAIT_TIMEOUT;
 
 		$this->mReadIndex = -1;
 		$this->mWriteIndex = -1;
-- 
2.20.1