impl: Fault Lib Initial Impl #4

vinodreddy-g · 2026-01-19T06:25:51Z

First implementation proposal for faultlib from Qorix.

Summary

Implementation proposal of fault lib .
No further work will be done on this PR by the original team.
If this should move forward, it needs a new owner/volunteer.

Checklist

I have tested my changes locally
I have added or updated documentation
I have linked related issues or discussions
I have added or updated tests

Notes for Reviewers

First implementation proposal for faultlib

mfaferek93

It's a solid early draft - the overall architecture makes sense :)

That said, there's still quite a bit of work needed before this can be merged:

Several critical issues (memory leaks, potential deadlocks, panic-happy error handling)
Many todo!() and unimplemented!() in production code paths
Mismatch between what's documented and what's actually implemented
Some design decisions that need team discussion (e.g. FaultCatalog location, debouncing scope, Weak vs Arc for global state)

As it stands, this PR needs a new owner to drive it to completion - the current state isn't mergeable but it's a great foundation to build on

mfaferek93 · 2026-01-26T09:07:13Z

src/common/src/sink_error.rs

+    BadDescriptor(&'static str),
+    #[error("other: {0}")]
+    Other(&'static str),


using &'static str forces callers to Box::leak dynamic strings, causing memory leaks
Suggestion: Change to Cow<'static, str> and remove Copy derive:

Suggested change

BadDescriptor(&'static str),

#[error("other: {0}")]

Other(&'static str),

BadDescriptor(Cow<'static, str>),

Other(Cow<'static, str>),

mfaferek93 · 2026-01-26T09:09:14Z

src/fault_lib/src/fault_manager_sink.rs

+impl From<FaultManagerError> for SinkError {
+    fn from(err: FaultManagerError) -> Self {
+        match err {
+            FaultManagerError::SendError(msg) => SinkError::Other(Box::leak(msg.into_boxed_str())),


Box::leak here causes memory leak on every SendError!
Fix: After changing SinkError to Cow (see my coomet in sink_error.rs):
FaultManagerError::SendError(msg) => SinkError::Other(Cow::Owned(msg))

mfaferek93 · 2026-01-26T09:09:49Z

src/fault_lib/src/fault_manager_sink.rs

+
+impl FaultManagerSink {
+    pub(crate) fn new() -> Self {
+        let (tx, rx) = mpsc::channel();


Unbounded channel - can grow forever if IPC is slow or DFM is down

mfaferek93 · 2026-01-26T09:10:31Z

src/dfm_lib/src/fault_lib_communicator.rs

+
+    pub fn run<S: SovdFaultStateStorage>(&self, mut processor: FaultRecordProcessor<S>) {
+        info!("FaultLibCommunicator listening...");
+        while self.node.wait(DIAGNOSTIC_FAULT_MANAGER_LISTENER_CYCLE_TIME).is_ok() {


This loop runs forever - no way to stop it gracefully.
when DiagnosticFaultManager is dropped, it will deadlock on join().

mfaferek93 · 2026-01-26T09:12:00Z

src/dfm_lib/src/diagnostic_fault_manager.rs

+    fn drop(&mut self) {
+        println!("Joining fault_lib_receiver_thread");
+        let handle = self.fault_lib_receiver_thread.take().unwrap();
+        if let Err(err) = handle.join() {


Drop tries to join the worker thread, but run() loops forever (see comment in fault_lib_communicator.rs). This will deadlock! Need a shutdown signal sent before join.

mfaferek93 · 2026-01-26T09:27:59Z

src/dfm_lib/examples/dfm.rs

+// Copyright (c) 2025 Contributors to the Eclipse Foundation
+//
+// See the NOTICE file(s) distributed with this work for additional
+// information regarding copyright ownership.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Apache License Version 2.0 which is available at
+// <https://www.apache.org/licenses/LICENSE-2.0>
+//
+// SPDX-License-Identifier: Apache-2.0
+//
+
+// Copyright (c) 2025 Contributors to the Eclipse Foundation
+//
+// See the NOTICE file(s) distributed with this work for additional
+// information regarding copyright ownership.
+//
+// This program and the accompanying materials are made available under the
+// terms of the Apache License Version 2.0 which is available at
+// <https://www.apache.org/licenses/LICENSE-2.0>
+//
+// SPDX-License-Identifier: Apache-2.0
+//


Duplicate copyright header

mfaferek93 · 2026-01-26T09:29:02Z

src/fault_lib/src/fault_manager_sink.rs

+    fn listen_hash_check_response(&self) -> Result<bool, SinkError> {
+        let start = Instant::now();
+        loop {
+            if let Some(msg) = self
+                .hash_check_response_subscriber
+                .as_ref()
+                .unwrap()
+                .receive()
+                .map_err(|_| SinkError::TransportDown)?
+            {
+                return Ok(*msg.payload());
+            }
+            if start.elapsed() >= TIMEOUT {
+                return Err(SinkError::Timeout);
+            }
+            std::thread::sleep(Duration::from_millis(10));
+        }
+    }
+}


Busy-wait with sleep(10ms) burns CPU. Could use waitset or condvar

mfaferek93 · 2026-01-26T09:30:12Z

src/dfm_lib/Cargo.toml

+[dependencies]
+env_logger.workspace = true
+common = { path = "../common" }
+fault_lib = { path = "../fault_lib" }


dfm_lib depends on fault_lib - this creates tight coupling. FaultCatalog and FaultCatalogBuilder should probably live in common crate, since both sides need them. Would allow cleaner separation.

mfaferek93 · 2026-01-26T09:31:29Z

docs/index.rst

+Use cases
+---------
+
+Following usecases are valid for the S-CORE application using the Fault Library:
+
+- registering new fault in the system
+   - depending on car configuration variant, enabled features etc. the number of faults detected and reported by the app can change
+   - depending on the current status and state of the car electronic system the APP can report different faults   
+- configuring debouncing for the fault
+   - different test can require the results to be filtered over time or debounced, to prevent setting the faults by glitches or false positives 
+- configuring enabling conditions for the fault
+   - each test can require different system conditions to be fulfilled before the test can be performed (e.g. the communication test can be done only if the power supply is in expected range)
+- reporting results of diagnostic tests (fail / pass)
+- reporting status of enabling conditions (if done in the app)
+   - the application can report only status on the enabling condition and does not report any faults 
+- react to the SOVD Fault Handling actions (e.g. delete faults can cause the test to restart)
+- react to change in the enabling conditions (some tests could be impossible to be process when enabling conditions are not fulfilled)
+- provide interface to the user which allow to provide additional environmental data to be stored with the fault 
+
+Following usecases applies for the Fault Library (FL) and Diagnostic Fault Manager (DFM):
+
+- validate the consistency of the fault catalog shared between DFM and FL 
+- DFM maintain global fault catalog based on the information from each FL
+- FL reports state changes in the faults to DFM over IPC
+- FL reports enabling condition state change to the DFM over IPC
+- DFM reports over IPC to FL enabling condition state change reported by another FL
+- DFM requests restart of the test for the faults reported by FL
+- DFM reports cleaning of the faults in the DFM by the SOVD client
+- DFM receives and maintain current status of the environment conditions to be stored together with faults
+


These use cases are documented but NOT implemented yet:

enabling conditions (subscribe, notify, local/global)

dynamic fault registration ("registering new fault in the system")

reset policies / debouncing on manager side

react to SOVD Fault Handling actions
Consider adding a "MVP Scope" section listing what's actually implemented vs planned.

mfaferek93 · 2026-01-26T09:32:40Z

src/common/src/fault.rs

+pub struct FaultDescriptor {
+    pub id: FaultId,
+
+    pub name: ShortString,
+    pub summary: Option<LongString>,
+
+    pub category: FaultType,
+    pub severity: FaultSeverity,
+    pub compliance: ComplianceVec,
+
+    pub reporter_side_debounce: Option<DebounceMode>,
+    pub reporter_side_reset: Option<ResetPolicy>,
+    pub manager_side_debounce: Option<DebounceMode>,
+    pub manager_side_reset: Option<ResetPolicy>,
+}


Fields:

reporter_side_debounce reporter_side_reset manager_side_debounce manager_side_reset

are defined but none of them are actually used in the code.
Either implement debouncing/reset logic or document these as "reserved for future".

lh-sag · 2026-01-26T09:46:57Z

It's a solid early draft - the overall architecture makes sense :)

That said, there's still quite a bit of work needed before this can be merged:

Several critical issues (memory leaks, potential deadlocks, panic-happy error handling)

Many todo!() and unimplemented!() in production code paths

Mismatch between what's documented and what's actually implemented

Some design decisions that need team discussion (e.g. FaultCatalog location, debouncing scope, Weak vs Arc for global state)

As it stands, this PR needs a new owner to drive it to completion - the current state isn't mergeable but it's a great foundation to build on

@mfaferek93 Would you take over this task?

mfaferek93 · 2026-01-26T09:57:06Z

@lh-sag I need to review the priorities and check internally with 42dot team, there’s a chance @bburda42dot will handle this on our side. If it ends up being on me, I’ll work on it in my free time, so it may take a bit longer.
I’ll get back to you later this week with an update.

IoanAndrasi · 2026-01-26T11:53:40Z

My overall review:

Good architecture that matches design documentation in most of the cases
Well organized and separations between libs
Trait-based abstraction offering clean separation
Still couple of TODO's but I think its sufficient for initial implementation

IoanAndrasi · 2026-01-23T11:14:09Z

src/dfm_lib/src/sovd_fault_manager.rs

+}
+
+#[derive(Clone, Default, Debug, PartialEq, Eq)]
+pub struct SovdFault {


I notice that three fields are missing, symptom, symptom_translation_id and schema

IoanAndrasi · 2026-01-27T08:27:29Z

src/dfm_lib/src/diagnostic_fault_manager.rs

+impl<S: SovdFaultStateStorage> Drop for DiagnosticFaultManager<S> {
+    fn drop(&mut self) {
+        println!("Joining fault_lib_receiver_thread");
+        let handle = self.fault_lib_receiver_thread.take().unwrap();


unwrap on take will panic if called twice

IoanAndrasi · 2026-01-27T09:01:22Z

src/fault_lib/src/fault_manager_sink.rs

+/// Diagnostic Fault Manager. This trait shall never become public
+impl FaultSinkApi for FaultManagerSink {
+    fn publish(&self, path: &str, record: FaultRecord) -> Result<(), SinkError> {
+        let event = DiagnosticEvent::Fault((to_static_long_string(path).expect("Failed to serialize path"), record));


path strings are converted to LongString without proper validation

impl: Fault Lib Initial Impl

ee95af0

First implementation proposal for faultlib

vinodreddy-g requested a review from a team as a code owner January 19, 2026 06:25

vinodreddy-g marked this pull request as draft January 19, 2026 07:07

mfaferek93 reviewed Jan 26, 2026

View reviewed changes

IoanAndrasi reviewed Jan 27, 2026

View reviewed changes

impl: Fault Lib Initial Impl #4

Are you sure you want to change the base?

impl: Fault Lib Initial Impl #4

Uh oh!

Conversation

vinodreddy-g commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Related

Notes for Reviewers

Uh oh!

mfaferek93 left a comment • 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!

lh-sag commented Jan 26, 2026

Uh oh!

mfaferek93 commented Jan 26, 2026

Uh oh!

IoanAndrasi commented Jan 26, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vinodreddy-g commented Jan 19, 2026 •

edited

Loading

mfaferek93 left a comment •

edited

Loading