Page MenuHomePhabricator
Create Task
Maniphest T435028

Core tests fail with Echo but not Flow installed
Closed, ResolvedPublicBUG REPORT
Actions

Description

Steps to replicate the issue:

  1. Install Echo but not Flow.
  2. Run: composer phpunit -- tests/phpunit/includes/Storage/PageUpdaterTest.php --stop-on-failure

What happens?

The test fails:

1) MediaWiki\Tests\Storage\PageUpdaterTest::testUpdatePropagation with data set "user talk" (MediaWiki\Page\PageIdentityValue Object (...), null, 'MediaWiki-Tests-Storage-PageU...ation1')
MediaWiki\User\TalkPageNotificationManager::setUserHasNewMessages(MediaWiki\User\User Object (...), null): void was not expected to be called more than once.

mediawiki/extensions/Echo/includes/Model/Notification.php:89
mediawiki/extensions/Echo/includes/Notifier.php:33
mediawiki/extensions/Echo/includes/Controller/NotificationController.php:458
mediawiki/extensions/Echo/includes/Controller/NotificationController.php:154
mediawiki/extensions/Echo/includes/Model/Event.php:200
mediawiki/extensions/Echo/includes/DiscussionParser.php:209
mediawiki/extensions/Echo/includes/MediaWikiEventIngress/PageEventIngress.php:58
mediawiki/includes/DomainEvent/EventDispatchEngine.php:205
mediawiki/includes/DomainEvent/EventDispatchEngine.php:194
mediawiki/includes/Deferred/MWCallableUpdate.php:52
mediawiki/includes/Deferred/DeferredUpdates.php:448
mediawiki/includes/Deferred/DeferredUpdates.php:187
mediawiki/includes/Deferred/DeferredUpdates.php:277
mediawiki/includes/Deferred/DeferredUpdatesScope.php:229
mediawiki/includes/Deferred/DeferredUpdatesScope.php:158
mediawiki/includes/Deferred/DeferredUpdates.php:271
mediawiki/includes/Deferred/DeferredUpdates.php:363
mediawiki/includes/Deferred/DeferredUpdates.php:142
mediawiki/includes/Deferred/DeferredUpdates.php:165
mediawiki/includes/Storage/PageUpdater.php:883
mediawiki/tests/phpunit/includes/Storage/PageUpdaterTest.php:806

What should have happened instead?:

The tests should pass with any combination of Echo and Flow being loaded.

Software versions:

Details

Related Changes in Gerrit:
SubjectAuthorRepoBranchLines +/-
Remove setting of new-message notification as it's now done in coreSamwilsonmediawiki/extensions/Echomaster+0 -4
Customize query in gerrit

Related Objects
Search...

Event Timeline

Samwilson created this task.Sun, Aug 16, 1:03 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSun, Aug 16, 1:03 PM
Samwilson added projects: Notifications (Echo), StructuredDiscussions.Sun, Aug 16, 1:24 PM
Samwilson removed a project: StructuredDiscussions.Mon, Aug 17, 12:20 AM
Samwilson added a parent task: T277470: Ignore (most) of LocalSettings.php when running PHPUnit (fix surprise test failures).Mon, Aug 17, 12:23 AM
Novem_Linguae subscribed.Mon, Aug 17, 12:57 AM
Samwilson added a comment.Mon, Aug 24, 9:40 AM

I'm guessing this isn't caught by CI because there we always have both Echo and Flow installed. But I'm not sure why it's not come up for other developers, because surely most people aren't running Flow locally (and are likely to have Echo enabled) so why aren't they hitting this error?

Restricted Application added a project: MediaWiki-Core-Platform-Team. · View Herald TranscriptMon, Aug 24, 9:40 AM
pmiazga moved this task from Inbox, needs triage to Needs refinement on the MediaWiki-Core-Platform-Team board.Fri, Aug 28, 10:19 AM
pmiazga subscribed.

When running test run on Core+Echo only we have two calls:

