Skip to content

Error propagation and handling#24

Merged
armin-acn merged 7 commits intoeclipse-score:mainfrom
Valeo-S-CORE-Organization:error_propagation
Dec 8, 2025
Merged

Error propagation and handling#24
armin-acn merged 7 commits intoeclipse-score:mainfrom
Valeo-S-CORE-Organization:error_propagation

Conversation

@ShoroukRamzy
Copy link
Contributor

@ShoroukRamzy ShoroukRamzy commented Nov 26, 2025

This PR resolves issue #23 by implementing a robust error handling mechanism for activity failures.

Design diagrams illustrating the new failure handling sequences have been added to the feo/Design directory. A comprehensive guide for verifying this feature is available in the new TESTING.md in examples/rust/mini-adas

Proposal:

Currently the implementation handles step and shutdown errors as following:

  1. Modify Activity Trait: Change step() and shutdown() to return a Result<(), ActivityError>.

  2. Update Worker: The worker will catch any Err and send a failure signal to the Scheduler.

  3. Enhance Scheduler: The scheduler will handle failures as following:

    • On Step Failure: Initiate a graceful shutdown of the entire system.
    • On Shutdown Failure: Log the specific activity's failure but continue to shut down all other activities to prevent the system from hanging.

Thanks!

@github-actions
Copy link

github-actions bot commented Nov 26, 2025

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]
Starting local Bazel server (8.3.0) and connecting to it...
INFO: Invocation ID: ee91d500-97fb-4b9d-a5af-3059a70003f6
Computing main repo mapping: 
Computing main repo mapping: 
DEBUG: Rule 'rules_boost+' indicated that a canonical reproducible form can be obtained by modifying arguments integrity = "sha256-pZY3kJDKUFN829ZdmWOrWu/MhcmnwKJyhAsIfTbfVPU="
DEBUG: Repository rules_boost+ instantiated at:
  <builtin>: in <toplevel>
Repository rule http_archive defined at:
  /home/runner/.bazel/external/bazel_tools/tools/build_defs/repo/http.bzl:394:31: in <toplevel>
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'rules_cc', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'googletest', the root module requires module version [email protected], but got [email protected] in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
WARNING: For repository 'rules_python', the root module requires module version [email protected], but got [email protected] 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: 4 packages loaded
Loading: 4 packages loaded
    currently loading: 
Analyzing: target //:license-check (5 packages loaded, 0 targets configured)
Analyzing: target //:license-check (5 packages loaded, 0 targets configured)

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

Analyzing: target //:license-check (150 packages loaded, 1885 targets configured)

Analyzing: target //:license-check (156 packages loaded, 7055 targets configured)

Analyzing: target //:license-check (156 packages loaded, 7055 targets configured)

INFO: Analyzed target //:license-check (159 packages loaded, 9071 targets configured).
[7 / 13] Creating runfiles tree bazel-out/k8-opt-exec-ST-d57f47055a04/bin/external/score_tooling+/dash/tool/formatters/dash_format_converter.runfiles [for tool]; 0s local
INFO: From Generating Dash formatted dependency file ...:
INFO: Successfully converted 2 packages from Cargo.lock to bazel-out/k8-fastbuild/bin/formatted.txt
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: 15.494s, Critical Path: 0.75s
INFO: 13 processes: 3 disk cache hit, 9 internal, 1 processwrapper-sandbox.
INFO: Build completed successfully, 13 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
Copy link

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

Copy link
Contributor

@masc2023 masc2023 Nov 26, 2025

Choose a reason for hiding this comment

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

Please follow for documentation the S-CORE process guidelines, address requirements, architecture changes in the according folders, if would also be from advantage, to use Change Management Issue to track the topics accordingly

Currently the documentation is still in score repo, but as part of next release, that shall be moved her and can be properly integrated, compare https://github.com/orgs/eclipse-score/discussions/516#discussioncomment-15039732

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @masc2023,

Thank you for your feedback. I will review your concerns. please note that I am not trying to change the architecture or requirements, I already implement these existing requirements Activity stepping error and Activity shutdown error

At this time, I will proceed with removing the previously included design sequence diagrams. Concurrently, I will examine the process guidelines to ensure that all future contributions to the design documentation are submitted through the appropriate channels and in the correct format.

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @masc2023,

you mean these architecture docs https://eclipse-score.github.io/reference_integration/main/_collections/score_process/process/process_areas/index.html, is it right?

