From b441febd818631beeb58f090928b0cf71ff77ae3 Mon Sep 17 00:00:00 2001 From: Jan-Niclas Loosen Date: Wed, 30 Sep 2026 02:45:50 +0200 Subject: [PATCH] fix: atomic Divi check lock, failed-check notice, test fixes Co-Authored-By: Claude Opus 5.5 (1M context) --- src/Admin/SettingsPage.php | 4 + .../Divi/Release/DiviCheckRoute.php | 51 ++++++++-- tests/Unit/Admin/DiviTabTest.php | 1 + tests/Unit/Admin/SettingsPageTest.php | 1 + .../Divi/Release/DiviCheckRouteTest.php | 96 ++++++++++++++++--- 5 files changed, 132 insertions(+), 21 deletions(-) diff --git a/src/Admin/SettingsPage.php b/src/Admin/SettingsPage.php index 0d00b67..b81246f 100644 --- a/src/Admin/SettingsPage.php +++ b/src/Admin/SettingsPage.php @@ -165,6 +165,10 @@ final class SettingsPage 'warning', __('Es läuft bereits eine Divi-Prüfung; bitte in ein paar Minuten erneut versuchen.', 'wp-lit-mcp'), ], + 'divi_check_failed' => [ + 'error', + __('Die Divi-Prüfung ist abgebrochen; Details stehen im PHP-Error-Log.', 'wp-lit-mcp'), + ], 'divi_released' => ['success', __('Die Divi-Version ist für LIT MCP freigegeben.', 'wp-lit-mcp')], 'divi_release_refused' => [ 'error', diff --git a/src/Integrations/Divi/Release/DiviCheckRoute.php b/src/Integrations/Divi/Release/DiviCheckRoute.php index c1ae3df..2078f3b 100644 --- a/src/Integrations/Divi/Release/DiviCheckRoute.php +++ b/src/Integrations/Divi/Release/DiviCheckRoute.php @@ -7,6 +7,8 @@ namespace LIT\MCP\Integrations\Divi\Release; use Closure; use LIT\MCP\Admin\DiviTab; use LIT\MCP\Admin\SettingsPage; +use LIT\MCP\Log; +use Throwable; use WP_REST_Response; defined('ABSPATH') || exit; @@ -15,8 +17,12 @@ defined('ABSPATH') || exit; * "Prüfung starten" of the Divi tab (spec §6.1): POST /wp-json/lit-mcp/v1/divi-check. A REST request on purpose – * not admin-post – so the check runs in the same context as MCP calls (Divi renders its blocks in REST requests, * lit-page-render-summary). The form sends the wp_rest nonce as _wpnonce (cookie authentication); only manage_options. - * One check at a time: the option LOCK (add_option is atomic); a lock older than LOCK_TTL is a dead check's and taken - * over. Answers 303 back to the tab. + * One check at a time: an atomic "INSERT IGNORE" on the options table (self::LOCK) is the lock – add_option() reads + * then writes (wp-includes/option.php) and is not atomic, so two simultaneous requests could both "acquire" it. A + * lock older than LOCK_TTL is a dead check's, taken over with a conditional UPDATE (compare-and-swap on the value + * just read) so only one of several concurrent takers wins. A check that throws still releases the lock (finally) + * and redirects with divi_check_failed (logged, never shown to the browser as an unhandled 500). Answers 303 back + * to the tab. */ final class DiviCheckRoute { @@ -59,6 +65,9 @@ final class DiviCheckRoute ($this->reports ?? new CheckReportStore())->save($report); ReleaseGate::forget(); $notice = 'divi_checked'; + } catch (Throwable $e) { + Log::failure('divi check', $e); + $notice = 'divi_check_failed'; } finally { delete_option(self::LOCK); } @@ -73,16 +82,44 @@ final class DiviCheckRoute private static function lock(): bool { - if (add_option(self::LOCK, time(), '', false)) { + global $wpdb; + + if (self::insertLock($wpdb, time())) { return true; } - $since = (int) get_option(self::LOCK, 0); - if ($since > 0 && time() - $since < self::LOCK_TTL) { + // get_var(), not get_option(): the row was written with raw SQL, bypassing the options cache. + $since = $wpdb->get_var($wpdb->prepare( + "SELECT option_value FROM {$wpdb->options} WHERE option_name = %s", + self::LOCK + )); + if ($since === null) { + // The lock was released between our failed insert and this read; try once more. + return self::insertLock($wpdb, time()); + } + if (time() - (int) $since < self::LOCK_TTL) { return false; } - delete_option(self::LOCK); - return (bool) add_option(self::LOCK, time(), '', false); + $taken = $wpdb->query($wpdb->prepare( + "UPDATE {$wpdb->options} SET option_value = %s WHERE option_name = %s AND option_value = %s", + (string) time(), + self::LOCK, + $since + )); + + return (int) $taken === 1; + } + + /** INSERT IGNORE: 1 row affected = the lock did not exist and is now ours; 0 = someone else holds it. */ + private static function insertLock(\wpdb $wpdb, int $now): bool + { + $inserted = $wpdb->query($wpdb->prepare( + "INSERT IGNORE INTO {$wpdb->options} (option_name, option_value, autoload) VALUES (%s, %s, 'no')", + self::LOCK, + (string) $now + )); + + return (int) $inserted === 1; } } diff --git a/tests/Unit/Admin/DiviTabTest.php b/tests/Unit/Admin/DiviTabTest.php index 3e347ec..a26c1a1 100644 --- a/tests/Unit/Admin/DiviTabTest.php +++ b/tests/Unit/Admin/DiviTabTest.php @@ -73,6 +73,7 @@ final class DiviTabTest extends TestCase { $reports = [null, self::report(false), self::report(true, '5.2.9'), self::report(true, '5.3.0', 'old')]; foreach ($reports as $report) { + $this->redirect = null; $releases = Mockery::mock(ReleaseStore::class); $releases->expects('add')->never(); diff --git a/tests/Unit/Admin/SettingsPageTest.php b/tests/Unit/Admin/SettingsPageTest.php index 848a723..0cea111 100644 --- a/tests/Unit/Admin/SettingsPageTest.php +++ b/tests/Unit/Admin/SettingsPageTest.php @@ -179,6 +179,7 @@ final class SettingsPageTest extends TestCase return [ ['divi_checked', 'success', 'Divi-Prüfung abgeschlossen'], ['divi_check_running', 'warning', 'bereits eine Divi-Prüfung'], + ['divi_check_failed', 'error', 'PHP-Error-Log'], ['divi_released', 'success', 'für LIT MCP freigegeben'], ['divi_release_refused', 'error', 'Prüfung erneut ausführen'], ['divi_withdrawn', 'success', 'Freigabe zurückgezogen'], diff --git a/tests/Unit/Integrations/Divi/Release/DiviCheckRouteTest.php b/tests/Unit/Integrations/Divi/Release/DiviCheckRouteTest.php index 8c5cd4a..56c45ad 100644 --- a/tests/Unit/Integrations/Divi/Release/DiviCheckRouteTest.php +++ b/tests/Unit/Integrations/Divi/Release/DiviCheckRouteTest.php @@ -11,6 +11,7 @@ use LIT\MCP\Integrations\Divi\Release\CompatibilityCheck; use LIT\MCP\Integrations\Divi\Release\DiviCheckRoute; use LIT\MCP\Tests\Unit\TestCase; use Mockery; +use RuntimeException; final class DiviCheckRouteTest extends TestCase { @@ -24,6 +25,28 @@ final class DiviCheckRouteTest extends TestCase Functions\when('get_current_user_id')->justReturn(1); } + protected function tearDown(): void + { + unset($GLOBALS['wpdb']); + parent::tearDown(); + } + + /** Mock of the global $wpdb, its INSERT IGNORE returning $inserted rows affected. */ + private static function wpdb(int $inserted): \wpdb + { + $db = Mockery::mock(\wpdb::class)->makePartial(); + $db->expects('query')->once()->with(Mockery::on(static function (string $sql): bool { + return str_starts_with( + $sql, + "INSERT IGNORE INTO wp_options (option_name, option_value, autoload) " + . "VALUES ('lit_mcp_divi_check_lock', '" + ) && str_ends_with($sql, "', 'no')"); + }))->andReturn($inserted); + $GLOBALS['wpdb'] = $db; + + return $db; + } + public function testRegistersAPostRouteForAdministratorsOnly(): void { Functions\expect('register_rest_route')->once()->with( @@ -43,15 +66,12 @@ final class DiviCheckRouteTest extends TestCase public function testRunsTheCheckStoresTheReportAndRedirectsToTheTab(): void { + self::wpdb(1); $report = new CheckReport('r1', 'd', 1, '5.3.0', '0.6.0', 'k', []); $check = Mockery::mock(CompatibilityCheck::class); $check->expects('run')->with(1)->andReturn($report); $reports = Mockery::mock(CheckReportStore::class); $reports->expects('save')->with($report); - Functions\expect('add_option') - ->once() - ->with(DiviCheckRoute::LOCK, Mockery::type('int'), '', false) - ->andReturn(true); Functions\expect('delete_option')->once()->with(DiviCheckRoute::LOCK); $response = (new DiviCheckRoute(static fn () => $check, $reports))->handle(); @@ -63,8 +83,8 @@ final class DiviCheckRouteTest extends TestCase public function testASecondCheckWaitsForTheFirst(): void { - Functions\when('add_option')->justReturn(false); - Functions\when('get_option')->alias(static fn (): int => time()); + $db = self::wpdb(0); + $db->expects('get_var')->once()->andReturn((string) time()); Functions\expect('delete_option')->never(); $reports = Mockery::mock(CheckReportStore::class); $reports->expects('save')->never(); @@ -76,18 +96,66 @@ final class DiviCheckRouteTest extends TestCase public function testAStaleLockIsTakenOver(): void { - Functions\when('add_option')->alias(static function (): bool { - static $calls = 0; - - return ++$calls > 1; - }); - Functions\when('get_option')->justReturn(1); - Functions\expect('delete_option')->twice()->with(DiviCheckRoute::LOCK); + $db = self::wpdb(0); + $db->expects('get_var')->once()->andReturn('1'); + $db->expects('query')->once()->with(Mockery::on(static function (string $sql): bool { + return str_starts_with($sql, "UPDATE wp_options SET option_value = '") + && str_ends_with( + $sql, + "' WHERE option_name = 'lit_mcp_divi_check_lock' AND option_value = '1'" + ); + }))->andReturn(1); + Functions\expect('delete_option')->once()->with(DiviCheckRoute::LOCK); $check = Mockery::mock(CompatibilityCheck::class); $check->expects('run')->andReturn(new CheckReport('r1', 'd', 1, '5.3.0', '0.6.0', 'k', [])); $reports = Mockery::mock(CheckReportStore::class); $reports->expects('save'); - (new DiviCheckRoute(static fn () => $check, $reports))->handle(); + $response = (new DiviCheckRoute(static fn () => $check, $reports))->handle(); + + self::assertStringContainsString('lit_mcp_notice=divi_checked', $response->get_headers()['Location']); + } + + public function testATakeoverLostToAnotherRequestWaitsForIt(): void + { + $db = self::wpdb(0); + $db->expects('get_var')->once()->andReturn('1'); + $db->expects('query')->once()->with(Mockery::on(static fn (string $sql): bool => str_starts_with( + $sql, + "UPDATE wp_options SET option_value = '" + )))->andReturn(0); + Functions\expect('delete_option')->never(); + $reports = Mockery::mock(CheckReportStore::class); + $reports->expects('save')->never(); + + $response = (new DiviCheckRoute(static fn () => self::fail('no second check'), $reports))->handle(); + + self::assertStringContainsString('lit_mcp_notice=divi_check_running', $response->get_headers()['Location']); + } + + public function testTheLockIsReleasedAndTheCheckMarkedFailedWhenItThrows(): void + { + self::wpdb(1); + Functions\expect('delete_option')->once()->with(DiviCheckRoute::LOCK); + $check = Mockery::mock(CompatibilityCheck::class); + $check->expects('run')->andThrow(new RuntimeException('divi kaputt')); + $reports = Mockery::mock(CheckReportStore::class); + $reports->expects('save')->never(); + // error_log() is a PHP internal and cannot be stubbed; redirect it to a temp file instead. + $log = (string) tempnam(sys_get_temp_dir(), 'lit-mcp-log'); + $previous = ini_set('error_log', $log); + + try { + $response = (new DiviCheckRoute(static fn () => $check, $reports))->handle(); + } finally { + ini_set('error_log', (string) $previous); + } + + self::assertStringContainsString('lit_mcp_notice=divi_check_failed', $response->get_headers()['Location']); + self::assertStringContainsString( + 'divi check failed: RuntimeException: divi kaputt', + (string) file_get_contents($log) + ); + unlink($log); } }