Page MenuHomePhabricator

The "changed since last visit" flag is not working correctly (watchlist items bold randomly etc.)
Closed, ResolvedPublic

Description

Author: wikipara

Description:
The "Pages which have been changed since you last visited them are shown in bold" feature of Special:Watchlist broke on Commons and Meta at some point during the last few days.

Some pages changed after my last visit are not bold, and some that I have visited after the latest change are still in bold. All my watched and changed pages seem to be correctly bolded in Recent Changes however. Both the expanded and normal watchlist show this bug. The <strong> tag or its absence seems random.

I don't see any pattern to this, and apparently it's only happening to some users, not all. See also http://commons.wikimedia.org/wiki/Commons:Help_desk#Changes_don.27t_get_marked.


Version: unspecified
Severity: normal

Details

Reference
bz10172

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 9:51 PM
bzimport set Reference to bz10172.

brianna.laugher wrote:

The problem is not that they are bold "at random", but they are not bold consistently as they used to be. It used to be pages you hadn't read since the last edit on that page would be bold. They would go non-bold after you visited the page. Now, there can be new changes you haven't read yet, but the page listing isn't bold as it should be according to the previous behaviour.

wikipara wrote:

It's working ok for me again. What happened?? Will it happen again?

brianna.laugher wrote:

Seems to be fixed now. Easy solution. ;)

RSYQFIOJGWZA wrote:

It does not seem to be fixed for me now. :(

wikipara wrote:

Indeed, the problem is back for me too. (Commons:User:Para)

The last few days I've had the watchlist lag behind some minutes, ie. updated pages I haven't seen yet are listed at the top of the watchlist, but not in bold until some moments of wait.

Today it seems to have turned into "random" again, with no logic in which unseen pages are listed in bold. If I visit one of the bolded pages, it correctly turns not-bold, but it's the unseen not-bold pages that are making it hard to stay up to date with changes.

The problem is that the on-edit update (mark the page as modified) is postponed to the job queue (if $wgEnotifUseJobQ is set, which probably is), but the on-view update (clear the modified flag on the page) is executed immediately during page view. That means that 1. your edits are always displayed bold, 2. if you view a page too soon after the respective change, that visit is ignored (since it is overwritten a bit later by the postponed EnotifNotifyJob) and you need to revisit it to clear the flag, 3. very recent edits are shown non-bold on top of the watchlist, and then, suddenly, become bold, as the postponed job is executed.

This is obviously broken: because the implementation using wl_notificationtimestamp being NULL/non-NULL is dependent on the order of operations, the operations need to be executed in the correct order, which they currently are not.

I can see a few options how to solve this:

  1. Switch $wgEnotifUseJobQ off, so that it works at least. (But, the code would still be broken, and there could be some performance implications.)
  2. Do not postpone wl_notificationtimestamp to the work queue and do it right away in notifyOnPageChange. (Are there any real performance implications in that, provided that the whole change-notification business is disabled on the large wikis, IIANM?)
  3. Move viewUpdate() to the job queue, too. (Kinda silly, and the problem #3 above would still hold, but at least it would work a little bit more deterministically and the ordering would be correct. Also: creating a job for every page view is probably a _really_ silly thing to do…)
  4. Implement the whole thing a little bit better. One idea that wouldn't need so much work would be to store both the last-visit and last-update timestamps separately, so that the operations wouldn't need the correct ordering (they could detect that the on-edit job is obsolete and ignore it, etc.). (You could even get without the last-update column completely, since the information is stored in the page history…) You could even do without a schema change with a small hack: store +timestamp for a timestamp of a non-visited edit, and -timestamp for a timestamp of the last visit to a page that has not yet been changed since that.
  • Bug 10884 has been marked as a duplicate of this bug. ***
  • Bug 9894 has been marked as a duplicate of this bug. ***

Changing summary so that it expresses the bigger scope better.

If this feature cannot be fixed, I would ask to have it deactivated, at least on my accounts, as it's pretty useless, distracting and thus an unnecessary server drain.

When fixing, could it be added as a setting in preferences as well? I do not see the additional value on wiki's I'm highly active.

webboy wrote:

(In reply to comment #11)

When fixing, could it be added as a setting in preferences as well? I do not
see the additional value on wiki's I'm highly active.

Added as bug 13910

Changing component to "Watchlist"

*** Bug 17688 has been marked as a duplicate of this bug. ***

(In reply to comment #6)

  1. Do not postpone wl_notificationtimestamp to the work queue and do it right

away in notifyOnPageChange. (Are there any real performance implications in
that, provided that the whole change-notification business is disabled on the
large wikis, IIANM?)

I think that's the way to go; I'll post a patch for this later today.

Created attachment 5868
Proposed patch

The attached patch aims to fix this bug by:

  • moving up the UPDATE on wl_notificationtimestamp to before the EnotifNotifyJob is scheduled
  • moving up the query fetching the users to be notified before that, because it needs the old value of wl_notificationtimestamp
  • passing actuallyNotifyOnPageChange() and the EnotifyNotifyJob an array of user IDs
  • adding UserArray::newFromIDs()

attachment 10172.patch ignored as obsolete

Created attachment 5873
Improved patch

Removed useless JOIN on user table, thanks to Tim for spotting it.

attachment 10172.patch ignored as obsolete

Created attachment 5874
More improvements

Casting $ids to an array, avoiding empty(), returning an empty array instead of null.

attachment 10172.patch ignored as obsolete

Created attachment 5875
Patch v4

...and more improvements:

  • Run the SELECT query on the master
  • Use the result of the SELECT query for the UPDATE query
  • Fix a $row->user_id reference
  • Use new ArrayIterator(array()) instead of array() in UserArray::newFromIDs()

Attached:

(In reply to comment #19)

Created an attachment (id=5875) [details]
Patch v4

...and more improvements:

  • Run the SELECT query on the master
  • Use the result of the SELECT query for the UPDATE query
  • Fix a $row->user_id reference
  • Use new ArrayIterator(array()) instead of array() in UserArray::newFromIDs()

Slightly modified version of this patch committed in r47927.

reopened the 10172, because this problem is ongoing (again present) since January 2009.

(In reply to comment #22)

reopened the 10172, because this problem is ongoing (again present) since
January 2009.

I committed a patch that should fix this issue a few hours ago, see comment #20. Reclosing as FIXED.

As Mormegil notes in comment #6, the problem is that the whole feature has been designed ass-backwards. Any design that requires the code to update a bazillion watchlist records whenever a busy and popular page changes is not going to scale well. It's not as if it should be difficult to do it the right way (store last visit timestamp in watchlist, compare with last update timestamp in page/recentchanges table).

OK, comment #25 might be correct, but the fix committed in r47927 might work, but it contains a terrible bug, AFAICT, which makes the result worse than the original problem.

See my comment at http://www.mediawiki.org/wiki/Special:Code/MediaWiki/47927#c2262

(In reply to comment #26)

OK, comment #25 might be correct, but the fix committed in r47927 might work,
but it contains a terrible bug, AFAICT, which makes the result worse than the
original problem.

Oops, that would be totally my fault. Seems to have been fixed in r49682, though, I'll push for that rev to go live.

(In reply to comment #26)
Oops, that would be totally my fault. Seems to have been fixed in r49682,
though, I'll push for that rev to go live.

I really appreciate that you reintroduced that _core_ piece of my algorithm.
Tom

Deployed r49682 on Wikimedia and backported to 1.15.

  • Bug 12803 has been marked as a duplicate of this bug. ***