Introduce time limit for mailman sync locking mechanism to avoid permanent failures

.env.dist

@@ -96,3 +96,7 @@ PHP_IDE_CONFIG=serverName=database.gewis.nl
 # Demo mode credentials (this will prepopulate the credential fields with these values)
 DEMO_CREDENTIALS_USERNAME=admin
 DEMO_CREDENTIALS_PASSWORD=gewisdbgewis
+
+# Timezone (incl Postfix)
+TZ=Europe/Amsterdam
+PGTZ=Europe/Amsterdam

Makefile

@@ -44,10 +44,6 @@ migrate: replenish
 migrate-to:
 		@docker compose exec -u www-data web sh -c '. ./scripts/migrate-version.sh && ./orm migrations:migrate $$migrations --object-manager doctrine.entitymanager.$$alias'
 
-migration-list: replenish
-		@docker compose exec -u www-data -T web ./orm migrations:list --object-manager doctrine.entitymanager.orm_default
-		@docker compose exec -u www-data -T web ./orm migrations:list --object-manager doctrine.entitymanager.orm_report
-
 migration-diff: replenish
 		@docker compose exec -u root web chown www-data:www-data /code/module/Database/migrations/
 		@docker compose exec -u www-data -T web ./orm migrations:diff --object-manager doctrine.entitymanager.orm_default
@@ -58,10 +54,10 @@ migration-diff: replenish
 		@docker cp "$(shell docker compose ps -q web)":/code/module/Report/migrations ./module/Report
 		@docker compose exec -u root web chown -R root:root /code/module/Report/migrations/
 
-migration-up: replenish migration-list
+migration-up: replenish
 		@docker compose exec -u www-data web sh -c '. ./scripts/migrate-version.sh && ./orm migrations:execute --up $$migrations --object-manager doctrine.entitymanager.$$alias'
 
-migration-down: replenish migration-list
+migration-down: replenish
 		@docker compose exec -u www-data web sh -c '. ./scripts/migrate-version.sh && ./orm migrations:execute --down $$migrations --object-manager doctrine.entitymanager.$$alias'
 
 seed: replenish

module/Application/src/Model/ConfigItem.php

@@ -6,6 +6,7 @@
 
 use Application\Model\Enums\ConfigNamespaces;
 use Database\Model\Trait\TimestampableTrait;
+use Database\Model\Trait\VersionTrait;
 use DateTime;
 use Doctrine\ORM\Mapping\Column;
 use Doctrine\ORM\Mapping\Entity;
