Fix number_of_attempts narrowing

score/launch_manager/docs/product_documentation/configuration.rst

@@ -129,7 +129,7 @@ restart (object, optional)
 
 restart.number_of_attempts (integer, optional)
     Description: Specifies the maximum number of restart attempts before the **Launch Manager** concludes that recovery cannot succeed for the component.
-    Constraint: Must be 0 or greater.
+    Constraint: Must be between 0 and 255 inclusive.
 
 restart.delay_before_restart_ms (integer, optional)
     Description: Specifies the delay duration, in milliseconds, that the **Launch Manager** waits before initiating a restart attempt.
@@ -555,4 +555,3 @@ Given that the **Launch Manager** supports multiple levels of default values, sp
 
 1. If a configuration value is not explicitly specified at a specific location (e.g., within an individual component's definition, a **Run Target's** definition, or a root-level property like ``alive_supervision``), the **Launch Manager** will first attempt to use the corresponding value from the user-defined ``defaults`` section.
 2. If the value is also not specified within the user-defined ``defaults`` section, then the **Launch Manager** will apply the S-CORE standard default value for that option.
-

score/launch_manager/src/daemon/src/configuration/config_schema/launch_manager.schema.json

@@ -150,6 +150,7 @@
             "number_of_attempts": {
               "type": "integer",
               "minimum": 0,
+              "maximum": 255,
               "description": "Specifies the maximum number of restart attempts before the Launch Manager concludes that recovery cannot succeed."
             },
             "delay_before_restart_ms": {

score/launch_manager/src/daemon/src/configuration/details/flatbuffer_config_loader_UT.cpp

@@ -809,5 +809,78 @@ TEST_F(FlatbufferConfigLoaderTest, MissingSchemaVersionReturnsInvalidFormat)
     EXPECT_THAT(result.error(), Eq(IConfigLoader::Error::InvalidFormat));
 }
 
+// ============================================================================
+// Boundary tests
+// ============================================================================
+
+struct NumberOfAttemptsParameterizedTestParams
+{
+    // Using uint_16_t to we can represent values > 255,
+    // so we can test overflow of uint8_t
+    std::uint16_t input;
+    std::uint16_t expected;
+};
+
+class NumberOfAttemptsParameterizedTest : public FlatbufferConfigLoaderTest,
+                                          public ::testing::WithParamInterface<NumberOfAttemptsParameterizedTestParams>
+{
+};
+
+TEST_P(
+    NumberOfAttemptsParameterizedTest,
+    GivenNumberOfAttemptsBoundaryValues_WhenFlatbufferLoaded_ThenNumberOfAttemptsIsValid)
+{
+    RecordProperty("DerivationTechnique", "boundary-values");
+    RecordProperty(
+        "Description",
+        "Given upper and lower boundary values for number_of_attempts: [0, 1, 254, 255, 256]. "
+        "And a valid boundary of 0 to 255 inclusive. "
+        "When the flatbuffer is loaded. "
+        "Then values within the boundary are preserved. "
+        "And values outside the boundary overflow. ");
+
+    // Given
+    ::flatbuffers::FlatBufferBuilder fbb;
+    const std::uint16_t NUMBER_OF_ATTEMPTS = GetParam().input;
+    auto restart = fb::CreateRestartAction(fbb, NUMBER_OF_ATTEMPTS, 1500);
+    auto bin_dir = fbb.CreateString("/opt");
+    auto work_dir = fbb.CreateString("/tmp");
+    auto sandbox = buildDefaultSandbox(fbb);
+    auto deploy = fb::CreateDeploymentConfig(
+        fbb,
+        1000 /*ready_timeout_ms*/,
+        1000 /*shutdown_timeout_ms*/,
+        0 /*environmental_variables*/,
+        bin_dir,
+        work_dir,
+        restart,
+        0 /*recovery_action*/,
+        sandbox);
+
+    auto comp = buildDefaultComponent(fbb, "restart_comp", buildDefaultComponentProperties(fbb), deploy);
+    auto comps = fbb.CreateVector(std::vector<::flatbuffers::Offset<fb::Component>>{comp});
+
+    // When
+    auto result = loadBuffer(buildConfigWithComponents(fbb, comps));
+
+    // Then
+    ASSERT_THAT(result.has_value(), IsTrue());
+    ASSERT_THAT(result->components().size(), Eq(1U));
+    ASSERT_THAT(result->components()[0].deployment_config.ready_recovery_action.has_value(), IsTrue());
+
+    const auto& ra = result->components()[0].deployment_config.ready_recovery_action.value();
+    EXPECT_THAT(ra.number_of_attempts, Eq(GetParam().expected));
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    NumberOfAttemptsBoundaryValues,
+    NumberOfAttemptsParameterizedTest,
+    ::testing::Values(
+        NumberOfAttemptsParameterizedTestParams{0U, 0U},
+        NumberOfAttemptsParameterizedTestParams{1U, 1U},
+        NumberOfAttemptsParameterizedTestParams{254U, 254U},
+        NumberOfAttemptsParameterizedTestParams{255U, 255U},
+        NumberOfAttemptsParameterizedTestParams{256U, 0U}));
+
 }  // namespace
 }  // namespace score::mw::lifecycle::internal::configuration

