Merge pull request #140 from sorru94/fw-port-exponential-backoff-to-0.7

Forward port exponential backoff fix from v0.6.2

Author: harlem88

CHANGELOG.md

@@ -10,6 +10,10 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.1.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
+## [0.6.2] - 2025-10-28
+### Fixed
+- An integer overflow in the exponential backoff module. The bug caused the calculated reconnection delay to wrap around to a negative value, leading to zero delay connection attempts during a network outage.
+
 ## [0.7.0] - 2025-08-13
 ### Added
 - API calls to fetch stored device properties in the gRPC Astarte device.

private/exponential_backoff.hpp

@@ -11,51 +11,93 @@
 #include <cstdint>
 #include <random>
 
+#include "astarte_device_sdk/exceptions.hpp"
+
 namespace AstarteDeviceSdk {
 
 class ExponentialBackoff {
  public:
   /**
    * @brief Construct an ExponentialBackoff instance.
-   * @param initial_delay The value for the first backoff delay.
-   * @param max_delay The upper bound for all the backoff delays.
+   *
+   * @details The exponential backoff will will compute an exponential delay using
+   * 2 as the base for the power operation and @p mul_coeff as the multiplier coefficient.
+   * The values returned by calls to getNextDelay will follow the formula:
+   * min( @p mul_coeff * 2 ^ ( number of calls ) , @p cutoff_coeff ) + random jitter
+   * The random jitter will be in the range [ - @p mul_coeff , + @p mul_coeff ]
+   *
+   * @note The jitter will be applied also once the @p cutoff_coeff has been reached. Effectively
+   * the maximum delay produced will be @p cutoff_coeff + @p mul_coeff.
+   *
+   * @param mul_coeff Multiplier coefficient used in the exponential delay calculation.
+   * @param cutoff_coeff The cut-off coefficient, an upper bound for the exponential curve.
    */
-  ExponentialBackoff(std::chrono::milliseconds initial_delay, std::chrono::milliseconds max_delay)
-      : initial_delay_(initial_delay), max_delay_(max_delay) {}
+  ExponentialBackoff(std::chrono::milliseconds mul_coeff, std::chrono::milliseconds cutoff_coeff)
+      : mul_coeff_(mul_coeff), cutoff_coeff_(cutoff_coeff) {
+    if ((mul_coeff <= std::chrono::milliseconds::zero()) ||
+        (cutoff_coeff <= std::chrono::milliseconds::zero())) {
+      throw AstarteInvalidInputException("Received zero or negative coefficients.");
+    }
+    if (cutoff_coeff < mul_coeff) {
+      throw AstarteInvalidInputException(
+          "The multiplier coefficient is larger than the cuttoff coefficient");
+    }
+  }
 
   /**
    * @brief Calculate and returns the next backoff delay.
-   * @details Computes the appropriate delay for the current backoff generation and increments the
-   * internal generation counter for the next call.
+   * @details See the documentation of the constructor of this class for an explanation on how
+   * this delay is computed.
    * @return The calculated delay duration.
    */
   auto getNextDelay() -> std::chrono::milliseconds {
-    constexpr double BACKOFF_FACTOR = 2.0;
+    const ChronoMillisRep mul_coeff = mul_coeff_.count();
+    const ChronoMillisRep max_milliseconds = std::chrono::milliseconds::max().count();
+    const ChronoMillisRep max_allowed_final_delay = max_milliseconds - mul_coeff;
 
-    const auto initial_delay_ms = static_cast<double>(initial_delay_.count());
+    // Update last delay value with the new value
+    ChronoMillisRep delay = 0;
+    if (prev_delay_ == 0) {
+      delay = mul_coeff;
+    } else if (prev_delay_ <= (max_allowed_final_delay / 2)) {
+      delay = 2 * prev_delay_;
+    } else {
+      delay = max_allowed_final_delay;
+    }
 
-    const auto delay_ms = initial_delay_ms * std::pow(BACKOFF_FACTOR, generated_delays_);
-    // Apply a positive jitter (a random value between 0 and initial_delay_)
-    const auto jitter = dist_(gen_) * initial_delay_ms;
+    // Bound the delay to the maximum
+    ChronoMillisRep bounded_delay = std::min(delay, cutoff_coeff_.count());
 
-    const auto total_delay_ms = static_cast<int64_t>(delay_ms + jitter);
-    const auto jittery_delay = std::chrono::milliseconds(total_delay_ms);
+    // Store the new delay before jitter application
+    prev_delay_ = bounded_delay;
 
-    generated_delays_++;
+    // Insert some jitter
+    ChronoMillisRep jitter_minimum = -mul_coeff;
+    if (bounded_delay - mul_coeff < 0) {
+      jitter_minimum = 0;
+    }
+    ChronoMillisRep jitter_maximum = mul_coeff;
+    if (bounded_delay > max_milliseconds - mul_coeff) {
+      jitter_maximum = max_milliseconds - bounded_delay;
+    }
+    std::uniform_int_distribution<ChronoMillisRep> dist(jitter_minimum, jitter_maximum);
+    ChronoMillisRep jittered_delay = bounded_delay + dist(gen_);
 
-    return std::min(jittery_delay, max_delay_);
+    // Convert to a chrono object
+    return std::chrono::milliseconds(jittered_delay);
   }
 
   /** @brief Reset the backoff generator. */
-  void reset() { generated_delays_ = 0; }
+  void reset() { prev_delay_ = 0; }
 
  private:
-  std::chrono::milliseconds initial_delay_;
-  std::chrono::milliseconds max_delay_;
-  int generated_delays_{0};
+  using ChronoMillisRep = std::chrono::milliseconds::rep;
+
+  std::chrono::milliseconds mul_coeff_;
+  std::chrono::milliseconds cutoff_coeff_;
   std::random_device rd_;
   std::mt19937 gen_{rd_()};
-  std::uniform_real_distribution<> dist_{0.0, 1.0};
+  ChronoMillisRep prev_delay_{0};
 };
 
 }  // namespace AstarteDeviceSdk

