Let background commands actually run in the background

[fredden]

Update 2023-05-30

After discussion with @engcom-Charlie (thanks!), I set up the Magento Functional Testing Framework locally to investigate the specifics of this test. The report was that the test worked fine on the 2.4-develop branch, but was failing on #32690, so it was suggested that the changes there were breaking this test, and that the test was fine as-is on 2.4-develop. I thought this was a race condition, but it seems it's more complicated than a simple race condition.

The reason the test isn't failing on 2.4-develop is because STDERR isn't being redirected for the child processes, so the testing framework wasn't actually generating background processes when it should have been. https://github.com/magento/magento2/pull/37511/commits/7d56fe499617d1ad03c77ecace4dfd7c60a4c3dd fixes that bug, which will mean that all other unreliable tests will now be failing here. Many of the test failures here will be fixed by https://github.com/magento/magento2-functional-testing-framework/pull/904

Description

While working on https://github.com/magento/magento2/pull/32690#discussion_r1196637387, it was discovered that the test StorefrontVerifyProductAfterPartialReindexOnSeveralWebsitesTest (from Magento\AcceptanceTest\_default\Backend\StorefrontVerifyProductAfterPartialReindexOnSeveralWebsitesTest) is unreliable. This pull request aims to improve the reliability of this test. While this is not a complete fix, it will improve the reliability.

Why is the test unreliable? The test aims to ensure that indexers are correctly triggered / rebuilt when making changes in the admin. The general process goes: log into admin, make a change, run cron, witness result. It's the "run cron" step that is causing issues. The internals of that step spawns separate processes, and then returns. These separate processes will run in the background and will work, however there is a race condition between these background processes running to completion and the next step of the test being carried out. When the indexer process is quick, all is well. When the test framework gets to the next step before the background process complete, the test fails.

How does this change help?

This pull request turns the background process into a foreground process, so the test framework won't ever reach the next step until the "background" process finishes.

Why is this change incomplete?

There is no guarantee that a job has actually been scheduled. There is still an expectation that there is a indexer_update_all_views job in the queue, ready to be executed. Usually this will be the case, however it's not guaranteed. Further test improvements are welcome in this area.

Manual testing scenarios

Run the following PHP script. It should report success.

#!/usr/bin/env php
<?php

use Magento\Framework\App\ObjectManager;
use Magento\Framework\Console\Cli;
use Magento\Framework\Shell\CommandRendererBackground;

$command = 'sleep 10';

require __DIR__ . '/app/bootstrap.php';
new Cli('Magento CLI');
$objectManager = ObjectManager::getInstance();
$commandRenderer = $objectManager->get(CommandRendererBackground::class);

$command = $commandRenderer->render($command);
$startTime = microtime(true);

echo "DEBUG: running `$command` ...\n";
$process = proc_open($command, [
    0 => ['pipe', 'r'],
    1 => ['pipe', 'w'],
    2 => ['pipe', 'w'],
], $pipes, getcwd());
fclose($pipes[0]); // STDIN
echo stream_get_contents($pipes[1]); // STDOUT
echo stream_get_contents($pipes[2]); // STDERR
proc_close($process);

$endTime = microtime(true);
$duration = $endTime - $startTime;
$result = $duration >= 5 ? 'fail' : 'pass';
printf('Test %sed. It took %.1f seconds to start a long-running background process.', $result, $duration);
echo "\n";

Contribution checklist

• [x] Pull request has a meaningful description of its purpose
• [x] All commits are accompanied by meaningful commit messages
• [x] All new or changed code is covered with unit/integration tests (if applicable)
• [x] README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
• [x] All automated tests passed successfully (all builds are green)

[m2-assistant[bot]]

Hi @fredden. Thank you for your contribution! Here are some useful tips on how you can test your changes using Magento test environment.

Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

:exclamation: Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s) For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here :information_source: Run only required test builds during development. Run all test builds before sending your pull request for review.

---

For more details, review the Code Contributions documentation. Join Magento Community Engineering Slack and ask your questions in #github channel.

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[fredden]

@engcom-Hotel / @engcom-Charlie, given this has been mentioned several times in https://github.com/magento/magento2/pull/32690, and the automated testing suite is passing 100% here, please can you look to get this reviewed/merged to avoid further delays in https://github.com/magento/magento2/pull/32690?

[fredden]

