Page MenuHomePhabricator

SimpleCacheWithBagOStuff shouldnt be so easy to use bad keys with
Closed, ResolvedPublic

Description

Relates to:

SimpleCacheWithBagOStuff should not be so easy to use with cache keys that dont work.

TBA all of the things written in this comment

So we (@Jakob_WMDE and @Rosalie_WMDE and me) looked at this in some detail. We came to the following conclusions (chip in if I misrepresented anything)

The ideal solution is for us to fix \Wikibase\Lib\SimpleCacheWithBagOStuff to reduce the number of times it fails by including some attempt to use makeKey. We were not keen to touch this on a Friday to fix the UBN though because:

  • it's currently untested due to T237164
  • it will change all our Wikibase caches (and potentially mean things like using new keys)

We evaluated the patches by Leszek and concluded that:

  • it only filtered out possible non-term languages from the fallback chain
    • this meant that bad languages could still be passed on to the EntityAccessor

We considered moving this test for a valid term language to the getLanguage method but the problem was still that:

  • this would mean that valid interface languages (but not valid terms languages) would be filtered out (e.g. users with interface language 'fil' would not fallback to 'tag' but straight to 'en'. I.e. we don't want to mix up user interface and term languages at this level.

We concluded that code merged that resulted in these errors starting now is probably due to the usage of \Wikibase\Lib\Store\TermCacheKeyBuilder::buildCacheKey. The two "new" places that use this are \Wikibase\Lib\Store\CachingPrefetchingTermLookup::getCacheKey and \Wikibase\Lib\Store\UncachedTermsPrefetcher::getCacheKey whereas \Wikibase\Lib\Store\CachingFallbackLabelDescriptionLookup::getTerm has used this for some time.

We are happy to see that these two are only concerned with terms so this seems an appropriate place to add that filtering.

To quickly follow is:

  • back-portable patch to get the train going ASAP
  • separate patch for testing CachingPrefetchingTermLookup and UncachedTermsPrefetcher

We will also make tickets/reevaluate priority for:

  • SimpleCacheWithBagOStuff being tested
  • Making SimpleCacheWithBagOStuff less likely to throw
  • investigating in greater detail how those bad languages got into wikibase scribunto (we struggled to see how in our rough and ready testing)

Event Timeline

Addshore triaged this task as Medium priority.Feb 17 2020, 8:55 AM

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?

Lydia_Pintscher added a subscriber: Lydia_Pintscher.

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

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.

MyAlternateCacheInterface.php
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.

ThingCache.php
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

hoo added a subscriber: hoo.

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

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

Addshore added subscribers: elukey, aaron.

The gerrit patch above fixes the error described in T245396 and will be deployed with .30 this week.