SONAR-23224 Reduce risk of telemetry daemon from endlessly sending messages

2 files changed, 49 insertions, 30 deletions

diff --git a/server/sonar-telemetry/src/main/java/org/sonar/telemetry/TelemetryDaemon.java b/server/sonar-telemetry/src/main/java/org/sonar/telemetry/TelemetryDaemon.java
index a7c60adffa0..8d6ac35d955 100644
--- a/server/sonar-telemetry/src/main/java/org/sonar/telemetry/TelemetryDaemon.java
+++ b/server/sonar-telemetry/src/main/java/org/sonar/telemetry/TelemetryDaemon.java
@@ -38,13 +38,13 @@ import org.sonar.db.DbSession;
 import org.sonar.server.property.InternalProperties;
 import org.sonar.server.util.AbstractStoppableScheduledExecutorServiceImpl;
 import org.sonar.server.util.GlobalLockManager;
+import org.sonar.telemetry.core.MessageSerializer;
 import org.sonar.telemetry.core.TelemetryClient;
+import org.sonar.telemetry.core.schema.BaseMessage;
 import org.sonar.telemetry.legacy.TelemetryData;
 import org.sonar.telemetry.legacy.TelemetryDataJsonWriter;
 import org.sonar.telemetry.legacy.TelemetryDataLoader;
 import org.sonar.telemetry.metrics.TelemetryMetricsLoader;
-import org.sonar.telemetry.core.schema.BaseMessage;
-import org.sonar.telemetry.core.MessageSerializer;
 
 import static org.sonar.process.ProcessProperties.Property.SONAR_TELEMETRY_ENABLE;
 import static org.sonar.process.ProcessProperties.Property.SONAR_TELEMETRY_FREQUENCY_IN_SECONDS;
