Page MenuHomePhabricator
Create Task
Maniphest T315489

Wikibase circular dependency CI break: LanguageFactory -> NamespaceInfo -> UserNameUtils -> ContentLanguage -> LanguageFactory
Closed, ResolvedPublicBUG REPORT
Actions

Description

Breaks CI for all patches in GrowthExperiments at least. Example: https://integration.wikimedia.org/ci/job/quibble-vendor-mysql-php73-noselenium-docker/56960/console

10:26:38 1) Wikibase\Repo\Tests\Hooks\Formatters\EntityLinkFormatterFactoryTest::testGivenSameTypeAndLanguage_getLinkFormatterCachesResult
10:26:38 Wikimedia\Services\RecursiveServiceDependencyException: Recursive service instantiation: Circular dependency when creating service! LanguageFactory -> NamespaceInfo -> UserNameUtils -> ContentLanguage -> LanguageFactory
10:26:38 
10:26:38 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:435
10:26:38 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:411
10:26:38 /workspace/src/includes/MediaWikiServices.php:299
10:26:38 /workspace/src/includes/MediaWikiServices.php:1117
10:26:38 /workspace/src/includes/ServiceWiring.php:450
10:26:38 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
10:26:38 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:411
10:26:38 /workspace/src/includes/MediaWikiServices.php:299
10:26:38 /workspace/src/includes/MediaWikiServices.php:853
10:26:38 /workspace/src/includes/ServiceWiring.php:2047
10:26:38 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
10:26:38 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:411
10:26:38 /workspace/src/includes/MediaWikiServices.php:299
10:26:38 /workspace/src/includes/MediaWikiServices.php:1922
10:26:38 /workspace/src/includes/session/SessionBackend.php:765
10:26:38 /workspace/src/includes/session/SessionBackend.php:236
10:26:38 /workspace/src/includes/session/Session.php:75
10:26:38 /workspace/src/includes/ServiceWiring.php:1131
10:26:38 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
10:26:38 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:411
10:26:38 /workspace/src/includes/MediaWikiServices.php:299
10:26:38 /workspace/src/includes/MediaWikiServices.php:1320
10:26:38 /workspace/src/includes/ServiceWiring.php:803
10:26:38 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:447
10:26:38 /workspace/src/vendor/wikimedia/services/src/ServiceContainer.php:411
10:26:38 /workspace/src/includes/MediaWikiServices.php:299
10:26:38 /workspace/src/includes/MediaWikiServices.php:1117
10:26:38 /workspace/src/includes/language/Language.php:299
10:26:38 /workspace/src/extensions/Wikibase/repo/tests/phpunit/includes/Hooks/Formatters/EntityLinkFormatterFactoryTest.php:59
10:26:38 /workspace/src/tests/phpunit/MediaWikiIntegrationTestCase.php:498

At a glance patches for other extensions seem to be passing so maybe this is somehow specific to GrowthExperiments. It is possible for one extension to interfere with another's tests via hooks, but it's not obvious to be how this would be happening here.

Details

SubjectRepoBranchLines +/-
EntityLinkFormatterFactoryTest: Use mock for languagemediawiki/extensions/Wikibasemaster+6 -2
[DNM] CI testmediawiki/extensions/GrowthExperimentsmaster+0 -1
Customize query in gerrit

Related Objects
Search...

Event Timeline

Tgr created this task.Aug 17 2022, 8:56 PM
Restricted Application added a project: Growth-Team. · View Herald TranscriptAug 17 2022, 8:56 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Tgr added a subscriber: Urbanecm.Aug 17 2022, 8:57 PM
Tgr edited subscribers, added: Urbanecm_WMF; removed: Urbanecm.
gerritbot added a comment.Aug 17 2022, 8:58 PM

Change 824293 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/GrowthExperiments@master] [DNM] CI test

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

gerritbot added a project: Patch-For-Review.Aug 17 2022, 8:58 PM
gerritbot added a comment.Aug 17 2022, 11:57 PM

Change 824293 abandoned by Gergő Tisza:

[mediawiki/extensions/GrowthExperiments@master] [DNM] CI test

Reason:

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

Tgr added a comment.Aug 18 2022, 12:04 AM

So it seems EarlyLifeCycleHooks is somehow causing this or contributing to it. Presumably because it adds a UserOptionsLookup dependency to language-related things (more specifically anything that would do message loading during initialization), but then why doesn't UserOptionsLookup appear in the dependency chain? And none of the constructors of the services indicated in the error do anything that would trigger the MessageCache__get hook.