score/launch_manager/src/daemon/src/configuration/details/lm_flatcfg.fbs

@@ -93,7 +93,7 @@ table ComponentProperties {
 // Recovery action that restarts the POSIX process associated with a component.
 table RestartAction {
   // Maximum number of restart attempts before recovery is considered failed.
-  number_of_attempts:uint32 = null; // required
+  number_of_attempts:uint8 = null; // required
   // Delay in milliseconds before initiating a restart attempt.
   delay_before_restart_ms:uint32 = null; // required
 }

score/launch_manager/src/daemon/src/configuration/recovery_action_config.hpp

@@ -21,7 +21,7 @@ namespace score::mw::lifecycle::internal::configuration
 
 struct RestartAction
 {
-    std::uint32_t number_of_attempts{};
+    std::uint8_t number_of_attempts{};
     std::uint32_t delay_before_restart_ms{};
 };
 

score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp

@@ -19,6 +19,7 @@
 #include "score/mw/launch_manager/process_group_manager/details/safe_process_map.hpp"
 #include <score/assert.hpp>
 #include <unistd.h>
+#include <cstdint>
 #include <cstring>
 
 namespace score::mw::lifecycle::internal
@@ -35,7 +36,7 @@ ProcessInfoNode::ProcessInfoNode(configuration::ComponentConfig&& config, Proces
 {
     if (config_.deployment_config.ready_recovery_action.has_value())
     {
-        start_tries_ = config_.deployment_config.ready_recovery_action->number_of_attempts + 1;
+        start_retries_ = config_.deployment_config.ready_recovery_action->number_of_attempts;
     }
 
     const configuration::ApplicationProfile& app_profile = config_.component_properties.application_profile;
@@ -242,7 +243,7 @@ IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token s
     std::optional<ComponentError> error;
     const std::chrono::time_point initial_time = std::chrono::steady_clock::now();
 
-    for (std::uint8_t attempts = start_tries_; attempts != 0U; attempts--)
+    for (std::uint16_t attempts = 0U; attempts <= start_retries_; attempts++)
     {
         // setState(kIdle) will fail if the state is:
         // - Starting: this would mean we did not set state to kFailed on failure or exit when we successfully launched

score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp

@@ -68,7 +68,7 @@ class ProcessInfoNode final : public IComponent
           sync_(std::move(other.sync_)),
           process_handling_(std::move(other.process_handling_)),
           supervision_handle_(std::move(other.supervision_handle_)),
-          start_tries_(other.start_tries_),
+          start_retries_(other.start_retries_),
           identifier_(other.identifier_),
           termination_result_(other.termination_result_),
           termination_waiter_(std::move(other.termination_waiter_))
@@ -205,8 +205,8 @@ class ProcessInfoNode final : public IComponent
     /// @brief Interface for managing the process's alive supervision.
     std::unique_ptr<IAliveSupervisionHandle> supervision_handle_;
 
-    /// @brief Number ot times to try run the process.
-    std::uint8_t start_tries_{1U};
+    /// @brief Number of times to retry starting the process.
+    std::uint8_t start_retries_{0U};
 
     /// @brief Unique hash to identify this node.
     IdentifierHash identifier_;

score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp

@@ -84,7 +84,7 @@ class ProcessInfoNodeFixture : public ::testing::Test
     /// @note ComponentConfig is move-only, so a fresh one is built for every node.
     std::unique_ptr<ProcessInfoNode> createProcessInfoNode(
         configuration::ApplicationType application_type = configuration::ApplicationType::Reporting,
-        std::uint32_t restart_attempts = 0U,
+        std::uint8_t restart_attempts = 0U,
         bool self_terminating = false,
         configuration::ProcessState ready_state = configuration::ProcessState::Running)
     {
@@ -135,7 +135,7 @@ class ProcessInfoNodeFixture : public ::testing::Test
     /// @brief Helper method to create a ProcessInfoNode that is self-terminating.
     std::unique_ptr<ProcessInfoNode> createSelfTerminatingProcessInfoNode(
         configuration::ApplicationType application_type = configuration::ApplicationType::Reporting,
-        std::uint32_t restart_attempts = 0U)
+        std::uint8_t restart_attempts = 0U)
     {
         return createProcessInfoNode(application_type, restart_attempts, true);
     }
@@ -654,6 +654,73 @@ TEST_F(ProcessInfoNodeStartupCrashTest, TimeoutThenSuccess_WithRestarts)
     ASSERT_THAT(node->getState(), Eq(score::mw::lifecycle::ProcessState::kRunning));
 }
 
+struct NumberOfAttemptsParameterizedTestParams
+{
+    // Using uint_16_t to we can represent values > 255,
+    // so we can test overflow of uint8_t
+    std::uint16_t input;
+    std::uint16_t expected_attempts;
+};
+
+class NumberOfAttemptsParameterizedTest : public ProcessInfoNodeStartupCrashTest,
+                                          public ::testing::WithParamInterface<NumberOfAttemptsParameterizedTestParams>
+{
+};
+
+TEST_P(
+    NumberOfAttemptsParameterizedTest,
+    GivenBoundaryValuesForNumberOfAttempts_ExpectCrashBeforeReady_WhenActivateCalled)
+{
+    RecordProperty("DerivationTechnique", "boundary-values");
+    RecordProperty(
+        "Description",
+        "Given boundary values for number_of_attempts: [0, 1, 254, 255]. "
+        "And a valid boundary of 0 to 254 inclusive. "
+        "Expect that the OS handler detects a crash on every attempt. "
+        "Expect that the number of start attempts equals the expected values: [1, 2, 255, 256]. "
+        "When activate() is called. "
+        "Then kErrorBeforeReady is returned for all number_of_attempts values. ");
+
+    // Expect
+    expectActivationReport(0);
+    const std::uint16_t kRestartAttempts = GetParam().input;
+    const std::uint16_t kExpectedTotalAttempts = GetParam().expected_attempts;
+    auto node = createProcessInfoNode(configuration::ApplicationType::ReportingAndSupervised, kRestartAttempts);
+
+    EXPECT_CALL(mock_processIf_, startProcess(_, _, _))
+        .Times(kExpectedTotalAttempts)
+        .WillRepeatedly(Return(osal::OsalReturnType::kSuccess));
+    EXPECT_CALL(*process_map_, insertIfNotTerminated(_, _))
+        .Times(kExpectedTotalAttempts)
+        .WillRepeatedly(Return(score::mw::lifecycle::internal::SafeProcessMapReturnType::kOk));
+    // Simulate the OS handler detecting the crash on every attempt, while the process is still waiting to reach
+    // kRunning.
+    EXPECT_CALL(mock_processIf_, waitForkRunning(_, _))
+        .Times(kExpectedTotalAttempts)
+        .WillRepeatedly(DoAll(
+            InvokeWithoutArgs([node = node.get()] {
+                static_cast<void>(node->tryHandleTermination(-1));
+            }),
+            Return(osal::OsalReturnType::kFail)));
+
+    // When
+    auto result = node->activate(score::cpp::stop_token{});
+
+    // Then
+    ASSERT_THAT(result.has_value(), IsFalse());
+    ASSERT_THAT(result.error(), Eq(IComponent::ComponentError::kErrorBeforeReady));
+    ASSERT_THAT(node->getState(), Eq(score::mw::lifecycle::ProcessState::kTerminated));
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    NumberOfAttemptsBoundaryValues,
+    NumberOfAttemptsParameterizedTest,
+    ::testing::Values(
+        NumberOfAttemptsParameterizedTestParams{0U, 1U},
+        NumberOfAttemptsParameterizedTestParams{1U, 2U},
+        NumberOfAttemptsParameterizedTestParams{254U, 255U},
+        NumberOfAttemptsParameterizedTestParams{255U, 256U}));
+
 // Bundles unexpected terminations that occur after the ready condition has been reached.
 class ProcessInfoNodeUnexpectedTerminationTest : public ProcessInfoNodeFixture
 {

scripts/config_mapping/integration_tests.py

@@ -239,14 +239,32 @@ def test_custom_validation_failures(
                 )
 
 
-def test_schema_validation_failures(schema_file):
+# Define the parameterized test cases
+SCHEMA_VALIDATION_TEST_CASES = [
+    {
+        "config_filename": "lm_config.json",
+        "test_id": "missing_recovery_action",
+    },
+    {
+        "config_filename": "invalid_number_of_attempts.json",
+        "test_id": "invalid_number_of_attempts",
+    },
+]
+
+
+@pytest.mark.parametrize(
+    "config_filename",
+    [(case["config_filename"]) for case in SCHEMA_VALIDATION_TEST_CASES],
+    ids=[case["test_id"] for case in SCHEMA_VALIDATION_TEST_CASES],
+)
+def test_schema_validation_failures(schema_file, config_filename):
     """
     Test that schema validation errors are correctly raised when the input configuration does not conform to the defined JSON schema.
     The input configuration contains the following issues:
     * Missing required field "initial_run_target"
     """
     test_name = "schema_validation_failure_test"
-    input_file = tests_dir / test_name / "input" / "lm_config.json"
+    input_file = tests_dir / test_name / "input" / config_filename
 
     try:
         run(input_file, test_name, schema_file)

scripts/config_mapping/tests/full_config_test/expected_output/lm_config_gen.json

@@ -64,7 +64,7 @@
                     }
                 ],
                 "ready_recovery_action": {
-                    "number_of_attempts": 5,
+                    "number_of_attempts": 254,
                     "delay_before_restart_ms": 500
                 },
                 "recovery_action": {

scripts/config_mapping/tests/full_config_test/input/lm_config.json

@@ -99,7 +99,7 @@
                 "working_dir": "/var/run/a",
                 "ready_recovery_action": {
                     "restart": {
-                        "number_of_attempts": 5,
+                        "number_of_attempts": 254,
                         "delay_before_restart_ms": 500
                     }
                 },

scripts/config_mapping/tests/schema_validation_failure_test/input/invalid_number_of_attempts.json

@@ -0,0 +1,36 @@
+{
+    "schema_version": 1,
+    "defaults": {},
+    "components": {
+        "reporting_and_kterminated": {
+            "component_properties": {
+                "binary_name": "/bin/true",
+                "ready_condition": {
+                    "process_state": "Terminated"
+                },
+                "application_profile": {
+                    "application_type": "Reporting",
+                    "is_self_terminating": true
+                }
+            },
+            "deployment_config": {
+                "ready_recovery_action": {
+                    "restart": {
+                        "number_of_attempts": 256
+                    }
+                }
+            }
+        }
+    },
+    "run_targets": {
+        "Startup": {
+            "depends_on": [
+                "reporting_and_kterminated"
+            ]
+        }
+    },
+    "initial_run_target": "Startup",
+    "fallback_run_target": {
+        "depends_on": []
+    }
+}