Called from:
#0 /var/www/html/w/includes/RecentChanges/ChangeTrackingEventIngress.php(333): MediaWiki\User\TalkPageNotificationManager->setUserHasNewMessages()
#1 /var/www/html/w/includes/RecentChanges/ChangeTrackingEventIngress.php(192): MediaWiki\RecentChanges\ChangeTrackingEventIngress->updateNewTalkAfterPageUpdated()
#2 /var/www/html/w/includes/DomainEvent/EventDispatchEngine.php(205): MediaWiki\RecentChanges\ChangeTrackingEventIngress->handlePageLatestRevisionChangedEvent()
#3 /var/www/html/w/includes/DomainEvent/EventDispatchEngine.php(194): MediaWiki\DomainEvent\EventDispatchEngine->invoke()
#4 /var/www/html/w/includes/Deferred/MWCallableUpdate.php(52): MediaWiki\DomainEvent\EventDispatchEngine->MediaWiki\DomainEvent\{closure}()
#5 /var/www/html/w/includes/Deferred/DeferredUpdates.php(448): MediaWiki\Deferred\MWCallableUpdate->doUpdate()
#6 /var/www/html/w/includes/Deferred/DeferredUpdates.php(187): MediaWiki\Deferred\DeferredUpdates::attemptUpdate()
#7 /var/www/html/w/includes/Deferred/DeferredUpdates.php(277): MediaWiki\Deferred\DeferredUpdates::run()
#8 /var/www/html/w/includes/Deferred/DeferredUpdatesScope.php(229): MediaWiki\Deferred\DeferredUpdates::MediaWiki\Deferred\{closure}()
#9 /var/www/html/w/includes/Deferred/DeferredUpdatesScope.php(158): MediaWiki\Deferred\DeferredUpdatesScope->processStageQueue()
#10 /var/www/html/w/includes/Deferred/DeferredUpdates.php(271): MediaWiki\Deferred\DeferredUpdatesScope->processUpdates()
#11 /var/www/html/w/includes/Deferred/DeferredUpdates.php(363): MediaWiki\Deferred\DeferredUpdates::doUpdates()
#12 /var/www/html/w/includes/Deferred/DeferredUpdates.php(142): MediaWiki\Deferred\DeferredUpdates::tryOpportunisticExecute()
#13 /var/www/html/w/includes/Deferred/DeferredUpdates.php(165): MediaWiki\Deferred\DeferredUpdates::addUpdate()
#14 /var/www/html/w/includes/Storage/PageUpdater.php(883): MediaWiki\Deferred\DeferredUpdates::addCallableUpdate()
#15 /var/www/html/w/includes/Page/WikiPage.php(1694): MediaWiki\Storage\PageUpdater->saveRevision()
#16 /var/www/html/w/tests/phpunit/MediaWikiIntegrationTestCase.php(2662): MediaWiki\Page\WikiPage->doUserEditContent()
#17 /var/www/html/w/tests/phpunit/includes/Storage/PageUpdaterTest.php(828): MediaWikiIntegrationTestCase->editPage()
#18 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestCase.php(1617): MediaWiki\Tests\Storage\PageUpdaterTest->testUpdatePropagation_null()
#19 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestCase.php(1223): PHPUnit\Framework\TestCase->runTest()
#20 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestResult.php(729): PHPUnit\Framework\TestCase->runBare()
#21 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestCase.php(973): PHPUnit\Framework\TestResult->run()
#22 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestSuite.php(685): PHPUnit\Framework\TestCase->run()
#23 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestSuite.php(685): PHPUnit\Framework\TestSuite->run()
#24 /var/www/html/w/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(651): PHPUnit\Framework\TestSuite->run()
#25 /var/www/html/w/vendor/phpunit/phpunit/src/TextUI/Command.php(146): PHPUnit\TextUI\TestRunner->run()
#26 /var/www/html/w/vendor/phpunit/phpunit/src/TextUI/Command.php(99): PHPUnit\TextUI\Command->run()
#27 /var/www/html/w/vendor/phpunit/phpunit/phpunit(107): PHPUnit\TextUI\Command::main()
#28 /var/www/html/w/vendor/bin/phpunit(122): include('...')
#29 {main}

