From f5cd94b4d00c08987cd285bbb88a0706bfe36720 Mon Sep 17 00:00:00 2001
From: Tej Singh <singhtejinder@google.com>
Date: Thu, 25 Jun 2020 22:38:03 -0700
Subject: [PATCH] Fix edge case in statsd alarms

Right now, if an alarm fires exactly on time (within 1 second), or
exactly on a multiple of its period, statsd will reset the alarm for the
same time, which can result a loop of statsd setting an alarm, the alarm
firing, statsd setting an alarm at the same time, etc.

The fix is that if an alarm fires on time, we should move to the next
period.

Test: atest statsd_test (added to a test, which would fail without the
code change)
Bug: 159098080

Change-Id: I150538cb1d3d5a2b943df2a32d92f8f84bb96626
---
 cmds/statsd/src/anomaly/AlarmTracker.cpp      |  4 +--
 .../tests/anomaly/AlarmTracker_test.cpp       | 30 +++++++++++++++++--
 2 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/cmds/statsd/src/anomaly/AlarmTracker.cpp b/cmds/statsd/src/anomaly/AlarmTracker.cpp
index 5722f923d11ec..6d9beb8f718de 100644
--- a/cmds/statsd/src/anomaly/AlarmTracker.cpp
+++ b/cmds/statsd/src/anomaly/AlarmTracker.cpp
@@ -60,11 +60,11 @@ void AlarmTracker::addSubscription(const Subscription& subscription) {
 }
 
 int64_t AlarmTracker::findNextAlarmSec(int64_t currentTimeSec) {
-    if (currentTimeSec <= mAlarmSec) {
+    if (currentTimeSec < mAlarmSec) {
         return mAlarmSec;
     }
     int64_t periodsForward =
-        ((currentTimeSec - mAlarmSec) * MS_PER_SEC - 1) / mAlarmConfig.period_millis() + 1;
+        ((currentTimeSec - mAlarmSec) * MS_PER_SEC) / mAlarmConfig.period_millis() + 1;
     return mAlarmSec + periodsForward * mAlarmConfig.period_millis() / MS_PER_SEC;
 }
 
diff --git a/cmds/statsd/tests/anomaly/AlarmTracker_test.cpp b/cmds/statsd/tests/anomaly/AlarmTracker_test.cpp
index 322cfaf68a41a..64ea219c84656 100644
--- a/cmds/statsd/tests/anomaly/AlarmTracker_test.cpp
+++ b/cmds/statsd/tests/anomaly/AlarmTracker_test.cpp
@@ -43,23 +43,47 @@ TEST(AlarmTrackerTest, TestTriggerTimestamp) {
     alarm.set_offset_millis(15 * MS_PER_SEC);
     alarm.set_period_millis(60 * 60 * MS_PER_SEC);  // 1hr
     int64_t startMillis = 100000000 * MS_PER_SEC;
+    int64_t nextAlarmTime = startMillis / MS_PER_SEC + 15;
     AlarmTracker tracker(startMillis, startMillis, alarm, kConfigKey, subscriberAlarmMonitor);
 
-    EXPECT_EQ(tracker.mAlarmSec, (int64_t)(startMillis / MS_PER_SEC + 15));
+    EXPECT_EQ(tracker.mAlarmSec, nextAlarmTime);
 
     uint64_t currentTimeSec = startMillis / MS_PER_SEC + 10;
     std::unordered_set<sp<const InternalAlarm>, SpHash<InternalAlarm>> firedAlarmSet =
         subscriberAlarmMonitor->popSoonerThan(static_cast<uint32_t>(currentTimeSec));
     EXPECT_TRUE(firedAlarmSet.empty());
     tracker.informAlarmsFired(currentTimeSec * NS_PER_SEC, firedAlarmSet);
-    EXPECT_EQ(tracker.mAlarmSec, (int64_t)(startMillis / MS_PER_SEC + 15));
+    EXPECT_EQ(tracker.mAlarmSec, nextAlarmTime);
+    EXPECT_EQ(tracker.getAlarmTimestampSec(), nextAlarmTime);
 
     currentTimeSec = startMillis / MS_PER_SEC + 7000;
+    nextAlarmTime = startMillis / MS_PER_SEC + 15 + 2 * 60 * 60;
     firedAlarmSet = subscriberAlarmMonitor->popSoonerThan(static_cast<uint32_t>(currentTimeSec));
     ASSERT_EQ(firedAlarmSet.size(), 1u);
     tracker.informAlarmsFired(currentTimeSec * NS_PER_SEC, firedAlarmSet);
     EXPECT_TRUE(firedAlarmSet.empty());
-    EXPECT_EQ(tracker.mAlarmSec, (int64_t)(startMillis / MS_PER_SEC + 15 + 2 * 60 * 60));
+    EXPECT_EQ(tracker.mAlarmSec, nextAlarmTime);
+    EXPECT_EQ(tracker.getAlarmTimestampSec(), nextAlarmTime);
+
+    // Alarm fires exactly on time.
+    currentTimeSec = startMillis / MS_PER_SEC + 15 + 2 * 60 * 60;
+    nextAlarmTime = startMillis / MS_PER_SEC + 15 + 3 * 60 * 60;
+    firedAlarmSet = subscriberAlarmMonitor->popSoonerThan(static_cast<uint32_t>(currentTimeSec));
+    ASSERT_EQ(firedAlarmSet.size(), 1u);
+    tracker.informAlarmsFired(currentTimeSec * NS_PER_SEC, firedAlarmSet);
+    EXPECT_TRUE(firedAlarmSet.empty());
+    EXPECT_EQ(tracker.mAlarmSec, nextAlarmTime);
+    EXPECT_EQ(tracker.getAlarmTimestampSec(), nextAlarmTime);
+
+    // Alarm fires exactly 1 period late.
+    currentTimeSec = startMillis / MS_PER_SEC + 15 + 4 * 60 * 60;
+    nextAlarmTime = startMillis / MS_PER_SEC + 15 + 5 * 60 * 60;
+    firedAlarmSet = subscriberAlarmMonitor->popSoonerThan(static_cast<uint32_t>(currentTimeSec));
+    ASSERT_EQ(firedAlarmSet.size(), 1u);
+    tracker.informAlarmsFired(currentTimeSec * NS_PER_SEC, firedAlarmSet);
+    EXPECT_TRUE(firedAlarmSet.empty());
+    EXPECT_EQ(tracker.mAlarmSec, nextAlarmTime);
+    EXPECT_EQ(tracker.getAlarmTimestampSec(), nextAlarmTime);
 }
 
 }  // namespace statsd