Fixed: Random failures in CommentInterfaceTest - Link with label 1 new comment not found.

[indigoxela]

Description of the bug

Part of #1478

In core/modules/comment/tests/comment.test, a random failure with GHA.

Not to get confused with #2601, which was about the opposite (expected link not found). But very similar.

Steps To Reproduce

Still evaluating - possibly caused by the order of tests as they run.

Test run output

Detailed test results
---------------------
Fail      Other      comment.test       526 CommentInterfaceTest->testCommentNe

Fail      Other      comment.test       529 CommentInterfaceTest->testCommentNe

    Link with label 1 new comment not found.
    Array
    (
        [0] => SimpleXMLElement Object
            (
                [@attributes] => Array
                    (
                        [class] => node-readmore odd first
                    )
    
                [a] => Read more
            )
    
        [1] => SimpleXMLElement Object
            (
                [@attributes] => Array
                    (
                        [class] => comment-comments even
                    )
    
                [a] => 1 comment
            )
    
        [2] => SimpleXMLElement Object
            (
                [@attributes] => Array
                    (
                        [class] => comment-new-comments odd
                    )
    
                [a] => 1 new comment
            )
    
        [3] => SimpleXMLElement Object
            (
                [@attributes] => Array
                    (
                        [class] => comment-add even last
                    )
    
                [a] => Add comment
            )

    )

[indigoxela]

As usual, I'm not able to reproduce the problem locally. It looks like a race condition (wild guessing, though...). Maybe stuffing in a sleep(1) second might fix it.

It appears as if the first and second GET request are so close to each other and the database in the test box so busy, that the db record isn't written yet, when the second request is sent? Link to failing test code

[indigoxela]

Based on my assumptions (previous comment), I created a PR - seems to work so far, but only time can proof it. At least, it doesn't make things worse.

(And obviously PathPatternBulkUpdateTestCase and BatchProcessingTestCase should be the next candidates in our ongoing random failures task.)

[quicksketch]

This issue seems very similar to the batch API processing issue in #5205. The root cause I think is the same: Node module uses a shutdown function to update the history table, which keeps track of page views by individual users. This is then used to determine whether comments are considered "new" or not. However, shutdown functions are processed in the background, after the HTML content of the page is returned. This means that a race condition can exist if updating the database is slower than the subsequent page request that tries to read the history table.

This is the relevant code in node_show():

  // Update the history table, stating that this user viewed this node.
  node_tag_new($node, TRUE);
  backdrop_register_shutdown_function('node_tag_new');

This switching to use a shutdown function was added in #2926.

In #5205, I removed the shutdown function to solve the problem. I think @indigoxela's PR is the better approach for this issue, simply giving the background processing a second to update the history table, rather than removing the shutdown function. In this case, the impact on end-users is very low if their view history is not perfectly up-to-date.

[quicksketch]

I merged backdrop/backdrop#3727 into 1.x and 1.19.x. I expanded on the code comments slightly.

Thanks @indigoxela!