Graph refactor initial components by WilliamRoebuck · Pull Request #370 · eclipse-score/lifecycle

WilliamRoebuck · 2026-07-24T14:58:25Z

This PR introduces some initial components required for an upcoming refactor to the Graph and ProcessInfoNode classes. Firstly, we want to introduce some components that are more easily understood and unit tested. The next PR will integrate these into existing code. Excluding interfaces and data-only classes, the new components are:

ComponentEventQueue - Allows us to handle component state changes sequentially in Graph
ProcessMonitor - Decouples ProcessInfoNode from Graph and allows us to simplify its APIs

The refactor aims to address the following concerns:

Race conditions throughout Graph
Support for different component types than POSIX processes, e.g. containers
Circular dependencies impeding unit test development
Outdated "PG state" based transition logic
Overlapping & confusing class responsibilities

github-actions · 2026-07-24T14:59:22Z

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: b1999356-4e3f-4d11-86c1-a557fb2f34c8
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 (85 packages loaded, 10 targets configured)

Analyzing: target //:license-check (144 packages loaded, 2767 targets configured)

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

Analyzing: target //:license-check (156 packages loaded, 7404 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 (167 packages loaded, 10159 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 ... (2 actions, 1 running)
[15 / 16] [Prepa] Building license.check.license_check.jar ()
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.709s, Critical Path: 2.71s
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-24T15:11:57Z

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

MaciejKaszynski

Looks good, would really like to split this into separate folders because it's a bit unclear if we are at the component level or process level.

MaciejKaszynski · 2026-07-27T06:14:09Z

 #include <cstdint>