Called from:
#0 /var/www/html/w/extensions/Echo/includes/Model/Notification.php(89): MediaWiki\User\TalkPageNotificationManager->setUserHasNewMessages()
#1 /var/www/html/w/extensions/Echo/includes/Notifier.php(33): MediaWiki\Extension\Notifications\Model\Notification->insert()
#2 /var/www/html/w/extensions/Echo/includes/Controller/NotificationController.php(460): MediaWiki\Extension\Notifications\Notifier::notifyWithNotification()
#3 /var/www/html/w/extensions/Echo/includes/Controller/NotificationController.php(154): MediaWiki\Extension\Notifications\Controller\NotificationController::doNotification()
#4 /var/www/html/w/extensions/Echo/includes/Model/Event.php(204): MediaWiki\Extension\Notifications\Controller\NotificationController::notify()
#5 /var/www/html/w/extensions/Echo/includes/DiscussionParser.php(209): MediaWiki\Extension\Notifications\Model\Event::create()
#6 /var/www/html/w/extensions/Echo/includes/MediaWikiEventIngress/PageEventIngress.php(61): MediaWiki\Extension\Notifications\DiscussionParser::generateEventsForRevision()
#7 /var/www/html/w/includes/DomainEvent/EventDispatchEngine.php(205): MediaWiki\Extension\Notifications\MediaWikiEventIngress\PageEventIngress->handlePageLatestRevisionChangedEvent()
#8 /var/www/html/w/includes/DomainEvent/EventDispatchEngine.php(194): MediaWiki\DomainEvent\EventDispatchEngine->invoke()
#9 /var/www/html/w/includes/Deferred/MWCallableUpdate.php(52): MediaWiki\DomainEvent\EventDispatchEngine->MediaWiki\DomainEvent\{closure}()
#10 /var/www/html/w/includes/Deferred/DeferredUpdates.php(448): MediaWiki\Deferred\MWCallableUpdate->doUpdate()
#11 /var/www/html/w/includes/Deferred/DeferredUpdates.php(187): MediaWiki\Deferred\DeferredUpdates::attemptUpdate()
#12 /var/www/html/w/includes/Deferred/DeferredUpdates.php(277): MediaWiki\Deferred\DeferredUpdates::run()
#13 /var/www/html/w/includes/Deferred/DeferredUpdatesScope.php(229): MediaWiki\Deferred\DeferredUpdates::MediaWiki\Deferred\{closure}()
#14 /var/www/html/w/includes/Deferred/DeferredUpdatesScope.php(158): MediaWiki\Deferred\DeferredUpdatesScope->processStageQueue()
#15 /var/www/html/w/includes/Deferred/DeferredUpdates.php(271): MediaWiki\Deferred\DeferredUpdatesScope->processUpdates()
#16 /var/www/html/w/includes/Deferred/DeferredUpdates.php(363): MediaWiki\Deferred\DeferredUpdates::doUpdates()
#17 /var/www/html/w/includes/Deferred/DeferredUpdates.php(142): MediaWiki\Deferred\DeferredUpdates::tryOpportunisticExecute()
#18 /var/www/html/w/includes/Deferred/DeferredUpdates.php(165): MediaWiki\Deferred\DeferredUpdates::addUpdate()
#19 /var/www/html/w/includes/Storage/PageUpdater.php(883): MediaWiki\Deferred\DeferredUpdates::addCallableUpdate()
#20 /var/www/html/w/includes/Page/WikiPage.php(1694): MediaWiki\Storage\PageUpdater->saveRevision()
#21 /var/www/html/w/tests/phpunit/MediaWikiIntegrationTestCase.php(2662): MediaWiki\Page\WikiPage->doUserEditContent()
#22 /var/www/html/w/tests/phpunit/includes/Storage/PageUpdaterTest.php(828): MediaWikiIntegrationTestCase->editPage()
#23 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestCase.php(1617): MediaWiki\Tests\Storage\PageUpdaterTest->testUpdatePropagation_null()
#24 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestCase.php(1223): PHPUnit\Framework\TestCase->runTest()
#25 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestResult.php(729): PHPUnit\Framework\TestCase->runBare()
#26 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestCase.php(973): PHPUnit\Framework\TestResult->run()
#27 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestSuite.php(685): PHPUnit\Framework\TestCase->run()
#28 /var/www/html/w/vendor/phpunit/phpunit/src/Framework/TestSuite.php(685): PHPUnit\Framework\TestSuite->run()
#29 /var/www/html/w/vendor/phpunit/phpunit/src/TextUI/TestRunner.php(651): PHPUnit\Framework\TestSuite->run()
#30 /var/www/html/w/vendor/phpunit/phpunit/src/TextUI/Command.php(146): PHPUnit\TextUI\TestRunner->run()
#31 /var/www/html/w/vendor/phpunit/phpunit/src/TextUI/Command.php(99): PHPUnit\TextUI\Command->run()
#32 /var/www/html/w/vendor/phpunit/phpunit/phpunit(107): PHPUnit\TextUI\Command::main()
#33 /var/www/html/w/vendor/bin/phpunit(122): include('...')