unit/CMakeLists.txt

@@ -17,7 +17,13 @@ FetchContent_MakeAvailable(googletest)
 
 enable_testing()
 
-add_executable(unit_test conversion_test.cpp data_test.cpp msg_test.cpp)
+add_executable(
+    unit_test
+    conversion_test.cpp
+    data_test.cpp
+    exponential_backoff_test.cpp
+    msg_test.cpp
+)
 
 # Add the Astarte sdk root directory
 add_subdirectory(${CMAKE_CURRENT_SOURCE_DIR}/.. ${CMAKE_CURRENT_BINARY_DIR}/lib_build)

unit/exponential_backoff_test.cpp

@@ -0,0 +1,67 @@
+// (C) Copyright 2025, SECO Mind Srl
+//
+// SPDX-License-Identifier: Apache-2.0
+
+#include "exponential_backoff.hpp"
+
+#include <gmock/gmock.h>
+#include <gtest/gtest.h>
+
+using ::testing::AllOf;
+using ::testing::Ge;
+using ::testing::Le;
+
+using AstarteDeviceSdk::ExponentialBackoff;
+
+TEST(AstarteTestExponentialBackoff, IncorrectInputs) {
+  EXPECT_THROW(ExponentialBackoff backoff(std::chrono::minutes(-1), std::chrono::minutes(1)),
+               std::exception);
+  EXPECT_THROW(ExponentialBackoff backoff(std::chrono::minutes(1), std::chrono::minutes(-1)),
+               std::exception);
+  EXPECT_THROW(
+      ExponentialBackoff backoff(std::chrono::minutes(1), std::chrono::milliseconds::zero()),
+      std::exception);
+  EXPECT_THROW(
+      ExponentialBackoff backoff(std::chrono::milliseconds::zero(), std::chrono::minutes(1)),
+      std::exception);
+}
+
+TEST(AstarteTestExponentialBackoff, OrdinaryBackoff) {
+  ExponentialBackoff backoff(std::chrono::minutes(1), std::chrono::minutes(18));
+  EXPECT_THAT(backoff.getNextDelay(),
+              AllOf(Ge(std::chrono::milliseconds::zero()), Le(std::chrono::minutes(2))));
+  EXPECT_THAT(backoff.getNextDelay(),
+              AllOf(Ge(std::chrono::minutes(1)), Le(std::chrono::minutes(3))));
+  EXPECT_THAT(backoff.getNextDelay(),
+              AllOf(Ge(std::chrono::minutes(3)), Le(std::chrono::minutes(5))));
+  EXPECT_THAT(backoff.getNextDelay(),
+              AllOf(Ge(std::chrono::minutes(7)), Le(std::chrono::minutes(9))));
+  EXPECT_THAT(backoff.getNextDelay(),
+              AllOf(Ge(std::chrono::minutes(15)), Le(std::chrono::minutes(17))));
+  for (size_t i = 0; i < 1048576u; i++) {
+    EXPECT_THAT(backoff.getNextDelay(),
+                AllOf(Ge(std::chrono::minutes(17)), Le(std::chrono::minutes(19))));
+  }
+}
+
+TEST(AstarteTestExponentialBackoff, VeryLargeBackoff) {
+  ExponentialBackoff backoff(
+      std::chrono::hours(1),
+      std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::years(100)));
+  EXPECT_THAT(backoff.getNextDelay(),
+              AllOf(Ge(std::chrono::milliseconds::zero()), Le(std::chrono::hours(2))));
+  EXPECT_THAT(backoff.getNextDelay(), AllOf(Ge(std::chrono::hours(1)), Le(std::chrono::hours(3))));
+  EXPECT_THAT(backoff.getNextDelay(), AllOf(Ge(std::chrono::hours(3)), Le(std::chrono::hours(5))));
+  EXPECT_THAT(backoff.getNextDelay(), AllOf(Ge(std::chrono::hours(7)), Le(std::chrono::hours(9))));
+  // A lot of calls in between
+  for (size_t i = 0; i < 1000000u; i++) {
+    backoff.getNextDelay();
+  }
+  // Check it settled around the proper value
+  for (size_t i = 0; i < 100u; i++) {
+    EXPECT_THAT(
+        backoff.getNextDelay(),
+        AllOf(Ge(std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::years(99))),
+              Le(std::chrono::duration_cast<std::chrono::milliseconds>(std::chrono::years(101)))));
+  }
+}