In general, it would be great if we could somehow upstream these tests into Wikibase proper. Learning about such regressions over a year after the fact isn't exactly ideal.

Indeed, the authors of the tests have suggested it when they were created, see e.g. T282479

Task Breakdown Notes:

• Calrification: This involves the following steps:
  1. Creating an item in a wikibase instance
  2. Linking that item into a client wiki
  3. Linking the item into another client wiki
  4. Deleting the wikidata item does not result in the removal of sitelinks between the two wiki pages, no removal of the sitelink appears in recent changes either
• This is going to be difficult to reproduce locally
• We believe the regression was introduced when we rewrote the change dispatching system into a MediaWiki job
• Might be able to test this in one wiki alone, since change propagation seems to occur also for links between an item and an article on the same wiki
• We want to think of the testability of several Recent Changes entries while we write the tests (in case we are able to test dispatching in a single wiki)

Potential Plan of Action
(1a) We can simply write a browser test (that would probably not be run locally) to ensure this behavior is correct
(1b) Alternately, we could potentially write it as API Integration tests to wait for jobs to complete while testing this behavior

(2) Look into change propagation [entry file here] and in particular how it relates to item deletion, we can track down the class that was mentioned in the description: JobQueueChangeNotificationSender to figure out where the call to sendNotification has gone to

Change 878193 had a related patch set uploaded (by Michael Große; author: Michael Große):

[mediawiki/extensions/Wikibase@master] Add browser test for Change Dispatching

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

In T326082#8512537, @ItamarWMDE wrote:

Task Breakdown Notes:

• Calrification: This involves the following steps:
  1. Creating an item in a wikibase instance
  2. Linking that item into a client wiki
  3. Linking the item into another client wiki
  4. Deleting the wikidata item does not result in the removal of sitelinks between the two wiki pages, no removal of the sitelink appears in recent changes either

Correct.
No notification is given to the client at all that the entity was deleted.
This included the removal of interwiki links if an item is linked to multiple pages, and those pages thus have interwiki links
But this also includes for example the "data item" link the side bar, or displaying infomation about the linked item on the page info page (such as the ID).

Thus in a minimal reproduction you only need 1 repo and 1 client and to follow these steps

1. Creating an item in a wikibase instance
2. Linking that item into a client wiki
3. Delete that item
4. Deleting the item does notify the client the item was deleted, and thus does not result in removal of interwiki links on the client page, or removal of the data item link from the side page, or item infomation from page info.

• This is going to be difficult to reproduce locally
• We believe the regression was introduced when we rewrote the change dispatching system into a MediaWiki job
• Might be able to test this in one wiki alone, since change propagation seems to occur also for links between an item and an article on the same wiki

Hopefully with only a single repo & client required this should become a little easier as it should all be possible on a single wiki.

• We want to think of the testability of several Recent Changes entries while we write the tests (in case we are able to test dispatching in a single wiki)

Potential Plan of Action
(1a) We can simply write a browser test (that would probably not be run locally) to ensure this behavior is correct
(1b) Alternately, we could potentially write it as API Integration tests to wait for jobs to complete while testing this behavior

(2) Look into change propagation [entry file here] and in particular how it relates to item deletion, we can track down the class that was mentioned in the description: JobQueueChangeNotificationSender to figure out where the call to sendNotification has gone to

Change 878954 had a related patch set uploaded (by Addshore; author: Addshore):

[mediawiki/extensions/Wikibase@REL1_38] RELNOTES 1.38, Note about regression

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

Change 878954 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@REL1_38] RELNOTES 1.38, Note about regression

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

Change 878965 had a related patch set uploaded (by Lucas Werkmeister (WMDE); author: Lucas Werkmeister (WMDE)):

[mediawiki/extensions/Wikibase@master] Add API integration test for change dispatching

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

What I’ve found in the code so far:

• RepoHooks::onArticleDeleteComplete() calls ChangeNotifier::notifyOnPageDeleted()
• which calls ChangeFactory::newForPageDeleted() and transmits the resulting change
• RecentChangeSaveHookHandler::onRecentChange_save() gets all the transmitted changes and dispatches them if needed, but:

onRecentChange_save() only handles edits and page restores, not deletions?

if ( $logType === null || ( $logType === 'delete' && $logAction === 'restore' ) ) {
	foreach ( $this->changeHolder->getChanges() as $change ) {
		$this->handleChange( $change, $recentChange );
	}
}

That looks like it would explain the issue – but that condition is almost eight years old (Set Change timestamp based on RC timestamp), so that doesn’t explain why this would be a regression with the new change dispatching code…

That looks like it would explain the issue – but that condition is almost eight years old (Set Change timestamp based on RC timestamp), so that doesn’t explain why this would be a regression with the new change dispatching code…

As far as I found in my digging, the old dispatching code calls a bunch of stuff that is no longer called. I believe this might have been the path for the old RC entry that is missing?)

