Process monitor refactor by WilliamRoebuck · Pull Request #392 · eclipse-score/lifecycle

WilliamRoebuck · 2026-07-30T11:49:05Z

The description below is not AI generated. It includes some key information needed to begin a review.

Overview

This is the completion of the refactor started in #370. With these changes, the following is achieved:

Transitions now activate run targets instead of states and operate on the component level, rather than processes. See IComponent. This means that different types of component can now be implemented, such as containers.
Ready conditions implemented in code.
Graph now processes events sequentially rather than in parallel. See ComponentEventQueue.
ProcessInfoNode is no longer responsible for dependency resolution. This responsibility is moved to Transition.
ProcessInfoNode & Graph coupling resolved - ProcessInfoNode no longer has access to Graph.

New components

DependencyGraph - Data structure for storing and traversing component dependencies
Transition - Resolves run target activation logic. Activates and deactivates components as required.

Behavioral change

Processes are now managed based on component state, rather than process state. This means that once a component is activated, the underlying process shall not be started again until the component becomes inactive (either due to an error or a run target activation). Previously, a self-terminating process would be restarted on every run target activation that included it.

Minor changes

kLaunchManager comms type removed. This was legacy and no longer needed.
Configuration now generates an "Off" run target if none is configured
Several new interfaces have been added to allow for unit testing of existing components

WIP (while this PR is a draft)

Unit tests for Graph & ProcessInfoNode
Some remaining outdated documentation

Review strategy

As this PR is quite large, it may be necessary for reviewers each to tackle subsections of the changes. In terms of line numbers, these sections are roughly equal:

graph.cpp + graph.hpp (+ unit tests)
processinfonode.cpp + processinfonode.hpp (+ unit tests)
transition.hpp + transition_UT.hpp
dependency_graph.hpp + dependency_graph_UT.cpp + process_group_manager.cpp + process_group_manager.hpp
Any remaining unreviewed changes

Introduce Run Target handling in refactoring * Run Target exploration * Small improvements * Restore table format * Make active flag atomic Proposal for component event queue * Proposal for central event queue * Introduce MPSC queue for event queue * Make use of concurrency error domain * Remove atomics/mutexes that now obsolete * Apply review findings * Move doxyen above the line * Remove blocking push behavior * Change queue push API * EventQueue only accepting r-value refs * Fixes after rebase Fixes after rebase Fix transition to Off states during shutdown * Fix transition to Off * Fix transition to off v2 * Fix test flakyness * RunTarget optimization * Fix copyright Fix race condition for self-term proc Integrate recoveryclient with event queue * Integrate recoveryclient with event queue * Fix include header Fix build with new config Fix concurrency build file Fix compiler errors & warnings Fix component event queue Separate transition bookkeeping from DependencyGraph topology * poc * Improve the transition design * Rollout to production code * Improve documentation * Remove graph poc folder * Improvements and more testing * fix compiler errors * Traverse whole graph on deactivate * Remove initial transition special case * Remove special case for Off * Improve documentation * Cleanup * Remove unneeded file * Cleanup documentation * Remove private inheritance * Reduce component states to minimum * Remove redundant checks * Replace component state with flag * Simply tryReportState method

github-actions · 2026-07-30T11:49:55Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run --lockfile_mode=error //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.6.0) and connecting to it...
INFO: Invocation ID: 58945471-42e6-414b-94d4-1a2761372d06
Computing main repo mapping: 
Computing main repo mapping: 
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)

Analyzing: target //:license-check (33 packages loaded, 10 targets configured)

Analyzing: target //:license-check (83 packages loaded, 10 targets configured)

Analyzing: target //:license-check (86 packages loaded, 10 targets configured)

Analyzing: target //:license-check (139 packages loaded, 2370 targets configured)

Analyzing: target //:license-check (152 packages loaded, 5482 targets configured)

Analyzing: target //:license-check (153 packages loaded, 7256 targets configured)

Analyzing: target //:license-check (161 packages loaded, 8241 targets configured)

Analyzing: target //:license-check (164 packages loaded, 8271 targets configured)

Analyzing: target //:license-check (164 packages loaded, 8271 targets configured)

Analyzing: target //:license-check (164 packages loaded, 8271 targets configured)

Analyzing: target //:license-check (168 packages loaded, 10283 targets configured)

INFO: Analyzed target //:license-check (169 packages loaded, 10409 targets configured).
[12 / 16] JavaToolchainCompileClasses external/rules_java+/toolchains/platformclasspath_classes; 0s disk-cache, processwrapper-sandbox
[14 / 16] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar; 0s disk-cache, processwrapper-sandbox
[15 / 16] Building license.check.license_check.jar (); 0s disk-cache, multiplex-worker
INFO: Found 1 target...
Target //:license.check.license_check up-to-date:
  bazel-bin/license.check.license_check
  bazel-bin/license.check.license_check.jar
