Fix busy-loop in withQueueLock() on non-EEXIST mkdir failure
If mkdir(queue.lock) failed for a reason other than "already exists" (e.g. a permissions or disk problem on the data directory), is_dir() was false, so the loop `continue`d before ever reaching the tries>=40 retry-limit check or the usleep(), spinning at full CPU forever instead of failing with a clear error. Move the staleness check inside an is_dir() guard instead of using continue to skip past the retry-limit logic, so the bounded-retry behavior applies regardless of why mkdir failed.
This commit is contained in:
@@ -1295,13 +1295,12 @@ final class BackupQueueService
|
|||||||
$tries = 0;
|
$tries = 0;
|
||||||
while (!@mkdir($lockDir, 0700)) {
|
while (!@mkdir($lockDir, 0700)) {
|
||||||
$tries++;
|
$tries++;
|
||||||
if (!is_dir($lockDir)) {
|
|
||||||
continue;
|
|
||||||
}
|
|
||||||
|
|
||||||
$mtime = (int)@filemtime($lockDir);
|
if (is_dir($lockDir)) {
|
||||||
if ($mtime > 0 && (time() - $mtime) > 120) {
|
$mtime = (int)@filemtime($lockDir);
|
||||||
@rmdir($lockDir);
|
if ($mtime > 0 && (time() - $mtime) > 120) {
|
||||||
|
@rmdir($lockDir);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if ($tries >= 40) {
|
if ($tries >= 40) {
|
||||||
|
|||||||
@@ -0,0 +1,114 @@
|
|||||||
|
<?php
|
||||||
|
declare(strict_types=1);
|
||||||
|
|
||||||
|
$pluginTempRoot = sys_get_temp_dir() . '/altmysql-plugin-root-' . bin2hex(random_bytes(4));
|
||||||
|
mkdir($pluginTempRoot . '/data/jobs', 0700, true);
|
||||||
|
define('PLUGIN_ROOT', $pluginTempRoot);
|
||||||
|
|
||||||
|
$pluginDir = dirname(__DIR__);
|
||||||
|
require_once $pluginDir . '/exec/lib/Settings.php';
|
||||||
|
require_once $pluginDir . '/exec/lib/DirectAdminUser.php';
|
||||||
|
require_once $pluginDir . '/exec/lib/BackupQueueService.php';
|
||||||
|
|
||||||
|
function fail(string $message): void
|
||||||
|
{
|
||||||
|
fwrite(STDERR, "FAIL: {$message}\n");
|
||||||
|
exit(1);
|
||||||
|
}
|
||||||
|
|
||||||
|
function load_settings(string $contents): Settings
|
||||||
|
{
|
||||||
|
$path = tempnam(sys_get_temp_dir(), 'altmysql-settings-');
|
||||||
|
if ($path === false) {
|
||||||
|
fail('cannot create temp settings file');
|
||||||
|
}
|
||||||
|
file_put_contents($path, $contents);
|
||||||
|
$settings = Settings::load($path);
|
||||||
|
unlink($path);
|
||||||
|
return $settings;
|
||||||
|
}
|
||||||
|
|
||||||
|
function make_da_user(Settings $settings, string $username): DirectAdminUser
|
||||||
|
{
|
||||||
|
$class = new ReflectionClass(DirectAdminUser::class);
|
||||||
|
/** @var DirectAdminUser $user */
|
||||||
|
$user = $class->newInstanceWithoutConstructor();
|
||||||
|
foreach ([
|
||||||
|
'username' => $username,
|
||||||
|
'prefix' => $username . '_',
|
||||||
|
'userConf' => [],
|
||||||
|
'packageConf' => [],
|
||||||
|
'settings' => $settings,
|
||||||
|
] as $property => $value) {
|
||||||
|
$prop = $class->getProperty($property);
|
||||||
|
$prop->setAccessible(true);
|
||||||
|
$prop->setValue($user, $value);
|
||||||
|
}
|
||||||
|
return $user;
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!function_exists('pcntl_fork')) {
|
||||||
|
fail('pcntl extension is required to safely test withQueueLock() without risking a hang');
|
||||||
|
}
|
||||||
|
|
||||||
|
// Make the jobs/ directory read-only so mkdir(queue.lock) fails for a reason
|
||||||
|
// OTHER than "already exists" (EACCES, not EEXIST) - this is the branch that
|
||||||
|
// must never busy-loop forever.
|
||||||
|
chmod($pluginTempRoot . '/data/jobs', 0500);
|
||||||
|
|
||||||
|
$pid = pcntl_fork();
|
||||||
|
if ($pid === -1) {
|
||||||
|
fail('could not fork to safely bound the withQueueLock() call');
|
||||||
|
}
|
||||||
|
|
||||||
|
if ($pid === 0) {
|
||||||
|
// Child: attempt the call. If it hangs, the parent will kill us after
|
||||||
|
// the deadline below and the test fails. If it throws (correct fixed
|
||||||
|
// behavior), exit 42. Any other outcome exits 1.
|
||||||
|
$settings = load_settings('');
|
||||||
|
$daUser = make_da_user($settings, 'demo');
|
||||||
|
$queue = new BackupQueueService($settings, $daUser);
|
||||||
|
|
||||||
|
$class = new ReflectionClass(BackupQueueService::class);
|
||||||
|
$method = $class->getMethod('withQueueLock');
|
||||||
|
$method->setAccessible(true);
|
||||||
|
|
||||||
|
try {
|
||||||
|
$method->invoke($queue, static fn () => 'unreachable');
|
||||||
|
exit(1);
|
||||||
|
} catch (RuntimeException $e) {
|
||||||
|
exit(42);
|
||||||
|
} catch (Throwable $e) {
|
||||||
|
exit(1);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
$deadline = microtime(true) + 5.0;
|
||||||
|
$status = 0;
|
||||||
|
$exited = false;
|
||||||
|
while (microtime(true) < $deadline) {
|
||||||
|
$result = pcntl_waitpid($pid, $status, WNOHANG);
|
||||||
|
if ($result === $pid) {
|
||||||
|
$exited = true;
|
||||||
|
break;
|
||||||
|
}
|
||||||
|
usleep(50000);
|
||||||
|
}
|
||||||
|
|
||||||
|
if (!$exited) {
|
||||||
|
posix_kill($pid, SIGKILL);
|
||||||
|
pcntl_waitpid($pid, $status);
|
||||||
|
@chmod($pluginTempRoot . '/data/jobs', 0700);
|
||||||
|
exec('rm -rf ' . escapeshellarg($pluginTempRoot));
|
||||||
|
fail('withQueueLock() busy-looped instead of giving up after a bounded number of retries (killed after 5s)');
|
||||||
|
}
|
||||||
|
|
||||||
|
@chmod($pluginTempRoot . '/data/jobs', 0700);
|
||||||
|
exec('rm -rf ' . escapeshellarg($pluginTempRoot));
|
||||||
|
|
||||||
|
$exitCode = pcntl_wexitstatus($status);
|
||||||
|
if ($exitCode !== 42) {
|
||||||
|
fail("withQueueLock() did not fail with the expected RuntimeException (child exit code: $exitCode)");
|
||||||
|
}
|
||||||
|
|
||||||
|
echo "backup_queue_lock_test: OK\n";
|
||||||
Reference in New Issue
Block a user