hmon: approach to monitors rework by arkjedrz · Pull Request #81 · eclipse-score/lifecycle

arkjedrz · 2026-02-17T11:51:37Z

Change approach to monitors abstraction.
Improve docs.
Small esthetics fixes.

github-actions · 2026-02-17T11:52:15Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.4.2) and connecting to it...
INFO: Invocation ID: 6292fb3f-9cfb-4721-b6ef-296dbd363331
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'score_rust_policies', the root module requires module version score_rust_policies@0.0.3, but got score_rust_policies@0.0.5 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
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)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)

Analyzing: target //:license-check (46 packages loaded, 9 targets configured)

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

Analyzing: target //:license-check (137 packages loaded, 1442 targets configured)

Analyzing: target //:license-check (148 packages loaded, 5172 targets configured)

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

Analyzing: target //:license-check (155 packages loaded, 7768 targets configured)

Analyzing: target //:license-check (160 packages loaded, 7817 targets configured)

Analyzing: target //:license-check (160 packages loaded, 7817 targets configured)

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

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

INFO: Analyzed target //:license-check (165 packages loaded, 9955 targets configured).
[14 / 16] [Prepa] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar
[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: 26.096s, Critical Path: 2.81s
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-02-17T11:55:23Z

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

Copilot

Pull request overview

This pull request refactors the health monitoring library to remove unnecessary abstractions, improve type safety, and better organize the codebase. The changes introduce separate MonitorTag and DeadlineTag types to replace the generic IdentTag, eliminate the MonitorEvaluator trait in favor of direct usage of Arc<DeadlineMonitorInner>, and reorganize the supervisor API client code into a dedicated module with proper feature flags.

Changes:

Introduced MonitorTag and DeadlineTag types for better type safety and clarity
Removed MonitorEvaluator trait abstraction and MonitorEvalHandle wrapper
Moved supervisor API client implementations to dedicated module with mutually exclusive feature flags
Improved documentation throughout the codebase
Fixed Cargo build configuration with proper feature flag management

Reviewed changes

Copilot reviewed 26 out of 28 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/health_monitoring_lib/rust/tag.rs	New module implementing `MonitorTag` and `DeadlineTag` with FFI-safe memory management
src/health_monitoring_lib/rust/worker.rs	Updated to use `DeadlineMonitorInner` directly and import `SupervisorAPIClient` from new module
src/health_monitoring_lib/rust/supervisor_api_client/*.rs	New module structure with feature-gated implementations
src/health_monitoring_lib/rust/lib.rs	Refactored monitor state management using `MonitorState` enum
src/health_monitoring_lib/rust/ffi.rs	Updated FFI functions to use new tag types
src/health_monitoring_lib/rust/deadline/*.rs	Updated to use new tag types and expose `DeadlineMonitorInner`
src/health_monitoring_lib/rust/common.rs	Removed `IdentTag`, `MonitorEvaluator`, and related types
src/health_monitoring_lib/cpp/include/score/hm/tag.h	New C++ tag types for FFI compatibility
src/health_monitoring_lib/cpp/include/score/hm/*.h	Updated to use new tag types
src/health_monitoring_lib/cpp/*.cpp	Updated implementations to use new tag types
src/health_monitoring_lib/Cargo.toml	Added feature flags for supervisor API client selection
Cargo.toml	Set default-members and removed global monitor_rs dependency
src/health_monitoring_lib/BUILD	Added crate_features configuration
.github/workflows/lint_clippy.yml	Updated to test both feature flag configurations
examples/*	Updated to use new tag types

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/health_monitoring_lib/rust/tag.rs

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/health_monitoring_lib/rust/lib.rs

src/health_monitoring_lib/rust/worker.rs

src/health_monitoring_lib/rust/supervisor_api_client/mod.rs

- Remove unnecessary abstraction from monitors. - Improve docs. - Move `FFIBorrowed` and `FFIHandle` to main `ffi` module. - Small esthetics fixes.

- Restore some parts. - Separate errors into groups.

- Add `HealthMonitorError`. - Not fully utilized in this change, required for future monitors. - Simplify implementation of `start` and `build`. - No longer must be reimplemented for FFI. - New unit tests for `lib.rs`.

pawelrutkaq

Cpp aligment in next PR ?

src/health_monitoring_lib/rust/deadline/deadline_monitor.rs

src/health_monitoring_lib/rust/common.rs

src/health_monitoring_lib/rust/lib.rs

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

src/health_monitoring_lib/rust/deadline/deadline_monitor.rs:276

This debug_assert!(snapshot.is_stopped(), "Deadline snapshot cannot be both running and stopped") is internally inconsistent: DeadlineStateSnapshot::set_running() does not clear the stopped bit (tests even assert both can be true), so the message is misleading and the assertion doesn't validate what it claims. Either fix the state flags to make running/stopped mutually exclusive, or update/remove this assertion/message to match the intended semantics.

                debug_assert!(
                    snapshot.is_stopped(),
                    "Deadline snapshot cannot be both running and stopped"
                );

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/health_monitoring_lib/rust/lib.rs

- Rename trait. - Fix docs. - Fix some behaviors.

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/health_monitoring_lib/rust/deadline/deadline_monitor.rs:276

The debug_assert! message says a snapshot "cannot be both running and stopped", but DeadlineStateSnapshot intentionally has is_stopped() remain true even when set_running() is called (see deadline_state tests). Updating the assertion/message to reflect the actual state semantics would avoid confusion when debugging.

                debug_assert!(
                    snapshot.is_stopped(),
                    "Deadline snapshot cannot be both running and stopped"
                );

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-02T13:51:12Z

src/health_monitoring_lib/rust/lib.rs

+        for (tag, monitor) in monitors_to_collect.iter_mut() {
            match monitor.take() {
-                Some(DeadlineMonitorState::Taken(handle)) => {
-                    if monitors.push(handle).is_err() {
-                        // Should not fail since we preallocated enough capacity
-                        return Err("Failed to push monitor handle".to_string());
+                Some(MonitorState::Taken(handle)) => {
+                    if collected_monitors.push(handle).is_err() {
+                        // Should not fail - capacity was preallocated.
+                        error!("Failed to push monitor handle.");
+                        return Err(HealthMonitorError::WrongState);
                    }
                },


collect_given_monitors mutates the map with take() as it iterates. If there are multiple monitors and the function returns early (e.g., due to an Available(_) entry), any previously-seen Taken(handle) entries will have been removed and replaced with None, making subsequent retries/start attempts fail with an invalid state. Consider a two-pass approach (first validate all entries are Taken without mutating, then collect) or otherwise ensure state is restored on early-return paths.

Copilot · 2026-03-02T13:51:13Z

src/health_monitoring_lib/rust/lib.rs

+        let supervisor_api_cycle_ms = self.supervisor_api_cycle.as_millis() as u64;
+        let internal_processing_cycle_ms = self.internal_processing_cycle.as_millis() as u64;
+        if !supervisor_api_cycle_ms.is_multiple_of(internal_processing_cycle_ms) {


Cycle validation casts Duration::as_millis() (u128) to u64, which can truncate for large durations and lead to incorrect validation. Consider keeping the values as u128 (or explicitly validating the cast) to avoid silent overflow/truncation.

Suggested change

let supervisor_api_cycle_ms = self.supervisor_api_cycle.as_millis() as u64;

let internal_processing_cycle_ms = self.internal_processing_cycle.as_millis() as u64;

if !supervisor_api_cycle_ms.is_multiple_of(internal_processing_cycle_ms) {

let supervisor_api_cycle_ms = self.supervisor_api_cycle.as_millis();

let internal_processing_cycle_ms = self.internal_processing_cycle.as_millis();

if supervisor_api_cycle_ms % internal_processing_cycle_ms != 0 {

arkjedrz had a problem deploying to workflow-approval February 17, 2026 11:51 — with GitHub Actions Error

arkjedrz had a problem deploying to workflow-approval February 17, 2026 11:51 — with GitHub Actions Failure

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from fda115c to 9e958f1 Compare February 17, 2026 12:22

arkjedrz had a problem deploying to workflow-approval February 17, 2026 12:22 — with GitHub Actions Failure

arkjedrz had a problem deploying to workflow-approval February 17, 2026 12:22 — with GitHub Actions Error

arkjedrz self-assigned this Feb 17, 2026

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from 9e958f1 to 88e93ee Compare February 23, 2026 13:09

arkjedrz temporarily deployed to workflow-approval February 23, 2026 13:09 — with GitHub Actions Inactive

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from 88e93ee to f08a948 Compare February 23, 2026 13:16

arkjedrz temporarily deployed to workflow-approval February 23, 2026 13:16 — with GitHub Actions Inactive

arkjedrz marked this pull request as ready for review February 23, 2026 13:44

arkjedrz requested review from Copilot and pawelrutkaq February 23, 2026 13:44

Copilot started reviewing on behalf of arkjedrz February 23, 2026 13:45 View session

Copilot AI reviewed Feb 23, 2026

View reviewed changes

src/health_monitoring_lib/rust/tag.rs Outdated Show resolved Hide resolved

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from f08a948 to df8135d Compare February 23, 2026 14:02

arkjedrz temporarily deployed to workflow-approval February 23, 2026 14:02 — with GitHub Actions Inactive

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from df8135d to 08e6e6d Compare February 23, 2026 15:52

arkjedrz temporarily deployed to workflow-approval February 23, 2026 15:52 — with GitHub Actions Inactive

arkjedrz commented Feb 23, 2026

View reviewed changes

src/health_monitoring_lib/rust/tag.rs Show resolved Hide resolved

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from 08e6e6d to 6afc701 Compare February 24, 2026 15:23

arkjedrz temporarily deployed to workflow-approval February 24, 2026 15:23 — with GitHub Actions Inactive

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from 6afc701 to 94920a7 Compare February 25, 2026 07:56

Copilot AI reviewed Feb 26, 2026

View reviewed changes

arkjedrz added 3 commits March 2, 2026 13:54

hmon: approach to monitors rework

3564562

- Remove unnecessary abstraction from monitors. - Improve docs. - Move `FFIBorrowed` and `FFIHandle` to main `ffi` module. - Small esthetics fixes.

hmon: approach to monitors rework

0cbe327

- Restore some parts. - Separate errors into groups.

hmon: rework behavior on panics

d00cc2e

- Add `HealthMonitorError`. - Not fully utilized in this change, required for future monitors. - Simplify implementation of `start` and `build`. - No longer must be reimplemented for FFI. - New unit tests for `lib.rs`.

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from aa819a8 to d00cc2e Compare March 2, 2026 12:55

arkjedrz temporarily deployed to workflow-approval March 2, 2026 12:55 — with GitHub Actions Inactive

pawelrutkaq requested changes Mar 2, 2026

View reviewed changes

arkjedrz temporarily deployed to workflow-approval March 2, 2026 13:25 — with GitHub Actions Inactive

arkjedrz requested review from Copilot and pawelrutkaq March 2, 2026 13:26

Copilot started reviewing on behalf of arkjedrz March 2, 2026 13:27 View session

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from 3aefb18 to c313b59 Compare March 2, 2026 13:29

arkjedrz had a problem deploying to workflow-approval March 2, 2026 13:29 — with GitHub Actions Error

arkjedrz had a problem deploying to workflow-approval March 2, 2026 13:29 — with GitHub Actions Failure

Copilot AI reviewed Mar 2, 2026

View reviewed changes

src/health_monitoring_lib/rust/lib.rs Show resolved Hide resolved

arkjedrz temporarily deployed to workflow-approval March 2, 2026 13:33 — with GitHub Actions Inactive

hmon: monitors rework post-review fixes

0eca8fb

- Rename trait. - Fix docs. - Fix some behaviors.

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from c313b59 to 0eca8fb Compare March 2, 2026 13:44

arkjedrz had a problem deploying to workflow-approval March 2, 2026 13:44 — with GitHub Actions Failure

arkjedrz had a problem deploying to workflow-approval March 2, 2026 13:44 — with GitHub Actions Error

arkjedrz requested a review from Copilot March 2, 2026 13:44

Copilot started reviewing on behalf of arkjedrz March 2, 2026 13:45 View session

arkjedrz temporarily deployed to workflow-approval March 2, 2026 13:47 — with GitHub Actions Inactive

Copilot AI reviewed Mar 2, 2026

View reviewed changes

pawelrutkaq approved these changes Mar 2, 2026

View reviewed changes

arkjedrz merged commit 7ef3ed8 into eclipse-score:main Mar 2, 2026
20 of 22 checks passed

arkjedrz deleted the arkjedrz_remove-monitor-abstraction branch March 2, 2026 14:03

Conversation

arkjedrz commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 17, 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 Feb 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pawelrutkaq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

arkjedrz commented Feb 17, 2026 •

edited

Loading

github-actions bot commented Feb 17, 2026 •

edited

Loading