@engcom-Charlie thanks very much for taking the time to discuss this and https://github.com/magento/magento2/pull/32690 with me today. I've updated the "manual testing" steps in this pull request as discussed. I will try to get the Magento Functional Testing Framework set up to run at my end to verify that these steps are sufficient to clearly show that the test is broken on the 2.4-develop branch and fixed here. If changes are required to these steps, I'll update here and contact you on Slack as suggested.

[fredden]

@engcom-Charlie I've worked out why you were not seeing the expected failure of this test in 2.4-develop, but were seeing it fail in #32690. With the old/current \Magento\Framework\Shell\CommandRendererBackground::render() class in use, STDERR is not redirected from the child process, so the HTTP call to /dev/tests/acceptance/utils/command.php spawns bin/magento cron:run which in turn spawns a separate process for the indexer group (and others), but because STDERR is still bound to the initial cron:run process, this becomes a zombie until the grandchild process has closed its STDERR (ie, terminated). This all means that the cron:run process effectively "waits" for its child processes to complete before returning, so the race cannot be witnessed.

To reliably reproduce the bug this pull request is solving, we need to redirect STDERR here: https://github.com/magento/magento2/blob/39ec4faeec251213a3b8d5570acbd294b82c1c9c/lib/internal/Magento/Framework/Shell/CommandRendererBackground.php#L38 eg, : str_replace('2>&1', '2>/dev/null >/dev/null &', $command);

The changes introduced in #32690 include redirecting STDERR as well as STDOUT, which is why we can see the race condition here in that pull request.

Edit: I've repurposed this pull request to fix that bug (background processes aren't run in the background), and we can fix the (already) broken tests which this bug-fix highlights in scope of this pull request.

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[fredden]

Many of the test failures here will be fixed by https://github.com/magento/magento2-functional-testing-framework/pull/904

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[ihor-sviziev]

@engcom-Bravo I think it should have priority P2 because it significantly improves reliability of the test suite /cc @engcom-Charlie

[engcom-Hotel]

@magento run Functional Tests CE, Functional Tests EE, Functional Tests B2B

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[fredden]

@engcom-Hotel tests here will fail until https://github.com/magento/magento2-functional-testing-framework/pull/904 gets released. I don't see any value in running the test-suite here before that is done. See also https://github.com/magento/magento2/pull/37511#issuecomment-1568870581.

[engcom-Hotel]

Hello @fredden,

I have added a Related Pull Requests tag in the description. It should work now.

Thanks

[engcom-Hotel]

@magento run Functional Tests CE, Functional Tests EE, Functional Tests B2B

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[engcom-Hotel]

@magento run all tests

[magento-automated-testing[bot]]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please message the #magento-devops slack channel if they don't show in a reasonable amount of time and a representative will look into any issues.

[fredden]

I have added a Related Pull Requests tag in the description. It should work now.

I had on-purpose not done this so that this pull request didn't hold up that one. (My previous experience linking pull requests like this is that it slows down both sides when they're not part of the same works, but are pre-requisites.) I'll leave this with you to organise / progress.

[engcom-Hotel]

Hello @fredden,

Thanks for the contribution!

In order to test the PR, we first tried to reproduce the issue in the 2.4-develop branch with mentioned manual testing scenarios in the main description.

We got the below error while running the StorefrontVerifyProductAfterPartialReindexOnSeveralWebsitesTest test:

Then we tried the same test with this PR by including the changes in the PR https://github.com/magento/magento2-functional-testing-framework/pull/904

But still, we are getting the same error:

Please let us know if we are missing anything.

Thanks

[fredden]

@engcom-Hotel perhaps 'sleep(65)' is longer than your HTTP time-out. Can you try with 'sleep(5)' (or without adding a sleep) instead?

[engcom-Hotel]

Hello @fredden,

Thanks for the reply!

We have tried to run the below test without sleep as well, but we are getting the same error:

vendor/bin/mftf run:test StorefrontVerifyProductAfterPartialReindexOnSeveralWebsitesTest --remove

You can see the same failure has been occurring here functional tests

Thanks

[fredden]

@engcom-Hotel thanks for looking into this. I think there are too many variables here to easily identify why you're getting these results. Let's get https://github.com/magento/magento2-functional-testing-framework/pull/904 merged in, and I can then update the composer.lock file here and test again.

[engcom-Charlie]

Hi @fredden,

As per the latest updates #904 is merged and as discussed with internal team member MFTF 4.3.2 release is done. The release tag is avaiable now.

I have updated the composer.json and composer.lock. Updated the other required changes too in composer.json after having discussion with MFTF's internal team member. Removing #904 from the Related PR section now.

Thank you!

[engcom-Charlie]

@magento run all tests