Page MenuHomePhabricator

FlaggedRevs might show the wrong error message when PoolCounter is full
Closed, ResolvedPublicBUG REPORT


While working on (tracked via T277883) we believe we found a potential bug.

In FlaggedRevs::parseStableRevisionPooled() is a PoolCounterWorkViaCallback with doWork, doCachedWork, and fallback functions. The code that executes these functions can be seen in PoolCounterWork::execute(). Fallback is called 2 times there. In both cases the result of the fallback is used as-is when it is anything but false. The current implementation never return false. Instead an empty Status object is returned, which is going to be ignored in FlaggablePageView::showStableVersion() and will show an missing-article error message instead.

That's the wrong error.

See, the fallback is just that, a fallback in case everything else failed. All the fallback does is to try for the last time to find some possibly dirty result in the parser cache to prevent the user being hit by the original error. The fact that the fallback might not be able do this is not the error. It's an expected state that a cache doesn't contain everything. The original error is either that the PoolCounter timed out or the queue was full. This is what's interesting and should be shown.

The correct code path ends with the more correct view-pool-error message instead of missing-article.

A similar issue exists in the doCachedWork function, but that a crazy, probably entirely unreachable edge-case, and doesn't seem like it's easy to fix or even worth it.

Event Timeline

Change 778560 had a related patch set uploaded (by Thiemo Kreuz (WMDE); author: Thiemo Kreuz (WMDE)):

[mediawiki/extensions/FlaggedRevs@master] Show correct error message when PoolCounter is full

Adding to the analysis in the description, in this special case:

  • running in "fast" mode (the default)
  • the pool counter times out
  • there is no dirty result in the cache,

then the PoolCounterWork code is intended to run the work (parse) again, but due to this bug we've identified, it would immediately fail and ultimately show a "missing page" error.

Change 778560 merged by jenkins-bot:

[mediawiki/extensions/FlaggedRevs@master] Show correct error message when PoolCounter is full