Fix incorrect behaviour on early termination

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

@@ -26,37 +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;
 };
 

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<T, UnexpectedTermination>)
             {
-                // 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]);

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);
 

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());
@@ -580,6 +581,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, IComponent::ComponentError::kErrorAfterReady});
+    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
 {
 };

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

@@ -105,8 +105,7 @@ IComponent::RequestResult ProcessInfoNode::tryReportSuccess()
 
 std::optional<timespec> ProcessInfoNode::getTimeForReport() const
 {
-    if (config_.component_properties.application_profile.application_type ==
-        score::mw::lifecycle::internal::configuration::ApplicationType::Native)
+    if (!isReporting())
     {
         return std::nullopt;
     }
@@ -165,26 +164,37 @@ 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))
+    ProcessState starting = ProcessState::kStarting;
+
+    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))  // 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<void>(terminator_.post());
     }
-    else if (getState() < ProcessState::kRunning)
+    else if (process_state_.compare_exchange_strong(starting, ProcessState::kTerminated))  // Process still starting
     {
-        // Defer to the startup thread to handle this
-        setState(ProcessState::kTerminated);
-
+        // In this case, we can't return anything because any definite result given here would invalidate startup
+        // recovery actions
         unblockSync();
     }
-    else
+    else  // This is a termination during normal execution
     {
         setState(ProcessState::kTerminated);
-        if (config_.component_properties.application_profile.is_self_terminating && process_status == 0)
+
+        if (termination_result_ == TeminationResult::kOk)
         {
-            // Only valid case for a process to terminate without it being requested
             res = tryReportCompletion(ProcessState::kTerminated);
         }
         else
@@ -204,6 +214,11 @@ IComponent::RequestResult ProcessInfoNode::tryHandleTermination(int32_t process_
     return res;
 }
 
+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
@@ -231,6 +246,7 @@ IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token s
         pid_ = 0;
         exit_code_ = 0;
         error = std::nullopt;
+        termination_result_ = TeminationResult::kNone;
         static_cast<void>(setState(score::mw::lifecycle::ProcessState::kStarting));  // Cannot fail by design
 
         if (osal::OsalReturnType::kSuccess == process_handling_.process_interface_->startProcess(pid_, sync_, config_))
@@ -280,8 +296,42 @@ 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);
+    }
+
+    // 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)
+    {
+        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
+{
+    if (const configuration::ProcessState* state_condition =
+            std::get_if<configuration::ProcessState>(&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()
@@ -293,16 +343,13 @@ void ProcessInfoNode::setupControlClientChannel()
 score::cpp::expected_blank<IComponent::ComponentError> 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<decltype(arg)>;
 
             if constexpr (std::is_same_v<T, configuration::ProcessState>)
             {
-                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;
@@ -316,7 +363,7 @@ score::cpp::expected_blank<IComponent::ComponentError> ProcessInfoNode::handlePr
             else if constexpr (std::is_same_v<T, configuration::FileState>)
             {
 
-                if (!is_native)
+                if (isReporting())
                 {
                     // currently we do not support multiple ready conditions so we need
                     // to ignore the krunning signal.
@@ -359,21 +406,13 @@ score::cpp::expected_blank<IComponent::ComponentError> ProcessInfoNode::handlePr
 
 score::cpp::expected_blank<IComponent::ComponentError> ProcessInfoNode::handleProcessAlreadyTerminated()
 {
-    if ((0 != exit_code_) ||
-        (configuration::ApplicationType::Native != config_.component_properties.application_profile.application_type))
+    // The process did start successfully, but didn't report properly.
+    if (isReporting())
     {
-        // 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
-    {
-        // 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<score::cpp::expected_blank<IComponent::ComponentError>, IComponent::ComponentError>
@@ -384,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
@@ -396,7 +438,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_ << ")";
     }

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

@@ -40,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.
@@ -92,6 +103,13 @@ 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 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.
@@ -192,6 +210,9 @@ class ProcessInfoNode final : public IComponent
 
     /// @brief Unique hash to identify this node.
     IdentifierHash identifier_;
+
+    /// @brief The result of the last termination since the process started.
+    TeminationResult termination_result_{};
 };
 
 }  // namespace score::mw::lifecycle::internal