INFO: Elapsed time: 24.800s, Critical Path: 2.76s
INFO: 16 processes: 12 internal, 3 processwrapper-sandbox, 1 worker.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
usage: org.eclipse.dash.licenses.cli.Main [-batch <int>] [-cd <url>]
       [-confidence <int>] [-ef <url>] [-excludeSources <sources>] [-help] [-lic
       <url>] [-project <shortname>] [-repo <url>] [-review] [-summary <file>]
       [-timeout <seconds>] [-token <token>]

github-actions · 2026-07-30T12:09:05Z

The created documentation from the pull request is available at: docu-html

Unit Tests for ProcessInfoNode

NicolasFussberger · 2026-07-31T13:29:54Z

                                    sigterm = static_cast<uint8_t>(score::lcm::ProcessState::kTerminating),
-                                    off = static_cast<uint8_t>(score::lcm::ProcessState::kTerminated)};
+                                    off = static_cast<uint8_t>(score::lcm::ProcessState::kTerminated),
+                                    failed = static_cast<uint8_t>(score::lcm::ProcessState::kFailed)};


I see that failed state is currently not reachable from Running state.
However, to be safe maybe we should nonetheless make sure the AliveSupervision treats a failed process state the same as off or sigterm

score/launch_manager/src/daemon/src/alive_monitor/details/supervision/Alive.cpp:

if (std::holds_alternative<ProcessStateSnapshot>(f_updateEvent)) { const auto& snapshot = std::get<ProcessStateSnapshot>(f_updateEvent); if (snapshot.eProcState == ifexm::ProcessState::EProcState::running) { return EUpdateEventType::kActivation; } if (snapshot.eProcState == ifexm::ProcessState::EProcState::sigterm || snapshot.eProcState == ifexm::ProcessState::EProcState::off || + snapshot.eProcState == ifexm::ProcessState::EProcState::failed) { return EUpdateEventType::kDeactivation; } return EUpdateEventType::kNoChange; }

NicolasFussberger · 2026-07-31T13:39:01Z

-    ITerminationCallback(ITerminationCallback&&) = default;
-    ITerminationCallback& operator=(ITerminationCallback&&) = default;
-};
+using namespace score::mw::lifecycle::internal;


I wonder if we should remove any "using namespace" from header files.
Anything that includes the header transitively pulls every name from score::mw::lifecycle::internal into score::lcm::internal scope, which could result in unexpected naming collisions.

Though this exists in several places: process_info_node.hpp:37, transition.hpp:34, component_of.hpp:25, dependency_graph.hpp:26, graph.hpp:47

It could be done in a separate PR to clean this up.

NicolasFussberger · 2026-07-31T13:47:37Z

+        return nodes.capacity();
+    }
+
+    T& operator[](GraphIndex index)


should we add a const overload?

NicolasFussberger · 2026-07-31T13:49:34Z

+    }
+
+    /// @brief Traverse the graph, starting at @p start, performing @p per_node on each node and moving to the nodes
+    /// provided by the return value from @p per_node. Only nodes such that @c filter(node) is true are traversed.


nitpick: The provided start node is traversed whether the filter for this node is true or not

NicolasFussberger · 2026-07-31T13:51:41Z

+    /// provided by the return value from @p per_node. Only nodes such that @c filter(node) is true are traversed.
+    /// Nodes are visited at most once.
+    template <typename PerNodeFn, typename FilterFn>
+    void traverse(const GraphIndex start, PerNodeFn per_node, FilterFn filter)


Maybe the filter could be optional or removed entirely, as it is currently not being used by the code.

It is used but making it optional would be nice

NicolasFussberger · 2026-07-31T13:53:13Z

+
+  public:
+    /// @param count The exact number of nodes that will be added.
+    DependencyGraph(std::size_t count) : traversal_queue(std::max(count, std::size_t(2)) - 1)


Maybe a comment why the queue size is determined like this would be good

NicolasFussberger · 2026-07-31T13:55:42Z

+    {
+        nodes[node].depends_on.push_back(depends_on);
+        nodes[depends_on].dependents.push_back(node);
+    }


I wonder if an assert makes sense that we are not pushing dependencies beyond the capacity.
At lest for now, I think this is the expectation.

danth

Various nitpicks and ramblings. I have seen all the code but did not check the unit tests in much detail yet.

danth · 2026-07-31T13:15:18Z

+        while (!traversal_queue.empty())
+        {
+            const auto pop_res = traversal_queue.tryPop();
+            SCORE_LANGUAGE_FUTURECPP_ASSERT_PRD_MESSAGE(


Why do some checks use PRD and some not?

danth · 2026-07-31T14:16:50Z

+        }
+    };
+
+    /// @returns Iterator at the beginning of the nodes store.


I think it's @return not @returns, same below

danth · 2026-07-31T14:26:09Z

A few of these methods have docs for parameters which do not exist

danth · 2026-07-31T14:35:07Z

