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. 7d56fe4 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 magento/magento2-functional-testing-framework#904

Description

While working on #32690 (comment), 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

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

Resolved issues:

1. resolves [Issue] Let background commands actually run in the background #38359: Let background commands actually run in the background

[m2-assistant]

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

❗ 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
ℹ️ 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]

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]

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]

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]

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]

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]

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 #32690, and the automated testing suite is passing 100% here, please can you look to get this reviewed/merged to avoid further delays in #32690?

[fredden]

@engcom-Charlie thanks very much for taking the time to discuss this and #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:

magento2/lib/internal/Magento/Framework/Shell/CommandRendererBackground.php

Line 38 in 39ec4fa

: str_replace('2>&1', '> /dev/null &', $command);

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]

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]

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 magento/magento2-functional-testing-framework#904

[magento-automated-testing]

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-Charlie]

HI @fredden,

Thank you for the updated Manual testing scenario.

✔️ QA Passed

Before: ✖️
I have executed the given script in latest magento 2.4-develop and the issue is reproducible.

After: ✔️
In PR branch the same script is working fine.

As the build is green so moving it to Merge in Progress.

Thank you!

[engcom-Charlie]

@magento create issue

[engcom-Charlie]

It seems that bot moved it to testing hence moving it back to its previous status, Merge in Progress.

[engcom-Charlie]

@magento run Semantic Version Checker