fix: DesignLock gegen parallele Bestätigungen; confirm ohne design_rev ist ein Eingabefehler

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

src/Integrations/Divi/Design/DesignChange.php

@@ -19,10 +19,12 @@ defined('ABSPATH') || exit;
 
 /**
  * The change pipeline of every design system write (spec §4, G2, G6): plan on the live inventory → without
- * confirm a dry run (diff, affected posts, design_rev; nothing saved) → with confirm the design_rev must still match
+ * confirm a dry run (diff, affected posts, design_rev; nothing saved) → with confirm the design_rev is required, the
- * → no live post may block (P8/P8a) → release lock → snapshot of the area → write through Divi → read back and
+ * area's DesignLock is taken and the plan made again on the inventory read under the lock, whose design_rev must
- * verify → change log. A failed write or read-back writes the snapshot back (Rollback); when that fails too the
+ * still match → no live post may block (P8/P8a) → Divi release gate → snapshot of the area → write through Divi →
- * error is partial_write with the snapshot to restore by hand. The rights are the tool's (spec §10).
+ * read back and verify → change log; the lock is released at the end, also after a failure. A failed write or
+ * read-back writes the snapshot back (DesignArea::restore(), Rollback); when that fails too the error is
+ * partial_write with the snapshot to restore by hand. The rights are the tool's (spec §10).
  */
 final class DesignChange
 {
@@ -30,8 +32,11 @@ final class DesignChange
 
     private const LISTED_IDS = 10;
 
-    public function __construct(private SnapshotStore $snapshots, private ChangeLog $log)
+    public function __construct(
-    {
+        private SnapshotStore $snapshots,
+        private ChangeLog $log,
+        private DesignLock $lock = new DesignLock()
+    ) {
     }
 
     /** @return array<string, array<string, mixed>> Input properties every design write tool takes. */
@@ -66,6 +71,44 @@ final class DesignChange
      * @throws DiviLocked
      */
     public function run(string $ability, DesignArea $area, array $input, callable $plan): array|WP_Error
+    {
+        if (($input['confirm'] ?? false) !== true) {
+            return $this->attempt($ability, $area, null, $plan);
+        }
+
+        $rev = $input['design_rev'] ?? null;
+        if (! is_string($rev) || $rev === '') {
+            return Errors::invalidInput(
+                'design_rev fehlt; nichts wurde gespeichert.',
+                ['design_rev' => 'aus dem Probelauf übernehmen']
+            );
+        }
+
+        if (! $this->lock->acquire($area->name())) {
+            return Errors::make(
+                'conflict',
+                sprintf('Gerade läuft eine andere Änderung an den %s; nichts wurde gespeichert.', $area->label()),
+                'Kurz warten und den Probelauf wiederholen.',
+                409
+            );
+        }
+        try {
+            return $this->attempt($ability, $area, $rev, $plan);
+        } finally {
+            $this->lock->release($area->name());
+        }
+    }
+
+    /**
+     * Plans on the inventory read now – under the lock when confirmed ($confirmedRev) – and runs the dry run or
+     * the confirmed change.
+     *
+     * @param callable(array<array-key, mixed>): (DesignPlan|WP_Error) $plan
+     * @return array<string, mixed>|WP_Error
+     * @throws WriteFailed
+     * @throws DiviLocked
+     */
+    private function attempt(string $ability, DesignArea $area, ?string $confirmedRev, callable $plan): array|WP_Error
     {
         $current = $area->raw();
         $rev = $area->rev($current);
@@ -82,7 +125,7 @@ final class DesignChange
                 ->toArray();
         }
 
-        if (($input['confirm'] ?? false) !== true) {
+        if ($confirmedRev === null) {
             return Errors::confirmRequired(
                 $planned->summary . ' Nichts wurde gespeichert (Probelauf).',
                 ['diff' => $planned->diff, 'design_rev' => $rev, 'warnings' => $planned->warnings]
@@ -92,7 +135,7 @@ final class DesignChange
             );
         }
 
-        if (($input['design_rev'] ?? null) !== $rev) {
+        if ($confirmedRev !== $rev) {
             return Errors::revMismatch(
                 sprintf('Die %s wurden seit dem Probelauf geändert; nichts wurde gespeichert.', $area->label()),
                 'Erneut ohne confirm aufrufen (Probelauf), dem Nutzer die Änderung zeigen und mit der neuen '
@@ -100,6 +143,17 @@ final class DesignChange
             );
         }
 
+        return $this->apply($ability, $area, $planned, $current);
+    }
+
+    /**
+     * @param array<array-key, mixed> $current
+     * @return array<string, mixed>|WP_Error
+     * @throws WriteFailed
+     * @throws DiviLocked
+     */
+    private function apply(string $ability, DesignArea $area, DesignPlan $planned, array $current): array|WP_Error
+    {
         $blocked = $this->blocked($planned);
         if ($blocked !== null) {
             return $blocked;

src/Integrations/Divi/Design/DesignLock.php

@@ -0,0 +1,52 @@
+<?php
+
+declare(strict_types=1);
+
+namespace LIT\MCP\Integrations\Divi\Design;
+
+use InvalidArgumentException;
+
+defined('ABSPATH') || exit;
+
+/**
+ * One confirmed change per design area at a time (final review I-5): an option lit_mcp_lock_<area> (never
+ * autoloaded) holds the time the lock was taken. add_option() is atomic – it fails while the option exists. A lock
+ * older than STALE_AFTER seconds (a request that died) is removed and taken once more.
+ */
+final class DesignLock
+{
+    public const STALE_AFTER = 120;
+
+    private const PREFIX = 'lit_mcp_lock_';
+
+    /** @throws InvalidArgumentException For an area outside SnapshotStore::AREAS. */
+    public static function option(string $area): string
+    {
+        if (! in_array($area, SnapshotStore::AREAS, true)) {
+            throw new InvalidArgumentException(sprintf('Unknown design area "%s".', $area));
+        }
+
+        return self::PREFIX . $area;
+    }
+
+    public function acquire(string $area): bool
+    {
+        $option = self::option($area);
+        if (add_option($option, time(), '', false)) {
+            return true;
+        }
+
+        $since = get_option($option, 0);
+        if (is_numeric($since) && time() - (int) $since <= self::STALE_AFTER) {
+            return false;
+        }
+        delete_option($option);
+
+        return add_option($option, time(), '', false);
+    }
+
+    public function release(string $area): void
+    {
+        delete_option(self::option($area));
+    }
+}

src/Uninstaller.php

@@ -8,9 +8,9 @@ defined('ABSPATH') || exit;
 
 /**
  * Removes WP-LIT-MCP's own data (uninstall.php, Task 12): options (incl. the Snapshots des
- * Designsystems, Etappe 4), the endpoint-check and Divi-release transients, the log-cleanup
+ * Designsystems and their change locks, Etappe 4), the endpoint-check and Divi-release
- * cron hook and the change log table – on every site of a network – and the Protokoll tab's
+ * transients, the log-cleanup cron hook and the change log table – on every site of a
- * per-user screen option.
+ * network – and the Protokoll tab's per-user screen option.
  *
  * Deliberately self-contained (no reference to any other LIT\MCP\* class): uninstall.php
  * is executed by WordPress core standalone, without loading the plugin's main file first,
@@ -18,9 +18,10 @@ defined('ABSPATH') || exit;
  * names below are therefore duplicated from Access::OPTION, Abilities\ExposurePolicy::OPTION,
  * Limits::OPTION, ChangeLog\ChangeLogInstaller::VERSION_OPTION, Integrations\Divi\Release\
  * ReleaseStore::OPTION, Integrations\Divi\Release\CheckReportStore::OPTION, Integrations\
- * Divi\Release\DiviCheckRoute::LOCK, Admin\ConnectionStatus::ENDPOINT_TRANSIENT,
+ * Divi\Release\DiviCheckRoute::LOCK, Integrations\Divi\Design\DesignLock::option(),
- * Integrations\Divi\Release\ReleaseResolver::CACHE, ChangeLog\ChangeLogCleanup::HOOK,
+ * Admin\ConnectionStatus::ENDPOINT_TRANSIENT, Integrations\Divi\Release\ReleaseResolver::CACHE,
- * ChangeLog\ChangeLog::TABLE and Admin\LogTab::PER_PAGE_OPTION on purpose;
+ * ChangeLog\ChangeLogCleanup::HOOK, ChangeLog\ChangeLog::TABLE and Admin\LogTab::PER_PAGE_OPTION
+ * on purpose;
  * tests/Unit/UninstallerTest.php and tests/Unit/UninstallTest.php keep them in sync with
  * those constants.
  *
@@ -36,6 +37,7 @@ final class Uninstaller
         'lit_mcp_divi_releases', 'lit_mcp_divi_check_report', 'lit_mcp_divi_check_lock',
         'lit_mcp_snapshots_colors', 'lit_mcp_snapshots_variables', 'lit_mcp_snapshots_presets',
         'lit_mcp_snapshots_theme_options',
+        'lit_mcp_lock_colors', 'lit_mcp_lock_variables', 'lit_mcp_lock_presets', 'lit_mcp_lock_theme_options',
     ];
     private const TRANSIENTS = ['lit_mcp_endpoint_check', 'lit_mcp_divi_state'];
     private const CRON_HOOK = 'lit_mcp_log_cleanup';

tests/Unit/Integrations/Divi/Design/DesignChangeTest.php

@@ -6,6 +6,7 @@ namespace LIT\MCP\Tests\Unit\Integrations\Divi\Design;
 
 use LIT\MCP\Integrations\Divi\Design\DesignArea;
 use LIT\MCP\Integrations\Divi\Design\DesignChange;
+use LIT\MCP\Integrations\Divi\Design\DesignLock;
 use LIT\MCP\Integrations\Divi\Design\DesignPlan;
 use LIT\MCP\Integrations\Divi\Design\SnapshotStore;
 use LIT\MCP\Integrations\Divi\Design\SnapshotTooLarge;
@@ -17,11 +18,35 @@ use LIT\MCP\Integrations\Divi\Release\ReleaseStatus;
 use LIT\MCP\Abilities\WriteFailed;
 use LIT\MCP\Tests\Support\FakeChangeLog;
 use LIT\MCP\Tests\Unit\TestCase;
+use Brain\Monkey\Functions;
 use Mockery;
 use RuntimeException;
 
 final class DesignChangeTest extends TestCase
 {
+    /** @var array<string, mixed> Options as add_option()/delete_option() see them (DesignLock). */
+    private array $options = [];
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->options = [];
+        Functions\when('add_option')->alias(function (string $name, mixed $value = ''): bool {
+            if (array_key_exists($name, $this->options)) {
+                return false;
+            }
+            $this->options[$name] = $value;
+
+            return true;
+        });
+        Functions\when('get_option')->alias(fn (string $name, mixed $default = false): mixed => $this->options[$name] ?? $default);
+        Functions\when('delete_option')->alias(function (string $name): bool {
+            unset($this->options[$name]);
+
+            return true;
+        });
+    }
+
     /**
      * A DesignArea over an array; the first $failWrites calls of write()/restore() throw, $differences are the
      * value warnings of differences().
@@ -164,6 +189,118 @@ final class DesignChangeTest extends TestCase
         self::assertSame([], $area->writes);
     }
 
+    /** Final review M-9: confirm without design_rev is a missing input, not a stale one. */
+    public function testConfirmWithoutDesignRevIsInvalidInput(): void
+    {
+        $stored = ['v' => 1];
+        $area = self::area($stored);
+        $snapshots = Mockery::mock(SnapshotStore::class);
+        $snapshots->shouldNotReceive('save');
+
+        $result = (new DesignChange($snapshots, FakeChangeLog::forbidden()))
+            ->run('lit/divi-presets-set', $area, ['confirm' => true], self::plan(['v' => 2]));
+
+        $details = self::assertToolError($result, 'validation_failed');
+        self::assertSame('design_rev fehlt; nichts wurde gespeichert.', $details['message']);
+        self::assertSame('aus dem Probelauf übernehmen', $details['fields']['design_rev']);
+        self::assertSame([], $area->writes);
+    }
+
+    /** Final review I-5: a confirm while another change of the area runs writes nothing. */
+    public function testAHeldLockRefusesTheConfirm(): void
+    {
+        $this->options['lit_mcp_lock_presets'] = time();
+        $stored = ['v' => 1];
+        $area = self::area($stored);
+        $snapshots = Mockery::mock(SnapshotStore::class);
+        $snapshots->shouldNotReceive('save');
+
+        $result = (new DesignChange($snapshots, FakeChangeLog::forbidden(), new DesignLock()))->run(
+            'lit/divi-presets-set',
+            $area,
+            ['confirm' => true, 'design_rev' => $area->rev($stored)],
+            self::plan(['v' => 2])
+        );
+
+        self::assertToolError($result, 'conflict');
+        self::assertSame(409, $result->get_error_data()['status']);
+        self::assertStringContainsString('andere Änderung an den Presets', $result->get_error_message());
+        self::assertSame([], $area->writes);
+        self::assertArrayHasKey('lit_mcp_lock_presets', $this->options, 'a foreign lock is not released');
+    }
+
+    public function testTheLockIsHeldWhileWritingAndReleasedAfterSuccess(): void
+    {
+        $stored = ['v' => 1];
+        $area = self::area($stored);
+        $snapshots = Mockery::mock(SnapshotStore::class);
+        $snapshots->expects('save')->andReturnUsing(function (): string {
+            self::assertArrayHasKey('lit_mcp_lock_presets', $this->options, 'snapshot under the lock');
+
+            return 'presets-1';
+        });
+        $snapshots->expects('attachLog')->andReturnUsing(function (): void {
+            self::assertArrayHasKey('lit_mcp_lock_presets', $this->options, 'log under the lock');
+        });
+
+        $result = (new DesignChange($snapshots, new FakeChangeLog(9)))->run(
+            'lit/divi-presets-set',
+            $area,
+            ['confirm' => true, 'design_rev' => $area->rev($stored)],
+            self::plan(['v' => 2])
+        );
+
+        self::assertTrue($result['ok']);
+        self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->options);
+    }
+
+    public function testTheLockIsReleasedAfterAFailedWrite(): void
+    {
+        $stored = ['v' => 1];
+        $area = self::area($stored, 1);
+        $snapshots = Mockery::mock(SnapshotStore::class);
+        $snapshots->expects('save')->andReturn('presets-1');
+
+        try {
+            (new DesignChange($snapshots, FakeChangeLog::forbidden()))->run(
+                'lit/divi-presets-set',
+                $area,
+                ['confirm' => true, 'design_rev' => $area->rev($stored)],
+                self::plan(['v' => 2])
+            );
+            self::fail('expected WriteFailed');
+        } catch (WriteFailed) {
+            self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->options);
+        }
+    }
+
+    /** The design_rev is checked against the inventory read under the lock, not before it. */
+    public function testTheRevIsCheckedOnTheInventoryReadUnderTheLock(): void
+    {
+        $stored = ['v' => 1];
+        $area = self::area($stored);
+        $rev = $area->rev($stored);
+        $snapshots = Mockery::mock(SnapshotStore::class);
+        $snapshots->shouldNotReceive('save');
+        Functions\when('add_option')->alias(function (string $name, mixed $value = '') use (&$stored): bool {
+            $stored = ['v' => 'parallel'];
+            $this->options[$name] = $value;
+
+            return true;
+        });
+
+        $result = (new DesignChange($snapshots, FakeChangeLog::forbidden()))->run(
+            'lit/divi-presets-set',
+            $area,
+            ['confirm' => true, 'design_rev' => $rev],
+            self::plan(['v' => 2])
+        );
+
+        self::assertToolError($result, 'rev_mismatch');
+        self::assertSame([], $area->writes);
+        self::assertArrayNotHasKey('lit_mcp_lock_presets', $this->options);
+    }
+
     public function testBlockedUsagesRefuseTheConfirm(): void
     {
         $stored = ['v' => 1];

tests/Unit/Integrations/Divi/Design/DesignLockTest.php

@@ -0,0 +1,73 @@
+<?php
+
+declare(strict_types=1);
+
+namespace LIT\MCP\Tests\Unit\Integrations\Divi\Design;
+
+use Brain\Monkey\Functions;
+use InvalidArgumentException;
+use LIT\MCP\Integrations\Divi\Design\DesignLock;
+use LIT\MCP\Tests\Unit\TestCase;
+
+final class DesignLockTest extends TestCase
+{
+    /** @var array<string, mixed> */
+    private array $options = [];
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+        $this->options = [];
+        Functions\when('add_option')->alias(function (string $name, mixed $value = '', string $unused = '', mixed $autoload = null): bool {
+            self::assertFalse($autoload, 'locks are never autoloaded');
+            if (array_key_exists($name, $this->options)) {
+                return false;
+            }
+            $this->options[$name] = $value;
+
+            return true;
+        });
+        Functions\when('get_option')->alias(fn (string $name, mixed $default = false): mixed => $this->options[$name] ?? $default);
+        Functions\when('delete_option')->alias(function (string $name): bool {
+            unset($this->options[$name]);
+
+            return true;
+        });
+    }
+
+    public function testAcquireIsExclusiveUntilReleased(): void
+    {
+        $lock = new DesignLock();
+
+        self::assertTrue($lock->acquire('presets'));
+        self::assertIsInt($this->options['lit_mcp_lock_presets']);
+        self::assertFalse($lock->acquire('presets'));
+        self::assertTrue($lock->acquire('colors'), 'areas lock independently');
+
+        $lock->release('presets');
+        self::assertTrue($lock->acquire('presets'));
+    }
+
+    public function testAStaleLockIsTakenOver(): void
+    {
+        $this->options['lit_mcp_lock_presets'] = time() - DesignLock::STALE_AFTER - 1;
+
+        self::assertTrue((new DesignLock())->acquire('presets'));
+        self::assertGreaterThan(time() - 5, $this->options['lit_mcp_lock_presets']);
+    }
+
+    public function testAFreshLockIsNotTakenOver(): void
+    {
+        $this->options['lit_mcp_lock_presets'] = time() - 10;
+
+        self::assertFalse((new DesignLock())->acquire('presets'));
+    }
+
+    public function testOptionNamesAndUnknownAreas(): void
+    {
+        self::assertSame('lit_mcp_lock_theme_options', DesignLock::option('theme_options'));
+
+        $this->expectException(InvalidArgumentException::class);
+        DesignLock::option('pages');
+    }
+}

tests/Unit/Integrations/Divi/Design/DiviDesignRestoreAbilityTest.php

@@ -15,6 +15,14 @@ use Mockery;
 
 final class DiviDesignRestoreAbilityTest extends TestCase
 {
+    protected function setUp(): void
+    {
+        parent::setUp();
+        // DesignLock (final review I-5): the lock is always free here.
+        Functions\when('add_option')->justReturn(true);
+        Functions\when('delete_option')->justReturn(true);
+    }
+
     public function testRestoresWithDryRunAndASnapshotOfTheCurrentState(): void
     {
         Functions\when('current_user_can')->justReturn(true);

tests/Unit/Integrations/Divi/Design/DiviPresetsDeleteAbilityTest.php

@@ -17,6 +17,14 @@ use Mockery;
 
 final class DiviPresetsDeleteAbilityTest extends TestCase
 {
+    protected function setUp(): void
+    {
+        parent::setUp();
+        // DesignLock (final review I-5): the lock is always free here.
+        Functions\when('add_option')->justReturn(true);
+        Functions\when('delete_option')->justReturn(true);
+    }
+
     private function ability(UsageReport $report, ?DesignSystemWriter $writer = null, ?SnapshotStore $snapshots = null): DiviPresetsDeleteAbility
     {
         Functions\when('current_user_can')->justReturn(true);

tests/Unit/Integrations/Divi/Design/DiviPresetsSetAbilityTest.php

@@ -36,6 +36,9 @@ final class DiviPresetsSetAbilityTest extends TestCase
         $this->stored = PresetBookTest::stored();
         $this->snapshots = Mockery::mock(SnapshotStore::class);
         Functions\when('current_user_can')->justReturn(true);
+        // DesignLock (final review I-5): the lock is always free here.
+        Functions\when('add_option')->justReturn(true);
+        Functions\when('delete_option')->justReturn(true);
     }
 
     private function ability(bool $unfiltered = true, ?GlobalUsages $usages = null, ?DiviPageLoader $loader = null, ?DesignSystemWriter $writer = null): DiviPresetsSetAbility

tests/Unit/UninstallTest.php

@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace LIT\MCP\Tests\Unit;
 
+use LIT\MCP\Integrations\Divi\Design\DesignLock;
 use Brain\Monkey\Functions;
 use LIT\MCP\Abilities\ExposurePolicy;
 use LIT\MCP\Access;
@@ -57,6 +58,8 @@ final class UninstallTest extends TestCase
             ReleaseStore::OPTION, CheckReportStore::OPTION, DiviCheckRoute::LOCK,
             'lit_mcp_snapshots_colors', 'lit_mcp_snapshots_variables', 'lit_mcp_snapshots_presets',
             'lit_mcp_snapshots_theme_options',
+            DesignLock::option('colors'), DesignLock::option('variables'), DesignLock::option('presets'),
+            DesignLock::option('theme_options'),
         ];
         self::assertSame(array_merge($own, $own), $deleted);
         self::assertNotContains('mcp_jwt_secret', $deleted);

tests/Unit/UninstallerTest.php

@@ -4,6 +4,7 @@ declare(strict_types=1);
 
 namespace LIT\MCP\Tests\Unit;
 
+use LIT\MCP\Integrations\Divi\Design\DesignLock;
 use Brain\Monkey\Functions;
 use LIT\MCP\Abilities\ExposurePolicy;
 use LIT\MCP\Access;
@@ -63,6 +64,8 @@ final class UninstallerTest extends TestCase
                 ReleaseStore::OPTION, CheckReportStore::OPTION, DiviCheckRoute::LOCK,
                 'lit_mcp_snapshots_colors', 'lit_mcp_snapshots_variables', 'lit_mcp_snapshots_presets',
                 'lit_mcp_snapshots_theme_options',
+                DesignLock::option('colors'), DesignLock::option('variables'), DesignLock::option('presets'),
+                DesignLock::option('theme_options'),
             ],
             $deleted
         );