First call is from Echo, and then second call is from ChangeTrackingEventIngress.
After adding Flow, nothing changes; the test still fails - it has to be some specific config.

pmiazga added a comment.EditedFri, Aug 28, 10:50 AM

Core issue

Disclaimer: used some help from Claude to analyze code and format the message

TalkPageNotificationManager::setUserHasNewMessages is called twice for a user-talk edit when Echo is loaded:

  1. Core — includes/RecentChanges/ChangeTrackingEventIngress.php:331/333 (updateNewTalkAfterPageUpdated) — subscribes to PageLatestRevisionChangedEvent and writes newtalk for the talk-page owner.
  2. Echo — extensions/Echo/includes/Model/Notification.php:87-90 — when Notification::insert() runs for an edit-user-talk category event, it also writes newtalk for the same user.

Echo's PageEventIngress handler also subscribes to PageLatestRevisionChangedEvent, so on the same edit both listeners fire → Echo funnels through DiscussionParser → Event::create('edit-user-talk') → Notification::insert → second setUserHasNewMessages call.

Both target the same user ($title->getText() on the NS_USER_TALK page — see DiscussionParser.php:118), so this is a pure duplicate write.

Why it wasn't caught before

  • Echo's write has existed since 2016 (Notification.php:87-90, I0b49c3). Long-standing dead-lock-free duplication.
  • Core's newtalk write used to live in DerivedPageDataUpdater::doUpdates and moved to ChangeTrackingEventIngress in Jan 2025 (change I5f2083, {T378936: Use the PageUpdatedEvent to trigger relevant updates in MediaWiki core}) — same behaviour, different call site.
  • The test only became strict on 2024-11-29 in change If06e2a ("Tests: assert updates on page lifecycle changes", also T378936) — that's the change that added the $talkPageNotifications param to expectChangeTrackingUpdates and pinned it to exactly-once.
  • CI runs core tests without extensions, so CI stayed green. Locally, with Echo installed, the exact-once assertion trips on the duplicate write.

The design intent already documented in Echo

extensions/Echo/includes/Hooks.php:1119-1123 says explicitly:

Handler for GetNewMessagesAlert hook. We're using the GetNewMessagesAlert hook instead of the ArticleEditUpdateNewTalk hook since we still want the user_newtalk data to be updated and available to client-side tools and the API.

Echo already intends core to own the user_newtalk write; Echo only wants to suppress the "You have new messages" banner UI. The setUserHasNewMessages at Notification.php:89 contradicts that stated design — it's a 2016 leftover that never got removed when the responsibility was formally handed back to core.

Proposal