• JobQueueChangeNotificationSender::sendNotification I don't believe is called any more
• a ChangeNotification job spec is created by the above method, again this job is no longer used at all?
• The class ChangeNotificationJob (also not used?) calls ChangeHandler::handleChanges which in turn calls handleChange, then calling updater->injectRCRecords

There were some of my notes from my investigation,
But I am ready to be wrong! and havn't double checked this in great detail

I think there’s one more condition in onRecentChange_save() that filters out the deletion – if I disable that, then most of my API integration tests (see Gerrit change above) actually work:

diff --git a/repo/includes/Hooks/RecentChangeSaveHookHandler.php b/repo/includes/Hooks/RecentChangeSaveHookHandler.php
index aba78cad15..a86e3a69b3 100644
--- a/repo/includes/Hooks/RecentChangeSaveHookHandler.php
+++ b/repo/includes/Hooks/RecentChangeSaveHookHandler.php
@@ -66,22 +66,33 @@ public static function factory(
     public function onRecentChange_save( RecentChange $recentChange ): void {
         $logType = $recentChange->getAttribute( 'rc_log_type' );
         $logAction = $recentChange->getAttribute( 'rc_log_action' );
 
         if ( $recentChange->getAttribute( 'rc_this_oldid' ) <= 0 ) {
             // If we don't have a revision ID, we have no chance to find the right change to update.
             // NOTE: As of February 2015, RC entries for undeletion have rc_this_oldid = 0.
-            return;
+            // return;
         }
 
-        if ( $logType === null || ( $logType === 'delete' && $logAction === 'restore' ) ) {
+        if ( $logType === null || ( $logType === 'delete' && ( $logAction === 'restore' || $logAction === 'delete' ) ) ) { // TODO delete_redir?
             foreach ( $this->changeHolder->getChanges() as $change ) {
                 $this->handleChange( $change, $recentChange );
             }
         }
     }

Specifically, the tests for “no more sitelink usage” and “no more wikibase_item page prop” pass, but the test for “external recent change for item deletion” only passed once (right after I made that change) but has been failing again ever since, frustratingly. Also, that return is probably there for a reason… needs more investigation. (Also with what Adam wrote above.)

That revision Id is used in \Wikibase\Repo\Hooks\RecentChangeSaveHookHandler::setChangeMetaData. So we probably need to track down how exactly it is being used further down in process. It could be that this is just a left-over from the previous change dispatching system. But it could also be that it was created specifically for this new one. I guess this is a good opportunity to get xdebug working again and stepping through the code

As far as I can see, that revision ID is mainly used in \Wikibase\Client\Changes\ChangeRunCoalescer - a part that wasn't touched in the rework. In theory, that ChangeRunCoalescer says that it handles delete actions correctly, though I did not see that in code. But if there are any issues there, we can probably refactor/fix that part, it doesn't seem to be crucial.

The browser test I added does pass locally with the changes suggested by @Lucas_Werkmeister_WMDE. However, that does only deal with a single change, and we should probably double check that ChangeRunCoalescer does still a sensible thing if there a multiple changes that include one that deletes an Item.

That being said, I'm not sure that all assumptions that went into creating ChangeRunCoalescer are still true. (IIRC, now one job only contains the changes to one Entity, but I'm not fully sure, as the rework was created quite some time ago.)

Change 880537 had a related patch set uploaded (by Hoo man; author: Hoo man):

[mediawiki/extensions/Wikibase@master] Remove (JobQueue)ChangeNotificationSender

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

I have not fully finished this yet, but one thing to note: The return if rc_this_oldid is <= 0 is no longer needed. This was added back when changes were saved to wb_changes and later loaded by revision id (used to be ChangeLookup::loadByRevisionId), but nowadays the changes are only identified/ updated by their own id.

Change 883165 had a related patch set uploaded (by Hoo man; author: Hoo man):

[mediawiki/extensions/Wikibase@master] RecentChangeSaveHookHandler: Handle (un)deletions

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

Change 883165 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] RecentChangeSaveHookHandler: Handle (un)deletions

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

Change 878965 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Add API integration test for change dispatching

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

Change 880537 merged by jenkins-bot:

[mediawiki/extensions/Wikibase@master] Remove (JobQueue)ChangeNotificationSender

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

This should be fixed now – @Addshore is it possible to verify this using the release pipeline tests? (By backporting the fix to REL1_38 and REL1_39, I assume?)

Change 878193 abandoned by Michael Große:

[mediawiki/extensions/Wikibase@master] Add browser test for Change Dispatching

Reason:

This is not providing value over the api-testing test (If4a281bc) and still would have significant hurdles to overcome

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

To me this seems complete, so I'll move it to done on our board, but not resolve this. Pending news from @Addshore and @jon_amar-WMDE whether the tests pass in the wikibase release pipeline test suite.