Page MenuHomePhabricator

Board moves in Flow not working
Closed, ResolvedPublic

Description

Happens locally.

Also tested on Beta with both Flow namespace -> Flow namespace and arbitrary page -> arbitrary page (with flow-create-board).

Details

Related Gerrit Patches:

Event Timeline

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptFeb 23 2016, 1:47 AM
Restricted Application added subscribers: StudiesWorld, Aklapper. · View Herald Transcript

A warning on mismatched transactions was changed to an error: rMWef44f4308edb: Upgrade mismatched commit() warnings to exceptions

We might be able to use startAtomic.

I have a WIP to change it to startAtomic/endAtomic (making sure to use the same fname). However, it's not quite there yet.

Most places in production will use a separate database connection (and thus atomic stack) for the Flow database (which includes BoardMover).

However, it needs to also work when the Flow tables use the same connection (e.g. local, Beta, and officewiki).

The problem is that the TitleMove hook runs outside the atomic (a little before), whereas TitleMoveCompleting runs inside (immediately before endAtomic). So the push/pop is not in the right order.

TitleMoveComplete (which it currently uses) also does not work, because by then the transaction is fully committed, so the endAtomic does not make sense and fails with "Got COMMIT while atomic sections Flow\BoardMover are still open".

Most obvious approach I can think of is add TitleMoveStarting after the MovePage::move startAtomic, but open to suggestions.

Change 272665 had a related patch set uploaded (by Mattflaschen):
DO NOT MERGE: Show not-yet-working board move change

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

A TitleMoveStarting hook after the startAtomic seems same and the rest of that patch also looks good.

I'm sure I'm overlooking something obvious, but we doesn't the current order of operations also work?
TitleMove (where Flow hooks in) starts before MovePage starts the transaction, and TitleMoveComplete (where Flow hooks in again) runs after it has completed.
Stacking order should be fine:

  • Flow starts transaction
  • MovePage starts (nested) transaction
  • MovePage completes (nested) transaction
  • Flow completes transaction

Because there is an attempt to commit the Flow transaction with 'flush' before TitleMoveComplete gets to run. That attempt now triggers an exception.

Mediawiki::preOutputCommit triggers (via some intermediary functions) LoadBalancer->commitMasterChanges, which calls commit with 'flush'. That now triggers an exception if the open transaction is an explicit transaction, as it is here.

TitleMoveComplete is inside an onTransactionIdle callback (which is called e.g. from Database::commit), but that never gets to run due to the exception.

[f8084c75] /w/index.php?title=Special:MovePage&action=submit DBUnexpectedError from line 2659 of /vagrant/mediawiki/includes/db/Database.php: MediaWiki::preOutputCommit: Flushing an explicit transaction, getting out of sync!

Backtrace:

#0 /vagrant/mediawiki/includes/db/loadbalancer/LoadBalancer.php(1068): DatabaseBase->commit(string, string)
#1 [internal function]: LoadBalancer->commitMasterChanges(string)
#2 /vagrant/mediawiki/includes/db/loadbalancer/LBFactory.php(206): call_user_func_array(array, array)
#3 [internal function]: LBFactory->{closure}(LoadBalancer, string, array)
#4 /vagrant/mediawiki/includes/db/loadbalancer/LBFactorySimple.php(154): call_user_func_array(Closure, array)
#5 /vagrant/mediawiki/includes/db/loadbalancer/LBFactory.php(209): LBFactorySimple->forEachLB(Closure, array)
#6 /vagrant/mediawiki/includes/db/loadbalancer/LBFactory.php(251): LBFactory->forEachLBCallMethod(string, array)
#7 /vagrant/mediawiki/includes/MediaWiki.php(561): LBFactory->commitMasterChanges(string, array)
#8 /vagrant/mediawiki/includes/MediaWiki.php(539): MediaWiki::preOutputCommit(RequestContext)
#9 /vagrant/mediawiki/includes/MediaWiki.php(743): MediaWiki->doPreOutputCommit()
#10 /vagrant/mediawiki/includes/MediaWiki.php(519): MediaWiki->main()
#11 /vagrant/mediawiki/index.php(43): MediaWiki->run()
#12 /var/www/w/index.php(5): require(string)
#13 {main}

Change 273143 had a related patch set uploaded (by Mattflaschen):
Add TitleMoveStarting, mirroring TitleMoveCompleting

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

Change 273143 merged by jenkins-bot:
Add TitleMoveStarting, mirroring TitleMoveCompleting

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

Change 272665 merged by jenkins-bot:
Fix board move DB issue using new hook TitleMoveStarting

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

Change 273535 had a related patch set uploaded (by Mattflaschen):
Add TitleMoveStarting, mirroring TitleMoveCompleting

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

Change 273536 had a related patch set uploaded (by Mattflaschen):
Fix board move DB issue using new hook TitleMoveStarting

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

Change 273535 merged by jenkins-bot:
Add TitleMoveStarting, mirroring TitleMoveCompleting

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

Change 273536 merged by jenkins-bot:
Fix board move DB issue using new hook TitleMoveStarting

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

It turns out this worked without the fix on wikis that use a separate flowdb. I don't really understand why. I would still have expected it to fail by committing all open explicit transactions (which Flow had one of) implicitly.

I checked and:

$factory->forEachLB( function ( LoadBalancer $lb ) { $lb->forEachOpenConnection( function ( IDatabase $db ) { echo $db->getWikiID(); echo "\n"; } ); } );

*does* include flowdb.

That foreach is a much-simplified version of the structure used by MediaWiki->preOutputCommit/$factory->commitMasterChanges.

However, I confirmed it's fixed on both MediaWiki.org and officewiki. I did not test on officewiki before the fix, but we have a specific report from there (I also did that move)

Etonkovidova added a subscriber: Etonkovidova.EditedMar 2 2016, 12:25 AM

Checked in betalabs:

  • moving Flow pages is available only for users with Flow bot rights

However, some current shortcomings such as a confusing feedback to a user.

  • There is no redirect is created - when moving Flow page the redirect option is unavailable (a know issue).
  • After the page has been moved, the page says that:

This page has been deleted. The deletion and move log for the page are provided below for reference.

The next line though clearly states that the page was moved:
(del/undel) 00:09, 2 March 2016 Etonkovidova (talk | contribs | block) moved page Talk:ET15 to Wikipedia talk:ET15 3 without leaving a redirect (test) (revert)

  • there is no link to Move log, but the link to the Delete log is present where the moved page is not found (of course)

This is standard behavior for all pages, not just Flow, when moving without leaving a redirect.

For a regular page, you can uncheck "Leave a redirect behind" if you have the right (e.g. admins).

You can think of it as "I moved it and then deleted the redirect" conceptually.

Non-Flow example: http://en.wikipedia.beta.wmflabs.org/w/index.php?title=Move_without_redirect_test&redirect=no

Not the most intuitive (though there is a link to the destination), but if we want to change it, we need to do so in core.

jmatazzoni closed this task as Resolved.Mar 23 2016, 8:43 PM