Because the setUserHasNewMessages is already handled by Core ChangeTrackingEventIngress we should be safe to remove this handling from Echo extension. in Model/Notification there is a special when $event->getCategory() === 'edit-user-talk to call setUserHasNewMessages(). It should be safe to drop that.

Samwilson added a comment.Fri, Aug 28, 11:03 AM

@pmiazga What Claude says sounds plausible (hmm... I mean, it would wouldn't it! :-P ) but it sounds like you're not able to reproduce this locally yourself? I'll try to see what I've got in my config that might be changing things.

pmiazga added a comment.EditedFri, Aug 28, 11:36 AM

The test fails locally with Core+Echo, but it also fails with Core+Echo+Flow. I think maybe my flow is misconfigured, because my understanding is that probably it should be somehow set up to capture/modify Talk pages edits, which would mean one of the calls is not triggered. Let me check Flow configs.

Edit:
Added

$wgNamespaceContentModels[NS_TALK] = 'flow-board';
$wgNamespacesWithSubpages[NS_TALK] = true;
$wgNamespaceContentModels[NS_USER_TALK] = 'flow-board';
$wgNamespacesWithSubpages[NS_USER_TALK] = true;

but tests still fail when having both Flow and Echo

pmiazga added a comment.Fri, Aug 28, 12:04 PM

The only thing that comes to my mind is this line https://gerrit.wikimedia.org/g/mediawiki/extensions/Flow/+/6314e20f9c3cfe2963e6a27a6ac73623dc6e11a1/includes/EchoHooks.php#25 - where Flow would stop the Echo event to happen, thus there would be just a single call from MediaWiki\RecentChanges\ChangeTrackingEventIngress::updateNewTalkAfterPageUpdated().

As a trick, I changed the default return to false for edit-user-talk and tests pass

	public function onBeforeEchoEventInsert( Event $event ) {
		$agent = $event->getAgent();
		
		if ( $agent === null ) {
			return true;
		}

		if ( Hooks::isTalkpageManagerUser( $agent ) ) {
			return false;
		}

		if ( $event->getType() === 'edit-user-talk') {
			return false;
		}
		return true;
	}

@Samwilson, when you're going to check this, check if anything is listening for BeforeEchoEventInsert and block Echo from triggering the Event.

gerritbot added a comment.Sat, Aug 29, 9:35 AM

Change #1332182 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/Echo@master] Remove setting of new-message notification as it's now done in core

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

gerritbot added a project: Patch-For-Review.Sat, Aug 29, 9:35 AM
Samwilson added a comment.Sat, Aug 29, 9:39 AM

I've had a go; see the above patch. It appears that there's no need to call it in \MediaWiki\Extension\Notifications\Model\Notification::insert — at least, the tests all pass, and in my testing locally I still correctly get the talk page yellow bar.

pmiazga added a comment.Mon, Aug 31, 9:45 AM

Looks like we came up to the same conclusion. I was going to remove exact same lines. The only thing that confuses me here is why this didn't break on prod CI? As to me, those tests were failing locally both with Echo and Echo+Flow, and from what I see most likely it would break with any set of extensions.

This means that most likely core just runs core tests without any extensions installed, and different extensions run extension tests without the core unit test suite.

Samwilson added a comment.Mon, Aug 31, 10:40 AM

I'm not sure why it's passing in CI. Your hypothesis sounds correct. Or perhaps there's some sort of shared state that only happens when all tests are run (i.e. PageUpdaterTest is never run on its own).

gerritbot added a comment.Mon, Aug 31, 2:12 PM

Change #1332182 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Remove setting of new-message notification as it's now done in core

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

Maintenance_bot removed a project: Patch-For-Review.Mon, Aug 31, 2:31 PM
ReleaseTaggerBot added a project: MW-1.47-notes (1.47.0-wmf.18; 2026-09-01).Mon, Aug 31, 3:00 PM
pmiazga closed this task as Resolved.Wed, Sep 2, 8:49 AM
pmiazga claimed this task.

Im going to resolve this ticket - in the other thread I'll try to find out why this didn't break on prod.

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits