Page MenuHomePhabricator

WikiPage config cache should be invalidated upon page deletion
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

  • Choose your favourite CommunityConfiguration provider that uses a WikiPage
  • Set any override in the config, make sure it works
  • Delete the config page

What happens?:
MediaWikiConfigReader keeps returning the cached value before deletion (including the override set above).

What should have happened instead?:
It should return default values.

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia): master

Other information (browser name/version, screenshots, etc.): AbstractJsonStore caches the configuration in WANCache. This cache is invalidates when a config page is edited (in WikiPageStoreEventIngress), but not when the page is deleted.

On a related note, maybe double-check what happens when the page is moved. With suppressredirect it should count as a page deletion, so it would be covered by listening for deletions. Otherwise, I think it might be covered by the existing revision update (page moves insert a dummy revision).

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Urbanecm_WMF triaged this task as High priority.
Urbanecm_WMF subscribed.

👀 Good to know! Thanks for reporting.

I think we should just switch the listener to use PageRecordChangedEvent instead, which includes also other events besides an edit (such as a move or a deletion).

Change #1176770 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/CommunityConfiguration@master] fix: WikiPageStore's cache should be invalidated upon page deletion

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

@daniel I realise I am the first one who tries to subscribe to PageRecordChangedEvent directly. https://www.mediawiki.org/wiki/Manual:Domain_events says that is allowed (in case I want to cover more events at the same time), but I would appreciate a double-check from you as well.

@daniel I realise I am the first one who tries to subscribe to PageRecordChangedEvent directly. https://www.mediawiki.org/wiki/Manual:Domain_events says that is allowed (in case I want to cover more events at the same time), but I would appreciate a double-check from you as well.

Yes, that's fine. That'S exactly the kind of use case we had in mind when making this possible.

Keep in mind that the information you can get from the generic event object is rather limited. Let me know how this goes for you. One big question was whether listeners would end up needing to type-check and down-cast anyway, so they could as well just subscribe to the more specific events...

Thanks @daniel. I think PageRecordChangedEvent::getPageRecordBefore() gives me what I need (and allows me to filter "page creation" cases out that I am not interested in). It seems to work locally, but CI is having an issue with the test I wrote for that at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CommunityConfiguration/+/1176770. Help would be appreciated.

Thanks @daniel. I think PageRecordChangedEvent::getPageRecordBefore() gives me what I need (and allows me to filter "page creation" cases out that I am not interested in). It seems to work locally, but CI is having an issue with the test I wrote for that at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/CommunityConfiguration/+/1176770. Help would be appreciated.

@Daimona helped out in Slack. Thank you very much!

(For posterity, the cause was WANCache returning the purged value as a "volatile key hit", basically the same as T392852)

Change #1182146 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/CommunityConfiguration@master] [DNM] Split PageRecordChangedListener into two events

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

Change #1182567 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/CommunityConfiguration@master] [DNM] Switch to PageLatestRevisionChangedEvent

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

Change #1182568 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/CommunityConfiguration@master] [DNM] Add more events

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

Change #1182146 abandoned by Urbanecm:

[mediawiki/extensions/CommunityConfiguration@master] [DNM] Split PageRecordChangedListener into two events

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

Change #1182567 abandoned by Urbanecm:

[mediawiki/extensions/CommunityConfiguration@master] [DNM] Switch to PageLatestRevisionChangedEvent

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

Change #1182568 abandoned by Urbanecm:

[mediawiki/extensions/CommunityConfiguration@master] [DNM] Add more events

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

Change #1176770 merged by jenkins-bot:

[mediawiki/extensions/CommunityConfiguration@master] fix: WikiPageStore's cache should be invalidated upon page deletion

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