fix(firewall): serialize rule writes to kill SQLite lock races (#1020)

Alexey Portnov · Alexey Portnov · commit 36c1813a6925 · 2026-04-15T21:23:44.000+08:00
IptablesConf::updateFirewallRules() looped `$rule->update()` with no mutex or transaction, so ReloadFirewallAction running in WorkerModelsEvents raced with WorkerApiCommands writers and tripped "database is locked" (Sentry MIKOPBX-KMK/MIKOPBX-KMM, 24 events across 3+ installs on 2026.1.223). Route the batch through BaseActionHelper::executeInTransaction() so it takes the shared 'db-write' Redis mutex and runs in a single SQLite transaction. Rules are materialized first and the mutex/transaction is only entered when there is real work to do — the common no-op path stays free of Redis round-trips. Guard the upgrade path with isMutexAvailable(): UpdateSystemConfig calls updateFirewallRules() during release upgrades, when Redis may not yet be reachable. The probe forces a real Redis round-trip via isLocked(); on failure it logs a warning and falls back to direct per-row save(). Upgrade is single-threaded in that phase, so the fallback cannot race. Probe failures are not cached — transient Redis outages auto-recover on the next call. Also route Firewall SaveRecordAction through executeInTransaction(): it previously called $db->begin()/commit()/rollback() directly, without the 'db-write' mutex, so it contended with every other writer. Validation failures now raise \DomainException as a sentinel — the outer catch preserves the original API contract (individual Phalcon messages pushed to $res->messages['error'], no extra syslog noise), while infrastructure errors still flow through handleError(). Verified on boffart.miko.ru (2026.1.233-dev) with 5 parallel PHP stress workers × 15 iterations + 5 concurrent REST PATCH: - baseline (pre-fix): 1 ok / 74 fail, 74 PDOException database-locked - with fix: 75 ok / 0 fail, 0 locks, 5/5 REST 200 OK - fallback (Redis down): warning logged, direct writes succeeded Closes #1020
diff --git a/src/Core/System/Configs/IptablesConf.php b/src/Core/System/Configs/IptablesConf.php
@@ -21,6 +21,7 @@
 namespace MikoPBX\Core\System\Configs;
 
 use MikoPBX\Common\Models\{FirewallRules, NetworkFilters, PbxSettings, Sip};
+use MikoPBX\Common\Providers\MutexProvider;
 use MikoPBX\Common\Providers\PBXConfModulesProvider;
 use MikoPBX\Core\Asterisk\Configs\SIPConf;
 use MikoPBX\Core\System\Processes;
@@ -29,6 +30,8 @@
 use MikoPBX\Core\System\Util;
 use MikoPBX\Core\Utilities\IpAddressHelper;
 use MikoPBX\Modules\Config\SystemConfigInterface;
+use MikoPBX\PBXCoreREST\Lib\Common\BaseActionHelper;
+use Phalcon\Di\Di;
 use Phalcon\Di\Injectable;
 
 /**
@@ -451,7 +454,16 @@ private function makeCmdMultiportBySubnet(array $options, array &$arr_command):
     }
 
     /**
-     * Updates firewall rules according to default template
+     * Updates firewall rules according to default template.
+     *
+     * Normally runs inside the shared 'db-write' mutex + SQLite transaction so
+     * the batch update cannot race with parallel API writes from
+     * WorkerApiCommands (see GitHub #1020 and Sentry MIKOPBX-KMK/MIKOPBX-KMM).
+     *
+     * When MutexProvider/Redis is not yet reachable — as happens during early
+     * boot and the release-upgrade path (UpdateSystemConfig) — this method
+     * degrades to a direct per-row save without transaction. Upgrade code is
+     * single-threaded in that phase, so there is no writer to race with.
      */
     public static function updateFirewallRules(): void
     {
@@ -465,15 +477,80 @@ public static function updateFirewallRules(): void
             ],
         ];
         $rules = FirewallRules::find($conditions);
