Page MenuHomePhabricator

[Beta Commons] All attempts to upload with UploadWizard fail
Closed, DuplicatePublicBug

Description

User story: N/A

We have this:
Everything is failing with a generic "error" message

We want this:
UploadWizard should function as normal.

Screenshots (if possible):

Acceptance Criteria:

  • UploadWizard actually works on Beta

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 31 2019, 6:17 PM
Ramsey-WMF triaged this task as High priority.May 31 2019, 6:18 PM
Ramsey-WMF added a subscriber: Jdforrester-WMF.

Any thoughts on this one, @Jdforrester-WMF 😃

API response is an internal fatal:

Argument 1 passed to AbuseFilterCachingParser::evalNode() must be an instance of AFPTreeNode, null given  </div>
Restricted Application added a subscriber: Daimona. · View Herald TranscriptMay 31 2019, 6:28 PM
Jdforrester-WMF raised the priority of this task from High to Unbreak Now!.May 31 2019, 6:28 PM

I'm declaring this a train blocker for next week.

Restricted Application added a subscriber: Liuxinyu970226. · View Herald TranscriptMay 31 2019, 6:28 PM

I'll check it. Anyway, AbuseFilterCachingParser is only used in test wikis, whereas production uses AbuseFilterParser. Thus, it's a beta-only issue. Of note, CachingParser is currently very bugged (T156095), and we're working on it these days.

Daimona lowered the priority of this task from Unbreak Now! to High.May 31 2019, 6:46 PM

Aha, interesting. Why is Beta Cluster using CachingParser if it doesn't work yet?

I think it was enabled to test it and see what errors (like this one) could come out. The config patch is https://gerrit.wikimedia.org/r/#/c/operations/mediawiki-config/+/314604/, which doesn't have a real reason.
Since we're here, I guess we should first try to fix this specific bug, unless 1-it turns out to be complicated 2-it's absolutely necessary that it doesn't happen on beta commons anymore, in which case we can simply make beta commons use the usual parser.
Do we have a more detailed stacktrace? Is it available on beta logstash?

No trace (Beta Cluster is prod-like) or fatal hash (API is unhelpful), sorry.

I could actually find it on beta logstash, we have

message
PHP Fatal Error: Argument 1 passed to AbuseFilterCachingParser::evalNode() must be an instance of AFPTreeNode, null given
trace
#0 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php(123): array_map(array, array)
#1 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php(230): AbuseFilterCachingParser->evalNode(AFPTreeNode)
#2 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/parser/AbuseFilterCachingParser.php(66): AbuseFilterCachingParser->evalNode(AFPTreeNode)
#3 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/parser/AbuseFilterParser.php(190): AbuseFilterCachingParser->intEval(string)
#4 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilter.php(541): AbuseFilterParser->parse(string)
#5 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilter.php(699): AbuseFilter::checkConditions(string, AbuseFilterVariableHolder, boolean, string)
#6 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilter.php(600): AbuseFilter::checkFilter(stdClass, AbuseFilterVariableHolder, Title, string, string)
#7 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilter.php(1195): AbuseFilter::checkAllFilters(AbuseFilterVariableHolder, Title, string, string)
#8 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilterHooks.php(832): AbuseFilter::filterAction(AbuseFilterVariableHolder, Title, string, User)
#9 /srv/mediawiki/php-master/extensions/AbuseFilter/includes/AbuseFilterHooks.php(744): AbuseFilterHooks::filterUpload(string, UploadFromFile, User, array, NULL, NULL, NULL)
#10 /srv/mediawiki/php-master/includes/Hooks.php(174): AbuseFilterHooks::onUploadStashFile(UploadFromFile, User, array, NULL)
#11 /srv/mediawiki/php-master/includes/Hooks.php(202): Hooks::callHook(string, array, array, NULL)
#12 /srv/mediawiki/php-master/includes/upload/UploadBase.php(1081): Hooks::run(string, array)
#13 /srv/mediawiki/php-master/includes/upload/UploadBase.php(1061): UploadBase->runUploadStashFileHook(User)
#14 /srv/mediawiki/php-master/includes/api/ApiUpload.php(315): UploadBase->tryStashFile(User, boolean)
#15 /srv/mediawiki/php-master/includes/api/ApiUpload.php(162): ApiUpload->performStash(string, array)
#16 /srv/mediawiki/php-master/includes/api/ApiUpload.php(135): ApiUpload->getStashResult(array)
#17 /srv/mediawiki/php-master/includes/api/ApiUpload.php(104): ApiUpload->getContextResult()
#18 /srv/mediawiki/php-master/includes/api/ApiMain.php(1595): ApiUpload->execute()
#19 /srv/mediawiki/php-master/includes/api/ApiMain.php(531): ApiMain->executeAction()
#20 /srv/mediawiki/php-master/includes/api/ApiMain.php(502): ApiMain->executeActionWithErrorHandling()
#21 /srv/mediawiki/php-master/api.php(87): ApiMain->execute()
#22 /srv/mediawiki/w/api.php(3): include(string)
#23 {main}