-namespace score {
+namespace score


I think the namespace should be score::mw::lifecycle so here score::mw::lifecycle::internal

This whole PR could update this.

Should we use this namespace for the new code and then update the other namespaces with a separate PR?

I would say let's just fix what is touched with this PR

Done in bcbda43

MaciejKaszynski · 2026-07-27T06:14:34Z

+{

-namespace lcm {
+namespace lcm


We should also enable the concat-nested-namespaces clang-tidy rule to concatenate the nested namepsaces

MaciejKaszynski · 2026-07-27T06:28:59Z

-        256U,  ///< Maximum number of ControlClient instances that should be created by state manager. If state manager create more instances than kMaxInstances, those instances will always return kCommunicationError when used
+enum class ControlClientLimits : uint16_t
+{
+    kControlClientMaxInstances = 256U,  ///< Maximum number of ControlClient instances that should be created by state


I don't think you actually touched this line but could you convert this to just a variable and have the enum become a namespace. I think then this all would be less weird.

I've reverted the changes in this file now, it wouldn't make sense to change this in this PR

MaciejKaszynski · 2026-07-27T06:43:16Z

+TEST_F(ComponentEventQueueTest, WaitForEventsReturnsFalseOnEmptyQueue)
+{
+    RecordProperty("Description", "Verify waitForEvents returns false promptly when no event has been pushed.");
+    EXPECT_FALSE(queue_.waitForEvents(std::chrono::milliseconds{1}));


Suggested change

EXPECT_FALSE(queue_.waitForEvents(std::chrono::milliseconds{1}));

EXPECT_FALSE(queue_.waitForEvents(std::chrono::milliseconds{0}));

The queue should handle 0 here.

Resolved in c87ac1c

MaciejKaszynski · 2026-07-27T07:23:13Z

+namespace score::lcm::internal
+{
+
+class IComponentEventReceiver


Done in acaf3c9

MaciejKaszynski · 2026-07-27T07:25:44Z

+
+    /// @brief Start work on @p task and push the result to the event queue if the task completes
+    void doWork(Task task) override;
+    /// @brief Notify @p component that it has terminated with status @p status. If this is an error or finishes a


Can you space stuff out so it's more readable.

Suggested change

/// @brief Notify @p component that it has terminated with status @p status. If this is an error or finishes a

/// @brief Notify @p component that it has terminated with status @p status. If this is an error or finishes a

Done in acaf3c9

MaciejKaszynski · 2026-07-27T07:27:02Z

+    void terminated(IComponent& component, int32_t status) override;
+
+  private:
+    void taskFinished(const Task& task, const score::cpp::expected_blank<IComponent::ComponentError>& error);


still need briefs on private :) also feel like the name should be markFinished.

Actually you might want to just make this a lambda in doWork

Resolved in ccdddfe

MaciejKaszynski · 2026-07-27T07:27:42Z

+using namespace testing;
+using namespace score::lcm::internal;
+
+class MockComponent : public IComponent


Can you make this into a separate header for easier re-use.

Done in dc6aa46

MaciejKaszynski · 2026-07-27T07:30:03Z

+class IComponentController
+{
+  public:
+    virtual void doWork(Task task) = 0;


Are you sure about copying, seems like we could just always take a && here?

Resolved in cd69ca9

NicolasFussberger · 2026-07-27T07:46:23Z

+
+/// @brief Queue of ComponentEvents produced by worker/OS-handler threads and consumed
+/// exclusively by the main thread, backed by a fixed-capacity MpscBoundedQueue.
+class ComponentEventQueue final : public IComponentEventReceiver


Should we rename this interface to e.g. IComponentEventPublisher?
It looks like its for publishing events into the queue, not actually for reading events from the queue

Yes, the name seems to be incorrect. Should we have two interfaces IComponentEventPublisher and IComponentEventSubscriber? And the queue would implement both

Added in acaf3c9 and 2052521

danth · 2026-07-27T09:55:18Z

 };

+/// @brief Capacity of the ComponentEventQueue.
+constexpr std::size_t kComponentEventQueueSize = 1024U;


Maybe copy the way it is done in MpscBoundedQueue, and make the ComponentEventQueue take the capacity as a type parameter:

lifecycle/score/launch_manager/src/daemon/src/common/concurrency/mpsc_bounded_queue.hpp

Lines 40 to 41 in 2e68cc8

template <typename T, std::size_t Capacity>

class MpscBoundedQueue

This would allow mocking the capacity in tests, having multiple queues with different capacities, etc

No longer relevant due to #367

danth · 2026-07-27T10:08:37Z

+        default:
+            break;


Should we add SCORE_LANGUAGE_FUTURECPP_UNREACHABLE here? If someone defines a new task type and does not add code to handle it the behaviour is not clear

Resolved in ccdddfe

danth · 2026-07-27T10:11:32Z

+    if (res.has_value() && res.value() == IComponent::RequestState::kWaiting)
+    {
+        return;
+    }
+
+    if (res.has_value())
+    {
+        taskFinished(task, {});
+    }
+    else
+    {
+        taskFinished(task, score::cpp::make_unexpected(res.error()));
+    }


Personally I would find this easier to read if it were nested

Suggested change

if (res.has_value() && res.value() == IComponent::RequestState::kWaiting)

{

return;

}

if (res.has_value())

{

taskFinished(task, {});

}

else

{

taskFinished(task, score::cpp::make_unexpected(res.error()));

}

if (res.has_value())

{

if (res.value() == IComponent::RequestState::kWaiting)

{

return;

}

taskFinished(task, {});

}

else

{

taskFinished(task, score::cpp::make_unexpected(res.error()));

}

Resolved in ccdddfe

danth · 2026-07-27T10:15:34Z

+    else if (error.has_value())
+    {
+        event_queue_.push(ActivationSuccessful{node_index});
+    }
+    else
+    {
+        event_queue_.push(ActivationFailed{node_index, error.error()});
+    }


This is correct but because of the naming it reads the wrong way round- "if there is an error, push successful"...

Maybe rename the parameter to something like status or result?

Resolved in ccdddfe

danth · 2026-07-27T10:18:05Z

@@ -1,2 +1,2 @@
 Checks: ' -*, bugprone-*, cert-*, clang-analyzer-core.*, clang-analyzer-cplusplus.*, clang-analyzer-security.*, cppcoreguidelines-pro-bounds-pointer-arithmetic, cppcoreguidelines-pro-type-cstyle-cast, hicpp-* '
-WarningsAsErrors: 'bugprone-*,cert-*,clang-analyzer-*,hicpp-*'
+WarningsAsErrors: 'bugprone-*,cert-*,clang-analyzer-*'


Why was this changed?

I made all of my interfaces show up as errors 😄 . I've refined it now in acaf3c9, I added an option that allows classes with default destructors only

danth · 2026-07-27T10:47:45Z

+    RecordProperty("Description",
+                   "Verify stop() causes a subsequently-called waitForEvents() to return false immediately "
+                   "rather than blocking, matching the shutdown usage in ProcessGroupManager::deinitialize().");
+    queue_.stop();
+    EXPECT_FALSE(queue_.waitForEvents(std::chrono::milliseconds{2000}));


This test would still pass if it blocks. Or we are relying on the timeout of the test framework to make it fail

I don't think there's any way to test this without fragility, I've changed the test slightly: ebc75c3

NicolasFussberger · 2026-07-27T13:35:12Z

+{
+
+class ComponentEventQueueTest : public ::testing::Test
+{


I think we need to add here
RecordProperty("TestType", "interface-test");
RecordProperty("DerivationTechnique", "explorative-testing");

Fixed in acaf3c9

This resolves several review comments: - `taskFinished` is no longer present in the header - Confusing nesting is improved by having a clear switch statement for each purpose - By removing the default case we get a compiler warning if a possible value is not handled

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

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

WilliamRoebuck requested a deployment to workflow-approval July 24, 2026 14:58 — with GitHub Actions Waiting

WilliamRoebuck force-pushed the feature/process-mon-refactor-1 branch from fe8eca0 to 04b715d Compare July 24, 2026 15:00

WilliamRoebuck requested a deployment to workflow-approval July 24, 2026 15:00 — with GitHub Actions Waiting

WilliamRoebuck force-pushed the feature/process-mon-refactor-1 branch from 04b715d to 5001193 Compare July 24, 2026 15:02

WilliamRoebuck requested a deployment to workflow-approval July 24, 2026 15:02 — with GitHub Actions Waiting

WilliamRoebuck force-pushed the feature/process-mon-refactor-1 branch from 5001193 to 9c02cd9 Compare July 24, 2026 15:11

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

WilliamRoebuck mentioned this pull request Jul 24, 2026

PoC of https://github.com/orgs/eclipse-score/discussions/2934 #342

Draft

MaciejKaszynski requested changes Jul 27, 2026

View reviewed changes

NicolasFussberger reviewed Jul 27, 2026

View reviewed changes

danth suggested changes Jul 27, 2026

View reviewed changes

danth requested a deployment to workflow-approval July 27, 2026 10:42 — with GitHub Actions Waiting

danth reviewed Jul 27, 2026

View reviewed changes

danth requested a deployment to workflow-approval July 27, 2026 10:54 — with GitHub Actions Waiting

danth requested a deployment to workflow-approval July 27, 2026 11:00 — with GitHub Actions Waiting

danth requested a deployment to workflow-approval July 27, 2026 12:42 — with GitHub Actions Waiting

NicolasFussberger reviewed Jul 27, 2026

View reviewed changes

Bring in files

1f0b1cc

WilliamRoebuck and others added 7 commits July 28, 2026 09:38

Add process monitor UT

c795da9

More docs

303e74b

Use rvalue for parameter to doWork

a797fcc

Reduce waitForEvents timeout to zero in tests

3b7a6e9

Rename Task to ComponentTask

cc18bc7

Update namespaces, use new queue format

bcbda43

WilliamRoebuck force-pushed the feature/process-mon-refactor-1 branch from ccdddfe to bcbda43 Compare July 28, 2026 08:56

WilliamRoebuck requested a deployment to workflow-approval July 28, 2026 08:56 — with GitHub Actions Waiting

WilliamRoebuck added 2 commits July 28, 2026 11:47

Changes after review

acaf3c9

Renames, create mock_component.hpp

dc6aa46

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

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

WilliamRoebuck added 2 commits July 28, 2026 13:08

Revert changes in constants, add testonly

110b59d

Improve event queue interfaces

2052521

WilliamRoebuck force-pushed the feature/process-mon-refactor-1 branch from 128f839 to 2052521 Compare July 28, 2026 12:14

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

Fix incorrect test

ebc75c3

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

WilliamRoebuck requested review from MaciejKaszynski and danth July 28, 2026 12:33

Fix format and copyright

55c822b

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

MaciejKaszynski mentioned this pull request Jul 28, 2026

Cleanup process_group_manager directory #375

Open

	EXPECT_FALSE(queue_.waitForEvents(std::chrono::milliseconds{1}));
	EXPECT_FALSE(queue_.waitForEvents(std::chrono::milliseconds{0}));

	/// @brief Notify @p component that it has terminated with status @p status. If this is an error or finishes a

	/// @brief Notify @p component that it has terminated with status @p status. If this is an error or finishes a

	template <typename T, std::size_t Capacity>
	class MpscBoundedQueue

Uh oh!

Conversation

WilliamRoebuck commented Jul 24, 2026

Uh oh!

github-actions Bot commented Jul 24, 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 24, 2026

Uh oh!

MaciejKaszynski 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!

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!

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!

NicolasFussberger Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Jul 24, 2026 •

edited

Loading

NicolasFussberger Jul 27, 2026 •

edited

Loading