New handful of errors in prod:

class@anonymous /srv/mediawiki/php-1.35.0-wmf.20/extensions/Wikibase/lib/includes/SimpleCacheWithBagOStuff.php0x7f9378c5e02f    wmf.20 e/W/l/i/SimpleCacheWithBagOStuff.php:271  Cache key contains characters that are not allowed: `Q177856_1116973465_⧼lang⧽_label`

Is this caused by running in uselang=qqx, perhaps?

We are moving this to blocked because we need to fix T237164 first. We are going to prioritize that one.

In T245396#5912675, @Jdforrester-WMF wrote:

New handful of errors in prod:

class@anonymous /srv/mediawiki/php-1.35.0-wmf.20/extensions/Wikibase/lib/includes/SimpleCacheWithBagOStuff.php0x7f9378c5e02f    wmf.20 e/W/l/i/SimpleCacheWithBagOStuff.php:271  Cache key contains characters that are not allowed: `Q177856_1116973465_⧼lang⧽_label`

Is this caused by running in uselang=qqx, perhaps?

While resolving this ticket will suppress these errors we also want to track down why they are happening. I opened a new ticket to track this: T246207

Seen again in prod: T247466: SimpleCacheWithBagOStuff: Cache key contains characters that are not allowed

Our BagOStuff class allows any character to be used in a cache key, no problem. There should be absolutely no need to make the caller accomodate itself to the characters required by some random backend implementation. That is now how BagOStuff is meant to work.

It just needs to be encoded by the cache service instead of the caller. This happens automaticaly when you call makeKey() on the cache object - the same object that will store the value.

The problem is that Wikibase's SimpleCacheWithBagOStuff, unlike all other BagOStuff proxy classes, is not proxying the makeKey methods correctly.

@Tarrow I'm looking at the SimpleCacheWithBagOStuff source, and not only is it not proxying makeKey - it doesn't even have a makeKey or similar method from what I can see. How is it currently supposed to be used?

I tried to find a usage example, and the first one I found was SqlEntityInfoBuilder.php

$value = $this->termCache->get( implode( '.', [ $entityId, $language, $termType ] ) );

If that is a typical use for Wikibase, then we might have a lot of work to do as this is both contractually problematic (not calling the makeKey method) and conventionally highly problematic (not using any method, and no wiki prefix, and no key class component, and no colon separator). This leads to production traffic to the cache backend in question in a way that will not be able to be monitored or investigated by the means common to most MW developers or SRE.

The good news is, that this is relatively easy to do that. Either by grouping them all together in a single makeKey inside the proxy (and remove the hardcoded regex which is not needed), of if the the keys are not all closely related then by editing all get() statements and adding a call to a new makeKey() method. As you do this make sure that the first component of makeKey() is understandable without local context (e.g. wikibase-termlist). For cache keys that are intentionally to be shared across all wikis, use makeGlobalKey() instead.

Thanks for those thoughts!

So in your opinion it would be good if SimpleCacheWithBagOStuff:

• automatically used makeKey
• helped build more conventional cache keys

Can you point me at the canonical naming scheme? I looked in https://www.mediawiki.org/wiki/Manual:Caching but didn't see it

Yes, all keys passed to get/set must come from makeKey or makeGlobalKey on the same BagOStuff class.

Where to call that depends on whether you intent to expose a general cache interface or one that has a finegrained purpose.

Two examples. The generic one requires the caller to use (indirectly) use makeKey, and the caller is responsible for the TTL etc.

use Wikimedia\LightweightObjectStore\ExpirationAwareness;
class MyAlternateCacheInterface implements ExpirationAwareness {
	__construct(BagOStuff $cache) {
		$this->cache = $cache;
	}
	getKey ($keyspace, ...$components) {
		return $this->cache->makeKey($keyspace, ...$components);
	}
	fetch ($key) {
		return $this->cache->get($key);
	}
	store ($key, $val, $ttl = self:TTL_HOUR) {
		return $this->cache->set($key, $val, $ttl);
	}
}

The specific one takes care of that for you, and only provides getting and storing of specific things which have known key fragments.

class ThingCache {
  private const VERSION = 2;
  private const TTL = BagOStuff::TTL_HOUR;
	__construct(BagOStuff $cache) {
		$this->cache = $cache;
	}
	private getKey (string $name, int $width, int $height) {
		return $this->cache->makeKey('wikibase-thing-named', $name, $width, $height, self::VERSION);
	}
	getNamedThing (ThingDesc $desc) {
		return $this->cache->get($this->getKey($desc->name, $desc->width, $desc->height));
	}
	setNamedThing (ThingDesc $desc, array $thingData) {
		return $this->cache->set(
			$this->getKey($desc->name, $desc->width, $desc->height),
			$thingData,
			self::TTL
		);
	}
}

This example would produce a key like testwiki:wikibase-thing-named: … : … : …, and if any of the parts contain special characters these are escaped for you based on the characters and size supported by the current backend. Requirements vary by apcu, wincache, sql, memcached, redis etc.

Change 583199 had a related patch set uploaded (by Ladsgroup; owner: Ladsgroup):
[mediawiki/extensions/Wikibase@master] Call makeKey when sending the key to the internal cache in SimpleCacheWithBagOStuff

https://gerrit.wikimedia.org/r/583199

Moving this to test verification as all keys will go through makeKey now. Sadly we don't have a programmatic way to ensure all callers follow our conventions.

Change 583199 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Call makeKey when sending the key to the internal cache in SimpleCacheWithBagOStuff

https://gerrit.wikimedia.org/r/583199

There is still an issue with this as identified in T246207#6078825 and T246207#6078826

In T246207#6078826, @Addshore wrote:

The comment in https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Wikibase/+/583199/2/lib/includes/SimpleCacheWithBagOStuff.php is relevant here, as this is the extra assert that is now in Wikibase code that is throwing.
As said in that comment Wikibase code shouldn't care about the validity of the cache key for this class, as the contract of the cache class that we are using says that if we use makeKey which we do we will be good.

The URL decoded cache key is commonswiki:wikibase.repo.formatter.:Q6998382_1137583700_⧼lang⧽_label and again we see this ⧼lang⧽ that keeps cropping up.
Anyway, the bag o stuff is getting a valid cache key for storing in the cache medium, and our assert is then throwing the exception when it doesnt need to, so the assert should go.

Now, ⧼lang⧽ still shouldn't be making it all the way down the stack here...
I dont know what triggers calls to pages such as https://commons.wikimedia.org/wiki/Category:Plates?uselang=%E2%A7%BCLang%E2%A7%BD but I will file a seperate task for that.

Change 592304 had a related patch set uploaded (by Addshore; owner: Addshore):
[mediawiki/extensions/Wikibase@master] Remove bad assertions for keys in SimpleCacheWithBagOStuff

https://gerrit.wikimedia.org/r/592304

Change 592304 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] Remove bad assertions for keys in SimpleCacheWithBagOStuff

https://gerrit.wikimedia.org/r/592304