In T400113#11042622 I suggested to a DeferredUpdate to ensure that the session gets saved at the end of the request. We already have mechanisms in place that prevent deferred updates from piling up in tests and job runners, so we'd automatically benefit from that.

Hello @Tgr , I’ve been trying to come up with a scenario where session::save is called from destruct, but I haven't been able to.

Before Destruct gets called, shutdown is called to set $this->shutdown to true, which in turn influences the session save in deregisterSession.
It might also be useful to point out that we currently also do session::save in
MediaWikiEntryPoint::commitMainTransaction().

rMW5f010ef32ed2: session: Move SessionManager::getGlobalSession() to WebRequest (which is getting deployed this week) reduced the number of Session objects created and destroyed a lot. Before that, saves at random times were common. Not sure how much of a problem it is now; the Session object for the global session is stored in the WebRequest object in RequestContext, which will be around until the end of the request, so that Session won't be destructed before shutdown. You'd probably need to look for code which constructs a FauxRequest and then fetches a session from that request. Or for non-WebRequest-related SessionManager::getSession* calls. Neither of those might happen much.

Maybe you could start by logging it - we are already logging saves and using $sessionWriteReason to track what triggered it, but we are not setting it in the destructor or the shutdown handler, so you could add that and then after a few days in production check how often we have destructor-triggered saves. You could also temporarily add a stack trace to conveniently get reproduction steps.

Change #1175138 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/core@master] SessionManager: Temporarily add debug logging for session saves during shutdown or in destructor

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

The dashboard for the session write logs is here.

Change #1175138 merged by jenkins-bot:

[mediawiki/core@master] SessionManager: Add $sessionWriteReason to shutdown and when saves are triggered from the destructor

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

Change #1175574 had a related patch set uploaded (by Bartosz Dziewoński; author: Hokwelum):

[mediawiki/core@wmf/1.45.0-wmf.12] SessionManager: Add $sessionWriteReason to shutdown and when saves are triggered from the destructor

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

Change #1175574 merged by jenkins-bot:

[mediawiki/core@wmf/1.45.0-wmf.12] SessionManager: Add $sessionWriteReason to shutdown and when saves are triggered from the destructor

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

Mentioned in SAL (#wikimedia-operations) [2025-08-04T20:55:37Z] <cjming@deploy1003> Started scap sync-world: Backport for [[gerrit:1175511|Clear edit count when unattaching local users for rename (T313900)]], [[gerrit:1175512|fixStuckGlobalRename: Fix using actor_id from the wrong wiki (T398177)]], [[gerrit:1175574|SessionManager: Add $sessionWriteReason to shutdown and when saves are triggered from the destructor (T400249)]]

Mentioned in SAL (#wikimedia-operations) [2025-08-04T20:57:14Z] <cjming@deploy1003> matmarex, cjming: Backport for [[gerrit:1175511|Clear edit count when unattaching local users for rename (T313900)]], [[gerrit:1175512|fixStuckGlobalRename: Fix using actor_id from the wrong wiki (T398177)]], [[gerrit:1175574|SessionManager: Add $sessionWriteReason to shutdown and when saves are triggered from the destructor (T400249)]] synced to the testservers (see https://wikitech.wikimedia.org/w

Mentioned in SAL (#wikimedia-operations) [2025-08-04T21:03:13Z] <cjming@deploy1003> Finished scap sync-world: Backport for [[gerrit:1175511|Clear edit count when unattaching local users for rename (T313900)]], [[gerrit:1175512|fixStuckGlobalRename: Fix using actor_id from the wrong wiki (T398177)]], [[gerrit:1175574|SessionManager: Add $sessionWriteReason to shutdown and when saves are triggered from the destructor (T400249)]] (duration: 07m 36s)

In T400249#11054442, @Tgr wrote:

The dashboard for the session write logs is here.

Thanks for the link. First log entries are in, and there are no __destruct calls logged as yet (as expected), but there are some shutdown calls (surprising to me): https://logstash.wikimedia.org/goto/a0bbe70ac793f3ab7a0dc6fdce44148e. They are all for completely normal logged-in page views, where I would expect commitMainTransaction() to be called first, so I don't know how they can happen. One idea is that something is causing these writes after that point, but I'm not sure if that's possible.

(Even if we need to keep the code in both shutdown() and commitMainTransaction(), having two places where sessions can be saved would still be much better than infinitely many places.)

In T400249#11059388, @matmarex wrote:

there are no __destruct calls logged as yet

Nice, I didn't expect there to be zero, but it seems like your patch got rid of that path entirely then.
@Hokwelum we might still want to do this task because of __destruct calls in unit tests being annoying (per T400950), but it looks like the only way to trigger it is manually via custom code. Something like

$r = new \MediaWiki\Request\WebRequest();
$s = $r->getSession();
unset( $s );

should do the trick I think.

[shutdown calls] are all for completely normal logged-in page views, where I would expect commitMainTransaction() to be called first, so I don't know how they can happen. One idea is that something is causing these writes after that point, but I'm not sure if that's possible.

All the entries have dataDirty: true, metaDirty: false, they are all logged-in users, and almost all (though not all) have referers indicating the user just arrived from a different domain. And it's happening on non-SUL wikis as well (well, happened exactly once, but those wikis don't get many requests). Not sure what to make of it. I looked at a few random reqIds, and it's always the only log record for that request (although session writes are sampled, so that doesn't mean there weren't other writes).

Thanks @Tgr! I'll do that and see how it goes!

Is it safe to remove the test referencing the destructor function for the test files, or do we move that to the test to make the test pass? CC @Tgr @matmarex

Which test do you mean? At a glance, I don't see anything destructor-related in SessionBackendTest.
In general, if the test checks some behavior that's still relevant after the refactoring, it should ideally be kept, otherwise not.

Change #1176542 had a related patch set uploaded (by Hokwelum; author: Hokwelum):

[mediawiki/core@master] SessionManager: Remove session saves in session destructor

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

Change #1176542 merged by jenkins-bot:

[mediawiki/core@master] SessionManager: Remove session saves in session destructor

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