Maintenance_bot added a project: Wikidata.Aug 18 2022, 12:29 AM
Maintenance_bot removed a project: Patch-For-Review.
gerritbot added a comment.Aug 18 2022, 7:07 AM

Change 824293 restored by Gergő Tisza:

[mediawiki/extensions/GrowthExperiments@master] [DNM] CI test

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

gerritbot added a project: Patch-For-Review.Aug 18 2022, 7:07 AM
Tgr added a comment.Aug 18 2022, 7:08 AM

What a mess. The test calls Language::factory, which needs NamespaceInfo, which tries to initialize a bunch of configuration settings. Somehow something inside new ServiceOptions( NamespaceInfo::CONSTRUCTOR_OPTIONS, $services->getMainConfig() ) triggers session handling (no idea how that would happen, and the stack trace doesn't tell anything about that). When exiting the method, the session destructor is called. The session is dirty (which I think shouldn't happen at this point, but then this is a unit test so who knows how many components behave in unusual ways) and validates the user's name, which requires UserNameUtils, which needs the content language, which would be created by the language factory.

Why does that standard-looking ServiceOptions call trigger session handling, when a pretty much identical call in the LanguageFactory wiring doesn't? And what does any of it have to with the GrowthExperiments extension? Seems mystical to me.

Tgr added a comment.EditedAug 18 2022, 7:12 AM

Also, why is this only affecting a single testcase in a single test file? It doesn't seem to be doing anything special, and plenty of other tests call Language::factory without breaking.

Urbanecm_WMF mentioned this in T315516: [regression-wmf.25] mobile: Structured Add link task displays "Page is protected, abandoning structured task" for non-protected pages.Aug 18 2022, 7:52 AM
gerritbot added a comment.Aug 18 2022, 8:34 AM

Change 824293 abandoned by Gergő Tisza:

[mediawiki/extensions/GrowthExperiments@master] [DNM] CI test

Reason:

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

Lydia_Pintscher added a project: wmde-wikidata-tech.Aug 18 2022, 9:08 AM
Urbanecm_WMF added a parent task: T315516: [regression-wmf.25] mobile: Structured Add link task displays "Page is protected, abandoning structured task" for non-protected pages.Aug 18 2022, 9:26 PM
Urbanecm_WMF added a parent task: T312842: [[MediaWiki:Growthexperiments-manage-mentors-remove-mentor-pretext/nl]] translation issue.
Urbanecm_WMF mentioned this in T312842: [[MediaWiki:Growthexperiments-manage-mentors-remove-mentor-pretext/nl]] translation issue.
Urbanecm_WMF added a parent task: T314414: Grant "enrollasmentor" right automatically according to community's specifications.Aug 18 2022, 9:28 PM
Urbanecm_WMF mentioned this in T314414: Grant "enrollasmentor" right automatically according to community's specifications.

I added tasks with patches blocked by this issue as parent tasks of this one, so this is tracked.

Tgr removed a project: Patch-For-Review.Aug 19 2022, 12:32 AM
Tgr added a project: ci-test-error (WMF-deployed Build Failure).Aug 19 2022, 12:54 AM
Tgr added a comment.Aug 19 2022, 12:57 AM

I guess the nice thing to would be to properly dependency-inject UserNameInfo into SessionBackend, so what dependency loading order we end with does not depend on when session saving happens to be triggered.

WMDE-leszek subscribed.Aug 19 2022, 10:14 AM
kostajh triaged this task as High priority.Aug 22 2022, 8:56 AM
kostajh edited projects, added Growth-Team (Sprint 0 (Growth Team)); removed Growth-Team.
kostajh subscribed.

Blocking patches for GrowthExperiments so upping the priority for our team.

gerritbot added a comment.Aug 22 2022, 10:56 AM

Change 825297 had a related patch set uploaded (by Kosta Harlan; author: Kosta Harlan):

[mediawiki/extensions/Wikibase@master] EntityLinkFormatterFactoryTest: Use mock for language

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

gerritbot added a project: Patch-For-Review.Aug 22 2022, 10:56 AM
kostajh claimed this task.Aug 22 2022, 12:00 PM
kostajh moved this task from Incoming to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.
WMDE-leszek added a comment.Aug 22 2022, 2:42 PM

@kostajh shouldn't it even be Unbreak Now? As it essentially blocks your work as I understand it?
Thanks for the Wikibase patch, asking WMDE engineers to have a look today if still feasible. Feel free to merge it by your team if we do not get to it quickly enough. We can always revert and redo it, no reason to block your team.

