From dc10216e77b06b0fc7bb5e0824eb0967129c2e1f 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: If9d8f4ef47258d5afb7a0e947c6e6f040cbc7e10
---
 RELEASE-NOTES-1.27                        |  2 ++
 includes/DefaultSettings.php              |  7 ++++
 includes/api/ApiMain.php                  | 39 ++++++++++++++++++++---
 includes/db/loadbalancer/LoadBalancer.php |  6 ++--
 4 files changed, 48 insertions(+), 6 deletions(-)

diff --git a/RELEASE-NOTES-1.27 b/RELEASE-NOTES-1.27
index f8293b975b..f3f0704d25 100644
--- a/RELEASE-NOTES-1.27
+++ b/RELEASE-NOTES-1.27
@@ -63,6 +63,8 @@ production.
   setting in the relevant section of $wgLBFactoryConf.
 * User::newSystemUser() may be used to simplify the creation of passwordless
   "system" users for logged actions from scripts and extensions.
+* $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 04f3f31789..310b241bc1 100644
--- a/includes/DefaultSettings.php
+++ b/includes/DefaultSettings.php
@@ -7323,6 +7323,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 c641c9513c..f60f5df6e9 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,6 +1147,7 @@ class ApiMain extends ApiBase {
 		) {
 			$this->dieUsageMsg( 'readrequired' );
 		}
+
 		if ( $module->isWriteMode() ) {
 			if ( !$this->mEnableWrite ) {
 				$this->dieUsageMsg( 'writedisabled' );
@@ -1155,9 +1155,7 @@ class ApiMain extends ApiBase {
 			if ( !$user->isAllowed( 'writeapi' ) ) {
 				$this->dieUsageMsg( 'writerequired' );
 			}
-			if ( wfReadOnly() ) {
-				$this->dieReadOnly();
-			}
+			$this->checkReadOnly( $module );
 		}
 
 		// Allow extensions to stop execution for arbitrary reasons.
@@ -1167,6 +1165,39 @@ 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() && $this->getUser()->isAllowed( 'bot' ) ) {
+			// 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