@@ -34,6 +35,11 @@
 class ConfigItem
 {
     use TimestampableTrait;
+    // We implement locking by using version numbers (optimistic locking)
+    // rather than by banning other processes from locking the same row.
+    // This is more versatile and is possible because we do not care which
+    // process in the end changes the config, as long as it is only one.
+    use VersionTrait;
 
     /**
      * Primary key item ID (to avoid reference issues).

module/Database/migrations/Version20251207150548.php

@@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Database\Migrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+final class Version20251207150548 extends AbstractMigration
+{
+    public function getDescription(): string
+    {
+        return 'Implement versioning of config items for optimistic locking';
+    }
+
+    public function up(Schema $schema): void
+    {
+        $this->addSql('ALTER TABLE configitem ADD version INT DEFAULT 1000 NOT NULL');
+    }
+
+    public function down(Schema $schema): void
+    {
+        $this->addSql('ALTER TABLE ConfigItem DROP version');
+    }
+}

module/Database/migrations/Version20260211203320.php

@@ -0,0 +1,31 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Database\Migrations;
+
+use DateTime;
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * phpcs:disable Generic.Files.LineLength.TooLong
+ * phpcs:disable SlevomatCodingStandard.Functions.RequireMultiLineCall.RequiredMultiLineCall
+ */
+final class Version20260211203320 extends AbstractMigration
+{
+    public function getDescription(): string
+    {
+        return 'Change config item for mailman sync to date value';
+    }
+
+    public function up(Schema $schema): void
+    {
+        $this->addSql('UPDATE configitem SET valuebool = null, valuedate = updatedat + interval \'23h\' * valuebool::int WHERE key = \'locked\' AND namespace = \'database_mailman\'');
+    }
+
+    public function down(Schema $schema): void
+    {
+        $this->addSql('UPDATE configitem SET valuebool = valuedate > \'' . (new DateTime())->format('Y-m-d H:i:s') . '\', valuedate = null WHERE key = \'locked\' AND namespace = \'database_mailman\'');
+    }
+}

module/Database/src/Model/Trait/VersionTrait.php

@@ -0,0 +1,20 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Database\Model\Trait;
+
+use Doctrine\ORM\Mapping\Column;
+use Doctrine\ORM\Mapping\Version;
+
+trait VersionTrait
+{
+    /**
+     * integer version
+     * From the docs:
+     * "Version numbers [should] be preferred as they can not potentially conflict in a highly concurrent environment"
+     */
+    #[Version()]
+    #[Column(type: 'integer')]
+    private int $version;
+}

module/Database/src/Service/Mailman.php

@@ -130,20 +130,29 @@ private function performMailmanRequest(
      * Acquire sync lock.
      *
      * To ensure that the sync between GEWISDB and Mailman is as clean as possible, we need to acquire a global lock on
-     * the mail list administration. This will prevent (if properly implemented and used) the secretary from modifying
-     * any mailing list memberships.
+     * the mail list administration. This will prevent (if properly implemented and used) concurrent syncs from running.
      */
-    private function acquireSyncLock(int $retries = 3): void
-    {
+    private function acquireSyncLock(
+        int $retries = 3,
+        bool $renew = false,
+    ): void {
         if (0 === $retries) {
             throw new RuntimeException('Unable to acquire sync lock for Mailman sync: timeout.');
         }
 
-        if ($this->isSyncLocked()) {
+        if ($this->isSyncLocked() && !$renew) {
             throw new RuntimeException('Unable to acquire sync lock for Mailman sync: locked by other process.');
         }
 
-        $this->configService->setConfig(ConfigNamespaces::DatabaseMailman, 'locked', true);
+        if (!$this->isSyncLocked() && $renew) {
+            throw new RuntimeException('Unable to renew sync lock for Mailman sync: currently unlocked.');
+        }
+
+        $this->configService->setConfig(
+            ConfigNamespaces::DatabaseMailman,
+            'locked',
+            (new DateTime())->modify('+23 hours'),
+        );
 
         if ($this->isSyncLocked()) {
             return;
@@ -159,15 +168,15 @@ private function acquireSyncLock(int $retries = 3): void
      */
     private function releaseSyncLock(): void
     {
-        $this->configService->setConfig(ConfigNamespaces::DatabaseMailman, 'locked', false);
+        $this->configService->setConfig(ConfigNamespaces::DatabaseMailman, 'locked', new DateTime());
     }
 
     /**
      * Get state of sync lock.
      */
     public function isSyncLocked(): bool
     {
-        return $this->configService->getConfig(ConfigNamespaces::DatabaseMailman, 'locked', false);
+        return $this->configService->getConfig(ConfigNamespaces::DatabaseMailman, 'locked') > new DateTime();
     }
 
     /**
@@ -185,6 +194,7 @@ public function syncMembership(
         $lists = $this->mailingListMapper->findAll();
 
         foreach ($lists as $list) {
+            $this->acquireSyncLock(renew: true);
             $this->syncMembershipSingle($list, $output, $dryRun);
         }
 

scripts/migrate-list.sh

@@ -0,0 +1,11 @@
+#/bin/sh
+
+# This script prints the available migrations for a specific alias
+set -e
+
+. /code/scripts/migrate-alias.sh
+
+./orm migrations:list --no-interaction --object-manager doctrine.entitymanager.$alias
+
+export alias=$alias
+export migrations=$migrations

scripts/migrate-version.sh

@@ -4,7 +4,7 @@
 # If you put this directly in the makefile, replace $ with $$
 set -e
 
-. /code/scripts/migrate-alias.sh
+. /code/scripts/migrate-list.sh
 
 read -rp "Give (partial, unique) version name (e.g. Database\Migrations\Version20241020224949 or 20241020)): " version