kostajh raised the priority of this task from High to Unbreak Now!.Aug 22 2022, 3:21 PM
In T315489#8174478, @WMDE-leszek wrote:

@kostajh shouldn't it even be Unbreak Now? As it essentially blocks your work as I understand it?

Yeah, I think that's right.

Thanks for the Wikibase patch, asking WMDE engineers to have a look today if still feasible. Feel free to merge it by your team if we do not get to it quickly enough. We can always revert and redo it, no reason to block your team.

Thanks! We might merge it and a more comprehensive fix could come later from one of our teams.

Urbanecm_WMF removed a parent task: T315516: [regression-wmf.25] mobile: Structured Add link task displays "Page is protected, abandoning structured task" for non-protected pages.Aug 22 2022, 3:28 PM
gerritbot added a comment.Aug 22 2022, 5:18 PM

Change 825297 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] EntityLinkFormatterFactoryTest: Use mock for language

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

Urbanecm_WMF lowered the priority of this task from Unbreak Now! to Needs Triage.Aug 22 2022, 5:38 PM

Bandaid fix was merged, and CI is no longer broken: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/GrowthExperiments/+/819657. Resetting priority.

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.26; 2022-08-22).Aug 22 2022, 6:00 PM
kostajh removed kostajh as the assignee of this task.Aug 22 2022, 6:56 PM
Tgr added a comment.Sep 6 2022, 2:06 AM

A new, similar CI break:

18:24:47 1) Wikibase\Repo\Tests\Hooks\Formatters\EntityLinkFormatterFactoryTest::testGivenNotArrayOfCallbacks_throwsException with data set #0 (array(null))
18:24:47 Wikimedia\Services\RecursiveServiceDependencyException: Recursive service instantiation: Circular dependency when creating service! JobQueueGroupFactory -> MainWANObjectCache -> UserNameUtils -> TitleParser -> _MediaWikiTitleCodec -> InterwikiLookup -> MainWANObjectCache

The dependency circle is different but it is again caused by Session::__destruct() being invoked at some random place (here, apparently, which makes even less sense than the previous instance).

Urbanecm_WMF added a comment.Sep 6 2022, 6:01 AM
In T315489#8212560, @Tgr wrote:

A new, similar CI break:

18:24:47 1) Wikibase\Repo\Tests\Hooks\Formatters\EntityLinkFormatterFactoryTest::testGivenNotArrayOfCallbacks_throwsException with data set #0 (array(null))
18:24:47 Wikimedia\Services\RecursiveServiceDependencyException: Recursive service instantiation: Circular dependency when creating service! JobQueueGroupFactory -> MainWANObjectCache -> UserNameUtils -> TitleParser -> _MediaWikiTitleCodec -> InterwikiLookup -> MainWANObjectCache

The dependency circle is different but it is again caused by Session::__destruct() being invoked at some random place (here, apparently, which makes even less sense than the previous instance).

Attaching CI's output and patch (@Tgr, please correct me if this is incorrect).

ItamarWMDE moved this task from Incoming to Radar on the wmde-wikidata-tech board.Sep 12 2022, 4:19 PM
kostajh edited projects, added Growth-Team; removed Growth-Team (Sprint 0 (Growth Team)).Sep 16 2022, 12:35 PM

Not happening anymore, as far as I know, so removing from our current sprint.

kostajh moved this task from Inbox to Triaged on the Growth-Team board.Sep 16 2022, 12:35 PM
Lucas_Werkmeister_WMDE mentioned this in T320952: Wikibase\Repo\Tests\Hooks\OutputPageBeforeHTMLHookHandlerTest::testGivenExternallyRenderedMarkup_usesRespectivePlaceholderExpander Wikimedia\Services\RecursiveServiceDependencyException: Recursive service instantiation: Circular dependency when creating service!.Oct 17 2022, 4:27 PM
Lucas_Werkmeister_WMDE subscribed.Oct 18 2022, 2:54 PM

I think we can probably close this task? The specific circular service error doesn’t happen at the moment as far as I’m aware, and while it occasionally (and seemingly almost at random) pops up again, I don’t think having this task open in various backlogs is very useful.

(For anyone else encountering these errors: as far as I can tell, the successful mitigation strategy seems to be to look at related test code and find some “integrated” MediaWiki objects that can be replaced by simpler mocks.)

kostajh closed this task as Resolved.Oct 18 2022, 3:22 PM
kostajh claimed this task.

Thanks @Lucas_Werkmeister_WMDE!

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