Page MenuHomePhabricator

Draft purge script should not fail when sending notification is failed
Closed, ResolvedPublic

Description

While running script scripts/purge-unpublished-drafts.php (T199658), I got this error. Draft purge script should not fail when sending notification is failed or handle it gracefully.

MWException from line 332 of /srv/mediawiki/php-1.32.0-wmf.12/extensions/Echo/includes/controller/NotificationController.php: Cannot notify anonymous user: 127.0.0.1
Backtrace:
#0 /srv/mediawiki/php-1.32.0-wmf.12/extensions/Echo/includes/controller/NotificationController.php(116): EchoNotificationController::doNotification(EchoEvent, User, string)
#1 /srv/mediawiki/php-1.32.0-wmf.12/extensions/Echo/includes/model/Event.php(168): EchoNotificationController::notify(EchoEvent, boolean)
#2 /srv/mediawiki/php-1.32.0-wmf.12/extensions/ContentTranslation/includes/Notification.php(82): EchoEvent::create(array)
#3 /srv/mediawiki/php-1.32.0-wmf.12/extensions/ContentTranslation/scripts/purge-unpublished-drafts.php(148): ContentTranslation\Notification::draftDeleted(integer, Title, string, string)
#4 /srv/mediawiki/php-1.32.0-wmf.12/extensions/ContentTranslation/scripts/purge-unpublished-drafts.php(101): ContentTranslation\Scripts\PurgeUnpublishedDrafts->notifyUsers(array)
#5 /srv/mediawiki/php-1.32.0-wmf.12/maintenance/doMaintenance.php(94): ContentTranslation\Scripts\PurgeUnpublishedDrafts->execute()
#6 /srv/mediawiki/php-1.32.0-wmf.12/extensions/ContentTranslation/scripts/purge-unpublished-drafts.php(155): include(string)
#7 /srv/mediawiki/multiversion/MWScript.php(100): include(string)
#8 {main}

Details

Related Gerrit Patches:
mediawiki/extensions/ContentTranslation : masterHandle exceptions while notifying users about purged drafts

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 19 2018, 4:21 AM
KartikMistry updated the task description. (Show Details)Jul 19 2018, 4:22 AM

Option 1: manually purge these entries from the database (error prone)
Option 2: Add a check to avoid sending notifications to anonymous users (less error prone, requires code changes)

Both options will achieve the same end result. I would do option 2.

Pginer-WMF triaged this task as Medium priority.Aug 8 2018, 6:41 AM

Change 451279 had a related patch set uploaded (by Petar.petkovic; owner: Petar.petkovic):
[mediawiki/extensions/ContentTranslation@master] Handle exceptions while notifying users about purged drafts

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

This exception is thrown from NotificationController::doNotification( $event, $user, $type ) method, which gets $user param from getUsersToNotifyForEvent( EchoEvent $event ) method, that does some checks to filter out the anonymous users from the list of users which get the notification.

Even with such safety method, we have the exception because user is anon. @Nikerabbit suspects that this is caused because we store global user IDs in the table, but we are assuming they are local user IDs when sending notifications. Patch that I have submitted checks CentralIdLookup to find local user from central user ID before attempting notification. Also, some exception handling logic is added to prevent the case of users not getting notification if something breaks while processing the queue.

Petar.petkovic moved this task from Backlog to Other on the CX-deployments board.

Change 451279 merged by KartikMistry:
[mediawiki/extensions/ContentTranslation@master] Handle exceptions while notifying users about purged drafts

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

The error looks odd to me - how anon user was able to create an article with Tag: ContentTranslation? I am moving it to Blocked since the task is pending scheduling script run.

Script ran and no such failed and it was handled gracefully.

Example:

ERROR: Trying to notify unknown user with ID 42138907 about the deletion of the following page(s):
 * Дин Мартин

I want to investigate further what is the root cause.

https://www.mediawiki.org/wiki/Special:ApiSandbox#action=query&format=json&meta=globaluserinfo&guiid=42138907
https://meta.wikimedia.org/wiki/Special:CentralAuth/KREVSKA

So there is no guarantee that an account attached to the global user account exists on the wiki where we run the script, unless that is loginwiki where I suppose everyone has one, but CX is not installed there.

One option would be to run this on each Wikipedia and filter per target or source language. Or ask for Growth team for advice.

I propose we stop script runs until a decision has been made on this issue.

https://www.mediawiki.org/wiki/Special:ApiSandbox#action=query&format=json&meta=globaluserinfo&guiid=42138907
https://meta.wikimedia.org/wiki/Special:CentralAuth/KREVSKA
So there is no guarantee that an account attached to the global user account exists on the wiki where we run the script, unless that is loginwiki where I suppose everyone has one, but CX is not installed there.
One option would be to run this on each Wikipedia and filter per target or source language. Or ask for Growth team for advice.
I propose we stop script runs until a decision has been made on this issue.

Thanks for the investigation @Nikerabbit
I created a separate ticket: T203071: Allow the notification of deleted old translations to users missing a local account

Pginer-WMF closed this task as Resolved.Sep 3 2018, 9:22 AM