No, these are only the general process guidelines, the real documentation for the FEO Module are here: https://eclipse-score.github.io/score/main/modules/feo/index.html

FEO Component architecture are defined in this folder, https://eclipse-score.github.io/score/main/modules/feo/feo/docs/architecture/component_architecture.html

These should now be transferred completely to the FEO Module Folder and that would be then the right place to add your diagram and link it properly to the requirements.

There are also Feature High Level requirements, but they should be reflected on the component level, too.
https://eclipse-score.github.io/score/main/features/frameworks/feo/architecture/feature_architecture.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@masc2023, I removed the sequence diagrams. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @masc2023,
you mean these architecture docs https://eclipse-score.github.io/reference_integration/main/_collections/score_process/process/process_areas/index.html, is it right?

No, these are only the general process guidelines, the real documentation for the FEO Module are here: https://eclipse-score.github.io/score/main/modules/feo/index.html

FEO Component architecture are defined in this folder, https://eclipse-score.github.io/score/main/modules/feo/feo/docs/architecture/component_architecture.html

These should now be transferred completely to the FEO Module Folder and that would be then the right place to add your diagram and link it properly to the requirements.

There are also Feature High Level requirements, but they should be reflected on the component level, too. https://eclipse-score.github.io/score/main/features/frameworks/feo/architecture/feature_architecture.html

Okay great, Thank you for the clarification @masc2023!

@ShoroukRamzy
Copy link
Contributor Author

ShoroukRamzy commented Nov 26, 2025

Hi @masc2023, I removed the design sequence diagrams to be added in another PR in the relevant docs as we discussed.
Could you please let me know your feedbacks and help me merge this PR as I don't have permissions to merge?
Thanks!

@ShoroukRamzy
Copy link
Contributor Author

Hi @armin-acn,
Could you please review this PR when you have time and help me merge it if possible? Thanks!

Copy link
Contributor

@armin-acn armin-acn left a comment

Choose a reason for hiding this comment

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

Thanks for the nice PR! I would like to ask for two small changes but will also ask @artemsheinacn to take a look.

* **Relayed TCP Mode (Default):**
```sh
# Terminal 1 (Primary Agent)
FAIL_STEP_AFTER=3 cargo run -p mini-adas --features test-error-injection --bin adas_primary -- 400
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be changed to call bazel instead of cargo, because cargo builds are no longer supported.

@armin-acn armin-acn requested a review from masc2023 December 8, 2025 09:56
@artemshein
Copy link

Hi @ShoroukRamzy, @armin-acn, in my view we don't put tests into mini-adas. We have a separate tests agent in one of our internal PRs and a tests scripts runner. Otherwise the PR looks good to me.

@ShoroukRamzy
Copy link
Contributor Author

Hi @artemshein,
Thank you for your feedback, I removed the testing doc from mini-adas .

@ShoroukRamzy
Copy link
Contributor Author

Hi @armin-acn, if you don't have any further comments, Could you please help me merge this PR?

@armin-acn
Copy link
Contributor

Hi @ShoroukRamzy, please do not only remove the test documentation, but also the test code itself (error injection) from the commit. As @artemshein mentioned, we have an internal PR that will be published soon, that is intended to provide a very similar functionality but in a more generic approach.
We would be happy to receive your feedback on that PR once it is there.

@ShoroukRamzy
Copy link
Contributor Author

Hi @ShoroukRamzy, please do not only remove the test documentation, but also the test code itself (error injection) from the commit. As @artemshein mentioned, we have an internal PR that will be published soon, that is intended to provide a very similar functionality but in a more generic approach. We would be happy to receive your feedback on that PR once it is there.

Hi @armin-acn, thank you for the clarification. I removed all test code I added.

@ShoroukRamzy
Copy link
Contributor Author

Hi @ShoroukRamzy, please do not only remove the test documentation, but also the test code itself (error injection) from the commit. As @artemshein mentioned, we have an internal PR that will be published soon, that is intended to provide a very similar functionality but in a more generic approach. We would be happy to receive your feedback on that PR once it is there.

Hi @armin-acn, thank you for the clarification. I removed all test code I added.

Hi @armin-acn, I don't have sufficient permissions to merge this PR, Could you please help me merge it if it's ready? Thanks!

@armin-acn armin-acn merged commit fc64af2 into eclipse-score:main Dec 8, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants