In T352913#9404831, @Tgr wrote:

That sounds bad. Session data should be written to MediaWiki's session backend, not files - the default PHP session handler should be disabled as soon as Setup.php runs.

In T352913#9696109, @Krinkle wrote:

It appears to me that the immediate failure here is that the session directory at /var/lib/php/sessions is not writable (ie it doesn't exist and the "nobody" user in CI isn't allowed to create it).

The error comes from \Wikimedia\PhpSessionSerializer::decode - it seems the serialization format may have changed.

Patch updated.

@Yaron_Koren Yeah, this could also be integrated with the special page, good idea!

Something that came up recently (in the context of lol.fandom.com) was the ability to add rate limits to the Cargo query API, since there have been a few cases of people accidentally issuing too many queries in too short a timeframe. I made a patch for that.

It seems that kqueue is usable on macOS for at least wall-time profiling and simple timers—and unlike other Apple-specific APIs, should be available on other OSes too like BSD, which should facilitate testing. I cobbled together a patch leveraging this that seems to work on macOS.

This should no longer be necessary, as the mentioned tests have been adjusted to not need such functionality.

In T257703#9417729, @Tgr wrote:

Making session initialization happen at a predictable point in time was one of the design goals of the current session system, and IMO it resulted in much easier to reason about behavior, so it would be nice to keep that.

Having something in place for REST that would allow handlers to specify whether they depend on a session SGTM irregardless—but we should consider the case where a handler unknowingly ends up depending on the session because of some deep dependency.

In T257703#9417249, @Tgr wrote:

That would mean the session is not available during hooks that run between the beginning of Setup.php and wherever session initialization would happen. Checking for registered or temp users would return the wrong result, messages would be rendered in the wrong language etc. Currently the only hook that runs before session initialization is MediaWikiServices and there the services are not set up yet so it's very unlikely that something session-dependent would be attempted.

The opposite approach would be to have some sort of URL routing step very early in Setup.php or the entry point which would determine whether this request needs a session. I think that's how a more modern web application would work: determining which controller handles the request is one of the first steps. MediaWiki doesn't really have controllers but it could be approximated for REST URLs.

In T257703#9416780, @Seb35 wrote:

In T257703#8424310, @TK-999 wrote:

More broadly, we may need to revisit the practice of eagerly setting up the session itself in Setup.php as well, since that itself may set cookies even if nothing attempts to interact with the session user during the lifetime of the request.

I had the same conclusion when analysing the related task T285210#9415537, and defering session management would make the warning Cookies set on {url} with Cache-Control "{cache-control}" more useful for developers to track actual code paths where both session is used and cache-control is declared as public.

At the same time, some session-independent API-like requests (API with action=query, index.php with action=raw, Wikibase’s Special:EntityData…) might also be marked as "should not use $wgUser" to extend a bit more the mechanism MW_NO_SESSION, and these would not set Set-Cookie if it don’t use $wgUser (and emit a log if they do).

In T302083#9353946, @Novem_Linguae wrote:

Adding @TK-999 since they wrote a patch for a similar issue in VE. Maybe they can provide some insights :)

What page(s) does this happen on reliably?

Thanks @Alex44019 and @cscott for raising this one. I remember discussing it with the team a while back, but the Parsoid extension API was considerably less fleshed out back then.

In T324207#9250941, @Daimona wrote:

Taint-check is fully under our control, but it's not causing this crash as per above. This is a bug in phan itself. But unfortunately I agree with the second part of your comment. Phan is all but actively maintained at the moment, and this is already causing us troubles (T325321 immediately comes to mind).

Might be worth to manually patch the affected plugin in CI—Phan seems to have been in development limbo for most of this year, there is very little activity both in terms of code changes and responding to issues.

Rebased this since it got a question about its status, just wanted to note per earlier discussions on the patch that T296023 probably needs to be resolved first to unblock this.

Perhaps one option could be to add a new method to WikiPageFactory that returns an optionally cached WikiPage instance for a given PageIdentity. That could then be used as a replacement without a performance cost.

Thanks for the CC.

Possibly. I think it's worth disabling the JIT (ie making sure that opcache.jit_buffer_size is unset or 0, which is the default) though just to rule it out as a cause—it's been known to cause oddball issues when it is enabled that simply do not occur when it is off, and its maintenance status in general is dubious.

Might be worth an upstream report if a minimal reproducer can be isolated.

@Clement_Goubert Yeah, we currently set a limit of 1 CPU per worker. We have not experimented with pinning.

We were consistently throttled until we set limits == FPM worker count. Per the description (and Dan Luu's insightful foray[1]) into the topic, I don't think there is much that can be done besides adjusting or removing the limits or tweaking the CFS period that k8s uses. Removing the limits is probably fine given that the size of the worker pool is a natural upper bound on concurrency with pm = static.

We've encountered spurious memcached timeouts (likely due to packet loss) with MW on k8s since forever. Are there any memcached errors logged as part of the same request?

Thanks, this is great!

I think this is not a PHP 8.0 specific issue, it'd likely happen on earlier versions as well. We just happened to notice it because we were on the lookout for core dumps while investigating unrelated JIT issues.

Thanks a lot, Tim :)- using your advice, I inspected a more recent dump and found that it was a request where PHP was exceeding its configured memory limit while it was parsing a rather complex page. Going by that, I managed to narrow it down to a consistent reproducer:

--TEST--
Memory limit exceeded during sandbox init
--INI--
memory_limit=2M
--FILE--
<?php
$buf = str_repeat('a', 1000000);
$sandboxes = [];
for ($i = 0; $i < 100; $i++) {
    $sandboxes[] = new LuaSandbox();
}
?>
--EXPECTF--
Fatal error: Allowed memory size of 2097152 bytes exhausted%s(tried to allocate %d bytes) in %s on line %d