+    /// provided by the return value from @p per_node. Only nodes such that @c filter(node) is true are traversed.
+    /// Nodes are visited at most once.
+    template <typename PerNodeFn, typename FilterFn>
+    void traverse(const GraphIndex start, PerNodeFn per_node, FilterFn filter)


It is used but making it optional would be nice

danth · 2026-07-31T14:46:09Z

+            report_state_lambda,
+            pgm_->getProcessInterface(),
+            pgm_->getProcessMap());
+        assert(index == process_id && "Graph indicies must line up with os process indices");


Use the SCORE_LANGUAGE_FUTURECPP_ assertions

danth · 2026-07-31T15:16:08Z

 {
-    if (0 >= --nodes_in_flight_)
+    auto from_state = getState();
+    if (from_state < GraphState::kAborting)


Not sure if I like relying on the numeric value of the enum for comparisons. Feel free to argue

danth · 2026-07-31T15:33:14Z

+            break;
    }
-    else
+    assert(desired_state.has_value() && "Ready condition is not implemented");


If this must be implemented for every possible ready condition, removing the default case would let the compiler emit a warning if one is missing. Since we treat warnings as errors it would therefore fail to build.

Otherwise: Use SCORE_LANGUAGE_FUTURECPP_ assertions rather than normal assert

danth · 2026-07-31T15:38:54Z

 }

-void ProcessInfoNode::startProcess()
+IComponent::RequestResult ProcessInfoNode::startProcess(score::cpp::stop_token stop_token)


Same thing with the SCORE_LANGUAGE_FUTURECPP_ assertions in this method 😄

danth · 2026-07-31T15:49:20Z

+/// @details When transitioning to a target state, the Transition computes the nodes that need to be
+/// deactivated (those nodes not reachable from the target state) and those that need to be activated (those reachable
+/// from the target that are not yet active). This list is then used to drive the transition by iterating over the ready
+/// nodes (@ref nextReady()) and marking them as finished once activated/deactivated (@ref onNodeFinished()). The
+/// transition is split into two phases: Stopping phase and Starting phase. Stopping Phase: Every node that is currently
+/// running (@ref stopped() is false) and is not part of the target subgraph is deactivated. The stop set is derived
+/// from live component state across the whole graph rather than from a source subgraph, so it also captures nodes left
+/// running by a previously aborted transition. Once all nodes in the Stopping phase are finished, the transition moves
+/// to the Starting phase. Starting Phase: The nodes that need to be activated are processed next. Once all nodes in the
+/// Starting phase are finished, the transition is complete.
+///


Could we break up this text into paragraphs?

danth · 2026-07-31T15:57:05Z

+    void beginSetup(GraphIndex target)
+    {
+        state_.target_root = target;
+        clearNextNodes();
+        state_.pending = 0;
+        state_.phase = Phase::Stopping;
+    }


This beginSetup method is currently only used for setupDeactivation, I think the _target_root and phase lines could be removed, and done directly by the caller.

Then it could be called within setupActivation / setupDeactivation so the caller does not need to do additional setup first.

The std::fill(false) which is duplicated across both methods could also be moved into beginSetup.

WilliamRoebuck and others added 5 commits July 29, 2026 14:27

ProcessGroupMonitor Refactoring

aaf6777

Initial fixes after rebase

1598c23

Revert test changes

140a668

Cleanup

e5d5a33

github-project-automation Bot moved this to Backlog in LCM - Lifecycle & Health FT Jul 30, 2026

github-project-automation Bot added this to LCM - Lifecycle & Health FT Jul 30, 2026

WilliamRoebuck requested a deployment to workflow-approval July 30, 2026 11:49 — with GitHub Actions Waiting

Introduce interface for SafeProcessMap

099b270

NicolasFussberger requested a deployment to workflow-approval July 30, 2026 11:53 — with GitHub Actions Waiting

Restore force termination method

838690c

WilliamRoebuck requested a deployment to workflow-approval July 30, 2026 12:13 — with GitHub Actions Waiting

MaciejKaszynski assigned WilliamRoebuck Jul 30, 2026

NicolasFussberger and others added 6 commits July 31, 2026 07:46

Setup mocks

a6eeb96

ProcessInfoNode unit tests

6af4057

Remove stop_token usage

4005624

Include safeprocessmap header

4facb4c

Assert more process state transitions

29fc91b

Merge pull request #4 from etas-contrib/feature/processinfonode-uts

ce22794

Unit Tests for ProcessInfoNode

WilliamRoebuck temporarily deployed to workflow-approval July 31, 2026 12:19 — with GitHub Actions Inactive

NicolasFussberger reviewed Jul 31, 2026

View reviewed changes

danth suggested changes Jul 31, 2026

View reviewed changes

Uh oh!

Conversation

WilliamRoebuck commented Jul 30, 2026

Overview

New components

Behavioral change

Minor changes

WIP (while this PR is a draft)

Review strategy

Uh oh!

github-actions Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jul 30, 2026 •

edited

Loading