jobqueue: Use explicit retry when refreshLinks can't get a lock

While RefreshLinksJob is de-duplicated by page-id, it is possible
for two jobs to run for the same page ID if the second one was queued
after the first one started running. In that case they the newer
one must not be skipped or ignored because it will have newer
information to record to the database, but it also has no way
to stop the old one, and we can't run them concurrently.

Instead of letting the lock exception mark the job as error,
making it implicitly retry, do this more explicitly, which avoids
logspam.

Bug: T170596
Co-Authored-By: Aaron Schulz <aschulz@wikimedia.org>
Change-Id: Id2852d73d00daf83f72cf5ff778c638083f5fc73

diff --git a/includes/deferred/LinksDeletionUpdate.php b/includes/deferred/LinksDeletionUpdate.php
index f370e43..5ab83c6 100644 (file)
--- a/includes/deferred/LinksDeletionUpdate.php
+++ b/includes/deferred/LinksDeletionUpdate.php
@@ -71,6 +71,9 @@ class LinksDeletionUpdate extends DataUpdate implements EnqueueableDataUpdate {
                        // Make sure all links update threads see the changes of each other.
                        // This handles the case when updates have to batched into several COMMITs.
                        $scopedLock = LinksUpdate::acquirePageLock( $this->getDB(), $id );
+                       if ( !$scopedLock ) {
+                               throw new RuntimeException( "Could not acquire lock for page ID '{$id}'." );
+                       }
                }
 
                $title = $this->page->getTitle();

diff --git a/includes/deferred/LinksUpdate.php b/includes/deferred/LinksUpdate.php
index 5b1be6d..dbe387b 100644 (file)
--- a/includes/deferred/LinksUpdate.php
+++ b/includes/deferred/LinksUpdate.php
@@ -21,6 +21,7 @@
  */
 
 use Wikimedia\Rdbms\IDatabase;
+use MediaWiki\Logger\LoggerFactory;
 use MediaWiki\MediaWikiServices;
 use Wikimedia\ScopedCallback;
 
@@ -163,6 +164,9 @@ class LinksUpdate extends DataUpdate implements EnqueueableDataUpdate {
                        // Make sure all links update threads see the changes of each other.
                        // This handles the case when updates have to batched into several COMMITs.
                        $scopedLock = self::acquirePageLock( $this->getDB(), $this->mId );
+                       if ( !$scopedLock ) {
+                               throw new RuntimeException( "Could not acquire lock for page ID '{$this->mId}'." );
+                       }
                }
 
                // Avoid PHP 7.1 warning from passing $this by reference
@@ -190,15 +194,19 @@ class LinksUpdate extends DataUpdate implements EnqueueableDataUpdate {
         * @param IDatabase $dbw
         * @param int $pageId
         * @param string $why One of (job, atomicity)
-        * @return ScopedCallback
-        * @throws RuntimeException
+        * @return ScopedCallback|null
         * @since 1.27
         */
        public static function acquirePageLock( IDatabase $dbw, $pageId, $why = 'atomicity' ) {
                $key = "LinksUpdate:$why:pageid:$pageId";
                $scopedLock = $dbw->getScopedLockAndFlush( $key, __METHOD__, 15 );
                if ( !$scopedLock ) {
-                       throw new RuntimeException( "Could not acquire lock '$key'." );
+                       $logger = LoggerFactory::getInstance( 'SecondaryDataUpdate' );
+                       $logger->info( "Could not acquire lock '{key}' for page ID '{page_id}'.", [
+                               'key' => $key,
+                               'page_id' => $pageId,
+                       ] );
+                       return null;
                }
 
                return $scopedLock;

diff --git a/includes/jobqueue/jobs/DeleteLinksJob.php b/includes/jobqueue/jobs/DeleteLinksJob.php
index 0a14192..8328abf 100644 (file)
--- a/includes/jobqueue/jobs/DeleteLinksJob.php
+++ b/includes/jobqueue/jobs/DeleteLinksJob.php
@@ -47,6 +47,10 @@ class DeleteLinksJob extends Job {
 
                // Serialize links updates by page ID so they see each others' changes
                $scopedLock = LinksUpdate::acquirePageLock( wfGetDB( DB_MASTER ), $pageId, 'job' );
+               if ( $scopedLock === null ) {
+                       $this->setLastError( 'LinksUpdate already running for this page, try again later.' );
+                       return false;
+               }
 
                if ( WikiPage::newFromID( $pageId, WikiPage::READ_LATEST ) ) {
                        // The page was restored somehow or something went wrong

diff --git a/includes/jobqueue/jobs/RefreshLinksJob.php b/includes/jobqueue/jobs/RefreshLinksJob.php
index 8854c65..aa8e121 100644 (file)
--- a/includes/jobqueue/jobs/RefreshLinksJob.php
+++ b/includes/jobqueue/jobs/RefreshLinksJob.php
@@ -146,6 +146,11 @@ class RefreshLinksJob extends Job {
                $dbw = $lbFactory->getMainLB()->getConnection( DB_MASTER );
                /** @noinspection PhpUnusedLocalVariableInspection */
                $scopedLock = LinksUpdate::acquirePageLock( $dbw, $page->getId(), 'job' );
+               if ( $scopedLock === null ) {
+                       // Another job is already updating the page, likely for an older revision (T170596).
+                       $this->setLastError( 'LinksUpdate already running for this page, try again later.' );
+                       return false;
+               }
                // Get the latest ID *after* acquirePageLock() flushed the transaction.
                // This is used to detect edits/moves after loadPageData() but before the scope lock.
                // The works around the chicken/egg problem of determining the scope lock key.