@@ -116,29 +116,27 @@ public class TelemetryDaemon extends AbstractStoppableScheduledExecutorServiceIm
 
   private Runnable telemetryCommand() {
     return () -> {
-      try {
-
-        if (!lockManager.tryLock(LOCK_NAME, lockDuration())) {
-          return;
-        }
+      if (!lockManager.tryLock(LOCK_NAME, lockDuration())) {
+        return;
+      }
 
-        long now = system2.now();
+      long now = system2.now();
+      try {
         if (shouldUploadStatistics(now)) {
           uploadLegacyTelemetry();
           uploadMetrics();
-
-          updateTelemetryProps(now);
+          writeTelemetrySequence();
         }
       } catch (Exception e) {
         LOG.debug("Error while checking SonarQube statistics: {}", e.getMessage(), e);
+      } finally {
+        writeLastPing(now);
       }
       // do not check at start up to exclude test instance which are not up for a long time
     };
   }
 
-  private void updateTelemetryProps(long now) {
-    internalProperties.write(I_PROP_LAST_PING, String.valueOf(now));
-
+  private void writeTelemetrySequence() {
     Optional<String> currentSequence = internalProperties.read(I_PROP_MESSAGE_SEQUENCE);
     if (currentSequence.isEmpty()) {
       internalProperties.write(I_PROP_MESSAGE_SEQUENCE, String.valueOf(1));
@@ -149,6 +147,10 @@ public class TelemetryDaemon extends AbstractStoppableScheduledExecutorServiceIm
     internalProperties.write(I_PROP_MESSAGE_SEQUENCE, String.valueOf(current + 1));
   }
 
+  private void writeLastPing(long now) {
+    internalProperties.write(I_PROP_LAST_PING, String.valueOf(now));
+  }
+
   private void optOut() {
     StringWriter json = new StringWriter();
     try (JsonWriter writer = JsonWriter.of(json)) {
diff --git a/server/sonar-telemetry/src/test/java/org/sonar/telemetry/TelemetryDaemonTest.java b/server/sonar-telemetry/src/test/java/org/sonar/telemetry/TelemetryDaemonTest.java
index 4d3aac41a58..c5b4044877f 100644
--- a/server/sonar-telemetry/src/test/java/org/sonar/telemetry/TelemetryDaemonTest.java
+++ b/server/sonar-telemetry/src/test/java/org/sonar/telemetry/TelemetryDaemonTest.java
@@ -99,7 +99,7 @@ class TelemetryDaemonTest {
   }
 
   @Test
-  void send_data_via_client_at_startup_after_initial_delay() throws IOException {
+  void start_sendsDataAtStartupAfterInitialDelay() throws IOException {
     initTelemetrySettingsToDefaultValues();
     when(lockManager.tryLock(any(), anyInt())).thenReturn(true);
     settings.setProperty("sonar.telemetry.frequencyInSeconds", "1");
@@ -112,18 +112,8 @@ class TelemetryDaemonTest {
     verify(dataJsonWriter).writeTelemetryData(any(JsonWriter.class), same(SOME_TELEMETRY_DATA));
   }
 
-  private void mockDataJsonWriterDoingSomething() {
-    doAnswer(t -> {
-      JsonWriter json = t.getArgument(0);
-      json.beginObject().prop("foo", "bar").endObject();
-      return null;
-    })
-      .when(dataJsonWriter)
-      .writeTelemetryData(any(), any());
-  }
-
   @Test
-  void check_if_should_send_data_periodically() throws IOException {
+  void start_shouldCheckIfDataSentPeriodically() throws IOException {
     initTelemetrySettingsToDefaultValues();
     when(lockManager.tryLock(any(), anyInt())).thenReturn(true);
     long now = system2.now();
@@ -146,7 +136,7 @@ class TelemetryDaemonTest {
   }
 
   @Test
-  void do_not_send_data_if_last_ping_earlier_than_one_day_ago() throws IOException {
+  void start_whenLastPingEarlierThanOneDayAgo_shouldNotSendData() throws IOException {
     initTelemetrySettingsToDefaultValues();
     when(lockManager.tryLock(any(), anyInt())).thenReturn(true);
     settings.setProperty("sonar.telemetry.frequencyInSeconds", "1");
@@ -161,7 +151,24 @@ class TelemetryDaemonTest {
   }
 
   @Test
-  void send_data_if_last_ping_is_over_one_day_ago() throws IOException {
+  void start_whenExceptionThrown_shouldNotRepeatedlySendDataAndLastPingPropIsStillSet() throws IOException {
+    initTelemetrySettingsToDefaultValues();
+    when(lockManager.tryLock(any(), anyInt())).thenReturn(true);
+    settings.setProperty("sonar.telemetry.frequencyInSeconds", "1");
+    long today = parseDate("2017-08-01").getTime();
+    system2.setNow(today);
+    settings.removeProperty("telemetry.lastPing");
+    mockDataJsonWriterDoingSomething();
+    when(dataLoader.load()).thenThrow(new IllegalStateException("Some error was thrown."));
+
+    underTest.start();
+
+    verify(client, after(2_000).never()).upload(anyString());
+    verify(internalProperties, timeout(4_000)).write("telemetry.lastPing", String.valueOf(today));
+  }
+
+  @Test
+  void start_whenLastPingOverOneDayAgo_shouldSendData() throws IOException {
     initTelemetrySettingsToDefaultValues();
     when(lockManager.tryLock(any(), anyInt())).thenReturn(true);
     settings.setProperty("sonar.telemetry.frequencyInSeconds", "1");
@@ -180,7 +187,7 @@ class TelemetryDaemonTest {
   }
 
   @Test
-  void opt_out_sent_once() throws IOException {
+  void start_whenOptOut_shouldSendOnceAndLogsPresent() throws IOException {
     initTelemetrySettingsToDefaultValues();
     when(lockManager.tryLock(any(), anyInt())).thenReturn(true);
     settings.setProperty("sonar.telemetry.frequencyInSeconds", "1");
@@ -196,7 +203,7 @@ class TelemetryDaemonTest {
   }
 
   @Test
-  void write_sequence_as_one_if_not_previously_present() {
+  void start_whenSequenceNotPresent_shouldBeSetToOne() {
     initTelemetrySettingsToDefaultValues();
     when(lockManager.tryLock(any(), anyInt())).thenReturn(true);
     settings.setProperty("sonar.telemetry.frequencyInSeconds", "1");
@@ -208,7 +215,7 @@ class TelemetryDaemonTest {
   }
 
   @Test
-  void write_sequence_correctly_incremented() {
+  void start_whenSequencePresent_shouldIncrementCorrectly() {
     initTelemetrySettingsToDefaultValues();
     when(lockManager.tryLock(any(), anyInt())).thenReturn(true);
     settings.setProperty("sonar.telemetry.frequencyInSeconds", "1");
@@ -225,6 +232,16 @@ class TelemetryDaemonTest {
     verify(internalProperties, timeout(4_000)).write("telemetry.messageSeq", "11");
   }
 
+  private void mockDataJsonWriterDoingSomething() {
+    doAnswer(t -> {
+      JsonWriter json = t.getArgument(0);
+      json.beginObject().prop("foo", "bar").endObject();
+      return null;
+    })
+      .when(dataJsonWriter)
+      .writeTelemetryData(any(), any());
+  }
+
   private void initTelemetrySettingsToDefaultValues() {
     settings.setProperty(SONAR_TELEMETRY_ENABLE.getKey(), SONAR_TELEMETRY_ENABLE.getDefaultValue());
     settings.setProperty(SONAR_TELEMETRY_URL.getKey(), SONAR_TELEMETRY_URL.getDefaultValue());