Avoid unnecessary WaitConditionLoop delays in ChronologyProtector

Since it takes time for the agent to get the response and set the
cookie and, as well, the time into a request that a LoadBalancer is
initialized varies by many seconds (cookies loaded from the start),
give the cookie a much lower TTL than the DB positions in the stash.

This avoids having to wait for a position with a given cpPosIndex
value, when the position already expired from the store, which is
a waste of time.

Also include the timestamp in "cpPosIndex" cookies to implement
logical expiration in case clients do not expire them correctly.

Bug: T194403
Bug: T190082
Change-Id: I97d8f108dec59c5ccead66432a097cda8ef4a178

includes/MediaWiki.php

@@ -636,9 +636,11 @@ class MediaWiki {
 
 		if ( $cpIndex > 0 ) {
 			if ( $allowHeaders ) {
-				$expires = time() + ChronologyProtector::POSITION_TTL;
+				$now = time();
+				$expires = $now + ChronologyProtector::POSITION_COOKIE_TTL;
 				$options = [ 'prefix' => '' ];
-				$request->response()->setCookie( 'cpPosIndex', $cpIndex, $expires, $options );
+				$value = LBFactory::makeCookieValueFromCPIndex( $cpIndex, $now ); // T190082
+				$request->response()->setCookie( 'cpPosIndex', $value, $expires, $options );
 			}
 
 			if ( $strategy === 'cookie+url' ) {

includes/Setup.php

@@ -24,6 +24,8 @@
  * @file
  */
 use MediaWiki\MediaWikiServices;
+use Wikimedia\Rdbms\LBFactory;
+use Wikimedia\Rdbms\ChronologyProtector;
 
 /**
  * This file is not a valid entry point, perform no further processing unless
@@ -738,9 +740,15 @@ MediaWikiServices::getInstance()->getDBLoadBalancerFactory()->setRequestInfo( [
 	'IPAddress' => $wgRequest->getIP(),
 	'UserAgent' => $wgRequest->getHeader( 'User-Agent' ),
 	'ChronologyProtection' => $wgRequest->getHeader( 'ChronologyProtection' ),
-	// The cpPosIndex cookie has no prefix and is set by MediaWiki::preOutputCommit()
-	'ChronologyPositionIndex' =>
-		$wgRequest->getInt( 'cpPosIndex', (int)$wgRequest->getCookie( 'cpPosIndex', '' ) )
+	'ChronologyPositionIndex' => $wgRequest->getInt(
+		'cpPosIndex',
+		LBFactory::getCPIndexFromCookieValue(
+			// The cookie has no prefix and is set by MediaWiki::preOutputCommit()
+			$wgRequest->getCookie( 'cpPosIndex', '' ),
+			// Mitigate broken client-side cookie expiration handling (T190082)
+			time() - ChronologyProtector::POSITION_COOKIE_TTL
+		)
+	)
 ] );
 // Make sure that object caching does not undermine the ChronologyProtector improvements
 if ( $wgRequest->getCookie( 'UseDC', '' ) === 'master' ) {

includes/libs/rdbms/ChronologyProtector.php

@@ -63,6 +63,8 @@ class ChronologyProtector implements LoggerAwareInterface {
 
 	/** @var int Seconds to store positions */
 	const POSITION_TTL = 60;
+	/** @var int Seconds to store position write index cookies (safely less than POSITION_TTL) */
+	const POSITION_COOKIE_TTL = 60;
 	/** @var int Max time to wait for positions to appear */
 	const POS_STORE_WAIT_TIMEOUT = 5;
 

includes/libs/rdbms/lbfactory/ILBFactory.php

@@ -321,10 +321,10 @@ interface ILBFactory {
 	 * Note that unlike cookies, this works accross domains
 	 *
 	 * @param string $url
-	 * @param float $time UNIX timestamp just before shutdown() was called
+	 * @param int $index Write counter index
 	 * @return string
 	 */
-	public function appendShutdownCPIndexAsQuery( $url, $time );
+	public function appendShutdownCPIndexAsQuery( $url, $index );
 
 	/**
 	 * @param array $info Map of fields, including:

includes/libs/rdbms/lbfactory/LBFactory.php

@@ -640,6 +640,36 @@ abstract class LBFactory implements ILBFactory {
 		return strpos( $url, '?' ) === false ? "$url?cpPosIndex=$index" : "$url&cpPosIndex=$index";
 	}
 
+	/**
+	 * @param int $index Write index
+	 * @param int $time UNIX timestamp
+	 * @return string Timestamp-qualified write index of the form "<index>.<timestamp>"
+	 * @since 1.32
+	 */
+	public static function makeCookieValueFromCPIndex( $index, $time ) {
+		return $index . '@' . $time;
+	}
+
+	/**
+	 * @param string $value String possibly of the form "<index>" or "<index>@<timestamp>"
+	 * @param int $minTimestamp Lowest UNIX timestamp of non-expired values (if present)
+	 * @return int|null Write index or null if $value is empty or expired
+	 * @since 1.32
+	 */
+	public static function getCPIndexFromCookieValue( $value, $minTimestamp ) {
+		if ( !preg_match( '/^(\d+)(?:@(\d+))?$/', $value, $m ) ) {
+			return null;
+		}
+
+		$index = (int)$m[1];
+
+		if ( isset( $m[2] ) && $m[2] !== '' && (int)$m[2] < $minTimestamp ) {
+			return null; // expired
+		}
+
+		return ( $index > 0 ) ? $index : null;
+	}
+
 	public function setRequestInfo( array $info ) {
 		if ( $this->chronProt ) {
 			throw new LogicException( 'ChronologyProtector already initialized.' );

tests/phpunit/includes/db/LBFactoryTest.php

@@ -606,4 +606,38 @@ class LBFactoryTest extends MediaWikiTestCase {
 			return $db->addIdentifierQuotes( $table );
 		}
 	}
+
+	/**
+	 * @covers \Wikimedia\Rdbms\LBFactory::makeCookieValueFromCPIndex()
+	 * @covers \Wikimedia\Rdbms\LBFactory::getCPIndexFromCookieValue()
+	 */
+	public function testCPPosIndexCookieValues() {
+		$this->assertEquals( '3@542', LBFactory::makeCookieValueFromCPIndex( 3, 542 ) );
+
+		$time = 1526522031;
+		$this->assertSame(
+			5,
+			LBFactory::getCPIndexFromCookieValue( "5", $time - 10 )
+		);
+		$this->assertSame(
+			null,
+			LBFactory::getCPIndexFromCookieValue( "0", $time - 10 )
+		);
+		$this->assertSame(
+			2,
+			LBFactory::getCPIndexFromCookieValue( "2@$time", $time - 10 )
+		);
+		$this->assertSame(
+			2,
+			LBFactory::getCPIndexFromCookieValue( "2@$time", $time + 9 - 10 )
+		);
+		$this->assertSame(
+			null,
+			LBFactory::getCPIndexFromCookieValue( "0@$time", $time + 9 - 10 )
+		);
+		$this->assertSame(
+			null,
+			LBFactory::getCPIndexFromCookieValue( "2@$time", $time + 11 - 10 )
+		);
+	}
 }