From 32b31391d1780902f231a7e7063e4f8d01fd7c41 Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Mon, 31 Aug 2026 15:50:32 +0100 Subject: [PATCH 01/12] Renames --- .../details/safe_process_map.cpp | 156 +++++++++--------- .../details/safe_process_map.hpp | 28 ++-- 2 files changed, 92 insertions(+), 92 deletions(-) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.cpp index d023fe8d06..a3d063bec7 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.cpp @@ -25,33 +25,33 @@ SafeProcessMap::SafeProcessMap(uint32_t capacity, IComponentController& terminat { if (capacity) { - free_root_ = 0U; - pid_root_.store(LINK_NO_VALUE); + free_list_head_ = 0U; + tree_root_.store(NULL_INDEX); for (std::size_t i = 0U; i < capacity; ++i) { items_[i].pid_ = 0; - items_[i].data_.pin_ = nullptr; - items_[i].pid_left_ = LINK_NO_VALUE; + items_[i].data_.component_ = nullptr; + items_[i].pid_left_ = NULL_INDEX; items_[i].pid_right_ = static_cast(i + 1U); items_[i].data_.status_ = -1; } - items_[capacity - 1UL].pid_right_ = LINK_NO_VALUE; + items_[capacity - 1UL].pid_right_ = NULL_INDEX; } } -void SafeProcessMap::findNode(uint32_t& mask, uint32_t& last, osal::ProcessID key) +void SafeProcessMap::findNode(uint32_t& mask, uint32_t& parent, osal::ProcessID key) { - while (rover_ != LINK_NO_VALUE && key != items_[rover_].pid_) + while (current_ != NULL_INDEX && key != items_[current_].pid_) { - last = rover_; + parent = current_; if (static_cast(key) & mask) { - rover_ = items_[last].pid_left_; + current_ = items_[parent].pid_left_; } else { - rover_ = items_[last].pid_right_; + current_ = items_[parent].pid_right_; } mask = mask << 1U; @@ -67,13 +67,13 @@ void SafeProcessMap::findNode(uint32_t& mask, uint32_t& last, osal::ProcessID ke } // RULECHECKER_comment(1, 1, check_max_parameters, "refactored with WI #9343", true); -int32_t SafeProcessMap::insertNode(uint32_t& mask, uint32_t& last, osal::ProcessID& key, ProcessInfoData& data) +int32_t SafeProcessMap::insertNode(uint32_t& mask, uint32_t& parent, osal::ProcessID& key, ProcessInfoData& data) { int32_t ret_value = -1; - rover_ = free_root_; + current_ = free_list_head_; - if (rover_ == LINK_NO_VALUE) + if (current_ == NULL_INDEX) { // too bad, we are out of memory ret_value = -1; @@ -81,22 +81,22 @@ int32_t SafeProcessMap::insertNode(uint32_t& mask, uint32_t& last, osal::Process else { mask = mask >> 1U; - free_root_ = items_[rover_].pid_right_; - items_[rover_].pid_ = key; - items_[rover_].data_ = data; - items_[rover_].pid_left_ = LINK_NO_VALUE; - items_[rover_].pid_right_ = LINK_NO_VALUE; + free_list_head_ = items_[current_].pid_right_; + items_[current_].pid_ = key; + items_[current_].data_ = data; + items_[current_].pid_left_ = NULL_INDEX; + items_[current_].pid_right_ = NULL_INDEX; if (static_cast(key) & mask) { - items_[last].pid_left_ = rover_; + items_[parent].pid_left_ = current_; } else { - items_[last].pid_right_ = rover_; + items_[parent].pid_right_ = current_; } - if (data.pin_ == nullptr) + if (data.component_ == nullptr) { ret_value = 1; } @@ -110,29 +110,29 @@ int32_t SafeProcessMap::insertNode(uint32_t& mask, uint32_t& last, osal::Process } // RULECHECKER_comment(1, 1, check_max_parameters, "refactored with WI #9343", true); -int32_t SafeProcessMap::removeNode(ProcessInfoData& target, ProcessInfoData& data, uint32_t& last, uint32_t& local_root) +int32_t SafeProcessMap::removeNode(ProcessInfoData& target, ProcessInfoData& data, uint32_t& parent, uint32_t& root) { // found key. There are 4 situations: - // data.pin_ == nullptr, stored pin_ != nullptr: normal findTerminated - // data.pin_ != nullptr, stored pin_ == nullptr: normal insertIfNotTerminated - // both data.pin_ and stored pin_ point to an ITerminationCallback: anomalous - // both data.pin_ and stored pin_ are null: anomalous - // In other words, exactly one of data.pin_ and stored pin_ must be nullptr + // data.component_ == nullptr, stored component_ != nullptr: normal findTerminated + // data.component_ != nullptr, stored component_ == nullptr: normal insertIfNotTerminated + // both data.component_ and stored component_ point to an ITerminationCallback: anomalous + // both data.component_ and stored component_ are null: anomalous + // In other words, exactly one of data.component_ and stored component_ must be nullptr // or there is an anomaly and we return -2 int32_t ret_value = -2; - if ((nullptr == data.pin_) ^ (nullptr == items_[rover_].data_.pin_)) + if ((nullptr == data.component_) ^ (nullptr == items_[current_].data_.component_)) { // found key, we will remove it! - target = items_[rover_].data_; - if (target.pin_) + target = items_[current_].data_; + if (target.component_) { target.status_ = data.status_; } else { - target.pin_ = data.pin_; + target.component_ = data.component_; } - if (data.pin_) + if (data.component_) { ret_value = 1; } @@ -141,26 +141,26 @@ int32_t SafeProcessMap::removeNode(ProcessInfoData& target, ProcessInfoData& dat ret_value = 0; } // Need to find a suitable leaf to use as the replacement - uint32_t leaf = rover_; - uint32_t previous = rover_; - findLeaf(leaf, previous); - deleteNode(last, leaf, local_root, previous); + uint32_t leaf = current_; + uint32_t leaf_parent = current_; + findLeaf(leaf, leaf_parent); + deleteNode(parent, leaf, root, leaf_parent); } return ret_value; } -void SafeProcessMap::findLeaf(uint32_t& leaf, uint32_t& previous) +void SafeProcessMap::findLeaf(uint32_t& leaf, uint32_t& leaf_parent) { while (true) { - if (items_[leaf].pid_left_ != LINK_NO_VALUE) + if (items_[leaf].pid_left_ != NULL_INDEX) { - previous = leaf; + leaf_parent = leaf; leaf = items_[leaf].pid_left_; } - else if (items_[leaf].pid_right_ != LINK_NO_VALUE) + else if (items_[leaf].pid_right_ != NULL_INDEX) { - previous = leaf; + leaf_parent = leaf; leaf = items_[leaf].pid_right_; } else @@ -171,50 +171,50 @@ void SafeProcessMap::findLeaf(uint32_t& leaf, uint32_t& previous) } // RULECHECKER_comment(1, 1, check_max_parameters, "refactored with WI #9343", true); -void SafeProcessMap::deleteNode(uint32_t& last, uint32_t& leaf, uint32_t& local_root, uint32_t& previous) +void SafeProcessMap::deleteNode(uint32_t& parent, uint32_t& leaf, uint32_t& root, uint32_t& leaf_parent) { - if (leaf == local_root) + if (leaf == root) { // tree is now empty! - local_root = LINK_NO_VALUE; + root = NULL_INDEX; } else { - if (leaf == rover_) + if (leaf == current_) { - // simply remove the link to rover - if (items_[last].pid_left_ == rover_) + // simply remove the link to current_ + if (items_[parent].pid_left_ == current_) { - items_[last].pid_left_ = LINK_NO_VALUE; + items_[parent].pid_left_ = NULL_INDEX; } else { - items_[last].pid_right_ = LINK_NO_VALUE; + items_[parent].pid_right_ = NULL_INDEX; } } else { // Put the leaf in place of the item we are replacing - items_[rover_].pid_ = items_[leaf].pid_; - items_[rover_].data_ = items_[leaf].data_; + items_[current_].pid_ = items_[leaf].pid_; + items_[current_].data_ = items_[leaf].data_; // Remove the links on the item that previously pointed to the leaf - if (items_[previous].pid_left_ == leaf) + if (items_[leaf_parent].pid_left_ == leaf) { - items_[previous].pid_left_ = LINK_NO_VALUE; + items_[leaf_parent].pid_left_ = NULL_INDEX; } else { - items_[previous].pid_right_ = LINK_NO_VALUE; + items_[leaf_parent].pid_right_ = NULL_INDEX; } } } // now return the leaf we found to the free list items_[leaf].pid_ = 0; items_[leaf].data_ = {-1, nullptr}; - items_[leaf].pid_left_ = LINK_NO_VALUE; - items_[leaf].pid_right_ = free_root_; - free_root_ = leaf; + items_[leaf].pid_left_ = NULL_INDEX; + items_[leaf].pid_right_ = free_list_head_; + free_list_head_ = leaf; } int32_t SafeProcessMap::search(osal::ProcessID key, ProcessInfoData data) @@ -228,27 +228,27 @@ int32_t SafeProcessMap::search(osal::ProcessID key, ProcessInfoData data) ProcessInfoData target; target = {data.status_, nullptr}; // Gain a lock on the root of the tree - uint32_t local_root = pid_root_.exchange(LINK_LOCKED); + uint32_t root = tree_root_.exchange(LOCKED_INDEX); - while (local_root == LINK_LOCKED) + while (root == LOCKED_INDEX) { std::this_thread::yield(); - local_root = pid_root_.exchange(LINK_LOCKED); + root = tree_root_.exchange(LOCKED_INDEX); } - rover_ = local_root; + current_ = root; - if (local_root == LINK_NO_VALUE) + if (root == NULL_INDEX) { // no tree, special case. - rover_ = free_root_; - free_root_ = items_[rover_].pid_right_; - items_[rover_].pid_ = key; - items_[rover_].data_ = data; - items_[rover_].pid_left_ = LINK_NO_VALUE; - items_[rover_].pid_right_ = LINK_NO_VALUE; - local_root = rover_; + current_ = free_list_head_; + free_list_head_ = items_[current_].pid_right_; + items_[current_].pid_ = key; + items_[current_].data_ = data; + items_[current_].pid_left_ = NULL_INDEX; + items_[current_].pid_right_ = NULL_INDEX; + root = current_; - if (data.pin_ == nullptr) + if (data.component_ == nullptr) { ret_value = 1; } @@ -260,33 +260,33 @@ int32_t SafeProcessMap::search(osal::ProcessID key, ProcessInfoData data) else { // Look for the key - uint32_t last = LINK_NO_VALUE; + uint32_t parent = NULL_INDEX; uint32_t mask = 1U; - findNode(mask, last, key); + findNode(mask, parent, key); - if (rover_ == LINK_NO_VALUE) + if (current_ == NULL_INDEX) { // key not found, we will add it - ret_value = insertNode(mask, last, key, data); + ret_value = insertNode(mask, parent, key, data); } else { // found key, we will remove it! - ret_value = removeNode(target, data, last, local_root); + ret_value = removeNode(target, data, parent, root); } } // release the lock on the tree - pid_root_.store(local_root); + tree_root_.store(root); if (-2 == ret_value) { // allow another thread to run to resolve the anomaly std::this_thread::yield(); } - else if (target.pin_) + else if (target.component_) { - termination_handler_.terminated(*target.pin_, target.status_); + termination_handler_.terminated(*target.component_, target.status_); } } } diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.hpp index ac83cdd773..8c051945c5 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/safe_process_map.hpp @@ -25,8 +25,8 @@ namespace score::mw::lifecycle::internal /// @brief Struct representing data in a map item struct ProcessInfoData { - int32_t status_ = -1; ///< Exit status for process - IComponent* pin_ = nullptr; ///< Pointer to the termination callback associated with this item. + int32_t status_ = -1; ///< Exit status for process + IComponent* component_ = nullptr; ///< Pointer to the termination callback associated with this item. }; /// @brief Struct representing an item in the map. struct ProcessTreeNode @@ -138,7 +138,7 @@ class SafeProcessMap final : public SafeProcessMapInserter /// @param last Reference to an integer where the index of the last visited node will be stored. /// This parameter is updated during the traversal to keep track of the last node visited. /// @param key The process ID to find in the tree. - void findNode(uint32_t& mask, uint32_t& last, osal::ProcessID key); + void findNode(uint32_t& mask, uint32_t& parent, osal::ProcessID key); /// @brief Inserts a node into the SafeProcessMap with the given process ID and associated information. /// This function inserts a node into the SafeProcessMap using a rover mechanism for safe traversal and insertion. @@ -154,7 +154,7 @@ class SafeProcessMap final : public SafeProcessMapInserter /// - 0 if the node was successfully inserted. /// - 1 if the insertion was successful but the object pointer was null. /// - -1 if the insertion failed due to memory constraints (out of memory). - int32_t insertNode(uint32_t& mask, uint32_t& last, osal::ProcessID& key, ProcessInfoData& data); + int32_t insertNode(uint32_t& mask, uint32_t& parent, osal::ProcessID& key, ProcessInfoData& data); /// @brief Removes a node from the process map tree. /// This function removes the node currently pointed to by the rover in the SafeProcessMap. @@ -167,7 +167,7 @@ class SafeProcessMap final : public SafeProcessMapInserter /// @param local_root Index of the first object /// @return int32_t Returns 0 if the node was successfully removed and `object` was nullptr, /// 1 if `object` was not nullptr, and -2 if the removal failed due to PID re-use - int32_t removeNode(ProcessInfoData& target, ProcessInfoData& data, uint32_t& last, uint32_t& local_root); + int32_t removeNode(ProcessInfoData& target, ProcessInfoData& data, uint32_t& parent, uint32_t& root); /// @brief Finds the leaf node in the SafeProcessMap starting from the given node. /// This function traverses the SafeProcessMap starting from the specified node `leaf` @@ -176,7 +176,7 @@ class SafeProcessMap final : public SafeProcessMapInserter /// node. Upon successful execution, this parameter will store the index of the found leaf node. /// @param previous Reference to an integer that will store the index of the parent node of the found leaf node. /// If the `leaf` node itself is the root or a leaf, `previous` will be set to the same as `leaf`. - void findLeaf(uint32_t& leaf, uint32_t& previous); + void findLeaf(uint32_t& leaf, uint32_t& leaf_parent); /// @brief Deletes a node from the SafeProcessMap, handling reorganization and freeing of resources. /// This function deletes a node from the SafeProcessMap structure based on the given parameters, @@ -186,30 +186,30 @@ class SafeProcessMap final : public SafeProcessMapInserter /// @param leaf Reference to an integer representing the index of the node to be deleted from the map. /// Upon successful deletion, this parameter will be returned to the free list for reuse. /// @param local_root Reference to an integer storing the index of the root node of the current tree structure. - /// If the deleted node is the root, this parameter is updated to LINK_NO_VALUE, indicating an empty tree. + /// If the deleted node is the root, this parameter is updated to NULL_INDEX, indicating an empty tree. /// @param previous Reference to an integer storing the index of the parent node of the deleted node. /// This parameter is used to update the link of the parent node after deletion. - void deleteNode(uint32_t& last, uint32_t& leaf, uint32_t& local_root, uint32_t& previous); + void deleteNode(uint32_t& parent, uint32_t& leaf, uint32_t& root, uint32_t& leaf_parent); ///@brief Unique pointer managing an array of ProcessTreeNode objects. std::unique_ptr items_; /// @brief Value indicating that no node is assigned. - static constexpr uint32_t LINK_NO_VALUE = 0xFFFFFFFF; + static constexpr uint32_t NULL_INDEX = 0xFFFFFFFF; /// @brief Value indicating that a node is locked. - static constexpr uint32_t LINK_LOCKED = 0xFFFFFFFE; + static constexpr uint32_t LOCKED_INDEX = 0xFFFFFFFE; /// @brief Root of the binary tree used to find an entry by process ID (pid). - std::atomic_uint32_t pid_root_{LINK_NO_VALUE}; + std::atomic_uint32_t tree_root_{NULL_INDEX}; /// @brief Root of the list of free entries. - uint32_t free_root_{LINK_NO_VALUE}; + uint32_t free_list_head_{NULL_INDEX}; /// @brief Current rover index in the SafeProcessMap. /// This variable represents the current index used for traversal within the SafeProcessMap. - /// It initially starts with LINK_NO_VALUE, indicating no valid position. - uint32_t rover_{LINK_NO_VALUE}; + /// It initially starts with NULL_INDEX, indicating no valid position. + uint32_t current_{NULL_INDEX}; IComponentController& termination_handler_; }; From 734318f8e56e851a0a04ecf8621c98ec3b67115f Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Mon, 31 Aug 2026 17:17:23 +0100 Subject: [PATCH 02/12] Add test cases --- .../details/process_info_node_UT.cpp | 140 ++++++++++++++++++ 1 file changed, 140 insertions(+) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp index 0e38a596a3..27cceeccc2 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp @@ -284,6 +284,146 @@ TEST_F(ProcessInfoNodeStartupTest, ActivateAlreadyActiveNode_ReturnsSuccess) ASSERT_THAT(node->getState(), Eq(score::mw::lifecycle::ProcessState::kRunning)); } +struct YieldTestCasesData +{ + configuration::ApplicationType app_type_; + configuration::ProcessState condition_; + int status_to_exit_with_; + bool is_self_terminating_; + IComponent::RequestResult expected_activation_result_; + IComponent::RequestResult expected_term_result_; + std::string description_; +}; + +void PrintTo(const YieldTestCasesData& params, std::ostream* os) +{ + *os << "{ Reporting: " << (params.app_type_ == configuration::ApplicationType::Native ? "No" : "Yes") + << ", Condition: " << (params.condition_ == configuration::ProcessState::Running ? "Running" : "Terminated") + << ", Exit status: " << params.status_to_exit_with_ + << ", Self Terminating: " << (params.is_self_terminating_ ? "Yes" : "No") << " }, " << params.description_; +} + +class ProcessInfoNodeMapYieldTest : public ::WithParamInterface, public ProcessInfoNodeFixture +{ +}; + +TEST_P(ProcessInfoNodeMapYieldTest, InsertReturnsYield) +{ + RecordProperty("Description", GetParam().description_); + + auto node = createProcessInfoNode(GetParam().app_type_, 0, GetParam().is_self_terminating_, GetParam().condition_); + IComponent::RequestResult tryHandleTerminationResult; + auto status = GetParam().status_to_exit_with_; + + EXPECT_CALL(mock_processIf_, startProcess).WillOnce(Return(osal::OsalReturnType::kSuccess)); + // kYield means the process already terminated, so we should not request termination again + EXPECT_CALL(mock_processIf_, requestTermination).Times(0); + EXPECT_CALL(*process_map_, insertIfNotTerminated) + .WillOnce(DoAll( + InvokeWithoutArgs([node = node.get(), &tryHandleTerminationResult, status] { + tryHandleTerminationResult = node->tryHandleTermination(status); + }), + Return(SafeProcessMapReturnType::kYield))); + + auto res = node->activate(score::cpp::stop_token{}); + + ASSERT_EQ(res.has_value(), GetParam().expected_activation_result_.has_value()); + if (GetParam().expected_activation_result_.has_value()) + { + EXPECT_EQ(res.value(), GetParam().expected_activation_result_.value()); + } + else + { + EXPECT_EQ(res.error(), GetParam().expected_activation_result_.error()); + } + + ASSERT_EQ(tryHandleTerminationResult.has_value(), GetParam().expected_term_result_.has_value()); + if (GetParam().expected_term_result_.has_value()) + { + EXPECT_EQ(tryHandleTerminationResult.value(), GetParam().expected_term_result_.value()); + } + else + { + EXPECT_EQ(tryHandleTerminationResult.error(), GetParam().expected_term_result_.error()); + } + + EXPECT_EQ(node->getState(), ProcessState::kTerminated); // Not kFailed, the posix process did start successfully +} + +INSTANTIATE_TEST_SUITE_P( + ProcessInfoNodeTest, + ProcessInfoNodeMapYieldTest, + Values( + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Running, + 0, + true, + {IComponent::RequestState::kSuccess}, + {IComponent::RequestState::kWaiting}, + "A native, self-terminating process exiting quickly with status 0 should report a successful activation"}, + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Running, + 111, + true, + {IComponent::RequestState::kSuccess}, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorAfterReady), + "A native, self-terminating process exiting quickly with a non-zero status should report a failure to " + "activate"}, + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Running, + 0, + false, + {IComponent::RequestState::kSuccess}, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorAfterReady), + "A native, non-self-terminating process exiting quickly should report a failure to activate"}, + YieldTestCasesData{ + configuration::ApplicationType::Reporting, + configuration::ProcessState::Running, + 0, + true, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), + {IComponent::RequestState::kWaiting}, + "A reporting process exiting quickly (i.e. without waiting for a response from launch manager) should " + "report a failure to activate"}, + + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Terminated, + 0, + true, + {IComponent::RequestState::kWaiting}, + {IComponent::RequestState::kSuccess}, + "A native, self-terminating process exiting quickly with status 0 should report a successful activation"}, + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Terminated, + 111, + true, + {IComponent::RequestState::kWaiting}, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), + "A native, self-terminating process exiting quickly with a non-zero status should report a failure to " + "activate"}, + YieldTestCasesData{ + configuration::ApplicationType::Native, + configuration::ProcessState::Terminated, + 0, + false, + {IComponent::RequestState::kWaiting}, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), + "A native, non-self-terminating process exiting quickly should report a failure to activate"}, + YieldTestCasesData{ + configuration::ApplicationType::Reporting, + configuration::ProcessState::Terminated, + 0, + true, + score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), + {IComponent::RequestState::kWaiting}, + "A reporting process exiting quickly (i.e. without waiting for a response from launch manager) should " + "report a failure to activate"})); + // Bundles process crashes and timeouts that occur during activate(), before the ready condition is reached. class ProcessInfoNodeStartupCrashTest : public ProcessInfoNodeFixture { From 7cabceed0a43496765fe9687d74b8fb8daef4624 Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Tue, 1 Sep 2026 10:04:30 +0100 Subject: [PATCH 03/12] Initial fix --- .../details/process_info_node.cpp | 54 +++++++++++++------ .../details/process_info_node.hpp | 7 +++ 2 files changed, 44 insertions(+), 17 deletions(-) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp index ab943feecf..a4ee5d257b 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp @@ -174,17 +174,34 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ } else if (getState() < ProcessState::kRunning) { - // Defer to the startup thread to handle this setState(ProcessState::kTerminated); - unblockSync(); + if (isReporting()) + { + // Defer to the startup thread to handle this + unblockSync(); + } + else if (terminationIsValid(process_status)) + { + res = tryReportCompletion(ProcessState::kTerminated); + } + else + { + if (tryReportCompletion(ProcessState::kTerminated).value() == IComponent::RequestState::kSuccess) + { + res = score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady); + } + else + { + res = score::cpp::make_unexpected(IComponent::ComponentError::kErrorAfterReady); + } + } } else { setState(ProcessState::kTerminated); - if (config_.component_properties.application_profile.is_self_terminating && process_status == 0) + if (terminationIsValid(process_status)) { - // Only valid case for a process to terminate without it being requested res = tryReportCompletion(ProcessState::kTerminated); } else @@ -204,6 +221,17 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ return res; } +bool ProcessInfoNode::terminationIsValid(int32_t exit_code) const +{ + // Only valid case for a process to terminate without it being requested + return config_.component_properties.application_profile.is_self_terminating && exit_code == 0; +} + +bool ProcessInfoNode::isReporting() const +{ + return config_.component_properties.application_profile.application_type != configuration::ApplicationType::Native; +} + IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token stop_token) { LM_LOG_DEBUG() << "Starting process (" << identifier_ << ") from executable" << config_.deployment_config.bin_dir @@ -359,21 +387,13 @@ score::cpp::expected_blank ProcessInfoNode::handlePr score::cpp::expected_blank ProcessInfoNode::handleProcessAlreadyTerminated() { - if ((0 != exit_code_) || - (configuration::ApplicationType::Native != config_.component_properties.application_profile.application_type)) - { - // Error. To get a legal terminated before kRunning the process must be self-terminating, non-reporting - // and to have exited with zero exit code - LM_LOG_WARN() << "Got process termination before kRunning for pid" << pid_ << "(" << identifier_ << ")"; - // This will cause the graph to fail unless we have restart attempts left - return score::cpp::make_unexpected(ComponentError::kErrorBeforeReady); - } - else + // The process did start successfully, but didn't report properly. + if (isReporting()) { - // case of a self-terminating, non-reporting process exiting nicely before we've had a chance to put an - // entry in the map - return {}; + return score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady); } + // In all other cases, the process did successfully reach the running state (because pid_ was set). + return {}; } score::cpp::expected, IComponent::ComponentError> diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp index b059ec2466..18004b4d92 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp @@ -92,6 +92,13 @@ class ProcessInfoNode final : public IComponent [[nodiscard]] ControlClientChannelP getControlClientChannel() const; private: + /// @brief Returns true if this process terminating with code @p exit_code is acceptable even when termination has + /// not been requested. + bool terminationIsValid(int32_t exit_code) const; + + /// @brief Returns true if the process is configured to report kRunning + bool isReporting() const; + /// @brief Atomically transitions to new_state if the transition is valid. For reporting /// processes, also notifies the platform health manager of the state change. /// @param new_state The desired process state. From e8eb8f078b9c64e46217dd2afc7d0f21e43b8b73 Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Tue, 1 Sep 2026 10:34:16 +0100 Subject: [PATCH 04/12] Add graph UT --- .../details/graph_UT.cpp | 27 +++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp index f1ddd08b12..1edd263f8a 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp @@ -580,6 +580,33 @@ TEST_F(GraphHandleComponentEventTest, unexpectedTerminationDuringTransition) EXPECT_EQ(graph_->getPendingEvent(), ControlClientCode::kFailedUnexpectedTermination); } +class GraphTransitionFailuresTest : public GraphTest +{ +}; + +TEST_F(GraphTransitionFailuresTest, UnusualOrderOfFailures) +{ + RecordProperty( + "Description", + "Test that even if an unexpected termination is recieved before a successful activation, the graph reacts " + "correctly"); + + graph_->startTransition(IdentifierHash{run_target_name(0)}); + + const auto first_job = job_queue_->pop(); + const auto component_id = first_job.value()->component.get().getIdentifier(); + + EXPECT_CALL(process_interface_, requestTermination) + .WillOnce(Return(osal::OsalReturnType::kSuccess)); // Process is already gone, semaphore will time out + EXPECT_CALL(process_interface_, forceTermination).WillOnce(Return(osal::OsalReturnType::kFail)); + + graph_->handleComponentEvent(UnexpectedTermination{component_id}); + graph_->handleComponentEvent(ActivationSuccessful{component_id}); + + EXPECT_EQ(graph_->getPendingEvent(), ControlClientCode::kFailedUnexpectedTermination); + EXPECT_EQ(graph_->getState(), GraphState::kUndefinedState) << "Graph should be in a final state"; +} + class GraphCancelTest : public GraphTest { }; From bbf7a5eeaae0e6d61032d07bbafae2d3a9f2bdfb Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Tue, 1 Sep 2026 10:37:50 +0100 Subject: [PATCH 05/12] Comments --- .../src/process_group_manager/details/process_info_node.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp index a4ee5d257b..1112504c8b 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp @@ -178,15 +178,17 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ if (isReporting()) { - // Defer to the startup thread to handle this + // Defer to the startup thread to handle this (failed startup) unblockSync(); } else if (terminationIsValid(process_status)) { + // Termination during startup is okay in this case. This might also satisfy our ready condition res = tryReportCompletion(ProcessState::kTerminated); } else { + // This termination is not okay, but whether it was before or after ready depends on the ready condition if (tryReportCompletion(ProcessState::kTerminated).value() == IComponent::RequestState::kSuccess) { res = score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady); From de8758c686201f83093c3a64bc04969577362e07 Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Tue, 1 Sep 2026 12:38:18 +0100 Subject: [PATCH 06/12] Cleanup, add reason to unexpected term --- .../details/component_event.hpp | 1 + .../src/process_group_manager/details/graph.cpp | 4 +--- .../src/process_group_manager/details/graph.hpp | 9 +-------- .../src/process_group_manager/details/graph_UT.cpp | 7 ++++--- .../details/process_info_node.cpp | 14 +++++--------- .../details/process_monitor.cpp | 2 +- 6 files changed, 13 insertions(+), 24 deletions(-) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp index b668a23732..ea04e53e35 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp @@ -46,6 +46,7 @@ struct [[nodiscard]] DeactivationComplete struct [[nodiscard]] UnexpectedTermination { IdentifierHash node_identifier; + IComponent::ComponentError reason; }; /// @brief A job was queued but cancelled by the time it was processed diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/graph.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/graph.cpp index 2ae43239c4..966a5f5dc2 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/graph.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/graph.cpp @@ -349,9 +349,7 @@ void Graph::handleComponentEvent(const ComponentEvent& event) } else if constexpr (std::is_same_v) { - // This is always an error after ready - an unexpected termination before ready is an activation failure - const auto error = IComponent::ComponentError::kErrorAfterReady; - abort(1, error); + abort(1, data.reason); // Need to clean up any leftover resources IComponent& failingComponent = componentOf(nodes_[data.node_identifier]); diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/graph.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/graph.hpp index 8abb965a11..3ddce53280 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/graph.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/graph.hpp @@ -172,14 +172,7 @@ class Graph final /// @brief Move assignment operator(deleted). Graph& operator=(Graph&&) noexcept = delete; - /// @brief Applies a ComponentEvent — produced by ProcessMonitor from worker/OS-handler thread - /// callbacks and drained on the main thread — to this graph. - /// @details Dispatches on the event's variant: - /// - ActivationSuccessful / DeactivationComplete: `nodeExecuted(node_identifier, {})` - /// - ActivationFailed: `nodeExecuted(node_identifier, make_unexpected(reason))` - /// - UnexpectedTermination: `abort(1, kErrorAfterReady)` — ProcessMonitor::terminated() only - /// pushes this event once a process has already reached its ready condition, so it is - /// always a post-ready crash. + /// @brief Applies a ComponentEvent to this graph. /// @param event The event to process. void handleComponentEvent(const ComponentEvent& event); diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp index 1edd263f8a..43602b7407 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/graph_UT.cpp @@ -543,7 +543,8 @@ TEST_F(GraphHandleComponentEventTest, unexpectedTerminationDuringSuccess) }), Return(osal::OsalReturnType::kSuccess))); - graph_->handleComponentEvent(UnexpectedTermination{component->getIdentifier()}); + graph_->handleComponentEvent( + UnexpectedTermination{component->getIdentifier(), IComponent::ComponentError::kErrorAfterReady}); EXPECT_EQ(graph_->getState(), GraphState::kUndefinedState); } @@ -571,7 +572,7 @@ TEST_F(GraphHandleComponentEventTest, unexpectedTerminationDuringTransition) Return(osal::OsalReturnType::kSuccess))); // The active component then crashes - graph_->handleComponentEvent(UnexpectedTermination{component_index}); + graph_->handleComponentEvent(UnexpectedTermination{component_index, IComponent::ComponentError::kErrorAfterReady}); const auto second_job = job_queue_->pop(); executeJobSuccessfully(second_job->value()); @@ -600,7 +601,7 @@ TEST_F(GraphTransitionFailuresTest, UnusualOrderOfFailures) .WillOnce(Return(osal::OsalReturnType::kSuccess)); // Process is already gone, semaphore will time out EXPECT_CALL(process_interface_, forceTermination).WillOnce(Return(osal::OsalReturnType::kFail)); - graph_->handleComponentEvent(UnexpectedTermination{component_id}); + graph_->handleComponentEvent(UnexpectedTermination{component_id, IComponent::ComponentError::kErrorAfterReady}); graph_->handleComponentEvent(ActivationSuccessful{component_id}); EXPECT_EQ(graph_->getPendingEvent(), ControlClientCode::kFailedUnexpectedTermination); diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp index 1112504c8b..40a338a530 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp @@ -105,8 +105,7 @@ IComponent::RequestResult ProcessInfoNode::tryReportSuccess() std::optional ProcessInfoNode::getTimeForReport() const { - if (config_.component_properties.application_profile.application_type == - score::mw::lifecycle::internal::configuration::ApplicationType::Native) + if (!isReporting()) { return std::nullopt; } @@ -323,16 +322,13 @@ void ProcessInfoNode::setupControlClientChannel() score::cpp::expected_blank ProcessInfoNode::handleProcessStillStarting( const score::cpp::stop_token& stop_token) { - const bool is_native = - config_.component_properties.application_profile.application_type == configuration::ApplicationType::Native; - const bool startup_condition_met = std::visit( - [this, is_native, &stop_token](auto&& arg) -> bool { + [this, &stop_token](auto&& arg) -> bool { using T = std::decay_t; if constexpr (std::is_same_v) { - if (is_native) + if (!isReporting()) { // A native process does not report kRunning, so its exit code is the only readiness indication. return exit_code_ == 0; @@ -346,7 +342,7 @@ score::cpp::expected_blank ProcessInfoNode::handlePr else if constexpr (std::is_same_v) { - if (!is_native) + if (isReporting()) { // currently we do not support multiple ready conditions so we need // to ignore the krunning signal. @@ -418,7 +414,7 @@ ProcessInfoNode::handleProcessStarted(const score::cpp::stop_token& stop_token) void ProcessInfoNode::handleProcessRunning() { - if (configuration::ApplicationType::Native == config_.component_properties.application_profile.application_type) + if (!isReporting()) { LM_LOG_DEBUG() << "Considered kRunning for Non Reporting Process pid" << pid_ << "(" << identifier_ << ")"; } diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_monitor.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_monitor.cpp index 1b7401b189..3f8e6a79d2 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_monitor.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_monitor.cpp @@ -116,7 +116,7 @@ void ProcessMonitor::terminated(IComponent& component, int32_t status) bool push_res = true; if (!res.has_value()) { - push_res = event_queue_.push(UnexpectedTermination{component.getIdentifier()}); + push_res = event_queue_.push(UnexpectedTermination{component.getIdentifier(), res.error()}); } else if (res.value() != IComponent::RequestState::kWaiting) { From 8079393aeeead5343b1298425efb774ac7de08f2 Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:09:17 +0100 Subject: [PATCH 07/12] Better fix --- .../details/process_info_node.cpp | 74 +++++++++++++------ .../details/process_info_node.hpp | 10 +++ .../details/process_info_node_UT.cpp | 30 ++------ 3 files changed, 70 insertions(+), 44 deletions(-) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp index 40a338a530..37f0f913af 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp @@ -21,6 +21,7 @@ #include #include #include +#include namespace score::mw::lifecycle::internal { @@ -164,43 +165,36 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ LM_LOG_DEBUG() << "Process" << identifier_ << "( pid" << pid_ << ") terminated with exit code" << process_status; exit_code_ = process_status; IComponent::RequestResult res = {IComponent::RequestState::kWaiting}; + if (has_semaphore_.exchange(false)) { - // Termination was requested, we don't care if exit code is not 0 (a SIGKILL will set exit code to 9) setState(ProcessState::kTerminated); + + // Termination was requested, we don't care if exit code is not 0 (a SIGKILL will set exit code to 9) unblockSync(); static_cast(terminator_.post()); } - else if (getState() < ProcessState::kRunning) + else if (getState() == ProcessState::kStarting) { setState(ProcessState::kTerminated); + // In this case, we can't return anything because any definite result given here would invalidate startup + // recovery actions - if (isReporting()) - { - // Defer to the startup thread to handle this (failed startup) - unblockSync(); - } - else if (terminationIsValid(process_status)) + if (terminationIsValid(process_status)) { - // Termination during startup is okay in this case. This might also satisfy our ready condition - res = tryReportCompletion(ProcessState::kTerminated); + termination_result_ = true; } else { - // This termination is not okay, but whether it was before or after ready depends on the ready condition - if (tryReportCompletion(ProcessState::kTerminated).value() == IComponent::RequestState::kSuccess) - { - res = score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady); - } - else - { - res = score::cpp::make_unexpected(IComponent::ComponentError::kErrorAfterReady); - } + termination_result_ = false; } + + unblockSync(); } else { setState(ProcessState::kTerminated); + if (terminationIsValid(process_status)) { res = tryReportCompletion(ProcessState::kTerminated); @@ -260,6 +254,7 @@ IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token s pid_ = 0; exit_code_ = 0; error = std::nullopt; + termination_result_ = std::nullopt; static_cast(setState(score::mw::lifecycle::ProcessState::kStarting)); // Cannot fail by design if (osal::OsalReturnType::kSuccess == process_handling_.process_interface_->startProcess(pid_, sync_, config_)) @@ -309,8 +304,45 @@ IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token s return tryReportError(error.value()); } - setState(ProcessState::kRunning); // Can fail if we've terminated already - return tryReportCompletion(ProcessState::kRunning); + if (setState(ProcessState::kRunning)) + { + return tryReportCompletion(ProcessState::kRunning); + } + else // We have terminated after starting up + { + SCORE_LANGUAGE_FUTURECPP_ASSERT_DBG_MESSAGE( + termination_result_.has_value(), "setState(kRunning) failed without a termination result"); + // Assuming we have already waited for all ready conditions that require waiting, we have now started up and + // terminated. Therefore, as long as the termination was valid, we have satisfied any running or terminated + // condition. + if (termination_result_.value() == true) + { + return tryReportSuccess(); + } + else + { + // We successfully reached kRunning, but reached kTerminated in error. This affects whether the error + // occurred before or after reaching the ready state + return tryReportError(getErrorAfterState(ProcessState::kRunning)); + } + } +} + +IComponent::ComponentError ProcessInfoNode::getErrorAfterState(ProcessState state_reached) const +{ + if (const configuration::ProcessState* state_condition = + std::get_if(&config_.component_properties.ready_condition)) + { + if (*state_condition == configuration::ProcessState::Terminated && state_reached == ProcessState::kRunning) + { + return IComponent::ComponentError::kErrorBeforeReady; + } + } + if (state_reached < ProcessState::kRunning) + { + return IComponent::ComponentError::kErrorBeforeReady; + } + return IComponent::ComponentError::kErrorAfterReady; } void ProcessInfoNode::setupControlClientChannel() diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp index 18004b4d92..8251c4f43d 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp @@ -22,9 +22,11 @@ #include "score/mw/launch_manager/process_group_manager/details/safe_process_map.hpp" #include "score/mw/launch_manager/process_group_manager/process_state.hpp" #include "score/mw/launch_manager/supervision_control_client/isupervision_event_publisher.hpp" +#include "score/result/result.h" #include #include #include +#include namespace score::mw::lifecycle::internal { @@ -92,6 +94,10 @@ class ProcessInfoNode final : public IComponent [[nodiscard]] ControlClientChannelP getControlClientChannel() const; private: + /// @brief Given that an error has occurred after the process has reached state @p state_reached, return an error + /// indicating whether this was an error before the ready condition was satisfied, or after. + ComponentError getErrorAfterState(ProcessState state_reached) const; + /// @brief Returns true if this process terminating with code @p exit_code is acceptable even when termination has /// not been requested. bool terminationIsValid(int32_t exit_code) const; @@ -199,6 +205,10 @@ class ProcessInfoNode final : public IComponent /// @brief Unique hash to identify this node. IdentifierHash identifier_; + + /// @brief If the process has terminated during the last startup, holds a bool indicating whether the termination is + /// valid or not + std::optional termination_result_{}; }; } // namespace score::mw::lifecycle::internal diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp index 27cceeccc2..a17986b0a9 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node_UT.cpp @@ -291,7 +291,6 @@ struct YieldTestCasesData int status_to_exit_with_; bool is_self_terminating_; IComponent::RequestResult expected_activation_result_; - IComponent::RequestResult expected_term_result_; std::string description_; }; @@ -325,28 +324,21 @@ TEST_P(ProcessInfoNodeMapYieldTest, InsertReturnsYield) }), Return(SafeProcessMapReturnType::kYield))); - auto res = node->activate(score::cpp::stop_token{}); + auto activation_result_ = node->activate(score::cpp::stop_token{}); - ASSERT_EQ(res.has_value(), GetParam().expected_activation_result_.has_value()); + ASSERT_EQ(activation_result_.has_value(), GetParam().expected_activation_result_.has_value()); if (GetParam().expected_activation_result_.has_value()) { - EXPECT_EQ(res.value(), GetParam().expected_activation_result_.value()); + EXPECT_EQ(activation_result_.value(), GetParam().expected_activation_result_.value()); } else { - EXPECT_EQ(res.error(), GetParam().expected_activation_result_.error()); - } - - ASSERT_EQ(tryHandleTerminationResult.has_value(), GetParam().expected_term_result_.has_value()); - if (GetParam().expected_term_result_.has_value()) - { - EXPECT_EQ(tryHandleTerminationResult.value(), GetParam().expected_term_result_.value()); - } - else - { - EXPECT_EQ(tryHandleTerminationResult.error(), GetParam().expected_term_result_.error()); + EXPECT_EQ(activation_result_.error(), GetParam().expected_activation_result_.error()); } + ASSERT_TRUE(tryHandleTerminationResult.has_value()); + EXPECT_EQ(tryHandleTerminationResult.value(), IComponent::RequestState::kWaiting) + << "An error occurring during startup should never be reported by tryHandleTermination"; EXPECT_EQ(node->getState(), ProcessState::kTerminated); // Not kFailed, the posix process did start successfully } @@ -360,14 +352,12 @@ INSTANTIATE_TEST_SUITE_P( 0, true, {IComponent::RequestState::kSuccess}, - {IComponent::RequestState::kWaiting}, "A native, self-terminating process exiting quickly with status 0 should report a successful activation"}, YieldTestCasesData{ configuration::ApplicationType::Native, configuration::ProcessState::Running, 111, true, - {IComponent::RequestState::kSuccess}, score::cpp::make_unexpected(IComponent::ComponentError::kErrorAfterReady), "A native, self-terminating process exiting quickly with a non-zero status should report a failure to " "activate"}, @@ -376,7 +366,6 @@ INSTANTIATE_TEST_SUITE_P( configuration::ProcessState::Running, 0, false, - {IComponent::RequestState::kSuccess}, score::cpp::make_unexpected(IComponent::ComponentError::kErrorAfterReady), "A native, non-self-terminating process exiting quickly should report a failure to activate"}, YieldTestCasesData{ @@ -385,7 +374,6 @@ INSTANTIATE_TEST_SUITE_P( 0, true, score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), - {IComponent::RequestState::kWaiting}, "A reporting process exiting quickly (i.e. without waiting for a response from launch manager) should " "report a failure to activate"}, @@ -394,7 +382,6 @@ INSTANTIATE_TEST_SUITE_P( configuration::ProcessState::Terminated, 0, true, - {IComponent::RequestState::kWaiting}, {IComponent::RequestState::kSuccess}, "A native, self-terminating process exiting quickly with status 0 should report a successful activation"}, YieldTestCasesData{ @@ -402,7 +389,6 @@ INSTANTIATE_TEST_SUITE_P( configuration::ProcessState::Terminated, 111, true, - {IComponent::RequestState::kWaiting}, score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), "A native, self-terminating process exiting quickly with a non-zero status should report a failure to " "activate"}, @@ -411,7 +397,6 @@ INSTANTIATE_TEST_SUITE_P( configuration::ProcessState::Terminated, 0, false, - {IComponent::RequestState::kWaiting}, score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), "A native, non-self-terminating process exiting quickly should report a failure to activate"}, YieldTestCasesData{ @@ -420,7 +405,6 @@ INSTANTIATE_TEST_SUITE_P( 0, true, score::cpp::make_unexpected(IComponent::ComponentError::kErrorBeforeReady), - {IComponent::RequestState::kWaiting}, "A reporting process exiting quickly (i.e. without waiting for a response from launch manager) should " "report a failure to activate"})); From a44fb732ea1bd12df7194f1c0978d5c9fec82e89 Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Thu, 3 Sep 2026 15:59:56 +0100 Subject: [PATCH 08/12] Fixed a potential race --- .../details/process_info_node.cpp | 28 +++++++++---------- .../details/process_info_node.hpp | 2 +- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp index 37f0f913af..ceeb74bf65 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp @@ -166,32 +166,32 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ exit_code_ = process_status; IComponent::RequestResult res = {IComponent::RequestState::kWaiting}; + ProcessState starting = ProcessState::kStarting; + + if (terminationIsValid(process_status)) + { + termination_result_ = true; + } + else + { + termination_result_ = false; + } + if (has_semaphore_.exchange(false)) { + // Termination was requested, we don't care if exit code is not 0 (a SIGKILL will set exit code to 9) setState(ProcessState::kTerminated); - // Termination was requested, we don't care if exit code is not 0 (a SIGKILL will set exit code to 9) unblockSync(); static_cast(terminator_.post()); } - else if (getState() == ProcessState::kStarting) + else if (process_state_.compare_exchange_strong(starting, ProcessState::kTerminated)) { - setState(ProcessState::kTerminated); // In this case, we can't return anything because any definite result given here would invalidate startup // recovery actions - - if (terminationIsValid(process_status)) - { - termination_result_ = true; - } - else - { - termination_result_ = false; - } - unblockSync(); } - else + else // This is a termination during normal execution { setState(ProcessState::kTerminated); diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp index 8251c4f43d..76991435db 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp @@ -206,7 +206,7 @@ class ProcessInfoNode final : public IComponent /// @brief Unique hash to identify this node. IdentifierHash identifier_; - /// @brief If the process has terminated during the last startup, holds a bool indicating whether the termination is + /// @brief If the process has terminated since the last startup, holds a bool indicating whether the termination is /// valid or not std::optional termination_result_{}; }; From 3158c76e5530c7791df87a62394a3f9d3a939db5 Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:32:10 +0100 Subject: [PATCH 09/12] Remove method --- .../details/process_info_node.cpp | 21 +++++-------------- .../details/process_info_node.hpp | 4 ---- 2 files changed, 5 insertions(+), 20 deletions(-) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp index ceeb74bf65..358927ce53 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp @@ -165,17 +165,12 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ LM_LOG_DEBUG() << "Process" << identifier_ << "( pid" << pid_ << ") terminated with exit code" << process_status; exit_code_ = process_status; IComponent::RequestResult res = {IComponent::RequestState::kWaiting}; - ProcessState starting = ProcessState::kStarting; - if (terminationIsValid(process_status)) - { - termination_result_ = true; - } - else - { - termination_result_ = false; - } + const bool termination_is_valid = + config_.component_properties.application_profile.is_self_terminating && process_status == 0; + + termination_result_ = termination_is_valid; if (has_semaphore_.exchange(false)) { @@ -195,7 +190,7 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ { setState(ProcessState::kTerminated); - if (terminationIsValid(process_status)) + if (termination_is_valid) { res = tryReportCompletion(ProcessState::kTerminated); } @@ -216,12 +211,6 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ return res; } -bool ProcessInfoNode::terminationIsValid(int32_t exit_code) const -{ - // Only valid case for a process to terminate without it being requested - return config_.component_properties.application_profile.is_self_terminating && exit_code == 0; -} - bool ProcessInfoNode::isReporting() const { return config_.component_properties.application_profile.application_type != configuration::ApplicationType::Native; diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp index 76991435db..b513199c70 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp @@ -98,10 +98,6 @@ class ProcessInfoNode final : public IComponent /// indicating whether this was an error before the ready condition was satisfied, or after. ComponentError getErrorAfterState(ProcessState state_reached) const; - /// @brief Returns true if this process terminating with code @p exit_code is acceptable even when termination has - /// not been requested. - bool terminationIsValid(int32_t exit_code) const; - /// @brief Returns true if the process is configured to report kRunning bool isReporting() const; From c8e7635547c3f30e81477a761165092d7993782f Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Thu, 3 Sep 2026 16:50:09 +0100 Subject: [PATCH 10/12] Add a comment --- .../src/process_group_manager/details/process_info_node.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp index 358927ce53..70df3e6074 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp @@ -423,7 +423,10 @@ ProcessInfoNode::handleProcessStarted(const score::cpp::stop_token& stop_token) case score::mw::lifecycle::internal::SafeProcessMapReturnType::kOk: // Normal case, entry was put in // the map, process still running return handleProcessStillStarting(stop_token); - case score::mw::lifecycle::internal::SafeProcessMapReturnType::kYield: // Process has already exited + case score::mw::lifecycle::internal::SafeProcessMapReturnType::kYield: + // Process has already exited and tryHandleTermination has completed. + // tryHandleTermination is called by insertIfNotTerminated() and therefore executes in sequence in this + // case. return handleProcessAlreadyTerminated(); default: // Error case when pn == -1 // really bad fatal error, should not happen, treat as a failure to set the state & kill the process From b74ce4f2149b3dcd068f5bf0609ab58c132c44db Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:17:19 +0100 Subject: [PATCH 11/12] Remove some includes --- .../src/process_group_manager/details/process_info_node.cpp | 1 - .../src/process_group_manager/details/process_info_node.hpp | 1 - 2 files changed, 2 deletions(-) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp index 70df3e6074..7b8563a9cf 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp @@ -21,7 +21,6 @@ #include #include #include -#include namespace score::mw::lifecycle::internal { diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp index b513199c70..3338dd1bd2 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp @@ -26,7 +26,6 @@ #include #include #include -#include namespace score::mw::lifecycle::internal { From aaeb686343169b901d51e88e677f05a767dd8ecb Mon Sep 17 00:00:00 2001 From: William Roebuck <244554584+WilliamRoebuck@users.noreply.github.com> Date: Fri, 4 Sep 2026 15:55:49 +0100 Subject: [PATCH 12/12] Changes after review --- .../details/component_event.hpp | 8 +++ .../details/process_info_node.cpp | 51 ++++++++++--------- .../details/process_info_node.hpp | 17 +++++-- 3 files changed, 47 insertions(+), 29 deletions(-) diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp index ea04e53e35..686b1f5a33 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/component_event.hpp @@ -26,38 +26,46 @@ namespace score::mw::lifecycle::internal /// @brief A node finished activating successfully. struct [[nodiscard]] ActivationSuccessful { + /// @brief The identifier of the node that activated. IdentifierHash node_identifier; }; /// @brief A node failed to activate. struct [[nodiscard]] ActivationFailed { + /// @brief The identifier of the node that failed. IdentifierHash node_identifier; + /// @brief Description of the error. IComponent::ComponentError reason; }; /// @brief A node finished deactivating. struct [[nodiscard]] DeactivationComplete { + /// @brief The identifier of the node that deactivated. IdentifierHash node_identifier; }; /// @brief A node terminated without having been requested to. struct [[nodiscard]] UnexpectedTermination { + /// @brief The identifier of the node that terminated. IdentifierHash node_identifier; + /// @brief Description of the error. IComponent::ComponentError reason; }; /// @brief A job was queued but cancelled by the time it was processed struct [[nodiscard]] JobSkipped { + /// @brief The identifier of the node that was skipped. IdentifierHash node_identifier; }; /// @brief Alive supervision has failed for the given process identifier. struct [[nodiscard]] SupervisionFailure { + /// @brief The identifier of the node that failed. IdentifierHash process_identifier; }; diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp index 7b8563a9cf..b901b4cc43 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.cpp @@ -166,20 +166,24 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ IComponent::RequestResult res = {IComponent::RequestState::kWaiting}; ProcessState starting = ProcessState::kStarting; - const bool termination_is_valid = - config_.component_properties.application_profile.is_self_terminating && process_status == 0; - - termination_result_ = termination_is_valid; + if (config_.component_properties.application_profile.is_self_terminating && process_status == 0) + { + termination_result_ = TeminationResult::kOk; + } + else + { + termination_result_ = TeminationResult::kError; + } - if (has_semaphore_.exchange(false)) + if (has_semaphore_.exchange(false)) // Termination was requested { - // Termination was requested, we don't care if exit code is not 0 (a SIGKILL will set exit code to 9) + // We don't care if the termination was valid, we requested it (e.g. a SIGKILL will set exit code to 9) setState(ProcessState::kTerminated); unblockSync(); static_cast(terminator_.post()); } - else if (process_state_.compare_exchange_strong(starting, ProcessState::kTerminated)) + else if (process_state_.compare_exchange_strong(starting, ProcessState::kTerminated)) // Process still starting { // In this case, we can't return anything because any definite result given here would invalidate startup // recovery actions @@ -189,7 +193,7 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_ { setState(ProcessState::kTerminated); - if (termination_is_valid) + if (termination_result_ == TeminationResult::kOk) { res = tryReportCompletion(ProcessState::kTerminated); } @@ -242,7 +246,7 @@ IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token s pid_ = 0; exit_code_ = 0; error = std::nullopt; - termination_result_ = std::nullopt; + termination_result_ = TeminationResult::kNone; static_cast(setState(score::mw::lifecycle::ProcessState::kStarting)); // Cannot fail by design if (osal::OsalReturnType::kSuccess == process_handling_.process_interface_->startProcess(pid_, sync_, config_)) @@ -296,24 +300,21 @@ IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token s { return tryReportCompletion(ProcessState::kRunning); } - else // We have terminated after starting up + + // We have terminated after starting up + SCORE_LANGUAGE_FUTURECPP_ASSERT_DBG_MESSAGE( + termination_result_ != TerminationResult::kNone, "setState(kRunning) failed without a termination result"); + // Assuming we have already waited for all ready conditions that require waiting, we have now started up and + // terminated. Therefore, as long as the termination was valid, we have satisfied any running or terminated + // condition. + if (termination_result_ == TeminationResult::kOk) { - SCORE_LANGUAGE_FUTURECPP_ASSERT_DBG_MESSAGE( - termination_result_.has_value(), "setState(kRunning) failed without a termination result"); - // Assuming we have already waited for all ready conditions that require waiting, we have now started up and - // terminated. Therefore, as long as the termination was valid, we have satisfied any running or terminated - // condition. - if (termination_result_.value() == true) - { - return tryReportSuccess(); - } - else - { - // We successfully reached kRunning, but reached kTerminated in error. This affects whether the error - // occurred before or after reaching the ready state - return tryReportError(getErrorAfterState(ProcessState::kRunning)); - } + return tryReportSuccess(); } + + // We successfully reached kRunning, but reached kTerminated in error. This affects whether the error + // occurred before or after reaching the ready state + return tryReportError(getErrorAfterState(ProcessState::kRunning)); } IComponent::ComponentError ProcessInfoNode::getErrorAfterState(ProcessState state_reached) const diff --git a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp index 3338dd1bd2..bd41747abb 100644 --- a/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp +++ b/score/launch_manager/src/daemon/src/process_group_manager/details/process_info_node.hpp @@ -22,7 +22,6 @@ #include "score/mw/launch_manager/process_group_manager/details/safe_process_map.hpp" #include "score/mw/launch_manager/process_group_manager/process_state.hpp" #include "score/mw/launch_manager/supervision_control_client/isupervision_event_publisher.hpp" -#include "score/result/result.h" #include #include #include @@ -41,6 +40,17 @@ namespace score::mw::lifecycle::internal /// In the future, this class shall be split up to properly separate Component and Process lifecycle. class ProcessInfoNode final : public IComponent { + /// @brief Enum representing different outcomes of a termination. + enum class TeminationResult : uint8_t + { + /// @brief Since the last startup, the process has not terminated. + kNone, + /// @brief The last termination was acceptable. + kOk, + /// @brief The last termination was invalid/unexpected. + kError, + }; + public: /// @brief Constructs a ProcessInfoNode. /// @param config Configuration for the OS process. @@ -201,9 +211,8 @@ class ProcessInfoNode final : public IComponent /// @brief Unique hash to identify this node. IdentifierHash identifier_; - /// @brief If the process has terminated since the last startup, holds a bool indicating whether the termination is - /// valid or not - std::optional termination_result_{}; + /// @brief The result of the last termination since the process started. + TeminationResult termination_result_{}; }; } // namespace score::mw::lifecycle::internal