From 3be5671c03998c5a0c033f1838e15bd02d767c73 Mon Sep 17 00:00:00 2001
From: Jeffrey Huang <jeffreyhuang@google.com>
Date: Thu, 4 Jun 2020 12:31:47 -0700
Subject: [PATCH] Fix ConditionUnknown after valid buckets

Because non-zero enums are evaluated to true, it is possible for pulls
to occur on bucket split even if the condition is unknown. Explicitly
check that the condition is true instead.

Bug: 155489707
Test: atest statsd_test
Change-Id: I0bc3de6e885c2200885caf01d07c793b83eca17a
---
 cmds/statsd/src/metrics/ValueMetricProducer.h |  4 +-
 .../metrics/ValueMetricProducer_test.cpp      | 40 +++++++++++++++++++
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/cmds/statsd/src/metrics/ValueMetricProducer.h b/cmds/statsd/src/metrics/ValueMetricProducer.h
index 3de5b99a2b090..505b239218064 100644
--- a/cmds/statsd/src/metrics/ValueMetricProducer.h
+++ b/cmds/statsd/src/metrics/ValueMetricProducer.h
@@ -75,7 +75,7 @@ public:
         if (!mSplitBucketForAppUpgrade) {
             return;
         }
-        if (mIsPulled && mCondition) {
+        if (mIsPulled && mCondition == ConditionState::kTrue) {
             pullAndMatchEventsLocked(eventTimeNs);
         }
         flushCurrentBucketLocked(eventTimeNs, eventTimeNs);
@@ -84,7 +84,7 @@ public:
     // ValueMetric needs special logic if it's a pulled atom.
     void onStatsdInitCompleted(const int64_t& eventTimeNs) override {
         std::lock_guard<std::mutex> lock(mMutex);
-        if (mIsPulled && mCondition) {
+        if (mIsPulled && mCondition == ConditionState::kTrue) {
             pullAndMatchEventsLocked(eventTimeNs);
         }
         flushCurrentBucketLocked(eventTimeNs, eventTimeNs);
diff --git a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
index 1bcc35d99d18f..58a3259bbf2ca 100644
--- a/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
+++ b/cmds/statsd/tests/metrics/ValueMetricProducer_test.cpp
@@ -4722,6 +4722,46 @@ TEST(ValueMetricProducerTest, TestSlicedStateWithPrimaryField_WithDimensions) {
     EXPECT_EQ(5, report.value_metrics().data(4).bucket_info(1).values(0).value_long());
 }
 
+/*
+ * Test bucket splits when condition is unknown.
+ */
+TEST(ValueMetricProducerTest, TestForcedBucketSplitWhenConditionUnknownSkipsBucket) {
+    ValueMetric metric = ValueMetricProducerTestHelper::createMetricWithCondition();
+
+    sp<MockStatsPullerManager> pullerManager = new StrictMock<MockStatsPullerManager>();
+
+    sp<ValueMetricProducer> valueProducer =
+            ValueMetricProducerTestHelper::createValueProducerWithCondition(
+                    pullerManager, metric,
+                    ConditionState::kUnknown);
+
+    // App update event.
+    int64_t appUpdateTimeNs = bucketStartTimeNs + 1000;
+    valueProducer->notifyAppUpgrade(appUpdateTimeNs);
+
+    // Check dump report.
+    ProtoOutputStream output;
+    std::set<string> strSet;
+    int64_t dumpReportTimeNs = bucketStartTimeNs + 10000000000; // 10 seconds
+    valueProducer->onDumpReport(dumpReportTimeNs, false /* include current buckets */, true,
+                                NO_TIME_CONSTRAINTS /* dumpLatency */, &strSet, &output);
+
+    StatsLogReport report = outputStreamToProto(&output);
+    EXPECT_TRUE(report.has_value_metrics());
+    ASSERT_EQ(0, report.value_metrics().data_size());
+    ASSERT_EQ(1, report.value_metrics().skipped_size());
+
+    EXPECT_EQ(NanoToMillis(bucketStartTimeNs),
+              report.value_metrics().skipped(0).start_bucket_elapsed_millis());
+    EXPECT_EQ(NanoToMillis(appUpdateTimeNs),
+              report.value_metrics().skipped(0).end_bucket_elapsed_millis());
+    ASSERT_EQ(1, report.value_metrics().skipped(0).drop_event_size());
+
+    auto dropEvent = report.value_metrics().skipped(0).drop_event(0);
+    EXPECT_EQ(BucketDropReason::NO_DATA, dropEvent.drop_reason());
+    EXPECT_EQ(NanoToMillis(appUpdateTimeNs), dropEvent.drop_time_millis());
+}
+
 }  // namespace statsd
 }  // namespace os
 }  // namespace android