+
+        // Materialize only rules that actually change, so we skip
+        // taking the mutex and opening a transaction on the common no-op path.
+        $toUpdate = [];
         foreach ($rules as $rule) {
             $from = $portSet[$rule->portFromKey] ?? '0';
-            $to = $portSet[$rule->portToKey] ?? '0';
+            $to   = $portSet[$rule->portToKey] ?? '0';
             if ($from === $rule->portfrom && $to === $rule->portto) {
                 continue;
             }
             $rule->portfrom = $from;
-            $rule->portto = $to;
-            $rule->update();
+            $rule->portto   = $to;
+            $toUpdate[]     = $rule;
+        }
+
+        if (empty($toUpdate)) {
+            return;
+        }
+
+        $applyUpdates = static function () use ($toUpdate): void {
+            foreach ($toUpdate as $rule) {
+                if (!$rule->save()) {
+                    $messages = array_map(
+                        static fn($m) => (string)$m->getMessage(),
+                        $rule->getMessages()
+                    );
+                    throw new \RuntimeException(
+                        'IptablesConf::updateFirewallRules failed: ' . implode(', ', $messages)
+                    );
+                }
+            }
+        };
+
+        if (self::isMutexAvailable()) {
+            BaseActionHelper::executeInTransaction($applyUpdates);
+            return;
+        }
+
+        // Fallback: early boot / release upgrade — Redis not ready yet.
+        $applyUpdates();
+    }
+
+    /**
+     * Probe MutexProvider / Redis availability.
+     *
+     * Used by updateFirewallRules() to decide between the mutex-guarded path
+     * (runtime) and the direct-write fallback (early boot / upgrade). A failure
+     * here is not cached so transient Redis outages auto-recover on the next
+     * call.
+     */
+    private static function isMutexAvailable(): bool
+    {
+        try {
+            $di = Di::getDefault();
+            if ($di === null || !$di->has(MutexProvider::SERVICE_NAME)) {
+                return false;
+            }
+
+            /** @var MutexProvider $mutex */
+            $mutex = $di->get(MutexProvider::SERVICE_NAME);
+
+            // Force a real Redis round-trip so providers that defer the
+            // connection until first use surface their failure here.
+            $mutex->isLocked('iptables-conf-probe');
+
+            return true;
+        } catch (\Throwable $e) {
+            SystemMessages::sysLogMsg(
+                __METHOD__,
+                'Mutex/Redis unavailable, falling back to direct firewall rule writes: '
+                . $e->getMessage(),
+                LOG_WARNING
+            );
+            return false;
         }
     }
 
diff --git a/src/PBXCoreREST/Lib/Firewall/SaveRecordAction.php b/src/PBXCoreREST/Lib/Firewall/SaveRecordAction.php
@@ -26,7 +26,6 @@
 use MikoPBX\Core\Utilities\IpAddressHelper;
 use MikoPBX\PBXCoreREST\Lib\Common\AbstractSaveRecordAction;
 use MikoPBX\PBXCoreREST\Lib\PBXApiResult;
