From a7d47d7813fde053b0e2ad8bfc03cf3bec7705ac Mon Sep 17 00:00:00 2001
From: sam marshall <s.marshall@open.ac.uk>
Date: Tue, 13 Sep 2022 14:18:04 +0100
Subject: [PATCH] MDL-75736 lib: Course cache can be rebuilt unnecessarily

This change puts back the behaviour that get_fast_modinfo will accept
a cached version of modinfo that is newer than expected (according to
the course cacherev).

Not accepting newer versions can lead to poor performance in cases
where multiple requests take place at once, or using a clustered
database.
---
 lib/modinfolib.php            |  7 ++++--
 lib/tests/modinfolib_test.php | 45 +++++++++++++++++++++++++++++++++++
 2 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/lib/modinfolib.php b/lib/modinfolib.php
index f84c0fcbfac..2d43af5b149 100644
--- a/lib/modinfolib.php
+++ b/lib/modinfolib.php
@@ -476,13 +476,16 @@ class course_modinfo {
 
         // Retrieve modinfo from cache. If not present or cacherev mismatches, call rebuild and retrieve again.
         $coursemodinfo = $cachecoursemodinfo->get_versioned($course->id, $course->cacherev);
-        if ($coursemodinfo === false || ($course->cacherev != $coursemodinfo->cacherev)) {
+        // Note the version comparison using the data in the cache should not be necessary, but the
+        // partial rebuild logic sometimes sets the $coursemodinfo->cacherev to -1 which is an
+        // indicator that it needs rebuilding.
+        if ($coursemodinfo === false || ($course->cacherev > $coursemodinfo->cacherev)) {
             $lock = self::get_course_cache_lock($course->id);
             try {
                 // Only actually do the build if it's still needed after getting the lock (not if
                 // somebody else, who might have been holding the lock, built it already).
                 $coursemodinfo = $cachecoursemodinfo->get_versioned($course->id, $course->cacherev);
-                if ($coursemodinfo === false || ($course->cacherev != $coursemodinfo->cacherev)) {
+                if ($coursemodinfo === false || ($course->cacherev > $coursemodinfo->cacherev)) {
                     $coursemodinfo = self::inner_build_course_cache($course, $lock);
                 }
             } finally {
diff --git a/lib/tests/modinfolib_test.php b/lib/tests/modinfolib_test.php
index da19910af82..103ef607a95 100644
--- a/lib/tests/modinfolib_test.php
+++ b/lib/tests/modinfolib_test.php
@@ -1096,4 +1096,49 @@ class modinfolib_test extends advanced_testcase {
         // Make sure that the cacherev will be reset.
         $this->assertEquals(-1, $coursemodinfo->cacherev);
     }
+
+    /**
+     * Tests that if the modinfo cache returns a newer-than-expected version, Moodle won't rebuild
+     * it.
+     *
+     * This is important to avoid wasted time/effort and poor performance, for example in cases
+     * where multiple requests are accessing the course.
+     *
+     * Certain cases could be particularly bad if this test fails. For example, if using clustered
+     * databases where there is a 100ms delay between updates to the course table being available
+     * to all users (but no such delay on the cache infrastructure), then during that 100ms, every
+     * request that calls get_fast_modinfo and uses the read-only database will rebuild the course
+     * cache. Since these will then create a still-newer version, future requests for the next
+     * 100ms will also rebuild it again... etc.
+     *
+     * @covers \course_modinfo
+     */
+    public function test_get_modinfo_with_newer_version(): void {
+        global $DB;
+
+        $this->resetAfterTest();
+
+        // Get info about a course and build the initial cache, then drop it from memory.
+        $course = $this->getDataGenerator()->create_course();
+        get_fast_modinfo($course);
+        get_fast_modinfo(0, 0, true);
+
+        // User A starts a request, which takes some time...
+        $useracourse = $DB->get_record('course', ['id' => $course->id]);
+
+        // User B also starts a request and makes a change to the course.
+        $userbcourse = $DB->get_record('course', ['id' => $course->id]);
+        $this->getDataGenerator()->create_module('page', ['course' => $course->id]);
+        rebuild_course_cache($userbcourse->id, false);
+
+        // Finally, user A's request now gets modinfo. It should accept the version from B even
+        // though the course version (of cache) is newer than the one expected by A.
+        $before = $DB->perf_get_queries();
+        $modinfo = get_fast_modinfo($useracourse);
+        $after = $DB->perf_get_queries();
+        $this->assertEquals($after, $before, 'Should use cached version, making no DB queries');
+
+        // Obviously, modinfo should include the Page now.
+        $this->assertCount(1, $modinfo->get_instances_of('page'));
+    }
 }