From cb396eb711d800a774cfe523e9a4a3c3bbfc0fa1 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
Merged-In: 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 019a9f7e5f9a4..a21327b4aae0b 100644
--- a/cmds/statsd/src/anomaly/AlarmTracker.cpp
+++ b/cmds/statsd/src/anomaly/AlarmTracker.cpp
@@ -61,11 +61,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 5e6de1cc6b91f..e664023a16479 100644
--- a/cmds/statsd/tests/anomaly/AlarmTracker_test.cpp
+++ b/cmds/statsd/tests/anomaly/AlarmTracker_test.cpp
@@ -40,23 +40,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));
     EXPECT_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