Re-voicing from Daimona, I agree it'd be useful to keep the caching parser enabled in Beta so as to keep finding new issues and work on them.

But, if a specific issue we find is non-trivial to fix or we're unavailable, should be fine to disable during those dips, and then re-enable afterwards.

I forgot to say that I took a quick look at the issue. Unfortunately, we don't know what's the faulty filter (the parser is unaware), and the triggering line doesn't tell much. All I know ATM is that, during a function call, an argument is evaluated to null. We could use some heuristics to dig through the parser and try to figure out the exact code path, but I don't think it's worthwhile. Instead, I believe we should first try to find the faulty filter on beta commons, and then dig through the parser with a failing sample at hand. Note that I also manually hit "Check syntax" for all enabled filters, but none of them failed. So this is probably another example of T214643. Plus, given that it's related to function calls, I also used the filter search to see if any filter has dangling commas in function calls (which would be T153251). The answer is, apparently, no.
The first idea that comes to mind is, given that the error can consistently be reproduced, to bisect existing filters; i.e., disable half of them and try again, etc. Maybe this operation could be sped up by changing values in the DB directly. But even then, it won't be that quick. Suggestions are welcome.

Ramsey-WMF moved this task from Untriaged to Tracking on the Multimedia board.Jun 4 2019, 3:56 PM

Any updates on this? We do need this addressed in the near future 🔜

@Ramsey-WMF As I said above, we first need a minimal test case. Beta Commons, being production-like, has several filters enabled, so it's not easy to find the culprit one. To summarize, I see three different ways to find the offending filter:

  1. We disable ~5 filters at a time, re-trying the faulty upload every time. I could do that myself (I'm already g-sysop on BC), but I'd need to know what to upload, with what title and comment, etc. in order to trigger the error (or someone else could do that while I disable filters)
  2. Same as 1., but instead of disabling filters manually, we do it via direct DB manipulation. Being in deployment-prep I should be able to connect to MariaDB, but I'm unsure if it would allow me to issue write queries.
  3. Some ad-hoc debugging is added for beta-cluster only, so to include the faulty filter in the error message. Note this won't probably be easy to do in production because it'd need a not-so-small change.

Whatever is better for you, I can help. For 1., I can start right now if you provide me the upload info (or we can coordinate e.g. on IRC so that I edit filters and you try to upload the file).

Hello @Daimona

Regarding the quoted section below:

We disable ~5 filters at a time, re-trying the faulty upload every time. I could do that myself (I'm already g-sysop on BC), but I'd need to know what to upload, with what title and comment, etc. in order to trigger the error (or someone else could do that while I disable filters)

The error is happening on Beta Commons on ALL uploads, so there's no particular title or comment or anything to add. Just try to upload any image to Beta Commons and you should get the error triggered.

OK, thanks. So I tried as above, and found that the culprit is filter 31, which is now disabled. Now it's also clear that this bug is a duplicate of T153251, which for some reason didn't have a stack trace.

Restricted Application removed a subscriber: Liuxinyu970226. · View Herald TranscriptJun 6 2019, 1:05 PM

Thank you! 😄