Thank you, I forgot to circle back and close this task.

I wonder if OpenTelemetry metrics could be an alternative here.[1] T340551 makes it seem likely that the OTEL Collector will be deployed as an infrastructure component, so it could potentially be leveraged for exporting metrics in Prometheus format[2] while avoiding the use of APCu or a dedicated storage backend. What do you think?

Thanks! Sorry, it seems I missed one spot so I made a fast follow.

It seems that the libmustache library used by php-mustache for actual mustache rendering purposes does not yet support referencing section data in some cases, i.e. given data like

$data = [ 'foo' => 'bar' ];

then

{{#foo}}{{.}} should be bar {{/foo}}

won't render correctly. Vector 2022 heavily uses this, so I made https://github.com/jbboehr/libmustache/pull/25 to hopefully try and fix this.

Tagging Wikimedia-Hackathon-2023 to showcase a basic PoC created during this event.

That should be fine for now. After that, we could either:

• Configure PageForms to ignore Phan stubs in other extensions via a config stanza such as $cfg['exclude_file_regex'] = '@^../../extensions/Cargo/.phan/stubs/@';, or
• Reimplement the hooks change in Cargo without stubs. This will require people to have a checkout of the AdminLinks extension to run phan locally and will also need a corresponding integration/config patch as noted by Sam.

In T333181#8730071, @Yaron_Koren wrote:

Okay, that would work. And now I think I understand why Cargo has these class re-declarations: so that the validation will know that these classes exist, since Admin Links is not being loaded. But why do none of the other extensions that also hook in to Admin Links (https://www.mediawiki.org/wiki/Category:AdminLinks_extensions) need these class declarations? (I think.)

Sorry about that.

AIUI PHP's max_execution_time is a CPU-time limit, rather than a wall time limit. It also cannot interrupt pending queries.

And indeed it always seems to do so (at least on MW 1.40 CLI):

 % php maintenance/run.php shell.php
Psy Shell v0.11.12 (PHP 8.0.28 — cli) by Justin Hileman
> $services = MediaWiki\MediaWikiServices::getInstance();
= MediaWiki\MediaWikiServices {#26}

FWIW, on MW 1.39 it seems like the double parse may be avoided if the parser output doesn't depend on any revision/page specific bits (i.e. no {{REVISIONID}}) and friends, as generate-html does not actually appear to change whether the ParserOutput generated from wikitext has HTML or not.

Hi there, just wanted to note that the hydrawiki org is no longer used for any actively utilized extensions. Relevant extensions have been migrated to the Wikia org on GitHub under the same names.

Sorry about the delay in following up, I was busy with some other work.

@Umherirrender Thanks, that's very useful context. npm dedupe seems the best approach in this case indeed.

@Umherirrender One issue that I see is that grunt-stylelint explicitly warns consumers to depend on stylelint as well:

Note that this installs both grunt-stylelint and the stylelint tool itself, which is a peer dependency. If you do not explicitly depend on stylelint in your package.json file and do not have it available, grunt-stylelint will not work. Modern versions of npm will warn you of such unmet peer dependencies.

— https://github.com/wikimedia/grunt-stylelint/blob/de0b7b3ffe80acbfab2639984fa79c58d1b6a243/README.md#getting-started

One concern I had when I last thought about this was i18n support. Localization messages can currently be customized locally via MediaWiki:* pages, use a MediaWiki-specific plural/gender rule syntax, and may even support wikitext (if the caller so desires). Language fallbacks and conversion are also something configured and managed in MediaWiki. Outside of the most basic of logic-less templates that can afford taking in prerendered i18n messages as parameters, it seems likely to me that an SSR service would end up needing to render i18n messages dynamically - which opens up a whole new can of worms with regards to supporting the above that'd either require additional crosstalk with MediaWiki or extra service-side logic.

WikiPage instances are expensive to instantiate because initializing them always requires a DB query. If IContextSource::getWikiPage calls are to be replaced by instantiations via WikiPageFactory, then every extension that needs access to the WikiPage instance corresponding to the current page will trigger a new, effectively redundant, DB query. I think this cost needs to be considered here.

In T328503#8578510, @daniel wrote:

getNativeData was deprecated in 1.33. It's still used in XmlDumpWriter though:

		if ( $content instanceof TextContent ) {
			// HACK: For text based models, bypass the serialization step. This allows extensions (like Flow)
			// that use incompatible combinations of serialization format and content model.
			$data = $content->getNativeData();
		} else {
			$data = $content->serialize( $contentFormat );
		}

It seems to me like this should be fixed in Flow. getNativeData is broken by design, nothing should use it. Import/export should rely on serialize().

Anyway, if you have a Content object that extends TextContent but does not return a string from getNativeData(), this will explode.

In T328503#8576744, @Reedy wrote:

As the code is from rMWfdc3e9f9524d: Add support for xml dump schema 0.11, I suspect it's applicable all the way back to 1.34 :)

In T328503#8576741, @Reedy wrote:

Are you running with modifications? Line 571 is the strlen() - https://github.com/wikimedia/mediawiki/blame/REL1_37/includes/export/XmlDumpWriter.php#L571

Obviously 1.37 is unsupported, but clearly can still happen on master as the code is basically identical...

Yeah, should be fine now!

Yeah, should be good now! Sorry, I've been meaning to close the task after the changeset was merged but I forgot about it.

Could these extensions use ParserOutput's mechanism already? Not sure whether there's a benefit in using fields on Parser itself here.