Skip to content

Fix incorrect behaviour on early termination - #576

Open
WilliamRoebuck wants to merge 10 commits into
eclipse-score:mainfrom
etas-contrib:fix/issue-553
Open

Fix incorrect behaviour on early termination#576
WilliamRoebuck wants to merge 10 commits into
eclipse-score:mainfrom
etas-contrib:fix/issue-553

Conversation

@WilliamRoebuck

@WilliamRoebuck WilliamRoebuck commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Most likely does not \fix\ #567 but I have not been able to reproduce that failure.

Fix details

The case where the process terminated during activation was not handled correctly. The main fix is at the end of startProcess(), where we now handle the stored result of the last termination instead of ignoring when a termination happened and hoping that tryHandleTermination() happened at just the right time to report an unexpected termination.

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

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.7.0) and connecting to it...
INFO: Invocation ID: 2ca1f857-eaf2-4a4a-88b7-de585806433b
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: 
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 (48 packages loaded, 10 targets configured)

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

Analyzing: target //:license-check (131 packages loaded, 404 targets configured)

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

Analyzing: target //:license-check (157 packages loaded, 8986 targets configured)

Analyzing: target //:license-check (163 packages loaded, 9368 targets configured)

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

Analyzing: target //:license-check (169 packages loaded, 9541 targets configured)

INFO: Analyzed target //:license-check (173 packages loaded, 11555 targets configured).
[15 / 16] [Prepa] Generating Dash formatted dependency file ...
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: 23.529s, Critical Path: 0.38s
INFO: 16 processes: 4 disk cache hit, 12 internal.
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>]

@WilliamRoebuck
WilliamRoebuck marked this pull request as ready for review September 3, 2026 15:04
bool terminationIsValid(int32_t exit_code) const;

/// @brief Returns true if the process is configured to report kRunning
bool isReporting() const;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we should have this as a member as it's a copy the data in the config, should just add
auto& is_reporting = to the methods that use it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I don't think it can be considered a copy because the config doesn't directly provide whether the component is reporting or not. At the moment is_reporting = !config::Native, but that might not always be the case. I prefer the function because otherwise we repeat logic in several places, even if the logic is just !=

return res;
}

bool ProcessInfoNode::terminationIsValid(int32_t exit_code) const

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only used in tryHandleTermination should this just be a lambda?

@WilliamRoebuck WilliamRoebuck Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in 3158c76, no lambda needed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

2 participants