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

@@ -471,13 +471,16 @@ public function __construct($course, $userid) {
 
         // 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 {

lib/tests/modinfolib_test.php

@@ -1126,4 +1126,49 @@ public function test_invalid_course_module_id(): void {
         $this->expectExceptionMessage('Invalid course module id: ' . $forum1->cmid);
         delete_course($course, false);
     }
+
+    /**
+     * 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'));
+    }
 }