Page MenuHomePhabricator

After leaving the mentorship and joining it again via the Homepage, the mentor constantly changes after the page is reloaded
Closed, ResolvedPublicBUG REPORT

Description

Summary of the issue

  1. On first load, Special:Homepage generates a random mentor for the visitor and stores it into database via the job queue (to avoid writes on GET).
  2. If the visitor refreshes Special:Homepage before the job completed, Special:Homepage assigns a new mentor, as it doesn't know which mentor was assigned previously.
  3. On each refresh, the mentor sees a different user assigned to them as a mentor, which is confusing and annoying.

Steps to replicate the issue (include links if applicable):

  • Quit mentorship from Newcomer homepage
  • Ask for a new mentor
  • Refresh the page, mentor will change each time

What happens?:
The mentor assigned changes each time we refresh the Newcomer homepage after having asked for a new mentor. It stops to change after some time.

What should have happened instead?:
Mentor shouldn't be reassigned each time we go to the Newcomer Homepage.

Software version (skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Escargot_rouge renamed this task from After quitting mentorship and joining again, mentor keeps changing for a little time to After quitting mentorship and joining again, mentor keeps changing for some time.Nov 18 2022, 2:06 PM
Escargot_rouge updated the task description. (Show Details)
Iniquity renamed this task from After quitting mentorship and joining again, mentor keeps changing for some time to After leaving the mentorship and joining it again via the Homepage, the mentor constantly changes after the page is reloaded.Nov 18 2022, 2:14 PM
Tgr added subscribers: Urbanecm_WMF, Tgr.

@Urbanecm_WMF for your attention. Sounds like the mentor can't be written to the DB so it gets re-randomized on every request.

Urbanecm_WMF triaged this task as High priority.

I'll check this. Thanks for reporting, @Escargot_rouge, for reproducing @Iniquity and for the ping @Tgr.

Moving to sprint as a reported bug on an actively maintained feature.

This reminds me of T275429: Homepage mentor is not stored persistently at Romanian Wikipedia.

The symptomps are fairly similar. With every reload, I see a setUserMentorDatabaseJob submitted, but it takes a while to propagate, resulting in the weird reload of mentees. The job queue probably takes a while to process it, or something.

I guess the cure we can implement in GE is to call WANObjectCache::set from MentorStore::setMentorForUser, instead of invalidating the cache and potentially relying on the job queue to do its job.

[...]
I guess the cure we can implement in GE is to call WANObjectCache::set from MentorStore::setMentorForUser, instead of invalidating the cache and potentially relying on the job queue to do its job.

Or not. The docstring for WANObjectCache::set explicitly say not to call it on source data change (which is our case):

Simply calling this method when source data changes is not valid because the changes do not replicate to the other WAN sites. In that case, delete() should be used instead.

Unfortunately, calling delete() is what we do now, and it results in the repeated changes (due to the job, DB sometimes takes a while to catch up). In theory, we could call delete(), followed by a set(), but MediaWiki's active/active now, and nothing guarantees us that the same DC processes all requests by any given user.

Urbanecm_WMF changed the task status from Open to In Progress.Nov 21 2022, 7:45 AM

Change 859613 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@master] Ignore duplicates in mentorship related jobs

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

Change 859613 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Ignore duplicates in mentorship related jobs

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

Summary of the issue

  1. On first load, Special:Homepage generates a random mentor for the visitor and stores it into database via the job queue (to avoid writes on GET).
  2. If the visitor refreshes Special:Homepage before the job completed, Special:Homepage assigns a new mentor, as it doesn't know which mentor was assigned previously.
  3. On each refresh, the mentor sees a different user assigned to them as a mentor, which is confusing and annoying.

The question is: How to ensure Special:Homepage is aware of the generated mentor even before the job stores it into the database?

It seems this can possibly be solved by proactively setting the newly-generated mentor's name in the WAN cache. That way, Special:Homepage can see the change immediately, and doesn't need to wait for the job to finish.

This, however, has several issues:

  1. WANObjectCache::set is documented to work only within a single DC. It is also explicitly documented as "do not use when the cache key value changes", which is our case here.
  2. WANObjectCache::set is documented as "do not use while also using getWithSetCallback" (also our case here)
  3. Due to the single DC issue described above, WANObjectCache::delete needs to be called as well, which would introduce a tombstone (and presumably, resulting in the set to be ignored).

@Krinkle Would you mind providing your opinion here about how this issue can be resolved? Thanks in advance for the help!

We discussed this with @Urbanecm_WMF, could think of three ways to handle it:

  • Use the main stash instead of the WAN cache. The mentor cache was switched to use WAN not so long ago in rEGRE9a35feab78f5: MentorStore: Stop using local cluster cache, it's a bit of a pain point that there is no shared interface for these so switching requires a bunch of boilerplate code changes.
  • Use a deferred instead of a job for writing to the DB. That's still a cross-DC DB write, but I think somewhat less strongly discouraged?
  • Use deterministic mentor selection so the mentor is sort of defined even if we don't store it. (Only sort of because any change in the set of mentors would effectively re-randomize assignment, but that's probably good enough for dealing with job queue delay.) Tricky because we don't actually want the mentor to be entirely deterministic - users should be able to change mentors if they want. But maybe not an issue because that always involves selecting the mentor in a POST request so we can just write to the DB immediately?

@Krinkle would appreciate your thoughts on the first two options - how hard should we try to avoid deferred cross-DC writes? Is there any guidance against using the main stash for this (AIUI it's not going to be very different from doing a DB write in a deferred)?

Ask for a new mentor

This button is making a POST request to growthsetmenteestatus. That API module invokes MentorPageManager->setMentorshipStateForUser(). That method drops the mentee relationship when the user is opting out; we could have it get the randomly assigned mentor and save it to the DB when the user is opting-in.

Δ includes/Mentorship/MentorPageMentorManager.php
──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

────────────────────────────────────────────────────────────────────────────────────────┐
 301: class MentorPageMentorManager extends MentorManager implements LoggerAwareInterf 
────────────────────────────────────────────────────────────────────────────────────────┘
        );
        $this->userOptionsManager->saveOptions( $user );

        if ( $state === self::MENTORSHIP_ENABLED ) {
            $this->mentorStore->setMentorForUser(
                $user,
                $this->getRandomAutoAssignedMentor( $user ),
                MentorStore::ROLE_PRIMARY
            );
        }

        if ( $state === self::MENTORSHIP_OPTED_OUT ) {
            // user opted out, drop mentor/mentee relationship
            $this->mentorStore->dropMenteeRelationship( $user );

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

[mediawiki/extensions/GrowthExperiments@master] Mentorship: When opting in, persist mentor relationship in MentorStore

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

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

[mediawiki/extensions/GrowthExperiments@master] Mentorship: When opting in, persist mentor relationship in MentorStore

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

This patch implements the idea from T323374#8453631.

Change 881805 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] Mentorship: When opting in, persist mentor relationship in MentorStore

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

Etonkovidova subscribed.

Checked in testwiki wmf.21 - the issue seems to be fixed.