-use Phalcon\Di\Di;
 
 /**
  * ✨ REFERENCE IMPLEMENTATION: Firewall Save Action (Security Critical)
@@ -219,100 +218,98 @@ public static function main(array $data): PBXApiResult
 
         // ============================================================
         // PHASE 6: SAVE TO DATABASE
-        // WHY: All-or-nothing transaction for NetworkFilter + FirewallRules
+        // All writes run inside shared 'db-write' mutex + transaction
+        // via BaseActionHelper::executeInTransaction(). This avoids
+        // "database is locked" races with WorkerModelsEvents / IptablesConf
+        // (see GitHub #1020, Sentry MIKOPBX-KMK/MIKOPBX-KMM).
         // ============================================================
 
-        $di = Di::getDefault();
-        if ($di === null) {
-            $res->messages['error'][] = 'DI container is not available';
-            $res->httpCode = 500;
-            return $res;
-        }
-        $db = $di->get('db');
-
         try {
-            $db->begin();
-
-            // Process network/subnet fields to calculate permit (CIDR notation)
-            if (isset($sanitizedData['network']) && isset($sanitizedData['subnet'])) {
-                $network = $sanitizedData['network'];
-                $subnet = $sanitizedData['subnet'];
-
-                // Detect IP version to apply correct normalization
-                $version = IpAddressHelper::getIpVersion($network);
-
-                if ($version === IpAddressHelper::IP_VERSION_4) {
-                    // IPv4: Normalize network address using Cidr calculator
-                    $calculator = new Cidr();
-                    $normalizedNetwork = $calculator->cidr2network($network, intval($subnet));
-                    $networkFilter->permit = $normalizedNetwork . '/' . $subnet;
-                } else {
-                    // IPv6: Use address as-is (already validated in validateIpAndCidr)
-                    $networkFilter->permit = $network . '/' . $subnet;
+            self::executeInTransaction(function () use (
+                $networkFilter,
+                $sanitizedData,
+                $isNewRecord,
+                $httpMethod,
+                $res
+            ): void {
+                // Process network/subnet fields to calculate permit (CIDR notation)
+                if (isset($sanitizedData['network']) && isset($sanitizedData['subnet'])) {
+                    $network = $sanitizedData['network'];
+                    $subnet  = $sanitizedData['subnet'];
+
+                    // Detect IP version to apply correct normalization
+                    $version = IpAddressHelper::getIpVersion($network);
+
+                    if ($version === IpAddressHelper::IP_VERSION_4) {
+                        // IPv4: Normalize network address using Cidr calculator
+                        $calculator = new Cidr();
+                        $normalizedNetwork = $calculator->cidr2network($network, intval($subnet));
+                        $networkFilter->permit = $normalizedNetwork . '/' . $subnet;
+                    } else {
+                        // IPv6: Use address as-is (already validated in validateIpAndCidr)
+                        $networkFilter->permit = $network . '/' . $subnet;
+                    }
                 }
-            }
 
-            // Convert boolean fields to '0'/'1' strings
-            $sanitizedData = self::convertBooleanFields($sanitizedData, ['newer_block_ip', 'local_network']);
+                // Convert boolean fields to '0'/'1' strings
+                $convertedData = self::convertBooleanFields(
+                    $sanitizedData,
+                    ['newer_block_ip', 'local_network']
+                );
 
-            // Update NetworkFilter fields
-            // For CREATE: All fields from $sanitizedData (with defaults)
-            // For PATCH: Only provided fields (no defaults)
-            if (isset($sanitizedData['deny'])) {
-                $networkFilter->deny = $sanitizedData['deny'];
-            }
-            if (isset($sanitizedData['description'])) {
-                $networkFilter->description = $sanitizedData['description'];
-            }
-            if (isset($sanitizedData['newer_block_ip'])) {
-                $networkFilter->newer_block_ip = $sanitizedData['newer_block_ip'];
-            }
-            if (isset($sanitizedData['local_network'])) {
-                $networkFilter->local_network = $sanitizedData['local_network'];
-            }
+                // Update NetworkFilter fields
+                // For CREATE: All fields from $convertedData (with defaults)
+                // For PATCH: Only provided fields (no defaults)
+                if (isset($convertedData['deny'])) {
+                    $networkFilter->deny = $convertedData['deny'];
+                }
+                if (isset($convertedData['description'])) {
+                    $networkFilter->description = $convertedData['description'];
+                }
+                if (isset($convertedData['newer_block_ip'])) {
+                    $networkFilter->newer_block_ip = $convertedData['newer_block_ip'];
+                }
+                if (isset($convertedData['local_network'])) {
+                    $networkFilter->local_network = $convertedData['local_network'];
+                }
 
-            if (!$networkFilter->save()) {
-                $errors = $networkFilter->getMessages();
-                foreach ($errors as $error) {
-                    $res->messages['error'][] = $error->getMessage();
+                if (!$networkFilter->save()) {
+                    // Preserve per-field error messages as individual array
+                    // entries so the frontend can still display them one by
+                    // one. Throwing \DomainException signals a validation
+                    // failure (not an infrastructure error) — the outer catch
+                    // handles it without pushing another combined message.
+                    foreach ($networkFilter->getMessages() as $message) {
+                        $res->messages['error'][] = $message->getMessage();
+                    }
+                    throw new \DomainException('Network filter validation failed');
                 }
-                $db->rollback();
-                $res->success = false;
-                return $res;
-            }
 
-            // Update FirewallRules if currentRules data is provided
-            if (isset($sanitizedData['currentRules']) && is_array($sanitizedData['currentRules'])) {
-                if ($isNewRecord) {
-                    // CREATE: Create new rules
-                    if (!self::createFirewallRules($networkFilter->id, $sanitizedData['currentRules'])) {
-                        $res->messages['error'][] = 'Failed to create firewall rules';
-                        $db->rollback();
-                        $res->success = false;
-                        return $res;
+                // Update FirewallRules if currentRules data is provided
+                if (isset($sanitizedData['currentRules']) && is_array($sanitizedData['currentRules'])) {
+                    if ($isNewRecord) {
+                        // CREATE: Create new rules
+                        if (!self::createFirewallRules($networkFilter->id, $sanitizedData['currentRules'])) {
+                            $res->messages['error'][] = 'Failed to create firewall rules';
+                            throw new \DomainException('createFirewallRules returned false');
+                        }
+                    } else {
+                        // UPDATE/PATCH: Update existing rules
+                        // WHY: PATCH = partial update (only provided fields), PUT = full update (all fields)
+                        $isPatch = ($httpMethod === 'PATCH');
+                        if (!self::updateFirewallRules($networkFilter->id, $sanitizedData['currentRules'], $isPatch)) {
+                            $res->messages['error'][] = 'Failed to update firewall rules';
+                            throw new \DomainException('updateFirewallRules returned false');
+                        }
                     }
-                } else {
-                    // UPDATE/PATCH: Update existing rules
-                    // WHY: PATCH = partial update (only provided fields), PUT = full update (all fields)
-                    $isPatch = ($httpMethod === 'PATCH');
-                    if (!self::updateFirewallRules($networkFilter->id, $sanitizedData['currentRules'], $isPatch)) {
-                        $res->messages['error'][] = 'Failed to update firewall rules';
-                        $db->rollback();
-                        $res->success = false;
-                        return $res;
+                } elseif ($isNewRecord) {
+                    // CREATE without rules: Use defaults
+                    if (!self::createFirewallRules($networkFilter->id, [])) {
+                        $res->messages['error'][] = 'Failed to create default firewall rules';
+                        throw new \DomainException('createFirewallRules (defaults) returned false');
                     }
                 }
-            } elseif ($isNewRecord) {
-                // CREATE without rules: Use defaults
-                if (!self::createFirewallRules($networkFilter->id, [])) {
-                    $res->messages['error'][] = 'Failed to create default firewall rules';
-                    $db->rollback();
-                    $res->success = false;
-                    return $res;
-                }
-            }
-
-            $db->commit();
+            });
 
             // ============================================================
             // PHASE 7: BUILD RESPONSE
@@ -324,10 +321,18 @@ public static function main(array $data): PBXApiResult
             $res->httpCode = $isNewRecord ? 201 : 200; // 201 Created, 200 OK
             $res->reload = "firewall/modify/{$networkFilter->id}";
 
-            self::logSuccessfulSave('Firewall rule', $networkFilter->description ?: $networkFilter->permit, (string)$networkFilter->id, __METHOD__);
-
+            self::logSuccessfulSave(
+                'Firewall rule',
+                $networkFilter->description ?: $networkFilter->permit,
+                (string)$networkFilter->id,
+                __METHOD__
+            );
+        } catch (\DomainException) {
+            // Validation/business failure — messages already populated inside
+            // the callback, rollback already performed by executeInTransaction.
+            // Deliberately swallow the exception (no syslog, no extra message).
+            $res->success = false;
         } catch (\Exception $e) {
-            $db->rollback();
             return self::handleError($e, $res);
         }
 
