From 979b0f8facaf1622981b674ef0ffbc33bd32f79e Mon Sep 17 00:00:00 2001 From: Jeppe Bundgaard Date: Tue, 17 Mar 2026 15:23:02 +0100 Subject: [PATCH] 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 | 17 +++- services/nginx/app/cron/Cron.php | 85 ++++++++++--------- .../Unit/Redis/RedisAtomicReservationTest.php | 73 ++++++++++++++++ 3 files changed, 134 insertions(+), 41 deletions(-) create mode 100644 services/nginx/app/tests/Unit/Redis/RedisAtomicReservationTest.php diff --git a/services/nginx/app/classes/redis.php b/services/nginx/app/classes/redis.php index d247dbdd..704b14d4 100644 --- a/services/nginx/app/classes/redis.php +++ b/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; } -} \ No newline at end of file +} diff --git a/services/nginx/app/cron/Cron.php b/services/nginx/app/cron/Cron.php index 2d2e114a..e7df2cc4 100644 --- a/services/nginx/app/cron/Cron.php +++ b/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 diff --git a/services/nginx/app/tests/Unit/Redis/RedisAtomicReservationTest.php b/services/nginx/app/tests/Unit/Redis/RedisAtomicReservationTest.php new file mode 100644 index 00000000..500ccf79 --- /dev/null +++ b/services/nginx/app/tests/Unit/Redis/RedisAtomicReservationTest.php @@ -0,0 +1,73 @@ +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'], + ]); +});