Add atomic reservation support in Redis for goal alert deduplication with expiration. Update Cron logic and add unit tests for validation.

services/nginx/app/classes/redis.php

@@ -450,6 +450,21 @@ class redis implements redis_i
         return 'temporary_cache_' . uniqid();
     }
 
+    /**
+     * Atomically set a key with TTL only when it does not already exist.
+     */
+    public function set_if_absent_with_expiration(string $key, string $value, int $seconds): bool
+    {
+        if (!self::is_connected()) {
+            self::connect();
+        }
+
+        $seconds = max(1, $seconds);
+        $result = $this->redis->set($key, $value, 'EX', $seconds, 'NX');
+
+        return $result === true || strtoupper((string)$result) === 'OK';
+    }
+
     public function mget(array $array_map): array
     {
         // Get multiple keys from Redis
@@ -485,4 +500,4 @@ class redis implements redis_i
         $this->delete('perm:' . $cache_key);
         return $this;
     }
-}
+}

services/nginx/app/cron/Cron.php

@@ -339,55 +339,60 @@ function GoalsProgressAlertsCron(): void
                 if (!$dueInfo['due']) { continue; }
             }
 
-            // Deduplicate per goal per slot via Redis
+            // Deduplicate per goal per slot via Redis using an atomic reservation.
+            // This prevents two concurrent cron runners from both sending the same alert.
             $slotKey = $dueInfo['slot'];
             $redisKey = 'goal_alert_sent:' . $goalId . ':' . $slotKey;
-            $already = redis->get($redisKey) ?? null;
-            if ($already) { continue; }
+            $ttl = max(1, (int)$dueInfo['ttl']);
+            if (!redis->set_if_absent_with_expiration($redisKey, '1', $ttl)) {
+                continue;
+            }
 
             // Render message
             $message = goals_progress_alert_renderer::render($criteria);
 
             // Dispatch according to destination
             $destination = $criteria->progress_alert_destination ?? Dest::SLACK;
-            switch ($destination) {
-                case Dest::SLACK:
-                    $departments = (array)$goal->departments->value();
-                    $sentToDept = false;
-                    if (count($departments) > 0) {
-                        foreach ($departments as $deptId) {
-                            if (!is_numeric($deptId)) { continue; }
-                            $dept = (new departments_o())->select((int)$deptId);
-                            if (!$dept->exists()) { continue; }
-                            $webhook = (string)$dept->slack_webhook->value();
-                            if (empty($webhook)) { continue; }
-                            (new Slack())->send_webhook_message((string)goals_progress_alert_renderer::render($criteria, $dept), $webhook);
-                            $sentToDept = true;
+            $sent = false;
+            try {
+                switch ($destination) {
+                    case Dest::SLACK:
+                        $departments = (array)$goal->departments->value();
+                        $sentToDept = false;
+                        if (count($departments) > 0) {
+                            foreach ($departments as $deptId) {
+                                if (!is_numeric($deptId)) { continue; }
+                                $dept = (new departments_o())->select((int)$deptId);
+                                if (!$dept->exists()) { continue; }
+                                $webhook = (string)$dept->slack_webhook->value();
+                                if (empty($webhook)) { continue; }
+                                (new Slack())->send_webhook_message((string)goals_progress_alert_renderer::render($criteria, $dept), $webhook);
+                                $sentToDept = true;
+                            }
                         }
-                    }
-                    if (!$sentToDept) {
-                        // Fallback to default webhook
-                        (new Slack())->send_message($message);
-                    }
-                    break;
-                case Dest::EMAIL:
-                    // No recipient context in goal for automated cron
-                    warn('GoalsProgressAlertsCron: EMAIL destination requires explicit recipients; skipping goal #' . $goalId);
-                    break;
-                case Dest::SMS:
-                    // No recipient context in goal for automated cron
-                    warn('GoalsProgressAlertsCron: SMS destination requires explicit recipients; skipping goal #' . $goalId);
-                    break;
-                default:
-                    // Unsupported or NONE
-                    warn('GoalsProgressAlertsCron: Unsupported destination for goal #' . $goalId);
-            }
-
-            // Mark slot as sent (expire in a reasonable window)
-            $ttl = $dueInfo['ttl'];
-            redis->set('' . $redisKey, 1);
-            if (method_exists(redis, 'expire')) {
-                redis->expire($redisKey, $ttl);
+                        if (!$sentToDept) {
+                            // Fallback to default webhook
+                            (new Slack())->send_message($message);
+                        }
+                        $sent = true;
+                        break;
+                    case Dest::EMAIL:
+                        // No recipient context in goal for automated cron
+                        warn('GoalsProgressAlertsCron: EMAIL destination requires explicit recipients; skipping goal #' . $goalId);
+                        break;
+                    case Dest::SMS:
+                        // No recipient context in goal for automated cron
+                        warn('GoalsProgressAlertsCron: SMS destination requires explicit recipients; skipping goal #' . $goalId);
+                        break;
+                    default:
+                        // Unsupported or NONE
+                        warn('GoalsProgressAlertsCron: Unsupported destination for goal #' . $goalId);
+                }
+            } catch (Throwable $dispatchError) {
+                if (!$sent) {
+                    redis->delete($redisKey);
+                }
+                throw $dispatchError;
             }
 
             // Track last progress for CHANGED

services/nginx/app/tests/Unit/Redis/RedisAtomicReservationTest.php

@@ -0,0 +1,73 @@
+<?php
+
+app_require('classes/redis.php');
+
+use classes\redis;
+use Predis\Client as PredisClient;
+
+class RedisAtomicReservationTestClient extends PredisClient
+{
+    public array $calls = [];
+
+    public function __construct(private mixed $returnValue)
+    {
+    }
+
+    public function set(...$arguments): mixed
+    {
+        $this->calls[] = $arguments;
+        return $this->returnValue;
+    }
+
+    public function disconnect(): void
+    {
+    }
+}
+
+function redis_test_inject_client(redis $redis, PredisClient $client): void
+{
+    $reflection = new ReflectionClass($redis);
+    $property = $reflection->getProperty('redis');
+    $property->setAccessible(true);
+    $property->setValue($redis, $client);
+}
+
+it('claims a slot atomically with nx and expiration', function (): void {
+    global $REDIS_CONFIG;
+
+    $REDIS_CONFIG = [
+        'host' => 'redis',
+        'database' => 0,
+        'password' => '',
+    ];
+
+    $client = new RedisAtomicReservationTestClient('OK');
+
+    $redis = new redis();
+    redis_test_inject_client($redis, $client);
+
+    expect($redis->set_if_absent_with_expiration('goal_alert_sent:22:2026-03-17', '1', 86400))->toBeTrue();
+    expect($client->calls)->toBe([
+        ['goal_alert_sent:22:2026-03-17', '1', 'EX', 86400, 'NX'],
+    ]);
+});
+
+it('returns false when the slot is already claimed and clamps ttl to one second', function (): void {
+    global $REDIS_CONFIG;
+
+    $REDIS_CONFIG = [
+        'host' => 'redis',
+        'database' => 0,
+        'password' => '',
+    ];
+
+    $client = new RedisAtomicReservationTestClient(null);
+
+    $redis = new redis();
+    redis_test_inject_client($redis, $client);
+
+    expect($redis->set_if_absent_with_expiration('goal_alert_sent:22:2026-03-17', '1', 0))->toBeFalse();
+    expect($client->calls)->toBe([
+        ['goal_alert_sent:22:2026-03-17', '1', 'EX', 1, 'NX'],
+    ]);
+});