Prevent WAN cache invalidation from purging concurrent refreshes (#9)

* Fix WAN cache invalidation race

* Correct WAN cache concurrency tests

* Bump version from 2.0.0 to 2.0.1

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: Jeffrey Wang <jeffw16@users.noreply.github.com>

Author: Copilot

README.md

@@ -73,7 +73,7 @@ of truth.
 Definitions are memoized within the request and cached through
 `WANObjectCache`, including process caching, hot refresh, stampede protection,
 and a cross-server check key. Saves replace the complete definition set in one
-database transaction and invalidate both the check key and cached value.
+database transaction and invalidate the check key.
 Primary-database reads are used when regenerating the cache because replica lag
 must not temporarily change title interpretation.
 

extension.json

@@ -1,6 +1,6 @@
 {
 	"name": "NamespaceManager",
-	"version": "2.0.0",
+	"version": "2.0.1",
 	"author": [
 		"[https://github.com/mywikis MyWikis LLC]"
 	],

src/NamespaceRepository.php

@@ -113,7 +113,6 @@ public function isEmpty(): bool {
 
 	public function invalidate(): void {
 		$this->cache->touchCheckKey( $this->getCheckKey() );
-		$this->cache->delete( $this->getCacheKey() );
 		$this->loaded = false;
 		$this->memoizedDefinitions = [];
 	}

tests/phpunit/integration/NamespaceRepositoryTest.php

@@ -4,6 +4,10 @@
 
 use MediaWiki\Extension\NamespaceManager\NamespaceRepository;
 use MediaWikiIntegrationTestCase;
+use Psr\Log\NullLogger;
+use Wikimedia\ObjectCache\HashBagOStuff;
+use Wikimedia\ObjectCache\WANObjectCache;
+use Wikimedia\Rdbms\IConnectionProvider;
 use Wikimedia\Rdbms\IDatabase;
 
 /**
@@ -66,6 +70,105 @@ public function testReplaceWithEmptyListDeletesAllRows(): void {
 		$this->assertTrue( $this->repository->isEmpty() );
 	}
 
+	public function testInvalidationRejectsConcurrentStaleCacheFill(): void {
+		$cache = new WANObjectCache( [ 'cache' => new HashBagOStuff() ] );
+		$mockTime = 1_700_000_000.0;
+		$cache->setMockTime( $mockTime );
+		$repository = $this->newCacheOnlyRepository( $cache );
+		$key = $cache->makeKey( 'namespacemanager', 'definitions' );
+		$checkKey = $cache->makeKey( 'namespacemanager', 'definitions', 'check' );
+		$options = $this->getCacheOptions( $checkKey );
+
+		$stale = $cache->getWithSetCallback(
+			$key,
+			WANObjectCache::TTL_WEEK,
+			static function () use ( $repository ): string {
+				$repository->invalidate();
+				return 'stale';
+			},
+			$options
+		);
+		$cache->setMockTime( $mockTime + 20 );
+		$regenerations = 0;
+		$fresh = $cache->getWithSetCallback(
+			$key,
+			WANObjectCache::TTL_WEEK,
+			static function () use ( &$regenerations ): string {
+				$regenerations++;
+				return 'fresh';
+			},
+			$options
+		);
+
+		$this->assertSame( 'stale', $stale );
+		$this->assertSame( 'fresh', $fresh );
+		$this->assertSame( 1, $regenerations );
+	}
+
+	public function testInvalidationDoesNotPurgeConcurrentFreshCacheFill(): void {
+		$cacheBag = new CallbackHashBagOStuff();
+		$writerCache = new WANObjectCache( [ 'cache' => $cacheBag ] );
+		$readerCache = new WANObjectCache( [ 'cache' => $cacheBag ] );
+		$repository = $this->newCacheOnlyRepository( $writerCache );
+		$key = $readerCache->makeKey( 'namespacemanager', 'definitions' );
+		$checkKey = $readerCache->makeKey( 'namespacemanager', 'definitions', 'check' );
+		$options = $this->getCacheOptions( $checkKey );
+		$regenerations = 0;
+
+		$cacheBag->afterNextSet(
+			static function () use (
+				$readerCache,
+				$key,
+				$options,
+				&$regenerations
+			): void {
+				$readerCache->getWithSetCallback(
+					$key,
+					WANObjectCache::TTL_WEEK,
+					static function () use ( &$regenerations ): string {
+						$regenerations++;
+						return 'fresh';
+					},
+					$options
+				);
+			}
+		);
+
+		$repository->invalidate();
+		$value = $readerCache->getWithSetCallback(
+			$key,
+			WANObjectCache::TTL_WEEK,
+			static function () use ( &$regenerations ): string {
+				$regenerations++;
+				return 'unexpected';
+			},
+			$options
+		);
+
+		$this->assertSame( 'fresh', $value );
+		$this->assertSame( 1, $regenerations );
+	}
+
+	/**
+	 * @return array<string,mixed>
+	 */
+	private function getCacheOptions( string $checkKey ): array {
+		return [
+			'checkKeys' => [ $checkKey ],
+			'hotTTR' => WANObjectCache::TTL_HOUR,
+			'lockTSE' => 30,
+			'version' => 1,
+		];
+	}
+
+	private function newCacheOnlyRepository( WANObjectCache $cache ): NamespaceRepository {
+		return new NamespaceRepository(
+			$this->createMock( IConnectionProvider::class ),
+			$cache,
+			new NullLogger()
+		);
+	}
+
 	/**
 	 * @return array<int,array<string,mixed>>
 	 */
@@ -90,3 +193,30 @@ private function getDefinitions(): array {
 		];
 	}
 }
+
+class CallbackHashBagOStuff extends HashBagOStuff {
+
+	/** @var callable|null */
+	private $afterNextSet;
+
+	public function afterNextSet( callable $callback ): void {
+		$this->afterNextSet = $callback;
+	}
+
+	/**
+	 * @param string $key
+	 * @param mixed $value
+	 * @param int $exptime
+	 * @param int $flags
+	 * @return bool
+	 */
+	protected function doSet( $key, $value, $exptime = 0, $flags = 0 ) {
+		$result = parent::doSet( $key, $value, $exptime, $flags );
+		if ( $this->afterNextSet !== null ) {
+			$callback = $this->afterNextSet;
+			$this->afterNextSet = null;
+			$callback();
+		}
+		return $result;
+	}
+}