[00:00:30] according to https://logstash.wikimedia.org/app/kibana#/doc/logstash-*/logstash-2019.04.16/mediawiki?id=AWooZdTFm2VjIW06nmOT someone had a lot of deleted edits [00:08:45] tgr: how can you tell there were lots of deleted edits? Maybe I just don't know how to read that output... [00:09:18] the update on the archive table is the one that took long [00:10:07] ah. ok [00:10:37] this says there are none? -- https://wikitech.wikimedia.org/wiki/Special:DeletedContributions/Gwicke [00:10:54] hmm.. but maybe that's a partial move? [00:11:07] there are a lot at https://wikitech.wikimedia.org/wiki/Special:DeletedContributions/GWicke (the target user) [00:11:24] well...not a lot really [00:11:27] a few [00:12:09] I wonder if that [00:12:10] in theory the transaction got rolled back so the edits shouldn't be in an inbetween state [00:12:31] and this is not fired from the jobqueue so it should not have been retried [00:12:37] I wonder if there is a missing index on the labswiki db? [00:13:03] missing index is something that has happened before there [00:16:02] seems about right [00:16:13] no index on ar_user [00:16:32] or maybe we are using ar_actor now? [00:18:36] no, if I read the code correctly (which is a big if with UserMerge) it uses ar_user [00:24:05] not sure what's going on there because other DBs don't seem to have ar_user indexes either [00:27:23] bd808: well, the query took ~5 sec which is not tragic, so maybe just raise the time limit temporarily for wikitech? [00:27:54] probably easier than figuring out how UserMerge is supposed to work, given that it has not been used for years [00:29:23] yeah, if we can just dial up the timeout that seems easiest [00:29:57] I can do that with local hacks too to test [00:33:15] bd808: that would be $wgMaxUserDBWriteDuration [00:33:52] * bd808 makes note [00:40:21] Note that some of the transaction warnings also measure time spent in PHP between writes that hold open the same transaction. [00:40:34] I don't know if that's the case here, but something to keep in mind :) [00:49:08] looks like all the merge writes that need to happen together already have an atomic section, retry seems fine. In that, it's fine for the editcount merge to be committed and then fail elsewhere, the admin can re-try it at that point and it'll do less work each time. I guess something is interferring with the transaction globally causing it to be one big one. [00:55:11] \o/ a second run of merging Gwicke into GWicke worked! [01:02:00] * bd808 decides to end his work day on this merge highnote [15:13:39] <_joe_> what more is needed to get a +2 on https://gerrit.wikimedia.org/r/c/mediawiki/core/+/502546 [15:13:54] <_joe_> then backport it so that I can do some tests before the week ends? [15:14:11] Got a preferred online beer delivery service? [15:14:45] <_joe_> Reedy: most people here owe me more beers than I'll ever collect [15:14:49] Aha [15:14:56] I think hit rebase and merge [15:14:59] I can do that now [15:15:00] <_joe_> you're exmpted because you bring chocolate :) [15:15:36] <_joe_> jokes, aside, if you feel confident with merging it, that's appreciated :) [15:15:50] <_joe_> without configuration is supposed to be a complete noop :) [15:15:54] +1+1=+2 [15:17:01] Actually, one very minor thing. I'll amend [15:18:13] * Reedy adds a @since tag [15:21:46] <_joe_> heh right it got swallowed by refactors :P [15:22:05] yeah, NBD [15:22:46] _joe_: only backporting to .1 going out this week? [15:23:16] <_joe_> it's enough to get it on mwmaint so I can create a series of tests [15:23:21] <_joe_> that would unblock me [15:23:45] https://tools.wmflabs.org/versions/ [15:23:59] I guess you don't really care about which wiki you run it against... [15:24:05] As long as you know which you can [15:24:05] <_joe_> exactly [15:24:10] <_joe_> testwiki is ok [15:24:28] <_joe_> actually it makes me feel less uneasy if I decide to merge the config change as well [15:25:07] <_joe_> but I guess I'll have to wait for tomorrow's swat to merge the backport and deploy? [15:25:22] There's a swat in half an hour... [15:25:25] Was just going to do it now [15:25:45] When it lands in master, you can test on beta too [15:25:56] <_joe_> right [16:15:46] Do we have a preference on using anonymous user or IP user? It seems like this RFC https://phabricator.wikimedia.org/T133452 would imply the former, but I think Krinkle mentioned the opposite to dmaza [16:17:18] "IP user" has never been a term used in the software. "Anonymous user" is the term used the most. In more recent history, this has been moving instead towards "Unregistered user" [16:17:23] the latter is used on recent changes, for example. [16:17:48] historically MediaWiki used to mostly say "anonymous", and you'll find a lot of code that talks about "anons", but i think recently we have been trying to use "IP" since they are not really anonymous. [16:18:06] or "unregistered", yeah. that may be a better term [16:18:14] Yeah, in speech "IP" has been used a fair bit. But not in code or UIs. [16:18:27] davidwbarratt: IP user is a bit more accurate, since "anonymous" users aren't all that anonymous since their IPs are published. [16:18:50] "unregistered" may not be entirely accurate either, since someone might be registered but logged out. [16:19:20] And "non-logged-in users" is just awkward. ;) [16:22:01] so for example.. $isIpUser would be the new equivalent for the $isAnon all over the place? I'm with anomie here that unregistered is not accurate [16:25:56] it's anonymous _to the software_ not to the world [16:26:41] theoretically a user not being logged in and them being identified by their IP address are different things [16:26:54] tgr++ [16:27:22] possibly soon in practice as well, see T133452 [16:27:22] T133452: Create temporary accounts for anonymous editors - https://phabricator.wikimedia.org/T133452 [16:27:45] tgr: That's the exact task that started this discussion. [16:27:50] Glancing through that RFC, it seems some of the potential issues like those raised in https://phabricator.wikimedia.org/T133452#4313877 could be mentioned in the description. [16:28:37] right, sorry [16:28:52] so I'd avoid "IP user" because of that [16:30:01] (technically, all users are pseudonymous, not anonymous like on services which don't give you the same name for two successive comments) [16:30:18] For now "IP user" is reasonably accurate. The fact that that RFC wants to change that doesn't mean it's inaccurate now, it just means that in the future we might need to change to something like "temp user" instead. [16:31:30] I did add a mention of anti-abuse tooling to the description of that task, btw, but everyone should feel free to expand [16:32:14] In code identifiers, use "anon" because that's how our methods and software refer to the user type. How it is implemented and what user interfaces shown are different. But for consistency I'd recommend sticking with "anon" there unless and until we agree on a new term and are prepared to start renaming methods. [16:32:29] In user interfaces, for better or worse, newer labels now use "Unregistered", some older labels use "Anonymous users" still. [16:40:58] thanks! [16:48:47] Reedy, James_F: Don't know if you guys saw https://phabricator.wikimedia.org/T221257 [16:49:42] Yeah, just now. [16:52:07] sorry, I have been having trouble keeping up with phab/gerrit notifications recently and I know others do too [16:52:22] I'm guessing it's more likely the first [16:52:30] the hotp one is the same code we had before [16:52:44] plus we use TOTP rather than HOTP [16:53:04] it's also possible this isn't in the OATHAuth extension at all but rather something in core or CentralAuth or...? [16:54:29] All possible [16:55:05] I don't think the TOTP vs HOTP makes much difference [16:55:10] $results = HOTP::generateByTimeWindow( [16:55:13] if ( $window > $lastWindow && $result->toHOTP( 6 ) === $token ) { [16:55:35] I can't do anything about it just yet, I'm about to be kicked out of where I am currently [16:56:48] Actually, I can stage some reverts [16:56:51] We can ignore the vendor commits for now [16:56:53] Reedy, James_F, Krenair: I find that it works on my local test wiki with bac94daedba^ and fails with bac94daedba itself. [16:57:14] Interesting [16:57:21] Did we make some changes to that library that I missed? [16:59:04] should be easy enough to sort looking at a diff [16:59:19] But we might aswell just revert that out for now [16:59:57] https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/OATHAuth/+/504614/ is the revert on .1 [17:02:53] Back in a couple of hours [17:03:07] I'll deploy that now. [17:04:50] Reedy, James_F, Krenair: Looks like upstream didn't get the change to hotp.php in 1f7dae86fb, so the changes to OATHUser.php in that patch aren't getting the result they expect. [17:05:50] anomie: Ah, fun. [18:55:31] anomie: Cheers for narrowing it down. I can upstream it :) [18:56:48] James_F: Thanks for deploying joe's change too [18:57:26] Reedy: Of course. Though in retrospect, 3.25 hours of continuous code deployment is not fun. [18:57:35] James_F: Been there, done that... [18:57:44] Or, up 12 hours doing a deploy and fixing massive fallout [18:58:03] Reedy: You're not the benchmark against which we should try to measure ourselves. ;-) [18:58:32] <_joe_> anomie: if I try to dump the full unicode table, json_encode fails [18:59:53] <_joe_> I will test if it's a depth issue (the array being too large) or the inability of json's implementation in hhvm to dump a json for higher-plane unicode chars [19:00:20] _joe_: Hmm. Can you insert a line to print the string from json_last_error_msg() to see what it's complaining about? Although probably the solution is going to turn out to just be switching to StaticArrayWriter instead of using JSON. [19:00:38] <_joe_> sure [19:01:04] <_joe_> it's less diffable manually, but yeah I guess so [19:01:34] <_joe_> not now though, I'm around since 15 hours :) I'll let you know what I find out tomorrow [19:02:11] Sounds good [19:04:54] Hi everyone, some closure is giving me hard times. Can't figure out ATM why Revision is passed instead of RevisionRecord, patch here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/+/504589 [19:05:04] Not sure exactly what I'm doing wrong [19:05:29] any help will be very much appreciated, thanks [19:06:07] _joe_: Oh, I can reproduce it locally. We need to have the script skip the surrogate codepoints, U+D800 to U+DFFF. [19:06:08] D800: Fix bug when some of aqs requests return 202 and others 404 - https://phabricator.wikimedia.org/D800 [19:12:11] xSavitar: Looks like it's coming in to EchoHooks::onPageContentSaveComplete(), the hook is passed a Revision rather than a RevisionRecord as $revision. You should use $revision->getRevisionRecord() in that hook function to get the RevisionRecord you need for the call to EchoDiscussionParser::generateEventsForRevision(). [19:17:28] James_F: Patches up [19:17:39] anomie: Thanks a lot, I'll to think in that direction and see how I can fix it, thanks again [19:20:07] Reedy: Yup, was just looking. [19:41:37] anomie: I'm not sure but ->getRevisionRecord() doesn't exist. [19:41:48] Maybe I'm missing something [19:42:02] That is within the hook function [19:42:26] /** [19:42:26] * @return RevisionRecord [19:42:26] */ [19:42:26] public function getRevisionRecord() { [19:42:26] return $this->mRecord; [19:42:27] } [19:43:26] xSavitar: It's a method on the Revision class. [19:53:28] Reedy, anomie, yeah, was missing it, fixed it now [19:58:45] So my PHPDoc param was messing up with my life :( [19:59:12] I missed the fact that the hook function takes Revision only [19:59:19] anomie: Thanks :) [20:01:54] Reedy: Revision migrations are fun :D [21:27:02] James_F: Your wikitech accounts have been merged. "Jforrester" is the canonical username [21:28:01] bd808: Gosh. Thanks! [21:34:39] bd808: Interestingly the password is the JForrester password but the 2FA token is the Jforrester one. 🤷🏽‍♂️ [21:35:58] I.... wow [21:36:03] well, that's the correct way to prove that you own both accounts [21:37:25] lol, I guess OATHAuth doesn't have a mergeaccount hook [21:37:47] Not that it really should... Hard to resolve the conflict [21:37:52] But nice not to leave dangling rows around? [21:38:27] ugh. I guess that will be another bug to fix [21:38:31] * James_F coughs. [21:38:59] just in the db on wikitech James_F :) I'm not touching that extension [21:39:27] Yeah, let's just wait to move the wiki be an SUL wiki. [21:39:28] even if it did, wouldn't the sane behavior be for the rename target to win? [21:39:39] the WTF is that the password gets overridden [21:39:57] Then we can do "real" finalisation of SUL with only one local user table across the cluster, and then we can drop all the local stuff. Simples. [21:40:16] of course that's an LDAP password so probably UserMerge has no control over it [21:40:16] That's probably the most likely way we'll "clean up" our DBs. [21:40:48] tgr: True. I was moderately surprised that I needed the "other" password. [21:41:33] it probably means there's some duplication at the LDAP level as well [21:41:44] * James_F nods. [21:41:55] which I imagine has to be cleaned up manually [21:42:25] as for SUL, the personal + WMF account thing makes that a bit complicated [21:42:35] I'd just go with OAuthAuthentication, personally [22:02:38] There should only be one LDAP account. And its password should be "the" password. [22:03:30] Thankfully (intentionally), my wikitech wiki account name doesn't clash with either of my SUL wiki accounts' names. [23:29:10] bd808: OK, can I disable UserMerge from wikitech again? [23:29:20] James_F: yes! [23:29:23] Excellent. [23:29:25] Thank you! [23:30:57] thank you for doing the needful to turn it on. I would have probably never actually done that and just blocked the bogus accounts instead (which had its own set of challenges)